Message-ID: <0c6073253466314acde4394c5b8d3703a59b28b7.camel@bootlin.com>
Date: Wed, 14 Jan 2026 09:59:03 +0100
From: Thomas Perrot <thomas.perrot@...tlin.com>
To: Lee Jones <lee@...nel.org>
Cc: thomas.perrot@...tlin.com, Rob Herring <robh@...nel.org>, Krzysztof
 Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, Linus
 Walleij <linusw@...nel.org>,  Bartosz Golaszewski	 <brgl@...nel.org>, Shawn
 Guo <shawnguo@...nel.org>, Sascha Hauer	 <s.hauer@...gutronix.de>,
 Pengutronix Kernel Team <kernel@...gutronix.de>,  Fabio Estevam
 <festevam@...il.com>, Jérémie Dautheribes
 <jeremie.dautheribes@...tlin.com>,  Wim Van Sebroeck
 <wim@...ux-watchdog.org>, Guenter Roeck <linux@...ck-us.net>,
 devicetree@...r.kernel.org, 	linux-kernel@...r.kernel.org,
 linux-gpio@...r.kernel.org, imx@...ts.linux.dev, 
	linux-arm-kernel@...ts.infradead.org, linux-watchdog@...r.kernel.org,
 Thomas Petazzoni <thomas.petazzoni@...tlin.com>
Subject: Re: [PATCH 5/8] mfd: aaeon: Add SRG-IMX8PL MCU driver

Hello Lee,

On Fri, 2026-01-09 at 17:14 +0000, Lee Jones wrote:
> On Fri, 12 Dec 2025, Thomas Perrot (Schneider Electric) wrote:
> 
> > Add Multi-Function Device (MFD) driver for the Aaeon SRG-IMX8PL
> 
> Drop all mentions of MFD.  It's not a real thing - we made it up.
> 
> > embedded controller. This driver provides the core I2C
> > communication
> > interface and registers child devices (GPIO and watchdog
> > controllers).
> > 
> > The MCU firmware version is queried during probe and logged for
> > diagnostic purposes. All I2C transactions are serialized using a
> > mutex
> > to ensure proper communication with the microcontroller.
> > 
> > Co-developed-by: Jérémie Dautheribes (Schneider Electric)
> > <jeremie.dautheribes@...tlin.com>
> > Signed-off-by: Jérémie Dautheribes (Schneider Electric)
> > <jeremie.dautheribes@...tlin.com>
> > Signed-off-by: Thomas Perrot (Schneider Electric)
> > <thomas.perrot@...tlin.com>
> > ---
> >  drivers/mfd/Kconfig           |  10 ++++
> >  drivers/mfd/aaeon-mcu.c       | 133
> > ++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/mfd/aaeon-mcu.h |  30 ++++++++++
> >  3 files changed, 173 insertions(+)
> > 
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > index
> > aace5766b38aa5e46e32a8a7b42eea238159fbcf..9195115c7bcd619439cb9ff71
> > d70e46629291867 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-IMX8PL MCU Driver"
> > +	depends on I2C
> > +	select MFD_CORE
> > +	help
> > +	  Select this option to enable support for the Aaeon SRG-
> > IMX8PL
> > +	  onboard microcontroller (MCU). This driver provides the
> > core
> > +	  functionality to communicate with the MCU over I2C. The
> > MCU
> > +	  provides various sub-devices including GPIO and watchdog
> > controllers.
> 
> Is that an exhaustive list of sub-devices?
> 
> >  config MFD_DB8500_PRCMU
> >  	bool "ST-Ericsson DB8500 Power Reset Control Management
> > Unit"
> >  	depends on UX500_SOC_DB8500
> > diff --git a/drivers/mfd/aaeon-mcu.c b/drivers/mfd/aaeon-mcu.c
> > new file mode 100644
> > index
> > 0000000000000000000000000000000000000000..472d44d5e8627f46806015599
> > 542753a5bda4526
> > --- /dev/null
> > +++ b/drivers/mfd/aaeon-mcu.c
> > @@ -0,0 +1,133 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Aaeon MCU MFD driver
> 
> Not MFD - describe the actual device.
> 
> > + *
> > + * Copyright (C) 2025 Bootlin
> 
> Has it been agreed that you would hold the copyright to this?
> 

Yes, we would like to retain the copyright.

> > + * Author: Jérémie Dautheribes <jeremie.dautheribes@...tlin.com>
> > + * Author: Thomas Perrot <thomas.perrot@...tlin.com>
> > + */
> > +
> > +#include <linux/err.h>
> > +#include <linux/i2c.h>
> > +#include <linux/mfd/core.h>
> > +#include <linux/mfd/aaeon-mcu.h>
> 
> Alphabetical.
> 
> > +#define AAEON_MCU_GET_FW_VERSION 0x76
> 
> Is that what the register is called in the datasheet?
> 
> The GET part is odd.

There isn't a datasheet; the driver was written using reverse
engineering.

I will rename it to AAEON_MCU_FW_VERSION.

> 
> > +static struct mfd_cell aaeon_mcu_devs[] = {
> > +	{
> > +		.name = "aaeon-mcu-wdt",
> > +		.of_compatible = "aaeon,srg-imx8pl-wdt",
> > +	},
> > +	{
> > +		.name = "aaeon-mcu-gpio",
> > +		.of_compatible = "aaeon,srg-imx8pl-gpio",
> > +	},
> > +};
> > +
> > +static int aaeon_mcu_print_fw_version(struct i2c_client *client)
> > +{
> > +	u8 cmd[3], version[2];
> > +	int ret;
> > +
> > +	/* Major version number */
> > +	cmd[0] = AAEON_MCU_GET_FW_VERSION;
> > +	cmd[1] = 0x00;
> > +	cmd[2] = 0x00;
> > +
> > +	ret = aaeon_mcu_i2c_xfer(client, cmd, 3, &version[0], 1);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	/* Minor version number */
> > +	cmd[0] = AAEON_MCU_GET_FW_VERSION;
> > +	cmd[1] = 0x01;
> > +	/* cmd[2] = 0x00; */
> > +
> > +	ret = aaeon_mcu_i2c_xfer(client, cmd, 3, &version[1], 1);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	dev_info(&client->dev, "firmware version: v%d.%d\n",
> > +		 version[0], version[1]);
> 
> What do you expect a user to do with this information?
> 

I believe this information can be useful, as there may be different
firmware versions, potentially leading to varying behaviors or bugs.
This will help us determine how to extend the driver to implement
specific quirks or features based on the firmware version.

> Let's cut the debug cruft - you can add it again locally if you need
> to debug.
> 
> > +
> > +	return 0;
> > +}
> 
> Besides providing a questionable print, you don't seem to be doing
> anything with this information - is it needed at all?

This information isn't required for now, but it can be useful if
different behavior is observed in the field for products using
different firmware versions.

> 
> > +static int aaeon_mcu_probe(struct i2c_client *client)
> > +{
> > +	struct aaeon_mcu_dev *mcu;
> > +	int ret;
> > +
> > +	mcu = devm_kzalloc(&client->dev, sizeof(*mcu),
> > GFP_KERNEL);
> > +	if (!mcu)
> > +		return -ENOMEM;
> > +
> > +	i2c_set_clientdata(client, mcu);
> 
> > +	mcu->dev = &client->dev;
> > +	mcu->i2c_client = client;
> 
> How do you expect to be able to 'get' this data back if you do not
> have
> the 'dev' or the 'client'?

Thank you. I will remove the client and use to_i2c_client() instead.

> 
> > +	mutex_init(&mcu->i2c_lock);
> > +
> > +	ret = aaeon_mcu_print_fw_version(client);
> > +	if (ret) {
> > +		dev_err(&client->dev, "unable to read firmware
> > version\n");
> > +		return ret;
> > +	}
> > +
> > +	return devm_mfd_add_devices(mcu->dev, PLATFORM_DEVID_NONE,
> > aaeon_mcu_devs,
> > +				    ARRAY_SIZE(aaeon_mcu_devs),
> > NULL, 0, NULL);
> > +}
> > +
> > +int aaeon_mcu_i2c_xfer(struct i2c_client *client,
> > +		       const u8 *cmd, int cmd_len,
> > +		       u8 *rsp, int rsp_len)
> > +{
> > +	struct aaeon_mcu_dev *mcu = i2c_get_clientdata(client);
> > +	int ret;
> > +
> > +	mutex_lock(&mcu->i2c_lock);
> > +
> > +	ret = i2c_master_send(client, cmd, cmd_len);
> > +	if (ret < 0)
> > +		goto unlock;
> > +
> > +	ret = i2c_master_recv(client, rsp, rsp_len);
> > +	if (ret < 0)
> > +		goto unlock;
> 
> Isn't this all very generic?

In this case, it doesn’t seem possible to use i2c_transfer() because
the mcu requires a stop condition after receiving the command before it
can respond.

Kind regards,
Thomas Perrot

> I wonder how many similar functions there are in the kernel.
> 
> Worth making this global?
> 
> > +	if (ret != rsp_len) {
> > +		dev_err(&client->dev,
> > +			"i2c recv count error (expected: %d,
> > actual: %d)\n",
> > +			rsp_len, ret);
> > +		ret = -EIO;
> > +		goto unlock;
> > +	}
> > +
> > +	ret = 0;
> > +
> > +unlock:
> > +	mutex_unlock(&mcu->i2c_lock);
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(aaeon_mcu_i2c_xfer);
> 
> This should be much further up.  At least above probe - perhaps
> higher.
> > +static const struct of_device_id aaeon_mcu_of_match[] = {
> > +	{ .compatible = "aaeon,srg-imx8pl-mcu" },
> > +	{},
> > +};
> > +
> 
> Remove this line.
> 
> > +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,
> > +};
> > +
> 
> And this one.
> 
> > +module_i2c_driver(aaeon_mcu_driver);
> > +
> > +MODULE_DESCRIPTION("Aaeon MCU MFD Driver");
> 
> Not MFD.
> 
> > +MODULE_AUTHOR("Jérémie Dautheribes");
> 
> Email?
> 
> > +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..89632cb46bc6c9518755dc43a
> > fb87faa94acb6f5
> > --- /dev/null
> > +++ b/include/linux/mfd/aaeon-mcu.h
> > @@ -0,0 +1,30 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Aaeon MCU driver definitions
> > + *
> > + * Copyright (C) 2025 Bootlin
> > + * Author: Jérémie Dautheribes <jeremie.dautheribes@...tlin.com>
> > + * Author: Thomas Perrot <thomas.perrot@...tlin.com>
> > + */
> > +
> > +#ifndef __LINUX_MFD_AAEON_MCU_H
> > +#define __LINUX_MFD_AAEON_MCU_H
> > +
> > +/**
> > + * struct aaeon_mcu_dev - Internal representation of the Aaeon MCU
> > + * @dev: Pointer to kernel device structure
> > + * @i2c_client: Pointer to the Aaeon MCU I2C client
> > + * @i2c_lock: Mutex to serialize I2C bus access
> > + */
> > +
> > +struct aaeon_mcu_dev {
> > +	struct device *dev;
> > +	struct i2c_client *i2c_client;
> > +	struct mutex i2c_lock;
> > +};
> > +
> > +int aaeon_mcu_i2c_xfer(struct i2c_client *client,
> > +		       const u8 *cmd, int cmd_len,
> > +		       u8 *rsp, int rsp_len);
> > +
> > +#endif /*  __LINUX_MFD_AAEON_MCU_H */
> > 
> > -- 
> > 2.52.0
> > 

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

Download attachment "signature.asc" of type "application/pgp-signature" (660 bytes)

Powered by blists - more mailing lists