| Message ID | 20240528190315.3865-3-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ADP5585 GPIO expander, PWM and keypad controller support | expand |
Tue, May 28, 2024 at 10:03:12PM +0300, Laurent Pinchart kirjoitti: > From: Haibo Chen <haibo.chen@nxp.com> > > The ADP5585 is a 10/11 input/output port expander with a built in keypad > matrix decoder, programmable logic, reset generator, and PWM generator. > This driver supports the chip by modelling it as an MFD device, with two > child devices for the GPIO and PWM functions. > > The driver is derived from an initial implementation from NXP, available > in commit 8059835bee19 ("MLK-25917-1 mfd: adp5585: add ADI adp5585 core > support") in their BSP kernel tree. It has been extensively rewritten. ... > + tristate "Analog Devices ADP5585 MFD driver" > + select MFD_CORE > + select REGMAP_I2C > + depends on I2C && OF Why OF? No COMPILE_TEST? ... + array_size.h + device.h // e.g., devm_kzalloc() > +#include <linux/module.h> > +#include <linux/moduleparam.h> > +#include <linux/init.h> > +#include <linux/slab.h> > +#include <linux/i2c.h> > +#include <linux/of.h> > +#include <linux/of_device.h> You don't need them, instead of proxying... > +#include <linux/mfd/core.h> > +#include <linux/mfd/adp5585.h> m is earlier than 'o', but with above drop no more issue :-) ...just include mod_devicetable.h. > +#include <linux/regmap.h> + types.h // e.g., u8 ... > + regmap_config = of_device_get_match_data(&i2c->dev); We have i2c_get_match_data(). ... > +#ifndef __LINUX_MFD_ADP5585_H_ > +#define __LINUX_MFD_ADP5585_H_ > + > +#include <linux/bits.h> ... > +#define ADP5585_MAN_ID(v) (((v) & 0xf0) >> 4) GENMASK() ... > +#define ADP5585_Rx_PULL_CFG_MASK (3) GENMASK() Why parentheses in all of them, btw? ... > +#define ADP5585_C4_EXTEND_CFG_MASK (1U << 6) > +#define ADP5585_R4_EXTEND_CFG_MASK (1U << 5) > +#define ADP5585_R3_EXTEND_CFG_MASK (3U << 2) > +#define ADP5585_R0_EXTEND_CFG_MASK (1U << 0) > +#define ADP5585_OSC_FREQ_MASK (3U << 5) BIT() / GENMASK() > +#endif
On Tue, May 28, 2024 at 10:27:34PM +0300, Andy Shevchenko wrote: > Tue, May 28, 2024 at 10:03:12PM +0300, Laurent Pinchart kirjoitti: > > From: Haibo Chen <haibo.chen@nxp.com> > > > > The ADP5585 is a 10/11 input/output port expander with a built in keypad > > matrix decoder, programmable logic, reset generator, and PWM generator. > > This driver supports the chip by modelling it as an MFD device, with two > > child devices for the GPIO and PWM functions. > > > > The driver is derived from an initial implementation from NXP, available > > in commit 8059835bee19 ("MLK-25917-1 mfd: adp5585: add ADI adp5585 core > > support") in their BSP kernel tree. It has been extensively rewritten. > > ... > > > + tristate "Analog Devices ADP5585 MFD driver" > > + select MFD_CORE > > + select REGMAP_I2C > > + depends on I2C && OF > > Why OF? Because the driver works on OF systems only. > No COMPILE_TEST? The driver won't compile without CONFIG_I2C, so I can use depends on I2C depends on OF || COMPILE_TEST Do you think that's better ? > > ... > > + array_size.h > + device.h // e.g., devm_kzalloc() > > > +#include <linux/module.h> > > +#include <linux/moduleparam.h> > > +#include <linux/init.h> > > +#include <linux/slab.h> I'll drop those 3 headers, there's not needed anymore. > > +#include <linux/i2c.h> > > > +#include <linux/of.h> > > +#include <linux/of_device.h> > > You don't need them, instead of proxying... of.h for of_device_get_match_data() and of_match_ptr(). I'll drop the former, but I need the latter, so I'll keep of.h of_device.h for historical reasons probably, I'll drop it. > > +#include <linux/mfd/core.h> > > +#include <linux/mfd/adp5585.h> > > m is earlier than 'o', but with above drop no more issue :-) > > ...just include mod_devicetable.h. > > > +#include <linux/regmap.h> > > + types.h // e.g., u8 > > ... > > > + regmap_config = of_device_get_match_data(&i2c->dev); > > We have i2c_get_match_data(). Sounds good. > ... > > > +#ifndef __LINUX_MFD_ADP5585_H_ > > +#define __LINUX_MFD_ADP5585_H_ > > + > > +#include <linux/bits.h> > > ... > > > +#define ADP5585_MAN_ID(v) (((v) & 0xf0) >> 4) > > GENMASK() This is not a mask. Or do you mean (((v) & GENMASK(7, 4)) >> 4) ? I think that's overkill. > ... > > > +#define ADP5585_Rx_PULL_CFG_MASK (3) > > GENMASK() Not here, as this value is meant to be passed to ADP5585_Rx_PULL_CFG(). > Why parentheses in all of them, btw? Probably for consistency, but I don't mind dropping them. > ... > > > +#define ADP5585_C4_EXTEND_CFG_MASK (1U << 6) > > > +#define ADP5585_R4_EXTEND_CFG_MASK (1U << 5) > > > +#define ADP5585_R3_EXTEND_CFG_MASK (3U << 2) > > > +#define ADP5585_R0_EXTEND_CFG_MASK (1U << 0) > > > +#define ADP5585_OSC_FREQ_MASK (3U << 5) > > BIT() / GENMASK() I'll use GENMASK for the masks. > > +#endif
On Tue, May 28, 2024 at 11:13 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > On Tue, May 28, 2024 at 10:27:34PM +0300, Andy Shevchenko wrote: > > Tue, May 28, 2024 at 10:03:12PM +0300, Laurent Pinchart kirjoitti: ... > > > + depends on I2C && OF > > > > Why OF? > > Because the driver works on OF systems only. > > > No COMPILE_TEST? > > The driver won't compile without CONFIG_I2C, so I can use > > depends on I2C > depends on OF || COMPILE_TEST > > Do you think that's better ? I think that dropping OF completely is the best. OF || COMPILE_TEST would work as well, but I still don't know why we need this. ... > > + array_size.h > > + device.h // e.g., devm_kzalloc() > > > > > +#include <linux/module.h> > > > +#include <linux/moduleparam.h> > > > +#include <linux/init.h> > > > +#include <linux/slab.h> > > I'll drop those 3 headers, there's not needed anymore. > > > > +#include <linux/i2c.h> > > > > > +#include <linux/of.h> > > > +#include <linux/of_device.h> > > > > You don't need them, instead of proxying... > > of.h for of_device_get_match_data() and of_match_ptr(). I'll drop the > former, but I need the latter, so I'll keep of.h Why do you need of_match_ptr()? What for? > of_device.h for historical reasons probably, I'll drop it. > > > > +#include <linux/mfd/core.h> > > > +#include <linux/mfd/adp5585.h> > > > > m is earlier than 'o', but with above drop no more issue :-) > > > > ...just include mod_devicetable.h. > > > > > +#include <linux/regmap.h> > > > > + types.h // e.g., u8 I assume that all marked with + in my previous reply you agree on? ... > > > +#define ADP5585_MAN_ID(v) (((v) & 0xf0) >> 4) > > > > GENMASK() > > This is not a mask. Or do you mean > > (((v) & GENMASK(7, 4)) >> 4) > > ? Yes. > I think that's overkill. Why? You have a mask, use it for less error prone code. ... > > > +#define ADP5585_Rx_PULL_CFG_MASK (3) > > > > GENMASK() > > Not here, as this value is meant to be passed to ADP5585_Rx_PULL_CFG(). Why is it marked as a mask? Rename it to _ALL or alike. ... > > > +#define ADP5585_C4_EXTEND_CFG_MASK (1U << 6) > > > > > +#define ADP5585_R4_EXTEND_CFG_MASK (1U << 5) > > > > > +#define ADP5585_R3_EXTEND_CFG_MASK (3U << 2) > > > > > +#define ADP5585_R0_EXTEND_CFG_MASK (1U << 0) > > > > > +#define ADP5585_OSC_FREQ_MASK (3U << 5) > > > > BIT() / GENMASK() > > I'll use GENMASK for the masks. For a single bit the BIT() is okay, and TBH I don't remember if GENMASK() supports h == l cases.
Hi Andy, On Wed, May 29, 2024 at 08:44:26AM +0300, Andy Shevchenko wrote: > On Tue, May 28, 2024 at 11:13 PM Laurent Pinchart wrote: > > On Tue, May 28, 2024 at 10:27:34PM +0300, Andy Shevchenko wrote: > > > Tue, May 28, 2024 at 10:03:12PM +0300, Laurent Pinchart kirjoitti: > > ... > > > > > + depends on I2C && OF > > > > > > Why OF? > > > > Because the driver works on OF systems only. > > > > > No COMPILE_TEST? > > > > The driver won't compile without CONFIG_I2C, so I can use > > > > depends on I2C > > depends on OF || COMPILE_TEST > > > > Do you think that's better ? > > I think that dropping OF completely is the best. > OF || COMPILE_TEST would work as well, but I still don't know why we need this. For the same reason that many drivers depend on specific CONFIG_$ARCH. They can't run on other platforms, the dependency hides the symbol for users who can't use the driver. This driver works on OF platforms only. > ... > > > > + array_size.h > > > + device.h // e.g., devm_kzalloc() > > > > > > > +#include <linux/module.h> > > > > +#include <linux/moduleparam.h> > > > > +#include <linux/init.h> > > > > +#include <linux/slab.h> > > > > I'll drop those 3 headers, there's not needed anymore. > > > > > > +#include <linux/i2c.h> > > > > > > > +#include <linux/of.h> > > > > +#include <linux/of_device.h> > > > > > > You don't need them, instead of proxying... > > > > of.h for of_device_get_match_data() and of_match_ptr(). I'll drop the > > former, but I need the latter, so I'll keep of.h > > Why do you need of_match_ptr()? What for? That's actually not needed, I'll drop it. > > of_device.h for historical reasons probably, I'll drop it. > > > > > > +#include <linux/mfd/core.h> > > > > +#include <linux/mfd/adp5585.h> > > > > > > m is earlier than 'o', but with above drop no more issue :-) > > > > > > ...just include mod_devicetable.h. > > > > > > > +#include <linux/regmap.h> > > > > > > + types.h // e.g., u8 > > I assume that all marked with + in my previous reply you agree on? If I don't reply to a particular comment it means I agree with it and will address it, yes. > ... > > > > > +#define ADP5585_MAN_ID(v) (((v) & 0xf0) >> 4) > > > > > > GENMASK() > > > > This is not a mask. Or do you mean > > > > (((v) & GENMASK(7, 4)) >> 4) > > > > ? > > Yes. > > > I think that's overkill. > > Why? You have a mask, use it for less error prone code. I'll change this to diff --git a/drivers/mfd/adp5585.c b/drivers/mfd/adp5585.c index fa4092a5c97f..924758b8a3cd 100644 --- a/drivers/mfd/adp5585.c +++ b/drivers/mfd/adp5585.c @@ -125,7 +125,7 @@ static int adp5585_i2c_probe(struct i2c_client *i2c) return dev_err_probe(&i2c->dev, ret, "Failed to read device ID\n"); - if (ADP5585_MAN_ID(id) != ADP5585_MAN_ID_VALUE) + if (id & ADP5585_MAN_ID_MASK != ADP5585_MAN_ID_VALUE) return dev_err_probe(&i2c->dev, -ENODEV, "Invalid device ID 0x%02x\n", id); diff --git a/include/linux/mfd/adp5585.h b/include/linux/mfd/adp5585.h index f06a574afedf..f5776ee844dc 100644 --- a/include/linux/mfd/adp5585.h +++ b/include/linux/mfd/adp5585.h @@ -12,8 +12,8 @@ #include <linux/bits.h> #define ADP5585_ID 0x00 -#define ADP5585_MAN_ID(v) (((v) & 0xf0) >> 4) -#define ADP5585_MAN_ID_VALUE 0x02 +#define ADP5585_MAN_ID_VALUE 0x20 +#define ADP5585_MAN_ID_MASK GENMASK(7, 4) #define ADP5585_INT_STATUS 0x01 #define ADP5585_STATUS 0x02 #define ADP5585_FIFO_1 0x03 > ... > > > > > +#define ADP5585_Rx_PULL_CFG_MASK (3) > > > > > > GENMASK() > > > > Not here, as this value is meant to be passed to ADP5585_Rx_PULL_CFG(). > > Why is it marked as a mask? Rename it to _ALL or alike. It's a mask, but used as ADP5585_Rx_PULL_CFG(ADP5585_Rx_PULL_CFG_MASK) We're reaching a level of bike-shedding that even I find impressive :-) As with a few other of your review comments that I think are related to personal taste more than anything else, I'll defer to the subsystem maintainer and follow their preference on this one. > ... > > > > > +#define ADP5585_C4_EXTEND_CFG_MASK (1U << 6) > > > > > > > +#define ADP5585_R4_EXTEND_CFG_MASK (1U << 5) > > > > > > > +#define ADP5585_R3_EXTEND_CFG_MASK (3U << 2) > > > > > > > +#define ADP5585_R0_EXTEND_CFG_MASK (1U << 0) > > > > > > > +#define ADP5585_OSC_FREQ_MASK (3U << 5) > > > > > > BIT() / GENMASK() > > > > I'll use GENMASK for the masks. > > For a single bit the BIT() is okay, and TBH I don't remember if > GENMASK() supports h == l cases. I've tested it and it works.
On Wed, May 29, 2024 at 12:35 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > On Wed, May 29, 2024 at 08:44:26AM +0300, Andy Shevchenko wrote: > > On Tue, May 28, 2024 at 11:13 PM Laurent Pinchart wrote: > > > On Tue, May 28, 2024 at 10:27:34PM +0300, Andy Shevchenko wrote: > > > > Tue, May 28, 2024 at 10:03:12PM +0300, Laurent Pinchart kirjoitti: ... > > > > > + depends on I2C && OF > > > > > > > > Why OF? > > > > > > Because the driver works on OF systems only. > > > > > > > No COMPILE_TEST? > > > > > > The driver won't compile without CONFIG_I2C, so I can use > > > > > > depends on I2C > > > depends on OF || COMPILE_TEST > > > > > > Do you think that's better ? > > > > I think that dropping OF completely is the best. > > OF || COMPILE_TEST would work as well, but I still don't know why we need this. > > For the same reason that many drivers depend on specific CONFIG_$ARCH. It's different. You may not do in many cases the $ARCH || COMPILE_TEST, while OF || COMPILE_TEST should just work in 100% cases. > They can't run on other platforms, the dependency hides the symbol for > users who can't use the driver. This driver works on OF platforms only. What you are talking about is functional dependency, what I'm talking about is the compilation one. So, it's a pity that Kbuild doesn't distinguish these two basic concepts in dependencies and FOO || COMPILE_TEST is basically an artificial hack to tell "hey, FOO is _functional_ dependency, I do not care if I can compile without it". ... > > > > > +#define ADP5585_MAN_ID(v) (((v) & 0xf0) >> 4) > > > > > > > > GENMASK() > > > > > > This is not a mask. Or do you mean > > > > > > (((v) & GENMASK(7, 4)) >> 4) > > > > > > ? > > > > Yes. > > > > > I think that's overkill. > > > > Why? You have a mask, use it for less error prone code. > > I'll change this to ... > - if (ADP5585_MAN_ID(id) != ADP5585_MAN_ID_VALUE) > + if (id & ADP5585_MAN_ID_MASK != ADP5585_MAN_ID_VALUE) (Don't forget inner parentheses) ... > > > > > +#define ADP5585_Rx_PULL_CFG_MASK (3) > > > > > > > > GENMASK() > > > > > > Not here, as this value is meant to be passed to ADP5585_Rx_PULL_CFG(). > > > > Why is it marked as a mask? Rename it to _ALL or alike. > > It's a mask, but used as > > ADP5585_Rx_PULL_CFG(ADP5585_Rx_PULL_CFG_MASK) > > We're reaching a level of bike-shedding that even I find impressive :-) > As with a few other of your review comments that I think are related to > personal taste more than anything else, I'll defer to the subsystem > maintainer and follow their preference on this one. I would name it _ALL and use (BIT(2) - 1) notation to show that you want all of them to be set. But okay, let's leave this to the maintainer.
diff --git a/MAINTAINERS b/MAINTAINERS index 3016b003ead3..77b93fbdf5cc 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -532,6 +532,8 @@ L: linux-gpio@vger.kernel.org L: linux-pwm@vger.kernel.org S: Maintained F: Documentation/devicetree/bindings/*/adi,adp5585*.yaml +F: drivers/mfd/adp5585.c +F: include/linux/mfd/adp5585.h ADP5588 QWERTY KEYPAD AND IO EXPANDER DRIVER (ADP5588/ADP5587) M: Michael Hennerich <michael.hennerich@analog.com> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index 266b4f54af60..8a7ab8af217f 100644 --- a/drivers/mfd/Kconfig +++ b/drivers/mfd/Kconfig @@ -20,6 +20,17 @@ config MFD_CS5535 This is the core driver for CS5535/CS5536 MFD functions. This is necessary for using the board's GPIO and MFGPT functionality. +config MFD_ADP5585 + tristate "Analog Devices ADP5585 MFD driver" + select MFD_CORE + select REGMAP_I2C + depends on I2C && OF + help + Say yes here to add support for the Analog Devices ADP5585 GPIO + expander, PWM and keypad controller. This includes the I2C driver and + the core APIs _only_, you have to select individual components like + the GPIO and PWM functions under the corresponding menus. + config MFD_ALTERA_A10SR bool "Altera Arria10 DevKit System Resource chip" depends on ARCH_INTEL_SOCFPGA && SPI_MASTER=y && OF diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile index c66f07edcd0e..37f36a019a68 100644 --- a/drivers/mfd/Makefile +++ b/drivers/mfd/Makefile @@ -188,6 +188,7 @@ obj-$(CONFIG_MFD_DB8500_PRCMU) += db8500-prcmu.o obj-$(CONFIG_AB8500_CORE) += ab8500-core.o ab8500-sysctrl.o obj-$(CONFIG_MFD_TIMBERDALE) += timberdale.o obj-$(CONFIG_PMIC_ADP5520) += adp5520.o +obj-$(CONFIG_MFD_ADP5585) += adp5585.o obj-$(CONFIG_MFD_KEMPLD) += kempld-core.o obj-$(CONFIG_MFD_INTEL_QUARK_I2C_GPIO) += intel_quark_i2c_gpio.o obj-$(CONFIG_LPC_SCH) += lpc_sch.o diff --git a/drivers/mfd/adp5585.c b/drivers/mfd/adp5585.c new file mode 100644 index 000000000000..4f7e065e5314 --- /dev/null +++ b/drivers/mfd/adp5585.c @@ -0,0 +1,200 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Analog Devices ADP5585 I/O expander, PWM controller and keypad controller + * + * Copyright 2022 NXP + * Copyright 2024 Ideas on Board Oy + */ + +#include <linux/module.h> +#include <linux/moduleparam.h> +#include <linux/init.h> +#include <linux/slab.h> +#include <linux/i2c.h> +#include <linux/of.h> +#include <linux/of_device.h> +#include <linux/mfd/core.h> +#include <linux/mfd/adp5585.h> +#include <linux/regmap.h> + +static const struct mfd_cell adp5585_devs[] = { + { .name = "adp5585-gpio", }, + { .name = "adp5585-pwm", }, +}; + +static const struct regmap_range adp5585_volatile_ranges[] = { + regmap_reg_range(ADP5585_ID, ADP5585_GPI_STATUS_B), +}; + +static const struct regmap_access_table adp5585_volatile_regs = { + .yes_ranges = adp5585_volatile_ranges, + .n_yes_ranges = ARRAY_SIZE(adp5585_volatile_ranges), +}; + +static const u8 adp5585_regmap_defaults_00[ADP5585_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, +}; + +static const u8 adp5585_regmap_defaults_02[ADP5585_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, 0xc3, + /* 0x18 */ 0x03, 0x00, 0x03, 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, +}; + +static const u8 adp5585_regmap_defaults_04[ADP5585_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, 0x55, + /* 0x18 */ 0x05, 0x55, 0x01, 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, +}; + +enum adp5585_regmap_type { + ADP5585_REGMAP_00, + ADP5585_REGMAP_02, + ADP5585_REGMAP_04, +}; + +static const struct regmap_config adp5585_regmap_configs[] = { + [ADP5585_REGMAP_00] = { + .reg_bits = 8, + .val_bits = 8, + .max_register = ADP5585_MAX_REG, + .volatile_table = &adp5585_volatile_regs, + .cache_type = REGCACHE_MAPLE, + .reg_defaults_raw = adp5585_regmap_defaults_00, + .num_reg_defaults_raw = sizeof(adp5585_regmap_defaults_00), + }, + [ADP5585_REGMAP_02] = { + .reg_bits = 8, + .val_bits = 8, + .max_register = ADP5585_MAX_REG, + .volatile_table = &adp5585_volatile_regs, + .cache_type = REGCACHE_MAPLE, + .reg_defaults_raw = adp5585_regmap_defaults_02, + .num_reg_defaults_raw = sizeof(adp5585_regmap_defaults_02), + }, + [ADP5585_REGMAP_04] = { + .reg_bits = 8, + .val_bits = 8, + .max_register = ADP5585_MAX_REG, + .volatile_table = &adp5585_volatile_regs, + .cache_type = REGCACHE_MAPLE, + .reg_defaults_raw = adp5585_regmap_defaults_04, + .num_reg_defaults_raw = sizeof(adp5585_regmap_defaults_04), + }, +}; + +static int adp5585_i2c_probe(struct i2c_client *i2c) +{ + const struct regmap_config *regmap_config; + struct adp5585_dev *adp5585; + unsigned int id; + int ret; + + adp5585 = devm_kzalloc(&i2c->dev, sizeof(struct adp5585_dev), + GFP_KERNEL); + if (!adp5585) + return -ENOMEM; + + i2c_set_clientdata(i2c, adp5585); + + regmap_config = of_device_get_match_data(&i2c->dev); + adp5585->regmap = devm_regmap_init_i2c(i2c, regmap_config); + if (IS_ERR(adp5585->regmap)) + return dev_err_probe(&i2c->dev, PTR_ERR(adp5585->regmap), + "Failed to initialize register map\n"); + + /* Verify the device ID. */ + ret = regmap_read(adp5585->regmap, ADP5585_ID, &id); + if (ret) + return dev_err_probe(&i2c->dev, ret, + "Failed to read device ID\n"); + + if (ADP5585_MAN_ID(id) != ADP5585_MAN_ID_VALUE) + return dev_err_probe(&i2c->dev, -ENODEV, + "Invalid device ID 0x%02x\n", id); + + dev_dbg(&i2c->dev, "device ID 0x%02x\n", id); + + /* Add MFD devices. */ + ret = devm_mfd_add_devices(&i2c->dev, PLATFORM_DEVID_AUTO, + adp5585_devs, ARRAY_SIZE(adp5585_devs), + NULL, 0, NULL); + if (ret) + return dev_err_probe(&i2c->dev, ret, + "Failed to add MFD devices\n"); + + return 0; +} + +static int adp5585_suspend(struct device *dev) +{ + struct adp5585_dev *adp5585 = dev_get_drvdata(dev); + + regcache_cache_only(adp5585->regmap, true); + + return 0; +} + +static int adp5585_resume(struct device *dev) +{ + struct adp5585_dev *adp5585 = dev_get_drvdata(dev); + + regcache_cache_only(adp5585->regmap, false); + regcache_mark_dirty(adp5585->regmap); + + return regcache_sync(adp5585->regmap); +} + +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], + }, { + .compatible = "adi,adp5585-01", + .data = &adp5585_regmap_configs[ADP5585_REGMAP_00], + }, { + .compatible = "adi,adp5585-02", + .data = &adp5585_regmap_configs[ADP5585_REGMAP_02], + }, { + .compatible = "adi,adp5585-03", + .data = &adp5585_regmap_configs[ADP5585_REGMAP_00], + }, { + .compatible = "adi,adp5585-04", + .data = &adp5585_regmap_configs[ADP5585_REGMAP_04], + }, + { /* sentinel */ } +}; +MODULE_DEVICE_TABLE(of, adp5585_of_match); + +static struct i2c_driver adp5585_i2c_driver = { + .driver = { + .name = "adp5585", + .of_match_table = of_match_ptr(adp5585_of_match), + .pm = pm_sleep_ptr(&adp5585_pm), + }, + .probe = adp5585_i2c_probe, +}; +module_i2c_driver(adp5585_i2c_driver); + +MODULE_DESCRIPTION("ADP5585 core driver"); +MODULE_AUTHOR("Haibo Chen <haibo.chen@nxp.com>"); +MODULE_LICENSE("GPL"); diff --git a/include/linux/mfd/adp5585.h b/include/linux/mfd/adp5585.h new file mode 100644 index 000000000000..7682182b10d1 --- /dev/null +++ b/include/linux/mfd/adp5585.h @@ -0,0 +1,127 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * Analog Devices ADP5585 I/O expander, PWM controller and keypad controller + * + * Copyright 2022 NXP + * Copyright 2024 Ideas on Board Oy + */ + +#ifndef __LINUX_MFD_ADP5585_H_ +#define __LINUX_MFD_ADP5585_H_ + +#include <linux/bits.h> + +#define ADP5585_ID 0x00 +#define ADP5585_MAN_ID(v) (((v) & 0xf0) >> 4) +#define ADP5585_MAN_ID_VALUE 0x02 +#define ADP5585_INT_STATUS 0x01 +#define ADP5585_STATUS 0x02 +#define ADP5585_FIFO_1 0x03 +#define ADP5585_FIFO_2 0x04 +#define ADP5585_FIFO_3 0x05 +#define ADP5585_FIFO_4 0x06 +#define ADP5585_FIFO_5 0x07 +#define ADP5585_FIFO_6 0x08 +#define ADP5585_FIFO_7 0x09 +#define ADP5585_FIFO_8 0x0a +#define ADP5585_FIFO_9 0x0b +#define ADP5585_FIFO_10 0x0c +#define ADP5585_FIFO_11 0x0d +#define ADP5585_FIFO_12 0x0e +#define ADP5585_FIFO_13 0x0f +#define ADP5585_FIFO_14 0x10 +#define ADP5585_FIFO_15 0x11 +#define ADP5585_FIFO_16 0x12 +#define ADP5585_GPI_INT_STAT_A 0x13 +#define ADP5585_GPI_INT_STAT_B 0x14 +#define ADP5585_GPI_STATUS_A 0x15 +#define ADP5585_GPI_STATUS_B 0x16 +#define ADP5585_RPULL_CONFIG_A 0x17 +#define ADP5585_RPULL_CONFIG_B 0x18 +#define ADP5585_RPULL_CONFIG_C 0x19 +#define ADP5585_RPULL_CONFIG_D 0x1a +#define ADP5585_Rx_PULL_CFG(n, v) ((v) << ((n) * 2)) +#define ADP5585_Rx_PULL_CFG_PU_300K (0) +#define ADP5585_Rx_PULL_CFG_PD_300K (1) +#define ADP5585_Rx_PULL_CFG_PU_100K (2) +#define ADP5585_Rx_PULL_CFG_DISABLE (3) +#define ADP5585_Rx_PULL_CFG_MASK (3) +#define ADP5585_GPI_INT_LEVEL_A 0x1b +#define ADP5585_GPI_INT_LEVEL_B 0x1c +#define ADP5585_GPI_EVENT_EN_A 0x1d +#define ADP5585_GPI_EVENT_EN_B 0x1e +#define ADP5585_GPI_INTERRUPT_EN_A 0x1f +#define ADP5585_GPI_INTERRUPT_EN_B 0x20 +#define ADP5585_DEBOUNCE_DIS_A 0x21 +#define ADP5585_DEBOUNCE_DIS_B 0x22 +#define ADP5585_GPO_DATA_OUT_A 0x23 +#define ADP5585_GPO_DATA_OUT_B 0x24 +#define ADP5585_GPO_OUT_MODE_A 0x25 +#define ADP5585_GPO_OUT_MODE_B 0x26 +#define ADP5585_GPIO_DIRECTION_A 0x27 +#define ADP5585_GPIO_DIRECTION_B 0x28 +#define ADP5585_RESET1_EVENT_A 0x29 +#define ADP5585_RESET1_EVENT_B 0x2a +#define ADP5585_RESET1_EVENT_C 0x2b +#define ADP5585_RESET2_EVENT_A 0x2c +#define ADP5585_RESET2_EVENT_B 0x2d +#define ADP5585_RESET_CFG 0x2e +#define ADP5585_PWM_OFFT_LOW 0x2f +#define ADP5585_PWM_OFFT_HIGH 0x30 +#define ADP5585_PWM_ONT_LOW 0x31 +#define ADP5585_PWM_ONT_HIGH 0x32 +#define ADP5585_PWM_CFG 0x33 +#define ADP5585_PWM_IN_AND BIT(2) +#define ADP5585_PWM_MODE BIT(1) +#define ADP5585_PWM_EN BIT(0) +#define ADP5585_LOGIC_CFG 0x34 +#define ADP5585_LOGIC_FF_CFG 0x35 +#define ADP5585_LOGIC_INT_EVENT_EN 0x36 +#define ADP5585_POLL_PTIME_CFG 0x37 +#define ADP5585_PIN_CONFIG_A 0x38 +#define ADP5585_PIN_CONFIG_B 0x39 +#define ADP5585_PIN_CONFIG_C 0x3a +#define ADP5585_PULL_SELECT BIT(7) +#define ADP5585_C4_EXTEND_CFG_GPIO11 (0U << 6) +#define ADP5585_C4_EXTEND_CFG_RESET2 (1U << 6) +#define ADP5585_C4_EXTEND_CFG_MASK (1U << 6) +#define ADP5585_R4_EXTEND_CFG_GPIO5 (0U << 5) +#define ADP5585_R4_EXTEND_CFG_RESET1 (1U << 5) +#define ADP5585_R4_EXTEND_CFG_MASK (1U << 5) +#define ADP5585_R3_EXTEND_CFG_GPIO4 (0U << 2) +#define ADP5585_R3_EXTEND_CFG_LC (1U << 2) +#define ADP5585_R3_EXTEND_CFG_PWM_OUT (2U << 2) +#define ADP5585_R3_EXTEND_CFG_MASK (3U << 2) +#define ADP5585_R0_EXTEND_CFG_GPIO1 (0U << 0) +#define ADP5585_R0_EXTEND_CFG_LY (1U << 0) +#define ADP5585_R0_EXTEND_CFG_MASK (1U << 0) +#define ADP5585_GENERAL_CFG 0x3b +#define ADP5585_OSC_EN BIT(7) +#define ADP5585_OSC_FREQ_50KHZ (0U << 5) +#define ADP5585_OSC_FREQ_100KHZ (1U << 5) +#define ADP5585_OSC_FREQ_200KHZ (2U << 5) +#define ADP5585_OSC_FREQ_500KHZ (3U << 5) +#define ADP5585_OSC_FREQ_MASK (3U << 5) +#define ADP5585_INT_CFG BIT(1) +#define ADP5585_RST_CFG BIT(0) +#define ADP5585_INT_EN 0x3c + +#define ADP5585_MAX_REG ADP5585_INT_EN + +/* + * 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 + * 10. Some variants of the ADP5585 don't support "GPIO 6/R5". As the driver + * uses identical GPIO numbering for all variants to avoid confusion, GPIO 5 is + * marked as reserved in the device tree for variants that don't support it. + */ +#define ADP5585_BANK(n) ((n) >= 6 ? 1 : 0) +#define ADP5585_BIT(n) ((n) >= 6 ? BIT((n) - 6) : BIT(n)) + +struct regmap; + +struct adp5585_dev { + struct regmap *regmap; +}; + +#endif