* 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 v2 0/3] pinctrl: sunxi: a523: fix GPIO IRQ operation
From: Linus Walleij @ 2026-06-11 11:49 UTC (permalink / raw)
To: Andre Przywara
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Chen-Yu Tsai,
Jernej Skrabec, Samuel Holland, linux-gpio, devicetree,
linux-arm-kernel, linux-sunxi, linux-kernel
In-Reply-To: <20260327113006.3135663-1-andre.przywara@arm.com>
On Fri, Mar 27, 2026 at 12:30 PM Andre Przywara <andre.przywara@arm.com> wrote:
> this is the minimal fix version for the GPIO IRQ operation on the
> Allwinner A523/A527/T527 SoCs. SD card detection is broken as a result,
> which is a major annoyance. Those patches here fix that problem, and
> should go into v7.0 still, if possible.
Patches 1 & 2 applied to the pinctrl tree, please send patch 3 to
the SoC tree.
Sorry for missing this, dunno what happened. Probably it got
lost by me trying to use korgalore and screwing up.
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH v8 1/3] dt-bindings: timer: mips,p8700-gcru
From: Aleksa Paunovic @ 2026-06-11 11:51 UTC (permalink / raw)
To: krzk@kernel.org
Cc: Djordje Todorovic, Aleksa Paunovic, alex@ghiti.fr,
aou@eecs.berkeley.edu, cfu@mips.com, conor+dt@kernel.org,
conor.dooley@microchip.com, daniel.lezcano@linaro.org,
devicetree@vger.kernel.org, jstultz@google.com,
krzk+dt@kernel.org, linux-kernel@vger.kernel.org,
linux-riscv@lists.infradead.org, palmer@dabbelt.com,
paul.walmsley@sifive.com, pjw@kernel.org, robh@kernel.org,
sboyd@kernel.org, tglx@linutronix.de, wangruikang@iscas.ac.cn
In-Reply-To: <a2b5c9b4-5fc0-4507-a221-8695ed9574d8@kernel.org>
On 6/11/26 09:50, Krzysztof Kozlowski wrote:
> On 11/06/2026 09:39, Aleksa Paunovic wrote:
>> Hi Krzysztof,
>>
>>
>> On 6/11/26 08:54, Krzysztof Kozlowski wrote:
>>> On 11/06/2026 08:51, Krzysztof Kozlowski wrote:
>>>> On 10/06/2026 10:22, Aleksa Paunovic via B4 Relay wrote:
>>>>> From: Aleksa Paunovic <aleksa.paunovic@htecgroup.com>
>>>>>
>>>>> Add dt-bindings for the GCR.U memory mapped timer device for RISC-V
>>>>> platforms. The GCR.U memory region contains shadow copies of the RISC-V
>>>>> mtime register and the hrtime Global Configuration Register.
>>>>>
>>>>> Signed-off-by: Aleksa Paunovic <aleksa.paunovic@htecgroup.com>
>>>> You keep ignoring reviews you received (14th May!) and sending same mistake.
>>>>
>>>> Can you address the emails?
>> I wasn't really sure what the etiquette was for replying to Sashiko reviews, so I decided to
>> address the comments for other patches and send a v8 without replying.
>>
>> As for this patch, the GCR.U itself does start at 0x7F000, but the first
>> actual register (mtime) is at 0x7F050 [1].
>> I'm not seeing any warnings when running dt_binding_check.
> It's still a warning which you can easily reproduce on W=1 on dts. You
> CANNOT have mismatch. If block starts at 0x7f000, first register CANNOT
> start at different address or it completely does not matter where the
> register is. It's contradictory. The block start address defines
> where... does it start.
You are right: I just checked by manually running dtc and the warning's clearly there.
Will fix the alignment in v9. Thanks!
Best regards,
Aleksa
^ permalink raw reply
* Re: [PATCH v3 4/4] net: phy: at803x: add RX and TX clock management for IPQ5018 PHY
From: Konrad Dybcio @ 2026-06-11 11:54 UTC (permalink / raw)
To: Andrew Lunn, George Moussalem, Kathiravan Thirumoorthy
Cc: Heiner Kallweit, Russell King, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Florian Fainelli, Bjorn Andersson, Konrad Dybcio,
netdev, devicetree, linux-kernel, linux-arm-msm
In-Reply-To: <afdced5b-73b9-4214-a94a-c13fadd39dce@lunn.ch>
On 6/5/26 8:14 PM, Andrew Lunn wrote:
>>>> This PHY is integrated into the IPQ5018 SoC, connected to the first GMAC
>>>> (GMAC0) and probed upon boot. However, this PHY is not used on all
>>>> boards because an external PHY or switch can be wired to the SoC's
>>>> second GMAC instead (through a PCS). So from a power management
>>>> perspective, it would be better if we can disable the clocks if there's
>>>> no link detected.
>>>
>>> Humm, is link the correct criteria? If the PHY is not used,
>>> .config_aneg should not be called. Why not have the probe method get
>>> the optional clocks, but leave them off. When .config_aneg is called
>>> for the first time, enable the clocks?
>>
>> Will check if config_aneg is called and test accordingly.
>>
>> ip link set eth0 up/down and cable (un)plug do trigger
>> link_change_notify, and based on the link state the RX/TX clocks are
>> turned off/on properly.
>
> You are talking about something else here. You say the device is not
> used. If it is not used, .config_aneg should not be called.
>
> This is a second use case, the device is used, and you want to limit
> the power it consumes, when there is no link. Do you have any numbers?
> How much power is actually saved?
If the PHY is part of the SoC, keeping it online would require some
more hw to be online, so it probably sums up.. I don't know for sure,
I don't really work with the router SoCs
Is there any prior art wrt enabling/disabling the PHYs (not necessarily
clocks specifically, but say power supplies) at runtime?
A quick grep only points to this very driver, which gets the regulator
during probe, enables it and never turns it off
Maybe +Kathiravan knows more?
Konrad
^ permalink raw reply
* RE: [PATCH v3 1/5] dt-bindings: soc: cix,sky1-system-control: add audss system control
From: Joakim Zhang @ 2026-06-11 11:56 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: mturquette@baylibre.com, sboyd@kernel.org, bmasney@redhat.com,
robh@kernel.org, krzk+dt@kernel.org, conor+dt@kernel.org,
p.zabel@pengutronix.de, Gary Yang, cix-kernel-upstream,
linux-clk@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <20260611-gorgeous-macho-cricket-f1b78c@quoll>
Hi,
> -----Original Message-----
> From: Krzysztof Kozlowski <krzk@kernel.org>
> Sent: Thursday, June 11, 2026 3:40 PM
> To: Joakim Zhang <joakim.zhang@cixtech.com>
> Cc: mturquette@baylibre.com; sboyd@kernel.org; bmasney@redhat.com;
> robh@kernel.org; krzk+dt@kernel.org; conor+dt@kernel.org;
> p.zabel@pengutronix.de; Gary Yang <gary.yang@cixtech.com>; cix-kernel-
> upstream <cix-kernel-upstream@cixtech.com>; linux-clk@vger.kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org
> Subject: Re: [PATCH v3 1/5] dt-bindings: soc: cix,sky1-system-control: add audss
> system control
>
> EXTERNAL EMAIL
>
> On Wed, Jun 10, 2026 at 03:56:41PM +0800, joakim.zhang@cixtech.com wrote:
> > From: Joakim Zhang <joakim.zhang@cixtech.com>
> >
> > The Cix Sky1 Audio Subsystem (AUDSS) groups audio-related clock, reset
> > and control registers in a dedicated CRU block. Software reset lines
> > are exposed on the syscon parent via #reset-cells, following the same
> > model as the existing Sky1 FCH and S5 system control bindings.
> >
> > Add the cix,sky1-audss-system-control compatible to
> > cix,sky1-system-control.yaml for the MFD/syscon parent node, and
> > define AUDSS software reset indices in
> > include/dt-bindings/reset/cix,sky1-audss-system-control.h for I2S,
> > HDA, DMAC, mailbox, watchdog and timer blocks.
>
> All this is pretty pointless - you explained the binding, which answers nothing
> why you did it that way. Instead you must explain the hardware design.
>
> >
> > Signed-off-by: Joakim Zhang <joakim.zhang@cixtech.com>
> > ---
> > .../soc/cix/cix,sky1-system-control.yaml | 52 +++++++++++++++++--
> > .../reset/cix,sky1-audss-system-control.h | 25 +++++++++
> > 2 files changed, 72 insertions(+), 5 deletions(-) create mode 100644
> > include/dt-bindings/reset/cix,sky1-audss-system-control.h
> >
> > diff --git
> > a/Documentation/devicetree/bindings/soc/cix/cix,sky1-system-control.ya
> > ml
> > b/Documentation/devicetree/bindings/soc/cix/cix,sky1-system-control.ya
> > ml index a01a515222c6..61d26a69fd44 100644
> > ---
> > a/Documentation/devicetree/bindings/soc/cix/cix,sky1-system-control.ya
> > ml
> > +++ b/Documentation/devicetree/bindings/soc/cix/cix,sky1-system-contro
> > +++ l.yaml
> > @@ -15,11 +15,16 @@ description:
> >
> > properties:
> > compatible:
> > - items:
> > - - enum:
> > - - cix,sky1-system-control
> > - - cix,sky1-s5-system-control
> > - - const: syscon
> > + oneOf:
> > + - items:
> > + - enum:
> > + - cix,sky1-system-control
> > + - cix,sky1-s5-system-control
> > + - const: syscon
> > + - items:
> > + - const: cix,sky1-audss-system-control
> > + - const: simple-mfd
>
> Just so you are aware - this means children do not depend on the parent for
> operation. You will not be able to fix it later, if it turns out that children do
> depend...
Understood. simple-mfd is intentional: the clock child only accesses the parent MMIO via syscon and external resets/clocks via phandles. No parent driver coordination is needed today. We attached all resources audss needed from child node now.
> > + - const: syscon
> >
> > reg:
> > maxItems: 1
> > @@ -27,6 +32,28 @@ properties:
> > '#reset-cells':
> > const: 1
> >
> > + clock-controller:
> > + type: object
> > + properties:
> > + compatible:
> > + const: cix,sky1-audss-clock
> > + required:
> > + - compatible
> > + additionalProperties: true
> > +
> > +allOf:
> > + - if:
> > + properties:
> > + compatible:
> > + contains:
> > + const: cix,sky1-audss-system-control
> > + then:
> > + required:
> > + - clock-controller
> > + else:
> > + properties:
> > + clock-controller: false
> > +
> > required:
> > - compatible
> > - reg
> > @@ -40,3 +67,18 @@ examples:
> > reg = <0x4160000 0x100>;
> > #reset-cells = <1>;
> > };
> > + - |
> > + audss_syscon: system-controller@7110000 {
> > + compatible = "cix,sky1-audss-system-control", "simple-mfd", "syscon";
> > + reg = <0x7110000 0x10000>;
> > + #reset-cells = <1>;
> > +
> > + clock-controller {
> > + compatible = "cix,sky1-audss-clock";
> > + power-domains = <&smc_devpd 0>;
>
> My questions from v2 from the other patch are still valid - why audss system
> clock controller is outside of the power domain? Why the audss reset is outside,
> but audss clock not?
>
> This does not feel like correct hardware representation.
Yes, I agree with your point. This does not really reflect the hardware well. Both noc reset and power-domain takes effect on audss, should move to parent node.
Thanks,
Joakim
^ permalink raw reply
* RE: [PATCH v2 3/5] dt-bindings: clock: cix,sky1-audss-clock: add audss clock controller
From: Joakim Zhang @ 2026-06-11 11:57 UTC (permalink / raw)
To: Krzysztof Kozlowski, mturquette@baylibre.com, sboyd@kernel.org,
bmasney@redhat.com, robh@kernel.org, krzk+dt@kernel.org,
conor+dt@kernel.org, p.zabel@pengutronix.de, Gary Yang
Cc: cix-kernel-upstream, linux-clk@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <992261bb-6e2e-4662-96f2-c5b18d513b32@kernel.org>
> -----Original Message-----
> From: Krzysztof Kozlowski <krzk@kernel.org>
> Sent: Thursday, June 11, 2026 3:41 PM
> To: Joakim Zhang <joakim.zhang@cixtech.com>; mturquette@baylibre.com;
> sboyd@kernel.org; bmasney@redhat.com; robh@kernel.org;
> krzk+dt@kernel.org; conor+dt@kernel.org; p.zabel@pengutronix.de; Gary Yang
> <Gary.Yang@cixtech.com>
> Cc: cix-kernel-upstream <cix-kernel-upstream@cixtech.com>; linux-
> clk@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH v2 3/5] dt-bindings: clock: cix,sky1-audss-clock: add audss
> clock controller
>
> EXTERNAL EMAIL
>
> On 09/06/2026 08:27, Joakim Zhang wrote:
> >
> > Hi Krzysztof,
> >
> >> -----Original Message-----
> >> From: Krzysztof Kozlowski <krzk@kernel.org>
> >> Sent: Friday, June 5, 2026 5:24 PM
> >> To: Joakim Zhang <joakim.zhang@cixtech.com>; mturquette@baylibre.com;
> >> sboyd@kernel.org; bmasney@redhat.com; robh@kernel.org;
> >> krzk+dt@kernel.org; conor+dt@kernel.org; p.zabel@pengutronix.de; Gary
> >> krzk+Yang
> >> <gary.yang@cixtech.com>
> >> Cc: cix-kernel-upstream <cix-kernel-upstream@cixtech.com>; linux-
> >> clk@vger.kernel.org; devicetree@vger.kernel.org;
> >> linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org
> >> Subject: Re: [PATCH v2 3/5] dt-bindings: clock: cix,sky1-audss-clock:
> >> add audss clock controller
> >>
> >> EXTERNAL EMAIL
> >>
> >> On 05/06/2026 05:22, joakim.zhang@cixtech.com wrote:
> >>> +description: |
> >>> + Clock provider for the Cix Sky1 audio subsystem (AUDSS).
> >>> +
> >>> + This node is a child of a cix,sky1-audss-system-control
> >>> + MFD/syscon node (see cix,sky1-system-control.yaml). It does not
> >>> + have a reg property; clock mux, divider and gate fields are
> >>> + accessed through the parent
> >> register block.
> >>> +
> >>> + Software reset lines for AUDSS blocks are exposed on the parent
> >>> + syscon via #reset-cells. Reset indices are defined in
> >>> + include/dt-bindings/reset/cix,sky1-audss-system-control.h.
> >>> +
> >>> + Six SoC-level reference clocks listed in clocks/clock-names feed
> >>> + the AUDSS clock tree. The provider exposes the internal AUDSS
> >>> + clocks to other devices via #clock-cells; indices are defined in
> >>> + cix,sky1-
> >> audss.h.
> >>> +
> >>> +properties:
> >>> + compatible:
> >>> + const: cix,sky1-audss-clock
> >>> +
> >>> + '#clock-cells':
> >>> + const: 1
> >>> + description:
> >>> + Clock indices are defined in include/dt-bindings/clock/cix,sky1-audss.h.
> >>> +
> >>> + clocks:
> >>> + minItems: 6
> >>
> >> Drop
> > OK
> >
> >>> + maxItems: 6
> >>> + description:
> >>> + Six SoC-level audio reference clocks that feed the audio subsystem,
> >>> + in the same order as clock-names.
> >>> +
> >>> + clock-names:
> >>> + items:
> >>> + - const: audio_clk0
> >>> + - const: audio_clk1
> >>> + - const: audio_clk2
> >>> + - const: audio_clk3
> >>> + - const: audio_clk4
> >>> + - const: audio_clk5
> >>
> >> Pretty pointless names. Names matching indexes have no benefits, drop
> >> all of them and instead list items in "clocks" with description.
> > Yes, you are right, I will describe these more meaningful.
> >
> >>> +
> >>> + resets:
> >>> + maxItems: 1
> >>> + description: Audio subsystem NoC (or bus) reset line.
> >>> +
> >>> + power-domains:
> >>> + maxItems: 1
> >>> + description: Audio subsystem power domain.
> >>
> >> So the clock part has power domain but reset part does not? This is odd.
> >> Especially that parent is audss (right?) and here you describe that
> >> this is audss poer domain.
> >>
> >> Same question about resets.
> >
> > The reset and power domain takes effect on the entire subsystem, i.e., audss
> can be accessed only after powered on and reset released, including the CRU
> registers which contains clock/reset/control bits for all device within the audss.
> >
> > Because the reset controller probe does not access the hardware, while the
> clock controller does, so at that time, the power domain and reset were placed
> in the clock driver. At present, it does not seem very reasonable either.
> >
> > Linking the "reset" and "power domain" to the parent node requires us to
> ensure the order of the probes. We need to perform deferred probes within the
> child nodes until the parent node has been probed.
> >
>
> Please wrap your replies.
>
> You refer here to probe, so driver design, but I did not ask about that.
> I asked about hardware design.
Just as you understand, power domain and noc reset for the whole audss.
Thanks,
Joakim
^ permalink raw reply
* RE: [PATCH v3 3/5] dt-bindings: clock: cix,sky1-audss-clock: add audss clock controller
From: Joakim Zhang @ 2026-06-11 11:57 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: mturquette@baylibre.com, sboyd@kernel.org, bmasney@redhat.com,
robh@kernel.org, krzk+dt@kernel.org, conor+dt@kernel.org,
p.zabel@pengutronix.de, Gary Yang, cix-kernel-upstream,
linux-clk@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <20260611-numbat-of-unmistakable-excitement-f6cfed@quoll>
Hi,
[...]
> -----Original Message-----
> From: Krzysztof Kozlowski <krzk@kernel.org>
> Sent: Thursday, June 11, 2026 3:42 PM
> To: Joakim Zhang <joakim.zhang@cixtech.com>
> Cc: mturquette@baylibre.com; sboyd@kernel.org; bmasney@redhat.com;
> robh@kernel.org; krzk+dt@kernel.org; conor+dt@kernel.org;
> p.zabel@pengutronix.de; Gary Yang <gary.yang@cixtech.com>; cix-kernel-
> upstream <cix-kernel-upstream@cixtech.com>; linux-clk@vger.kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org
> Subject: Re: [PATCH v3 3/5] dt-bindings: clock: cix,sky1-audss-clock: add audss
> clock controller
>
> EXTERNAL EMAIL
>
> > diff --git a/include/dt-bindings/clock/cix,sky1-audss.h b/include/dt-
> bindings/clock/cix,sky1-audss.h
> > new file mode 100644
> > index 000000000000..033046407dee
> > --- /dev/null
> > +++ b/include/dt-bindings/clock/cix,sky1-audss.h
>
> Filename must match the compatible.
Will rename to include/dt-bindings/clock/cix,sky1-audss-clock.h to match the compatible.
Thanks,
Joakim
^ permalink raw reply
* Re: [PATCH v5 4/8] dt-bindings: can: fsl,flexcan: add NXP S32N79 SoC support
From: Ciprian Marian Costea @ 2026-06-11 11:57 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Marc Kleine-Budde, Vincent Mailhol, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Frank Li, Sascha Hauer,
Fabio Estevam, Pengutronix Kernel Team, linux-can, devicetree,
linux-kernel, imx, linux-arm-kernel, NXP S32 Linux Team,
Christophe Lizzi, Alberto Ruiz, Enric Balletbo, Eric Chanudet,
Andra-Teodora Ilie, Larisa Grigore, Conor Dooley, Haibo Chen
In-Reply-To: <20260610-crouching-wild-mushroom-c8bf6a@quoll>
On 6/10/2026 9:37 AM, Krzysztof Kozlowski wrote:
> On Tue, Jun 09, 2026 at 04:29:50PM +0200, Ciprian Costea wrote:
>> From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
>>
>> Add NXP S32N79 SoC compatible string and interrupt properties.
>>
>> On S32N79, FlexCAN IP is integrated with two interrupt lines:
>> one for the mailbox interrupts (0-127) and one for signaling
>> bus errors and device state changes.
>>
>> Co-developed-by: Andra-Teodora Ilie <andra.ilie@nxp.com>
>> Signed-off-by: Andra-Teodora Ilie <andra.ilie@nxp.com>
>> Co-developed-by: Larisa Grigore <larisa.grigore@nxp.com>
>> Signed-off-by: Larisa Grigore <larisa.grigore@nxp.com>
>> Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
>> Acked-by: Conor Dooley <conor.dooley@microchip.com>
>> Reviewed-and-tested-by: Haibo Chen <haibo.chen@nxp.com>
>
> You cannot test a binding (in a meaning what "testing" means). Building
> code is not testing.
>
>> Tested-by: Enric Balletbo i Serra <eballetb@redhat.com>
>
> Not possible.
>
> Best regards,
> Krzysztof
>
Hello Krzysztof,
Yes, my bad. It makes total sense.
I presume I shouldn't send a new version just for this 'Tested-by'
removal - right ?
Best Regards,
Ciprian
^ permalink raw reply
* Re: [PATCH 3/4] clk: qcom: Add EVA clock controller driver for Glymur SoC
From: Konrad Dybcio @ 2026-06-11 12:00 UTC (permalink / raw)
To: Taniya Das, Bjorn Andersson, Michael Turquette, Stephen Boyd,
Brian Masney, Dmitry Baryshkov, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Konrad Dybcio
Cc: Ajit Pandey, Imran Shaik, Jagadeesh Kona, linux-arm-msm,
linux-clk, linux-kernel, devicetree
In-Reply-To: <20260526-evacc_glymur-v1-3-b61c7755c403@oss.qualcomm.com>
On 5/26/26 7:29 AM, Taniya Das wrote:
> Add the Enhanced Video Analytics (EVA) clock controller driver for
> the Glymur SoC. The EVACC manages the PLL, RCGs, branch clocks, GDSCs
> and resets for the EVA subsystem which handles vision processing
> workloads.
>
> Signed-off-by: Taniya Das <taniya.das@oss.qualcomm.com>
> ---
[...]
> +enum {
> + DT_AHB_CLK,
> + DT_BI_TCXO,
> + DT_BI_TCXO_AO,
DT_BI_TCXO_AO is unused, will it ever be?
[...]
> +static void clk_glymur_regs_configure(struct device *dev, struct regmap *regmap)
> +{
> + /* Update CTRL_IN register */
Is there any better comment we could share here?
> + regmap_update_bits(regmap, 0x9f24, BIT(0), BIT(0));
regmap_set_bits()
otherwise lgtm
Konrad
^ permalink raw reply
* Re: [PATCH 1/4] clk: qcom: gcc-glymur: Move EVA clocks to critical clock list
From: Konrad Dybcio @ 2026-06-11 12:01 UTC (permalink / raw)
To: Dmitry Baryshkov, Taniya Das
Cc: Bjorn Andersson, Michael Turquette, Stephen Boyd, Brian Masney,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Konrad Dybcio,
Ajit Pandey, Imran Shaik, Jagadeesh Kona, linux-arm-msm,
linux-clk, linux-kernel, devicetree
In-Reply-To: <61353034-51a8-4be9-8f7f-b15f5d60fcb3@oss.qualcomm.com>
On 5/26/26 9:12 AM, Dmitry Baryshkov wrote:
> On 26/05/2026 09:07, Taniya Das wrote:
>>
>>
>> On 5/26/2026 11:16 AM, Dmitry Baryshkov wrote:
>>> On Tue, May 26, 2026 at 10:59:44AM +0530, Taniya Das wrote:
>>>> The gcc_eva_ahb_clk and gcc_eva_xo_clk branch clocks should not be
>>>> registered as standalone GCC branch clocks.
>>>
>>> , otherwise .... what?
>>
>> If registered as normal branch clocks, they may be gated, which
>> breaks access to the EVA clock controller during clock controller probe.
>
> At least for the gcc_eva_ahb_clk I'd expect platforms actually reference that clock (as well as they do for GCC_VIDEO_AHB_CLK). For the XO clk it's fine as it follows other XO clocks, but please add it to the commit message.
+1
Konrad
^ permalink raw reply
* Re: [PATCH 4/4] arm64: dts: qcom: glymur: Add EVA clock controller node
From: Konrad Dybcio @ 2026-06-11 12:00 UTC (permalink / raw)
To: Taniya Das, Bjorn Andersson, Michael Turquette, Stephen Boyd,
Brian Masney, Dmitry Baryshkov, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Konrad Dybcio
Cc: Ajit Pandey, Imran Shaik, Jagadeesh Kona, linux-arm-msm,
linux-clk, linux-kernel, devicetree
In-Reply-To: <20260526-evacc_glymur-v1-4-b61c7755c403@oss.qualcomm.com>
On 5/26/26 7:29 AM, Taniya Das wrote:
> Add the device node for the EVA clock controller (evacc) for Qualcomm
> Glymur SoC. The EVACC provides clocks and resets to the EVA hardware block.
>
> Signed-off-by: Taniya Das <taniya.das@oss.qualcomm.com>
> ---
> arch/arm64/boot/dts/qcom/glymur.dtsi | 19 +++++++++++++++++++
> 1 file changed, 19 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/glymur.dtsi b/arch/arm64/boot/dts/qcom/glymur.dtsi
> index 20b49af7298e9549d126aa50a0dc7a90943a3249..66948808d197bd17ffe65190b472bb845cba0eb8 100644
> --- a/arch/arm64/boot/dts/qcom/glymur.dtsi
> +++ b/arch/arm64/boot/dts/qcom/glymur.dtsi
> @@ -4,6 +4,7 @@
> */
>
> #include <dt-bindings/clock/qcom,glymur-dispcc.h>
> +#include <dt-bindings/clock/qcom,glymur-evacc.h>
> #include <dt-bindings/clock/qcom,glymur-gcc.h>
> #include <dt-bindings/clock/qcom,glymur-gpucc.h>
> #include <dt-bindings/clock/qcom,glymur-tcsr.h>
> @@ -4804,6 +4805,24 @@ videocc: clock-controller@aaf0000 {
> #power-domain-cells = <1>;
> };
>
> + evacc: clock-controller@abf0000 {
> + compatible = "qcom,glymur-evacc";
> + reg = <0x0 0x0abf0000 0x0 0x10000>;
> + clocks = <&gcc GCC_EVA_AHB_CLK>,
> + <&rpmhcc RPMH_CXO_CLK>,
> + <&rpmhcc RPMH_CXO_CLK_A>,
With the XO_A situation resolved:
Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Konrad
^ permalink raw reply
* RE: [PATCH v1 3/7] ASoC: codecs: ES8389: Fix the issue about mclk_src
From: Zhang Yi @ 2026-06-11 12:01 UTC (permalink / raw)
To: krzk
Cc: broonie, conor+dt, devicetree, krzk+dt, linux-sound, robh, tiwai,
zhangyi
In-Reply-To: <20260611-likable-ultraviolet-moth-fdf2f6@quoll>
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 1363 bytes --]
> > Fix the issue with incorrect modifications to mclk_src When the system
> > needs to be configured to use the MCLK from the SCLK pin, the code
> > still sets the relevant registers to use the MCLK from the MCLK pin
> > And setting `mclk_src` to `u8` is inappropriate, because the purpose
> > of `mclk_src` is to determine whether to use SCLK as MCLK.
> > Therefore, we will change the `mclk_src` member from `u8` to `bool`.
> >
> > Signed-off-by: Zhang Yi <zhangyi@everest-semi.com>
>
> NAK, same comments as before. You just ignore feedback.
I thought I had responded to your feedback, but I haven't received a reply since then.
I¡¯d like to know if I need to respond to all of the AI¡¯s feedback.
Much of the AI¡¯s feedback concerns pre-existing issues that are unrelated to my patch.
At the same time, I assumed that when you saw a ¡°High¡± severity level on Sashiko,
you wouldn't be able to view the patches, which is why I sent the following patches and assumed they were all v0.
https://lore.kernel.org/all/20260608083540.12581-1-zhangyi@everest-semi.com/
https://lore.kernel.org/all/20260609025605.16945-1-zhangyi@everest-semi.com/
https://lore.kernel.org/all/20260609030623.17404-1-zhangyi@everest-semi.com/
If you can see my reply, should the patch I send next be v2 or v5? I think it should be v2.
I'm sorry for the trouble I've caused you
^ permalink raw reply
* Re: [PATCH 1/2] dt-bindings: pinctrl: tegra238: add missing AON pin groups
From: Linus Walleij @ 2026-06-11 12:03 UTC (permalink / raw)
To: Prathamesh Shete
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Thierry Reding,
Jonathan Hunter, Arnd Bergmann, linux-gpio, devicetree,
linux-tegra, linux-kernel
In-Reply-To: <20260608094122.1245189-1-pshete@nvidia.com>
On Mon, Jun 8, 2026 at 11:41 AM Prathamesh Shete <pshete@nvidia.com> wrote:
> Add 24 pin groups, and their matching drive groups, on ports EE, FF,
> GG and HH to the Tegra238 AON pinmux binding. These groups are present
> on the AON pin controller, so device trees that mux these pins through
> it validate against the schema.
>
> Fixes: 9323f8a0e12c ("dt-bindings: pinctrl: Document Tegra238 pin controllers")
> Signed-off-by: Prathamesh Shete <pshete@nvidia.com>
Patch applied.
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH 5/7] arm64: dts: qcom: sm8350: expand UART18 to 4 pins config
From: Konrad Dybcio @ 2026-06-11 12:04 UTC (permalink / raw)
To: Dmitry Baryshkov, Manivannan Sadhasivam, Lorenzo Pieralisi,
Krzysztof Wilczyński, Rob Herring, Bjorn Helgaas, Qiang Yu,
Jeff Johnson, Liam Girdwood, Mark Brown, Krzysztof Kozlowski,
Conor Dooley, Bartosz Golaszewski, Marcel Holtmann,
Luiz Augusto von Dentz, Balakrishna Godavarthi, Rocky Liao,
Bjorn Andersson, Konrad Dybcio
Cc: linux-arm-msm, linux-pci, linux-kernel, linux-wireless, ath11k,
devicetree, Bartosz Golaszewski, linux-bluetooth
In-Reply-To: <20260601-sm8350-wifi-v1-5-242917d88031@oss.qualcomm.com>
On 6/1/26 11:46 AM, Dmitry Baryshkov wrote:
> On SM8350 platforms the primary use of UART18 is a 4-pin UART (targeting
> Bluetooth or other similar applications). Add all 4 pins to the default
> pinctrl entry for the UART.
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> ---
Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Konrad
^ permalink raw reply
* Re: [PATCH 2/2] pinctrl: tegra238: add missing AON pin groups
From: Linus Walleij @ 2026-06-11 12:04 UTC (permalink / raw)
To: Prathamesh Shete
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Thierry Reding,
Jonathan Hunter, Arnd Bergmann, linux-gpio, devicetree,
linux-tegra, linux-kernel
In-Reply-To: <20260608094122.1245189-2-pshete@nvidia.com>
On Mon, Jun 8, 2026 at 11:42 AM Prathamesh Shete <pshete@nvidia.com> wrote:
> Add 24 pin groups on ports EE, FF, GG and HH to the AON pin controller
> group table (tegra238_aon_groups[]). Their pin arrays, drive-group
> macros and pin descriptors were already defined, but the matching
> PINGROUP() entries were not present, so these pins could not be muxed
> or configured through the AON pin controller.
>
> The pin arrays were not referenced, so the build emitted
> -Wunused-const-variable warnings, and commit 119de2c33d96 ("pinctrl:
> tegra238: remove unused entries") removed three of them. Restore those
> arrays and add the full set of PINGROUP() entries to make the pins
> usable.
>
> Fixes: 25cac7292d49 ("pinctrl: tegra: Add Tegra238 pinmux driver")
> Signed-off-by: Prathamesh Shete <pshete@nvidia.com>
Patches applied.
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH v3 00/11] kdump: reduce vmcore size and capture time
From: Baoquan He @ 2026-06-11 12:03 UTC (permalink / raw)
To: Wandun
Cc: linux-arm-kernel, linux-kernel, loongarch, linux-riscv,
devicetree, kexec, iommu, zhaomeijing, Rob Herring, saravanak,
bhe, rppt, pjw, palmer, aou, chenhuacai, kernel, catalin.marinas,
will, alex, akpm, pasha.tatashin, pratyush, ruirui.yang,
m.szyprowski, robin.murphy
In-Reply-To: <a3993db0-6975-455d-9674-4fd7cfcf80fc@gmail.com>
On 06/11/26 at 11:09am, Wandun wrote:
>
>
> On 6/11/26 10:09, Wandun wrote:
> >
> >
> > On 5/27/26 11:29, Wandun Chen wrote:
> >> From: Wandun Chen <chenwandun@lixiang.com>
> >>
> >> On SoCs that carve out large firmware-owned reserved memory (GPU
> >> firmware, DSP, modem, camera ISP, NPU, ...), kdump currently dumps
> >> those carveouts as part of system RAM even though their contents are
> >> firmware state that is not useful for kernel crash analysis.
> >>
> >> This series introduces an opt-in 'dumpable' flag [1] on struct
> >> reserved_mem and uses it to filter the elfcorehdr PT_LOAD ranges on
> >> DT-based architectures (arm64, riscv, loongarch). By default reserved
> >> regions are treated as non-dumpable; CMA regions are explicitly opted
> >> in because their pages are returned to the buddy allocator and may
> >> carry key crash-analysis data.
> >>
> >> The series is organized as follows:
> >> Patches 1-3: Pre-existing fixes and a small prep change.
> >> Patches 4-5: Restructure to allow appending /memreserve/ entries.
> >> Patches 6-7: Add a dumpable flag and append /memreserve/ entries.
> >> Patch 8: Add generic kdump helpers.
> >> Patches 9-11: Wire the helpers into arm64, riscv and loongarch kdump
> >> elfcorehdr preparation.
> > Hi,
> >
> > Gentle ping on this series.
> >
> > Status summary:
> > -patch 03: respun separately per Rob's suggestion, picked up for 7.2
> > -patch 06: Acked-by: Marek Szyprowski -patch 09: Acked-by: Will Deacon
> > The remaining patches (01, 02, 04, 05, 07, 08, 10, 11) are still
> > awaiting review. your feedback would be greately appreciated. I know we
> > are at the end of 7.1 -rc cycle, I don't want to rush this series, just
> > collecting more feedback, and will send next version based on 7.2-rc1.
> > If spliting the series into smaller logical group would make review
> > easier, please let me know. Best regards, Wandun
>
> Apologies for the formatting issue in my previous email.
> Here is the properly formatted version.
>
> Gentle ping on this series.
Thanks for the effort, the overral looks good to me at 1st glance. I will
check if there's concern on generic part. And meanwhile, I am wondering
if there's any chance x86 or other ARCH-es w/o OF/FDT can also choose to
not dump some areas, e.g GPU stolen memory. Surely, that's another story.
>
> Status summary:
> - patch 03: respun separately per Rob's suggestion, picked up for 7.2
> - patch 06: Acked-by: Marek Szyprowski
> - patch 09: Acked-by: Will Deacon
>
> The remaining patches (01, 02, 04, 05, 07, 08, 10, 11) are still
> awaiting review. Your feedback would be greatly appreciated.
>
> I know we are at the end of 7.1-rc cycle, I don't want to rush this
> series, just collecting more feedback, and will send next version based
> on 7.2-rc1.
>
> If splitting the series into smaller logical groups would make review
> easier, please let me know.
>
> Best regards,
> Wandun
>
>
> >>
> >> v2 --> v3:
> >> 1. Fix out-of-bounds issue if device tree lacks /reserved-memory node.[2]
> >> 2. Fix UAF issue when alloc_reserved_mem_array() fails.
> >> 3. Add some prepare patches.
> >>
> >> v1 --> v2:
> >> 1. v1 added an opt-out DT property ('linux,no-dump'). Per Rob's
> >> feedback [1], v2 drop that property and exclude reserve memory
> >> by default.
> >> 2. Split some prepared patches from the original patches.
> >> 3. Address coding-style comments on patch 5 from Rob.
> >>
> >> [1] https://lore.kernel.org/lkml/20260506144542.GA2072596-
> >> robh@kernel.org/
> >> [2] https://sashiko.dev/#/patchset/20260520091844.592753-1-
> >> chenwandun%40lixiang.com?part=4
> >>
> >> Wandun Chen (11):
> >> of: reserved_mem: handle NULL name in of_reserved_mem_lookup()
> >> kexec/crash: provide crash_exclude_mem_range() stub when
> >> CONFIG_CRASH_DUMP=n
> >> of: reserved_mem: avoid post-init UAF when alloc_reserved_mem_array()
> >> fails
> >> of: reserved_mem: zero total_reserved_mem_cnt if no valid
> >> /reserved-memory entry
> >> of: reserved_mem: split alloc_reserved_mem_array() from
> >> fdt_scan_reserved_mem_late()
> >> of: reserved_mem: add dumpable flag to opt-in vmcore
> >> of: reserved_mem: save /memreserve/ entries into the reserved_mem
> >> array
> >> of: reserved_mem: add kdump helpers to exclude non-dumpable regions
> >> arm64: kdump: exclude non-dumpable reserved memory regions from vmcore
> >> riscv: kdump: exclude non-dumpable reserved memory regions from vmcore
> >> loongarch: kdump: exclude non-dumpable reserved memory regions from
> >> vmcore
> >>
> >> arch/arm64/kernel/machine_kexec_file.c | 6 ++
> >> arch/loongarch/kernel/machine_kexec_file.c | 6 ++
> >> arch/riscv/kernel/machine_kexec_file.c | 4 +
> >> drivers/of/fdt.c | 11 +-
> >> drivers/of/of_private.h | 3 +
> >> drivers/of/of_reserved_mem.c | 117 +++++++++++++++++++--
> >> include/linux/crash_core.h | 6 ++
> >> include/linux/of_reserved_mem.h | 15 +++
> >> kernel/dma/contiguous.c | 1 +
> >> 9 files changed, 157 insertions(+), 12 deletions(-)
> >>
> >
>
^ permalink raw reply
* Re: [PATCH v3 2/3] hwmon: pmbus: Add support for Silergy SQ24860
From: Guenter Roeck @ 2026-06-11 12:06 UTC (permalink / raw)
To: sashiko-reviews, Ziming Zhu; +Cc: robh, linux-hwmon, conor+dt, devicetree
In-Reply-To: <20260611075433.325CB1F00893@smtp.kernel.org>
On 6/11/26 00:54, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] Missing signedness check on PMBus limits causes negative inputs to silently set maximum hardware limits.
> - [Medium] Returns -EINVAL instead of clamping for out-of-bounds lower limits in PMBUS_IIN_OC_FAULT_LIMIT.
> --
>
> commit 1edf6322da1b629fce9b16a96496a18863aacef8
> Author: Ziming Zhu <ziming.zhu@silergycorp.com>
>
> hwmon: pmbus: Add support for Silergy SQ24860
>
> Add PMBus hwmon support for the Silergy SQ24860 eFuse. The driver reports
> voltage, current, power, and temperature, along with history attributes
> and over-current fault limits.
>
>> diff --git a/drivers/hwmon/pmbus/sq24860.c b/drivers/hwmon/pmbus/sq24860.c
>> new file mode 100644
>> index 0000000000000..f16f650ff7ba2
>> --- /dev/null
>> +++ b/drivers/hwmon/pmbus/sq24860.c
> [ ... ]
>> +static int sq24860_write_word_data(struct i2c_client *client,
>> + int page, int reg, u16 value)
>> +{
>> + int ret;
>> +
>> + switch (reg) {
>> + case PMBUS_VIN_UV_WARN_LIMIT:
>> + case PMBUS_VIN_UV_FAULT_LIMIT:
>> + case PMBUS_VIN_OV_WARN_LIMIT:
>> + case PMBUS_VIN_OV_FAULT_LIMIT:
>> + case PMBUS_VOUT_UV_WARN_LIMIT:
>> + case PMBUS_IIN_OC_WARN_LIMIT:
>> + case PMBUS_OT_WARN_LIMIT:
>> + case PMBUS_OT_FAULT_LIMIT:
>> + case PMBUS_PIN_OP_WARN_LIMIT:
>> + value >>= SQ24860_8B_SHIFT;
>> + value = clamp_val(value, 0, 0xff);
>
> [Severity: High]
> Does this handle negative user inputs correctly?
>
> The PMBus core converts limit values into 16-bit 2's complement integers
> (s16), but passes them as u16 to sq24860_write_word_data(). Because this
> driver treats value as an unsigned u16, a negative user limit written via
> sysfs will become a large positive value (e.g., 0xFFFF).
>
> When this large value is logically right-shifted and clamped here, it
> results in setting the maximum possible hardware limit (0xff) instead of
> clamping to the minimum limit (0).
>
Without a datasheet it is all but impossible to determine how to handle
this. Is the 8-bit value signed or not ? Who knows. The read function
suggests that it is unsigned, given that the sign bit is not extended,
but that is just a guess. If the value is unsigned, negative values
(0x8000 and higher) would have to be set to 0 prior to shifting and
clamping. However, that really depends on the chip.
>> + ret = pmbus_write_word_data(client, page, reg, value);
>> + break;
>> +
>> + case PMBUS_IIN_OC_FAULT_LIMIT:
>> + if (value < SQ24860_IIN_OCF_OFF)
>> + return -EINVAL;
>
> [Severity: High]
> Could a negative user limit bypass this bounds check?
>
> Since value is unsigned, a negative limit (e.g., 0xFFFF) will evaluate
> as greater than SQ24860_IIN_OCF_OFF (165). This bypasses the check below
> and overflows the 32-bit unsigned calculation, effectively disabling
> over-current protection by incorrectly setting the hardware limit to its
> maximum.
>
Same as above.
> [Severity: Medium]
> Should this clamp the value instead of returning an error?
>
> The hwmon sysfs ABI convention requires drivers to silently clamp
> out-of-range user limits to the nearest supported hardware value rather
> than returning an error.
>
> If the requested limit translates to a value below SQ24860_IIN_OCF_OFF,
> returning -EINVAL violates this convention. Would it be better to clamp
> the limit to SQ24860_IIN_OCF_OFF so the underlying register is set to 0?
>
Yes.
Thanks,
Guenter
>> + value -= SQ24860_IIN_OCF_OFF;
>> + value = DIV_ROUND_CLOSEST(((unsigned int)value) * SQ24860_IIN_OCF_DIV,
>> + SQ24860_IIN_OCF_NUM);
>> + value = clamp_val(value, 0, 0x3f);
>> + ret = pmbus_write_byte_data(client, page, SQ24860_VIREF, value);
>> + break;
>
^ permalink raw reply
* Re: [PATCH 7/7] arm64: dts: qcom: sm8350-hdk: describe WiFi/BT chip
From: Konrad Dybcio @ 2026-06-11 12:09 UTC (permalink / raw)
To: Dmitry Baryshkov, Manivannan Sadhasivam, Lorenzo Pieralisi,
Krzysztof Wilczyński, Rob Herring, Bjorn Helgaas, Qiang Yu,
Jeff Johnson, Liam Girdwood, Mark Brown, Krzysztof Kozlowski,
Conor Dooley, Bartosz Golaszewski, Marcel Holtmann,
Luiz Augusto von Dentz, Balakrishna Godavarthi, Rocky Liao,
Bjorn Andersson, Konrad Dybcio
Cc: linux-arm-msm, linux-pci, linux-kernel, linux-wireless, ath11k,
devicetree, Bartosz Golaszewski, linux-bluetooth
In-Reply-To: <20260601-sm8350-wifi-v1-7-242917d88031@oss.qualcomm.com>
On 6/1/26 11:46 AM, Dmitry Baryshkov wrote:
> The SM8350 HDK has onboard WiFi/BT chip, WCN6851. It is an earlier
> version of well-known WCN6855 WiFI/BT SoC. Describe the PMU, BT and WiFI
> parts of the device.
[...]
> + wcn6855-pmu {
> + compatible = "qcom,wcn6851-pmu", "qcom,wcn6855-pmu";
> +
> + pinctrl-0 = <&bt_en>, <&wlan_en>, <&swctrl>;
> + pinctrl-names = "default";
> +
> + wlan-enable-gpios = <&tlmm 64 GPIO_ACTIVE_HIGH>;
> + bt-enable-gpios = <&tlmm 65 GPIO_ACTIVE_HIGH>;
> + swctrl-gpios = <&tlmm 153 GPIO_ACTIVE_HIGH>;
> +
> + vddio-supply = <&vreg_s10b_1p8>;
> + vddaon-supply = <&vreg_s11b_0p95>;
> + vddpmu-supply = <&vreg_s11b_0p95>;
> + vddpmumx-supply = <&vreg_s2e_0p85>;
> + vddpmucx-supply = <&vreg_s11b_0p95>;
> + vddrfa0p95-supply = <&vreg_s11b_0p95>;
> + vddrfa1p3-supply = <&vreg_s12b_1p25>;
> + vddrfa1p9-supply = <&vreg_s1c_1p86>;
> + vddpcie1p3-supply = <&vreg_s12b_1p25>;
> + vddpcie1p9-supply = <&vreg_s1c_1p86>;
[...]
> @@ -373,6 +437,13 @@ vreg_l7e_2p8: ldo7 {
> regulator-name = "vreg_l7e_2p8";
> regulator-min-microvolt = <2800000>;
> regulator-max-microvolt = <2800000>;
> +
> + /*
> + * This is used by the RF front-end for which there is
> + * no way to represent it in DT (yet?).
> + */
> + regulator-boot-on;
> + regulator-always-on;
msm-5.4 maps this to bt-vdd-asd-supply (asd being a keyboard smash,
perhaps?) - what is its actual use?
Konrad
^ permalink raw reply
* Re: [PATCH v11 4/6] arm64: dts: qcom: kodiak: Add OPP-table for ICE UFS and ICE eMMC nodes
From: Kuldeep Singh @ 2026-06-11 12:12 UTC (permalink / raw)
To: Abhinaba Rakshit, Bjorn Andersson, Konrad Dybcio,
Manivannan Sadhasivam, James E.J. Bottomley, Martin K. Petersen,
Adrian Hunter, Ulf Hansson, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Neeraj Soni, Harshal Dev
Cc: linux-arm-msm, linux-kernel, linux-scsi, linux-mmc, devicetree
In-Reply-To: <20260609-enable-ice-clock-scaling-v11-4-1cebc8b3275b@oss.qualcomm.com>
On 09-06-2026 03:17, Abhinaba Rakshit wrote:
> Qualcomm Inline Crypto Engine (ICE) platform driver now, supports
> an optional OPP-table.
>
> Add OPP-table for ICE UFS and ICE eMMC device nodes for Kodiak
> platform.
s/eMMC/sdhc
>
> Signed-off-by: Abhinaba Rakshit <abhinaba.rakshit@oss.qualcomm.com>
> ---
> arch/arm64/boot/dts/qcom/kodiak.dtsi | 42 ++++++++++++++++++++++++++++++++++++
> 1 file changed, 42 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/kodiak.dtsi b/arch/arm64/boot/dts/qcom/kodiak.dtsi
> index ecf4790f3415c46781c8e790d7892a41300ee7a0..cd76da7e49d8c664df6a60b5c18418c4e97a3ba4 100644
> --- a/arch/arm64/boot/dts/qcom/kodiak.dtsi
> +++ b/arch/arm64/boot/dts/qcom/kodiak.dtsi
> @@ -1087,6 +1087,27 @@ sdhc_ice: crypto@7c8000 {
> clock-names = "core",
> "iface";
> power-domains = <&rpmhpd SC7280_CX>;
> +
> + operating-points-v2 = <&ice_mmc_opp_table>;
To align with sdhc_ice(as label name), can we rename to ice_sdhc_opp_table?
With this udpate,
Reviewed-by: Kuldeep Singh <kuldeep.singh@oss.qualcomm.com>
--
Regards
Kuldeep
^ permalink raw reply
* Re: [PATCH 1/2] dt-bindings: iio: adc: Add TI ADS1220
From: Jonathan Cameron @ 2026-06-11 12:15 UTC (permalink / raw)
To: Nguyen Minh Tien
Cc: linux-iio, devicetree, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, David Lechner, Nuno Sá, Andy Shevchenko,
linux-kernel
In-Reply-To: <20260610151342.44274-2-zizuzacker@gmail.com>
On Wed, 10 Jun 2026 22:13:41 +0700
Nguyen Minh Tien <zizuzacker@gmail.com> wrote:
> The ADS1220 is a 24-bit, 2-kSPS, 4-channel delta-sigma ADC from Texas
> Instruments with an SPI (mode 1) interface, a programmable gain amplifier,
> an internal 2.048V reference and a dedicated DRDY data-ready output.
>
> Add a device tree binding describing the SPI device and its per-input
> channel child nodes (single-ended AINx or the multiplexer's differential
> pairs), the optional external/AVDD reference selection and the DRDY
> interrupt.
>
> Signed-off-by: Nguyen Minh Tien <zizuzacker@gmail.com>
Given Andy asked the big questions, I'll focus in on what is here
with assumption there is a good reason this can't be merged with existing
driver and / or binding. Note that it is fine to combine bindings but not
drivers, or indeed the other way around.
There are some interesting complex corners on this device - such as the RTD
configurations (current outputs on what would otherwise be input channels).
It might be worth looking at some of the other RTD supporting sensors
in tree and how their bindings are done. The bridge setup is also
a little interesting as we need to know to enable the bypass of AIN3 to
AVSS.
Normally we strongly push for a binding to be complete as possible from
the start, even if the driver doesn't support some features yet but
here that may make it more complex.
Just a couple of comments below
Thanks,
Jonathan
> ---
> .../bindings/iio/adc/ti,ads1220.yaml | 146 ++++++++++++++++++
> 1 file changed, 146 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iio/adc/ti,ads1220.yaml
>
> diff --git a/Documentation/devicetree/bindings/iio/adc/ti,ads1220.yaml b/Documentation/devicetree/bindings/iio/adc/ti,ads1220.yaml
> new file mode 100644
> index 000000000..1fedffc2a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/ti,ads1220.yaml
> @@ -0,0 +1,146 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/ti,ads1220.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Texas Instruments ADS1220 ADC
> +
> +maintainers:
> + - Nguyen Minh Tien <zizuzacker@gmail.com>
> +
> +description:
> + The TI ADS1220 is a precision 24-bit, 2-kSPS, delta-sigma ADC with an SPI
> + (mode 1) interface. It provides two differential or four single-ended inputs
> + through a multiplexer, a programmable gain amplifier (gain 1 to 128), an
> + internal 2.048V reference and oscillator, two programmable excitation current
> + sources and a 50/60Hz rejection filter. A dedicated DRDY output signals when a
> + new conversion result is available.
Ideally put a datasheet reference in this description.
> +
> +properties:
> + compatible:
> + const: ti,ads1220
> +
> + reg:
> + maxItems: 1
> +
> + spi-cpha: true
> +
> + interrupts:
> + description: DRDY pin, signals that a new conversion result is ready.
> + maxItems: 1
> +
> + avdd-supply:
> + description: Analog power supply (AVDD/AVSS).
> +
> + dvdd-supply:
> + description: Digital power supply (DVDD/DGND).
> +
> + vref-supply:
> + description:
> + External reference voltage (REFP0/REFN0). If omitted, the internal
> + 2.048V reference is used unless ti,vref-avdd is set.
> +
> + ti,vref-avdd:
> + type: boolean
> + description:
> + Use the analog supply (AVDD/AVSS) as the conversion reference instead of
> + the internal 2.048V reference. Suited to ratiometric single-supply
> + measurements (for example a potentiometer wired across AVDD), giving a
> + full 0..AVDD input range without an external reference. Ignored when
> + vref-supply is present.
> +
> + "#address-cells":
> + const: 1
> +
> + "#size-cells":
> + const: 0
> +
> + "#io-channel-cells":
> + const: 1
> +
> +required:
> + - compatible
> + - reg
> + - "#address-cells"
> + - "#size-cells"
> + - avdd-supply
> + - dvdd-supply
> +
> +patternProperties:
> + "^channel@[0-6]$":
> + $ref: adc.yaml
> + type: object
> + description: Represents one ADC input configuration (channel).
> +
> + properties:
> + reg:
> + minimum: 0
> + maximum: 6
Why 6? Is the idea that you might have both differential and single ended
inputs from the same physical wires, or overlapping differential
pairs? In practice that rarely reflects how boards are wired.
However I can't immediately figure out a combination that ends up with 7 channels.
> +
> + diff-channels:
> + description:
> + Differential input pair routable by the ADS1220 multiplexer.
> + oneOf:
> + - items: [const: 0, const: 1]
> + - items: [const: 0, const: 2]
> + - items: [const: 0, const: 3]
> + - items: [const: 1, const: 2]
> + - items: [const: 1, const: 3]
> + - items: [const: 2, const: 3]
> + - items: [const: 1, const: 0]
> + - items: [const: 3, const: 2]
> +
> + single-channel:
> + description:
> + Single-ended input channel AINx measured against AVSS.
> + minimum: 0
> + maximum: 3
> +
> + oneOf:
> + - required: [diff-channels]
> + - required: [single-channel]
> +
> + required:
> + - reg
> +
> + unevaluatedProperties: false
> +
> +allOf:
> + - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +unevaluatedProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/interrupt-controller/irq.h>
> +
> + spi {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + adc@0 {
> + compatible = "ti,ads1220";
> + reg = <0>;
> + spi-max-frequency = <2500000>;
> + spi-cpha;
> + interrupt-parent = <&pio>;
> + interrupts = <4 4 IRQ_TYPE_EDGE_FALLING>;
> + avdd-supply = <®_vcc3v3>;
> + dvdd-supply = <®_vcc3v3>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + #io-channel-cells = <1>;
> +
> + channel@0 {
> + reg = <0>;
> + single-channel = <0>;
> + };
> +
> + channel@1 {
> + reg = <1>;
> + diff-channels = <0 1>;
> + };
> + };
> + };
> +...
^ 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 v11 5/6] arm64: dts: qcom: monaco: Add OPP-table for ICE UFS and ICE eMMC nodes
From: Kuldeep Singh @ 2026-06-11 12:20 UTC (permalink / raw)
To: Abhinaba Rakshit, Bjorn Andersson, Konrad Dybcio,
Manivannan Sadhasivam, James E.J. Bottomley, Martin K. Petersen,
Adrian Hunter, Ulf Hansson, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Neeraj Soni, Harshal Dev
Cc: linux-arm-msm, linux-kernel, linux-scsi, linux-mmc, devicetree
In-Reply-To: <20260609-enable-ice-clock-scaling-v11-5-1cebc8b3275b@oss.qualcomm.com>
On 09-06-2026 03:17, Abhinaba Rakshit wrote:
> Qualcomm Inline Crypto Engine (ICE) platform driver now, supports
> an optional OPP-table.
>
> Add OPP-table for ICE UFS and ICE eMMC device nodes for Monaco
> platform.
>
> Signed-off-by: Abhinaba Rakshit <abhinaba.rakshit@oss.qualcomm.com>
> ---
> arch/arm64/boot/dts/qcom/monaco.dtsi | 37 ++++++++++++++++++++++++++++++++++++
> 1 file changed, 37 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/monaco.dtsi b/arch/arm64/boot/dts/qcom/monaco.dtsi
> index a1b6e6211b84d0d5008231c55613a0ccd61b9450..d9298d8b7874b8669b2cded2a28a99dce6eadbda 100644
> --- a/arch/arm64/boot/dts/qcom/monaco.dtsi
> +++ b/arch/arm64/boot/dts/qcom/monaco.dtsi
> @@ -2742,6 +2742,27 @@ ice: crypto@1d88000 {
> clock-names = "core",
> "iface";
> power-domains = <&gcc GCC_UFS_PHY_GDSC>;
> +
> + operating-points-v2 = <&ice_opp_table>;
> +
> + ice_opp_table: opp-table {
> + compatible = "operating-points-v2";
> +
> + opp-75000000 {
> + opp-hz = /bits/ 64 <75000000>;
> + required-opps = <&rpmhpd_opp_svs_l1>;
> + };
> +
> + opp-201600000 {
> + opp-hz = /bits/ 64 <201600000>;
> + required-opps = <&rpmhpd_opp_svs_l1>;
> + };
> +
> + opp-403200000 {
> + opp-hz = /bits/ 64 <403200000>;
> + required-opps = <&rpmhpd_opp_nom>;
> + };
> + };
> };
>
> crypto: crypto@1dfa000 {
> @@ -4878,6 +4899,22 @@ sdhc_ice: crypto@87c8000 {
> clock-names = "core",
> "iface";
> power-domains = <&rpmhpd RPMHPD_CX>;
> +
> + operating-points-v2 = <&ice_mmc_opp_table>;
s/ice_mmc_opp_table/sdhc_ice_opp_table?
--
Regards
Kuldeep
^ permalink raw reply
* Re: [PATCH 2/2] hwmon: (pmbus/lm25066) add SMBus current limit configuration support
From: Guenter Roeck @ 2026-06-11 12:20 UTC (permalink / raw)
To: Potin Lai, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Zev Weiss
Cc: linux-hwmon, devicetree, linux-kernel, Cosmo Chou, Mike Hsieh,
Potin Lai
In-Reply-To: <20260611-lm25066-cl-config-v1-2-02e567bf3d91@gmail.com>
On 6/11/26 02:58, Potin Lai wrote:
> Add support for the mutually exclusive 'ti,cl-smbus-high' and
> 'ti,cl-smbus-low' devicetree properties. When present, these properties
> override the hardware configuration pins via the DEVICE_SETUP (0xD9)
> register to set the Current Limit Configuration bit (bit 2) and
> Current Limit Setting bit (bit 4) to SMBus settings.
>
> The Bit 4 mapping to High/Low current limit is handled dynamically on
> probe because it is swapped for lm25056 and lm25066 compared to other
> supported chips (lm5064, lm5066, and lm5066i).
>
> Signed-off-by: Potin Lai <potin.lai.pt@gmail.com>
> ---
> drivers/hwmon/pmbus/lm25066.c | 25 +++++++++++++++++++++++++
> 1 file changed, 25 insertions(+)
>
> diff --git a/drivers/hwmon/pmbus/lm25066.c b/drivers/hwmon/pmbus/lm25066.c
> index dd7275a67a0a..20e114bdc882 100644
> --- a/drivers/hwmon/pmbus/lm25066.c
> +++ b/drivers/hwmon/pmbus/lm25066.c
> @@ -34,6 +34,7 @@ enum chips { lm25056, lm25066, lm5064, lm5066, lm5066i };
> #define LM25066_READ_AVG_PIN 0xdf
>
> #define LM25066_DEV_SETUP_CL BIT(4) /* Current limit */
> +#define LM25066_DEV_SETUP_CL_CFG BIT(2) /* Current limit configuration */
>
> #define LM25066_SAMPLES_FOR_AVG_MAX 4096
>
> @@ -464,6 +465,8 @@ MODULE_DEVICE_TABLE(of, lm25066_of_match);
> static int lm25066_probe(struct i2c_client *client)
> {
> int config;
> + int config_new;
> + int ret;
> u32 shunt;
> struct lm25066_data *data;
> struct pmbus_driver_info *info;
> @@ -484,6 +487,28 @@ static int lm25066_probe(struct i2c_client *client)
>
> data->id = (enum chips)(unsigned long)i2c_get_match_data(client);
>
> + config_new = config;
> + if (of_property_read_bool(client->dev.of_node, "ti,cl-smbus-high")) {
> + config_new |= LM25066_DEV_SETUP_CL_CFG;
> + if (data->id == lm25056 || data->id == lm25066)
LM25056 does not support setting the gain via software, and bit 2 of this
register is reserved. These properties need to be disabled for that chip.
That will have to be reflected both here and in the devicetree file.
Thanks,
Guenter
^ permalink raw reply
* [PATCH] dt-bindings: i2c: mux-gpio: name correct maintainer
From: Wolfram Sang @ 2026-06-11 12:20 UTC (permalink / raw)
To: linux-i2c
Cc: Peter Korsgaard, Wolfram Sang, Peter Rosin, Andi Shyti,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, devicetree
The YAML conversion added me as maintainer but I can't recall being
asked nor do I want to maintain it. Add Peter as maintainer for the
binding as he is maintainer of the driver.
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
Documentation/devicetree/bindings/i2c/i2c-mux-gpio.yaml | 2 +-
MAINTAINERS | 1 +
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-gpio.yaml b/Documentation/devicetree/bindings/i2c/i2c-mux-gpio.yaml
index 4a93d1f78f93..6e44510aaef6 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-mux-gpio.yaml
+++ b/Documentation/devicetree/bindings/i2c/i2c-mux-gpio.yaml
@@ -7,7 +7,7 @@ $schema: http://devicetree.org/meta-schemas/core.yaml#
title: GPIO-based I2C Bus Mux
maintainers:
- - Wolfram Sang <wsa@kernel.org>
+ - Peter Korsgaard <peter.korsgaard@barco.com>
description: |
This binding describes an I2C bus multiplexer that uses GPIOs to route the I2C signals.
diff --git a/MAINTAINERS b/MAINTAINERS
index e035a3be797c..a616abfce644 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10772,6 +10772,7 @@ GENERIC GPIO I2C MULTIPLEXER DRIVER
M: Peter Korsgaard <peter.korsgaard@barco.com>
L: linux-i2c@vger.kernel.org
S: Supported
+F: Documentation/devicetree/bindings/i2c/i2c-mux-gpio.yaml
F: Documentation/i2c/muxes/i2c-mux-gpio.rst
F: drivers/i2c/muxes/i2c-mux-gpio.c
F: include/linux/platform_data/i2c-mux-gpio.h
--
2.51.0
^ permalink raw reply related
* Re: [PATCH v11 6/6] arm64: dts: qcom: lemans: Add OPP-table for ICE UFS device node
From: Kuldeep Singh @ 2026-06-11 12:23 UTC (permalink / raw)
To: Abhinaba Rakshit, Bjorn Andersson, Konrad Dybcio,
Manivannan Sadhasivam, James E.J. Bottomley, Martin K. Petersen,
Adrian Hunter, Ulf Hansson, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Neeraj Soni, Harshal Dev
Cc: linux-arm-msm, linux-kernel, linux-scsi, linux-mmc, devicetree
In-Reply-To: <20260609-enable-ice-clock-scaling-v11-6-1cebc8b3275b@oss.qualcomm.com>
On 09-06-2026 03:17, Abhinaba Rakshit wrote:
> Qualcomm Inline Crypto Engine (ICE) platform driver now supports
> an optional OPP-table.
>
> Add OPP-table for ICE UFS device nodes for LeMans platform.
>
> Signed-off-by: Abhinaba Rakshit <abhinaba.rakshit@oss.qualcomm.com>
Reviewed-by: Kuldeep Singh <kuldeep.singh@oss.qualcomm.com>
--
Regards
Kuldeep
^ permalink raw reply
page: next (older) | prev (newer) | latest
- 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