Received-SPF: pass (google.com: domain of linux-kernel+bounces-191766-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Tue, 28 May 2024 07:25:07 +0200
From: Thomas =?utf-8?Q?Wei=C3=9Fschuh?= <linux@weissschuh.net>
To: Tzung-Bi Shih <tzungbi@kernel.org>
Cc: Pavel Machek <pavel@ucw.cz>, Lee Jones <lee@kernel.org>, 
	Benson Leung <bleung@chromium.org>, Guenter Roeck <groeck@chromium.org>, linux-leds@vger.kernel.org, 
	linux-kernel@vger.kernel.org, chrome-platform@lists.linux.dev, 
	Dustin Howett <dustin@howett.net>, Mario Limonciello <mario.limonciello@amd.com>
Subject: Re: [PATCH 4/5] leds: add ChromeOS EC driver
Message-ID: <2d03e62c-13ad-4c6f-94e1-7dff817386a4@t-8ch.de>
References: <20240520-cros_ec-led-v1-0-4068fc5c051a@weissschuh.net>
 <20240520-cros_ec-led-v1-4-4068fc5c051a@weissschuh.net>
 <ZlVnCX41HdksPwUo@google.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <ZlVnCX41HdksPwUo@google.com>

On 2024-05-28 05:09:29+0000, Tzung-Bi Shih wrote:
> On Mon, May 20, 2024 at 12:00:32PM +0200, Thomas Weißschuh wrote:
> > diff --git a/drivers/leds/leds-cros_ec.c b/drivers/leds/leds-cros_ec.c
> [...]
> > + *  ChromesOS EC LED Driver
> 
> s/ChromesOS/ChromeOS/.

Ack.

> > +static int cros_ec_led_trigger_activate(struct led_classdev *led_cdev)
> > +{
> > +	struct cros_ec_led_priv *priv = cros_ec_led_cdev_to_priv(led_cdev);
> > +	union cros_ec_led_cmd_data arg = { };
> 
> To be neat, { } -> {}.

Ack.
 
> > +static int cros_ec_led_brightness_set_blocking(struct led_classdev *led_cdev,
> > +					       enum led_brightness brightness)
> > +{
> > +	struct cros_ec_led_priv *priv = cros_ec_led_cdev_to_priv(led_cdev);
> > +	union cros_ec_led_cmd_data arg = { };
> 
> Ditto.
> 
> > +static int cros_ec_led_count_subleds(struct device *dev,
> > +				     struct ec_response_led_control *resp,
> > +				     unsigned int *max_brightness)
> > +{
> > +	unsigned int range, common_range = 0;
> > +	int num_subleds = 0;
> > +	size_t i;
> > +
> > +	for (i = 0; i < EC_LED_COLOR_COUNT; i++) {
> > +		range = resp->brightness_range[i];
> > +
> > +		if (!range)
> > +			continue;
> > +
> > +		num_subleds++;
> > +
> > +		if (!common_range)
> > +			common_range = range;
> > +
> > +		if (common_range != range) {
> > +			/* The multicolor LED API expects a uniform max_brightness */
> > +			dev_warn(dev, "Inconsistent LED brightness values\n");
> > +			return -EINVAL;
> > +		}
> 
> What if the array is [0, 1, 1]?

The "0" will be skipped by 

if (!range)
	continue;

And the two "1" will pass the check.

> 
> > +static int cros_ec_led_probe_led(struct device *dev, struct cros_ec_device *cros_ec,
> > +				 enum ec_led_id id)
> > +{
> > +	union cros_ec_led_cmd_data arg = { };
> 
> Ditto.
> 
> > +static int cros_ec_led_probe(struct platform_device *pdev)
> > +{
> [...]
> > +	int ret;
> > +
> > +	for (i = 0; i < EC_LED_ID_COUNT; i++) {
> > +		ret = cros_ec_led_probe_led(dev, cros_ec, i);
> > +		if (ret)
> > +			break;
> > +	}
> > +
> > +	return ret;
> 
> `ret` should be initialized in case EC_LED_ID_COUNT would be somehow 0.

As that's a constant define, this should never happen.
But after thinking about it, it seems a bit clearer.
The compiler should figure out that it's redundant anyways.

> > +static int __init cros_ec_led_init(void)
> > +{
> > +	int ret;
> > +
> > +	ret = led_trigger_register(&cros_ec_led_trigger);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = platform_driver_register(&cros_ec_led_driver);
> > +	if (ret)
> > +		led_trigger_unregister(&cros_ec_led_trigger);
> > +
> > +	return ret;
> > +};
> > +module_init(cros_ec_led_init);
> > +
> > +static void __exit cros_ec_led_exit(void)
> > +{
> > +	platform_driver_unregister(&cros_ec_led_driver);
> > +	led_trigger_unregister(&cros_ec_led_trigger);
> > +};
> > +module_exit(cros_ec_led_exit);
> 
> I wonder it could use module_led_trigger() and module_platform_driver().

This won't compile as the macros generate various duplicate symbols.

Also the order is important, so I think the explicit logic is clearer.


Thomas