Re: [PATCH v2 3/7] USB: serial: xr: add support for multi-port XR21V141X variants

[Johan Hovold]

On Wed, Mar 24, 2021 at 08:41:07AM +0100, Mauro Carvalho Chehab wrote:
> XR21V1412 and XR21V1414 have exactly the same interface, but
> they support multiple 2 and 4 ports, respectively.
> 
> On such devices, the "CDC Union" field shows how they're
> grouped, as can be seen on those lsusb -v outputs:
> 
> 	https://linux-usb.vger.kernel.narkive.com/YaTYwHkM/usb-uart-device-from-exar-co-not-working-with-cdc-acm-but-usbserial
> 	https://jquery.developreference.com/article/22043997/udev+rule+with+bInterfaceNumber+doesn't+work+%5bclosed%5d
> 
> So, for instance, on XR21V1414, (0x04e2:0x1414), the 3rd
> port is:
> 
> 	CDC Union:
> 		bMasterInterface 4
> 		bSlaveInterface 5
> 
> 	CDC Call Management:
> 		bmCapabilities 0x01
> 			call management
> 				bDataInterface 5
> 
> In other words, the control interface is an even number,
> and the data interface is the next odd number.
> 
> The logic to get the proper register for an specific channel
> came from this patch:
> 
> 	https://lore.kernel.org/linux-usb/20180404070634.nhspvmxcjwfgjkcv@advantechmxl-desktop
> 
> Add support for them.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx>
> ---
>  drivers/usb/serial/xr_serial.c | 30 ++++++++++++++++++++++++++++--
>  1 file changed, 28 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/serial/xr_serial.c b/drivers/usb/serial/xr_serial.c
> index 518c4725431a..f161394d9b2d 100644
> --- a/drivers/usb/serial/xr_serial.c
> +++ b/drivers/usb/serial/xr_serial.c
> @@ -145,6 +145,7 @@ static const int xr_hal_table[MAX_XR_MODELS][MAX_XR_HAL_TYPE] = {
>  
>  struct xr_port_private {
>  	enum xr_model model;
> +	unsigned int channel;
>  };
>  
>  static int xr_set_reg(struct usb_serial_port *port, u8 block, u8 reg, u8 val)
> @@ -153,6 +154,14 @@ static int xr_set_reg(struct usb_serial_port *port, u8 block, u8 reg, u8 val)
>  	struct usb_serial *serial = port->serial;
>  	int ret;
>  
> +	switch (port_priv->model) {
> +	case XR21V141X:
> +		if (port_priv->channel)
> +			reg |= (port_priv->channel - 1) << 8;

reg is u8 so you're simply discarding the channel index here and
effectively only the first port will work as intended after this patch.

> +		break;
> +	default:
> +		return -EINVAL;
> +	};
>  	ret = usb_control_msg(serial->dev,
>  			      usb_sndctrlpipe(serial->dev, 0),
>  			      xr_hal_table[port_priv->model][REQ_SET],
 
>  static int xr_probe(struct usb_serial *serial, const struct usb_device_id *id)
>  {
> +	struct usb_interface *intf = serial->interface;
> +	struct usb_endpoint_descriptor *data_ep;
>  	struct xr_port_private *port_priv;
> +	int ifnum;
>  
> -	/* Don't bind to control interface */
> -	if (serial->interface->cur_altsetting->desc.bInterfaceNumber == 0)
> +	/* Attach only data interfaces */
> +	ifnum = intf->cur_altsetting->desc.bInterfaceNumber;
> +	if (!(ifnum % 2))
>  		return -ENODEV;
>  
>  	port_priv = kzalloc(sizeof(*port_priv), GFP_KERNEL);
>  	if (!port_priv)
>  		return -ENOMEM;
>  
> +	data_ep = &intf->cur_altsetting->endpoint[0].desc;
> +
>  	port_priv->model = id->driver_info;
> +	port_priv->channel = data_ep->bEndpointAddress;

This creative but it seems cleaner to simply use the interface number
of the control interface divided by two. That gives you a zero-based
index which doesn't require the "channel--" added at various places by
the rest of the series.

Johan