diff mbox series

[2/4] ASoC: codecs: lpass-rx-macro: Keep static regmap_config as const

Message ID 20240627-b4-qcom-audio-lpass-codec-cleanups-v1-2-ede31891d238@linaro.org (mailing list archive)
State Superseded
Headers show
Series ASoC: codecs: lpass-rx-macro: Few code cleanups | expand

Commit Message

Krzysztof Kozlowski June 27, 2024, 3:23 p.m. UTC
The driver has static 'struct regmap_config', which is then customized
depending on device version.  This works fine, because there should not
be two devices in a system simultaneously and even less likely that such
two devices would have different versions, thus different regmap config.
However code is cleaner and more obvious when static data in the driver
is also const - it serves as a template.

Mark the 'struct regmap_config' as const and duplicate it in the probe()
with devm_kmemdup to allow customizing per detected device variant.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 sound/soc/codecs/lpass-rx-macro.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

Comments

Dmitry Baryshkov June 28, 2024, 8:34 a.m. UTC | #1
On Thu, Jun 27, 2024 at 05:23:44PM GMT, Krzysztof Kozlowski wrote:
> The driver has static 'struct regmap_config', which is then customized
> depending on device version.  This works fine, because there should not
> be two devices in a system simultaneously and even less likely that such
> two devices would have different versions, thus different regmap config.
> However code is cleaner and more obvious when static data in the driver
> is also const - it serves as a template.
> 
> Mark the 'struct regmap_config' as const and duplicate it in the probe()
> with devm_kmemdup to allow customizing per detected device variant.
> 
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> ---
>  sound/soc/codecs/lpass-rx-macro.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

> 
> diff --git a/sound/soc/codecs/lpass-rx-macro.c b/sound/soc/codecs/lpass-rx-macro.c
> index 59fe76b13cdb..3d8149665439 100644
> --- a/sound/soc/codecs/lpass-rx-macro.c
> +++ b/sound/soc/codecs/lpass-rx-macro.c
> @@ -1662,7 +1662,7 @@ static bool rx_is_readable_register(struct device *dev, unsigned int reg)
>  	return rx_is_rw_register(dev, reg);
>  }
>  
> -static struct regmap_config rx_regmap_config = {
> +static const struct regmap_config rx_regmap_config = {
>  	.name = "rx_macro",
>  	.reg_bits = 16,
>  	.val_bits = 32, /* 8 but with 32 bit read/write */
> @@ -3765,6 +3765,7 @@ static const struct snd_soc_component_driver rx_macro_component_drv = {
>  static int rx_macro_probe(struct platform_device *pdev)
>  {
>  	struct reg_default *reg_defaults;
> +	struct regmap_config *reg_config;
>  	struct device *dev = &pdev->dev;
>  	kernel_ulong_t flags;
>  	struct rx_macro *rx;
> @@ -3851,14 +3852,22 @@ static int rx_macro_probe(struct platform_device *pdev)
>  		goto err;
>  	}
>  
> -	rx_regmap_config.reg_defaults = reg_defaults;
> -	rx_regmap_config.num_reg_defaults = def_count;
> +	reg_config = devm_kmemdup(dev, &rx_regmap_config, sizeof(*reg_config),
> +				  GFP_KERNEL);
> +	if (!reg_config) {
> +		ret = -ENOMEM;
> +		goto err;
> +	}
>  
> -	rx->regmap = devm_regmap_init_mmio(dev, base, &rx_regmap_config);
> +	reg_config->reg_defaults = reg_defaults;
> +	reg_config->num_reg_defaults = def_count;
> +
> +	rx->regmap = devm_regmap_init_mmio(dev, base, reg_config);
>  	if (IS_ERR(rx->regmap)) {
>  		ret = PTR_ERR(rx->regmap);
>  		goto err;
>  	}
> +	devm_kfree(dev, reg_config);
>  	devm_kfree(dev, reg_defaults);

Seeing devm_kfree in the non-error path makes me feel strange. Maybe
it's one of the rare occasions when I can say that __free is suitable
here.

>  
>  	dev_set_drvdata(dev, rx);
> 
> -- 
> 2.43.0
>
Krzysztof Kozlowski June 28, 2024, 8:39 a.m. UTC | #2
On 28/06/2024 10:34, Dmitry Baryshkov wrote:
> On Thu, Jun 27, 2024 at 05:23:44PM GMT, Krzysztof Kozlowski wrote:
>> The driver has static 'struct regmap_config', which is then customized
>> depending on device version.  This works fine, because there should not
>> be two devices in a system simultaneously and even less likely that such
>> two devices would have different versions, thus different regmap config.
>> However code is cleaner and more obvious when static data in the driver
>> is also const - it serves as a template.
>>
>> Mark the 'struct regmap_config' as const and duplicate it in the probe()
>> with devm_kmemdup to allow customizing per detected device variant.
>>
>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> ---
>>  sound/soc/codecs/lpass-rx-macro.c | 17 +++++++++++++----
>>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> 
>>
>> diff --git a/sound/soc/codecs/lpass-rx-macro.c b/sound/soc/codecs/lpass-rx-macro.c
>> index 59fe76b13cdb..3d8149665439 100644
>> --- a/sound/soc/codecs/lpass-rx-macro.c
>> +++ b/sound/soc/codecs/lpass-rx-macro.c
>> @@ -1662,7 +1662,7 @@ static bool rx_is_readable_register(struct device *dev, unsigned int reg)
>>  	return rx_is_rw_register(dev, reg);
>>  }
>>  
>> -static struct regmap_config rx_regmap_config = {
>> +static const struct regmap_config rx_regmap_config = {
>>  	.name = "rx_macro",
>>  	.reg_bits = 16,
>>  	.val_bits = 32, /* 8 but with 32 bit read/write */
>> @@ -3765,6 +3765,7 @@ static const struct snd_soc_component_driver rx_macro_component_drv = {
>>  static int rx_macro_probe(struct platform_device *pdev)
>>  {
>>  	struct reg_default *reg_defaults;
>> +	struct regmap_config *reg_config;
>>  	struct device *dev = &pdev->dev;
>>  	kernel_ulong_t flags;
>>  	struct rx_macro *rx;
>> @@ -3851,14 +3852,22 @@ static int rx_macro_probe(struct platform_device *pdev)
>>  		goto err;
>>  	}
>>  
>> -	rx_regmap_config.reg_defaults = reg_defaults;
>> -	rx_regmap_config.num_reg_defaults = def_count;
>> +	reg_config = devm_kmemdup(dev, &rx_regmap_config, sizeof(*reg_config),
>> +				  GFP_KERNEL);
>> +	if (!reg_config) {
>> +		ret = -ENOMEM;
>> +		goto err;
>> +	}
>>  
>> -	rx->regmap = devm_regmap_init_mmio(dev, base, &rx_regmap_config);
>> +	reg_config->reg_defaults = reg_defaults;
>> +	reg_config->num_reg_defaults = def_count;
>> +
>> +	rx->regmap = devm_regmap_init_mmio(dev, base, reg_config);
>>  	if (IS_ERR(rx->regmap)) {
>>  		ret = PTR_ERR(rx->regmap);
>>  		goto err;
>>  	}
>> +	devm_kfree(dev, reg_config);
>>  	devm_kfree(dev, reg_defaults);
> 
> Seeing devm_kfree in the non-error path makes me feel strange. Maybe
> it's one of the rare occasions when I can say that __free is suitable
> here.

These would have a bit different meaning in such case. The __free would
not clean it in this spot, but on exit from the scope. I wanted to
kfree() here, because the config (and reg_defaults) are not used by past
regmap_init. I mentioned it briefly in previous patch msg.

To me this code is readable and obvious - past this point nothing uses
that allocation. However maybe instead of devm(), the code would be
easier to read if non-devm-malloc + __free()?

Best regards,
Krzysztof
Dmitry Baryshkov June 28, 2024, 2:17 p.m. UTC | #3
On Fri, 28 Jun 2024 at 11:39, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 28/06/2024 10:34, Dmitry Baryshkov wrote:
> > On Thu, Jun 27, 2024 at 05:23:44PM GMT, Krzysztof Kozlowski wrote:
> >> The driver has static 'struct regmap_config', which is then customized
> >> depending on device version.  This works fine, because there should not
> >> be two devices in a system simultaneously and even less likely that such
> >> two devices would have different versions, thus different regmap config.
> >> However code is cleaner and more obvious when static data in the driver
> >> is also const - it serves as a template.
> >>
> >> Mark the 'struct regmap_config' as const and duplicate it in the probe()
> >> with devm_kmemdup to allow customizing per detected device variant.
> >>
> >> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> >> ---
> >>  sound/soc/codecs/lpass-rx-macro.c | 17 +++++++++++++----
> >>  1 file changed, 13 insertions(+), 4 deletions(-)
> >
> > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >
> >>
> >> diff --git a/sound/soc/codecs/lpass-rx-macro.c b/sound/soc/codecs/lpass-rx-macro.c
> >> index 59fe76b13cdb..3d8149665439 100644
> >> --- a/sound/soc/codecs/lpass-rx-macro.c
> >> +++ b/sound/soc/codecs/lpass-rx-macro.c
> >> @@ -1662,7 +1662,7 @@ static bool rx_is_readable_register(struct device *dev, unsigned int reg)
> >>      return rx_is_rw_register(dev, reg);
> >>  }
> >>
> >> -static struct regmap_config rx_regmap_config = {
> >> +static const struct regmap_config rx_regmap_config = {
> >>      .name = "rx_macro",
> >>      .reg_bits = 16,
> >>      .val_bits = 32, /* 8 but with 32 bit read/write */
> >> @@ -3765,6 +3765,7 @@ static const struct snd_soc_component_driver rx_macro_component_drv = {
> >>  static int rx_macro_probe(struct platform_device *pdev)
> >>  {
> >>      struct reg_default *reg_defaults;
> >> +    struct regmap_config *reg_config;
> >>      struct device *dev = &pdev->dev;
> >>      kernel_ulong_t flags;
> >>      struct rx_macro *rx;
> >> @@ -3851,14 +3852,22 @@ static int rx_macro_probe(struct platform_device *pdev)
> >>              goto err;
> >>      }
> >>
> >> -    rx_regmap_config.reg_defaults = reg_defaults;
> >> -    rx_regmap_config.num_reg_defaults = def_count;
> >> +    reg_config = devm_kmemdup(dev, &rx_regmap_config, sizeof(*reg_config),
> >> +                              GFP_KERNEL);
> >> +    if (!reg_config) {
> >> +            ret = -ENOMEM;
> >> +            goto err;
> >> +    }
> >>
> >> -    rx->regmap = devm_regmap_init_mmio(dev, base, &rx_regmap_config);
> >> +    reg_config->reg_defaults = reg_defaults;
> >> +    reg_config->num_reg_defaults = def_count;
> >> +
> >> +    rx->regmap = devm_regmap_init_mmio(dev, base, reg_config);
> >>      if (IS_ERR(rx->regmap)) {
> >>              ret = PTR_ERR(rx->regmap);
> >>              goto err;
> >>      }
> >> +    devm_kfree(dev, reg_config);
> >>      devm_kfree(dev, reg_defaults);
> >
> > Seeing devm_kfree in the non-error path makes me feel strange. Maybe
> > it's one of the rare occasions when I can say that __free is suitable
> > here.
>
> These would have a bit different meaning in such case. The __free would
> not clean it in this spot, but on exit from the scope. I wanted to
> kfree() here, because the config (and reg_defaults) are not used by past
> regmap_init. I mentioned it briefly in previous patch msg.
>
> To me this code is readable and obvious - past this point nothing uses
> that allocation. However maybe instead of devm(), the code would be
> easier to read if non-devm-malloc + __free()?

Yes, that's what I was thinking about. But it's definitely an optional
topic. Your code is correct.
diff mbox series

Patch

diff --git a/sound/soc/codecs/lpass-rx-macro.c b/sound/soc/codecs/lpass-rx-macro.c
index 59fe76b13cdb..3d8149665439 100644
--- a/sound/soc/codecs/lpass-rx-macro.c
+++ b/sound/soc/codecs/lpass-rx-macro.c
@@ -1662,7 +1662,7 @@  static bool rx_is_readable_register(struct device *dev, unsigned int reg)
 	return rx_is_rw_register(dev, reg);
 }
 
-static struct regmap_config rx_regmap_config = {
+static const struct regmap_config rx_regmap_config = {
 	.name = "rx_macro",
 	.reg_bits = 16,
 	.val_bits = 32, /* 8 but with 32 bit read/write */
@@ -3765,6 +3765,7 @@  static const struct snd_soc_component_driver rx_macro_component_drv = {
 static int rx_macro_probe(struct platform_device *pdev)
 {
 	struct reg_default *reg_defaults;
+	struct regmap_config *reg_config;
 	struct device *dev = &pdev->dev;
 	kernel_ulong_t flags;
 	struct rx_macro *rx;
@@ -3851,14 +3852,22 @@  static int rx_macro_probe(struct platform_device *pdev)
 		goto err;
 	}
 
-	rx_regmap_config.reg_defaults = reg_defaults;
-	rx_regmap_config.num_reg_defaults = def_count;
+	reg_config = devm_kmemdup(dev, &rx_regmap_config, sizeof(*reg_config),
+				  GFP_KERNEL);
+	if (!reg_config) {
+		ret = -ENOMEM;
+		goto err;
+	}
 
-	rx->regmap = devm_regmap_init_mmio(dev, base, &rx_regmap_config);
+	reg_config->reg_defaults = reg_defaults;
+	reg_config->num_reg_defaults = def_count;
+
+	rx->regmap = devm_regmap_init_mmio(dev, base, reg_config);
 	if (IS_ERR(rx->regmap)) {
 		ret = PTR_ERR(rx->regmap);
 		goto err;
 	}
+	devm_kfree(dev, reg_config);
 	devm_kfree(dev, reg_defaults);
 
 	dev_set_drvdata(dev, rx);