Re: [PATCH v3 2/4] Watchdog: sp5100_tco: Refactor MMIO base address initialization

[Andy Shevchenko <andy.shevchenko@gmail.com>]

On Wed, Jan 19, 2022 at 5:46 PM Guenter Roeck <linux@roeck-us.net> wrote:
> On 1/19/22 3:53 AM, Andy Shevchenko wrote:
> > On Tue, Jan 18, 2022 at 10:23 PM Terry Bowman <terry.bowman@amd.com> wrote:

...

> >> +               devm_release_mem_region(dev, mmio_addr,
> >> +                                       SP5100_WDT_MEM_MAP_SIZE);
> >
> > Why? If it's a short live mapping, do not use devm.
>
> This is not short lived; it is needed by the driver. The release
> is an artifact of calling this function twice and ignoring the error
> from devm_ioremap() if the first call fails. devm_release_mem_region()
> isn't strictly needed but that would result in keeping the memory
> region reserved even though it isn't used by the driver.

So, this seems like micro-optimization, but okay, at least it
justifies it. Thanks for explaining.

> There is a functional difference to the original code, though.
> The failing devm_ioremap() causes the code to try the alternate
> address. I am not sure if that really adds value; devm_ioremap()
> fails because the system is out of virtual memory, and calling
> it again on a different address doesn't seem to add much value.
> I preferred the original code, which would only call devm_ioremap()
> after successfully reserving a memory region.

...

> > On top of above it's a NIH devm_ioremap_resource().
>
> Not sure what NIH means

Not Invented Here (syndrome)

...

> >> +       /* Check MMIO address conflict */
> >> +       ret = __sp5100_tco_prepare_base(tco, mmio_addr, dev_name);
> >
> >> +
> >> +       /* Check alternate MMIO address conflict */
> >
> > Unify this with the previous comment.
>
> Why ? It refers to the code below. If that is a single or two comments
> is really just POV.

It depends on the angle from which you see it (i.o.w. like you said
POV). I considered it from the code perspective and personally found
the
/*
 * Bla bla bla
 */
ret = foo();
if (ret)
  bar();

better than above.

> >> +       if (ret)
> >> +               ret = __sp5100_tco_prepare_base(tco, alt_mmio_addr,
> >> +                                               dev_name);

...

> >>          release_region(SP5100_IO_PM_INDEX_REG, SP5100_PM_IOPORTS_SIZE);
> >
> > Is it still needed? I have no context to say if devm_iomap() and this
> > are not colliding, please double check the correctness.
> >
> Not sure I understand. This is the release of the io region reserved with
> request_muxed_region() at the beginning of this function. Why would it no
> longer be necessary to release that region ?

Thank you for explaining, as I said I have no full context here, and I
simply asked for double check.

-- 
With Best Regards,
Andy Shevchenko