| Message ID | 20231031015119.29756-2-larry.lai@yunjingtech.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add support control UP board CPLD/FPGA pin control | expand |
On Tue, Oct 31, 2023 at 09:51:17AM +0800, larry.lai 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 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 s/This patch implements/Implement/ > manages direction and enable state for those header pins. > > Partial support UP boards: "for UP" or "supported" (choose one). > * UP core + CREX > * UP core + CRST02 > Reported-by: kernel test robot <lkp@intel.com> No, this tag can't be applied to the new code. > Signed-off-by: Gary Wang <garywang@aaeon.com.tw> > Signed-off-by: larry.lai <larry.lai@yunjingtech.com> Missing Co-developed-by? ... > +config MFD_INTEL_UPBOARD_FPGA I believe Intel has nothing to do with this one. The Intel SoC is accompanied with OEM FPGA, right? > + tristate "Support for the Intel platform foundation kit UP board FPGA" Depends on the above this most likely to be updated. > + select MFD_CORE > + depends on X86 && ACPI No COMPILE_TEST? > + help > + Select this option to enable the Intel AAEON UP and UP^2 on-board FPGA. Intel is Intel. AAEON is part of ASUS. They never been part of Intel AFAICT. > + This is core driver for the UP board that implements certain (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. ... > +obj-$(CONFIG_MFD_INTEL_UPBOARD_FPGA) += upboard-fpga.o Just drop INTEL_ ... > + * UP Board control CPLD/FPGA to provide more GPIO driving power > + * also provide CPLD LEDs and pin mux function > + * recognize HID AANT0F00 ~ AAANT0F04 in ACPI name space This needs a bit of English grammar / punctuation update... ... > +#include <linux/acpi.h> How is this being used? Perhaps you need mod_devicetable.h and property.h instead (see below). > +#include <linux/dmi.h> Unused. > +#include <linux/gpio/consumer.h> > +#include <linux/kernel.h> What this header is for? Perhaps you meant array_size.h and other(s)? > +#include <linux/leds.h> > +#include <linux/mfd/core.h> > +#include <linux/mfd/upboard-fpga.h> > +#include <linux/module.h> > +#include <linux/regmap.h> ... > +/* > + * read CPLD register on custom protocol > + * send clock and addr bit in strobe and datain pins then read from dataout pin > + */ As per above, seems like all your comments need to be updated to follow some English language rules... ... > +static int upboard_cpld_read(void *context, unsigned int reg, unsigned int *val) > +{ > + struct upboard_fpga * const fpga = context; > + int i; > + > + /* clear to start new transcation */ > + gpiod_set_value(fpga->clear_gpio, 0); No wait? > + gpiod_set_value(fpga->clear_gpio, 1); > + > + reg |= UPFPGA_READ_FLAG; > + > + /* send clock and data from strobe & datain */ > + for (i = UPFPGA_ADDRESS_SIZE; i >= 0; i--) { > + gpiod_set_value(fpga->strobe_gpio, 0); > + gpiod_set_value(fpga->datain_gpio, !!(reg & BIT(i))); > + gpiod_set_value(fpga->strobe_gpio, 1); > + } > + > + gpiod_set_value(fpga->strobe_gpio, 0); > + *val = 0; > + > + /* read from dataout */ > + for (i = UPFPGA_REGISTER_SIZE - 1; i >= 0; i--) { > + gpiod_set_value(fpga->strobe_gpio, 1); No wait? > + gpiod_set_value(fpga->strobe_gpio, 0); > + *val |= gpiod_get_value(fpga->dataout_gpio) << i; > + } > + > + gpiod_set_value(fpga->strobe_gpio, 1); > + > + return 0; > +} This looks like SPI bitbang. Can you utilize that driver to do this for you? ... > +static struct upboard_led_data upboard_gpio_led_data[] = { > + { .bit = 0, .colour = "gpio" }, > +}; > + > +/* 3 LEDs controlled by CPLD */ > +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[] = { > + MFD_CELL_NAME("upboard-pinctrl"), > + MFD_CELL_BASIC("upboard-led", NULL, &upboard_up_led_data[0], > + sizeof(*upboard_up_led_data), 0), > + MFD_CELL_BASIC("upboard-led", NULL, &upboard_up_led_data[1], > + sizeof(*upboard_up_led_data), 1), > + MFD_CELL_BASIC("upboard-led", NULL, &upboard_up_led_data[2], > + sizeof(*upboard_up_led_data), 2), > + MFD_CELL_BASIC("upboard-led", NULL, &upboard_gpio_led_data[0], > + sizeof(*upboard_gpio_led_data), 0), > +}; Why is not using LED framework? ... > +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[] = { > + MFD_CELL_NAME("upboard-pinctrl"), > + MFD_CELL_BASIC("upboard-led", NULL, &upboard_up2_led_data[0], > + sizeof(*upboard_up2_led_data), 0), > + MFD_CELL_BASIC("upboard-led", NULL, &upboard_up2_led_data[1], > + sizeof(*upboard_up2_led_data), 1), > + MFD_CELL_BASIC("upboard-led", NULL, &upboard_up2_led_data[2], > + sizeof(*upboard_up2_led_data), 2), > + MFD_CELL_BASIC("upboard-led", NULL, &upboard_up2_led_data[3], > + sizeof(*upboard_up2_led_data), 3), > + MFD_CELL_BASIC("upboard-led", NULL, &upboard_gpio_led_data[0], > + sizeof(*upboard_gpio_led_data), 0), > +}; Ditto. ... > +static int __init upboard_cpld_gpio_init(struct upboard_fpga *fpga) > +{ > + enum gpiod_flags flags = fpga->uninitialised ? GPIOD_OUT_LOW : GPIOD_ASIS; > + /* > + * 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. > + * Reset GPIO defaults HIGH, get GPIO and set to LOW, then set back to > + * HIGH as a pulse. > + */ So... > + if (fpga->uninitialised) { > + fpga->reset_gpio = devm_gpiod_get(fpga->dev, "reset", GPIOD_OUT_LOW); ...make it _optional() and use GPIOD_ASIS. > + if (IS_ERR(fpga->reset_gpio)) > + return PTR_ERR(fpga->reset_gpio); > + gpiod_set_value(fpga->reset_gpio, RESET_DEVICE); And with gpiod_direction_output() it may be conditionally called. > + } > + gpiod_set_value(fpga->enable_gpio, ENABLE_DEVICE); > + fpga->uninitialised = false; How this flag is anyhow useful? Are you expecting the __init marked function to be called twice? Oh, it seems even __init is wrong here... > + return 0; > +} ... > +static const struct acpi_device_id upboard_fpga_acpi_match[] = { > + { "AANT0000", AANT0000_ID }, > + { "AANT0F00", AANT0F00_ID }, > + { "AANT0F01", AANT0F01_ID }, > + { "AANT0F02", AANT0F02_ID }, > + { "AANT0F03", AANT0F03_ID }, > + { "AANT0F04", AANT0F04_ID }, > + { } > +}; > +MODULE_DEVICE_TABLE(acpi, upboard_fpga_acpi_match); Move this closer to its real user (struct platform_driver below). ... > +static int __init upboard_fpga_probe(struct platform_device *pdev) How comes it's marked with __init?! Have you tested it? ... > + id = acpi_match_device(upboard_fpga_acpi_match, dev); > + if (!id) > + return -ENODEV; No, use device_get_match_data() from property.h. ... > + switch (id->driver_data) { > + case AANT0F00_ID: > + cpld_config = &upboard_up_regmap_config; > + cells = upboard_up_mfd_cells; > + ncells = ARRAY_SIZE(upboard_up_mfd_cells); > + break; > + case AANT0F01_ID: > + cpld_config = &upboard_up2_regmap_config; > + cells = upboard_up2_mfd_cells; > + ncells = ARRAY_SIZE(upboard_up2_mfd_cells); > + break; > + case AANT0F02_ID: > + cpld_config = &upboard_up_regmap_config; > + cells = upboard_up_mfd_cells; > + ncells = ARRAY_SIZE(upboard_up_mfd_cells); > + break; > + case AANT0F03_ID: > + cpld_config = &upboard_up2_regmap_config; > + cells = upboard_up_mfd_cells; > + ncells = ARRAY_SIZE(upboard_up_mfd_cells); > + break; > + case AANT0F04_ID: > + cpld_config = &upboard_up_regmap_config; > + cells = upboard_up_mfd_cells; > + ncells = ARRAY_SIZE(upboard_up_mfd_cells); > + break; > + case AANT0000_ID: > + default: > + cpld_config = &upboard_up_regmap_config; > + cells = upboard_pinctrl_cells; > + ncells = ARRAY_SIZE(upboard_pinctrl_cells); > + break; > + } Drop this and make a custom structure which will be part of the driver data, let's call it struct upboard_info. When it's done, you will simply have to access constant info structure whenever you want to. ... > + platform_set_drvdata(pdev, ddata); How is this being used? ... > +enum upcpld_ids { > + AANT0000_ID = 255, Why not to start from 0? > + AANT0F00_ID = 0, > + AANT0F01_ID = 1, > + AANT0F02_ID = 2, > + AANT0F03_ID = 3, > + AANT0F04_ID = 4, > +}; ... > +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, No comma for the termination. > +}; ... Also, please split by models, first you add a driver with a single board support and each new board addition is done in a separate change.
Hi Andy,
Thank you for review the V7 patch and sorry for my poor English, for your question, please kindly to check our comments with ">>" beginning.
-----Original Message-----
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Sent: Tuesday, November 14, 2023 10:11 PM
To: larry.lai <larry.lai@yunjingtech.com>
Cc: lee@kernel.org; linus.walleij@linaro.org; pavel@ucw.cz; linux-kernel@vger.kernel.org; linux-gpio@vger.kernel.org; linux-leds@vger.kernel.org; GaryWang 汪之逸 <GaryWang@aaeon.com.tw>; musa.lin@yunjingtech.com; jack.chang@yunjingtech.com; noah.hung@yunjingtech.com
Subject: Re: [PATCH V7 1/3] mfd: Add support for UP board CPLD/FPGA
On Tue, Oct 31, 2023 at 09:51:17AM +0800, larry.lai 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 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
s/This patch implements/Implement/
> manages direction and enable state for those header pins.
>
> Partial support UP boards:
"for UP" or "supported" (choose one).
> * UP core + CREX
> * UP core + CRST02
> Reported-by: kernel test robot <lkp@intel.com>
No, this tag can't be applied to the new code.
> Signed-off-by: Gary Wang <garywang@aaeon.com.tw>
> Signed-off-by: larry.lai <larry.lai@yunjingtech.com>
Missing Co-developed-by?
>> larry is our consultant for upstream
...
> +config MFD_INTEL_UPBOARD_FPGA
I believe Intel has nothing to do with this one. The Intel SoC is accompanied with OEM FPGA, right?
>> we used Intel CPLD Altera MAX V/X for pin mux and provide more driving power for Raspberry Pi compatible HAT pins, will remove "INTEL"
> + tristate "Support for the Intel platform foundation kit UP board FPGA"
Depends on the above this most likely to be updated.
>> ok
> + select MFD_CORE
> + depends on X86 && ACPI
No COMPILE_TEST?
>> will do in next patch
> + help
> + Select this option to enable the Intel AAEON UP and UP^2 on-board FPGA.
Intel is Intel.
AAEON is part of ASUS.
They never been part of Intel AFAICT.
>> yeah, will do in next patch
> + This is core driver for the UP board that implements certain (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.
...
> +obj-$(CONFIG_MFD_INTEL_UPBOARD_FPGA) += upboard-fpga.o
Just drop INTEL_
>> will do
...
> + * UP Board control CPLD/FPGA to provide more GPIO driving power
> + * also provide CPLD LEDs and pin mux function
> + * recognize HID AANT0F00 ~ AAANT0F04 in ACPI name space
This needs a bit of English grammar / punctuation update...
>> will do
...
> +#include <linux/acpi.h>
How is this being used? Perhaps you need mod_devicetable.h and property.h instead (see below).
>> acpi_match_device to check id and assign driver data.
> +#include <linux/dmi.h>
Unused.
>> yes
> +#include <linux/gpio/consumer.h>
> +#include <linux/kernel.h>
What this header is for? Perhaps you meant array_size.h and other(s)?
>> no need, will remove.
> +#include <linux/leds.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/upboard-fpga.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
...
> +/*
> + * read CPLD register on custom protocol
> + * send clock and addr bit in strobe and datain pins then read from
> +dataout pin */
As per above, seems like all your comments need to be updated to follow some English language rules...
>> will do
...
> +static int upboard_cpld_read(void *context, unsigned int reg,
> +unsigned int *val) {
> + struct upboard_fpga * const fpga = context;
> + int i;
> +
> + /* clear to start new transcation */
> + gpiod_set_value(fpga->clear_gpio, 0);
No wait?
> + gpiod_set_value(fpga->clear_gpio, 1);
> +
> + reg |= UPFPGA_READ_FLAG;
> +
> + /* send clock and data from strobe & datain */
> + for (i = UPFPGA_ADDRESS_SIZE; i >= 0; i--) {
> + gpiod_set_value(fpga->strobe_gpio, 0);
> + gpiod_set_value(fpga->datain_gpio, !!(reg & BIT(i)));
> + gpiod_set_value(fpga->strobe_gpio, 1);
> + }
> +
> + gpiod_set_value(fpga->strobe_gpio, 0);
> + *val = 0;
> +
> + /* read from dataout */
> + for (i = UPFPGA_REGISTER_SIZE - 1; i >= 0; i--) {
> + gpiod_set_value(fpga->strobe_gpio, 1);
No wait?
> + gpiod_set_value(fpga->strobe_gpio, 0);
> + *val |= gpiod_get_value(fpga->dataout_gpio) << i;
> + }
> +
> + gpiod_set_value(fpga->strobe_gpio, 1);
> +
> + return 0;
> +}
This looks like SPI bitbang. Can you utilize that driver to do this for you?
>> the protocol is defined by ourselves use 5 gpio pins to communication with CPLD 16bit or 24bit data(different CPLD and firmware).
...
> +static struct upboard_led_data upboard_gpio_led_data[] = {
> + { .bit = 0, .colour = "gpio" },
> +};
> +
> +/* 3 LEDs controlled by CPLD */
> +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[] = {
> + MFD_CELL_NAME("upboard-pinctrl"),
> + MFD_CELL_BASIC("upboard-led", NULL, &upboard_up_led_data[0],
> + sizeof(*upboard_up_led_data), 0),
> + MFD_CELL_BASIC("upboard-led", NULL, &upboard_up_led_data[1],
> + sizeof(*upboard_up_led_data), 1),
> + MFD_CELL_BASIC("upboard-led", NULL, &upboard_up_led_data[2],
> + sizeof(*upboard_up_led_data), 2),
> + MFD_CELL_BASIC("upboard-led", NULL, &upboard_gpio_led_data[0],
> + sizeof(*upboard_gpio_led_data), 0), };
Why is not using LED framework?
...
>> see below
> +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[] = {
> + MFD_CELL_NAME("upboard-pinctrl"),
> + MFD_CELL_BASIC("upboard-led", NULL, &upboard_up2_led_data[0],
> + sizeof(*upboard_up2_led_data), 0),
> + MFD_CELL_BASIC("upboard-led", NULL, &upboard_up2_led_data[1],
> + sizeof(*upboard_up2_led_data), 1),
> + MFD_CELL_BASIC("upboard-led", NULL, &upboard_up2_led_data[2],
> + sizeof(*upboard_up2_led_data), 2),
> + MFD_CELL_BASIC("upboard-led", NULL, &upboard_up2_led_data[3],
> + sizeof(*upboard_up2_led_data), 3),
> + MFD_CELL_BASIC("upboard-led", NULL, &upboard_gpio_led_data[0],
> + sizeof(*upboard_gpio_led_data), 0), };
Ditto.
>> CPLD has 3~4 pins to control LED, the CPLD is a multi-function device
...
> +static int __init upboard_cpld_gpio_init(struct upboard_fpga *fpga) {
> + enum gpiod_flags flags = fpga->uninitialised ? GPIOD_OUT_LOW :
> +GPIOD_ASIS;
> + /*
> + * 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.
> + * Reset GPIO defaults HIGH, get GPIO and set to LOW, then set back to
> + * HIGH as a pulse.
> + */
So...
> + if (fpga->uninitialised) {
> + fpga->reset_gpio = devm_gpiod_get(fpga->dev, "reset",
> +GPIOD_OUT_LOW);
...make it _optional() and use GPIOD_ASIS.
> + if (IS_ERR(fpga->reset_gpio))
> + return PTR_ERR(fpga->reset_gpio);
> + gpiod_set_value(fpga->reset_gpio, RESET_DEVICE);
And with gpiod_direction_output() it may be conditionally called.
> + }
> + gpiod_set_value(fpga->enable_gpio, ENABLE_DEVICE);
> + fpga->uninitialised = false;
How this flag is anyhow useful? Are you expecting the __init marked function to be called twice?
>> it's for older firmware, no needed and will remove.
Oh, it seems even __init is wrong here...
>> yeah, no needed will remove.
> + return 0;
> +}
...
> +static const struct acpi_device_id upboard_fpga_acpi_match[] = {
> + { "AANT0000", AANT0000_ID },
> + { "AANT0F00", AANT0F00_ID },
> + { "AANT0F01", AANT0F01_ID },
> + { "AANT0F02", AANT0F02_ID },
> + { "AANT0F03", AANT0F03_ID },
> + { "AANT0F04", AANT0F04_ID },
> + { }
> +};
> +MODULE_DEVICE_TABLE(acpi, upboard_fpga_acpi_match);
Move this closer to its real user (struct platform_driver below).
...
> +static int __init upboard_fpga_probe(struct platform_device *pdev)
How comes it's marked with __init?! Have you tested it?
>> yeah we have compiler and tested before send patch, will remove it the other reviewer has mention this.
...
> + id = acpi_match_device(upboard_fpga_acpi_match, dev);
> + if (!id)
> + return -ENODEV;
No, use device_get_match_data() from property.h.
...
> + switch (id->driver_data) {
> + case AANT0F00_ID:
> + cpld_config = &upboard_up_regmap_config;
> + cells = upboard_up_mfd_cells;
> + ncells = ARRAY_SIZE(upboard_up_mfd_cells);
> + break;
> + case AANT0F01_ID:
> + cpld_config = &upboard_up2_regmap_config;
> + cells = upboard_up2_mfd_cells;
> + ncells = ARRAY_SIZE(upboard_up2_mfd_cells);
> + break;
> + case AANT0F02_ID:
> + cpld_config = &upboard_up_regmap_config;
> + cells = upboard_up_mfd_cells;
> + ncells = ARRAY_SIZE(upboard_up_mfd_cells);
> + break;
> + case AANT0F03_ID:
> + cpld_config = &upboard_up2_regmap_config;
> + cells = upboard_up_mfd_cells;
> + ncells = ARRAY_SIZE(upboard_up_mfd_cells);
> + break;
> + case AANT0F04_ID:
> + cpld_config = &upboard_up_regmap_config;
> + cells = upboard_up_mfd_cells;
> + ncells = ARRAY_SIZE(upboard_up_mfd_cells);
> + break;
> + case AANT0000_ID:
> + default:
> + cpld_config = &upboard_up_regmap_config;
> + cells = upboard_pinctrl_cells;
> + ncells = ARRAY_SIZE(upboard_pinctrl_cells);
> + break;
> + }
Drop this and make a custom structure which will be part of the driver data, let's call it struct upboard_info. When it's done, you will simply have to access constant info structure whenever you want to.
...
>> will do.
> + platform_set_drvdata(pdev, ddata);
How is this being used?
...
> +enum upcpld_ids {
> + AANT0000_ID = 255,
Why not to start from 0?
>> this HID is testing purpose for ACPI table injection, not included in BIOS firmware.
> + AANT0F00_ID = 0,
> + AANT0F01_ID = 1,
> + AANT0F02_ID = 2,
> + AANT0F03_ID = 3,
> + AANT0F04_ID = 4,
> +};
...
> +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,
No comma for the termination.
>> will do.
> +};
...
Also, please split by models, first you add a driver with a single board support and each new board addition is done in a separate change.
>> the driver design is based on CPLD, the black diagram showing the dependence, pinctrl-upboard driver will check each models.
-------------------------------------- --------------
| Intel SOC,1.8V | <---> |ADC Chip | kernel native driver
| GPIO/I2C/SPI/UART/PWM | |SPI/I2C |
-------------------------------------- --------------
| |
----------------------------------------------------------
| CPLD/FPGA | upboard-fpga CPLD control driver
| provide more driving power | register leds-upboard
| HAT pins mux function | register pinctrl-upboard
---------------------------------------------------------
| |
---------- -------------------------------------------
|3 or 4| | HAT 40 pins, 3.3V | leds-upboard led driver
| Leds | |GPIO/ADC/I2C/SPI/UART/PWM| pinctrl-upboard hat pin control driver
---------- -------------------------------------------
--
With Best Regards,
Andy Shevchenko
On Fri, 24 Nov 2023, GaryWang 汪之逸 wrote: > Hi Andy, > > Thank you for review the V7 patch and sorry for my poor English, for your question, please kindly to check our comments with ">>" beginning. Please fix your mail client instead. Or use a different one. > -----Original Message----- > From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Sent: Tuesday, November 14, 2023 10:11 PM > To: larry.lai <larry.lai@yunjingtech.com> > Cc: lee@kernel.org; linus.walleij@linaro.org; pavel@ucw.cz; linux-kernel@vger.kernel.org; linux-gpio@vger.kernel.org; linux-leds@vger.kernel.org; GaryWang 汪之逸 <GaryWang@aaeon.com.tw>; musa.lin@yunjingtech.com; jack.chang@yunjingtech.com; noah.hung@yunjingtech.com > Subject: Re: [PATCH V7 1/3] mfd: Add support for UP board CPLD/FPGA No headers in the body please. > On Tue, Oct 31, 2023 at 09:51:17AM +0800, larry.lai 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 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 > > s/This patch implements/Implement/ > > > manages direction and enable state for those header pins. > > > > Partial support UP boards: > > "for UP" or "supported" (choose one). > > > * UP core + CREX > > * UP core + CRST02 > > > Reported-by: kernel test robot <lkp@intel.com> > > No, this tag can't be applied to the new code. > > > Signed-off-by: Gary Wang <garywang@aaeon.com.tw> > > Signed-off-by: larry.lai <larry.lai@yunjingtech.com> > > Missing Co-developed-by? > >> larry is our consultant for upstream This is confusing. More '>'s usually means deeper quotes. Your reply should be up against the left wall, like this one. > ... > > > +config MFD_INTEL_UPBOARD_FPGA > > I believe Intel has nothing to do with this one. The Intel SoC is accompanied with OEM FPGA, right? > >> we used Intel CPLD Altera MAX V/X for pin mux and provide more driving power for Raspberry Pi compatible HAT pins, will remove "INTEL" Please enable line-wrap. > > + tristate "Support for the Intel platform foundation kit UP board FPGA" > > Depends on the above this most likely to be updated. > >> ok If you agree with a comment, please trim it from your reply. [...]
All, Reply again to plain text format & line-warp and trim agree part as Jones's suggestion, please let me know if there are still having format issue. please kindly to check our comments with ">>" beginning. -----Original Message----- From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Sent: Tuesday, November 14, 2023 10:11 PM To: larry.lai <larry.lai@yunjingtech.com> Cc: lee@kernel.org; linus.walleij@linaro.org; pavel@ucw.cz; linux-kernel@vger.kernel.org; linux-gpio@vger.kernel.org; linux-leds@vger.kernel.org; GaryWang 汪之逸 <GaryWang@aaeon.com.tw>; musa.lin@yunjingtech.com; jack.chang@yunjingtech.com; noah.hung@yunjingtech.com Subject: Re: [PATCH V7 1/3] mfd: Add support for UP board CPLD/FPGA > Signed-off-by: Gary Wang <garywang@aaeon.com.tw> > Signed-off-by: larry.lai <larry.lai@yunjingtech.com> Missing Co-developed-by? >> larry is our consultant for upstream > +config MFD_INTEL_UPBOARD_FPGA I believe Intel has nothing to do with this one. The Intel SoC is accompanied with OEM FPGA, right? >> we used Intel Altera CPLD MAX V/X for pin mux and provide more driving power for Raspberry Pi compatible HAT pins, but will remove INTEL instead > +static int upboard_cpld_read(void *context, unsigned int reg, > +unsigned int *val) { > + struct upboard_fpga * const fpga = context; > + int i; > + > + /* clear to start new transcation */ > + gpiod_set_value(fpga->clear_gpio, 0); No wait? > + gpiod_set_value(fpga->clear_gpio, 1); > + > + reg |= UPFPGA_READ_FLAG; > + > + /* send clock and data from strobe & datain */ > + for (i = UPFPGA_ADDRESS_SIZE; i >= 0; i--) { > + gpiod_set_value(fpga->strobe_gpio, 0); > + gpiod_set_value(fpga->datain_gpio, !!(reg & BIT(i))); > + gpiod_set_value(fpga->strobe_gpio, 1); > + } > + > + gpiod_set_value(fpga->strobe_gpio, 0); > + *val = 0; > + > + /* read from dataout */ > + for (i = UPFPGA_REGISTER_SIZE - 1; i >= 0; i--) { > + gpiod_set_value(fpga->strobe_gpio, 1); No wait? > + gpiod_set_value(fpga->strobe_gpio, 0); > + *val |= gpiod_get_value(fpga->dataout_gpio) << i; > + } > + > + gpiod_set_value(fpga->strobe_gpio, 1); > + > + return 0; > +} This looks like SPI bitbang. Can you utilize that driver to do this for you? >> the protocol is defined by ourselves using gpio pins to communication with CPLD 16bit or 24bit data(different CPLD and firmware). ... > +static struct upboard_led_data upboard_gpio_led_data[] = { > + { .bit = 0, .colour = "gpio" }, > +}; > + > +/* 3 LEDs controlled by CPLD */ > +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[] = { > + MFD_CELL_NAME("upboard-pinctrl"), > + MFD_CELL_BASIC("upboard-led", NULL, &upboard_up_led_data[0], > + sizeof(*upboard_up_led_data), 0), > + MFD_CELL_BASIC("upboard-led", NULL, &upboard_up_led_data[1], > + sizeof(*upboard_up_led_data), 1), > + MFD_CELL_BASIC("upboard-led", NULL, &upboard_up_led_data[2], > + sizeof(*upboard_up_led_data), 2), > + MFD_CELL_BASIC("upboard-led", NULL, &upboard_gpio_led_data[0], > + sizeof(*upboard_gpio_led_data), 0), }; Why is not using LED framework? >> see below ... > +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[] = { > + MFD_CELL_NAME("upboard-pinctrl"), > + MFD_CELL_BASIC("upboard-led", NULL, &upboard_up2_led_data[0], > + sizeof(*upboard_up2_led_data), 0), > + MFD_CELL_BASIC("upboard-led", NULL, &upboard_up2_led_data[1], > + sizeof(*upboard_up2_led_data), 1), > + MFD_CELL_BASIC("upboard-led", NULL, &upboard_up2_led_data[2], > + sizeof(*upboard_up2_led_data), 2), > + MFD_CELL_BASIC("upboard-led", NULL, &upboard_up2_led_data[3], > + sizeof(*upboard_up2_led_data), 3), > + MFD_CELL_BASIC("upboard-led", NULL, &upboard_gpio_led_data[0], > + sizeof(*upboard_gpio_led_data), 0), }; Ditto. >> CPLD has 3~4 pins to control LED, read/write CPLD to control these pins. > + AANT0F00_ID = 0, > + AANT0F01_ID = 1, > + AANT0F02_ID = 2, > + AANT0F03_ID = 3, > + AANT0F04_ID = 4, > +}; Also, please split by models, first you add a driver with a single board support and each new board addition is done in a separate change. >> the driver design is based on CPLD, the block diagram showing the dependence, pinctrl-upboard driver will check each models from DMI. -------------------------------------- -------------- | Intel SOC,1.8V | <---> |ADC Chip | kernel native driver | GPIO/I2C/SPI/UART/PWM | |SPI/I2C | -------------------------------------- -------------- | | ---------------------------------------------------------- | CPLD/FPGA | ACPI upboard-fpga CPLD control driver | provide more driving power | register leds-upboard | HAT pins mux function | register pinctrl-upboard --------------------------------------------------------- | | ---------- ------------------------------------------- |3 or 4| | HAT 40 pins, 3.3V | leds-upboard led driver | Leds | |GPIO/ADC/I2C/SPI/UART/PWM| pinctrl-upboard hat pin control driver ---------- ------------------------------------------- -- With Best Regards, Andy Shevchenko
All, Sorry, replied Gary's mail again for plain text format & mail client as Jones's suggestion. On 14/11/2023 22:11, Andy Shevchenko wrote: > On Tue, Oct 31, 2023 at 09:51:17AM +0800, larry.lai 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 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 > > s/This patch implements/Implement/ > >> manages direction and enable state for those header pins. >> >> Partial support UP boards: > > "for UP" or "supported" (choose one). > >> * UP core + CREX >> * UP core + CRST02 > >> Reported-by: kernel test robot <lkp@intel.com> > > No, this tag can't be applied to the new code. > >> Signed-off-by: Gary Wang <garywang@aaeon.com.tw> >> Signed-off-by: larry.lai <larry.lai@yunjingtech.com> > > Missing Co-developed-by? larry is our consultant for upstream > > ... > >> +config MFD_INTEL_UPBOARD_FPGA > > I believe Intel has nothing to do with this one. The Intel SoC is accompanied > with OEM FPGA, right? we used Intel Altera CPLD MAX V/X for pin mux and provide more driving power for Raspberry Pi compatible HAT pins, but will remove INTEL instead > >> + tristate "Support for the Intel platform foundation kit UP board FPGA" > > Depends on the above this most likely to be updated. > >> + select MFD_CORE > >> + depends on X86 && ACPI > > No COMPILE_TEST? > >> + help >> + Select this option to enable the Intel AAEON UP and UP^2 on-board FPGA. > > Intel is Intel. > AAEON is part of ASUS. > > They never been part of Intel AFAICT. > >> + This is core driver for the UP board that implements certain (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. > > ... > >> +obj-$(CONFIG_MFD_INTEL_UPBOARD_FPGA) += upboard-fpga.o > > Just drop INTEL_ > > ... > >> + * UP Board control CPLD/FPGA to provide more GPIO driving power >> + * also provide CPLD LEDs and pin mux function >> + * recognize HID AANT0F00 ~ AAANT0F04 in ACPI name space > > This needs a bit of English grammar / punctuation update... > > ... > >> +#include <linux/acpi.h> > > How is this being used? Perhaps you need mod_devicetable.h and property.h > instead (see below). > >> +#include <linux/dmi.h> > > Unused. > >> +#include <linux/gpio/consumer.h> > >> +#include <linux/kernel.h> > > What this header is for? Perhaps you meant array_size.h and other(s)? > >> +#include <linux/leds.h> >> +#include <linux/mfd/core.h> >> +#include <linux/mfd/upboard-fpga.h> >> +#include <linux/module.h> >> +#include <linux/regmap.h> > > ... > >> +/* >> + * read CPLD register on custom protocol >> + * send clock and addr bit in strobe and datain pins then read from dataout pin >> + */ > > As per above, seems like all your comments need to be updated to follow some > English language rules... > > ... > >> +static int upboard_cpld_read(void *context, unsigned int reg, unsigned int *val) >> +{ >> + struct upboard_fpga * const fpga = context; >> + int i; >> + >> + /* clear to start new transcation */ >> + gpiod_set_value(fpga->clear_gpio, 0); > > No wait? > >> + gpiod_set_value(fpga->clear_gpio, 1); >> + >> + reg |= UPFPGA_READ_FLAG; >> + >> + /* send clock and data from strobe & datain */ >> + for (i = UPFPGA_ADDRESS_SIZE; i >= 0; i--) { >> + gpiod_set_value(fpga->strobe_gpio, 0); >> + gpiod_set_value(fpga->datain_gpio, !!(reg & BIT(i))); >> + gpiod_set_value(fpga->strobe_gpio, 1); >> + } >> + >> + gpiod_set_value(fpga->strobe_gpio, 0); >> + *val = 0; >> + >> + /* read from dataout */ >> + for (i = UPFPGA_REGISTER_SIZE - 1; i >= 0; i--) { >> + gpiod_set_value(fpga->strobe_gpio, 1); > > No wait? > >> + gpiod_set_value(fpga->strobe_gpio, 0); >> + *val |= gpiod_get_value(fpga->dataout_gpio) << i; >> + } >> + >> + gpiod_set_value(fpga->strobe_gpio, 1); >> + >> + return 0; >> +} > > This looks like SPI bitbang. Can you utilize that driver to do this for you? the protocol is defined by ourselves using gpio pins to communication with CPLD 16bit or 24bit data(different CPLD and firmware). > > ... > >> +static struct upboard_led_data upboard_gpio_led_data[] = { >> + { .bit = 0, .colour = "gpio" }, >> +}; >> + >> +/* 3 LEDs controlled by CPLD */ >> +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[] = { >> + MFD_CELL_NAME("upboard-pinctrl"), >> + MFD_CELL_BASIC("upboard-led", NULL, &upboard_up_led_data[0], >> + sizeof(*upboard_up_led_data), 0), >> + MFD_CELL_BASIC("upboard-led", NULL, &upboard_up_led_data[1], >> + sizeof(*upboard_up_led_data), 1), >> + MFD_CELL_BASIC("upboard-led", NULL, &upboard_up_led_data[2], >> + sizeof(*upboard_up_led_data), 2), >> + MFD_CELL_BASIC("upboard-led", NULL, &upboard_gpio_led_data[0], >> + sizeof(*upboard_gpio_led_data), 0), >> +}; > > Why is not using LED framework? CPLD has 3~4 pins to control LED, read/write CPLD to control these pins. > > ... > >> +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[] = { >> + MFD_CELL_NAME("upboard-pinctrl"), >> + MFD_CELL_BASIC("upboard-led", NULL, &upboard_up2_led_data[0], >> + sizeof(*upboard_up2_led_data), 0), >> + MFD_CELL_BASIC("upboard-led", NULL, &upboard_up2_led_data[1], >> + sizeof(*upboard_up2_led_data), 1), >> + MFD_CELL_BASIC("upboard-led", NULL, &upboard_up2_led_data[2], >> + sizeof(*upboard_up2_led_data), 2), >> + MFD_CELL_BASIC("upboard-led", NULL, &upboard_up2_led_data[3], >> + sizeof(*upboard_up2_led_data), 3), >> + MFD_CELL_BASIC("upboard-led", NULL, &upboard_gpio_led_data[0], >> + sizeof(*upboard_gpio_led_data), 0), >> +}; > > Ditto. > > ... > >> +static int __init upboard_cpld_gpio_init(struct upboard_fpga *fpga) >> +{ >> + enum gpiod_flags flags = fpga->uninitialised ? GPIOD_OUT_LOW : GPIOD_ASIS; > >> + /* >> + * 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. >> + * Reset GPIO defaults HIGH, get GPIO and set to LOW, then set back to >> + * HIGH as a pulse. >> + */ > > So... > >> + if (fpga->uninitialised) { >> + fpga->reset_gpio = devm_gpiod_get(fpga->dev, "reset", GPIOD_OUT_LOW); > > ...make it _optional() and use GPIOD_ASIS. > >> + if (IS_ERR(fpga->reset_gpio)) >> + return PTR_ERR(fpga->reset_gpio); > >> + gpiod_set_value(fpga->reset_gpio, RESET_DEVICE); > > And with gpiod_direction_output() it may be conditionally called. > >> + } > >> + gpiod_set_value(fpga->enable_gpio, ENABLE_DEVICE); > >> + fpga->uninitialised = false; > > How this flag is anyhow useful? Are you expecting the __init marked function to > be called twice? > > Oh, it seems even __init is wrong here... > >> + return 0; >> +} > > ... > >> +static const struct acpi_device_id upboard_fpga_acpi_match[] = { >> + { "AANT0000", AANT0000_ID }, >> + { "AANT0F00", AANT0F00_ID }, >> + { "AANT0F01", AANT0F01_ID }, >> + { "AANT0F02", AANT0F02_ID }, >> + { "AANT0F03", AANT0F03_ID }, >> + { "AANT0F04", AANT0F04_ID }, >> + { } >> +}; >> +MODULE_DEVICE_TABLE(acpi, upboard_fpga_acpi_match); > > Move this closer to its real user (struct platform_driver below). > > ... > >> +static int __init upboard_fpga_probe(struct platform_device *pdev) > > How comes it's marked with __init?! Have you tested it? > > ... > >> + id = acpi_match_device(upboard_fpga_acpi_match, dev); >> + if (!id) >> + return -ENODEV; > > No, use device_get_match_data() from property.h. > > ... > >> + switch (id->driver_data) { >> + case AANT0F00_ID: >> + cpld_config = &upboard_up_regmap_config; >> + cells = upboard_up_mfd_cells; >> + ncells = ARRAY_SIZE(upboard_up_mfd_cells); >> + break; >> + case AANT0F01_ID: >> + cpld_config = &upboard_up2_regmap_config; >> + cells = upboard_up2_mfd_cells; >> + ncells = ARRAY_SIZE(upboard_up2_mfd_cells); >> + break; >> + case AANT0F02_ID: >> + cpld_config = &upboard_up_regmap_config; >> + cells = upboard_up_mfd_cells; >> + ncells = ARRAY_SIZE(upboard_up_mfd_cells); >> + break; >> + case AANT0F03_ID: >> + cpld_config = &upboard_up2_regmap_config; >> + cells = upboard_up_mfd_cells; >> + ncells = ARRAY_SIZE(upboard_up_mfd_cells); >> + break; >> + case AANT0F04_ID: >> + cpld_config = &upboard_up_regmap_config; >> + cells = upboard_up_mfd_cells; >> + ncells = ARRAY_SIZE(upboard_up_mfd_cells); >> + break; >> + case AANT0000_ID: >> + default: >> + cpld_config = &upboard_up_regmap_config; >> + cells = upboard_pinctrl_cells; >> + ncells = ARRAY_SIZE(upboard_pinctrl_cells); >> + break; >> + } > > Drop this and make a custom structure which will be part of the driver data, > let's call it struct upboard_info. When it's done, you will simply have > to access constant info structure whenever you want to. > > ... > >> + platform_set_drvdata(pdev, ddata); > > How is this being used? > > ... > >> +enum upcpld_ids { >> + AANT0000_ID = 255, > > Why not to start from 0? > >> + AANT0F00_ID = 0, >> + AANT0F01_ID = 1, >> + AANT0F02_ID = 2, >> + AANT0F03_ID = 3, >> + AANT0F04_ID = 4, >> +}; > > ... > >> +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, > > No comma for the termination. > >> +}; > > ... > > Also, please split by models, first you add a driver with a single board > support and each new board addition is done in a separate change. the driver design is based on CPLD, the block diagram showing the dependence, pinctrl-upboard driver will check each models from DMI. ------------------------- ------------ | Intel SOC,1.8V | --- |ADC Chip | native driver | GPIO/I2C/SPI/UART/PWM | |SPI/I2C | ------------------------ ------------ | | -------------------------------------- | CPLD/FPGA Driver | upboard-fpga CPLD control drv | provide more GPIO driving power | register leds-upboard | HAT 40 pin mux function | register pinctrl-upboard ------------------------------------- | | -------- ---------------------------- |3 or 4| | HAT 40 pins, 3.3V | leds-upboard | Leds | |GPIO/ADC/I2C/SPI/UART/PWM | pinctrl-upboard -------- ---------------------------- > Best Regards, Larry
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index abb58ab1a1a4..1041e937fc7a 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_INTEL_UPBOARD_FPGA + tristate "Support for the Intel platform foundation kit 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. + This is core driver for the UP board that implements certain (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..8374a05f6f43 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_INTEL_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..c40d35ace24e --- /dev/null +++ b/drivers/mfd/upboard-fpga.c @@ -0,0 +1,404 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * UP Board control CPLD/FPGA to provide more GPIO driving power + * also provide CPLD LEDs and pin mux function + * recognize HID AANT0F00 ~ AAANT0F04 in ACPI name space + * + * Copyright (c) AAEON. All rights reserved. + * + * Author: Gary Wang <garywang@aaeon.com.tw> + */ + +#include <linux/acpi.h> +#include <linux/dmi.h> +#include <linux/gpio/consumer.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/regmap.h> + +#define RESET_DEVICE 1 +#define ENABLE_DEVICE 1 + +#define AAEON_MANUFACTURER_ID 0x01 +#define SUPPORTED_FW_MAJOR 0x0 +#define MENUFACTURER_ID_MASK GENMASK(7, 0) + +#define FIRMWARE_ID_BUILD_OFFSET 12 +#define FIRMWARE_ID_MAJOR_OFFSET 8 +#define FIRMWARE_ID_MINOR_OFFSET 4 +#define FIRMWARE_ID_PATCH_OFFSET 0 +#define FIRMWARE_ID_MASK GENMASK(3, 0) + +/* + * read CPLD register on custom protocol + * send clock and addr bit in strobe and datain pins then read from dataout pin + */ +static int upboard_cpld_read(void *context, unsigned int reg, unsigned int *val) +{ + struct upboard_fpga * const fpga = context; + int i; + + /* clear to start new transcation */ + gpiod_set_value(fpga->clear_gpio, 0); + gpiod_set_value(fpga->clear_gpio, 1); + + reg |= UPFPGA_READ_FLAG; + + /* send clock and data from strobe & datain */ + for (i = UPFPGA_ADDRESS_SIZE; i >= 0; i--) { + gpiod_set_value(fpga->strobe_gpio, 0); + gpiod_set_value(fpga->datain_gpio, !!(reg & BIT(i))); + gpiod_set_value(fpga->strobe_gpio, 1); + } + + gpiod_set_value(fpga->strobe_gpio, 0); + *val = 0; + + /* read from dataout */ + 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; +} + +/* + * write CPLD register on custom protocol + * send clock and addr bit in strobe and datain pins then write to datain pin + */ +static int upboard_cpld_write(void *context, unsigned int reg, unsigned int val) +{ + struct upboard_fpga * const fpga = context; + int i; + + /* clear to start new transcation */ + gpiod_set_value(fpga->clear_gpio, 0); + gpiod_set_value(fpga->clear_gpio, 1); + + /* send clock and data from strobe & datain */ + for (i = UPFPGA_ADDRESS_SIZE; i >= 0; i--) { + gpiod_set_value(fpga->strobe_gpio, 0); + gpiod_set_value(fpga->datain_gpio, !!(reg & BIT(i))); + gpiod_set_value(fpga->strobe_gpio, 1); + } + + gpiod_set_value(fpga->strobe_gpio, 0); + + /* write to datain pin */ + for (i = UPFPGA_REGISTER_SIZE - 1; i >= 0; i--) { + gpiod_set_value(fpga->datain_gpio, !!(val & BIT(i))); + gpiod_set_value(fpga->strobe_gpio, 1); + gpiod_set_value(fpga->strobe_gpio, 0); + } + + gpiod_set_value(fpga->strobe_gpio, 1); + + return 0; +} + +static const struct regmap_range upboard_up_readable_ranges[] = { + regmap_reg_range(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_cpld_read, + .reg_write = upboard_cpld_write, + .fast_io = false, + .cache_type = REGCACHE_NONE, + .rd_table = &upboard_up_readable_table, + .wr_table = &upboard_up_writable_table, +}; + +static struct upboard_led_data upboard_gpio_led_data[] = { + { .bit = 0, .colour = "gpio" }, +}; + +/* 3 LEDs controlled by CPLD */ +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[] = { + MFD_CELL_NAME("upboard-pinctrl"), + MFD_CELL_BASIC("upboard-led", NULL, &upboard_up_led_data[0], + sizeof(*upboard_up_led_data), 0), + MFD_CELL_BASIC("upboard-led", NULL, &upboard_up_led_data[1], + sizeof(*upboard_up_led_data), 1), + MFD_CELL_BASIC("upboard-led", NULL, &upboard_up_led_data[2], + sizeof(*upboard_up_led_data), 2), + MFD_CELL_BASIC("upboard-led", NULL, &upboard_gpio_led_data[0], + sizeof(*upboard_gpio_led_data), 0), +}; + +/* UP Squared 6000 EHL board */ +static const struct mfd_cell upboard_pinctrl_cells[] = { + MFD_CELL_BASIC("upboard-pinctrl", NULL, &upboard_gpio_led_data[0], + sizeof(*upboard_up_led_data), 0), +}; + +/* 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_cpld_read, + .reg_write = upboard_cpld_write, + .fast_io = false, + .cache_type = REGCACHE_NONE, + .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[] = { + MFD_CELL_NAME("upboard-pinctrl"), + MFD_CELL_BASIC("upboard-led", NULL, &upboard_up2_led_data[0], + sizeof(*upboard_up2_led_data), 0), + MFD_CELL_BASIC("upboard-led", NULL, &upboard_up2_led_data[1], + sizeof(*upboard_up2_led_data), 1), + MFD_CELL_BASIC("upboard-led", NULL, &upboard_up2_led_data[2], + sizeof(*upboard_up2_led_data), 2), + MFD_CELL_BASIC("upboard-led", NULL, &upboard_up2_led_data[3], + sizeof(*upboard_up2_led_data), 3), + MFD_CELL_BASIC("upboard-led", NULL, &upboard_gpio_led_data[0], + sizeof(*upboard_gpio_led_data), 0), +}; + +static int __init upboard_cpld_gpio_init(struct upboard_fpga *fpga) +{ + enum gpiod_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. + * Reset GPIO defaults HIGH, get GPIO and set to LOW, then set back to + * HIGH as a pulse. + */ + 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, RESET_DEVICE); + } + + gpiod_set_value(fpga->enable_gpio, ENABLE_DEVICE); + fpga->uninitialised = false; + + return 0; +} + +/* This function is for debugging with user for showing firmware information. */ +static int __init upboard_fpga_verify_device(struct upboard_fpga *fpga) +{ + 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 & MENUFACTURER_ID_MASK; + if (manufacturer_id != AAEON_MANUFACTURER_ID) { + dev_err(fpga->dev, + "Manufacturer ID 0x%02x not supported\n", + manufacturer_id); + + return -ENODEV; + } + + ret = regmap_read(fpga->regmap, UPFPGA_REG_FIRMWARE_ID, &firmware_id); + if (ret) + return ret; + + build = (firmware_id >> FIRMWARE_ID_BUILD_OFFSET) & FIRMWARE_ID_MASK; + major = (firmware_id >> FIRMWARE_ID_MAJOR_OFFSET) & FIRMWARE_ID_MASK; + minor = (firmware_id >> FIRMWARE_ID_MINOR_OFFSET) & FIRMWARE_ID_MASK; + patch = (firmware_id >> FIRMWARE_ID_PATCH_OFFSET) & FIRMWARE_ID_MASK; + + if (major != SUPPORTED_FW_MAJOR) { + dev_err(fpga->dev, "Manufacturer ID 0x%02x not supported\n", 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[] = { + { "AANT0000", AANT0000_ID }, + { "AANT0F00", AANT0F00_ID }, + { "AANT0F01", AANT0F01_ID }, + { "AANT0F02", AANT0F02_ID }, + { "AANT0F03", AANT0F03_ID }, + { "AANT0F04", AANT0F04_ID }, + { } +}; +MODULE_DEVICE_TABLE(acpi, upboard_fpga_acpi_match); + +static int __init upboard_fpga_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct upboard_fpga *ddata; + const struct acpi_device_id *id; + static const struct regmap_config *cpld_config; + static const struct mfd_cell *cells; + static size_t ncells; + int ret; + + id = acpi_match_device(upboard_fpga_acpi_match, dev); + if (!id) + return -ENODEV; + + switch (id->driver_data) { + case AANT0F00_ID: + cpld_config = &upboard_up_regmap_config; + cells = upboard_up_mfd_cells; + ncells = ARRAY_SIZE(upboard_up_mfd_cells); + break; + case AANT0F01_ID: + cpld_config = &upboard_up2_regmap_config; + cells = upboard_up2_mfd_cells; + ncells = ARRAY_SIZE(upboard_up2_mfd_cells); + break; + case AANT0F02_ID: + cpld_config = &upboard_up_regmap_config; + cells = upboard_up_mfd_cells; + ncells = ARRAY_SIZE(upboard_up_mfd_cells); + break; + case AANT0F03_ID: + cpld_config = &upboard_up2_regmap_config; + cells = upboard_up_mfd_cells; + ncells = ARRAY_SIZE(upboard_up_mfd_cells); + break; + case AANT0F04_ID: + cpld_config = &upboard_up_regmap_config; + cells = upboard_up_mfd_cells; + ncells = ARRAY_SIZE(upboard_up_mfd_cells); + break; + case AANT0000_ID: + default: + cpld_config = &upboard_up_regmap_config; + cells = upboard_pinctrl_cells; + ncells = ARRAY_SIZE(upboard_pinctrl_cells); + break; + } + + ddata = devm_kzalloc(dev, sizeof(*ddata), GFP_KERNEL); + if (!ddata) + return -ENOMEM; + + platform_set_drvdata(pdev, ddata); + ddata->dev = dev; + ddata->regmap = devm_regmap_init(dev, NULL, ddata, cpld_config); + if (IS_ERR(ddata->regmap)) + return PTR_ERR(ddata->regmap); + + ret = upboard_cpld_gpio_init(ddata); + if (ret) { + /* Not FPGA firmware, abort FPGA GPIO initialize process */ + dev_warn(dev, "Failed to initialize CPLD common GPIOs: %d", ret); + } else { + upboard_fpga_verify_device(ddata); + } + + return devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, + cells, + ncells, + NULL, 0, NULL); +} + +static struct platform_driver upboard_fpga_driver = { + .driver = { + .name = "upboard-cpld", + .acpi_match_table = upboard_fpga_acpi_match, + }, +}; +module_platform_driver_probe(upboard_fpga_driver, upboard_fpga_probe); + +MODULE_AUTHOR("Gary Wang <garywang@aaeon.com.tw>"); +MODULE_DESCRIPTION("UP Board CPLD/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..6e7aa3c9cd24 --- /dev/null +++ b/include/linux/mfd/upboard-fpga.h @@ -0,0 +1,62 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * UP Board CPLD/FPGA driver + * + * Copyright (c) AAEON. All rights reserved. + * + * Author: Gary Wang <garywang@aaeon.com.tw> + */ + +#ifndef __MFD_UPBOARD_FPGA_H +#define __MFD_UPBOARD_FPGA_H + +/* CPLD/FPGA protocol version */ +#define UPFPGA_PROTOCOL_V1_HRV 1 +#define UPFPGA_PROTOCOL_V2_HRV 2 + +#define UPFPGA_ADDRESS_SIZE 7 +#define UPFPGA_REGISTER_SIZE 16 + +#define UPFPGA_READ_FLAG BIT(UPFPGA_ADDRESS_SIZE) + +enum upcpld_ids { + AANT0000_ID = 255, + AANT0F00_ID = 0, + AANT0F01_ID = 1, + AANT0F02_ID = 2, + AANT0F03_ID = 3, + AANT0F04_ID = 4, +}; + +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 /* __MFD_UPBOARD_FPGA_H */