diff mbox series

[v6,2/4] mfd: adp5585: Add Analog Devices ADP5585 core support

Message ID 20240721160049.20470-3-laurent.pinchart@ideasonboard.com
State Superseded
Headers show
Series ADP5585 GPIO expander, PWM and keypad controller support | expand

Commit Message

Laurent Pinchart July 21, 2024, 4 p.m. UTC
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.

Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Changes since v4:

- One more GENMASK() usage
- Include err.h

Changes since v2:

- Add missing and remove extraneous headers
- Use i2c_get_match_data()
- Drop unneeded parentheses
- Use GENMASK()
- Drop of_match_ptr()
- Allow compilation on !OF with COMPILE_TEST
- Replace ADP5585_MAN_ID() macro with ADP5585_MAN_ID_MASK
- Drop unneeded macro

Changes since v1:

- Add comment to explain BANK and BIT macros
- Drop compatible strings from cells
- White space fixes
- Fix comparison to NULL

Changes compared to the NXP original version:

- Add MAINTAINERS entry
- Fix compatible strings for child devices
- Fix header guards
- Use lowercase hex constants
- White space fixes
- Use module_i2c_driver()
- Switch to regmap
- Drop I2C device ID table
- Drop ADP5585_REG_MASK
- Support R5 GPIO pin
- Drop dev field from adp5585_dev structure
- Check device ID at probe time
- Fix register field names
- Update copyright
- Update license to GPL-2.0-only
- Implement suspend/resume
---
 MAINTAINERS                 |   2 +
 drivers/mfd/Kconfig         |  12 +++
 drivers/mfd/Makefile        |   1 +
 drivers/mfd/adp5585.c       | 200 ++++++++++++++++++++++++++++++++++++
 include/linux/mfd/adp5585.h | 126 +++++++++++++++++++++++
 5 files changed, 341 insertions(+)
 create mode 100644 drivers/mfd/adp5585.c
 create mode 100644 include/linux/mfd/adp5585.h

Comments

Lee Jones July 22, 2024, 8:56 a.m. UTC | #1
On Sun, 21 Jul 2024, Laurent Pinchart wrote:

> 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.
> 
> Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> Changes since v4:
> 
> - One more GENMASK() usage
> - Include err.h
> 
> Changes since v2:
> 
> - Add missing and remove extraneous headers
> - Use i2c_get_match_data()
> - Drop unneeded parentheses
> - Use GENMASK()
> - Drop of_match_ptr()
> - Allow compilation on !OF with COMPILE_TEST
> - Replace ADP5585_MAN_ID() macro with ADP5585_MAN_ID_MASK
> - Drop unneeded macro
> 
> Changes since v1:
> 
> - Add comment to explain BANK and BIT macros
> - Drop compatible strings from cells
> - White space fixes
> - Fix comparison to NULL
> 
> Changes compared to the NXP original version:
> 
> - Add MAINTAINERS entry
> - Fix compatible strings for child devices
> - Fix header guards
> - Use lowercase hex constants
> - White space fixes
> - Use module_i2c_driver()
> - Switch to regmap
> - Drop I2C device ID table
> - Drop ADP5585_REG_MASK
> - Support R5 GPIO pin
> - Drop dev field from adp5585_dev structure
> - Check device ID at probe time
> - Fix register field names
> - Update copyright
> - Update license to GPL-2.0-only
> - Implement suspend/resume
> ---
>  MAINTAINERS                 |   2 +
>  drivers/mfd/Kconfig         |  12 +++
>  drivers/mfd/Makefile        |   1 +
>  drivers/mfd/adp5585.c       | 200 ++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/adp5585.h | 126 +++++++++++++++++++++++
>  5 files changed, 341 insertions(+)
>  create mode 100644 drivers/mfd/adp5585.c
>  create mode 100644 include/linux/mfd/adp5585.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 4fe8bd8752a5..ebb1a1833bbc 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..05e8e1f0b602 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -20,6 +20,18 @@ 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"

It's not an MFD driver (whatever one of those is), is a Keypad Decoder
and I/O Expander.

> +	select MFD_CORE
> +	select REGMAP_I2C
> +	depends on I2C
> +	depends on OF || COMPILE_TEST
> +	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..5dc3e47a0533
> --- /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/array_size.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/mfd/adp5585.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <linux/types.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,
> +};

This 'patches', 'default's whatever you want to call them are horrid!

> +enum adp5585_regmap_type {
> +	ADP5585_REGMAP_00,
> +	ADP5585_REGMAP_02,
> +	ADP5585_REGMAP_04,
> +};

What is a type 00, 02 and 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),

sizeof(*adp5585)

> +			       GFP_KERNEL);

No need to line-wrap here - user 100 chars to improve readability.

> +	if (!adp5585)
> +		return -ENOMEM;
> +
> +	i2c_set_clientdata(i2c, adp5585);
> +
> +	regmap_config = i2c_get_match_data(i2c);
> +	adp5585->regmap = devm_regmap_init_i2c(i2c, regmap_config);

Is regmap_config guaranteed to be !NULL?

How would devm_regmap_init_i2c() handle that if it were?

> +	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. */

Probably superfluous.

> +	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)
> +		return dev_err_probe(&i2c->dev, -ENODEV,
> +				     "Invalid device ID 0x%02x\n", id);
> +
> +	dev_dbg(&i2c->dev, "device ID 0x%02x\n", id);

How often to you think this well be useful post-dev?

I'd wager, never.

> +
> +	/* Add MFD devices. */

Definitely superfluous.

> +	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");

s/MFD/child/

Or

s/MFD /sub-/

> +	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],
> +	}, {

	{	.compatible = "adi,adp5585-05"	},  /* Whoops, did I just dereference a NULL poiner? */

> +	{ /* sentinel */ }

Comment sounds cool, but is it necessary?

> +};
> +MODULE_DEVICE_TABLE(of, adp5585_of_match);
> +
> +static struct i2c_driver adp5585_i2c_driver = {
> +	.driver = {
> +		.name = "adp5585",
> +		.of_match_table = 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..25025b381c63
> --- /dev/null
> +++ b/include/linux/mfd/adp5585.h
> @@ -0,0 +1,126 @@
> +/* 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_

You can probably drop the LINUX_ part.

> +#define __LINUX_MFD_ADP5585_H_
> +
> +#include <linux/bits.h>
> +
> +#define ADP5585_ID			0x00
> +#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_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_PU_300K	0

Assuming these are bits - 2 spaces is usually enough.

> +#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	GENMASK(6, 6)
> +#define		ADP5585_R4_EXTEND_CFG_GPIO5	(0U << 5)
> +#define		ADP5585_R4_EXTEND_CFG_RESET1	(1U << 5)
> +#define		ADP5585_R4_EXTEND_CFG_MASK	GENMASK(5, 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	GENMASK(3, 2)
> +#define		ADP5585_R0_EXTEND_CFG_GPIO1	(0U << 0)
> +#define		ADP5585_R0_EXTEND_CFG_LY	(1U << 0)
> +#define		ADP5585_R0_EXTEND_CFG_MASK	GENMASK(0, 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		GENMASK(6, 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
> -- 
> Regards,
> 
> Laurent Pinchart
>
Laurent Pinchart July 22, 2024, 9:14 a.m. UTC | #2
Hi Lee,

Thank you for the review.

On Mon, Jul 22, 2024 at 09:56:29AM +0100, Lee Jones wrote:
> On Sun, 21 Jul 2024, Laurent Pinchart wrote:
> 
> > 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.
> > 
> > Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > Changes since v4:
> > 
> > - One more GENMASK() usage
> > - Include err.h
> > 
> > Changes since v2:
> > 
> > - Add missing and remove extraneous headers
> > - Use i2c_get_match_data()
> > - Drop unneeded parentheses
> > - Use GENMASK()
> > - Drop of_match_ptr()
> > - Allow compilation on !OF with COMPILE_TEST
> > - Replace ADP5585_MAN_ID() macro with ADP5585_MAN_ID_MASK
> > - Drop unneeded macro
> > 
> > Changes since v1:
> > 
> > - Add comment to explain BANK and BIT macros
> > - Drop compatible strings from cells
> > - White space fixes
> > - Fix comparison to NULL
> > 
> > Changes compared to the NXP original version:
> > 
> > - Add MAINTAINERS entry
> > - Fix compatible strings for child devices
> > - Fix header guards
> > - Use lowercase hex constants
> > - White space fixes
> > - Use module_i2c_driver()
> > - Switch to regmap
> > - Drop I2C device ID table
> > - Drop ADP5585_REG_MASK
> > - Support R5 GPIO pin
> > - Drop dev field from adp5585_dev structure
> > - Check device ID at probe time
> > - Fix register field names
> > - Update copyright
> > - Update license to GPL-2.0-only
> > - Implement suspend/resume
> > ---
> >  MAINTAINERS                 |   2 +
> >  drivers/mfd/Kconfig         |  12 +++
> >  drivers/mfd/Makefile        |   1 +
> >  drivers/mfd/adp5585.c       | 200 ++++++++++++++++++++++++++++++++++++
> >  include/linux/mfd/adp5585.h | 126 +++++++++++++++++++++++
> >  5 files changed, 341 insertions(+)
> >  create mode 100644 drivers/mfd/adp5585.c
> >  create mode 100644 include/linux/mfd/adp5585.h
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 4fe8bd8752a5..ebb1a1833bbc 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..05e8e1f0b602 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -20,6 +20,18 @@ 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"
> 
> It's not an MFD driver (whatever one of those is), is a Keypad Decoder
> and I/O Expander.

OK.

> > +	select MFD_CORE
> > +	select REGMAP_I2C
> > +	depends on I2C
> > +	depends on OF || COMPILE_TEST
> > +	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..5dc3e47a0533
> > --- /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/array_size.h>
> > +#include <linux/device.h>
> > +#include <linux/err.h>
> > +#include <linux/i2c.h>
> > +#include <linux/mfd/adp5585.h>
> > +#include <linux/mfd/core.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/module.h>
> > +#include <linux/regmap.h>
> > +#include <linux/types.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,
> > +};
> 
> This 'patches', 'default's whatever you want to call them are horrid!

As far as I understand, that's what regmap requires to handle default
register values when using the regmap cache. Is there a better way ?

> > +enum adp5585_regmap_type {
> > +	ADP5585_REGMAP_00,
> > +	ADP5585_REGMAP_02,
> > +	ADP5585_REGMAP_04,
> > +};
> 
> What is a type 00, 02 and 04?

Those are devices variants, described in the datasheet. -00 is the
default, -02 doesn't have internal pull-up resistors, and -04 has
pull-down resistors instead. See
https://www.analog.com/media/en/technical-documentation/data-sheets/ADP5585.pdf
if you're curious.

> > +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),
> 
> sizeof(*adp5585)

Indeed, not sure how I missed that.

> > +			       GFP_KERNEL);
> 
> No need to line-wrap here - user 100 chars to improve readability.
> 
> > +	if (!adp5585)
> > +		return -ENOMEM;
> > +
> > +	i2c_set_clientdata(i2c, adp5585);
> > +
> > +	regmap_config = i2c_get_match_data(i2c);
> > +	adp5585->regmap = devm_regmap_init_i2c(i2c, regmap_config);
> 
> Is regmap_config guaranteed to be !NULL?

Unless someone modifies the driver without understanding what they're
doing, yes. Please see below.

> How would devm_regmap_init_i2c() handle that if it were?
> 
> > +	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. */
> 
> Probably superfluous.

The comment, or verifying the 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 ((id & ADP5585_MAN_ID_MASK) != 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);
> 
> How often to you think this well be useful post-dev?
> 
> I'd wager, never.

I'll drop that.

> > +
> > +	/* Add MFD devices. */
> 
> Definitely superfluous.

I assume you mean the comment only :-) I can drop it.

> > +	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");
> 
> s/MFD/child/
> 
> Or
> 
> s/MFD /sub-/
> 
> > +	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],
> > +	}, {
> 
> 	{	.compatible = "adi,adp5585-05"	},  /* Whoops, did I just dereference a NULL poiner? */

Don't add an entry without .data.

 	{ .compatible = "adi,adp5585-05", .data = (void *)ADP5585_REGMAP_05 },

would also lead to problems. Someone adding support for a new variant is
expected to understand a bit of the driver. Furthemore, the NULL pointer
dereference would be caught immediately at development time, so it won't
make it to mainline. I don't think this calls for a runtime check in
probe().

> > +	{ /* sentinel */ }
> 
> Comment sounds cool, but is it necessary?

That's the common practice, and I think it makes the code clearer..

> > +};
> > +MODULE_DEVICE_TABLE(of, adp5585_of_match);
> > +
> > +static struct i2c_driver adp5585_i2c_driver = {
> > +	.driver = {
> > +		.name = "adp5585",
> > +		.of_match_table = 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..25025b381c63
> > --- /dev/null
> > +++ b/include/linux/mfd/adp5585.h
> > @@ -0,0 +1,126 @@
> > +/* 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_
> 
> You can probably drop the LINUX_ part.

$ git grep 'ifndef.*_H_\?$' include/linux/ | grep LINUX_ | wc -l
1408
$ git grep 'ifndef.*_H_\?$' include/linux/ | grep -v LINUX_ | wc -l
804

Namespacing the header guards seems a good practice to me. I agree that
we will likely not have another mfd/adp5585.h file in any other
directory in include/, so that's more of a theoretical concern, but can
I still go with the majority practice ?

> > +#define __LINUX_MFD_ADP5585_H_
> > +
> > +#include <linux/bits.h>
> > +
> > +#define ADP5585_ID			0x00
> > +#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_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_PU_300K	0
> 
> Assuming these are bits - 2 spaces is usually enough.

I find a tab tobe more readable. Is this a showstopper or can I keep the
tab ?

> > +#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	GENMASK(6, 6)
> > +#define		ADP5585_R4_EXTEND_CFG_GPIO5	(0U << 5)
> > +#define		ADP5585_R4_EXTEND_CFG_RESET1	(1U << 5)
> > +#define		ADP5585_R4_EXTEND_CFG_MASK	GENMASK(5, 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	GENMASK(3, 2)
> > +#define		ADP5585_R0_EXTEND_CFG_GPIO1	(0U << 0)
> > +#define		ADP5585_R0_EXTEND_CFG_LY	(1U << 0)
> > +#define		ADP5585_R0_EXTEND_CFG_MASK	GENMASK(0, 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		GENMASK(6, 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
Lee Jones July 22, 2024, 9:32 a.m. UTC | #3
On Mon, 22 Jul 2024, Laurent Pinchart wrote:

> Hi Lee,
> 
> Thank you for the review.
> 
> On Mon, Jul 22, 2024 at 09:56:29AM +0100, Lee Jones wrote:
> > On Sun, 21 Jul 2024, Laurent Pinchart wrote:
> > 
> > > 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.
> > > 
> > > Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > > Changes since v4:
> > > 
> > > - One more GENMASK() usage
> > > - Include err.h
> > > 
> > > Changes since v2:
> > > 
> > > - Add missing and remove extraneous headers
> > > - Use i2c_get_match_data()
> > > - Drop unneeded parentheses
> > > - Use GENMASK()
> > > - Drop of_match_ptr()
> > > - Allow compilation on !OF with COMPILE_TEST
> > > - Replace ADP5585_MAN_ID() macro with ADP5585_MAN_ID_MASK
> > > - Drop unneeded macro
> > > 
> > > Changes since v1:
> > > 
> > > - Add comment to explain BANK and BIT macros
> > > - Drop compatible strings from cells
> > > - White space fixes
> > > - Fix comparison to NULL
> > > 
> > > Changes compared to the NXP original version:
> > > 
> > > - Add MAINTAINERS entry
> > > - Fix compatible strings for child devices
> > > - Fix header guards
> > > - Use lowercase hex constants
> > > - White space fixes
> > > - Use module_i2c_driver()
> > > - Switch to regmap
> > > - Drop I2C device ID table
> > > - Drop ADP5585_REG_MASK
> > > - Support R5 GPIO pin
> > > - Drop dev field from adp5585_dev structure
> > > - Check device ID at probe time
> > > - Fix register field names
> > > - Update copyright
> > > - Update license to GPL-2.0-only
> > > - Implement suspend/resume
> > > ---
> > >  MAINTAINERS                 |   2 +
> > >  drivers/mfd/Kconfig         |  12 +++
> > >  drivers/mfd/Makefile        |   1 +
> > >  drivers/mfd/adp5585.c       | 200 ++++++++++++++++++++++++++++++++++++
> > >  include/linux/mfd/adp5585.h | 126 +++++++++++++++++++++++
> > >  5 files changed, 341 insertions(+)
> > >  create mode 100644 drivers/mfd/adp5585.c
> > >  create mode 100644 include/linux/mfd/adp5585.h
> > > 
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index 4fe8bd8752a5..ebb1a1833bbc 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..05e8e1f0b602 100644
> > > --- a/drivers/mfd/Kconfig
> > > +++ b/drivers/mfd/Kconfig
> > > @@ -20,6 +20,18 @@ 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"
> > 
> > It's not an MFD driver (whatever one of those is), is a Keypad Decoder
> > and I/O Expander.
> 
> OK.
> 
> > > +	select MFD_CORE
> > > +	select REGMAP_I2C
> > > +	depends on I2C
> > > +	depends on OF || COMPILE_TEST
> > > +	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..5dc3e47a0533
> > > --- /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/array_size.h>
> > > +#include <linux/device.h>
> > > +#include <linux/err.h>
> > > +#include <linux/i2c.h>
> > > +#include <linux/mfd/adp5585.h>
> > > +#include <linux/mfd/core.h>
> > > +#include <linux/mod_devicetable.h>
> > > +#include <linux/module.h>
> > > +#include <linux/regmap.h>
> > > +#include <linux/types.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,
> > > +};
> > 
> > This 'patches', 'default's whatever you want to call them are horrid!
> 
> As far as I understand, that's what regmap requires to handle default
> register values when using the regmap cache. Is there a better way ?

Not that I'm aware of - I just like to moan!  :(

> > > +enum adp5585_regmap_type {
> > > +	ADP5585_REGMAP_00,
> > > +	ADP5585_REGMAP_02,
> > > +	ADP5585_REGMAP_04,
> > > +};
> > 
> > What is a type 00, 02 and 04?
> 
> Those are devices variants, described in the datasheet. -00 is the
> default, -02 doesn't have internal pull-up resistors, and -04 has
> pull-down resistors instead. See
> https://www.analog.com/media/en/technical-documentation/data-sheets/ADP5585.pdf
> if you're curious.

Comments please.

> > > +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),
> > 
> > sizeof(*adp5585)
> 
> Indeed, not sure how I missed that.
> 
> > > +			       GFP_KERNEL);
> > 
> > No need to line-wrap here - user 100 chars to improve readability.
> > 
> > > +	if (!adp5585)
> > > +		return -ENOMEM;
> > > +
> > > +	i2c_set_clientdata(i2c, adp5585);
> > > +
> > > +	regmap_config = i2c_get_match_data(i2c);
> > > +	adp5585->regmap = devm_regmap_init_i2c(i2c, regmap_config);
> > 
> > Is regmap_config guaranteed to be !NULL?
> 
> Unless someone modifies the driver without understanding what they're
> doing, yes. Please see below.
> 
> > How would devm_regmap_init_i2c() handle that if it were?
> > 
> > > +	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. */
> > 
> > Probably superfluous.
> 
> The comment, or verifying the ID ?

The comment.

> > > +	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)
> > > +		return dev_err_probe(&i2c->dev, -ENODEV,
> > > +				     "Invalid device ID 0x%02x\n", id);
> > > +
> > > +	dev_dbg(&i2c->dev, "device ID 0x%02x\n", id);
> > 
> > How often to you think this well be useful post-dev?
> > 
> > I'd wager, never.
> 
> I'll drop that.
> 
> > > +
> > > +	/* Add MFD devices. */
> > 
> > Definitely superfluous.
> 
> I assume you mean the comment only :-) I can drop it.

Si.

> > > +	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");
> > 
> > s/MFD/child/
> > 
> > Or
> > 
> > s/MFD /sub-/
> > 
> > > +	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],
> > > +	}, {
> > 
> > 	{	.compatible = "adi,adp5585-05"	},  /* Whoops, did I just dereference a NULL poiner? */
> 
> Don't add an entry without .data.
> 
>  	{ .compatible = "adi,adp5585-05", .data = (void *)ADP5585_REGMAP_05 },
> 
> would also lead to problems. Someone adding support for a new variant is
> expected to understand a bit of the driver. Furthemore, the NULL pointer
> dereference would be caught immediately at development time, so it won't
> make it to mainline. I don't think this calls for a runtime check in
> probe().

Your call.  Just so long as you've thought about it.

> > > +	{ /* sentinel */ }
> > 
> > Comment sounds cool, but is it necessary?
> 
> That's the common practice, and I think it makes the code clearer..

I used to think it was cool when it was first introduced.

Now I just think it's silly.  But again, your call.

> > > +};
> > > +MODULE_DEVICE_TABLE(of, adp5585_of_match);
> > > +
> > > +static struct i2c_driver adp5585_i2c_driver = {
> > > +	.driver = {
> > > +		.name = "adp5585",
> > > +		.of_match_table = 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..25025b381c63
> > > --- /dev/null
> > > +++ b/include/linux/mfd/adp5585.h
> > > @@ -0,0 +1,126 @@
> > > +/* 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_
> > 
> > You can probably drop the LINUX_ part.
> 
> $ git grep 'ifndef.*_H_\?$' include/linux/ | grep LINUX_ | wc -l
> 1408
> $ git grep 'ifndef.*_H_\?$' include/linux/ | grep -v LINUX_ | wc -l
> 804
> 
> Namespacing the header guards seems a good practice to me. I agree that
> we will likely not have another mfd/adp5585.h file in any other
> directory in include/, so that's more of a theoretical concern, but can
> I still go with the majority practice ?

It only really makes sense if you're also writing drivers for other OSes.

> > > +#define __LINUX_MFD_ADP5585_H_
> > > +
> > > +#include <linux/bits.h>
> > > +
> > > +#define ADP5585_ID			0x00
> > > +#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_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_PU_300K	0
> > 
> > Assuming these are bits - 2 spaces is usually enough.
> 
> I find a tab tobe more readable. Is this a showstopper or can I keep the
> tab ?

No show stoppers, just what I've seen more regularly/personal preference.

> > > +#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	GENMASK(6, 6)
> > > +#define		ADP5585_R4_EXTEND_CFG_GPIO5	(0U << 5)
> > > +#define		ADP5585_R4_EXTEND_CFG_RESET1	(1U << 5)
> > > +#define		ADP5585_R4_EXTEND_CFG_MASK	GENMASK(5, 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	GENMASK(3, 2)
> > > +#define		ADP5585_R0_EXTEND_CFG_GPIO1	(0U << 0)
> > > +#define		ADP5585_R0_EXTEND_CFG_LY	(1U << 0)
> > > +#define		ADP5585_R0_EXTEND_CFG_MASK	GENMASK(0, 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		GENMASK(6, 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
> 
> -- 
> Regards,
> 
> Laurent Pinchart
diff mbox series

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index 4fe8bd8752a5..ebb1a1833bbc 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..05e8e1f0b602 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -20,6 +20,18 @@  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
+	depends on OF || COMPILE_TEST
+	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..5dc3e47a0533
--- /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/array_size.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/mfd/adp5585.h>
+#include <linux/mfd/core.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/types.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 = i2c_get_match_data(i2c);
+	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 ((id & ADP5585_MAN_ID_MASK) != 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 = 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..25025b381c63
--- /dev/null
+++ b/include/linux/mfd/adp5585.h
@@ -0,0 +1,126 @@ 
+/* 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_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_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_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	GENMASK(6, 6)
+#define		ADP5585_R4_EXTEND_CFG_GPIO5	(0U << 5)
+#define		ADP5585_R4_EXTEND_CFG_RESET1	(1U << 5)
+#define		ADP5585_R4_EXTEND_CFG_MASK	GENMASK(5, 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	GENMASK(3, 2)
+#define		ADP5585_R0_EXTEND_CFG_GPIO1	(0U << 0)
+#define		ADP5585_R0_EXTEND_CFG_LY	(1U << 0)
+#define		ADP5585_R0_EXTEND_CFG_MASK	GENMASK(0, 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		GENMASK(6, 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