On Wed, Jan 19, 2022 at 5:46 PM Guenter Roeck <linux@xxxxxxxxxxxx> wrote: > On 1/19/22 3:53 AM, Andy Shevchenko wrote: > > On Tue, Jan 18, 2022 at 10:23 PM Terry Bowman <terry.bowman@xxxxxxx> 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
