Messages in this thread | | | Subject | Re: [PATCH v3 2/4] Watchdog: sp5100_tco: Refactor MMIO base address initialization | From | Guenter Roeck <> | Date | Wed, 19 Jan 2022 09:08:34 -0800 |
| |
On 1/19/22 8:57 AM, Terry Bowman wrote: > > > On 1/19/22 5:53 AM, Andy Shevchenko wrote: >> On Tue, Jan 18, 2022 at 10:23 PM Terry Bowman <terry.bowman@amd.com> wrote: >>> >>> Combine MMIO base address and alternate base address detection. Combine >>> based on layout type. This will simplify the function by eliminating >>> a switch case. >>> >>> Move existing request/release code into functions. This currently only >>> supports port I/O request/release. The move into a separate function >>> will make it ready for adding MMIO region support. >> >> ... >> >>> To: Guenter Roeck <linux@roeck-us.net> >>> To: linux-watchdog@vger.kernel.org >>> To: Jean Delvare <jdelvare@suse.com> >>> To: linux-i2c@vger.kernel.org >>> To: Wolfram Sang <wsa@kernel.org> >>> To: Andy Shevchenko <andy.shevchenko@gmail.com> >>> To: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >>> Cc: linux-kernel@vger.kernel.org >>> Cc: Wim Van Sebroeck <wim@linux-watchdog.org> >>> Cc: Robert Richter <rrichter@amd.com> >>> Cc: Thomas Lendacky <thomas.lendacky@amd.com> >> >> Same comment to all your patches. > > Ok. I'll reduce the patches' to/cc list to only contain maintainers owning > the current patch. I prefer to leave the lengthy list in the cover letter > if that is ok because it will not be added to the tree but will provide > context this series has multiple systems and may need communication > between maintainers. I'll use the -to & -cc commandline as you mentioned to > send to the longer list of recipients without cluttering the patch. Let me > know if you prefer otherwise. >> >> ... >> >>> +static int __sp5100_tco_prepare_base(struct sp5100_tco *tco, >>> + u32 mmio_addr, >>> + const char *dev_name) >>> +{ >>> + struct device *dev = tco->wdd.parent; >> >>> + int ret = 0; >> >> Not really used variable. > > Yes, I'll remove 'ret' and set hardcoded 0 return value below. > >> >>> + if (!mmio_addr) >>> + return -ENOMEM; >>> + >>> + if (!devm_request_mem_region(dev, mmio_addr, >>> + SP5100_WDT_MEM_MAP_SIZE, >>> + dev_name)) { >>> + dev_dbg(dev, "MMIO address 0x%08x already in use\n", >>> + mmio_addr); >>> + return -EBUSY; >>> + } >>> + >>> + tco->tcobase = devm_ioremap(dev, mmio_addr, >>> + SP5100_WDT_MEM_MAP_SIZE); >>> + if (!tco->tcobase) { >>> + dev_dbg(dev, "MMIO address 0x%08x failed mapping.\n", >>> + mmio_addr); >> >>> + 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 region isn't short lived. This is a region request for the > WDT registers used through the lifetime of the driver. > > The short lived mapping you may be thinking of is in > sp5100_tco_setupdevice_mmio() from patch 3. The first register in > this region is FCH::PM::DECODEEN and is used to setup the mmio_addr > and alt_mmio_addr MMIO (longterm) regions. > >> >>> + return -ENOMEM; >>> + } >> >>> + dev_info(dev, "Using 0x%08x for watchdog MMIO address\n", >>> + mmio_addr); >> >> Unneeded noise. > > This was present pre-series. The current driver dmesg output with default > logging settings is: > > dmesg | grep -i sp5100 > [ 8.508515] sp5100_tco: SP5100/SB800 TCO WatchDog Timer Driver > [ 8.525172] sp5100-tco sp5100-tco: Using 0xfeb00000 for watchdog MMIO address > [ 8.539912] sp5100-tco sp5100-tco: initialized. heartbeat=60 sec (nowayout=0) > > I'll remove the dev_info. > >> >>> + return ret; >> >> On top of above it's a NIH devm_ioremap_resource(). > > I'm not familiar with NIH term. My friends google and grep weren't much help. > >> >>> +} >> >> >> ... >> >>> + int ret = 0; >> >> Redundant assignment. > > Thanks. I'll leave the variable but remove the 0 assignment in the definition. > >> >> ... >> >>> + /* 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. > > Ok > >> >>> + if (ret) >>> + ret = __sp5100_tco_prepare_base(tco, alt_mmio_addr, >>> + dev_name); >> >> ... >> >>> + if (alt_mmio_addr & ((SB800_ACPI_MMIO_DECODE_EN | >>> + SB800_ACPI_MMIO_SEL) != >>> + SB800_ACPI_MMIO_DECODE_EN)) { >> >> The split looks ugly. Consider to use temporary variables or somehow >> rearrange the condition that it doesn't break in the middle of the one >> logical token. > > I'll try splitting at the '&' as Guenter mentioned in other email. > >> >>> + alt_mmio_addr &= ~0xFFF; >> >> Why capital letters? > > This was already present pre-series. I'll change to lowercase to make it > consistent with the other constants in the file. > >> >>> + alt_mmio_addr += SB800_PM_WDT_MMIO_OFFSET; >>> + } >> >> ... >> >>> + if (!(alt_mmio_addr & (((SB800_ACPI_MMIO_DECODE_EN | >>> + SB800_ACPI_MMIO_SEL)) != >>> + SB800_ACPI_MMIO_DECODE_EN))) { >> >> Ditto. > > My understanding is #defines should be capitalized. No? >
I think that Ditto referred to the line split comment.
Guenter
>> >>> + alt_mmio_addr &= ~0xFFF; >> >> Ditto. > > Another uppercase constant I will make lowercase. > >> >>> + alt_mmio_addr += SB800_PM_WDT_MMIO_OFFSET; >> >> ... >> >> Okay, I see this is the original code like this... Perhaps it makes >> sense to reshuffle them (indentation-wise) at the same time and >> mention this in the changelog. >> >> ... >> >>> 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. >> > > Yes, this is needed. The release/request in sp5100_tco_setupdevice() is > for the non-efch mmio layout cases. It is using port I/O registers to > detect mmio_addr, alt_mmio_addr, and configure the device. > > Regards, > Terry Bowman >
| |