[U-Boot] [PATCH] i2c_eeprom: Add reading support

[Mario Six]

Hi Simon,

On Fri, Jun 10, 2016 at 2:35 AM, Simon Glass <sjg at chromium.org> wrote:
> Hi Mario,
>
> On 2 June 2016 at 07:07, Mario Six <mario.six at gdsys.cc> wrote:
>> This patch implements the reading functionality for the generic I2C
>> EEPROM driver, which was just a non-functional stub until now.
>>
>> Since the page size will be of importance for the writing support, we
>> add suitable members to the private data structure to keep track of it.
>>
>> Compatibility strings for a range of at24c* chips are added.
>>
>> Signed-off-by: Mario Six <mario.six at gdsys.cc>
>> ---
>>
>> Writing functionality doesn't quite work yet; but with these I2C EEPROMs
>> reading is probably more important, anyway.
>>
>
> Looks OK to me, with some comments below.
>
>> ---
>>  drivers/misc/Kconfig      |  5 +++++
>>  drivers/misc/i2c_eeprom.c | 57 ++++++++++++++++++++++++++++++++++++++++-------
>>  include/i2c_eeprom.h      |  4 ++++
>>  3 files changed, 58 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
>> index 2373037..66453d5 100644
>> --- a/drivers/misc/Kconfig
>> +++ b/drivers/misc/Kconfig
>> @@ -144,4 +144,9 @@ config QFW
>>           Hidden option to enable QEMU fw_cfg interface. This will be selected by
>>           either CONFIG_CMD_QFW or CONFIG_GENERATE_ACPI_TABLE.
>>
>> +config I2C_EEPROM
>> +       bool "Enable driver for generic I2C-attached EEPROMs"
>> +       depends on DM
>> +       help
>> +         Enable a generic driver for EEPROMs attached via I2C.
>>  endmenu
>> diff --git a/drivers/misc/i2c_eeprom.c b/drivers/misc/i2c_eeprom.c
>> index 814134a..d59f126 100644
>> --- a/drivers/misc/i2c_eeprom.c
>> +++ b/drivers/misc/i2c_eeprom.c
>> @@ -10,10 +10,20 @@
>>  #include <i2c.h>
>>  #include <i2c_eeprom.h>
>>
>> +DECLARE_GLOBAL_DATA_PTR;
>> +
>>  static int i2c_eeprom_read(struct udevice *dev, int offset, uint8_t *buf,
>>                            int size)
>>  {
>> -       return -ENODEV;
>> +       struct i2c_eeprom *data = dev_get_priv(dev);
>> +       struct udevice *chip;
>> +
>> +       if (i2c_get_chip(dev->parent, data->addr, data->olen, &chip)) {
>> +               printf("i2c_get_chip failed\n");
>> +               return -1;
>
> What is this call for? Won't the I2C device already be set up? You
> should be able to do:
>
> struct udevice *chip = dev_get_parent_platdata(dev);
>

Indeed. Reading it again, I don't even need to get the chip at all, I can just
use the device that is passed in for the dm_i2c_read call. Will fix in v2.

>> +       }
>> +
>> +       return dm_i2c_read(chip, offset, buf, size);
>>  }
>>
>>  static int i2c_eeprom_write(struct udevice *dev, int offset,
>> @@ -27,23 +37,54 @@ struct i2c_eeprom_ops i2c_eeprom_std_ops = {
>>         .write  = i2c_eeprom_write,
>>  };
>>
>> +static int i2c_eeprom_std_ofdata_to_platdata(struct udevice *dev)
>> +{
>> +       int addr, olen;
>> +       struct i2c_eeprom *priv = dev_get_priv(dev);
>> +       u64 data = dev_get_driver_data(dev);
>> +
>> +       addr = fdtdec_get_int(gd->fdt_blob, dev->of_offset, "reg", 0);
>> +       priv->addr = addr;
>> +
>> +       olen = fdtdec_get_int(gd->fdt_blob, dev->of_offset,
>> +                             "u-boot,i2c-offset-len", 1);
>> +       priv->olen = olen;
>> +
>> +       /* 6 bit -> page size of up to 2^63 (should be sufficient) */
>> +       priv->pagewidth = data & 0x3F;
>> +       priv->pagesize = (1 << priv->pagewidth);
>> +
>> +       return 0;
>> +}
>> +
>>  int i2c_eeprom_std_probe(struct udevice *dev)
>>  {
>>         return 0;
>>  }
>>
>>  static const struct udevice_id i2c_eeprom_std_ids[] = {
>> -       { .compatible = "i2c-eeprom" },
>> +       { .compatible = "i2c-eeprom", .data = 0 },
>> +       { .compatible = "atmel,24c01a", .data = 3 },
>> +       { .compatible = "atmel,24c02", .data = 3 },
>> +       { .compatible = "atmel,24c04", .data = 4 },
>> +       { .compatible = "atmel,24c08a", .data = 4 },
>> +       { .compatible = "atmel,24c16a", .data = 4 },
>> +       { .compatible = "atmel,24c32", .data = 5 },
>> +       { .compatible = "atmel,24c64", .data = 5 },
>> +       { .compatible = "atmel,24c128", .data = 6 },
>> +       { .compatible = "atmel,24c256", .data = 6 },
>> +       { .compatible = "atmel,24c512", .data = 6 },
>>         { }
>>  };
>>
>>  U_BOOT_DRIVER(i2c_eeprom_std) = {
>> -       .name           = "i2c_eeprom",
>> -       .id             = UCLASS_I2C_EEPROM,
>> -       .of_match       = i2c_eeprom_std_ids,
>> -       .probe          = i2c_eeprom_std_probe,
>> -       .priv_auto_alloc_size = sizeof(struct i2c_eeprom),
>> -       .ops            = &i2c_eeprom_std_ops,
>> +       .name                   = "i2c_eeprom",
>> +       .id                     = UCLASS_I2C_EEPROM,
>> +       .of_match               = i2c_eeprom_std_ids,
>> +       .probe                  = i2c_eeprom_std_probe,
>> +       .ofdata_to_platdata     = i2c_eeprom_std_ofdata_to_platdata,
>> +       .priv_auto_alloc_size   = sizeof(struct i2c_eeprom),
>> +       .ops                    = &i2c_eeprom_std_ops,
>>  };
>>
>>  UCLASS_DRIVER(i2c_eeprom) = {
>> diff --git a/include/i2c_eeprom.h b/include/i2c_eeprom.h
>> index ea6c962..52f4a6d 100644
>> --- a/include/i2c_eeprom.h
>> +++ b/include/i2c_eeprom.h
>> @@ -14,6 +14,10 @@ struct i2c_eeprom_ops {
>>  };
>>
>>  struct i2c_eeprom {
>> +       int addr;
>> +       int olen;
>> +       unsigned long pagesize;
>> +       unsigned pagewidth;
>
> Please can you comment this structure?
>

Sure, will add in v2.

>>  };
>>
>>  #endif
>> --
>> 2.7.0.GIT
>>
>
> Regards,
> Simon

Thanks for the review!

Best regards,
Mario