* RE: [PATCH v9 2/2] leds: ltc3220: Add Support for LTC3220 18 channel LED Driver
From: Escala, Edelweise @ 2026-06-15 1:12 UTC (permalink / raw)
To: Lee Jones
Cc: Pavel Machek, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
linux-leds@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20260611114741.GP4151951@google.com>
Hi Lee,
Yes I'll take care of these.
> -----Original Message-----
> From: Lee Jones <lee@kernel.org>
> Sent: Thursday, June 11, 2026 7:48 PM
> To: Escala, Edelweise <Edelweise.Escala@analog.com>
> Cc: Pavel Machek <pavel@kernel.org>; Rob Herring <robh@kernel.org>; Krzysztof
> Kozlowski <krzk+dt@kernel.org>; Conor Dooley <conor+dt@kernel.org>; linux-
> leds@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v9 2/2] leds: ltc3220: Add Support for LTC3220 18 channel
> LED Driver
>
> [External]
>
> /* Sashiko Automation: Issues Found (10 Findings) */
>
> Would you mind taking care of these before I conduct my next review please?
>
> On Thu, 28 May 2026, Edelweise Escala wrote:
>
> > Add driver for the LTC3220 18-channel LED driver with I2C interface,
> > individual brightness control, and hardware-assisted blink/gradation
> > features.
> >
> > Signed-off-by: Edelweise Escala <edelweise.escala@analog.com>
> > ---
> > MAINTAINERS | 1 +
> > drivers/leds/Kconfig | 13 ++
> > drivers/leds/Makefile | 1 +
> > drivers/leds/leds-ltc3220.c | 440
> > ++++++++++++++++++++++++++++++++++++++++++++
> > 4 files changed, 455 insertions(+)
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS index
> > c8a242577d2f..0f553ada61d9 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -15229,6 +15229,7 @@ L: linux-leds@vger.kernel.org
> > S: Maintained
> > W: https://ez.analog.com/linux-software-drivers
> > F: Documentation/devicetree/bindings/leds/adi,ltc3220.yaml
> > +F: drivers/leds/leds-ltc3220.c
> >
> > LTC4282 HARDWARE MONITOR DRIVER
> > M: Nuno Sa <nuno.sa@analog.com>
> > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig index
> > f4a0a3c8c870..31b1e3ff094c 100644
> > --- a/drivers/leds/Kconfig
> > +++ b/drivers/leds/Kconfig
> > @@ -1000,6 +1000,19 @@ config LEDS_ST1202
> > Say Y to enable support for LEDs connected to LED1202
> > LED driver chips accessed via the I2C bus.
> >
> > +config LEDS_LTC3220
> > + tristate "LED Driver for Analog Devices Inc. LTC3220"
> > + depends on I2C && LEDS_CLASS
> > + select REGMAP_I2C
> > + help
> > + Say Y to enable support for the Analog Devices LTC3220
> > + 18-channel LED controller with I2C interface.
> > + The driver supports individual LED brightness control (64 steps),
> > + hardware-assisted blinking and gradation effects.
> > +
> > + To compile this driver as a module, choose M here: the module will
> > + be called leds-ltc3220.
> > +
> > config LEDS_TPS6105X
> > tristate "LED support for TI TPS6105X"
> > depends on LEDS_CLASS
> > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile index
> > 7db3768912ca..a68244bd50fb 100644
> > --- a/drivers/leds/Makefile
> > +++ b/drivers/leds/Makefile
> > @@ -61,6 +61,7 @@ obj-$(CONFIG_LEDS_LP8788) += leds-
> lp8788.o
> > obj-$(CONFIG_LEDS_LP8860) += leds-lp8860.o
> > obj-$(CONFIG_LEDS_LP8864) += leds-lp8864.o
> > obj-$(CONFIG_LEDS_LT3593) += leds-lt3593.o
> > +obj-$(CONFIG_LEDS_LTC3220) += leds-ltc3220.o
> > obj-$(CONFIG_LEDS_MAX5970) += leds-max5970.o
> > obj-$(CONFIG_LEDS_MAX77650) += leds-max77650.o
> > obj-$(CONFIG_LEDS_MAX77705) += leds-max77705.o
> > diff --git a/drivers/leds/leds-ltc3220.c b/drivers/leds/leds-ltc3220.c
> > new file mode 100644 index 000000000000..a633ac5ca6ae
> > --- /dev/null
> > +++ b/drivers/leds/leds-ltc3220.c
> > @@ -0,0 +1,440 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * LTC3220 18-Channel LED Driver
> > + *
> > + * Copyright 2026 Analog Devices Inc.
> > + *
> > + * Author: Edelweise Escala <edelweise.escala@analog.com> */
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/delay.h>
>
> [Severity: Low]
> Is this include needed?
>
> It doesn't appear that any delay functions like msleep() or usleep_range() are
> used in the driver implementation.
>
I'll probably need delay again for the reset comment later.
>
> > +#include <linux/device.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/i2c.h>
> > +#include <linux/leds.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/module.h>
> > +#include <linux/regmap.h>
> > +#include <linux/types.h>
>
> [Severity: Low]
> Should this file include <linux/property.h>?
>
> The driver uses fwnode property APIs such as
> device_for_each_child_node_scoped(),
> fwnode_property_read_u32(), and fwnode_property_present(), which are
> defined in <linux/property.h>.
>
>
I'll check this.
> > +
> > +/* LTC3220 Registers */
> > +#define LTC3220_COMMAND_REG 0x00
> > +#define LTC3220_QUICK_WRITE_MASK BIT(0)
> > +#define LTC3220_SHUTDOWN_MASK BIT(3)
> > +
> > +#define LTC3220_ULED_REG(x) (0x01 + (x))
> > +#define LTC3220_LED_CURRENT_MASK GENMASK(5,
> 0)
> > +#define LTC3220_LED_MODE_MASK GENMASK(7,
> 6)
> > +
> > +#define LTC3220_GRAD_BLINK_REG 0x13
> > +#define LTC3220_GRADATION_MASK GENMASK(2, 0)
> > +#define LTC3220_GRADATION_DIRECTION_MASK BIT(0)
> > +#define LTC3220_GRADATION_PERIOD_MASK
> GENMASK(2, 1)
> > +#define LTC3220_BLINK_MASK GENMASK(4,
> 3)
> > +
> > +#define LTC3220_NUM_LEDS 18
> > +
> > +#define LTC3220_GRADATION_START_VALUE 128
>
> [Severity: Low]
> Is this macro used anywhere in the driver?
>
> It appears to be dead code as it is never referenced.
>
>
Will remove
> > +#define LTC3220_GRADATION_RAMP_TIME_240MS 240
> > +#define LTC3220_GRADATION_RAMP_TIME_480MS 480
> > +
> > +#define LTC3220_BLINK_ON_156MS 156
> > +#define LTC3220_BLINK_ON_625MS 625
> > +#define LTC3220_BLINK_PERIOD_1250MS 1250
> > +#define LTC3220_BLINK_PERIOD_2500MS 2500
> > +
> > +#define LTC3220_BLINK_SHORT_ON_TIME BIT(0)
> > +#define LTC3220_BLINK_LONG_PERIOD BIT(1)
> > +
> > +enum ltc3220_led_mode {
> > + LTC3220_NORMAL_MODE,
> > + LTC3220_BLINK_MODE,
> > + LTC3220_GRADATION_MODE,
> > +};
> > +
> > +enum ltc3220_blink_mode {
> > + LTC3220_BLINK_MODE_625MS_1250MS,
> > + LTC3220_BLINK_MODE_156MS_1250MS,
> > + LTC3220_BLINK_MODE_625MS_2500MS,
> > + LTC3220_BLINK_MODE_156MS_2500MS
> > +};
> > +
> > +enum ltc3220_gradation_mode {
> > + LTC3220_GRADATION_MODE_DISABLED,
> > + LTC3220_GRADATION_MODE_240MS_RAMP_TIME,
> > + LTC3220_GRADATION_MODE_480MS_RAMP_TIME,
> > + LTC3220_GRADATION_MODE_960MS_RAMP_TIME
> > +};
> > +
> > +static bool ltc3220_volatile_reg(struct device *dev, unsigned int
> > +reg) {
> > + return reg == LTC3220_GRAD_BLINK_REG; }
>
> [Severity: High]
> Does this register need to be marked volatile?
>
> Marking a register as volatile tells regmap to bypass the cache and read from the
> physical hardware. If the LTC3220 is a write-only I2C device, the physical read will
> fail, causing regmap_update_bits() to fail entirely in
> ltc3220_pattern_set() and ltc3220_blink_set().
>
> Even if the device supports reads, this register is exclusively controlled by
> software and does not change autonomously, so could it be cached normally to
> avoid unnecessary I2C reads?
>
>
Will check if it will work if I remove this, I had an issue with the gradation and blink register when I did not add volatile regisgters
> > +
> > +static const struct regmap_config ltc3220_regmap_config = {
> > + .reg_bits = 8,
> > + .val_bits = 8,
> > + .max_register = LTC3220_GRAD_BLINK_REG,
> > + .cache_type = REGCACHE_FLAT_S,
> > + .volatile_reg = ltc3220_volatile_reg, };
> > +
> > +struct ltc3220_uled_cfg {
> > + struct led_classdev led_cdev;
> > + u8 reg_value;
> > + u8 led_index;
> > +};
> > +
> > +struct ltc3220 {
> > + struct ltc3220_uled_cfg uled_cfg[LTC3220_NUM_LEDS];
> > + struct regmap *regmap;
> > + bool is_aggregated;
> > +};
> > +
> > +/*
> > + * Set LED brightness. Hardware supports 0-63 brightness levels.
> > + * Mode switching (blink/gradation) is handled through dedicated callbacks.
> > + */
> > +static int ltc3220_set_led_data(struct led_classdev *led_cdev,
> > + enum led_brightness brightness)
> > +{
> > + struct ltc3220_uled_cfg *uled_cfg = container_of(led_cdev, struct
> ltc3220_uled_cfg,
> > + led_cdev);
> > + struct ltc3220 *ltc3220 = container_of(uled_cfg, struct ltc3220,
> > + uled_cfg[uled_cfg->led_index]);
>
> [Severity: Low]
> Will this dynamic variable index inside container_of() cause a build failure?
>
> The driver attempts to retrieve the parent structure using uled_cfg[uled_cfg-
> >led_index]. The container_of macro relies on offsetof(), which strictly requires
> the member argument to be a constant expression.
>
> Since uled_cfg->led_index is a runtime variable, this violates the C standard and
> can abort compilation on compilers like Clang with an error about a non-constant
> integer expression.
>
>
Will fix this as well as other container of issues
> > + int ret;
> > +
> > + ret = regmap_write(ltc3220->regmap, LTC3220_ULED_REG(uled_cfg-
> >led_index),
> > + brightness);
> > + if (ret)
> > + return ret;
> > +
> > + uled_cfg->reg_value = brightness;
> > +
> > + /*
> > + * When aggregated LED mode is enabled, writing to LED 1 updates all
> > + * LEDs simultaneously via quick-write mode. Update cached values for
> > + * all LEDs to reflect the synchronized state.
> > + * See Documentation/devicetree/bindings/leds/adi,ltc3220.yaml for
> how
> > + * to configure aggregated LED mode.
> > + */
> > + if (ltc3220->is_aggregated && uled_cfg->led_index == 0) {
> > + for (int i = 0; i < LTC3220_NUM_LEDS; i++)
> > + ltc3220->uled_cfg[i].reg_value = brightness;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static enum led_brightness ltc3220_get_led_data(struct led_classdev
> > +*led_cdev) {
> > + struct ltc3220_uled_cfg *uled_cfg = container_of(led_cdev, struct
> ltc3220_uled_cfg,
> > + led_cdev);
> > +
> > + return uled_cfg->reg_value;
> > +}
> > +
> > +/*
> > + * LTC3220 pattern support for hardware-assisted breathing/gradation.
> > + * The hardware supports 3 gradation ramp time 240ms, 480ms, 960ms)
> > + * and can ramp up or down.
> > + *
> > + * Pattern array interpretation:
> > + * pattern[0].brightness = start brightness (0-63)
> > + * pattern[0].delta_t = ramp time in milliseconds
> > + * pattern[1].brightness = end brightness (0-63)
> > + * pattern[1].delta_t = (optional, can be 0 or same as pattern[0].delta_t)
> > + */
> > +static int ltc3220_pattern_set(struct led_classdev *led_cdev,
> > + struct led_pattern *pattern,
> > + u32 len, int repeat)
> > +{
> > + struct ltc3220_uled_cfg *uled_cfg = container_of(led_cdev, struct
> ltc3220_uled_cfg,
> > + led_cdev);
> > + struct ltc3220 *ltc3220 = container_of(uled_cfg, struct ltc3220,
> > + uled_cfg[uled_cfg->led_index]);
>
> [Severity: Low]
> Will this dynamic variable index inside container_of() cause a build failure?
>
> The driver attempts to retrieve the parent structure using uled_cfg[uled_cfg-
> >led_index]. The container_of macro relies on offsetof(), which strictly requires
> the member argument to be a constant expression.
>
> Since uled_cfg->led_index is a runtime variable, this violates the C standard and
> can abort compilation on compilers like Clang with an error about a non-constant
> integer expression.
>
>
> > + u8 gradation_period;
> > + u8 start_brightness;
> > + u8 end_brightness;
> > + u8 gradation_val;
> > + bool is_increasing;
> > + int ret;
> > +
> > + if (len != 2)
> > + return -EINVAL;
> > +
> > + start_brightness = pattern[0].brightness &
> LTC3220_LED_CURRENT_MASK;
> > + end_brightness = pattern[1].brightness &
> LTC3220_LED_CURRENT_MASK;
> > +
> > + is_increasing = end_brightness > start_brightness;
> > +
> > + if (pattern[0].delta_t == 0)
> > + gradation_period = LTC3220_GRADATION_MODE_DISABLED;
> > + else if (pattern[0].delta_t <=
> LTC3220_GRADATION_RAMP_TIME_240MS)
> > + gradation_period =
> LTC3220_GRADATION_MODE_240MS_RAMP_TIME;
> > + else if (pattern[0].delta_t <=
> LTC3220_GRADATION_RAMP_TIME_480MS)
> > + gradation_period =
> LTC3220_GRADATION_MODE_480MS_RAMP_TIME;
> > + else
> > + gradation_period =
> LTC3220_GRADATION_MODE_960MS_RAMP_TIME;
> > +
> > + gradation_val = FIELD_PREP(LTC3220_GRADATION_PERIOD_MASK,
> gradation_period);
> > + gradation_val |= FIELD_PREP(LTC3220_GRADATION_DIRECTION_MASK,
> > +is_increasing);
> > +
> > + ret = regmap_update_bits(ltc3220->regmap,
> LTC3220_GRAD_BLINK_REG,
> > + LTC3220_GRADATION_MASK, gradation_val);
> > + if (ret)
> > + return ret;
> > +
> > + ret = ltc3220_set_led_data(led_cdev, start_brightness);
> > + if (ret)
> > + return ret;
> > +
> > + ret = regmap_write(ltc3220->regmap, LTC3220_ULED_REG(uled_cfg-
> >led_index),
> > + FIELD_PREP(LTC3220_LED_MODE_MASK,
> LTC3220_GRADATION_MODE) |
> > + end_brightness);
> > + if (ret)
> > + return ret;
> > +
> > + uled_cfg->reg_value = end_brightness;
> > +
> > + return 0;
> > +}
> > +
> > +static int ltc3220_pattern_clear(struct led_classdev *led_cdev) {
> > + struct ltc3220_uled_cfg *uled_cfg = container_of(led_cdev, struct
> ltc3220_uled_cfg,
> > + led_cdev);
> > + struct ltc3220 *ltc3220 = container_of(uled_cfg, struct ltc3220,
> > + uled_cfg[uled_cfg->led_index]);
>
> [Severity: Low]
> Will this dynamic variable index inside container_of() cause a build failure?
>
> The driver attempts to retrieve the parent structure using uled_cfg[uled_cfg-
> >led_index]. The container_of macro relies on offsetof(), which strictly requires
> the member argument to be a constant expression.
>
> Since uled_cfg->led_index is a runtime variable, this violates the C standard and
> can abort compilation on compilers like Clang with an error about a non-constant
> integer expression.
>
>
> > + int ret;
> > +
> > + ret = regmap_update_bits(ltc3220->regmap,
> LTC3220_ULED_REG(uled_cfg->led_index),
> > + LTC3220_LED_MODE_MASK,
> LTC3220_NORMAL_MODE);
> > + if (ret)
> > + return ret;
> > +
> > + return ltc3220_set_led_data(led_cdev, LED_OFF); }
> > +
> > +/*
> > + * LTC3220 has a global blink configuration that affects all LEDs.
> > + * This implementation allows per-LED blink requests, but the blink
> > +timing
> > + * will be shared across all LEDs. The delay values are mapped to the
> > + * hardware's discrete blink rates.
> > + */
> > +static int ltc3220_blink_set(struct led_classdev *led_cdev,
> > + unsigned long *delay_on,
> > + unsigned long *delay_off)
> > +{
> > + struct ltc3220_uled_cfg *uled_cfg = container_of(led_cdev, struct
> ltc3220_uled_cfg,
> > + led_cdev);
> > + struct ltc3220 *ltc3220 = container_of(uled_cfg, struct ltc3220,
> > + uled_cfg[uled_cfg->led_index]);
>
> [Severity: Low]
> Will this dynamic variable index inside container_of() cause a build failure?
>
> The driver attempts to retrieve the parent structure using uled_cfg[uled_cfg-
> >led_index]. The container_of macro relies on offsetof(), which strictly requires
> the member argument to be a constant expression.
>
> Since uled_cfg->led_index is a runtime variable, this violates the C standard and
> can abort compilation on compilers like Clang with an error about a non-constant
> integer expression.
>
>
> > + u8 blink_mode = 0;
> > + int ret;
> > +
> > + if (*delay_on <= LTC3220_BLINK_ON_156MS)
> > + blink_mode = LTC3220_BLINK_SHORT_ON_TIME;
> > +
> > + if (*delay_on + *delay_off > LTC3220_BLINK_PERIOD_1250MS)
> > + blink_mode |= LTC3220_BLINK_LONG_PERIOD;
> > +
> > + switch (blink_mode) {
> > + case LTC3220_BLINK_MODE_625MS_1250MS:
> > + *delay_on = LTC3220_BLINK_ON_625MS;
> > + *delay_off = LTC3220_BLINK_PERIOD_1250MS -
> LTC3220_BLINK_ON_625MS;
> > + break;
> > + case LTC3220_BLINK_MODE_156MS_1250MS:
> > + *delay_on = LTC3220_BLINK_ON_156MS;
> > + *delay_off = LTC3220_BLINK_PERIOD_1250MS -
> LTC3220_BLINK_ON_156MS;
> > + break;
> > + case LTC3220_BLINK_MODE_625MS_2500MS:
> > + *delay_on = LTC3220_BLINK_ON_625MS;
> > + *delay_off = LTC3220_BLINK_PERIOD_2500MS -
> LTC3220_BLINK_ON_625MS;
> > + break;
> > + case LTC3220_BLINK_MODE_156MS_2500MS:
> > + *delay_on = LTC3220_BLINK_ON_156MS;
> > + *delay_off = LTC3220_BLINK_PERIOD_2500MS -
> LTC3220_BLINK_ON_156MS;
> > + break;
> > + }
> > +
> > + ret = regmap_update_bits(ltc3220->regmap,
> LTC3220_GRAD_BLINK_REG,
> > + LTC3220_BLINK_MASK,
> FIELD_PREP(LTC3220_BLINK_MASK, blink_mode));
> > + if (ret)
> > + return ret;
> > +
> > + return regmap_update_bits(ltc3220->regmap,
> LTC3220_ULED_REG(uled_cfg->led_index),
> > + LTC3220_LED_MODE_MASK,
> > + FIELD_PREP(LTC3220_LED_MODE_MASK,
> LTC3220_BLINK_MODE)); }
>
> [Severity: Medium]
> Will the hardware blink activate at 0 brightness if the LED was previously off?
>
> When the LED is off, its current bits in LTC3220_ULED_REG are 0. The driver
> updates the mode bits to LTC3220_BLINK_MODE using regmap_update_bits() but
> does not update the brightness bits.
>
> Because the core expects the driver to handle brightness configuration for
> hardware blinking, could this hardware blink execute at 0 brightness, leaving the
> LED unlit?
>
Will make sure to have brightness when setting blinking
>
> > +
> > +static void ltc3220_reset_gpio_action(void *data) {
> > + struct gpio_desc *reset_gpio = data;
> > +
> > + gpiod_set_value_cansleep(reset_gpio, 1); }
> > +
> > +static int ltc3220_reset(struct ltc3220 *ltc3220, struct i2c_client
> > +*client) {
> > + struct gpio_desc *reset_gpio;
> > + int ret;
> > +
> > + reset_gpio = devm_gpiod_get_optional(&client->dev, "reset",
> GPIOD_OUT_HIGH);
> > + if (IS_ERR(reset_gpio))
> > + return dev_err_probe(&client->dev, PTR_ERR(reset_gpio),
> "Failed on
> > +reset GPIO\n");
> > +
> > + if (reset_gpio) {
> > + gpiod_set_value_cansleep(reset_gpio, 0);
>
> [Severity: High]
> Does this sequence lack the necessary delays to ensure a successful reset?
>
> The driver asserts the reset GPIO by requesting it with GPIOD_OUT_HIGH and
> immediately de-asserts it with gpiod_set_value_cansleep(reset_gpio, 0). There is
> no delay between these calls, creating a near-zero width pulse that may fail to
> meet the IC's minimum reset pulse width requirement.
>
> It also lacks a wakeup delay before subsequent I2C commands might be sent.
>
Will add delays
>
> > +
> > + return devm_add_action_or_reset(&client->dev,
> ltc3220_reset_gpio_action,
> > + reset_gpio);
>
> [Severity: High]
> Does returning early here skip software initialization and regmap cache setup?
>
> If the reset_gpio is successfully acquired, this function immediately returns 0,
> entirely skipping the subsequent regmap_write() calls intended to initialize the
> hardware registers.
>
> Because the driver does not define a .reg_defaults table, skipping these writes
> leaves the regmap cache uninitialized, which can break subsequent read-modify-
> write logic via regmap_update_bits().
>
>
> > + }
> > +
> > + ret = regmap_write(ltc3220->regmap, LTC3220_COMMAND_REG, 0);
> > + if (ret)
> > + return ret;
> > +
> > + for (int i = 0; i < LTC3220_NUM_LEDS; i++) {
> > + ret = regmap_write(ltc3220->regmap, LTC3220_ULED_REG(i), 0);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + return regmap_write(ltc3220->regmap, LTC3220_GRAD_BLINK_REG, 0); }
> > +
> > +static int ltc3220_suspend(struct device *dev) {
> > + struct ltc3220 *ltc3220 = i2c_get_clientdata(to_i2c_client(dev));
> > +
> > + return regmap_update_bits(ltc3220->regmap,
> LTC3220_COMMAND_REG,
> > + LTC3220_SHUTDOWN_MASK,
> LTC3220_SHUTDOWN_MASK); }
> > +
> > +static int ltc3220_resume(struct device *dev) {
> > + struct ltc3220 *ltc3220 = i2c_get_clientdata(to_i2c_client(dev));
> > +
> > + return regmap_update_bits(ltc3220->regmap,
> LTC3220_COMMAND_REG,
> > + LTC3220_SHUTDOWN_MASK, 0);
> > +}
>
> [Severity: High]
> Are the hardware registers left permanently off after resume?
>
> In ltc3220_suspend(), the driver sets the LTC3220_SHUTDOWN_MASK bit. On
> many such chips, asserting the shutdown bit resets internal registers. Additionally,
> power to the IC may be lost during system suspend.
>
> In ltc3220_resume(), the driver clears the shutdown bit but does not call
> regcache_mark_dirty() and regcache_sync(). Could this leave the physical LED
> outputs permanently off, while the kernel's regmap cache and LED classdev state
> incorrectly reflect that the LEDs are still configured and illuminated?
>
Will add regcache_sync() call to ltc3220_resume()
> > +
> > +static DEFINE_SIMPLE_DEV_PM_OPS(ltc3220_pm_ops, ltc3220_suspend,
> > +ltc3220_resume);
> > +
> > +static int ltc3220_probe(struct i2c_client *client) {
> > + struct ltc3220 *ltc3220;
> > + bool aggregated_led_found = false;
> > + int num_leds = 0;
> > + u8 led_index = 0;
> > + int ret;
> > +
> > + ltc3220 = devm_kzalloc(&client->dev, sizeof(*ltc3220), GFP_KERNEL);
> > + if (!ltc3220)
> > + return -ENOMEM;
> > +
> > + ltc3220->regmap = devm_regmap_init_i2c(client,
> <c3220_regmap_config);
> > + if (IS_ERR(ltc3220->regmap))
> > + return dev_err_probe(&client->dev, PTR_ERR(ltc3220->regmap),
> > + "Failed to initialize regmap\n");
> > +
> > + i2c_set_clientdata(client, ltc3220);
> > +
> > + ret = ltc3220_reset(ltc3220, client);
> > + if (ret)
> > + return dev_err_probe(&client->dev, ret, "Failed to reset
> > +device\n");
> > +
> > + device_for_each_child_node_scoped(&client->dev, child) {
> > + struct led_init_data init_data = {};
> > + struct ltc3220_uled_cfg *led;
> > + u32 source;
> > +
> > + ret = fwnode_property_read_u32(child, "reg", &source);
> > + if (ret)
> > + return dev_err_probe(&client->dev, ret, "Couldn't read
> LED
> > +address\n");
> > +
> > + if (!source || source > LTC3220_NUM_LEDS)
> > + return dev_err_probe(&client->dev, -EINVAL, "LED
> address out of
> > +range\n");
> > +
> > + init_data.fwnode = child;
> > + init_data.devicename = "ltc3220";
> > +
> > + if (fwnode_property_present(child, "led-sources")) {
> > + if (source != 1)
> > + return dev_err_probe(&client->dev, -EINVAL,
> > + "Aggregated LED out of
> range\n");
> > +
> > + if (aggregated_led_found)
> > + return dev_err_probe(&client->dev, -EINVAL,
> > + "One Aggregated LED
> only\n");
> > +
> > + aggregated_led_found = true;
> > + ltc3220->is_aggregated = true;
> > +
> > + ret = regmap_update_bits(ltc3220->regmap,
> > + LTC3220_COMMAND_REG,
> > +
> LTC3220_QUICK_WRITE_MASK,
> > +
> LTC3220_QUICK_WRITE_MASK);
> > + if (ret)
> > + return dev_err_probe(&client->dev, ret,
> > + "Failed to set quick write
> mode\n");
> > + }
> > +
> > + num_leds++;
> > +
> > + /* LED node reg/index/address goes from 1 to 18 */
> > + led_index = source - 1;
> > + led = <c3220->uled_cfg[led_index];
> > +
> > + if (led->led_cdev.dev)
> > + return dev_err_probe(&client->dev, -EINVAL,
> > + "Duplicate LED reg %u found",
> source);
> > +
> > + led->led_index = led_index;
> > + led->reg_value = 0;
> > + led->led_cdev.brightness_set_blocking = ltc3220_set_led_data;
> > + led->led_cdev.brightness_get = ltc3220_get_led_data;
> > + led->led_cdev.max_brightness = 63;
> > + led->led_cdev.blink_set = ltc3220_blink_set;
> > + led->led_cdev.pattern_set = ltc3220_pattern_set;
> > + led->led_cdev.pattern_clear = ltc3220_pattern_clear;
> > +
> > + ret = devm_led_classdev_register_ext(&client->dev, &led-
> >led_cdev, &init_data);
> > + if (ret)
> > + return dev_err_probe(&client->dev, ret, "Failed to
> register LED class\n");
> > + }
> > +
> > + /*
> > + * Aggregated LED mode uses hardware quick-write to control all 18 LEDs
> > + * simultaneously. This is mutually exclusive with individual LED control.
> > + * See Documentation/devicetree/bindings/leds/adi,ltc3220.yaml for
> details
> > + * on how to configure aggregated LED mode.
> > + */
> > + if (aggregated_led_found && num_leds > 1)
> > + return dev_err_probe(&client->dev, -EINVAL,
> > + "Aggregated LED must be the only LED
> node\n");
>
> [Severity: Medium]
> Are the LED sysfs interfaces exposed to userspace before the driver probe is fully
> validated?
>
> Inside the loop above, devm_led_classdev_register_ext() is called, which
> immediately exposes sysfs interfaces. If the validation check here fails and
> returns an error, userspace could have already discovered and interacted with the
> LED devices before they are unexpectedly torn down by devres.
>
> Could these validation checks be completed before registering the interfaces?
>
Restructured ltc3220_probe() to use two passes:
- First pass: Validate all device tree configuration and set up LED structures
- Second pass: Register LED class devices only after validation succeeds
> > +
> > + return 0;
> > +}
> > +
> > +static const struct of_device_id ltc3220_of_match[] = {
> > + { .compatible = "adi,ltc3220" },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(of, ltc3220_of_match);
> > +
> > +static struct i2c_driver ltc3220_led_driver = {
> > + .driver = {
> > + .name = "ltc3220",
> > + .of_match_table = ltc3220_of_match,
> > + .pm = pm_sleep_ptr(<c3220_pm_ops),
> > + },
> > + .probe = ltc3220_probe,
> > +};
> > +module_i2c_driver(ltc3220_led_driver);
> > +
> > +MODULE_AUTHOR("Edelweise Escala <edelweise.escala@analog.com>");
> > +MODULE_DESCRIPTION("LED driver for LTC3220 controllers");
> > +MODULE_LICENSE("GPL");
> >
> > --
> > 2.43.0
> >
>
> --
> Lee Jones
Thank you!
Best Regards,
Edelweise Escala
^ permalink raw reply
* [PATCH v2] leds: tests: use a fresh instance for name conflict rejection
From: Lorenzo Egidio @ 2026-06-12 23:06 UTC (permalink / raw)
To: lee; +Cc: pavel, linux-leds, Lorenzo Egidio
The LED_REJECT_NAME_CONFLICT test currently re-registers an
already registered led_classdev instance.
Use a fresh copy instead so the test exercises the
name-conflict rejection path directly.
Signed-off-by: Lorenzo Egidio <lorenzoegidioms@gmail.com>
---
drivers/leds/led-test.c | 102 ++++++++++++++++++++++++++--------------
1 file changed, 66 insertions(+), 36 deletions(-)
diff --git a/drivers/leds/led-test.c b/drivers/leds/led-test.c
index ddf9aa967..36aef3e13 100644
--- a/drivers/leds/led-test.c
+++ b/drivers/leds/led-test.c
@@ -1,4 +1,5 @@
// SPDX-License-Identifier: GPL-2.0-only
+// leds: tests: clarify name conflict test
/*
* Copyright (C) 2025 Google LLC
*
@@ -10,77 +11,106 @@
#include <linux/device.h>
#include <linux/leds.h>
-#define LED_TEST_POST_REG_BRIGHTNESS 10
+enum {
+ LED_TEST_POST_REG_BRIGHTNESS = 10,
+};
-struct led_test_ddata {
+struct led_test_data {
struct led_classdev cdev;
struct device *dev;
};
-static enum led_brightness led_test_brightness_get(struct led_classdev *cdev)
+static enum led_brightness
+led_test_brightness_get(struct led_classdev *cdev)
{
return LED_TEST_POST_REG_BRIGHTNESS;
}
-static void led_test_class_register(struct kunit *test)
+static void led_test_init_cdev(struct led_classdev *cdev)
{
- struct led_test_ddata *ddata = test->priv;
- struct led_classdev *cdev_clash, *cdev = &ddata->cdev;
- struct device *dev = ddata->dev;
- int ret;
+ memset(cdev, 0, sizeof(*cdev));
- /* Register a LED class device */
cdev->name = "led-test";
- cdev->brightness_get = led_test_brightness_get;
cdev->brightness = 0;
+ cdev->brightness_get = led_test_brightness_get;
+}
+
+static void led_test_class_register(struct kunit *test)
+{
+ struct led_test_data *data = test->priv;
+ struct led_classdev *cdev = &data->cdev;
+ struct led_classdev *cdev_clash;
+ struct led_classdev *cdev_reject;
+ struct device *dev = data->dev;
+ int ret;
+
+ led_test_init_cdev(cdev);
ret = devm_led_classdev_register(dev, cdev);
KUNIT_ASSERT_EQ(test, ret, 0);
+ KUNIT_EXPECT_NOT_NULL(test, cdev->dev);
KUNIT_EXPECT_EQ(test, cdev->max_brightness, LED_FULL);
- KUNIT_EXPECT_EQ(test, cdev->brightness, LED_TEST_POST_REG_BRIGHTNESS);
+ KUNIT_EXPECT_EQ(test, cdev->brightness,
+ LED_TEST_POST_REG_BRIGHTNESS);
KUNIT_EXPECT_STREQ(test, cdev->dev->kobj.name, "led-test");
- /* Register again with the same name - expect it to pass with the LED renamed */
+ /*
+ * Name collision should be resolved automatically by
+ * renaming the device instance while preserving the
+ * logical LED name.
+ */
cdev_clash = devm_kmemdup(dev, cdev, sizeof(*cdev), GFP_KERNEL);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, cdev_clash);
ret = devm_led_classdev_register(dev, cdev_clash);
KUNIT_ASSERT_EQ(test, ret, 0);
- KUNIT_EXPECT_STREQ(test, cdev_clash->dev->kobj.name, "led-test_1");
- KUNIT_EXPECT_STREQ(test, cdev_clash->name, "led-test");
+ KUNIT_EXPECT_STREQ(test,
+ cdev_clash->dev->kobj.name,
+ "led-test_1");
+ KUNIT_EXPECT_STREQ(test,
+ cdev_clash->name,
+ "led-test");
- /* Enable name conflict rejection and register with the same name again - expect failure */
- cdev_clash->flags |= LED_REJECT_NAME_CONFLICT;
- ret = devm_led_classdev_register(dev, cdev_clash);
+ /*
+ * Verify that explicit conflict rejection fails.
+ */
+ cdev_reject = devm_kmemdup(dev, cdev, sizeof(*cdev), GFP_KERNEL);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, cdev_reject);
+
+ cdev_reject->flags |= LED_REJECT_NAME_CONFLICT;
+
+ ret = devm_led_classdev_register(dev, cdev_reject);
KUNIT_EXPECT_EQ(test, ret, -EEXIST);
}
static void led_test_class_add_lookup_and_get(struct kunit *test)
{
- struct led_test_ddata *ddata = test->priv;
- struct led_classdev *cdev = &ddata->cdev, *cdev_get;
- struct device *dev = ddata->dev;
- struct led_lookup_data lookup;
+ struct led_test_data *data = test->priv;
+ struct led_classdev *cdev = &data->cdev;
+ struct led_classdev *cdev_get;
+ struct device *dev = data->dev;
+ struct led_lookup_data lookup = { };
int ret;
- /* First, register a LED class device */
- cdev->name = "led-test";
+ led_test_init_cdev(cdev);
+
ret = devm_led_classdev_register(dev, cdev);
KUNIT_ASSERT_EQ(test, ret, 0);
- /* Then make the LED available for lookup */
lookup.provider = cdev->name;
lookup.dev_id = dev_name(dev);
lookup.con_id = "led-test-1";
+
led_add_lookup(&lookup);
- /* Finally, attempt to look it up via the API - imagine this was an orthogonal driver */
cdev_get = devm_led_get(dev, "led-test-1");
- KUNIT_ASSERT_FALSE(test, IS_ERR(cdev_get));
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, cdev_get);
- KUNIT_EXPECT_STREQ(test, cdev_get->name, cdev->name);
+ KUNIT_EXPECT_STREQ(test,
+ cdev_get->name,
+ cdev->name);
led_remove_lookup(&lookup);
}
@@ -93,30 +123,29 @@ static struct kunit_case led_test_cases[] = {
static int led_test_init(struct kunit *test)
{
- struct led_test_ddata *ddata;
+ struct led_test_data *data;
struct device *dev;
- ddata = kunit_kzalloc(test, sizeof(*ddata), GFP_KERNEL);
- if (!ddata)
+ data = kunit_kzalloc(test, sizeof(*data), GFP_KERNEL);
+ if (!data)
return -ENOMEM;
- test->priv = ddata;
-
dev = kunit_device_register(test, "led_test");
if (IS_ERR(dev))
return PTR_ERR(dev);
- ddata->dev = get_device(dev);
+ data->dev = get_device(dev);
+ test->priv = data;
return 0;
}
static void led_test_exit(struct kunit *test)
{
- struct led_test_ddata *ddata = test->priv;
+ struct led_test_data *data = test->priv;
- if (ddata && ddata->dev)
- put_device(ddata->dev);
+ if (data && data->dev)
+ put_device(data->dev);
}
static struct kunit_suite led_test_suite = {
@@ -125,6 +154,7 @@ static struct kunit_suite led_test_suite = {
.exit = led_test_exit,
.test_cases = led_test_cases,
};
+
kunit_test_suite(led_test_suite);
MODULE_AUTHOR("Lee Jones <lee@kernel.org>");
--
2.51.0
^ permalink raw reply related
* [PATCH] leds: lp5860: Return an error for an out-of-range 'reg' property
From: Mert Seftali @ 2026-06-12 20:16 UTC (permalink / raw)
To: Lee Jones, Pavel Machek
Cc: Steffen Trumtrar, Dan Carpenter, linux-leds, linux-kernel,
Mert Seftali, kernel test robot
When fwnode_property_read_u32() succeeds but the channel number exceeds
LP5860_MAX_LED, ret is 0. The error path then passes 0 to dev_err_probe()
and returns 0, so an out-of-range "reg" value is silently treated as
success instead of being rejected.
Set ret to -EINVAL in that case so the invalid channel is reported and
propagated as an error.
Fixes: 3daf2c4ef82b ("leds: Add support for TI LP5860 LED driver chip")
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <error27@gmail.com>
Closes: https://lore.kernel.org/r/202605210624.3gcr3prk-lkp@intel.com/
Signed-off-by: Mert Seftali <mertsftl@gmail.com>
---
drivers/leds/rgb/leds-lp5860-core.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/leds/rgb/leds-lp5860-core.c b/drivers/leds/rgb/leds-lp5860-core.c
index fd0e2f6e6e0f..9eeb01b3e56a 100644
--- a/drivers/leds/rgb/leds-lp5860-core.c
+++ b/drivers/leds/rgb/leds-lp5860-core.c
@@ -115,6 +115,8 @@ static int lp5860_iterate_subleds(struct lp5860_led *led, struct led_init_data *
ret = fwnode_property_read_u32(led_node, "reg", &channel);
if (ret < 0 || channel > LP5860_MAX_LED) {
+ if (ret >= 0)
+ ret = -EINVAL;
dev_err_probe(led->chip->dev, ret,
"%pfwP: 'reg' property is missing. Skipping.\n", led_node);
fwnode_handle_put(led_node);
--
2.54.0
^ permalink raw reply related
* Re: [PATCH] leds: as3668: correct name of config option to match Makefile
From: Lukas Timmermann @ 2026-06-12 20:16 UTC (permalink / raw)
To: Ethan Nelson-Moore, Lee Jones, linux-leds; +Cc: stable, Pavel Machek
In-Reply-To: <20260610224244.128063-1-enelsonmoore@gmail.com>
On Wed, Jun 10, 2026 at 03:42:43PM -0700, Ethan Nelson-Moore wrote:
> The Makefile for the AS3668 LED driver refers to CONFIG_LEDS_AS3668,
> whereas the config file defines CONFIG_LEDS_OSRAM_AMS_AS3668. This
> causes the driver to never be compiled. Correct the name in the Kconfig
> file to match the Makefile. Doing the opposite would also have worked,
> but the name in the Makefile better matches the format of other
> drivers' options.
>
> Fixes: c7dd343a3756 ("leds: as3668: Driver for the ams Osram 4-channel i2c LED driver")
> Cc: stable@vger.kernel.org # 7.0+
> Signed-off-by: Ethan Nelson-Moore <enelsonmoore@gmail.com>
> ---
> drivers/leds/Kconfig | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index f4a0a3c8c870..5ac63cb59469 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -107,7 +107,7 @@ config LEDS_ARIEL
>
> Say Y to if your machine is a Dell Wyse 3020 thin client.
>
> -config LEDS_OSRAM_AMS_AS3668
> +config LEDS_AS3668
> tristate "LED support for Osram AMS AS3668"
> depends on LEDS_CLASS
> depends on I2C
> --
> 2.43.0
>
>
Hi Ethan,
thank you for noticing that issue.
This was already fixed here:
https://lore.kernel.org/all/177815964265.1857555.230971455145782585.b4-ty@b4/
Best regards
Lukas
^ permalink raw reply
* Re: [PATCH] Documentation: leds: fix broken reference to the multicolor LED ABI
From: Randy Dunlap @ 2026-06-12 18:08 UTC (permalink / raw)
To: Shardul Deshpande, Nam Tran, Lee Jones, Pavel Machek,
Jonathan Corbet, Shuah Khan, linux-leds, linux-doc, linux-kernel
In-Reply-To: <20260612171528.728111-1-iamsharduld@gmail.com>
On 6/12/26 10:15 AM, Shardul Deshpande wrote:
> The reference pointed to a non-existent .rst file. The ABI file is named
> sysfs-class-led-multicolor (without extension), so fix the reference to
> match the actual file and resolve the warning from
> tools/docs/documentation-file-ref-check.
>
> Signed-off-by: Shardul Deshpande <iamsharduld@gmail.com>
Acked-by: Randy Dunlap <rdunlap@infradead.org>
Thanks.
> ---
> Documentation/leds/leds-lp5812.rst | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Documentation/leds/leds-lp5812.rst b/Documentation/leds/leds-lp5812.rst
> index c2a6368d5..12e757d45 100644
> --- a/Documentation/leds/leds-lp5812.rst
> +++ b/Documentation/leds/leds-lp5812.rst
> @@ -20,7 +20,7 @@ Sysfs Interface
> ===============
>
> This driver uses the standard multicolor LED class interfaces defined
> -in Documentation/ABI/testing/sysfs-class-led-multicolor.rst.
> +in Documentation/ABI/testing/sysfs-class-led-multicolor.
>
> Each LP5812 LED output appears under ``/sys/class/leds/`` with its
> assigned label (for example ``LED_A``).
--
~Randy
^ permalink raw reply
* [PATCH] Documentation: leds: fix broken reference to the multicolor LED ABI
From: Shardul Deshpande @ 2026-06-12 17:15 UTC (permalink / raw)
To: Nam Tran, Lee Jones, Pavel Machek, Jonathan Corbet, Shuah Khan,
linux-leds, linux-doc, linux-kernel
The reference pointed to a non-existent .rst file. The ABI file is named
sysfs-class-led-multicolor (without extension), so fix the reference to
match the actual file and resolve the warning from
tools/docs/documentation-file-ref-check.
Signed-off-by: Shardul Deshpande <iamsharduld@gmail.com>
---
Documentation/leds/leds-lp5812.rst | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/leds/leds-lp5812.rst b/Documentation/leds/leds-lp5812.rst
index c2a6368d5..12e757d45 100644
--- a/Documentation/leds/leds-lp5812.rst
+++ b/Documentation/leds/leds-lp5812.rst
@@ -20,7 +20,7 @@ Sysfs Interface
===============
This driver uses the standard multicolor LED class interfaces defined
-in Documentation/ABI/testing/sysfs-class-led-multicolor.rst.
+in Documentation/ABI/testing/sysfs-class-led-multicolor.
Each LP5812 LED output appears under ``/sys/class/leds/`` with its
assigned label (for example ``LED_A``).
--
2.43.0
^ permalink raw reply related
* Re: [PATCH v4 01/14] dt-bindings: leds: Document TI LM3533 LED controller
From: Rob Herring (Arm) @ 2026-06-12 16:18 UTC (permalink / raw)
To: Svyatoslav Ryhel
Cc: Jonathan Cameron, dri-devel, linux-kernel, Jingoo Han,
Nuno Sá, Johan Hovold, linux-iio, Andy Shevchenko,
linux-leds, devicetree, Krzysztof Kozlowski, Pavel Machek,
David Lechner, Daniel Thompson, Lee Jones, linux-fbdev,
Helge Deller, Conor Dooley
In-Reply-To: <20260606045738.21050-2-clamor95@gmail.com>
On Sat, 06 Jun 2026 07:57:25 +0300, Svyatoslav Ryhel wrote:
> Document the LM3533 - a complete power source for backlight, keypad and
> indicator LEDs in smartphone handsets. The high-voltage inductive boost
> converter provides the power for two series LED strings display backlight
> and keypad functions.
>
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> Reviewed-by: Jonathan Cameron <jic23@kernel.org> #for light sensor
> Reviewed-by: Daniel Thompson (RISCstar) <danielt@kernel.org> #for backlight
> ---
> .../leds/backlight/ti,lm3533-backlight.yaml | 69 +++++++
> .../bindings/leds/ti,lm3533-leds.yaml | 67 +++++++
> .../devicetree/bindings/leds/ti,lm3533.yaml | 169 ++++++++++++++++++
> 3 files changed, 305 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/leds/backlight/ti,lm3533-backlight.yaml
> create mode 100644 Documentation/devicetree/bindings/leds/ti,lm3533-leds.yaml
> create mode 100644 Documentation/devicetree/bindings/leds/ti,lm3533.yaml
>
Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
^ permalink raw reply
* Re: [PATCH] leds: tests: clarify name conflict test It is a good practice to use a fresh led_classdev instance when doing the testing. LED_REJECT_NAME_CONFLICT rather than re-registering the already registered LED class device. Besides that, do a zero initialization of led_lookup_data.
From: Pavel Machek @ 2026-06-12 10:32 UTC (permalink / raw)
To: Lee Jones; +Cc: Lorenzo Egidio, linux-leds
In-Reply-To: <20260612084519.GE1212816@google.com>
> /* Sashiko Automation: Reviewed (0 Findings) */
If Sashinko can't see problem with this, Sashiko needs to be fixed.
> > > Signed-off-by: Lorenzo Egidio <lorenzoegidioms@gmail.com>
> > > ---
> > > drivers/leds/led-test.c | 102 ++++++++++++++++++++++++++--------------
> > > 1 file changed, 66 insertions(+), 36 deletions(-)
> >
> > NAK.
>
> Definitely, but can you give the contributor something to work on?
Do we put entire changelog in a message subject? No.
BR,
Pavel
^ permalink raw reply
* Re: [PATCH] leds: tests: clarify name conflict test It is a good practice to use a fresh led_classdev instance when doing the testing. LED_REJECT_NAME_CONFLICT rather than re-registering the already registered LED class device. Besides that, do a zero initialization of led_lookup_data.
From: Lee Jones @ 2026-06-12 8:45 UTC (permalink / raw)
To: Pavel Machek; +Cc: Lorenzo Egidio, linux-leds
In-Reply-To: <aintc15RHlD5iU2m@ucw.cz>
/* Sashiko Automation: Reviewed (0 Findings) */
On Thu, 11 Jun 2026, Pavel Machek wrote:
> > Signed-off-by: Lorenzo Egidio <lorenzoegidioms@gmail.com>
> > ---
> > drivers/leds/led-test.c | 102 ++++++++++++++++++++++++++--------------
> > 1 file changed, 66 insertions(+), 36 deletions(-)
>
> NAK.
Definitely, but can you give the contributor something to work on?
--
Lee Jones
^ permalink raw reply
* Re: [PATCH v4 14/14] video: leds: backlight: lm3533: Support getting LED sources from DT
From: Andy Shevchenko @ 2026-06-11 19:27 UTC (permalink / raw)
To: Svyatoslav Ryhel
Cc: Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Helge Deller,
Johan Hovold, dri-devel, linux-leds, devicetree, linux-kernel,
linux-iio, linux-fbdev
In-Reply-To: <CAPVz0n0bpo6PAfdN+LGEgPYhEx8mqtu_SL=WnDxhWn-Cq4rQKA@mail.gmail.com>
On Thu, Jun 11, 2026 at 03:33:42PM +0300, Svyatoslav Ryhel wrote:
> ср, 10 черв. 2026 р. о 21:54 Andy Shevchenko <andriy.shevchenko@intel.com> пише:
> > On Wed, Jun 10, 2026 at 05:45:28PM +0300, Svyatoslav Ryhel wrote:
> > > вт, 9 черв. 2026 р. о 22:23 Andy Shevchenko <andriy.shevchenko@intel.com> пише:
> > > > On Sat, Jun 06, 2026 at 07:57:38AM +0300, Svyatoslav Ryhel wrote:
> > > > > Add Control Bank to HVLED/LVLED muxing support based on the led-sources
> > > > > defined in the device tree.
...
> > > > > + int ret, i;
> > > >
> > > > No need to add 'i'.
> > >
> > > This is personal preference as well. There is no strict rule that
> > > iteration variable must be defined strictly in the for loop.
> >
> > This is a preference by Linus who is the leader of the project.
> > Also in IIO we have some set of maintainer preferences.
>
> Well, this is not meant for IIO,
Oh, my bad. I am overwhelmed with the IIO patches, automatically thought that
I am dealing with yet another IIO patch.
> though it seems that Lee is also in favor if this approach.
Good to know!
> > > > > + for (i = 0; i < led->num_leds; i++) {
> > > >
> > > > for (unsigned int i = 0; i < led->num_leds; i++) {
> > > >
> > > > > + if (led->leds[i] >= LM3533_LVCTRLBANK_MAX)
> > > > > + continue;
> > > > > +
> > > > > + output_cfg_shift = led->leds[i] * 2;
> > > > > + output_cfg_val |= led->id << output_cfg_shift;
> > > > > + output_cfg_mask |= OUTPUT_LVLED_MASK << output_cfg_shift;
> > > > > + }
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH net-next 1/3] leds: trigger: netdev: Extend speeds up to 100G
From: Andrew Lunn @ 2026-06-11 15:19 UTC (permalink / raw)
To: Lee Jones
Cc: mike.marciniszyn, Pavel Machek, Alexander Duyck, Jakub Kicinski,
kernel-team, Andrew Lunn, David S. Miller, Eric Dumazet,
Paolo Abeni, Russell King, Daniel Golle, Kees Cook, Simon Horman,
Dimitri Daskalakis, Jacob Keller, Lee Trager, Mohsin Bashir,
Alok Tiwari, Chengfeng Ye, Andy Shevchenko, linux-kernel,
linux-leds, netdev
In-Reply-To: <20260611142032.GA1127823@google.com>
On Thu, Jun 11, 2026 at 03:20:32PM +0100, Lee Jones wrote:
> On Wed, 20 May 2026, Andrew Lunn wrote:
>
> > On Wed, May 20, 2026 at 04:03:35PM -0400, mike.marciniszyn@gmail.com wrote:
> > > From: "Mike Marciniszyn (Meta)" <mike.marciniszyn@gmail.com>
> > >
> > > Add 25G, 40G, 50G, and 100G as available speeds to the netdev LED trigger.
> > >
> > > Signed-off-by: Mike Marciniszyn (Meta) <mike.marciniszyn@gmail.com>
> >
> > Reviewed-by: Andrew Lunn <andrew@lunn.ch>
>
> Should I wait until the net reviews are satisfied or take this right away?
This one can be merged any time. The speeds are generic, so even if
they are not used now, they can be in future.
Andrew
^ permalink raw reply
* Re: [PATCH net-next 1/3] leds: trigger: netdev: Extend speeds up to 100G
From: Lee Jones @ 2026-06-11 14:20 UTC (permalink / raw)
To: Andrew Lunn
Cc: mike.marciniszyn, Pavel Machek, Alexander Duyck, Jakub Kicinski,
kernel-team, Andrew Lunn, David S. Miller, Eric Dumazet,
Paolo Abeni, Russell King, Daniel Golle, Kees Cook, Simon Horman,
Dimitri Daskalakis, Jacob Keller, Lee Trager, Mohsin Bashir,
Alok Tiwari, Chengfeng Ye, Andy Shevchenko, linux-kernel,
linux-leds, netdev
In-Reply-To: <d2faba3d-68ed-492f-a914-d513e9f8b34f@lunn.ch>
On Wed, 20 May 2026, Andrew Lunn wrote:
> On Wed, May 20, 2026 at 04:03:35PM -0400, mike.marciniszyn@gmail.com wrote:
> > From: "Mike Marciniszyn (Meta)" <mike.marciniszyn@gmail.com>
> >
> > Add 25G, 40G, 50G, and 100G as available speeds to the netdev LED trigger.
> >
> > Signed-off-by: Mike Marciniszyn (Meta) <mike.marciniszyn@gmail.com>
>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Should I wait until the net reviews are satisfied or take this right away?
--
Lee Jones
^ permalink raw reply
* Re: [PATCH v4] leds: group-multicolor: Add support for initial value.
From: Lee Jones @ 2026-06-11 14:16 UTC (permalink / raw)
To: Martijn de Gouw
Cc: Jean-Jacques Hiblot, Pavel Machek, linux-leds, linux-kernel
In-Reply-To: <20260602105608.196291-1-martijn.de.gouw@prodrive-technologies.com>
/* Sashiko Automation: Issues Found (5 Findings) */
Please answer or act upon these Sashiko reviews:
On Tue, 02 Jun 2026, Martijn de Gouw wrote:
> When the driver is loaded, it turned off all LEDs in the group. This
> patch changes the driver to take over existing LED states and set
> the brighness and intensity in the group accordingly.
>
> Signed-off-by: Martijn de Gouw <martijn.de.gouw@prodrive-technologies.com>
> ---
> Changes in v4:
> - Use DIV_ROUND_CLOSEST_ULL to avoid potential overflow when calculating
> the intensity
> - Protect reading of led_cdev->brightness by acquire the lock.
> - Link to v3: https://lore.kernel.org/linux-leds/20260316201321.2789158-1-martijn.de.gouw@prodrive-technologies.com/
>
> Changes in v3:
> - Use is_on boolean instead of storing the max_brightness twice
> - Link to v2: https://lore.kernel.org/linux-leds/20251124210521.2064660-1-martijn.de.gouw@prodrive-technologies.com/
>
> Changes in v2:
> - Fix multiline comments
> - Improve comments
> - Link to v1: https://lore.kernel.org/linux-leds/20251111204556.2803878-1-martijn.de.gouw@prodrive-technologies.com/
> ---
> drivers/leds/rgb/leds-group-multicolor.c | 22 ++++++++++++++++++++--
> 1 file changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/leds/rgb/leds-group-multicolor.c b/drivers/leds/rgb/leds-group-multicolor.c
> index 548c7dd63ba1e..7a77ee6776255 100644
> --- a/drivers/leds/rgb/leds-group-multicolor.c
> +++ b/drivers/leds/rgb/leds-group-multicolor.c
> @@ -69,6 +69,7 @@ static int leds_gmc_probe(struct platform_device *pdev)
> struct mc_subled *subled;
> struct leds_multicolor *priv;
> unsigned int max_brightness = 0;
> + bool is_on = false;
> int i, ret, count = 0, common_flags = 0;
>
> priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> @@ -96,6 +97,15 @@ static int leds_gmc_probe(struct platform_device *pdev)
>
> max_brightness = max(max_brightness, led_cdev->max_brightness);
>
> + /*
> + * If any LED is on, set brightness to the max brightness.
> + * The actual brightness of the LED is set as intensity value.
> + */
> + mutex_lock(&led_cdev->led_access);
> + if (led_cdev->brightness)
> + is_on = true;
> + mutex_unlock(&led_cdev->led_access);
[Severity: High]
Does reading led_cdev->brightness directly risk getting a stale value?
If any of the monochromatic LED drivers implement the brightness_get()
callback, they might change autonomously by hardware or firmware. Should
this code call led_update_brightness() to correctly query the actual
hardware brightness before attempting to preserve state?
[Severity: Medium]
Can userspace alter a sub-LED's brightness concurrently between this loop
and the next one?
Since individual sub-LEDs are fully registered devices whose sysfs
interfaces are not disabled until the end of the probe function, their
brightness could change from > 0 to 0 after this first loop. If that
happens, is_on remains true, but the second loop would compute an
intensity of 0.
> +
> count++;
> }
>
> @@ -109,14 +119,22 @@ static int leds_gmc_probe(struct platform_device *pdev)
>
> subled[i].color_index = led_cdev->color;
>
> - /* Configure the LED intensity to its maximum */
> - subled[i].intensity = max_brightness;
> + /* Configure the LED intensity to its current brightness */
> + if (led_cdev->max_brightness) {
> + mutex_lock(&led_cdev->led_access);
> + subled[i].intensity = DIV_ROUND_CLOSEST_ULL((u64)led_cdev->brightness *
> + max_brightness,
> + led_cdev->max_brightness);
> + mutex_unlock(&led_cdev->led_access);
> + } else
> + subled[i].intensity = 0;
> }
[Severity: High]
Does initializing the intensity based on the current brightness break
standard sysfs control and permanently cap the dynamic range?
If all sub-LEDs are off during probe, intensity becomes 0. Because
leds_gmc_set() calculates physical brightness by multiplying the requested
brightness with this intensity, the resulting hardware brightness will
always be 0 unless userspace manually writes to the multi_intensity file.
Additionally, if LEDs are dimmed during probe (e.g., 50% brightness), that
dimming factor is permanently baked into the intensity array, artificially
capping the maximum possible brightness the user can request.
>
> /* Initialise the multicolor's LED class device */
> cdev = &priv->mc_cdev.led_cdev;
> cdev->brightness_set_blocking = leds_gmc_set;
> cdev->max_brightness = max_brightness;
> + cdev->brightness = is_on ? max_brightness : 0;
> cdev->color = LED_COLOR_ID_MULTI;
> priv->mc_cdev.num_colors = count;
[Severity: High]
This is a pre-existing issue, but the group LED device seems to be exposed
to userspace before its initial hardware state is synchronized.
Further down in leds_gmc_probe(), devm_led_classdev_multicolor_register_ext()
is called before leds_gmc_set(). Once registered, the multi-color LED is
visible in sysfs, and userspace can immediately write to its attributes.
These sysfs writes correctly acquire the group's cdev->led_access mutex.
However, probe continues and calls leds_gmc_set() directly without holding
the mutex:
ret = leds_gmc_set(cdev, cdev->brightness);
Can this result in concurrent executions of leds_gmc_set() corrupting the
hardware state?
[Severity: High]
This is a pre-existing issue, but does the 32-bit integer math in
leds_gmc_set() overflow for hardware with high-resolution max_brightness?
leds_gmc_set() calculates physical brightness using:
mono_brightness = DIV_ROUND_CLOSEST(brightness * intensity * mono_max_brightness,
group_max_brightness * group_max_brightness);
If the underlying monochromatic LED components use a high-resolution
max_brightness value (e.g., 4095 or 65535), their product can easily exceed
UINT_MAX. This would silently overflow the 32-bit integer limit before
division, resulting in wrapped hardware brightness values being sent to the
LED driver.
>
> --
> 2.39.2
>
--
Lee Jones
^ permalink raw reply
* Re: [PATCH v4 14/14] video: leds: backlight: lm3533: Support getting LED sources from DT
From: Svyatoslav Ryhel @ 2026-06-11 12:33 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Helge Deller,
Johan Hovold, dri-devel, linux-leds, devicetree, linux-kernel,
linux-iio, linux-fbdev
In-Reply-To: <aimy3BxBaXQ3Uigd@ashevche-desk.local>
ср, 10 черв. 2026 р. о 21:54 Andy Shevchenko <andriy.shevchenko@intel.com> пише:
>
> On Wed, Jun 10, 2026 at 05:45:28PM +0300, Svyatoslav Ryhel wrote:
> > вт, 9 черв. 2026 р. о 22:23 Andy Shevchenko <andriy.shevchenko@intel.com> пише:
> > > On Sat, Jun 06, 2026 at 07:57:38AM +0300, Svyatoslav Ryhel wrote:
> > > > Add Control Bank to HVLED/LVLED muxing support based on the led-sources
> > > > defined in the device tree.
>
> ...
>
> > > > + int ret, i;
> > >
> > > No need to add 'i'.
> >
> > This is personal preference as well. There is no strict rule that
> > iteration variable must be defined strictly in the for loop.
>
> This is a preference by Linus who is the leader of the project.
> Also in IIO we have some set of maintainer preferences.
>
Well, this is not meant for IIO, though it seems that Lee is also in
favor if this approach.
> > > > + for (i = 0; i < led->num_leds; i++) {
> > >
> > > for (unsigned int i = 0; i < led->num_leds; i++) {
> > >
> > > > + if (led->leds[i] >= LM3533_LVCTRLBANK_MAX)
> > > > + continue;
> > > > +
> > > > + output_cfg_shift = led->leds[i] * 2;
> > > > + output_cfg_val |= led->id << output_cfg_shift;
> > > > + output_cfg_mask |= OUTPUT_LVLED_MASK << output_cfg_shift;
> > > > + }
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
^ permalink raw reply
* Re: [PATCH v4 07/14] mfd: lm3533: Switch sysfs_create_group() to device_add_group()
From: Svyatoslav Ryhel @ 2026-06-11 12:31 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Helge Deller,
Johan Hovold, dri-devel, linux-leds, devicetree, linux-kernel,
linux-iio, linux-fbdev
In-Reply-To: <aimxK3asZjebYrNt@ashevche-desk.local>
ср, 10 черв. 2026 р. о 21:47 Andy Shevchenko <andriy.shevchenko@intel.com> пише:
>
> On Wed, Jun 10, 2026 at 05:38:38PM +0300, Svyatoslav Ryhel wrote:
> > вт, 9 черв. 2026 р. о 22:14 Andy Shevchenko <andriy.shevchenko@intel.com> пише:
> > > On Sat, Jun 06, 2026 at 07:57:31AM +0300, Svyatoslav Ryhel wrote:
> > > > Switch from sysfs_create_group() to device_add_group() including device
> > > > managed where appropriate.
> > >
> > > This should use .dev_groups member of struct device_driver.
> >
> > Specify pls, device_add_group literally uses dev_groups, I don't
> > understand what is wrong.
>
> dev_groups of the struct device_driver. It means that the data should be
> static and be available before driver probe is called.
>
> ...
>
> > > > + ret = devm_device_add_group(&bd->dev, &lm3533_bl_attribute_group);
> > >
> > > This will make Greg KH very grumpy. (For the record, original code as well
> > > but it already is in upstream. So, thanks for trying to address this, just
> > > needs a bit more of work.)
> >
> > In the prev iteration YOU asked to me to adjust this. I have adjusted
> > and now you say that this is not appropriate. I will just drop this
> > commit altogether.
>
> Yes, and I still tell that this is the way to fix that issue.
>
> You can even do it yourself in a few clicks (hint: `git log --grep` is the tool
> of the day): 93afe8ba9b01 ("ACPI: TAD: Use dev_groups in struct device_driver").
> This is an example of what I meant.
>
Oh, very nice, thanks!
> > > > + if (ret < 0)
> > > > + return dev_err_probe(&pdev->dev, ret,
> > > > + "failed to create sysfs attributes\n");
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
^ permalink raw reply
* Re: [PATCH v8 5/7] leds: Add driver for ASUS Transformer LEDs
From: Svyatoslav Ryhel @ 2026-06-11 12:27 UTC (permalink / raw)
To: Lee Jones
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Dmitry Torokhov,
Pavel Machek, Sebastian Reichel, Ion Agorria,
Michał Mirosław, devicetree, linux-kernel, linux-input,
linux-leds, linux-pm
In-Reply-To: <20260611113037.GO4151951@google.com>
чт, 11 черв. 2026 р. о 14:30 Lee Jones <lee@kernel.org> пише:
>
> On Thu, 28 May 2026, Svyatoslav Ryhel wrote:
>
> > From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> >
> > ASUS Transformer tablets have a green and an amber LED on both the Pad
> > and the Dock. If both LEDs are enabled simultaneously, the emitted light
> > will be yellow.
> >
> > Co-developed-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> > ---
> > drivers/leds/Kconfig | 11 +++
> > drivers/leds/Makefile | 1 +
> > drivers/leds/leds-asus-transformer-ec.c | 125 ++++++++++++++++++++++++
> > 3 files changed, 137 insertions(+)
> > create mode 100644 drivers/leds/leds-asus-transformer-ec.c
> >
> > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> > index f4a0a3c8c870..f637d23400a8 100644
> > --- a/drivers/leds/Kconfig
> > +++ b/drivers/leds/Kconfig
> > @@ -120,6 +120,17 @@ config LEDS_OSRAM_AMS_AS3668
> > To compile this driver as a module, choose M here: the module
> > will be called leds-as3668.
> >
> > +config LEDS_ASUS_TRANSFORMER_EC
> > + tristate "LED Support for Asus Transformer charging LED"
> > + depends on LEDS_CLASS
> > + depends on MFD_ASUS_TRANSFORMER_EC
> > + help
> > + This option enables support for charging indicator on
> > + Asus Transformer's Pad and it's Dock.
> > +
> > + To compile this driver as a module, choose M here: the module
> > + will be called leds-asus-transformer-ec.
> > +
> > config LEDS_AW200XX
> > tristate "LED support for Awinic AW20036/AW20054/AW20072/AW20108"
> > depends on LEDS_CLASS
> > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> > index 8fdb45d5b439..d5395c3f1124 100644
> > --- a/drivers/leds/Makefile
> > +++ b/drivers/leds/Makefile
> > @@ -16,6 +16,7 @@ obj-$(CONFIG_LEDS_AN30259A) += leds-an30259a.o
> > obj-$(CONFIG_LEDS_APU) += leds-apu.o
> > obj-$(CONFIG_LEDS_ARIEL) += leds-ariel.o
> > obj-$(CONFIG_LEDS_AS3668) += leds-as3668.o
> > +obj-$(CONFIG_LEDS_ASUS_TRANSFORMER_EC) += leds-asus-transformer-ec.o
> > obj-$(CONFIG_LEDS_AW200XX) += leds-aw200xx.o
> > obj-$(CONFIG_LEDS_AW2013) += leds-aw2013.o
> > obj-$(CONFIG_LEDS_BCM6328) += leds-bcm6328.o
> > diff --git a/drivers/leds/leds-asus-transformer-ec.c b/drivers/leds/leds-asus-transformer-ec.c
> > new file mode 100644
> > index 000000000000..09503e76331c
> > --- /dev/null
> > +++ b/drivers/leds/leds-asus-transformer-ec.c
> > @@ -0,0 +1,125 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +
> > +#include <linux/err.h>
> > +#include <linux/leds.h>
> > +#include <linux/mfd/asus-transformer-ec.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/slab.h>
> > +
> > +enum {
> > + ASUSEC_LED_AMBER,
> > + ASUSEC_LED_GREEN,
> > + ASUSEC_LED_MAX
> > +};
> > +
> > +struct asus_ec_led_config {
> > + const char *name;
> > + unsigned int color;
> > + unsigned long long ctrl_bit;
>
> Should we use 'u64' here instead of 'unsigned long long' to align with standard
> kernel integer types?
>
sure
> > +};
> > +
> > +struct asus_ec_led {
> > + struct asus_ec_leds_data *ddata;
> > + struct led_classdev cdev;
> > + unsigned long long ctrl_bit;
>
> Should we use 'u64' here as well to keep it consistent?
>
sure
> > +};
> > +
> > +struct asus_ec_leds_data {
> > + const struct asusec_core *ec;
> > + struct asus_ec_led leds[ASUSEC_LED_MAX];
> > +};
> > +
> > +static const struct asus_ec_led_config asus_ec_leds[] = {
> > + [ASUSEC_LED_AMBER] = {
> > + .name = "amber",
> > + .color = LED_COLOR_ID_AMBER,
> > + .ctrl_bit = ASUSEC_CTL_LED_AMBER,
> > + },
> > + [ASUSEC_LED_GREEN] = {
> > + .name = "green",
> > + .color = LED_COLOR_ID_GREEN,
> > + .ctrl_bit = ASUSEC_CTL_LED_GREEN,
> > + },
> > +};
> > +
> > +static enum led_brightness asus_ec_led_get_brightness(struct led_classdev *cdev)
> > +{
> > + struct asus_ec_led *led = container_of(cdev, struct asus_ec_led, cdev);
> > + const struct asusec_core *ec = led->ddata->ec;
>
> I'm getting confused here.
>
> ddata is what I'd be calling the device data struct passed by the parent?
>
> In fact, ddata is a little known concept in Leds. Any reason to go for
> this over the standard nomenclature?
>
How then it should be named? It holds each LED's control bit.
> > + u64 ctl;
> > + int ret;
> > +
> > + ret = asus_dockram_access_ctl(ec->dockram, &ctl, 0, 0);
>
> Did we discuss preferring regmap already?
>
Yes, you were fine with all and even said that you will merge
everything with Dmitry Ack for input
HERE https://lore.kernel.org/lkml/20260527144619.GA671544@google.com/
Yet now I get a new pile of complaints and nits.
> > + if (ret)
> > + return LED_OFF;
> > +
> > + return ctl & led->ctrl_bit ? LED_ON : LED_OFF;
> > +}
> > +
> > +static int asus_ec_led_set_brightness(struct led_classdev *cdev,
> > + enum led_brightness brightness)
> > +{
> > + struct asus_ec_led *led = container_of(cdev, struct asus_ec_led, cdev);
> > + const struct asusec_core *ec = led->ddata->ec;
> > +
> > + if (brightness)
> > + return asus_dockram_access_ctl(ec->dockram, NULL,
> > + led->ctrl_bit, led->ctrl_bit);
> > +
> > + return asus_dockram_access_ctl(ec->dockram, NULL, led->ctrl_bit, 0);
> > +}
> > +
> > +static int asus_ec_led_probe(struct platform_device *pdev)
> > +{
> > + const struct asusec_core *ec = dev_get_drvdata(pdev->dev.parent);
> > + struct asus_ec_leds_data *ddata;
> > + struct device *dev = &pdev->dev;
> > + int i, ret;
>
> Could we declare the loop counter 'i' directly within the 'for' statement's
> scope to keep its scope limited? For example, 'for (int i = 0; ...)'.
>
> > +
> > + ddata = devm_kzalloc(dev, sizeof(*ddata), GFP_KERNEL);
> > + if (!ddata)
> > + return -ENOMEM;
> > +
> > + platform_set_drvdata(pdev, ddata);
> > + ddata->ec = ec;
> > +
> > + for (i = 0; i < ASUSEC_LED_MAX; i++) {
>
> Nit: for (int i = ...
>
> > + const struct asus_ec_led_config *cfg = &asus_ec_leds[i];
> > + struct asus_ec_led *led = &ddata->leds[i];
> > +
> > + led->cdev.name = devm_kasprintf(dev, GFP_KERNEL, "%s::%s",
> > + ddata->ec->name, cfg->name);
> > + if (!led->cdev.name)
> > + return -ENOMEM;
> > +
> > + led->cdev.max_brightness = 1;
> > + led->cdev.color = cfg->color;
> > + led->cdev.flags = LED_CORE_SUSPENDRESUME | LED_RETAIN_AT_SHUTDOWN;
> > + led->cdev.brightness_get = asus_ec_led_get_brightness;
> > + led->cdev.brightness_set_blocking = asus_ec_led_set_brightness;
> > +
> > + led->ddata = ddata;
> > + led->ctrl_bit = cfg->ctrl_bit;
> > +
> > + ret = devm_led_classdev_register(dev, &led->cdev);
> > + if (ret)
> > + return dev_err_probe(dev, ret,
> > + "failed to register %s LED\n",
> > + cfg->name);
>
> Should we capitalise the error message here to match our style guidelines
> (e.g. 'Failed to register...')?
>
dev messages end with ":" so it should continue with lower case. You
want cap, I don't mind
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static struct platform_driver asus_ec_led_driver = {
> > + .driver.name = "asus-transformer-ec-led",
> > + .probe = asus_ec_led_probe,
> > +};
> > +module_platform_driver(asus_ec_led_driver);
> > +
> > +MODULE_ALIAS("platform:asus-transformer-ec-led");
> > +MODULE_AUTHOR("Michał Mirosław <mirq-linux@rere.qmqm.pl>");
> > +MODULE_AUTHOR("Svyatoslav Ryhel <clamor95@gmail.com>");
> > +MODULE_DESCRIPTION("ASUS Transformer's charging LED driver");
> > +MODULE_LICENSE("GPL");
> > --
> > 2.51.0
> >
> >
>
> --
> Lee Jones
^ permalink raw reply
* Re: [PATCH v8 2/7] mfd: Add driver for ASUS Transformer embedded controller
From: Svyatoslav Ryhel @ 2026-06-11 12:16 UTC (permalink / raw)
To: Lee Jones
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Dmitry Torokhov,
Pavel Machek, Sebastian Reichel, Ion Agorria,
Michał Mirosław, devicetree, linux-kernel, linux-input,
linux-leds, linux-pm
In-Reply-To: <20260611111732.GN4151951@google.com>
чт, 11 черв. 2026 р. о 14:17 Lee Jones <lee@kernel.org> пише:
>
> On Thu, 28 May 2026, Svyatoslav Ryhel wrote:
> > From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> >
> > Support Nuvoton NPCE795-based ECs as used in Asus Transformer TF201,
> > TF300T, TF300TG, TF300TL and TF700T pad and dock, as well as TF101 dock
> > and TF600T, P1801-T and TF701T pad. This is a glue driver handling
> > detection and common operations for EC's functions.
> >
> > Co-developed-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> > ---
> > drivers/mfd/Kconfig | 16 +
> > drivers/mfd/Makefile | 1 +
> > drivers/mfd/asus-transformer-ec.c | 542 ++++++++++++++++++++++++
> > include/linux/mfd/asus-transformer-ec.h | 92 ++++
> > 4 files changed, 651 insertions(+)
> > create mode 100644 drivers/mfd/asus-transformer-ec.c
> > create mode 100644 include/linux/mfd/asus-transformer-ec.h
> >
...
> > +
> > + memset(buf, 0, ASUSEC_ENTRY_BUFSIZE);
> > + ret = i2c_smbus_read_i2c_block_data(client, ASUSEC_DOCKRAM_CONTROL,
> > + ASUSEC_ENTRY_SIZE, buf);
> > + if (ret < ASUSEC_ENTRY_SIZE) {
> > + dev_err(&client->dev, "failed to access control buffer: %d\n",
> > + ret);
> > + return ret;
>
> Should we return a negative error code here if the read is shorter than
> expected, rather than propagating the positive byte count?
>
Yes, I have adjusted it already locally for the next iteration. It
will return ret if negative and -EIO if ret is pos but less then
ASUSEC_ENTRY_SIZE (return ret < 0 ? ret : -EIO)
> > + }
> > +
> > + if (buf[0] != ASUSEC_CTL_SIZE) {
> > + dev_err(&client->dev, "buffer size exceeds %d: %d\n",
> > + ASUSEC_CTL_SIZE, buf[0]);
> > + return -EPROTO;
> > + }
> > +
> > + val = get_unaligned_le64(buf + 1);
> > +
> > + if (out)
> > + *out = val;
> > +
> > + if (mask || xor) {
> > + put_unaligned_le64((val & ~mask) ^ xor, buf + 1);
> > + ret = i2c_smbus_write_i2c_block_data(client,
> > + ASUSEC_DOCKRAM_CONTROL,
> > + ASUSEC_ENTRY_SIZE, buf);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(asus_dockram_access_ctl);
> > +
> > +static int asus_ec_signal_request(struct asus_ec_data *ddata)
> > +{
> > + guard(mutex)(&ddata->ecreq_lock);
> > +
> > + gpiod_set_value_cansleep(ddata->ecreq_gpio, 1);
> > + msleep(50);
> > +
> > + gpiod_set_value_cansleep(ddata->ecreq_gpio, 0);
> > + msleep(200);
>
> Do these numbers come from the datasheet or were they arbitrarily chosen?
>
There is no datasheet. These delays come from downstream driver and
with lower values or removed delays request fails.
> > +
> > + return 0;
> > +}
> > +
> > +static void asus_ec_clear_buffer(struct asus_ec_data *ddata)
> > +{
> > + int ret, retry = ASUSEC_RSP_BUFFER_SIZE;
> > +
> > + /*
> > + * Read the buffer till we get valid data by checking ASUSEC_OBF_MASK
> > + * of the status byte or till we reach end of the 256 byte buffer.
> > + */
> > + while (retry--) {
> > + ret = i2c_smbus_read_i2c_block_data(ddata->client, ASUSEC_READ_BUF,
> > + ASUSEC_ENTRY_SIZE,
> > + ddata->ec_buf);
> > + if (ret < ASUSEC_ENTRY_SIZE)
> > + continue;
> > +
> > + if (ddata->ec_buf[ASUSEC_IRQ_STATUS] & ASUSEC_OBF_MASK)
> > + continue;
> > +
> > + break;
> > + }
> > +}
> > +
> > +static int asus_ec_log_info(struct asus_ec_data *ddata, unsigned int reg,
> > + const char *name, const char **out)
> > +{
> > + struct device *dev = &ddata->client->dev;
> > + u8 buf[ASUSEC_ENTRY_BUFSIZE];
> > + int ret;
> > +
> > + memset(buf, 0, ASUSEC_ENTRY_BUFSIZE);
> > + ret = i2c_smbus_read_i2c_block_data(ddata->ec.dockram, reg,
> > + ASUSEC_ENTRY_SIZE, buf);
> > + if (ret < ASUSEC_ENTRY_SIZE)
> > + return ret;
>
> Same here. These should be negative.
>
return ret < 0 ? ret : -EIO same as above
> > +
> > + if (buf[0] > ASUSEC_ENTRY_SIZE) {
> > + dev_err(dev, "bad data len; buffer: %*ph; ret: %d\n",
> > + ASUSEC_ENTRY_BUFSIZE, buf, ret);
> > + return -EPROTO;
> > + }
> > +
> > + if (!ddata->logging_disabled) {
> > + dev_info(dev, "%-14s: %.*s\n", name, buf[0], buf + 1);
> > +
> > + if (out) {
> > + *out = devm_kasprintf(dev, GFP_KERNEL, "%.*s",
> > + buf[0], buf + 1);
> > + if (!*out)
> > + return -ENOMEM;
> > + }
> > + }
>
> FWIW, I hate this! What does it give you now that development is done?
>
We have already discussed this, and you agreed that EC and firmware
prints may stay! This prints EC model and firmware info as well as EC
firmware behavior. It allows identify possible new revisions of EC -
Firmware combo and address possible regressions (check if it is chip
malfunction or firmware needs a new programming model) without
rebuilding kernel and digging downstream kernel for needed bits of
code.
> > + return 0;
> > +}
> > +
> > +static int asus_ec_reset(struct asus_ec_data *ddata)
> > +{
> > + int retry, ret;
> > +
> > + guard(mutex)(&ddata->ecreq_lock);
> > +
> > + for (retry = 0; retry < ASUSEC_RETRY_MAX; retry++) {
>
> for (int return = ... is generally preferred for throwaway variables.
>
Not that I care too much, but I am defining ret anyway, why not add
retry too there?
>
> > + ret = i2c_smbus_write_word_data(ddata->client, ASUSEC_WRITE_BUF,
> > + ASUSEC_RESET);
> > + if (!ret)
> > + return 0;
> > +
> > + msleep(ASUSEC_ACCESS_TIMEOUT);
>
> I like that this is defined, can we do that with the others please?
>
I don't see any benefits of defining delays, they are all arbitrary
and derive from downstream kernel, removing or altering them caused
malfunction.
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static int asus_ec_susb_on_status(struct asus_ec_data *ddata)
> > +{
> > + u64 flag;
> > + int ret;
> > +
> > + ret = asus_dockram_access_ctl(ddata->ec.dockram, &flag, 0, 0);
> > + if (ret)
> > + return ret;
> > +
> > + flag &= ASUSEC_CTL_SUSB_MODE;
> > + dev_info(&ddata->client->dev, "EC FW behaviour: %s\n",
> > + flag ? "susb on when receive ec_req" :
> > + "susb on when system wakeup");
> > +
> > + return 0;
> > +}
> > +
> > +static int asus_ec_set_factory_mode(struct asus_ec_data *ddata,
> > + enum asusec_mode fmode)
> > +{
> > + dev_info(&ddata->client->dev, "Entering %s mode.\n",
> > + fmode == ASUSEC_MODE_FACTORY ? "factory" : "normal");
> > +
> > + return asus_dockram_access_ctl(ddata->ec.dockram, NULL,
> > + ASUSEC_CTL_FACTORY_MODE,
> > + fmode == ASUSEC_MODE_FACTORY ?
> > + ASUSEC_CTL_FACTORY_MODE : 0);
>
> Why not create make:
>
> ASUSEC_MODE_FACTORY == ASUSEC_CTL_FACTORY_MODE
>
> What happens to NORMAL?
>
ASUSEC_CTL_FACTORY_MODE is a bit in the ctl register. For NORMAL mode
bit is cleared,
for FACTORY bit it set, for NONE bit is ignored.
> > +}
> > +
> > +static int asus_ec_detect(struct asus_ec_data *ddata)
> > +{
> > + int ret;
> > +
> > + ret = asus_ec_reset(ddata);
> > + if (ret)
> > + goto err_exit;
> > +
> > + asus_ec_clear_buffer(ddata);
> > +
> > + ret = asus_ec_log_info(ddata, ASUSEC_DOCKRAM_INFO_MODEL, "Model",
> > + &ddata->ec.model);
>
> You can use 100-chars and make the code look beautiful! :)
>
Not every subsystem permits 100 chars, some stick to 80 as a strict
rule, so it is better be safe.
> > + if (ret)
> > + goto err_exit;
> > +
> > + ret = asus_ec_log_info(ddata, ASUSEC_DOCKRAM_INFO_FW, "FW version",
> > + NULL);
> > + if (ret)
> > + goto err_exit;
> > +
> > + ret = asus_ec_log_info(ddata, ASUSEC_DOCKRAM_INFO_CFGFMT, "Config format",
> > + NULL);
> > + if (ret)
> > + goto err_exit;
> > +
> > + ret = asus_ec_log_info(ddata, ASUSEC_DOCKRAM_INFO_HW, "HW version",
> > + NULL);
> > + if (ret)
> > + goto err_exit;
> > +
> > + /* Disable logging on next EC request */
>
> Why, but why?
>
Cause EC requests are frequent (handshake/reset) and constant logging
same data is not acceptable.
> > + ddata->logging_disabled = true;
> > +
> > + /* Check and inform about EC firmware behavior */
> > + ret = asus_ec_susb_on_status(ddata);
> > + if (ret)
> > + goto err_exit;
> > +
> > + ddata->ec.name = ddata->info->name;
> > +
> > + /* Some EC require factory mode to be set normal on each request */
> > + if (ddata->info->fmode)
> > + ret = asus_ec_set_factory_mode(ddata, ddata->info->fmode);
> > +
> > +err_exit:
> > + if (ret)
> > + dev_err(&ddata->client->dev, "failed to access EC: %d\n", ret);
> > +
> > + return ret;
> > +}
> > +
> > +static void asus_ec_handle_smi(struct asus_ec_data *ddata, unsigned int code)
> > +{
> > + switch (code) {
> > + case ASUSEC_SMI_HANDSHAKE:
> > + case ASUSEC_SMI_RESET:
> > + asus_ec_detect(ddata);
> > + break;
> > + }
> > +}
> > +
> > +static irqreturn_t asus_ec_interrupt(int irq, void *dev_id)
> > +{
> > + struct asus_ec_data *ddata = dev_id;
> > + unsigned long notify_action;
> > + int ret;
> > +
> > + ret = i2c_smbus_read_i2c_block_data(ddata->client, ASUSEC_READ_BUF,
> > + ASUSEC_ENTRY_SIZE, ddata->ec_buf);
> > + if (ret < ASUSEC_ENTRY_SIZE ||
> > + !(ddata->ec_buf[ASUSEC_IRQ_STATUS] & ASUSEC_OBF_MASK))
>
> Unwrap for readability.
>
> Also, I think a comment would be helpful.
>
if (ret < ASUSEC_ENTRY_SIZE)
return IRQ_NONE;
ret = ddata->ec_buf[ASUSEC_IRQ_STATUS] & ASUSEC_OBF_MASK;
if (!ret)
return IRQ_NONE;
This would be acceptable? (I will add comments later on)
> > + return IRQ_NONE;
> > +
> > + notify_action = ddata->ec_buf[ASUSEC_IRQ_STATUS];
> > + if (notify_action & ASUSEC_SMI_MASK) {
> > + unsigned int code = ddata->ec_buf[ASUSEC_SMI_CODE];
> > +
> > + asus_ec_handle_smi(ddata, code);
> > +
> > + notify_action |= code << 8;
> > + }
> > +
> > + blocking_notifier_call_chain(&ddata->ec.notify_list,
> > + notify_action, ddata->ec_buf);
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static void asus_ec_release_dockram_dev(void *client)
> > +{
> > + i2c_unregister_device(client);
> > +}
> > +
> > +static struct i2c_client *devm_asus_dockram_get(struct device *dev)
> > +{
> > + struct i2c_client *parent = to_i2c_client(dev);
> > + struct i2c_client *dockram;
> > + struct dockram_ec_data *ddata;
> > + int ret;
> > +
> > + dockram = i2c_new_ancillary_device(parent, "dockram",
> > + parent->addr + 2);
>
> Could we define a macro for the address offset '2' here to avoid using a magic
> number?
>
It seems that you are excessively concerned with "magic numbers".
> > + if (IS_ERR(dockram))
> > + return dockram;
> > +
> > + ret = devm_add_action_or_reset(dev, asus_ec_release_dockram_dev,
> > + dockram);
> > + if (ret)
> > + return ERR_PTR(ret);
> > +
> > + ddata = devm_kzalloc(&dockram->dev, sizeof(*ddata), GFP_KERNEL);
> > + if (!ddata)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + i2c_set_clientdata(dockram, ddata);
> > + mutex_init(&ddata->ctl_lock);
> > +
> > + return dockram;
> > +}
> > +
> > +static const struct mfd_cell asus_ec_sl101_dock_mfd_devices[] = {
> > + MFD_CELL_NAME("asus-transformer-ec-kbc"),
> > +};
> > +
> > +static const struct mfd_cell asus_ec_tf101_dock_mfd_devices[] = {
> > + MFD_CELL_BASIC("asus-transformer-ec-battery", NULL, NULL, 0, 1),
> > + MFD_CELL_BASIC("asus-transformer-ec-charger", NULL, NULL, 0, 1),
> > + MFD_CELL_BASIC("asus-transformer-ec-led", NULL, NULL, 0, 1),
> > + MFD_CELL_NAME("asus-transformer-ec-keys"),
> > + MFD_CELL_NAME("asus-transformer-ec-kbc"),
> > +};
> > +
> > +static const struct mfd_cell asus_ec_tf201_pad_mfd_devices[] = {
> > + MFD_CELL_NAME("asus-transformer-ec-battery"),
> > + MFD_CELL_NAME("asus-transformer-ec-led"),
> > +};
> > +
> > +static const struct mfd_cell asus_ec_tf600t_pad_mfd_devices[] = {
> > + MFD_CELL_NAME("asus-transformer-ec-battery"),
> > + MFD_CELL_NAME("asus-transformer-ec-charger"),
> > + MFD_CELL_NAME("asus-transformer-ec-led"),
> > +};
> > +
> > +static int asus_ec_probe(struct i2c_client *client)
> > +{
> > + struct device *dev = &client->dev;
> > + struct asus_ec_data *ddata;
> > + const struct mfd_cell *cells;
> > + unsigned int num_cells;
> > + unsigned long irqflags;
> > + int ret;
> > +
> > + if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_I2C_BLOCK))
> > + return dev_err_probe(dev, -ENXIO,
> > + "I2C bus is missing required SMBus block mode support\n");
> > +
> > + ddata = devm_kzalloc(dev, sizeof(*ddata), GFP_KERNEL);
> > + if (!ddata)
> > + return -ENOMEM;
> > +
> > + ddata->info = device_get_match_data(dev);
> > + if (!ddata->info)
> > + return -ENODEV;
> > +
> > + switch (ddata->info->variant) {
> > + case ASUSEC_SL101_DOCK:
> > + cells = asus_ec_sl101_dock_mfd_devices;
> > + num_cells = ARRAY_SIZE(asus_ec_sl101_dock_mfd_devices);
> > + break;
> > + case ASUSEC_TF101_DOCK:
> > + cells = asus_ec_tf101_dock_mfd_devices;
> > + num_cells = ARRAY_SIZE(asus_ec_tf101_dock_mfd_devices);
> > + break;
> > + case ASUSEC_TF201_PAD:
> > + cells = asus_ec_tf201_pad_mfd_devices;
> > + num_cells = ARRAY_SIZE(asus_ec_tf201_pad_mfd_devices);
> > + break;
> > + case ASUSEC_TF600T_PAD:
> > + cells = asus_ec_tf600t_pad_mfd_devices;
> > + num_cells = ARRAY_SIZE(asus_ec_tf600t_pad_mfd_devices);
> > + break;
> > + default:
> > + return dev_err_probe(dev, -EINVAL,
> > + "unknown device variant %d\n",
> > + ddata->info->variant);
> > + }
> > +
> > + i2c_set_clientdata(client, ddata);
> > + ddata->client = client;
> > +
> > + ddata->ec.dockram = devm_asus_dockram_get(dev);
> > + if (IS_ERR(ddata->ec.dockram))
> > + return dev_err_probe(dev, PTR_ERR(ddata->ec.dockram),
> > + "failed to get dockram\n");
> > +
> > + ddata->ecreq_gpio = devm_gpiod_get(dev, "request", GPIOD_OUT_LOW);
> > + if (IS_ERR(ddata->ecreq_gpio))
> > + return dev_err_probe(dev, PTR_ERR(ddata->ecreq_gpio),
> > + "failed to get EC request GPIO\n");
> > +
> > + BLOCKING_INIT_NOTIFIER_HEAD(&ddata->ec.notify_list);
> > + mutex_init(&ddata->ecreq_lock);
> > +
> > + asus_ec_signal_request(ddata);
> > +
> > + ret = asus_ec_detect(ddata);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "failed to detect EC version\n");
> > +
> > + /*
> > + * Systems using device tree should set up interrupt via DTS,
> > + * the rest will use the default low interrupt.
> > + */
> > + irqflags = dev->of_node ? 0 : IRQF_TRIGGER_LOW;
> > +
> > + ret = devm_request_threaded_irq(dev, client->irq, NULL,
> > + &asus_ec_interrupt,
> > + IRQF_ONESHOT | irqflags,
> > + client->name, ddata);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "failed to register IRQ\n");
> > +
> > + /* Parent I2C controller uses DMA, ASUS EC and child devices do not */
> > + client->dev.coherent_dma_mask = 0;
> > + client->dev.dma_mask = &client->dev.coherent_dma_mask;
> > +
> > + return devm_mfd_add_devices(dev, 0, cells, num_cells, NULL, 0, NULL);
> > +}
> > +
> > +static const struct asus_ec_chip_info asus_ec_sl101_dock_data = {
> > + .name = "dock",
> > + .variant = ASUSEC_SL101_DOCK,
> > + .fmode = ASUSEC_MODE_NONE,
> > +};
> > +
> > +static const struct asus_ec_chip_info asus_ec_tf101_dock_data = {
> > + .name = "dock",
> > + .variant = ASUSEC_TF101_DOCK,
> > + .fmode = ASUSEC_MODE_NONE,
> > +};
> > +
> > +static const struct asus_ec_chip_info asus_ec_tf201_pad_data = {
> > + .name = "pad",
> > + .variant = ASUSEC_TF201_PAD,
> > + .fmode = ASUSEC_MODE_NORMAL,
> > +};
> > +
> > +static const struct asus_ec_chip_info asus_ec_tf600t_pad_data = {
> > + .name = "pad",
> > + .variant = ASUSEC_TF600T_PAD,
> > + .fmode = ASUSEC_MODE_NORMAL,
> > +};
>
> Any reason not to just pass the identifier (variant) and add the name
> and fmode attribues to the switch() above?
Why not set it here, I am not passing any mfd or any other API via of data.
> > +
> > +static const struct of_device_id asus_ec_match[] = {
> > + {
> > + .compatible = "asus,sl101-ec-dock",
> > + .data = &asus_ec_sl101_dock_data
> > + }, {
> > + .compatible = "asus,tf101-ec-dock",
> > + .data = &asus_ec_tf101_dock_data
> > + }, {
> > + .compatible = "asus,tf201-ec-pad",
> > + .data = &asus_ec_tf201_pad_data
> > + }, {
> > + .compatible = "asus,tf600t-ec-pad",
> > + .data = &asus_ec_tf600t_pad_data
> > + },
> > + { /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, asus_ec_match);
> > +
> > +static struct i2c_driver asus_ec_driver = {
> > + .driver = {
> > + .name = "asus-transformer-ec",
> > + .of_match_table = asus_ec_match,
> > + },
> > + .probe = asus_ec_probe,
> > +};
> > +module_i2c_driver(asus_ec_driver);
> > +
> > +MODULE_AUTHOR("Michał Mirosław <mirq-linux@rere.qmqm.pl>");
> > +MODULE_AUTHOR("Svyatoslav Ryhel <clamor95@gmail.com>");
> > +MODULE_DESCRIPTION("ASUS Transformer's EC driver");
> > +MODULE_LICENSE("GPL");
> > diff --git a/include/linux/mfd/asus-transformer-ec.h b/include/linux/mfd/asus-transformer-ec.h
> > new file mode 100644
> > index 000000000000..f085eea2193e
> > --- /dev/null
> > +++ b/include/linux/mfd/asus-transformer-ec.h
> > @@ -0,0 +1,92 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +#ifndef __MFD_ASUS_TRANSFORMER_EC_H
> > +#define __MFD_ASUS_TRANSFORMER_EC_H
> > +
> > +#include <linux/notifier.h>
> > +#include <linux/platform_device.h>
> > +
> > +struct i2c_client;
> > +
> > +/**
> > + * struct asusec_core - public part shared with all cells
> > + *
> > + * @model: firmware version running on the EC
> > + * @name: prefix associated with the EC
> > + * @dockram: pointer to Dockram's i2c_client
> > + * @notify_list: notify list used by cells
> > + */
> > +struct asusec_core {
> > + const char *model;
> > + const char *name;
> > + struct i2c_client *dockram;
> > + struct blocking_notifier_head notify_list;
> > +};
> > +
> > +#define ASUSEC_ENTRIES 0x100
> > +#define ASUSEC_ENTRY_SIZE 32
> > +#define ASUSEC_ENTRY_BUFSIZE (ASUSEC_ENTRY_SIZE + 1)
> > +
> > +/* interrupt sources */
> > +#define ASUSEC_IRQ_STATUS 1
> > +#define ASUSEC_OBF_MASK BIT(0)
> > +#define ASUSEC_KEY_MASK BIT(2)
> > +#define ASUSEC_KBC_MASK BIT(3)
> > +#define ASUSEC_AUX_MASK BIT(5)
> > +#define ASUSEC_SCI_MASK BIT(6)
> > +#define ASUSEC_SMI_MASK BIT(7)
> > +
> > +/* SMI notification codes */
> > +#define ASUSEC_SMI_CODE 2
> > +#define ASUSEC_SMI_POWER_NOTIFY 0x31 /* USB cable plug event */
> > +#define ASUSEC_SMI_HANDSHAKE 0x50 /* response to ec_req edge */
> > +#define ASUSEC_SMI_WAKE 0x53
> > +#define ASUSEC_SMI_RESET 0x5f
> > +#define ASUSEC_SMI_ADAPTER_EVENT 0x60 /* charger to dock plug event */
> > +#define ASUSEC_SMI_BACKLIGHT_ON 0x63
> > +#define ASUSEC_SMI_AUDIO_DOCK_IN 0x70
> > +
> > +#define ASUSEC_SMI_ACTION(code) (ASUSEC_SMI_MASK | ASUSEC_OBF_MASK | \
> > + (ASUSEC_SMI_##code << 8))
> > +
> > +/* control register [0x0a] layout */
> > +#define ASUSEC_CTL_SIZE 8
> > +
> > +/*
> > + * EC reports power from 40-pin connector in the LSB of the control
> > + * register. The following values have been observed (xor 0x02):
> > + *
> > + * PAD-ec no-plug 0x40 / PAD-ec DOCK 0x20 / DOCK-ec no-plug 0x40
> > + * PAD-ec AC 0x25 / PAD-ec DOCK+AC 0x24 / DOCK-ec AC 0x25
> > + * PAD-ec USB 0x45 / PAD-ec DOCK+USB 0x24 / DOCK-ec USB 0x41
> > + */
> > +
> > +#define ASUSEC_CTL_DIRECT_POWER_SOURCE BIT_ULL(0)
> > +#define ASUSEC_STAT_CHARGING BIT_ULL(2)
> > +#define ASUSEC_CTL_FULL_POWER_SOURCE BIT_ULL(5)
> > +#define ASUSEC_CTL_SUSB_MODE BIT_ULL(9)
> > +#define ASUSEC_CMD_SUSPEND_S3 BIT_ULL(33)
> > +#define ASUSEC_CTL_TEST_DISCHARGE BIT_ULL(35)
> > +#define ASUSEC_CMD_SUSPEND_INHIBIT BIT_ULL(37)
> > +#define ASUSEC_CTL_FACTORY_MODE BIT_ULL(38)
> > +#define ASUSEC_CTL_KEEP_AWAKE BIT_ULL(39)
> > +#define ASUSEC_CTL_USB_CHARGE BIT_ULL(40)
> > +#define ASUSEC_CTL_LED_BLINK BIT_ULL(40)
> > +#define ASUSEC_CTL_LED_AMBER BIT_ULL(41)
> > +#define ASUSEC_CTL_LED_GREEN BIT_ULL(42)
> > +#define ASUSEC_CMD_SWITCH_HDMI BIT_ULL(56)
> > +#define ASUSEC_CMD_WIN_SHUTDOWN BIT_ULL(62)
> > +
> > +#define ASUSEC_DOCKRAM_INFO_MODEL 0x01
> > +#define ASUSEC_DOCKRAM_INFO_FW 0x02
> > +#define ASUSEC_DOCKRAM_INFO_CFGFMT 0x03
> > +#define ASUSEC_DOCKRAM_INFO_HW 0x04
> > +#define ASUSEC_DOCKRAM_CONTROL 0x0a
> > +#define ASUSEC_DOCKRAM_BATT_CTL 0x14
> > +
> > +#define ASUSEC_WRITE_BUF 0x64
> > +#define ASUSEC_READ_BUF 0x6a
> > +
> > +int asus_dockram_access_ctl(struct i2c_client *client,
> > + u64 *out, u64 mask, u64 xor);
> > +
> > +#endif /* __MFD_ASUS_TRANSFORMER_EC_H */
> > --
> > 2.51.0
> >
>
> --
> Lee Jones
^ permalink raw reply
* Re: [PATCH v9 2/2] leds: ltc3220: Add Support for LTC3220 18 channel LED Driver
From: Lee Jones @ 2026-06-11 11:47 UTC (permalink / raw)
To: Edelweise Escala
Cc: Pavel Machek, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
linux-leds, devicetree, linux-kernel
In-Reply-To: <20260528-ltc3220-driver-v9-2-69450fc213cb@analog.com>
/* Sashiko Automation: Issues Found (10 Findings) */
Would you mind taking care of these before I conduct my next review please?
On Thu, 28 May 2026, Edelweise Escala wrote:
> Add driver for the LTC3220 18-channel LED driver
> with I2C interface, individual brightness control, and hardware-assisted
> blink/gradation features.
>
> Signed-off-by: Edelweise Escala <edelweise.escala@analog.com>
> ---
> MAINTAINERS | 1 +
> drivers/leds/Kconfig | 13 ++
> drivers/leds/Makefile | 1 +
> drivers/leds/leds-ltc3220.c | 440 ++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 455 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index c8a242577d2f..0f553ada61d9 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -15229,6 +15229,7 @@ L: linux-leds@vger.kernel.org
> S: Maintained
> W: https://ez.analog.com/linux-software-drivers
> F: Documentation/devicetree/bindings/leds/adi,ltc3220.yaml
> +F: drivers/leds/leds-ltc3220.c
>
> LTC4282 HARDWARE MONITOR DRIVER
> M: Nuno Sa <nuno.sa@analog.com>
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index f4a0a3c8c870..31b1e3ff094c 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -1000,6 +1000,19 @@ config LEDS_ST1202
> Say Y to enable support for LEDs connected to LED1202
> LED driver chips accessed via the I2C bus.
>
> +config LEDS_LTC3220
> + tristate "LED Driver for Analog Devices Inc. LTC3220"
> + depends on I2C && LEDS_CLASS
> + select REGMAP_I2C
> + help
> + Say Y to enable support for the Analog Devices LTC3220
> + 18-channel LED controller with I2C interface.
> + The driver supports individual LED brightness control (64 steps),
> + hardware-assisted blinking and gradation effects.
> +
> + To compile this driver as a module, choose M here: the module will
> + be called leds-ltc3220.
> +
> config LEDS_TPS6105X
> tristate "LED support for TI TPS6105X"
> depends on LEDS_CLASS
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index 7db3768912ca..a68244bd50fb 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -61,6 +61,7 @@ obj-$(CONFIG_LEDS_LP8788) += leds-lp8788.o
> obj-$(CONFIG_LEDS_LP8860) += leds-lp8860.o
> obj-$(CONFIG_LEDS_LP8864) += leds-lp8864.o
> obj-$(CONFIG_LEDS_LT3593) += leds-lt3593.o
> +obj-$(CONFIG_LEDS_LTC3220) += leds-ltc3220.o
> obj-$(CONFIG_LEDS_MAX5970) += leds-max5970.o
> obj-$(CONFIG_LEDS_MAX77650) += leds-max77650.o
> obj-$(CONFIG_LEDS_MAX77705) += leds-max77705.o
> diff --git a/drivers/leds/leds-ltc3220.c b/drivers/leds/leds-ltc3220.c
> new file mode 100644
> index 000000000000..a633ac5ca6ae
> --- /dev/null
> +++ b/drivers/leds/leds-ltc3220.c
> @@ -0,0 +1,440 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * LTC3220 18-Channel LED Driver
> + *
> + * Copyright 2026 Analog Devices Inc.
> + *
> + * Author: Edelweise Escala <edelweise.escala@analog.com>
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/delay.h>
[Severity: Low]
Is this include needed?
It doesn't appear that any delay functions like msleep() or usleep_range()
are used in the driver implementation.
> +#include <linux/device.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/i2c.h>
> +#include <linux/leds.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <linux/types.h>
[Severity: Low]
Should this file include <linux/property.h>?
The driver uses fwnode property APIs such as device_for_each_child_node_scoped(),
fwnode_property_read_u32(), and fwnode_property_present(), which are defined
in <linux/property.h>.
> +
> +/* LTC3220 Registers */
> +#define LTC3220_COMMAND_REG 0x00
> +#define LTC3220_QUICK_WRITE_MASK BIT(0)
> +#define LTC3220_SHUTDOWN_MASK BIT(3)
> +
> +#define LTC3220_ULED_REG(x) (0x01 + (x))
> +#define LTC3220_LED_CURRENT_MASK GENMASK(5, 0)
> +#define LTC3220_LED_MODE_MASK GENMASK(7, 6)
> +
> +#define LTC3220_GRAD_BLINK_REG 0x13
> +#define LTC3220_GRADATION_MASK GENMASK(2, 0)
> +#define LTC3220_GRADATION_DIRECTION_MASK BIT(0)
> +#define LTC3220_GRADATION_PERIOD_MASK GENMASK(2, 1)
> +#define LTC3220_BLINK_MASK GENMASK(4, 3)
> +
> +#define LTC3220_NUM_LEDS 18
> +
> +#define LTC3220_GRADATION_START_VALUE 128
[Severity: Low]
Is this macro used anywhere in the driver?
It appears to be dead code as it is never referenced.
> +#define LTC3220_GRADATION_RAMP_TIME_240MS 240
> +#define LTC3220_GRADATION_RAMP_TIME_480MS 480
> +
> +#define LTC3220_BLINK_ON_156MS 156
> +#define LTC3220_BLINK_ON_625MS 625
> +#define LTC3220_BLINK_PERIOD_1250MS 1250
> +#define LTC3220_BLINK_PERIOD_2500MS 2500
> +
> +#define LTC3220_BLINK_SHORT_ON_TIME BIT(0)
> +#define LTC3220_BLINK_LONG_PERIOD BIT(1)
> +
> +enum ltc3220_led_mode {
> + LTC3220_NORMAL_MODE,
> + LTC3220_BLINK_MODE,
> + LTC3220_GRADATION_MODE,
> +};
> +
> +enum ltc3220_blink_mode {
> + LTC3220_BLINK_MODE_625MS_1250MS,
> + LTC3220_BLINK_MODE_156MS_1250MS,
> + LTC3220_BLINK_MODE_625MS_2500MS,
> + LTC3220_BLINK_MODE_156MS_2500MS
> +};
> +
> +enum ltc3220_gradation_mode {
> + LTC3220_GRADATION_MODE_DISABLED,
> + LTC3220_GRADATION_MODE_240MS_RAMP_TIME,
> + LTC3220_GRADATION_MODE_480MS_RAMP_TIME,
> + LTC3220_GRADATION_MODE_960MS_RAMP_TIME
> +};
> +
> +static bool ltc3220_volatile_reg(struct device *dev, unsigned int reg)
> +{
> + return reg == LTC3220_GRAD_BLINK_REG;
> +}
[Severity: High]
Does this register need to be marked volatile?
Marking a register as volatile tells regmap to bypass the cache and read from
the physical hardware. If the LTC3220 is a write-only I2C device, the physical
read will fail, causing regmap_update_bits() to fail entirely in
ltc3220_pattern_set() and ltc3220_blink_set().
Even if the device supports reads, this register is exclusively controlled by
software and does not change autonomously, so could it be cached normally to
avoid unnecessary I2C reads?
> +
> +static const struct regmap_config ltc3220_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 8,
> + .max_register = LTC3220_GRAD_BLINK_REG,
> + .cache_type = REGCACHE_FLAT_S,
> + .volatile_reg = ltc3220_volatile_reg,
> +};
> +
> +struct ltc3220_uled_cfg {
> + struct led_classdev led_cdev;
> + u8 reg_value;
> + u8 led_index;
> +};
> +
> +struct ltc3220 {
> + struct ltc3220_uled_cfg uled_cfg[LTC3220_NUM_LEDS];
> + struct regmap *regmap;
> + bool is_aggregated;
> +};
> +
> +/*
> + * Set LED brightness. Hardware supports 0-63 brightness levels.
> + * Mode switching (blink/gradation) is handled through dedicated callbacks.
> + */
> +static int ltc3220_set_led_data(struct led_classdev *led_cdev,
> + enum led_brightness brightness)
> +{
> + struct ltc3220_uled_cfg *uled_cfg = container_of(led_cdev, struct ltc3220_uled_cfg,
> + led_cdev);
> + struct ltc3220 *ltc3220 = container_of(uled_cfg, struct ltc3220,
> + uled_cfg[uled_cfg->led_index]);
[Severity: Low]
Will this dynamic variable index inside container_of() cause a build failure?
The driver attempts to retrieve the parent structure using
uled_cfg[uled_cfg->led_index]. The container_of macro relies on offsetof(),
which strictly requires the member argument to be a constant expression.
Since uled_cfg->led_index is a runtime variable, this violates the C standard
and can abort compilation on compilers like Clang with an error about a
non-constant integer expression.
> + int ret;
> +
> + ret = regmap_write(ltc3220->regmap, LTC3220_ULED_REG(uled_cfg->led_index),
> + brightness);
> + if (ret)
> + return ret;
> +
> + uled_cfg->reg_value = brightness;
> +
> + /*
> + * When aggregated LED mode is enabled, writing to LED 1 updates all
> + * LEDs simultaneously via quick-write mode. Update cached values for
> + * all LEDs to reflect the synchronized state.
> + * See Documentation/devicetree/bindings/leds/adi,ltc3220.yaml for how
> + * to configure aggregated LED mode.
> + */
> + if (ltc3220->is_aggregated && uled_cfg->led_index == 0) {
> + for (int i = 0; i < LTC3220_NUM_LEDS; i++)
> + ltc3220->uled_cfg[i].reg_value = brightness;
> + }
> +
> + return 0;
> +}
> +
> +static enum led_brightness ltc3220_get_led_data(struct led_classdev *led_cdev)
> +{
> + struct ltc3220_uled_cfg *uled_cfg = container_of(led_cdev, struct ltc3220_uled_cfg,
> + led_cdev);
> +
> + return uled_cfg->reg_value;
> +}
> +
> +/*
> + * LTC3220 pattern support for hardware-assisted breathing/gradation.
> + * The hardware supports 3 gradation ramp time 240ms, 480ms, 960ms)
> + * and can ramp up or down.
> + *
> + * Pattern array interpretation:
> + * pattern[0].brightness = start brightness (0-63)
> + * pattern[0].delta_t = ramp time in milliseconds
> + * pattern[1].brightness = end brightness (0-63)
> + * pattern[1].delta_t = (optional, can be 0 or same as pattern[0].delta_t)
> + */
> +static int ltc3220_pattern_set(struct led_classdev *led_cdev,
> + struct led_pattern *pattern,
> + u32 len, int repeat)
> +{
> + struct ltc3220_uled_cfg *uled_cfg = container_of(led_cdev, struct ltc3220_uled_cfg,
> + led_cdev);
> + struct ltc3220 *ltc3220 = container_of(uled_cfg, struct ltc3220,
> + uled_cfg[uled_cfg->led_index]);
[Severity: Low]
Will this dynamic variable index inside container_of() cause a build failure?
The driver attempts to retrieve the parent structure using
uled_cfg[uled_cfg->led_index]. The container_of macro relies on offsetof(),
which strictly requires the member argument to be a constant expression.
Since uled_cfg->led_index is a runtime variable, this violates the C standard
and can abort compilation on compilers like Clang with an error about a
non-constant integer expression.
> + u8 gradation_period;
> + u8 start_brightness;
> + u8 end_brightness;
> + u8 gradation_val;
> + bool is_increasing;
> + int ret;
> +
> + if (len != 2)
> + return -EINVAL;
> +
> + start_brightness = pattern[0].brightness & LTC3220_LED_CURRENT_MASK;
> + end_brightness = pattern[1].brightness & LTC3220_LED_CURRENT_MASK;
> +
> + is_increasing = end_brightness > start_brightness;
> +
> + if (pattern[0].delta_t == 0)
> + gradation_period = LTC3220_GRADATION_MODE_DISABLED;
> + else if (pattern[0].delta_t <= LTC3220_GRADATION_RAMP_TIME_240MS)
> + gradation_period = LTC3220_GRADATION_MODE_240MS_RAMP_TIME;
> + else if (pattern[0].delta_t <= LTC3220_GRADATION_RAMP_TIME_480MS)
> + gradation_period = LTC3220_GRADATION_MODE_480MS_RAMP_TIME;
> + else
> + gradation_period = LTC3220_GRADATION_MODE_960MS_RAMP_TIME;
> +
> + gradation_val = FIELD_PREP(LTC3220_GRADATION_PERIOD_MASK, gradation_period);
> + gradation_val |= FIELD_PREP(LTC3220_GRADATION_DIRECTION_MASK, is_increasing);
> +
> + ret = regmap_update_bits(ltc3220->regmap, LTC3220_GRAD_BLINK_REG,
> + LTC3220_GRADATION_MASK, gradation_val);
> + if (ret)
> + return ret;
> +
> + ret = ltc3220_set_led_data(led_cdev, start_brightness);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(ltc3220->regmap, LTC3220_ULED_REG(uled_cfg->led_index),
> + FIELD_PREP(LTC3220_LED_MODE_MASK, LTC3220_GRADATION_MODE) |
> + end_brightness);
> + if (ret)
> + return ret;
> +
> + uled_cfg->reg_value = end_brightness;
> +
> + return 0;
> +}
> +
> +static int ltc3220_pattern_clear(struct led_classdev *led_cdev)
> +{
> + struct ltc3220_uled_cfg *uled_cfg = container_of(led_cdev, struct ltc3220_uled_cfg,
> + led_cdev);
> + struct ltc3220 *ltc3220 = container_of(uled_cfg, struct ltc3220,
> + uled_cfg[uled_cfg->led_index]);
[Severity: Low]
Will this dynamic variable index inside container_of() cause a build failure?
The driver attempts to retrieve the parent structure using
uled_cfg[uled_cfg->led_index]. The container_of macro relies on offsetof(),
which strictly requires the member argument to be a constant expression.
Since uled_cfg->led_index is a runtime variable, this violates the C standard
and can abort compilation on compilers like Clang with an error about a
non-constant integer expression.
> + int ret;
> +
> + ret = regmap_update_bits(ltc3220->regmap, LTC3220_ULED_REG(uled_cfg->led_index),
> + LTC3220_LED_MODE_MASK, LTC3220_NORMAL_MODE);
> + if (ret)
> + return ret;
> +
> + return ltc3220_set_led_data(led_cdev, LED_OFF);
> +}
> +
> +/*
> + * LTC3220 has a global blink configuration that affects all LEDs.
> + * This implementation allows per-LED blink requests, but the blink timing
> + * will be shared across all LEDs. The delay values are mapped to the
> + * hardware's discrete blink rates.
> + */
> +static int ltc3220_blink_set(struct led_classdev *led_cdev,
> + unsigned long *delay_on,
> + unsigned long *delay_off)
> +{
> + struct ltc3220_uled_cfg *uled_cfg = container_of(led_cdev, struct ltc3220_uled_cfg,
> + led_cdev);
> + struct ltc3220 *ltc3220 = container_of(uled_cfg, struct ltc3220,
> + uled_cfg[uled_cfg->led_index]);
[Severity: Low]
Will this dynamic variable index inside container_of() cause a build failure?
The driver attempts to retrieve the parent structure using
uled_cfg[uled_cfg->led_index]. The container_of macro relies on offsetof(),
which strictly requires the member argument to be a constant expression.
Since uled_cfg->led_index is a runtime variable, this violates the C standard
and can abort compilation on compilers like Clang with an error about a
non-constant integer expression.
> + u8 blink_mode = 0;
> + int ret;
> +
> + if (*delay_on <= LTC3220_BLINK_ON_156MS)
> + blink_mode = LTC3220_BLINK_SHORT_ON_TIME;
> +
> + if (*delay_on + *delay_off > LTC3220_BLINK_PERIOD_1250MS)
> + blink_mode |= LTC3220_BLINK_LONG_PERIOD;
> +
> + switch (blink_mode) {
> + case LTC3220_BLINK_MODE_625MS_1250MS:
> + *delay_on = LTC3220_BLINK_ON_625MS;
> + *delay_off = LTC3220_BLINK_PERIOD_1250MS - LTC3220_BLINK_ON_625MS;
> + break;
> + case LTC3220_BLINK_MODE_156MS_1250MS:
> + *delay_on = LTC3220_BLINK_ON_156MS;
> + *delay_off = LTC3220_BLINK_PERIOD_1250MS - LTC3220_BLINK_ON_156MS;
> + break;
> + case LTC3220_BLINK_MODE_625MS_2500MS:
> + *delay_on = LTC3220_BLINK_ON_625MS;
> + *delay_off = LTC3220_BLINK_PERIOD_2500MS - LTC3220_BLINK_ON_625MS;
> + break;
> + case LTC3220_BLINK_MODE_156MS_2500MS:
> + *delay_on = LTC3220_BLINK_ON_156MS;
> + *delay_off = LTC3220_BLINK_PERIOD_2500MS - LTC3220_BLINK_ON_156MS;
> + break;
> + }
> +
> + ret = regmap_update_bits(ltc3220->regmap, LTC3220_GRAD_BLINK_REG,
> + LTC3220_BLINK_MASK, FIELD_PREP(LTC3220_BLINK_MASK, blink_mode));
> + if (ret)
> + return ret;
> +
> + return regmap_update_bits(ltc3220->regmap, LTC3220_ULED_REG(uled_cfg->led_index),
> + LTC3220_LED_MODE_MASK,
> + FIELD_PREP(LTC3220_LED_MODE_MASK, LTC3220_BLINK_MODE));
> +}
[Severity: Medium]
Will the hardware blink activate at 0 brightness if the LED was previously off?
When the LED is off, its current bits in LTC3220_ULED_REG are 0. The driver
updates the mode bits to LTC3220_BLINK_MODE using regmap_update_bits() but
does not update the brightness bits.
Because the core expects the driver to handle brightness configuration for
hardware blinking, could this hardware blink execute at 0 brightness, leaving
the LED unlit?
> +
> +static void ltc3220_reset_gpio_action(void *data)
> +{
> + struct gpio_desc *reset_gpio = data;
> +
> + gpiod_set_value_cansleep(reset_gpio, 1);
> +}
> +
> +static int ltc3220_reset(struct ltc3220 *ltc3220, struct i2c_client *client)
> +{
> + struct gpio_desc *reset_gpio;
> + int ret;
> +
> + reset_gpio = devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_HIGH);
> + if (IS_ERR(reset_gpio))
> + return dev_err_probe(&client->dev, PTR_ERR(reset_gpio), "Failed on reset GPIO\n");
> +
> + if (reset_gpio) {
> + gpiod_set_value_cansleep(reset_gpio, 0);
[Severity: High]
Does this sequence lack the necessary delays to ensure a successful reset?
The driver asserts the reset GPIO by requesting it with GPIOD_OUT_HIGH and
immediately de-asserts it with gpiod_set_value_cansleep(reset_gpio, 0). There
is no delay between these calls, creating a near-zero width pulse that may fail
to meet the IC's minimum reset pulse width requirement.
It also lacks a wakeup delay before subsequent I2C commands might be sent.
> +
> + return devm_add_action_or_reset(&client->dev, ltc3220_reset_gpio_action,
> + reset_gpio);
[Severity: High]
Does returning early here skip software initialization and regmap cache setup?
If the reset_gpio is successfully acquired, this function immediately returns
0, entirely skipping the subsequent regmap_write() calls intended to initialize
the hardware registers.
Because the driver does not define a .reg_defaults table, skipping these writes
leaves the regmap cache uninitialized, which can break subsequent
read-modify-write logic via regmap_update_bits().
> + }
> +
> + ret = regmap_write(ltc3220->regmap, LTC3220_COMMAND_REG, 0);
> + if (ret)
> + return ret;
> +
> + for (int i = 0; i < LTC3220_NUM_LEDS; i++) {
> + ret = regmap_write(ltc3220->regmap, LTC3220_ULED_REG(i), 0);
> + if (ret)
> + return ret;
> + }
> +
> + return regmap_write(ltc3220->regmap, LTC3220_GRAD_BLINK_REG, 0);
> +}
> +
> +static int ltc3220_suspend(struct device *dev)
> +{
> + struct ltc3220 *ltc3220 = i2c_get_clientdata(to_i2c_client(dev));
> +
> + return regmap_update_bits(ltc3220->regmap, LTC3220_COMMAND_REG,
> + LTC3220_SHUTDOWN_MASK, LTC3220_SHUTDOWN_MASK);
> +}
> +
> +static int ltc3220_resume(struct device *dev)
> +{
> + struct ltc3220 *ltc3220 = i2c_get_clientdata(to_i2c_client(dev));
> +
> + return regmap_update_bits(ltc3220->regmap, LTC3220_COMMAND_REG,
> + LTC3220_SHUTDOWN_MASK, 0);
> +}
[Severity: High]
Are the hardware registers left permanently off after resume?
In ltc3220_suspend(), the driver sets the LTC3220_SHUTDOWN_MASK bit. On many
such chips, asserting the shutdown bit resets internal registers. Additionally,
power to the IC may be lost during system suspend.
In ltc3220_resume(), the driver clears the shutdown bit but does not call
regcache_mark_dirty() and regcache_sync(). Could this leave the physical LED
outputs permanently off, while the kernel's regmap cache and LED classdev state
incorrectly reflect that the LEDs are still configured and illuminated?
> +
> +static DEFINE_SIMPLE_DEV_PM_OPS(ltc3220_pm_ops, ltc3220_suspend, ltc3220_resume);
> +
> +static int ltc3220_probe(struct i2c_client *client)
> +{
> + struct ltc3220 *ltc3220;
> + bool aggregated_led_found = false;
> + int num_leds = 0;
> + u8 led_index = 0;
> + int ret;
> +
> + ltc3220 = devm_kzalloc(&client->dev, sizeof(*ltc3220), GFP_KERNEL);
> + if (!ltc3220)
> + return -ENOMEM;
> +
> + ltc3220->regmap = devm_regmap_init_i2c(client, <c3220_regmap_config);
> + if (IS_ERR(ltc3220->regmap))
> + return dev_err_probe(&client->dev, PTR_ERR(ltc3220->regmap),
> + "Failed to initialize regmap\n");
> +
> + i2c_set_clientdata(client, ltc3220);
> +
> + ret = ltc3220_reset(ltc3220, client);
> + if (ret)
> + return dev_err_probe(&client->dev, ret, "Failed to reset device\n");
> +
> + device_for_each_child_node_scoped(&client->dev, child) {
> + struct led_init_data init_data = {};
> + struct ltc3220_uled_cfg *led;
> + u32 source;
> +
> + ret = fwnode_property_read_u32(child, "reg", &source);
> + if (ret)
> + return dev_err_probe(&client->dev, ret, "Couldn't read LED address\n");
> +
> + if (!source || source > LTC3220_NUM_LEDS)
> + return dev_err_probe(&client->dev, -EINVAL, "LED address out of range\n");
> +
> + init_data.fwnode = child;
> + init_data.devicename = "ltc3220";
> +
> + if (fwnode_property_present(child, "led-sources")) {
> + if (source != 1)
> + return dev_err_probe(&client->dev, -EINVAL,
> + "Aggregated LED out of range\n");
> +
> + if (aggregated_led_found)
> + return dev_err_probe(&client->dev, -EINVAL,
> + "One Aggregated LED only\n");
> +
> + aggregated_led_found = true;
> + ltc3220->is_aggregated = true;
> +
> + ret = regmap_update_bits(ltc3220->regmap,
> + LTC3220_COMMAND_REG,
> + LTC3220_QUICK_WRITE_MASK,
> + LTC3220_QUICK_WRITE_MASK);
> + if (ret)
> + return dev_err_probe(&client->dev, ret,
> + "Failed to set quick write mode\n");
> + }
> +
> + num_leds++;
> +
> + /* LED node reg/index/address goes from 1 to 18 */
> + led_index = source - 1;
> + led = <c3220->uled_cfg[led_index];
> +
> + if (led->led_cdev.dev)
> + return dev_err_probe(&client->dev, -EINVAL,
> + "Duplicate LED reg %u found", source);
> +
> + led->led_index = led_index;
> + led->reg_value = 0;
> + led->led_cdev.brightness_set_blocking = ltc3220_set_led_data;
> + led->led_cdev.brightness_get = ltc3220_get_led_data;
> + led->led_cdev.max_brightness = 63;
> + led->led_cdev.blink_set = ltc3220_blink_set;
> + led->led_cdev.pattern_set = ltc3220_pattern_set;
> + led->led_cdev.pattern_clear = ltc3220_pattern_clear;
> +
> + ret = devm_led_classdev_register_ext(&client->dev, &led->led_cdev, &init_data);
> + if (ret)
> + return dev_err_probe(&client->dev, ret, "Failed to register LED class\n");
> + }
> +
> + /*
> + * Aggregated LED mode uses hardware quick-write to control all 18 LEDs
> + * simultaneously. This is mutually exclusive with individual LED control.
> + * See Documentation/devicetree/bindings/leds/adi,ltc3220.yaml for details
> + * on how to configure aggregated LED mode.
> + */
> + if (aggregated_led_found && num_leds > 1)
> + return dev_err_probe(&client->dev, -EINVAL,
> + "Aggregated LED must be the only LED node\n");
[Severity: Medium]
Are the LED sysfs interfaces exposed to userspace before the driver probe is
fully validated?
Inside the loop above, devm_led_classdev_register_ext() is called, which
immediately exposes sysfs interfaces. If the validation check here fails and
returns an error, userspace could have already discovered and interacted with
the LED devices before they are unexpectedly torn down by devres.
Could these validation checks be completed before registering the interfaces?
> +
> + return 0;
> +}
> +
> +static const struct of_device_id ltc3220_of_match[] = {
> + { .compatible = "adi,ltc3220" },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, ltc3220_of_match);
> +
> +static struct i2c_driver ltc3220_led_driver = {
> + .driver = {
> + .name = "ltc3220",
> + .of_match_table = ltc3220_of_match,
> + .pm = pm_sleep_ptr(<c3220_pm_ops),
> + },
> + .probe = ltc3220_probe,
> +};
> +module_i2c_driver(ltc3220_led_driver);
> +
> +MODULE_AUTHOR("Edelweise Escala <edelweise.escala@analog.com>");
> +MODULE_DESCRIPTION("LED driver for LTC3220 controllers");
> +MODULE_LICENSE("GPL");
>
> --
> 2.43.0
>
--
Lee Jones
^ permalink raw reply
* Re: [PATCH v8 5/7] leds: Add driver for ASUS Transformer LEDs
From: Lee Jones @ 2026-06-11 11:30 UTC (permalink / raw)
To: Svyatoslav Ryhel
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Dmitry Torokhov,
Pavel Machek, Sebastian Reichel, Ion Agorria,
Michał Mirosław, devicetree, linux-kernel, linux-input,
linux-leds, linux-pm
In-Reply-To: <20260528053203.9339-6-clamor95@gmail.com>
On Thu, 28 May 2026, Svyatoslav Ryhel wrote:
> From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
>
> ASUS Transformer tablets have a green and an amber LED on both the Pad
> and the Dock. If both LEDs are enabled simultaneously, the emitted light
> will be yellow.
>
> Co-developed-by: Svyatoslav Ryhel <clamor95@gmail.com>
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> ---
> drivers/leds/Kconfig | 11 +++
> drivers/leds/Makefile | 1 +
> drivers/leds/leds-asus-transformer-ec.c | 125 ++++++++++++++++++++++++
> 3 files changed, 137 insertions(+)
> create mode 100644 drivers/leds/leds-asus-transformer-ec.c
>
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index f4a0a3c8c870..f637d23400a8 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -120,6 +120,17 @@ config LEDS_OSRAM_AMS_AS3668
> To compile this driver as a module, choose M here: the module
> will be called leds-as3668.
>
> +config LEDS_ASUS_TRANSFORMER_EC
> + tristate "LED Support for Asus Transformer charging LED"
> + depends on LEDS_CLASS
> + depends on MFD_ASUS_TRANSFORMER_EC
> + help
> + This option enables support for charging indicator on
> + Asus Transformer's Pad and it's Dock.
> +
> + To compile this driver as a module, choose M here: the module
> + will be called leds-asus-transformer-ec.
> +
> config LEDS_AW200XX
> tristate "LED support for Awinic AW20036/AW20054/AW20072/AW20108"
> depends on LEDS_CLASS
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index 8fdb45d5b439..d5395c3f1124 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -16,6 +16,7 @@ obj-$(CONFIG_LEDS_AN30259A) += leds-an30259a.o
> obj-$(CONFIG_LEDS_APU) += leds-apu.o
> obj-$(CONFIG_LEDS_ARIEL) += leds-ariel.o
> obj-$(CONFIG_LEDS_AS3668) += leds-as3668.o
> +obj-$(CONFIG_LEDS_ASUS_TRANSFORMER_EC) += leds-asus-transformer-ec.o
> obj-$(CONFIG_LEDS_AW200XX) += leds-aw200xx.o
> obj-$(CONFIG_LEDS_AW2013) += leds-aw2013.o
> obj-$(CONFIG_LEDS_BCM6328) += leds-bcm6328.o
> diff --git a/drivers/leds/leds-asus-transformer-ec.c b/drivers/leds/leds-asus-transformer-ec.c
> new file mode 100644
> index 000000000000..09503e76331c
> --- /dev/null
> +++ b/drivers/leds/leds-asus-transformer-ec.c
> @@ -0,0 +1,125 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <linux/err.h>
> +#include <linux/leds.h>
> +#include <linux/mfd/asus-transformer-ec.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +enum {
> + ASUSEC_LED_AMBER,
> + ASUSEC_LED_GREEN,
> + ASUSEC_LED_MAX
> +};
> +
> +struct asus_ec_led_config {
> + const char *name;
> + unsigned int color;
> + unsigned long long ctrl_bit;
Should we use 'u64' here instead of 'unsigned long long' to align with standard
kernel integer types?
> +};
> +
> +struct asus_ec_led {
> + struct asus_ec_leds_data *ddata;
> + struct led_classdev cdev;
> + unsigned long long ctrl_bit;
Should we use 'u64' here as well to keep it consistent?
> +};
> +
> +struct asus_ec_leds_data {
> + const struct asusec_core *ec;
> + struct asus_ec_led leds[ASUSEC_LED_MAX];
> +};
> +
> +static const struct asus_ec_led_config asus_ec_leds[] = {
> + [ASUSEC_LED_AMBER] = {
> + .name = "amber",
> + .color = LED_COLOR_ID_AMBER,
> + .ctrl_bit = ASUSEC_CTL_LED_AMBER,
> + },
> + [ASUSEC_LED_GREEN] = {
> + .name = "green",
> + .color = LED_COLOR_ID_GREEN,
> + .ctrl_bit = ASUSEC_CTL_LED_GREEN,
> + },
> +};
> +
> +static enum led_brightness asus_ec_led_get_brightness(struct led_classdev *cdev)
> +{
> + struct asus_ec_led *led = container_of(cdev, struct asus_ec_led, cdev);
> + const struct asusec_core *ec = led->ddata->ec;
I'm getting confused here.
ddata is what I'd be calling the device data struct passed by the parent?
In fact, ddata is a little known concept in Leds. Any reason to go for
this over the standard nomenclature?
> + u64 ctl;
> + int ret;
> +
> + ret = asus_dockram_access_ctl(ec->dockram, &ctl, 0, 0);
Did we discuss preferring regmap already?
> + if (ret)
> + return LED_OFF;
> +
> + return ctl & led->ctrl_bit ? LED_ON : LED_OFF;
> +}
> +
> +static int asus_ec_led_set_brightness(struct led_classdev *cdev,
> + enum led_brightness brightness)
> +{
> + struct asus_ec_led *led = container_of(cdev, struct asus_ec_led, cdev);
> + const struct asusec_core *ec = led->ddata->ec;
> +
> + if (brightness)
> + return asus_dockram_access_ctl(ec->dockram, NULL,
> + led->ctrl_bit, led->ctrl_bit);
> +
> + return asus_dockram_access_ctl(ec->dockram, NULL, led->ctrl_bit, 0);
> +}
> +
> +static int asus_ec_led_probe(struct platform_device *pdev)
> +{
> + const struct asusec_core *ec = dev_get_drvdata(pdev->dev.parent);
> + struct asus_ec_leds_data *ddata;
> + struct device *dev = &pdev->dev;
> + int i, ret;
Could we declare the loop counter 'i' directly within the 'for' statement's
scope to keep its scope limited? For example, 'for (int i = 0; ...)'.
> +
> + ddata = devm_kzalloc(dev, sizeof(*ddata), GFP_KERNEL);
> + if (!ddata)
> + return -ENOMEM;
> +
> + platform_set_drvdata(pdev, ddata);
> + ddata->ec = ec;
> +
> + for (i = 0; i < ASUSEC_LED_MAX; i++) {
Nit: for (int i = ...
> + const struct asus_ec_led_config *cfg = &asus_ec_leds[i];
> + struct asus_ec_led *led = &ddata->leds[i];
> +
> + led->cdev.name = devm_kasprintf(dev, GFP_KERNEL, "%s::%s",
> + ddata->ec->name, cfg->name);
> + if (!led->cdev.name)
> + return -ENOMEM;
> +
> + led->cdev.max_brightness = 1;
> + led->cdev.color = cfg->color;
> + led->cdev.flags = LED_CORE_SUSPENDRESUME | LED_RETAIN_AT_SHUTDOWN;
> + led->cdev.brightness_get = asus_ec_led_get_brightness;
> + led->cdev.brightness_set_blocking = asus_ec_led_set_brightness;
> +
> + led->ddata = ddata;
> + led->ctrl_bit = cfg->ctrl_bit;
> +
> + ret = devm_led_classdev_register(dev, &led->cdev);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "failed to register %s LED\n",
> + cfg->name);
Should we capitalise the error message here to match our style guidelines
(e.g. 'Failed to register...')?
> + }
> +
> + return 0;
> +}
> +
> +static struct platform_driver asus_ec_led_driver = {
> + .driver.name = "asus-transformer-ec-led",
> + .probe = asus_ec_led_probe,
> +};
> +module_platform_driver(asus_ec_led_driver);
> +
> +MODULE_ALIAS("platform:asus-transformer-ec-led");
> +MODULE_AUTHOR("Michał Mirosław <mirq-linux@rere.qmqm.pl>");
> +MODULE_AUTHOR("Svyatoslav Ryhel <clamor95@gmail.com>");
> +MODULE_DESCRIPTION("ASUS Transformer's charging LED driver");
> +MODULE_LICENSE("GPL");
> --
> 2.51.0
>
>
--
Lee Jones
^ permalink raw reply
* Re: [PATCH v8 2/7] mfd: Add driver for ASUS Transformer embedded controller
From: Lee Jones @ 2026-06-11 11:17 UTC (permalink / raw)
To: Svyatoslav Ryhel
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Dmitry Torokhov,
Pavel Machek, Sebastian Reichel, Ion Agorria,
Michał Mirosław, devicetree, linux-kernel, linux-input,
linux-leds, linux-pm
In-Reply-To: <20260528053203.9339-3-clamor95@gmail.com>
On Thu, 28 May 2026, Svyatoslav Ryhel wrote:
> From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
>
> Support Nuvoton NPCE795-based ECs as used in Asus Transformer TF201,
> TF300T, TF300TG, TF300TL and TF700T pad and dock, as well as TF101 dock
> and TF600T, P1801-T and TF701T pad. This is a glue driver handling
> detection and common operations for EC's functions.
>
> Co-developed-by: Svyatoslav Ryhel <clamor95@gmail.com>
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> ---
> drivers/mfd/Kconfig | 16 +
> drivers/mfd/Makefile | 1 +
> drivers/mfd/asus-transformer-ec.c | 542 ++++++++++++++++++++++++
> include/linux/mfd/asus-transformer-ec.h | 92 ++++
> 4 files changed, 651 insertions(+)
> create mode 100644 drivers/mfd/asus-transformer-ec.c
> create mode 100644 include/linux/mfd/asus-transformer-ec.h
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 7192c9d1d268..e1c32505b97a 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -137,6 +137,22 @@ config MFD_AAT2870_CORE
> additional drivers must be enabled in order to use the
> functionality of the device.
>
> +config MFD_ASUS_TRANSFORMER_EC
> + tristate "ASUS Transformer's embedded controller"
> + select MFD_CORE
> + depends on I2C && OF
> + help
> + Select this to enable support for the Embedded Controller (EC)
> + found in Tegra based ASUS Transformer series tablets and mobile
> + docks.
> +
> + This driver handles the core I2C communication with the EC and
> + provides support for its sub-devices, including battery management,
> + charger detection, LEDs and keyboard dock functions support.
> +
> + This driver can also be built as a module. If so, the module
> + will be called asus-transformer-ec.
> +
> config MFD_AT91_USART
> tristate "AT91 USART Driver"
> select MFD_CORE
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index e75e8045c28a..fd80088d8a9a 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -10,6 +10,7 @@ obj-$(CONFIG_MFD_88PM805) += 88pm805.o 88pm80x.o
> obj-$(CONFIG_MFD_88PM886_PMIC) += 88pm886.o
> obj-$(CONFIG_MFD_ACT8945A) += act8945a.o
> obj-$(CONFIG_MFD_SM501) += sm501.o
> +obj-$(CONFIG_MFD_ASUS_TRANSFORMER_EC) += asus-transformer-ec.o
> obj-$(CONFIG_ARCH_BCM2835) += bcm2835-pm.o
> obj-$(CONFIG_MFD_BCM590XX) += bcm590xx.o
> obj-$(CONFIG_MFD_BD9571MWV) += bd9571mwv.o
> diff --git a/drivers/mfd/asus-transformer-ec.c b/drivers/mfd/asus-transformer-ec.c
> new file mode 100644
> index 000000000000..1f5900d0fdc9
> --- /dev/null
> +++ b/drivers/mfd/asus-transformer-ec.c
> @@ -0,0 +1,542 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +
> +#include <linux/array_size.h>
> +#include <linux/debugfs.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/mfd/asus-transformer-ec.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/property.h>
> +#include <linux/slab.h>
> +#include <linux/string.h>
> +#include <linux/types.h>
> +#include <linux/unaligned.h>
> +
> +#define ASUSEC_RSP_BUFFER_SIZE (ASUSEC_ENTRIES / ASUSEC_ENTRY_SIZE)
> +
> +#define ASUSEC_RESET 0
> +#define ASUSEC_RETRY_MAX 3
> +#define ASUSEC_ACCESS_TIMEOUT 300
> +
> +enum asusec_variant {
> + ASUSEC_SL101_DOCK = 1,
> + ASUSEC_TF101_DOCK,
> + ASUSEC_TF201_PAD,
> + ASUSEC_TF600T_PAD,
> + ASUSEC_MAX
> +};
> +
> +enum asusec_mode {
> + ASUSEC_MODE_NONE,
> + ASUSEC_MODE_NORMAL,
> + ASUSEC_MODE_FACTORY,
> + ASUSEC_MODE_MAX
> +};
> +
> +/**
> + * struct asus_ec_chip_info
> + *
> + * @name: prefix associated with the EC
> + * @variant: id of programming model of EC
> + * @mode: state of Factory Mode bit in EC control register
> + */
> +struct asus_ec_chip_info {
> + const char *name;
> + enum asusec_variant variant;
> + enum asusec_mode fmode;
> +};
> +
> +/**
> + * struct asus_ec_data
> + *
> + * @ec: public part shared with all cells (must be first)
> + * @ecreq_lock: prevents simultaneous access to EC
> + * @ecreq_gpio: EC request GPIO
> + * @client: pointer to EC's i2c_client
> + * @info: pointer to EC's version description
> + * @ec_buf: buffer for EC read
> + * @logging_disabled: flag disabling logging on reset events
> + */
> +struct asus_ec_data {
> + struct asusec_core ec;
> + struct mutex ecreq_lock;
> + struct gpio_desc *ecreq_gpio;
> + struct i2c_client *client;
> + const struct asus_ec_chip_info *info;
> + u8 ec_buf[ASUSEC_ENTRY_BUFSIZE];
> + bool logging_disabled;
> +};
> +
> +/**
> + * struct dockram_ec_data
> + *
> + * @ctl_lock: prevent simultaneous access to Dockram
> + * @ctl_buf: buffer for Dockram read
> + */
> +struct dockram_ec_data {
> + struct mutex ctl_lock;
> + u8 ctl_buf[ASUSEC_ENTRY_BUFSIZE];
> +};
> +
> +/**
> + * asus_dockram_access_ctl - Read from or write to the DockRAM control register.
> + * @client: Handle to the DockRAM device.
> + * @out: Pointer to a variable where the register value will be stored.
> + * @mask: Bitmask of bits to be cleared.
> + * @xor: Bitmask of bits to be set (via XOR).
> + *
> + * This performs a control register read if @out is provided and both @mask
> + * and @xor are zero. Otherwise, it performs a control register update if
> + * @mask and @xor are provided.
> + *
> + * Returns a negative errno code else zero on success.
> + */
> +int asus_dockram_access_ctl(struct i2c_client *client, u64 *out, u64 mask,
> + u64 xor)
> +{
> + struct dockram_ec_data *ddata = i2c_get_clientdata(client);
> + u8 *buf = ddata->ctl_buf;
> + u64 val;
> + int ret = 0;
> +
> + guard(mutex)(&ddata->ctl_lock);
> +
> + memset(buf, 0, ASUSEC_ENTRY_BUFSIZE);
> + ret = i2c_smbus_read_i2c_block_data(client, ASUSEC_DOCKRAM_CONTROL,
> + ASUSEC_ENTRY_SIZE, buf);
> + if (ret < ASUSEC_ENTRY_SIZE) {
> + dev_err(&client->dev, "failed to access control buffer: %d\n",
> + ret);
> + return ret;
Should we return a negative error code here if the read is shorter than
expected, rather than propagating the positive byte count?
> + }
> +
> + if (buf[0] != ASUSEC_CTL_SIZE) {
> + dev_err(&client->dev, "buffer size exceeds %d: %d\n",
> + ASUSEC_CTL_SIZE, buf[0]);
> + return -EPROTO;
> + }
> +
> + val = get_unaligned_le64(buf + 1);
> +
> + if (out)
> + *out = val;
> +
> + if (mask || xor) {
> + put_unaligned_le64((val & ~mask) ^ xor, buf + 1);
> + ret = i2c_smbus_write_i2c_block_data(client,
> + ASUSEC_DOCKRAM_CONTROL,
> + ASUSEC_ENTRY_SIZE, buf);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(asus_dockram_access_ctl);
> +
> +static int asus_ec_signal_request(struct asus_ec_data *ddata)
> +{
> + guard(mutex)(&ddata->ecreq_lock);
> +
> + gpiod_set_value_cansleep(ddata->ecreq_gpio, 1);
> + msleep(50);
> +
> + gpiod_set_value_cansleep(ddata->ecreq_gpio, 0);
> + msleep(200);
Do these numbers come from the datasheet or were they arbitrarily chosen?
> +
> + return 0;
> +}
> +
> +static void asus_ec_clear_buffer(struct asus_ec_data *ddata)
> +{
> + int ret, retry = ASUSEC_RSP_BUFFER_SIZE;
> +
> + /*
> + * Read the buffer till we get valid data by checking ASUSEC_OBF_MASK
> + * of the status byte or till we reach end of the 256 byte buffer.
> + */
> + while (retry--) {
> + ret = i2c_smbus_read_i2c_block_data(ddata->client, ASUSEC_READ_BUF,
> + ASUSEC_ENTRY_SIZE,
> + ddata->ec_buf);
> + if (ret < ASUSEC_ENTRY_SIZE)
> + continue;
> +
> + if (ddata->ec_buf[ASUSEC_IRQ_STATUS] & ASUSEC_OBF_MASK)
> + continue;
> +
> + break;
> + }
> +}
> +
> +static int asus_ec_log_info(struct asus_ec_data *ddata, unsigned int reg,
> + const char *name, const char **out)
> +{
> + struct device *dev = &ddata->client->dev;
> + u8 buf[ASUSEC_ENTRY_BUFSIZE];
> + int ret;
> +
> + memset(buf, 0, ASUSEC_ENTRY_BUFSIZE);
> + ret = i2c_smbus_read_i2c_block_data(ddata->ec.dockram, reg,
> + ASUSEC_ENTRY_SIZE, buf);
> + if (ret < ASUSEC_ENTRY_SIZE)
> + return ret;
Same here. These should be negative.
> +
> + if (buf[0] > ASUSEC_ENTRY_SIZE) {
> + dev_err(dev, "bad data len; buffer: %*ph; ret: %d\n",
> + ASUSEC_ENTRY_BUFSIZE, buf, ret);
> + return -EPROTO;
> + }
> +
> + if (!ddata->logging_disabled) {
> + dev_info(dev, "%-14s: %.*s\n", name, buf[0], buf + 1);
> +
> + if (out) {
> + *out = devm_kasprintf(dev, GFP_KERNEL, "%.*s",
> + buf[0], buf + 1);
> + if (!*out)
> + return -ENOMEM;
> + }
> + }
FWIW, I hate this! What does it give you now that development is done?
> + return 0;
> +}
> +
> +static int asus_ec_reset(struct asus_ec_data *ddata)
> +{
> + int retry, ret;
> +
> + guard(mutex)(&ddata->ecreq_lock);
> +
> + for (retry = 0; retry < ASUSEC_RETRY_MAX; retry++) {
for (int return = ... is generally preferred for throwaway variables.
> + ret = i2c_smbus_write_word_data(ddata->client, ASUSEC_WRITE_BUF,
> + ASUSEC_RESET);
> + if (!ret)
> + return 0;
> +
> + msleep(ASUSEC_ACCESS_TIMEOUT);
I like that this is defined, can we do that with the others please?
> + }
> +
> + return ret;
> +}
> +
> +static int asus_ec_susb_on_status(struct asus_ec_data *ddata)
> +{
> + u64 flag;
> + int ret;
> +
> + ret = asus_dockram_access_ctl(ddata->ec.dockram, &flag, 0, 0);
> + if (ret)
> + return ret;
> +
> + flag &= ASUSEC_CTL_SUSB_MODE;
> + dev_info(&ddata->client->dev, "EC FW behaviour: %s\n",
> + flag ? "susb on when receive ec_req" :
> + "susb on when system wakeup");
> +
> + return 0;
> +}
> +
> +static int asus_ec_set_factory_mode(struct asus_ec_data *ddata,
> + enum asusec_mode fmode)
> +{
> + dev_info(&ddata->client->dev, "Entering %s mode.\n",
> + fmode == ASUSEC_MODE_FACTORY ? "factory" : "normal");
> +
> + return asus_dockram_access_ctl(ddata->ec.dockram, NULL,
> + ASUSEC_CTL_FACTORY_MODE,
> + fmode == ASUSEC_MODE_FACTORY ?
> + ASUSEC_CTL_FACTORY_MODE : 0);
Why not create make:
ASUSEC_MODE_FACTORY == ASUSEC_CTL_FACTORY_MODE
What happens to NORMAL?
> +}
> +
> +static int asus_ec_detect(struct asus_ec_data *ddata)
> +{
> + int ret;
> +
> + ret = asus_ec_reset(ddata);
> + if (ret)
> + goto err_exit;
> +
> + asus_ec_clear_buffer(ddata);
> +
> + ret = asus_ec_log_info(ddata, ASUSEC_DOCKRAM_INFO_MODEL, "Model",
> + &ddata->ec.model);
You can use 100-chars and make the code look beautiful! :)
> + if (ret)
> + goto err_exit;
> +
> + ret = asus_ec_log_info(ddata, ASUSEC_DOCKRAM_INFO_FW, "FW version",
> + NULL);
> + if (ret)
> + goto err_exit;
> +
> + ret = asus_ec_log_info(ddata, ASUSEC_DOCKRAM_INFO_CFGFMT, "Config format",
> + NULL);
> + if (ret)
> + goto err_exit;
> +
> + ret = asus_ec_log_info(ddata, ASUSEC_DOCKRAM_INFO_HW, "HW version",
> + NULL);
> + if (ret)
> + goto err_exit;
> +
> + /* Disable logging on next EC request */
Why, but why?
> + ddata->logging_disabled = true;
> +
> + /* Check and inform about EC firmware behavior */
> + ret = asus_ec_susb_on_status(ddata);
> + if (ret)
> + goto err_exit;
> +
> + ddata->ec.name = ddata->info->name;
> +
> + /* Some EC require factory mode to be set normal on each request */
> + if (ddata->info->fmode)
> + ret = asus_ec_set_factory_mode(ddata, ddata->info->fmode);
> +
> +err_exit:
> + if (ret)
> + dev_err(&ddata->client->dev, "failed to access EC: %d\n", ret);
> +
> + return ret;
> +}
> +
> +static void asus_ec_handle_smi(struct asus_ec_data *ddata, unsigned int code)
> +{
> + switch (code) {
> + case ASUSEC_SMI_HANDSHAKE:
> + case ASUSEC_SMI_RESET:
> + asus_ec_detect(ddata);
> + break;
> + }
> +}
> +
> +static irqreturn_t asus_ec_interrupt(int irq, void *dev_id)
> +{
> + struct asus_ec_data *ddata = dev_id;
> + unsigned long notify_action;
> + int ret;
> +
> + ret = i2c_smbus_read_i2c_block_data(ddata->client, ASUSEC_READ_BUF,
> + ASUSEC_ENTRY_SIZE, ddata->ec_buf);
> + if (ret < ASUSEC_ENTRY_SIZE ||
> + !(ddata->ec_buf[ASUSEC_IRQ_STATUS] & ASUSEC_OBF_MASK))
Unwrap for readability.
Also, I think a comment would be helpful.
> + return IRQ_NONE;
> +
> + notify_action = ddata->ec_buf[ASUSEC_IRQ_STATUS];
> + if (notify_action & ASUSEC_SMI_MASK) {
> + unsigned int code = ddata->ec_buf[ASUSEC_SMI_CODE];
> +
> + asus_ec_handle_smi(ddata, code);
> +
> + notify_action |= code << 8;
> + }
> +
> + blocking_notifier_call_chain(&ddata->ec.notify_list,
> + notify_action, ddata->ec_buf);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static void asus_ec_release_dockram_dev(void *client)
> +{
> + i2c_unregister_device(client);
> +}
> +
> +static struct i2c_client *devm_asus_dockram_get(struct device *dev)
> +{
> + struct i2c_client *parent = to_i2c_client(dev);
> + struct i2c_client *dockram;
> + struct dockram_ec_data *ddata;
> + int ret;
> +
> + dockram = i2c_new_ancillary_device(parent, "dockram",
> + parent->addr + 2);
Could we define a macro for the address offset '2' here to avoid using a magic
number?
> + if (IS_ERR(dockram))
> + return dockram;
> +
> + ret = devm_add_action_or_reset(dev, asus_ec_release_dockram_dev,
> + dockram);
> + if (ret)
> + return ERR_PTR(ret);
> +
> + ddata = devm_kzalloc(&dockram->dev, sizeof(*ddata), GFP_KERNEL);
> + if (!ddata)
> + return ERR_PTR(-ENOMEM);
> +
> + i2c_set_clientdata(dockram, ddata);
> + mutex_init(&ddata->ctl_lock);
> +
> + return dockram;
> +}
> +
> +static const struct mfd_cell asus_ec_sl101_dock_mfd_devices[] = {
> + MFD_CELL_NAME("asus-transformer-ec-kbc"),
> +};
> +
> +static const struct mfd_cell asus_ec_tf101_dock_mfd_devices[] = {
> + MFD_CELL_BASIC("asus-transformer-ec-battery", NULL, NULL, 0, 1),
> + MFD_CELL_BASIC("asus-transformer-ec-charger", NULL, NULL, 0, 1),
> + MFD_CELL_BASIC("asus-transformer-ec-led", NULL, NULL, 0, 1),
> + MFD_CELL_NAME("asus-transformer-ec-keys"),
> + MFD_CELL_NAME("asus-transformer-ec-kbc"),
> +};
> +
> +static const struct mfd_cell asus_ec_tf201_pad_mfd_devices[] = {
> + MFD_CELL_NAME("asus-transformer-ec-battery"),
> + MFD_CELL_NAME("asus-transformer-ec-led"),
> +};
> +
> +static const struct mfd_cell asus_ec_tf600t_pad_mfd_devices[] = {
> + MFD_CELL_NAME("asus-transformer-ec-battery"),
> + MFD_CELL_NAME("asus-transformer-ec-charger"),
> + MFD_CELL_NAME("asus-transformer-ec-led"),
> +};
> +
> +static int asus_ec_probe(struct i2c_client *client)
> +{
> + struct device *dev = &client->dev;
> + struct asus_ec_data *ddata;
> + const struct mfd_cell *cells;
> + unsigned int num_cells;
> + unsigned long irqflags;
> + int ret;
> +
> + if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_I2C_BLOCK))
> + return dev_err_probe(dev, -ENXIO,
> + "I2C bus is missing required SMBus block mode support\n");
> +
> + ddata = devm_kzalloc(dev, sizeof(*ddata), GFP_KERNEL);
> + if (!ddata)
> + return -ENOMEM;
> +
> + ddata->info = device_get_match_data(dev);
> + if (!ddata->info)
> + return -ENODEV;
> +
> + switch (ddata->info->variant) {
> + case ASUSEC_SL101_DOCK:
> + cells = asus_ec_sl101_dock_mfd_devices;
> + num_cells = ARRAY_SIZE(asus_ec_sl101_dock_mfd_devices);
> + break;
> + case ASUSEC_TF101_DOCK:
> + cells = asus_ec_tf101_dock_mfd_devices;
> + num_cells = ARRAY_SIZE(asus_ec_tf101_dock_mfd_devices);
> + break;
> + case ASUSEC_TF201_PAD:
> + cells = asus_ec_tf201_pad_mfd_devices;
> + num_cells = ARRAY_SIZE(asus_ec_tf201_pad_mfd_devices);
> + break;
> + case ASUSEC_TF600T_PAD:
> + cells = asus_ec_tf600t_pad_mfd_devices;
> + num_cells = ARRAY_SIZE(asus_ec_tf600t_pad_mfd_devices);
> + break;
> + default:
> + return dev_err_probe(dev, -EINVAL,
> + "unknown device variant %d\n",
> + ddata->info->variant);
> + }
> +
> + i2c_set_clientdata(client, ddata);
> + ddata->client = client;
> +
> + ddata->ec.dockram = devm_asus_dockram_get(dev);
> + if (IS_ERR(ddata->ec.dockram))
> + return dev_err_probe(dev, PTR_ERR(ddata->ec.dockram),
> + "failed to get dockram\n");
> +
> + ddata->ecreq_gpio = devm_gpiod_get(dev, "request", GPIOD_OUT_LOW);
> + if (IS_ERR(ddata->ecreq_gpio))
> + return dev_err_probe(dev, PTR_ERR(ddata->ecreq_gpio),
> + "failed to get EC request GPIO\n");
> +
> + BLOCKING_INIT_NOTIFIER_HEAD(&ddata->ec.notify_list);
> + mutex_init(&ddata->ecreq_lock);
> +
> + asus_ec_signal_request(ddata);
> +
> + ret = asus_ec_detect(ddata);
> + if (ret)
> + return dev_err_probe(dev, ret, "failed to detect EC version\n");
> +
> + /*
> + * Systems using device tree should set up interrupt via DTS,
> + * the rest will use the default low interrupt.
> + */
> + irqflags = dev->of_node ? 0 : IRQF_TRIGGER_LOW;
> +
> + ret = devm_request_threaded_irq(dev, client->irq, NULL,
> + &asus_ec_interrupt,
> + IRQF_ONESHOT | irqflags,
> + client->name, ddata);
> + if (ret)
> + return dev_err_probe(dev, ret, "failed to register IRQ\n");
> +
> + /* Parent I2C controller uses DMA, ASUS EC and child devices do not */
> + client->dev.coherent_dma_mask = 0;
> + client->dev.dma_mask = &client->dev.coherent_dma_mask;
> +
> + return devm_mfd_add_devices(dev, 0, cells, num_cells, NULL, 0, NULL);
> +}
> +
> +static const struct asus_ec_chip_info asus_ec_sl101_dock_data = {
> + .name = "dock",
> + .variant = ASUSEC_SL101_DOCK,
> + .fmode = ASUSEC_MODE_NONE,
> +};
> +
> +static const struct asus_ec_chip_info asus_ec_tf101_dock_data = {
> + .name = "dock",
> + .variant = ASUSEC_TF101_DOCK,
> + .fmode = ASUSEC_MODE_NONE,
> +};
> +
> +static const struct asus_ec_chip_info asus_ec_tf201_pad_data = {
> + .name = "pad",
> + .variant = ASUSEC_TF201_PAD,
> + .fmode = ASUSEC_MODE_NORMAL,
> +};
> +
> +static const struct asus_ec_chip_info asus_ec_tf600t_pad_data = {
> + .name = "pad",
> + .variant = ASUSEC_TF600T_PAD,
> + .fmode = ASUSEC_MODE_NORMAL,
> +};
Any reason not to just pass the identifier (variant) and add the name
and fmode attribues to the switch() above?
> +
> +static const struct of_device_id asus_ec_match[] = {
> + {
> + .compatible = "asus,sl101-ec-dock",
> + .data = &asus_ec_sl101_dock_data
> + }, {
> + .compatible = "asus,tf101-ec-dock",
> + .data = &asus_ec_tf101_dock_data
> + }, {
> + .compatible = "asus,tf201-ec-pad",
> + .data = &asus_ec_tf201_pad_data
> + }, {
> + .compatible = "asus,tf600t-ec-pad",
> + .data = &asus_ec_tf600t_pad_data
> + },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, asus_ec_match);
> +
> +static struct i2c_driver asus_ec_driver = {
> + .driver = {
> + .name = "asus-transformer-ec",
> + .of_match_table = asus_ec_match,
> + },
> + .probe = asus_ec_probe,
> +};
> +module_i2c_driver(asus_ec_driver);
> +
> +MODULE_AUTHOR("Michał Mirosław <mirq-linux@rere.qmqm.pl>");
> +MODULE_AUTHOR("Svyatoslav Ryhel <clamor95@gmail.com>");
> +MODULE_DESCRIPTION("ASUS Transformer's EC driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/mfd/asus-transformer-ec.h b/include/linux/mfd/asus-transformer-ec.h
> new file mode 100644
> index 000000000000..f085eea2193e
> --- /dev/null
> +++ b/include/linux/mfd/asus-transformer-ec.h
> @@ -0,0 +1,92 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#ifndef __MFD_ASUS_TRANSFORMER_EC_H
> +#define __MFD_ASUS_TRANSFORMER_EC_H
> +
> +#include <linux/notifier.h>
> +#include <linux/platform_device.h>
> +
> +struct i2c_client;
> +
> +/**
> + * struct asusec_core - public part shared with all cells
> + *
> + * @model: firmware version running on the EC
> + * @name: prefix associated with the EC
> + * @dockram: pointer to Dockram's i2c_client
> + * @notify_list: notify list used by cells
> + */
> +struct asusec_core {
> + const char *model;
> + const char *name;
> + struct i2c_client *dockram;
> + struct blocking_notifier_head notify_list;
> +};
> +
> +#define ASUSEC_ENTRIES 0x100
> +#define ASUSEC_ENTRY_SIZE 32
> +#define ASUSEC_ENTRY_BUFSIZE (ASUSEC_ENTRY_SIZE + 1)
> +
> +/* interrupt sources */
> +#define ASUSEC_IRQ_STATUS 1
> +#define ASUSEC_OBF_MASK BIT(0)
> +#define ASUSEC_KEY_MASK BIT(2)
> +#define ASUSEC_KBC_MASK BIT(3)
> +#define ASUSEC_AUX_MASK BIT(5)
> +#define ASUSEC_SCI_MASK BIT(6)
> +#define ASUSEC_SMI_MASK BIT(7)
> +
> +/* SMI notification codes */
> +#define ASUSEC_SMI_CODE 2
> +#define ASUSEC_SMI_POWER_NOTIFY 0x31 /* USB cable plug event */
> +#define ASUSEC_SMI_HANDSHAKE 0x50 /* response to ec_req edge */
> +#define ASUSEC_SMI_WAKE 0x53
> +#define ASUSEC_SMI_RESET 0x5f
> +#define ASUSEC_SMI_ADAPTER_EVENT 0x60 /* charger to dock plug event */
> +#define ASUSEC_SMI_BACKLIGHT_ON 0x63
> +#define ASUSEC_SMI_AUDIO_DOCK_IN 0x70
> +
> +#define ASUSEC_SMI_ACTION(code) (ASUSEC_SMI_MASK | ASUSEC_OBF_MASK | \
> + (ASUSEC_SMI_##code << 8))
> +
> +/* control register [0x0a] layout */
> +#define ASUSEC_CTL_SIZE 8
> +
> +/*
> + * EC reports power from 40-pin connector in the LSB of the control
> + * register. The following values have been observed (xor 0x02):
> + *
> + * PAD-ec no-plug 0x40 / PAD-ec DOCK 0x20 / DOCK-ec no-plug 0x40
> + * PAD-ec AC 0x25 / PAD-ec DOCK+AC 0x24 / DOCK-ec AC 0x25
> + * PAD-ec USB 0x45 / PAD-ec DOCK+USB 0x24 / DOCK-ec USB 0x41
> + */
> +
> +#define ASUSEC_CTL_DIRECT_POWER_SOURCE BIT_ULL(0)
> +#define ASUSEC_STAT_CHARGING BIT_ULL(2)
> +#define ASUSEC_CTL_FULL_POWER_SOURCE BIT_ULL(5)
> +#define ASUSEC_CTL_SUSB_MODE BIT_ULL(9)
> +#define ASUSEC_CMD_SUSPEND_S3 BIT_ULL(33)
> +#define ASUSEC_CTL_TEST_DISCHARGE BIT_ULL(35)
> +#define ASUSEC_CMD_SUSPEND_INHIBIT BIT_ULL(37)
> +#define ASUSEC_CTL_FACTORY_MODE BIT_ULL(38)
> +#define ASUSEC_CTL_KEEP_AWAKE BIT_ULL(39)
> +#define ASUSEC_CTL_USB_CHARGE BIT_ULL(40)
> +#define ASUSEC_CTL_LED_BLINK BIT_ULL(40)
> +#define ASUSEC_CTL_LED_AMBER BIT_ULL(41)
> +#define ASUSEC_CTL_LED_GREEN BIT_ULL(42)
> +#define ASUSEC_CMD_SWITCH_HDMI BIT_ULL(56)
> +#define ASUSEC_CMD_WIN_SHUTDOWN BIT_ULL(62)
> +
> +#define ASUSEC_DOCKRAM_INFO_MODEL 0x01
> +#define ASUSEC_DOCKRAM_INFO_FW 0x02
> +#define ASUSEC_DOCKRAM_INFO_CFGFMT 0x03
> +#define ASUSEC_DOCKRAM_INFO_HW 0x04
> +#define ASUSEC_DOCKRAM_CONTROL 0x0a
> +#define ASUSEC_DOCKRAM_BATT_CTL 0x14
> +
> +#define ASUSEC_WRITE_BUF 0x64
> +#define ASUSEC_READ_BUF 0x6a
> +
> +int asus_dockram_access_ctl(struct i2c_client *client,
> + u64 *out, u64 mask, u64 xor);
> +
> +#endif /* __MFD_ASUS_TRANSFORMER_EC_H */
> --
> 2.51.0
>
--
Lee Jones
^ permalink raw reply
* Re: [PATCH] leds: tests: clarify name conflict test It is a good practice to use a fresh led_classdev instance when doing the testing. LED_REJECT_NAME_CONFLICT rather than re-registering the already registered LED class device. Besides that, do a zero initialization of led_lookup_data.
From: Pavel Machek @ 2026-06-10 23:04 UTC (permalink / raw)
To: Lorenzo Egidio; +Cc: lee, linux-leds
In-Reply-To: <20260610172107.507-1-lorenzoegidioms@gmail.com>
> Signed-off-by: Lorenzo Egidio <lorenzoegidioms@gmail.com>
> ---
> drivers/leds/led-test.c | 102 ++++++++++++++++++++++++++--------------
> 1 file changed, 66 insertions(+), 36 deletions(-)
NAK.
Pavel
^ permalink raw reply
* [PATCH] leds: as3668: correct name of config option to match Makefile
From: Ethan Nelson-Moore @ 2026-06-10 22:42 UTC (permalink / raw)
To: Lee Jones, Lukas Timmermann, linux-leds
Cc: Ethan Nelson-Moore, stable, Pavel Machek
The Makefile for the AS3668 LED driver refers to CONFIG_LEDS_AS3668,
whereas the config file defines CONFIG_LEDS_OSRAM_AMS_AS3668. This
causes the driver to never be compiled. Correct the name in the Kconfig
file to match the Makefile. Doing the opposite would also have worked,
but the name in the Makefile better matches the format of other
drivers' options.
Fixes: c7dd343a3756 ("leds: as3668: Driver for the ams Osram 4-channel i2c LED driver")
Cc: stable@vger.kernel.org # 7.0+
Signed-off-by: Ethan Nelson-Moore <enelsonmoore@gmail.com>
---
drivers/leds/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index f4a0a3c8c870..5ac63cb59469 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -107,7 +107,7 @@ config LEDS_ARIEL
Say Y to if your machine is a Dell Wyse 3020 thin client.
-config LEDS_OSRAM_AMS_AS3668
+config LEDS_AS3668
tristate "LED support for Osram AMS AS3668"
depends on LEDS_CLASS
depends on I2C
--
2.43.0
^ permalink raw reply related
* Re: [PATCH v4 14/14] video: leds: backlight: lm3533: Support getting LED sources from DT
From: Andy Shevchenko @ 2026-06-10 18:54 UTC (permalink / raw)
To: Svyatoslav Ryhel
Cc: Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Helge Deller,
Johan Hovold, dri-devel, linux-leds, devicetree, linux-kernel,
linux-iio, linux-fbdev
In-Reply-To: <CAPVz0n0m+i7wexfC0BWwvdOuJxDk_=x+EpxEbNmoHN1AsEmfNg@mail.gmail.com>
On Wed, Jun 10, 2026 at 05:45:28PM +0300, Svyatoslav Ryhel wrote:
> вт, 9 черв. 2026 р. о 22:23 Andy Shevchenko <andriy.shevchenko@intel.com> пише:
> > On Sat, Jun 06, 2026 at 07:57:38AM +0300, Svyatoslav Ryhel wrote:
> > > Add Control Bank to HVLED/LVLED muxing support based on the led-sources
> > > defined in the device tree.
...
> > > + int ret, i;
> >
> > No need to add 'i'.
>
> This is personal preference as well. There is no strict rule that
> iteration variable must be defined strictly in the for loop.
This is a preference by Linus who is the leader of the project.
Also in IIO we have some set of maintainer preferences.
> > > + for (i = 0; i < led->num_leds; i++) {
> >
> > for (unsigned int i = 0; i < led->num_leds; i++) {
> >
> > > + if (led->leds[i] >= LM3533_LVCTRLBANK_MAX)
> > > + continue;
> > > +
> > > + output_cfg_shift = led->leds[i] * 2;
> > > + output_cfg_val |= led->id << output_cfg_shift;
> > > + output_cfg_mask |= OUTPUT_LVLED_MASK << output_cfg_shift;
> > > + }
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v4 10/14] mfd: lm3533: Set DMA mask
From: Andy Shevchenko @ 2026-06-10 18:51 UTC (permalink / raw)
To: Svyatoslav Ryhel
Cc: Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Helge Deller,
Johan Hovold, dri-devel, linux-leds, devicetree, linux-kernel,
linux-iio, linux-fbdev
In-Reply-To: <CAPVz0n01ymfYGH+_MgfHvWqzg+tczLi8E-1q=rZ5cHu+uJrpdg@mail.gmail.com>
On Wed, Jun 10, 2026 at 05:40:56PM +0300, Svyatoslav Ryhel wrote:
> вт, 9 черв. 2026 р. о 22:17 Andy Shevchenko <andriy.shevchenko@intel.com> пише:
> > On Sat, Jun 06, 2026 at 07:57:34AM +0300, Svyatoslav Ryhel wrote:
> > > Missing coherent_dma_mask assigning triggers the following warning in
> > > dmesg:
> > >
> > > [ 3.287872] platform lm3533-backlight.0: DMA mask not set
> > >
> > > Since this warning might be elevated to an error in the future, set
> > > coherent_dma_mask to zero because both the core and cells do not utilize
> > > DMA.
> >
> > Hmm... I am not sure about this. The entire kernel has only two drivers that
> > do that, and thanks to their commit messages one of them pointed out to the
> > commit from 2018. So, if no other devices suffer from this, I think it has to
> > be a better way of achieving the same.
>
> If mfd framework warns that DMA mask is not set then this must be
> addressed. Why then there is such warning at the first place if mask
> can be just skipped. Then warning would be just a debug message. What
> is warning today can become error tomorrow.
It's there for 8 years. And only a few drivers explicitly nullify it.
I think doing this patch one should really understand what's going on.
(Note, I'm not that person, it's my gut feelings based on the experience
and the above statistics.) MFD creates a lot of platform devices for
which DMA mask is set automatically. So, why is it not set properly for
this device?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v4 07/14] mfd: lm3533: Switch sysfs_create_group() to device_add_group()
From: Andy Shevchenko @ 2026-06-10 18:47 UTC (permalink / raw)
To: Svyatoslav Ryhel
Cc: Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Helge Deller,
Johan Hovold, dri-devel, linux-leds, devicetree, linux-kernel,
linux-iio, linux-fbdev
In-Reply-To: <CAPVz0n2+27QVeWNgPm3PH6V2Ceuym6sbMtUrh7hSHe9PcRmfMA@mail.gmail.com>
On Wed, Jun 10, 2026 at 05:38:38PM +0300, Svyatoslav Ryhel wrote:
> вт, 9 черв. 2026 р. о 22:14 Andy Shevchenko <andriy.shevchenko@intel.com> пише:
> > On Sat, Jun 06, 2026 at 07:57:31AM +0300, Svyatoslav Ryhel wrote:
> > > Switch from sysfs_create_group() to device_add_group() including device
> > > managed where appropriate.
> >
> > This should use .dev_groups member of struct device_driver.
>
> Specify pls, device_add_group literally uses dev_groups, I don't
> understand what is wrong.
dev_groups of the struct device_driver. It means that the data should be
static and be available before driver probe is called.
...
> > > + ret = devm_device_add_group(&bd->dev, &lm3533_bl_attribute_group);
> >
> > This will make Greg KH very grumpy. (For the record, original code as well
> > but it already is in upstream. So, thanks for trying to address this, just
> > needs a bit more of work.)
>
> In the prev iteration YOU asked to me to adjust this. I have adjusted
> and now you say that this is not appropriate. I will just drop this
> commit altogether.
Yes, and I still tell that this is the way to fix that issue.
You can even do it yourself in a few clicks (hint: `git log --grep` is the tool
of the day): 93afe8ba9b01 ("ACPI: TAD: Use dev_groups in struct device_driver").
This is an example of what I meant.
> > > + if (ret < 0)
> > > + return dev_err_probe(&pdev->dev, ret,
> > > + "failed to create sysfs attributes\n");
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
page: next (older)
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox