Re: [RFC] regmap: Add regmap_field APIs

2013-06-10 Thread Srinivas KANDAGATLA 
On 10/06/13 10:15, Mark Brown wrote:
> On Sun, Jun 09, 2013 at 06:00:19PM +0200, Lars-Peter Clausen wrote:
> 
>>> +int regmap_field_write(struct regmap_field *field, unsigned int val)
>>> +{
>>> +   int field_bits;
>>> +   unsigned int reg_mask;
>>> +   field_bits = field->msb - field->lsb + 1;
>>> +   reg_mask = ((BIT(field_bits) - 1) << field->lsb);
>>> +   return regmap_update_bits(field->regmap, field->reg,
>>> +   reg_mask, val << field->lsb);
> 
>> Considering that you'd do the same calculations over and over again it would
>> probably make more sense store the mask rather than the msb in the struct
> 
> However as an interface for registering either is OK - the current
> MSB/LSB approach is probably better as that's what datasheets tend to
> include (which is why I didn't say anything).

regmap field interface still will be of lsb/msb style, However, As Lars
said, the internal data structure which holds these info can have mask
field rather than storing lsb/msb info.
I just posted a V2 patch with your review comments. I did not read your
response before I hit the send button.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC] regmap: Add regmap_field APIs

2013-06-10 Thread Mark Brown 
On Sun, Jun 09, 2013 at 06:00:19PM +0200, Lars-Peter Clausen wrote:

> > +int regmap_field_write(struct regmap_field *field, unsigned int val)
> > +{
> > +   int field_bits;
> > +   unsigned int reg_mask;
> > +   field_bits = field->msb - field->lsb + 1;
> > +   reg_mask = ((BIT(field_bits) - 1) << field->lsb);
> > +   return regmap_update_bits(field->regmap, field->reg,
> > +   reg_mask, val << field->lsb);

> Considering that you'd do the same calculations over and over again it would
> probably make more sense store the mask rather than the msb in the struct

However as an interface for registering either is OK - the current
MSB/LSB approach is probably better as that's what datasheets tend to
include (which is why I didn't say anything).


signature.asc
Description: Digital signature

Re: [RFC] regmap: Add regmap_field APIs

2013-06-10 Thread Srinivas KANDAGATLA 
On 09/06/13 17:00, Lars-Peter Clausen wrote:
> [...]
>> +int regmap_field_write(struct regmap_field *field, unsigned int val)
>> +{
>> +int field_bits;
>> +unsigned int reg_mask;
>> +field_bits = field->msb - field->lsb + 1;
>> +reg_mask = ((BIT(field_bits) - 1) << field->lsb);
>> +return regmap_update_bits(field->regmap, field->reg,
>> +reg_mask, val << field->lsb);
> 
> Considering that you'd do the same calculations over and over again it would
> probably make more sense store the mask rather than the msb in the struct
It makes sense, I will do that change.

Thanks,
srini
> 
>> +}
>> +EXPORT_SYMBOL_GPL(regmap_field_write);

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC] regmap: Add regmap_field APIs

2013-06-10 Thread Srinivas KANDAGATLA 
On 09/06/13 17:00, Lars-Peter Clausen wrote:
 [...]
 +int regmap_field_write(struct regmap_field *field, unsigned int val)
 +{
 +int field_bits;
 +unsigned int reg_mask;
 +field_bits = field-msb - field-lsb + 1;
 +reg_mask = ((BIT(field_bits) - 1)  field-lsb);
 +return regmap_update_bits(field-regmap, field-reg,
 +reg_mask, val  field-lsb);
 
 Considering that you'd do the same calculations over and over again it would
 probably make more sense store the mask rather than the msb in the struct
It makes sense, I will do that change.

Thanks,
srini
 
 +}
 +EXPORT_SYMBOL_GPL(regmap_field_write);

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC] regmap: Add regmap_field APIs

2013-06-10 Thread Mark Brown 
On Sun, Jun 09, 2013 at 06:00:19PM +0200, Lars-Peter Clausen wrote:

  +int regmap_field_write(struct regmap_field *field, unsigned int val)
  +{
  +   int field_bits;
  +   unsigned int reg_mask;
  +   field_bits = field-msb - field-lsb + 1;
  +   reg_mask = ((BIT(field_bits) - 1)  field-lsb);
  +   return regmap_update_bits(field-regmap, field-reg,
  +   reg_mask, val  field-lsb);

 Considering that you'd do the same calculations over and over again it would
 probably make more sense store the mask rather than the msb in the struct

However as an interface for registering either is OK - the current
MSB/LSB approach is probably better as that's what datasheets tend to
include (which is why I didn't say anything).


signature.asc
Description: Digital signature

Re: [RFC] regmap: Add regmap_field APIs

2013-06-10 Thread Srinivas KANDAGATLA 
On 10/06/13 10:15, Mark Brown wrote:
 On Sun, Jun 09, 2013 at 06:00:19PM +0200, Lars-Peter Clausen wrote:
 
 +int regmap_field_write(struct regmap_field *field, unsigned int val)
 +{
 +   int field_bits;
 +   unsigned int reg_mask;
 +   field_bits = field-msb - field-lsb + 1;
 +   reg_mask = ((BIT(field_bits) - 1)  field-lsb);
 +   return regmap_update_bits(field-regmap, field-reg,
 +   reg_mask, val  field-lsb);
 
 Considering that you'd do the same calculations over and over again it would
 probably make more sense store the mask rather than the msb in the struct
 
 However as an interface for registering either is OK - the current
 MSB/LSB approach is probably better as that's what datasheets tend to
 include (which is why I didn't say anything).

regmap field interface still will be of lsb/msb style, However, As Lars
said, the internal data structure which holds these info can have mask
field rather than storing lsb/msb info.
I just posted a V2 patch with your review comments. I did not read your
response before I hit the send button.

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC] regmap: Add regmap_field APIs

2013-06-09 Thread Lars-Peter Clausen 
[...]
> +int regmap_field_write(struct regmap_field *field, unsigned int val)
> +{
> + int field_bits;
> + unsigned int reg_mask;
> + field_bits = field->msb - field->lsb + 1;
> + reg_mask = ((BIT(field_bits) - 1) << field->lsb);
> + return regmap_update_bits(field->regmap, field->reg,
> + reg_mask, val << field->lsb);

Considering that you'd do the same calculations over and over again it would
probably make more sense store the mask rather than the msb in the struct

> +}
> +EXPORT_SYMBOL_GPL(regmap_field_write);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC] regmap: Add regmap_field APIs

2013-06-09 Thread Lars-Peter Clausen 
[...]
 +int regmap_field_write(struct regmap_field *field, unsigned int val)
 +{
 + int field_bits;
 + unsigned int reg_mask;
 + field_bits = field-msb - field-lsb + 1;
 + reg_mask = ((BIT(field_bits) - 1)  field-lsb);
 + return regmap_update_bits(field-regmap, field-reg,
 + reg_mask, val  field-lsb);

Considering that you'd do the same calculations over and over again it would
probably make more sense store the mask rather than the msb in the struct

 +}
 +EXPORT_SYMBOL_GPL(regmap_field_write);
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC] regmap: Add regmap_field APIs

2013-06-05 Thread Mark Brown 
On Wed, Jun 05, 2013 at 03:41:29PM +0100, Srinivas KANDAGATLA wrote:

> Is it Ok If I send this patch with my "STiH41x SOC support series", as
> we can see the actual usage of this new apis in the follow on patches?

Yes.


signature.asc
Description: Digital signature

Re: [RFC] regmap: Add regmap_field APIs

2013-06-05 Thread Srinivas KANDAGATLA 
Thankyou for reviewing the patch.
On 05/06/13 12:41, Mark Brown wrote:
> On Wed, Jun 05, 2013 at 10:21:23AM +0100, Srinivas KANDAGATLA wrote:
> I think I'd prefer to see a struct passed in here for the field
> definition - this would make it easier to initialise from static data,
> otherwise people will end up writing a loop that reads from a locally
> defined struct once they get more than a couple of fields.
> 
I will modify the APIs to take reg_field structure instead of reg, lsb,
and msb arguments.

Is it Ok If I send this patch with my "STiH41x SOC support series", as
we can see the actual usage of this new apis in the follow on patches?

Thanks,
srini
> Otherwise this makes sense.
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC] regmap: Add regmap_field APIs

2013-06-05 Thread Mark Brown 
On Wed, Jun 05, 2013 at 10:21:23AM +0100, Srinivas KANDAGATLA wrote:

> Is it ok if we rename the regmap_field_init function to
> regmap_field_alloc, as it will make it obvious that its allocating
> memory which should be freed?
> I also thought we could add devm version of it as well.

Yes, that's all sensible.

> With this change here is what the init/alloc function would look like:

> static void _regmap_field_init(struct regmap_field *field,
>   struct regmap *regmap, unsigned int reg,
>   unsigned int lsb, unsigned int msb)

> struct regmap_field *devm_regmap_field_alloc(struct device *dev,
>   struct regmap *regmap, unsigned int reg,
>   unsigned int lsb, unsigned int msb)

I think I'd prefer to see a struct passed in here for the field
definition - this would make it easier to initialise from static data,
otherwise people will end up writing a loop that reads from a locally
defined struct once they get more than a couple of fields.

Otherwise this makes sense.


signature.asc
Description: Digital signature

Re: [RFC] regmap: Add regmap_field APIs

2013-06-05 Thread Srinivas KANDAGATLA 
On 04/06/13 22:01, Mark Brown wrote:
> On Tue, May 28, 2013 at 03:58:00PM +0100, Srinivas KANDAGATLA wrote:
> 
>> +#define REGMAP_FIELD_INIT(regmap, reg, lsb, msb) {  \
>> +.regmap = regmap,   \
>> +.reg = reg, \
>> +.lsb = lsb, \
>> +.msb = msb, \
>> +}
> 
> Having a macro for this is really odd since macros are generally only
> used at compile time but the regmap is only available at runtime and
> this...
> 
Yes, I think the macro is bit over do.. I will remove it.

>> +static inline void regmap_field_init(struct regmap_field *field,
>> +struct regmap *regmap,  unsigned int reg,
>> +unsigned int lsb, unsigned int msb)
>> +{
>> +field->regmap = regmap;
>> +field->reg = reg;
>> +field->lsb = lsb;
>> +field->msb = msb;
>> +}
> 
> ...is a bit awkward since you can't use it with static data.  I think
> either the read/write/modify APIs should be changed to take both the map
> and the field as arguments (with the field only containing the bitfield
> definitions) or the init function should be something that allocates a
> new, runtime only structure from static data.
I agree with you and I think init function should allocate and
initialize the regmap_field and return it, and the read/write apis will
take the field and value. This approach looks neat.

Is it ok if we rename the regmap_field_init function to
regmap_field_alloc, as it will make it obvious that its allocating
memory which should be freed?
I also thought we could add devm version of it as well.

With this change here is what the init/alloc function would look like:

static void _regmap_field_init(struct regmap_field *field,
struct regmap *regmap, unsigned int reg,
unsigned int lsb, unsigned int msb)
{
field->regmap = regmap;
field->reg = reg;
field->lsb = lsb;
field->msb = msb;
}

struct regmap_field *devm_regmap_field_alloc(struct device *dev,
struct regmap *regmap, unsigned int reg,
unsigned int lsb, unsigned int msb)
{
struct regmap_field *field = devm_kzalloc(dev,
sizeof(*field), GFP_KERNEL);
if (!field)
return ERR_PTR(-ENOMEM);

_regmap_field_init(field, regmap, reg, lsb, msb);
return field;

}
EXPORT_SYMBOL_GPL(devm_regmap_field_alloc);

struct regmap_field *regmap_field_alloc(struct regmap *regmap,
unsigned int reg, unsigned int lsb, unsigned int msb)
{
struct regmap_field *field = kzalloc(sizeof(*field), GFP_KERNEL);
if (!field)
return ERR_PTR(-ENOMEM);

_regmap_field_init(field, regmap, reg, lsb, msb);
return field;
}
EXPORT_SYMBOL_GPL(regmap_field_alloc);

In header file..
static void inline regmap_field_free(struct regmap_field *field)
{
kfree(field);
}

static void inline devm_regmap_field_free(struct device *dev, struct
regmap_field *field)
{
devm_kfree(dev, field);
}

Thanks,
srini
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC] regmap: Add regmap_field APIs

2013-06-05 Thread Srinivas KANDAGATLA 
On 04/06/13 22:01, Mark Brown wrote:
 On Tue, May 28, 2013 at 03:58:00PM +0100, Srinivas KANDAGATLA wrote:
 
 +#define REGMAP_FIELD_INIT(regmap, reg, lsb, msb) {  \
 +.regmap = regmap,   \
 +.reg = reg, \
 +.lsb = lsb, \
 +.msb = msb, \
 +}
 
 Having a macro for this is really odd since macros are generally only
 used at compile time but the regmap is only available at runtime and
 this...
 
Yes, I think the macro is bit over do.. I will remove it.

 +static inline void regmap_field_init(struct regmap_field *field,
 +struct regmap *regmap,  unsigned int reg,
 +unsigned int lsb, unsigned int msb)
 +{
 +field-regmap = regmap;
 +field-reg = reg;
 +field-lsb = lsb;
 +field-msb = msb;
 +}
 
 ...is a bit awkward since you can't use it with static data.  I think
 either the read/write/modify APIs should be changed to take both the map
 and the field as arguments (with the field only containing the bitfield
 definitions) or the init function should be something that allocates a
 new, runtime only structure from static data.
I agree with you and I think init function should allocate and
initialize the regmap_field and return it, and the read/write apis will
take the field and value. This approach looks neat.

Is it ok if we rename the regmap_field_init function to
regmap_field_alloc, as it will make it obvious that its allocating
memory which should be freed?
I also thought we could add devm version of it as well.

With this change here is what the init/alloc function would look like:

static void _regmap_field_init(struct regmap_field *field,
struct regmap *regmap, unsigned int reg,
unsigned int lsb, unsigned int msb)
{
field-regmap = regmap;
field-reg = reg;
field-lsb = lsb;
field-msb = msb;
}

struct regmap_field *devm_regmap_field_alloc(struct device *dev,
struct regmap *regmap, unsigned int reg,
unsigned int lsb, unsigned int msb)
{
struct regmap_field *field = devm_kzalloc(dev,
sizeof(*field), GFP_KERNEL);
if (!field)
return ERR_PTR(-ENOMEM);

_regmap_field_init(field, regmap, reg, lsb, msb);
return field;

}
EXPORT_SYMBOL_GPL(devm_regmap_field_alloc);

struct regmap_field *regmap_field_alloc(struct regmap *regmap,
unsigned int reg, unsigned int lsb, unsigned int msb)
{
struct regmap_field *field = kzalloc(sizeof(*field), GFP_KERNEL);
if (!field)
return ERR_PTR(-ENOMEM);

_regmap_field_init(field, regmap, reg, lsb, msb);
return field;
}
EXPORT_SYMBOL_GPL(regmap_field_alloc);

In header file..
static void inline regmap_field_free(struct regmap_field *field)
{
kfree(field);
}

static void inline devm_regmap_field_free(struct device *dev, struct
regmap_field *field)
{
devm_kfree(dev, field);
}

Thanks,
srini
 

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC] regmap: Add regmap_field APIs

2013-06-05 Thread Mark Brown 
On Wed, Jun 05, 2013 at 10:21:23AM +0100, Srinivas KANDAGATLA wrote:

 Is it ok if we rename the regmap_field_init function to
 regmap_field_alloc, as it will make it obvious that its allocating
 memory which should be freed?
 I also thought we could add devm version of it as well.

Yes, that's all sensible.

 With this change here is what the init/alloc function would look like:

 static void _regmap_field_init(struct regmap_field *field,
   struct regmap *regmap, unsigned int reg,
   unsigned int lsb, unsigned int msb)

 struct regmap_field *devm_regmap_field_alloc(struct device *dev,
   struct regmap *regmap, unsigned int reg,
   unsigned int lsb, unsigned int msb)

I think I'd prefer to see a struct passed in here for the field
definition - this would make it easier to initialise from static data,
otherwise people will end up writing a loop that reads from a locally
defined struct once they get more than a couple of fields.

Otherwise this makes sense.


signature.asc
Description: Digital signature

Re: [RFC] regmap: Add regmap_field APIs

2013-06-05 Thread Srinivas KANDAGATLA 
Thankyou for reviewing the patch.
On 05/06/13 12:41, Mark Brown wrote:
 On Wed, Jun 05, 2013 at 10:21:23AM +0100, Srinivas KANDAGATLA wrote:
 I think I'd prefer to see a struct passed in here for the field
 definition - this would make it easier to initialise from static data,
 otherwise people will end up writing a loop that reads from a locally
 defined struct once they get more than a couple of fields.
 
I will modify the APIs to take reg_field structure instead of reg, lsb,
and msb arguments.

Is it Ok If I send this patch with my STiH41x SOC support series, as
we can see the actual usage of this new apis in the follow on patches?

Thanks,
srini
 Otherwise this makes sense.
 

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC] regmap: Add regmap_field APIs

2013-06-05 Thread Mark Brown 
On Wed, Jun 05, 2013 at 03:41:29PM +0100, Srinivas KANDAGATLA wrote:

 Is it Ok If I send this patch with my STiH41x SOC support series, as
 we can see the actual usage of this new apis in the follow on patches?

Yes.


signature.asc
Description: Digital signature

Re: [RFC] regmap: Add regmap_field APIs

2013-06-04 Thread Mark Brown 
On Tue, May 28, 2013 at 03:58:00PM +0100, Srinivas KANDAGATLA wrote:

> +#define REGMAP_FIELD_INIT(regmap, reg, lsb, msb) {   \
> + .regmap = regmap,   \
> + .reg = reg, \
> + .lsb = lsb, \
> + .msb = msb, \
> + }

Having a macro for this is really odd since macros are generally only
used at compile time but the regmap is only available at runtime and
this...

> +static inline void regmap_field_init(struct regmap_field *field,
> + struct regmap *regmap,  unsigned int reg,
> + unsigned int lsb, unsigned int msb)
> +{
> + field->regmap = regmap;
> + field->reg = reg;
> + field->lsb = lsb;
> + field->msb = msb;
> +}

...is a bit awkward since you can't use it with static data.  I think
either the read/write/modify APIs should be changed to take both the map
and the field as arguments (with the field only containing the bitfield
definitions) or the init function should be something that allocates a
new, runtime only structure from static data.


signature.asc
Description: Digital signature

Re: [RFC] regmap: Add regmap_field APIs

2013-06-04 Thread Mark Brown 
On Tue, May 28, 2013 at 03:58:00PM +0100, Srinivas KANDAGATLA wrote:

 +#define REGMAP_FIELD_INIT(regmap, reg, lsb, msb) {   \
 + .regmap = regmap,   \
 + .reg = reg, \
 + .lsb = lsb, \
 + .msb = msb, \
 + }

Having a macro for this is really odd since macros are generally only
used at compile time but the regmap is only available at runtime and
this...

 +static inline void regmap_field_init(struct regmap_field *field,
 + struct regmap *regmap,  unsigned int reg,
 + unsigned int lsb, unsigned int msb)
 +{
 + field-regmap = regmap;
 + field-reg = reg;
 + field-lsb = lsb;
 + field-msb = msb;
 +}

...is a bit awkward since you can't use it with static data.  I think
either the read/write/modify APIs should be changed to take both the map
and the field as arguments (with the field only containing the bitfield
definitions) or the init function should be something that allocates a
new, runtime only structure from static data.


signature.asc
Description: Digital signature

Re: [RFC] regmap: Add regmap_field APIs

2013-06-01 Thread Mark Brown 
On Fri, May 31, 2013 at 07:31:48AM +0100, Srinivas KANDAGATLA wrote:

> We have pretty much completed reworking the patch-set we sent recently
> for the STiH41x SOC support. We are waiting for your feedback on this patch.

Don't top post and don't send contentless pings; you should generally
allow a reasonable length of time for replies.  Nagging like this often
just makes things take longer, if only because it makes people remember
that they did something with the message.


signature.asc
Description: Digital signature

Re: [RFC] regmap: Add regmap_field APIs

2013-06-01 Thread Mark Brown 
On Fri, May 31, 2013 at 07:31:48AM +0100, Srinivas KANDAGATLA wrote:

 We have pretty much completed reworking the patch-set we sent recently
 for the STiH41x SOC support. We are waiting for your feedback on this patch.

Don't top post and don't send contentless pings; you should generally
allow a reasonable length of time for replies.  Nagging like this often
just makes things take longer, if only because it makes people remember
that they did something with the message.


signature.asc
Description: Digital signature

Re: [RFC] regmap: Add regmap_field APIs

2013-05-31 Thread Srinivas KANDAGATLA 
Hi Mark,

We have pretty much completed reworking the patch-set we sent recently
for the STiH41x SOC support. We are waiting for your feedback on this patch.

Thanks,
srini

On 28/05/13 15:58, Srinivas KANDAGATLA wrote:
> From: Srinivas Kandagatla 
> 
> It is common to access regmap registers at bit level, using
> regmap_update_bits or regmap_read functions, however the end user has to
> take care of a mask or shifting. This becomes overhead when such use
> cases are high. Having a common function to do this is much convient and less
> error prone.
> 
> The idea of regmap_field is simple, regmap_field gives a logical structure to
> bits of the regmap register, and the driver can use this logical entity 
> without
> the knowledge of the bit postions and masks all over the code. This way code
> looks much neat and it need not handle the masks, shifts every time it access
> the those entities.
> 
> With this new regmap_field_read/write apis the end user can setup a
> regmap field using regmap_field_init and use the return regmap_field to
> read write the register field without worrying about the masks or
> shifts.
> Also this apis will be usefull for drivers which are based on regmaps,
> like some clocks or pinctrls which can work on the regmap_fields
> directly without having to worry about bit positions.
> 
> Signed-off-by: Srinivas Kandagatla 
> ---
> Hi Mark,
> 
> I have been looking at using regmap mmio directly for ST "System Configuration
> registers". One thing which we discussed 2 weeks back in syscon patch was, if
> this new functionality would benifit others. Having thought about it, I think
> that if regmap had a concept like regmap_field we would have used it straight 
> way
> without a new driver.
> 
> I generated this patch mainly to get your opinion on these new APIs, Is this 
> the
> right place for these APIs, or do you suggest that these APIs should go in SOC
> Specific driver?
> 
> Comments?
> 
> Thanks,
> srini
> 
> 
> 
>  drivers/base/regmap/regmap.c |   47 
> ++
>  include/linux/regmap.h   |   28 +
>  2 files changed, 75 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
> index a941dcf..4512df7 100644
> --- a/drivers/base/regmap/regmap.c
> +++ b/drivers/base/regmap/regmap.c
> @@ -1249,6 +1249,27 @@ int regmap_raw_write(struct regmap *map, unsigned int 
> reg,
>  }
>  EXPORT_SYMBOL_GPL(regmap_raw_write);
>  
> +/**
> + * regmap_field_write(): Write a value to a single register field
> + *
> + * @field: Register field to write to
> + * @val: Value to be written
> + *
> + * A value of zero will be returned on success, a negative errno will
> + * be returned in error cases.
> + */
> +
> +int regmap_field_write(struct regmap_field *field, unsigned int val)
> +{
> + int field_bits;
> + unsigned int reg_mask;
> + field_bits = field->msb - field->lsb + 1;
> + reg_mask = ((BIT(field_bits) - 1) << field->lsb);
> + return regmap_update_bits(field->regmap, field->reg,
> + reg_mask, val << field->lsb);
> +}
> +EXPORT_SYMBOL_GPL(regmap_field_write);
> +
>  /*
>   * regmap_bulk_write(): Write multiple registers to the device
>   *
> @@ -1532,6 +1553,32 @@ 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
> + *
> + * @field: Register field to read from
> + * @val: Pointer to store read value
> + *
> + * A value of zero will be returned on success, a negative errno will
> + * be returned in error cases.
> + */
> +
> +int regmap_field_read(struct regmap_field *field, unsigned int *val)
> +{
> + int field_bits;
> + int ret;
> + ret = regmap_read(field->regmap, field->reg, val);
> + if (ret != 0)
> + return ret;
> +
> + field_bits = field->msb - field->lsb + 1;
> + *val >>= field->lsb;
> + *val &= (BIT(field_bits) - 1);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(regmap_field_read);
> +
> +/**
>   * regmap_bulk_read(): Read multiple registers from the device
>   *
>   * @map: Register map to write to
> diff --git a/include/linux/regmap.h b/include/linux/regmap.h
> index 02d84e2..f8dba11 100644
> --- a/include/linux/regmap.h
> +++ b/include/linux/regmap.h
> @@ -411,6 +411,34 @@ bool regmap_reg_in_ranges(unsigned int reg,
> const struct regmap_range *ranges,
> unsigned int nranges);
>  
> +struct regmap_field {
> + struct regmap *regmap;
> + unsigned int reg;
> + unsigned int lsb;
> + unsigned int msb;
> +};
> +
> +#define REGMAP_FIELD_INIT(regmap, reg, lsb, msb) {   \
> + .regmap = regmap,   \
> + .reg = reg, \
> + .lsb = lsb, \
> + .msb = msb,

Re: [RFC] regmap: Add regmap_field APIs

2013-05-31 Thread Srinivas KANDAGATLA 
Hi Mark,

We have pretty much completed reworking the patch-set we sent recently
for the STiH41x SOC support. We are waiting for your feedback on this patch.

Thanks,
srini

On 28/05/13 15:58, Srinivas KANDAGATLA wrote:
 From: Srinivas Kandagatla srinivas.kandaga...@st.com
 
 It is common to access regmap registers at bit level, using
 regmap_update_bits or regmap_read functions, however the end user has to
 take care of a mask or shifting. This becomes overhead when such use
 cases are high. Having a common function to do this is much convient and less
 error prone.
 
 The idea of regmap_field is simple, regmap_field gives a logical structure to
 bits of the regmap register, and the driver can use this logical entity 
 without
 the knowledge of the bit postions and masks all over the code. This way code
 looks much neat and it need not handle the masks, shifts every time it access
 the those entities.
 
 With this new regmap_field_read/write apis the end user can setup a
 regmap field using regmap_field_init and use the return regmap_field to
 read write the register field without worrying about the masks or
 shifts.
 Also this apis will be usefull for drivers which are based on regmaps,
 like some clocks or pinctrls which can work on the regmap_fields
 directly without having to worry about bit positions.
 
 Signed-off-by: Srinivas Kandagatla srinivas.kandaga...@st.com
 ---
 Hi Mark,
 
 I have been looking at using regmap mmio directly for ST System Configuration
 registers. One thing which we discussed 2 weeks back in syscon patch was, if
 this new functionality would benifit others. Having thought about it, I think
 that if regmap had a concept like regmap_field we would have used it straight 
 way
 without a new driver.
 
 I generated this patch mainly to get your opinion on these new APIs, Is this 
 the
 right place for these APIs, or do you suggest that these APIs should go in SOC
 Specific driver?
 
 Comments?
 
 Thanks,
 srini
 
 
 
  drivers/base/regmap/regmap.c |   47 
 ++
  include/linux/regmap.h   |   28 +
  2 files changed, 75 insertions(+), 0 deletions(-)
 
 diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
 index a941dcf..4512df7 100644
 --- a/drivers/base/regmap/regmap.c
 +++ b/drivers/base/regmap/regmap.c
 @@ -1249,6 +1249,27 @@ int regmap_raw_write(struct regmap *map, unsigned int 
 reg,
  }
  EXPORT_SYMBOL_GPL(regmap_raw_write);
  
 +/**
 + * regmap_field_write(): Write a value to a single register field
 + *
 + * @field: Register field to write to
 + * @val: Value to be written
 + *
 + * A value of zero will be returned on success, a negative errno will
 + * be returned in error cases.
 + */
 +
 +int regmap_field_write(struct regmap_field *field, unsigned int val)
 +{
 + int field_bits;
 + unsigned int reg_mask;
 + field_bits = field-msb - field-lsb + 1;
 + reg_mask = ((BIT(field_bits) - 1)  field-lsb);
 + return regmap_update_bits(field-regmap, field-reg,
 + reg_mask, val  field-lsb);
 +}
 +EXPORT_SYMBOL_GPL(regmap_field_write);
 +
  /*
   * regmap_bulk_write(): Write multiple registers to the device
   *
 @@ -1532,6 +1553,32 @@ 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
 + *
 + * @field: Register field to read from
 + * @val: Pointer to store read value
 + *
 + * A value of zero will be returned on success, a negative errno will
 + * be returned in error cases.
 + */
 +
 +int regmap_field_read(struct regmap_field *field, unsigned int *val)
 +{
 + int field_bits;
 + int ret;
 + ret = regmap_read(field-regmap, field-reg, val);
 + if (ret != 0)
 + return ret;
 +
 + field_bits = field-msb - field-lsb + 1;
 + *val = field-lsb;
 + *val = (BIT(field_bits) - 1);
 +
 + return ret;
 +}
 +EXPORT_SYMBOL_GPL(regmap_field_read);
 +
 +/**
   * regmap_bulk_read(): Read multiple registers from the device
   *
   * @map: Register map to write to
 diff --git a/include/linux/regmap.h b/include/linux/regmap.h
 index 02d84e2..f8dba11 100644
 --- a/include/linux/regmap.h
 +++ b/include/linux/regmap.h
 @@ -411,6 +411,34 @@ bool regmap_reg_in_ranges(unsigned int reg,
 const struct regmap_range *ranges,
 unsigned int nranges);
  
 +struct regmap_field {
 + struct regmap *regmap;
 + unsigned int reg;
 + unsigned int lsb;
 + unsigned int msb;
 +};
 +
 +#define REGMAP_FIELD_INIT(regmap, reg, lsb, msb) {   \
 + .regmap = regmap,   \
 + .reg = reg, \
 + .lsb = lsb, \
 + .msb = msb, \
 + }
 +
 +/* dynamic version of regmap_field intialization */
 +static inline

[RFC] regmap: Add regmap_field APIs

2013-05-28 Thread Srinivas KANDAGATLA 
From: Srinivas Kandagatla 

It is common to access regmap registers at bit level, using
regmap_update_bits or regmap_read functions, however the end user has to
take care of a mask or shifting. This becomes overhead when such use
cases are high. Having a common function to do this is much convient and less
error prone.

The idea of regmap_field is simple, regmap_field gives a logical structure to
bits of the regmap register, and the driver can use this logical entity without
the knowledge of the bit postions and masks all over the code. This way code
looks much neat and it need not handle the masks, shifts every time it access
the those entities.

With this new regmap_field_read/write apis the end user can setup a
regmap field using regmap_field_init and use the return regmap_field to
read write the register field without worrying about the masks or
shifts.
Also this apis will be usefull for drivers which are based on regmaps,
like some clocks or pinctrls which can work on the regmap_fields
directly without having to worry about bit positions.

Signed-off-by: Srinivas Kandagatla 
---
Hi Mark,

I have been looking at using regmap mmio directly for ST "System Configuration
registers". One thing which we discussed 2 weeks back in syscon patch was, if
this new functionality would benifit others. Having thought about it, I think
that if regmap had a concept like regmap_field we would have used it straight 
way
without a new driver.

I generated this patch mainly to get your opinion on these new APIs, Is this the
right place for these APIs, or do you suggest that these APIs should go in SOC
Specific driver?

Comments?

Thanks,
srini



 drivers/base/regmap/regmap.c |   47 ++
 include/linux/regmap.h   |   28 +
 2 files changed, 75 insertions(+), 0 deletions(-)

diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index a941dcf..4512df7 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -1249,6 +1249,27 @@ int regmap_raw_write(struct regmap *map, unsigned int 
reg,
 }
 EXPORT_SYMBOL_GPL(regmap_raw_write);
 
+/**
+ * regmap_field_write(): Write a value to a single register field
+ *
+ * @field: Register field to write to
+ * @val: Value to be written
+ *
+ * A value of zero will be returned on success, a negative errno will
+ * be returned in error cases.
+ */
+
+int regmap_field_write(struct regmap_field *field, unsigned int val)
+{
+   int field_bits;
+   unsigned int reg_mask;
+   field_bits = field->msb - field->lsb + 1;
+   reg_mask = ((BIT(field_bits) - 1) << field->lsb);
+   return regmap_update_bits(field->regmap, field->reg,
+   reg_mask, val << field->lsb);
+}
+EXPORT_SYMBOL_GPL(regmap_field_write);
+
 /*
  * regmap_bulk_write(): Write multiple registers to the device
  *
@@ -1532,6 +1553,32 @@ 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
+ *
+ * @field: Register field to read from
+ * @val: Pointer to store read value
+ *
+ * A value of zero will be returned on success, a negative errno will
+ * be returned in error cases.
+ */
+
+int regmap_field_read(struct regmap_field *field, unsigned int *val)
+{
+   int field_bits;
+   int ret;
+   ret = regmap_read(field->regmap, field->reg, val);
+   if (ret != 0)
+   return ret;
+
+   field_bits = field->msb - field->lsb + 1;
+   *val >>= field->lsb;
+   *val &= (BIT(field_bits) - 1);
+
+   return ret;
+}
+EXPORT_SYMBOL_GPL(regmap_field_read);
+
+/**
  * regmap_bulk_read(): Read multiple registers from the device
  *
  * @map: Register map to write to
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index 02d84e2..f8dba11 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -411,6 +411,34 @@ bool regmap_reg_in_ranges(unsigned int reg,
  const struct regmap_range *ranges,
  unsigned int nranges);
 
+struct regmap_field {
+   struct regmap *regmap;
+   unsigned int reg;
+   unsigned int lsb;
+   unsigned int msb;
+};
+
+#define REGMAP_FIELD_INIT(regmap, reg, lsb, msb) { \
+   .regmap = regmap,   \
+   .reg = reg, \
+   .lsb = lsb, \
+   .msb = msb, \
+   }
+
+/* dynamic version of regmap_field intialization */
+static inline void regmap_field_init(struct regmap_field *field,
+   struct regmap *regmap,  unsigned int reg,
+   unsigned int lsb, unsigned int msb)
+{
+   field->regmap = regmap;
+   field->reg = reg;
+   field->lsb = lsb;
+   field->msb = msb;
+}
+
+int regmap_field_read(struct regmap_field *field, unsigned int *val);
+int

[RFC] regmap: Add regmap_field APIs

2013-05-28 Thread Srinivas KANDAGATLA 
From: Srinivas Kandagatla srinivas.kandaga...@st.com

It is common to access regmap registers at bit level, using
regmap_update_bits or regmap_read functions, however the end user has to
take care of a mask or shifting. This becomes overhead when such use
cases are high. Having a common function to do this is much convient and less
error prone.

The idea of regmap_field is simple, regmap_field gives a logical structure to
bits of the regmap register, and the driver can use this logical entity without
the knowledge of the bit postions and masks all over the code. This way code
looks much neat and it need not handle the masks, shifts every time it access
the those entities.

With this new regmap_field_read/write apis the end user can setup a
regmap field using regmap_field_init and use the return regmap_field to
read write the register field without worrying about the masks or
shifts.
Also this apis will be usefull for drivers which are based on regmaps,
like some clocks or pinctrls which can work on the regmap_fields
directly without having to worry about bit positions.

Signed-off-by: Srinivas Kandagatla srinivas.kandaga...@st.com
---
Hi Mark,

I have been looking at using regmap mmio directly for ST System Configuration
registers. One thing which we discussed 2 weeks back in syscon patch was, if
this new functionality would benifit others. Having thought about it, I think
that if regmap had a concept like regmap_field we would have used it straight 
way
without a new driver.

I generated this patch mainly to get your opinion on these new APIs, Is this the
right place for these APIs, or do you suggest that these APIs should go in SOC
Specific driver?

Comments?

Thanks,
srini



 drivers/base/regmap/regmap.c |   47 ++
 include/linux/regmap.h   |   28 +
 2 files changed, 75 insertions(+), 0 deletions(-)

diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index a941dcf..4512df7 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -1249,6 +1249,27 @@ int regmap_raw_write(struct regmap *map, unsigned int 
reg,
 }
 EXPORT_SYMBOL_GPL(regmap_raw_write);
 
+/**
+ * regmap_field_write(): Write a value to a single register field
+ *
+ * @field: Register field to write to
+ * @val: Value to be written
+ *
+ * A value of zero will be returned on success, a negative errno will
+ * be returned in error cases.
+ */
+
+int regmap_field_write(struct regmap_field *field, unsigned int val)
+{
+   int field_bits;
+   unsigned int reg_mask;
+   field_bits = field-msb - field-lsb + 1;
+   reg_mask = ((BIT(field_bits) - 1)  field-lsb);
+   return regmap_update_bits(field-regmap, field-reg,
+   reg_mask, val  field-lsb);
+}
+EXPORT_SYMBOL_GPL(regmap_field_write);
+
 /*
  * regmap_bulk_write(): Write multiple registers to the device
  *
@@ -1532,6 +1553,32 @@ 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
+ *
+ * @field: Register field to read from
+ * @val: Pointer to store read value
+ *
+ * A value of zero will be returned on success, a negative errno will
+ * be returned in error cases.
+ */
+
+int regmap_field_read(struct regmap_field *field, unsigned int *val)
+{
+   int field_bits;
+   int ret;
+   ret = regmap_read(field-regmap, field-reg, val);
+   if (ret != 0)
+   return ret;
+
+   field_bits = field-msb - field-lsb + 1;
+   *val = field-lsb;
+   *val = (BIT(field_bits) - 1);
+
+   return ret;
+}
+EXPORT_SYMBOL_GPL(regmap_field_read);
+
+/**
  * regmap_bulk_read(): Read multiple registers from the device
  *
  * @map: Register map to write to
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index 02d84e2..f8dba11 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -411,6 +411,34 @@ bool regmap_reg_in_ranges(unsigned int reg,
  const struct regmap_range *ranges,
  unsigned int nranges);
 
+struct regmap_field {
+   struct regmap *regmap;
+   unsigned int reg;
+   unsigned int lsb;
+   unsigned int msb;
+};
+
+#define REGMAP_FIELD_INIT(regmap, reg, lsb, msb) { \
+   .regmap = regmap,   \
+   .reg = reg, \
+   .lsb = lsb, \
+   .msb = msb, \
+   }
+
+/* dynamic version of regmap_field intialization */
+static inline void regmap_field_init(struct regmap_field *field,
+   struct regmap *regmap,  unsigned int reg,
+   unsigned int lsb, unsigned int msb)
+{
+   field-regmap = regmap;
+   field-reg = reg;
+   field-lsb = lsb;
+   field-msb = msb;
+}
+
+int regmap_field_read(struct regmap_field *field,