Skip to contentSkip to conversation

leds: lm3532: Fix brightness control for i2c mode

PATCH Applied Dan Murphy 4 participants 22 messages Aug 20, 2019 – Aug 28, 2019

Subsystems (2)
Device tree bindings LED
lore
Share & tools
lore

Dan Murphy

author
#0 lore raw

PATCH v3 1/5 leds: lm3532: Fix brightness control for i2c mode

Fix the brightness control for I2C mode. Instead of changing the full scale current register update the ALS target register for the appropriate banks.

In addition clean up some code errors and random misspellings found during coding.

Tested on Droid4 as well as LM3532 EVM connected to a BeagleBoneBlack

Fixes e37a7f8d77e1 ("leds: lm3532: Introduce the lm3532 LED driver")
Reported-by Pavel Machek <[email protected]>
Signed-off-by Dan Murphy <[email protected]>

---

v3 - Removed register define updates - https://lore.kernel.org/patchwork/patch/1114542/

 drivers/leds/leds-lm3532.c | 44 ++++++++++++++++++++++++++------------
 1 file changed, 30 insertions(+), 14 deletions(-)
drivers/leds/leds-lm3532.c +30 -14
index 646100724971..8132d05f7add 100644
@@ -38,6 +38,9 @@
 #define LM3532_REG_ZN_2_LO	0x65
 #define LM3532_REG_ZN_3_HI	0x66
 #define LM3532_REG_ZN_3_LO	0x67
+#define LM3532_REG_ZONE_TRGT_A	0x70
+#define LM3532_REG_ZONE_TRGT_B	0x75
+#define LM3532_REG_ZONE_TRGT_C	0x7a
 #define LM3532_REG_MAX		0x7e
 
 /* Contorl Enable */

@@ -116,6 +119,7 @@ struct lm3532_als_data {
  * @priv - Pointer the device data structure
  * @control_bank - Control bank the LED is associated to
  * @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
  * @led_strings - The LED strings supported in this array
  * @label - LED label

@@ -126,6 +130,7 @@ struct lm3532_led {
 
 	int control_bank;
 	int mode;
+	int ctrl_brt_pointer;
 	int num_leds;
 	u32 led_strings[LM3532_MAX_CONTROL_BANKS];
 	char label[LED_MAX_NAME_SIZE];

@@ -339,8 +344,8 @@ 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);
 

@@ -356,8 +361,30 @@ 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;
+	unsigned int brightness_config_reg;
+	unsigned 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
+	 * brightness register may have changed during boot.
+	 */
+	ret = regmap_read(drvdata->regmap, brightness_config_reg,
+			  &led->ctrl_brt_pointer);
+	if (ret)
+		return ret;
+
+	led->ctrl_brt_pointer &= LM3532_ZONE_MASK;
+	brightness_config_val = led->ctrl_brt_pointer | led->mode;
+	ret = regmap_write(drvdata->regmap, brightness_config_reg,
+			   brightness_config_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);

@@ -382,7 +409,6 @@ static int lm3532_als_configure(struct lm3532_data *priv,
 	struct lm3532_als_data *als = priv->als_data;
 	u32 als_vmin, als_vmax, als_vstep;
 	int zone_reg = LM3532_REG_ZN_0_HI;
-	int brightnes_config_reg;
 	int ret;
 	int i;
 

@@ -411,14 +437,7 @@ static int lm3532_als_configure(struct lm3532_data *priv,
 	als->config = (als->als_avrg_time | (LM3532_ENABLE_ALS) |
 		(als->als_input_mode << LM3532_ALS_SEL_SHIFT));
 
-	ret = regmap_write(priv->regmap, LM3532_ALS_CONFIG, als->config);
-	if (ret)
-		return ret;
-
-	brightnes_config_reg = LM3532_REG_ZONE_CFG_A + led->control_bank * 2;
-
-	return regmap_update_bits(priv->regmap, brightnes_config_reg,
-				  LM3532_I2C_CTRL, LM3532_ALS_CTRL);
+	return regmap_write(priv->regmap, LM3532_ALS_CONFIG, als->config);
 }
 
 static int lm3532_parse_als(struct lm3532_data *priv)

@@ -634,9 +653,6 @@ static int lm3532_probe(struct i2c_client *client,
 		return ret;
 	}
 
-	if (drvdata->enable_gpio)
-		gpiod_direction_output(drvdata->enable_gpio, 1);
-
 	return ret;
 }
Signature · 1 line
2.22.0.214.g8dca754b1e
author
#1 lore raw

PATCH v3 2/5 leds: lm3532: Change the define for the fs current register

Change the define name of the full scale current registers.

Signed-off-by Dan Murphy <[email protected]>

---

v3 - New patch - https://lore.kernel.org/patchwork/patch/1114542/

 drivers/leds/leds-lm3532.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
drivers/leds/leds-lm3532.c +6 -6
index 8132d05f7add..0c3d67671ab2 100644
@@ -23,11 +23,11 @@
 #define LM3532_REG_PWM_B_CFG	0x14
 #define LM3532_REG_PWM_C_CFG	0x15
 #define LM3532_REG_ZONE_CFG_A	0x16
-#define LM3532_REG_CTRL_A_BRT	0x17
+#define LM3532_REG_CTRL_A_FS_CURR	0x17
 #define LM3532_REG_ZONE_CFG_B	0x18
-#define LM3532_REG_CTRL_B_BRT	0x19
+#define LM3532_REG_CTRL_B_FS_CURR	0x19
 #define LM3532_REG_ZONE_CFG_C	0x1a
-#define LM3532_REG_CTRL_C_BRT	0x1b
+#define LM3532_REG_CTRL_C_FS_CURR	0x1b
 #define LM3532_REG_ENABLE	0x1d
 #define LM3532_ALS_CONFIG	0x23
 #define LM3532_REG_ZN_0_HI	0x60

@@ -173,11 +173,11 @@ static const struct reg_default lm3532_reg_defs[] = {
 	{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, 0xf3},
 	{LM3532_REG_ZONE_CFG_B, 0xf1},
-	{LM3532_REG_CTRL_B_BRT, 0xf3},
+	{LM3532_REG_CTRL_B_FS_CURR, 0xf3},
 	{LM3532_REG_ZONE_CFG_C, 0xf1},
-	{LM3532_REG_CTRL_C_BRT, 0xf3},
+	{LM3532_REG_CTRL_C_FS_CURR, 0xf3},
 	{LM3532_REG_ENABLE, 0xf8},
 	{LM3532_ALS_CONFIG, 0x44},
 	{LM3532_REG_ZN_0_HI, 0x35},
Signature · 1 line
2.22.0.214.g8dca754b1e
Acked-by on v3 2/5
#2 lore raw

Re: [PATCH v3 2/5] leds: lm3532: Change the define for the fs current register

Quoted 3 lines · Dan Murphy

Change the define name of the full scale current registers.

Signed-off-by Dan Murphy <[email protected]>
Acked-by Pavel Machek <[email protected]>
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
#4 lore raw

Re: [PATCH v3 2/5] leds: lm3532: Change the define for the fs current register

Quoted 10 lines · Jacek Anaszewski
Quoted 7 lines · Pavel Machek
Quoted 3 lines · Dan Murphy

Change the define name of the full scale current registers.

Signed-off-by Dan Murphy <[email protected]>
Acked-by Pavel Machek <[email protected]>

Thanks, applied.

My above reply is a mistake, please ignore.

Patch was applied yesterday anyway.

Signature · 2 lines
Best regards,
Jacek Anaszewski
author
#5 lore raw

PATCH v3 3/5 leds: lm3532: Fixes for the driver for stability

In reply to Dan Murphy

Fixed misspelled words, added error check during probe on the init of the registers, and fixed ALS/I2C control mode.

Fixes e37a7f8d77e1 ("leds: lm3532: Introduce the lm3532 LED driver")
Reported-by Pavel Machek <[email protected]>
Acked-by Pavel Machek <[email protected]>
Signed-off-by Dan Murphy <[email protected]>

---

v3 - No changes - https://lore.kernel.org/patchwork/patch/1114541/

 drivers/leds/leds-lm3532.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)
drivers/leds/leds-lm3532.c +13 -4
index 0c3d67671ab2..5e33ae4561f2 100644
@@ -43,7 +43,7 @@
 #define LM3532_REG_ZONE_TRGT_C	0x7a
 #define LM3532_REG_MAX		0x7e
 
-/* Contorl Enable */
+/* Control Enable */
 #define LM3532_CTRL_A_ENABLE	BIT(0)
 #define LM3532_CTRL_B_ENABLE	BIT(1)
 #define LM3532_CTRL_C_ENABLE	BIT(2)

@@ -307,7 +307,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;

@@ -326,7 +326,7 @@ static int lm3532_brightness_set(struct led_classdev *led_cdev,
 
 	mutex_lock(&led->priv->lock);
 
-	if (led->mode == LM3532_BL_MODE_ALS) {
+	if (led->mode == LM3532_ALS_CTRL) {
 		if (brt_val > LED_OFF)
 			ret = lm3532_led_enable(led);
 		else

@@ -561,11 +561,14 @@ static int lm3532_parse_node(struct lm3532_data *priv)
 		}
 
 		if (led->mode == LM3532_BL_MODE_ALS) {
+			led->mode = LM3532_ALS_CTRL;
 			ret = lm3532_parse_als(priv);
 			if (ret)
 				dev_err(&priv->client->dev, "Failed to parse als\n");
 			else
 				lm3532_als_configure(priv, led);
+		} else {
+			led->mode = LM3532_I2C_CTRL;
 		}
 
 		led->num_leds = fwnode_property_count_u32(child, "led-sources");

@@ -606,7 +609,13 @@ static int lm3532_parse_node(struct lm3532_data *priv)
 			goto child_out;
 		}
 
-		lm3532_init_registers(led);
+		ret = lm3532_init_registers(led);
+		if (ret) {
+			dev_err(&priv->client->dev, "register init err: %d\n",
+				ret);
+			fwnode_handle_put(child);
+			goto child_out;
+		}
 
 		i++;
 	}
Signature · 1 line
2.22.0.214.g8dca754b1e
author
#6 lore raw

PATCH v3 4/5 dt: lm3532: Add property for full scale current.

In reply to Dan Murphy

Add a property for each control bank to configure the full scale current setting for the device.

Acked-by Pavel Machek <[email protected]>
Signed-off-by Dan Murphy <[email protected]>

---

v3 - No changes - https://lore.kernel.org/patchwork/patch/1114540/

 Documentation/devicetree/bindings/leds/leds-lm3532.txt | 4 ++++
 1 file changed, 4 insertions(+)
Documentation/devicetree/bindings/leds/leds-lm3532.txt +4 -0
index c087f85ddddc..53793213dd52 100644
@@ -62,6 +62,9 @@ Optional LED child properties:
 	- label : see Documentation/devicetree/bindings/leds/common.txt
 	- linux,default-trigger :
 	   see Documentation/devicetree/bindings/leds/common.txt
+	- led-max-microamp : Defines the full scale current value for each control
+			  bank.  The range is from 5000uA-29800uA in increments
+			  of 800uA.
 
 Example:
 led-controller@38 {

@@ -85,6 +88,7 @@ led-controller@38 {
 		reg = <0>;
 		led-sources = <2>;
 		ti,led-mode = <1>;
+		led-max-microamp = <21800>;
 		label = ":backlight";
 		linux,default-trigger = "backlight";
 	};
Signature · 1 line
2.22.0.214.g8dca754b1e
author
#7 lore raw

PATCH v3 5/5 leds: lm3532: Add full scale current configuration

In reply to Dan Murphy

Allow the full scale current to be configured at init. Valid rangles are 5mA->29.8mA.

Signed-off-by Dan Murphy <[email protected]>

---

v3 - Fixed variable movement change and updated the fs current register calculation -
https://lore.kernel.org/patchwork/patch/1114544/

 drivers/leds/leds-lm3532.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)
drivers/leds/leds-lm3532.c +25 -0
index 5e33ae4561f2..ba352d76a8fb 100644
@@ -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
  */

@@ -132,6 +137,7 @@ struct lm3532_led {
 	int mode;
 	int ctrl_brt_pointer;
 	int num_leds;
+	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,17 @@ 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 +579,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);
Signature · 1 line
2.22.0.214.g8dca754b1e
#8 lore raw
In reply to Dan Murphy

Hi Dan,

Thank you for the update.

Quoted 117 lines · Dan Murphy

Fix the brightness control for I2C mode. Instead of changing the full scale current register update the ALS target register for the appropriate banks.

In addition clean up some code errors and random misspellings found during coding.

Tested on Droid4 as well as LM3532 EVM connected to a BeagleBoneBlack

Fixes e37a7f8d77e1 ("leds: lm3532: Introduce the lm3532 LED driver")
Reported-by Pavel Machek <[email protected]>
Signed-off-by Dan Murphy <[email protected]>

---

v3 - Removed register define updates - https://lore.kernel.org/patchwork/patch/1114542/

 drivers/leds/leds-lm3532.c | 44 ++++++++++++++++++++++++++------------
 1 file changed, 30 insertions(+), 14 deletions(-)
drivers/leds/leds-lm3532.c +30 -11
index 646100724971..8132d05f7add 100644
@@ -38,6 +38,9 @@
 #define LM3532_REG_ZN_2_LO	0x65
 #define LM3532_REG_ZN_3_HI	0x66
 #define LM3532_REG_ZN_3_LO	0x67
+#define LM3532_REG_ZONE_TRGT_A	0x70
+#define LM3532_REG_ZONE_TRGT_B	0x75
+#define LM3532_REG_ZONE_TRGT_C	0x7a
 #define LM3532_REG_MAX		0x7e
 
 /* Contorl Enable */

@@ -116,6 +119,7 @@ struct lm3532_als_data {
  * @priv - Pointer the device data structure
  * @control_bank - Control bank the LED is associated to
  * @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
  * @led_strings - The LED strings supported in this array
  * @label - LED label

@@ -126,6 +130,7 @@ struct lm3532_led {
 
 	int control_bank;
 	int mode;
+	int ctrl_brt_pointer;
 	int num_leds;
 	u32 led_strings[LM3532_MAX_CONTROL_BANKS];
 	char label[LED_MAX_NAME_SIZE];

@@ -339,8 +344,8 @@ 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);
 

@@ -356,8 +361,30 @@ 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;
+	unsigned int brightness_config_reg;
+	unsigned 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
+	 * brightness register may have changed during boot.
+	 */
+	ret = regmap_read(drvdata->regmap, brightness_config_reg,
+			  &led->ctrl_brt_pointer);
+	if (ret)
+		return ret;
+
+	led->ctrl_brt_pointer &= LM3532_ZONE_MASK;
+	brightness_config_val = led->ctrl_brt_pointer | led->mode;
+	ret = regmap_write(drvdata->regmap, brightness_config_reg,
+			   brightness_config_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);

@@ -382,7 +409,6 @@ static int lm3532_als_configure(struct lm3532_data *priv,
 	struct lm3532_als_data *als = priv->als_data;
 	u32 als_vmin, als_vmax, als_vstep;
 	int zone_reg = LM3532_REG_ZN_0_HI;
-	int brightnes_config_reg;
 	int ret;
 	int i;
 

@@ -411,14 +437,7 @@ static int lm3532_als_configure(struct lm3532_data *priv,
 	als->config = (als->als_avrg_time | (LM3532_ENABLE_ALS) |
 		(als->als_input_mode << LM3532_ALS_SEL_SHIFT));
 
-	ret = regmap_write(priv->regmap, LM3532_ALS_CONFIG, als->config);
-	if (ret)
-		return ret;
-
-	brightnes_config_reg = LM3532_REG_ZONE_CFG_A + led->control_bank * 2;
-
-	return regmap_update_bits(priv->regmap, brightnes_config_reg,
-				  LM3532_I2C_CTRL, LM3532_ALS_CTRL);
+	return regmap_write(priv->regmap, LM3532_ALS_CONFIG, als->config);
 }
 
 static int lm3532_parse_als(struct lm3532_data *priv)

drivers/leds/leds-lm3532.c static int lm3532_probe(struct i2c_client *client, · line 658

Quoted 9 lines
drivers/leds/leds-lm3532.c +0 -3
@@ -634,9 +653,6 @@ static int lm3532_probe(struct i2c_client *client,
 		return ret;
 	}
 
-	if (drvdata->enable_gpio)
-		gpiod_direction_output(drvdata->enable_gpio, 1);
-
 	return ret;
 }

Patch set applied.

Signature · 2 lines
Best regards,
Jacek Anaszewski
#9 lore raw
In reply to Dan Murphy

Hi,

Quoted 7 lines · Dan Murphy

Fix the brightness control for I2C mode. Instead of changing the full scale current register update the ALS target register for the appropriate banks.

In addition clean up some code errors and random misspellings found during coding.

Quoted 8 lines

Tested on Droid4 as well as LM3532 EVM connected to a BeagleBoneBlack

Fixes e37a7f8d77e1 ("leds: lm3532: Introduce the lm3532 LED driver")
Reported-by Pavel Machek <[email protected]>
Signed-off-by Dan Murphy <[email protected]>

---

v3 - Removed register define updates - https://lore.kernel.org/patchwork/patch/1114542/

Looks like starting with this patch in Linux next the LCD on droid4 is so dim it's unreadable even with brightness set to 255. Setting brightness to 0 does blank it completely though.

Did something maybe break with the various patch revisions or are we now missing some dts patch?

Regards,

Tony

#10 lore raw
Quoted 17 lines · Tony Lindgren

Hi,

Quoted 14 lines · Dan Murphy

Fix the brightness control for I2C mode. Instead of changing the full scale current register update the ALS target register for the appropriate banks.

In addition clean up some code errors and random misspellings found during coding.

Tested on Droid4 as well as LM3532 EVM connected to a BeagleBoneBlack

Fixes e37a7f8d77e1 ("leds: lm3532: Introduce the lm3532 LED driver")
Reported-by Pavel Machek <[email protected]>
Signed-off-by Dan Murphy <[email protected]>

---

Quoted 8 lines
Quoted 1 line

v3 - Removed register define updates - https://lore.kernel.org/patchwork/patch/1114542/

Looks like starting with this patch in Linux next the LCD on droid4 is so dim it's unreadable even with brightness set to 255. Setting brightness to 0 does blank it completely though.

Did something maybe break with the various patch revisions or are we now missing some dts patch?

Maybe missing dts patch. We should provide maximum current the LED can handle...

Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
#11 lore raw
Quoted 24 lines · Pavel Machek
Quoted 23 lines · Tony Lindgren

Hi,

Quoted 7 lines · Dan Murphy

Fix the brightness control for I2C mode. Instead of changing the full scale current register update the ALS target register for the appropriate banks.

In addition clean up some code errors and random misspellings found during coding.

Quoted 8 lines

Tested on Droid4 as well as LM3532 EVM connected to a BeagleBoneBlack

Fixes e37a7f8d77e1 ("leds: lm3532: Introduce the lm3532 LED driver")
Reported-by Pavel Machek <[email protected]>
Signed-off-by Dan Murphy <[email protected]>

---

v3 - Removed register define updates - https://lore.kernel.org/patchwork/patch/1114542/

Looks like starting with this patch in Linux next the LCD on droid4 is so dim it's unreadable even with brightness set to 255. Setting brightness to 0 does blank it completely though.

Quoted 5 lines
Quoted 2 lines

Did something maybe break with the various patch revisions or are we now missing some dts patch?

Maybe missing dts patch. We should provide maximum current the LED can handle...

Or i2c control is somehow broken and only als control now works?

Regards,

Tony

author
#12 lore raw

Tony

Quoted 24 lines · Tony Lindgren
Quoted 23 lines · Pavel Machek
Quoted 22 lines · Tony Lindgren

Hi,

Quoted 7 lines · Dan Murphy

Fix the brightness control for I2C mode. Instead of changing the full scale current register update the ALS target register for the appropriate banks.

In addition clean up some code errors and random misspellings found during coding.

Quoted 8 lines

Tested on Droid4 as well as LM3532 EVM connected to a BeagleBoneBlack

Fixes e37a7f8d77e1 ("leds: lm3532: Introduce the lm3532 LED driver")
Reported-by Pavel Machek <[email protected]>
Signed-off-by Dan Murphy <[email protected]>

---

v3 - Removed register define updates - https://lore.kernel.org/patchwork/patch/1114542/

Looks like starting with this patch in Linux next the LCD on droid4 is so dim it's unreadable even with brightness set to 255. Setting brightness to 0 does blank it completely though.

Quoted 5 lines
Quoted 4 lines
Quoted 2 lines

Did something maybe break with the various patch revisions or are we now missing some dts patch?

Maybe missing dts patch. We should provide maximum current the LED can handle...

Or i2c control is somehow broken and only als control now works?

Let me test the next branch.

I did not see this when I wrote the patches on the Droid 4.

But I did not recheck Droid 4.

Dan

Quoted 4 lines

Regards,

Tony

#13 lore raw
In reply to Tony Lindgren
Quoted 25 lines · Tony Lindgren
Quoted 24 lines · Pavel Machek
Quoted 23 lines · Tony Lindgren

Hi,

Quoted 7 lines · Dan Murphy

Fix the brightness control for I2C mode. Instead of changing the full scale current register update the ALS target register for the appropriate banks.

In addition clean up some code errors and random misspellings found during coding.

Quoted 8 lines

Tested on Droid4 as well as LM3532 EVM connected to a BeagleBoneBlack

Fixes e37a7f8d77e1 ("leds: lm3532: Introduce the lm3532 LED driver")
Reported-by Pavel Machek <[email protected]>
Signed-off-by Dan Murphy <[email protected]>

---

v3 - Removed register define updates - https://lore.kernel.org/patchwork/patch/1114542/

Looks like starting with this patch in Linux next the LCD on droid4 is so dim it's unreadable even with brightness set to 255. Setting brightness to 0 does blank it completely though.

Quoted 7 lines
Quoted 5 lines
Quoted 2 lines

Did something maybe break with the various patch revisions or are we now missing some dts patch?

Maybe missing dts patch. We should provide maximum current the LED can handle...

Or i2c control is somehow broken and only als control now works?

Well, max current led is obviously missing. Plus code does not check the return from reading led-max-microamp.

ret = fwnode_property_read_u32(child, "led-max-microamp",

                                               &led->full_scale_current);

Untested, but something like this is neccessary according to code review.

Signed-off-by Pavel Machek <[email protected]>

Pavel

arch/arm/boot/dts/omap4-droid4-xt894.dts +2 -0
index 4454449..b883b84 100644
@@ -395,6 +395,7 @@
 			ti,led-mode = <0>;
 			label = ":backlight";
 			linux,default-trigger = "backlight";
+			led-max-microamp = 29800;
 		};
 
 		led@1 {

@@ -402,6 +403,7 @@
 			led-sources = <1>;
 			ti,led-mode = <0>;
 			label = ":kbd_backlight";
+			led-max-microamp = 29800;
 		};
 	};
 };
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
author
#14 lore raw

Tony

Quoted 25 lines · Pavel Machek
Quoted 24 lines · Tony Lindgren
Quoted 23 lines · Pavel Machek
Quoted 22 lines · Tony Lindgren

Hi,

Quoted 7 lines · Dan Murphy

Fix the brightness control for I2C mode. Instead of changing the full scale current register update the ALS target register for the appropriate banks.

In addition clean up some code errors and random misspellings found during coding.

Quoted 8 lines

Tested on Droid4 as well as LM3532 EVM connected to a BeagleBoneBlack

Fixes e37a7f8d77e1 ("leds: lm3532: Introduce the lm3532 LED driver")
Reported-by Pavel Machek <[email protected]>
Signed-off-by Dan Murphy <[email protected]>

---

v3 - Removed register define updates - https://lore.kernel.org/patchwork/patch/1114542/

Looks like starting with this patch in Linux next the LCD on droid4 is so dim it's unreadable even with brightness set to 255. Setting brightness to 0 does blank it completely though.

Quoted 5 lines
Quoted 5 lines
Quoted 4 lines
Quoted 2 lines

Did something maybe break with the various patch revisions or are we now missing some dts patch?

Maybe missing dts patch. We should provide maximum current the LED can handle...

Or i2c control is somehow broken and only als control now works?

With only setting CONFIG_LEDS_LM3532=m to the next branch I get full brightness with 255.

I also see half brightness at 128 with the ramp down working.

I am not able to reproduce this issue on my device.

Quoted 2 lines

Well, max current led is obviously missing. Plus code does not check the return from reading led-max-microamp.

led-max-microamp is optional so there is no need to check the return.

full_scale_current should be 0 if not populated and in the init only if this variable is set does

the code program the register otherwise it is default of 20.2 mA.

Dan

Quoted 32 lines

ret = fwnode_property_read_u32(child, "led-max-microamp",

                                                &led->full_scale_current);

Untested, but something like this is neccessary according to code review.

Signed-off-by Pavel Machek <[email protected]>

Pavel

arch/arm/boot/dts/omap4-droid4-xt894.dts +2 -0
index 4454449..b883b84 100644
@@ -395,6 +395,7 @@
  			ti,led-mode = <0>;
  			label = ":backlight";
  			linux,default-trigger = "backlight";
+			led-max-microamp = 29800;
  		};
 
  		led@1 {

@@ -402,6 +403,7 @@
  			led-sources = <1>;
  			ti,led-mode = <0>;
  			label = ":kbd_backlight";
+			led-max-microamp = 29800;
  		};
  	};
  };
author
#15 lore raw

Hello

Quoted 32 lines · Dan Murphy

Tony

Quoted 29 lines · Pavel Machek
Quoted 28 lines · Tony Lindgren
Quoted 27 lines · Pavel Machek
Quoted 17 lines · Tony Lindgren

Hi,

Quoted 14 lines · Dan Murphy

Fix the brightness control for I2C mode.  Instead of changing the full scale current register update the ALS target register for the appropriate banks.

In addition clean up some code errors and random misspellings found during coding.

Tested on Droid4 as well as LM3532 EVM connected to a BeagleBoneBlack

Fixes e37a7f8d77e1 ("leds: lm3532: Introduce the lm3532 LED driver")
Reported-by Pavel Machek <[email protected]>
Signed-off-by Dan Murphy <[email protected]>

---

Quoted 7 lines
Quoted 1 line

v3 - Removed register define updates - https://lore.kernel.org/patchwork/patch/1114542/

Looks like starting with this patch in Linux next the LCD on droid4 is so dim it's unreadable even with brightness set to 255. Setting brightness to 0 does blank it completely though.

Did something maybe break with the various patch revisions or are we now missing some dts patch?

Maybe missing dts patch. We should provide maximum current the LED can handle...

Quoted 8 lines
Quoted 1 line
Quoted 1 line

Or i2c control is somehow broken and only als control now works?

With only setting CONFIG_LEDS_LM3532=m to the next branch I get full brightness with 255.

I also see half brightness at 128 with the ramp down working.

I am not able to reproduce this issue on my device.

Just to make sure my data was right I did a clean rebuild on commit 1dbb9fb4082ce2a2f1cf9596881ddece062d15d0

from the led-next branch.

Just adding the above config flag.  I still cannot reproduce the issue

See attached pic

Dan

<snip>

#16 lore raw
Quoted 47 lines · Dan Murphy

Hello

Quoted 35 lines · Dan Murphy

Tony

Quoted 32 lines · Pavel Machek
Quoted 31 lines · Tony Lindgren
Quoted 30 lines · Pavel Machek
Quoted 19 lines · Tony Lindgren

Hi,

Quoted 16 lines · Dan Murphy

Fix the brightness control for I2C mode.  Instead of changing the full scale current register update the ALS target register for the appropriate banks.

In addition clean up some code errors and random misspellings found during coding.

Tested on Droid4 as well as LM3532 EVM connected to a BeagleBoneBlack

Fixes e37a7f8d77e1 ("leds: lm3532: Introduce the

lm3532 LED driver")

Reported-by Pavel Machek <[email protected]>
Signed-off-by Dan Murphy <[email protected]>

---

Quoted 8 lines
Quoted 2 lines

v3 - Removed register define updates -
https://lore.kernel.org/patchwork/patch/1114542/

Looks like starting with this patch in Linux next the LCD on droid4 is so dim it's unreadable even with brightness set to 255. Setting brightness to 0 does blank it completely though.

Did something maybe break with the various patch revisions or are we now missing some dts patch?

Maybe missing dts patch. We should provide maximum current the LED can handle...

Quoted 9 lines
Quoted 1 line
Quoted 1 line

Or i2c control is somehow broken and only als control now works?

With only setting CONFIG_LEDS_LM3532=m to the next branch I get full brightness with 255.

I also see half brightness at 128 with the ramp down working.

I am not able to reproduce this issue on my device.

Quoted 8 lines

Just to make sure my data was right I did a clean rebuild on commit 1dbb9fb4082ce2a2f1cf9596881ddece062d15d0

from the led-next branch.

Just adding the above config flag.  I still cannot reproduce the issue

See attached pic

OK thanks for checking. Probably you can reproduce the issue if you reset things to commit c4b8354e5341 ("leds: lm3532: Fix brightness control for i2c mode"). There might now be something uninitialized with that commit depending on the hardware state on boot if you care to check that. Or maybe there's some interaction with other patches not yet at commit c4b8354e5341 level.

I confirmed again that things fail at commit c4b8354e5341. But now testing with the same Linux next as yesterday things works again. Not sure what's going on as failures with Linux next yestreday made me start narrowing down what commit causes the issues.

Anyways, playing with loading and unloading the leds-lm3532.ko I noticed we can also have unpaired regulator calls when using sysfs brightness that the below patch attempts to fix. Not sure how I got to the point of regulator warnings, but I was trying to enable the brightness via sysfs. Maybe I had other patches too when I got the regulator warnings.. But please check if the below patch makes sense.

Regards,

Tony

8< ---------------------------

drivers/leds/leds-lm3532.c +24 -2
@@ -127,6 +127,7 @@ struct lm3532_als_data {
  * @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
+ * @enabled - Enabled status
  * @label - LED label
  */
 struct lm3532_led {

@@ -138,6 +139,7 @@ struct lm3532_led {
 	int ctrl_brt_pointer;
 	int num_leds;
 	int full_scale_current;
+	int enabled:1;
 	u32 led_strings[LM3532_MAX_CONTROL_BANKS];
 	char label[LED_MAX_NAME_SIZE];
 };

@@ -292,11 +294,15 @@ static int lm3532_get_ramp_index(int ramp_time)
 				ramp_time);
 }
 
+/* Caller must take care of locking */
 static int lm3532_led_enable(struct lm3532_led *led_data)
 {
 	int ctrl_en_val = BIT(led_data->control_bank);
 	int ret;
 
+	if (led_data->enabled)
+		return 0;
+
 	ret = regmap_update_bits(led_data->priv->regmap, LM3532_REG_ENABLE,
 					 ctrl_en_val, ctrl_en_val);
 	if (ret) {

@@ -304,14 +310,24 @@ static int lm3532_led_enable(struct lm3532_led *led_data)
 		return ret;
 	}
 
-	return regulator_enable(led_data->priv->regulator);
+	ret = regulator_enable(led_data->priv->regulator);
+	if (ret < 0)
+		return ret;
+
+	led_data->enabled = 1;
+
+	return 0;
 }
 
+/* Caller must take care of locking */
 static int lm3532_led_disable(struct lm3532_led *led_data)
 {
 	int ctrl_en_val = BIT(led_data->control_bank);
 	int ret;
 
+	if (!led_data->enabled)
+		return 0;
+
 	ret = regmap_update_bits(led_data->priv->regmap, LM3532_REG_ENABLE,
 					 ctrl_en_val, 0);
 	if (ret) {

@@ -319,7 +335,13 @@ static int lm3532_led_disable(struct lm3532_led *led_data)
 		return ret;
 	}
 
-	return regulator_disable(led_data->priv->regulator);
+	ret = regulator_disable(led_data->priv->regulator);
+	if (ret < 0)
+		return ret;
+
+	led_data->enabled = 0;
+
+	return 0;
 }
 
 static int lm3532_brightness_set(struct led_classdev *led_cdev,
Signature · 1 line
2.23.0
author
#17 lore raw

Tony

Quoted 54 lines · Tony Lindgren
Quoted 53 lines · Dan Murphy

Hello

Quoted 35 lines · Dan Murphy

Tony

Quoted 32 lines · Pavel Machek
Quoted 31 lines · Tony Lindgren
Quoted 30 lines · Pavel Machek
Quoted 19 lines · Tony Lindgren

Hi,

Quoted 16 lines · Dan Murphy

Fix the brightness control for I2C mode.  Instead of changing the full scale current register update the ALS target register for the appropriate banks.

In addition clean up some code errors and random misspellings found during coding.

Tested on Droid4 as well as LM3532 EVM connected to a BeagleBoneBlack

Fixes e37a7f8d77e1 ("leds: lm3532: Introduce the

lm3532 LED driver")

Reported-by Pavel Machek <[email protected]>
Signed-off-by Dan Murphy <[email protected]>

---

Quoted 8 lines
Quoted 2 lines

v3 - Removed register define updates -
https://lore.kernel.org/patchwork/patch/1114542/

Looks like starting with this patch in Linux next the LCD on droid4 is so dim it's unreadable even with brightness set to 255. Setting brightness to 0 does blank it completely though.

Did something maybe break with the various patch revisions or are we now missing some dts patch?

Maybe missing dts patch. We should provide maximum current the LED can handle...

Quoted 8 lines
Quoted 1 line
Quoted 1 line

Or i2c control is somehow broken and only als control now works?

With only setting CONFIG_LEDS_LM3532=m to the next branch I get full brightness with 255.

I also see half brightness at 128 with the ramp down working.

I am not able to reproduce this issue on my device.

Just to make sure my data was right I did a clean rebuild on commit 1dbb9fb4082ce2a2f1cf9596881ddece062d15d0

from the led-next branch.

Just adding the above config flag.  I still cannot reproduce the issue

Quoted 7 lines
Quoted 1 line

See attached pic

OK thanks for checking. Probably you can reproduce the issue if you reset things to commit c4b8354e5341 ("leds: lm3532: Fix brightness control for i2c mode"). There might now be something uninitialized with that commit depending on the hardware state on boot if you care to check that. Or maybe there's some interaction with other patches not yet at commit c4b8354e5341 level.

Ok the fix for that issue is in this patch

leds: lm3532: Fixes for the driver for stability

The fix for setting the brightness control register during the init forced the ALS mode to

be enabled.  The Fixes for driver stability patch then set the led->mode to the correct register setting.

Quoted 5 lines

I confirmed again that things fail at commit c4b8354e5341. But now testing with the same Linux next as yesterday things works again. Not sure what's going on as failures with Linux next yestreday made me start narrowing down what commit causes the issues.

Quoted 7 lines

Anyways, playing with loading and unloading the leds-lm3532.ko I noticed we can also have unpaired regulator calls when using sysfs brightness that the below patch attempts to fix. Not sure how I got to the point of regulator warnings, but I was trying to enable the brightness via sysfs. Maybe I had other patches too when I got the regulator warnings.. But please check if the below patch makes sense.

Makes sense did you want me to push a patch?

Dan

Quoted 81 lines

Regards,

Tony

8< ---------------------------

drivers/leds/leds-lm3532.c +24 -2
@@ -127,6 +127,7 @@ struct lm3532_als_data {
   * @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
+ * @enabled - Enabled status
   * @label - LED label
   */
  struct lm3532_led {

@@ -138,6 +139,7 @@ struct lm3532_led {
  	int ctrl_brt_pointer;
  	int num_leds;
  	int full_scale_current;
+	int enabled:1;
  	u32 led_strings[LM3532_MAX_CONTROL_BANKS];
  	char label[LED_MAX_NAME_SIZE];
  };

@@ -292,11 +294,15 @@ static int lm3532_get_ramp_index(int ramp_time)
  				ramp_time);
  }
 
+/* Caller must take care of locking */
  static int lm3532_led_enable(struct lm3532_led *led_data)
  {
  	int ctrl_en_val = BIT(led_data->control_bank);
  	int ret;
 
+	if (led_data->enabled)
+		return 0;
+
  	ret = regmap_update_bits(led_data->priv->regmap, LM3532_REG_ENABLE,
  					 ctrl_en_val, ctrl_en_val);
  	if (ret) {

@@ -304,14 +310,24 @@ static int lm3532_led_enable(struct lm3532_led *led_data)
  		return ret;
  	}
 
-	return regulator_enable(led_data->priv->regulator);
+	ret = regulator_enable(led_data->priv->regulator);
+	if (ret < 0)
+		return ret;
+
+	led_data->enabled = 1;
+
+	return 0;
  }
 
+/* Caller must take care of locking */
  static int lm3532_led_disable(struct lm3532_led *led_data)
  {
  	int ctrl_en_val = BIT(led_data->control_bank);
  	int ret;
 
+	if (!led_data->enabled)
+		return 0;
+
  	ret = regmap_update_bits(led_data->priv->regmap, LM3532_REG_ENABLE,
  					 ctrl_en_val, 0);
  	if (ret) {

@@ -319,7 +335,13 @@ static int lm3532_led_disable(struct lm3532_led *led_data)
  		return ret;
  	}
 
-	return regulator_disable(led_data->priv->regulator);
+	ret = regulator_disable(led_data->priv->regulator);
+	if (ret < 0)
+		return ret;
+
+	led_data->enabled = 0;
+
+	return 0;
  }
 
  static int lm3532_brightness_set(struct led_classdev *led_cdev,
#18 lore raw
Quoted 67 lines · Dan Murphy

Tony

Quoted 54 lines · Tony Lindgren
Quoted 53 lines · Dan Murphy

Hello

Quoted 35 lines · Dan Murphy

Tony

Quoted 32 lines · Pavel Machek
Quoted 31 lines · Tony Lindgren
Quoted 30 lines · Pavel Machek
Quoted 19 lines · Tony Lindgren

Hi,

Quoted 16 lines · Dan Murphy

Fix the brightness control for I2C mode.  Instead of changing the full scale current register update the ALS target register for the appropriate banks.

In addition clean up some code errors and random misspellings found during coding.

Tested on Droid4 as well as LM3532 EVM connected to a BeagleBoneBlack

Fixes e37a7f8d77e1 ("leds: lm3532: Introduce the

lm3532 LED driver")

Reported-by Pavel Machek <[email protected]>
Signed-off-by Dan Murphy <[email protected]>

---

Quoted 8 lines
Quoted 2 lines

v3 - Removed register define updates -
https://lore.kernel.org/patchwork/patch/1114542/

Looks like starting with this patch in Linux next the LCD on droid4 is so dim it's unreadable even with brightness set to 255. Setting brightness to 0 does blank it completely though.

Did something maybe break with the various patch revisions or are we now missing some dts patch?

Maybe missing dts patch. We should provide maximum current the LED can handle...

Quoted 8 lines
Quoted 1 line
Quoted 1 line

Or i2c control is somehow broken and only als control now works?

With only setting CONFIG_LEDS_LM3532=m to the next branch I get full brightness with 255.

I also see half brightness at 128 with the ramp down working.

I am not able to reproduce this issue on my device.

Just to make sure my data was right I did a clean rebuild on commit 1dbb9fb4082ce2a2f1cf9596881ddece062d15d0

from the led-next branch.

Just adding the above config flag.  I still cannot reproduce the issue

Quoted 7 lines
Quoted 1 line

See attached pic

OK thanks for checking. Probably you can reproduce the issue if you reset things to commit c4b8354e5341 ("leds: lm3532: Fix brightness control for i2c mode"). There might now be something uninitialized with that commit depending on the hardware state on boot if you care to check that. Or maybe there's some interaction with other patches not yet at commit c4b8354e5341 level.

Ok the fix for that issue is in this patch

Quoted 7 lines

leds: lm3532: Fixes for the driver for stability

The fix for setting the brightness control register during the init forced the ALS mode to

be enabled.  The Fixes for driver stability patch then set the led->mode to the correct register setting.

OK yeah so that part starts to make sense now. I wonder if the reason for my issues was caused by change in sysfs brightness level values and I ended up chasing an already fixed issue. I think I was using value of 30 recently, which is now very dim :)

Quoted 13 lines
Quoted 5 lines

I confirmed again that things fail at commit c4b8354e5341. But now testing with the same Linux next as yesterday things works again. Not sure what's going on as failures with Linux next yestreday made me start narrowing down what commit causes the issues.

Quoted 7 lines

Anyways, playing with loading and unloading the leds-lm3532.ko I noticed we can also have unpaired regulator calls when using sysfs brightness that the below patch attempts to fix. Not sure how I got to the point of regulator warnings, but I was trying to enable the brightness via sysfs. Maybe I had other patches too when I got the regulator warnings.. But please check if the below patch makes sense.

Quoted 1 line

Makes sense did you want me to push a patch?

No need to, I can send out a proper patch later today after typing up the patch description if no other comments.

Regards,

Tony

#19 lore raw
In reply to Dan Murphy

Dan,

Quoted 38 lines · Dan Murphy

Tony

Quoted 26 lines · Pavel Machek
Quoted 25 lines · Tony Lindgren
Quoted 24 lines · Pavel Machek
Quoted 23 lines · Tony Lindgren

Hi,

Quoted 9 lines · Dan Murphy

Fix the brightness control for I2C mode.  Instead of changing the full scale current register update the ALS target register for the appropriate banks.

In addition clean up some code errors and random misspellings found during coding.

Tested on Droid4 as well as LM3532 EVM connected to a BeagleBoneBlack

Quoted 7 lines
Fixes e37a7f8d77e1 ("leds: lm3532: Introduce the lm3532 LED driver")
Reported-by Pavel Machek <[email protected]>
Signed-off-by Dan Murphy <[email protected]>

---

v3 - Removed register define updates -
https://lore.kernel.org/patchwork/patch/1114542/

Looks like starting with this patch in Linux next the LCD on droid4 is so dim it's unreadable even with brightness set to 255. Setting brightness to 0 does blank it completely though.

Quoted 5 lines
Quoted 5 lines
Quoted 4 lines
Quoted 2 lines

Did something maybe break with the various patch revisions or are we now missing some dts patch?

Maybe missing dts patch. We should provide maximum current the LED can handle...

Or i2c control is somehow broken and only als control now works?

With only setting CONFIG_LEDS_LM3532=m to the next branch I get full brightness with 255.

Quoted 8 lines

I also see half brightness at 128 with the ramp down working.

I am not able to reproduce this issue on my device.

Quoted 2 lines

Well, max current led is obviously missing. Plus code does not check the return from reading led-max-microamp.

led-max-microamp is optional so there is no need to check the return.

It's also ugly to not check it when you have it assigned. We'll soon receive complaints from static checkers about pointless assignment.

I'd distinguish between cases when parsing failed, and when property has not been provided.

if (fwnode_property_present(child, "led-max-microamp")) {
	if (fwnode_property_read_u32(child, "led-max-microamp",
				&led->full_scale_current);
		dev_err(&priv->client->dev,
                         "Failed to parse led-max-microamp property\n")
} else {
	dev_info(&priv->client->dev,
		 led-max-microamp property is missing\n")
}
Quoted 4 lines

full_scale_current should be 0 if not populated and in the init only if this variable is set does

the code program the register otherwise it is default of 20.2 mA.

Signature · 2 lines
Best regards,
Jacek Anaszewski
author
#20 lore raw

Jacek

Quoted 51 lines · Jacek Anaszewski

Dan,

Quoted 37 lines · Dan Murphy

Tony

Quoted 26 lines · Pavel Machek
Quoted 25 lines · Tony Lindgren
Quoted 24 lines · Pavel Machek
Quoted 23 lines · Tony Lindgren

Hi,

Quoted 9 lines · Dan Murphy

Fix the brightness control for I2C mode.  Instead of changing the full scale current register update the ALS target register for the appropriate banks.

In addition clean up some code errors and random misspellings found during coding.

Tested on Droid4 as well as LM3532 EVM connected to a BeagleBoneBlack

Quoted 7 lines
Fixes e37a7f8d77e1 ("leds: lm3532: Introduce the lm3532 LED driver")
Reported-by Pavel Machek <[email protected]>
Signed-off-by Dan Murphy <[email protected]>

---

v3 - Removed register define updates -
https://lore.kernel.org/patchwork/patch/1114542/

Looks like starting with this patch in Linux next the LCD on droid4 is so dim it's unreadable even with brightness set to 255. Setting brightness to 0 does blank it completely though.

Quoted 5 lines
Quoted 5 lines
Quoted 4 lines
Quoted 2 lines

Did something maybe break with the various patch revisions or are we now missing some dts patch?

Maybe missing dts patch. We should provide maximum current the LED can handle...

Or i2c control is somehow broken and only als control now works?

With only setting CONFIG_LEDS_LM3532=m to the next branch I get full brightness with 255.

Quoted 7 lines

I also see half brightness at 128 with the ramp down working.

I am not able to reproduce this issue on my device.

Quoted 2 lines

Well, max current led is obviously missing. Plus code does not check the return from reading led-max-microamp.

led-max-microamp is optional so there is no need to check the return.

It's also ugly to not check it when you have it assigned. We'll soon receive complaints from static checkers about pointless assignment.

Quoted 8 lines

I'd distinguish between cases when parsing failed, and when property has not been provided.

if (fwnode_property_present(child, "led-max-microamp")) {
	if (fwnode_property_read_u32(child, "led-max-microamp",
				&led->full_scale_current);
		dev_err(&priv->client->dev,
                          "Failed to parse led-max-microamp property\n")

I am OK with doing this but I think the else case logging is extra.

Again the property is optional and if the user decides not to populate it then there should not

be a log of that it is missing.

Dan

Quoted 9 lines
} else {
	dev_info(&priv->client->dev,
		 led-max-microamp property is missing\n")
}
Quoted 4 lines

full_scale_current should be 0 if not populated and in the init only if this variable is set does

the code program the register otherwise it is default of 20.2 mA.

#21 lore raw

Dan,

Quoted 1 line · Dan Murphy

Jacek

[...]

Quoted 24 lines
Quoted 15 lines
Quoted 11 lines
Quoted 1 line
Quoted 1 line

Or i2c control is somehow broken and only als control now works?

With only setting CONFIG_LEDS_LM3532=m to the next branch I get full brightness with 255.

I also see half brightness at 128 with the ramp down working.

I am not able to reproduce this issue on my device.

Quoted 2 lines

Well, max current led is obviously missing. Plus code does not check the return from reading led-max-microamp.

led-max-microamp is optional so there is no need to check the return.

It's also ugly to not check it when you have it assigned. We'll soon receive complaints from static checkers about pointless assignment.

Quoted 8 lines

I'd distinguish between cases when parsing failed, and when property has not been provided.

if (fwnode_property_present(child, "led-max-microamp")) {
    if (fwnode_property_read_u32(child, "led-max-microamp", &led->full_scale_current); dev_err(&priv->client->dev,
                          "Failed to parse led-max-microamp property\n")

Quoted 6 lines

I am OK with doing this but I think the else case logging is extra.

Again the property is optional and if the user decides not to populate it then there should not

be a log of that it is missing.

That's why I used lower log level (info). But it's up to you. I will not insist on keeping the logging for missing property case.

Quoted 12 lines

Dan

Quoted 9 lines

} else {
    dev_info(&priv->client->dev,
         led-max-microamp property is missing\n")
}

Quoted 4 lines

full_scale_current should be 0 if not populated and in the init only if this variable is set does

the code program the register otherwise it is default of 20.2 mA.

Signature · 2 lines
Best regards,
Jacek Anaszewski