Re: [PATCH v4 12/39] dt-bindings: serial: atmel,at91-usart: add compatible for sam9x7.

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3562 bytes --]

On Wed, Feb 28, 2024 at 07:03:01AM +0000, Varshini.Rajendran@microchip.com wrote:
> Hi Conor,
> 
> On 25/02/24 1:32 am, Conor Dooley wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > On Fri, Feb 23, 2024 at 10:55:59PM +0530, Varshini Rajendran wrote:
> >> Add sam9x7 compatible to DT bindings documentation.
> >>
> >> Signed-off-by: Varshini Rajendran <varshini.rajendran@microchip.com>
> >> ---
> >> Changes in v4:
> >> - Fixed the wrong addition of compatible
> >> - Added further compatibles that are possible correct (as per DT)
> >> ---
> >>  .../devicetree/bindings/serial/atmel,at91-usart.yaml | 12 +++++++++---
> >>  1 file changed, 9 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml b/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml
> >> index 65cb2e5c5eee..30af537e8e81 100644
> >> --- a/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml
> >> +++ b/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml
> >> @@ -23,11 +23,17 @@ properties:
> >>            - const: atmel,at91sam9260-dbgu
> >>            - const: atmel,at91sam9260-usart
> >>        - items:
> >> -          - const: microchip,sam9x60-usart
> >> +          - enum:
> >> +              - microchip,sam9x60-usart
> >> +              - microchip,sam9x7-usart
> >>            - const: atmel,at91sam9260-usart
> >>        - items:
> >> -          - const: microchip,sam9x60-dbgu
> >> -          - const: microchip,sam9x60-usart
> >> +          - enum:
> >> +              - microchip,sam9x60-dbgu
> >> +              - microchip,sam9x7-dbgu
> > 
> >> +          - enum:
> >> +              - microchip,sam9x60-usart
> >> +              - microchip,sam9x7-usart
> > 
> > This doesn't make sense - this enum should be a const.
> > I don't really understand the idea behind of the original binding here that
> > allowed:
> > "microchip,sam9x60-dbgu", "microchip,sam9x60-usart", "atmel,at91sam9260-dbgu", "atmel,at91sam9260-usart"
> > 
> > Specifically, I don't get the purpose of the "microchip,sam9x60-usart".
> > Either make it
> >       - items:
> >           - enum:
> >               - microchip,sam9x60-dbgu
> >               - microchip,sam9x7-dbgu
> >           - const: microchip,sam9x60-usart
> >           - const: atmel,at91sam9260-dbgu
> >           - const: atmel,at91sam9260-usart
> > or add
> >       - items:
> >           - const: microchip,sam9x60-dbgu
> >           - const: atmel,at91sam9260-dbgu
> >           - const: atmel,at91sam9260-usart
> > or explain exactly why this needs to be
> > "chipa-dgbu", "chipa-usart", "chipb-dbgu", "chipb-dbgu"
> The compatible has to be "chipa-usart", "chipb-usart", "chipa-dbgu", 
> "chipb-dbgu" for the device to work as a debug console over UART
> wher the chipa-<periph> is the device specific compatible
> and the chipb-<periph> is the fallback compatible that the driver 
> actually uses.

This examples why you have "microchip,sam9x60-dbgu", "atmel,at91sam9260-dbgu"
and "atmel,at91sam9260-usart".
It does not explain "microchip,sam9x60-usart" though, I don't see what
purpose that serves. If used as a debug uart, you fall back to the
sam9260 debug uart compatible and if not you fall back to the sam9260
usart compatible.

In addition, the current setup implies that sam9x60 usart supports all
the features that the sam9260 debug usart does. I doubt that that is
true.

Thanks,
Conor.

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