linux-usb.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* USB: serial: ftdi_sio: Add support for CBUS GPIO
@ 2018-08-01 15:46 Loic Poulain
  0 siblings, 0 replies; 3+ messages in thread
From: Loic Poulain @ 2018-08-01 15:46 UTC (permalink / raw)
  To: johan; +Cc: linux-usb, andy.shevchenko, ajaykuee, daniel.thompson,
	Loic Poulain

Some FTDI devices like FTX or FT232R support CBUS Bit Bang mode on CBUS
pins, allowing host to control them via simple USB control transfers.
To make use of a CBUS pin in Bit Bang mode, the pin must be configured
to I/O mode in the FTDI EEPROM. This mode perfectly coexists with regular
USB to Serial function.

In this implementation, a GPIO controller is registered on FTDI probe
if at least one CBUS pin is configured for I/O mode. For now, only
FTX devices are supported.

This patch is based on previous Stefan Agner implementation tentative
on LKML ([PATCH 0/2] FTDI CBUS GPIO support).

Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
---
 drivers/usb/serial/Kconfig    |   9 ++
 drivers/usb/serial/ftdi_sio.c | 222 ++++++++++++++++++++++++++++++++++++++++++
 drivers/usb/serial/ftdi_sio.h |  83 ++++++++++++++++
 3 files changed, 314 insertions(+)

diff --git a/drivers/usb/serial/Kconfig b/drivers/usb/serial/Kconfig
index 533f127..64c9f2e 100644
--- a/drivers/usb/serial/Kconfig
+++ b/drivers/usb/serial/Kconfig
@@ -181,6 +181,15 @@ config USB_SERIAL_FTDI_SIO
 	  To compile this driver as a module, choose M here: the
 	  module will be called ftdi_sio.
 
+config USB_SERIAL_FTDI_SIO_CBUS_GPIO
+	bool "USB FDTI CBUS GPIO support"
+	depends on USB_SERIAL_FTDI_SIO
+	depends on GPIOLIB
+	help
+	  Say yes here to add support for the CBUS bit-bang mode, allowing CBUS
+	  pins to act as GPIOs. Note that pins must first be configured for GPIO
+	  in the device's EEPROM. The FT232R and FT-X series support this mode.
+
 config USB_SERIAL_VISOR
 	tristate "USB Handspring Visor / Palm m50x / Sony Clie Driver"
 	help
diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
index b5cef32..3cfb5fd 100644
--- a/drivers/usb/serial/ftdi_sio.c
+++ b/drivers/usb/serial/ftdi_sio.c
@@ -33,6 +33,7 @@
 #include <linux/tty.h>
 #include <linux/tty_driver.h>
 #include <linux/tty_flip.h>
+#include <linux/gpio/driver.h>
 #include <linux/module.h>
 #include <linux/spinlock.h>
 #include <linux/mutex.h>
@@ -40,12 +41,19 @@
 #include <linux/usb.h>
 #include <linux/serial.h>
 #include <linux/usb/serial.h>
+
 #include "ftdi_sio.h"
 #include "ftdi_sio_ids.h"
 
 #define DRIVER_AUTHOR "Greg Kroah-Hartman <greg@kroah.com>, Bill Ryder <bryder@sgi.com>, Kuba Ober <kuba@mareimbrium.org>, Andreas Mohr, Johan Hovold <jhovold@gmail.com>"
 #define DRIVER_DESC "USB FTDI Serial Converters Driver"
 
+struct ftdi_gpiochip {
+	struct gpio_chip gc;
+	unsigned int cbus_map[4];
+	unsigned long cbus_mask;
+	struct usb_serial_port *port;
+};
 
 struct ftdi_private {
 	enum ftdi_chip_type chip_type;
@@ -72,6 +80,8 @@ struct ftdi_private {
 	unsigned int latency;		/* latency setting in use */
 	unsigned short max_packet_size;
 	struct mutex cfg_lock; /* Avoid mess by parallel calls of config ioctl() and change_speed() */
+
+	struct ftdi_gpiochip *fgc;
 };
 
 /* struct ftdi_sio_quirk is used by devices requiring special attention. */
@@ -1528,6 +1538,211 @@ static int get_lsr_info(struct usb_serial_port *port,
 	return 0;
 }
 
+#ifdef CONFIG_USB_SERIAL_FTDI_SIO_CBUS_GPIO
+
+static int ftdi_read_eeprom(struct usb_device *udev, unsigned int off,
+			    void *val, size_t bytes)
+{
+	if (bytes % 2) /* 16-bit eeprom */
+		return -EINVAL;
+
+	while (bytes) {
+		int rv;
+
+		rv = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
+				     FTDI_SIO_READ_EEPROM_REQUEST,
+				     FTDI_SIO_READ_EEPROM_REQUEST_TYPE,
+				     0, off / 2, val, 2, WDR_TIMEOUT);
+		if (rv < 0)
+			return rv;
+
+		off += 2;
+		val += 2;
+		bytes -= 2;
+	}
+
+	return 0;
+}
+
+static int ftdi_set_bitmode(struct usb_serial_port *port, u8 bitmask,
+			    u8 bitmode)
+{
+	struct ftdi_private *priv = usb_get_serial_port_data(port);
+	__u16 urb_value = 0;
+
+	urb_value = bitmode << 8 | bitmask;
+
+	if (usb_control_msg(port->serial->dev,
+			     usb_sndctrlpipe(port->serial->dev, 0),
+			     FTDI_SIO_SET_BITMODE_REQUEST,
+			     FTDI_SIO_SET_BITMODE_REQUEST_TYPE,
+			     urb_value, priv->interface,
+			     NULL, 0, WDR_SHORT_TIMEOUT) < 0) {
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static int ftdi_read_pins(struct usb_serial_port *port, u8 *val)
+{
+	struct ftdi_private *priv = usb_get_serial_port_data(port);
+	unsigned char *buf;
+
+	buf = kmalloc(1, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	if (usb_control_msg(port->serial->dev,
+			     usb_rcvctrlpipe(port->serial->dev, 0),
+			     FTDI_SIO_READ_PINS_REQUEST,
+			     FTDI_SIO_READ_PINS_REQUEST_TYPE,
+			     0, priv->interface, buf, 1, WDR_TIMEOUT) < 0) {
+		kfree(buf);
+		return -EIO;
+	}
+
+	*val = buf[0];
+	kfree(buf);
+
+	return 0;
+}
+
+static int ftdi_cbus_gpio_dir_in(struct gpio_chip *gc, unsigned gpio)
+{
+	struct ftdi_gpiochip *fgc = gpiochip_get_data(gc);
+	unsigned int cbus_idx = fgc->cbus_map[gpio];
+
+	clear_bit(cbus_idx + 4, &fgc->cbus_mask);
+
+	return ftdi_set_bitmode(fgc->port, fgc->cbus_mask,
+				FTDI_SIO_BITMODE_CBUS);
+}
+
+static int ftdi_cbus_gpio_dir_out(struct gpio_chip *gc, unsigned gpio, int val)
+{
+	struct ftdi_gpiochip *fgc = gpiochip_get_data(gc);
+	unsigned int cbus_idx = fgc->cbus_map[gpio];
+
+	set_bit(cbus_idx + 4, &fgc->cbus_mask); /* direction */
+	set_bit(cbus_idx, &fgc->cbus_mask); /* value */
+
+	return ftdi_set_bitmode(fgc->port, fgc->cbus_mask,
+				FTDI_SIO_BITMODE_CBUS);
+}
+
+static int ftdi_cbus_gpio_get(struct gpio_chip *gc, unsigned gpio)
+{
+	struct ftdi_gpiochip *fgc = gpiochip_get_data(gc);
+	struct usb_device *udev = fgc->port->serial->dev;
+	unsigned int cbus_idx = fgc->cbus_map[gpio];
+	u8 val = 0;
+	int rv;
+
+	rv = ftdi_read_pins(fgc->port, &val);
+	if (rv)
+		dev_err(&udev->dev, "Unable to read CBUS GPIO pins, %d\n", rv);
+
+	return !!(val & BIT(cbus_idx));
+}
+
+static void ftdi_cbus_gpio_set(struct gpio_chip *gc, unsigned gpio, int val)
+{
+	struct ftdi_gpiochip *fgc = gpiochip_get_data(gc);
+	struct usb_device *udev = fgc->port->serial->dev;
+	int rv;
+
+	if (val)
+		set_bit(fgc->cbus_map[gpio], &fgc->cbus_mask);
+	else
+		clear_bit(fgc->cbus_map[gpio], &fgc->cbus_mask);
+
+	rv = ftdi_set_bitmode(fgc->port, fgc->cbus_mask, FTDI_SIO_BITMODE_CBUS);
+	if (rv)
+		dev_err(&udev->dev, "Unable set CBUS Bit-Bang mode, %d\n", rv);
+}
+
+static int ftdi_register_cbus_gpiochip(struct usb_serial_port *port)
+{
+	struct ftdi_private *priv = usb_get_serial_port_data(port);
+	struct usb_device *udev = port->serial->dev;
+	struct ftdi_gpiochip *fgc;
+	int rv;
+
+	fgc = kzalloc(sizeof(*fgc), GFP_KERNEL);
+	if (!fgc)
+		return -ENOMEM;
+
+	if (priv->chip_type == FTX) {
+		unsigned char *cbus_mux;
+		unsigned int i;
+
+		cbus_mux = kmalloc(4, GFP_KERNEL);
+		if (!cbus_mux)
+			return -ENOMEM;
+
+		/* CBUS pins are individually configurable via 8-bit MUX control
+		 * value, living at 0x1a for CBUS0. cf application note AN_201.
+		 */
+		rv = ftdi_read_eeprom(udev, 0x1a, cbus_mux, 4);
+		if (rv) {
+			dev_err(&udev->dev, "Unable to read CBUS config\n");
+			kfree(cbus_mux);
+			return -EIO;
+		}
+
+		for (i = 0; i < 4; i++) {
+			if (cbus_mux[i] == FTX_CBUS_MUX_IO)
+				fgc->cbus_map[fgc->gc.ngpio++] = i;
+		}
+
+		kfree(cbus_mux);
+	}
+
+	if (!fgc->gc.ngpio) {
+		kfree(fgc);
+		return 0;
+	}
+
+	fgc->gc.label = "ftdi-cbus-gpio";
+	fgc->gc.direction_input = ftdi_cbus_gpio_dir_in;
+	fgc->gc.direction_output = ftdi_cbus_gpio_dir_out;
+	fgc->gc.set = ftdi_cbus_gpio_set;
+	fgc->gc.get = ftdi_cbus_gpio_get;
+	fgc->gc.can_sleep = true;
+	fgc->gc.parent = &udev->dev;
+	fgc->gc.base = -1;
+	fgc->gc.owner = THIS_MODULE;
+	fgc->port = port;
+
+	rv = gpiochip_add_data(&fgc->gc, fgc);
+	if (rv) {
+		dev_err(&udev->dev, "Unable to add gpiochip\n");
+		kfree(fgc);
+		return rv;
+	}
+
+	priv->fgc = fgc;
+
+	dev_info(&udev->dev,
+		 "FTDI USB GPIO controller Registered, base=%d, ngpio=%u\n",
+		 fgc->gc.base, fgc->gc.ngpio);
+
+	return 0;
+}
+
+static void ftdi_unregister_cbus_gpiochip(struct usb_serial_port *port)
+{
+	struct ftdi_private *priv = usb_get_serial_port_data(port);
+	struct ftdi_gpiochip *fgc = priv->fgc;
+
+	if (fgc) {
+		gpiochip_remove(&fgc->gc);
+		kfree(fgc);
+	}
+}
+
+#endif /*  CONFIG_USB_SERIAL_FTDI_SIO_CBUS_GPIO */
 
 /* Determine type of FTDI chip based on USB config and descriptor. */
 static void ftdi_determine_type(struct usb_serial_port *port)
@@ -1813,6 +2028,10 @@ static int ftdi_sio_port_probe(struct usb_serial_port *port)
 		priv->latency = 16;
 	write_latency_timer(port);
 	create_sysfs_attrs(port);
+#ifdef CONFIG_USB_SERIAL_FTDI_SIO_CBUS_GPIO
+	ftdi_register_cbus_gpiochip(port);
+#endif
+
 	return 0;
 }
 
@@ -1930,6 +2149,9 @@ static int ftdi_sio_port_remove(struct usb_serial_port *port)
 {
 	struct ftdi_private *priv = usb_get_serial_port_data(port);
 
+#ifdef CONFIG_USB_SERIAL_FTDI_SIO_CBUS_GPIO
+	ftdi_unregister_cbus_gpiochip(port);
+#endif
 	remove_sysfs_attrs(port);
 
 	kfree(priv);
diff --git a/drivers/usb/serial/ftdi_sio.h b/drivers/usb/serial/ftdi_sio.h
index dcd0b6e..3bd248f 100644
--- a/drivers/usb/serial/ftdi_sio.h
+++ b/drivers/usb/serial/ftdi_sio.h
@@ -36,6 +36,10 @@
 #define FTDI_SIO_SET_ERROR_CHAR		7 /* Set the error character */
 #define FTDI_SIO_SET_LATENCY_TIMER	9 /* Set the latency timer */
 #define FTDI_SIO_GET_LATENCY_TIMER	10 /* Get the latency timer */
+#define FTDI_SIO_SET_BITMODE		11 /* Set the bitmode */
+#define FTDI_SIO_READ_PINS		12 /* Read pins in bitmode */
+#define FTDI_SIO_READ_EEPROM		0x90 /* Read eeprom */
+#define FTDI_SIO_WRITE_EEPROM		0x91 /* Write eeprom */
 
 /* Interface indices for FT2232, FT2232H and FT4232H devices */
 #define INTERFACE_A		1
@@ -400,6 +404,60 @@ enum ftdi_sio_baudrate {
  *
  */
 
+ /* FTDI_SIO_READ_EEPROM */
+#define FTDI_SIO_READ_EEPROM_REQUEST_TYPE 0xc0
+#define FTDI_SIO_READ_EEPROM_REQUEST FTDI_SIO_READ_EEPROM
+/*
+ *  BmRequestType:   1100 0000b
+ *  bRequest:        FTDI_SIO_READ_EEPROM
+ *  wValue:          0
+ *  wIndex:          Word Index
+ *  wLength:         2
+ *  Data:            return data (a word)
+ *
+ */
+
+/* FTDI_SIO_WRITE_EEPROM */
+#define FTDI_SIO_WRITE_EEPROM_REQUEST_TYPE 0x40
+#define FTDI_SIO_WRITE_EEPROM_REQUEST FTDI_SIO_WRITE_EEPROM
+/*
+ *  BmRequestType:   0100 0000b
+ *  bRequest:        FTDI_SIO_WRITE_EEPROM
+ *  wValue:          Data (word)
+ *  wIndex:          Word Index
+ *  wLength:         0
+ *  Data:            None
+ *
+ */
+
+ /* FTDI_SIO_SET_BITMODE */
+#define FTDI_SIO_SET_BITMODE_REQUEST_TYPE 0x40
+#define FTDI_SIO_SET_BITMODE_REQUEST FTDI_SIO_SET_BITMODE
+#define FTDI_SIO_BITMODE_RESET 0x00
+#define FTDI_SIO_BITMODE_CBUS 0x20
+/*
+ *  BmRequestType:   0100 0000b
+ *  bRequest:        FTDI_SIO_SET_BITMODE
+ *  wValue:          [0-7] bitmask, [8-15] bitmode
+ *  wIndex:          Port
+ *  wLength:         0
+ *  Data:            None
+ *
+ */
+
+/* FTDI_SIO_READ_PINS */
+#define FTDI_SIO_READ_PINS_REQUEST_TYPE 0xC0
+#define FTDI_SIO_READ_PINS_REQUEST FTDI_SIO_READ_PINS
+/*
+ *  BmRequestType:   1100 0000b
+ *  bRequest:        FTDI_SIO_READ_PINS
+ *  wValue:          0
+ *  wIndex:          Port
+ *  wLength:         2
+ *  Data:            return data (a word)
+ *
+ */
+
 /* FTDI_SIO_GET_MODEM_STATUS */
 /* Retrieve the current value of the modem status register */
 
@@ -563,3 +621,28 @@ enum ftdi_sio_baudrate {
  * B2..7	Length of message - (not including Byte 0)
  *
  */
+
+enum ftdi_ftx_cbus_mux {
+	FTX_CBUS_MUX_TRISTATE,
+	FTX_CBUS_MUX_RXLED,
+	FTX_CBUS_MUX_TXLED,
+	FTX_CBUS_MUX_TXRXLED,
+	FTX_CBUS_MUX_PWREN,
+	FTX_CBUS_MUX_SLEEP,
+	FTX_CBUS_MUX_DRIVE0,
+	FTX_CBUS_MUX_DRIVE1,
+	FTX_CBUS_MUX_IO,
+	FTX_CBUS_MUX_TXDEN,
+	FTX_CBUS_MUX_CLK24,
+	FTX_CBUS_MUX_CLK12,
+	FTX_CBUS_MUX_CLK6,
+	FTX_CBUS_MUX_BCD_CHARGER,
+	FTX_CBUS_MUX_BCD_CHARGER_N,
+	FTX_CBUS_MUX_I2C_TXE,
+	FTX_CBUS_MUX_I2C_RXF,
+	FTX_CBUS_MUX_VBUS_SENSE,
+	FTX_CBUS_MUX_BITBANG_WR,
+	FTX_CBUS_MUX_BITBANG_RD,
+	FTX_CBUS_MUX_TIMESTAMP,
+	FTX_CBUS_MUX_KEEP_AWAKE
+};

^ permalink raw reply related	[flat|nested] 3+ messages in thread

* USB: serial: ftdi_sio: Add support for CBUS GPIO
@ 2018-08-01 16:08 Andy Shevchenko
  0 siblings, 0 replies; 3+ messages in thread
From: Andy Shevchenko @ 2018-08-01 16:08 UTC (permalink / raw)
  To: Loic Poulain; +Cc: Johan Hovold, USB, Ajay Gupta, Daniel Thompson

On Wed, Aug 1, 2018 at 6:46 PM, Loic Poulain <loic.poulain@linaro.org> wrote:
> Some FTDI devices like FTX or FT232R support CBUS Bit Bang mode on CBUS
> pins, allowing host to control them via simple USB control transfers.
> To make use of a CBUS pin in Bit Bang mode, the pin must be configured
> to I/O mode in the FTDI EEPROM. This mode perfectly coexists with regular
> USB to Serial function.
>
> In this implementation, a GPIO controller is registered on FTDI probe
> if at least one CBUS pin is configured for I/O mode. For now, only
> FTX devices are supported.
>

> This patch is based on previous Stefan Agner implementation tentative
> on LKML ([PATCH 0/2] FTDI CBUS GPIO support).

The best approach to refer to a mail is to put a message-id, something like

(... [1].)

[1]: Message-Id: <...message-id...>

> +static int ftdi_read_eeprom(struct usb_device *udev, unsigned int off,
> +                           void *val, size_t bytes)
> +{

> +       if (bytes % 2) /* 16-bit eeprom */
> +               return -EINVAL;

Why is it fatal?
You may read one byte less (and provide an error code like -EIO, or
whatever fits better) or more (and provide exact amount asked).

> +       return 0;
> +}

> +       rv = ftdi_read_pins(fgc->port, &val);
> +       if (rv)
> +               dev_err(&udev->dev, "Unable to read CBUS GPIO pins, %d\n", rv);

Why not to return an error?

(Message itself sounds like a noise. Perhaps, dev_dbg() or throw away.

> +       rv = ftdi_set_bitmode(fgc->port, fgc->cbus_mask, FTDI_SIO_BITMODE_CBUS);
> +       if (rv)
> +               dev_err(&udev->dev, "Unable set CBUS Bit-Bang mode, %d\n", rv);

Ditto WRT message.

> +static int ftdi_register_cbus_gpiochip(struct usb_serial_port *port)
> +{
> +       struct ftdi_private *priv = usb_get_serial_port_data(port);
> +       struct usb_device *udev = port->serial->dev;
> +       struct ftdi_gpiochip *fgc;
> +       int rv;
> +

> +       fgc = kzalloc(sizeof(*fgc), GFP_KERNEL);
> +       if (!fgc)
> +               return -ENOMEM;

devm_ ?

> +               cbus_mux = kmalloc(4, GFP_KERNEL);
> +               if (!cbus_mux)
> +                       return -ENOMEM;

Is it mandatory to alloc so small amount on heap in this case?

> +               /* CBUS pins are individually configurable via 8-bit MUX control
> +                * value, living at 0x1a for CBUS0. cf application note AN_201.
> +                */

Is it a comment style used in this file?

> +               rv = ftdi_read_eeprom(udev, 0x1a, cbus_mux, 4);

0x1a is a magic.

> +       rv = gpiochip_add_data(&fgc->gc, fgc);
> +       if (rv) {
> +               dev_err(&udev->dev, "Unable to add gpiochip\n");
> +               kfree(fgc);
> +               return rv;
> +       }

devm_ ?

> +       return 0;
> +}
> +
> +static void ftdi_unregister_cbus_gpiochip(struct usb_serial_port *port)
> +{
> +       struct ftdi_private *priv = usb_get_serial_port_data(port);
> +       struct ftdi_gpiochip *fgc = priv->fgc;
> +

> +       if (fgc) {

How you can be here when fgc == NULL?!

> +               gpiochip_remove(&fgc->gc);
> +               kfree(fgc);
> +       }
> +}

I guess complete function will go away when you switch to devm.

^ permalink raw reply	[flat|nested] 3+ messages in thread

* USB: serial: ftdi_sio: Add support for CBUS GPIO
@ 2018-08-01 21:27 Loic Poulain
  0 siblings, 0 replies; 3+ messages in thread
From: Loic Poulain @ 2018-08-01 21:27 UTC (permalink / raw)
  To: Andy Shevchenko; +Cc: Johan Hovold, USB, Ajay Gupta, Daniel Thompson

Thanks Andy,

On 1 August 2018 at 18:08, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Wed, Aug 1, 2018 at 6:46 PM, Loic Poulain <loic.poulain@linaro.org> wrote:
>> Some FTDI devices like FTX or FT232R support CBUS Bit Bang mode on CBUS
>> pins, allowing host to control them via simple USB control transfers.
>> To make use of a CBUS pin in Bit Bang mode, the pin must be configured
>> to I/O mode in the FTDI EEPROM. This mode perfectly coexists with regular
>> USB to Serial function.
>>
>> In this implementation, a GPIO controller is registered on FTDI probe
>> if at least one CBUS pin is configured for I/O mode. For now, only
>> FTX devices are supported.
>>
>
>> This patch is based on previous Stefan Agner implementation tentative
>> on LKML ([PATCH 0/2] FTDI CBUS GPIO support).
>
> The best approach to refer to a mail is to put a message-id, something like
>
> (... [1].)
>
> [1]: Message-Id: <...message-id...>
>
>> +static int ftdi_read_eeprom(struct usb_device *udev, unsigned int off,
>> +                           void *val, size_t bytes)
>> +{
>
>> +       if (bytes % 2) /* 16-bit eeprom */
>> +               return -EINVAL;
>
> Why is it fatal?
> You may read one byte less (and provide an error code like -EIO, or
> whatever fits better) or more (and provide exact amount asked).

Yes, indeed.

>
>> +       return 0;
>> +}
>
>> +       rv = ftdi_read_pins(fgc->port, &val);
>> +       if (rv)
>> +               dev_err(&udev->dev, "Unable to read CBUS GPIO pins, %d\n", rv);
>
> Why not to return an error?
>
> (Message itself sounds like a noise. Perhaps, dev_dbg() or throw away.

Will do.

>
>> +       rv = ftdi_set_bitmode(fgc->port, fgc->cbus_mask, FTDI_SIO_BITMODE_CBUS);
>> +       if (rv)
>> +               dev_err(&udev->dev, "Unable set CBUS Bit-Bang mode, %d\n", rv);
>
> Ditto WRT message.

yes

>
>> +static int ftdi_register_cbus_gpiochip(struct usb_serial_port *port)
>> +{
>> +       struct ftdi_private *priv = usb_get_serial_port_data(port);
>> +       struct usb_device *udev = port->serial->dev;
>> +       struct ftdi_gpiochip *fgc;
>> +       int rv;
>> +
>
>> +       fgc = kzalloc(sizeof(*fgc), GFP_KERNEL);
>> +       if (!fgc)
>> +               return -ENOMEM;
>
> devm_ ?
>
>> +               cbus_mux = kmalloc(4, GFP_KERNEL);
>> +               if (!cbus_mux)
>> +                       return -ENOMEM;
>
> Is it mandatory to alloc so small amount on heap in this case?

Yes, because it is used as USB transfer buffer (DMA...) which can not
be on the stack.

>
>> +               /* CBUS pins are individually configurable via 8-bit MUX control
>> +                * value, living at 0x1a for CBUS0. cf application note AN_201.
>> +                */
>
> Is it a comment style used in this file?
>
>> +               rv = ftdi_read_eeprom(udev, 0x1a, cbus_mux, 4);
>
> 0x1a is a magic.
>
>> +       rv = gpiochip_add_data(&fgc->gc, fgc);
>> +       if (rv) {
>> +               dev_err(&udev->dev, "Unable to add gpiochip\n");
>> +               kfree(fgc);
>> +               return rv;
>> +       }
>
> devm_ ?
>
>> +       return 0;
>> +}
>> +
>> +static void ftdi_unregister_cbus_gpiochip(struct usb_serial_port *port)
>> +{
>> +       struct ftdi_private *priv = usb_get_serial_port_data(port);
>> +       struct ftdi_gpiochip *fgc = priv->fgc;
>> +
>
>> +       if (fgc) {
>
> How you can be here when fgc == NULL?!
>
>> +               gpiochip_remove(&fgc->gc);
>> +               kfree(fgc);
>> +       }
>> +}
>
> I guess complete function will go away when you switch to devm.
>
>
> --
> With Best Regards,
> Andy Shevchenko

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

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2018-08-01 21:27 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-08-01 15:46 USB: serial: ftdi_sio: Add support for CBUS GPIO Loic Poulain
  -- strict thread matches above, loose matches on Subject: below --
2018-08-01 16:08 Andy Shevchenko
2018-08-01 21:27 Loic Poulain

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).