Re: [PATCH v3 4/4] tty: serial: 8250: add DFL bus driver for Altera 16550.

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Thu, Oct 06, 2022 at 10:00:43AM -0700, matthew.gerlach@linux.intel.com wrote:
> On Tue, 4 Oct 2022, Andy Shevchenko wrote:
> > On Tue, Oct 04, 2022 at 07:37:18AM -0700, matthew.gerlach@linux.intel.com wrote:

...

> > > Reported-by: kernel test robot <lkp@intel.com>
> > 
> > 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?

What's yours take from the above documentation?

...

> > > +	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.

But why will you need request_mem_region() in that case?

> > > +	if (ret < 0)
> > > +		return dev_err_probe(dev, ret, "failed uart feature walk\n");

...

> > > +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

My point is that user who will run `make menuconfig` will read this and have
no clue after the kernel build if the module was built or not. Look into other
(recent) sections of the Kconfig for drivers in the kernel for how they inform
user about the module name (this more or less standard pattern you just need
to copy'n'paste'n'edit carefully).

...

> > >  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.

The Makefile is a bit chaotic, but try to find the sorted (more or less)
group of drivers that are not 4 ports and squeeze your entry there
(I expect somewhere between the LPSS/MID lines).

It will help to sort out that mess in the future.

> > >  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