Re: [PATCH 1/3] leds: lm3532: Fix brightness control for i2c mode

[Dan Murphy]

Pavel

Thanks for the review

On 8/1/19 4:36 PM, Pavel Machek wrote:
> Hi!
>
> If we are going to complain about coding style... this should really
> be split, one change per patch.
>
> > > @@ -161,18 +167,18 @@ struct lm3532_data {
> > >   };
> > >   
> > >   static const struct reg_default lm3532_reg_defs[] = {
> > > -	{LM3532_REG_OUTPUT_CFG, 0xe4},
> > > +	{LM3532_REG_OUTPUT_CFG, 0x24},
> > >   	{LM3532_REG_STARTSHUT_RAMP, 0xc0},
> > >   	{LM3532_REG_RT_RAMP, 0xc0},
> > >   	{LM3532_REG_PWM_A_CFG, 0x82},
> > >   	{LM3532_REG_PWM_B_CFG, 0x82},
> > >   	{LM3532_REG_PWM_C_CFG, 0x82},
> > >   	{LM3532_REG_ZONE_CFG_A, 0xf1},
> > > -	{LM3532_REG_CTRL_A_BRT, 0xf3},
> > > +	{LM3532_REG_CTRL_A_FS_CURR, 0x13},
> > >   	{LM3532_REG_ZONE_CFG_B, 0xf1},
> > > -	{LM3532_REG_CTRL_B_BRT, 0xf3},
> > > +	{LM3532_REG_CTRL_B_FS_CURR, 0x13},
> > >   	{LM3532_REG_ZONE_CFG_C, 0xf1},
> > > -	{LM3532_REG_CTRL_C_BRT, 0xf3},
> > > +	{LM3532_REG_CTRL_C_FS_CURR, 0x13},
> > >   	{LM3532_REG_ENABLE, 0xf8},
> > >   	{LM3532_ALS_CONFIG, 0x44},
> > >   	{LM3532_REG_ZN_0_HI, 0x35},
> Default register values; are they related to the rest?

Yes and no. I changed the #define so we would see a change anyway.

And the default is 0x13.

I can move it to a clean up patch

> > > @@ -302,7 +308,7 @@ static int lm3532_led_disable(struct lm3532_led *led_data)
> > >   	int ret;
> > >   
> > >   	ret = regmap_update_bits(led_data->priv->regmap, LM3532_REG_ENABLE,
> > > -					 ctrl_en_val, ~ctrl_en_val);
> > > +					 ctrl_en_val, 0);
> > >   	if (ret) {
> > >   		dev_err(led_data->priv->dev, "Failed to set ctrl:%d\n", ret);
> > >   		return ret;
> This should have no functional impact, its just a clenaup, probably
> should go separately.

I took it from your patch.  Thought it was a good clean up.

I can move it to a separate patch and give you credit


> > > @@ -339,11 +345,9 @@ static int lm3532_brightness_set(struct led_classdev *led_cdev,
> > >   	if (ret)
> > >   		goto unlock;
> > >   
> > > -	brightness_reg = LM3532_REG_CTRL_A_BRT + led->control_bank * 2;
> > > -	brt_val = brt_val / LM3532_BRT_VAL_ADJUST;
> > > -
> > > +	brightness_reg = LM3532_REG_ZONE_TRGT_A + led->control_bank * 5 +
> > > +			 (led->ctrl_brt_pointer >> 2);
> > >   	ret = regmap_write(led->priv->regmap, brightness_reg, brt_val);
> > > -
> > >   unlock:
> > >   	mutex_unlock(&led->priv->lock);
> > >   	return ret;
> This is the core change, AFAICT.

Yep.  This is the fix you want.


> > > @@ -356,8 +360,29 @@ static int lm3532_init_registers(struct lm3532_led *led)
> > >   	unsigned int output_cfg_val = 0;
> > >   	unsigned int output_cfg_shift = 0;
> > >   	unsigned int output_cfg_mask = 0;
> > > +	int brightness_config_reg;
> > > +	int brightness_config_val;
> > >   	int ret, i;
> > >   
> > > +	if (drvdata->enable_gpio)
> > > +		gpiod_direction_output(drvdata->enable_gpio, 1);
> > > +
> > > +	brightness_config_reg = LM3532_REG_ZONE_CFG_A + led->control_bank * 2;
> > > +	/* This could be hard coded to the default value but the control
> Code is moved, probably should go in separately. We'll have less fun
> bisecting problems when things are separate...

On my Droid4 moving this enable call allowed the init to pass without a 
regmap failure.

But I did not see the same issue on the BBB with the LM3532 EVM.

Without this change in this patch the backlight failed to register.  I 
think we want to keep this change here.

Dan


> Thanks and best regards,
> 									Pavel