Re: [PATCH v5 1/3] usb: serial: Add MaxLinear/Exar USB to Serial driver
From: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Date: 2021-02-24 08:53:41
Also in:
lkml
Em Mon, 22 Feb 2021 16:47:36 +0100 Johan Hovold [off-list ref] escreveu:
On Mon, Feb 22, 2021 at 04:27:34PM +0100, Mauro Carvalho Chehab wrote:quoted
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.quoted
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.quoted
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.quoted
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