Linux-GPIO Archive on lore.kernel.org
 help / color / Atom feed
* [RESEND PATCH v4 2/3] usb: serial: xr_serial: Add gpiochip support
       [not found] <20200607162350.21297-1-mani@kernel.org>
@ 2020-06-07 16:23 ` Manivannan Sadhasivam
  2020-07-01 13:02   ` Johan Hovold
  0 siblings, 1 reply; 7+ messages in thread
From: Manivannan Sadhasivam @ 2020-06-07 16:23 UTC (permalink / raw)
  To: johan, gregkh
  Cc: linux-usb, linux-kernel, patong.mxl, linus.walleij,
	mchehab+huawei, Manivannan Sadhasivam, linux-gpio

Add gpiochip support for Maxlinear/Exar USB to serial converter
for controlling the available gpios.

Inspired from cp210x usb to serial converter driver.

Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: linux-gpio@vger.kernel.org
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Manivannan Sadhasivam <mani@kernel.org>
---
 drivers/usb/serial/xr_serial.c | 209 ++++++++++++++++++++++++++++++++-
 1 file changed, 208 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/serial/xr_serial.c b/drivers/usb/serial/xr_serial.c
index bb7df79cc129..2240fbc9ea7f 100644
--- a/drivers/usb/serial/xr_serial.c
+++ b/drivers/usb/serial/xr_serial.c
@@ -10,6 +10,7 @@
  * Copyright (c) 2020 Manivannan Sadhasivam <mani@kernel.org>
  */
 
+#include <linux/gpio/driver.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/slab.h>
@@ -17,6 +18,18 @@
 #include <linux/usb.h>
 #include <linux/usb/serial.h>
 
+#ifdef CONFIG_GPIOLIB
+enum gpio_pins {
+	GPIO_RI = 0,
+	GPIO_CD,
+	GPIO_DSR,
+	GPIO_DTR,
+	GPIO_CTS,
+	GPIO_RTS,
+	GPIO_MAX,
+};
+#endif
+
 static void xr_set_termios(struct tty_struct *tty,
 			   struct usb_serial_port *port,
 			   struct ktermios *old_termios);
@@ -39,6 +52,11 @@ struct xr_uart_regs {
 };
 
 struct xr_port_private {
+#ifdef CONFIG_GPIOLIB
+	struct gpio_chip gc;
+	bool gpio_registered;
+	enum gpio_pins pin_status[GPIO_MAX];
+#endif
 	const struct xr_uart_regs *regs;
 	bool port_open;
 };
@@ -390,6 +408,13 @@ static void xr_set_flow_mode(struct tty_struct *tty,
 	 */
 	gpio_mode &= ~UART_MODE_GPIO_MASK;
 	if (cflag & CRTSCTS) {
+#ifdef CONFIG_GPIOLIB
+		/* Check if the CTS/RTS pins are occupied */
+		if (port_priv->pin_status[GPIO_RTS] ||
+		    port_priv->pin_status[GPIO_CTS])
+			return;
+#endif
+
 		dev_dbg(&port->dev, "Enabling hardware flow ctrl\n");
 		flow = UART_FLOW_MODE_HW;
 		gpio_mode |= UART_MODE_RTS_CTS;
@@ -497,6 +522,17 @@ static int xr_tiocmset_port(struct usb_serial_port *port,
 	u8 gpio_set = 0;
 	u8 gpio_clr = 0;
 
+#ifdef CONFIG_GPIOLIB
+	/* Check if the RTS/DTR pins are occupied */
+	if (set & TIOCM_RTS || clear & TIOCM_RTS)
+		if (port_priv->pin_status[GPIO_RTS])
+			return -EBUSY;
+
+	if (set & TIOCM_DTR || clear & TIOCM_DTR)
+		if (port_priv->pin_status[GPIO_DTR])
+			return -EBUSY;
+#endif
+
 	/* Modem control pins are active low, so set & clr are swapped */
 	if (set & TIOCM_RTS)
 		gpio_clr |= UART_MODE_RTS;
@@ -589,9 +625,175 @@ static void xr_break_ctl(struct tty_struct *tty, int break_state)
 		   state);
 }
 
+#ifdef CONFIG_GPIOLIB
+
+static int xr_gpio_request(struct gpio_chip *gc, unsigned int offset)
+{
+	struct usb_serial_port *port = gpiochip_get_data(gc);
+	struct xr_port_private *port_priv = usb_get_serial_port_data(port);
+
+	/*
+	 * Do not proceed if the port is open. This is done to avoid changing
+	 * the GPIO configuration used by the serial driver.
+	 */
+	if (port_priv->port_open)
+		return -EBUSY;
+
+	/* Mark the GPIO pin as busy */
+	port_priv->pin_status[offset] = true;
+
+	return 0;
+}
+
+static void xr_gpio_free(struct gpio_chip *gc, unsigned int offset)
+{
+	struct usb_serial_port *port = gpiochip_get_data(gc);
+	struct xr_port_private *port_priv = usb_get_serial_port_data(port);
+
+	/* Mark the GPIO pin as free */
+	port_priv->pin_status[offset] = false;
+}
+
+static int xr_gpio_get(struct gpio_chip *gc, unsigned int gpio)
+{
+	struct usb_serial_port *port = gpiochip_get_data(gc);
+	struct xr_port_private *port_priv = usb_get_serial_port_data(port);
+	int ret;
+	u8 gpio_status;
+
+	ret = xr_get_reg(port, XR21V141X_UART_REG_BLOCK,
+			 port_priv->regs->gpio_status, &gpio_status);
+	if (ret)
+		return ret;
+
+	return !!(gpio_status & BIT(gpio));
+}
+
+static void xr_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val)
+{
+	struct usb_serial_port *port = gpiochip_get_data(gc);
+	struct xr_port_private *port_priv = usb_get_serial_port_data(port);
+
+	if (val)
+		xr_set_reg(port, XR21V141X_UART_REG_BLOCK,
+			   port_priv->regs->gpio_set, BIT(gpio));
+	else
+		xr_set_reg(port, XR21V141X_UART_REG_BLOCK,
+			   port_priv->regs->gpio_clr, BIT(gpio));
+}
+
+static int xr_gpio_direction_get(struct gpio_chip *gc, unsigned int gpio)
+{
+	struct usb_serial_port *port = gpiochip_get_data(gc);
+	struct xr_port_private *port_priv = usb_get_serial_port_data(port);
+	int ret;
+	u8 gpio_dir;
+
+	ret = xr_get_reg(port, XR21V141X_UART_REG_BLOCK,
+			 port_priv->regs->gpio_dir, &gpio_dir);
+	if (ret)
+		return ret;
+
+	/* Logic 0 = input and Logic 1 = output */
+	return !(gpio_dir & BIT(gpio));
+}
+
+static int xr_gpio_direction_input(struct gpio_chip *gc, unsigned int gpio)
+{
+	struct usb_serial_port *port = gpiochip_get_data(gc);
+	struct xr_port_private *port_priv = usb_get_serial_port_data(port);
+	int ret;
+	u8 gpio_dir;
+
+	ret = xr_get_reg(port, XR21V141X_UART_REG_BLOCK,
+			 port_priv->regs->gpio_dir, &gpio_dir);
+	if (ret)
+		return ret;
+
+	gpio_dir &= ~BIT(gpio);
+
+	return xr_set_reg(port, XR21V141X_UART_REG_BLOCK,
+			  port_priv->regs->gpio_dir, gpio_dir);
+}
+
+static int xr_gpio_direction_output(struct gpio_chip *gc, unsigned int gpio,
+				    int val)
+{
+	struct usb_serial_port *port = gpiochip_get_data(gc);
+	struct xr_port_private *port_priv = usb_get_serial_port_data(port);
+	int ret;
+	u8 gpio_dir;
+
+	xr_gpio_set(gc, gpio, val);
+
+	ret = xr_get_reg(port, XR21V141X_UART_REG_BLOCK,
+			 port_priv->regs->gpio_dir, &gpio_dir);
+	if (ret)
+		return ret;
+
+	gpio_dir |= BIT(gpio);
+
+	ret = xr_set_reg(port, XR21V141X_UART_REG_BLOCK,
+			 port_priv->regs->gpio_dir, gpio_dir);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int xr_gpio_init(struct usb_serial_port *port)
+{
+	struct xr_port_private *port_priv = usb_get_serial_port_data(port);
+	int ret = 0;
+
+	port_priv->gc.ngpio = 6;
+	port_priv->gc.label = "xr_gpios";
+	port_priv->gc.request = xr_gpio_request;
+	port_priv->gc.free = xr_gpio_free;
+	port_priv->gc.get_direction = xr_gpio_direction_get;
+	port_priv->gc.direction_input = xr_gpio_direction_input;
+	port_priv->gc.direction_output = xr_gpio_direction_output;
+	port_priv->gc.get = xr_gpio_get;
+	port_priv->gc.set = xr_gpio_set;
+	port_priv->gc.owner = THIS_MODULE;
+	port_priv->gc.parent = &port->dev;
+	port_priv->gc.base = -1;
+	port_priv->gc.can_sleep = true;
+
+	ret = gpiochip_add_data(&port_priv->gc, port);
+	if (!ret)
+		port_priv->gpio_registered = true;
+
+	return ret;
+}
+
+static void xr_gpio_remove(struct usb_serial_port *port)
+{
+	struct xr_port_private *port_priv = usb_get_serial_port_data(port);
+
+	if (port_priv->gpio_registered) {
+		gpiochip_remove(&port_priv->gc);
+		port_priv->gpio_registered = false;
+	}
+}
+
+#else
+
+static int xr_gpio_init(struct usb_serial_port *port)
+{
+	return 0;
+}
+
+static void xr_gpio_remove(struct usb_serial_port *port)
+{
+}
+
+#endif
+
 static int xr_port_probe(struct usb_serial_port *port)
 {
 	struct xr_port_private *port_priv;
+	int ret;
 
 	port_priv = kzalloc(sizeof(*port_priv), GFP_KERNEL);
 	if (!port_priv)
@@ -602,13 +804,18 @@ static int xr_port_probe(struct usb_serial_port *port)
 
 	usb_set_serial_port_data(port, port_priv);
 
-	return 0;
+	ret = xr_gpio_init(port);
+	if (ret)
+		kfree(port_priv);
+
+	return ret;
 }
 
 static int xr_port_remove(struct usb_serial_port *port)
 {
 	struct xr_port_private *port_priv = usb_get_serial_port_data(port);
 
+	xr_gpio_remove(port);
 	kfree(port_priv);
 
 	return 0;
-- 
2.17.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RESEND PATCH v4 2/3] usb: serial: xr_serial: Add gpiochip support
  2020-06-07 16:23 ` [RESEND PATCH v4 2/3] usb: serial: xr_serial: Add gpiochip support Manivannan Sadhasivam
@ 2020-07-01 13:02   ` Johan Hovold
  2020-07-26 15:52     ` Manivannan Sadhasivam
  0 siblings, 1 reply; 7+ messages in thread
From: Johan Hovold @ 2020-07-01 13:02 UTC (permalink / raw)
  To: Manivannan Sadhasivam
  Cc: johan, gregkh, linux-usb, linux-kernel, patong.mxl,
	linus.walleij, mchehab+huawei, linux-gpio

On Sun, Jun 07, 2020 at 09:53:49PM +0530, Manivannan Sadhasivam wrote:
> Add gpiochip support for Maxlinear/Exar USB to serial converter
> for controlling the available gpios.
> 
> Inspired from cp210x usb to serial converter driver.
> 
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: linux-gpio@vger.kernel.org
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Manivannan Sadhasivam <mani@kernel.org>
> ---
>  drivers/usb/serial/xr_serial.c | 209 ++++++++++++++++++++++++++++++++-
>  1 file changed, 208 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/serial/xr_serial.c b/drivers/usb/serial/xr_serial.c
> index bb7df79cc129..2240fbc9ea7f 100644
> --- a/drivers/usb/serial/xr_serial.c
> +++ b/drivers/usb/serial/xr_serial.c
> @@ -10,6 +10,7 @@
>   * Copyright (c) 2020 Manivannan Sadhasivam <mani@kernel.org>
>   */
>  
> +#include <linux/gpio/driver.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/slab.h>
> @@ -17,6 +18,18 @@
>  #include <linux/usb.h>
>  #include <linux/usb/serial.h>
>  
> +#ifdef CONFIG_GPIOLIB
> +enum gpio_pins {
> +	GPIO_RI = 0,
> +	GPIO_CD,
> +	GPIO_DSR,
> +	GPIO_DTR,
> +	GPIO_CTS,
> +	GPIO_RTS,
> +	GPIO_MAX,
> +};
> +#endif

Try to avoid littering the driver with GPIOLIB ifdefs. One or two is
fine, but no more even if it means declaring an unused type. Add
stubbed out helpers where appropriate.

> +
>  static void xr_set_termios(struct tty_struct *tty,
>  			   struct usb_serial_port *port,
>  			   struct ktermios *old_termios);
> @@ -39,6 +52,11 @@ struct xr_uart_regs {
>  };
>  
>  struct xr_port_private {
> +#ifdef CONFIG_GPIOLIB
> +	struct gpio_chip gc;
> +	bool gpio_registered;
> +	enum gpio_pins pin_status[GPIO_MAX];
> +#endif
>  	const struct xr_uart_regs *regs;
>  	bool port_open;
>  };
> @@ -390,6 +408,13 @@ static void xr_set_flow_mode(struct tty_struct *tty,
>  	 */
>  	gpio_mode &= ~UART_MODE_GPIO_MASK;
>  	if (cflag & CRTSCTS) {
> +#ifdef CONFIG_GPIOLIB
> +		/* Check if the CTS/RTS pins are occupied */
> +		if (port_priv->pin_status[GPIO_RTS] ||
> +		    port_priv->pin_status[GPIO_CTS])
> +			return;
> +#endif

You cannot just bail out as this could leave software flow enabled etc.

You also need to claim these pins once at open or leave them be. We
don't want CRTSCTS to suddenly start toggling because a pin is released
by gpiolib.

That is, determine who owns each pin at open() and keep it that way till
close() (by setting some flags at open).

> +
>  		dev_dbg(&port->dev, "Enabling hardware flow ctrl\n");
>  		flow = UART_FLOW_MODE_HW;
>  		gpio_mode |= UART_MODE_RTS_CTS;
> @@ -497,6 +522,17 @@ static int xr_tiocmset_port(struct usb_serial_port *port,
>  	u8 gpio_set = 0;
>  	u8 gpio_clr = 0;
>  
> +#ifdef CONFIG_GPIOLIB
> +	/* Check if the RTS/DTR pins are occupied */
> +	if (set & TIOCM_RTS || clear & TIOCM_RTS)
> +		if (port_priv->pin_status[GPIO_RTS])
> +			return -EBUSY;
> +
> +	if (set & TIOCM_DTR || clear & TIOCM_DTR)
> +		if (port_priv->pin_status[GPIO_DTR])
> +			return -EBUSY;
> +#endif

Same here. And perhaps just ignoring the pins managed by gpiolib is
better (cf. gpiolib and pinctrl being orthogonal).

> +
>  	/* Modem control pins are active low, so set & clr are swapped */
>  	if (set & TIOCM_RTS)
>  		gpio_clr |= UART_MODE_RTS;
> @@ -589,9 +625,175 @@ static void xr_break_ctl(struct tty_struct *tty, int break_state)
>  		   state);
>  }
>  
> +#ifdef CONFIG_GPIOLIB
> +
> +static int xr_gpio_request(struct gpio_chip *gc, unsigned int offset)
> +{
> +	struct usb_serial_port *port = gpiochip_get_data(gc);
> +	struct xr_port_private *port_priv = usb_get_serial_port_data(port);
> +
> +	/*
> +	 * Do not proceed if the port is open. This is done to avoid changing
> +	 * the GPIO configuration used by the serial driver.
> +	 */
> +	if (port_priv->port_open)
> +		return -EBUSY;
> +
> +	/* Mark the GPIO pin as busy */
> +	port_priv->pin_status[offset] = true;

You need a lock to serialise against open/close properly.

> +
> +	return 0;
> +}
> +
> +static void xr_gpio_free(struct gpio_chip *gc, unsigned int offset)
> +{
> +	struct usb_serial_port *port = gpiochip_get_data(gc);
> +	struct xr_port_private *port_priv = usb_get_serial_port_data(port);
> +
> +	/* Mark the GPIO pin as free */
> +	port_priv->pin_status[offset] = false;
> +}

Johan

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RESEND PATCH v4 2/3] usb: serial: xr_serial: Add gpiochip support
  2020-07-01 13:02   ` Johan Hovold
@ 2020-07-26 15:52     ` Manivannan Sadhasivam
  2020-07-26 16:34       ` Andy Shevchenko
  2020-08-06 13:53       ` Johan Hovold
  0 siblings, 2 replies; 7+ messages in thread
From: Manivannan Sadhasivam @ 2020-07-26 15:52 UTC (permalink / raw)
  To: Johan Hovold
  Cc: gregkh, linux-usb, linux-kernel, patong.mxl, linus.walleij,
	mchehab+huawei, linux-gpio

On Wed, Jul 01, 2020 at 03:02:06PM +0200, Johan Hovold wrote:
> On Sun, Jun 07, 2020 at 09:53:49PM +0530, Manivannan Sadhasivam wrote:
> > Add gpiochip support for Maxlinear/Exar USB to serial converter
> > for controlling the available gpios.
> > 
> > Inspired from cp210x usb to serial converter driver.
> > 
> > Cc: Linus Walleij <linus.walleij@linaro.org>
> > Cc: linux-gpio@vger.kernel.org
> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> > Signed-off-by: Manivannan Sadhasivam <mani@kernel.org>
> > ---
> >  drivers/usb/serial/xr_serial.c | 209 ++++++++++++++++++++++++++++++++-
> >  1 file changed, 208 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/usb/serial/xr_serial.c b/drivers/usb/serial/xr_serial.c
> > index bb7df79cc129..2240fbc9ea7f 100644
> > --- a/drivers/usb/serial/xr_serial.c
> > +++ b/drivers/usb/serial/xr_serial.c
> > @@ -10,6 +10,7 @@
> >   * Copyright (c) 2020 Manivannan Sadhasivam <mani@kernel.org>
> >   */
> >  
> > +#include <linux/gpio/driver.h>
> >  #include <linux/kernel.h>
> >  #include <linux/module.h>
> >  #include <linux/slab.h>
> > @@ -17,6 +18,18 @@
> >  #include <linux/usb.h>
> >  #include <linux/usb/serial.h>
> >  
> > +#ifdef CONFIG_GPIOLIB
> > +enum gpio_pins {
> > +	GPIO_RI = 0,
> > +	GPIO_CD,
> > +	GPIO_DSR,
> > +	GPIO_DTR,
> > +	GPIO_CTS,
> > +	GPIO_RTS,
> > +	GPIO_MAX,
> > +};
> > +#endif
> 
> Try to avoid littering the driver with GPIOLIB ifdefs. One or two is
> fine, but no more even if it means declaring an unused type. Add
> stubbed out helpers where appropriate.
> 
> > +
> >  static void xr_set_termios(struct tty_struct *tty,
> >  			   struct usb_serial_port *port,
> >  			   struct ktermios *old_termios);
> > @@ -39,6 +52,11 @@ struct xr_uart_regs {
> >  };
> >  
> >  struct xr_port_private {
> > +#ifdef CONFIG_GPIOLIB
> > +	struct gpio_chip gc;
> > +	bool gpio_registered;
> > +	enum gpio_pins pin_status[GPIO_MAX];
> > +#endif
> >  	const struct xr_uart_regs *regs;
> >  	bool port_open;
> >  };
> > @@ -390,6 +408,13 @@ static void xr_set_flow_mode(struct tty_struct *tty,
> >  	 */
> >  	gpio_mode &= ~UART_MODE_GPIO_MASK;
> >  	if (cflag & CRTSCTS) {
> > +#ifdef CONFIG_GPIOLIB
> > +		/* Check if the CTS/RTS pins are occupied */
> > +		if (port_priv->pin_status[GPIO_RTS] ||
> > +		    port_priv->pin_status[GPIO_CTS])
> > +			return;
> > +#endif
> 
> You cannot just bail out as this could leave software flow enabled etc.
> 
> You also need to claim these pins once at open or leave them be. We
> don't want CRTSCTS to suddenly start toggling because a pin is released
> by gpiolib.
> 
> That is, determine who owns each pin at open() and keep it that way till
> close() (by setting some flags at open).
> 
> > +
> >  		dev_dbg(&port->dev, "Enabling hardware flow ctrl\n");
> >  		flow = UART_FLOW_MODE_HW;
> >  		gpio_mode |= UART_MODE_RTS_CTS;
> > @@ -497,6 +522,17 @@ static int xr_tiocmset_port(struct usb_serial_port *port,
> >  	u8 gpio_set = 0;
> >  	u8 gpio_clr = 0;
> >  
> > +#ifdef CONFIG_GPIOLIB
> > +	/* Check if the RTS/DTR pins are occupied */
> > +	if (set & TIOCM_RTS || clear & TIOCM_RTS)
> > +		if (port_priv->pin_status[GPIO_RTS])
> > +			return -EBUSY;
> > +
> > +	if (set & TIOCM_DTR || clear & TIOCM_DTR)
> > +		if (port_priv->pin_status[GPIO_DTR])
> > +			return -EBUSY;
> > +#endif
> 
> Same here. And perhaps just ignoring the pins managed by gpiolib is
> better (cf. gpiolib and pinctrl being orthogonal).

You mean, we can just make TX,RX,CTS,RTS pins controlled only by the serial
driver and the rest only by gpiolib?

Thanks,
Mani

> 
> > +
> >  	/* Modem control pins are active low, so set & clr are swapped */
> >  	if (set & TIOCM_RTS)
> >  		gpio_clr |= UART_MODE_RTS;
> > @@ -589,9 +625,175 @@ static void xr_break_ctl(struct tty_struct *tty, int break_state)
> >  		   state);
> >  }
> >  
> > +#ifdef CONFIG_GPIOLIB
> > +
> > +static int xr_gpio_request(struct gpio_chip *gc, unsigned int offset)
> > +{
> > +	struct usb_serial_port *port = gpiochip_get_data(gc);
> > +	struct xr_port_private *port_priv = usb_get_serial_port_data(port);
> > +
> > +	/*
> > +	 * Do not proceed if the port is open. This is done to avoid changing
> > +	 * the GPIO configuration used by the serial driver.
> > +	 */
> > +	if (port_priv->port_open)
> > +		return -EBUSY;
> > +
> > +	/* Mark the GPIO pin as busy */
> > +	port_priv->pin_status[offset] = true;
> 
> You need a lock to serialise against open/close properly.
> 
> > +
> > +	return 0;
> > +}
> > +
> > +static void xr_gpio_free(struct gpio_chip *gc, unsigned int offset)
> > +{
> > +	struct usb_serial_port *port = gpiochip_get_data(gc);
> > +	struct xr_port_private *port_priv = usb_get_serial_port_data(port);
> > +
> > +	/* Mark the GPIO pin as free */
> > +	port_priv->pin_status[offset] = false;
> > +}
> 
> Johan

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RESEND PATCH v4 2/3] usb: serial: xr_serial: Add gpiochip support
  2020-07-26 15:52     ` Manivannan Sadhasivam
@ 2020-07-26 16:34       ` Andy Shevchenko
  2020-07-27  4:46         ` Manivannan Sadhasivam
  2020-08-06 13:53       ` Johan Hovold
  1 sibling, 1 reply; 7+ messages in thread
From: Andy Shevchenko @ 2020-07-26 16:34 UTC (permalink / raw)
  To: Manivannan Sadhasivam
  Cc: Johan Hovold, Greg Kroah-Hartman, USB, Linux Kernel Mailing List,
	patong.mxl, Linus Walleij, Mauro Carvalho Chehab,
	open list:GPIO SUBSYSTEM

On Sun, Jul 26, 2020 at 6:53 PM Manivannan Sadhasivam <mani@kernel.org> wrote:
> On Wed, Jul 01, 2020 at 03:02:06PM +0200, Johan Hovold wrote:
> > On Sun, Jun 07, 2020 at 09:53:49PM +0530, Manivannan Sadhasivam wrote:

...

> > Same here. And perhaps just ignoring the pins managed by gpiolib is
> > better (cf. gpiolib and pinctrl being orthogonal).
>
> You mean, we can just make TX,RX,CTS,RTS pins controlled only by the serial
> driver and the rest only by gpiolib?

I'm wondering if you may use mctrl_gpio_*() API instead.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RESEND PATCH v4 2/3] usb: serial: xr_serial: Add gpiochip support
  2020-07-26 16:34       ` Andy Shevchenko
@ 2020-07-27  4:46         ` Manivannan Sadhasivam
  2020-07-27  7:50           ` Andy Shevchenko
  0 siblings, 1 reply; 7+ messages in thread
From: Manivannan Sadhasivam @ 2020-07-27  4:46 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Johan Hovold, Greg Kroah-Hartman, USB, Linux Kernel Mailing List,
	patong.mxl, Linus Walleij, Mauro Carvalho Chehab,
	open list:GPIO SUBSYSTEM

On Sun, Jul 26, 2020 at 07:34:54PM +0300, Andy Shevchenko wrote:
> On Sun, Jul 26, 2020 at 6:53 PM Manivannan Sadhasivam <mani@kernel.org> wrote:
> > On Wed, Jul 01, 2020 at 03:02:06PM +0200, Johan Hovold wrote:
> > > On Sun, Jun 07, 2020 at 09:53:49PM +0530, Manivannan Sadhasivam wrote:
> 
> ...
> 
> > > Same here. And perhaps just ignoring the pins managed by gpiolib is
> > > better (cf. gpiolib and pinctrl being orthogonal).
> >
> > You mean, we can just make TX,RX,CTS,RTS pins controlled only by the serial
> > driver and the rest only by gpiolib?
> 
> I'm wondering if you may use mctrl_gpio_*() API instead.

How? mctrl_gpio APIs are a wrapper for accessing modem control gpio pins but
here we are not accessing the pins but rather exposing the pins as a gpiochip.
Am I missing something?

Thanks,
Mani

> 
> -- 
> With Best Regards,
> Andy Shevchenko

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RESEND PATCH v4 2/3] usb: serial: xr_serial: Add gpiochip support
  2020-07-27  4:46         ` Manivannan Sadhasivam
@ 2020-07-27  7:50           ` Andy Shevchenko
  0 siblings, 0 replies; 7+ messages in thread
From: Andy Shevchenko @ 2020-07-27  7:50 UTC (permalink / raw)
  To: Manivannan Sadhasivam
  Cc: Johan Hovold, Greg Kroah-Hartman, USB, Linux Kernel Mailing List,
	patong.mxl, Linus Walleij, Mauro Carvalho Chehab,
	open list:GPIO SUBSYSTEM

On Mon, Jul 27, 2020 at 7:46 AM Manivannan Sadhasivam <mani@kernel.org> wrote:
> On Sun, Jul 26, 2020 at 07:34:54PM +0300, Andy Shevchenko wrote:
> > On Sun, Jul 26, 2020 at 6:53 PM Manivannan Sadhasivam <mani@kernel.org> wrote:

...

> > I'm wondering if you may use mctrl_gpio_*() API instead.
>
> How? mctrl_gpio APIs are a wrapper for accessing modem control gpio pins but
> here we are not accessing the pins but rather exposing the pins as a gpiochip.

I see. Thanks for the explanation.

> Am I missing something?

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RESEND PATCH v4 2/3] usb: serial: xr_serial: Add gpiochip support
  2020-07-26 15:52     ` Manivannan Sadhasivam
  2020-07-26 16:34       ` Andy Shevchenko
@ 2020-08-06 13:53       ` Johan Hovold
  1 sibling, 0 replies; 7+ messages in thread
From: Johan Hovold @ 2020-08-06 13:53 UTC (permalink / raw)
  To: Manivannan Sadhasivam
  Cc: Johan Hovold, gregkh, linux-usb, linux-kernel, patong.mxl,
	linus.walleij, mchehab+huawei, linux-gpio

On Sun, Jul 26, 2020 at 09:22:23PM +0530, Manivannan Sadhasivam wrote:
> On Wed, Jul 01, 2020 at 03:02:06PM +0200, Johan Hovold wrote:
> > On Sun, Jun 07, 2020 at 09:53:49PM +0530, Manivannan Sadhasivam wrote:
> > > Add gpiochip support for Maxlinear/Exar USB to serial converter
> > > for controlling the available gpios.
> > > 
> > > Inspired from cp210x usb to serial converter driver.
> > > 
> > > Cc: Linus Walleij <linus.walleij@linaro.org>
> > > Cc: linux-gpio@vger.kernel.org
> > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> > > Signed-off-by: Manivannan Sadhasivam <mani@kernel.org>
> > > ---
> > >  drivers/usb/serial/xr_serial.c | 209 ++++++++++++++++++++++++++++++++-
> > >  1 file changed, 208 insertions(+), 1 deletion(-)

> > > @@ -390,6 +408,13 @@ static void xr_set_flow_mode(struct tty_struct *tty,
> > >  	 */
> > >  	gpio_mode &= ~UART_MODE_GPIO_MASK;
> > >  	if (cflag & CRTSCTS) {
> > > +#ifdef CONFIG_GPIOLIB
> > > +		/* Check if the CTS/RTS pins are occupied */
> > > +		if (port_priv->pin_status[GPIO_RTS] ||
> > > +		    port_priv->pin_status[GPIO_CTS])
> > > +			return;
> > > +#endif
> > 
> > You cannot just bail out as this could leave software flow enabled etc.
> > 
> > You also need to claim these pins once at open or leave them be. We
> > don't want CRTSCTS to suddenly start toggling because a pin is released
> > by gpiolib.
> > 
> > That is, determine who owns each pin at open() and keep it that way till
> > close() (by setting some flags at open).
> > 
> > > +
> > >  		dev_dbg(&port->dev, "Enabling hardware flow ctrl\n");
> > >  		flow = UART_FLOW_MODE_HW;
> > >  		gpio_mode |= UART_MODE_RTS_CTS;
> > > @@ -497,6 +522,17 @@ static int xr_tiocmset_port(struct usb_serial_port *port,
> > >  	u8 gpio_set = 0;
> > >  	u8 gpio_clr = 0;
> > >  
> > > +#ifdef CONFIG_GPIOLIB
> > > +	/* Check if the RTS/DTR pins are occupied */
> > > +	if (set & TIOCM_RTS || clear & TIOCM_RTS)
> > > +		if (port_priv->pin_status[GPIO_RTS])
> > > +			return -EBUSY;
> > > +
> > > +	if (set & TIOCM_DTR || clear & TIOCM_DTR)
> > > +		if (port_priv->pin_status[GPIO_DTR])
> > > +			return -EBUSY;
> > > +#endif
> > 
> > Same here. And perhaps just ignoring the pins managed by gpiolib is
> > better (cf. gpiolib and pinctrl being orthogonal).
> 
> You mean, we can just make TX,RX,CTS,RTS pins controlled only by the serial
> driver and the rest only by gpiolib?

No, I think I just meant that you shouldn't return an error here for
signals whose pins happen to be muxed for a different function (gpio).

Johan

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, back to index

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20200607162350.21297-1-mani@kernel.org>
2020-06-07 16:23 ` [RESEND PATCH v4 2/3] usb: serial: xr_serial: Add gpiochip support Manivannan Sadhasivam
2020-07-01 13:02   ` Johan Hovold
2020-07-26 15:52     ` Manivannan Sadhasivam
2020-07-26 16:34       ` Andy Shevchenko
2020-07-27  4:46         ` Manivannan Sadhasivam
2020-07-27  7:50           ` Andy Shevchenko
2020-08-06 13:53       ` Johan Hovold

Linux-GPIO Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-gpio/0 linux-gpio/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-gpio linux-gpio/ https://lore.kernel.org/linux-gpio \
		linux-gpio@vger.kernel.org
	public-inbox-index linux-gpio

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-gpio


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git