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

[Lee Jones <lee@kernel.org>]

--- checkpatch.pl (1 issue) ---
Line 83: WARNING: DT compatible string "issi,is31fl3206" appears un-documented -- check ./Documentation/devicetree/bindings/
----------------------------------

On Wed, 28 Oct 2026, Flavio Amieiro wrote:

> 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.
>
> Signed-off-by: Flavio Amieiro <flavio@amieiro.net>
> ---
>  drivers/leds/leds-is31fl32xx.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
>
> diff --git a/drivers/leds/leds-is31fl32xx.c b/drivers/leds/leds-is31fl32xx.c
> index 285085d27453..586713ff7d98 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 bit 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_bit_offset_in_led_control_reg determines the offset of the
> + * enable bits from the least significant bit of the LED control

Make sure you run W=1, particularly when changing Kerneldoc.

> + * register. This is needed for chips that have a LED Control Register
> + * that control 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);
> @@ -351,6 +359,7 @@ static int is31fl32xx_init_regs(struct is31fl32xx_priv *priv)
>  	if (cdef->led_control_register_base != IS31FL32XX_REG_NONE) {
>  		u8 value =
>  		    GENMASK(cdef->enable_bits_per_led_control_register-1, 0);

Feel free to unwrap this - you can use up to 100-chars to avoid this.

> +		value = value << cdef->enable_bits_offset_in_led_control_reg;

Could we avoid mixing declarations and statements here?

Also, instead of shifting afterwards, could we just pass the offset
directly to 'GENMASK()', or use '<<='?

>  		u8 num_regs = cdef->channels /
>  				cdef->enable_bits_per_led_control_register;
>  		int i;
> @@ -552,7 +561,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, },
> @@ -616,6 +640,7 @@ static void is31fl32xx_remove(struct i2c_client *client)
>   * even though it is not used for DeviceTree based instantiation.
>   */
>  static const struct i2c_device_id is31fl32xx_id[] = {
> +	{ .name = "is31fl3206" },

Looks like this is going to need rebasing on -next to apply.

>  	{ .name = "is31fl3293" },
>  	{ .name = "is31fl3236" },
>  	{ .name = "is31fl3236a" },
>
> -- 
> 2.53.0
>

-- 
Lee Jones