Messages in this thread | | | Subject | Re: [PATCH 1/3] leds: lm3532: Fix brightness control for i2c mode | From | Dan Murphy <> | Date | Thu, 1 Aug 2019 19:02:20 -0500 |
| |
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 >
| |