From: "Nuno Sá" <noname.nuno@gmail.com>
To: Lee Jones <lee@kernel.org>, 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: Tue, 27 May 2025 11:20:11 +0100 [thread overview]
Message-ID: <e54e2ae8a1231ce44e9a0becd14e210f3c8f2bca.camel@gmail.com> (raw)
In-Reply-To: <20250523151514.GI1378991@google.com>
On Fri, 2025-05-23 at 16:15 +0100, Lee Jones wrote:
> 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..53ac46365e56874a05855f1df184371714
> > 8f181a 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?
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á
> > +};
> > +
> > +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.
ack
>
> > + &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..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.
>
> > enum adp5585_variant variant;
> > + unsigned int id;
> > };
> >
> > #endif
> >
> > --
> > 2.49.0
> >
> >
next prev parent reply other threads:[~2025-05-27 10:20 UTC|newest]
Thread overview: 45+ 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á via B4 Relay
2025-05-21 13:02 ` [PATCH v4 01/20] dt-bindings: mfd: adp5585: ease on the required properties Nuno Sá via B4 Relay
2025-05-21 13:02 ` [PATCH v4 02/20] mfd: adp5585: only add devices given in FW 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á 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á 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á via B4 Relay
2025-05-21 13:02 ` [PATCH v4 06/20] mfd: adp5585: refactor how regmap defaults are handled 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á via B4 Relay
2025-05-23 15:15 ` Lee Jones
2025-05-27 10:20 ` Nuno Sá [this message]
2025-05-21 13:02 ` [PATCH v4 08/20] mfd: adp5585: add a per chip reg struture 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á via B4 Relay
2025-05-21 13:03 ` [PATCH v4 10/20] pwm: adp5585: add support for adp5589 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á via B4 Relay
2025-05-21 13:03 ` [PATCH v4 12/20] mfd: adp5585: add support for event handling 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á 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á 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á via B4 Relay
2025-05-21 13:03 ` [PATCH v4 16/20] Input: adp5585: Add Analog Devices ADP5585/89 support Nuno Sá via B4 Relay
2025-05-21 13:03 ` [PATCH v4 17/20] Input: adp5589: remove the driver Nuno Sá via B4 Relay
2025-05-21 13:03 ` [PATCH v4 18/20] mfd: adp5585: support getting vdd regulator 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á via B4 Relay
2025-05-21 13:03 ` [PATCH v4 20/20] mfd: adp5585: add support for a reset pin 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=e54e2ae8a1231ce44e9a0becd14e210f3c8f2bca.camel@gmail.com \
--to=noname.nuno@gmail.com \
--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=lee@kernel.org \
--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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).