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

[PATCH v5 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.

This patch allows the chipdef to define at which offset the LED enable
bits are inside the LED Control Register, so when calculating the
value to write to the register we can offset it to the correct
position in the register for ICs like the IS31FL3206.

The patches keep the behavior of the drivers for the other ICs in the
family: initializing the LEDs by enabling all of them and setting
output current to maximum and then using only the PWM registers to
control the brightness of the LEDs.

Signed-off-by: Flavio Amieiro <flavio@amieiro.net>
---
Changes in v5:
- Fix spelling in the documentation for the new attribute
- Rebase onto newest for-leds-next
- Use assignment operator for left shift
- Remove unecessary line wrap
- Link to v4: https://patch.msgid.link/20261028-add-is31fl3206-support-v4-0-e331cee9fcb3@amieiro.net

Changes in v4:
- Changes approach from pointers to functions (used in v3) to
  describing the LED control register layout.
- Link to v3: https://patch.msgid.link/20261013-add-is31fl3206-support-v3-0-66a762a3c12f@amieiro.net

Changes in v3:
- Reorganizes the commits so the dt-bindings are documented before using
  them
- Link to v2: https://patch.msgid.link/20261013-add-is31fl3206-support-v2-0-ca0d2bf5783f@amieiro.net

Changes in v2:
- Avoid double initialization when enable_all_channels is non-NULL
- Add missing entry in the i2c_device_id table
- Link to v1: https://patch.msgid.link/20261013-add-is31fl3206-support-v1-0-08c066cc241d@amieiro.net

To: Lee Jones <lee@kernel.org>
To: Pavel Machek <pavel@kernel.org>
To: Griffin Kroah-Hartman <griffin@kroah.com>
To: Rob Herring <robh@kernel.org>
To: Krzysztof Kozlowski <krzk+dt@kernel.org>
To: Conor Dooley <conor+dt@kernel.org>
Cc: linux-leds@vger.kernel.org
Cc: devicetree@vger.kernel.org
Cc: linux-kernel@vger.kernel.org

---
Flavio Amieiro (2):
      dt-bindings: leds: Add issi,is31fl3206 to leds-is31fl32xx
      leds: is31f132xx: Add support for is31fl3206

 .../devicetree/bindings/leds/leds-is31fl32xx.txt   |  1 +
 drivers/leds/leds-is31fl32xx.c                     | 30 ++++++++++++++++++++--
 2 files changed, 29 insertions(+), 2 deletions(-)
---
base-commit: 4e88404a72e5e26720539e5555c383069cefb486
change-id: 20261012-add-is31fl3206-support-d557f863a97a

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

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

[Flavio Amieiro]

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

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
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

[PATCH v5 2/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 (in the lowest
nibble).

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 allowing the chipdef to define an
offset for the enable bits inside the LED Control Register.

The limitation that already existed for other variants is still in
this one: all LEDs are enabled and set with maximum current, letting
brightness be controlled by the PWM registers.

Reviewed-by: Griffin Kroah-Hartman <griffin@kroah.com>
Signed-off-by: Flavio Amieiro <flavio@amieiro.net>
---
 drivers/leds/leds-is31fl32xx.c | 30 ++++++++++++++++++++++++++++--
 1 file changed, 28 insertions(+), 2 deletions(-)

diff --git a/drivers/leds/leds-is31fl32xx.c b/drivers/leds/leds-is31fl32xx.c
index 581c05bab969..1c76b0fb1f15 100644
--- a/drivers/leds/leds-is31fl32xx.c
+++ b/drivers/leds/leds-is31fl32xx.c
@@ -77,6 +77,7 @@ struct is31fl32xx_priv {
  * @pwm_registers_reversed: : true if PWM registers count down instead of up
  * @led_control_register_base : address of first LED control register (optional)
  * @enable_bits_per_led_control_register: number of LEDs enable bits in each
+ * @enable_bits_offset_in_led_control_reg: offset of the enable bits in the LED Control Register
  * @brightness_steps    : number of brightness steps supported by the chip
  * @reset_func          : pointer to reset function
  * @sw_shutdown_func    : pointer to software shutdown function
@@ -88,6 +89,12 @@ struct is31fl32xx_priv {
  * necessary registers to a known initialization state. This is needed
  * for chips that do not have a @reset_reg.
  *
+ * @enable_bits_offset_in_led_control_reg determines the offset of the
+ * enable bits from the least significant bit of the LED control
+ * register. This is needed for chips that have a LED Control Register
+ * that controls more than simply the enable/disable state of the
+ * channel such as the IS31FL3206.
+ *
  * @enable_bits_per_led_control_register must be >=1 if
  * @led_control_register_base != %IS31FL32XX_REG_NONE.
  */
@@ -103,6 +110,7 @@ struct is31fl32xx_chipdef {
 	bool	pwm_registers_reversed;
 	u8	led_control_register_base;
 	u8	enable_bits_per_led_control_register;
+	u8	enable_bits_offset_in_led_control_reg;
 	u16	brightness_steps;
 	int (*reset_func)(struct is31fl32xx_priv *priv);
 	int (*sw_shutdown_func)(struct is31fl32xx_priv *priv, bool enable);
@@ -350,8 +358,10 @@ static int is31fl32xx_init_regs(struct is31fl32xx_priv *priv)
 	 * 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 value = GENMASK(cdef->enable_bits_per_led_control_register-1, 0);
+
+		value <<= cdef->enable_bits_offset_in_led_control_reg;
+
 		u8 num_regs = cdef->channels /
 				cdef->enable_bits_per_led_control_register;
 		int i;
@@ -553,7 +563,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,
+	.enable_bits_offset_in_led_control_reg	= 4,
+	.brightness_steps			= 256,
+};
+
 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, },
@@ -613,6 +638,7 @@ static void is31fl32xx_remove(struct i2c_client *client)
 }
 
 static const struct i2c_device_id is31fl32xx_id[] = {
+	{ .name = "is31fl3206", .driver_data = (kernel_ulong_t)&is31fl3206_cdef },
 	{ .name = "is31fl3293", .driver_data = (kernel_ulong_t)&is31fl3293_cdef },
 	{ .name = "is31fl3236", .driver_data = (kernel_ulong_t)&is31fl3236_cdef },
 	{ .name = "is31fl3236a", .driver_data = (kernel_ulong_t)&is31fl3236a_cdef },

-- 
2.53.0

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

[sashiko-bot]

> This is a 12 channel variant of the is31fl32xx led driver series.
> 
> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
> Signed-off-by: Flavio Amieiro <flavio@amieiro.net>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114-add-is31fl3206-support-v5-0-332db80233df@amieiro.net?part=1

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] leds: is31fl32xx: mixed declarations and code in
  is31fl32xx_init_regs()
--

commit 17585c728364ffbaee88a6c09e1a1085eccc5391
Author: Flavio Amieiro <flavio@amieiro.net>

leds: is31f132xx: Add support for is31fl3206

This commit adds support for the ISSI IS31FL3206 LED controller by allowing
the chip definition to specify a bit offset for the enable bits in the LED
control register.

> diff --git a/drivers/leds/leds-is31fl32xx.c b/drivers/leds/leds-is31fl32xx.c
> index 581c05bab9697..1c76b0fb1f159 100644
> --- a/drivers/leds/leds-is31fl32xx.c
> +++ b/drivers/leds/leds-is31fl32xx.c

[ ... ]

> @@ -350,8 +358,10 @@ static int is31fl32xx_init_regs(struct is31fl32xx_priv *priv)
>  	 * 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 value = GENMASK(cdef->enable_bits_per_led_control_register-1, 0);
> +
> +		value <<= cdef->enable_bits_offset_in_led_control_reg;
> +
>  		u8 num_regs = cdef->channels /
>  				cdef->enable_bits_per_led_control_register;
>  		int i;

[Severity: Low]
As reported by Lee Jones on v4 [1], placing the shift of value before the
declarations of num_regs and i mixes declarations and statements in
is31fl32xx_init_regs(). Could the shift be moved below the variable
declarations or folded into the initializer?

[1] https://lore.kernel.org/r/179370064610.793788.8239015594326628135@kernel.org

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114-add-is31fl3206-support-v5-0-332db80233df@amieiro.net?part=2

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

[Flávio Amieiro]

sashiko-bot@kernel.org writes:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue to consider:
> - [Low] leds: is31fl32xx: mixed declarations and code in
>   is31fl32xx_init_regs()
> --
> [Severity: Low]
> As reported by Lee Jones on v4 [1], placing the shift of value before the
> declarations of num_regs and i mixes declarations and statements in
> is31fl32xx_init_regs(). Could the shift be moved below the variable
> declarations or folded into the initializer?
...
> [1] https://lore.kernel.org/r/179370064610.793788.8239015594326628135@kernel.org


I missed that change in the new version. I'm sorry about that. I will
fix this, but I'll wait a while for more feedback before I send a new
version in case there is anything else to change.