回覆: [RFC 1/3] mfd: Add support for UP board CPLD/FPGA
From: Larry Lai
Date: Tue Apr 25 2023 - 11:28:33 EST
Dear Andy,
Thank you for spending time to review this code, please kindly check the following comments with “>>>” beginning.
Some of issues we will fix in new RFC_230426, some may need you give us more examples or comments.
Best Regards,
Larry Lai
寄件者: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
日期: 星期四, 2022年12月8日 上午5:10
收件者: Larry Lai <larry.lai@xxxxxxxxxxxxxxx>
副本: lee@xxxxxxxxxx <lee@xxxxxxxxxx>, linus.walleij@xxxxxxxxxx <linus.walleij@xxxxxxxxxx>, pavel@xxxxxx <pavel@xxxxxx>, linux-kernel@xxxxxxxxxxxxxxx <linux-kernel@xxxxxxxxxxxxxxx>, linux-gpio@xxxxxxxxxxxxxxx <linux-gpio@xxxxxxxxxxxxxxx>, linux-leds@xxxxxxxxxxxxxxx <linux-leds@xxxxxxxxxxxxxxx>, GaryWang@xxxxxxxxxxxx <GaryWang@xxxxxxxxxxxx>, Musa Lin <musa.lin@xxxxxxxxxxxxxxx>, Jack Chang <jack.chang@xxxxxxxxxxxxxxx>, Noah Hung <noah.hung@xxxxxxxxxxxxxxx>, Javier Arteaga <javier@xxxxxxxxxx>, Nicola Lunghi <nicola.lunghi@xxxxxxxxxx>
主旨: Re: [RFC 1/3] mfd: Add support for UP board CPLD/FPGA
On Thu, Dec 08, 2022 at 12:33:57AM +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 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
...
> +#include <linux/acpi.h>
> +#include <linux/dmi.h>
> +#include <linux/gpio.h>
I'm not sure if you read my previous emails regarding the topic.
This header must not be in the new code.
>>> fixed.
> +#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>
Missing bits.h and err.h at least.
>>> fixed.
...
> +struct upboard_fpga_data {
> + const struct regmap_config *regmapconf;
No need to repeat regmap twice.
>>> fixed.
> + const struct mfd_cell *cells;
> + size_t ncells;
> +};
...
> +#define MENUFACTURER_ID_MASK 0xFF
GENMASK()?
>>> fixed.
...
> +#define FIRMWARE_ID_MASK 0xF
Ditto.
>>> fixed.
...
> +/* Apollo Lake GPIO pin number mapping to FPGA LED */
> +#define APL_GPIO_218 507
No way. It should be addressed as GPIO chip reference and relative pin
(or GPIO, whichever suits better for your purposes) number.
>>> This section has been removed in new RFC patch, please ignore it.
...
> +/* For UP Board Series FPGA register read/write protocols */
> +/* EMUTEX specs: */
> +/* D0 D1 D2 D3 D4 D5 D6 D7 D8 D9 .... D22 D23 */
> +/* [RW][ address ][ DATA ] */
> +
> +/* Read Sequence: */
> +/* ___ ____________________________________________________ _________*/
> +/* clr: \_/ <--low-pulse does start the write-readback \_/<--start */
> +/* sequence with partital reset of internal new sequence*/
> +/* registers but the CONF-REG. */
> +/* ____________________________________________________________________*/
> +/* rst: _/ _ _ _ _ _ _ __ __ __ _ */
> +/* stb: STB#1->_/1\_/2\_/3\_...._/7\_/8\_/9\_/10\_..../23\_/24\_/<-STB#25 edge*/
> +/* is needed */
> +/* to ACK */
> +/* (D0 - D7 stb rising latch) */
> +/* data_in: D0 D1 D2 .... D6 D7 don't ........ care(DC) */
> +/* data_out: don't ...........care(DC) D8 D9 .... D22 D23 */
> +/* (D8 - D23 stb falling latch) */
> +/* flag_Read: _________...._________ */
> +/* __DC_ ____________...._________/ \_ */
> +/* counter: */
> +/* [00]DC[00][01][02] ............[08][9][10]............[24][00] */
> +/* CONF-REG: */
> +/* [00] [ CONF-REG ] */
> +/* wreg: */
> +/* [00]DC[00][ wreg=SHFT(wreg) ][ADR][DATA][wreg=SHFT(wreg] */
> +/* wreg2: */
> +/* [ (COPY)=ADDR ] */
This has too many /* */ and TABs vs space mix... Please, fix it.
>>> fixed.
Is it SPI 24-bit bit-banging? Why spi-gpio can't be utilized for it?
>>> The UP board FPGA protocol (stb, clr, rst, in, out) is different with SPI 24-bit banging (CS, CLK, MOST, MOSO).
...
> +/* Write Sequence: */
> +/* ___ ____________________________________________________ _________*/
> +/* clr: \_/ <--low-pulse does start the write-readback \_/<--start */
> +/* sequence with partital reset of internal new sequence*/
> +/* registers but the CONF-REG. */
> +/* ____________________________________________________________________*/
> +/* rst: _/ _ _ _ _ _ _ __ __ __ _ */
> +/* stb: STB#1->_/1\_/2\_/3\_...._/7\_/8\_/9\_/10\_..../23\_/24\_/<-STB#25 edge*/
> +/* is needed */
> +/* to ACK */
> +/* (D0 - D23 stb rising latch) */
> +/* data_in: D0 D1 D2 .... D6 D7 D8 D9 .... D22 D23 */
> +/* data_out: don't ................................care (DC) */
> +/* flag_Read: */
> +/* __DC_ ____________....__________________________________ */
> +/* counter: */
> +/* [00]DC[00][01][02] ............[08][9][10]............[24][00] */
> +/* wreg: */
> +/* [00]DC[00][wreg=SHFT(wreg)&dat_in ][SHFT(wreg)&dat_in][DAT] */
> +/* wreg2: */
> +/* [ (COPY)=ADDR ] */
> +/* CONF-REG: */
> +/* [00] [ CONF-REG = OLD VALUE ][CONF-REG=DAT]*/
Same comments as per above.
>>> fixed.
...
> + gpiod_set_value(fpga->datain_gpio, (reg >> i) & 0x1);
!!(reg & BIT(i))
>>> fixed.
...
> + gpiod_set_value(fpga->datain_gpio, (val >> i) & 0x1);
Ditto.
>>> fixed.
But see above.
...
> +static struct gpio_led upboard_gpio_leds[] = {
> + {
> + .name = "upboard:blue:",
> + .gpio = APL_GPIO_218,
You must understand that it won't work with dynamic GPIO bases which will be
enabled in v6.2-rc1. And even in general it must not be like this.
>>> fixed.
> + .default_state = LEDS_GPIO_DEFSTATE_KEEP,
> + },
> +};
...
> + enum gpiod_flags flags;
> +
> + flags = fpga->uninitialised ? GPIOD_OUT_LOW : GPIOD_ASIS;
Can be united.
>>> fixed.
...
> + /*
> + * 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);
No sleep for the hardware to be really reset?
>>> This is hardware reset pin, so there's no need the sleep wait.
> + gpiod_set_value(fpga->reset_gpio, 1);
> + }
> +/*
> + * MFD upboard-fpga is acpi driver and can recognize the AANT ID from different
ACPI
>>> fixed.
> + * kind of upboards. We get the led gpio initialized information from this
LED GPIO
>>> fixed.
> + * then add led-upboard driver.
> + */
...
> + int blue_gpio = -1, yellow_gpio = -1, green_gpio = -1, red_gpio = -1;
NAK.
>>> fixed.
...
> + blue_gpio = desc_to_gpio(desc);
NAK.
>>> fixed.
...
> + yellow_gpio = desc_to_gpio(desc);
NAK.
>>> fixed.
...
> + green_gpio = desc_to_gpio(desc);
NAK.
>>> fixed.
...
> + red_gpio = desc_to_gpio(desc);
NAK.
>>> fixed.
...
> +/*
> + * Refer https://www.kernel.org/doc/htmldocs/writing_musb_glue_layer/device-platform-data.html,
> + * the id field could be set to -1 (equivalent to PLATFORM_DEVID_NONE),
> + * -2 (equivalent to PLATFORM_DEVID_AUTO) or start with 0 for the first
> + * device of this kind if we want a specific id number.
> + */
Useless comment. Just use the proper definition.
>>> remove useless comment and use PLATFORM_DEVID_AUTO proper definition.
> + if (devm_mfd_add_devices(fpga->dev, 0,
> + upboard_gpio_led_cells,
> + ARRAY_SIZE(upboard_gpio_led_cells),
> + NULL, 0, NULL)) {
> + dev_info(fpga->dev, "Failed to add GPIO leds");
> + }
ret = ...(...);
if (ret)
dev_warn(...);
>>> fixed.
...
> + /* get fpga/EC protocol hardware version */
> + acpi_evaluate_integer(ACPI_HANDLE(&pdev->dev), "_HRV", NULL, &hrv);
No error check?
>>> This section has been removed in new RFC patch, please ignore it.
...
> + system_id = dmi_first_match(upboard_dmi_table);
> + if (system_id)
> + quirks = (unsigned long)system_id->driver_data;
> +
> + if (hrv == UPFPGA_PROTOCOL_V1_HRV &&
> + (quirks & UPFPGA_QUIRK_HRV1_IS_PROTO2))
> + hrv = UPFPGA_PROTOCOL_V2_HRV;
Maybe it's easier to provide driver data?
>>> This section has been removed in new RFC patch, please ignore it.
...
> + fpga_data = (const struct upboard_fpga_data *) id->driver_data;
Use device_get_match_data().
>>> This section has been removed in new RFC patch, please ignore it.
...
> + if (quirks & UPFPGA_QUIRK_UNINITIALISED) {
> + dev_info(&pdev->dev, "FPGA not initialised by this BIOS");
dev_warn()?
>>> This section has been removed in new RFC patch, please ignore it.
> + fpga->uninitialised = true;
> + }
...
> + dev_set_drvdata(&pdev->dev, fpga);
platform_set_drvdata().
>>> fixed.
> + fpga->dev = &pdev->dev;
> + fpga->regmap = devm_regmap_init(&pdev->dev, NULL,
> + fpga, fpga_data->regmapconf);
Can be one line and you can actually have
struct device *dev = &pdev->dev;
at the top of the function.
>>> fixed.
> + fpga->regmapconf = fpga_data->regmapconf;
Why is it done if you know that error might happen?
>>> This section has been removed in new RFC patch, please ignore it.
> + if (IS_ERR(fpga->regmap))
> + return PTR_ERR(fpga->regmap);
...
> + /* gpio leds initialize */
GPIO LEDs
>>> fixed.
...
> + ret = devm_mfd_add_devices(&pdev->dev, 0,
Use proper definition.
>>> This section has been removed in new RFC patch, please ignore it.
> + upboard_gpio_led_cells,
> + ARRAY_SIZE(upboard_gpio_led_cells),
> + NULL, 0, NULL);
> + dev_err(&pdev->dev, "Failed to add GPIO leds");
> + return ret;
return dev_err_probe();
>>> This section has been removed in new RFC patch, please ignore it.
> + }
> + }
+ blank line.
>>> fixed.
> + return devm_mfd_add_devices(&pdev->dev, 0,
Use proper definition.
>>> fixed by using PLATFORM_DEVID_AUTO definition.
> + fpga_data->cells,
> + fpga_data->ncells,
> + NULL, 0, NULL);
> +}
...
Move ACPI ID table here, it's not needed to have it upper.
>>> Could you kindly explain more and give some examples?
> +static struct platform_driver upboard_fpga_driver = {
> + .driver = {
> + .name = "upboard-fpga",
> + .acpi_match_table = upboard_fpga_acpi_match,
> + },
> +};
...
The header file missing several forward declarations and inclusions, like
>>> fixed.
#include <linux/types.h>
struct regmap;
--
With Best Regards,
Andy Shevchenko