On Fri, Jun 22, 2018 at 5:22 PM, Loic Poulain [off-list ref] 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.
FT230R chip integrates a 128-byte eeprom, FT230X a 2048-byte
eeprom...
This sounds unfinished. What is the continuation of the sentence?
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.
In some cases you are using EEPROM, in the rest, eeprom. What the difference?
quoted hunk
+config USB_SERIAL_FTDI_SIO_NVMEM
+ bool "FTDI MTP non-volatile memory support"
+ depends on USB_SERIAL_FTDI_SIO
+ select NVMEM
+ default y
First of all, I didn't find an evidence why it should be y...
quoted hunk
+ 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.
...second, the help semantics is inconsistent with default.
quoted hunk
#include <linux/usb.h>
#include <linux/serial.h>
#include <linux/usb/serial.h>
+#include <linux/nvmem-provider.h>
Perhaps squeeze it into most ordered part and keep there an order?
+#define EEPROM_SZ_FTX 2048 /* cf FTDI AN_201 */
+#define EEPROM_SZ_FT232RL 128 /* cf FTDI AN_121 */
These defines looks too particular, shouldn't be this a part of driver
data / platform data / device properties?
Also, above I think is already defined in a common way in linux/sizes.h.
+static int write_eeprom(void *priv, unsigned int off, void *_val, size_t bytes)
Leading _ in the name looks weird here, since it's not a macro.
(and inconsistent with below function definitions)
+ val = buf[0] + (buf[1] << 8);
get_unaligned_le16() ?
+ if (register_eeprom(port)) {
+ dev_err(&port->dev, "Unable to register FTDI EEPROM\n");
+ /* non-fatal */
Doesn't register_eeprom() have some error messaging already?
Btw, can we rename it to be less generic, like adding a prefix?
+ }