On Fri, Jun 22, 2018 at 5:22 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.
> 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?
> +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...
> + 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.
> #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?
> + }
--
With Best Regards,
Andy Shevchenko
--
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
