Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH] leds: pwm: Allow automatic labels for DT based devices
To:     Alexander Dahl <ada@thorsis.com>, linux-leds@vger.kernel.org
Cc:     Pavel Machek <pavel@ucw.cz>, Dan Murphy <dmurphy@ti.com>,
        linux-kernel@vger.kernel.org, Alexander Dahl <post@lespocky.de>
References: <20200826093737.29008-1-ada@thorsis.com>
From:   Jacek Anaszewski <jacek.anaszewski@gmail.com>
Message-ID: <d6f69d4a-3e46-cad4-5756-4d15e5ef95ba@gmail.com>
Date:   Thu, 27 Aug 2020 23:28:45 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.11.0
MIME-Version: 1.0
In-Reply-To: <20200826093737.29008-1-ada@thorsis.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Alexander,

On 8/26/20 11:37 AM, Alexander Dahl wrote:
> From: Alexander Dahl <post@lespocky.de>
> 
> If LEDs are configured through device tree and the property 'label' is
> omitted, the label is supposed to be generated from the properties
> 'function' and 'color' if present.  While this works fine for e.g. the
> 'leds-gpio' driver, it did not for 'leds-pwm'.
> 
> The reason is, you get this label naming magic only if you add a LED
> device through 'devm_led_classdev_register_ext()' and pass a pointer to
> the current device tree node.  The approach to fix this was adopted from
> the 'leds-gpio' driver.
> 
> For the following node from dts the LED appeared as 'led5' in sysfs
> before and as 'red:debug' after this change.
> 
>          pwm_leds {
>                  compatible = "pwm-leds";
> 
>                  led5 {
>                          function = LED_FUNCTION_DEBUG;
>                          color = <LED_COLOR_ID_RED>;
>                          pwms = <&pwm0 2 10000000 0>;
>                          max-brightness = <127>;
> 
>                          linux,default-trigger = "heartbeat";
>                          panic-indicator;
>                  };
>          };
> 
> Signed-off-by: Alexander Dahl <post@lespocky.de>
> ---
> 
> Notes:
>      v1: based on v5.9-rc2, backport on v5.4.59 also works
> 
>   drivers/leds/leds-pwm.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
> index ef7b91bd2064..a27a1d75a3e9 100644
> --- a/drivers/leds/leds-pwm.c
> +++ b/drivers/leds/leds-pwm.c
> @@ -65,6 +65,7 @@ static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv,
>   		       struct led_pwm *led, struct fwnode_handle *fwnode)
>   {
>   	struct led_pwm_data *led_data = &priv->leds[priv->num_leds];
> +	struct led_init_data init_data = {};
>   	int ret;
>   
>   	led_data->active_low = led->active_low;
> @@ -90,7 +91,13 @@ static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv,
>   
>   	pwm_init_state(led_data->pwm, &led_data->pwmstate);
>   
> -	ret = devm_led_classdev_register(dev, &led_data->cdev);
> +	if (fwnode) {
> +		init_data.fwnode = fwnode;
> +		ret = devm_led_classdev_register_ext(dev, &led_data->cdev,
> +						     &init_data);
> +	} else {
> +		ret = devm_led_classdev_register(dev, &led_data->cdev);
> +	}
>   	if (ret) {
>   		dev_err(dev, "failed to register PWM led for %s: %d\n",
>   			led->name, ret);
> 

This part looks good, but corresponding update of
Documentation/devicetree/bindings/leds/leds-pwm.txt is needed as well.
It would be good to switch to yaml by this occassion.

-- 
Best regards,
Jacek Anaszewski