[PATCH 2/3] dm: button: add support for linux_code in button-gpio.c driver

[Dzmitry Sankouski]

I guess, u-boot button driver was created as minimal as possible
intentionally, to keep the size small.
It may be used in scripts without event code with button command. Some
dts has no event code, for example
sandbox.dtsi.

пн, 16 янв. 2023 г. в 12:53, Quentin Schulz
<quentin.schulz at theobroma-systems.com>:
>
> Hi Dzmitry,
>
> On 1/14/23 20:42, Dzmitry Sankouski wrote:
> > dev_read_u32 will fail, if linux,code is not found.
> > We shouldn't fail here, as linux,code is optional, so maybe dev_read_u32_default
> > with 0 default value, instead of negative error code?
> >
>
> No, 0 is an existing and valid code.
>
> FYI, linux,code is not optional for GPIO buttons in the Linux kernel and
> fails the device probing. What would be the usecase for GPIO buttons
> without a code?
>
> In the event we really want to allow no linux,cpde for GPIO button:
> It's fine if dev_read_u32 fails, you can use the return code as logic on
> what to do next.
>
> I believe you need to have a signed variable to store one invalid value
> (e.g. -EINVAL or -ENOTSUPP or something like that). You could also have
> a boolean stating whether a linux,code was found for this device. You
> could also use some pointer to an u32 which would be NULL if not
> present, but then you'd have to do memory allocation/freeing.
>
> > ср, 11 янв. 2023 г. в 18:48, Quentin Schulz
> > <quentin.schulz at theobroma-systems.com>:
> >>
> >> Hi Dzmitry,
> >>
> >> On 1/11/23 11:19, Dzmitry Sankouski wrote:
> >>> Linux event code may be used in input devices, using buttons.
> >>>
> >>> Signed-off-by: Dzmitry Sankouski <dsankouski at gmail.com>
> >>> ---
> >>>    drivers/button/button-gpio.c   | 20 ++++++++++++++++++++
> >>>    drivers/button/button-uclass.c | 10 ++++++++++
> >>>    include/button.h               | 16 ++++++++++++++++
> >>>    3 files changed, 46 insertions(+)
> >>>
> >>> diff --git a/drivers/button/button-gpio.c b/drivers/button/button-gpio.c
> >>> index dbb000622c..e6eff5c1da 100644
> >>> --- a/drivers/button/button-gpio.c
> >>> +++ b/drivers/button/button-gpio.c
> >>> @@ -13,6 +13,7 @@
> >>>
> >>>    struct button_gpio_priv {
> >>>        struct gpio_desc gpio;
> >>> +     u32 linux_code;
> >>>    };
> >>>
> >>>    static enum button_state_t button_gpio_get_state(struct udevice *dev)
> >>> @@ -29,10 +30,22 @@ static enum button_state_t button_gpio_get_state(struct udevice *dev)
> >>>        return ret ? BUTTON_ON : BUTTON_OFF;
> >>>    }
> >>>
> >>> +static u32 button_gpio_get_code(struct udevice *dev)
> >>> +{
> >>> +     struct button_gpio_priv *priv = dev_get_priv(dev);
> >>> +     u32 code = priv->linux_code;
> >>> +
> >>> +     if (!code)
> >>> +             return 0;
> >>> +
>
> I think we need something better than returning 0 in case there's no
> linux,code, since 0 is technically a valid linux,code.
>
> There are multiple ways of doing this, here are the two that comes to my
> mind right now.
>
> 1) have
> int (*get_code)(struct udevice *dev, u32 *code);
> instead of
> u32 (*get_code)(struct udevice *dev);
>
> and have the function return 0 on success or -EINVAL/-ENOTSUPP/whatever
> if linux,code was not set.
>
> 2) use a struct
> struct res {
>      int ret;
>      u32 code;
> };
> struct res (*get_code)(struct udevice *dev);
> instead of
> u32 (*get_code)(struct udevice *dev);
>
> and check on res.ret before reading res.code in your framework.
>
> Cheers,
> Quentin
>
> >>> +     return code;
> >>> +}
> >>> +
> >>>    static int button_gpio_probe(struct udevice *dev)
> >>>    {
> >>>        struct button_uc_plat *uc_plat = dev_get_uclass_plat(dev);
> >>>        struct button_gpio_priv *priv = dev_get_priv(dev);
> >>> +     u32 linux_code;
> >>>        int ret;
> >>>
> >>>        /* Ignore the top-level button node */
> >>> @@ -43,6 +56,12 @@ static int button_gpio_probe(struct udevice *dev)
> >>>        if (ret)
> >>>                return ret;
> >>>
> >>> +     linux_code = dev_read_u32_default(dev, "linux,code", -ENODATA);
> >>> +     debug("linux code value: %d, ret: %d", linux_code, ret);
> >>> +     if (ret)
> >>> +             return ret;
> >>
> >> ret is not modified here so it'll always pass even if it fails to parse
> >> dev_read_u32_default.
> >>
> >> I believe dev_read_u32_default incorrectly requests an int as last
> >> argument while it's going to fill it with u32 data on success.
> >> dev_read_u8/u16 get this correctly so that might just be an oversight.
> >>
> >> Please use dev_read_u32 instead:
> >>
> >> ret = dev_read_u32(dev, "linux,code", &priv->linux_code);
> >> debug("linux code value: %d, ret: %d", linux_code, ret);
> >> return ret;
> >>
> >> (no need to check the value of ret since it's the same as
> >> if (ret)
> >>       return ret;
> >>
> >> return 0;
> >> )
> >>
> >> Cheers,
> >> Quentin