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

["Nuno Sá" <noname.nuno@gmail.com>]

On Fri, 2025-05-23 at 16:03 +0100, Lee Jones wrote:
> On Wed, 21 May 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 condig for every variant
> 
> Spellcheck.
> 
> > (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
> 
> "ID"
> 
> > 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       | 74 +++++++++++++++++++++---------------------
> > ---
> >  include/linux/mfd/adp5585.h | 10 ++++++
> >  2 files changed, 44 insertions(+), 40 deletions(-)
> > 
> > diff --git a/drivers/mfd/adp5585.c b/drivers/mfd/adp5585.c
> > index
> > 179dc284833ae8f39eefc6787dd2c7158dfd3ad7..672f3468bda5be6af85a5982c3626053b4
> > cb59bf 100644
> > --- a/drivers/mfd/adp5585.c
> > +++ b/drivers/mfd/adp5585.c
> > @@ -81,42 +81,31 @@ 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()*/
> 
> Space before the '*'.
> 
> > +static const u8 *adp5585_regmap_defaults[ADP5585_MAX - 1] = {
> > +	[ADP5585_00 - 1] = adp5585_regmap_defaults_00,
> > +	[ADP5585_01 - 1] = adp5585_regmap_defaults_00,
> > +	[ADP5585_02 - 1] = adp5585_regmap_defaults_02,
> > +	[ADP5585_03 - 1] = adp5585_regmap_defaults_00,
> > +	[ADP5585_04 - 1] = adp5585_regmap_defaults_04,
> 
> Just leave the first entry blank.  No need for all he gymnastics.

alright

> 
> >  };
> >  
> > -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 void adp5585_fill_regmap_config(const struct adp5585_dev *adp5585,
> > +				       struct regmap_config *regmap_config)
> > +{
> > +	*regmap_config = adp5585_regmap_config_template;
> 
> Return struct regmap_config * instead.
> 
> > +	regmap_config->reg_defaults_raw = adp5585_regmap_defaults[adp5585-
> > >variant - 1];
> 
> Does this really warrant a separate function?

In this particular patch, not really. But since I know how things will evolve :)

- Nuno Sá

> 
> > +}
> > +
> >  static void adp5585_remove_devices(void *dev)
> >  {
> >  	mfd_remove_devices(dev);
> > @@ -157,7 +146,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;
> > @@ -168,8 +157,13 @@ static int adp5585_i2c_probe(struct i2c_client *i2c)
> >  
> >  	i2c_set_clientdata(i2c, adp5585);
> >  
> > -	regmap_config = i2c_get_match_data(i2c);
> > -	adp5585->regmap = devm_regmap_init_i2c(i2c, regmap_config);
> > +	adp5585->variant = (enum
> > adp5585_variant)(uintptr_t)i2c_get_match_data(i2c);
> > +	if (!adp5585->variant)
> > +		return -ENODEV;
> > +
> > +	adp5585_fill_regmap_config(adp5585, &regmap_config);
> > +
> > +	adp5585->regmap = devm_regmap_init_i2c(i2c, &regmap_config);
> >  	if (IS_ERR(adp5585->regmap))
> >  		return dev_err_probe(&i2c->dev, PTR_ERR(adp5585->regmap),
> >  				     "Failed to initialize register
> > map\n");
> > @@ -226,19 +220,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..2813b20e638b6e73ef198e43af07ef29ff
> > 25f273 100644
> > --- a/include/linux/mfd/adp5585.h
> > +++ b/include/linux/mfd/adp5585.h
> > @@ -119,8 +119,18 @@
> >  
> >  struct regmap;
> >  
> > +enum adp5585_variant {
> > +	ADP5585_00 = 1,
> > +	ADP5585_01,
> > +	ADP5585_02,
> > +	ADP5585_03,
> > +	ADP5585_04,
> > +	ADP5585_MAX
> > +};
> > +
> >  struct adp5585_dev {
> >  	struct regmap *regmap;
> > +	enum adp5585_variant variant;
> >  };
> >  
> >  #endif
> > 
> > -- 
> > 2.49.0
> > 
> >