| Message ID | 20230621133215.109254-3-tanure@linux.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Add Amlogic A311D2 and Khadas Vim4 Board Support | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
On 21/06/2023 15:32, Lucas Tanure wrote: > Amlogic T7 SoCs uses the same UART controller as S4 SoCs and G12A. > There is no need for an extra compatible line in the driver, but > add T7 compatible line for documentation. > > Signed-off-by: Lucas Tanure <tanure@linux.com> > --- > .../devicetree/bindings/serial/amlogic,meson-uart.yaml | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml b/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml > index 01ec45b3b406..860ab58d87b0 100644 > --- a/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml > +++ b/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml > @@ -33,6 +33,7 @@ properties: > - amlogic,meson8b-uart > - amlogic,meson-gx-uart > - amlogic,meson-s4-uart > + - amlogic,meson-t7-uart > - const: amlogic,meson-ao-uart > - description: Always-on power domain UART controller on G12A SoCs > items: > @@ -46,6 +47,7 @@ properties: > - amlogic,meson8b-uart > - amlogic,meson-gx-uart > - amlogic,meson-s4-uart > + - amlogic,meson-t7-uart It does not look like you tested the DTS against bindings. Please run `make dtbs_check` (see Documentation/devicetree/bindings/writing-schema.rst or https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/ for instructions). Best regards, Krzysztof
Lucas, On Wed, Jun 21, 2023 at 03:53:04PM +0200, Krzysztof Kozlowski wrote: > On 21/06/2023 15:32, Lucas Tanure wrote: > > Amlogic T7 SoCs uses the same UART controller as S4 SoCs and G12A. > > There is no need for an extra compatible line in the driver, but > > add T7 compatible line for documentation. > > > > Signed-off-by: Lucas Tanure <tanure@linux.com> > > --- > > .../devicetree/bindings/serial/amlogic,meson-uart.yaml | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml b/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml > > index 01ec45b3b406..860ab58d87b0 100644 > > --- a/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml > > +++ b/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml > > @@ -33,6 +33,7 @@ properties: > > - amlogic,meson8b-uart > > - amlogic,meson-gx-uart > > - amlogic,meson-s4-uart > > + - amlogic,meson-t7-uart > > - const: amlogic,meson-ao-uart > > - description: Always-on power domain UART controller on G12A SoCs > > items: > > @@ -46,6 +47,7 @@ properties: > > - amlogic,meson8b-uart > > - amlogic,meson-gx-uart > > - amlogic,meson-s4-uart > > + - amlogic,meson-t7-uart > > It does not look like you tested the DTS against bindings. Please run > `make dtbs_check` (see > Documentation/devicetree/bindings/writing-schema.rst or > https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/ > for instructions). Check back on the previous version, I should've posted an untested version of what you need to add. Cheers, Conor.
On Wed, Jun 21, 2023 at 7:12 PM Conor Dooley <conor@kernel.org> wrote: > > Lucas, > > On Wed, Jun 21, 2023 at 03:53:04PM +0200, Krzysztof Kozlowski wrote: > > On 21/06/2023 15:32, Lucas Tanure wrote: > > > Amlogic T7 SoCs uses the same UART controller as S4 SoCs and G12A. > > > There is no need for an extra compatible line in the driver, but > > > add T7 compatible line for documentation. > > > > > > Signed-off-by: Lucas Tanure <tanure@linux.com> > > > --- > > > .../devicetree/bindings/serial/amlogic,meson-uart.yaml | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml b/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml > > > index 01ec45b3b406..860ab58d87b0 100644 > > > --- a/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml > > > +++ b/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml > > > @@ -33,6 +33,7 @@ properties: > > > - amlogic,meson8b-uart > > > - amlogic,meson-gx-uart > > > - amlogic,meson-s4-uart > > > + - amlogic,meson-t7-uart > > > - const: amlogic,meson-ao-uart > > > - description: Always-on power domain UART controller on G12A SoCs > > > items: > > > @@ -46,6 +47,7 @@ properties: > > > - amlogic,meson8b-uart > > > - amlogic,meson-gx-uart > > > - amlogic,meson-s4-uart > > > + - amlogic,meson-t7-uart > > > > It does not look like you tested the DTS against bindings. Please run > > `make dtbs_check` (see > > Documentation/devicetree/bindings/writing-schema.rst or > > https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/ > > for instructions). > > Check back on the previous version, I should've posted an untested > version of what you need to add. I saw that, but adding a S4 doesn't make sense to me. And you didn't show the entire change, so I can't understand what you want there. > > Cheers, > Conor.
On 22/06/2023 07:32, Lucas Tanure wrote: > On Wed, Jun 21, 2023 at 7:12 PM Conor Dooley <conor@kernel.org> wrote: >> >> Lucas, >> >> On Wed, Jun 21, 2023 at 03:53:04PM +0200, Krzysztof Kozlowski wrote: >>> On 21/06/2023 15:32, Lucas Tanure wrote: >>>> Amlogic T7 SoCs uses the same UART controller as S4 SoCs and G12A. >>>> There is no need for an extra compatible line in the driver, but >>>> add T7 compatible line for documentation. >>>> >>>> Signed-off-by: Lucas Tanure <tanure@linux.com> >>>> --- >>>> .../devicetree/bindings/serial/amlogic,meson-uart.yaml | 2 ++ >>>> 1 file changed, 2 insertions(+) >>>> >>>> diff --git a/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml b/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml >>>> index 01ec45b3b406..860ab58d87b0 100644 >>>> --- a/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml >>>> +++ b/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml >>>> @@ -33,6 +33,7 @@ properties: >>>> - amlogic,meson8b-uart >>>> - amlogic,meson-gx-uart >>>> - amlogic,meson-s4-uart >>>> + - amlogic,meson-t7-uart >>>> - const: amlogic,meson-ao-uart >>>> - description: Always-on power domain UART controller on G12A SoCs >>>> items: >>>> @@ -46,6 +47,7 @@ properties: >>>> - amlogic,meson8b-uart >>>> - amlogic,meson-gx-uart >>>> - amlogic,meson-s4-uart >>>> + - amlogic,meson-t7-uart >>> >>> It does not look like you tested the DTS against bindings. Please run >>> `make dtbs_check` (see >>> Documentation/devicetree/bindings/writing-schema.rst or >>> https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/ >>> for instructions). >> >> Check back on the previous version, I should've posted an untested >> version of what you need to add. > I saw that, but adding a S4 doesn't make sense to me. And you didn't > show the entire change, so I can't understand what you want there. For sure you need something which does not trigger errors. If you claim adding S4 as fallback does not make sense, then why did you use it? Sending a code which is clearly incorrect does not make sense. Best regards, Krzysztof
On Thu, Jun 22, 2023 at 7:05 AM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 22/06/2023 07:32, Lucas Tanure wrote: > > On Wed, Jun 21, 2023 at 7:12 PM Conor Dooley <conor@kernel.org> wrote: > >> > >> Lucas, > >> > >> On Wed, Jun 21, 2023 at 03:53:04PM +0200, Krzysztof Kozlowski wrote: > >>> On 21/06/2023 15:32, Lucas Tanure wrote: > >>>> Amlogic T7 SoCs uses the same UART controller as S4 SoCs and G12A. > >>>> There is no need for an extra compatible line in the driver, but > >>>> add T7 compatible line for documentation. > >>>> > >>>> Signed-off-by: Lucas Tanure <tanure@linux.com> > >>>> --- > >>>> .../devicetree/bindings/serial/amlogic,meson-uart.yaml | 2 ++ > >>>> 1 file changed, 2 insertions(+) > >>>> > >>>> diff --git a/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml b/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml > >>>> index 01ec45b3b406..860ab58d87b0 100644 > >>>> --- a/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml > >>>> +++ b/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml > >>>> @@ -33,6 +33,7 @@ properties: > >>>> - amlogic,meson8b-uart > >>>> - amlogic,meson-gx-uart > >>>> - amlogic,meson-s4-uart > >>>> + - amlogic,meson-t7-uart > >>>> - const: amlogic,meson-ao-uart > >>>> - description: Always-on power domain UART controller on G12A SoCs > >>>> items: > >>>> @@ -46,6 +47,7 @@ properties: > >>>> - amlogic,meson8b-uart > >>>> - amlogic,meson-gx-uart > >>>> - amlogic,meson-s4-uart > >>>> + - amlogic,meson-t7-uart > >>> > >>> It does not look like you tested the DTS against bindings. Please run > >>> `make dtbs_check` (see > >>> Documentation/devicetree/bindings/writing-schema.rst or > >>> https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/ > >>> for instructions). > >> > >> Check back on the previous version, I should've posted an untested > >> version of what you need to add. > > I saw that, but adding a S4 doesn't make sense to me. And you didn't > > show the entire change, so I can't understand what you want there. > > For sure you need something which does not trigger errors. If you claim > adding S4 as fallback does not make sense, then why did you use it? > Sending a code which is clearly incorrect does not make sense. > Sorry, I think we are talking about different things. It does not make sense to me to add an S4 line in the documentation when it is already there. So I could not understand or make sense of the patch Conor sent in reply to my V2. Krzysztof, I will check again with dtbs_check and re-send. > > Best regards, > Krzysztof >
On Thu, Jun 22, 2023 at 07:43:31AM +0100, Lucas Tanure wrote: > On Thu, Jun 22, 2023 at 7:05 AM Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: > > On 22/06/2023 07:32, Lucas Tanure wrote: > > > On Wed, Jun 21, 2023 at 7:12 PM Conor Dooley <conor@kernel.org> wrote: > > >> On Wed, Jun 21, 2023 at 03:53:04PM +0200, Krzysztof Kozlowski wrote: > > >>> On 21/06/2023 15:32, Lucas Tanure wrote: > > >>>> Amlogic T7 SoCs uses the same UART controller as S4 SoCs and G12A. > > >>>> There is no need for an extra compatible line in the driver, but > > >>>> add T7 compatible line for documentation. > > >>>> > > >>>> Signed-off-by: Lucas Tanure <tanure@linux.com> > > >>>> --- > > >>>> .../devicetree/bindings/serial/amlogic,meson-uart.yaml | 2 ++ > > >>>> 1 file changed, 2 insertions(+) > > >>>> > > >>>> diff --git a/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml b/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml > > >>>> index 01ec45b3b406..860ab58d87b0 100644 > > >>>> --- a/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml > > >>>> +++ b/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml > > >>>> @@ -33,6 +33,7 @@ properties: > > >>>> - amlogic,meson8b-uart > > >>>> - amlogic,meson-gx-uart > > >>>> - amlogic,meson-s4-uart > > >>>> + - amlogic,meson-t7-uart > > >>>> - const: amlogic,meson-ao-uart > > >>>> - description: Always-on power domain UART controller on G12A SoCs > > >>>> items: > > >>>> @@ -46,6 +47,7 @@ properties: > > >>>> - amlogic,meson8b-uart > > >>>> - amlogic,meson-gx-uart > > >>>> - amlogic,meson-s4-uart > > >>>> + - amlogic,meson-t7-uart > > >>> > > >>> It does not look like you tested the DTS against bindings. Please run > > >>> `make dtbs_check` (see > > >>> Documentation/devicetree/bindings/writing-schema.rst or > > >>> https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/ > > >>> for instructions). > > >> > > >> Check back on the previous version, I should've posted an untested > > >> version of what you need to add. > > > I saw that, but adding a S4 doesn't make sense to me. And you didn't > > > show the entire change, so I can't understand what you want there. > > > > For sure you need something which does not trigger errors. If you claim > > adding S4 as fallback does not make sense, then why did you use it? > > Sending a code which is clearly incorrect does not make sense. > > > Sorry, I think we are talking about different things. It does not make > sense to me to add an S4 line in the documentation when it is already > there. So I could not understand or make sense of the patch Conor sent > in reply to my V2. That is just how it works. You need to spell out exactly which combinations are permitted. The current entry for s4 says that s4 is only permitted in isolation. Since you are adding "amlogic,meson-t7-uart", "amlogic,meson-s4-uart" you need to explicitly allow that combination. You'll notice if you look at the file that the gx uart appears more than once. Given the g12a was the most recently added compatible, it might make sense to follow the pattern that it had set, given the thing your original patch copied the match data from was the g12a. That change to the dt-binding would look like: diff --git a/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml b/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml index 01ec45b3b406..eae11e87b88a 100644 --- a/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml +++ b/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml @@ -50,6 +50,13 @@ properties: items: - const: amlogic,meson-g12a-uart - const: amlogic,meson-gx-uart + - description: + Everything-Else power domain UART controller on G12A compatible SoCs + items: + - enum: + - amlogic,meson-t7-uart + - const: amlogic,meson-g12a-uart + - const: amlogic,meson-gx-uart reg: maxItems: 1 /I/ don't really care whether you do that, or do the s4 version of it, but following the most recent pattern might make more sense. When I suggested s4, it was because I only looked at the driver patch rather than the code itself. > Krzysztof, I will check again with dtbs_check and re-send. Cheers, Conor.
On Thu, Jun 22, 2023 at 8:12 AM Conor Dooley <conor.dooley@microchip.com> wrote: > > On Thu, Jun 22, 2023 at 07:43:31AM +0100, Lucas Tanure wrote: > > On Thu, Jun 22, 2023 at 7:05 AM Krzysztof Kozlowski > > <krzysztof.kozlowski@linaro.org> wrote: > > > On 22/06/2023 07:32, Lucas Tanure wrote: > > > > On Wed, Jun 21, 2023 at 7:12 PM Conor Dooley <conor@kernel.org> wrote: > > > >> On Wed, Jun 21, 2023 at 03:53:04PM +0200, Krzysztof Kozlowski wrote: > > > >>> On 21/06/2023 15:32, Lucas Tanure wrote: > > > >>>> Amlogic T7 SoCs uses the same UART controller as S4 SoCs and G12A. > > > >>>> There is no need for an extra compatible line in the driver, but > > > >>>> add T7 compatible line for documentation. > > > >>>> > > > >>>> Signed-off-by: Lucas Tanure <tanure@linux.com> > > > >>>> --- > > > >>>> .../devicetree/bindings/serial/amlogic,meson-uart.yaml | 2 ++ > > > >>>> 1 file changed, 2 insertions(+) > > > >>>> > > > >>>> diff --git a/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml b/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml > > > >>>> index 01ec45b3b406..860ab58d87b0 100644 > > > >>>> --- a/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml > > > >>>> +++ b/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml > > > >>>> @@ -33,6 +33,7 @@ properties: > > > >>>> - amlogic,meson8b-uart > > > >>>> - amlogic,meson-gx-uart > > > >>>> - amlogic,meson-s4-uart > > > >>>> + - amlogic,meson-t7-uart > > > >>>> - const: amlogic,meson-ao-uart > > > >>>> - description: Always-on power domain UART controller on G12A SoCs > > > >>>> items: > > > >>>> @@ -46,6 +47,7 @@ properties: > > > >>>> - amlogic,meson8b-uart > > > >>>> - amlogic,meson-gx-uart > > > >>>> - amlogic,meson-s4-uart > > > >>>> + - amlogic,meson-t7-uart > > > >>> > > > >>> It does not look like you tested the DTS against bindings. Please run > > > >>> `make dtbs_check` (see > > > >>> Documentation/devicetree/bindings/writing-schema.rst or > > > >>> https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/ > > > >>> for instructions). > > > >> > > > >> Check back on the previous version, I should've posted an untested > > > >> version of what you need to add. > > > > I saw that, but adding a S4 doesn't make sense to me. And you didn't > > > > show the entire change, so I can't understand what you want there. > > > > > > For sure you need something which does not trigger errors. If you claim > > > adding S4 as fallback does not make sense, then why did you use it? > > > Sending a code which is clearly incorrect does not make sense. > > > > > Sorry, I think we are talking about different things. It does not make > > sense to me to add an S4 line in the documentation when it is already > > there. So I could not understand or make sense of the patch Conor sent > > in reply to my V2. > > That is just how it works. You need to spell out exactly which > combinations are permitted. The current entry for s4 says that s4 is > only permitted in isolation. > Since you are adding "amlogic,meson-t7-uart", "amlogic,meson-s4-uart" > you need to explicitly allow that combination. You'll notice if you look > at the file that the gx uart appears more than once. > > Given the g12a was the most recently added compatible, it might make > sense to follow the pattern that it had set, given the thing your > original patch copied the match data from was the g12a. That change to > the dt-binding would look like: > diff --git a/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml b/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml > index 01ec45b3b406..eae11e87b88a 100644 > --- a/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml > +++ b/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml > @@ -50,6 +50,13 @@ properties: > items: > - const: amlogic,meson-g12a-uart > - const: amlogic,meson-gx-uart > + - description: > + Everything-Else power domain UART controller on G12A compatible SoCs > + items: > + - enum: > + - amlogic,meson-t7-uart > + - const: amlogic,meson-g12a-uart > + - const: amlogic,meson-gx-uart > > reg: > maxItems: 1 > > /I/ don't really care whether you do that, or do the s4 version of it, > but following the most recent pattern might make more sense. When I > suggested s4, it was because I only looked at the driver patch rather > than the code itself. > > > Krzysztof, I will check again with dtbs_check and re-send. > > Cheers, > Conor. I am struggling to understand this. Everything I try fails the check.
On 22/06/2023 09:36, Lucas Tanure wrote: > On Thu, Jun 22, 2023 at 8:12 AM Conor Dooley <conor.dooley@microchip.com> wrote: >> >> On Thu, Jun 22, 2023 at 07:43:31AM +0100, Lucas Tanure wrote: >>> On Thu, Jun 22, 2023 at 7:05 AM Krzysztof Kozlowski >>> <krzysztof.kozlowski@linaro.org> wrote: >>>> On 22/06/2023 07:32, Lucas Tanure wrote: >>>>> On Wed, Jun 21, 2023 at 7:12 PM Conor Dooley <conor@kernel.org> wrote: >>>>>> On Wed, Jun 21, 2023 at 03:53:04PM +0200, Krzysztof Kozlowski wrote: >>>>>>> On 21/06/2023 15:32, Lucas Tanure wrote: >>>>>>>> Amlogic T7 SoCs uses the same UART controller as S4 SoCs and G12A. >>>>>>>> There is no need for an extra compatible line in the driver, but >>>>>>>> add T7 compatible line for documentation. >>>>>>>> >>>>>>>> Signed-off-by: Lucas Tanure <tanure@linux.com> >>>>>>>> --- >>>>>>>> .../devicetree/bindings/serial/amlogic,meson-uart.yaml | 2 ++ >>>>>>>> 1 file changed, 2 insertions(+) >>>>>>>> >>>>>>>> diff --git a/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml b/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml >>>>>>>> index 01ec45b3b406..860ab58d87b0 100644 >>>>>>>> --- a/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml >>>>>>>> +++ b/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml >>>>>>>> @@ -33,6 +33,7 @@ properties: >>>>>>>> - amlogic,meson8b-uart >>>>>>>> - amlogic,meson-gx-uart >>>>>>>> - amlogic,meson-s4-uart >>>>>>>> + - amlogic,meson-t7-uart >>>>>>>> - const: amlogic,meson-ao-uart >>>>>>>> - description: Always-on power domain UART controller on G12A SoCs >>>>>>>> items: >>>>>>>> @@ -46,6 +47,7 @@ properties: >>>>>>>> - amlogic,meson8b-uart >>>>>>>> - amlogic,meson-gx-uart >>>>>>>> - amlogic,meson-s4-uart >>>>>>>> + - amlogic,meson-t7-uart >>>>>>> >>>>>>> It does not look like you tested the DTS against bindings. Please run >>>>>>> `make dtbs_check` (see >>>>>>> Documentation/devicetree/bindings/writing-schema.rst or >>>>>>> https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/ >>>>>>> for instructions). >>>>>> >>>>>> Check back on the previous version, I should've posted an untested >>>>>> version of what you need to add. >>>>> I saw that, but adding a S4 doesn't make sense to me. And you didn't >>>>> show the entire change, so I can't understand what you want there. >>>> >>>> For sure you need something which does not trigger errors. If you claim >>>> adding S4 as fallback does not make sense, then why did you use it? >>>> Sending a code which is clearly incorrect does not make sense. >>>> >>> Sorry, I think we are talking about different things. It does not make >>> sense to me to add an S4 line in the documentation when it is already >>> there. So I could not understand or make sense of the patch Conor sent >>> in reply to my V2. >> >> That is just how it works. You need to spell out exactly which >> combinations are permitted. The current entry for s4 says that s4 is >> only permitted in isolation. >> Since you are adding "amlogic,meson-t7-uart", "amlogic,meson-s4-uart" >> you need to explicitly allow that combination. You'll notice if you look >> at the file that the gx uart appears more than once. >> >> Given the g12a was the most recently added compatible, it might make >> sense to follow the pattern that it had set, given the thing your >> original patch copied the match data from was the g12a. That change to >> the dt-binding would look like: >> diff --git a/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml b/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml >> index 01ec45b3b406..eae11e87b88a 100644 >> --- a/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml >> +++ b/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml >> @@ -50,6 +50,13 @@ properties: >> items: >> - const: amlogic,meson-g12a-uart >> - const: amlogic,meson-gx-uart >> + - description: >> + Everything-Else power domain UART controller on G12A compatible SoCs >> + items: >> + - enum: >> + - amlogic,meson-t7-uart >> + - const: amlogic,meson-g12a-uart >> + - const: amlogic,meson-gx-uart >> >> reg: >> maxItems: 1 >> >> /I/ don't really care whether you do that, or do the s4 version of it, >> but following the most recent pattern might make more sense. When I >> suggested s4, it was because I only looked at the driver patch rather >> than the code itself. >> >>> Krzysztof, I will check again with dtbs_check and re-send. >> >> Cheers, >> Conor. > I am struggling to understand this. Everything I try fails the check. I just applied Conor's change on top of v6.4-rc1 and ran: make dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml and the check was successful. Neil
On Thu, Jun 22, 2023 at 9:13 AM Neil Armstrong <neil.armstrong@linaro.org> wrote: > > On 22/06/2023 09:36, Lucas Tanure wrote: > > On Thu, Jun 22, 2023 at 8:12 AM Conor Dooley <conor.dooley@microchip.com> wrote: > >> > >> On Thu, Jun 22, 2023 at 07:43:31AM +0100, Lucas Tanure wrote: > >>> On Thu, Jun 22, 2023 at 7:05 AM Krzysztof Kozlowski > >>> <krzysztof.kozlowski@linaro.org> wrote: > >>>> On 22/06/2023 07:32, Lucas Tanure wrote: > >>>>> On Wed, Jun 21, 2023 at 7:12 PM Conor Dooley <conor@kernel.org> wrote: > >>>>>> On Wed, Jun 21, 2023 at 03:53:04PM +0200, Krzysztof Kozlowski wrote: > >>>>>>> On 21/06/2023 15:32, Lucas Tanure wrote: > >>>>>>>> Amlogic T7 SoCs uses the same UART controller as S4 SoCs and G12A. > >>>>>>>> There is no need for an extra compatible line in the driver, but > >>>>>>>> add T7 compatible line for documentation. > >>>>>>>> > >>>>>>>> Signed-off-by: Lucas Tanure <tanure@linux.com> > >>>>>>>> --- > >>>>>>>> .../devicetree/bindings/serial/amlogic,meson-uart.yaml | 2 ++ > >>>>>>>> 1 file changed, 2 insertions(+) > >>>>>>>> > >>>>>>>> diff --git a/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml b/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml > >>>>>>>> index 01ec45b3b406..860ab58d87b0 100644 > >>>>>>>> --- a/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml > >>>>>>>> +++ b/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml > >>>>>>>> @@ -33,6 +33,7 @@ properties: > >>>>>>>> - amlogic,meson8b-uart > >>>>>>>> - amlogic,meson-gx-uart > >>>>>>>> - amlogic,meson-s4-uart > >>>>>>>> + - amlogic,meson-t7-uart > >>>>>>>> - const: amlogic,meson-ao-uart > >>>>>>>> - description: Always-on power domain UART controller on G12A SoCs > >>>>>>>> items: > >>>>>>>> @@ -46,6 +47,7 @@ properties: > >>>>>>>> - amlogic,meson8b-uart > >>>>>>>> - amlogic,meson-gx-uart > >>>>>>>> - amlogic,meson-s4-uart > >>>>>>>> + - amlogic,meson-t7-uart > >>>>>>> > >>>>>>> It does not look like you tested the DTS against bindings. Please run > >>>>>>> `make dtbs_check` (see > >>>>>>> Documentation/devicetree/bindings/writing-schema.rst or > >>>>>>> https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/ > >>>>>>> for instructions). > >>>>>> > >>>>>> Check back on the previous version, I should've posted an untested > >>>>>> version of what you need to add. > >>>>> I saw that, but adding a S4 doesn't make sense to me. And you didn't > >>>>> show the entire change, so I can't understand what you want there. > >>>> > >>>> For sure you need something which does not trigger errors. If you claim > >>>> adding S4 as fallback does not make sense, then why did you use it? > >>>> Sending a code which is clearly incorrect does not make sense. > >>>> > >>> Sorry, I think we are talking about different things. It does not make > >>> sense to me to add an S4 line in the documentation when it is already > >>> there. So I could not understand or make sense of the patch Conor sent > >>> in reply to my V2. > >> > >> That is just how it works. You need to spell out exactly which > >> combinations are permitted. The current entry for s4 says that s4 is > >> only permitted in isolation. > >> Since you are adding "amlogic,meson-t7-uart", "amlogic,meson-s4-uart" > >> you need to explicitly allow that combination. You'll notice if you look > >> at the file that the gx uart appears more than once. > >> > >> Given the g12a was the most recently added compatible, it might make > >> sense to follow the pattern that it had set, given the thing your > >> original patch copied the match data from was the g12a. That change to > >> the dt-binding would look like: > >> diff --git a/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml b/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml > >> index 01ec45b3b406..eae11e87b88a 100644 > >> --- a/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml > >> +++ b/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml > >> @@ -50,6 +50,13 @@ properties: > >> items: > >> - const: amlogic,meson-g12a-uart > >> - const: amlogic,meson-gx-uart > >> + - description: > >> + Everything-Else power domain UART controller on G12A compatible SoCs > >> + items: > >> + - enum: > >> + - amlogic,meson-t7-uart > >> + - const: amlogic,meson-g12a-uart > >> + - const: amlogic,meson-gx-uart > >> > >> reg: > >> maxItems: 1 > >> > >> /I/ don't really care whether you do that, or do the s4 version of it, > >> but following the most recent pattern might make more sense. When I > >> suggested s4, it was because I only looked at the driver patch rather > >> than the code itself. > >> > >>> Krzysztof, I will check again with dtbs_check and re-send. > >> > >> Cheers, > >> Conor. > > I am struggling to understand this. Everything I try fails the check. > > I just applied Conor's change on top of v6.4-rc1 and ran: > make dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml > > and the check was successful. > > Neil > > I am sending v4 in a few minutes
diff --git a/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml b/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml index 01ec45b3b406..860ab58d87b0 100644 --- a/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml +++ b/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml @@ -33,6 +33,7 @@ properties: - amlogic,meson8b-uart - amlogic,meson-gx-uart - amlogic,meson-s4-uart + - amlogic,meson-t7-uart - const: amlogic,meson-ao-uart - description: Always-on power domain UART controller on G12A SoCs items: @@ -46,6 +47,7 @@ properties: - amlogic,meson8b-uart - amlogic,meson-gx-uart - amlogic,meson-s4-uart + - amlogic,meson-t7-uart - description: Everything-Else power domain UART controller on G12A SoCs items: - const: amlogic,meson-g12a-uart
Amlogic T7 SoCs uses the same UART controller as S4 SoCs and G12A. There is no need for an extra compatible line in the driver, but add T7 compatible line for documentation. Signed-off-by: Lucas Tanure <tanure@linux.com> --- .../devicetree/bindings/serial/amlogic,meson-uart.yaml | 2 ++ 1 file changed, 2 insertions(+)