| Message ID | 20250415-dev-adp5589-fw-v2-6-3a799c3ed812@analog.com |
|---|---|
| State | New |
| Headers | show |
| Series | mfd: adp5585: support keymap events and drop legacy Input driver | expand |
Hi Nuno, On Mon, Apr 21, 2025 at 01:21:08PM +0100, Nuno Sá wrote: > On Mon, 2025-04-21 at 12:15 +0300, Laurent Pinchart wrote: > > On Tue, Apr 15, 2025 at 03:49:22PM +0100, Nuno Sá via B4 Relay wrote: > > > From: Nuno Sá <nuno.sa@analog.com> > > > > > > The ADP5589 is a 19 I/O port expander with built-in keypad matrix decoder, > > > programmable logic, reset generator, and PWM generator. > > > > > > This patch adds the foundation to add support for the adp5589 gpio and pwm > > > drivers. Most importantly, we need to differentiate between some > > > registers addresses. It also hints to future keymap support. > > > > Please split this in two patches, one that reworks the driver to support > > different register addresses, and one that adds adp5589 support. > > > > > Signed-off-by: Nuno Sá <nuno.sa@analog.com> > > > --- > > > drivers/mfd/adp5585.c | 223 +++++++++++++++++++++++++++++++++++++++++--- > > > include/linux/mfd/adp5585.h | 57 ++++++++++- > > > 2 files changed, 268 insertions(+), 12 deletions(-) > > > > > > diff --git a/drivers/mfd/adp5585.c b/drivers/mfd/adp5585.c > > > index fafe3ad93ea196e1eb8e79fecba58f36f12167eb..c3586c0d6aa2e7e7d94667993410610be7fc3672 100644 > > > --- a/drivers/mfd/adp5585.c > > > +++ b/drivers/mfd/adp5585.c > > > @@ -25,6 +25,13 @@ static const struct mfd_cell adp5585_devs[] = { > > > > > > }; > > > > > > +static const struct mfd_cell adp5589_devs[] = { > > > + MFD_CELL_NAME("adp5589-keys"), > > > + MFD_CELL_NAME("adp5589-gpio"), > > > + MFD_CELL_NAME("adp5589-pwm"), > > > + > > > +}; > > > + > > > static const struct regmap_range adp5585_volatile_ranges[] = { > > > regmap_reg_range(ADP5585_ID, ADP5585_GPI_STATUS_B), > > > }; > > > @@ -34,6 +41,15 @@ static const struct regmap_access_table adp5585_volatile_regs = { > > > .n_yes_ranges = ARRAY_SIZE(adp5585_volatile_ranges), > > > }; > > > > > > +static const struct regmap_range adp5589_volatile_ranges[] = { > > > + regmap_reg_range(ADP5585_ID, ADP5589_GPI_STATUS_C), > > > +}; > > > + > > > +static const struct regmap_access_table adp5589_volatile_regs = { > > > + .yes_ranges = adp5589_volatile_ranges, > > > + .n_yes_ranges = ARRAY_SIZE(adp5589_volatile_ranges), > > > +}; > > > + > > > /* > > > * Chip variants differ in the default configuration of pull-up and pull-down > > > * resistors, and therefore have different default register values: > > > @@ -77,10 +93,52 @@ static const u8 adp5585_regmap_defaults_04[ADP5585_MAX_REG + 1] = { > > > /* 0x38 */ 0x00, 0x00, 0x00, 0x00, 0x00, > > > }; > > > > > > +static const u8 adp5589_regmap_defaults_00[ADP5589_MAX_REG + 1] = { > > > + /* 0x00 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > + /* 0x08 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > + /* 0x10 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > + /* 0x18 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > + /* 0x20 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > + /* 0x28 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > + /* 0x30 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > + /* 0x38 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > + /* 0x40 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > + /* 0x48 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > +}; > > > + > > > +static const u8 adp5589_regmap_defaults_01[ADP5589_MAX_REG + 1] = { > > > + /* 0x00 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > + /* 0x08 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > + /* 0x10 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > + /* 0x18 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > + /* 0x20 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > + /* 0x28 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > + /* 0x30 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > + /* 0x38 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x20, 0x00, 0x00, > > > + /* 0x40 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > + /* 0x48 */ 0x00, 0x00, 0x00, 0x00, 0x20, 0x00, 0x00, > > > +}; > > > + > > > +static const u8 adp5589_regmap_defaults_02[ADP5589_MAX_REG + 1] = { > > > + /* 0x00 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > + /* 0x08 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > + /* 0x10 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > + /* 0x18 */ 0x00, 0x41, 0x01, 0x00, 0x11, 0x04, 0x00, 0x00, > > > + /* 0x20 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > + /* 0x28 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > + /* 0x30 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > + /* 0x38 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > + /* 0x40 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > + /* 0x48 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > +}; > > > + > > > enum adp5585_regmap_type { > > > ADP5585_REGMAP_00, > > > ADP5585_REGMAP_02, > > > ADP5585_REGMAP_04, > > > + ADP5589_REGMAP_00, > > > + ADP5589_REGMAP_01, > > > + ADP5589_REGMAP_02, > > > }; > > > > > > static const struct regmap_config adp5585_regmap_configs[] = { > > > @@ -111,6 +169,131 @@ static const struct regmap_config adp5585_regmap_configs[] = { > > > .reg_defaults_raw = adp5585_regmap_defaults_04, > > > .num_reg_defaults_raw = sizeof(adp5585_regmap_defaults_04), > > > }, > > > + [ADP5589_REGMAP_00] = { > > > + .reg_bits = 8, > > > + .val_bits = 8, > > > + .max_register = ADP5589_MAX_REG, > > > + .volatile_table = &adp5589_volatile_regs, > > > + .cache_type = REGCACHE_MAPLE, > > > + .reg_defaults_raw = adp5589_regmap_defaults_00, > > > + .num_reg_defaults_raw = sizeof(adp5589_regmap_defaults_00), > > > + }, > > > + [ADP5589_REGMAP_01] = { > > > + .reg_bits = 8, > > > + .val_bits = 8, > > > + .max_register = ADP5589_MAX_REG, > > > + .volatile_table = &adp5589_volatile_regs, > > > + .cache_type = REGCACHE_MAPLE, > > > + .reg_defaults_raw = adp5589_regmap_defaults_01, > > > + .num_reg_defaults_raw = sizeof(adp5589_regmap_defaults_01), > > > + }, > > > + [ADP5589_REGMAP_02] = { > > > + .reg_bits = 8, > > > + .val_bits = 8, > > > + .max_register = ADP5589_MAX_REG, > > > + .volatile_table = &adp5589_volatile_regs, > > > + .cache_type = REGCACHE_MAPLE, > > > + .reg_defaults_raw = adp5589_regmap_defaults_02, > > > + .num_reg_defaults_raw = sizeof(adp5589_regmap_defaults_02), > > > + }, > > > +}; > > > + > > > +static const struct adp5585_regs adp5585_regs = { > > > + .debounce_dis_a = ADP5585_DEBOUNCE_DIS_A, > > > + .rpull_cfg_a = ADP5585_RPULL_CONFIG_A, > > > + .gpo_data_a = ADP5585_GPO_DATA_OUT_A, > > > + .gpo_out_a = ADP5585_GPO_OUT_MODE_A, > > > + .gpio_dir_a = ADP5585_GPIO_DIRECTION_A, > > > + .gpi_stat_a = ADP5585_GPI_STATUS_A, > > > + .pwm_cfg = ADP5585_PWM_CFG, > > > + .pwm_offt_low = ADP5585_PWM_OFFT_LOW, > > > + .pwm_ont_low = ADP5585_PWM_ONT_LOW, > > > + .gen_cfg = ADP5585_GENERAL_CFG, > > > + .ext_cfg = ADP5585_PIN_CONFIG_C, > > > +}; > > > > Why does this need to be stored in this driver, and not in the drivers > > for the gpio and pwm cells ? If the kernel is compiled without e.g. the > > adp5585-pwm driver, we shouldn't waste memory here by adding data that > > only the adp5585-pwm driver needs. > > I don't really think the memory we would save to be that relevant but I can > better separate things. I guess i went like this because there's some shared > variables that will have to be in the top level structs and I did not wanted to > have a "global" and "local" regs thingy... I understand, and I think it's at least partly a coding style preference. Personally, I find that having child-specific data in child drivers makes the code easier to read, as it increases locality. Otherwise, I have to look through multiple child drivers to see if and where each field is used. > > > + > > > +static const struct adp5585_regs adp5589_regs = { > > > + .debounce_dis_a = ADP5589_DEBOUNCE_DIS_A, > > > + .rpull_cfg_a = ADP5589_RPULL_CONFIG_A, > > > + .gpo_data_a = ADP5589_GPO_DATA_OUT_A, > > > + .gpo_out_a = ADP5589_GPO_OUT_MODE_A, > > > + .gpio_dir_a = ADP5589_GPIO_DIRECTION_A, > > > + .gpi_stat_a = ADP5589_GPI_STATUS_A, > > > + .pwm_cfg = ADP5589_PWM_CFG, > > > + .pwm_offt_low = ADP5589_PWM_OFFT_LOW, > > > + .pwm_ont_low = ADP5589_PWM_ONT_LOW, > > > + .gen_cfg = ADP5589_GENERAL_CFG, > > > + .ext_cfg = ADP5589_PIN_CONFIG_D, > > > +}; > > > + > > > +static const struct adp5585_info adp5585_info = { > > > + .adp5585_devs = adp5585_devs, > > > + .regmap_config = &adp5585_regmap_configs[ADP5585_REGMAP_00], > > > + .n_devs = ARRAY_SIZE(adp5585_devs), > > > + .id = ADP5585_MAN_ID_VALUE, > > > + .regs = &adp5585_regs, > > > + .max_rows = ADP5585_MAX_ROW_NUM, > > > + .max_cols = ADP5585_MAX_COL_NUM, > > > > Same here, the max_rows and max_cols fields don't seem to belong to this > > driver. > > > > > +}; > > > + > > > +static const struct adp5585_info adp5585_01_info = { > > > + .adp5585_devs = adp5585_devs, > > > + .regmap_config = &adp5585_regmap_configs[ADP5585_REGMAP_00], > > > + .n_devs = ARRAY_SIZE(adp5585_devs), > > > + .id = ADP5585_MAN_ID_VALUE, > > > + .regs = &adp5585_regs, > > > + .max_rows = ADP5585_MAX_ROW_NUM, > > > + .max_cols = ADP5585_MAX_COL_NUM, > > > +}; > > > + > > > +static const struct adp5585_info adp5585_02_info = { > > > + .adp5585_devs = adp5585_devs, > > > + .regmap_config = &adp5585_regmap_configs[ADP5585_REGMAP_02], > > > + .n_devs = ARRAY_SIZE(adp5585_devs), > > > + .id = ADP5585_MAN_ID_VALUE, > > > + .regs = &adp5585_regs, > > > + .max_rows = ADP5585_MAX_ROW_NUM, > > > + .max_cols = ADP5585_MAX_COL_NUM, > > > +}; > > > + > > > +static const struct adp5585_info adp5585_04_info = { > > > + .adp5585_devs = adp5585_devs, > > > + .regmap_config = &adp5585_regmap_configs[ADP5585_REGMAP_04], > > > + .n_devs = ARRAY_SIZE(adp5585_devs), > > > + .id = ADP5585_MAN_ID_VALUE, > > > + .regs = &adp5585_regs, > > > + .max_rows = ADP5585_MAX_ROW_NUM, > > > + .max_cols = ADP5585_MAX_COL_NUM, > > > +}; > > > + > > > +static const struct adp5585_info adp5589_info = { > > > + .adp5585_devs = adp5589_devs, > > > + .regmap_config = &adp5585_regmap_configs[ADP5589_REGMAP_00], > > > + .n_devs = ARRAY_SIZE(adp5589_devs), > > > + .id = ADP5589_MAN_ID_VALUE, > > > + .regs = &adp5589_regs, > > > + .max_rows = ADP5589_MAX_ROW_NUM, > > > + .max_cols = ADP5589_MAX_COL_NUM, > > > +}; > > > + > > > +static const struct adp5585_info adp5589_01_info = { > > > + .adp5585_devs = adp5589_devs, > > > + .regmap_config = &adp5585_regmap_configs[ADP5589_REGMAP_01], > > > + .n_devs = ARRAY_SIZE(adp5589_devs), > > > + .id = ADP5589_MAN_ID_VALUE, > > > + .regs = &adp5589_regs, > > > + .max_rows = ADP5589_MAX_ROW_NUM, > > > + .max_cols = ADP5589_MAX_COL_NUM, > > > +}; > > > + > > > +static const struct adp5585_info adp5589_02_info = { > > > + .adp5585_devs = adp5589_devs, > > > + .regmap_config = &adp5585_regmap_configs[ADP5589_REGMAP_02], > > > + .n_devs = ARRAY_SIZE(adp5589_devs), > > > + .id = ADP5589_MAN_ID_VALUE, > > > + .regs = &adp5589_regs, > > > + .max_rows = ADP5589_MAX_ROW_NUM, > > > + .max_cols = ADP5589_MAX_COL_NUM, > > > }; > > > > > > static void adp5585_osc_disable(void *data) > > > @@ -122,7 +305,7 @@ static void adp5585_osc_disable(void *data) > > > > > > static int adp5585_i2c_probe(struct i2c_client *i2c) > > > { > > > - const struct regmap_config *regmap_config; > > > + const struct adp5585_info *info; > > > struct adp5585_dev *adp5585; > > > unsigned int id; > > > int ret; > > > @@ -133,8 +316,13 @@ static int adp5585_i2c_probe(struct i2c_client *i2c) > > > > > > i2c_set_clientdata(i2c, adp5585); > > > > > > - regmap_config = i2c_get_match_data(i2c); > > > - adp5585->regmap = devm_regmap_init_i2c(i2c, regmap_config); > > > + info = i2c_get_match_data(i2c); > > > + if (!info) > > > + return -ENODEV; > > > > Can this happen ? > > > > > + > > > + adp5585->info = info; > > > > Drop the local info variable and assign the value to adp5585->info > > directly. > > > > > + > > > + adp5585->regmap = devm_regmap_init_i2c(i2c, info->regmap_config); > > > if (IS_ERR(adp5585->regmap)) > > > return dev_err_probe(&i2c->dev, PTR_ERR(adp5585->regmap), > > > "Failed to initialize register > > > map\n"); > > > @@ -144,7 +332,8 @@ static int adp5585_i2c_probe(struct i2c_client *i2c) > > > return dev_err_probe(&i2c->dev, ret, > > > "Failed to read device ID\n"); > > > > > > - if ((id & ADP5585_MAN_ID_MASK) != ADP5585_MAN_ID_VALUE) > > > + id &= ADP5585_MAN_ID_MASK; > > > + if (id != adp5585->info->id) > > > return dev_err_probe(&i2c->dev, -ENODEV, > > > "Invalid device ID 0x%02x\n", id); > > > > > > @@ -158,8 +347,8 @@ static int adp5585_i2c_probe(struct i2c_client *i2c) > > > return ret; > > > > > > ret = devm_mfd_add_devices(&i2c->dev, PLATFORM_DEVID_AUTO, > > > - adp5585_devs, ARRAY_SIZE(adp5585_devs), > > > - NULL, 0, NULL); > > > + adp5585->info->adp5585_devs, > > > + adp5585->info->n_devs, NULL, 0, NULL); > > > if (ret) > > > return dev_err_probe(&i2c->dev, ret, > > > "Failed to add child devices\n"); > > > @@ -191,19 +380,31 @@ static DEFINE_SIMPLE_DEV_PM_OPS(adp5585_pm, > > > adp5585_suspend, adp5585_resume); > > > static const struct of_device_id adp5585_of_match[] = { > > > { > > > .compatible = "adi,adp5585-00", > > > - .data = &adp5585_regmap_configs[ADP5585_REGMAP_00], > > > + .data = &adp5585_info, > > > }, { > > > .compatible = "adi,adp5585-01", > > > - .data = &adp5585_regmap_configs[ADP5585_REGMAP_00], > > > + .data = &adp5585_01_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, > > > + }, { > > > + .compatible = "adi,adp5589-00", > > > + .data = &adp5589_info, > > > + }, { > > > + .compatible = "adi,adp5589-01", > > > + .data = &adp5589_01_info, > > > + }, { > > > + .compatible = "adi,adp5589-02", > > > + .data = &adp5589_02_info, > > > + }, { > > > + .compatible = "adi,adp5589", > > > + .data = &adp5589_info, > > > }, > > > { /* sentinel */ } > > > }; > > > diff --git a/include/linux/mfd/adp5585.h b/include/linux/mfd/adp5585.h > > > index > > > 016033cd68e46757aca86d21dd37025fd354b801..dffe1449de01dacf8fe78cf0e87d1f176d > > > 11f620 100644 > > > --- a/include/linux/mfd/adp5585.h > > > +++ b/include/linux/mfd/adp5585.h > > > @@ -104,9 +104,11 @@ > > > #define ADP5585_INT_CFG BIT(1) > > > #define ADP5585_RST_CFG BIT(0) > > > #define ADP5585_INT_EN 0x3c > > > - > > > #define ADP5585_MAX_REG ADP5585_INT_EN > > > > > > +#define ADP5585_MAX_ROW_NUM 6 > > > +#define ADP5585_MAX_COL_NUM 5 > > > + > > > /* > > > * Bank 0 covers pins "GPIO 1/R0" to "GPIO 6/R5", numbered 0 to 5 by the > > > * driver, and bank 1 covers pins "GPIO 7/C0" to "GPIO 11/C4", numbered 6 > > > to > > > @@ -117,10 +119,63 @@ > > > #define ADP5585_BANK(n) ((n) >= 6 ? 1 : 0) > > > #define ADP5585_BIT(n) ((n) >= 6 ? BIT((n) - 6) : BIT(n)) > > > > > > +/* ADP5589 */ > > > +#define ADP5589_MAN_ID_VALUE 0x10 > > > +#define ADP5589_GPI_STATUS_A 0x16 > > > +#define ADP5589_GPI_STATUS_C 0x18 > > > +#define ADP5589_RPULL_CONFIG_A 0x19 > > > +#define ADP5589_DEBOUNCE_DIS_A 0x27 > > > +#define ADP5589_GPO_DATA_OUT_A 0x2a > > > +#define ADP5589_GPO_OUT_MODE_A 0x2d > > > +#define ADP5589_GPIO_DIRECTION_A 0x30 > > > > Indentation looks wrong. > > > > > +#define ADP5589_PWM_OFFT_LOW 0x3e > > > +#define ADP5589_PWM_ONT_LOW 0x40 > > > +#define ADP5589_PWM_CFG 0x42 > > > +#define ADP5589_PIN_CONFIG_D 0x4C > > > +#define ADP5589_GENERAL_CFG 0x4d > > > +#define ADP5589_INT_EN 0x4e > > > +#define ADP5589_MAX_REG ADP5589_INT_EN > > > + > > > +#define ADP5589_MAX_ROW_NUM 8 > > > +#define ADP5589_MAX_COL_NUM 11 > > > + > > > +/* > > > + * Bank 0 covers pins "GPIO 1/R0" to "GPIO 8/R7", numbered 0 to 7 by the > > > + * driver, bank 1 covers pins "GPIO 9/C0" to "GPIO 16/C7", numbered 8 to > > > + * 15 and bank 3 covers pins "GPIO 17/C8" to "GPIO 19/C10", numbered 16 to > > > 18. > > > + */ > > > +#define ADP5589_BANK(n) ((n) >> 3) > > > +#define ADP5589_BIT(n) BIT((n) & 0x7) > > > + > > > +struct adp5585_regs { > > > + unsigned int debounce_dis_a; > > > + unsigned int rpull_cfg_a; > > > + unsigned int gpo_data_a; > > > + unsigned int gpo_out_a; > > > + unsigned int gpio_dir_a; > > > + unsigned int gpi_stat_a; > > > + unsigned int pwm_cfg; > > > + unsigned int pwm_offt_low; > > > + unsigned int pwm_ont_low; > > > + unsigned int gen_cfg; > > > + unsigned int ext_cfg; > > > +}; > > > + > > > +struct adp5585_info { > > > + const struct mfd_cell *adp5585_devs; > > > + const struct regmap_config *regmap_config; > > > + const struct adp5585_regs *regs; > > > + unsigned int n_devs; > > > + unsigned int id; > > > + u8 max_rows; > > > + u8 max_cols; > > > +}; > > > + > > > struct regmap; > > > > > > struct adp5585_dev { > > > struct regmap *regmap; > > > + const struct adp5585_info *info; > > > }; > > > > > > #endif
On Tue, 2025-04-22 at 01:06 +0300, Laurent Pinchart wrote: > Hi Nuno, > > On Mon, Apr 21, 2025 at 01:21:08PM +0100, Nuno Sá wrote: > > On Mon, 2025-04-21 at 12:15 +0300, Laurent Pinchart wrote: > > > On Tue, Apr 15, 2025 at 03:49:22PM +0100, Nuno Sá via B4 Relay wrote: > > > > From: Nuno Sá <nuno.sa@analog.com> > > > > > > > > The ADP5589 is a 19 I/O port expander with built-in keypad matrix > > > > decoder, > > > > programmable logic, reset generator, and PWM generator. > > > > > > > > This patch adds the foundation to add support for the adp5589 gpio and > > > > pwm > > > > drivers. Most importantly, we need to differentiate between some > > > > registers addresses. It also hints to future keymap support. > > > > > > Please split this in two patches, one that reworks the driver to support > > > different register addresses, and one that adds adp5589 support. > > > > > > > Signed-off-by: Nuno Sá <nuno.sa@analog.com> > > > > --- > > > > drivers/mfd/adp5585.c | 223 > > > > +++++++++++++++++++++++++++++++++++++++++--- > > > > include/linux/mfd/adp5585.h | 57 ++++++++++- > > > > 2 files changed, 268 insertions(+), 12 deletions(-) > > > > > > > > diff --git a/drivers/mfd/adp5585.c b/drivers/mfd/adp5585.c > > > > index > > > > fafe3ad93ea196e1eb8e79fecba58f36f12167eb..c3586c0d6aa2e7e7d9466799341061 > > > > 0be7fc3672 100644 > > > > --- a/drivers/mfd/adp5585.c > > > > +++ b/drivers/mfd/adp5585.c > > > > @@ -25,6 +25,13 @@ static const struct mfd_cell adp5585_devs[] = { > > > > > > > > }; > > > > > > > > +static const struct mfd_cell adp5589_devs[] = { > > > > + MFD_CELL_NAME("adp5589-keys"), > > > > + MFD_CELL_NAME("adp5589-gpio"), > > > > + MFD_CELL_NAME("adp5589-pwm"), > > > > + > > > > +}; > > > > + > > > > static const struct regmap_range adp5585_volatile_ranges[] = { > > > > regmap_reg_range(ADP5585_ID, ADP5585_GPI_STATUS_B), > > > > }; > > > > @@ -34,6 +41,15 @@ static const struct regmap_access_table > > > > adp5585_volatile_regs = { > > > > .n_yes_ranges = ARRAY_SIZE(adp5585_volatile_ranges), > > > > }; > > > > > > > > +static const struct regmap_range adp5589_volatile_ranges[] = { > > > > + regmap_reg_range(ADP5585_ID, ADP5589_GPI_STATUS_C), > > > > +}; > > > > + > > > > +static const struct regmap_access_table adp5589_volatile_regs = { > > > > + .yes_ranges = adp5589_volatile_ranges, > > > > + .n_yes_ranges = ARRAY_SIZE(adp5589_volatile_ranges), > > > > +}; > > > > + > > > > /* > > > > * Chip variants differ in the default configuration of pull-up and > > > > pull-down > > > > * resistors, and therefore have different default register values: > > > > @@ -77,10 +93,52 @@ static const u8 > > > > adp5585_regmap_defaults_04[ADP5585_MAX_REG + 1] = { > > > > /* 0x38 */ 0x00, 0x00, 0x00, 0x00, 0x00, > > > > }; > > > > > > > > +static const u8 adp5589_regmap_defaults_00[ADP5589_MAX_REG + 1] = { > > > > + /* 0x00 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > > + /* 0x08 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > > + /* 0x10 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > > + /* 0x18 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > > + /* 0x20 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > > + /* 0x28 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > > + /* 0x30 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > > + /* 0x38 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > > + /* 0x40 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > > + /* 0x48 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > > +}; > > > > + > > > > +static const u8 adp5589_regmap_defaults_01[ADP5589_MAX_REG + 1] = { > > > > + /* 0x00 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > > + /* 0x08 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > > + /* 0x10 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > > + /* 0x18 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > > + /* 0x20 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > > + /* 0x28 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > > + /* 0x30 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > > + /* 0x38 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x20, 0x00, 0x00, > > > > + /* 0x40 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > > + /* 0x48 */ 0x00, 0x00, 0x00, 0x00, 0x20, 0x00, 0x00, > > > > +}; > > > > + > > > > +static const u8 adp5589_regmap_defaults_02[ADP5589_MAX_REG + 1] = { > > > > + /* 0x00 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > > + /* 0x08 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > > + /* 0x10 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > > + /* 0x18 */ 0x00, 0x41, 0x01, 0x00, 0x11, 0x04, 0x00, 0x00, > > > > + /* 0x20 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > > + /* 0x28 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > > + /* 0x30 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > > + /* 0x38 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > > + /* 0x40 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > > + /* 0x48 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > > +}; > > > > + > > > > enum adp5585_regmap_type { > > > > ADP5585_REGMAP_00, > > > > ADP5585_REGMAP_02, > > > > ADP5585_REGMAP_04, > > > > + ADP5589_REGMAP_00, > > > > + ADP5589_REGMAP_01, > > > > + ADP5589_REGMAP_02, > > > > }; > > > > > > > > static const struct regmap_config adp5585_regmap_configs[] = { > > > > @@ -111,6 +169,131 @@ static const struct regmap_config > > > > adp5585_regmap_configs[] = { > > > > .reg_defaults_raw = adp5585_regmap_defaults_04, > > > > .num_reg_defaults_raw = > > > > sizeof(adp5585_regmap_defaults_04), > > > > }, > > > > + [ADP5589_REGMAP_00] = { > > > > + .reg_bits = 8, > > > > + .val_bits = 8, > > > > + .max_register = ADP5589_MAX_REG, > > > > + .volatile_table = &adp5589_volatile_regs, > > > > + .cache_type = REGCACHE_MAPLE, > > > > + .reg_defaults_raw = adp5589_regmap_defaults_00, > > > > + .num_reg_defaults_raw = > > > > sizeof(adp5589_regmap_defaults_00), > > > > + }, > > > > + [ADP5589_REGMAP_01] = { > > > > + .reg_bits = 8, > > > > + .val_bits = 8, > > > > + .max_register = ADP5589_MAX_REG, > > > > + .volatile_table = &adp5589_volatile_regs, > > > > + .cache_type = REGCACHE_MAPLE, > > > > + .reg_defaults_raw = adp5589_regmap_defaults_01, > > > > + .num_reg_defaults_raw = > > > > sizeof(adp5589_regmap_defaults_01), > > > > + }, > > > > + [ADP5589_REGMAP_02] = { > > > > + .reg_bits = 8, > > > > + .val_bits = 8, > > > > + .max_register = ADP5589_MAX_REG, > > > > + .volatile_table = &adp5589_volatile_regs, > > > > + .cache_type = REGCACHE_MAPLE, > > > > + .reg_defaults_raw = adp5589_regmap_defaults_02, > > > > + .num_reg_defaults_raw = > > > > sizeof(adp5589_regmap_defaults_02), > > > > + }, > > > > +}; > > > > + > > > > +static const struct adp5585_regs adp5585_regs = { > > > > + .debounce_dis_a = ADP5585_DEBOUNCE_DIS_A, > > > > + .rpull_cfg_a = ADP5585_RPULL_CONFIG_A, > > > > + .gpo_data_a = ADP5585_GPO_DATA_OUT_A, > > > > + .gpo_out_a = ADP5585_GPO_OUT_MODE_A, > > > > + .gpio_dir_a = ADP5585_GPIO_DIRECTION_A, > > > > + .gpi_stat_a = ADP5585_GPI_STATUS_A, > > > > + .pwm_cfg = ADP5585_PWM_CFG, > > > > + .pwm_offt_low = ADP5585_PWM_OFFT_LOW, > > > > + .pwm_ont_low = ADP5585_PWM_ONT_LOW, > > > > + .gen_cfg = ADP5585_GENERAL_CFG, > > > > + .ext_cfg = ADP5585_PIN_CONFIG_C, > > > > +}; > > > > > > Why does this need to be stored in this driver, and not in the drivers > > > for the gpio and pwm cells ? If the kernel is compiled without e.g. the > > > adp5585-pwm driver, we shouldn't waste memory here by adding data that > > > only the adp5585-pwm driver needs. > > > > I don't really think the memory we would save to be that relevant but I can > > better separate things. I guess i went like this because there's some shared > > variables that will have to be in the top level structs and I did not wanted > > to > > have a "global" and "local" regs thingy... > > I understand, and I think it's at least partly a coding style > preference. Personally, I find that having child-specific data in child > drivers makes the code easier to read, as it increases locality. > Otherwise, I have to look through multiple child drivers to see if and > where each field is used. No strong feelings on my side. So, I'll give this a shot in v3 > > > > > + > > > > +static const struct adp5585_regs adp5589_regs = { > > > > + .debounce_dis_a = ADP5589_DEBOUNCE_DIS_A, > > > > + .rpull_cfg_a = ADP5589_RPULL_CONFIG_A, > > > > + .gpo_data_a = ADP5589_GPO_DATA_OUT_A, > > > > + .gpo_out_a = ADP5589_GPO_OUT_MODE_A, > > > > + .gpio_dir_a = ADP5589_GPIO_DIRECTION_A, > > > > + .gpi_stat_a = ADP5589_GPI_STATUS_A, > > > > + .pwm_cfg = ADP5589_PWM_CFG, > > > > + .pwm_offt_low = ADP5589_PWM_OFFT_LOW, > > > > + .pwm_ont_low = ADP5589_PWM_ONT_LOW, > > > > + .gen_cfg = ADP5589_GENERAL_CFG, > > > > + .ext_cfg = ADP5589_PIN_CONFIG_D, > > > > +}; > > > > + > > > > +static const struct adp5585_info adp5585_info = { > > > > + .adp5585_devs = adp5585_devs, > > > > + .regmap_config = &adp5585_regmap_configs[ADP5585_REGMAP_00], > > > > + .n_devs = ARRAY_SIZE(adp5585_devs), > > > > + .id = ADP5585_MAN_ID_VALUE, > > > > + .regs = &adp5585_regs, > > > > + .max_rows = ADP5585_MAX_ROW_NUM, > > > > + .max_cols = ADP5585_MAX_COL_NUM, > > > > > > Same here, the max_rows and max_cols fields don't seem to belong to this > > > driver. > > > > > > > +}; > > > > + > > > > +static const struct adp5585_info adp5585_01_info = { > > > > + .adp5585_devs = adp5585_devs, > > > > + .regmap_config = &adp5585_regmap_configs[ADP5585_REGMAP_00], > > > > + .n_devs = ARRAY_SIZE(adp5585_devs), > > > > + .id = ADP5585_MAN_ID_VALUE, > > > > + .regs = &adp5585_regs, > > > > + .max_rows = ADP5585_MAX_ROW_NUM, > > > > + .max_cols = ADP5585_MAX_COL_NUM, > > > > +}; > > > > + > > > > +static const struct adp5585_info adp5585_02_info = { > > > > + .adp5585_devs = adp5585_devs, > > > > + .regmap_config = &adp5585_regmap_configs[ADP5585_REGMAP_02], > > > > + .n_devs = ARRAY_SIZE(adp5585_devs), > > > > + .id = ADP5585_MAN_ID_VALUE, > > > > + .regs = &adp5585_regs, > > > > + .max_rows = ADP5585_MAX_ROW_NUM, > > > > + .max_cols = ADP5585_MAX_COL_NUM, > > > > +}; > > > > + > > > > +static const struct adp5585_info adp5585_04_info = { > > > > + .adp5585_devs = adp5585_devs, > > > > + .regmap_config = &adp5585_regmap_configs[ADP5585_REGMAP_04], > > > > + .n_devs = ARRAY_SIZE(adp5585_devs), > > > > + .id = ADP5585_MAN_ID_VALUE, > > > > + .regs = &adp5585_regs, > > > > + .max_rows = ADP5585_MAX_ROW_NUM, > > > > + .max_cols = ADP5585_MAX_COL_NUM, > > > > +}; > > > > + > > > > +static const struct adp5585_info adp5589_info = { > > > > + .adp5585_devs = adp5589_devs, > > > > + .regmap_config = &adp5585_regmap_configs[ADP5589_REGMAP_00], > > > > + .n_devs = ARRAY_SIZE(adp5589_devs), > > > > + .id = ADP5589_MAN_ID_VALUE, > > > > + .regs = &adp5589_regs, > > > > + .max_rows = ADP5589_MAX_ROW_NUM, > > > > + .max_cols = ADP5589_MAX_COL_NUM, > > > > +}; > > > > + > > > > +static const struct adp5585_info adp5589_01_info = { > > > > + .adp5585_devs = adp5589_devs, > > > > + .regmap_config = &adp5585_regmap_configs[ADP5589_REGMAP_01], > > > > + .n_devs = ARRAY_SIZE(adp5589_devs), > > > > + .id = ADP5589_MAN_ID_VALUE, > > > > + .regs = &adp5589_regs, > > > > + .max_rows = ADP5589_MAX_ROW_NUM, > > > > + .max_cols = ADP5589_MAX_COL_NUM, > > > > +}; > > > > + > > > > +static const struct adp5585_info adp5589_02_info = { > > > > + .adp5585_devs = adp5589_devs, > > > > + .regmap_config = &adp5585_regmap_configs[ADP5589_REGMAP_02], > > > > + .n_devs = ARRAY_SIZE(adp5589_devs), > > > > + .id = ADP5589_MAN_ID_VALUE, > > > > + .regs = &adp5589_regs, > > > > + .max_rows = ADP5589_MAX_ROW_NUM, > > > > + .max_cols = ADP5589_MAX_COL_NUM, > > > > }; > > > > > > > > static void adp5585_osc_disable(void *data) > > > > @@ -122,7 +305,7 @@ static void adp5585_osc_disable(void *data) > > > > > > > > static int adp5585_i2c_probe(struct i2c_client *i2c) > > > > { > > > > - const struct regmap_config *regmap_config; > > > > + const struct adp5585_info *info; > > > > struct adp5585_dev *adp5585; > > > > unsigned int id; > > > > int ret; > > > > @@ -133,8 +316,13 @@ static int adp5585_i2c_probe(struct i2c_client > > > > *i2c) > > > > > > > > i2c_set_clientdata(i2c, adp5585); > > > > > > > > - regmap_config = i2c_get_match_data(i2c); > > > > - adp5585->regmap = devm_regmap_init_i2c(i2c, regmap_config); > > > > + info = i2c_get_match_data(i2c); > > > > + if (!info) > > > > + return -ENODEV; > > > > > > Can this happen ? > > > > > > > + > > > > + adp5585->info = info; > > > > > > Drop the local info variable and assign the value to adp5585->info > > > directly. > > > > > > > + > > > > + adp5585->regmap = devm_regmap_init_i2c(i2c, info- > > > > >regmap_config); > > > > if (IS_ERR(adp5585->regmap)) > > > > return dev_err_probe(&i2c->dev, PTR_ERR(adp5585- > > > > >regmap), > > > > "Failed to initialize register > > > > map\n"); > > > > @@ -144,7 +332,8 @@ static int adp5585_i2c_probe(struct i2c_client *i2c) > > > > return dev_err_probe(&i2c->dev, ret, > > > > "Failed to read device ID\n"); > > > > > > > > - if ((id & ADP5585_MAN_ID_MASK) != ADP5585_MAN_ID_VALUE) > > > > + id &= ADP5585_MAN_ID_MASK; > > > > + if (id != adp5585->info->id) > > > > return dev_err_probe(&i2c->dev, -ENODEV, > > > > "Invalid device ID 0x%02x\n", id); > > > > > > > > @@ -158,8 +347,8 @@ static int adp5585_i2c_probe(struct i2c_client *i2c) > > > > return ret; > > > > > > > > ret = devm_mfd_add_devices(&i2c->dev, PLATFORM_DEVID_AUTO, > > > > - adp5585_devs, > > > > ARRAY_SIZE(adp5585_devs), > > > > - NULL, 0, NULL); > > > > + adp5585->info->adp5585_devs, > > > > + adp5585->info->n_devs, NULL, 0, > > > > NULL); > > > > if (ret) > > > > return dev_err_probe(&i2c->dev, ret, > > > > "Failed to add child devices\n"); > > > > @@ -191,19 +380,31 @@ static DEFINE_SIMPLE_DEV_PM_OPS(adp5585_pm, > > > > adp5585_suspend, adp5585_resume); > > > > static const struct of_device_id adp5585_of_match[] = { > > > > { > > > > .compatible = "adi,adp5585-00", > > > > - .data = &adp5585_regmap_configs[ADP5585_REGMAP_00], > > > > + .data = &adp5585_info, > > > > }, { > > > > .compatible = "adi,adp5585-01", > > > > - .data = &adp5585_regmap_configs[ADP5585_REGMAP_00], > > > > + .data = &adp5585_01_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, > > > > + }, { > > > > + .compatible = "adi,adp5589-00", > > > > + .data = &adp5589_info, > > > > + }, { > > > > + .compatible = "adi,adp5589-01", > > > > + .data = &adp5589_01_info, > > > > + }, { > > > > + .compatible = "adi,adp5589-02", > > > > + .data = &adp5589_02_info, > > > > + }, { > > > > + .compatible = "adi,adp5589", > > > > + .data = &adp5589_info, > > > > }, > > > > { /* sentinel */ } > > > > }; > > > > diff --git a/include/linux/mfd/adp5585.h b/include/linux/mfd/adp5585.h > > > > index > > > > 016033cd68e46757aca86d21dd37025fd354b801..dffe1449de01dacf8fe78cf0e87d1f > > > > 176d > > > > 11f620 100644 > > > > --- a/include/linux/mfd/adp5585.h > > > > +++ b/include/linux/mfd/adp5585.h > > > > @@ -104,9 +104,11 @@ > > > > #define ADP5585_INT_CFG BIT(1) > > > > #define ADP5585_RST_CFG BIT(0) > > > > #define ADP5585_INT_EN 0x3c > > > > - > > > > #define ADP5585_MAX_REG ADP5585_INT_EN > > > > > > > > +#define ADP5585_MAX_ROW_NUM 6 > > > > +#define ADP5585_MAX_COL_NUM 5 > > > > + > > > > /* > > > > * Bank 0 covers pins "GPIO 1/R0" to "GPIO 6/R5", numbered 0 to 5 by > > > > the > > > > * driver, and bank 1 covers pins "GPIO 7/C0" to "GPIO 11/C4", numbered > > > > 6 > > > > to > > > > @@ -117,10 +119,63 @@ > > > > #define ADP5585_BANK(n) ((n) >= 6 ? 1 : 0) > > > > #define ADP5585_BIT(n) ((n) >= 6 ? BIT((n) - 6) : > > > > BIT(n)) > > > > > > > > +/* ADP5589 */ > > > > +#define ADP5589_MAN_ID_VALUE 0x10 > > > > +#define ADP5589_GPI_STATUS_A 0x16 > > > > +#define ADP5589_GPI_STATUS_C 0x18 > > > > +#define ADP5589_RPULL_CONFIG_A 0x19 > > > > +#define ADP5589_DEBOUNCE_DIS_A 0x27 > > > > +#define ADP5589_GPO_DATA_OUT_A 0x2a > > > > +#define ADP5589_GPO_OUT_MODE_A 0x2d > > > > +#define ADP5589_GPIO_DIRECTION_A 0x30 > > > > > > Indentation looks wrong. > > > > > > > +#define ADP5589_PWM_OFFT_LOW 0x3e > > > > +#define ADP5589_PWM_ONT_LOW 0x40 > > > > +#define ADP5589_PWM_CFG 0x42 > > > > +#define ADP5589_PIN_CONFIG_D 0x4C > > > > +#define ADP5589_GENERAL_CFG 0x4d > > > > +#define ADP5589_INT_EN 0x4e > > > > +#define ADP5589_MAX_REG ADP5589_INT_EN > > > > + > > > > +#define ADP5589_MAX_ROW_NUM 8 > > > > +#define ADP5589_MAX_COL_NUM 11 > > > > + > > > > +/* > > > > + * Bank 0 covers pins "GPIO 1/R0" to "GPIO 8/R7", numbered 0 to 7 by > > > > the > > > > + * driver, bank 1 covers pins "GPIO 9/C0" to "GPIO 16/C7", numbered 8 > > > > to > > > > + * 15 and bank 3 covers pins "GPIO 17/C8" to "GPIO 19/C10", numbered 16 > > > > to > > > > 18. > > > > + */ > > > > +#define ADP5589_BANK(n) ((n) >> 3) > > > > +#define ADP5589_BIT(n) BIT((n) & 0x7) > > > > + > > > > +struct adp5585_regs { > > > > + unsigned int debounce_dis_a; > > > > + unsigned int rpull_cfg_a; > > > > + unsigned int gpo_data_a; > > > > + unsigned int gpo_out_a; > > > > + unsigned int gpio_dir_a; > > > > + unsigned int gpi_stat_a; > > > > + unsigned int pwm_cfg; > > > > + unsigned int pwm_offt_low; > > > > + unsigned int pwm_ont_low; > > > > + unsigned int gen_cfg; > > > > + unsigned int ext_cfg; > > > > +}; > > > > + > > > > +struct adp5585_info { > > > > + const struct mfd_cell *adp5585_devs; > > > > + const struct regmap_config *regmap_config; > > > > + const struct adp5585_regs *regs; > > > > + unsigned int n_devs; > > > > + unsigned int id; > > > > + u8 max_rows; > > > > + u8 max_cols; > > > > +}; > > > > + > > > > struct regmap; > > > > > > > > struct adp5585_dev { > > > > struct regmap *regmap; > > > > + const struct adp5585_info *info; > > > > }; > > > > > > > > #endif
On Tue, 22 Apr 2025, Laurent Pinchart wrote: > Hi Nuno, > > On Mon, Apr 21, 2025 at 01:21:08PM +0100, Nuno Sá wrote: > > On Mon, 2025-04-21 at 12:15 +0300, Laurent Pinchart wrote: > > > On Tue, Apr 15, 2025 at 03:49:22PM +0100, Nuno Sá via B4 Relay wrote: > > > > From: Nuno Sá <nuno.sa@analog.com> > > > > > > > > The ADP5589 is a 19 I/O port expander with built-in keypad matrix decoder, > > > > programmable logic, reset generator, and PWM generator. > > > > > > > > This patch adds the foundation to add support for the adp5589 gpio and pwm > > > > drivers. Most importantly, we need to differentiate between some > > > > registers addresses. It also hints to future keymap support. > > > > > > Please split this in two patches, one that reworks the driver to support > > > different register addresses, and one that adds adp5589 support. > > > > > > > Signed-off-by: Nuno Sá <nuno.sa@analog.com> > > > > --- > > > > drivers/mfd/adp5585.c | 223 +++++++++++++++++++++++++++++++++++++++++--- > > > > include/linux/mfd/adp5585.h | 57 ++++++++++- > > > > 2 files changed, 268 insertions(+), 12 deletions(-) > > > > > > > > diff --git a/drivers/mfd/adp5585.c b/drivers/mfd/adp5585.c > > > > index fafe3ad93ea196e1eb8e79fecba58f36f12167eb..c3586c0d6aa2e7e7d94667993410610be7fc3672 100644 > > > > --- a/drivers/mfd/adp5585.c > > > > +++ b/drivers/mfd/adp5585.c > > > > @@ -25,6 +25,13 @@ static const struct mfd_cell adp5585_devs[] = { > > > > > > > > }; > > > > > > > > +static const struct mfd_cell adp5589_devs[] = { > > > > + MFD_CELL_NAME("adp5589-keys"), > > > > + MFD_CELL_NAME("adp5589-gpio"), > > > > + MFD_CELL_NAME("adp5589-pwm"), > > > > + > > > > +}; > > > > + > > > > static const struct regmap_range adp5585_volatile_ranges[] = { > > > > regmap_reg_range(ADP5585_ID, ADP5585_GPI_STATUS_B), > > > > }; > > > > @@ -34,6 +41,15 @@ static const struct regmap_access_table adp5585_volatile_regs = { > > > > .n_yes_ranges = ARRAY_SIZE(adp5585_volatile_ranges), > > > > }; > > > > > > > > +static const struct regmap_range adp5589_volatile_ranges[] = { > > > > + regmap_reg_range(ADP5585_ID, ADP5589_GPI_STATUS_C), > > > > +}; > > > > + > > > > +static const struct regmap_access_table adp5589_volatile_regs = { > > > > + .yes_ranges = adp5589_volatile_ranges, > > > > + .n_yes_ranges = ARRAY_SIZE(adp5589_volatile_ranges), > > > > +}; > > > > + > > > > /* > > > > * Chip variants differ in the default configuration of pull-up and pull-down > > > > * resistors, and therefore have different default register values: > > > > @@ -77,10 +93,52 @@ static const u8 adp5585_regmap_defaults_04[ADP5585_MAX_REG + 1] = { > > > > /* 0x38 */ 0x00, 0x00, 0x00, 0x00, 0x00, > > > > }; > > > > > > > > +static const u8 adp5589_regmap_defaults_00[ADP5589_MAX_REG + 1] = { > > > > + /* 0x00 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > > + /* 0x08 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > > + /* 0x10 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > > + /* 0x18 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > > + /* 0x20 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > > + /* 0x28 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > > + /* 0x30 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > > + /* 0x38 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > > + /* 0x40 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > > + /* 0x48 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > > +}; > > > > + > > > > +static const u8 adp5589_regmap_defaults_01[ADP5589_MAX_REG + 1] = { > > > > + /* 0x00 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > > + /* 0x08 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > > + /* 0x10 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > > + /* 0x18 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > > + /* 0x20 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > > + /* 0x28 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > > + /* 0x30 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > > + /* 0x38 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x20, 0x00, 0x00, > > > > + /* 0x40 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > > + /* 0x48 */ 0x00, 0x00, 0x00, 0x00, 0x20, 0x00, 0x00, > > > > +}; > > > > + > > > > +static const u8 adp5589_regmap_defaults_02[ADP5589_MAX_REG + 1] = { > > > > + /* 0x00 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > > + /* 0x08 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > > + /* 0x10 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > > + /* 0x18 */ 0x00, 0x41, 0x01, 0x00, 0x11, 0x04, 0x00, 0x00, > > > > + /* 0x20 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > > + /* 0x28 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > > + /* 0x30 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > > + /* 0x38 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > > + /* 0x40 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > > + /* 0x48 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > > +}; > > > > + > > > > enum adp5585_regmap_type { > > > > ADP5585_REGMAP_00, > > > > ADP5585_REGMAP_02, > > > > ADP5585_REGMAP_04, > > > > + ADP5589_REGMAP_00, > > > > + ADP5589_REGMAP_01, > > > > + ADP5589_REGMAP_02, > > > > }; > > > > > > > > static const struct regmap_config adp5585_regmap_configs[] = { > > > > @@ -111,6 +169,131 @@ static const struct regmap_config adp5585_regmap_configs[] = { > > > > .reg_defaults_raw = adp5585_regmap_defaults_04, > > > > .num_reg_defaults_raw = sizeof(adp5585_regmap_defaults_04), > > > > }, > > > > + [ADP5589_REGMAP_00] = { > > > > + .reg_bits = 8, > > > > + .val_bits = 8, > > > > + .max_register = ADP5589_MAX_REG, > > > > + .volatile_table = &adp5589_volatile_regs, > > > > + .cache_type = REGCACHE_MAPLE, > > > > + .reg_defaults_raw = adp5589_regmap_defaults_00, > > > > + .num_reg_defaults_raw = sizeof(adp5589_regmap_defaults_00), > > > > + }, > > > > + [ADP5589_REGMAP_01] = { > > > > + .reg_bits = 8, > > > > + .val_bits = 8, > > > > + .max_register = ADP5589_MAX_REG, > > > > + .volatile_table = &adp5589_volatile_regs, > > > > + .cache_type = REGCACHE_MAPLE, > > > > + .reg_defaults_raw = adp5589_regmap_defaults_01, > > > > + .num_reg_defaults_raw = sizeof(adp5589_regmap_defaults_01), > > > > + }, > > > > + [ADP5589_REGMAP_02] = { > > > > + .reg_bits = 8, > > > > + .val_bits = 8, > > > > + .max_register = ADP5589_MAX_REG, > > > > + .volatile_table = &adp5589_volatile_regs, > > > > + .cache_type = REGCACHE_MAPLE, > > > > + .reg_defaults_raw = adp5589_regmap_defaults_02, > > > > + .num_reg_defaults_raw = sizeof(adp5589_regmap_defaults_02), > > > > + }, > > > > +}; > > > > + > > > > +static const struct adp5585_regs adp5585_regs = { > > > > + .debounce_dis_a = ADP5585_DEBOUNCE_DIS_A, > > > > + .rpull_cfg_a = ADP5585_RPULL_CONFIG_A, > > > > + .gpo_data_a = ADP5585_GPO_DATA_OUT_A, > > > > + .gpo_out_a = ADP5585_GPO_OUT_MODE_A, > > > > + .gpio_dir_a = ADP5585_GPIO_DIRECTION_A, > > > > + .gpi_stat_a = ADP5585_GPI_STATUS_A, > > > > + .pwm_cfg = ADP5585_PWM_CFG, > > > > + .pwm_offt_low = ADP5585_PWM_OFFT_LOW, > > > > + .pwm_ont_low = ADP5585_PWM_ONT_LOW, > > > > + .gen_cfg = ADP5585_GENERAL_CFG, > > > > + .ext_cfg = ADP5585_PIN_CONFIG_C, > > > > +}; > > > > > > Why does this need to be stored in this driver, and not in the drivers > > > for the gpio and pwm cells ? If the kernel is compiled without e.g. the > > > adp5585-pwm driver, we shouldn't waste memory here by adding data that > > > only the adp5585-pwm driver needs. > > > > I don't really think the memory we would save to be that relevant but I can > > better separate things. I guess i went like this because there's some shared > > variables that will have to be in the top level structs and I did not wanted to > > have a "global" and "local" regs thingy... Memory and power savings are very important. Please prioritise them over effort. > I understand, and I think it's at least partly a coding style > preference. Personally, I find that having child-specific data in child > drivers makes the code easier to read, as it increases locality. > Otherwise, I have to look through multiple child drivers to see if and > where each field is used. Right. If resources are only used in specific sub-devices, please move it there. Only shared resources should be handed in the parent.
On Tue, 15 Apr 2025, Nuno Sá via B4 Relay wrote: > From: Nuno Sá <nuno.sa@analog.com> > > The ADP5589 is a 19 I/O port expander with built-in keypad matrix decoder, > programmable logic, reset generator, and PWM generator. > > This patch adds the foundation to add support for the adp5589 gpio and pwm > drivers. Most importantly, we need to differentiate between some > registers addresses. It also hints to future keymap support. > > Signed-off-by: Nuno Sá <nuno.sa@analog.com> > --- > drivers/mfd/adp5585.c | 223 +++++++++++++++++++++++++++++++++++++++++--- > include/linux/mfd/adp5585.h | 57 ++++++++++- > 2 files changed, 268 insertions(+), 12 deletions(-) [...] > + * Bank 0 covers pins "GPIO 1/R0" to "GPIO 8/R7", numbered 0 to 7 by the > + * driver, bank 1 covers pins "GPIO 9/C0" to "GPIO 16/C7", numbered 8 to > + * 15 and bank 3 covers pins "GPIO 17/C8" to "GPIO 19/C10", numbered 16 to 18. > + */ > +#define ADP5589_BANK(n) ((n) >> 3) > +#define ADP5589_BIT(n) BIT((n) & 0x7) > + > +struct adp5585_regs { > + unsigned int debounce_dis_a; > + unsigned int rpull_cfg_a; > + unsigned int gpo_data_a; > + unsigned int gpo_out_a; > + unsigned int gpio_dir_a; > + unsigned int gpi_stat_a; > + unsigned int pwm_cfg; > + unsigned int pwm_offt_low; > + unsigned int pwm_ont_low; > + unsigned int gen_cfg; > + unsigned int ext_cfg; > +}; > + > +struct adp5585_info { > + const struct mfd_cell *adp5585_devs; Okay, we are never doing this. Either use OF for platform registration or use MFD (or ACPI or PCI), but please do not pass MFD data through OF. > + const struct regmap_config *regmap_config; > + const struct adp5585_regs *regs; > + unsigned int n_devs; > + unsigned int id; What ID is this? We already have platform IDs and MFD cell IDs. > + u8 max_rows; > + u8 max_cols; > +}; > + > struct regmap; > > struct adp5585_dev { > struct regmap *regmap; > + const struct adp5585_info *info; > }; > > #endif > > -- > 2.49.0 > >
On Thu, Apr 24, 2025 at 05:18:38PM +0100, Lee Jones wrote: > On Tue, 15 Apr 2025, Nuno Sá via B4 Relay wrote: > > > From: Nuno Sá <nuno.sa@analog.com> > > > > The ADP5589 is a 19 I/O port expander with built-in keypad matrix decoder, > > programmable logic, reset generator, and PWM generator. > > > > This patch adds the foundation to add support for the adp5589 gpio and pwm > > drivers. Most importantly, we need to differentiate between some > > registers addresses. It also hints to future keymap support. > > > > Signed-off-by: Nuno Sá <nuno.sa@analog.com> > > --- > > drivers/mfd/adp5585.c | 223 +++++++++++++++++++++++++++++++++++++++++--- > > include/linux/mfd/adp5585.h | 57 ++++++++++- > > 2 files changed, 268 insertions(+), 12 deletions(-) > > [...] > > > + * Bank 0 covers pins "GPIO 1/R0" to "GPIO 8/R7", numbered 0 to 7 by the > > + * driver, bank 1 covers pins "GPIO 9/C0" to "GPIO 16/C7", numbered 8 to > > + * 15 and bank 3 covers pins "GPIO 17/C8" to "GPIO 19/C10", numbered 16 to 18. > > + */ > > +#define ADP5589_BANK(n) ((n) >> 3) > > +#define ADP5589_BIT(n) BIT((n) & 0x7) > > + > > +struct adp5585_regs { > > + unsigned int debounce_dis_a; > > + unsigned int rpull_cfg_a; > > + unsigned int gpo_data_a; > > + unsigned int gpo_out_a; > > + unsigned int gpio_dir_a; > > + unsigned int gpi_stat_a; > > + unsigned int pwm_cfg; > > + unsigned int pwm_offt_low; > > + unsigned int pwm_ont_low; > > + unsigned int gen_cfg; > > + unsigned int ext_cfg; > > +}; > > + > > +struct adp5585_info { > > + const struct mfd_cell *adp5585_devs; > > Okay, we are never doing this. Either use OF for platform registration > or use MFD (or ACPI or PCI), but please do not pass MFD data through OF. When I upstreamed the initial driver, I modelled the different functions through child nodes in DT, with a compatible string for each child. I was told very strongly to remove that. We have therefore no other choice than constructing the name of the cells based on the model of the main device. > > + const struct regmap_config *regmap_config; > > + const struct adp5585_regs *regs; > > + unsigned int n_devs; > > + unsigned int id; > > What ID is this? We already have platform IDs and MFD cell IDs. That's the value of the hardware model ID read-only register, it is used as a safety check to verify that the connected device corresponds to the compatible string. > > + u8 max_rows; > > + u8 max_cols; > > +}; > > + > > struct regmap; > > > > struct adp5585_dev { > > struct regmap *regmap; > > + const struct adp5585_info *info; > > }; > > > > #endif
On Thu, 24 Apr 2025, Laurent Pinchart wrote: > On Thu, Apr 24, 2025 at 05:18:38PM +0100, Lee Jones wrote: > > On Tue, 15 Apr 2025, Nuno Sá via B4 Relay wrote: > > > > > From: Nuno Sá <nuno.sa@analog.com> > > > > > > The ADP5589 is a 19 I/O port expander with built-in keypad matrix decoder, > > > programmable logic, reset generator, and PWM generator. > > > > > > This patch adds the foundation to add support for the adp5589 gpio and pwm > > > drivers. Most importantly, we need to differentiate between some > > > registers addresses. It also hints to future keymap support. > > > > > > Signed-off-by: Nuno Sá <nuno.sa@analog.com> > > > --- > > > drivers/mfd/adp5585.c | 223 +++++++++++++++++++++++++++++++++++++++++--- > > > include/linux/mfd/adp5585.h | 57 ++++++++++- > > > 2 files changed, 268 insertions(+), 12 deletions(-) > > > > [...] > > > > > + * Bank 0 covers pins "GPIO 1/R0" to "GPIO 8/R7", numbered 0 to 7 by the > > > + * driver, bank 1 covers pins "GPIO 9/C0" to "GPIO 16/C7", numbered 8 to > > > + * 15 and bank 3 covers pins "GPIO 17/C8" to "GPIO 19/C10", numbered 16 to 18. > > > + */ > > > +#define ADP5589_BANK(n) ((n) >> 3) > > > +#define ADP5589_BIT(n) BIT((n) & 0x7) > > > + > > > +struct adp5585_regs { > > > + unsigned int debounce_dis_a; > > > + unsigned int rpull_cfg_a; > > > + unsigned int gpo_data_a; > > > + unsigned int gpo_out_a; > > > + unsigned int gpio_dir_a; > > > + unsigned int gpi_stat_a; > > > + unsigned int pwm_cfg; > > > + unsigned int pwm_offt_low; > > > + unsigned int pwm_ont_low; > > > + unsigned int gen_cfg; > > > + unsigned int ext_cfg; > > > +}; > > > + > > > +struct adp5585_info { > > > + const struct mfd_cell *adp5585_devs; > > > > Okay, we are never doing this. Either use OF for platform registration > > or use MFD (or ACPI or PCI), but please do not pass MFD data through OF. > > When I upstreamed the initial driver, I modelled the different functions > through child nodes in DT, with a compatible string for each child. I > was told very strongly to remove that. We have therefore no other choice > than constructing the name of the cells based on the model of the main > device. It's okay to add this information statically in this driver. It's not okay to then pass it through the OF API. You can pass an identifier through the .data attribute to match on, but we are not passing MFD cell data through like this. > > > + const struct regmap_config *regmap_config; > > > + const struct adp5585_regs *regs; > > > + unsigned int n_devs; > > > + unsigned int id; > > > > What ID is this? We already have platform IDs and MFD cell IDs. > > That's the value of the hardware model ID read-only register, it is used > as a safety check to verify that the connected device corresponds to the > compatible string. I suggest changing the nomenclature to be more forthcoming. 'model', 'version', 'hwid', 'chipid', etc. Why is it being stored? Is it used to match on at a later date?
On Thu, Apr 24, 2025 at 05:38:30PM +0100, Lee Jones wrote: > On Thu, 24 Apr 2025, Laurent Pinchart wrote: > > On Thu, Apr 24, 2025 at 05:18:38PM +0100, Lee Jones wrote: > > > On Tue, 15 Apr 2025, Nuno Sá via B4 Relay wrote: > > > > > > > From: Nuno Sá <nuno.sa@analog.com> > > > > > > > > The ADP5589 is a 19 I/O port expander with built-in keypad matrix decoder, > > > > programmable logic, reset generator, and PWM generator. > > > > > > > > This patch adds the foundation to add support for the adp5589 gpio and pwm > > > > drivers. Most importantly, we need to differentiate between some > > > > registers addresses. It also hints to future keymap support. > > > > > > > > Signed-off-by: Nuno Sá <nuno.sa@analog.com> > > > > --- > > > > drivers/mfd/adp5585.c | 223 +++++++++++++++++++++++++++++++++++++++++--- > > > > include/linux/mfd/adp5585.h | 57 ++++++++++- > > > > 2 files changed, 268 insertions(+), 12 deletions(-) > > > > > > [...] > > > > > > > + * Bank 0 covers pins "GPIO 1/R0" to "GPIO 8/R7", numbered 0 to 7 by the > > > > + * driver, bank 1 covers pins "GPIO 9/C0" to "GPIO 16/C7", numbered 8 to > > > > + * 15 and bank 3 covers pins "GPIO 17/C8" to "GPIO 19/C10", numbered 16 to 18. > > > > + */ > > > > +#define ADP5589_BANK(n) ((n) >> 3) > > > > +#define ADP5589_BIT(n) BIT((n) & 0x7) > > > > + > > > > +struct adp5585_regs { > > > > + unsigned int debounce_dis_a; > > > > + unsigned int rpull_cfg_a; > > > > + unsigned int gpo_data_a; > > > > + unsigned int gpo_out_a; > > > > + unsigned int gpio_dir_a; > > > > + unsigned int gpi_stat_a; > > > > + unsigned int pwm_cfg; > > > > + unsigned int pwm_offt_low; > > > > + unsigned int pwm_ont_low; > > > > + unsigned int gen_cfg; > > > > + unsigned int ext_cfg; > > > > +}; > > > > + > > > > +struct adp5585_info { > > > > + const struct mfd_cell *adp5585_devs; > > > > > > Okay, we are never doing this. Either use OF for platform registration > > > or use MFD (or ACPI or PCI), but please do not pass MFD data through OF. > > > > When I upstreamed the initial driver, I modelled the different functions > > through child nodes in DT, with a compatible string for each child. I > > was told very strongly to remove that. We have therefore no other choice > > than constructing the name of the cells based on the model of the main > > device. > > It's okay to add this information statically in this driver. It's not > okay to then pass it through the OF API. You can pass an identifier > through the .data attribute to match on, but we are not passing MFD cell > data through like this. Sorry, I'm not following you. What's the issue with the .data field pointing to an instance of a structure that lists properties related to the device model ? > > > > + const struct regmap_config *regmap_config; > > > > + const struct adp5585_regs *regs; > > > > + unsigned int n_devs; > > > > + unsigned int id; > > > > > > What ID is this? We already have platform IDs and MFD cell IDs. > > > > That's the value of the hardware model ID read-only register, it is used > > as a safety check to verify that the connected device corresponds to the > > compatible string. > > I suggest changing the nomenclature to be more forthcoming. > > 'model', 'version', 'hwid', 'chipid', etc. > > Why is it being stored? Is it used to match on at a later date? The adp5585_info structure contains static information the describe each device model. There's one global static const instance per device model, and they are referenced from device id structures (e.g. of_device_id). The driver gets an info pointer corresponding to the model reported by the platform firmware (e.g. DT). It reads the device ID from the device at probe time, and compares it with the value stored in the structure as a safety check to ensure there's no mismatch.
On Thu, 24 Apr 2025, Laurent Pinchart wrote: > On Thu, Apr 24, 2025 at 05:38:30PM +0100, Lee Jones wrote: > > On Thu, 24 Apr 2025, Laurent Pinchart wrote: > > > On Thu, Apr 24, 2025 at 05:18:38PM +0100, Lee Jones wrote: > > > > On Tue, 15 Apr 2025, Nuno Sá via B4 Relay wrote: > > > > > > > > > From: Nuno Sá <nuno.sa@analog.com> > > > > > > > > > > The ADP5589 is a 19 I/O port expander with built-in keypad matrix decoder, > > > > > programmable logic, reset generator, and PWM generator. > > > > > > > > > > This patch adds the foundation to add support for the adp5589 gpio and pwm > > > > > drivers. Most importantly, we need to differentiate between some > > > > > registers addresses. It also hints to future keymap support. > > > > > > > > > > Signed-off-by: Nuno Sá <nuno.sa@analog.com> > > > > > --- > > > > > drivers/mfd/adp5585.c | 223 +++++++++++++++++++++++++++++++++++++++++--- > > > > > include/linux/mfd/adp5585.h | 57 ++++++++++- > > > > > 2 files changed, 268 insertions(+), 12 deletions(-) > > > > > > > > [...] > > > > > > > > > + * Bank 0 covers pins "GPIO 1/R0" to "GPIO 8/R7", numbered 0 to 7 by the > > > > > + * driver, bank 1 covers pins "GPIO 9/C0" to "GPIO 16/C7", numbered 8 to > > > > > + * 15 and bank 3 covers pins "GPIO 17/C8" to "GPIO 19/C10", numbered 16 to 18. > > > > > + */ > > > > > +#define ADP5589_BANK(n) ((n) >> 3) > > > > > +#define ADP5589_BIT(n) BIT((n) & 0x7) > > > > > + > > > > > +struct adp5585_regs { > > > > > + unsigned int debounce_dis_a; > > > > > + unsigned int rpull_cfg_a; > > > > > + unsigned int gpo_data_a; > > > > > + unsigned int gpo_out_a; > > > > > + unsigned int gpio_dir_a; > > > > > + unsigned int gpi_stat_a; > > > > > + unsigned int pwm_cfg; > > > > > + unsigned int pwm_offt_low; > > > > > + unsigned int pwm_ont_low; > > > > > + unsigned int gen_cfg; > > > > > + unsigned int ext_cfg; > > > > > +}; > > > > > + > > > > > +struct adp5585_info { > > > > > + const struct mfd_cell *adp5585_devs; > > > > > > > > Okay, we are never doing this. Either use OF for platform registration > > > > or use MFD (or ACPI or PCI), but please do not pass MFD data through OF. > > > > > > When I upstreamed the initial driver, I modelled the different functions > > > through child nodes in DT, with a compatible string for each child. I > > > was told very strongly to remove that. We have therefore no other choice > > > than constructing the name of the cells based on the model of the main > > > device. > > > > It's okay to add this information statically in this driver. It's not > > okay to then pass it through the OF API. You can pass an identifier > > through the .data attribute to match on, but we are not passing MFD cell > > data through like this. > > Sorry, I'm not following you. What's the issue with the .data field > pointing to an instance of a structure that lists properties related to > the device model ? There isn't one. By all means place any type of platform data you want in there. Similar to the information you'd find in Device Tree or the old board-files type pdata. You can even extract the platform data you pass through the OF API and place it into MFD platform data. The line is being drawn on passing through one type of initialisation API with another, MFD through OF in this case. MFD cells containing device registration data (including platform data!) is not itself platform data. > > > > > + const struct regmap_config *regmap_config; > > > > > + const struct adp5585_regs *regs; > > > > > + unsigned int n_devs; > > > > > + unsigned int id; > > > > > > > > What ID is this? We already have platform IDs and MFD cell IDs. > > > > > > That's the value of the hardware model ID read-only register, it is used > > > as a safety check to verify that the connected device corresponds to the > > > compatible string. > > > > I suggest changing the nomenclature to be more forthcoming. > > > > 'model', 'version', 'hwid', 'chipid', etc. > > > > Why is it being stored? Is it used to match on at a later date? > > The adp5585_info structure contains static information the describe each > device model. There's one global static const instance per device model, > and they are referenced from device id structures (e.g. of_device_id). > The driver gets an info pointer corresponding to the model reported by > the platform firmware (e.g. DT). It reads the device ID from the device > at probe time, and compares it with the value stored in the structure as > a safety check to ensure there's no mismatch. I think the current implementation (as a whole, not just the IDs) needs a rethink. Very few attributes are changing here, both between the 2 platforms and the several variants you're trying to support, leading to masses of repetition. Looking at the static configuration here, this is starting to look like 2 pieces of hardware with the only variation within each being the default register values. Is that a correct assumption? If so, does mean all of this added complexity is just to configure a few register values such that the two platforms can be used for different things? Or are these really 6 true hardware variants of one another? Either way, this approach doesn't scale. Instead of multiplying the amount of platforms / variants together and creating that number of static structs, I'd suggest using templating and only adapting what actually changes. For instance, the following attributes in 'struct regmap_config' never change; reg_bits, val_bits, and cache_type. And max_register only changes between the 2 hardware platforms. The reg_defaults_raw values can be changed in a switch statement. Same goes for 'struct adp5585_info'. Only regmap_config changes between variants. Everything else is exactly the same. So, with the use of a few of templates and a couple of succinct switch cases, you can control all of the differentiation you need. And for every variant you wish to add, it's a couple of extra lines rather than many, leading to a much more scaleable implementation.
Hi Lee, On Fri, Apr 25, 2025 at 08:58:59AM +0100, Lee Jones wrote: > On Thu, 24 Apr 2025, Laurent Pinchart wrote: > > On Thu, Apr 24, 2025 at 05:38:30PM +0100, Lee Jones wrote: > > > On Thu, 24 Apr 2025, Laurent Pinchart wrote: > > > > On Thu, Apr 24, 2025 at 05:18:38PM +0100, Lee Jones wrote: > > > > > On Tue, 15 Apr 2025, Nuno Sá via B4 Relay wrote: > > > > > > > > > > > From: Nuno Sá <nuno.sa@analog.com> > > > > > > > > > > > > The ADP5589 is a 19 I/O port expander with built-in keypad matrix decoder, > > > > > > programmable logic, reset generator, and PWM generator. > > > > > > > > > > > > This patch adds the foundation to add support for the adp5589 gpio and pwm > > > > > > drivers. Most importantly, we need to differentiate between some > > > > > > registers addresses. It also hints to future keymap support. > > > > > > > > > > > > Signed-off-by: Nuno Sá <nuno.sa@analog.com> > > > > > > --- > > > > > > drivers/mfd/adp5585.c | 223 +++++++++++++++++++++++++++++++++++++++++--- > > > > > > include/linux/mfd/adp5585.h | 57 ++++++++++- > > > > > > 2 files changed, 268 insertions(+), 12 deletions(-) > > > > > > > > > > [...] > > > > > > > > > > > + * Bank 0 covers pins "GPIO 1/R0" to "GPIO 8/R7", numbered 0 to 7 by the > > > > > > + * driver, bank 1 covers pins "GPIO 9/C0" to "GPIO 16/C7", numbered 8 to > > > > > > + * 15 and bank 3 covers pins "GPIO 17/C8" to "GPIO 19/C10", numbered 16 to 18. > > > > > > + */ > > > > > > +#define ADP5589_BANK(n) ((n) >> 3) > > > > > > +#define ADP5589_BIT(n) BIT((n) & 0x7) > > > > > > + > > > > > > +struct adp5585_regs { > > > > > > + unsigned int debounce_dis_a; > > > > > > + unsigned int rpull_cfg_a; > > > > > > + unsigned int gpo_data_a; > > > > > > + unsigned int gpo_out_a; > > > > > > + unsigned int gpio_dir_a; > > > > > > + unsigned int gpi_stat_a; > > > > > > + unsigned int pwm_cfg; > > > > > > + unsigned int pwm_offt_low; > > > > > > + unsigned int pwm_ont_low; > > > > > > + unsigned int gen_cfg; > > > > > > + unsigned int ext_cfg; > > > > > > +}; > > > > > > + > > > > > > +struct adp5585_info { > > > > > > + const struct mfd_cell *adp5585_devs; > > > > > > > > > > Okay, we are never doing this. Either use OF for platform registration > > > > > or use MFD (or ACPI or PCI), but please do not pass MFD data through OF. > > > > > > > > When I upstreamed the initial driver, I modelled the different functions > > > > through child nodes in DT, with a compatible string for each child. I > > > > was told very strongly to remove that. We have therefore no other choice > > > > than constructing the name of the cells based on the model of the main > > > > device. > > > > > > It's okay to add this information statically in this driver. It's not > > > okay to then pass it through the OF API. You can pass an identifier > > > through the .data attribute to match on, but we are not passing MFD cell > > > data through like this. > > > > Sorry, I'm not following you. What's the issue with the .data field > > pointing to an instance of a structure that lists properties related to > > the device model ? > > There isn't one. By all means place any type of platform data you want > in there. Similar to the information you'd find in Device Tree or the > old board-files type pdata. You can even extract the platform data you > pass through the OF API and place it into MFD platform data. The line > is being drawn on passing through one type of initialisation API with > another, MFD through OF in this case. MFD cells containing device > registration data (including platform data!) is not itself platform > data. I'm still not following you. The issue will likely go away in the next version anyway, as the MFD cells registration code needs to be rewritten to be more dynamic. > > > > > > + const struct regmap_config *regmap_config; > > > > > > + const struct adp5585_regs *regs; > > > > > > + unsigned int n_devs; > > > > > > + unsigned int id; > > > > > > > > > > What ID is this? We already have platform IDs and MFD cell IDs. > > > > > > > > That's the value of the hardware model ID read-only register, it is used > > > > as a safety check to verify that the connected device corresponds to the > > > > compatible string. > > > > > > I suggest changing the nomenclature to be more forthcoming. > > > > > > 'model', 'version', 'hwid', 'chipid', etc. > > > > > > Why is it being stored? Is it used to match on at a later date? > > > > The adp5585_info structure contains static information the describe each > > device model. There's one global static const instance per device model, > > and they are referenced from device id structures (e.g. of_device_id). > > The driver gets an info pointer corresponding to the model reported by > > the platform firmware (e.g. DT). It reads the device ID from the device > > at probe time, and compares it with the value stored in the structure as > > a safety check to ensure there's no mismatch. > > I think the current implementation (as a whole, not just the IDs) needs > a rethink. Very few attributes are changing here, both between the 2 > platforms and the several variants you're trying to support, leading to > masses of repetition. > > Looking at the static configuration here, this is starting to look like > 2 pieces of hardware with the only variation within each being the > default register values. Is that a correct assumption? The variants of the ADP5585 differ mainly by how they handle the default configuration of pull-up and pull-down resistors. The consequence on the driver side is limited to default register values, yes. ADP5589 differs more significantly from the ADP5585. Differences between the ADP5589 variants are small as far as I understand (datasheets are public, should you want to have a look). > If so, does > mean all of this added complexity is just to configure a few register > values such that the two platforms can be used for different things? Or > are these really 6 true hardware variants of one another? They are different physical chips with different product numbers. > Either way, this approach doesn't scale. Instead of multiplying the > amount of platforms / variants together and creating that number of > static structs, I'd suggest using templating and only adapting what > actually changes. > > For instance, the following attributes in 'struct regmap_config' never > change; reg_bits, val_bits, and cache_type. And max_register only > changes between the 2 hardware platforms. The reg_defaults_raw values > can be changed in a switch statement. All the fields of the adp5585_info structure that you would like to dynamically set would then need to be stored in the adp5585 structure. The would essentially trade static const data for dynamic data and more code. Is that a personal coding style preference, or are there clear advantages ? > Same goes for 'struct adp5585_info'. Only regmap_config changes between > variants. Everything else is exactly the same. I assume this comment relates only to the different variants of the info structure for the same model (e.g. ADP5585 or ADP5589). There are more differences between the ADP5585 and ADP5589 entries. > So, with the use of a > few of templates and a couple of succinct switch cases, you can control > all of the differentiation you need. And for every variant you wish to > add, it's a couple of extra lines rather than many, leading to a > much more scaleable implementation. That also seems like a personal coding style preference :-) Adding a new compatible variant with the existing approach only requires adding an instance of the info structure, while your proposal would require changes in multiple places. It seems more work to me (from a personal preference point of view). Of course, if the new variant requires developing abstractions that don't exist (such as supporting large differences in the registers layout as needed for the ADP5589), refactoring of the code will always be required. This seems a bit of a theoretical concern though, as I'm not aware of any other chip that would require such development. In any case, let's see how the next version will look like, after reworking the MFD cells registration code. Maybe it will make everybody happy :-)
On Fri, 25 Apr 2025, Laurent Pinchart wrote: > Hi Lee, > > On Fri, Apr 25, 2025 at 08:58:59AM +0100, Lee Jones wrote: > > On Thu, 24 Apr 2025, Laurent Pinchart wrote: > > > On Thu, Apr 24, 2025 at 05:38:30PM +0100, Lee Jones wrote: > > > > On Thu, 24 Apr 2025, Laurent Pinchart wrote: > > > > > On Thu, Apr 24, 2025 at 05:18:38PM +0100, Lee Jones wrote: > > > > > > On Tue, 15 Apr 2025, Nuno Sá via B4 Relay wrote: > > > > > > > > > > > > > From: Nuno Sá <nuno.sa@analog.com> > > > > > > > > > > > > > > The ADP5589 is a 19 I/O port expander with built-in keypad matrix decoder, > > > > > > > programmable logic, reset generator, and PWM generator. > > > > > > > > > > > > > > This patch adds the foundation to add support for the adp5589 gpio and pwm > > > > > > > drivers. Most importantly, we need to differentiate between some > > > > > > > registers addresses. It also hints to future keymap support. > > > > > > > > > > > > > > Signed-off-by: Nuno Sá <nuno.sa@analog.com> > > > > > > > --- > > > > > > > drivers/mfd/adp5585.c | 223 +++++++++++++++++++++++++++++++++++++++++--- > > > > > > > include/linux/mfd/adp5585.h | 57 ++++++++++- > > > > > > > 2 files changed, 268 insertions(+), 12 deletions(-) > > > > > > > > > > > > [...] > > > > > > > > > > > > > + * Bank 0 covers pins "GPIO 1/R0" to "GPIO 8/R7", numbered 0 to 7 by the > > > > > > > + * driver, bank 1 covers pins "GPIO 9/C0" to "GPIO 16/C7", numbered 8 to > > > > > > > + * 15 and bank 3 covers pins "GPIO 17/C8" to "GPIO 19/C10", numbered 16 to 18. > > > > > > > + */ > > > > > > > +#define ADP5589_BANK(n) ((n) >> 3) > > > > > > > +#define ADP5589_BIT(n) BIT((n) & 0x7) > > > > > > > + > > > > > > > +struct adp5585_regs { > > > > > > > + unsigned int debounce_dis_a; > > > > > > > + unsigned int rpull_cfg_a; > > > > > > > + unsigned int gpo_data_a; > > > > > > > + unsigned int gpo_out_a; > > > > > > > + unsigned int gpio_dir_a; > > > > > > > + unsigned int gpi_stat_a; > > > > > > > + unsigned int pwm_cfg; > > > > > > > + unsigned int pwm_offt_low; > > > > > > > + unsigned int pwm_ont_low; > > > > > > > + unsigned int gen_cfg; > > > > > > > + unsigned int ext_cfg; > > > > > > > +}; > > > > > > > + > > > > > > > +struct adp5585_info { > > > > > > > + const struct mfd_cell *adp5585_devs; > > > > > > > > > > > > Okay, we are never doing this. Either use OF for platform registration > > > > > > or use MFD (or ACPI or PCI), but please do not pass MFD data through OF. > > > > > > > > > > When I upstreamed the initial driver, I modelled the different functions > > > > > through child nodes in DT, with a compatible string for each child. I > > > > > was told very strongly to remove that. We have therefore no other choice > > > > > than constructing the name of the cells based on the model of the main > > > > > device. > > > > > > > > It's okay to add this information statically in this driver. It's not > > > > okay to then pass it through the OF API. You can pass an identifier > > > > through the .data attribute to match on, but we are not passing MFD cell > > > > data through like this. > > > > > > Sorry, I'm not following you. What's the issue with the .data field > > > pointing to an instance of a structure that lists properties related to > > > the device model ? > > > > There isn't one. By all means place any type of platform data you want > > in there. Similar to the information you'd find in Device Tree or the > > old board-files type pdata. You can even extract the platform data you > > pass through the OF API and place it into MFD platform data. The line > > is being drawn on passing through one type of initialisation API with > > another, MFD through OF in this case. MFD cells containing device > > registration data (including platform data!) is not itself platform > > data. > > I'm still not following you. The issue will likely go away in the next > version anyway, as the MFD cells registration code needs to be rewritten > to be more dynamic. > > > > > > > > + const struct regmap_config *regmap_config; > > > > > > > + const struct adp5585_regs *regs; > > > > > > > + unsigned int n_devs; > > > > > > > + unsigned int id; > > > > > > > > > > > > What ID is this? We already have platform IDs and MFD cell IDs. > > > > > > > > > > That's the value of the hardware model ID read-only register, it is used > > > > > as a safety check to verify that the connected device corresponds to the > > > > > compatible string. > > > > > > > > I suggest changing the nomenclature to be more forthcoming. > > > > > > > > 'model', 'version', 'hwid', 'chipid', etc. > > > > > > > > Why is it being stored? Is it used to match on at a later date? > > > > > > The adp5585_info structure contains static information the describe each > > > device model. There's one global static const instance per device model, > > > and they are referenced from device id structures (e.g. of_device_id). > > > The driver gets an info pointer corresponding to the model reported by > > > the platform firmware (e.g. DT). It reads the device ID from the device > > > at probe time, and compares it with the value stored in the structure as > > > a safety check to ensure there's no mismatch. > > > > I think the current implementation (as a whole, not just the IDs) needs > > a rethink. Very few attributes are changing here, both between the 2 > > platforms and the several variants you're trying to support, leading to > > masses of repetition. > > > > Looking at the static configuration here, this is starting to look like > > 2 pieces of hardware with the only variation within each being the > > default register values. Is that a correct assumption? > > The variants of the ADP5585 differ mainly by how they handle the default > configuration of pull-up and pull-down resistors. The consequence on the > driver side is limited to default register values, yes. > > ADP5589 differs more significantly from the ADP5585. Differences between > the ADP5589 variants are small as far as I understand (datasheets are > public, should you want to have a look). > > > If so, does > > mean all of this added complexity is just to configure a few register > > values such that the two platforms can be used for different things? Or > > are these really 6 true hardware variants of one another? > > They are different physical chips with different product numbers. > > > Either way, this approach doesn't scale. Instead of multiplying the > > amount of platforms / variants together and creating that number of > > static structs, I'd suggest using templating and only adapting what > > actually changes. > > > > For instance, the following attributes in 'struct regmap_config' never > > change; reg_bits, val_bits, and cache_type. And max_register only > > changes between the 2 hardware platforms. The reg_defaults_raw values > > can be changed in a switch statement. > > All the fields of the adp5585_info structure that you would like to > dynamically set would then need to be stored in the adp5585 structure. > The would essentially trade static const data for dynamic data and more > code. Is that a personal coding style preference, or are there clear > advantages ? > > > Same goes for 'struct adp5585_info'. Only regmap_config changes between > > variants. Everything else is exactly the same. > > I assume this comment relates only to the different variants of the info > structure for the same model (e.g. ADP5585 or ADP5589). There are more > differences between the ADP5585 and ADP5589 entries. > > > So, with the use of a > > few of templates and a couple of succinct switch cases, you can control > > all of the differentiation you need. And for every variant you wish to > > add, it's a couple of extra lines rather than many, leading to a > > much more scaleable implementation. > > That also seems like a personal coding style preference :-) Adding a new > compatible variant with the existing approach only requires adding an > instance of the info structure, while your proposal would require > changes in multiple places. It seems more work to me (from a personal > preference point of view). > > Of course, if the new variant requires developing abstractions that > don't exist (such as supporting large differences in the registers > layout as needed for the ADP5589), refactoring of the code will always > be required. This seems a bit of a theoretical concern though, as I'm > not aware of any other chip that would require such development. > > In any case, let's see how the next version will look like, after > reworking the MFD cells registration code. Maybe it will make everybody > happy :-) Let's hope. =:)
On Fri, 2025-04-25 at 12:13 +0300, Laurent Pinchart wrote: > Hi Lee, > > On Fri, Apr 25, 2025 at 08:58:59AM +0100, Lee Jones wrote: > > On Thu, 24 Apr 2025, Laurent Pinchart wrote: > > > On Thu, Apr 24, 2025 at 05:38:30PM +0100, Lee Jones wrote: > > > > On Thu, 24 Apr 2025, Laurent Pinchart wrote: > > > > > On Thu, Apr 24, 2025 at 05:18:38PM +0100, Lee Jones wrote: > > > > > > On Tue, 15 Apr 2025, Nuno Sá via B4 Relay wrote: > > > > > > > > > > > > > From: Nuno Sá <nuno.sa@analog.com> > > > > > > > > > > > > > > The ADP5589 is a 19 I/O port expander with built-in keypad matrix > > > > > > > decoder, > > > > > > > programmable logic, reset generator, and PWM generator. > > > > > > > > > > > > > > This patch adds the foundation to add support for the adp5589 gpio > > > > > > > and pwm > > > > > > > drivers. Most importantly, we need to differentiate between some > > > > > > > registers addresses. It also hints to future keymap support. > > > > > > > > > > > > > > Signed-off-by: Nuno Sá <nuno.sa@analog.com> > > > > > > > --- > > > > > > > drivers/mfd/adp5585.c | 223 > > > > > > > +++++++++++++++++++++++++++++++++++++++++--- > > > > > > > include/linux/mfd/adp5585.h | 57 ++++++++++- > > > > > > > 2 files changed, 268 insertions(+), 12 deletions(-) > > > > > > > > > > > > [...] > > > > > > > > > > > > > + * Bank 0 covers pins "GPIO 1/R0" to "GPIO 8/R7", numbered 0 to 7 > > > > > > > by the > > > > > > > + * driver, bank 1 covers pins "GPIO 9/C0" to "GPIO 16/C7", > > > > > > > numbered 8 to > > > > > > > + * 15 and bank 3 covers pins "GPIO 17/C8" to "GPIO 19/C10", > > > > > > > numbered 16 to 18. > > > > > > > + */ > > > > > > > +#define ADP5589_BANK(n) ((n) >> 3) > > > > > > > +#define ADP5589_BIT(n) BIT((n) & 0x7) > > > > > > > + > > > > > > > +struct adp5585_regs { > > > > > > > + unsigned int debounce_dis_a; > > > > > > > + unsigned int rpull_cfg_a; > > > > > > > + unsigned int gpo_data_a; > > > > > > > + unsigned int gpo_out_a; > > > > > > > + unsigned int gpio_dir_a; > > > > > > > + unsigned int gpi_stat_a; > > > > > > > + unsigned int pwm_cfg; > > > > > > > + unsigned int pwm_offt_low; > > > > > > > + unsigned int pwm_ont_low; > > > > > > > + unsigned int gen_cfg; > > > > > > > + unsigned int ext_cfg; > > > > > > > +}; > > > > > > > + > > > > > > > +struct adp5585_info { > > > > > > > + const struct mfd_cell *adp5585_devs; > > > > > > > > > > > > Okay, we are never doing this. Either use OF for platform > > > > > > registration > > > > > > or use MFD (or ACPI or PCI), but please do not pass MFD data through > > > > > > OF. > > > > > > > > > > When I upstreamed the initial driver, I modelled the different > > > > > functions > > > > > through child nodes in DT, with a compatible string for each child. I > > > > > was told very strongly to remove that. We have therefore no other > > > > > choice > > > > > than constructing the name of the cells based on the model of the main > > > > > device. > > > > > > > > It's okay to add this information statically in this driver. It's not > > > > okay to then pass it through the OF API. You can pass an identifier > > > > through the .data attribute to match on, but we are not passing MFD cell > > > > data through like this. > > > > > > Sorry, I'm not following you. What's the issue with the .data field > > > pointing to an instance of a structure that lists properties related to > > > the device model ? > > > > There isn't one. By all means place any type of platform data you want > > in there. Similar to the information you'd find in Device Tree or the > > old board-files type pdata. You can even extract the platform data you > > pass through the OF API and place it into MFD platform data. The line > > is being drawn on passing through one type of initialisation API with > > another, MFD through OF in this case. MFD cells containing device > > registration data (including platform data!) is not itself platform > > data. > > I'm still not following you. The issue will likely go away in the next > version anyway, as the MFD cells registration code needs to be rewritten > to be more dynamic. Not sure if there's any real issue but I think Lee's main concern is passing MFD related data (struct mfd_cell) though OF (via the of table). Not sure if this is one of the things Lee does not like but in theory, like this, you can get this data from child platform devices for example. > > > > > > > > + const struct regmap_config *regmap_config; > > > > > > > + const struct adp5585_regs *regs; > > > > > > > + unsigned int n_devs; > > > > > > > + unsigned int id; > > > > > > > > > > > > What ID is this? We already have platform IDs and MFD cell IDs. > > > > > > > > > > That's the value of the hardware model ID read-only register, it is > > > > > used > > > > > as a safety check to verify that the connected device corresponds to > > > > > the > > > > > compatible string. > > > > > > > > I suggest changing the nomenclature to be more forthcoming. > > > > > > > > 'model', 'version', 'hwid', 'chipid', etc. > > > > > > > > Why is it being stored? Is it used to match on at a later date? > > > > > > The adp5585_info structure contains static information the describe each > > > device model. There's one global static const instance per device model, > > > and they are referenced from device id structures (e.g. of_device_id). > > > The driver gets an info pointer corresponding to the model reported by > > > the platform firmware (e.g. DT). It reads the device ID from the device > > > at probe time, and compares it with the value stored in the structure as > > > a safety check to ensure there's no mismatch. > > > > I think the current implementation (as a whole, not just the IDs) needs > > a rethink. Very few attributes are changing here, both between the 2 > > platforms and the several variants you're trying to support, leading to > > masses of repetition. > > > > Looking at the static configuration here, this is starting to look like > > 2 pieces of hardware with the only variation within each being the > > default register values. Is that a correct assumption? > > The variants of the ADP5585 differ mainly by how they handle the default > configuration of pull-up and pull-down resistors. The consequence on the > driver side is limited to default register values, yes. > > ADP5589 differs more significantly from the ADP5585. Differences between > the ADP5589 variants are small as far as I understand (datasheets are > public, should you want to have a look). > > > If so, does > > mean all of this added complexity is just to configure a few register > > values such that the two platforms can be used for different things? Or > > are these really 6 true hardware variants of one another? > > They are different physical chips with different product numbers. > > > Either way, this approach doesn't scale. Instead of multiplying the > > amount of platforms / variants together and creating that number of > > static structs, I'd suggest using templating and only adapting what > > actually changes. > > > > For instance, the following attributes in 'struct regmap_config' never > > change; reg_bits, val_bits, and cache_type. And max_register only > > changes between the 2 hardware platforms. The reg_defaults_raw values > > can be changed in a switch statement. > > All the fields of the adp5585_info structure that you would like to > dynamically set would then need to be stored in the adp5585 structure. > The would essentially trade static const data for dynamic data and more > code. Is that a personal coding style preference, or are there clear > advantages ? > > > Same goes for 'struct adp5585_info'. Only regmap_config changes between > > variants. Everything else is exactly the same. > > I assume this comment relates only to the different variants of the info > structure for the same model (e.g. ADP5585 or ADP5589). There are more > differences between the ADP5585 and ADP5589 entries. > > > So, with the use of a > > few of templates and a couple of succinct switch cases, you can control > > all of the differentiation you need. And for every variant you wish to > > add, it's a couple of extra lines rather than many, leading to a > > much more scaleable implementation. > > That also seems like a personal coding style preference :-) Adding a new > compatible variant with the existing approach only requires adding an > instance of the info structure, while your proposal would require > changes in multiple places. It seems more work to me (from a personal > preference point of view). > > Of course, if the new variant requires developing abstractions that > don't exist (such as supporting large differences in the registers > layout as needed for the ADP5589), refactoring of the code will always > be required. This seems a bit of a theoretical concern though, as I'm > not aware of any other chip that would require such development. > > In any case, let's see how the next version will look like, after > reworking the MFD cells registration code. Maybe it will make everybody > happy :-) Let's see! There's a lot to cover for v3 :sweat_smile:.I think I got the general idea that Lee proposed. I'll probably try it so we can have a taste of it in v3. If needed we can then rollback (not ideal, but that's life). - Nuno Sá
diff --git a/drivers/mfd/adp5585.c b/drivers/mfd/adp5585.c index fafe3ad93ea196e1eb8e79fecba58f36f12167eb..c3586c0d6aa2e7e7d94667993410610be7fc3672 100644 --- a/drivers/mfd/adp5585.c +++ b/drivers/mfd/adp5585.c @@ -25,6 +25,13 @@ static const struct mfd_cell adp5585_devs[] = { }; +static const struct mfd_cell adp5589_devs[] = { + MFD_CELL_NAME("adp5589-keys"), + MFD_CELL_NAME("adp5589-gpio"), + MFD_CELL_NAME("adp5589-pwm"), + +}; + static const struct regmap_range adp5585_volatile_ranges[] = { regmap_reg_range(ADP5585_ID, ADP5585_GPI_STATUS_B), }; @@ -34,6 +41,15 @@ static const struct regmap_access_table adp5585_volatile_regs = { .n_yes_ranges = ARRAY_SIZE(adp5585_volatile_ranges), }; +static const struct regmap_range adp5589_volatile_ranges[] = { + regmap_reg_range(ADP5585_ID, ADP5589_GPI_STATUS_C), +}; + +static const struct regmap_access_table adp5589_volatile_regs = { + .yes_ranges = adp5589_volatile_ranges, + .n_yes_ranges = ARRAY_SIZE(adp5589_volatile_ranges), +}; + /* * Chip variants differ in the default configuration of pull-up and pull-down * resistors, and therefore have different default register values: @@ -77,10 +93,52 @@ static const u8 adp5585_regmap_defaults_04[ADP5585_MAX_REG + 1] = { /* 0x38 */ 0x00, 0x00, 0x00, 0x00, 0x00, }; +static const u8 adp5589_regmap_defaults_00[ADP5589_MAX_REG + 1] = { + /* 0x00 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + /* 0x08 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + /* 0x10 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + /* 0x18 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + /* 0x20 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + /* 0x28 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + /* 0x30 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + /* 0x38 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + /* 0x40 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + /* 0x48 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, +}; + +static const u8 adp5589_regmap_defaults_01[ADP5589_MAX_REG + 1] = { + /* 0x00 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + /* 0x08 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + /* 0x10 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + /* 0x18 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + /* 0x20 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + /* 0x28 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + /* 0x30 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + /* 0x38 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x20, 0x00, 0x00, + /* 0x40 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + /* 0x48 */ 0x00, 0x00, 0x00, 0x00, 0x20, 0x00, 0x00, +}; + +static const u8 adp5589_regmap_defaults_02[ADP5589_MAX_REG + 1] = { + /* 0x00 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + /* 0x08 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + /* 0x10 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + /* 0x18 */ 0x00, 0x41, 0x01, 0x00, 0x11, 0x04, 0x00, 0x00, + /* 0x20 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + /* 0x28 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + /* 0x30 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + /* 0x38 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + /* 0x40 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + /* 0x48 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, +}; + enum adp5585_regmap_type { ADP5585_REGMAP_00, ADP5585_REGMAP_02, ADP5585_REGMAP_04, + ADP5589_REGMAP_00, + ADP5589_REGMAP_01, + ADP5589_REGMAP_02, }; static const struct regmap_config adp5585_regmap_configs[] = { @@ -111,6 +169,131 @@ static const struct regmap_config adp5585_regmap_configs[] = { .reg_defaults_raw = adp5585_regmap_defaults_04, .num_reg_defaults_raw = sizeof(adp5585_regmap_defaults_04), }, + [ADP5589_REGMAP_00] = { + .reg_bits = 8, + .val_bits = 8, + .max_register = ADP5589_MAX_REG, + .volatile_table = &adp5589_volatile_regs, + .cache_type = REGCACHE_MAPLE, + .reg_defaults_raw = adp5589_regmap_defaults_00, + .num_reg_defaults_raw = sizeof(adp5589_regmap_defaults_00), + }, + [ADP5589_REGMAP_01] = { + .reg_bits = 8, + .val_bits = 8, + .max_register = ADP5589_MAX_REG, + .volatile_table = &adp5589_volatile_regs, + .cache_type = REGCACHE_MAPLE, + .reg_defaults_raw = adp5589_regmap_defaults_01, + .num_reg_defaults_raw = sizeof(adp5589_regmap_defaults_01), + }, + [ADP5589_REGMAP_02] = { + .reg_bits = 8, + .val_bits = 8, + .max_register = ADP5589_MAX_REG, + .volatile_table = &adp5589_volatile_regs, + .cache_type = REGCACHE_MAPLE, + .reg_defaults_raw = adp5589_regmap_defaults_02, + .num_reg_defaults_raw = sizeof(adp5589_regmap_defaults_02), + }, +}; + +static const struct adp5585_regs adp5585_regs = { + .debounce_dis_a = ADP5585_DEBOUNCE_DIS_A, + .rpull_cfg_a = ADP5585_RPULL_CONFIG_A, + .gpo_data_a = ADP5585_GPO_DATA_OUT_A, + .gpo_out_a = ADP5585_GPO_OUT_MODE_A, + .gpio_dir_a = ADP5585_GPIO_DIRECTION_A, + .gpi_stat_a = ADP5585_GPI_STATUS_A, + .pwm_cfg = ADP5585_PWM_CFG, + .pwm_offt_low = ADP5585_PWM_OFFT_LOW, + .pwm_ont_low = ADP5585_PWM_ONT_LOW, + .gen_cfg = ADP5585_GENERAL_CFG, + .ext_cfg = ADP5585_PIN_CONFIG_C, +}; + +static const struct adp5585_regs adp5589_regs = { + .debounce_dis_a = ADP5589_DEBOUNCE_DIS_A, + .rpull_cfg_a = ADP5589_RPULL_CONFIG_A, + .gpo_data_a = ADP5589_GPO_DATA_OUT_A, + .gpo_out_a = ADP5589_GPO_OUT_MODE_A, + .gpio_dir_a = ADP5589_GPIO_DIRECTION_A, + .gpi_stat_a = ADP5589_GPI_STATUS_A, + .pwm_cfg = ADP5589_PWM_CFG, + .pwm_offt_low = ADP5589_PWM_OFFT_LOW, + .pwm_ont_low = ADP5589_PWM_ONT_LOW, + .gen_cfg = ADP5589_GENERAL_CFG, + .ext_cfg = ADP5589_PIN_CONFIG_D, +}; + +static const struct adp5585_info adp5585_info = { + .adp5585_devs = adp5585_devs, + .regmap_config = &adp5585_regmap_configs[ADP5585_REGMAP_00], + .n_devs = ARRAY_SIZE(adp5585_devs), + .id = ADP5585_MAN_ID_VALUE, + .regs = &adp5585_regs, + .max_rows = ADP5585_MAX_ROW_NUM, + .max_cols = ADP5585_MAX_COL_NUM, +}; + +static const struct adp5585_info adp5585_01_info = { + .adp5585_devs = adp5585_devs, + .regmap_config = &adp5585_regmap_configs[ADP5585_REGMAP_00], + .n_devs = ARRAY_SIZE(adp5585_devs), + .id = ADP5585_MAN_ID_VALUE, + .regs = &adp5585_regs, + .max_rows = ADP5585_MAX_ROW_NUM, + .max_cols = ADP5585_MAX_COL_NUM, +}; + +static const struct adp5585_info adp5585_02_info = { + .adp5585_devs = adp5585_devs, + .regmap_config = &adp5585_regmap_configs[ADP5585_REGMAP_02], + .n_devs = ARRAY_SIZE(adp5585_devs), + .id = ADP5585_MAN_ID_VALUE, + .regs = &adp5585_regs, + .max_rows = ADP5585_MAX_ROW_NUM, + .max_cols = ADP5585_MAX_COL_NUM, +}; + +static const struct adp5585_info adp5585_04_info = { + .adp5585_devs = adp5585_devs, + .regmap_config = &adp5585_regmap_configs[ADP5585_REGMAP_04], + .n_devs = ARRAY_SIZE(adp5585_devs), + .id = ADP5585_MAN_ID_VALUE, + .regs = &adp5585_regs, + .max_rows = ADP5585_MAX_ROW_NUM, + .max_cols = ADP5585_MAX_COL_NUM, +}; + +static const struct adp5585_info adp5589_info = { + .adp5585_devs = adp5589_devs, + .regmap_config = &adp5585_regmap_configs[ADP5589_REGMAP_00], + .n_devs = ARRAY_SIZE(adp5589_devs), + .id = ADP5589_MAN_ID_VALUE, + .regs = &adp5589_regs, + .max_rows = ADP5589_MAX_ROW_NUM, + .max_cols = ADP5589_MAX_COL_NUM, +}; + +static const struct adp5585_info adp5589_01_info = { + .adp5585_devs = adp5589_devs, + .regmap_config = &adp5585_regmap_configs[ADP5589_REGMAP_01], + .n_devs = ARRAY_SIZE(adp5589_devs), + .id = ADP5589_MAN_ID_VALUE, + .regs = &adp5589_regs, + .max_rows = ADP5589_MAX_ROW_NUM, + .max_cols = ADP5589_MAX_COL_NUM, +}; + +static const struct adp5585_info adp5589_02_info = { + .adp5585_devs = adp5589_devs, + .regmap_config = &adp5585_regmap_configs[ADP5589_REGMAP_02], + .n_devs = ARRAY_SIZE(adp5589_devs), + .id = ADP5589_MAN_ID_VALUE, + .regs = &adp5589_regs, + .max_rows = ADP5589_MAX_ROW_NUM, + .max_cols = ADP5589_MAX_COL_NUM, }; static void adp5585_osc_disable(void *data) @@ -122,7 +305,7 @@ static void adp5585_osc_disable(void *data) static int adp5585_i2c_probe(struct i2c_client *i2c) { - const struct regmap_config *regmap_config; + const struct adp5585_info *info; struct adp5585_dev *adp5585; unsigned int id; int ret; @@ -133,8 +316,13 @@ static int adp5585_i2c_probe(struct i2c_client *i2c) i2c_set_clientdata(i2c, adp5585); - regmap_config = i2c_get_match_data(i2c); - adp5585->regmap = devm_regmap_init_i2c(i2c, regmap_config); + info = i2c_get_match_data(i2c); + if (!info) + return -ENODEV; + + adp5585->info = info; + + adp5585->regmap = devm_regmap_init_i2c(i2c, info->regmap_config); if (IS_ERR(adp5585->regmap)) return dev_err_probe(&i2c->dev, PTR_ERR(adp5585->regmap), "Failed to initialize register map\n"); @@ -144,7 +332,8 @@ static int adp5585_i2c_probe(struct i2c_client *i2c) return dev_err_probe(&i2c->dev, ret, "Failed to read device ID\n"); - if ((id & ADP5585_MAN_ID_MASK) != ADP5585_MAN_ID_VALUE) + id &= ADP5585_MAN_ID_MASK; + if (id != adp5585->info->id) return dev_err_probe(&i2c->dev, -ENODEV, "Invalid device ID 0x%02x\n", id); @@ -158,8 +347,8 @@ static int adp5585_i2c_probe(struct i2c_client *i2c) return ret; ret = devm_mfd_add_devices(&i2c->dev, PLATFORM_DEVID_AUTO, - adp5585_devs, ARRAY_SIZE(adp5585_devs), - NULL, 0, NULL); + adp5585->info->adp5585_devs, + adp5585->info->n_devs, NULL, 0, NULL); if (ret) return dev_err_probe(&i2c->dev, ret, "Failed to add child devices\n"); @@ -191,19 +380,31 @@ static DEFINE_SIMPLE_DEV_PM_OPS(adp5585_pm, adp5585_suspend, adp5585_resume); static const struct of_device_id adp5585_of_match[] = { { .compatible = "adi,adp5585-00", - .data = &adp5585_regmap_configs[ADP5585_REGMAP_00], + .data = &adp5585_info, }, { .compatible = "adi,adp5585-01", - .data = &adp5585_regmap_configs[ADP5585_REGMAP_00], + .data = &adp5585_01_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, + }, { + .compatible = "adi,adp5589-00", + .data = &adp5589_info, + }, { + .compatible = "adi,adp5589-01", + .data = &adp5589_01_info, + }, { + .compatible = "adi,adp5589-02", + .data = &adp5589_02_info, + }, { + .compatible = "adi,adp5589", + .data = &adp5589_info, }, { /* sentinel */ } }; diff --git a/include/linux/mfd/adp5585.h b/include/linux/mfd/adp5585.h index 016033cd68e46757aca86d21dd37025fd354b801..dffe1449de01dacf8fe78cf0e87d1f176d11f620 100644 --- a/include/linux/mfd/adp5585.h +++ b/include/linux/mfd/adp5585.h @@ -104,9 +104,11 @@ #define ADP5585_INT_CFG BIT(1) #define ADP5585_RST_CFG BIT(0) #define ADP5585_INT_EN 0x3c - #define ADP5585_MAX_REG ADP5585_INT_EN +#define ADP5585_MAX_ROW_NUM 6 +#define ADP5585_MAX_COL_NUM 5 + /* * Bank 0 covers pins "GPIO 1/R0" to "GPIO 6/R5", numbered 0 to 5 by the * driver, and bank 1 covers pins "GPIO 7/C0" to "GPIO 11/C4", numbered 6 to @@ -117,10 +119,63 @@ #define ADP5585_BANK(n) ((n) >= 6 ? 1 : 0) #define ADP5585_BIT(n) ((n) >= 6 ? BIT((n) - 6) : BIT(n)) +/* ADP5589 */ +#define ADP5589_MAN_ID_VALUE 0x10 +#define ADP5589_GPI_STATUS_A 0x16 +#define ADP5589_GPI_STATUS_C 0x18 +#define ADP5589_RPULL_CONFIG_A 0x19 +#define ADP5589_DEBOUNCE_DIS_A 0x27 +#define ADP5589_GPO_DATA_OUT_A 0x2a +#define ADP5589_GPO_OUT_MODE_A 0x2d +#define ADP5589_GPIO_DIRECTION_A 0x30 +#define ADP5589_PWM_OFFT_LOW 0x3e +#define ADP5589_PWM_ONT_LOW 0x40 +#define ADP5589_PWM_CFG 0x42 +#define ADP5589_PIN_CONFIG_D 0x4C +#define ADP5589_GENERAL_CFG 0x4d +#define ADP5589_INT_EN 0x4e +#define ADP5589_MAX_REG ADP5589_INT_EN + +#define ADP5589_MAX_ROW_NUM 8 +#define ADP5589_MAX_COL_NUM 11 + +/* + * Bank 0 covers pins "GPIO 1/R0" to "GPIO 8/R7", numbered 0 to 7 by the + * driver, bank 1 covers pins "GPIO 9/C0" to "GPIO 16/C7", numbered 8 to + * 15 and bank 3 covers pins "GPIO 17/C8" to "GPIO 19/C10", numbered 16 to 18. + */ +#define ADP5589_BANK(n) ((n) >> 3) +#define ADP5589_BIT(n) BIT((n) & 0x7) + +struct adp5585_regs { + unsigned int debounce_dis_a; + unsigned int rpull_cfg_a; + unsigned int gpo_data_a; + unsigned int gpo_out_a; + unsigned int gpio_dir_a; + unsigned int gpi_stat_a; + unsigned int pwm_cfg; + unsigned int pwm_offt_low; + unsigned int pwm_ont_low; + unsigned int gen_cfg; + unsigned int ext_cfg; +}; + +struct adp5585_info { + const struct mfd_cell *adp5585_devs; + const struct regmap_config *regmap_config; + const struct adp5585_regs *regs; + unsigned int n_devs; + unsigned int id; + u8 max_rows; + u8 max_cols; +}; + struct regmap; struct adp5585_dev { struct regmap *regmap; + const struct adp5585_info *info; }; #endif