Re: [PATCH] ARM: dts: aspeed: clemente: Add HDD LED GPIO

[Andrew Jeffery <andrew@codeconstruct.com.au>]

Hi Leo,

On Fri, 2025-09-26 at 17:37 +0800, Leo Wang wrote:
> Define a GPIO expander pin for the HDD LED and expose it via the
> LED subsystem. This allows the BMC to control the front panel
> HDD activity LED.
> 
> Changes include:
>  - Add `led-hdd` node under the LED definitions
>  - Name the expander pin "HDD_LED_N" in io_expander13

Can you please drop this "Changes include" section? What you've written
here is plain to see from the diff below. The commit message should
explain motivations, concerns and trade-offs, and not be a wrote
description of the change. The documentation on submitting patches
explores what's required:

https://docs.kernel.org/process/submitting-patches.html#describe-your-changes

Cheers,

Andrew

> 
> Signed-off-by: Leo Wang <leo.jt.wang@gmail.com>
> ---
>  arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-clemente.dts | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-clemente.dts b/arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-clemente.dts
> index a614f947dafc9a6628c3f3d45b62ff3ca9d2b618..450446913e36b1418fab901cde44280468990c7a 100644
> --- a/arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-clemente.dts
> +++ b/arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-clemente.dts
> @@ -95,6 +95,11 @@ led-3 {
>  			label = "bmc_ready_cpld_noled";
>  			gpios = <&gpio0 ASPEED_GPIO(P, 5) (GPIO_ACTIVE_HIGH|GPIO_TRANSITORY)>;
>  		};
> +
> +		led-hdd {
> +			label = "hdd_led";
> +			gpios = <&io_expander13 1 GPIO_ACTIVE_LOW>;
> +		};
>  	};
>  
>  	memory@80000000 {
> @@ -1199,7 +1204,7 @@ io_expander13: gpio@14 {
>  		#gpio-cells = <2>;
>  		gpio-line-names =
>  			"rmc_en_dc_pwr_on",
> -			"",
> +			"HDD_LED_N",
>  			"",
>  			"",
>  			"",
> 
> ---
> base-commit: c65261717599d419e9c683d85f515d3ca2261549
> change-id: 20250926-leo-dts-add-shunt-resistor-0cba43f75bce
> 
> Best regards,