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

[Jun Yan <jerrysteve1101@gmail.com>]

> 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.

OK.

> 
> > +		ret = is31fl32xx_write(priv, cdef->output_frequency_setting_reg,
> > +							IS31FL32XX_PWM_FREQUENCY_22KHZ);
> 
> Nit: Line-up with the '('.

Got it.

Thanks for the review, will fix everything in v8.

> 
> > +		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
>