lkml.org 
[lkml]   [2024]   [Mar]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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

\
 
 \ /
  Last update: 2024-05-27 15:34    [W:0.033 / U:0.356 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site