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
>
