Message-ID: <b3629c57-efb6-4459-e931-e160a47bca4c@yunjingtech.com>
Date:   Fri, 1 Dec 2023 16:19:10 +0800
From:   larry <larry.lai@...jingtech.com>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:     lee@...nel.org, linus.walleij@...aro.org, pavel@....cz,
        linux-kernel@...r.kernel.org, linux-gpio@...r.kernel.org,
        linux-leds@...r.kernel.org, GaryWang@...on.com.tw,
        musa.lin@...jingtech.com, jack.chang@...jingtech.com,
        noah.hung@...jingtech.com
Subject: Re: [PATCH V7 1/3] mfd: Add support for UP board CPLD/FPGA

All,
   Sorry, replied Gary's mail again for plain text format & mail client
as Jones's suggestion.


On 14/11/2023 22:11, Andy Shevchenko wrote:
> 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@...el.com>
> 
> No, this tag can't be applied to the new code.
> 
>> Signed-off-by: Gary Wang <garywang@...on.com.tw>
>> Signed-off-by: larry.lai <larry.lai@...jingtech.com>
> 
> 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 Altera CPLD MAX V/X for pin mux and provide more driving
power for Raspberry Pi compatible HAT pins, but will remove INTEL instead

> 
>> +	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?

the protocol is defined by ourselves using 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?

CPLD has 3~4 pins to control LED, read/write CPLD to control these pins.

> 
> ...
> 
>> +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.

the driver design is based on CPLD, the block diagram showing the
dependence, pinctrl-upboard driver will check each models from DMI.

-------------------------     ------------
|    Intel SOC,1.8V     | --- |ADC Chip   |  native driver
| GPIO/I2C/SPI/UART/PWM |     |SPI/I2C    |
------------------------      ------------
            |                      |
--------------------------------------
|        CPLD/FPGA Driver            |   upboard-fpga CPLD control drv
|   provide more GPIO driving power  |   register leds-upboard
|        HAT 40 pin mux function     |   register pinctrl-upboard
-------------------------------------
    |                     |
--------   ----------------------------
|3 or 4|   |    HAT 40 pins, 3.3V     |   leds-upboard
| Leds |   |GPIO/ADC/I2C/SPI/UART/PWM |  pinctrl-upboard
--------   ----------------------------

> 

Best Regards,
Larry

Powered by blists - more mailing lists