linux-kernel - Re: [RFC] regmap: Add regmap

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <51AF0313.6000205@st.com>
Date:	Wed, 05 Jun 2013 10:21:23 +0100
From:	Srinivas KANDAGATLA <srinivas.kandagatla@...com>
To:	Mark Brown <broonie@...nel.org>
Cc:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	linux-kernel@...r.kernel.org
Subject: Re: [RFC] regmap: Add regmap_field APIs

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