On Tue, 4 Oct 2022, Andy Shevchenko wrote:
On Tue, Oct 04, 2022 at 07:37:18AM -0700, matthew.gerlach@xxxxxxxxxxxxxxx wrote:
From: Matthew Gerlach <matthew.gerlach@xxxxxxxxxxxxxxx>
Add a Device Feature List (DFL) bus driver for the Altera
16550 implementation of UART.
...
Reported-by: kernel test robot <lkp@xxxxxxxxx>
https://docs.kernel.org/process/submitting-patches.html?highlight=reported#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes
"The Reported-by tag gives credit to people who find bugs and report them and it
hopefully inspires them to help us again in the future. Please note that if the
bug was reported in private, then ask for permission first before using the
Reported-by tag. The tag is intended for bugs; please do not use it to credit
feature requests."
The kernel test robot did find a bug in my v1 submission. I was missing
the static keyword for a function declaration. Should I remove the tag?
...
+ if (!dfhv1_has_params(dfh_base)) {
+ dev_err(dev, "missing required DFH parameters\n");
+ return -EINVAL;
+ }
Why not use dev_err_probe() everywhere since this is called only at ->probe()
stage?
I wasn't sure if using dev_err_probe() was correct, since the usage is
technically in a different function. Since the code is only called from
->probe(), and it is much cleaner, I'll switch to dev_err_probe()
everywhere
> ...
+ off = dfhv1_find_param(dfh_base, max, DFHv1_PARAM_ID_CLK_FRQ);
+ if (off < 0) {
+ dev_err(dev, "missing CLK_FRQ param\n");
+ return -EINVAL;
Why error code is being shadowed?
Definitely a mistake.
+ }
...
+ off = dfhv1_find_param(dfh_base, max, DFHv1_PARAM_ID_FIFO_LEN);
+ if (off < 0) {
+ dev_err(dev, "missing FIFO_LEN param\n");
+ return -EINVAL;
Ditto.
+ }
...
+ off = dfhv1_find_param(dfh_base, max, DFHv1_PARAM_ID_REG_LAYOUT);
+ if (off < 0) {
+ dev_err(dev, "missing REG_LAYOUT param\n");
+ return -EINVAL;
+ }
Ditto.
...
+ dev_dbg(dev, "UART_LAYOUT_ID width %lld shift %d\n",
+ FIELD_GET(DFHv1_PARAM_ID_REG_WIDTH, v), (int)uart->port.regshift);
Casting in printf() in kernel in 99% shows the wrong specifier in use. Try to
select the best suitable one.
I will remove the casting and find the correct format specifier.
...
+ dfh_base = devm_ioremap_resource(dev, &dfl_dev->mmio_res);
+ if (IS_ERR(dfh_base))
+ return PTR_ERR(dfh_base);
+
+ res_size = resource_size(&dfl_dev->mmio_res);
+
+ ret = dfl_uart_get_params(dev, dfh_base, res_size, &uart);
+ devm_iounmap(dev, dfh_base);
+ devm_release_mem_region(dev, dfl_dev->mmio_res.start, res_size);
If it's temporary, may be you shouldn't even consider devm_ioremap_resource()
to begin with? The devm_* release type of functions in 99% of the cases
indicate of the abusing devm_.
I will change the code to call ioremap() and request_mem_region() directly
instead of the devm_ versions.
+ if (ret < 0)
+ return dev_err_probe(dev, ret, "failed uart feature walk\n");
...
+ dev_info(dev, "serial8250_register_8250_port %d\n", dfluart->line);
Why do we need this noise?
No, we do not need this noise.
...
+ if (dfluart->line >= 0)
When this can be false?
This can never be false. I will remove it.
+ serial8250_unregister_port(dfluart->line);
...
+config SERIAL_8250_DFL
+ tristate "DFL bus driver for Altera 16550 UART"
+ depends on SERIAL_8250 && FPGA_DFL
+ help
+ This option enables support for a Device Feature List (DFL) bus
+ driver for the Altera 16650 UART. One or more Altera 16650 UARTs
+ can be instantiated in a FPGA and then be discovered during
+ enumeration of the DFL bus.
When m, what be the module name?
I see the file, kernel/drivers/tty/serial/8250/8250_dfl.ko, installed
into /lib/modules/... I also see "alias dfl:t0000f0024* 8250_dfl" in
modules.alias
...
obj-$(CONFIG_SERIAL_8250_FOURPORT) += 8250_fourport.o
obj-$(CONFIG_SERIAL_8250_ACCENT) += 8250_accent.o
obj-$(CONFIG_SERIAL_8250_BOCA) += 8250_boca.o
+obj-$(CONFIG_SERIAL_8250_DFL) += 8250_dfl.o
This group of drivers for the 4 UARTs on the board or so, does FPGA belong to
it? (Same Q, btw, for the Kconfig section. And yes, I know that some of the
entries are not properly placed there and in Makefile.)
Since 8250_dfl results in its own module, and my kernel config doesn't
have FOURPORT, ACCENT, nor BOCA, I guess I don't understand the problem.
obj-$(CONFIG_SERIAL_8250_EXAR_ST16C554) += 8250_exar_st16c554.o
obj-$(CONFIG_SERIAL_8250_HUB6) += 8250_hub6.o
obj-$(CONFIG_SERIAL_8250_FSL) += 8250_fsl.o
--
With Best Regards,
Andy Shevchenko
Thanks for the feedback.
