[PATCH 07/11] reboot-mode: pm8916-pon: add new driver

[Simon Glass]

Hi Sam,

On 2026-06-06T08:47:32, Sam Day via B4 Relay
<devnull+me.samcday.com at kernel.org> wrote:
> reboot-mode: pm8916-pon: add new driver
>
> The PM8916 provides a PON_SOFT_RB_SPARE register that survives across
> reboots.
>
> Signed-off-by: Sam Day <me at samcday.com>
>
> drivers/misc/pm8916_pon.c                    |  7 +++
>  drivers/reboot-mode/Makefile                 |  1 +
>  drivers/reboot-mode/reboot-mode-pm8916-pon.c | 88 ++++++++++++++++++++++++++++
>  3 files changed, 96 insertions(+)

> diff --git a/drivers/reboot-mode/reboot-mode-pm8916-pon.c b/drivers/reboot-mode/reboot-mode-pm8916-pon.c
> @@ -0,0 +1,88 @@
> +static int pm8916_pon_reboot_mode_probe(struct udevice *dev)
> +{
> +     struct pm8916_pon_reboot_mode_priv *priv = dev_get_priv(dev);
> +     struct udevice *pon = dev_get_parent(dev);
> +     fdt_addr_t base;
> +
> +     /* this driver only works as a child of pm8916_pon */
> +     if (!pon || !pon->driver || strcmp(pon->driver->name, 'pm8916_pon'))
> +             return -EINVAL;

Matching the parent by driver-name string is brittle - please compare
against DM_DRIVER_GET(pm8916_pon). Or better, since this driver is
only ever bound by the glue, drop the sanity check and trust the
parent.

> diff --git a/drivers/reboot-mode/reboot-mode-pm8916-pon.c b/drivers/reboot-mode/reboot-mode-pm8916-pon.c
> @@ -0,0 +1,88 @@
> +     priv->pmic = dev_get_parent(pon);
> +     if (!priv->pmic) {
> +             dev_err(dev, "PMIC driver not found\n");
> +             return -EINVAL;
> +     }
> +
> +     base = dev_read_addr(pon);
> +     if (base == FDT_ADDR_T_NONE) {
> +             dev_err(dev, "missing PON reg base\n");
> +             return -EINVAL;
> +     }
> +     priv->base = base;

This duplicates work already done by pm8916_pon_probe(), and walks the
parent's internals. Since pm8916_pon_priv already holds pmic and base,
please expose a small accessor (or share the struct via the existing
pm8916_pon.h) and read them from the parent's priv. That also removes
the need for the driver-name check above.

> diff --git a/drivers/reboot-mode/reboot-mode-pm8916-pon.c b/drivers/reboot-mode/reboot-mode-pm8916-pon.c
> @@ -0,0 +1,88 @@
> +static int pm8916_pon_reboot_mode_set(struct udevice *dev, u32 rebootmode)
> +{
> +     struct pm8916_pon_reboot_mode_priv *priv = dev_get_priv(dev);
> +     uint mask = GENMASK(7, GEN1_REASON_SHIFT);
> +     int ret;
> +
> +     ret = pmic_clrsetbits(priv->pmic, priv->base + PON_SOFT_RB_SPARE, mask,
> +                           rebootmode << GEN1_REASON_SHIFT);

pmic_clrsetbits() does (val & ~clr) | set without masking set to clr.
If the caller ever passes a mode_id wider than 6 bits this silently
writes garbage outside the field. Please AND the shifted value with
mask before passing it in.

> diff --git a/drivers/misc/pm8916_pon.c b/drivers/misc/pm8916_pon.c
> @@ -142,6 +142,13 @@ static int pm8916_pon_bind(struct udevice *dev)
> +     if (CONFIG_IS_ENABLED(DM_REBOOT_MODE)) {
> +             ret = device_bind_driver_to_node(dev, 'pm8916_pon_reboot_mode',
> +                                              'reboot_mode', dev_ofnode(dev), NULL);
> +             if (ret)
> +                     dev_warn(dev, "failed to bind reboot_mode: %d\n", ret);
> +     }

Binding the child to the parent's own ofnode means
dm_reboot_mode_pre_probe() iterates the pm8916_pon node looking for
mode-* properties. That works only if the board DT places them
directly on the PON node - please mention this expectation in the
commit message (or a binding doc snippet)...from what I can tell this
is different from the kernel binding where reboot-mode is a separate
subnode.

>     reboot-mode: pm8916-pon: add new driver
>
>     The PM8916 provides a PON_SOFT_RB_SPARE register that survives across
>     reboots.

The commit message is very thin - it states a fact about the hardware
but does not say what the patch does. Please describe that this adds a
reboot-mode uclass driver bound as a child of pm8916_pon, how it
relates to the existing NVMEM-based approach, and the expected DT
layout (mode-* properties on the PON node).

Regards,
Simon