Hi Andy,
On 26 June 2018 at 13:02, Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:
> On Mon, Jun 25, 2018 at 3:35 PM, Loic Poulain <loic.poulain@xxxxxxxxxx> wrote:
>> Most of FTDI's devices have an EEPROM which records FTDI devices
>> configuration setting (e.g. the VID, PID, I/O config...) and user
>> data. For example, FT232R and FTX chips have 128-byte and 2048-byte
>> internal EEPROM respectively.
>>
>> This patch adds support for FTDI EEPROM read/write via USB control
>> transfers and register a new nvm device to the nvmem core.
>>
>> This permits to expose the EEPROM as a sysfs file, allowing userspace
>> to read/modify FTDI configuration and its user data without having to
>> rely on a specific userspace USB driver.
>>
>> Moreover, any upcoming new tentative to add CBUS GPIO support could
>> integrate CBUS EEPROM configuration reading in order to determine
>> which of the CBUS pins are available as GPIO.
>>
>
> FWIW,
> Reviewed-by: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
>
> One nit below, though.
>
>> Signed-off-by: Loic Poulain <loic.poulain@xxxxxxxxxx>
>> ---
>> v2: Use ifdef instead of IS_ENABLED
>> error message in case of nvmem registering failure
>> Fix space/tab in Kconfig
>> v3: Make nvmem a child of the usb dev instead of the serial port
>> Add macros defining eeprom sizes
>> Check read/write size is a nultiple of the eeprom word-size
>> Remove useless change in Kconfig
>> v4: Reword commit message
>> Remove default-yes from Kconfig
>> Change includes ordering
>> Use default linux size defines
>> Use get_unaligned_le16 helper
>> Prepend EEPROM functions with ftdi_
>> Error message in ftdi_eeprom_register()
>> v5: Fix missing linux/sizes header
>>
>> drivers/usb/serial/Kconfig | 10 ++++
>> drivers/usb/serial/ftdi_sio.c | 119 ++++++++++++++++++++++++++++++++++++++++++
>> drivers/usb/serial/ftdi_sio.h | 28 ++++++++++
>> 3 files changed, 157 insertions(+)
>>
>> diff --git a/drivers/usb/serial/Kconfig b/drivers/usb/serial/Kconfig
>> index 533f127..5747562 100644
>> --- a/drivers/usb/serial/Kconfig
>> +++ b/drivers/usb/serial/Kconfig
>> @@ -181,6 +181,16 @@ 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_NVMEM
>> + bool "FTDI MTP non-volatile memory support"
>> + depends on USB_SERIAL_FTDI_SIO
>> + select NVMEM
>> + help
>> + Say yes here to add support for the MTP non-volatile memory
>> + present on FTDI. Most of FTDI's devices have an EEPROM which
>> + records FTDI device's configuration setting as well as user
>> + data.
>> +
>> 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 7ea221d..75a35a8 100644
>> --- a/drivers/usb/serial/ftdi_sio.c
>> +++ b/drivers/usb/serial/ftdi_sio.c
>> @@ -37,9 +37,13 @@
>> #include <linux/spinlock.h>
>> #include <linux/mutex.h>
>> #include <linux/uaccess.h>
>> +#include <linux/sizes.h>
>> +#include <linux/nvmem-provider.h>
>> #include <linux/usb.h>
>> #include <linux/serial.h>
>> #include <linux/usb/serial.h>
>> +#include <asm/unaligned.h>
>> +
>
> I would put it rather as follows
>
> +#include <linux/nvmem-provider.h>
> ... // something else is still in order here?
> +#include <linux/sizes.h>
> #include <linux/spinlock.h>
> #include <linux/mutex.h>
> #include <linux/uaccess.h>
> #include <linux/usb.h>
> #include <linux/serial.h>
> #include <linux/usb/serial.h>
> +
> +#include <asm/unaligned.h>
> +
>
> // also note another blank line as divider.
>
Ok !
>> #include "ftdi_sio.h"
>> #include "ftdi_sio_ids.h"
>>
>> @@ -73,6 +77,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 nvmem_device *eeprom;
>> };
>>
>> /* struct ftdi_sio_quirk is used by devices requiring special attention. */
>> @@ -1529,6 +1535,112 @@ static int get_lsr_info(struct usb_serial_port *port,
>> return 0;
>> }
>>
>> +#ifdef CONFIG_USB_SERIAL_FTDI_SIO_NVMEM
>> +
>> +static int ftdi_write_eeprom(void *priv, unsigned int off, void *val,
>> + size_t bytes)
>> +{
>> + struct usb_serial_port *port = priv;
>> + struct usb_device *udev = port->serial->dev;
>
>> + unsigned char *buf = val;
>
> Btw, not sure why you need this now...
Not needed indeed, just wanted to avoid arithmetic on void pointer,
but I can cast instead.
>
>> +
>> + if (bytes % 2) /* 16-bit eeprom */
>> + return -EINVAL;
>> +
>> + while (bytes) {
>> + int rv;
>> +
>> + rv = usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
>> + FTDI_SIO_WRITE_EEPROM_REQUEST,
>> + FTDI_SIO_WRITE_EEPROM_REQUEST_TYPE,
>> + get_unaligned_le16(buf), off / 2, NULL,
>> + 0, WDR_TIMEOUT);
>> + if (rv < 0)
>> + return rv;
>> +
>> + off += 2;
>> + buf += 2;
>> + bytes -= 2;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int ftdi_read_eeprom(void *priv, unsigned int off, void *val,
>> + size_t bytes)
>> +{
>> + struct usb_serial_port *port = priv;
>> + struct usb_device *udev = port->serial->dev;
>
>> + unsigned char *buf = val;
>
> Ditto.
>
>> +
>> + 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, buf, 2, WDR_TIMEOUT);
>> + if (rv < 0)
>> + return rv;
>> +
>> + off += 2;
>> + buf += 2;
>> + bytes -= 2;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int ftdi_register_eeprom(struct usb_serial_port *port)
>> +{
>> + struct ftdi_private *priv = usb_get_serial_port_data(port);
>> + struct usb_device *udev = port->serial->dev;
>> + struct nvmem_config nvmconf = {};
>> +
>> + switch (priv->chip_type) {
>> + case FTX:
>> + nvmconf.size = SZ_2K;
>> + break;
>> + case FT232RL:
>> + nvmconf.size = SZ_128;
>> + break;
>> + default:
>> + return 0;
>> + }
>> +
>> + nvmconf.word_size = 2;
>> + nvmconf.stride = 2;
>> + nvmconf.read_only = false;
>> + nvmconf.priv = port;
>> + nvmconf.dev = &udev->dev;
>> + nvmconf.reg_read = ftdi_read_eeprom;
>> + nvmconf.reg_write = ftdi_write_eeprom;
>> + nvmconf.owner = THIS_MODULE;
>> +
>> + priv->eeprom = nvmem_register(&nvmconf);
>> + if (IS_ERR(priv->eeprom)) {
>> + dev_err(&udev->dev, "Unable to register FTDI EEPROM\n");
>> + priv->eeprom = NULL;
>> + return -ENOMEM;
>> + }
>> +
>> + dev_info(&udev->dev, "Registered %d-byte FTDI EEPROM\n", nvmconf.size);
>> +
>> + return 0;
>> +}
>> +
>> +static void ftdi_unregister_eeprom(struct usb_serial_port *port)
>> +{
>> + struct ftdi_private *priv = usb_get_serial_port_data(port);
>> +
>> + if (priv->eeprom)
>> + nvmem_unregister(priv->eeprom);
>> +}
>> +
>> +#endif /* CONFIG_USB_SERIAL_FTDI_SIO_NVMEM */
>>
>> /* Determine type of FTDI chip based on USB config and descriptor. */
>> static void ftdi_determine_type(struct usb_serial_port *port)
>> @@ -1814,6 +1926,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_NVMEM
>> + ftdi_register_eeprom(port);
>> +#endif
>> +
>> return 0;
>> }
>>
>> @@ -1931,6 +2047,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_NVMEM
>> + ftdi_unregister_eeprom(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..9eab961 100644
>> --- a/drivers/usb/serial/ftdi_sio.h
>> +++ b/drivers/usb/serial/ftdi_sio.h
>> @@ -36,6 +36,8 @@
>> #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_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 +402,32 @@ 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_GET_MODEM_STATUS */
>> /* Retrieve the current value of the modem status register */
>>
>> --
>> 2.7.4
>>
>
>
>
> --
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
