[PATCH RFC] serial: imx: support an enable-gpio

[PATCH RFC] serial: imx: support an enable-gpio

[Uwe Kleine-König]

A part of my machine looks as follows (simplified):

,------------------------.
| ,---------.            |
| |  imx25  o--RX----◁---o---
| |         o--GPIO--'   |
| `---------'            |
`------------------------'

that is, there is a driver on the RX line that must be enabled before
the UART can be used. (That is necessary because the default mux of the
RX pad after reset is an output.)

To represent this in the device tree I do:

	pinctrl_uart5: uart5 {
		fsl,pins = <
			...
			MX25_PAD_LBA__UART5_RXD		0x00000000
			MX25_PAD_CS5__GPIO_3_21		0x00002001
			...
	};

	&uart5 {
		pinctrl-names = "default";
		pinctrl-0 = <&pinctrl_uart5>;

		enable-gpio = <&gpio3 21 GPIO_ACTIVE_LOW>;
		...
	};

This way it's ensured that the gpio is only enabled when the LBA pad is
muxed as RX (together with the bootloader that sets the GPIO high).

Signed-off-by: Uwe Kleine-König <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
---
Hello,

I'm not sure about the naming. Do you have a better suggestion how to handle
this situation?

Best regards
Uwe

 drivers/tty/serial/imx.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 0df2b1c091ae..56eaa18aa5be 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -39,6 +39,7 @@
 #include <linux/of_device.h>
 #include <linux/io.h>
 #include <linux/dma-mapping.h>
+#include <linux/gpio/consumer.h>
 
 #include <asm/irq.h>
 #include <linux/platform_data/serial-imx.h>
@@ -1987,7 +1988,9 @@ static int serial_imx_probe_dt(struct imx_port *sport,
 	if (of_get_property(np, "fsl,dte-mode", NULL))
 		sport->dte_mode = 1;
 
-	return 0;
+	ret = PTR_ERR_OR_ZERO(devm_gpiod_get_optional(&pdev->dev, "enable", GPIOD_OUT_HIGH));
+	dev_info(&pdev->dev, "Tralala: ret = %d\n", ret);
+	return ret;
 }
 #else
 static inline int serial_imx_probe_dt(struct imx_port *sport,
-- 
2.8.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC] serial: imx: support an enable-gpio

[Uwe Kleine-König]

On Wed, Jul 13, 2016 at 11:01:35AM +0200, Uwe Kleine-König wrote:
> A part of my machine looks as follows (simplified):
> 
> ,------------------------.
> | ,---------.            |
> | |  imx25  o--RX----◁---o---
> | |         o--GPIO--'   |
> | `---------'            |
> `------------------------'
> 
> that is, there is a driver on the RX line that must be enabled before
> the UART can be used. (That is necessary because the default mux of the
> RX pad after reset is an output.)
> 
> To represent this in the device tree I do:
> 
> 	pinctrl_uart5: uart5 {
> 		fsl,pins = <
> 			...
> 			MX25_PAD_LBA__UART5_RXD		0x00000000
> 			MX25_PAD_CS5__GPIO_3_21		0x00002001
> 			...
> 	};
> 
> 	&uart5 {
> 		pinctrl-names = "default";
> 		pinctrl-0 = <&pinctrl_uart5>;
> 
> 		enable-gpio = <&gpio3 21 GPIO_ACTIVE_LOW>;
> 		...
> 	};
> 
> This way it's ensured that the gpio is only enabled when the LBA pad is
> muxed as RX (together with the bootloader that sets the GPIO high).
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> ---
> Hello,
> 
> I'm not sure about the naming. Do you have a better suggestion how to handle
> this situation?
> 
> Best regards
> Uwe
> 
>  drivers/tty/serial/imx.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 0df2b1c091ae..56eaa18aa5be 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -39,6 +39,7 @@
>  #include <linux/of_device.h>
>  #include <linux/io.h>
>  #include <linux/dma-mapping.h>
> +#include <linux/gpio/consumer.h>
>  
>  #include <asm/irq.h>
>  #include <linux/platform_data/serial-imx.h>
> @@ -1987,7 +1988,9 @@ static int serial_imx_probe_dt(struct imx_port *sport,
>  	if (of_get_property(np, "fsl,dte-mode", NULL))
>  		sport->dte_mode = 1;
>  
> -	return 0;
> +	ret = PTR_ERR_OR_ZERO(devm_gpiod_get_optional(&pdev->dev, "enable", GPIOD_OUT_HIGH));
> +	dev_info(&pdev->dev, "Tralala: ret = %d\n", ret);

That line is a debug left over and obviously should be removed. Other
than taht I still wonder if this patch is good enough for mainline as it
solves a real problem.

Best regards
Uwe

> +	return ret;
>  }
>  #else
>  static inline int serial_imx_probe_dt(struct imx_port *sport,
> -- 
> 2.8.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC] serial: imx: support an enable-gpio

[Fabio Estevam]

Hi Uwe,

On Wed, Jul 13, 2016 at 6:01 AM, Uwe Kleine-König
<u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> A part of my machine looks as follows (simplified):
>
> ,------------------------.
> | ,---------.            |
> | |  imx25  o--RX----◁---o---
> | |         o--GPIO--'   |
> | `---------'            |
> `------------------------'
>
> that is, there is a driver on the RX line that must be enabled before
> the UART can be used. (That is necessary because the default mux of the
> RX pad after reset is an output.)
>
> To represent this in the device tree I do:
>
>         pinctrl_uart5: uart5 {
>                 fsl,pins = <
>                         ...
>                         MX25_PAD_LBA__UART5_RXD         0x00000000
>                         MX25_PAD_CS5__GPIO_3_21         0x00002001
>                         ...
>         };
>
>         &uart5 {
>                 pinctrl-names = "default";
>                 pinctrl-0 = <&pinctrl_uart5>;
>
>                 enable-gpio = <&gpio3 21 GPIO_ACTIVE_LOW>;
>                 ...
>         };
>
> This way it's ensured that the gpio is only enabled when the LBA pad is
> muxed as RX (together with the bootloader that sets the GPIO high).
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>

Since this is not imx serial specific it could be made more generic.

What about extending
Documentation/devicetree/bindings/serial/slave-device.txt to handle
this GPIO, or maybe a regulator?
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC] serial: imx: support an enable-gpio

[Rob Herring]

On Mon, Apr 3, 2017 at 10:01 AM, Fabio Estevam <festevam-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Hi Uwe,
>
> On Wed, Jul 13, 2016 at 6:01 AM, Uwe Kleine-König
> <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
>> A part of my machine looks as follows (simplified):
>>
>> ,------------------------.
>> | ,---------.            |
>> | |  imx25  o--RX----◁---o---
>> | |         o--GPIO--'   |
>> | `---------'            |
>> `------------------------'
>>
>> that is, there is a driver on the RX line that must be enabled before
>> the UART can be used. (That is necessary because the default mux of the
>> RX pad after reset is an output.)
>>
>> To represent this in the device tree I do:
>>
>>         pinctrl_uart5: uart5 {
>>                 fsl,pins = <
>>                         ...
>>                         MX25_PAD_LBA__UART5_RXD         0x00000000
>>                         MX25_PAD_CS5__GPIO_3_21         0x00002001
>>                         ...
>>         };
>>
>>         &uart5 {
>>                 pinctrl-names = "default";
>>                 pinctrl-0 = <&pinctrl_uart5>;
>>
>>                 enable-gpio = <&gpio3 21 GPIO_ACTIVE_LOW>;

enable-gpios

I imagine you already know this needs documentation. Make it common please.

>>                 ...
>>         };
>>
>> This way it's ensured that the gpio is only enabled when the LBA pad is
>> muxed as RX (together with the bootloader that sets the GPIO high).
>>
>> Signed-off-by: Uwe Kleine-König <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>
> Since this is not imx serial specific it could be made more generic.
>
> What about extending
> Documentation/devicetree/bindings/serial/slave-device.txt to handle
> this GPIO, or maybe a regulator?

This is more like a phy than a device you talk to. It could also be
something like an RS-232 xcvr enable (no one has done that already?).
I think it belongs in the uart's node. You could additionally have an
enable-gpios for a slave device.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC] serial: imx: support an enable-gpio

[Uwe Kleine-König]

Hello,

On Mon, Apr 03, 2017 at 03:25:23PM -0500, Rob Herring wrote:
> On Mon, Apr 3, 2017 at 10:01 AM, Fabio Estevam <festevam-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > Hi Uwe,
> >
> > On Wed, Jul 13, 2016 at 6:01 AM, Uwe Kleine-König
> > <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> >> A part of my machine looks as follows (simplified):
> >>
> >> ,------------------------.
> >> | ,---------.            |
> >> | |  imx25  o--RX----◁---o---
> >> | |         o--GPIO--'   |
> >> | `---------'            |
> >> `------------------------'
> >>
> >> that is, there is a driver on the RX line that must be enabled before
> >> the UART can be used. (That is necessary because the default mux of the
> >> RX pad after reset is an output.)
> >>
> >> To represent this in the device tree I do:
> >>
> >>         pinctrl_uart5: uart5 {
> >>                 fsl,pins = <
> >>                         ...
> >>                         MX25_PAD_LBA__UART5_RXD         0x00000000
> >>                         MX25_PAD_CS5__GPIO_3_21         0x00002001
> >>                         ...
> >>         };
> >>
> >>         &uart5 {
> >>                 pinctrl-names = "default";
> >>                 pinctrl-0 = <&pinctrl_uart5>;
> >>
> >>                 enable-gpio = <&gpio3 21 GPIO_ACTIVE_LOW>;
> 
> enable-gpios

ack.

> I imagine you already know this needs documentation. Make it common please.

Sure, I first wanted to collect some feedback to get an idea if this
would be accepted at all.

The only candidate for common code to add this functionality would be
uart_add_one_port. I wonder if this is early enough in every case.

> >>                 ...
> >>         };
> >>
> >> This way it's ensured that the gpio is only enabled when the LBA pad is
> >> muxed as RX (together with the bootloader that sets the GPIO high).
> >>
> >> Signed-off-by: Uwe Kleine-König <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> >
> > Since this is not imx serial specific it could be made more generic.
> >
> > What about extending
> > Documentation/devicetree/bindings/serial/slave-device.txt to handle
> > this GPIO, or maybe a regulator?

I could add a regulator that would do the right thing, that would not
match the hardware though.

> This is more like a phy than a device you talk to. It could also be
> something like an RS-232 xcvr enable (no one has done that already?).
> I think it belongs in the uart's node. You could additionally have an
> enable-gpios for a slave device.

Yes, I agree here, it is better defined in the uart's node, not in the
slave node.

Is xcvr-enable-gpios or xceiver-enable-gpios a better name?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html