From: Matti Vaittinen <mazziesaccount@gmail.com>
To: Guenter Roeck <linux@roeck-us.net>
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
Date: Wed, 3 Apr 2024 15:47:12 +0300 [thread overview]
Message-ID: <1d956aab-2892-4a2b-a4b3-0a93504668eb@gmail.com> (raw)
In-Reply-To: <d2ab33e6-4d3e-472a-b4d7-b703955989ba@roeck-us.net>
On 4/3/24 15:41, Guenter Roeck wrote:
> 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. If it works with systems using other devices, then it
should work (to the same extent) with systems using this PMIC. Thanks.
I'll add the IRQ handling to next version - but it may take a while as
I'm currently having some problems with the IRQs in general, and because
I'll wait for feedback from Mark to the regulator part.
Yours,
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
next prev parent reply other threads:[~2024-04-03 12:47 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-02 13:07 [RFC PATCH 0/6] Support ROHM BD96801 scalable PMIC Matti Vaittinen
2024-04-02 13:07 ` [RFC PATCH 1/6] dt-bindings: ROHM BD96801 PMIC regulators Matti Vaittinen
2024-04-02 13:08 ` [RFC PATCH 2/6] dt-bindings: mfd: bd96801 PMIC core Matti Vaittinen
2024-04-02 13:08 ` [RFC PATCH 3/6] mfd: support ROHM BD96801 " Matti Vaittinen
2024-04-11 14:38 ` Lee Jones
2024-04-12 5:40 ` Matti Vaittinen
2024-04-12 5:50 ` Matti Vaittinen
2024-04-12 7:23 ` Lee Jones
2024-04-12 8:58 ` Matti Vaittinen
2024-04-17 12:24 ` Lee Jones
2024-04-02 13:11 ` [RFC PATCH 5/6] watchdog: ROHM BD96801 PMIC WDG driver Matti Vaittinen
2024-04-02 16:15 ` Krzysztof Kozlowski
2024-04-02 17:11 ` Guenter Roeck
2024-04-03 6:34 ` Matti Vaittinen
2024-04-03 12:41 ` Guenter Roeck
2024-04-03 12:47 ` Matti Vaittinen [this message]
2024-04-03 13:26 ` Guenter Roeck
2024-04-02 13:12 ` [RFC PATCH 6/6] MAINTAINERS: Add ROHM BD96801 'scalable PMIC' entries Matti Vaittinen
2024-04-04 7:26 ` [RFC PATCH 0/6] Support ROHM BD96801 scalable PMIC Matti Vaittinen
2024-04-04 12:09 ` Mark Brown
2024-04-04 13:15 ` Matti Vaittinen
2024-04-05 9:19 ` Matti Vaittinen
2024-04-05 21:27 ` Mark Brown
2024-04-22 10:52 ` Matti Vaittinen
2024-05-09 5:08 ` Mark Brown
2024-05-09 7:03 ` Matti Vaittinen
2024-05-09 15:38 ` Mark Brown
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1d956aab-2892-4a2b-a4b3-0a93504668eb@gmail.com \
--to=mazziesaccount@gmail.com \
--cc=devicetree@vger.kernel.org \
--cc=lee@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-watchdog@vger.kernel.org \
--cc=linux@roeck-us.net \
--cc=matti.vaittinen@fi.rohmeurope.com \
--cc=wim@linux-watchdog.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).