| Message ID | 20240212083426.26757-1-krzysztof.kozlowski@linaro.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [1/3] dt-bindings: auxdisplay: hit,hd44780: drop redundant GPIO node | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
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
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
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
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
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.
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.
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
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
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!
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.
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
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!
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
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
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
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
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?
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
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>;
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(-)