| Message ID | 7ca3857aa8869a1e1f4709860f57f7d92abf1c6b.1466089603.git.leonard.crestez@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 *.
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.
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.
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) {
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(+)