Re: [PATCH v4 8/9] leds: lp50xx: Add the LP50XX family of the RGB LED driver

[Jacek Anaszewski <jacek.anaszewski@gmail.com>]

Hi Dan,

Thank you for the patch  I see few things to improve.

On 7/25/19 8:28 PM, Dan Murphy wrote:
> Introduce the LP5036/30/24/18 RGB LED driver.
> The difference in these parts are the number of
> LED outputs where the:
> 
> LP5036 can control 36 LEDs
> LP5030 can control 30 LEDs
> LP5024 can control 24 LEDs
> LP5018 can control 18 LEDs
> 
> The device has the ability to group LED output into control banks
> so that multiple LED banks can be controlled with the same mixing and
> brightness.  Inversely the LEDs can also be controlled independently.
> 
> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> ---
>  drivers/leds/Kconfig       |   7 +
>  drivers/leds/Makefile      |   1 +
>  drivers/leds/leds-lp50xx.c | 789 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 797 insertions(+)
>  create mode 100644 drivers/leds/leds-lp50xx.c
> 
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 7f780d5b8490..69c037020f6b 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -350,6 +350,13 @@ config LEDS_LP3952
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called leds-lp3952.
>  
> +config LEDS_LP50XX
> +	tristate "LED Support for TI LP5036/30/24/18 LED driver chip"
> +	depends on LEDS_CLASS && REGMAP_I2C
> +	help
> +	  If you say yes here you get support for the Texas Instruments
> +	  LP5036, LP5030, LP5024 and LP5018 LED driver.
> +
>  config LEDS_LP55XX_COMMON
>  	tristate "Common Driver for TI/National LP5521/5523/55231/5562/8501"
>  	depends on LEDS_LP5521 || LEDS_LP5523 || LEDS_LP5562 || LEDS_LP8501
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index 897b810257dd..438a5499f3ee 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -34,6 +34,7 @@ obj-$(CONFIG_LEDS_GPIO_REGISTER)	+= leds-gpio-register.o
>  obj-$(CONFIG_LEDS_GPIO)			+= leds-gpio.o
>  obj-$(CONFIG_LEDS_LP3944)		+= leds-lp3944.o
>  obj-$(CONFIG_LEDS_LP3952)		+= leds-lp3952.o
> +obj-$(CONFIG_LEDS_LP50XX)		+= leds-lp50xx.o
>  obj-$(CONFIG_LEDS_LP55XX_COMMON)	+= leds-lp55xx-common.o
>  obj-$(CONFIG_LEDS_LP5521)		+= leds-lp5521.o
>  obj-$(CONFIG_LEDS_LP5523)		+= leds-lp5523.o
> diff --git a/drivers/leds/leds-lp50xx.c b/drivers/leds/leds-lp50xx.c
> new file mode 100644
> index 000000000000..5a32410e4f34
[...]
> +static int lp50xx_probe_dt(struct lp50xx *priv)
> +{
> +	u32 led_strings[LP5036_MAX_LED_MODULES];
> +	struct fwnode_handle *child = NULL;
> +	struct fwnode_handle *led_node = NULL;
> +	struct led_init_data init_data;
> +	struct lp50xx_led *led;
> +	int num_colors;
> +	u32 color_id;
> +	int led_number;
> +	size_t i = 0;
> +	int ret;
> +
> +	priv->enable_gpio = devm_gpiod_get_optional(&priv->client->dev,
> +						   "enable", GPIOD_OUT_LOW);
> +	if (IS_ERR(priv->enable_gpio)) {
> +		ret = PTR_ERR(priv->enable_gpio);
> +		dev_err(&priv->client->dev, "Failed to get enable gpio: %d\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	priv->regulator = devm_regulator_get(&priv->client->dev, "vled");
> +	if (IS_ERR(priv->regulator))
> +		priv->regulator = NULL;
> +
> +	if (priv->model_id == LP5009)
> +		priv->max_leds = LP5009_MAX_LED_MODULES;
> +	else if (priv->model_id == LP5012)
> +		priv->max_leds = LP5012_MAX_LED_MODULES;
> +	else if (priv->model_id == LP5018)
> +		priv->max_leds = LP5018_MAX_LED_MODULES;
> +	else if (priv->model_id == LP5024)
> +		priv->max_leds = LP5024_MAX_LED_MODULES;
> +	else if (priv->model_id == LP5030)
> +		priv->max_leds = LP5030_MAX_LED_MODULES;
> +	else
> +		priv->max_leds = LP5036_MAX_LED_MODULES;

You could simplify that by initializing data property of
lp50xx_leds_match array elements and then call of_match_device.
You can compare e.g. drivers/leds/leds-pca9532.c

> +	device_for_each_child_node(&priv->client->dev, child) {
> +		led = &priv->leds[i];
> +		if (fwnode_property_present(child, "ti,led-bank")) {
> +			ret = fwnode_property_read_u32_array(child,
> +							     "ti,led-bank",
> +							     NULL, 0);
> +			ret = fwnode_property_read_u32_array(child,
> +							     "ti,led-bank",
> +							     led_strings,
> +							     ret);
> +
> +			priv->num_of_banked_leds = ARRAY_SIZE(led_strings);
> +
> +			ret = lp50xx_set_banks(priv, led_strings);
> +			if (ret) {
> +				dev_err(&priv->client->dev,
> +					"Cannot setup banked LEDs\n");
> +				fwnode_handle_put(child);
> +				goto child_out;
> +			}
> +			led->ctrl_bank_enabled = 1;
> +
> +		} else {
> +			ret = fwnode_property_read_u32(child, "reg",
> +					       &led_number);
> +
> +			led->led_number = led_number;
> +		}
> +		if (ret) {
> +			dev_err(&priv->client->dev,
> +				"led sourcing property missing\n");
> +			fwnode_handle_put(child);
> +			goto child_out;
> +		}
> +
> +		if (led_number > priv->max_leds) {
> +			dev_err(&priv->client->dev,
> +				"led-sources property is invalid\n");
> +			ret = -EINVAL;
> +			fwnode_handle_put(child);
> +			goto child_out;
> +		}
> +
> +		init_data.fwnode = child;
> +		init_data.devicename = priv->client->name;

We'd rather not have devicename in the name of this LED class device
according to the new LED naming convention. It's not a hot-pluggable
device, so for properly designed DT we should not encounter name
clash.

> +		init_data.default_label = ":";

default_label should not be set for new drivers. See the comment in
the include/linux/leds.h:

        /*
         * string to be used for devicename section of LED class device
         * either for label based LED name composition path or for fwnode
         * based when devname_mandatory is true
         */

This is new driver and we should not add any convenience fallbacks
for label based DT bindings. Label DT property has been marked
deprecated, so for new drivers it should be deemed non-existent.


> +
> +		fwnode_property_read_string(child, "linux,default-trigger",
> +				    &led->led_dev.default_trigger);
> +		num_colors = 0;
> +
> +		fwnode_for_each_child_node(child, led_node) {
> +			ret = fwnode_property_read_u32(led_node, "color",
> +						       &color_id);
> +			if (ret)
> +				dev_err(&priv->client->dev,
> +				"Cannot read color\n");
> +
> +			led->mc_cdev.available_colors |= (1 << color_id);
> +			num_colors++;
> +
> +		}
> +
> +		led->priv = priv;
> +		led->mc_cdev.ops = &lp50xx_mc_ops;
> +		led->mc_cdev.num_leds = num_colors;
> +		led->mc_cdev.led_cdev = &led->led_dev;
> +		led->led_dev.brightness_set_blocking = lp50xx_brightness_set;
> +		led->led_dev.brightness_get = lp50xx_brightness_get;
> +		ret = led_classdev_multicolor_register_ext(&priv->client->dev,
> +						       &led->mc_cdev,
> +						       &init_data);

Why not devm_* prefixed version?

[...]

-- 
Best regards,
Jacek Anaszewski