On Fri, Apr 15, 2016 at 10:18:58AM +0800, Li, Aubrey wrote:
Hi Aubrey,
> >>>> 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.
> >
> > 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?
>
> dev_info with different strings makes the helper useless, or more
> complex than desired if we have to write a helper here.
>
> Also, we have necessary resource above, which returns directly if there
> is a error. For the coding style consistency in a routine, I really
> don't like we call platform_get_resource() directly at first and then
> put it into a helper later. The current implementation is
> straightforward and clean.
Both of those could be addressed with arguments, macros, etc. However, that
becomes subjective quickly, and I appreciate the incremental functional fix
here. I think this bit is fine as is.
...
> >>>> 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.
> >
> > 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.
> >
> We discussed this.
> - For the necessary resources, if we obtain an error address, we should
> return immediately.
> - For the optional resources, we keep quiet if we don't get them, that
> is, not throwing a warning out.
Andy, he's checking for "res" now too, which is a good extra check since
devm_ioremap_resource will issue a dev_err "invalid resource" if it's NULL, even
though in our case, that's expected for an optional resource. This adds the
extra nesting, and a dev_warn wouldn't be appropriate for an option resource.
I'm happy to queue this to fixes at this point. Andy, are you OK with the
resolution here?
--
Darren Hart
Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
