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. --Sean