Re: [PATCH V9 1/2] intel_pmc_ipc: update acpi resource structure for Punit

[Darren Hart <dvhart@infradead.org>]

On Thu, Dec 10, 2015 at 01:13:01PM +0200, Andy Shevchenko wrote:
> On Fri, 2015-12-11 at 02:21 +0800, Qipeng Zha wrote:
> > BIOS restructure exported memory resources for Punit
> > in acpi table, So update resources for Punit.
> > 
> > Signed-off-by: Qipeng Zha <qipeng.zha@intel.com>
> > 
> 
> Almost. See below.
> 
> > ---
> > change in v9
> >  rename indexes of all acpi resources;
> >  add comments for punit_res;
> > ---
> >  drivers/platform/x86/intel_pmc_ipc.c | 114 ++++++++++++++++++++++++-
> > ----------
> >  1 file changed, 80 insertions(+), 34 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/intel_pmc_ipc.c
> > b/drivers/platform/x86/intel_pmc_ipc.c
> > index 28b2a12..00812a4 100644
> > --- a/drivers/platform/x86/intel_pmc_ipc.c
> > +++ b/drivers/platform/x86/intel_pmc_ipc.c
> > @@ -68,8 +68,12 @@
> >  #define PLAT_RESOURCE_IPC_INDEX		0
> >  #define PLAT_RESOURCE_IPC_SIZE		0x1000
> >  #define PLAT_RESOURCE_GCR_SIZE		0x1000
> > -#define PLAT_RESOURCE_PUNIT_DATA_INDEX	1
> > -#define PLAT_RESOURCE_PUNIT_INTER_INDEX	2
> > +#define PLAT_RESOURCE_BIOS_DATA_INDEX	1
> > +#define PLAT_RESOURCE_BIOS_INTER_INDEX	2
> 
> Since there is a comment and new version will be anticipated I also
> would like to propose to change INTER to IFACE because latter more
> particular. Darren?

Yes, I had the same thought, but wasn't going to request the change just for
this. Andriy did point out the pointer format below previously however. Please
do correct that - updating INTER to IFACE would be a welcome improvement as
well.

> > -	size = resource_size(res);
> > -	ipcdev.punit_base2 = res->start;
> > -	ipcdev.punit_size2 = size;
> > -	dev_info(&pdev->dev, "punit interface res: %llx %x\n",
> > +	/* This is index 1 to cover BIOS interface register */
> > +	*++punit_res = *res;
> > +	dev_info(&pdev->dev, "punit BIOS interface res: %llx %x\n",
> > +		 (long long)res->start, (int)resource_size(res));
> 
> There is a specifier to print struct resource, please use it instead.
> %pR IIRC.
> 

Yes, %pR, see Documentation/printk-formats.txt

> Same for all similar cases below.
> 
-- 
Darren Hart
Intel Open Source Technology Center