[PATCH 0/5] Congatec Board Controller drivers

[PATCH 0/5] Congatec Board Controller drivers

[Thomas Richard]

The Congatec Board Controller is a microcontroller embedded on the x86 SoM
of Congatec. It's able to manage lots of features, such as a watchdog, some
GPIOs, I2C busses ...

There is no datasheet or specific documentation for this Board Controller.
The only sources of information are the driver, library and tools provided
by Congatec in their yocto metalayer [1].

The Congatec implementation (available in [1]) doesn't follow the good
practice (a unique driver, and all accesses are done using custom ioctls).

This series implements an mfd driver, a gpio driver, a watchdog driver and
an I2C bus driver, to use the standard API from userspace.

For now, only the conga-SA7 module [2] is supported. For this board, the
Board Controller has:
- Two I2C busses
- 14 GPIOs
- A wathdog (with pretimeout support)

It also has temperature, voltage and fan sensors. They will be supported
later.

For the development, the conga-SEVAL board [3] was used.
With this board you have access to the 14 GPIOs, and the two I2C busses.
On each I2C bus, a 24c16 EEPROM is present by default.

To be able to drive GPIO 4, 5 and 6, a specific BIOS configuration is
needed: HD audio shall be disabled, and they shall be set in GPIO mode.

[1] https://git.congatec.com/x86/meta-congatec-x86/
[2] https://www.congatec.com/fileadmin/user_upload/Documents/Manual/SA70.pdf
[3] https://www.congatec.com/fileadmin/user_upload/Documents/Manual/SEVAL.pdf

Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
Thomas Richard (5):
      mfd: add Congatec Board Controller mfd driver
      gpio: Congatec Board Controller gpio driver
      i2c: Congatec Board Controller i2c bus driver
      watchdog: Congatec Board Controller watchdog timer driver
      MAINTAINERS: Add entry for Congatec Board Controller

 MAINTAINERS                   |   9 +
 drivers/gpio/Kconfig          |  10 +
 drivers/gpio/Makefile         |   1 +
 drivers/gpio/gpio-cgbc.c      | 203 +++++++++++++++++++
 drivers/i2c/busses/Kconfig    |  10 +
 drivers/i2c/busses/Makefile   |   1 +
 drivers/i2c/busses/i2c-cgbc.c | 407 +++++++++++++++++++++++++++++++++++++
 drivers/mfd/Kconfig           |  12 ++
 drivers/mfd/Makefile          |   1 +
 drivers/mfd/cgbc-core.c       | 453 ++++++++++++++++++++++++++++++++++++++++++
 drivers/watchdog/Kconfig      |  10 +
 drivers/watchdog/Makefile     |   1 +
 drivers/watchdog/cgbc_wdt.c   | 217 ++++++++++++++++++++
 include/linux/mfd/cgbc.h      |  44 ++++
 14 files changed, 1379 insertions(+)
---
base-commit: d31d4ea2a5e337e60f6da9a90e41d4061bdcec91
change-id: 20240503-congatec-board-controller-82c6b84cd4ea

Best regards,
-- 
Thomas Richard <thomas.richard@bootlin.com>

[PATCH 1/5] mfd: add Congatec Board Controller mfd driver

[Thomas Richard]

Add core MFD driver for the Board Controller found on some Congatec SMARC
module. This Board Controller provides functions like watchdog, GPIO, and
I2C busses.

This commit add support only for the conga-SA7 module.

Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
 drivers/mfd/Kconfig      |  12 ++
 drivers/mfd/Makefile     |   1 +
 drivers/mfd/cgbc-core.c  | 453 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/cgbc.h |  44 +++++
 4 files changed, 510 insertions(+)

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index bc8be2e593b6..3e0530f30267 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -224,6 +224,18 @@ config MFD_AXP20X_RSB
 	  components like regulators or the PEK (Power Enable Key) under the
 	  corresponding menus.
 
+config MFD_CGBC
+	tristate "Congatec Board Controller"
+	select MFD_CORE
+	depends on X86
+	help
+	  This is the core driver of the Board Controller found on some Congatec
+	  SMARC modules. The Board Controller provides functions like watchdog,
+	  I2C busses, and GPIO controller.
+
+	  To compile this driver as a module, choose M here: the module will be
+	  called cgbc-core.
+
 config MFD_CROS_EC_DEV
 	tristate "ChromeOS Embedded Controller multifunction device"
 	select MFD_CORE
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 02b651cd7535..d5da3fcd691c 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -13,6 +13,7 @@ obj-$(CONFIG_MFD_SM501)		+= sm501.o
 obj-$(CONFIG_ARCH_BCM2835)	+= bcm2835-pm.o
 obj-$(CONFIG_MFD_BCM590XX)	+= bcm590xx.o
 obj-$(CONFIG_MFD_BD9571MWV)	+= bd9571mwv.o
+obj-$(CONFIG_MFD_CGBC)		+= cgbc-core.o
 obj-$(CONFIG_MFD_CROS_EC_DEV)	+= cros_ec_dev.o
 obj-$(CONFIG_MFD_CS42L43)	+= cs42l43.o
 obj-$(CONFIG_MFD_CS42L43_I2C)	+= cs42l43-i2c.o
diff --git a/drivers/mfd/cgbc-core.c b/drivers/mfd/cgbc-core.c
new file mode 100644
index 000000000000..cca9b1170cc9
--- /dev/null
+++ b/drivers/mfd/cgbc-core.c
@@ -0,0 +1,453 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Congatec Board Controller MFD core driver.
+ *
+ * The x86 Congatec modules have an embedded micro controller named Board
+ * Controller.
+ * This Board Controller have a watchdog timer, some GPIOs, and two i2c busses.
+ *
+ * Copyright (C) 2024 Bootlin
+ * Author: Thomas Richard <thomas.richard@bootlin.com>
+ */
+
+#include <linux/dmi.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/cgbc.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/iopoll.h>
+#include <linux/sysfs.h>
+
+#define CGBC_MASK_STATUS        (BIT(6) | BIT(7))
+#define CGBC_MASK_DATA_COUNT	0x1F
+#define CGBC_MASK_ERROR_CODE	0x1F
+
+#define CGBC_STATUS_DATA_READY	0x00
+#define CGBC_STATUS_CMD_READY	BIT(6)
+#define CGBC_STATUS_ERROR	(BIT(6) | BIT(7))
+
+#define CGBC_CMD_GET_FW_REV	0x21
+
+#define CGBC_IO_SESSION_BASE	0x0E20
+#define CGBC_IO_SESSION_END	0x0E30
+#define CGBC_IO_CMD_BASE	0x0E00
+#define CGBC_IO_CMD_END		0x0E10
+
+#define CGBC_SESSION_CMD	0x00
+#define		CGBC_SESSION_CMD_IDLE		0x00
+#define		CGBC_SESSION_CMD_REQUEST	0x01
+#define	CGBC_SESSION_DATA	0x01
+#define CGBC_SESSION_STATUS	0x02
+#define		CGBC_SESSION_STATUS_FREE	0x03
+#define CGBC_SESSION_ACCESS	0x04
+#define		CGBC_SESSION_ACCESS_GAINED	0x00
+
+#define CGBC_SESSION_VALID_MIN  0x02
+#define CGBC_SESSION_VALID_MAX  0xFE
+
+#define CGBC_CMD_STROBE	0x00
+#define CGBC_CMD_INDEX	0x02
+#define		CGBC_CMD_INDEX_CBM_MAN8		0x00
+#define		CGBC_CMD_INDEX_CBM_AUTO32	0x03
+#define CGBC_CMD_DATA	0x04
+#define CGBC_CMD_ACCESS	0x0C
+
+struct cgbc_platform_data {
+	const struct resource	*ioresource;
+	unsigned int		num_ioresource;
+};
+
+static struct platform_device *cgbc_pdev;
+
+static int cgbc_detect_device(struct cgbc_device_data *cgbc)
+{
+	u16 status;
+	int ret;
+
+	ret = readx_poll_timeout(ioread16, cgbc->io_session + CGBC_SESSION_STATUS, status,
+				 status == CGBC_SESSION_STATUS_FREE, 0, 500000);
+
+	if (ret || ioread32(cgbc->io_session + CGBC_SESSION_ACCESS))
+		ret = -ENODEV;
+
+	return ret;
+}
+
+static int cgbc_session_command(struct cgbc_device_data *cgbc, u8 cmd)
+{
+	int ret;
+	u8 val;
+
+	ret = readx_poll_timeout(ioread8, cgbc->io_session + CGBC_SESSION_CMD, val,
+				 val == CGBC_SESSION_CMD_IDLE, 0, 100000);
+	if (ret)
+		return ret;
+
+	iowrite8(cmd, cgbc->io_session + CGBC_SESSION_CMD);
+
+	ret = readx_poll_timeout(ioread8, cgbc->io_session + CGBC_SESSION_CMD, val,
+				 val == CGBC_SESSION_CMD_IDLE, 0, 100000);
+	if (ret)
+		return ret;
+
+	ret = (int)ioread8(cgbc->io_session + CGBC_SESSION_DATA);
+
+	iowrite8(CGBC_SESSION_STATUS_FREE,
+		 cgbc->io_session + CGBC_SESSION_STATUS);
+
+	return ret;
+}
+
+static int cgbc_session_request(struct cgbc_device_data *cgbc)
+{
+	unsigned int ret = cgbc_detect_device(cgbc);
+
+	if (ret)
+		return dev_err_probe(cgbc->dev, ret, "device not found\n");
+
+	cgbc->session = cgbc_session_command(cgbc, CGBC_SESSION_CMD_REQUEST);
+
+	/* the Board Controller sent us a wrong session handle, we cannot
+	 * communicate with it.
+	 */
+	if (cgbc->session < CGBC_SESSION_VALID_MIN ||
+	    cgbc->session > CGBC_SESSION_VALID_MAX) {
+		cgbc->session = 0;
+		return dev_err_probe(cgbc->dev, (cgbc->session < 0) ? cgbc->session : -ECONNREFUSED,
+				     "failed to get a valid session handle\n");
+	}
+
+	return 0;
+}
+
+static void cgbc_session_release(struct cgbc_device_data *cgbc)
+{
+	if (cgbc_session_command(cgbc, cgbc->session) != cgbc->session)
+		dev_err(cgbc->dev, "failed to release session\n");
+}
+
+static bool cgbc_command_lock(struct cgbc_device_data *cgbc)
+{
+	iowrite8(cgbc->session, cgbc->io_cmd + CGBC_CMD_ACCESS);
+
+	return ioread8(cgbc->io_cmd + CGBC_CMD_ACCESS) == cgbc->session;
+}
+
+static void cgbc_command_unlock(struct cgbc_device_data *cgbc)
+{
+	iowrite8(cgbc->session, cgbc->io_cmd + CGBC_CMD_ACCESS);
+}
+
+static int __cgbc_command(struct cgbc_device_data *cgbc, u8 *cmd, u8 cmd_size,
+			  u8 *data, u8 data_size, u8 *status)
+{
+	u8 checksum = 0, data_checksum = 0, istatus = 0, val;
+	int mode_change = -1;
+	bool lock;
+	int ret, i;
+
+	mutex_lock(&cgbc->lock);
+
+	/* request access */
+	ret = readx_poll_timeout(cgbc_command_lock, cgbc, lock, lock, 0, 100000);
+	if (ret)
+		goto out;
+
+	/* wait board controller is ready */
+	ret = readx_poll_timeout(ioread8, cgbc->io_cmd + CGBC_CMD_STROBE, val,
+				 val == CGBC_CMD_STROBE, 0, 100000);
+	if (ret)
+		goto release;
+
+	/* write command packet */
+	if (cmd_size <= 2) {
+		iowrite8(CGBC_CMD_INDEX_CBM_MAN8,
+			 cgbc->io_cmd + CGBC_CMD_INDEX);
+	} else {
+		iowrite8(CGBC_CMD_INDEX_CBM_AUTO32,
+			 cgbc->io_cmd + CGBC_CMD_INDEX);
+		if ((cmd_size % 4) != 0x03)
+			mode_change = (cmd_size & 0xFFFC) - 1;
+	}
+
+	for (i = 0; i < cmd_size; i++) {
+		iowrite8(cmd[i], cgbc->io_cmd + CGBC_CMD_DATA + (i % 4));
+		checksum ^= cmd[i];
+		if (mode_change == i)
+			iowrite8((i + 1) | CGBC_CMD_INDEX_CBM_MAN8,
+				 cgbc->io_cmd + CGBC_CMD_INDEX);
+	}
+
+	/* append checksum byte */
+	iowrite8(checksum, cgbc->io_cmd + CGBC_CMD_DATA + (i % 4));
+
+	/* perform command strobe */
+	iowrite8(cgbc->session, cgbc->io_cmd + CGBC_CMD_STROBE);
+
+	/* rewind cmd buffer index */
+	iowrite8(CGBC_CMD_INDEX_CBM_AUTO32,
+		 cgbc->io_cmd + CGBC_CMD_INDEX);
+
+	/* wait command completion */
+	ret = read_poll_timeout(ioread8, val, val == CGBC_CMD_STROBE, 0,
+				100000, false,
+				cgbc->io_cmd + CGBC_CMD_STROBE);
+	if (ret)
+		goto release;
+
+	istatus = ioread8(cgbc->io_cmd + CGBC_CMD_DATA);
+	checksum = istatus;
+
+	/* check command status */
+	switch (istatus & CGBC_MASK_STATUS) {
+	case CGBC_STATUS_DATA_READY:
+		if (istatus > data_size)
+			istatus = data_size;
+		for (i = 0; i < istatus; i++) {
+			data[i] = ioread8(cgbc->io_cmd +
+					  CGBC_CMD_DATA + ((i + 1) % 4));
+			checksum ^= data[i];
+		}
+		data_checksum = ioread8(cgbc->io_cmd +
+					CGBC_CMD_DATA + ((i + 1) % 4));
+		istatus &= CGBC_MASK_DATA_COUNT;
+		break;
+	case CGBC_STATUS_ERROR:
+	case CGBC_STATUS_CMD_READY:
+		data_checksum = ioread8(cgbc->io_cmd +
+					CGBC_CMD_DATA + 1);
+		if ((istatus & CGBC_MASK_STATUS) == CGBC_STATUS_ERROR)
+			ret = -EIO;
+		istatus = istatus & CGBC_MASK_ERROR_CODE;
+		break;
+	default:
+		data_checksum = ioread8(cgbc->io_cmd + CGBC_CMD_DATA + 1);
+		istatus &= CGBC_MASK_ERROR_CODE;
+		ret = -EIO;
+		break;
+	}
+
+	/* checksum verification */
+	if (ret == 0 && data_checksum != checksum)
+		ret = -EIO;
+
+release:
+	cgbc_command_unlock(cgbc);
+
+out:
+	mutex_unlock(&cgbc->lock);
+
+	if (status)
+		*status = istatus;
+
+	return ret;
+}
+
+int cgbc_command(struct cgbc_device_data *cgbc, void *cmd, unsigned int cmd_size,
+		 void *data, unsigned int data_size, u8 *status)
+{
+	return __cgbc_command(cgbc, (u8 *)cmd, cmd_size, (u8 *)data, data_size, status);
+}
+EXPORT_SYMBOL_GPL(cgbc_command);
+
+static struct mfd_cell cgbc_devs[] = {
+	{ .name = "cgbc-wdt"	},
+	{ .name = "cgbc-gpio"	},
+	{ .name = "cgbc-i2c", .id = 1 },
+	{ .name = "cgbc-i2c", .id = 2 },
+};
+
+static int cgbc_map(struct cgbc_device_data *cgbc)
+{
+	struct device *dev = cgbc->dev;
+	struct platform_device *pdev = to_platform_device(dev);
+	struct resource *ioport;
+
+	ioport = platform_get_resource(pdev, IORESOURCE_IO, 0);
+	if (!ioport)
+		return -EINVAL;
+
+	cgbc->io_session = devm_ioport_map(dev, ioport->start,
+					   resource_size(ioport));
+	if (!cgbc->io_session)
+		return -ENOMEM;
+
+	ioport = platform_get_resource(pdev, IORESOURCE_IO, 1);
+	if (!ioport)
+		return -EINVAL;
+
+	cgbc->io_cmd = devm_ioport_map(dev, ioport->start,
+				       resource_size(ioport));
+	if (!cgbc->io_cmd)
+		return -ENOMEM;
+
+	return 0;
+}
+
+static struct resource cgbc_ioresource[] = {
+	{
+		.start  = CGBC_IO_SESSION_BASE,
+		.end    = CGBC_IO_SESSION_END,
+		.flags  = IORESOURCE_IO,
+	},
+	{
+		.start  = CGBC_IO_CMD_BASE,
+		.end    = CGBC_IO_CMD_END,
+		.flags  = IORESOURCE_IO,
+	},
+};
+
+static const struct cgbc_platform_data cgbc_platform_data = {
+	.ioresource = &cgbc_ioresource[0],
+	.num_ioresource = ARRAY_SIZE(cgbc_ioresource),
+};
+
+static int cgbc_create_platform_device(const struct cgbc_platform_data *pdata)
+{
+	const struct platform_device_info pdevinfo = {
+		.name = "cgbc",
+		.id = PLATFORM_DEVID_NONE,
+		.res = pdata->ioresource,
+		.num_res = pdata->num_ioresource,
+	};
+
+	cgbc_pdev = platform_device_register_full(&pdevinfo);
+	if (IS_ERR(cgbc_pdev))
+		return PTR_ERR(cgbc_pdev);
+
+	return 0;
+}
+
+static ssize_t cgbc_version_show(struct device *dev,
+				 struct device_attribute *attr, char *buf)
+{
+	struct cgbc_device_data *cgbc = dev_get_drvdata(dev);
+
+	return sysfs_emit(buf, "CGBCP%c%c%c\n", cgbc->version.feature,
+			  cgbc->version.major, cgbc->version.minor);
+}
+
+static DEVICE_ATTR_RO(cgbc_version);
+
+static struct attribute *cgbc_attrs[] = {
+	&dev_attr_cgbc_version.attr,
+	NULL
+};
+
+ATTRIBUTE_GROUPS(cgbc);
+
+static int cgbc_get_version(struct cgbc_device_data *cgbc)
+{
+	u8 cmd = CGBC_CMD_GET_FW_REV;
+	u8 data[4];
+	int ret;
+
+	ret = cgbc_command(cgbc, &cmd, 1, &data, sizeof(data), NULL);
+	if (ret)
+		return ret;
+
+	cgbc->version.feature = data[0];
+	cgbc->version.major = data[1];
+	cgbc->version.minor = data[2];
+
+	return 0;
+}
+
+static int cgbc_init_device(struct cgbc_device_data *cgbc)
+{
+	int ret;
+
+	ret = cgbc_session_request(cgbc);
+	if (ret)
+		return ret;
+
+	ret = cgbc_get_version(cgbc);
+	if (ret)
+		return ret;
+
+	return mfd_add_devices(cgbc->dev, -1, cgbc_devs,
+			       ARRAY_SIZE(cgbc_devs), NULL, 0, NULL);
+}
+
+static int cgbc_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct cgbc_device_data *cgbc;
+	int ret;
+
+	cgbc = devm_kzalloc(dev, sizeof(*cgbc), GFP_KERNEL);
+	if (!cgbc)
+		return -ENOMEM;
+
+	cgbc->dev = dev;
+
+	ret = cgbc_map(cgbc);
+	if (ret)
+		return ret;
+
+	mutex_init(&cgbc->lock);
+
+	platform_set_drvdata(pdev, cgbc);
+
+	return cgbc_init_device(cgbc);
+}
+
+static void cgbc_remove(struct platform_device *pdev)
+{
+	struct cgbc_device_data *cgbc = platform_get_drvdata(pdev);
+
+	cgbc_session_release(cgbc);
+
+	mfd_remove_devices(&pdev->dev);
+}
+
+static struct platform_driver cgbc_driver = {
+	.driver		= {
+		.name		= "cgbc",
+		.dev_groups	= cgbc_groups,
+	},
+	.probe		= cgbc_probe,
+	.remove_new	= cgbc_remove,
+};
+
+static const struct dmi_system_id cgbc_dmi_table[] __initconst = {
+	{
+		.ident = "SA7",
+		.matches = {
+			DMI_MATCH(DMI_BOARD_VENDOR, "congatec"),
+			DMI_MATCH(DMI_BOARD_NAME, "conga-SA7"),
+		},
+	},
+	{}
+};
+MODULE_DEVICE_TABLE(dmi, cgbc_dmi_table);
+
+static int __init cgbc_init(void)
+{
+	const struct dmi_system_id *id;
+	int ret = -ENODEV;
+
+	id = dmi_first_match(cgbc_dmi_table);
+	if (IS_ERR_OR_NULL(id))
+		return ret;
+
+	ret = cgbc_create_platform_device(&cgbc_platform_data);
+	if (ret)
+		return ret;
+
+	return platform_driver_register(&cgbc_driver);
+}
+
+static void __exit cgbc_exit(void)
+{
+	platform_device_unregister(cgbc_pdev);
+	platform_driver_unregister(&cgbc_driver);
+}
+
+module_init(cgbc_init);
+module_exit(cgbc_exit);
+
+MODULE_DESCRIPTION("Congatec Board Controller Core Driver");
+MODULE_AUTHOR("Thomas Richard <thomas.richard@bootlin.com>");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:cgbc-core");
diff --git a/include/linux/mfd/cgbc.h b/include/linux/mfd/cgbc.h
new file mode 100644
index 000000000000..badbec4c7033
--- /dev/null
+++ b/include/linux/mfd/cgbc.h
@@ -0,0 +1,44 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Congatec Board Controller driver definitions
+ *
+ * Copyright (C) 2024 Bootlin
+ * Author: Thomas Richard <thomas.richard@bootlin.com>
+ */
+
+#ifndef _LINUX_MFD_CGBC_H_
+
+/**
+ * struct cgbc_version - Board Controller device version structure
+ * @feature:	Board Controller feature number
+ * @major:	Board Controller major revision
+ * @minor:	Board Controller minor revision
+ */
+struct cgbc_version {
+	unsigned char feature;
+	unsigned char major;
+	unsigned char minor;
+};
+
+/**
+ * struct cgbc_device_data - Internal representation of the Board Controller device
+ * @io_session:		Pointer to the session IO memory
+ * @io_cmd:		Pointer to the command IO memory
+ * @session:		Session id returned by the Board Controller
+ * @dev:		Pointer to kernel device structure
+ * @cgbc_version:	Board Controller version structure
+ * @mutex:		Board Controller mutex
+ */
+struct cgbc_device_data {
+	void __iomem		*io_session;
+	void __iomem		*io_cmd;
+	u8			session;
+	struct device		*dev;
+	struct cgbc_version	version;
+	struct mutex		lock;
+};
+
+int cgbc_command(struct cgbc_device_data *cgbc, void *cmd, unsigned int cmd_size,
+		 void *data, unsigned int data_size, u8 *status);
+
+#endif /*_LINUX_MFD_CGBC_H_*/

-- 
2.39.2

[PATCH 2/5] gpio: Congatec Board Controller gpio driver

[Thomas Richard]

Add gpio support for the Congatec Board Controller.

Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
 drivers/gpio/Kconfig     |  10 +++
 drivers/gpio/Makefile    |   1 +
 drivers/gpio/gpio-cgbc.c | 203 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 214 insertions(+)

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 58f43bcced7c..ce77bad40087 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -233,6 +233,16 @@ config GPIO_CADENCE
 	help
 	  Say yes here to enable support for Cadence GPIO controller.
 
+config GPIO_CGBC
+	tristate "Congatec Board Controller GPIO support"
+	depends on MFD_CGBC
+	help
+	  Select this option to enable GPIO support for the Congatec Board
+	  Controller.
+
+	  This driver can also be built as a module. If so, the module will be
+	  called gpio-cgbc.
+
 config GPIO_CLPS711X
 	tristate "CLPS711X GPIO support"
 	depends on ARCH_CLPS711X || COMPILE_TEST
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 64dd6d9d730d..3a96e3c27a2d 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -44,6 +44,7 @@ obj-$(CONFIG_GPIO_BD9571MWV)		+= gpio-bd9571mwv.o
 obj-$(CONFIG_GPIO_BRCMSTB)		+= gpio-brcmstb.o
 obj-$(CONFIG_GPIO_BT8XX)		+= gpio-bt8xx.o
 obj-$(CONFIG_GPIO_CADENCE)		+= gpio-cadence.o
+obj-$(CONFIG_GPIO_CGBC)			+= gpio-cgbc.o
 obj-$(CONFIG_GPIO_CLPS711X)		+= gpio-clps711x.o
 obj-$(CONFIG_GPIO_SNPS_CREG)		+= gpio-creg-snps.o
 obj-$(CONFIG_GPIO_CROS_EC)		+= gpio-cros-ec.o
diff --git a/drivers/gpio/gpio-cgbc.c b/drivers/gpio/gpio-cgbc.c
new file mode 100644
index 000000000000..6da50c794872
--- /dev/null
+++ b/drivers/gpio/gpio-cgbc.c
@@ -0,0 +1,203 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Congatec Board Controller GPIO driver
+ *
+ * Copyright (C) 2024 Bootlin
+ * Author: Thomas Richard <thomas.richard@bootlin.com>
+ */
+
+#include <linux/gpio/driver.h>
+#include <linux/mfd/cgbc.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/platform_device.h>
+
+#define CGBC_GPIO_NGPIO	14
+
+#define CGBC_GPIO_CMD_GET	0x64
+#define CGBC_GPIO_CMD_SET	0x65
+#define CGBC_GPIO_CMD_DIR_GET	0x66
+#define CGBC_GPIO_CMD_DIR_SET	0x67
+
+struct cgbc_gpio_data {
+	struct gpio_chip	chip;
+	struct cgbc_device_data	*cgbc;
+	struct mutex lock;
+};
+
+static int cgbc_gpio_cmd(struct cgbc_device_data *cgbc,
+			 u8 cmd0, u8 cmd1, u8 cmd2, u8 *value)
+{
+	u8 cmd[3] = {cmd0, cmd1, cmd2};
+
+	return cgbc_command(cgbc, cmd, sizeof(cmd), value, 1, NULL);
+}
+
+static int cgbc_gpio_get(struct gpio_chip *chip, unsigned int offset)
+{
+	struct cgbc_gpio_data *gpio = gpiochip_get_data(chip);
+	struct cgbc_device_data *cgbc = gpio->cgbc;
+	int ret;
+	u8 val;
+
+	mutex_lock(&gpio->lock);
+
+	ret = cgbc_gpio_cmd(cgbc, CGBC_GPIO_CMD_GET, (offset > 7) ? 1 : 0, 0, &val);
+
+	mutex_unlock(&gpio->lock);
+
+	offset %= 8;
+
+	if (ret)
+		return ret;
+	else
+		return (int)(val & (u8)BIT(offset));
+}
+
+static void __cgbc_gpio_set(struct gpio_chip *chip,
+			    unsigned int offset, int value)
+{
+	struct cgbc_gpio_data *gpio = gpiochip_get_data(chip);
+	struct cgbc_device_data *cgbc = gpio->cgbc;
+	u8 val;
+	int ret;
+
+	ret = cgbc_gpio_cmd(cgbc, CGBC_GPIO_CMD_GET, (offset > 7) ? 1 : 0, 0, &val);
+	if (ret)
+		return;
+
+	if (value)
+		val |= BIT(offset % 8);
+	else
+		val &= ~((u8)BIT(offset % 8));
+
+	cgbc_gpio_cmd(cgbc, CGBC_GPIO_CMD_SET, (offset > 7) ? 1 : 0, val, &val);
+}
+
+static void cgbc_gpio_set(struct gpio_chip *chip,
+			  unsigned int offset, int value)
+{
+	struct cgbc_gpio_data *gpio = gpiochip_get_data(chip);
+
+	mutex_lock(&gpio->lock);
+	__cgbc_gpio_set(chip, offset, value);
+	mutex_unlock(&gpio->lock);
+}
+
+static int cgbc_gpio_direction_set(struct gpio_chip *chip,
+				   unsigned int offset, int direction)
+{
+	struct cgbc_gpio_data *gpio = gpiochip_get_data(chip);
+	struct cgbc_device_data *cgbc = gpio->cgbc;
+	int ret;
+	u8 val;
+
+	ret = cgbc_gpio_cmd(cgbc, CGBC_GPIO_CMD_DIR_GET, (offset > 7) ? 1 : 0, 0, &val);
+	if (ret)
+		goto end;
+
+	if (direction == GPIO_LINE_DIRECTION_IN)
+		val &= ~((u8)BIT(offset % 8));
+	else
+		val |= BIT(offset % 8);
+
+	ret = cgbc_gpio_cmd(cgbc, CGBC_GPIO_CMD_DIR_SET, (offset > 7) ? 1 : 0, val, &val);
+
+end:
+	return ret;
+}
+
+static int cgbc_gpio_direction_input(struct gpio_chip *chip,
+				     unsigned int offset)
+{
+	struct cgbc_gpio_data *gpio = gpiochip_get_data(chip);
+	int ret;
+
+	mutex_lock(&gpio->lock);
+	ret = cgbc_gpio_direction_set(chip, offset, GPIO_LINE_DIRECTION_IN);
+	mutex_unlock(&gpio->lock);
+
+	return ret;
+}
+
+static int cgbc_gpio_direction_output(struct gpio_chip *chip,
+				      unsigned int offset, int value)
+{
+	struct cgbc_gpio_data *gpio = gpiochip_get_data(chip);
+	int ret;
+
+	mutex_lock(&gpio->lock);
+	__cgbc_gpio_set(chip, offset, value);
+	ret = cgbc_gpio_direction_set(chip, offset, GPIO_LINE_DIRECTION_OUT);
+	mutex_unlock(&gpio->lock);
+
+	return ret;
+}
+
+static int cgbc_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
+{
+	struct cgbc_gpio_data *gpio = gpiochip_get_data(chip);
+	struct cgbc_device_data *cgbc = gpio->cgbc;
+	int ret;
+	u8 val;
+
+	ret = cgbc_gpio_cmd(cgbc, CGBC_GPIO_CMD_DIR_GET, (offset > 7) ? 1 : 0, 0, &val);
+	if (ret)
+		return ret;
+
+	if (val & BIT(offset % 8))
+		return GPIO_LINE_DIRECTION_OUT;
+	else
+		return GPIO_LINE_DIRECTION_IN;
+}
+
+static int cgbc_gpio_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct cgbc_device_data *cgbc = dev_get_drvdata(dev->parent);
+	struct cgbc_gpio_data *gpio;
+	struct gpio_chip *chip;
+	int ret;
+
+	gpio = devm_kzalloc(dev, sizeof(*gpio), GFP_KERNEL);
+	if (!gpio)
+		return -ENOMEM;
+
+	gpio->cgbc = cgbc;
+
+	platform_set_drvdata(pdev, gpio);
+
+	chip = &gpio->chip;
+	chip->label = dev_name(&pdev->dev);
+	chip->owner = THIS_MODULE;
+	chip->parent = dev;
+	chip->base = -1;
+	chip->direction_input = cgbc_gpio_direction_input;
+	chip->direction_output = cgbc_gpio_direction_output;
+	chip->get_direction = cgbc_gpio_get_direction;
+	chip->get = cgbc_gpio_get;
+	chip->set = cgbc_gpio_set;
+	chip->ngpio = CGBC_GPIO_NGPIO;
+
+	mutex_init(&gpio->lock);
+
+	ret = devm_gpiochip_add_data(dev, chip, gpio);
+	if (ret)
+		return dev_err_probe(dev, ret, "Could not register GPIO chip\n");
+
+	return 0;
+}
+
+static struct platform_driver cgbc_gpio_driver = {
+	.driver = {
+		.name = "cgbc-gpio",
+	},
+	.probe	= cgbc_gpio_probe,
+};
+
+module_platform_driver(cgbc_gpio_driver);
+
+MODULE_DESCRIPTION("Congatec Board Controller GPIO Driver");
+MODULE_AUTHOR("Thomas Richard <thomas.richard@bootlin.com>");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:cgbc-gpio");

-- 
2.39.2

[PATCH 3/5] i2c: Congatec Board Controller i2c bus driver

[Thomas Richard]

Add i2c support for the Congatec Board Controller.

Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
 drivers/i2c/busses/Kconfig    |  10 ++
 drivers/i2c/busses/Makefile   |   1 +
 drivers/i2c/busses/i2c-cgbc.c | 407 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 418 insertions(+)

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index a22f9125322a..3657338d0346 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -535,6 +535,16 @@ config I2C_CBUS_GPIO
 	  This driver can also be built as a module.  If so, the module
 	  will be called i2c-cbus-gpio.
 
+config I2C_CGBC
+	tristate "Congatec I2C Controller"
+	depends on MFD_CGBC
+	help
+	  This enables the I2C bus interfaces for the Congatec Board
+	  Controller.
+
+	  This driver can also be built as a module. If so, the module will
+	  be called i2c-cgbc.ko.
+
 config I2C_CPM
 	tristate "Freescale CPM1 or CPM2 (MPC8xx/826x)"
 	depends on CPM1 || CPM2
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 78d0561339e5..3e6bb569c546 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -50,6 +50,7 @@ obj-$(CONFIG_I2C_BCM2835)	+= i2c-bcm2835.o
 obj-$(CONFIG_I2C_BCM_IPROC)	+= i2c-bcm-iproc.o
 obj-$(CONFIG_I2C_CADENCE)	+= i2c-cadence.o
 obj-$(CONFIG_I2C_CBUS_GPIO)	+= i2c-cbus-gpio.o
+obj-$(CONFIG_I2C_CGBC)		+= i2c-cgbc.o
 obj-$(CONFIG_I2C_CPM)		+= i2c-cpm.o
 obj-$(CONFIG_I2C_DAVINCI)	+= i2c-davinci.o
 obj-$(CONFIG_I2C_DESIGNWARE_CORE)			+= i2c-designware-core.o
diff --git a/drivers/i2c/busses/i2c-cgbc.c b/drivers/i2c/busses/i2c-cgbc.c
new file mode 100644
index 000000000000..5fffe07c40e6
--- /dev/null
+++ b/drivers/i2c/busses/i2c-cgbc.c
@@ -0,0 +1,407 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Congatec Board Controller I2C busses driver
+ *
+ * Copyright (C) 2024 Bootlin
+ * Author: Thomas Richard <thomas.richard@bootlin.com>
+ */
+
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/i2c.h>
+#include <linux/iopoll.h>
+
+#include <linux/mfd/cgbc.h>
+
+#define CGBC_I2C_PRIMARY_BUS_ID	0
+#define CGBC_I2C_PM_BUS_ID	4
+
+#define CGBC_I2C_CMD_START	0x40
+#define CGBC_I2C_CMD_STAT	0x48
+#define CGBC_I2C_CMD_DATA	0x50
+#define CGBC_I2C_CMD_SPEED	0x58
+
+#define CGBC_I2C_STAT_IDL	0x00
+#define CGBC_I2C_STAT_DAT	0x01
+#define CGBC_I2C_STAT_BUSY	0x02
+
+#define CGBC_I2C_START	0x80
+#define CGBC_I2C_STOP	0x40
+
+#define CGBC_I2C_LAST_ACK  0x80    /* send ACK on last read byte */
+
+/*
+ * Reference code defines 1kHz as min freq and 6.1MHz as max freq.
+ * But in practice, the board controller limits the frequency to 1MHz, and the
+ * 1kHz is not functional (minimal working freq is 50kHz).
+ * So use these values as limits.
+ */
+#define CGBC_I2C_FREQ_MIN_HZ	50000	/* 50 kHz */
+#define CGBC_I2C_FREQ_MAX_HZ	1000000 /* 1 MHz */
+
+#define CGBC_I2C_FREQ_UNIT_1KHZ		0x40
+#define CGBC_I2C_FREQ_UNIT_10KHZ	0x80
+#define CGBC_I2C_FREQ_UNIT_100KHZ	0xC0
+
+#define CGBC_I2C_FREQ_UNIT_MASK		0xC0
+#define CGBC_I2C_FREQ_VALUE_MASK	0x3F
+
+#define CGBC_I2C_READ_MAX_LEN	31
+#define CGBC_I2C_WRITE_MAX_LEN	32
+
+#define CGBC_I2C_CMD_HEADER_SIZE	4
+#define CGBC_I2C_CMD_SIZE		(CGBC_I2C_CMD_HEADER_SIZE + CGBC_I2C_WRITE_MAX_LEN)
+
+enum i2c_state {
+	STATE_DONE = 0,
+	STATE_INIT,
+	STATE_START,
+	STATE_READ,
+	STATE_WRITE,
+	STATE_ERROR,
+};
+
+struct i2c_algo_cgbc_data {
+	u8		bus_id;
+	unsigned long	read_maxtime_us;
+};
+
+struct cgbc_i2c_data {
+	struct device		*dev;
+	struct cgbc_device_data *cgbc;
+	struct i2c_adapter      adap;
+	struct i2c_msg		*msg;
+	int			nmsgs;
+	int			pos;
+	enum i2c_state		state;
+};
+
+struct cgbc_i2c_transfer {
+	u8 bus_id;
+	bool start;
+	bool stop;
+	bool last_ack;
+	u8 read;
+	u8 write;
+	u8 addr;
+	u8 data[CGBC_I2C_WRITE_MAX_LEN];
+};
+
+static u8 cgbc_i2c_freq_to_reg(unsigned int bus_frequency)
+{
+	u8 reg;
+
+	if (bus_frequency <= 10000)
+		reg = CGBC_I2C_FREQ_UNIT_1KHZ | (bus_frequency / 1000);
+	else if (bus_frequency <= 100000)
+		reg = CGBC_I2C_FREQ_UNIT_10KHZ | (bus_frequency / 10000);
+	else
+		reg = CGBC_I2C_FREQ_UNIT_100KHZ | (bus_frequency / 100000);
+
+	return reg;
+}
+
+static unsigned int cgbc_i2c_reg_to_freq(u8 reg)
+{
+	unsigned int freq = reg & CGBC_I2C_FREQ_VALUE_MASK;
+	u8 unit = reg & CGBC_I2C_FREQ_UNIT_MASK;
+
+	if (unit == CGBC_I2C_FREQ_UNIT_100KHZ)
+		return freq * 100000;
+	else if (unit == CGBC_I2C_FREQ_UNIT_10KHZ)
+		return freq * 10000;
+	else
+		return freq * 1000;
+}
+
+static int cgbc_i2c_get_status(struct i2c_adapter *adap)
+{
+	struct i2c_algo_cgbc_data *algo_data = adap->algo_data;
+	struct cgbc_i2c_data *i2c = i2c_get_adapdata(adap);
+	struct cgbc_device_data *cgbc = i2c->cgbc;
+	u8 cmd = CGBC_I2C_CMD_STAT | algo_data->bus_id;
+	u8 status;
+	int ret;
+
+	ret = cgbc_command(cgbc, &cmd, sizeof(cmd), NULL, 0, &status);
+	if (ret)
+		return ret;
+
+	return status;
+}
+
+static int cgbc_i2c_set_frequency(struct i2c_adapter *adap,
+				  unsigned int bus_frequency)
+{
+	struct i2c_algo_cgbc_data *algo_data = adap->algo_data;
+	struct cgbc_i2c_data *i2c = i2c_get_adapdata(adap);
+	struct cgbc_device_data *cgbc = i2c->cgbc;
+	u8 cmd[2], data;
+	int ret;
+
+	if (bus_frequency > CGBC_I2C_FREQ_MAX_HZ ||
+	    bus_frequency < CGBC_I2C_FREQ_MIN_HZ) {
+		dev_warn(i2c->dev, "invalid frequency %u, using default\n", bus_frequency);
+		bus_frequency = I2C_MAX_STANDARD_MODE_FREQ;
+	}
+
+	cmd[0] = CGBC_I2C_CMD_SPEED | algo_data->bus_id;
+	cmd[1] = cgbc_i2c_freq_to_reg(bus_frequency);
+
+	ret = cgbc_command(cgbc, &cmd, sizeof(cmd), &data, 1, NULL);
+	if (ret)
+		return dev_err_probe(i2c->dev, ret,
+				     "Failed to initialize I2C bus %s",
+				     adap->name);
+
+	cmd[1] = 0x00;
+
+	ret = cgbc_command(cgbc, &cmd, sizeof(cmd), &data, 1, NULL);
+	if (ret)
+		return dev_err_probe(i2c->dev, ret,
+				     "Failed to get I2C bus frequency");
+
+	bus_frequency = cgbc_i2c_reg_to_freq(data);
+
+	dev_dbg(i2c->dev, "%s is running at %d Hz\n", adap->name, bus_frequency);
+
+	/*
+	 * The read_maxtime_us is the maximum time to wait during a read to get
+	 * data. At maximum CGBC_I2C_READ_MAX_LEN can be read by command.
+	 * So calculate the max time to size correctly the timeout.
+	 */
+	algo_data->read_maxtime_us = (BITS_PER_BYTE + 1) * CGBC_I2C_READ_MAX_LEN
+		* USEC_PER_SEC / bus_frequency;
+
+	return 0;
+}
+
+static unsigned int cgbc_i2c_xfer_to_cmd(struct cgbc_i2c_transfer xfer, u8 *cmd)
+{
+	int i = 0;
+
+	cmd[i++] = CGBC_I2C_CMD_START | xfer.bus_id;
+
+	cmd[i] = (xfer.start) ? CGBC_I2C_START : 0x00;
+	if (xfer.stop)
+		cmd[i] |= CGBC_I2C_STOP;
+	cmd[i++] |= (xfer.start) ? xfer.write + 1 : xfer.write;
+
+	cmd[i++] = (xfer.last_ack) ? (xfer.read | CGBC_I2C_LAST_ACK) : xfer.read;
+
+	if (xfer.start)
+		cmd[i++] = xfer.addr;
+
+	if (xfer.write > 0)
+		memcpy(&cmd[i], &xfer.data, xfer.write);
+
+	return i + xfer.write;
+}
+
+static int cgbc_i2c_xfer_msg(struct i2c_adapter *adap)
+{
+	struct i2c_algo_cgbc_data *algo_data = adap->algo_data;
+	struct cgbc_i2c_data *i2c = i2c_get_adapdata(adap);
+	struct cgbc_device_data *cgbc = i2c->cgbc;
+	struct i2c_msg *msg = i2c->msg;
+	u8 cmd[CGBC_I2C_CMD_SIZE];
+	int ret, max_len, len, i;
+	unsigned int cmd_len;
+	u8 cmd_data;
+
+	struct cgbc_i2c_transfer xfer = {
+		.bus_id = algo_data->bus_id,
+		.addr = i2c_8bit_addr_from_msg(msg),
+	};
+
+	if (i2c->state == STATE_DONE)
+		return 0;
+
+	ret = cgbc_i2c_get_status(adap);
+
+	if (ret == CGBC_I2C_STAT_BUSY)
+		return -EBUSY;
+	else if (ret < 0)
+		goto err;
+
+	if (i2c->state == STATE_INIT ||
+	    (i2c->state == STATE_WRITE && msg->flags & I2C_M_RD))
+		xfer.start = true;
+
+	i2c->state = (msg->flags & I2C_M_RD) ? STATE_READ : STATE_WRITE;
+
+	max_len = (i2c->state == STATE_READ) ?
+		CGBC_I2C_READ_MAX_LEN : CGBC_I2C_WRITE_MAX_LEN;
+
+	if (msg->len - i2c->pos > max_len) {
+		len = max_len;
+	} else {
+		len = msg->len - i2c->pos;
+
+		if (i2c->nmsgs == 1)
+			xfer.stop = true;
+	}
+
+	if (i2c->state == STATE_WRITE) {
+		xfer.write = len;
+		xfer.read = 0;
+
+		for (i = 0; i < len; i++)
+			xfer.data[i] = msg->buf[i2c->pos + i];
+
+		cmd_len = cgbc_i2c_xfer_to_cmd(xfer, &cmd[0]);
+
+		ret = cgbc_command(cgbc, &cmd, cmd_len, NULL, 0, NULL);
+		if (ret)
+			goto err;
+	} else if (i2c->state == STATE_READ) {
+		xfer.write = 0;
+		xfer.read = len;
+
+		if (i2c->nmsgs > 1 || msg->len - i2c->pos > max_len)
+			xfer.read |= CGBC_I2C_LAST_ACK;
+
+		cmd_len = cgbc_i2c_xfer_to_cmd(xfer, &cmd[0]);
+		ret = cgbc_command(cgbc, &cmd, cmd_len, NULL, 0, NULL);
+		if (ret)
+			goto err;
+
+		ret = read_poll_timeout(cgbc_i2c_get_status, ret,
+					ret != CGBC_I2C_STAT_BUSY, 0,
+					2 * algo_data->read_maxtime_us, false, adap);
+		if (ret < 0)
+			goto err;
+
+		cmd_data = CGBC_I2C_CMD_DATA | algo_data->bus_id;
+		ret = cgbc_command(cgbc, &cmd_data, sizeof(cmd_data),
+				   msg->buf + i2c->pos, len, NULL);
+		if (ret)
+			goto err;
+	}
+
+	if (len == (msg->len - i2c->pos)) {
+		i2c->msg++;
+		i2c->nmsgs--;
+		i2c->pos = 0;
+	} else {
+		i2c->pos += len;
+	}
+
+	if (i2c->nmsgs == 0)
+		i2c->state = STATE_DONE;
+
+	return 0;
+
+err:
+	i2c->state = STATE_ERROR;
+	return ret;
+}
+
+static int cgbc_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
+			 int num)
+{
+	struct cgbc_i2c_data *i2c = i2c_get_adapdata(adap);
+	unsigned long timeout = jiffies + HZ;
+	int ret;
+
+	i2c->state = STATE_INIT;
+	i2c->msg = msgs;
+	i2c->nmsgs = num;
+	i2c->pos = 0;
+
+	while (time_before(jiffies, timeout)) {
+		ret = cgbc_i2c_xfer_msg(adap);
+
+		if (i2c->state == STATE_DONE)
+			return num;
+
+		if (i2c->state == STATE_ERROR)
+			return ret;
+
+		if (ret == 0)
+			timeout = jiffies + HZ;
+	}
+
+	i2c->state = STATE_ERROR;
+	return -ETIMEDOUT;
+}
+
+static u32 cgbc_i2c_func(struct i2c_adapter *adap)
+{
+	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
+}
+
+static const struct i2c_algorithm cgbc_i2c_algorithm = {
+	.master_xfer	= cgbc_i2c_xfer,
+	.functionality	= cgbc_i2c_func,
+};
+
+static struct i2c_algo_cgbc_data cgbc_i2c_algo_data[] = {
+	{ .bus_id = CGBC_I2C_PRIMARY_BUS_ID },
+	{ .bus_id = CGBC_I2C_PM_BUS_ID },
+};
+
+static const struct i2c_adapter cgbc_i2c_adapter[] = {
+	{
+		.owner		= THIS_MODULE,
+		.name		= "Congatec Primary I2C adapter",
+		.class		= I2C_CLASS_DEPRECATED,
+		.algo		= &cgbc_i2c_algorithm,
+		.algo_data	= &cgbc_i2c_algo_data[0],
+		.nr		= -1,
+	},
+	{
+		.owner		= THIS_MODULE,
+		.name		= "Congatec Power Management I2C adapter",
+		.class		= I2C_CLASS_DEPRECATED,
+		.algo		= &cgbc_i2c_algorithm,
+		.algo_data	= &cgbc_i2c_algo_data[1],
+		.nr		= -1,
+	},
+};
+
+static int cgbc_i2c_probe(struct platform_device *pdev)
+{
+	struct cgbc_device_data *cgbc = dev_get_drvdata(pdev->dev.parent);
+	struct cgbc_i2c_data *i2c;
+	int ret;
+
+	i2c = devm_kzalloc(&pdev->dev, sizeof(*i2c), GFP_KERNEL);
+	if (!i2c)
+		return -ENOMEM;
+
+	i2c->cgbc = cgbc;
+	i2c->dev = &pdev->dev;
+	i2c->adap = cgbc_i2c_adapter[pdev->id];
+	i2c->adap.dev.parent = i2c->dev;
+	i2c_set_adapdata(&i2c->adap, i2c);
+	platform_set_drvdata(pdev, i2c);
+
+	ret = cgbc_i2c_set_frequency(&i2c->adap, I2C_MAX_STANDARD_MODE_FREQ);
+	if (ret)
+		return ret;
+
+	return i2c_add_numbered_adapter(&i2c->adap);
+}
+
+static void cgbc_i2c_remove(struct platform_device *pdev)
+{
+	struct cgbc_i2c_data *i2c = platform_get_drvdata(pdev);
+
+	i2c_del_adapter(&i2c->adap);
+}
+
+static struct platform_driver cgbc_i2c_driver = {
+	.driver = {
+		.name = "cgbc-i2c",
+	},
+	.probe		= cgbc_i2c_probe,
+	.remove_new	= cgbc_i2c_remove,
+};
+
+module_platform_driver(cgbc_i2c_driver);
+
+MODULE_DESCRIPTION("Congatec Board Controller I2C Driver");
+MODULE_AUTHOR("Thomas Richard <thomas.richard@bootlin.com>");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:cgbc_i2c");

-- 
2.39.2

[PATCH 4/5] watchdog: Congatec Board Controller watchdog timer driver

[Thomas Richard]

Add watchdog timer support for the Congatec Board Controller.

Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
 drivers/watchdog/Kconfig    |  10 ++
 drivers/watchdog/Makefile   |   1 +
 drivers/watchdog/cgbc_wdt.c | 217 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 228 insertions(+)

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index bae1d97cce89..07b711fc8bb2 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1142,6 +1142,16 @@ config ALIM7101_WDT
 
 	  Most people will say N.
 
+config CGBC_WDT
+	tristate "Congatec Board Controller Watchdog Timer"
+	depends on MFD_CGBC
+	select WATCHDOG_CORE
+	help
+	  Enables watchdog timer support for the Congatec Board Controller.
+
+	  This driver can also be built as a module. If so, the module will be
+	  called cgbc_wdt.
+
 config EBC_C384_WDT
 	tristate "WinSystems EBC-C384 Watchdog Timer"
 	depends on (X86 || COMPILE_TEST) && HAS_IOPORT
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index b51030f035a6..5aa66ba91346 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -106,6 +106,7 @@ obj-$(CONFIG_ADVANTECH_WDT) += advantechwdt.o
 obj-$(CONFIG_ADVANTECH_EC_WDT) += advantech_ec_wdt.o
 obj-$(CONFIG_ALIM1535_WDT) += alim1535_wdt.o
 obj-$(CONFIG_ALIM7101_WDT) += alim7101_wdt.o
+obj-$(CONFIG_CGBC_WDT) += cgbc_wdt.o
 obj-$(CONFIG_EBC_C384_WDT) += ebc-c384_wdt.o
 obj-$(CONFIG_EXAR_WDT) += exar_wdt.o
 obj-$(CONFIG_F71808E_WDT) += f71808e_wdt.o
diff --git a/drivers/watchdog/cgbc_wdt.c b/drivers/watchdog/cgbc_wdt.c
new file mode 100644
index 000000000000..9327e87b52e8
--- /dev/null
+++ b/drivers/watchdog/cgbc_wdt.c
@@ -0,0 +1,217 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Congatec Board Controller watchdog driver
+ *
+ * Copyright (C) 2024 Bootlin
+ * Author: Thomas Richard <thomas.richard@bootlin.com>
+ */
+
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/watchdog.h>
+
+#include <linux/mfd/cgbc.h>
+
+#define CGBC_WDT_CMD_TRIGGER	0x27
+#define CGBC_WDT_CMD_INIT	0x28
+#define CGBC_WDT_DISABLE	0x00
+
+#define CGBC_WDT_MODE_SINGLE_EVENT 0x02
+
+#define DEFAULT_TIMEOUT_SEC	30
+#define DEFAULT_PRETIMEOUT_SEC	0
+
+enum action {
+	ACTION_INT = 0,
+	ACTION_SMI,
+	ACTION_RESET,
+	ACTION_BUTTON,
+};
+
+static unsigned int timeout = DEFAULT_TIMEOUT_SEC;
+module_param(timeout, uint, 0);
+MODULE_PARM_DESC(timeout,
+		 "Watchdog timeout in seconds. (>=0, default="
+		 __MODULE_STRING(DEFAULT_TIMEOUT_SEC) ")");
+
+static unsigned int pretimeout = DEFAULT_PRETIMEOUT_SEC;
+module_param(pretimeout, uint, 0);
+MODULE_PARM_DESC(pretimeout,
+		 "Watchdog pretimeout in seconds. (>=0, default="
+		 __MODULE_STRING(DEFAULT_PRETIMEOUT_SEC) ")");
+
+static bool nowayout = WATCHDOG_NOWAYOUT;
+module_param(nowayout, bool, 0);
+MODULE_PARM_DESC(nowayout,
+		 "Watchdog cannot be stopped once started (default="
+		 __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
+
+struct cgbc_wdt_data {
+	struct cgbc_device_data	*cgbc;
+	struct watchdog_device	wdd;
+	enum action timeout_action;
+	enum action pretimeout_action;
+};
+
+struct cgbc_wdt_cmd_cfg {
+	u8 cmd;
+	u8 mode;
+	u8 action;
+	u8 timeout1[3];
+	u8 timeout2[3];
+	u8 reserved[3];
+	u8 delay[3];
+} __packed;
+
+static_assert(sizeof(struct cgbc_wdt_cmd_cfg) == 15);
+
+static int cgbc_wdt_start(struct watchdog_device *wdd)
+{
+	struct cgbc_wdt_data *wdt_data = watchdog_get_drvdata(wdd);
+	struct cgbc_device_data *cgbc = wdt_data->cgbc;
+	unsigned int timeout1 = (wdd->timeout - wdd->pretimeout) * 1000;
+	unsigned int timeout2 = wdd->pretimeout * 1000;
+	u8 action;
+
+	struct cgbc_wdt_cmd_cfg cmd_start = {
+		.cmd = CGBC_WDT_CMD_INIT,
+		.mode = CGBC_WDT_MODE_SINGLE_EVENT,
+		.timeout1[0] = (u8)timeout1,
+		.timeout1[1] = (u8)(timeout1 >> 8),
+		.timeout1[2] = (u8)(timeout1 >> 16),
+		.timeout2[0] = (u8)timeout2,
+		.timeout2[1] = (u8)(timeout2 >> 8),
+		.timeout2[2] = (u8)(timeout2 >> 16),
+	};
+
+	if (wdd->pretimeout) {
+		action = 2;
+		action |= wdt_data->pretimeout_action << 2;
+		action |= wdt_data->timeout_action << 4;
+	} else {
+		action = 1;
+		action |= wdt_data->timeout_action << 2;
+	}
+
+	cmd_start.action = action;
+
+	return cgbc_command(cgbc, &cmd_start, sizeof(cmd_start), NULL, 0, NULL);
+}
+
+static int cgbc_wdt_stop(struct watchdog_device *wdd)
+{
+	struct cgbc_wdt_data *wdt_data = watchdog_get_drvdata(wdd);
+	struct cgbc_device_data *cgbc = wdt_data->cgbc;
+	struct cgbc_wdt_cmd_cfg cmd_stop = {
+		.cmd = CGBC_WDT_CMD_INIT,
+		.mode = CGBC_WDT_DISABLE,
+	};
+
+	return cgbc_command(cgbc, &cmd_stop, sizeof(cmd_stop), NULL, 0, NULL);
+}
+
+static int cgbc_wdt_keepalive(struct watchdog_device *wdd)
+{
+	struct cgbc_wdt_data *wdt_data = watchdog_get_drvdata(wdd);
+	struct cgbc_device_data *cgbc = wdt_data->cgbc;
+	u8 cmd_ping = CGBC_WDT_CMD_TRIGGER;
+
+	return cgbc_command(cgbc, &cmd_ping, sizeof(cmd_ping), NULL, 0, NULL);
+}
+
+static int cgbc_wdt_set_pretimeout(struct watchdog_device *wdd,
+				   unsigned int pretimeout)
+{
+	struct cgbc_wdt_data *wdt_data = watchdog_get_drvdata(wdd);
+
+	wdd->pretimeout = pretimeout;
+	wdt_data->pretimeout_action = ACTION_SMI;
+
+	if (watchdog_active(wdd))
+		return cgbc_wdt_start(wdd);
+
+	return 0;
+}
+
+static int cgbc_wdt_set_timeout(struct watchdog_device *wdd,
+				unsigned int timeout)
+{
+	struct cgbc_wdt_data *wdt_data = watchdog_get_drvdata(wdd);
+
+	if (timeout < wdd->pretimeout) {
+		dev_warn(wdd->parent, "timeout <= pretimeout. Setting pretimeout to zero\n");
+		wdd->pretimeout = 0;
+	}
+
+	wdd->timeout = timeout;
+	wdt_data->timeout_action = ACTION_RESET;
+
+	if (watchdog_active(wdd))
+		return cgbc_wdt_start(wdd);
+
+	return 0;
+}
+
+static const struct watchdog_info cgbc_wdt_info = {
+	.identity	= "CGBC Watchdog",
+	.options	= WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING |
+		WDIOF_MAGICCLOSE | WDIOF_PRETIMEOUT
+};
+
+static const struct watchdog_ops cgbc_wdt_ops = {
+	.owner		= THIS_MODULE,
+	.start		= cgbc_wdt_start,
+	.stop		= cgbc_wdt_stop,
+	.ping		= cgbc_wdt_keepalive,
+	.set_timeout	= cgbc_wdt_set_timeout,
+	.set_pretimeout = cgbc_wdt_set_pretimeout,
+};
+
+static int cgbc_wdt_probe(struct platform_device *pdev)
+{
+	struct cgbc_device_data *cgbc = dev_get_drvdata(pdev->dev.parent);
+	struct device *dev = &pdev->dev;
+	struct cgbc_wdt_data *wdt_data;
+	struct watchdog_device *wdd;
+	int ret;
+
+	wdt_data = devm_kzalloc(dev, sizeof(*wdt_data), GFP_KERNEL);
+	if (!wdt_data)
+		return -ENOMEM;
+
+	wdt_data->cgbc = cgbc;
+	wdd = &wdt_data->wdd;
+	wdd->parent = dev;
+
+	wdd->info = &cgbc_wdt_info;
+	wdd->ops = &cgbc_wdt_ops;
+
+	watchdog_set_drvdata(wdd, wdt_data);
+	watchdog_set_nowayout(wdd, nowayout);
+
+	cgbc_wdt_set_timeout(wdd, timeout);
+	cgbc_wdt_set_pretimeout(wdd, pretimeout);
+
+	platform_set_drvdata(pdev, wdt_data);
+	watchdog_stop_on_reboot(wdd);
+	watchdog_stop_on_unregister(wdd);
+
+	ret = devm_watchdog_register_device(dev, wdd);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static struct platform_driver cgbc_wdt_driver = {
+	.driver		= {
+		.name	= "cgbc-wdt",
+	},
+	.probe		= cgbc_wdt_probe,
+};
+
+module_platform_driver(cgbc_wdt_driver);
+
+MODULE_DESCRIPTION("Congatec Board Controller Watchdog Driver");
+MODULE_AUTHOR("Thomas Richard <thomas.richard@bootlin.com>");
+MODULE_LICENSE("GPL");

-- 
2.39.2

[PATCH 5/5] MAINTAINERS: Add entry for Congatec Board Controller

[Thomas Richard]

Add the Congatec Board Controller drivers and header as Maintained by
myself.

Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
 MAINTAINERS | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 8766f3e5e87e..781803bb5d0b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5648,6 +5648,15 @@ F:	fs/configfs/
 F:	include/linux/configfs.h
 F:	samples/configfs/
 
+CONGATEC BOARD CONTROLLER MFD DRIVER
+M:	Thomas Richard <thomas.richard@bootlin.com>
+S:	Maintained
+F:	drivers/gpio/gpio-cgbc.c
+F:	drivers/i2c/busses/i2c-cgbc.c
+F:	drivers/mfd/cgbc-core.c
+F:	drivers/watchdog/cgbc_wdt.c
+F:	include/linux/mfd/cgbc.h
+
 CONSOLE SUBSYSTEM
 M:	Greg Kroah-Hartman <gregkh@linuxfoundation.org>
 S:	Supported

-- 
2.39.2

Re: [PATCH 4/5] watchdog: Congatec Board Controller watchdog timer driver

[Guenter Roeck]

On Fri, Aug 09, 2024 at 04:52:08PM +0200, Thomas Richard wrote:
> Add watchdog timer support for the Congatec Board Controller.
> 
> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
> ---
>  drivers/watchdog/Kconfig    |  10 ++
>  drivers/watchdog/Makefile   |   1 +
>  drivers/watchdog/cgbc_wdt.c | 217 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 228 insertions(+)
> 
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index bae1d97cce89..07b711fc8bb2 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -1142,6 +1142,16 @@ config ALIM7101_WDT
>  
>  	  Most people will say N.
>  
> +config CGBC_WDT
> +	tristate "Congatec Board Controller Watchdog Timer"
> +	depends on MFD_CGBC
> +	select WATCHDOG_CORE
> +	help
> +	  Enables watchdog timer support for the Congatec Board Controller.
> +
> +	  This driver can also be built as a module. If so, the module will be
> +	  called cgbc_wdt.
> +
>  config EBC_C384_WDT
>  	tristate "WinSystems EBC-C384 Watchdog Timer"
>  	depends on (X86 || COMPILE_TEST) && HAS_IOPORT
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index b51030f035a6..5aa66ba91346 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -106,6 +106,7 @@ obj-$(CONFIG_ADVANTECH_WDT) += advantechwdt.o
>  obj-$(CONFIG_ADVANTECH_EC_WDT) += advantech_ec_wdt.o
>  obj-$(CONFIG_ALIM1535_WDT) += alim1535_wdt.o
>  obj-$(CONFIG_ALIM7101_WDT) += alim7101_wdt.o
> +obj-$(CONFIG_CGBC_WDT) += cgbc_wdt.o
>  obj-$(CONFIG_EBC_C384_WDT) += ebc-c384_wdt.o
>  obj-$(CONFIG_EXAR_WDT) += exar_wdt.o
>  obj-$(CONFIG_F71808E_WDT) += f71808e_wdt.o
> diff --git a/drivers/watchdog/cgbc_wdt.c b/drivers/watchdog/cgbc_wdt.c
> new file mode 100644
> index 000000000000..9327e87b52e8
> --- /dev/null
> +++ b/drivers/watchdog/cgbc_wdt.c
> @@ -0,0 +1,217 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Congatec Board Controller watchdog driver
> + *
> + * Copyright (C) 2024 Bootlin
> + * Author: Thomas Richard <thomas.richard@bootlin.com>
> + */
> +
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/watchdog.h>
> +
> +#include <linux/mfd/cgbc.h>
> +
> +#define CGBC_WDT_CMD_TRIGGER	0x27
> +#define CGBC_WDT_CMD_INIT	0x28
> +#define CGBC_WDT_DISABLE	0x00
> +
> +#define CGBC_WDT_MODE_SINGLE_EVENT 0x02
> +
> +#define DEFAULT_TIMEOUT_SEC	30
> +#define DEFAULT_PRETIMEOUT_SEC	0
> +
> +enum action {
> +	ACTION_INT = 0,
> +	ACTION_SMI,
> +	ACTION_RESET,
> +	ACTION_BUTTON,
> +};
> +
> +static unsigned int timeout = DEFAULT_TIMEOUT_SEC;
> +module_param(timeout, uint, 0);
> +MODULE_PARM_DESC(timeout,
> +		 "Watchdog timeout in seconds. (>=0, default="
> +		 __MODULE_STRING(DEFAULT_TIMEOUT_SEC) ")");
> +
> +static unsigned int pretimeout = DEFAULT_PRETIMEOUT_SEC;
> +module_param(pretimeout, uint, 0);
> +MODULE_PARM_DESC(pretimeout,
> +		 "Watchdog pretimeout in seconds. (>=0, default="
> +		 __MODULE_STRING(DEFAULT_PRETIMEOUT_SEC) ")");
> +
> +static bool nowayout = WATCHDOG_NOWAYOUT;
> +module_param(nowayout, bool, 0);
> +MODULE_PARM_DESC(nowayout,
> +		 "Watchdog cannot be stopped once started (default="
> +		 __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
> +
> +struct cgbc_wdt_data {
> +	struct cgbc_device_data	*cgbc;
> +	struct watchdog_device	wdd;
> +	enum action timeout_action;
> +	enum action pretimeout_action;
> +};
> +
> +struct cgbc_wdt_cmd_cfg {
> +	u8 cmd;
> +	u8 mode;
> +	u8 action;
> +	u8 timeout1[3];
> +	u8 timeout2[3];
> +	u8 reserved[3];
> +	u8 delay[3];
> +} __packed;
> +
> +static_assert(sizeof(struct cgbc_wdt_cmd_cfg) == 15);

static_assert() is declared in linux/build_bug.h. Please include all
necessary include files explicitly and do not depend on indirect includes.

> +
> +static int cgbc_wdt_start(struct watchdog_device *wdd)
> +{
> +	struct cgbc_wdt_data *wdt_data = watchdog_get_drvdata(wdd);

Unusual way to get wdt_data instead of using container_of().
Any special reason ?

> +	struct cgbc_device_data *cgbc = wdt_data->cgbc;
> +	unsigned int timeout1 = (wdd->timeout - wdd->pretimeout) * 1000;
> +	unsigned int timeout2 = wdd->pretimeout * 1000;
> +	u8 action;
> +
> +	struct cgbc_wdt_cmd_cfg cmd_start = {
> +		.cmd = CGBC_WDT_CMD_INIT,
> +		.mode = CGBC_WDT_MODE_SINGLE_EVENT,
> +		.timeout1[0] = (u8)timeout1,
> +		.timeout1[1] = (u8)(timeout1 >> 8),
> +		.timeout1[2] = (u8)(timeout1 >> 16),
> +		.timeout2[0] = (u8)timeout2,
> +		.timeout2[1] = (u8)(timeout2 >> 8),
> +		.timeout2[2] = (u8)(timeout2 >> 16),
> +	};
> +
> +	if (wdd->pretimeout) {
> +		action = 2;
> +		action |= wdt_data->pretimeout_action << 2;
> +		action |= wdt_data->timeout_action << 4;
> +	} else {
> +		action = 1;
> +		action |= wdt_data->timeout_action << 2;
> +	}
> +
> +	cmd_start.action = action;
> +
> +	return cgbc_command(cgbc, &cmd_start, sizeof(cmd_start), NULL, 0, NULL);
> +}
> +
> +static int cgbc_wdt_stop(struct watchdog_device *wdd)
> +{
> +	struct cgbc_wdt_data *wdt_data = watchdog_get_drvdata(wdd);
> +	struct cgbc_device_data *cgbc = wdt_data->cgbc;
> +	struct cgbc_wdt_cmd_cfg cmd_stop = {
> +		.cmd = CGBC_WDT_CMD_INIT,
> +		.mode = CGBC_WDT_DISABLE,
> +	};
> +
> +	return cgbc_command(cgbc, &cmd_stop, sizeof(cmd_stop), NULL, 0, NULL);
> +}
> +
> +static int cgbc_wdt_keepalive(struct watchdog_device *wdd)
> +{
> +	struct cgbc_wdt_data *wdt_data = watchdog_get_drvdata(wdd);
> +	struct cgbc_device_data *cgbc = wdt_data->cgbc;
> +	u8 cmd_ping = CGBC_WDT_CMD_TRIGGER;
> +
> +	return cgbc_command(cgbc, &cmd_ping, sizeof(cmd_ping), NULL, 0, NULL);
> +}
> +
> +static int cgbc_wdt_set_pretimeout(struct watchdog_device *wdd,
> +				   unsigned int pretimeout)
> +{
> +	struct cgbc_wdt_data *wdt_data = watchdog_get_drvdata(wdd);
> +
> +	wdd->pretimeout = pretimeout;
> +	wdt_data->pretimeout_action = ACTION_SMI;
> +
> +	if (watchdog_active(wdd))
> +		return cgbc_wdt_start(wdd);
> +
> +	return 0;
> +}
> +
> +static int cgbc_wdt_set_timeout(struct watchdog_device *wdd,
> +				unsigned int timeout)
> +{
> +	struct cgbc_wdt_data *wdt_data = watchdog_get_drvdata(wdd);
> +
> +	if (timeout < wdd->pretimeout) {
> +		dev_warn(wdd->parent, "timeout <= pretimeout. Setting pretimeout to zero\n");

That is a normal condition which does not warrant a log message.
Also see drivers/watchdog/watchdog_dev.c around line 385.

> +		wdd->pretimeout = 0;
> +	}
> +
> +	wdd->timeout = timeout;
> +	wdt_data->timeout_action = ACTION_RESET;

Both timeout_action and pretimeout_action are set statically.
What is the point of doing that instead of just using
ACTION_RESET and ACTION_SMI as needed irectly ?

> +
> +	if (watchdog_active(wdd))
> +		return cgbc_wdt_start(wdd);
> +
> +	return 0;
> +}
> +
> +static const struct watchdog_info cgbc_wdt_info = {
> +	.identity	= "CGBC Watchdog",
> +	.options	= WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING |
> +		WDIOF_MAGICCLOSE | WDIOF_PRETIMEOUT
> +};
> +
> +static const struct watchdog_ops cgbc_wdt_ops = {
> +	.owner		= THIS_MODULE,
> +	.start		= cgbc_wdt_start,
> +	.stop		= cgbc_wdt_stop,
> +	.ping		= cgbc_wdt_keepalive,
> +	.set_timeout	= cgbc_wdt_set_timeout,
> +	.set_pretimeout = cgbc_wdt_set_pretimeout,
> +};
> +
> +static int cgbc_wdt_probe(struct platform_device *pdev)
> +{
> +	struct cgbc_device_data *cgbc = dev_get_drvdata(pdev->dev.parent);
> +	struct device *dev = &pdev->dev;
> +	struct cgbc_wdt_data *wdt_data;
> +	struct watchdog_device *wdd;
> +	int ret;
> +
> +	wdt_data = devm_kzalloc(dev, sizeof(*wdt_data), GFP_KERNEL);

devm_kzalloc() is declared in linux/device.h. Again, please include all
necessary include files explicitly.

> +	if (!wdt_data)
> +		return -ENOMEM;
> +
> +	wdt_data->cgbc = cgbc;
> +	wdd = &wdt_data->wdd;
> +	wdd->parent = dev;
> +

No limits ? That is unusual. Are you sure the driver accepts all
timeouts from 0 to UINT_MAX ?

> +	wdd->info = &cgbc_wdt_info;
> +	wdd->ops = &cgbc_wdt_ops;
> +
> +	watchdog_set_drvdata(wdd, wdt_data);
> +	watchdog_set_nowayout(wdd, nowayout);
> +
> +	cgbc_wdt_set_timeout(wdd, timeout);
> +	cgbc_wdt_set_pretimeout(wdd, pretimeout);

The more common approach would be to set default limits in wdd->{timout,pretimeout}
and only override the values if needed, ie if provided using module parameters.
That implies initializing the module parameters with 0. YOur call, though.

> +
> +	platform_set_drvdata(pdev, wdt_data);
> +	watchdog_stop_on_reboot(wdd);
> +	watchdog_stop_on_unregister(wdd);
> +
> +	ret = devm_watchdog_register_device(dev, wdd);
> +	if (ret)
> +		return ret;
> +
> +	return 0;

Why not just
	return devm_watchdog_register_device(dev, wdd);
?

> +}
> +
> +static struct platform_driver cgbc_wdt_driver = {
> +	.driver		= {
> +		.name	= "cgbc-wdt",
> +	},
> +	.probe		= cgbc_wdt_probe,
> +};
> +
> +module_platform_driver(cgbc_wdt_driver);
> +
> +MODULE_DESCRIPTION("Congatec Board Controller Watchdog Driver");
> +MODULE_AUTHOR("Thomas Richard <thomas.richard@bootlin.com>");
> +MODULE_LICENSE("GPL");
> 
> -- 
> 2.39.2
> 
> 

Re: [PATCH 3/5] i2c: Congatec Board Controller i2c bus driver

[Andi Shyti]

Hi Thomas,

On Fri, Aug 09, 2024 at 04:52:07PM GMT, Thomas Richard wrote:
> Add i2c support for the Congatec Board Controller.

do you mind adding some more description?

> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>

...

> +config I2C_CGBC
> +	tristate "Congatec I2C Controller"
> +	depends on MFD_CGBC
> +	help
> +	  This enables the I2C bus interfaces for the Congatec Board

This what? :-)

> +	  Controller.
> +
> +	  This driver can also be built as a module. If so, the module will
> +	  be called i2c-cgbc.ko.
> +

...

> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/i2c.h>
> +#include <linux/iopoll.h>

please sort includes in alphabetical order?

> +#include <linux/mfd/cgbc.h>

...

> +enum i2c_state {
> +	STATE_DONE = 0,
> +	STATE_INIT,
> +	STATE_START,
> +	STATE_READ,
> +	STATE_WRITE,
> +	STATE_ERROR,
> +};

can you please use the cgbc prefix for this enum and all the
members?

...

> +	if (bus_frequency > CGBC_I2C_FREQ_MAX_HZ ||
> +	    bus_frequency < CGBC_I2C_FREQ_MIN_HZ) {
> +		dev_warn(i2c->dev, "invalid frequency %u, using default\n", bus_frequency);

should this rather be a dev_info()? (supernit: please start with
capital leter).

> +		bus_frequency = I2C_MAX_STANDARD_MODE_FREQ;
> +	}
> +
> +	cmd[0] = CGBC_I2C_CMD_SPEED | algo_data->bus_id;
> +	cmd[1] = cgbc_i2c_freq_to_reg(bus_frequency);
> +
> +	ret = cgbc_command(cgbc, &cmd, sizeof(cmd), &data, 1, NULL);
> +	if (ret)
> +		return dev_err_probe(i2c->dev, ret,
> +				     "Failed to initialize I2C bus %s",
> +				     adap->name);
> +
> +	cmd[1] = 0x00;
> +
> +	ret = cgbc_command(cgbc, &cmd, sizeof(cmd), &data, 1, NULL);
> +	if (ret)
> +		return dev_err_probe(i2c->dev, ret,
> +				     "Failed to get I2C bus frequency");
> +
> +	bus_frequency = cgbc_i2c_reg_to_freq(data);
> +
> +	dev_dbg(i2c->dev, "%s is running at %d Hz\n", adap->name, bus_frequency);
> +
> +	/*
> +	 * The read_maxtime_us is the maximum time to wait during a read to get
> +	 * data. At maximum CGBC_I2C_READ_MAX_LEN can be read by command.
> +	 * So calculate the max time to size correctly the timeout.
> +	 */

this comment is a bit wild, can we rephrase to something like:

/*
 * The read_maxtime_us variable represents the maximum time to wait
 * for data during a read operation. The maximum amount of data that
 * can be read by a command is CGBC_I2C_READ_MAX_LEN.
 * Therefore, calculate the max time to properly size the timeout.
 */

(it's a suggestion, please choose the words you prefer).

Andi

Re: [PATCH 5/5] MAINTAINERS: Add entry for Congatec Board Controller

[Andi Shyti]

Hi Thomas,

On Fri, Aug 09, 2024 at 04:52:09PM GMT, Thomas Richard wrote:
> Add the Congatec Board Controller drivers and header as Maintained by
> myself.
> 
> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>

Acked-by: Andi Shyti <andi.shyti@kernel.org>

Thanks,
Andi

Re: [PATCH 2/5] gpio: Congatec Board Controller gpio driver

[Bartosz Golaszewski]

On Fri, Aug 9, 2024 at 4:52 PM Thomas Richard
<thomas.richard@bootlin.com> wrote:
>
> Add gpio support for the Congatec Board Controller.
>
> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
> ---
>  drivers/gpio/Kconfig     |  10 +++
>  drivers/gpio/Makefile    |   1 +
>  drivers/gpio/gpio-cgbc.c | 203 +++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 214 insertions(+)
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 58f43bcced7c..ce77bad40087 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -233,6 +233,16 @@ config GPIO_CADENCE
>         help
>           Say yes here to enable support for Cadence GPIO controller.
>
> +config GPIO_CGBC
> +       tristate "Congatec Board Controller GPIO support"
> +       depends on MFD_CGBC
> +       help
> +         Select this option to enable GPIO support for the Congatec Board
> +         Controller.
> +
> +         This driver can also be built as a module. If so, the module will be
> +         called gpio-cgbc.
> +
>  config GPIO_CLPS711X
>         tristate "CLPS711X GPIO support"
>         depends on ARCH_CLPS711X || COMPILE_TEST
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index 64dd6d9d730d..3a96e3c27a2d 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -44,6 +44,7 @@ obj-$(CONFIG_GPIO_BD9571MWV)          += gpio-bd9571mwv.o
>  obj-$(CONFIG_GPIO_BRCMSTB)             += gpio-brcmstb.o
>  obj-$(CONFIG_GPIO_BT8XX)               += gpio-bt8xx.o
>  obj-$(CONFIG_GPIO_CADENCE)             += gpio-cadence.o
> +obj-$(CONFIG_GPIO_CGBC)                        += gpio-cgbc.o
>  obj-$(CONFIG_GPIO_CLPS711X)            += gpio-clps711x.o
>  obj-$(CONFIG_GPIO_SNPS_CREG)           += gpio-creg-snps.o
>  obj-$(CONFIG_GPIO_CROS_EC)             += gpio-cros-ec.o
> diff --git a/drivers/gpio/gpio-cgbc.c b/drivers/gpio/gpio-cgbc.c
> new file mode 100644
> index 000000000000..6da50c794872
> --- /dev/null
> +++ b/drivers/gpio/gpio-cgbc.c
> @@ -0,0 +1,203 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Congatec Board Controller GPIO driver
> + *
> + * Copyright (C) 2024 Bootlin
> + * Author: Thomas Richard <thomas.richard@bootlin.com>
> + */
> +
> +#include <linux/gpio/driver.h>
> +#include <linux/mfd/cgbc.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/platform_device.h>
> +
> +#define CGBC_GPIO_NGPIO        14
> +
> +#define CGBC_GPIO_CMD_GET      0x64
> +#define CGBC_GPIO_CMD_SET      0x65
> +#define CGBC_GPIO_CMD_DIR_GET  0x66
> +#define CGBC_GPIO_CMD_DIR_SET  0x67
> +
> +struct cgbc_gpio_data {
> +       struct gpio_chip        chip;
> +       struct cgbc_device_data *cgbc;
> +       struct mutex lock;
> +};
> +
> +static int cgbc_gpio_cmd(struct cgbc_device_data *cgbc,
> +                        u8 cmd0, u8 cmd1, u8 cmd2, u8 *value)
> +{
> +       u8 cmd[3] = {cmd0, cmd1, cmd2};
> +
> +       return cgbc_command(cgbc, cmd, sizeof(cmd), value, 1, NULL);
> +}
> +
> +static int cgbc_gpio_get(struct gpio_chip *chip, unsigned int offset)
> +{
> +       struct cgbc_gpio_data *gpio = gpiochip_get_data(chip);
> +       struct cgbc_device_data *cgbc = gpio->cgbc;
> +       int ret;
> +       u8 val;
> +

Can you use scoped_guard() here and elsewhere?

> +       mutex_lock(&gpio->lock);
> +
> +       ret = cgbc_gpio_cmd(cgbc, CGBC_GPIO_CMD_GET, (offset > 7) ? 1 : 0, 0, &val);
> +
> +       mutex_unlock(&gpio->lock);
> +
> +       offset %= 8;
> +
> +       if (ret)
> +               return ret;
> +       else
> +               return (int)(val & (u8)BIT(offset));
> +}
> +
> +static void __cgbc_gpio_set(struct gpio_chip *chip,
> +                           unsigned int offset, int value)
> +{
> +       struct cgbc_gpio_data *gpio = gpiochip_get_data(chip);
> +       struct cgbc_device_data *cgbc = gpio->cgbc;
> +       u8 val;
> +       int ret;
> +
> +       ret = cgbc_gpio_cmd(cgbc, CGBC_GPIO_CMD_GET, (offset > 7) ? 1 : 0, 0, &val);
> +       if (ret)
> +               return;
> +
> +       if (value)
> +               val |= BIT(offset % 8);
> +       else
> +               val &= ~((u8)BIT(offset % 8));
> +
> +       cgbc_gpio_cmd(cgbc, CGBC_GPIO_CMD_SET, (offset > 7) ? 1 : 0, val, &val);
> +}
> +
> +static void cgbc_gpio_set(struct gpio_chip *chip,
> +                         unsigned int offset, int value)
> +{
> +       struct cgbc_gpio_data *gpio = gpiochip_get_data(chip);
> +
> +       mutex_lock(&gpio->lock);
> +       __cgbc_gpio_set(chip, offset, value);
> +       mutex_unlock(&gpio->lock);
> +}
> +
> +static int cgbc_gpio_direction_set(struct gpio_chip *chip,
> +                                  unsigned int offset, int direction)
> +{
> +       struct cgbc_gpio_data *gpio = gpiochip_get_data(chip);
> +       struct cgbc_device_data *cgbc = gpio->cgbc;
> +       int ret;
> +       u8 val;
> +
> +       ret = cgbc_gpio_cmd(cgbc, CGBC_GPIO_CMD_DIR_GET, (offset > 7) ? 1 : 0, 0, &val);
> +       if (ret)
> +               goto end;
> +
> +       if (direction == GPIO_LINE_DIRECTION_IN)
> +               val &= ~((u8)BIT(offset % 8));
> +       else
> +               val |= BIT(offset % 8);
> +
> +       ret = cgbc_gpio_cmd(cgbc, CGBC_GPIO_CMD_DIR_SET, (offset > 7) ? 1 : 0, val, &val);
> +
> +end:
> +       return ret;
> +}
> +
> +static int cgbc_gpio_direction_input(struct gpio_chip *chip,
> +                                    unsigned int offset)
> +{
> +       struct cgbc_gpio_data *gpio = gpiochip_get_data(chip);
> +       int ret;
> +
> +       mutex_lock(&gpio->lock);
> +       ret = cgbc_gpio_direction_set(chip, offset, GPIO_LINE_DIRECTION_IN);
> +       mutex_unlock(&gpio->lock);
> +
> +       return ret;
> +}
> +
> +static int cgbc_gpio_direction_output(struct gpio_chip *chip,
> +                                     unsigned int offset, int value)
> +{
> +       struct cgbc_gpio_data *gpio = gpiochip_get_data(chip);
> +       int ret;
> +
> +       mutex_lock(&gpio->lock);
> +       __cgbc_gpio_set(chip, offset, value);
> +       ret = cgbc_gpio_direction_set(chip, offset, GPIO_LINE_DIRECTION_OUT);
> +       mutex_unlock(&gpio->lock);
> +
> +       return ret;
> +}
> +
> +static int cgbc_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
> +{
> +       struct cgbc_gpio_data *gpio = gpiochip_get_data(chip);
> +       struct cgbc_device_data *cgbc = gpio->cgbc;
> +       int ret;
> +       u8 val;
> +
> +       ret = cgbc_gpio_cmd(cgbc, CGBC_GPIO_CMD_DIR_GET, (offset > 7) ? 1 : 0, 0, &val);
> +       if (ret)
> +               return ret;
> +
> +       if (val & BIT(offset % 8))
> +               return GPIO_LINE_DIRECTION_OUT;
> +       else
> +               return GPIO_LINE_DIRECTION_IN;
> +}
> +
> +static int cgbc_gpio_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct cgbc_device_data *cgbc = dev_get_drvdata(dev->parent);
> +       struct cgbc_gpio_data *gpio;
> +       struct gpio_chip *chip;
> +       int ret;
> +
> +       gpio = devm_kzalloc(dev, sizeof(*gpio), GFP_KERNEL);
> +       if (!gpio)
> +               return -ENOMEM;
> +
> +       gpio->cgbc = cgbc;
> +
> +       platform_set_drvdata(pdev, gpio);
> +
> +       chip = &gpio->chip;
> +       chip->label = dev_name(&pdev->dev);
> +       chip->owner = THIS_MODULE;
> +       chip->parent = dev;
> +       chip->base = -1;
> +       chip->direction_input = cgbc_gpio_direction_input;
> +       chip->direction_output = cgbc_gpio_direction_output;
> +       chip->get_direction = cgbc_gpio_get_direction;
> +       chip->get = cgbc_gpio_get;
> +       chip->set = cgbc_gpio_set;
> +       chip->ngpio = CGBC_GPIO_NGPIO;
> +
> +       mutex_init(&gpio->lock);

Please use devm_mutex_init() so that it gets cleaned up at exit. It's
not strictly necessary but helps with lock debugging.

> +
> +       ret = devm_gpiochip_add_data(dev, chip, gpio);
> +       if (ret)
> +               return dev_err_probe(dev, ret, "Could not register GPIO chip\n");
> +
> +       return 0;
> +}
> +
> +static struct platform_driver cgbc_gpio_driver = {
> +       .driver = {
> +               .name = "cgbc-gpio",
> +       },
> +       .probe  = cgbc_gpio_probe,
> +};
> +
> +module_platform_driver(cgbc_gpio_driver);
> +
> +MODULE_DESCRIPTION("Congatec Board Controller GPIO Driver");
> +MODULE_AUTHOR("Thomas Richard <thomas.richard@bootlin.com>");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:cgbc-gpio");
>
> --
> 2.39.2
>

Bart

Re: [PATCH 2/5] gpio: Congatec Board Controller gpio driver

[Thomas Richard]

>> +static int cgbc_gpio_get(struct gpio_chip *chip, unsigned int offset)
>> +{
>> +       struct cgbc_gpio_data *gpio = gpiochip_get_data(chip);
>> +       struct cgbc_device_data *cgbc = gpio->cgbc;
>> +       int ret;
>> +       u8 val;
>> +
> 
> Can you use scoped_guard() here and elsewhere?

Hi Bartosz,

Thanks for the review.

For the next iteration I added scoped_guard() in cgbc_gpio_get(), and
guard() in cgbc_gpio_set(), cgbc_gpio_direction_input(), and
cgbc_gpio_direction_output().

> 
>> +       mutex_lock(&gpio->lock);
>> +
>> +       ret = cgbc_gpio_cmd(cgbc, CGBC_GPIO_CMD_GET, (offset > 7) ? 1 : 0, 0, &val);
>> +
>> +       mutex_unlock(&gpio->lock);
>> +
>> +       offset %= 8;
>> +
>> +       if (ret)
>> +               return ret;

...

>> +static int cgbc_gpio_probe(struct platform_device *pdev)
>> +{
>> +       struct device *dev = &pdev->dev;
>> +       struct cgbc_device_data *cgbc = dev_get_drvdata(dev->parent);
>> +       struct cgbc_gpio_data *gpio;
>> +       struct gpio_chip *chip;
>> +       int ret;
>> +
>> +       gpio = devm_kzalloc(dev, sizeof(*gpio), GFP_KERNEL);
>> +       if (!gpio)
>> +               return -ENOMEM;
>> +
>> +       gpio->cgbc = cgbc;
>> +
>> +       platform_set_drvdata(pdev, gpio);
>> +
>> +       chip = &gpio->chip;
>> +       chip->label = dev_name(&pdev->dev);
>> +       chip->owner = THIS_MODULE;
>> +       chip->parent = dev;
>> +       chip->base = -1;
>> +       chip->direction_input = cgbc_gpio_direction_input;
>> +       chip->direction_output = cgbc_gpio_direction_output;
>> +       chip->get_direction = cgbc_gpio_get_direction;
>> +       chip->get = cgbc_gpio_get;
>> +       chip->set = cgbc_gpio_set;
>> +       chip->ngpio = CGBC_GPIO_NGPIO;
>> +
>> +       mutex_init(&gpio->lock);
> 
> Please use devm_mutex_init() so that it gets cleaned up at exit. It's
> not strictly necessary but helps with lock debugging.

Fixed in the next iteration.

Regards,

Thomas

-- 
Thomas Richard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 1/5] mfd: add Congatec Board Controller mfd driver

[Lee Jones]

On Fri, 09 Aug 2024, Thomas Richard wrote:

> Add core MFD driver for the Board Controller found on some Congatec SMARC
> module. This Board Controller provides functions like watchdog, GPIO, and
> I2C busses.
> 
> This commit add support only for the conga-SA7 module.
> 
> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
> ---
>  drivers/mfd/Kconfig      |  12 ++
>  drivers/mfd/Makefile     |   1 +
>  drivers/mfd/cgbc-core.c  | 453 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/cgbc.h |  44 +++++
>  4 files changed, 510 insertions(+)
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index bc8be2e593b6..3e0530f30267 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -224,6 +224,18 @@ config MFD_AXP20X_RSB
>  	  components like regulators or the PEK (Power Enable Key) under the
>  	  corresponding menus.
>  
> +config MFD_CGBC
> +	tristate "Congatec Board Controller"
> +	select MFD_CORE
> +	depends on X86
> +	help
> +	  This is the core driver of the Board Controller found on some Congatec
> +	  SMARC modules. The Board Controller provides functions like watchdog,
> +	  I2C busses, and GPIO controller.
> +
> +	  To compile this driver as a module, choose M here: the module will be
> +	  called cgbc-core.
> +
>  config MFD_CROS_EC_DEV
>  	tristate "ChromeOS Embedded Controller multifunction device"
>  	select MFD_CORE
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 02b651cd7535..d5da3fcd691c 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -13,6 +13,7 @@ obj-$(CONFIG_MFD_SM501)		+= sm501.o
>  obj-$(CONFIG_ARCH_BCM2835)	+= bcm2835-pm.o
>  obj-$(CONFIG_MFD_BCM590XX)	+= bcm590xx.o
>  obj-$(CONFIG_MFD_BD9571MWV)	+= bd9571mwv.o
> +obj-$(CONFIG_MFD_CGBC)		+= cgbc-core.o
>  obj-$(CONFIG_MFD_CROS_EC_DEV)	+= cros_ec_dev.o
>  obj-$(CONFIG_MFD_CS42L43)	+= cs42l43.o
>  obj-$(CONFIG_MFD_CS42L43_I2C)	+= cs42l43-i2c.o
> diff --git a/drivers/mfd/cgbc-core.c b/drivers/mfd/cgbc-core.c
> new file mode 100644
> index 000000000000..cca9b1170cc9
> --- /dev/null
> +++ b/drivers/mfd/cgbc-core.c
> @@ -0,0 +1,453 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Congatec Board Controller MFD core driver.

No such thing as an MFD.

> + * The x86 Congatec modules have an embedded micro controller named Board
> + * Controller.

I think the '\n' can be dropped without issue.

> + * This Board Controller have a watchdog timer, some GPIOs, and two i2c busses.

"has a"

Consider "Watchdog"

"I2C"

> + *
> + * Copyright (C) 2024 Bootlin

Nit: '\n' here.

> + * Author: Thomas Richard <thomas.richard@bootlin.com>
> + */
> +
> +#include <linux/dmi.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/cgbc.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/iopoll.h>
> +#include <linux/sysfs.h>

Alphabetical.

> +#define CGBC_MASK_STATUS        (BIT(6) | BIT(7))
> +#define CGBC_MASK_DATA_COUNT	0x1F
> +#define CGBC_MASK_ERROR_CODE	0x1F

Not sure if it's just my mailer, but if all of these are not already
aligned, please can you tab them out so that they are.

> +#define CGBC_STATUS_DATA_READY	0x00
> +#define CGBC_STATUS_CMD_READY	BIT(6)
> +#define CGBC_STATUS_ERROR	(BIT(6) | BIT(7))
> +
> +#define CGBC_CMD_GET_FW_REV	0x21
> +
> +#define CGBC_IO_SESSION_BASE	0x0E20
> +#define CGBC_IO_SESSION_END	0x0E30
> +#define CGBC_IO_CMD_BASE	0x0E00
> +#define CGBC_IO_CMD_END		0x0E10
> +
> +#define CGBC_SESSION_CMD	0x00
> +#define		CGBC_SESSION_CMD_IDLE		0x00
> +#define		CGBC_SESSION_CMD_REQUEST	0x01
> +#define	CGBC_SESSION_DATA	0x01
> +#define CGBC_SESSION_STATUS	0x02
> +#define		CGBC_SESSION_STATUS_FREE	0x03
> +#define CGBC_SESSION_ACCESS	0x04
> +#define		CGBC_SESSION_ACCESS_GAINED	0x00
> +
> +#define CGBC_SESSION_VALID_MIN  0x02
> +#define CGBC_SESSION_VALID_MAX  0xFE
> +
> +#define CGBC_CMD_STROBE	0x00
> +#define CGBC_CMD_INDEX	0x02
> +#define		CGBC_CMD_INDEX_CBM_MAN8		0x00
> +#define		CGBC_CMD_INDEX_CBM_AUTO32	0x03
> +#define CGBC_CMD_DATA	0x04
> +#define CGBC_CMD_ACCESS	0x0C
> +
> +struct cgbc_platform_data {
> +	const struct resource	*ioresource;
> +	unsigned int		num_ioresource;
> +};

This looks way over engineered.

Why not provide platform_device_info statically in the first place?

> +static struct platform_device *cgbc_pdev;

No avoidable globals.

This stuff should get stored in driver data (ddata)

> +static int cgbc_detect_device(struct cgbc_device_data *cgbc)
> +{
> +	u16 status;
> +	int ret;
> +
> +	ret = readx_poll_timeout(ioread16, cgbc->io_session + CGBC_SESSION_STATUS, status,
> +				 status == CGBC_SESSION_STATUS_FREE, 0, 500000);
> +
> +	if (ret || ioread32(cgbc->io_session + CGBC_SESSION_ACCESS))
> +		ret = -ENODEV;
> +
> +	return ret;
> +}

This function could do with some commentary.

> +static int cgbc_session_command(struct cgbc_device_data *cgbc, u8 cmd)
> +{
> +	int ret;
> +	u8 val;
> +
> +	ret = readx_poll_timeout(ioread8, cgbc->io_session + CGBC_SESSION_CMD, val,
> +				 val == CGBC_SESSION_CMD_IDLE, 0, 100000);
> +	if (ret)
> +		return ret;
> +
> +	iowrite8(cmd, cgbc->io_session + CGBC_SESSION_CMD);
> +
> +	ret = readx_poll_timeout(ioread8, cgbc->io_session + CGBC_SESSION_CMD, val,
> +				 val == CGBC_SESSION_CMD_IDLE, 0, 100000);
> +	if (ret)
> +		return ret;
> +
> +	ret = (int)ioread8(cgbc->io_session + CGBC_SESSION_DATA);
> +
> +	iowrite8(CGBC_SESSION_STATUS_FREE,
> +		 cgbc->io_session + CGBC_SESSION_STATUS);

Which char are you using to line-feed?  This looks inconsistent.

> +
> +	return ret;
> +}
> +
> +static int cgbc_session_request(struct cgbc_device_data *cgbc)
> +{
> +	unsigned int ret = cgbc_detect_device(cgbc);

What does cgbc_detect_device() do?

You're not getting the device?  Just seeing if it's present?

Please separate the declaration from the assignment when calling
functions.

> +	if (ret)
> +		return dev_err_probe(cgbc->dev, ret, "device not found\n");
> +
> +	cgbc->session = cgbc_session_command(cgbc, CGBC_SESSION_CMD_REQUEST);
> +
> +	/* the Board Controller sent us a wrong session handle, we cannot

This is a non-standard multi-line comment.

> +	 * communicate with it.
> +	 */
> +	if (cgbc->session < CGBC_SESSION_VALID_MIN ||
> +	    cgbc->session > CGBC_SESSION_VALID_MAX) {

How are you justifying this line-break and then not line-breaking 2
lines down?

> +		cgbc->session = 0;

Why are we setting variables before bombing out?

> +		return dev_err_probe(cgbc->dev, (cgbc->session < 0) ? cgbc->session : -ECONNREFUSED,

How can (cgbc->session < 0) ever be true?

> +				     "failed to get a valid session handle\n");
> +	}
> +
> +	return 0;
> +}
> +
> +static void cgbc_session_release(struct cgbc_device_data *cgbc)
> +{
> +	if (cgbc_session_command(cgbc, cgbc->session) != cgbc->session)

What does this do?

Are we checking or doing something?  If the latter, then we shouldn't be
doing that in an if() statement.  If the former, then what's the point
of the function?

> +		dev_err(cgbc->dev, "failed to release session\n");

Why print if there isn't anything you can do about it?

> +}
> +
> +static bool cgbc_command_lock(struct cgbc_device_data *cgbc)
> +{
> +	iowrite8(cgbc->session, cgbc->io_cmd + CGBC_CMD_ACCESS);
> +
> +	return ioread8(cgbc->io_cmd + CGBC_CMD_ACCESS) == cgbc->session;
> +}
> +
> +static void cgbc_command_unlock(struct cgbc_device_data *cgbc)
> +{
> +	iowrite8(cgbc->session, cgbc->io_cmd + CGBC_CMD_ACCESS);
> +}
> +
> +static int __cgbc_command(struct cgbc_device_data *cgbc, u8 *cmd, u8 cmd_size,
> +			  u8 *data, u8 data_size, u8 *status)
> +{
> +	u8 checksum = 0, data_checksum = 0, istatus = 0, val;
> +	int mode_change = -1;
> +	bool lock;
> +	int ret, i;
> +
> +	mutex_lock(&cgbc->lock);
> +
> +	/* request access */

Start comments with uppercase chars please.

> +	ret = readx_poll_timeout(cgbc_command_lock, cgbc, lock, lock, 0, 100000);
> +	if (ret)
> +		goto out;
> +
> +	/* wait board controller is ready */
> +	ret = readx_poll_timeout(ioread8, cgbc->io_cmd + CGBC_CMD_STROBE, val,
> +				 val == CGBC_CMD_STROBE, 0, 100000);
> +	if (ret)
> +		goto release;
> +
> +	/* write command packet */
> +	if (cmd_size <= 2) {
> +		iowrite8(CGBC_CMD_INDEX_CBM_MAN8,
> +			 cgbc->io_cmd + CGBC_CMD_INDEX);
> +	} else {
> +		iowrite8(CGBC_CMD_INDEX_CBM_AUTO32,
> +			 cgbc->io_cmd + CGBC_CMD_INDEX);
> +		if ((cmd_size % 4) != 0x03)
> +			mode_change = (cmd_size & 0xFFFC) - 1;
> +	}
> +
> +	for (i = 0; i < cmd_size; i++) {
> +		iowrite8(cmd[i], cgbc->io_cmd + CGBC_CMD_DATA + (i % 4));
> +		checksum ^= cmd[i];
> +		if (mode_change == i)
> +			iowrite8((i + 1) | CGBC_CMD_INDEX_CBM_MAN8,
> +				 cgbc->io_cmd + CGBC_CMD_INDEX);
> +	}
> +
> +	/* append checksum byte */
> +	iowrite8(checksum, cgbc->io_cmd + CGBC_CMD_DATA + (i % 4));
> +
> +	/* perform command strobe */
> +	iowrite8(cgbc->session, cgbc->io_cmd + CGBC_CMD_STROBE);
> +
> +	/* rewind cmd buffer index */
> +	iowrite8(CGBC_CMD_INDEX_CBM_AUTO32,
> +		 cgbc->io_cmd + CGBC_CMD_INDEX);
> +
> +	/* wait command completion */
> +	ret = read_poll_timeout(ioread8, val, val == CGBC_CMD_STROBE, 0,
> +				100000, false,
> +				cgbc->io_cmd + CGBC_CMD_STROBE);
> +	if (ret)
> +		goto release;
> +
> +	istatus = ioread8(cgbc->io_cmd + CGBC_CMD_DATA);
> +	checksum = istatus;
> +
> +	/* check command status */
> +	switch (istatus & CGBC_MASK_STATUS) {
> +	case CGBC_STATUS_DATA_READY:
> +		if (istatus > data_size)
> +			istatus = data_size;
> +		for (i = 0; i < istatus; i++) {
> +			data[i] = ioread8(cgbc->io_cmd +
> +					  CGBC_CMD_DATA + ((i + 1) % 4));
> +			checksum ^= data[i];
> +		}
> +		data_checksum = ioread8(cgbc->io_cmd +
> +					CGBC_CMD_DATA + ((i + 1) % 4));
> +		istatus &= CGBC_MASK_DATA_COUNT;
> +		break;
> +	case CGBC_STATUS_ERROR:
> +	case CGBC_STATUS_CMD_READY:
> +		data_checksum = ioread8(cgbc->io_cmd +
> +					CGBC_CMD_DATA + 1);
> +		if ((istatus & CGBC_MASK_STATUS) == CGBC_STATUS_ERROR)
> +			ret = -EIO;
> +		istatus = istatus & CGBC_MASK_ERROR_CODE;
> +		break;
> +	default:
> +		data_checksum = ioread8(cgbc->io_cmd + CGBC_CMD_DATA + 1);
> +		istatus &= CGBC_MASK_ERROR_CODE;
> +		ret = -EIO;
> +		break;
> +	}
> +
> +	/* checksum verification */
> +	if (ret == 0 && data_checksum != checksum)
> +		ret = -EIO;
> +
> +release:
> +	cgbc_command_unlock(cgbc);
> +
> +out:
> +	mutex_unlock(&cgbc->lock);
> +
> +	if (status)
> +		*status = istatus;
> +
> +	return ret;
> +}
> +
> +int cgbc_command(struct cgbc_device_data *cgbc, void *cmd, unsigned int cmd_size,
> +		 void *data, unsigned int data_size, u8 *status)

What's the point of this abstracted function?

> +{
> +	return __cgbc_command(cgbc, (u8 *)cmd, cmd_size, (u8 *)data, data_size, status);
> +}
> +EXPORT_SYMBOL_GPL(cgbc_command);
> +
> +static struct mfd_cell cgbc_devs[] = {
> +	{ .name = "cgbc-wdt"	},
> +	{ .name = "cgbc-gpio"	},
> +	{ .name = "cgbc-i2c", .id = 1 },
> +	{ .name = "cgbc-i2c", .id = 2 },
> +};
> +
> +static int cgbc_map(struct cgbc_device_data *cgbc)
> +{
> +	struct device *dev = cgbc->dev;
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct resource *ioport;
> +
> +	ioport = platform_get_resource(pdev, IORESOURCE_IO, 0);
> +	if (!ioport)
> +		return -EINVAL;
> +
> +	cgbc->io_session = devm_ioport_map(dev, ioport->start,
> +					   resource_size(ioport));
> +	if (!cgbc->io_session)
> +		return -ENOMEM;
> +
> +	ioport = platform_get_resource(pdev, IORESOURCE_IO, 1);
> +	if (!ioport)
> +		return -EINVAL;
> +
> +	cgbc->io_cmd = devm_ioport_map(dev, ioport->start,
> +				       resource_size(ioport));
> +	if (!cgbc->io_cmd)
> +		return -ENOMEM;
> +
> +	return 0;
> +}
> +
> +static struct resource cgbc_ioresource[] = {
> +	{
> +		.start  = CGBC_IO_SESSION_BASE,
> +		.end    = CGBC_IO_SESSION_END,
> +		.flags  = IORESOURCE_IO,
> +	},
> +	{
> +		.start  = CGBC_IO_CMD_BASE,
> +		.end    = CGBC_IO_CMD_END,
> +		.flags  = IORESOURCE_IO,
> +	},
> +};
> +
> +static const struct cgbc_platform_data cgbc_platform_data = {
> +	.ioresource = &cgbc_ioresource[0],
> +	.num_ioresource = ARRAY_SIZE(cgbc_ioresource),
> +};
> +
> +static int cgbc_create_platform_device(const struct cgbc_platform_data *pdata)
> +{
> +	const struct platform_device_info pdevinfo = {
> +		.name = "cgbc",
> +		.id = PLATFORM_DEVID_NONE,
> +		.res = pdata->ioresource,
> +		.num_res = pdata->num_ioresource,
> +	};
> +
> +	cgbc_pdev = platform_device_register_full(&pdevinfo);
> +	if (IS_ERR(cgbc_pdev))
> +		return PTR_ERR(cgbc_pdev);
> +
> +	return 0;
> +}
> +
> +static ssize_t cgbc_version_show(struct device *dev,
> +				 struct device_attribute *attr, char *buf)
> +{
> +	struct cgbc_device_data *cgbc = dev_get_drvdata(dev);
> +
> +	return sysfs_emit(buf, "CGBCP%c%c%c\n", cgbc->version.feature,
> +			  cgbc->version.major, cgbc->version.minor);
> +}
> +
> +static DEVICE_ATTR_RO(cgbc_version);
> +
> +static struct attribute *cgbc_attrs[] = {
> +	&dev_attr_cgbc_version.attr,
> +	NULL
> +};
> +
> +ATTRIBUTE_GROUPS(cgbc);
> +
> +static int cgbc_get_version(struct cgbc_device_data *cgbc)
> +{
> +	u8 cmd = CGBC_CMD_GET_FW_REV;
> +	u8 data[4];
> +	int ret;
> +
> +	ret = cgbc_command(cgbc, &cmd, 1, &data, sizeof(data), NULL);
> +	if (ret)
> +		return ret;
> +
> +	cgbc->version.feature = data[0];
> +	cgbc->version.major = data[1];
> +	cgbc->version.minor = data[2];
> +
> +	return 0;
> +}
> +
> +static int cgbc_init_device(struct cgbc_device_data *cgbc)
> +{
> +	int ret;
> +
> +	ret = cgbc_session_request(cgbc);
> +	if (ret)
> +		return ret;
> +
> +	ret = cgbc_get_version(cgbc);
> +	if (ret)
> +		return ret;
> +
> +	return mfd_add_devices(cgbc->dev, -1, cgbc_devs,
> +			       ARRAY_SIZE(cgbc_devs), NULL, 0, NULL);
> +}
> +
> +static int cgbc_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct cgbc_device_data *cgbc;
> +	int ret;
> +
> +	cgbc = devm_kzalloc(dev, sizeof(*cgbc), GFP_KERNEL);
> +	if (!cgbc)
> +		return -ENOMEM;
> +
> +	cgbc->dev = dev;
> +
> +	ret = cgbc_map(cgbc);
> +	if (ret)
> +		return ret;
> +
> +	mutex_init(&cgbc->lock);
> +
> +	platform_set_drvdata(pdev, cgbc);
> +
> +	return cgbc_init_device(cgbc);
> +}
> +
> +static void cgbc_remove(struct platform_device *pdev)
> +{
> +	struct cgbc_device_data *cgbc = platform_get_drvdata(pdev);
> +
> +	cgbc_session_release(cgbc);
> +
> +	mfd_remove_devices(&pdev->dev);
> +}
> +
> +static struct platform_driver cgbc_driver = {
> +	.driver		= {
> +		.name		= "cgbc",
> +		.dev_groups	= cgbc_groups,
> +	},
> +	.probe		= cgbc_probe,
> +	.remove_new	= cgbc_remove,
> +};
> +
> +static const struct dmi_system_id cgbc_dmi_table[] __initconst = {
> +	{
> +		.ident = "SA7",
> +		.matches = {
> +			DMI_MATCH(DMI_BOARD_VENDOR, "congatec"),
> +			DMI_MATCH(DMI_BOARD_NAME, "conga-SA7"),
> +		},
> +	},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(dmi, cgbc_dmi_table);
> +
> +static int __init cgbc_init(void)
> +{
> +	const struct dmi_system_id *id;
> +	int ret = -ENODEV;
> +
> +	id = dmi_first_match(cgbc_dmi_table);
> +	if (IS_ERR_OR_NULL(id))
> +		return ret;
> +
> +	ret = cgbc_create_platform_device(&cgbc_platform_data);

You have no other way to register this device?  ACPI/PCI/I2C?

> +	if (ret)
> +		return ret;
> +
> +	return platform_driver_register(&cgbc_driver);
> +}
> +
> +static void __exit cgbc_exit(void)
> +{
> +	platform_device_unregister(cgbc_pdev);
> +	platform_driver_unregister(&cgbc_driver);
> +}
> +
> +module_init(cgbc_init);
> +module_exit(cgbc_exit);
> +
> +MODULE_DESCRIPTION("Congatec Board Controller Core Driver");
> +MODULE_AUTHOR("Thomas Richard <thomas.richard@bootlin.com>");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:cgbc-core");
> diff --git a/include/linux/mfd/cgbc.h b/include/linux/mfd/cgbc.h
> new file mode 100644
> index 000000000000..badbec4c7033
> --- /dev/null
> +++ b/include/linux/mfd/cgbc.h
> @@ -0,0 +1,44 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Congatec Board Controller driver definitions
> + *
> + * Copyright (C) 2024 Bootlin
> + * Author: Thomas Richard <thomas.richard@bootlin.com>
> + */
> +
> +#ifndef _LINUX_MFD_CGBC_H_
> +
> +/**
> + * struct cgbc_version - Board Controller device version structure
> + * @feature:	Board Controller feature number
> + * @major:	Board Controller major revision
> + * @minor:	Board Controller minor revision
> + */
> +struct cgbc_version {
> +	unsigned char feature;
> +	unsigned char major;
> +	unsigned char minor;
> +};
> +
> +/**
> + * struct cgbc_device_data - Internal representation of the Board Controller device
> + * @io_session:		Pointer to the session IO memory
> + * @io_cmd:		Pointer to the command IO memory
> + * @session:		Session id returned by the Board Controller
> + * @dev:		Pointer to kernel device structure
> + * @cgbc_version:	Board Controller version structure
> + * @mutex:		Board Controller mutex
> + */
> +struct cgbc_device_data {
> +	void __iomem		*io_session;
> +	void __iomem		*io_cmd;
> +	u8			session;
> +	struct device		*dev;
> +	struct cgbc_version	version;
> +	struct mutex		lock;
> +};
> +
> +int cgbc_command(struct cgbc_device_data *cgbc, void *cmd, unsigned int cmd_size,
> +		 void *data, unsigned int data_size, u8 *status);
> +
> +#endif /*_LINUX_MFD_CGBC_H_*/

There's more to review, but I think there is enough to turn around for now.

-- 
Lee Jones [李琼斯]

Re: [PATCH 4/5] watchdog: Congatec Board Controller watchdog timer driver

[Thomas Richard]

Hi Guenter,

>> +
>> +struct cgbc_wdt_cmd_cfg {
>> +	u8 cmd;
>> +	u8 mode;
>> +	u8 action;
>> +	u8 timeout1[3];
>> +	u8 timeout2[3];
>> +	u8 reserved[3];
>> +	u8 delay[3];
>> +} __packed;
>> +
>> +static_assert(sizeof(struct cgbc_wdt_cmd_cfg) == 15);
> 
> static_assert() is declared in linux/build_bug.h. Please include all
> necessary include files explicitly and do not depend on indirect includes.

Fixed in next iteration.

> 
>> +
>> +static int cgbc_wdt_start(struct watchdog_device *wdd)
>> +{
>> +	struct cgbc_wdt_data *wdt_data = watchdog_get_drvdata(wdd);
> 
> Unusual way to get wdt_data instead of using container_of().
> Any special reason ?

No special reason, I saw that watchdog_get_drvdata() was often used in
watchdog drivers (more than container_of()) to get wdt_data.
But I can use container_of() if you think I should.

> 
>> +	struct cgbc_device_data *cgbc = wdt_data->cgbc;
>> +	unsigned int timeout1 = (wdd->timeout - wdd->pretimeout) * 1000;
>> +	unsigned int timeout2 = wdd->pretimeout * 1000;
>> +	u8 action;
>> +
>> +	struct cgbc_wdt_cmd_cfg cmd_start = {
>> +		.cmd = CGBC_WDT_CMD_INIT,
>> +		.mode = CGBC_WDT_MODE_SINGLE_EVENT,
>> +		.timeout1[0] = (u8)timeout1,
>> +		.timeout1[1] = (u8)(timeout1 >> 8),
>> +		.timeout1[2] = (u8)(timeout1 >> 16),
>> +		.timeout2[0] = (u8)timeout2,
>> +		.timeout2[1] = (u8)(timeout2 >> 8),
>> +		.timeout2[2] = (u8)(timeout2 >> 16),
>> +	};
>> +
>> +	if (wdd->pretimeout) {
>> +		action = 2;
>> +		action |= wdt_data->pretimeout_action << 2;
>> +		action |= wdt_data->timeout_action << 4;
>> +	} else {
>> +		action = 1;
>> +		action |= wdt_data->timeout_action << 2;
>> +	}
>> +
>> +	cmd_start.action = action;
>> +
>> +	return cgbc_command(cgbc, &cmd_start, sizeof(cmd_start), NULL, 0, NULL);
>> +}
>> +
>> +static int cgbc_wdt_stop(struct watchdog_device *wdd)
>> +{
>> +	struct cgbc_wdt_data *wdt_data = watchdog_get_drvdata(wdd);
>> +	struct cgbc_device_data *cgbc = wdt_data->cgbc;
>> +	struct cgbc_wdt_cmd_cfg cmd_stop = {
>> +		.cmd = CGBC_WDT_CMD_INIT,
>> +		.mode = CGBC_WDT_DISABLE,
>> +	};
>> +
>> +	return cgbc_command(cgbc, &cmd_stop, sizeof(cmd_stop), NULL, 0, NULL);
>> +}
>> +
>> +static int cgbc_wdt_keepalive(struct watchdog_device *wdd)
>> +{
>> +	struct cgbc_wdt_data *wdt_data = watchdog_get_drvdata(wdd);
>> +	struct cgbc_device_data *cgbc = wdt_data->cgbc;
>> +	u8 cmd_ping = CGBC_WDT_CMD_TRIGGER;
>> +
>> +	return cgbc_command(cgbc, &cmd_ping, sizeof(cmd_ping), NULL, 0, NULL);
>> +}
>> +
>> +static int cgbc_wdt_set_pretimeout(struct watchdog_device *wdd,
>> +				   unsigned int pretimeout)
>> +{
>> +	struct cgbc_wdt_data *wdt_data = watchdog_get_drvdata(wdd);
>> +
>> +	wdd->pretimeout = pretimeout;
>> +	wdt_data->pretimeout_action = ACTION_SMI;
>> +
>> +	if (watchdog_active(wdd))
>> +		return cgbc_wdt_start(wdd);
>> +
>> +	return 0;
>> +}
>> +
>> +static int cgbc_wdt_set_timeout(struct watchdog_device *wdd,
>> +				unsigned int timeout)
>> +{
>> +	struct cgbc_wdt_data *wdt_data = watchdog_get_drvdata(wdd);
>> +
>> +	if (timeout < wdd->pretimeout) {
>> +		dev_warn(wdd->parent, "timeout <= pretimeout. Setting pretimeout to zero\n");
> 
> That is a normal condition which does not warrant a log message.
> Also see drivers/watchdog/watchdog_dev.c around line 385.

Fixed in next iteration.

> 
>> +		wdd->pretimeout = 0;
>> +	}
>> +
>> +	wdd->timeout = timeout;
>> +	wdt_data->timeout_action = ACTION_RESET;
> 
> Both timeout_action and pretimeout_action are set statically.
> What is the point of doing that instead of just using
> ACTION_RESET and ACTION_SMI as needed irectly ?

Yes indeed, using ACTION_RESET and ACTION_SMI directly in
cgbc_wdt_start() makes the code smaller.

> 
>> +
>> +	if (watchdog_active(wdd))
>> +		return cgbc_wdt_start(wdd);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct watchdog_info cgbc_wdt_info = {
>> +	.identity	= "CGBC Watchdog",
>> +	.options	= WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING |
>> +		WDIOF_MAGICCLOSE | WDIOF_PRETIMEOUT
>> +};
>> +
>> +static const struct watchdog_ops cgbc_wdt_ops = {
>> +	.owner		= THIS_MODULE,
>> +	.start		= cgbc_wdt_start,
>> +	.stop		= cgbc_wdt_stop,
>> +	.ping		= cgbc_wdt_keepalive,
>> +	.set_timeout	= cgbc_wdt_set_timeout,
>> +	.set_pretimeout = cgbc_wdt_set_pretimeout,
>> +};
>> +
>> +static int cgbc_wdt_probe(struct platform_device *pdev)
>> +{
>> +	struct cgbc_device_data *cgbc = dev_get_drvdata(pdev->dev.parent);
>> +	struct device *dev = &pdev->dev;
>> +	struct cgbc_wdt_data *wdt_data;
>> +	struct watchdog_device *wdd;
>> +	int ret;
>> +
>> +	wdt_data = devm_kzalloc(dev, sizeof(*wdt_data), GFP_KERNEL);
> 
> devm_kzalloc() is declared in linux/device.h. Again, please include all
> necessary include files explicitly.

Fixed in next iteration.

> 
>> +	if (!wdt_data)
>> +		return -ENOMEM;
>> +
>> +	wdt_data->cgbc = cgbc;
>> +	wdd = &wdt_data->wdd;
>> +	wdd->parent = dev;
>> +
> 
> No limits ? That is unusual. Are you sure the driver accepts all
> timeouts from 0 to UINT_MAX ?

Yes limits are needed.
For next iteration I added 1s as min_timeout (which seems to be the
usual value, and it is accepted by the hardware), and a max_timeout.

> 
>> +	wdd->info = &cgbc_wdt_info;
>> +	wdd->ops = &cgbc_wdt_ops;
>> +
>> +	watchdog_set_drvdata(wdd, wdt_data);
>> +	watchdog_set_nowayout(wdd, nowayout);
>> +
>> +	cgbc_wdt_set_timeout(wdd, timeout);
>> +	cgbc_wdt_set_pretimeout(wdd, pretimeout);
> 
> The more common approach would be to set default limits in wdd->{timout,pretimeout}
> and only override the values if needed, ie if provided using module parameters.
> That implies initializing the module parameters with 0. YOur call, though.

Ok.
For next iteration I added limits (min_timeout, max_timeout), the
timeout module parameter is set to 0 by default, and
watchdog_init_timeout() is called in the probe.

> 
>> +
>> +	platform_set_drvdata(pdev, wdt_data);
>> +	watchdog_stop_on_reboot(wdd);
>> +	watchdog_stop_on_unregister(wdd);
>> +
>> +	ret = devm_watchdog_register_device(dev, wdd);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return 0;
> 
> Why not just
> 	return devm_watchdog_register_device(dev, wdd);
> ?

I don't know :)
Fixed in the next iteration.

Thanks for the review !!

Thomas.

Re: [PATCH 3/5] i2c: Congatec Board Controller i2c bus driver

[Thomas Richard]

On 8/14/24 01:24, Andi Shyti wrote:
> Hi Thomas,

Hi Andi,

Thanks for the review !!

> 
> On Fri, Aug 09, 2024 at 04:52:07PM GMT, Thomas Richard wrote:
>> Add i2c support for the Congatec Board Controller.
>> do you mind adding some more description?

I'll mention that there are 2 busses.

> 
>> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
> 
> ...
> 
>> +config I2C_CGBC
>> +	tristate "Congatec I2C Controller"
>> +	depends on MFD_CGBC
>> +	help
>> +	  This enables the I2C bus interfaces for the Congatec Board
> 
> This what? :-)

Rephrased it for next iteration.

> 
>> +	  Controller.
>> +
>> +	  This driver can also be built as a module. If so, the module will
>> +	  be called i2c-cgbc.ko.
>> +
> 
> ...
> 
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/i2c.h>
>> +#include <linux/iopoll.h>
> 
> please sort includes in alphabetical order?

Fixed in the next iteration.

> 
>> +#include <linux/mfd/cgbc.h>
> 
> ...
> 
>> +enum i2c_state {
>> +	STATE_DONE = 0,
>> +	STATE_INIT,
>> +	STATE_START,
>> +	STATE_READ,
>> +	STATE_WRITE,
>> +	STATE_ERROR,
>> +};
> 
> can you please use the cgbc prefix for this enum and all the
> members?

Ok, fixed in the next iteration.

> 
> ...
> 
>> +	if (bus_frequency > CGBC_I2C_FREQ_MAX_HZ ||
>> +	    bus_frequency < CGBC_I2C_FREQ_MIN_HZ) {
>> +		dev_warn(i2c->dev, "invalid frequency %u, using default\n", bus_frequency);
> 
> should this rather be a dev_info()? (supernit: please start with
> capital leter).

The driver i2c-xlp9xx has a similar message [1] and it uses a dev_warn().
So I don't know.
If you think dev_info() is more relevant in this case, I'll change it.
Supernit will be fixed in next iteration.

[1]
https://elixir.bootlin.com/linux/v6.11-rc6/source/drivers/i2c/busses/i2c-xlp9xx.c#L480

> 
>> +		bus_frequency = I2C_MAX_STANDARD_MODE_FREQ;
>> +	}
>> +
>> +	cmd[0] = CGBC_I2C_CMD_SPEED | algo_data->bus_id;
>> +	cmd[1] = cgbc_i2c_freq_to_reg(bus_frequency);
>> +
>> +	ret = cgbc_command(cgbc, &cmd, sizeof(cmd), &data, 1, NULL);
>> +	if (ret)
>> +		return dev_err_probe(i2c->dev, ret,
>> +				     "Failed to initialize I2C bus %s",
>> +				     adap->name);
>> +
>> +	cmd[1] = 0x00;
>> +
>> +	ret = cgbc_command(cgbc, &cmd, sizeof(cmd), &data, 1, NULL);
>> +	if (ret)
>> +		return dev_err_probe(i2c->dev, ret,
>> +				     "Failed to get I2C bus frequency");
>> +
>> +	bus_frequency = cgbc_i2c_reg_to_freq(data);
>> +
>> +	dev_dbg(i2c->dev, "%s is running at %d Hz\n", adap->name, bus_frequency);
>> +
>> +	/*
>> +	 * The read_maxtime_us is the maximum time to wait during a read to get
>> +	 * data. At maximum CGBC_I2C_READ_MAX_LEN can be read by command.
>> +	 * So calculate the max time to size correctly the timeout.
>> +	 */
> 
> this comment is a bit wild, can we rephrase to something like:
> 
> /*
>  * The read_maxtime_us variable represents the maximum time to wait
>  * for data during a read operation. The maximum amount of data that
>  * can be read by a command is CGBC_I2C_READ_MAX_LEN.
>  * Therefore, calculate the max time to properly size the timeout.
>  */
> 
> (it's a suggestion, please choose the words you prefer).

thanks for the rephrasing.

Thomas

Re: [PATCH 3/5] i2c: Congatec Board Controller i2c bus driver

[Andi Shyti]

Hi Thomas,

> >> +	if (bus_frequency > CGBC_I2C_FREQ_MAX_HZ ||
> >> +	    bus_frequency < CGBC_I2C_FREQ_MIN_HZ) {
> >> +		dev_warn(i2c->dev, "invalid frequency %u, using default\n", bus_frequency);
> > 
> > should this rather be a dev_info()? (supernit: please start with
> > capital leter).
> 
> The driver i2c-xlp9xx has a similar message [1] and it uses a dev_warn().
> So I don't know.
> If you think dev_info() is more relevant in this case, I'll change it.
> Supernit will be fixed in next iteration.

Not a binding comment, it looks to me more a dev_info(), if you
prefer you can use dev_warn(). In any case, at this stage, I
don't see this printed anywhere :-)

Thanks,
Andi

Re: [PATCH 1/5] mfd: add Congatec Board Controller mfd driver

[Thomas Richard]

On 8/22/24 12:38, Lee Jones wrote:
> On Fri, 09 Aug 2024, Thomas Richard wrote:
> 
>> Add core MFD driver for the Board Controller found on some Congatec SMARC
>> module. This Board Controller provides functions like watchdog, GPIO, and
>> I2C busses.
>>
>> This commit add support only for the conga-SA7 module.
>>
>> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
>> ---
>>  drivers/mfd/Kconfig      |  12 ++
>>  drivers/mfd/Makefile     |   1 +
>>  drivers/mfd/cgbc-core.c  | 453 +++++++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/mfd/cgbc.h |  44 +++++
>>  4 files changed, 510 insertions(+)
>>
>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
>> index bc8be2e593b6..3e0530f30267 100644
>> --- a/drivers/mfd/Kconfig
>> +++ b/drivers/mfd/Kconfig
>> @@ -224,6 +224,18 @@ config MFD_AXP20X_RSB
>>  	  components like regulators or the PEK (Power Enable Key) under the
>>  	  corresponding menus.
>>  
>> +config MFD_CGBC
>> +	tristate "Congatec Board Controller"
>> +	select MFD_CORE
>> +	depends on X86
>> +	help
>> +	  This is the core driver of the Board Controller found on some Congatec
>> +	  SMARC modules. The Board Controller provides functions like watchdog,
>> +	  I2C busses, and GPIO controller.
>> +
>> +	  To compile this driver as a module, choose M here: the module will be
>> +	  called cgbc-core.
>> +
>>  config MFD_CROS_EC_DEV
>>  	tristate "ChromeOS Embedded Controller multifunction device"
>>  	select MFD_CORE
>> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
>> index 02b651cd7535..d5da3fcd691c 100644
>> --- a/drivers/mfd/Makefile
>> +++ b/drivers/mfd/Makefile
>> @@ -13,6 +13,7 @@ obj-$(CONFIG_MFD_SM501)		+= sm501.o
>>  obj-$(CONFIG_ARCH_BCM2835)	+= bcm2835-pm.o
>>  obj-$(CONFIG_MFD_BCM590XX)	+= bcm590xx.o
>>  obj-$(CONFIG_MFD_BD9571MWV)	+= bd9571mwv.o
>> +obj-$(CONFIG_MFD_CGBC)		+= cgbc-core.o
>>  obj-$(CONFIG_MFD_CROS_EC_DEV)	+= cros_ec_dev.o
>>  obj-$(CONFIG_MFD_CS42L43)	+= cs42l43.o
>>  obj-$(CONFIG_MFD_CS42L43_I2C)	+= cs42l43-i2c.o
>> diff --git a/drivers/mfd/cgbc-core.c b/drivers/mfd/cgbc-core.c
>> new file mode 100644
>> index 000000000000..cca9b1170cc9
>> --- /dev/null
>> +++ b/drivers/mfd/cgbc-core.c
>> @@ -0,0 +1,453 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * Congatec Board Controller MFD core driver.
> 
> No such thing as an MFD.

What should it be if it's not an MFD ?

> 
>> + * The x86 Congatec modules have an embedded micro controller named Board
>> + * Controller.
> 
> I think the '\n' can be dropped without issue.

Will be fixed in v2

> 
>> + * This Board Controller have a watchdog timer, some GPIOs, and two i2c busses.
> 
> "has a"
> 
> Consider "Watchdog"
> 
> "I2C"

Will be fixed in v2

> 
>> + *
>> + * Copyright (C) 2024 Bootlin
> 
> Nit: '\n' here.

Will be fixed in v2

> 
>> + * Author: Thomas Richard <thomas.richard@bootlin.com>
>> + */
>> +
>> +#include <linux/dmi.h>
>> +#include <linux/mfd/core.h>
>> +#include <linux/mfd/cgbc.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/iopoll.h>
>> +#include <linux/sysfs.h>
> 
> Alphabetical.

Will be fixed in v2

> 
>> +#define CGBC_MASK_STATUS        (BIT(6) | BIT(7))
>> +#define CGBC_MASK_DATA_COUNT	0x1F
>> +#define CGBC_MASK_ERROR_CODE	0x1F
> 
> Not sure if it's just my mailer, but if all of these are not already
> aligned, please can you tab them out so that they are.

It was not your mailer :)
Will be fixed in v2

> 
>> +#define CGBC_STATUS_DATA_READY	0x00
>> +#define CGBC_STATUS_CMD_READY	BIT(6)
>> +#define CGBC_STATUS_ERROR	(BIT(6) | BIT(7))
>> +
>> +#define CGBC_CMD_GET_FW_REV	0x21
>> +
>> +#define CGBC_IO_SESSION_BASE	0x0E20
>> +#define CGBC_IO_SESSION_END	0x0E30
>> +#define CGBC_IO_CMD_BASE	0x0E00
>> +#define CGBC_IO_CMD_END		0x0E10
>> +
>> +#define CGBC_SESSION_CMD	0x00
>> +#define		CGBC_SESSION_CMD_IDLE		0x00
>> +#define		CGBC_SESSION_CMD_REQUEST	0x01
>> +#define	CGBC_SESSION_DATA	0x01
>> +#define CGBC_SESSION_STATUS	0x02
>> +#define		CGBC_SESSION_STATUS_FREE	0x03
>> +#define CGBC_SESSION_ACCESS	0x04
>> +#define		CGBC_SESSION_ACCESS_GAINED	0x00
>> +
>> +#define CGBC_SESSION_VALID_MIN  0x02
>> +#define CGBC_SESSION_VALID_MAX  0xFE
>> +
>> +#define CGBC_CMD_STROBE	0x00
>> +#define CGBC_CMD_INDEX	0x02
>> +#define		CGBC_CMD_INDEX_CBM_MAN8		0x00
>> +#define		CGBC_CMD_INDEX_CBM_AUTO32	0x03
>> +#define CGBC_CMD_DATA	0x04
>> +#define CGBC_CMD_ACCESS	0x0C
>> +
>> +struct cgbc_platform_data {
>> +	const struct resource	*ioresource;
>> +	unsigned int		num_ioresource;
>> +};
> 
> This looks way over engineered.
> 
> Why not provide platform_device_info statically in the first place?

Yes indeed, if I use platform_device_register_simple(), I can remove the
struct cgbc_platform_data, and cgbc_create_platform_device().
It makes the code smaller.

> 
>> +static struct platform_device *cgbc_pdev;
> 
> No avoidable globals.

I don't see how can I have cgbc_pdev not global.

> 
> This stuff should get stored in driver data (ddata)
> 
>> +static int cgbc_detect_device(struct cgbc_device_data *cgbc)
>> +{
>> +	u16 status;
>> +	int ret;
>> +
>> +	ret = readx_poll_timeout(ioread16, cgbc->io_session + CGBC_SESSION_STATUS, status,
>> +				 status == CGBC_SESSION_STATUS_FREE, 0, 500000);
>> +
>> +	if (ret || ioread32(cgbc->io_session + CGBC_SESSION_ACCESS))
>> +		ret = -ENODEV;
>> +
>> +	return ret;
>> +}
> 
> This function could do with some commentary.

Comment added in v2

> 
>> +static int cgbc_session_command(struct cgbc_device_data *cgbc, u8 cmd)
>> +{
>> +	int ret;
>> +	u8 val;
>> +
>> +	ret = readx_poll_timeout(ioread8, cgbc->io_session + CGBC_SESSION_CMD, val,
>> +				 val == CGBC_SESSION_CMD_IDLE, 0, 100000);
>> +	if (ret)
>> +		return ret;
>> +
>> +	iowrite8(cmd, cgbc->io_session + CGBC_SESSION_CMD);
>> +
>> +	ret = readx_poll_timeout(ioread8, cgbc->io_session + CGBC_SESSION_CMD, val,
>> +				 val == CGBC_SESSION_CMD_IDLE, 0, 100000);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = (int)ioread8(cgbc->io_session + CGBC_SESSION_DATA);
>> +
>> +	iowrite8(CGBC_SESSION_STATUS_FREE,
>> +		 cgbc->io_session + CGBC_SESSION_STATUS);
> 
> Which char are you using to line-feed?  This looks inconsistent.
> 
>> +
>> +	return ret;
>> +}
>> +
>> +static int cgbc_session_request(struct cgbc_device_data *cgbc)
>> +{
>> +	unsigned int ret = cgbc_detect_device(cgbc);
> 
> What does cgbc_detect_device() do?
> 
> You're not getting the device?  Just seeing if it's present?

cgbc_detect_device() waits the device ready to create (or close) a session.
I will rename it to cgbc_wait_device().
Indirectly it checks if the device is present as it waits some specific
values.

> 
> Please separate the declaration from the assignment when calling
> functions.
> 

Will be fixed in v2

>> +	if (ret)
>> +		return dev_err_probe(cgbc->dev, ret, "device not found\n");
>> +
>> +	cgbc->session = cgbc_session_command(cgbc, CGBC_SESSION_CMD_REQUEST);
>> +
>> +	/* the Board Controller sent us a wrong session handle, we cannot
> 
> This is a non-standard multi-line comment.

Will be fixed in v2

> 
>> +	 * communicate with it.
>> +	 */
>> +	if (cgbc->session < CGBC_SESSION_VALID_MIN ||
>> +	    cgbc->session > CGBC_SESSION_VALID_MAX) {
> 
> How are you justifying this line-break and then not line-breaking 2
> lines down?

Will be fixed in v2

> 
>> +		cgbc->session = 0;
> 
> Why are we setting variables before bombing out?
> 
>> +		return dev_err_probe(cgbc->dev, (cgbc->session < 0) ? cgbc->session : -ECONNREFUSED,
> 
> How can (cgbc->session < 0) ever be true?

It cannot.
It's a total nonsense, I'll fix it in v2.

> 
>> +				     "failed to get a valid session handle\n");
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static void cgbc_session_release(struct cgbc_device_data *cgbc)
>> +{
>> +	if (cgbc_session_command(cgbc, cgbc->session) != cgbc->session)
> 
> What does this do?
> 
> Are we checking or doing something?  If the latter, then we shouldn't be
> doing that in an if() statement.  If the former, then what's the point
> of the function?

We try to close the session we opened with the device.
We check that the session is closed correctly.

> 
>> +		dev_err(cgbc->dev, "failed to release session\n");
> 
> Why print if there isn't anything you can do about it?

There is a limited number of session ids.
Not releasing correctly the session could be a problem in case of lots
of module load/unload.
But as 252 sessions can be opened, the maximum number of session will
probably never be reached.
But it could be interesting to have the information that something is
wrong and we failed to close the session.
A dev_info() or dev_warn() is maybe more relevant.

> 
>> +}
>> +
>> +static bool cgbc_command_lock(struct cgbc_device_data *cgbc)
>> +{
>> +	iowrite8(cgbc->session, cgbc->io_cmd + CGBC_CMD_ACCESS);
>> +
>> +	return ioread8(cgbc->io_cmd + CGBC_CMD_ACCESS) == cgbc->session;
>> +}
>> +
>> +static void cgbc_command_unlock(struct cgbc_device_data *cgbc)
>> +{
>> +	iowrite8(cgbc->session, cgbc->io_cmd + CGBC_CMD_ACCESS);
>> +}
>> +
>> +static int __cgbc_command(struct cgbc_device_data *cgbc, u8 *cmd, u8 cmd_size,
>> +			  u8 *data, u8 data_size, u8 *status)
>> +{
>> +	u8 checksum = 0, data_checksum = 0, istatus = 0, val;
>> +	int mode_change = -1;
>> +	bool lock;
>> +	int ret, i;
>> +
>> +	mutex_lock(&cgbc->lock);
>> +
>> +	/* request access */
> 
> Start comments with uppercase chars please.

Will be fixed in v2

> 
>> +	ret = readx_poll_timeout(cgbc_command_lock, cgbc, lock, lock, 0, 100000);
>> +	if (ret)
>> +		goto out;
>> +
>> +	/* wait board controller is ready */
>> +	ret = readx_poll_timeout(ioread8, cgbc->io_cmd + CGBC_CMD_STROBE, val,
>> +				 val == CGBC_CMD_STROBE, 0, 100000);
>> +	if (ret)
>> +		goto release;
>> +
>> +	/* write command packet */
>> +	if (cmd_size <= 2) {
>> +		iowrite8(CGBC_CMD_INDEX_CBM_MAN8,
>> +			 cgbc->io_cmd + CGBC_CMD_INDEX);
>> +	} else {
>> +		iowrite8(CGBC_CMD_INDEX_CBM_AUTO32,
>> +			 cgbc->io_cmd + CGBC_CMD_INDEX);
>> +		if ((cmd_size % 4) != 0x03)
>> +			mode_change = (cmd_size & 0xFFFC) - 1;
>> +	}
>> +
>> +	for (i = 0; i < cmd_size; i++) {
>> +		iowrite8(cmd[i], cgbc->io_cmd + CGBC_CMD_DATA + (i % 4));
>> +		checksum ^= cmd[i];
>> +		if (mode_change == i)
>> +			iowrite8((i + 1) | CGBC_CMD_INDEX_CBM_MAN8,
>> +				 cgbc->io_cmd + CGBC_CMD_INDEX);
>> +	}
>> +
>> +	/* append checksum byte */
>> +	iowrite8(checksum, cgbc->io_cmd + CGBC_CMD_DATA + (i % 4));
>> +
>> +	/* perform command strobe */
>> +	iowrite8(cgbc->session, cgbc->io_cmd + CGBC_CMD_STROBE);
>> +
>> +	/* rewind cmd buffer index */
>> +	iowrite8(CGBC_CMD_INDEX_CBM_AUTO32,
>> +		 cgbc->io_cmd + CGBC_CMD_INDEX);
>> +
>> +	/* wait command completion */
>> +	ret = read_poll_timeout(ioread8, val, val == CGBC_CMD_STROBE, 0,
>> +				100000, false,
>> +				cgbc->io_cmd + CGBC_CMD_STROBE);
>> +	if (ret)
>> +		goto release;
>> +
>> +	istatus = ioread8(cgbc->io_cmd + CGBC_CMD_DATA);
>> +	checksum = istatus;
>> +
>> +	/* check command status */
>> +	switch (istatus & CGBC_MASK_STATUS) {
>> +	case CGBC_STATUS_DATA_READY:
>> +		if (istatus > data_size)
>> +			istatus = data_size;
>> +		for (i = 0; i < istatus; i++) {
>> +			data[i] = ioread8(cgbc->io_cmd +
>> +					  CGBC_CMD_DATA + ((i + 1) % 4));
>> +			checksum ^= data[i];
>> +		}
>> +		data_checksum = ioread8(cgbc->io_cmd +
>> +					CGBC_CMD_DATA + ((i + 1) % 4));
>> +		istatus &= CGBC_MASK_DATA_COUNT;
>> +		break;
>> +	case CGBC_STATUS_ERROR:
>> +	case CGBC_STATUS_CMD_READY:
>> +		data_checksum = ioread8(cgbc->io_cmd +
>> +					CGBC_CMD_DATA + 1);
>> +		if ((istatus & CGBC_MASK_STATUS) == CGBC_STATUS_ERROR)
>> +			ret = -EIO;
>> +		istatus = istatus & CGBC_MASK_ERROR_CODE;
>> +		break;
>> +	default:
>> +		data_checksum = ioread8(cgbc->io_cmd + CGBC_CMD_DATA + 1);
>> +		istatus &= CGBC_MASK_ERROR_CODE;
>> +		ret = -EIO;
>> +		break;
>> +	}
>> +
>> +	/* checksum verification */
>> +	if (ret == 0 && data_checksum != checksum)
>> +		ret = -EIO;
>> +
>> +release:
>> +	cgbc_command_unlock(cgbc);
>> +
>> +out:
>> +	mutex_unlock(&cgbc->lock);
>> +
>> +	if (status)
>> +		*status = istatus;
>> +
>> +	return ret;
>> +}
>> +
>> +int cgbc_command(struct cgbc_device_data *cgbc, void *cmd, unsigned int cmd_size,
>> +		 void *data, unsigned int data_size, u8 *status)
> 
> What's the point of this abstracted function?

The goal was to cast cmd and data.
But it can be done directly in the function. So this wrapper is useless.

> 
>> +{
>> +	return __cgbc_command(cgbc, (u8 *)cmd, cmd_size, (u8 *)data, data_size, status);
>> +}
>> +EXPORT_SYMBOL_GPL(cgbc_command);
>> +
>> +static struct mfd_cell cgbc_devs[] = {
>> +	{ .name = "cgbc-wdt"	},
>> +	{ .name = "cgbc-gpio"	},
>> +	{ .name = "cgbc-i2c", .id = 1 },
>> +	{ .name = "cgbc-i2c", .id = 2 },
>> +};
>> +
>> +static int cgbc_map(struct cgbc_device_data *cgbc)
>> +{
>> +	struct device *dev = cgbc->dev;
>> +	struct platform_device *pdev = to_platform_device(dev);
>> +	struct resource *ioport;
>> +
>> +	ioport = platform_get_resource(pdev, IORESOURCE_IO, 0);
>> +	if (!ioport)
>> +		return -EINVAL;
>> +
>> +	cgbc->io_session = devm_ioport_map(dev, ioport->start,
>> +					   resource_size(ioport));
>> +	if (!cgbc->io_session)
>> +		return -ENOMEM;
>> +
>> +	ioport = platform_get_resource(pdev, IORESOURCE_IO, 1);
>> +	if (!ioport)
>> +		return -EINVAL;
>> +
>> +	cgbc->io_cmd = devm_ioport_map(dev, ioport->start,
>> +				       resource_size(ioport));
>> +	if (!cgbc->io_cmd)
>> +		return -ENOMEM;
>> +
>> +	return 0;
>> +}
>> +
>> +static struct resource cgbc_ioresource[] = {
>> +	{
>> +		.start  = CGBC_IO_SESSION_BASE,
>> +		.end    = CGBC_IO_SESSION_END,
>> +		.flags  = IORESOURCE_IO,
>> +	},
>> +	{
>> +		.start  = CGBC_IO_CMD_BASE,
>> +		.end    = CGBC_IO_CMD_END,
>> +		.flags  = IORESOURCE_IO,
>> +	},
>> +};
>> +
>> +static const struct cgbc_platform_data cgbc_platform_data = {
>> +	.ioresource = &cgbc_ioresource[0],
>> +	.num_ioresource = ARRAY_SIZE(cgbc_ioresource),
>> +};
>> +
>> +static int cgbc_create_platform_device(const struct cgbc_platform_data *pdata)
>> +{
>> +	const struct platform_device_info pdevinfo = {
>> +		.name = "cgbc",
>> +		.id = PLATFORM_DEVID_NONE,
>> +		.res = pdata->ioresource,
>> +		.num_res = pdata->num_ioresource,
>> +	};
>> +
>> +	cgbc_pdev = platform_device_register_full(&pdevinfo);
>> +	if (IS_ERR(cgbc_pdev))
>> +		return PTR_ERR(cgbc_pdev);
>> +
>> +	return 0;
>> +}
>> +
>> +static ssize_t cgbc_version_show(struct device *dev,
>> +				 struct device_attribute *attr, char *buf)
>> +{
>> +	struct cgbc_device_data *cgbc = dev_get_drvdata(dev);
>> +
>> +	return sysfs_emit(buf, "CGBCP%c%c%c\n", cgbc->version.feature,
>> +			  cgbc->version.major, cgbc->version.minor);
>> +}
>> +
>> +static DEVICE_ATTR_RO(cgbc_version);
>> +
>> +static struct attribute *cgbc_attrs[] = {
>> +	&dev_attr_cgbc_version.attr,
>> +	NULL
>> +};
>> +
>> +ATTRIBUTE_GROUPS(cgbc);
>> +
>> +static int cgbc_get_version(struct cgbc_device_data *cgbc)
>> +{
>> +	u8 cmd = CGBC_CMD_GET_FW_REV;
>> +	u8 data[4];
>> +	int ret;
>> +
>> +	ret = cgbc_command(cgbc, &cmd, 1, &data, sizeof(data), NULL);
>> +	if (ret)
>> +		return ret;
>> +
>> +	cgbc->version.feature = data[0];
>> +	cgbc->version.major = data[1];
>> +	cgbc->version.minor = data[2];
>> +
>> +	return 0;
>> +}
>> +
>> +static int cgbc_init_device(struct cgbc_device_data *cgbc)
>> +{
>> +	int ret;
>> +
>> +	ret = cgbc_session_request(cgbc);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = cgbc_get_version(cgbc);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return mfd_add_devices(cgbc->dev, -1, cgbc_devs,
>> +			       ARRAY_SIZE(cgbc_devs), NULL, 0, NULL);
>> +}
>> +
>> +static int cgbc_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct cgbc_device_data *cgbc;
>> +	int ret;
>> +
>> +	cgbc = devm_kzalloc(dev, sizeof(*cgbc), GFP_KERNEL);
>> +	if (!cgbc)
>> +		return -ENOMEM;
>> +
>> +	cgbc->dev = dev;
>> +
>> +	ret = cgbc_map(cgbc);
>> +	if (ret)
>> +		return ret;
>> +
>> +	mutex_init(&cgbc->lock);
>> +
>> +	platform_set_drvdata(pdev, cgbc);
>> +
>> +	return cgbc_init_device(cgbc);
>> +}
>> +
>> +static void cgbc_remove(struct platform_device *pdev)
>> +{
>> +	struct cgbc_device_data *cgbc = platform_get_drvdata(pdev);
>> +
>> +	cgbc_session_release(cgbc);
>> +
>> +	mfd_remove_devices(&pdev->dev);
>> +}
>> +
>> +static struct platform_driver cgbc_driver = {
>> +	.driver		= {
>> +		.name		= "cgbc",
>> +		.dev_groups	= cgbc_groups,
>> +	},
>> +	.probe		= cgbc_probe,
>> +	.remove_new	= cgbc_remove,
>> +};
>> +
>> +static const struct dmi_system_id cgbc_dmi_table[] __initconst = {
>> +	{
>> +		.ident = "SA7",
>> +		.matches = {
>> +			DMI_MATCH(DMI_BOARD_VENDOR, "congatec"),
>> +			DMI_MATCH(DMI_BOARD_NAME, "conga-SA7"),
>> +		},
>> +	},
>> +	{}
>> +};
>> +MODULE_DEVICE_TABLE(dmi, cgbc_dmi_table);
>> +
>> +static int __init cgbc_init(void)
>> +{
>> +	const struct dmi_system_id *id;
>> +	int ret = -ENODEV;
>> +
>> +	id = dmi_first_match(cgbc_dmi_table);
>> +	if (IS_ERR_OR_NULL(id))
>> +		return ret;
>> +
>> +	ret = cgbc_create_platform_device(&cgbc_platform_data);
> 
> You have no other way to register this device?  ACPI/PCI/I2C?

No, the Board Controller is on a eSPI bus.
I found nothing in ACPI tables related to this Board Controller.
The only way is to use DMI tables and match board name.

> 
>> +	if (ret)
>> +		return ret;
>> +
>> +	return platform_driver_register(&cgbc_driver);
>> +}
>> +
>> +static void __exit cgbc_exit(void)
>> +{
>> +	platform_device_unregister(cgbc_pdev);
>> +	platform_driver_unregister(&cgbc_driver);
>> +}
>> +
>> +module_init(cgbc_init);
>> +module_exit(cgbc_exit);
>> +
>> +MODULE_DESCRIPTION("Congatec Board Controller Core Driver");
>> +MODULE_AUTHOR("Thomas Richard <thomas.richard@bootlin.com>");
>> +MODULE_LICENSE("GPL");
>> +MODULE_ALIAS("platform:cgbc-core");
>> diff --git a/include/linux/mfd/cgbc.h b/include/linux/mfd/cgbc.h
>> new file mode 100644
>> index 000000000000..badbec4c7033
>> --- /dev/null
>> +++ b/include/linux/mfd/cgbc.h
>> @@ -0,0 +1,44 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * Congatec Board Controller driver definitions
>> + *
>> + * Copyright (C) 2024 Bootlin
>> + * Author: Thomas Richard <thomas.richard@bootlin.com>
>> + */
>> +
>> +#ifndef _LINUX_MFD_CGBC_H_
>> +
>> +/**
>> + * struct cgbc_version - Board Controller device version structure
>> + * @feature:	Board Controller feature number
>> + * @major:	Board Controller major revision
>> + * @minor:	Board Controller minor revision
>> + */
>> +struct cgbc_version {
>> +	unsigned char feature;
>> +	unsigned char major;
>> +	unsigned char minor;
>> +};
>> +
>> +/**
>> + * struct cgbc_device_data - Internal representation of the Board Controller device
>> + * @io_session:		Pointer to the session IO memory
>> + * @io_cmd:		Pointer to the command IO memory
>> + * @session:		Session id returned by the Board Controller
>> + * @dev:		Pointer to kernel device structure
>> + * @cgbc_version:	Board Controller version structure
>> + * @mutex:		Board Controller mutex
>> + */
>> +struct cgbc_device_data {
>> +	void __iomem		*io_session;
>> +	void __iomem		*io_cmd;
>> +	u8			session;
>> +	struct device		*dev;
>> +	struct cgbc_version	version;
>> +	struct mutex		lock;
>> +};
>> +
>> +int cgbc_command(struct cgbc_device_data *cgbc, void *cmd, unsigned int cmd_size,
>> +		 void *data, unsigned int data_size, u8 *status);
>> +
>> +#endif /*_LINUX_MFD_CGBC_H_*/
> 
> There's more to review, but I think there is enough to turn around for now.

Thanks for the review !!

-- 
Thomas Richard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 1/5] mfd: add Congatec Board Controller mfd driver

[Lee Jones]

On Tue, 10 Sep 2024, Thomas Richard wrote:

> On 8/22/24 12:38, Lee Jones wrote:
> > On Fri, 09 Aug 2024, Thomas Richard wrote:
> > 
> >> Add core MFD driver for the Board Controller found on some Congatec SMARC
> >> module. This Board Controller provides functions like watchdog, GPIO, and
> >> I2C busses.
> >>
> >> This commit add support only for the conga-SA7 module.
> >>
> >> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
> >> ---
> >>  drivers/mfd/Kconfig      |  12 ++
> >>  drivers/mfd/Makefile     |   1 +
> >>  drivers/mfd/cgbc-core.c  | 453 +++++++++++++++++++++++++++++++++++++++++++++++
> >>  include/linux/mfd/cgbc.h |  44 +++++
> >>  4 files changed, 510 insertions(+)
> >>
> >> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> >> index bc8be2e593b6..3e0530f30267 100644
> >> --- a/drivers/mfd/Kconfig
> >> +++ b/drivers/mfd/Kconfig
> >> @@ -224,6 +224,18 @@ config MFD_AXP20X_RSB
> >>  	  components like regulators or the PEK (Power Enable Key) under the
> >>  	  corresponding menus.
> >>  
> >> +config MFD_CGBC
> >> +	tristate "Congatec Board Controller"
> >> +	select MFD_CORE
> >> +	depends on X86
> >> +	help
> >> +	  This is the core driver of the Board Controller found on some Congatec
> >> +	  SMARC modules. The Board Controller provides functions like watchdog,
> >> +	  I2C busses, and GPIO controller.
> >> +
> >> +	  To compile this driver as a module, choose M here: the module will be
> >> +	  called cgbc-core.
> >> +
> >>  config MFD_CROS_EC_DEV
> >>  	tristate "ChromeOS Embedded Controller multifunction device"
> >>  	select MFD_CORE
> >> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> >> index 02b651cd7535..d5da3fcd691c 100644
> >> --- a/drivers/mfd/Makefile
> >> +++ b/drivers/mfd/Makefile
> >> @@ -13,6 +13,7 @@ obj-$(CONFIG_MFD_SM501)		+= sm501.o
> >>  obj-$(CONFIG_ARCH_BCM2835)	+= bcm2835-pm.o
> >>  obj-$(CONFIG_MFD_BCM590XX)	+= bcm590xx.o
> >>  obj-$(CONFIG_MFD_BD9571MWV)	+= bd9571mwv.o
> >> +obj-$(CONFIG_MFD_CGBC)		+= cgbc-core.o
> >>  obj-$(CONFIG_MFD_CROS_EC_DEV)	+= cros_ec_dev.o
> >>  obj-$(CONFIG_MFD_CS42L43)	+= cs42l43.o
> >>  obj-$(CONFIG_MFD_CS42L43_I2C)	+= cs42l43-i2c.o
> >> diff --git a/drivers/mfd/cgbc-core.c b/drivers/mfd/cgbc-core.c
> >> new file mode 100644
> >> index 000000000000..cca9b1170cc9
> >> --- /dev/null
> >> +++ b/drivers/mfd/cgbc-core.c
> >> @@ -0,0 +1,453 @@
> >> +// SPDX-License-Identifier: GPL-2.0-or-later
> >> +/*
> >> + * Congatec Board Controller MFD core driver.
> > 
> > No such thing as an MFD.
> 
> What should it be if it's not an MFD ?

You should be telling me this. :)

"Board Controller" according to the Kconfig entry.

> >> +static struct platform_device *cgbc_pdev;
> > 
> > No avoidable globals.
> 
> I don't see how can I have cgbc_pdev not global.

Because of the incestuous, "I am my own parent", thing?  Okay.

> >> +static void cgbc_session_release(struct cgbc_device_data *cgbc)
> >> +{
> >> +	if (cgbc_session_command(cgbc, cgbc->session) != cgbc->session)
> > 
> > What does this do?
> > 
> > Are we checking or doing something?  If the latter, then we shouldn't be
> > doing that in an if() statement.  If the former, then what's the point
> > of the function?
> 
> We try to close the session we opened with the device.
> We check that the session is closed correctly.

Please remove it from the if statement and store the result in variable
with suitable nomenclature please.

[...]

In future, please snip everything you do not wish to discuss.

> Thanks for the review !!

No problem.

-- 
Lee Jones [李琼斯]

Re: [PATCH 1/5] mfd: add Congatec Board Controller mfd driver

[Thomas Richard]

On 9/12/24 16:13, Lee Jones wrote:
> On Tue, 10 Sep 2024, Thomas Richard wrote:
> 
>> On 8/22/24 12:38, Lee Jones wrote:
>>> On Fri, 09 Aug 2024, Thomas Richard wrote:
>>>
>>>> Add core MFD driver for the Board Controller found on some Congatec SMARC
>>>> module. This Board Controller provides functions like watchdog, GPIO, and
>>>> I2C busses.
>>>>
>>>> This commit add support only for the conga-SA7 module.
>>>>
>>>> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
>>>> ---
>>>>  drivers/mfd/Kconfig      |  12 ++
>>>>  drivers/mfd/Makefile     |   1 +
>>>>  drivers/mfd/cgbc-core.c  | 453 +++++++++++++++++++++++++++++++++++++++++++++++
>>>>  include/linux/mfd/cgbc.h |  44 +++++
>>>>  4 files changed, 510 insertions(+)
>>>>
>>>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
>>>> index bc8be2e593b6..3e0530f30267 100644
>>>> --- a/drivers/mfd/Kconfig
>>>> +++ b/drivers/mfd/Kconfig
>>>> @@ -224,6 +224,18 @@ config MFD_AXP20X_RSB
>>>>  	  components like regulators or the PEK (Power Enable Key) under the
>>>>  	  corresponding menus.
>>>>  
>>>> +config MFD_CGBC
>>>> +	tristate "Congatec Board Controller"
>>>> +	select MFD_CORE
>>>> +	depends on X86
>>>> +	help
>>>> +	  This is the core driver of the Board Controller found on some Congatec
>>>> +	  SMARC modules. The Board Controller provides functions like watchdog,
>>>> +	  I2C busses, and GPIO controller.
>>>> +
>>>> +	  To compile this driver as a module, choose M here: the module will be
>>>> +	  called cgbc-core.
>>>> +
>>>>  config MFD_CROS_EC_DEV
>>>>  	tristate "ChromeOS Embedded Controller multifunction device"
>>>>  	select MFD_CORE
>>>> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
>>>> index 02b651cd7535..d5da3fcd691c 100644
>>>> --- a/drivers/mfd/Makefile
>>>> +++ b/drivers/mfd/Makefile
>>>> @@ -13,6 +13,7 @@ obj-$(CONFIG_MFD_SM501)		+= sm501.o
>>>>  obj-$(CONFIG_ARCH_BCM2835)	+= bcm2835-pm.o
>>>>  obj-$(CONFIG_MFD_BCM590XX)	+= bcm590xx.o
>>>>  obj-$(CONFIG_MFD_BD9571MWV)	+= bd9571mwv.o
>>>> +obj-$(CONFIG_MFD_CGBC)		+= cgbc-core.o
>>>>  obj-$(CONFIG_MFD_CROS_EC_DEV)	+= cros_ec_dev.o
>>>>  obj-$(CONFIG_MFD_CS42L43)	+= cs42l43.o
>>>>  obj-$(CONFIG_MFD_CS42L43_I2C)	+= cs42l43-i2c.o
>>>> diff --git a/drivers/mfd/cgbc-core.c b/drivers/mfd/cgbc-core.c
>>>> new file mode 100644
>>>> index 000000000000..cca9b1170cc9
>>>> --- /dev/null
>>>> +++ b/drivers/mfd/cgbc-core.c
>>>> @@ -0,0 +1,453 @@
>>>> +// SPDX-License-Identifier: GPL-2.0-or-later
>>>> +/*
>>>> + * Congatec Board Controller MFD core driver.
>>>
>>> No such thing as an MFD.
>>
>> What should it be if it's not an MFD ?
> 
> You should be telling me this. :)
> 
> "Board Controller" according to the Kconfig entry.
> 

This Congatec Board Controller is an external micro-controller that is
interfaced with the CPU through a eSPI bus.
This Board Controller provides multiple functions: an I2C controller, a
GPIO controller, a watchdog and other not yet implemented functions
(temp/voltage sensors, backlight).

Therefore, the MFD subsystem is a very good fit, as it allows to have a
core driver that implements the communication with the external
micro-controller, and then individual drivers for each of the functions
offered by this Board Controller.

Thomas

Re: [PATCH 1/5] mfd: add Congatec Board Controller mfd driver

[Lee Jones]

On Fri, 13 Sep 2024, Thomas Richard wrote:

> On 9/12/24 16:13, Lee Jones wrote:
> > On Tue, 10 Sep 2024, Thomas Richard wrote:
> > 
> >> On 8/22/24 12:38, Lee Jones wrote:
> >>> On Fri, 09 Aug 2024, Thomas Richard wrote:
> >>>
> >>>> Add core MFD driver for the Board Controller found on some Congatec SMARC
> >>>> module. This Board Controller provides functions like watchdog, GPIO, and
> >>>> I2C busses.
> >>>>
> >>>> This commit add support only for the conga-SA7 module.
> >>>>
> >>>> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
> >>>> ---
> >>>>  drivers/mfd/Kconfig      |  12 ++
> >>>>  drivers/mfd/Makefile     |   1 +
> >>>>  drivers/mfd/cgbc-core.c  | 453 +++++++++++++++++++++++++++++++++++++++++++++++
> >>>>  include/linux/mfd/cgbc.h |  44 +++++
> >>>>  4 files changed, 510 insertions(+)
> >>>>
> >>>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> >>>> index bc8be2e593b6..3e0530f30267 100644
> >>>> --- a/drivers/mfd/Kconfig
> >>>> +++ b/drivers/mfd/Kconfig
> >>>> @@ -224,6 +224,18 @@ config MFD_AXP20X_RSB
> >>>>  	  components like regulators or the PEK (Power Enable Key) under the
> >>>>  	  corresponding menus.
> >>>>  
> >>>> +config MFD_CGBC
> >>>> +	tristate "Congatec Board Controller"
> >>>> +	select MFD_CORE
> >>>> +	depends on X86
> >>>> +	help
> >>>> +	  This is the core driver of the Board Controller found on some Congatec
> >>>> +	  SMARC modules. The Board Controller provides functions like watchdog,
> >>>> +	  I2C busses, and GPIO controller.
> >>>> +
> >>>> +	  To compile this driver as a module, choose M here: the module will be
> >>>> +	  called cgbc-core.
> >>>> +
> >>>>  config MFD_CROS_EC_DEV
> >>>>  	tristate "ChromeOS Embedded Controller multifunction device"
> >>>>  	select MFD_CORE
> >>>> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> >>>> index 02b651cd7535..d5da3fcd691c 100644
> >>>> --- a/drivers/mfd/Makefile
> >>>> +++ b/drivers/mfd/Makefile
> >>>> @@ -13,6 +13,7 @@ obj-$(CONFIG_MFD_SM501)		+= sm501.o
> >>>>  obj-$(CONFIG_ARCH_BCM2835)	+= bcm2835-pm.o
> >>>>  obj-$(CONFIG_MFD_BCM590XX)	+= bcm590xx.o
> >>>>  obj-$(CONFIG_MFD_BD9571MWV)	+= bd9571mwv.o
> >>>> +obj-$(CONFIG_MFD_CGBC)		+= cgbc-core.o
> >>>>  obj-$(CONFIG_MFD_CROS_EC_DEV)	+= cros_ec_dev.o
> >>>>  obj-$(CONFIG_MFD_CS42L43)	+= cs42l43.o
> >>>>  obj-$(CONFIG_MFD_CS42L43_I2C)	+= cs42l43-i2c.o
> >>>> diff --git a/drivers/mfd/cgbc-core.c b/drivers/mfd/cgbc-core.c
> >>>> new file mode 100644
> >>>> index 000000000000..cca9b1170cc9
> >>>> --- /dev/null
> >>>> +++ b/drivers/mfd/cgbc-core.c
> >>>> @@ -0,0 +1,453 @@
> >>>> +// SPDX-License-Identifier: GPL-2.0-or-later
> >>>> +/*
> >>>> + * Congatec Board Controller MFD core driver.
> >>>
> >>> No such thing as an MFD.
> >>
> >> What should it be if it's not an MFD ?
> > 
> > You should be telling me this. :)
> > 
> > "Board Controller" according to the Kconfig entry.
> > 
> 
> This Congatec Board Controller is an external micro-controller that is
> interfaced with the CPU through a eSPI bus.
> This Board Controller provides multiple functions: an I2C controller, a
> GPIO controller, a watchdog and other not yet implemented functions
> (temp/voltage sensors, backlight).
> 
> Therefore, the MFD subsystem is a very good fit, as it allows to have a
> core driver that implements the communication with the external
> micro-controller, and then individual drivers for each of the functions
> offered by this Board Controller.

Sorry for the ambiguity, that was not the point of the review comment.

Please remove all mentions to MFD in the help/header text.

-- 
Lee Jones [李琼斯]