Re: [PATCH RFC v2 3/3] drivers: leds: add support for apa102c leds

[Jacek Anaszewski]

Hi Nicolas,

Regardless of the fact that LED mc framework in current shape
will probably not materialize in mainline, I have single
remark regarding LED initialization. Please take a look below.

On 2/26/20 3:33 PM, Nicolas Belin wrote:
> Initilial commit in order to support the apa102c RGB leds. This
> is based on the Multicolor Framework.
> 
> Reviewed-by: Corentin Labbe <clabbe@xxxxxxxxxxxx>
> Signed-off-by: Nicolas Belin <nbelin@xxxxxxxxxxxx>
> ---
>  drivers/leds/Kconfig        |   7 ++
>  drivers/leds/Makefile       |   1 +
>  drivers/leds/leds-apa102c.c | 291 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 299 insertions(+)
>  create mode 100644 drivers/leds/leds-apa102c.c
> 
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 5dc6535a88ef..71e29727c6ec 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -79,6 +79,13 @@ config LEDS_AN30259A
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called leds-an30259a.
>  
> +config LEDS_APA102C
> +	tristate "LED Support for Shiji APA102C"
> +	depends on SPI
> +	depends on LEDS_CLASS_MULTI_COLOR
> +	help
> +	  This option enables support for APA102C LEDs.
> +
>  config LEDS_APU
>  	tristate "Front panel LED support for PC Engines APU/APU2/APU3 boards"
>  	depends on LEDS_CLASS
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index b5305b7d43fb..8334cb6dc7e8 100644
[...]
> +
> +		led->priv			= priv;
> +		led->ldev.max_brightness	= MAX_BRIGHTNESS;
> +		fwnode_property_read_string(child, "linux,default-trigger",
> +					    &led->ldev.default_trigger);
> +
> +		init_data.fwnode = child;
> +		init_data.devicename = APA_DEV_NAME;
> +		init_data.default_label = ":";

devicename property should be filled in new drivers only in case
devname_mandatory is set to true.
default_label property is for legacy drivers, for backward compatibility
with old LED naming convention.

For more information please refer to:
- Documentation/leds/leds-class.rst, "LED Device Naming" section
- struct led_init_data documention in linux/leds.h

In effect you need only fwnode here,

> +
> +		num_colors = 0;
> +		fwnode_for_each_child_node(child, grandchild) {
> +			ret = fwnode_property_read_u32(grandchild, "color",
> +						       &color_id);
> +			if (ret) {
> +				dev_err(priv->dev, "Cannot read color\n");
> +				goto child_out;
> +			}
> +
> +			set_bit(color_id, &led->mc_cdev.available_colors);
> +			num_colors++;
> +		}
> +
> +		if (num_colors != 3) {
> +			ret = -EINVAL;
> +			dev_err(priv->dev, "There should be 3 colors\n");
> +			goto child_out;
> +		}
> +
> +		if (led->mc_cdev.available_colors != IS_RGB) {
> +			ret = -EINVAL;
> +			dev_err(priv->dev, "The led is expected to be RGB\n");
> +			goto child_out;
> +		}
> +
> +		led->mc_cdev.num_leds = num_colors;
> +		led->mc_cdev.led_cdev = &led->ldev;
> +		led->ldev.brightness_set_blocking = apa102c_brightness_set;
> +		ret = devm_led_classdev_multicolor_register_ext(priv->dev,

-- 
Best regards,
Jacek Anaszewski