[v3,1/5] leds: lm3532: Fix brightness control for i2c mode

Message ID 20190820195307.27590-1-dmurphy@ti.com
State New
Headers show
Series
  • [v3,1/5] leds: lm3532: Fix brightness control for i2c mode
Related show

Commit Message

Dan Murphy Aug. 20, 2019, 7:53 p.m.
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 <pavel@ucw.cz>
Signed-off-by: Dan Murphy <dmurphy@ti.com>

---

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

-- 
2.22.0.214.g8dca754b1e

Comments

Pavel Machek Aug. 25, 2019, 9:32 a.m. | #1
On Tue 2019-08-20 14:53:04, Dan Murphy wrote:
> Change the define name of the full scale current registers.

> 

> Signed-off-by: Dan Murphy <dmurphy@ti.com>


Acked-by: Pavel Machek <pavel@ucw.cz>


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Jacek Anaszewski Aug. 25, 2019, 10:50 a.m. | #2
Hi Dan,

Thank you for the update.

On 8/20/19 9:53 PM, Dan Murphy wrote:
> 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 <pavel@ucw.cz>

> Signed-off-by: Dan Murphy <dmurphy@ti.com>

> ---

> 

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

> 

> diff --git a/drivers/leds/leds-lm3532.c b/drivers/leds/leds-lm3532.c

> index 646100724971..8132d05f7add 100644

> --- a/drivers/leds/leds-lm3532.c

> +++ b/drivers/leds/leds-lm3532.c

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

>  }

>  

> 


Patch set applied.

-- 
Best regards,
Jacek Anaszewski
Tony Lindgren Aug. 26, 2019, 9:58 p.m. | #3
Hi,

* Dan Murphy <dmurphy@ti.com> [190820 19:53]:
> 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 <pavel@ucw.cz>

> Signed-off-by: Dan Murphy <dmurphy@ti.com>

> ---

> 

> 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
Pavel Machek Aug. 26, 2019, 10:14 p.m. | #4
On Mon 2019-08-26 14:58:22, Tony Lindgren wrote:
> Hi,

> 

> * Dan Murphy <dmurphy@ti.com> [190820 19:53]:

> > 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 <pavel@ucw.cz>

> > Signed-off-by: Dan Murphy <dmurphy@ti.com>

> > ---

> > 

> > 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... 

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Tony Lindgren Aug. 26, 2019, 10:44 p.m. | #5
* Pavel Machek <pavel@ucw.cz> [190826 22:14]:
> On Mon 2019-08-26 14:58:22, Tony Lindgren wrote:

> > Hi,

> > 

> > * Dan Murphy <dmurphy@ti.com> [190820 19:53]:

> > > 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 <pavel@ucw.cz>

> > > Signed-off-by: Dan Murphy <dmurphy@ti.com>

> > > ---

> > > 

> > > 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... 


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

Regards,

Tony
Pavel Machek Aug. 27, 2019, 12:18 p.m. | #6
On Mon 2019-08-26 15:44:37, Tony Lindgren wrote:
> * Pavel Machek <pavel@ucw.cz> [190826 22:14]:

> > On Mon 2019-08-26 14:58:22, Tony Lindgren wrote:

> > > Hi,

> > > 

> > > * Dan Murphy <dmurphy@ti.com> [190820 19:53]:

> > > > 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 <pavel@ucw.cz>

> > > > Signed-off-by: Dan Murphy <dmurphy@ti.com>

> > > > ---

> > > > 

> > > > 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... 

> 

> 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 <pavel@ucw.cz>

								Pavel

diff --git a/arch/arm/boot/dts/omap4-droid4-xt894.dts b/arch/arm/boot/dts/omap4-droid4-xt894.dts
index 4454449..b883b84 100644
--- a/arch/arm/boot/dts/omap4-droid4-xt894.dts
+++ b/arch/arm/boot/dts/omap4-droid4-xt894.dts
@@ -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;
 		};
 	};
 };


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Dan Murphy Aug. 27, 2019, 12:44 p.m. | #7
Tony

On 8/27/19 7:18 AM, Pavel Machek wrote:
> On Mon 2019-08-26 15:44:37, Tony Lindgren wrote:

>> * Pavel Machek <pavel@ucw.cz> [190826 22:14]:

>>> On Mon 2019-08-26 14:58:22, Tony Lindgren wrote:

>>>> Hi,

>>>>

>>>> * Dan Murphy <dmurphy@ti.com> [190820 19:53]:

>>>>> 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 <pavel@ucw.cz>

>>>>> Signed-off-by: Dan Murphy <dmurphy@ti.com>

>>>>> ---

>>>>>

>>>>> 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...

>> 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.

> 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


>

> 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 <pavel@ucw.cz>

> 								Pavel

>

> diff --git a/arch/arm/boot/dts/omap4-droid4-xt894.dts b/arch/arm/boot/dts/omap4-droid4-xt894.dts

> index 4454449..b883b84 100644

> --- a/arch/arm/boot/dts/omap4-droid4-xt894.dts

> +++ b/arch/arm/boot/dts/omap4-droid4-xt894.dts

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

>   		};

>   	};

>   };

>

>
Tony Lindgren Aug. 27, 2019, 4:04 p.m. | #8
* Dan Murphy <dmurphy@ti.com> [190827 13:02]:
> Hello

> 

> On 8/27/19 7:44 AM, Dan Murphy wrote:

> > Tony

> > 

> > On 8/27/19 7:18 AM, Pavel Machek wrote:

> > > On Mon 2019-08-26 15:44:37, Tony Lindgren wrote:

> > > > * Pavel Machek <pavel@ucw.cz> [190826 22:14]:

> > > > > On Mon 2019-08-26 14:58:22, Tony Lindgren wrote:

> > > > > > Hi,

> > > > > > 

> > > > > > * Dan Murphy <dmurphy@ti.com> [190820 19:53]:

> > > > > > > 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 <pavel@ucw.cz>

> > > > > > > Signed-off-by: Dan Murphy <dmurphy@ti.com>

> > > > > > > ---

> > > > > > > 

> > > > > > > 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...

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


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< ---------------------------
diff --git a/drivers/leds/leds-lm3532.c b/drivers/leds/leds-lm3532.c
--- a/drivers/leds/leds-lm3532.c
+++ b/drivers/leds/leds-lm3532.c
@@ -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,
-- 
2.23.0
Dan Murphy Aug. 27, 2019, 4:19 p.m. | #9
Tony

On 8/27/19 11:04 AM, Tony Lindgren wrote:
> * Dan Murphy <dmurphy@ti.com> [190827 13:02]:

>> Hello

>>

>> On 8/27/19 7:44 AM, Dan Murphy wrote:

>>> Tony

>>>

>>> On 8/27/19 7:18 AM, Pavel Machek wrote:

>>>> On Mon 2019-08-26 15:44:37, Tony Lindgren wrote:

>>>>> * Pavel Machek <pavel@ucw.cz> [190826 22:14]:

>>>>>> On Mon 2019-08-26 14:58:22, Tony Lindgren wrote:

>>>>>>> Hi,

>>>>>>>

>>>>>>> * Dan Murphy <dmurphy@ti.com> [190820 19:53]:

>>>>>>>> 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 <pavel@ucw.cz>

>>>>>>>> Signed-off-by: Dan Murphy <dmurphy@ti.com>

>>>>>>>> ---

>>>>>>>>

>>>>>>>> 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...

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

> 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.


> 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.


Makes sense did you want me to push a patch?

Dan


> Regards,

>

> Tony

>

> 8< ---------------------------

> diff --git a/drivers/leds/leds-lm3532.c b/drivers/leds/leds-lm3532.c

> --- a/drivers/leds/leds-lm3532.c

> +++ b/drivers/leds/leds-lm3532.c

> @@ -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,
Jacek Anaszewski Aug. 27, 2019, 9:14 p.m. | #10
Dan,

On 8/27/19 2:44 PM, Dan Murphy wrote:
> Tony

> 

> On 8/27/19 7:18 AM, Pavel Machek wrote:

>> On Mon 2019-08-26 15:44:37, Tony Lindgren wrote:

>>> * Pavel Machek <pavel@ucw.cz> [190826 22:14]:

>>>> On Mon 2019-08-26 14:58:22, Tony Lindgren wrote:

>>>>> Hi,

>>>>>

>>>>> * Dan Murphy <dmurphy@ti.com> [190820 19:53]:

>>>>>> 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 <pavel@ucw.cz>

>>>>>> Signed-off-by: Dan Murphy <dmurphy@ti.com>

>>>>>> ---

>>>>>>

>>>>>> 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...

>>> 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.

> 

>> 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")
}

> 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.


-- 
Best regards,
Jacek Anaszewski
Jacek Anaszewski Aug. 27, 2019, 9:33 p.m. | #11
On 8/25/19 11:32 AM, Pavel Machek wrote:
> On Tue 2019-08-20 14:53:04, Dan Murphy wrote:

>> Change the define name of the full scale current registers.

>>

>> Signed-off-by: Dan Murphy <dmurphy@ti.com>

> 

> Acked-by: Pavel Machek <pavel@ucw.cz>

> 


Thanks, applied.

-- 
Best regards,
Jacek Anaszewski
Jacek Anaszewski Aug. 27, 2019, 9:37 p.m. | #12
On 8/27/19 11:33 PM, Jacek Anaszewski wrote:
> On 8/25/19 11:32 AM, Pavel Machek wrote:

>> On Tue 2019-08-20 14:53:04, Dan Murphy wrote:

>>> Change the define name of the full scale current registers.

>>>

>>> Signed-off-by: Dan Murphy <dmurphy@ti.com>

>>

>> Acked-by: Pavel Machek <pavel@ucw.cz>

>>

> 

> Thanks, applied.


My above reply is a mistake, please ignore.

Patch was applied yesterday anyway.

-- 
Best regards,
Jacek Anaszewski
Dan Murphy Aug. 28, 2019, 3:28 p.m. | #13
Jacek

On 8/27/19 4:14 PM, Jacek Anaszewski wrote:
> Dan,

>

> On 8/27/19 2:44 PM, Dan Murphy wrote:

>> Tony

>>

>> On 8/27/19 7:18 AM, Pavel Machek wrote:

>>> On Mon 2019-08-26 15:44:37, Tony Lindgren wrote:

>>>> * Pavel Machek <pavel@ucw.cz> [190826 22:14]:

>>>>> On Mon 2019-08-26 14:58:22, Tony Lindgren wrote:

>>>>>> Hi,

>>>>>>

>>>>>> * Dan Murphy <dmurphy@ti.com> [190820 19:53]:

>>>>>>> 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 <pavel@ucw.cz>

>>>>>>> Signed-off-by: Dan Murphy <dmurphy@ti.com>

>>>>>>> ---

>>>>>>>

>>>>>>> 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...

>>>> 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.

>>

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


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

> } else {

> 	dev_info(&priv->client->dev,

> 		 led-max-microamp property is missing\n")

> }

>

>> 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.
Jacek Anaszewski Aug. 28, 2019, 8:37 p.m. | #14
Dan,

On 8/28/19 5:28 PM, Dan Murphy wrote:
> Jacek

[...]
>>>>> 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.

>>>

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

> 

> 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.

> Dan

> 

>> } else {

>>     dev_info(&priv->client->dev,

>>          led-max-microamp property is missing\n")

>> }

>>

>>> 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.

> 


-- 
Best regards,
Jacek Anaszewski

Patch

diff --git a/drivers/leds/leds-lm3532.c b/drivers/leds/leds-lm3532.c
index 646100724971..8132d05f7add 100644
--- a/drivers/leds/leds-lm3532.c
+++ b/drivers/leds/leds-lm3532.c
@@ -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;
 }