Re: [PATCH v4] Serial: silabs si4455 serial driver
From: Jiri Slaby
Date: Mon Dec 14 2020 - 03:05:08 EST
On 12. 12. 20, 8:09, József Horváth wrote:
This is a serial port driver for
Silicon Labs Si4455 Sub-GHz transciver.
The goal of this driver is to removing wires
between central(linux) device and remote serial devices/sensors,
but keeping the original user software.
It represents regular serial interface for the user space.
Datasheet: https://www.silabs.com/documents/public/data-sheets/Si4455.pdf
A description of changes between v1..v4 here, please.
Signed-off-by: József Horváth <info@xxxxxxxxxxx>
...
--- /dev/null
+++ b/drivers/tty/serial/si4455.c
@@ -0,0 +1,1328 @@
...
+struct si4455_one {
What is this si4455_one good for? I mean, why not just inline these two
in si4455_port?
+ struct uart_port port;
+ struct work_struct tx_work;
+};
+
+struct si4455_port {
+ struct mutex mutex; /* For syncing access to device */
+ int power_count;
+ bool connected;
If you put this int and bool after u32s below, the structure won't
contain holes AFAICS.
+ struct gpio_desc *shdn_gpio;
+ struct si4455_part_info part_info;
+ struct si4455_modem_status modem_status;
+ u32 tx_channel;
+ u32 rx_channel;
+ u32 package_size;
+ bool configured;
+ bool tx_pending;
+ bool rx_pending;
+ u32 current_rssi;
+ struct si4455_one one;
+};
...
+static int si4455_get_response(struct uart_port *port, int length, u8 *data)
+{
+ int ret;
+ u8 data_out[] = { SI4455_CMD_ID_READ_CMD_BUFF };
+ u8 *data_in = NULL;
A useless initialization.
+ struct spi_transfer xfer[2];
+ int timeout = 10000;
+
+ if (length > 0 && !data)
+ return -EINVAL;
+
+ data_in = kzalloc(1 + length, GFP_KERNEL);
+ if (!data_in)
+ return -ENOMEM;
+
+ memset(&xfer, 0x00, sizeof(xfer));
+ xfer[0].tx_buf = data_out;
+ xfer[0].len = sizeof(data_out);
+ xfer[1].rx_buf = data_in;
+ xfer[1].len = 1 + length;
+
+ while (--timeout > 0) {
+ data_out[0] = SI4455_CMD_ID_READ_CMD_BUFF;
+ 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);
Missing \n. And a space before (.
+ break;
+ }
+
+ if (data_in[0] == 0xFF) {
+ if (length > 0 && data)
+ memcpy(data, &data_in[1], length);
+
+ break;
+ }
+ usleep_range(100, 200);
+ }
+ if (timeout == 0) {
+ dev_err(port->dev, "%s:timeout==%i", __func__, timeout);
+ ret = -EIO;
+ }
+ kfree(data_in);
+ return ret;
+}
...
+static int si4455_send_command(struct uart_port *port, int length, u8 *data)
+{
+ int ret;
+
+ ret = si4455_poll_cts(port);
+ if (ret) {
+ dev_err(port->dev,
+ "%s: si4455_poll_cts error(%i)", __func__, ret);
+ return ret;
+ }
Put a newline here.
+ ret = spi_write(to_spi_device(port->dev), data, length);
+ if (ret) {
+ dev_err(port->dev,
+ "%s: spi_write error(%i)", __func__, ret);
+ }
And here.
+ return ret;
+}
+
+static int si4455_send_command_get_response(struct uart_port *port,
+ int out_length, u8 *data_out,
+ int in_length, u8 *data_in)
+{
+ int ret;
+
+ ret = si4455_send_command(port, out_length, data_out);
+ if (ret) {
+ dev_err(port->dev,
+ "%s: si4455_send_command error(%i)", __func__, ret);
+ return ret;
+ }
+
+ ret = si4455_get_response(port, in_length, data_in);
+
+ return ret;
Simplify by return si4455_get_response() directly.
+}
+
+static int si4455_read_data(struct uart_port *port, u8 command, int poll,
+ int length, u8 *data)
+{
+ int ret = 0;
+ u8 data_out[] = { command };
+ struct spi_transfer xfer[] = {
+ {
+ .tx_buf = data_out,
+ .len = sizeof(data_out),
+ }, {
+ .rx_buf = data,
+ .len = length,
+ }
+ };
+
+ if (poll) {
+ ret = si4455_poll_cts(port);
+ if (ret)
+ return ret;
+ }
+
+ 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);
\n here. And in all the other dev_errs all around.
+ }
A newline here.
+ return ret;
+}
...
+static int si4455_get_int_status(struct uart_port *port, u8 ph_clear,
+ u8 modem_clear, u8 chip_clear,
+ struct si4455_int_status *result)
+{
+ int ret;
+ u8 data_out[] = {
+ SI4455_CMD_ID_GET_INT_STATUS,
+ ph_clear,
+ modem_clear,
+ chip_clear
+ };
+ u8 data_in[SI4455_CMD_REPLY_COUNT_GET_INT_STATUS];
+
+ ret = si4455_send_command_get_response(port, sizeof(data_out), data_out,
+ sizeof(data_in), data_in);
+ if (ret) {
+ dev_err(port->dev,
+ "%s: si4455_send_command_get_response error(%i)",
+ __func__, ret);
return ret; here. And no else there:
+ } else {
+ result->int_pend = data_in[0];
+ result->int_status = data_in[1];
+ result->ph_pend = data_in[2];
+ result->ph_status = data_in[3];
+ result->modem_pend = data_in[4];
+ result->modem_status = data_in[5];
+ result->chip_pend = data_in[6];
+ result->chip_status = data_in[7];
+ }
+ return ret;
+}
+
+static int si4455_get_modem_status(struct uart_port *port, u8 modem_clear,
+ struct si4455_modem_status *result)
+{
+ int ret;
+ u8 data_out[] = {
+ SI4455_CMD_ID_GET_MODEM_STATUS,
+ modem_clear,
+ };
+ u8 data_in[SI4455_CMD_REPLY_COUNT_GET_MODEM_STATUS];
+
+ ret = si4455_send_command_get_response(port, sizeof(data_out), data_out,
+ sizeof(data_in), data_in);
+ if (ret) {
+ dev_err(port->dev,
+ "%s: si4455_send_command_get_response error(%i)",
+ __func__, ret);
The same here and the other functions below.
+ } else {
+ result->modem_pend = data_in[0];
+ result->modem_status = data_in[1];
+ result->curr_rssi = data_in[2];
+ result->latch_rssi = data_in[3];
+ result->ant1_rssi = data_in[4];
+ result->ant2_rssi = data_in[5];
+ memcpy(&result->afc_freq_offset,
+ &data_in[6], sizeof(result->afc_freq_offset));
+ }
+ return ret;
+}
...
+static int si4455_configure(struct uart_port *port, const u8 *configuration_data)
+{
+ int ret = 0;
+ u8 col;
+ u8 response;
+ u8 count;
+ struct si4455_int_status int_status = { 0 };
+ u8 radio_cmd[16u];
Any reason for 16 specifically _unsigned_?
+
+ /* While cycle as far as the pointer points to a command */
+ while (*configuration_data != 0x00) {
+ /* Commands structure in the array:
+ * --------------------------------
+ * LEN | <LEN length of data>
Multiline comments should look like this:
/*
* comment 1
* comment 2
*/
+ */
+ count = *configuration_data++;
+ dev_dbg(port->dev, "%s: count(%u)",
+ __func__, count);
+ if (count > 16u) {
And here?
+ /* Initial configuration of Si4x55 */
+ if (SI4455_CMD_ID_WRITE_TX_FIFO
+ == *configuration_data) {
+ if (count > 128u) {
And here?
BTW too many nestings here. Could you switch the
SI4455_CMD_ID_WRITE_TX_FIFO if condition and break here. No need for
else branch then and the code would go one indentation level left.
+ /* Number of command bytes exceeds
+ * maximal allowable length
+ */
+ dev_err(port->dev, "%s: command length error(%i)",
+ __func__, count);
+ ret = -EINVAL;
+ break;
+ }
+
+ /* Load array to the device */
+ configuration_data++;
+ ret = si4455_write_data(port,
+ SI4455_CMD_ID_WRITE_TX_FIFO,
+ 1,
+ count - 1,
+ configuration_data);
+ if (ret) {
+ dev_err(port->dev, "%s: si4455_write_data error(%i)",
+ __func__, ret);
+ break;
+ }
+
+ /* Point to the next command */
+ configuration_data += count - 1;
+
+ /* Continue command interpreter */
+ continue;
+ } else {
+ /* Number of command bytes exceeds
+ * maximal allowable length
+ */
+ ret = -EINVAL;
+ break;
+ }
+ }
+
+ for (col = 0u; col < count; col++) {
+ radio_cmd[col] = *configuration_data;
+ configuration_data++;
+ }
+
+ dev_dbg(port->dev, "%s: radio_cmd[0](%u)", __func__, radio_cmd[0]);
+ ret = si4455_send_command_get_response(port, count, radio_cmd,
+ 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 (radio_cmd[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__, radio_cmd[0]);
+ break;
+ }
+ }
+
+ /* Get and clear all interrupts. An error has occurred... */
+ si4455_get_int_status(port, 0, 0, 0, &int_status);
+ if (int_status.chip_pend
+ & SI4455_CMD_GET_CHIP_STATUS_ERROR_PEND_MASK) {
+ ret = -EIO;
+ dev_err(port->dev, "%s: chip error(%i)",
+ __func__, int_status.chip_pend);
+ break;
+ }
+ }
+
+ 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);
+ dev_dbg(port->dev, "%s(connected=%i, configured=%i, power_count=%i)",
+ __func__, s->connected, s->configured, s->power_count);
+ if (s->connected && 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 = kzalloc(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) {
Again, too much spaghetti here.
+ 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;
+ }
+ kfree(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 irqreturn_t si4455_port_irq(struct si4455_port *s)
+{
+ struct uart_port *port = &s->one.port;
+ irqreturn_t ret = IRQ_NONE;
You never return IRQ_NONE, right?
+ struct si4455_int_status int_status = { 0 };
+ struct si4455_fifo_info fifo_info = { 0 };
+
+ mutex_lock(&s->mutex);
+ if (s->connected && s->configured && s->power_count > 0) {
+ if (!si4455_get_int_status(port, 0, 0, 0, &int_status)) {
+ si4455_get_modem_status(port, 0, &s->modem_status);
+ if (int_status.chip_pend
+ & SI4455_CMD_GET_CHIP_STATUS_ERROR_PEND_BIT) {
+ dev_err(port->dev,
+ "%s: chip_status:CMD_ERROR_PEND",
+ __func__);
+ } else if (int_status.ph_pend
+ & SI4455_CMD_GET_INT_STATUS_PACKET_SENT_PEND_BIT) {
+ dev_dbg(port->dev,
+ "%s: ph_status:PACKET_SENT_PEND",
+ __func__);
+ si4455_handle_tx_pend(s);
+ } else if (int_status.ph_pend
+ & SI4455_CMD_GET_INT_STATUS_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, &fifo_info);
+ si4455_handle_rx_pend(s);
+ } else if (int_status.ph_pend
+ & SI4455_CMD_GET_INT_STATUS_CRC_ERROR_BIT) {
+ dev_dbg(port->dev,
+ "%s: ph_status:CRC_ERROR_PEND",
+ __func__);
+ }
+ ret = IRQ_HANDLED;
+ }
+ } else {
+ ret = IRQ_HANDLED;
How can this be IRQ_HANDLED when the device is off?
+ }
+ mutex_unlock(&s->mutex);
+ si4455_do_work(port);
+ return ret;
+}
+
+static irqreturn_t si4455_ist(int irq, void *dev_id)
Why is this wrapper needed?
+{
+ 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);
A weird way of just doing:
return si4455_port_irq(s);
+}
...
+static unsigned int si4455_tx_empty(struct uart_port *port)
+{
+ return TIOCSER_TEMT;
Always free to send?
+}
+
+static unsigned int si4455_get_mctrl(struct uart_port *port)
+{
+ dev_dbg(port->dev, "%s", __func__);
+ return TIOCM_DSR | TIOCM_CAR;
And always carrier?
+}
...
+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))
This if is pointless.
+ schedule_work(&one->tx_work);
+}
...
+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);
Newline here.
+ return 0;
+}
...
+static int si4455_probe(struct device *dev,
+ int irq)
+{
+ int ret;
+ struct si4455_port *s;
+ const void *of_ptr;
+ char ez_fw_name[255] = { 0 };
+ const struct firmware *ez_fw = NULL;
+
+ /* 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, "shutdown", GPIOD_OUT_HIGH);
+ if (IS_ERR(s->shdn_gpio)) {
+ dev_err(dev, "Unable to reguest shdn gpio\n");
+ ret = -EINVAL;
+ goto out_generic;
+ }
+
+ of_ptr = of_get_property(dev->of_node, "silabs,package-size", NULL);
+ if (IS_ERR_OR_NULL(of_ptr)) {
+ dev_err(dev, "dt silabs,package-size property not present\n");
+ ret = -EINVAL;
+ goto out_generic;
+ }
+ s->package_size = be32_to_cpup(of_ptr);
+ if (s->package_size > SI4455_FIFO_SIZE) {
+ dev_err(dev, "dt silabs,package-size property maximum is %i\n", SI4455_FIFO_SIZE);
+ ret = -EINVAL;
+ goto out_generic;
+ }
+
+ of_ptr = of_get_property(dev->of_node, "silabs,tx-channel", NULL);
+ if (IS_ERR_OR_NULL(of_ptr)) {
+ dev_err(dev, "dt silabs,tx-channel property not present\n");
+ ret = -EINVAL;
+ goto out_generic;
+ }
+ s->tx_channel = be32_to_cpup(of_ptr);
+
+ of_ptr = of_get_property(dev->of_node, "silabs,rx-channel", NULL);
+ if (IS_ERR_OR_NULL(of_ptr)) {
+ dev_err(dev, "dt silabs,rx-channel property not present\n");
+ ret = -EINVAL;
+ goto out_generic;
+ }
+ s->rx_channel = be32_to_cpup(of_ptr);
+
+ of_ptr = of_get_property(dev->of_node, "silabs,ez-config", NULL);
+ if (IS_ERR_OR_NULL(of_ptr)) {
+ dev_err(dev, "dt silabs,ez-config property not present\n");
+ ret = -EINVAL;
+ goto out_generic;
+ }
+ strncpy(ez_fw_name, of_ptr, sizeof(ez_fw_name) - 1);
+
+ /* 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;
This is a cast of int to a pointer, isn't it?
+ s->one.port.rs485_config = NULL;
+ 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.chip_rev= %u", s->part_info.chip_rev);
+ 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.rom_id= %u", s->part_info.rom_id);
+ 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;
+ }
+ }
+
+ if (ret)
+ goto out_generic;
+
+ ret = request_firmware(&ez_fw, ez_fw_name, dev);
+ if (ret) {
+ dev_err(dev, "firmware(%s) request error(%i)\n", ez_fw_name, ret);
+ ret = -EINVAL;
+ goto out_generic;
+ }
+
+ ret = si4455_re_configure(&s->one.port, ez_fw);
+ release_firmware(ez_fw);
+ if (ret) {
+ dev_err(dev, "device configuration error(%i)\n", ret);
+ ret = -EINVAL;
+ goto out_generic;
+ }
+
+ /* Initialize queue for start TX */
+ INIT_WORK(&s->one.tx_work, si4455_tx_proc);
Isn't this too late? It might not be right now. But I would do it right
after allocation, along with others above.
+
+ /* Register port */
+ ret = uart_add_one_port(&si4455_uart, &s->one.port);
+ if (ret) {
+ s->one.port.dev = NULL;
+ goto out_uart;
+ }
+
+ ret = sysfs_create_group(&dev->kobj, &si4455_attr_group);
+ if (ret) {
+ dev_err(dev, "sysfs_create_group error(%i)\n", ret);
+ 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;
+
+ dev_err(dev, "Unable to reguest IRQ %i\n", irq);
+ sysfs_remove_group(&dev->kobj, &si4455_attr_group);
+
+out_uart:
+ uart_remove_one_port(&si4455_uart, &s->one.port);
+out_generic:
+ mutex_destroy(&s->mutex);
+ si4455_s_power(dev, 0);
+
+ return ret;
+}
...
+static int __init si4455_uart_init(void)
+{
+ int ret;
+
+ ret = uart_register_driver(&si4455_uart);
+ if (ret)
+ return ret;
+ spi_register_driver(&si4455_spi_driver);
spi_register_driver can fail too.
+
+ return 0;
+}
regards,
--
js