lkml.org 
[lkml]   [2019]   [Aug]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 1/3] leds: lm3532: Fix brightness control for i2c mode
From
Date
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
>

\
 
 \ /
  Last update: 2019-08-02 02:03    [W:0.177 / U:0.012 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site