| Message ID | 20200925164856.10315-2-srinivas.kandagatla@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | regmap: add support to regmap_field_bulk_alloc/free | expand |
On Fri, Sep 25, 2020 at 05:48:55PM +0100, Srinivas Kandagatla wrote: > Usage of regmap_field_alloc becomes much overhead when number of fields > exceed more than 3. > QCOM LPASS driver has extensively converted to use regmap_fields. > > Using new bulk api to allocate fields makes it much more cleaner code to read! > > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > Tested-by: Srinivasa Rao Mandadapu <srivasam@codeaurora.org> > --- > drivers/base/regmap/regmap.c | 100 +++++++++++++++++++++++++++++++++++ > include/linux/regmap.h | 11 ++++ > 2 files changed, 111 insertions(+) > > diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c > index aec3f26bf711..8d6aedce666d 100644 > --- a/drivers/base/regmap/regmap.c > +++ b/drivers/base/regmap/regmap.c > @@ -1270,6 +1270,106 @@ struct regmap_field *devm_regmap_field_alloc(struct device *dev, > } > EXPORT_SYMBOL_GPL(devm_regmap_field_alloc); > > + > +/** > + * regmap_field_bulk_alloc() - Allocate and initialise a bulk register field. > + * > + * @regmap: regmap bank in which this register field is located. > + * @rm_field: regmap register fields within the bank. > + * @reg_field: Register fields within the bank. > + * @num_fields: Number of register fields. > + * > + * The return value will be an -ENOMEM on error or zero for success. > + * Newly allocated regmap_fields should be freed by calling > + * regmap_field_bulk_free() > + */ > +int regmap_field_bulk_alloc(struct regmap *regmap, > + struct regmap_field **rm_field, > + struct reg_field *reg_field, > + int num_fields) > +{ > + struct regmap_field *rf; > + int i; > + > + rf = kcalloc(num_fields, sizeof(*rf), GFP_KERNEL); > + if (!rf) > + return -ENOMEM; > + > + for (i = 0; i < num_fields; i++) { > + regmap_field_init(&rf[i], regmap, reg_field[i]); > + rm_field[i] = &rf[i]; > + } > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(regmap_field_bulk_alloc); > + > +/** > + * devm_regmap_field_bulk_alloc() - Allocate and initialise a bulk register > + * fields. > + * > + * @dev: Device that will be interacted with > + * @regmap: regmap bank in which this register field is located. > + * @rm_field: regmap register fields within the bank. > + * @reg_field: Register fields within the bank. > + * @num_fields: Number of register fields. > + * > + * The return value will be an -ENOMEM on error or zero for success. > + * Newly allocated regmap_fields will be automatically freed by the > + * device management code. > + */ > +int devm_regmap_field_bulk_alloc(struct device *dev, > + struct regmap *regmap, > + struct regmap_field **rm_field, > + struct reg_field *reg_field, > + int num_fields) > +{ > + struct regmap_field *rf; > + int i; > + > + rf = devm_kcalloc(dev, num_fields, sizeof(*rf), GFP_KERNEL); > + if (!rf) > + return -ENOMEM; > + > + for (i = 0; i < num_fields; i++) { > + regmap_field_init(&rf[i], regmap, reg_field[i]); > + rm_field[i] = &rf[i]; > + } > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(devm_regmap_field_bulk_alloc); > + > +/** > + * regmap_field_bulk_free() - Free register field allocated using > + * regmap_field_bulk_alloc. > + * > + * @field: regmap fields which should be freed. > + */ > +void regmap_field_bulk_free(struct regmap_field *field) > +{ > + kfree(field); > +} > +EXPORT_SYMBOL_GPL(regmap_field_bulk_free); > + > +/** > + * devm_regmap_field_bulk_free() - Free a bulk register field allocated using > + * devm_regmap_field_bulk_alloc. > + * > + * @dev: Device that will be interacted with > + * @field: regmap field which should be freed. > + * > + * Free register field allocated using devm_regmap_field_bulk_alloc(). Usually > + * drivers need not call this function, as the memory allocated via devm > + * will be freed as per device-driver life-cycle. > + */ > +void devm_regmap_field_bulk_free(struct device *dev, > + struct regmap_field *field) > +{ > + devm_kfree(dev, field); > +} > +EXPORT_SYMBOL_GPL(devm_regmap_field_bulk_free); > + > /** > * devm_regmap_field_free() - Free a register field allocated using > * devm_regmap_field_alloc. > diff --git a/include/linux/regmap.h b/include/linux/regmap.h > index 0c49d59168b5..a35ec0a0d6e0 100644 > --- a/include/linux/regmap.h > +++ b/include/linux/regmap.h > @@ -1189,6 +1189,17 @@ struct regmap_field *devm_regmap_field_alloc(struct device *dev, > struct regmap *regmap, struct reg_field reg_field); > void devm_regmap_field_free(struct device *dev, struct regmap_field *field); > > +int regmap_field_bulk_alloc(struct regmap *regmap, > + struct regmap_field **rm_field, > + struct reg_field *reg_field, > + int num_fields); > +void regmap_field_bulk_free(struct regmap_field *field); > +int devm_regmap_field_bulk_alloc(struct device *dev, struct regmap *regmap, > + struct regmap_field **field, > + struct reg_field *reg_field, int num_fields); > +void devm_regmap_field_bulk_free(struct device *dev, > + struct regmap_field *field); > + You only have a patch that uses these last two functions, so why add all 4? We don't add infrastructure to the kernel without users. thanks, greg k-h
On Wed, Sep 30, 2020 at 01:59:15PM +0200, Greg KH wrote: > On Fri, Sep 25, 2020 at 05:48:55PM +0100, Srinivas Kandagatla wrote: > > +int devm_regmap_field_bulk_alloc(struct device *dev, struct regmap *regmap, > > + struct regmap_field **field, > > + struct reg_field *reg_field, int num_fields); > > +void devm_regmap_field_bulk_free(struct device *dev, > > + struct regmap_field *field); > > + > > You only have a patch that uses these last two functions, so why add all > 4? We don't add infrastructure to the kernel without users. We have managed versions of the other regmap allocation functions, it makes sense for consistency to have managed versions of these too. I think there's a meaningful difference between a simple and expected wrapper like these and infrastructure which hasn't been proved out by users. Please delete unneeded context from mails when replying. Doing this makes it much easier to find your reply in the message, helping ensure it won't be missed by people scrolling through the irrelevant quoted material.
On Wed, Sep 30, 2020 at 01:08:49PM +0100, Mark Brown wrote: > On Wed, Sep 30, 2020 at 01:59:15PM +0200, Greg KH wrote: > > On Fri, Sep 25, 2020 at 05:48:55PM +0100, Srinivas Kandagatla wrote: > > > > +int devm_regmap_field_bulk_alloc(struct device *dev, struct regmap *regmap, > > > + struct regmap_field **field, > > > + struct reg_field *reg_field, int num_fields); > > > +void devm_regmap_field_bulk_free(struct device *dev, > > > + struct regmap_field *field); > > > + > > > > You only have a patch that uses these last two functions, so why add all > > 4? We don't add infrastructure to the kernel without users. > > We have managed versions of the other regmap allocation functions, it > makes sense for consistency to have managed versions of these too. I > think there's a meaningful difference between a simple and expected > wrapper like these and infrastructure which hasn't been proved out by > users. Ok, do you think these are really needed? A review would be nice :) thanks, greg k-h
On Wed, Sep 30, 2020 at 02:11:00PM +0200, Greg KH wrote: > On Wed, Sep 30, 2020 at 01:08:49PM +0100, Mark Brown wrote: > > We have managed versions of the other regmap allocation functions, it > > makes sense for consistency to have managed versions of these too. I > > think there's a meaningful difference between a simple and expected > > wrapper like these and infrastructure which hasn't been proved out by > > users. > Ok, do you think these are really needed? > A review would be nice :) I applied this patch the other day - ea470b82f205fc in -next.
On Wed, Sep 30, 2020 at 01:15:52PM +0100, Mark Brown wrote: > On Wed, Sep 30, 2020 at 02:11:00PM +0200, Greg KH wrote: > > On Wed, Sep 30, 2020 at 01:08:49PM +0100, Mark Brown wrote: > > > > We have managed versions of the other regmap allocation functions, it > > > makes sense for consistency to have managed versions of these too. I > > > think there's a meaningful difference between a simple and expected > > > wrapper like these and infrastructure which hasn't been proved out by > > > users. > > > Ok, do you think these are really needed? > > > A review would be nice :) > > I applied this patch the other day - ea470b82f205fc in -next. Great, sorry for the noise. greg k-h
diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c index aec3f26bf711..8d6aedce666d 100644 --- a/drivers/base/regmap/regmap.c +++ b/drivers/base/regmap/regmap.c @@ -1270,6 +1270,106 @@ struct regmap_field *devm_regmap_field_alloc(struct device *dev, } EXPORT_SYMBOL_GPL(devm_regmap_field_alloc); + +/** + * regmap_field_bulk_alloc() - Allocate and initialise a bulk register field. + * + * @regmap: regmap bank in which this register field is located. + * @rm_field: regmap register fields within the bank. + * @reg_field: Register fields within the bank. + * @num_fields: Number of register fields. + * + * The return value will be an -ENOMEM on error or zero for success. + * Newly allocated regmap_fields should be freed by calling + * regmap_field_bulk_free() + */ +int regmap_field_bulk_alloc(struct regmap *regmap, + struct regmap_field **rm_field, + struct reg_field *reg_field, + int num_fields) +{ + struct regmap_field *rf; + int i; + + rf = kcalloc(num_fields, sizeof(*rf), GFP_KERNEL); + if (!rf) + return -ENOMEM; + + for (i = 0; i < num_fields; i++) { + regmap_field_init(&rf[i], regmap, reg_field[i]); + rm_field[i] = &rf[i]; + } + + return 0; +} +EXPORT_SYMBOL_GPL(regmap_field_bulk_alloc); + +/** + * devm_regmap_field_bulk_alloc() - Allocate and initialise a bulk register + * fields. + * + * @dev: Device that will be interacted with + * @regmap: regmap bank in which this register field is located. + * @rm_field: regmap register fields within the bank. + * @reg_field: Register fields within the bank. + * @num_fields: Number of register fields. + * + * The return value will be an -ENOMEM on error or zero for success. + * Newly allocated regmap_fields will be automatically freed by the + * device management code. + */ +int devm_regmap_field_bulk_alloc(struct device *dev, + struct regmap *regmap, + struct regmap_field **rm_field, + struct reg_field *reg_field, + int num_fields) +{ + struct regmap_field *rf; + int i; + + rf = devm_kcalloc(dev, num_fields, sizeof(*rf), GFP_KERNEL); + if (!rf) + return -ENOMEM; + + for (i = 0; i < num_fields; i++) { + regmap_field_init(&rf[i], regmap, reg_field[i]); + rm_field[i] = &rf[i]; + } + + return 0; +} +EXPORT_SYMBOL_GPL(devm_regmap_field_bulk_alloc); + +/** + * regmap_field_bulk_free() - Free register field allocated using + * regmap_field_bulk_alloc. + * + * @field: regmap fields which should be freed. + */ +void regmap_field_bulk_free(struct regmap_field *field) +{ + kfree(field); +} +EXPORT_SYMBOL_GPL(regmap_field_bulk_free); + +/** + * devm_regmap_field_bulk_free() - Free a bulk register field allocated using + * devm_regmap_field_bulk_alloc. + * + * @dev: Device that will be interacted with + * @field: regmap field which should be freed. + * + * Free register field allocated using devm_regmap_field_bulk_alloc(). Usually + * drivers need not call this function, as the memory allocated via devm + * will be freed as per device-driver life-cycle. + */ +void devm_regmap_field_bulk_free(struct device *dev, + struct regmap_field *field) +{ + devm_kfree(dev, field); +} +EXPORT_SYMBOL_GPL(devm_regmap_field_bulk_free); + /** * devm_regmap_field_free() - Free a register field allocated using * devm_regmap_field_alloc. diff --git a/include/linux/regmap.h b/include/linux/regmap.h index 0c49d59168b5..a35ec0a0d6e0 100644 --- a/include/linux/regmap.h +++ b/include/linux/regmap.h @@ -1189,6 +1189,17 @@ struct regmap_field *devm_regmap_field_alloc(struct device *dev, struct regmap *regmap, struct reg_field reg_field); void devm_regmap_field_free(struct device *dev, struct regmap_field *field); +int regmap_field_bulk_alloc(struct regmap *regmap, + struct regmap_field **rm_field, + struct reg_field *reg_field, + int num_fields); +void regmap_field_bulk_free(struct regmap_field *field); +int devm_regmap_field_bulk_alloc(struct device *dev, struct regmap *regmap, + struct regmap_field **field, + struct reg_field *reg_field, int num_fields); +void devm_regmap_field_bulk_free(struct device *dev, + struct regmap_field *field); + int regmap_field_read(struct regmap_field *field, unsigned int *val); int regmap_field_update_bits_base(struct regmap_field *field, unsigned int mask, unsigned int val,