[PATCH 1/3] dt-bindings: auxdisplay: hit,hd44780: drop redundant GPIO node

[PATCH 1/3] dt-bindings: auxdisplay: hit,hd44780: drop redundant GPIO node

[Krzysztof Kozlowski]

Examples of other nodes, like GPIO controller, are redundant and not
really needed in device bindings.

Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 .../devicetree/bindings/auxdisplay/hit,hd44780.yaml    | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/Documentation/devicetree/bindings/auxdisplay/hit,hd44780.yaml b/Documentation/devicetree/bindings/auxdisplay/hit,hd44780.yaml
index 406a922a714e..73d07f2cb303 100644
--- a/Documentation/devicetree/bindings/auxdisplay/hit,hd44780.yaml
+++ b/Documentation/devicetree/bindings/auxdisplay/hit,hd44780.yaml
@@ -99,17 +99,7 @@ examples:
     };
   - |
     #include <dt-bindings/gpio/gpio.h>
-    i2c {
-            #address-cells = <1>;
-            #size-cells = <0>;
 
-            pcf8574: pcf8574@27 {
-                    compatible = "nxp,pcf8574";
-                    reg = <0x27>;
-                    gpio-controller;
-                    #gpio-cells = <2>;
-            };
-    };
     hd44780 {
             compatible = "hit,hd44780";
             display-height-chars = <2>;
-- 
2.34.1

[PATCH 2/3] dt-bindings: auxdisplay: adjust example indentation and use generic node names

[Krzysztof Kozlowski]

The example DTS should be indented with two or four (preferred) spaces,
as mentioned in Writing Schema document.  While re-indenting, change the
node names to somehow generic names, as expected by Devicetree
specification.

Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 .../auxdisplay/arm,versatile-lcd.yaml         |  4 +-
 .../bindings/auxdisplay/hit,hd44780.yaml      | 44 ++++++++--------
 .../bindings/auxdisplay/holtek,ht16k33.yaml   | 50 +++++++++----------
 .../bindings/auxdisplay/img,ascii-lcd.yaml    |  4 +-
 4 files changed, 51 insertions(+), 51 deletions(-)

diff --git a/Documentation/devicetree/bindings/auxdisplay/arm,versatile-lcd.yaml b/Documentation/devicetree/bindings/auxdisplay/arm,versatile-lcd.yaml
index 5d02bd032a85..439f7b811a94 100644
--- a/Documentation/devicetree/bindings/auxdisplay/arm,versatile-lcd.yaml
+++ b/Documentation/devicetree/bindings/auxdisplay/arm,versatile-lcd.yaml
@@ -39,6 +39,6 @@ additionalProperties: false
 examples:
   - |
     lcd@10008000 {
-            compatible = "arm,versatile-lcd";
-            reg = <0x10008000 0x1000>;
+        compatible = "arm,versatile-lcd";
+        reg = <0x10008000 0x1000>;
     };
diff --git a/Documentation/devicetree/bindings/auxdisplay/hit,hd44780.yaml b/Documentation/devicetree/bindings/auxdisplay/hit,hd44780.yaml
index 73d07f2cb303..1c7fd29bbcc7 100644
--- a/Documentation/devicetree/bindings/auxdisplay/hit,hd44780.yaml
+++ b/Documentation/devicetree/bindings/auxdisplay/hit,hd44780.yaml
@@ -84,32 +84,32 @@ additionalProperties: false
 examples:
   - |
     #include <dt-bindings/gpio/gpio.h>
-    auxdisplay {
-            compatible = "hit,hd44780";
+    display-controller {
+        compatible = "hit,hd44780";
 
-            data-gpios = <&hc595 0 GPIO_ACTIVE_HIGH>,
-                         <&hc595 1 GPIO_ACTIVE_HIGH>,
-                         <&hc595 2 GPIO_ACTIVE_HIGH>,
-                         <&hc595 3 GPIO_ACTIVE_HIGH>;
-            enable-gpios = <&hc595 4 GPIO_ACTIVE_HIGH>;
-            rs-gpios = <&hc595 5 GPIO_ACTIVE_HIGH>;
+        data-gpios = <&hc595 0 GPIO_ACTIVE_HIGH>,
+                     <&hc595 1 GPIO_ACTIVE_HIGH>,
+                     <&hc595 2 GPIO_ACTIVE_HIGH>,
+                     <&hc595 3 GPIO_ACTIVE_HIGH>;
+        enable-gpios = <&hc595 4 GPIO_ACTIVE_HIGH>;
+        rs-gpios = <&hc595 5 GPIO_ACTIVE_HIGH>;
 
-            display-height-chars = <2>;
-            display-width-chars = <16>;
+        display-height-chars = <2>;
+        display-width-chars = <16>;
     };
   - |
     #include <dt-bindings/gpio/gpio.h>
 
-    hd44780 {
-            compatible = "hit,hd44780";
-            display-height-chars = <2>;
-            display-width-chars = <16>;
-            data-gpios = <&pcf8574 4 0>,
-                         <&pcf8574 5 0>,
-                         <&pcf8574 6 0>,
-                         <&pcf8574 7 0>;
-            enable-gpios = <&pcf8574 2 0>;
-            rs-gpios = <&pcf8574 0 0>;
-            rw-gpios = <&pcf8574 1 0>;
-            backlight-gpios = <&pcf8574 3 0>;
+    display-controller {
+        compatible = "hit,hd44780";
+        display-height-chars = <2>;
+        display-width-chars = <16>;
+        data-gpios = <&pcf8574 4 0>,
+                     <&pcf8574 5 0>,
+                     <&pcf8574 6 0>,
+                     <&pcf8574 7 0>;
+        enable-gpios = <&pcf8574 2 0>;
+        rs-gpios = <&pcf8574 0 0>;
+        rw-gpios = <&pcf8574 1 0>;
+        backlight-gpios = <&pcf8574 3 0>;
     };
diff --git a/Documentation/devicetree/bindings/auxdisplay/holtek,ht16k33.yaml b/Documentation/devicetree/bindings/auxdisplay/holtek,ht16k33.yaml
index be95f6b97b41..b90eec2077b4 100644
--- a/Documentation/devicetree/bindings/auxdisplay/holtek,ht16k33.yaml
+++ b/Documentation/devicetree/bindings/auxdisplay/holtek,ht16k33.yaml
@@ -74,31 +74,31 @@ examples:
     #include <dt-bindings/input/input.h>
     #include <dt-bindings/leds/common.h>
     i2c {
-            #address-cells = <1>;
-            #size-cells = <0>;
+        #address-cells = <1>;
+        #size-cells = <0>;
 
-            ht16k33: ht16k33@70 {
-                    compatible = "holtek,ht16k33";
-                    reg = <0x70>;
-                    refresh-rate-hz = <20>;
-                    interrupt-parent = <&gpio4>;
-                    interrupts = <5 (IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_EDGE_RISING)>;
-                    debounce-delay-ms = <50>;
-                    linux,keymap = <MATRIX_KEY(2, 0, KEY_F6)>,
-                                   <MATRIX_KEY(3, 0, KEY_F8)>,
-                                   <MATRIX_KEY(4, 0, KEY_F10)>,
-                                   <MATRIX_KEY(5, 0, KEY_F4)>,
-                                   <MATRIX_KEY(6, 0, KEY_F2)>,
-                                   <MATRIX_KEY(2, 1, KEY_F5)>,
-                                   <MATRIX_KEY(3, 1, KEY_F7)>,
-                                   <MATRIX_KEY(4, 1, KEY_F9)>,
-                                   <MATRIX_KEY(5, 1, KEY_F3)>,
-                                   <MATRIX_KEY(6, 1, KEY_F1)>;
+        display-controller@70 {
+            compatible = "holtek,ht16k33";
+            reg = <0x70>;
+            refresh-rate-hz = <20>;
+            interrupt-parent = <&gpio4>;
+            interrupts = <5 (IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_EDGE_RISING)>;
+            debounce-delay-ms = <50>;
+            linux,keymap = <MATRIX_KEY(2, 0, KEY_F6)>,
+                           <MATRIX_KEY(3, 0, KEY_F8)>,
+                           <MATRIX_KEY(4, 0, KEY_F10)>,
+                           <MATRIX_KEY(5, 0, KEY_F4)>,
+                           <MATRIX_KEY(6, 0, KEY_F2)>,
+                           <MATRIX_KEY(2, 1, KEY_F5)>,
+                           <MATRIX_KEY(3, 1, KEY_F7)>,
+                           <MATRIX_KEY(4, 1, KEY_F9)>,
+                           <MATRIX_KEY(5, 1, KEY_F3)>,
+                           <MATRIX_KEY(6, 1, KEY_F1)>;
 
-                    led {
-                            color = <LED_COLOR_ID_RED>;
-                            function = LED_FUNCTION_BACKLIGHT;
-                            linux,default-trigger = "backlight";
-                    };
+            led {
+                color = <LED_COLOR_ID_RED>;
+                function = LED_FUNCTION_BACKLIGHT;
+                linux,default-trigger = "backlight";
             };
-      };
+        };
+    };
diff --git a/Documentation/devicetree/bindings/auxdisplay/img,ascii-lcd.yaml b/Documentation/devicetree/bindings/auxdisplay/img,ascii-lcd.yaml
index 1899b23de7d1..55e9831b3f67 100644
--- a/Documentation/devicetree/bindings/auxdisplay/img,ascii-lcd.yaml
+++ b/Documentation/devicetree/bindings/auxdisplay/img,ascii-lcd.yaml
@@ -50,6 +50,6 @@ additionalProperties: false
 examples:
   - |
     lcd: lcd@17fff000 {
-            compatible = "img,boston-lcd";
-            reg = <0x17fff000 0x8>;
+        compatible = "img,boston-lcd";
+        reg = <0x17fff000 0x8>;
     };
-- 
2.34.1

Re: [PATCH 2/3] dt-bindings: auxdisplay: adjust example indentation and use generic node names

[Andy Shevchenko]

On Mon, Feb 12, 2024 at 09:34:25AM +0100, Krzysztof Kozlowski wrote:
> The example DTS should be indented with two or four (preferred) spaces,
> as mentioned in Writing Schema document.  While re-indenting, change the
> node names to somehow generic names, as expected by Devicetree
> specification.

Thank you, it's helpful!
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

-- 
With Best Regards,
Andy Shevchenko

[PATCH 3/3] dt-bindings: auxdisplay: hit,hd44780: use defines for GPIO flags

[Krzysztof Kozlowski]

Improve example DTS readability by using known defines for GPIO flags.

Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 .../bindings/auxdisplay/hit,hd44780.yaml         | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/Documentation/devicetree/bindings/auxdisplay/hit,hd44780.yaml b/Documentation/devicetree/bindings/auxdisplay/hit,hd44780.yaml
index 1c7fd29bbcc7..869ae0f6cf5d 100644
--- a/Documentation/devicetree/bindings/auxdisplay/hit,hd44780.yaml
+++ b/Documentation/devicetree/bindings/auxdisplay/hit,hd44780.yaml
@@ -104,12 +104,12 @@ examples:
         compatible = "hit,hd44780";
         display-height-chars = <2>;
         display-width-chars = <16>;
-        data-gpios = <&pcf8574 4 0>,
-                     <&pcf8574 5 0>,
-                     <&pcf8574 6 0>,
-                     <&pcf8574 7 0>;
-        enable-gpios = <&pcf8574 2 0>;
-        rs-gpios = <&pcf8574 0 0>;
-        rw-gpios = <&pcf8574 1 0>;
-        backlight-gpios = <&pcf8574 3 0>;
+        data-gpios = <&pcf8574 4 GPIO_ACTIVE_HIGH>,
+                     <&pcf8574 5 GPIO_ACTIVE_HIGH>,
+                     <&pcf8574 6 GPIO_ACTIVE_HIGH>,
+                     <&pcf8574 7 GPIO_ACTIVE_HIGH>;
+        enable-gpios = <&pcf8574 2 GPIO_ACTIVE_HIGH>;
+        rs-gpios = <&pcf8574 0 GPIO_ACTIVE_HIGH>;
+        rw-gpios = <&pcf8574 1 GPIO_ACTIVE_HIGH>;
+        backlight-gpios = <&pcf8574 3 GPIO_ACTIVE_HIGH>;
     };
-- 
2.34.1

Re: [PATCH 3/3] dt-bindings: auxdisplay: hit,hd44780: use defines for GPIO flags

[Andy Shevchenko]

On Mon, Feb 12, 2024 at 09:34:26AM +0100, Krzysztof Kozlowski wrote:
> Improve example DTS readability by using known defines for GPIO flags.

Yes, it's better this way.
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/3] dt-bindings: auxdisplay: hit,hd44780: drop redundant GPIO node

[Geert Uytterhoeven]

Hi Krzysztof,

CC Ralf

On Mon, Feb 12, 2024 at 9:34 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
> Examples of other nodes, like GPIO controller, are redundant and not
> really needed in device bindings.
>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Thanks for your patch!

> --- a/Documentation/devicetree/bindings/auxdisplay/hit,hd44780.yaml
> +++ b/Documentation/devicetree/bindings/auxdisplay/hit,hd44780.yaml
> @@ -99,17 +99,7 @@ examples:
>      };
>    - |
>      #include <dt-bindings/gpio/gpio.h>
> -    i2c {
> -            #address-cells = <1>;
> -            #size-cells = <0>;
>
> -            pcf8574: pcf8574@27 {
> -                    compatible = "nxp,pcf8574";
> -                    reg = <0x27>;
> -                    gpio-controller;
> -                    #gpio-cells = <2>;
> -            };
> -    };

This part was added delberately in commit c784e46c8445635a ("auxdisplay:
Add I2C gpio expander example"), to aid makers who are not DT experts.
Note that there are no other examples of this popular wiring scheme
in upstream DTS.

>      hd44780 {
>              compatible = "hit,hd44780";
>              display-height-chars = <2>;

If you do want to insist on removing the i2c GPIO expander part,
I think this node should be removed too, as it is almost identical
to the first example.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/3] dt-bindings: auxdisplay: hit,hd44780: drop redundant GPIO node

[Krzysztof Kozlowski]

On 12/02/2024 09:41, Geert Uytterhoeven wrote:
> Thanks for your patch!
> 
>> --- a/Documentation/devicetree/bindings/auxdisplay/hit,hd44780.yaml
>> +++ b/Documentation/devicetree/bindings/auxdisplay/hit,hd44780.yaml
>> @@ -99,17 +99,7 @@ examples:
>>      };
>>    - |
>>      #include <dt-bindings/gpio/gpio.h>
>> -    i2c {
>> -            #address-cells = <1>;
>> -            #size-cells = <0>;
>>
>> -            pcf8574: pcf8574@27 {
>> -                    compatible = "nxp,pcf8574";
>> -                    reg = <0x27>;
>> -                    gpio-controller;
>> -                    #gpio-cells = <2>;
>> -            };
>> -    };
> 
> This part was added delberately in commit c784e46c8445635a ("auxdisplay:
> Add I2C gpio expander example"), to aid makers who are not DT experts.
> Note that there are no other examples of this popular wiring scheme
> in upstream DTS.

Hm, I don't understand how exactly it helps. The GPIO expander has its
own example and as you pointed below, this is basically the same code,
except rw and backlight GPIOs.


> 
>>      hd44780 {
>>              compatible = "hit,hd44780";
>>              display-height-chars = <2>;
> 
> If you do want to insist on removing the i2c GPIO expander part,
> I think this node should be removed too, as it is almost identical
> to the first example.
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 

Best regards,
Krzysztof

Re: [PATCH 1/3] dt-bindings: auxdisplay: hit, hd44780: drop redundant GPIO node

[Ralf Schlatterbeck]

On Mon, Feb 12, 2024 at 12:25:48PM +0100, Krzysztof Kozlowski wrote:
> 
> Hm, I don't understand how exactly it helps. The GPIO expander has its
> own example and as you pointed below, this is basically the same code,
> except rw and backlight GPIOs.

The hd44780 is a display that is very often used.
By people (like me some time ago) not familiar with the nice io expander
implementation in Linux. The consequence of that is that you'll find
several out-of-tree implementations for this display with i2c out in the
wild. So my thought of documenting this (again) at that location is to
make it easier for people with a hd44780 with the standard i2c interface
to see how it is done in Linux.

So I really think it helps. It would have helped me :-)

Ralf
-- 
Dr. Ralf Schlatterbeck                  Tel:   +43/2243/26465-16
Open Source Consulting                  www:   www.runtux.com
Reichergasse 131, A-3411 Weidling       email: office@runtux.com

Re: [PATCH 1/3] dt-bindings: auxdisplay: hit, hd44780: drop redundant GPIO node

[Krzysztof Kozlowski]

On 12/02/2024 12:58, Ralf Schlatterbeck wrote:
> On Mon, Feb 12, 2024 at 12:25:48PM +0100, Krzysztof Kozlowski wrote:
>>
>> Hm, I don't understand how exactly it helps. The GPIO expander has its
>> own example and as you pointed below, this is basically the same code,
>> except rw and backlight GPIOs.
> 
> The hd44780 is a display that is very often used.
> By people (like me some time ago) not familiar with the nice io expander
> implementation in Linux. The consequence of that is that you'll find
> several out-of-tree implementations for this display with i2c out in the
> wild. So my thought of documenting this (again) at that location is to
> make it easier for people with a hd44780 with the standard i2c interface
> to see how it is done in Linux.

GPIO expanders and their usage is nothing specific to this device -
other devices also might benefit of them. Or the SoCs which have enough
of GPIOs... I really do not understand why do we need expander here and
how does it help

Anyway, binding examples should not be collection of unrelated
solutions, because then we should accept for each device schema several
other variations and combinations.

Best regards,
Krzysztof

Re: [PATCH 1/3] dt-bindings: auxdisplay: hit, hd44780: drop redundant GPIO node

[Andy Shevchenko]

On Mon, Feb 12, 2024 at 02:38:27PM +0100, Krzysztof Kozlowski wrote:
> On 12/02/2024 12:58, Ralf Schlatterbeck wrote:

...

> Anyway, binding examples should not be collection of unrelated
> solutions, because then we should accept for each device schema several
> other variations and combinations.

Is this documented?

In any case, you would need to amend your series one way or another.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/3] dt-bindings: auxdisplay: hit, hd44780: drop redundant GPIO node

[Krzysztof Kozlowski]

On 12/02/2024 14:43, Andy Shevchenko wrote:
> On Mon, Feb 12, 2024 at 02:38:27PM +0100, Krzysztof Kozlowski wrote:
>> On 12/02/2024 12:58, Ralf Schlatterbeck wrote:
> 
> ...
> 
>> Anyway, binding examples should not be collection of unrelated
>> solutions, because then we should accept for each device schema several
>> other variations and combinations.
> 
> Is this documented?

Yes, writing schema says what the example is. We repeated it multiple
times on multiple reviews, we made multiple commits multiple times and I
briefly mentioned it also in my talks.

Best regards,
Krzysztof

Re: [PATCH 1/3] dt-bindings: auxdisplay: hit, hd44780: drop redundant GPIO node

[Andy Shevchenko]

On Mon, Feb 12, 2024 at 02:59:02PM +0100, Krzysztof Kozlowski wrote:
> On 12/02/2024 14:43, Andy Shevchenko wrote:
> > On Mon, Feb 12, 2024 at 02:38:27PM +0100, Krzysztof Kozlowski wrote:
> >> On 12/02/2024 12:58, Ralf Schlatterbeck wrote:

...

> >> Anyway, binding examples should not be collection of unrelated
> >> solutions, because then we should accept for each device schema several
> >> other variations and combinations.
> > 
> > Is this documented?
> 
> Yes, writing schema says what the example is. We repeated it multiple
> times on multiple reviews, we made multiple commits multiple times and I
> briefly mentioned it also in my talks.

Good to know, thanks!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/3] dt-bindings: auxdisplay: hit, hd44780: drop redundant GPIO node

[Rob Herring]

On Mon, Feb 12, 2024 at 02:59:02PM +0100, Krzysztof Kozlowski wrote:
> On 12/02/2024 14:43, Andy Shevchenko wrote:
> > On Mon, Feb 12, 2024 at 02:38:27PM +0100, Krzysztof Kozlowski wrote:
> >> On 12/02/2024 12:58, Ralf Schlatterbeck wrote:
> > 
> > ...
> > 
> >> Anyway, binding examples should not be collection of unrelated
> >> solutions, because then we should accept for each device schema several
> >> other variations and combinations.
> > 
> > Is this documented?
> 
> Yes, writing schema says what the example is. We repeated it multiple
> times on multiple reviews, we made multiple commits multiple times and I
> briefly mentioned it also in my talks.

While yes, this is the guidance, I think this case has provided enough 
justification to keep it. Let's move on please.

Rob

Re: [PATCH 1/3] dt-bindings: auxdisplay: hit, hd44780: drop redundant GPIO node

[Andy Shevchenko]

On Tue, Feb 13, 2024 at 10:19:05AM -0600, Rob Herring wrote:
> On Mon, Feb 12, 2024 at 02:59:02PM +0100, Krzysztof Kozlowski wrote:
> > On 12/02/2024 14:43, Andy Shevchenko wrote:
> > > On Mon, Feb 12, 2024 at 02:38:27PM +0100, Krzysztof Kozlowski wrote:
> > >> On 12/02/2024 12:58, Ralf Schlatterbeck wrote:

...

> > >> Anyway, binding examples should not be collection of unrelated
> > >> solutions, because then we should accept for each device schema several
> > >> other variations and combinations.
> > > 
> > > Is this documented?
> > 
> > Yes, writing schema says what the example is. We repeated it multiple
> > times on multiple reviews, we made multiple commits multiple times and I
> > briefly mentioned it also in my talks.
> 
> While yes, this is the guidance, I think this case has provided enough
> justification to keep it. Let's move on please.

Thank you, Rob.

Krzysztof, can you send v2, I'll apply it to the tree?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/3] dt-bindings: auxdisplay: hit, hd44780: drop redundant GPIO node

[Krzysztof Kozlowski]

On 13/02/2024 17:43, Andy Shevchenko wrote:
> On Tue, Feb 13, 2024 at 10:19:05AM -0600, Rob Herring wrote:
>> On Mon, Feb 12, 2024 at 02:59:02PM +0100, Krzysztof Kozlowski wrote:
>>> On 12/02/2024 14:43, Andy Shevchenko wrote:
>>>> On Mon, Feb 12, 2024 at 02:38:27PM +0100, Krzysztof Kozlowski wrote:
>>>>> On 12/02/2024 12:58, Ralf Schlatterbeck wrote:
> 
> ...
> 
>>>>> Anyway, binding examples should not be collection of unrelated
>>>>> solutions, because then we should accept for each device schema several
>>>>> other variations and combinations.
>>>>
>>>> Is this documented?
>>>
>>> Yes, writing schema says what the example is. We repeated it multiple
>>> times on multiple reviews, we made multiple commits multiple times and I
>>> briefly mentioned it also in my talks.
>>
>> While yes, this is the guidance, I think this case has provided enough
>> justification to keep it. Let's move on please.
> 
> Thank you, Rob.
> 
> Krzysztof, can you send v2, I'll apply it to the tree?

Sure.

Best regards,
Krzysztof

Re: [PATCH 1/3] dt-bindings: auxdisplay: hit, hd44780: drop redundant GPIO node

[Ralf Schlatterbeck]

On Mon, Feb 12, 2024 at 02:38:27PM +0100, Krzysztof Kozlowski wrote:
> On 12/02/2024 12:58, Ralf Schlatterbeck wrote:
> 
> GPIO expanders and their usage is nothing specific to this device -
> other devices also might benefit of them. Or the SoCs which have enough
> of GPIOs... I really do not understand why do we need expander here and
> how does it help

Can we then at least link the I/O Expander example to the docs of that
display?
I've documented my experience here:
https://blog.runtux.com/posts/2021/01/06/

And at the time there were two out-of-tree implementations.

Note that I think that redundancy in code is bad.
Not so for documentation.

Thanks + kind regards
Ralf
-- 
Dr. Ralf Schlatterbeck                  Tel:   +43/2243/26465-16
Open Source Consulting                  www:   www.runtux.com
Reichergasse 131, A-3411 Weidling       email: office@runtux.com

Re: [PATCH 1/3] dt-bindings: auxdisplay: hit, hd44780: drop redundant GPIO node

[Geert Uytterhoeven]

Hi Ralf,

On Mon, Feb 12, 2024 at 3:40 PM Ralf Schlatterbeck <rsc@runtux.com> wrote:
> On Mon, Feb 12, 2024 at 02:38:27PM +0100, Krzysztof Kozlowski wrote:
> > On 12/02/2024 12:58, Ralf Schlatterbeck wrote:
> > GPIO expanders and their usage is nothing specific to this device -
> > other devices also might benefit of them. Or the SoCs which have enough
> > of GPIOs... I really do not understand why do we need expander here and
> > how does it help
>
> Can we then at least link the I/O Expander example to the docs of that
> display?
> I've documented my experience here:
> https://blog.runtux.com/posts/2021/01/06/

Any chance you can update the DT overlays to use sugar syntax instead
of raw fragments, target{,-path} properties, and __overlay__ subnodes?

You can find examples in my DT overlay collection, e.g.
https://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-drivers.git/commit/?h=topic/renesas-overlays&id=3a41e34a2dd902c7bf74d11254a56190787252b6

> And at the time there were two out-of-tree implementations.

Only two? ;-)

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/3] dt-bindings: auxdisplay: hit, hd44780: drop redundant GPIO node

[Ralf Schlatterbeck]

On Mon, Feb 12, 2024 at 02:38:27PM +0100, Krzysztof Kozlowski wrote:
> On 12/02/2024 12:58, Ralf Schlatterbeck wrote:
> > On Mon, Feb 12, 2024 at 12:25:48PM +0100, Krzysztof Kozlowski wrote:
> >>
> >> Hm, I don't understand how exactly it helps. The GPIO expander has its
> >> own example and as you pointed below, this is basically the same code,
> >> except rw and backlight GPIOs.
> > 
> > The hd44780 is a display that is very often used.
> 
> GPIO expanders and their usage is nothing specific to this device -
> other devices also might benefit of them. Or the SoCs which have enough
> of GPIOs... I really do not understand why do we need expander here and
> how does it help

The hd44780 is most often sold together with that specific I/O expander.
The idea was to help people with that combination how to get their
device working.

> Anyway, binding examples should not be collection of unrelated
> solutions, because then we should accept for each device schema several
> other variations and combinations.

The solutions in that case are not unrelated because they document the
most-often-used hw combo.

I also didn't find any documentation of how to actually *use* the
pcf8575 I/O expander. Even
Documentation/devicetree/bindings/gpio/nxp,pcf8575.yaml
has only docs on how to instantiate the device on the i2c bus but not
how then to use the I/Os of the chip for something else.

So I'd ask again to not remove that piece of useful documentation.

And to get somehow philosophic:
I think that docs should be didactic, not optimized to the least
redundancy.

Thanks and kind regards,
Ralf
-- 
Dr. Ralf Schlatterbeck                  Tel:   +43/2243/26465-16
Open Source Consulting                  www:   www.runtux.com
Reichergasse 131, A-3411 Weidling       email: office@runtux.com

Re: [PATCH 1/3] dt-bindings: auxdisplay: hit,hd44780: drop redundant GPIO node

[Andy Shevchenko]

On Mon, Feb 12, 2024 at 09:34:24AM +0100, Krzysztof Kozlowski wrote:
> Examples of other nodes, like GPIO controller, are redundant and not
> really needed in device bindings.

...

> -    i2c {
> -            #address-cells = <1>;
> -            #size-cells = <0>;
>  
> -            pcf8574: pcf8574@27 {
> -                    compatible = "nxp,pcf8574";
> -                    reg = <0x27>;
> -                    gpio-controller;
> -                    #gpio-cells = <2>;
> -            };
> -    };

In patch 3 you updated the lines that have lost their sense due to this one.
And I agree with others, please leave this example in place.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/3] dt-bindings: auxdisplay: hit,hd44780: drop redundant GPIO node

[Krzysztof Kozlowski]

On 12/02/2024 14:39, Andy Shevchenko wrote:
> On Mon, Feb 12, 2024 at 09:34:24AM +0100, Krzysztof Kozlowski wrote:
>> Examples of other nodes, like GPIO controller, are redundant and not
>> really needed in device bindings.
> 
> ...
> 
>> -    i2c {
>> -            #address-cells = <1>;
>> -            #size-cells = <0>;
>>  
>> -            pcf8574: pcf8574@27 {
>> -                    compatible = "nxp,pcf8574";
>> -                    reg = <0x27>;
>> -                    gpio-controller;
>> -                    #gpio-cells = <2>;
>> -            };
>> -    };
> 
> In patch 3 you updated the lines that have lost their sense due to this one.

How did they lose it?


> And I agree with others, please leave this example in place.

What for? Why this binding is special and 99% of others do not need GPIO
expander in the example?

Best regards,
Krzysztof

Re: [PATCH 1/3] dt-bindings: auxdisplay: hit,hd44780: drop redundant GPIO node

[Andy Shevchenko]

On Mon, Feb 12, 2024 at 02:56:43PM +0100, Krzysztof Kozlowski wrote:
> On 12/02/2024 14:39, Andy Shevchenko wrote:
> > On Mon, Feb 12, 2024 at 09:34:24AM +0100, Krzysztof Kozlowski wrote:

...

> >> -    i2c {
> >> -            #address-cells = <1>;
> >> -            #size-cells = <0>;
> >>  
> >> -            pcf8574: pcf8574@27 {
> >> -                    compatible = "nxp,pcf8574";
> >> -                    reg = <0x27>;
> >> -                    gpio-controller;
> >> -                    #gpio-cells = <2>;
> >> -            };
> >> -    };
> > 
> > In patch 3 you updated the lines that have lost their sense due to this one.
> 
> How did they lose it?

Now they are referring to the non-existed node in the example. OTOH, there is
already hc595 case...

The Q here (as you pointed out that it's better to name nodes in generic way),
how these names are okay with the schema (hc595, pcf8574) as being referred to?

...

> > And I agree with others, please leave this example in place.
> 
> What for? Why this binding is special and 99% of others do not need GPIO
> expander in the example?

Some people already tried to explain you their point of view, but I see that:
- the unrelated nodes in the schemas are not welcome (as per your talks
  and documentation);
- the current file has other references that have no existing node in the
  example;
- you are DT maintainer, so I believe you know this better.

With this, I'm almost (see above question though) satisfied with the series.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/3] dt-bindings: auxdisplay: hit,hd44780: drop redundant GPIO node

[Krzysztof Kozlowski]

On 12/02/2024 15:09, Andy Shevchenko wrote:
> On Mon, Feb 12, 2024 at 02:56:43PM +0100, Krzysztof Kozlowski wrote:
>> On 12/02/2024 14:39, Andy Shevchenko wrote:
>>> On Mon, Feb 12, 2024 at 09:34:24AM +0100, Krzysztof Kozlowski wrote:
> 
> ...
> 
>>>> -    i2c {
>>>> -            #address-cells = <1>;
>>>> -            #size-cells = <0>;
>>>>  
>>>> -            pcf8574: pcf8574@27 {
>>>> -                    compatible = "nxp,pcf8574";
>>>> -                    reg = <0x27>;
>>>> -                    gpio-controller;
>>>> -                    #gpio-cells = <2>;
>>>> -            };
>>>> -    };
>>>
>>> In patch 3 you updated the lines that have lost their sense due to this one.
>>
>> How did they lose it?
> 
> Now they are referring to the non-existed node in the example. OTOH, there is
> already hc595 case...

All of the bindings examples do it. It's expected.

> 
> The Q here (as you pointed out that it's better to name nodes in generic way),
> how these names are okay with the schema (hc595, pcf8574) as being referred to?

They are not OK, although I don't see the name "hc595". There is phandle
to the hc595 label, but that's fine. Not a node name.

Best regards,
Krzysztof

Re: [PATCH 1/3] dt-bindings: auxdisplay: hit,hd44780: drop redundant GPIO node

[Andy Shevchenko]

On Mon, Feb 12, 2024 at 03:20:26PM +0100, Krzysztof Kozlowski wrote:
> On 12/02/2024 15:09, Andy Shevchenko wrote:
> > On Mon, Feb 12, 2024 at 02:56:43PM +0100, Krzysztof Kozlowski wrote:
> >> On 12/02/2024 14:39, Andy Shevchenko wrote:
> >>> On Mon, Feb 12, 2024 at 09:34:24AM +0100, Krzysztof Kozlowski wrote:

...

> >>>> -    i2c {
> >>>> -            #address-cells = <1>;
> >>>> -            #size-cells = <0>;
> >>>>  
> >>>> -            pcf8574: pcf8574@27 {
> >>>> -                    compatible = "nxp,pcf8574";
> >>>> -                    reg = <0x27>;
> >>>> -                    gpio-controller;
> >>>> -                    #gpio-cells = <2>;
> >>>> -            };
> >>>> -    };
> >>>
> >>> In patch 3 you updated the lines that have lost their sense due to this one.
> >>
> >> How did they lose it?
> > 
> > Now they are referring to the non-existed node in the example. OTOH, there is
> > already hc595 case...
> 
> All of the bindings examples do it. It's expected.
> 
> > 
> > The Q here (as you pointed out that it's better to name nodes in generic way),
> > how these names are okay with the schema (hc595, pcf8574) as being referred to?
> 
> They are not OK, although I don't see the name "hc595". There is phandle
> to the hc595 label, but that's fine. Not a node name.

Ah, okay, so it's a semantic difference. Thank you for your patience and elaboration!

-- 
With Best Regards,
Andy Shevchenko