Re: [PATCH v7,1/2] Serial: silabs si4455 serial driver

[József Horváth]

On Tue, Jan 05, 2021 at 02:28:16PM +0100, 'Greg Kroah-Hartman' wrote:
> On Tue, Jan 05, 2021 at 10:29:25AM +0000, József Horváth wrote:
> > This is a serial port driver for
> >  Silicon Labs Si4455 Sub-GHz transciver.
> > 
> > The goal of this driver is to removing wires
> >  between central(linux) device and remote serial devices/sensors,
> >  but keeping the original user software.
> >  It represents regular serial interface for the user space.
> > 
> > Datasheet: https://www.silabs.com/documents/public/data-sheets/Si4455.pdf
> > 
> > Guide: https://github.com/dministro/linux-serial-si4455
> > 
> > Signed-off-by: Jozsef Horvath <info@xxxxxxxxxxx>
> > ---
> > 
> >  
> > +config SERIAL_SI4455
> > +	tristate "Si4455 support"
> > +	depends on SPI
> > +	select SERIAL_CORE
> > +	help
> > +	  This driver is for Silicon Labs's Si4455 Sub-GHz transciver.
> > +	  Say 'Y' here if you wish to use it as serial port.
> > +
> 
> No module name?

Sorry, I dont understand your question. Can you explain it?

> 
> >  endmenu
> > +#include <linux/string.h>
> > +#include <linux/firmware.h>
> > +#include <linux/timer.h>
> > +#ifdef CONFIG_DEBUG_FS
> > +#include <linux/debugfs.h>
> > +#endif
> 
> No need for #ifdef for .h files.
> 

Ok, its clear. I'll remove it from here and below.

> > +
> > +#define PORT_SI4455						1096
> > +#define SI4455_NAME						"Si4455"
> > +#define SI4455_MAJOR						432
> > +#define SI4455_MINOR						567
> 
> Where are these major/minor numbers being used and where did they come
> from?  Why do you need them?
> 
> > +struct si4455_port {
> > +	struct uart_port port;
> > +#ifdef CONFIG_DEBUG_FS
> > +	struct dentry *dbgfs_dir;
> > +#endif
> 
> Do not put #ifdefs in .c code, you never need to check for this type of
> thing.
> 
> > +static struct uart_driver si4455_uart = {
> > +	.owner			= THIS_MODULE,
> > +	.driver_name		= SI4455_NAME,
> > +#ifdef CONFIG_DEVFS_FS
> > +	.dev_name		= "ttySI%d",
> 
> Looks like you are porting this from a _VERY_ old kernel.  That config
> option went away 15+ years ago.  Are you sure this works?
> 

Ok, I'll remove it.

> 
> > +#else
> > +	.dev_name		= "ttySI",
> 
> Where did you get that name from?
> 

This is my suggestion. I dont know the naming rules.

> 
> > +static int si4455_begin_tx(struct uart_port *port, u32 channel, int length,
> > +			   u8 *data)
> > +{
> > +	int ret = 0;
> > +	struct si4455_int_status int_status = { 0 };
> > +	struct si4455_fifo_info fifo_info = { 0 };
> > +
> > +	dev_dbg(port->dev, "%s(%u, %u)\n", __func__, channel, length);
> 
> No need for these types of debugging lines, just use ftrace.
> 
> Please remove them, you have them in a few places (same for the end of
> functions.)
> 
> > +static void si4455_null_void(struct uart_port *port)
> > +{
> > +	/* Do nothing */
> 
> Why do you need this???

I'll check this.

> 
> > +#ifdef CONFIG_DEBUG_FS
> 
> Again, no #ifdef needed.
> 
> > +static int si4455_debugfs_init(struct device *dev)
> > +{
> > +	struct si4455_port *s = dev_get_drvdata(dev);
> > +	struct dentry *dbgfs_si_dir;
> > +	struct dentry *dbgfs_partinfo_dir;
> > +	struct dentry *dbgfs_entry;
> > +
> > +	s->dbgfs_dir = debugfs_create_dir(dev_name(dev), NULL);
> > +	if (IS_ERR(s->dbgfs_dir))
> > +		return PTR_ERR(s->dbgfs_dir);
> 
> No  need to check any debugfs return value, just use it and move on.
> 
> > +
> > +	dbgfs_si_dir = debugfs_create_dir("si4455", s->dbgfs_dir);
> > +	if (IS_ERR(dbgfs_si_dir))
> > +		return PTR_ERR(dbgfs_si_dir);
> > +
> > +	dbgfs_entry = debugfs_create_u32("cts_error_count", 0444,
> > +					 dbgfs_si_dir, &s->cts_error_count);
> > +	if (IS_ERR(dbgfs_entry))
> > +		return PTR_ERR(dbgfs_entry);
> 
> Same for all of these, no need to check anything.

Ok, its clear.

> 
> > +
> > +	dbgfs_entry = debugfs_create_u32("tx_error_count", 0444,
> > +					 dbgfs_si_dir, &s->tx_error_count);
> > +	if (IS_ERR(dbgfs_entry))
> > +		return PTR_ERR(dbgfs_entry);
> > +
> > +	dbgfs_partinfo_dir = debugfs_create_dir("partinfo", dbgfs_si_dir);
> > +	if (IS_ERR(dbgfs_partinfo_dir))
> > +		return PTR_ERR(dbgfs_partinfo_dir);
> > +
> > +	dbgfs_entry = debugfs_create_u8("chip_rev", 0444,
> > +					dbgfs_partinfo_dir,
> > +					&s->part_info.chip_rev);
> 
> Wait, did you even build this code?  Does it work?  It shouldn't, these
> debugfs calls have changed...
> 
> I'm stopping reviewing here.

Working test systems:

- #1 - one Si4455 connected to spi1.2:
 $ uname -r
   4.19.66-v7+

 $ ls -R /sys/kernel/debug/spi1.2/si4455
   /sys/kernel/debug/spi1.2/si4455:
   cts_error_count  partinfo  tx_error_count

   /sys/kernel/debug/spi1.2/si4455/partinfo:
   chip_rev  part  rom_id

 $ ls /dev | grep ttySI
   ttySI0

- #2 - one Si4455 connected to spi0.0 and an other to spi0.1:
 $ uname -r
   5.4.79-v7+

 $ ls -R /sys/kernel/debug/spi0.0/si4455
   /sys/kernel/debug/spi0.0/si4455:
   cts_error_count  partinfo  tx_error_count

   /sys/kernel/debug/spi0.0/si4455/partinfo:
   chip_rev  part  rom_id

 $ ls -R /sys/kernel/debug/spi0.1/si4455
   /sys/kernel/debug/spi0.1/si4455:
   cts_error_count  partinfo  tx_error_count

   /sys/kernel/debug/spi0.1/si4455/partinfo:
   chip_rev  part  rom_id

 $ cat /sys/kernel/debug/spi0.0/si4455/partinfo/chip_rev
   34 <- Its valid

 $ ls /dev | grep ttySI
   ttySI0
   ttySI1

I made a short guide to using the interfaces, generating the firmware and a simple setup.
You can see it: https://github.com/dministro/linux-serial-si4455
I always test, compile, test, compile, test, test, test, checkpatch before sending my patch.

So my answer is yes, but you are right too :)

I found the answer just now, I compiled this for kernel v4.19.66 and v5.4.79 but not for v5.10.

Sorry for this, and thank you for suggestions.

> 
> thanks,
> 
> greg k-h

Üdvözlettel / Best regards:
József Horváth