Re: [PATCH 1/2] SPI: Add SPI driver for Sunplus SP7021

[Mark Brown]

On Mon, Nov 01, 2021 at 02:18:44PM +0800, LH.Kuo wrote:

> Add SPI driver for Sunplus SP7021.

In general it looks like this needs quite a bit of a refresh for
mainline - a lot of it looks to be a combination of minor, easily
fixable things and stylistic issues which make things hard to follow but
I think there are some more substantial things going on here as well.

One big thing is that the driver appears to copy everything through
bounce buffers ore or less unconditionally.  It's not clear why it's
doing this - if it's for DMA usage in general the buffers supplied to
SPI devices should be suitable for this.

Stylistically the code just doesn't really look like Linux code, there's
a few frequent issues I've highlighted in the review but there's
probably more that are masked, it'd be worth visually comparing the
driver to others and making sure it looks similar.

> +++ b/drivers/spi/spi-sunplus-sp7021.c
> @@ -0,0 +1,1356 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Sunplus SPI controller driver
> + *
> + * Author: Sunplus Technology Co., Ltd.

Please make the entire comment a C++ one so things look more
intentional.

> +//#define DEBUG

Please drop commented out lines like this, there's *lots* of this in the
driver.

> +/* ---------------------------------------------------------------------------------------------- */
> +
> +//#define SPI_FUNC_DEBUG
> +//#define SPI_DBG_INFO
> +#define SPI_DBG_ERR

All this stuff is just duplicating the standard facilities we have in
the kernel for controlling output, please remove it and convert things
over to using normal logging infrastructure rather than a custom setup
for your device driver.

> +#define SPI_FULL_DUPLEX
> +
> +#define MAS_REG_NAME "spi_master"
> +#define SLA_REG_NAME "spi_slave"
> +
> +#define DMA_IRQ_NAME "dma_w_intr"
> +#define MAS_IRQ_NAME "mas_risc_intr"
> +
> +#define SLA_IRQ_NAME "slave_risc_intr"
> +
> +#define SPI_TRANS_DATA_CNT (4)
> +#define SPI_TRANS_DATA_SIZE (255)
> +#define SPI_MSG_DATA_SIZE (SPI_TRANS_DATA_SIZE * SPI_TRANS_DATA_CNT)
> +
> +
> +#define CLEAR_MASTER_INT (1<<6)
> +#define MASTER_INT (1<<7)
> +
> +#define DMA_READ (0xd)
> +#define SLA_SW_RST (1<<1)

Many of these names seem very generic and likely to have collisions in
future, please use prefixes to avoid potential future collisions.

> +struct SPI_MAS {
> +	// Group 091 : SPI_MASTER
> +	unsigned int MST_TX_DATA_ADDR                      ; // 00  (ADDR : 0x9C00_2D80)

Please try to follow the kernel coding style, we don't use upper case
for struct or variable names.

> +	unsigned int MST_TX_DATA_3_2_1_0                   ; // 01  (ADDR : 0x9C00_2D84)

I don't know what these numbers at the end of variable names are
intended to represent but they don't feel like they're helping.

> +	u8 tx_data_buf[SPI_TRANS_DATA_SIZE];
> +	u8 rx_data_buf[SPI_TRANS_DATA_SIZE];

These look like they'd be better as dynamically allocated buffers, aside
from anything else this will ensure that they are well aligned for DMA.

> +static unsigned int bufsiz = 4096;

This should be per device.

> +static void pentagram_set_cs(struct spi_device *_s, bool _on)
> +{
> +	if (_s->mode & SPI_NO_CS)
> +		return;
> +	if (!(_s->cs_gpiod))
> +		return;
> +	dev_dbg(&(_s->dev), "%d gpiod:%d", _on, desc_to_gpio(_s->cs_gpiod));
> +	if (_s->mode & SPI_CS_HIGH)
> +		_on = !_on;
> +	gpiod_set_value_cansleep(_s->cs_gpiod, _on);
> +}

If the device has a GPIO chip select it should use the core support for
GPIO chip selects rather than open coding.

> +static irqreturn_t pentagram_spi_S_irq(int _irq, void *_dev)
> +{
> +	unsigned long flags;
> +	struct pentagram_spi_master *pspim = (struct pentagram_spi_master *)_dev;
> +	struct SPI_SLA *sr = (struct SPI_SLA *)pspim->sla_base;
> +
> +	FUNC_DBG();
> +	spin_lock_irqsave(&pspim->lock, flags);

If you're in an interrupt you should only use the regular spin lock, no
need to save IRQ status.

> +	writel(readl(&sr->RISC_INT_DATA_RDY) | CLEAR_SLAVE_INT, &sr->RISC_INT_DATA_RDY);
> +	spin_unlock_irqrestore(&pspim->lock, flags);
> +	complete(&pspim->sla_isr);
> +	return IRQ_HANDLED;

This appears to unconditionally ack an interrupt regardless of any
status bits?  This seems to be a problem for all the interrupts in this
functions.

> +		writel(readl(&spis_reg->RISC_INT_DATA_RDY) | SLAVE_DATA_RDY,
> +					&spis_reg->RISC_INT_DATA_RDY);
> +		//regs1->SLV_DMA_CTRL = 0x4d;
> +		//regs1->SLV_DMA_LENGTH = 0x50;
> +		//regs1->SLV_DMA_INI_ADDR = 0x300;
> +		//regs1->RISC_INT_DATA_RDY |= 0x1;

This commented out stuff looks worrying...

> +		if (!wait_for_completion_timeout(&pspim->isr_done, timeout)) {
> +			dev_err(&dev, "wait_for_completion_timeout\n");
> +			goto exit_spi_slave_rw;
> +		}

> +exit_spi_slave_rw:
> +	mutex_unlock(&pspim->buf_lock);
> +	return 0;
> +}

If there's an error it's not reported anywhere.

> +	if (RW_phase == SPI_SLAVE_WRITE) {
> +		DBG_INF("S_WRITE len %d", _t->len);
> +		reinit_completion(&pspim->sla_isr);
> +
> +		if (_t->tx_dma == pspim->tx_dma_phy_base)
> +			memcpy(pspim->tx_dma_vir_base, _t->tx_buf, _t->len);
> +
> +		writel_relaxed(DMA_WRITE, &spis_reg->SLV_DMA_CTRL);
> +		writel_relaxed(_t->len, &spis_reg->SLV_DMA_LENGTH);
> +		writel_relaxed(_t->tx_dma, &spis_reg->SLV_DMA_INI_ADDR);
> +		writel(readl(&spis_reg->RISC_INT_DATA_RDY) | SLAVE_DATA_RDY,
> +				&spis_reg->RISC_INT_DATA_RDY);
> +
> +		if (wait_for_completion_interruptible(&pspim->sla_isr))
> +			dev_err(devp, "%s() wait_for_completion timeout\n", __func__);
> +
> +	} else if (RW_phase == SPI_SLAVE_READ) {

These two cases share no code, they should probably be separate
functions (and what happens if it's an unknown phase?).

> +	}
> +}
> +void sp7021spi_wb(struct pentagram_spi_master *_m, u8 _len)

Missing blank line between functions.  In general more blank lines
between blocks would help.

> +{
> +	struct SPI_MAS *sr = (struct SPI_MAS *)_m->mas_base;

These _ names really aren't at all idiomatic for the kernel.  This is a
problem throughout the driver, it's a barrier to legibility.  I'm also
concerned that it looks like you're trying to do accesses to the device
via a struct dereference rather than readl/writel - this will probably
work a lot of the time but I'm not sure it's guaranteed.

> +static irqreturn_t pentagram_spi_M_irq(int _irq, void *_dev)

Nor are capitals in function names.

> +static void spspi_delay_ns(u32 _ns)
> +{
> +

Why is this open coded?

> +	if (_t->speed_hz <= _s->max_speed_hz)
> +		div = clk_rate / _t->speed_hz;
> +	else if (_s->max_speed_hz <= _c->max_speed_hz)
> +		div = clk_rate / _s->max_speed_hz;
> +	else if (_c->max_speed_hz < pspim->spi_max_frequency)
> +		div = clk_rate / _c->max_speed_hz;
> +	else
> +		div = clk_rate / pspim->spi_max_frequency;

The core will set the speed up for the transfer, don't open code this.

> +	if (pspim->tx_cur_len < data_len) {
> +		len_temp = min(pspim->data_unit, data_len);
> +		sp7021spi_wb(pspim, len_temp);
> +	}

What if there's more data?

> +	data_len = 0;
> +	t = first;
> +	for (i = 0; i < transfers_cnt; i++) {
> +		if (t->rx_buf)
> +			memcpy(t->rx_buf, &pspim->rx_data_buf[data_len], t->len);
> +
> +		data_len += t->len;
> +		t = list_entry(t->transfer_list.next, struct spi_transfer, transfer_list);
> +	}

I find it difficult to convince myself that this isn't going to have any
overflow issues, and it'll break operation with anything that does any
manipulation of chip select during the message.  Given that the device
uses GPIO chip selects I'd expect it to just implement transfer_one()
and not handle messages at all.  This would greatly simplify the code.

> +#ifdef CONFIG_PM_RUNTIME_SPI
> +
> +	struct pentagram_spi_master *pspim = spi_controller_get_devdata(_s->controller);
> +
> +	if (pm_runtime_enabled(pspim->dev)) {
> +		ret = pm_runtime_get_sync(pspim->dev)
> +		if (ret < 0)
> +			goto pm_out;
> +	}
> +#endif
> +
> +#ifdef CONFIG_PM_RUNTIME_SPI

The runtime PM code compiles out when disabled, you shouldn't need these
ifdefs (and you definitely shouldn't need two blocks for the same define
together like this).

> +static int pentagram_spi_controller_unprepare_message(struct spi_controller *ctlr,
> +				    struct spi_message *msg)
> +{
> +	FUNC_DBG();
> +	return 0;
> +}

Remove empty functions, the core will cope.

> +static int pentagram_spi_S_transfer_one(struct spi_controller *_c, struct spi_device *_s,
> +					struct spi_transfer *_t)
> +{

So we are using transfer_one?  In that case I'm very confused why the
driver would be walking a transfer list...

> +	struct pentagram_spi_master *pspim = spi_master_get_devdata(_c);
> +	struct device *dev = pspim->dev;
> +	int mode = SPI_IDLE, ret = 0;
> +
> +	FUNC_DBG();
> +#ifdef CONFIG_PM_RUNTIME_SPI
> +	if (pm_runtime_enabled(pspim->dev)) {
> +		ret = pm_runtime_get_sync(pspim->dev);
> +		if (ret < 0)
> +			goto pm_out;
> +	}
> +#endif

Let the core handle runtime PM for you, don't open code it.

> +
> +	if (spi_controller_is_slave(_c)) {
> +
> +		pspim->isr_flag = SPI_IDLE;
> +
> +		if ((_t->tx_buf) && (_t->rx_buf)) {
> +			dev_dbg(&_c->dev, "%s() wrong command\n", __func__);

Return an error, don't just ignore problems.

> +		} else if (_t->tx_buf) {
> +			/* tx_buf is a const void* where we need a void * for
> +			 * the dma mapping
> +			 */
> +			void *nonconst_tx = (void *)_t->tx_buf;

Why do other drivers not need this?  Are you *sure* that's just getting
rid of a const here?

> +			} else
> +				mode = SPI_SLAVE_WRITE;

{ } on both sides of the ifelse.

> +		switch (mode) {
> +		case SPI_SLAVE_WRITE:
> +		case SPI_SLAVE_READ:
> +			pentagram_spi_S_rw(_s, _t, mode);
> +			break;
> +		default:
> +			DBG_INF("idle?");
> +			break;
> +		}

> +	pdev->id = 0;

Why?

> +	ctlr->bus_num = pdev->id;
> +	// flags, understood by the driver
> +	ctlr->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH | SPI_LSB_FIRST;
> +	ctlr->bits_per_word_mask = SPI_BPW_MASK(8);
> +	ctlr->min_speed_hz = 40000;
> +	ctlr->max_speed_hz = 50000000;
> +	// ctlr->flags = 0

The driver should at least be flagging itself as half duplex.

> +	/* find and map our resources */
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, MAS_REG_NAME);
> +	if (res) {
> +		pspim->mas_base = devm_ioremap_resource(&pdev->dev, res);

devm_platform_ioremap_resource_by_name().

> +	FUNC_DBG();
> +
> +	reset_control_assert(pspim->rstc);
> +
> +	return 0;
> +}
> +
> +static int pentagram_spi_controller_resume(struct platform_device *pdev)
> +{
> +	struct spi_controller *ctlr = platform_get_drvdata(pdev);
> +	struct pentagram_spi_master *pspim = spi_master_get_devdata(ctlr);
> +
> +	FUNC_DBG();
> +
> +	reset_control_deassert(pspim->rstc);
> +	clk_prepare_enable(pspim->spi_clk);

There's no matching disable...

> +#ifdef CONFIG_PM_RUNTIME_SPI
> +		.pm     = &sp7021_spi_pm_ops,
> +#endif

There's a helper for this.

Attachment: signature.asc
Description: PGP signature