RE: [PATCH v2 3/5] spi: Add support for Renesas CSI

[Fabrizio Castro <fabrizio.castro.jz@xxxxxxxxxxx>]

Hi Andy,

Thanks for your feedback!

> From: andy.shevchenko@xxxxxxxxx <andy.shevchenko@xxxxxxxxx>
> Subject: Re: [PATCH v2 3/5] spi: Add support for Renesas CSI
> 
> 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

Okay

> 
> > +#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).

That value represents the register setting for the maximum clock divider.
The maximum divider and corresponding register setting are plainly stated
in the HW User Manual, therefore I would like to use either (plain) value
to make it easier for the reader.

I think perhaps the below makes this clearer:
#define CSI_CKS_MAX_DIV_RATIO   32766
#define CSI_CKS_MAX             (CSI_CKS_MAX_DIV_RATIO >> 1)


> 
> ...
> 
> > +#define CSI_MAX_SPI_SCKO	8000000
> 
> Units?
> Also, HZ_PER_MHZ?

I'll replace that with:
#define CSI_MAX_SPI_SCKO        (8 * 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.

I think I am going to replace those tables with the below (and of course
adjust the callers accordingly since the argument is not an index anymore):

static inline unsigned int x_trg(unsigned int words)
{
	return fls(words) - 1;
}

static inline unsigned int x_trg_words(unsigned int words)
{
	return 1 << x_trg(words);
}

> 
> ...
> 
> > +	rzv2m_csi_reg_write_bit(csi, CSI_CNT, CSI_CNT_CSIRST, assert);
> > +
> > +	if (assert) {
> 
> 	If (!assert)
> 		return 0;

Can do

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

Okay

> 
> > +		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()?

I think you mean writesw and writesb.
They should simplify things a bit, I'll go for them.

> 
> ...
> 
> > +		for (i = 0; i < csi->words_to_transfer; i++)
> > +			buf[i] = (u16)readl(csi->base + CSI_IFIFO);
> 
> readsl()?

I'll use readsw

> 
> ...
> 
> > +		for (i = 0; i < csi->words_to_transfer; i++)
> > +			buf[i] = (u8)readl(csi->base + CSI_IFIFO);
> 
> readsl()?

I'll use readsb

> 
> ...
> 
> Yes, in read cases you would need carefully handle the buffer data.
> Or drop the idea if it looks too monsterous.

It should be okay in my case. Thanks.

> 
> ...
> 
> > +	ret = rzv2m_csi_wait_for_interrupt(csi, CSI_INT_TREND,
> CSI_CNT_TREND_E);
> 
> > +
> 
> Unneeded blank line.

Will take out

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

Of course.

> 
> ...
> 
> > +	/* 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?

It's not a must, I'll combine those 2 statements into 1.

> 
> ...
> 
> > +	bool tx_completed = csi->txbuf ? false : true;
> > +	bool rx_completed = csi->rxbuf ? false : true;
> 
> Ternaries are redundant in the above.

They are also horrible, the below should do:
bool tx_completed = !csi->txbuf;
bool rx_completed = !csi->rxbuf;

> 
> ...
> 
> > +	controller->mode_bits = SPI_CPOL | SPI_CPHA | SPI_LSB_FIRST;
> 
> SPI_MODE_X_MASK

This statement sets the mode_bits. Using a macro meant to be used as a
mask in this context is something I would want to avoid if possible.

> 
> ...
> 
> > +	controller->dev.of_node = pdev->dev.of_node;
> 
> device_set_node();

Will change.

I'll send an enhancement patch shortly to reflect your suggestions and
also Geert's.

Thanks for your help!

Cheers,
Fab

> 
> --
> With Best Regards,
> Andy Shevchenko
>