Date:   Thu, 20 Jun 2019 18:10:26 +0200
From:   Jacek Anaszewski <jacek.anaszewski@...il.com>
To:     Dan Murphy <dmurphy@...com>, pavel@....cz, robh+dt@...nel.org
Cc:     devicetree@...r.kernel.org, linux-leds@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 6/9] leds: multicolor: Introduce a multicolor class
 definition

Hi Dan,

Thank you for the v5.

I will confine myself to commenting only some parts since
the rest will undergo rework due to removal of sync API.

On 5/23/19 9:08 PM, Dan Murphy wrote:
> Introduce a multicolor class that groups colored LEDs
> within a LED node.
> 
> The framework allows for dynamically setting individual LEDs
> or setting brightness levels of LEDs and updating them virtually
> simultaneously.
> 
> Signed-off-by: Dan Murphy <dmurphy@...com>
> ---
>   drivers/leds/Kconfig                 |  10 +
>   drivers/leds/Makefile                |   1 +
>   drivers/leds/led-class-multicolor.c  | 421 +++++++++++++++++++++++++++
>   include/linux/led-class-multicolor.h |  95 ++++++
>   4 files changed, 527 insertions(+)
>   create mode 100644 drivers/leds/led-class-multicolor.c
>   create mode 100644 include/linux/led-class-multicolor.h
> 
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 0414adebb177..0696a13c9527 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -29,6 +29,16 @@ config LEDS_CLASS_FLASH
>   	  for the flash related features of a LED device. It can be built
>   	  as a module.
>   
> +config LEDS_CLASS_MULTI_COLOR
> +	tristate "LED Mulit Color LED Class Support"
> +	depends on LEDS_CLASS
> +	help
> +	  This option enables the multicolor LED sysfs class in /sys/class/leds.
> +	  It wraps LED Class and adds multicolor LED specific sysfs attributes
> +	  and kernel internal API to it. You'll need this to provide support
> +	  for multicolor LEDs that are grouped together. This class is not
> +	  intended for single color LEDs.  It can be built as a module.

extra whitespace:

s/ It can/It can/

[...]
> +
> +static int multicolor_set_brightness(struct led_classdev *led_cdev,
> +			     enum led_brightness brightness)
> +{
> +	struct led_classdev_mc *mcled_cdev = lcdev_to_mccdev(led_cdev);
> +	struct led_classdev_mc_data *data = mcled_cdev->data;
> +	struct led_multicolor_ops *ops = mcled_cdev->ops;
> +	struct led_classdev_mc_priv *priv;
> +	unsigned long state = brightness;
> +	int adj_value;
> +	ssize_t ret = -EINVAL;
> +
> +	mutex_lock(&led_cdev->led_access);
> +
> +	if (ops->set_module_brightness) {
> +		ret = ops->set_module_brightness(mcled_cdev, state);
> +		goto unlock;
> +	}
> +
> +	list_for_each_entry(priv, &data->color_list, list) {
> +		if (state && priv->brightness && priv->max_brightness) {
> +			adj_value = state * ((priv->brightness * 100) / priv->max_brightness);
> +			adj_value = adj_value / 100;

Why the multiplication an then division by 100? And priv->max_brightness
stays unaltered? This changes the proportions. My python script works
just fine without those.

> +		} else
> +			adj_value = LED_OFF;
> +
> +		ret = ops->set_color_brightness(priv->mcled_cdev,
> +						priv->color_id,	adj_value);
> +		if (ret < 0)
> +			goto unlock;
> +	}
> +
> +unlock:
> +	mutex_unlock(&led_cdev->led_access);
> +	return ret;
> +}
[...]
> +int led_classdev_multicolor_register_ext(struct device *parent,
> +				     struct led_classdev_mc *mcled_cdev,
> +				     struct led_init_data *init_data)
> +{
> +	struct led_classdev *led_cdev;
> +	struct led_multicolor_ops *ops;
> +	struct led_classdev_mc_data *data;
> +	int ret;
> +	int i;
> +
> +	if (!mcled_cdev)
> +		return -EINVAL;
> +
> +	ops = mcled_cdev->ops;
> +	if (!ops || !ops->set_color_brightness)
> +		return -EINVAL;
> +
> +	data = kzalloc(sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	mcled_cdev->data = data;
> +	led_cdev = &mcled_cdev->led_cdev;
> +
> +	if (led_cdev->brightness_set_blocking)
> +		led_cdev->brightness_set_blocking = multicolor_set_brightness;

This is weird. In leds-lp50xx.c you don't initialize
brightness_set_blocking and this still works?

I believe this is kind of omission.

And it is not reasonable to just override driver supplied op with
generic one just like that.

I propose to initialize brightness_set or brightness_set_blocking
op as we used to do it for monochrome LEDs. Those function(s) on
driver side will either use device's hardware support for setting
color lightness, or will call a generic function provided by
LED multi color class for calculating intensities of all colors
it comprises in the cluster.

I know this is different to what we've discussed on IRC, but now
it looks for me the most reasonable way to go.

> +	INIT_LIST_HEAD(&data->color_list);
> +
> +	/* Register led class device */
> +	ret = led_classdev_register_ext(parent, led_cdev, init_data);
> +	if (ret)
> +		return ret;
> +
> +	ret = led_multicolor_init_color_dir(data, mcled_cdev);
> +	if (ret)
> +		return ret;
> +
> +	/* Select the sysfs attributes to be created for the device */
> +	for (i = 0; i < mcled_cdev->num_leds; i++) {
> +		ret = led_multicolor_init_color(data, mcled_cdev,
> +						mcled_cdev->available_colors[i]);
> +		if (ret)
> +			break;
> +	}
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(led_classdev_multicolor_register_ext);
> +
> +void led_classdev_multicolor_unregister(struct led_classdev_mc *mcled_cdev)
> +{
> +	if (!mcled_cdev)
> +		return;
> +
> +	led_classdev_unregister(&mcled_cdev->led_cdev);
> +}
> +EXPORT_SYMBOL_GPL(led_classdev_multicolor_unregister);
> +
> +static void devm_led_classdev_multicolor_release(struct device *dev, void *res)
> +{
> +	led_classdev_multicolor_unregister(*(struct led_classdev_mc **)res);
> +}
> +
> +/**
> + * devm_of_led_classdev_register - resource managed led_classdev_register()
> + *
> + * @parent: parent of LED device
> + * @led_cdev: the led_classdev structure for this device.
> + */
> +int devm_led_classdev_multicolor_register(struct device *parent,
> +					  struct led_classdev_mc *mcled_cdev)
> +{
> +	struct led_classdev_mc **dr;
> +	int ret;
> +
> +	dr = devres_alloc(devm_led_classdev_multicolor_release,
> +			  sizeof(*dr), GFP_KERNEL);
> +	if (!dr)
> +		return -ENOMEM;
> +
> +	ret = led_classdev_multicolor_register(parent, mcled_cdev);
> +	if (ret) {
> +		devres_free(dr);
> +		return ret;
> +	}
> +
> +	*dr = mcled_cdev;
> +	devres_add(parent, dr);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(devm_led_classdev_multicolor_register);
> +
> +static int devm_led_classdev_multicolor_match(struct device *dev,
> +					      void *res, void *data)
> +{
> +	struct mcled_cdev **p = res;
> +
> +	if (WARN_ON(!p || !*p))
> +		return 0;
> +
> +	return *p == data;
> +}
> +
> +/**
> + * devm_led_classdev_multicolor_unregister() - resource managed
> + *					led_classdev_multicolor_unregister()
> + * @parent: The device to unregister.
> + * @mcled_cdev: the led_classdev_mc structure for this device.
> + */
> +void devm_led_classdev_multicolor_unregister(struct device *dev,
> +				  struct led_classdev_mc *mcled_cdev)
> +{
> +	WARN_ON(devres_release(dev,
> +			       devm_led_classdev_multicolor_release,
> +			       devm_led_classdev_multicolor_match, mcled_cdev));
> +}
> +EXPORT_SYMBOL_GPL(devm_led_classdev_multicolor_unregister);
> +
> +MODULE_AUTHOR("Dan Murphy <dmurphy@...com>");
> +MODULE_DESCRIPTION("Multi Color LED class interface");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/led-class-multicolor.h b/include/linux/led-class-multicolor.h
> new file mode 100644
> index 000000000000..f9e71d984b03
> --- /dev/null
> +++ b/include/linux/led-class-multicolor.h
> @@ -0,0 +1,95 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/* LED Multicolor class interface
> + * Copyright (C) 2019 Texas Instruments Incorporated - http://www.ti.com/
> + */

Let's have C++ comment style also here.

> +#ifndef __LINUX_MULTICOLOR_LEDS_H_INCLUDED
> +#define __LINUX_MULTICOLOR_LEDS_H_INCLUDED
> +
> +#include <linux/leds.h>
> +#include <dt-bindings/leds/common.h>
[...]


-- 
Best regards,
Jacek Anaszewski

Powered by blists - more mailing lists