linux-kernel - Re: [PATCH v2 2/2] leds: Add NCP5623 multi-led driver

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZeaRpSrkeFKAXIlq@abdel>
Date: Mon, 4 Mar 2024 22:29:41 -0500
From: Abdel Alkuor <alkuor@...il.com>
To: Lee Jones <lee@...nel.org>
Cc: Pavel Machek <pavel@....cz>, Rob Herring <robh+dt@...nel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Alice Chen <alice_chen@...htek.com>,
	Jean-Jacques Hiblot <jjhiblot@...phandler.com>,
	ChiYuan Huang <cy_huang@...htek.com>,
	ChiaEn Wu <chiaen_wu@...htek.com>,
	Lukas Bulwahn <lukas.bulwahn@...il.com>,
	André Apitzsch <git@...tzsch.eu>,
	linux-leds@...r.kernel.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] leds: Add NCP5623 multi-led driver

On Fri, Mar 01, 2024 at 08:50:46AM +0000, Lee Jones wrote:

Hi Lee,
> > +#define NCP5623_REG(x)			((x) << 0x5)
> 
> What's 0x5?  Probably worth defining.
This is a function offset. I'll add a define.

> 
> > +	guard(mutex)(&ncp->lock);
> 
> Are these self-unlocking?
Correct. Here is a short introduction about it
https://www.marcusfolkesson.se/blog/mutex-guards-in-the-linux-kernel/
> 
> > +	ncp->old_brightness = brightness;
> 
> The nomenclature is confusing here.
> 
> For the most part, this will carry the present value, no?
>
Yes, I'll change it to current_brightness instead

> > +	ret = ncp5623_write(ncp->client,
> > +			    NCP5623_DIMMING_TIME_REG, pattern[0].delta_t / 8);
> 
> Why 8?  Magic numbers should be replaced with #defines.
> 
This is dim step in ms. I'll add a define for it.

> > +static int ncp5623_pattern_clear(struct led_classdev *led_cdev)
> > +{
> > +	return 0;
> > +}
> 
> Not sure I see the point in this.
> 
> Is the .pattern_clear() compulsorily?
>
Unfortunately, it is. For example, in pattern_trig_store_patterns, when
hw pattern is used, it is expected to have pattern_clear implemented.

static ssize_t pattern_trig_store_patterns(struct led_classdev *led_cdev,
                                            const char *buf, const u32 *buf_int,
                                            size_t count, bool hw_pattern)
{
	...
         if (data->is_hw_pattern)
                 led_cdev->pattern_clear(led_cdev);
 	...
}

> > +	init_data.fwnode = mc_node;
> > +
> > +	ncp->mc_dev.led_cdev.max_brightness = NCP5623_MAX_BRIGHTNESS;
> > +	ncp->mc_dev.subled_info = subled_info;
> > +	ncp->mc_dev.led_cdev.brightness_set_blocking = ncp5623_brightness_set;
> > +	ncp->mc_dev.led_cdev.pattern_set = ncp5623_pattern_set;
> > +	ncp->mc_dev.led_cdev.pattern_clear = ncp5623_pattern_clear;
> > +	ncp->mc_dev.led_cdev.default_trigger = "pattern";
> > +
> > +	mutex_init(&ncp->lock);
> > +	i2c_set_clientdata(client, ncp);
> > +
> > +	ret = led_classdev_multicolor_register_ext(dev, &ncp->mc_dev, &init_data);
> > +	if (ret)
> > +		goto destroy_lock;
> > +
> > +	fwnode_handle_put(mc_node);
> 
> Didn't you just store this ~16 lines up?
> 
Ops! I'll remove it.

Thanks,
Abdel