Received-SPF: pass (google.com: domain of linux-kernel+bounces-129726-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Sender: Guenter Roeck <groeck7@gmail.com>
Date: Wed, 3 Apr 2024 05:41:41 -0700
From: Guenter Roeck <linux@roeck-us.net>
To: Matti Vaittinen <mazziesaccount@gmail.com>
Cc: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>,
	Lee Jones <lee@kernel.org>,
	Wim Van Sebroeck <wim@linux-watchdog.org>,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-watchdog@vger.kernel.org
Subject: Re: [RFC PATCH 5/6] watchdog: ROHM BD96801 PMIC WDG driver
Message-ID: <d2ab33e6-4d3e-472a-b4d7-b703955989ba@roeck-us.net>
References: <cover.1712058690.git.mazziesaccount@gmail.com>
 <f8e743a6c49607de0dd7a27778383477e051b130.1712058690.git.mazziesaccount@gmail.com>
 <4fa3a64b-60fb-4e5e-8785-0f14da37eea2@roeck-us.net>
 <279336b3-f28d-48ee-a10f-47abba7b0b89@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <279336b3-f28d-48ee-a10f-47abba7b0b89@gmail.com>

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