lkml.org 
[lkml]   [2023]   [Mar]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v7 6/6] leds: Add a multicolor LED driver to group monochromatic LEDs
From


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@traphandler.com>");
>> +MODULE_DESCRIPTION("multi-color LED group driver");
>> +MODULE_LICENSE("GPL");
>> +MODULE_ALIAS("platform:leds-group-multicolor");
>> --
>> 2.25.1
>>
>
> --
> Lee Jones [李琼斯]

\
 
 \ /
  Last update: 2023-03-28 17:51    [W:0.060 / U:1.756 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site