Re: [PATCH v1 1/2] platform/x86: intel_pmc_ipc: Use MFD framework to create dependent devices

[Andy Shevchenko]

On Fri, Jan 26, 2018 at 12:53 AM,
<sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx> wrote:
> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx>
>
> Currently, we have lot of repetitive code in dependent device resource
> allocation and device creation handling code. This logic can be improved if
> we use MFD framework for dependent device creation. This patch adds this
> support.

Thanks for an update. My comments below.

> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx>

> Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>

First of all, I barely remember what I did to this patch. In any case
this one is redundant since it will have mine when I push it to our
repo.

> @@ -508,7 +492,7 @@ static int ipc_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>         ret = devm_request_irq(&pdev->dev, pdev->irq, ioc, 0, "intel_pmc_ipc",
>                                 pmc);
>         if (ret) {
> -               dev_err(&pdev->dev, "Failed to request irq\n");
> +               dev_err(&pdev->dev, "Failed to request IRQ\n");
>                 return ret;
>         }

Split this kind of changes in a separate patch.

> +static int ipc_create_punit_device(struct platform_device *pdev)
>  {
> +       static struct resource punit_res[PLAT_RESOURCE_MEM_MAX_INDEX];
> +       static struct mfd_cell punit_cell;
> +       struct resource *res;
> +       int ret, mindex, pindex = 0;
> +
> +       for (mindex = 0; mindex <= PLAT_RESOURCE_MEM_MAX_INDEX; mindex++) {

'<=' ??? (Why = is here?)

> +               res = platform_get_resource(pdev, IORESOURCE_MEM, mindex);
> +
> +               switch (mindex) {
> +               /* Get PUNIT resources */
> +               case PLAT_RESOURCE_BIOS_DATA_INDEX:
> +               case PLAT_RESOURCE_BIOS_IFACE_INDEX:

> +                       /* BIOS resources are required, so return error if not
> +                        * available
> +                        */

It's not the network subsystem, please, do a proper style for
multi-line comments.

> +                       if (!res) {

Would the following work for you?

if (res)
 break;
     dev_err(&pdev->dev, "Failed to get PUNIT MEM resource %d\n", pindex);
     return -ENXIO;
case ...:
...
if (res)
 break;
default:
 continue;

memcpy(...);
...

> +                               dev_err(&pdev->dev,
> +                                       "Failed to get PUNIT MEM resource %d\n",
> +                                       pindex);
> +                               return -ENXIO;
> +                       }
> +               case PLAT_RESOURCE_ISP_DATA_INDEX:
> +               case PLAT_RESOURCE_ISP_IFACE_INDEX:
> +               case PLAT_RESOURCE_GTD_DATA_INDEX:
> +               case PLAT_RESOURCE_GTD_IFACE_INDEX:
> +                       /* if valid resource found, copy the resource to PUNIT
> +                        * resource
> +                        */
> +                       if (res)
> +                               memcpy(&punit_res[pindex], res, sizeof(*res));
> +                       punit_res[pindex].flags = IORESOURCE_MEM;
> +                       pindex++;
> +                       break;
>                 };
> +       }

> +       ret = devm_mfd_add_devices(&pdev->dev, PLATFORM_DEVID_NONE, &punit_cell,
> +                                  1, NULL, 0, NULL);
>
> +       dev_dbg(&pdev->dev, "Successfully created PUNIT device\n");
>

Wrong. If ret is not 0 the message is misleading.
Just remove it.

Same for the rest cases.

> +       return ret;
>  }

> +static int ipc_create_wdt_device(struct platform_device *pdev)
>  {
> +       static struct resource wdt_ipc_res[2];
> +       static struct mfd_cell wdt_cell;
>         struct resource *res;
> +       int ret;
>
> +       /* If we have ACPI based watchdog use that instead, othewise create
> +        * a MFD cell for iTCO watchdog
> +        */

Style.

> +       if (acpi_has_watchdog())
> +               return 0;

> +       ret = devm_mfd_add_devices(&pdev->dev, PLATFORM_DEVID_NONE, &wdt_cell,
> +                                  1, NULL, 0, NULL);
> +
> +       dev_dbg(&pdev->dev, "Successfully created watchdog device\n");
> +
> +       return ret;

What Heikki meant is to fill cells by those helper functions and call
mfd_add_devices() only once.

See lpc_ich.c as an example.

>  }

> +static int ipc_create_pmc_devices(struct platform_device *pdev)
>  {
>         int ret;
>
> +       ret = ipc_create_punit_device(pdev);
> +       if (ret < 0)
> +               return ret;

Is it fatal? (Hint: it's quite likely not)

> +       ret = ipc_create_wdt_device(pdev);
> +       if (ret < 0)
> +               return ret;

Is it fatal?

> +       ret = ipc_create_telemetry_device(pdev);
> +       if (ret < 0)
> +               return ret;

Is it fatal?

> +       return 0;
>  }

> +       ret = devm_request_irq(&pdev->dev, ipcdev.irq, ioc, IRQF_NO_SUSPEND,
> +                              "intel_pmc_ipc", &ipcdev);
> +       if (ret) {
> +               dev_err(&pdev->dev, "Failed to request IRQ\n");
> +               return ret;
>         }
>
>         ret = sysfs_create_group(&pdev->dev.kobj, &intel_ipc_group);
>         if (ret) {
>                 dev_err(&pdev->dev, "Failed to create sysfs group %d\n",
>                         ret);
> -               goto err_sys;

> +               devm_free_irq(&pdev->dev, ipcdev.irq, &ipcdev);

Why do you need this one?

> +               return ret;
>         }
>
>         ipcdev.has_gcr_regs = true;
>
>         return 0;
>  }

And to the main question, what this is doing in PDx86 now? There
should be a patch to move it under drivers/mfd.

In _any case_ I need an Ack from Lee.

-- 
With Best Regards,
Andy Shevchenko