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

[Simon Glass]

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);

> +       }
> +
> +       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?

>  };
>
>  #endif
> --
> 2.7.0.GIT
>

Regards,
Simon