Re: [PATCH v7 6/6] leds: is31fl32xx: Move pwm frequency setting to init_regs()

[Lee Jones <lee@kernel.org>]

On Wed, 20 May 2026, Jun Yan wrote:

> commit a18983b95a61 ("leds: is31f132xx: Add support for is31fl3293")
> swapped the order of is31fl32xx_parse_dt() and is31fl32xx_init_regs().
> 
> This causes the PWM frequency configuration programmed in
> is31fl32xx_parse_dt() to be overwritten by the register reset
> operation performed in is31fl32xx_init_regs().
> 
> Move the PWM frequency setting logic from is31fl32xx_parse_dt() to
> is31fl32xx_init_regs() and separates device tree parsing from hardware
> initialization.
> 
> Fixes: a18983b95a61 ("leds: is31f132xx: Add support for is31fl3293")
> Signed-off-by: Jun Yan <jerrysteve1101@gmail.com>
> ---
>  drivers/leds/leds-is31fl32xx.c | 22 ++++++++++------------
>  1 file changed, 10 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/leds/leds-is31fl32xx.c b/drivers/leds/leds-is31fl32xx.c
> index 2d7ff4c5a08c..c545a3b7a05a 100644
> --- a/drivers/leds/leds-is31fl32xx.c
> +++ b/drivers/leds/leds-is31fl32xx.c
> @@ -63,6 +63,7 @@ struct is31fl32xx_priv {
>  	struct i2c_client *client;
>  	struct gpio_desc *powerdown_gpio;
>  	unsigned int num_leds;
> +	bool pwm_22khz;
>  	struct is31fl32xx_led_data leds[];
>  };
>  
> @@ -346,6 +347,14 @@ static int is31fl32xx_init_regs(struct is31fl32xx_priv *priv)
>  	if (ret)
>  		return ret;
>  
> +	if ((cdef->output_frequency_setting_reg != IS31FL32XX_REG_NONE) &&
> +		priv->pwm_22khz) {

Nit: No need to wrap here.  Use up to 100-chars to beautify.

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

Nit: Line-up with the '('.

> +		if (ret)
> +			return ret;
> +	}
> +
>  	/*
>  	 * Set enable bit for all channels.
>  	 * We will control state with PWM registers alone.
> @@ -420,7 +429,6 @@ 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;
>  
>  	/* Driving this GPIO line low takes the chip out of shutdown,
> @@ -431,17 +439,7 @@ static int is31fl32xx_parse_dt(struct device *dev,
>  		return dev_err_probe(dev, PTR_ERR(priv->powerdown_gpio),
>  				"Failed to get powerdown gpio\n");
>  
> -	if ((cdef->output_frequency_setting_reg != IS31FL32XX_REG_NONE) &&
> -	    of_property_read_bool(dev_of_node(dev), "issi,22khz-pwm")) {
> -
> -		ret = is31fl32xx_write(priv, cdef->output_frequency_setting_reg,
> -				       IS31FL32XX_PWM_FREQUENCY_22KHZ);
> -
> -		if (ret) {
> -			dev_err(dev, "Failed to write output PWM frequency register\n");
> -			return ret;
> -		}
> -	}
> +	priv->pwm_22khz = of_property_read_bool(dev_of_node(dev), "issi,22khz-pwm");
>  
>  	for_each_available_child_of_node_scoped(dev_of_node(dev), child) {
>  		struct led_init_data init_data = {};
> -- 
> 2.54.0
> 
> 

-- 
Lee Jones