Add support for MaxLinear/Exar USB to serial converters
Subsystems (3)
cover Add support for MaxLinear/Exar USB to serial converters
From: Manivannan Sadhasivam <[email protected]>
Hello,
This series adds support for MaxLinear/Exar USB to serial converters. This driver only supports XR21V141X series but provision has been made to support other series in future.
This driver is inspired from the initial one submitted by Patong Yang:
https://patchwork.kernel.org/patch/10543261/
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.
This series has been tested on Hikey970 board hosting XR21V141X chip.
Thanks,
Mani
Manivannan Sadhasivam (2):
usb: serial: Add MaxLinear/Exar USB to Serial driver
usb: serial: xr_serial: Add gpiochip support
drivers/usb/serial/Kconfig | 9 + drivers/usb/serial/Makefile | 1 + drivers/usb/serial/xr_serial.c | 728 +++++++++++++++++++++++++++++++++ drivers/usb/serial/xr_serial.h | 110 +++++ 4 files changed, 848 insertions(+) create mode 100644 drivers/usb/serial/xr_serial.c create mode 100644 drivers/usb/serial/xr_serial.h
Signature · 1 line
2.17.1
PATCH 1/2 usb: serial: Add MaxLinear/Exar USB to Serial driver
From: Manivannan Sadhasivam <[email protected]>
Add support for MaxLinear/Exar USB to Serial converters. This driver only supports XR21V141X series but provision has been made to support other series in future.
This driver is inspired from the initial one submitted by Patong Yang:
https://patchwork.kernel.org/patch/10543261/
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.
--- drivers/usb/serial/Kconfig | 9 + drivers/usb/serial/Makefile | 1 + drivers/usb/serial/xr_serial.c | 544 +++++++++++++++++++++++++++++++++ drivers/usb/serial/xr_serial.h | 103 +++++++ 4 files changed, 657 insertions(+) create mode 100644 drivers/usb/serial/xr_serial.c create mode 100644 drivers/usb/serial/xr_serial.h
drivers/usb/serial/Kconfig +9 -0
@@ -644,6 +644,15 @@ config USB_SERIAL_UPD78F0730 To compile this driver as a module, choose M here: the module will be called upd78f0730. +config USB_SERIAL_XR + tristate "USB MaxLinear/Exar USB to Serial driver" + help + Say Y here if you want to use MaxLinear/Exar USB to Serial converter + devices. + + To compile this driver as a module, choose M here: the + module will be called xr_serial. + config USB_SERIAL_DEBUG tristate "USB Debugging Device" help
drivers/usb/serial/Makefile +1 -0
@@ -62,4 +62,5 @@ obj-$(CONFIG_USB_SERIAL_VISOR) += visor.o obj-$(CONFIG_USB_SERIAL_WISHBONE) += wishbone-serial.o obj-$(CONFIG_USB_SERIAL_WHITEHEAT) += whiteheat.o obj-$(CONFIG_USB_SERIAL_XIRCOM) += keyspan_pda.o +obj-$(CONFIG_USB_SERIAL_XR) += xr_serial.o obj-$(CONFIG_USB_SERIAL_XSENS_MT) += xsens_mt.o
drivers/usb/serial/xr_serial.c new file +544 -0
544 lines not on this page · raw message
drivers/usb/serial/xr_serial.h new file +103 -0
103 lines not on this page · raw message
Signature · 1 line
2.17.1
Greg KH
maintainerRe: [PATCH 1/2] usb: serial: Add MaxLinear/Exar USB to Serial driver
Quoted 8 lines · On Wed, Apr 29, 2020 at 01:26:50AM +0530, [email protected] wrote:
From: Manivannan Sadhasivam <[email protected]>
Add support for MaxLinear/Exar USB to Serial converters. This driver only supports XR21V141X series but provision has been made to support other series in future.
This driver is inspired from the initial one submitted by Patong Yang:
Quoted 7 lines
https://patchwork.kernel.org/patch/10543261/
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.
Nice work!
Some comments below:
Quoted 7 lines
drivers/usb/serial/xr_serial.c new file +7
-0+// SPDX-License-Identifier: GPL-2.0+ +/* +* MaxLinear/Exar USB to Serial driver +* +* Based on initial driver written by Patong Yang <[email protected]> +* +* Copyright (c) 2020 Manivannan Sadhasivam <[email protected]>
drivers/usb/serial/xr_serial.c line 17
Quoted 10 lines
drivers/usb/serial/xr_serial.c new file +10
-0+*/ + +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/slab.h> +#include <linux/tty.h> +#include <linux/usb.h> +#include <linux/usb/serial.h> + +#include "xr_serial.h"
No need for a .h file for a single .c file.
drivers/usb/serial/xr_serial.c line 68
Quoted 9 lines
drivers/usb/serial/xr_serial.c new file +9
-0+static int xr_get_reg (struct usb_serial_port *port,u8 block,u16 reg, +u16 *val) +{ +struct usb_serial *serial = port->serial; +struct xr_port_private *port_priv =usb_get_serial_port_data (port); +void *dmabuf; +int ret = -EINVAL ; + + dmabuf =kmalloc (sizeof (reg),GFP_KERNEL );
So that is 2 bytes?
Quoted 6 lines
drivers/usb/serial/xr_serial.c new file +6
-0+if (!dmabuf) +return -ENOMEM ; + +if (port_priv->idProduct ==XR21V141X_ID ) { +/* XR21V141X uses custom command for reading UART registers */ + ret = usb_control_msg(serial->dev,
drivers/usb/serial/xr_serial.c line 84
Quoted 10 lines
drivers/usb/serial/xr_serial.c new file +10
-0+usb_rcvctrlpipe (serial->dev,0 ), +XR_GET_XR21V141X , +USB_DIR_IN |USB_TYPE_VENDOR ,0 , + reg | (block <<8 ), dmabuf, + port_priv->reg_width, + USB_CTRL_SET_TIMEOUT); + } + +if (ret == port_priv->reg_width) { +memcpy (val, dmabuf, port_priv->reg_width);
But here you copy ->reg_width bytes in? How do you know val can hold that much? It's only set to be 1, so you copy 1 byte to a 16bit value? What part of the 16bits did you just copy those 8 bits to (hint, think cpu endian issues...)
That feels really really odd and a bit broken.
drivers/usb/serial/xr_serial.h line 1
Quoted 4 lines
drivers/usb/serial/xr_serial.h new file +1
-0@@ -0,0 +1,103 @@ +/* SPDX-License-Identifier: GPL-2.0+ */
Are you sure about the "+"? I have to ask :)
drivers/usb/serial/xr_serial.h line 4
Quoted 3 lines
drivers/usb/serial/xr_serial.h new file +3
-0+ +#ifndef __LINUX_USB_SERIAL_XR_SERIAL_H +#define __LINUX_USB_SERIAL_XR_SERIAL_H
As you will drop this file, just a general statement, no need for __LINUX as this is all Linux :)
thanks,
greg k-h
Re: [PATCH 1/2] usb: serial: Add MaxLinear/Exar USB to Serial driver
Hi Greg,
Quoted 32 lines · Greg KH
Quoted 8 lines · On Wed, Apr 29, 2020 at 01:26:50AM +0530, [email protected] wrote:
From: Manivannan Sadhasivam <[email protected]>
Add support for MaxLinear/Exar USB to Serial converters. This driver only supports XR21V141X series but provision has been made to support other series in future.
This driver is inspired from the initial one submitted by Patong Yang:
Quoted 7 lines
https://patchwork.kernel.org/patch/10543261/
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.
Nice work!
Some comments below:
Quoted 11 lines
drivers/usb/serial/xr_serial.c new file +11
-0+// SPDX-License-Identifier: GPL-2.0+ +/* +* MaxLinear/Exar USB to Serial driver +* +* Based on initial driver written by Patong Yang <[email protected]> +* +* Copyright (c) 2020 Manivannan Sadhasivam <[email protected]> +*/ + +#include <linux/kernel.h> +#include <linux/module.h>
Quoted 9 lines
drivers/usb/serial/xr_serial.c line 17
Quoted 6 lines
drivers/usb/serial/xr_serial.c new file +6
-0+#include <linux/slab.h> +#include <linux/tty.h> +#include <linux/usb.h> +#include <linux/usb/serial.h> + +#include "xr_serial.h" No need for a .h file for a single .c file.
Yeah but since this driver can support multiple series of XR chips (they might have separate register definitions and such), I thought it is a good idea to have a header file to keep the driver sane. But can club it to the source file for now.
Quoted 12 lines
drivers/usb/serial/xr_serial.c line 68
Quoted 9 lines
drivers/usb/serial/xr_serial.c new file +9
-0+static int xr_get_reg (struct usb_serial_port *port,u8 block,u16 reg, +u16 *val) +{ +struct usb_serial *serial = port->serial; +struct xr_port_private *port_priv =usb_get_serial_port_data (port); +void *dmabuf; +int ret = -EINVAL ; + + dmabuf =kmalloc (sizeof (reg),GFP_KERNEL );So that is 2 bytes?
Explanation below...
Quoted 22 lines
Quoted 6 lines
drivers/usb/serial/xr_serial.c new file +6
-0+if (!dmabuf) +return -ENOMEM ; + +if (port_priv->idProduct ==XR21V141X_ID ) { +/* XR21V141X uses custom command for reading UART registers */ + ret = usb_control_msg(serial->dev,drivers/usb/serial/xr_serial.c line 84
Quoted 10 lines
drivers/usb/serial/xr_serial.c new file +10
-0+usb_rcvctrlpipe (serial->dev,0 ), +XR_GET_XR21V141X , +USB_DIR_IN |USB_TYPE_VENDOR ,0 , + reg | (block <<8 ), dmabuf, + port_priv->reg_width, + USB_CTRL_SET_TIMEOUT); + } + +if (ret == port_priv->reg_width) { +memcpy (val, dmabuf, port_priv->reg_width);But here you copy ->reg_width bytes in? How do you know val can hold that much? It's only set to be 1, so you copy 1 byte to a 16bit value? What part of the 16bits did you just copy those 8 bits to (hint, think cpu endian issues...)
Quoted 2 lines
That feels really really odd and a bit broken.
Right. The reason is, the other series which can be supported by this driver have different register widths. For instance XR2280x. I haven't used them personally but seen this in initial driver. So I just used the max u16 type to make the reg_{set/get} routines work with those.
But agree, I should've used le16_to_cpu() cast to avoid endian issues.
If you think this hack is not required now, I can just use u8 and worry about compatibility later.
Quoted 7 lines
drivers/usb/serial/xr_serial.h line 1
Quoted 4 lines
drivers/usb/serial/xr_serial.h new file +1
-0@@ -0,0 +1,103 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ Are you sure about the "+"? I have to ask :)
I'm not a fan but since I've inherited the code from initial driver (which was GPL-2.0+), I kept it.
Quoted 7 lines
drivers/usb/serial/xr_serial.h line 4
Quoted 3 lines
drivers/usb/serial/xr_serial.h new file +3
-0+ +#ifndef __LINUX_USB_SERIAL_XR_SERIAL_H +#define __LINUX_USB_SERIAL_XR_SERIAL_HAs you will drop this file, just a general statement, no need for
__LINUXas this is all Linux :)
Sure.
Thanks,
Mani
Quoted 3 lines
thanks,
greg k-h
Greg KH
maintainerRe: [PATCH 1/2] usb: serial: Add MaxLinear/Exar USB to Serial driver
Quoted 45 lines · Manivannan Sadhasivam
Hi Greg,
Quoted 32 lines · Greg KH
Quoted 8 lines · On Wed, Apr 29, 2020 at 01:26:50AM +0530, [email protected] wrote:
From: Manivannan Sadhasivam <[email protected]>
Add support for MaxLinear/Exar USB to Serial converters. This driver only supports XR21V141X series but provision has been made to support other series in future.
This driver is inspired from the initial one submitted by Patong Yang:
Quoted 7 lines
https://patchwork.kernel.org/patch/10543261/
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.
Nice work!
Some comments below:
Quoted 11 lines
drivers/usb/serial/xr_serial.c new file +11
-0+// SPDX-License-Identifier: GPL-2.0+ +/* +* MaxLinear/Exar USB to Serial driver +* +* Based on initial driver written by Patong Yang <[email protected]> +* +* Copyright (c) 2020 Manivannan Sadhasivam <[email protected]> +*/ + +#include <linux/kernel.h> +#include <linux/module.h> Quoted 9 lines
drivers/usb/serial/xr_serial.c line 17
Quoted 6 lines
drivers/usb/serial/xr_serial.c new file +6
-0+#include <linux/slab.h> +#include <linux/tty.h> +#include <linux/usb.h> +#include <linux/usb/serial.h> + +#include "xr_serial.h" No need for a .h file for a single .c file.
Quoted 4 lines
Yeah but since this driver can support multiple series of XR chips (they might have separate register definitions and such), I thought it is a good idea to have a header file to keep the driver sane. But can club it to the source file for now.
Don't worry about future stuff, focus on what you need to do now :)
Quoted 37 lines
Quoted 12 lines
drivers/usb/serial/xr_serial.c line 68
Quoted 9 lines
drivers/usb/serial/xr_serial.c new file +9
-0+static int xr_get_reg (struct usb_serial_port *port,u8 block,u16 reg, +u16 *val) +{ +struct usb_serial *serial = port->serial; +struct xr_port_private *port_priv =usb_get_serial_port_data (port); +void *dmabuf; +int ret = -EINVAL ; + + dmabuf =kmalloc (sizeof (reg),GFP_KERNEL );So that is 2 bytes?
Explanation below...
Quoted 22 lines
Quoted 6 lines
drivers/usb/serial/xr_serial.c new file +6
-0+if (!dmabuf) +return -ENOMEM ; + +if (port_priv->idProduct ==XR21V141X_ID ) { +/* XR21V141X uses custom command for reading UART registers */ + ret = usb_control_msg(serial->dev,drivers/usb/serial/xr_serial.c line 84
Quoted 10 lines
drivers/usb/serial/xr_serial.c new file +10
-0+usb_rcvctrlpipe (serial->dev,0 ), +XR_GET_XR21V141X , +USB_DIR_IN |USB_TYPE_VENDOR ,0 , + reg | (block <<8 ), dmabuf, + port_priv->reg_width, + USB_CTRL_SET_TIMEOUT); + } + +if (ret == port_priv->reg_width) { +memcpy (val, dmabuf, port_priv->reg_width);But here you copy ->reg_width bytes in? How do you know val can hold that much? It's only set to be 1, so you copy 1 byte to a 16bit value? What part of the 16bits did you just copy those 8 bits to (hint, think cpu endian issues...)
Quoted 7 lines
Quoted 2 lines
That feels really really odd and a bit broken.
Right. The reason is, the other series which can be supported by this driver have different register widths. For instance XR2280x. I haven't used them personally but seen this in initial driver. So I just used the max u16 type to make the reg_{set/get} routines work with those.
Drop the whole "different register width" stuff for now, as the driver does not support it and it adds additional complexity that is hard to review for no good reason. If you want to add support for new devices later, _then_ we can add support for that.
Don't over-engineer :)
Quoted 1 line
But agree, I should've used
le16_to_cpu()cast to avoid endian issues.
You have to, the code is broken as-is right now.
Quoted 2 lines
If you think this hack is not required now, I can just use u8 and worry about compatibility later.
Please do so.
thanks,
greg k-h
Re: [PATCH 1/2] usb: serial: Add MaxLinear/Exar USB to Serial driver
Quoted 46 lines · Greg KH
Quoted 45 lines · Manivannan Sadhasivam
Hi Greg,
Quoted 41 lines · Greg KH
Quoted 8 lines · On Wed, Apr 29, 2020 at 01:26:50AM +0530, [email protected] wrote:
From: Manivannan Sadhasivam <[email protected]>
Add support for MaxLinear/Exar USB to Serial converters. This driver only supports XR21V141X series but provision has been made to support other series in future.
This driver is inspired from the initial one submitted by Patong Yang:
Quoted 7 lines
https://patchwork.kernel.org/patch/10543261/
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.
Nice work!
Some comments below:
Quoted 7 lines
drivers/usb/serial/xr_serial.c new file +7
-0+// SPDX-License-Identifier: GPL-2.0+ +/* +* MaxLinear/Exar USB to Serial driver +* +* Based on initial driver written by Patong Yang <[email protected]> +* +* Copyright (c) 2020 Manivannan Sadhasivam <[email protected]> drivers/usb/serial/xr_serial.c line 17
Quoted 10 lines
drivers/usb/serial/xr_serial.c new file +10
-0+*/ + +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/slab.h> +#include <linux/tty.h> +#include <linux/usb.h> +#include <linux/usb/serial.h> + +#include "xr_serial.h" No need for a .h file for a single .c file.
Quoted 7 lines
Quoted 4 lines
Yeah but since this driver can support multiple series of XR chips (they might have separate register definitions and such), I thought it is a good idea to have a header file to keep the driver sane. But can club it to the source file for now.
Don't worry about future stuff, focus on what you need to do now :)
Alright, will do :)
Quoted 45 lines
Quoted 37 lines
Quoted 12 lines
drivers/usb/serial/xr_serial.c line 68
Quoted 9 lines
drivers/usb/serial/xr_serial.c new file +9
-0+static int xr_get_reg (struct usb_serial_port *port,u8 block,u16 reg, +u16 *val) +{ +struct usb_serial *serial = port->serial; +struct xr_port_private *port_priv =usb_get_serial_port_data (port); +void *dmabuf; +int ret = -EINVAL ; + + dmabuf =kmalloc (sizeof (reg),GFP_KERNEL );So that is 2 bytes?
Explanation below...
Quoted 22 lines
Quoted 6 lines
drivers/usb/serial/xr_serial.c new file +6
-0+if (!dmabuf) +return -ENOMEM ; + +if (port_priv->idProduct ==XR21V141X_ID ) { +/* XR21V141X uses custom command for reading UART registers */ + ret = usb_control_msg(serial->dev,drivers/usb/serial/xr_serial.c line 84
Quoted 10 lines
drivers/usb/serial/xr_serial.c new file +10
-0+usb_rcvctrlpipe (serial->dev,0 ), +XR_GET_XR21V141X , +USB_DIR_IN |USB_TYPE_VENDOR ,0 , + reg | (block <<8 ), dmabuf, + port_priv->reg_width, + USB_CTRL_SET_TIMEOUT); + } + +if (ret == port_priv->reg_width) { +memcpy (val, dmabuf, port_priv->reg_width);But here you copy ->reg_width bytes in? How do you know val can hold that much? It's only set to be 1, so you copy 1 byte to a 16bit value? What part of the 16bits did you just copy those 8 bits to (hint, think cpu endian issues...)
Quoted 7 lines
Quoted 2 lines
That feels really really odd and a bit broken.
Right. The reason is, the other series which can be supported by this driver have different register widths. For instance XR2280x. I haven't used them personally but seen this in initial driver. So I just used the max u16 type to make the reg_{set/get} routines work with those.
Quoted 7 lines
Drop the whole "different register width" stuff for now, as the driver does not support it and it adds additional complexity that is hard to review for no good reason. If you want to add support for new devices later, _then_ we can add support for that.
Don't over-engineer :)
Sure!
Quoted 4 lines
Quoted 1 line
But agree, I should've used
le16_to_cpu()cast to avoid endian issues.You have to, the code is broken as-is right now.
We don't need the conversion if u8 is used everywhere :)
Thanks,
Mani
Quoted 8 lines
Quoted 2 lines
If you think this hack is not required now, I can just use u8 and worry about compatibility later.
Please do so.
thanks,
greg k-h
PATCH 2/2 usb: serial: xr_serial: Add gpiochip support
In reply to [email protected]From: Manivannan Sadhasivam <[email protected]>
Add gpiochip support for Maxlinear/Exar USB to serial converter for controlling the available gpios.
--- drivers/usb/serial/xr_serial.c | 186 ++++++++++++++++++++++++++++++++- drivers/usb/serial/xr_serial.h | 7 ++ 2 files changed, 192 insertions(+), 1 deletion(-)
drivers/usb/serial/xr_serial.c +185 -1
@@ -476,6 +476,189 @@ static void xr_break_ctl(struct tty_struct *tty, int break_state) state); } +#ifdef CONFIG_GPIOLIB + +static int xr_gpio_request (struct gpio_chip *gc,unsigned int offset) +{ +struct usb_serial_port *port =gpiochip_get_data (gc); +struct xr_port_private *port_priv =usb_get_serial_port_data (port); + +/* Check if the requested GPIO is occupied */ +if (port_priv->gpio_altfunc &BIT (offset)) +return -ENODEV ; + +return 0 ; +} + +static int xr_gpio_get (struct gpio_chip *gc,unsigned int gpio) +{ +struct usb_serial_port *port =gpiochip_get_data (gc); +struct xr_port_private *port_priv =usb_get_serial_port_data (port); +int ret; +u16 gpio_status; + + ret =xr_get_reg (port,XR21V141X_UART_REG_BLOCK , + port_priv->regs->gpio_status, &gpio_status); +if (ret) +return ret; + +return !!(gpio_status &BIT (gpio)); +} + +static void xr_gpio_set (struct gpio_chip *gc,unsigned int gpio,int val) +{ +struct usb_serial_port *port =gpiochip_get_data (gc); +struct xr_port_private *port_priv =usb_get_serial_port_data (port); + +if (val) +xr_set_reg (port,XR21V141X_UART_REG_BLOCK , + port_priv->regs->gpio_set,BIT (gpio)); +else +xr_set_reg (port,XR21V141X_UART_REG_BLOCK , + port_priv->regs->gpio_clr,BIT (gpio)); +} + +static int xr_gpio_direction_get (struct gpio_chip *gc,unsigned int gpio) +{ +struct usb_serial_port *port =gpiochip_get_data (gc); +struct xr_port_private *port_priv =usb_get_serial_port_data (port); +int ret; +u16 gpio_dir; + + ret =xr_get_reg (port,XR21V141X_UART_REG_BLOCK , + port_priv->regs->gpio_dir, &gpio_dir); +if (ret) +return ret; + +/* Logic 0 = input and Logic 1 = output */ +return !(gpio_dir &BIT (gpio)); +} + +static int xr_gpio_direction_input (struct gpio_chip *gc,unsigned int gpio) +{ +struct usb_serial_port *port =gpiochip_get_data (gc); +struct xr_port_private *port_priv =usb_get_serial_port_data (port); +int ret; +u16 gpio_dir; + + ret =xr_get_reg (port,XR21V141X_UART_REG_BLOCK , + port_priv->regs->gpio_dir, &gpio_dir); +if (ret) +return ret; + + gpio_dir &= ~BIT (gpio); + +return xr_set_reg (port,XR21V141X_UART_REG_BLOCK , + port_priv->regs->gpio_dir, gpio_dir); +} + +static int xr_gpio_direction_output (struct gpio_chip *gc,unsigned int gpio, +int val) +{ +struct usb_serial_port *port =gpiochip_get_data (gc); +struct xr_port_private *port_priv =usb_get_serial_port_data (port); +int ret; +u16 gpio_dir; + + ret =xr_get_reg (port,XR21V141X_UART_REG_BLOCK , + port_priv->regs->gpio_dir, &gpio_dir); +if (ret) +return ret; + + gpio_dir |=BIT (gpio); + + ret =xr_set_reg (port,XR21V141X_UART_REG_BLOCK , + port_priv->regs->gpio_dir, gpio_dir); +if (ret) +return ret; + +xr_gpio_set (gc, gpio, val); + +return 0 ; +} + +static int xr21v141x_gpio_init (struct usb_serial_port *port) +{ +struct xr_port_private *port_priv =usb_get_serial_port_data (port); +int ret; +u16 gpio_mode; + + port_priv->gc.ngpio =6 ; + + ret =xr_get_reg (port,XR21V141X_UART_REG_BLOCK , + port_priv->regs->gpio_mode, &gpio_mode); +if (ret) +return ret; + +/* Mark all pins which are not in GPIO mode */ +if (gpio_mode &UART_MODE_RTS_CTS ) + port_priv->gpio_altfunc |= (BIT (4 ) |BIT (5 )); +else if (gpio_mode &UART_MODE_DTR_DSR ) + port_priv->gpio_altfunc |= (BIT (2 ) |BIT (3 )); +else if (gpio_mode &UART_MODE_RS485 ) + port_priv->gpio_altfunc |=BIT (5 ); +else if (gpio_mode &UART_MODE_RS485_ADDR ) + port_priv->gpio_altfunc |=BIT (5 ); +else + port_priv->gpio_altfunc =0 ;/* All GPIOs are available */ + +return ret; +} + +static int xr_gpio_init (struct usb_serial_port *port) +{ +struct xr_port_private *port_priv =usb_get_serial_port_data (port); +int ret =0 ; + +if (port_priv->idProduct ==XR21V141X_ID ) + ret =xr21v141x_gpio_init (port); + +if (ret <0 ) +return ret; + + port_priv->gc.label ="xr_gpios" ; + port_priv->gc.request = xr_gpio_request; + port_priv->gc.get_direction = xr_gpio_direction_get; + port_priv->gc.direction_input = xr_gpio_direction_input; + port_priv->gc.direction_output = xr_gpio_direction_output; + port_priv->gc.get = xr_gpio_get; + port_priv->gc.set = xr_gpio_set; + port_priv->gc.owner =THIS_MODULE ; + port_priv->gc.parent = &port->dev; + port_priv->gc.base =-1 ; + port_priv->gc.can_sleep = true; + + ret =gpiochip_add_data (&port_priv->gc, port); +if (!ret) + port_priv->gpio_registered = true; + +return ret; +} + +static void xr_gpio_remove (struct usb_serial_port *port) +{ +struct xr_port_private *port_priv =usb_get_serial_port_data (port); + +if (port_priv->gpio_registered) { +gpiochip_remove (&port_priv->gc); + port_priv->gpio_registered = false; + } +} + +#else + +static int xr_gpio_init (struct usb_serial_port *port) +{ +return 0 ; +} + +static void xr_gpio_remove (struct usb_serial_port *port) +{ +/* Nothing to do */ +} + +#endif +static int xr_port_probe (struct usb_serial_port *port) {struct usb_serial *serial = port->serial;
@@ -495,13 +678,14 @@ static int xr_port_probe(struct usb_serial_port *port)usb_set_serial_port_data (port, port_priv); -return 0 ; +return xr_gpio_init (port); }static int xr_port_remove (struct usb_serial_port *port) {struct xr_port_private *port_priv =usb_get_serial_port_data (port); +xr_gpio_remove (port);kfree (port_priv);return 0 ;
drivers/usb/serial/xr_serial.h +7 -0
@@ -3,6 +3,8 @@ #ifndef __LINUX_USB_SERIAL_XR_SERIAL_H #define __LINUX_USB_SERIAL_XR_SERIAL_H +#include <linux/gpio/driver.h> + struct xr_uart_regs { u8 enable; u8 format;
@@ -21,6 +23,11 @@ struct xr_uart_regs { };struct xr_port_private { +#ifdef CONFIG_GPIOLIB +struct gpio_chip gc; +bool gpio_registered; +u8 gpio_altfunc; +#endif const struct xr_uart_regs *regs;u16 idProduct;u8 reg_width;
Signature · 1 line
2.17.1
Re: [PATCH 2/2] usb: serial: xr_serial: Add gpiochip support
Quoted 8 lines · On Tue, Apr 28, 2020 at 9:57 PM <[email protected]> wrote:
From: Manivannan Sadhasivam <[email protected]>
Add gpiochip support for Maxlinear/Exar USB to serial converter for controlling the available gpios.
Cc Linus Walleij <[email protected]>Signed-off-by Manivannan Sadhasivam <[email protected]>
That's a nice and clean GPIO driver.
I would change this:
port_priv->gc.label = "xr_gpios";
to something that is device-unique, like "xr-gpios-<serial number>" which makes it easy to locate the GPIOs on a specific serial converter for lab use. However the USB serial maintainers know better what to use here. Whatever makes a USB-to-serial unique from a TTY point of view is probably fine with me too.
My idea is that people might want to know which USB cable this is sitting on, so I have this USB cable and from this label I can always figure out which GPIO device it is.
Either way, it is not a super-big issue so:
Is this a off-the-shelf product that can be bought or is it mainly integrated on boards?
I'm asking because I'm looking for a neat USB-to-serial adapter with some GPIOs (2 is enough) that can be used for reset and power cycling of lab boards using one simple piece of equipment.
Yours,
Linus Walleij
Re: [PATCH 2/2] usb: serial: xr_serial: Add gpiochip support
Hi Linus,
Quoted 4 lines · Linus Walleij
Quoted 2 lines · On Tue, Apr 28, 2020 at 9:57 PM <[email protected]> wrote:
From: Manivannan Sadhasivam <[email protected]>
Quoted 8 lines
Quoted 6 lines
Add gpiochip support for Maxlinear/Exar USB to serial converter for controlling the available gpios.
Cc Linus Walleij <[email protected]>Signed-off-by Manivannan Sadhasivam <[email protected]>That's a nice and clean GPIO driver.
Thanks for the compliments :)
Quoted 11 lines
I would change this:
port_priv->gc.label = "xr_gpios";to something that is device-unique, like "xr-gpios-<serial number>" which makes it easy to locate the GPIOs on a specific serial converter for lab use. However the USB serial maintainers know better what to use here. Whatever makes a USB-to-serial unique from a TTY point of view is probably fine with me too.
Quoted 4 lines
My idea is that people might want to know which USB cable this is sitting on, so I have this USB cable and from this label I can always figure out which GPIO device it is.
Sounds reasonable. I can postfix the PID as below:
port_priv->gc.label = devm_kasprintf(port->dev, GFP_KERNEL, "XR%04x",
port_priv->idProduct);
So this will become, "XR1410".
Quoted 6 lines
Either way, it is not a super-big issue so:
Reviewed-by Linus Walleij <[email protected]>Is this a off-the-shelf product that can be bought or is it mainly integrated on boards?
Both I believe, though I have only used it integrated in dev boards. But a quick googling gives me below,
https://www.digikey.in/product-detail/en/maxlinear-inc/XR21V1410IL-0C-EB/1016-1425-ND/2636664
Thanks,
Mani
Quoted 6 lines
I'm asking because I'm looking for a neat USB-to-serial adapter with some GPIOs (2 is enough) that can be used for reset and power cycling of lab boards using one simple piece of equipment.
Yours,
Linus Walleij
Johan Hovold
maintainerRe: [PATCH 2/2] usb: serial: xr_serial: Add gpiochip support
Quoted 22 lines · Manivannan Sadhasivam
Quoted 21 lines · Linus Walleij
Quoted 8 lines · On Tue, Apr 28, 2020 at 9:57 PM <[email protected]> wrote:
From: Manivannan Sadhasivam <[email protected]>
Add gpiochip support for Maxlinear/Exar USB to serial converter for controlling the available gpios.
Cc Linus Walleij <[email protected]>Signed-off-by Manivannan Sadhasivam <[email protected]>I would change this:
port_priv->gc.label = "xr_gpios";to something that is device-unique, like "xr-gpios-<serial number>" which makes it easy to locate the GPIOs on a specific serial converter for lab use. However the USB serial maintainers know better what to use here. Whatever makes a USB-to-serial unique from a TTY point of view is probably fine with me too.
Quoted 3 lines
Quoted 3 lines
My idea is that people might want to know which USB cable this is sitting on, so I have this USB cable and from this label I can always figure out which GPIO device it is.
I think we've had this discussion before. First, not every device has a unique serial number. Second, we already have a universal way of distinguishing devices namely by using the bus topology. That's available through sysfs and shouldn't have to be be re-encoded by every driver in the gpiochip name.
Quoted 6 lines
Sounds reasonable. I can postfix the PID as below:
port_priv->gc.label= devm_kasprintf(port->dev,GFP_KERNEL, "XR%04x",port_priv->idProduct);So this will become, "XR1410".
So this doesn't really buy us anything; what if you have two of these devices?
Johan
Re: [PATCH 2/2] usb: serial: xr_serial: Add gpiochip support
Quoted 12 lines · Johan Hovold
Quoted 11 lines · Linus Walleij
Quoted 10 lines
to something that is device-unique, like "xr-gpios-<serial number>" which makes it easy to locate the GPIOs on a specific serial converter for lab use. However the USB serial maintainers know better what to use here. Whatever makes a USB-to-serial unique from a TTY point of view is probably fine with me too.
My idea is that people might want to know which USB cable this is sitting on, so I have this USB cable and from this label I can always figure out which GPIO device it is.
Quoted 5 lines
I think we've had this discussion before. First, not every device has a unique serial number. Second, we already have a universal way of distinguishing devices namely by using the bus topology. That's available through sysfs and shouldn't have to be be re-encoded by every driver in the gpiochip name.
I remember I even referred to this myself, but I've been waning a bit on it recently, because it turns out that userspace/users aren't very good at parsing sysfs for topology.
For userspace other than udev there seems to be a kind of agreement gap. Dunno how best to bridge it though. Education maybe.
Yours,
Linus Walleij
Greg KH
maintainerRe: [PATCH 2/2] usb: serial: xr_serial: Add gpiochip support
Quoted 19 lines · Linus Walleij
Quoted 12 lines · Johan Hovold
Quoted 11 lines · Linus Walleij
Quoted 10 lines
to something that is device-unique, like "xr-gpios-<serial number>" which makes it easy to locate the GPIOs on a specific serial converter for lab use. However the USB serial maintainers know better what to use here. Whatever makes a USB-to-serial unique from a TTY point of view is probably fine with me too.
My idea is that people might want to know which USB cable this is sitting on, so I have this USB cable and from this label I can always figure out which GPIO device it is.
Quoted 5 lines
I think we've had this discussion before. First, not every device has a unique serial number. Second, we already have a universal way of distinguishing devices namely by using the bus topology. That's available through sysfs and shouldn't have to be be re-encoded by every driver in the gpiochip name.
Quoted 3 lines
I remember I even referred to this myself, but I've been waning a bit on it recently, because it turns out that userspace/users aren't very good at parsing sysfs for topology.
Which is why they could use libudev :)
thanks,
greg k-h
Re: [PATCH 2/2] usb: serial: xr_serial: Add gpiochip support
Quoted 5 lines · Greg KH
Quoted 3 lines
I remember I even referred to this myself, but I've been waning a bit on it recently, because it turns out that userspace/users aren't very good at parsing sysfs for topology.
Which is why they could use libudev :)
Yet they insist on using things like Busybox' mdev (e.g. OpenWrt) or Android ... or is Android using libudev now? I'd be delighted if they did.
Yours,
Linus Walleij
Greg KH
maintainerRe: [PATCH 2/2] usb: serial: xr_serial: Add gpiochip support
Quoted 11 lines · Linus Walleij
Quoted 5 lines · Greg KH
Quoted 3 lines
I remember I even referred to this myself, but I've been waning a bit on it recently, because it turns out that userspace/users aren't very good at parsing sysfs for topology.
Which is why they could use libudev :)
Yet they insist on using things like Busybox' mdev (e.g. OpenWrt) or Android ... or is Android using libudev now? I'd be delighted if they did.
No, Android is not using libudev yet, they seem to be reinventing the same thing, slowly, over time :(
greg k-h
Re: [PATCH 2/2] usb: serial: xr_serial: Add gpiochip support
In reply to [email protected]Hi Greg,
Quoted 23 lines · On Wed, Apr 29, 2020 at 01:26:51AM +0530, [email protected] wrote:
From: Manivannan Sadhasivam <[email protected]>
Add gpiochip support for Maxlinear/Exar USB to serial converter for controlling the available gpios.
Cc Linus Walleij <[email protected]>Signed-off-by Manivannan Sadhasivam <[email protected]>--- drivers/usb/serial/xr_serial.c | 186 ++++++++++++++++++++++++++++++++- drivers/usb/serial/xr_serial.h | 7 ++ 2 files changed, 192 insertions(+), 1 deletion(-)drivers/usb/serial/xr_serial.c +2
-0@@ -476,6 +476,189 @@ static void xr_break_ctl(struct tty_struct *tty, int break_state) state); } +#ifdef CONFIG_GPIOLIB +
[...]
Quoted 59 lines
drivers/usb/serial/xr_serial.c +55
-0+ +static int xr_gpio_init (struct usb_serial_port *port) +{ +struct xr_port_private *port_priv =usb_get_serial_port_data (port); +int ret =0 ; + +if (port_priv->idProduct ==XR21V141X_ID ) + ret =xr21v141x_gpio_init (port); + +if (ret <0 ) +return ret; + + port_priv->gc.label ="xr_gpios" ; + port_priv->gc.request = xr_gpio_request; + port_priv->gc.get_direction = xr_gpio_direction_get; + port_priv->gc.direction_input = xr_gpio_direction_input; + port_priv->gc.direction_output = xr_gpio_direction_output; + port_priv->gc.get = xr_gpio_get; + port_priv->gc.set = xr_gpio_set; + port_priv->gc.owner =THIS_MODULE ; + port_priv->gc.parent = &port->dev; + port_priv->gc.base =-1 ; + port_priv->gc.can_sleep = true; + + ret =gpiochip_add_data (&port_priv->gc, port); +if (!ret) + port_priv->gpio_registered = true; + +return ret; +} + +static void xr_gpio_remove (struct usb_serial_port *port) +{ +struct xr_port_private *port_priv =usb_get_serial_port_data (port); + +if (port_priv->gpio_registered) { +gpiochip_remove (&port_priv->gc); + port_priv->gpio_registered = false; + } +} + +#else + +static int xr_gpio_init (struct usb_serial_port *port) +{ +return 0 ; +} + +static void xr_gpio_remove (struct usb_serial_port *port) +{ +/* Nothing to do */ +} + +#endif +static int xr_port_probe (struct usb_serial_port *port) {struct usb_serial *serial = port->serial;
drivers/usb/serial/xr_serial.c static int xr_port_probe(struct usb_serial_port *port) · line 681
Quoted 5 lines
drivers/usb/serial/xr_serial.c +1
-1@@ -495,13 +678,14 @@ static int xr_port_probe(struct usb_serial_port *port)usb_set_serial_port_data (port, port_priv); -return 0 ; +return xr_gpio_init (port);
Just realised that the gpiochip is registered for 2 interfaces exposed by this chip. This is due to the fact that this chip presents CDC-ACM model, so there are 2 interfaces (interrupt and bulk IN/OUT).
We shouldn't need gpiochip for interface 0. So what is the recommended way to filter that?
Thanks,
Mani
Quoted 38 lines
drivers/usb/serial/xr_serial.c +1
-0}static int xr_port_remove (struct usb_serial_port *port) {struct xr_port_private *port_priv =usb_get_serial_port_data (port); +xr_gpio_remove (port);kfree (port_priv);return 0 ;drivers/usb/serial/xr_serial.h +7
-0@@ -3,6 +3,8 @@ #ifndef __LINUX_USB_SERIAL_XR_SERIAL_H #define __LINUX_USB_SERIAL_XR_SERIAL_H +#include <linux/gpio/driver.h> + struct xr_uart_regs { u8 enable; u8 format;@@ -21,6 +23,11 @@ struct xr_uart_regs { };struct xr_port_private { +#ifdef CONFIG_GPIOLIB +struct gpio_chip gc; +bool gpio_registered; +u8 gpio_altfunc; +#endif const struct xr_uart_regs *regs;u16 idProduct;u8 reg_width;Signature · 1 line
2.17.1
Greg KH
maintainerRe: [PATCH 2/2] usb: serial: xr_serial: Add gpiochip support
Quoted 98 lines · Manivannan Sadhasivam
Hi Greg,
Quoted 23 lines · On Wed, Apr 29, 2020 at 01:26:51AM +0530, [email protected] wrote:
From: Manivannan Sadhasivam <[email protected]>
Add gpiochip support for Maxlinear/Exar USB to serial converter for controlling the available gpios.
Cc Linus Walleij <[email protected]>Signed-off-by Manivannan Sadhasivam <[email protected]>--- drivers/usb/serial/xr_serial.c | 186 ++++++++++++++++++++++++++++++++- drivers/usb/serial/xr_serial.h | 7 ++ 2 files changed, 192 insertions(+), 1 deletion(-)drivers/usb/serial/xr_serial.c +2
-0@@ -476,6 +476,189 @@ static void xr_break_ctl(struct tty_struct *tty, int break_state) state); } +#ifdef CONFIG_GPIOLIB +[...]
Quoted 59 lines
drivers/usb/serial/xr_serial.c +55
-0+ +static int xr_gpio_init (struct usb_serial_port *port) +{ +struct xr_port_private *port_priv =usb_get_serial_port_data (port); +int ret =0 ; + +if (port_priv->idProduct ==XR21V141X_ID ) + ret =xr21v141x_gpio_init (port); + +if (ret <0 ) +return ret; + + port_priv->gc.label ="xr_gpios" ; + port_priv->gc.request = xr_gpio_request; + port_priv->gc.get_direction = xr_gpio_direction_get; + port_priv->gc.direction_input = xr_gpio_direction_input; + port_priv->gc.direction_output = xr_gpio_direction_output; + port_priv->gc.get = xr_gpio_get; + port_priv->gc.set = xr_gpio_set; + port_priv->gc.owner =THIS_MODULE ; + port_priv->gc.parent = &port->dev; + port_priv->gc.base =-1 ; + port_priv->gc.can_sleep = true; + + ret =gpiochip_add_data (&port_priv->gc, port); +if (!ret) + port_priv->gpio_registered = true; + +return ret; +} + +static void xr_gpio_remove (struct usb_serial_port *port) +{ +struct xr_port_private *port_priv =usb_get_serial_port_data (port); + +if (port_priv->gpio_registered) { +gpiochip_remove (&port_priv->gc); + port_priv->gpio_registered = false; + } +} + +#else + +static int xr_gpio_init (struct usb_serial_port *port) +{ +return 0 ; +} + +static void xr_gpio_remove (struct usb_serial_port *port) +{ +/* Nothing to do */ +} + +#endif +static int xr_port_probe (struct usb_serial_port *port) {struct usb_serial *serial = port->serial;drivers/usb/serial/xr_serial.c static int xr_port_probe(struct usb_serial_port *port) · line 681
Quoted 5 lines
drivers/usb/serial/xr_serial.c +1
-1@@ -495,13 +678,14 @@ static int xr_port_probe(struct usb_serial_port *port)usb_set_serial_port_data (port, port_priv); -return 0 ; +return xr_gpio_init (port);Just realised that the gpiochip is registered for 2 interfaces exposed by this chip. This is due to the fact that this chip presents CDC-ACM model, so there are 2 interfaces (interrupt and bulk IN/OUT).
Quoted 2 lines
We shouldn't need gpiochip for interface 0. So what is the recommended way to filter that?
Not create the gpiochip for interface 0? :)
I really don't know what else to say here, sorry.
greg k-h
Johan Hovold
maintainerRe: [PATCH 2/2] usb: serial: xr_serial: Add gpiochip support
In reply to Manivannan SadhasivamQuoted 24 lines · Manivannan Sadhasivam
Quoted 13 lines · On Wed, Apr 29, 2020 at 01:26:51AM +0530, [email protected] wrote:
From: Manivannan Sadhasivam <[email protected]>
Add gpiochip support for Maxlinear/Exar USB to serial converter for controlling the available gpios.
Cc Linus Walleij <[email protected]>Signed-off-by Manivannan Sadhasivam <[email protected]>static int
xr_port_probe(struct usb_serial_port *port){drivers/usb/serial/xr_serial.c +0
-0struct usb_serial *serial = port->serial;drivers/usb/serial/xr_serial.c static int xr_port_probe(struct usb_serial_port *port) · line 681
Quoted 5 lines
drivers/usb/serial/xr_serial.c +1
-1@@ -495,13 +678,14 @@ static int xr_port_probe(struct usb_serial_port *port)usb_set_serial_port_data (port, port_priv); -return 0 ; +return xr_gpio_init (port);Just realised that the gpiochip is registered for 2 interfaces exposed by this chip. This is due to the fact that this chip presents CDC-ACM model, so there are 2 interfaces (interrupt and bulk IN/OUT).
Quoted 2 lines
We shouldn't need gpiochip for interface 0. So what is the recommended way to filter that?
Your driver should only bind to the data interface, but also claim the control interface (i.e. the reverse of what cdc-acm is doing).
This CDC model doesn't really fit the assumptions of usb-serial core, but it might be doable. Try returning 1 from the attach callback for the control interface so that core claims it but doesn't register a tty device.
Johan