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

[Guenter Roeck]

On Wed, Apr 03, 2024 at 09:34:35AM +0300, Matti Vaittinen wrote:
> Hi Guenter,
> 
> First of all, thanks for the review. It was quick! Especially when we speak
> of a RFC series. Very much appreciated.
> 
> On 4/2/24 20:11, Guenter Roeck wrote:
> > On Tue, Apr 02, 2024 at 04:11:41PM +0300, Matti Vaittinen wrote >> +static int init_wdg_hw(struct wdtbd96801 *w)
> > > +{
> > > +	u32 hw_margin[2];
> > > +	int count, ret;
> > > +	u32 hw_margin_max = BD96801_WDT_DEFAULT_MARGIN, hw_margin_min = 0;
> > > +
> > > +	count = device_property_count_u32(w->dev->parent, "rohm,hw-timeout-ms");
> > > +	if (count < 0 && count != -EINVAL)
> > > +		return count;
> > > +
> > > +	if (count > 0) {
> > > +		if (count > ARRAY_SIZE(hw_margin))
> > > +			return -EINVAL;
> > > +
> > > +		ret = device_property_read_u32_array(w->dev->parent,
> > > +						     "rohm,hw-timeout-ms",
> > > +						     &hw_margin[0], count);
> > > +		if (ret < 0)
> > > +			return ret;
> > > +
> > > +		if (count == 1)
> > > +			hw_margin_max = hw_margin[0];
> > > +
> > > +		if (count == 2) {
> > > +			hw_margin_max = hw_margin[1];
> > > +			hw_margin_min = hw_margin[0];
> > > +		}
> > > +	}
> > > +
> > > +	ret = bd96801_set_wdt_mode(w, hw_margin_max, hw_margin_min);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = device_property_match_string(w->dev->parent, "rohm,wdg-action",
> > > +					   "prstb");
> > > +	if (ret >= 0) {
> > > +		ret = regmap_update_bits(w->regmap, BD96801_REG_WD_CONF,
> > > +				 BD96801_WD_ASSERT_MASK,
> > > +				 BD96801_WD_ASSERT_RST);
> > > +		return ret;
> > > +	}
> > > +
> > > +	ret = device_property_match_string(w->dev->parent, "rohm,wdg-action",
> > > +					   "intb-only");
> > > +	if (ret >= 0) {
> > > +		ret = regmap_update_bits(w->regmap, BD96801_REG_WD_CONF,
> > > +				 BD96801_WD_ASSERT_MASK,
> > > +				 BD96801_WD_ASSERT_IRQ);
> > > +		return ret;
> > > +	}
> > 
> > I don't see the devicetree bindings documented in the series.
> 
> Seems like I have missed this WDG binding. But after reading your comment
> below, I am wondering if I should just drop the binding and default to
> "prstb" (shutdown should the feeding be skipped) - and leave the "intb-only"
> case for one who actually needs such.
> 
> > I am also a bit surprised that the interrupt isn't handled in the driver.
> > Please explain.
> 
> Basically, I just had no idea what the IRQ should do in the generic case. If
> we get an interrupt, it means the WDG feeding has failed. My thinking is
> that, what should happen is forced reset. I don't see how that can be done
> in reliably manner from an IRQ handler.
> 
> When the "prstb WDG action" is set (please, see the above DT binding
> handling), the PMIC shall shut down power outputs. This should get the
> watchdog's job done.
> 
> With the "intb-only"-option, PMIC will not turn off the power. I'd expect
> there to be some external HW connection which handles the reset by HW.
> 
> After all this being said, I wonder if I should just unconditionally
> configure the PMIC to always turn off the power (prstb option) should the
> feeding fail? Or do someone have some suggestion what the IRQ handler should
> do (except maybe print an error msg)?
> 

Other watchdog drivers call emergency_restart() if the watchdog times out
and triggers an interrupt. Are you saying this won't work for this system ?
If so, please explain.

Thanks,
Guenter