[PATCH 0/5] Add support control UP board CPLD/FPGA pin control

[PATCH 0/5] Add support control UP board CPLD/FPGA pin control

[chengwei]

The UP board <http://www.upboard.com> is the computer board for 
Professional Makers and Industrial Applications. We want to upstream 
the UP board 40-pin GP-bus Kernel driver for giving the users better 
experience on the software release. (not just download from UP board 
github)

These patches are generated from the Linux kernel mainline tag v6.0.

(1) PATCH 1 (mfd: Add support for UP board CPLD/FPGA)
We did git send-email this patch to maintainer on 2022/10/11 for reviewing.

(2) PATCH 2 (regmap: Expose regmap_writable function to check if a register is
    writable)
The regmap patch expose the regmap_writeable function for pinctrl-upboard 
reference.

(3) PATCH 3 (ACPI: acpi_node_add_pin_mapping added to header file)
Declare acpi_node_add_pin_mapping added in header file.

(4) PATCH 4 (GPIO ACPI: Add support to map GPIO resources to ranges)
Add a pin mapping for named GPIO resources for pinctrl-upboard 
reference.

(3) PATCH 5 (pinctrl: Add support pin control for UP board CPLD/FPGA)
The UP board implements certain features (pin control) through an on-board CPLD.
** This patch depends on PATCH 1 (mfd: Add support for UP board CPLD/FPGA).
** This patch depends on PATCH 2 to refer to regmap_writeable function.
** This patch depends on PATCH 3 and PATCH 4 to refer to acpi_node_add_pin_mapping 
function.

chengwei (5):
  mfd: Add support for UP board CPLD/FPGA
  regmap: Expose regmap_writeable function to check if a register is
    writable
  ACPI: acpi_node_add_pin_mapping added to header file
  GPIO ACPI: Add support to map GPIO resources to ranges
  pinctrl: Add support pin control for UP board CPLD/FPGA

 drivers/base/regmap/internal.h    |    5 -
 drivers/base/regmap/regmap.c      |    5 +
 drivers/gpio/gpiolib-acpi.c       |   88 ++-
 drivers/mfd/Kconfig               |   12 +
 drivers/mfd/Makefile              |    1 +
 drivers/mfd/upboard-fpga.c        |  482 ++++++++++++++
 drivers/pinctrl/Kconfig           |   15 +
 drivers/pinctrl/Makefile          |    1 +
 drivers/pinctrl/pinctrl-upboard.c | 1003 +++++++++++++++++++++++++++++
 include/linux/acpi.h              |   14 +
 include/linux/mfd/upboard-fpga.h  |   49 ++
 include/linux/regmap.h            |    6 +
 12 files changed, 1659 insertions(+), 22 deletions(-)
 create mode 100644 drivers/mfd/upboard-fpga.c
 create mode 100644 drivers/pinctrl/pinctrl-upboard.c
 create mode 100644 include/linux/mfd/upboard-fpga.h


base-commit: 4fe89d07dcc2804c8b562f6c7896a45643d34b2f
-- 
2.17.1

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

[chengwei]

The UP Squared board <http://www.upboard.com> implements certain
features (pin control, onboard LEDs or CEC) through an on-board CPLD/FPGA.

This mfd driver implements the line protocol to read and write registers
from the FPGA through regmap. The register address map is also included.

The UP Boards provide a few I/O pin headers (for both GPIO and
functions), including a 40-pin Raspberry Pi compatible header.

This patch implements support for the FPGA-based pin controller that
manages direction and enable state for those header pins.

Partial support UP boards:
* UP core + CREX
* UP core + CRST02

Signed-off-by: Javier Arteaga <javier@emutex.com>
[merge various fixes]
Signed-off-by: Nicola Lunghi <nicola.lunghi@emutex.com>
Signed-off-by: chengwei <larry.lai@yunjingtech.com>
---
 drivers/mfd/Kconfig              |  12 +
 drivers/mfd/Makefile             |   1 +
 drivers/mfd/upboard-fpga.c       | 482 +++++++++++++++++++++++++++++++
 include/linux/mfd/upboard-fpga.h |  49 ++++
 4 files changed, 544 insertions(+)
 create mode 100644 drivers/mfd/upboard-fpga.c
 create mode 100644 include/linux/mfd/upboard-fpga.h

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index abb58ab1a1a4..c1d72a70e5f2 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -2104,6 +2104,18 @@ config MFD_QCOM_PM8008
 	  under it in the device tree. Additional drivers must be enabled in
 	  order to use the functionality of the device.
 
+config MFD_UPBOARD_FPGA
+	tristate "Support for the UP board FPGA"
+	select MFD_CORE
+	depends on X86 && ACPI
+	help
+	  Select this option to enable the Intel AAEON UP and UP^2 on-board FPGA.
+	  The UP board implements certain features (pin control, onboard LEDs or
+	  CEC) through an on-board FPGA.
+
+	  To compile this driver as a module, choose M here: the module will be
+	  called upboard-fpga.
+
 menu "Multimedia Capabilities Port drivers"
 	depends on ARCH_SA1100
 
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 858cacf659d6..d9d10e3664f7 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -250,6 +250,7 @@ obj-$(CONFIG_MFD_ALTERA_A10SR)	+= altera-a10sr.o
 obj-$(CONFIG_MFD_ALTERA_SYSMGR) += altera-sysmgr.o
 obj-$(CONFIG_MFD_STPMIC1)	+= stpmic1.o
 obj-$(CONFIG_MFD_SUN4I_GPADC)	+= sun4i-gpadc.o
+obj-$(CONFIG_MFD_UPBOARD_FPGA)	+= upboard-fpga.o
 
 obj-$(CONFIG_MFD_STM32_LPTIMER)	+= stm32-lptimer.o
 obj-$(CONFIG_MFD_STM32_TIMERS) 	+= stm32-timers.o
diff --git a/drivers/mfd/upboard-fpga.c b/drivers/mfd/upboard-fpga.c
new file mode 100644
index 000000000000..6f73c2fa6f4a
--- /dev/null
+++ b/drivers/mfd/upboard-fpga.c
@@ -0,0 +1,482 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * UP Board main platform driver and FPGA configuration support
+ *
+ * Copyright (c) 2017, Emutex Ltd. All rights reserved.
+ *
+ * Author: Javier Arteaga <javier@emutex.com>
+ */
+
+#include <linux/acpi.h>
+#include <linux/dmi.h>
+#include <linux/gpio.h>
+#include <linux/kernel.h>
+#include <linux/leds.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/upboard-fpga.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+static int upboard_fpga_read(void *, unsigned int, unsigned int *);
+static int upboard_fpga_write(void *, unsigned int, unsigned int);
+
+struct upboard_fpga_data {
+	const struct regmap_config *regmapconf;
+	const struct mfd_cell *cells;
+	size_t ncells;
+};
+
+#define UPBOARD_LED_CELL(led_data, n)                   \
+	{                                               \
+		.name = "upboard-led",                  \
+		.id = (n),                              \
+		.platform_data = &led_data[(n)],        \
+		.pdata_size = sizeof(*(led_data)),      \
+	}
+
+/* UP board */
+
+static const struct regmap_range upboard_up_readable_ranges[] = {
+	regmap_reg_range(UPFPGA_REG_PLATFORM_ID, UPFPGA_REG_FIRMWARE_ID),
+	regmap_reg_range(UPFPGA_REG_FUNC_EN0, UPFPGA_REG_FUNC_EN0),
+	regmap_reg_range(UPFPGA_REG_GPIO_DIR0, UPFPGA_REG_GPIO_DIR1),
+};
+
+static const struct regmap_range upboard_up_writable_ranges[] = {
+	regmap_reg_range(UPFPGA_REG_FUNC_EN0, UPFPGA_REG_FUNC_EN0),
+	regmap_reg_range(UPFPGA_REG_GPIO_DIR0, UPFPGA_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 = UPFPGA_ADDRESS_SIZE,
+	.val_bits = UPFPGA_REGISTER_SIZE,
+	.max_register = UPFPGA_REG_MAX,
+	.reg_read = upboard_fpga_read,
+	.reg_write = upboard_fpga_write,
+	.fast_io = false,
+	.cache_type = REGCACHE_RBTREE,
+	.rd_table = &upboard_up_readable_table,
+	.wr_table = &upboard_up_writable_table,
+};
+
+static struct upboard_led_data upboard_up_led_data[] = {
+	{ .bit = 0, .colour = "yellow" },
+	{ .bit = 1, .colour = "green" },
+	{ .bit = 2, .colour = "red" },
+};
+
+static const struct mfd_cell upboard_up_mfd_cells[] = {
+	{ .name = "upboard-pinctrl" },
+	UPBOARD_LED_CELL(upboard_up_led_data, 0),
+	UPBOARD_LED_CELL(upboard_up_led_data, 1),
+	UPBOARD_LED_CELL(upboard_up_led_data, 2),
+};
+
+static const struct upboard_fpga_data upboard_up_fpga_data = {
+	.regmapconf = &upboard_up_regmap_config,
+	.cells = upboard_up_mfd_cells,
+	.ncells = ARRAY_SIZE(upboard_up_mfd_cells),
+};
+
+/* UP^2 board */
+
+static const struct regmap_range upboard_up2_readable_ranges[] = {
+	regmap_reg_range(UPFPGA_REG_PLATFORM_ID, UPFPGA_REG_FIRMWARE_ID),
+	regmap_reg_range(UPFPGA_REG_FUNC_EN0, UPFPGA_REG_FUNC_EN1),
+	regmap_reg_range(UPFPGA_REG_GPIO_EN0, UPFPGA_REG_GPIO_EN2),
+	regmap_reg_range(UPFPGA_REG_GPIO_DIR0, UPFPGA_REG_GPIO_DIR2),
+};
+
+static const struct regmap_range upboard_up2_writable_ranges[] = {
+	regmap_reg_range(UPFPGA_REG_FUNC_EN0, UPFPGA_REG_FUNC_EN1),
+	regmap_reg_range(UPFPGA_REG_GPIO_EN0, UPFPGA_REG_GPIO_EN2),
+	regmap_reg_range(UPFPGA_REG_GPIO_DIR0, UPFPGA_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 = UPFPGA_ADDRESS_SIZE,
+	.val_bits = UPFPGA_REGISTER_SIZE,
+	.max_register = UPFPGA_REG_MAX,
+	.reg_read = upboard_fpga_read,
+	.reg_write = upboard_fpga_write,
+	.fast_io = false,
+	.cache_type = REGCACHE_RBTREE,
+	.rd_table = &upboard_up2_readable_table,
+	.wr_table = &upboard_up2_writable_table,
+};
+
+static struct upboard_led_data upboard_up2_led_data[] = {
+	{ .bit = 0, .colour = "blue" },
+	{ .bit = 1, .colour = "yellow" },
+	{ .bit = 2, .colour = "green" },
+	{ .bit = 3, .colour = "red" },
+};
+
+static const struct mfd_cell upboard_up2_mfd_cells[] = {
+	{ .name = "upboard-pinctrl" },
+	UPBOARD_LED_CELL(upboard_up2_led_data, 0),
+	UPBOARD_LED_CELL(upboard_up2_led_data, 1),
+	UPBOARD_LED_CELL(upboard_up2_led_data, 2),
+	UPBOARD_LED_CELL(upboard_up2_led_data, 3),
+};
+
+static const struct upboard_fpga_data upboard_up2_fpga_data = {
+	.regmapconf = &upboard_up2_regmap_config,
+	.cells = upboard_up2_mfd_cells,
+	.ncells = ARRAY_SIZE(upboard_up2_mfd_cells),
+};
+
+/* UP-CREX carrier board for UP Core */
+
+/* same MAXV config as UP1 (proto2 release) */
+#define upboard_upcore_crex_fpga_data upboard_up_fpga_data
+
+/* UP-CRST02 carrier board for UP Core */
+
+/* same MAX10 config as UP2, but same LED cells as UP1 */
+static const struct upboard_fpga_data upboard_upcore_crst02_fpga_data = {
+	.regmapconf = &upboard_up2_regmap_config,
+	.cells = upboard_up_mfd_cells,
+	.ncells = ARRAY_SIZE(upboard_up_mfd_cells),
+};
+
+static int upboard_fpga_read(void *context, unsigned int reg, unsigned int *val)
+{
+	struct upboard_fpga * const fpga = context;
+	int i;
+
+	gpiod_set_value(fpga->clear_gpio, 0);
+	gpiod_set_value(fpga->clear_gpio, 1);
+
+	reg |= UPFPGA_READ_FLAG;
+
+	for (i = UPFPGA_ADDRESS_SIZE; i >= 0; i--) {
+		gpiod_set_value(fpga->strobe_gpio, 0);
+		gpiod_set_value(fpga->datain_gpio, (reg >> i) & 0x1);
+		gpiod_set_value(fpga->strobe_gpio, 1);
+	}
+
+	gpiod_set_value(fpga->strobe_gpio, 0);
+	*val = 0;
+
+	for (i = UPFPGA_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 * const fpga = context;
+	int i;
+
+	gpiod_set_value(fpga->clear_gpio, 0);
+	gpiod_set_value(fpga->clear_gpio, 1);
+
+	for (i = UPFPGA_ADDRESS_SIZE; i >= 0; i--) {
+		gpiod_set_value(fpga->strobe_gpio, 0);
+		gpiod_set_value(fpga->datain_gpio, (reg >> i) & 0x1);
+		gpiod_set_value(fpga->strobe_gpio, 1);
+	}
+
+	gpiod_set_value(fpga->strobe_gpio, 0);
+
+	for (i = UPFPGA_REGISTER_SIZE - 1; i >= 0; i--) {
+		gpiod_set_value(fpga->datain_gpio, (val >> i) & 0x1);
+		gpiod_set_value(fpga->strobe_gpio, 1);
+		gpiod_set_value(fpga->strobe_gpio, 0);
+	}
+
+	gpiod_set_value(fpga->strobe_gpio, 1);
+
+	return 0;
+};
+
+static int __init upboard_fpga_gpio_init(struct upboard_fpga *fpga)
+{
+	enum gpiod_flags flags;
+
+	flags = fpga->uninitialised ? GPIOD_OUT_LOW : GPIOD_ASIS;
+	fpga->enable_gpio = devm_gpiod_get(fpga->dev, "enable", flags);
+	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);
+
+	/* The SoC pinctrl driver may not support reserving the GPIO line for
+	 * FPGA reset without causing an undesired reset pulse. This will clear
+	 * any settings on the FPGA, so only do it if we must.
+	 */
+	if (fpga->uninitialised) {
+		fpga->reset_gpio = devm_gpiod_get(fpga->dev, "reset",
+						  GPIOD_OUT_LOW);
+		if (IS_ERR(fpga->reset_gpio))
+			return PTR_ERR(fpga->reset_gpio);
+
+		gpiod_set_value(fpga->reset_gpio, 1);
+	}
+
+	gpiod_set_value(fpga->enable_gpio, 1);
+	fpga->uninitialised = false;
+
+	return 0;
+}
+
+static int __init upboard_fpga_detect_firmware(struct upboard_fpga *fpga)
+{
+	const unsigned int AAEON_MANUFACTURER_ID = 0x01;
+	const unsigned int SUPPORTED_FW_MAJOR = 0x0;
+	unsigned int platform_id, manufacturer_id;
+	unsigned int firmware_id, build, major, minor, patch;
+	int ret;
+
+	ret = regmap_read(fpga->regmap, UPFPGA_REG_PLATFORM_ID, &platform_id);
+	if (ret)
+		return ret;
+
+	manufacturer_id = platform_id & 0xff;
+	if (manufacturer_id != AAEON_MANUFACTURER_ID) {
+		dev_dbg(fpga->dev,
+			"driver not compatible with custom FPGA FW from manufacturer id 0x%02x. Exiting",
+			manufacturer_id);
+		return -ENODEV;
+	}
+
+	ret = regmap_read(fpga->regmap, UPFPGA_REG_FIRMWARE_ID, &firmware_id);
+	if (ret)
+		return ret;
+
+	build = (firmware_id >> 12) & 0xf;
+	major = (firmware_id >> 8) & 0xf;
+	minor = (firmware_id >> 4) & 0xf;
+	patch = firmware_id & 0xf;
+	if (major != SUPPORTED_FW_MAJOR) {
+		dev_dbg(fpga->dev, "unsupported FPGA FW v%u.%u.%u build 0x%02x",
+			major, minor, patch, build);
+		return -ENODEV;
+	}
+
+	dev_info(fpga->dev, "compatible FPGA FW v%u.%u.%u build 0x%02x",
+		 major, minor, patch, build);
+	return 0;
+}
+
+static const struct acpi_device_id upboard_fpga_acpi_match[] = {
+	{ "AANT0F00", (kernel_ulong_t)&upboard_up_fpga_data },
+	{ "AANT0F01", (kernel_ulong_t)&upboard_up2_fpga_data },
+	{ "AANT0F02", (kernel_ulong_t)&upboard_upcore_crex_fpga_data },
+	{ "AANT0F03", (kernel_ulong_t)&upboard_upcore_crst02_fpga_data },
+	{ "AANT0F04", (kernel_ulong_t)&upboard_up_fpga_data },
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, upboard_fpga_acpi_match);
+
+#define UPFPGA_QUIRK_UNINITIALISED  BIT(0)
+#define UPFPGA_QUIRK_HRV1_IS_PROTO2 BIT(1)
+#define UPFPGA_QUIRK_GPIO_LED       BIT(2)
+
+static const struct dmi_system_id upboard_dmi_table[] __initconst = {
+	{
+		.matches = { /* UP */
+			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "AAEON"),
+			DMI_EXACT_MATCH(DMI_BOARD_NAME, "UP-CHT01"),
+			DMI_EXACT_MATCH(DMI_BOARD_VERSION, "V0.4"),
+		},
+		.driver_data = (void *)UPFPGA_QUIRK_UNINITIALISED,
+	},
+	{
+		.matches = { /* UP2 */
+			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "AAEON"),
+			DMI_EXACT_MATCH(DMI_BOARD_NAME, "UP-APL01"),
+			DMI_EXACT_MATCH(DMI_BOARD_VERSION, "V0.3"),
+		},
+		.driver_data = (void *)(UPFPGA_QUIRK_UNINITIALISED |
+			UPFPGA_QUIRK_HRV1_IS_PROTO2),
+	},
+	{
+		.matches = { /* UP2 Pro*/
+			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "AAEON"),
+			DMI_EXACT_MATCH(DMI_BOARD_NAME, "UPN-APL01"),
+			DMI_EXACT_MATCH(DMI_BOARD_VERSION, "V1.0"),
+		},
+		.driver_data = (void *)UPFPGA_QUIRK_HRV1_IS_PROTO2,
+	},
+	{
+		.matches = { /* UP2 */
+			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "AAEON"),
+			DMI_EXACT_MATCH(DMI_BOARD_NAME, "UP-APL01"),
+			DMI_EXACT_MATCH(DMI_BOARD_VERSION, "V0.4"),
+		},
+		.driver_data = (void *)UPFPGA_QUIRK_HRV1_IS_PROTO2,
+	},
+	{
+		.matches = { /* UP Xtreme */
+			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "AAEON"),
+			DMI_EXACT_MATCH(DMI_BOARD_NAME, "UP-WHL01"),
+			DMI_EXACT_MATCH(DMI_BOARD_VERSION, "V0.1"),
+		},
+	},
+	{
+		.matches = { /* UP APL03 */
+			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "AAEON"),
+			DMI_EXACT_MATCH(DMI_BOARD_NAME, "UP-APL03"),
+			DMI_EXACT_MATCH(DMI_BOARD_VERSION, "V1.0"),
+		},
+		.driver_data = (void *)(UPFPGA_QUIRK_HRV1_IS_PROTO2 |
+			UPFPGA_QUIRK_GPIO_LED),
+	},
+	{ },
+};
+
+#define UPFPGA_PROTOCOL_V2_HRV 2
+
+static int __init upboard_fpga_probe(struct platform_device *pdev)
+{
+	struct upboard_fpga *fpga;
+	const struct acpi_device_id *id;
+	const struct upboard_fpga_data *fpga_data;
+	const struct dmi_system_id *system_id;
+	acpi_handle handle;
+	acpi_status status;
+	unsigned long long hrv;
+	unsigned long quirks = 0;
+	int ret;
+
+	id = acpi_match_device(upboard_fpga_acpi_match, &pdev->dev);
+	if (!id)
+		return -ENODEV;
+
+	handle = ACPI_HANDLE(&pdev->dev);
+	status = acpi_evaluate_integer(handle, "_HRV", NULL, &hrv);
+	if (ACPI_FAILURE(status)) {
+		dev_err(&pdev->dev, "failed to get PCTL revision");
+		return -ENODEV;
+	}
+
+	system_id = dmi_first_match(upboard_dmi_table);
+	if (system_id)
+		quirks = (unsigned long)system_id->driver_data;
+
+	if (hrv == 1 && (quirks & UPFPGA_QUIRK_HRV1_IS_PROTO2))
+		hrv = UPFPGA_PROTOCOL_V2_HRV;
+
+	if (hrv != UPFPGA_PROTOCOL_V2_HRV) {
+		dev_dbg(&pdev->dev, "unsupported PCTL revision: %llu", hrv);
+		return -ENODEV;
+	}
+
+	fpga_data = (const struct upboard_fpga_data *) id->driver_data;
+
+	fpga = devm_kzalloc(&pdev->dev, sizeof(*fpga), GFP_KERNEL);
+	if (!fpga)
+		return -ENOMEM;
+
+	if (quirks & UPFPGA_QUIRK_UNINITIALISED) {
+		dev_info(&pdev->dev, "FPGA not initialised by this BIOS");
+		fpga->uninitialised = true;
+	}
+
+	dev_set_drvdata(&pdev->dev, fpga);
+	fpga->dev = &pdev->dev;
+	fpga->regmap = devm_regmap_init(&pdev->dev, NULL, fpga,
+					fpga_data->regmapconf);
+	if (IS_ERR(fpga->regmap))
+		return PTR_ERR(fpga->regmap);
+
+	ret = upboard_fpga_gpio_init(fpga);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to init FPGA comm GPIOs: %d", ret);
+		return ret;
+	}
+
+	ret = upboard_fpga_detect_firmware(fpga);
+	if (ret)
+		return ret;
+
+	if (quirks & UPFPGA_QUIRK_GPIO_LED) {
+#define APL_GPIO_218	507
+		static struct gpio_led upboard_gpio_leds[] = {
+			{
+				.name = "upboard:blue:",
+				.gpio = APL_GPIO_218,
+				.default_state = LEDS_GPIO_DEFSTATE_KEEP,
+			},
+		};
+		static struct gpio_led_platform_data upboard_gpio_led_platform_data = {
+			.num_leds = ARRAY_SIZE(upboard_gpio_leds),
+			.leds = upboard_gpio_leds,
+		};
+		static const struct mfd_cell upboard_gpio_led_cells[] = {
+			{
+				.name = "leds-gpio",
+				.id = 0,
+				.platform_data = &upboard_gpio_led_platform_data,
+				.pdata_size = sizeof(upboard_gpio_led_platform_data),
+			},
+		};
+
+		ret =  devm_mfd_add_devices(&pdev->dev, 0, upboard_gpio_led_cells,
+				    ARRAY_SIZE(upboard_gpio_led_cells), NULL, 0, NULL);
+		if (ret) {
+			dev_err(&pdev->dev, "Failed to add GPIO leds");
+			return ret;
+		}
+
+	}
+
+	return devm_mfd_add_devices(&pdev->dev, 0, fpga_data->cells,
+				    fpga_data->ncells, NULL, 0, NULL);
+}
+
+static struct platform_driver upboard_fpga_driver = {
+	.driver = {
+		.name = "upboard-fpga",
+		.acpi_match_table = upboard_fpga_acpi_match,
+	},
+};
+
+module_platform_driver_probe(upboard_fpga_driver, upboard_fpga_probe);
+
+MODULE_AUTHOR("Javier Arteaga <javier@emutex.com>");
+MODULE_DESCRIPTION("UP Board FPGA driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/mfd/upboard-fpga.h b/include/linux/mfd/upboard-fpga.h
new file mode 100644
index 000000000000..e0940120514d
--- /dev/null
+++ b/include/linux/mfd/upboard-fpga.h
@@ -0,0 +1,49 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * UP Board FPGA MFD driver interface
+ *
+ * Copyright (c) 2017, Emutex Ltd. All rights reserved.
+ *
+ * Author: Javier Arteaga <javier@emutex.com>
+ */
+
+#ifndef __LINUX_MFD_UPBOARD_FPGA_H
+#define __LINUX_MFD_UPBOARD_FPGA_H
+
+#define UPFPGA_ADDRESS_SIZE  7
+#define UPFPGA_REGISTER_SIZE 16
+
+#define UPFPGA_READ_FLAG     (1 << UPFPGA_ADDRESS_SIZE)
+
+enum upboard_fpgareg {
+	UPFPGA_REG_PLATFORM_ID   = 0x10,
+	UPFPGA_REG_FIRMWARE_ID   = 0x11,
+	UPFPGA_REG_FUNC_EN0      = 0x20,
+	UPFPGA_REG_FUNC_EN1      = 0x21,
+	UPFPGA_REG_GPIO_EN0      = 0x30,
+	UPFPGA_REG_GPIO_EN1      = 0x31,
+	UPFPGA_REG_GPIO_EN2      = 0x32,
+	UPFPGA_REG_GPIO_DIR0     = 0x40,
+	UPFPGA_REG_GPIO_DIR1     = 0x41,
+	UPFPGA_REG_GPIO_DIR2     = 0x42,
+	UPFPGA_REG_MAX,
+};
+
+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;
+	bool uninitialised;
+};
+
+struct upboard_led_data {
+	unsigned int bit;
+	const char *colour;
+};
+
+#endif /*  __LINUX_MFD_UPBOARD_FPGA_H */
-- 
2.17.1

[PATCH 2/5] regmap: Expose regmap_writeable function to check if a register is writable

[chengwei]

Expose the regmap_writeable function for pinctrl-upboard reference.

Signed-off-by: Nicola Lunghi <nicola.lunghi@emutex.com>
Signed-off-by: chengwei <larry.lai@yunjingtech.com>
---
 drivers/base/regmap/internal.h | 5 -----
 drivers/base/regmap/regmap.c   | 5 +++++
 include/linux/regmap.h         | 6 ++++++
 3 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/base/regmap/internal.h b/drivers/base/regmap/internal.h
index da8996e7a1f1..eec477755a5c 100644
--- a/drivers/base/regmap/internal.h
+++ b/drivers/base/regmap/internal.h
@@ -190,11 +190,6 @@ struct regcache_ops {
 	int (*drop)(struct regmap *map, unsigned int min, unsigned int max);
 };
 
-bool regmap_cached(struct regmap *map, unsigned int reg);
-bool regmap_writeable(struct regmap *map, unsigned int reg);
-bool regmap_readable(struct regmap *map, unsigned int reg);
-bool regmap_volatile(struct regmap *map, unsigned int reg);
-bool regmap_precious(struct regmap *map, unsigned int reg);
 bool regmap_writeable_noinc(struct regmap *map, unsigned int reg);
 bool regmap_readable_noinc(struct regmap *map, unsigned int reg);
 
diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index fee221c5008c..182536fcce46 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -100,6 +100,7 @@ bool regmap_writeable(struct regmap *map, unsigned int reg)
 
 	return true;
 }
+EXPORT_SYMBOL_GPL(regmap_writeable);
 
 bool regmap_cached(struct regmap *map, unsigned int reg)
 {
@@ -123,6 +124,7 @@ bool regmap_cached(struct regmap *map, unsigned int reg)
 
 	return true;
 }
+EXPORT_SYMBOL_GPL(regmap_cached);
 
 bool regmap_readable(struct regmap *map, unsigned int reg)
 {
@@ -143,6 +145,7 @@ bool regmap_readable(struct regmap *map, unsigned int reg)
 
 	return true;
 }
+EXPORT_SYMBOL_GPL(regmap_readable);
 
 bool regmap_volatile(struct regmap *map, unsigned int reg)
 {
@@ -160,6 +163,7 @@ bool regmap_volatile(struct regmap *map, unsigned int reg)
 	else
 		return true;
 }
+EXPORT_SYMBOL_GPL(regmap_volatile);
 
 bool regmap_precious(struct regmap *map, unsigned int reg)
 {
@@ -174,6 +178,7 @@ bool regmap_precious(struct regmap *map, unsigned int reg)
 
 	return false;
 }
+EXPORT_SYMBOL_GPL(regmap_precious);
 
 bool regmap_writeable_noinc(struct regmap *map, unsigned int reg)
 {
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index 7cf2157134ac..4bfa681805cb 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -1225,6 +1225,12 @@ void regcache_mark_dirty(struct regmap *map);
 bool regmap_check_range_table(struct regmap *map, unsigned int reg,
 			      const struct regmap_access_table *table);
 
+bool regmap_cached(struct regmap *map, unsigned int reg);
+bool regmap_writeable(struct regmap *map, unsigned int reg);
+bool regmap_readable(struct regmap *map, unsigned int reg);
+bool regmap_volatile(struct regmap *map, unsigned int reg);
+bool regmap_precious(struct regmap *map, unsigned int reg);
+
 int regmap_register_patch(struct regmap *map, const struct reg_sequence *regs,
 			  int num_regs);
 int regmap_parse_val(struct regmap *map, const void *buf,
-- 
2.17.1

[PATCH 3/5] ACPI: acpi_node_add_pin_mapping added to header file

[chengwei]

Declare acpi_node_add_pin_mapping added for pinctrl-upboard
reference.

Signed-off-by: chengwei <larry.lai@yunjingtech.com>
---
 include/linux/acpi.h | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 6f64b2f3dc54..7b4668342de0 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -1203,6 +1203,11 @@ bool acpi_gpio_get_irq_resource(struct acpi_resource *ares,
 bool acpi_gpio_get_io_resource(struct acpi_resource *ares,
 			       struct acpi_resource_gpio **agpio);
 int acpi_dev_gpio_irq_get_by(struct acpi_device *adev, const char *name, int index);
+int acpi_node_add_pin_mapping(struct fwnode_handle *fwnode,
+			      const char *propname,
+			      const char *pinctl_name,
+			      unsigned int pin_offset,
+			      unsigned int npins);
 #else
 static inline bool acpi_gpio_get_irq_resource(struct acpi_resource *ares,
 					      struct acpi_resource_gpio **agpio)
@@ -1219,6 +1224,15 @@ static inline int acpi_dev_gpio_irq_get_by(struct acpi_device *adev,
 {
 	return -ENXIO;
 }
+
+static inline int acpi_node_add_pin_mapping(struct fwnode_handle *fwnode,
+					    const char *propname,
+					    const char *pinctl_name,
+					    unsigned int pin_offset,
+					    unsigned int npins)
+{
+	return -ENXIO;
+}
 #endif
 
 static inline int acpi_dev_gpio_irq_get(struct acpi_device *adev, int index)
-- 
2.17.1

[PATCH 4/5] GPIO ACPI: Add support to map GPIO resources to ranges

[chengwei]

Add a function to gpiolib to facilitate registering a pin controller for
a range of GPIO pins, but using ACPI resource references and without
claiming the GPIO resource.

Signed-off-by: chengwei <larry.lai@yunjingtech.com>
---
 drivers/gpio/gpiolib-acpi.c | 88 ++++++++++++++++++++++++++++++-------
 1 file changed, 71 insertions(+), 17 deletions(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 9be1376f9a62..d25c6cb610e3 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -1385,6 +1385,32 @@ static int acpi_find_gpio_count(struct acpi_resource *ares, void *data)
 	return 1;
 }
 
+static int acpi_gpio_count_from_property(struct acpi_device *adev,
+					 const char *propname)
+{
+	const struct acpi_gpio_mapping *gm;
+	const union acpi_object *obj;
+	int count = -ENOENT;
+	int ret;
+
+	ret = acpi_dev_get_property(adev, propname, ACPI_TYPE_ANY,
+				    &obj);
+	if (ret == 0) {
+		if (obj->type == ACPI_TYPE_LOCAL_REFERENCE)
+			count = 1;
+		else if (obj->type == ACPI_TYPE_PACKAGE)
+			count = acpi_gpio_package_count(obj);
+	} else if (adev->driver_gpios) {
+		for (gm = adev->driver_gpios; gm->name; gm++)
+			if (strcmp(propname, gm->name) == 0) {
+				count = gm->size;
+				break;
+			}
+	}
+
+	return count;
+}
+
 /**
  * acpi_gpio_count - count the GPIOs associated with a device / function
  * @dev:	GPIO consumer, can be %NULL for system-global GPIOs
@@ -1397,10 +1423,7 @@ static int acpi_find_gpio_count(struct acpi_resource *ares, void *data)
 int acpi_gpio_count(struct device *dev, const char *con_id)
 {
 	struct acpi_device *adev = ACPI_COMPANION(dev);
-	const union acpi_object *obj;
-	const struct acpi_gpio_mapping *gm;
 	int count = -ENOENT;
-	int ret;
 	char propname[32];
 	unsigned int i;
 
@@ -1413,20 +1436,7 @@ int acpi_gpio_count(struct device *dev, const char *con_id)
 			snprintf(propname, sizeof(propname), "%s",
 				 gpio_suffixes[i]);
 
-		ret = acpi_dev_get_property(adev, propname, ACPI_TYPE_ANY,
-					    &obj);
-		if (ret == 0) {
-			if (obj->type == ACPI_TYPE_LOCAL_REFERENCE)
-				count = 1;
-			else if (obj->type == ACPI_TYPE_PACKAGE)
-				count = acpi_gpio_package_count(obj);
-		} else if (adev->driver_gpios) {
-			for (gm = adev->driver_gpios; gm->name; gm++)
-				if (strcmp(propname, gm->name) == 0) {
-					count = gm->size;
-					break;
-				}
-		}
+		count = acpi_gpio_count_from_property(adev, propname);
 		if (count > 0)
 			break;
 	}
@@ -1449,6 +1459,50 @@ int acpi_gpio_count(struct device *dev, const char *con_id)
 	return count ? count : -ENOENT;
 }
 
+/**
+ * acpi_node_add_pin_mapping - add a pin mapping for named GPIO resources
+ * @fwnode: pointer to an ACPI firmware node to get the GPIO information from
+ * @propname: Property name of the GPIO
+ * @pinctrl_name: the dev_name() of the pin controller to map to
+ * @pin_offset: the start offset in the pin controller number space
+ * @npins: the maximum number of pins from the offset of each pin space (GPIO
+ *         and pin controller) to map
+ *
+ * Lookup the GPIO resources and map them individually to the specified pins.
+ */
+int acpi_node_add_pin_mapping(struct fwnode_handle *fwnode,
+			      const char *propname,
+			      const char *pinctl_name,
+			      unsigned int pin_offset,
+			      unsigned int npins)
+{
+	struct acpi_device *adev = to_acpi_device_node(fwnode);
+	int count, i;
+
+	count = acpi_gpio_count_from_property(adev, propname);
+	if (count < 0)
+		return count;
+
+	for (i = 0; i < count && i < npins; i++) {
+		struct gpio_desc *desc;
+		int ret;
+
+		desc = acpi_node_get_gpiod(fwnode, propname, i, NULL);
+		if (IS_ERR(desc))
+			return PTR_ERR(desc);
+
+		/* The GPIOs may not be contiguous, so add them 1-by-1 */
+		ret = gpiochip_add_pin_range(gpiod_to_chip(desc), pinctl_name,
+					     gpio_chip_hwgpio(desc),
+					     pin_offset + i, 1);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(acpi_node_add_pin_mapping);
+
 /* Run deferred acpi_gpiochip_request_irqs() */
 static int __init acpi_gpio_handle_deferred_request_irqs(void)
 {
-- 
2.17.1

[PATCH 5/5] pinctrl: Add support pin control for UP board CPLD/FPGA

[chengwei]

The UP Squared board <http://www.upboard.com> implements certain
features (pin control) through an on-board FPGA.

Signed-off-by: Javier Arteaga <javier@emutex.com>
[merge various fixes]
Signed-off-by: Nicola Lunghi <nicola.lunghi@emutex.com>
Signed-off-by: chengwei <larry.lai@yunjingtech.com>
---
 drivers/pinctrl/Kconfig           |   15 +
 drivers/pinctrl/Makefile          |    1 +
 drivers/pinctrl/pinctrl-upboard.c | 1003 +++++++++++++++++++++++++++++
 3 files changed, 1019 insertions(+)
 create mode 100644 drivers/pinctrl/pinctrl-upboard.c

diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index 1cf74b0c42e5..5d719eadecdb 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -483,6 +483,21 @@ config PINCTRL_THUNDERBAY
 	  rate control and direction control. This module will be
 	  called as pinctrl-thunderbay.
 
+config PINCTRL_UPBOARD
+	tristate "UP board FPGA pin controller"
+	depends on ACPI
+	depends on MFD_UPBOARD_FPGA
+	depends on X86
+	select GENERIC_PINCONF
+	select PINMUX
+	select PINCONF
+	help
+	  Pin controller for the FPGA GPIO lines on UP boards. Due to the
+	  hardware layout, these are meant to be controlled 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 e76f5cdc64b0..c366706d36e7 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -48,6 +48,7 @@ obj-$(CONFIG_PINCTRL_STMFX) 	+= pinctrl-stmfx.o
 obj-$(CONFIG_PINCTRL_SX150X)	+= pinctrl-sx150x.o
 obj-$(CONFIG_PINCTRL_TB10X)	+= pinctrl-tb10x.o
 obj-$(CONFIG_PINCTRL_THUNDERBAY) += pinctrl-thunderbay.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..39cd45357f6b
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-upboard.c
@@ -0,0 +1,1003 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * UP Board FPGA-based pin controller driver
+ *
+ * Copyright (c) 2017, Emutex Ltd. All rights reserved.
+ *
+ * Authors: Javier Arteaga <javier@emutex.com>
+ *          Dan O'Donovan <dan@emutex.com>
+ *          Nicola Lunghi <nicolal@emutex.com>
+ */
+
+#include <linux/acpi.h>
+#include <linux/dmi.h>
+#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
+#include <linux/gpio/driver.h>
+#include <linux/kernel.h>
+#include <linux/mfd/upboard-fpga.h>
+#include <linux/module.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinmux.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/string.h>
+
+#include "core.h"
+
+struct upboard_pin {
+	struct regmap_field *funcbit;
+	struct regmap_field *enbit;
+	struct regmap_field *dirbit;
+};
+
+struct upboard_bios {
+	const struct reg_sequence *patches;
+	size_t npatches;
+};
+
+struct upboard_pinctrl {
+	struct device *dev;
+	struct pinctrl_dev *pctldev;
+	struct regmap *regmap;
+	struct gpio_chip chip;
+	const unsigned int *rpi_mapping;
+};
+
+enum upboard_func0_fpgabit {
+	UPFPGA_I2C0_EN = 8,
+	UPFPGA_I2C1_EN = 9,
+	UPFPGA_CEC0_EN = 12,
+	UPFPGA_ADC0_EN = 14,
+};
+
+static const struct reg_field upboard_i2c0_reg =
+	REG_FIELD(UPFPGA_REG_FUNC_EN0, UPFPGA_I2C0_EN, UPFPGA_I2C0_EN);
+
+static const struct reg_field upboard_i2c1_reg =
+	REG_FIELD(UPFPGA_REG_FUNC_EN0, UPFPGA_I2C1_EN, UPFPGA_I2C1_EN);
+
+static const struct reg_field upboard_adc0_reg =
+	REG_FIELD(UPFPGA_REG_FUNC_EN0, UPFPGA_ADC0_EN, UPFPGA_ADC0_EN);
+
+#define UPBOARD_BIT_TO_PIN(r, bit) \
+	((r) * UPFPGA_REGISTER_SIZE + (bit))
+
+/*
+ * UP board data
+ */
+
+#define UPBOARD_UP_BIT_TO_PIN(r, id) (UPBOARD_BIT_TO_PIN(r, UPFPGA_UP_##id))
+
+#define UPBOARD_UP_PIN_ANON(r, bit)					\
+	{								\
+		.number = UPBOARD_BIT_TO_PIN(r, bit),			\
+	}
+
+#define UPBOARD_UP_PIN_NAME(r, id)					\
+	{								\
+		.number = UPBOARD_UP_BIT_TO_PIN(r, id),			\
+		.name = #id,						\
+	}
+
+#define UPBOARD_UP_PIN_FUNC(r, id, data)				\
+	{								\
+		.number = UPBOARD_UP_BIT_TO_PIN(r, id),			\
+		.name = #id,						\
+		.drv_data = (void *)(data),				\
+	}
+
+enum upboard_up_reg1_fpgabit {
+	UPFPGA_UP_I2C1_SDA,
+	UPFPGA_UP_I2C1_SCL,
+	UPFPGA_UP_ADC0,
+	UPFPGA_UP_GPIO17,
+	UPFPGA_UP_GPIO27,
+	UPFPGA_UP_GPIO22,
+	UPFPGA_UP_SPI_MOSI,
+	UPFPGA_UP_SPI_MISO,
+	UPFPGA_UP_SPI_CLK,
+	UPFPGA_UP_I2C0_SDA,
+	UPFPGA_UP_GPIO5,
+	UPFPGA_UP_GPIO6,
+	UPFPGA_UP_PWM1,
+	UPFPGA_UP_I2S_FRM,
+	UPFPGA_UP_GPIO26,
+	UPFPGA_UP_UART1_TX,
+};
+
+enum upboard_up_reg2_fpgabit {
+	UPFPGA_UP_UART1_RX,
+	UPFPGA_UP_I2S_CLK,
+	UPFPGA_UP_GPIO23,
+	UPFPGA_UP_GPIO24,
+	UPFPGA_UP_GPIO25,
+	UPFPGA_UP_SPI_CS0,
+	UPFPGA_UP_SPI_CS1,
+	UPFPGA_UP_I2C0_SCL,
+	UPFPGA_UP_PWM0,
+	UPFPGA_UP_GPIO16,
+	UPFPGA_UP_I2S_DIN,
+	UPFPGA_UP_I2S_DOUT,
+};
+
+static struct pinctrl_pin_desc upboard_up_pins[] = {
+	UPBOARD_UP_PIN_FUNC(0, I2C1_SDA, &upboard_i2c1_reg),
+	UPBOARD_UP_PIN_FUNC(0, I2C1_SCL, &upboard_i2c1_reg),
+	UPBOARD_UP_PIN_FUNC(0, ADC0, &upboard_adc0_reg),
+	UPBOARD_UP_PIN_NAME(0, GPIO17),
+	UPBOARD_UP_PIN_NAME(0, GPIO27),
+	UPBOARD_UP_PIN_NAME(0, GPIO22),
+	UPBOARD_UP_PIN_NAME(0, SPI_MOSI),
+	UPBOARD_UP_PIN_NAME(0, SPI_MISO),
+	UPBOARD_UP_PIN_NAME(0, SPI_CLK),
+	UPBOARD_UP_PIN_FUNC(0, I2C0_SDA, &upboard_i2c0_reg),
+	UPBOARD_UP_PIN_NAME(0, GPIO5),
+	UPBOARD_UP_PIN_NAME(0, GPIO6),
+	UPBOARD_UP_PIN_NAME(0, PWM1),
+	UPBOARD_UP_PIN_NAME(0, I2S_FRM),
+	UPBOARD_UP_PIN_NAME(0, GPIO26),
+	UPBOARD_UP_PIN_NAME(0, UART1_TX),
+	/* register 1 */
+	UPBOARD_UP_PIN_NAME(1, UART1_RX),
+	UPBOARD_UP_PIN_NAME(1, I2S_CLK),
+	UPBOARD_UP_PIN_NAME(1, GPIO23),
+	UPBOARD_UP_PIN_NAME(1, GPIO24),
+	UPBOARD_UP_PIN_NAME(1, GPIO25),
+	UPBOARD_UP_PIN_NAME(1, SPI_CS0),
+	UPBOARD_UP_PIN_NAME(1, SPI_CS1),
+	UPBOARD_UP_PIN_FUNC(1, I2C0_SCL, &upboard_i2c0_reg),
+	UPBOARD_UP_PIN_NAME(1, PWM0),
+	UPBOARD_UP_PIN_NAME(1, GPIO16),
+	UPBOARD_UP_PIN_NAME(1, I2S_DIN),
+	UPBOARD_UP_PIN_NAME(1, I2S_DOUT),
+};
+
+static const unsigned int upboard_up_rpi_mapping[] = {
+	UPBOARD_UP_BIT_TO_PIN(0, I2C0_SDA),
+	UPBOARD_UP_BIT_TO_PIN(1, I2C0_SCL),
+	UPBOARD_UP_BIT_TO_PIN(0, I2C1_SDA),
+	UPBOARD_UP_BIT_TO_PIN(0, I2C1_SCL),
+	UPBOARD_UP_BIT_TO_PIN(0, ADC0),
+	UPBOARD_UP_BIT_TO_PIN(0, GPIO5),
+	UPBOARD_UP_BIT_TO_PIN(0, GPIO6),
+	UPBOARD_UP_BIT_TO_PIN(1, SPI_CS1),
+	UPBOARD_UP_BIT_TO_PIN(1, SPI_CS0),
+	UPBOARD_UP_BIT_TO_PIN(0, SPI_MISO),
+	UPBOARD_UP_BIT_TO_PIN(0, SPI_MOSI),
+	UPBOARD_UP_BIT_TO_PIN(0, SPI_CLK),
+	UPBOARD_UP_BIT_TO_PIN(1, PWM0),
+	UPBOARD_UP_BIT_TO_PIN(0, PWM1),
+	UPBOARD_UP_BIT_TO_PIN(0, UART1_TX),
+	UPBOARD_UP_BIT_TO_PIN(1, UART1_RX),
+	UPBOARD_UP_BIT_TO_PIN(1, GPIO16),
+	UPBOARD_UP_BIT_TO_PIN(0, GPIO17),
+	UPBOARD_UP_BIT_TO_PIN(1, I2S_CLK),
+	UPBOARD_UP_BIT_TO_PIN(0, I2S_FRM),
+	UPBOARD_UP_BIT_TO_PIN(1, I2S_DIN),
+	UPBOARD_UP_BIT_TO_PIN(1, I2S_DOUT),
+	UPBOARD_UP_BIT_TO_PIN(0, GPIO22),
+	UPBOARD_UP_BIT_TO_PIN(1, GPIO23),
+	UPBOARD_UP_BIT_TO_PIN(1, GPIO24),
+	UPBOARD_UP_BIT_TO_PIN(1, GPIO25),
+	UPBOARD_UP_BIT_TO_PIN(0, GPIO26),
+	UPBOARD_UP_BIT_TO_PIN(0, GPIO27),
+};
+
+/*
+ * Init patches applied to the registers until the BIOS sets proper defaults
+ */
+static const struct reg_sequence upboard_up_reg_patches[] __initconst = {
+	{ UPFPGA_REG_FUNC_EN0,
+		// enable I2C voltage-level shifters
+		BIT(UPFPGA_I2C0_EN) |
+		BIT(UPFPGA_I2C1_EN) |
+		// enable adc
+		BIT(UPFPGA_ADC0_EN)
+	},
+	/* HAT function pins initially set as inputs */
+	{ UPFPGA_REG_GPIO_DIR0,
+		BIT(UPFPGA_UP_I2C1_SDA)	    |
+		BIT(UPFPGA_UP_I2C1_SCL)	    |
+		BIT(UPFPGA_UP_ADC0)	    |
+		BIT(UPFPGA_UP_GPIO27)	    |
+		BIT(UPFPGA_UP_GPIO22)	    |
+		BIT(UPFPGA_UP_SPI_MISO)	    |
+		BIT(UPFPGA_UP_I2C0_SDA)	    |
+		BIT(UPFPGA_UP_GPIO5)	    |
+		BIT(UPFPGA_UP_GPIO6)	    |
+		BIT(UPFPGA_UP_GPIO26)
+	},
+	{ UPFPGA_REG_GPIO_DIR1,
+		BIT(UPFPGA_UP_UART1_RX)	|
+		BIT(UPFPGA_UP_GPIO23)	|
+		BIT(UPFPGA_UP_GPIO24)	|
+		BIT(UPFPGA_UP_GPIO25)	|
+		BIT(UPFPGA_UP_I2C0_SCL)	|
+		BIT(UPFPGA_UP_GPIO16)	|
+		BIT(UPFPGA_UP_I2S_DIN)
+	},
+};
+
+static const struct upboard_bios upboard_up_bios_info_dvt __initconst = {
+	.patches = upboard_up_reg_patches,
+	.npatches = ARRAY_SIZE(upboard_up_reg_patches),
+};
+
+/*
+ * UP^2 board data
+ */
+
+#define UPBOARD_UP2_BIT_TO_PIN(r, id) (UPBOARD_BIT_TO_PIN(r, UPFPGA_UP2_##id))
+
+#define UPBOARD_UP2_PIN_ANON(r, bit)					\
+	{								\
+		.number = UPBOARD_BIT_TO_PIN(r, bit),			\
+	}
+
+#define UPBOARD_UP2_PIN_NAME(r, id)					\
+	{								\
+		.number = UPBOARD_UP2_BIT_TO_PIN(r, id),		\
+		.name = #id,						\
+	}
+
+#define UPBOARD_UP2_PIN_FUNC(r, id, data)				\
+	{								\
+		.number = UPBOARD_UP2_BIT_TO_PIN(r, id),		\
+		.name = #id,						\
+		.drv_data = (void *)(data),				\
+	}
+
+enum upboard_up2_reg0_fpgabit {
+	UPFPGA_UP2_UART1_TXD,
+	UPFPGA_UP2_UART1_RXD,
+	UPFPGA_UP2_UART1_RTS,
+	UPFPGA_UP2_UART1_CTS,
+	UPFPGA_UP2_GPIO3,
+	UPFPGA_UP2_GPIO5,
+	UPFPGA_UP2_GPIO6,
+	UPFPGA_UP2_GPIO11,
+	UPFPGA_UP2_EXHAT_LVDS1n,
+	UPFPGA_UP2_EXHAT_LVDS1p,
+	UPFPGA_UP2_SPI2_TXD,
+	UPFPGA_UP2_SPI2_RXD,
+	UPFPGA_UP2_SPI2_FS1,
+	UPFPGA_UP2_SPI2_FS0,
+	UPFPGA_UP2_SPI2_CLK,
+	UPFPGA_UP2_SPI1_TXD,
+};
+
+enum upboard_up2_reg1_fpgabit {
+	UPFPGA_UP2_SPI1_RXD,
+	UPFPGA_UP2_SPI1_FS1,
+	UPFPGA_UP2_SPI1_FS0,
+	UPFPGA_UP2_SPI1_CLK,
+	UPFPGA_UP2_BIT20,
+	UPFPGA_UP2_BIT21,
+	UPFPGA_UP2_BIT22,
+	UPFPGA_UP2_BIT23,
+	UPFPGA_UP2_PWM1,
+	UPFPGA_UP2_PWM0,
+	UPFPGA_UP2_EXHAT_LVDS0n,
+	UPFPGA_UP2_EXHAT_LVDS0p,
+	UPFPGA_UP2_I2C0_SCL,
+	UPFPGA_UP2_I2C0_SDA,
+	UPFPGA_UP2_I2C1_SCL,
+	UPFPGA_UP2_I2C1_SDA,
+};
+
+enum upboard_up2_reg2_fpgabit {
+	UPFPGA_UP2_EXHAT_LVDS3n,
+	UPFPGA_UP2_EXHAT_LVDS3p,
+	UPFPGA_UP2_EXHAT_LVDS4n,
+	UPFPGA_UP2_EXHAT_LVDS4p,
+	UPFPGA_UP2_EXHAT_LVDS5n,
+	UPFPGA_UP2_EXHAT_LVDS5p,
+	UPFPGA_UP2_I2S_SDO,
+	UPFPGA_UP2_I2S_SDI,
+	UPFPGA_UP2_I2S_WS_SYNC,
+	UPFPGA_UP2_I2S_BCLK,
+	UPFPGA_UP2_EXHAT_LVDS6n,
+	UPFPGA_UP2_EXHAT_LVDS6p,
+	UPFPGA_UP2_EXHAT_LVDS7n,
+	UPFPGA_UP2_EXHAT_LVDS7p,
+	UPFPGA_UP2_EXHAT_LVDS2n,
+	UPFPGA_UP2_EXHAT_LVDS2p,
+};
+
+static struct pinctrl_pin_desc upboard_up2_pins[] = {
+	UPBOARD_UP2_PIN_NAME(0, UART1_TXD),
+	UPBOARD_UP2_PIN_NAME(0, UART1_RXD),
+	UPBOARD_UP2_PIN_NAME(0, UART1_RTS),
+	UPBOARD_UP2_PIN_NAME(0, UART1_CTS),
+	UPBOARD_UP2_PIN_NAME(0, GPIO3),
+	UPBOARD_UP2_PIN_NAME(0, GPIO5),
+	UPBOARD_UP2_PIN_NAME(0, GPIO6),
+	UPBOARD_UP2_PIN_NAME(0, GPIO11),
+	UPBOARD_UP2_PIN_NAME(0, EXHAT_LVDS1n),
+	UPBOARD_UP2_PIN_NAME(0, EXHAT_LVDS1p),
+	UPBOARD_UP2_PIN_NAME(0, SPI2_TXD),
+	UPBOARD_UP2_PIN_NAME(0, SPI2_RXD),
+	UPBOARD_UP2_PIN_NAME(0, SPI2_FS1),
+	UPBOARD_UP2_PIN_NAME(0, SPI2_FS0),
+	UPBOARD_UP2_PIN_NAME(0, SPI2_CLK),
+	UPBOARD_UP2_PIN_NAME(0, SPI1_TXD),
+	UPBOARD_UP2_PIN_NAME(1, SPI1_RXD),
+	UPBOARD_UP2_PIN_NAME(1, SPI1_FS1),
+	UPBOARD_UP2_PIN_NAME(1, SPI1_FS0),
+	UPBOARD_UP2_PIN_NAME(1, SPI1_CLK),
+	UPBOARD_UP2_PIN_ANON(1, 4),
+	UPBOARD_UP2_PIN_ANON(1, 5),
+	UPBOARD_UP2_PIN_ANON(1, 6),
+	UPBOARD_UP2_PIN_ANON(1, 7),
+	UPBOARD_UP2_PIN_NAME(1, PWM1),
+	UPBOARD_UP2_PIN_NAME(1, PWM0),
+	UPBOARD_UP2_PIN_NAME(1, EXHAT_LVDS0n),
+	UPBOARD_UP2_PIN_NAME(1, EXHAT_LVDS0p),
+	UPBOARD_UP2_PIN_FUNC(1, I2C0_SCL, &upboard_i2c0_reg),
+	UPBOARD_UP2_PIN_FUNC(1, I2C0_SDA, &upboard_i2c0_reg),
+	UPBOARD_UP2_PIN_FUNC(1, I2C1_SCL, &upboard_i2c1_reg),
+	UPBOARD_UP2_PIN_FUNC(1, I2C1_SDA, &upboard_i2c1_reg),
+	UPBOARD_UP2_PIN_NAME(2, EXHAT_LVDS3n),
+	UPBOARD_UP2_PIN_NAME(2, EXHAT_LVDS3p),
+	UPBOARD_UP2_PIN_NAME(2, EXHAT_LVDS4n),
+	UPBOARD_UP2_PIN_NAME(2, EXHAT_LVDS4p),
+	UPBOARD_UP2_PIN_NAME(2, EXHAT_LVDS5n),
+	UPBOARD_UP2_PIN_NAME(2, EXHAT_LVDS5p),
+	UPBOARD_UP2_PIN_NAME(2, I2S_SDO),
+	UPBOARD_UP2_PIN_NAME(2, I2S_SDI),
+	UPBOARD_UP2_PIN_NAME(2, I2S_WS_SYNC),
+	UPBOARD_UP2_PIN_NAME(2, I2S_BCLK),
+	UPBOARD_UP2_PIN_NAME(2, EXHAT_LVDS6n),
+	UPBOARD_UP2_PIN_NAME(2, EXHAT_LVDS6p),
+	UPBOARD_UP2_PIN_NAME(2, EXHAT_LVDS7n),
+	UPBOARD_UP2_PIN_NAME(2, EXHAT_LVDS7p),
+	UPBOARD_UP2_PIN_NAME(2, EXHAT_LVDS2n),
+	UPBOARD_UP2_PIN_NAME(2, EXHAT_LVDS2p),
+};
+
+static const unsigned int upboard_up2_rpi_mapping[] = {
+	UPBOARD_UP2_BIT_TO_PIN(1, I2C0_SDA),
+	UPBOARD_UP2_BIT_TO_PIN(1, I2C0_SCL),
+	UPBOARD_UP2_BIT_TO_PIN(1, I2C1_SDA),
+	UPBOARD_UP2_BIT_TO_PIN(1, I2C1_SCL),
+	UPBOARD_UP2_BIT_TO_PIN(0, GPIO3),
+	UPBOARD_UP2_BIT_TO_PIN(0, GPIO11),
+	UPBOARD_UP2_BIT_TO_PIN(0, SPI2_CLK),
+	UPBOARD_UP2_BIT_TO_PIN(1, SPI1_FS1),
+	UPBOARD_UP2_BIT_TO_PIN(1, SPI1_FS0),
+	UPBOARD_UP2_BIT_TO_PIN(1, SPI1_RXD),
+	UPBOARD_UP2_BIT_TO_PIN(0, SPI1_TXD),
+	UPBOARD_UP2_BIT_TO_PIN(1, SPI1_CLK),
+	UPBOARD_UP2_BIT_TO_PIN(1, PWM0),
+	UPBOARD_UP2_BIT_TO_PIN(1, PWM1),
+	UPBOARD_UP2_BIT_TO_PIN(0, UART1_TXD),
+	UPBOARD_UP2_BIT_TO_PIN(0, UART1_RXD),
+	UPBOARD_UP2_BIT_TO_PIN(0, UART1_CTS),
+	UPBOARD_UP2_BIT_TO_PIN(0, UART1_RTS),
+	UPBOARD_UP2_BIT_TO_PIN(2, I2S_BCLK),
+	UPBOARD_UP2_BIT_TO_PIN(2, I2S_WS_SYNC),
+	UPBOARD_UP2_BIT_TO_PIN(2, I2S_SDI),
+	UPBOARD_UP2_BIT_TO_PIN(2, I2S_SDO),
+	UPBOARD_UP2_BIT_TO_PIN(0, GPIO6),
+	UPBOARD_UP2_BIT_TO_PIN(0, SPI2_FS1),
+	UPBOARD_UP2_BIT_TO_PIN(0, SPI2_RXD),
+	UPBOARD_UP2_BIT_TO_PIN(0, SPI2_TXD),
+	UPBOARD_UP2_BIT_TO_PIN(0, SPI2_FS0),
+	UPBOARD_UP2_BIT_TO_PIN(0, GPIO5),
+};
+
+/*
+ * Init patches applied to the registers until the BIOS sets proper defaults
+ */
+static const struct reg_sequence upboard_up2_reg_patches[] __initconst = {
+	// enable I2C voltage-level shifters
+	{ UPFPGA_REG_FUNC_EN0,
+		BIT(UPFPGA_I2C0_EN) |
+		BIT(UPFPGA_I2C1_EN)
+	},
+	// HAT function pins initially set as inputs
+	{ UPFPGA_REG_GPIO_DIR0,
+		BIT(UPFPGA_UP2_UART1_RXD) |
+		BIT(UPFPGA_UP2_UART1_CTS)
+	},
+	{ UPFPGA_REG_GPIO_DIR1,
+		BIT(UPFPGA_UP2_SPI1_RXD)
+	},
+	// HAT function pins initially enabled (i.e. not hi-Z)
+	{ UPFPGA_REG_GPIO_EN0,
+		BIT(UPFPGA_UP2_UART1_TXD) |
+		BIT(UPFPGA_UP2_UART1_RXD) |
+		BIT(UPFPGA_UP2_UART1_RTS) |
+		BIT(UPFPGA_UP2_UART1_CTS) |
+		BIT(UPFPGA_UP2_SPI1_TXD)
+	},
+	{ UPFPGA_REG_GPIO_EN1,
+		BIT(UPFPGA_UP2_SPI1_RXD) |
+		BIT(UPFPGA_UP2_SPI1_FS1) |
+		BIT(UPFPGA_UP2_SPI1_FS0) |
+		BIT(UPFPGA_UP2_SPI1_CLK) |
+		BIT(UPFPGA_UP2_PWM1) |
+		BIT(UPFPGA_UP2_PWM0)
+	},
+};
+
+static const struct upboard_bios upboard_up2_bios_info_v0_3 __initconst = {
+	.patches = upboard_up2_reg_patches,
+	.npatches = ARRAY_SIZE(upboard_up2_reg_patches),
+};
+
+/*
+ * UP Core board + CREX carrier board data
+ */
+
+#define UPBOARD_UPCORE_CREX_BIT_TO_PIN(r, id)				\
+	(UPBOARD_BIT_TO_PIN(r, UPFPGA_UPCORE_CREX_##id))
+
+#define UPBOARD_UPCORE_CREX_PIN_ANON(r, bit)				\
+	{								\
+		.number = UPBOARD_BIT_TO_PIN(r, bit),			\
+	}
+
+#define UPBOARD_UPCORE_CREX_PIN_NAME(r, id)				\
+	{								\
+		.number = UPBOARD_UPCORE_CREX_BIT_TO_PIN(r, id),	\
+		.name = #id,						\
+	}
+
+#define UPBOARD_UPCORE_CREX_PIN_FUNC(r, id, data)			\
+	{								\
+		.number = UPBOARD_UPCORE_CREX_BIT_TO_PIN(r, id),	\
+		.name = #id,						\
+		.drv_data = (void *)(data),				\
+	}
+
+enum upboard_upcore_crex_reg1_fpgabit {
+	UPFPGA_UPCORE_CREX_I2C0_SDA,
+	UPFPGA_UPCORE_CREX_I2C0_SCL,
+	UPFPGA_UPCORE_CREX_I2C1_SDA,
+	UPFPGA_UPCORE_CREX_I2C1_SCL,
+	UPFPGA_UPCORE_CREX_SPI2_CS0,
+	UPFPGA_UPCORE_CREX_SPI2_CS1,
+	UPFPGA_UPCORE_CREX_SPI2_MOSI,
+	UPFPGA_UPCORE_CREX_SPI2_MISO,
+	UPFPGA_UPCORE_CREX_SPI2_CLK,
+	UPFPGA_UPCORE_CREX_UART1_TXD,
+	UPFPGA_UPCORE_CREX_UART1_RXD,
+	UPFPGA_UPCORE_CREX_PWM0,
+	UPFPGA_UPCORE_CREX_PWM1,
+	UPFPGA_UPCORE_CREX_I2S2_FRM,
+	UPFPGA_UPCORE_CREX_I2S2_CLK,
+	UPFPGA_UPCORE_CREX_I2S2_RX,
+};
+
+enum upboard_upcore_crex_reg2_fpgabit {
+	UPFPGA_UPCORE_CREX_I2S2_TX,
+	UPFPGA_UPCORE_CREX_GPIO0,
+	UPFPGA_UPCORE_CREX_GPIO2,
+	UPFPGA_UPCORE_CREX_GPIO3,
+	UPFPGA_UPCORE_CREX_GPIO4,
+	UPFPGA_UPCORE_CREX_GPIO9,
+};
+
+static struct pinctrl_pin_desc upboard_upcore_crex_pins[] = {
+	UPBOARD_UPCORE_CREX_PIN_FUNC(0, I2C0_SDA, &upboard_i2c0_reg),
+	UPBOARD_UPCORE_CREX_PIN_FUNC(0, I2C0_SCL, &upboard_i2c0_reg),
+	UPBOARD_UPCORE_CREX_PIN_FUNC(0, I2C1_SDA, &upboard_i2c1_reg),
+	UPBOARD_UPCORE_CREX_PIN_FUNC(0, I2C1_SCL, &upboard_i2c1_reg),
+	UPBOARD_UPCORE_CREX_PIN_NAME(0, SPI2_CS0),
+	UPBOARD_UPCORE_CREX_PIN_NAME(0, SPI2_CS1),
+	UPBOARD_UPCORE_CREX_PIN_NAME(0, SPI2_MOSI),
+	UPBOARD_UPCORE_CREX_PIN_NAME(0, SPI2_MISO),
+	UPBOARD_UPCORE_CREX_PIN_NAME(0, SPI2_CLK),
+	UPBOARD_UPCORE_CREX_PIN_NAME(0, UART1_TXD),
+	UPBOARD_UPCORE_CREX_PIN_NAME(0, UART1_RXD),
+	UPBOARD_UPCORE_CREX_PIN_NAME(0, PWM0),
+	UPBOARD_UPCORE_CREX_PIN_NAME(0, PWM1),
+	UPBOARD_UPCORE_CREX_PIN_NAME(0, I2S2_FRM),
+	UPBOARD_UPCORE_CREX_PIN_NAME(0, I2S2_CLK),
+	UPBOARD_UPCORE_CREX_PIN_NAME(0, I2S2_RX),
+	/* register 1 */
+	UPBOARD_UPCORE_CREX_PIN_NAME(1, I2S2_TX),
+	UPBOARD_UPCORE_CREX_PIN_NAME(1, GPIO0),
+	UPBOARD_UPCORE_CREX_PIN_FUNC(1, GPIO2, &upboard_adc0_reg),
+	UPBOARD_UPCORE_CREX_PIN_NAME(1, GPIO3),
+	UPBOARD_UPCORE_CREX_PIN_NAME(1, GPIO4),
+	UPBOARD_UPCORE_CREX_PIN_NAME(1, GPIO9),
+};
+
+static unsigned int upboard_upcore_crex_rpi_mapping[] = {
+	UPBOARD_UPCORE_CREX_BIT_TO_PIN(0, I2C0_SDA),
+	UPBOARD_UPCORE_CREX_BIT_TO_PIN(0, I2C0_SCL),
+	UPBOARD_UPCORE_CREX_BIT_TO_PIN(0, I2C1_SDA),
+	UPBOARD_UPCORE_CREX_BIT_TO_PIN(0, I2C1_SCL),
+	UPBOARD_UPCORE_CREX_BIT_TO_PIN(1, GPIO0),
+	UPBOARD_UPCORE_CREX_BIT_TO_PIN(1, GPIO2),
+	UPBOARD_UPCORE_CREX_BIT_TO_PIN(1, GPIO3),
+	UPBOARD_UPCORE_CREX_BIT_TO_PIN(0, SPI2_CS1),
+	UPBOARD_UPCORE_CREX_BIT_TO_PIN(0, SPI2_CS0),
+	UPBOARD_UPCORE_CREX_BIT_TO_PIN(0, SPI2_MISO),
+	UPBOARD_UPCORE_CREX_BIT_TO_PIN(0, SPI2_MOSI),
+	UPBOARD_UPCORE_CREX_BIT_TO_PIN(0, SPI2_CLK),
+	UPBOARD_UPCORE_CREX_BIT_TO_PIN(0, PWM0),
+	UPBOARD_UPCORE_CREX_BIT_TO_PIN(0, PWM1),
+	UPBOARD_UPCORE_CREX_BIT_TO_PIN(0, UART1_TXD),
+	UPBOARD_UPCORE_CREX_BIT_TO_PIN(0, UART1_RXD),
+	UPBOARD_UPCORE_CREX_BIT_TO_PIN(1, GPIO9),
+	UPBOARD_UPCORE_CREX_BIT_TO_PIN(1, GPIO4),
+	UPBOARD_UPCORE_CREX_BIT_TO_PIN(0, I2S2_CLK),
+	UPBOARD_UPCORE_CREX_BIT_TO_PIN(0, I2S2_FRM),
+	UPBOARD_UPCORE_CREX_BIT_TO_PIN(0, I2S2_RX),
+	UPBOARD_UPCORE_CREX_BIT_TO_PIN(1, I2S2_TX),
+};
+
+/*
+ * Init patches applied to the registers until the BIOS sets proper defaults
+ */
+static const struct reg_sequence upboard_upcore_crex_reg_patches[] __initconst = {
+	// enable I2C voltage-level shifters
+	{ UPFPGA_REG_FUNC_EN0,
+		BIT(UPFPGA_I2C0_EN) |
+		BIT(UPFPGA_I2C1_EN)
+	},
+	// HAT function pins initially set as inputs
+	{ UPFPGA_REG_GPIO_DIR0,
+		BIT(UPFPGA_UPCORE_CREX_SPI2_MISO) |
+		BIT(UPFPGA_UPCORE_CREX_UART1_RXD) |
+		BIT(UPFPGA_UPCORE_CREX_I2S2_FRM) |
+		BIT(UPFPGA_UPCORE_CREX_I2S2_CLK) |
+		BIT(UPFPGA_UPCORE_CREX_I2S2_RX)
+	},
+};
+
+static const struct upboard_bios upboard_upcore_crex_bios_info __initconst = {
+	.patches = upboard_upcore_crex_reg_patches,
+	.npatches = ARRAY_SIZE(upboard_upcore_crex_reg_patches),
+};
+
+/*
+ * UP Core board + CRST02 carrier board data
+ */
+
+#define upboard_upcore_crst02_pins        upboard_upcore_crex_pins
+#define upboard_upcore_crst02_rpi_mapping upboard_upcore_crex_rpi_mapping
+
+/*
+ * Init patches applied to the registers until the BIOS sets proper defaults
+ */
+static const struct reg_sequence upboard_upcore_crst02_reg_patches[] __initconst = {
+	// enable I2C voltage-level shifters
+	{ UPFPGA_REG_FUNC_EN0,
+		BIT(UPFPGA_I2C0_EN) |
+		BIT(UPFPGA_I2C1_EN)
+	},
+	// HAT function pins initially set as inputs
+	{ UPFPGA_REG_GPIO_DIR0,
+		BIT(UPFPGA_UPCORE_CREX_SPI2_MISO) |
+		BIT(UPFPGA_UPCORE_CREX_UART1_RXD) |
+		BIT(UPFPGA_UPCORE_CREX_I2S2_FRM) |
+		BIT(UPFPGA_UPCORE_CREX_I2S2_CLK) |
+		BIT(UPFPGA_UPCORE_CREX_I2S2_RX)
+	},
+	// HAT function pins initially enabled (i.e. not hi-Z)
+	{ UPFPGA_REG_GPIO_EN0,
+		BIT(UPFPGA_UPCORE_CREX_SPI2_CS0) |
+		BIT(UPFPGA_UPCORE_CREX_SPI2_MOSI) |
+		BIT(UPFPGA_UPCORE_CREX_SPI2_MISO) |
+		BIT(UPFPGA_UPCORE_CREX_SPI2_CLK) |
+		BIT(UPFPGA_UPCORE_CREX_UART1_TXD) |
+		BIT(UPFPGA_UPCORE_CREX_UART1_RXD) |
+		BIT(UPFPGA_UPCORE_CREX_PWM0) |
+		BIT(UPFPGA_UPCORE_CREX_PWM1) |
+		BIT(UPFPGA_UPCORE_CREX_I2S2_FRM) |
+		BIT(UPFPGA_UPCORE_CREX_I2S2_CLK) |
+		BIT(UPFPGA_UPCORE_CREX_I2S2_RX)
+	},
+	{ UPFPGA_REG_GPIO_EN1,
+		BIT(UPFPGA_UPCORE_CREX_I2S2_TX)
+	},
+};
+
+static const struct upboard_bios upboard_upcore_crst02_bios_info __initconst = {
+	.patches = upboard_upcore_crst02_reg_patches,
+	.npatches = ARRAY_SIZE(upboard_upcore_crst02_reg_patches),
+};
+
+static int upboard_set_mux(struct pinctrl_dev *pctldev, unsigned int function,
+			   unsigned int group)
+{
+	return 0;
+};
+
+static int upboard_gpio_request_enable(struct pinctrl_dev *pctldev,
+				       struct pinctrl_gpio_range *range,
+				       unsigned int pin)
+{
+	const struct pin_desc * const pd = pin_desc_get(pctldev, pin);
+	const struct upboard_pin *p;
+	int ret;
+
+	if (!pd)
+		return -EINVAL;
+	p = pd->drv_data;
+
+	if (p->funcbit) {
+		ret = regmap_field_write(p->funcbit, 0);
+		if (ret)
+			return ret;
+	}
+
+	if (p->enbit) {
+		ret = regmap_field_write(p->enbit, 1);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+};
+
+static int upboard_gpio_set_direction(struct pinctrl_dev *pctldev,
+				      struct pinctrl_gpio_range *range,
+				      unsigned int pin, bool input)
+{
+	const struct pin_desc * const pd = pin_desc_get(pctldev, pin);
+	const struct upboard_pin *p;
+
+	if (!pd)
+		return -EINVAL;
+	p = pd->drv_data;
+
+	return regmap_field_write(p->dirbit, input);
+};
+
+static int upboard_get_functions_count(struct pinctrl_dev *pctldev)
+{
+	return 0;
+}
+
+static const char *upboard_get_function_name(struct pinctrl_dev *pctldev,
+				     unsigned int selector)
+{
+	return NULL;
+}
+
+static int upboard_get_function_groups(struct pinctrl_dev *pctldev,
+			       unsigned int selector,
+			       const char * const **groups,
+			       unsigned int *num_groups)
+{
+	*groups = NULL;
+	*num_groups = 0;
+	return 0;
+}
+
+static const struct pinmux_ops upboard_pinmux_ops = {
+	.get_functions_count = upboard_get_functions_count,
+	.get_function_groups = upboard_get_function_groups,
+	.get_function_name = upboard_get_function_name,
+	.set_mux = upboard_set_mux,
+	.gpio_request_enable = upboard_gpio_request_enable,
+	.gpio_set_direction = upboard_gpio_set_direction,
+};
+
+static int upboard_get_groups_count(struct pinctrl_dev *pctldev)
+{
+	return 0;
+}
+
+static const char *upboard_get_group_name(struct pinctrl_dev *pctldev,
+					  unsigned int selector)
+{
+	return NULL;
+}
+
+static const struct pinctrl_ops upboard_pinctrl_ops = {
+	.get_groups_count = upboard_get_groups_count,
+	.get_group_name = upboard_get_group_name,
+};
+
+static struct pinctrl_desc upboard_up_pinctrl_desc = {
+	.pins = upboard_up_pins,
+	.npins = ARRAY_SIZE(upboard_up_pins),
+	.pctlops = &upboard_pinctrl_ops,
+	.pmxops = &upboard_pinmux_ops,
+	.owner = THIS_MODULE,
+};
+
+static struct pinctrl_desc upboard_up2_pinctrl_desc = {
+	.pins = upboard_up2_pins,
+	.npins = ARRAY_SIZE(upboard_up2_pins),
+	.pctlops = &upboard_pinctrl_ops,
+	.pmxops = &upboard_pinmux_ops,
+	.owner = THIS_MODULE,
+};
+
+static struct pinctrl_desc upboard_upcore_crex_pinctrl_desc = {
+	.pins = upboard_upcore_crex_pins,
+	.npins = ARRAY_SIZE(upboard_upcore_crex_pins),
+	.pctlops = &upboard_pinctrl_ops,
+	.pmxops = &upboard_pinmux_ops,
+	.owner = THIS_MODULE,
+};
+
+static struct pinctrl_desc upboard_upcore_crst02_pinctrl_desc = {
+	.pins = upboard_upcore_crst02_pins,
+	.npins = ARRAY_SIZE(upboard_upcore_crst02_pins),
+	.pctlops = &upboard_pinctrl_ops,
+	.pmxops = &upboard_pinmux_ops,
+	.owner = THIS_MODULE,
+};
+
+
+static int upboard_rpi_to_native_gpio(struct gpio_chip *gc, unsigned int gpio)
+{
+	struct upboard_pinctrl *pctrl =
+		container_of(gc, struct upboard_pinctrl, chip);
+	unsigned int pin = pctrl->rpi_mapping[gpio];
+	struct pinctrl_gpio_range *range =
+		pinctrl_find_gpio_range_from_pin(pctrl->pctldev, pin);
+
+	if (!range)
+		return -ENODEV;
+
+	return range->base;
+}
+
+static int upboard_gpio_request(struct gpio_chip *gc, unsigned int offset)
+{
+	int gpio = upboard_rpi_to_native_gpio(gc, offset);
+
+	if (gpio < 0)
+		return gpio;
+
+	return gpio_request(gpio, module_name(THIS_MODULE));
+}
+
+static void upboard_gpio_free(struct gpio_chip *gc, unsigned int offset)
+{
+	int gpio = upboard_rpi_to_native_gpio(gc, offset);
+
+	if (gpio < 0)
+		return;
+
+	gpio_free(gpio);
+}
+
+static int upboard_gpio_get(struct gpio_chip *gc, unsigned int offset)
+{
+	int gpio = upboard_rpi_to_native_gpio(gc, offset);
+
+	if (gpio < 0)
+		return gpio;
+
+	return gpio_get_value(gpio);
+}
+
+static void upboard_gpio_set(struct gpio_chip *gc, unsigned int offset, int
+			     value)
+{
+	int gpio = upboard_rpi_to_native_gpio(gc, offset);
+
+	if (gpio < 0)
+		return;
+
+	gpio_set_value(gpio, value);
+}
+
+static int upboard_gpio_direction_input(struct gpio_chip *gc,
+					unsigned int offset)
+{
+	int gpio = upboard_rpi_to_native_gpio(gc, offset);
+
+	if (gpio < 0)
+		return gpio;
+
+	return gpio_direction_input(gpio);
+}
+
+static int upboard_gpio_direction_output(struct gpio_chip *gc,
+					 unsigned int offset, int value)
+{
+	int gpio = upboard_rpi_to_native_gpio(gc, offset);
+
+	if (gpio < 0)
+		return gpio;
+
+	return gpio_direction_output(gpio, value);
+}
+
+static struct gpio_chip upboard_gpio_chip = {
+	.label = "Raspberry Pi compatible UP GPIO",
+	.base = 0,
+	.request = upboard_gpio_request,
+	.free = upboard_gpio_free,
+	.get = upboard_gpio_get,
+	.set = upboard_gpio_set,
+	.direction_input = upboard_gpio_direction_input,
+	.direction_output = upboard_gpio_direction_output,
+	.owner = THIS_MODULE,
+};
+
+/* DMI Matches for older bios without fpga initialization */
+static const struct dmi_system_id upboard_dmi_table[] __initconst = {
+	{
+		.matches = { /* UP */
+			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "AAEON"),
+			DMI_EXACT_MATCH(DMI_BOARD_NAME, "UP-CHT01"),
+			DMI_EXACT_MATCH(DMI_BOARD_VERSION, "V0.4"),
+		},
+		.driver_data = (void *)&upboard_up_bios_info_dvt,
+	},
+	{
+		.matches = { /* UP2 */
+			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "AAEON"),
+			DMI_EXACT_MATCH(DMI_BOARD_NAME, "UP-APL01"),
+			DMI_EXACT_MATCH(DMI_BOARD_VERSION, "V0.3"),
+		},
+		.driver_data = (void *)&upboard_up2_bios_info_v0_3,
+	},
+	{ },
+};
+
+static int __init upboard_pinctrl_probe(struct platform_device *pdev)
+{
+	struct upboard_fpga * const fpga = dev_get_drvdata(pdev->dev.parent);
+	struct acpi_device * const adev = ACPI_COMPANION(&pdev->dev);
+	struct pinctrl_desc *pctldesc;
+	const struct upboard_bios *bios_info = NULL;
+	struct upboard_pinctrl *pctrl;
+	struct upboard_pin *pins;
+	const struct dmi_system_id *system_id;
+	const char *hid;
+	const unsigned int *rpi_mapping;
+	unsigned int ngpio;
+	int ret;
+	int i;
+
+	if (!fpga)
+		return -EINVAL;
+
+	if (!adev)
+		return -ENODEV;
+
+	hid = acpi_device_hid(adev);
+	if (!strcmp(hid, "AANT0F00") || !strcmp(hid, "AANT0F04")) {
+		pctldesc = &upboard_up_pinctrl_desc;
+		rpi_mapping = upboard_up_rpi_mapping;
+		ngpio  = ARRAY_SIZE(upboard_up_rpi_mapping);
+	} else if (!strcmp(hid, "AANT0F01")) {
+		pctldesc = &upboard_up2_pinctrl_desc;
+		rpi_mapping = upboard_up2_rpi_mapping;
+		ngpio  = ARRAY_SIZE(upboard_up2_rpi_mapping);
+	} else if (!strcmp(hid, "AANT0F02")) {
+		pctldesc = &upboard_upcore_crex_pinctrl_desc;
+		rpi_mapping = upboard_upcore_crex_rpi_mapping;
+		ngpio  = ARRAY_SIZE(upboard_upcore_crex_rpi_mapping);
+		bios_info = &upboard_upcore_crex_bios_info;
+	} else if (!strcmp(hid, "AANT0F03")) {
+		pctldesc = &upboard_upcore_crst02_pinctrl_desc;
+		rpi_mapping = upboard_upcore_crst02_rpi_mapping;
+		ngpio  = ARRAY_SIZE(upboard_upcore_crst02_rpi_mapping);
+		bios_info = &upboard_upcore_crst02_bios_info;
+	} else
+		return -ENODEV;
+
+	pctldesc->name = dev_name(&pdev->dev);
+
+	pins = devm_kzalloc(&pdev->dev,
+			    sizeof(*pins) * pctldesc->npins,
+			    GFP_KERNEL);
+	if (!pins)
+		return -ENOMEM;
+
+	/* initialise pins */
+	for (i = 0; i < pctldesc->npins; i++) {
+		struct upboard_pin *pin = &pins[i];
+		struct pinctrl_pin_desc *pd = (struct pinctrl_pin_desc *)
+			&pctldesc->pins[i];
+		struct reg_field fldconf = {0};
+		unsigned int regoff = (pd->number / UPFPGA_REGISTER_SIZE);
+		unsigned int lsb = pd->number % UPFPGA_REGISTER_SIZE;
+
+		pin->funcbit = NULL;
+		if (pd->drv_data) {
+			fldconf = *(struct reg_field *)pd->drv_data;
+			if (!regmap_writeable(fpga->regmap, fldconf.reg))
+				return -EINVAL;
+
+			pin->funcbit = devm_regmap_field_alloc(&pdev->dev,
+							       fpga->regmap,
+							       fldconf);
+			if (IS_ERR(pin->funcbit))
+				return PTR_ERR(pin->funcbit);
+		}
+
+		pin->enbit = NULL;
+		fldconf.reg = UPFPGA_REG_GPIO_EN0 + regoff;
+		fldconf.lsb = lsb;
+		fldconf.msb = lsb;
+
+		/* some platform don't have enable bit, ignore if not present */
+		if (regmap_writeable(fpga->regmap, fldconf.reg)) {
+			pin->enbit = devm_regmap_field_alloc(&pdev->dev,
+							     fpga->regmap,
+							     fldconf);
+			if (IS_ERR(pin->enbit))
+				return PTR_ERR(pin->enbit);
+		}
+
+		fldconf.reg = UPFPGA_REG_GPIO_DIR0 + regoff;
+		fldconf.lsb = lsb;
+		fldconf.msb = lsb;
+
+		if (!regmap_writeable(fpga->regmap, fldconf.reg))
+			return -EINVAL;
+
+		pin->dirbit = devm_regmap_field_alloc(&pdev->dev,
+						      fpga->regmap,
+						      fldconf);
+		if (IS_ERR(pin->dirbit))
+			return PTR_ERR(pin->dirbit);
+
+		pd->drv_data = pin;
+	}
+
+	/* create a new pinctrl device and register it */
+	pctrl = devm_kzalloc(&pdev->dev, sizeof(*pctrl), GFP_KERNEL);
+	if (!pctrl)
+		return -ENOMEM;
+
+	pctrl->regmap = fpga->regmap;
+	pctrl->rpi_mapping = rpi_mapping;
+	pctrl->chip = upboard_gpio_chip;
+	pctrl->chip.parent = &pdev->dev;
+	pctrl->chip.ngpio = ngpio;
+
+	ret = devm_gpiochip_add_data(&pdev->dev, &pctrl->chip, &pctrl->chip);
+	if (ret)
+		return ret;
+
+	pctrl->pctldev = devm_pinctrl_register(&pdev->dev, pctldesc, pctrl);
+	if (IS_ERR(pctrl->pctldev))
+		return PTR_ERR(pctrl->pctldev);
+
+	/* add acpi pin mapping according to external-gpios key */
+	ret = acpi_node_add_pin_mapping(acpi_fwnode_handle(adev),
+					"external-gpios",
+					dev_name(&pdev->dev),
+					0, UINT_MAX);
+	if (ret)
+		return ret;
+
+	if (!bios_info) {
+		/* check for special board versions that require register patches */
+		system_id = dmi_first_match(upboard_dmi_table);
+		if (system_id)
+			bios_info = system_id->driver_data;
+	}
+
+	if (bios_info && bios_info->patches) {
+		ret = regmap_register_patch(pctrl->regmap,
+					    bios_info->patches,
+					    bios_info->npatches);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static struct platform_driver upboard_pinctrl_driver = {
+	.driver = {
+		.name = "upboard-pinctrl",
+	},
+};
+
+module_platform_driver_probe(upboard_pinctrl_driver, upboard_pinctrl_probe);
+
+MODULE_AUTHOR("Javier Arteaga <javier@emutex.com>");
+MODULE_AUTHOR("Dan O'Donovan <dan@emutex.com>");
+MODULE_AUTHOR("Nicola Lunghi <nicolal@emutex.com>");
+MODULE_DESCRIPTION("UP Board HAT pin controller driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:upboard-pinctrl");
-- 
2.17.1

Re: [PATCH 2/5] regmap: Expose regmap_writeable function to check if a register is writable

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 213 bytes --]

On Wed, Oct 19, 2022 at 10:24:47AM +0800, chengwei wrote:

> Expose the regmap_writeable function for pinctrl-upboard reference.

Why?  A driver should understand the register map of the device it is
controlling.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/5] ACPI: acpi_node_add_pin_mapping added to header file

[Andy Shevchenko]

On Wed, Oct 19, 2022 at 10:24:48AM +0800, chengwei wrote:
> Declare acpi_node_add_pin_mapping added for pinctrl-upboard
> reference.

This should be a part of the next change.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 4/5] GPIO ACPI: Add support to map GPIO resources to ranges

[Andy Shevchenko]

On Wed, Oct 19, 2022 at 10:24:49AM +0800, chengwei wrote:
> Add a function to gpiolib to facilitate registering a pin controller for
> a range of GPIO pins, but using ACPI resource references and without
> claiming the GPIO resource.

This is quite under explained.

First of all, why do you need all these?

Second, where is the link to ACPI DSDT excerpt of the device node
which needs that?

Third, is the BIOS for these platforms is already in wild or
can be amended?

...

> +		count = acpi_gpio_count_from_property(adev, propname);
>  		if (count > 0)
>  			break;

This part can be split to a separate change as a prerequisite.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 5/5] pinctrl: Add support pin control for UP board CPLD/FPGA

[Andy Shevchenko]

On Wed, Oct 19, 2022 at 10:24:50AM +0800, chengwei wrote:
> The UP Squared board <http://www.upboard.com> implements certain
> features (pin control) through an on-board FPGA.

...

> +config PINCTRL_UPBOARD
> +	tristate "UP board FPGA pin controller"
> +	depends on ACPI
> +	depends on MFD_UPBOARD_FPGA
> +	depends on X86

No compile test coverage?
I think you wanted something like

	depends on (X86 && ACPI) || COMPILE_TEST

I'm not even sure, why you have ACPI dependency. I do not see right now it has
a compile one.

> +	select GENERIC_PINCONF
> +	select PINMUX
> +	select PINCONF

...

> +#include <linux/acpi.h>

See above.

> +#include <linux/dmi.h>

> +#include <linux/gpio.h>

No way, no new code should ever use this.

> +#include <linux/gpio/consumer.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/kernel.h>
> +#include <linux/mfd/upboard-fpga.h>
> +#include <linux/module.h>
> +#include <linux/pinctrl/pinctrl.h>
> +#include <linux/pinctrl/pinmux.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/string.h>

...

> +static int upboard_gpio_request_enable(struct pinctrl_dev *pctldev,
> +				       struct pinctrl_gpio_range *range,
> +				       unsigned int pin)
> +{
> +	const struct pin_desc * const pd = pin_desc_get(pctldev, pin);
> +	const struct upboard_pin *p;
> +	int ret;

> +	if (!pd)
> +		return -EINVAL;

Why do you need this check?
Ditto for all the same checks over the code.

> +	p = pd->drv_data;
> +
> +	if (p->funcbit) {
> +		ret = regmap_field_write(p->funcbit, 0);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (p->enbit) {
> +		ret = regmap_field_write(p->enbit, 1);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +};

...

> +
> +

One blank line is enough. Please, check all your code for this.

...

> +static int upboard_rpi_to_native_gpio(struct gpio_chip *gc, unsigned int gpio)
> +{
> +	struct upboard_pinctrl *pctrl =
> +		container_of(gc, struct upboard_pinctrl, chip);
> +	unsigned int pin = pctrl->rpi_mapping[gpio];

> +	struct pinctrl_gpio_range *range =
> +		pinctrl_find_gpio_range_from_pin(pctrl->pctldev, pin);

Instead, split the assignment and the definition. Same amount of LoCs, but
reads and maintained better.

> +	if (!range)
> +		return -ENODEV;
> +
> +	return range->base;
> +}

...

> +static int upboard_gpio_request(struct gpio_chip *gc, unsigned int offset)
> +{
> +	int gpio = upboard_rpi_to_native_gpio(gc, offset);
> +
> +	if (gpio < 0)
> +		return gpio;
> +
> +	return gpio_request(gpio, module_name(THIS_MODULE));

Nope, new code mustn't use this APIs.

> +}

...

> +	gpio_free(gpio);

Ditto.

> +	return gpio_get_value(gpio);

Ditto.

> +	gpio_set_value(gpio, value);

Ditto.

> +	return gpio_direction_input(gpio);

Ditto.

> +	return gpio_direction_output(gpio, value);

Ditto.

...

> +	{
> +		.matches = { /* UP2 */
> +			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "AAEON"),
> +			DMI_EXACT_MATCH(DMI_BOARD_NAME, "UP-APL01"),
> +			DMI_EXACT_MATCH(DMI_BOARD_VERSION, "V0.3"),
> +		},
> +		.driver_data = (void *)&upboard_up2_bios_info_v0_3,
> +	},
> +	{ },

No comma for the terminator entry.

...

> +	hid = acpi_device_hid(adev);
> +	if (!strcmp(hid, "AANT0F00") || !strcmp(hid, "AANT0F04")) {
> +		pctldesc = &upboard_up_pinctrl_desc;
> +		rpi_mapping = upboard_up_rpi_mapping;
> +		ngpio  = ARRAY_SIZE(upboard_up_rpi_mapping);
> +	} else if (!strcmp(hid, "AANT0F01")) {
> +		pctldesc = &upboard_up2_pinctrl_desc;
> +		rpi_mapping = upboard_up2_rpi_mapping;
> +		ngpio  = ARRAY_SIZE(upboard_up2_rpi_mapping);
> +	} else if (!strcmp(hid, "AANT0F02")) {
> +		pctldesc = &upboard_upcore_crex_pinctrl_desc;
> +		rpi_mapping = upboard_upcore_crex_rpi_mapping;
> +		ngpio  = ARRAY_SIZE(upboard_upcore_crex_rpi_mapping);
> +		bios_info = &upboard_upcore_crex_bios_info;
> +	} else if (!strcmp(hid, "AANT0F03")) {
> +		pctldesc = &upboard_upcore_crst02_pinctrl_desc;
> +		rpi_mapping = upboard_upcore_crst02_rpi_mapping;
> +		ngpio  = ARRAY_SIZE(upboard_upcore_crst02_rpi_mapping);
> +		bios_info = &upboard_upcore_crst02_bios_info;
> +	} else
> +		return -ENODEV;

NIH device_get_match_data().

...

> +	ret = acpi_node_add_pin_mapping(acpi_fwnode_handle(adev),
> +					"external-gpios",
> +					dev_name(&pdev->dev),
> +					0, UINT_MAX);
> +	if (ret)
> +		return ret;

This is something strange. Can you point out to the DSDT, etc.?


...

> +

Blank line is not needed.

> +module_platform_driver_probe(upboard_pinctrl_driver, upboard_pinctrl_probe);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 5/5] pinctrl: Add support pin control for UP board CPLD/FPGA

[Linus Walleij]

Hi Chengwei,

thanks for your patch!

On Wed, Oct 19, 2022 at 4:26 AM chengwei <foxfly.lai.tw@gmail.com> wrote:

> The UP Squared board <http://www.upboard.com> implements certain
> features (pin control) through an on-board FPGA.
>
> Signed-off-by: Javier Arteaga <javier@emutex.com>
> [merge various fixes]
> Signed-off-by: Nicola Lunghi <nicola.lunghi@emutex.com>
> Signed-off-by: chengwei <larry.lai@yunjingtech.com>

I am a bit confused by this driver. Andy pointed out some obvious nits that
need to be fixed but the overall architecture here is also a bit puzzling.

This seems to want to be compatible to Raspberry Pi (RPi), then which one?

The driver seems to translate GPIO calls to "native GPIO" in some cases,
which GPIO controller is that?

Also I don't see why, normally a pin control
driver is an agnostic back-end for a GPIO controller, so the GPIO driver
should be the same (whatever "native") means, and this driver should
not even implement a gpio chip, just let the GPIO driver do its job
and call back into the pin control back-end whenever it needs it.

Also we already have a driver that collects existing GPIOs to a new
GPIO chip, the GPIO aggregator:
drivers/gpio/gpio-aggregator.c

Maybe if you can explain a bit about how this hardware works and why
you have to do indirect calls to another GPIO controller, things will
be easier to understand?

Yours,
Linus Walleij

Re: [PATCH 5/5] pinctrl: Add support pin control for UP board CPLD/FPGA

[Andy Shevchenko]

On Fri, Oct 21, 2022 at 11:09:27AM +0200, Linus Walleij wrote:
> On Wed, Oct 19, 2022 at 4:26 AM chengwei <foxfly.lai.tw@gmail.com> wrote:

> > The UP Squared board <http://www.upboard.com> implements certain
> > features (pin control) through an on-board FPGA.

> I am a bit confused by this driver. Andy pointed out some obvious nits that
> need to be fixed but the overall architecture here is also a bit puzzling.
> 
> This seems to want to be compatible to Raspberry Pi (RPi), then which one?
> 
> The driver seems to translate GPIO calls to "native GPIO" in some cases,
> which GPIO controller is that?

There is an SoC level GPIO (Apollo Lake I believe) and there is a discrete
component between it and user visible header (connector). This driver AFAIU
is about controlling that discrete component.

> Also I don't see why, normally a pin control
> driver is an agnostic back-end for a GPIO controller, so the GPIO driver
> should be the same (whatever "native") means, and this driver should
> not even implement a gpio chip, just let the GPIO driver do its job
> and call back into the pin control back-end whenever it needs it.
> 
> Also we already have a driver that collects existing GPIOs to a new
> GPIO chip, the GPIO aggregator:
> drivers/gpio/gpio-aggregator.c
> 
> Maybe if you can explain a bit about how this hardware works and why
> you have to do indirect calls to another GPIO controller, things will
> be easier to understand?

-- 
With Best Regards,
Andy Shevchenko

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

[Lee Jones]

On Wed, 19 Oct 2022, chengwei wrote:

> The UP Squared board <http://www.upboard.com> implements certain
> features (pin control, onboard LEDs or CEC) through an on-board CPLD/FPGA.
> 
> This mfd driver implements the line protocol to read and write registers
> from the FPGA through regmap. The register address map is also included.
> 
> The UP Boards provide a few I/O pin headers (for both GPIO and
> functions), including a 40-pin Raspberry Pi compatible header.
> 
> This patch implements support for the FPGA-based pin controller that
> manages direction and enable state for those header pins.
> 
> Partial support UP boards:
> * UP core + CREX
> * UP core + CRST02
> 
> Signed-off-by: Javier Arteaga <javier@emutex.com>
> [merge various fixes]
> Signed-off-by: Nicola Lunghi <nicola.lunghi@emutex.com>
> Signed-off-by: chengwei <larry.lai@yunjingtech.com>

Why does your name not match your email?

> ---

What happened to all of the other versions?

And the change-log?

>  drivers/mfd/Kconfig              |  12 +
>  drivers/mfd/Makefile             |   1 +
>  drivers/mfd/upboard-fpga.c       | 482 +++++++++++++++++++++++++++++++
>  include/linux/mfd/upboard-fpga.h |  49 ++++
>  4 files changed, 544 insertions(+)
>  create mode 100644 drivers/mfd/upboard-fpga.c
>  create mode 100644 include/linux/mfd/upboard-fpga.h
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index abb58ab1a1a4..c1d72a70e5f2 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -2104,6 +2104,18 @@ config MFD_QCOM_PM8008
>  	  under it in the device tree. Additional drivers must be enabled in
>  	  order to use the functionality of the device.
>  
> +config MFD_UPBOARD_FPGA
> +	tristate "Support for the UP board FPGA"

"Intel"?

> +	select MFD_CORE
> +	depends on X86 && ACPI
> +	help
> +	  Select this option to enable the Intel AAEON UP and UP^2 on-board FPGA.
> +	  The UP board implements certain features (pin control, onboard LEDs or
> +	  CEC) through an on-board FPGA.
> +
> +	  To compile this driver as a module, choose M here: the module will be
> +	  called upboard-fpga.
> +
>  menu "Multimedia Capabilities Port drivers"
>  	depends on ARCH_SA1100
>  
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 858cacf659d6..d9d10e3664f7 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -250,6 +250,7 @@ obj-$(CONFIG_MFD_ALTERA_A10SR)	+= altera-a10sr.o
>  obj-$(CONFIG_MFD_ALTERA_SYSMGR) += altera-sysmgr.o
>  obj-$(CONFIG_MFD_STPMIC1)	+= stpmic1.o
>  obj-$(CONFIG_MFD_SUN4I_GPADC)	+= sun4i-gpadc.o
> +obj-$(CONFIG_MFD_UPBOARD_FPGA)	+= upboard-fpga.o
>  
>  obj-$(CONFIG_MFD_STM32_LPTIMER)	+= stm32-lptimer.o
>  obj-$(CONFIG_MFD_STM32_TIMERS) 	+= stm32-timers.o
> diff --git a/drivers/mfd/upboard-fpga.c b/drivers/mfd/upboard-fpga.c
> new file mode 100644
> index 000000000000..6f73c2fa6f4a
> --- /dev/null
> +++ b/drivers/mfd/upboard-fpga.c
> @@ -0,0 +1,482 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * UP Board main platform driver and FPGA configuration support

"Intel AAEON UP and UP^2"

> + * Copyright (c) 2017, Emutex Ltd. All rights reserved.

No changes since 2017?

> + * Author: Javier Arteaga <javier@emutex.com>
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/dmi.h>
> +#include <linux/gpio.h>
> +#include <linux/kernel.h>
> +#include <linux/leds.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/upboard-fpga.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +static int upboard_fpga_read(void *, unsigned int, unsigned int *);
> +static int upboard_fpga_write(void *, unsigned int, unsigned int);

No prototypes please.

Reorder the file instead.

> +struct upboard_fpga_data {
> +	const struct regmap_config *regmapconf;

> +	const struct mfd_cell *cells;
> +	size_t ncells;

This is unlikely to be acceptable.

Please reference the tables where you use them, instead of passing
MFD initialisation pointers through the ACPI API.

> +};
> +
> +#define UPBOARD_LED_CELL(led_data, n)                   \
> +	{                                               \
> +		.name = "upboard-led",                  \
> +		.id = (n),                              \
> +		.platform_data = &led_data[(n)],        \
> +		.pdata_size = sizeof(*(led_data)),      \
> +	}

We already have defines like this.  Please use them instead.

> +/* UP board */
> +
> +static const struct regmap_range upboard_up_readable_ranges[] = {
> +	regmap_reg_range(UPFPGA_REG_PLATFORM_ID, UPFPGA_REG_FIRMWARE_ID),
> +	regmap_reg_range(UPFPGA_REG_FUNC_EN0, UPFPGA_REG_FUNC_EN0),
> +	regmap_reg_range(UPFPGA_REG_GPIO_DIR0, UPFPGA_REG_GPIO_DIR1),
> +};
> +
> +static const struct regmap_range upboard_up_writable_ranges[] = {
> +	regmap_reg_range(UPFPGA_REG_FUNC_EN0, UPFPGA_REG_FUNC_EN0),
> +	regmap_reg_range(UPFPGA_REG_GPIO_DIR0, UPFPGA_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 = UPFPGA_ADDRESS_SIZE,
> +	.val_bits = UPFPGA_REGISTER_SIZE,
> +	.max_register = UPFPGA_REG_MAX,
> +	.reg_read = upboard_fpga_read,
> +	.reg_write = upboard_fpga_write,
> +	.fast_io = false,
> +	.cache_type = REGCACHE_RBTREE,
> +	.rd_table = &upboard_up_readable_table,
> +	.wr_table = &upboard_up_writable_table,
> +};
> +
> +static struct upboard_led_data upboard_up_led_data[] = {
> +	{ .bit = 0, .colour = "yellow" },
> +	{ .bit = 1, .colour = "green" },
> +	{ .bit = 2, .colour = "red" },
> +};

Not a big deal, but can't this info be derived from the ACPI tables?

> +static const struct mfd_cell upboard_up_mfd_cells[] = {
> +	{ .name = "upboard-pinctrl" },
> +	UPBOARD_LED_CELL(upboard_up_led_data, 0),
> +	UPBOARD_LED_CELL(upboard_up_led_data, 1),
> +	UPBOARD_LED_CELL(upboard_up_led_data, 2),
> +};
> +
> +static const struct upboard_fpga_data upboard_up_fpga_data = {
> +	.regmapconf = &upboard_up_regmap_config,
> +	.cells = upboard_up_mfd_cells,
> +	.ncells = ARRAY_SIZE(upboard_up_mfd_cells),
> +};
> +
> +/* UP^2 board */
> +
> +static const struct regmap_range upboard_up2_readable_ranges[] = {
> +	regmap_reg_range(UPFPGA_REG_PLATFORM_ID, UPFPGA_REG_FIRMWARE_ID),
> +	regmap_reg_range(UPFPGA_REG_FUNC_EN0, UPFPGA_REG_FUNC_EN1),
> +	regmap_reg_range(UPFPGA_REG_GPIO_EN0, UPFPGA_REG_GPIO_EN2),
> +	regmap_reg_range(UPFPGA_REG_GPIO_DIR0, UPFPGA_REG_GPIO_DIR2),
> +};
> +
> +static const struct regmap_range upboard_up2_writable_ranges[] = {
> +	regmap_reg_range(UPFPGA_REG_FUNC_EN0, UPFPGA_REG_FUNC_EN1),
> +	regmap_reg_range(UPFPGA_REG_GPIO_EN0, UPFPGA_REG_GPIO_EN2),
> +	regmap_reg_range(UPFPGA_REG_GPIO_DIR0, UPFPGA_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 = UPFPGA_ADDRESS_SIZE,
> +	.val_bits = UPFPGA_REGISTER_SIZE,
> +	.max_register = UPFPGA_REG_MAX,
> +	.reg_read = upboard_fpga_read,
> +	.reg_write = upboard_fpga_write,
> +	.fast_io = false,
> +	.cache_type = REGCACHE_RBTREE,
> +	.rd_table = &upboard_up2_readable_table,
> +	.wr_table = &upboard_up2_writable_table,
> +};
> +
> +static struct upboard_led_data upboard_up2_led_data[] = {
> +	{ .bit = 0, .colour = "blue" },
> +	{ .bit = 1, .colour = "yellow" },
> +	{ .bit = 2, .colour = "green" },
> +	{ .bit = 3, .colour = "red" },
> +};
> +
> +static const struct mfd_cell upboard_up2_mfd_cells[] = {
> +	{ .name = "upboard-pinctrl" },
> +	UPBOARD_LED_CELL(upboard_up2_led_data, 0),
> +	UPBOARD_LED_CELL(upboard_up2_led_data, 1),
> +	UPBOARD_LED_CELL(upboard_up2_led_data, 2),
> +	UPBOARD_LED_CELL(upboard_up2_led_data, 3),
> +};
> +
> +static const struct upboard_fpga_data upboard_up2_fpga_data = {
> +	.regmapconf = &upboard_up2_regmap_config,
> +	.cells = upboard_up2_mfd_cells,
> +	.ncells = ARRAY_SIZE(upboard_up2_mfd_cells),
> +};
> +
> +/* UP-CREX carrier board for UP Core */
> +
> +/* same MAXV config as UP1 (proto2 release) */
> +#define upboard_upcore_crex_fpga_data upboard_up_fpga_data
> +
> +/* UP-CRST02 carrier board for UP Core */
> +
> +/* same MAX10 config as UP2, but same LED cells as UP1 */
> +static const struct upboard_fpga_data upboard_upcore_crst02_fpga_data = {
> +	.regmapconf = &upboard_up2_regmap_config,
> +	.cells = upboard_up_mfd_cells,
> +	.ncells = ARRAY_SIZE(upboard_up_mfd_cells),
> +};
> +
> +static int upboard_fpga_read(void *context, unsigned int reg, unsigned int *val)
> +{
> +	struct upboard_fpga * const fpga = context;
> +	int i;
> +
> +	gpiod_set_value(fpga->clear_gpio, 0);
> +	gpiod_set_value(fpga->clear_gpio, 1);
> +
> +	reg |= UPFPGA_READ_FLAG;
> +
> +	for (i = UPFPGA_ADDRESS_SIZE; i >= 0; i--) {
> +		gpiod_set_value(fpga->strobe_gpio, 0);
> +		gpiod_set_value(fpga->datain_gpio, (reg >> i) & 0x1);
> +		gpiod_set_value(fpga->strobe_gpio, 1);
> +	}
> +
> +	gpiod_set_value(fpga->strobe_gpio, 0);
> +	*val = 0;
> +
> +	for (i = UPFPGA_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);

A little bit of commentary in here would help us understand how this
works.

> +	return 0;
> +};
> +
> +static int upboard_fpga_write(void *context, unsigned int reg, unsigned int val)
> +{
> +	struct upboard_fpga * const fpga = context;
> +	int i;
> +
> +	gpiod_set_value(fpga->clear_gpio, 0);
> +	gpiod_set_value(fpga->clear_gpio, 1);
> +
> +	for (i = UPFPGA_ADDRESS_SIZE; i >= 0; i--) {
> +		gpiod_set_value(fpga->strobe_gpio, 0);
> +		gpiod_set_value(fpga->datain_gpio, (reg >> i) & 0x1);
> +		gpiod_set_value(fpga->strobe_gpio, 1);
> +	}
> +
> +	gpiod_set_value(fpga->strobe_gpio, 0);
> +
> +	for (i = UPFPGA_REGISTER_SIZE - 1; i >= 0; i--) {
> +		gpiod_set_value(fpga->datain_gpio, (val >> i) & 0x1);
> +		gpiod_set_value(fpga->strobe_gpio, 1);
> +		gpiod_set_value(fpga->strobe_gpio, 0);
> +	}
> +
> +	gpiod_set_value(fpga->strobe_gpio, 1);

As above.

> +	return 0;
> +};
> +
> +static int __init upboard_fpga_gpio_init(struct upboard_fpga *fpga)
> +{
> +	enum gpiod_flags flags;
> +
> +	flags = fpga->uninitialised ? GPIOD_OUT_LOW : GPIOD_ASIS;

Nit: '\n'

> +	fpga->enable_gpio = devm_gpiod_get(fpga->dev, "enable", flags);
> +	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);
> +
> +	/* The SoC pinctrl driver may not support reserving the GPIO line for
> +	 * FPGA reset without causing an undesired reset pulse. This will clear
> +	 * any settings on the FPGA, so only do it if we must.
> +	 */

Please use correctly formatted multi-line comments.

> +	if (fpga->uninitialised) {
> +		fpga->reset_gpio = devm_gpiod_get(fpga->dev, "reset",
> +						  GPIOD_OUT_LOW);
> +		if (IS_ERR(fpga->reset_gpio))
> +			return PTR_ERR(fpga->reset_gpio);
> +
> +		gpiod_set_value(fpga->reset_gpio, 1);
> +	}
> +
> +	gpiod_set_value(fpga->enable_gpio, 1);
> +	fpga->uninitialised = false;
> +
> +	return 0;
> +}
> +
> +static int __init upboard_fpga_detect_firmware(struct upboard_fpga *fpga)
> +{
> +	const unsigned int AAEON_MANUFACTURER_ID = 0x01;

Please define these.

Variables should be lower case.

> +	const unsigned int SUPPORTED_FW_MAJOR = 0x0;
> +	unsigned int platform_id, manufacturer_id;
> +	unsigned int firmware_id, build, major, minor, patch;
> +	int ret;
> +
> +	ret = regmap_read(fpga->regmap, UPFPGA_REG_PLATFORM_ID, &platform_id);
> +	if (ret)
> +		return ret;
> +
> +	manufacturer_id = platform_id & 0xff;

#define the 0xff - perhaps MANUFACTURER_MASK.

> +	if (manufacturer_id != AAEON_MANUFACTURER_ID) {
> +		dev_dbg(fpga->dev,
> +			"driver not compatible with custom FPGA FW from manufacturer id 0x%02x. Exiting",
> +			manufacturer_id);
> +		return -ENODEV;

If this is an error, your print messages should be dev_err().

> +	}
> +
> +	ret = regmap_read(fpga->regmap, UPFPGA_REG_FIRMWARE_ID, &firmware_id);
> +	if (ret)
> +		return ret;
> +
> +	build = (firmware_id >> 12) & 0xf;
> +	major = (firmware_id >> 8) & 0xf;
> +	minor = (firmware_id >> 4) & 0xf;
> +	patch = firmware_id & 0xf;

Nit: '\n'

> +	if (major != SUPPORTED_FW_MAJOR) {
> +		dev_dbg(fpga->dev, "unsupported FPGA FW v%u.%u.%u build 0x%02x",
> +			major, minor, patch, build);
> +		return -ENODEV;
> +	}
> +
> +	dev_info(fpga->dev, "compatible FPGA FW v%u.%u.%u build 0x%02x",
> +		 major, minor, patch, build);

Nit: '\n'

> +	return 0;
> +}
> +
> +static const struct acpi_device_id upboard_fpga_acpi_match[] = {
> +	{ "AANT0F00", (kernel_ulong_t)&upboard_up_fpga_data },
> +	{ "AANT0F01", (kernel_ulong_t)&upboard_up2_fpga_data },
> +	{ "AANT0F02", (kernel_ulong_t)&upboard_upcore_crex_fpga_data },
> +	{ "AANT0F03", (kernel_ulong_t)&upboard_upcore_crst02_fpga_data },
> +	{ "AANT0F04", (kernel_ulong_t)&upboard_up_fpga_data },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(acpi, upboard_fpga_acpi_match);

Please don't mix platform registration strategies.

Pass platform / hardware IDs instead and to the selecting in C.

> +#define UPFPGA_QUIRK_UNINITIALISED  BIT(0)
> +#define UPFPGA_QUIRK_HRV1_IS_PROTO2 BIT(1)
> +#define UPFPGA_QUIRK_GPIO_LED       BIT(2)
> +
> +static const struct dmi_system_id upboard_dmi_table[] __initconst = {
> +	{
> +		.matches = { /* UP */
> +			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "AAEON"),
> +			DMI_EXACT_MATCH(DMI_BOARD_NAME, "UP-CHT01"),
> +			DMI_EXACT_MATCH(DMI_BOARD_VERSION, "V0.4"),
> +		},
> +		.driver_data = (void *)UPFPGA_QUIRK_UNINITIALISED,
> +	},
> +	{
> +		.matches = { /* UP2 */
> +			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "AAEON"),
> +			DMI_EXACT_MATCH(DMI_BOARD_NAME, "UP-APL01"),
> +			DMI_EXACT_MATCH(DMI_BOARD_VERSION, "V0.3"),
> +		},
> +		.driver_data = (void *)(UPFPGA_QUIRK_UNINITIALISED |
> +			UPFPGA_QUIRK_HRV1_IS_PROTO2),
> +	},
> +	{
> +		.matches = { /* UP2 Pro*/
> +			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "AAEON"),
> +			DMI_EXACT_MATCH(DMI_BOARD_NAME, "UPN-APL01"),
> +			DMI_EXACT_MATCH(DMI_BOARD_VERSION, "V1.0"),
> +		},
> +		.driver_data = (void *)UPFPGA_QUIRK_HRV1_IS_PROTO2,
> +	},
> +	{
> +		.matches = { /* UP2 */
> +			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "AAEON"),
> +			DMI_EXACT_MATCH(DMI_BOARD_NAME, "UP-APL01"),
> +			DMI_EXACT_MATCH(DMI_BOARD_VERSION, "V0.4"),
> +		},
> +		.driver_data = (void *)UPFPGA_QUIRK_HRV1_IS_PROTO2,
> +	},
> +	{
> +		.matches = { /* UP Xtreme */
> +			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "AAEON"),
> +			DMI_EXACT_MATCH(DMI_BOARD_NAME, "UP-WHL01"),
> +			DMI_EXACT_MATCH(DMI_BOARD_VERSION, "V0.1"),
> +		},
> +	},
> +	{
> +		.matches = { /* UP APL03 */
> +			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "AAEON"),
> +			DMI_EXACT_MATCH(DMI_BOARD_NAME, "UP-APL03"),
> +			DMI_EXACT_MATCH(DMI_BOARD_VERSION, "V1.0"),
> +		},
> +		.driver_data = (void *)(UPFPGA_QUIRK_HRV1_IS_PROTO2 |
> +			UPFPGA_QUIRK_GPIO_LED),
> +	},
> +	{ },
> +};
> +
> +#define UPFPGA_PROTOCOL_V2_HRV 2

How does this differ from UPFPGA_QUIRK_HRV1_IS_PROTO2?

> +static int __init upboard_fpga_probe(struct platform_device *pdev)
> +{
> +	struct upboard_fpga *fpga;

This is more widely known as 'ddata'.

> +	const struct acpi_device_id *id;
> +	const struct upboard_fpga_data *fpga_data;
> +	const struct dmi_system_id *system_id;
> +	acpi_handle handle;
> +	acpi_status status;
> +	unsigned long long hrv;
> +	unsigned long quirks = 0;
> +	int ret;
> +
> +	id = acpi_match_device(upboard_fpga_acpi_match, &pdev->dev);
> +	if (!id)
> +		return -ENODEV;
> +
> +	handle = ACPI_HANDLE(&pdev->dev);
> +	status = acpi_evaluate_integer(handle, "_HRV", NULL, &hrv);
> +	if (ACPI_FAILURE(status)) {
> +		dev_err(&pdev->dev, "failed to get PCTL revision");
> +		return -ENODEV;
> +	}
> +
> +	system_id = dmi_first_match(upboard_dmi_table);
> +	if (system_id)
> +		quirks = (unsigned long)system_id->driver_data;
> +
> +	if (hrv == 1 && (quirks & UPFPGA_QUIRK_HRV1_IS_PROTO2))

What is HRV?  And what is 1?  Please define.

> +		hrv = UPFPGA_PROTOCOL_V2_HRV;
> +
> +	if (hrv != UPFPGA_PROTOCOL_V2_HRV) {
> +		dev_dbg(&pdev->dev, "unsupported PCTL revision: %llu", hrv);
> +		return -ENODEV;
> +	}
> +
> +	fpga_data = (const struct upboard_fpga_data *) id->driver_data;
> +
> +	fpga = devm_kzalloc(&pdev->dev, sizeof(*fpga), GFP_KERNEL);
> +	if (!fpga)
> +		return -ENOMEM;
> +
> +	if (quirks & UPFPGA_QUIRK_UNINITIALISED) {
> +		dev_info(&pdev->dev, "FPGA not initialised by this BIOS");
> +		fpga->uninitialised = true;
> +	}
> +
> +	dev_set_drvdata(&pdev->dev, fpga);
> +	fpga->dev = &pdev->dev;
> +	fpga->regmap = devm_regmap_init(&pdev->dev, NULL, fpga,
> +					fpga_data->regmapconf);
> +	if (IS_ERR(fpga->regmap))
> +		return PTR_ERR(fpga->regmap);
> +
> +	ret = upboard_fpga_gpio_init(fpga);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to init FPGA comm GPIOs: %d", ret);

Please expand 'init' and 'comm'.

> +		return ret;
> +	}
> +
> +	ret = upboard_fpga_detect_firmware(fpga);
> +	if (ret)
> +		return ret;
> +
> +	if (quirks & UPFPGA_QUIRK_GPIO_LED) {
> +#define APL_GPIO_218	507

Please define somewhere more central.

> +		static struct gpio_led upboard_gpio_leds[] = {
> +			{
> +				.name = "upboard:blue:",
> +				.gpio = APL_GPIO_218,
> +				.default_state = LEDS_GPIO_DEFSTATE_KEEP,
> +			},
> +		};
> +		static struct gpio_led_platform_data upboard_gpio_led_platform_data = {
> +			.num_leds = ARRAY_SIZE(upboard_gpio_leds),
> +			.leds = upboard_gpio_leds,
> +		};
> +		static const struct mfd_cell upboard_gpio_led_cells[] = {
> +			{
> +				.name = "leds-gpio",
> +				.id = 0,
> +				.platform_data = &upboard_gpio_led_platform_data,
> +				.pdata_size = sizeof(upboard_gpio_led_platform_data),
> +			},
> +		};

This is ugly and serves little purpose.

Place all of these structs with the others above.

> +		ret =  devm_mfd_add_devices(&pdev->dev, 0, upboard_gpio_led_cells,

Not '0'.  Please use the defines provided.

> +				    ARRAY_SIZE(upboard_gpio_led_cells), NULL, 0, NULL);
> +		if (ret) {
> +			dev_err(&pdev->dev, "Failed to add GPIO leds");
> +			return ret;
> +		}
> +

Superfluous line.

> +	}
> +
> +	return devm_mfd_add_devices(&pdev->dev, 0, fpga_data->cells,

As above.

> +				    fpga_data->ncells, NULL, 0, NULL);
> +}
> +
> +static struct platform_driver upboard_fpga_driver = {
> +	.driver = {
> +		.name = "upboard-fpga",
> +		.acpi_match_table = upboard_fpga_acpi_match,
> +	},
> +};
> +
> +module_platform_driver_probe(upboard_fpga_driver, upboard_fpga_probe);
> +
> +MODULE_AUTHOR("Javier Arteaga <javier@emutex.com>");
> +MODULE_DESCRIPTION("UP Board FPGA driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/mfd/upboard-fpga.h b/include/linux/mfd/upboard-fpga.h
> new file mode 100644
> index 000000000000..e0940120514d
> --- /dev/null
> +++ b/include/linux/mfd/upboard-fpga.h
> @@ -0,0 +1,49 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * UP Board FPGA MFD driver interface

Please remove the term MFD.

> + * Copyright (c) 2017, Emutex Ltd. All rights reserved.
> + *
> + * Author: Javier Arteaga <javier@emutex.com>
> + */
> +
> +#ifndef __LINUX_MFD_UPBOARD_FPGA_H
> +#define __LINUX_MFD_UPBOARD_FPGA_H
> +
> +#define UPFPGA_ADDRESS_SIZE  7
> +#define UPFPGA_REGISTER_SIZE 16
> +
> +#define UPFPGA_READ_FLAG     (1 << UPFPGA_ADDRESS_SIZE)
> +
> +enum upboard_fpgareg {
> +	UPFPGA_REG_PLATFORM_ID   = 0x10,
> +	UPFPGA_REG_FIRMWARE_ID   = 0x11,
> +	UPFPGA_REG_FUNC_EN0      = 0x20,
> +	UPFPGA_REG_FUNC_EN1      = 0x21,
> +	UPFPGA_REG_GPIO_EN0      = 0x30,
> +	UPFPGA_REG_GPIO_EN1      = 0x31,
> +	UPFPGA_REG_GPIO_EN2      = 0x32,
> +	UPFPGA_REG_GPIO_DIR0     = 0x40,
> +	UPFPGA_REG_GPIO_DIR1     = 0x41,
> +	UPFPGA_REG_GPIO_DIR2     = 0x42,
> +	UPFPGA_REG_MAX,
> +};
> +
> +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;
> +	bool uninitialised;
> +};
> +
> +struct upboard_led_data {
> +	unsigned int bit;
> +	const char *colour;
> +};
> +
> +#endif /*  __LINUX_MFD_UPBOARD_FPGA_H */

-- 
Lee Jones [李琼斯]

Re: 回覆: [PATCH 1/5] mfd: Add support for UP board CPLD/FPGA

[gregkh]

On Tue, Nov 08, 2022 at 03:45:45PM +0000, Larry Lai wrote:
> Dear Jones,
> 
>         Thank you for spending time to review this code, please check our response below your comment with yellow background.

html email will be rejected by the mailing lists.  Please resend in
plain-text format.

thanks,

greg k-h

Re: 回覆: [PATCH 1/5] mfd: Add support for UP board CPLD/FPGA

[gregkh]

On Tue, Nov 08, 2022 at 04:31:17PM +0000, Larry Lai wrote:
> Send again with Text only.

Sorry, was in HTML format :(

回覆: [PATCH 1/5] mfd: Add support for UP board CPLD/FPGA

[Larry Lai]

[-- Attachment #1: Type: text/plain, Size: 31758 bytes --]

Dear Gregkh, 

      Send this mail again with Text only, I am sorry for any inconvenience.

Larry


寄件者: Larry Lai <larry.lai@yunjingtech.com>
寄件日期: 2022年11月9日 上午 12:31
收件者: Lee Jones <lee@kernel.org>; chengwei <foxfly.lai.tw@gmail.com>
副本: broonie@kernel.org <broonie@kernel.org>; rafael@kernel.org <rafael@kernel.org>; mika.westerberg@linux.intel.com <mika.westerberg@linux.intel.com>; andriy.shevchenko@linux.intel.com <andriy.shevchenko@linux.intel.com>; linus.walleij@linaro.org <linus.walleij@linaro.org>; brgl@bgdev.pl <brgl@bgdev.pl>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>; gregkh@linuxfoundation.org <gregkh@linuxfoundation.org>; lenb@kernel.org <lenb@kernel.org>; linux-acpi@vger.kernel.org <linux-acpi@vger.kernel.org>; linux-gpio@vger.kernel.org <linux-gpio@vger.kernel.org>; GaryWang@aaeon.com.tw <GaryWang@aaeon.com.tw>; Musa Lin <musa.lin@yunjingtech.com>; Jack Chang <jack.chang@yunjingtech.com>; Javier Arteaga <javier@emutex.com>; Nicola Lunghi <nicola.lunghi@emutex.com>
主旨: 回覆: [PATCH 1/5] mfd: Add support for UP board CPLD/FPGA 
 
Send again with Text only.
 
Dear Jones,
        Thank you for spending time to review this code, please check our response below your comment with Larry (11/09).
Some of your comments we cannot understand clearly, please kindly explain it more or give us some examples.
Besides, we will send the Patch V2 later to modify most of the issues you mentioned before.
Ps. The excel file sheet 2 "ref(FPGA comm port)" is for FPGA read and write protocol usage. 
 
Larry
 
寄件者: Lee Jones <lee@kernel.org>
日期: 星期一, 2022年10月31日 下午10:58
收件者: chengwei <foxfly.lai.tw@gmail.com>
副本: broonie@kernel.org <broonie@kernel.org>, rafael@kernel.org <rafael@kernel.org>, mika.westerberg@linux.intel.com <mika.westerberg@linux.intel.com>, andriy.shevchenko@linux.intel.com <andriy.shevchenko@linux.intel.com>, linus.walleij@linaro.org <linus.walleij@linaro.org>, brgl@bgdev.pl <brgl@bgdev.pl>, linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>, gregkh@linuxfoundation.org <gregkh@linuxfoundation.org>, lenb@kernel.org <lenb@kernel.org>, linux-acpi@vger.kernel.org <linux-acpi@vger.kernel.org>, linux-gpio@vger.kernel.org <linux-gpio@vger.kernel.org>, GaryWang@aaeon.com.tw <GaryWang@aaeon.com.tw>, Musa Lin <musa.lin@yunjingtech.com>, Jack Chang <jack.chang@yunjingtech.com>, Larry Lai <larry.lai@yunjingtech.com>, Javier Arteaga <javier@emutex.com>, Nicola Lunghi <nicola.lunghi@emutex.com>
主旨: Re: [PATCH 1/5] mfd: Add support for UP board CPLD/FPGA
On Wed, 19 Oct 2022, chengwei wrote:

> The UP Squared board <http://www.upboard.com> implements certain
> features (pin control, onboard LEDs or CEC) through an on-board CPLD/FPGA.
> 
> This mfd driver implements the line protocol to read and write registers
> from the FPGA through regmap. The register address map is also included.
> 
> The UP Boards provide a few I/O pin headers (for both GPIO and
> functions), including a 40-pin Raspberry Pi compatible header.
> 
> This patch implements support for the FPGA-based pin controller that
> manages direction and enable state for those header pins.
> 
> Partial support UP boards:
> * UP core + CREX
> * UP core + CRST02
> 
> Signed-off-by: Javier Arteaga <javier@emutex.com>
> [merge various fixes]
> Signed-off-by: Nicola Lunghi <nicola.lunghi@emutex.com>
> Signed-off-by: chengwei <larry.lai@yunjingtech.com>

Why does your name not match your email?

> --- Larry (11/09) : Sorry to confuse you, we will fix it on next release

What happened to all of the other versions?
Larry (11/09) : the old other versions : fixed the Kernel robot auto build test ERROR on ARCH=arm64 or ARCH=mips.

And the change-log?

>  drivers/mfd/Kconfig              |  12 +
>  drivers/mfd/Makefile             |   1 +
>  drivers/mfd/upboard-fpga.c       | 482 +++++++++++++++++++++++++++++++
>  include/linux/mfd/upboard-fpga.h |  49 ++++
>  4 files changed, 544 insertions(+)
>  create mode 100644 drivers/mfd/upboard-fpga.c
>  create mode 100644 include/linux/mfd/upboard-fpga.h
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index abb58ab1a1a4..c1d72a70e5f2 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -2104,6 +2104,18 @@ config MFD_QCOM_PM8008
>          under it in the device tree. Additional drivers must be enabled in
>          order to use the functionality of the device.
>  
> +config MFD_UPBOARD_FPGA
> +     tristate "Support for the UP board FPGA"

"Intel"?
Larry (11/09) : the UP board series are Intel foundation kit , please browse the link 
( https://www.aaeon.com/en/c/aaeon-up-developer-boards ) to check the UP Developer Board series.

> +     select MFD_CORE
> +     depends on X86 && ACPI
> +     help
> +       Select this option to enable the Intel AAEON UP and UP^2 on-board FPGA.
> +       The UP board implements certain features (pin control, onboard LEDs or
> +       CEC) through an on-board FPGA.
> +
> +       To compile this driver as a module, choose M here: the module will be
> +       called upboard-fpga.
> +
>  menu "Multimedia Capabilities Port drivers"
>        depends on ARCH_SA1100
>  
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 858cacf659d6..d9d10e3664f7 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -250,6 +250,7 @@ obj-$(CONFIG_MFD_ALTERA_A10SR)    += altera-a10sr.o
>  obj-$(CONFIG_MFD_ALTERA_SYSMGR) += altera-sysmgr.o
>  obj-$(CONFIG_MFD_STPMIC1)    += stpmic1.o
>  obj-$(CONFIG_MFD_SUN4I_GPADC)        += sun4i-gpadc.o
> +obj-$(CONFIG_MFD_UPBOARD_FPGA)       += upboard-fpga.o
>  
>  obj-$(CONFIG_MFD_STM32_LPTIMER)      += stm32-lptimer.o
>  obj-$(CONFIG_MFD_STM32_TIMERS)        += stm32-timers.o
> diff --git a/drivers/mfd/upboard-fpga.c b/drivers/mfd/upboard-fpga.c
> new file mode 100644
> index 000000000000..6f73c2fa6f4a
> --- /dev/null
> +++ b/drivers/mfd/upboard-fpga.c
> @@ -0,0 +1,482 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * UP Board main platform driver and FPGA configuration support

"Intel AAEON UP and UP^2"

> + * Copyright (c) 2017, Emutex Ltd. All rights reserved.

No changes since 2017?
Larry (11/09) :  we will fix it on next release (PATCH V2)

> + * Author: Javier Arteaga <javier@emutex.com>
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/dmi.h>
> +#include <linux/gpio.h>
> +#include <linux/kernel.h>
> +#include <linux/leds.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/upboard-fpga.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +static int upboard_fpga_read(void *, unsigned int, unsigned int *);
> +static int upboard_fpga_write(void *, unsigned int, unsigned int);

No prototypes please.

Reorder the file instead.
Larry (11/09) :  we will fix it on next release (PATCH V2)

> +struct upboard_fpga_data {
> +     const struct regmap_config *regmapconf;

> +     const struct mfd_cell *cells;
> +     size_t ncells;

This is unlikely to be acceptable.

Please reference the tables where you use them, instead of passing
MFD initialisation pointers through the ACPI API.
Larry (11/09) :  we cannot understand it clearly. could you kindly explain it more?

> +};
> +
> +#define UPBOARD_LED_CELL(led_data, n)                   \
> +     {                                               \
> +             .name = "upboard-led",                  \
> +             .id = (n),                              \
> +             .platform_data = &led_data[(n)],        \
> +             .pdata_size = sizeof(*(led_data)),      \
> +     }

We already have defines like this.  Please use them instead.
Larry (11/09) :  we cannot understand it clearly. could you kindly explain it more?

> +/* UP board */
> +
> +static const struct regmap_range upboard_up_readable_ranges[] = {
> +     regmap_reg_range(UPFPGA_REG_PLATFORM_ID, UPFPGA_REG_FIRMWARE_ID),
> +     regmap_reg_range(UPFPGA_REG_FUNC_EN0, UPFPGA_REG_FUNC_EN0),
> +     regmap_reg_range(UPFPGA_REG_GPIO_DIR0, UPFPGA_REG_GPIO_DIR1),
> +};
> +
> +static const struct regmap_range upboard_up_writable_ranges[] = {
> +     regmap_reg_range(UPFPGA_REG_FUNC_EN0, UPFPGA_REG_FUNC_EN0),
> +     regmap_reg_range(UPFPGA_REG_GPIO_DIR0, UPFPGA_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 = UPFPGA_ADDRESS_SIZE,
> +     .val_bits = UPFPGA_REGISTER_SIZE,
> +     .max_register = UPFPGA_REG_MAX,
> +     .reg_read = upboard_fpga_read,
> +     .reg_write = upboard_fpga_write,
> +     .fast_io = false,
> +     .cache_type = REGCACHE_RBTREE,
> +     .rd_table = &upboard_up_readable_table,
> +     .wr_table = &upboard_up_writable_table,
> +};
> +
> +static struct upboard_led_data upboard_up_led_data[] = {
> +     { .bit = 0, .colour = "yellow" },
> +     { .bit = 1, .colour = "green" },
> +     { .bit = 2, .colour = "red" },
> +};

Not a big deal, but can't this info be derived from the ACPI tables?
Larry (11/09) :  we cannot understand it clearly. could you kindly explain it more?

> +static const struct mfd_cell upboard_up_mfd_cells[] = {
> +     { .name = "upboard-pinctrl" },
> +     UPBOARD_LED_CELL(upboard_up_led_data, 0),
> +     UPBOARD_LED_CELL(upboard_up_led_data, 1),
> +     UPBOARD_LED_CELL(upboard_up_led_data, 2),
> +};
> +
> +static const struct upboard_fpga_data upboard_up_fpga_data = {
> +     .regmapconf = &upboard_up_regmap_config,
> +     .cells = upboard_up_mfd_cells,
> +     .ncells = ARRAY_SIZE(upboard_up_mfd_cells),
> +};
> +
> +/* UP^2 board */
> +
> +static const struct regmap_range upboard_up2_readable_ranges[] = {
> +     regmap_reg_range(UPFPGA_REG_PLATFORM_ID, UPFPGA_REG_FIRMWARE_ID),
> +     regmap_reg_range(UPFPGA_REG_FUNC_EN0, UPFPGA_REG_FUNC_EN1),
> +     regmap_reg_range(UPFPGA_REG_GPIO_EN0, UPFPGA_REG_GPIO_EN2),
> +     regmap_reg_range(UPFPGA_REG_GPIO_DIR0, UPFPGA_REG_GPIO_DIR2),
> +};
> +
> +static const struct regmap_range upboard_up2_writable_ranges[] = {
> +     regmap_reg_range(UPFPGA_REG_FUNC_EN0, UPFPGA_REG_FUNC_EN1),
> +     regmap_reg_range(UPFPGA_REG_GPIO_EN0, UPFPGA_REG_GPIO_EN2),
> +     regmap_reg_range(UPFPGA_REG_GPIO_DIR0, UPFPGA_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 = UPFPGA_ADDRESS_SIZE,
> +     .val_bits = UPFPGA_REGISTER_SIZE,
> +     .max_register = UPFPGA_REG_MAX,
> +     .reg_read = upboard_fpga_read,
> +     .reg_write = upboard_fpga_write,
> +     .fast_io = false,
> +     .cache_type = REGCACHE_RBTREE,
> +     .rd_table = &upboard_up2_readable_table,
> +     .wr_table = &upboard_up2_writable_table,
> +};
> +
> +static struct upboard_led_data upboard_up2_led_data[] = {
> +     { .bit = 0, .colour = "blue" },
> +     { .bit = 1, .colour = "yellow" },
> +     { .bit = 2, .colour = "green" },
> +     { .bit = 3, .colour = "red" },
> +};
> +
> +static const struct mfd_cell upboard_up2_mfd_cells[] = {
> +     { .name = "upboard-pinctrl" },
> +     UPBOARD_LED_CELL(upboard_up2_led_data, 0),
> +     UPBOARD_LED_CELL(upboard_up2_led_data, 1),
> +     UPBOARD_LED_CELL(upboard_up2_led_data, 2),
> +     UPBOARD_LED_CELL(upboard_up2_led_data, 3),
> +};
> +
> +static const struct upboard_fpga_data upboard_up2_fpga_data = {
> +     .regmapconf = &upboard_up2_regmap_config,
> +     .cells = upboard_up2_mfd_cells,
> +     .ncells = ARRAY_SIZE(upboard_up2_mfd_cells),
> +};
> +
> +/* UP-CREX carrier board for UP Core */
> +
> +/* same MAXV config as UP1 (proto2 release) */
> +#define upboard_upcore_crex_fpga_data upboard_up_fpga_data
> +
> +/* UP-CRST02 carrier board for UP Core */
> +
> +/* same MAX10 config as UP2, but same LED cells as UP1 */
> +static const struct upboard_fpga_data upboard_upcore_crst02_fpga_data = {
> +     .regmapconf = &upboard_up2_regmap_config,
> +     .cells = upboard_up_mfd_cells,
> +     .ncells = ARRAY_SIZE(upboard_up_mfd_cells),
> +};
> +
> +static int upboard_fpga_read(void *context, unsigned int reg, unsigned int *val)
> +{
> +     struct upboard_fpga * const fpga = context;
> +     int i;
> +
> +     gpiod_set_value(fpga->clear_gpio, 0);
> +     gpiod_set_value(fpga->clear_gpio, 1);
> +
> +     reg |= UPFPGA_READ_FLAG;
> +
> +     for (i = UPFPGA_ADDRESS_SIZE; i >= 0; i--) {
> +             gpiod_set_value(fpga->strobe_gpio, 0);
> +             gpiod_set_value(fpga->datain_gpio, (reg >> i) & 0x1);
> +             gpiod_set_value(fpga->strobe_gpio, 1);
> +     }
> +
> +     gpiod_set_value(fpga->strobe_gpio, 0);
> +     *val = 0;
> +
> +     for (i = UPFPGA_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);

A little bit of commentary in here would help us understand how this
works.
Larry (11/09) :  please check the excel file "180208 UP2 FPGAcomm (new) protocol v0.2" sheet 2 " ref(FPGA comm port)", 
the above code is for FPGA read and write protocol usage.

> +     return 0;
> +};
> +
> +static int upboard_fpga_write(void *context, unsigned int reg, unsigned int val)
> +{
> +     struct upboard_fpga * const fpga = context;
> +     int i;
> +
> +     gpiod_set_value(fpga->clear_gpio, 0);
> +     gpiod_set_value(fpga->clear_gpio, 1);
> +
> +     for (i = UPFPGA_ADDRESS_SIZE; i >= 0; i--) {
> +             gpiod_set_value(fpga->strobe_gpio, 0);
> +             gpiod_set_value(fpga->datain_gpio, (reg >> i) & 0x1);
> +             gpiod_set_value(fpga->strobe_gpio, 1);
> +     }
> +
> +     gpiod_set_value(fpga->strobe_gpio, 0);
> +
> +     for (i = UPFPGA_REGISTER_SIZE - 1; i >= 0; i--) {
> +             gpiod_set_value(fpga->datain_gpio, (val >> i) & 0x1);
> +             gpiod_set_value(fpga->strobe_gpio, 1);
> +             gpiod_set_value(fpga->strobe_gpio, 0);
> +     }
> +
> +     gpiod_set_value(fpga->strobe_gpio, 1);

As above.
Larry (11/09) :  please check the excel file "180208 UP2 FPGAcomm (new) protocol v0.2" sheet 2 " ref(FPGA comm port)", 
the above code is for FPGA read and write protocol usage.

> +     return 0;
> +};
> +
> +static int __init upboard_fpga_gpio_init(struct upboard_fpga *fpga)
> +{
> +     enum gpiod_flags flags;
> +
> +     flags = fpga->uninitialised ? GPIOD_OUT_LOW : GPIOD_ASIS;

Nit: '\n'
Larry (11/09) : we will fix it on next release (PATCH V2)

> +     fpga->enable_gpio = devm_gpiod_get(fpga->dev, "enable", flags);
> +     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);
> +
> +     /* The SoC pinctrl driver may not support reserving the GPIO line for
> +      * FPGA reset without causing an undesired reset pulse. This will clear
> +      * any settings on the FPGA, so only do it if we must.
> +      */

Please use correctly formatted multi-line comments.
Larry (11/09) : we will fix it on next release (PATCH V2)

> +     if (fpga->uninitialised) {
> +             fpga->reset_gpio = devm_gpiod_get(fpga->dev, "reset",
> +                                               GPIOD_OUT_LOW);
> +             if (IS_ERR(fpga->reset_gpio))
> +                     return PTR_ERR(fpga->reset_gpio);
> +
> +             gpiod_set_value(fpga->reset_gpio, 1);
> +     }
> +
> +     gpiod_set_value(fpga->enable_gpio, 1);
> +     fpga->uninitialised = false;
> +
> +     return 0;
> +}
> +
> +static int __init upboard_fpga_detect_firmware(struct upboard_fpga *fpga)
> +{
> +     const unsigned int AAEON_MANUFACTURER_ID = 0x01;

Please define these.

Variables should be lower case.
Larry (11/09) : we will fix it on next release (PATCH V2)

> +     const unsigned int SUPPORTED_FW_MAJOR = 0x0;
> +     unsigned int platform_id, manufacturer_id;
> +     unsigned int firmware_id, build, major, minor, patch;
> +     int ret;
> +
> +     ret = regmap_read(fpga->regmap, UPFPGA_REG_PLATFORM_ID, &platform_id);
> +     if (ret)
> +             return ret;
> +
> +     manufacturer_id = platform_id & 0xff;

#define the 0xff - perhaps MANUFACTURER_MASK.

> +     if (manufacturer_id != AAEON_MANUFACTURER_ID) {
> +             dev_dbg(fpga->dev,
> +                     "driver not compatible with custom FPGA FW from manufacturer id 0x%02x. Exiting",
> +                     manufacturer_id);
> +             return -ENODEV;

If this is an error, your print messages should be dev_err().
Larry (11/09) : we will fix it on next release (PATCH V2)

> +     }
> +
> +     ret = regmap_read(fpga->regmap, UPFPGA_REG_FIRMWARE_ID, &firmware_id);
> +     if (ret)
> +             return ret;
> +
> +     build = (firmware_id >> 12) & 0xf;
> +     major = (firmware_id >> 8) & 0xf;
> +     minor = (firmware_id >> 4) & 0xf;
> +     patch = firmware_id & 0xf;

Nit: '\n'
Larry (11/09) : we will fix it on next release (PATCH V2)

> +     if (major != SUPPORTED_FW_MAJOR) {
> +             dev_dbg(fpga->dev, "unsupported FPGA FW v%u.%u.%u build 0x%02x",
> +                     major, minor, patch, build);
> +             return -ENODEV;
> +     }
> +
> +     dev_info(fpga->dev, "compatible FPGA FW v%u.%u.%u build 0x%02x",
> +              major, minor, patch, build);

Nit: '\n'
Larry (11/09) : we will fix it on next release (PATCH V2)

> +     return 0;
> +}
> +
> +static const struct acpi_device_id upboard_fpga_acpi_match[] = {
> +     { "AANT0F00", (kernel_ulong_t)&upboard_up_fpga_data },
> +     { "AANT0F01", (kernel_ulong_t)&upboard_up2_fpga_data },
> +     { "AANT0F02", (kernel_ulong_t)&upboard_upcore_crex_fpga_data },
> +     { "AANT0F03", (kernel_ulong_t)&upboard_upcore_crst02_fpga_data },
> +     { "AANT0F04", (kernel_ulong_t)&upboard_up_fpga_data },
> +     { }
> +};
> +MODULE_DEVICE_TABLE(acpi, upboard_fpga_acpi_match);

Please don't mix platform registration strategies.

Pass platform / hardware IDs instead and to the selecting in C.
Larry (11/09) : we cannot understand it clearly. could you kindly explain it more?

> +#define UPFPGA_QUIRK_UNINITIALISED  BIT(0)
> +#define UPFPGA_QUIRK_HRV1_IS_PROTO2 BIT(1)
> +#define UPFPGA_QUIRK_GPIO_LED       BIT(2)
> +
> +static const struct dmi_system_id upboard_dmi_table[] __initconst = {
> +     {
> +             .matches = { /* UP */
> +                     DMI_EXACT_MATCH(DMI_SYS_VENDOR, "AAEON"),
> +                     DMI_EXACT_MATCH(DMI_BOARD_NAME, "UP-CHT01"),
> +                     DMI_EXACT_MATCH(DMI_BOARD_VERSION, "V0.4"),
> +             },
> +             .driver_data = (void *)UPFPGA_QUIRK_UNINITIALISED,
> +     },
> +     {
> +             .matches = { /* UP2 */
> +                     DMI_EXACT_MATCH(DMI_SYS_VENDOR, "AAEON"),
> +                     DMI_EXACT_MATCH(DMI_BOARD_NAME, "UP-APL01"),
> +                     DMI_EXACT_MATCH(DMI_BOARD_VERSION, "V0.3"),
> +             },
> +             .driver_data = (void *)(UPFPGA_QUIRK_UNINITIALISED |
> +                     UPFPGA_QUIRK_HRV1_IS_PROTO2),
> +     },
> +     {
> +             .matches = { /* UP2 Pro*/
> +                     DMI_EXACT_MATCH(DMI_SYS_VENDOR, "AAEON"),
> +                     DMI_EXACT_MATCH(DMI_BOARD_NAME, "UPN-APL01"),
> +                     DMI_EXACT_MATCH(DMI_BOARD_VERSION, "V1.0"),
> +             },
> +             .driver_data = (void *)UPFPGA_QUIRK_HRV1_IS_PROTO2,
> +     },
> +     {
> +             .matches = { /* UP2 */
> +                     DMI_EXACT_MATCH(DMI_SYS_VENDOR, "AAEON"),
> +                     DMI_EXACT_MATCH(DMI_BOARD_NAME, "UP-APL01"),
> +                     DMI_EXACT_MATCH(DMI_BOARD_VERSION, "V0.4"),
> +             },
> +             .driver_data = (void *)UPFPGA_QUIRK_HRV1_IS_PROTO2,
> +     },
> +     {
> +             .matches = { /* UP Xtreme */
> +                     DMI_EXACT_MATCH(DMI_SYS_VENDOR, "AAEON"),
> +                     DMI_EXACT_MATCH(DMI_BOARD_NAME, "UP-WHL01"),
> +                     DMI_EXACT_MATCH(DMI_BOARD_VERSION, "V0.1"),
> +             },
> +     },
> +     {
> +             .matches = { /* UP APL03 */
> +                     DMI_EXACT_MATCH(DMI_SYS_VENDOR, "AAEON"),
> +                     DMI_EXACT_MATCH(DMI_BOARD_NAME, "UP-APL03"),
> +                     DMI_EXACT_MATCH(DMI_BOARD_VERSION, "V1.0"),
> +             },
> +             .driver_data = (void *)(UPFPGA_QUIRK_HRV1_IS_PROTO2 |
> +                     UPFPGA_QUIRK_GPIO_LED),
> +     },
> +     { },
> +};
> +
> +#define UPFPGA_PROTOCOL_V2_HRV 2

How does this differ from UPFPGA_QUIRK_HRV1_IS_PROTO2?
Larry (11/09) : There is an exception (quirk) that HRV1 support protocol 2, 
this is different with UPFPGA_PROTOCOL_V2_HRV.

> +static int __init upboard_fpga_probe(struct platform_device *pdev)
> +{
> +     struct upboard_fpga *fpga;

This is more widely known as 'ddata'.
Larry (11/09) : we cannot understand it clearly. could you kindly explain it more?

> +     const struct acpi_device_id *id;
> +     const struct upboard_fpga_data *fpga_data;
> +     const struct dmi_system_id *system_id;
> +     acpi_handle handle;
> +     acpi_status status;
> +     unsigned long long hrv;
> +     unsigned long quirks = 0;
> +     int ret;
> +
> +     id = acpi_match_device(upboard_fpga_acpi_match, &pdev->dev);
> +     if (!id)
> +             return -ENODEV;
> +
> +     handle = ACPI_HANDLE(&pdev->dev);
> +     status = acpi_evaluate_integer(handle, "_HRV", NULL, &hrv);
> +     if (ACPI_FAILURE(status)) {
> +             dev_err(&pdev->dev, "failed to get PCTL revision");
> +             return -ENODEV;
> +     }
> +
> +     system_id = dmi_first_match(upboard_dmi_table);
> +     if (system_id)
> +             quirks = (unsigned long)system_id->driver_data;
> +
> +     if (hrv == 1 && (quirks & UPFPGA_QUIRK_HRV1_IS_PROTO2))

What is HRV?  And what is 1?  Please define.
Larry (11/09) : we will fix it on next release (PATCH V2)

> +             hrv = UPFPGA_PROTOCOL_V2_HRV;
> +
> +     if (hrv != UPFPGA_PROTOCOL_V2_HRV) {
> +             dev_dbg(&pdev->dev, "unsupported PCTL revision: %llu", hrv);
> +             return -ENODEV;
> +     }
> +
> +     fpga_data = (const struct upboard_fpga_data *) id->driver_data;
> +
> +     fpga = devm_kzalloc(&pdev->dev, sizeof(*fpga), GFP_KERNEL);
> +     if (!fpga)
> +             return -ENOMEM;
> +
> +     if (quirks & UPFPGA_QUIRK_UNINITIALISED) {
> +             dev_info(&pdev->dev, "FPGA not initialised by this BIOS");
> +             fpga->uninitialised = true;
> +     }
> +
> +     dev_set_drvdata(&pdev->dev, fpga);
> +     fpga->dev = &pdev->dev;
> +     fpga->regmap = devm_regmap_init(&pdev->dev, NULL, fpga,
> +                                     fpga_data->regmapconf);
> +     if (IS_ERR(fpga->regmap))
> +             return PTR_ERR(fpga->regmap);
> +
> +     ret = upboard_fpga_gpio_init(fpga);
> +     if (ret) {
> +             dev_err(&pdev->dev, "failed to init FPGA comm GPIOs: %d", ret);

Please expand 'init' and 'comm'.
Larry (11/09) : we will fix it on next release (PATCH V2)

> +             return ret;
> +     }
> +
> +     ret = upboard_fpga_detect_firmware(fpga);
> +     if (ret)
> +             return ret;
> +
> +     if (quirks & UPFPGA_QUIRK_GPIO_LED) {
> +#define APL_GPIO_218 507

Please define somewhere more central.
Larry (11/09) : we will fix it on next release (PATCH V2)

> +             static struct gpio_led upboard_gpio_leds[] = {
> +                     {
> +                             .name = "upboard:blue:",
> +                             .gpio = APL_GPIO_218,
> +                             .default_state = LEDS_GPIO_DEFSTATE_KEEP,
> +                     },
> +             };
> +             static struct gpio_led_platform_data upboard_gpio_led_platform_data = {
> +                     .num_leds = ARRAY_SIZE(upboard_gpio_leds),
> +                     .leds = upboard_gpio_leds,
> +             };
> +             static const struct mfd_cell upboard_gpio_led_cells[] = {
> +                     {
> +                             .name = "leds-gpio",
> +                             .id = 0,
> +                             .platform_data = &upboard_gpio_led_platform_data,
> +                             .pdata_size = sizeof(upboard_gpio_led_platform_data),
> +                     },
> +             };

This is ugly and serves little purpose.

Place all of these structs with the others above.
Larry (11/09) : we will fix it on next release (PATCH V2)

> +             ret =  devm_mfd_add_devices(&pdev->dev, 0, upboard_gpio_led_cells,

Not '0'.  Please use the defines provided.
Larry (11/09) : we will fix it on next release (PATCH V2)

> +                                 ARRAY_SIZE(upboard_gpio_led_cells), NULL, 0, NULL);
> +             if (ret) {
> +                     dev_err(&pdev->dev, "Failed to add GPIO leds");
> +                     return ret;
> +             }
> +

Superfluous line.
Larry (11/09) : we will fix it on next release (PATCH V2)

> +     }
> +
> +     return devm_mfd_add_devices(&pdev->dev, 0, fpga_data->cells,

As above.
Larry (11/09) : we will fix it on next release (PATCH V2)

> +                                 fpga_data->ncells, NULL, 0, NULL);
> +}
> +
> +static struct platform_driver upboard_fpga_driver = {
> +     .driver = {
> +             .name = "upboard-fpga",
> +             .acpi_match_table = upboard_fpga_acpi_match,
> +     },
> +};
> +
> +module_platform_driver_probe(upboard_fpga_driver, upboard_fpga_probe);
> +
> +MODULE_AUTHOR("Javier Arteaga <javier@emutex.com>");
> +MODULE_DESCRIPTION("UP Board FPGA driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/mfd/upboard-fpga.h b/include/linux/mfd/upboard-fpga.h
> new file mode 100644
> index 000000000000..e0940120514d
> --- /dev/null
> +++ b/include/linux/mfd/upboard-fpga.h
> @@ -0,0 +1,49 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * UP Board FPGA MFD driver interface

Please remove the term MFD.
Larry (11/09) : we will fix it on next release (PATCH V2)

> + * Copyright (c) 2017, Emutex Ltd. All rights reserved.
> + *
> + * Author: Javier Arteaga <javier@emutex.com>
> + */
> +
> +#ifndef __LINUX_MFD_UPBOARD_FPGA_H
> +#define __LINUX_MFD_UPBOARD_FPGA_H
> +
> +#define UPFPGA_ADDRESS_SIZE  7
> +#define UPFPGA_REGISTER_SIZE 16
> +
> +#define UPFPGA_READ_FLAG     (1 << UPFPGA_ADDRESS_SIZE)
> +
> +enum upboard_fpgareg {
> +     UPFPGA_REG_PLATFORM_ID   = 0x10,
> +     UPFPGA_REG_FIRMWARE_ID   = 0x11,
> +     UPFPGA_REG_FUNC_EN0      = 0x20,
> +     UPFPGA_REG_FUNC_EN1      = 0x21,
> +     UPFPGA_REG_GPIO_EN0      = 0x30,
> +     UPFPGA_REG_GPIO_EN1      = 0x31,
> +     UPFPGA_REG_GPIO_EN2      = 0x32,
> +     UPFPGA_REG_GPIO_DIR0     = 0x40,
> +     UPFPGA_REG_GPIO_DIR1     = 0x41,
> +     UPFPGA_REG_GPIO_DIR2     = 0x42,
> +     UPFPGA_REG_MAX,
> +};
> +
> +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;
> +     bool uninitialised;
> +};
> +
> +struct upboard_led_data {
> +     unsigned int bit;
> +     const char *colour;
> +};
> +
> +#endif /*  __LINUX_MFD_UPBOARD_FPGA_H */

-- 
Lee Jones [李琼斯]

[-- Attachment #2: 180208 UP2 FPGAcomm (new) protocol v0.2.xlsx --]
[-- Type: application/vnd.openxmlformats-officedocument.spreadsheetml.sheet, Size: 194212 bytes --]

Re: 回覆: [PATCH 1/5] mfd: Add support for UP board CPLD/FPGA

[Lee Jones]

On Tue, 08 Nov 2022, gregkh@linuxfoundation.org wrote:

> On Tue, Nov 08, 2022 at 04:31:17PM +0000, Larry Lai wrote:
> > Send again with Text only.
> 
> Sorry, was in HTML format :(

Not sure what Greg is replying to here, but it's not in my inbox.

-- 
Lee Jones [李琼斯]