From: Johan Hovold <johan@kernel.org>
To: Manivannan Sadhasivam <mani@kernel.org>
Cc: Johan Hovold <johan@kernel.org>,
gregkh@linuxfoundation.org, linux-usb@vger.kernel.org,
linux-kernel@vger.kernel.org, patong.mxl@gmail.com,
linus.walleij@linaro.org, mchehab+huawei@kernel.org
Subject: Re: [RESEND PATCH v4 1/3] usb: serial: Add MaxLinear/Exar USB to Serial driver
Date: Thu, 6 Aug 2020 14:33:35 +0200 [thread overview]
Message-ID: <20200806123335.GS3634@localhost> (raw)
In-Reply-To: <20200726154928.GA12036@Mani-XPS-13-9360>
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@linuxfoundation.org>
> > > Tested-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > > Signed-off-by: Manivannan Sadhasivam <mani@kernel.org>
> > > ---
> > > 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
next prev parent reply other threads:[~2020-08-06 16:48 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-06-07 16:23 [RESEND PATCH v4 0/3] Add support for MaxLinear/Exar USB to serial converters Manivannan Sadhasivam
2020-06-07 16:23 ` [RESEND PATCH v4 1/3] usb: serial: Add MaxLinear/Exar USB to Serial driver Manivannan Sadhasivam
2020-07-01 10:34 ` Johan Hovold
2020-07-26 15:49 ` Manivannan Sadhasivam
2020-08-06 12:33 ` Johan Hovold [this message]
2020-07-01 13:09 ` Johan Hovold
2020-06-07 16:23 ` [RESEND PATCH v4 2/3] usb: serial: xr_serial: Add gpiochip support Manivannan Sadhasivam
2020-07-01 13:02 ` Johan Hovold
2020-07-26 15:52 ` Manivannan Sadhasivam
2020-07-26 16:34 ` Andy Shevchenko
2020-07-27 4:46 ` Manivannan Sadhasivam
2020-07-27 7:50 ` Andy Shevchenko
2020-08-06 13:53 ` Johan Hovold
2020-06-07 16:23 ` [RESEND PATCH v4 3/3] usb: cdc-acm: Ignore Exar XR21V141X when serial driver is built Manivannan Sadhasivam
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200806123335.GS3634@localhost \
--to=johan@kernel.org \
--cc=gregkh@linuxfoundation.org \
--cc=linus.walleij@linaro.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
--cc=mani@kernel.org \
--cc=mchehab+huawei@kernel.org \
--cc=patong.mxl@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).