lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Thu, 13 Jul 2023 15:52:11 +0000
From:   Fabrizio Castro <fabrizio.castro.jz@...esas.com>
To:     "andy.shevchenko@...il.com" <andy.shevchenko@...il.com>
CC:     Mark Brown <broonie@...nel.org>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        Geert Uytterhoeven <geert+renesas@...der.be>,
        Magnus Damm <magnus.damm@...il.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-spi@...r.kernel.org" <linux-spi@...r.kernel.org>,
        "linux-renesas-soc@...r.kernel.org" 
        <linux-renesas-soc@...r.kernel.org>,
        Chris Paterson <Chris.Paterson2@...esas.com>,
        Biju Das <biju.das@...renesas.com>
Subject: RE: [PATCH v2 3/5] spi: Add support for Renesas CSI

Hi Andy,

Thanks for your feedback!

> From: andy.shevchenko@...il.com <andy.shevchenko@...il.com>
> 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
> 

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ