Received-SPF: pass (google.com: domain of linux-kernel+bounces-165131-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <86677a82-e91f-45de-89c5-66c1cd82897d@gmail.com>
Date: Wed, 1 May 2024 13:16:03 +0300
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v1 5/6] watchdog: ROHM BD96801 PMIC WDG driver
Content-Language: en-US, en-GB
To: Guenter Roeck <linux@roeck-us.net>,
 Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
Cc: Lee Jones <lee@kernel.org>, Rob Herring <robh@kernel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
 Conor Dooley <conor+dt@kernel.org>, Liam Girdwood <lgirdwood@gmail.com>,
 Mark Brown <broonie@kernel.org>, Wim Van Sebroeck <wim@linux-watchdog.org>,
 devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
 linux-watchdog@vger.kernel.org
References: <cover.1714478142.git.mazziesaccount@gmail.com>
 <d1d5429e4a34a06af0ceb853d72344cf9b8ae67c.1714478142.git.mazziesaccount@gmail.com>
 <b4c67e76-fad8-47bc-a9f8-29cc5630bc7b@roeck-us.net>
From: Matti Vaittinen <mazziesaccount@gmail.com>
In-Reply-To: <b4c67e76-fad8-47bc-a9f8-29cc5630bc7b@roeck-us.net>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

Hi Guenter,

Thanks again for the review :) I agree with all of your comments, thanks!

On 4/30/24 19:24, Guenter Roeck wrote:
> On 4/30/24 05:01, Matti Vaittinen wrote:
>> Introduce driver for WDG block on ROHM BD96801 scalable PMIC.
>>
>> This driver only supports watchdog with I2C feeding and delayed
>> response detection. Whether the watchdog toggles PRSTB pin or
>> just causes an interrupt can be configured via device-tree.
>>
>> The BD96801 PMIC HW supports also window watchdog (too early
>> feeding detection) and Q&A mode. These are not supported by
>> this driver.
>>
>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
>>
>> ---
>> Revision history:
>> RFCv2 => v1:
>> - Fix watchdog time-outs to match DS4
>> - Fix target timeout overflow
>> - Improve dbg prints
>>
>> RFCv1 => RFCv2:
>> - remove always running
>> - add IRQ handling
>> - call emergency_restart()
>> - drop MODULE_ALIAS and add MODULE_DEVICE_TABLE
>> ---

..

>> +
>> +static int bd96801_set_wdt_mode(struct wdtbd96801 *w, unsigned int 
>> hw_margin,
>> +                   unsigned int hw_margin_min)
>> +{
>> +    int fastng, slowng, type, ret, reg, mask;
>> +    struct device *dev = w->dev;
>> +
>> +    /*
>> +     * Convert to 100uS to guarantee reasonable timeouts fit in
>> +     * 32bit maintaining also a decent accuracy.
>> +     */
>> +    hw_margin *= 10;
>> +    hw_margin_min *= 10;
>> +
>> +    if (hw_margin_min) {
>> +        unsigned int min;
>> +
>> +        type = BD96801_WD_TYPE_WIN;
>> +        dev_dbg(dev, "Setting type WINDOW 0x%x\n", type);
>> +        ret = find_closest_fast(hw_margin_min, &fastng, &min);
>> +        if (ret) {
>> +            dev_err(dev, "bad WDT window for fast timeout\n");
>> +            return ret;
>> +        }
>> +
>> +        ret = find_closest_slow_by_fast(min, &hw_margin, &slowng);
>> +        if (ret) {
>> +            dev_err(dev, "bad WDT window\n");
>> +            return ret;
>> +        }
>> +        w->wdt.min_hw_heartbeat_ms = min / 10;
>> +    } else {
>> +        type = BD96801_WD_TYPE_SLOW;
>> +        dev_dbg(dev, "Setting type SLOW 0x%x\n", type);
>> +        ret = find_closest_slow(&hw_margin, &slowng, &fastng);
>> +        if (ret) {
>> +            dev_err(dev, "bad WDT window\n");
> 
> What is the value of those error messages ? To me they only leave big
> question marks. One would see "bad WDT window" or "bad WDT window for
> fast timeout" and then what ? If the cause is bad values for hw_margin
> and/or hw_margin_min, the message should include the offending values
> and not leave the user in the dark.

Well, at least one can grep the error from module which printed it :) 
But yes, you have a very valid point. I will see how to improve, thanks!

> 
>> +            return ret;
>> +        }
>> +    }
>> +
>> +    w->wdt.max_hw_heartbeat_ms = hw_margin / 10;
>> +
>> +    fastng <<= ffs(BD96801_WD_TMO_SHORT_MASK) - 1;
> 
> Any reason fore not using standard functionality such as FIELD_PREP here ?

A reason, yes. A good reason, no :) Reason is that I wrote this before I 
learned about the FIELD_PREP() and FIELD_GET(). And actually, I have 
always found those two more confusing than the open-coded shift, '|' and 
'&'. Still, I believe the FIELD_GET() and FIELD_PREP() make it obvious 
for a reader that the bit magic is just a standard register field stuff.

So, no good reason. I'll fix this for the next version.

..

>> +extern void emergency_restart(void);
> 
> No. include linux/reboot.h instead.

Hmm.. I am sure I had checked the headers for prototypes before doing 
something like this... I think I must've missed something. I'll fix this 
for the next version, or add a good explanation if there indeed was some 
problem finding the prototype.

Yours,
	-- Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~