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

[Quentin Schulz]

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