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

[PATCH v4 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. The previous implementation
was "auto" mode, which remains the default.

v1 of the patch implemented a bool to disable the charge pump and had some
issues in the yaml binding.

v2 implemented all options of the charge pump as a string which was too
complex to parse & check.

v3 replaces the string by constants.

v4 resend with changelog (notes) in each patch

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              | 14 ++++++++++++++
 drivers/leds/leds-lp8501.c                     |  8 ++++++--
 include/dt-bindings/leds/leds-lp55xx.h         | 10 ++++++++++
 include/linux/platform_data/leds-lp55xx.h      |  3 +++
 7 files changed, 60 insertions(+), 13 deletions(-)
 create mode 100644 include/dt-bindings/leds/leds-lp55xx.h

-- 
2.37.3

[PATCH v4 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>
---

Notes:
    v1: implement as bool to disable charge pump
    v2: rewrite to use string configuration, supporting all modes
    v3: simplification by replacing string option by u8 constant,
        removing previous Reviewed-by tags as it's a complete
        rewrite of the patch.
    v4: added notes

 .../devicetree/bindings/leds/leds-lp55xx.yaml          |  8 ++++++++
 include/dt-bindings/leds/leds-lp55xx.h                 | 10 ++++++++++
 2 files changed, 18 insertions(+)
 create mode 100644 include/dt-bindings/leds/leds-lp55xx.h

diff --git a/Documentation/devicetree/bindings/leds/leds-lp55xx.yaml b/Documentation/devicetree/bindings/leds/leds-lp55xx.yaml
index ae607911f1db..22e63d89d770 100644
--- a/Documentation/devicetree/bindings/leds/leds-lp55xx.yaml
+++ b/Documentation/devicetree/bindings/leds/leds-lp55xx.yaml
@@ -66,6 +66,12 @@ properties:
   '#size-cells':
     const: 0
 
+  ti,charge-pump-mode:
+    description:
+      Set the operating mode of the internal charge pump as defined in
+      <dt-bindings/leds/leds-lp55xx.h>. Defaults to auto.
+    $ref: /schemas/types.yaml#/definitions/uint8
+
 patternProperties:
   '^multi-led@[0-8]$':
     type: object
@@ -152,6 +158,7 @@ additionalProperties: false
 examples:
   - |
     #include <dt-bindings/leds/common.h>
+    #include <dt-bindings/leds/leds-lp55xx.h>
 
     i2c {
         #address-cells = <1>;
@@ -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 = /bits/ 8 <LP55XX_CP_BYPASS>;
 
             led@0 {
                 reg = <0>;
diff --git a/include/dt-bindings/leds/leds-lp55xx.h b/include/dt-bindings/leds/leds-lp55xx.h
new file mode 100644
index 000000000000..8f59c1c12dee
--- /dev/null
+++ b/include/dt-bindings/leds/leds-lp55xx.h
@@ -0,0 +1,10 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _DT_BINDINGS_LEDS_LP55XX_H
+#define _DT_BINDINGS_LEDS_LP55XX_H
+
+#define LP55XX_CP_OFF		0
+#define LP55XX_CP_BYPASS	1
+#define LP55XX_CP_BOOST		2
+#define LP55XX_CP_AUTO		3
+
+#endif /* _DT_BINDINGS_LEDS_LP55XX_H */
-- 
2.37.3

[PATCH v4 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>
---

Notes:
    v1: implement as bool to disable charge pump
    v2: rewrite to use string configuration, supporting all modes
    v3: simplification by replacing string from DTS by constant
    v4: added notes

 drivers/leds/leds-lp5521.c                | 12 ++++++------
 drivers/leds/leds-lp5523.c                | 18 +++++++++++++-----
 drivers/leds/leds-lp55xx-common.c         | 14 ++++++++++++++
 drivers/leds/leds-lp8501.c                |  8 ++++++--
 include/linux/platform_data/leds-lp55xx.h |  3 +++
 5 files changed, 42 insertions(+), 13 deletions(-)

diff --git a/drivers/leds/leds-lp5521.c b/drivers/leds/leds-lp5521.c
index 19478d9c19a7..76c6b81afb38 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..086033860a6f 100644
--- a/drivers/leds/leds-lp55xx-common.c
+++ b/drivers/leds/leds-lp55xx-common.c
@@ -19,6 +19,8 @@
 #include <linux/slab.h>
 #include <linux/gpio/consumer.h>
 
+#include <dt-bindings/leds/leds-lp55xx.h>
+
 #include "leds-lp55xx-common.h"
 
 /* External clock rate */
@@ -691,6 +693,18 @@ struct lp55xx_platform_data *lp55xx_of_populate_pdata(struct device *dev,
 		i++;
 	}
 
+	ret = of_property_read_u8(np, "ti,charge-pump-mode",
+				  &pdata->charge_pump_mode);
+	if (ret) {
+		pdata->charge_pump_mode = LP55XX_CP_AUTO;
+	} else {
+		if (pdata->charge_pump_mode > LP55XX_CP_AUTO) {
+			dev_err(dev, "invalid charge pump mode %d\n",
+				pdata->charge_pump_mode);
+			return ERR_PTR(-EINVAL);
+		}
+	}
+
 	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..9a738979e1ce 100644
--- a/include/linux/platform_data/leds-lp55xx.h
+++ b/include/linux/platform_data/leds-lp55xx.h
@@ -73,6 +73,9 @@ struct lp55xx_platform_data {
 	/* Clock configuration */
 	u8 clock_mode;
 
+	/* Charge pump mode */
+	u8 charge_pump_mode;
+
 	/* optional enable GPIO */
 	struct gpio_desc *enable_gpiod;
 
-- 
2.37.3

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

[Krzysztof Kozlowski]

On 02/02/2023 11:10, Maarten Zanders wrote:
> Add a binding to configure the internal charge pump for lp55xx.
> 
> Signed-off-by: Maarten Zanders <maarten.zanders@mind.be>
> ---
> 
> Notes:
>     v1: implement as bool to disable charge pump
>     v2: rewrite to use string configuration, supporting all modes
>     v3: simplification by replacing string option by u8 constant,
>         removing previous Reviewed-by tags as it's a complete
>         rewrite of the patch.
>     v4: added notes


> 
>  .../devicetree/bindings/leds/leds-lp55xx.yaml          |  8 ++++++++
>  include/dt-bindings/leds/leds-lp55xx.h                 | 10 ++++++++++
>  2 files changed, 18 insertions(+)
>  create mode 100644 include/dt-bindings/leds/leds-lp55xx.h
> 
> diff --git a/Documentation/devicetree/bindings/leds/leds-lp55xx.yaml b/Documentation/devicetree/bindings/leds/leds-lp55xx.yaml
> index ae607911f1db..22e63d89d770 100644
> --- a/Documentation/devicetree/bindings/leds/leds-lp55xx.yaml
> +++ b/Documentation/devicetree/bindings/leds/leds-lp55xx.yaml
> @@ -66,6 +66,12 @@ properties:
>    '#size-cells':
>      const: 0
>  
> +  ti,charge-pump-mode:
> +    description:
> +      Set the operating mode of the internal charge pump as defined in
> +      <dt-bindings/leds/leds-lp55xx.h>. Defaults to auto.
> +    $ref: /schemas/types.yaml#/definitions/uint8
> +
>  patternProperties:
>    '^multi-led@[0-8]$':
>      type: object
> @@ -152,6 +158,7 @@ additionalProperties: false
>  examples:
>    - |
>      #include <dt-bindings/leds/common.h>
> +    #include <dt-bindings/leds/leds-lp55xx.h>
>  
>      i2c {
>          #address-cells = <1>;
> @@ -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 = /bits/ 8 <LP55XX_CP_BYPASS>;


No. V2 was correct. What happened here? You got review for v2, but
suddenly entire patch goes into other direction...

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

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



> diff --git a/include/dt-bindings/leds/leds-lp55xx.h b/include/dt-bindings/leds/leds-lp55xx.h
> new file mode 100644
> index 000000000000..8f59c1c12dee
> --- /dev/null
> +++ b/include/dt-bindings/leds/leds-lp55xx.h
> @@ -0,0 +1,10 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _DT_BINDINGS_LEDS_LP55XX_H
> +#define _DT_BINDINGS_LEDS_LP55XX_H
> +
> +#define LP55XX_CP_OFF		0
> +#define LP55XX_CP_BYPASS	1
> +#define LP55XX_CP_BOOST		2
> +#define LP55XX_CP_AUTO		3

Additionally, these are not used, so it's a dead binding. Drop. Sorry,
this is not the approach you should take.

Best regards,
Krzysztof

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

[Maarten Zanders]

First off, bear with me here, I only recently started upstreaming 
patches to kernel. It still feels like navigating an extremely busy 
shipping lane... Either way, your feedback is highly valued.

On 2/2/23 14:15, Krzysztof Kozlowski wrote:
>
>> diff --git a/include/dt-bindings/leds/leds-lp55xx.h b/include/dt-bindings/leds/leds-lp55xx.h
>> new file mode 100644
>> index 000000000000..8f59c1c12dee
>> --- /dev/null
>> +++ b/include/dt-bindings/leds/leds-lp55xx.h
>> @@ -0,0 +1,10 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +#ifndef _DT_BINDINGS_LEDS_LP55XX_H
>> +#define _DT_BINDINGS_LEDS_LP55XX_H
>> +
>> +#define LP55XX_CP_OFF		0
>> +#define LP55XX_CP_BYPASS	1
>> +#define LP55XX_CP_BOOST		2
>> +#define LP55XX_CP_AUTO		3
> Additionally, these are not used, so it's a dead binding. Drop. Sorry,
> this is not the approach you should take.
>
> Best regards,
> Krzysztof
>
These definitions are intended to be used in the DTS's, so it seems 
normal to me that most of them go unused in the code? What am I missing?

As for the changes v2 > v3, this was based on input I got on v2 from Lee 
Jones, maintainer for leds, on the implementation of the parsing of this 
option:

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

I found examples of similar configuration options of both types with 
other drivers in the kernel tree (ie string & uint8). I can appreciate 
both viewpoints but unfortunately cannot comply with both.


Best regards,
Maarten

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

[Krzysztof Kozlowski]

On 02/02/2023 14:35, Maarten Zanders wrote:
> First off, bear with me here, I only recently started upstreaming 
> patches to kernel. It still feels like navigating an extremely busy 
> shipping lane... Either way, your feedback is highly valued.
> 
> On 2/2/23 14:15, Krzysztof Kozlowski wrote:
>>
>>> diff --git a/include/dt-bindings/leds/leds-lp55xx.h b/include/dt-bindings/leds/leds-lp55xx.h
>>> new file mode 100644
>>> index 000000000000..8f59c1c12dee
>>> --- /dev/null
>>> +++ b/include/dt-bindings/leds/leds-lp55xx.h
>>> @@ -0,0 +1,10 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>> +#ifndef _DT_BINDINGS_LEDS_LP55XX_H
>>> +#define _DT_BINDINGS_LEDS_LP55XX_H
>>> +
>>> +#define LP55XX_CP_OFF		0
>>> +#define LP55XX_CP_BYPASS	1
>>> +#define LP55XX_CP_BOOST		2
>>> +#define LP55XX_CP_AUTO		3
>> Additionally, these are not used, so it's a dead binding. Drop. Sorry,
>> this is not the approach you should take.
>>
>> Best regards,
>> Krzysztof
>>
> These definitions are intended to be used in the DTS's, so it seems 
> normal to me that most of them go unused in the code? What am I missing?

Bindings mean drivers are using them. Your driver is not using it. It's
a register value, isn't it? Register values are not suitable for
bindings. There is no need for them to be in bindings.

> 
> As for the changes v2 > v3, this was based on input I got on v2 from Lee 
> Jones, maintainer for leds, on the implementation of the parsing of this 
> option:
> 
>>> +	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;
> 
> I found examples of similar configuration options of both types with 
> other drivers in the kernel tree (ie string & uint8). I can appreciate 
> both viewpoints but unfortunately cannot comply with both.

Strings in DTS are usually easier to for humans to read, but it's not a
requirement to use them. The problem of storing register values is that
binding is tied/coupled with hardware programming model, so you cannot
add a new device if the register value is a bit different (e.g.
LP55XX_CP_OFF is 0x1). You need entire new binding for such case. With
string - no need. With binding constants (IDs) also no need, so was this
the intention? Just to be clear - it is then ID or binding constant, not
a value for hardware register.

Best regards,
Krzysztof

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

[Maarten Zanders]

On 2/2/23 14:43, Krzysztof Kozlowski wrote:
>
> Strings in DTS are usually easier to for humans to read, but it's not a
> requirement to use them. The problem of storing register values is that
> binding is tied/coupled with hardware programming model, so you cannot
> add a new device if the register value is a bit different (e.g.
> LP55XX_CP_OFF is 0x1). You need entire new binding for such case. With
> string - no need.
I understand and this is why I started with the string in the first 
place (as suggested by yourself in V1).
> With binding constants (IDs) also no need, so was this
> the intention? Just to be clear - it is then ID or binding constant, not
> a value for hardware register.
>
For simplicity sake, yes, now the setting is propagating directly into 
the register as a bit value. But this is how the current implementation 
of the drivers work. If we add a device in the future which indeed has 
different bit mappings, that driver will have to do a mapping of the DT 
binding to its own bit field definitions. I consider this DT binding as 
the "master", which is now conveniently chosen to match the register values.

Cheers,

Maarten

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

[Krzysztof Kozlowski]

On 02/02/2023 15:12, Maarten Zanders wrote:
> 
> On 2/2/23 14:43, Krzysztof Kozlowski wrote:
>>
>> Strings in DTS are usually easier to for humans to read, but it's not a
>> requirement to use them. The problem of storing register values is that
>> binding is tied/coupled with hardware programming model, so you cannot
>> add a new device if the register value is a bit different (e.g.
>> LP55XX_CP_OFF is 0x1). You need entire new binding for such case. With
>> string - no need.
> I understand and this is why I started with the string in the first 
> place (as suggested by yourself in V1).
>> With binding constants (IDs) also no need, so was this
>> the intention? Just to be clear - it is then ID or binding constant, not
>> a value for hardware register.
>>
> For simplicity sake, yes, now the setting is propagating directly into 
> the register as a bit value. But this is how the current implementation 
> of the drivers work. If we add a device in the future which indeed has 
> different bit mappings, that driver will have to do a mapping of the DT 
> binding to its own bit field definitions. I consider this DT binding as 
> the "master", which is now conveniently chosen to match the register values.

OK, that makes sense.

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 02/02/2023 11:10, Maarten Zanders wrote:
> Add a binding to configure the internal charge pump for lp55xx.
> 
> Signed-off-by: Maarten Zanders <maarten.zanders@mind.be>
> ---
> 
> Notes:
>     v1: implement as bool to disable charge pump
>     v2: rewrite to use string configuration, supporting all modes
>     v3: simplification by replacing string option by u8 constant,
>         removing previous Reviewed-by tags as it's a complete
>         rewrite of the patch.
>     v4: added notes
> 
>  .../devicetree/bindings/leds/leds-lp55xx.yaml          |  8 ++++++++
>  include/dt-bindings/leds/leds-lp55xx.h                 | 10 ++++++++++
>  2 files changed, 18 insertions(+)
>  create mode 100644 include/dt-bindings/leds/leds-lp55xx.h
> 
> diff --git a/Documentation/devicetree/bindings/leds/leds-lp55xx.yaml b/Documentation/devicetree/bindings/leds/leds-lp55xx.yaml
> index ae607911f1db..22e63d89d770 100644
> --- a/Documentation/devicetree/bindings/leds/leds-lp55xx.yaml
> +++ b/Documentation/devicetree/bindings/leds/leds-lp55xx.yaml
> @@ -66,6 +66,12 @@ properties:
>    '#size-cells':
>      const: 0
>  
> +  ti,charge-pump-mode:
> +    description:
> +      Set the operating mode of the internal charge pump as defined in
> +      <dt-bindings/leds/leds-lp55xx.h>. Defaults to auto.
> +    $ref: /schemas/types.yaml#/definitions/uint8

This should be then uint32

default: 3
(and drop last sentence about default)

> +
>  patternProperties:
>    '^multi-led@[0-8]$':
>      type: object
> @@ -152,6 +158,7 @@ additionalProperties: false
>  examples:
>    - |
>      #include <dt-bindings/leds/common.h>
> +    #include <dt-bindings/leds/leds-lp55xx.h>
>  
>      i2c {
>          #address-cells = <1>;
> @@ -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 = /bits/ 8 <LP55XX_CP_BYPASS>;
>  
>              led@0 {
>                  reg = <0>;
> diff --git a/include/dt-bindings/leds/leds-lp55xx.h b/include/dt-bindings/leds/leds-lp55xx.h
> new file mode 100644
> index 000000000000..8f59c1c12dee
> --- /dev/null
> +++ b/include/dt-bindings/leds/leds-lp55xx.h
> @@ -0,0 +1,10 @@
> +/* SPDX-License-Identifier: GPL-2.0 */

Dual license.

> +#ifndef _DT_BINDINGS_LEDS_LP55XX_H
> +#define _DT_BINDINGS_LEDS_LP55XX_H
> +
> +#define LP55XX_CP_OFF		0
> +#define LP55XX_CP_BYPASS	1
> +#define LP55XX_CP_BOOST		2
> +#define LP55XX_CP_AUTO		3
> +
> +#endif /* _DT_BINDINGS_LEDS_LP55XX_H */

Best regards,
Krzysztof

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

[Maarten Zanders]

On 2/2/23 21:13, Krzysztof Kozlowski wrote:
> + ti,charge-pump-mode:
>> +    description:
>> +      Set the operating mode of the internal charge pump as defined in
>> +      <dt-bindings/leds/leds-lp55xx.h>. Defaults to auto.
>> +    $ref: /schemas/types.yaml#/definitions/uint8
> This should be then uint32

Why is that? I specifically chose uint8 because other settings for LED 
are also uint8. The implementation is also uint8. I surely hope we'll 
never get to >256 modes for a charge pump.


> default: 3
> (and drop last sentence about default)
OK
> +/* SPDX-License-Identifier: GPL-2.0 */
> Dual license.

OK

Best regards,
Maarten

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

[Krzysztof Kozlowski]

On 03/02/2023 16:38, Maarten Zanders wrote:
> 
> On 2/2/23 21:13, Krzysztof Kozlowski wrote:
>> + ti,charge-pump-mode:
>>> +    description:
>>> +      Set the operating mode of the internal charge pump as defined in
>>> +      <dt-bindings/leds/leds-lp55xx.h>. Defaults to auto.
>>> +    $ref: /schemas/types.yaml#/definitions/uint8
>> This should be then uint32
> 
> Why is that? I specifically chose uint8 because other settings for LED 
> are also uint8. The implementation is also uint8. I surely hope we'll 
> never get to >256 modes for a charge pump.

Because all IDs are unsigned int.

Best regards,
Krzysztof