Re: [PATCH v7 6/6] leds: Add a multicolor LED driver to group monochromatic LEDs

[Jean-Jacques Hiblot]

On 15/03/2023 16:52, Lee Jones wrote:
> > +	for (;;) {
> > +		struct led_classdev *led_cdev;
> > +
> > +		led_cdev = devm_of_led_get_optional(dev, count);
> > +		if (IS_ERR(led_cdev))
>
> Doesn't devm_of_led_get_optional() return NULL on failure?

Hi Lee,

Thanks for you review. I'll send an updated version shortly.


devm_of_led_get_optional() return an error-pointer when it cannot get 
the LED (like EPROBE_DEFER or ENOMEM). When the LED is not defined, it 
returns NULL.

> > +			return dev_err_probe(dev, PTR_ERR(led_cdev),
> > +					     "Unable to get led #%d", count);
> > +		/* Reached the end of the list ?*/
>
> Besides the incorrect formatting, that '?' isn't filling me with
> confidence.
The comment just meant that NULL indicates the end of the list of the LEDs.

JJ
> > +		if (!led_cdev)
> > +			break;
> > +
> > +		priv->monochromatics = devm_krealloc_array(dev, priv->monochromatics,
> > +					count + 1, sizeof(*priv->monochromatics),
> > +					GFP_KERNEL);
> > +		if (!priv->monochromatics)
> > +			return -ENOMEM;
> > +
> > +		priv->monochromatics[count] = led_cdev;
> > +
> > +		max_brightness = max(max_brightness, led_cdev->max_brightness);
> > +		count++;
> > +	}
> > +
> > +	subled = devm_kcalloc(dev, count, sizeof(*subled), GFP_KERNEL);
> > +	if (!subled)
> > +		return -ENOMEM;
> > +	priv->mc_cdev.subled_info = subled;
> > +
> > +	for (i = 0; i < count; i++) {
> > +		struct led_classdev *led_cdev = priv->monochromatics[i];
> > +
> > +		subled[i].color_index = led_cdev->color;
>
> '\n'
>
> > +		/* configure the LED intensity to its maximum */
>
> Use correct grammar in comments please.
>
> Start with an uppercase char.
>
> > +		subled[i].intensity = max_brightness;
> > +	}
> > +
> > +	/* init the multicolor's LED class device */
>
> As above and please be fully forthcoming in comments: "Initialise".
>
> > +	cdev = &priv->mc_cdev.led_cdev;
> > +	cdev->flags = LED_CORE_SUSPENDRESUME;
> > +	cdev->brightness_set_blocking = led_mcg_set;
> > +	cdev->max_brightness = max_brightness;
> > +	cdev->color = LED_COLOR_ID_MULTI;
> > +	priv->mc_cdev.num_colors = count;
> > +
> > +	init_data.fwnode = dev_fwnode(dev);
> > +	ret = devm_led_classdev_multicolor_register_ext(dev, &priv->mc_cdev,
> > +							&init_data);
>
> Use the full 100-char limit everywhere please.
>
> > +	if (ret)
> > +		return dev_err_probe(dev, ret,
> > +			"failed to register multicolor led for %s.\n",
>
> "LED"
>
> > +			cdev->name);
> > +
> > +	ret = led_mcg_set(cdev, cdev->brightness);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret,
> > +				     "failed to set led value for %s.",
> > +				     cdev->name);
> > +
> > +	for (i = 0; i < count; i++) {
> > +		struct led_classdev *led_cdev = priv->monochromatics[i];
> > +
> > +		/* Make the sysfs of the monochromatic LED read-only */
> > +		mutex_lock(&led_cdev->led_access);
> > +		led_sysfs_disable(led_cdev);
> > +		mutex_unlock(&led_cdev->led_access);
> > +
> > +		/* Restore sysfs access when the multicolor LED is released */
> > +		devm_add_action_or_reset(dev, restore_sysfs_access, led_cdev);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id of_led_mcg_match[] = {
> > +	{ .compatible = "leds-group-multicolor" },
> > +	{}
> > +};
> > +MODULE_DEVICE_TABLE(of, of_led_mcg_match);
> > +
> > +static struct platform_driver led_mcg_driver = {
> > +	.probe		= led_mcg_probe,
> > +	.driver		= {
> > +		.name	= "leds_group_multicolor",
> > +		.of_match_table = of_led_mcg_match,
> > +	}
> > +};
> > +module_platform_driver(led_mcg_driver);
> > +
> > +MODULE_AUTHOR("Jean-Jacques Hiblot <jjhiblot@xxxxxxxxxxxxxxx>");
> > +MODULE_DESCRIPTION("multi-color LED group driver");
> > +MODULE_LICENSE("GPL");
> > +MODULE_ALIAS("platform:leds-group-multicolor");
> > --
> > 2.25.1
>
> --
> Lee Jones [李琼斯]