| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <2907305.Mh6RI2rZIc@pc-42>
Date: Wed, 09 Dec 2020 18:38:08 +0100
From: Jérôme Pouiller <jerome.pouiller@...abs.com>
To: 'Rob Herring' <robh+dt@...nel.org>,
'Greg Kroah-Hartman' <gregkh@...uxfoundation.org>,
'József Horváth' <info@...istro.hu>,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
devel@...verdev.osuosl.org, driverdev-devel@...uxdriverproject.org
Cc: Info <info@...istro.hu>
Subject: Re: [PATCH] Staging: silabs si4455 serial driver
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@...istro.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@...r.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
Powered by blists - more mailing lists