No subject
Tue Feb 17 01:45:02 AEDT 2026
and that
I cannot use Device Tree properties to describe software behavior, and DT
should only describe hardware.
Given that, I am trying to understand what would be the correct upstream
way to expose the different reset causes that the GCR reports. The watchdog
framework provides only a few standardized bootstatus flags, and I would
like to check whether it is acceptable to map the different reset causes
into these existing flags.
For example, conceptually:
- WDIOF_CARDRESET =E2=86=92 power=E2=80=91on reset
- WDIOF_OVERHEAT =E2=86=92 core reset
- WDIOF_FANFAULT =E2=86=92 watchdog reset
- WDIOF_EXTERN1 =E2=86=92 SW0 reset
- WDIOF_EXTERN2 =E2=86=92 SW1 reset
- WDIOF_POWERUNDER =E2=86=92 SW2 reset
- WDIOF_POWEROVER =E2=86=92 SW3 reset
Is such a mapping acceptable?
Any guidance on the expected upstream approach would be greatly appreciated=
.
Thanks,
Tomer
On Tue, 10 Feb 2026 at 18:02, Guenter Roeck <linux at roeck-us.net> wrote:
> On 2/10/26 05:38, Tomer Maimon wrote:
> > Add reset status detection for NPCM watchdog driver on both NPCM7XX and
> > NPCM8XX platforms. Implement GCR register integration via syscon for
> > reset status detection and configurable reset type mapping via device
> > tree properties.
> >
> > Signed-off-by: Tomer Maimon <tmaimon77 at gmail.com>
> > ---
> > drivers/watchdog/npcm_wdt.c | 110 +++++++++++++++++++++++++++++++++++=
+
> > 1 file changed, 110 insertions(+)
> >
> > diff --git a/drivers/watchdog/npcm_wdt.c b/drivers/watchdog/npcm_wdt.c
> > index e62ea054bc61..ebece5d6240a 100644
> > --- a/drivers/watchdog/npcm_wdt.c
> > +++ b/drivers/watchdog/npcm_wdt.c
> > @@ -12,9 +12,25 @@
> > #include <linux/platform_device.h>
> > #include <linux/slab.h>
> > #include <linux/watchdog.h>
> > +#include <linux/regmap.h>
> > +#include <linux/mfd/syscon.h>
> > +
> > +#define NPCM7XX_RESSR_OFFSET 0x6C
> > +#define NPCM7XX_INTCR2_OFFSET 0x60
> >
> > #define NPCM_WTCR 0x1C
> >
> > +#define NPCM7XX_PORST BIT(31)
> > +#define NPCM7XX_CORST BIT(30)
> > +#define NPCM7XX_WD0RST BIT(29)
> > +#define NPCM7XX_WD1RST BIT(24)
> > +#define NPCM7XX_WD2RST BIT(23)
> > +#define NPCM7XX_SWR1RST BIT(28)
> > +#define NPCM7XX_SWR2RST BIT(27)
> > +#define NPCM7XX_SWR3RST BIT(26)
> > +#define NPCM7XX_SWR4RST BIT(25)
> > +#define NPCM8XX_RST (GENMASK(31, 23) | GENMASK(15, 12))
> > +
> > #define NPCM_WTCLK (BIT(10) | BIT(11)) /* Clock divider */
> > #define NPCM_WTE BIT(7) /* Enable */
> > #define NPCM_WTIE BIT(6) /* Enable irq */
> > @@ -45,6 +61,9 @@ struct npcm_wdt {
> > struct watchdog_device wdd;
> > void __iomem *reg;
> > struct clk *clk;
> > + u32 card_reset;
> > + u32 ext1_reset;
> > + u32 ext2_reset;
> > };
> >
> > static inline struct npcm_wdt *to_npcm_wdt(struct watchdog_device *wd=
d)
> > @@ -185,6 +204,95 @@ static const struct watchdog_ops npcm_wdt_ops =3D =
{
> > .restart =3D npcm_wdt_restart,
> > };
> >
> > +static u32 npcm_wdt_reset_type(const char *reset_type)
> > +{
> > + if (!strcmp(reset_type, "porst"))
> > + return NPCM7XX_PORST;
> > + else if (!strcmp(reset_type, "corst"))
> > + return NPCM7XX_CORST;
> > + else if (!strcmp(reset_type, "wd0"))
> > + return NPCM7XX_WD0RST;
> > + else if (!strcmp(reset_type, "wd1"))
> > + return NPCM7XX_WD1RST;
> > + else if (!strcmp(reset_type, "wd2"))
> > + return NPCM7XX_WD2RST;
> > + else if (!strcmp(reset_type, "sw1"))
> > + return NPCM7XX_SWR1RST;
> > + else if (!strcmp(reset_type, "sw2"))
> > + return NPCM7XX_SWR2RST;
> > + else if (!strcmp(reset_type, "sw3"))
> > + return NPCM7XX_SWR3RST;
> > + else if (!strcmp(reset_type, "sw4"))
> > + return NPCM7XX_SWR4RST;
> > +
> > + return 0;
> > +}
> > +
> > +static void npcm_get_reset_status(struct npcm_wdt *wdt, struct device
> *dev)
> > +{
> > + const char *card_reset_type;
> > + const char *ext1_reset_type;
> > + const char *ext2_reset_type;
> > + struct regmap *gcr_regmap;
> > + u32 rstval, ressrval;
> > + int ret;
> > +
> > + gcr_regmap =3D syscon_regmap_lookup_by_phandle(dev->of_node,
> "syscon");
> > + if (IS_ERR(gcr_regmap)) {
> > + dev_warn(dev, "Failed to find gcr syscon, WD reset status
> not supported\n");
>
> A warning is quite strong here, given that this is new code and the
> syscon reference may not exist in existing devicetree files. notice
> should be good enough.
>
> > + return;
> > + }
> > +
> > + ret =3D of_property_read_string(dev->of_node,
> > + "nuvoton,card-reset-type",
> > + &card_reset_type);
> > + if (ret)
> > + wdt->card_reset =3D NPCM7XX_PORST;
> > + else
> > + wdt->card_reset =3D npcm_wdt_reset_type(card_reset_type);
> > +
> > + ret =3D of_property_read_string(dev->of_node,
> > + "nuvoton,ext1-reset-type",
> > + &ext1_reset_type);
> > + if (ret)
> > + wdt->ext1_reset =3D 0;
>
> wdt is zero-allocated, so setting those variables to 0 is not necessary.
>
> > + else
> > + wdt->ext1_reset =3D npcm_wdt_reset_type(ext1_reset_type);
> > +
> > + ret =3D of_property_read_string(dev->of_node,
> > + "nuvoton,ext2-reset-type",
> > + &ext2_reset_type);
> > + if (ret)
> > + wdt->ext2_reset =3D 0;
> > + else
> > + wdt->ext2_reset =3D npcm_wdt_reset_type(ext2_reset_type);
> > +
> > + regmap_read(gcr_regmap, NPCM7XX_INTCR2_OFFSET, &rstval);
>
> This warrants an explanation/comment: Why is it not necessary to check
> the return value of the regmap operations ?
>
> > + /* prefer the most specific SoC first */
> > + if (of_device_is_compatible(dev->of_node, "nuvoton,npcm845-wdt"))=
{
> > + regmap_write(gcr_regmap, NPCM7XX_INTCR2_OFFSET,
> > + rstval & ~NPCM8XX_RST);
> > + } else if (of_device_is_compatible(dev->of_node,
> "nuvoton,npcm750-wdt")) {
> > + if ((rstval & NPCM7XX_PORST) =3D=3D 0) {
> > + rstval =3D NPCM7XX_PORST;
> > + regmap_write(gcr_regmap, NPCM7XX_INTCR2_OFFSET,
> > + rstval | NPCM7XX_PORST);
>
> That "| NPCM7XX_PORST" is pretty pointless here since rstval was
> just set to that value.
>
> > + } else {
> > + rstval =3D 0;
> > + }
>
> Another comment needed: This negates NPCM7XX_PORST and otherwise clear
> rstval. The reason is not immediately (or, rather, at all) obvious.
>
> > + regmap_read(gcr_regmap, NPCM7XX_RESSR_OFFSET, &ressrval);
> > + rstval |=3D ressrval;
> > + regmap_write(gcr_regmap, NPCM7XX_RESSR_OFFSET, ressrval);
> > + }
>
> If the device is not compatible to either chip, retval is just passed
> on and nothing is written to the chip. That warrants another comment.
>
> [ Yes, I see that the driver does not currently support another chip.
>
> > +
> > + if (rstval & wdt->card_reset)
> > + wdt->wdd.bootstatus |=3D WDIOF_CARDRESET;
> > + if (rstval & wdt->ext1_reset)
> > + wdt->wdd.bootstatus |=3D WDIOF_EXTERN1;
> > + if (rstval & wdt->ext2_reset)
> > + wdt->wdd.bootstatus |=3D WDIOF_EXTERN2;
> > +}
> > +
> > static int npcm_wdt_probe(struct platform_device *pdev)
> > {
> > struct device *dev =3D &pdev->dev;
> > @@ -208,6 +316,8 @@ static int npcm_wdt_probe(struct platform_device
> *pdev)
> > if (irq < 0)
> > return irq;
> >
> > + npcm_get_reset_status(wdt, dev);
> > +
> > wdt->wdd.info =3D &npcm_wdt_info;
> > wdt->wdd.ops =3D &npcm_wdt_ops;
> > wdt->wdd.min_timeout =3D 1;
>
>
--000000000000b4d51b064af1cd93
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
<div dir=3D"ltr"><div style=3D"font-family:"Segoe UI";font-size:1=
4px;line-height:20px"><p>Hi Guenter,</p>
<p>Thanks for your review.</p>
<p>From your comments and from Krzysztof=E2=80=99s earlier feedback, I unde=
rstand that I cannot use Device Tree properties to describe software behavi=
or, and DT should only describe hardware.</p>
<p>Given that, I am trying to understand what would be the correct upstream=
way to expose the different reset causes that the GCR reports. The watchdo=
g framework provides only a few standardized <code>bootstatus</code> flags,=
and I would like to check whether it is acceptable to map the different re=
set causes into these existing flags.</p>
<p>For example, conceptually:</p>
<ul>
<li><code>WDIOF_CARDRESET</code> =E2=86=92 power=E2=80=91on reset</li>
<li><code>WDIOF_OVERHEAT</code> =E2=86=92 core reset</li>
<li><code>WDIOF_FANFAULT</code> =E2=86=92 watchdog reset</li>
<li><code>WDIOF_EXTERN1</code> =E2=86=92 SW0 reset</li>
<li><code>WDIOF_EXTERN2</code> =E2=86=92 SW1 reset</li>
<li><code>WDIOF_POWERUNDER</code> =E2=86=92 SW2 reset</li>
<li><code>WDIOF_POWEROVER</code> =E2=86=92 SW3 reset</li>
</ul>
<p>Is such a mapping acceptable?</p>
<p>Any guidance on the expected upstream approach would be greatly apprecia=
ted.</p>
<p>Thanks,<br>
Tomer</p>
</div><div><br class=3D"gmail-Apple-interchange-newline"></div></div><br><d=
iv class=3D"gmail_quote"><div dir=3D"ltr" class=3D"gmail_attr">On Tue, 10 F=
eb 2026 at 18:02, Guenter Roeck <<a href=3D"mailto:linux at roeck-us.net" t=
arget=3D"_blank">linux at roeck-us.net</a>> wrote:<br></div><blockquote cla=
ss=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid =
rgb(204,204,204);padding-left:1ex">On 2/10/26 05:38, Tomer Maimon wrote:<br=
>
> Add reset status detection for NPCM watchdog driver on both NPCM7XX an=
d<br>
> NPCM8XX platforms. Implement GCR register integration via syscon for<b=
r>
> reset status detection and configurable reset type mapping via device<=
br>
> tree properties.<br>
> <br>
> Signed-off-by: Tomer Maimon <<a href=3D"mailto:tmaimon77 at gmail.com"=
target=3D"_blank">tmaimon77 at gmail.com</a>><br>
> ---<br>
>=C2=A0 =C2=A0drivers/watchdog/npcm_wdt.c | 110 ++++++++++++++++++++++++=
++++++++++++<br>
>=C2=A0 =C2=A01 file changed, 110 insertions(+)<br>
> <br>
> diff --git a/drivers/watchdog/npcm_wdt.c b/drivers/watchdog/npcm_wdt.c=
<br>
> index e62ea054bc61..ebece5d6240a 100644<br>
> --- a/drivers/watchdog/npcm_wdt.c<br>
> +++ b/drivers/watchdog/npcm_wdt.c<br>
> @@ -12,9 +12,25 @@<br>
>=C2=A0 =C2=A0#include <linux/platform_device.h><br>
>=C2=A0 =C2=A0#include <linux/slab.h><br>
>=C2=A0 =C2=A0#include <linux/watchdog.h><br>
> +#include <linux/regmap.h><br>
> +#include <linux/mfd/syscon.h><br>
> +<br>
> +#define NPCM7XX_RESSR_OFFSET 0x6C<br>
> +#define NPCM7XX_INTCR2_OFFSET=C2=A0 =C2=A0 =C2=A0 =C2=A0 0x60<br>
>=C2=A0 =C2=A0<br>
>=C2=A0 =C2=A0#define NPCM_WTCR=C2=A0 =C2=A00x1C<br>
>=C2=A0 =C2=A0<br>
> +#define NPCM7XX_PORST=C2=A0 =C2=A0 =C2=A0 =C2=A0 BIT(31)<br>
> +#define NPCM7XX_CORST=C2=A0 =C2=A0 =C2=A0 =C2=A0 BIT(30)<br>
> +#define NPCM7XX_WD0RST=C2=A0 =C2=A0 =C2=A0 =C2=A0BIT(29)<br>
> +#define NPCM7XX_WD1RST=C2=A0 =C2=A0 =C2=A0 =C2=A0BIT(24)<br>
> +#define NPCM7XX_WD2RST=C2=A0 =C2=A0 =C2=A0 =C2=A0BIT(23)<br>
> +#define NPCM7XX_SWR1RST=C2=A0 =C2=A0 =C2=A0 BIT(28)<br>
> +#define NPCM7XX_SWR2RST=C2=A0 =C2=A0 =C2=A0 BIT(27)<br>
> +#define NPCM7XX_SWR3RST=C2=A0 =C2=A0 =C2=A0 BIT(26)<br>
> +#define NPCM7XX_SWR4RST=C2=A0 =C2=A0 =C2=A0 BIT(25)<br>
> +#define NPCM8XX_RST=C2=A0 (GENMASK(31, 23) | GENMASK(15, 12))<br>
> +<br>
>=C2=A0 =C2=A0#define NPCM_WTCLK=C2=A0 (BIT(10) | BIT(11))=C2=A0 =C2=A0 =
=C2=A0/* Clock divider */<br>
>=C2=A0 =C2=A0#define NPCM_WTE=C2=A0 =C2=A0 BIT(7)=C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /* Enable */<br>
>=C2=A0 =C2=A0#define NPCM_WTIE=C2=A0 =C2=A0BIT(6)=C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /* Enable irq */<br>
> @@ -45,6 +61,9 @@ struct npcm_wdt {<br>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0struct watchdog_device=C2=A0 wdd;<br>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0void __iomem=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 *reg;<br>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0struct clk=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
=C2=A0 =C2=A0 *clk;<br>
> +=C2=A0 =C2=A0 =C2=A0u32=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0card_reset;<br>
> +=C2=A0 =C2=A0 =C2=A0u32=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0ext1_reset;<br>
> +=C2=A0 =C2=A0 =C2=A0u32=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0ext2_reset;<br>
>=C2=A0 =C2=A0};<br>
>=C2=A0 =C2=A0<br>
>=C2=A0 =C2=A0static inline struct npcm_wdt *to_npcm_wdt(struct watchdog=
_device *wdd)<br>
> @@ -185,6 +204,95 @@ static const struct watchdog_ops npcm_wdt_ops =3D=
{<br>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0.restart =3D npcm_wdt_restart,<br>
>=C2=A0 =C2=A0};<br>
>=C2=A0 =C2=A0<br>
> +static u32 npcm_wdt_reset_type(const char *reset_type)<br>
> +{<br>
> +=C2=A0 =C2=A0 =C2=A0if (!strcmp(reset_type, "porst"))<br>
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return NPCM7XX_PORST;=
<br>
> +=C2=A0 =C2=A0 =C2=A0else if (!strcmp(reset_type, "corst"))<=
br>
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return NPCM7XX_CORST;=
<br>
> +=C2=A0 =C2=A0 =C2=A0else if (!strcmp(reset_type, "wd0"))<br=
>
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return NPCM7XX_WD0RST=
;<br>
> +=C2=A0 =C2=A0 =C2=A0else if (!strcmp(reset_type, "wd1"))<br=
>
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return NPCM7XX_WD1RST=
;<br>
> +=C2=A0 =C2=A0 =C2=A0else if (!strcmp(reset_type, "wd2"))<br=
>
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return NPCM7XX_WD2RST=
;<br>
> +=C2=A0 =C2=A0 =C2=A0else if (!strcmp(reset_type, "sw1"))<br=
>
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return NPCM7XX_SWR1RS=
T;<br>
> +=C2=A0 =C2=A0 =C2=A0else if (!strcmp(reset_type, "sw2"))<br=
>
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return NPCM7XX_SWR2RS=
T;<br>
> +=C2=A0 =C2=A0 =C2=A0else if (!strcmp(reset_type, "sw3"))<br=
>
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return NPCM7XX_SWR3RS=
T;<br>
> +=C2=A0 =C2=A0 =C2=A0else if (!strcmp(reset_type, "sw4"))<br=
>
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return NPCM7XX_SWR4RS=
T;<br>
> +<br>
> +=C2=A0 =C2=A0 =C2=A0return 0;<br>
> +}<br>
> +<br>
> +static void npcm_get_reset_status(struct npcm_wdt *wdt, struct device=
*dev)<br>
> +{<br>
> +=C2=A0 =C2=A0 =C2=A0const char *card_reset_type;<br>
> +=C2=A0 =C2=A0 =C2=A0const char *ext1_reset_type;<br>
> +=C2=A0 =C2=A0 =C2=A0const char *ext2_reset_type;<br>
> +=C2=A0 =C2=A0 =C2=A0struct regmap *gcr_regmap;<br>
> +=C2=A0 =C2=A0 =C2=A0u32 rstval, ressrval;<br>
> +=C2=A0 =C2=A0 =C2=A0int ret;<br>
> +<br>
> +=C2=A0 =C2=A0 =C2=A0gcr_regmap =3D syscon_regmap_lookup_by_phandle(de=
v->of_node, "syscon");<br>
> +=C2=A0 =C2=A0 =C2=A0if (IS_ERR(gcr_regmap)) {<br>
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0dev_warn(dev, "F=
ailed to find gcr syscon, WD reset status not supported\n");<br>
<br>
A warning is quite strong here, given that this is new code and the<br>
syscon reference may not exist in existing devicetree files. notice<br>
should be good enough.<br>
<br>
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return;<br>
> +=C2=A0 =C2=A0 =C2=A0}<br>
> +<br>
> +=C2=A0 =C2=A0 =C2=A0ret =3D of_property_read_string(dev->of_node,<=
br>
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0"nuvoton,card-=
reset-type",<br>
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0&card_reset_typ=
e);<br>
> +=C2=A0 =C2=A0 =C2=A0if (ret)<br>
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0wdt->card_reset =
=3D NPCM7XX_PORST;<br>
> +=C2=A0 =C2=A0 =C2=A0else<br>
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0wdt->card_reset =
=3D npcm_wdt_reset_type(card_reset_type);<br>
> +<br>
> +=C2=A0 =C2=A0 =C2=A0ret =3D of_property_read_string(dev->of_node,<=
br>
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0"nuvoton,ext1-=
reset-type",<br>
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0&ext1_reset_typ=
e);<br>
> +=C2=A0 =C2=A0 =C2=A0if (ret)<br>
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0wdt->ext1_reset =
=3D 0;<br>
<br>
wdt is zero-allocated, so setting those variables to 0 is not necessary.<br=
>
<br>
> +=C2=A0 =C2=A0 =C2=A0else<br>
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0wdt->ext1_reset =
=3D npcm_wdt_reset_type(ext1_reset_type);<br>
> +<br>
> +=C2=A0 =C2=A0 =C2=A0ret =3D of_property_read_string(dev->of_node,<=
br>
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0"nuvoton,ext2-=
reset-type",<br>
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0&ext2_reset_typ=
e);<br>
> +=C2=A0 =C2=A0 =C2=A0if (ret)<br>
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0wdt->ext2_reset =
=3D 0;<br>
> +=C2=A0 =C2=A0 =C2=A0else<br>
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0wdt->ext2_reset =
=3D npcm_wdt_reset_type(ext2_reset_type);<br>
> +<br>
> +=C2=A0 =C2=A0 =C2=A0regmap_read(gcr_regmap, NPCM7XX_INTCR2_OFFSET, &a=
mp;rstval);<br>
<br>
This warrants an explanation/comment: Why is it not necessary to check<br>
the return value of the regmap operations ?<br>
<br>
> +=C2=A0 =C2=A0 =C2=A0/* prefer the most specific SoC first */<br>
> +=C2=A0 =C2=A0 =C2=A0if (of_device_is_compatible(dev->of_node, &quo=
t;nuvoton,npcm845-wdt")) {<br>
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0regmap_write(gcr_regm=
ap, NPCM7XX_INTCR2_OFFSET,<br>
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
=C2=A0 =C2=A0 =C2=A0 rstval & ~NPCM8XX_RST);<br>
> +=C2=A0 =C2=A0 =C2=A0} else if (of_device_is_compatible(dev->of_nod=
e, "nuvoton,npcm750-wdt")) {<br>
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if ((rstval & NPC=
M7XX_PORST) =3D=3D 0) {<br>
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
=C2=A0rstval =3D NPCM7XX_PORST;<br>
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
=C2=A0regmap_write(gcr_regmap, NPCM7XX_INTCR2_OFFSET,<br>
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 rstval | NPCM7XX_PORST);<=
br>
<br>
That "| NPCM7XX_PORST" is pretty pointless here since rstval was<=
br>
just set to that value.<br>
<br>
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0} else {<br>
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
=C2=A0rstval =3D 0;<br>
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0}<br>
<br>
Another comment needed: This negates NPCM7XX_PORST and otherwise clear<br>
rstval. The reason is not immediately (or, rather, at all) obvious.<br>
<br>
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0regmap_read(gcr_regma=
p, NPCM7XX_RESSR_OFFSET, &ressrval);<br>
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0rstval |=3D ressrval;=
<br>
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0regmap_write(gcr_regm=
ap, NPCM7XX_RESSR_OFFSET, ressrval);<br>
> +=C2=A0 =C2=A0 =C2=A0}<br>
<br>
If the device is not compatible to either chip, retval is just passed<br>
on and nothing is written to the chip. That warrants another comment.<br>
<br>
[ Yes, I see that the driver does not currently support another chip.<br>
<br>
> +<br>
> +=C2=A0 =C2=A0 =C2=A0if (rstval & wdt->card_reset)<br>
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0wdt->wdd.bootstatu=
s |=3D WDIOF_CARDRESET;<br>
> +=C2=A0 =C2=A0 =C2=A0if (rstval & wdt->ext1_reset)<br>
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0wdt->wdd.bootstatu=
s |=3D WDIOF_EXTERN1;<br>
> +=C2=A0 =C2=A0 =C2=A0if (rstval & wdt->ext2_reset)<br>
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0wdt->wdd.bootstatu=
s |=3D WDIOF_EXTERN2;<br>
> +}<br>
> +<br>
>=C2=A0 =C2=A0static int npcm_wdt_probe(struct platform_device *pdev)<br=
>
>=C2=A0 =C2=A0{<br>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0struct device *dev =3D &pdev->dev;<br=
>
> @@ -208,6 +316,8 @@ static int npcm_wdt_probe(struct platform_device *=
pdev)<br>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0if (irq < 0)<br>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return irq;<br>
>=C2=A0 =C2=A0<br>
> +=C2=A0 =C2=A0 =C2=A0npcm_get_reset_status(wdt, dev);<br>
> +<br>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0wdt-><a href=3D"http://wdd.info" rel=3D"n=
oreferrer" target=3D"_blank">wdd.info</a> =3D &npcm_wdt_info;<br>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0wdt->wdd.ops =3D &npcm_wdt_ops;<br>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0wdt->wdd.min_timeout =3D 1;<br>
<br>
</blockquote></div>
--000000000000b4d51b064af1cd93--
More information about the openbmc
mailing list