On Tue, Oct 11, 2016 at 12:04:48PM +0100, Martyn Welch wrote:
> This patch adds support for the GPIO found on the CP2105. Unlike the GPIO
> provided by some of the other devices supported by the cp210x driver, the
> GPIO on the CP2015 is muxed on pins otherwise used for serial control
> lines. The GPIO have been configured in 2 separate banks as the choice to
> configure the pins for GPIO is made separately for pins shared with each
> of the 2 serial ports this device provides, though the choice is made for
> all pins associated with that port in one go. The choice of whether to use
> the pins for GPIO or serial is made by adding configuration to a one-time
> programable PROM in the chip and can not be changed at runtime. The device
> defaults to GPIO.
>
> This device supports either push-pull or open-drain modes, it doesn't
> provide an explicit input mode, though the state of the GPIO can be read
> when used in open-drain mode. Like with pin use, the mode is configured in
> the one-time programable PROM and can't be changed at runtime.
>
> Signed-off-by: Martyn Welch <martyn.welch@xxxxxxxxxxxxxxx>
> ---
>
> V2: - Doesn't break build when gpiolib isn't selected.
>
> V3: - Tracking GPIO state so pins no longer get their state changed should
> the pin be in open-drain mode and be pulled down externally whilst
> another pin is set.
> - Reworked buffers and moved to byte accesses to remove the
> questionable buffer size logic and byte swapping.
> - Added error reporting.
> - Removed incorrect/pointless comments.
> - Renamed tmp variable to make use clearer.
>
> V4: - Fixed memory leak in cp210x_gpio_get error path.
>
> V5: - Determining shared GPIO based on device type.
> - Reordered vendor specific values by value.
> - Use interface device for gpio messages.
> - Remove unnecessary empty lines.
> - Using kzalloc rather than kcalloc.
> - Added locking to port_priv->output_state.
> - Added dummy cp2105_shared_gpio_init for !CONFIG_GPIOLIB.
> - Removed unnecessary masking on u8.
> - Added support for use of GPIO pin as RS485 traffic indication or
> activity LEDs.
> - Use correct dev for GPIO device.
> - Set can_sleep.
> - Roll in initial configuration state support.
> - Print error message & continue if GPIO fails.
> - Simplified ifdef'ing.
>
> V6: - Remove unused define.
> - Renamed cp210x_dev_private, cp210x_serial_private.
> - Moved GPIO variables to cp210x_serial_private.
> - Renamed PARTNUM defines.
> - Switched to using bitops for GPIO mode bits.
> - Moved vendor-specific defiles to end of defines.
> - Added helpers for vendor requests.
> - Using structs rather than byte arrays for sent and recieved values.
> - Exposing total number of GPIOs and refusing requests for pins not
> configured as GPIO, removing gpio_map in process.
> - Added checks for allocation failures.
> - Using same label for both banks of GPIO.
> - Moved GPIO into to attach function.
>
> V7: - Using GPIO private data for GPIO bits.
> - Adding limited .set_single_ended() and direction support.
> - Simplifying attach() and removing release() as it's no longer required.
>
> v8: - Re-fix for when gpiolib isn't selected.
> drivers/usb/serial/cp210x.c | 366 +++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 363 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
> index 4d6a5c6..f996142 100644
> --- a/drivers/usb/serial/cp210x.c
> +++ b/drivers/usb/serial/cp210x.c
> @@ -23,6 +23,9 @@
> #include <linux/usb.h>
> #include <linux/uaccess.h>
> #include <linux/usb/serial.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/bitops.h>
> +#include <linux/mutex.h>
>
> #define DRIVER_DESC "Silicon Labs CP210x RS232 serial adaptor driver"
>
> @@ -44,6 +47,7 @@ static int cp210x_tiocmset(struct tty_struct *, unsigned int, unsigned int);
> static int cp210x_tiocmset_port(struct usb_serial_port *port,
> unsigned int, unsigned int);
> static void cp210x_break_ctl(struct tty_struct *, int);
> +static int cp210x_attach(struct usb_serial *);
> static int cp210x_port_probe(struct usb_serial_port *);
> static int cp210x_port_remove(struct usb_serial_port *);
> static void cp210x_dtr_rts(struct usb_serial_port *p, int on);
> @@ -207,6 +211,15 @@ static const struct usb_device_id id_table[] = {
>
> MODULE_DEVICE_TABLE(usb, id_table);
>
> +#ifdef CONFIG_GPIOLIB
> +struct cp210x_gpio_private {
> + struct usb_serial *serial;
> + struct gpio_chip gc;
> + u8 config;
> + u8 gpio_mode;
> +};
> +#endif
> +
> struct cp210x_port_private {
> __u8 bInterfaceNumber;
> bool has_swapped_line_ctl;
> @@ -228,6 +241,7 @@ static struct usb_serial_driver cp210x_device = {
> .tx_empty = cp210x_tx_empty,
> .tiocmget = cp210x_tiocmget,
> .tiocmset = cp210x_tiocmset,
> + .attach = cp210x_attach,
> .port_probe = cp210x_port_probe,
> .port_remove = cp210x_port_remove,
> .dtr_rts = cp210x_dtr_rts
> @@ -270,6 +284,7 @@ static struct usb_serial_driver * const serial_drivers[] = {
> #define CP210X_SET_CHARS 0x19
> #define CP210X_GET_BAUDRATE 0x1D
> #define CP210X_SET_BAUDRATE 0x1E
> +#define CP210X_VENDOR_SPECIFIC 0xFF
>
> /* CP210X_IFC_ENABLE */
> #define UART_ENABLE 0x0001
> @@ -312,6 +327,21 @@ static struct usb_serial_driver * const serial_drivers[] = {
> #define CONTROL_WRITE_DTR 0x0100
> #define CONTROL_WRITE_RTS 0x0200
>
> +/* CP210X_VENDOR_SPECIFIC values */
> +#define CP210X_READ_LATCH 0x00C2
> +#define CP210X_GET_PARTNUM 0x370B
> +#define CP210X_GET_PORTCONFIG 0x370C
> +#define CP210X_GET_DEVICEMODE 0x3711
> +#define CP210X_WRITE_LATCH 0x37E1
> +
> +/* Part number definitions */
> +#define CP210X_PARTNUM_CP2101 0x01
> +#define CP210X_PARTNUM_CP2102 0x02
> +#define CP210X_PARTNUM_CP2103 0x03
> +#define CP210X_PARTNUM_CP2104 0x04
> +#define CP210X_PARTNUM_CP2105 0x05
> +#define CP210X_PARTNUM_CP2108 0x08
> +
> /* CP210X_GET_COMM_STATUS returns these 0x13 bytes */
> struct cp210x_comm_status {
> __le32 ulErrors;
> @@ -367,6 +397,61 @@ struct cp210x_flow_ctl {
> #define CP210X_SERIAL_RTS_ACTIVE 1
> #define CP210X_SERIAL_RTS_FLOW_CTL 2
>
> +/* CP210X_VENDOR_SPECIFIC, CP210X_GET_DEVICEMODE call reads these 0x2 bytes. */
> +struct cp210x_pin_mode {
> + u8 eci;
> + u8 sci;
> +} __packed;
> +
> +/*
> + * CP210X_VENDOR_SPECIFIC, CP210X_GET_DEVICEMODE call reads these 0xf bytes.
> + * Structure needs padding due to unused/unspecified bytes.
> + */
> +struct cp210x_config {
> + __le16 gpio_mode;
> + __le16 __pad0;
Nit: I'd use u8 arrays for the pad fields.
> + __le16 reset_state;
> + __le16 __pad1;
> + __le16 __pad2;
> + __le16 suspend_state;
> + u8 sci_cfg;
> + u8 eci_cfg;
> + u8 device_cfg;
> +} __packed;
> +
> +/* GPIO modes */
> +#define CP210X_SCI_GPIO_MODE_OFFSET 9
> +#define CP210X_SCI_GPIO_MODE_MASK GENMASK(11, 9)
> +
> +#define CP210X_ECI_GPIO_MODE_OFFSET 2
> +#define CP210X_ECI_GPIO_MODE_MASK GENMASK(3, 2)
> +
> +/* CP2105 port configuration values */
> +#define CP2105_SCI_GPIO0_TXLED_MODE BIT(0)
> +#define CP2105_SCI_GPIO1_RXLED_MODE BIT(1)
> +
> +#define CP2105_ECI_GPIO0_TXLED_MODE BIT(0)
> +#define CP2105_ECI_GPIO1_RXLED_MODE BIT(1)
> +#define CP2105_ECI_GPIO1_RS485_MODE BIT(2)
> +
> +/* CP210X_VENDOR_SPECIFIC, CP210X_WRITE_LATCH call writes these 0x2 bytes. */
> +struct cp210x_gpio_write {
> + u8 mask;
> + u8 state;
> +} __packed;
> +
> +/*
> + * Helper to get interface number when we only have struct usb_serial.
> + */
> +static u8 cp210x_interface_num(struct usb_serial *serial)
> +{
> + struct usb_host_interface *cur_altsetting;
> +
> + cur_altsetting = serial->interface->cur_altsetting;
> +
> + return cur_altsetting->desc.bInterfaceNumber;
> +}
> +
> /*
> * Reads a variable-sized block of CP210X_ registers, identified by req.
> * Returns data into buf in native USB byte order.
> @@ -463,6 +548,40 @@ static int cp210x_read_u8_reg(struct usb_serial_port *port, u8 req, u8 *val)
> }
>
> /*
> + * Reads a variable-sized vendor block of CP210X_ registers, identified by val.
> + * Returns data into buf in native USB byte order.
> + */
> +static int cp210x_read_vendor_block(struct usb_serial *serial, u8 type, u16 val,
> + void *buf, int bufsize)
> +{
> + void *dmabuf;
> + int result;
> +
> + dmabuf = kmalloc(bufsize, GFP_KERNEL);
> + if (!dmabuf)
> + return -ENOMEM;
> +
> + result = usb_control_msg(serial->dev, usb_rcvctrlpipe(serial->dev, 0),
> + CP210X_VENDOR_SPECIFIC, type, val,
> + cp210x_interface_num(serial), dmabuf, bufsize,
> + USB_CTRL_SET_TIMEOUT);
Use USB_CTRL_GET_TIMEOUT to avoid any confusion (same value).
> + if (result == bufsize) {
> + memcpy(buf, dmabuf, bufsize);
> + result = 0;
> + } else {
> + dev_err(&serial->interface->dev,
> + "failed get vendor val 0x%x size %d status: %d\n", val,
s/failed/failed to/
Use 0x%04x for val.
You can drop " status" as that's typically implied for the value after
the colon.
> + bufsize, result);
> + if (result >= 0)
> + result = -EPROTO;
We should be using -EIO here (even if the rest of the driver does not
yet do so) as -EPROTO is used to indicate lower-level hardware issues.
> + }
> +
> + kfree(dmabuf);
> +
> + return result;
> +}
> +
> +/*
> * Writes any 16-bit CP210X_ register (req) whose value is passed
> * entirely in the wValue field of the USB request.
> */
> @@ -531,6 +650,42 @@ static int cp210x_write_u32_reg(struct usb_serial_port *port, u8 req, u32 val)
> return cp210x_write_reg_block(port, req, &le32_val, sizeof(le32_val));
> }
>
> +#ifdef CONFIG_GPIOLIB
> +/*
> + * Writes a variable-sized vendor block of CP210X_ registers, identified by val.
> + * Data in buf must be in native USB byte order.
> + */
> +static int cp210x_write_vendor_block(struct usb_serial *serial, u8 type,
> + u16 val, void *buf, int bufsize)
> +{
> + void *dmabuf;
> + int result;
> +
> + dmabuf = kmemdup(buf, bufsize, GFP_KERNEL);
> + if (!dmabuf)
> + return -ENOMEM;
> +
> + result = usb_control_msg(serial->dev, usb_sndctrlpipe(serial->dev, 0),
> + CP210X_VENDOR_SPECIFIC, type, val,
> + cp210x_interface_num(serial), dmabuf, bufsize,
> + USB_CTRL_SET_TIMEOUT);
> +
> + kfree(dmabuf);
> +
> + if (result == bufsize) {
> + result = 0;
> + } else {
> + dev_err(&serial->interface->dev,
> + "failed set vendor val 0x%x size %d status: %d\n", val,
> + bufsize, result);
> + if (result >= 0)
> + result = -EPROTO;
Same comments as above.
> + }
> +
> + return result;
> +}
> +#endif
> +
> /*
> * Detect CP2108 GET_LINE_CTL bug and activate workaround.
> * Write a known good value 0x800, read it back.
> @@ -1104,10 +1259,195 @@ static void cp210x_break_ctl(struct tty_struct *tty, int break_state)
> cp210x_write_u16_reg(port, CP210X_SET_BREAK, state);
> }
>
> +#ifdef CONFIG_GPIOLIB
> +static int cp210x_gpio_request(struct gpio_chip *gc, unsigned int offset)
> +{
> + struct cp210x_gpio_private *priv = gpiochip_get_data(gc);
> + u8 intf_num = cp210x_interface_num(priv->serial);
> +
> + if (intf_num == 0) {
> + switch (offset) {
> + case 0:
> + if (priv->config & CP2105_ECI_GPIO0_TXLED_MODE)
> + return -ENODEV;
> + break;
> + case 1:
> + if (priv->config & (CP2105_ECI_GPIO1_RXLED_MODE |
> + CP2105_ECI_GPIO1_RS485_MODE))
> + return -ENODEV;
> + break;
> + }
> + } else if (intf_num == 1) {
> + switch (offset) {
> + case 0:
> + if (priv->config & CP2105_SCI_GPIO0_TXLED_MODE)
> + return -ENODEV;
> + case 1:
> + if (priv->config & CP2105_SCI_GPIO1_RXLED_MODE)
> + return -ENODEV;
> + break;
> + }
> + } else {
> + return -ENODEV;
> + }
Now that you shift the configuration bits during setup, would it make
sense to simplify this further and only use three defines:
#define CP2105_GPIO0_TXLED_MODE BIT(0)
#define CP2105_GPIO1_RXLED_MODE BIT(1)
#define CP2105_GPIO1_RS485_MODE BIT(2)
even if RS485 will never be set for interface 1?
> +
> + return 0;
> +}
> +/*
> + * This function is for configuring GPIO using shared pins, where other signals
> + * are made unavailable by configuring the use of GPIO. This is believed to be
> + * only applicable to the cp2105 at this point, the other devices supported by
> + * this driver that provide GPIO do so in a way that does not impact other
> + * signals and are thus expected to have very different initialisation.
> + */
> +static int cp2105_shared_gpio_init(struct usb_serial *serial)
> +{
> + struct cp210x_gpio_private *priv;
> + struct cp210x_pin_mode mode;
> + struct cp210x_config config;
> + u8 intf_num = cp210x_interface_num(serial);
> + int result;
> +
> + priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + result = cp210x_read_vendor_block(serial, REQTYPE_DEVICE_TO_HOST,
> + CP210X_GET_DEVICEMODE, &mode,
> + sizeof(mode));
> + if (result < 0)
> + return result;
As I mentioned in my "pre-review", you leak priv here and in the error
paths below.
> +
> + result = cp210x_read_vendor_block(serial, REQTYPE_DEVICE_TO_HOST,
> + CP210X_GET_PORTCONFIG, &config,
> + sizeof(config));
> + if (result < 0)
> + return result;
> +
> + /* 2 banks of GPIO - One for the pins taken from each serial port */
> + if (intf_num == 0) {
> + if (mode.eci == 0)
> + return 0;
Out of curiosity, what values can mode.eci/sci take on? Perhaps you can
add a comment somewhere, the rest you've done a great job at making
self-documenting.
> +
> + priv->config = config.eci_cfg;
> + priv->gpio_mode = (u8)((le16_to_cpu(config.gpio_mode) &
> + CP210X_ECI_GPIO_MODE_MASK) >>
> + CP210X_ECI_GPIO_MODE_OFFSET);
> + priv->gc.ngpio = 2;
> + } else if (intf_num == 1) {
> + if (mode.sci == 0)
> + return 0;
> +
> + priv->config = config.sci_cfg;
> + priv->gpio_mode = (u8)((le16_to_cpu(config.gpio_mode) &
> + CP210X_SCI_GPIO_MODE_MASK) >>
> + CP210X_SCI_GPIO_MODE_OFFSET);
> + priv->gc.ngpio = 3;
> + } else {
> + return -ENODEV;
> + }
> +
> + priv->gc.label = "cp210x";
> + priv->gc.request = cp210x_gpio_request;
> + priv->gc.get_direction = cp210x_gpio_direction_get;
> + priv->gc.direction_input = cp210x_gpio_direction_input;
> + priv->gc.direction_output = cp210x_gpio_direction_output;
> + priv->gc.get = cp210x_gpio_get;
> + priv->gc.set = cp210x_gpio_set;
> + priv->gc.set_single_ended = cp210x_gpio_set_single_ended;
> + priv->gc.owner = THIS_MODULE;
> + priv->gc.parent = &serial->interface->dev;
> + priv->gc.base = -1;
> + priv->gc.can_sleep = true;
> +
> + priv->serial = serial;
> +
> + result = devm_gpiochip_add_data(&serial->interface->dev, &priv->gc,
> + priv);
As also mentioned in my mail from last weekend, you should not use the
devres interface here for a number of reasons.
First of all, you are currently leaking the priv buffer allocated above.
Using devres also means the gpiochip stays registered for a short time
even after the USB disconnect callback returns. This could potentially
lead to I/O requests post disconnect which is not allowed, and you could
also end up dereferencing the usb-serial struct that might already have
been released.
Just don't use the devres interface, and make sure to deregister the
gpiochip explicitly in a new disconnect callback. Any memory allocated
should be freed in a release callback (matching attach).
It's probably better to use a struct cp210x_serial_private as you did in
an earlier version, and pass the struct usb_serial to gpiochip_add_data
instead.
> +
You can drop the empty line here.
> + if (result < 0)
> + priv->gc.label = NULL;
You currently never use this (as you did in prior version).
> +
> + return result;
> +}
Nice and clean overall. Great job!
Thanks,
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
