diff mbox series

[v4,3/6] regmap: Add regmap_noinc_read API

Message ID 1533557087-10401-4-git-send-email-stefan.popa@analog.com (mailing list archive)
State New, archived
Headers show
Series iio: accel: Add adxl372 driver | expand

Commit Message

Stefan Popa Aug. 6, 2018, 12:04 p.m. UTC 
From: Crestez Dan Leonard <leonard.crestez@intel.com>

The regmap API usually assumes that bulk read operations will read a
range of registers but some I2C/SPI devices have certain registers for
which a such a read operation will return data from an internal FIFO
instead. Add an explicit API to support bulk read without range semantics.

Some linux drivers use regmap_bulk_read or regmap_raw_read for such
registers, for example mpu6050 or bmi150 from IIO. This only happens to
work because when caching is disabled a single regmap read op will map
to a single bus read op (as desired). This breaks if caching is enabled and
reg+1 happens to be a cacheable register.

Without regmap support refactoring a driver to enable regmap caching
requires separate I2C and SPI paths. This is exactly what regmap is
supposed to help avoid.

Suggested-by: Jonathan Cameron <jic23@kernel.org>
Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com>
Signed-off-by: Stefan Popa <stefan.popa@analog.com>
---
 drivers/base/regmap/regmap.c | 65 +++++++++++++++++++++++++++++++++++++++++++-
 include/linux/regmap.h       |  9 ++++++
 2 files changed, 73 insertions(+), 1 deletion(-)

Comments

Mark Brown Aug. 6, 2018, 2:39 p.m. UTC | #1 
On Mon, Aug 06, 2018 at 03:04:44PM +0300, Stefan Popa wrote:

> +	if (!regmap_volatile(map, reg) || !regmap_readable(map, reg)) {
> +		ret = -EINVAL;
> +		goto out_unlock;
> +	}

I suggested having an explicit operation to check if a register supports
this mode of operation when I reviewed an earlier version, I didn't
notice a reply on that?
Popa, Stefan Serban Aug. 6, 2018, 3:39 p.m. UTC | #2 
On Lu, 2018-08-06 at 15:39 +0100, Mark Brown wrote:
> On Mon, Aug 06, 2018 at 03:04:44PM +0300, Stefan Popa wrote:
> 
> > 
> > +	if (!regmap_volatile(map, reg) || !regmap_readable(map, reg))
> > {
> > +		ret = -EINVAL;
> > +		goto out_unlock;
> > +	}
> I suggested having an explicit operation to check if a register supports
> this mode of operation when I reviewed an earlier version, I didn't
> notice a reply on that?

Hi Mark,

Sorry, I got confused about what needs to be done. Should I add a new field
to the regmap_config struct which will be checked during the function call?
Something like can_multi_write? How do you suggest it should be called? Is
readable_noinc_reg ok?

Thank you!
Stefan
Mark Brown Aug. 6, 2018, 4:04 p.m. UTC | #3 
On Mon, Aug 06, 2018 at 03:39:53PM +0000, Popa, Stefan Serban wrote:
> On Lu, 2018-08-06 at 15:39 +0100, Mark Brown wrote:

> > I suggested having an explicit operation to check if a register supports
> > this mode of operation when I reviewed an earlier version, I didn't
> > notice a reply on that?

> Sorry, I got confused about what needs to be done. Should I add a new field
> to the regmap_config struct which will be checked during the function call?
> Something like can_multi_write? How do you suggest it should be called? Is
> readable_noinc_reg ok?

Like I said last time I'd suggest making it more like the readable and
volatile checks - I'm fairly sure I've seen devices that don't
autoincrement only for some registers.
Charles Keepax Aug. 8, 2018, 9:59 a.m. UTC | #4 
On Mon, Aug 06, 2018 at 05:04:02PM +0100, Mark Brown wrote:
> On Mon, Aug 06, 2018 at 03:39:53PM +0000, Popa, Stefan Serban wrote:
> > On Lu, 2018-08-06 at 15:39 +0100, Mark Brown wrote:
> 
> > > I suggested having an explicit operation to check if a register supports
> > > this mode of operation when I reviewed an earlier version, I didn't
> > > notice a reply on that?
> 
> > Sorry, I got confused about what needs to be done. Should I add a new field
> > to the regmap_config struct which will be checked during the function call?
> > Something like can_multi_write? How do you suggest it should be called? Is
> > readable_noinc_reg ok?
> 
> Like I said last time I'd suggest making it more like the readable and
> volatile checks - I'm fairly sure I've seen devices that don't
> autoincrement only for some registers.

I can confirm I have seen hardware that autoincrements for some
registers and not for others.

Thanks,
Charles
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox series

Patch

diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index 3bc8488..e632503 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -2564,7 +2564,70 @@  int regmap_raw_read(struct regmap *map, unsigned int reg, void *val,
 EXPORT_SYMBOL_GPL(regmap_raw_read);
 
 /**
- * regmap_field_read() - Read a value to a single register field
+ * regmap_noinc_read(): Read data from a register without incrementing the
+ *			register number
+ *
+ * @map: Register map to read from
+ * @reg: Register to read from
+ * @val: Pointer to data buffer
+ * @val_len: Length of output buffer in bytes.
+ *
+ * The regmap API usually assumes that bulk bus read operations will read a
+ * range of registers. Some devices have certain registers for which a read
+ * operation read will read from an internal FIFO.
+ *
+ * The target register must be volatile but registers after it can be
+ * completely unrelated cacheable registers.
+ *
+ * This will attempt multiple reads as required to read val_len bytes.
+ *
+ * A value of zero will be returned on success, a negative errno will be
+ * returned in error cases.
+ */
+int regmap_noinc_read(struct regmap *map, unsigned int reg,
+		      void *val, size_t val_len)
+{
+	size_t read_len;
+	int ret;
+
+	if (!map->bus)
+		return -EINVAL;
+	if (!map->bus->read)
+		return -ENOTSUPP;
+	if (val_len % map->format.val_bytes)
+		return -EINVAL;
+	if (!IS_ALIGNED(reg, map->reg_stride))
+		return -EINVAL;
+	if (val_len == 0)
+		return -EINVAL;
+
+	map->lock(map->lock_arg);
+
+	if (!regmap_volatile(map, reg) || !regmap_readable(map, reg)) {
+		ret = -EINVAL;
+		goto out_unlock;
+	}
+
+	while (val_len) {
+		if (map->max_raw_read && map->max_raw_read < val_len)
+			read_len = map->max_raw_read;
+		else
+			read_len = val_len;
+		ret = _regmap_raw_read(map, reg, val, read_len);
+		if (ret)
+			goto out_unlock;
+		val = ((u8 *)val) + read_len;
+		val_len -= read_len;
+	}
+
+out_unlock:
+	map->unlock(map->lock_arg);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(regmap_noinc_read);
+
+/**
+ * regmap_field_read(): Read a value to a single register field
  *
  * @field: Register field to read from
  * @val: Pointer to store read value
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index 4f38068..ddaa5e6 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -946,6 +946,8 @@  int regmap_raw_write_async(struct regmap *map, unsigned int reg,
 int regmap_read(struct regmap *map, unsigned int reg, unsigned int *val);
 int regmap_raw_read(struct regmap *map, unsigned int reg,
 		    void *val, size_t val_len);
+int regmap_noinc_read(struct regmap *map, unsigned int reg,
+		      void *val, size_t val_len);
 int regmap_bulk_read(struct regmap *map, unsigned int reg, void *val,
 		     size_t val_count);
 int regmap_update_bits_base(struct regmap *map, unsigned int reg,
@@ -1196,6 +1198,13 @@  static inline int regmap_raw_read(struct regmap *map, unsigned int reg,
 	return -EINVAL;
 }
 
+static inline int regmap_noinc_read(struct regmap *map, unsigned int reg,
+				    void *val, size_t val_len)
+{
+	WARN_ONCE(1, "regmap API is disabled");
+	return -EINVAL;
+}
+
 static inline int regmap_bulk_read(struct regmap *map, unsigned int reg,
 				   void *val, size_t val_count)
 {