[PATCH 06/14] misc: Add support for nvmem cells

[Sean Anderson]

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.anderson at 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.anderson at 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 at 700000 {
>> >>                 #address-cells = <1>;
>> >>                 #size-cells = <1>;
>> >>                 reg = <0x00700000 0x100000>;
>> >>
>> >>                 /* ... */
>> >>
>> >>                 tsens_calibration: calib at 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.anderson at 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