| Message ID | 20250512-dev-adp5589-fw-v3-7-092b14b79a88@analog.com |
|---|---|
| State | New |
| Headers | show |
| Series | mfd: adp5585: support keymap events and drop legacy Input driver | expand |
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..874aed7d7cfe052587720d899096c995c19667af 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. > + 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, ®map_config); > + if (ret) > + return ret; > + > + adp5585->regmap = devm_regmap_init_i2c(i2c, ®map_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..4b48614970a811a8a95116faa20e58ea4f19ede6 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 > > -- > 2.49.0 > >
On Tue, 2025-05-13 at 16:00 +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..874aed7d7cfe052587720d899096c995c1 > > 9667af 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. I'm probably missing your point but what would be the benefit? The info is done like that because I wanted the pointer to be 'const'. Here I do not think the same applies... > > > + 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, ®map_config); > > + if (ret) > > + return ret; > > + > > + adp5585->regmap = devm_regmap_init_i2c(i2c, ®map_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. I get the general idea... We will also have to pack the regmap defaults into an array so that we can easily reference it with 'info->id' which I don't like too much tbh (but I do see that adp5585_fill_chip_configs() will become simpler) . I guess I can also just move everything into the "main" struct as we will fill everything during probe (no real reason for struct adp5585_info) Anyways, If you prefer the above I'm not going to argue against 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, I see, needs a cast but should work. I personally prefer valid pointers than "encoding" integers in here. I know we can start the enum at 1 so that we can still look for 0 for any possible issue but... > > > }, { > > .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..4b48614970a811a8a95116faa20e58ea4f > > 19ede6 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 > > > > -- > > 2.49.0 > > > >
On Tue, May 13, 2025 at 04:32:39PM +0100, Nuno Sá wrote: > On Tue, 2025-05-13 at 16:00 +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..874aed7d7cfe052587720d899096c995c1 > > > 9667af 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. > > I'm probably missing your point but what would be the benefit? The info is done > like that because I wanted the pointer to be 'const'. Here I do not think the > same applies... > > > > > > + 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, ®map_config); > > > + if (ret) > > > + return ret; > > > + > > > + adp5585->regmap = devm_regmap_init_i2c(i2c, ®map_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. > > I get the general idea... We will also have to pack the regmap defaults into an > array so that we can easily reference it with 'info->id' which I don't like too > much tbh (but I do see that adp5585_fill_chip_configs() will become simpler) . I > guess I can also just move everything into the "main" struct as we will fill > everything during probe (no real reason for struct adp5585_info) > > Anyways, If you prefer the above I'm not going to argue against 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, > > I see, needs a cast but should work. I personally prefer valid pointers than > "encoding" integers in here. I know we can start the enum at 1 so that we can > still look for 0 for any possible issue but... I also prefer pointers to structures, but won't fight for it. > > > }, { > > > .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..4b48614970a811a8a95116faa20e58ea4f > > > 19ede6 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
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, ®map_config); > > > + if (ret) > > > + return ret; > > > + > > > + adp5585->regmap = devm_regmap_init_i2c(i2c, ®map_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
diff --git a/drivers/mfd/adp5585.c b/drivers/mfd/adp5585.c index 19d4a0ab1bb4c261e82559630624059529765fbd..874aed7d7cfe052587720d899096c995c19667af 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) +{ + *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; + 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, ®map_config); + if (ret) + return ret; + + adp5585->regmap = devm_regmap_init_i2c(i2c, ®map_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, +}; + +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, }, { .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..4b48614970a811a8a95116faa20e58ea4f19ede6 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