| Message ID | a75c77fa8528f44832993f9780ae4ea409125a90.1585503354.git.hns@goldelico.com (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
| Series | MIPS: CI20: add HDMI out support | expand |
On Sun, 29 Mar 2020 19:35:47 +0200, "H. Nikolaus Schaller" wrote: > and add compatible: jz4780-lcd, including an example how to > configure both lcd controllers. > > Also fix the clock names and examples. > > Based on work by Paul Cercueil <paul@crapouillou.net> and > Sam Ravnborg <sam@ravnborg.org> > > Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> > Cc: Rob Herring <robh@kernel.org> > Cc: devicetree@vger.kernel.org > --- > .../bindings/display/ingenic,lcd.txt | 45 ------ > .../bindings/display/ingenic,lcd.yaml | 128 ++++++++++++++++++ > 2 files changed, 128 insertions(+), 45 deletions(-) > delete mode 100644 Documentation/devicetree/bindings/display/ingenic,lcd.txt > create mode 100644 Documentation/devicetree/bindings/display/ingenic,lcd.yaml > My bot found errors running 'make dt_binding_check' on your patch: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/display/ingenic,lcd.example.dt.yaml: lcd-controller@13050000: clocks: [[4294967295, 9]] is too short See https://patchwork.ozlabs.org/patch/1263508 If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure dt-schema is up to date: pip3 install git+https://github.com/devicetree-org/dt-schema.git@master --upgrade Please check and re-submit.
Hi Paul, > Am 30.03.2020 um 17:42 schrieb Rob Herring <robh@kernel.org>: > > On Sun, 29 Mar 2020 19:35:47 +0200, "H. Nikolaus Schaller" wrote: >> and add compatible: jz4780-lcd, including an example how to >> configure both lcd controllers. >> >> Also fix the clock names and examples. >> >> Based on work by Paul Cercueil <paul@crapouillou.net> and >> Sam Ravnborg <sam@ravnborg.org> >> >> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> >> Cc: Rob Herring <robh@kernel.org> >> Cc: devicetree@vger.kernel.org >> --- >> .../bindings/display/ingenic,lcd.txt | 45 ------ >> .../bindings/display/ingenic,lcd.yaml | 128 ++++++++++++++++++ >> 2 files changed, 128 insertions(+), 45 deletions(-) >> delete mode 100644 Documentation/devicetree/bindings/display/ingenic,lcd.txt >> create mode 100644 Documentation/devicetree/bindings/display/ingenic,lcd.yaml >> > > My bot found errors running 'make dt_binding_check' on your patch: > > /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/display/ingenic,lcd.example.dt.yaml: lcd-controller@13050000: clocks: [[4294967295, 9]] is too short > > See https://patchwork.ozlabs.org/patch/1263508 If I read the message correctly, I think there should be 2 clocks specified in the jz4725b-lcd example and not just clocks = <&cgu JZ4725B_CLK_LCD>; Unfortunately the jz4725b.dtsi does not seem to be upstream or in linux-next so I don't know if it works without lcd_pclk or not. If there is really just one clock, we need to modify the clocks and clock-names schema and add minItems: 1 and maxItems: 2 to allow for this flexibility. Otherwise we have to fix the example. Do you have some git with an up-to-date jz4725b.dtsi to look at? > > If you already ran 'make dt_binding_check' and didn't see the above > error(s), then make sure dt-schema is up to date: > > pip3 install git+https://github.com/devicetree-org/dt-schema.git@master --upgrade +++ :) > > Please check and re-submit. Sure, since it is a RFC. BR and thanks, Nikolaus
Hi Paul, > Am 30.03.2020 um 19:07 schrieb Paul Cercueil <paul@crapouillou.net>: > > > > Le lun. 30 mars 2020 à 18:48, H. Nikolaus Schaller <hns@goldelico.com> a écrit : >>> Am 30.03.2020 um 18:43 schrieb Paul Cercueil <paul@crapouillou.net>: >>> Hi Nikolaus, >>> Le lun. 30 mars 2020 à 18:33, H. Nikolaus Schaller <hns@goldelico.com> a écrit : >>>> Hi Paul, >>>>> Am 30.03.2020 um 17:42 schrieb Rob Herring <robh@kernel.org>: >>>>> On Sun, 29 Mar 2020 19:35:47 +0200, "H. Nikolaus Schaller" wrote: >>>>>> and add compatible: jz4780-lcd, including an example how to >>>>>> configure both lcd controllers. >>>>>> Also fix the clock names and examples. >>>>>> Based on work by Paul Cercueil <paul@crapouillou.net> and >>>>>> Sam Ravnborg <sam@ravnborg.org> >>>>>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> >>>>>> Cc: Rob Herring <robh@kernel.org> >>>>>> Cc: devicetree@vger.kernel.org >>>>>> --- >>>>>> .../bindings/display/ingenic,lcd.txt | 45 ------ >>>>>> .../bindings/display/ingenic,lcd.yaml | 128 ++++++++++++++++++ >>>>>> 2 files changed, 128 insertions(+), 45 deletions(-) >>>>>> delete mode 100644 Documentation/devicetree/bindings/display/ingenic,lcd.txt >>>>>> create mode 100644 Documentation/devicetree/bindings/display/ingenic,lcd.yaml >>>>> My bot found errors running 'make dt_binding_check' on your patch: >>>>> /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/display/ingenic,lcd.example.dt.yaml: lcd-controller@13050000: clocks: [[4294967295, 9]] is too short >>>>> See https://patchwork.ozlabs.org/patch/1263508 >>>> If I read the message correctly, I think there should be 2 clocks specified in >>>> the jz4725b-lcd example and not just >>>> clocks = <&cgu JZ4725B_CLK_LCD>; >>> You get this message because that's what the YAML says. There is only one clock on JZ4725B so the yaml should reflect that. >> That was the missing piece of information that there is only one clock on the jz4780. >> Anyways, is there some example? Does some jz4725b.dtsi exist somewhere? >> It is difficult to write correct yaml for a device that is not upstream. > > https://github.com/OpenDingux/linux/blob/jz-5.5-rs90/arch/mips/boot/dts/ingenic/jz4725b.dtsi#L331 Great! Ah, it only requires the "lcd_pclk" (and not the "lcd" clock) which means the original ingenic,lcd.txt wasn't perfect in this case either. I'll queue it for the next RFC version. So the next important thing for this series is that we find help for making the CI20 HDMI work... BR and thanks, Nikolaus > > -Paul > >> Otherwise I'd propose to drop the jz4725b and use the jz4740 as example. >> BR and thanks, >> Nikolaus >>> -Paul >>>> Unfortunately the jz4725b.dtsi does not seem to be upstream or in linux-next so >>>> I don't know if it works without lcd_pclk or not. >>>> If there is really just one clock, we need to modify the clocks and clock-names >>>> schema and add minItems: 1 and maxItems: 2 to allow for this flexibility. >>>> Otherwise we have to fix the example. Do you have some git with an up-to-date >>>> jz4725b.dtsi to look at? >>>>> If you already ran 'make dt_binding_check' and didn't see the above >>>>> error(s), then make sure dt-schema is up to date: >>>>> pip3 install git+https://github.com/devicetree-org/dt-schema.git@master --upgrade >>>> +++ :) >>>>> Please check and re-submit. >>>> Sure, since it is a RFC. >>>> BR and thanks, >>>> Nikolaus
Hi Nikolaus Some comments below that will result in a simplee binding that passes the checks. Thanks for pushing this. Sam On Sun, Mar 29, 2020 at 07:35:47PM +0200, H. Nikolaus Schaller wrote: > and add compatible: jz4780-lcd, including an example how to > configure both lcd controllers. > > Also fix the clock names and examples. > > Based on work by Paul Cercueil <paul@crapouillou.net> and > Sam Ravnborg <sam@ravnborg.org> > > Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> > Cc: Rob Herring <robh@kernel.org> > Cc: devicetree@vger.kernel.org > --- > .../bindings/display/ingenic,lcd.txt | 45 ------ > .../bindings/display/ingenic,lcd.yaml | 128 ++++++++++++++++++ > 2 files changed, 128 insertions(+), 45 deletions(-) > delete mode 100644 Documentation/devicetree/bindings/display/ingenic,lcd.txt > create mode 100644 Documentation/devicetree/bindings/display/ingenic,lcd.yaml > > diff --git a/Documentation/devicetree/bindings/display/ingenic,lcd.txt b/Documentation/devicetree/bindings/display/ingenic,lcd.txt > deleted file mode 100644 > index 01e3261defb6..000000000000 > --- a/Documentation/devicetree/bindings/display/ingenic,lcd.txt > +++ /dev/null > @@ -1,45 +0,0 @@ > -Ingenic JZ47xx LCD driver > - > -Required properties: > -- compatible: one of: > - * ingenic,jz4740-lcd > - * ingenic,jz4725b-lcd > - * ingenic,jz4770-lcd > -- reg: LCD registers location and length > -- clocks: LCD pixclock and device clock specifiers. > - The device clock is only required on the JZ4740. > -- clock-names: "lcd_pclk" and "lcd" > -- interrupts: Specifies the interrupt line the LCD controller is connected to. > - > -Example: > - > -panel { > - compatible = "sharp,ls020b1dd01d"; > - > - backlight = <&backlight>; > - power-supply = <&vcc>; > - > - port { > - panel_input: endpoint { > - remote-endpoint = <&panel_output>; > - }; > - }; > -}; > - > - > -lcd: lcd-controller@13050000 { > - compatible = "ingenic,jz4725b-lcd"; > - reg = <0x13050000 0x1000>; > - > - interrupt-parent = <&intc>; > - interrupts = <31>; > - > - clocks = <&cgu JZ4725B_CLK_LCD>; > - clock-names = "lcd"; > - > - port { > - panel_output: endpoint { > - remote-endpoint = <&panel_input>; > - }; > - }; > -}; > diff --git a/Documentation/devicetree/bindings/display/ingenic,lcd.yaml b/Documentation/devicetree/bindings/display/ingenic,lcd.yaml > new file mode 100644 > index 000000000000..8b6467cfc191 > --- /dev/null > +++ b/Documentation/devicetree/bindings/display/ingenic,lcd.yaml > @@ -0,0 +1,128 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/display/ingenic,lcd.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Bindings for Ingenic JZ4780 LCD Controller > + > +maintainers: > + - Paul Cercueil <paul@crapouillou.net> > + > +description: | > + LCD Controller is the Display Controller for the Ingenic JZ47xx SoC > + > +properties: > + compatible: > + oneOf: > + - const: ingenic,jz4725b-lcd > + - const: ingenic,jz4740-lcd > + - const: ingenic,jz4770-lcd > + - const: ingenic,jz4780-lcd > + > + reg: > + maxItems: 1 > + description: LCD registers location and length > + > + interrupts: > + maxItems: 1 > + description: Specifies the interrupt provided by parent > + > + clocks: > + maxItems: 2 > + description: Clock specifiers for LCD pixclock and device clock. > + The device clock is only required on the JZ4740 and JZ4780 > + > + clock-names: > + items: > + - const: lcd > + - const: lcd_pclk > + > + port: > + type: object > + description: | > + A port node with endpoint definitions as defined in > + Documentation/devicetree/bindings/media/video-interfaces.txt > + > +required: > + - compatible > + - reg > + - interrupts > + - clocks > + - clock-names > + - port > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/clock/jz4725b-cgu.h> > + > + panel { > + compatible = "sharp,ls020b1dd01d"; > + > + backlight = <&backlight>; > + power-supply = <&vcc>; > + > + port { > + panel_input: endpoint { > + remote-endpoint = <&panel_output>; > + }; > + }; > + }; The panel part is not needed - better to drop it. > + > + lcd: lcd-controller@13050000 { > + compatible = "ingenic,jz4725b-lcd"; > + reg = <0x13050000 0x1000>; > + > + interrupt-parent = <&intc>; > + interrupts = <31>; > + > + clocks = <&cgu JZ4725B_CLK_LCD>; > + clock-names = "lcd", "lcd_pclk"; > + > + port { > + panel_output: endpoint { > + remote-endpoint = <&panel_input>; > + }; > + }; > + }; We know this example will not pass the check, as there is only one clock specified. I suggest to drop this example. If it later turns out that jz4725b only have one clock, then the binding needs to be updated. But the best guess is that the example is wrong. The example below for jz4780-lcd cover all relevant parts - so just keep it as the only example. > + > + - | > + #include <dt-bindings/clock/jz4780-cgu.h> > + > + lcdc0: lcdc0@13050000 { Name this lcdc And drop "lcdc0@13050000" as this is not relevant for this example. Remember - the examples exist to explain the binding. They are just examples. > + compatible = "ingenic,jz4780-lcd"; > + reg = <0x13050000 0x1800>; > + > + clocks = <&cgu JZ4780_CLK_TVE>, <&cgu JZ4780_CLK_LCD0PIXCLK>; > + clock-names = "lcd", "lcd_pclk"; > + > + interrupt-parent = <&intc>; > + interrupts = <31>; > + > + jz4780_lcd_out: port { > + #address-cells = <1>; > + #size-cells = <0>; > + > + jz4780_out_hdmi: endpoint@0 { > + reg = <0>; > + remote-endpoint = <&hdmi_in_lcd>; > + }; > + }; > + }; > + And drop this as it does not add anything extra. > + lcdc1: lcdc1@130a0000 { > + compatible = "ingenic,jz4780-lcd"; > + reg = <0x130a0000 0x1800>; > + > + clocks = <&cgu JZ4780_CLK_TVE>, <&cgu JZ4780_CLK_LCD1PIXCLK>; > + clock-names = "lcd", "lcd_pclk"; > + > + interrupt-parent = <&intc>; > + interrupts = <31>; > + > + status = "disabled"; > + }; Sam
Hi Sam, > Am 09.04.2020 um 09:25 schrieb Sam Ravnborg <sam@ravnborg.org>: > > Hi Nikolaus > > > Some comments below that will result in a simplee binding that passes > the checks. > Thanks for pushing this. > > Sam > > On Sun, Mar 29, 2020 at 07:35:47PM +0200, H. Nikolaus Schaller wrote: >> and add compatible: jz4780-lcd, including an example how to >> configure both lcd controllers. >> >> Also fix the clock names and examples. >> >> Based on work by Paul Cercueil <paul@crapouillou.net> and >> Sam Ravnborg <sam@ravnborg.org> >> >> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> >> Cc: Rob Herring <robh@kernel.org> >> Cc: devicetree@vger.kernel.org >> --- >> .../bindings/display/ingenic,lcd.txt | 45 ------ >> .../bindings/display/ingenic,lcd.yaml | 128 ++++++++++++++++++ >> 2 files changed, 128 insertions(+), 45 deletions(-) >> delete mode 100644 Documentation/devicetree/bindings/display/ingenic,lcd.txt >> create mode 100644 Documentation/devicetree/bindings/display/ingenic,lcd.yaml >> >> diff --git a/Documentation/devicetree/bindings/display/ingenic,lcd.txt b/Documentation/devicetree/bindings/display/ingenic,lcd.txt >> deleted file mode 100644 >> index 01e3261defb6..000000000000 >> --- a/Documentation/devicetree/bindings/display/ingenic,lcd.txt >> +++ /dev/null >> @@ -1,45 +0,0 @@ >> -Ingenic JZ47xx LCD driver >> - >> -Required properties: >> -- compatible: one of: >> - * ingenic,jz4740-lcd >> - * ingenic,jz4725b-lcd >> - * ingenic,jz4770-lcd >> -- reg: LCD registers location and length >> -- clocks: LCD pixclock and device clock specifiers. >> - The device clock is only required on the JZ4740. >> -- clock-names: "lcd_pclk" and "lcd" >> -- interrupts: Specifies the interrupt line the LCD controller is connected to. >> - >> -Example: >> - >> -panel { >> - compatible = "sharp,ls020b1dd01d"; >> - >> - backlight = <&backlight>; >> - power-supply = <&vcc>; >> - >> - port { >> - panel_input: endpoint { >> - remote-endpoint = <&panel_output>; >> - }; >> - }; >> -}; >> - >> - >> -lcd: lcd-controller@13050000 { >> - compatible = "ingenic,jz4725b-lcd"; >> - reg = <0x13050000 0x1000>; >> - >> - interrupt-parent = <&intc>; >> - interrupts = <31>; >> - >> - clocks = <&cgu JZ4725B_CLK_LCD>; >> - clock-names = "lcd"; >> - >> - port { >> - panel_output: endpoint { >> - remote-endpoint = <&panel_input>; >> - }; >> - }; >> -}; >> diff --git a/Documentation/devicetree/bindings/display/ingenic,lcd.yaml b/Documentation/devicetree/bindings/display/ingenic,lcd.yaml >> new file mode 100644 >> index 000000000000..8b6467cfc191 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/display/ingenic,lcd.yaml >> @@ -0,0 +1,128 @@ >> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/display/ingenic,lcd.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Bindings for Ingenic JZ4780 LCD Controller >> + >> +maintainers: >> + - Paul Cercueil <paul@crapouillou.net> >> + >> +description: | >> + LCD Controller is the Display Controller for the Ingenic JZ47xx SoC >> + >> +properties: >> + compatible: >> + oneOf: >> + - const: ingenic,jz4725b-lcd >> + - const: ingenic,jz4740-lcd >> + - const: ingenic,jz4770-lcd >> + - const: ingenic,jz4780-lcd >> + >> + reg: >> + maxItems: 1 >> + description: LCD registers location and length >> + >> + interrupts: >> + maxItems: 1 >> + description: Specifies the interrupt provided by parent >> + >> + clocks: >> + maxItems: 2 >> + description: Clock specifiers for LCD pixclock and device clock. >> + The device clock is only required on the JZ4740 and JZ4780 >> + >> + clock-names: >> + items: >> + - const: lcd >> + - const: lcd_pclk >> + >> + port: >> + type: object >> + description: | >> + A port node with endpoint definitions as defined in >> + Documentation/devicetree/bindings/media/video-interfaces.txt >> + >> +required: >> + - compatible >> + - reg >> + - interrupts >> + - clocks >> + - clock-names >> + - port >> + >> +additionalProperties: false >> + >> +examples: >> + - | >> + #include <dt-bindings/clock/jz4725b-cgu.h> >> + >> + panel { >> + compatible = "sharp,ls020b1dd01d"; >> + >> + backlight = <&backlight>; >> + power-supply = <&vcc>; >> + >> + port { >> + panel_input: endpoint { >> + remote-endpoint = <&panel_output>; >> + }; >> + }; >> + }; > The panel part is not needed - better to drop it. Well, it is needed to fulfill the remote-endpoint below. > > >> + >> + lcd: lcd-controller@13050000 { >> + compatible = "ingenic,jz4725b-lcd"; >> + reg = <0x13050000 0x1000>; >> + >> + interrupt-parent = <&intc>; >> + interrupts = <31>; >> + >> + clocks = <&cgu JZ4725B_CLK_LCD>; >> + clock-names = "lcd", "lcd_pclk"; >> + >> + port { >> + panel_output: endpoint { >> + remote-endpoint = <&panel_input>; >> + }; >> + }; >> + }; > We know this example will not pass the check, as there is only > one clock specified. > I suggest to drop this example. > If it later turns out that jz4725b only have one clock, Paul already reported that it only wants to see one clock. > then the binding > needs to be updated. Yes, I have that on my to-do list to update the binding to reflect this minItems/maxItems thing but I am not yet sure about how to handle the clock-names in that case. I.e. make "lcd" optional and enforce "lcd_pclk" only. > But the best guess is that the example is wrong. > > The example below for jz4780-lcd cover all relevant parts - so > just keep it as the only example. > >> + >> + - | >> + #include <dt-bindings/clock/jz4780-cgu.h> >> + >> + lcdc0: lcdc0@13050000 { > Name this lcdc > And drop "lcdc0@13050000" as this is not relevant for this example. > > Remember - the examples exist to explain the binding. They are > just examples. > >> + compatible = "ingenic,jz4780-lcd"; >> + reg = <0x13050000 0x1800>; >> + >> + clocks = <&cgu JZ4780_CLK_TVE>, <&cgu JZ4780_CLK_LCD0PIXCLK>; >> + clock-names = "lcd", "lcd_pclk"; >> + >> + interrupt-parent = <&intc>; >> + interrupts = <31>; >> + >> + jz4780_lcd_out: port { >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + jz4780_out_hdmi: endpoint@0 { >> + reg = <0>; >> + remote-endpoint = <&hdmi_in_lcd>; >> + }; >> + }; >> + }; >> + > > And drop this as it does not add anything extra. Well, it demonstrates how to add a second lcdc which is disabled. Showing that it is possible to do so is IMHO the most important part of the example because it is not at all obvious. I have also added both SoC to show how differently they can and should be. >> + lcdc1: lcdc1@130a0000 { >> + compatible = "ingenic,jz4780-lcd"; >> + reg = <0x130a0000 0x1800>; >> + >> + clocks = <&cgu JZ4780_CLK_TVE>, <&cgu JZ4780_CLK_LCD1PIXCLK>; >> + clock-names = "lcd", "lcd_pclk"; >> + >> + interrupt-parent = <&intc>; >> + interrupts = <31>; >> + >> + status = "disabled"; >> + }; > > Sam BR and thanks, Nikolaus
Hi Nikolaus. > >> + > >> +examples: > >> + - | > >> + #include <dt-bindings/clock/jz4725b-cgu.h> > >> + > >> + panel { > >> + compatible = "sharp,ls020b1dd01d"; > >> + > >> + backlight = <&backlight>; > >> + power-supply = <&vcc>; > >> + > >> + port { > >> + panel_input: endpoint { > >> + remote-endpoint = <&panel_output>; > >> + }; > >> + }; > >> + }; > > The panel part is not needed - better to drop it. > > Well, it is needed to fulfill the remote-endpoint below. Examples may have phandle that are not defined. So the example will work fine without it. See other similar examples. > > > > > > >> + > >> + lcd: lcd-controller@13050000 { > >> + compatible = "ingenic,jz4725b-lcd"; > >> + reg = <0x13050000 0x1000>; > >> + > >> + interrupt-parent = <&intc>; > >> + interrupts = <31>; > >> + > >> + clocks = <&cgu JZ4725B_CLK_LCD>; > >> + clock-names = "lcd", "lcd_pclk"; > >> + > >> + port { > >> + panel_output: endpoint { > >> + remote-endpoint = <&panel_input>; > >> + }; > >> + }; > >> + }; > > We know this example will not pass the check, as there is only > > one clock specified. > > I suggest to drop this example. > > If it later turns out that jz4725b only have one clock, > > Paul already reported that it only wants to see one clock. > > > then the binding > > needs to be updated. > > Yes, I have that on my to-do list to update the binding to reflect > this minItems/maxItems thing but I am not yet sure about how > to handle the clock-names in that case. I.e. make "lcd" optional > and enforce "lcd_pclk" only. Look forward to next version. > > > But the best guess is that the example is wrong. > > > > The example below for jz4780-lcd cover all relevant parts - so > > just keep it as the only example. > > > >> + > >> + - | > >> + #include <dt-bindings/clock/jz4780-cgu.h> > >> + > >> + lcdc0: lcdc0@13050000 { > > Name this lcdc > > And drop "lcdc0@13050000" as this is not relevant for this example. > > > > Remember - the examples exist to explain the binding. They are > > just examples. > > > >> + compatible = "ingenic,jz4780-lcd"; > >> + reg = <0x13050000 0x1800>; > >> + > >> + clocks = <&cgu JZ4780_CLK_TVE>, <&cgu JZ4780_CLK_LCD0PIXCLK>; > >> + clock-names = "lcd", "lcd_pclk"; > >> + > >> + interrupt-parent = <&intc>; > >> + interrupts = <31>; > >> + > >> + jz4780_lcd_out: port { > >> + #address-cells = <1>; > >> + #size-cells = <0>; > >> + > >> + jz4780_out_hdmi: endpoint@0 { > >> + reg = <0>; > >> + remote-endpoint = <&hdmi_in_lcd>; > >> + }; > >> + }; > >> + }; > >> + > > > > And drop this as it does not add anything extra. > > Well, it demonstrates how to add a second lcdc which is disabled. The purpose of the example is to show an example of the binding specified in this file. Adding a second disabled lcdc is a general thing, and not something we want in all the individual examples. Also the actual content, for example register values can be random as they are not part of the binding. This is not a documentation of the HW but a binding example. Sam > > Showing that it is possible to do so is IMHO the most important > part of the example because it is not at all obvious. > > I have also added both SoC to show how differently they can > and should be. > > >> + lcdc1: lcdc1@130a0000 { > >> + compatible = "ingenic,jz4780-lcd"; > >> + reg = <0x130a0000 0x1800>; > >> + > >> + clocks = <&cgu JZ4780_CLK_TVE>, <&cgu JZ4780_CLK_LCD1PIXCLK>; > >> + clock-names = "lcd", "lcd_pclk"; > >> + > >> + interrupt-parent = <&intc>; > >> + interrupts = <31>; > >> + > >> + status = "disabled"; > >> + }; > > > > Sam > > BR and thanks, > Nikolaus >
Hi Sam, > Am 09.04.2020 um 14:20 schrieb Sam Ravnborg <sam@ravnborg.org>: > > Hi Nikolaus. > >>>> + >>>> +examples: >>>> + - | >>>> + #include <dt-bindings/clock/jz4725b-cgu.h> >>>> + >>>> + panel { >>>> + compatible = "sharp,ls020b1dd01d"; >>>> + >>>> + backlight = <&backlight>; >>>> + power-supply = <&vcc>; >>>> + >>>> + port { >>>> + panel_input: endpoint { >>>> + remote-endpoint = <&panel_output>; >>>> + }; >>>> + }; >>>> + }; >>> The panel part is not needed - better to drop it. >> >> Well, it is needed to fulfill the remote-endpoint below. > > Examples may have phandle that are not defined. > So the example will work fine without it. > See other similar examples. Ok. > >> >>> >>> >>>> + >>>> + lcd: lcd-controller@13050000 { >>>> + compatible = "ingenic,jz4725b-lcd"; >>>> + reg = <0x13050000 0x1000>; >>>> + >>>> + interrupt-parent = <&intc>; >>>> + interrupts = <31>; >>>> + >>>> + clocks = <&cgu JZ4725B_CLK_LCD>; >>>> + clock-names = "lcd", "lcd_pclk"; >>>> + >>>> + port { >>>> + panel_output: endpoint { >>>> + remote-endpoint = <&panel_input>; >>>> + }; >>>> + }; >>>> + }; >>> We know this example will not pass the check, as there is only >>> one clock specified. >>> I suggest to drop this example. >>> If it later turns out that jz4725b only have one clock, >> >> Paul already reported that it only wants to see one clock. >> >>> then the binding >>> needs to be updated. >> >> Yes, I have that on my to-do list to update the binding to reflect >> this minItems/maxItems thing but I am not yet sure about how >> to handle the clock-names in that case. I.e. make "lcd" optional >> and enforce "lcd_pclk" only. > Look forward to next version. > >> >>> But the best guess is that the example is wrong. >>> >>> The example below for jz4780-lcd cover all relevant parts - so >>> just keep it as the only example. >>> >>>> + >>>> + - | >>>> + #include <dt-bindings/clock/jz4780-cgu.h> >>>> + >>>> + lcdc0: lcdc0@13050000 { >>> Name this lcdc >>> And drop "lcdc0@13050000" as this is not relevant for this example. >>> >>> Remember - the examples exist to explain the binding. They are >>> just examples. >>> >>>> + compatible = "ingenic,jz4780-lcd"; >>>> + reg = <0x13050000 0x1800>; >>>> + >>>> + clocks = <&cgu JZ4780_CLK_TVE>, <&cgu JZ4780_CLK_LCD0PIXCLK>; >>>> + clock-names = "lcd", "lcd_pclk"; >>>> + >>>> + interrupt-parent = <&intc>; >>>> + interrupts = <31>; >>>> + >>>> + jz4780_lcd_out: port { >>>> + #address-cells = <1>; >>>> + #size-cells = <0>; >>>> + >>>> + jz4780_out_hdmi: endpoint@0 { >>>> + reg = <0>; >>>> + remote-endpoint = <&hdmi_in_lcd>; >>>> + }; >>>> + }; >>>> + }; >>>> + >>> >>> And drop this as it does not add anything extra. >> >> Well, it demonstrates how to add a second lcdc which is disabled. > The purpose of the example is to show an example of the > binding specified in this file. > Adding a second disabled lcdc is a general thing, and not > something we want in all the individual examples. This is contrary to what I have expected. My assumption is that the example is some piece of code that you (and me and other readers of the bindings documentation) can easily understand and even copy&paste into a DTS. Like an example in a training book. This is how I always have used the .txt bindings when writing new DTS. This ease of use is lost if the examples are incomplete and show only part of what is needed to get a working DTS. If not here, where can one get the required information from? Or do you see this as unit-test-cases for the formal bindings definitions? Then, it should IMHO not be named "example". > Also the actual content, for example register values can be > random as they are not part of the binding. > This is not a documentation of the HW but a binding example. Here it is the bindings for a very specific SoC and the register values are well defined and there is no choice at all. So I'll simplify it for v4 although I don't like the concept behind. BR and thanks, Nikolaus > > Sam > >> >> Showing that it is possible to do so is IMHO the most important >> part of the example because it is not at all obvious. >> >> I have also added both SoC to show how differently they can >> and should be. >> >>>> + lcdc1: lcdc1@130a0000 { >>>> + compatible = "ingenic,jz4780-lcd"; >>>> + reg = <0x130a0000 0x1800>; >>>> + >>>> + clocks = <&cgu JZ4780_CLK_TVE>, <&cgu JZ4780_CLK_LCD1PIXCLK>; >>>> + clock-names = "lcd", "lcd_pclk"; >>>> + >>>> + interrupt-parent = <&intc>; >>>> + interrupts = <31>; >>>> + >>>> + status = "disabled"; >>>> + }; >>> >>> Sam >> >> BR and thanks, >> Nikolaus >>
diff --git a/Documentation/devicetree/bindings/display/ingenic,lcd.txt b/Documentation/devicetree/bindings/display/ingenic,lcd.txt deleted file mode 100644 index 01e3261defb6..000000000000 --- a/Documentation/devicetree/bindings/display/ingenic,lcd.txt +++ /dev/null @@ -1,45 +0,0 @@ -Ingenic JZ47xx LCD driver - -Required properties: -- compatible: one of: - * ingenic,jz4740-lcd - * ingenic,jz4725b-lcd - * ingenic,jz4770-lcd -- reg: LCD registers location and length -- clocks: LCD pixclock and device clock specifiers. - The device clock is only required on the JZ4740. -- clock-names: "lcd_pclk" and "lcd" -- interrupts: Specifies the interrupt line the LCD controller is connected to. - -Example: - -panel { - compatible = "sharp,ls020b1dd01d"; - - backlight = <&backlight>; - power-supply = <&vcc>; - - port { - panel_input: endpoint { - remote-endpoint = <&panel_output>; - }; - }; -}; - - -lcd: lcd-controller@13050000 { - compatible = "ingenic,jz4725b-lcd"; - reg = <0x13050000 0x1000>; - - interrupt-parent = <&intc>; - interrupts = <31>; - - clocks = <&cgu JZ4725B_CLK_LCD>; - clock-names = "lcd"; - - port { - panel_output: endpoint { - remote-endpoint = <&panel_input>; - }; - }; -}; diff --git a/Documentation/devicetree/bindings/display/ingenic,lcd.yaml b/Documentation/devicetree/bindings/display/ingenic,lcd.yaml new file mode 100644 index 000000000000..8b6467cfc191 --- /dev/null +++ b/Documentation/devicetree/bindings/display/ingenic,lcd.yaml @@ -0,0 +1,128 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/display/ingenic,lcd.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Bindings for Ingenic JZ4780 LCD Controller + +maintainers: + - Paul Cercueil <paul@crapouillou.net> + +description: | + LCD Controller is the Display Controller for the Ingenic JZ47xx SoC + +properties: + compatible: + oneOf: + - const: ingenic,jz4725b-lcd + - const: ingenic,jz4740-lcd + - const: ingenic,jz4770-lcd + - const: ingenic,jz4780-lcd + + reg: + maxItems: 1 + description: LCD registers location and length + + interrupts: + maxItems: 1 + description: Specifies the interrupt provided by parent + + clocks: + maxItems: 2 + description: Clock specifiers for LCD pixclock and device clock. + The device clock is only required on the JZ4740 and JZ4780 + + clock-names: + items: + - const: lcd + - const: lcd_pclk + + port: + type: object + description: | + A port node with endpoint definitions as defined in + Documentation/devicetree/bindings/media/video-interfaces.txt + +required: + - compatible + - reg + - interrupts + - clocks + - clock-names + - port + +additionalProperties: false + +examples: + - | + #include <dt-bindings/clock/jz4725b-cgu.h> + + panel { + compatible = "sharp,ls020b1dd01d"; + + backlight = <&backlight>; + power-supply = <&vcc>; + + port { + panel_input: endpoint { + remote-endpoint = <&panel_output>; + }; + }; + }; + + lcd: lcd-controller@13050000 { + compatible = "ingenic,jz4725b-lcd"; + reg = <0x13050000 0x1000>; + + interrupt-parent = <&intc>; + interrupts = <31>; + + clocks = <&cgu JZ4725B_CLK_LCD>; + clock-names = "lcd", "lcd_pclk"; + + port { + panel_output: endpoint { + remote-endpoint = <&panel_input>; + }; + }; + }; + + - | + #include <dt-bindings/clock/jz4780-cgu.h> + + lcdc0: lcdc0@13050000 { + compatible = "ingenic,jz4780-lcd"; + reg = <0x13050000 0x1800>; + + clocks = <&cgu JZ4780_CLK_TVE>, <&cgu JZ4780_CLK_LCD0PIXCLK>; + clock-names = "lcd", "lcd_pclk"; + + interrupt-parent = <&intc>; + interrupts = <31>; + + jz4780_lcd_out: port { + #address-cells = <1>; + #size-cells = <0>; + + jz4780_out_hdmi: endpoint@0 { + reg = <0>; + remote-endpoint = <&hdmi_in_lcd>; + }; + }; + }; + + lcdc1: lcdc1@130a0000 { + compatible = "ingenic,jz4780-lcd"; + reg = <0x130a0000 0x1800>; + + clocks = <&cgu JZ4780_CLK_TVE>, <&cgu JZ4780_CLK_LCD1PIXCLK>; + clock-names = "lcd", "lcd_pclk"; + + interrupt-parent = <&intc>; + interrupts = <31>; + + status = "disabled"; + }; + +...
and add compatible: jz4780-lcd, including an example how to configure both lcd controllers. Also fix the clock names and examples. Based on work by Paul Cercueil <paul@crapouillou.net> and Sam Ravnborg <sam@ravnborg.org> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> Cc: Rob Herring <robh@kernel.org> Cc: devicetree@vger.kernel.org --- .../bindings/display/ingenic,lcd.txt | 45 ------ .../bindings/display/ingenic,lcd.yaml | 128 ++++++++++++++++++ 2 files changed, 128 insertions(+), 45 deletions(-) delete mode 100644 Documentation/devicetree/bindings/display/ingenic,lcd.txt create mode 100644 Documentation/devicetree/bindings/display/ingenic,lcd.yaml