Re: [PATCH v2] Serial: silabs si4455 serial driver

On Thu, Dec 10, 2020 at 07:46:25PM +0000, József Horváth wrote:

On Thu, Dec 10, 2020 at 08:03:22PM +0100, 'Greg Kroah-Hartman' wrote:

quoted

On Thu, Dec 10, 2020 at 05:04:46PM +0000, József Horváth wrote:

quoted

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

Signed-off-by: József Horváth <redacted>
---
 .../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

--- /dev/null
+++ b/drivers/tty/serial/si4455_api.h

@@ -0,0 +1,56 @@
+/* SPDX-License-Identifier: GPL-2.0
+ *
+ * Copyright (C) 2020 József Horváth <info@ministro.hu>
+ *
+ */
+#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

+};
+
+#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.

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

I can change it to BASE_TTYIOC or SI4455_IOC_BASE

quoted

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?

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