[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 at 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 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.

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

Regards,
Simon