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? ... > +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. -- With Best Regards, Andy Shevchenko
