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

[Andy Shevchenko]

On Sun, May 14, 2017 at 6:51 PM, Anatolij Gustschin <agust@xxxxxxx> wrote:
> Add FPGA manager driver for loading Arria-V/Cyclone-V/Stratix-V
> and Arria-10 FPGAs via CvP.

Few comments from me.

> +struct altera_cvp_conf {
> +       struct fpga_manager     *mgr;
> +       struct pci_dev          *pci_dev;
> +       void __iomem            *map;

> +       void                    (*write_data)(struct altera_cvp_conf *conf,
> +                                             u32 val);

Is it too far beyond 80 characters? I would leave it in one line (~83
characters are okay).

> +       char                    mgr_name[64];
> +       u8                      numclks;
> +};
> +static void altera_cvp_dummy_write(struct altera_cvp_conf *conf)
> +{
> +       unsigned int i;
> +       u32 val32;

Drop the useless suffix.

> +}

> +static int altera_cvp_wait_status(struct altera_cvp_conf *conf, u32 status_msk,
> +                                 u32 status_val, int timeout_us)
> +{
> +       u32 val32;

Ditto.

> +       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).

> +
> +       do {
> +               /* use small usleep value to re-check and break early */
> +               usleep_range(10, 11);
> +
> +               pci_read_config_dword(conf->pci_dev, VSEC_CVP_STATUS, &val32);
> +               if ((val32 & status_msk) == status_val)
> +                       return 0;
> +
> +               timeout_us -= 10;
> +       } while (timeout_us > 0);
> +
> +       return -ETIMEDOUT;
> +}

> +static int altera_cvp_teardown(struct fpga_manager *mgr,
> +                              struct fpga_image_info *info)
> +{

> +       u32 val32;

Drop the suffix.

> +       return ret;
> +}

> +static int altera_cvp_write_init(struct fpga_manager *mgr,
> +                                struct fpga_image_info *info,
> +                                const char *buf, size_t count)
> +{

> +       u32 val32;

Ditto.

> +       int ret;
> +

> +       /* clock to data ratio for uncompressed and unencrypted images */
> +       conf->numclks = 1;

To else branch of below?

> +       if (info) {

> +       }

> +               ret = altera_cvp_teardown(mgr, info);
> +               if (ret < 0)
> +                       return ret;

What is the meaning of ret > 0?

Can't it be just if (ret) here?


> +       ret = altera_cvp_wait_status(conf, VSEC_CVP_STATUS_CFG_RDY,
> +                                    VSEC_CVP_STATUS_CFG_RDY, 10);
> +       if (ret < 0) {
> +               dev_warn(&mgr->dev, "CFG_RDY == 1 timeout\n");
> +               return ret;
> +       }

Ditto.

Also check another code like this above and below.

> +       return 0;
> +}

> +static inline int altera_cvp_chk_error(struct fpga_manager *mgr, size_t bytes)
> +{
> +       struct altera_cvp_conf *conf = mgr->priv;
> +       u32 val32;

Suffix.

> +}

> +static int altera_cvp_write_complete(struct fpga_manager *mgr,
> +                                    struct fpga_image_info *info)
> +{

> +       u32 status_msk;

status_mask

> +       u32 val32;

Drop the suffix.

> +}

> +static ssize_t show_chkcfg(struct device_driver *dev, char *buf)
> +{
> +       return snprintf(buf, 3, "%d\n", altera_cvp_chkcfg ? 1 : 0);

Just altera_cvp_chkcfg.

> +}

> +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.

> +static int altera_cvp_probe(struct pci_dev *pdev,
> +                           const struct pci_device_id *dev_id)
> +{
> +       struct altera_cvp_conf *conf;
> +       u16 cmd, val16;

Drop the suffix.

> +       /*
> +        * First check if this is the expected FPGA device. PCI config
> +        * space access works without enabling the pci device, memory

pci -> PCI

> +        * space access is enabled further down.
> +        */

> +       /*
> +        * Enable memory BAR access. We cannot use pci_enable_device() here
> +        * because it will make the driver unusable with FPGA devices that
> +        * have additional big iomem resources (e.g. 4GiB BARs) on 32-bit

iomem -> IOMEM

> +        * platform. Such BARs will not have an assigned address range and
> +        * pci_enable_device() will fail, complaining about not claimed BAR,
> +        * even if the concerned BAR is not needed for FPGA configuration

> +        * at all. Thus, enable the device via pci config space command.

pci -> PCI

> +        */

> +       ret = pci_request_region(pdev, CVP_BAR, "CVP");
> +       if (ret < 0) {

if (ret)

> +               dev_err(&pdev->dev, "Requesting CVP BAR region failed\n");
> +               goto err;
> +       }

> +       snprintf(conf->mgr_name, sizeof(conf->mgr_name), "%s @%02x:%02x.%d",
> +                ALTERA_CVP_MGR_NAME,

pdev->bus->number,
> +                PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));

pci_name() ?

> +err_unmap:
> +       pci_iounmap(pdev, conf->map);
> +       pci_release_region(pdev, CVP_BAR);

> +err:

err_disable:

> +       cmd &= ~PCI_COMMAND_MEMORY;
> +       pci_write_config_word(pdev, PCI_COMMAND, cmd);
> +       return ret;
> +}

-- 
With Best Regards,
Andy Shevchenko