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

[Dan Murphy <dmurphy@ti.com>]

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
>