| Message ID | 20190813181154.6614-4-dmurphy@ti.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/4] leds: lm3532: Fix brightness control for i2c mode | expand |
Hi! > Allow the full scale current to be configured at init. > Valid rangles are 5mA->29.8mA. > > Signed-off-by: Dan Murphy <dmurphy@ti.com> > @@ -121,6 +125,7 @@ struct lm3532_als_data { > * @mode - Mode of the LED string > * @ctrl_brt_pointer - Zone target register that controls the sink > * @num_leds - Number of LED strings are supported in this array > + * @full_scale_current - The full-scale current setting for the current sink. > * @led_strings - The LED strings supported in this array > * @label - LED label > */ > @@ -130,8 +135,9 @@ struct lm3532_led { > > int control_bank; > int mode; > - int ctrl_brt_pointer; > int num_leds; > + int ctrl_brt_pointer; > + int full_scale_current; > u32 led_strings[LM3532_MAX_CONTROL_BANKS]; > char label[LED_MAX_NAME_SIZE]; > }; No need to move ctrl_brt_pointer... to keep order consistent with docs. > + fs_current_val = led->full_scale_current - LM3532_FS_CURR_MIN / > + LM3532_FS_CURR_STEP; The computation is wrong ... needs () AFAICT. Best regards, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Pavel Thanks for the review On 8/19/19 5:55 AM, Pavel Machek wrote: > Hi! > >> Allow the full scale current to be configured at init. >> Valid rangles are 5mA->29.8mA. >> >> Signed-off-by: Dan Murphy <dmurphy@ti.com> >> @@ -121,6 +125,7 @@ struct lm3532_als_data { >> * @mode - Mode of the LED string >> * @ctrl_brt_pointer - Zone target register that controls the sink >> * @num_leds - Number of LED strings are supported in this array >> + * @full_scale_current - The full-scale current setting for the current sink. >> * @led_strings - The LED strings supported in this array >> * @label - LED label >> */ >> @@ -130,8 +135,9 @@ struct lm3532_led { >> >> int control_bank; >> int mode; >> - int ctrl_brt_pointer; >> int num_leds; >> + int ctrl_brt_pointer; >> + int full_scale_current; >> u32 led_strings[LM3532_MAX_CONTROL_BANKS]; >> char label[LED_MAX_NAME_SIZE]; >> }; > No need to move ctrl_brt_pointer... to keep order consistent with docs. OK I will reset the patches and get rid of that change. I think this got moved when I applied the v1 patch. >> + fs_current_val = led->full_scale_current - LM3532_FS_CURR_MIN / >> + LM3532_FS_CURR_STEP; > The computation is wrong ... needs () AFAICT. Hmm. Doesn't order of operations take precedence? I will add the () unless checkpatch cribs about them Dan > > Best regards, > Pavel
Hi! > >No need to move ctrl_brt_pointer... to keep order consistent with docs. > > OK I will reset the patches and get rid of that change. I think this got > moved when I applied the v1 patch. > > > >>+ fs_current_val = led->full_scale_current - LM3532_FS_CURR_MIN / > >>+ LM3532_FS_CURR_STEP; > >The computation is wrong ... needs () AFAICT. > > Hmm. Doesn't order of operations take precedence? > > I will add the () unless checkpatch cribs about them I may be misunderstanding. What do you expect the computation to be? / has higher priority than -, right? Can you test it provides expected results? Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Hello On 8/20/19 11:29 AM, Pavel Machek wrote: > Hi! > >>> No need to move ctrl_brt_pointer... to keep order consistent with docs. >> OK I will reset the patches and get rid of that change. I think this got >> moved when I applied the v1 patch. >> >> >>>> + fs_current_val = led->full_scale_current - LM3532_FS_CURR_MIN / >>>> + LM3532_FS_CURR_STEP; >>> The computation is wrong ... needs () AFAICT. >> Hmm. Doesn't order of operations take precedence? >> >> I will add the () unless checkpatch cribs about them > I may be misunderstanding. What do you expect the computation to be? / > has higher priority than -, right? Can you test it provides expected > results? Fixed. I think this is what you expected. fs_current_val = (led->full_scale_current - LM3532_FS_CURR_MIN) / LM3532_FS_CURR_STEP; > Pavel
diff --git a/drivers/leds/leds-lm3532.c b/drivers/leds/leds-lm3532.c index ef4c74cbcdc0..dabf4be1e8c8 100644 --- a/drivers/leds/leds-lm3532.c +++ b/drivers/leds/leds-lm3532.c @@ -89,6 +89,10 @@ #define LM3532_NUM_AVG_VALS 8 #define LM3532_NUM_IMP_VALS 32 +#define LM3532_FS_CURR_MIN 5000 +#define LM3532_FS_CURR_MAX 29800 +#define LM3532_FS_CURR_STEP 800 + /* * struct lm3532_als_data * @config - value of ALS configuration register @@ -121,6 +125,7 @@ struct lm3532_als_data { * @mode - Mode of the LED string * @ctrl_brt_pointer - Zone target register that controls the sink * @num_leds - Number of LED strings are supported in this array + * @full_scale_current - The full-scale current setting for the current sink. * @led_strings - The LED strings supported in this array * @label - LED label */ @@ -130,8 +135,9 @@ struct lm3532_led { int control_bank; int mode; - int ctrl_brt_pointer; int num_leds; + int ctrl_brt_pointer; + int full_scale_current; u32 led_strings[LM3532_MAX_CONTROL_BANKS]; char label[LED_MAX_NAME_SIZE]; }; @@ -363,6 +369,8 @@ static int lm3532_init_registers(struct lm3532_led *led) unsigned int output_cfg_mask = 0; unsigned int brightness_config_reg; unsigned int brightness_config_val; + int fs_current_reg; + int fs_current_val; int ret, i; if (drvdata->enable_gpio) @@ -385,6 +393,16 @@ static int lm3532_init_registers(struct lm3532_led *led) if (ret) return ret; + if (led->full_scale_current) { + fs_current_reg = LM3532_REG_CTRL_A_FS_CURR + led->control_bank * 2; + fs_current_val = led->full_scale_current - LM3532_FS_CURR_MIN / + LM3532_FS_CURR_STEP; + ret = regmap_write(drvdata->regmap, fs_current_reg, + fs_current_val); + if (ret) + return ret; + } + for (i = 0; i < led->num_leds; i++) { output_cfg_shift = led->led_strings[i] * 2; output_cfg_val |= (led->control_bank << output_cfg_shift); @@ -560,6 +578,12 @@ static int lm3532_parse_node(struct lm3532_data *priv) goto child_out; } + ret = fwnode_property_read_u32(child, "led-max-microamp", + &led->full_scale_current); + + if (led->full_scale_current > LM3532_FS_CURR_MAX) + led->full_scale_current = LM3532_FS_CURR_MAX; + if (led->mode == LM3532_BL_MODE_ALS) { led->mode = LM3532_ALS_CTRL; ret = lm3532_parse_als(priv);
Allow the full scale current to be configured at init. Valid rangles are 5mA->29.8mA. Signed-off-by: Dan Murphy <dmurphy@ti.com> --- v2 - Change ti,fs-current to led-max-microamp - https://lore.kernel.org/patchwork/patch/1109503/ drivers/leds/leds-lm3532.c | 26 +++++++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) -- 2.22.0.214.g8dca754b1e