From: Lee Jones <lee@kernel.org>
To: nuno.sa@analog.com
Cc: linux-gpio@vger.kernel.org, linux-pwm@vger.kernel.org,
devicetree@vger.kernel.org, linux-input@vger.kernel.org,
"Rob Herring" <robh@kernel.org>,
"Krzysztof Kozlowski" <krzk+dt@kernel.org>,
"Conor Dooley" <conor+dt@kernel.org>,
"Uwe Kleine-König" <ukleinek@kernel.org>,
"Linus Walleij" <linus.walleij@linaro.org>,
"Bartosz Golaszewski" <brgl@bgdev.pl>,
"Dmitry Torokhov" <dmitry.torokhov@gmail.com>,
"Laurent Pinchart" <laurent.pinchart@ideasonboard.com>,
"Liu Ying" <victor.liu@nxp.com>
Subject: Re: [PATCH v4 07/20] mfd: adp5585: add support for adp5589
Date: Fri, 23 May 2025 16:15:14 +0100 [thread overview]
Message-ID: <20250523151514.GI1378991@google.com> (raw)
In-Reply-To: <20250521-dev-adp5589-fw-v4-7-f2c988d7a7a0@analog.com>
On Wed, 21 May 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.
>
> 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.c
> index 672f3468bda5be6af85a5982c3626053b4cb59bf..53ac46365e56874a05855f1df1843717148f181a 100644
> --- a/drivers/mfd/adp5585.c
> +++ b/drivers/mfd/adp5585.c
> @@ -29,6 +29,11 @@ static const struct mfd_cell adp5585_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_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:
> @@ -81,6 +95,45 @@ 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] = {
Why +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,
> +};
> +
> /* -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_config adp5585_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?
> +};
> +
> +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();
> + }
> +
> regmap_config->reg_defaults_raw = adp5585_regmap_defaults[adp5585->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.
> + &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.h
> index 2813b20e638b6e73ef198e43af07ef29ff25f273..40c87981e5a24f8a175cc41e38b1495ed9f194e5 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.
> enum adp5585_variant variant;
> + unsigned int id;
> };
>
> #endif
>
> --
> 2.49.0
>
>
--
Lee Jones [李琼斯]
next prev parent reply other threads:[~2025-05-23 15:15 UTC|newest]
Thread overview: 66+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-05-21 13:02 [PATCH v4 00/20] mfd: adp5585: support keymap events and drop legacy Input driver Nuno Sá
2025-05-21 13:02 ` Nuno Sá via B4 Relay
2025-05-21 13:02 ` [PATCH v4 01/20] dt-bindings: mfd: adp5585: ease on the required properties Nuno Sá
2025-05-21 13:02 ` Nuno Sá via B4 Relay
2025-05-21 13:02 ` [PATCH v4 02/20] mfd: adp5585: only add devices given in FW Nuno Sá
2025-05-21 13:02 ` Nuno Sá via B4 Relay
2025-05-23 14:51 ` Lee Jones
2025-05-23 15:07 ` Nuno Sá
2025-05-23 15:19 ` Lee Jones
2025-05-23 15:45 ` Nuno Sá
2025-06-12 14:17 ` Lee Jones
2025-05-21 13:02 ` [PATCH v4 03/20] mfd: adp5585: enable oscilator during probe Nuno Sá
2025-05-21 13:02 ` Nuno Sá via B4 Relay
2025-06-12 14:20 ` Lee Jones
2025-06-12 14:40 ` Nuno Sá
2025-06-12 15:20 ` Lee Jones
2025-06-13 9:43 ` Nuno Sá
2025-05-21 13:02 ` [PATCH v4 04/20] mfd: adp5585: make use of MFD_CELL_NAME() Nuno Sá
2025-05-21 13:02 ` Nuno Sá via B4 Relay
2025-05-23 14:53 ` Lee Jones
2025-05-21 13:02 ` [PATCH v4 05/20] dt-bindings: mfd: adp5585: document adp5589 I/O expander Nuno Sá
2025-05-21 13:02 ` Nuno Sá via B4 Relay
2025-05-21 13:02 ` [PATCH v4 06/20] mfd: adp5585: refactor how regmap defaults are handled Nuno Sá
2025-05-21 13:02 ` Nuno Sá via B4 Relay
2025-05-23 15:03 ` Lee Jones
2025-05-27 10:08 ` Nuno Sá
2025-05-21 13:02 ` [PATCH v4 07/20] mfd: adp5585: add support for adp5589 Nuno Sá
2025-05-21 13:02 ` Nuno Sá via B4 Relay
2025-05-23 15:15 ` Lee Jones [this message]
2025-05-27 10:20 ` Nuno Sá
2025-05-21 13:02 ` [PATCH v4 08/20] mfd: adp5585: add a per chip reg struture Nuno Sá
2025-05-21 13:02 ` Nuno Sá via B4 Relay
2025-06-12 14:22 ` Lee Jones
2025-05-21 13:02 ` [PATCH v4 09/20] gpio: adp5585: add support for the adp5589 expander Nuno Sá
2025-05-21 13:02 ` Nuno Sá via B4 Relay
2025-05-21 13:03 ` [PATCH v4 10/20] pwm: adp5585: add support for adp5589 Nuno Sá
2025-05-21 13:03 ` Nuno Sá via B4 Relay
2025-05-21 13:03 ` [PATCH v4 11/20] dt-bindings: mfd: adp5585: add properties for input events Nuno Sá
2025-05-21 13:03 ` Nuno Sá via B4 Relay
2025-05-21 13:03 ` [PATCH v4 12/20] mfd: adp5585: add support for event handling Nuno Sá
2025-05-21 13:03 ` Nuno Sá via B4 Relay
2025-06-12 14:29 ` Lee Jones
2025-05-21 13:03 ` [PATCH v4 13/20] mfd: adp5585: support reset and unlock events Nuno Sá
2025-05-21 13:03 ` Nuno Sá via B4 Relay
2025-06-12 14:55 ` Lee Jones
2025-06-13 9:48 ` Nuno Sá
2025-06-13 13:07 ` Lee Jones
2025-06-13 13:13 ` Nuno Sá
2025-06-13 13:27 ` Lee Jones
2025-05-21 13:03 ` [PATCH v4 14/20] mfd: adp5585: add support for input devices Nuno Sá
2025-05-21 13:03 ` Nuno Sá via B4 Relay
2025-06-12 15:16 ` Lee Jones
2025-05-21 13:03 ` [PATCH v4 15/20] gpio: adp5585: support gpi events Nuno Sá
2025-05-21 13:03 ` Nuno Sá via B4 Relay
2025-05-21 13:03 ` [PATCH v4 16/20] Input: adp5585: Add Analog Devices ADP5585/89 support Nuno Sá
2025-05-21 13:03 ` Nuno Sá via B4 Relay
2025-05-21 13:03 ` [PATCH v4 17/20] Input: adp5589: remove the driver Nuno Sá
2025-05-21 13:03 ` Nuno Sá via B4 Relay
2025-05-21 13:03 ` [PATCH v4 18/20] mfd: adp5585: support getting vdd regulator Nuno Sá
2025-05-21 13:03 ` Nuno Sá via B4 Relay
2025-06-12 15:17 ` Lee Jones
2025-05-21 13:03 ` [PATCH v4 19/20] dt-bindings: mfd: adp5585: document reset gpio Nuno Sá
2025-05-21 13:03 ` Nuno Sá via B4 Relay
2025-05-21 13:03 ` [PATCH v4 20/20] mfd: adp5585: add support for a reset pin Nuno Sá
2025-05-21 13:03 ` Nuno Sá via B4 Relay
2025-06-12 15:18 ` Lee Jones
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20250523151514.GI1378991@google.com \
--to=lee@kernel.org \
--cc=brgl@bgdev.pl \
--cc=conor+dt@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=dmitry.torokhov@gmail.com \
--cc=krzk+dt@kernel.org \
--cc=laurent.pinchart@ideasonboard.com \
--cc=linus.walleij@linaro.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-input@vger.kernel.org \
--cc=linux-pwm@vger.kernel.org \
--cc=nuno.sa@analog.com \
--cc=robh@kernel.org \
--cc=ukleinek@kernel.org \
--cc=victor.liu@nxp.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.