[PATCH 02/11] drivers: misc: pm8916_pon: add glue driver

[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: add glue driver
>
> Initially, this driver binds the existing qcom_pwrkey driver to continue
> supporting pwrkey/resin buttons. In follow-up commits it will be further
> extended to bind a reboot-mode driver.
>
> The 'qcom,pm8916-pon' compatible is removed from qcom_pwrkey, since
> pm8916_pon owns it and handles binding qcom_pwrkey.
>
> Signed-off-by: Sam Day <me at samcday.com>
>
> drivers/button/button-qcom-pmic.c |  1 -
>  drivers/misc/Kconfig              |  7 ++++
>  drivers/misc/Makefile             |  1 +
>  drivers/misc/pm8916_pon.c         | 78 +++++++++++++++++++++++++++++++++++++++
>  4 files changed, 86 insertions(+), 1 deletion(-)

> diff --git a/drivers/misc/pm8916_pon.c b/drivers/misc/pm8916_pon.c
> @@ -0,0 +1,78 @@
> +struct pm8916_pon_priv {
> +     struct udevice *pmic;
> +     phys_addr_t base;
> +     u32 revision;
> +};

In addition to Casey's comments...

base here is an SPMI register offset within the parent PMIC, not a CPU
physical address - phys_addr_t is misleading. Please use u32 (or at
most fdt_addr_t, matching the dev_read_addr() return).

> diff --git a/drivers/misc/pm8916_pon.c b/drivers/misc/pm8916_pon.c
> @@ -0,0 +1,78 @@
> +static int pm8916_pon_bind(struct udevice *dev)
> +{
> +     int ret;
> +
> +     if (CONFIG_IS_ENABLED(BUTTON_QCOM_PMIC)) {
> +             ret = button_qcom_pmic_setup(dev);
> +             if (ret)
> +                     dev_warn(dev, "failed to bind qcom_pwrkey: %d\n", ret);
> +     }
> +
> +     return 0;
> +}

Two things. The CONFIG_IS_ENABLED() guard is redundant - the stub in
<button/qcom-pmic.h> already returns -ENOSYS when the option is off,
which is the whole point of the stub. Second, swallowing the error and
returning 0 means a real binding failure is invisible to the caller;
please log via dev_err() or propagate it. As written, -ENOSYS from the
stub would also trigger the warning on every probe when
BUTTON_QCOM_PMIC=n, which I suspect is not what you want.

> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> @@ -439,6 +439,13 @@ config SPL_PWRSEQ
> +config PM8916_PON
> +     bool "Enable PM8916 PON driver"
> +     depends on PMIC_QCOM && MISC
> +     help
> +       The PM8916 PMIC is a multifunction device that handles pwr/resin buttons,
> +       reboot type (soft/hard/poweroff) and reboot mode (recovery/bootloader).

The help text describes functionality this patch does not yet provide
- reboot type and reboot mode come in later patches. Please describe
what this driver does at this point (binds the pwrkey/resin buttons)
and expand the text in the patches that add the further functionality,
so bisection points at a coherent description.

Regards,
Simon