Re: [PATCH v4 07/20] mfd: adp5585: add support for adp5589
From: Nuno Sá <hidden>
Date: 2025-05-27 10:20:10
Also in:
linux-devicetree, linux-gpio, linux-pwm
On Fri, 2025-05-23 at 16:15 +0100, Lee Jones wrote:
On Wed, 21 May 2025, Nuno Sá via B4 Relay wrote:quoted
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. Signed-off-by: Nuno Sá <nuno.sa@analog.com> --- drivers/mfd/adp5585.c | 139 +++++++++++++++++++++++++++++++++++++++-- --- include/linux/mfd/adp5585.h | 11 ++++ 2 files changed, 134 insertions(+), 16 deletions(-)diff --git a/drivers/mfd/adp5585.c b/drivers/mfd/adp5585.cindex 672f3468bda5be6af85a5982c3626053b4cb59bf..53ac46365e56874a05855f1df184371714 8f181a 100644--- a/drivers/mfd/adp5585.c +++ b/drivers/mfd/adp5585.c@@ -29,6 +29,11 @@ static const struct mfd_celladp5585_devs[ADP5585_DEV_MAX] = { MFD_CELL_NAME("adp5585-pwm"), }; +static const struct mfd_cell adp5589_devs[] = { + 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), };@@ -38,6 +43,15 @@ static const struct regmap_access_tableadp5585_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:@@ -81,6 +95,45 @@ static const u8adp5585_regmap_defaults_04[ADP5585_MAX_REG + 1] = { /* 0x38 */ 0x00, 0x00, 0x00, 0x00, 0x00, }; +static const u8 adp5589_regmap_defaults_00[ADP5589_MAX_REG + 1] = {Why +1?quoted
+ /* 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, +}; + /* -1 since the enum starts at 1 for error checking in i2c_get_match_data()*/ static const u8 *adp5585_regmap_defaults[ADP5585_MAX - 1] = { [ADP5585_00 - 1] = adp5585_regmap_defaults_00,@@ -88,6 +141,9 @@ static const u8 *adp5585_regmap_defaults[ADP5585_MAX - 1]= { [ADP5585_02 - 1] = adp5585_regmap_defaults_02, [ADP5585_03 - 1] = adp5585_regmap_defaults_00, [ADP5585_04 - 1] = adp5585_regmap_defaults_04, + [ADP5589_00 - 1] = adp5589_regmap_defaults_00, + [ADP5589_01 - 1] = adp5589_regmap_defaults_01, + [ADP5589_02 - 1] = adp5589_regmap_defaults_02, }; static const struct regmap_config adp5585_regmap_config_template = {@@ -99,11 +155,39 @@ static const struct regmap_configadp5585_regmap_config_template = { .num_reg_defaults_raw = ADP5585_MAX_REG + 1, }; -static void adp5585_fill_regmap_config(const struct adp5585_dev *adp5585, +static const struct regmap_config adp5589_regmap_config_template = { + .reg_bits = 8, + .val_bits = 8, + .max_register = ADP5589_MAX_REG, + .volatile_table = &adp5589_volatile_regs, + .cache_type = REGCACHE_MAPLE, + .num_reg_defaults_raw = ADP5589_MAX_REG + 1,This is 0x4F, but there are only 0x4E values?
Well, you want to write all registers while i < num_reg_defaults_raw. So, to actually write in ADP5589_MAX_REG, you need + 1. IOW, if your max register address is 8 and you want to write it, your array map needs to be able go until index 8. - Nuno Sá
quoted
+}; + +static int adp5585_fill_variant_config(struct adp5585_dev *adp5585, struct regmap_config *regmap_config) { - *regmap_config = adp5585_regmap_config_template; + switch (adp5585->variant) { + case ADP5585_00: + case ADP5585_01: + case ADP5585_02: + case ADP5585_03: + case ADP5585_04: + *regmap_config = adp5585_regmap_config_template; + adp5585->id = ADP5585_MAN_ID_VALUE; + break; + case ADP5589_00: + case ADP5589_01: + case ADP5589_02: + *regmap_config = adp5589_regmap_config_template; + adp5585->id = ADP5589_MAN_ID_VALUE; + break; + default: + return -ENODEV;If you take my previous advice, this will become ERR_PTR();quoted
+ } + regmap_config->reg_defaults_raw = adp5585_regmap_defaults[adp5585-quoted
variant - 1];+ return 0; } static void adp5585_remove_devices(void *dev)@@ -111,29 +195,35 @@ static void adp5585_remove_devices(void *dev)mfd_remove_devices(dev); } -static int adp5585_add_devices(struct device *dev) +static int adp5585_add_devices(const struct adp5585_dev *adp5585) { + const struct mfd_cell *cells; int ret; - if (device_property_present(dev, "#pwm-cells")) { - ret = mfd_add_devices(dev, PLATFORM_DEVID_AUTO, - &adp5585_devs[ADP5585_DEV_PWM], 1, NULL, 0, NULL); + if (adp5585->id == ADP5585_MAN_ID_VALUE) + cells = adp5585_devs; + else + cells = adp5589_devs; + + if (device_property_present(adp5585->dev, "#pwm-cells")) { + ret = mfd_add_devices(adp5585->dev, PLATFORM_DEVID_AUTO,Instead of making all of these changes, add the following at the top: struct device *dev = adp5585->dev; ... and continue using the short form.
ack
quoted
+ &cells[ADP5585_DEV_PWM], 1, NULL, 0, NULL); if (ret) - return dev_err_probe(dev, ret, "Failed to add pwm device\n"); + return dev_err_probe(adp5585->dev, ret, "Failed to add pwm device\n"); } - if (device_property_present(dev, "#gpio-cells")) { - ret = mfd_add_devices(dev, PLATFORM_DEVID_AUTO, - &adp5585_devs[ADP5585_DEV_GPIO], 1, NULL, 0, NULL); + if (device_property_present(adp5585->dev, "#gpio-cells")) { + ret = mfd_add_devices(adp5585->dev, PLATFORM_DEVID_AUTO, + &cells[ADP5585_DEV_GPIO], 1, NULL, 0, NULL); if (ret) { - ret = dev_err_probe(dev, ret, "Failed to add gpio device\n"); + ret = dev_err_probe(adp5585->dev, ret, "Failed to add gpio device\n"); goto out_error; } } - return devm_add_action_or_reset(dev, adp5585_remove_devices, dev); + return devm_add_action_or_reset(adp5585->dev, adp5585_remove_devices, adp5585->dev); out_error: - mfd_remove_devices(dev); + mfd_remove_devices(adp5585->dev); return ret; }@@ -161,19 +251,24 @@ static int adp5585_i2c_probe(struct i2c_client *i2c)if (!adp5585->variant) return -ENODEV; - adp5585_fill_regmap_config(adp5585, ®map_config); + ret = adp5585_fill_variant_config(adp5585, ®map_config); + if (ret) + return ret; adp5585->regmap = devm_regmap_init_i2c(i2c, ®map_config); if (IS_ERR(adp5585->regmap)) return dev_err_probe(&i2c->dev, PTR_ERR(adp5585->regmap), "Failed to initialize register map\n"); + adp5585->dev = &i2c->dev; + ret = regmap_read(adp5585->regmap, ADP5585_ID, &id); if (ret) 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->id) return dev_err_probe(&i2c->dev, -ENODEV, "Invalid device ID 0x%02x\n", id);@@ -193,7 +288,7 @@ static int adp5585_i2c_probe(struct i2c_client *i2c)if (ret) return ret; - return adp5585_add_devices(&i2c->dev); + return adp5585_add_devices(adp5585); } static int adp5585_suspend(struct device *dev)@@ -233,6 +328,18 @@ static const struct of_device_id adp5585_of_match[] = {}, { .compatible = "adi,adp5585-04", .data = (void *)ADP5585_04, + }, { + .compatible = "adi,adp5589-00", + .data = (void *)ADP5589_00, + }, { + .compatible = "adi,adp5589-01", + .data = (void *)ADP5589_01, + }, { + .compatible = "adi,adp5589-02", + .data = (void *)ADP5589_02, + }, { + .compatible = "adi,adp5589", + .data = (void *)ADP5589_00, }, { /* sentinel */ } };diff --git a/include/linux/mfd/adp5585.h b/include/linux/mfd/adp5585.hindex 2813b20e638b6e73ef198e43af07ef29ff25f273..40c87981e5a24f8a175cc41e38b1495ed9 f194e5 100644--- a/include/linux/mfd/adp5585.h +++ b/include/linux/mfd/adp5585.h@@ -117,6 +117,12 @@#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_C 0x18 +#define ADP5589_INT_EN 0x4e +#define ADP5589_MAX_REG ADP5589_INT_EN + struct regmap; enum adp5585_variant {@@ -125,12 +131,17 @@ enum adp5585_variant {ADP5585_02, ADP5585_03, ADP5585_04, + ADP5589_00, + ADP5589_01, + ADP5589_02, ADP5585_MAX }; struct adp5585_dev { struct regmap *regmap; + struct device *dev;Can you pop this at the top please. It has a higher standing (in my own head *twitch*) than regmap.
Sure.
quoted
enum adp5585_variant variant; + unsigned int id; }; #endif -- 2.49.0