[PATCH v2 0/2] regmap: add support to regmap_field_bulk_alloc/free
Usage of regmap_field_alloc becomes much overhead when number of fields exceed more than 3. Most of driver seems to totally covered up with these allocs/free making to very hard to read the code! On such driver is QCOM LPASS driver has extensively converted to use regmap_fields.
This patchset add this new api and a user of it.
Using new bulk api to allocate fields makes it much more cleaner code to read!
Changes since v1: - Fix lot of spelling! No code changes!
Srinivas Kandagatla (2): regmap: add support to regmap_field_bulk_alloc/free apis ASoC: lpass-platform: use devm_regmap_field_bulk_alloc
drivers/base/regmap/regmap.c | 100 ++++++++++++++++++++++++++++++++ include/linux/regmap.h | 11 ++++ sound/soc/qcom/lpass-platform.c | 31 +++------- 3 files changed, 118 insertions(+), 24 deletions(-)
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); + 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,
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
use new devm_regmap_field_bulk_alloc to allocate fields as it make the code more readable!
Signed-off-by: Srinivas Kandagatla srinivas.kandagatla@linaro.org Tested-by: Srinivasa Rao Mandadapu srivasam@codeaurora.org --- sound/soc/qcom/lpass-platform.c | 31 +++++++------------------------ 1 file changed, 7 insertions(+), 24 deletions(-)
diff --git a/sound/soc/qcom/lpass-platform.c b/sound/soc/qcom/lpass-platform.c index df692ed95503..7ac26290082f 100644 --- a/sound/soc/qcom/lpass-platform.c +++ b/sound/soc/qcom/lpass-platform.c @@ -56,6 +56,7 @@ static int lpass_platform_alloc_dmactl_fields(struct device *dev, struct lpass_data *drvdata = dev_get_drvdata(dev); struct lpass_variant *v = drvdata->variant; struct lpaif_dmactl *rd_dmactl, *wr_dmactl; + int rval;
drvdata->rd_dmactl = devm_kzalloc(dev, sizeof(struct lpaif_dmactl), GFP_KERNEL); @@ -70,31 +71,13 @@ static int lpass_platform_alloc_dmactl_fields(struct device *dev, rd_dmactl = drvdata->rd_dmactl; wr_dmactl = drvdata->wr_dmactl;
- rd_dmactl->bursten = devm_regmap_field_alloc(dev, map, v->rdma_bursten); - rd_dmactl->wpscnt = devm_regmap_field_alloc(dev, map, v->rdma_wpscnt); - rd_dmactl->fifowm = devm_regmap_field_alloc(dev, map, v->rdma_fifowm); - rd_dmactl->intf = devm_regmap_field_alloc(dev, map, v->rdma_intf); - rd_dmactl->enable = devm_regmap_field_alloc(dev, map, v->rdma_enable); - rd_dmactl->dyncclk = devm_regmap_field_alloc(dev, map, v->rdma_dyncclk); + rval = devm_regmap_field_bulk_alloc(dev, map, &rd_dmactl->bursten, + &v->rdma_bursten, 6); + if (rval) + return rval;
- if (IS_ERR(rd_dmactl->bursten) || IS_ERR(rd_dmactl->wpscnt) || - IS_ERR(rd_dmactl->fifowm) || IS_ERR(rd_dmactl->intf) || - IS_ERR(rd_dmactl->enable) || IS_ERR(rd_dmactl->dyncclk)) - return -EINVAL; - - wr_dmactl->bursten = devm_regmap_field_alloc(dev, map, v->wrdma_bursten); - wr_dmactl->wpscnt = devm_regmap_field_alloc(dev, map, v->wrdma_wpscnt); - wr_dmactl->fifowm = devm_regmap_field_alloc(dev, map, v->wrdma_fifowm); - wr_dmactl->intf = devm_regmap_field_alloc(dev, map, v->wrdma_intf); - wr_dmactl->enable = devm_regmap_field_alloc(dev, map, v->wrdma_enable); - wr_dmactl->dyncclk = devm_regmap_field_alloc(dev, map, v->wrdma_dyncclk); - - if (IS_ERR(wr_dmactl->bursten) || IS_ERR(wr_dmactl->wpscnt) || - IS_ERR(wr_dmactl->fifowm) || IS_ERR(wr_dmactl->intf) || - IS_ERR(wr_dmactl->enable) || IS_ERR(wr_dmactl->dyncclk)) - return -EINVAL; - - return 0; + return devm_regmap_field_bulk_alloc(dev, map, &wr_dmactl->bursten, + &v->wrdma_bursten, 6); }
static int lpass_platform_pcmops_open(struct snd_soc_component *component,
On Fri, 25 Sep 2020 17:48:54 +0100, Srinivas Kandagatla wrote:
Usage of regmap_field_alloc becomes much overhead when number of fields exceed more than 3. Most of driver seems to totally covered up with these allocs/free making to very hard to read the code! On such driver is QCOM LPASS driver has extensively converted to use regmap_fields.
This patchset add this new api and a user of it.
[...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
Thanks!
[1/2] regmap: add support to regmap_field_bulk_alloc/free apis commit: 95892d4075db67fb570a5d43c950318057e8a871 [2/2] ASoC: lpass-platform: use devm_regmap_field_bulk_alloc commit: 44e755fb54feda74e7af7c2ddc04cc23b64ee39c
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
participants (3)
-
Greg KH -
Mark Brown -
Srinivas Kandagatla