Re: [PATCH] platform:x86 decouple telemetry driver from the optional IPC resources

["Li, Aubrey" <aubrey.li@linux.intel.com>]

On 2016/4/10 21:17, Andy Shevchenko wrote:
> On Thu, Mar 31, 2016 at 10:28 PM, Aubrey Li <aubrey.li@linux.intel.com> wrote:
>> Currently the optional IPC resources prevent telemetry driver from
>> probing if these resources are not in ACPI table. This patch decouples
>> telemetry driver from these optional resources, so that telemetry driver
>> has dependency only on the necessary ACPI resources.
> 
> Darren, I have comments as well.
> 
>>
>> Signed-off-by: Aubrey Li <aubrey.li@linux.intel.com>
>> ---
>>  drivers/platform/x86/intel_pmc_ipc.c   |   48 +++++++++++++++-----------------
>>  drivers/platform/x86/intel_punit_ipc.c |   48 +++++++++++++++++++++-----------
>>  2 files changed, 54 insertions(+), 42 deletions(-)
>>
>> diff --git a/drivers/platform/x86/intel_pmc_ipc.c b/drivers/platform/x86/intel_pmc_ipc.c
>> index 092519e..29d9c02 100644
>> --- a/drivers/platform/x86/intel_pmc_ipc.c
>> +++ b/drivers/platform/x86/intel_pmc_ipc.c
>> @@ -686,8 +686,8 @@ static int ipc_plat_get_res(struct platform_device *pdev)
>>         ipcdev.acpi_io_size = size;
>>         dev_info(&pdev->dev, "io res: %pR\n", res);
>>
>> -       /* This is index 0 to cover BIOS data register */
>>         punit_res = punit_res_array;
>> +       /* This is index 0 to cover BIOS data register */
>>         res = platform_get_resource(pdev, IORESOURCE_MEM,
>>                                     PLAT_RESOURCE_BIOS_DATA_INDEX);
>>         if (!res) {
>> @@ -697,55 +697,51 @@ static int ipc_plat_get_res(struct platform_device *pdev)
>>         *punit_res = *res;
>>         dev_info(&pdev->dev, "punit BIOS data res: %pR\n", res);
>>
>> +       /* This is index 1 to cover BIOS interface register */
>>         res = platform_get_resource(pdev, IORESOURCE_MEM,
>>                                     PLAT_RESOURCE_BIOS_IFACE_INDEX);
>>         if (!res) {
>>                 dev_err(&pdev->dev, "Failed to get res of punit BIOS iface\n");
>>                 return -ENXIO;
>>         }
>> -       /* This is index 1 to cover BIOS interface register */
>>         *++punit_res = *res;
>>         dev_info(&pdev->dev, "punit BIOS interface res: %pR\n", res);
>>
>> +       /* This is index 2 to cover ISP data register, optional */
> 
> All above looks like a commentary fixes (except an additional
> 'optional' word in one case). Can you do this separately?

I don't think it's necessary.

> 
> 
>>         res = platform_get_resource(pdev, IORESOURCE_MEM,
>>                                     PLAT_RESOURCE_ISP_DATA_INDEX);
>> -       if (!res) {
>> -               dev_err(&pdev->dev, "Failed to get res of punit ISP data\n");
>> -               return -ENXIO;
>> +       ++punit_res;
>> +       if (res) {
>> +               *punit_res = *res;
>> +               dev_info(&pdev->dev, "punit ISP data res: %pR\n", res);
> 
> Okay, what if you re-arrange this to some helper first
> 

Thanks for the idea, but I don't like a helper here, did you see
anything harmful of the current implementation?

> int …_assign_res(*pdev, index, *punit_res)
> {
>  struct resource res;
>  res = platform_get_resource(pdev, …, index);
>  if (!res)
>   return -ERRNO;
>  *punit_res = *res;
>  dev_dbg(%pR);
>  return 0;
> }
> 
> In this patch you move to optional by
> dev_err -> dev_warn
> 
> and use
> 
> if (ret)
>   dev_warn( "…skip  optional resource…" );
> 
> instead of
> if (ret) {
>  dev_err();
>  return ret;
> }
> 
>>         }
>> -       /* This is index 2 to cover ISP data register */
>> -       *++punit_res = *res;
>> -       dev_info(&pdev->dev, "punit ISP data res: %pR\n", res);
>>
>> +       /* This is index 3 to cover ISP interface register, optional */
>>         res = platform_get_resource(pdev, IORESOURCE_MEM,
>>                                     PLAT_RESOURCE_ISP_IFACE_INDEX);
>> -       if (!res) {
>> -               dev_err(&pdev->dev, "Failed to get res of punit ISP iface\n");
>> -               return -ENXIO;
>> +       ++punit_res;
>> +       if (res) {
>> +               *punit_res = *res;
>> +               dev_info(&pdev->dev, "punit ISP interface res: %pR\n", res);
>>         }
>> -       /* This is index 3 to cover ISP interface register */
>> -       *++punit_res = *res;
>> -       dev_info(&pdev->dev, "punit ISP interface res: %pR\n", res);
>>
>> +       /* This is index 4 to cover GTD data register, optional */
>>         res = platform_get_resource(pdev, IORESOURCE_MEM,
>>                                     PLAT_RESOURCE_GTD_DATA_INDEX);
>> -       if (!res) {
>> -               dev_err(&pdev->dev, "Failed to get res of punit GTD data\n");
>> -               return -ENXIO;
>> +       ++punit_res;
>> +       if (res) {
>> +               *punit_res = *res;
>> +               dev_info(&pdev->dev, "punit GTD data res: %pR\n", res);
>>         }
>> -       /* This is index 4 to cover GTD data register */
>> -       *++punit_res = *res;
>> -       dev_info(&pdev->dev, "punit GTD data res: %pR\n", res);
>>
>> +       /* This is index 5 to cover GTD interface register, optional */
>>         res = platform_get_resource(pdev, IORESOURCE_MEM,
>>                                     PLAT_RESOURCE_GTD_IFACE_INDEX);
>> -       if (!res) {
>> -               dev_err(&pdev->dev, "Failed to get res of punit GTD iface\n");
>> -               return -ENXIO;
>> +       ++punit_res;
>> +       if (res) {
>> +               *punit_res = *res;
>> +               dev_info(&pdev->dev, "punit GTD interface res: %pR\n", res);
>>         }
>> -       /* This is index 5 to cover GTD interface register */
>> -       *++punit_res = *res;
>> -       dev_info(&pdev->dev, "punit GTD interface res: %pR\n", res);
>>
>>         res = platform_get_resource(pdev, IORESOURCE_MEM,
>>                                     PLAT_RESOURCE_IPC_INDEX);
>> diff --git a/drivers/platform/x86/intel_punit_ipc.c b/drivers/platform/x86/intel_punit_ipc.c
>> index bd87540..a47a41f 100644
>> --- a/drivers/platform/x86/intel_punit_ipc.c
>> +++ b/drivers/platform/x86/intel_punit_ipc.c
>> @@ -227,6 +227,11 @@ static int intel_punit_get_bars(struct platform_device *pdev)
>>         struct resource *res;
>>         void __iomem *addr;
>>
>> +       /*
>> +        * The following resources are required
>> +        * - BIOS_IPC BASE_DATA
>> +        * - BIOS_IPC BASE_IFACE
>> +        */
>>         res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>         addr = devm_ioremap_resource(&pdev->dev, res);
>>         if (IS_ERR(addr))
>> @@ -239,29 +244,40 @@ static int intel_punit_get_bars(struct platform_device *pdev)
>>                 return PTR_ERR(addr);
>>         punit_ipcdev->base[BIOS_IPC][BASE_IFACE] = addr;
>>
>> +       /*
>> +        * The following resources are optional
>> +        * - ISPDRIVER_IPC BASE_DATA
>> +        * - ISPDRIVER_IPC BASE_IFACE
>> +        * - GTDRIVER_IPC BASE_DATA
>> +        * - GTDRIVER_IPC BASE_IFACE
>> +        */
>>         res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
>> -       addr = devm_ioremap_resource(&pdev->dev, res);
>> -       if (IS_ERR(addr))
>> -               return PTR_ERR(addr);
>> -       punit_ipcdev->base[ISPDRIVER_IPC][BASE_DATA] = addr;
>> +       if (res) {
>> +               addr = devm_ioremap_resource(&pdev->dev, res);
>> +               if (!IS_ERR(addr))
>> +                       punit_ipcdev->base[ISPDRIVER_IPC][BASE_DATA] = addr;
>> +       }
> 
> And here, what about just replacing return to dev_warn()?

I don't think we need to continue the subsequent ops if an error address
returns.

Thanks,
-Aubrey