Hi Andy,
On 25 June 2018 at 09:36, Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:
> 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?
I'll unify this.
>
>> +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...
Yes, I'll fix this.
>
>> + 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?
OK
>
>> +#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?
I'm not sure where to move this since it depends on chip type which is
detected at runtime. I don't think ftdi_sio_quirk is the right place.
So maybe I could just skip the defines for now and come back to:
switch (priv->chip_type) {
case FTX:
nvmconf.size = SZ_2K;
break;
...
> Also, above I think is already defined in a common way in linux/sizes.h.
Correct
>
>> +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?
I can move this into register_eeprom for consistency.
> Btw, can we rename it to be less generic, like adding a prefix?
OK, I'll add ftdi_ prefix.
>
>> + }
>
> --
> With Best Regards,
> Andy Shevchenko
Thanks,
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
