| 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 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. That's too old. Please use latest mainline. 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. > ... > + > +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 boards come with a few FPGA-controlled onboard LEDs: > * UP Board: yellow, green, red > * UP Squared: blue, yellow, green, red > ... > + > +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. ... > +module_platform_driver_probe(upboard_led_driver, upboard_led_probe); > + > +MODULE_AUTHOR("Gary Wang <garywang@aaeon.com.tw>"); > +MODULE_DESCRIPTION("UP Board LED driver"); > +MODULE_LICENSE("GPL v2"); > +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. Thank you. 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