Re: [PATCH] Staging: silabs si4455 serial driver

["Jérôme Pouiller" <jerome.pouiller@silabs.com>]

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@ministro.hu>

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@vger.kernel.org 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