Re: [PATCH v1 5/6] watchdog: ROHM BD96801 PMIC WDG driver

[Matti Vaittinen <mazziesaccount@gmail.com>]

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! ~~