Re: [PATCH v4 3/5] mfd: aaeon: Add SRG-IMX8P MCU driver

[Thomas Perrot]

Hello Lee,

Thank you for the review. Please find answers to your questions inline,
and the remaining items will be addressed in v5.

On Tue, 2026-03-31 at 14:08 +0100, Lee Jones wrote:
> On Tue, 24 Mar 2026, Thomas Perrot (Schneider Electric) wrote:
> 
> > Add Multi-Function Device (MFD) driver for the Aaeon SRG-IMX8P
> > embedded controller. This driver provides the core I2C
> > communication
> > interface and registers child devices (GPIO and watchdog
> > controllers).
> > 
> > The driver implements a custom regmap bus over I2C to match the
> > MCU's
> > fixed 3-byte command format [opcode, arg, value]. Register
> > addresses
> > are encoded as 16-bit values (opcode << 8 | arg) using the
> > AAEON_MCU_REG() macro defined in the shared header. The regmap
> > instance is shared with child drivers via dev_get_regmap().
> > Concurrent
> > I2C accesses from child drivers are serialized by regmap's built-in
> > locking.
> > 
> > Co-developed-by: Jérémie Dautheribes (Schneider Electric)
> > <jeremie.dautheribes@xxxxxxxxxxx>
> > Signed-off-by: Jérémie Dautheribes (Schneider Electric)
> > <jeremie.dautheribes@xxxxxxxxxxx>
> > Signed-off-by: Thomas Perrot (Schneider Electric)
> > <thomas.perrot@xxxxxxxxxxx>
> > ---
> >  MAINTAINERS                   |   2 +
> >  drivers/mfd/Kconfig           |  10 +++
> >  drivers/mfd/Makefile          |   1 +
> >  drivers/mfd/aaeon-mcu.c       | 155
> > ++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/mfd/aaeon-mcu.h |  20 ++++++
> >  5 files changed, 188 insertions(+)
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index
> > ea9d55f76f3509c7f6ba6d1bc86ca2e2e71aa954..f91b6a1826d04bef8a0f88221
> > f6c8e8a3652cd77 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -191,6 +191,8 @@ M:	Thomas Perrot <thomas.perrot@xxxxxxxxxxx>
> >  R:	Jérémie Dautheribes <jeremie.dautheribes@xxxxxxxxxxx>
> >  S:	Maintained
> >  F:	Documentation/devicetree/bindings/mfd/aaeon,srg-imx8p-
> > mcu.yaml
> > +F:	drivers/mfd/aaeon-mcu.c
> > +F:	include/linux/mfd/aaeon-mcu.h
> >  
> >  AAEON UPBOARD FPGA MFD DRIVER
> >  M:	Thomas Richard <thomas.richard@xxxxxxxxxxx>
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > index
> > aace5766b38aa5e46e32a8a7b42eea238159fbcf..7a1ceedece899faad7a03a1fe
> > 7b1c91b72253c05 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -1574,6 +1574,16 @@ config AB8500_CORE
> >  	  the irq_chip parts for handling the Mixed Signal chip
> > events.
> >  	  This chip embeds various other multimedia
> > functionalities as well.
> >  
> > +config MFD_AAEON_MCU
> > +	tristate "Aaeon SRG-IMX8P MCU Driver"
> > +	depends on I2C || COMPILE_TEST
> > +	select MFD_CORE
> > +	help
> > +	  Select this option to enable support for the Aaeon SRG-
> > IMX8P
> > +	  onboard microcontroller (MCU). This driver provides the
> > core
> > +	  functionality to communicate with the MCU over I2C. The
> > MCU
> > +	  provides GPIO and watchdog functionality.
> > +
> >  config MFD_DB8500_PRCMU
> >  	bool "ST-Ericsson DB8500 Power Reset Control Management
> > Unit"
> >  	depends on UX500_SOC_DB8500
> > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> > index
> > e75e8045c28afae975ac61d282b3b85af5440119..34db5b033584368b7a269b1ee
> > f12528a74baf8f5 100644
> > --- a/drivers/mfd/Makefile
> > +++ b/drivers/mfd/Makefile
> > @@ -8,6 +8,7 @@ obj-$(CONFIG_MFD_88PM860X)	+= 88pm860x.o
> >  obj-$(CONFIG_MFD_88PM800)	+= 88pm800.o 88pm80x.o
> >  obj-$(CONFIG_MFD_88PM805)	+= 88pm805.o 88pm80x.o
> >  obj-$(CONFIG_MFD_88PM886_PMIC)	+= 88pm886.o
> > +obj-$(CONFIG_MFD_AAEON_MCU)	+= aaeon-mcu.o
> >  obj-$(CONFIG_MFD_ACT8945A)	+= act8945a.o
> >  obj-$(CONFIG_MFD_SM501)		+= sm501.o
> >  obj-$(CONFIG_ARCH_BCM2835)	+= bcm2835-pm.o
> > diff --git a/drivers/mfd/aaeon-mcu.c b/drivers/mfd/aaeon-mcu.c
> > new file mode 100644
> > index
> > 0000000000000000000000000000000000000000..5a969890d201c027eb25c324b
> > 4d4d89b1f8c563e
> > --- /dev/null
> > +++ b/drivers/mfd/aaeon-mcu.c
> > @@ -0,0 +1,155 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Aaeon MCU driver
> > + *
> > + * Copyright (C) 2025 Bootlin
> > + * Author: Jérémie Dautheribes <jeremie.dautheribes@xxxxxxxxxxx>
> > + * Author: Thomas Perrot <thomas.perrot@xxxxxxxxxxx>
> > + */
> 
> Consider updating the Copyright date - we're pretty deep into 2026 at
> this point.
> 
> > +#include <linux/err.h>
> > +#include <linux/i2c.h>
> > +#include <linux/mfd/core.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> > +
> > +static const struct mfd_cell aaeon_mcu_devs[] = {
> > +	{
> > +		.name = "aaeon-mcu-wdt",
> > +	},
> > +	{
> > +		.name = "aaeon-mcu-gpio",
> > +	},
> > +};
> 
> MFD_CELL_BASIC()
> 
> > +/*
> > + * Custom regmap bus for the Aaeon MCU I2C protocol.
> > + *
> > + * The MCU uses a fixed 3-byte command format [opcode, arg, value]
> > followed
> > + * by a 1-byte response.  It requires a STOP condition between the
> > command
> > + * write and the response read, so two separate i2c_transfer()
> > calls are
> > + * issued.  The regmap lock serialises concurrent accesses from
> > the GPIO
> > + * and watchdog child drivers.
> > + *
> > + * Register addresses are encoded as a 16-bit big-endian value
> > where the
> > + * high byte is the opcode and the low byte is the argument,
> > matching the
> > + * wire layout produced by regmap for reg_bits=16.
> > + */
> > +
> > +static int aaeon_mcu_regmap_write(void *context, const void *data,
> > size_t count)
> > +{
> > +	struct i2c_client *client = context;
> > +	/* data = [opcode, arg, value] as formatted by regmap */
> > +	struct i2c_msg write_msg = {
> > +		.addr  = client->addr,
> > +		.flags = 0,
> > +		.buf   = (u8 *)data,
> > +		.len   = count,
> > +	};
> > +	u8 rsp;
> > +	/* The MCU always sends a response byte after each
> > command; discard it. */
> > +	struct i2c_msg rsp_msg = {
> 
> Assuming 'rsp' means response, let's just write that out in full.
> 
> Readability wins over brevity every time.
> 
> > +		.addr  = client->addr,
> > +		.flags = I2C_M_RD,
> > +		.buf   = &rsp,
> > +		.len   = 1,
> > +	};
> > +	int ret;
> 
> Since some I2C host controllers might use DMA, should we ensure that
> the
> 'rsp' buffer is allocated in DMA-safe memory rather than on the stack
> to
> prevent potential cache-line corruption?
> 
> Also allocation of structs during in declaration statements is rough!
> 
> And adding that u8 in the middle is just rubbing it in.
> 
> > +	ret = i2c_transfer(client->adapter, &write_msg, 1);
> > +	if (ret < 0)
> > +		return ret;
> > +	if (ret != 1)
> > +		return -EIO;
> > +
> > +	ret = i2c_transfer(client->adapter, &rsp_msg, 1);
> > +	if (ret < 0)
> > +		return ret;
> > +	if (ret != 1)
> > +		return -EIO;
> > +
> > +	return 0;
> > +}
> > +
> > +static int aaeon_mcu_regmap_read(void *context, const void
> > *reg_buf,
> > +				 size_t reg_size, void *val_buf,
> > size_t val_size)
> > +{
> > +	struct i2c_client *client = context;
> > +	/*
> > +	 * reg_buf holds the 2-byte big-endian register address
> > [opcode, arg].
> > +	 * Append a trailing 0x00 to form the full 3-byte MCU
> > command.
> > +	 */
> > +	u8 cmd[3] = { ((u8 *)reg_buf)[0], ((u8 *)reg_buf)[1], 0x00
> > };
> > +	struct i2c_msg write_msg = {
> > +		.addr  = client->addr,
> > +		.flags = 0,
> > +		.buf   = cmd,
> > +		.len   = sizeof(cmd),
> > +	};
> > +	struct i2c_msg read_msg = {
> > +		.addr  = client->addr,
> > +		.flags = I2C_M_RD,
> > +		.buf   = val_buf,
> > +		.len   = val_size,
> > +	};
> > +	int ret;
> > +
> > +	ret = i2c_transfer(client->adapter, &write_msg, 1);
> > +	if (ret < 0)
> > +		return ret;
> > +	if (ret != 1)
> > +		return -EIO;
> > +
> > +	ret = i2c_transfer(client->adapter, &read_msg, 1);
> > +	if (ret < 0)
> > +		return ret;
> > +	if (ret != 1)
> > +		return -EIO;
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct regmap_bus aaeon_mcu_regmap_bus = {
> > +	.write = aaeon_mcu_regmap_write,
> > +	.read  = aaeon_mcu_regmap_read,
> > +};
> > +
> > +static const struct regmap_config aaeon_mcu_regmap_config = {
> > +	.reg_bits          = 16,
> > +	.val_bits          = 8,
> > +	.reg_format_endian = REGMAP_ENDIAN_BIG,
> > +	.cache_type        = REGCACHE_NONE,
> 
> Are you sure?  Why none?

The GPIO and watchdog states are managed entirely by the MCU firmware,
which makes the design safer because every access goes directly to the
hardware. I will look into adding a cache; otherwise I will add a
comment in v5.

> 
> > +};
> > +
> > +static int aaeon_mcu_probe(struct i2c_client *client)
> > +{
> > +	struct regmap *regmap;
> > +
> > +	regmap = devm_regmap_init(&client->dev,
> > &aaeon_mcu_regmap_bus,
> > +				  client,
> > &aaeon_mcu_regmap_config);
> > +	if (IS_ERR(regmap))
> > +		return PTR_ERR(regmap);
> 
> dev_err_probe()
> 
> > +
> > +	return devm_mfd_add_devices(&client->dev,
> > PLATFORM_DEVID_NONE,
> > +				    aaeon_mcu_devs,
> > ARRAY_SIZE(aaeon_mcu_devs),
> > +				    NULL, 0, NULL);
> 
> Why PLATFORM_DEVID_NONE over AUTO here?

No strong reason, it was an oversight. Since multiple instances of this
MCU could theoretically be present, AUTO is the safer choice and avoids
potential ID collisions.
Fixed in v5.

> 
> > +}
> > +
> > +static const struct of_device_id aaeon_mcu_of_match[] = {
> > +	{ .compatible = "aaeon,srg-imx8p-mcu" },
> > +	{},
> > +};
> > +MODULE_DEVICE_TABLE(of, aaeon_mcu_of_match);
> > +
> > +static struct i2c_driver aaeon_mcu_driver = {
> > +	.driver = {
> > +		.name = "aaeon_mcu",
> > +		.of_match_table = aaeon_mcu_of_match,
> > +	},
> > +	.probe = aaeon_mcu_probe,
> > +};
> > +module_i2c_driver(aaeon_mcu_driver);
> > +
> > +MODULE_DESCRIPTION("Aaeon MCU Driver");
> > +MODULE_AUTHOR("Jérémie Dautheribes
> > <jeremie.dautheribes@xxxxxxxxxxx>");
> > +MODULE_LICENSE("GPL");
> > diff --git a/include/linux/mfd/aaeon-mcu.h
> > b/include/linux/mfd/aaeon-mcu.h
> > new file mode 100644
> > index
> > 0000000000000000000000000000000000000000..861003f6dfd20424c3785008b
> > d2cf89aaa1715b9
> > --- /dev/null
> > +++ b/include/linux/mfd/aaeon-mcu.h
> > @@ -0,0 +1,20 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Aaeon MCU driver definitions
> > + *
> > + * Copyright (C) 2025 Bootlin
> > + * Author: Jérémie Dautheribes <jeremie.dautheribes@xxxxxxxxxxx>
> > + * Author: Thomas Perrot <thomas.perrot@xxxxxxxxxxx>
> > + */
> 
> As above.
> 
> > +
> > +#ifndef __LINUX_MFD_AAEON_MCU_H
> > +#define __LINUX_MFD_AAEON_MCU_H
> > +
> > +/*
> > + * MCU register address: the high byte is the command opcode, the
> > low
> > + * byte is the argument.  This matches the 3-byte wire format
> > + * [opcode, arg, value] used by the MCU I2C protocol.
> > + */
> > +#define AAEON_MCU_REG(op, arg)	(((op) << 8) | (arg))
> 
> Where else is this used?

It is used by both child drivers:                                     
  - drivers/gpio/gpio-aaeon-mcu.c
  - drivers/watchdog/aaeon_mcu_wdt.c                                  
                                                            
This macro encodes the regmap register address from the opcode and
argument that form the first two bytes of the MCU's 3-byte wire
command, so keeping it in the shared header avoids duplicating that
encoding in each child.

Kind regards,
Thomas Perrot

> 
> > +#endif /* __LINUX_MFD_AAEON_MCU_H */
> > 
> > -- 
> > 2.53.0
> > 

-- 
Thomas Perrot, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Attachment: signature.asc
Description: This is a digitally signed message part