Dear Gregkh,
Send this mail again with Text only, I am sorry for any inconvenience.
Larry
寄件者: Larry Lai <larry.lai@xxxxxxxxxxxxxxx>
寄件日期: 2022年11月9日 上午 12:31
收件者: Lee Jones <lee@xxxxxxxxxx>; chengwei <foxfly.lai.tw@xxxxxxxxx>
副本: broonie@xxxxxxxxxx <broonie@xxxxxxxxxx>; rafael@xxxxxxxxxx <rafael@xxxxxxxxxx>; mika.westerberg@xxxxxxxxxxxxxxx <mika.westerberg@xxxxxxxxxxxxxxx>; andriy.shevchenko@xxxxxxxxxxxxxxx <andriy.shevchenko@xxxxxxxxxxxxxxx>; linus.walleij@xxxxxxxxxx <linus.walleij@xxxxxxxxxx>; brgl@xxxxxxxx <brgl@xxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx <linux-kernel@xxxxxxxxxxxxxxx>; gregkh@xxxxxxxxxxxxxxxxxxx <gregkh@xxxxxxxxxxxxxxxxxxx>; lenb@xxxxxxxxxx <lenb@xxxxxxxxxx>; linux-acpi@xxxxxxxxxxxxxxx <linux-acpi@xxxxxxxxxxxxxxx>; linux-gpio@xxxxxxxxxxxxxxx <linux-gpio@xxxxxxxxxxxxxxx>; GaryWang@xxxxxxxxxxxx <GaryWang@xxxxxxxxxxxx>; Musa Lin <musa.lin@xxxxxxxxxxxxxxx>; Jack Chang <jack.chang@xxxxxxxxxxxxxxx>; Javier Arteaga <javier@xxxxxxxxxx>; Nicola Lunghi <nicola.lunghi@xxxxxxxxxx>
主旨: 回覆: [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@xxxxxxxxxx>
日期: 星期一, 2022年10月31日 下午10:58
收件者: chengwei <foxfly.lai.tw@xxxxxxxxx>
副本: broonie@xxxxxxxxxx <broonie@xxxxxxxxxx>, rafael@xxxxxxxxxx <rafael@xxxxxxxxxx>, mika.westerberg@xxxxxxxxxxxxxxx <mika.westerberg@xxxxxxxxxxxxxxx>, andriy.shevchenko@xxxxxxxxxxxxxxx <andriy.shevchenko@xxxxxxxxxxxxxxx>, linus.walleij@xxxxxxxxxx <linus.walleij@xxxxxxxxxx>, brgl@xxxxxxxx <brgl@xxxxxxxx>, linux-kernel@xxxxxxxxxxxxxxx <linux-kernel@xxxxxxxxxxxxxxx>, gregkh@xxxxxxxxxxxxxxxxxxx <gregkh@xxxxxxxxxxxxxxxxxxx>, lenb@xxxxxxxxxx <lenb@xxxxxxxxxx>, linux-acpi@xxxxxxxxxxxxxxx <linux-acpi@xxxxxxxxxxxxxxx>, linux-gpio@xxxxxxxxxxxxxxx <linux-gpio@xxxxxxxxxxxxxxx>, GaryWang@xxxxxxxxxxxx <GaryWang@xxxxxxxxxxxx>, Musa Lin <musa.lin@xxxxxxxxxxxxxxx>, Jack Chang <jack.chang@xxxxxxxxxxxxxxx>, Larry Lai <larry.lai@xxxxxxxxxxxxxxx>, Javier Arteaga <javier@xxxxxxxxxx>, Nicola Lunghi <nicola.lunghi@xxxxxxxxxx>
主旨: 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@xxxxxxxxxx>
> [merge various fixes]
> Signed-off-by: Nicola Lunghi <nicola.lunghi@xxxxxxxxxx>
> Signed-off-by: chengwei <larry.lai@xxxxxxxxxxxxxxx>
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@xxxxxxxxxx>
> + */
> +
> +#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@xxxxxxxxxx>");
> +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@xxxxxxxxxx>
> + */
> +
> +#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:
180208 UP2 FPGAcomm (new) protocol v0.2.xlsx
Description: 180208 UP2 FPGAcomm (new) protocol v0.2.xlsx
