Re: [PATCH v2 3/5] spi: Add support for Renesas CSI
From: andy . shevchenko
Date: Wed Jul 05 2023 - 06:41:44 EST
Thu, Jun 22, 2023 at 12:33:39PM +0100, Fabrizio Castro kirjoitti:
> The RZ/V2M SoC comes with the Clocked Serial Interface (CSI)
> IP, which is a master/slave SPI controller.
>
> This commit adds a driver to support CSI master mode.
Submitting Patches recommends to use imperative voice.
...
+ bits.h
> +#include <linux/clk.h>
> +#include <linux/count_zeros.h>
> +#include <linux/interrupt.h>
> +#include <linux/iopoll.h>
> +#include <linux/platform_device.h>
> +#include <linux/reset.h>
> +#include <linux/spi/spi.h>
...
> +#define CSI_CKS_MAX 0x3FFF
If it's limited by number of bits, i would explicitly use that information as
(BIT(14) - 1).
...
> +#define CSI_MAX_SPI_SCKO 8000000
Units?
Also, HZ_PER_MHZ?
...
> +static const unsigned char x_trg[] = {
> + 0, 1, 1, 2, 2, 2, 2, 3,
> + 3, 3, 3, 3, 3, 3, 3, 4,
> + 4, 4, 4, 4, 4, 4, 4, 4,
> + 4, 4, 4, 4, 4, 4, 4, 5
> +};
> +
> +static const unsigned char x_trg_words[] = {
> + 1, 2, 2, 4, 4, 4, 4, 8,
> + 8, 8, 8, 8, 8, 8, 8, 16,
> + 16, 16, 16, 16, 16, 16, 16, 16,
> + 16, 16, 16, 16, 16, 16, 16, 32
> +};
Why do you need tables? At the first glance these values can be easily
calculated from indexes.
...
> + rzv2m_csi_reg_write_bit(csi, CSI_CNT, CSI_CNT_CSIRST, assert);
> +
> + if (assert) {
If (!assert)
return 0;
> + return readl_poll_timeout(csi->base + CSI_MODE, reg,
> + !(reg & CSI_MODE_CSOT), 0,
> + CSI_EN_DIS_TIMEOUT_US);
> + }
> +
> + return 0;
...
> + rzv2m_csi_reg_write_bit(csi, CSI_MODE, CSI_MODE_CSIE, enable);
> +
> + if (!enable && wait)
In the similar way.
> + return readl_poll_timeout(csi->base + CSI_MODE, reg,
> + !(reg & CSI_MODE_CSOT), 0,
> + CSI_EN_DIS_TIMEOUT_US);
> +
> + return 0;
...
> + for (i = 0; i < csi->words_to_transfer; i++)
> + writel(buf[i], csi->base + CSI_OFIFO);
writesl()?
...
> + for (i = 0; i < csi->words_to_transfer; i++)
> + buf[i] = (u16)readl(csi->base + CSI_IFIFO);
readsl()?
...
> + for (i = 0; i < csi->words_to_transfer; i++)
> + buf[i] = (u8)readl(csi->base + CSI_IFIFO);
readsl()?
...
Yes, in read cases you would need carefully handle the buffer data.
Or drop the idea if it looks too monsterous.
...
> + ret = rzv2m_csi_wait_for_interrupt(csi, CSI_INT_TREND, CSI_CNT_TREND_E);
> +
Unneeded blank line.
> + if (ret == -ETIMEDOUT)
> + csi->errors |= TX_TIMEOUT_ERROR;
...
> + struct rzv2m_csi_priv *csi = (struct rzv2m_csi_priv *)data;
From/to void * does not need an explicit casting in C.
...
> + /* Setup clock polarity and phase timing */
> + rzv2m_csi_reg_write_bit(csi, CSI_CLKSEL, CSI_CLKSEL_CKP,
> + !(spi->mode & SPI_CPOL));
> + rzv2m_csi_reg_write_bit(csi, CSI_CLKSEL, CSI_CLKSEL_DAP,
> + !(spi->mode & SPI_CPHA));
Is it a must to do in a sequential writes?
...
> + bool tx_completed = csi->txbuf ? false : true;
> + bool rx_completed = csi->rxbuf ? false : true;
Ternaries are redundant in the above.
...
> + controller->mode_bits = SPI_CPOL | SPI_CPHA | SPI_LSB_FIRST;
SPI_MODE_X_MASK
...
> + controller->dev.of_node = pdev->dev.of_node;
device_set_node();
--
With Best Regards,
Andy Shevchenko