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@xxxxxxxxxxxxxxx>
Sent: Tuesday, November 14, 2023 10:11 PM
To: larry.lai <larry.lai@xxxxxxxxxxxxxxx>
Cc: lee@xxxxxxxxxx; linus.walleij@xxxxxxxxxx; pavel@xxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-gpio@xxxxxxxxxxxxxxx; linux-leds@xxxxxxxxxxxxxxx; GaryWang 汪之逸 <GaryWang@xxxxxxxxxxxx>; musa.lin@xxxxxxxxxxxxxxx; jack.chang@xxxxxxxxxxxxxxx; noah.hung@xxxxxxxxxxxxxxx
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@xxxxxxxxx>
No, this tag can't be applied to the new code.
> Signed-off-by: Gary Wang <garywang@xxxxxxxxxxxx>
> Signed-off-by: larry.lai <larry.lai@xxxxxxxxxxxxxxx>
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
