Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 13 Jul 2023 10:53:28 +0100
From:   Lee Jones <lee@kernel.org>
To:     Jean-Jacques Hiblot <jjhiblot@traphandler.com>
Cc:     pavel@ucw.cz, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org,
        linux-leds@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v10 3/5] leds: class: store the color index in struct
 led_classdev
Message-ID: <20230713095328.GH10768@google.com>
References: <20230624084217.3079205-1-jjhiblot@traphandler.com>
 <20230624084217.3079205-4-jjhiblot@traphandler.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20230624084217.3079205-4-jjhiblot@traphandler.com>
Precedence: bulk

On Sat, 24 Jun 2023, Jean-Jacques Hiblot wrote:

> This information might be useful for more than only deriving the led's

Please expand on this.  What more?

> name. And since we have this information, we can expose it in the sysfs.

The second sentence doesn't make sense to me.

It's good practice to try and avoid placing "And" as the first word.

> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com>
> ---
>  Documentation/ABI/testing/sysfs-class-led |  9 +++++++++
>  drivers/leds/led-class.c                  | 20 ++++++++++++++++++++
>  include/linux/leds.h                      |  1 +
>  3 files changed, 30 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-led b/Documentation/ABI/testing/sysfs-class-led
> index 2e24ac3bd7ef..1509e71fcde1 100644
> --- a/Documentation/ABI/testing/sysfs-class-led
> +++ b/Documentation/ABI/testing/sysfs-class-led
> @@ -59,6 +59,15 @@ Description:
>  		brightness. Reading this file when no hw brightness change
>  		event has happened will return an ENODATA error.
>  
> +What:		/sys/class/leds/<led>/color
> +Date:		June 2023
> +KernelVersion:	6.5
> +Description:
> +		Color of the led.

s/led/LED/  everywhere please.

> +		This is a read-only file. Reading this file returns the color
> +		of the led as a string (ex: "red", "green", "multicolor").

e.g.

> +
>  What:		/sys/class/leds/<led>/trigger
>  Date:		March 2006
>  KernelVersion:	2.6.17
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index eb1a8494dc5b..6cca21b227dd 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -76,6 +76,18 @@ static ssize_t max_brightness_show(struct device *dev,
>  }
>  static DEVICE_ATTR_RO(max_brightness);
>  
> +static ssize_t color_show(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +	const char *color_text = "invalid";
> +	struct led_classdev *led_cdev = dev_get_drvdata(dev);

Can this be NULL?

> +	if (led_cdev->color < LED_COLOR_ID_MAX)
> +		color_text = led_colors[led_cdev->color];

'\n'

> +	return sysfs_emit(buf, "%s\n", color_text);
> +}
> +static DEVICE_ATTR_RO(color);
> +
>  #ifdef CONFIG_LEDS_TRIGGERS
>  static BIN_ATTR(trigger, 0644, led_trigger_read, led_trigger_write, 0);
>  static struct bin_attribute *led_trigger_bin_attrs[] = {
> @@ -90,6 +102,7 @@ static const struct attribute_group led_trigger_group = {
>  static struct attribute *led_class_attrs[] = {
>  	&dev_attr_brightness.attr,
>  	&dev_attr_max_brightness.attr,
> +	&dev_attr_color.attr,
>  	NULL,
>  };
>  
> @@ -482,6 +495,10 @@ int led_classdev_register_ext(struct device *parent,
>  			if (fwnode_property_present(init_data->fwnode,
>  						    "retain-state-shutdown"))
>  				led_cdev->flags |= LED_RETAIN_AT_SHUTDOWN;
> +
> +			if (fwnode_property_present(init_data->fwnode, "color"))
> +				fwnode_property_read_u32(init_data->fwnode, "color",
> +							 &led_cdev->color);
>  		}
>  	} else {
>  		proposed_name = led_cdev->name;
> @@ -491,6 +508,9 @@ int led_classdev_register_ext(struct device *parent,
>  	if (ret < 0)
>  		return ret;
>  
> +	if (led_cdev->color >= LED_COLOR_ID_MAX)
> +		dev_warn(parent, "LED %s color identifier out of range\n", final_name);
> +
>  	mutex_init(&led_cdev->led_access);
>  	mutex_lock(&led_cdev->led_access);
>  	led_cdev->dev = device_create_with_groups(leds_class, parent, 0,
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index 95311c70d95c..487d00dac4de 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -100,6 +100,7 @@ struct led_classdev {
>  	const char		*name;
>  	unsigned int brightness;
>  	unsigned int max_brightness;
> +	unsigned int color;
>  	int			 flags;
>  
>  	/* Lower 16 bits reflect status */
> -- 
> 2.34.1
> 

-- 
Lee Jones [李琼斯]