Re: [PATCH v5 1/3] usb: serial: Add MaxLinear/Exar USB to Serial driver
From: Mauro Carvalho Chehab
Date: Wed Feb 24 2021 - 03:53:37 EST
Em 2021年2月22日 16:47:36 +0100
Johan Hovold <johan@xxxxxxxxxx> escreveu:
>
On Mon, Feb 22, 2021 at 04:27:34PM +0100, Mauro Carvalho Chehab wrote:
>
> Hi Johan,
>
>
>
> Em 2021年1月26日 17:26:36 +0100
>
> Johan Hovold <johan@xxxxxxxxxx> escreveu:
>
>
>
> > On Tue, Jan 26, 2021 at 09:16:04PM +0530, Manivannan Sadhasivam wrote:
>
> > > On Thu, Jan 21, 2021 at 11:19:24AM +0100, Johan Hovold wrote:
>
> > > > On Sun, Nov 22, 2020 at 10:38:20PM +0530, Manivannan Sadhasivam wrote:
>
> > > > > 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
>
>
>
> 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.
Great! I'll work on a patch and submit when done.
Thanks!
Mauro