Re: [PATCH v3 2/3] leds: add support for TI LP5860 LED driver chip

[Jacek Anaszewski <jacek.anaszewski@gmail.com>]

Hi Steffen

On 9/30/25 09:40, Steffen Trumtrar wrote:
> 
> Hi,
> 
> On 2025-09-16 at 16:34 +01, Lee Jones <lee@kernel.org> wrote:
> 
>> > +#include <linux/gpio.h>
>> > +#include <linux/led-class-multicolor.h>
>> > +#include <linux/module.h>
>> > +#include <linux/of_gpio.h>
>> > +#include <linux/of_platform.h>
>> > +#include <linux/regmap.h>
>> > +
>> > +#include <linux/platform_data/leds-lp5860.h>
>> > +
>> > +static struct lp5860_led *mcled_cdev_to_led(struct led_classdev_mc 
>> *mc_cdev)
>> > +{
>> > +    return container_of(mc_cdev, struct lp5860_led, mc_cdev);
>> > +}
>> > +
>> > +LP5860_SHOW_MODE(r_global_brightness_set, LP5860_REG_R_CURRENT_SET, 
>> LP5860_CC_GROUP_MASK, 0)
>> > +LP5860_STORE_MODE(r_global_brightness_set, 
>> LP5860_REG_R_CURRENT_SET, LP5860_CC_GROUP_MASK, 0)
>> > +DEVICE_ATTR_RW(r_global_brightness_set);
>>
>> How is this different to /sys/class/leds/<led>/multi_intensity?
>>
>> # echo 43 226 138 > /sys/class/leds/multicolor:status/multi_intensity
>> red -
>>     intensity = 138
>>     max_brightness = 255
>> green -
>>     intensity = 43
>>     max_brightness = 255
>> blue -
>>     intensity = 226
>>     max_brightness = 255
>>
> 
> the LP5860 has a register for setting the maximal brightness that holds 
> for all LEDs in the matrix. multi_intensity and max_brightness is only 
> for that one multicolor LED, right? And I can only manipulate the 
> max_brightness of that one multicolor LED instance.
> If I'm wrong, I'd be happy to not have to add the sysfs files.

It seems that this device is similar in the aspect of LED grouping
to LP50xx family. There is already a driver for that one [0] with
related bindings. Grouping solution could be addressed similarly to the
banking mechanism in that driver.

That of course means that this patch needs a significant rework.

First thing that strikes me after analyzing datasheet is that
LEDs are not assigned to any group since LP5860_REG_GRP_SEL_START
address is not referenced anywhere, and this is base address for
Dot_grp_selN registers that enable affiliation of the LED to given
group. No need for global brightness setting then.

Anyway, I'd add proper support for this device with DT knobs
to enable both grouping and individual approach to controlling the LEDs.

Driver [0] should serve as good guidance for that.

[0] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/leds/leds-lp50xx.c

-- 
Best regards,
Jacek Anaszewski