Re: [PATCH] ARM: dts: microchip: sama7d65: Add GPIO buttons and LEDs

[Ryan Wanner <ryan.wanner@microchip.com>]

On 9/11/25 00:00, Alexander Dahl wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Hello Ryan,
> 
> Am Wed, Sep 10, 2025 at 10:16:03AM -0700 schrieb Ryan Wanner:
>> On 9/9/25 23:25, Alexander Dahl wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> Hello Ryan,
>>>
>>> Am Wed, Sep 10, 2025 at 08:20:28AM +0200 schrieb Alexander Dahl:
>>>> Hello Ryan,
>>>>
>>>> Am Tue, Sep 09, 2025 at 09:08:38AM -0700 schrieb Ryan.Wanner@microchip.com:
>>>>> From: Ryan Wanner <Ryan.Wanner@microchip.com>
>>>>>
>>>>> Add the USER button as a GPIO input as well as add the LEDs and enable
>>>>> the blue LED as a heartbeat.
>>>>>
>>>>> Signed-off-by: Ryan Wanner <Ryan.Wanner@microchip.com>
>>>>> ---
>>>>>  .../dts/microchip/at91-sama7d65_curiosity.dts | 49 +++++++++++++++++++
>>>>>  1 file changed, 49 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm/boot/dts/microchip/at91-sama7d65_curiosity.dts b/arch/arm/boot/dts/microchip/at91-sama7d65_curiosity.dts
>>>>> index f091cc40a9f0..2fe34c59d942 100644
>>>>> --- a/arch/arm/boot/dts/microchip/at91-sama7d65_curiosity.dts
>>>>> +++ b/arch/arm/boot/dts/microchip/at91-sama7d65_curiosity.dts
>>>>> @@ -11,6 +11,7 @@
>>>>>  #include "sama7d65-pinfunc.h"
>>>>>  #include "sama7d65.dtsi"
>>>>>  #include <dt-bindings/mfd/atmel-flexcom.h>
>>>>> +#include <dt-bindings/input/input.h>
>>>>>  #include <dt-bindings/pinctrl/at91.h>
>>>>>
>>>>>  / {
>>>>> @@ -26,6 +27,42 @@ chosen {
>>>>>             stdout-path = "serial0:115200n8";
>>>>>     };
>>>>>
>>>>> +   gpio-keys {
>>>>> +           compatible = "gpio-keys";
>>>>> +
>>>>> +           pinctrl-names = "default";
>>>>> +           pinctrl-0 = <&pinctrl_key_gpio_default>;
>>>>> +
>>>>> +           button {
>>>>> +                   label = "PB_USER";
>>>>> +                   gpios = <&pioa PIN_PC10 GPIO_ACTIVE_LOW>;
>>>>> +                   linux,code = <KEY_PROG1>;
>>>>> +                   wakeup-source;
>>>>> +           };
>>>>> +   };
>>>>> +
>>>>> +   leds {
>>>>> +           compatible = "gpio-leds";
>>>>> +           pinctrl-names = "default";
>>>>> +           pinctrl-0 = <&pinctrl_led_gpio_default>;
>>>>> +
>>>>> +           led-red {
>>>>> +                   label = "red";
>>>>> +                   gpios = <&pioa PIN_PB17 GPIO_ACTIVE_HIGH>; /* Conflict with pwm. */
>>>>> +           };
>>>>> +
>>>>> +           led-green {
>>>>> +                   label = "green";
>>>>> +                   gpios = <&pioa PIN_PB15 GPIO_ACTIVE_HIGH>; /* Conflict with pwm. */
>>>>> +           };
>>>>> +
>>>>> +           led-blue {
>>>>> +                   label = "blue";
>>>>> +                   gpios = <&pioa PIN_PA21 GPIO_ACTIVE_HIGH>;
>>>>> +                   linux,default-trigger = "heartbeat";
>>>>> +           };
>>>>> +   };
>>>>
>>>> The label property is deprecated.  Please use the properties "color"
>>>> and "function" for new boards.  See devicetree binding documentation
>>>> for LEDs.
>>>
>>> From a quick glance, this seems to be an RGB-LED, so I would suggest
>>> to not model it as three distinct LEDs, but make use of the
>>> "leds-group-multicolor" feature, example:
>>>
>>>  59         multi-led {
>>>  60                 compatible = "leds-group-multicolor";
>>>  61                 color = <LED_COLOR_ID_RGB>;
>>>  62                 function = LED_FUNCTION_INDICATOR;
>>>  63                 leds = <&led_red>, <&led_green>, <&led_blue>;
>>>  64         };
>>
>> I see, I was not aware of this feature. This would combine all of the
>> LED pins into one RGB light correct, it seems from sysfs that this is
>> the case.
> 
> The group-multicolor feature was merged for kernel v6.6 so it's still
> quite new.  I tried this some time ago, so this is from memory only.
> The three single color gpio leds are still visible in sysfs, but you
> can not control them independently anymore, only through the sysfs
> interface of that one multicolor led.
> 
>> Would having the default-trigger="heartbeat" still be allowed for the
>> led-blue node or should that be moved into the multi-led node? From the
>> bindings it seems that the default trigger is still in the gpio-led nodes.
> 
> Sorry, not sure here.  I put linux-leds in Cc, maybe someone over
> there can answer.  If this does not fit how Microchip wants to handle
> that LED on their boards I think that's fine, too.  Just wanted to
> make people aware of the possibility.

It does not fit very well how we want to handle the LEDs on this board
as of right now. I am assuming it is fine for now to have these as
individual LEDs?

Best,
Ryan>
> Greets
> Alex
> 
>>
>> Best,
>> Ryan
>>>
>>> Greets
>>> Alex
>>>
>>>>
>>>> Thanks and greetings
>>>> Alex
>>>>
>>>>> +
>>>>>     memory@60000000 {
>>>>>             device_type = "memory";
>>>>>             reg = <0x60000000 0x40000000>;
>>>>> @@ -352,6 +389,18 @@ pinctrl_i2c10_default: i2c10-default {
>>>>>             bias-pull-up;
>>>>>     };
>>>>>
>>>>> +   pinctrl_key_gpio_default: key-gpio-default {
>>>>> +           pinmux = <PIN_PC10__GPIO>;
>>>>> +           bias-pull-up;
>>>>> +   };
>>>>> +
>>>>> +   pinctrl_led_gpio_default: led-gpio-default {
>>>>> +           pinmux = <PIN_PB15__GPIO>,
>>>>> +                    <PIN_PB17__GPIO>,
>>>>> +                    <PIN_PA21__GPIO>;
>>>>> +           bias-pull-up;
>>>>> +   };
>>>>> +
>>>>>     pinctrl_sdmmc1_default: sdmmc1-default {
>>>>>             cmd-data {
>>>>>                     pinmux = <PIN_PB22__SDMMC1_CMD>,
>>>>> --
>>>>> 2.43.0
>>>>>
>>>>>
>>>>
>>
>>