| 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, 13 May 2025, 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..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, It goes without saying that the defines would have to become more generic as well, since they will be expanded to describe more than just 'REGMAP'.
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
On Tue, 13 May 2025, Laurent Pinchart wrote: > 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. They can be nice to avoid any switch() matching, but here we have both anyway so it improves very little.
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