[v2,4/4] leds: lm3532: Add full scale current configuration
diff mbox series

Message ID 20190813181154.6614-4-dmurphy@ti.com
State Superseded
Headers show
Series
  • [v2,1/4] leds: lm3532: Fix brightness control for i2c mode
Related show

Commit Message

Dan Murphy Aug. 13, 2019, 6:11 p.m. UTC
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(-)

Comments

Pavel Machek Aug. 19, 2019, 10:55 a.m. UTC | #1
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
Dan Murphy Aug. 20, 2019, 3:51 p.m. UTC | #2
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
Pavel Machek Aug. 20, 2019, 4:29 p.m. UTC | #3
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
Dan Murphy Aug. 20, 2019, 5:48 p.m. UTC | #4
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

Patch
diff mbox series

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);