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

[Matti Vaittinen]

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