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

[Andy Shevchenko]

On Fri, Apr 15, 2016 at 3:32 AM, Darren Hart <dvhart@xxxxxxxxxxxxx> wrote:
> On Sun, Apr 10, 2016 at 09:46:48PM +0800, Li, Aubrey wrote:
>> On 2016/4/10 21:17, Andy Shevchenko wrote:
>> > On Thu, Mar 31, 2016 at 10:28 PM, Aubrey Li <aubrey.li@xxxxxxxxxxxxxxx> 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.

>> >> --- 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.
>>
>
> This is typically necessary as you would not want the comment fixes above to be
> backed out if the functional changes below were found to be buggy and reverted.
> This is why we encourage small functional changes. It protects against
> inadvertent reverts and facilitates review.
>
> That said, these comment changes continue below in a way that makes it a bit
> difficult to isolate them out, so I do not particularly object.
>
> That said, everyone should understand that Andy is part of the
> platform-driver-x86 maintainer team so please respect his comments as such.
>
>> >
>> >
>> >>         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?
>
> In both arguments, we need to identify the WHY.
>
> I imagine Andy is trying to reduce the copy and paste potential for error as
> well as error introduction in future patches. There are... 7 or so cases with
> near identical usage, that's a compelling argument for a refactor such as the
> helper Andy suggests.

Correct, But it might be done in a separate patch I suppose.

> Aubrey, you said you don't like it. Why is that? Will it not save enough lines
> of code to be worth it? Are you concerned about revalidating the change?
>
> In my opinion, a refactor is a good suggestion, but I would be OK with this
> patch as it is and a refactor to follow. I hesitate to do this when the refactor
> is really critical as it may not happen, but in this case, it doesn't seem
> absolutely necessary.
>
>>
>> > 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;
>> > }

>> >> @@ -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.
>
> Why is that? Will the driver fail to provide any functionality? Or could it be
> the other IFACEs could still be of some use?
>
> This one does need a justification.


-- 
With Best Regards,
Andy Shevchenko