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
