[PATCH 0/5] Add support for the AAEON UP board FPGA

[PATCH 0/5] Add support for the AAEON UP board FPGA

[Thomas Richard]

This is the revival of the AAEON UP boards FPGA.
The latest version was sent one year ago [1].

I started a new series, as I re-wrote all the drivers using the AAEON
DKMS driver [2] as reference. I also read all series sent to the ML to
collect the comments sent by the maintainers and reviewers (to not
reproduce the same errors).

The FPGA acts as a level shifter between the Intel SoC and the HAT interfaces.
The pinctrl driver shall manage FPGA pins in tandem with the corresponding
Intel SoC pins. In the previous series [2], this was done using Intel pinctrl
code (included in the FPGA pinctrl driver).
Now this code was removed and gpiolib is used to drive the SoC pins.

The FPGA also drives some LEDs.

This series adds support for UP and UP Squared boards.

More informations (like the HAT mapping) are available on the UP community
wiki [3].

[1] https://lore.kernel.org/all/20231031015119.29756-1-larry.lai@yunjingtech.com/
[2] https://github.com/up-division/pinctrl-upboard
[3] https://github.com/up-board/up-community/wiki

Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
Thomas Richard (5):
      mfd: Add support for AAEON UP board FPGA
      leds: Add AAEON UP board LED driver
      gpiolib: add gpiochip_add_pinlist_range() function
      pinctrl: Add pin controller driver for AAEON UP boards
      MAINTAINERS: Add entry for AAEON UP board FPGA drivers

 MAINTAINERS                       |    8 +
 drivers/gpio/gpiolib.c            |   74 ++-
 drivers/leds/Kconfig              |    9 +
 drivers/leds/Makefile             |    1 +
 drivers/leds/leds-upboard.c       |  126 +++++
 drivers/mfd/Kconfig               |   12 +
 drivers/mfd/Makefile              |    2 +
 drivers/mfd/upboard-fpga.c        |  325 +++++++++++
 drivers/pinctrl/Kconfig           |   14 +
 drivers/pinctrl/Makefile          |    1 +
 drivers/pinctrl/pinctrl-upboard.c | 1090 +++++++++++++++++++++++++++++++++++++
 include/linux/gpio/driver.h       |   12 +
 include/linux/mfd/upboard-fpga.h  |   55 ++
 13 files changed, 1709 insertions(+), 20 deletions(-)
---
base-commit: cbc4912199deab59fdbd830b115d81941d0add46
change-id: 20240930-aaeon-up-board-pinctrl-support-98fa4a030490

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

[PATCH 1/5] mfd: Add support for AAEON UP board FPGA

[Thomas Richard]

The UP boards implement some features (pin controller, LEDs) through an
on-board FPGA.

This MFD driver implements the line protocol to communicate with the FPGA
through regmap, and registers pin controller and led cells.

This commit adds support for UP and UP Squared boards.

Based on the work done by Gary Wang <garywang@aaeon.com.tw>.

Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
 drivers/mfd/Kconfig              |  12 ++
 drivers/mfd/Makefile             |   2 +
 drivers/mfd/upboard-fpga.c       | 325 +++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/upboard-fpga.h |  55 +++++++
 4 files changed, 394 insertions(+)

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index ae23b317a64e..3fc8ddac68bf 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -2414,5 +2414,17 @@ config MFD_RSMU_SPI
 	  Additional drivers must be enabled in order to use the functionality
 	  of the device.
 
+config MFD_UPBOARD_FPGA
+	tristate "Support for the AAeon UP board FPGA"
+	depends on (X86 && ACPI)
+	select MFD_CORE
+	help
+	  Select this option to enable the AAEON UP and UP^2 onboard FPGA.
+	  This is the core driver of this FPGA, which has a pin controller and a
+	  LED controller.
+
+	  To compile this driver as a module, choose M here: the module will be
+	  called upboard-fpga.
+
 endmenu
 endif
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index e057d6d6faef..6ce5bbdf078f 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -290,3 +290,5 @@ obj-$(CONFIG_MFD_ATC260X_I2C)	+= atc260x-i2c.o
 
 obj-$(CONFIG_MFD_RSMU_I2C)	+= rsmu_i2c.o rsmu_core.o
 obj-$(CONFIG_MFD_RSMU_SPI)	+= rsmu_spi.o rsmu_core.o
+
+obj-$(CONFIG_MFD_UPBOARD_FPGA)	+= upboard-fpga.o
diff --git a/drivers/mfd/upboard-fpga.c b/drivers/mfd/upboard-fpga.c
new file mode 100644
index 000000000000..5a330e2f2229
--- /dev/null
+++ b/drivers/mfd/upboard-fpga.c
@@ -0,0 +1,325 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * UP Board FPGA driver.
+ *
+ * FPGA provides more GPIO driving power, LEDS and pin mux function.
+ *
+ * Copyright (c) AAEON. All rights reserved.
+ * Copyright (C) 2024 Bootlin
+ *
+ * Author: Gary Wang <garywang@aaeon.com.tw>
+ * Author: Thomas Richard <thomas.richard@bootlin.com>
+ */
+
+#include <linux/acpi.h>
+#include <linux/bitfield.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/gpio/consumer.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/upboard-fpga.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+#include <linux/sysfs.h>
+
+#define UPBOARD_AAEON_MANUFACTURER_ID	0x01
+#define UPBOARD_MANUFACTURER_ID_MASK	GENMASK(7, 0)
+
+#define UPBOARD_ADDRESS_SIZE  7
+#define UPBOARD_REGISTER_SIZE 16
+
+#define UPBOARD_READ_FLAG     BIT(UPBOARD_ADDRESS_SIZE)
+
+#define UPBOARD_FW_ID_MAJOR_SUPPORTED	0x0
+
+#define UPBOARD_FW_ID_BUILD_MASK	GENMASK(15, 12)
+#define UPBOARD_FW_ID_MAJOR_MASK	GENMASK(11, 8)
+#define UPBOARD_FW_ID_MINOR_MASK	GENMASK(7, 4)
+#define UPBOARD_FW_ID_PATCH_MASK	GENMASK(3, 0)
+
+static int upboard_fpga_read(void *context, unsigned int reg, unsigned int *val)
+{
+	struct upboard_fpga *fpga = context;
+	int i;
+
+	/* Clear to start new transaction */
+	gpiod_set_value(fpga->clear_gpio, 0);
+	gpiod_set_value(fpga->clear_gpio, 1);
+
+	reg |= UPBOARD_READ_FLAG;
+
+	/* Send clock and addr from strobe & datain pins */
+	for (i = UPBOARD_ADDRESS_SIZE; i >= 0; i--) {
+		gpiod_set_value(fpga->strobe_gpio, 0);
+		gpiod_set_value(fpga->datain_gpio, !!(reg & BIT(i)));
+		gpiod_set_value(fpga->strobe_gpio, 1);
+	}
+
+	gpiod_set_value(fpga->strobe_gpio, 0);
+	*val = 0;
+
+	/* Read data from dataout pin */
+	for (i = UPBOARD_REGISTER_SIZE - 1; i >= 0; i--) {
+		gpiod_set_value(fpga->strobe_gpio, 1);
+		gpiod_set_value(fpga->strobe_gpio, 0);
+		*val |= gpiod_get_value(fpga->dataout_gpio) << i;
+	}
+
+	gpiod_set_value(fpga->strobe_gpio, 1);
+
+	return 0;
+}
+
+static int upboard_fpga_write(void *context, unsigned int reg, unsigned int val)
+{
+	struct upboard_fpga *fpga = context;
+	int i;
+
+	/* Clear to start new transcation */
+	gpiod_set_value(fpga->clear_gpio, 0);
+	gpiod_set_value(fpga->clear_gpio, 1);
+
+	/* Send clock and addr from strobe & datain pins */
+	for (i = UPBOARD_ADDRESS_SIZE; i >= 0; i--) {
+		gpiod_set_value(fpga->strobe_gpio, 0);
+		gpiod_set_value(fpga->datain_gpio, !!(reg & BIT(i)));
+		gpiod_set_value(fpga->strobe_gpio, 1);
+	}
+
+	gpiod_set_value(fpga->strobe_gpio, 0);
+
+	/* Write data to datain pin */
+	for (i = UPBOARD_REGISTER_SIZE - 1; i >= 0; i--) {
+		gpiod_set_value(fpga->datain_gpio, !!(val & BIT(i)));
+		gpiod_set_value(fpga->strobe_gpio, 1);
+		gpiod_set_value(fpga->strobe_gpio, 0);
+	}
+
+	gpiod_set_value(fpga->strobe_gpio, 1);
+
+	return 0;
+}
+
+static const struct regmap_range upboard_up_readable_ranges[] = {
+	regmap_reg_range(UPBOARD_REG_PLATFORM_ID, UPBOARD_REG_FIRMWARE_ID),
+	regmap_reg_range(UPBOARD_REG_FUNC_EN0, UPBOARD_REG_FUNC_EN0),
+	regmap_reg_range(UPBOARD_REG_GPIO_EN0, UPBOARD_REG_GPIO_EN1),
+	regmap_reg_range(UPBOARD_REG_GPIO_DIR0, UPBOARD_REG_GPIO_DIR1),
+};
+
+static const struct regmap_range upboard_up_writable_ranges[] = {
+	regmap_reg_range(UPBOARD_REG_FUNC_EN0, UPBOARD_REG_FUNC_EN0),
+	regmap_reg_range(UPBOARD_REG_GPIO_EN0, UPBOARD_REG_GPIO_EN1),
+	regmap_reg_range(UPBOARD_REG_GPIO_DIR0, UPBOARD_REG_GPIO_DIR1),
+};
+
+static const struct regmap_access_table upboard_up_readable_table = {
+	.yes_ranges = upboard_up_readable_ranges,
+	.n_yes_ranges = ARRAY_SIZE(upboard_up_readable_ranges),
+};
+
+static const struct regmap_access_table upboard_up_writable_table = {
+	.yes_ranges = upboard_up_writable_ranges,
+	.n_yes_ranges = ARRAY_SIZE(upboard_up_writable_ranges),
+};
+
+static const struct regmap_config upboard_up_regmap_config = {
+	.reg_bits = UPBOARD_ADDRESS_SIZE,
+	.val_bits = UPBOARD_REGISTER_SIZE,
+	.max_register = UPBOARD_REG_MAX,
+	.reg_read = upboard_fpga_read,
+	.reg_write = upboard_fpga_write,
+	.fast_io = false,
+	.cache_type = REGCACHE_NONE,
+	.rd_table = &upboard_up_readable_table,
+	.wr_table = &upboard_up_writable_table,
+};
+
+static const struct regmap_range upboard_up2_readable_ranges[] = {
+	regmap_reg_range(UPBOARD_REG_PLATFORM_ID, UPBOARD_REG_FIRMWARE_ID),
+	regmap_reg_range(UPBOARD_REG_FUNC_EN0, UPBOARD_REG_FUNC_EN1),
+	regmap_reg_range(UPBOARD_REG_GPIO_EN0, UPBOARD_REG_GPIO_EN2),
+	regmap_reg_range(UPBOARD_REG_GPIO_DIR0, UPBOARD_REG_GPIO_DIR2),
+};
+
+static const struct regmap_range upboard_up2_writable_ranges[] = {
+	regmap_reg_range(UPBOARD_REG_FUNC_EN0, UPBOARD_REG_FUNC_EN1),
+	regmap_reg_range(UPBOARD_REG_GPIO_EN0, UPBOARD_REG_GPIO_EN2),
+	regmap_reg_range(UPBOARD_REG_GPIO_DIR0, UPBOARD_REG_GPIO_DIR2),
+};
+
+static const struct regmap_access_table upboard_up2_readable_table = {
+	.yes_ranges = upboard_up2_readable_ranges,
+	.n_yes_ranges = ARRAY_SIZE(upboard_up2_readable_ranges),
+};
+
+static const struct regmap_access_table upboard_up2_writable_table = {
+	.yes_ranges = upboard_up2_writable_ranges,
+	.n_yes_ranges = ARRAY_SIZE(upboard_up2_writable_ranges),
+};
+
+static const struct regmap_config upboard_up2_regmap_config = {
+	.reg_bits = UPBOARD_ADDRESS_SIZE,
+	.val_bits = UPBOARD_REGISTER_SIZE,
+	.max_register = UPBOARD_REG_MAX,
+	.reg_read = upboard_fpga_read,
+	.reg_write = upboard_fpga_write,
+	.fast_io = false,
+	.cache_type = REGCACHE_NONE,
+	.rd_table = &upboard_up2_readable_table,
+	.wr_table = &upboard_up2_writable_table,
+};
+
+static const struct mfd_cell upboard_up_mfd_cells[] = {
+	{ .name = "upboard-pinctrl" },
+	{ .name = "upboard-leds" },
+};
+
+static const struct upboard_fpga_data upboard_up_fpga_data = {
+	.type = UPBOARD_UP_FPGA,
+	.regmap_config = &upboard_up_regmap_config,
+};
+
+static const struct upboard_fpga_data upboard_up2_fpga_data = {
+	.type = UPBOARD_UP2_FPGA,
+	.regmap_config = &upboard_up2_regmap_config,
+};
+
+static int upboard_fpga_gpio_init(struct upboard_fpga *fpga)
+{
+	fpga->enable_gpio = devm_gpiod_get(fpga->dev, "enable", GPIOD_ASIS);
+	if (IS_ERR(fpga->enable_gpio))
+		return PTR_ERR(fpga->enable_gpio);
+
+	fpga->clear_gpio = devm_gpiod_get(fpga->dev, "clear", GPIOD_OUT_LOW);
+	if (IS_ERR(fpga->clear_gpio))
+		return PTR_ERR(fpga->clear_gpio);
+
+	fpga->strobe_gpio = devm_gpiod_get(fpga->dev, "strobe", GPIOD_OUT_LOW);
+	if (IS_ERR(fpga->strobe_gpio))
+		return PTR_ERR(fpga->strobe_gpio);
+
+	fpga->datain_gpio = devm_gpiod_get(fpga->dev, "datain", GPIOD_OUT_LOW);
+	if (IS_ERR(fpga->datain_gpio))
+		return PTR_ERR(fpga->datain_gpio);
+
+	fpga->dataout_gpio = devm_gpiod_get(fpga->dev, "dataout", GPIOD_IN);
+	if (IS_ERR(fpga->dataout_gpio))
+		return PTR_ERR(fpga->dataout_gpio);
+
+	gpiod_set_value(fpga->enable_gpio, 1);
+
+	return 0;
+}
+
+static int upboard_fpga_get_firmware_version(struct upboard_fpga *fpga)
+{
+	unsigned int platform_id, manufacturer_id;
+	int ret;
+
+	if (!fpga)
+		return -ENOMEM;
+
+	ret = regmap_read(fpga->regmap, UPBOARD_REG_PLATFORM_ID, &platform_id);
+	if (ret)
+		return ret;
+
+	manufacturer_id = platform_id & UPBOARD_MANUFACTURER_ID_MASK;
+	if (manufacturer_id != UPBOARD_AAEON_MANUFACTURER_ID)
+		return dev_err_probe(fpga->dev, -ENODEV,
+				     "driver not compatible with custom FPGA FW from manufacturer id %#02x.",
+				     manufacturer_id);
+
+	ret = regmap_read(fpga->regmap, UPBOARD_REG_FIRMWARE_ID, &fpga->firmware_version);
+	if (ret)
+		return ret;
+
+	if (FIELD_GET(UPBOARD_FW_ID_MAJOR_MASK, fpga->firmware_version) !=
+	    UPBOARD_FW_ID_MAJOR_SUPPORTED)
+		return dev_err_probe(fpga->dev, -ENODEV,
+				     "unsupported FPGA FW v%lu.%lu.%lu build %#02lx",
+				     FIELD_GET(UPBOARD_FW_ID_MAJOR_MASK, fpga->firmware_version),
+				     FIELD_GET(UPBOARD_FW_ID_MINOR_MASK, fpga->firmware_version),
+				     FIELD_GET(UPBOARD_FW_ID_PATCH_MASK, fpga->firmware_version),
+				     FIELD_GET(UPBOARD_FW_ID_BUILD_MASK, fpga->firmware_version));
+	return 0;
+}
+
+static ssize_t upboard_fpga_version_show(struct device *dev, struct device_attribute *attr,
+					 char *buf)
+{
+	struct upboard_fpga *fpga = dev_get_drvdata(dev);
+
+	return sysfs_emit(buf, "FPGA FW v%lu.%lu.%lu build %#02lx\n",
+			  FIELD_GET(UPBOARD_FW_ID_MAJOR_MASK, fpga->firmware_version),
+			  FIELD_GET(UPBOARD_FW_ID_MINOR_MASK, fpga->firmware_version),
+			  FIELD_GET(UPBOARD_FW_ID_PATCH_MASK, fpga->firmware_version),
+			  FIELD_GET(UPBOARD_FW_ID_BUILD_MASK, fpga->firmware_version));
+}
+
+static DEVICE_ATTR_RO(upboard_fpga_version);
+
+static struct attribute *upboard_fpga_attrs[] = {
+	&dev_attr_upboard_fpga_version.attr,
+	NULL
+};
+
+ATTRIBUTE_GROUPS(upboard_fpga);
+
+static int upboard_fpga_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct upboard_fpga *fpga;
+	int ret;
+
+	fpga = devm_kzalloc(dev, sizeof(*fpga), GFP_KERNEL);
+	if (!fpga)
+		return -ENOMEM;
+
+	fpga->fpga_data = device_get_match_data(dev);
+
+	fpga->dev = dev;
+
+	platform_set_drvdata(pdev, fpga);
+
+	fpga->regmap = devm_regmap_init(dev, NULL, fpga, fpga->fpga_data->regmap_config);
+	if (IS_ERR(fpga->regmap))
+		return PTR_ERR(fpga->regmap);
+
+	ret = upboard_fpga_gpio_init(fpga);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to initialize FPGA common GPIOs");
+
+	ret = upboard_fpga_get_firmware_version(fpga);
+	if (ret)
+		return ret;
+
+	return devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE, upboard_up_mfd_cells,
+				    ARRAY_SIZE(upboard_up_mfd_cells), NULL, 0, NULL);
+}
+
+static const struct acpi_device_id upboard_fpga_acpi_match[] = {
+	{ "AANT0F01", (kernel_ulong_t)&upboard_up2_fpga_data },
+	{ "AANT0F04", (kernel_ulong_t)&upboard_up_fpga_data },
+	{}
+};
+MODULE_DEVICE_TABLE(acpi, upboard_fpga_acpi_match);
+
+static struct platform_driver upboard_fpga_driver = {
+	.driver = {
+		.name = "upboard-fpga",
+		.acpi_match_table = ACPI_PTR(upboard_fpga_acpi_match),
+		.dev_groups	= upboard_fpga_groups,
+	},
+	.probe = upboard_fpga_probe,
+};
+
+module_platform_driver(upboard_fpga_driver);
+
+MODULE_AUTHOR("Gary Wang <garywang@aaeon.com.tw>");
+MODULE_AUTHOR("Thomas Richard <thomas.richard@bootlin.com>");
+MODULE_DESCRIPTION("UP Board FPGA driver");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/mfd/upboard-fpga.h b/include/linux/mfd/upboard-fpga.h
new file mode 100644
index 000000000000..12231e40f5da
--- /dev/null
+++ b/include/linux/mfd/upboard-fpga.h
@@ -0,0 +1,55 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * UP Board CPLD/FPGA driver
+ *
+ * Copyright (c) AAEON. All rights reserved.
+ * Copyright (C) 2024 Bootlin
+ *
+ * Author: Gary Wang <garywang@aaeon.com.tw>
+ * Author: Thomas Richard <thomas.richard@bootlin.com>
+ *
+ */
+
+#ifndef __LINUX_MFD_UPBOARD_FPGA_H
+#define __LINUX_MFD_UPBOARD_FPGA_H
+
+#define UPBOARD_REGISTER_SIZE 16
+
+enum upboard_fpgareg {
+	UPBOARD_REG_PLATFORM_ID   = 0x10,
+	UPBOARD_REG_FIRMWARE_ID   = 0x11,
+	UPBOARD_REG_FUNC_EN0      = 0x20,
+	UPBOARD_REG_FUNC_EN1      = 0x21,
+	UPBOARD_REG_GPIO_EN0      = 0x30,
+	UPBOARD_REG_GPIO_EN1      = 0x31,
+	UPBOARD_REG_GPIO_EN2      = 0x32,
+	UPBOARD_REG_GPIO_DIR0     = 0x40,
+	UPBOARD_REG_GPIO_DIR1     = 0x41,
+	UPBOARD_REG_GPIO_DIR2     = 0x42,
+	UPBOARD_REG_MAX,
+};
+
+enum upboard_fpga_type {
+	UPBOARD_UP_FPGA,
+	UPBOARD_UP2_FPGA,
+};
+
+struct upboard_fpga_data {
+	enum upboard_fpga_type type;
+	const struct regmap_config *regmap_config;
+};
+
+struct upboard_fpga {
+	struct device *dev;
+	struct regmap *regmap;
+	struct gpio_desc *enable_gpio;
+	struct gpio_desc *reset_gpio;
+	struct gpio_desc *clear_gpio;
+	struct gpio_desc *strobe_gpio;
+	struct gpio_desc *datain_gpio;
+	struct gpio_desc *dataout_gpio;
+	unsigned int firmware_version;
+	const struct upboard_fpga_data *fpga_data;
+};
+
+#endif /*  __LINUX_MFD_UPBOARD_FPGA_H */

-- 
2.39.5

[PATCH 2/5] leds: Add AAEON UP board LED driver

[Thomas Richard]

Add support for LEDs on AAEON UP boards. These leds are managed by the
onboard FPGA:
- UP boards: yellow, green, red
- UP Squared boards: blue, yellow, green, red

Based on the work done by Gary Wang <garywang@aaeon.com.tw>, largely
rewritten.

Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
 drivers/leds/Kconfig        |   9 ++++
 drivers/leds/Makefile       |   1 +
 drivers/leds/leds-upboard.c | 126 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 136 insertions(+)

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index b784bb74a837..984ef5cfa9fa 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -815,6 +815,15 @@ config LEDS_SC27XX_BLTC
 	  This driver can also be built as a module. If so the module will be
 	  called leds-sc27xx-bltc.
 
+config LEDS_UPBOARD
+	tristate "LED support for the UP board"
+	depends on LEDS_CLASS && MFD_UPBOARD_FPGA
+	help
+	  This option enables support for the UP board LEDs.
+
+	  This driver can also be built as a module. If so the module will be
+	  called leds-upboard.
+
 comment "LED driver for blink(1) USB RGB LED is under Special HID drivers (HID_THINGM)"
 
 config LEDS_BLINKM
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 18afbb5a23ee..e7982938ddc1 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -89,6 +89,7 @@ obj-$(CONFIG_LEDS_TI_LMU_COMMON)	+= leds-ti-lmu-common.o
 obj-$(CONFIG_LEDS_TLC591XX)		+= leds-tlc591xx.o
 obj-$(CONFIG_LEDS_TPS6105X)		+= leds-tps6105x.o
 obj-$(CONFIG_LEDS_TURRIS_OMNIA)		+= leds-turris-omnia.o
+obj-$(CONFIG_LEDS_UPBOARD)		+= leds-upboard.o
 obj-$(CONFIG_LEDS_WM831X_STATUS)	+= leds-wm831x-status.o
 obj-$(CONFIG_LEDS_WM8350)		+= leds-wm8350.o
 obj-$(CONFIG_LEDS_WRAP)			+= leds-wrap.o
diff --git a/drivers/leds/leds-upboard.c b/drivers/leds/leds-upboard.c
new file mode 100644
index 000000000000..b350eb294280
--- /dev/null
+++ b/drivers/leds/leds-upboard.c
@@ -0,0 +1,126 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * UP board LED driver.
+ *
+ * Copyright (c) AAEON. All rights reserved.
+ * Copyright (C) 2024 Bootlin
+ *
+ * Author: Gary Wang <garywang@aaeon.com.tw>
+ * Author: Thomas Richard <thomas.richard@bootlin.com>
+ */
+
+#include <linux/device.h>
+#include <linux/container_of.h>
+#include <linux/leds.h>
+#include <linux/mfd/upboard-fpga.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#define led_cdev_to_led_upboard(c)	container_of(c, struct upboard_led, cdev)
+
+struct upboard_led {
+	struct regmap_field *field;
+	struct led_classdev cdev;
+};
+
+struct upboard_led_profile {
+	const char *name;
+	unsigned int bit;
+};
+
+static struct upboard_led_profile upboard_up_led_profile[] = {
+	{ "upboard:yellow:" LED_FUNCTION_STATUS, 0 },
+	{ "upboard:green:" LED_FUNCTION_STATUS, 1 },
+	{ "upboard:red:" LED_FUNCTION_STATUS, 2 },
+};
+
+static struct upboard_led_profile upboard_up2_led_profile[] = {
+	{ "upboard:blue:" LED_FUNCTION_STATUS, 0 },
+	{ "upboard:yellow:" LED_FUNCTION_STATUS, 1 },
+	{ "upboard:green:" LED_FUNCTION_STATUS, 2 },
+	{ "upboard:red:" LED_FUNCTION_STATUS, 3 },
+};
+
+static enum led_brightness upboard_led_brightness_get(struct led_classdev *cdev)
+{
+	struct upboard_led *led = led_cdev_to_led_upboard(cdev);
+	int brightness, ret;
+
+	ret = regmap_field_read(led->field, &brightness);
+
+	return ret ? LED_OFF : brightness;
+};
+
+static int upboard_led_brightness_set(struct led_classdev *cdev, enum led_brightness brightness)
+{
+	struct upboard_led *led = led_cdev_to_led_upboard(cdev);
+
+	return regmap_field_write(led->field, brightness != LED_OFF);
+};
+
+static int upboard_led_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct upboard_fpga *fpga = dev_get_drvdata(dev->parent);
+	struct upboard_led_profile *led_profile;
+	struct upboard_led *led;
+	int led_instances, ret, i;
+
+	switch (fpga->fpga_data->type) {
+	case UPBOARD_UP_FPGA:
+		led_profile = upboard_up_led_profile;
+		led_instances = ARRAY_SIZE(upboard_up_led_profile);
+		break;
+	case UPBOARD_UP2_FPGA:
+		led_profile = upboard_up2_led_profile;
+		led_instances = ARRAY_SIZE(upboard_up2_led_profile);
+		break;
+	default:
+		return dev_err_probe(dev, -EINVAL, "Unknown device type %d\n",
+				     fpga->fpga_data->type);
+	}
+
+	for (i = 0; i < led_instances; i++) {
+		const struct reg_field fldconf = {
+			.reg = UPBOARD_REG_FUNC_EN0,
+			.lsb = led_profile[i].bit,
+			.msb = led_profile[i].bit,
+		};
+
+		led = devm_kzalloc(dev, sizeof(*led), GFP_KERNEL);
+		if (!led)
+			return -ENOMEM;
+
+		led->field = devm_regmap_field_alloc(&pdev->dev, fpga->regmap, fldconf);
+		if (IS_ERR(led->field))
+			return PTR_ERR(led->field);
+
+		led->cdev.brightness_get = upboard_led_brightness_get;
+		led->cdev.brightness_set_blocking = upboard_led_brightness_set;
+		led->cdev.max_brightness = LED_ON;
+
+		led->cdev.name = led_profile[i].name;
+
+		ret = devm_led_classdev_register(dev, &led->cdev);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static struct platform_driver upboard_led_driver = {
+	.driver = {
+		.name = "upboard-leds",
+	},
+	.probe = upboard_led_probe,
+};
+
+module_platform_driver(upboard_led_driver);
+
+MODULE_AUTHOR("Gary Wang <garywang@aaeon.com.tw>");
+MODULE_AUTHOR("Thomas Richard <thomas.richard@bootlin.com>");
+MODULE_DESCRIPTION("UP Board LED driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:upboard-led");

-- 
2.39.5

[PATCH 3/5] gpiolib: add gpiochip_add_pinlist_range() function

[Thomas Richard]

Add gpiochip_add_pinlist_range() function to add a range for GPIO <-> pin
mapping, using a list of non consecutive pins.
Previously, it was only possible to add range of consecutive pins using
gpiochip_add_pin_range().

The struct pinctrl_gpio_range has a 'pins' member which allows to set a
list of pins (which can be non consecutive). gpiochip_add_pinlist_range()
is identical to gpiochip_add_pin_range(), except it set 'pins' member
instead of 'pin_base' member.

Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
 drivers/gpio/gpiolib.c      | 74 +++++++++++++++++++++++++++++++++------------
 include/linux/gpio/driver.h | 12 ++++++++
 2 files changed, 66 insertions(+), 20 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 3c8ec7f3a83b..e0fe07167e62 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2227,26 +2227,9 @@ int gpiochip_add_pingroup_range(struct gpio_chip *gc,
 }
 EXPORT_SYMBOL_GPL(gpiochip_add_pingroup_range);
 
-/**
- * gpiochip_add_pin_range() - add a range for GPIO <-> pin mapping
- * @gc: the gpiochip to add the range for
- * @pinctl_name: the dev_name() of the pin controller to map to
- * @gpio_offset: the start offset in the current gpio_chip number space
- * @pin_offset: the start offset in the pin controller number space
- * @npins: the number of pins from the offset of each pin space (GPIO and
- *	pin controller) to accumulate in this range
- *
- * Calling this function directly from a DeviceTree-supported
- * pinctrl driver is DEPRECATED. Please see Section 2.1 of
- * Documentation/devicetree/bindings/gpio/gpio.txt on how to
- * bind pinctrl and gpio drivers via the "gpio-ranges" property.
- *
- * Returns:
- * 0 on success, or a negative errno on failure.
- */
-int gpiochip_add_pin_range(struct gpio_chip *gc, const char *pinctl_name,
-			   unsigned int gpio_offset, unsigned int pin_offset,
-			   unsigned int npins)
+static int __gpiochip_add_pin_range(struct gpio_chip *gc, const char *pinctl_name,
+				    unsigned int gpio_offset, unsigned int pin_offset,
+				    unsigned int const *pins, unsigned int npins)
 {
 	struct gpio_pin_range *pin_range;
 	struct gpio_device *gdev = gc->gpiodev;
@@ -2264,6 +2247,7 @@ int gpiochip_add_pin_range(struct gpio_chip *gc, const char *pinctl_name,
 	pin_range->range.name = gc->label;
 	pin_range->range.base = gdev->base + gpio_offset;
 	pin_range->range.pin_base = pin_offset;
+	pin_range->range.pins = pins;
 	pin_range->range.npins = npins;
 	pin_range->pctldev = pinctrl_find_and_add_gpio_range(pinctl_name,
 			&pin_range->range);
@@ -2282,8 +2266,58 @@ int gpiochip_add_pin_range(struct gpio_chip *gc, const char *pinctl_name,
 
 	return 0;
 }
+
+/**
+ * gpiochip_add_pin_range() - add a range for GPIO <-> pin mapping
+ * @gc: the gpiochip to add the range for
+ * @pinctl_name: the dev_name() of the pin controller to map to
+ * @gpio_offset: the start offset in the current gpio_chip number space
+ * @pin_offset: the start offset in the pin controller number space
+ * @npins: the number of pins from the offset of each pin space (GPIO and
+ *	pin controller) to accumulate in this range
+ *
+ * Calling this function directly from a DeviceTree-supported
+ * pinctrl driver is DEPRECATED. Please see Section 2.1 of
+ * Documentation/devicetree/bindings/gpio/gpio.txt on how to
+ * bind pinctrl and gpio drivers via the "gpio-ranges" property.
+ *
+ * Returns:
+ * 0 on success, or a negative errno on failure.
+ */
+int gpiochip_add_pin_range(struct gpio_chip *gc, const char *pinctl_name,
+			   unsigned int gpio_offset, unsigned int pin_offset,
+			   unsigned int npins)
+{
+	return __gpiochip_add_pin_range(gc, pinctl_name, gpio_offset,
+					pin_offset, NULL, npins);
+}
 EXPORT_SYMBOL_GPL(gpiochip_add_pin_range);
 
+/**
+ * gpiochip_add_pinlist_range() - add a range for GPIO <-> pin mapping
+ * @gc: the gpiochip to add the range for
+ * @pinctl_name: the dev_name() of the pin controller to map to
+ * @gpio_offset: the start offset in the current gpio_chip number space
+ * @pin_list: the list of pins to accumulate in this range
+ * @npins: the number of pins to accumulate in this range
+ *
+ * Calling this function directly from a DeviceTree-supported
+ * pinctrl driver is DEPRECATED. Please see Section 2.1 of
+ * Documentation/devicetree/bindings/gpio/gpio.txt on how to
+ * bind pinctrl and gpio drivers via the "gpio-ranges" property.
+ *
+ * Returns:
+ * 0 on success, or a negative errno on failure.
+ */
+int gpiochip_add_pinlist_range(struct gpio_chip *gc, const char *pinctl_name,
+			       unsigned int gpio_offset, unsigned int const *pins,
+			       unsigned int npins)
+{
+	return __gpiochip_add_pin_range(gc, pinctl_name, gpio_offset, 0, pins,
+					npins);
+}
+EXPORT_SYMBOL_GPL(gpiochip_add_pinlist_range);
+
 /**
  * gpiochip_remove_pin_ranges() - remove all the GPIO <-> pin mappings
  * @gc: the chip to remove all the mappings for
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index 2dd7cb9cc270..8037f9a1e2a9 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -751,6 +751,9 @@ struct gpio_pin_range {
 int gpiochip_add_pin_range(struct gpio_chip *gc, const char *pinctl_name,
 			   unsigned int gpio_offset, unsigned int pin_offset,
 			   unsigned int npins);
+int gpiochip_add_pinlist_range(struct gpio_chip *gc, const char *pinctl_name,
+			       unsigned int gpio_offset, unsigned int const *pins,
+			       unsigned int npins);
 int gpiochip_add_pingroup_range(struct gpio_chip *gc,
 			struct pinctrl_dev *pctldev,
 			unsigned int gpio_offset, const char *pin_group);
@@ -765,6 +768,15 @@ gpiochip_add_pin_range(struct gpio_chip *gc, const char *pinctl_name,
 {
 	return 0;
 }
+
+static inline int
+gpiochip_add_pinlist_range(struct gpio_chip *gc, const char *pinctl_name,
+			   unsigned int gpio_offset, unsigned int const *pins,
+			   unsigned int npins)
+{
+	return 0;
+}
+
 static inline int
 gpiochip_add_pingroup_range(struct gpio_chip *gc,
 			struct pinctrl_dev *pctldev,

-- 
2.39.5

Re: [PATCH 3/5] gpiolib: add gpiochip_add_pinlist_range() function

[Bartosz Golaszewski]

On Wed, Dec 11, 2024 at 5:27 PM Thomas Richard
<thomas.richard@bootlin.com> wrote:
>
> Add gpiochip_add_pinlist_range() function to add a range for GPIO <-> pin
> mapping, using a list of non consecutive pins.
> Previously, it was only possible to add range of consecutive pins using
> gpiochip_add_pin_range().
>
> The struct pinctrl_gpio_range has a 'pins' member which allows to set a
> list of pins (which can be non consecutive). gpiochip_add_pinlist_range()
> is identical to gpiochip_add_pin_range(), except it set 'pins' member
> instead of 'pin_base' member.
>
> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
> ---

I don't have anything against this change so in any case:

Acked-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

But may I suggest the name to be changed to
gpiochip_add_pin_range_sparse() for a better indication of its
purpose?

Bart

Re: [PATCH 3/5] gpiolib: add gpiochip_add_pinlist_range() function

[Thomas Richard]

On 12/16/24 10:17, Bartosz Golaszewski wrote:
> On Wed, Dec 11, 2024 at 5:27 PM Thomas Richard
> <thomas.richard@bootlin.com> wrote:
>>
>> Add gpiochip_add_pinlist_range() function to add a range for GPIO <-> pin
>> mapping, using a list of non consecutive pins.
>> Previously, it was only possible to add range of consecutive pins using
>> gpiochip_add_pin_range().
>>
>> The struct pinctrl_gpio_range has a 'pins' member which allows to set a
>> list of pins (which can be non consecutive). gpiochip_add_pinlist_range()
>> is identical to gpiochip_add_pin_range(), except it set 'pins' member
>> instead of 'pin_base' member.
>>
>> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
>> ---
> 
> I don't have anything against this change so in any case:
> 
> Acked-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> But may I suggest the name to be changed to
> gpiochip_add_pin_range_sparse() for a better indication of its
> purpose?

Hi Bartosz,

Yes for sure, I will change the name for the v2.

Regards,

Thomas

Re: [PATCH 3/5] gpiolib: add gpiochip_add_pinlist_range() function

[Linus Walleij]

Hi Thomas,

thanks for your patch!

On Wed, Dec 11, 2024 at 5:27 PM Thomas Richard
<thomas.richard@bootlin.com> wrote:

> Add gpiochip_add_pinlist_range() function to add a range for GPIO <-> pin
> mapping, using a list of non consecutive pins.
> Previously, it was only possible to add range of consecutive pins using
> gpiochip_add_pin_range().
>
> The struct pinctrl_gpio_range has a 'pins' member which allows to set a
> list of pins (which can be non consecutive). gpiochip_add_pinlist_range()
> is identical to gpiochip_add_pin_range(), except it set 'pins' member
> instead of 'pin_base' member.
>
> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>

(...)
> -int gpiochip_add_pin_range(struct gpio_chip *gc, const char *pinctl_name,
> -                          unsigned int gpio_offset, unsigned int pin_offset,
> -                          unsigned int npins)
> +static int __gpiochip_add_pin_range(struct gpio_chip *gc, const char *pinctl_name,
> +                                   unsigned int gpio_offset, unsigned int pin_offset,
> +                                   unsigned int const *pins, unsigned int npins)

Uh this looks messy and I'm not a fan, sadly.

Overall I dislike __inner_function() syntax, so use some name that
describe what is going on please, but I don't think we wanna do
this at all.

Instead of this hack that start to soften the boundary between GPIO
and pin control drivers, I think it is better to do what we often do
and move the whole GPIO driver over into the pin control driver
and have it all inside one single file, since I suspect the hardware
is supposed to be used as one single unit.

Please look at other combined GPIO+pin control drivers
for inspiration, such as:
pinctrl-stmfx.c
pinctrl-sx150x.c

those just access any registers they need as they are in one
driver, but still maintains the separation by just calling the
existing gpiochip_add_pin_range() and be done with it.

This should remove your need for the strange hacks like this
patch and the gpiochip wrapper in the pin control driver.

Yours,
Linus Walleij

[PATCH 4/5] pinctrl: Add pin controller driver for AAEON UP boards

[Thomas Richard]

This enables the pin control support of the onboard FPGA on AAEON UP
boards.
Due to the hardware design, the driver shall control its pins in tandem
with their corresponding Intel SoC GPIOs.

UP boards and UP Squared boards are supported.

Based on the work done by Gary Wang <garywang@aaeon.com.tw>, largely
rewritten.

Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
 drivers/pinctrl/Kconfig           |   14 +
 drivers/pinctrl/Makefile          |    1 +
 drivers/pinctrl/pinctrl-upboard.c | 1090 +++++++++++++++++++++++++++++++++++++
 3 files changed, 1105 insertions(+)

diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index 95a8e2b9a614..0289a0a6fb6d 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -576,6 +576,20 @@ config PINCTRL_TH1520
 	  This driver is needed for RISC-V development boards like
 	  the BeagleV Ahead and the LicheePi 4A.
 
+config PINCTRL_UPBOARD
+	tristate "AAeon UP board FPGA pin controller"
+	depends on MFD_UPBOARD_FPGA
+	select PINMUX
+	select GENERIC_PINCTRL_GROUPS
+	select GENERIC_PINMUX_FUNCTIONS
+	help
+	  Pin controller for the FPGA GPIO lines on UP boards. Due to the
+	  hardware layout, the driver control the FPGA pins in tandem with
+	  their corresponding Intel SoC GPIOs.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pinctrl-upboard.
+
 config PINCTRL_ZYNQ
 	bool "Pinctrl driver for Xilinx Zynq"
 	depends on ARCH_ZYNQ
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index fba1c56624c0..989b8d28ecac 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -56,6 +56,7 @@ obj-$(CONFIG_PINCTRL_SX150X)	+= pinctrl-sx150x.o
 obj-$(CONFIG_PINCTRL_TB10X)	+= pinctrl-tb10x.o
 obj-$(CONFIG_PINCTRL_TPS6594)	+= pinctrl-tps6594.o
 obj-$(CONFIG_PINCTRL_TH1520)	+= pinctrl-th1520.o
+obj-$(CONFIG_PINCTRL_UPBOARD)	+= pinctrl-upboard.o
 obj-$(CONFIG_PINCTRL_ZYNQMP)	+= pinctrl-zynqmp.o
 obj-$(CONFIG_PINCTRL_ZYNQ)	+= pinctrl-zynq.o
 
diff --git a/drivers/pinctrl/pinctrl-upboard.c b/drivers/pinctrl/pinctrl-upboard.c
new file mode 100644
index 000000000000..14bf64b9587d
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-upboard.c
@@ -0,0 +1,1090 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * UP board pin control driver.
+ *
+ * FPGA provides more GPIO driving power, LEDS and pin mux function.
+ *
+ * Copyright (c) AAEON. All rights reserved.
+ * Copyright (C) 2024 Bootlin
+ *
+ * Author: Gary Wang <garywang@aaeon.com.tw>
+ * Author: Thomas Richard <thomas.richard@bootlin.com>
+ */
+
+#include <linux/device.h>
+#include <linux/gpio/consumer.h>
+#include <linux/gpio/driver.h>
+#include <linux/mfd/upboard-fpga.h>
+#include <linux/module.h>
+#include <linux/pinctrl/consumer.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinmux.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/seq_file.h>
+
+#include "core.h"
+#include "pinmux.h"
+
+enum upboard_pin_mode {
+	UPBOARD_PIN_MODE_FUNCTION = 1,
+	UPBOARD_PIN_MODE_GPIO_IN,
+	UPBOARD_PIN_MODE_GPIO_OUT,
+	UPBOARD_PIN_MODE_DISABLED,
+};
+
+struct upboard_pin {
+	struct regmap_field *funcbit;
+	struct regmap_field *enbit;
+	struct regmap_field *dirbit;
+};
+
+struct upboard_pingroup {
+	struct pingroup grp;
+	enum upboard_pin_mode mode;
+	const enum upboard_pin_mode *modes;
+};
+
+struct upboard_pinctrl_data {
+	const struct upboard_pingroup *groups;
+	size_t ngroups;
+	const struct pinfunction *funcs;
+	size_t nfuncs;
+	const struct pinctrl_map *maps;
+	size_t nmaps;
+	const unsigned int *pin_header;
+	size_t ngpio;
+};
+
+struct upboard_pinctrl {
+	struct gpio_chip chip;
+	struct device *dev;
+	struct pinctrl_dev *pctldev;
+	const struct upboard_pinctrl_data *pctrl_data;
+	struct gpio_pin_range pin_range;
+	struct upboard_pin *pins;
+	struct gpio_desc **gpio;
+};
+
+enum upboard_func0_fpgabit {
+	UPBOARD_FUNC_I2C0_EN = 8,
+	UPBOARD_FUNC_I2C1_EN = 9,
+	UPBOARD_FUNC_CEC0_EN = 12,
+	UPBOARD_FUNC_ADC0_EN = 14,
+};
+
+static const struct reg_field upboard_i2c0_reg =
+	REG_FIELD(UPBOARD_REG_FUNC_EN0, UPBOARD_FUNC_I2C0_EN, UPBOARD_FUNC_I2C0_EN);
+
+static const struct reg_field upboard_i2c1_reg =
+	REG_FIELD(UPBOARD_REG_FUNC_EN0, UPBOARD_FUNC_I2C1_EN, UPBOARD_FUNC_I2C1_EN);
+
+static const struct reg_field upboard_adc0_reg =
+	REG_FIELD(UPBOARD_REG_FUNC_EN0, UPBOARD_FUNC_ADC0_EN, UPBOARD_FUNC_ADC0_EN);
+
+#define UPBOARD_UP_BIT_TO_PIN(bit) UPBOARD_UP_BIT_##bit
+
+#define UPBOARD_UP_PIN_NAME(id)					\
+	{							\
+		.number = UPBOARD_UP_BIT_##id,			\
+		.name = #id,					\
+	}
+
+#define UPBOARD_UP_PIN_MUX(bit, data)				\
+	{							\
+		.number = UPBOARD_UP_BIT_##bit,			\
+		.name = "PINMUX_"#bit,				\
+		.drv_data = (void *)(data),			\
+	}
+
+#define UPBOARD_UP_PIN_FUNC(id, data)				\
+	{							\
+		.number = UPBOARD_UP_BIT_##id,			\
+		.name = #id,					\
+		.drv_data = (void *)(data),			\
+	}
+
+enum upboard_up_fpgabit {
+	UPBOARD_UP_BIT_I2C1_SDA,
+	UPBOARD_UP_BIT_I2C1_SCL,
+	UPBOARD_UP_BIT_ADC0,
+	UPBOARD_UP_BIT_UART1_RTS,
+	UPBOARD_UP_BIT_GPIO27,
+	UPBOARD_UP_BIT_GPIO22,
+	UPBOARD_UP_BIT_SPI_MOSI,
+	UPBOARD_UP_BIT_SPI_MISO,
+	UPBOARD_UP_BIT_SPI_CLK,
+	UPBOARD_UP_BIT_I2C0_SDA,
+	UPBOARD_UP_BIT_GPIO5,
+	UPBOARD_UP_BIT_GPIO6,
+	UPBOARD_UP_BIT_PWM1,
+	UPBOARD_UP_BIT_I2S_FRM,
+	UPBOARD_UP_BIT_GPIO26,
+	UPBOARD_UP_BIT_UART1_TX,
+	UPBOARD_UP_BIT_UART1_RX,
+	UPBOARD_UP_BIT_I2S_CLK,
+	UPBOARD_UP_BIT_GPIO23,
+	UPBOARD_UP_BIT_GPIO24,
+	UPBOARD_UP_BIT_GPIO25,
+	UPBOARD_UP_BIT_SPI_CS0,
+	UPBOARD_UP_BIT_SPI_CS1,
+	UPBOARD_UP_BIT_I2C0_SCL,
+	UPBOARD_UP_BIT_PWM0,
+	UPBOARD_UP_BIT_UART1_CTS,
+	UPBOARD_UP_BIT_I2S_DIN,
+	UPBOARD_UP_BIT_I2S_DOUT,
+};
+
+static const struct pinctrl_pin_desc upboard_up_pins[] = {
+	UPBOARD_UP_PIN_FUNC(I2C1_SDA, &upboard_i2c1_reg),
+	UPBOARD_UP_PIN_FUNC(I2C1_SCL, &upboard_i2c1_reg),
+	UPBOARD_UP_PIN_FUNC(ADC0, &upboard_adc0_reg),
+	UPBOARD_UP_PIN_NAME(UART1_RTS),
+	UPBOARD_UP_PIN_NAME(GPIO27),
+	UPBOARD_UP_PIN_NAME(GPIO22),
+	UPBOARD_UP_PIN_NAME(SPI_MOSI),
+	UPBOARD_UP_PIN_NAME(SPI_MISO),
+	UPBOARD_UP_PIN_NAME(SPI_CLK),
+	UPBOARD_UP_PIN_FUNC(I2C0_SDA, &upboard_i2c0_reg),
+	UPBOARD_UP_PIN_NAME(GPIO5),
+	UPBOARD_UP_PIN_NAME(GPIO6),
+	UPBOARD_UP_PIN_NAME(PWM1),
+	UPBOARD_UP_PIN_NAME(I2S_FRM),
+	UPBOARD_UP_PIN_NAME(GPIO26),
+	UPBOARD_UP_PIN_NAME(UART1_TX),
+	UPBOARD_UP_PIN_NAME(UART1_RX),
+	UPBOARD_UP_PIN_NAME(I2S_CLK),
+	UPBOARD_UP_PIN_NAME(GPIO23),
+	UPBOARD_UP_PIN_NAME(GPIO24),
+	UPBOARD_UP_PIN_NAME(GPIO25),
+	UPBOARD_UP_PIN_NAME(SPI_CS0),
+	UPBOARD_UP_PIN_NAME(SPI_CS1),
+	UPBOARD_UP_PIN_FUNC(I2C0_SCL, &upboard_i2c0_reg),
+	UPBOARD_UP_PIN_NAME(PWM0),
+	UPBOARD_UP_PIN_NAME(UART1_CTS),
+	UPBOARD_UP_PIN_NAME(I2S_DIN),
+	UPBOARD_UP_PIN_NAME(I2S_DOUT),
+};
+
+static const unsigned int upboard_up_pin_header[] = {
+	UPBOARD_UP_BIT_TO_PIN(I2C0_SDA),
+	UPBOARD_UP_BIT_TO_PIN(I2C0_SCL),
+	UPBOARD_UP_BIT_TO_PIN(I2C1_SDA),
+	UPBOARD_UP_BIT_TO_PIN(I2C1_SCL),
+	UPBOARD_UP_BIT_TO_PIN(ADC0),
+	UPBOARD_UP_BIT_TO_PIN(GPIO5),
+	UPBOARD_UP_BIT_TO_PIN(GPIO6),
+	UPBOARD_UP_BIT_TO_PIN(SPI_CS1),
+	UPBOARD_UP_BIT_TO_PIN(SPI_CS0),
+	UPBOARD_UP_BIT_TO_PIN(SPI_MISO),
+	UPBOARD_UP_BIT_TO_PIN(SPI_MOSI),
+	UPBOARD_UP_BIT_TO_PIN(SPI_CLK),
+	UPBOARD_UP_BIT_TO_PIN(PWM0),
+	UPBOARD_UP_BIT_TO_PIN(PWM1),
+	UPBOARD_UP_BIT_TO_PIN(UART1_TX),
+	UPBOARD_UP_BIT_TO_PIN(UART1_RX),
+	UPBOARD_UP_BIT_TO_PIN(UART1_CTS),
+	UPBOARD_UP_BIT_TO_PIN(UART1_RTS),
+	UPBOARD_UP_BIT_TO_PIN(I2S_CLK),
+	UPBOARD_UP_BIT_TO_PIN(I2S_FRM),
+	UPBOARD_UP_BIT_TO_PIN(I2S_DIN),
+	UPBOARD_UP_BIT_TO_PIN(I2S_DOUT),
+	UPBOARD_UP_BIT_TO_PIN(GPIO22),
+	UPBOARD_UP_BIT_TO_PIN(GPIO23),
+	UPBOARD_UP_BIT_TO_PIN(GPIO24),
+	UPBOARD_UP_BIT_TO_PIN(GPIO25),
+	UPBOARD_UP_BIT_TO_PIN(GPIO26),
+	UPBOARD_UP_BIT_TO_PIN(GPIO27),
+};
+
+static const unsigned int upboard_up_uart1_pins[] = {
+	UPBOARD_UP_BIT_TO_PIN(UART1_TX),
+	UPBOARD_UP_BIT_TO_PIN(UART1_RX),
+	UPBOARD_UP_BIT_TO_PIN(UART1_RTS),
+	UPBOARD_UP_BIT_TO_PIN(UART1_CTS),
+};
+
+static const enum upboard_pin_mode upboard_up_uart1_modes[] = {
+	UPBOARD_PIN_MODE_GPIO_OUT,
+	UPBOARD_PIN_MODE_GPIO_IN,
+	UPBOARD_PIN_MODE_GPIO_OUT,
+	UPBOARD_PIN_MODE_GPIO_IN,
+};
+
+static_assert(ARRAY_SIZE(upboard_up_uart1_modes) ==
+	      ARRAY_SIZE(upboard_up_uart1_pins));
+
+static const unsigned int upboard_up_i2c0_pins[] = {
+	UPBOARD_UP_BIT_TO_PIN(I2C0_SCL),
+	UPBOARD_UP_BIT_TO_PIN(I2C0_SDA),
+};
+
+static const unsigned int upboard_up_i2c1_pins[] = {
+	UPBOARD_UP_BIT_TO_PIN(I2C1_SCL),
+	UPBOARD_UP_BIT_TO_PIN(I2C1_SDA),
+};
+
+static const unsigned int upboard_up_spi2_pins[] = {
+	UPBOARD_UP_BIT_TO_PIN(SPI_MOSI),
+	UPBOARD_UP_BIT_TO_PIN(SPI_MISO),
+	UPBOARD_UP_BIT_TO_PIN(SPI_CLK),
+	UPBOARD_UP_BIT_TO_PIN(SPI_CS0),
+	UPBOARD_UP_BIT_TO_PIN(SPI_CS1),
+};
+
+static const enum upboard_pin_mode upboard_up_spi2_modes[] = {
+	UPBOARD_PIN_MODE_GPIO_OUT,
+	UPBOARD_PIN_MODE_GPIO_IN,
+	UPBOARD_PIN_MODE_GPIO_OUT,
+	UPBOARD_PIN_MODE_GPIO_OUT,
+	UPBOARD_PIN_MODE_GPIO_OUT,
+};
+
+static_assert(ARRAY_SIZE(upboard_up_spi2_modes) ==
+	      ARRAY_SIZE(upboard_up_spi2_pins));
+
+static const unsigned int upboard_up_i2s0_pins[]  = {
+	UPBOARD_UP_BIT_TO_PIN(I2S_FRM),
+	UPBOARD_UP_BIT_TO_PIN(I2S_CLK),
+	UPBOARD_UP_BIT_TO_PIN(I2S_DIN),
+	UPBOARD_UP_BIT_TO_PIN(I2S_DOUT),
+};
+
+static const enum upboard_pin_mode upboard_up_i2s0_modes[] = {
+	UPBOARD_PIN_MODE_GPIO_OUT,
+	UPBOARD_PIN_MODE_GPIO_OUT,
+	UPBOARD_PIN_MODE_GPIO_IN,
+	UPBOARD_PIN_MODE_GPIO_OUT,
+};
+
+static_assert(ARRAY_SIZE(upboard_up_i2s0_pins) ==
+	      ARRAY_SIZE(upboard_up_i2s0_modes));
+
+static const unsigned int upboard_up_pwm0_pins[] = {
+	UPBOARD_UP_BIT_TO_PIN(PWM0),
+};
+
+static const unsigned int upboard_up_pwm1_pins[] = {
+	UPBOARD_UP_BIT_TO_PIN(PWM1),
+};
+
+static const unsigned int upboard_up_adc0_pins[] = {
+	UPBOARD_UP_BIT_TO_PIN(ADC0),
+};
+
+#define UPBOARD_PINGROUP_MODE(_grp, _pins, _mode)			\
+{									\
+	.grp = PINCTRL_PINGROUP(_grp, _pins, ARRAY_SIZE(_pins)),	\
+	.mode = _mode,							\
+}
+
+#define UPBOARD_PINGROUP_MODES(_grp, _pins, _modes)			\
+{									\
+	.grp = PINCTRL_PINGROUP(_grp, _pins, ARRAY_SIZE(_pins)),	\
+	.modes = _modes,							\
+}
+
+static const struct upboard_pingroup upboard_up_pin_groups[] = {
+	UPBOARD_PINGROUP_MODES("uart1_grp", upboard_up_uart1_pins, upboard_up_uart1_modes),
+	UPBOARD_PINGROUP_MODE("i2c0_grp", upboard_up_i2c0_pins, UPBOARD_PIN_MODE_GPIO_OUT),
+	UPBOARD_PINGROUP_MODE("i2c1_grp", upboard_up_i2c1_pins, UPBOARD_PIN_MODE_GPIO_OUT),
+	UPBOARD_PINGROUP_MODES("spi2_grp", upboard_up_spi2_pins, &upboard_up_spi2_modes[0]),
+	UPBOARD_PINGROUP_MODES("i2s0_grp", upboard_up_i2s0_pins, &upboard_up_i2s0_modes[0]),
+	UPBOARD_PINGROUP_MODE("pwm0_grp", upboard_up_pwm0_pins, UPBOARD_PIN_MODE_GPIO_OUT),
+	UPBOARD_PINGROUP_MODE("pwm1_grp", upboard_up_pwm1_pins, UPBOARD_PIN_MODE_GPIO_OUT),
+	UPBOARD_PINGROUP_MODE("adc0_grp", upboard_up_adc0_pins, UPBOARD_PIN_MODE_GPIO_IN),
+};
+
+static const char * const upboard_up_uart1_groups[] = { "uart1_grp" };
+static const char * const upboard_up_i2c0_groups[]  = { "i2c0_grp" };
+static const char * const upboard_up_i2c1_groups[]  = { "i2c1_grp" };
+static const char * const upboard_up_spi2_groups[]  = { "spi2_grp" };
+static const char * const upboard_up_i2s0_groups[]  = { "i2s0_grp" };
+static const char * const upboard_up_pwm0_groups[]  = { "pwm0_grp" };
+static const char * const upboard_up_pwm1_groups[]  = { "pwm1_grp" };
+static const char * const upboard_up_adc0_groups[]  = { "adc0_grp" };
+
+#define UPBOARD_FUNCTION(func, groups)	PINCTRL_PINFUNCTION(func, groups, ARRAY_SIZE(groups))
+
+static const struct pinfunction upboard_up_pin_functions[] = {
+	UPBOARD_FUNCTION("uart1", upboard_up_uart1_groups),
+	UPBOARD_FUNCTION("i2c0", upboard_up_i2c0_groups),
+	UPBOARD_FUNCTION("i2c1", upboard_up_i2c1_groups),
+	UPBOARD_FUNCTION("spi2", upboard_up_spi2_groups),
+	UPBOARD_FUNCTION("i2s0", upboard_up_i2s0_groups),
+	UPBOARD_FUNCTION("pwm0", upboard_up_pwm0_groups),
+	UPBOARD_FUNCTION("pwm1", upboard_up_pwm1_groups),
+	UPBOARD_FUNCTION("adc0", upboard_up_adc0_groups),
+};
+
+static const struct pinctrl_map upboard_up_pin_mapping[] = {
+	PIN_MAP_MUX_GROUP_HOG_DEFAULT("upboard-pinctrl", "uart1_grp", "uart1"),
+	PIN_MAP_MUX_GROUP_HOG_DEFAULT("upboard-pinctrl", "i2c0_grp", "i2c0"),
+	PIN_MAP_MUX_GROUP_HOG_DEFAULT("upboard-pinctrl", "i2c1_grp", "i2c1"),
+	PIN_MAP_MUX_GROUP_HOG_DEFAULT("upboard-pinctrl", "spi2_grp", "spi2"),
+	PIN_MAP_MUX_GROUP_HOG_DEFAULT("upboard-pinctrl", "i2s0_grp", "i2s0"),
+	PIN_MAP_MUX_GROUP_HOG_DEFAULT("upboard-pinctrl", "pwm0_grp", "pwm0"),
+	PIN_MAP_MUX_GROUP_HOG_DEFAULT("upboard-pinctrl", "pwm1_grp", "pwm1"),
+	PIN_MAP_MUX_GROUP_HOG_DEFAULT("upboard-pinctrl", "adc0_grp", "adc0"),
+};
+
+static const struct upboard_pinctrl_data upboard_up_pinctrl_data = {
+	.groups = &upboard_up_pin_groups[0],
+	.ngroups = ARRAY_SIZE(upboard_up_pin_groups),
+	.funcs = &upboard_up_pin_functions[0],
+	.nfuncs = ARRAY_SIZE(upboard_up_pin_functions),
+	.maps = &upboard_up_pin_mapping[0],
+	.nmaps = ARRAY_SIZE(upboard_up_pin_mapping),
+	.pin_header = &upboard_up_pin_header[0],
+	.ngpio = ARRAY_SIZE(upboard_up_pin_header),
+};
+
+#define UPBOARD_UP2_BIT_TO_PIN(bit) UPBOARD_UP2_BIT_##bit
+
+#define UPBOARD_UP2_PIN_NAME(id)					\
+	{								\
+		.number = UPBOARD_UP2_BIT_##id,				\
+		.name = #id,						\
+	}
+
+#define UPBOARD_UP2_PIN_MUX(bit, data)					\
+	{								\
+		.number = UPBOARD_UP2_BIT_##bit,			\
+		.name = "PINMUX_"#bit,					\
+		.drv_data = (void *)(data),				\
+	}
+
+#define UPBOARD_UP2_PIN_FUNC(id, data)					\
+	{								\
+		.number = UPBOARD_UP2_BIT_##id,				\
+		.name = #id,						\
+		.drv_data = (void *)(data),				\
+	}
+
+enum upboard_up2_fpgabit {
+	UPBOARD_UP2_BIT_UART1_TXD,
+	UPBOARD_UP2_BIT_UART1_RXD,
+	UPBOARD_UP2_BIT_UART1_RTS,
+	UPBOARD_UP2_BIT_UART1_CTS,
+	UPBOARD_UP2_BIT_GPIO3_ADC0,
+	UPBOARD_UP2_BIT_GPIO5_ADC2,
+	UPBOARD_UP2_BIT_GPIO6_ADC3,
+	UPBOARD_UP2_BIT_GPIO11,
+	UPBOARD_UP2_BIT_EXHAT_LVDS1n,
+	UPBOARD_UP2_BIT_EXHAT_LVDS1p,
+	UPBOARD_UP2_BIT_SPI2_TXD,
+	UPBOARD_UP2_BIT_SPI2_RXD,
+	UPBOARD_UP2_BIT_SPI2_FS1,
+	UPBOARD_UP2_BIT_SPI2_FS0,
+	UPBOARD_UP2_BIT_SPI2_CLK,
+	UPBOARD_UP2_BIT_SPI1_TXD,
+	UPBOARD_UP2_BIT_SPI1_RXD,
+	UPBOARD_UP2_BIT_SPI1_FS1,
+	UPBOARD_UP2_BIT_SPI1_FS0,
+	UPBOARD_UP2_BIT_SPI1_CLK,
+	UPBOARD_UP2_BIT_I2C0_SCL,
+	UPBOARD_UP2_BIT_I2C0_SDA,
+	UPBOARD_UP2_BIT_I2C1_SCL,
+	UPBOARD_UP2_BIT_I2C1_SDA,
+	UPBOARD_UP2_BIT_PWM1,
+	UPBOARD_UP2_BIT_PWM0,
+	UPBOARD_UP2_BIT_EXHAT_LVDS0n,
+	UPBOARD_UP2_BIT_EXHAT_LVDS0p,
+	UPBOARD_UP2_BIT_GPIO24,
+	UPBOARD_UP2_BIT_GPIO10,
+	UPBOARD_UP2_BIT_GPIO2,
+	UPBOARD_UP2_BIT_GPIO1,
+	UPBOARD_UP2_BIT_EXHAT_LVDS3n,
+	UPBOARD_UP2_BIT_EXHAT_LVDS3p,
+	UPBOARD_UP2_BIT_EXHAT_LVDS4n,
+	UPBOARD_UP2_BIT_EXHAT_LVDS4p,
+	UPBOARD_UP2_BIT_EXHAT_LVDS5n,
+	UPBOARD_UP2_BIT_EXHAT_LVDS5p,
+	UPBOARD_UP2_BIT_I2S_SDO,
+	UPBOARD_UP2_BIT_I2S_SDI,
+	UPBOARD_UP2_BIT_I2S_WS_SYNC,
+	UPBOARD_UP2_BIT_I2S_BCLK,
+	UPBOARD_UP2_BIT_EXHAT_LVDS6n,
+	UPBOARD_UP2_BIT_EXHAT_LVDS6p,
+	UPBOARD_UP2_BIT_EXHAT_LVDS7n,
+	UPBOARD_UP2_BIT_EXHAT_LVDS7p,
+	UPBOARD_UP2_BIT_EXHAT_LVDS2n,
+	UPBOARD_UP2_BIT_EXHAT_LVDS2p,
+};
+
+static const struct pinctrl_pin_desc upboard_up2_pins[] = {
+	UPBOARD_UP2_PIN_NAME(UART1_TXD),
+	UPBOARD_UP2_PIN_NAME(UART1_RXD),
+	UPBOARD_UP2_PIN_NAME(UART1_RTS),
+	UPBOARD_UP2_PIN_NAME(UART1_CTS),
+	UPBOARD_UP2_PIN_NAME(GPIO3_ADC0),
+	UPBOARD_UP2_PIN_NAME(GPIO5_ADC2),
+	UPBOARD_UP2_PIN_NAME(GPIO6_ADC3),
+	UPBOARD_UP2_PIN_NAME(GPIO11),
+	UPBOARD_UP2_PIN_NAME(EXHAT_LVDS1n),
+	UPBOARD_UP2_PIN_NAME(EXHAT_LVDS1p),
+	UPBOARD_UP2_PIN_NAME(SPI2_TXD),
+	UPBOARD_UP2_PIN_NAME(SPI2_RXD),
+	UPBOARD_UP2_PIN_NAME(SPI2_FS1),
+	UPBOARD_UP2_PIN_NAME(SPI2_FS0),
+	UPBOARD_UP2_PIN_NAME(SPI2_CLK),
+	UPBOARD_UP2_PIN_NAME(SPI1_TXD),
+	UPBOARD_UP2_PIN_NAME(SPI1_RXD),
+	UPBOARD_UP2_PIN_NAME(SPI1_FS1),
+	UPBOARD_UP2_PIN_NAME(SPI1_FS0),
+	UPBOARD_UP2_PIN_NAME(SPI1_CLK),
+	UPBOARD_UP2_PIN_MUX(I2C0_SCL, &upboard_i2c0_reg),
+	UPBOARD_UP2_PIN_MUX(I2C0_SDA, &upboard_i2c0_reg),
+	UPBOARD_UP2_PIN_MUX(I2C1_SCL, &upboard_i2c1_reg),
+	UPBOARD_UP2_PIN_MUX(I2C1_SDA, &upboard_i2c1_reg),
+	UPBOARD_UP2_PIN_NAME(PWM1),
+	UPBOARD_UP2_PIN_NAME(PWM0),
+	UPBOARD_UP2_PIN_NAME(EXHAT_LVDS0n),
+	UPBOARD_UP2_PIN_NAME(EXHAT_LVDS0p),
+	UPBOARD_UP2_PIN_MUX(GPIO24, &upboard_i2c0_reg),
+	UPBOARD_UP2_PIN_MUX(GPIO10, &upboard_i2c0_reg),
+	UPBOARD_UP2_PIN_MUX(GPIO2, &upboard_i2c1_reg),
+	UPBOARD_UP2_PIN_MUX(GPIO1, &upboard_i2c1_reg),
+	UPBOARD_UP2_PIN_NAME(EXHAT_LVDS3n),
+	UPBOARD_UP2_PIN_NAME(EXHAT_LVDS3p),
+	UPBOARD_UP2_PIN_NAME(EXHAT_LVDS4n),
+	UPBOARD_UP2_PIN_NAME(EXHAT_LVDS4p),
+	UPBOARD_UP2_PIN_NAME(EXHAT_LVDS5n),
+	UPBOARD_UP2_PIN_NAME(EXHAT_LVDS5p),
+	UPBOARD_UP2_PIN_NAME(I2S_SDO),
+	UPBOARD_UP2_PIN_NAME(I2S_SDI),
+	UPBOARD_UP2_PIN_NAME(I2S_WS_SYNC),
+	UPBOARD_UP2_PIN_NAME(I2S_BCLK),
+	UPBOARD_UP2_PIN_NAME(EXHAT_LVDS6n),
+	UPBOARD_UP2_PIN_NAME(EXHAT_LVDS6p),
+	UPBOARD_UP2_PIN_NAME(EXHAT_LVDS7n),
+	UPBOARD_UP2_PIN_NAME(EXHAT_LVDS7p),
+	UPBOARD_UP2_PIN_NAME(EXHAT_LVDS2n),
+	UPBOARD_UP2_PIN_NAME(EXHAT_LVDS2p),
+};
+
+static const unsigned int upboard_up2_pin_header[] = {
+	UPBOARD_UP2_BIT_TO_PIN(GPIO10),
+	UPBOARD_UP2_BIT_TO_PIN(GPIO24),
+	UPBOARD_UP2_BIT_TO_PIN(GPIO1),
+	UPBOARD_UP2_BIT_TO_PIN(GPIO2),
+	UPBOARD_UP2_BIT_TO_PIN(GPIO3_ADC0),
+	UPBOARD_UP2_BIT_TO_PIN(GPIO11),
+	UPBOARD_UP2_BIT_TO_PIN(SPI2_CLK),
+	UPBOARD_UP2_BIT_TO_PIN(SPI1_FS1),
+	UPBOARD_UP2_BIT_TO_PIN(SPI1_FS0),
+	UPBOARD_UP2_BIT_TO_PIN(SPI1_RXD),
+	UPBOARD_UP2_BIT_TO_PIN(SPI1_TXD),
+	UPBOARD_UP2_BIT_TO_PIN(SPI1_CLK),
+	UPBOARD_UP2_BIT_TO_PIN(PWM0),
+	UPBOARD_UP2_BIT_TO_PIN(PWM1),
+	UPBOARD_UP2_BIT_TO_PIN(UART1_TXD),
+	UPBOARD_UP2_BIT_TO_PIN(UART1_RXD),
+	UPBOARD_UP2_BIT_TO_PIN(UART1_CTS),
+	UPBOARD_UP2_BIT_TO_PIN(UART1_RTS),
+	UPBOARD_UP2_BIT_TO_PIN(I2S_BCLK),
+	UPBOARD_UP2_BIT_TO_PIN(I2S_WS_SYNC),
+	UPBOARD_UP2_BIT_TO_PIN(I2S_SDI),
+	UPBOARD_UP2_BIT_TO_PIN(I2S_SDO),
+	UPBOARD_UP2_BIT_TO_PIN(GPIO6_ADC3),
+	UPBOARD_UP2_BIT_TO_PIN(SPI2_FS1),
+	UPBOARD_UP2_BIT_TO_PIN(SPI2_RXD),
+	UPBOARD_UP2_BIT_TO_PIN(SPI2_TXD),
+	UPBOARD_UP2_BIT_TO_PIN(SPI2_FS0),
+	UPBOARD_UP2_BIT_TO_PIN(GPIO5_ADC2),
+};
+
+static const unsigned int upboard_up2_uart1_pins[] = {
+	UPBOARD_UP2_BIT_TO_PIN(UART1_TXD),
+	UPBOARD_UP2_BIT_TO_PIN(UART1_RXD),
+	UPBOARD_UP2_BIT_TO_PIN(UART1_RTS),
+	UPBOARD_UP2_BIT_TO_PIN(UART1_CTS),
+};
+
+static const enum upboard_pin_mode upboard_up2_uart1_modes[] = {
+	UPBOARD_PIN_MODE_GPIO_OUT,
+	UPBOARD_PIN_MODE_GPIO_IN,
+	UPBOARD_PIN_MODE_GPIO_OUT,
+	UPBOARD_PIN_MODE_GPIO_IN
+};
+
+static_assert(ARRAY_SIZE(upboard_up2_uart1_modes) ==
+	      ARRAY_SIZE(upboard_up2_uart1_pins));
+
+static const unsigned int upboard_up2_i2c0_pins[] = {
+	UPBOARD_UP2_BIT_TO_PIN(I2C0_SCL),
+	UPBOARD_UP2_BIT_TO_PIN(I2C0_SDA),
+	UPBOARD_UP2_BIT_TO_PIN(GPIO24),
+	UPBOARD_UP2_BIT_TO_PIN(GPIO10),
+};
+
+static const unsigned int upboard_up2_i2c1_pins[] = {
+	UPBOARD_UP2_BIT_TO_PIN(I2C1_SCL),
+	UPBOARD_UP2_BIT_TO_PIN(I2C1_SDA),
+	UPBOARD_UP2_BIT_TO_PIN(GPIO2),
+	UPBOARD_UP2_BIT_TO_PIN(GPIO1),
+};
+
+static const unsigned int upboard_up2_spi1_pins[] = {
+	UPBOARD_UP2_BIT_TO_PIN(SPI1_TXD),
+	UPBOARD_UP2_BIT_TO_PIN(SPI1_RXD),
+	UPBOARD_UP2_BIT_TO_PIN(SPI1_FS1),
+	UPBOARD_UP2_BIT_TO_PIN(SPI1_FS0),
+	UPBOARD_UP2_BIT_TO_PIN(SPI1_CLK),
+};
+
+static const unsigned int upboard_up2_spi2_pins[] = {
+	UPBOARD_UP2_BIT_TO_PIN(SPI2_TXD),
+	UPBOARD_UP2_BIT_TO_PIN(SPI2_RXD),
+	UPBOARD_UP2_BIT_TO_PIN(SPI2_FS1),
+	UPBOARD_UP2_BIT_TO_PIN(SPI2_FS0),
+	UPBOARD_UP2_BIT_TO_PIN(SPI2_CLK),
+};
+
+static const enum upboard_pin_mode upboard_up2_spi_modes[] = {
+	UPBOARD_PIN_MODE_GPIO_OUT,
+	UPBOARD_PIN_MODE_GPIO_IN,
+	UPBOARD_PIN_MODE_GPIO_OUT,
+	UPBOARD_PIN_MODE_GPIO_OUT,
+	UPBOARD_PIN_MODE_GPIO_OUT,
+};
+
+static_assert(ARRAY_SIZE(upboard_up2_spi_modes) ==
+	      ARRAY_SIZE(upboard_up2_spi1_pins));
+
+static_assert(ARRAY_SIZE(upboard_up2_spi_modes) ==
+	      ARRAY_SIZE(upboard_up2_spi2_pins));
+
+static const unsigned int upboard_up2_i2s0_pins[] = {
+	UPBOARD_UP2_BIT_TO_PIN(I2S_BCLK),
+	UPBOARD_UP2_BIT_TO_PIN(I2S_WS_SYNC),
+	UPBOARD_UP2_BIT_TO_PIN(I2S_SDI),
+	UPBOARD_UP2_BIT_TO_PIN(I2S_SDO),
+};
+
+static const enum upboard_pin_mode upboard_up2_i2s0_modes[] = {
+	UPBOARD_PIN_MODE_GPIO_OUT,
+	UPBOARD_PIN_MODE_GPIO_OUT,
+	UPBOARD_PIN_MODE_GPIO_IN,
+	UPBOARD_PIN_MODE_GPIO_OUT
+};
+
+static_assert(ARRAY_SIZE(upboard_up2_i2s0_modes) ==
+	      ARRAY_SIZE(upboard_up2_i2s0_pins));
+
+static const unsigned int upboard_up2_pwm0_pins[] = {
+	UPBOARD_UP2_BIT_TO_PIN(PWM0),
+};
+
+static const unsigned int upboard_up2_pwm1_pins[] = {
+	UPBOARD_UP2_BIT_TO_PIN(PWM1),
+};
+
+static const unsigned int upboard_up2_adc0_pins[] = {
+	UPBOARD_UP2_BIT_TO_PIN(GPIO3_ADC0),
+};
+
+static const unsigned int upboard_up2_adc2_pins[] = {
+	UPBOARD_UP2_BIT_TO_PIN(GPIO5_ADC2),
+};
+
+static const unsigned int upboard_up2_adc3_pins[] = {
+	UPBOARD_UP2_BIT_TO_PIN(GPIO6_ADC3),
+};
+
+static const struct upboard_pingroup upboard_up2_pin_groups[] = {
+	UPBOARD_PINGROUP_MODES("uart1_grp", upboard_up2_uart1_pins, upboard_up2_uart1_modes),
+	UPBOARD_PINGROUP_MODE("i2c0_grp", upboard_up2_i2c0_pins, UPBOARD_PIN_MODE_FUNCTION),
+	UPBOARD_PINGROUP_MODE("i2c1_grp", upboard_up2_i2c1_pins, UPBOARD_PIN_MODE_FUNCTION),
+	UPBOARD_PINGROUP_MODES("spi1_grp", upboard_up2_spi1_pins, upboard_up2_spi_modes),
+	UPBOARD_PINGROUP_MODES("spi2_grp", upboard_up2_spi2_pins, upboard_up2_spi_modes),
+	UPBOARD_PINGROUP_MODES("i2s0_grp", upboard_up2_i2s0_pins, upboard_up2_i2s0_modes),
+	UPBOARD_PINGROUP_MODE("pwm0_grp", upboard_up2_pwm0_pins, UPBOARD_PIN_MODE_GPIO_OUT),
+	UPBOARD_PINGROUP_MODE("pwm1_grp", upboard_up2_pwm1_pins, UPBOARD_PIN_MODE_GPIO_OUT),
+	UPBOARD_PINGROUP_MODE("adc0_grp", upboard_up2_adc0_pins, UPBOARD_PIN_MODE_GPIO_IN),
+	UPBOARD_PINGROUP_MODE("adc2_grp", upboard_up2_adc2_pins, UPBOARD_PIN_MODE_GPIO_IN),
+	UPBOARD_PINGROUP_MODE("adc3_grp", upboard_up2_adc3_pins, UPBOARD_PIN_MODE_GPIO_IN),
+};
+
+static const char * const upboard_up2_uart1_groups[] = { "uart1_grp" };
+static const char * const upboard_up2_i2c0_groups[]  = { "i2c0_grp" };
+static const char * const upboard_up2_i2c1_groups[]  = { "i2c1_grp" };
+static const char * const upboard_up2_spi1_groups[]  = { "spi1_grp" };
+static const char * const upboard_up2_spi2_groups[]  = { "spi2_grp" };
+static const char * const upboard_up2_i2s0_groups[]  = { "i2s0_grp" };
+static const char * const upboard_up2_pwm0_groups[]  = { "pwm0_grp" };
+static const char * const upboard_up2_pwm1_groups[]  = { "pwm1_grp" };
+static const char * const upboard_up2_adc0_groups[]  = { "adc0_grp" };
+static const char * const upboard_up2_adc2_groups[]  = { "adc2_grp" };
+static const char * const upboard_up2_adc3_groups[]  = { "adc3_grp" };
+
+static const struct pinfunction upboard_up2_pin_functions[] = {
+	UPBOARD_FUNCTION("uart1", upboard_up2_uart1_groups),
+	UPBOARD_FUNCTION("i2c0", upboard_up2_i2c0_groups),
+	UPBOARD_FUNCTION("i2c1", upboard_up2_i2c1_groups),
+	UPBOARD_FUNCTION("spi1", upboard_up2_spi1_groups),
+	UPBOARD_FUNCTION("spi2", upboard_up2_spi2_groups),
+	UPBOARD_FUNCTION("i2s0", upboard_up2_i2s0_groups),
+	UPBOARD_FUNCTION("pwm0", upboard_up2_pwm0_groups),
+	UPBOARD_FUNCTION("pwm1", upboard_up2_pwm1_groups),
+	UPBOARD_FUNCTION("adc0", upboard_up2_adc0_groups),
+	UPBOARD_FUNCTION("adc2", upboard_up2_adc2_groups),
+	UPBOARD_FUNCTION("adc3", upboard_up2_adc3_groups),
+};
+
+static const struct pinctrl_map upboard_up2_pin_mapping[] = {
+	PIN_MAP_MUX_GROUP_HOG_DEFAULT("upboard-pinctrl", "uart1_grp", "uart1"),
+	PIN_MAP_MUX_GROUP_HOG_DEFAULT("upboard-pinctrl", "i2c0_grp", "i2c0"),
+	PIN_MAP_MUX_GROUP_HOG_DEFAULT("upboard-pinctrl", "i2c1_grp", "i2c1"),
+	PIN_MAP_MUX_GROUP_HOG_DEFAULT("upboard-pinctrl", "spi1_grp", "spi1"),
+	PIN_MAP_MUX_GROUP_HOG_DEFAULT("upboard-pinctrl", "spi2_grp", "spi2"),
+	PIN_MAP_MUX_GROUP_HOG_DEFAULT("upboard-pinctrl", "i2s0_grp", "i2s0"),
+	PIN_MAP_MUX_GROUP_HOG_DEFAULT("upboard-pinctrl", "pwm0_grp", "pwm0"),
+	PIN_MAP_MUX_GROUP_HOG_DEFAULT("upboard-pinctrl", "pwm1_grp", "pwm1"),
+	PIN_MAP_MUX_GROUP_HOG_DEFAULT("upboard-pinctrl", "adc0_grp", "adc0"),
+	PIN_MAP_MUX_GROUP_HOG_DEFAULT("upboard-pinctrl", "adc2_grp", "adc2"),
+	PIN_MAP_MUX_GROUP_HOG_DEFAULT("upboard-pinctrl", "adc3_grp", "adc3"),
+};
+
+static const struct upboard_pinctrl_data upboard_up2_pinctrl_data = {
+	.groups = &upboard_up2_pin_groups[0],
+	.ngroups = ARRAY_SIZE(upboard_up2_pin_groups),
+	.funcs = &upboard_up2_pin_functions[0],
+	.nfuncs = ARRAY_SIZE(upboard_up2_pin_functions),
+	.maps = &upboard_up2_pin_mapping[0],
+	.nmaps = ARRAY_SIZE(upboard_up2_pin_mapping),
+	.pin_header = &upboard_up2_pin_header[0],
+	.ngpio = ARRAY_SIZE(upboard_up2_pin_header),
+};
+
+static int __upboard_pinctrl_gpio_request_enable(struct pinctrl_dev *pctldev,
+						 unsigned int offset)
+{
+	struct upboard_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
+	struct upboard_pin *p = &pctrl->pins[offset];
+
+	if (p->funcbit) {
+		int ret = regmap_field_write(p->funcbit, 0);
+
+		if (ret)
+			return ret;
+	}
+
+	return regmap_field_write(p->enbit, 1);
+}
+
+static int upboard_pinctrl_gpio_request_enable(struct pinctrl_dev *pctldev,
+					       struct pinctrl_gpio_range *range,
+					       unsigned int offset)
+{
+	return __upboard_pinctrl_gpio_request_enable(pctldev, offset);
+}
+
+static void __upboard_pinctrl_gpio_disable_free(struct pinctrl_dev *pctldev, unsigned int offset)
+{
+	struct upboard_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
+	struct upboard_pin *p = &pctrl->pins[offset];
+
+	regmap_field_write(p->enbit, 0);
+
+	if (p->funcbit)
+		regmap_field_write(p->funcbit, 1);
+};
+
+static void upboard_pinctrl_gpio_disable_free(struct pinctrl_dev *pctldev,
+					      struct pinctrl_gpio_range *range, unsigned int offset)
+{
+	return __upboard_pinctrl_gpio_disable_free(pctldev, offset);
+}
+
+static int __upboard_pinctrl_gpio_set_direction(struct pinctrl_dev *pctldev,
+						unsigned int offset, bool input)
+{
+	struct upboard_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
+	struct upboard_pin *p = &pctrl->pins[offset];
+
+	return regmap_field_write(p->dirbit, input);
+}
+
+static int upboard_pinctrl_gpio_set_direction(struct pinctrl_dev *pctldev,
+					      struct pinctrl_gpio_range *range,
+					      unsigned int offset, bool input)
+{
+	return __upboard_pinctrl_gpio_set_direction(pctldev, offset, input);
+}
+
+static int upboard_pinctrl_set_mux(struct pinctrl_dev *pctldev, unsigned int func_selector,
+				   unsigned int group_selector)
+{
+	struct upboard_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
+	const struct upboard_pinctrl_data *pctrl_data = pctrl->pctrl_data;
+	const struct upboard_pingroup *upgroups = pctrl_data->groups;
+	struct group_desc *grp;
+	int mode, ret, i;
+
+	grp = pinctrl_generic_get_group(pctldev, group_selector);
+	if (!grp)
+		return -EINVAL;
+
+	for (i = 0; i < grp->grp.npins; i++) {
+		mode = upgroups[group_selector].mode ? upgroups[group_selector].mode :
+			upgroups[group_selector].modes[i];
+
+		if (mode == UPBOARD_PIN_MODE_FUNCTION) {
+			__upboard_pinctrl_gpio_disable_free(pctldev, grp->grp.pins[i]);
+			continue;
+		}
+
+		ret = __upboard_pinctrl_gpio_request_enable(pctldev, grp->grp.pins[i]);
+		if (ret)
+			return ret;
+
+		ret = __upboard_pinctrl_gpio_set_direction(pctldev, grp->grp.pins[i],
+							   mode == UPBOARD_PIN_MODE_GPIO_IN ?
+							   true : false);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static const struct pinmux_ops upboard_pinmux_ops = {
+	.get_functions_count = pinmux_generic_get_function_count,
+	.get_function_name = pinmux_generic_get_function_name,
+	.get_function_groups = pinmux_generic_get_function_groups,
+	.set_mux = upboard_pinctrl_set_mux,
+	.gpio_request_enable = upboard_pinctrl_gpio_request_enable,
+	.gpio_disable_free = upboard_pinctrl_gpio_disable_free,
+	.gpio_set_direction = upboard_pinctrl_gpio_set_direction,
+};
+
+static int upboard_pinctrl_pin_get_mode(struct pinctrl_dev *pctldev, unsigned int pin)
+{
+	struct upboard_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
+	struct upboard_pin *p = &pctrl->pins[pin];
+
+	unsigned int val;
+	int ret;
+
+	if (p->funcbit) {
+		ret = regmap_field_read(p->funcbit, &val);
+		if (ret)
+			return ret;
+		if (val)
+			return UPBOARD_PIN_MODE_FUNCTION;
+	}
+
+	ret = regmap_field_read(p->enbit, &val);
+	if (ret)
+		return ret;
+	if (!val)
+		return UPBOARD_PIN_MODE_DISABLED;
+
+	ret = regmap_field_read(p->dirbit, &val);
+	if (ret)
+		return ret;
+
+	return val ? UPBOARD_PIN_MODE_GPIO_IN : UPBOARD_PIN_MODE_GPIO_OUT;
+}
+
+static void upboard_pinctrl_dbg_show(struct pinctrl_dev *pctldev, struct seq_file *s,
+				     unsigned int offset)
+{
+	int ret = upboard_pinctrl_pin_get_mode(pctldev, offset);
+
+	if (ret == UPBOARD_PIN_MODE_FUNCTION)
+		seq_puts(s, "mode function ");
+	else if (ret == UPBOARD_PIN_MODE_DISABLED)
+		seq_puts(s, "HIGH-Z");
+	else
+		seq_printf(s, "GPIO (%s) ", ret == UPBOARD_PIN_MODE_GPIO_IN ? "input" : "output");
+}
+
+static const struct pinctrl_ops upboard_pinctrl_ops = {
+	.get_groups_count = pinctrl_generic_get_group_count,
+	.get_group_name = pinctrl_generic_get_group_name,
+	.get_group_pins = pinctrl_generic_get_group_pins,
+	.pin_dbg_show = upboard_pinctrl_dbg_show,
+};
+
+static int upboard_gpio_request(struct gpio_chip *gc, unsigned int offset)
+{
+	struct upboard_pinctrl *pctrl = container_of(gc, struct upboard_pinctrl, chip);
+	unsigned int pin = pctrl->pctrl_data->pin_header[offset];
+	int ret;
+
+	ret = pinctrl_gpio_request(gc, offset);
+	if (ret)
+		return ret;
+
+	pctrl->gpio[offset] = gpiod_get_index(pctrl->dev, "external", pin, 0);
+
+	if (IS_ERR(pctrl->gpio[offset])) {
+		pinctrl_gpio_free(gc, offset);
+		return PTR_ERR(pctrl->gpio[offset]);
+	}
+
+	return 0;
+}
+
+static void upboard_gpio_free(struct gpio_chip *gc, unsigned int offset)
+{
+	struct upboard_pinctrl *pctrl = container_of(gc, struct upboard_pinctrl, chip);
+
+	gpiod_put(pctrl->gpio[offset]);
+
+	pinctrl_gpio_free(gc, offset);
+}
+
+static int upboard_gpio_get_direction(struct gpio_chip *gc, unsigned int offset)
+{
+	struct upboard_pinctrl *pctrl = container_of(gc, struct upboard_pinctrl, chip);
+	unsigned int pin = pctrl->pctrl_data->pin_header[offset];
+	int mode;
+
+	if (pctrl->gpio[offset])
+		return gpiod_get_direction(pctrl->gpio[offset]);
+
+	/*
+	 * GPIO was not requested so SoC pin is probably not in GPIO mode.
+	 * When a gpio_chip is registered, the core calls get_direction() for all lines.
+	 * At this time, upboard_gpio_request() was not yet called, so the driver didn't
+	 * request the corresponding SoC pin. So the SoC pin is probably in function (not in
+	 * GPIO mode).
+	 *
+	 * To get the direction of the SoC pin, it shall be requested in GPIO mode.
+	 * Once a SoC pin is set in GPIO mode, there is no way to set it back to its
+	 * function mode.
+	 * Instead of returning the SoC pin direction, the direction of the FPGA pin is
+	 * returned (only for the get_direction() called during the gpio_chip registration).
+	 */
+	mode = upboard_pinctrl_pin_get_mode(pctrl->pctldev, pin);
+
+	/* If the pin is in function mode or high-z, input direction is returned */
+	if (mode < 0)
+		return mode;
+	if (mode == UPBOARD_PIN_MODE_GPIO_OUT)
+		return GPIO_LINE_DIRECTION_OUT;
+	else
+		return GPIO_LINE_DIRECTION_IN;
+}
+
+static int upboard_gpio_get(struct gpio_chip *gc, unsigned int offset)
+{
+	struct upboard_pinctrl *pctrl = container_of(gc, struct upboard_pinctrl, chip);
+
+	return gpiod_get_value(pctrl->gpio[offset]);
+}
+
+static void upboard_gpio_set(struct gpio_chip *gc, unsigned int offset, int value)
+{
+	struct upboard_pinctrl *pctrl = container_of(gc, struct upboard_pinctrl, chip);
+
+	gpiod_set_value(pctrl->gpio[offset], value);
+}
+
+static int upboard_gpio_direction_input(struct gpio_chip *gc, unsigned int offset)
+{
+	struct upboard_pinctrl *pctrl = container_of(gc, struct upboard_pinctrl, chip);
+	int ret;
+
+	ret = pinctrl_gpio_direction_input(gc, offset);
+	if (ret)
+		return ret;
+
+	return gpiod_direction_input(pctrl->gpio[offset]);
+}
+
+static int upboard_gpio_direction_output(struct gpio_chip *gc, unsigned int offset, int value)
+{
+	struct upboard_pinctrl *pctrl = container_of(gc, struct upboard_pinctrl, chip);
+	int ret;
+
+	ret = pinctrl_gpio_direction_output(gc, offset);
+	if (ret)
+		return ret;
+
+	return gpiod_direction_output(pctrl->gpio[offset], value);
+}
+
+static int upboard_gpio_to_irq(struct gpio_chip *gc, unsigned int offset)
+{
+	struct upboard_pinctrl *pctrl = container_of(gc, struct upboard_pinctrl, chip);
+
+	return gpiod_to_irq(pctrl->gpio[offset]);
+}
+
+static int upboard_pinctrl_register_groups(struct upboard_pinctrl *pctrl)
+{
+	const struct upboard_pingroup *groups = pctrl->pctrl_data->groups;
+	size_t ngroups = pctrl->pctrl_data->ngroups;
+	int ret, i;
+
+	for (i = 0; i < ngroups; i++) {
+		ret = pinctrl_generic_add_group(pctrl->pctldev, groups[i].grp.name,
+						groups[i].grp.pins, groups[i].grp.npins, pctrl);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int upboard_pinctrl_register_functions(struct upboard_pinctrl *pctrl)
+{
+	const struct pinfunction *funcs = pctrl->pctrl_data->funcs;
+	size_t nfuncs = pctrl->pctrl_data->nfuncs;
+	int ret, i;
+
+	for (i = 0; i < nfuncs ; ++i) {
+		ret = pinmux_generic_add_function(pctrl->pctldev, funcs[i].name, funcs[i].groups,
+						  funcs[i].ngroups, NULL);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int upboard_pinctrl_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct upboard_fpga *fpga = dev_get_drvdata(dev->parent);
+	struct pinctrl_desc *pctldesc;
+	struct upboard_pinctrl *pctrl;
+	struct upboard_pin *pins;
+	struct gpio_chip *chip;
+	int ret, i;
+
+	pctldesc = devm_kzalloc(dev, sizeof(*pctldesc), GFP_KERNEL);
+	if (!pctldesc)
+		return -ENOMEM;
+
+	pctrl = devm_kzalloc(dev, sizeof(*pctrl), GFP_KERNEL);
+	if (!pctrl)
+		return -ENOMEM;
+
+	switch (fpga->fpga_data->type) {
+	case UPBOARD_UP_FPGA:
+		pctldesc->pins = upboard_up_pins;
+		pctldesc->npins = ARRAY_SIZE(upboard_up_pins);
+		pctrl->pctrl_data = &upboard_up_pinctrl_data;
+		break;
+	case UPBOARD_UP2_FPGA:
+		pctldesc->pins = upboard_up2_pins;
+		pctldesc->npins = ARRAY_SIZE(upboard_up2_pins);
+		pctrl->pctrl_data = &upboard_up2_pinctrl_data;
+		break;
+	default:
+		return dev_err_probe(dev, -ENODEV, "Unsupported device type %d\n",
+				     fpga->fpga_data->type);
+	}
+
+	pctldesc->name = dev_name(dev);
+	pctldesc->owner = THIS_MODULE;
+	pctldesc->pctlops = &upboard_pinctrl_ops;
+	pctldesc->pmxops = &upboard_pinmux_ops;
+
+	pctrl->dev = dev;
+
+	pins = devm_kcalloc(dev, pctldesc->npins, sizeof(*pins), GFP_KERNEL);
+	if (!pins)
+		return -ENOMEM;
+
+	/* Initialize pins */
+	for (i = 0; i < pctldesc->npins; i++) {
+		const struct pinctrl_pin_desc *pin_desc = &pctldesc->pins[i];
+		unsigned int regoff = pin_desc->number / UPBOARD_REGISTER_SIZE;
+		unsigned int lsb = pin_desc->number % UPBOARD_REGISTER_SIZE;
+		struct reg_field * const fld_func = pin_desc->drv_data;
+		struct upboard_pin *pin = &pins[i];
+		struct reg_field fldconf = {};
+
+		if (fld_func) {
+			pin->funcbit = devm_regmap_field_alloc(dev, fpga->regmap, *fld_func);
+
+			if (IS_ERR(pin->funcbit))
+				return PTR_ERR(pin->funcbit);
+		}
+
+		fldconf.reg = UPBOARD_REG_GPIO_EN0 + regoff;
+		fldconf.lsb = lsb;
+		fldconf.msb = lsb;
+		pin->enbit = devm_regmap_field_alloc(dev, fpga->regmap, fldconf);
+		if (IS_ERR(pin->enbit))
+			return PTR_ERR(pin->enbit);
+
+		fldconf.reg = UPBOARD_REG_GPIO_DIR0 + regoff;
+		fldconf.lsb = lsb;
+		fldconf.msb = lsb;
+		pin->dirbit = devm_regmap_field_alloc(dev, fpga->regmap, fldconf);
+		if (IS_ERR(pin->dirbit))
+			return PTR_ERR(pin->dirbit);
+	}
+
+	pctrl->pins = pins;
+
+	ret = pinctrl_register_mappings(pctrl->pctrl_data->maps, pctrl->pctrl_data->nmaps);
+
+	ret = devm_pinctrl_register_and_init(dev, pctldesc, pctrl, &pctrl->pctldev);
+	if (ret)
+		return ret;
+
+	ret = upboard_pinctrl_register_groups(pctrl);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to register groups");
+
+	ret = upboard_pinctrl_register_functions(pctrl);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to register functions");
+
+	ret = pinctrl_enable(pctrl->pctldev);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to enable pinctrl");
+
+	chip = &pctrl->chip;
+	chip->owner = THIS_MODULE;
+	chip->label = dev_name(&pdev->dev);
+	chip->parent = &pdev->dev;
+	chip->ngpio = pctrl->pctrl_data->ngpio;
+	chip->base = -1;
+	chip->request = upboard_gpio_request;
+	chip->free = upboard_gpio_free;
+	chip->get = upboard_gpio_get;
+	chip->set = upboard_gpio_set;
+	chip->get_direction = upboard_gpio_get_direction;
+	chip->direction_input = upboard_gpio_direction_input;
+	chip->direction_output = upboard_gpio_direction_output;
+	chip->to_irq = upboard_gpio_to_irq;
+
+	pctrl->gpio = devm_kcalloc(dev, pctrl->pctrl_data->ngpio, sizeof(*pctrl->gpio), GFP_KERNEL);
+	if (!pctrl->gpio)
+		return -ENOMEM;
+
+	ret = devm_gpiochip_add_data(&pdev->dev, chip, pctrl);
+	if (ret)
+		return ret;
+
+	ret = gpiochip_add_pinlist_range(chip, dev_name(dev), 0, pctrl->pctrl_data->pin_header,
+					 pctrl->pctrl_data->ngpio);
+	if (ret)
+		gpiochip_remove(chip);
+
+	return ret;
+}
+
+static struct platform_driver upboard_pinctrl_driver = {
+	.driver = {
+		.name = "upboard-pinctrl",
+	},
+	.probe = upboard_pinctrl_probe,
+};
+
+module_platform_driver(upboard_pinctrl_driver);
+
+MODULE_AUTHOR("Gary Wang <garywang@aaeon.com.tw>");
+MODULE_AUTHOR("Thomas Richard <thomas.richard@bootlin.com");
+MODULE_DESCRIPTION("UP Board HAT pin controller driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:upboard-pinctrl");

-- 
2.39.5

Re: [PATCH 4/5] pinctrl: Add pin controller driver for AAEON UP boards

[Linus Walleij]

Hi Thomas,

thanks for your patch!

On Wed, Dec 11, 2024 at 5:27 PM Thomas Richard
<thomas.richard@bootlin.com> wrote:

> This enables the pin control support of the onboard FPGA on AAEON UP
> boards.
> Due to the hardware design, the driver shall control its pins in tandem
> with their corresponding Intel SoC GPIOs.
>
> UP boards and UP Squared boards are supported.
>
> Based on the work done by Gary Wang <garywang@aaeon.com.tw>, largely
> rewritten.
>
> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>

Overall this looks as a good start, some comments below.

> +config PINCTRL_UPBOARD
> +       tristate "AAeon UP board FPGA pin controller"
> +       depends on MFD_UPBOARD_FPGA
> +       select PINMUX
> +       select GENERIC_PINCTRL_GROUPS
> +       select GENERIC_PINMUX_FUNCTIONS

This implements GPIO so you need:
select GPIOLIB

But I'm not sure because of some oddities, see below.

> +#include <linux/device.h>
> +#include <linux/gpio/consumer.h>

Questionable include, see below.

> +static int __upboard_pinctrl_gpio_request_enable(struct pinctrl_dev *pctldev,
> +                                                unsigned int offset)

I'm not a fan of functions named with __inner_function() double-underscore
convention. The reason is that double underscore is also used for
compiler intrinsics. Can you just name it

committ_upboard_pinctrl_gpio_request_enable()?

> +static void __upboard_pinctrl_gpio_disable_free(struct pinctrl_dev *pctldev, unsigned int offset)

Dito

> +static int __upboard_pinctrl_gpio_set_direction(struct pinctrl_dev *pctldev,
> +                                               unsigned int offset, bool input)

Dito

The pinmux code is very straight forward otherwise, good job!

> +static int upboard_gpio_get_direction(struct gpio_chip *gc, unsigned int offset)
> +{
> +       struct upboard_pinctrl *pctrl = container_of(gc, struct upboard_pinctrl, chip);
> +       unsigned int pin = pctrl->pctrl_data->pin_header[offset];
> +       int mode;
> +
> +       if (pctrl->gpio[offset])
> +               return gpiod_get_direction(pctrl->gpio[offset]);

See below.

> +       /*
> +        * GPIO was not requested so SoC pin is probably not in GPIO mode.
> +        * When a gpio_chip is registered, the core calls get_direction() for all lines.
> +        * At this time, upboard_gpio_request() was not yet called, so the driver didn't
> +        * request the corresponding SoC pin. So the SoC pin is probably in function (not in
> +        * GPIO mode).
> +        *
> +        * To get the direction of the SoC pin, it shall be requested in GPIO mode.
> +        * Once a SoC pin is set in GPIO mode, there is no way to set it back to its
> +        * function mode.
> +        * Instead of returning the SoC pin direction, the direction of the FPGA pin is
> +        * returned (only for the get_direction() called during the gpio_chip registration).
> +        */
> +       mode = upboard_pinctrl_pin_get_mode(pctrl->pctldev, pin);

Fair enough I guess it's the best we can do here.

> +static int upboard_gpio_get(struct gpio_chip *gc, unsigned int offset)
> +{
> +       struct upboard_pinctrl *pctrl = container_of(gc, struct upboard_pinctrl, chip);
> +
> +       return gpiod_get_value(pctrl->gpio[offset]);
> +}
> +
> +static void upboard_gpio_set(struct gpio_chip *gc, unsigned int offset, int value)
> +{
> +       struct upboard_pinctrl *pctrl = container_of(gc, struct upboard_pinctrl, chip);
> +
> +       gpiod_set_value(pctrl->gpio[offset], value);
> +}
> +
> +static int upboard_gpio_direction_input(struct gpio_chip *gc, unsigned int offset)
> +{
> +       struct upboard_pinctrl *pctrl = container_of(gc, struct upboard_pinctrl, chip);
> +       int ret;
> +
> +       ret = pinctrl_gpio_direction_input(gc, offset);
> +       if (ret)
> +               return ret;
> +
> +       return gpiod_direction_input(pctrl->gpio[offset]);
> +}
> +
> +static int upboard_gpio_direction_output(struct gpio_chip *gc, unsigned int offset, int value)
> +{
> +       struct upboard_pinctrl *pctrl = container_of(gc, struct upboard_pinctrl, chip);
> +       int ret;
> +
> +       ret = pinctrl_gpio_direction_output(gc, offset);
> +       if (ret)
> +               return ret;
> +
> +       return gpiod_direction_output(pctrl->gpio[offset], value);
> +}

This looks dangerous and I guess also the reason you are including consumer.h.

Explain with a comment in the code what is going on here, like if this
GPIO comes from a completely different hardware unit, it looks like
a recepie for an eternal loop if it would point back to the same GPIO.

All of these have the same "loop out to another hardware" feature
that looks weird to me, but explain what's going on so I understand
it.

To me usually pin control works like this:

linux gpio <-> gpio driver <-> pin control driver

so the pin control driver is a pure "backend" for GPIO,
typically implements in struct pinmux_ops:
.gpio_request_enable()
.gpio_disable_free()
.gpio_set_direction()

that just set up the pin in the corresponding way. If your hardware
cannot mux back a pin from GPIO mode (as a comment says)
I would say that gpio_disable_free() can just return -ENODEV
or something if the pin has been put into gpio mode, maybe
some experimentation is needed there.

The corresponding GPIO driver typically uses GPIO ranges
to access the corresponding pin. It usually call
gpiochip_add_pin_range() to map its pins to the pin control
driver (if e.g. device tree is not used for the ranges).

What you do here is confusing to me, it looks like:

linux gpio <-> this gpio shim <-> pin control <-> other gpio driver

I think it is better to try to keep things separate if you can,
the current design seems to come from an attempt to be
"complete" and protect users from themselves, but we can
never protect users from themselves.

> +static int upboard_gpio_to_irq(struct gpio_chip *gc, unsigned int offset)
> +{
> +       struct upboard_pinctrl *pctrl = container_of(gc, struct upboard_pinctrl, chip);
> +
> +       return gpiod_to_irq(pctrl->gpio[offset]);
> +}

If you use the GPIOLIB_IRQCHIP, you do not need to define this function
at all, it is handled by gpiolib.

> +       ret = gpiochip_add_pinlist_range(chip, dev_name(dev), 0, pctrl->pctrl_data->pin_header,
> +                                        pctrl->pctrl_data->ngpio);

I would rather have it that the actual gpio chip (the one that write
something into hardware registers) do this without another gpio chip
inbetween if you see what I mean.

But explain what's going on! I'm curious.

Yours,
Linus Walleij

Re: [PATCH 4/5] pinctrl: Add pin controller driver for AAEON UP boards

[Thomas Richard]

On 12/20/24 13:22, Linus Walleij wrote:
> Hi Thomas,
> 
> thanks for your patch!

Hi Linus,

Thanks for the review !!

> 
> On Wed, Dec 11, 2024 at 5:27 PM Thomas Richard
> <thomas.richard@bootlin.com> wrote:
> 
>> This enables the pin control support of the onboard FPGA on AAEON UP
>> boards.
>> Due to the hardware design, the driver shall control its pins in tandem
>> with their corresponding Intel SoC GPIOs.
>>
>> UP boards and UP Squared boards are supported.
>>
>> Based on the work done by Gary Wang <garywang@aaeon.com.tw>, largely
>> rewritten.
>>
>> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
> 
> Overall this looks as a good start, some comments below.
> 
>> +config PINCTRL_UPBOARD
>> +       tristate "AAeon UP board FPGA pin controller"
>> +       depends on MFD_UPBOARD_FPGA
>> +       select PINMUX
>> +       select GENERIC_PINCTRL_GROUPS
>> +       select GENERIC_PINMUX_FUNCTIONS
> 
> This implements GPIO so you need:
> select GPIOLIB
> 
> But I'm not sure because of some oddities, see below.
> 
>> +#include <linux/device.h>
>> +#include <linux/gpio/consumer.h>
> 
> Questionable include, see below.
> 
>> +static int __upboard_pinctrl_gpio_request_enable(struct pinctrl_dev *pctldev,
>> +                                                unsigned int offset)
> 
> I'm not a fan of functions named with __inner_function() double-underscore
> convention. The reason is that double underscore is also used for
> compiler intrinsics. Can you just name it
> 
> committ_upboard_pinctrl_gpio_request_enable()?

ack

> 
>> +static void __upboard_pinctrl_gpio_disable_free(struct pinctrl_dev *pctldev, unsigned int offset)
> 

[...]

>> +static int upboard_gpio_direction_output(struct gpio_chip *gc, unsigned int offset, int value)
>> +{
>> +       struct upboard_pinctrl *pctrl = container_of(gc, struct upboard_pinctrl, chip);
>> +       int ret;
>> +
>> +       ret = pinctrl_gpio_direction_output(gc, offset);
>> +       if (ret)
>> +               return ret;
>> +
>> +       return gpiod_direction_output(pctrl->gpio[offset], value);
>> +}
> 
> This looks dangerous and I guess also the reason you are including consumer.h.
> 
> Explain with a comment in the code what is going on here, like if this
> GPIO comes from a completely different hardware unit, it looks like
> a recepie for an eternal loop if it would point back to the same GPIO.
> 
> All of these have the same "loop out to another hardware" feature
> that looks weird to me, but explain what's going on so I understand
> it.
> 
> To me usually pin control works like this:
> 
> linux gpio <-> gpio driver <-> pin control driver
> 
> so the pin control driver is a pure "backend" for GPIO,
> typically implements in struct pinmux_ops:
> .gpio_request_enable()
> .gpio_disable_free()
> .gpio_set_direction()
> 
> that just set up the pin in the corresponding way. If your hardware
> cannot mux back a pin from GPIO mode (as a comment says)
> I would say that gpio_disable_free() can just return -ENODEV
> or something if the pin has been put into gpio mode, maybe
> some experimentation is needed there.
> 
> The corresponding GPIO driver typically uses GPIO ranges
> to access the corresponding pin. It usually call
> gpiochip_add_pin_range() to map its pins to the pin control
> driver (if e.g. device tree is not used for the ranges).
> 
> What you do here is confusing to me, it looks like:
> 
> linux gpio <-> this gpio shim <-> pin control <-> other gpio driver
> 
> I think it is better to try to keep things separate if you can,
> the current design seems to come from an attempt to be
> "complete" and protect users from themselves, but we can
> never protect users from themselves.
> 
>> +static int upboard_gpio_to_irq(struct gpio_chip *gc, unsigned int offset)
>> +{
>> +       struct upboard_pinctrl *pctrl = container_of(gc, struct upboard_pinctrl, chip);
>> +
>> +       return gpiod_to_irq(pctrl->gpio[offset]);
>> +}
> 
> If you use the GPIOLIB_IRQCHIP, you do not need to define this function
> at all, it is handled by gpiolib.
> 
>> +       ret = gpiochip_add_pinlist_range(chip, dev_name(dev), 0, pctrl->pctrl_data->pin_header,
>> +                                        pctrl->pctrl_data->ngpio);
> 
> I would rather have it that the actual gpio chip (the one that write
> something into hardware registers) do this without another gpio chip
> inbetween if you see what I mean.
> 
> But explain what's going on! I'm curious.

Yes my cover letter was a bit short, and maybe some context was missing.

This FPGA acts as a level shifter between the Intel SoC pins and the pin
header, and also makes a kind of switch/mux.

+---------+         +--------------+             +---+
          |         |              |             | H |
          |---------|              |-------------| E |
          |         |              |             | A |
Intel Soc |---------|    FPGA      |-------------| D |
          |         |              |             | E |
          |---------|              |-------------| R |
          |         |              |             |   |
----------+         +--------------+             +---+


For most of the pins, the FPGA opens/closes a switch to enable/disable
the access to the SoC pin from a pin header.
Each "switch", has a direction flag that shall be set in tandem with the
status of the SoC pin.
For example, if the SoC pin is in PWM mode, the "switch" shall be
configured in output direction.
If the SoC pin is set in GPIO mode, the direction of the "switch" shall
corresponds to the GPIO direction.

+---------+              +--------------+             +---+
          |              |              |             | H |
          |              |      \       |             | E |
          |   PWM1       |       \      |             | A |
Intel Soc |--------------|-----   \-----|-------------| D |
          |              |              |             | E |
          |              |              |             | R |
          |              |    FPGA      |             |   |
----------+              +--------------+             +---+

(PWM1 pin from Intel SoC can be used as PWM, and also in GPIO mode,
thanks to the Intel pinctrl driver).


Few pins (PINMUX_* pins) work differently. The FPGA acts as a mux and
routes for example the I2C0_SDA pin or GPIOX (of the SoC) to the pin header.

+---------+           +--------------+             +---+
          | I2C0_SDA  |              |             | H |
          |-----------|----- \       |             | E |
          |           |       \      |             | A |
Intel Soc |           |        \-----|-------------| D |
          | GPIOX     |              |             | E |
          |-----------|-----         |             | R |
          |           |    FPGA      |             |   |
----------+           +--------------+             +---+

The pin header looks like this:
+--------------------+--------------------+
|      3.3V	     |       5V           |
| GPIO2 / I2C1_SDA   |       5V           |
| GPIO3 / I2C1_SCL   |       GND          |
| GPIO4 / ADC0       | GPIO14 / UART1_TX  |
|      GND	     | GPIO15 / UART1_RX  |
| GPIO17 / UART1_RTS | GPIO18 / I2S_CLK   |
|     GPIO27         |       GND          |
|     GPIO22         |      GPIO23        |
|      3.3V          |      GPIO24        |
| GPIO10 / SPI_MOSI  |       GND          |
| GPIO9 / SPI_MISO   |      GPIO25        |
| GPIO11 / SPI_CLK   | GPIO8 / SPI_CS0    |
|      GND	     | GPIO7 / SPI_CS1    |
| GPIO0 / I2C0_SDA   | GPIO1 / I2C0_SCL   |
|     GPIO5          |       GND          |
|     GPIO6	     | GPIO12 / PWM0      |
| GPIO13 / PWM1      |       GND          |
| GPIO19 / I2S_FRM   | GPIO16 / UART1_CTS |
|     GPIO26	     | GPIO20 / I2S_DIN   |
|      GND	     | GPIO21 / I2S_DOUT  |
+--------------------+--------------------+

The GPIOs in the pin header corresponds to the gpiochip I declare in
this driver.
So when I want to use a pin in GPIO mode, the upboard pinctrl driver
requests the corresponding SoC GPIO to the Intel pinctrl driver.
The SoC pins connected to the FPGA, are identified with "external" id.

The hardware and the FPGA were designed in tandem, so you know for
example that for the GPIOX you need to request the Nth "external" GPIO.

When you drive your GPIO, the upboard gpiochip manages in the same time
the direction of the "switch" and the value/direction of the
corresponding SoC pin.

+------------------+         +--------------+             +---+
                   |---------|              |-------------| H |
                   |---------|   GPIOCHIP   |-------------| E |
   Intel gpiochip  |---------|              |-------------| A |
 provided by Intel |---------|    FPGA      |-------------| D |
  pinctrl driver   |---------|              |-------------| E |
                   |---------|              |-------------| R |
                   |---------|              |-------------|   |
+------------------+         +--------------+             +---+


About gpiochip_add_pinlist_range(), I added it because the FPGA pins
used by the gpiochip are not consecutive.

Please let me know if it is not clear.
And sorry I'm not very good to make ascii art.

Best Regards,

Thomas

Re: [PATCH 4/5] pinctrl: Add pin controller driver for AAEON UP boards

[Linus Walleij]

Hi Thomas,

thanks for your detailed reply!

On Fri, Dec 20, 2024 at 2:50 PM Thomas Richard
<thomas.richard@bootlin.com> wrote:

> Yes my cover letter was a bit short, and maybe some context was missing.

The text and graphics below explain it very well, so please include them
into the commit message so we have it there!

> This FPGA acts as a level shifter between the Intel SoC pins and the pin
> header, and also makes a kind of switch/mux.

Since it's Intel we need to notify Andy to help out with this so that
it gets done in a way that works with how he think consumers
should interact with Intel pin control and GPIO.

> +---------+         +--------------+             +---+
>           |         |              |             | H |
>           |---------|              |-------------| E |
>           |         |              |             | A |
> Intel Soc |---------|    FPGA      |-------------| D |
>           |         |              |             | E |
>           |---------|              |-------------| R |
>           |         |              |             |   |
> ----------+         +--------------+             +---+
>
>
> For most of the pins, the FPGA opens/closes a switch to enable/disable
> the access to the SoC pin from a pin header.
> Each "switch", has a direction flag that shall be set in tandem with the
> status of the SoC pin.
> For example, if the SoC pin is in PWM mode, the "switch" shall be
> configured in output direction.
> If the SoC pin is set in GPIO mode, the direction of the "switch" shall
> corresponds to the GPIO direction.
>
> +---------+              +--------------+             +---+
>           |              |              |             | H |
>           |              |      \       |             | E |
>           |   PWM1       |       \      |             | A |
> Intel Soc |--------------|-----   \-----|-------------| D |
>           |              |              |             | E |
>           |              |              |             | R |
>           |              |    FPGA      |             |   |
> ----------+              +--------------+             +---+
>
> (PWM1 pin from Intel SoC can be used as PWM, and also in GPIO mode,
> thanks to the Intel pinctrl driver).
>
>
> Few pins (PINMUX_* pins) work differently. The FPGA acts as a mux and
> routes for example the I2C0_SDA pin or GPIOX (of the SoC) to the pin header.
>
> +---------+           +--------------+             +---+
>           | I2C0_SDA  |              |             | H |
>           |-----------|----- \       |             | E |
>           |           |       \      |             | A |
> Intel Soc |           |        \-----|-------------| D |
>           | GPIOX     |              |             | E |
>           |-----------|-----         |             | R |
>           |           |    FPGA      |             |   |
> ----------+           +--------------+             +---+
>
> The pin header looks like this:
> +--------------------+--------------------+
> |      3.3V          |       5V           |
> | GPIO2 / I2C1_SDA   |       5V           |
> | GPIO3 / I2C1_SCL   |       GND          |
> | GPIO4 / ADC0       | GPIO14 / UART1_TX  |
> |      GND           | GPIO15 / UART1_RX  |
> | GPIO17 / UART1_RTS | GPIO18 / I2S_CLK   |
> |     GPIO27         |       GND          |
> |     GPIO22         |      GPIO23        |
> |      3.3V          |      GPIO24        |
> | GPIO10 / SPI_MOSI  |       GND          |
> | GPIO9 / SPI_MISO   |      GPIO25        |
> | GPIO11 / SPI_CLK   | GPIO8 / SPI_CS0    |
> |      GND           | GPIO7 / SPI_CS1    |
> | GPIO0 / I2C0_SDA   | GPIO1 / I2C0_SCL   |
> |     GPIO5          |       GND          |
> |     GPIO6          | GPIO12 / PWM0      |
> | GPIO13 / PWM1      |       GND          |
> | GPIO19 / I2S_FRM   | GPIO16 / UART1_CTS |
> |     GPIO26         | GPIO20 / I2S_DIN   |
> |      GND           | GPIO21 / I2S_DOUT  |
> +--------------------+--------------------+
>
> The GPIOs in the pin header corresponds to the gpiochip I declare in
> this driver.
> So when I want to use a pin in GPIO mode, the upboard pinctrl driver
> requests the corresponding SoC GPIO to the Intel pinctrl driver.
> The SoC pins connected to the FPGA, are identified with "external" id.
>
> The hardware and the FPGA were designed in tandem, so you know for
> example that for the GPIOX you need to request the Nth "external" GPIO.
>
> When you drive your GPIO, the upboard gpiochip manages in the same time
> the direction of the "switch" and the value/direction of the
> corresponding SoC pin.
>
> +------------------+         +--------------+             +---+
>                    |---------|              |-------------| H |
>                    |---------|   GPIOCHIP   |-------------| E |
>    Intel gpiochip  |---------|              |-------------| A |
>  provided by Intel |---------|    FPGA      |-------------| D |
>   pinctrl driver   |---------|              |-------------| E |
>                    |---------|              |-------------| R |
>                    |---------|              |-------------|   |
> +------------------+         +--------------+             +---+
>
>
> About gpiochip_add_pinlist_range(), I added it because the FPGA pins
> used by the gpiochip are not consecutive.
>
> Please let me know if it is not clear.
> And sorry I'm not very good to make ascii art.

I get it! We have a similar driver in the kernel already, look into:
drivers/gpio/gpio-aggregator.c

The aggregator abstraction is however just software. What you
need here is a gpio-aggregator that adds some hardware
control on top. But it has a very nice design using a bitmap
to keep track of the GPIOs etc, and it supports operations
on multiple GPIOs (many man-hours of hard coding and
design went into that driver, ask Geert and Andy...)

So I would proceed like this:

- The pin control part of the driver looks sound, except
  for the way you add ranges.

- The gpiochip part needs to be refactored using the
  ideas from gpio-aggregator.c.

- Look closely at aggregator and see what you can do
  based on that code, if you can mimic how it picks up
  and forwards all GPIO functions. Maybe part of it
  needs to be made into a library?
 <linux/gpio/gpio-aggregator.h>?
  For example if you start to feel like "I would really like
  to just call gpio_fwd_get_multiple() then this is what
  you want to do. The library can probably still be
  inside gpio-aggregator.c the way we do it in
  e.g. gpio-mmio.c, just export and keep library functions
  separately.

- The way you split up gpiochip_add_pin_range() I
  still do not understand at all, in my view you just want
  this gpiochip to refer to the pin controller pins in the
  same file so I don't see it. How can e.g.
  pinctrl-sx150x.c do this trick but not your driver?
  I might be missing something though.

Yours,
Linus Walleij

Re: [PATCH 4/5] pinctrl: Add pin controller driver for AAEON UP boards

[Thomas Richard]

On 12/22/24 00:43, Linus Walleij wrote:
> Hi Thomas,
> 
> thanks for your detailed reply!
> 
> On Fri, Dec 20, 2024 at 2:50 PM Thomas Richard
> <thomas.richard@bootlin.com> wrote:
> 
>> Yes my cover letter was a bit short, and maybe some context was missing.
> 
> The text and graphics below explain it very well, so please include them
> into the commit message so we have it there!
> 
>> This FPGA acts as a level shifter between the Intel SoC pins and the pin
>> header, and also makes a kind of switch/mux.
> 
> Since it's Intel we need to notify Andy to help out with this so that
> it gets done in a way that works with how he think consumers
> should interact with Intel pin control and GPIO.
> 
>> +---------+         +--------------+             +---+
>>           |         |              |             | H |
>>           |---------|              |-------------| E |
>>           |         |              |             | A |
>> Intel Soc |---------|    FPGA      |-------------| D |
>>           |         |              |             | E |
>>           |---------|              |-------------| R |
>>           |         |              |             |   |
>> ----------+         +--------------+             +---+
>>
>>
>> For most of the pins, the FPGA opens/closes a switch to enable/disable
>> the access to the SoC pin from a pin header.
>> Each "switch", has a direction flag that shall be set in tandem with the
>> status of the SoC pin.
>> For example, if the SoC pin is in PWM mode, the "switch" shall be
>> configured in output direction.
>> If the SoC pin is set in GPIO mode, the direction of the "switch" shall
>> corresponds to the GPIO direction.
>>
>> +---------+              +--------------+             +---+
>>           |              |              |             | H |
>>           |              |      \       |             | E |
>>           |   PWM1       |       \      |             | A |
>> Intel Soc |--------------|-----   \-----|-------------| D |
>>           |              |              |             | E |
>>           |              |              |             | R |
>>           |              |    FPGA      |             |   |
>> ----------+              +--------------+             +---+
>>
>> (PWM1 pin from Intel SoC can be used as PWM, and also in GPIO mode,
>> thanks to the Intel pinctrl driver).
>>
>>
>> Few pins (PINMUX_* pins) work differently. The FPGA acts as a mux and
>> routes for example the I2C0_SDA pin or GPIOX (of the SoC) to the pin header.
>>
>> +---------+           +--------------+             +---+
>>           | I2C0_SDA  |              |             | H |
>>           |-----------|----- \       |             | E |
>>           |           |       \      |             | A |
>> Intel Soc |           |        \-----|-------------| D |
>>           | GPIOX     |              |             | E |
>>           |-----------|-----         |             | R |
>>           |           |    FPGA      |             |   |
>> ----------+           +--------------+             +---+
>>
>> The pin header looks like this:
>> +--------------------+--------------------+
>> |      3.3V          |       5V           |
>> | GPIO2 / I2C1_SDA   |       5V           |
>> | GPIO3 / I2C1_SCL   |       GND          |
>> | GPIO4 / ADC0       | GPIO14 / UART1_TX  |
>> |      GND           | GPIO15 / UART1_RX  |
>> | GPIO17 / UART1_RTS | GPIO18 / I2S_CLK   |
>> |     GPIO27         |       GND          |
>> |     GPIO22         |      GPIO23        |
>> |      3.3V          |      GPIO24        |
>> | GPIO10 / SPI_MOSI  |       GND          |
>> | GPIO9 / SPI_MISO   |      GPIO25        |
>> | GPIO11 / SPI_CLK   | GPIO8 / SPI_CS0    |
>> |      GND           | GPIO7 / SPI_CS1    |
>> | GPIO0 / I2C0_SDA   | GPIO1 / I2C0_SCL   |
>> |     GPIO5          |       GND          |
>> |     GPIO6          | GPIO12 / PWM0      |
>> | GPIO13 / PWM1      |       GND          |
>> | GPIO19 / I2S_FRM   | GPIO16 / UART1_CTS |
>> |     GPIO26         | GPIO20 / I2S_DIN   |
>> |      GND           | GPIO21 / I2S_DOUT  |
>> +--------------------+--------------------+
>>
>> The GPIOs in the pin header corresponds to the gpiochip I declare in
>> this driver.
>> So when I want to use a pin in GPIO mode, the upboard pinctrl driver
>> requests the corresponding SoC GPIO to the Intel pinctrl driver.
>> The SoC pins connected to the FPGA, are identified with "external" id.
>>
>> The hardware and the FPGA were designed in tandem, so you know for
>> example that for the GPIOX you need to request the Nth "external" GPIO.
>>
>> When you drive your GPIO, the upboard gpiochip manages in the same time
>> the direction of the "switch" and the value/direction of the
>> corresponding SoC pin.
>>
>> +------------------+         +--------------+             +---+
>>                    |---------|              |-------------| H |
>>                    |---------|   GPIOCHIP   |-------------| E |
>>    Intel gpiochip  |---------|              |-------------| A |
>>  provided by Intel |---------|    FPGA      |-------------| D |
>>   pinctrl driver   |---------|              |-------------| E |
>>                    |---------|              |-------------| R |
>>                    |---------|              |-------------|   |
>> +------------------+         +--------------+             +---+
>>
>>
>> About gpiochip_add_pinlist_range(), I added it because the FPGA pins
>> used by the gpiochip are not consecutive.
>>
>> Please let me know if it is not clear.
>> And sorry I'm not very good to make ascii art.
> 
> I get it! We have a similar driver in the kernel already, look into:
> drivers/gpio/gpio-aggregator.c
> 
> The aggregator abstraction is however just software. What you
> need here is a gpio-aggregator that adds some hardware
> control on top. But it has a very nice design using a bitmap
> to keep track of the GPIOs etc, and it supports operations
> on multiple GPIOs (many man-hours of hard coding and
> design went into that driver, ask Geert and Andy...)
> 
> So I would proceed like this:
> 
> - The pin control part of the driver looks sound, except
>   for the way you add ranges.
> 
> - The gpiochip part needs to be refactored using the
>   ideas from gpio-aggregator.c.
> 
> - Look closely at aggregator and see what you can do
>   based on that code, if you can mimic how it picks up
>   and forwards all GPIO functions. Maybe part of it
>   needs to be made into a library?
>  <linux/gpio/gpio-aggregator.h>?
>   For example if you start to feel like "I would really like
>   to just call gpio_fwd_get_multiple() then this is what
>   you want to do. The library can probably still be
>   inside gpio-aggregator.c the way we do it in
>   e.g. gpio-mmio.c, just export and keep library functions
>   separately.

Hi Linus,

Ok I think I understand what you expect.
I started to look at the gpio-aggregator code, play a bit with it, and
refactor it to use it from my driver.

My main issue is about the request of the SoC GPIOs done by the aggregator.
If from my driver I call the aggregator library to create a gpiochip,
the SoC pins will be requested. So the SoC pins will be set in GPIO
mode, and the pins will never be in function mode.
There is no way to set the pins back to function mode (even if the GPIO
is free).

I tried to add a feature in the aggregator to defer the request of the gpio.
So at the beginning of each ops the gpio_desc is checked. If it is
valid, the gpio can be used. Otherwise, the gpio is requested.
For example:

gpio_fwd_get() {
	if (!gpio_desc_is_valid(desc))
		desc = request_gpio()

	return gpiod_get_value(desc)
}

But when a gpiochip is registered, the core calls get_direction() or
direction_input(), so all GPIOs are requested and it does not solve my
problem.

I expect to register a gpiochip without setting all pins in GPIO mode at
probe time (like all pinctrl driver do).
But I did not find a solution.

Best Regards,

Thomas

Re: [PATCH 4/5] pinctrl: Add pin controller driver for AAEON UP boards

[Andy Shevchenko]

On Fri, Jan 03, 2025 at 11:28:30AM +0100, Thomas Richard wrote:
> On 12/22/24 00:43, Linus Walleij wrote:
> > Hi Thomas,
> > 
> > thanks for your detailed reply!
> > 
> > On Fri, Dec 20, 2024 at 2:50 PM Thomas Richard
> > <thomas.richard@bootlin.com> wrote:
> > 
> >> Yes my cover letter was a bit short, and maybe some context was missing.
> > 
> > The text and graphics below explain it very well, so please include them
> > into the commit message so we have it there!
> > 
> >> This FPGA acts as a level shifter between the Intel SoC pins and the pin
> >> header, and also makes a kind of switch/mux.
> > 
> > Since it's Intel we need to notify Andy to help out with this so that
> > it gets done in a way that works with how he think consumers
> > should interact with Intel pin control and GPIO.
> > 
> >> +---------+         +--------------+             +---+
> >>           |         |              |             | H |
> >>           |---------|              |-------------| E |
> >>           |         |              |             | A |
> >> Intel Soc |---------|    FPGA      |-------------| D |
> >>           |         |              |             | E |
> >>           |---------|              |-------------| R |
> >>           |         |              |             |   |
> >> ----------+         +--------------+             +---+
> >>
> >>
> >> For most of the pins, the FPGA opens/closes a switch to enable/disable
> >> the access to the SoC pin from a pin header.
> >> Each "switch", has a direction flag that shall be set in tandem with the
> >> status of the SoC pin.
> >> For example, if the SoC pin is in PWM mode, the "switch" shall be
> >> configured in output direction.
> >> If the SoC pin is set in GPIO mode, the direction of the "switch" shall
> >> corresponds to the GPIO direction.
> >>
> >> +---------+              +--------------+             +---+
> >>           |              |              |             | H |
> >>           |              |      \       |             | E |
> >>           |   PWM1       |       \      |             | A |
> >> Intel Soc |--------------|-----   \-----|-------------| D |
> >>           |              |              |             | E |
> >>           |              |              |             | R |
> >>           |              |    FPGA      |             |   |
> >> ----------+              +--------------+             +---+
> >>
> >> (PWM1 pin from Intel SoC can be used as PWM, and also in GPIO mode,
> >> thanks to the Intel pinctrl driver).
> >>
> >>
> >> Few pins (PINMUX_* pins) work differently. The FPGA acts as a mux and
> >> routes for example the I2C0_SDA pin or GPIOX (of the SoC) to the pin header.
> >>
> >> +---------+           +--------------+             +---+
> >>           | I2C0_SDA  |              |             | H |
> >>           |-----------|----- \       |             | E |
> >>           |           |       \      |             | A |
> >> Intel Soc |           |        \-----|-------------| D |
> >>           | GPIOX     |              |             | E |
> >>           |-----------|-----         |             | R |
> >>           |           |    FPGA      |             |   |
> >> ----------+           +--------------+             +---+
> >>
> >> The pin header looks like this:
> >> +--------------------+--------------------+
> >> |      3.3V          |       5V           |
> >> | GPIO2 / I2C1_SDA   |       5V           |
> >> | GPIO3 / I2C1_SCL   |       GND          |
> >> | GPIO4 / ADC0       | GPIO14 / UART1_TX  |
> >> |      GND           | GPIO15 / UART1_RX  |
> >> | GPIO17 / UART1_RTS | GPIO18 / I2S_CLK   |
> >> |     GPIO27         |       GND          |
> >> |     GPIO22         |      GPIO23        |
> >> |      3.3V          |      GPIO24        |
> >> | GPIO10 / SPI_MOSI  |       GND          |
> >> | GPIO9 / SPI_MISO   |      GPIO25        |
> >> | GPIO11 / SPI_CLK   | GPIO8 / SPI_CS0    |
> >> |      GND           | GPIO7 / SPI_CS1    |
> >> | GPIO0 / I2C0_SDA   | GPIO1 / I2C0_SCL   |
> >> |     GPIO5          |       GND          |
> >> |     GPIO6          | GPIO12 / PWM0      |
> >> | GPIO13 / PWM1      |       GND          |
> >> | GPIO19 / I2S_FRM   | GPIO16 / UART1_CTS |
> >> |     GPIO26         | GPIO20 / I2S_DIN   |
> >> |      GND           | GPIO21 / I2S_DOUT  |
> >> +--------------------+--------------------+
> >>
> >> The GPIOs in the pin header corresponds to the gpiochip I declare in
> >> this driver.
> >> So when I want to use a pin in GPIO mode, the upboard pinctrl driver
> >> requests the corresponding SoC GPIO to the Intel pinctrl driver.
> >> The SoC pins connected to the FPGA, are identified with "external" id.
> >>
> >> The hardware and the FPGA were designed in tandem, so you know for
> >> example that for the GPIOX you need to request the Nth "external" GPIO.
> >>
> >> When you drive your GPIO, the upboard gpiochip manages in the same time
> >> the direction of the "switch" and the value/direction of the
> >> corresponding SoC pin.
> >>
> >> +------------------+         +--------------+             +---+
> >>                    |---------|              |-------------| H |
> >>                    |---------|   GPIOCHIP   |-------------| E |
> >>    Intel gpiochip  |---------|              |-------------| A |
> >>  provided by Intel |---------|    FPGA      |-------------| D |
> >>   pinctrl driver   |---------|              |-------------| E |
> >>                    |---------|              |-------------| R |
> >>                    |---------|              |-------------|   |
> >> +------------------+         +--------------+             +---+
> >>
> >>
> >> About gpiochip_add_pinlist_range(), I added it because the FPGA pins
> >> used by the gpiochip are not consecutive.
> >>
> >> Please let me know if it is not clear.
> >> And sorry I'm not very good to make ascii art.
> > 
> > I get it! We have a similar driver in the kernel already, look into:
> > drivers/gpio/gpio-aggregator.c
> > 
> > The aggregator abstraction is however just software. What you
> > need here is a gpio-aggregator that adds some hardware
> > control on top. But it has a very nice design using a bitmap
> > to keep track of the GPIOs etc, and it supports operations
> > on multiple GPIOs (many man-hours of hard coding and
> > design went into that driver, ask Geert and Andy...)
> > 
> > So I would proceed like this:
> > 
> > - The pin control part of the driver looks sound, except
> >   for the way you add ranges.
> > 
> > - The gpiochip part needs to be refactored using the
> >   ideas from gpio-aggregator.c.
> > 
> > - Look closely at aggregator and see what you can do
> >   based on that code, if you can mimic how it picks up
> >   and forwards all GPIO functions. Maybe part of it
> >   needs to be made into a library?
> >  <linux/gpio/gpio-aggregator.h>?
> >   For example if you start to feel like "I would really like
> >   to just call gpio_fwd_get_multiple() then this is what
> >   you want to do. The library can probably still be
> >   inside gpio-aggregator.c the way we do it in
> >   e.g. gpio-mmio.c, just export and keep library functions
> >   separately.
> 
> Hi Linus,
> 
> Ok I think I understand what you expect.
> I started to look at the gpio-aggregator code, play a bit with it, and
> refactor it to use it from my driver.
> 
> My main issue is about the request of the SoC GPIOs done by the aggregator.
> If from my driver I call the aggregator library to create a gpiochip,
> the SoC pins will be requested. So the SoC pins will be set in GPIO
> mode, and the pins will never be in function mode.
> There is no way to set the pins back to function mode (even if the GPIO
> is free).
> 
> I tried to add a feature in the aggregator to defer the request of the gpio.
> So at the beginning of each ops the gpio_desc is checked. If it is
> valid, the gpio can be used. Otherwise, the gpio is requested.
> For example:
> 
> gpio_fwd_get() {
> 	if (!gpio_desc_is_valid(desc))
> 		desc = request_gpio()
> 
> 	return gpiod_get_value(desc)
> }
> 
> But when a gpiochip is registered, the core calls get_direction() or
> direction_input(), so all GPIOs are requested and it does not solve my
> problem.
> 
> I expect to register a gpiochip without setting all pins in GPIO mode at
> probe time (like all pinctrl driver do).
> But I did not find a solution.

Basically what you need is a pinctrl-aggregattor (an analogue for the pin
muxing and configuration).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 4/5] pinctrl: Add pin controller driver for AAEON UP boards

[Thomas Richard]

On 1/13/25 10:46, Andy Shevchenko wrote:
> On Fri, Jan 03, 2025 at 11:28:30AM +0100, Thomas Richard wrote:
>> On 12/22/24 00:43, Linus Walleij wrote:
>>> Hi Thomas,
>>>
>>> thanks for your detailed reply!
>>>
>>> On Fri, Dec 20, 2024 at 2:50 PM Thomas Richard
>>> <thomas.richard@bootlin.com> wrote:
>>>
>>>> Yes my cover letter was a bit short, and maybe some context was missing.
>>>
>>> The text and graphics below explain it very well, so please include them
>>> into the commit message so we have it there!
>>>
>>>> This FPGA acts as a level shifter between the Intel SoC pins and the pin
>>>> header, and also makes a kind of switch/mux.
>>>
>>> Since it's Intel we need to notify Andy to help out with this so that
>>> it gets done in a way that works with how he think consumers
>>> should interact with Intel pin control and GPIO.
>>>
>>>> +---------+         +--------------+             +---+
>>>>           |         |              |             | H |
>>>>           |---------|              |-------------| E |
>>>>           |         |              |             | A |
>>>> Intel Soc |---------|    FPGA      |-------------| D |
>>>>           |         |              |             | E |
>>>>           |---------|              |-------------| R |
>>>>           |         |              |             |   |
>>>> ----------+         +--------------+             +---+
>>>>
>>>>
>>>> For most of the pins, the FPGA opens/closes a switch to enable/disable
>>>> the access to the SoC pin from a pin header.
>>>> Each "switch", has a direction flag that shall be set in tandem with the
>>>> status of the SoC pin.
>>>> For example, if the SoC pin is in PWM mode, the "switch" shall be
>>>> configured in output direction.
>>>> If the SoC pin is set in GPIO mode, the direction of the "switch" shall
>>>> corresponds to the GPIO direction.
>>>>
>>>> +---------+              +--------------+             +---+
>>>>           |              |              |             | H |
>>>>           |              |      \       |             | E |
>>>>           |   PWM1       |       \      |             | A |
>>>> Intel Soc |--------------|-----   \-----|-------------| D |
>>>>           |              |              |             | E |
>>>>           |              |              |             | R |
>>>>           |              |    FPGA      |             |   |
>>>> ----------+              +--------------+             +---+
>>>>
>>>> (PWM1 pin from Intel SoC can be used as PWM, and also in GPIO mode,
>>>> thanks to the Intel pinctrl driver).
>>>>
>>>>
>>>> Few pins (PINMUX_* pins) work differently. The FPGA acts as a mux and
>>>> routes for example the I2C0_SDA pin or GPIOX (of the SoC) to the pin header.
>>>>
>>>> +---------+           +--------------+             +---+
>>>>           | I2C0_SDA  |              |             | H |
>>>>           |-----------|----- \       |             | E |
>>>>           |           |       \      |             | A |
>>>> Intel Soc |           |        \-----|-------------| D |
>>>>           | GPIOX     |              |             | E |
>>>>           |-----------|-----         |             | R |
>>>>           |           |    FPGA      |             |   |
>>>> ----------+           +--------------+             +---+
>>>>
>>>> The pin header looks like this:
>>>> +--------------------+--------------------+
>>>> |      3.3V          |       5V           |
>>>> | GPIO2 / I2C1_SDA   |       5V           |
>>>> | GPIO3 / I2C1_SCL   |       GND          |
>>>> | GPIO4 / ADC0       | GPIO14 / UART1_TX  |
>>>> |      GND           | GPIO15 / UART1_RX  |
>>>> | GPIO17 / UART1_RTS | GPIO18 / I2S_CLK   |
>>>> |     GPIO27         |       GND          |
>>>> |     GPIO22         |      GPIO23        |
>>>> |      3.3V          |      GPIO24        |
>>>> | GPIO10 / SPI_MOSI  |       GND          |
>>>> | GPIO9 / SPI_MISO   |      GPIO25        |
>>>> | GPIO11 / SPI_CLK   | GPIO8 / SPI_CS0    |
>>>> |      GND           | GPIO7 / SPI_CS1    |
>>>> | GPIO0 / I2C0_SDA   | GPIO1 / I2C0_SCL   |
>>>> |     GPIO5          |       GND          |
>>>> |     GPIO6          | GPIO12 / PWM0      |
>>>> | GPIO13 / PWM1      |       GND          |
>>>> | GPIO19 / I2S_FRM   | GPIO16 / UART1_CTS |
>>>> |     GPIO26         | GPIO20 / I2S_DIN   |
>>>> |      GND           | GPIO21 / I2S_DOUT  |
>>>> +--------------------+--------------------+
>>>>
>>>> The GPIOs in the pin header corresponds to the gpiochip I declare in
>>>> this driver.
>>>> So when I want to use a pin in GPIO mode, the upboard pinctrl driver
>>>> requests the corresponding SoC GPIO to the Intel pinctrl driver.
>>>> The SoC pins connected to the FPGA, are identified with "external" id.
>>>>
>>>> The hardware and the FPGA were designed in tandem, so you know for
>>>> example that for the GPIOX you need to request the Nth "external" GPIO.
>>>>
>>>> When you drive your GPIO, the upboard gpiochip manages in the same time
>>>> the direction of the "switch" and the value/direction of the
>>>> corresponding SoC pin.
>>>>
>>>> +------------------+         +--------------+             +---+
>>>>                    |---------|              |-------------| H |
>>>>                    |---------|   GPIOCHIP   |-------------| E |
>>>>    Intel gpiochip  |---------|              |-------------| A |
>>>>  provided by Intel |---------|    FPGA      |-------------| D |
>>>>   pinctrl driver   |---------|              |-------------| E |
>>>>                    |---------|              |-------------| R |
>>>>                    |---------|              |-------------|   |
>>>> +------------------+         +--------------+             +---+
>>>>
>>>>
>>>> About gpiochip_add_pinlist_range(), I added it because the FPGA pins
>>>> used by the gpiochip are not consecutive.
>>>>
>>>> Please let me know if it is not clear.
>>>> And sorry I'm not very good to make ascii art.
>>>
>>> I get it! We have a similar driver in the kernel already, look into:
>>> drivers/gpio/gpio-aggregator.c
>>>
>>> The aggregator abstraction is however just software. What you
>>> need here is a gpio-aggregator that adds some hardware
>>> control on top. But it has a very nice design using a bitmap
>>> to keep track of the GPIOs etc, and it supports operations
>>> on multiple GPIOs (many man-hours of hard coding and
>>> design went into that driver, ask Geert and Andy...)
>>>
>>> So I would proceed like this:
>>>
>>> - The pin control part of the driver looks sound, except
>>>   for the way you add ranges.
>>>
>>> - The gpiochip part needs to be refactored using the
>>>   ideas from gpio-aggregator.c.
>>>
>>> - Look closely at aggregator and see what you can do
>>>   based on that code, if you can mimic how it picks up
>>>   and forwards all GPIO functions. Maybe part of it
>>>   needs to be made into a library?
>>>  <linux/gpio/gpio-aggregator.h>?
>>>   For example if you start to feel like "I would really like
>>>   to just call gpio_fwd_get_multiple() then this is what
>>>   you want to do. The library can probably still be
>>>   inside gpio-aggregator.c the way we do it in
>>>   e.g. gpio-mmio.c, just export and keep library functions
>>>   separately.
>>
>> Hi Linus,
>>
>> Ok I think I understand what you expect.
>> I started to look at the gpio-aggregator code, play a bit with it, and
>> refactor it to use it from my driver.
>>
>> My main issue is about the request of the SoC GPIOs done by the aggregator.
>> If from my driver I call the aggregator library to create a gpiochip,
>> the SoC pins will be requested. So the SoC pins will be set in GPIO
>> mode, and the pins will never be in function mode.
>> There is no way to set the pins back to function mode (even if the GPIO
>> is free).
>>
>> I tried to add a feature in the aggregator to defer the request of the gpio.
>> So at the beginning of each ops the gpio_desc is checked. If it is
>> valid, the gpio can be used. Otherwise, the gpio is requested.
>> For example:
>>
>> gpio_fwd_get() {
>> 	if (!gpio_desc_is_valid(desc))
>> 		desc = request_gpio()
>>
>> 	return gpiod_get_value(desc)
>> }
>>
>> But when a gpiochip is registered, the core calls get_direction() or
>> direction_input(), so all GPIOs are requested and it does not solve my
>> problem.
>>
>> I expect to register a gpiochip without setting all pins in GPIO mode at
>> probe time (like all pinctrl driver do).
>> But I did not find a solution.
> 
> Basically what you need is a pinctrl-aggregattor (an analogue for the pin
> muxing and configuration).
> 

Hi Andy,

I found a trick to workaround the get_direction() issue in the
gpio-aggregator.

I added a "request on first use" feature on the aggregator.
The GPIO is requested during the request() operation of the fowarder.

static int gpio_fwd_request(struct gpio_chip *chip, unsigned int offset)
{
	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);

	if (!IS_ERR_OR_NULL(fwd->descs[offset]))
		return 0;

	fwd->descs[offset] = devm_gpiod_get_index(fwd->dev, NULL,
						offset, GPIOD_ASIS);

	return PTR_ERR_OR_ZERO(fwd->descs[offset]);
}

The remaining problem is that the get_direction() callback is called
during gpiochip registration. For now if the gpio_desc is not valid (so
the GPIO was not yet requested) I return GPIO_LINE_DIRECTION_OUT by
default. But I'm not very convinced by this hack.
Maybe I could retrieve the gpio_chip and call its get_direction()
callback, but it seems not to be a better idea.

For the pinctrl-aggregator you mentioned, if I understand correctly the
idea to aggregate the SoC pins in a pinctrl aggregator (with a
gpio_chip) which just forwards gpio_request_enable(),
gpio_disable_free(), gpio_set_direction() and also all gpio_chip operations.

But how to deal with the pinctrl of my FPGA ? For one of its fake pin
the dummy pinctrl drives the corresponding SoC pin and FPGA pin ? So for
each pin, the aggregator may have multiple pins to drive ?

Was it your idea Andy ?

Other challenge is to retrieve all the pins to add in the
pinctrl-aggregator. As input I have only GPIO descriptors, but I guess
it should be feasible.

Best Regards,

Thomas

Re: [PATCH 4/5] pinctrl: Add pin controller driver for AAEON UP boards

[Andy Shevchenko]

On Tue, Jan 14, 2025 at 11:28:26AM +0100, Thomas Richard wrote:
> On 1/13/25 10:46, Andy Shevchenko wrote:
> > On Fri, Jan 03, 2025 at 11:28:30AM +0100, Thomas Richard wrote:
> >> On 12/22/24 00:43, Linus Walleij wrote:
> >>> On Fri, Dec 20, 2024 at 2:50 PM Thomas Richard
> >>> <thomas.richard@bootlin.com> wrote:
> >>>
> >>>> Yes my cover letter was a bit short, and maybe some context was missing.
> >>>
> >>> The text and graphics below explain it very well, so please include them
> >>> into the commit message so we have it there!
> >>>
> >>>> This FPGA acts as a level shifter between the Intel SoC pins and the pin
> >>>> header, and also makes a kind of switch/mux.
> >>>
> >>> Since it's Intel we need to notify Andy to help out with this so that
> >>> it gets done in a way that works with how he think consumers
> >>> should interact with Intel pin control and GPIO.
> >>>
> >>>> +---------+         +--------------+             +---+
> >>>>           |         |              |             | H |
> >>>>           |---------|              |-------------| E |
> >>>>           |         |              |             | A |
> >>>> Intel Soc |---------|    FPGA      |-------------| D |
> >>>>           |         |              |             | E |
> >>>>           |---------|              |-------------| R |
> >>>>           |         |              |             |   |
> >>>> ----------+         +--------------+             +---+
> >>>>
> >>>>
> >>>> For most of the pins, the FPGA opens/closes a switch to enable/disable
> >>>> the access to the SoC pin from a pin header.
> >>>> Each "switch", has a direction flag that shall be set in tandem with the
> >>>> status of the SoC pin.
> >>>> For example, if the SoC pin is in PWM mode, the "switch" shall be
> >>>> configured in output direction.
> >>>> If the SoC pin is set in GPIO mode, the direction of the "switch" shall
> >>>> corresponds to the GPIO direction.
> >>>>
> >>>> +---------+              +--------------+             +---+
> >>>>           |              |              |             | H |
> >>>>           |              |      \       |             | E |
> >>>>           |   PWM1       |       \      |             | A |
> >>>> Intel Soc |--------------|-----   \-----|-------------| D |
> >>>>           |              |              |             | E |
> >>>>           |              |              |             | R |
> >>>>           |              |    FPGA      |             |   |
> >>>> ----------+              +--------------+             +---+
> >>>>
> >>>> (PWM1 pin from Intel SoC can be used as PWM, and also in GPIO mode,
> >>>> thanks to the Intel pinctrl driver).
> >>>>
> >>>> Few pins (PINMUX_* pins) work differently. The FPGA acts as a mux and
> >>>> routes for example the I2C0_SDA pin or GPIOX (of the SoC) to the pin header.

Yep, that's clear.

> >>>> +---------+           +--------------+             +---+
> >>>>           | I2C0_SDA  |              |             | H |
> >>>>           |-----------|----- \       |             | E |
> >>>>           |           |       \      |             | A |
> >>>> Intel Soc |           |        \-----|-------------| D |
> >>>>           | GPIOX     |              |             | E |
> >>>>           |-----------|-----         |             | R |
> >>>>           |           |    FPGA      |             |   |
> >>>> ----------+           +--------------+             +---+
> >>>>
> >>>> The pin header looks like this:
> >>>> +--------------------+--------------------+
> >>>> |      3.3V          |       5V           |
> >>>> | GPIO2 / I2C1_SDA   |       5V           |
> >>>> | GPIO3 / I2C1_SCL   |       GND          |
> >>>> | GPIO4 / ADC0       | GPIO14 / UART1_TX  |
> >>>> |      GND           | GPIO15 / UART1_RX  |
> >>>> | GPIO17 / UART1_RTS | GPIO18 / I2S_CLK   |
> >>>> |     GPIO27         |       GND          |
> >>>> |     GPIO22         |      GPIO23        |
> >>>> |      3.3V          |      GPIO24        |
> >>>> | GPIO10 / SPI_MOSI  |       GND          |
> >>>> | GPIO9 / SPI_MISO   |      GPIO25        |
> >>>> | GPIO11 / SPI_CLK   | GPIO8 / SPI_CS0    |
> >>>> |      GND           | GPIO7 / SPI_CS1    |
> >>>> | GPIO0 / I2C0_SDA   | GPIO1 / I2C0_SCL   |
> >>>> |     GPIO5          |       GND          |
> >>>> |     GPIO6          | GPIO12 / PWM0      |
> >>>> | GPIO13 / PWM1      |       GND          |
> >>>> | GPIO19 / I2S_FRM   | GPIO16 / UART1_CTS |
> >>>> |     GPIO26         | GPIO20 / I2S_DIN   |
> >>>> |      GND           | GPIO21 / I2S_DOUT  |
> >>>> +--------------------+--------------------+
> >>>>
> >>>> The GPIOs in the pin header corresponds to the gpiochip I declare in
> >>>> this driver.
> >>>> So when I want to use a pin in GPIO mode, the upboard pinctrl driver
> >>>> requests the corresponding SoC GPIO to the Intel pinctrl driver.
> >>>> The SoC pins connected to the FPGA, are identified with "external" id.
> >>>>
> >>>> The hardware and the FPGA were designed in tandem, so you know for
> >>>> example that for the GPIOX you need to request the Nth "external" GPIO.
> >>>>
> >>>> When you drive your GPIO, the upboard gpiochip manages in the same time
> >>>> the direction of the "switch" and the value/direction of the
> >>>> corresponding SoC pin.
> >>>>
> >>>> +------------------+         +--------------+             +---+
> >>>>                    |---------|              |-------------| H |
> >>>>                    |---------|   GPIOCHIP   |-------------| E |
> >>>>    Intel gpiochip  |---------|              |-------------| A |
> >>>>  provided by Intel |---------|    FPGA      |-------------| D |
> >>>>   pinctrl driver   |---------|              |-------------| E |
> >>>>                    |---------|              |-------------| R |
> >>>>                    |---------|              |-------------|   |
> >>>> +------------------+         +--------------+             +---+
> >>>>
> >>>> About gpiochip_add_pinlist_range(), I added it because the FPGA pins
> >>>> used by the gpiochip are not consecutive.
> >>>>
> >>>> Please let me know if it is not clear.
> >>>> And sorry I'm not very good to make ascii art.
> >>>
> >>> I get it! We have a similar driver in the kernel already, look into:
> >>> drivers/gpio/gpio-aggregator.c
> >>>
> >>> The aggregator abstraction is however just software. What you
> >>> need here is a gpio-aggregator that adds some hardware
> >>> control on top. But it has a very nice design using a bitmap
> >>> to keep track of the GPIOs etc, and it supports operations
> >>> on multiple GPIOs (many man-hours of hard coding and
> >>> design went into that driver, ask Geert and Andy...)
> >>>
> >>> So I would proceed like this:
> >>>
> >>> - The pin control part of the driver looks sound, except
> >>>   for the way you add ranges.
> >>>
> >>> - The gpiochip part needs to be refactored using the
> >>>   ideas from gpio-aggregator.c.
> >>>
> >>> - Look closely at aggregator and see what you can do
> >>>   based on that code, if you can mimic how it picks up
> >>>   and forwards all GPIO functions. Maybe part of it
> >>>   needs to be made into a library?
> >>>  <linux/gpio/gpio-aggregator.h>?
> >>>   For example if you start to feel like "I would really like
> >>>   to just call gpio_fwd_get_multiple() then this is what
> >>>   you want to do. The library can probably still be
> >>>   inside gpio-aggregator.c the way we do it in
> >>>   e.g. gpio-mmio.c, just export and keep library functions
> >>>   separately.
> >>
> >> Ok I think I understand what you expect.
> >> I started to look at the gpio-aggregator code, play a bit with it, and
> >> refactor it to use it from my driver.
> >>
> >> My main issue is about the request of the SoC GPIOs done by the aggregator.
> >> If from my driver I call the aggregator library to create a gpiochip,
> >> the SoC pins will be requested. So the SoC pins will be set in GPIO
> >> mode, and the pins will never be in function mode.
> >> There is no way to set the pins back to function mode (even if the GPIO
> >> is free).
> >>
> >> I tried to add a feature in the aggregator to defer the request of the gpio.
> >> So at the beginning of each ops the gpio_desc is checked. If it is
> >> valid, the gpio can be used. Otherwise, the gpio is requested.
> >> For example:
> >>
> >> gpio_fwd_get() {
> >> 	if (!gpio_desc_is_valid(desc))
> >> 		desc = request_gpio()
> >>
> >> 	return gpiod_get_value(desc)
> >> }
> >>
> >> But when a gpiochip is registered, the core calls get_direction() or
> >> direction_input(), so all GPIOs are requested and it does not solve my
> >> problem.
> >>
> >> I expect to register a gpiochip without setting all pins in GPIO mode at
> >> probe time (like all pinctrl driver do).
> >> But I did not find a solution.
> > 
> > Basically what you need is a pinctrl-aggregattor (an analogue for the pin
> > muxing and configuration).
> 
> I found a trick to workaround the get_direction() issue in the
> gpio-aggregator.
> 
> I added a "request on first use" feature on the aggregator.
> The GPIO is requested during the request() operation of the fowarder.
> 
> static int gpio_fwd_request(struct gpio_chip *chip, unsigned int offset)
> {
> 	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
> 
> 	if (!IS_ERR_OR_NULL(fwd->descs[offset]))
> 		return 0;
> 
> 	fwd->descs[offset] = devm_gpiod_get_index(fwd->dev, NULL,
> 						offset, GPIOD_ASIS);
> 
> 	return PTR_ERR_OR_ZERO(fwd->descs[offset]);
> }
> 
> The remaining problem is that the get_direction() callback is called
> during gpiochip registration. For now if the gpio_desc is not valid (so
> the GPIO was not yet requested) I return GPIO_LINE_DIRECTION_OUT by
> default. But I'm not very convinced by this hack.
> Maybe I could retrieve the gpio_chip and call its get_direction()
> callback, but it seems not to be a better idea.
> 
> For the pinctrl-aggregator you mentioned, if I understand correctly the
> idea to aggregate the SoC pins in a pinctrl aggregator (with a
> gpio_chip) which just forwards gpio_request_enable(),
> gpio_disable_free(), gpio_set_direction() and also all gpio_chip operations.

No only these, all of the pin mux, pin configuration, and GPIO operations
should be proxied.

> But how to deal with the pinctrl of my FPGA ? For one of its fake pin
> the dummy pinctrl drives the corresponding SoC pin and FPGA pin ?

What's the "fake pin"? I can't find the one up in your schemas.

> So for each pin, the aggregator may have multiple pins to drive ?

Depending on the case, but yes. Currently we have implementations of
the discrete pin controls on Intel platforms based on ACPI (see Intel Minnow,
Intel Galileo, Intel Edison/Arduino boards in meta-acpi project [1]).
You probably want something similar to be written in C.

> Was it your idea Andy ?
> 
> Other challenge is to retrieve all the pins to add in the
> pinctrl-aggregator. As input I have only GPIO descriptors, but I guess
> it should be feasible.

In general your "proxy" (FPGA) pin control driver should be consumer of all SoC
pins (and their respective muxing and configurations) and be provider of the
pins that are available to the user.

[1]: https://github.com/westeri/meta-acpi/tree/master/recipes-bsp/acpi-tables/samples

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 4/5] pinctrl: Add pin controller driver for AAEON UP boards

[Thomas Richard]

On 1/16/25 10:12, Andy Shevchenko wrote:
> On Tue, Jan 14, 2025 at 11:28:26AM +0100, Thomas Richard wrote:
>> On 1/13/25 10:46, Andy Shevchenko wrote:
>>> On Fri, Jan 03, 2025 at 11:28:30AM +0100, Thomas Richard wrote:
>>>> On 12/22/24 00:43, Linus Walleij wrote:
>>>>> On Fri, Dec 20, 2024 at 2:50 PM Thomas Richard
>>>>> <thomas.richard@bootlin.com> wrote:
>>>>>
>>>>>> Yes my cover letter was a bit short, and maybe some context was missing.
>>>>>
>>>>> The text and graphics below explain it very well, so please include them
>>>>> into the commit message so we have it there!
>>>>>
>>>>>> This FPGA acts as a level shifter between the Intel SoC pins and the pin
>>>>>> header, and also makes a kind of switch/mux.
>>>>>
>>>>> Since it's Intel we need to notify Andy to help out with this so that
>>>>> it gets done in a way that works with how he think consumers
>>>>> should interact with Intel pin control and GPIO.
>>>>>
>>>>>> +---------+         +--------------+             +---+
>>>>>>           |         |              |             | H |
>>>>>>           |---------|              |-------------| E |
>>>>>>           |         |              |             | A |
>>>>>> Intel Soc |---------|    FPGA      |-------------| D |
>>>>>>           |         |              |             | E |
>>>>>>           |---------|              |-------------| R |
>>>>>>           |         |              |             |   |
>>>>>> ----------+         +--------------+             +---+
>>>>>>
>>>>>>
>>>>>> For most of the pins, the FPGA opens/closes a switch to enable/disable
>>>>>> the access to the SoC pin from a pin header.
>>>>>> Each "switch", has a direction flag that shall be set in tandem with the
>>>>>> status of the SoC pin.
>>>>>> For example, if the SoC pin is in PWM mode, the "switch" shall be
>>>>>> configured in output direction.
>>>>>> If the SoC pin is set in GPIO mode, the direction of the "switch" shall
>>>>>> corresponds to the GPIO direction.
>>>>>>
>>>>>> +---------+              +--------------+             +---+
>>>>>>           |              |              |             | H |
>>>>>>           |              |      \       |             | E |
>>>>>>           |   PWM1       |       \      |             | A |
>>>>>> Intel Soc |--------------|-----   \-----|-------------| D |
>>>>>>           |              |              |             | E |
>>>>>>           |              |              |             | R |
>>>>>>           |              |    FPGA      |             |   |
>>>>>> ----------+              +--------------+             +---+
>>>>>>
>>>>>> (PWM1 pin from Intel SoC can be used as PWM, and also in GPIO mode,
>>>>>> thanks to the Intel pinctrl driver).
>>>>>>
>>>>>> Few pins (PINMUX_* pins) work differently. The FPGA acts as a mux and
>>>>>> routes for example the I2C0_SDA pin or GPIOX (of the SoC) to the pin header.
> 
> Yep, that's clear.
> 
>>>>>> +---------+           +--------------+             +---+
>>>>>>           | I2C0_SDA  |              |             | H |
>>>>>>           |-----------|----- \       |             | E |
>>>>>>           |           |       \      |             | A |
>>>>>> Intel Soc |           |        \-----|-------------| D |
>>>>>>           | GPIOX     |              |             | E |
>>>>>>           |-----------|-----         |             | R |
>>>>>>           |           |    FPGA      |             |   |
>>>>>> ----------+           +--------------+             +---+
>>>>>>
>>>>>> The pin header looks like this:
>>>>>> +--------------------+--------------------+
>>>>>> |      3.3V          |       5V           |
>>>>>> | GPIO2 / I2C1_SDA   |       5V           |
>>>>>> | GPIO3 / I2C1_SCL   |       GND          |
>>>>>> | GPIO4 / ADC0       | GPIO14 / UART1_TX  |
>>>>>> |      GND           | GPIO15 / UART1_RX  |
>>>>>> | GPIO17 / UART1_RTS | GPIO18 / I2S_CLK   |
>>>>>> |     GPIO27         |       GND          |
>>>>>> |     GPIO22         |      GPIO23        |
>>>>>> |      3.3V          |      GPIO24        |
>>>>>> | GPIO10 / SPI_MOSI  |       GND          |
>>>>>> | GPIO9 / SPI_MISO   |      GPIO25        |
>>>>>> | GPIO11 / SPI_CLK   | GPIO8 / SPI_CS0    |
>>>>>> |      GND           | GPIO7 / SPI_CS1    |
>>>>>> | GPIO0 / I2C0_SDA   | GPIO1 / I2C0_SCL   |
>>>>>> |     GPIO5          |       GND          |
>>>>>> |     GPIO6          | GPIO12 / PWM0      |
>>>>>> | GPIO13 / PWM1      |       GND          |
>>>>>> | GPIO19 / I2S_FRM   | GPIO16 / UART1_CTS |
>>>>>> |     GPIO26         | GPIO20 / I2S_DIN   |
>>>>>> |      GND           | GPIO21 / I2S_DOUT  |
>>>>>> +--------------------+--------------------+
>>>>>>
>>>>>> The GPIOs in the pin header corresponds to the gpiochip I declare in
>>>>>> this driver.
>>>>>> So when I want to use a pin in GPIO mode, the upboard pinctrl driver
>>>>>> requests the corresponding SoC GPIO to the Intel pinctrl driver.
>>>>>> The SoC pins connected to the FPGA, are identified with "external" id.
>>>>>>
>>>>>> The hardware and the FPGA were designed in tandem, so you know for
>>>>>> example that for the GPIOX you need to request the Nth "external" GPIO.
>>>>>>
>>>>>> When you drive your GPIO, the upboard gpiochip manages in the same time
>>>>>> the direction of the "switch" and the value/direction of the
>>>>>> corresponding SoC pin.
>>>>>>
>>>>>> +------------------+         +--------------+             +---+
>>>>>>                    |---------|              |-------------| H |
>>>>>>                    |---------|   GPIOCHIP   |-------------| E |
>>>>>>    Intel gpiochip  |---------|              |-------------| A |
>>>>>>  provided by Intel |---------|    FPGA      |-------------| D |
>>>>>>   pinctrl driver   |---------|              |-------------| E |
>>>>>>                    |---------|              |-------------| R |
>>>>>>                    |---------|              |-------------|   |
>>>>>> +------------------+         +--------------+             +---+
>>>>>>
>>>>>> About gpiochip_add_pinlist_range(), I added it because the FPGA pins
>>>>>> used by the gpiochip are not consecutive.
>>>>>>
>>>>>> Please let me know if it is not clear.
>>>>>> And sorry I'm not very good to make ascii art.
>>>>>
>>>>> I get it! We have a similar driver in the kernel already, look into:
>>>>> drivers/gpio/gpio-aggregator.c
>>>>>
>>>>> The aggregator abstraction is however just software. What you
>>>>> need here is a gpio-aggregator that adds some hardware
>>>>> control on top. But it has a very nice design using a bitmap
>>>>> to keep track of the GPIOs etc, and it supports operations
>>>>> on multiple GPIOs (many man-hours of hard coding and
>>>>> design went into that driver, ask Geert and Andy...)
>>>>>
>>>>> So I would proceed like this:
>>>>>
>>>>> - The pin control part of the driver looks sound, except
>>>>>   for the way you add ranges.
>>>>>
>>>>> - The gpiochip part needs to be refactored using the
>>>>>   ideas from gpio-aggregator.c.
>>>>>
>>>>> - Look closely at aggregator and see what you can do
>>>>>   based on that code, if you can mimic how it picks up
>>>>>   and forwards all GPIO functions. Maybe part of it
>>>>>   needs to be made into a library?
>>>>>  <linux/gpio/gpio-aggregator.h>?
>>>>>   For example if you start to feel like "I would really like
>>>>>   to just call gpio_fwd_get_multiple() then this is what
>>>>>   you want to do. The library can probably still be
>>>>>   inside gpio-aggregator.c the way we do it in
>>>>>   e.g. gpio-mmio.c, just export and keep library functions
>>>>>   separately.
>>>>
>>>> Ok I think I understand what you expect.
>>>> I started to look at the gpio-aggregator code, play a bit with it, and
>>>> refactor it to use it from my driver.
>>>>
>>>> My main issue is about the request of the SoC GPIOs done by the aggregator.
>>>> If from my driver I call the aggregator library to create a gpiochip,
>>>> the SoC pins will be requested. So the SoC pins will be set in GPIO
>>>> mode, and the pins will never be in function mode.
>>>> There is no way to set the pins back to function mode (even if the GPIO
>>>> is free).
>>>>
>>>> I tried to add a feature in the aggregator to defer the request of the gpio.
>>>> So at the beginning of each ops the gpio_desc is checked. If it is
>>>> valid, the gpio can be used. Otherwise, the gpio is requested.
>>>> For example:
>>>>
>>>> gpio_fwd_get() {
>>>> 	if (!gpio_desc_is_valid(desc))
>>>> 		desc = request_gpio()
>>>>
>>>> 	return gpiod_get_value(desc)
>>>> }
>>>>
>>>> But when a gpiochip is registered, the core calls get_direction() or
>>>> direction_input(), so all GPIOs are requested and it does not solve my
>>>> problem.
>>>>
>>>> I expect to register a gpiochip without setting all pins in GPIO mode at
>>>> probe time (like all pinctrl driver do).
>>>> But I did not find a solution.
>>>
>>> Basically what you need is a pinctrl-aggregattor (an analogue for the pin
>>> muxing and configuration).
>>
>> I found a trick to workaround the get_direction() issue in the
>> gpio-aggregator.
>>
>> I added a "request on first use" feature on the aggregator.
>> The GPIO is requested during the request() operation of the fowarder.
>>
>> static int gpio_fwd_request(struct gpio_chip *chip, unsigned int offset)
>> {
>> 	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
>>
>> 	if (!IS_ERR_OR_NULL(fwd->descs[offset]))
>> 		return 0;
>>
>> 	fwd->descs[offset] = devm_gpiod_get_index(fwd->dev, NULL,
>> 						offset, GPIOD_ASIS);
>>
>> 	return PTR_ERR_OR_ZERO(fwd->descs[offset]);
>> }
>>
>> The remaining problem is that the get_direction() callback is called
>> during gpiochip registration. For now if the gpio_desc is not valid (so
>> the GPIO was not yet requested) I return GPIO_LINE_DIRECTION_OUT by
>> default. But I'm not very convinced by this hack.
>> Maybe I could retrieve the gpio_chip and call its get_direction()
>> callback, but it seems not to be a better idea.
>>
>> For the pinctrl-aggregator you mentioned, if I understand correctly the
>> idea to aggregate the SoC pins in a pinctrl aggregator (with a
>> gpio_chip) which just forwards gpio_request_enable(),
>> gpio_disable_free(), gpio_set_direction() and also all gpio_chip operations.
> 
> No only these, all of the pin mux, pin configuration, and GPIO operations
> should be proxied.

Why should I proxy operations that will never be used (pin mux, pin
conf) ? I mean there will never be a driver that will configure FPGA pins.

> 
>> But how to deal with the pinctrl of my FPGA ? For one of its fake pin
>> the dummy pinctrl drives the corresponding SoC pin and FPGA pin ?
> 
> What's the "fake pin"? I can't find the one up in your schemas.

I thought that the idea was a virtual pinctrl which drive the SoC
pinctrl and the FPGA pinctrl. But what you mean is the FPGA pinctrl acts
as a proxy.

> 
>> So for each pin, the aggregator may have multiple pins to drive ?
> 
> Depending on the case, but yes. Currently we have implementations of
> the discrete pin controls on Intel platforms based on ACPI (see Intel Minnow,
> Intel Galileo, Intel Edison/Arduino boards in meta-acpi project [1]).
> You probably want something similar to be written in C.
> 
>> Was it your idea Andy ?
>>
>> Other challenge is to retrieve all the pins to add in the
>> pinctrl-aggregator. As input I have only GPIO descriptors, but I guess
>> it should be feasible.
> 
> In general your "proxy" (FPGA) pin control driver should be consumer of all SoC
> pins (and their respective muxing and configurations) and be provider of the
> pins that are available to the user.

Isn't that a bit over-engineered for my use case ?

For the pinconf / pinmux, the FPGA is just a voltage translator. It is
transparent. The only relevant thing for the FPGA is the direction to
set for the switch of each pin. And the drivers knows which directions
to apply during the probe. This direction will only change in GPIO mode,
but in GPIO mode we know which direction to set.

For the GPIO part, the FPGA provides GPIOs. And in this case it is a
consumer of SoC GPIOs, it is already a kind of aggregator.

> 
> [1]: https://github.com/westeri/meta-acpi/tree/master/recipes-bsp/acpi-tables/samples
> 

Best Regards,

Thomas

Re: [PATCH 4/5] pinctrl: Add pin controller driver for AAEON UP boards

[Andy Shevchenko]

On Thu, Jan 16, 2025 at 01:21:16PM +0100, Thomas Richard wrote:
> On 1/16/25 10:12, Andy Shevchenko wrote:
> > On Tue, Jan 14, 2025 at 11:28:26AM +0100, Thomas Richard wrote:
> >> On 1/13/25 10:46, Andy Shevchenko wrote:
> >>> On Fri, Jan 03, 2025 at 11:28:30AM +0100, Thomas Richard wrote:
> >>>> On 12/22/24 00:43, Linus Walleij wrote:
> >>>>> On Fri, Dec 20, 2024 at 2:50 PM Thomas Richard
> >>>>> <thomas.richard@bootlin.com> wrote:

> >>>>>> Yes my cover letter was a bit short, and maybe some context was missing.
> >>>>>
> >>>>> The text and graphics below explain it very well, so please include them
> >>>>> into the commit message so we have it there!
> >>>>>
> >>>>>> This FPGA acts as a level shifter between the Intel SoC pins and the pin
> >>>>>> header, and also makes a kind of switch/mux.
> >>>>>
> >>>>> Since it's Intel we need to notify Andy to help out with this so that
> >>>>> it gets done in a way that works with how he think consumers
> >>>>> should interact with Intel pin control and GPIO.
> >>>>>
> >>>>>> +---------+         +--------------+             +---+
> >>>>>>           |         |              |             | H |
> >>>>>>           |---------|              |-------------| E |
> >>>>>>           |         |              |             | A |
> >>>>>> Intel Soc |---------|    FPGA      |-------------| D |
> >>>>>>           |         |              |             | E |
> >>>>>>           |---------|              |-------------| R |
> >>>>>>           |         |              |             |   |
> >>>>>> ----------+         +--------------+             +---+
> >>>>>>
> >>>>>>
> >>>>>> For most of the pins, the FPGA opens/closes a switch to enable/disable
> >>>>>> the access to the SoC pin from a pin header.
> >>>>>> Each "switch", has a direction flag that shall be set in tandem with the
> >>>>>> status of the SoC pin.
> >>>>>> For example, if the SoC pin is in PWM mode, the "switch" shall be
> >>>>>> configured in output direction.
> >>>>>> If the SoC pin is set in GPIO mode, the direction of the "switch" shall
> >>>>>> corresponds to the GPIO direction.
> >>>>>>
> >>>>>> +---------+              +--------------+             +---+
> >>>>>>           |              |              |             | H |
> >>>>>>           |              |      \       |             | E |
> >>>>>>           |   PWM1       |       \      |             | A |
> >>>>>> Intel Soc |--------------|-----   \-----|-------------| D |
> >>>>>>           |              |              |             | E |
> >>>>>>           |              |              |             | R |
> >>>>>>           |              |    FPGA      |             |   |
> >>>>>> ----------+              +--------------+             +---+
> >>>>>>
> >>>>>> (PWM1 pin from Intel SoC can be used as PWM, and also in GPIO mode,
> >>>>>> thanks to the Intel pinctrl driver).
> >>>>>>
> >>>>>> Few pins (PINMUX_* pins) work differently. The FPGA acts as a mux and
> >>>>>> routes for example the I2C0_SDA pin or GPIOX (of the SoC) to the pin header.
> > 
> > Yep, that's clear.
> > 
> >>>>>> +---------+           +--------------+             +---+
> >>>>>>           | I2C0_SDA  |              |             | H |
> >>>>>>           |-----------|----- \       |             | E |
> >>>>>>           |           |       \      |             | A |
> >>>>>> Intel Soc |           |        \-----|-------------| D |
> >>>>>>           | GPIOX     |              |             | E |
> >>>>>>           |-----------|-----         |             | R |
> >>>>>>           |           |    FPGA      |             |   |
> >>>>>> ----------+           +--------------+             +---+
> >>>>>>
> >>>>>> The pin header looks like this:
> >>>>>> +--------------------+--------------------+
> >>>>>> |      3.3V          |       5V           |
> >>>>>> | GPIO2 / I2C1_SDA   |       5V           |
> >>>>>> | GPIO3 / I2C1_SCL   |       GND          |
> >>>>>> | GPIO4 / ADC0       | GPIO14 / UART1_TX  |
> >>>>>> |      GND           | GPIO15 / UART1_RX  |
> >>>>>> | GPIO17 / UART1_RTS | GPIO18 / I2S_CLK   |
> >>>>>> |     GPIO27         |       GND          |
> >>>>>> |     GPIO22         |      GPIO23        |
> >>>>>> |      3.3V          |      GPIO24        |
> >>>>>> | GPIO10 / SPI_MOSI  |       GND          |
> >>>>>> | GPIO9 / SPI_MISO   |      GPIO25        |
> >>>>>> | GPIO11 / SPI_CLK   | GPIO8 / SPI_CS0    |
> >>>>>> |      GND           | GPIO7 / SPI_CS1    |
> >>>>>> | GPIO0 / I2C0_SDA   | GPIO1 / I2C0_SCL   |
> >>>>>> |     GPIO5          |       GND          |
> >>>>>> |     GPIO6          | GPIO12 / PWM0      |
> >>>>>> | GPIO13 / PWM1      |       GND          |
> >>>>>> | GPIO19 / I2S_FRM   | GPIO16 / UART1_CTS |
> >>>>>> |     GPIO26         | GPIO20 / I2S_DIN   |
> >>>>>> |      GND           | GPIO21 / I2S_DOUT  |
> >>>>>> +--------------------+--------------------+
> >>>>>>
> >>>>>> The GPIOs in the pin header corresponds to the gpiochip I declare in
> >>>>>> this driver.
> >>>>>> So when I want to use a pin in GPIO mode, the upboard pinctrl driver
> >>>>>> requests the corresponding SoC GPIO to the Intel pinctrl driver.
> >>>>>> The SoC pins connected to the FPGA, are identified with "external" id.
> >>>>>>
> >>>>>> The hardware and the FPGA were designed in tandem, so you know for
> >>>>>> example that for the GPIOX you need to request the Nth "external" GPIO.
> >>>>>>
> >>>>>> When you drive your GPIO, the upboard gpiochip manages in the same time
> >>>>>> the direction of the "switch" and the value/direction of the
> >>>>>> corresponding SoC pin.
> >>>>>>
> >>>>>> +------------------+         +--------------+             +---+
> >>>>>>                    |---------|              |-------------| H |
> >>>>>>                    |---------|   GPIOCHIP   |-------------| E |
> >>>>>>    Intel gpiochip  |---------|              |-------------| A |
> >>>>>>  provided by Intel |---------|    FPGA      |-------------| D |
> >>>>>>   pinctrl driver   |---------|              |-------------| E |
> >>>>>>                    |---------|              |-------------| R |
> >>>>>>                    |---------|              |-------------|   |
> >>>>>> +------------------+         +--------------+             +---+
> >>>>>>
> >>>>>> About gpiochip_add_pinlist_range(), I added it because the FPGA pins
> >>>>>> used by the gpiochip are not consecutive.
> >>>>>>
> >>>>>> Please let me know if it is not clear.
> >>>>>> And sorry I'm not very good to make ascii art.
> >>>>>
> >>>>> I get it! We have a similar driver in the kernel already, look into:
> >>>>> drivers/gpio/gpio-aggregator.c
> >>>>>
> >>>>> The aggregator abstraction is however just software. What you
> >>>>> need here is a gpio-aggregator that adds some hardware
> >>>>> control on top. But it has a very nice design using a bitmap
> >>>>> to keep track of the GPIOs etc, and it supports operations
> >>>>> on multiple GPIOs (many man-hours of hard coding and
> >>>>> design went into that driver, ask Geert and Andy...)
> >>>>>
> >>>>> So I would proceed like this:
> >>>>>
> >>>>> - The pin control part of the driver looks sound, except
> >>>>>   for the way you add ranges.
> >>>>>
> >>>>> - The gpiochip part needs to be refactored using the
> >>>>>   ideas from gpio-aggregator.c.
> >>>>>
> >>>>> - Look closely at aggregator and see what you can do
> >>>>>   based on that code, if you can mimic how it picks up
> >>>>>   and forwards all GPIO functions. Maybe part of it
> >>>>>   needs to be made into a library?
> >>>>>  <linux/gpio/gpio-aggregator.h>?
> >>>>>   For example if you start to feel like "I would really like
> >>>>>   to just call gpio_fwd_get_multiple() then this is what
> >>>>>   you want to do. The library can probably still be
> >>>>>   inside gpio-aggregator.c the way we do it in
> >>>>>   e.g. gpio-mmio.c, just export and keep library functions
> >>>>>   separately.
> >>>>
> >>>> Ok I think I understand what you expect.
> >>>> I started to look at the gpio-aggregator code, play a bit with it, and
> >>>> refactor it to use it from my driver.
> >>>>
> >>>> My main issue is about the request of the SoC GPIOs done by the aggregator.
> >>>> If from my driver I call the aggregator library to create a gpiochip,
> >>>> the SoC pins will be requested. So the SoC pins will be set in GPIO
> >>>> mode, and the pins will never be in function mode.
> >>>> There is no way to set the pins back to function mode (even if the GPIO
> >>>> is free).
> >>>>
> >>>> I tried to add a feature in the aggregator to defer the request of the gpio.
> >>>> So at the beginning of each ops the gpio_desc is checked. If it is
> >>>> valid, the gpio can be used. Otherwise, the gpio is requested.
> >>>> For example:
> >>>>
> >>>> gpio_fwd_get() {
> >>>> 	if (!gpio_desc_is_valid(desc))
> >>>> 		desc = request_gpio()
> >>>>
> >>>> 	return gpiod_get_value(desc)
> >>>> }
> >>>>
> >>>> But when a gpiochip is registered, the core calls get_direction() or
> >>>> direction_input(), so all GPIOs are requested and it does not solve my
> >>>> problem.
> >>>>
> >>>> I expect to register a gpiochip without setting all pins in GPIO mode at
> >>>> probe time (like all pinctrl driver do).
> >>>> But I did not find a solution.
> >>>
> >>> Basically what you need is a pinctrl-aggregattor (an analogue for the pin
> >>> muxing and configuration).
> >>
> >> I found a trick to workaround the get_direction() issue in the
> >> gpio-aggregator.
> >>
> >> I added a "request on first use" feature on the aggregator.
> >> The GPIO is requested during the request() operation of the fowarder.
> >>
> >> static int gpio_fwd_request(struct gpio_chip *chip, unsigned int offset)
> >> {
> >> 	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
> >>
> >> 	if (!IS_ERR_OR_NULL(fwd->descs[offset]))
> >> 		return 0;
> >>
> >> 	fwd->descs[offset] = devm_gpiod_get_index(fwd->dev, NULL,
> >> 						offset, GPIOD_ASIS);
> >>
> >> 	return PTR_ERR_OR_ZERO(fwd->descs[offset]);
> >> }
> >>
> >> The remaining problem is that the get_direction() callback is called
> >> during gpiochip registration. For now if the gpio_desc is not valid (so
> >> the GPIO was not yet requested) I return GPIO_LINE_DIRECTION_OUT by
> >> default. But I'm not very convinced by this hack.
> >> Maybe I could retrieve the gpio_chip and call its get_direction()
> >> callback, but it seems not to be a better idea.
> >>
> >> For the pinctrl-aggregator you mentioned, if I understand correctly the
> >> idea to aggregate the SoC pins in a pinctrl aggregator (with a
> >> gpio_chip) which just forwards gpio_request_enable(),
> >> gpio_disable_free(), gpio_set_direction() and also all gpio_chip operations.
> > 
> > No only these, all of the pin mux, pin configuration, and GPIO operations
> > should be proxied.
> 
> Why should I proxy operations that will never be used (pin mux, pin
> conf) ? I mean there will never be a driver that will configure FPGA pins.

According to your picture above the FPGA works as a pin function selector,
which also implies different pin configuration (e.g., slew rate for I²C pins).
Did I get it wrong?

> >> But how to deal with the pinctrl of my FPGA ? For one of its fake pin
> >> the dummy pinctrl drives the corresponding SoC pin and FPGA pin ?
> > 
> > What's the "fake pin"? I can't find the one up in your schemas.
> 
> I thought that the idea was a virtual pinctrl which drive the SoC
> pinctrl and the FPGA pinctrl. But what you mean is the FPGA pinctrl acts
> as a proxy.

When one wants to configure the pin parameters (let's say pin bias) the driver
delegates that to the SoC, in case the FPGA doesn't repeat this itself. Or both,
but this makes things too complicated already.

> >> So for each pin, the aggregator may have multiple pins to drive ?
> > 
> > Depending on the case, but yes. Currently we have implementations of
> > the discrete pin controls on Intel platforms based on ACPI (see Intel Minnow,
> > Intel Galileo, Intel Edison/Arduino boards in meta-acpi project [1]).
> > You probably want something similar to be written in C.
> > 
> >> Was it your idea Andy ?
> >>
> >> Other challenge is to retrieve all the pins to add in the
> >> pinctrl-aggregator. As input I have only GPIO descriptors, but I guess
> >> it should be feasible.
> > 
> > In general your "proxy" (FPGA) pin control driver should be consumer of all SoC
> > pins (and their respective muxing and configurations) and be provider of the
> > pins that are available to the user.
> 
> Isn't that a bit over-engineered for my use case ?

Yes, but it's already over-engineered in the HW, no?

> For the pinconf / pinmux, the FPGA is just a voltage translator.

It doesn't correspond to your picture where the pin function selector is depicted.

> It is transparent. The only relevant thing for the FPGA is the direction to
> set for the switch of each pin. And the drivers knows which directions
> to apply during the probe. This direction will only change in GPIO mode,
> but in GPIO mode we know which direction to set.
> 
> For the GPIO part, the FPGA provides GPIOs. And in this case it is a
> consumer of SoC GPIOs, it is already a kind of aggregator.
> 
> > [1]: https://github.com/westeri/meta-acpi/tree/master/recipes-bsp/acpi-tables/samples

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 4/5] pinctrl: Add pin controller driver for AAEON UP boards

[Thomas Richard]

On 1/16/25 13:31, Andy Shevchenko wrote:
> On Thu, Jan 16, 2025 at 01:21:16PM +0100, Thomas Richard wrote:
>> On 1/16/25 10:12, Andy Shevchenko wrote:
>>> On Tue, Jan 14, 2025 at 11:28:26AM +0100, Thomas Richard wrote:
>>>> On 1/13/25 10:46, Andy Shevchenko wrote:
>>>>> On Fri, Jan 03, 2025 at 11:28:30AM +0100, Thomas Richard wrote:
>>>>>> On 12/22/24 00:43, Linus Walleij wrote:
>>>>>>> On Fri, Dec 20, 2024 at 2:50 PM Thomas Richard
>>>>>>> <thomas.richard@bootlin.com> wrote:
> 
>>>>>>>> Yes my cover letter was a bit short, and maybe some context was missing.
>>>>>>>
>>>>>>> The text and graphics below explain it very well, so please include them
>>>>>>> into the commit message so we have it there!
>>>>>>>
>>>>>>>> This FPGA acts as a level shifter between the Intel SoC pins and the pin
>>>>>>>> header, and also makes a kind of switch/mux.
>>>>>>>
>>>>>>> Since it's Intel we need to notify Andy to help out with this so that
>>>>>>> it gets done in a way that works with how he think consumers
>>>>>>> should interact with Intel pin control and GPIO.
>>>>>>>
>>>>>>>> +---------+         +--------------+             +---+
>>>>>>>>           |         |              |             | H |
>>>>>>>>           |---------|              |-------------| E |
>>>>>>>>           |         |              |             | A |
>>>>>>>> Intel Soc |---------|    FPGA      |-------------| D |
>>>>>>>>           |         |              |             | E |
>>>>>>>>           |---------|              |-------------| R |
>>>>>>>>           |         |              |             |   |
>>>>>>>> ----------+         +--------------+             +---+
>>>>>>>>
>>>>>>>>
>>>>>>>> For most of the pins, the FPGA opens/closes a switch to enable/disable
>>>>>>>> the access to the SoC pin from a pin header.
>>>>>>>> Each "switch", has a direction flag that shall be set in tandem with the
>>>>>>>> status of the SoC pin.
>>>>>>>> For example, if the SoC pin is in PWM mode, the "switch" shall be
>>>>>>>> configured in output direction.
>>>>>>>> If the SoC pin is set in GPIO mode, the direction of the "switch" shall
>>>>>>>> corresponds to the GPIO direction.
>>>>>>>>
>>>>>>>> +---------+              +--------------+             +---+
>>>>>>>>           |              |              |             | H |
>>>>>>>>           |              |      \       |             | E |
>>>>>>>>           |   PWM1       |       \      |             | A |
>>>>>>>> Intel Soc |--------------|-----   \-----|-------------| D |
>>>>>>>>           |              |              |             | E |
>>>>>>>>           |              |              |             | R |
>>>>>>>>           |              |    FPGA      |             |   |
>>>>>>>> ----------+              +--------------+             +---+
>>>>>>>>
>>>>>>>> (PWM1 pin from Intel SoC can be used as PWM, and also in GPIO mode,
>>>>>>>> thanks to the Intel pinctrl driver).
>>>>>>>>
>>>>>>>> Few pins (PINMUX_* pins) work differently. The FPGA acts as a mux and
>>>>>>>> routes for example the I2C0_SDA pin or GPIOX (of the SoC) to the pin header.
>>>
>>> Yep, that's clear.
>>>
>>>>>>>> +---------+           +--------------+             +---+
>>>>>>>>           | I2C0_SDA  |              |             | H |
>>>>>>>>           |-----------|----- \       |             | E |
>>>>>>>>           |           |       \      |             | A |
>>>>>>>> Intel Soc |           |        \-----|-------------| D |
>>>>>>>>           | GPIOX     |              |             | E |
>>>>>>>>           |-----------|-----         |             | R |
>>>>>>>>           |           |    FPGA      |             |   |
>>>>>>>> ----------+           +--------------+             +---+
>>>>>>>>
>>>>>>>> The pin header looks like this:
>>>>>>>> +--------------------+--------------------+
>>>>>>>> |      3.3V          |       5V           |
>>>>>>>> | GPIO2 / I2C1_SDA   |       5V           |
>>>>>>>> | GPIO3 / I2C1_SCL   |       GND          |
>>>>>>>> | GPIO4 / ADC0       | GPIO14 / UART1_TX  |
>>>>>>>> |      GND           | GPIO15 / UART1_RX  |
>>>>>>>> | GPIO17 / UART1_RTS | GPIO18 / I2S_CLK   |
>>>>>>>> |     GPIO27         |       GND          |
>>>>>>>> |     GPIO22         |      GPIO23        |
>>>>>>>> |      3.3V          |      GPIO24        |
>>>>>>>> | GPIO10 / SPI_MOSI  |       GND          |
>>>>>>>> | GPIO9 / SPI_MISO   |      GPIO25        |
>>>>>>>> | GPIO11 / SPI_CLK   | GPIO8 / SPI_CS0    |
>>>>>>>> |      GND           | GPIO7 / SPI_CS1    |
>>>>>>>> | GPIO0 / I2C0_SDA   | GPIO1 / I2C0_SCL   |
>>>>>>>> |     GPIO5          |       GND          |
>>>>>>>> |     GPIO6          | GPIO12 / PWM0      |
>>>>>>>> | GPIO13 / PWM1      |       GND          |
>>>>>>>> | GPIO19 / I2S_FRM   | GPIO16 / UART1_CTS |
>>>>>>>> |     GPIO26         | GPIO20 / I2S_DIN   |
>>>>>>>> |      GND           | GPIO21 / I2S_DOUT  |
>>>>>>>> +--------------------+--------------------+
>>>>>>>>
>>>>>>>> The GPIOs in the pin header corresponds to the gpiochip I declare in
>>>>>>>> this driver.
>>>>>>>> So when I want to use a pin in GPIO mode, the upboard pinctrl driver
>>>>>>>> requests the corresponding SoC GPIO to the Intel pinctrl driver.
>>>>>>>> The SoC pins connected to the FPGA, are identified with "external" id.
>>>>>>>>
>>>>>>>> The hardware and the FPGA were designed in tandem, so you know for
>>>>>>>> example that for the GPIOX you need to request the Nth "external" GPIO.
>>>>>>>>
>>>>>>>> When you drive your GPIO, the upboard gpiochip manages in the same time
>>>>>>>> the direction of the "switch" and the value/direction of the
>>>>>>>> corresponding SoC pin.
>>>>>>>>
>>>>>>>> +------------------+         +--------------+             +---+
>>>>>>>>                    |---------|              |-------------| H |
>>>>>>>>                    |---------|   GPIOCHIP   |-------------| E |
>>>>>>>>    Intel gpiochip  |---------|              |-------------| A |
>>>>>>>>  provided by Intel |---------|    FPGA      |-------------| D |
>>>>>>>>   pinctrl driver   |---------|              |-------------| E |
>>>>>>>>                    |---------|              |-------------| R |
>>>>>>>>                    |---------|              |-------------|   |
>>>>>>>> +------------------+         +--------------+             +---+
>>>>>>>>
>>>>>>>> About gpiochip_add_pinlist_range(), I added it because the FPGA pins
>>>>>>>> used by the gpiochip are not consecutive.
>>>>>>>>
>>>>>>>> Please let me know if it is not clear.
>>>>>>>> And sorry I'm not very good to make ascii art.
>>>>>>>
>>>>>>> I get it! We have a similar driver in the kernel already, look into:
>>>>>>> drivers/gpio/gpio-aggregator.c
>>>>>>>
>>>>>>> The aggregator abstraction is however just software. What you
>>>>>>> need here is a gpio-aggregator that adds some hardware
>>>>>>> control on top. But it has a very nice design using a bitmap
>>>>>>> to keep track of the GPIOs etc, and it supports operations
>>>>>>> on multiple GPIOs (many man-hours of hard coding and
>>>>>>> design went into that driver, ask Geert and Andy...)
>>>>>>>
>>>>>>> So I would proceed like this:
>>>>>>>
>>>>>>> - The pin control part of the driver looks sound, except
>>>>>>>   for the way you add ranges.
>>>>>>>
>>>>>>> - The gpiochip part needs to be refactored using the
>>>>>>>   ideas from gpio-aggregator.c.
>>>>>>>
>>>>>>> - Look closely at aggregator and see what you can do
>>>>>>>   based on that code, if you can mimic how it picks up
>>>>>>>   and forwards all GPIO functions. Maybe part of it
>>>>>>>   needs to be made into a library?
>>>>>>>  <linux/gpio/gpio-aggregator.h>?
>>>>>>>   For example if you start to feel like "I would really like
>>>>>>>   to just call gpio_fwd_get_multiple() then this is what
>>>>>>>   you want to do. The library can probably still be
>>>>>>>   inside gpio-aggregator.c the way we do it in
>>>>>>>   e.g. gpio-mmio.c, just export and keep library functions
>>>>>>>   separately.
>>>>>>
>>>>>> Ok I think I understand what you expect.
>>>>>> I started to look at the gpio-aggregator code, play a bit with it, and
>>>>>> refactor it to use it from my driver.
>>>>>>
>>>>>> My main issue is about the request of the SoC GPIOs done by the aggregator.
>>>>>> If from my driver I call the aggregator library to create a gpiochip,
>>>>>> the SoC pins will be requested. So the SoC pins will be set in GPIO
>>>>>> mode, and the pins will never be in function mode.
>>>>>> There is no way to set the pins back to function mode (even if the GPIO
>>>>>> is free).
>>>>>>
>>>>>> I tried to add a feature in the aggregator to defer the request of the gpio.
>>>>>> So at the beginning of each ops the gpio_desc is checked. If it is
>>>>>> valid, the gpio can be used. Otherwise, the gpio is requested.
>>>>>> For example:
>>>>>>
>>>>>> gpio_fwd_get() {
>>>>>> 	if (!gpio_desc_is_valid(desc))
>>>>>> 		desc = request_gpio()
>>>>>>
>>>>>> 	return gpiod_get_value(desc)
>>>>>> }
>>>>>>
>>>>>> But when a gpiochip is registered, the core calls get_direction() or
>>>>>> direction_input(), so all GPIOs are requested and it does not solve my
>>>>>> problem.
>>>>>>
>>>>>> I expect to register a gpiochip without setting all pins in GPIO mode at
>>>>>> probe time (like all pinctrl driver do).
>>>>>> But I did not find a solution.
>>>>>
>>>>> Basically what you need is a pinctrl-aggregattor (an analogue for the pin
>>>>> muxing and configuration).
>>>>
>>>> I found a trick to workaround the get_direction() issue in the
>>>> gpio-aggregator.
>>>>
>>>> I added a "request on first use" feature on the aggregator.
>>>> The GPIO is requested during the request() operation of the fowarder.
>>>>
>>>> static int gpio_fwd_request(struct gpio_chip *chip, unsigned int offset)
>>>> {
>>>> 	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
>>>>
>>>> 	if (!IS_ERR_OR_NULL(fwd->descs[offset]))
>>>> 		return 0;
>>>>
>>>> 	fwd->descs[offset] = devm_gpiod_get_index(fwd->dev, NULL,
>>>> 						offset, GPIOD_ASIS);
>>>>
>>>> 	return PTR_ERR_OR_ZERO(fwd->descs[offset]);
>>>> }
>>>>
>>>> The remaining problem is that the get_direction() callback is called
>>>> during gpiochip registration. For now if the gpio_desc is not valid (so
>>>> the GPIO was not yet requested) I return GPIO_LINE_DIRECTION_OUT by
>>>> default. But I'm not very convinced by this hack.
>>>> Maybe I could retrieve the gpio_chip and call its get_direction()
>>>> callback, but it seems not to be a better idea.
>>>>
>>>> For the pinctrl-aggregator you mentioned, if I understand correctly the
>>>> idea to aggregate the SoC pins in a pinctrl aggregator (with a
>>>> gpio_chip) which just forwards gpio_request_enable(),
>>>> gpio_disable_free(), gpio_set_direction() and also all gpio_chip operations.
>>>
>>> No only these, all of the pin mux, pin configuration, and GPIO operations
>>> should be proxied.
>>
>> Why should I proxy operations that will never be used (pin mux, pin
>> conf) ? I mean there will never be a driver that will configure FPGA pins.
> 
> According to your picture above the FPGA works as a pin function selector,
> which also implies different pin configuration (e.g., slew rate for I²C pins).
> Did I get it wrong?

Yes you are right. In function mode the FPGA selects the I2C pin. When
the FPGA GPIO is requested, the FPGA changes the mux to select a SoC
GPIO pin (a pure GPIO pin).

> 
>>>> But how to deal with the pinctrl of my FPGA ? For one of its fake pin
>>>> the dummy pinctrl drives the corresponding SoC pin and FPGA pin ?
>>>
>>> What's the "fake pin"? I can't find the one up in your schemas.
>>
>> I thought that the idea was a virtual pinctrl which drive the SoC
>> pinctrl and the FPGA pinctrl. But what you mean is the FPGA pinctrl acts
>> as a proxy.
> 
> When one wants to configure the pin parameters (let's say pin bias) the driver
> delegates that to the SoC, in case the FPGA doesn't repeat this itself. Or both,
> but this makes things too complicated already.

The FPGA cannot configure pin parameter. It can only configures the
forward direction.
For example, for UART_TX pin the FPGA will set direction as "output" and
for UART_RX pin the FPGA will set direction as "input".
It can also enable/disable the forward of the SoC pin. If the forward of
a pin is disabled, the FPGA pin is in HIGH-Z.

> 
>>>> So for each pin, the aggregator may have multiple pins to drive ?
>>>
>>> Depending on the case, but yes. Currently we have implementations of
>>> the discrete pin controls on Intel platforms based on ACPI (see Intel Minnow,
>>> Intel Galileo, Intel Edison/Arduino boards in meta-acpi project [1]).
>>> You probably want something similar to be written in C.
>>>
>>>> Was it your idea Andy ?
>>>>
>>>> Other challenge is to retrieve all the pins to add in the
>>>> pinctrl-aggregator. As input I have only GPIO descriptors, but I guess
>>>> it should be feasible.
>>>
>>> In general your "proxy" (FPGA) pin control driver should be consumer of all SoC
>>> pins (and their respective muxing and configurations) and be provider of the
>>> pins that are available to the user.
>>
>> Isn't that a bit over-engineered for my use case ?
> 
> Yes, but it's already over-engineered in the HW, no?
> 
>> For the pinconf / pinmux, the FPGA is just a voltage translator.
> 
> It doesn't correspond to your picture where the pin function selector is depicted.

Yes you're right. I mean the mux can only select one function or GPIO mode.
I don't know why there is a mux to select a GPIO only pin, I'm pretty
sure the I2C pins can be set in GPIO mode.
It's probably for an hardware reason that a mux was added for only few pins.

> 
>> It is transparent. The only relevant thing for the FPGA is the direction to
>> set for the switch of each pin. And the drivers knows which directions
>> to apply during the probe. This direction will only change in GPIO mode,
>> but in GPIO mode we know which direction to set.
>>
>> For the GPIO part, the FPGA provides GPIOs. And in this case it is a
>> consumer of SoC GPIOs, it is already a kind of aggregator.
>>
>>> [1]: https://github.com/westeri/meta-acpi/tree/master/recipes-bsp/acpi-tables/samples
> 

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

Re: [PATCH 4/5] pinctrl: Add pin controller driver for AAEON UP boards

[Andy Shevchenko]

On Thu, Jan 16, 2025 at 02:23:57PM +0100, Thomas Richard wrote:
> On 1/16/25 13:31, Andy Shevchenko wrote:
> > On Thu, Jan 16, 2025 at 01:21:16PM +0100, Thomas Richard wrote:
> >> On 1/16/25 10:12, Andy Shevchenko wrote:
> >>> On Tue, Jan 14, 2025 at 11:28:26AM +0100, Thomas Richard wrote:
> >>>> On 1/13/25 10:46, Andy Shevchenko wrote:
> >>>>> On Fri, Jan 03, 2025 at 11:28:30AM +0100, Thomas Richard wrote:
> >>>>>> On 12/22/24 00:43, Linus Walleij wrote:
> >>>>>>> On Fri, Dec 20, 2024 at 2:50 PM Thomas Richard
> >>>>>>> <thomas.richard@bootlin.com> wrote:

...

> >>>>>>>> Yes my cover letter was a bit short, and maybe some context was missing.
> >>>>>>>
> >>>>>>> The text and graphics below explain it very well, so please include them
> >>>>>>> into the commit message so we have it there!
> >>>>>>>
> >>>>>>>> This FPGA acts as a level shifter between the Intel SoC pins and the pin
> >>>>>>>> header, and also makes a kind of switch/mux.
> >>>>>>>
> >>>>>>> Since it's Intel we need to notify Andy to help out with this so that
> >>>>>>> it gets done in a way that works with how he think consumers
> >>>>>>> should interact with Intel pin control and GPIO.
> >>>>>>>
> >>>>>>>> +---------+         +--------------+             +---+
> >>>>>>>>           |         |              |             | H |
> >>>>>>>>           |---------|              |-------------| E |
> >>>>>>>>           |         |              |             | A |
> >>>>>>>> Intel Soc |---------|    FPGA      |-------------| D |
> >>>>>>>>           |         |              |             | E |
> >>>>>>>>           |---------|              |-------------| R |
> >>>>>>>>           |         |              |             |   |
> >>>>>>>> ----------+         +--------------+             +---+
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> For most of the pins, the FPGA opens/closes a switch to enable/disable
> >>>>>>>> the access to the SoC pin from a pin header.
> >>>>>>>> Each "switch", has a direction flag that shall be set in tandem with the
> >>>>>>>> status of the SoC pin.
> >>>>>>>> For example, if the SoC pin is in PWM mode, the "switch" shall be
> >>>>>>>> configured in output direction.
> >>>>>>>> If the SoC pin is set in GPIO mode, the direction of the "switch" shall
> >>>>>>>> corresponds to the GPIO direction.
> >>>>>>>>
> >>>>>>>> +---------+              +--------------+             +---+
> >>>>>>>>           |              |              |             | H |
> >>>>>>>>           |              |      \       |             | E |
> >>>>>>>>           |   PWM1       |       \      |             | A |
> >>>>>>>> Intel Soc |--------------|-----   \-----|-------------| D |
> >>>>>>>>           |              |              |             | E |
> >>>>>>>>           |              |              |             | R |
> >>>>>>>>           |              |    FPGA      |             |   |
> >>>>>>>> ----------+              +--------------+             +---+
> >>>>>>>>
> >>>>>>>> (PWM1 pin from Intel SoC can be used as PWM, and also in GPIO mode,
> >>>>>>>> thanks to the Intel pinctrl driver).
> >>>>>>>>
> >>>>>>>> Few pins (PINMUX_* pins) work differently. The FPGA acts as a mux and
> >>>>>>>> routes for example the I2C0_SDA pin or GPIOX (of the SoC) to the pin header.
> >>>
> >>> Yep, that's clear.
> >>>
> >>>>>>>> +---------+           +--------------+             +---+
> >>>>>>>>           | I2C0_SDA  |              |             | H |
> >>>>>>>>           |-----------|----- \       |             | E |
> >>>>>>>>           |           |       \      |             | A |
> >>>>>>>> Intel Soc |           |        \-----|-------------| D |
> >>>>>>>>           | GPIOX     |              |             | E |
> >>>>>>>>           |-----------|-----         |             | R |
> >>>>>>>>           |           |    FPGA      |             |   |
> >>>>>>>> ----------+           +--------------+             +---+
> >>>>>>>>
> >>>>>>>> The pin header looks like this:
> >>>>>>>> +--------------------+--------------------+
> >>>>>>>> |      3.3V          |       5V           |
> >>>>>>>> | GPIO2 / I2C1_SDA   |       5V           |
> >>>>>>>> | GPIO3 / I2C1_SCL   |       GND          |
> >>>>>>>> | GPIO4 / ADC0       | GPIO14 / UART1_TX  |
> >>>>>>>> |      GND           | GPIO15 / UART1_RX  |
> >>>>>>>> | GPIO17 / UART1_RTS | GPIO18 / I2S_CLK   |
> >>>>>>>> |     GPIO27         |       GND          |
> >>>>>>>> |     GPIO22         |      GPIO23        |
> >>>>>>>> |      3.3V          |      GPIO24        |
> >>>>>>>> | GPIO10 / SPI_MOSI  |       GND          |
> >>>>>>>> | GPIO9 / SPI_MISO   |      GPIO25        |
> >>>>>>>> | GPIO11 / SPI_CLK   | GPIO8 / SPI_CS0    |
> >>>>>>>> |      GND           | GPIO7 / SPI_CS1    |
> >>>>>>>> | GPIO0 / I2C0_SDA   | GPIO1 / I2C0_SCL   |
> >>>>>>>> |     GPIO5          |       GND          |
> >>>>>>>> |     GPIO6          | GPIO12 / PWM0      |
> >>>>>>>> | GPIO13 / PWM1      |       GND          |
> >>>>>>>> | GPIO19 / I2S_FRM   | GPIO16 / UART1_CTS |
> >>>>>>>> |     GPIO26         | GPIO20 / I2S_DIN   |
> >>>>>>>> |      GND           | GPIO21 / I2S_DOUT  |
> >>>>>>>> +--------------------+--------------------+
> >>>>>>>>
> >>>>>>>> The GPIOs in the pin header corresponds to the gpiochip I declare in
> >>>>>>>> this driver.
> >>>>>>>> So when I want to use a pin in GPIO mode, the upboard pinctrl driver
> >>>>>>>> requests the corresponding SoC GPIO to the Intel pinctrl driver.
> >>>>>>>> The SoC pins connected to the FPGA, are identified with "external" id.
> >>>>>>>>
> >>>>>>>> The hardware and the FPGA were designed in tandem, so you know for
> >>>>>>>> example that for the GPIOX you need to request the Nth "external" GPIO.
> >>>>>>>>
> >>>>>>>> When you drive your GPIO, the upboard gpiochip manages in the same time
> >>>>>>>> the direction of the "switch" and the value/direction of the
> >>>>>>>> corresponding SoC pin.
> >>>>>>>>
> >>>>>>>> +------------------+         +--------------+             +---+
> >>>>>>>>                    |---------|              |-------------| H |
> >>>>>>>>                    |---------|   GPIOCHIP   |-------------| E |
> >>>>>>>>    Intel gpiochip  |---------|              |-------------| A |
> >>>>>>>>  provided by Intel |---------|    FPGA      |-------------| D |
> >>>>>>>>   pinctrl driver   |---------|              |-------------| E |
> >>>>>>>>                    |---------|              |-------------| R |
> >>>>>>>>                    |---------|              |-------------|   |
> >>>>>>>> +------------------+         +--------------+             +---+
> >>>>>>>>
> >>>>>>>> About gpiochip_add_pinlist_range(), I added it because the FPGA pins
> >>>>>>>> used by the gpiochip are not consecutive.
> >>>>>>>>
> >>>>>>>> Please let me know if it is not clear.
> >>>>>>>> And sorry I'm not very good to make ascii art.
> >>>>>>>
> >>>>>>> I get it! We have a similar driver in the kernel already, look into:
> >>>>>>> drivers/gpio/gpio-aggregator.c
> >>>>>>>
> >>>>>>> The aggregator abstraction is however just software. What you
> >>>>>>> need here is a gpio-aggregator that adds some hardware
> >>>>>>> control on top. But it has a very nice design using a bitmap
> >>>>>>> to keep track of the GPIOs etc, and it supports operations
> >>>>>>> on multiple GPIOs (many man-hours of hard coding and
> >>>>>>> design went into that driver, ask Geert and Andy...)
> >>>>>>>
> >>>>>>> So I would proceed like this:
> >>>>>>>
> >>>>>>> - The pin control part of the driver looks sound, except
> >>>>>>>   for the way you add ranges.
> >>>>>>>
> >>>>>>> - The gpiochip part needs to be refactored using the
> >>>>>>>   ideas from gpio-aggregator.c.
> >>>>>>>
> >>>>>>> - Look closely at aggregator and see what you can do
> >>>>>>>   based on that code, if you can mimic how it picks up
> >>>>>>>   and forwards all GPIO functions. Maybe part of it
> >>>>>>>   needs to be made into a library?
> >>>>>>>  <linux/gpio/gpio-aggregator.h>?
> >>>>>>>   For example if you start to feel like "I would really like
> >>>>>>>   to just call gpio_fwd_get_multiple() then this is what
> >>>>>>>   you want to do. The library can probably still be
> >>>>>>>   inside gpio-aggregator.c the way we do it in
> >>>>>>>   e.g. gpio-mmio.c, just export and keep library functions
> >>>>>>>   separately.
> >>>>>>
> >>>>>> Ok I think I understand what you expect.
> >>>>>> I started to look at the gpio-aggregator code, play a bit with it, and
> >>>>>> refactor it to use it from my driver.
> >>>>>>
> >>>>>> My main issue is about the request of the SoC GPIOs done by the aggregator.
> >>>>>> If from my driver I call the aggregator library to create a gpiochip,
> >>>>>> the SoC pins will be requested. So the SoC pins will be set in GPIO
> >>>>>> mode, and the pins will never be in function mode.
> >>>>>> There is no way to set the pins back to function mode (even if the GPIO
> >>>>>> is free).
> >>>>>>
> >>>>>> I tried to add a feature in the aggregator to defer the request of the gpio.
> >>>>>> So at the beginning of each ops the gpio_desc is checked. If it is
> >>>>>> valid, the gpio can be used. Otherwise, the gpio is requested.
> >>>>>> For example:
> >>>>>>
> >>>>>> gpio_fwd_get() {
> >>>>>> 	if (!gpio_desc_is_valid(desc))
> >>>>>> 		desc = request_gpio()
> >>>>>>
> >>>>>> 	return gpiod_get_value(desc)
> >>>>>> }
> >>>>>>
> >>>>>> But when a gpiochip is registered, the core calls get_direction() or
> >>>>>> direction_input(), so all GPIOs are requested and it does not solve my
> >>>>>> problem.
> >>>>>>
> >>>>>> I expect to register a gpiochip without setting all pins in GPIO mode at
> >>>>>> probe time (like all pinctrl driver do).
> >>>>>> But I did not find a solution.
> >>>>>
> >>>>> Basically what you need is a pinctrl-aggregattor (an analogue for the pin
> >>>>> muxing and configuration).
> >>>>
> >>>> I found a trick to workaround the get_direction() issue in the
> >>>> gpio-aggregator.
> >>>>
> >>>> I added a "request on first use" feature on the aggregator.
> >>>> The GPIO is requested during the request() operation of the fowarder.
> >>>>
> >>>> static int gpio_fwd_request(struct gpio_chip *chip, unsigned int offset)
> >>>> {
> >>>> 	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
> >>>>
> >>>> 	if (!IS_ERR_OR_NULL(fwd->descs[offset]))
> >>>> 		return 0;
> >>>>
> >>>> 	fwd->descs[offset] = devm_gpiod_get_index(fwd->dev, NULL,
> >>>> 						offset, GPIOD_ASIS);
> >>>>
> >>>> 	return PTR_ERR_OR_ZERO(fwd->descs[offset]);
> >>>> }
> >>>>
> >>>> The remaining problem is that the get_direction() callback is called
> >>>> during gpiochip registration. For now if the gpio_desc is not valid (so
> >>>> the GPIO was not yet requested) I return GPIO_LINE_DIRECTION_OUT by
> >>>> default. But I'm not very convinced by this hack.
> >>>> Maybe I could retrieve the gpio_chip and call its get_direction()
> >>>> callback, but it seems not to be a better idea.
> >>>>
> >>>> For the pinctrl-aggregator you mentioned, if I understand correctly the
> >>>> idea to aggregate the SoC pins in a pinctrl aggregator (with a
> >>>> gpio_chip) which just forwards gpio_request_enable(),
> >>>> gpio_disable_free(), gpio_set_direction() and also all gpio_chip operations.
> >>>
> >>> No only these, all of the pin mux, pin configuration, and GPIO operations
> >>> should be proxied.
> >>
> >> Why should I proxy operations that will never be used (pin mux, pin
> >> conf) ? I mean there will never be a driver that will configure FPGA pins.
> > 
> > According to your picture above the FPGA works as a pin function selector,
> > which also implies different pin configuration (e.g., slew rate for I²C pins).
> > Did I get it wrong?
> 
> Yes you are right. In function mode the FPGA selects the I2C pin. When
> the FPGA GPIO is requested, the FPGA changes the mux to select a SoC
> GPIO pin (a pure GPIO pin).
> 
> >>>> But how to deal with the pinctrl of my FPGA ? For one of its fake pin
> >>>> the dummy pinctrl drives the corresponding SoC pin and FPGA pin ?
> >>>
> >>> What's the "fake pin"? I can't find the one up in your schemas.
> >>
> >> I thought that the idea was a virtual pinctrl which drive the SoC
> >> pinctrl and the FPGA pinctrl. But what you mean is the FPGA pinctrl acts
> >> as a proxy.
> > 
> > When one wants to configure the pin parameters (let's say pin bias) the driver
> > delegates that to the SoC, in case the FPGA doesn't repeat this itself. Or both,
> > but this makes things too complicated already.
> 
> The FPGA cannot configure pin parameter. It can only configures the
> forward direction.
> For example, for UART_TX pin the FPGA will set direction as "output" and
> for UART_RX pin the FPGA will set direction as "input".
> It can also enable/disable the forward of the SoC pin. If the forward of
> a pin is disabled, the FPGA pin is in HIGH-Z.

So, which is exactly why proxying is needed:
1) it delegates pin configuration when user asks for a change;
2) it properly configures SoC pin directions, etc in accordance to the signal
on the header and the user ask.

> >>>> So for each pin, the aggregator may have multiple pins to drive ?
> >>>
> >>> Depending on the case, but yes. Currently we have implementations of
> >>> the discrete pin controls on Intel platforms based on ACPI (see Intel Minnow,
> >>> Intel Galileo, Intel Edison/Arduino boards in meta-acpi project [1]).
> >>> You probably want something similar to be written in C.
> >>>
> >>>> Was it your idea Andy ?
> >>>>
> >>>> Other challenge is to retrieve all the pins to add in the
> >>>> pinctrl-aggregator. As input I have only GPIO descriptors, but I guess
> >>>> it should be feasible.
> >>>
> >>> In general your "proxy" (FPGA) pin control driver should be consumer of all SoC
> >>> pins (and their respective muxing and configurations) and be provider of the
> >>> pins that are available to the user.
> >>
> >> Isn't that a bit over-engineered for my use case ?
> > 
> > Yes, but it's already over-engineered in the HW, no?
> > 
> >> For the pinconf / pinmux, the FPGA is just a voltage translator.
> > 
> > It doesn't correspond to your picture where the pin function selector is depicted.
> 
> Yes you're right. I mean the mux can only select one function or GPIO mode.

Which is fine, but it is still a pin muxing.

> I don't know why there is a mux to select a GPIO only pin, I'm pretty
> sure the I2C pins can be set in GPIO mode.
> It's probably for an hardware reason that a mux was added for only few pins.
> 
> >> It is transparent. The only relevant thing for the FPGA is the direction to
> >> set for the switch of each pin. And the drivers knows which directions
> >> to apply during the probe. This direction will only change in GPIO mode,
> >> but in GPIO mode we know which direction to set.
> >>
> >> For the GPIO part, the FPGA provides GPIOs. And in this case it is a
> >> consumer of SoC GPIOs, it is already a kind of aggregator.
> >>
> >>> [1]: https://github.com/westeri/meta-acpi/tree/master/recipes-bsp/acpi-tables/samples

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 4/5] pinctrl: Add pin controller driver for AAEON UP boards

[Thomas Richard]

On 1/16/25 15:14, Andy Shevchenko wrote:
> On Thu, Jan 16, 2025 at 02:23:57PM +0100, Thomas Richard wrote:
>> On 1/16/25 13:31, Andy Shevchenko wrote:
>>> On Thu, Jan 16, 2025 at 01:21:16PM +0100, Thomas Richard wrote:
>>>> On 1/16/25 10:12, Andy Shevchenko wrote:
>>>>> On Tue, Jan 14, 2025 at 11:28:26AM +0100, Thomas Richard wrote:
>>>>>> On 1/13/25 10:46, Andy Shevchenko wrote:
>>>>>>> On Fri, Jan 03, 2025 at 11:28:30AM +0100, Thomas Richard wrote:
>>>>>>>> On 12/22/24 00:43, Linus Walleij wrote:
>>>>>>>>> On Fri, Dec 20, 2024 at 2:50 PM Thomas Richard
>>>>>>>>> <thomas.richard@bootlin.com> wrote:
> 
> ...
> 
>>>>>>>>>> Yes my cover letter was a bit short, and maybe some context was missing.
>>>>>>>>>
>>>>>>>>> The text and graphics below explain it very well, so please include them
>>>>>>>>> into the commit message so we have it there!
>>>>>>>>>
>>>>>>>>>> This FPGA acts as a level shifter between the Intel SoC pins and the pin
>>>>>>>>>> header, and also makes a kind of switch/mux.
>>>>>>>>>
>>>>>>>>> Since it's Intel we need to notify Andy to help out with this so that
>>>>>>>>> it gets done in a way that works with how he think consumers
>>>>>>>>> should interact with Intel pin control and GPIO.
>>>>>>>>>
>>>>>>>>>> +---------+         +--------------+             +---+
>>>>>>>>>>           |         |              |             | H |
>>>>>>>>>>           |---------|              |-------------| E |
>>>>>>>>>>           |         |              |             | A |
>>>>>>>>>> Intel Soc |---------|    FPGA      |-------------| D |
>>>>>>>>>>           |         |              |             | E |
>>>>>>>>>>           |---------|              |-------------| R |
>>>>>>>>>>           |         |              |             |   |
>>>>>>>>>> ----------+         +--------------+             +---+
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> For most of the pins, the FPGA opens/closes a switch to enable/disable
>>>>>>>>>> the access to the SoC pin from a pin header.
>>>>>>>>>> Each "switch", has a direction flag that shall be set in tandem with the
>>>>>>>>>> status of the SoC pin.
>>>>>>>>>> For example, if the SoC pin is in PWM mode, the "switch" shall be
>>>>>>>>>> configured in output direction.
>>>>>>>>>> If the SoC pin is set in GPIO mode, the direction of the "switch" shall
>>>>>>>>>> corresponds to the GPIO direction.
>>>>>>>>>>
>>>>>>>>>> +---------+              +--------------+             +---+
>>>>>>>>>>           |              |              |             | H |
>>>>>>>>>>           |              |      \       |             | E |
>>>>>>>>>>           |   PWM1       |       \      |             | A |
>>>>>>>>>> Intel Soc |--------------|-----   \-----|-------------| D |
>>>>>>>>>>           |              |              |             | E |
>>>>>>>>>>           |              |              |             | R |
>>>>>>>>>>           |              |    FPGA      |             |   |
>>>>>>>>>> ----------+              +--------------+             +---+
>>>>>>>>>>
>>>>>>>>>> (PWM1 pin from Intel SoC can be used as PWM, and also in GPIO mode,
>>>>>>>>>> thanks to the Intel pinctrl driver).
>>>>>>>>>>
>>>>>>>>>> Few pins (PINMUX_* pins) work differently. The FPGA acts as a mux and
>>>>>>>>>> routes for example the I2C0_SDA pin or GPIOX (of the SoC) to the pin header.
>>>>>
>>>>> Yep, that's clear.
>>>>>
>>>>>>>>>> +---------+           +--------------+             +---+
>>>>>>>>>>           | I2C0_SDA  |              |             | H |
>>>>>>>>>>           |-----------|----- \       |             | E |
>>>>>>>>>>           |           |       \      |             | A |
>>>>>>>>>> Intel Soc |           |        \-----|-------------| D |
>>>>>>>>>>           | GPIOX     |              |             | E |
>>>>>>>>>>           |-----------|-----         |             | R |
>>>>>>>>>>           |           |    FPGA      |             |   |
>>>>>>>>>> ----------+           +--------------+             +---+
>>>>>>>>>>
>>>>>>>>>> The pin header looks like this:
>>>>>>>>>> +--------------------+--------------------+
>>>>>>>>>> |      3.3V          |       5V           |
>>>>>>>>>> | GPIO2 / I2C1_SDA   |       5V           |
>>>>>>>>>> | GPIO3 / I2C1_SCL   |       GND          |
>>>>>>>>>> | GPIO4 / ADC0       | GPIO14 / UART1_TX  |
>>>>>>>>>> |      GND           | GPIO15 / UART1_RX  |
>>>>>>>>>> | GPIO17 / UART1_RTS | GPIO18 / I2S_CLK   |
>>>>>>>>>> |     GPIO27         |       GND          |
>>>>>>>>>> |     GPIO22         |      GPIO23        |
>>>>>>>>>> |      3.3V          |      GPIO24        |
>>>>>>>>>> | GPIO10 / SPI_MOSI  |       GND          |
>>>>>>>>>> | GPIO9 / SPI_MISO   |      GPIO25        |
>>>>>>>>>> | GPIO11 / SPI_CLK   | GPIO8 / SPI_CS0    |
>>>>>>>>>> |      GND           | GPIO7 / SPI_CS1    |
>>>>>>>>>> | GPIO0 / I2C0_SDA   | GPIO1 / I2C0_SCL   |
>>>>>>>>>> |     GPIO5          |       GND          |
>>>>>>>>>> |     GPIO6          | GPIO12 / PWM0      |
>>>>>>>>>> | GPIO13 / PWM1      |       GND          |
>>>>>>>>>> | GPIO19 / I2S_FRM   | GPIO16 / UART1_CTS |
>>>>>>>>>> |     GPIO26         | GPIO20 / I2S_DIN   |
>>>>>>>>>> |      GND           | GPIO21 / I2S_DOUT  |
>>>>>>>>>> +--------------------+--------------------+
>>>>>>>>>>
>>>>>>>>>> The GPIOs in the pin header corresponds to the gpiochip I declare in
>>>>>>>>>> this driver.
>>>>>>>>>> So when I want to use a pin in GPIO mode, the upboard pinctrl driver
>>>>>>>>>> requests the corresponding SoC GPIO to the Intel pinctrl driver.
>>>>>>>>>> The SoC pins connected to the FPGA, are identified with "external" id.
>>>>>>>>>>
>>>>>>>>>> The hardware and the FPGA were designed in tandem, so you know for
>>>>>>>>>> example that for the GPIOX you need to request the Nth "external" GPIO.
>>>>>>>>>>
>>>>>>>>>> When you drive your GPIO, the upboard gpiochip manages in the same time
>>>>>>>>>> the direction of the "switch" and the value/direction of the
>>>>>>>>>> corresponding SoC pin.
>>>>>>>>>>
>>>>>>>>>> +------------------+         +--------------+             +---+
>>>>>>>>>>                    |---------|              |-------------| H |
>>>>>>>>>>                    |---------|   GPIOCHIP   |-------------| E |
>>>>>>>>>>    Intel gpiochip  |---------|              |-------------| A |
>>>>>>>>>>  provided by Intel |---------|    FPGA      |-------------| D |
>>>>>>>>>>   pinctrl driver   |---------|              |-------------| E |
>>>>>>>>>>                    |---------|              |-------------| R |
>>>>>>>>>>                    |---------|              |-------------|   |
>>>>>>>>>> +------------------+         +--------------+             +---+
>>>>>>>>>>
>>>>>>>>>> About gpiochip_add_pinlist_range(), I added it because the FPGA pins
>>>>>>>>>> used by the gpiochip are not consecutive.
>>>>>>>>>>
>>>>>>>>>> Please let me know if it is not clear.
>>>>>>>>>> And sorry I'm not very good to make ascii art.
>>>>>>>>>
>>>>>>>>> I get it! We have a similar driver in the kernel already, look into:
>>>>>>>>> drivers/gpio/gpio-aggregator.c
>>>>>>>>>
>>>>>>>>> The aggregator abstraction is however just software. What you
>>>>>>>>> need here is a gpio-aggregator that adds some hardware
>>>>>>>>> control on top. But it has a very nice design using a bitmap
>>>>>>>>> to keep track of the GPIOs etc, and it supports operations
>>>>>>>>> on multiple GPIOs (many man-hours of hard coding and
>>>>>>>>> design went into that driver, ask Geert and Andy...)
>>>>>>>>>
>>>>>>>>> So I would proceed like this:
>>>>>>>>>
>>>>>>>>> - The pin control part of the driver looks sound, except
>>>>>>>>>   for the way you add ranges.
>>>>>>>>>
>>>>>>>>> - The gpiochip part needs to be refactored using the
>>>>>>>>>   ideas from gpio-aggregator.c.
>>>>>>>>>
>>>>>>>>> - Look closely at aggregator and see what you can do
>>>>>>>>>   based on that code, if you can mimic how it picks up
>>>>>>>>>   and forwards all GPIO functions. Maybe part of it
>>>>>>>>>   needs to be made into a library?
>>>>>>>>>  <linux/gpio/gpio-aggregator.h>?
>>>>>>>>>   For example if you start to feel like "I would really like
>>>>>>>>>   to just call gpio_fwd_get_multiple() then this is what
>>>>>>>>>   you want to do. The library can probably still be
>>>>>>>>>   inside gpio-aggregator.c the way we do it in
>>>>>>>>>   e.g. gpio-mmio.c, just export and keep library functions
>>>>>>>>>   separately.
>>>>>>>>
>>>>>>>> Ok I think I understand what you expect.
>>>>>>>> I started to look at the gpio-aggregator code, play a bit with it, and
>>>>>>>> refactor it to use it from my driver.
>>>>>>>>
>>>>>>>> My main issue is about the request of the SoC GPIOs done by the aggregator.
>>>>>>>> If from my driver I call the aggregator library to create a gpiochip,
>>>>>>>> the SoC pins will be requested. So the SoC pins will be set in GPIO
>>>>>>>> mode, and the pins will never be in function mode.
>>>>>>>> There is no way to set the pins back to function mode (even if the GPIO
>>>>>>>> is free).
>>>>>>>>
>>>>>>>> I tried to add a feature in the aggregator to defer the request of the gpio.
>>>>>>>> So at the beginning of each ops the gpio_desc is checked. If it is
>>>>>>>> valid, the gpio can be used. Otherwise, the gpio is requested.
>>>>>>>> For example:
>>>>>>>>
>>>>>>>> gpio_fwd_get() {
>>>>>>>> 	if (!gpio_desc_is_valid(desc))
>>>>>>>> 		desc = request_gpio()
>>>>>>>>
>>>>>>>> 	return gpiod_get_value(desc)
>>>>>>>> }
>>>>>>>>
>>>>>>>> But when a gpiochip is registered, the core calls get_direction() or
>>>>>>>> direction_input(), so all GPIOs are requested and it does not solve my
>>>>>>>> problem.
>>>>>>>>
>>>>>>>> I expect to register a gpiochip without setting all pins in GPIO mode at
>>>>>>>> probe time (like all pinctrl driver do).
>>>>>>>> But I did not find a solution.
>>>>>>>
>>>>>>> Basically what you need is a pinctrl-aggregattor (an analogue for the pin
>>>>>>> muxing and configuration).
>>>>>>
>>>>>> I found a trick to workaround the get_direction() issue in the
>>>>>> gpio-aggregator.
>>>>>>
>>>>>> I added a "request on first use" feature on the aggregator.
>>>>>> The GPIO is requested during the request() operation of the fowarder.
>>>>>>
>>>>>> static int gpio_fwd_request(struct gpio_chip *chip, unsigned int offset)
>>>>>> {
>>>>>> 	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
>>>>>>
>>>>>> 	if (!IS_ERR_OR_NULL(fwd->descs[offset]))
>>>>>> 		return 0;
>>>>>>
>>>>>> 	fwd->descs[offset] = devm_gpiod_get_index(fwd->dev, NULL,
>>>>>> 						offset, GPIOD_ASIS);
>>>>>>
>>>>>> 	return PTR_ERR_OR_ZERO(fwd->descs[offset]);
>>>>>> }
>>>>>>
>>>>>> The remaining problem is that the get_direction() callback is called
>>>>>> during gpiochip registration. For now if the gpio_desc is not valid (so
>>>>>> the GPIO was not yet requested) I return GPIO_LINE_DIRECTION_OUT by
>>>>>> default. But I'm not very convinced by this hack.
>>>>>> Maybe I could retrieve the gpio_chip and call its get_direction()
>>>>>> callback, but it seems not to be a better idea.
>>>>>>
>>>>>> For the pinctrl-aggregator you mentioned, if I understand correctly the
>>>>>> idea to aggregate the SoC pins in a pinctrl aggregator (with a
>>>>>> gpio_chip) which just forwards gpio_request_enable(),
>>>>>> gpio_disable_free(), gpio_set_direction() and also all gpio_chip operations.
>>>>>
>>>>> No only these, all of the pin mux, pin configuration, and GPIO operations
>>>>> should be proxied.
>>>>
>>>> Why should I proxy operations that will never be used (pin mux, pin
>>>> conf) ? I mean there will never be a driver that will configure FPGA pins.
>>>
>>> According to your picture above the FPGA works as a pin function selector,
>>> which also implies different pin configuration (e.g., slew rate for I²C pins).
>>> Did I get it wrong?
>>
>> Yes you are right. In function mode the FPGA selects the I2C pin. When
>> the FPGA GPIO is requested, the FPGA changes the mux to select a SoC
>> GPIO pin (a pure GPIO pin).
>>
>>>>>> But how to deal with the pinctrl of my FPGA ? For one of its fake pin
>>>>>> the dummy pinctrl drives the corresponding SoC pin and FPGA pin ?
>>>>>
>>>>> What's the "fake pin"? I can't find the one up in your schemas.
>>>>
>>>> I thought that the idea was a virtual pinctrl which drive the SoC
>>>> pinctrl and the FPGA pinctrl. But what you mean is the FPGA pinctrl acts
>>>> as a proxy.
>>>
>>> When one wants to configure the pin parameters (let's say pin bias) the driver
>>> delegates that to the SoC, in case the FPGA doesn't repeat this itself. Or both,
>>> but this makes things too complicated already.
>>
>> The FPGA cannot configure pin parameter. It can only configures the
>> forward direction.
>> For example, for UART_TX pin the FPGA will set direction as "output" and
>> for UART_RX pin the FPGA will set direction as "input".
>> It can also enable/disable the forward of the SoC pin. If the forward of
>> a pin is disabled, the FPGA pin is in HIGH-Z.
> 
> So, which is exactly why proxying is needed:
> 1) it delegates pin configuration when user asks for a change;
> 2) it properly configures SoC pin directions, etc in accordance to the signal
> on the header and the user ask.

Hi Andy, Linus,

I did some tests/experiments to implement a proxy pinctrl as requested
by Andy.
I have made progress, but now I have questions and blocking points.

First thing, to identify the SoC pins used by the FPGA, I have to
request the corresponding GPIOs (defined in the ACPI tables). Using the
GPIO descriptor I'm able to find the corresponding pinctrl device and
the pin number (thanks to pinctrl_get_device_gpio_range()).

I store all the pinctrl device an pin number couples in a radix tree.

struct pin_fwd {
	struct pinctrl_dev *pctldev;
	int pin;
};

struct pinctrl_fwd {
	struct device *dev;
	struct radix_tree_root pin_fwd_tree;
	unsigned int num_pins;
};

So for a FPGA pin, I can easily get find the corresponding SoC pin.
I request the SoC pins (thanks to pin_request()) during the probe to
lock access.
I'm able to forward some pinctrl or pinconf ops like dbg_show(),
config_get() ...

But I don't really know how to deal with pinmux ops, groups and functions.
I cannot just forward a set_mux() call.
Should I have a mapping between SoC pinctrl and FPGA pinctrl for all
groups and functions ?
Other issue with GPIO mode, in gpio_request_enable() I'm able to request
the SoC pin using pin_request() and a gpio_range. But in this case
gpiolib does not know the GPIO is requested.
And how to deal with set() and get() ?

Other problem I noticed, but maybe only due to the fact I implemented
everything in my driver, I had to export symbols of many static
functions like pin_request(), pinctrl_get_device_gpio_range(),
pinconf_dump_pin() ...

An other problem is that as I requested GPIO to identify all SoC pins
connected to the FPGA, I have to set it back to function mode, and I'm
not sure it is possible to do it for all pins (some pins are not
registered in a group, maybe they are not writable ?).

So I'm not really convinced by all this complexity for only one driver.

In the same time I had an other idea and I would like your advises.

The FPGA pinctrl is a consumer of SoC pins, so I can add some
pinctrl_map to request the SoC pins and mark them as used by the FPGA:

PIN_MAP_MUX_GROUP("upboard-pinctrl", "soc_pins", "INT3452:00",
		"pwm0_grp", "pwm0"),
PIN_MAP_MUX_GROUP("upboard-pinctrl", "soc_pins", "INT3452:00",
		"uart1_grp", "uart1"),

And in the probe() I call devm_pinctrl_get_select() to request and
configure SoC pins.

Pros:
- the SoC pins are marked as used by the FPGA.
- less complex solution, no change in the core.

Cons:
- probably one mapping for each board as Intel pinctrl device, groups
and functions may change depending the board. So the right mapping
should be selected based on DMI table.

This solution also implies to make some changes in Intel pinctrl driver
to create missing groups. Or maybe the pins which are not in a group are
not writeable (so no need to request them) ?

For the GPIO part, no changes, the SoC GPIO are requested at runtime
using gpiod (Linus I did not forget you mentioned to use/rework
gpio-aggregator).

Best Regards,

Thomas

Re: [PATCH 4/5] pinctrl: Add pin controller driver for AAEON UP boards

[Andy Shevchenko]

On Wed, Feb 05, 2025 at 12:17:29PM +0100, Thomas Richard wrote:
> On 1/16/25 15:14, Andy Shevchenko wrote:

...

> So I'm not really convinced by all this complexity for only one driver.

I am not sure if I asked you to show the excerpt from DSDT for this device.
Is there any link I can browse the ASL code (for that particular device,
most likely I wouldn't need the full DSDT)?

...

> In the same time I had an other idea and I would like your advises.
> 
> The FPGA pinctrl is a consumer of SoC pins, so I can add some
> pinctrl_map to request the SoC pins and mark them as used by the FPGA:
> 
> PIN_MAP_MUX_GROUP("upboard-pinctrl", "soc_pins", "INT3452:00",
> 		"pwm0_grp", "pwm0"),
> PIN_MAP_MUX_GROUP("upboard-pinctrl", "soc_pins", "INT3452:00",
> 		"uart1_grp", "uart1"),
> 
> And in the probe() I call devm_pinctrl_get_select() to request and
> configure SoC pins.
> 
> Pros:
> - the SoC pins are marked as used by the FPGA.
> - less complex solution, no change in the core.
> 
> Cons:
> - probably one mapping for each board as Intel pinctrl device, groups
> and functions may change depending the board. So the right mapping
> should be selected based on DMI table.

Yes, this solution is what we can do for now, I don't see the Cons part is
really cons as we do that in many drivers, but ideally, of course, this
information should come from DSDT. Saying this reminds me that we still have
a gap that should link the ACPICA resources for pin control and muxing to
the pin control layer in the Linux kernel. That's what we probably should
focus on instead of creating a pin control proxy driver which indeed sounds
like a complex and over engineered solution for a niche.

> This solution also implies to make some changes in Intel pinctrl driver
> to create missing groups. Or maybe the pins which are not in a group are
> not writeable (so no need to request them) ?

This is not a problem, it's an improvement in my opinion. Again, this,
of course, should come from DSDT (see the respective Pin*() resource
descriptions in the ACPI specification).

> For the GPIO part, no changes, the SoC GPIO are requested at runtime
> using gpiod (Linus I did not forget you mentioned to use/rework
> gpio-aggregator).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 4/5] pinctrl: Add pin controller driver for AAEON UP boards

[Thomas Richard]

Hi Andy,

On 2/5/25 12:41, Andy Shevchenko wrote:
> On Wed, Feb 05, 2025 at 12:17:29PM +0100, Thomas Richard wrote:
>> On 1/16/25 15:14, Andy Shevchenko wrote:
> 
> ...
> 
>> So I'm not really convinced by all this complexity for only one driver.
> 
> I am not sure if I asked you to show the excerpt from DSDT for this device.
> Is there any link I can browse the ASL code (for that particular device,
> most likely I wouldn't need the full DSDT)?
> 

I'm currently working on the V3, and I just remembered that you asked me
DSDT file. So for the UP Squared board, please find the full DSDT, and
also SSDT1 (which contains the FPGA declaration).

DSDT: https://gist.github.com/thom24/4d24c2a2f58d93f799e512c2485efd45
SSDT1: https://gist.github.com/thom24/2da44ef86eacfa5d2d492ce43fa41aa4

Best Regards,

Thomas

Re: [PATCH 4/5] pinctrl: Add pin controller driver for AAEON UP boards

[Andy Shevchenko]

On Fri, Apr 04, 2025 at 03:11:00PM +0200, Thomas Richard wrote:
> On 2/5/25 12:41, Andy Shevchenko wrote:
> > On Wed, Feb 05, 2025 at 12:17:29PM +0100, Thomas Richard wrote:
> >> On 1/16/25 15:14, Andy Shevchenko wrote:

...

> >> So I'm not really convinced by all this complexity for only one driver.
> > 
> > I am not sure if I asked you to show the excerpt from DSDT for this device.
> > Is there any link I can browse the ASL code (for that particular device,
> > most likely I wouldn't need the full DSDT)?
> > 
> 
> I'm currently working on the V3, and I just remembered that you asked me
> DSDT file. So for the UP Squared board, please find the full DSDT, and
> also SSDT1 (which contains the FPGA declaration).
> 
> DSDT: https://gist.github.com/thom24/4d24c2a2f58d93f799e512c2485efd45
> SSDT1: https://gist.github.com/thom24/2da44ef86eacfa5d2d492ce43fa41aa4

Thank you!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 4/5] pinctrl: Add pin controller driver for AAEON UP boards

[Linus Walleij]

On Thu, Jan 16, 2025 at 1:21 PM Thomas Richard
<thomas.richard@bootlin.com> wrote:

> For the pinconf / pinmux, the FPGA is just a voltage translator. It is
> transparent. The only relevant thing for the FPGA is the direction to
> set for the switch of each pin. And the drivers knows which directions
> to apply during the probe. This direction will only change in GPIO mode,
> but in GPIO mode we know which direction to set.

Just a thought:

Maybe the lowest impact is to just patch in the extra operations
directly in the existing Intel pin control/GPIO driver in
drivers/pinctrl/intel?

I don't know how this is detected by the system (I guess some
ACPI magic since it's Intel, in DT we can detect the top-level
board) but it can certainly be done and
probably replaced with compiled-out stubs if not used.

This might not be what Andy desires though, I think he has
the final word on how this should be engineered.

Yours,
Linus Walleij

[PATCH 5/5] MAINTAINERS: Add entry for AAEON UP board FPGA drivers

[Thomas Richard]

Add the AAEON UP board FPGA drivers and header as Maintained by myself.

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

diff --git a/MAINTAINERS b/MAINTAINERS
index 1e930c7a58b1..cbe4cfbe110d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -185,6 +185,14 @@ W:	http://www.adaptec.com/
 F:	Documentation/scsi/aacraid.rst
 F:	drivers/scsi/aacraid/
 
+AAEON UPBOARD FPGA MFD DRIVER
+M:	Thomas Richard <thomas.richard@bootlin.com>
+S:	Maintained
+F:	drivers/leds/leds-upboard.c
+F:	drivers/mfd/upboard-fpga.c
+F:	drivers/pinctrl/pinctrl-upboard.c
+F:	include/linux/mfd/upboard-fpga.h
+
 AB8500 BATTERY AND CHARGER DRIVERS
 M:	Linus Walleij <linus.walleij@linaro.org>
 F:	Documentation/devicetree/bindings/power/supply/*ab8500*

-- 
2.39.5

Re: (subset) [PATCH 0/5] Add support for the AAEON UP board FPGA

[Lee Jones]

On Wed, 11 Dec 2024 17:27:15 +0100, Thomas Richard wrote:
> This is the revival of the AAEON UP boards FPGA.
> The latest version was sent one year ago [1].
> 
> I started a new series, as I re-wrote all the drivers using the AAEON
> DKMS driver [2] as reference. I also read all series sent to the ML to
> collect the comments sent by the maintainers and reviewers (to not
> reproduce the same errors).
> 
> [...]

Applied, thanks!

[1/5] mfd: Add support for AAEON UP board FPGA
      commit: 577ced9b8ac3791edfddc7c371a7f948cdd0e35c
[2/5] leds: Add AAEON UP board LED driver
      commit: 5885a1731d0052d5d8a8cbc9871f94fae1f3baa3
[5/5] MAINTAINERS: Add entry for AAEON UP board FPGA drivers
      commit: f35d82b6f63994e7d7944daa99651ab88300ce22

--
Lee Jones [李琼斯]