From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
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@aaeon.com.tw,
musa.lin@yunjingtech.com, jack.chang@yunjingtech.com,
noah.hung@yunjingtech.com, Javier Arteaga <javier@emutex.com>,
Nicola Lunghi <nicola.lunghi@emutex.com>
Subject: Re: [RFC 1/3] mfd: Add support for UP board CPLD/FPGA
Date: Wed, 7 Dec 2022 23:10:08 +0200 [thread overview]
Message-ID: <Y5EBMCWEPgCFPnMj@smile.fi.intel.com> (raw)
In-Reply-To: <20221207163359.26564-2-larry.lai@yunjingtech.com>
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.
> +#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.
...
> +struct upboard_fpga_data {
> + const struct regmap_config *regmapconf;
No need to repeat regmap twice.
> + const struct mfd_cell *cells;
> + size_t ncells;
> +};
...
> +#define MENUFACTURER_ID_MASK 0xFF
GENMASK()?
...
> +#define FIRMWARE_ID_MASK 0xF
Ditto.
...
> +/* 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.
...
> +/* 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.
Is it SPI 24-bit bit-banging? Why spi-gpio can't be utilized for it?
...
> +/* 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.
...
> + gpiod_set_value(fpga->datain_gpio, (reg >> i) & 0x1);
!!(reg & BIT(i))
...
> + gpiod_set_value(fpga->datain_gpio, (val >> i) & 0x1);
Ditto.
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.
> + .default_state = LEDS_GPIO_DEFSTATE_KEEP,
> + },
> +};
...
> + enum gpiod_flags flags;
> +
> + flags = fpga->uninitialised ? GPIOD_OUT_LOW : GPIOD_ASIS;
Can be united.
...
> + /*
> + * 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?
> + gpiod_set_value(fpga->reset_gpio, 1);
> + }
> +/*
> + * MFD upboard-fpga is acpi driver and can recognize the AANT ID from different
ACPI
> + * kind of upboards. We get the led gpio initialized information from this
LED GPIO
> + * then add led-upboard driver.
> + */
...
> + int blue_gpio = -1, yellow_gpio = -1, green_gpio = -1, red_gpio = -1;
NAK.
...
> + blue_gpio = desc_to_gpio(desc);
NAK.
...
> + yellow_gpio = desc_to_gpio(desc);
NAK.
...
> + green_gpio = desc_to_gpio(desc);
NAK.
...
> + red_gpio = desc_to_gpio(desc);
NAK.
...
> +/*
> + * 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.
> + 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(...);
...
> + /* get fpga/EC protocol hardware version */
> + acpi_evaluate_integer(ACPI_HANDLE(&pdev->dev), "_HRV", NULL, &hrv);
No error check?
...
> + 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?
...
> + fpga_data = (const struct upboard_fpga_data *) id->driver_data;
Use device_get_match_data().
...
> + if (quirks & UPFPGA_QUIRK_UNINITIALISED) {
> + dev_info(&pdev->dev, "FPGA not initialised by this BIOS");
dev_warn()?
> + fpga->uninitialised = true;
> + }
...
> + dev_set_drvdata(&pdev->dev, fpga);
platform_set_drvdata().
> + 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.
> + fpga->regmapconf = fpga_data->regmapconf;
Why is it done if you know that error might happen?
> + if (IS_ERR(fpga->regmap))
> + return PTR_ERR(fpga->regmap);
...
> + /* gpio leds initialize */
GPIO LEDs
...
> + ret = devm_mfd_add_devices(&pdev->dev, 0,
Use proper definition.
> + 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();
> + }
> + }
+ blank line.
> + return devm_mfd_add_devices(&pdev->dev, 0,
Use proper definition.
> + fpga_data->cells,
> + fpga_data->ncells,
> + NULL, 0, NULL);
> +}
...
Move ACPI ID table here, it's not needed to have it upper.
> +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
#include <linux/types.h>
struct regmap;
--
With Best Regards,
Andy Shevchenko
next prev parent reply other threads:[~2022-12-07 21:11 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-12-07 16:33 [RFC 0/3] Add support control UP board CPLD/FPGA pin control larry.lai
2022-12-07 16:33 ` [RFC 1/3] mfd: Add support for UP board CPLD/FPGA larry.lai
2022-12-07 21:10 ` Andy Shevchenko [this message]
2023-04-25 15:28 ` 回覆: " Larry Lai
2022-12-07 16:33 ` [RFC 2/3] pinctrl: Add support pin control " larry.lai
2022-12-07 21:32 ` Andy Shevchenko
2023-04-25 15:28 ` 回覆: " Larry Lai
2022-12-07 22:55 ` kernel test robot
2022-12-07 16:33 ` [RFC 3/3] leds: Add support for UP board CPLD onboard LEDS larry.lai
2022-12-07 21:09 ` [RFC 0/3] Add support control UP board CPLD/FPGA pin control Linus Walleij
2022-12-07 21:33 ` Andy Shevchenko
2022-12-07 21:36 ` Andy Shevchenko
2022-12-07 21:38 ` Andy Shevchenko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Y5EBMCWEPgCFPnMj@smile.fi.intel.com \
--to=andriy.shevchenko@linux.intel.com \
--cc=GaryWang@aaeon.com.tw \
--cc=jack.chang@yunjingtech.com \
--cc=javier@emutex.com \
--cc=larry.lai@yunjingtech.com \
--cc=lee@kernel.org \
--cc=linus.walleij@linaro.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-leds@vger.kernel.org \
--cc=musa.lin@yunjingtech.com \
--cc=nicola.lunghi@emutex.com \
--cc=noah.hung@yunjingtech.com \
--cc=pavel@ucw.cz \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.