From mboxrd@z Thu Jan 1 00:00:00 1970 From: Laurent Pinchart Date: Mon, 30 Dec 2013 23:47:33 +0000 Subject: Re: [PATCH 4/8] spi: rspi: Add support for 8-bit Data Register access Message-Id: <13614197.Z9gKDhB8kb@avalon> List-Id: References: <1387885248-28425-1-git-send-email-geert+renesas@linux-m68k.org> <1387885248-28425-5-git-send-email-geert+renesas@linux-m68k.org> In-Reply-To: <1387885248-28425-5-git-send-email-geert+renesas-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Geert Uytterhoeven Cc: linux-spi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-sh-u79uwXL29TY76Z2rM5mHXA@public.gmane.org Hi Geert, Thank you for the patch. On Tuesday 24 December 2013 12:40:44 Geert Uytterhoeven wrote: > Add support for accessing the RSPI Data Register using 8-bit operations, > based on the SDK reference code. This is needed for RZ/A1H. > > The data width is passed using platform data, defaulting to 16-bit for > legacy RSPI. QSPI always uses 8-bit accesses. > > Signed-off-by: Geert Uytterhoeven > --- > drivers/spi/spi-rspi.c | 90 ++++++++++++++++++++++++++++++++++++------- > include/linux/spi/rspi.h | 2 ++ > 2 files changed, 79 insertions(+), 13 deletions(-) > > diff --git a/drivers/spi/spi-rspi.c b/drivers/spi/spi-rspi.c > index c7bbc54ef785..00788091fc16 100644 > --- a/drivers/spi/spi-rspi.c > +++ b/drivers/spi/spi-rspi.c > @@ -179,6 +179,8 @@ struct rspi_data { > spinlock_t lock; > struct clk *clk; > u8 spsr; > + u8 spdcr; > + u8 data_width; > int irq[MAX_NUM_IRQ]; > unsigned int numirq; > const struct spi_ops *ops; > @@ -216,8 +218,27 @@ static u16 rspi_read16(const struct rspi_data *rspi, > u16 offset) return ioread16(rspi->addr + offset); > } > > +static void rspi_write_data(const struct rspi_data *rspi, u16 data) > +{ > + if (rspi->data_width = 8) > + rspi_write8(rspi, data, RSPI_SPDR); > + else if (rspi->data_width = 16) > + rspi_write16(rspi, data, RSPI_SPDR); As the driver only handles 8-bits and 16-bits data, replacing the else if with an else should be more efficient. > +} > + > +static u16 rspi_read_data(const struct rspi_data *rspi) > +{ > + if (rspi->data_width = 8) > + return rspi_read8(rspi, RSPI_SPDR); > + else if (rspi->data_width = 16) > + return rspi_read16(rspi, RSPI_SPDR); Same here. > + return 0; > +} > + > /* optional functions */ > struct spi_ops { > + int (*parse_platform_data)(struct rspi_data *rspi, > + const struct rspi_plat_data *rspi_pd); > int (*set_config_register)(const struct rspi_data *rspi, > int access_size); > int (*send_pio)(struct rspi_data *rspi, struct spi_message *mesg, > @@ -230,6 +251,33 @@ struct spi_ops { > /* > * functions for RSPI > */ > +static int rspi_parse_platform_data(struct rspi_data *rspi, > + const struct rspi_plat_data *rspi_pd) > +{ > + if (rspi_pd && rspi_pd->data_width) { > + rspi->data_width = rspi_pd->data_width; > + switch (rspi_pd->data_width) { > + case 8: > + rspi->spdcr = SPDCR_SPLBYTE; > + break; > + case 16: > + rspi->spdcr = SPDCR_SPLWORD; > + break; > + default: > + return -1; Shouldn't you return a defined error code (-EINVAL maybe) ? > + } > + } else { > + /* > + * Use legacy 16-bit width data access if a data_width value > + * isn't defined in the platform data. > + */ > + rspi->data_width = 16; > + rspi->spdcr = 0; > + } > + > + return 0; > +} > + > static int rspi_set_config_register(const struct rspi_data *rspi, > int access_size) > { > @@ -243,7 +291,7 @@ static int rspi_set_config_register(const struct > rspi_data *rspi, rspi_write8(rspi, clamp(spbr, 0, 255), RSPI_SPBR); > > /* Sets number of frames to be used: 1 frame */ > - rspi_write8(rspi, 0x00, RSPI_SPDCR); > + rspi_write8(rspi, rspi->spdcr, RSPI_SPDCR); > > /* Sets RSPCK, SSL, next-access delay value */ > rspi_write8(rspi, 0x00, RSPI_SPCKD); > @@ -266,6 +314,16 @@ static int rspi_set_config_register(const struct > rspi_data *rspi, /* > * functions for QSPI > */ > +static int qspi_parse_platform_data(struct rspi_data *rspi, > + const struct rspi_plat_data *rspi_pd) > +{ > + /* Fixed 8-bit for now */ > + rspi->data_width = 8; > + rspi->spdcr = 0; > + > + return 0; > +} > + > static int qspi_set_config_register(const struct rspi_data *rspi, > int access_size) > { > @@ -280,7 +338,7 @@ static int qspi_set_config_register(const struct > rspi_data *rspi, rspi_write8(rspi, clamp(spbr, 0, 255), RSPI_SPBR); > > /* Sets number of frames to be used: 1 frame */ > - rspi_write8(rspi, 0x00, RSPI_SPDCR); > + rspi_write8(rspi, rspi->spdcr, RSPI_SPDCR); > > /* Sets RSPCK, SSL, next-access delay value */ > rspi_write8(rspi, 0x00, RSPI_SPCKD); > @@ -365,7 +423,7 @@ static int rspi_send_pio(struct rspi_data *rspi, struct > spi_message *mesg, return -ETIMEDOUT; > } > > - rspi_write16(rspi, *data, RSPI_SPDR); > + rspi_write_data(rspi, *data); > data++; > remain--; > } > @@ -392,14 +450,14 @@ static int qspi_send_pio(struct rspi_data *rspi, > struct spi_message *mesg, "%s: tx empty timeout\n", __func__); > return -ETIMEDOUT; > } > - rspi_write8(rspi, *data++, RSPI_SPDR); > + rspi_write_data(rspi, *data++); > > if (rspi_wait_for_interrupt(rspi, SPSR_SPRF, SPCR_SPRIE) < 0) { > dev_err(&rspi->master->dev, > "%s: receive timeout\n", __func__); > return -ETIMEDOUT; > } > - rspi_read8(rspi, RSPI_SPDR); > + rspi_read_data(rspi); > > remain--; > } > @@ -539,7 +597,7 @@ static void rspi_receive_init(const struct rspi_data > *rspi) > > spsr = rspi_read8(rspi, RSPI_SPSR); > if (spsr & SPSR_SPRF) > - rspi_read16(rspi, RSPI_SPDR); /* dummy read */ > + rspi_read_data(rspi); /* dummy read */ > if (spsr & SPSR_OVRF) > rspi_write8(rspi, rspi_read8(rspi, RSPI_SPSR) & ~SPSR_OVRF, > RSPI_SPSR); > @@ -564,15 +622,14 @@ static int rspi_receive_pio(struct rspi_data *rspi, > struct spi_message *mesg, return -ETIMEDOUT; > } > /* dummy write for generate clock */ > - rspi_write16(rspi, DUMMY_DATA, RSPI_SPDR); > + rspi_write_data(rspi, DUMMY_DATA); > > if (rspi_wait_for_interrupt(rspi, SPSR_SPRF, SPCR_SPRIE) < 0) { > dev_err(&rspi->master->dev, > "%s: receive timeout\n", __func__); > return -ETIMEDOUT; > } > - /* SPDR allows 16 or 32-bit access only */ > - *data = (u8)rspi_read16(rspi, RSPI_SPDR); > + *data = rspi_read_data(rspi); > > data++; > remain--; > @@ -587,7 +644,7 @@ static void qspi_receive_init(const struct rspi_data > *rspi) > > spsr = rspi_read8(rspi, RSPI_SPSR); > if (spsr & SPSR_SPRF) > - rspi_read8(rspi, RSPI_SPDR); /* dummy read */ > + rspi_read_data(rspi); /* dummy read */ > rspi_write8(rspi, SPBFCR_TXRST | SPBFCR_RXRST, QSPI_SPBFCR); > rspi_write8(rspi, 0x00, QSPI_SPBFCR); > } > @@ -609,15 +666,14 @@ static int qspi_receive_pio(struct rspi_data *rspi, > struct spi_message *mesg, return -ETIMEDOUT; > } > /* dummy write for generate clock */ > - rspi_write8(rspi, DUMMY_DATA, RSPI_SPDR); > + rspi_write_data(rspi, DUMMY_DATA); > > if (rspi_wait_for_interrupt(rspi, SPSR_SPRF, SPCR_SPRIE) < 0) { > dev_err(&rspi->master->dev, > "%s: receive timeout\n", __func__); > return -ETIMEDOUT; > } > - /* SPDR allows 8, 16 or 32-bit access */ > - *data++ = rspi_read8(rspi, RSPI_SPDR); > + *data++ = rspi_read_data(rspi); > remain--; > } > > @@ -1002,6 +1058,12 @@ static int rspi_probe(struct platform_device *pdev) > } > } > > + ret = ops->parse_platform_data(rspi, rspi_pd); > + if (ret < 0) { > + dev_err(&pdev->dev, "rspi invalid platform data.\n"); > + goto error1; > + } > + > ret = rspi_request_dma(rspi, pdev); > if (ret < 0) { > dev_err(&pdev->dev, "rspi_request_dma failed.\n"); > @@ -1027,12 +1089,14 @@ error1: > } > > static struct spi_ops rspi_ops = { > + .parse_platform_data = rspi_parse_platform_data, > .set_config_register = rspi_set_config_register, > .send_pio = rspi_send_pio, > .receive_pio = rspi_receive_pio, > }; > > static struct spi_ops qspi_ops = { > + .parse_platform_data = qspi_parse_platform_data, > .set_config_register = qspi_set_config_register, > .send_pio = qspi_send_pio, > .receive_pio = qspi_receive_pio, > diff --git a/include/linux/spi/rspi.h b/include/linux/spi/rspi.h > index 3f55232f74ff..7316dd9c7ba9 100644 > --- a/include/linux/spi/rspi.h > +++ b/include/linux/spi/rspi.h > @@ -22,6 +22,8 @@ > #define __LINUX_SPI_RENESAS_SPI_H__ > > struct rspi_plat_data { > + u8 data_width; /* Data register access width */ > + > unsigned int dma_tx_id; > unsigned int dma_rx_id; -- Regards, Laurent Pinchart