Re: [PATCH v4 4/6] mfd: max77759: add Maxim MAX77759 core mfd driver

[Lee Jones <lee@kernel.org>]

On Wed, 12 Mar 2025, André Draszik wrote:

> The Maxim MAX77759 is a companion PMIC for USB Type-C applications and
> includes Battery Charger, Fuel Gauge, temperature sensors, USB Type-C
> Port Controller (TCPC), NVMEM, and a GPIO expander.
> 
> Fuel Gauge and TCPC have separate and independent I2C addresses,
> register maps, and interrupt lines and are therefore excluded from the
> MFD core device driver here.
> 
> The GPIO and NVMEM interfaces are accessed via specific commands to the
> built-in microprocessor. This driver implements an API that client
> drivers can use for accessing those.
> 
> Signed-off-by: André Draszik <andre.draszik@linaro.org>
> 
> ---
> v4:
> * add missing build_bug.h include
> * update an irq chip comment
> * fix a whitespace in register definitions
> 
> v2:
> * add kernel doc for max77759_maxq_command() and related structs
> * fix an msec / usec typo
> * add missing error handling of devm_mutex_init() (Christophe)
> * align sentinel in max77759_of_id[] with max77759_i2c_id[]
>   (Christophe)
> * some tidy-ups in max77759_maxq_command() (Christophe)
> ---
>  MAINTAINERS                  |   2 +
>  drivers/mfd/Kconfig          |  20 ++
>  drivers/mfd/Makefile         |   1 +
>  drivers/mfd/max77759.c       | 738 +++++++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/max77759.h |  98 ++++++
>  5 files changed, 859 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index a45d1bd64d116d14bc05a64fa3da852a41e1de7f..38844ea24e464d0b58f8852b79e2b94f18d48998 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -14352,6 +14352,8 @@ M:	André Draszik <andre.draszik@linaro.org>
>  L:	linux-kernel@vger.kernel.org
>  S:	Maintained
>  F:	Documentation/devicetree/bindings/*/maxim,max77759*.yaml
> +F:	drivers/mfd/max77759.c
> +F:	include/linux/mfd/max77759.h
>  
>  MAXIM MAX77802 PMIC REGULATOR DEVICE DRIVER
>  M:	Javier Martinez Canillas <javier@dowhile0.org>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index d44c69bb3dfd52d53fe26aa2d5e5ace346448f57..1d72bf086401064608cc41e9f9a0af044c6df68e 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -930,6 +930,26 @@ config MFD_MAX77714
>  	  drivers must be enabled in order to use each functionality of the
>  	  device.
>  
> +config MFD_MAX77759
> +	tristate "Maxim Integrated MAX77759 PMIC"
> +	depends on I2C
> +	depends on OF
> +	select IRQ_DOMAIN
> +	select MFD_CORE
> +	select REGMAP_I2C
> +	select REGMAP_IRQ
> +	help
> +	  Say yes here to add support for Maxim Integrated MAX77759.
> +	  This is a companion Power Management IC for USB Type-C applications
> +	  with Battery Charger, Fuel Gauge, temperature sensors, USB Type-C
> +	  Port Controller (TCPC), NVMEM, and additional GPIO interfaces.
> +	  This driver provides common support for accessing the device;
> +	  additional drivers must be enabled in order to use the functionality
> +	  of the device.
> +
> +	  To compile this driver as a module, choose M here: the module will be
> +	  called max77759.
> +
>  config MFD_MAX77843
>  	bool "Maxim Semiconductor MAX77843 PMIC Support"
>  	depends on I2C=y
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 9220eaf7cf1255a8922430fe3e50e41771bbaa60..cc9362afd8f060d64ca0f0f028a0d7cfe9cfe512 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -169,6 +169,7 @@ obj-$(CONFIG_MFD_MAX77650)	+= max77650.o
>  obj-$(CONFIG_MFD_MAX77686)	+= max77686.o
>  obj-$(CONFIG_MFD_MAX77693)	+= max77693.o
>  obj-$(CONFIG_MFD_MAX77714)	+= max77714.o
> +obj-$(CONFIG_MFD_MAX77759)	+= max77759.o
>  obj-$(CONFIG_MFD_MAX77843)	+= max77843.o
>  obj-$(CONFIG_MFD_MAX8907)	+= max8907.o
>  max8925-objs			:= max8925-core.o max8925-i2c.o
> diff --git a/drivers/mfd/max77759.c b/drivers/mfd/max77759.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..a4f1616d600bde250a73b69464ada1fbf6d7e67e
> --- /dev/null
> +++ b/drivers/mfd/max77759.c
> @@ -0,0 +1,738 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +//
> +// Copyright 2020 Google Inc
> +// Copyright 2025 Linaro Ltd.
> +//
> +// Core MFD driver for Maxim MAX77759 companion PMIC for USB Type-C

Only the SPDX should be in C++ style comments.

Please C block-comment the rest.

Drop all references to MFD - it's not a real thing.

> +#include <linux/array_size.h>
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/build_bug.h>
> +#include <linux/cleanup.h>
> +#include <linux/completion.h>
> +#include <linux/dev_printk.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/jiffies.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/max77759.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/overflow.h>
> +#include <linux/regmap.h>

This is a huge list of includes.

Please comb them to see if you really do need all of them.

> +/* registers - registers useful to drivers are declared in the public header */

This sort of goes without saying.  Suggest you remove this comment.

> +/* PMIC / TOP */
> +#define MAX77759_PMIC_REG_PMIC_ID             0x00
> +#define MAX77759_PMIC_REG_PMIC_ID_MAX77759    59

Is this a register or a value?

> +#define MAX77759_PMIC_REG_PMIC_REVISION       0x01
> +#define MAX77759_PMIC_REG_OTP_REVISION        0x02
> +
> +#define MAX77759_PMIC_REG_INTSRC              0x22
> +#define MAX77759_PMIC_REG_INTSRCMASK          0x23
> +#define MAX77759_PMIC_REG_INTSRC_MAXQ         BIT(3)
> +#define MAX77759_PMIC_REG_INTSRC_TOPSYS       BIT(1)
> +#define MAX77759_PMIC_REG_INTSRC_CHGR         BIT(0)

These look like bit offsets rather than reg addresses?

> +#define MAX77759_PMIC_REG_TOPSYS_INT          0x24
> +#define MAX77759_PMIC_REG_TOPSYS_INT_MASK     0x26
> +#define MAX77759_PMIC_REG_TOPSYS_INT_TSHDN    BIT(6)
> +#define MAX77759_PMIC_REG_TOPSYS_INT_SYSOVLO  BIT(5)
> +#define MAX77759_PMIC_REG_TOPSYS_INT_SYSUVLO  BIT(4)
> +#define MAX77759_PMIC_REG_TOPSYS_INT_FSHIP    BIT(0)
> +
> +#define MAX77759_PMIC_REG_I2C_CNFG            0x40
> +#define MAX77759_PMIC_REG_SWRESET             0x50
> +#define MAX77759_PMIC_REG_CONTROL_FG          0x51
> +#define MAX77759_PMIC_REG_LAST_REGISTER       MAX77759_PMIC_REG_CONTROL_FG

You could just use MAX77759_PMIC_REG_CONTROL_FG in-place?

> +/* MaxQ */

How does this improve readability?  The name is already in the define.
If you want to provide a comment, which isn't a bad idea, extend the
meaning and describe the purpose instead.

> +#define MAX77759_MAXQ_REG_AP_DATAOUT0       0x81
> +#define MAX77759_MAXQ_REG_AP_DATAOUT32      0xa1
> +#define MAX77759_MAXQ_REG_AP_MESSAGESZ_MAX  (MAX77759_MAXQ_REG_AP_DATAOUT32 \
> +					     - MAX77759_MAXQ_REG_AP_DATAOUT0 \
> +					     + 1)

This is a strange way to line things up.

> +#define MAX77759_MAXQ_REG_AP_DATAIN0        0xb1
> +#define MAX77759_MAXQ_REG_LAST_REGISTER     0xe0

What is the last register's purpose, besides being the last one?

> +/* charger */

Not sure this provides much, but if you insist on keeping it, all
comments should start with an upper-case char here.

> +#define MAX77759_CHGR_REG_LAST_REGISTER  0xcc

Where are the other registers?

> +enum max77759_i2c_subdev_id {
> +	MAX77759_I2C_SUBDEV_ID_MAXQ,
> +	MAX77759_I2C_SUBDEV_ID_CHARGER,

Are these truly arbitrary or are you relying on the fact that the
compiler usually starts from 0 and incs by 1?

> +};
> +
> +struct max77759_mfd {

Drop all "mfd"ness.

> +	/* protecting MaxQ commands - only one can be active */

"This protects"

> +	struct mutex maxq_lock;
> +	struct regmap *regmap_maxq;
> +	struct completion cmd_done;
> +
> +	struct regmap *regmap_top;

What is top?

> +	struct regmap *regmap_charger;
> +};
> +
> +struct max77759_i2c_subdev {
> +	enum max77759_i2c_subdev_id id;
> +	const struct regmap_config *cfg;
> +	u16 i2c_address;
> +};
> +
> +/* TOP registers */
> +static const struct regmap_range max77759_top_registers[] = {
> +	regmap_reg_range(0x00, 0x02),
> +	regmap_reg_range(0x22, 0x24),
> +	regmap_reg_range(0x26, 0x26),
> +	regmap_reg_range(0x40, 0x40),
> +	regmap_reg_range(0x50, 0x51),

What are these magic numbers?  Can you define them?

> +};
> +
> +static const struct regmap_range max77759_top_ro_registers[] = {
> +	regmap_reg_range(0x00, 0x02),
> +	regmap_reg_range(0x22, 0x22),
> +};
> +
> +static const struct regmap_range max77759_top_volatile_registers[] = {
> +	regmap_reg_range(0x22, 0x22),
> +	regmap_reg_range(0x24, 0x24),
> +};
> +
> +static const struct regmap_access_table max77759_top_wr_table = {
> +	.yes_ranges = max77759_top_registers,
> +	.n_yes_ranges = ARRAY_SIZE(max77759_top_registers),
> +	.no_ranges = max77759_top_ro_registers,
> +	.n_no_ranges = ARRAY_SIZE(max77759_top_ro_registers),
> +};
> +
> +static const struct regmap_access_table max77759_top_rd_table = {
> +	.yes_ranges = max77759_top_registers,
> +	.n_yes_ranges = ARRAY_SIZE(max77759_top_registers),
> +};
> +
> +static const struct regmap_access_table max77759_top_volatile_table = {
> +	.yes_ranges = max77759_top_volatile_registers,
> +	.n_yes_ranges = ARRAY_SIZE(max77759_top_volatile_registers),
> +};
> +
> +static const struct regmap_config max77759_regmap_config_top = {
> +	.name = "top",
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = MAX77759_PMIC_REG_LAST_REGISTER,
> +	.wr_table = &max77759_top_wr_table,
> +	.rd_table = &max77759_top_rd_table,
> +	.volatile_table = &max77759_top_volatile_table,
> +	.num_reg_defaults_raw = MAX77759_PMIC_REG_LAST_REGISTER + 1,
> +	.cache_type = REGCACHE_MAPLE,
> +};
> +
> +/* MaxQ registers */

Again, this is obvious due to the nomenclature.

> +static const struct regmap_range max77759_maxq_registers[] = {
> +	regmap_reg_range(0x60, 0x73),
> +	regmap_reg_range(0x81, 0xa1),
> +	regmap_reg_range(0xb1, 0xd1),
> +	regmap_reg_range(0xe0, 0xe0),
> +};
> +
> +static const struct regmap_range max77759_maxq_ro_registers[] = {
> +	regmap_reg_range(0x60, 0x63),
> +	regmap_reg_range(0x68, 0x6f),
> +	regmap_reg_range(0xb1, 0xd1),
> +};
> +
> +static const struct regmap_range max77759_maxq_volatile_registers[] = {
> +	regmap_reg_range(0x64, 0x6f),
> +	regmap_reg_range(0xb1, 0xd1),
> +	regmap_reg_range(0xe0, 0xe0),
> +};
> +
> +static const struct regmap_access_table max77759_maxq_wr_table = {
> +	.yes_ranges = max77759_maxq_registers,
> +	.n_yes_ranges = ARRAY_SIZE(max77759_maxq_registers),
> +	.no_ranges = max77759_maxq_ro_registers,
> +	.n_no_ranges = ARRAY_SIZE(max77759_maxq_ro_registers),
> +};
> +
> +static const struct regmap_access_table max77759_maxq_rd_table = {
> +	.yes_ranges = max77759_maxq_registers,
> +	.n_yes_ranges = ARRAY_SIZE(max77759_maxq_registers),
> +};
> +
> +static const struct regmap_access_table max77759_maxq_volatile_table = {
> +	.yes_ranges = max77759_maxq_volatile_registers,
> +	.n_yes_ranges = ARRAY_SIZE(max77759_maxq_volatile_registers),
> +};
> +
> +static const struct regmap_config max77759_regmap_config_maxq = {
> +	.name = "maxq",
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = MAX77759_MAXQ_REG_LAST_REGISTER,
> +	.wr_table = &max77759_maxq_wr_table,
> +	.rd_table = &max77759_maxq_rd_table,
> +	.volatile_table = &max77759_maxq_volatile_table,
> +	.num_reg_defaults_raw = MAX77759_MAXQ_REG_LAST_REGISTER + 1,
> +	.cache_type = REGCACHE_MAPLE,
> +};
> +
> +/* charger registers */
> +static const struct regmap_range max77759_charger_registers[] = {
> +	regmap_reg_range(0xb0, 0xcc),
> +};
> +
> +static const struct regmap_range max77759_charger_ro_registers[] = {
> +	regmap_reg_range(0xb4, 0xb8),
> +};
> +
> +static const struct regmap_range max77759_charger_volatile_registers[] = {
> +	regmap_reg_range(0xb0, 0xb1),
> +	regmap_reg_range(0xb4, 0xb8),
> +};
> +
> +static const struct regmap_access_table max77759_charger_wr_table = {
> +	.yes_ranges = max77759_charger_registers,
> +	.n_yes_ranges = ARRAY_SIZE(max77759_charger_registers),
> +	.no_ranges = max77759_charger_ro_registers,
> +	.n_no_ranges = ARRAY_SIZE(max77759_charger_ro_registers),
> +};
> +
> +static const struct regmap_access_table max77759_charger_rd_table = {
> +	.yes_ranges = max77759_charger_registers,
> +	.n_yes_ranges = ARRAY_SIZE(max77759_charger_registers),
> +};
> +
> +static const struct regmap_access_table max77759_charger_volatile_table = {
> +	.yes_ranges = max77759_charger_volatile_registers,
> +	.n_yes_ranges = ARRAY_SIZE(max77759_charger_volatile_registers),
> +};
> +
> +static const struct regmap_config max77759_regmap_config_charger = {
> +	.name = "charger",
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = MAX77759_CHGR_REG_LAST_REGISTER,
> +	.wr_table = &max77759_charger_wr_table,
> +	.rd_table = &max77759_charger_rd_table,
> +	.volatile_table = &max77759_charger_volatile_table,
> +	.num_reg_defaults_raw = MAX77759_CHGR_REG_LAST_REGISTER + 1,
> +	.cache_type = REGCACHE_MAPLE,
> +};
> +
> +/*
> + * Interrupts - with the following interrupt hierarchy:
> + *   pmic IRQs (INTSRC)
> + *     - MAXQ_INT: MaxQ IRQs
> + *       - UIC_INT1
> + *         - APCmdResI
> + *         - SysMsgI
> + *         - GPIOxI
> + *     - TOPSYS_INT: topsys
> + *       - TOPSYS_INT
> + *         - TSHDN_INT
> + *         - SYSOVLO_INT
> + *         - SYSUVLO_INT
> + *         - FSHIP_NOT_RD
> + *     - CHGR_INT: charger
> + *       - CHG_INT
> + *       - CHG_INT2
> + */
> +enum {
> +	MAX77759_INT_MAXQ,
> +	MAX77759_INT_TOPSYS,
> +	MAX77759_INT_CHGR,
> +};
> +
> +enum {
> +	MAX77759_TOPSYS_INT_TSHDN,
> +	MAX77759_TOPSYS_INT_SYSOVLO,
> +	MAX77759_TOPSYS_INT_SYSUVLO,
> +	MAX77759_TOPSYS_INT_FSHIP_NOT_RD,
> +};
> +
> +enum {
> +	MAX77759_MAXQ_INT_APCMDRESI,
> +	MAX77759_MAXQ_INT_SYSMSGI,
> +	MAX77759_MAXQ_INT_GPIO,
> +	MAX77759_MAXQ_INT_UIC1,
> +	MAX77759_MAXQ_INT_UIC2,
> +	MAX77759_MAXQ_INT_UIC3,
> +	MAX77759_MAXQ_INT_UIC4,
> +};
> +
> +enum {
> +	MAX77759_CHARGER_INT_1,
> +	MAX77759_CHARGER_INT_2,
> +};
> +
> +static const struct regmap_irq max77759_pmic_irqs[] = {
> +	REGMAP_IRQ_REG(MAX77759_INT_MAXQ, 0, MAX77759_PMIC_REG_INTSRC_MAXQ),
> +	REGMAP_IRQ_REG(MAX77759_INT_TOPSYS, 0, MAX77759_PMIC_REG_INTSRC_TOPSYS),
> +	REGMAP_IRQ_REG(MAX77759_INT_CHGR, 0, MAX77759_PMIC_REG_INTSRC_CHGR),
> +};
> +
> +static const struct regmap_irq max77759_maxq_irqs[] = {
> +	REGMAP_IRQ_REG(MAX77759_MAXQ_INT_APCMDRESI,
> +		       0, MAX77759_MAXQ_REG_UIC_INT1_APCMDRESI),

Use up to 100-chars here to avoid the line-break.

This isn't 1999 anymore - we have wide-screen 4k monitors. =:-)

> +	REGMAP_IRQ_REG(MAX77759_MAXQ_INT_SYSMSGI,
> +		       0, MAX77759_MAXQ_REG_UIC_INT1_SYSMSGI),
> +	REGMAP_IRQ_REG(MAX77759_MAXQ_INT_GPIO, 0, GENMASK(1, 0)),
> +	REGMAP_IRQ_REG(MAX77759_MAXQ_INT_UIC1, 0, GENMASK(5, 2)),
> +	REGMAP_IRQ_REG(MAX77759_MAXQ_INT_UIC2, 1, GENMASK(7, 0)),
> +	REGMAP_IRQ_REG(MAX77759_MAXQ_INT_UIC3, 2, GENMASK(7, 0)),
> +	REGMAP_IRQ_REG(MAX77759_MAXQ_INT_UIC4, 3, GENMASK(7, 0)),
> +};
> +
> +static const struct regmap_irq max77759_topsys_irqs[] = {
> +	REGMAP_IRQ_REG(MAX77759_TOPSYS_INT_TSHDN,
> +		       0, MAX77759_PMIC_REG_TOPSYS_INT_TSHDN),
> +	REGMAP_IRQ_REG(MAX77759_TOPSYS_INT_SYSOVLO,
> +		       0, MAX77759_PMIC_REG_TOPSYS_INT_SYSOVLO),
> +	REGMAP_IRQ_REG(MAX77759_TOPSYS_INT_SYSUVLO,
> +		       0, MAX77759_PMIC_REG_TOPSYS_INT_SYSUVLO),
> +	REGMAP_IRQ_REG(MAX77759_TOPSYS_INT_FSHIP_NOT_RD,
> +		       0, MAX77759_PMIC_REG_TOPSYS_INT_FSHIP),
> +};
> +
> +static const struct regmap_irq max77759_chgr_irqs[] = {
> +	REGMAP_IRQ_REG(MAX77759_CHARGER_INT_1, 0, GENMASK(7, 0)),
> +	REGMAP_IRQ_REG(MAX77759_CHARGER_INT_2, 1, GENMASK(7, 0)),
> +};
> +
> +static const struct regmap_irq_chip max77759_pmic_irq_chip = {
> +	.name = "max77759-pmic",
> +	/* INTSRC is read-only and doesn't require clearing */
> +	.status_base = MAX77759_PMIC_REG_INTSRC,
> +	.mask_base = MAX77759_PMIC_REG_INTSRCMASK,
> +	.num_regs = 1,
> +	.irqs = max77759_pmic_irqs,
> +	.num_irqs = ARRAY_SIZE(max77759_pmic_irqs),
> +};
> +
> +/*
> + * We can let regmap-irq auto-ack the topsys interrupt bits as required, but
> + * for all others the individual drivers need to know which interrupt bit
> + * exactly is set inside their interrupt handlers, and therefore we can not set
> + * .ack_base for those.
> + */

Nice - thanks for this!

> +static const struct regmap_irq_chip max77759_maxq_irq_chip = {
> +	.name = "max77759-maxq",
> +	.domain_suffix = "MAXQ",
> +	.status_base = MAX77759_MAXQ_REG_UIC_INT1,
> +	.mask_base = MAX77759_MAXQ_REG_UIC_INT1_M,
> +	.num_regs = 4,
> +	.irqs = max77759_maxq_irqs,
> +	.num_irqs = ARRAY_SIZE(max77759_maxq_irqs),
> +};
> +
> +static const struct regmap_irq_chip max77759_topsys_irq_chip = {
> +	.name = "max77759-topsys",
> +	.domain_suffix = "TOPSYS",
> +	.status_base = MAX77759_PMIC_REG_TOPSYS_INT,
> +	.mask_base = MAX77759_PMIC_REG_TOPSYS_INT_MASK,
> +	.ack_base = MAX77759_PMIC_REG_TOPSYS_INT,
> +	.num_regs = 1,
> +	.irqs = max77759_topsys_irqs,
> +	.num_irqs = ARRAY_SIZE(max77759_topsys_irqs),
> +};
> +
> +static const struct regmap_irq_chip max77759_chrg_irq_chip = {
> +	.name = "max77759-chgr",
> +	.domain_suffix = "CHGR",
> +	.status_base = MAX77759_CHGR_REG_CHG_INT,
> +	.mask_base = MAX77759_CHGR_REG_CHG_INT_MASK,
> +	.num_regs = 2,
> +	.irqs = max77759_chgr_irqs,
> +	.num_irqs = ARRAY_SIZE(max77759_chgr_irqs),
> +};
> +
> +static const struct max77759_i2c_subdev max77759_i2c_subdevs[] = {
> +	{
> +		.id = MAX77759_I2C_SUBDEV_ID_MAXQ,
> +		/* I2C address is same as top's */

It took me a moment to figure out why this is relevant.

Maybe shift this down, so it lines-up with the next element.

> +		.cfg = &max77759_regmap_config_maxq,
> +	},
> +	{
> +		.id = MAX77759_I2C_SUBDEV_ID_CHARGER,
> +		.cfg = &max77759_regmap_config_charger,
> +		.i2c_address = 0x69,
> +	},
> +};
> +
> +static const struct resource max77759_gpio_resources[] = {
> +	DEFINE_RES_IRQ_NAMED(MAX77759_MAXQ_INT_GPIO, "GPI"),
> +};
> +
> +static const struct resource max77759_charger_resources[] = {
> +	DEFINE_RES_IRQ_NAMED(MAX77759_CHARGER_INT_1, "INT1"),
> +	DEFINE_RES_IRQ_NAMED(MAX77759_CHARGER_INT_2, "INT2"),
> +};
> +
> +static const struct mfd_cell max77759_cells[] = {
> +	MFD_CELL_OF("max77759-nvmem", NULL, NULL, 0, 0,
> +		    "maxim,max77759-nvmem"),
> +};
> +
> +static const struct mfd_cell max77759_maxq_cells[] = {
> +	MFD_CELL_OF("max77759-gpio", max77759_gpio_resources, NULL, 0, 0,
> +		    "maxim,max77759-gpio"),
> +};
> +
> +static const struct mfd_cell max77759_charger_cells[] = {
> +	MFD_CELL_RES("max77759-charger", max77759_charger_resources),
> +};

Are there situations where only 1 or 2 of these will be used?

If they will all be used all of the time, save yourself a bunch of lines
and complexity and add them all to a single struct.

> +int max77759_maxq_command(struct max77759_mfd *max77759_mfd,
> +			  const struct max77759_maxq_command *cmd,
> +			  struct max77759_maxq_response *rsp)
> +{
> +	DEFINE_FLEX(struct max77759_maxq_response, _rsp, rsp, length, 1);
> +	int ret;

Nit: Please place this at the bottom of the block.

*twitch*

> +	struct device *dev = regmap_get_device(max77759_mfd->regmap_maxq);

> +	static const unsigned int timeout_ms = 200;

Why 200?

> +
> +	if (cmd->length > MAX77759_MAXQ_REG_AP_MESSAGESZ_MAX)
> +		return -EINVAL;
> +
> +	/* rsp is allowed to be NULL. In that case we do need a temporary. */

More details please.

Why is NULL valid?

Why does it need a placeholder?  What are you using as the placeholder?

> +	if (!rsp)
> +		rsp = _rsp;
> +
> +	BUILD_BUG_ON(MAX77759_MAXQ_OPCODE_MAXLENGTH
> +		     != MAX77759_MAXQ_REG_AP_MESSAGESZ_MAX);

Probably useful during development.  Let useful in production code.

> +	if (!rsp->length || rsp->length > MAX77759_MAXQ_REG_AP_MESSAGESZ_MAX)
> +		return -EINVAL;
> +
> +	guard(mutex)(&max77759_mfd->maxq_lock);
> +
> +	reinit_completion(&max77759_mfd->cmd_done);
> +
> +	/* write the opcode and data */

Upper case throughout please.

> +	ret = regmap_bulk_write(max77759_mfd->regmap_maxq,
> +				MAX77759_MAXQ_REG_AP_DATAOUT0, cmd->cmd,
> +				cmd->length);
> +	if (!ret && cmd->length < MAX77759_MAXQ_REG_AP_MESSAGESZ_MAX)
> +		/* writing the last byte triggers MaxQ */
> +		ret = regmap_write(max77759_mfd->regmap_maxq,
> +				   MAX77759_MAXQ_REG_AP_DATAOUT32, 0);
> +	if (ret) {

Do this first.

Then you only need to check:

  cmd->length < MAX77759_MAXQ_REG_AP_MESSAGESZ_MAX

... in the subsequent statement.

But what happens if:

  !ret && cmd->length >= MAX77759_MAXQ_REG_AP_MESSAGESZ_MAX
                                         ^
Is that okay?  Nothing to be done?

> +		dev_warn(dev, "write data failed: %d\n", ret);

This should be dev_err().

> +		return ret;
> +	}
> +
> +	/* wait for response from MaxQ */
> +	if (!wait_for_completion_timeout(&max77759_mfd->cmd_done,
> +					 msecs_to_jiffies(timeout_ms))) {
> +		dev_err(dev, "timed out waiting for data\n");

What data?

> +		return -ETIMEDOUT;
> +	}
> +
> +	ret = regmap_bulk_read(max77759_mfd->regmap_maxq,
> +			       MAX77759_MAXQ_REG_AP_DATAIN0,
> +			       rsp->rsp, rsp->length);
> +	if (ret) {
> +		dev_warn(dev, "read data failed: %d\n", ret);

As above.

> +		return ret;
> +	}
> +
> +	/*
> +	 * As per the protocol, the first byte of the reply will match the
> +	 * request.
> +	 */
> +	if (cmd->cmd[0] != rsp->rsp[0]) {
> +		dev_warn(dev, "unexpected opcode response for %#.2x: %*ph\n",
> +			 cmd->cmd[0], (int)rsp->length, rsp->rsp);
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(max77759_maxq_command);
> +
> +static irqreturn_t apcmdres_irq_handler(int irq, void *irq_data)
> +{
> +	struct max77759_mfd *max77759_mfd = irq_data;
> +
> +	regmap_write(max77759_mfd->regmap_maxq, MAX77759_MAXQ_REG_UIC_INT1,
> +		     MAX77759_MAXQ_REG_UIC_INT1_APCMDRESI);
> +
> +	complete(&max77759_mfd->cmd_done);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int max77759_create_i2c_subdev(struct i2c_client *client,
> +				      struct max77759_mfd *max77759_mfd,
> +				      const struct max77759_i2c_subdev *sd)
> +{
> +	struct i2c_client *sub;
> +	struct regmap *regmap;
> +	int ret;
> +
> +	if (sd->i2c_address) {
> +		sub = devm_i2c_new_dummy_device(&client->dev,
> +						client->adapter,
> +						sd->i2c_address);
> +
> +		if (IS_ERR(sub))
> +			return dev_err_probe(&client->dev, PTR_ERR(sub),
> +					"failed to claim i2c device %s\n",

"I2C"

> +					sd->cfg->name);
> +	} else {
> +		sub = client;

Assign during declaration, with a comment, then dispose of this leg.

> +	}
> +
> +	regmap = devm_regmap_init_i2c(sub, sd->cfg);
> +	if (IS_ERR(regmap))
> +		return dev_err_probe(&sub->dev, PTR_ERR(regmap),
> +				     "regmap init failed\n");
> +
> +	ret = regmap_attach_dev(&client->dev, regmap, sd->cfg);
> +	if (ret)
> +		return dev_err_probe(&client->dev, ret,
> +				     "regmap attach failed\n");
> +
> +	if (sd->id == MAX77759_I2C_SUBDEV_ID_MAXQ)
> +		max77759_mfd->regmap_maxq = regmap;
> +	else if (sd->id == MAX77759_I2C_SUBDEV_ID_CHARGER)
> +		max77759_mfd->regmap_charger = regmap;
> +
> +	return 0;
> +}
> +
> +static int max77759_add_chained_irq_chip(struct device *dev,
> +					 struct regmap *regmap,
> +					 int pirq,
> +					 struct regmap_irq_chip_data *parent,
> +					 const struct regmap_irq_chip *chip,
> +					 struct regmap_irq_chip_data **data)
> +{
> +	int irq, ret;
> +
> +	irq = regmap_irq_get_virq(parent, pirq);
> +	if (irq < 0)
> +		return dev_err_probe(dev, irq,
> +				     "Failed to get parent vIRQ(%d) for chip %s\n",
> +				     pirq, chip->name);
> +
> +	ret = devm_regmap_add_irq_chip(dev, regmap, irq,
> +				       IRQF_ONESHOT | IRQF_SHARED, 0, chip,
> +				       data);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to add %s IRQ chip\n",

Some of your messages start will uppercase whilst others do not.

*twitch*

Please pick one and use throughout.

> +				     chip->name);
> +
> +	return 0;
> +}
> +
> +static int max77759_add_chained_maxq(struct i2c_client *client,
> +				     struct max77759_mfd *max77759_mfd,
> +				     struct regmap_irq_chip_data *parent)
> +{
> +	struct regmap_irq_chip_data *irq_chip_data;
> +	int ret;

Bottom please.

Some people like reverse Christmas tree.

Structure helps me sleep at night!  =;-)

> +	int apcmdres_irq;
> +
> +	ret = max77759_add_chained_irq_chip(&client->dev,
> +					    max77759_mfd->regmap_maxq,
> +					    MAX77759_INT_MAXQ,
> +					    parent,
> +					    &max77759_maxq_irq_chip,
> +					    &irq_chip_data);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * We need to register our own IRQ handler before any MFD cells, to
> +	 * ensure client drivers can use our MaxQ interface APIs without
> +	 * any race conditions.
> +	 */

This is normal.

> +	init_completion(&max77759_mfd->cmd_done);
> +	apcmdres_irq = regmap_irq_get_virq(irq_chip_data,
> +					   MAX77759_MAXQ_INT_APCMDRESI);
> +
> +	ret = devm_request_threaded_irq(&client->dev, apcmdres_irq,
> +					NULL, apcmdres_irq_handler,
> +					IRQF_ONESHOT | IRQF_SHARED,
> +					dev_name(&client->dev), max77759_mfd);
> +	if (ret)
> +		return dev_err_probe(&client->dev, ret,
> +				     "MAX77759_MAXQ_INT_APCMDRESI failed\n");
> +
> +	ret = devm_mfd_add_devices(&client->dev, PLATFORM_DEVID_AUTO,
> +				   max77759_maxq_cells,
> +				   ARRAY_SIZE(max77759_maxq_cells),
> +				   NULL, 0,
> +				   regmap_irq_get_domain(irq_chip_data));
> +	if (ret)
> +		return dev_err_probe(&client->dev, ret,
> +				     "failed to add MFD devices (MaxQ)\n");
> +
> +	return 0;
> +}
> +
> +static int max77759_add_chained_topsys(struct i2c_client *client,
> +				       struct max77759_mfd *max77759_mfd,
> +				       struct regmap_irq_chip_data *parent)
> +{
> +	struct regmap_irq_chip_data *irq_chip_data;
> +	int ret;
> +
> +	ret = max77759_add_chained_irq_chip(&client->dev,
> +					    max77759_mfd->regmap_top,
> +					    MAX77759_INT_TOPSYS,
> +					    parent,
> +					    &max77759_topsys_irq_chip,
> +					    &irq_chip_data);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int max77759_add_chained_charger(struct i2c_client *client,
> +					struct max77759_mfd *max77759_mfd,
> +					struct regmap_irq_chip_data *parent)
> +{
> +	struct regmap_irq_chip_data *irq_chip_data;
> +	int ret;
> +
> +	ret = max77759_add_chained_irq_chip(&client->dev,
> +					    max77759_mfd->regmap_charger,
> +					    MAX77759_INT_CHGR,
> +					    parent,
> +					    &max77759_chrg_irq_chip,
> +					    &irq_chip_data);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_mfd_add_devices(&client->dev, PLATFORM_DEVID_AUTO,
> +				   max77759_charger_cells,
> +				   ARRAY_SIZE(max77759_charger_cells),
> +				   NULL, 0,
> +				   regmap_irq_get_domain(irq_chip_data));
> +	if (ret)
> +		return dev_err_probe(&client->dev, ret,
> +				     "failed to add MFD devices (charger)\n");

"sub" or "child" devices.

> +
> +	return 0;
> +}
> +
> +static int max77759_probe(struct i2c_client *client)
> +{
> +	struct regmap *regmap_top;
> +	unsigned int pmic_id;
> +	int ret;
> +	struct irq_data *irq_data;
> +	struct max77759_mfd *max77759_mfd;
> +	unsigned long irq_flags;
> +	struct regmap_irq_chip_data *irq_chip_data_pmic;

Structure !

> +
> +	regmap_top = devm_regmap_init_i2c(client, &max77759_regmap_config_top);
> +	if (IS_ERR(regmap_top))
> +		return dev_err_probe(&client->dev, PTR_ERR(regmap_top),
> +				     "regmap init failed\n");

Which regmap failed?

> +	ret = regmap_read(regmap_top, MAX77759_PMIC_REG_PMIC_ID, &pmic_id);
> +	if (ret)
> +		return dev_err_probe(&client->dev, ret,
> +				     "Unable to read Device ID\n");
> +
> +	if (pmic_id != MAX77759_PMIC_REG_PMIC_ID_MAX77759)
> +		return dev_err_probe(&client->dev, -ENODEV,
> +				     "Unsupported Device ID %#.2x (%d)\n",
> +				     pmic_id, pmic_id);
> +
> +	irq_data = irq_get_irq_data(client->irq);
> +	if (!irq_data)
> +		return dev_err_probe(&client->dev, -EINVAL,
> +				     "Invalid IRQ: %d\n", client->irq);
> +
> +	max77759_mfd = devm_kzalloc(&client->dev, sizeof(*max77759_mfd),
> +				    GFP_KERNEL);
> +	if (!max77759_mfd)
> +		return -ENOMEM;

Device data is almost universally allocated at the top of .probe().

> +	max77759_mfd->regmap_top = regmap_top;

These two blocks are unrelated - please separate them with a line break here.

> +	ret = devm_mutex_init(&client->dev, &max77759_mfd->maxq_lock);
> +	if (ret)
> +		return ret;
> +
> +	i2c_set_clientdata(client, max77759_mfd);
> +
> +	for (int i = 0; i < ARRAY_SIZE(max77759_i2c_subdevs); ++i) {

Any reason for the pre-increment?

If not, it's more standards to post-inc.

> +		ret = max77759_create_i2c_subdev(client,
> +						 max77759_mfd,
> +						 &max77759_i2c_subdevs[i]);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	irq_flags = IRQF_ONESHOT | IRQF_SHARED;
> +	irq_flags |= irqd_get_trigger_type(irq_data);
> +
> +	ret = devm_regmap_add_irq_chip(&client->dev, max77759_mfd->regmap_top,
> +				       client->irq, irq_flags, 0,
> +				       &max77759_pmic_irq_chip,
> +				       &irq_chip_data_pmic);
> +	if (ret)
> +		return dev_err_probe(&client->dev, ret,
> +				     "Failed to add IRQ chip\n");
> +
> +	/* INTSRC - MaxQ & children */

What does these comments add beyond what is already obvious by the nomenclature?

> +	ret = max77759_add_chained_maxq(client, max77759_mfd,
> +					irq_chip_data_pmic);
> +	if (ret)
> +		return ret;
> +
> +	/* INTSRC - topsys & children */
> +	ret = max77759_add_chained_topsys(client, max77759_mfd,
> +					  irq_chip_data_pmic);
> +	if (ret)
> +		return ret;
> +
> +	/* INTSRC - charger & children */
> +	ret = max77759_add_chained_charger(client, max77759_mfd,
> +					   irq_chip_data_pmic);
> +	if (ret)
> +		return ret;
> +
> +	return devm_mfd_add_devices(&client->dev, PLATFORM_DEVID_AUTO,
> +				    max77759_cells, ARRAY_SIZE(max77759_cells),
> +				    NULL, 0,
> +				    regmap_irq_get_domain(irq_chip_data_pmic));
> +}
> +
> +static const struct i2c_device_id max77759_i2c_id[] = {
> +	{ "max77759", 0 },

The driver data attribute can be omitted if it's 0.

> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, max77759_i2c_id);
> +
> +static const struct of_device_id max77759_of_id[] = {
> +	{ .compatible = "maxim,max77759", },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, max77759_of_id);
> +
> +static struct i2c_driver max77759_i2c_driver = {
> +	.driver = {
> +		.name = "max77759",
> +		.of_match_table = max77759_of_id,
> +	},
> +	.probe = max77759_probe,
> +	.id_table = max77759_i2c_id,
> +};
> +module_i2c_driver(max77759_i2c_driver);
> +
> +MODULE_AUTHOR("André Draszik <andre.draszik@linaro.org>");
> +MODULE_DESCRIPTION("Maxim MAX77759 multi-function core driver");

No it isn't. :)

> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/mfd/max77759.h b/include/linux/mfd/max77759.h
> new file mode 100644
> index 0000000000000000000000000000000000000000..b038b4e9b748287e23e3a7030496f09dc8bdc816
> --- /dev/null
> +++ b/include/linux/mfd/max77759.h
> @@ -0,0 +1,98 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright 2020 Google Inc.
> + * Copyright 2025 Linaro Ltd.
> + *
> + * Client interface for Maxim MAX77759 MFD driver
> + */
> +
> +#ifndef __LINUX_MFD_MAX77759_H
> +#define __LINUX_MFD_MAX77759_H
> +
> +/* MaxQ opcodes */
> +#define MAX77759_MAXQ_OPCODE_MAXLENGTH 33
> +
> +#define MAX77759_MAXQ_OPCODE_GPIO_TRIGGER_READ   0x21
> +#define MAX77759_MAXQ_OPCODE_GPIO_TRIGGER_WRITE  0x22
> +#define MAX77759_MAXQ_OPCODE_GPIO_CONTROL_READ   0x23
> +#define MAX77759_MAXQ_OPCODE_GPIO_CONTROL_WRITE  0x24
> +#define MAX77759_MAXQ_OPCODE_USER_SPACE_READ     0x81
> +#define MAX77759_MAXQ_OPCODE_USER_SPACE_WRITE    0x82
> +
> +/*
> + * register map (incomplete) - registers not useful for drivers are not
> + * declared here
> + */
> +/* MaxQ */
> +#define MAX77759_MAXQ_REG_UIC_INT1            0x64
> +#define MAX77759_MAXQ_REG_UIC_INT1_APCMDRESI  BIT(7)
> +#define MAX77759_MAXQ_REG_UIC_INT1_SYSMSGI    BIT(6)
> +#define MAX77759_MAXQ_REG_UIC_INT1_GPIO6I     BIT(1)
> +#define MAX77759_MAXQ_REG_UIC_INT1_GPIO5I     BIT(0)
> +#define MAX77759_MAXQ_REG_UIC_INT1_GPIOxI(offs, en)  (((en) & 1) << (offs))
> +#define MAX77759_MAXQ_REG_UIC_INT1_GPIOxI_MASK(offs) \
> +				MAX77759_MAXQ_REG_UIC_INT1_GPIOxI(offs, ~0)
> +
> +#define MAX77759_MAXQ_REG_UIC_INT2            0x65
> +#define MAX77759_MAXQ_REG_UIC_INT3            0x66
> +#define MAX77759_MAXQ_REG_UIC_INT4            0x67
> +#define MAX77759_MAXQ_REG_UIC_UIC_STATUS1     0x68
> +#define MAX77759_MAXQ_REG_UIC_UIC_STATUS2     0x69
> +#define MAX77759_MAXQ_REG_UIC_UIC_STATUS3     0x6a
> +#define MAX77759_MAXQ_REG_UIC_UIC_STATUS4     0x6b
> +#define MAX77759_MAXQ_REG_UIC_UIC_STATUS5     0x6c
> +#define MAX77759_MAXQ_REG_UIC_UIC_STATUS6     0x6d
> +#define MAX77759_MAXQ_REG_UIC_UIC_STATUS7     0x6f
> +#define MAX77759_MAXQ_REG_UIC_UIC_STATUS8     0x6f
> +#define MAX77759_MAXQ_REG_UIC_INT1_M          0x70
> +#define MAX77759_MAXQ_REG_UIC_INT2_M          0x71
> +#define MAX77759_MAXQ_REG_UIC_INT3_M          0x72
> +#define MAX77759_MAXQ_REG_UIC_INT4_M          0x73
> +
> +/* charger */
> +#define MAX77759_CHGR_REG_CHG_INT        0xb0
> +#define MAX77759_CHGR_REG_CHG_INT2       0xb1
> +#define MAX77759_CHGR_REG_CHG_INT_MASK   0xb2
> +#define MAX77759_CHGR_REG_CHG_INT2_MASK  0xb3
> +
> +struct max77759_mfd;

Place the definition in here instead.

> +/**
> + * struct max77759_maxq_command - structure containing the MaxQ command to
> + * send
> + *
> + * @length: The number of bytes to send.
> + * @cmd: The data to send.
> + */
> +struct max77759_maxq_command {
> +	u8 length;
> +	u8 cmd[] __counted_by(length);
> +};
> +
> +/**
> + * struct max77759_maxq_response - structure containing the MaxQ response
> + *
> + * @length: The number of bytes to receive.
> + * @rsp: The data received. Must have at least @length bytes space.
> + */
> +struct max77759_maxq_response {
> +	u8 length;
> +	u8 rsp[] __counted_by(length);
> +};
> +
> +/**
> + * max77759_maxq_command() - issue a MaxQ command and wait for the response
> + * and associated data
> + *
> + * @max77759_mfd: The core max77759 mfd device handle.
> + * @cmd: The command to be sent.
> + * @rsp: Any response data associated with the command will be copied here;
> + *     can be %NULL if the command has no response (other than ACK).
> + *
> + * Return: 0 on success, a negative error number otherwise.
> + */
> +int max77759_maxq_command(struct max77759_mfd *max77759_mfd,
> +			  const struct max77759_maxq_command *cmd,
> +			  struct max77759_maxq_response *rsp);
> +
> +#endif /* __LINUX_MFD_MAX77759_H */
> 
> -- 
> 2.49.0.rc0.332.g42c0ae87b1-goog
> 

-- 
Lee Jones [李琼斯]