| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAMRc=McYKZ8TY9wAhZ3sZtYRNJhoh2VfQ+6FA=+VRwstpWQOHg@mail.gmail.com>
Date: Tue, 27 Jan 2026 01:51:17 -0800
From: Bartosz Golaszewski <brgl@...nel.org>
To: "Thomas Perrot (Schneider Electric)" <thomas.perrot@...tlin.com>
Cc: 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>, 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>, Lee Jones <lee@...nel.org>
Subject: Re: [PATCH v2 3/5] mfd: aaeon: Add SRG-IMX8PL MCU driver
On Fri, 23 Jan 2026 10:54:32 +0100, "Thomas Perrot (Schneider
Electric)" <thomas.perrot@...tlin.com> said:
> Add Multi-Function Device (MFD) driver for the Aaeon SRG-IMX8PL
> 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>
> ---
[snip]
+++ b/drivers/mfd/aaeon-mcu.c
> @@ -0,0 +1,129 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Aaeon MCU driver
> + *
> + * Copyright (C) 2025 Bootlin
> + * 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/aaeon-mcu.h>
> +#include <linux/mfd/core.h>
> +#include <linux/platform_device.h>
> +
> +#define AAEON_MCU_FW_VERSION 0x76
> +
> +static struct mfd_cell aaeon_mcu_devs[] = {
Can be const.
> + {
> + .name = "aaeon-mcu-wdt",
> + },
> + {
> + .name = "aaeon-mcu-gpio",
> + },
> +};
> +
> +static int aaeon_mcu_read_version(struct device *dev, u8 index, u8 *version)
> +{
> + u8 cmd[3] = { AAEON_MCU_FW_VERSION, index, 0x00 };
> +
> + return aaeon_mcu_i2c_xfer(dev, cmd, sizeof(cmd), version, sizeof(*version));
> +}
> +
> +static int aaeon_mcu_print_fw_version(struct i2c_client *client)
> +{
> + struct device *dev = &client->dev;
> + u8 major, minor;
> + int ret;
> +
> + ret = aaeon_mcu_read_version(dev, 0x00, &major);
> + if (ret)
> + return ret;
> +
> + ret = aaeon_mcu_read_version(dev, 0x01, &minor);
> + if (ret)
> + return ret;
> +
> + dev_info(dev, "firmware version: v%d.%d\n", major, minor);
I would drop this entirely or demote it to dev_dbg(), there's no reason for
a successfully probed device to yell about it.
> +
> + return 0;
> +}
> +
> +int aaeon_mcu_i2c_xfer(struct device *dev,
> + const u8 *cmd, int cmd_len,
> + u8 *rsp, int rsp_len)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + 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;
> +
> + if (ret != rsp_len) {
> + dev_err(dev,
> + "i2c recv count error (expected: %d, actual: %d)\n",
> + rsp_len, ret);
> + ret = -EIO;
> + goto unlock;
> + }
> +
> + ret = 0;
> +
> +unlock:
Just use guard(mutex) from cleanup.h and drop the label.
> + mutex_unlock(&mcu->i2c_lock);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(aaeon_mcu_i2c_xfer);
> +
> +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;
> + mutex_init(&mcu->i2c_lock);
For lock debugging purposes it's better to destroy the mutex too. Or use
devm_mutex_init().
> +
> + 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);
> +}
> +
> +static const struct of_device_id aaeon_mcu_of_match[] = {
> + { .compatible = "aaeon,srg-imx8pl-mcu" },
> + {},
> +};
> +
Unneeded newline.
> +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,
> +};
> +
Same here.
> +module_i2c_driver(aaeon_mcu_driver);
> +
> +MODULE_DESCRIPTION("Aaeon MCU Driver");
> +MODULE_AUTHOR("Jérémie Dautheribes <jeremie.dautheribes@...tlin.com>");
> +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..c9dc7a9adbc86c489f03a550e7776d3b1da4e7b2
> --- /dev/null
> +++ b/include/linux/mfd/aaeon-mcu.h
> @@ -0,0 +1,31 @@
> +/* 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
> +
> +#include <linux/mutex.h>
> +#include <linux/types.h>
> +
> +/**
> + * struct aaeon_mcu_dev - Internal representation of the Aaeon MCU
> + * @dev: Pointer to kernel device structure
> + * @i2c_lock: Mutex to serialize I2C bus access
> + */
> +
Kerneldocs should be attached to the struct they describe. But...
> +struct aaeon_mcu_dev {
> + struct device *dev;
> + struct mutex i2c_lock;
> +};
... you don't seem to need to export this structure. You don't use it at all
in the subsequent GPIO patch.
> +
> +int aaeon_mcu_i2c_xfer(struct device *dev,
> + const u8 *cmd, int cmd_len,
> + u8 *rsp, int rsp_len);
> +
> +#endif /* __LINUX_MFD_AAEON_MCU_H */
>
> --
> 2.52.0
>
>
Bart
Powered by blists - more mailing lists