diff mbox series

[1/5] dt-bindings: leds: Add bindings for lltc,lt3593

Message ID 20180617114929.719-1-daniel@zonque.org
State Superseded, archived
Headers show
Series [1/5] dt-bindings: leds: Add bindings for lltc,lt3593 | expand

Commit Message

Daniel Mack June 17, 2018, 11:49 a.m. UTC
Signed-off-by: Daniel Mack <daniel@zonque.org>
---
 .../devicetree/bindings/leds/leds-lt3593.txt  | 25 +++++++++++++++++++
 1 file changed, 25 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-lt3593.txt

Comments

Jacek Anaszewski June 17, 2018, 7:39 p.m. UTC | #1
Hi Daniel,

Thank you for the patch.

On 06/17/2018 01:49 PM, Daniel Mack wrote:
> Signed-off-by: Daniel Mack <daniel@zonque.org>
> ---
>   .../devicetree/bindings/leds/leds-lt3593.txt  | 25 +++++++++++++++++++
>   1 file changed, 25 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/leds/leds-lt3593.txt
> 
> diff --git a/Documentation/devicetree/bindings/leds/leds-lt3593.txt b/Documentation/devicetree/bindings/leds/leds-lt3593.txt
> new file mode 100644
> index 000000000000..0a7035acda8f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-lt3593.txt
> @@ -0,0 +1,25 @@
> +Bindings for Linear Technologies LT3593 LED controller
> +
> +Required properties:
> +- compatible : should be "lltc,lt3593".
> +- label: A label for the LED.
> +
> +Optional properties:
> +- linux,default-trigger :  (optional)
> +  see Documentation/devicetree/bindings/leds/common.txt
> +- default-state:  (optional) The initial state of the LED.
> +  see Documentation/devicetree/bindings/leds/common.txt
> +
> +The controller only suports one LED, hence the properties are all set
> +in the root node of the device. If multiple chips of this kind are
> +found in a design, each one needs to be handled by its own device node.

Please do not make an excuse for a single LED. Let's keep the bindings
consistent across all LED class drivers, i.e. each LED should have
a dedicated child node. Even there is a single output available.

Please compare how recently added LED bindings look like.
This also influences the way of LED class device name construction.
Please also refer to the recently added drivers for that, e.g.:
drivers/leds/leds-cr0014114.c.

> +Example:
> +
> +led-controller {
> +	compatible = "lltc,lt3593";
> +	label = "led-stripe";

Label should stick to the "color:function" pattern.

> +	gpios = <&gpio 0 GPIO_ACTIVE_HIGH>;
> +	linux,default-trigger = "heartbeat";
> +};
> +
>
Daniel Mack June 17, 2018, 7:52 p.m. UTC | #2
Hi Jacek,

Thanks for the prompt reply!

On Sunday, June 17, 2018 09:39 PM, Jacek Anaszewski wrote:
> On 06/17/2018 01:49 PM, Daniel Mack wrote:
>> Signed-off-by: Daniel Mack <daniel@zonque.org>
>> ---
>>    .../devicetree/bindings/leds/leds-lt3593.txt  | 25 +++++++++++++++++++
>>    1 file changed, 25 insertions(+)
>>    create mode 100644 Documentation/devicetree/bindings/leds/leds-lt3593.txt
>>
>> diff --git a/Documentation/devicetree/bindings/leds/leds-lt3593.txt b/Documentation/devicetree/bindings/leds/leds-lt3593.txt
>> new file mode 100644
>> index 000000000000..0a7035acda8f
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/leds/leds-lt3593.txt
>> @@ -0,0 +1,25 @@
>> +Bindings for Linear Technologies LT3593 LED controller
>> +
>> +Required properties:
>> +- compatible : should be "lltc,lt3593".
>> +- label: A label for the LED.
>> +
>> +Optional properties:
>> +- linux,default-trigger :  (optional)
>> +  see Documentation/devicetree/bindings/leds/common.txt
>> +- default-state:  (optional) The initial state of the LED.
>> +  see Documentation/devicetree/bindings/leds/common.txt
>> +
>> +The controller only suports one LED, hence the properties are all set
>> +in the root node of the device. If multiple chips of this kind are
>> +found in a design, each one needs to be handled by its own device node.
> 
> Please do not make an excuse for a single LED. Let's keep the bindings
> consistent across all LED class drivers, i.e. each LED should have
> a dedicated child node. Even there is a single output available.

Okay, I can do that. But you do agree with the decision to only support 
one single LED in the driver and the binding? The hardware chip can only 
drive one single LED, but of course, the driver could support multiple 
chips with a single device node and different sub-nodes. In fact the 
driver did support this via pdata before this series.

I believe, however, that this is against the principles of how 
device-trees are supposed to describe the hardware, so I changed it. And 
as I wrote in the commit log, the only mainline user only uses one LED 
anyway, so the change won't cause any regression.


Thanks,
Daniel
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Daniel Mack June 17, 2018, 8:08 p.m. UTC | #3
On Sunday, June 17, 2018 09:52 PM, Daniel Mack wrote:
> On Sunday, June 17, 2018 09:39 PM, Jacek Anaszewski wrote:

>> Please do not make an excuse for a single LED. Let's keep the bindings
>> consistent across all LED class drivers, i.e. each LED should have
>> a dedicated child node. Even there is a single output available.
> 
> Okay, I can do that. But you do agree with the decision to only support
> one single LED in the driver and the binding? The hardware chip can only
> drive one single LED, but of course, the driver could support multiple
> chips with a single device node and different sub-nodes. In fact the
> driver did support this via pdata before this series.
> 
> I believe, however, that this is against the principles of how
> device-trees are supposed to describe the hardware, so I changed it. And
> as I wrote in the commit log, the only mainline user only uses one LED
> anyway, so the change won't cause any regression.

And btw, that one user of the driver is soon to be replaced by a 
device-tree based version, and once that's finished, I'll send another 
patch to kill pdata handling from this driver entirely.

So the question is only how the DT bindings should look like, the pdata 
bits are just kept around for legacy compat and don't matter.


Thanks,
Daniel
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/leds/leds-lt3593.txt b/Documentation/devicetree/bindings/leds/leds-lt3593.txt
new file mode 100644
index 000000000000..0a7035acda8f
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-lt3593.txt
@@ -0,0 +1,25 @@ 
+Bindings for Linear Technologies LT3593 LED controller
+
+Required properties:
+- compatible : should be "lltc,lt3593".
+- label: A label for the LED.
+
+Optional properties:
+- linux,default-trigger :  (optional)
+  see Documentation/devicetree/bindings/leds/common.txt
+- default-state:  (optional) The initial state of the LED.
+  see Documentation/devicetree/bindings/leds/common.txt
+
+The controller only suports one LED, hence the properties are all set
+in the root node of the device. If multiple chips of this kind are
+found in a design, each one needs to be handled by its own device node.
+
+Example:
+
+led-controller {
+	compatible = "lltc,lt3593";
+	label = "led-stripe";
+	gpios = <&gpio 0 GPIO_ACTIVE_HIGH>;
+	linux,default-trigger = "heartbeat";
+};
+