On Mon, Feb 22, 2021 at 04:27:34PM +0100, Mauro Carvalho Chehab wrote:

Hi Johan,

Em Tue, 26 Jan 2021 17:26:36 +0100
Johan Hovold [off-list ref] escreveu:

quoted

On Tue, Jan 26, 2021 at 09:16:04PM +0530, Manivannan Sadhasivam wrote:

quoted

On Thu, Jan 21, 2021 at 11:19:24AM +0100, Johan Hovold wrote:  

quoted

On Sun, Nov 22, 2020 at 10:38:20PM +0530, Manivannan Sadhasivam wrote:  

quoted

Add support for MaxLinear/Exar USB to Serial converters. This driver
only supports XR21V141X series but it can be extended to other series
from Exar as well in future.  

I'm now facing an issue with this driver. I have here two different
boards with those USB UART from MaxLinear/Exar.

The first one is identical to Mani's one:
	USB_DEVICE(0x04e2, 0x1411)
The second one is a different version of it:
	USB_DEVICE(0x04e2, 0x1424)

By looking at the final driver merged at linux-next, it sounds that
somewhere during the review of this series, it lost the priv struct,
and the xr_probe function. It also lost support for all MaxLinear/Exar
devices, except for just one model (04e2:1411).

The original submission:

	https://lore.kernel.org/linux-usb/20180404070634.nhspvmxcjwfgjkcv@advantechmxl-desktop (local)

And the manufacturer's Linux driver on their website:

	https://www.maxlinear.com/support/design-tools/software-drivers

Had support for other 12 different models of the MaxLinear/Exar USB
UART. 

IIRC Manivannan only had access to one of these models and his original
submission (based on the patch you link to above) didn't include support
for the others. And keeping the type abstraction didn't make sense for
just one model.

Those are grouped into 5 different major types:

	+	init_xr2280x_reg_map();
	+	init_xr21b142x_reg_map();
	+	init_xr21b1411_reg_map();
	+	init_xr21v141x_reg_map();
	+
	+	if ((xrusb->DeviceProduct & 0xfff0) == 0x1400)
	+		memcpy(&(xrusb->reg_map), &xr2280x_reg_map,
	+			sizeof(struct reg_addr_map));
	+	else if ((xrusb->DeviceProduct & 0xFFF0) == 0x1420)
	+		memcpy(&(xrusb->reg_map), &xr21b142x_reg_map,
	+			sizeof(struct reg_addr_map));
	+	else if (xrusb->DeviceProduct == 0x1411)
	+		memcpy(&(xrusb->reg_map), &xr21b1411_reg_map,
	+			sizeof(struct reg_addr_map));
	+	else if ((xrusb->DeviceProduct & 0xfff0) == 0x1410)
	+		memcpy(&(xrusb->reg_map), &xr21v141x_reg_map,
	+			sizeof(struct reg_addr_map));
	+	else
	+		rv = -1;

Note: Please don't be confused by "reg_map" name. This has nothing
      to do with Linux regmap API ;-)

What happens is that different USB IDs have different values for
each register. So, for instance, the UART enable register is set to
either one of the following values, depending on the value of
udev->descriptor.idProduct:

	xr21b140x_reg_map.uart_enable_addr = 0x00;
	xr21b1411_reg_map.uart_enable_addr = 0xc00;
	xr21v141x_reg_map.uart_enable_addr = 0x03;
	xr21b142x_reg_map.uart_enable_addr = 0x00;

There are other values that depend on the probing time detection,
based on other USB descriptors. Those set several fields at the
priv data that would allow to properly map the registers.

Also, there are 4 models that support multiple channels. On those,
there are one pair of register get/set for each channel.

-

In summary, while supporting just 04e2:1411 there's no need for
a private struct, in order to properly support the other models,
some autodetection is needed. The best way of doing that is to
re-add the .probe method and adding a priv struct.

As I dunno why this was dropped in the first place, I'm wondering
if it would be ok to re-introduce them.

Sure. It was just not needed if we were only going to support one model.

To be clear: my main focus here is just to avoid needing to use 
Windows in order to use the serial console of the hardware with
the 0x1424 variant ;-)

I can't test the driver with the other hardware, but, IMHO, instead
of adding a hack to support 0x1424, the better (but more painful)
would be to re-add the auto-detection part and support for the
other models.

Sounds good to me. 

Johan