[PATCH 0/4] video: backlight: lp855x: modernize bindings

[PATCH 0/4] video: backlight: lp855x: modernize bindings

[Artur Weber]

Convert TI LP855X backlight controller bindings from TXT to YAML and,
while we're at it, rework some of the code related to PWM handling.
Also correct existing DTS files to avoid introducing new dtb_check
errors.

Signed-off-by: Artur Weber <aweber.kernel@gmail.com>

Artur Weber (4):
  dt-bindings: backlight: lp855x: convert to YAML and modernize
  video: backlight: lp855x: get PWM for PWM mode during probe
  ARM: dts: adapt to LP855X bindings changes
  arm64: dts: adapt to LP855X bindings changes

 .../leds/backlight/lp855x-backlight.yaml      | 148 ++++++++++++++++++
 .../bindings/leds/backlight/lp855x.txt        |  72 ---------
 .../dts/qcom-apq8026-samsung-matisse-wifi.dts |   1 -
 ...-msm8974pro-sony-xperia-shinano-castor.dts |  23 +--
 .../boot/dts/nvidia/tegra210-p2371-2180.dts   |   6 +-
 drivers/video/backlight/lp855x_bl.c           |  48 +++---
 6 files changed, 188 insertions(+), 110 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/leds/backlight/lp855x-backlight.yaml
 delete mode 100644 Documentation/devicetree/bindings/leds/backlight/lp855x.txt


base-commit: e154a338e16cc3b3bbd54c891253319d22383746
-- 
2.40.1

[PATCH 1/4] dt-bindings: backlight: lp855x: convert to YAML and modernize

[Artur Weber]

Notable changes:
- ROM child nodes use dashes instead of underscores; the driver
  reads all child nodes regardless of their names, so this doesn't
  break ABI.
- pwm-period argument is deprecated, as it effectively duplicates
  the period value provided in pwms. The driver continues to accept
  the property, so this should not break ABI.

Signed-off-by: Artur Weber <aweber.kernel@gmail.com>
---
 .../leds/backlight/lp855x-backlight.yaml      | 148 ++++++++++++++++++
 .../bindings/leds/backlight/lp855x.txt        |  72 ---------
 2 files changed, 148 insertions(+), 72 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/leds/backlight/lp855x-backlight.yaml
 delete mode 100644 Documentation/devicetree/bindings/leds/backlight/lp855x.txt

diff --git a/Documentation/devicetree/bindings/leds/backlight/lp855x-backlight.yaml b/Documentation/devicetree/bindings/leds/backlight/lp855x-backlight.yaml
new file mode 100644
index 000000000000..dfe8131d2a32
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/backlight/lp855x-backlight.yaml
@@ -0,0 +1,148 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/leds/backlight/lp855x-backlight.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Texas Instruments LP855X backlight controllers
+
+maintainers:
+  - Artur Weber <aweber.kernel@gmail.com>
+
+properties:
+  compatible:
+    enum:
+      - ti,lp8550
+      - ti,lp8551
+      - ti,lp8552
+      - ti,lp8553
+      - ti,lp8555
+      - ti,lp8556
+      - ti,lp8557
+
+  reg:
+    maxItems: 1
+
+  dev-ctrl:
+    $ref: /schemas/types.yaml#/definitions/uint8
+    description:
+      Value of device control register. This is a device-specific value.
+
+  bl-name:
+    $ref: /schemas/types.yaml#/definitions/string
+    description: Backlight device name.
+
+  init-brt:
+    $ref: /schemas/types.yaml#/definitions/uint8
+    description: Initial value of backlight brightness.
+
+  power-supply:
+    description: Regulator which controls the 3V rail.
+
+  enable-supply:
+    description: Regulator which controls the EN/VDDIO input.
+
+  pwms:
+    maxItems: 1
+    description: |
+      PWM channel to use for controlling the backlight; setting this
+      enables the PWM-based backlight control mode.
+
+  pwm-names: true
+
+  pwm-period:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      PWM period value. Deprecated; set the period value in the pwms
+      property instead.
+    deprecated: true
+
+patternProperties:
+  "^rom-[0-9a-f]{2}h$":
+    type: object
+    description: Nodes containing the values of configuration registers.
+    properties:
+      rom-addr:
+        $ref: /schemas/types.yaml#/definitions/uint8
+        description: Register address of ROM area to be updated.
+
+      rom-val:
+        $ref: /schemas/types.yaml#/definitions/uint8
+        description: Value to write to the ROM register.
+
+required:
+  - compatible
+  - reg
+  - dev-ctrl
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        backlight@2c {
+            compatible = "ti,lp8555";
+            reg = <0x2c>;
+
+            dev-ctrl = /bits/ 8 <0x00>;
+
+            pwms = <&pwm 0 10000>;
+            pwm-names = "lp8555";
+
+            /* 4V OV, 4 output LED0 string enabled */
+            rom-14h {
+              rom-addr = /bits/ 8 <0x14>;
+              rom-val = /bits/ 8 <0xcf>;
+            };
+
+            /* Heavy smoothing, 24ms ramp time step */
+            rom-15h {
+              rom-addr = /bits/ 8 <0x15>;
+              rom-val = /bits/ 8 <0xc7>;
+            };
+
+            /* 4 output LED1 string enabled */
+            rom-19h {
+              rom-addr = /bits/ 8 <0x19>;
+              rom-val = /bits/ 8 <0x0f>;
+            };
+        };
+    };
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        backlight@2c {
+            compatible = "ti,lp8556";
+            reg = <0x2c>;
+
+            bl-name = "lcd-bl";
+            dev-ctrl = /bits/ 8 <0x85>;
+            init-brt = /bits/ 8 <0x10>;
+        };
+      };
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        backlight@2c {
+            compatible = "ti,lp8557";
+            reg = <0x2c>;
+            enable-supply = <&backlight_vddio>;
+            power-supply = <&backlight_vdd>;
+
+            dev-ctrl = /bits/ 8 <0x41>;
+            init-brt = /bits/ 8 <0x0a>;
+
+            /* 4V OV, 4 output LED string enabled */
+            rom-14h {
+              rom-addr = /bits/ 8 <0x14>;
+              rom-val = /bits/ 8 <0xcf>;
+            };
+        };
+    };
diff --git a/Documentation/devicetree/bindings/leds/backlight/lp855x.txt b/Documentation/devicetree/bindings/leds/backlight/lp855x.txt
deleted file mode 100644
index 88f56641fc28..000000000000
--- a/Documentation/devicetree/bindings/leds/backlight/lp855x.txt
+++ /dev/null
@@ -1,72 +0,0 @@
-lp855x bindings
-
-Required properties:
-  - compatible: "ti,lp8550", "ti,lp8551", "ti,lp8552", "ti,lp8553",
-                "ti,lp8555", "ti,lp8556", "ti,lp8557"
-  - reg: I2C slave address (u8)
-  - dev-ctrl: Value of DEVICE CONTROL register (u8). It depends on the device.
-
-Optional properties:
-  - bl-name: Backlight device name (string)
-  - init-brt: Initial value of backlight brightness (u8)
-  - pwm-period: PWM period value. Set only PWM input mode used (u32)
-  - rom-addr: Register address of ROM area to be updated (u8)
-  - rom-val: Register value to be updated (u8)
-  - power-supply: Regulator which controls the 3V rail
-  - enable-supply: Regulator which controls the EN/VDDIO input
-
-Example:
-
-	/* LP8555 */
-	backlight@2c {
-		compatible = "ti,lp8555";
-		reg = <0x2c>;
-
-		dev-ctrl = /bits/ 8 <0x00>;
-		pwm-period = <10000>;
-
-		/* 4V OV, 4 output LED0 string enabled */
-		rom_14h {
-			rom-addr = /bits/ 8 <0x14>;
-			rom-val = /bits/ 8 <0xcf>;
-		};
-
-		/* Heavy smoothing, 24ms ramp time step */
-		rom_15h {
-			rom-addr = /bits/ 8 <0x15>;
-			rom-val = /bits/ 8 <0xc7>;
-		};
-
-		/* 4 output LED1 string enabled */
-		rom_19h {
-			rom-addr = /bits/ 8 <0x19>;
-			rom-val = /bits/ 8 <0x0f>;
-		};
-	};
-
-	/* LP8556 */
-	backlight@2c {
-		compatible = "ti,lp8556";
-		reg = <0x2c>;
-
-		bl-name = "lcd-bl";
-		dev-ctrl = /bits/ 8 <0x85>;
-		init-brt = /bits/ 8 <0x10>;
-	};
-
-	/* LP8557 */
-	backlight@2c {
-		compatible = "ti,lp8557";
-		reg = <0x2c>;
-		enable-supply = <&backlight_vddio>;
-		power-supply = <&backlight_vdd>;
-
-		dev-ctrl = /bits/ 8 <0x41>;
-		init-brt = /bits/ 8 <0x0a>;
-
-		/* 4V OV, 4 output LED string enabled */
-		rom_14h {
-			rom-addr = /bits/ 8 <0x14>;
-			rom-val = /bits/ 8 <0xcf>;
-		};
-	};
-- 
2.40.1

[PATCH 2/4] video: backlight: lp855x: get PWM for PWM mode during probe

[Artur Weber]

Also deprecate the pwm-period DT property, as it is now redundant
(pwms property already contains period value).

Signed-off-by: Artur Weber <aweber.kernel@gmail.com>
---
 drivers/video/backlight/lp855x_bl.c | 48 ++++++++++++++++-------------
 1 file changed, 26 insertions(+), 22 deletions(-)

diff --git a/drivers/video/backlight/lp855x_bl.c b/drivers/video/backlight/lp855x_bl.c
index 81012bf29baf..21eb4943ed56 100644
--- a/drivers/video/backlight/lp855x_bl.c
+++ b/drivers/video/backlight/lp855x_bl.c
@@ -218,23 +218,10 @@ static int lp855x_configure(struct lp855x *lp)
 
 static void lp855x_pwm_ctrl(struct lp855x *lp, int br, int max_br)
 {
-	struct pwm_device *pwm;
 	struct pwm_state state;
 
-	/* request pwm device with the consumer name */
-	if (!lp->pwm) {
-		pwm = devm_pwm_get(lp->dev, lp->chipname);
-		if (IS_ERR(pwm))
-			return;
-
-		lp->pwm = pwm;
-
-		pwm_init_state(lp->pwm, &state);
-	} else {
-		pwm_get_state(lp->pwm, &state);
-	}
+	pwm_get_state(lp->pwm, &state);
 
-	state.period = lp->pdata->period_ns;
 	state.duty_cycle = div_u64(br * state.period, max_br);
 	state.enabled = state.duty_cycle;
 
@@ -339,6 +326,7 @@ static int lp855x_parse_dt(struct lp855x *lp)
 	of_property_read_string(node, "bl-name", &pdata->name);
 	of_property_read_u8(node, "dev-ctrl", &pdata->device_control);
 	of_property_read_u8(node, "init-brt", &pdata->initial_brightness);
+	/* Deprecated, specify period in pwms property instead */
 	of_property_read_u32(node, "pwm-period", &pdata->period_ns);
 
 	/* Fill ROM platform data if defined */
@@ -399,6 +387,7 @@ static int lp855x_probe(struct i2c_client *cl)
 	const struct i2c_device_id *id = i2c_client_get_device_id(cl);
 	const struct acpi_device_id *acpi_id = NULL;
 	struct device *dev = &cl->dev;
+	struct pwm_state pwmstate;
 	struct lp855x *lp;
 	int ret;
 
@@ -457,11 +446,6 @@ static int lp855x_probe(struct i2c_client *cl)
 		}
 	}
 
-	if (lp->pdata->period_ns > 0)
-		lp->mode = PWM_BASED;
-	else
-		lp->mode = REGISTER_BASED;
-
 	lp->supply = devm_regulator_get(dev, "power");
 	if (IS_ERR(lp->supply)) {
 		if (PTR_ERR(lp->supply) == -EPROBE_DEFER)
@@ -472,11 +456,31 @@ static int lp855x_probe(struct i2c_client *cl)
 	lp->enable = devm_regulator_get_optional(dev, "enable");
 	if (IS_ERR(lp->enable)) {
 		ret = PTR_ERR(lp->enable);
-		if (ret == -ENODEV) {
+		if (ret == -ENODEV)
 			lp->enable = NULL;
-		} else {
+		else
 			return dev_err_probe(dev, ret, "getting enable regulator\n");
-		}
+	}
+
+	lp->pwm = devm_pwm_get(lp->dev, lp->chipname);
+	if (IS_ERR(lp->pwm)) {
+		ret = PTR_ERR(lp->pwm);
+		if (ret == -ENODEV || ret == -EINVAL)
+			lp->pwm = NULL;
+		else
+			return dev_err_probe(dev, ret, "getting PWM\n");
+
+		lp->mode = REGISTER_BASED;
+		dev_dbg(dev, "mode: register based\n");
+	} else {
+		pwm_init_state(lp->pwm, &pwmstate);
+		/* Legacy platform data compatibility */
+		if (lp->pdata->period_ns > 0)
+			pwmstate.period = lp->pdata->period_ns;
+		pwm_apply_state(lp->pwm, &pwmstate);
+
+		lp->mode = PWM_BASED;
+		dev_dbg(dev, "mode: PWM based\n");
 	}
 
 	if (lp->supply) {
-- 
2.40.1

[PATCH 3/4] ARM: dts: adapt to LP855X bindings changes

[Artur Weber]

Change underscores in ROM node names to dashes, and remove deprecated
pwm-period property.

Signed-off-by: Artur Weber <aweber.kernel@gmail.com>
---
 .../dts/qcom-apq8026-samsung-matisse-wifi.dts |  1 -
 ...-msm8974pro-sony-xperia-shinano-castor.dts | 23 ++++++++++---------
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/arch/arm/boot/dts/qcom-apq8026-samsung-matisse-wifi.dts b/arch/arm/boot/dts/qcom-apq8026-samsung-matisse-wifi.dts
index 91b860e24681..884d99297d4c 100644
--- a/arch/arm/boot/dts/qcom-apq8026-samsung-matisse-wifi.dts
+++ b/arch/arm/boot/dts/qcom-apq8026-samsung-matisse-wifi.dts
@@ -99,7 +99,6 @@ backlight@2c {
 
 			dev-ctrl = /bits/ 8 <0x80>;
 			init-brt = /bits/ 8 <0x3f>;
-			pwm-period = <100000>;
 
 			pwms = <&backlight_pwm 0 100000>;
 			pwm-names = "lp8556";
diff --git a/arch/arm/boot/dts/qcom-msm8974pro-sony-xperia-shinano-castor.dts b/arch/arm/boot/dts/qcom-msm8974pro-sony-xperia-shinano-castor.dts
index 04bc58d87abf..2396253f953a 100644
--- a/arch/arm/boot/dts/qcom-msm8974pro-sony-xperia-shinano-castor.dts
+++ b/arch/arm/boot/dts/qcom-msm8974pro-sony-xperia-shinano-castor.dts
@@ -150,47 +150,48 @@ lp8566_wled: backlight@2c {
 		bl-name = "backlight";
 		dev-ctrl = /bits/ 8 <0x05>;
 		init-brt = /bits/ 8 <0x3f>;
-		rom_a0h {
+
+		rom-a0h {
 			rom-addr = /bits/ 8 <0xa0>;
 			rom-val = /bits/ 8 <0xff>;
 		};
-		rom_a1h {
+		rom-a1h {
 			rom-addr = /bits/ 8 <0xa1>;
 			rom-val = /bits/ 8 <0x3f>;
 		};
-		rom_a2h {
+		rom-a2h {
 			rom-addr = /bits/ 8 <0xa2>;
 			rom-val = /bits/ 8 <0x20>;
 		};
-		rom_a3h {
+		rom-a3h {
 			rom-addr = /bits/ 8 <0xa3>;
 			rom-val = /bits/ 8 <0x5e>;
 		};
-		rom_a4h {
+		rom-a4h {
 			rom-addr = /bits/ 8 <0xa4>;
 			rom-val = /bits/ 8 <0x02>;
 		};
-		rom_a5h {
+		rom-a5h {
 			rom-addr = /bits/ 8 <0xa5>;
 			rom-val = /bits/ 8 <0x04>;
 		};
-		rom_a6h {
+		rom-a6h {
 			rom-addr = /bits/ 8 <0xa6>;
 			rom-val = /bits/ 8 <0x80>;
 		};
-		rom_a7h {
+		rom-a7h {
 			rom-addr = /bits/ 8 <0xa7>;
 			rom-val = /bits/ 8 <0xf7>;
 		};
-		rom_a9h {
+		rom-a9h {
 			rom-addr = /bits/ 8 <0xa9>;
 			rom-val = /bits/ 8 <0x80>;
 		};
-		rom_aah {
+		rom-aah {
 			rom-addr = /bits/ 8 <0xaa>;
 			rom-val = /bits/ 8 <0x0f>;
 		};
-		rom_aeh {
+		rom-aeh {
 			rom-addr = /bits/ 8 <0xae>;
 			rom-val = /bits/ 8 <0x0f>;
 		};
-- 
2.40.1

[PATCH 4/4] arm64: dts: adapt to LP855X bindings changes

[Artur Weber]

Change underscores in ROM node names to dashes, and remove deprecated
pwm-period property.

Signed-off-by: Artur Weber <aweber.kernel@gmail.com>
---
 arch/arm64/boot/dts/nvidia/tegra210-p2371-2180.dts | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/boot/dts/nvidia/tegra210-p2371-2180.dts b/arch/arm64/boot/dts/nvidia/tegra210-p2371-2180.dts
index 38f4ff229bef..a6a58e51822d 100644
--- a/arch/arm64/boot/dts/nvidia/tegra210-p2371-2180.dts
+++ b/arch/arm64/boot/dts/nvidia/tegra210-p2371-2180.dts
@@ -58,19 +58,17 @@ backlight: backlight@2c {
 			dev-ctrl = /bits/ 8 <0x80>;
 			init-brt = /bits/ 8 <0xff>;
 
-			pwm-period = <29334>;
-
 			pwms = <&pwm 0 29334>;
 			pwm-names = "lp8557";
 
 			/* boost frequency 1 MHz */
-			rom_13h {
+			rom-13h {
 				rom-addr = /bits/ 8 <0x13>;
 				rom-val = /bits/ 8 <0x01>;
 			};
 
 			/* 3 LED string */
-			rom_14h {
+			rom-14h {
 				rom-addr = /bits/ 8 <0x14>;
 				rom-val = /bits/ 8 <0x87>;
 			};
-- 
2.40.1

Re: [PATCH 3/4] ARM: dts: adapt to LP855X bindings changes

[Luca Weiss]

On Samstag, 29. April 2023 12:45:33 CEST Artur Weber wrote:
> Change underscores in ROM node names to dashes, and remove deprecated
> pwm-period property.
> 
> Signed-off-by: Artur Weber <aweber.kernel@gmail.com>

Reviewed-by: Luca Weiss <luca@z3ntu.xyz>

> ---
>  .../dts/qcom-apq8026-samsung-matisse-wifi.dts |  1 -
>  ...-msm8974pro-sony-xperia-shinano-castor.dts | 23 ++++++++++---------
>  2 files changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/qcom-apq8026-samsung-matisse-wifi.dts
> b/arch/arm/boot/dts/qcom-apq8026-samsung-matisse-wifi.dts index
> 91b860e24681..884d99297d4c 100644
> --- a/arch/arm/boot/dts/qcom-apq8026-samsung-matisse-wifi.dts
> +++ b/arch/arm/boot/dts/qcom-apq8026-samsung-matisse-wifi.dts
> @@ -99,7 +99,6 @@ backlight@2c {
> 
>  			dev-ctrl = /bits/ 8 <0x80>;
>  			init-brt = /bits/ 8 <0x3f>;
> -			pwm-period = <100000>;
> 
>  			pwms = <&backlight_pwm 0 100000>;
>  			pwm-names = "lp8556";
> diff --git
> a/arch/arm/boot/dts/qcom-msm8974pro-sony-xperia-shinano-castor.dts
> b/arch/arm/boot/dts/qcom-msm8974pro-sony-xperia-shinano-castor.dts index
> 04bc58d87abf..2396253f953a 100644
> --- a/arch/arm/boot/dts/qcom-msm8974pro-sony-xperia-shinano-castor.dts
> +++ b/arch/arm/boot/dts/qcom-msm8974pro-sony-xperia-shinano-castor.dts
> @@ -150,47 +150,48 @@ lp8566_wled: backlight@2c {
>  		bl-name = "backlight";
>  		dev-ctrl = /bits/ 8 <0x05>;
>  		init-brt = /bits/ 8 <0x3f>;
> -		rom_a0h {
> +
> +		rom-a0h {
>  			rom-addr = /bits/ 8 <0xa0>;
>  			rom-val = /bits/ 8 <0xff>;
>  		};
> -		rom_a1h {
> +		rom-a1h {
>  			rom-addr = /bits/ 8 <0xa1>;
>  			rom-val = /bits/ 8 <0x3f>;
>  		};
> -		rom_a2h {
> +		rom-a2h {
>  			rom-addr = /bits/ 8 <0xa2>;
>  			rom-val = /bits/ 8 <0x20>;
>  		};
> -		rom_a3h {
> +		rom-a3h {
>  			rom-addr = /bits/ 8 <0xa3>;
>  			rom-val = /bits/ 8 <0x5e>;
>  		};
> -		rom_a4h {
> +		rom-a4h {
>  			rom-addr = /bits/ 8 <0xa4>;
>  			rom-val = /bits/ 8 <0x02>;
>  		};
> -		rom_a5h {
> +		rom-a5h {
>  			rom-addr = /bits/ 8 <0xa5>;
>  			rom-val = /bits/ 8 <0x04>;
>  		};
> -		rom_a6h {
> +		rom-a6h {
>  			rom-addr = /bits/ 8 <0xa6>;
>  			rom-val = /bits/ 8 <0x80>;
>  		};
> -		rom_a7h {
> +		rom-a7h {
>  			rom-addr = /bits/ 8 <0xa7>;
>  			rom-val = /bits/ 8 <0xf7>;
>  		};
> -		rom_a9h {
> +		rom-a9h {
>  			rom-addr = /bits/ 8 <0xa9>;
>  			rom-val = /bits/ 8 <0x80>;
>  		};
> -		rom_aah {
> +		rom-aah {
>  			rom-addr = /bits/ 8 <0xaa>;
>  			rom-val = /bits/ 8 <0x0f>;
>  		};
> -		rom_aeh {
> +		rom-aeh {
>  			rom-addr = /bits/ 8 <0xae>;
>  			rom-val = /bits/ 8 <0x0f>;
>  		};

Re: [PATCH 1/4] dt-bindings: backlight: lp855x: convert to YAML and modernize

[Rob Herring]

On Sat, Apr 29, 2023 at 12:45:31PM +0200, Artur Weber wrote:
> Notable changes:
> - ROM child nodes use dashes instead of underscores; the driver
>   reads all child nodes regardless of their names, so this doesn't
>   break ABI.
> - pwm-period argument is deprecated, as it effectively duplicates
>   the period value provided in pwms. The driver continues to accept
>   the property, so this should not break ABI.
> 
> Signed-off-by: Artur Weber <aweber.kernel@gmail.com>
> ---
>  .../leds/backlight/lp855x-backlight.yaml      | 148 ++++++++++++++++++
>  .../bindings/leds/backlight/lp855x.txt        |  72 ---------
>  2 files changed, 148 insertions(+), 72 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/leds/backlight/lp855x-backlight.yaml
>  delete mode 100644 Documentation/devicetree/bindings/leds/backlight/lp855x.txt
> 
> diff --git a/Documentation/devicetree/bindings/leds/backlight/lp855x-backlight.yaml b/Documentation/devicetree/bindings/leds/backlight/lp855x-backlight.yaml
> new file mode 100644
> index 000000000000..dfe8131d2a32
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/backlight/lp855x-backlight.yaml
> @@ -0,0 +1,148 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/leds/backlight/lp855x-backlight.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Texas Instruments LP855X backlight controllers
> +
> +maintainers:
> +  - Artur Weber <aweber.kernel@gmail.com>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - ti,lp8550
> +      - ti,lp8551
> +      - ti,lp8552
> +      - ti,lp8553
> +      - ti,lp8555
> +      - ti,lp8556
> +      - ti,lp8557
> +
> +  reg:
> +    maxItems: 1
> +
> +  dev-ctrl:
> +    $ref: /schemas/types.yaml#/definitions/uint8
> +    description:
> +      Value of device control register. This is a device-specific value.
> +
> +  bl-name:
> +    $ref: /schemas/types.yaml#/definitions/string
> +    description: Backlight device name.
> +
> +  init-brt:
> +    $ref: /schemas/types.yaml#/definitions/uint8
> +    description: Initial value of backlight brightness.
> +
> +  power-supply:
> +    description: Regulator which controls the 3V rail.
> +
> +  enable-supply:
> +    description: Regulator which controls the EN/VDDIO input.
> +
> +  pwms:
> +    maxItems: 1
> +    description: |
> +      PWM channel to use for controlling the backlight; setting this
> +      enables the PWM-based backlight control mode.
> +
> +  pwm-names: true
> +
> +  pwm-period:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      PWM period value. Deprecated; set the period value in the pwms
> +      property instead.
> +    deprecated: true
> +
> +patternProperties:
> +  "^rom-[0-9a-f]{2}h$":
> +    type: object
> +    description: Nodes containing the values of configuration registers.

       additionalProperties: false

With that,

Reviewed-by: Rob Herring <robh@kernel.org>

> +    properties:
> +      rom-addr:
> +        $ref: /schemas/types.yaml#/definitions/uint8
> +        description: Register address of ROM area to be updated.
> +
> +      rom-val:
> +        $ref: /schemas/types.yaml#/definitions/uint8
> +        description: Value to write to the ROM register.
> +
> +required:
> +  - compatible
> +  - reg
> +  - dev-ctrl
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        backlight@2c {
> +            compatible = "ti,lp8555";
> +            reg = <0x2c>;
> +
> +            dev-ctrl = /bits/ 8 <0x00>;
> +
> +            pwms = <&pwm 0 10000>;
> +            pwm-names = "lp8555";
> +
> +            /* 4V OV, 4 output LED0 string enabled */
> +            rom-14h {
> +              rom-addr = /bits/ 8 <0x14>;
> +              rom-val = /bits/ 8 <0xcf>;
> +            };
> +
> +            /* Heavy smoothing, 24ms ramp time step */
> +            rom-15h {
> +              rom-addr = /bits/ 8 <0x15>;
> +              rom-val = /bits/ 8 <0xc7>;
> +            };
> +
> +            /* 4 output LED1 string enabled */
> +            rom-19h {
> +              rom-addr = /bits/ 8 <0x19>;
> +              rom-val = /bits/ 8 <0x0f>;
> +            };
> +        };
> +    };
> +  - |
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        backlight@2c {
> +            compatible = "ti,lp8556";
> +            reg = <0x2c>;
> +
> +            bl-name = "lcd-bl";
> +            dev-ctrl = /bits/ 8 <0x85>;
> +            init-brt = /bits/ 8 <0x10>;
> +        };
> +      };
> +  - |
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        backlight@2c {
> +            compatible = "ti,lp8557";
> +            reg = <0x2c>;
> +            enable-supply = <&backlight_vddio>;
> +            power-supply = <&backlight_vdd>;
> +
> +            dev-ctrl = /bits/ 8 <0x41>;
> +            init-brt = /bits/ 8 <0x0a>;
> +
> +            /* 4V OV, 4 output LED string enabled */
> +            rom-14h {
> +              rom-addr = /bits/ 8 <0x14>;
> +              rom-val = /bits/ 8 <0xcf>;
> +            };
> +        };
> +    };

Re: [PATCH 2/4] video: backlight: lp855x: get PWM for PWM mode during probe

[Uwe Kleine-König]

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

On Sat, Apr 29, 2023 at 12:45:32PM +0200, Artur Weber wrote:
> Also deprecate the pwm-period DT property, as it is now redundant
> (pwms property already contains period value).
> 
> Signed-off-by: Artur Weber <aweber.kernel@gmail.com>
> ---
>  drivers/video/backlight/lp855x_bl.c | 48 ++++++++++++++++-------------
>  1 file changed, 26 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/video/backlight/lp855x_bl.c b/drivers/video/backlight/lp855x_bl.c
> index 81012bf29baf..21eb4943ed56 100644
> --- a/drivers/video/backlight/lp855x_bl.c
> +++ b/drivers/video/backlight/lp855x_bl.c
> @@ -218,23 +218,10 @@ static int lp855x_configure(struct lp855x *lp)
>  
>  static void lp855x_pwm_ctrl(struct lp855x *lp, int br, int max_br)
>  {
> -	struct pwm_device *pwm;
>  	struct pwm_state state;
>  
> -	/* request pwm device with the consumer name */
> -	if (!lp->pwm) {
> -		pwm = devm_pwm_get(lp->dev, lp->chipname);
> -		if (IS_ERR(pwm))
> -			return;
> -
> -		lp->pwm = pwm;
> -
> -		pwm_init_state(lp->pwm, &state);
> -	} else {
> -		pwm_get_state(lp->pwm, &state);
> -	}
> +	pwm_get_state(lp->pwm, &state);

pwm_get_state returns an error code. Do you care if it fails? (You
probably should.)
>  
> -	state.period = lp->pdata->period_ns;
>  	state.duty_cycle = div_u64(br * state.period, max_br);
>  	state.enabled = state.duty_cycle;
>  
> @@ -339,6 +326,7 @@ static int lp855x_parse_dt(struct lp855x *lp)
>  	of_property_read_string(node, "bl-name", &pdata->name);
>  	of_property_read_u8(node, "dev-ctrl", &pdata->device_control);
>  	of_property_read_u8(node, "init-brt", &pdata->initial_brightness);
> +	/* Deprecated, specify period in pwms property instead */
>  	of_property_read_u32(node, "pwm-period", &pdata->period_ns);
>  
>  	/* Fill ROM platform data if defined */
> @@ -399,6 +387,7 @@ static int lp855x_probe(struct i2c_client *cl)
>  	const struct i2c_device_id *id = i2c_client_get_device_id(cl);
>  	const struct acpi_device_id *acpi_id = NULL;
>  	struct device *dev = &cl->dev;
> +	struct pwm_state pwmstate;
>  	struct lp855x *lp;
>  	int ret;
>  
> @@ -457,11 +446,6 @@ static int lp855x_probe(struct i2c_client *cl)
>  		}
>  	}
>  
> -	if (lp->pdata->period_ns > 0)
> -		lp->mode = PWM_BASED;
> -	else
> -		lp->mode = REGISTER_BASED;
> -
>  	lp->supply = devm_regulator_get(dev, "power");
>  	if (IS_ERR(lp->supply)) {
>  		if (PTR_ERR(lp->supply) == -EPROBE_DEFER)
> @@ -472,11 +456,31 @@ static int lp855x_probe(struct i2c_client *cl)
>  	lp->enable = devm_regulator_get_optional(dev, "enable");
>  	if (IS_ERR(lp->enable)) {
>  		ret = PTR_ERR(lp->enable);
> -		if (ret == -ENODEV) {
> +		if (ret == -ENODEV)
>  			lp->enable = NULL;
> -		} else {
> +		else
>  			return dev_err_probe(dev, ret, "getting enable regulator\n");
> -		}
> +	}
> +
> +	lp->pwm = devm_pwm_get(lp->dev, lp->chipname);
> +	if (IS_ERR(lp->pwm)) {
> +		ret = PTR_ERR(lp->pwm);
> +		if (ret == -ENODEV || ret == -EINVAL)

Why would you ignore EINVAL?

> +			lp->pwm = NULL;
> +		else
> +			return dev_err_probe(dev, ret, "getting PWM\n");
> +
> +		lp->mode = REGISTER_BASED;
> +		dev_dbg(dev, "mode: register based\n");
> +	} else {

pwmstate could be declared here.

> +		pwm_init_state(lp->pwm, &pwmstate);
> +		/* Legacy platform data compatibility */
> +		if (lp->pdata->period_ns > 0)
> +			pwmstate.period = lp->pdata->period_ns;
> +		pwm_apply_state(lp->pwm, &pwmstate);

This is a change in behaviour. Before lp855x_probe() didn't modify the
state the bootloader left the backlight in. Now you're disabling it (I
think). Is this intended?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/4] video: backlight: lp855x: get PWM for PWM mode during probe

[Artur Weber]

On 14/06/2023 10:39, Uwe Kleine-König wrote:
> On Sat, Apr 29, 2023 at 12:45:32PM +0200, Artur Weber wrote:
>> Also deprecate the pwm-period DT property, as it is now redundant
>> (pwms property already contains period value).
>>
>> Signed-off-by: Artur Weber <aweber.kernel@gmail.com>
>> ---
>>  drivers/video/backlight/lp855x_bl.c | 48 ++++++++++++++++-------------
>>  1 file changed, 26 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/video/backlight/lp855x_bl.c b/drivers/video/backlight/lp855x_bl.c
>> index 81012bf29baf..21eb4943ed56 100644
>> --- a/drivers/video/backlight/lp855x_bl.c
>> +++ b/drivers/video/backlight/lp855x_bl.c
>> ...
>> @@ -472,11 +456,31 @@ static int lp855x_probe(struct i2c_client *cl)
>>  	lp->enable = devm_regulator_get_optional(dev, "enable");
>>  	if (IS_ERR(lp->enable)) {
>>  		ret = PTR_ERR(lp->enable);
>> -		if (ret == -ENODEV) {
>> +		if (ret == -ENODEV)
>>  			lp->enable = NULL;
>> -		} else {
>> +		else
>>  			return dev_err_probe(dev, ret, "getting enable regulator\n");
>> -		}
>> +	}
>> +
>> +	lp->pwm = devm_pwm_get(lp->dev, lp->chipname);
>> +	if (IS_ERR(lp->pwm)) {
>> +		ret = PTR_ERR(lp->pwm);
>> +		if (ret == -ENODEV || ret == -EINVAL)
> 
> Why would you ignore EINVAL?

EINVAL is returned when the pwms property is not found in the DT node
for the backlight. Not sure if there's a better way of separately
detecting whether it's present (especially when taking into
consideration non-DT platforms that might use the driver). Would be nice
to have something like devm_regulator_get_optional but for PWMs...

Still, someone who's setting up the driver could check the debug
messages to see if the backlight was set up in PWM mode or register mode.

> ...
>> +		pwm_init_state(lp->pwm, &pwmstate);
>> +		/* Legacy platform data compatibility */
>> +		if (lp->pdata->period_ns > 0)
>> +			pwmstate.period = lp->pdata->period_ns;
>> +		pwm_apply_state(lp->pwm, &pwmstate);
> 
> This is a change in behaviour. Before lp855x_probe() didn't modify the
> state the bootloader left the backlight in. Now you're disabling it (I
> think). Is this intended?

I didn't really consider the implication of this in this way, as on the
device I was testing this on (Exynos4212-based tablet) the PWM state
would get reset during PWM chip initialization in the kernel anyways,
meaning that the state from the bootloader would be lost regardless of
this change. Either way, there's no guarantee that this would be the
same on other devices, though I'd assume that in most cases it's not
noticeable anyways as brightness is usually set somewhere in userspace
(or even earlier, in the driver, if the init-brt property is set).
Nonetheless, that's an oversight on my part.

As for the reasoning for this change in behavior - the previous behavior
was to silently fail if, while setting the brightness, the PWM could not
be set up. This seemed rather confusing to me (I encountered this while
I was initially working on the tablet, I added a "pwm" property instead
of "pwms" and was wondering why the backlight didn't work...)

Of course, that could be fixed by adding error detection in the
brightness set function, but since I was already working on it, it made
more sense to me for the PWM to be set up during the probing process,
given that this way we could 1. warn about errors early, and 2. catch
deferred probes and defer the backlight's probe if we're still waiting
for the PWM. That's why it's done the way it is in this patch.

If this is undesired behavior, let me know and I'll submit another patch
to revert it.

Best regards
Artur

Re: [PATCH 2/4] video: backlight: lp855x: get PWM for PWM mode during probe

[Uwe Kleine-König]

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

Hello,

On Fri, Jun 16, 2023 at 05:29:08PM +0200, Artur Weber wrote:
> On 14/06/2023 10:39, Uwe Kleine-König wrote:
> > On Sat, Apr 29, 2023 at 12:45:32PM +0200, Artur Weber wrote:
> >> Also deprecate the pwm-period DT property, as it is now redundant
> >> (pwms property already contains period value).
> >>
> >> Signed-off-by: Artur Weber <aweber.kernel@gmail.com>
> >> ---
> >>  drivers/video/backlight/lp855x_bl.c | 48 ++++++++++++++++-------------
> >>  1 file changed, 26 insertions(+), 22 deletions(-)
> >>
> >> diff --git a/drivers/video/backlight/lp855x_bl.c b/drivers/video/backlight/lp855x_bl.c
> >> index 81012bf29baf..21eb4943ed56 100644
> >> --- a/drivers/video/backlight/lp855x_bl.c
> >> +++ b/drivers/video/backlight/lp855x_bl.c
> >> ...
> >> @@ -472,11 +456,31 @@ static int lp855x_probe(struct i2c_client *cl)
> >>  	lp->enable = devm_regulator_get_optional(dev, "enable");
> >>  	if (IS_ERR(lp->enable)) {
> >>  		ret = PTR_ERR(lp->enable);
> >> -		if (ret == -ENODEV) {
> >> +		if (ret == -ENODEV)
> >>  			lp->enable = NULL;
> >> -		} else {
> >> +		else
> >>  			return dev_err_probe(dev, ret, "getting enable regulator\n");
> >> -		}
> >> +	}
> >> +
> >> +	lp->pwm = devm_pwm_get(lp->dev, lp->chipname);
> >> +	if (IS_ERR(lp->pwm)) {
> >> +		ret = PTR_ERR(lp->pwm);
> >> +		if (ret == -ENODEV || ret == -EINVAL)
> > 
> > Why would you ignore EINVAL?
> 
> EINVAL is returned when the pwms property is not found in the DT node
> for the backlight. Not sure if there's a better way of separately
> detecting whether it's present (especially when taking into
> consideration non-DT platforms that might use the driver). Would be nice
> to have something like devm_regulator_get_optional but for PWMs...

Ah, that is because of_pwm_get() calls of_property_match_string(np,
"pwm-names", con_id) which returns -EINVAL if there is no pwm-names
property. This is different for clocks. I wonder if pwm should adapt
accordingly? Thierry?

> Still, someone who's setting up the driver could check the debug
> messages to see if the backlight was set up in PWM mode or register mode.
> 
> > ...
> >> +		pwm_init_state(lp->pwm, &pwmstate);
> >> +		/* Legacy platform data compatibility */
> >> +		if (lp->pdata->period_ns > 0)
> >> +			pwmstate.period = lp->pdata->period_ns;
> >> +		pwm_apply_state(lp->pwm, &pwmstate);
> > 
> > This is a change in behaviour. Before lp855x_probe() didn't modify the
> > state the bootloader left the backlight in. Now you're disabling it (I
> > think). Is this intended?
> 
> I didn't really consider the implication of this in this way, as on the
> device I was testing this on (Exynos4212-based tablet) the PWM state
> would get reset during PWM chip initialization in the kernel anyways,

Which chip driver is in use here? That's a patch opportunity.

> meaning that the state from the bootloader would be lost regardless of
> this change. Either way, there's no guarantee that this would be the
> same on other devices, though I'd assume that in most cases it's not
> noticeable anyways as brightness is usually set somewhere in userspace
> (or even earlier, in the driver, if the init-brt property is set).
> Nonetheless, that's an oversight on my part.
> 
> As for the reasoning for this change in behavior - the previous behavior
> was to silently fail if, while setting the brightness, the PWM could not

This sounds wrong.

> be set up. This seemed rather confusing to me (I encountered this while
> I was initially working on the tablet, I added a "pwm" property instead
> of "pwms" and was wondering why the backlight didn't work...)
> 
> Of course, that could be fixed by adding error detection in the
> brightness set function, but since I was already working on it, it made
> more sense to me for the PWM to be set up during the probing process,
> given that this way we could 1. warn about errors early, and 2. catch
> deferred probes and defer the backlight's probe if we're still waiting
> for the PWM. That's why it's done the way it is in this patch.
> 
> If this is undesired behavior, let me know and I'll submit another patch
> to revert it.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]