Re: [RFC] regmap: Add regmap_field APIs

[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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/