Re: [PATCH] Driver for MaxLinear/Exar USB (UART) Serial Adapters

[Greg KH]

On Thu, Aug 16, 2018 at 01:28:54AM -0700, Patong Yang wrote:
> On Thu, Aug 16, 2018 at 08:34:47AM +0200, Greg KH wrote:
> > On Wed, Aug 15, 2018 at 10:56:47PM -0700, Patong Yang wrote:
> > > Greg,
> > > 
> > > Please see my response inline below.
> > > 
> > > Patong
> > > 
> > > > But there is a bigger problem here:
> > > > 
> > > > > + xrusb_tty_driver = alloc_tty_driver(XRUSB_TTY_MINORS);
> > > > > + if (!xrusb_tty_driver)
> > > > > + return -ENOMEM;
> > > > 
> > > > Why are you not using the usb serial core here?  You need to do that,
> > > > not try to provide your own custom tty driver.  That way userspace
> > > > programs will "just work" with your new device, no changes needed as
> > > > your major/minor number and device name would be custom only for your
> > > > device, which is not acceptable.
> > > 
> > > The MaxLinear/USB serial devices support the CDC-ACM commands.
> > > Therefore, we used the cdc-acm driver instead of the usb serial driver
> > > as the starting point for developing the driver.  We replaced "ACM" 
> > > with "XRUSB" throughout the driver.  Would it be better if we just used 
> > > the same major/minor number as the CDC-ACM driver since it was based on
> > > the cdc-acm driver?  
> > 
> > No, just use the cdc-acm driver itself and add your product/device id to
> > it and it should work just fine.  Why do you need to write a whole new
> > driver at all?
> 
> The basic TX/RX functionality works fine now with the cdc-acm driver.
> That's the driver that is loaded because it's advertised as a cdc-acm
> compatible device in the device descriptors.
> 
> However, the cdc-acm driver (and spec) does not have support all of the 
> features in the MaxLinear/Exar USB UARTs.  Hence the reason for a separate
> and new driver.  

So your device should not be exposing itself as a cdc-acm driver if it
is doing vendor-specific things, right?

Ok, that's fine, but then you still need to tie into the usb-serial
core, just use that and do not implement all of the duplicated tty
handling logic that you have done here.  You will end up with a ttyUSB*
device node, which is what you, and your users want, not some
custom-name that no program supports.

thanks,

greg k-h

Re: [PATCH] Driver for MaxLinear/Exar USB (UART) Serial Adapters

[Oliver Neukum]

On Do, 2018-08-16 at 01:28 -0700, Patong Yang wrote:
> Features not supported by the cdc-acm driver are 
> enabling/disabling flow control, enabling/disabling RS-485 mode, 
> GPIOs and GPIO modes, etc. Support for these features had to be added in this
> new driver.

Hi,

I am afraid RS-485 in cdc-acm is a no-go.
I don't like the duplication, but it is the lesser evil.
We might want to include more stuff in the new driver,
so we duplicate only compiled code.

Regards
Oliver

Re: [PATCH] Driver for MaxLinear/Exar USB (UART) Serial Adapters

[Patong Yang]

On Thu, Aug 16, 2018 at 08:34:47AM +0200, Greg KH wrote:
> On Wed, Aug 15, 2018 at 10:56:47PM -0700, Patong Yang wrote:
> > Greg,
> > 
> > Please see my response inline below.
> > 
> > Patong
> > 
> > > But there is a bigger problem here:
> > > 
> > > > +   xrusb_tty_driver = alloc_tty_driver(XRUSB_TTY_MINORS);
> > > > +   if (!xrusb_tty_driver)
> > > > +   return -ENOMEM;
> > > 
> > > Why are you not using the usb serial core here?  You need to do that,
> > > not try to provide your own custom tty driver.  That way userspace
> > > programs will "just work" with your new device, no changes needed as
> > > your major/minor number and device name would be custom only for your
> > > device, which is not acceptable.
> > 
> > The MaxLinear/USB serial devices support the CDC-ACM commands.
> > Therefore, we used the cdc-acm driver instead of the usb serial driver
> > as the starting point for developing the driver.  We replaced "ACM" 
> > with "XRUSB" throughout the driver.  Would it be better if we just used 
> > the same major/minor number as the CDC-ACM driver since it was based on
> > the cdc-acm driver?  
> 
> No, just use the cdc-acm driver itself and add your product/device id to
> it and it should work just fine.  Why do you need to write a whole new
> driver at all?

The basic TX/RX functionality works fine now with the cdc-acm driver.
That's the driver that is loaded because it's advertised as a cdc-acm
compatible device in the device descriptors.

However, the cdc-acm driver (and spec) does not have support all of the 
features in the MaxLinear/Exar USB UARTs.  Hence the reason for a separate
and new driver.  

Features not supported by the cdc-acm driver are 
enabling/disabling flow control, enabling/disabling RS-485 mode, 
GPIOs and GPIO modes, etc. Support for these features had to be added in this
new driver.  

Please let me know if there are any futher questions and advise on how
to proceed.

Thanks,
Patong

Re: [PATCH] Driver for MaxLinear/Exar USB (UART) Serial Adapters

[Greg KH]

On Wed, Aug 15, 2018 at 10:56:47PM -0700, Patong Yang wrote:
> Greg,
> 
> Please see my response inline below.
> 
> Patong
> 
> > But there is a bigger problem here:
> > 
> > > + xrusb_tty_driver = alloc_tty_driver(XRUSB_TTY_MINORS);
> > > + if (!xrusb_tty_driver)
> > > + return -ENOMEM;
> > 
> > Why are you not using the usb serial core here?  You need to do that,
> > not try to provide your own custom tty driver.  That way userspace
> > programs will "just work" with your new device, no changes needed as
> > your major/minor number and device name would be custom only for your
> > device, which is not acceptable.
> 
> The MaxLinear/USB serial devices support the CDC-ACM commands.
> Therefore, we used the cdc-acm driver instead of the usb serial driver
> as the starting point for developing the driver.  We replaced "ACM" 
> with "XRUSB" throughout the driver.  Would it be better if we just used 
> the same major/minor number as the CDC-ACM driver since it was based on
> the cdc-acm driver?  

No, just use the cdc-acm driver itself and add your product/device id to
it and it should work just fine.  Why do you need to write a whole new
driver at all?

thanks,

greg k-h

Re: [PATCH] Driver for MaxLinear/Exar USB (UART) Serial Adapters

[Greg KH]

On Tue, Jul 24, 2018 at 03:36:36PM -0700, Patong Yang wrote:
> The original driver/patch was submitted on April 4, 2018.  This is the
> second version based on the feedback received on the original patch.
> 
> v2: Removed custom IOCTLs, as suggested by Greg KH
> Using standard Linux GPIO APIs, as suggested by Greg KH
> Removed file reads/writes as suggested by Greg KH
> 
> Signed-off-by: Patong Yang 
> ---
>  drivers/usb/serial/xrusb_serial.c | 2380 +
>  drivers/usb/serial/xrusb_serial.h |  234 +++

Why do you need a .h file for a single driver?  Please just put it all
into one file.

But there is a bigger problem here:

> + xrusb_tty_driver = alloc_tty_driver(XRUSB_TTY_MINORS);
> + if (!xrusb_tty_driver)
> + return -ENOMEM;

Why are you not using the usb serial core here?  You need to do that,
not try to provide your own custom tty driver.  That way userspace
programs will "just work" with your new device, no changes needed as
your major/minor number and device name would be custom only for your
device, which is not acceptable.

By doing that, your code will also be much smaller, always a good
benefit as well.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Driver for MaxLinear/Exar USB (UART) Serial Adapters

[Oliver Neukum]

On Di, 2018-07-24 at 15:36 -0700, Patong Yang wrote:
> +static int xrusb_ctrl_msg(struct xrusb *xrusb,
> +   int request, int value, void *buf, int len)
> +{
> +   int rv = usb_control_msg(xrusb->dev,
> +   usb_sndctrlpipe(xrusb->dev, 0),
> +   request,
> +   USB_RT_XRUSB,
> +   value,
> +   xrusb->control->altsetting[0].desc.bInterfaceNumber,
> +   buf,
> +   len,
> +   5000);

Please use the symbolic constant.

> +   return rv < 0 ? rv : 0;
> +}

Regards
Oliver

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Driver for MaxLinear/Exar USB (UART) Serial adapters.

[Greg KH]

On Thu, Apr 05, 2018 at 08:42:57PM -0700, Patong Yang wrote:
> On Wed, Apr 4, 2018 at 11:26 PM, Greg KH  wrote:
> > > This same customer is designing a new board to support the same
> transceiver
> > > configurations.  Instead of using the BIOS settings, they would like to
> use
> > > a user-space application to configure the additional GPIOs in a newer
> USB
> > > UART to set the transceiver modes, however, there are no standard APIs
> to
> > > support setting different modes, hence the custom IOCTLs.
> >
> > What is wrong with the current GPIO Linux api?  Doesn't that support
> > everything you need here?
> 
> Sorry, I am a kernel newbie and was not aware of the GPIO support.  I found
> and read through the documentation.  It's unclear to me how to map the
> GPIOs of the USB UART so that I can access them using the APIs.

That is up to you to figure out how that works for your device.

> Can someone point me to an example of how that can be done?  Would the
> support be added in the xrusb_serial driver or in a separate driver?

Probably in this driver, but look around at the current examples in the
kernel for other ideas.

> I also went through the documentation for the standard API for RS485
> support.  I can use the standard IOCTLs for enabling RS485 half-duplex
> control with the RTS pin.  However, is there a way to specify a
> different pin?  Some of the Exar USB UARTs can use a different pin for
> that function.

Why would they want to use a different pin?  Anyway, propose a standard
tty ioctl for that and we can take it from there.

> There is a custom IOCTL for enabling the "wide mode" feature in the Exar
> USB UARTs that inserts an additional byte in the RX FIFO that reports
> the parity, framing, parity and break error status for every data byte
> received.  This is the equivalent of reading LSR and RHR in 16550
> UARTs, and is useful for multidrop/9-bit mode applications.  I don't
> think any other USB UART has this function so there's no standard
> IOCTL for it.  Can I use a custom IOCTL for this or is there a better
> approach?

Isn't there already an option for that for other UARTs that do this?

> Also, as I mentioned in my original patch submission, the driver checks for
> a port_config file at startup and loads the appropriate settings.  The
> driver was creating the port_config file in the /etc/ directory when
> some of the custom IOCTLs were called to store the settings for the
> next time that the driver was loaded.  Without the custom IOCTLs, I
> was thinking that the user would just create those files in the /etc/
> directory if they wanted the driver to load/store the port
> configurations.  Do you see any issues with doing that?

No Linux kernel driver can ever read a file from a disk.  That's not how
Linux works at all, sorry.  Configuration comes from userspace programs
when they want to configure things, not when the kernel finds a device.

The kernel does notify userspace when devices are found, so you can run
your program at that point in time.

Hope this helps,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Driver for MaxLinear/Exar USB (UART) Serial adapters.

[Oliver Neukum]

Am Donnerstag, den 05.04.2018, 08:26 +0200 schrieb Greg KH:
> > The USB device can describe itself properly.  The SMBIOS function is a
> > requirement from our customer who designed a board using our device where
> > their CPU reads from a specific BIOS location and initializes GPIOs based
> > on the settings.  These GPIOs set the mode of the transceiver (LOOPBACK,
> > RS232, RS485, or RS422).  Therefore, the driver also reads the same
> > settings from the BIOS, so that it can enable the appropriate mode.
> 
> That logic can be done in userspace, no need to do that within the
> kernel, right?

No, not if you want to do full RS485 and similar stuff.

Yet ACM is not a full serial interface. It is for modems.
That raises two questions

a) why not add this to cdc-acm?
b) why custom ioctls? The kernel can do those serial protocols at upper
levels.

Regards
Oliver

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Driver for MaxLinear/Exar USB (UART) Serial adapters.

[Greg KH]

On Wed, Apr 04, 2018 at 12:03:51PM -0700, Patong Yang wrote:
> Thanks for the quick reply and feedback.  This needs to be a different
> driver because although the USB UARTs can work with the CDC-ACM driver,
> there are limitations in the CDC-ACM spec and therefore the driver that
> prevents it from fully functioning as a standard serial port (ie. status of
> CTS pin, ability to enable/disable flow control).  Therefore, the USB UARTs
> also support custom USB Vendor requests so that it can behave as a standard
> serial port and support other device features such as GPIO control and the
> automatic direction control feature typically used in RS-485 and RS-422
> designs.

The GPIO stuff needs to be a separate interface, using the kernel's GPIO
api, not custom ioctls.

> The USB device can describe itself properly.  The SMBIOS function is a
> requirement from our customer who designed a board using our device where
> their CPU reads from a specific BIOS location and initializes GPIOs based
> on the settings.  These GPIOs set the mode of the transceiver (LOOPBACK,
> RS232, RS485, or RS422).  Therefore, the driver also reads the same
> settings from the BIOS, so that it can enable the appropriate mode.

That logic can be done in userspace, no need to do that within the
kernel, right?

> This same customer is designing a new board to support the same transceiver
> configurations.  Instead of using the BIOS settings, they would like to use
> a user-space application to configure the additional GPIOs in a newer USB
> UART to set the transceiver modes, however, there are no standard APIs to
> support setting different modes, hence the custom IOCTLs.

What is wrong with the current GPIO Linux api?  Doesn't that support
everything you need here?

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Driver for MaxLinear/Exar USB (UART) Serial adapters.

[Greg KH]

On Wed, Apr 04, 2018 at 12:06:34AM -0700, Patong Yang wrote:
> - When specific IOCTLs are called by a user-space application, a 
>   port_config file is created for the /dev/ttyXRUSB device at a
>   specific USB tree location, and some configuration data is stored. 
>   The driver checks for the port_config file when the driver is loaded
>   for each port and loads the configuration settings if there is a
>   port_config file for the USB tree location.

Custom IOCTLS on a USB serial driver are not ok, sorry.  Please use the
standard ioctls and userspace apis for setting up and handling
configuration of your device.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Driver for MaxLinear/Exar USB (UART) Serial adapters.

[Greg KH]

On Wed, Apr 04, 2018 at 12:06:34AM -0700, Patong Yang wrote:
> The driver is based on the CDC-ACM driver. In addition to supporting 
> the features of the MaxLinear/Exar USB UART devices, the driver also 
> has support for 2 other functions per customer requirements:
> 
> - Specific entries are checked in the BIOS to detect if the board is a
>   "Caracalla" board before enabling specific modes in the MaxLinear/Exar
>   USB UARTs.  The smbios code is based on the example at:
>   https://sourceforge.net/projects/smbios/
> 
> - When specific IOCTLs are called by a user-space application, a 
>   port_config file is created for the /dev/ttyXRUSB device at a
>   specific USB tree location, and some configuration data is stored. 
>   The driver checks for the port_config file when the driver is loaded
>   for each port and loads the configuration settings if there is a
>   port_config file for the USB tree location.
> 
> Signed-off-by: Patong Yang 
> ---
>  drivers/usb/serial/xrusb_serial.c | 3285 
> +
>  drivers/usb/serial/xrusb_serial.h |  448 +
>  2 files changed, 3733 insertions(+)
>  create mode 100644 drivers/usb/serial/xrusb_serial.c
>  create mode 100644 drivers/usb/serial/xrusb_serial.h
> 
> diff --git a/drivers/usb/serial/xrusb_serial.c 
> b/drivers/usb/serial/xrusb_serial.c
> new file mode 100644
> index ..16a5bcff9103
> --- /dev/null
> +++ b/drivers/usb/serial/xrusb_serial.c
> @@ -0,0 +1,3285 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * xrusb_serial.c
> + *
> + * Copyright (c) 2018 Patong Yang 
> + *
> + * USB Serial Driver based on the cdc-acm.c driver for the
> + * MaxLinear/Exar USB UARTs/Serial adapters
> + */
> +
> +
> +#undef DEBUG
> +#undef VERBOSE_DEBUG

No need for these #undef in the driver.

And as Oliver points out, why does this have to be a totally different
driver?  And putting SMBIOS calls in a USB driver is very strange, can't
the USB devices describe themselves properly?

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Driver for MaxLinear/Exar USB (UART) Serial adapters.

[Oliver Neukum]

Am Mittwoch, den 04.04.2018, 00:06 -0700 schrieb Patong Yang:
> The driver is based on the CDC-ACM driver. In addition to supporting 
> the features of the MaxLinear/Exar USB UART devices, the driver also 

Hi,

it is rather drastic a measure to duplicate a driver just for a low
number of devices. It makes fixing bugs double the work. At a minimum,
before we look at the code itself, could you please explain why

a) the code cannot be added to cdc-acm?
b) the check for SMBIOS is needed?

Regards
Oliver

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html