Hi Sean, On Thu, 3 Mar 2022 at 10:45, Sean Anderson <sean.ander...@seco.com> wrote: > > Hi Simon, > > On 3/1/22 9:58 AM, Simon Glass wrote: > > Hi Sean, > > > > On Mon, 28 Feb 2022 at 09:43, Sean Anderson <sean.ander...@seco.com> wrote: > >> > >> > >> > >> On 2/26/22 1:36 PM, Simon Glass wrote: > >> > Hi Sean, > >> > > >> > On Mon, 7 Feb 2022 at 16:42, Sean Anderson <sean.ander...@seco.com> > >> > wrote: > >> >> > >> >> This adds support for "nvmem cells" as seen in Linux. The nvmem device > >> >> class in Linux is used for various assorted ROMs and EEPROMs. In this > >> >> sense, it is similar to UCLASS_MISC, but also includes > >> >> UCLASS_I2C_EEPROM, UCLASS_RTC, and UCLASS_MTD. While nvmem devices can > >> >> be accessed directly, they are most often used by reading/writing > >> >> contiguous values called "cells". Cells typically hold information like > >> >> calibration, versions, or configuration (such as mac addresses). > >> >> > >> >> nvmem devices can specify "cells" in their device tree: > >> >> > >> >> qfprom: eeprom@700000 { > >> >> #address-cells = <1>; > >> >> #size-cells = <1>; > >> >> reg = <0x00700000 0x100000>; > >> >> > >> >> /* ... */ > >> >> > >> >> tsens_calibration: calib@404 { > >> >> reg = <0x404 0x10>; > >> >> }; > >> >> }; > >> >> > >> >> which can then be referenced like: > >> >> > >> >> tsens { > >> >> /* ... */ > >> >> nvmem-cells = <&tsens_calibration>; > >> >> nvmem-cell-names = "calibration"; > >> >> }; > >> >> > >> >> The tsens driver could then read the calibration value like: > >> >> > >> >> struct nvmem_cell cal_cell; > >> >> u8 cal[16]; > >> >> nvmem_cell_get_by_name(dev, "calibration", &cal_cell); > >> >> nvmem_cell_read(&cal_cell, cal, sizeof(cal)); > >> >> > >> >> Because nvmem devices are not all of the same uclass, supported uclasses > >> >> must register a nvmem_interface struct. This allows CONFIG_NVMEM to be > >> >> enabled without depending on specific uclasses. At the moment, > >> >> nvmem_interface is very bare-bones, and assumes that no initialization > >> >> is necessary. However, this could be amended in the future. > >> >> > >> >> Although I2C_EEPROM and MISC are quite similar (and could likely be > >> >> unified), they present different read/write function signatures. To > >> >> abstract over this, NVMEM uses the same read/write signature as Linux. > >> >> In particular, short read/writes are not allowed, which is allowed by > >> >> MISC. > >> >> > >> >> The functionality implemented by nvmem cells is very similar to that > >> >> provided by i2c_eeprom_partition. "fixed-partition"s for eeproms does > >> >> not seem to have made its way into Linux or into any device tree other > >> >> than sandbox. It is possible that with the introduction of this API it > >> >> would be possible to remove it. > >> >> > >> >> Signed-off-by: Sean Anderson <sean.ander...@seco.com> > >> >> --- > >> >> > >> >> MAINTAINERS | 7 ++ > >> >> doc/api/index.rst | 1 + > >> >> doc/api/nvmem.rst | 7 ++ > >> >> drivers/misc/Kconfig | 16 ++++ > >> >> drivers/misc/Makefile | 1 + > >> >> drivers/misc/nvmem.c | 109 +++++++++++++++++++++++++ > >> >> include/nvmem.h | 185 ++++++++++++++++++++++++++++++++++++++++++ > >> >> 7 files changed, 326 insertions(+) > >> >> create mode 100644 doc/api/nvmem.rst > >> >> create mode 100644 drivers/misc/nvmem.c > >> >> create mode 100644 include/nvmem.h > >> > > >> > Here I think it would be better to add a new uclass so that drivers > >> > which support it can add a child device in that uclass. This is done > >> > in lots of places in U-Boot. > >> > >> I'm not sure exactly what you have in mind. The issue is that there are at > >> least 6 uclasses which I would like to support: > >> > >> - UCLASS_MISC > >> - UCLASS_I2C_EEPROM > >> - UCLASS_RTC > >> - UCLASS_MTD > >> - UCLASS_FUSE (doesn't exist yet, but probably should) > >> - Possibly UCLASS_PMIC > >> > >> Most of these uclasses have existing interfaces which expose an NVMEM-like > >> API, > >> in addition to other uclass-specific functionality. Instead of having an > >> additional API which drivers must implement, I would like to leverage these > >> existing APIs to make adding NVMEM support as painless as possible. NVMEM > >> is > >> more of a "meta-uclass" which allows us to leverage existing read/write > >> functions in uclasses. If any additional devices are to be created, they > >> need > >> to be created by the nvmem subsystem, or by the supported uclasses, rather > >> than > >> in drivers. > > > > I may be missing something as I have not looked in detail at your API > > changes. > > > > But the way to have a consistent API is to use a uclass. We do this > > with BLK. When a PMIC have GPIOs, RTC and regulators, we add them as > > child devices. We also have it with bootstd, where a bootdev is > > created as a child device of a storage device. We can put the required > > stuff in a helper function. We can even avoid any new code in the > > drivers by using the pending event system. > > > > Can you first help me understand what is wrong with using a new uclass? > > I suppose it could be done this way. > > Effectively, we are "picking" out two functions from the existing API. > NVMEM is a proper sub-uclass of every uclass added in this series except > UCLASS_MISC (which just needs some API adjustment). In essence, we could > actually implement something like nvmem_cell_read as > > int nvmem_cell_read(struct nvmem_cell *cell, void *buf, size_t size) > { > dev_dbg(cell->nvmem, "%s: off=%u size=%zu\n", __func__, cell->offset, > size); > if (size != cell->size) > return -EINVAL; > > switch (cell->nvmem->driver->id) { > case UCLASS_I2C_EEPROM: > return i2c_eeprom_read(dev, offset, buf, size) > case UCLASS_RTC: > return dm_rtc_read(cell->nvmem, offset, buf, size); > case UCLASS_MISC: { > int ret = misc_read(cell->nvmem, offset, buf, size); > > if (ret < 0) > return ret; > if (ret != size) > return -EIO; > return 0; > /* etc */ > } > } > > return -ENOSYS; > } > > Actually, that is probably cleaner than my current approach.
Yes but it is still not using a new uclass, right? Regards, Simon