[RFC] regmap: Add regmap_pipe_read API

[RFC] regmap: Add regmap_pipe_read API

[Crestez Dan Leonard]

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 with pipe rather than
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>
---

This works as expected for mpu6050 i2c/spi. Unfortunately looking through the
regmap_bus implementations it looks that at least regmap_spmi_base_read assumes
range semantics at the regmap_hw_read level. If any SPMI devices exist that
have registers with this kind of pipe semantics the correct way to handle them
would be reading from the same register in a loop.

I'd say the correct way to handle this would be to add a regmap_hw_pipe_read
bus operation which is almost always implemented identically to regmap_hw_read.
If that operation is missing regmap_pipe_read should attempt to read_reg in a
loop.

I'm also not absolutely certain that a separate API is required. The
regmap_raw_read currently doesn't work because it check for a
"regmap_volatile_range" which returns true only if the entire range is
volatile. It's not clear if this is intentional?

 drivers/base/regmap/regmap.c | 62 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/regmap.h       |  9 +++++++
 2 files changed, 71 insertions(+)

diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index df2d2ef..342f326 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -2408,6 +2408,68 @@ int regmap_raw_read(struct regmap *map, unsigned int reg, void *val,
 EXPORT_SYMBOL_GPL(regmap_raw_read);
 
 /**
+ * regmap_pipe_read(): Read data from a register with pipe semantics
+ *
+ * @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_pipe_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)) {
+		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_pipe_read);
+
+/**
  * regmap_field_read(): Read a value to a single register field
  *
  * @field: Register field to read from
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index 3dc08ce..18ee90e 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -719,6 +719,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_pipe_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,
@@ -955,6 +957,13 @@ static inline int regmap_raw_read(struct regmap *map, unsigned int reg,
 	return -EINVAL;
 }
 
+static inline int regmap_pipe_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)
 {
-- 
2.5.5

Re: [RFC] regmap: Add regmap_pipe_read API

[Geert Uytterhoeven]

Hi Leonard,

On Thu, Jun 16, 2016 at 5:24 PM, Crestez Dan Leonard
<leonard.crestez@intel.com> wrote:
> 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 with pipe rather than
> range semantics.

Please settle on either "fifo" or "pipe", instead of mixing both.
Personally, I prefer the former.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [RFC] regmap: Add regmap_pipe_read API

[Crestez Dan Leonard]

On 06/16/2016 06:43 PM, Geert Uytterhoeven wrote:
> Hi Leonard,
> 
> On Thu, Jun 16, 2016 at 5:24 PM, Crestez Dan Leonard
> <leonard.crestez@intel.com> wrote:
>> 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 with pipe rather than
>> range semantics.
> 
> Please settle on either "fifo" or "pipe", instead of mixing both.
> Personally, I prefer the former.

Well, it doesn't have to be a fifo. The device can return data from some
other kind of buffer (maybe a stack). I can adjust the documentation to
clarify.

I considered naming it something like regmap_multi_read_one_reg or
something but regmap_pipe_read sounds reasonable and short.

Re: [RFC] regmap: Add regmap_pipe_read API

[Lars-Peter Clausen]

On 06/17/2016 09:04 AM, Crestez Dan Leonard wrote:
> On 06/16/2016 06:43 PM, Geert Uytterhoeven wrote:
>> Hi Leonard,
>>
>> On Thu, Jun 16, 2016 at 5:24 PM, Crestez Dan Leonard
>> <leonard.crestez@intel.com> wrote:
>>> 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 with pipe rather than
>>> range semantics.
>>
>> Please settle on either "fifo" or "pipe", instead of mixing both.
>> Personally, I prefer the former.
> 
> Well, it doesn't have to be a fifo. The device can return data from some
> other kind of buffer (maybe a stack). I can adjust the documentation to
> clarify.
> 
> I considered naming it something like regmap_multi_read_one_reg or
> something but regmap_pipe_read sounds reasonable and short.

stream might be another option, but pipe is ok in my opinion.

Re: [RFC] regmap: Add regmap_pipe_read API

[Jonathan Cameron]

On 17/06/16 09:05, Lars-Peter Clausen wrote:
> On 06/17/2016 09:04 AM, Crestez Dan Leonard wrote:
>> On 06/16/2016 06:43 PM, Geert Uytterhoeven wrote:
>>> Hi Leonard,
>>>
>>> On Thu, Jun 16, 2016 at 5:24 PM, Crestez Dan Leonard
>>> <leonard.crestez@intel.com> wrote:
>>>> 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 with pipe rather than
>>>> range semantics.
>>>
>>> Please settle on either "fifo" or "pipe", instead of mixing both.
>>> Personally, I prefer the former.
>>
>> Well, it doesn't have to be a fifo. The device can return data from some
>> other kind of buffer (maybe a stack). I can adjust the documentation to
>> clarify.
>>
>> I considered naming it something like regmap_multi_read_one_reg or
>> something but regmap_pipe_read sounds reasonable and short.
> 
> stream might be another option, but pipe is ok in my opinion.
> 
I'm not fussy on naming, but definitely support having the functionality!

Jonathan

Re: [RFC] regmap: Add regmap_pipe_read API

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 218 bytes --]

On Thu, Jun 16, 2016 at 06:24:36PM +0300, Crestez Dan Leonard wrote:

> +		val = ((u8*)val) + read_len;

This cast looks broken, you should be able to do pointer arithmetic on
void pointers as though they were char *.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [RFC] regmap: Add regmap_pipe_read API

[Crestez Dan Leonard]

On 06/21/2016 09:42 PM, Mark Brown wrote:
> On Thu, Jun 16, 2016 at 06:24:36PM +0300, Crestez Dan Leonard wrote:
> 
>> +		val = ((u8*)val) + read_len;
> 
> This cast looks broken, you should be able to do pointer arithmetic on
> void pointers as though they were char *.
> 
Pointer arithmetic on void* is not standard C, it's a GCC extension. I
know that GCC extensions are allowed for the kernel but is it really
encouraged to rely on them this way?

Anyway, are my concerns about the regmap_bus implementation for SPMI
valid? In theory this could be submitted in it's present form and let
regmap+spmi users

If this API is otherwise fine I can just resend this marked as [PATCH]
with the cast removed and some rearranged comments.

-- 
Regards,
Leonard

Re: [RFC] regmap: Add regmap_pipe_read API

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 998 bytes --]

On Wed, Jun 22, 2016 at 11:32:20AM +0300, Crestez Dan Leonard wrote:
> On 06/21/2016 09:42 PM, Mark Brown wrote:
> > On Thu, Jun 16, 2016 at 06:24:36PM +0300, Crestez Dan Leonard wrote:

> >> +		val = ((u8*)val) + read_len;

> > This cast looks broken, you should be able to do pointer arithmetic on
> > void pointers as though they were char *.

> Pointer arithmetic on void* is not standard C, it's a GCC extension. I
> know that GCC extensions are allowed for the kernel but is it really
> encouraged to rely on them this way?

Hrm, I thought it had been standardized in one of the more recent spec
revisions but I can't seem to find that right now.

> Anyway, are my concerns about the regmap_bus implementation for SPMI
> valid? In theory this could be submitted in it's present form and let
> regmap+spmi users

That looks unfinished...

> If this API is otherwise fine I can just resend this marked as [PATCH]
> with the cast removed and some rearranged comments.

Well, there's the naming.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]