diff mbox series

[v8,05/10] dt-bindings: sc16is7xx: Add property to change GPIO function

Message ID 20230607140525.833982-6-hugo@hugovil.com
State New
Headers
Series serial: sc16is7xx: fix GPIO regression and rs485 improvements |

Commit Message

Hugo Villeneuve June 7, 2023, 2:05 p.m. UTC 
  From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

Some variants in this series of UART controllers have GPIO pins that
are shared between GPIO and modem control lines.

The pin mux mode (GPIO or modem control lines) can be set for each
ports (channels) supported by the variant.

This adds a property to the device tree to set the GPIO pin mux to
modem control lines on selected ports if needed.

Cc: <stable@vger.kernel.org> # 6.1.x
Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
Acked-by: Conor Dooley <conor.dooley@microchip.com>
Reviewed-by: Lech Perczak <lech.perczak@camlingroup.com>
---
 .../bindings/serial/nxp,sc16is7xx.txt         | 46 +++++++++++++++++++
 1 file changed, 46 insertions(+)

Comments

Krzysztof Kozlowski June 7, 2023, 2:30 p.m. UTC | #1 
On 07/06/2023 16:05, Hugo Villeneuve wrote:
> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> 
> Some variants in this series of UART controllers have GPIO pins that
> are shared between GPIO and modem control lines.
> 
> The pin mux mode (GPIO or modem control lines) can be set for each
> ports (channels) supported by the variant.
> 
> This adds a property to the device tree to set the GPIO pin mux to
> modem control lines on selected ports if needed.
> 
> Cc: <stable@vger.kernel.org> # 6.1.x
> Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> Acked-by: Conor Dooley <conor.dooley@microchip.com>
> Reviewed-by: Lech Perczak <lech.perczak@camlingroup.com>
> ---
>  .../bindings/serial/nxp,sc16is7xx.txt         | 46 +++++++++++++++++++
>  1 file changed, 46 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt b/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt
> index 0fa8e3e43bf8..1a7e4bff0456 100644
> --- a/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt
> +++ b/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt
> @@ -23,6 +23,9 @@ Optional properties:
>      1 = active low.
>  - irda-mode-ports: An array that lists the indices of the port that
>  		   should operate in IrDA mode.
> +- nxp,modem-control-line-ports: An array that lists the indices of the port that
> +				should have shared GPIO lines configured as
> +				modem control lines.
>  
>  Example:
>          sc16is750: sc16is750@51 {
> @@ -35,6 +38,26 @@ Example:
>                  #gpio-cells = <2>;
>          };
>  
> +	sc16is752: sc16is752@53 {

Since you keep sending new versions, fix the names. nNode names should
be generic. See also explanation and list of examples in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

Best regards,
Krzysztof
Hugo Villeneuve June 7, 2023, 2:41 p.m. UTC | #2 
On Wed, 7 Jun 2023 16:30:26 +0200
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> On 07/06/2023 16:05, Hugo Villeneuve wrote:
> > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > 
> > Some variants in this series of UART controllers have GPIO pins that
> > are shared between GPIO and modem control lines.
> > 
> > The pin mux mode (GPIO or modem control lines) can be set for each
> > ports (channels) supported by the variant.
> > 
> > This adds a property to the device tree to set the GPIO pin mux to
> > modem control lines on selected ports if needed.
> > 
> > Cc: <stable@vger.kernel.org> # 6.1.x
> > Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > Acked-by: Conor Dooley <conor.dooley@microchip.com>
> > Reviewed-by: Lech Perczak <lech.perczak@camlingroup.com>
> > ---
> >  .../bindings/serial/nxp,sc16is7xx.txt         | 46 +++++++++++++++++++
> >  1 file changed, 46 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt b/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt
> > index 0fa8e3e43bf8..1a7e4bff0456 100644
> > --- a/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt
> > +++ b/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt
> > @@ -23,6 +23,9 @@ Optional properties:
> >      1 = active low.
> >  - irda-mode-ports: An array that lists the indices of the port that
> >  		   should operate in IrDA mode.
> > +- nxp,modem-control-line-ports: An array that lists the indices of the port that
> > +				should have shared GPIO lines configured as
> > +				modem control lines.
> >  
> >  Example:
> >          sc16is750: sc16is750@51 {
> > @@ -35,6 +38,26 @@ Example:
> >                  #gpio-cells = <2>;
> >          };
> >  
> > +	sc16is752: sc16is752@53 {
> 
> Since you keep sending new versions, fix the names. nNode names should
> be generic. See also explanation and list of examples in DT specification:
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

Hi,
by the way, I do not "keep sending new versions" just for the fun of
it...

Even after reading your link, I cannot see what is wrong with that
name.

You are the first one since V1 to think it is inappropriate.

Please suggest what the name should be.

Hugo.
Krzysztof Kozlowski June 7, 2023, 2:46 p.m. UTC | #3 
On 07/06/2023 16:41, Hugo Villeneuve wrote:
> On Wed, 7 Jun 2023 16:30:26 +0200
> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> 
>> On 07/06/2023 16:05, Hugo Villeneuve wrote:
>>> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
>>>
>>> Some variants in this series of UART controllers have GPIO pins that
>>> are shared between GPIO and modem control lines.
>>>
>>> The pin mux mode (GPIO or modem control lines) can be set for each
>>> ports (channels) supported by the variant.
>>>
>>> This adds a property to the device tree to set the GPIO pin mux to
>>> modem control lines on selected ports if needed.
>>>
>>> Cc: <stable@vger.kernel.org> # 6.1.x
>>> Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
>>> Acked-by: Conor Dooley <conor.dooley@microchip.com>
>>> Reviewed-by: Lech Perczak <lech.perczak@camlingroup.com>
>>> ---
>>>  .../bindings/serial/nxp,sc16is7xx.txt         | 46 +++++++++++++++++++
>>>  1 file changed, 46 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt b/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt
>>> index 0fa8e3e43bf8..1a7e4bff0456 100644
>>> --- a/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt
>>> +++ b/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt
>>> @@ -23,6 +23,9 @@ Optional properties:
>>>      1 = active low.
>>>  - irda-mode-ports: An array that lists the indices of the port that
>>>  		   should operate in IrDA mode.
>>> +- nxp,modem-control-line-ports: An array that lists the indices of the port that
>>> +				should have shared GPIO lines configured as
>>> +				modem control lines.
>>>  
>>>  Example:
>>>          sc16is750: sc16is750@51 {
>>> @@ -35,6 +38,26 @@ Example:
>>>                  #gpio-cells = <2>;
>>>          };
>>>  
>>> +	sc16is752: sc16is752@53 {
>>
>> Since you keep sending new versions, fix the names. nNode names should
>> be generic. See also explanation and list of examples in DT specification:
>> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> 
> Hi,
> by the way, I do not "keep sending new versions" just for the fun of
> it...

Sure, I know. So when the next version is necessary to send, fix also this.

>
> Even after reading your link, I cannot see what is wrong with that
> name.

They are not generic. They are specific.

> 
> You are the first one since V1 to think it is inappropriate.
> 
> Please suggest what the name should be.

Depends what this is. I know nothing about this device, so I cannot
suggest you. Therefore I gave you link with ideas to choose from.

Best regards,
Krzysztof
Hugo Villeneuve June 7, 2023, 2:52 p.m. UTC | #4 
On Wed, 7 Jun 2023 16:46:56 +0200
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> On 07/06/2023 16:41, Hugo Villeneuve wrote:
> > On Wed, 7 Jun 2023 16:30:26 +0200
> > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> > 
> >> On 07/06/2023 16:05, Hugo Villeneuve wrote:
> >>> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> >>>
> >>> Some variants in this series of UART controllers have GPIO pins that
> >>> are shared between GPIO and modem control lines.
> >>>
> >>> The pin mux mode (GPIO or modem control lines) can be set for each
> >>> ports (channels) supported by the variant.
> >>>
> >>> This adds a property to the device tree to set the GPIO pin mux to
> >>> modem control lines on selected ports if needed.
> >>>
> >>> Cc: <stable@vger.kernel.org> # 6.1.x
> >>> Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> >>> Acked-by: Conor Dooley <conor.dooley@microchip.com>
> >>> Reviewed-by: Lech Perczak <lech.perczak@camlingroup.com>
> >>> ---
> >>>  .../bindings/serial/nxp,sc16is7xx.txt         | 46 +++++++++++++++++++
> >>>  1 file changed, 46 insertions(+)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt b/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt
> >>> index 0fa8e3e43bf8..1a7e4bff0456 100644
> >>> --- a/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt
> >>> +++ b/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt
> >>> @@ -23,6 +23,9 @@ Optional properties:
> >>>      1 = active low.
> >>>  - irda-mode-ports: An array that lists the indices of the port that
> >>>  		   should operate in IrDA mode.
> >>> +- nxp,modem-control-line-ports: An array that lists the indices of the port that
> >>> +				should have shared GPIO lines configured as
> >>> +				modem control lines.
> >>>  
> >>>  Example:
> >>>          sc16is750: sc16is750@51 {
> >>> @@ -35,6 +38,26 @@ Example:
> >>>                  #gpio-cells = <2>;
> >>>          };
> >>>  
> >>> +	sc16is752: sc16is752@53 {
> >>
> >> Since you keep sending new versions, fix the names. nNode names should
> >> be generic. See also explanation and list of examples in DT specification:
> >> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> > 
> > Hi,
> > by the way, I do not "keep sending new versions" just for the fun of
> > it...
> 
> Sure, I know. So when the next version is necessary to send, fix also this.
> 
> >
> > Even after reading your link, I cannot see what is wrong with that
> > name.
> 
> They are not generic. They are specific.

What do you mean by "They"? My patch adds only a new property...

This patch will not fix old names, but a future patch could do it.

And what do you mean by "They are not generic. They are specific". Of
course the property is specific, because it relates to something very
specific to this chip?


> > You are the first one since V1 to think it is inappropriate.
> > 
> > Please suggest what the name should be.
> 
> Depends what this is. I know nothing about this device, so I cannot
> suggest you. Therefore I gave you link with ideas to choose from.
> 
> Best regards,
> Krzysztof
> 
>
Krzysztof Kozlowski June 7, 2023, 2:57 p.m. UTC | #5 
On 07/06/2023 16:52, Hugo Villeneuve wrote:
> On Wed, 7 Jun 2023 16:46:56 +0200
> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> 
>> On 07/06/2023 16:41, Hugo Villeneuve wrote:
>>> On Wed, 7 Jun 2023 16:30:26 +0200
>>> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
>>>
>>>> On 07/06/2023 16:05, Hugo Villeneuve wrote:
>>>>> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
>>>>>
>>>>> Some variants in this series of UART controllers have GPIO pins that
>>>>> are shared between GPIO and modem control lines.
>>>>>
>>>>> The pin mux mode (GPIO or modem control lines) can be set for each
>>>>> ports (channels) supported by the variant.
>>>>>
>>>>> This adds a property to the device tree to set the GPIO pin mux to
>>>>> modem control lines on selected ports if needed.
>>>>>
>>>>> Cc: <stable@vger.kernel.org> # 6.1.x
>>>>> Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
>>>>> Acked-by: Conor Dooley <conor.dooley@microchip.com>
>>>>> Reviewed-by: Lech Perczak <lech.perczak@camlingroup.com>
>>>>> ---
>>>>>  .../bindings/serial/nxp,sc16is7xx.txt         | 46 +++++++++++++++++++
>>>>>  1 file changed, 46 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt b/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt
>>>>> index 0fa8e3e43bf8..1a7e4bff0456 100644
>>>>> --- a/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt
>>>>> +++ b/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt
>>>>> @@ -23,6 +23,9 @@ Optional properties:
>>>>>      1 = active low.
>>>>>  - irda-mode-ports: An array that lists the indices of the port that
>>>>>  		   should operate in IrDA mode.
>>>>> +- nxp,modem-control-line-ports: An array that lists the indices of the port that
>>>>> +				should have shared GPIO lines configured as
>>>>> +				modem control lines.
>>>>>  
>>>>>  Example:
>>>>>          sc16is750: sc16is750@51 {
>>>>> @@ -35,6 +38,26 @@ Example:
>>>>>                  #gpio-cells = <2>;
>>>>>          };
>>>>>  
>>>>> +	sc16is752: sc16is752@53 {
>>>>
>>>> Since you keep sending new versions, fix the names. nNode names should
>>>> be generic. See also explanation and list of examples in DT specification:
>>>> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
>>>
>>> Hi,
>>> by the way, I do not "keep sending new versions" just for the fun of
>>> it...
>>
>> Sure, I know. So when the next version is necessary to send, fix also this.
>>
>>>
>>> Even after reading your link, I cannot see what is wrong with that
>>> name.
>>
>> They are not generic. They are specific.
> 
> What do you mean by "They"? My patch adds only a new property...

Your patch adds multiple new examples. My comment was at appropriate
place to fix, although only the first place. I did not refer to
unrelated pieces or other parts of the code. Specifically - I wrote
nothing about property.

> 
> This patch will not fix old names, but a future patch could do it.

No worries, I asked for new code.

> 
> And what do you mean by "They are not generic. They are specific". Of
> course the property is specific, because it relates to something very
> specific to this chip?

Again, I did not comment under a property. I did not refer to any property.


Best regards,
Krzysztof
Hugo Villeneuve June 7, 2023, 3:09 p.m. UTC | #6 
On Wed, 7 Jun 2023 16:57:48 +0200
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> On 07/06/2023 16:52, Hugo Villeneuve wrote:
> > On Wed, 7 Jun 2023 16:46:56 +0200
> > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> > 
> >> On 07/06/2023 16:41, Hugo Villeneuve wrote:
> >>> On Wed, 7 Jun 2023 16:30:26 +0200
> >>> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> >>>
> >>>> On 07/06/2023 16:05, Hugo Villeneuve wrote:
> >>>>> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> >>>>>
> >>>>> Some variants in this series of UART controllers have GPIO pins that
> >>>>> are shared between GPIO and modem control lines.
> >>>>>
> >>>>> The pin mux mode (GPIO or modem control lines) can be set for each
> >>>>> ports (channels) supported by the variant.
> >>>>>
> >>>>> This adds a property to the device tree to set the GPIO pin mux to
> >>>>> modem control lines on selected ports if needed.
> >>>>>
> >>>>> Cc: <stable@vger.kernel.org> # 6.1.x
> >>>>> Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> >>>>> Acked-by: Conor Dooley <conor.dooley@microchip.com>
> >>>>> Reviewed-by: Lech Perczak <lech.perczak@camlingroup.com>
> >>>>> ---
> >>>>>  .../bindings/serial/nxp,sc16is7xx.txt         | 46 +++++++++++++++++++
> >>>>>  1 file changed, 46 insertions(+)
> >>>>>
> >>>>> diff --git a/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt b/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt
> >>>>> index 0fa8e3e43bf8..1a7e4bff0456 100644
> >>>>> --- a/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt
> >>>>> +++ b/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt
> >>>>> @@ -23,6 +23,9 @@ Optional properties:
> >>>>>      1 = active low.
> >>>>>  - irda-mode-ports: An array that lists the indices of the port that
> >>>>>  		   should operate in IrDA mode.
> >>>>> +- nxp,modem-control-line-ports: An array that lists the indices of the port that
> >>>>> +				should have shared GPIO lines configured as
> >>>>> +				modem control lines.
> >>>>>  
> >>>>>  Example:
> >>>>>          sc16is750: sc16is750@51 {
> >>>>> @@ -35,6 +38,26 @@ Example:
> >>>>>                  #gpio-cells = <2>;
> >>>>>          };
> >>>>>  
> >>>>> +	sc16is752: sc16is752@53 {
> >>>>
> >>>> Since you keep sending new versions, fix the names. nNode names should
> >>>> be generic. See also explanation and list of examples in DT specification:
> >>>> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> >>>
> >>> Hi,
> >>> by the way, I do not "keep sending new versions" just for the fun of
> >>> it...
> >>
> >> Sure, I know. So when the next version is necessary to send, fix also this.
> >>
> >>>
> >>> Even after reading your link, I cannot see what is wrong with that
> >>> name.
> >>
> >> They are not generic. They are specific.
> > 
> > What do you mean by "They"? My patch adds only a new property...
> 
> Your patch adds multiple new examples. My comment was at appropriate
> place to fix, although only the first place. I did not refer to
> unrelated pieces or other parts of the code. Specifically - I wrote
> nothing about property.

Ahhh!!!!!!!!!!!!

Ok, now I see.

But I simply added new examples based on the previous examples, which
already used "inappropriate" names.

If I modify only the node names in my new examples, this will clash
with the existing examples.

I have already prepared a new patch, that I will send one day if we can
ever finish this never ending series, to convert the binding to YAML,
and to fix a few things with old properties missing vendor prefix,
etc.

I will modify all the examples in this new series to follow what you
suggest and incorporate it into this new serie.

Hugo.


> > This patch will not fix old names, but a future patch could do it.
> 
> No worries, I asked for new code.
> 
> > 
> > And what do you mean by "They are not generic. They are specific". Of
> > course the property is specific, because it relates to something very
> > specific to this chip?
> 
> Again, I did not comment under a property. I did not refer to any property.
> 
> 
> Best regards,
> Krzysztof
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt b/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt
index 0fa8e3e43bf8..1a7e4bff0456 100644
--- a/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt
+++ b/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt
@@ -23,6 +23,9 @@  Optional properties:
     1 = active low.
 - irda-mode-ports: An array that lists the indices of the port that
 		   should operate in IrDA mode.
+- nxp,modem-control-line-ports: An array that lists the indices of the port that
+				should have shared GPIO lines configured as
+				modem control lines.
 
 Example:
         sc16is750: sc16is750@51 {
@@ -35,6 +38,26 @@  Example:
                 #gpio-cells = <2>;
         };
 
+	sc16is752: sc16is752@53 {
+		compatible = "nxp,sc16is752";
+		reg = <0x53>;
+		clocks = <&clk20m>;
+		interrupt-parent = <&gpio3>;
+		interrupts = <7 IRQ_TYPE_EDGE_FALLING>;
+		nxp,modem-control-line-ports = <1>; /* Port 1 as modem control lines */
+		gpio-controller; /* Port 0 as GPIOs */
+		#gpio-cells = <2>;
+	};
+
+	sc16is752: sc16is752@54 {
+		compatible = "nxp,sc16is752";
+		reg = <0x54>;
+		clocks = <&clk20m>;
+		interrupt-parent = <&gpio3>;
+		interrupts = <7 IRQ_TYPE_EDGE_FALLING>;
+		nxp,modem-control-line-ports = <0 1>; /* Ports 0 and 1 as modem control lines */
+	};
+
 * spi as bus
 
 Required properties:
@@ -59,6 +82,9 @@  Optional properties:
     1 = active low.
 - irda-mode-ports: An array that lists the indices of the port that
 		   should operate in IrDA mode.
+- nxp,modem-control-line-ports: An array that lists the indices of the port that
+				should have shared GPIO lines configured as
+				modem control lines.
 
 Example:
 	sc16is750: sc16is750@0 {
@@ -70,3 +96,23 @@  Example:
 		gpio-controller;
 		#gpio-cells = <2>;
 	};
+
+	sc16is752: sc16is752@1 {
+		compatible = "nxp,sc16is752";
+		reg = <1>;
+		clocks = <&clk20m>;
+		interrupt-parent = <&gpio3>;
+		interrupts = <7 IRQ_TYPE_EDGE_FALLING>;
+		nxp,modem-control-line-ports = <1>; /* Port 1 as modem control lines */
+		gpio-controller; /* Port 0 as GPIOs */
+		#gpio-cells = <2>;
+	};
+
+	sc16is752: sc16is752@2 {
+		compatible = "nxp,sc16is752";
+		reg = <2>;
+		clocks = <&clk20m>;
+		interrupt-parent = <&gpio3>;
+		interrupts = <7 IRQ_TYPE_EDGE_FALLING>;
+		nxp,modem-control-line-ports = <0 1>; /* Ports 0 and 1 as modem control lines */
+	};