Re: [PATCH 1/2] leds/leds-is31fl32xx: add support for is31fl3236a

[Lee Jones <lee@kernel.org>]

On Thu, 19 Jun 2025, Pawel Zalewski wrote:

> This commit is adding an additional and optional control
> register for setting the output PWM frequency to 22kHz.
> The default is 3kHz and this option puts the operational
> frequency outside of the audible range. The control over this
> parameter was added via the device tree parser function,
> as really whether to use this functionality or not would
> depend on the board.
> 
> Signed-off-by: Pawel Zalewski <pzalewski@thegoodpenguin.co.uk>
> ---
>  drivers/leds/leds-is31fl32xx.c | 35 ++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/drivers/leds/leds-is31fl32xx.c b/drivers/leds/leds-is31fl32xx.c
> index 8793330dd414..d23260f3f6ce 100644
> --- a/drivers/leds/leds-is31fl32xx.c
> +++ b/drivers/leds/leds-is31fl32xx.c
> @@ -32,6 +32,8 @@
>  #define IS31FL3216_CONFIG_SSD_ENABLE  BIT(7)
>  #define IS31FL3216_CONFIG_SSD_DISABLE 0
>  
> +#define IS31FL32XX_PWM_FREQUENCY_22kHz  0x01
> +
>  struct is31fl32xx_priv;
>  struct is31fl32xx_led_data {
>  	struct led_classdev cdev;
> @@ -57,6 +59,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
> + * @output_frequency_setting_register: address of outupt frequency register (optional)
>   * @reset_func          : pointer to reset function
>   * @sw_shutdown_func    : pointer to software shutdown function
>   *
> @@ -80,6 +83,7 @@ struct is31fl32xx_chipdef {
>  	bool	pwm_registers_reversed;
>  	u8	led_control_register_base;
>  	u8	enable_bits_per_led_control_register;
> +	u8	output_frequency_setting_register;
>  	int (*reset_func)(struct is31fl32xx_priv *priv);
>  	int (*sw_shutdown_func)(struct is31fl32xx_priv *priv, bool enable);
>  };
> @@ -93,6 +97,19 @@ static const struct is31fl32xx_chipdef is31fl3236_cdef = {
>  	.pwm_register_base			= 0x01,
>  	.led_control_register_base		= 0x26,
>  	.enable_bits_per_led_control_register	= 1,
> +	.output_frequency_setting_register = IS31FL32XX_REG_NONE,

Line up with the others please </ocd_voice>

> +};
> +
> +static const struct is31fl32xx_chipdef is31fl3236a_cdef = {
> +	.channels				= 36,
> +	.shutdown_reg				= 0x00,
> +	.pwm_update_reg				= 0x25,
> +	.global_control_reg			= 0x4a,
> +	.reset_reg				= 0x4f,
> +	.pwm_register_base			= 0x01,
> +	.led_control_register_base		= 0x26,
> +	.enable_bits_per_led_control_register	= 1,
> +	.output_frequency_setting_register = 0x4b,

As above.

>  };
>  
>  static const struct is31fl32xx_chipdef is31fl3235_cdef = {
> @@ -104,6 +121,7 @@ static const struct is31fl32xx_chipdef is31fl3235_cdef = {
>  	.pwm_register_base			= 0x05,
>  	.led_control_register_base		= 0x2a,
>  	.enable_bits_per_led_control_register	= 1,
> +	.output_frequency_setting_register = IS31FL32XX_REG_NONE,

Here too - and everywhere.

>  };
>  
>  static const struct is31fl32xx_chipdef is31fl3218_cdef = {
> @@ -115,6 +133,7 @@ static const struct is31fl32xx_chipdef is31fl3218_cdef = {
>  	.pwm_register_base			= 0x01,
>  	.led_control_register_base		= 0x13,
>  	.enable_bits_per_led_control_register	= 6,
> +	.output_frequency_setting_register = IS31FL32XX_REG_NONE,
>  };
>  
>  static int is31fl3216_reset(struct is31fl32xx_priv *priv);
> @@ -130,6 +149,7 @@ static const struct is31fl32xx_chipdef is31fl3216_cdef = {
>  	.pwm_registers_reversed			= true,
>  	.led_control_register_base		= 0x01,
>  	.enable_bits_per_led_control_register	= 8,
> +	.output_frequency_setting_register = IS31FL32XX_REG_NONE,

How do you sleep at night!   =;-)

>  	.reset_func				= is31fl3216_reset,
>  	.sw_shutdown_func			= is31fl3216_software_shutdown,
>  };
> @@ -363,8 +383,21 @@ static struct is31fl32xx_led_data *is31fl32xx_find_led_data(
>  static int is31fl32xx_parse_dt(struct device *dev,
>  			       struct is31fl32xx_priv *priv)
>  {
> +	const struct is31fl32xx_chipdef *cdef = priv->cdef;
>  	int ret = 0;
>  
> +	if (of_property_read_bool(dev_of_node(dev), "is31fl32xx,22kHz-pwm")
> +	&& (cdef->output_frequency_setting_register != IS31FL32XX_REG_NONE)) {

Please the '&&' and the end of the previous line, then indent this one.

> +
> +		ret = is31fl32xx_write(priv, cdef->output_frequency_setting_register,
> +							IS31FL32XX_PWM_FREQUENCY_22kHz);

Line-up with the '('.

> +
> +		if (ret) {
> +			dev_err(dev, "Failed to write output pwm frequency register\n");

"PWM"

> +			return ret;
> +		}
> +	}
> +
>  	for_each_available_child_of_node_scoped(dev_of_node(dev), child) {
>  		struct led_init_data init_data = {};
>  		struct is31fl32xx_led_data *led_data =
> @@ -405,6 +438,7 @@ static int is31fl32xx_parse_dt(struct device *dev,
>  
>  static const struct of_device_id of_is31fl32xx_match[] = {
>  	{ .compatible = "issi,is31fl3236", .data = &is31fl3236_cdef, },
> +	{ .compatible = "issi,is31fl3236a", .data = &is31fl3236a_cdef, },
>  	{ .compatible = "issi,is31fl3235", .data = &is31fl3235_cdef, },
>  	{ .compatible = "issi,is31fl3218", .data = &is31fl3218_cdef, },
>  	{ .compatible = "si-en,sn3218",    .data = &is31fl3218_cdef, },
> @@ -466,6 +500,7 @@ static void is31fl32xx_remove(struct i2c_client *client)
>   */
>  static const struct i2c_device_id is31fl32xx_id[] = {
>  	{ "is31fl3236" },
> +	{ "is31fl3236a" },
>  	{ "is31fl3235" },
>  	{ "is31fl3218" },
>  	{ "sn3218" },
> -- 
> 2.48.1
> 
> 

-- 
Lee Jones [李琼斯]