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
