mbox series

[V8,0/3] Add support control UP board CPLD/FPGA pin control

Message ID 20231228151544.14408-1-larry.lai@yunjingtech.com
Headers show
Series Add support control UP board CPLD/FPGA pin control | expand

Message

larry.lai Dec. 28, 2023, 3:15 p.m. UTC
The UP board <https://up-board.org/> is the computer board for 
Professional Makers and Industrial Applications. We want to upstream 
the UP board 40-pin GP-bus Kernel driver for giving the users better 
experience on the software release. (not just download from UP board 
github)

These patches are generated from the Linux kernel mainline tag v6.0.

This is the PATCH V8. It fixed and addressed Andy Shevchenko and 
Krzysztof Kozlowski review comments.

larry.lai (3):
  mfd: Add support for UP board CPLD/FPGA
  pinctrl: Add support pin control for UP board CPLD/FPGA
  leds: Add support for UP board CPLD onboard LEDS

 drivers/leds/Kconfig              |   10 +
 drivers/leds/Makefile             |    1 +
 drivers/leds/leds-upboard.c       |  154 ++++
 drivers/mfd/Kconfig               |   12 +
 drivers/mfd/Makefile              |    1 +
 drivers/mfd/upboard-fpga.c        |  364 ++++++++
 drivers/pinctrl/Kconfig           |   15 +
 drivers/pinctrl/Makefile          |    1 +
 drivers/pinctrl/pinctrl-upboard.c | 1309 +++++++++++++++++++++++++++++
 include/linux/mfd/upboard-fpga.h  |   59 ++
 10 files changed, 1926 insertions(+)
 create mode 100644 drivers/leds/leds-upboard.c
 create mode 100644 drivers/mfd/upboard-fpga.c
 create mode 100644 drivers/pinctrl/pinctrl-upboard.c
 create mode 100644 include/linux/mfd/upboard-fpga.h


base-commit: 4fe89d07dcc2804c8b562f6c7896a45643d34b2f

Comments

Krzysztof Kozlowski Dec. 28, 2023, 3:27 p.m. UTC | #1
On 28/12/2023 16:15, 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.
> 

...

> +
> +static const struct acpi_device_id upboard_fpga_acpi_match[] = {
> +	{ "AANT0F00", (kernel_ulong_t)&upboard_up_fpga_data },
> +	{ "AANT0F01", (kernel_ulong_t)&upboard_up2_fpga_data },
> +	{ "AANT0F02", (kernel_ulong_t)&upboard_upcore_crex_fpga_data },
> +	{ "AANT0F03", (kernel_ulong_t)&upboard_upcore_crst02_fpga_data },
> +	{ "AANT0F04", (kernel_ulong_t)&upboard_up_fpga_data },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(acpi, upboard_fpga_acpi_match);
> +
> +static int upboard_fpga_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct upboard_fpga *ddata;
> +	int ret;
> +
> +	ddata = devm_kzalloc(dev, sizeof(ddata), GFP_KERNEL);

This wasn't tested. You allocate size of the pointer.


Best regards,
Krzysztof
Krzysztof Kozlowski Dec. 28, 2023, 3:31 p.m. UTC | #2
On 28/12/2023 16:15, 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.
> 

...

> +#define FIRMWARE_ID_BUILD_OFFSET	12
> +#define FIRMWARE_ID_MAJOR_OFFSET	8
> +#define FIRMWARE_ID_MINOR_OFFSET	4
> +#define FIRMWARE_ID_PATCH_OFFSET	0
> +#define FIRMWARE_ID_MASK		GENMASK(3, 0)
> +
> +/*
> + * read CPLD register on custom protocol
> + * send clock and addr bit in strobe and datain pins then read from dataout pin
> + */
> +static int upboard_cpld_read(void *context, unsigned int reg, unsigned int *val)
> +{
> +	struct upboard_fpga * const fpga = context;

This const does not make sense. Drop it. Unless you wanted to say
something else, like struct is not modified, but then please read C
tutorials. This applies to all your three patches.

Best regards,
Krzysztof
Jack Chang Dec. 29, 2023, 9:18 a.m. UTC | #3
On 2023/12/28 23:31, Krzysztof Kozlowski wrote:
> On 28/12/2023 16:15, 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.

...

>> +static int upboard_cpld_read(void *context, unsigned int reg, unsigned int *val)
>> +{
>> +	struct upboard_fpga * const fpga = context;
> 
> This const does not make sense. Drop it. Unless you wanted to say
> something else, like struct is not modified, but then please read C
> tutorials. This applies to all your three patches.

will do
Jack Chang Dec. 29, 2023, 9:18 a.m. UTC | #4
On 2023/12/28 23:29, Krzysztof Kozlowski wrote:
> On 28/12/2023 16:15, larry.lai wrote:
>> The UP boards come with a few FPGA-controlled onboard LEDs:

...

>> +static int upboard_led_probe(struct platform_device *pdev)
>> +{
>> +	struct upboard_fpga * const cpld = dev_get_drvdata(pdev->dev.parent);
>> +	struct reg_field fldconf = {
>> +		.reg = UPFPGA_REG_FUNC_EN0,
>> +	};
>> +	struct upboard_led_data * const pdata = pdev->dev.platform_data;
> 
> Your const does not make sense. Please read C standard how qualifiers
> are applied.

will do

>> +MODULE_ALIAS("platform:upboard-led");
> 
> Nothing improved here.
> 
> This is a friendly reminder during the review process.
> 
> It seems my or other reviewer's previous comments were not fully
> addressed. Maybe the feedback got lost between the quotes, maybe you
> just forgot to apply it. Please go back to the previous discussion and
> either implement all requested changes or keep discussing them.

the same as pinctrl-upboard doing