| Message ID | E1hWfTn-0003fP-Rl@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Convert sa1100 serial to use mctrl gpios | expand |
On Fri, May 31, 2019 at 12:13:47PM +0100, Russell King wrote: > Add support for the generic mctrl gpio helper. This will allow us to > convert several board files to use the gpiod tables to assign GPIOs to > serial ports, rather than needing to have private function callbacks. > > If the generic mctrl gpio helper fails, ignore the mctrl gpios rather > than preventing the (possibly console) serial port from being created. > > Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Acked-by: Linus Walleij <linus.walleij@linaro.org> > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> > --- > drivers/tty/serial/Kconfig | 1 + > drivers/tty/serial/sa1100.c | 42 ++++++++++++++++++++++++++++++++++++++---- > 2 files changed, 39 insertions(+), 4 deletions(-) > > diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig > index 72966bc0ac76..f4372ac1a774 100644 > --- a/drivers/tty/serial/Kconfig > +++ b/drivers/tty/serial/Kconfig > @@ -511,6 +511,7 @@ config SERIAL_SA1100 > bool "SA1100 serial port support" > depends on ARCH_SA1100 > select SERIAL_CORE > + select SERIAL_MCTRL_GPIO if GPIOLIB > help > If you have a machine based on a SA1100/SA1110 StrongARM(R) CPU you > can enable its onboard serial port by enabling this option. > diff --git a/drivers/tty/serial/sa1100.c b/drivers/tty/serial/sa1100.c > index a399772be3fc..97bdfeccbea9 100644 > --- a/drivers/tty/serial/sa1100.c > +++ b/drivers/tty/serial/sa1100.c > @@ -28,6 +28,8 @@ > #include <mach/hardware.h> > #include <mach/irqs.h> > > +#include "serial_mctrl_gpio.h" > + > /* We've been assigned a range on the "Low-density serial ports" major */ > #define SERIAL_SA1100_MAJOR 204 > #define MINOR_START 5 > @@ -77,6 +79,7 @@ struct sa1100_port { > struct uart_port port; > struct timer_list timer; > unsigned int old_status; > + struct mctrl_gpios *gpios; > }; > > /* > @@ -174,6 +177,8 @@ static void sa1100_enable_ms(struct uart_port *port) > container_of(port, struct sa1100_port, port); > > mod_timer(&sport->timer, jiffies); > + > + mctrl_gpio_enable_ms(sport->gpios); > } > > static void > @@ -322,11 +327,21 @@ static unsigned int sa1100_tx_empty(struct uart_port *port) > > static unsigned int sa1100_get_mctrl(struct uart_port *port) > { > - return TIOCM_CTS | TIOCM_DSR | TIOCM_CAR; > + struct sa1100_port *sport = > + container_of(port, struct sa1100_port, port); > + int ret = TIOCM_CTS | TIOCM_DSR | TIOCM_CAR; > + > + mctrl_gpio_get(sport->gpios, &ret); > + > + return ret; > } > > static void sa1100_set_mctrl(struct uart_port *port, unsigned int mctrl) > { > + struct sa1100_port *sport = > + container_of(port, struct sa1100_port, port); > + > + mctrl_gpio_set(sport->gpios, mctrl); > } > > /* > @@ -842,6 +857,27 @@ static int sa1100_serial_resume(struct platform_device *dev) > return 0; > } > > +static int sa1100_serial_add_one_port(struct sa1100_port *sport, struct platform_device *dev) > +{ > + sport->port.dev = &dev->dev; > + sport->gpios = mctrl_gpio_init_noauto(sport->port.dev, 0); the _noauto function was only introduced to ease a transition. I think the driver would benefit to use mctrl_gpio_init() instead. Getting rid of mctrl_gpio_init_noauto() was on my todo list for some time, but it was pushed down too far :-| Best regards Uwe
On Fri, May 31, 2019 at 02:50:13PM +0200, Uwe Kleine-König wrote: > On Fri, May 31, 2019 at 12:13:47PM +0100, Russell King wrote: > > Add support for the generic mctrl gpio helper. This will allow us to > > convert several board files to use the gpiod tables to assign GPIOs to > > serial ports, rather than needing to have private function callbacks. > > > > If the generic mctrl gpio helper fails, ignore the mctrl gpios rather > > than preventing the (possibly console) serial port from being created. > > > > Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > Acked-by: Linus Walleij <linus.walleij@linaro.org> > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> > > --- > > drivers/tty/serial/Kconfig | 1 + > > drivers/tty/serial/sa1100.c | 42 ++++++++++++++++++++++++++++++++++++++---- > > 2 files changed, 39 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig > > index 72966bc0ac76..f4372ac1a774 100644 > > --- a/drivers/tty/serial/Kconfig > > +++ b/drivers/tty/serial/Kconfig > > @@ -511,6 +511,7 @@ config SERIAL_SA1100 > > bool "SA1100 serial port support" > > depends on ARCH_SA1100 > > select SERIAL_CORE > > + select SERIAL_MCTRL_GPIO if GPIOLIB > > help > > If you have a machine based on a SA1100/SA1110 StrongARM(R) CPU you > > can enable its onboard serial port by enabling this option. > > diff --git a/drivers/tty/serial/sa1100.c b/drivers/tty/serial/sa1100.c > > index a399772be3fc..97bdfeccbea9 100644 > > --- a/drivers/tty/serial/sa1100.c > > +++ b/drivers/tty/serial/sa1100.c > > @@ -28,6 +28,8 @@ > > #include <mach/hardware.h> > > #include <mach/irqs.h> > > > > +#include "serial_mctrl_gpio.h" > > + > > /* We've been assigned a range on the "Low-density serial ports" major */ > > #define SERIAL_SA1100_MAJOR 204 > > #define MINOR_START 5 > > @@ -77,6 +79,7 @@ struct sa1100_port { > > struct uart_port port; > > struct timer_list timer; > > unsigned int old_status; > > + struct mctrl_gpios *gpios; > > }; > > > > /* > > @@ -174,6 +177,8 @@ static void sa1100_enable_ms(struct uart_port *port) > > container_of(port, struct sa1100_port, port); > > > > mod_timer(&sport->timer, jiffies); > > + > > + mctrl_gpio_enable_ms(sport->gpios); > > } > > > > static void > > @@ -322,11 +327,21 @@ static unsigned int sa1100_tx_empty(struct uart_port *port) > > > > static unsigned int sa1100_get_mctrl(struct uart_port *port) > > { > > - return TIOCM_CTS | TIOCM_DSR | TIOCM_CAR; > > + struct sa1100_port *sport = > > + container_of(port, struct sa1100_port, port); > > + int ret = TIOCM_CTS | TIOCM_DSR | TIOCM_CAR; > > + > > + mctrl_gpio_get(sport->gpios, &ret); > > + > > + return ret; > > } > > > > static void sa1100_set_mctrl(struct uart_port *port, unsigned int mctrl) > > { > > + struct sa1100_port *sport = > > + container_of(port, struct sa1100_port, port); > > + > > + mctrl_gpio_set(sport->gpios, mctrl); > > } > > > > /* > > @@ -842,6 +857,27 @@ static int sa1100_serial_resume(struct platform_device *dev) > > return 0; > > } > > > > +static int sa1100_serial_add_one_port(struct sa1100_port *sport, struct platform_device *dev) > > +{ > > + sport->port.dev = &dev->dev; > > + sport->gpios = mctrl_gpio_init_noauto(sport->port.dev, 0); > > the _noauto function was only introduced to ease a transition. I think > the driver would benefit to use mctrl_gpio_init() instead. In what way would the driver benefit? mctrl_gpio_init() requires that there are IRQs for each input GPIO. This is not the case with most SA11x0 platforms, where the GPIO controls are implemented using simple latches, hence that interface is entirely unsuitable.
On Fri, May 31, 2019 at 02:23:40PM +0100, Russell King - ARM Linux admin wrote: > On Fri, May 31, 2019 at 02:50:13PM +0200, Uwe Kleine-König wrote: > > On Fri, May 31, 2019 at 12:13:47PM +0100, Russell King wrote: > > > +static int sa1100_serial_add_one_port(struct sa1100_port *sport, struct platform_device *dev) > > > +{ > > > + sport->port.dev = &dev->dev; > > > + sport->gpios = mctrl_gpio_init_noauto(sport->port.dev, 0); > > > > the _noauto function was only introduced to ease a transition. I think > > the driver would benefit to use mctrl_gpio_init() instead. > > In what way would the driver benefit? mctrl_gpio_init() requires that > there are IRQs for each input GPIO. This is not the case with most > SA11x0 platforms, where the GPIO controls are implemented using simple > latches, hence that interface is entirely unsuitable. Ah, but then you can only use the outputs reliably here as an edge on (say) CTS stays unnoticed with both mctrl_gpio_init() and mctrl_gpio_init_noauto(). Unless I miss something (which is quite possible given that it's quite some time ago I looked into mctrl_gpio) with mctrl_gpio_init_noauto() having a CTS-gpio is just ignored unless the modem ctrl lines are explicitely requestet while with mctrl_gpio_init() it results in an error. Isn't the error the better alternative? Best regards Uwe
On Fri, May 31, 2019 at 03:56:58PM +0200, Uwe Kleine-König wrote: > On Fri, May 31, 2019 at 02:23:40PM +0100, Russell King - ARM Linux admin wrote: > > On Fri, May 31, 2019 at 02:50:13PM +0200, Uwe Kleine-König wrote: > > > On Fri, May 31, 2019 at 12:13:47PM +0100, Russell King wrote: > > > > +static int sa1100_serial_add_one_port(struct sa1100_port *sport, struct platform_device *dev) > > > > +{ > > > > + sport->port.dev = &dev->dev; > > > > + sport->gpios = mctrl_gpio_init_noauto(sport->port.dev, 0); > > > > > > the _noauto function was only introduced to ease a transition. I think > > > the driver would benefit to use mctrl_gpio_init() instead. > > > > In what way would the driver benefit? mctrl_gpio_init() requires that > > there are IRQs for each input GPIO. This is not the case with most > > SA11x0 platforms, where the GPIO controls are implemented using simple > > latches, hence that interface is entirely unsuitable. > > Ah, but then you can only use the outputs reliably here as an edge on > (say) CTS stays unnoticed with both mctrl_gpio_init() and > mctrl_gpio_init_noauto(). Right that is a risk with a polled approach, but that is the approach that the SA1100 serial driver has taken ever since it was written almost twenty years ago, and no one has raised any concerns about that until now. > Unless I miss something (which is quite possible given that it's quite > some time ago I looked into mctrl_gpio) with mctrl_gpio_init_noauto() > having a CTS-gpio is just ignored unless the modem ctrl lines are > explicitely requestet while with mctrl_gpio_init() it results in an > error. Isn't the error the better alternative? Unless the serial driver polls the modem control line status, which the SA1100 driver continues to do in exactly the same way after this conversion. Do you suggest that we just regress the driver by ripping out this support that no one has had any problems with, and that is known to work sufficiently in its day, just because we now don't like it?
On Fri, May 31, 2019 at 03:01:28PM +0100, Russell King - ARM Linux admin wrote: > On Fri, May 31, 2019 at 03:56:58PM +0200, Uwe Kleine-König wrote: > > Unless I miss something (which is quite possible given that it's quite > > some time ago I looked into mctrl_gpio) with mctrl_gpio_init_noauto() > > having a CTS-gpio is just ignored unless the modem ctrl lines are > > explicitely requestet while with mctrl_gpio_init() it results in an > > error. Isn't the error the better alternative? > > Unless the serial driver polls the modem control line status, which > the SA1100 driver continues to do in exactly the same way after this > conversion. > > Do you suggest that we just regress the driver by ripping out this > support that no one has had any problems with, and that is known to > work sufficiently in its day, just because we now don't like it? No, of course not. A nice improvement would be to teach gpio_mctrl (or serial core?) about polling. But this is of course out of scope for this patch, so I suggest to stay with mctrl_gpio_init_noauto and document the lack of irq-capability somewhere prominently such that someone who picks up converting mctrl_gpio_init_noauto to mctrl_gpio_init notices this problem before actually hitting it. Best regards Uwe
diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig index 72966bc0ac76..f4372ac1a774 100644 --- a/drivers/tty/serial/Kconfig +++ b/drivers/tty/serial/Kconfig @@ -511,6 +511,7 @@ config SERIAL_SA1100 bool "SA1100 serial port support" depends on ARCH_SA1100 select SERIAL_CORE + select SERIAL_MCTRL_GPIO if GPIOLIB help If you have a machine based on a SA1100/SA1110 StrongARM(R) CPU you can enable its onboard serial port by enabling this option. diff --git a/drivers/tty/serial/sa1100.c b/drivers/tty/serial/sa1100.c index a399772be3fc..97bdfeccbea9 100644 --- a/drivers/tty/serial/sa1100.c +++ b/drivers/tty/serial/sa1100.c @@ -28,6 +28,8 @@ #include <mach/hardware.h> #include <mach/irqs.h> +#include "serial_mctrl_gpio.h" + /* We've been assigned a range on the "Low-density serial ports" major */ #define SERIAL_SA1100_MAJOR 204 #define MINOR_START 5 @@ -77,6 +79,7 @@ struct sa1100_port { struct uart_port port; struct timer_list timer; unsigned int old_status; + struct mctrl_gpios *gpios; }; /* @@ -174,6 +177,8 @@ static void sa1100_enable_ms(struct uart_port *port) container_of(port, struct sa1100_port, port); mod_timer(&sport->timer, jiffies); + + mctrl_gpio_enable_ms(sport->gpios); } static void @@ -322,11 +327,21 @@ static unsigned int sa1100_tx_empty(struct uart_port *port) static unsigned int sa1100_get_mctrl(struct uart_port *port) { - return TIOCM_CTS | TIOCM_DSR | TIOCM_CAR; + struct sa1100_port *sport = + container_of(port, struct sa1100_port, port); + int ret = TIOCM_CTS | TIOCM_DSR | TIOCM_CAR; + + mctrl_gpio_get(sport->gpios, &ret); + + return ret; } static void sa1100_set_mctrl(struct uart_port *port, unsigned int mctrl) { + struct sa1100_port *sport = + container_of(port, struct sa1100_port, port); + + mctrl_gpio_set(sport->gpios, mctrl); } /* @@ -842,6 +857,27 @@ static int sa1100_serial_resume(struct platform_device *dev) return 0; } +static int sa1100_serial_add_one_port(struct sa1100_port *sport, struct platform_device *dev) +{ + sport->port.dev = &dev->dev; + sport->gpios = mctrl_gpio_init_noauto(sport->port.dev, 0); + if (IS_ERR(sport->gpios)) { + int err = PTR_ERR(sport->gpios); + + dev_err(sport->port.dev, "failed to get mctrl gpios: %d\n", + err); + + if (err == -EPROBE_DEFER) + return err; + + sport->gpios = NULL; + } + + platform_set_drvdata(dev, sport); + + return uart_add_one_port(&sa1100_reg, &sport->port); +} + static int sa1100_serial_probe(struct platform_device *dev) { struct resource *res = dev->resource; @@ -856,9 +892,7 @@ static int sa1100_serial_probe(struct platform_device *dev) if (sa1100_ports[i].port.mapbase != res->start) continue; - sa1100_ports[i].port.dev = &dev->dev; - uart_add_one_port(&sa1100_reg, &sa1100_ports[i].port); - platform_set_drvdata(dev, &sa1100_ports[i]); + sa1100_serial_add_one_port(&sa1100_ports[i], dev); break; } }