From: Srinivas Kandagatla <[email protected]>
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 <[email protected]>
---
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 regmap_field_write(struct regmap_field *field, unsigned int val);
+
/**
* Description of an IRQ for the generic regmap irq_chip.
*
--
1.7.6.5
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 <[email protected]>
>
> 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 <[email protected]>
> ---
> 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 regmap_field_write(struct regmap_field *field, unsigned int val);
> +
> /**
> * Description of an IRQ for the generic regmap irq_chip.
> *
>
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.
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.
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
>
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.
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.
>
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.
[...]
> +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);
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);
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).
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.