Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1660452

Re: [PATCH v5] fpga manager: Add Altera CvP driver

Show all headers | View raw

On Thu, Jun 8, 2017 at 2:09 AM, Anatolij Gustschin <agust@denx.de> wrote:
> On Fri, 2 Jun 2017 20:43:21 +0300
> Andy Shevchenko andy.shevchenko@gmail.com wrote:

Besides below comments, please, do

s/VSEC_/VSE_/g

for entire file.

We are following PCI and Thunderbolt pattern for use of Vendor
Specific Extended Capability.

>>> +       if (!timeout_us)
>>> +               return -ETIMEDOUT;
>>
>>Hmm...
>>What as a user I would expect here is at least one attempt (0 -- no
>>timeout, but try once).
>
> yes, this first attempt is above, please see original patch for full
> context.

Ah, it means you don't correctly use do {} while approach.

Remove everything above do { and move usleep after check for the
status inside the loop.

>>> +       /* clock to data ratio for uncompressed and unencrypted images */
>>> +       conf->numclks = 1;
>>
>>To else branch of below?
>>
>>> +       if (info) {
>>
>>> +       }
>
> then, the default numclks value would have to be set in the if branch
> as well. It is easier to set it before.

See the magic below:

        u32 iflags = info ? info->flags : 0;
...
        if (iflags & FPGA_MGR_PARTIAL_RECONFIG) {
                 dev_err(&mgr->dev, "Partial reconfiguration not supported.\n");
                 return -EINVAL;
        }

        if (iflags & FPGA_MGR_COMPRESSED_BITSTREAM)
                conf->numclks = 8; /* ratio for all compressed images */
        else if (iflags & FPGA_MGR_ENCRYPTED_BITSTREAM)
                conf->numclks = 4; /* ratio for encrypted images */
        else
                conf->numclks = 1; /* clock to data ratio for
uncompressed and unencrypted images */

I would really recommend to double check the code every time you are
about to send.
A little thought can make a beauteful result!

>>> +static struct pci_device_id altera_cvp_id_tbl[] = {
>>> +       { PCI_VDEVICE(ALTERA, PCI_ANY_ID) },
>>
>>Does it have dedicated PCI class?
>>
>>PCI_ANY_ID usually is too broad.
>
> no, it doesn't have dedicated class.

Hmm... It means any device of this vendor will jump into this
driver... Not good.

-- 
With Best Regards,
Andy Shevchenko

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

Re: [PATCH v5] fpga manager: Add Altera CvP driver Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-06-02 19:50 +0200
  Re: [PATCH v5] fpga manager: Add Altera CvP driver Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-06-02 19:50 +0200
  Re: [PATCH v5] fpga manager: Add Altera CvP driver Anatolij Gustschin <agust@denx.de> - 2017-06-08 01:10 +0200
    Re: [PATCH v5] fpga manager: Add Altera CvP driver Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-06-08 01:40 +0200
      Re: [PATCH v5] fpga manager: Add Altera CvP driver Anatolij Gustschin <agust@denx.de> - 2017-06-08 16:20 +0200
        Re: [PATCH v5] fpga manager: Add Altera CvP driver Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-06-08 16:50 +0200
          Re: [PATCH v5] fpga manager: Add Altera CvP driver Anatolij Gustschin <agust@denx.de> - 2017-06-08 17:10 +0200

csiph-web