Staging: silabs si4455 serial driver

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

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

+#define SI4455_IOC_SSIZ		_IOW('T', BASE_TTYIOC_PRIVATE +
0x16, uint32_t)
+/* Get last rssi value. */
+#define SI4455_IOC_GRSSI	_IOR('T', BASE_TTYIOC_PRIVATE + 0x20,
uint32_t)
+
+#endif

On Wed, Dec 09, 2020 at 12:09:58PM +0100, Info wrote:
> This is a serial port driver for
> Silicon Labs Si4455 Sub-GHz transciver.
> 
> Signed-off-by: József Horváth <info@ministro.hu>

Note, your From: line does not match this line, so I can't take this.

But:

> ---
>  .../bindings/staging/serial/silabs,si4455.txt |   39 +

staging drivers need to be self-contained, so this should be here.  It
needs to be reviewed by the DT maintainers when moving out of staging.

> index 000000000000..aee5c7613b31
> --- /dev/null
> +++ b/drivers/staging/si4455/TODO
> @@ -0,0 +1,3 @@
> +TODO:
> +        - add variable packet length support
> +        - add firmware patching support in case of Si4455-C2A

Why are these a requirement to get it out of staging?  Why go into
staging at all?  Why not go into the 'real' part of the kernel directly?
What is keeping that from happening today?

These look like new features that you can add later, and shouldn't be a
requirement for acceptance into the normal part of the kernel for this
driver.  Why have you not tried doing that first?

thanks,

greg k-h

Change the From email header to say your name.

The patch is corrupted and can't be applied.  Please read the first
paragraphs of Documentation/process/email-clients.rst

This driver is pretty small and it might be easier to clean it up first
before merging it into staging.  Run checkpatch.pl --strict on the
driver.

> --- /dev/null
> +++ b/drivers/staging/si4455/si4455.c
> @@ -0,0 +1,1465 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2020 József Horváth <info@ministro.hu>
> + *
> + */
> +#include <linux/bitops.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/of_gpio.h>
> +#include <linux/regmap.h>
> +#include <linux/serial_core.h>
> +#include <linux/serial.h>
> +#include <linux/tty.h>
> +#include <linux/tty_flip.h>
> +#include <linux/spi/spi.h>
> +#include <linux/uaccess.h>
> +#include "si4455_api.h"
> +
> +#define PORT_SI4455						1096
> +#define SI4455_NAME						"si4455"
> +#define SI4455_MAJOR						432
> +#define SI4455_MINOR						567
> +#define SI4455_UART_NRMAX					1
> +#define SI4455_FIFO_SIZE					64
> +
> +#define SI4455_CMD_ID_EZCONFIG_CHECK				0x19
> +#define SI4455_CMD_ID_PART_INFO					0x01
> +#define SI4455_CMD_REPLY_COUNT_PART_INFO			9
> +#define SI4455_CMD_ID_GET_INT_STATUS				0x20
> +#define SI4455_CMD_REPLY_COUNT_GET_INT_STATUS			8
> +#define SI4455_CMD_ID_FIFO_INFO					0x15
> +#define SI4455_CMD_ARG_COUNT_FIFO_INFO				2
> +#define SI4455_CMD_REPLY_COUNT_FIFO_INFO			2
> +#define SI4455_CMD_FIFO_INFO_ARG_TX_BIT				0x01
> +#define SI4455_CMD_FIFO_INFO_ARG_RX_BIT				0x02
> +#define SI4455_CMD_ID_READ_CMD_BUFF				0x44
> +#define SI4455_CMD_ID_READ_RX_FIFO				0x77
> +#define SI4455_CMD_ID_WRITE_TX_FIFO				0x66
> +#define SI4455_CMD_ID_START_RX					0x32
> +#define SI4455_CMD_ARG_COUNT_START_RX				8
> +#define SI4455_CMD_START_RX_ARG_RXTIMEOUT_STATE_ENUM_RX		8
> +#define SI4455_CMD_START_RX_ARG_RXVALID_STATE_ENUM_RX		8
> +#define SI4455_CMD_START_RX_ARG_RXINVALID_STATE_ENUM_RX		8
> +#define SI4455_CMD_ID_START_TX					0x31
> +#define SI4455_CMD_ARG_COUNT_START_TX				5
> +#define SI4455_CMD_ID_CHANGE_STATE				0x34
> +#define SI4455_CMD_ARG_COUNT_CHANGE_STATE			2
> +#define SI4455_CMD_CHANGE_STATE_ARG_NEW_STATE_ENUM_READY	3
> +#define SI4455_CMD_GET_CHIP_STATUS_REP_CMD_ERROR_PEND_MASK	0x08
> +#define SI4455_CMD_GET_CHIP_STATUS_REP_CMD_ERROR_PEND_BIT	0x08
> +#define SI4455_CMD_GET_INT_STATUS_REP_PACKET_SENT_PEND_BIT	0x20
> +#define SI4455_CMD_GET_INT_STATUS_REP_PACKET_RX_PEND_BIT	0x10
> +#define SI4455_CMD_GET_INT_STATUS_REP_CRC_ERROR_BIT		0x08

These names are unreadably long and just make the code messy.

> +#define SI4455_CMD_ID_GET_MODEM_STATUS				0x22
> +#define SI4455_CMD_ARG_COUNT_GET_MODEM_STATUS			2
> +#define SI4455_CMD_REPLY_COUNT_GET_MODEM_STATUS			8
> +
> +struct si4455_part_info {
> +	u8				CHIPREV;
> +	u16				PART;
> +	u8				PBUILD;
> +	u16				ID;
> +	u8				CUSTOMER;
> +	u8				ROMID;
> +	u8				BOND;

Also the huge gap between the type and the struct member makes it
hard to read.

	u8  chip_rev;
	u16 part;
	u8  pbuild;

etc.

> +};
> +
> +struct si4455_int_status {
> +	u8				INT_PEND;
> +	u8				INT_STATUS;
> +	u8				PH_PEND;
> +	u8				PH_STATUS;
> +	u8				MODEM_PEND;
> +	u8				MODEM_STATUS;
> +	u8				CHIP_PEND;
> +	u8				CHIP_STATUS;
> +};
> +
> +struct si4455_modem_status {
> +	u8				MODEM_PEND;
> +	u8				MODEM_STATUS;
> +	u8				CURR_RSSI;
> +	u8				LATCH_RSSI;
> +	u8				ANT1_RSSI;
> +	u8				ANT2_RSSI;
> +	u16			AFC_FREQ_OFFSET;
> +};
> +
> +struct si4455_fifo_info {
> +	u8				RX_FIFO_COUNT;
> +	u8				TX_FIFO_SPACE;
> +};
> +
> +struct si4455_one {
> +	struct uart_port		port;
> +	struct work_struct		tx_work;
> +};
> +
> +struct si4455_port {
> +	struct mutex			mutex;
> +	int				power_count;
> +	struct gpio_desc		*shdn_gpio;
> +	struct si4455_part_info		part_info;
> +	struct si4455_modem_status	modem_status;
> +	struct si4455_iocbuff		configuration;
> +	struct si4455_iocbuff		patch;
> +	u32				tx_channel;
> +	u32				rx_channel;
> +	u32				package_size;
> +	bool				configured;
> +	bool				tx_pending;
> +	bool				rx_pending;
> +	u32				current_rssi;
> +	struct si4455_one		one;
> +};

Since the struct is not defined by the spec then don't bother aligning
the members.  It just makes changing the code complicated because you
have to re-align stuff.  Just put a single space between the type and
the name.


> +
> +static struct uart_driver si4455_uart = {
> +	.owner			= THIS_MODULE,
> +	.driver_name		= SI4455_NAME,
> +#ifdef CONFIG_DEVFS_FS
> +	.dev_name		= "ttySI%d",
> +#else
> +	.dev_name		= "ttySI",
> +#endif
> +	.major			= SI4455_MAJOR,
> +	.minor			= SI4455_MINOR,
> +	.nr			= SI4455_UART_NRMAX,
> +};
> +
> +static int si4455_get_response(struct uart_port *port,
> +				int length,
> +				u8 *data);

Remove unecessary declarations like this.

> +static int si4455_send_command(struct uart_port *port,
> +				int length,
> +				u8 *data);
> +static int si4455_send_command_get_response(struct uart_port *port,
> +						int outLength,
> +						u8 *outData,
> +						int inLength,
> +						u8 *inData);
> +static int si4455_read_data(struct uart_port *port,
> +				u8 command,
> +				int pollCts,
> +				int length,
> +				u8 *data);
> +static int si4455_write_data(struct uart_port *port,
> +				u8 command,
> +				int pollCts,
> +				int length,
> +				u8 *data);
> +static int si4455_poll_cts(struct uart_port *port);
> +static void si4455_set_power(struct si4455_port *priv,
> +				int on);
> +static int si4455_get_part_info(struct uart_port *port,
> +				struct si4455_part_info *result);
> +
> +static int si4455_get_response(struct uart_port *port,
> +				int length,
> +				u8 *data)
> +{
> +	int ret = -EIO;

Remove unecessary initializations.  It disables static checkers' ability
to find uninitialized variable bugs so it leads to bugs.

> +	u8 dataOut[] = { SI4455_CMD_ID_READ_CMD_BUFF };
> +	u8 *dataIn = devm_kzalloc(port->dev, 1 + length, GFP_KERNEL);

Never put functions which can fail in the declaration block.  It leads
to "forgot to check for NULL bugs" which is the case here.

Don't use devm_ for this.  It has a different lifetime.  Use normal
kzalloc().

> +	struct spi_transfer xfer[] = {
> +		{
> +			.tx_buf = dataOut,
> +			.len = sizeof(dataOut),
> +		}, {
> +			.rx_buf = dataIn,
> +			.len = 1 + length,
> +		}
> +	};
> +	int errCnt = 10000;
> +
> +	while (errCnt > 0) {

while (--cnt > 9) {

> +		dataOut[0] = SI4455_CMD_ID_READ_CMD_BUFF;
> +		ret = spi_sync_transfer(to_spi_device(port->dev),
> +					xfer,
> +					ARRAY_SIZE(xfer));
> +		if (ret == 0) {

Always do Error Handling as opposed to success handling.

		if (ret)
			break;

> +			if (dataIn[0] == 0xFF) {
> +				if ((length > 0) && (data != NULL)) {
> +					memcpy(data, &dataIn[1], length);
> +				} else {
> +					if (length > 0)
> +						ret = -EINVAL;
> +				}
> +				break;
> +			}
> +			usleep_range(100, 200);
> +		} else {
> +			break;
> +		}
> +		errCnt--;
> +	}
> +	if (errCnt == 0) {
> +		dev_err(port->dev, "si4455_poll_cts:errCnt==%i", errCnt);
> +		ret = -EIO;
> +	}
> +	if (dataIn != NULL)
> +		devm_kfree(port->dev, dataIn);
> +	return ret;
> +}
> +
> +static int si4455_send_command(struct uart_port *port,
> +				int length,
> +				u8 *data)
> +{
> +	int ret;
> +
> +	ret = si4455_poll_cts(port);
> +	if (ret == 0) {

	ret = si4455_poll_cts(port);
	if (ret) {
		dev_err(port->dev, "%s: si4455_poll_cts error(%i)", __func__,
			ret);
		return ret;
	}

All the checks need to be reversed.  The printk only needs two lines.


> +		ret = spi_write(to_spi_device(port->dev), data, length);
> +		if (ret) {
> +			dev_err(port->dev,
> +				"%s: spi_write error(%i)",
> +				__func__,
> +				ret);
> +		}
> +	} else {
> +		dev_err(port->dev,
> +			"%s: si4455_poll_cts error(%i)",
> +			__func__,
> +			ret);
> +	}
> +	return ret;
> +}
> +
> +static int si4455_send_command_get_response(struct uart_port *port,
> +						int outLength,
> +						u8 *outData,
> +						int inLength,
> +						u8 *inData)
> +{
> +	int ret;
> +
> +	ret = si4455_send_command(port, outLength, outData);
> +	if (!ret) {
> +		ret = si4455_get_response(port, inLength, inData);
> +	} else {
> +		dev_err(port->dev,
> +			"%s: si4455_send_command error(%i)",
> +			__func__,
> +			ret);
> +	}
> +	return ret;
> +}
> +
> +static int si4455_read_data(struct uart_port *port,
> +				u8 command,
> +				int pollCts,
> +				int length,
> +				u8 *data)
> +{
> +	int ret = 0;
> +	u8 dataOut[] = { command };
> +	struct spi_transfer xfer[] = {
> +		{
> +			.tx_buf = dataOut,
> +			.len = sizeof(dataOut),
> +		}, {
> +			.rx_buf = data,
> +			.len = length,
> +		}
> +	};
> +
> +	if (pollCts)
> +		ret = si4455_poll_cts(port);

	if (poll) {
		ret = si4455_poll_cts(port);
		if (ret)
			return ret;
	}

> +
> +	if (ret == 0) {
> +		ret = spi_sync_transfer(to_spi_device(port->dev),
> +					xfer,
> +					ARRAY_SIZE(xfer));
> +		if (ret) {
> +			dev_err(port->dev,
> +				"%s: spi_sync_transfer error(%i)",
> +				__func__,
> +				ret);
> +		}
> +	}
> +	return ret;
> +}
> +
> +static int si4455_write_data(struct uart_port *port,
> +				u8 command,
> +				int pollCts,
> +				int length,
> +				u8 *data)
> +{
> +	int ret = 0;
> +	u8 *dataOut = NULL;
> +
> +	if (pollCts)
> +		ret = si4455_poll_cts(port);
> +
> +	if (ret == 0) {
> +		dataOut = devm_kzalloc(port->dev, 1 + length, GFP_KERNEL);

Use vanilla kzalloc().

> +		if (dataOut != NULL) {
> +			dataOut[0] = command;
> +			memcpy(&dataOut[1], data, length);
> +			ret = spi_write(to_spi_device(port->dev),
> +					dataOut,
> +					1 + length);
> +			if (ret) {
> +				dev_err(port->dev,
> +					"%s: spi_write error(%i)",
> +					__func__,
> +					ret);
> +			}
> +		} else {
> +			dev_err(port->dev,
> +				"%s: devm_kzalloc error",
> +				__func__);
> +			ret = -ENOMEM;
> +		}
> +	}
> +	if (dataOut != NULL)
> +		devm_kfree(port->dev, dataOut);
> +
> +	return ret;
> +}
> +
> +static int si4455_poll_cts(struct uart_port *port)
> +{
> +	return si4455_get_response(port, 0, NULL);
> +}
> +
> +static void si4455_set_power(struct si4455_port *priv,
> +				int on)
> +{
> +	if (priv->shdn_gpio) {

Reverse this test:

	if (!priv->shdn_gpio)
		return;

> +		if (on) {
> +			gpiod_direction_output(priv->shdn_gpio, 0);
> +			usleep_range(4000, 5000);
> +			gpiod_set_value(priv->shdn_gpio, 1);
> +			usleep_range(4000, 5000);
> +		} else {
> +			gpiod_direction_output(priv->shdn_gpio, 0);
> +		}
> +	}
> +}
> +
> +static int si4455_s_power(struct device *dev,
> +				int on)
> +{
> +	struct si4455_port *s = dev_get_drvdata(dev);
> +
> +	dev_dbg(dev, "%s(on=%d)\n", __func__, on);
> +	if (s->power_count == !on)
> +		si4455_set_power(s, on);
> +	s->power_count += on ? 1 : -1;
> +	WARN_ON(s->power_count < 0);
> +
> +	return 0;
> +}
> +
> +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);

This can fit on two lines:

	ret = si4455_send_command_get_response(port, sizeof(dataOut), dataOut,
					       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];
> +	} else {
> +		dev_err(port->dev,
> +			"%s: si4455_send_command_get_response error(%i)",
> +			__func__,
> +			ret);
> +	}
> +	return ret;
> +}
> +
> +static int si4455_get_int_status(struct uart_port *port,
> +					u8 phClear,
> +					u8 modemClear,
> +					u8 chipClear,
> +					struct si4455_int_status *result)
> +{
> +	int ret;
> +	u8 dataOut[] = {
> +		SI4455_CMD_ID_GET_INT_STATUS,
> +		phClear,
> +		modemClear,
> +		chipClear
> +	};
> +	u8 dataIn[SI4455_CMD_REPLY_COUNT_GET_INT_STATUS];
> +
> +	ret = si4455_send_command_get_response(port,
> +						sizeof(dataOut),
> +						dataOut,
> +						sizeof(dataIn),
> +						dataIn);
> +	if (ret == 0) {
> +		result->INT_PEND       = dataIn[0];
> +		result->INT_STATUS     = dataIn[1];
> +		result->PH_PEND        = dataIn[2];
> +		result->PH_STATUS      = dataIn[3];
> +		result->MODEM_PEND     = dataIn[4];
> +		result->MODEM_STATUS   = dataIn[5];
> +		result->CHIP_PEND      = dataIn[6];
> +		result->CHIP_STATUS    = dataIn[7];
> +	} else {
> +		dev_err(port->dev,
> +			"%s: si4455_send_command_get_response error(%i)",
> +			__func__,
> +			ret);
> +	}
> +	return ret;
> +}
> +
> +static int si4455_get_modem_status(struct uart_port *port,
> +					u8 modemClear,
> +					struct si4455_modem_status *result)
> +{
> +	int ret;
> +	u8 dataOut[] = {
> +		SI4455_CMD_ID_GET_MODEM_STATUS,
> +		modemClear,
> +	};
> +	u8 dataIn[SI4455_CMD_REPLY_COUNT_GET_MODEM_STATUS];
> +
> +	ret = si4455_send_command_get_response(port,
> +						sizeof(dataOut),
> +						dataOut,
> +						sizeof(dataIn),
> +						dataIn);
> +	if (ret == 0) {
> +		result->MODEM_PEND      = dataIn[0];
> +		result->MODEM_STATUS    = dataIn[1];
> +		result->CURR_RSSI       = dataIn[2];
> +		result->LATCH_RSSI      = dataIn[3];
> +		result->ANT1_RSSI       = dataIn[4];
> +		result->ANT2_RSSI       = dataIn[5];
> +		memcpy(&result->AFC_FREQ_OFFSET,
> +			&dataIn[6],
> +			sizeof(result->AFC_FREQ_OFFSET));
> +	} else {
> +		dev_err(port->dev,
> +			"%s: si4455_send_command_get_response error(%i)",
> +			__func__,
> +			ret);
> +	}
> +	return ret;
> +}
> +
> +static int si4455_fifo_info(struct uart_port *port,
> +				u8 fifo,
> +				struct si4455_fifo_info *result)
> +{
> +	int ret = 0;
> +	u8 dataOut[SI4455_CMD_ARG_COUNT_FIFO_INFO] = {
> +		SI4455_CMD_ID_FIFO_INFO, fifo
> +	};
> +	u8 dataIn[SI4455_CMD_REPLY_COUNT_FIFO_INFO] = { 0 };
> +
> +	ret = si4455_send_command_get_response(port,
> +						sizeof(dataOut),
> +						dataOut,
> +						sizeof(dataIn),
> +						dataIn);
> +	if (ret == 0) {
> +		result->RX_FIFO_COUNT  = dataIn[0];
> +		result->TX_FIFO_SPACE  = dataIn[1];
> +	} else {
> +		dev_err(port->dev,
> +			"%s: si4455_send_command_get_response error(%i)",
> +			__func__,
> +			ret);
> +	}
> +	return ret;
> +}
> +
> +static int si4455_read_rx_fifo(struct uart_port *port,
> +				int length,
> +				u8 *data)
> +{
> +	return si4455_read_data(port,
> +				SI4455_CMD_ID_READ_RX_FIFO,
> +				0,
> +				length,
> +				data);
> +}
> +
> +static int si4455_write_tx_fifo(struct uart_port *port,
> +				int length,
> +				u8 *data)
> +{
> +	return si4455_write_data(port,
> +					SI4455_CMD_ID_WRITE_TX_FIFO,
> +					0,
> +					length,
> +					data);
> +}
> +
> +static int si4455_rx(struct uart_port *port,
> +			u32 channel,
> +			u8 condition,
> +			u16 length,
> +			u8 nextState1,
> +			u8 nextState2,
> +			u8 nextState3)
> +{
> +	u8 dataOut[SI4455_CMD_ARG_COUNT_START_RX];
> +
> +	dataOut[0] = SI4455_CMD_ID_START_RX;
> +	dataOut[1] = channel;
> +	dataOut[2] = condition;
> +	dataOut[3] = (u8)(length >> 8);
> +	dataOut[4] = (u8)(length);
> +	dataOut[5] = nextState1;
> +	dataOut[6] = nextState2;
> +	dataOut[7] = nextState3;
> +
> +	return si4455_send_command(port,
> +					SI4455_CMD_ARG_COUNT_START_RX,
> +					dataOut);
> +}
> +
> +static int si4455_tx(struct uart_port *port,
> +			u8 channel,
> +			u8 condition,
> +			u16 length)
> +{
> +	u8 dataOut[/*6*/ SI4455_CMD_ARG_COUNT_START_TX];
> +
> +	dataOut[0] = SI4455_CMD_ID_START_TX;
> +	dataOut[1] = channel;
> +	dataOut[2] = condition;
> +	dataOut[3] = (u8)(length >> 8);
> +	dataOut[4] = (u8)(length);
> +	/*TODO: radioCmd[5] = 0x44; in case of rev c2a*/
> +
> +	return si4455_send_command(port,
> +					/*6*/ SI4455_CMD_ARG_COUNT_START_TX,
> +					dataOut);
> +}
> +
> +static int si4455_change_state(struct uart_port *port,
> +				u8 nextState1)
> +{
> +	u8 dataOut[SI4455_CMD_ARG_COUNT_CHANGE_STATE];
> +
> +	dataOut[0] = SI4455_CMD_ID_CHANGE_STATE;
> +	dataOut[1] = (u8)nextState1;
> +
> +	return si4455_send_command(port,
> +					SI4455_CMD_ARG_COUNT_CHANGE_STATE,
> +					dataOut);
> +}
> +
> +static int si4455_begin_tx(struct uart_port *port,
> +				u32 channel,
> +				int length,
> +				u8 *data)
> +{
> +	int ret = 0;
> +	struct si4455_int_status intStatus = { 0 };
> +	struct si4455_fifo_info fifoInfo = { 0 };
> +
> +	dev_dbg(port->dev, "%s(%u, %u)", __func__, channel, length);
> +	if ((length > SI4455_FIFO_SIZE) || (length < 0))
> +		ret = -EINVAL;
> +
> +	if (ret == 0) {
> +		ret = si4455_change_state(port,
> +			SI4455_CMD_CHANGE_STATE_ARG_NEW_STATE_ENUM_READY);
> +		if (ret) {
> +			dev_err(port->dev,
> +				"%s: si4455_change_state error(%i)",
> +				__func__,
> +				ret);
> +			goto end;
> +		}
> +		ret = si4455_get_int_status(port, 0, 0, 0, &intStatus);
> +		if (ret) {
> +			dev_err(port->dev,
> +				"%s: si4455_get_int_status error(%i)",
> +				__func__,
> +				ret);
> +			goto end;
> +		}
> +		ret = si4455_fifo_info(port,
> +					SI4455_CMD_FIFO_INFO_ARG_TX_BIT,
> +					&fifoInfo);
> +		if (ret) {
> +			dev_err(port->dev,
> +				"%s: si4455_fifo_info error(%i)",
> +				__func__,
> +				ret);
> +			goto end;
> +		}
> +		ret = si4455_write_tx_fifo(port, (u16)length, data);

No need to cast this.

> +		if (ret) {
> +			dev_err(port->dev,
> +				"%s: si4455_write_tx_fifo error(%i)",
> +				__func__,
> +				ret);
> +			goto end;

Just return directly.  Do nothing gotos just end up introducing "Forgot
to set the error code" bugs.

> +		}
> +		ret = si4455_tx(port,
> +				(u8)channel,
> +				0x30,
> +				(u16)length);
> +		if (ret) {
> +			dev_err(port->dev,
> +				"%s: si4455_tx error(%i)",
> +				__func__,
> +				ret);
> +			goto end;
> +		}
> +	}
> +end:
> +	return ret;
> +}
> +
> +static int si4455_end_tx(struct uart_port *port)
> +{
> +	int ret = 0;
> +	struct si4455_int_status intStatus = { 0 };
> +
> +	ret = si4455_change_state(port,
> +			SI4455_CMD_CHANGE_STATE_ARG_NEW_STATE_ENUM_READY);
> +	if (ret) {
> +		dev_err(port->dev,
> +			"%s: si4455_change_state error(%i)",
> +			__func__,
> +			ret);
> +		goto end;
> +	}
> +	ret = si4455_get_int_status(port, 0, 0, 0, &intStatus);
> +	if (ret) {
> +		dev_err(port->dev,
> +			"%s: si4455_get_int_status error(%i)",
> +			__func__,
> +			ret);
> +		goto end;
> +	}
> +end:
> +	return ret;
> +}
> +
> +static int si4455_begin_rx(struct uart_port *port,
> +				u32 channel,
> +				u32 length)
> +{
> +	int ret = 0;
> +	struct si4455_int_status intStatus = { 0 };
> +	struct si4455_fifo_info fifoInfo = { 0 };
> +
> +	dev_dbg(port->dev, "%s(%u, %u)", __func__, channel, length);
> +	ret = si4455_get_int_status(port, 0, 0, 0, &intStatus);
> +	if (ret) {
> +		dev_err(port->dev,
> +			"%s: si4455_get_int_status error(%i)",
> +			__func__,
> +			ret);
> +		goto end;
> +	}
> +	ret = si4455_fifo_info(port,
> +				SI4455_CMD_FIFO_INFO_ARG_RX_BIT,
> +				&fifoInfo);
> +	if (ret) {
> +		dev_err(port->dev,
> +			"%s: si4455_fifo_info error(%i)",
> +			__func__,
> +			ret);
> +		goto end;
> +	}
> +	ret = si4455_rx(port,
> +			channel,
> +			0x00,
> +			length,
> +			SI4455_CMD_START_RX_ARG_RXTIMEOUT_STATE_ENUM_RX,
> +			SI4455_CMD_START_RX_ARG_RXVALID_STATE_ENUM_RX,
> +			SI4455_CMD_START_RX_ARG_RXINVALID_STATE_ENUM_RX);
> +	if (ret) {
> +		dev_err(port->dev,
> +			"%s: si4455_rx error(%i)",
> +			__func__,
> +			ret);
> +		goto end;
> +	}
> +end:
> +	return ret;
> +}
> +
> +static int si4455_end_rx(struct uart_port *port,
> +				u32 length,
> +				u8 *data)
> +{
> +	return si4455_read_rx_fifo(port, length, data);
> +}
> +
> +static int si4455_configure(struct uart_port *port,
> +				u8 *configurationData)
> +{
> +	int ret = 0;
> +	u8 col;
> +	u8 response;
> +	u8 numOfBytes;
> +	struct si4455_int_status intStatus = { 0 };
> +	u8 radioCmd[16u];
> +
> +	/* While cycle as far as the pointer points to a command */
> +	while (*configurationData != 0x00) {
> +		/* Commands structure in the array:
> +		 * --------------------------------
> +		 * LEN | <LEN length of data>
> +		 */
> +		numOfBytes = *configurationData++;
> +		dev_dbg(port->dev,
> +			"%s: numOfBytes(%u)",
> +			__func__,
> +			numOfBytes);
> +		if (numOfBytes > 16u) {
> +			/* Initial configuration of Si4x55 */
> +			if (SI4455_CMD_ID_WRITE_TX_FIFO
> +				 == *configurationData) {
> +				if (numOfBytes > 128u) {
> +					/* Number of command bytes exceeds
> +					 * maximal allowable length
> +					 */
> +					dev_err(port->dev,
> +						"%s: command length
> error(%i)",
> +						__func__,
> +						numOfBytes);
> +					ret = -EINVAL;
> +					break;
> +				}
> +
> +				/* Load array to the device */
> +				configurationData++;
> +				ret = si4455_write_data(port,
> +						SI4455_CMD_ID_WRITE_TX_FIFO,
> +						1,
> +						numOfBytes - 1,
> +						configurationData);
> +				if (ret) {
> +					dev_err(port->dev,
> +						"%s: si4455_write_data
> error(%i)",
> +						__func__,
> +						ret);
> +					break;
> +				}
> +
> +				/* Point to the next command */
> +				configurationData += numOfBytes - 1;
> +
> +				/* Continue command interpreter */
> +				continue;
> +			} else {
> +				/* Number of command bytes exceeds
> +				 * maximal allowable length
> +				 */
> +				ret = -EINVAL;
> +				break;
> +			}
> +		}
> +
> +		for (col = 0u; col < numOfBytes; col++) {
> +			radioCmd[col] = *configurationData;
> +			configurationData++;
> +		}
> +
> +		dev_dbg(port->dev,
> +			"%s: radioCmd[0](%u)",
> +			__func__,
> +			radioCmd[0]);
> +		ret = si4455_send_command_get_response(port,
> +							numOfBytes,
> +							radioCmd,
> +							1,
> +							&response);
> +		if (ret) {
> +			dev_err(port->dev,
> +				"%s: si4455_send_command_get_response
> error(%i)",
> +				__func__,
> +				ret);
> +			break;
> +		}
> +
> +		/* Check response byte of EZCONFIG_CHECK command */
> +		if (radioCmd[0] == SI4455_CMD_ID_EZCONFIG_CHECK) {
> +			if (response) {
> +				/* Number of command bytes exceeds
> +				 * maximal allowable length
> +				 */
> +				ret = -EIO;
> +				dev_err(port->dev,
> +					"%s: EZConfig check error(%i)",
> +					__func__,
> +					radioCmd[0]);
> +				break;
> +			}
> +		}
> +
> +		/* Get and clear all interrupts.  An error has occurred...
> */
> +		si4455_get_int_status(port, 0, 0, 0, &intStatus);
> +		if (intStatus.CHIP_PEND
> +			&
> SI4455_CMD_GET_CHIP_STATUS_REP_CMD_ERROR_PEND_MASK) {
> +			ret = -EIO;
> +			dev_err(port->dev,
> +				"%s: chip error(%i)",
> +				__func__,
> +				intStatus.CHIP_PEND);
> +			break;
> +		}
> +	}
> +
> +	return ret;
> +}
> +
> +static int si4455_re_configure(struct uart_port *port)
> +{
> +	int ret = 0;
> +	struct si4455_port *s = dev_get_drvdata(port->dev);
> +
> +	mutex_lock(&s->mutex);
> +	s->configured = 0;
> +	if (s->power_count > 0)
> +		si4455_s_power(port->dev, 0);
> +
> +	si4455_s_power(port->dev, 1);
> +	if (s->configuration.length > 0) {
> +		ret = si4455_configure(port, s->configuration.data);
> +		if (ret == 0)
> +			s->configured = 1;
> +
> +	}
> +	mutex_unlock(&s->mutex);
> +	return ret;
> +}
> +
> +static int si4455_do_work(struct uart_port *port)
> +{
> +	int ret = 0;
> +	struct si4455_port *s = dev_get_drvdata(port->dev);
> +	struct circ_buf *xmit = &port->state->xmit;
> +	unsigned int tx_pending = 0;
> +	unsigned int tx_to_end = 0;
> +	u8 *data = NULL;
> +
> +	mutex_lock(&s->mutex);
> +	if (s->configured && (s->power_count > 0)) {
> +		if (!(uart_circ_empty(xmit)
> +			|| uart_tx_stopped(port)
> +			|| s->tx_pending)) {
> +			tx_pending = uart_circ_chars_pending(xmit);
> +			if (s->package_size > 0) {
> +				if (tx_pending >= s->package_size) {
> +					tx_pending = s->package_size;
> +					data = devm_kzalloc(port->dev,
> +						s->package_size,
> +						GFP_KERNEL);
> +					tx_to_end =
> CIRC_CNT_TO_END(xmit->head,
> +								xmit->tail,
> +
> UART_XMIT_SIZE);
> +					if (tx_to_end < tx_pending) {
> +						memcpy(data,
> +							xmit->buf +
> xmit->tail,
> +							tx_to_end);
> +						memcpy(data,
> +							xmit->buf,
> +							tx_pending -
> tx_to_end);
> +					} else {
> +						memcpy(data,
> +							xmit->buf +
> xmit->tail,
> +							tx_pending);
> +					}
> +					if (si4455_begin_tx(port,
> +							s->tx_channel,
> +							tx_pending,
> +							data) == 0) {
> +						s->tx_pending = true;
> +					}
> +					devm_kfree(port->dev, data);
> +				}
> +			} else {
> +				//TODO: variable packet length
> +			}
> +		}
> +		if (!s->tx_pending) {
> +			if (s->package_size > 0) {
> +				ret = si4455_begin_rx(port,
> +							s->rx_channel,
> +							s->package_size);
> +			} else {
> +				//TODO: variable packet length
> +			}
> +		}
> +	}
> +	mutex_unlock(&s->mutex);
> +	return ret;
> +}
> +
> +static void si4455_handle_rx_pend(struct si4455_port *s)
> +{
> +	struct uart_port *port = &s->one.port;
> +	u8 *data = NULL;
> +	int sret = 0;
> +	int i = 0;
> +
> +	if (s->package_size > 0) {
> +		data = devm_kzalloc(port->dev,
> +					s->package_size,
> +					GFP_KERNEL);
> +		sret = si4455_end_rx(port,
> +				s->package_size,
> +				data);
> +		if (sret == 0) {
> +			for (i = 0; i < s->package_size; i++) {
> +				uart_insert_char(port,
> +					0,
> +					0,
> +					data[i],
> +					TTY_NORMAL);
> +				port->icount.rx++;
> +			}
> +			tty_flip_buffer_push(&port->state->port);
> +		} else {
> +			dev_err(port->dev,
> +				"%s: si4455_end_rx error(%i)",
> +				__func__,
> +				sret);
> +		}
> +		devm_kfree(port->dev, data);
> +	} else {
> +		//TODO: variable packet length
> +	}
> +}
> +
> +static void si4455_handle_tx_pend(struct si4455_port *s)
> +{
> +	struct uart_port *port = &s->one.port;
> +	struct circ_buf *xmit = &port->state->xmit;
> +
> +	if (s->tx_pending) {
> +		if (s->package_size) {
> +			port->icount.tx += s->package_size;
> +			xmit->tail = (xmit->tail + s->package_size)
> +					& (UART_XMIT_SIZE - 1);
> +		} else {
> +			//TODO: variable packet length
> +		}
> +		si4455_end_tx(port);
> +		s->tx_pending = 0;
> +	}
> +}
> +
> +static irqreturn_t si4455_port_irq(struct si4455_port *s)
> +{
> +	struct uart_port *port = &s->one.port;
> +	irqreturn_t ret = IRQ_NONE;
> +	struct si4455_int_status intStatus = { 0 };
> +	struct si4455_fifo_info fifoInfo = { 0 };
> +
> +	mutex_lock(&s->mutex);
> +	if (s->configured && (s->power_count > 0)) {
> +		if (!si4455_get_int_status(port, 0, 0, 0, &intStatus)) {
> +			si4455_get_modem_status(port, 0, &s->modem_status);
> +			if (intStatus.CHIP_PEND
> +			& SI4455_CMD_GET_CHIP_STATUS_REP_CMD_ERROR_PEND_BIT)
> {
> +				dev_err(port->dev,
> +					"%s: CHIP_STATUS:CMD_ERROR_PEND",
> +					__func__);
> +			} else if (intStatus.PH_PEND
> +			&
> SI4455_CMD_GET_INT_STATUS_REP_PACKET_SENT_PEND_BIT) {
> +				dev_dbg(port->dev,
> +					"%s: PH_STATUS:PACKET_SENT_PEND",
> +					__func__);
> +				si4455_handle_tx_pend(s);
> +			} else if (intStatus.PH_PEND
> +			& SI4455_CMD_GET_INT_STATUS_REP_PACKET_RX_PEND_BIT)
> {
> +				dev_dbg(port->dev,
> +					"%s: PH_STATUS:PACKET_RX_PEND",
> +					__func__);
> +				s->current_rssi = s->modem_status.CURR_RSSI;
> +				si4455_fifo_info(port, 0, &fifoInfo);
> +				si4455_handle_rx_pend(s);
> +			} else if (intStatus.PH_PEND
> +			& SI4455_CMD_GET_INT_STATUS_REP_CRC_ERROR_BIT) {
> +				dev_dbg(port->dev,
> +					"%s: PH_STATUS:CRC_ERROR_PEND",
> +					__func__);
> +			}
> +			ret = IRQ_HANDLED;
> +		}
> +	} else {
> +		ret = IRQ_HANDLED;
> +	}
> +	mutex_unlock(&s->mutex);
> +	si4455_do_work(port);
> +	return ret;
> +}
> +
> +static irqreturn_t si4455_ist(int irq,
> +				void *dev_id)
> +{
> +	struct si4455_port *s = (struct si4455_port *)dev_id;
> +	bool handled = false;
> +
> +	if (si4455_port_irq(s) == IRQ_HANDLED)
> +		handled = true;
> +
> +	return IRQ_RETVAL(handled);
> +}
> +
> +static void si4455_tx_proc(struct work_struct *ws)
> +{
> +	struct si4455_one *one = container_of(ws,
> +					struct si4455_one,
> +					tx_work);
> +
> +	si4455_do_work(&one->port);
> +}
> +
> +static unsigned int si4455_tx_empty(struct uart_port *port)
> +{
> +	return TIOCSER_TEMT;
> +}
> +
> +static unsigned int si4455_get_mctrl(struct uart_port *port)
> +{
> +	dev_dbg(port->dev, "%s", __func__);
> +	return TIOCM_DSR | TIOCM_CAR;
> +}
> +
> +static void si4455_set_mctrl(struct uart_port *port,
> +				unsigned int mctrl)
> +{
> +	dev_dbg(port->dev, "%s", __func__);
> +}

Delete all these empty functions.

> +
> +static void si4455_break_ctl(struct uart_port *port,
> +				int break_state)
> +{
> +	dev_dbg(port->dev, "%s", __func__);
> +}
> +
> +static void si4455_set_termios(struct uart_port *port,
> +				struct ktermios *termios,
> +				struct ktermios *old)
> +{
> +	dev_dbg(port->dev, "%s", __func__);
> +}
> +
> +static int si4455_rs485_config(struct uart_port *port,
> +				struct serial_rs485 *rs485)
> +{
> +	dev_dbg(port->dev, "%s", __func__);
> +
> +	return 0;
> +}
> +
> +static int si4455_startup(struct uart_port *port)
> +{
> +	dev_dbg(port->dev, "%s", __func__);
> +	si4455_s_power(port->dev, 1);
> +	return 0;
> +}
> +
> +static void si4455_shutdown(struct uart_port *port)
> +{
> +	dev_dbg(port->dev, "%s", __func__);
> +	si4455_s_power(port->dev, 0);
> +}
> +
> +static const char *si4455_type(struct uart_port *port)
> +{
> +	struct si4455_port *s = dev_get_drvdata(port->dev);
> +
> +	if (port->type == PORT_SI4455) {

Reverse this test.

> +		if (s->part_info.ROMID == 3)
> +			return "SI4455-B1A";
> +		else if (s->part_info.ROMID == 6)
> +			return "SI4455-C2A";
> +
> +	}
> +	return NULL;
> +}
> +
> +static int si4455_request_port(struct uart_port *port)
> +{
> +	/* Do nothing */
> +	dev_dbg(port->dev, "%s", __func__);
> +	return 0;
> +}
> +
> +static void si4455_config_port(struct uart_port *port,
> +				int flags)
> +{
> +	dev_dbg(port->dev, "%s", __func__);

Delete all these debug statements which only print the name of the
function.  Use ftrace for this instead.

> +	if (flags & UART_CONFIG_TYPE)
> +		port->type = PORT_SI4455;
> +}
> +
> +static int si4455_verify_port(struct uart_port *port,
> +				struct serial_struct *s)
> +{
> +	if ((s->type != PORT_UNKNOWN) && (s->type != PORT_SI4455))
> +		return -EINVAL;
> +
> +	if (s->irq != port->irq)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static void si4455_start_tx(struct uart_port *port)
> +{
> +	struct si4455_one *one = container_of(port,
> +					struct si4455_one,
> +					port);
> +
> +	dev_dbg(port->dev, "%s", __func__);
> +
> +	if (!work_pending(&one->tx_work))
> +		schedule_work(&one->tx_work);
> +
> +}
> +
> +static int si4455_ioctl(struct uart_port *port,
> +			unsigned int cmd,
> +			unsigned long arg)
> +{
> +	struct si4455_port *s = dev_get_drvdata(port->dev);
> +	int ret = 0;
> +
> +	dev_dbg(port->dev, "%s(%u)", __func__, cmd);
> +	switch (cmd) {
> +	case SI4455_IOC_SEZC:
> +	memcpy(&s->configuration, (void *)arg, sizeof(s->configuration));
> +	dev_dbg(port->dev,
> +		"%s(%u): SI4455_IOC_SEZC, length(%i)",
> +		__func__,
> +		cmd,
> +		s->configuration.length);
> +	ret = si4455_re_configure(port);

This needs to indented another tab.

	switch (cmd) {
	case SI4455_IOC_SEZC:
		memcpy(&s->configuration, (void *)arg, sizeof(s->configuration));

Eep!!!

Don't use memcpy() here.  Use copy_from_user().

	void __user *argp = arg;

		if (copy_from_user(&s->configuration, argp,
				   sizeof(s->configuration)))
			return -EFAULT;

> +	break;
> +	case SI4455_IOC_CEZC:
> +	dev_dbg(port->dev,
> +		"%s(%u): SI4455_IOC_CEZC",
> +		__func__,
> +		cmd);
> +	memset(&s->configuration, 0x00, sizeof(s->configuration));
> +	ret = si4455_re_configure(port);
> +	break;
> +	case SI4455_IOC_SEZP:
> +	memcpy(&s->patch, (void *)arg, sizeof(s->patch));
> +	dev_dbg(port->dev,
> +		"%s(%u): SI4455_IOC_SEZP, length(%i)",
> +		__func__,
> +		cmd,
> +		s->configuration.length);
> +	break;
> +	case SI4455_IOC_CEZP:
> +	dev_dbg(port->dev,
> +		"%s(%u): SI4455_IOC_CEZP",
> +		__func__,
> +		cmd);
> +	memset(&s->patch, 0x00, sizeof(s->patch));
> +	break;
> +	case SI4455_IOC_STXC:
> +	s->tx_channel = *((u32 *)arg);
> +	dev_dbg(port->dev,
> +		"%s(%u): SI4455_IOC_STXC, tx_channel(%i)",
> +		__func__,
> +		cmd,
> +		s->tx_channel);
> +	break;
> +	case SI4455_IOC_GTXC:
> +	dev_dbg(port->dev,
> +		"%s(%u): SI4455_IOC_GTXC",
> +		__func__,
> +		cmd);
> +	*((u32 *)arg) = s->tx_channel;
> +	break;
> +	case SI4455_IOC_SRXC:
> +	s->rx_channel = *((u32 *)arg);
> +	dev_dbg(port->dev,
> +		"%s(%u): SI4455_IOC_SRXC, rx_channel(%i)",
> +		__func__,
> +		cmd,
> +		s->rx_channel);
> +	break;
> +	case SI4455_IOC_GRXC:
> +	dev_dbg(port->dev,
> +		"%s(%u): SI4455_IOC_GRXC",
> +		__func__,
> +		cmd);
> +	*((u32 *)arg) = s->rx_channel;
> +	break;
> +	case SI4455_IOC_SSIZ:
> +	s->package_size = *((u32 *)arg);
> +	dev_dbg(port->dev,
> +		"%s(%u): SI4455_IOC_SSIZ, package_size(%i)",
> +		__func__,
> +		cmd,
> +		s->package_size);
> +	break;
> +	case SI4455_IOC_GSIZ:
> +	dev_dbg(port->dev,
> +		"%s(%u): SI4455_IOC_GSIZ",
> +		__func__,
> +		cmd);
> +	*((u32 *)arg) = s->package_size;
> +	break;
> +	case SI4455_IOC_GRSSI:
> +	dev_dbg(port->dev,
> +		"%s(%u): SI4455_IOC_GRSSI",
> +		__func__,
> +		cmd);
> +	*((u32 *)arg) = s->current_rssi;
> +	break;
> +	default:
> +		ret = -ENOIOCTLCMD;
> +	break;
> +	}
> +
> +	if (ret == 0)
> +		si4455_do_work(port);
> +
> +	return ret;
> +}
> +
> +
> +static void si4455_null_void(struct uart_port *port)
> +{
> +	/* Do nothing */
> +}
> +
> +static const struct uart_ops si4455_ops = {
> +	.tx_empty		= si4455_tx_empty,
> +	.set_mctrl		= si4455_set_mctrl,
> +	.get_mctrl		= si4455_get_mctrl,
> +	.stop_tx		= si4455_null_void,
> +	.start_tx		= si4455_start_tx,
> +	.stop_rx		= si4455_null_void,
> +	.break_ctl		= si4455_break_ctl,
> +	.startup		= si4455_startup,
> +	.shutdown		= si4455_shutdown,
> +	.set_termios		= si4455_set_termios,
> +	.type			= si4455_type,
> +	.request_port		= si4455_request_port,
> +	.release_port		= si4455_null_void,
> +	.config_port		= si4455_config_port,
> +	.verify_port		= si4455_verify_port,
> +	.ioctl			= si4455_ioctl,
> +};
> +
> +static int __maybe_unused si4455_suspend(struct device *dev)
> +{
> +	struct si4455_port *s = dev_get_drvdata(dev);
> +
> +	uart_suspend_port(&si4455_uart, &s->one.port);
> +	return 0;
> +}
> +
> +static int __maybe_unused si4455_resume(struct device *dev)
> +{
> +	struct si4455_port *s = dev_get_drvdata(dev);
> +
> +	uart_resume_port(&si4455_uart, &s->one.port);
> +
> +	return 0;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(si4455_pm_ops, si4455_suspend, si4455_resume);
> +
> +static int si4455_probe(struct device *dev,
> +			int irq)
> +{
> +	int ret;
> +	struct si4455_port *s;
> +
> +	/* Alloc port structure */
> +	dev_dbg(dev, "%s\n", __func__);
> +	s = devm_kzalloc(dev, sizeof(*s), GFP_KERNEL);
> +	if (!s) {
> +		dev_err(dev, "Error allocating port structure\n");
> +		return -ENOMEM;
> +	}
> +
> +	dev_set_drvdata(dev, s);
> +	mutex_init(&s->mutex);
> +
> +	s->shdn_gpio = devm_gpiod_get(dev, "shdn", GPIOD_OUT_HIGH);
> +	if (IS_ERR(s->shdn_gpio)) {
> +		dev_err(dev, "Unable to reguest shdn gpio\n");
> +		ret = -EINVAL;

Preserve the error code:

		ret = PTR_ERR(s->shdn_gpio);

> +		goto out_generic;
> +	}
> +
> +	/* Initialize port data */
> +	s->one.port.dev	= dev;
> +	s->one.port.line = 0;
> +	s->one.port.irq	= irq;
> +	s->one.port.type	= PORT_SI4455;
> +	s->one.port.fifosize	= SI4455_FIFO_SIZE;
> +	s->one.port.flags	= UPF_FIXED_TYPE | UPF_LOW_LATENCY;
> +	s->one.port.iotype	= UPIO_PORT;
> +	s->one.port.iobase	= 0x00;
> +	s->one.port.membase	= (void __iomem *)~0;
> +	s->one.port.rs485_config = si4455_rs485_config;
> +	s->one.port.ops	= &si4455_ops;
> +
> +	si4455_s_power(dev, 1);
> +
> +	//detect
> +	ret = si4455_get_part_info(&s->one.port, &s->part_info);
> +	dev_dbg(dev, "si4455_get_part_info()==%i", ret);
> +	if (ret == 0) {
> +		dev_dbg(dev, "partInfo.CHIPREV= %u", s->part_info.CHIPREV);
> +		dev_dbg(dev, "partInfo.PART= %u", s->part_info.PART);
> +		dev_dbg(dev, "partInfo.PBUILD= %u", s->part_info.PBUILD);
> +		dev_dbg(dev, "partInfo.ID= %u", s->part_info.ID);
> +		dev_dbg(dev, "partInfo.CUSTOMER= %u",
> s->part_info.CUSTOMER);
> +		dev_dbg(dev, "partInfo.ROMID= %u", s->part_info.ROMID);
> +		dev_dbg(dev, "partInfo.BOND= %u", s->part_info.BOND);
> +		if (s->part_info.PART != 0x5544) {
> +			dev_err(dev, "PART(%u) error", s->part_info.PART);
> +			ret = -ENODEV;
> +		}
> +	}
> +	si4455_s_power(dev, 0);
> +	if (ret)
> +		goto out_generic;
> +
> +	/* Initialize queue for start TX */
> +	INIT_WORK(&s->one.tx_work, si4455_tx_proc);
> +
> +	/* Register port */
> +	ret = uart_add_one_port(&si4455_uart, &s->one.port);
> +	if (ret) {
> +		s->one.port.dev = NULL;
> +		goto out_uart;
> +	}
> +
> +	/* Setup interrupt */
> +	ret = devm_request_threaded_irq(dev,
> +					irq,
> +					NULL,
> +					si4455_ist,
> +					IRQF_ONESHOT | IRQF_SHARED,
> +					dev_name(dev),
> +					s);
> +	if (!ret)
> +		return 0;

This is "Last if condition is reversed" anti-pattern.  Always do error
handling, never success handling.

> +
> +	dev_err(dev, "Unable to reguest IRQ %i\n", irq);
> +
> +out_uart:
> +	uart_remove_one_port(&si4455_uart, &s->one.port);
> +out_generic:
> +	mutex_destroy(&s->mutex);
> +
> +	return ret;
> +}
> +
> +static int si4455_remove(struct device *dev)
> +{
> +	struct si4455_port *s = dev_get_drvdata(dev);
> +
> +	cancel_work_sync(&s->one.tx_work);
> +	uart_remove_one_port(&si4455_uart, &s->one.port);
> +
> +	mutex_destroy(&s->mutex);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id __maybe_unused si4455_dt_ids[] = {
> +	{ .compatible = "silabs,si4455" },
> +	{ .compatible = "silabs,si4455b1a" },
> +	{ .compatible = "silabs,si4455c2a" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, si4455_dt_ids);
> +
> +static int si4455_spi_probe(struct spi_device *spi)
> +{
> +	int ret;
> +
> +	/* Setup SPI bus */
> +	spi->bits_per_word	= 8;
> +	spi->mode		    = SPI_MODE_0;
> +	spi->max_speed_hz   = 100000;

This white space is whacky.


> +	ret = spi_setup(spi);
> +	if (ret)
> +		return ret;
> +
> +	if (spi->dev.of_node) {
> +		const struct of_device_id *of_id
> +			= of_match_device(si4455_dt_ids, &spi->dev);
> +		if (!of_id)
> +			return -ENODEV;
> +
> +	}
> +
> +	return si4455_probe(&spi->dev, spi->irq);
> +}

Anyway, hopefully that's some ideas.

regards,
dan carpenter

> On Wed, Dec 09, 2020 at 12:09:58PM +0100, Info wrote:
> > This is a serial port driver for
> > Silicon Labs Si4455 Sub-GHz transciver.
> > 
> > Signed-off-by: József Horváth <info@ministro.hu>
>
> Note, your From: line does not match this line, so I can't take this.
>
> But:
>
> > ---
> >  .../bindings/staging/serial/silabs,si4455.txt |   39 +

I'll fix this, sorry.

>
> staging drivers need to be self-contained, so this should be here.  It needs to be reviewed by the DT maintainers when moving out of staging.
>
> > index 000000000000..aee5c7613b31
> > --- /dev/null
> > +++ b/drivers/staging/si4455/TODO
> > @@ -0,0 +1,3 @@
> > +TODO:
> > +        - add variable packet length support
> > +        - add firmware patching support in case of Si4455-C2A
>
> Why are these a requirement to get it out of staging?  Why go into staging at all?  Why not go into the 'real' part of the kernel directly?
> What is keeping that from happening today?
>
> These look like new features that you can add later, and shouldn't be a requirement for acceptance into the normal part of the kernel for this driver.  Why have you not tried doing that first?

Yes, you are right. The TODO list is for me, and the driver can get out of staging without these.
The curent state of the driver is completly covers my requirements, but I would complete these functions int he future.
The main reason for not implementing the firmware patching feature is, currently I have no Si4455-C2A in my development system.
My product is based on Si4455-B1A, but I'm waiting for the Si4455-C2A module.

> thanks,
>
> greg k-h


Üdvözlettel / Best regards:
József Horváth

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

[...]

On Wed, Dec 09, 2020 at 03:42:06PM +0300, Dan Carpenter wrote:
> Change the From email header to say your name.
> 
> The patch is corrupted and can't be applied.  Please read the first
> paragraphs of Documentation/process/email-clients.rst
> 
> This driver is pretty small and it might be easier to clean it up first
> before merging it into staging.  Run checkpatch.pl --strict on the
> driver.
> 
> > --- /dev/null
> > +++ b/drivers/staging/si4455/si4455.c
> > @@ -0,0 +1,1465 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2020 József Horváth <info@ministro.hu>
> > + *
> > + */
> > +#include <linux/bitops.h>
> > +#include <linux/clk.h>
> > +#include <linux/delay.h>
> > +#include <linux/device.h>
> > +#include <linux/gpio/driver.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> > +#include <linux/of_gpio.h>
> > +#include <linux/regmap.h>
> > +#include <linux/serial_core.h>
> > +#include <linux/serial.h>
> > +#include <linux/tty.h>
> > +#include <linux/tty_flip.h>
> > +#include <linux/spi/spi.h>
> > +#include <linux/uaccess.h>
> > +#include "si4455_api.h"
> > +
> > +#define PORT_SI4455						1096
> > +#define SI4455_NAME						"si4455"
> > +#define SI4455_MAJOR						432
> > +#define SI4455_MINOR						567
> > +#define SI4455_UART_NRMAX					1
> > +#define SI4455_FIFO_SIZE					64
> > +
> > +#define SI4455_CMD_ID_EZCONFIG_CHECK				0x19
> > +#define SI4455_CMD_ID_PART_INFO					0x01
> > +#define SI4455_CMD_REPLY_COUNT_PART_INFO			9
> > +#define SI4455_CMD_ID_GET_INT_STATUS				0x20
> > +#define SI4455_CMD_REPLY_COUNT_GET_INT_STATUS			8
> > +#define SI4455_CMD_ID_FIFO_INFO					0x15
> > +#define SI4455_CMD_ARG_COUNT_FIFO_INFO				2
> > +#define SI4455_CMD_REPLY_COUNT_FIFO_INFO			2
> > +#define SI4455_CMD_FIFO_INFO_ARG_TX_BIT				0x01
> > +#define SI4455_CMD_FIFO_INFO_ARG_RX_BIT				0x02
> > +#define SI4455_CMD_ID_READ_CMD_BUFF				0x44
> > +#define SI4455_CMD_ID_READ_RX_FIFO				0x77
> > +#define SI4455_CMD_ID_WRITE_TX_FIFO				0x66
> > +#define SI4455_CMD_ID_START_RX					0x32
> > +#define SI4455_CMD_ARG_COUNT_START_RX				8
> > +#define SI4455_CMD_START_RX_ARG_RXTIMEOUT_STATE_ENUM_RX		8
> > +#define SI4455_CMD_START_RX_ARG_RXVALID_STATE_ENUM_RX		8
> > +#define SI4455_CMD_START_RX_ARG_RXINVALID_STATE_ENUM_RX		8
> > +#define SI4455_CMD_ID_START_TX					0x31
> > +#define SI4455_CMD_ARG_COUNT_START_TX				5
> > +#define SI4455_CMD_ID_CHANGE_STATE				0x34
> > +#define SI4455_CMD_ARG_COUNT_CHANGE_STATE			2
> > +#define SI4455_CMD_CHANGE_STATE_ARG_NEW_STATE_ENUM_READY	3
> > +#define SI4455_CMD_GET_CHIP_STATUS_REP_CMD_ERROR_PEND_MASK	0x08
> > +#define SI4455_CMD_GET_CHIP_STATUS_REP_CMD_ERROR_PEND_BIT	0x08
> > +#define SI4455_CMD_GET_INT_STATUS_REP_PACKET_SENT_PEND_BIT	0x20
> > +#define SI4455_CMD_GET_INT_STATUS_REP_PACKET_RX_PEND_BIT	0x10
> > +#define SI4455_CMD_GET_INT_STATUS_REP_CRC_ERROR_BIT		0x08
> 
> These names are unreadably long and just make the code messy.
> 
> > +#define SI4455_CMD_ID_GET_MODEM_STATUS				0x22
> > +#define SI4455_CMD_ARG_COUNT_GET_MODEM_STATUS			2
> > +#define SI4455_CMD_REPLY_COUNT_GET_MODEM_STATUS			8
> > +
> > +struct si4455_part_info {
> > +	u8				CHIPREV;
> > +	u16				PART;
> > +	u8				PBUILD;
> > +	u16				ID;
> > +	u8				CUSTOMER;
> > +	u8				ROMID;
> > +	u8				BOND;
> 
> Also the huge gap between the type and the struct member makes it
> hard to read.
> 
> 	u8  chip_rev;
> 	u16 part;
> 	u8  pbuild;
> 
> etc.
> 
> > +};
> > +
> > +struct si4455_int_status {
> > +	u8				INT_PEND;
> > +	u8				INT_STATUS;
> > +	u8				PH_PEND;
> > +	u8				PH_STATUS;
> > +	u8				MODEM_PEND;
> > +	u8				MODEM_STATUS;
> > +	u8				CHIP_PEND;
> > +	u8				CHIP_STATUS;
> > +};
> > +
> > +struct si4455_modem_status {
> > +	u8				MODEM_PEND;
> > +	u8				MODEM_STATUS;
> > +	u8				CURR_RSSI;
> > +	u8				LATCH_RSSI;
> > +	u8				ANT1_RSSI;
> > +	u8				ANT2_RSSI;
> > +	u16			AFC_FREQ_OFFSET;
> > +};
> > +
> > +struct si4455_fifo_info {
> > +	u8				RX_FIFO_COUNT;
> > +	u8				TX_FIFO_SPACE;
> > +};
> > +
> > +struct si4455_one {
> > +	struct uart_port		port;
> > +	struct work_struct		tx_work;
> > +};
> > +
> > +struct si4455_port {
> > +	struct mutex			mutex;
> > +	int				power_count;
> > +	struct gpio_desc		*shdn_gpio;
> > +	struct si4455_part_info		part_info;
> > +	struct si4455_modem_status	modem_status;
> > +	struct si4455_iocbuff		configuration;
> > +	struct si4455_iocbuff		patch;
> > +	u32				tx_channel;
> > +	u32				rx_channel;
> > +	u32				package_size;
> > +	bool				configured;
> > +	bool				tx_pending;
> > +	bool				rx_pending;
> > +	u32				current_rssi;
> > +	struct si4455_one		one;
> > +};
> 
> Since the struct is not defined by the spec then don't bother aligning
> the members.  It just makes changing the code complicated because you
> have to re-align stuff.  Just put a single space between the type and
> the name.
> 
> 
> > +
> > +static struct uart_driver si4455_uart = {
> > +	.owner			= THIS_MODULE,
> > +	.driver_name		= SI4455_NAME,
> > +#ifdef CONFIG_DEVFS_FS
> > +	.dev_name		= "ttySI%d",
> > +#else
> > +	.dev_name		= "ttySI",
> > +#endif
> > +	.major			= SI4455_MAJOR,
> > +	.minor			= SI4455_MINOR,
> > +	.nr			= SI4455_UART_NRMAX,
> > +};
> > +
> > +static int si4455_get_response(struct uart_port *port,
> > +				int length,
> > +				u8 *data);
> 
> Remove unecessary declarations like this.
> 
> > +static int si4455_send_command(struct uart_port *port,
> > +				int length,
> > +				u8 *data);
> > +static int si4455_send_command_get_response(struct uart_port *port,
> > +						int outLength,
> > +						u8 *outData,
> > +						int inLength,
> > +						u8 *inData);
> > +static int si4455_read_data(struct uart_port *port,
> > +				u8 command,
> > +				int pollCts,
> > +				int length,
> > +				u8 *data);
> > +static int si4455_write_data(struct uart_port *port,
> > +				u8 command,
> > +				int pollCts,
> > +				int length,
> > +				u8 *data);
> > +static int si4455_poll_cts(struct uart_port *port);
> > +static void si4455_set_power(struct si4455_port *priv,
> > +				int on);
> > +static int si4455_get_part_info(struct uart_port *port,
> > +				struct si4455_part_info *result);
> > +
> > +static int si4455_get_response(struct uart_port *port,
> > +				int length,
> > +				u8 *data)
> > +{
> > +	int ret = -EIO;
> 
> Remove unecessary initializations.  It disables static checkers' ability
> to find uninitialized variable bugs so it leads to bugs.
> 
> > +	u8 dataOut[] = { SI4455_CMD_ID_READ_CMD_BUFF };
> > +	u8 *dataIn = devm_kzalloc(port->dev, 1 + length, GFP_KERNEL);
> 
> Never put functions which can fail in the declaration block.  It leads
> to "forgot to check for NULL bugs" which is the case here.
> 
> Don't use devm_ for this.  It has a different lifetime.  Use normal
> kzalloc().
> 
> > +	struct spi_transfer xfer[] = {
> > +		{
> > +			.tx_buf = dataOut,
> > +			.len = sizeof(dataOut),
> > +		}, {
> > +			.rx_buf = dataIn,
> > +			.len = 1 + length,
> > +		}
> > +	};
> > +	int errCnt = 10000;
> > +
> > +	while (errCnt > 0) {
> 
> while (--cnt > 9) {
> 
> > +		dataOut[0] = SI4455_CMD_ID_READ_CMD_BUFF;
> > +		ret = spi_sync_transfer(to_spi_device(port->dev),
> > +					xfer,
> > +					ARRAY_SIZE(xfer));
> > +		if (ret == 0) {
> 
> Always do Error Handling as opposed to success handling.
> 
> 		if (ret)
> 			break;
> 
> > +			if (dataIn[0] == 0xFF) {
> > +				if ((length > 0) && (data != NULL)) {
> > +					memcpy(data, &dataIn[1], length);
> > +				} else {
> > +					if (length > 0)
> > +						ret = -EINVAL;
> > +				}
> > +				break;
> > +			}
> > +			usleep_range(100, 200);
> > +		} else {
> > +			break;
> > +		}
> > +		errCnt--;
> > +	}
> > +	if (errCnt == 0) {
> > +		dev_err(port->dev, "si4455_poll_cts:errCnt==%i", errCnt);
> > +		ret = -EIO;
> > +	}
> > +	if (dataIn != NULL)
> > +		devm_kfree(port->dev, dataIn);
> > +	return ret;
> > +}
> > +
> > +static int si4455_send_command(struct uart_port *port,
> > +				int length,
> > +				u8 *data)
> > +{
> > +	int ret;
> > +
> > +	ret = si4455_poll_cts(port);
> > +	if (ret == 0) {
> 
> 	ret = si4455_poll_cts(port);
> 	if (ret) {
> 		dev_err(port->dev, "%s: si4455_poll_cts error(%i)", __func__,
> 			ret);
> 		return ret;
> 	}
> 
> All the checks need to be reversed.  The printk only needs two lines.
> 
> 
> > +		ret = spi_write(to_spi_device(port->dev), data, length);
> > +		if (ret) {
> > +			dev_err(port->dev,
> > +				"%s: spi_write error(%i)",
> > +				__func__,
> > +				ret);
> > +		}
> > +	} else {
> > +		dev_err(port->dev,
> > +			"%s: si4455_poll_cts error(%i)",
> > +			__func__,
> > +			ret);
> > +	}
> > +	return ret;
> > +}
> > +
> > +static int si4455_send_command_get_response(struct uart_port *port,
> > +						int outLength,
> > +						u8 *outData,
> > +						int inLength,
> > +						u8 *inData)
> > +{
> > +	int ret;
> > +
> > +	ret = si4455_send_command(port, outLength, outData);
> > +	if (!ret) {
> > +		ret = si4455_get_response(port, inLength, inData);
> > +	} else {
> > +		dev_err(port->dev,
> > +			"%s: si4455_send_command error(%i)",
> > +			__func__,
> > +			ret);
> > +	}
> > +	return ret;
> > +}
> > +
> > +static int si4455_read_data(struct uart_port *port,
> > +				u8 command,
> > +				int pollCts,
> > +				int length,
> > +				u8 *data)
> > +{
> > +	int ret = 0;
> > +	u8 dataOut[] = { command };
> > +	struct spi_transfer xfer[] = {
> > +		{
> > +			.tx_buf = dataOut,
> > +			.len = sizeof(dataOut),
> > +		}, {
> > +			.rx_buf = data,
> > +			.len = length,
> > +		}
> > +	};
> > +
> > +	if (pollCts)
> > +		ret = si4455_poll_cts(port);
> 
> 	if (poll) {
> 		ret = si4455_poll_cts(port);
> 		if (ret)
> 			return ret;
> 	}
> 
> > +
> > +	if (ret == 0) {
> > +		ret = spi_sync_transfer(to_spi_device(port->dev),
> > +					xfer,
> > +					ARRAY_SIZE(xfer));
> > +		if (ret) {
> > +			dev_err(port->dev,
> > +				"%s: spi_sync_transfer error(%i)",
> > +				__func__,
> > +				ret);
> > +		}
> > +	}
> > +	return ret;
> > +}
> > +
> > +static int si4455_write_data(struct uart_port *port,
> > +				u8 command,
> > +				int pollCts,
> > +				int length,
> > +				u8 *data)
> > +{
> > +	int ret = 0;
> > +	u8 *dataOut = NULL;
> > +
> > +	if (pollCts)
> > +		ret = si4455_poll_cts(port);
> > +
> > +	if (ret == 0) {
> > +		dataOut = devm_kzalloc(port->dev, 1 + length, GFP_KERNEL);
> 
> Use vanilla kzalloc().
> 
> > +		if (dataOut != NULL) {
> > +			dataOut[0] = command;
> > +			memcpy(&dataOut[1], data, length);
> > +			ret = spi_write(to_spi_device(port->dev),
> > +					dataOut,
> > +					1 + length);
> > +			if (ret) {
> > +				dev_err(port->dev,
> > +					"%s: spi_write error(%i)",
> > +					__func__,
> > +					ret);
> > +			}
> > +		} else {
> > +			dev_err(port->dev,
> > +				"%s: devm_kzalloc error",
> > +				__func__);
> > +			ret = -ENOMEM;
> > +		}
> > +	}
> > +	if (dataOut != NULL)
> > +		devm_kfree(port->dev, dataOut);
> > +
> > +	return ret;
> > +}
> > +
> > +static int si4455_poll_cts(struct uart_port *port)
> > +{
> > +	return si4455_get_response(port, 0, NULL);
> > +}
> > +
> > +static void si4455_set_power(struct si4455_port *priv,
> > +				int on)
> > +{
> > +	if (priv->shdn_gpio) {
> 
> Reverse this test:
> 
> 	if (!priv->shdn_gpio)
> 		return;
> 
> > +		if (on) {
> > +			gpiod_direction_output(priv->shdn_gpio, 0);
> > +			usleep_range(4000, 5000);
> > +			gpiod_set_value(priv->shdn_gpio, 1);
> > +			usleep_range(4000, 5000);
> > +		} else {
> > +			gpiod_direction_output(priv->shdn_gpio, 0);
> > +		}
> > +	}
> > +}
> > +
> > +static int si4455_s_power(struct device *dev,
> > +				int on)
> > +{
> > +	struct si4455_port *s = dev_get_drvdata(dev);
> > +
> > +	dev_dbg(dev, "%s(on=%d)\n", __func__, on);
> > +	if (s->power_count == !on)
> > +		si4455_set_power(s, on);
> > +	s->power_count += on ? 1 : -1;
> > +	WARN_ON(s->power_count < 0);
> > +
> > +	return 0;
> > +}
> > +
> > +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);
> 
> This can fit on two lines:
> 
> 	ret = si4455_send_command_get_response(port, sizeof(dataOut), dataOut,
> 					       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];
> > +	} else {
> > +		dev_err(port->dev,
> > +			"%s: si4455_send_command_get_response error(%i)",
> > +			__func__,
> > +			ret);
> > +	}
> > +	return ret;
> > +}
> > +
> > +static int si4455_get_int_status(struct uart_port *port,
> > +					u8 phClear,
> > +					u8 modemClear,
> > +					u8 chipClear,
> > +					struct si4455_int_status *result)
> > +{
> > +	int ret;
> > +	u8 dataOut[] = {
> > +		SI4455_CMD_ID_GET_INT_STATUS,
> > +		phClear,
> > +		modemClear,
> > +		chipClear
> > +	};
> > +	u8 dataIn[SI4455_CMD_REPLY_COUNT_GET_INT_STATUS];
> > +
> > +	ret = si4455_send_command_get_response(port,
> > +						sizeof(dataOut),
> > +						dataOut,
> > +						sizeof(dataIn),
> > +						dataIn);
> > +	if (ret == 0) {
> > +		result->INT_PEND       = dataIn[0];
> > +		result->INT_STATUS     = dataIn[1];
> > +		result->PH_PEND        = dataIn[2];
> > +		result->PH_STATUS      = dataIn[3];
> > +		result->MODEM_PEND     = dataIn[4];
> > +		result->MODEM_STATUS   = dataIn[5];
> > +		result->CHIP_PEND      = dataIn[6];
> > +		result->CHIP_STATUS    = dataIn[7];
> > +	} else {
> > +		dev_err(port->dev,
> > +			"%s: si4455_send_command_get_response error(%i)",
> > +			__func__,
> > +			ret);
> > +	}
> > +	return ret;
> > +}
> > +
> > +static int si4455_get_modem_status(struct uart_port *port,
> > +					u8 modemClear,
> > +					struct si4455_modem_status *result)
> > +{
> > +	int ret;
> > +	u8 dataOut[] = {
> > +		SI4455_CMD_ID_GET_MODEM_STATUS,
> > +		modemClear,
> > +	};
> > +	u8 dataIn[SI4455_CMD_REPLY_COUNT_GET_MODEM_STATUS];
> > +
> > +	ret = si4455_send_command_get_response(port,
> > +						sizeof(dataOut),
> > +						dataOut,
> > +						sizeof(dataIn),
> > +						dataIn);
> > +	if (ret == 0) {
> > +		result->MODEM_PEND      = dataIn[0];
> > +		result->MODEM_STATUS    = dataIn[1];
> > +		result->CURR_RSSI       = dataIn[2];
> > +		result->LATCH_RSSI      = dataIn[3];
> > +		result->ANT1_RSSI       = dataIn[4];
> > +		result->ANT2_RSSI       = dataIn[5];
> > +		memcpy(&result->AFC_FREQ_OFFSET,
> > +			&dataIn[6],
> > +			sizeof(result->AFC_FREQ_OFFSET));
> > +	} else {
> > +		dev_err(port->dev,
> > +			"%s: si4455_send_command_get_response error(%i)",
> > +			__func__,
> > +			ret);
> > +	}
> > +	return ret;
> > +}
> > +
> > +static int si4455_fifo_info(struct uart_port *port,
> > +				u8 fifo,
> > +				struct si4455_fifo_info *result)
> > +{
> > +	int ret = 0;
> > +	u8 dataOut[SI4455_CMD_ARG_COUNT_FIFO_INFO] = {
> > +		SI4455_CMD_ID_FIFO_INFO, fifo
> > +	};
> > +	u8 dataIn[SI4455_CMD_REPLY_COUNT_FIFO_INFO] = { 0 };
> > +
> > +	ret = si4455_send_command_get_response(port,
> > +						sizeof(dataOut),
> > +						dataOut,
> > +						sizeof(dataIn),
> > +						dataIn);
> > +	if (ret == 0) {
> > +		result->RX_FIFO_COUNT  = dataIn[0];
> > +		result->TX_FIFO_SPACE  = dataIn[1];
> > +	} else {
> > +		dev_err(port->dev,
> > +			"%s: si4455_send_command_get_response error(%i)",
> > +			__func__,
> > +			ret);
> > +	}
> > +	return ret;
> > +}
> > +
> > +static int si4455_read_rx_fifo(struct uart_port *port,
> > +				int length,
> > +				u8 *data)
> > +{
> > +	return si4455_read_data(port,
> > +				SI4455_CMD_ID_READ_RX_FIFO,
> > +				0,
> > +				length,
> > +				data);
> > +}
> > +
> > +static int si4455_write_tx_fifo(struct uart_port *port,
> > +				int length,
> > +				u8 *data)
> > +{
> > +	return si4455_write_data(port,
> > +					SI4455_CMD_ID_WRITE_TX_FIFO,
> > +					0,
> > +					length,
> > +					data);
> > +}
> > +
> > +static int si4455_rx(struct uart_port *port,
> > +			u32 channel,
> > +			u8 condition,
> > +			u16 length,
> > +			u8 nextState1,
> > +			u8 nextState2,
> > +			u8 nextState3)
> > +{
> > +	u8 dataOut[SI4455_CMD_ARG_COUNT_START_RX];
> > +
> > +	dataOut[0] = SI4455_CMD_ID_START_RX;
> > +	dataOut[1] = channel;
> > +	dataOut[2] = condition;
> > +	dataOut[3] = (u8)(length >> 8);
> > +	dataOut[4] = (u8)(length);
> > +	dataOut[5] = nextState1;
> > +	dataOut[6] = nextState2;
> > +	dataOut[7] = nextState3;
> > +
> > +	return si4455_send_command(port,
> > +					SI4455_CMD_ARG_COUNT_START_RX,
> > +					dataOut);
> > +}
> > +
> > +static int si4455_tx(struct uart_port *port,
> > +			u8 channel,
> > +			u8 condition,
> > +			u16 length)
> > +{
> > +	u8 dataOut[/*6*/ SI4455_CMD_ARG_COUNT_START_TX];
> > +
> > +	dataOut[0] = SI4455_CMD_ID_START_TX;
> > +	dataOut[1] = channel;
> > +	dataOut[2] = condition;
> > +	dataOut[3] = (u8)(length >> 8);
> > +	dataOut[4] = (u8)(length);
> > +	/*TODO: radioCmd[5] = 0x44; in case of rev c2a*/
> > +
> > +	return si4455_send_command(port,
> > +					/*6*/ SI4455_CMD_ARG_COUNT_START_TX,
> > +					dataOut);
> > +}
> > +
> > +static int si4455_change_state(struct uart_port *port,
> > +				u8 nextState1)
> > +{
> > +	u8 dataOut[SI4455_CMD_ARG_COUNT_CHANGE_STATE];
> > +
> > +	dataOut[0] = SI4455_CMD_ID_CHANGE_STATE;
> > +	dataOut[1] = (u8)nextState1;
> > +
> > +	return si4455_send_command(port,
> > +					SI4455_CMD_ARG_COUNT_CHANGE_STATE,
> > +					dataOut);
> > +}
> > +
> > +static int si4455_begin_tx(struct uart_port *port,
> > +				u32 channel,
> > +				int length,
> > +				u8 *data)
> > +{
> > +	int ret = 0;
> > +	struct si4455_int_status intStatus = { 0 };
> > +	struct si4455_fifo_info fifoInfo = { 0 };
> > +
> > +	dev_dbg(port->dev, "%s(%u, %u)", __func__, channel, length);
> > +	if ((length > SI4455_FIFO_SIZE) || (length < 0))
> > +		ret = -EINVAL;
> > +
> > +	if (ret == 0) {
> > +		ret = si4455_change_state(port,
> > +			SI4455_CMD_CHANGE_STATE_ARG_NEW_STATE_ENUM_READY);
> > +		if (ret) {
> > +			dev_err(port->dev,
> > +				"%s: si4455_change_state error(%i)",
> > +				__func__,
> > +				ret);
> > +			goto end;
> > +		}
> > +		ret = si4455_get_int_status(port, 0, 0, 0, &intStatus);
> > +		if (ret) {
> > +			dev_err(port->dev,
> > +				"%s: si4455_get_int_status error(%i)",
> > +				__func__,
> > +				ret);
> > +			goto end;
> > +		}
> > +		ret = si4455_fifo_info(port,
> > +					SI4455_CMD_FIFO_INFO_ARG_TX_BIT,
> > +					&fifoInfo);
> > +		if (ret) {
> > +			dev_err(port->dev,
> > +				"%s: si4455_fifo_info error(%i)",
> > +				__func__,
> > +				ret);
> > +			goto end;
> > +		}
> > +		ret = si4455_write_tx_fifo(port, (u16)length, data);
> 
> No need to cast this.
> 
> > +		if (ret) {
> > +			dev_err(port->dev,
> > +				"%s: si4455_write_tx_fifo error(%i)",
> > +				__func__,
> > +				ret);
> > +			goto end;
> 
> Just return directly.  Do nothing gotos just end up introducing "Forgot
> to set the error code" bugs.
> 
> > +		}
> > +		ret = si4455_tx(port,
> > +				(u8)channel,
> > +				0x30,
> > +				(u16)length);
> > +		if (ret) {
> > +			dev_err(port->dev,
> > +				"%s: si4455_tx error(%i)",
> > +				__func__,
> > +				ret);
> > +			goto end;
> > +		}
> > +	}
> > +end:
> > +	return ret;
> > +}
> > +
> > +static int si4455_end_tx(struct uart_port *port)
> > +{
> > +	int ret = 0;
> > +	struct si4455_int_status intStatus = { 0 };
> > +
> > +	ret = si4455_change_state(port,
> > +			SI4455_CMD_CHANGE_STATE_ARG_NEW_STATE_ENUM_READY);
> > +	if (ret) {
> > +		dev_err(port->dev,
> > +			"%s: si4455_change_state error(%i)",
> > +			__func__,
> > +			ret);
> > +		goto end;
> > +	}
> > +	ret = si4455_get_int_status(port, 0, 0, 0, &intStatus);
> > +	if (ret) {
> > +		dev_err(port->dev,
> > +			"%s: si4455_get_int_status error(%i)",
> > +			__func__,
> > +			ret);
> > +		goto end;
> > +	}
> > +end:
> > +	return ret;
> > +}
> > +
> > +static int si4455_begin_rx(struct uart_port *port,
> > +				u32 channel,
> > +				u32 length)
> > +{
> > +	int ret = 0;
> > +	struct si4455_int_status intStatus = { 0 };
> > +	struct si4455_fifo_info fifoInfo = { 0 };
> > +
> > +	dev_dbg(port->dev, "%s(%u, %u)", __func__, channel, length);
> > +	ret = si4455_get_int_status(port, 0, 0, 0, &intStatus);
> > +	if (ret) {
> > +		dev_err(port->dev,
> > +			"%s: si4455_get_int_status error(%i)",
> > +			__func__,
> > +			ret);
> > +		goto end;
> > +	}
> > +	ret = si4455_fifo_info(port,
> > +				SI4455_CMD_FIFO_INFO_ARG_RX_BIT,
> > +				&fifoInfo);
> > +	if (ret) {
> > +		dev_err(port->dev,
> > +			"%s: si4455_fifo_info error(%i)",
> > +			__func__,
> > +			ret);
> > +		goto end;
> > +	}
> > +	ret = si4455_rx(port,
> > +			channel,
> > +			0x00,
> > +			length,
> > +			SI4455_CMD_START_RX_ARG_RXTIMEOUT_STATE_ENUM_RX,
> > +			SI4455_CMD_START_RX_ARG_RXVALID_STATE_ENUM_RX,
> > +			SI4455_CMD_START_RX_ARG_RXINVALID_STATE_ENUM_RX);
> > +	if (ret) {
> > +		dev_err(port->dev,
> > +			"%s: si4455_rx error(%i)",
> > +			__func__,
> > +			ret);
> > +		goto end;
> > +	}
> > +end:
> > +	return ret;
> > +}
> > +
> > +static int si4455_end_rx(struct uart_port *port,
> > +				u32 length,
> > +				u8 *data)
> > +{
> > +	return si4455_read_rx_fifo(port, length, data);
> > +}
> > +
> > +static int si4455_configure(struct uart_port *port,
> > +				u8 *configurationData)
> > +{
> > +	int ret = 0;
> > +	u8 col;
> > +	u8 response;
> > +	u8 numOfBytes;
> > +	struct si4455_int_status intStatus = { 0 };
> > +	u8 radioCmd[16u];
> > +
> > +	/* While cycle as far as the pointer points to a command */
> > +	while (*configurationData != 0x00) {
> > +		/* Commands structure in the array:
> > +		 * --------------------------------
> > +		 * LEN | <LEN length of data>
> > +		 */
> > +		numOfBytes = *configurationData++;
> > +		dev_dbg(port->dev,
> > +			"%s: numOfBytes(%u)",
> > +			__func__,
> > +			numOfBytes);
> > +		if (numOfBytes > 16u) {
> > +			/* Initial configuration of Si4x55 */
> > +			if (SI4455_CMD_ID_WRITE_TX_FIFO
> > +				 == *configurationData) {
> > +				if (numOfBytes > 128u) {
> > +					/* Number of command bytes exceeds
> > +					 * maximal allowable length
> > +					 */
> > +					dev_err(port->dev,
> > +						"%s: command length
> > error(%i)",
> > +						__func__,
> > +						numOfBytes);
> > +					ret = -EINVAL;
> > +					break;
> > +				}
> > +
> > +				/* Load array to the device */
> > +				configurationData++;
> > +				ret = si4455_write_data(port,
> > +						SI4455_CMD_ID_WRITE_TX_FIFO,
> > +						1,
> > +						numOfBytes - 1,
> > +						configurationData);
> > +				if (ret) {
> > +					dev_err(port->dev,
> > +						"%s: si4455_write_data
> > error(%i)",
> > +						__func__,
> > +						ret);
> > +					break;
> > +				}
> > +
> > +				/* Point to the next command */
> > +				configurationData += numOfBytes - 1;
> > +
> > +				/* Continue command interpreter */
> > +				continue;
> > +			} else {
> > +				/* Number of command bytes exceeds
> > +				 * maximal allowable length
> > +				 */
> > +				ret = -EINVAL;
> > +				break;
> > +			}
> > +		}
> > +
> > +		for (col = 0u; col < numOfBytes; col++) {
> > +			radioCmd[col] = *configurationData;
> > +			configurationData++;
> > +		}
> > +
> > +		dev_dbg(port->dev,
> > +			"%s: radioCmd[0](%u)",
> > +			__func__,
> > +			radioCmd[0]);
> > +		ret = si4455_send_command_get_response(port,
> > +							numOfBytes,
> > +							radioCmd,
> > +							1,
> > +							&response);
> > +		if (ret) {
> > +			dev_err(port->dev,
> > +				"%s: si4455_send_command_get_response
> > error(%i)",
> > +				__func__,
> > +				ret);
> > +			break;
> > +		}
> > +
> > +		/* Check response byte of EZCONFIG_CHECK command */
> > +		if (radioCmd[0] == SI4455_CMD_ID_EZCONFIG_CHECK) {
> > +			if (response) {
> > +				/* Number of command bytes exceeds
> > +				 * maximal allowable length
> > +				 */
> > +				ret = -EIO;
> > +				dev_err(port->dev,
> > +					"%s: EZConfig check error(%i)",
> > +					__func__,
> > +					radioCmd[0]);
> > +				break;
> > +			}
> > +		}
> > +
> > +		/* Get and clear all interrupts.  An error has occurred...
> > */
> > +		si4455_get_int_status(port, 0, 0, 0, &intStatus);
> > +		if (intStatus.CHIP_PEND
> > +			&
> > SI4455_CMD_GET_CHIP_STATUS_REP_CMD_ERROR_PEND_MASK) {
> > +			ret = -EIO;
> > +			dev_err(port->dev,
> > +				"%s: chip error(%i)",
> > +				__func__,
> > +				intStatus.CHIP_PEND);
> > +			break;
> > +		}
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +static int si4455_re_configure(struct uart_port *port)
> > +{
> > +	int ret = 0;
> > +	struct si4455_port *s = dev_get_drvdata(port->dev);
> > +
> > +	mutex_lock(&s->mutex);
> > +	s->configured = 0;
> > +	if (s->power_count > 0)
> > +		si4455_s_power(port->dev, 0);
> > +
> > +	si4455_s_power(port->dev, 1);
> > +	if (s->configuration.length > 0) {
> > +		ret = si4455_configure(port, s->configuration.data);
> > +		if (ret == 0)
> > +			s->configured = 1;
> > +
> > +	}
> > +	mutex_unlock(&s->mutex);
> > +	return ret;
> > +}
> > +
> > +static int si4455_do_work(struct uart_port *port)
> > +{
> > +	int ret = 0;
> > +	struct si4455_port *s = dev_get_drvdata(port->dev);
> > +	struct circ_buf *xmit = &port->state->xmit;
> > +	unsigned int tx_pending = 0;
> > +	unsigned int tx_to_end = 0;
> > +	u8 *data = NULL;
> > +
> > +	mutex_lock(&s->mutex);
> > +	if (s->configured && (s->power_count > 0)) {
> > +		if (!(uart_circ_empty(xmit)
> > +			|| uart_tx_stopped(port)
> > +			|| s->tx_pending)) {
> > +			tx_pending = uart_circ_chars_pending(xmit);
> > +			if (s->package_size > 0) {
> > +				if (tx_pending >= s->package_size) {
> > +					tx_pending = s->package_size;
> > +					data = devm_kzalloc(port->dev,
> > +						s->package_size,
> > +						GFP_KERNEL);
> > +					tx_to_end =
> > CIRC_CNT_TO_END(xmit->head,
> > +								xmit->tail,
> > +
> > UART_XMIT_SIZE);
> > +					if (tx_to_end < tx_pending) {
> > +						memcpy(data,
> > +							xmit->buf +
> > xmit->tail,
> > +							tx_to_end);
> > +						memcpy(data,
> > +							xmit->buf,
> > +							tx_pending -
> > tx_to_end);
> > +					} else {
> > +						memcpy(data,
> > +							xmit->buf +
> > xmit->tail,
> > +							tx_pending);
> > +					}
> > +					if (si4455_begin_tx(port,
> > +							s->tx_channel,
> > +							tx_pending,
> > +							data) == 0) {
> > +						s->tx_pending = true;
> > +					}
> > +					devm_kfree(port->dev, data);
> > +				}
> > +			} else {
> > +				//TODO: variable packet length
> > +			}
> > +		}
> > +		if (!s->tx_pending) {
> > +			if (s->package_size > 0) {
> > +				ret = si4455_begin_rx(port,
> > +							s->rx_channel,
> > +							s->package_size);
> > +			} else {
> > +				//TODO: variable packet length
> > +			}
> > +		}
> > +	}
> > +	mutex_unlock(&s->mutex);
> > +	return ret;
> > +}
> > +
> > +static void si4455_handle_rx_pend(struct si4455_port *s)
> > +{
> > +	struct uart_port *port = &s->one.port;
> > +	u8 *data = NULL;
> > +	int sret = 0;
> > +	int i = 0;
> > +
> > +	if (s->package_size > 0) {
> > +		data = devm_kzalloc(port->dev,
> > +					s->package_size,
> > +					GFP_KERNEL);
> > +		sret = si4455_end_rx(port,
> > +				s->package_size,
> > +				data);
> > +		if (sret == 0) {
> > +			for (i = 0; i < s->package_size; i++) {
> > +				uart_insert_char(port,
> > +					0,
> > +					0,
> > +					data[i],
> > +					TTY_NORMAL);
> > +				port->icount.rx++;
> > +			}
> > +			tty_flip_buffer_push(&port->state->port);
> > +		} else {
> > +			dev_err(port->dev,
> > +				"%s: si4455_end_rx error(%i)",
> > +				__func__,
> > +				sret);
> > +		}
> > +		devm_kfree(port->dev, data);
> > +	} else {
> > +		//TODO: variable packet length
> > +	}
> > +}
> > +
> > +static void si4455_handle_tx_pend(struct si4455_port *s)
> > +{
> > +	struct uart_port *port = &s->one.port;
> > +	struct circ_buf *xmit = &port->state->xmit;
> > +
> > +	if (s->tx_pending) {
> > +		if (s->package_size) {
> > +			port->icount.tx += s->package_size;
> > +			xmit->tail = (xmit->tail + s->package_size)
> > +					& (UART_XMIT_SIZE - 1);
> > +		} else {
> > +			//TODO: variable packet length
> > +		}
> > +		si4455_end_tx(port);
> > +		s->tx_pending = 0;
> > +	}
> > +}
> > +
> > +static irqreturn_t si4455_port_irq(struct si4455_port *s)
> > +{
> > +	struct uart_port *port = &s->one.port;
> > +	irqreturn_t ret = IRQ_NONE;
> > +	struct si4455_int_status intStatus = { 0 };
> > +	struct si4455_fifo_info fifoInfo = { 0 };
> > +
> > +	mutex_lock(&s->mutex);
> > +	if (s->configured && (s->power_count > 0)) {
> > +		if (!si4455_get_int_status(port, 0, 0, 0, &intStatus)) {
> > +			si4455_get_modem_status(port, 0, &s->modem_status);
> > +			if (intStatus.CHIP_PEND
> > +			& SI4455_CMD_GET_CHIP_STATUS_REP_CMD_ERROR_PEND_BIT)
> > {
> > +				dev_err(port->dev,
> > +					"%s: CHIP_STATUS:CMD_ERROR_PEND",
> > +					__func__);
> > +			} else if (intStatus.PH_PEND
> > +			&
> > SI4455_CMD_GET_INT_STATUS_REP_PACKET_SENT_PEND_BIT) {
> > +				dev_dbg(port->dev,
> > +					"%s: PH_STATUS:PACKET_SENT_PEND",
> > +					__func__);
> > +				si4455_handle_tx_pend(s);
> > +			} else if (intStatus.PH_PEND
> > +			& SI4455_CMD_GET_INT_STATUS_REP_PACKET_RX_PEND_BIT)
> > {
> > +				dev_dbg(port->dev,
> > +					"%s: PH_STATUS:PACKET_RX_PEND",
> > +					__func__);
> > +				s->current_rssi = s->modem_status.CURR_RSSI;
> > +				si4455_fifo_info(port, 0, &fifoInfo);
> > +				si4455_handle_rx_pend(s);
> > +			} else if (intStatus.PH_PEND
> > +			& SI4455_CMD_GET_INT_STATUS_REP_CRC_ERROR_BIT) {
> > +				dev_dbg(port->dev,
> > +					"%s: PH_STATUS:CRC_ERROR_PEND",
> > +					__func__);
> > +			}
> > +			ret = IRQ_HANDLED;
> > +		}
> > +	} else {
> > +		ret = IRQ_HANDLED;
> > +	}
> > +	mutex_unlock(&s->mutex);
> > +	si4455_do_work(port);
> > +	return ret;
> > +}
> > +
> > +static irqreturn_t si4455_ist(int irq,
> > +				void *dev_id)
> > +{
> > +	struct si4455_port *s = (struct si4455_port *)dev_id;
> > +	bool handled = false;
> > +
> > +	if (si4455_port_irq(s) == IRQ_HANDLED)
> > +		handled = true;
> > +
> > +	return IRQ_RETVAL(handled);
> > +}
> > +
> > +static void si4455_tx_proc(struct work_struct *ws)
> > +{
> > +	struct si4455_one *one = container_of(ws,
> > +					struct si4455_one,
> > +					tx_work);
> > +
> > +	si4455_do_work(&one->port);
> > +}
> > +
> > +static unsigned int si4455_tx_empty(struct uart_port *port)
> > +{
> > +	return TIOCSER_TEMT;
> > +}
> > +
> > +static unsigned int si4455_get_mctrl(struct uart_port *port)
> > +{
> > +	dev_dbg(port->dev, "%s", __func__);
> > +	return TIOCM_DSR | TIOCM_CAR;
> > +}
> > +
> > +static void si4455_set_mctrl(struct uart_port *port,
> > +				unsigned int mctrl)
> > +{
> > +	dev_dbg(port->dev, "%s", __func__);
> > +}
> 
> Delete all these empty functions.
> 
> > +
> > +static void si4455_break_ctl(struct uart_port *port,
> > +				int break_state)
> > +{
> > +	dev_dbg(port->dev, "%s", __func__);
> > +}
> > +
> > +static void si4455_set_termios(struct uart_port *port,
> > +				struct ktermios *termios,
> > +				struct ktermios *old)
> > +{
> > +	dev_dbg(port->dev, "%s", __func__);
> > +}
> > +
> > +static int si4455_rs485_config(struct uart_port *port,
> > +				struct serial_rs485 *rs485)
> > +{
> > +	dev_dbg(port->dev, "%s", __func__);
> > +
> > +	return 0;
> > +}
> > +
> > +static int si4455_startup(struct uart_port *port)
> > +{
> > +	dev_dbg(port->dev, "%s", __func__);
> > +	si4455_s_power(port->dev, 1);
> > +	return 0;
> > +}
> > +
> > +static void si4455_shutdown(struct uart_port *port)
> > +{
> > +	dev_dbg(port->dev, "%s", __func__);
> > +	si4455_s_power(port->dev, 0);
> > +}
> > +
> > +static const char *si4455_type(struct uart_port *port)
> > +{
> > +	struct si4455_port *s = dev_get_drvdata(port->dev);
> > +
> > +	if (port->type == PORT_SI4455) {
> 
> Reverse this test.
> 
> > +		if (s->part_info.ROMID == 3)
> > +			return "SI4455-B1A";
> > +		else if (s->part_info.ROMID == 6)
> > +			return "SI4455-C2A";
> > +
> > +	}
> > +	return NULL;
> > +}
> > +
> > +static int si4455_request_port(struct uart_port *port)
> > +{
> > +	/* Do nothing */
> > +	dev_dbg(port->dev, "%s", __func__);
> > +	return 0;
> > +}
> > +
> > +static void si4455_config_port(struct uart_port *port,
> > +				int flags)
> > +{
> > +	dev_dbg(port->dev, "%s", __func__);
> 
> Delete all these debug statements which only print the name of the
> function.  Use ftrace for this instead.
> 
> > +	if (flags & UART_CONFIG_TYPE)
> > +		port->type = PORT_SI4455;
> > +}
> > +
> > +static int si4455_verify_port(struct uart_port *port,
> > +				struct serial_struct *s)
> > +{
> > +	if ((s->type != PORT_UNKNOWN) && (s->type != PORT_SI4455))
> > +		return -EINVAL;
> > +
> > +	if (s->irq != port->irq)
> > +		return -EINVAL;
> > +
> > +	return 0;
> > +}
> > +
> > +static void si4455_start_tx(struct uart_port *port)
> > +{
> > +	struct si4455_one *one = container_of(port,
> > +					struct si4455_one,
> > +					port);
> > +
> > +	dev_dbg(port->dev, "%s", __func__);
> > +
> > +	if (!work_pending(&one->tx_work))
> > +		schedule_work(&one->tx_work);
> > +
> > +}
> > +
> > +static int si4455_ioctl(struct uart_port *port,
> > +			unsigned int cmd,
> > +			unsigned long arg)
> > +{
> > +	struct si4455_port *s = dev_get_drvdata(port->dev);
> > +	int ret = 0;
> > +
> > +	dev_dbg(port->dev, "%s(%u)", __func__, cmd);
> > +	switch (cmd) {
> > +	case SI4455_IOC_SEZC:
> > +	memcpy(&s->configuration, (void *)arg, sizeof(s->configuration));
> > +	dev_dbg(port->dev,
> > +		"%s(%u): SI4455_IOC_SEZC, length(%i)",
> > +		__func__,
> > +		cmd,
> > +		s->configuration.length);
> > +	ret = si4455_re_configure(port);
> 
> This needs to indented another tab.
> 
> 	switch (cmd) {
> 	case SI4455_IOC_SEZC:
> 		memcpy(&s->configuration, (void *)arg, sizeof(s->configuration));
> 
> Eep!!!
> 
> Don't use memcpy() here.  Use copy_from_user().
> 
> 	void __user *argp = arg;
> 
> 		if (copy_from_user(&s->configuration, argp,
> 				   sizeof(s->configuration)))
> 			return -EFAULT;
> 
> > +	break;
> > +	case SI4455_IOC_CEZC:
> > +	dev_dbg(port->dev,
> > +		"%s(%u): SI4455_IOC_CEZC",
> > +		__func__,
> > +		cmd);
> > +	memset(&s->configuration, 0x00, sizeof(s->configuration));
> > +	ret = si4455_re_configure(port);
> > +	break;
> > +	case SI4455_IOC_SEZP:
> > +	memcpy(&s->patch, (void *)arg, sizeof(s->patch));
> > +	dev_dbg(port->dev,
> > +		"%s(%u): SI4455_IOC_SEZP, length(%i)",
> > +		__func__,
> > +		cmd,
> > +		s->configuration.length);
> > +	break;
> > +	case SI4455_IOC_CEZP:
> > +	dev_dbg(port->dev,
> > +		"%s(%u): SI4455_IOC_CEZP",
> > +		__func__,
> > +		cmd);
> > +	memset(&s->patch, 0x00, sizeof(s->patch));
> > +	break;
> > +	case SI4455_IOC_STXC:
> > +	s->tx_channel = *((u32 *)arg);
> > +	dev_dbg(port->dev,
> > +		"%s(%u): SI4455_IOC_STXC, tx_channel(%i)",
> > +		__func__,
> > +		cmd,
> > +		s->tx_channel);
> > +	break;
> > +	case SI4455_IOC_GTXC:
> > +	dev_dbg(port->dev,
> > +		"%s(%u): SI4455_IOC_GTXC",
> > +		__func__,
> > +		cmd);
> > +	*((u32 *)arg) = s->tx_channel;
> > +	break;
> > +	case SI4455_IOC_SRXC:
> > +	s->rx_channel = *((u32 *)arg);
> > +	dev_dbg(port->dev,
> > +		"%s(%u): SI4455_IOC_SRXC, rx_channel(%i)",
> > +		__func__,
> > +		cmd,
> > +		s->rx_channel);
> > +	break;
> > +	case SI4455_IOC_GRXC:
> > +	dev_dbg(port->dev,
> > +		"%s(%u): SI4455_IOC_GRXC",
> > +		__func__,
> > +		cmd);
> > +	*((u32 *)arg) = s->rx_channel;
> > +	break;
> > +	case SI4455_IOC_SSIZ:
> > +	s->package_size = *((u32 *)arg);
> > +	dev_dbg(port->dev,
> > +		"%s(%u): SI4455_IOC_SSIZ, package_size(%i)",
> > +		__func__,
> > +		cmd,
> > +		s->package_size);
> > +	break;
> > +	case SI4455_IOC_GSIZ:
> > +	dev_dbg(port->dev,
> > +		"%s(%u): SI4455_IOC_GSIZ",
> > +		__func__,
> > +		cmd);
> > +	*((u32 *)arg) = s->package_size;
> > +	break;
> > +	case SI4455_IOC_GRSSI:
> > +	dev_dbg(port->dev,
> > +		"%s(%u): SI4455_IOC_GRSSI",
> > +		__func__,
> > +		cmd);
> > +	*((u32 *)arg) = s->current_rssi;
> > +	break;
> > +	default:
> > +		ret = -ENOIOCTLCMD;
> > +	break;
> > +	}
> > +
> > +	if (ret == 0)
> > +		si4455_do_work(port);
> > +
> > +	return ret;
> > +}
> > +
> > +
> > +static void si4455_null_void(struct uart_port *port)
> > +{
> > +	/* Do nothing */
> > +}
> > +
> > +static const struct uart_ops si4455_ops = {
> > +	.tx_empty		= si4455_tx_empty,
> > +	.set_mctrl		= si4455_set_mctrl,
> > +	.get_mctrl		= si4455_get_mctrl,
> > +	.stop_tx		= si4455_null_void,
> > +	.start_tx		= si4455_start_tx,
> > +	.stop_rx		= si4455_null_void,
> > +	.break_ctl		= si4455_break_ctl,
> > +	.startup		= si4455_startup,
> > +	.shutdown		= si4455_shutdown,
> > +	.set_termios		= si4455_set_termios,
> > +	.type			= si4455_type,
> > +	.request_port		= si4455_request_port,
> > +	.release_port		= si4455_null_void,
> > +	.config_port		= si4455_config_port,
> > +	.verify_port		= si4455_verify_port,
> > +	.ioctl			= si4455_ioctl,
> > +};
> > +
> > +static int __maybe_unused si4455_suspend(struct device *dev)
> > +{
> > +	struct si4455_port *s = dev_get_drvdata(dev);
> > +
> > +	uart_suspend_port(&si4455_uart, &s->one.port);
> > +	return 0;
> > +}
> > +
> > +static int __maybe_unused si4455_resume(struct device *dev)
> > +{
> > +	struct si4455_port *s = dev_get_drvdata(dev);
> > +
> > +	uart_resume_port(&si4455_uart, &s->one.port);
> > +
> > +	return 0;
> > +}
> > +
> > +static SIMPLE_DEV_PM_OPS(si4455_pm_ops, si4455_suspend, si4455_resume);
> > +
> > +static int si4455_probe(struct device *dev,
> > +			int irq)
> > +{
> > +	int ret;
> > +	struct si4455_port *s;
> > +
> > +	/* Alloc port structure */
> > +	dev_dbg(dev, "%s\n", __func__);
> > +	s = devm_kzalloc(dev, sizeof(*s), GFP_KERNEL);
> > +	if (!s) {
> > +		dev_err(dev, "Error allocating port structure\n");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	dev_set_drvdata(dev, s);
> > +	mutex_init(&s->mutex);
> > +
> > +	s->shdn_gpio = devm_gpiod_get(dev, "shdn", GPIOD_OUT_HIGH);
> > +	if (IS_ERR(s->shdn_gpio)) {
> > +		dev_err(dev, "Unable to reguest shdn gpio\n");
> > +		ret = -EINVAL;
> 
> Preserve the error code:
> 
> 		ret = PTR_ERR(s->shdn_gpio);
> 
> > +		goto out_generic;
> > +	}
> > +
> > +	/* Initialize port data */
> > +	s->one.port.dev	= dev;
> > +	s->one.port.line = 0;
> > +	s->one.port.irq	= irq;
> > +	s->one.port.type	= PORT_SI4455;
> > +	s->one.port.fifosize	= SI4455_FIFO_SIZE;
> > +	s->one.port.flags	= UPF_FIXED_TYPE | UPF_LOW_LATENCY;
> > +	s->one.port.iotype	= UPIO_PORT;
> > +	s->one.port.iobase	= 0x00;
> > +	s->one.port.membase	= (void __iomem *)~0;
> > +	s->one.port.rs485_config = si4455_rs485_config;
> > +	s->one.port.ops	= &si4455_ops;
> > +
> > +	si4455_s_power(dev, 1);
> > +
> > +	//detect
> > +	ret = si4455_get_part_info(&s->one.port, &s->part_info);
> > +	dev_dbg(dev, "si4455_get_part_info()==%i", ret);
> > +	if (ret == 0) {
> > +		dev_dbg(dev, "partInfo.CHIPREV= %u", s->part_info.CHIPREV);
> > +		dev_dbg(dev, "partInfo.PART= %u", s->part_info.PART);
> > +		dev_dbg(dev, "partInfo.PBUILD= %u", s->part_info.PBUILD);
> > +		dev_dbg(dev, "partInfo.ID= %u", s->part_info.ID);
> > +		dev_dbg(dev, "partInfo.CUSTOMER= %u",
> > s->part_info.CUSTOMER);
> > +		dev_dbg(dev, "partInfo.ROMID= %u", s->part_info.ROMID);
> > +		dev_dbg(dev, "partInfo.BOND= %u", s->part_info.BOND);
> > +		if (s->part_info.PART != 0x5544) {
> > +			dev_err(dev, "PART(%u) error", s->part_info.PART);
> > +			ret = -ENODEV;
> > +		}
> > +	}
> > +	si4455_s_power(dev, 0);
> > +	if (ret)
> > +		goto out_generic;
> > +
> > +	/* Initialize queue for start TX */
> > +	INIT_WORK(&s->one.tx_work, si4455_tx_proc);
> > +
> > +	/* Register port */
> > +	ret = uart_add_one_port(&si4455_uart, &s->one.port);
> > +	if (ret) {
> > +		s->one.port.dev = NULL;
> > +		goto out_uart;
> > +	}
> > +
> > +	/* Setup interrupt */
> > +	ret = devm_request_threaded_irq(dev,
> > +					irq,
> > +					NULL,
> > +					si4455_ist,
> > +					IRQF_ONESHOT | IRQF_SHARED,
> > +					dev_name(dev),
> > +					s);
> > +	if (!ret)
> > +		return 0;
> 
> This is "Last if condition is reversed" anti-pattern.  Always do error
> handling, never success handling.
> 
> > +
> > +	dev_err(dev, "Unable to reguest IRQ %i\n", irq);
> > +
> > +out_uart:
> > +	uart_remove_one_port(&si4455_uart, &s->one.port);
> > +out_generic:
> > +	mutex_destroy(&s->mutex);
> > +
> > +	return ret;
> > +}
> > +
> > +static int si4455_remove(struct device *dev)
> > +{
> > +	struct si4455_port *s = dev_get_drvdata(dev);
> > +
> > +	cancel_work_sync(&s->one.tx_work);
> > +	uart_remove_one_port(&si4455_uart, &s->one.port);
> > +
> > +	mutex_destroy(&s->mutex);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id __maybe_unused si4455_dt_ids[] = {
> > +	{ .compatible = "silabs,si4455" },
> > +	{ .compatible = "silabs,si4455b1a" },
> > +	{ .compatible = "silabs,si4455c2a" },
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(of, si4455_dt_ids);
> > +
> > +static int si4455_spi_probe(struct spi_device *spi)
> > +{
> > +	int ret;
> > +
> > +	/* Setup SPI bus */
> > +	spi->bits_per_word	= 8;
> > +	spi->mode		    = SPI_MODE_0;
> > +	spi->max_speed_hz   = 100000;
> 
> This white space is whacky.
> 
> 
> > +	ret = spi_setup(spi);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (spi->dev.of_node) {
> > +		const struct of_device_id *of_id
> > +			= of_match_device(si4455_dt_ids, &spi->dev);
> > +		if (!of_id)
> > +			return -ENODEV;
> > +
> > +	}
> > +
> > +	return si4455_probe(&spi->dev, spi->irq);
> > +}
> 
> Anyway, hopefully that's some ideas.
> 
> regards,
> dan carpenter
> 
> 

Thank you for your suggestions. I made the canges suggested by you.

I'll test the driver in my development environment, then I'll come back with a new patch soon.

I set up a mail client on my linux dev environment, I hope my mails/patches/codes will be in proper quality in the future.

Üdvözlettel / Best regards:
József Horváth

On Wed, Dec 09, 2020 at 06:56:13PM +0000, József Horváth wrote:
> Thank you for your suggestions. I made the canges suggested by you.
> 
> I'll test the driver in my development environment, then I'll come
> back with a new patch soon.

Take your time.  No rush.

> 
> I set up a mail client on my linux dev environment, I hope my
> mails/patches/codes will be in proper quality in the future.

No problem at all.  Part of getting used to the process.

regards,
dan carpenter

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

On Wed, Dec 9, 2020 at 5:17 AM Info <info@ministro.hu> wrote:
>
> This is a serial port driver for
> Silicon Labs Si4455 Sub-GHz transciver.
>
> Signed-off-by: József Horváth <info@ministro.hu>
> ---
>  .../bindings/staging/serial/silabs,si4455.txt |   39 +

Looks straightforward enough to not be in staging. Plus
bindings/staging/serial is not a directory I want.

DT bindings are in DT schema format now. See
Documentation/devicetree/writing-schema.rst.

>  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
>
> 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
> +
> +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),

Either do this or...

> +  - "silabs,si4455b1a" for Silicon Labs Si4455-B1A,
> +  - "silabs,si4455c2a" for Silicon Labs Si4455-C2A,

this. Not both. I assume there's an id register or something you can
read to determine which one. That's fine, but consider if there's any
power sequencing differences that you'd need to handle before you can
read that register.

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

shutdown-gpios is the semi-standard name.

> +
> +Example:
> +
> +/ {
> +       clocks {
> +                si4455_1_2_osc: si4455_1_2_osc {
> +                        compatible = "fixed-clock";
> +                        #clock-cells = <0>;
> +                        clock-frequency  = <30000000>;
> +                };
> +       };

Don't need to show this for the example.

> +};
> +
> +&spi0 {
> +       si4455: si4455@0 {
> +               compatible = "silabs,si4455";
> +               reg = <0>;
> +               clocks = <&si4455_1_2_osc>;
> +               interrupt-parent = <&gpio>;
> +               interrupts = <7 IRQ_TYPE_LEVEL_LOW>;
> +                shdn-gpios = <&gpio 26 1>;
> +                status = "okay";

Don't show status in examples.

> +                spi-max-frequency = <3000000>;
> +       };
> +};

On Wed, Dec 09, 2020 at 07:41:53PM +0000, József Horváth wrote:
> > > +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);
> 

Use spaces to make arguments have to line up properly.
`checkpatch.pl --strict` will complain if it's not done.

	table(arg1, arg2,
	      arg_whatver, foo);
[tab][space x 7]arg_whaver, foo);

But I think Jérôme's main point was to get rid of the dataOut buffer and
use "result" directly.

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

regards,
dan carpenter