Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 9 Dec 2020 19:41:53 +0000
From:   =?iso-8859-1?Q?J=F3zsef_Horv=E1th?= <info@ministro.hu>
To:     =?iso-8859-1?B?Suly9G1l?= Pouiller <jerome.pouiller@silabs.com>
Cc:     'Rob Herring' <robh+dt@kernel.org>,
        'Greg Kroah-Hartman' <gregkh@linuxfoundation.org>,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        devel@driverdev.osuosl.org, driverdev-devel@linuxdriverproject.org
Subject: Re: [PATCH] Staging: silabs si4455 serial driver
Message-ID: <20201209194153.GC30918@dincontrollerdev>
References: <!&!AAAAAAAAAAAuAAAAAAAAAM7AkQxKEJRHh2BgMNSTrQkBAExvbAW64DNBoXXP8CRioZMAAAAzfOEAABAAAAAJUqiRO33GQqGIHffCVyG/AQAAAAA=@ministro.hu>
 <2907305.Mh6RI2rZIc@pc-42>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <2907305.Mh6RI2rZIc@pc-42>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk

Hello J?r?me,

On Wed, Dec 09, 2020 at 06:38:08PM +0100, J?r?me Pouiller wrote:
> 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 :)

I think great products :) and great support :)

> 
> 
> > Signed-off-by: J?zsef Horv?th <info@ministro.hu>
> 
> I think you have to use your personal address to sign-off.

I'm a self-employed, currently this is my "personal" e-mail address.

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

Si4455 is programmed by silabs.
In case of C2A, it is possible to load patch.

My solution is to loading EZConfig(generated by WDS) and firmware patch with ioctl calls.
You can see the definitions in si4455_api.h.
A short example for EZConfig loading(patch loading will be exacly the same if Si4455 is rev C2A):
	...
	#include "si4455_api.h"
	...
	#include "radio.h"			//< Generated by WDS3
	#include "radio_config_Si4455.h"	//< Generated by WDS3
	...
	struct si4455_iocbuff iocbuff = { 0 };
	iocbuff.length = sizeof(Radio_Configuration_Data_Array);
	memcpy(iocbuff.data, Radio_Configuration_Data_Array, iocbuff.length);
	ret = ioctl(portFd, SI4455_IOC_SEZC, &iocbuff);
	...

After SI4455_IOC_SEZC or SI4455_IOC_SEZP ioctl calls, the driver resets the Si4455, and apply the configuration/patch.

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

Yes you are right, but the uart subsystem maybe. I'll check it again, and if does not, I'll remove these definitions.

> > +               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 know Si4455 is not a regular UART, but in my mind it is a half-duplex serial transport channel, like UART with RS-485.
This is the reason why I designed it among serial drivers.
The special funcions like tx/rx channel, package size are controlled with ioctl calls.

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

I changed all like this in my code already. I test it, and I'll send it again.

Ps.: For my eyes is better to read line or list, reading table is harder :)

	line(arg1, arg2, arg3, arg4);

	list(arg1,
		arg2,
		arg3,
		arg4);

	table(arg1, arg2,
		arg3, arg4);


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

?dv?zlettel / Best regards:
J?zsef Horv?th