[PATCH v2 0/2] leds: lp55xx: configure internal charge pump

[PATCH v2 0/2] leds: lp55xx: configure internal charge pump

[Maarten Zanders]

A new option in the devicetree "ti,charge-pump-mode" allows the user to
configure the charge pump in a certain mode. Previously it was defaulting
to automatic mode.

v1 of the patch implemented a bool to disable the charge pump and had some
issues in the yaml binding. To avoid future modifications, implement all
possible configurations of the charge pump.

Maarten Zanders (2):
  dt-bindings: leds-lp55xx: add ti,charge-pump-mode
  leds: lp55xx: configure internal charge pump

 .../devicetree/bindings/leds/leds-lp55xx.yaml |  8 +++++++
 drivers/leds/leds-lp5521.c                    | 12 +++++-----
 drivers/leds/leds-lp5523.c                    | 18 ++++++++++-----
 drivers/leds/leds-lp55xx-common.c             | 22 +++++++++++++++++++
 drivers/leds/leds-lp8501.c                    |  8 +++++--
 include/linux/platform_data/leds-lp55xx.h     |  9 ++++++++
 6 files changed, 64 insertions(+), 13 deletions(-)

-- 
2.37.3

[PATCH v2 1/2] dt-bindings: leds-lp55xx: add ti,charge-pump-mode

[Maarten Zanders]

Add a binding to configure the internal charge pump for lp55xx.

Signed-off-by: Maarten Zanders <maarten.zanders@mind.be>
---
 Documentation/devicetree/bindings/leds/leds-lp55xx.yaml | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/leds/leds-lp55xx.yaml b/Documentation/devicetree/bindings/leds/leds-lp55xx.yaml
index ae607911f1db..40ab3cf597a4 100644
--- a/Documentation/devicetree/bindings/leds/leds-lp55xx.yaml
+++ b/Documentation/devicetree/bindings/leds/leds-lp55xx.yaml
@@ -66,6 +66,13 @@ properties:
   '#size-cells':
     const: 0
 
+  ti,charge-pump-mode:
+    description:
+      Set the operating mode of the internal charge pump.
+    $ref: /schemas/types.yaml#/definitions/string
+    enum: [off, auto, bypass, boost]
+    default: auto
+
 patternProperties:
   '^multi-led@[0-8]$':
     type: object
@@ -164,6 +171,7 @@ examples:
             reg = <0x32>;
             clock-mode = /bits/ 8 <2>;
             pwr-sel = /bits/ 8 <3>;	/* D1~9 connected to VOUT */
+            ti,charge-pump-mode = "bypass";
 
             led@0 {
                 reg = <0>;
-- 
2.37.3

[PATCH v2 2/2] leds: lp55xx: configure internal charge pump

[Maarten Zanders]

The LP55xx range of devices have an internal charge pump which
can (automatically) increase the output voltage towards the
LED's, boosting the output voltage to 4.5V.

Implement this option from the devicetree. When the setting
is not present it will operate in automatic mode as before.

Tested on LP55231. Datasheet analysis shows that LP5521, LP5523
and LP8501 are identical in topology and are modified in the
same way.

Signed-off-by: Maarten Zanders <maarten.zanders@mind.be>
---
 drivers/leds/leds-lp5521.c                | 12 ++++++------
 drivers/leds/leds-lp5523.c                | 18 +++++++++++++-----
 drivers/leds/leds-lp55xx-common.c         | 22 ++++++++++++++++++++++
 drivers/leds/leds-lp8501.c                |  8 ++++++--
 include/linux/platform_data/leds-lp55xx.h |  9 +++++++++
 5 files changed, 56 insertions(+), 13 deletions(-)

diff --git a/drivers/leds/leds-lp5521.c b/drivers/leds/leds-lp5521.c
index 19478d9c19a7..ee1c72cffdab 100644
--- a/drivers/leds/leds-lp5521.c
+++ b/drivers/leds/leds-lp5521.c
@@ -58,14 +58,11 @@
 /* CONFIG register */
 #define LP5521_PWM_HF			0x40	/* PWM: 0 = 256Hz, 1 = 558Hz */
 #define LP5521_PWRSAVE_EN		0x20	/* 1 = Power save mode */
-#define LP5521_CP_MODE_OFF		0	/* Charge pump (CP) off */
-#define LP5521_CP_MODE_BYPASS		8	/* CP forced to bypass mode */
-#define LP5521_CP_MODE_1X5		0x10	/* CP forced to 1.5x mode */
-#define LP5521_CP_MODE_AUTO		0x18	/* Automatic mode selection */
+#define LP5521_CP_MODE_MASK		0x18	/* Charge pump mode */
+#define LP5521_CP_MODE_SHIFT		3
 #define LP5521_R_TO_BATT		0x04	/* R out: 0 = CP, 1 = Vbat */
 #define LP5521_CLK_INT			0x01	/* Internal clock */
-#define LP5521_DEFAULT_CFG		\
-	(LP5521_PWM_HF | LP5521_PWRSAVE_EN | LP5521_CP_MODE_AUTO)
+#define LP5521_DEFAULT_CFG		(LP5521_PWM_HF | LP5521_PWRSAVE_EN)
 
 /* Status */
 #define LP5521_EXT_CLK_USED		0x08
@@ -310,6 +307,9 @@ static int lp5521_post_init_device(struct lp55xx_chip *chip)
 	if (!lp55xx_is_extclk_used(chip))
 		val |= LP5521_CLK_INT;
 
+	val |= (chip->pdata->charge_pump_mode << LP5521_CP_MODE_SHIFT) &
+	       LP5521_CP_MODE_MASK;
+
 	ret = lp55xx_write(chip, LP5521_REG_CONFIG, val);
 	if (ret)
 		return ret;
diff --git a/drivers/leds/leds-lp5523.c b/drivers/leds/leds-lp5523.c
index e08e3de1428d..b5d10d4252e6 100644
--- a/drivers/leds/leds-lp5523.c
+++ b/drivers/leds/leds-lp5523.c
@@ -57,8 +57,12 @@
 #define LP5523_AUTO_INC			0x40
 #define LP5523_PWR_SAVE			0x20
 #define LP5523_PWM_PWR_SAVE		0x04
-#define LP5523_CP_AUTO			0x18
+#define LP5523_CP_MODE_MASK		0x18
+#define LP5523_CP_MODE_SHIFT		3
 #define LP5523_AUTO_CLK			0x02
+#define LP5523_DEFAULT_CONFIG	\
+	(LP5523_AUTO_INC | LP5523_PWR_SAVE |\
+	 LP5523_AUTO_CLK | LP5523_PWM_PWR_SAVE)
 
 #define LP5523_EN_LEDTEST		0x80
 #define LP5523_LEDTEST_DONE		0x80
@@ -125,6 +129,7 @@ static void lp5523_set_led_current(struct lp55xx_led *led, u8 led_current)
 static int lp5523_post_init_device(struct lp55xx_chip *chip)
 {
 	int ret;
+	int val;
 
 	ret = lp55xx_write(chip, LP5523_REG_ENABLE, LP5523_ENABLE);
 	if (ret)
@@ -133,10 +138,13 @@ static int lp5523_post_init_device(struct lp55xx_chip *chip)
 	/* Chip startup time is 500 us, 1 - 2 ms gives some margin */
 	usleep_range(1000, 2000);
 
-	ret = lp55xx_write(chip, LP5523_REG_CONFIG,
-			    LP5523_AUTO_INC | LP5523_PWR_SAVE |
-			    LP5523_CP_AUTO | LP5523_AUTO_CLK |
-			    LP5523_PWM_PWR_SAVE);
+	val = LP5523_DEFAULT_CONFIG;
+
+	val |= (chip->pdata->charge_pump_mode << LP5523_CP_MODE_SHIFT) &
+	       LP5523_CP_MODE_MASK;
+
+	ret = lp55xx_write(chip, LP5523_REG_CONFIG, val);
+
 	if (ret)
 		return ret;
 
diff --git a/drivers/leds/leds-lp55xx-common.c b/drivers/leds/leds-lp55xx-common.c
index c1940964067a..a690a484fd7b 100644
--- a/drivers/leds/leds-lp55xx-common.c
+++ b/drivers/leds/leds-lp55xx-common.c
@@ -653,6 +653,13 @@ static int lp55xx_parse_logical_led(struct device_node *np,
 	return ret;
 }
 
+static const char * const charge_pump_modes[] = {
+	[LP55XX_CP_OFF] = "off",
+	[LP55XX_CP_BYPASS] = "bypass",
+	[LP55XX_CP_BOOST] = "boost",
+	[LP55XX_CP_AUTO] = "auto",
+};
+
 struct lp55xx_platform_data *lp55xx_of_populate_pdata(struct device *dev,
 						      struct device_node *np,
 						      struct lp55xx_chip *chip)
@@ -661,6 +668,8 @@ struct lp55xx_platform_data *lp55xx_of_populate_pdata(struct device *dev,
 	struct lp55xx_platform_data *pdata;
 	struct lp55xx_led_config *cfg;
 	int num_channels;
+	enum lp55xx_charge_pump_mode cp_mode;
+	const char *pm;
 	int i = 0;
 	int ret;
 
@@ -691,6 +700,19 @@ struct lp55xx_platform_data *lp55xx_of_populate_pdata(struct device *dev,
 		i++;
 	}
 
+	pdata->charge_pump_mode = LP55XX_CP_AUTO;
+	ret = of_property_read_string(np, "ti,charge-pump-mode", &pm);
+	if (!ret) {
+		for (cp_mode = LP55XX_CP_OFF;
+		     cp_mode < ARRAY_SIZE(charge_pump_modes);
+		     cp_mode++) {
+			if (!strcasecmp(pm, charge_pump_modes[cp_mode])) {
+				pdata->charge_pump_mode = cp_mode;
+				break;
+			}
+		}
+	}
+
 	of_property_read_string(np, "label", &pdata->label);
 	of_property_read_u8(np, "clock-mode", &pdata->clock_mode);
 
diff --git a/drivers/leds/leds-lp8501.c b/drivers/leds/leds-lp8501.c
index ae11a02c0ab2..f0e70e116919 100644
--- a/drivers/leds/leds-lp8501.c
+++ b/drivers/leds/leds-lp8501.c
@@ -53,10 +53,11 @@
 #define LP8501_PWM_PSAVE		BIT(7)
 #define LP8501_AUTO_INC			BIT(6)
 #define LP8501_PWR_SAVE			BIT(5)
-#define LP8501_CP_AUTO			0x18
+#define LP8501_CP_MODE_MASK		0x18
+#define LP8501_CP_MODE_SHIFT		3
 #define LP8501_INT_CLK			BIT(0)
 #define LP8501_DEFAULT_CFG	\
-	(LP8501_PWM_PSAVE | LP8501_AUTO_INC | LP8501_PWR_SAVE | LP8501_CP_AUTO)
+	(LP8501_PWM_PSAVE | LP8501_AUTO_INC | LP8501_PWR_SAVE)
 
 #define LP8501_REG_RESET		0x3D
 #define LP8501_RESET			0xFF
@@ -102,6 +103,9 @@ static int lp8501_post_init_device(struct lp55xx_chip *chip)
 	if (chip->pdata->clock_mode != LP55XX_CLOCK_EXT)
 		val |= LP8501_INT_CLK;
 
+	val |= (chip->pdata->charge_pump_mode << LP8501_CP_MODE_SHIFT) &
+	       LP8501_CP_MODE_MASK;
+
 	ret = lp55xx_write(chip, LP8501_REG_CONFIG, val);
 	if (ret)
 		return ret;
diff --git a/include/linux/platform_data/leds-lp55xx.h b/include/linux/platform_data/leds-lp55xx.h
index 3441064713a3..6ea1a74f94e4 100644
--- a/include/linux/platform_data/leds-lp55xx.h
+++ b/include/linux/platform_data/leds-lp55xx.h
@@ -50,6 +50,13 @@ enum lp8501_pwr_sel {
 	LP8501_ALL_VOUT,	/* D1~9 are connected to VOUT */
 };
 
+enum lp55xx_charge_pump_mode {
+	LP55XX_CP_OFF = 0,
+	LP55XX_CP_BYPASS = 1,
+	LP55XX_CP_BOOST = 2,
+	LP55XX_CP_AUTO = 3,
+};
+
 /*
  * struct lp55xx_platform_data
  * @led_config        : Configurable led class device
@@ -73,6 +80,8 @@ struct lp55xx_platform_data {
 	/* Clock configuration */
 	u8 clock_mode;
 
+	enum lp55xx_charge_pump_mode charge_pump_mode;
+
 	/* optional enable GPIO */
 	struct gpio_desc *enable_gpiod;
 
-- 
2.37.3

Re: [PATCH v2 0/2] leds: lp55xx: configure internal charge pump

[Krzysztof Kozlowski]

On 10/01/2023 10:23, Maarten Zanders wrote:
> A new option in the devicetree "ti,charge-pump-mode" allows the user to
> configure the charge pump in a certain mode. Previously it was defaulting
> to automatic mode.

Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC.  It might happen, that command when run on an older
kernel, gives you outdated entries.  Therefore please be sure you base
your patches on recent Linux kernel.

Best regards,
Krzysztof

Re: [PATCH v2 1/2] dt-bindings: leds-lp55xx: add ti,charge-pump-mode

[Krzysztof Kozlowski]

On 10/01/2023 10:23, Maarten Zanders wrote:
> Add a binding to configure the internal charge pump for lp55xx.
> 
> Signed-off-by: Maarten Zanders <maarten.zanders@mind.be>
> ---


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH v2 2/2] leds: lp55xx: configure internal charge pump

[Lee Jones]

On Tue, 10 Jan 2023, Maarten Zanders wrote:

> The LP55xx range of devices have an internal charge pump which
> can (automatically) increase the output voltage towards the
> LED's, boosting the output voltage to 4.5V.
> 
> Implement this option from the devicetree. When the setting
> is not present it will operate in automatic mode as before.
> 
> Tested on LP55231. Datasheet analysis shows that LP5521, LP5523
> and LP8501 are identical in topology and are modified in the
> same way.
> 
> Signed-off-by: Maarten Zanders <maarten.zanders@mind.be>
> ---
>  drivers/leds/leds-lp5521.c                | 12 ++++++------
>  drivers/leds/leds-lp5523.c                | 18 +++++++++++++-----
>  drivers/leds/leds-lp55xx-common.c         | 22 ++++++++++++++++++++++
>  drivers/leds/leds-lp8501.c                |  8 ++++++--
>  include/linux/platform_data/leds-lp55xx.h |  9 +++++++++
>  5 files changed, 56 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/leds/leds-lp5521.c b/drivers/leds/leds-lp5521.c
> index 19478d9c19a7..ee1c72cffdab 100644
> --- a/drivers/leds/leds-lp5521.c
> +++ b/drivers/leds/leds-lp5521.c
> @@ -58,14 +58,11 @@
>  /* CONFIG register */
>  #define LP5521_PWM_HF			0x40	/* PWM: 0 = 256Hz, 1 = 558Hz */
>  #define LP5521_PWRSAVE_EN		0x20	/* 1 = Power save mode */
> -#define LP5521_CP_MODE_OFF		0	/* Charge pump (CP) off */
> -#define LP5521_CP_MODE_BYPASS		8	/* CP forced to bypass mode */
> -#define LP5521_CP_MODE_1X5		0x10	/* CP forced to 1.5x mode */
> -#define LP5521_CP_MODE_AUTO		0x18	/* Automatic mode selection */
> +#define LP5521_CP_MODE_MASK		0x18	/* Charge pump mode */
> +#define LP5521_CP_MODE_SHIFT		3
>  #define LP5521_R_TO_BATT		0x04	/* R out: 0 = CP, 1 = Vbat */
>  #define LP5521_CLK_INT			0x01	/* Internal clock */
> -#define LP5521_DEFAULT_CFG		\
> -	(LP5521_PWM_HF | LP5521_PWRSAVE_EN | LP5521_CP_MODE_AUTO)
> +#define LP5521_DEFAULT_CFG		(LP5521_PWM_HF | LP5521_PWRSAVE_EN)
>  
>  /* Status */
>  #define LP5521_EXT_CLK_USED		0x08
> @@ -310,6 +307,9 @@ static int lp5521_post_init_device(struct lp55xx_chip *chip)
>  	if (!lp55xx_is_extclk_used(chip))
>  		val |= LP5521_CLK_INT;
>  
> +	val |= (chip->pdata->charge_pump_mode << LP5521_CP_MODE_SHIFT) &
> +	       LP5521_CP_MODE_MASK;
> +
>  	ret = lp55xx_write(chip, LP5521_REG_CONFIG, val);
>  	if (ret)
>  		return ret;
> diff --git a/drivers/leds/leds-lp5523.c b/drivers/leds/leds-lp5523.c
> index e08e3de1428d..b5d10d4252e6 100644
> --- a/drivers/leds/leds-lp5523.c
> +++ b/drivers/leds/leds-lp5523.c
> @@ -57,8 +57,12 @@
>  #define LP5523_AUTO_INC			0x40
>  #define LP5523_PWR_SAVE			0x20
>  #define LP5523_PWM_PWR_SAVE		0x04
> -#define LP5523_CP_AUTO			0x18
> +#define LP5523_CP_MODE_MASK		0x18
> +#define LP5523_CP_MODE_SHIFT		3
>  #define LP5523_AUTO_CLK			0x02
> +#define LP5523_DEFAULT_CONFIG	\
> +	(LP5523_AUTO_INC | LP5523_PWR_SAVE |\
> +	 LP5523_AUTO_CLK | LP5523_PWM_PWR_SAVE)
>  
>  #define LP5523_EN_LEDTEST		0x80
>  #define LP5523_LEDTEST_DONE		0x80
> @@ -125,6 +129,7 @@ static void lp5523_set_led_current(struct lp55xx_led *led, u8 led_current)
>  static int lp5523_post_init_device(struct lp55xx_chip *chip)
>  {
>  	int ret;
> +	int val;
>  
>  	ret = lp55xx_write(chip, LP5523_REG_ENABLE, LP5523_ENABLE);
>  	if (ret)
> @@ -133,10 +138,13 @@ static int lp5523_post_init_device(struct lp55xx_chip *chip)
>  	/* Chip startup time is 500 us, 1 - 2 ms gives some margin */
>  	usleep_range(1000, 2000);
>  
> -	ret = lp55xx_write(chip, LP5523_REG_CONFIG,
> -			    LP5523_AUTO_INC | LP5523_PWR_SAVE |
> -			    LP5523_CP_AUTO | LP5523_AUTO_CLK |
> -			    LP5523_PWM_PWR_SAVE);
> +	val = LP5523_DEFAULT_CONFIG;
> +
> +	val |= (chip->pdata->charge_pump_mode << LP5523_CP_MODE_SHIFT) &
> +	       LP5523_CP_MODE_MASK;
> +
> +	ret = lp55xx_write(chip, LP5523_REG_CONFIG, val);
> +
>  	if (ret)
>  		return ret;
>  
> diff --git a/drivers/leds/leds-lp55xx-common.c b/drivers/leds/leds-lp55xx-common.c
> index c1940964067a..a690a484fd7b 100644
> --- a/drivers/leds/leds-lp55xx-common.c
> +++ b/drivers/leds/leds-lp55xx-common.c
> @@ -653,6 +653,13 @@ static int lp55xx_parse_logical_led(struct device_node *np,
>  	return ret;
>  }
>  
> +static const char * const charge_pump_modes[] = {
> +	[LP55XX_CP_OFF] = "off",
> +	[LP55XX_CP_BYPASS] = "bypass",
> +	[LP55XX_CP_BOOST] = "boost",
> +	[LP55XX_CP_AUTO] = "auto",
> +};
> +
>  struct lp55xx_platform_data *lp55xx_of_populate_pdata(struct device *dev,
>  						      struct device_node *np,
>  						      struct lp55xx_chip *chip)
> @@ -661,6 +668,8 @@ struct lp55xx_platform_data *lp55xx_of_populate_pdata(struct device *dev,
>  	struct lp55xx_platform_data *pdata;
>  	struct lp55xx_led_config *cfg;
>  	int num_channels;
> +	enum lp55xx_charge_pump_mode cp_mode;
> +	const char *pm;
>  	int i = 0;
>  	int ret;
>  
> @@ -691,6 +700,19 @@ struct lp55xx_platform_data *lp55xx_of_populate_pdata(struct device *dev,
>  		i++;
>  	}
>  
> +	pdata->charge_pump_mode = LP55XX_CP_AUTO;
> +	ret = of_property_read_string(np, "ti,charge-pump-mode", &pm);
> +	if (!ret) {
> +		for (cp_mode = LP55XX_CP_OFF;
> +		     cp_mode < ARRAY_SIZE(charge_pump_modes);
> +		     cp_mode++) {
> +			if (!strcasecmp(pm, charge_pump_modes[cp_mode])) {
> +				pdata->charge_pump_mode = cp_mode;
> +				break;
> +			}
> +		}
> +	}

A little over-engineered, no?

Why not make the property a numerical value, then simply:

  ret = of_property_read_u32(np, "ti,charge-pump-mode", &pdata->charge_pump_mode);
  if (ret)
          data->charge_pump_mode = LP55XX_CP_AUTO;

Elevates the requirement for the crumby indexed array of strings above.

Remainder looks sane enough.

-- 
Lee Jones [李琼斯]

Re: [PATCH v2 2/2] leds: lp55xx: configure internal charge pump

[Maarten Zanders]

>>   
>> +	pdata->charge_pump_mode = LP55XX_CP_AUTO;
>> +	ret = of_property_read_string(np, "ti,charge-pump-mode", &pm);
>> +	if (!ret) {
>> +		for (cp_mode = LP55XX_CP_OFF;
>> +		     cp_mode < ARRAY_SIZE(charge_pump_modes);
>> +		     cp_mode++) {
>> +			if (!strcasecmp(pm, charge_pump_modes[cp_mode])) {
>> +				pdata->charge_pump_mode = cp_mode;
>> +				break;
>> +			}
>> +		}
>> +	}
> A little over-engineered, no?
>
> Why not make the property a numerical value, then simply:
>
>    ret = of_property_read_u32(np, "ti,charge-pump-mode", &pdata->charge_pump_mode);
>    if (ret)
>            data->charge_pump_mode = LP55XX_CP_AUTO;
>
> Elevates the requirement for the crumby indexed array of strings above.
>
> Remainder looks sane enough.

Thanks for your feedback.

I won't argue that your implementation isn't far more simple. The idea 
was to have an elaborate and clear and obvious devicetree, but that can 
also be achieved by moving constants into 
/includes/dt-bindings/leds/leds-lp55xx.h. Would that be more acceptable?

Maarten

Re: [PATCH v2 2/2] leds: lp55xx: configure internal charge pump

[Lee Jones]

On Fri, 20 Jan 2023, Maarten Zanders wrote:

> 
> > > +	pdata->charge_pump_mode = LP55XX_CP_AUTO;
> > > +	ret = of_property_read_string(np, "ti,charge-pump-mode", &pm);
> > > +	if (!ret) {
> > > +		for (cp_mode = LP55XX_CP_OFF;
> > > +		     cp_mode < ARRAY_SIZE(charge_pump_modes);
> > > +		     cp_mode++) {
> > > +			if (!strcasecmp(pm, charge_pump_modes[cp_mode])) {
> > > +				pdata->charge_pump_mode = cp_mode;
> > > +				break;
> > > +			}
> > > +		}
> > > +	}
> > A little over-engineered, no?
> > 
> > Why not make the property a numerical value, then simply:
> > 
> >    ret = of_property_read_u32(np, "ti,charge-pump-mode", &pdata->charge_pump_mode);
> >    if (ret)
> >            data->charge_pump_mode = LP55XX_CP_AUTO;
> > 
> > Elevates the requirement for the crumby indexed array of strings above.
> > 
> > Remainder looks sane enough.
> 
> Thanks for your feedback.
> 
> I won't argue that your implementation isn't far more simple. The idea was
> to have an elaborate and clear and obvious devicetree, but that can also be
> achieved by moving constants into /includes/dt-bindings/leds/leds-lp55xx.h.
> Would that be more acceptable?

Yes please.

-- 
Lee Jones [李琼斯]