Re: [PATCH] Staging: silabs si4455 serial driver

[Dan Carpenter]

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@xxxxxxxxxxx>
> + *
> + */
> +#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