All,
Reply again to plain text format & line-warp and trim agree part as Jones's suggestion,
please let me know if there are still having format issue.
please kindly to check our comments with ">>" beginning.
-----Original Message-----
From: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
Sent: Tuesday, November 14, 2023 10:11 PM
To: larry.lai <larry.lai@xxxxxxxxxxxxxxx>
Cc: lee@xxxxxxxxxx; linus.walleij@xxxxxxxxxx; pavel@xxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-gpio@xxxxxxxxxxxxxxx; linux-leds@xxxxxxxxxxxxxxx; GaryWang 汪之逸 <GaryWang@xxxxxxxxxxxx>; musa.lin@xxxxxxxxxxxxxxx; jack.chang@xxxxxxxxxxxxxxx; noah.hung@xxxxxxxxxxxxxxx
Subject: Re: [PATCH V7 1/3] mfd: Add support for UP board CPLD/FPGA
> Signed-off-by: Gary Wang <garywang@xxxxxxxxxxxx>
> Signed-off-by: larry.lai <larry.lai@xxxxxxxxxxxxxxx>
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
> +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?
>> see below
...
> +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.
>> CPLD has 3~4 pins to control LED, read/write CPLD to control these pins.
> + AANT0F00_ID = 0,
> + AANT0F01_ID = 1,
> + AANT0F02_ID = 2,
> + AANT0F03_ID = 3,
> + AANT0F04_ID = 4,
> +};
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 | kernel native driver
| GPIO/I2C/SPI/UART/PWM | |SPI/I2C |
-------------------------------------- --------------
| |
----------------------------------------------------------
| CPLD/FPGA | ACPI upboard-fpga CPLD control driver
| provide more driving power | register leds-upboard
| HAT pins mux function | register pinctrl-upboard
---------------------------------------------------------
| |
---------- -------------------------------------------
|3 or 4| | HAT 40 pins, 3.3V | leds-upboard led driver
| Leds | |GPIO/ADC/I2C/SPI/UART/PWM| pinctrl-upboard hat pin control driver
---------- -------------------------------------------
--
With Best Regards,
Andy Shevchenko
