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

[Terry Bowman]

On 1/19/22 5:53 AM, Andy Shevchenko wrote:
> On Tue, Jan 18, 2022 at 10:23 PM Terry Bowman <terry.bowman@xxxxxxx> 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@xxxxxxxxxxxx>
>> To: linux-watchdog@xxxxxxxxxxxxxxx
>> To: Jean Delvare <jdelvare@xxxxxxxx>
>> To: linux-i2c@xxxxxxxxxxxxxxx
>> To: Wolfram Sang <wsa@xxxxxxxxxx>
>> To: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
>> To: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
>> Cc: linux-kernel@xxxxxxxxxxxxxxx
>> Cc: Wim Van Sebroeck <wim@xxxxxxxxxxxxxxxxxx>
>> Cc: Robert Richter <rrichter@xxxxxxx>
>> Cc: Thomas Lendacky <thomas.lendacky@xxxxxxx>
> 
> 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?

> 
>> +                       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