Re: [PATCH v9] leds: add support for TI LP5860 LED driver chip

[Steffen Trumtrar <s.trumtrar@pengutronix.de>]

On 2026-05-04 at 21:33 +02, Jacek Anaszewski <jacek.anaszewski@gmail.com> wrote:

Hi,

> On 5/3/26 8:19 PM, Steffen Trumtrar wrote:
> > On 2026-04-26 at 14:41 +02, Jacek Anaszewski <jacek.anaszewski@gmail.com>
> > wrote:
> > Hi,
> > 
> >> Hi Steffen,
> >>
> >> On 4/24/26 3:28 PM, Steffen Trumtrar wrote:
> >> > Add support for the Texas Instruments LP5860 LED driver chip
> >> > via SPI interfaces.
> >> > The LP5860 is an LED matrix driver for up to 196 LEDs, which supports
> >> > short and open detection of the individual channel select lines.
> >> > It can be connected to SPI or I2C bus. For now add support for SPI only.
> >> > Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> >> > ---
> >> [...]
> >> > diff --git a/drivers/leds/rgb/leds-lp5860-core.c b/drivers/leds/rgb/
> >> leds-lp5860-core.c
> >> > new file mode 100644
> >> > index 0000000000000..648bf168f94bf
> >> > --- /dev/null
> >> > +++ b/drivers/leds/rgb/leds-lp5860-core.c
> >> > @@ -0,0 +1,231 @@
> >> > +// SPDX-License-Identifier: GPL-2.0-only
> >> > +/*
> >> > + * Copyright (c) 2025 Pengutronix
> >> > + *
> >> > + * Author: Steffen Trumtrar <kernel@pengutronix.de>
> >> > + */
> >> > +
> >> > +#include <linux/led-class-multicolor.h>
> >> > +#include <linux/module.h>
> >> > +#include <linux/of_platform.h>
> >> > +#include <linux/property.h>
> >> > +#include <linux/regmap.h>
> >> > +
> >> > +#include "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);
> >> > +}
> >> > +
> >> > +static int lp5860_set_dot_onoff(struct lp5860_led *led, unsigned int dot,
> >> bool enable)
> >> > +{
> >> > +    unsigned int offset = dot / LP5860_MAX_DOT_ONOFF_GROUP_NUM;
> >> > +    unsigned int mask = BIT(dot % LP5860_MAX_DOT_ONOFF_GROUP_NUM);
> >> > +
> >> > +    if (dot > LP5860_MAX_LED)
> >> > +        return -EINVAL;
> >> > +
> >> > +    return regmap_update_bits(led->chip->regmap,
> >> > +                  LP5860_REG_DOT_ONOFF_START + offset, mask,
> >> > +                  enable ? LP5860_DOT_ALL_ON : LP5860_DOT_ALL_OFF);
> >> > +}
> >> > +
> >> > +static int lp5860_set_mc_brightness(struct led_classdev *cdev,
> >> > +                    enum led_brightness brightness)
> >> > +{
> >> > +    struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(cdev);
> >> > +    struct lp5860_led *led = mcled_cdev_to_led(mc_cdev);
> >>
> >> You need mutex locking while accessing hw to avoid leaving the
> >> device in an inconsistent state in case of two parallel requests from
> >> different processes.
> >>
> > The hw is only accessed via regmap. It handles locking AFAIK or do I
> > misunderstand you?
> 
> 
> You're calling regmap_write() per subLED, so internal regmap
> locking has nothing to do with synchronizing the state of multicolor
> LED. It is possible that another Process2 jumps in while Process1
> has already written two subLEDs. Let's say that there are two
> parallel calls, where Process2 has greater priority:
> 
> Process1:
> echo "10 20 30" > multi_intensity
> Process2:
> echo "40 50 60" > multi_intensity
> 
> Process1:
> 	regmap_write subLED 1 10
> 	regmap_write subLED 2 20
> Process2:
> 	regmap_write subLED 1 40
> 	regmap_write subLED 2 50
> 	regmap_write subLED 3 60
> Process1:
> 	regmap_write subLED 3 30
> 	
> 
> Which leaves the multicolor LED registers in the state:
> 
> subLED1: 40
> subLED2: 50
> subLED3: 30

Okay, got it. Makes sense.


Thanks,
Steffen

-- 
Pengutronix e.K.                | Dipl.-Inform. Steffen Trumtrar |
Steuerwalder Str. 21            | https://www.pengutronix.de/    |
31137 Hildesheim, Germany       | Phone: +49-5121-206917-0       |
Amtsgericht Hildesheim, HRA 2686| Fax:   +49-5121-206917-5555    |