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

[Quentin Schulz]

Hi Dzmitry,

On 1/16/23 12:19, Dzmitry Sankouski wrote:
> I guess, u-boot button driver was created as minimal as possible
> intentionally, to keep the size small.

I'm not sure that we're *that* size constrained in U-Boot proper.

> It may be used in scripts without event code with button command. Some
> dts has no event code, for example
> sandbox.dtsi.
> 

sandbox.dtsi is only for testing purposes, it's fine to add one there. I 
believe we can have linux,code = <BTN_1>; for btn1 and linux,code = 
<BTN_2>; for the btn2.
You can do the same to test.dtsi probably.

I found three other dts without a linux,code entry:
- arch/arm/dts/rk3288-popmetal.dtsi gpio-keys>power. Upstream linux has 
one though, c.f. 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/rk3288-popmetal.dts#n35. 
Just needs to be synchronized
- arch/arm/dts/rk3288-tinker.dtsi gpio-keys>button0. Upstream linux has 
one though, c.f. 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/rk3288-tinker.dtsi#n36. 
Just needs to be synced
- arch/arm/dts/am3517-evm-ui.dtsi expander-keys>record. Nothing in 
upstream kernel either. I suspect, linux,code = <KEY_RECORD>; should be 
enough.

Note that linux,code is enforced in Linux both by the driver and also by 
the DT binding, c.f. 
https://www.kernel.org/doc/Documentation/devicetree/bindings/input/gpio-keys.yaml.

Cheers,
Quentin

> пн, 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