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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <d1771420-b45e-3ce1-f469-bc5fd9ab6dfa@traphandler.com>
Date:   Tue, 28 Mar 2023 17:31:36 +0200
From:   Jean-Jacques Hiblot <jjhiblot@...phandler.com>
To:     Lee Jones <lee@...nel.org>
CC:     <lee.jones@...aro.org>, <pavel@....cz>,
        <linux-leds@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v7 6/6] leds: Add a multicolor LED driver to group
 monochromatic LEDs



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