lkml.org 
[lkml]   [2022]   [Jan]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v3 2/4] Watchdog: sp5100_tco: Refactor MMIO base address initialization
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

\
 
 \ /
  Last update: 2022-01-20 12:15    [W:0.116 / U:0.948 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site