On Tue, Feb 15, 2022 at 05:29:51PM +0000, Lee Jones wrote:
> 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!
I have realised that in current form it has a bug (*), so I re-do a bit the way
that comment stays and the actual actions will be to *fill* the resource.
*) unbinding and binding back will bring us to the completely wrong resources.
--
With Best Regards,
Andy Shevchenko
