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

[Andy Shevchenko]

On Fri, Apr 15, 2016 at 8:05 AM, Darren Hart  wrote:

>>  +   /*
>>  +* 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(>dev, res);
>>  -   if (IS_ERR(addr))
>>  -   return PTR_ERR(addr);
>>  -   punit_ipcdev->base[ISPDRIVER_IPC][BASE_DATA] = addr;
>>  +   if (res) {
>>  +   addr = devm_ioremap_resource(>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?

Gmail hided this mail from me by some reason. I'm fine with the resolution.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Fri, Apr 15, 2016 at 8:05 AM, Darren Hart  wrote:

>>  +   /*
>>  +* 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(>dev, res);
>>  -   if (IS_ERR(addr))
>>  -   return PTR_ERR(addr);
>>  -   punit_ipcdev->base[ISPDRIVER_IPC][BASE_DATA] = addr;
>>  +   if (res) {
>>  +   addr = devm_ioremap_resource(>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?

Gmail hided this mail from me by some reason. I'm fine with the resolution.

-- 
With Best Regards,
Andy Shevchenko

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  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  
>> > 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(>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(>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(>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(>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(>dev, "Failed to get res of punit ISP 
>> >> data\n");
>> >> -   return -ENXIO;
>> >> +   ++punit_res;
>> >> +   if (res) {
>> >> +   *punit_res = *res;
>> >> +   dev_info(>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 

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  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  
>> > 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(>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(>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(>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(>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(>dev, "Failed to get res of punit ISP 
>> >> data\n");
>> >> -   return -ENXIO;
>> >> +   ++punit_res;
>> >> +   if (res) {
>> >> +   *punit_res = *res;
>> >> +   dev_info(>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
>> >> +* 

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

[Darren Hart]

On Fri, Apr 15, 2016 at 10:18:58AM +0800, Li, Aubrey wrote:
> On 2016/4/15 8:32, Darren Hart 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  
> >>> 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 

Lacking any additional follow-up, I've queued this to fixes for 4.6.

-- 
Darren Hart
Intel Open Source Technology Center

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

[Darren Hart]

On Fri, Apr 15, 2016 at 10:18:58AM +0800, Li, Aubrey wrote:
> On 2016/4/15 8:32, Darren Hart 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  
> >>> 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 

Lacking any additional follow-up, I've queued this to fixes for 4.6.

-- 
Darren Hart
Intel Open Source Technology Center

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

[Darren Hart]

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(>dev, "Failed to get res of punit ISP 
>  data\n");
>  -   return -ENXIO;
>  +   ++punit_res;
>  +   if (res) {
>  +   *punit_res = *res;
>  +   dev_info(>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(>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(>dev, res);
>  -   if (IS_ERR(addr))
>  -   return PTR_ERR(addr);
>  -   punit_ipcdev->base[ISPDRIVER_IPC][BASE_DATA] = addr;
>  +   if (res) {
>  +   addr = devm_ioremap_resource(>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

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

[Darren Hart]

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(>dev, "Failed to get res of punit ISP 
>  data\n");
>  -   return -ENXIO;
>  +   ++punit_res;
>  +   if (res) {
>  +   *punit_res = *res;
>  +   dev_info(>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(>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(>dev, res);
>  -   if (IS_ERR(addr))
>  -   return PTR_ERR(addr);
>  -   punit_ipcdev->base[ISPDRIVER_IPC][BASE_DATA] = addr;
>  +   if (res) {
>  +   addr = devm_ioremap_resource(>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

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

[Li, Aubrey]

On 2016/4/15 8:32, Darren Hart 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  
>>> 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 
 ---
  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(>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(>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(>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(>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.

Yes, these comment changes are supposed to be together with the
functional changes, without functional changes, I won't touch these
comments.

> 
> That said, everyone should understand that Andy is part of the
> platform-driver-x86 maintainer team so please respect his comments as such.

I know Andy very well, we co-worked with PMC driver before, it was a
nice process.

> 
>>>
>>>
 res = platform_get_resource(pdev, IORESOURCE_MEM,
 PLAT_RESOURCE_ISP_DATA_INDEX);
 -   if (!res) {
 -   dev_err(>dev, "Failed to get res of punit ISP 
 data\n");
 -   return -ENXIO;
 +   ++punit_res;
 +   if (res) {
 +   *punit_res = *res;
 +   dev_info(>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.

> 
> In my opinion, a refactor is a good 

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

[Li, Aubrey]

On 2016/4/15 8:32, Darren Hart 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  
>>> 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 
 ---
  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(>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(>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(>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(>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.

Yes, these comment changes are supposed to be together with the
functional changes, without functional changes, I won't touch these
comments.

> 
> That said, everyone should understand that Andy is part of the
> platform-driver-x86 maintainer team so please respect his comments as such.

I know Andy very well, we co-worked with PMC driver before, it was a
nice process.

> 
>>>
>>>
 res = platform_get_resource(pdev, IORESOURCE_MEM,
 PLAT_RESOURCE_ISP_DATA_INDEX);
 -   if (!res) {
 -   dev_err(>dev, "Failed to get res of punit ISP 
 data\n");
 -   return -ENXIO;
 +   ++punit_res;
 +   if (res) {
 +   *punit_res = *res;
 +   dev_info(>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.

> 
> In my opinion, a refactor is a good suggestion, but I would be OK with this
> patch as it is 

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

[Darren Hart]

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  
> > 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 
> >> ---
> >>  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(>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(>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(>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(>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(>dev, "Failed to get res of punit ISP 
> >> data\n");
> >> -   return -ENXIO;
> >> +   ++punit_res;
> >> +   if (res) {
> >> +   *punit_res = *res;
> >> +   dev_info(>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?

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;
> > }
> > 
> >> }
> >> -   /* This is index 2 to cover ISP 

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

[Darren Hart]

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  
> > 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 
> >> ---
> >>  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(>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(>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(>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(>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(>dev, "Failed to get res of punit ISP 
> >> data\n");
> >> -   return -ENXIO;
> >> +   ++punit_res;
> >> +   if (res) {
> >> +   *punit_res = *res;
> >> +   dev_info(>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?

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;
> > }
> > 
> >> }
> >> -   /* This is index 2 to cover ISP data register */
> >> -   *++punit_res = *res;
> 

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

[Li, Aubrey]

On 2016/4/11 14:48, Darren Hart wrote:
> On Mon, Apr 11, 2016 at 04:04:55AM +, Chakravarty, Souvik K wrote:
>>
>>
>>> -Original Message-
>>> From: Zha, Qipeng
>>> Sent: Monday, April 11, 2016 7:34 AM
>>> To: Darren Hart <dvh...@infradead.org>; Aubrey Li
>>> <aubrey...@linux.intel.com>; Chakravarty, Souvik K
>>> <souvik.k.chakrava...@intel.com>
>>> Cc: platform-driver-...@vger.kernel.org; linux-kernel@vger.kernel.org
>>> Subject: RE: [PATCH] platform:x86 decouple telemetry driver from the
>>> optional IPC resources
>>>
>>>>> 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.
>>>>>
>>>>> Signed-off-by: Aubrey Li <aubrey...@linux.intel.com>
>>>
>>>> Given the impact to their recent contributions, I'm looking for reviews 
>>>> from
>>> Qipeng and Souvik before I merge this.
>>>
>>>> Qipeng, as the listed maintainer for these two files, I particularly need 
>>>> to
>>> hear from you.
>>>
>>>> Thanks,
>>>
>>> Hi Darren,  I think this is a reasonable solution for such issue.
>>> Thanks Aubrey for this fixing patch.
>>
>> We went over this internally once. So OK from my POV.
> 
> Qipeng and Souvik, I appreciate the responses.


Thank you all. -Aubrey
> 
> As a point of process, I won't include a "Reviewed-by:" unless you explicitly
> include it in your response.
> 

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

[Li, Aubrey]

On 2016/4/11 14:48, Darren Hart wrote:
> On Mon, Apr 11, 2016 at 04:04:55AM +, Chakravarty, Souvik K wrote:
>>
>>
>>> -Original Message-
>>> From: Zha, Qipeng
>>> Sent: Monday, April 11, 2016 7:34 AM
>>> To: Darren Hart ; Aubrey Li
>>> ; Chakravarty, Souvik K
>>> 
>>> Cc: platform-driver-...@vger.kernel.org; linux-kernel@vger.kernel.org
>>> Subject: RE: [PATCH] platform:x86 decouple telemetry driver from the
>>> optional IPC resources
>>>
>>>>> 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.
>>>>>
>>>>> Signed-off-by: Aubrey Li 
>>>
>>>> Given the impact to their recent contributions, I'm looking for reviews 
>>>> from
>>> Qipeng and Souvik before I merge this.
>>>
>>>> Qipeng, as the listed maintainer for these two files, I particularly need 
>>>> to
>>> hear from you.
>>>
>>>> Thanks,
>>>
>>> Hi Darren,  I think this is a reasonable solution for such issue.
>>> Thanks Aubrey for this fixing patch.
>>
>> We went over this internally once. So OK from my POV.
> 
> Qipeng and Souvik, I appreciate the responses.


Thank you all. -Aubrey
> 
> As a point of process, I won't include a "Reviewed-by:" unless you explicitly
> include it in your response.
> 

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

[Darren Hart]

On Mon, Apr 11, 2016 at 04:04:55AM +, Chakravarty, Souvik K wrote:
> 
> 
> > -Original Message-
> > From: Zha, Qipeng
> > Sent: Monday, April 11, 2016 7:34 AM
> > To: Darren Hart <dvh...@infradead.org>; Aubrey Li
> > <aubrey...@linux.intel.com>; Chakravarty, Souvik K
> > <souvik.k.chakrava...@intel.com>
> > Cc: platform-driver-...@vger.kernel.org; linux-kernel@vger.kernel.org
> > Subject: RE: [PATCH] platform:x86 decouple telemetry driver from the
> > optional IPC resources
> > 
> > >> 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.
> > >>
> > >> Signed-off-by: Aubrey Li <aubrey...@linux.intel.com>
> > 
> > >Given the impact to their recent contributions, I'm looking for reviews 
> > >from
> > Qipeng and Souvik before I merge this.
> > 
> > >Qipeng, as the listed maintainer for these two files, I particularly need 
> > >to
> > hear from you.
> > 
> > >Thanks,
> > 
> > Hi Darren,  I think this is a reasonable solution for such issue.
> > Thanks Aubrey for this fixing patch.
> 
> We went over this internally once. So OK from my POV.

Qipeng and Souvik, I appreciate the responses.

As a point of process, I won't include a "Reviewed-by:" unless you explicitly
include it in your response.

-- 
Darren Hart
Intel Open Source Technology Center

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

[Darren Hart]

On Mon, Apr 11, 2016 at 04:04:55AM +, Chakravarty, Souvik K wrote:
> 
> 
> > -Original Message-
> > From: Zha, Qipeng
> > Sent: Monday, April 11, 2016 7:34 AM
> > To: Darren Hart ; Aubrey Li
> > ; Chakravarty, Souvik K
> > 
> > Cc: platform-driver-...@vger.kernel.org; linux-kernel@vger.kernel.org
> > Subject: RE: [PATCH] platform:x86 decouple telemetry driver from the
> > optional IPC resources
> > 
> > >> 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.
> > >>
> > >> Signed-off-by: Aubrey Li 
> > 
> > >Given the impact to their recent contributions, I'm looking for reviews 
> > >from
> > Qipeng and Souvik before I merge this.
> > 
> > >Qipeng, as the listed maintainer for these two files, I particularly need 
> > >to
> > hear from you.
> > 
> > >Thanks,
> > 
> > Hi Darren,  I think this is a reasonable solution for such issue.
> > Thanks Aubrey for this fixing patch.
> 
> We went over this internally once. So OK from my POV.

Qipeng and Souvik, I appreciate the responses.

As a point of process, I won't include a "Reviewed-by:" unless you explicitly
include it in your response.

-- 
Darren Hart
Intel Open Source Technology Center

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

[Chakravarty, Souvik K]

> -Original Message-
> From: Zha, Qipeng
> Sent: Monday, April 11, 2016 7:34 AM
> To: Darren Hart <dvh...@infradead.org>; Aubrey Li
> <aubrey...@linux.intel.com>; Chakravarty, Souvik K
> <souvik.k.chakrava...@intel.com>
> Cc: platform-driver-...@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: RE: [PATCH] platform:x86 decouple telemetry driver from the
> optional IPC resources
> 
> >> 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.
> >>
> >> Signed-off-by: Aubrey Li <aubrey...@linux.intel.com>
> 
> >Given the impact to their recent contributions, I'm looking for reviews from
> Qipeng and Souvik before I merge this.
> 
> >Qipeng, as the listed maintainer for these two files, I particularly need to
> hear from you.
> 
> >Thanks,
> 
> Hi Darren,  I think this is a reasonable solution for such issue.
> Thanks Aubrey for this fixing patch.

We went over this internally once. So OK from my POV.

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

[Chakravarty, Souvik K]

> -Original Message-
> From: Zha, Qipeng
> Sent: Monday, April 11, 2016 7:34 AM
> To: Darren Hart ; Aubrey Li
> ; Chakravarty, Souvik K
> 
> Cc: platform-driver-...@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: RE: [PATCH] platform:x86 decouple telemetry driver from the
> optional IPC resources
> 
> >> 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.
> >>
> >> Signed-off-by: Aubrey Li 
> 
> >Given the impact to their recent contributions, I'm looking for reviews from
> Qipeng and Souvik before I merge this.
> 
> >Qipeng, as the listed maintainer for these two files, I particularly need to
> hear from you.
> 
> >Thanks,
> 
> Hi Darren,  I think this is a reasonable solution for such issue.
> Thanks Aubrey for this fixing patch.

We went over this internally once. So OK from my POV.

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

[Zha, Qipeng]

>> 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.
>> 
>> Signed-off-by: Aubrey Li 

>Given the impact to their recent contributions, I'm looking for reviews from 
>Qipeng and Souvik before I merge this.

>Qipeng, as the listed maintainer for these two files, I particularly need to 
>hear from you.

>Thanks,

Hi Darren,  I think this is a reasonable solution for such issue.
Thanks Aubrey for this fixing patch.

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

[Zha, Qipeng]

>> 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.
>> 
>> Signed-off-by: Aubrey Li 

>Given the impact to their recent contributions, I'm looking for reviews from 
>Qipeng and Souvik before I merge this.

>Qipeng, as the listed maintainer for these two files, I particularly need to 
>hear from you.

>Thanks,

Hi Darren,  I think this is a reasonable solution for such issue.
Thanks Aubrey for this fixing patch.

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

[Li, Aubrey]

On 2016/4/10 21:17, Andy Shevchenko wrote:
> On Thu, Mar 31, 2016 at 10:28 PM, Aubrey Li  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 
>> ---
>>  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(>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(>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(>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(>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(>dev, "Failed to get res of punit ISP data\n");
>> -   return -ENXIO;
>> +   ++punit_res;
>> +   if (res) {
>> +   *punit_res = *res;
>> +   dev_info(>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(>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(>dev, "Failed to get res of punit ISP 
>> iface\n");
>> -   return -ENXIO;
>> +   ++punit_res;
>> +   if (res) {
>> +   *punit_res = *res;
>> +   dev_info(>dev, "punit ISP interface res: %pR\n", res);
>> }
>> -   /* This is index 3 to cover ISP interface register */
>> -   *++punit_res = *res;
>> -   dev_info(>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(>dev, "Failed to get res of punit GTD data\n");
>> -   return -ENXIO;
>> +   ++punit_res;
>> +   if (res) {
>> +   *punit_res = *res;
>> +   dev_info(>dev, "punit GTD data res: %pR\n", res);
>> }
>> -   /* This is index 4 to cover GTD data register */
>> -   *++punit_res = *res;
>> -   dev_info(>dev, "punit GTD data res: %pR\n", res);
>>
>> +   /* This is index 5 to cover GTD interface register, optional */
>> res = 

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

[Li, Aubrey]

On 2016/4/10 21:17, Andy Shevchenko wrote:
> On Thu, Mar 31, 2016 at 10:28 PM, Aubrey Li  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 
>> ---
>>  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(>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(>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(>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(>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(>dev, "Failed to get res of punit ISP data\n");
>> -   return -ENXIO;
>> +   ++punit_res;
>> +   if (res) {
>> +   *punit_res = *res;
>> +   dev_info(>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(>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(>dev, "Failed to get res of punit ISP 
>> iface\n");
>> -   return -ENXIO;
>> +   ++punit_res;
>> +   if (res) {
>> +   *punit_res = *res;
>> +   dev_info(>dev, "punit ISP interface res: %pR\n", res);
>> }
>> -   /* This is index 3 to cover ISP interface register */
>> -   *++punit_res = *res;
>> -   dev_info(>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(>dev, "Failed to get res of punit GTD data\n");
>> -   return -ENXIO;
>> +   ++punit_res;
>> +   if (res) {
>> +   *punit_res = *res;
>> +   dev_info(>dev, "punit GTD data res: %pR\n", res);
>> }
>> -   /* This is index 4 to cover GTD data register */
>> -   *++punit_res = *res;
>> -   dev_info(>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,
>> 

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

[Andy Shevchenko]

On Thu, Mar 31, 2016 at 10:28 PM, Aubrey Li  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 
> ---
>  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(>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(>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(>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(>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?


> res = platform_get_resource(pdev, IORESOURCE_MEM,
> PLAT_RESOURCE_ISP_DATA_INDEX);
> -   if (!res) {
> -   dev_err(>dev, "Failed to get res of punit ISP data\n");
> -   return -ENXIO;
> +   ++punit_res;
> +   if (res) {
> +   *punit_res = *res;
> +   dev_info(>dev, "punit ISP data res: %pR\n", res);

Okay, what if you re-arrange this to some helper first

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(>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(>dev, "Failed to get res of punit ISP iface\n");
> -   return -ENXIO;
> +   ++punit_res;
> +   if (res) {
> +   *punit_res = *res;
> +   dev_info(>dev, "punit ISP interface res: %pR\n", res);
> }
> -   /* This is index 3 to cover ISP interface register */
> -   *++punit_res = *res;
> -   dev_info(>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(>dev, "Failed to get res of punit GTD data\n");
> -   return -ENXIO;
> +   ++punit_res;
> +   if (res) {
> +   *punit_res = *res;
> +   dev_info(>dev, "punit GTD data res: %pR\n", res);
> }
> -   /* This is index 4 to cover GTD data register */
> -   *++punit_res = *res;
> -   dev_info(>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(>dev, "Failed to get res of punit GTD iface\n");
> -   return -ENXIO;
> +   ++punit_res;
> +   if (res) {
> +   *punit_res = *res;
> +   dev_info(>dev, "punit GTD 

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

[Andy Shevchenko]

On Thu, Mar 31, 2016 at 10:28 PM, Aubrey Li  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 
> ---
>  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(>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(>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(>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(>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?


> res = platform_get_resource(pdev, IORESOURCE_MEM,
> PLAT_RESOURCE_ISP_DATA_INDEX);
> -   if (!res) {
> -   dev_err(>dev, "Failed to get res of punit ISP data\n");
> -   return -ENXIO;
> +   ++punit_res;
> +   if (res) {
> +   *punit_res = *res;
> +   dev_info(>dev, "punit ISP data res: %pR\n", res);

Okay, what if you re-arrange this to some helper first

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(>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(>dev, "Failed to get res of punit ISP iface\n");
> -   return -ENXIO;
> +   ++punit_res;
> +   if (res) {
> +   *punit_res = *res;
> +   dev_info(>dev, "punit ISP interface res: %pR\n", res);
> }
> -   /* This is index 3 to cover ISP interface register */
> -   *++punit_res = *res;
> -   dev_info(>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(>dev, "Failed to get res of punit GTD data\n");
> -   return -ENXIO;
> +   ++punit_res;
> +   if (res) {
> +   *punit_res = *res;
> +   dev_info(>dev, "punit GTD data res: %pR\n", res);
> }
> -   /* This is index 4 to cover GTD data register */
> -   *++punit_res = *res;
> -   dev_info(>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(>dev, "Failed to get res of punit GTD iface\n");
> -   return -ENXIO;
> +   ++punit_res;
> +   if (res) {
> +   *punit_res = *res;
> +   dev_info(>dev, "punit GTD interface res: %pR\n", res);
> }
> -   /* 

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

[Darren Hart]

On Thu, Mar 31, 2016 at 02:28:09PM -0500, Aubrey Li 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.
> 
> Signed-off-by: Aubrey Li 

Given the impact to their recent contributions, I'm looking for reviews from
Qipeng and Souvik before I merge this.

Qipeng, as the listed maintainer for these two files, I particularly need to
hear from you.

Thanks,

> ---
>  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(>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(>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(>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(>dev, "punit BIOS interface res: %pR\n", res);
>  
> + /* This is index 2 to cover ISP data register, optional */
>   res = platform_get_resource(pdev, IORESOURCE_MEM,
>   PLAT_RESOURCE_ISP_DATA_INDEX);
> - if (!res) {
> - dev_err(>dev, "Failed to get res of punit ISP data\n");
> - return -ENXIO;
> + ++punit_res;
> + if (res) {
> + *punit_res = *res;
> + dev_info(>dev, "punit ISP data res: %pR\n", res);
>   }
> - /* This is index 2 to cover ISP data register */
> - *++punit_res = *res;
> - dev_info(>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(>dev, "Failed to get res of punit ISP iface\n");
> - return -ENXIO;
> + ++punit_res;
> + if (res) {
> + *punit_res = *res;
> + dev_info(>dev, "punit ISP interface res: %pR\n", res);
>   }
> - /* This is index 3 to cover ISP interface register */
> - *++punit_res = *res;
> - dev_info(>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(>dev, "Failed to get res of punit GTD data\n");
> - return -ENXIO;
> + ++punit_res;
> + if (res) {
> + *punit_res = *res;
> + dev_info(>dev, "punit GTD data res: %pR\n", res);
>   }
> - /* This is index 4 to cover GTD data register */
> - *++punit_res = *res;
> - dev_info(>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(>dev, "Failed to get res of punit GTD iface\n");
> - return -ENXIO;
> + ++punit_res;
> + if (res) {
> + *punit_res = *res;
> + dev_info(>dev, "punit GTD interface res: %pR\n", res);
>   }
> - /* This is index 5 to cover GTD interface register */
> - *++punit_res = *res;
> - dev_info(>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
> +++ 

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

[Darren Hart]

On Thu, Mar 31, 2016 at 02:28:09PM -0500, Aubrey Li 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.
> 
> Signed-off-by: Aubrey Li 

Given the impact to their recent contributions, I'm looking for reviews from
Qipeng and Souvik before I merge this.

Qipeng, as the listed maintainer for these two files, I particularly need to
hear from you.

Thanks,

> ---
>  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(>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(>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(>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(>dev, "punit BIOS interface res: %pR\n", res);
>  
> + /* This is index 2 to cover ISP data register, optional */
>   res = platform_get_resource(pdev, IORESOURCE_MEM,
>   PLAT_RESOURCE_ISP_DATA_INDEX);
> - if (!res) {
> - dev_err(>dev, "Failed to get res of punit ISP data\n");
> - return -ENXIO;
> + ++punit_res;
> + if (res) {
> + *punit_res = *res;
> + dev_info(>dev, "punit ISP data res: %pR\n", res);
>   }
> - /* This is index 2 to cover ISP data register */
> - *++punit_res = *res;
> - dev_info(>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(>dev, "Failed to get res of punit ISP iface\n");
> - return -ENXIO;
> + ++punit_res;
> + if (res) {
> + *punit_res = *res;
> + dev_info(>dev, "punit ISP interface res: %pR\n", res);
>   }
> - /* This is index 3 to cover ISP interface register */
> - *++punit_res = *res;
> - dev_info(>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(>dev, "Failed to get res of punit GTD data\n");
> - return -ENXIO;
> + ++punit_res;
> + if (res) {
> + *punit_res = *res;
> + dev_info(>dev, "punit GTD data res: %pR\n", res);
>   }
> - /* This is index 4 to cover GTD data register */
> - *++punit_res = *res;
> - dev_info(>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(>dev, "Failed to get res of punit GTD iface\n");
> - return -ENXIO;
> + ++punit_res;
> + if (res) {
> + *punit_res = *res;
> + dev_info(>dev, "punit GTD interface res: %pR\n", res);
>   }
> - /* This is index 5 to cover GTD interface register */
> - *++punit_res = *res;
> - dev_info(>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
> +++ 

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

[Aubrey Li]

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.

Signed-off-by: Aubrey Li 
---
 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(>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(>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(>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(>dev, "punit BIOS interface res: %pR\n", res);
 
+   /* This is index 2 to cover ISP data register, optional */
res = platform_get_resource(pdev, IORESOURCE_MEM,
PLAT_RESOURCE_ISP_DATA_INDEX);
-   if (!res) {
-   dev_err(>dev, "Failed to get res of punit ISP data\n");
-   return -ENXIO;
+   ++punit_res;
+   if (res) {
+   *punit_res = *res;
+   dev_info(>dev, "punit ISP data res: %pR\n", res);
}
-   /* This is index 2 to cover ISP data register */
-   *++punit_res = *res;
-   dev_info(>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(>dev, "Failed to get res of punit ISP iface\n");
-   return -ENXIO;
+   ++punit_res;
+   if (res) {
+   *punit_res = *res;
+   dev_info(>dev, "punit ISP interface res: %pR\n", res);
}
-   /* This is index 3 to cover ISP interface register */
-   *++punit_res = *res;
-   dev_info(>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(>dev, "Failed to get res of punit GTD data\n");
-   return -ENXIO;
+   ++punit_res;
+   if (res) {
+   *punit_res = *res;
+   dev_info(>dev, "punit GTD data res: %pR\n", res);
}
-   /* This is index 4 to cover GTD data register */
-   *++punit_res = *res;
-   dev_info(>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(>dev, "Failed to get res of punit GTD iface\n");
-   return -ENXIO;
+   ++punit_res;
+   if (res) {
+   *punit_res = *res;
+   dev_info(>dev, "punit GTD interface res: %pR\n", res);
}
-   /* This is index 5 to cover GTD interface register */
-   *++punit_res = *res;
-   dev_info(>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 = 

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

[Aubrey Li]

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.

Signed-off-by: Aubrey Li 
---
 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(>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(>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(>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(>dev, "punit BIOS interface res: %pR\n", res);
 
+   /* This is index 2 to cover ISP data register, optional */
res = platform_get_resource(pdev, IORESOURCE_MEM,
PLAT_RESOURCE_ISP_DATA_INDEX);
-   if (!res) {
-   dev_err(>dev, "Failed to get res of punit ISP data\n");
-   return -ENXIO;
+   ++punit_res;
+   if (res) {
+   *punit_res = *res;
+   dev_info(>dev, "punit ISP data res: %pR\n", res);
}
-   /* This is index 2 to cover ISP data register */
-   *++punit_res = *res;
-   dev_info(>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(>dev, "Failed to get res of punit ISP iface\n");
-   return -ENXIO;
+   ++punit_res;
+   if (res) {
+   *punit_res = *res;
+   dev_info(>dev, "punit ISP interface res: %pR\n", res);
}
-   /* This is index 3 to cover ISP interface register */
-   *++punit_res = *res;
-   dev_info(>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(>dev, "Failed to get res of punit GTD data\n");
-   return -ENXIO;
+   ++punit_res;
+   if (res) {
+   *punit_res = *res;
+   dev_info(>dev, "punit GTD data res: %pR\n", res);
}
-   /* This is index 4 to cover GTD data register */
-   *++punit_res = *res;
-   dev_info(>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(>dev, "Failed to get res of punit GTD iface\n");
-   return -ENXIO;
+   ++punit_res;
+   if (res) {
+   *punit_res = *res;
+   dev_info(>dev, "punit GTD interface res: %pR\n", res);
}
-   /* This is index 5 to cover GTD interface register */
-   *++punit_res = *res;
-   dev_info(>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);