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

[Lee Jones <lee@kernel.org>]

On Fri, 27 Jun 2025, Pawel Zalewski wrote:

> Add 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.
> 
> Signed-off-by: Pawel Zalewski <pzalewski@thegoodpenguin.co.uk>
> ---
>  drivers/leds/leds-is31fl32xx.c | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)

So close!

> diff --git a/drivers/leds/leds-is31fl32xx.c b/drivers/leds/leds-is31fl32xx.c
> index 8793330dd4142f49f15d6ee9d87468c08509859f..b3f25854f97eac0f87c5be762b1d8e3afaaecc21 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;
> @@ -53,6 +55,7 @@ struct is31fl32xx_priv {
>   * @pwm_update_reg      : address of PWM Update register
>   * @global_control_reg  : address of Global Control register (optional)
>   * @reset_reg           : address of Reset register (optional)
> + * @output_frequency_setting_reg: address of output frequency register (optional)
>   * @pwm_register_base   : address of first PWM register
>   * @pwm_registers_reversed: : true if PWM registers count down instead of up
>   * @led_control_register_base : address of first LED control register (optional)
> @@ -76,6 +79,7 @@ struct is31fl32xx_chipdef {
>  	u8	pwm_update_reg;
>  	u8	global_control_reg;
>  	u8	reset_reg;
> +	u8	output_frequency_setting_reg;
>  	u8	pwm_register_base;
>  	bool	pwm_registers_reversed;
>  	u8	led_control_register_base;
> @@ -90,6 +94,19 @@ static const struct is31fl32xx_chipdef is31fl3236_cdef = {
>  	.pwm_update_reg				= 0x25,
>  	.global_control_reg			= 0x4a,
>  	.reset_reg				= 0x4f,
> +	.output_frequency_setting_reg		= IS31FL32XX_REG_NONE,
> +	.pwm_register_base			= 0x01,
> +	.led_control_register_base		= 0x26,
> +	.enable_bits_per_led_control_register	= 1,
> +};
> +
> +static const struct is31fl32xx_chipdef is31fl3236a_cdef = {
> +	.channels				= 36,
> +	.shutdown_reg				= 0x00,
> +	.pwm_update_reg				= 0x25,
> +	.global_control_reg			= 0x4a,
> +	.reset_reg				= 0x4f,
> +	.output_frequency_setting_reg		= 0x4b,
>  	.pwm_register_base			= 0x01,
>  	.led_control_register_base		= 0x26,
>  	.enable_bits_per_led_control_register	= 1,
> @@ -101,6 +118,7 @@ static const struct is31fl32xx_chipdef is31fl3235_cdef = {
>  	.pwm_update_reg				= 0x25,
>  	.global_control_reg			= 0x4a,
>  	.reset_reg				= 0x4f,
> +	.output_frequency_setting_reg		= IS31FL32XX_REG_NONE,
>  	.pwm_register_base			= 0x05,
>  	.led_control_register_base		= 0x2a,
>  	.enable_bits_per_led_control_register	= 1,
> @@ -112,6 +130,7 @@ static const struct is31fl32xx_chipdef is31fl3218_cdef = {
>  	.pwm_update_reg				= 0x16,
>  	.global_control_reg			= IS31FL32XX_REG_NONE,
>  	.reset_reg				= 0x17,
> +	.output_frequency_setting_reg		= IS31FL32XX_REG_NONE,
>  	.pwm_register_base			= 0x01,
>  	.led_control_register_base		= 0x13,
>  	.enable_bits_per_led_control_register	= 6,
> @@ -126,6 +145,7 @@ static const struct is31fl32xx_chipdef is31fl3216_cdef = {
>  	.pwm_update_reg				= 0xB0,
>  	.global_control_reg			= IS31FL32XX_REG_NONE,
>  	.reset_reg				= IS31FL32XX_REG_NONE,
> +	.output_frequency_setting_reg		= IS31FL32XX_REG_NONE,
>  	.pwm_register_base			= 0x10,
>  	.pwm_registers_reversed			= true,
>  	.led_control_register_base		= 0x01,
> @@ -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 ((cdef->output_frequency_setting_reg != IS31FL32XX_REG_NONE) &&
> +	of_property_read_bool(dev_of_node(dev), "issi,22kHz-pwm")) {

This needs tabbing in.

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

This should line-up with the '(.

> +		if (ret) {
> +			dev_err(dev, "Failed to write output PWM frequency register\n");
> +			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 [李琼斯]