On Wednesday 9 December 2020 12:09:58 CET Info wrote:
>
> This is a serial port driver for
> Silicon Labs Si4455 Sub-GHz transciver.
Hello József,
Thank you for taking care of support of Silabs products :)
> Signed-off-by: József Horváth <info@xxxxxxxxxxx>
I think you have to use your personal address to sign-off.
> ---
> .../bindings/staging/serial/silabs,si4455.txt | 39 +
> drivers/staging/Kconfig | 2 +
> drivers/staging/Makefile | 1 +
> drivers/staging/si4455/Kconfig | 8 +
> drivers/staging/si4455/Makefile | 2 +
> drivers/staging/si4455/TODO | 3 +
> drivers/staging/si4455/si4455.c | 1465 +++++++++++++++++
> drivers/staging/si4455/si4455_api.h | 56 +
> 8 files changed, 1576 insertions(+)
> create mode 100644
> Documentation/devicetree/bindings/staging/serial/silabs,si4455.txt
> create mode 100644 drivers/staging/si4455/Kconfig
> create mode 100644 drivers/staging/si4455/Makefile
> create mode 100644 drivers/staging/si4455/TODO
> create mode 100644 drivers/staging/si4455/si4455.c
> create mode 100644 drivers/staging/si4455/si4455_api.h
Since you add a new directory, you should also update MAINTAINERS file
(checkpatch didn't warn you about that?).
> diff --git
> a/Documentation/devicetree/bindings/staging/serial/silabs,si4455.txt
> b/Documentation/devicetree/bindings/staging/serial/silabs,si4455.txt
> new file mode 100644
> index 000000000000..abd659b7b952
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/staging/serial/silabs,si4455.txt
> @@ -0,0 +1,39 @@
> +* Silicon Labs Si4455 EASY-TO-USE, LOW-CURRENT OOK/(G)FSK SUB-GHZ
> TRANSCEIVER
AFAIK, Si4455 is a programmable product. So I think that this driver only
work if the Si4455 use a specific firmware, isn't? In this case, you
should mention it in the documentation.
> +
> +Required properties:
> +- compatible: Should be one of the following:
> + - "silabs,si4455" for Silicon Labs Si4455-B1A or Si4455-C2A (driver
> automatically detects the part info),
> + - "silabs,si4455b1a" for Silicon Labs Si4455-B1A,
> + - "silabs,si4455c2a" for Silicon Labs Si4455-C2A,
> +- reg: SPI chip select number.
> +- interrupts: Specifies the interrupt source of the parent interrupt
> + controller. The format of the interrupt specifier depends on the
> + parent interrupt controller.
> +- clocks: phandle to the IC source clock (only external clock source
> supported).
> +- spi-max-frequency: maximum clock frequency on SPI port
> +- shdn-gpios: gpio pin for SDN
> +
> +Example:
> +
> +/ {
> + clocks {
> + si4455_1_2_osc: si4455_1_2_osc {
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency = <30000000>;
> + };
> + };
> +};
> +
> +&spi0 {
> + si4455: si4455@0 {
> + compatible = "silabs,si4455";
> + reg = <0>;
> + clocks = <&si4455_1_2_osc>;
It seems that the driver does not use this clock. So, is the clock
attribute mandatory? What is the purpose of declaring a fixed-clock
for this device?
> + interrupt-parent = <&gpio>;
> + interrupts = <7 IRQ_TYPE_LEVEL_LOW>;
> + shdn-gpios = <&gpio 26 1>;
> + status = "okay";
> + spi-max-frequency = <3000000>;
> + };
> +};
[...]
> diff --git a/drivers/staging/si4455/Kconfig b/drivers/staging/si4455/Kconfig
> new file mode 100644
> index 000000000000..666f726f2583
> --- /dev/null
> +++ b/drivers/staging/si4455/Kconfig
> @@ -0,0 +1,8 @@
> +# SPDX-License-Identifier: GPL-2.0
> +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.
So, in fact, Si4455 is not a UART. I don't know how this kind of device
should be presented to the userspace. Have you check if similar devices
already exists in the kernel?
I suggest to add linux-wpan@xxxxxxxxxxxxxxx to the recipients of your
patch.
[...]
> +static int si4455_get_part_info(struct uart_port *port,
> + struct si4455_part_info *result)
> +{
> + int ret;
> + u8 dataOut[] = { SI4455_CMD_ID_PART_INFO };
> + u8 dataIn[SI4455_CMD_REPLY_COUNT_PART_INFO];
> +
> + ret = si4455_send_command_get_response(port,
> + sizeof(dataOut),
> + dataOut,
> + sizeof(dataIn),
> + dataIn);
Why not:
ret = si4455_send_command_get_response(port,
sizeof(*result), result,
sizeof(dataIn), dataIn);
> + if (ret == 0) {
> + result->CHIPREV = dataIn[0];
> + memcpy(&result->PART, &dataIn[1],sizeof(result->PART));
> + result->PBUILD = dataIn[3];
> + memcpy(&result->ID, &dataIn[4], sizeof(result->ID));
> + result->CUSTOMER = dataIn[6];
> + result->ROMID = dataIn[7];
> + result->BOND = dataIn[8];
... it would avoid all these lines.
> + } else {
> + dev_err(port->dev,
> + "%s: si4455_send_command_get_response error(%i)",
> + __func__,
> + ret);
> + }
> + return ret;
> +}
[...]
--
Jérôme Pouiller
