Skip to contentSkip to conversation

Serial: silabs si4455 serial driver

PATCH Superseded József Horváth 3 participants 14 messages Dec 10, 2020 – Dec 11, 2020

Subsystems (2)
Device tree bindings TTY layer and serial drivers

v2 of 8 Superseded Latest is v8 →

lore
Share & tools
lore

József Horváth

author
#0 lore raw

PATCH v2 1/1 Serial: silabs si4455 serial driver

This is a serial port driver for
Silicon Labs Si4455 Sub-GHz transciver.

Signed-off-by József Horváth <[email protected]>
---
 .../bindings/serial/silabs,si4455.yaml        |   53 +
 MAINTAINERS                                   |    7 +
 drivers/tty/serial/Kconfig                    |    8 +
 drivers/tty/serial/Makefile                   |    1 +
 drivers/tty/serial/si4455.c                   | 1235 +++++++++++++++++
 drivers/tty/serial/si4455_api.h               |   56 +
 6 files changed, 1360 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/serial/silabs,si4455.yaml
 create mode 100644 drivers/tty/serial/si4455.c
 create mode 100644 drivers/tty/serial/si4455_api.h
Documentation/devicetree/bindings/serial/silabs,si4455.yaml new file +53 -0
new file mode 100644
index 000000000000..80a73a61755b
@@ -0,0 +1,53 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/serial/silabs,si4455.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: Silicon Labs Si4455 device tree bindings
+
+maintainers:
+  - József Horváth <[email protected]>
+
+allOf:
+  - $ref: "/schemas/serial.yaml#"
+
+properties:
+  compatible:
+    const: silabs,si4455
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  spi-max-frequency:
+    description: maximum clock frequency on SPI port
+    maximum: 500000
+
+  shutdown-gpios:
+    description: gpio pin for SDN
+    maxItems: 1
+
+required:
+  - reg
+  - interrupts
+  - spi-max-frequency
+  - shutdown-gpios
+
+additionalProperties: false
+
+examples:
+  - |
+    &spi0 {
+      serial0: si4455@0 {
+        compatible = "silabs,si4455";
+        reg = <0>;
+        interrupt-parent = <&gpio>;
+        interrupts = <7 IRQ_TYPE_LEVEL_LOW>;
+        shutdown-gpios = <&gpio 26 1>;
+        spi-max-frequency = <300000>;
+      };
+    };
+...
MAINTAINERS +7 -0
index a008b70f3c16..52ce36c5b029 100644
@@ -15952,6 +15952,13 @@ M:	Jérôme Pouiller <[email protected]>
 S:	Supported
 F:	drivers/staging/wfx/
 
+SILICON LABS SI4455 SERIAL DRIVER
+M:	József Horváth <[email protected]>
+S:	Maintained
+F:	Documentation/devicetree/bindings/serial/silabs,si4455.yaml
+F:	drivers/tty/serial/si4455.c
+F:	drivers/tty/serial/si4455_api.h
+
 SILICON MOTION SM712 FRAME BUFFER DRIVER
 M:	Sudip Mukherjee <[email protected]>
 M:	Teddy Wang <[email protected]>
drivers/tty/serial/Kconfig +8 -0
index 28f22e58639c..560aa311cd03 100644
@@ -1583,6 +1583,14 @@ config SERIAL_MILBEAUT_USIO_CONSOLE
 	  receives all kernel messages and warnings and which allows logins in
 	  single user mode).
 
+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.
+
 endmenu
 
 config SERIAL_MCTRL_GPIO
drivers/tty/serial/Makefile +1 -0
index caf167f0c10a..2a3076b98c78 100644
@@ -90,6 +90,7 @@ obj-$(CONFIG_SERIAL_OWL)	+= owl-uart.o
 obj-$(CONFIG_SERIAL_RDA)	+= rda-uart.o
 obj-$(CONFIG_SERIAL_MILBEAUT_USIO) += milbeaut_usio.o
 obj-$(CONFIG_SERIAL_SIFIVE)	+= sifive.o
+obj-$(CONFIG_SERIAL_SI4455) += si4455.o
 
 # GPIOLIB helpers for modem control lines
 obj-$(CONFIG_SERIAL_MCTRL_GPIO)	+= serial_mctrl_gpio.o
drivers/tty/serial/si4455.c new file +1235 -0

1,235 lines not on this page · raw message

drivers/tty/serial/si4455_api.h new file +56 -0

56 lines not on this page · raw message

Signature · 1 line
2.17.1
maintainer
#1 lore raw
Quoted 11 lines · József Horváth

This is a serial port driver for
Silicon Labs Si4455 Sub-GHz transciver.

Signed-off-by József Horváth <[email protected]>
---
 .../bindings/serial/silabs,si4455.yaml        |   53 +
 MAINTAINERS                                   |    7 +
 drivers/tty/serial/Kconfig                    |    8 +
 drivers/tty/serial/Makefile                   |    1 +
 drivers/tty/serial/si4455.c                   | 1235 +++++++++++++++++
 drivers/tty/serial/si4455_api.h               |   56 +

First thing, a single .c file should not need a .h file.

But then I looked at the .h file and see:

Quoted 4 lines
drivers/tty/serial/si4455_api.h new file +1 -0
@@ -0,0 +1,56 @@
+/* SPDX-License-Identifier: GPL-2.0

drivers/tty/serial/si4455_api.h line 11

Quoted 10 lines
drivers/tty/serial/si4455_api.h new file +10 -0
+ *
+ * Copyright (C) 2020 József Horváth <[email protected]>
+ *
+ */
+#ifndef SI4455_API_H_
+#define SI4455_API_H_
+
+struct si4455_iocbuff {
+	uint32_t length;
+	uint8_t	data[4096];

If you do have an ioctl, use proper data types. These are not the correct ones (hint, __u32 and __u8).

drivers/tty/serial/si4455_api.h line 21

Quoted 10 lines
drivers/tty/serial/si4455_api.h new file +10 -0
+};
+
+#define BASE_TTYIOC_PRIVATE		0xA0
+/* Set EZConfig.
+ * After this ioctl call, the driver restarts the si4455,
+ * then apply the new configuration and patch.
+ */
+#define SI4455_IOC_SEZC		_IOW('T', \
+				     BASE_TTYIOC_PRIVATE + 0x01, \
+				     struct si4455_iocbuff)

Why does a serial driver have private ioctls? Please no, don't do that. Implement the basic serial driver first, and then we can talk about "custom" configurations and the like, using the correct apis.

thanks,

greg k-h

author
#2 lore raw
Quoted 35 lines · Greg Kroah-Hartman
Quoted 11 lines · József Horváth

This is a serial port driver for
Silicon Labs Si4455 Sub-GHz transciver.

Signed-off-by József Horváth <[email protected]>
---
 .../bindings/serial/silabs,si4455.yaml        |   53 +
 MAINTAINERS                                   |    7 +
 drivers/tty/serial/Kconfig                    |    8 +
 drivers/tty/serial/Makefile                   |    1 +
 drivers/tty/serial/si4455.c                   | 1235 +++++++++++++++++
 drivers/tty/serial/si4455_api.h               |   56 +

First thing, a single .c file should not need a .h file.

But then I looked at the .h file and see:

Quoted 4 lines
drivers/tty/serial/si4455_api.h new file +1 -0
@@ -0,0 +1,56 @@
+/* SPDX-License-Identifier: GPL-2.0

drivers/tty/serial/si4455_api.h line 11

Quoted 10 lines
drivers/tty/serial/si4455_api.h new file +10 -0
+ *
+ * Copyright (C) 2020 József Horváth <[email protected]>
+ *
+ */
+#ifndef SI4455_API_H_
+#define SI4455_API_H_
+
+struct si4455_iocbuff {
+	uint32_t length;
+	uint8_t	data[4096];

If you do have an ioctl, use proper data types. These are not the correct ones (hint, __u32 and __u8).

Quoted 12 lines

drivers/tty/serial/si4455_api.h line 21

Quoted 10 lines
drivers/tty/serial/si4455_api.h new file +10 -0
+};
+
+#define BASE_TTYIOC_PRIVATE		0xA0
+/* Set EZConfig.
+ * After this ioctl call, the driver restarts the si4455,
+ * then apply the new configuration and patch.
+ */
+#define SI4455_IOC_SEZC		_IOW('T', \
+				     BASE_TTYIOC_PRIVATE + 0x01, \
+				     struct si4455_iocbuff)

Why does a serial driver have private ioctls? Please no, don't do that.

I checked the ioctl.h and serial_core.h, but I not found any similar definition, like BASE_VIDIOC_PRIVATE in videodev2.h. In this case the name of macro BASE_TTYIOC_PRIVATE means the base value of special ioctl commands owned by this driver.

I can change it to BASE_TTYIOC or SI4455_IOC_BASE

Quoted 2 lines

Implement the basic serial driver first, and then we can talk about "custom" configurations and the like, using the correct apis.

Without the SI4455_IOC_SEZC call, the driver can't configure the Si4455 and not working at all.
The cofiguration for interface is provided by user for application.
It contains the base frequency, channel spacing, modulation, and a lot of more stuff, and generated by Silicon Labs Wireless Development Suite. The generated configuration is in a non public(compressed, encrypted...who knows) format, so without this the driver can't provide configuration parameters to Si4455.

Quoted 4 lines

thanks,

greg k-h

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

maintainer
#3 lore raw
Quoted 7 lines · József Horváth
Quoted 2 lines

Implement the basic serial driver first, and then we can talk about "custom" configurations and the like, using the correct apis.

Without the SI4455_IOC_SEZC call, the driver can't configure the Si4455 and not working at all.
The cofiguration for interface is provided by user for application.
It contains the base frequency, channel spacing, modulation, and a lot of more stuff, and generated by Silicon Labs Wireless Development Suite. The generated configuration is in a non public(compressed, encrypted...who knows) format, so without this the driver can't provide configuration parameters to Si4455.

request_firmware() comes to my mind then.

thanks,

Signature · 2 lines
js
suse labs
maintainer
#4 lore raw
In reply to József Horváth
Quoted 49 lines · József Horváth
Quoted 35 lines · Greg Kroah-Hartman
Quoted 11 lines · József Horváth

This is a serial port driver for
Silicon Labs Si4455 Sub-GHz transciver.

Signed-off-by József Horváth <[email protected]>
---
 .../bindings/serial/silabs,si4455.yaml        |   53 +
 MAINTAINERS                                   |    7 +
 drivers/tty/serial/Kconfig                    |    8 +
 drivers/tty/serial/Makefile                   |    1 +
 drivers/tty/serial/si4455.c                   | 1235 +++++++++++++++++
 drivers/tty/serial/si4455_api.h               |   56 +

First thing, a single .c file should not need a .h file.

But then I looked at the .h file and see:

Quoted 4 lines
drivers/tty/serial/si4455_api.h new file +1 -0
@@ -0,0 +1,56 @@
+/* SPDX-License-Identifier: GPL-2.0

drivers/tty/serial/si4455_api.h line 11

Quoted 10 lines
drivers/tty/serial/si4455_api.h new file +10 -0
+ *
+ * Copyright (C) 2020 József Horváth <[email protected]>
+ *
+ */
+#ifndef SI4455_API_H_
+#define SI4455_API_H_
+
+struct si4455_iocbuff {
+	uint32_t length;
+	uint8_t	data[4096];

If you do have an ioctl, use proper data types. These are not the correct ones (hint, __u32 and __u8).

Quoted 12 lines

drivers/tty/serial/si4455_api.h line 21

Quoted 10 lines
drivers/tty/serial/si4455_api.h new file +10 -0
+};
+
+#define BASE_TTYIOC_PRIVATE		0xA0
+/* Set EZConfig.
+ * After this ioctl call, the driver restarts the si4455,
+ * then apply the new configuration and patch.
+ */
+#define SI4455_IOC_SEZC		_IOW('T', \
+				     BASE_TTYIOC_PRIVATE + 0x01, \
+				     struct si4455_iocbuff)

Why does a serial driver have private ioctls? Please no, don't do that.

Quoted 2 lines

I checked the ioctl.h and serial_core.h, but I not found any similar definition, like BASE_VIDIOC_PRIVATE in videodev2.h. In this case the name of macro BASE_TTYIOC_PRIVATE means the base value of special ioctl commands owned by this driver.

My point is, a serial driver should NOT have any custom ioctls.

Quoted 7 lines

I can change it to BASE_TTYIOC or SI4455_IOC_BASE

Quoted 2 lines

Implement the basic serial driver first, and then we can talk about "custom" configurations and the like, using the correct apis.

Without the SI4455_IOC_SEZC call, the driver can't configure the Si4455 and not working at all. The cofiguration for interface is provided by user for application.

That is what a device tree is for, to configure the device to have the correct system configuration, why can't that be the same here?

Quoted 6 lines

It contains the base frequency, channel spacing, modulation, and a lot of more stuff, and generated by Silicon Labs Wireless Development Suite.
The generated configuration is in a non public(compressed, encrypted...who knows) format, so without this the driver can't provide configuration parameters to Si4455.

So we have to take a "custom" userspace blob and send it to the device to configure it properly? Like Jiri said, sounds like firmware, so just use that interface instead.

thanks,

greg k-h

author
#5 lore raw
Quoted 40 lines · Greg Kroah-Hartman
Quoted 16 lines · József Horváth
Quoted 14 lines · Greg Kroah-Hartman

drivers/tty/serial/si4455_api.h line 21

Quoted 11 lines · József Horváth

This is a serial port driver for
Silicon Labs Si4455 Sub-GHz transciver.

drivers/tty/serial/si4455_api.h new file +9 -0
+
+#define BASE_TTYIOC_PRIVATE		0xA0
+/* Set EZConfig.
+ * After this ioctl call, the driver restarts the si4455,
+ * then apply the new configuration and patch.
+ */
+#define SI4455_IOC_SEZC		_IOW('T', \
+				     BASE_TTYIOC_PRIVATE + 0x01, \
+				     struct si4455_iocbuff)

Why does a serial driver have private ioctls? Please no, don't do that.

Quoted 2 lines

I checked the ioctl.h and serial_core.h, but I not found any similar definition, like BASE_VIDIOC_PRIVATE in videodev2.h. In this case the name of macro BASE_TTYIOC_PRIVATE means the base value of special ioctl commands owned by this driver.

My point is, a serial driver should NOT have any custom ioctls.

Quoted 7 lines

I can change it to BASE_TTYIOC or SI4455_IOC_BASE

Quoted 2 lines

Implement the basic serial driver first, and then we can talk about "custom" configurations and the like, using the correct apis.

Without the SI4455_IOC_SEZC call, the driver can't configure the Si4455 and not working at all. The cofiguration for interface is provided by user for application.

That is what a device tree is for, to configure the device to have the correct system configuration, why can't that be the same here?

Quoted 6 lines

It contains the base frequency, channel spacing, modulation, and a lot of more stuff, and generated by Silicon Labs Wireless Development Suite.
The generated configuration is in a non public(compressed, encrypted...who knows) format, so without this the driver can't provide configuration parameters to Si4455.

Quoted 3 lines

So we have to take a "custom" userspace blob and send it to the device to configure it properly? Like Jiri said, sounds like firmware, so just use that interface instead.

I checked Jiri's suggestion, and it is a good solution to replace SI4455_IOC_SEZC(configuration) and SI4455_IOC_SEZP(firmware patch). I can move SI4455_IOC_SSIZ(package size) to device tree property.

Maybe you have good suggestion for the following:
SI4455_IOC_STXC -> Radio transmit channel index. It is a real use case to control this parameter by user at runtime. SI4455_IOC_SRXC -> Radio receive channel index. It is a real use case to control this parameter by user at runtime. SI4455_IOC_GRSSI -> Last measured RSSI, when packet received. This is a useful information. (Currently I'm the only one user, and I need this :) )

Quoted 4 lines

thanks,

greg k-h

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

maintainer
#6 lore raw
Quoted 45 lines · József Horváth
Quoted 40 lines · Greg Kroah-Hartman
Quoted 16 lines · József Horváth
Quoted 14 lines · Greg Kroah-Hartman

drivers/tty/serial/si4455_api.h line 21

Quoted 11 lines · József Horváth

This is a serial port driver for
Silicon Labs Si4455 Sub-GHz transciver.

drivers/tty/serial/si4455_api.h new file +9 -0
+
+#define BASE_TTYIOC_PRIVATE		0xA0
+/* Set EZConfig.
+ * After this ioctl call, the driver restarts the si4455,
+ * then apply the new configuration and patch.
+ */
+#define SI4455_IOC_SEZC		_IOW('T', \
+				     BASE_TTYIOC_PRIVATE + 0x01, \
+				     struct si4455_iocbuff)

Why does a serial driver have private ioctls? Please no, don't do that.

Quoted 2 lines

I checked the ioctl.h and serial_core.h, but I not found any similar definition, like BASE_VIDIOC_PRIVATE in videodev2.h. In this case the name of macro BASE_TTYIOC_PRIVATE means the base value of special ioctl commands owned by this driver.

My point is, a serial driver should NOT have any custom ioctls.

Quoted 7 lines

I can change it to BASE_TTYIOC or SI4455_IOC_BASE

Quoted 2 lines

Implement the basic serial driver first, and then we can talk about "custom" configurations and the like, using the correct apis.

Without the SI4455_IOC_SEZC call, the driver can't configure the Si4455 and not working at all. The cofiguration for interface is provided by user for application.

That is what a device tree is for, to configure the device to have the correct system configuration, why can't that be the same here?

Quoted 6 lines

It contains the base frequency, channel spacing, modulation, and a lot of more stuff, and generated by Silicon Labs Wireless Development Suite.
The generated configuration is in a non public(compressed, encrypted...who knows) format, so without this the driver can't provide configuration parameters to Si4455.

Quoted 3 lines

So we have to take a "custom" userspace blob and send it to the device to configure it properly? Like Jiri said, sounds like firmware, so just use that interface instead.

Quoted 6 lines

I checked Jiri's suggestion, and it is a good solution to replace SI4455_IOC_SEZC(configuration) and SI4455_IOC_SEZP(firmware patch). I can move SI4455_IOC_SSIZ(package size) to device tree property.

Maybe you have good suggestion for the following:
SI4455_IOC_STXC -> Radio transmit channel index. It is a real use case to control this parameter by user at runtime. SI4455_IOC_SRXC -> Radio receive channel index. It is a real use case to control this parameter by user at runtime.

These are not serial port things, why would a serial port care about these?

Quoted 2 lines

SI4455_IOC_GRSSI -> Last measured RSSI, when packet received. This is a useful information. (Currently I'm the only one user, and I need this :) )

What is "RSSI"?

And why not debugfs if it's only debugging stuff?

thanks,

greg k-h

author
#7 lore raw
Quoted 49 lines · Greg Kroah-Hartman
Quoted 48 lines · József Horváth
Quoted 40 lines · Greg Kroah-Hartman
Quoted 16 lines · József Horváth
Quoted 14 lines · Greg Kroah-Hartman

drivers/tty/serial/si4455_api.h line 21

Quoted 11 lines · József Horváth

This is a serial port driver for
Silicon Labs Si4455 Sub-GHz transciver.

drivers/tty/serial/si4455_api.h new file +9 -0
+
+#define BASE_TTYIOC_PRIVATE		0xA0
+/* Set EZConfig.
+ * After this ioctl call, the driver restarts the si4455,
+ * then apply the new configuration and patch.
+ */
+#define SI4455_IOC_SEZC		_IOW('T', \
+				     BASE_TTYIOC_PRIVATE + 0x01, \
+				     struct si4455_iocbuff)

Why does a serial driver have private ioctls? Please no, don't do that.

Quoted 2 lines

I checked the ioctl.h and serial_core.h, but I not found any similar definition, like BASE_VIDIOC_PRIVATE in videodev2.h. In this case the name of macro BASE_TTYIOC_PRIVATE means the base value of special ioctl commands owned by this driver.

My point is, a serial driver should NOT have any custom ioctls.

Quoted 7 lines

I can change it to BASE_TTYIOC or SI4455_IOC_BASE

Quoted 2 lines

Implement the basic serial driver first, and then we can talk about "custom" configurations and the like, using the correct apis.

Without the SI4455_IOC_SEZC call, the driver can't configure the Si4455 and not working at all. The cofiguration for interface is provided by user for application.

That is what a device tree is for, to configure the device to have the correct system configuration, why can't that be the same here?

Quoted 6 lines

It contains the base frequency, channel spacing, modulation, and a lot of more stuff, and generated by Silicon Labs Wireless Development Suite.
The generated configuration is in a non public(compressed, encrypted...who knows) format, so without this the driver can't provide configuration parameters to Si4455.

Quoted 3 lines

So we have to take a "custom" userspace blob and send it to the device to configure it properly? Like Jiri said, sounds like firmware, so just use that interface instead.

I checked Jiri's suggestion, and it is a good solution to replace SI4455_IOC_SEZC(configuration) and SI4455_IOC_SEZP(firmware patch). I can move SI4455_IOC_SSIZ(package size) to device tree property.

Quoted 6 lines
Quoted 3 lines

Maybe you have good suggestion for the following:
SI4455_IOC_STXC -> Radio transmit channel index. It is a real use case to control this parameter by user at runtime. SI4455_IOC_SRXC -> Radio receive channel index. It is a real use case to control this parameter by user at runtime.

These are not serial port things, why would a serial port care about these?

You are right, these are not regular serial port things, but this device is not a regular uart, it is a sub-GHz transciever, digital radio. This driver tries to represent it as a serial port to user.

Quoted 7 lines
Quoted 3 lines

SI4455_IOC_GRSSI -> Last measured RSSI, when packet received. This is a useful information. (Currently I'm the only one user, and I need this :) )

What is "RSSI"?

And why not debugfs if it's only debugging stuff?

Received signal strength indication, and not only debugging. It is an information for the end user.

Quoted 4 lines

thanks,

greg k-h

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

maintainer
#8 lore raw
Quoted 54 lines · József Horváth
Quoted 53 lines · Greg Kroah-Hartman
Quoted 51 lines · József Horváth
Quoted 40 lines · Greg Kroah-Hartman
Quoted 16 lines · József Horváth
Quoted 14 lines · Greg Kroah-Hartman

Review

Quoted 11 lines · József Horváth

This is a serial port driver for
Silicon Labs Si4455 Sub-GHz transciver.

(quoted diff) +9 -0
+
+#define BASE_TTYIOC_PRIVATE		0xA0
+/* Set EZConfig.
+ * After this ioctl call, the driver restarts the si4455,
+ * then apply the new configuration and patch.
+ */
+#define SI4455_IOC_SEZC		_IOW('T', \
+				     BASE_TTYIOC_PRIVATE + 0x01, \
+				     struct si4455_iocbuff)

Why does a serial driver have private ioctls? Please no, don't do that.

Quoted 2 lines

I checked the ioctl.h and serial_core.h, but I not found any similar definition, like BASE_VIDIOC_PRIVATE in videodev2.h. In this case the name of macro BASE_TTYIOC_PRIVATE means the base value of special ioctl commands owned by this driver.

My point is, a serial driver should NOT have any custom ioctls.

Quoted 7 lines

I can change it to BASE_TTYIOC or SI4455_IOC_BASE

Quoted 2 lines

Implement the basic serial driver first, and then we can talk about "custom" configurations and the like, using the correct apis.

Without the SI4455_IOC_SEZC call, the driver can't configure the Si4455 and not working at all. The cofiguration for interface is provided by user for application.

That is what a device tree is for, to configure the device to have the correct system configuration, why can't that be the same here?

Quoted 6 lines

It contains the base frequency, channel spacing, modulation, and a lot of more stuff, and generated by Silicon Labs Wireless Development Suite.
The generated configuration is in a non public(compressed, encrypted...who knows) format, so without this the driver can't provide configuration parameters to Si4455.

Quoted 3 lines

So we have to take a "custom" userspace blob and send it to the device to configure it properly? Like Jiri said, sounds like firmware, so just use that interface instead.

I checked Jiri's suggestion, and it is a good solution to replace SI4455_IOC_SEZC(configuration) and SI4455_IOC_SEZP(firmware patch). I can move SI4455_IOC_SSIZ(package size) to device tree property.

Maybe you have good suggestion for the following:
SI4455_IOC_STXC -> Radio transmit channel index. It is a real use case to control this parameter by user at runtime. SI4455_IOC_SRXC -> Radio receive channel index. It is a real use case to control this parameter by user at runtime.

Quoted 5 lines
Quoted 2 lines

These are not serial port things, why would a serial port care about these?

You are right, these are not regular serial port things, but this device is not a regular uart, it is a sub-GHz transciever, digital radio. This driver tries to represent it as a serial port to user.

Is that the correct representation to be using here? Why not act like a proper radio device instead? That way you get to use the normal kernel apis for radio devices.

Quoted 8 lines
Quoted 6 lines
Quoted 2 lines

SI4455_IOC_GRSSI -> Last measured RSSI, when packet received. This is a useful information. (Currently I'm the only one user, and I need this :) )

What is "RSSI"?

And why not debugfs if it's only debugging stuff?

Received signal strength indication, and not only debugging. It is an information for the end user.

How do other radio devices (like wifi controllers) export this information to userspace? Don't create custom apis for only a single device when the goal of a kernel is to make hardware interfaces all work the same as far as userspace is concerned.

thanks,

greg k-h

author
#9 lore raw
Quoted 58 lines · Greg Kroah-Hartman
Quoted 57 lines · József Horváth
Quoted 55 lines · Greg Kroah-Hartman
Quoted 45 lines · József Horváth
Quoted 40 lines · Greg Kroah-Hartman
Quoted 16 lines · József Horváth
Quoted 14 lines · Greg Kroah-Hartman

Review

Quoted 11 lines · József Horváth

This is a serial port driver for
Silicon Labs Si4455 Sub-GHz transciver.

(quoted diff) +9 -0
+
+#define BASE_TTYIOC_PRIVATE		0xA0
+/* Set EZConfig.
+ * After this ioctl call, the driver restarts the si4455,
+ * then apply the new configuration and patch.
+ */
+#define SI4455_IOC_SEZC		_IOW('T', \
+				     BASE_TTYIOC_PRIVATE + 0x01, \
+				     struct si4455_iocbuff)

Why does a serial driver have private ioctls? Please no, don't do that.

Quoted 2 lines

I checked the ioctl.h and serial_core.h, but I not found any similar definition, like BASE_VIDIOC_PRIVATE in videodev2.h. In this case the name of macro BASE_TTYIOC_PRIVATE means the base value of special ioctl commands owned by this driver.

My point is, a serial driver should NOT have any custom ioctls.

Quoted 7 lines

I can change it to BASE_TTYIOC or SI4455_IOC_BASE

Quoted 2 lines

Implement the basic serial driver first, and then we can talk about "custom" configurations and the like, using the correct apis.

Without the SI4455_IOC_SEZC call, the driver can't configure the Si4455 and not working at all. The cofiguration for interface is provided by user for application.

That is what a device tree is for, to configure the device to have the correct system configuration, why can't that be the same here?

Quoted 6 lines

It contains the base frequency, channel spacing, modulation, and a lot of more stuff, and generated by Silicon Labs Wireless Development Suite.
The generated configuration is in a non public(compressed, encrypted...who knows) format, so without this the driver can't provide configuration parameters to Si4455.

Quoted 3 lines

So we have to take a "custom" userspace blob and send it to the device to configure it properly? Like Jiri said, sounds like firmware, so just use that interface instead.

Quoted 6 lines

I checked Jiri's suggestion, and it is a good solution to replace SI4455_IOC_SEZC(configuration) and SI4455_IOC_SEZP(firmware patch). I can move SI4455_IOC_SSIZ(package size) to device tree property.

Maybe you have good suggestion for the following:
SI4455_IOC_STXC -> Radio transmit channel index. It is a real use case to control this parameter by user at runtime. SI4455_IOC_SRXC -> Radio receive channel index. It is a real use case to control this parameter by user at runtime.

These are not serial port things, why would a serial port care about these?

Quoted 6 lines
Quoted 2 lines

You are right, these are not regular serial port things, but this device is not a regular uart, it is a sub-GHz transciever, digital radio. This driver tries to represent it as a serial port to user.

Is that the correct representation to be using here? Why not act like a proper radio device instead? That way you get to use the normal kernel apis for radio devices.

In my mind it is absolute a serial device by the application.

Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 3 lines

SI4455_IOC_GRSSI -> Last measured RSSI, when packet received. This is a useful information. (Currently I'm the only one user, and I need this :) )

What is "RSSI"?

Quoted 8 lines
Quoted 3 lines
Quoted 1 line

And why not debugfs if it's only debugging stuff?

Received signal strength indication, and not only debugging. It is an information for the end user.

How do other radio devices (like wifi controllers) export this information to userspace? Don't create custom apis for only a single device when the goal of a kernel is to make hardware interfaces all work the same as far as userspace is concerned.

I move the package size, tx/rx channel properties to dt as device parameter, and the user could control these properties in sysfs and get rssi too. Finally I can remove all custom ioctl commands. What do you think?

Quoted 4 lines

thanks,

greg k-h

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

maintainer
#10 lore raw
Quoted 59 lines · József Horváth
Quoted 58 lines · Greg Kroah-Hartman
Quoted 57 lines · József Horváth
Quoted 55 lines · Greg Kroah-Hartman
Quoted 45 lines · József Horváth
Quoted 40 lines · Greg Kroah-Hartman
Quoted 16 lines · József Horváth
Quoted 14 lines · Greg Kroah-Hartman

Review

Quoted 11 lines · József Horváth

This is a serial port driver for
Silicon Labs Si4455 Sub-GHz transciver.

(quoted diff) +9 -0
+
+#define BASE_TTYIOC_PRIVATE		0xA0
+/* Set EZConfig.
+ * After this ioctl call, the driver restarts the si4455,
+ * then apply the new configuration and patch.
+ */
+#define SI4455_IOC_SEZC		_IOW('T', \
+				     BASE_TTYIOC_PRIVATE + 0x01, \
+				     struct si4455_iocbuff)

Why does a serial driver have private ioctls? Please no, don't do that.

Quoted 2 lines

I checked the ioctl.h and serial_core.h, but I not found any similar definition, like BASE_VIDIOC_PRIVATE in videodev2.h. In this case the name of macro BASE_TTYIOC_PRIVATE means the base value of special ioctl commands owned by this driver.

My point is, a serial driver should NOT have any custom ioctls.

Quoted 7 lines

I can change it to BASE_TTYIOC or SI4455_IOC_BASE

Quoted 2 lines

Implement the basic serial driver first, and then we can talk about "custom" configurations and the like, using the correct apis.

Without the SI4455_IOC_SEZC call, the driver can't configure the Si4455 and not working at all. The cofiguration for interface is provided by user for application.

That is what a device tree is for, to configure the device to have the correct system configuration, why can't that be the same here?

Quoted 6 lines

It contains the base frequency, channel spacing, modulation, and a lot of more stuff, and generated by Silicon Labs Wireless Development Suite.
The generated configuration is in a non public(compressed, encrypted...who knows) format, so without this the driver can't provide configuration parameters to Si4455.

Quoted 3 lines

So we have to take a "custom" userspace blob and send it to the device to configure it properly? Like Jiri said, sounds like firmware, so just use that interface instead.

Quoted 6 lines

I checked Jiri's suggestion, and it is a good solution to replace SI4455_IOC_SEZC(configuration) and SI4455_IOC_SEZP(firmware patch). I can move SI4455_IOC_SSIZ(package size) to device tree property.

Maybe you have good suggestion for the following:
SI4455_IOC_STXC -> Radio transmit channel index. It is a real use case to control this parameter by user at runtime. SI4455_IOC_SRXC -> Radio receive channel index. It is a real use case to control this parameter by user at runtime.

These are not serial port things, why would a serial port care about these?

Quoted 8 lines
Quoted 6 lines
Quoted 2 lines

You are right, these are not regular serial port things, but this device is not a regular uart, it is a sub-GHz transciever, digital radio. This driver tries to represent it as a serial port to user.

Is that the correct representation to be using here? Why not act like a proper radio device instead? That way you get to use the normal kernel apis for radio devices.

In my mind it is absolute a serial device by the application.

What is the application? Traditionally serial ports don't need radio signals :)

Quoted 14 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 2 lines

SI4455_IOC_GRSSI -> Last measured RSSI, when packet received. This is a useful information. (Currently I'm the only one user, and I need this :) )

What is "RSSI"?

Quoted 8 lines
Quoted 3 lines
Quoted 1 line

And why not debugfs if it's only debugging stuff?

Received signal strength indication, and not only debugging. It is an information for the end user.

How do other radio devices (like wifi controllers) export this information to userspace? Don't create custom apis for only a single device when the goal of a kernel is to make hardware interfaces all work the same as far as userspace is concerned.

Quoted 4 lines

I move the package size, tx/rx channel properties to dt as device parameter, and the user could control these properties in sysfs and get rssi too. Finally I can remove all custom ioctl commands. What do you think?

I do not know, sorry, please try it and see.

thanks,

greg k-h

author
#11 lore raw
Quoted 63 lines · Greg Kroah-Hartman
Quoted 62 lines · József Horváth
Quoted 61 lines · Greg Kroah-Hartman
Quoted 59 lines · József Horváth
Quoted 49 lines · Greg Kroah-Hartman
Quoted 48 lines · József Horváth
Quoted 40 lines · Greg Kroah-Hartman
Quoted 16 lines · József Horváth
Quoted 14 lines · Greg Kroah-Hartman

Review

Quoted 11 lines · József Horváth

This is a serial port driver for
Silicon Labs Si4455 Sub-GHz transciver.

(quoted diff) +9 -0
+
+#define BASE_TTYIOC_PRIVATE		0xA0
+/* Set EZConfig.
+ * After this ioctl call, the driver restarts the si4455,
+ * then apply the new configuration and patch.
+ */
+#define SI4455_IOC_SEZC		_IOW('T', \
+				     BASE_TTYIOC_PRIVATE + 0x01, \
+				     struct si4455_iocbuff)

Why does a serial driver have private ioctls? Please no, don't do that.

Quoted 2 lines

I checked the ioctl.h and serial_core.h, but I not found any similar definition, like BASE_VIDIOC_PRIVATE in videodev2.h. In this case the name of macro BASE_TTYIOC_PRIVATE means the base value of special ioctl commands owned by this driver.

My point is, a serial driver should NOT have any custom ioctls.

Quoted 7 lines

I can change it to BASE_TTYIOC or SI4455_IOC_BASE

Quoted 2 lines

Implement the basic serial driver first, and then we can talk about "custom" configurations and the like, using the correct apis.

Without the SI4455_IOC_SEZC call, the driver can't configure the Si4455 and not working at all. The cofiguration for interface is provided by user for application.

That is what a device tree is for, to configure the device to have the correct system configuration, why can't that be the same here?

Quoted 6 lines

It contains the base frequency, channel spacing, modulation, and a lot of more stuff, and generated by Silicon Labs Wireless Development Suite.
The generated configuration is in a non public(compressed, encrypted...who knows) format, so without this the driver can't provide configuration parameters to Si4455.

Quoted 3 lines

So we have to take a "custom" userspace blob and send it to the device to configure it properly? Like Jiri said, sounds like firmware, so just use that interface instead.

I checked Jiri's suggestion, and it is a good solution to replace SI4455_IOC_SEZC(configuration) and SI4455_IOC_SEZP(firmware patch). I can move SI4455_IOC_SSIZ(package size) to device tree property.

Quoted 6 lines
Quoted 3 lines

Maybe you have good suggestion for the following:
SI4455_IOC_STXC -> Radio transmit channel index. It is a real use case to control this parameter by user at runtime. SI4455_IOC_SRXC -> Radio receive channel index. It is a real use case to control this parameter by user at runtime.

These are not serial port things, why would a serial port care about these?

You are right, these are not regular serial port things, but this device is not a regular uart, it is a sub-GHz transciever, digital radio. This driver tries to represent it as a serial port to user.

Quoted 7 lines
Quoted 5 lines
Quoted 3 lines

Is that the correct representation to be using here? Why not act like a proper radio device instead? That way you get to use the normal kernel apis for radio devices.

In my mind it is absolute a serial device by the application.

What is the application? Traditionally serial ports don't need radio signals :)

The application is connecting newly developed sensors(with only rf interface) and legacy sensors(with regular serial communication over rs-485 with modbus) keeping the legacy user software.

User sw [Java]

	<-> /dev/ttyXXX
		<-> si4455[driver]
			<-> si4455[hardware]
				<---air---> new device[si4455+ARM Cortex-M0] 1
					+-> new device[si4455+ARM Cortex-M0] 2
					+-> new device[si4455+ARM Cortex-M0] n
					+-> gateway[si4455+ARM Cortex-M0]<---RS485--> Legacy device 1
										  +-> Legacy device 2
										  +-> Legacy device n

I think this driver could be a good solution in a lot off applications where the user wants to change from wired to wireless with this(Si4455) device, without changing the user sw. Using sub GHz transport is better because the band below GHz is less loaded than Wifi or bluetooth.

Thanks to suggestions, I can remove unnecessary ioctl calls as well. This way, it remains fully compatible with user software and does not need to be hacked.

Quoted 20 lines
Quoted 15 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 3 lines

SI4455_IOC_GRSSI -> Last measured RSSI, when packet received. This is a useful information. (Currently I'm the only one user, and I need this :) )

What is "RSSI"?

Quoted 8 lines
Quoted 3 lines
Quoted 1 line

And why not debugfs if it's only debugging stuff?

Received signal strength indication, and not only debugging. It is an information for the end user.

How do other radio devices (like wifi controllers) export this information to userspace? Don't create custom apis for only a single device when the goal of a kernel is to make hardware interfaces all work the same as far as userspace is concerned.

Quoted 4 lines

I move the package size, tx/rx channel properties to dt as device parameter, and the user could control these properties in sysfs and get rssi too. Finally I can remove all custom ioctl commands. What do you think?

Quoted 5 lines

I do not know, sorry, please try it and see.

thanks,

greg k-h

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

maintainer
#12 lore raw
Quoted 75 lines · József Horváth
Quoted 63 lines · Greg Kroah-Hartman
Quoted 62 lines · József Horváth
Quoted 61 lines · Greg Kroah-Hartman
Quoted 59 lines · József Horváth
Quoted 49 lines · Greg Kroah-Hartman
Quoted 48 lines · József Horváth
Quoted 40 lines · Greg Kroah-Hartman
Quoted 16 lines · József Horváth
Quoted 14 lines · Greg Kroah-Hartman

Review

Quoted 11 lines · József Horváth

This is a serial port driver for
Silicon Labs Si4455 Sub-GHz transciver.

(quoted diff) +9 -0
+
+#define BASE_TTYIOC_PRIVATE		0xA0
+/* Set EZConfig.
+ * After this ioctl call, the driver restarts the si4455,
+ * then apply the new configuration and patch.
+ */
+#define SI4455_IOC_SEZC		_IOW('T', \
+				     BASE_TTYIOC_PRIVATE + 0x01, \
+				     struct si4455_iocbuff)

Why does a serial driver have private ioctls? Please no, don't do that.

Quoted 2 lines

I checked the ioctl.h and serial_core.h, but I not found any similar definition, like BASE_VIDIOC_PRIVATE in videodev2.h. In this case the name of macro BASE_TTYIOC_PRIVATE means the base value of special ioctl commands owned by this driver.

My point is, a serial driver should NOT have any custom ioctls.

Quoted 7 lines

I can change it to BASE_TTYIOC or SI4455_IOC_BASE

Quoted 2 lines

Implement the basic serial driver first, and then we can talk about "custom" configurations and the like, using the correct apis.

Without the SI4455_IOC_SEZC call, the driver can't configure the Si4455 and not working at all. The cofiguration for interface is provided by user for application.

That is what a device tree is for, to configure the device to have the correct system configuration, why can't that be the same here?

Quoted 6 lines

It contains the base frequency, channel spacing, modulation, and a lot of more stuff, and generated by Silicon Labs Wireless Development Suite.
The generated configuration is in a non public(compressed, encrypted...who knows) format, so without this the driver can't provide configuration parameters to Si4455.

Quoted 3 lines

So we have to take a "custom" userspace blob and send it to the device to configure it properly? Like Jiri said, sounds like firmware, so just use that interface instead.

I checked Jiri's suggestion, and it is a good solution to replace SI4455_IOC_SEZC(configuration) and SI4455_IOC_SEZP(firmware patch). I can move SI4455_IOC_SSIZ(package size) to device tree property.

Quoted 6 lines
Quoted 3 lines

Maybe you have good suggestion for the following:
SI4455_IOC_STXC -> Radio transmit channel index. It is a real use case to control this parameter by user at runtime. SI4455_IOC_SRXC -> Radio receive channel index. It is a real use case to control this parameter by user at runtime.

These are not serial port things, why would a serial port care about these?

You are right, these are not regular serial port things, but this device is not a regular uart, it is a sub-GHz transciever, digital radio. This driver tries to represent it as a serial port to user.

Quoted 7 lines
Quoted 5 lines
Quoted 3 lines

Is that the correct representation to be using here? Why not act like a proper radio device instead? That way you get to use the normal kernel apis for radio devices.

In my mind it is absolute a serial device by the application.

What is the application? Traditionally serial ports don't need radio signals :)

The application is connecting newly developed sensors(with only rf interface) and legacy sensors(with regular serial communication over rs-485 with modbus) keeping the legacy user software.

User sw [Java]

Quoted 9 lines
	<-> /dev/ttyXXX
		<-> si4455[driver]
			<-> si4455[hardware]
				<---air---> new device[si4455+ARM Cortex-M0] 1
					+-> new device[si4455+ARM Cortex-M0] 2
					+-> new device[si4455+ARM Cortex-M0] n
					+-> gateway[si4455+ARM Cortex-M0]<---RS485--> Legacy device 1
										  +-> Legacy device 2
										  +-> Legacy device n

If these are "sensors", why are you using a tty interface at all, and not just using the correct iio interface for them?

thanks,

greg k-h

author
#13 lore raw
Quoted 86 lines · Greg Kroah-Hartman
Quoted 75 lines · József Horváth
Quoted 63 lines · Greg Kroah-Hartman
Quoted 62 lines · József Horváth
Quoted 61 lines · Greg Kroah-Hartman
Quoted 59 lines · József Horváth
Quoted 49 lines · Greg Kroah-Hartman
Quoted 48 lines · József Horváth
Quoted 40 lines · Greg Kroah-Hartman
Quoted 16 lines · József Horváth
Quoted 14 lines · Greg Kroah-Hartman

Review

Quoted 11 lines · József Horváth

This is a serial port driver for
Silicon Labs Si4455 Sub-GHz transciver.

(quoted diff) +9 -0
+
+#define BASE_TTYIOC_PRIVATE		0xA0
+/* Set EZConfig.
+ * After this ioctl call, the driver restarts the si4455,
+ * then apply the new configuration and patch.
+ */
+#define SI4455_IOC_SEZC		_IOW('T', \
+				     BASE_TTYIOC_PRIVATE + 0x01, \
+				     struct si4455_iocbuff)

Why does a serial driver have private ioctls? Please no, don't do that.

Quoted 2 lines

I checked the ioctl.h and serial_core.h, but I not found any similar definition, like BASE_VIDIOC_PRIVATE in videodev2.h. In this case the name of macro BASE_TTYIOC_PRIVATE means the base value of special ioctl commands owned by this driver.

My point is, a serial driver should NOT have any custom ioctls.

Quoted 7 lines

I can change it to BASE_TTYIOC or SI4455_IOC_BASE

Quoted 2 lines

Implement the basic serial driver first, and then we can talk about "custom" configurations and the like, using the correct apis.

Without the SI4455_IOC_SEZC call, the driver can't configure the Si4455 and not working at all. The cofiguration for interface is provided by user for application.

That is what a device tree is for, to configure the device to have the correct system configuration, why can't that be the same here?

Quoted 6 lines

It contains the base frequency, channel spacing, modulation, and a lot of more stuff, and generated by Silicon Labs Wireless Development Suite.
The generated configuration is in a non public(compressed, encrypted...who knows) format, so without this the driver can't provide configuration parameters to Si4455.

Quoted 3 lines

So we have to take a "custom" userspace blob and send it to the device to configure it properly? Like Jiri said, sounds like firmware, so just use that interface instead.

I checked Jiri's suggestion, and it is a good solution to replace SI4455_IOC_SEZC(configuration) and SI4455_IOC_SEZP(firmware patch). I can move SI4455_IOC_SSIZ(package size) to device tree property.

Quoted 6 lines
Quoted 3 lines

Maybe you have good suggestion for the following:
SI4455_IOC_STXC -> Radio transmit channel index. It is a real use case to control this parameter by user at runtime. SI4455_IOC_SRXC -> Radio receive channel index. It is a real use case to control this parameter by user at runtime.

These are not serial port things, why would a serial port care about these?

You are right, these are not regular serial port things, but this device is not a regular uart, it is a sub-GHz transciever, digital radio. This driver tries to represent it as a serial port to user.

Quoted 7 lines
Quoted 5 lines
Quoted 3 lines

Is that the correct representation to be using here? Why not act like a proper radio device instead? That way you get to use the normal kernel apis for radio devices.

In my mind it is absolute a serial device by the application.

What is the application? Traditionally serial ports don't need radio signals :)

The application is connecting newly developed sensors(with only rf interface) and legacy sensors(with regular serial communication over rs-485 with modbus) keeping the legacy user software.

User sw [Java]

Quoted 9 lines
	<-> /dev/ttyXXX
		<-> si4455[driver]
			<-> si4455[hardware]
				<---air---> new device[si4455+ARM Cortex-M0] 1
					+-> new device[si4455+ARM Cortex-M0] 2
					+-> new device[si4455+ARM Cortex-M0] n
					+-> gateway[si4455+ARM Cortex-M0]<---RS485--> Legacy device 1
										  +-> Legacy device 2
										  +-> Legacy device n
Quoted 2 lines

If these are "sensors", why are you using a tty interface at all, and not just using the correct iio interface for them?

In this context "legacy sensor" means modbus protocol device over serial/rs485 interface, and "new sensors" means modbus protocol device over air(rf). All kind of devices are 50-100 m away from central(where linux using this serial driver).
The goal is to remove the wire between central(linux) and any kind of devices, but keeping the original user software.

Quoted 4 lines

thanks,

greg k-h

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