[PATCH 04/11] drivers: misc: pm8916_pon: support setting reboot type

[Simon Glass]

Hi Sam,

On 2026-06-06T08:47:32, Sam Day via B4 Relay
<devnull+me.samcday.com at kernel.org> wrote:
> drivers: misc: pm8916_pon: support setting reboot type
>
> This is to be used in conjunction with PSHOLD sysreset driver.
>
> This code manages the RST_CTL register block, which PM8916 uses to
> decide what action to take when PS_HOLD is de-asserted.
>
> Signed-off-by: Sam Day <me at samcday.com>
>
> drivers/misc/pm8916_pon.c | 82 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/pm8916_pon.h      | 18 +++++++++++
>  2 files changed, 100 insertions(+)

> diff --git a/drivers/misc/pm8916_pon.c b/drivers/misc/pm8916_pon.c
> @@ -10,16 +10,98 @@
> +     ret = uclass_get_device_by_driver(UCLASS_MISC,
> +                                       DM_DRIVER_GET(pm8916_pon), &dev);
> +     if (ret) {
> +             pr_warn("couldn't find pm8916_pon device: %d\n", ret);
> +             return ret;
> +     }
> +     if (!dev) {
> +             dev_warn(dev, "couldn't find pm8916_pon device\n");
> +             return -ENODEV;
> +     }

As Casey mentions, uclass_get_device_by_driver() either returns an
error or a valid dev - the second check is dead code, and dev_warn()
with a NULL dev would dereference it. Please drop the !dev block.

> diff --git a/drivers/misc/pm8916_pon.c b/drivers/misc/pm8916_pon.c
> @@ -10,16 +10,98 @@
> +     ret = pmic_clrsetbits(priv->pmic, priv->base + enable_reg,
> +                           PON_PS_HOLD_ENABLE, 0);
> +     if (ret) {
> +             dev_warn(dev, "clear PON_PS_HOLD_ENABLE failed: %d\n", ret);
> +             return ret;
> +     }
> +
> +     /* Delay needed for disable to kick in. */
> +     /* Kernel uses usleep_range(100, 1000), lk2nd waits 300usec */
> +     udelay(300);
> +
> +     ret = pmic_clrsetbits(priv->pmic, priv->base + PON_PS_HOLD_RST_CTL,
> +                           PON_PS_HOLD_TYPE_MASK, pmic_reset_type);
> +     if (ret) {
> +             dev_warn(dev, "RST_CTL set failed: %d\n", ret);
> +             return ret;
> +     }

If the type write fails after the enable has been cleared, the PON is
left disabled and the next PS_HOLD de-assertion will do something
undefined. Please attempt to restore PON_PS_HOLD_ENABLE on the error
path, or at least log loudly that the device has been left
half-configured.

> diff --git a/drivers/misc/pm8916_pon.c b/drivers/misc/pm8916_pon.c
> @@ -10,16 +10,98 @@
> +     switch (reset_type) {
> +     case SYSRESET_POWER_OFF:
> +             pmic_reset_type = PON_PS_HOLD_TYPE_SHUTDOWN;
> +             break;
> +     case SYSRESET_COLD:
> +     case SYSRESET_POWER:
> +             pmic_reset_type = PON_PS_HOLD_TYPE_HARD_RESET;
> +             break;
> +     default:
> +             pmic_reset_type = PON_PS_HOLD_TYPE_WARM_RESET;
> +     }

Silently mapping every unknown reset_type to a warm reset feels risky
- SYSRESET_POWER_OFF_WAIT or future additions would unexpectedly
reboot instead of erroring out. Please either return -ENOTSUPP for
unhandled values, or list each explicitly so adding a new enumerator
forces a compile-time review.

> diff --git a/drivers/misc/pm8916_pon.c b/drivers/misc/pm8916_pon.c
> @@ -10,16 +10,98 @@
>     This is to be used in conjunction with PSHOLD sysreset driver.
>
>     This code manages the RST_CTL register block, which PM8916 uses to
>     decide what action to take when PS_HOLD is de-asserted.

Please expand the commit message - mention that this function is
called from the sysreset driver (patch 5) and briefly describe the
rev-0 vs rev-1 register layout difference that the code handles, since
that is the main subtlety here.

Regards,
Simon