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

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

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

-- 
Best regards,
Jacek Anaszewski