| Message ID | 20221207163359.26564-1-larry.lai@yunjingtech.com |
|---|---|
| Headers | show |
| Series | Add support control UP board CPLD/FPGA pin control | expand |
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@linux.intel.com> 日期: 星期四, 2022年12月8日 上午5:10 收件者: Larry Lai <larry.lai@yunjingtech.com> 副本: lee@kernel.org <lee@kernel.org>, linus.walleij@linaro.org <linus.walleij@linaro.org>, pavel@ucw.cz <pavel@ucw.cz>, linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>, linux-gpio@vger.kernel.org <linux-gpio@vger.kernel.org>, linux-leds@vger.kernel.org <linux-leds@vger.kernel.org>, GaryWang@aaeon.com.tw <GaryWang@aaeon.com.tw>, Musa Lin <musa.lin@yunjingtech.com>, Jack Chang <jack.chang@yunjingtech.com>, Noah Hung <noah.hung@yunjingtech.com>, Javier Arteaga <javier@emutex.com>, Nicola Lunghi <nicola.lunghi@emutex.com> 主旨: 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;