Re: [PATCH 3/3] USB: serial: ftdi_sio: add support for FreeCalypso DUART28C adapter

[Johan Hovold <johan@kernel.org>]

On Wed, Sep 16, 2020 at 01:56:44AM +0000, Mychaela N. Falconia wrote:
> FreeCalypso DUART28C is an FT2232D-based USB to dual UART adapter
> with a special quirk: Channel B RTS and DTR outputs (BDBUS2 and BDBUS4
> on the chip) have been repurposed to drive power and reset controls
> on Calypso targets.  The circuit is wired such that BDBUS[24] high
> (RTS/DTR inactive) is the normal state with power/reset control
> NOT activated, whereas BDBUS[24] low (RTS or DTR active) turn ON
> the corresponding power/reset control drivers.
> 
> A special ftdi_sio driver quirk is needed in order to suppress
> automatic assertion of DTR & RTS on device open: this device's
> special power and reset control drivers MUST NOT be activated
> when the port is ordinarily opened for plain serial communication,
> instead they must only be activated when a special userspace
> application explicitly requests such activation with a TIOCMBIS ioctl.
> 
> The special quirk is conditionalized on the DUART28C adapter's custom
> USB ID, and is further limited to FT2232D Channel B only: Channel A
> is wired normally, with the chip's ADBUS2 and ADBUS4 outputs
> actually being RTS and DTR rather than something else.
> 
> Signed-off-by: Mychaela N. Falconia <falcon@freecalypso.org>
> ---
>  drivers/usb/serial/ftdi_sio.c | 51 +++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 47 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
> index cdf4f4e05fb2..73c6b4e2c3e4 100644
> --- a/drivers/usb/serial/ftdi_sio.c
> +++ b/drivers/usb/serial/ftdi_sio.c
> @@ -69,6 +69,8 @@ struct ftdi_private {
>  				   this value */
>  	int force_rtscts;	/* if non-zero, force RTS-CTS to always
>  				   be enabled */
> +	int no_auto_dtr_rts;	/* if non-zero, suppress automatic assertion
> +				   of DTR & RTS on device open */
>  
>  	unsigned int latency;		/* latency setting in use */
>  	unsigned short max_packet_size;
> @@ -99,6 +101,8 @@ static void  ftdi_USB_UIRT_setup(struct usb_serial_port *port,
>  				 struct ftdi_private *priv);
>  static void  ftdi_HE_TIRA1_setup(struct usb_serial_port *port,
>  				 struct ftdi_private *priv);
> +static void  ftdi_duart28c_setup(struct usb_serial_port *port,
> +				 struct ftdi_private *priv);
>  
>  static const struct ftdi_sio_quirk ftdi_jtag_quirk = {
>  	.probe	= ftdi_jtag_probe,
> @@ -124,6 +128,10 @@ static const struct ftdi_sio_quirk ftdi_8u2232c_quirk = {
>  	.probe	= ftdi_8u2232c_probe,
>  };
>  
> +static const struct ftdi_sio_quirk ftdi_duart28c_quirk = {
> +	.port_probe = ftdi_duart28c_setup,
> +};
> +
>  /*
>   * The 8U232AM has the same API as the sio except for:
>   * - it can support MUCH higher baudrates; up to:
> @@ -1044,6 +1052,8 @@ static const struct usb_device_id id_table_combined[] = {
>  		.driver_info = (kernel_ulong_t)&ftdi_jtag_quirk },
>  	{ USB_DEVICE(FTDI_VID, FTDI_FALCONIA_JTAG_UNBUF_PID),
>  		.driver_info = (kernel_ulong_t)&ftdi_jtag_quirk },
> +	{ USB_DEVICE(FTDI_VID, FTDI_FALCONIA_DUART28C_PID),
> +		.driver_info = (kernel_ulong_t)&ftdi_duart28c_quirk },

Make sure that this patch adds the dual-port PID define as well.

>  	{ }					/* Terminating entry */
>  };
>  
> @@ -2388,6 +2398,37 @@ static int ftdi_stmclite_probe(struct usb_serial *serial)
>  	return 0;
>  }
>  
> +/*
> + * FreeCalypso DUART28C is an FT2232D-based USB to dual UART adapter
> + * with a special quirk: Channel B RTS and DTR outputs (BDBUS2 and BDBUS4
> + * on the chip) have been repurposed to drive power and reset controls
> + * on Calypso targets.  The circuit is wired such that BDBUS[24] high
> + * (RTS/DTR inactive) is the normal state with power/reset control
> + * NOT activated, whereas BDBUS[24] low (RTS or DTR active) turn ON
> + * the corresponding power/reset control drivers.
> + *
> + * A special ftdi_sio driver quirk is needed in order to suppress
> + * automatic assertion of DTR & RTS on device open: this device's
> + * special power and reset control drivers MUST NOT be activated
> + * when the port is ordinarily opened for plain serial communication,
> + * instead they must only be activated when a special userspace
> + * application explicitly requests such activation with a TIOCMBIS ioctl.
> + *
> + * The special quirk must be applied only to FT2232D Channel B:
> + * Channel A is wired normally, with the chip's ADBUS2 and ADBUS4 outputs
> + * actually being RTS and DTR rather than something else.
> + */
> +static void ftdi_duart28c_setup(struct usb_serial_port *port,
> +				struct ftdi_private *priv)
> +{
> +	struct usb_serial *serial = port->serial;
> +	struct usb_device *udev = serial->dev;
> +	struct usb_interface *interface = serial->interface;
> +
> +	if (interface == udev->actconfig->interface[1])

Driver's shouldn't rely on the order of these pointers. I've prepared a
patch to fix up the other instances in this driver, and please use
bNumInterfaces here as well.

> +		priv->no_auto_dtr_rts = 1;
> +}
> +
>  static int ftdi_sio_port_remove(struct usb_serial_port *port)
>  {
>  	struct ftdi_private *priv = usb_get_serial_port_data(port);
> @@ -2440,9 +2481,10 @@ static void ftdi_dtr_rts(struct usb_serial_port *port, int on)
>  		}
>  	}
>  	/* drop RTS and DTR */
> -	if (on)
> -		set_mctrl(port, TIOCM_DTR | TIOCM_RTS);
> -	else
> +	if (on) {
> +		if (!priv->no_auto_dtr_rts)
> +			set_mctrl(port, TIOCM_DTR | TIOCM_RTS);
> +	} else
>  		clear_mctrl(port, TIOCM_DTR | TIOCM_RTS);

If we are to add this, then you shouldn't allow for automatic deassert
either.

>  }
>  
> @@ -2790,7 +2832,8 @@ static void ftdi_set_termios(struct tty_struct *tty,
>  			dev_err(ddev, "%s urb failed to set baudrate\n", __func__);
>  		mutex_unlock(&priv->cfg_lock);
>  		/* Ensure RTS and DTR are raised when baudrate changed from 0 */
> -		if (old_termios && (old_termios->c_cflag & CBAUD) == B0)
> +		if (old_termios && (old_termios->c_cflag & CBAUD) == B0
> +		    && !priv->no_auto_dtr_rts)
>  			set_mctrl(port, TIOCM_DTR | TIOCM_RTS);

Same here.

>  	}

And then there's CRTSCTS of course...

Johan