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

[Abdel Alkuor <alkuor@gmail.com>]

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