Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 6 Aug 2020 14:33:35 +0200
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
Message-ID: <20200806123335.GS3634@localhost>
References: <20200607162350.21297-1-mani@kernel.org>
 <20200607162350.21297-2-mani@kernel.org>
 <20200701103433.GC3334@localhost>
 <20200726154928.GA12036@Mani-XPS-13-9360>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200726154928.GA12036@Mani-XPS-13-9360>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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