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?
>
> Now, there are some instances where creating a new child device might be the
> best approach. For example, you might have some OTP memory in an unrelated
> device. In that case, creating a child device (of UCLASS_MISC, or UCLASS_FUSE
> if that gets created) is the right course of action.
Regards,
SImon
