Re: [PATCH v4 5/8] mfd: lpc_ich: Add support for pinctrl in non-ACPI system

[Lee Jones <lee.jones@xxxxxxxxxx>]

On Tue, 15 Feb 2022, Andy Shevchenko wrote:

> On Tue, Feb 15, 2022 at 04:54:00PM +0000, Lee Jones wrote:
> > On Mon, 31 Jan 2022, Andy Shevchenko wrote:
> 
> Thank you for the review, my answers below.
> 
> ...
> 
> > > +static struct resource apl_gpio_resources[APL_GPIO_NR_DEVICES][2] = {
> > > +	[APL_GPIO_NORTH] = {
> > > +		DEFINE_RES_MEM(APL_GPIO_NORTH_OFFSET, 0x1000),
> > 
> > Are these 0x1000's being over-written in lpc_ich_init_pinctrl()?
> > 
> > If so, why pre-initialise?
> 
> You mean to pre-initialize the offsets, but leave the length to be added
> in the function? It can be done, but it feels inconsistent, since we would
> have offsets and lengths in different places for the same thingy. That said,
> I prefer current way for the sake of consistency.

Don't you over-write this entry entirely?

  for (i = 0; i < ARRAY_SIZE(apl_gpio_devices); i++) {
        struct resource *mem = &apl_gpio_resources[i][0];

        /* Fill MEM resource */
        mem->start += base.start;
        mem->end += base.start;
        mem->flags = base.flags;
  }

Oh wait, you're just adding the base value to the offsets.

In which case that comment is also confusing!

-- 
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog