diff mbox series

[1/2] dt-bindings: Add optional gpio property to uartps node to support rs485

Message ID 1682512187-8828-2-git-send-email-manikanta.guntupalli@amd.com
State New
Headers show
Series Add rs485 support to uartps driver | expand

Commit Message

Manikanta Guntupalli April 26, 2023, 12:29 p.m. UTC
Add optional gpio property to uartps node and add reference to rs485.yaml

Signed-off-by: Manikanta Guntupalli <manikanta.guntupalli@amd.com>
---
 Documentation/devicetree/bindings/serial/cdns,uart.yaml | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Rob Herring April 27, 2023, 4:43 p.m. UTC | #1
On Wed, Apr 26, 2023 at 05:59:46PM +0530, Manikanta Guntupalli wrote:
> Add optional gpio property to uartps node and add reference to rs485.yaml

The diff tells me that already. Why?

> 
> Signed-off-by: Manikanta Guntupalli <manikanta.guntupalli@amd.com>
> ---
>  Documentation/devicetree/bindings/serial/cdns,uart.yaml | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/serial/cdns,uart.yaml b/Documentation/devicetree/bindings/serial/cdns,uart.yaml
> index a8b323d7bf94..34ed0d80b923 100644
> --- a/Documentation/devicetree/bindings/serial/cdns,uart.yaml
> +++ b/Documentation/devicetree/bindings/serial/cdns,uart.yaml
> @@ -46,6 +46,10 @@ properties:
>    power-domains:
>      maxItems: 1
>  
> +  txrx-gpios:
> +    description: Optional GPIO to switch transmit and receive modes via pmod.

What's pmod?

> +    maxItems: 1
> +
>  required:
>    - compatible
>    - reg
> @@ -55,6 +59,7 @@ required:
>  
>  allOf:
>    - $ref: serial.yaml#
> +  - $ref: rs485.yaml#
>    - if:
>        properties:
>          compatible:
> -- 
> 2.25.1
>
Manikanta Guntupalli May 2, 2023, 10:14 a.m. UTC | #2
Hi Rob Herring,

> -----Original Message-----
> From: Rob Herring <robh@kernel.org>
> Sent: Thursday, April 27, 2023 10:14 PM
> To: Guntupalli, Manikanta <manikanta.guntupalli@amd.com>
> Cc: gregkh@linuxfoundation.org; krzysztof.kozlowski+dt@linaro.org;
> michal.simek@xilinx.com; linux-serial@vger.kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> jirislaby@kernel.org; linux-arm-kernel@lists.infradead.org; Simek, Michal
> <michal.simek@amd.com>; git (AMD-Xilinx) <git@amd.com>; Pandey,
> Radhey Shyam <radhey.shyam.pandey@amd.com>; Datta, Shubhrajyoti
> <shubhrajyoti.datta@amd.com>; Goud, Srinivas <srinivas.goud@amd.com>;
> manion05gk@gmail.com
> Subject: Re: [PATCH 1/2] dt-bindings: Add optional gpio property to uartps
> node to support rs485
> 
> On Wed, Apr 26, 2023 at 05:59:46PM +0530, Manikanta Guntupalli wrote:
> > Add optional gpio property to uartps node and add reference to rs485.yaml
> 
> The diff tells me that already. Why?
Please check Figure 11-3 (Page number 37) in the below mentioned data sheet, It shows PMOD (ISOW14x2) devices used in half duplex configuration. Driver outputs Y and Z are shorted to A and B respectively. This reduces overall cabling requirements. Also DE/RE are shorted to each other, and at a time, any node acts as either a driver or a receiver.
Using above optional GPIO controlling driver or a receiver. This GPIO is optional because it is not required for uart console node.

Here,
DE - Driver enable. If pin is floating, driver is disabled (internal pull-down resistor)
RE - Receiver enable. If pin is floating, receiver buffer is disabled (internal pull-up resistor)

Datasheet:
https://www.ti.com/lit/ds/symlink/isow1432.pdf?ts=1682607122706&ref_url=https%253A%252F%252Fwww.ti.com%252Fproduct%252FISOW1432%252Fpart-details%252FISOW1432DFMR%253FkeyMatch%253DISOW1432DFMR%2526tisearch%253Dsearch-everything%2526usecase%253DOPN

> 
> >
> > Signed-off-by: Manikanta Guntupalli <manikanta.guntupalli@amd.com>
> > ---
> >  Documentation/devicetree/bindings/serial/cdns,uart.yaml | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/serial/cdns,uart.yaml
> b/Documentation/devicetree/bindings/serial/cdns,uart.yaml
> > index a8b323d7bf94..34ed0d80b923 100644
> > --- a/Documentation/devicetree/bindings/serial/cdns,uart.yaml
> > +++ b/Documentation/devicetree/bindings/serial/cdns,uart.yaml
> > @@ -46,6 +46,10 @@ properties:
> >    power-domains:
> >      maxItems: 1
> >
> > +  txrx-gpios:
> > +    description: Optional GPIO to switch transmit and receive modes via
> pmod.
> 
> What's pmod?
Pmod is galvanically-isolated RS-485/RS-422 transceivers with a built-in isolated
DC-DC converter, that eliminates the need for a separate isolated power supply in
space constrained isolated designs.

> 
> > +    maxItems: 1
> > +
> >  required:
> >    - compatible
> >    - reg
> > @@ -55,6 +59,7 @@ required:
> >
> >  allOf:
> >    - $ref: serial.yaml#
> > +  - $ref: rs485.yaml#
> >    - if:
> >        properties:
> >          compatible:
> > --
> > 2.25.1
> >
Krzysztof Kozlowski May 2, 2023, 10:19 a.m. UTC | #3
On 02/05/2023 12:14, Guntupalli, Manikanta wrote:
> Hi Rob Herring,
> 
>> -----Original Message-----
>> From: Rob Herring <robh@kernel.org>
>> Sent: Thursday, April 27, 2023 10:14 PM
>> To: Guntupalli, Manikanta <manikanta.guntupalli@amd.com>
>> Cc: gregkh@linuxfoundation.org; krzysztof.kozlowski+dt@linaro.org;
>> michal.simek@xilinx.com; linux-serial@vger.kernel.org;
>> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
>> jirislaby@kernel.org; linux-arm-kernel@lists.infradead.org; Simek, Michal
>> <michal.simek@amd.com>; git (AMD-Xilinx) <git@amd.com>; Pandey,
>> Radhey Shyam <radhey.shyam.pandey@amd.com>; Datta, Shubhrajyoti
>> <shubhrajyoti.datta@amd.com>; Goud, Srinivas <srinivas.goud@amd.com>;
>> manion05gk@gmail.com
>> Subject: Re: [PATCH 1/2] dt-bindings: Add optional gpio property to uartps
>> node to support rs485
>>
>> On Wed, Apr 26, 2023 at 05:59:46PM +0530, Manikanta Guntupalli wrote:
>>> Add optional gpio property to uartps node and add reference to rs485.yaml
>>
>> The diff tells me that already. Why?
> Please check Figure 11-3 (Page number 37) in the below mentioned data sheet, It shows PMOD (ISOW14x2) devices used in half duplex configuration. Driver outputs Y and Z are shorted to A and B respectively. This reduces overall cabling requirements. Also DE/RE are shorted to each other, and at a time, any node acts as either a driver or a receiver.
> Using above optional GPIO controlling driver or a receiver. This GPIO is optional because it is not required for uart console node.
> 
> Here,
> DE - Driver enable. If pin is floating, driver is disabled (internal pull-down resistor)
> RE - Receiver enable. If pin is floating, receiver buffer is disabled (internal pull-up resistor)
> 
> Datasheet:
> https://www.ti.com/lit/ds/symlink/isow1432.pdf?ts=1682607122706&ref_url=https%253A%252F%252Fwww.ti.com%252Fproduct%252FISOW1432%252Fpart-details%252FISOW1432DFMR%253FkeyMatch%253DISOW1432DFMR%2526tisearch%253Dsearch-everything%2526usecase%253DOPN

Your request for us to get into some datasheet somewhere, to get the
answer to the basic question, is not reasonable. Commit must justify its
existence, not some external source.

> 
>>
>>>
>>> Signed-off-by: Manikanta Guntupalli <manikanta.guntupalli@amd.com>
>>> ---
>>>  Documentation/devicetree/bindings/serial/cdns,uart.yaml | 5 +++++
>>>  1 file changed, 5 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/serial/cdns,uart.yaml
>> b/Documentation/devicetree/bindings/serial/cdns,uart.yaml
>>> index a8b323d7bf94..34ed0d80b923 100644
>>> --- a/Documentation/devicetree/bindings/serial/cdns,uart.yaml
>>> +++ b/Documentation/devicetree/bindings/serial/cdns,uart.yaml
>>> @@ -46,6 +46,10 @@ properties:
>>>    power-domains:
>>>      maxItems: 1
>>>
>>> +  txrx-gpios:
>>> +    description: Optional GPIO to switch transmit and receive modes via
>> pmod.
>>
>> What's pmod?
> Pmod is galvanically-isolated RS-485/RS-422 transceivers with a built-in isolated
> DC-DC converter, that eliminates the need for a separate isolated power supply in
> space constrained isolated designs.

It's not about explaining it here, but making your changes justified and
obvious once implemented.

Best regards,
Krzysztof
Michal Simek May 2, 2023, 10:57 a.m. UTC | #4
On 5/2/23 12:19, Krzysztof Kozlowski wrote:
> On 02/05/2023 12:14, Guntupalli, Manikanta wrote:
>> Hi Rob Herring,
>>
>>> -----Original Message-----
>>> From: Rob Herring <robh@kernel.org>
>>> Sent: Thursday, April 27, 2023 10:14 PM
>>> To: Guntupalli, Manikanta <manikanta.guntupalli@amd.com>
>>> Cc: gregkh@linuxfoundation.org; krzysztof.kozlowski+dt@linaro.org;
>>> michal.simek@xilinx.com; linux-serial@vger.kernel.org;
>>> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
>>> jirislaby@kernel.org; linux-arm-kernel@lists.infradead.org; Simek, Michal
>>> <michal.simek@amd.com>; git (AMD-Xilinx) <git@amd.com>; Pandey,
>>> Radhey Shyam <radhey.shyam.pandey@amd.com>; Datta, Shubhrajyoti
>>> <shubhrajyoti.datta@amd.com>; Goud, Srinivas <srinivas.goud@amd.com>;
>>> manion05gk@gmail.com
>>> Subject: Re: [PATCH 1/2] dt-bindings: Add optional gpio property to uartps
>>> node to support rs485
>>>
>>> On Wed, Apr 26, 2023 at 05:59:46PM +0530, Manikanta Guntupalli wrote:
>>>> Add optional gpio property to uartps node and add reference to rs485.yaml
>>>
>>> The diff tells me that already. Why?
>> Please check Figure 11-3 (Page number 37) in the below mentioned data sheet, It shows PMOD (ISOW14x2) devices used in half duplex configuration. Driver outputs Y and Z are shorted to A and B respectively. This reduces overall cabling requirements. Also DE/RE are shorted to each other, and at a time, any node acts as either a driver or a receiver.
>> Using above optional GPIO controlling driver or a receiver. This GPIO is optional because it is not required for uart console node.
>>
>> Here,
>> DE - Driver enable. If pin is floating, driver is disabled (internal pull-down resistor)
>> RE - Receiver enable. If pin is floating, receiver buffer is disabled (internal pull-up resistor)
>>
>> Datasheet:
>> https://www.ti.com/lit/ds/symlink/isow1432.pdf?ts=1682607122706&ref_url=https%253A%252F%252Fwww.ti.com%252Fproduct%252FISOW1432%252Fpart-details%252FISOW1432DFMR%253FkeyMatch%253DISOW1432DFMR%2526tisearch%253Dsearch-everything%2526usecase%253DOPN
> 
> Your request for us to get into some datasheet somewhere, to get the
> answer to the basic question, is not reasonable. Commit must justify its
> existence, not some external source.

Not really. Manual itself just shows picture of that connection if you are 
interested.
On schematics it looks like that DE and /RE are just handle via one gpio. That's 
why you need 3 wires instead of 4. Definitely it should be described better.

My concern was more about choosing proper name for this functionality.

Thanks,
Michal
Krzysztof Kozlowski May 2, 2023, 11:10 a.m. UTC | #5
On 02/05/2023 12:57, Michal Simek wrote:
> 
> 
> On 5/2/23 12:19, Krzysztof Kozlowski wrote:
>> On 02/05/2023 12:14, Guntupalli, Manikanta wrote:
>>> Hi Rob Herring,
>>>
>>>> -----Original Message-----
>>>> From: Rob Herring <robh@kernel.org>
>>>> Sent: Thursday, April 27, 2023 10:14 PM
>>>> To: Guntupalli, Manikanta <manikanta.guntupalli@amd.com>
>>>> Cc: gregkh@linuxfoundation.org; krzysztof.kozlowski+dt@linaro.org;
>>>> michal.simek@xilinx.com; linux-serial@vger.kernel.org;
>>>> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
>>>> jirislaby@kernel.org; linux-arm-kernel@lists.infradead.org; Simek, Michal
>>>> <michal.simek@amd.com>; git (AMD-Xilinx) <git@amd.com>; Pandey,
>>>> Radhey Shyam <radhey.shyam.pandey@amd.com>; Datta, Shubhrajyoti
>>>> <shubhrajyoti.datta@amd.com>; Goud, Srinivas <srinivas.goud@amd.com>;
>>>> manion05gk@gmail.com
>>>> Subject: Re: [PATCH 1/2] dt-bindings: Add optional gpio property to uartps
>>>> node to support rs485
>>>>
>>>> On Wed, Apr 26, 2023 at 05:59:46PM +0530, Manikanta Guntupalli wrote:
>>>>> Add optional gpio property to uartps node and add reference to rs485.yaml
>>>>
>>>> The diff tells me that already. Why?
>>> Please check Figure 11-3 (Page number 37) in the below mentioned data sheet, It shows PMOD (ISOW14x2) devices used in half duplex configuration. Driver outputs Y and Z are shorted to A and B respectively. This reduces overall cabling requirements. Also DE/RE are shorted to each other, and at a time, any node acts as either a driver or a receiver.
>>> Using above optional GPIO controlling driver or a receiver. This GPIO is optional because it is not required for uart console node.
>>>
>>> Here,
>>> DE - Driver enable. If pin is floating, driver is disabled (internal pull-down resistor)
>>> RE - Receiver enable. If pin is floating, receiver buffer is disabled (internal pull-up resistor)
>>>
>>> Datasheet:
>>> https://www.ti.com/lit/ds/symlink/isow1432.pdf?ts=1682607122706&ref_url=https%253A%252F%252Fwww.ti.com%252Fproduct%252FISOW1432%252Fpart-details%252FISOW1432DFMR%253FkeyMatch%253DISOW1432DFMR%2526tisearch%253Dsearch-everything%2526usecase%253DOPN
>>
>> Your request for us to get into some datasheet somewhere, to get the
>> answer to the basic question, is not reasonable. Commit must justify its
>> existence, not some external source.
> 
> Not really. Manual itself just shows picture of that connection if you are 
> interested.
> On schematics it looks like that DE and /RE are just handle via one gpio. That's 
> why you need 3 wires instead of 4. Definitely it should be described better.

By looking at commit reviewer did not figure out why do we need it.
Referencing datasheet in a reply in LKML thread does not solve it
because the next person reading the commit might have the same question.

The commit must answer to "why" if this is not obvious. I don't find
responses "Please check..." to this helpful.


Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/serial/cdns,uart.yaml b/Documentation/devicetree/bindings/serial/cdns,uart.yaml
index a8b323d7bf94..34ed0d80b923 100644
--- a/Documentation/devicetree/bindings/serial/cdns,uart.yaml
+++ b/Documentation/devicetree/bindings/serial/cdns,uart.yaml
@@ -46,6 +46,10 @@  properties:
   power-domains:
     maxItems: 1
 
+  txrx-gpios:
+    description: Optional GPIO to switch transmit and receive modes via pmod.
+    maxItems: 1
+
 required:
   - compatible
   - reg
@@ -55,6 +59,7 @@  required:
 
 allOf:
   - $ref: serial.yaml#
+  - $ref: rs485.yaml#
   - if:
       properties:
         compatible: