| Message ID | 1464872834-24164-1-git-send-email-mario.six@gdsys.cc |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
Hi Mario, On 2 June 2016 at 07:07, Mario Six <mario.six@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@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
Hi Simon, On Fri, Jun 10, 2016 at 2:35 AM, Simon Glass <sjg@chromium.org> wrote: > Hi Mario, > > On 2 June 2016 at 07:07, Mario Six <mario.six@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@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
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; + } + + 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; }; #endif
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@gdsys.cc> --- Writing functionality doesn't quite work yet; but with these I2C EEPROMs reading is probably more important, anyway. --- drivers/misc/Kconfig | 5 +++++ drivers/misc/i2c_eeprom.c | 57 ++++++++++++++++++++++++++++++++++++++++------- include/i2c_eeprom.h | 4 ++++ 3 files changed, 58 insertions(+), 8 deletions(-) -- 2.7.0.GIT