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

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

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 */

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

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

Sorry, was in HTML format :(

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.