Linux-USB Archive on lore.kernel.org
 help / color / Atom feed
From: Johan Hovold <johan@kernel.org>
To: Manivannan Sadhasivam <mani@kernel.org>
Cc: johan@kernel.org, gregkh@linuxfoundation.org,
	linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
	patong.mxl@gmail.com, linus.walleij@linaro.org,
	mchehab+huawei@kernel.org, linux-gpio@vger.kernel.org
Subject: Re: [RESEND PATCH v4 2/3] usb: serial: xr_serial: Add gpiochip support
Date: Wed, 1 Jul 2020 15:02:06 +0200
Message-ID: <20200701130206.GD3334@localhost> (raw)
In-Reply-To: <20200607162350.21297-3-mani@kernel.org>

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

  reply index

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-07 16:23 [RESEND PATCH v4 0/3] Add support for MaxLinear/Exar USB to serial converters Manivannan Sadhasivam
2020-06-07 16:23 ` [RESEND PATCH v4 1/3] usb: serial: Add MaxLinear/Exar USB to Serial driver Manivannan Sadhasivam
2020-07-01 10:34   ` Johan Hovold
2020-07-01 13:09   ` Johan Hovold
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 [this message]
2020-06-07 16:23 ` [RESEND PATCH v4 3/3] usb: cdc-acm: Ignore Exar XR21V141X when serial driver is built Manivannan Sadhasivam

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200701130206.GD3334@localhost \
    --to=johan@kernel.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=linus.walleij@linaro.org \
    --cc=linux-gpio@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-usb@vger.kernel.org \
    --cc=mani@kernel.org \
    --cc=mchehab+huawei@kernel.org \
    --cc=patong.mxl@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-USB Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-usb/0 linux-usb/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-usb linux-usb/ https://lore.kernel.org/linux-usb \
		linux-usb@vger.kernel.org
	public-inbox-index linux-usb

Example config snippet for mirrors

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


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