[PATCH v2 0/3] backlight: lm3630a: bug fix and device tree support

[PATCH v2 0/3] backlight: lm3630a: bug fix and device tree support

[Brian Masney]

Here is a patch series that fixes a single bug and adds device tree
support to the lm3630a driver. This work was tested on a LG Nexus 5
(hammerhead) phone. My status page at
https://masneyb.github.io/nexus-5-upstream/ describes what is working
so far with the upstream kernel.

Changes since v1 (from November):
- Don't use a trivial binding and expose some of the settings available
  in the platform data.

Sorry for the long delay sending out v2. I got busy with other projects.

Brian Masney (3):
  backlight: lm3630a: return 0 on success in update_status functions
  dt-bindings: backlight: add lm3630a bindings
  backlight: lm3630a: add device tree supprt

 .../leds/backlight/lm3630a-backlight.yaml     | 112 ++++++++++++++++++
 drivers/video/backlight/lm3630a_bl.c          |  73 +++++++++++-
 2 files changed, 183 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/leds/backlight/lm3630a-backlight.yaml

-- 
2.20.1

[PATCH v2 1/3] backlight: lm3630a: return 0 on success in update_status functions

[Brian Masney]

lm3630a_bank_a_update_status() and lm3630a_bank_b_update_status()
both return the brightness value if the brightness was successfully
updated. Writing to these attributes via sysfs would cause a 'Bad
address' error to be returned. These functions should return 0 on
success, so let's change it to correct that error.

Signed-off-by: Brian Masney <masneyb@onstation.org>
Fixes: 28e64a68a2ef ("backlight: lm3630: apply chip revision")
---
 drivers/video/backlight/lm3630a_bl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/video/backlight/lm3630a_bl.c b/drivers/video/backlight/lm3630a_bl.c
index 2030a6b77a09..ef2553f452ca 100644
--- a/drivers/video/backlight/lm3630a_bl.c
+++ b/drivers/video/backlight/lm3630a_bl.c
@@ -201,7 +201,7 @@ static int lm3630a_bank_a_update_status(struct backlight_device *bl)
 				      LM3630A_LEDA_ENABLE, LM3630A_LEDA_ENABLE);
 	if (ret < 0)
 		goto out_i2c_err;
-	return bl->props.brightness;
+	return 0;
 
 out_i2c_err:
 	dev_err(pchip->dev, "i2c failed to access\n");
@@ -278,7 +278,7 @@ static int lm3630a_bank_b_update_status(struct backlight_device *bl)
 				      LM3630A_LEDB_ENABLE, LM3630A_LEDB_ENABLE);
 	if (ret < 0)
 		goto out_i2c_err;
-	return bl->props.brightness;
+	return 0;
 
 out_i2c_err:
 	dev_err(pchip->dev, "i2c failed to access REG_CTRL\n");
-- 
2.20.1

Re: [PATCH v2 1/3] backlight: lm3630a: return 0 on success in update_status functions

[Pavel Machek]

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

On Mon 2019-04-01 06:30:32, Brian Masney wrote:
> lm3630a_bank_a_update_status() and lm3630a_bank_b_update_status()
> both return the brightness value if the brightness was successfully
> updated. Writing to these attributes via sysfs would cause a 'Bad
> address' error to be returned. These functions should return 0 on
> success, so let's change it to correct that error.
> 
> Signed-off-by: Brian Masney <masneyb@onstation.org>
> Fixes: 28e64a68a2ef ("backlight: lm3630: apply chip revision")

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

> ---
>  drivers/video/backlight/lm3630a_bl.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/video/backlight/lm3630a_bl.c b/drivers/video/backlight/lm3630a_bl.c
> index 2030a6b77a09..ef2553f452ca 100644
> --- a/drivers/video/backlight/lm3630a_bl.c
> +++ b/drivers/video/backlight/lm3630a_bl.c
> @@ -201,7 +201,7 @@ static int lm3630a_bank_a_update_status(struct backlight_device *bl)
>  				      LM3630A_LEDA_ENABLE, LM3630A_LEDA_ENABLE);
>  	if (ret < 0)
>  		goto out_i2c_err;
> -	return bl->props.brightness;
> +	return 0;
>  
>  out_i2c_err:
>  	dev_err(pchip->dev, "i2c failed to access\n");
> @@ -278,7 +278,7 @@ static int lm3630a_bank_b_update_status(struct backlight_device *bl)
>  				      LM3630A_LEDB_ENABLE, LM3630A_LEDB_ENABLE);
>  	if (ret < 0)
>  		goto out_i2c_err;
> -	return bl->props.brightness;
> +	return 0;
>  
>  out_i2c_err:
>  	dev_err(pchip->dev, "i2c failed to access REG_CTRL\n");

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

[PATCH v2 2/3] dt-bindings: backlight: add lm3630a bindings

[Brian Masney]

Add new backlight bindings for the TI LM3630A dual-string white LED.

Signed-off-by: Brian Masney <masneyb@onstation.org>
---
 .../leds/backlight/lm3630a-backlight.yaml     | 112 ++++++++++++++++++
 1 file changed, 112 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/backlight/lm3630a-backlight.yaml

diff --git a/Documentation/devicetree/bindings/leds/backlight/lm3630a-backlight.yaml b/Documentation/devicetree/bindings/leds/backlight/lm3630a-backlight.yaml
new file mode 100644
index 000000000000..42a8c59d237a
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/backlight/lm3630a-backlight.yaml
@@ -0,0 +1,112 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/leds/backlight/lm3630a-backlight.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: TI LM3630A High-Efficiency Dual-String White LED
+
+maintainers:
+  - Lee Jones <lee.jones@linaro.org>
+  - Daniel Thompson <daniel.thompson@linaro.org>
+  - Jingoo Han <jingoohan1@gmail.com>
+
+description: |
+  The LM3630A is a current-mode boost converter which supplies the power and
+  controls the current in up to two strings of 10 LEDs per string.
+  https://www.ti.com/product/LM3630A
+
+properties:
+  compatible:
+    const: ti,lm3630a
+
+  reg:
+    maxItems: 1
+
+  ti,linear-mapping-mode:
+    description: |
+      Enable linear mapping mode. If disabled, then it will use exponential
+      mapping mode in which the ramp up/down appears to have a more uniform
+      tranisiton to the human eye.
+    type: boolean
+
+required:
+  - compatible
+  - reg
+
+patternProperties:
+  "^led*$":
+    type: object
+    description: |
+      Properties for a string of connected LEDs.
+
+    properties:
+      label:
+        description: |
+          The label for this LED. If omitted, the label is taken from the node
+          name (excluding the unit address). It has to uniquely identify a
+          device, i.e. no other LED class device can be assigned the same label.
+
+      led-sources:
+        description: |
+          List of device current outputs the LED is connected to.
+        allOf:
+          - $ref: /schemas/types.yaml#/definitions/uint32-array
+          - minItems: 1
+            maxItems: 2
+            items:
+              minimum: 0
+              maximum: 1
+
+      default-brightness:
+        description: Default brightness level on boot.
+        minimum: 0
+        maximum: 255
+
+      max-brightness:
+        description: Maximum brightness level on boot.
+        minimum: 0
+        maximum: 255
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        lm3630a_bl@38 {
+                compatible = "ti,lm3630a";
+                status = "ok";
+                reg = <0x38>;
+
+                led {
+                        label = "main-lcd";
+                        led-sources = <0 1>;
+                        default-brightness = <200>;
+                        max-brightness = <255>;
+                };
+        };
+    };
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        lm3630a_bl@38 {
+                compatible = "ti,lm3630a";
+                status = "ok";
+                reg = <0x38>;
+
+                led-bank-a {
+                        led-sources = <0>;
+                        default-brightness = <150>;
+                        ti,linear-mapping-mode;
+                };
+
+                led-bank-b {
+                        led-sources = <1>;
+                        default-brightness = <225>;
+                        ti,linear-mapping-mode;
+                };
+        };
+    };
-- 
2.20.1

Re: [PATCH v2 2/3] dt-bindings: backlight: add lm3630a bindings

[Pavel Machek]

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

On Mon 2019-04-01 06:30:33, Brian Masney wrote:
> Add new backlight bindings for the TI LM3630A dual-string white LED.
> 
> Signed-off-by: Brian Masney <masneyb@onstation.org>
> ---
>  .../leds/backlight/lm3630a-backlight.yaml     | 112
++++++++++++++++++

What is that? Is it future of all the bindings?

Up to device tree people, I guess, but...

								Pavel


>  1 file changed, 112 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/backlight/lm3630a-backlight.yaml
> 
> diff --git a/Documentation/devicetree/bindings/leds/backlight/lm3630a-backlight.yaml b/Documentation/devicetree/bindings/leds/backlight/lm3630a-backlight.yaml
> new file mode 100644
> index 000000000000..42a8c59d237a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/backlight/lm3630a-backlight.yaml
> @@ -0,0 +1,112 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/leds/backlight/lm3630a-backlight.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: TI LM3630A High-Efficiency Dual-String White LED
> +
> +maintainers:
> +  - Lee Jones <lee.jones@linaro.org>
> +  - Daniel Thompson <daniel.thompson@linaro.org>
> +  - Jingoo Han <jingoohan1@gmail.com>
> +
> +description: |
> +  The LM3630A is a current-mode boost converter which supplies the power and
> +  controls the current in up to two strings of 10 LEDs per string.
> +  https://www.ti.com/product/LM3630A
> +
> +properties:
> +  compatible:
> +    const: ti,lm3630a
> +
> +  reg:
> +    maxItems: 1
> +
> +  ti,linear-mapping-mode:
> +    description: |
> +      Enable linear mapping mode. If disabled, then it will use exponential
> +      mapping mode in which the ramp up/down appears to have a more uniform
> +      tranisiton to the human eye.
> +    type: boolean
> +
> +required:
> +  - compatible
> +  - reg
> +
> +patternProperties:
> +  "^led*$":
> +    type: object
> +    description: |
> +      Properties for a string of connected LEDs.
> +
> +    properties:
> +      label:
> +        description: |
> +          The label for this LED. If omitted, the label is taken from the node
> +          name (excluding the unit address). It has to uniquely identify a
> +          device, i.e. no other LED class device can be assigned the same label.
> +
> +      led-sources:
> +        description: |
> +          List of device current outputs the LED is connected to.
> +        allOf:
> +          - $ref: /schemas/types.yaml#/definitions/uint32-array
> +          - minItems: 1
> +            maxItems: 2
> +            items:
> +              minimum: 0
> +              maximum: 1
> +
> +      default-brightness:
> +        description: Default brightness level on boot.
> +        minimum: 0
> +        maximum: 255
> +
> +      max-brightness:
> +        description: Maximum brightness level on boot.
> +        minimum: 0
> +        maximum: 255
> +
> +examples:
> +  - |
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        lm3630a_bl@38 {
> +                compatible = "ti,lm3630a";
> +                status = "ok";
> +                reg = <0x38>;
> +
> +                led {
> +                        label = "main-lcd";
> +                        led-sources = <0 1>;
> +                        default-brightness = <200>;
> +                        max-brightness = <255>;
> +                };
> +        };
> +    };
> +  - |
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        lm3630a_bl@38 {
> +                compatible = "ti,lm3630a";
> +                status = "ok";
> +                reg = <0x38>;
> +
> +                led-bank-a {
> +                        led-sources = <0>;
> +                        default-brightness = <150>;
> +                        ti,linear-mapping-mode;
> +                };
> +
> +                led-bank-b {
> +                        led-sources = <1>;
> +                        default-brightness = <225>;
> +                        ti,linear-mapping-mode;
> +                };
> +        };
> +    };

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

Re: [PATCH v2 2/3] dt-bindings: backlight: add lm3630a bindings

[Brian Masney]

On Mon, Apr 01, 2019 at 11:39:26PM +0200, Pavel Machek wrote:
> On Mon 2019-04-01 06:30:33, Brian Masney wrote:
> > Add new backlight bindings for the TI LM3630A dual-string white LED.
> > 
> > Signed-off-by: Brian Masney <masneyb@onstation.org>
> > ---
> >  .../leds/backlight/lm3630a-backlight.yaml     | 112
> ++++++++++++++++++
> 
> What is that? Is it future of all the bindings?
> 
> Up to device tree people, I guess, but...

Yes, this is the future format of the bindings so that automated
validation can be performed. See
Documentation/devicetree/writing-schema.md.

Brian

Re: [PATCH v2 2/3] dt-bindings: backlight: add lm3630a bindings

[Dan Murphy]

Hello

On 4/1/19 5:30 AM, Brian Masney wrote:
> Add new backlight bindings for the TI LM3630A dual-string white LED.
> 
> Signed-off-by: Brian Masney <masneyb@onstation.org>
> ---
>  .../leds/backlight/lm3630a-backlight.yaml     | 112 ++++++++++++++++++
>  1 file changed, 112 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/backlight/lm3630a-backlight.yaml
> 
> diff --git a/Documentation/devicetree/bindings/leds/backlight/lm3630a-backlight.yaml b/Documentation/devicetree/bindings/leds/backlight/lm3630a-backlight.yaml
> new file mode 100644
> index 000000000000..42a8c59d237a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/backlight/lm3630a-backlight.yaml
> @@ -0,0 +1,112 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/leds/backlight/lm3630a-backlight.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: TI LM3630A High-Efficiency Dual-String White LED
> +
> +maintainers:
> +  - Lee Jones <lee.jones@linaro.org>
> +  - Daniel Thompson <daniel.thompson@linaro.org>
> +  - Jingoo Han <jingoohan1@gmail.com>
> +
> +description: |
> +  The LM3630A is a current-mode boost converter which supplies the power and
> +  controls the current in up to two strings of 10 LEDs per string.
> +  https://www.ti.com/product/LM3630A
> +
> +properties:
> +  compatible:
> +    const: ti,lm3630a
> +
> +  reg:
> +    maxItems: 1
> +

No description on the reg here.  Assumed to be the i2c address?

> +  ti,linear-mapping-mode:
> +    description: |
> +      Enable linear mapping mode. If disabled, then it will use exponential
> +      mapping mode in which the ramp up/down appears to have a more uniform
> +      tranisiton to the human eye.

s/tranisiton/transition

> +    type: boolean
> +
> +required:
> +  - compatible
> +  - reg
> +
> +patternProperties:
> +  "^led*$":
> +    type: object
> +    description: |
> +      Properties for a string of connected LEDs.
> +

I have written a few of these drivers that have control bank support to control.  We have
been defining the reg property as the control bank id

reg 0 - Control bank A
reg 1 - Control bank B

Then use the led-sources to define what outputs are associated with which control bank.

This way the hardware can just have CB B enabled without CB A.

something like below would allow just control bank B to be enabled.

led@1 {
	reg = <1>;
	label = "main-lcd";
	default-brightness = <200>;
	max-brightness = <255>;
};

This would connect control bank B to control bank A.  Or just use a flag to denote to connect them
and not use led-sources.  But led-sources is the property of choice.

led@0 {
	reg = <0>;
	led-sources = < 0 1 >;
	label = "main-lcd";
	default-brightness = <200>;
	max-brightness = <255>;
};

And just the presence of 2 child nodes would indicate individual bank control
led@0 {
	reg = <0>;
	label = "main-lcd";
	default-brightness = <200>;
	max-brightness = <255>;
};

led@1 {
	reg = <1>;
	label = "main-keypad";
	default-brightness = <200>;
	max-brightness = <255>;
};

> +    properties:
> +      label:
> +        description: |
> +          The label for this LED. If omitted, the label is taken from the node
> +          name (excluding the unit address). It has to uniquely identify a
> +          device, i.e. no other LED class device can be assigned the same label.
> +
> +      led-sources:
> +        description: |
> +          List of device current outputs the LED is connected to.
> +        allOf:
> +          - $ref: /schemas/types.yaml#/definitions/uint32-array
> +          - minItems: 1
> +            maxItems: 2
> +            items:
> +              minimum: 0
> +              maximum: 1
> +

label and led-sources are already defined in the common.txt no need to redefine them here.

> +      default-brightness:
> +        description: Default brightness level on boot.
> +        minimum: 0
> +        maximum: 255
> +
> +      max-brightness:
> +        description: Maximum brightness level on boot.

This is not the max brightness on boot.  This is the max brightness that is allowed during runtime.

Dan

> +        minimum: 0
> +        maximum: 255
> +
> +examples:
> +  - |
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        lm3630a_bl@38 {
> +                compatible = "ti,lm3630a";
> +                status = "ok";
> +                reg = <0x38>;
> +
> +                led {
> +                        label = "main-lcd";
> +                        led-sources = <0 1>;
> +                        default-brightness = <200>;
> +                        max-brightness = <255>;
> +                };
> +        };
> +    };
> +  - |
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        lm3630a_bl@38 {
> +                compatible = "ti,lm3630a";
> +                status = "ok";
> +                reg = <0x38>;
> +
> +                led-bank-a {
> +                        led-sources = <0>;
> +                        default-brightness = <150>;
> +                        ti,linear-mapping-mode;
> +                };
> +
> +                led-bank-b {
> +                        led-sources = <1>;
> +                        default-brightness = <225>;
> +                        ti,linear-mapping-mode;
> +                };
> +        };
> +    };
> 


-- 
------------------
Dan Murphy

Re: [PATCH v2 2/3] dt-bindings: backlight: add lm3630a bindings

[Brian Masney]

On Tue, Apr 02, 2019 at 07:56:55AM -0500, Dan Murphy wrote:
> This would connect control bank B to control bank A.  Or just use a flag to denote to connect them
> and not use led-sources.  But led-sources is the property of choice.
> 
> led@0 {
> 	reg = <0>;
> 	led-sources = < 0 1 >;
> 	label = "main-lcd";
> 	default-brightness = <200>;
> 	max-brightness = <255>;
> };

OK, I see. I wondered how we could do that in device tree.

> > +    properties:
> > +      label:
> > +        description: |
> > +          The label for this LED. If omitted, the label is taken from the node
> > +          name (excluding the unit address). It has to uniquely identify a
> > +          device, i.e. no other LED class device can be assigned the same label.
> > +
> > +      led-sources:
> > +        description: |
> > +          List of device current outputs the LED is connected to.
> > +        allOf:
> > +          - $ref: /schemas/types.yaml#/definitions/uint32-array
> > +          - minItems: 1
> > +            maxItems: 2
> > +            items:
> > +              minimum: 0
> > +              maximum: 1
> > +
> 
> label and led-sources are already defined in the common.txt no need to redefine them here.

If I'm going to use the new-style bindings, then I'll need to convert
common.txt over to use the new format as well so that the automated
schema validations will work. I'm willing to do that work if there is
interest from the LED / backlight maintainers. The main issue is that
there are 62 references to the file common.txt in the directory
Documentation/devicetree/bindings/leds/. Would the maintainers prefer:

- Once common.txt is converted to common.yaml, make common.txt only
  have a line stating that the common bindings were moved into
  common.yaml. We can remove this file once all of the other bindings
  are converted to the new-style format.

- Update all references to common.txt to common.yaml. (1 patch or 62
  patches?)

- Or, just go with the older-style binding format for now.

Thanks Dan for your other comments. They make sense and I'll incorporate
those changes into my next version.

Brian

Re: [PATCH v2 2/3] dt-bindings: backlight: add lm3630a bindings

[Dan Murphy]

Brian

On 4/2/19 8:24 AM, Brian Masney wrote:
> On Tue, Apr 02, 2019 at 07:56:55AM -0500, Dan Murphy wrote:
>> This would connect control bank B to control bank A.  Or just use a flag to denote to connect them
>> and not use led-sources.  But led-sources is the property of choice.
>>
>> led@0 {
>> 	reg = <0>;
>> 	led-sources = < 0 1 >;
>> 	label = "main-lcd";
>> 	default-brightness = <200>;
>> 	max-brightness = <255>;
>> };
> 
> OK, I see. I wondered how we could do that in device tree.
> 
>>> +    properties:
>>> +      label:
>>> +        description: |
>>> +          The label for this LED. If omitted, the label is taken from the node
>>> +          name (excluding the unit address). It has to uniquely identify a
>>> +          device, i.e. no other LED class device can be assigned the same label.
>>> +
>>> +      led-sources:
>>> +        description: |
>>> +          List of device current outputs the LED is connected to.
>>> +        allOf:
>>> +          - $ref: /schemas/types.yaml#/definitions/uint32-array
>>> +          - minItems: 1
>>> +            maxItems: 2
>>> +            items:
>>> +              minimum: 0
>>> +              maximum: 1
>>> +
>>
>> label and led-sources are already defined in the common.txt no need to redefine them here.
> 
> If I'm going to use the new-style bindings, then I'll need to convert
> common.txt over to use the new format as well so that the automated
> schema validations will work. I'm willing to do that work if there is
> interest from the LED / backlight maintainers. The main issue is that
> there are 62 references to the file common.txt in the directory
> Documentation/devicetree/bindings/leds/. Would the maintainers prefer:
> 
> - Once common.txt is converted to common.yaml, make common.txt only
>   have a line stating that the common bindings were moved into
>   common.yaml. We can remove this file once all of the other bindings
>   are converted to the new-style format.
> 
> - Update all references to common.txt to common.yaml. (1 patch or 62
>   patches?)
> 
> - Or, just go with the older-style binding format for now.
> 
> Thanks Dan for your other comments. They make sense and I'll incorporate
> those changes into my next version.
> 

That is up to the maintainers.

Also one other comment I noticed when reviewing the code that there is no definition to
which child led properties are optional and which are required?

Dan

> Brian
> 


-- 
------------------
Dan Murphy

Re: [PATCH v2 2/3] dt-bindings: backlight: add lm3630a bindings

[Brian Masney]

Hi Dan,

On Tue, Apr 02, 2019 at 08:44:22AM -0500, Dan Murphy wrote:
> Also one other comment I noticed when reviewing the code that there is no definition to
> which child led properties are optional and which are required?

With the new YAML bindings, there is a separate toplevel 'required' tag in
the schema. Here is a snippet from my last submission that illustrates
this:

    properties:
      compatible:
        const: ti,lm3630a
    
      reg:
        maxItems: 1
    
      ti,linear-mapping-mode:
        description: |
          Enable linear mapping mode. If disabled, then it will use exponential
          mapping mode in which the ramp up/down appears to have a more uniform
          transition to the human eye.
        type: boolean
    
    required:
      - compatible
      - reg

So 'ti,linear-mapping-mode' is optional in this example.

> > +      led-sources:
> > +        description: |
> > +          List of device current outputs the LED is connected to.
> > +        allOf:
> > +          - $ref: /schemas/types.yaml#/definitions/uint32-array
> > +          - minItems: 1
> > +            maxItems: 2
> > +            items:
> > +              minimum: 0
> > +              maximum: 1
> > +
> 
> label and led-sources are already defined in the common.txt no need to redefine them here.

We'll still need to define the led-sources and label in this binding
with the new format even though it will also be in the LED common
bindings. We won't have to put the common things like the description
since that information will come from the common binding. We only need
to specify the additional constraints like the min/max number of items
and the min/max value for each item for this particular example.

Brian

Re: [PATCH v2 2/3] dt-bindings: backlight: add lm3630a bindings

[Rob Herring]

On Tue, Apr 2, 2019 at 8:24 AM Brian Masney <masneyb@onstation.org> wrote:
>
> On Tue, Apr 02, 2019 at 07:56:55AM -0500, Dan Murphy wrote:
> > This would connect control bank B to control bank A.  Or just use a flag to denote to connect them
> > and not use led-sources.  But led-sources is the property of choice.
> >
> > led@0 {
> >       reg = <0>;
> >       led-sources = < 0 1 >;
> >       label = "main-lcd";
> >       default-brightness = <200>;
> >       max-brightness = <255>;
> > };
>
> OK, I see. I wondered how we could do that in device tree.
>
> > > +    properties:
> > > +      label:
> > > +        description: |
> > > +          The label for this LED. If omitted, the label is taken from the node
> > > +          name (excluding the unit address). It has to uniquely identify a
> > > +          device, i.e. no other LED class device can be assigned the same label.
> > > +
> > > +      led-sources:
> > > +        description: |
> > > +          List of device current outputs the LED is connected to.
> > > +        allOf:
> > > +          - $ref: /schemas/types.yaml#/definitions/uint32-array
> > > +          - minItems: 1
> > > +            maxItems: 2
> > > +            items:
> > > +              minimum: 0
> > > +              maximum: 1
> > > +
> >
> > label and led-sources are already defined in the common.txt no need to redefine them here.
>
> If I'm going to use the new-style bindings, then I'll need to convert
> common.txt over to use the new format as well so that the automated
> schema validations will work. I'm willing to do that work if there is
> interest from the LED / backlight maintainers. The main issue is that
> there are 62 references to the file common.txt in the directory
> Documentation/devicetree/bindings/leds/. Would the maintainers prefer:

That would be awesome.

> - Once common.txt is converted to common.yaml, make common.txt only
>   have a line stating that the common bindings were moved into
>   common.yaml. We can remove this file once all of the other bindings
>   are converted to the new-style format.
>
> - Update all references to common.txt to common.yaml. (1 patch or 62
>   patches?)

I'd probably go with the former if I had to pick. The real
complication here is there's some other changes in flight.

>
> - Or, just go with the older-style binding format for now.

I'm okay if you write it with the assumption that the common
properties have a schema even if they don't yet. All that means is you
don't need to define the type, but only the additional constraints.
There doesn't have to be a reference back to the common property
(those references tend to get broken as we can see here). With
everything as structured data, we could even generate references or
have some tool that can lookup the common definitions.

Rob

[PATCH v2 3/3] backlight: lm3630a: add device tree supprt

[Brian Masney]

Add device tree support to the lm3630a driver and allow configuring
independently on both banks the mapping mode (linear or exponential),
initial and maximum LED brightness.

Driver was tested on a LG Nexus 5 (hammerhead) phone.

Signed-off-by: Brian Masney <masneyb@onstation.org>
---
 drivers/video/backlight/lm3630a_bl.c | 69 ++++++++++++++++++++++++++++
 1 file changed, 69 insertions(+)

diff --git a/drivers/video/backlight/lm3630a_bl.c b/drivers/video/backlight/lm3630a_bl.c
index ef2553f452ca..96fbc1273dda 100644
--- a/drivers/video/backlight/lm3630a_bl.c
+++ b/drivers/video/backlight/lm3630a_bl.c
@@ -35,6 +35,9 @@
 #define REG_MAX		0x50
 
 #define INT_DEBOUNCE_MSEC	10
+
+#define LM3630A_MAX_SOURCES	2
+
 struct lm3630a_chip {
 	struct device *dev;
 	struct delayed_work work;
@@ -364,6 +367,64 @@ static const struct regmap_config lm3630a_regmap = {
 	.max_register = REG_MAX,
 };
 
+static void lm3630a_parse_dt(struct lm3630a_chip *pchip)
+{
+	u32 sources[LM3630A_MAX_SOURCES], val;
+	struct device_node *child_node;
+	int num_sources, ret, i;
+	bool linear;
+
+	for_each_available_child_of_node(pchip->dev->of_node, child_node) {
+		num_sources = of_property_count_u32_elems(child_node,
+							  "led-sources");
+		if (num_sources < 0)
+			continue;
+
+		if (num_sources > LM3630A_MAX_SOURCES)
+			num_sources = LM3630A_MAX_SOURCES;
+
+		ret = of_property_read_u32_array(child_node, "led-sources",
+						 sources, num_sources);
+		if (ret) {
+			dev_err(pchip->dev,
+				"Error parsing led-sources node: %d\n", ret);
+			return;
+		}
+
+		linear = of_property_read_bool(child_node,
+					       "ti,linear-mapping-mode");
+
+		for (i = 0; i < num_sources; i++) {
+			if (sources[i] == 0)
+				pchip->pdata->leda_ctrl = linear ?
+					LM3630A_LEDA_ENABLE_LINEAR :
+					LM3630A_LEDA_ENABLE;
+			else if (sources[i] == 1)
+				pchip->pdata->ledb_ctrl = linear ?
+					LM3630A_LEDB_ENABLE_LINEAR :
+					LM3630A_LEDB_ENABLE;
+
+			ret = of_property_read_u32(child_node,
+						   "default-brightness", &val);
+			if (!ret) {
+				if (sources[i] == 0)
+					pchip->pdata->leda_init_brt = val;
+				else if (sources[i] == 1)
+					pchip->pdata->ledb_init_brt = val;
+			}
+
+			ret = of_property_read_u32(child_node, "max-brightness",
+						   &val);
+			if (!ret) {
+				if (sources[i] == 0)
+					pchip->pdata->leda_max_brt = val;
+				else if (sources[i] == 1)
+					pchip->pdata->ledb_max_brt = val;
+			}
+		}
+	};
+}
+
 static int lm3630a_probe(struct i2c_client *client,
 			 const struct i2c_device_id *id)
 {
@@ -405,6 +466,7 @@ static int lm3630a_probe(struct i2c_client *client,
 		pdata->ledb_init_brt = LM3630A_MAX_BRIGHTNESS;
 	}
 	pchip->pdata = pdata;
+	lm3630a_parse_dt(pchip);
 
 	/* chip initialize */
 	rval = lm3630a_chip_init(pchip);
@@ -470,11 +532,18 @@ static const struct i2c_device_id lm3630a_id[] = {
 	{}
 };
 
+static const struct of_device_id lm3630a_match_table[] = {
+	{ .compatible = "ti,lm3630a", },
+	{ },
+};
+
+
 MODULE_DEVICE_TABLE(i2c, lm3630a_id);
 
 static struct i2c_driver lm3630a_i2c_driver = {
 	.driver = {
 		   .name = LM3630A_NAME,
+		   .of_match_table = lm3630a_match_table,
 		   },
 	.probe = lm3630a_probe,
 	.remove = lm3630a_remove,
-- 
2.20.1

Re: [PATCH v2 3/3] backlight: lm3630a: add device tree supprt

[Pavel Machek]

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

On Mon 2019-04-01 06:30:34, Brian Masney wrote:
> Add device tree support to the lm3630a driver and allow configuring
> independently on both banks the mapping mode (linear or exponential),
> initial and maximum LED brightness.
> 
> Driver was tested on a LG Nexus 5 (hammerhead) phone.
> 
> Signed-off-by: Brian Masney <masneyb@onstation.org>
> ---
>  drivers/video/backlight/lm3630a_bl.c | 69 ++++++++++++++++++++++++++++
>  1 file changed, 69 insertions(+)
> 
> diff --git a/drivers/video/backlight/lm3630a_bl.c b/drivers/video/backlight/lm3630a_bl.c
> index ef2553f452ca..96fbc1273dda 100644
> --- a/drivers/video/backlight/lm3630a_bl.c
> +++ b/drivers/video/backlight/lm3630a_bl.c
> @@ -35,6 +35,9 @@
>  #define REG_MAX		0x50
>  
>  #define INT_DEBOUNCE_MSEC	10
> +
> +#define LM3630A_MAX_SOURCES	2
> +
>  struct lm3630a_chip {
>  	struct device *dev;
>  	struct delayed_work work;
> @@ -364,6 +367,64 @@ static const struct regmap_config lm3630a_regmap = {
>  	.max_register = REG_MAX,
>  };
>  
> +static void lm3630a_parse_dt(struct lm3630a_chip *pchip)
> +{
> +	u32 sources[LM3630A_MAX_SOURCES], val;
> +	struct device_node *child_node;
> +	int num_sources, ret, i;
> +	bool linear;
> +
> +	for_each_available_child_of_node(pchip->dev->of_node, child_node) {
> +		num_sources = of_property_count_u32_elems(child_node,
> +							  "led-sources");
> +		if (num_sources < 0)
> +			continue;
> +
> +		if (num_sources > LM3630A_MAX_SOURCES)
> +			num_sources = LM3630A_MAX_SOURCES;
> +
> +		ret = of_property_read_u32_array(child_node, "led-sources",
> +						 sources, num_sources);
> +		if (ret) {
> +			dev_err(pchip->dev,
> +				"Error parsing led-sources node: %d\n", ret);
> +			return;
> +		}
> +
> +		linear = of_property_read_bool(child_node,
> +					       "ti,linear-mapping-mode");
> +
> +		for (i = 0; i < num_sources; i++) {
> +			if (sources[i] == 0)
> +				pchip->pdata->leda_ctrl = linear ?
> +					LM3630A_LEDA_ENABLE_LINEAR :
> +					LM3630A_LEDA_ENABLE;
> +			else if (sources[i] == 1)
> +				pchip->pdata->ledb_ctrl = linear ?
> +					LM3630A_LEDB_ENABLE_LINEAR :
> +					LM3630A_LEDB_ENABLE;

This makes my head spin.

So ... we can have multiple LEDs, each can have up to two
sources.. and the settings are really per source, not per LED.

But you do not test for overlaps. What prevents me from having

   foo {
       led_sources = <0>;
       ti,linear-mapping-mode;
   }
   bar {
       led_sources = <0>;
   }

(I.e. conflicting settings for a source?)

Plus I do not see parsing of led labels etc...

> +			ret = of_property_read_u32(child_node,
> +						   "default-brightness", &val);
> +			if (!ret) {
> +				if (sources[i] == 0)
> +					pchip->pdata->leda_init_brt = val;
> +				else if (sources[i] == 1)
> +					pchip->pdata->ledb_init_brt = val;
> +			}
> +
> +			ret = of_property_read_u32(child_node, "max-brightness",
> +						   &val);
> +			if (!ret) {
> +				if (sources[i] == 0)
> +					pchip->pdata->leda_max_brt = val;
> +				else if (sources[i] == 1)
> +					pchip->pdata->ledb_max_brt = val;
> +			}
> +		}
> +	};

Extra ";"

> +}
> +
>  static int lm3630a_probe(struct i2c_client *client,
>  			 const struct i2c_device_id *id)
>  {
> @@ -405,6 +466,7 @@ static int lm3630a_probe(struct i2c_client *client,
>  		pdata->ledb_init_brt = LM3630A_MAX_BRIGHTNESS;
>  	}
>  	pchip->pdata = pdata;
> +	lm3630a_parse_dt(pchip);

I'd expect abort if we are using dt and dt parsing fails.

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

Re: [PATCH v2 3/3] backlight: lm3630a: add device tree supprt

[Brian Masney]

On Mon, Apr 01, 2019 at 11:48:47PM +0200, Pavel Machek wrote:
> So ... we can have multiple LEDs, each can have up to two
> sources.. and the settings are really per source, not per LED.
> 
> But you do not test for overlaps. What prevents me from having
> 
>    foo {
>        led_sources = <0>;
>        ti,linear-mapping-mode;
>    }
>    bar {
>        led_sources = <0>;
>    }
> 
> (I.e. conflicting settings for a source?)

In this case, it will go with the settings for 'bar'. I didn't check for
the conflicting settings since I was going for consistency with the
other two backlight drivers that already have the led-sources property:
arcxcnn_bl.c and sky81452-backlight.c. I can add the additional check
to fail if a source has already been encountered.

> Plus I do not see parsing of led labels etc...

OK... I can fix that up plus your other two comments.

Thanks,

Brian

Re: [PATCH v2 3/3] backlight: lm3630a: add device tree supprt

[Dan Murphy]

Hello

On 4/1/19 5:30 AM, Brian Masney wrote:
> Add device tree support to the lm3630a driver and allow configuring
> independently on both banks the mapping mode (linear or exponential),
> initial and maximum LED brightness.
> 
> Driver was tested on a LG Nexus 5 (hammerhead) phone.
> 

Don't need this in the commit message.

> Signed-off-by: Brian Masney <masneyb@onstation.org>
> ---
>  drivers/video/backlight/lm3630a_bl.c | 69 ++++++++++++++++++++++++++++
>  1 file changed, 69 insertions(+)
> 
> diff --git a/drivers/video/backlight/lm3630a_bl.c b/drivers/video/backlight/lm3630a_bl.c
> index ef2553f452ca..96fbc1273dda 100644
> --- a/drivers/video/backlight/lm3630a_bl.c
> +++ b/drivers/video/backlight/lm3630a_bl.c
> @@ -35,6 +35,9 @@
>  #define REG_MAX		0x50
>  
>  #define INT_DEBOUNCE_MSEC	10
> +
> +#define LM3630A_MAX_SOURCES	2
> +
>  struct lm3630a_chip {
>  	struct device *dev;
>  	struct delayed_work work;
> @@ -364,6 +367,64 @@ static const struct regmap_config lm3630a_regmap = {
>  	.max_register = REG_MAX,
>  };
>  
> +static void lm3630a_parse_dt(struct lm3630a_chip *pchip)
> +{
> +	u32 sources[LM3630A_MAX_SOURCES], val;
> +	struct device_node *child_node;
> +	int num_sources, ret, i;
> +	bool linear;
> +
> +	for_each_available_child_of_node(pchip->dev->of_node, child_node) {

I would prefer we use fwnode api's.

See lm3532 patch submission for example
https://lore.kernel.org/patchwork/patch/1053132/

> +		num_sources = of_property_count_u32_elems(child_node,
> +							  "led-sources");
> +		if (num_sources < 0)
> +			continue;
> +

Not sure what this is checking here.  Wondering if you should return an error here instead.

> +		if (num_sources > LM3630A_MAX_SOURCES)
> +			num_sources = LM3630A_MAX_SOURCES;
> +

If num_sources is greater than max should we not log and return an error?
Why does the driver fix a malformed property?

> +		ret = of_property_read_u32_array(child_node, "led-sources",
> +						 sources, num_sources);
> +		if (ret) {
> +			dev_err(pchip->dev,
> +				"Error parsing led-sources node: %d\n", ret);
> +			return;

We should return an error here see comment below on the order of operation pdata->dt->default parameters.

> +		}
> +
> +		linear = of_property_read_bool(child_node,
> +					       "ti,linear-mapping-mode");
> +
> +		for (i = 0; i < num_sources; i++) {

If the reg property is used to determine control bank then this for..loop may be eliminated


> +			if (sources[i] == 0)
> +				pchip->pdata->leda_ctrl = linear ?
> +					LM3630A_LEDA_ENABLE_LINEAR :
> +					LM3630A_LEDA_ENABLE;
> +			else if (sources[i] == 1)
> +				pchip->pdata->ledb_ctrl = linear ?
> +					LM3630A_LEDB_ENABLE_LINEAR :
> +					LM3630A_LEDB_ENABLE;
> +
> +			ret = of_property_read_u32(child_node,
> +						   "default-brightness", &val);
> +			if (!ret) {
> +				if (sources[i] == 0)
> +					pchip->pdata->leda_init_brt = val;
> +				else if (sources[i] == 1)
> +					pchip->pdata->ledb_init_brt = val;
> +			}
> +
> +			ret = of_property_read_u32(child_node, "max-brightness",
> +						   &val);
> +			if (!ret) {
> +				if (sources[i] == 0)
> +					pchip->pdata->leda_max_brt = val;
> +				else if (sources[i] == 1)
> +					pchip->pdata->ledb_max_brt = val;
> +			}
> +		}
> +	};
> +}
> +
>  static int lm3630a_probe(struct i2c_client *client,
>  			 const struct i2c_device_id *id)
>  {
> @@ -405,6 +466,7 @@ static int lm3630a_probe(struct i2c_client *client,
>  		pdata->ledb_init_brt = LM3630A_MAX_BRIGHTNESS;
>  	}
>  	pchip->pdata = pdata;
> +	lm3630a_parse_dt(pchip);
>  

Hmm.  Wondering if this should be in the if (pdata == NULL) case and if parsing the
node fails then set defaults.

Because the way it is written if pdata is null then all the defaults are set then the dt parsing
overwrites the data.

Not sure if thats what you intended.

Dan

>  	/* chip initialize */
>  	rval = lm3630a_chip_init(pchip);
> @@ -470,11 +532,18 @@ static const struct i2c_device_id lm3630a_id[] = {
>  	{}
>  };
>  
> +static const struct of_device_id lm3630a_match_table[] = {
> +	{ .compatible = "ti,lm3630a", },
> +	{ },
> +};
> +
> +
>  MODULE_DEVICE_TABLE(i2c, lm3630a_id);
>  
>  static struct i2c_driver lm3630a_i2c_driver = {
>  	.driver = {
>  		   .name = LM3630A_NAME,
> +		   .of_match_table = lm3630a_match_table,
>  		   },
>  	.probe = lm3630a_probe,
>  	.remove = lm3630a_remove,
> 


-- 
------------------
Dan Murphy

Re: [PATCH v2 3/3] backlight: lm3630a: add device tree supprt

[Pavel Machek]

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

On Tue 2019-04-02 08:45:40, Dan Murphy wrote:
> Hello
> 
> On 4/1/19 5:30 AM, Brian Masney wrote:
> > Add device tree support to the lm3630a driver and allow configuring
> > independently on both banks the mapping mode (linear or exponential),
> > initial and maximum LED brightness.
> > 
> > Driver was tested on a LG Nexus 5 (hammerhead) phone.
> > 
> 
> Don't need this in the commit message.

Actually yes, we want that in commit message.

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

Re: [PATCH v2 3/3] backlight: lm3630a: add device tree supprt

[Dan Murphy]

On 4/2/19 11:45 AM, Pavel Machek wrote:
> On Tue 2019-04-02 08:45:40, Dan Murphy wrote:
>> Hello
>>
>> On 4/1/19 5:30 AM, Brian Masney wrote:
>>> Add device tree support to the lm3630a driver and allow configuring
>>> independently on both banks the mapping mode (linear or exponential),
>>> initial and maximum LED brightness.
>>>
>>> Driver was tested on a LG Nexus 5 (hammerhead) phone.
>>>
>>
>> Don't need this in the commit message.
> 
> Actually yes, we want that in commit message.
> 

Noted for future patches.

Dan

> 									Pavel
> 


-- 
------------------
Dan Murphy