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:

quoted

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

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

quoted

 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.

quoted

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

quoted

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

quoted

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

quoted

+#else
+	.dev_name		= "ttySI",

Where did you get that name from?

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

quoted

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

quoted

+static void si4455_null_void(struct uart_port *port)
+{
+	/* Do nothing */

Why do you need this???

I'll check this.

quoted

+#ifdef CONFIG_DEBUG_FS

Again, no #ifdef needed.

quoted

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

quoted

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

quoted

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