[PATCH 0/8] regmap: Add managed API, regmap fields, regmap config

[PATCH 0/8] regmap: Add managed API, regmap fields, regmap config

[Pratyush Yadav]

Hi,

This series is a re-spin of Jean-Jacques' earlier effort [0], the goal
of which was to facilitate porting drivers from the Linux kernel. It
adds the managed API, using the same API as Linux. It also adds support
for regmap fields.

Jean-Jacques' series added support for custom regmap read/write
callbacks. The design was argued against by Simon [1]. He argued that
using the driver model was a better idea instead of the custom
functions. That would mean slightly more memory usage and a more
involved change.

The main aim of adding the custom functions is to support the Cadence
Sierra PHY driver from Linux [2]. The driver's custom functions aren't
very complicated, so they can be replaced by some simple formatting
options. So, add the struct regmap_config which contains fields to alter
the behaviour of the regmap. This includes specifying the size of the
read/write operations via 'width', specifying the start and size of
range from code instead of device tree via 'r_start' and 'r_size', and
specifying a left shift of the register offset before access via
'reg_offset_shift'. The driver can't be ported verbatim now, but this
allows the changes to be very isolated and minimal.

These config options allow us to avoid converting to driver model until
we really need it.

The patches are based on [3] which fixes a segmentation fault in sandbox
which didn't allow the tests to complete.

[0] https://patchwork.ozlabs.org/project/uboot/cover/20191105114700.24989-1-jjhiblot at ti.com/
[1] https://patchwork.ozlabs.org/comment/2426186/
[2] https://elixir.bootlin.com/linux/latest/source/drivers/phy/cadence/phy-cadence-sierra.c
[3] https://patchwork.ozlabs.org/project/uboot/patch/20200526120557.26240-1-p.yadav at ti.com/

Jean-Jacques Hiblot (3):
  regmap: Add devm_regmap_init()
  regmap: Add support for regmap fields
  test: dm: Add tests for regmap managed API and regmap fields

Pratyush Yadav (5):
  regmap: zero out the regmap on allocation
  regmap: Allow specifying read/write width
  regmap: Allow left shifting register offset before access
  regmap: Add regmap_init_mem_range()
  regmap: Allow devices to specify regmap range start and size in config

 arch/sandbox/dts/test.dts |  13 +++
 drivers/core/regmap.c     | 156 +++++++++++++++++++++++++++++-
 include/regmap.h          | 181 ++++++++++++++++++++++++++++++++--
 test/dm/regmap.c          | 198 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 537 insertions(+), 11 deletions(-)

--
2.26.2

[PATCH 1/8] regmap: Add devm_regmap_init()

[Pratyush Yadav]

From: Jean-Jacques Hiblot <jjhiblot@ti.com>

Most of new linux drivers are using managed-API to allocate resources. To
ease porting drivers from linux to U-Boot, introduce devm_regmap_init() as
a managed API to get a regmap from the device tree.

Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
---
 drivers/core/regmap.c | 29 +++++++++++++++++++++++++++++
 include/regmap.h      | 18 ++++++++++++++++++
 2 files changed, 47 insertions(+)

diff --git a/drivers/core/regmap.c b/drivers/core/regmap.c
index a67a237b88..74225361fd 100644
--- a/drivers/core/regmap.c
+++ b/drivers/core/regmap.c
@@ -14,7 +14,10 @@
 #include <regmap.h>
 #include <asm/io.h>
 #include <dm/of_addr.h>
+#include <dm/devres.h>
 #include <linux/ioport.h>
+#include <linux/compat.h>
+#include <linux/err.h>
 
 DECLARE_GLOBAL_DATA_PTR;
 
@@ -228,6 +231,32 @@ err:
 
 	return ret;
 }
+
+static void devm_regmap_release(struct udevice *dev, void *res)
+{
+	regmap_uninit(*(struct regmap **)res);
+}
+
+struct regmap *devm_regmap_init(struct udevice *dev,
+				const struct regmap_bus *bus,
+				void *bus_context,
+				const struct regmap_config *config)
+{
+	int rc;
+	struct regmap **mapp;
+
+	mapp = devres_alloc(devm_regmap_release, sizeof(struct regmap *),
+			    __GFP_ZERO);
+	if (unlikely(!mapp))
+		return ERR_PTR(-ENOMEM);
+
+	rc = regmap_init_mem(dev_ofnode(dev), mapp);
+	if (rc)
+		return ERR_PTR(rc);
+
+	devres_add(dev, mapp);
+	return *mapp;
+}
 #endif
 
 void *regmap_get_range(struct regmap *map, unsigned int range_num)
diff --git a/include/regmap.h b/include/regmap.h
index 30183c5e71..c7dd240a74 100644
--- a/include/regmap.h
+++ b/include/regmap.h
@@ -75,6 +75,9 @@ struct regmap_range {
 	ulong size;
 };
 
+struct regmap_bus;
+struct regmap_config;
+
 /**
  * struct regmap - a way of accessing hardware/bus registers
  *
@@ -335,6 +338,21 @@ int regmap_init_mem_platdata(struct udevice *dev, fdt_val_t *reg, int count,
 
 int regmap_init_mem_index(ofnode node, struct regmap **mapp, int index);
 
+/**
+ * devm_regmap_init() - Initialise register map (device managed)
+ *
+ * @dev: Device that will be interacted with
+ * @bus: Bus-specific callbacks to use with device (IGNORED)
+ * @bus_context: Data passed to bus-specific callbacks (IGNORED)
+ * @config: Configuration for register map (IGNORED)
+ *
+ * @Return a valid pointer to a struct regmap or a ERR_PTR() on error.
+ * The structure is automatically freed when the device is unbound
+ */
+struct regmap *devm_regmap_init(struct udevice *dev,
+				const struct regmap_bus *bus,
+				void *bus_context,
+				const struct regmap_config *config);
 /**
  * regmap_get_range() - Obtain the base memory address of a regmap range
  *
-- 
2.26.2

[PATCH 2/8] regmap: zero out the regmap on allocation

[Pratyush Yadav]

Some fields will be introduced in the regmap structure that should be
set to 0 by default. So, once we allocate a regmap, make sure it is
zeroed out to avoid unexpected defaults for those values.

Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
---
 drivers/core/regmap.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/core/regmap.c b/drivers/core/regmap.c
index 74225361fd..24651fb3ec 100644
--- a/drivers/core/regmap.c
+++ b/drivers/core/regmap.c
@@ -30,10 +30,12 @@ DECLARE_GLOBAL_DATA_PTR;
 static struct regmap *regmap_alloc(int count)
 {
 	struct regmap *map;
+	size_t size = sizeof(*map) + sizeof(map->ranges[0]) * count;
 
-	map = malloc(sizeof(*map) + sizeof(map->ranges[0]) * count);
+	map = malloc(size);
 	if (!map)
 		return NULL;
+	memset(map, 0, size);
 	map->range_count = count;
 
 	return map;
-- 
2.26.2

[PATCH 3/8] regmap: Allow specifying read/write width

[Pratyush Yadav]

Right now, regmap_read() and regmap_write() read/write a 32-bit value
only. To write other lengths, regmap_raw_read() and regmap_raw_write()
need to be used.

This means that any driver ported from Linux that relies on
regmap_{read,write}() to know the size already has to be updated at each
callsite. This makes the port harder to maintain.

So, allow specifying the read/write width to make it easier to port the
drivers, since now the only change needed is when initializing the
regmap.

Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
---
 drivers/core/regmap.c | 12 +++++++++---
 include/regmap.h      | 28 ++++++++++++++++++----------
 2 files changed, 27 insertions(+), 13 deletions(-)

diff --git a/drivers/core/regmap.c b/drivers/core/regmap.c
index 24651fb3ec..0180246095 100644
--- a/drivers/core/regmap.c
+++ b/drivers/core/regmap.c
@@ -245,7 +245,7 @@ struct regmap *devm_regmap_init(struct udevice *dev,
 				const struct regmap_config *config)
 {
 	int rc;
-	struct regmap **mapp;
+	struct regmap **mapp, *map;
 
 	mapp = devres_alloc(devm_regmap_release, sizeof(struct regmap *),
 			    __GFP_ZERO);
@@ -256,6 +256,10 @@ struct regmap *devm_regmap_init(struct udevice *dev,
 	if (rc)
 		return ERR_PTR(rc);
 
+	map = *mapp;
+	if (config)
+		map->width = config->width;
+
 	devres_add(dev, mapp);
 	return *mapp;
 }
@@ -378,7 +382,8 @@ int regmap_raw_read(struct regmap *map, uint offset, void *valp, size_t val_len)
 
 int regmap_read(struct regmap *map, uint offset, uint *valp)
 {
-	return regmap_raw_read(map, offset, valp, REGMAP_SIZE_32);
+	return regmap_raw_read(map, offset, valp,
+			       map->width ? map->width : REGMAP_SIZE_32);
 }
 
 static inline void __write_8(u8 *addr, const u8 *val,
@@ -488,7 +493,8 @@ int regmap_raw_write(struct regmap *map, uint offset, const void *val,
 
 int regmap_write(struct regmap *map, uint offset, uint val)
 {
-	return regmap_raw_write(map, offset, &val, REGMAP_SIZE_32);
+	return regmap_raw_write(map, offset, &val,
+				map->width ? map->width : REGMAP_SIZE_32);
 }
 
 int regmap_update_bits(struct regmap *map, uint offset, uint mask, uint val)
diff --git a/include/regmap.h b/include/regmap.h
index c7dd240a74..9362d864c0 100644
--- a/include/regmap.h
+++ b/include/regmap.h
@@ -76,16 +76,28 @@ struct regmap_range {
 };
 
 struct regmap_bus;
-struct regmap_config;
+
+/**
+ * struct regmap_config - Configure the behaviour of a regmap
+ *
+ * @width:		Width of the read/write operations. Defaults to
+ *			REGMAP_SIZE_32 if set to 0.
+ */
+struct regmap_config {
+	enum regmap_size_t width;
+};
 
 /**
  * struct regmap - a way of accessing hardware/bus registers
  *
+ * @width:		Width of the read/write operations. Defaults to
+ *			REGMAP_SIZE_32 if set to 0.
  * @range_count:	Number of ranges available within the map
  * @ranges:		Array of ranges
  */
 struct regmap {
 	enum regmap_endianness_t endianness;
+	enum regmap_size_t width;
 	int range_count;
 	struct regmap_range ranges[0];
 };
@@ -96,31 +108,27 @@ struct regmap {
  */
 
 /**
- * regmap_write() - Write a 32-bit value to a regmap
+ * regmap_write() - Write a value to a regmap
  *
  * @map:	Regmap to write to
  * @offset:	Offset in the regmap to write to
  * @val:	Data to write to the regmap at the specified offset
  *
- * Note that this function will only write values of 32 bit width to the
- * regmap; if the size of data to be read is different, the regmap_raw_write
- * function can be used.
+ * If the map's width is not set, this function will write a 32-bit value.
  *
  * Return: 0 if OK, -ve on error
  */
 int regmap_write(struct regmap *map, uint offset, uint val);
 
 /**
- * regmap_read() - Read a 32-bit value from a regmap
+ * regmap_read() - Read a value from a regmap
  *
  * @map:	Regmap to read from
  * @offset:	Offset in the regmap to read from
  * @valp:	Pointer to the buffer to receive the data read from the regmap
  *		at the specified offset
  *
- * Note that this function will only read values of 32 bit width from the
- * regmap; if the size of data to be read is different, the regmap_raw_read
- * function can be used.
+ * If the map's width is not set, this function will read a 32-bit value.
  *
  * Return: 0 if OK, -ve on error
  */
@@ -344,7 +352,7 @@ int regmap_init_mem_index(ofnode node, struct regmap **mapp, int index);
  * @dev: Device that will be interacted with
  * @bus: Bus-specific callbacks to use with device (IGNORED)
  * @bus_context: Data passed to bus-specific callbacks (IGNORED)
- * @config: Configuration for register map (IGNORED)
+ * @config: Configuration for register map
  *
  * @Return a valid pointer to a struct regmap or a ERR_PTR() on error.
  * The structure is automatically freed when the device is unbound
-- 
2.26.2

[PATCH 4/8] regmap: Allow left shifting register offset before access

[Pratyush Yadav]

Drivers can configure it to adjust the final read/write location.

Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
---
 drivers/core/regmap.c | 6 +++++-
 include/regmap.h      | 6 ++++++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/core/regmap.c b/drivers/core/regmap.c
index 0180246095..8ba4270d2d 100644
--- a/drivers/core/regmap.c
+++ b/drivers/core/regmap.c
@@ -257,8 +257,10 @@ struct regmap *devm_regmap_init(struct udevice *dev,
 		return ERR_PTR(rc);
 
 	map = *mapp;
-	if (config)
+	if (config) {
 		map->width = config->width;
+		map->reg_offset_shift = config->reg_offset_shift;
+	}
 
 	devres_add(dev, mapp);
 	return *mapp;
@@ -345,6 +347,7 @@ int regmap_raw_read_range(struct regmap *map, uint range_num, uint offset,
 	}
 	range = &map->ranges[range_num];
 
+	offset <<= map->reg_offset_shift;
 	if (offset + val_len > range->size) {
 		debug("%s: offset/size combination invalid\n", __func__);
 		return -ERANGE;
@@ -455,6 +458,7 @@ int regmap_raw_write_range(struct regmap *map, uint range_num, uint offset,
 	}
 	range = &map->ranges[range_num];
 
+	offset <<= map->reg_offset_shift;
 	if (offset + val_len > range->size) {
 		debug("%s: offset/size combination invalid\n", __func__);
 		return -ERANGE;
diff --git a/include/regmap.h b/include/regmap.h
index 9362d864c0..dacdf1f39b 100644
--- a/include/regmap.h
+++ b/include/regmap.h
@@ -82,9 +82,12 @@ struct regmap_bus;
  *
  * @width:		Width of the read/write operations. Defaults to
  *			REGMAP_SIZE_32 if set to 0.
+ * @reg_offset_shift	Left shift the register offset by this value before
+ *			performing read or write.
  */
 struct regmap_config {
 	enum regmap_size_t width;
+	u32 reg_offset_shift;
 };
 
 /**
@@ -92,12 +95,15 @@ struct regmap_config {
  *
  * @width:		Width of the read/write operations. Defaults to
  *			REGMAP_SIZE_32 if set to 0.
+ * @reg_offset_shift	Left shift the register offset by this value before
+ *			performing read or write.
  * @range_count:	Number of ranges available within the map
  * @ranges:		Array of ranges
  */
 struct regmap {
 	enum regmap_endianness_t endianness;
 	enum regmap_size_t width;
+	u32 reg_offset_shift;
 	int range_count;
 	struct regmap_range ranges[0];
 };
-- 
2.26.2

[PATCH 5/8] regmap: Add regmap_init_mem_range()

[Pratyush Yadav]

Right now, the base of a regmap can only be obtained from the device
tree. This makes it impossible for devices which calculate the base at
runtime to use a regmap. An example of such a device is the Cadence
Sierra PHY.

Allow creating a regmap with one range whose start and size can be
specified by the driver based on calculations at runtime.

Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
---
 drivers/core/regmap.c | 27 +++++++++++++++++++++++++++
 include/regmap.h      | 19 +++++++++++++++++++
 2 files changed, 46 insertions(+)

diff --git a/drivers/core/regmap.c b/drivers/core/regmap.c
index 8ba4270d2d..f019a4e9ce 100644
--- a/drivers/core/regmap.c
+++ b/drivers/core/regmap.c
@@ -160,6 +160,33 @@ err:
 	return ret;
 }
 
+int regmap_init_mem_range(ofnode node, ulong r_start, ulong r_size,
+			  struct regmap **mapp)
+{
+	struct regmap *map;
+	struct regmap_range *range;
+
+	map = regmap_alloc(1);
+	if (!map)
+		return -ENOMEM;
+
+	range = &map->ranges[0];
+	range->start = r_start;
+	range->size = r_size;
+
+	if (ofnode_read_bool(node, "little-endian"))
+		map->endianness = REGMAP_LITTLE_ENDIAN;
+	else if (ofnode_read_bool(node, "big-endian"))
+		map->endianness = REGMAP_BIG_ENDIAN;
+	else if (ofnode_read_bool(node, "native-endian"))
+		map->endianness = REGMAP_NATIVE_ENDIAN;
+	else /* Default: native endianness */
+		map->endianness = REGMAP_NATIVE_ENDIAN;
+
+	*mapp = map;
+	return 0;
+}
+
 int regmap_init_mem(ofnode node, struct regmap **mapp)
 {
 	struct regmap_range *range;
diff --git a/include/regmap.h b/include/regmap.h
index dacdf1f39b..ea6d8b253d 100644
--- a/include/regmap.h
+++ b/include/regmap.h
@@ -352,6 +352,25 @@ int regmap_init_mem_platdata(struct udevice *dev, fdt_val_t *reg, int count,
 
 int regmap_init_mem_index(ofnode node, struct regmap **mapp, int index);
 
+/**
+ * regmap_init_mem_range() - Set up a new memory region for ofnode with the
+ *			     specified range.
+ *
+ * @node:	The ofnode for the map.
+ * @r_start:	Start of the range.
+ * @r_size:	Size of the range.
+ * @mapp:	Returns allocated map.
+ *
+ * Return: 0 in success, -errno otherwise
+ *
+ * This creates a regmap with one range where instead of extracting the range
+ * from 'node', it is created based on the parameters specified. This is
+ * useful when a driver needs to calculate the base of the regmap at runtime,
+ * and can't specify it in device tree.
+ */
+int regmap_init_mem_range(ofnode node, ulong r_start, ulong r_size,
+			  struct regmap **mapp);
+
 /**
  * devm_regmap_init() - Initialise register map (device managed)
  *
-- 
2.26.2

[PATCH 6/8] regmap: Allow devices to specify regmap range start and size in config

[Pratyush Yadav]

Some devices need to calculate the regmap base address at runtime. This
makes it impossible to use device tree to get the regmap base. Instead,
allow devices to specify it in the regmap config. This will create a
regmap with a single range that corresponds to the start and size given
by the driver.

Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
---
 drivers/core/regmap.c | 6 +++++-
 include/regmap.h      | 6 ++++++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/core/regmap.c b/drivers/core/regmap.c
index f019a4e9ce..4792067f24 100644
--- a/drivers/core/regmap.c
+++ b/drivers/core/regmap.c
@@ -279,7 +279,11 @@ struct regmap *devm_regmap_init(struct udevice *dev,
 	if (unlikely(!mapp))
 		return ERR_PTR(-ENOMEM);
 
-	rc = regmap_init_mem(dev_ofnode(dev), mapp);
+	if (config && config->r_size != 0)
+		rc = regmap_init_mem_range(dev_ofnode(dev), config->r_start,
+					   config->r_size, mapp);
+	else
+		rc = regmap_init_mem(dev_ofnode(dev), mapp);
 	if (rc)
 		return ERR_PTR(rc);
 
diff --git a/include/regmap.h b/include/regmap.h
index ea6d8b253d..007e6f4b6f 100644
--- a/include/regmap.h
+++ b/include/regmap.h
@@ -84,10 +84,16 @@ struct regmap_bus;
  *			REGMAP_SIZE_32 if set to 0.
  * @reg_offset_shift	Left shift the register offset by this value before
  *			performing read or write.
+ * @r_start:		If specified, the regmap is created with one range
+ *			which starts at this address, instead of finding the
+ *			start from device tree.
+ * @r_size:		Same as above for the range size
  */
 struct regmap_config {
 	enum regmap_size_t width;
 	u32 reg_offset_shift;
+	ulong r_start;
+	ulong r_size;
 };
 
 /**
-- 
2.26.2

[PATCH 7/8] regmap: Add support for regmap fields

[Pratyush Yadav]

From: Jean-Jacques Hiblot <jjhiblot@ti.com>

A regmap field is an abstraction available in Linux. It provides to access
bitfields in a regmap without having to worry about shifts and masks.

Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
---
 drivers/core/regmap.c |  78 ++++++++++++++++++++++++++++++
 include/regmap.h      | 108 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 186 insertions(+)

diff --git a/drivers/core/regmap.c b/drivers/core/regmap.c
index 4792067f24..cbc01b689a 100644
--- a/drivers/core/regmap.c
+++ b/drivers/core/regmap.c
@@ -18,6 +18,15 @@
 #include <linux/ioport.h>
 #include <linux/compat.h>
 #include <linux/err.h>
+#include <linux/bitops.h>
+
+struct regmap_field {
+	struct regmap *regmap;
+	unsigned int mask;
+	/* lsb */
+	unsigned int shift;
+	unsigned int reg;
+};
 
 DECLARE_GLOBAL_DATA_PTR;
 
@@ -545,3 +554,72 @@ int regmap_update_bits(struct regmap *map, uint offset, uint mask, uint val)
 
 	return regmap_write(map, offset, reg | (val & mask));
 }
+
+int regmap_field_read(struct regmap_field *field, unsigned int *val)
+{
+	int ret;
+	unsigned int reg_val;
+
+	ret = regmap_read(field->regmap, field->reg, &reg_val);
+	if (ret != 0)
+		return ret;
+
+	reg_val &= field->mask;
+	reg_val >>= field->shift;
+	*val = reg_val;
+
+	return ret;
+}
+
+int regmap_field_write(struct regmap_field *field, unsigned int val)
+{
+	return regmap_update_bits(field->regmap, field->reg, field->mask,
+				  val << field->shift);
+}
+
+static void regmap_field_init(struct regmap_field *rm_field,
+			      struct regmap *regmap,
+			      struct reg_field reg_field)
+{
+	rm_field->regmap = regmap;
+	rm_field->reg = reg_field.reg;
+	rm_field->shift = reg_field.lsb;
+	rm_field->mask = GENMASK(reg_field.msb, reg_field.lsb);
+}
+
+struct regmap_field *devm_regmap_field_alloc(struct udevice *dev,
+					     struct regmap *regmap,
+					     struct reg_field reg_field)
+{
+	struct regmap_field *rm_field = devm_kzalloc(dev, sizeof(*rm_field),
+						     GFP_KERNEL);
+	if (!rm_field)
+		return ERR_PTR(-ENOMEM);
+
+	regmap_field_init(rm_field, regmap, reg_field);
+
+	return rm_field;
+}
+
+void devm_regmap_field_free(struct udevice *dev, struct regmap_field *field)
+{
+	devm_kfree(dev, field);
+}
+
+struct regmap_field *regmap_field_alloc(struct regmap *regmap,
+					struct reg_field reg_field)
+{
+	struct regmap_field *rm_field = kzalloc(sizeof(*rm_field), GFP_KERNEL);
+
+	if (!rm_field)
+		return ERR_PTR(-ENOMEM);
+
+	regmap_field_init(rm_field, regmap, reg_field);
+
+	return rm_field;
+}
+
+void regmap_field_free(struct regmap_field *field)
+{
+	kfree(field);
+}
diff --git a/include/regmap.h b/include/regmap.h
index 007e6f4b6f..190ea44f6a 100644
--- a/include/regmap.h
+++ b/include/regmap.h
@@ -314,6 +314,43 @@ int regmap_raw_read_range(struct regmap *map, uint range_num, uint offset,
 	regmap_read_poll_timeout_test(map, addr, val, cond, sleep_us, \
 				      timeout_ms, 0) \
 
+/**
+ * regmap_field_read_poll_timeout - Poll until a condition is met or a timeout
+ *				    occurs
+ *
+ * @field:	Regmap field to read from
+ * @val:	Unsigned integer variable to read the value into
+ * @cond:	Break condition (usually involving @val)
+ * @sleep_us:	Maximum time to sleep between reads in us (0 tight-loops).
+ * @timeout_ms:	Timeout in ms, 0 means never timeout
+ *
+ * Returns 0 on success and -ETIMEDOUT upon a timeout or the regmap_field_read
+ * error return value in case of a error read. In the two former cases,
+ * the last read value at @addr is stored in @val.
+ *
+ * This is modelled after the regmap_read_poll_timeout macros in linux but
+ * with millisecond timeout.
+ */
+#define regmap_field_read_poll_timeout(field, val, cond, sleep_us, timeout_ms) \
+({ \
+	unsigned long __start = get_timer(0); \
+	int __ret; \
+	for (;;) { \
+		__ret = regmap_field_read((field), &(val)); \
+		if (__ret) \
+			break; \
+		if (cond) \
+			break; \
+		if ((timeout_ms) && get_timer(__start) > (timeout_ms)) { \
+			__ret = regmap_field_read((field), &(val)); \
+			break; \
+		} \
+		if ((sleep_us)) \
+			udelay((sleep_us)); \
+	} \
+	__ret ?: ((cond) ? 0 : -ETIMEDOUT); \
+})
+
 /**
  * regmap_update_bits() - Perform a read/modify/write using a mask
  *
@@ -409,4 +446,75 @@ void *regmap_get_range(struct regmap *map, unsigned int range_num);
  */
 int regmap_uninit(struct regmap *map);
 
+/**
+ * struct reg_field - Description of an register field
+ *
+ * @reg: Offset of the register within the regmap bank
+ * @lsb: lsb of the register field.
+ * @msb: msb of the register field.
+ * @id_size: port size if it has some ports
+ * @id_offset: address offset for each ports
+ */
+struct reg_field {
+	unsigned int reg;
+	unsigned int lsb;
+	unsigned int msb;
+};
+
+struct regmap_field;
+
+#define REG_FIELD(_reg, _lsb, _msb) {		\
+				.reg = _reg,	\
+				.lsb = _lsb,	\
+				.msb = _msb,	\
+				}
+
+/**
+ * devm_regmap_field_alloc() - Allocate and initialise a register field.
+ *
+ * @dev: Device that will be interacted with
+ * @regmap: regmap bank in which this register field is located.
+ * @reg_field: Register field with in the bank.
+ *
+ * The return value will be an ERR_PTR() on error or a valid pointer
+ * to a struct regmap_field. The regmap_field will be automatically freed
+ * by the device management code.
+ */
+struct regmap_field *devm_regmap_field_alloc(struct udevice *dev,
+					     struct regmap *regmap,
+					     struct reg_field reg_field);
+/**
+ * devm_regmap_field_free() - Free a register field allocated using
+ *                            devm_regmap_field_alloc.
+ *
+ * @dev: Device that will be interacted with
+ * @field: regmap field which should be freed.
+ *
+ * Free register field allocated using devm_regmap_field_alloc(). Usually
+ * drivers need not call this function, as the memory allocated via devm
+ * will be freed as per device-driver life-cyle.
+ */
+void devm_regmap_field_free(struct udevice *dev, struct regmap_field *field);
+
+/**
+ * regmap_field_write() - Write a value to a regmap field
+ *
+ * @field:	Regmap field to write to
+ * @val:	Data to write to the regmap at the specified offset
+ *
+ * Return: 0 if OK, -ve on error
+ */
+int regmap_field_write(struct regmap_field *field, unsigned int val);
+
+/**
+ * regmap_read() - Read a 32-bit value from a regmap
+ *
+ * @field:	Regmap field to write to
+ * @valp:	Pointer to the buffer to receive the data read from the regmap
+ *		field
+ *
+ * Return: 0 if OK, -ve on error
+ */
+int regmap_field_read(struct regmap_field *field, unsigned int *val);
+
 #endif
-- 
2.26.2

[PATCH 8/8] test: dm: Add tests for regmap managed API and regmap fields

[Pratyush Yadav]

From: Jean-Jacques Hiblot <jjhiblot@ti.com>

The tests rely on a dummy driver to allocate and initialize the regmaps
and the regmap fields using the managed API. The first test checks if
the regmap config fields like width, reg_offset_shift, range specifiers,
etc work. The second test checks if regmap fields behave properly (mask
and shift are ok) by peeking into the regmap.

Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
---
 arch/sandbox/dts/test.dts |  13 +++
 test/dm/regmap.c          | 198 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 211 insertions(+)

diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts
index 5ce5e28476..5b2c6ca79f 100644
--- a/arch/sandbox/dts/test.dts
+++ b/arch/sandbox/dts/test.dts
@@ -1029,6 +1029,19 @@
 	mdio: mdio-test {
 		compatible = "sandbox,mdio";
 	};
+
+	some_regmapped-bus {
+		#address-cells = <0x1>;
+		#size-cells = <0x1>;
+
+		ranges = <0x0 0x0 0x10>;
+		compatible = "simple-bus";
+
+		regmap-test_0 {
+			reg = <0 0x10>;
+			compatible = "sandbox,regmap_test";
+		};
+	};
 };
 
 #include "sandbox_pmic.dtsi"
diff --git a/test/dm/regmap.c b/test/dm/regmap.c
index 809494d585..3bd197a94a 100644
--- a/test/dm/regmap.c
+++ b/test/dm/regmap.c
@@ -9,8 +9,10 @@
 #include <mapmem.h>
 #include <regmap.h>
 #include <syscon.h>
+#include <rand.h>
 #include <asm/test.h>
 #include <dm/test.h>
+#include <dm/devres.h>
 #include <linux/err.h>
 #include <test/ut.h>
 
@@ -186,3 +188,199 @@ static int dm_test_regmap_poll(struct unit_test_state *uts)
 }
 
 DM_TEST(dm_test_regmap_poll, DM_TESTF_SCAN_PDATA | DM_TESTF_SCAN_FDT);
+
+struct regmaptest_priv {
+	struct regmap *cfg_regmap; /* For testing regmap_config options. */
+	struct regmap *fld_regmap; /* For testing regmap fields. */
+	struct regmap_field **fields;
+};
+
+static const struct reg_field field_cfgs[] = {
+	{
+		.reg = 0,
+		.lsb = 0,
+		.msb = 6,
+	},
+	{
+		.reg = 2,
+		.lsb = 4,
+		.msb = 12,
+	},
+	{
+		.reg = 2,
+		.lsb = 12,
+		.msb = 15,
+	}
+};
+
+#define REGMAP_TEST_BUF_START 0
+#define REGMAP_TEST_BUF_SZ 5
+
+static int remaptest_probe(struct udevice *dev)
+{
+	struct regmaptest_priv *priv = dev_get_priv(dev);
+	struct regmap *regmap;
+	struct regmap_field *field;
+	struct regmap_config cfg;
+	int i;
+	static const int n = ARRAY_SIZE(field_cfgs);
+
+	/*
+	 * To exercise all the regmap config options, create a regmap that
+	 * points to a custom memory area instead of the one defined in device
+	 * tree. Use 2-byte elements. To allow directly indexing into the
+	 * elements, use an offset shift of 1. So, accessing offset 1 gets the
+	 * element at index 1 at memory location 2.
+	 *
+	 * REGMAP_TEST_BUF_SZ is the number of elements, so we need to multiply
+	 * it by 2 because r_size expects number of bytes.
+	 */
+	cfg.reg_offset_shift = 1;
+	cfg.r_start = REGMAP_TEST_BUF_START;
+	cfg.r_size = REGMAP_TEST_BUF_SZ * 2;
+	cfg.width = REGMAP_SIZE_16;
+
+	regmap = devm_regmap_init(dev, NULL, NULL, &cfg);
+	if (IS_ERR(regmap))
+		return PTR_ERR(regmap);
+	priv->cfg_regmap = regmap;
+
+	memset(&cfg, 0, sizeof(struct regmap_config));
+	cfg.width = REGMAP_SIZE_16;
+
+	regmap = devm_regmap_init(dev, NULL, NULL, &cfg);
+	if (IS_ERR(regmap))
+		return PTR_ERR(regmap);
+	priv->fld_regmap = regmap;
+
+	priv->fields = devm_kzalloc(dev, sizeof(struct regmap_field *) * n,
+				    GFP_KERNEL);
+	if (!priv->fields)
+		return -ENOMEM;
+
+	for (i = 0 ; i < n; i++) {
+		field = devm_regmap_field_alloc(dev, priv->fld_regmap,
+						field_cfgs[i]);
+		if (IS_ERR(field))
+			return PTR_ERR(field);
+		priv->fields[i] = field;
+	}
+
+	return 0;
+}
+
+static const struct udevice_id regmaptest_ids[] = {
+	{ .compatible = "sandbox,regmap_test" },
+	{ }
+};
+
+U_BOOT_DRIVER(regmap_test) = {
+	.name	= "regmaptest_drv",
+	.of_match	= regmaptest_ids,
+	.id	= UCLASS_NOP,
+	.probe = remaptest_probe,
+	.priv_auto_alloc_size = sizeof(struct regmaptest_priv),
+};
+
+static int dm_test_devm_regmap(struct unit_test_state *uts)
+{
+	int i = 0;
+	u32 val;
+	u16 pattern[REGMAP_TEST_BUF_SZ];
+	u16 *buffer;
+	struct udevice *dev;
+	struct regmaptest_priv *priv;
+
+	sandbox_set_enable_memio(true);
+
+	/*
+	 * Map the memory area the regmap should point to so we can make sure
+	 * the writes actually go to that location.
+	 */
+	buffer = map_physmem(REGMAP_TEST_BUF_START,
+			     REGMAP_TEST_BUF_SZ * 2, MAP_NOCACHE);
+
+	ut_assertok(uclass_get_device_by_name(UCLASS_NOP, "regmap-test_0",
+					      &dev));
+	priv = dev_get_priv(dev);
+
+	srand(get_ticks() + rand());
+	for (i = 0; i < REGMAP_TEST_BUF_SZ; i++) {
+		pattern[i] = rand();
+		ut_assertok(regmap_write(priv->cfg_regmap, i, pattern[i]));
+	}
+	for (i = 0; i < REGMAP_TEST_BUF_SZ; i++) {
+		ut_assertok(regmap_read(priv->cfg_regmap, i, &val));
+		ut_asserteq(val, buffer[i]);
+		ut_asserteq(val, pattern[i]);
+	}
+
+	ut_asserteq(-ERANGE, regmap_write(priv->cfg_regmap, REGMAP_TEST_BUF_SZ,
+					  val));
+	ut_asserteq(-ERANGE, regmap_read(priv->cfg_regmap, REGMAP_TEST_BUF_SZ,
+					 &val));
+	ut_asserteq(-ERANGE, regmap_write(priv->cfg_regmap, -1, val));
+	ut_asserteq(-ERANGE, regmap_read(priv->cfg_regmap, -1, &val));
+
+	return 0;
+}
+DM_TEST(dm_test_devm_regmap, DM_TESTF_SCAN_PDATA | DM_TESTF_SCAN_FDT);
+
+static int test_one_field(struct unit_test_state *uts,
+			  struct regmap *regmap,
+			  struct regmap_field *field,
+			  struct reg_field field_cfg)
+{
+	int j;
+	unsigned int val;
+	int mask = (1 << (field_cfg.msb - field_cfg.lsb + 1)) - 1;
+	int shift = field_cfg.lsb;
+
+	ut_assertok(regmap_write(regmap, field_cfg.reg, 0));
+	ut_assertok(regmap_read(regmap, field_cfg.reg, &val));
+	ut_asserteq(0, val);
+
+	for (j = 0; j <= mask; j++) {
+		ut_assertok(regmap_field_write(field, j));
+		ut_assertok(regmap_field_read(field, &val));
+		ut_asserteq(j, val);
+		ut_assertok(regmap_read(regmap, field_cfg.reg, &val));
+		ut_asserteq(j << shift, val);
+	}
+
+	ut_assertok(regmap_field_write(field, mask + 1));
+	ut_assertok(regmap_read(regmap, field_cfg.reg, &val));
+	ut_asserteq(0, val);
+
+	ut_assertok(regmap_field_write(field, 0xFFFF));
+	ut_assertok(regmap_read(regmap, field_cfg.reg, &val));
+	ut_asserteq(mask << shift, val);
+
+	ut_assertok(regmap_write(regmap, field_cfg.reg, 0xFFFF));
+	ut_assertok(regmap_field_write(field, 0));
+	ut_assertok(regmap_read(regmap, field_cfg.reg, &val));
+	ut_asserteq(0xFFFF & ~(mask << shift), val);
+	return 0;
+}
+
+static int dm_test_devm_regmap_field(struct unit_test_state *uts)
+{
+	int i, rc;
+	struct udevice *dev;
+	struct regmaptest_priv *priv;
+
+	ut_assertok(uclass_get_device_by_name(UCLASS_NOP, "regmap-test_0",
+					      &dev));
+	priv = dev_get_priv(dev);
+
+	sandbox_set_enable_memio(true);
+	for (i = 0 ; i < ARRAY_SIZE(field_cfgs); i++) {
+		rc = test_one_field(uts, priv->fld_regmap, priv->fields[i],
+				    field_cfgs[i]);
+		if (rc)
+			break;
+	}
+
+	return 0;
+}
+DM_TEST(dm_test_devm_regmap_field, DM_TESTF_SCAN_PDATA | DM_TESTF_SCAN_FDT);
-- 
2.26.2

[PATCH 2/8] regmap: zero out the regmap on allocation

[Simon Glass]

Hi Pratyush,

On Wed, 27 May 2020 at 06:52, Pratyush Yadav <p.yadav@ti.com> wrote:
>
> Some fields will be introduced in the regmap structure that should be
> set to 0 by default. So, once we allocate a regmap, make sure it is
> zeroed out to avoid unexpected defaults for those values.
>
> Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
> ---
>  drivers/core/regmap.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

But I think you should use calloc() instead


> diff --git a/drivers/core/regmap.c b/drivers/core/regmap.c
> index 74225361fd..24651fb3ec 100644
> --- a/drivers/core/regmap.c
> +++ b/drivers/core/regmap.c
> @@ -30,10 +30,12 @@ DECLARE_GLOBAL_DATA_PTR;
>  static struct regmap *regmap_alloc(int count)
>  {
>         struct regmap *map;
> +       size_t size = sizeof(*map) + sizeof(map->ranges[0]) * count;
>
> -       map = malloc(sizeof(*map) + sizeof(map->ranges[0]) * count);
> +       map = malloc(size);
>         if (!map)
>                 return NULL;
> +       memset(map, 0, size);
>         map->range_count = count;
>
>         return map;
> --
> 2.26.2
>

[PATCH 3/8] regmap: Allow specifying read/write width

[Simon Glass]

Hi Pratyush,

On Wed, 27 May 2020 at 06:52, Pratyush Yadav <p.yadav@ti.com> wrote:
>
> Right now, regmap_read() and regmap_write() read/write a 32-bit value
> only. To write other lengths, regmap_raw_read() and regmap_raw_write()
> need to be used.
>
> This means that any driver ported from Linux that relies on
> regmap_{read,write}() to know the size already has to be updated at each
> callsite. This makes the port harder to maintain.
>
> So, allow specifying the read/write width to make it easier to port the
> drivers, since now the only change needed is when initializing the
> regmap.
>
> Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
> ---
>  drivers/core/regmap.c | 12 +++++++++---
>  include/regmap.h      | 28 ++++++++++++++++++----------
>  2 files changed, 27 insertions(+), 13 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

See below

>
> diff --git a/drivers/core/regmap.c b/drivers/core/regmap.c
> index 24651fb3ec..0180246095 100644
> --- a/drivers/core/regmap.c
> +++ b/drivers/core/regmap.c
> @@ -245,7 +245,7 @@ struct regmap *devm_regmap_init(struct udevice *dev,
>                                 const struct regmap_config *config)
>  {
>         int rc;
> -       struct regmap **mapp;
> +       struct regmap **mapp, *map;
>
>         mapp = devres_alloc(devm_regmap_release, sizeof(struct regmap *),
>                             __GFP_ZERO);
> @@ -256,6 +256,10 @@ struct regmap *devm_regmap_init(struct udevice *dev,
>         if (rc)
>                 return ERR_PTR(rc);
>
> +       map = *mapp;
> +       if (config)
> +               map->width = config->width;
> +
>         devres_add(dev, mapp);
>         return *mapp;
>  }
> @@ -378,7 +382,8 @@ int regmap_raw_read(struct regmap *map, uint offset, void *valp, size_t val_len)
>
>  int regmap_read(struct regmap *map, uint offset, uint *valp)
>  {
> -       return regmap_raw_read(map, offset, valp, REGMAP_SIZE_32);
> +       return regmap_raw_read(map, offset, valp,
> +                              map->width ? map->width : REGMAP_SIZE_32);

Can you set up map->width so you can avoid the checks?

Regards,
Simon

[PATCH 4/8] regmap: Allow left shifting register offset before access

[Simon Glass]

On Wed, 27 May 2020 at 06:52, Pratyush Yadav <p.yadav@ti.com> wrote:
>
> Drivers can configure it to adjust the final read/write location.
>
> Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
> ---
>  drivers/core/regmap.c | 6 +++++-
>  include/regmap.h      | 6 ++++++
>  2 files changed, 11 insertions(+), 1 deletion(-)
>
Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH 5/8] regmap: Add regmap_init_mem_range()

[Simon Glass]

On Wed, 27 May 2020 at 06:52, Pratyush Yadav <p.yadav@ti.com> wrote:
>
> Right now, the base of a regmap can only be obtained from the device
> tree. This makes it impossible for devices which calculate the base at
> runtime to use a regmap. An example of such a device is the Cadence
> Sierra PHY.
>
> Allow creating a regmap with one range whose start and size can be
> specified by the driver based on calculations at runtime.
>
> Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
> ---
>  drivers/core/regmap.c | 27 +++++++++++++++++++++++++++
>  include/regmap.h      | 19 +++++++++++++++++++
>  2 files changed, 46 insertions(+)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH 6/8] regmap: Allow devices to specify regmap range start and size in config

[Simon Glass]

On Wed, 27 May 2020 at 06:52, Pratyush Yadav <p.yadav@ti.com> wrote:
>
> Some devices need to calculate the regmap base address at runtime. This
> makes it impossible to use device tree to get the regmap base. Instead,
> allow devices to specify it in the regmap config. This will create a
> regmap with a single range that corresponds to the start and size given
> by the driver.
>
> Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
> ---
>  drivers/core/regmap.c | 6 +++++-
>  include/regmap.h      | 6 ++++++
>  2 files changed, 11 insertions(+), 1 deletion(-)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH 7/8] regmap: Add support for regmap fields

[Simon Glass]

Hi Pratyush,

On Wed, 27 May 2020 at 06:52, Pratyush Yadav <p.yadav@ti.com> wrote:
>
> From: Jean-Jacques Hiblot <jjhiblot@ti.com>
>
> A regmap field is an abstraction available in Linux. It provides to access
> bitfields in a regmap without having to worry about shifts and masks.
>
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
> ---
>  drivers/core/regmap.c |  78 ++++++++++++++++++++++++++++++
>  include/regmap.h      | 108 ++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 186 insertions(+)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

Please see below

> diff --git a/drivers/core/regmap.c b/drivers/core/regmap.c
> index 4792067f24..cbc01b689a 100644
> --- a/drivers/core/regmap.c
> +++ b/drivers/core/regmap.c
> @@ -18,6 +18,15 @@
>  #include <linux/ioport.h>
>  #include <linux/compat.h>
>  #include <linux/err.h>
> +#include <linux/bitops.h>
> +
> +struct regmap_field {

Needs comments as I'm not sure what this is for

> +       struct regmap *regmap;
> +       unsigned int mask;
> +       /* lsb */
> +       unsigned int shift;
> +       unsigned int reg;
> +};
>
>  DECLARE_GLOBAL_DATA_PTR;
>
> @@ -545,3 +554,72 @@ int regmap_update_bits(struct regmap *map, uint offset, uint mask, uint val)
>
>         return regmap_write(map, offset, reg | (val & mask));
>  }
> +
> +int regmap_field_read(struct regmap_field *field, unsigned int *val)
> +{
> +       int ret;
> +       unsigned int reg_val;
> +
> +       ret = regmap_read(field->regmap, field->reg, &reg_val);
> +       if (ret != 0)
> +               return ret;
> +
> +       reg_val &= field->mask;
> +       reg_val >>= field->shift;
> +       *val = reg_val;
> +
> +       return ret;
> +}
> +
> +int regmap_field_write(struct regmap_field *field, unsigned int val)
> +{
> +       return regmap_update_bits(field->regmap, field->reg, field->mask,
> +                                 val << field->shift);
> +}
> +
> +static void regmap_field_init(struct regmap_field *rm_field,
> +                             struct regmap *regmap,
> +                             struct reg_field reg_field)
> +{
> +       rm_field->regmap = regmap;
> +       rm_field->reg = reg_field.reg;
> +       rm_field->shift = reg_field.lsb;
> +       rm_field->mask = GENMASK(reg_field.msb, reg_field.lsb);
> +}
> +
> +struct regmap_field *devm_regmap_field_alloc(struct udevice *dev,
> +                                            struct regmap *regmap,
> +                                            struct reg_field reg_field)
> +{
> +       struct regmap_field *rm_field = devm_kzalloc(dev, sizeof(*rm_field),
> +                                                    GFP_KERNEL);
> +       if (!rm_field)
> +               return ERR_PTR(-ENOMEM);
> +
> +       regmap_field_init(rm_field, regmap, reg_field);
> +
> +       return rm_field;
> +}
> +
> +void devm_regmap_field_free(struct udevice *dev, struct regmap_field *field)
> +{
> +       devm_kfree(dev, field);
> +}
> +
> +struct regmap_field *regmap_field_alloc(struct regmap *regmap,
> +                                       struct reg_field reg_field)
> +{
> +       struct regmap_field *rm_field = kzalloc(sizeof(*rm_field), GFP_KERNEL);
> +
> +       if (!rm_field)
> +               return ERR_PTR(-ENOMEM);
> +
> +       regmap_field_init(rm_field, regmap, reg_field);
> +
> +       return rm_field;
> +}
> +
> +void regmap_field_free(struct regmap_field *field)
> +{
> +       kfree(field);
> +}
> diff --git a/include/regmap.h b/include/regmap.h
> index 007e6f4b6f..190ea44f6a 100644
> --- a/include/regmap.h
> +++ b/include/regmap.h
> @@ -314,6 +314,43 @@ int regmap_raw_read_range(struct regmap *map, uint range_num, uint offset,
>         regmap_read_poll_timeout_test(map, addr, val, cond, sleep_us, \
>                                       timeout_ms, 0) \
>
> +/**
> + * regmap_field_read_poll_timeout - Poll until a condition is met or a timeout
> + *                                 occurs
> + *
> + * @field:     Regmap field to read from
> + * @val:       Unsigned integer variable to read the value into
> + * @cond:      Break condition (usually involving @val)
> + * @sleep_us:  Maximum time to sleep between reads in us (0 tight-loops).
> + * @timeout_ms:        Timeout in ms, 0 means never timeout
> + *
> + * Returns 0 on success and -ETIMEDOUT upon a timeout or the regmap_field_read
> + * error return value in case of a error read. In the two former cases,
> + * the last read value at @addr is stored in @val.
> + *
> + * This is modelled after the regmap_read_poll_timeout macros in linux but
> + * with millisecond timeout.
> + */
> +#define regmap_field_read_poll_timeout(field, val, cond, sleep_us, timeout_ms) \
> +({ \
> +       unsigned long __start = get_timer(0); \
> +       int __ret; \
> +       for (;;) { \
> +               __ret = regmap_field_read((field), &(val)); \
> +               if (__ret) \
> +                       break; \
> +               if (cond) \
> +                       break; \
> +               if ((timeout_ms) && get_timer(__start) > (timeout_ms)) { \
> +                       __ret = regmap_field_read((field), &(val)); \
> +                       break; \
> +               } \
> +               if ((sleep_us)) \
> +                       udelay((sleep_us)); \
> +       } \
> +       __ret ?: ((cond) ? 0 : -ETIMEDOUT); \
> +})
> +
>  /**
>   * regmap_update_bits() - Perform a read/modify/write using a mask
>   *
> @@ -409,4 +446,75 @@ void *regmap_get_range(struct regmap *map, unsigned int range_num);
>   */
>  int regmap_uninit(struct regmap *map);
>
> +/**
> + * struct reg_field - Description of an register field
> + *
> + * @reg: Offset of the register within the regmap bank
> + * @lsb: lsb of the register field.
> + * @msb: msb of the register field.
> + * @id_size: port size if it has some ports
> + * @id_offset: address offset for each ports

Those last two don't seem to be present.

> + */
> +struct reg_field {
> +       unsigned int reg;
> +       unsigned int lsb;
> +       unsigned int msb;
> +};
> +
> +struct regmap_field;
> +
> +#define REG_FIELD(_reg, _lsb, _msb) {          \

comment and perhaps an example of how to use it

> +                               .reg = _reg,    \
> +                               .lsb = _lsb,    \
> +                               .msb = _msb,    \
> +                               }
> +
> +/**
> + * devm_regmap_field_alloc() - Allocate and initialise a register field.
> + *
> + * @dev: Device that will be interacted with
> + * @regmap: regmap bank in which this register field is located.
> + * @reg_field: Register field with in the bank.
> + *
> + * The return value will be an ERR_PTR() on error or a valid pointer
> + * to a struct regmap_field. The regmap_field will be automatically freed
> + * by the device management code.
> + */
> +struct regmap_field *devm_regmap_field_alloc(struct udevice *dev,
> +                                            struct regmap *regmap,
> +                                            struct reg_field reg_field);
> +/**
> + * devm_regmap_field_free() - Free a register field allocated using
> + *                            devm_regmap_field_alloc.
> + *
> + * @dev: Device that will be interacted with
> + * @field: regmap field which should be freed.
> + *
> + * Free register field allocated using devm_regmap_field_alloc(). Usually
> + * drivers need not call this function, as the memory allocated via devm
> + * will be freed as per device-driver life-cyle.
> + */
> +void devm_regmap_field_free(struct udevice *dev, struct regmap_field *field);
> +
> +/**
> + * regmap_field_write() - Write a value to a regmap field
> + *
> + * @field:     Regmap field to write to
> + * @val:       Data to write to the regmap at the specified offset
> + *
> + * Return: 0 if OK, -ve on error
> + */
> +int regmap_field_write(struct regmap_field *field, unsigned int val);
> +
> +/**
> + * regmap_read() - Read a 32-bit value from a regmap
> + *
> + * @field:     Regmap field to write to
> + * @valp:      Pointer to the buffer to receive the data read from the regmap
> + *             field
> + *
> + * Return: 0 if OK, -ve on error
> + */
> +int regmap_field_read(struct regmap_field *field, unsigned int *val);
> +
>  #endif
> --
> 2.26.2
>

Regards,
Simon

[PATCH 8/8] test: dm: Add tests for regmap managed API and regmap fields

[Simon Glass]

On Wed, 27 May 2020 at 06:52, Pratyush Yadav <p.yadav@ti.com> wrote:
>
> From: Jean-Jacques Hiblot <jjhiblot@ti.com>
>
> The tests rely on a dummy driver to allocate and initialize the regmaps
> and the regmap fields using the managed API. The first test checks if
> the regmap config fields like width, reg_offset_shift, range specifiers,
> etc work. The second test checks if regmap fields behave properly (mask
> and shift are ok) by peeking into the regmap.
>
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
> Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
> ---
>  arch/sandbox/dts/test.dts |  13 +++
>  test/dm/regmap.c          | 198 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 211 insertions(+)

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH 3/8] regmap: Allow specifying read/write width

[Pratyush Yadav]

On 31/05/20 08:08AM, Simon Glass wrote:
> Hi Pratyush,
> 
> On Wed, 27 May 2020 at 06:52, Pratyush Yadav <p.yadav@ti.com> wrote:
> >
> > Right now, regmap_read() and regmap_write() read/write a 32-bit value
> > only. To write other lengths, regmap_raw_read() and regmap_raw_write()
> > need to be used.
> >
> > This means that any driver ported from Linux that relies on
> > regmap_{read,write}() to know the size already has to be updated at each
> > callsite. This makes the port harder to maintain.
> >
> > So, allow specifying the read/write width to make it easier to port the
> > drivers, since now the only change needed is when initializing the
> > regmap.
> >
> > Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
> > ---
> >  drivers/core/regmap.c | 12 +++++++++---
> >  include/regmap.h      | 28 ++++++++++++++++++----------
> >  2 files changed, 27 insertions(+), 13 deletions(-)
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> 
> See below
> 
> >
> > diff --git a/drivers/core/regmap.c b/drivers/core/regmap.c
> > index 24651fb3ec..0180246095 100644
> > --- a/drivers/core/regmap.c
> > +++ b/drivers/core/regmap.c
> > @@ -245,7 +245,7 @@ struct regmap *devm_regmap_init(struct udevice *dev,
> >                                 const struct regmap_config *config)
> >  {
> >         int rc;
> > -       struct regmap **mapp;
> > +       struct regmap **mapp, *map;
> >
> >         mapp = devres_alloc(devm_regmap_release, sizeof(struct regmap *),
> >                             __GFP_ZERO);
> > @@ -256,6 +256,10 @@ struct regmap *devm_regmap_init(struct udevice *dev,
> >         if (rc)
> >                 return ERR_PTR(rc);
> >
> > +       map = *mapp;
> > +       if (config)
> > +               map->width = config->width;
> > +
> >         devres_add(dev, mapp);
> >         return *mapp;
> >  }
> > @@ -378,7 +382,8 @@ int regmap_raw_read(struct regmap *map, uint offset, void *valp, size_t val_len)
> >
> >  int regmap_read(struct regmap *map, uint offset, uint *valp)
> >  {
> > -       return regmap_raw_read(map, offset, valp, REGMAP_SIZE_32);
> > +       return regmap_raw_read(map, offset, valp,
> > +                              map->width ? map->width : REGMAP_SIZE_32);
> 
> Can you set up map->width so you can avoid the checks?

Ok. I'll set the default to 4 in regmap_alloc() and let callers 
over-ride it if they need.

-- 
Regards,
Pratyush Yadav
Texas Instruments India

[PATCH 2/8] regmap: zero out the regmap on allocation

[Pratyush Yadav]

Hi Simon,

On 31/05/20 08:08AM, Simon Glass wrote:
> Hi Pratyush,
> 
> On Wed, 27 May 2020 at 06:52, Pratyush Yadav <p.yadav@ti.com> wrote:
> >
> > Some fields will be introduced in the regmap structure that should be
> > set to 0 by default. So, once we allocate a regmap, make sure it is
> > zeroed out to avoid unexpected defaults for those values.
> >
> > Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
> > ---
> >  drivers/core/regmap.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> 
> But I think you should use calloc() instead

Ok. Will do. FWIW, I don't see a clear separation of the total size into 
elements and size of each element so I think doing something like 
calloc(1, size) is a bit strange.
 
> > diff --git a/drivers/core/regmap.c b/drivers/core/regmap.c
> > index 74225361fd..24651fb3ec 100644
> > --- a/drivers/core/regmap.c
> > +++ b/drivers/core/regmap.c
> > @@ -30,10 +30,12 @@ DECLARE_GLOBAL_DATA_PTR;
> >  static struct regmap *regmap_alloc(int count)
> >  {
> >         struct regmap *map;
> > +       size_t size = sizeof(*map) + sizeof(map->ranges[0]) * count;
> >
> > -       map = malloc(sizeof(*map) + sizeof(map->ranges[0]) * count);
> > +       map = malloc(size);
> >         if (!map)
> >                 return NULL;
> > +       memset(map, 0, size);
> >         map->range_count = count;
> >
> >         return map;

-- 
Regards,
Pratyush Yadav
Texas Instruments India

[PATCH 7/8] regmap: Add support for regmap fields

[Pratyush Yadav]

Hi Simon,

On 31/05/20 08:08AM, Simon Glass wrote:
> Hi Pratyush,
> 
> On Wed, 27 May 2020 at 06:52, Pratyush Yadav <p.yadav@ti.com> wrote:
> >
> > From: Jean-Jacques Hiblot <jjhiblot@ti.com>
> >
> > A regmap field is an abstraction available in Linux. It provides to access
> > bitfields in a regmap without having to worry about shifts and masks.
> >
> > Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
> > Reviewed-by: Simon Glass <sjg@chromium.org>
> > Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
> > ---
> >  drivers/core/regmap.c |  78 ++++++++++++++++++++++++++++++
> >  include/regmap.h      | 108 ++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 186 insertions(+)
> >
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> 
> Please see below
> 
> > diff --git a/drivers/core/regmap.c b/drivers/core/regmap.c
> > index 4792067f24..cbc01b689a 100644
> > --- a/drivers/core/regmap.c
> > +++ b/drivers/core/regmap.c
> > @@ -18,6 +18,15 @@
> >  #include <linux/ioport.h>
> >  #include <linux/compat.h>
> >  #include <linux/err.h>
> > +#include <linux/bitops.h>
> > +
> > +struct regmap_field {
> 
> Needs comments as I'm not sure what this is for

Heh, I'm that sure either. I went digging in the Linux source, and the 
only commit that touches this is the one that added regmap fields and it 
doesn't mention why we have both "regmap_field" and "reg_field".

Looking at the patch, I gather that regmap_field and reg_field look at 
the field in two different ways. reg_field is how a driver would look at 
a regmap field: which register this field belongs to, and what are its 
LSB and MSB? Datasheets usually tell you this information directly.

regmap_field looks at it from a lower level view: what register this 
feild belongs to, and how do I need to shift and mask the data I read 
from the bus? This is closer to how to actually read/write the field. 
The fact that regmap_field's definition is private to regmap.c also 
points in this direction.

The conversion from reg_field to regmap_field is done in 
regmap_field_init().

As to why have these two representations of the same thing, I can't say 
with certainty. My guess is that it makes the internal regmap code 
cleaner and slightly faster because you don't have to calculate the 
shift and mask every time.
 
> > +       struct regmap *regmap;
> > +       unsigned int mask;
> > +       /* lsb */
> > +       unsigned int shift;
> > +       unsigned int reg;
> > +};
> >
> >  DECLARE_GLOBAL_DATA_PTR;
> >
[..]
> > diff --git a/include/regmap.h b/include/regmap.h
> > index 007e6f4b6f..190ea44f6a 100644
> > --- a/include/regmap.h
> > +++ b/include/regmap.h
> > @@ -314,6 +314,43 @@ int regmap_raw_read_range(struct regmap *map, uint range_num, uint offset,
> >         regmap_read_poll_timeout_test(map, addr, val, cond, sleep_us, \
> >                                       timeout_ms, 0) \
> >
> > +/**
> > + * regmap_field_read_poll_timeout - Poll until a condition is met or a timeout
> > + *                                 occurs
> > + *
> > + * @field:     Regmap field to read from
> > + * @val:       Unsigned integer variable to read the value into
> > + * @cond:      Break condition (usually involving @val)
> > + * @sleep_us:  Maximum time to sleep between reads in us (0 tight-loops).
> > + * @timeout_ms:        Timeout in ms, 0 means never timeout
> > + *
> > + * Returns 0 on success and -ETIMEDOUT upon a timeout or the regmap_field_read
> > + * error return value in case of a error read. In the two former cases,
> > + * the last read value at @addr is stored in @val.
> > + *
> > + * This is modelled after the regmap_read_poll_timeout macros in linux but
> > + * with millisecond timeout.
> > + */
> > +#define regmap_field_read_poll_timeout(field, val, cond, sleep_us, timeout_ms) \
> > +({ \
> > +       unsigned long __start = get_timer(0); \
> > +       int __ret; \
> > +       for (;;) { \
> > +               __ret = regmap_field_read((field), &(val)); \
> > +               if (__ret) \
> > +                       break; \
> > +               if (cond) \
> > +                       break; \
> > +               if ((timeout_ms) && get_timer(__start) > (timeout_ms)) { \
> > +                       __ret = regmap_field_read((field), &(val)); \
> > +                       break; \
> > +               } \
> > +               if ((sleep_us)) \
> > +                       udelay((sleep_us)); \
> > +       } \
> > +       __ret ?: ((cond) ? 0 : -ETIMEDOUT); \
> > +})
> > +
> >  /**
> >   * regmap_update_bits() - Perform a read/modify/write using a mask
> >   *
> > @@ -409,4 +446,75 @@ void *regmap_get_range(struct regmap *map, unsigned int range_num);
> >   */
> >  int regmap_uninit(struct regmap *map);
> >
> > +/**
> > + * struct reg_field - Description of an register field
> > + *
> > + * @reg: Offset of the register within the regmap bank
> > + * @lsb: lsb of the register field.
> > + * @msb: msb of the register field.
> > + * @id_size: port size if it has some ports
> > + * @id_offset: address offset for each ports
> 
> Those last two don't seem to be present.

Good catch! Will drop them.
 
> > + */
> > +struct reg_field {
> > +       unsigned int reg;
> > +       unsigned int lsb;
> > +       unsigned int msb;
> > +};
> > +
> > +struct regmap_field;
> > +
> > +#define REG_FIELD(_reg, _lsb, _msb) {          \
> 
> comment and perhaps an example of how to use it

Ok.
 
> > +                               .reg = _reg,    \
> > +                               .lsb = _lsb,    \
> > +                               .msb = _msb,    \
> > +                               }
> > +

-- 
Regards,
Pratyush Yadav
Texas Instruments India

[PATCH 7/8] regmap: Add support for regmap fields

[Simon Glass]

Hi Pratyush,

On Fri, 5 Jun 2020 at 13:03, Pratyush Yadav <p.yadav@ti.com> wrote:
>
> Hi Simon,
>
> On 31/05/20 08:08AM, Simon Glass wrote:
> > Hi Pratyush,
> >
> > On Wed, 27 May 2020 at 06:52, Pratyush Yadav <p.yadav@ti.com> wrote:
> > >
> > > From: Jean-Jacques Hiblot <jjhiblot@ti.com>
> > >
> > > A regmap field is an abstraction available in Linux. It provides to access
> > > bitfields in a regmap without having to worry about shifts and masks.
> > >
> > > Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
> > > Reviewed-by: Simon Glass <sjg@chromium.org>
> > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
> > > ---
> > >  drivers/core/regmap.c |  78 ++++++++++++++++++++++++++++++
> > >  include/regmap.h      | 108 ++++++++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 186 insertions(+)
> > >
> >
> > Reviewed-by: Simon Glass <sjg@chromium.org>
> >
> > Please see below
> >
> > > diff --git a/drivers/core/regmap.c b/drivers/core/regmap.c
> > > index 4792067f24..cbc01b689a 100644
> > > --- a/drivers/core/regmap.c
> > > +++ b/drivers/core/regmap.c
> > > @@ -18,6 +18,15 @@
> > >  #include <linux/ioport.h>
> > >  #include <linux/compat.h>
> > >  #include <linux/err.h>
> > > +#include <linux/bitops.h>
> > > +
> > > +struct regmap_field {
> >
> > Needs comments as I'm not sure what this is for
>
> Heh, I'm that sure either. I went digging in the Linux source, and the
> only commit that touches this is the one that added regmap fields and it
> doesn't mention why we have both "regmap_field" and "reg_field".
>
> Looking at the patch, I gather that regmap_field and reg_field look at
> the field in two different ways. reg_field is how a driver would look at
> a regmap field: which register this field belongs to, and what are its
> LSB and MSB? Datasheets usually tell you this information directly.
>
> regmap_field looks at it from a lower level view: what register this
> feild belongs to, and how do I need to shift and mask the data I read
> from the bus? This is closer to how to actually read/write the field.
> The fact that regmap_field's definition is private to regmap.c also
> points in this direction.
>
> The conversion from reg_field to regmap_field is done in
> regmap_field_init().
>
> As to why have these two representations of the same thing, I can't say
> with certainty. My guess is that it makes the internal regmap code
> cleaner and slightly faster because you don't have to calculate the
> shift and mask every time.

That is good insight - please put those comments in the code.

Regards,
Simon