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@xxxxxxxxxx>
>> ---
>> sound/soc/codecs/lpass-rx-macro.c | 17 +++++++++++++----
>> 1 file changed, 13 insertions(+), 4 deletions(-)
>
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
>
>>
>> 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
