[RFC,v2,0/2] Multicolor PWM LED support

From: Sven Schwermer <sven.schwermer@disruptive-technologies.com>

Hi,

As previously discussed [1] on the linux-leds list I am missing
multicolor PWM LED support. In the mean time I have put together a
working prototype for such a driver. This is my first Linux driver
so I'm hoping for some feedback. Here are some questions that came up
while putting this thing together:

  1. Currently, the max-brightness property is expected as a property to
     the multi-led node. That seems consistent with the existing
     multicolor class code, but I'm wondering whether it would make
     sense to have a max-brigthness for the individual LEDs as well?
  2. The current multi-led node definition calls for a node index which
     would in turn require the reg property to be set within the node.
     In this context, that doesn't seem to make sense. Should this
     requirement be lifted from leds-class-multicolor.yaml?
  3. I'm not currently reusing any leds-pwm code because there aren't
     too many overlaps. Does anyone have suggestions what could be
     factored out into a common source file?

I would appreciate if anyone would test this code. It runs on my
i.MX6ULL-based hardware.

Best regards,
Sven

[1]: https://www.spinics.net/lists/linux-leds/msg19988.html

Sven Schwermer (2):
  dt-bindings: leds: Add multicolor PWM LED bindings
  leds: Add PWM multicolor driver

 .../bindings/leds/leds-pwm-multicolor.yaml    |  76 ++++++++
 drivers/leds/Kconfig                          |   8 +
 drivers/leds/Makefile                         |   1 +
 drivers/leds/leds-pwm-multicolor.c            | 184 ++++++++++++++++++
 4 files changed, 269 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-pwm-multicolor.yaml
 create mode 100644 drivers/leds/leds-pwm-multicolor.c

Interdiff against v1:

On Tue, 25 Jan 2022 16:12:26 +0100
sven@svenschwermer.de wrote:

> From: Sven Schwermer <sven.schwermer@disruptive-technologies.com>
> 
> By allowing to group multiple monochrome PWM LEDs into multicolor LEDs,
> all involved LEDs can be controlled in-sync. This enables using effects
> using triggers, etc.
> 
> Signed-off-by: Sven Schwermer <sven.schwermer@disruptive-technologies.com>
> ---
>  drivers/leds/Kconfig               |   8 ++
>  drivers/leds/Makefile              |   1 +
>  drivers/leds/leds-pwm-multicolor.c | 184 +++++++++++++++++++++++++++++
>  3 files changed, 193 insertions(+)
>  create mode 100644 drivers/leds/leds-pwm-multicolor.c
> 
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 6090e647daee..bae1f63f6195 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -552,6 +552,14 @@ config LEDS_PWM
>  	help
>  	  This option enables support for pwm driven LEDs
>  
> +config LEDS_PWM_MULTICOLOR
> +	tristate "PWM driven multi-color LED Support"
> +	depends on LEDS_CLASS_MULTICOLOR
> +	depends on PWM
> +	help
> +	  This option enables support for PWM driven monochrome LEDs that are
> +	  grouped into multicolor LEDs.
> +
>  config LEDS_REGULATOR
>  	tristate "REGULATOR driven LED support"
>  	depends on LEDS_CLASS
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index e58ecb36360f..ba2c2c1edf12 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -73,6 +73,7 @@ obj-$(CONFIG_LEDS_PCA963X)		+= leds-pca963x.o
>  obj-$(CONFIG_LEDS_PM8058)		+= leds-pm8058.o
>  obj-$(CONFIG_LEDS_POWERNV)		+= leds-powernv.o
>  obj-$(CONFIG_LEDS_PWM)			+= leds-pwm.o
> +obj-$(CONFIG_LEDS_PWM_MULTICOLOR)	+= leds-pwm-multicolor.o
>  obj-$(CONFIG_LEDS_REGULATOR)		+= leds-regulator.o
>  obj-$(CONFIG_LEDS_S3C24XX)		+= leds-s3c24xx.o
>  obj-$(CONFIG_LEDS_SC27XX_BLTC)		+= leds-sc27xx-bltc.o
> diff --git a/drivers/leds/leds-pwm-multicolor.c b/drivers/leds/leds-pwm-multicolor.c
> new file mode 100644
> index 000000000000..c54bed4536d3
> --- /dev/null
> +++ b/drivers/leds/leds-pwm-multicolor.c
> @@ -0,0 +1,184 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * PWM-based multi-color LED control
> + *
> + * Copyright 2022 Sven Schwermer <sven.schwermer@disruptive-technologies.com>
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/platform_device.h>
> +#include <linux/led-class-multicolor.h>
> +#include <linux/leds.h>
> +#include <linux/err.h>
> +#include <linux/pwm.h>
> +#include <linux/mutex.h>

sort headers alphabetically

> +
> +struct pwm_led {
> +	struct pwm_device *pwm;
> +	struct pwm_state pwmstate;

maybe rename pwmstate to just state?

> +};
> +
> +struct pwm_mc_led {
> +	struct led_classdev_mc mc_cdev;
> +	struct mutex lock;
> +	struct pwm_led leds[];
> +};
> +
> +static int led_pwm_mc_set(struct led_classdev *cdev,
> +			  enum led_brightness brightness)
> +{
> +	int i;
> +	unsigned long long duty;
> +	int ret = 0;
> +	struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(cdev);
> +	struct pwm_mc_led *priv = container_of(mc_cdev, struct pwm_mc_led, mc_cdev);
> +
> +	led_mc_calc_color_components(mc_cdev, brightness);
> +
> +	mutex_lock(&priv->lock);
> +
> +	for (i = 0; i < mc_cdev->num_colors; ++i) {
> +		duty = priv->leds[i].pwmstate.period;
> +		duty *= mc_cdev->subled_info[i].brightness;
> +		do_div(duty, cdev->max_brightness);
> +
> +		priv->leds[i].pwmstate.duty_cycle = duty;
> +		priv->leds[i].pwmstate.enabled = duty > 0;
> +		ret = pwm_apply_state(priv->leds[i].pwm,
> +				      &priv->leds[i].pwmstate);
> +		if (ret)
> +			break;
> +	}
> +
> +	mutex_unlock(&priv->lock);
> +
> +	return ret;
> +}
> +
> +static int led_pwm_mc_probe(struct platform_device *pdev)
> +{
> +	struct fwnode_handle *mcnode, *fwnode;
> +	int count = 0;
> +	struct pwm_mc_led *priv;
> +	struct mc_subled *subled;
> +	struct led_classdev *cdev;
> +	struct pwm_led *pwmled;
> +	u32 color;
> +	int ret = 0;
> +	struct led_init_data init_data = {};
> +
> +	mcnode = device_get_named_child_node(&pdev->dev, "multi-led");
> +	if (!mcnode) {
> +		dev_err(&pdev->dev, "expected multi-led node\n");
> +		ret = -ENODEV;
> +		goto out;
> +	}
> +
> +	/* count the nodes inside the multi-led node */
> +	fwnode_for_each_child_node(mcnode, fwnode)
> +		++count;
> +
> +	priv = devm_kzalloc(&pdev->dev, struct_size(priv, leds, count),
> +			    GFP_KERNEL);
> +	if (!priv) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +	mutex_init(&priv->lock);
> +
> +	subled = devm_kcalloc(&pdev->dev, count, sizeof(*subled), GFP_KERNEL);
> +	if (!subled) {
> +		ret = -ENOMEM;
> +		goto destroy_mutex;
> +	}
> +	priv->mc_cdev.subled_info = subled;
> +
> +	/* init the multicolor's LED class device */
> +	cdev = &priv->mc_cdev.led_cdev;
> +	fwnode_property_read_string(mcnode, "label", &cdev->name);

	label is deprecated, do not introduce in new bindings

> +	cdev->brightness = LED_OFF;

dont use this LED_OFF/LED_ON constants, they are deprecated

> +	fwnode_property_read_u32(mcnode, "max-brightness",
> +				 &cdev->max_brightness);
> +	cdev->flags = LED_CORE_SUSPENDRESUME;
> +	cdev->brightness_set_blocking = led_pwm_mc_set;
> +
> +	/* iterate over the nodes inside the multi-led node */
> +	fwnode_for_each_child_node(mcnode, fwnode) {
> +		pwmled = &priv->leds[priv->mc_cdev.num_colors];
> +		pwmled->pwm = devm_fwnode_pwm_get(&pdev->dev, fwnode, NULL);
> +		if (IS_ERR(pwmled->pwm)) {
> +			ret = PTR_ERR(pwmled->pwm);
> +			dev_err(&pdev->dev, "unable to request PWM: %d\n", ret);

you need to release the fwnode with fwnode_handle_put(fwnode) here

> +			goto destroy_mutex;
> +		}
> +		pwm_init_state(pwmled->pwm, &pwmled->pwmstate);
> +
> +		ret = fwnode_property_read_u32(fwnode, "color", &color);
> +		if (ret) {
> +			dev_err(&pdev->dev, "cannot read color: %d\n", ret);

you need to release the fwnode with fwnode_handle_put(fwnode) here

> +			goto destroy_mutex;
> +		}
> +
> +		subled[priv->mc_cdev.num_colors].color_index = color;
> +		subled[priv->mc_cdev.num_colors].channel =
> +			priv->mc_cdev.num_colors;
> +		++priv->mc_cdev.num_colors;
> +	}
> +
> +	init_data.fwnode = mcnode;
> +	ret = devm_led_classdev_multicolor_register_ext(&pdev->dev,
> +							&priv->mc_cdev,
> +							&init_data);
> +	if (ret) {
> +		dev_err(&pdev->dev,
> +			"failed to register multicolor PWM led for %s: %d\n",
> +			cdev->name, ret);
> +		goto destroy_mutex;
> +	}
> +
> +	ret = led_pwm_mc_set(cdev, cdev->brightness);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to set led PWM value for %s: %d",
> +			cdev->name, ret);
> +		goto destroy_mutex;
> +	}
> +

you need to release mcnode with fwnode_handle_put(). You increased reference
count with device_get_named_child_node()

> +	platform_set_drvdata(pdev, priv);
> +	return 0;
> +
> +destroy_mutex:
> +	mutex_destroy(&priv->lock);
> +out:

here as well you need to release mcnode with fwnode_handle_put()

> +	return ret;
> +}
> +
> +static int led_pwm_mc_remove(struct platform_device *pdev)
> +{
> +	struct pwm_mc_led *priv = platform_get_drvdata(pdev);
> +
> +	mutex_destroy(&priv->lock);
> +	return 0;
> +}
> +
> +static const struct of_device_id of_pwm_leds_mc_match[] = {
> +	{ .compatible = "pwm-leds-multicolor", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, of_pwm_leds_mc_match);
> +
> +static struct platform_driver led_pwm_mc_driver = {
> +	.probe		= led_pwm_mc_probe,
> +	.remove		= led_pwm_mc_remove,
> +	.driver		= {
> +		.name	= "leds_pwm_multicolor",
> +		.of_match_table = of_pwm_leds_mc_match,
> +	},
> +};
> +
> +module_platform_driver(led_pwm_mc_driver);
> +
> +MODULE_AUTHOR("Sven Schwermer <sven.schwermer@disruptive-technologies.com>");
> +MODULE_DESCRIPTION("multi-color PWM LED driver");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:leds-pwm-multicolor");