Re: [PATCH v3 2/3] dt-bindings: serial: amlogic,meson-uart: Add compatible string for T7

[Conor Dooley <conor.dooley@microchip.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4588 bytes --]

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.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel