Re: [PATCH v3 07/22] mfd: adp5585: refactor how regmap defaults are handled

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

On Tue, 2025-05-13 at 17:35 +0200, Laurent Pinchart wrote:
> On Tue, May 13, 2025 at 04:00:29PM +0100, Lee Jones wrote:
> > On Mon, 12 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
> > > (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       | 94 +++++++++++++++++++++++++---------------
> > > -----
> > >  include/linux/mfd/adp5585.h | 11 ++++++
> > >  2 files changed, 64 insertions(+), 41 deletions(-)
> > > 
> > > diff --git a/drivers/mfd/adp5585.c b/drivers/mfd/adp5585.c
> > > index
> > > 19d4a0ab1bb4c261e82559630624059529765fbd..874aed7d7cfe052587720d899096c995
> > > c19667af 100644
> > > --- a/drivers/mfd/adp5585.c
> > > +++ b/drivers/mfd/adp5585.c
> > > @@ -81,41 +81,34 @@ 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,
> > > +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 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 int adp5585_fill_regmap_config(const struct adp5585_dev *adp5585,
> > > +				      struct regmap_config
> > > *regmap_config)
> > 
> > I like the general idea.  This is much more scaleable than before.
> > 
> > > +{
> > > +	*regmap_config = adp5585_regmap_config_template;
> > > +
> > > +	switch (adp5585->info->regmap_type) {
> > > +	case ADP5585_REGMAP_00:
> > > +		regmap_config->reg_defaults_raw =
> > > adp5585_regmap_defaults_00;
> > > +		return 0;
> > > +	case ADP5585_REGMAP_02:
> > > +		regmap_config->reg_defaults_raw =
> > > adp5585_regmap_defaults_02;
> > > +		return 0;
> > > +	case ADP5585_REGMAP_04:
> > > +		regmap_config->reg_defaults_raw =
> > > adp5585_regmap_defaults_04;
> > 
> > You could make this read a tiny bit nicer (as you do with the adp5585->info
> > in a later patch) and make reg_defaults_raw a local variable.
> 
> And as ADP585_REGMAP_* is an enum and we have to handle all values, you
> can replace the switch with a static const array lookup.

ack

> 
> > > +		return 0;
> > > +	default:
> > > +		return -ENODEV;
> > > +	}
> > > +}
> > >  
> > >  static int adp5585_parse_fw(struct device *dev, struct adp5585_dev
> > > *adp5585,
> > >  			    struct mfd_cell **devs)
> > > @@ -153,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;
> > >  	struct mfd_cell *devs;
> > >  	unsigned int id;
> > > @@ -165,8 +158,15 @@ 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->info = i2c_get_match_data(i2c);
> > > +	if (!adp5585->info)
> > > +		return -ENODEV;
> > > +
> > > +	ret = adp5585_fill_regmap_config(adp5585, &regmap_config);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	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");
> > > @@ -223,22 +223,34 @@ static int adp5585_resume(struct device *dev)
> > >  
> > >  static DEFINE_SIMPLE_DEV_PM_OPS(adp5585_pm, adp5585_suspend,
> > > adp5585_resume);
> > >  
> > > +static struct adp5585_info adp5585_info = {
> > > +	.regmap_type = ADP5585_REGMAP_00,
> > 
> > Instead of providing this enum, then later another one (id) which is a
> > subset of the same thing, why not pass just ADP5585_REGMAP_00, etc
> > through the DT .data attribute then match on those?  It will add a
> > couple of lines to the switch(info->id) statement, but will save on a
> > boat load of static structs and other complexity.
> > 
> > For instance:
> > 
> > switch (info->id) {
> > 	case ADP5585_MAN_ID_VALUE:
> > 
> > Would simply become:
> > 
> > switch (info->id) {
> > 	case ADP5585_REGMAP_00:
> > 	case ADP5585_REGMAP_02:
> > 	case ADP5585_REGMAP_04:
> > 
> > And that's it.
> > 
> > > +};
> > > +
> > > +static struct adp5585_info adp5585_02_info = {
> > > +	.regmap_type = ADP5585_REGMAP_02,
> > > +};
> > > +
> > > +static struct adp5585_info adp5585_04_info = {
> > > +	.regmap_type = ADP5585_REGMAP_04,
> > > +};
> > > +
> > >  static const struct of_device_id adp5585_of_match[] = {
> > >  	{
> > >  		.compatible = "adi,adp5585-00",
> > > -		.data = &adp5585_regmap_configs[ADP5585_REGMAP_00],
> > > +		.data = &adp5585_info,
> > 
> > 		.data = ADP5585_REGMAP_00,
> > 
> > >  	}, {
> > >  		.compatible = "adi,adp5585-01",
> > > -		.data = &adp5585_regmap_configs[ADP5585_REGMAP_00],
> > > +		.data = &adp5585_info,
> > >  	}, {
> > >  		.compatible = "adi,adp5585-02",
> > > -		.data = &adp5585_regmap_configs[ADP5585_REGMAP_02],
> > > +		.data = &adp5585_02_info,
> > >  	}, {
> > >  		.compatible = "adi,adp5585-03",
> > > -		.data = &adp5585_regmap_configs[ADP5585_REGMAP_00],
> > > +		.data = &adp5585_info,
> > >  	}, {
> > >  		.compatible = "adi,adp5585-04",
> > > -		.data = &adp5585_regmap_configs[ADP5585_REGMAP_04],
> > > +		.data = &adp5585_04_info,
> > >  	},
> > >  	{ /* sentinel */ }
> > >  };
> > > diff --git a/include/linux/mfd/adp5585.h b/include/linux/mfd/adp5585.h
> > > index
> > > 016033cd68e46757aca86d21dd37025fd354b801..4b48614970a811a8a95116faa20e58ea
> > > 4f19ede6 100644
> > > --- a/include/linux/mfd/adp5585.h
> > > +++ b/include/linux/mfd/adp5585.h
> > > @@ -119,8 +119,19 @@
> > >  
> > >  struct regmap;
> > >  
> > > +enum adp5585_regmap_type {
> > > +	ADP5585_REGMAP_00,
> > > +	ADP5585_REGMAP_02,
> > > +	ADP5585_REGMAP_04,
> > > +};
> > > +
> > > +struct adp5585_info {
> > > +	enum adp5585_regmap_type regmap_type;
> > > +};
> > > +
> > >  struct adp5585_dev {
> > >  	struct regmap *regmap;
> > > +	const struct adp5585_info *info;
> > >  };
> > >  
> > >  #endif