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