[PATCH 0/2] leds: Add support for the is31fl3206 LED driver

[PATCH 0/2] leds: Add support for the is31fl3206 LED driver

[Flavio Amieiro]

This patch series adds support for the is31fl3206 LED driver to the
already existing is31fl32xx driver.

The main difference between this specific variant and the others in
the series is that the LED Control Register controls not only the
enable status of the channel, but also the maximum output current for
it.

Because the layout of the register is very different from the
previously supported variants, this patch follows the same pattern
present in the driver for paths that deviated from the norm: the
chipdef can define a pointer to an extra function that (if non-NULL)
will be called during probing. This was already done for reset_func
and for sw_shutdown_func in order to support other chips in the same
family.

The patches keep the previous behavior of using only the PWM registers
to control the state of the LEDs. For that reason, the current output
is always set to the maximum value.

This is my first attempt at a patch for the linux kernel, so please
let me know how I can improve it.

Signed-off-by: Flavio Amieiro <flavio@amieiro.net>
---
Flavio Amieiro (2):
      leds: is31f132xx: Add support for is31fl3206
      dt-bindings: leds: Add issi,is31fl3206 to leds-is31fl32xx

 .../devicetree/bindings/leds/leds-is31fl32xx.txt   |  1 +
 drivers/leds/leds-is31fl32xx.c                     | 89 ++++++++++++++++++----
 2 files changed, 74 insertions(+), 16 deletions(-)
---
base-commit: 05b4738b0078f7d6f154f68068a11c8a0635e9df
change-id: 20261012-add-is31fl3206-support-d557f863a97a

Best regards,
--  
Flavio Amieiro <flavio@amieiro.net>

[PATCH 1/2] leds: is31f132xx: Add support for is31fl3206

[Flavio Amieiro]

This chip has a different layout for the LED Control Register when
compared to the other variants. This register contains the enable bit
(in the highest nibble) but also the current control.

According to the datasheet, a value of 010000 (0x10) in the register
enables an LED with the output current set to maximum.

This commit adds support for this IC following the previous limitation
of the other variants: all LEDs are enabled and set with maximum
current, letting brightness be controlled by the PWM registers.

Signed-off-by: Flavio Amieiro <flavio@amieiro.net>
---
 drivers/leds/leds-is31fl32xx.c | 89 ++++++++++++++++++++++++++++++++++--------
 1 file changed, 73 insertions(+), 16 deletions(-)

diff --git a/drivers/leds/leds-is31fl32xx.c b/drivers/leds/leds-is31fl32xx.c
index 285085d27453..9c09548acabf 100644
--- a/drivers/leds/leds-is31fl32xx.c
+++ b/drivers/leds/leds-is31fl32xx.c
@@ -49,6 +49,9 @@
 #define IS31FL3293_RESET_MAGIC 0xc5
 #define IS31FL3293_MAX_MICROAMP 20000
 
+/* Custom LED Control enable value for IS31FL3206 */
+#define IS31FL3206_CONTROL_ENABLE_WITH_IMAX BIT(4)
+
 struct is31fl32xx_priv;
 struct is31fl32xx_led_data {
 	struct led_classdev cdev;
@@ -79,6 +82,7 @@ struct is31fl32xx_priv {
  * @enable_bits_per_led_control_register: number of LEDs enable bits in each
  * @brightness_steps    : number of brightness steps supported by the chip
  * @reset_func          : pointer to reset function
+ * @enable_all_channels_func  : pointer to enable all channels function
  * @sw_shutdown_func    : pointer to software shutdown function
  *
  * For all optional register addresses, the sentinel value %IS31FL32XX_REG_NONE
@@ -105,6 +109,7 @@ struct is31fl32xx_chipdef {
 	u8	enable_bits_per_led_control_register;
 	u16	brightness_steps;
 	int (*reset_func)(struct is31fl32xx_priv *priv);
+	int (*enable_all_channels_func)(struct is31fl32xx_priv *priv);
 	int (*sw_shutdown_func)(struct is31fl32xx_priv *priv, bool enable);
 };
 
@@ -228,6 +233,28 @@ static int is31fl3293_software_shutdown(struct is31fl32xx_priv *priv,
 	return is31fl32xx_write(priv, IS31FL3293_SHUTDOWN_REG, value);
 }
 
+/*
+ * Custom enable_all_channels function for IS31FL3206 because this
+ * variant has only one bit in the higher nibble for enable, and the
+ * lower nibble controls output current.
+ */
+static int is31fl3206_enable_all_channels(struct is31fl32xx_priv *priv)
+{
+	const struct is31fl32xx_chipdef *cdef = priv->cdef;
+	u8 num_regs = cdef->channels / cdef->enable_bits_per_led_control_register;
+	int i, ret;
+
+	for (i = 0; i < num_regs; i++) {
+		ret = is31fl32xx_write(priv,
+				       cdef->led_control_register_base+i,
+				       IS31FL3206_CONTROL_ENABLE_WITH_IMAX);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 /*
  * NOTE: A mutex is not needed in this function because:
  * - All referenced data is read-only after probe()
@@ -315,6 +342,33 @@ static int is31fl32xx_reset_regs(struct is31fl32xx_priv *priv)
 	return 0;
 }
 
+static int is31fl32xx_enable_all_channels(struct is31fl32xx_priv *priv)
+{
+	const struct is31fl32xx_chipdef *cdef = priv->cdef;
+	int ret;
+
+	if (cdef->led_control_register_base != IS31FL32XX_REG_NONE) {
+		u8 value =
+			GENMASK(cdef->enable_bits_per_led_control_register-1, 0);
+		u8 num_regs = cdef->channels /
+			cdef->enable_bits_per_led_control_register;
+		int i;
+
+		for (i = 0; i < num_regs; i++) {
+			ret = is31fl32xx_write(priv,
+					       cdef->led_control_register_base+i,
+					       value);
+			if (ret)
+				return ret;
+		}
+	}
+
+	if (cdef->enable_all_channels_func)
+		return cdef->enable_all_channels_func(priv);
+
+	return 0;
+}
+
 static int is31fl32xx_software_shutdown(struct is31fl32xx_priv *priv,
 					bool enable)
 {
@@ -345,24 +399,12 @@ static int is31fl32xx_init_regs(struct is31fl32xx_priv *priv)
 		return ret;
 
 	/*
-	 * Set enable bit for all channels.
+	 * Enable all channels.
 	 * We will control state with PWM registers alone.
 	 */
-	if (cdef->led_control_register_base != IS31FL32XX_REG_NONE) {
-		u8 value =
-		    GENMASK(cdef->enable_bits_per_led_control_register-1, 0);
-		u8 num_regs = cdef->channels /
-				cdef->enable_bits_per_led_control_register;
-		int i;
-
-		for (i = 0; i < num_regs; i++) {
-			ret = is31fl32xx_write(priv,
-					       cdef->led_control_register_base+i,
-					       value);
-			if (ret)
-				return ret;
-		}
-	}
+	ret = is31fl32xx_enable_all_channels(priv);
+	if (ret)
+		return ret;
 
 	ret = is31fl32xx_software_shutdown(priv, false);
 	if (ret)
@@ -552,7 +594,22 @@ static const struct is31fl32xx_chipdef is31fl3293_cdef = {
 	.sw_shutdown_func			= is31fl3293_software_shutdown,
 };
 
+static const struct is31fl32xx_chipdef is31fl3206_cdef = {
+	.channels				= 12,
+	.shutdown_reg				= 0x00,
+	.pwm_update_reg			= 0x13,
+	.global_control_reg			= 0x26,
+	.reset_reg				= 0x2f,
+	.output_frequency_setting_reg		= 0x27,
+	.pwm_register_base			= 0x04,
+	.led_control_register_base		= 0x17,
+	.enable_bits_per_led_control_register	= 1,
+	.brightness_steps			= 256,
+	.enable_all_channels_func		= is31fl3206_enable_all_channels,
+};
+
 static const struct of_device_id of_is31fl32xx_match[] = {
+	{ .compatible = "issi,is31fl3206",  .data = &is31fl3206_cdef, },
 	{ .compatible = "issi,is31fl3293",  .data = &is31fl3293_cdef, },
 	{ .compatible = "issi,is31fl3236",  .data = &is31fl3236_cdef, },
 	{ .compatible = "issi,is31fl3236a", .data = &is31fl3236a_cdef, },

-- 
2.53.0

Re: [PATCH 1/2] leds: is31f132xx: Add support for is31fl3206

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] leds: Double execution of LED control register initialization
- [Medium] leds: Missing is31fl3206 entry in legacy i2c_device_id table
--

commit 4f6d324bacf9d9547e9fba515fa86ec07e7ad36d
Author: Flavio Amieiro <flavio@amieiro.net>

leds: is31f132xx: Add support for is31fl3206

This patch adds support for the IS31FL3206 variant to the is31fl32xx LED
driver. It introduces a custom function to enable all channels since this
chip uses a different LED control register layout.

> diff --git a/drivers/leds/leds-is31fl32xx.c b/drivers/leds/leds-is31fl32xx.c
> index 285085d274538..9c09548acabfa 100644
> --- a/drivers/leds/leds-is31fl32xx.c
> +++ b/drivers/leds/leds-is31fl32xx.c

[ ... ]

> @@ -315,6 +342,33 @@ static int is31fl32xx_reset_regs(struct is31fl32xx_priv *priv)
>  	return 0;
>  }
>  
> +static int is31fl32xx_enable_all_channels(struct is31fl32xx_priv *priv)
> +{
> +	const struct is31fl32xx_chipdef *cdef = priv->cdef;
> +	int ret;
> +
> +	if (cdef->led_control_register_base != IS31FL32XX_REG_NONE) {
> +		u8 value =
> +			GENMASK(cdef->enable_bits_per_led_control_register-1, 0);
> +		u8 num_regs = cdef->channels /
> +			cdef->enable_bits_per_led_control_register;
> +		int i;
> +
> +		for (i = 0; i < num_regs; i++) {
> +			ret = is31fl32xx_write(priv,
> +					       cdef->led_control_register_base+i,
> +					       value);
> +			if (ret)
> +				return ret;
> +		}
> +	}
> +
> +	if (cdef->enable_all_channels_func)
> +		return cdef->enable_all_channels_func(priv);

[Severity: Medium]
Since is31fl3206_cdef sets both led_control_register_base and
enable_all_channels_func, won't this generic loop write 0x01 to all 12 control
registers first?

Because there is no early return or else branch here, won't it fall through and
call the custom enable_all_channels_func, which writes 0x10 to the exact same
registers again?

Does this double-write waste I2C bandwidth during probe/resume and briefly
leave the hardware in an unintended state?

> +
> +	return 0;
> +}
>  

[ ... ]

> @@ -552,7 +594,22 @@ static const struct is31fl32xx_chipdef is31fl3293_cdef = {
>  	.sw_shutdown_func			= is31fl3293_software_shutdown,
>  };
>  
> +static const struct is31fl32xx_chipdef is31fl3206_cdef = {
> +	.channels				= 12,
> +	.shutdown_reg				= 0x00,
> +	.pwm_update_reg			= 0x13,
> +	.global_control_reg			= 0x26,
> +	.reset_reg				= 0x2f,
> +	.output_frequency_setting_reg		= 0x27,
> +	.pwm_register_base			= 0x04,
> +	.led_control_register_base		= 0x17,
> +	.enable_bits_per_led_control_register	= 1,
> +	.brightness_steps			= 256,
> +	.enable_all_channels_func		= is31fl3206_enable_all_channels,
> +};
> +
>  static const struct of_device_id of_is31fl32xx_match[] = {
> +	{ .compatible = "issi,is31fl3206",  .data = &is31fl3206_cdef, },
>  	{ .compatible = "issi,is31fl3293",  .data = &is31fl3293_cdef, },
>  	{ .compatible = "issi,is31fl3236",  .data = &is31fl3236_cdef, },

[Severity: Medium]
This adds the "issi,is31fl3206" string to the of_device_id match table, but
shouldn't a corresponding "is31fl3206" entry also be added to the is31fl32xx_id
array (the i2c_device_id table) further down in the file?

Does omitting this entry break driver auto-loading via I2C modalias and
sysfs-based device instantiation on non-DT platforms?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013-add-is31fl3206-support-v1-0-08c066cc241d@amieiro.net?part=1

[PATCH 2/2] dt-bindings: leds: Add issi,is31fl3206 to leds-is31fl32xx

[Flavio Amieiro]

This is a 12 channel variant of the is31fl32xx led driver series.

Signed-off-by: Flavio Amieiro <flavio@amieiro.net>
---
 Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt b/Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt
index 7082ed186dd9..06f85a1c6aaf 100644
--- a/Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt
+++ b/Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt
@@ -11,6 +11,7 @@ Required properties:
 	issi,is31fl3218
 	issi,is31fl3216
 	issi,is31fl3293
+	issi,is31fl3206
 	si-en,sn3218
 	si-en,sn3216
 - reg: I2C slave address

-- 
2.53.0

Re: [PATCH 2/2] dt-bindings: leds: Add issi,is31fl3206 to leds-is31fl32xx

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] leds: Missing I2C device ID for is31fl3206 breaks legacy binding and modaliases
- [Low] leds: Double I2C writes during initialization in is31fl32xx_enable_all_channels
--

commit 4f6d324bacf9d9547e9fba515fa86ec07e7ad36d
Author: Flavio Amieiro <flavio@amieiro.net>

leds: is31f132xx: Add support for is31fl3206

This commit adds support for the 12-channel IS31FL3206 variant to the
is31fl32xx led driver series. It introduces a custom enable sequence for the
new chip since its control register layout differs from existing variants.

> diff --git a/drivers/leds/leds-is31fl32xx.c b/drivers/leds/leds-is31fl32xx.c
> index 285085d274538..9c09548acabfa 100644
> --- a/drivers/leds/leds-is31fl32xx.c
> +++ b/drivers/leds/leds-is31fl32xx.c

[ ... ]

> @@ -315,6 +342,33 @@ static int is31fl32xx_reset_regs(struct is31fl32xx_priv *priv)
> +static int is31fl32xx_enable_all_channels(struct is31fl32xx_priv *priv)
> +{
> +	const struct is31fl32xx_chipdef *cdef = priv->cdef;
> +	int ret;
> +
> +	if (cdef->led_control_register_base != IS31FL32XX_REG_NONE) {
> +		u8 value =
> +			GENMASK(cdef->enable_bits_per_led_control_register-1, 0);
> +		u8 num_regs = cdef->channels /
> +			cdef->enable_bits_per_led_control_register;
> +		int i;
> +
> +		for (i = 0; i < num_regs; i++) {
> +			ret = is31fl32xx_write(priv,
> +					       cdef->led_control_register_base+i,
> +					       value);
> +			if (ret)
> +				return ret;
> +		}
> +	}
> +
> +	if (cdef->enable_all_channels_func)
> +		return cdef->enable_all_channels_func(priv);

[Severity: Low]
Does this function perform redundant I2C writes during initialization?

Because the is31fl3206 variant defines a valid led_control_register_base
(0x17), the generic initialization loop executes first, writing 0x01 to the
12 registers.

Immediately following this loop, the custom callback is executed:

	if (cdef->enable_all_channels_func)
		return cdef->enable_all_channels_func(priv);

The custom callback is31fl3206_enable_all_channels() then loops over those
exact same 12 registers and overwrites them with the correct value (0x10).

Should the generic loop be placed in an else block, or should the custom
override function be called first and return early to avoid wasting bus
bandwidth and briefly configuring the chip with incorrect values?

> +
> +	return 0;
> +}

[ ... ]

> @@ -552,7 +594,22 @@ static const struct is31fl32xx_chipdef is31fl3293_cdef = {
>  	.sw_shutdown_func			= is31fl3293_software_shutdown,
>  };
>  
> +static const struct is31fl32xx_chipdef is31fl3206_cdef = {
> +	.channels				= 12,
> +	.shutdown_reg				= 0x00,
> +	.pwm_update_reg			= 0x13,
> +	.global_control_reg			= 0x26,
> +	.reset_reg				= 0x2f,
> +	.output_frequency_setting_reg		= 0x27,
> +	.pwm_register_base			= 0x04,
> +	.led_control_register_base		= 0x17,
> +	.enable_bits_per_led_control_register	= 1,
> +	.brightness_steps			= 256,
> +	.enable_all_channels_func		= is31fl3206_enable_all_channels,
> +};
> +
>  static const struct of_device_id of_is31fl32xx_match[] = {
> +	{ .compatible = "issi,is31fl3206",  .data = &is31fl3206_cdef, },

[Severity: Medium]
Is the new is31fl3206 variant missing from the i2c_device_id table?

While the device tree of_device_id table is updated here, the I2C device ID
array is31fl32xx_id does not appear to have a corresponding entry for
is31fl3206.

Could this prevent the driver from auto-loading on systems that rely on
legacy I2C modaliases, and prevent manual instantiation via the I2C
new_device sysfs interface?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013-add-is31fl3206-support-v1-0-08c066cc241d@amieiro.net?part=2

Re: [PATCH 2/2] dt-bindings: leds: Add issi,is31fl3206 to leds-is31fl32xx

[Krzysztof Kozlowski]

On Tue, Oct 13, 2026 at 12:33:15AM -0300, Flavio Amieiro wrote:
> This is a 12 channel variant of the is31fl32xx led driver series.
> 
> Signed-off-by: Flavio Amieiro <flavio@amieiro.net>
> ---
>  Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt | 1 +
>  1 file changed, 1 insertion(+)

Please organize the patch documenting the compatible (DT bindings)
before the patch using that compatible.
See also: https://elixir.bootlin.com/linux/v6.14-rc6/source/Documentation/devicetree/bindings/submitting-patches.rst#L46

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>

Best regards,
Krzysztof

Re: [PATCH 2/2] dt-bindings: leds: Add issi,is31fl3206 to leds-is31fl32xx

[Flávio Amieiro]

Krzysztof Kozlowski <krzk@kernel.org> writes:
> Please organize the patch documenting the compatible (DT bindings)
> before the patch using that compatible.

I'm sorry, I read this e-mail after sending v2 of the patch series. I
will organize the commits and send v3.

Thank you for your review!

Best regards,
Flávio Amieiro

Re: [PATCH 2/2] dt-bindings: leds: Add issi,is31fl3206 to leds-is31fl32xx

[Krzysztof Kozlowski]

On 13/10/2026 11:58, Flávio Amieiro wrote:
> Krzysztof Kozlowski <krzk@kernel.org> writes:
>> Please organize the patch documenting the compatible (DT bindings)
>> before the patch using that compatible.
> 
> I'm sorry, I read this e-mail after sending v2 of the patch series. I
> will organize the commits and send v3.

v1 was sent today, so how could review be after v2?

One version per 24h, so you actually allow people to look at this code.

Best regards,
Krzysztof

Re: [PATCH 2/2] dt-bindings: leds: Add issi,is31fl3206 to leds-is31fl32xx

[Flávio Amieiro]

Krzysztof Kozlowski <krzk@kernel.org> writes:
> v1 was sent today, so how could review be after v2?

I sent it last night (in my timezone) and saw Shashiko reviews that
needed addressing in the morning. That's why I sent v2.

>
> One version per 24h, so you actually allow people to look at this code.
>

I didn't know about this rule, but of course it makes a lot of
sense. I'm sorry about the noise in the list, and I will for sure keep
this in mind for the future.

Best regards,
Flavio Amieiro