Message-ID: <e558133e-b222-6cd8-6e18-162a45cb12fa@roeck-us.net>
Date:   Wed, 19 Jan 2022 09:08:34 -0800
From:   Guenter Roeck <linux@...ck-us.net>
To:     Terry Bowman <Terry.Bowman@....com>,
        Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     linux-watchdog@...r.kernel.org, Jean Delvare <jdelvare@...e.com>,
        linux-i2c <linux-i2c@...r.kernel.org>,
        Wolfram Sang <wsa@...nel.org>,
        "Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Wim Van Sebroeck <wim@...ux-watchdog.org>,
        Robert Richter <rrichter@....com>,
        Tom Lendacky <thomas.lendacky@....com>,
        "Shah, Nehal-bakulchandra" <Nehal-bakulchandra.Shah@....com>,
        Basavaraj Natikar <Basavaraj.Natikar@....com>,
        Shyam Sundar S K <Shyam-sundar.S-k@....com>,
        Mario Limonciello <Mario.Limonciello@....com>
Subject: Re: [PATCH v3 2/4] Watchdog: sp5100_tco: Refactor MMIO base address
 initialization

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@....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@...ck-us.net>
>>> To: linux-watchdog@...r.kernel.org
>>> To: Jean Delvare <jdelvare@...e.com>
>>> To: linux-i2c@...r.kernel.org
>>> To: Wolfram Sang <wsa@...nel.org>
>>> To: Andy Shevchenko <andy.shevchenko@...il.com>
>>> To: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
>>> Cc: linux-kernel@...r.kernel.org
>>> Cc: Wim Van Sebroeck <wim@...ux-watchdog.org>
>>> Cc: Robert Richter <rrichter@....com>
>>> Cc: Thomas Lendacky <thomas.lendacky@....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
> 

Powered by blists - more mailing lists