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