linux-leds.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/3] leds: lm3532: Fix brightness control for i2c mode
@ 2019-08-01 15:14 Dan Murphy
  2019-08-01 15:14 ` [PATCH 2/3] dt: lm3532: Add property for full scale current Dan Murphy
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Dan Murphy @ 2019-08-01 15:14 UTC (permalink / raw)
  To: jacek.anaszewski, pavel, tony, sre, nekit1000, mpartap, merlijn
  Cc: linux-leds, linux-kernel, 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 <pavel@ucw.cz>
Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
 drivers/leds/leds-lm3532.c | 78 ++++++++++++++++++++++++--------------
 1 file changed, 50 insertions(+), 28 deletions(-)

diff --git a/drivers/leds/leds-lm3532.c b/drivers/leds/leds-lm3532.c
index 646100724971..ff197adb2f22 100644
--- a/drivers/leds/leds-lm3532.c
+++ b/drivers/leds/leds-lm3532.c
@@ -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
@@ -38,9 +38,12 @@
 #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 */
+/* Control Enable */
 #define LM3532_CTRL_A_ENABLE	BIT(0)
 #define LM3532_CTRL_B_ENABLE	BIT(1)
 #define LM3532_CTRL_C_ENABLE	BIT(2)
@@ -63,6 +66,8 @@
 #define LM3532_ZONE_2		BIT(3)
 #define LM3532_ZONE_3		(BIT(2) | BIT(3))
 #define LM3532_ZONE_4		BIT(4)
+#define LM3532_BRT_CFG_MASK	(LM3532_I2C_CTRL | LM3532_LINEAR_MAP | \
+				LM3532_ZONE_MASK)
 
 #define LM3532_ENABLE_ALS	BIT(3)
 #define LM3532_ALS_SEL_SHIFT	6
@@ -127,6 +132,7 @@ struct lm3532_led {
 	int control_bank;
 	int mode;
 	int num_leds;
+	int ctrl_brt_pointer;
 	u32 led_strings[LM3532_MAX_CONTROL_BANKS];
 	char label[LED_MAX_NAME_SIZE];
 };
@@ -161,18 +167,18 @@ struct lm3532_data {
 };
 
 static const struct reg_default lm3532_reg_defs[] = {
-	{LM3532_REG_OUTPUT_CFG, 0xe4},
+	{LM3532_REG_OUTPUT_CFG, 0x24},
 	{LM3532_REG_STARTSHUT_RAMP, 0xc0},
 	{LM3532_REG_RT_RAMP, 0xc0},
 	{LM3532_REG_PWM_A_CFG, 0x82},
 	{LM3532_REG_PWM_B_CFG, 0x82},
 	{LM3532_REG_PWM_C_CFG, 0x82},
 	{LM3532_REG_ZONE_CFG_A, 0xf1},
-	{LM3532_REG_CTRL_A_BRT, 0xf3},
+	{LM3532_REG_CTRL_A_FS_CURR, 0x13},
 	{LM3532_REG_ZONE_CFG_B, 0xf1},
-	{LM3532_REG_CTRL_B_BRT, 0xf3},
+	{LM3532_REG_CTRL_B_FS_CURR, 0x13},
 	{LM3532_REG_ZONE_CFG_C, 0xf1},
-	{LM3532_REG_CTRL_C_BRT, 0xf3},
+	{LM3532_REG_CTRL_C_FS_CURR, 0x13},
 	{LM3532_REG_ENABLE, 0xf8},
 	{LM3532_ALS_CONFIG, 0x44},
 	{LM3532_REG_ZN_0_HI, 0x35},
@@ -302,7 +308,7 @@ static int lm3532_led_disable(struct lm3532_led *led_data)
 	int ret;
 
 	ret = regmap_update_bits(led_data->priv->regmap, LM3532_REG_ENABLE,
-					 ctrl_en_val, ~ctrl_en_val);
+					 ctrl_en_val, 0);
 	if (ret) {
 		dev_err(led_data->priv->dev, "Failed to set ctrl:%d\n", ret);
 		return ret;
@@ -321,7 +327,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
@@ -339,11 +345,9 @@ static int lm3532_brightness_set(struct led_classdev *led_cdev,
 	if (ret)
 		goto unlock;
 
-	brightness_reg = LM3532_REG_CTRL_A_BRT + led->control_bank * 2;
-	brt_val = brt_val / LM3532_BRT_VAL_ADJUST;
-
+	brightness_reg = LM3532_REG_ZONE_TRGT_A + led->control_bank * 5 +
+			 (led->ctrl_brt_pointer >> 2);
 	ret = regmap_write(led->priv->regmap, brightness_reg, brt_val);
-
 unlock:
 	mutex_unlock(&led->priv->lock);
 	return ret;
@@ -356,8 +360,29 @@ static int lm3532_init_registers(struct lm3532_led *led)
 	unsigned int output_cfg_val = 0;
 	unsigned int output_cfg_shift = 0;
 	unsigned int output_cfg_mask = 0;
+	int brightness_config_reg;
+	int brightness_config_val;
 	int ret, i;
 
+	if (drvdata->enable_gpio)
+		gpiod_direction_output(drvdata->enable_gpio, 1);
+
+	brightness_config_reg = LM3532_REG_ZONE_CFG_A + led->control_bank * 2;
+	/* This could be hard coded to the default value but the control
+	 * 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 +407,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 +435,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)
@@ -542,12 +559,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");
 		if (led->num_leds > LM3532_MAX_LED_STRINGS) {
@@ -587,7 +606,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++;
 	}
@@ -634,9 +659,6 @@ static int lm3532_probe(struct i2c_client *client,
 		return ret;
 	}
 
-	if (drvdata->enable_gpio)
-		gpiod_direction_output(drvdata->enable_gpio, 1);
-
 	return ret;
 }
 
-- 
2.22.0.214.g8dca754b1e


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 2/3] dt: lm3532: Add property for full scale current.
  2019-08-01 15:14 [PATCH 1/3] leds: lm3532: Fix brightness control for i2c mode Dan Murphy
@ 2019-08-01 15:14 ` Dan Murphy
  2019-08-01 18:53   ` Jacek Anaszewski
  2019-08-01 15:14 ` [PATCH 3/3] leds: lm3532: Add full scale current configuration Dan Murphy
  2019-08-01 18:52 ` [PATCH 1/3] leds: lm3532: Fix brightness control for i2c mode Jacek Anaszewski
  2 siblings, 1 reply; 10+ messages in thread
From: Dan Murphy @ 2019-08-01 15:14 UTC (permalink / raw)
  To: jacek.anaszewski, pavel, tony, sre, nekit1000, mpartap, merlijn
  Cc: linux-leds, linux-kernel, Dan Murphy

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

Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
 Documentation/devicetree/bindings/leds/leds-lm3532.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/leds/leds-lm3532.txt b/Documentation/devicetree/bindings/leds/leds-lm3532.txt
index c087f85ddddc..d3498dd24e8e 100644
--- a/Documentation/devicetree/bindings/leds/leds-lm3532.txt
+++ b/Documentation/devicetree/bindings/leds/leds-lm3532.txt
@@ -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
+	- ti,fs-current : 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>;
+		ti,fs-current = <21800>;
 		label = ":backlight";
 		linux,default-trigger = "backlight";
 	};
-- 
2.22.0.214.g8dca754b1e


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 3/3] leds: lm3532: Add full scale current configuration
  2019-08-01 15:14 [PATCH 1/3] leds: lm3532: Fix brightness control for i2c mode Dan Murphy
  2019-08-01 15:14 ` [PATCH 2/3] dt: lm3532: Add property for full scale current Dan Murphy
@ 2019-08-01 15:14 ` Dan Murphy
  2019-08-01 18:52 ` [PATCH 1/3] leds: lm3532: Fix brightness control for i2c mode Jacek Anaszewski
  2 siblings, 0 replies; 10+ messages in thread
From: Dan Murphy @ 2019-08-01 15:14 UTC (permalink / raw)
  To: jacek.anaszewski, pavel, tony, sre, nekit1000, mpartap, merlijn
  Cc: linux-leds, linux-kernel, Dan Murphy

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

Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
 drivers/leds/leds-lm3532.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/leds/leds-lm3532.c b/drivers/leds/leds-lm3532.c
index ff197adb2f22..114fea12ec21 100644
--- a/drivers/leds/leds-lm3532.c
+++ b/drivers/leds/leds-lm3532.c
@@ -91,6 +91,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
@@ -133,6 +137,7 @@ struct lm3532_led {
 	int mode;
 	int num_leds;
 	int ctrl_brt_pointer;
+	int full_scale_current;
 	u32 led_strings[LM3532_MAX_CONTROL_BANKS];
 	char label[LED_MAX_NAME_SIZE];
 };
@@ -362,6 +367,8 @@ static int lm3532_init_registers(struct lm3532_led *led)
 	unsigned int output_cfg_mask = 0;
 	int brightness_config_reg;
 	int brightness_config_val;
+	int fs_current_reg;
+	int fs_current_val;
 	int ret, i;
 
 	if (drvdata->enable_gpio)
@@ -383,6 +390,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);
@@ -558,6 +575,12 @@ static int lm3532_parse_node(struct lm3532_data *priv)
 			goto child_out;
 		}
 
+		ret = fwnode_property_read_u32(child, "ti,fs-current",
+					       &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);
-- 
2.22.0.214.g8dca754b1e


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/3] leds: lm3532: Fix brightness control for i2c mode
  2019-08-01 15:14 [PATCH 1/3] leds: lm3532: Fix brightness control for i2c mode Dan Murphy
  2019-08-01 15:14 ` [PATCH 2/3] dt: lm3532: Add property for full scale current Dan Murphy
  2019-08-01 15:14 ` [PATCH 3/3] leds: lm3532: Add full scale current configuration Dan Murphy
@ 2019-08-01 18:52 ` Jacek Anaszewski
  2019-08-01 19:17   ` Dan Murphy
  2019-08-01 21:36   ` Pavel Machek
  2 siblings, 2 replies; 10+ messages in thread
From: Jacek Anaszewski @ 2019-08-01 18:52 UTC (permalink / raw)
  To: Dan Murphy, pavel, tony, sre, nekit1000, mpartap, merlijn
  Cc: linux-leds, linux-kernel

Hi Dan,

Thank you for the patch set. Please find my comments
in the code below.

On 8/1/19 5:14 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>
> ---
>  drivers/leds/leds-lm3532.c | 78 ++++++++++++++++++++++++--------------
>  1 file changed, 50 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/leds/leds-lm3532.c b/drivers/leds/leds-lm3532.c
> index 646100724971..ff197adb2f22 100644
> --- a/drivers/leds/leds-lm3532.c
> +++ b/drivers/leds/leds-lm3532.c
> @@ -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
> @@ -38,9 +38,12 @@
>  #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 */
> +/* Control Enable */
>  #define LM3532_CTRL_A_ENABLE	BIT(0)
>  #define LM3532_CTRL_B_ENABLE	BIT(1)
>  #define LM3532_CTRL_C_ENABLE	BIT(2)
> @@ -63,6 +66,8 @@
>  #define LM3532_ZONE_2		BIT(3)
>  #define LM3532_ZONE_3		(BIT(2) | BIT(3))
>  #define LM3532_ZONE_4		BIT(4)
> +#define LM3532_BRT_CFG_MASK	(LM3532_I2C_CTRL | LM3532_LINEAR_MAP | \
> +				LM3532_ZONE_MASK)
>  
>  #define LM3532_ENABLE_ALS	BIT(3)
>  #define LM3532_ALS_SEL_SHIFT	6
> @@ -127,6 +132,7 @@ struct lm3532_led {
>  	int control_bank;
>  	int mode;
>  	int num_leds;
> +	int ctrl_brt_pointer;
>  	u32 led_strings[LM3532_MAX_CONTROL_BANKS];
>  	char label[LED_MAX_NAME_SIZE];
>  };
> @@ -161,18 +167,18 @@ struct lm3532_data {
>  };
>  
>  static const struct reg_default lm3532_reg_defs[] = {
> -	{LM3532_REG_OUTPUT_CFG, 0xe4},
> +	{LM3532_REG_OUTPUT_CFG, 0x24},
>  	{LM3532_REG_STARTSHUT_RAMP, 0xc0},
>  	{LM3532_REG_RT_RAMP, 0xc0},
>  	{LM3532_REG_PWM_A_CFG, 0x82},
>  	{LM3532_REG_PWM_B_CFG, 0x82},
>  	{LM3532_REG_PWM_C_CFG, 0x82},
>  	{LM3532_REG_ZONE_CFG_A, 0xf1},
> -	{LM3532_REG_CTRL_A_BRT, 0xf3},
> +	{LM3532_REG_CTRL_A_FS_CURR, 0x13},
>  	{LM3532_REG_ZONE_CFG_B, 0xf1},
> -	{LM3532_REG_CTRL_B_BRT, 0xf3},
> +	{LM3532_REG_CTRL_B_FS_CURR, 0x13},
>  	{LM3532_REG_ZONE_CFG_C, 0xf1},
> -	{LM3532_REG_CTRL_C_BRT, 0xf3},
> +	{LM3532_REG_CTRL_C_FS_CURR, 0x13},
>  	{LM3532_REG_ENABLE, 0xf8},
>  	{LM3532_ALS_CONFIG, 0x44},
>  	{LM3532_REG_ZN_0_HI, 0x35},
> @@ -302,7 +308,7 @@ static int lm3532_led_disable(struct lm3532_led *led_data)
>  	int ret;
>  
>  	ret = regmap_update_bits(led_data->priv->regmap, LM3532_REG_ENABLE,
> -					 ctrl_en_val, ~ctrl_en_val);
> +					 ctrl_en_val, 0);
>  	if (ret) {
>  		dev_err(led_data->priv->dev, "Failed to set ctrl:%d\n", ret);
>  		return ret;
> @@ -321,7 +327,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
> @@ -339,11 +345,9 @@ static int lm3532_brightness_set(struct led_classdev *led_cdev,
>  	if (ret)
>  		goto unlock;
>  
> -	brightness_reg = LM3532_REG_CTRL_A_BRT + led->control_bank * 2;
> -	brt_val = brt_val / LM3532_BRT_VAL_ADJUST;
> -
> +	brightness_reg = LM3532_REG_ZONE_TRGT_A + led->control_bank * 5 +
> +			 (led->ctrl_brt_pointer >> 2);
>  	ret = regmap_write(led->priv->regmap, brightness_reg, brt_val);
> -
>  unlock:
>  	mutex_unlock(&led->priv->lock);
>  	return ret;
> @@ -356,8 +360,29 @@ static int lm3532_init_registers(struct lm3532_led *led)
>  	unsigned int output_cfg_val = 0;
>  	unsigned int output_cfg_shift = 0;
>  	unsigned int output_cfg_mask = 0;
> +	int brightness_config_reg;
> +	int brightness_config_val;
>  	int ret, i;
>  
> +	if (drvdata->enable_gpio)
> +		gpiod_direction_output(drvdata->enable_gpio, 1);
> +
> +	brightness_config_reg = LM3532_REG_ZONE_CFG_A + led->control_bank * 2;
> +	/* This could be hard coded to the default value but the control

Coding style issue:

        /*
         * This is the preferred style for multi-line comments


> +	 * 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;

Could you shed more light on this hardware peculiarity?
What kind of pointer is it?

> +	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 +407,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 +435,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)
> @@ -542,12 +559,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;

Coding style issue: you need curly braces around the body of "else"
in this case as well.

>  
>  		led->num_leds = fwnode_property_count_u32(child, "led-sources");
>  		if (led->num_leds > LM3532_MAX_LED_STRINGS) {
> @@ -587,7 +606,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++;
>  	}
> @@ -634,9 +659,6 @@ static int lm3532_probe(struct i2c_client *client,
>  		return ret;
>  	}
>  
> -	if (drvdata->enable_gpio)
> -		gpiod_direction_output(drvdata->enable_gpio, 1);
> -
>  	return ret;
>  }
>  
> 

-- 
Best regards,
Jacek Anaszewski

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/3] dt: lm3532: Add property for full scale current.
  2019-08-01 15:14 ` [PATCH 2/3] dt: lm3532: Add property for full scale current Dan Murphy
@ 2019-08-01 18:53   ` Jacek Anaszewski
  2019-08-01 19:12     ` Dan Murphy
  0 siblings, 1 reply; 10+ messages in thread
From: Jacek Anaszewski @ 2019-08-01 18:53 UTC (permalink / raw)
  To: Dan Murphy, pavel, tony, sre, nekit1000, mpartap, merlijn
  Cc: linux-leds, linux-kernel

Dan,

On 8/1/19 5:14 PM, Dan Murphy wrote:
> Add a property for each control bank to configure the
> full scale current setting for the device.
> 
> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> ---
>  Documentation/devicetree/bindings/leds/leds-lm3532.txt | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/leds/leds-lm3532.txt b/Documentation/devicetree/bindings/leds/leds-lm3532.txt
> index c087f85ddddc..d3498dd24e8e 100644
> --- a/Documentation/devicetree/bindings/leds/leds-lm3532.txt
> +++ b/Documentation/devicetree/bindings/leds/leds-lm3532.txt
> @@ -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
> +	- ti,fs-current : Defines the full scale current value for each control
> +			  bank.  The range is from 5000uA-29800uA in increments
> +			  of 800uA.

Wouldn't led-max-microamp fit here?

>  
>  Example:
>  led-controller@38 {
> @@ -85,6 +88,7 @@ led-controller@38 {
>  		reg = <0>;
>  		led-sources = <2>;
>  		ti,led-mode = <1>;
> +		ti,fs-current = <21800>;
>  		label = ":backlight";
>  		linux,default-trigger = "backlight";
>  	};
> 

-- 
Best regards,
Jacek Anaszewski

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/3] dt: lm3532: Add property for full scale current.
  2019-08-01 18:53   ` Jacek Anaszewski
@ 2019-08-01 19:12     ` Dan Murphy
  0 siblings, 0 replies; 10+ messages in thread
From: Dan Murphy @ 2019-08-01 19:12 UTC (permalink / raw)
  To: Jacek Anaszewski, pavel, tony, sre, nekit1000, mpartap, merlijn
  Cc: linux-leds, linux-kernel

Jacek

On 8/1/19 1:53 PM, Jacek Anaszewski wrote:
> Dan,
>
> On 8/1/19 5:14 PM, Dan Murphy wrote:
>> Add a property for each control bank to configure the
>> full scale current setting for the device.
>>
>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>> ---
>>   Documentation/devicetree/bindings/leds/leds-lm3532.txt | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/leds/leds-lm3532.txt b/Documentation/devicetree/bindings/leds/leds-lm3532.txt
>> index c087f85ddddc..d3498dd24e8e 100644
>> --- a/Documentation/devicetree/bindings/leds/leds-lm3532.txt
>> +++ b/Documentation/devicetree/bindings/leds/leds-lm3532.txt
>> @@ -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
>> +	- ti,fs-current : Defines the full scale current value for each control
>> +			  bank.  The range is from 5000uA-29800uA in increments
>> +			  of 800uA.
> Wouldn't led-max-microamp fit here?

Ah thats what I was looking for.  I would rather use existing properties.

I will change that.

Dan



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/3] leds: lm3532: Fix brightness control for i2c mode
  2019-08-01 18:52 ` [PATCH 1/3] leds: lm3532: Fix brightness control for i2c mode Jacek Anaszewski
@ 2019-08-01 19:17   ` Dan Murphy
  2019-08-01 19:48     ` Jacek Anaszewski
  2019-08-01 21:36   ` Pavel Machek
  1 sibling, 1 reply; 10+ messages in thread
From: Dan Murphy @ 2019-08-01 19:17 UTC (permalink / raw)
  To: Jacek Anaszewski, pavel, tony, sre, nekit1000, mpartap, merlijn
  Cc: linux-leds, linux-kernel

Jacek

On 8/1/19 1:52 PM, Jacek Anaszewski wrote:
> Hi Dan,
>
> Thank you for the patch set. Please find my comments
> in the code below.
>
> On 8/1/19 5:14 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>
>> ---
>>   drivers/leds/leds-lm3532.c | 78 ++++++++++++++++++++++++--------------
>>   1 file changed, 50 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/leds/leds-lm3532.c b/drivers/leds/leds-lm3532.c
>> index 646100724971..ff197adb2f22 100644
>> --- a/drivers/leds/leds-lm3532.c
>> +++ b/drivers/leds/leds-lm3532.c
>> @@ -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
>> @@ -38,9 +38,12 @@
>>   #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 */
>> +/* Control Enable */
>>   #define LM3532_CTRL_A_ENABLE	BIT(0)
>>   #define LM3532_CTRL_B_ENABLE	BIT(1)
>>   #define LM3532_CTRL_C_ENABLE	BIT(2)
>> @@ -63,6 +66,8 @@
>>   #define LM3532_ZONE_2		BIT(3)
>>   #define LM3532_ZONE_3		(BIT(2) | BIT(3))
>>   #define LM3532_ZONE_4		BIT(4)
>> +#define LM3532_BRT_CFG_MASK	(LM3532_I2C_CTRL | LM3532_LINEAR_MAP | \
>> +				LM3532_ZONE_MASK)
>>   
>>   #define LM3532_ENABLE_ALS	BIT(3)
>>   #define LM3532_ALS_SEL_SHIFT	6
>> @@ -127,6 +132,7 @@ struct lm3532_led {
>>   	int control_bank;
>>   	int mode;
>>   	int num_leds;
>> +	int ctrl_brt_pointer;
>>   	u32 led_strings[LM3532_MAX_CONTROL_BANKS];
>>   	char label[LED_MAX_NAME_SIZE];
>>   };
>> @@ -161,18 +167,18 @@ struct lm3532_data {
>>   };
>>   
>>   static const struct reg_default lm3532_reg_defs[] = {
>> -	{LM3532_REG_OUTPUT_CFG, 0xe4},
>> +	{LM3532_REG_OUTPUT_CFG, 0x24},
>>   	{LM3532_REG_STARTSHUT_RAMP, 0xc0},
>>   	{LM3532_REG_RT_RAMP, 0xc0},
>>   	{LM3532_REG_PWM_A_CFG, 0x82},
>>   	{LM3532_REG_PWM_B_CFG, 0x82},
>>   	{LM3532_REG_PWM_C_CFG, 0x82},
>>   	{LM3532_REG_ZONE_CFG_A, 0xf1},
>> -	{LM3532_REG_CTRL_A_BRT, 0xf3},
>> +	{LM3532_REG_CTRL_A_FS_CURR, 0x13},
>>   	{LM3532_REG_ZONE_CFG_B, 0xf1},
>> -	{LM3532_REG_CTRL_B_BRT, 0xf3},
>> +	{LM3532_REG_CTRL_B_FS_CURR, 0x13},
>>   	{LM3532_REG_ZONE_CFG_C, 0xf1},
>> -	{LM3532_REG_CTRL_C_BRT, 0xf3},
>> +	{LM3532_REG_CTRL_C_FS_CURR, 0x13},
>>   	{LM3532_REG_ENABLE, 0xf8},
>>   	{LM3532_ALS_CONFIG, 0x44},
>>   	{LM3532_REG_ZN_0_HI, 0x35},
>> @@ -302,7 +308,7 @@ static int lm3532_led_disable(struct lm3532_led *led_data)
>>   	int ret;
>>   
>>   	ret = regmap_update_bits(led_data->priv->regmap, LM3532_REG_ENABLE,
>> -					 ctrl_en_val, ~ctrl_en_val);
>> +					 ctrl_en_val, 0);
>>   	if (ret) {
>>   		dev_err(led_data->priv->dev, "Failed to set ctrl:%d\n", ret);
>>   		return ret;
>> @@ -321,7 +327,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
>> @@ -339,11 +345,9 @@ static int lm3532_brightness_set(struct led_classdev *led_cdev,
>>   	if (ret)
>>   		goto unlock;
>>   
>> -	brightness_reg = LM3532_REG_CTRL_A_BRT + led->control_bank * 2;
>> -	brt_val = brt_val / LM3532_BRT_VAL_ADJUST;
>> -
>> +	brightness_reg = LM3532_REG_ZONE_TRGT_A + led->control_bank * 5 +
>> +			 (led->ctrl_brt_pointer >> 2);
>>   	ret = regmap_write(led->priv->regmap, brightness_reg, brt_val);
>> -
>>   unlock:
>>   	mutex_unlock(&led->priv->lock);
>>   	return ret;
>> @@ -356,8 +360,29 @@ static int lm3532_init_registers(struct lm3532_led *led)
>>   	unsigned int output_cfg_val = 0;
>>   	unsigned int output_cfg_shift = 0;
>>   	unsigned int output_cfg_mask = 0;
>> +	int brightness_config_reg;
>> +	int brightness_config_val;
>>   	int ret, i;
>>   
>> +	if (drvdata->enable_gpio)
>> +		gpiod_direction_output(drvdata->enable_gpio, 1);
>> +
>> +	brightness_config_reg = LM3532_REG_ZONE_CFG_A + led->control_bank * 2;
>> +	/* This could be hard coded to the default value but the control
> Coding style issue:
>
>          /*
>           * This is the preferred style for multi-line comments

OK checkpatch did not crib about it but I can change it.


>
>> +	 * 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;
> Could you shed more light on this hardware peculiarity?
> What kind of pointer is it?

The bits in the register are called Control A,B or C Brightness Pointer 
in the data sheet.

Basically BITS(4:2) control or points to which Zone target register will 
be used to control

the output current for the particular control bank

000 = Control A Zone Target 0
001 = Control A Zone Target 1
010 = Control A Zone Target 2
011 = Control A Zone Target 3
1XX = Control A Zone Target 4 (default)

I was trying to keep the variable and register bits name some what the same.

>
>> +	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 +407,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 +435,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)
>> @@ -542,12 +559,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;
> Coding style issue: you need curly braces around the body of "else"
> in this case as well.

No checkpatch complaint I will fix it.

Dan


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/3] leds: lm3532: Fix brightness control for i2c mode
  2019-08-01 19:17   ` Dan Murphy
@ 2019-08-01 19:48     ` Jacek Anaszewski
  0 siblings, 0 replies; 10+ messages in thread
From: Jacek Anaszewski @ 2019-08-01 19:48 UTC (permalink / raw)
  To: Dan Murphy, pavel, tony, sre, nekit1000, mpartap, merlijn
  Cc: linux-leds, linux-kernel

Dan,

On 8/1/19 9:17 PM, Dan Murphy wrote:
> Jacek
> 
> On 8/1/19 1:52 PM, Jacek Anaszewski wrote:
>> Hi Dan,
>>
>> Thank you for the patch set. Please find my comments
>> in the code below.
>>
>> On 8/1/19 5:14 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>
>>> ---
>>>   drivers/leds/leds-lm3532.c | 78 ++++++++++++++++++++++++--------------
>>>   1 file changed, 50 insertions(+), 28 deletions(-)
>>>
[...]
>>> +     * 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;
>> Could you shed more light on this hardware peculiarity?
>> What kind of pointer is it?
> 
> The bits in the register are called Control A,B or C Brightness Pointer
> in the data sheet.
> 
> Basically BITS(4:2) control or points to which Zone target register will
> be used to control
> 
> the output current for the particular control bank
> 
> 000 = Control A Zone Target 0
> 001 = Control A Zone Target 1
> 010 = Control A Zone Target 2
> 011 = Control A Zone Target 3
> 1XX = Control A Zone Target 4 (default)
> 
> I was trying to keep the variable and register bits name some what the
> same.

Thanks for the explanation.

If that's the name from data sheet then it's indeed reasonable to
keep the same name for the variable.

>>
>>> +    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 +407,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 +435,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)
>>> @@ -542,12 +559,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;
>> Coding style issue: you need curly braces around the body of "else"
>> in this case as well.
> 
> No checkpatch complaint I will fix it.

Check section 3) Placing Braces and Spaces from
Documentation/process/coding-style.rst.

-- 
Best regards,
Jacek Anaszewski

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/3] leds: lm3532: Fix brightness control for i2c mode
  2019-08-01 18:52 ` [PATCH 1/3] leds: lm3532: Fix brightness control for i2c mode Jacek Anaszewski
  2019-08-01 19:17   ` Dan Murphy
@ 2019-08-01 21:36   ` Pavel Machek
  2019-08-02  0:02     ` Dan Murphy
  1 sibling, 1 reply; 10+ messages in thread
From: Pavel Machek @ 2019-08-01 21:36 UTC (permalink / raw)
  To: Jacek Anaszewski
  Cc: Dan Murphy, tony, sre, nekit1000, mpartap, merlijn, linux-leds,
	linux-kernel

[-- Attachment #1: Type: text/plain, Size: 2892 bytes --]

Hi!

If we are going to complain about coding style... this should really
be split, one change per patch.

> > @@ -161,18 +167,18 @@ struct lm3532_data {
> >  };
> >  
> >  static const struct reg_default lm3532_reg_defs[] = {
> > -	{LM3532_REG_OUTPUT_CFG, 0xe4},
> > +	{LM3532_REG_OUTPUT_CFG, 0x24},
> >  	{LM3532_REG_STARTSHUT_RAMP, 0xc0},
> >  	{LM3532_REG_RT_RAMP, 0xc0},
> >  	{LM3532_REG_PWM_A_CFG, 0x82},
> >  	{LM3532_REG_PWM_B_CFG, 0x82},
> >  	{LM3532_REG_PWM_C_CFG, 0x82},
> >  	{LM3532_REG_ZONE_CFG_A, 0xf1},
> > -	{LM3532_REG_CTRL_A_BRT, 0xf3},
> > +	{LM3532_REG_CTRL_A_FS_CURR, 0x13},
> >  	{LM3532_REG_ZONE_CFG_B, 0xf1},
> > -	{LM3532_REG_CTRL_B_BRT, 0xf3},
> > +	{LM3532_REG_CTRL_B_FS_CURR, 0x13},
> >  	{LM3532_REG_ZONE_CFG_C, 0xf1},
> > -	{LM3532_REG_CTRL_C_BRT, 0xf3},
> > +	{LM3532_REG_CTRL_C_FS_CURR, 0x13},
> >  	{LM3532_REG_ENABLE, 0xf8},
> >  	{LM3532_ALS_CONFIG, 0x44},
> >  	{LM3532_REG_ZN_0_HI, 0x35},

Default register values; are they related to the rest?

> > @@ -302,7 +308,7 @@ static int lm3532_led_disable(struct lm3532_led *led_data)
> >  	int ret;
> >  
> >  	ret = regmap_update_bits(led_data->priv->regmap, LM3532_REG_ENABLE,
> > -					 ctrl_en_val, ~ctrl_en_val);
> > +					 ctrl_en_val, 0);
> >  	if (ret) {
> >  		dev_err(led_data->priv->dev, "Failed to set ctrl:%d\n", ret);
> >  		return ret;

This should have no functional impact, its just a clenaup, probably
should go separately. 

> > @@ -339,11 +345,9 @@ static int lm3532_brightness_set(struct led_classdev *led_cdev,
> >  	if (ret)
> >  		goto unlock;
> >  
> > -	brightness_reg = LM3532_REG_CTRL_A_BRT + led->control_bank * 2;
> > -	brt_val = brt_val / LM3532_BRT_VAL_ADJUST;
> > -
> > +	brightness_reg = LM3532_REG_ZONE_TRGT_A + led->control_bank * 5 +
> > +			 (led->ctrl_brt_pointer >> 2);
> >  	ret = regmap_write(led->priv->regmap, brightness_reg, brt_val);
> > -
> >  unlock:
> >  	mutex_unlock(&led->priv->lock);
> >  	return ret;

This is the core change, AFAICT.

> > @@ -356,8 +360,29 @@ static int lm3532_init_registers(struct lm3532_led *led)
> >  	unsigned int output_cfg_val = 0;
> >  	unsigned int output_cfg_shift = 0;
> >  	unsigned int output_cfg_mask = 0;
> > +	int brightness_config_reg;
> > +	int brightness_config_val;
> >  	int ret, i;
> >  
> > +	if (drvdata->enable_gpio)
> > +		gpiod_direction_output(drvdata->enable_gpio, 1);
> > +
> > +	brightness_config_reg = LM3532_REG_ZONE_CFG_A + led->control_bank * 2;
> > +	/* This could be hard coded to the default value but the control
> 

Code is moved, probably should go in separately. We'll have less fun
bisecting problems when things are separate...

Thanks and best regards,
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/3] leds: lm3532: Fix brightness control for i2c mode
  2019-08-01 21:36   ` Pavel Machek
@ 2019-08-02  0:02     ` Dan Murphy
  0 siblings, 0 replies; 10+ messages in thread
From: Dan Murphy @ 2019-08-02  0:02 UTC (permalink / raw)
  To: Pavel Machek, Jacek Anaszewski
  Cc: tony, sre, nekit1000, mpartap, merlijn, linux-leds, linux-kernel

Pavel

Thanks for the review

On 8/1/19 4:36 PM, Pavel Machek wrote:
> Hi!
>
> If we are going to complain about coding style... this should really
> be split, one change per patch.
>
>>> @@ -161,18 +167,18 @@ struct lm3532_data {
>>>   };
>>>   
>>>   static const struct reg_default lm3532_reg_defs[] = {
>>> -	{LM3532_REG_OUTPUT_CFG, 0xe4},
>>> +	{LM3532_REG_OUTPUT_CFG, 0x24},
>>>   	{LM3532_REG_STARTSHUT_RAMP, 0xc0},
>>>   	{LM3532_REG_RT_RAMP, 0xc0},
>>>   	{LM3532_REG_PWM_A_CFG, 0x82},
>>>   	{LM3532_REG_PWM_B_CFG, 0x82},
>>>   	{LM3532_REG_PWM_C_CFG, 0x82},
>>>   	{LM3532_REG_ZONE_CFG_A, 0xf1},
>>> -	{LM3532_REG_CTRL_A_BRT, 0xf3},
>>> +	{LM3532_REG_CTRL_A_FS_CURR, 0x13},
>>>   	{LM3532_REG_ZONE_CFG_B, 0xf1},
>>> -	{LM3532_REG_CTRL_B_BRT, 0xf3},
>>> +	{LM3532_REG_CTRL_B_FS_CURR, 0x13},
>>>   	{LM3532_REG_ZONE_CFG_C, 0xf1},
>>> -	{LM3532_REG_CTRL_C_BRT, 0xf3},
>>> +	{LM3532_REG_CTRL_C_FS_CURR, 0x13},
>>>   	{LM3532_REG_ENABLE, 0xf8},
>>>   	{LM3532_ALS_CONFIG, 0x44},
>>>   	{LM3532_REG_ZN_0_HI, 0x35},
> Default register values; are they related to the rest?

Yes and no. I changed the #define so we would see a change anyway.

And the default is 0x13.

I can move it to a clean up patch

>
>>> @@ -302,7 +308,7 @@ static int lm3532_led_disable(struct lm3532_led *led_data)
>>>   	int ret;
>>>   
>>>   	ret = regmap_update_bits(led_data->priv->regmap, LM3532_REG_ENABLE,
>>> -					 ctrl_en_val, ~ctrl_en_val);
>>> +					 ctrl_en_val, 0);
>>>   	if (ret) {
>>>   		dev_err(led_data->priv->dev, "Failed to set ctrl:%d\n", ret);
>>>   		return ret;
> This should have no functional impact, its just a clenaup, probably
> should go separately.

I took it from your patch.  Thought it was a good clean up.

I can move it to a separate patch and give you credit


>
>>> @@ -339,11 +345,9 @@ static int lm3532_brightness_set(struct led_classdev *led_cdev,
>>>   	if (ret)
>>>   		goto unlock;
>>>   
>>> -	brightness_reg = LM3532_REG_CTRL_A_BRT + led->control_bank * 2;
>>> -	brt_val = brt_val / LM3532_BRT_VAL_ADJUST;
>>> -
>>> +	brightness_reg = LM3532_REG_ZONE_TRGT_A + led->control_bank * 5 +
>>> +			 (led->ctrl_brt_pointer >> 2);
>>>   	ret = regmap_write(led->priv->regmap, brightness_reg, brt_val);
>>> -
>>>   unlock:
>>>   	mutex_unlock(&led->priv->lock);
>>>   	return ret;
> This is the core change, AFAICT.

Yep.  This is the fix you want.


>
>>> @@ -356,8 +360,29 @@ static int lm3532_init_registers(struct lm3532_led *led)
>>>   	unsigned int output_cfg_val = 0;
>>>   	unsigned int output_cfg_shift = 0;
>>>   	unsigned int output_cfg_mask = 0;
>>> +	int brightness_config_reg;
>>> +	int brightness_config_val;
>>>   	int ret, i;
>>>   
>>> +	if (drvdata->enable_gpio)
>>> +		gpiod_direction_output(drvdata->enable_gpio, 1);
>>> +
>>> +	brightness_config_reg = LM3532_REG_ZONE_CFG_A + led->control_bank * 2;
>>> +	/* This could be hard coded to the default value but the control
> Code is moved, probably should go in separately. We'll have less fun
> bisecting problems when things are separate...

On my Droid4 moving this enable call allowed the init to pass without a 
regmap failure.

But I did not see the same issue on the BBB with the LM3532 EVM.

Without this change in this patch the backlight failed to register.  I 
think we want to keep this change here.

Dan


>
> Thanks and best regards,
> 									Pavel
>

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2019-08-02  0:02 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-01 15:14 [PATCH 1/3] leds: lm3532: Fix brightness control for i2c mode Dan Murphy
2019-08-01 15:14 ` [PATCH 2/3] dt: lm3532: Add property for full scale current Dan Murphy
2019-08-01 18:53   ` Jacek Anaszewski
2019-08-01 19:12     ` Dan Murphy
2019-08-01 15:14 ` [PATCH 3/3] leds: lm3532: Add full scale current configuration Dan Murphy
2019-08-01 18:52 ` [PATCH 1/3] leds: lm3532: Fix brightness control for i2c mode Jacek Anaszewski
2019-08-01 19:17   ` Dan Murphy
2019-08-01 19:48     ` Jacek Anaszewski
2019-08-01 21:36   ` Pavel Machek
2019-08-02  0:02     ` Dan Murphy

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).