On Sun, Jul 26, 2020 at 09:19:28PM +0530, Manivannan Sadhasivam wrote: > Hi, > > Sorry for the late reply! No worries at all. > On Wed, Jul 01, 2020 at 12:34:33PM +0200, Johan Hovold wrote: > > On Sun, Jun 07, 2020 at 09:53:48PM +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. > > > > > > This driver is inspired from the initial one submitted by Patong Yang: > > > > > > https://patchwork.kernel.org/patch/10543261/ > > > > You've also copied code from that driver so you need to maintain its > > copyright as well. > > > > Probably better you link to lore than patchwork. Do that in the file > > header as well. > > > > > While the initial driver was a custom tty USB driver exposing whole > > > new serial interface ttyXRUSBn, this version is completely based on USB > > > serial core thus exposing the interfaces as ttyUSBn. This will avoid > > > the overhead of exposing a new USB serial interface which the userspace > > > tools are unaware of. > > > > > > Reviewed-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> > > > Tested-by: Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx> > > > Signed-off-by: Manivannan Sadhasivam <mani@xxxxxxxxxx> > > > --- > > > drivers/usb/serial/Kconfig | 9 + > > > drivers/usb/serial/Makefile | 1 + > > > drivers/usb/serial/xr_serial.c | 650 +++++++++++++++++++++++++++++++++ > > > 3 files changed, 660 insertions(+) > > > create mode 100644 drivers/usb/serial/xr_serial.c > > > +#define XR21V141X_CLOCK_DIVISOR_0 0x4 > > > +#define XR21V141X_CLOCK_DIVISOR_1 0x5 > > > +#define XR21V141X_CLOCK_DIVISOR_2 0x6 > > > +#define XR21V141X_TX_CLOCK_MASK_0 0x7 > > > +#define XR21V141X_TX_CLOCK_MASK_1 0x8 > > > +#define XR21V141X_RX_CLOCK_MASK_0 0x9 > > > +#define XR21V141X_RX_CLOCK_MASK_1 0xa > > > > Please 0-pad these are they are registers. > > You mean adding 0 after 0x? Yes, exactly. > > > +static int xr_attach(struct usb_serial *serial) > > > +{ > > > + /* Do not register tty device for the control interface */ > > > + if (serial->interface->cur_altsetting->desc.bInterfaceNumber == 0) > > > + return 1; > > > > Ok, so you went for my first suggestion here instead of explicitly > > claiming the sibling interface. > > > > I still think you should bind to the data interface and then explicitly > > claim the control interface instead, since that better reflects that > > these interfaces are used together (and allows for unbinding through > > sysfs etc). > > How about something like below? > > static int xr_probe(struct usb_serial *serial, const struct usb_device_id *id) > { > struct usb_device *usb_dev = interface_to_usbdev(serial->interface); > struct usb_driver *driver = serial->type->usb_driver; > struct usb_interface *control_interface; > > /* Don't bind to control interface */ > if (serial->interface->cur_altsetting->desc.bInterfaceNumber == 0) > return -ENODEV; > > /* But claim the control interface during data interface probe */ > control_interface = usb_ifnum_to_if(usb_dev, 0); > if (usb_driver_claim_interface(driver, control_interface, NULL) != 0) > dev_err(serial->interface->dev, "Can't claim control interface"); > > return 0; > } Yes, something like that, but with error handling and a '\n' added to the error message. ;) Johan
