Re: [PATCH v5 06/20] mfd: adp5585: refactor how regmap defaults are handled

[Lee Jones <lee@kernel.org>]

On Sat, 14 Jun 2025, Nuno Sá via B4 Relay wrote:

> From: Nuno Sá <nuno.sa@analog.com>
> 
> The only thing changing between variants is the regmap default
> registers. Hence, instead of having a regmap configuration for every
> variant (duplicating lots of fields), add a chip info type of structure
> with a regmap ID to identify which defaults to use and populate
> regmap_config at runtime given a template plus the id. Also note that
> between variants, the defaults can be the same which means the chip info
> structure can be used in more than one compatible.
> 
> This will also make it simpler adding new chips with more variants.
> 
> Also note that the chip info structures are deliberately not const as
> they will also contain lots of members that are the same between the
> different devices variants and so we will fill those at runtime.
> 
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> ---
>  drivers/mfd/adp5585.c       | 81 +++++++++++++++++++++++----------------------
>  include/linux/mfd/adp5585.h | 11 ++++++
>  2 files changed, 53 insertions(+), 39 deletions(-)

Very close.  Couple of nits.

NB: When you next submit, could you please ensure all subject match the
style expected by the subsystem.  In the case of MFD, it's:

mfd: <driver>: Succinct subject line starting with an uppercase char

> diff --git a/drivers/mfd/adp5585.c b/drivers/mfd/adp5585.c
> index c764f481875831ff55bccb8cdc59421719afbedd..ec88adbace92791f10953fc2bbb463fc59557bd6 100644
> --- a/drivers/mfd/adp5585.c
> +++ b/drivers/mfd/adp5585.c
> @@ -81,42 +81,37 @@ static const u8 adp5585_regmap_defaults_04[ADP5585_MAX_REG + 1] = {
>  	/* 0x38 */ 0x00, 0x00, 0x00, 0x00, 0x00,
>  };
>  
> -enum adp5585_regmap_type {
> -	ADP5585_REGMAP_00,
> -	ADP5585_REGMAP_02,
> -	ADP5585_REGMAP_04,
> +/* -1 since the enum starts at 1 for error checking in i2c_get_match_data() */

This comment is no longer applicable.

> +static const u8 *adp5585_regmap_defaults[ADP5585_MAX] = {
> +	[ADP5585_00] = adp5585_regmap_defaults_00,
> +	[ADP5585_01] = adp5585_regmap_defaults_00,
> +	[ADP5585_02] = adp5585_regmap_defaults_02,
> +	[ADP5585_03] = adp5585_regmap_defaults_00,
> +	[ADP5585_04] = adp5585_regmap_defaults_04,
>  };
>  
> -static const struct regmap_config adp5585_regmap_configs[] = {
> -	[ADP5585_REGMAP_00] = {
> -		.reg_bits = 8,
> -		.val_bits = 8,
> -		.max_register = ADP5585_MAX_REG,
> -		.volatile_table = &adp5585_volatile_regs,
> -		.cache_type = REGCACHE_MAPLE,
> -		.reg_defaults_raw = adp5585_regmap_defaults_00,
> -		.num_reg_defaults_raw = sizeof(adp5585_regmap_defaults_00),
> -	},
> -	[ADP5585_REGMAP_02] = {
> -		.reg_bits = 8,
> -		.val_bits = 8,
> -		.max_register = ADP5585_MAX_REG,
> -		.volatile_table = &adp5585_volatile_regs,
> -		.cache_type = REGCACHE_MAPLE,
> -		.reg_defaults_raw = adp5585_regmap_defaults_02,
> -		.num_reg_defaults_raw = sizeof(adp5585_regmap_defaults_02),
> -	},
> -	[ADP5585_REGMAP_04] = {
> -		.reg_bits = 8,
> -		.val_bits = 8,
> -		.max_register = ADP5585_MAX_REG,
> -		.volatile_table = &adp5585_volatile_regs,
> -		.cache_type = REGCACHE_MAPLE,
> -		.reg_defaults_raw = adp5585_regmap_defaults_04,
> -		.num_reg_defaults_raw = sizeof(adp5585_regmap_defaults_04),
> -	},
> +static const struct regmap_config adp5585_regmap_config_template = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = ADP5585_MAX_REG,
> +	.volatile_table = &adp5585_volatile_regs,
> +	.cache_type = REGCACHE_MAPLE,
> +	.num_reg_defaults_raw = ADP5585_MAX_REG + 1,
>  };
>  
> +static struct regmap_config *adp5585_fill_regmap_config(const struct adp5585_dev *adp5585)
> +{
> +	struct regmap_config *regmap_config;
> +
> +	regmap_config = devm_kmemdup(adp5585->dev, &adp5585_regmap_config_template,
> +				     sizeof(struct regmap_config), GFP_KERNEL);

sizeof(*regmap_config)

> +	if (!regmap_config)
> +		return ERR_PTR(-ENOMEM);
> +
> +	regmap_config->reg_defaults_raw = adp5585_regmap_defaults[adp5585->variant];
> +	return regmap_config;
> +}
> +
>  static int adp5585_add_devices(struct device *dev)
>  {
>  	int ret;
> @@ -147,7 +142,7 @@ static void adp5585_osc_disable(void *data)
>  
>  static int adp5585_i2c_probe(struct i2c_client *i2c)
>  {
> -	const struct regmap_config *regmap_config;
> +	struct regmap_config *regmap_config;
>  	struct adp5585_dev *adp5585;
>  	unsigned int id;
>  	int ret;
> @@ -157,8 +152,16 @@ static int adp5585_i2c_probe(struct i2c_client *i2c)
>  		return -ENOMEM;
>  
>  	i2c_set_clientdata(i2c, adp5585);
> +	adp5585->dev = &i2c->dev;
> +
> +	adp5585->variant = (enum adp5585_variant)(uintptr_t)i2c_get_match_data(i2c);
> +	if (!adp5585->variant)
> +		return -ENODEV;
> +
> +	regmap_config = adp5585_fill_regmap_config(adp5585);
> +	if (IS_ERR(regmap_config))
> +		return PTR_ERR(regmap_config);
>  
> -	regmap_config = i2c_get_match_data(i2c);
>  	adp5585->regmap = devm_regmap_init_i2c(i2c, regmap_config);
>  	if (IS_ERR(adp5585->regmap))
>  		return dev_err_probe(&i2c->dev, PTR_ERR(adp5585->regmap),
> @@ -212,19 +215,19 @@ static DEFINE_SIMPLE_DEV_PM_OPS(adp5585_pm, adp5585_suspend, adp5585_resume);
>  static const struct of_device_id adp5585_of_match[] = {
>  	{
>  		.compatible = "adi,adp5585-00",
> -		.data = &adp5585_regmap_configs[ADP5585_REGMAP_00],
> +		.data = (void *)ADP5585_00,
>  	}, {
>  		.compatible = "adi,adp5585-01",
> -		.data = &adp5585_regmap_configs[ADP5585_REGMAP_00],
> +		.data = (void *)ADP5585_01,
>  	}, {
>  		.compatible = "adi,adp5585-02",
> -		.data = &adp5585_regmap_configs[ADP5585_REGMAP_02],
> +		.data = (void *)ADP5585_02,
>  	}, {
>  		.compatible = "adi,adp5585-03",
> -		.data = &adp5585_regmap_configs[ADP5585_REGMAP_00],
> +		.data = (void *)ADP5585_03,
>  	}, {
>  		.compatible = "adi,adp5585-04",
> -		.data = &adp5585_regmap_configs[ADP5585_REGMAP_04],
> +		.data = (void *)ADP5585_04,
>  	},
>  	{ /* sentinel */ }
>  };
> diff --git a/include/linux/mfd/adp5585.h b/include/linux/mfd/adp5585.h
> index 016033cd68e46757aca86d21dd37025fd354b801..c56af8d8d76c4ebc0ede1ee4769ca059de29f53c 100644
> --- a/include/linux/mfd/adp5585.h
> +++ b/include/linux/mfd/adp5585.h
> @@ -119,8 +119,19 @@
>  
>  struct regmap;
>  
> +enum adp5585_variant {
> +	ADP5585_00 = 1,
> +	ADP5585_01,
> +	ADP5585_02,
> +	ADP5585_03,
> +	ADP5585_04,
> +	ADP5585_MAX
> +};
> +
>  struct adp5585_dev {
> +	struct device *dev;
>  	struct regmap *regmap;
> +	enum adp5585_variant variant;
>  };
>  
>  #endif
> 
> -- 
> 2.49.0
> 
> 

-- 
Lee Jones [李琼斯]