On Tue, Feb 6, 2024 at 2:52 AM Tudor Ambarus <tudor.ambarus@xxxxxxxxxx> wrote:
>
> Allow SoCs that require 32 bits register accesses to write data in
> chunks of 8 or 16 bits. One SoC that requires 32 bit register accesses
> is the google gs101. The operation is rare, thus open code it in the
> driver rather than making it generic (through asm-generic/io.h)
>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@xxxxxxxxxx>
> ---
> drivers/spi/spi-s3c64xx.c | 70 +++++++++++++++++++++++++++++++--------
> 1 file changed, 56 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
> index c15ca6a910dc..cb45ad615f3d 100644
> --- a/drivers/spi/spi-s3c64xx.c
> +++ b/drivers/spi/spi-s3c64xx.c
> @@ -142,6 +142,7 @@ struct s3c64xx_spi_dma_data {
> * prescaler unit.
> * @clk_ioclk: True if clock is present on this device
> * @has_loopback: True if loopback mode can be supported
> + * @use_32bit_io: True if the SoC allows just 32-bit register accesses.
A matter of taste, but: just -> only.
> *
> * The Samsung s3c64xx SPI controller are used on various Samsung SoC's but
> * differ in some aspects such as the size of the fifo and spi bus clock
> @@ -158,6 +159,7 @@ struct s3c64xx_spi_port_config {
> bool clk_from_cmu;
> bool clk_ioclk;
> bool has_loopback;
> + bool use_32bit_io;
> };
>
> /**
> @@ -412,6 +414,59 @@ static bool s3c64xx_spi_can_dma(struct spi_controller *host,
> return false;
> }
>
> +static void s3c64xx_iowrite8_32_rep(volatile void __iomem *addr,
> + const void *buffer, unsigned int count)
> +{
> + if (count) {
> + const u8 *buf = buffer;
> +
> + do {
> + __raw_writel(*buf++, addr);
> + } while (--count);
> + }
How about:
while (count--)
__raw_writel(*buf++, addr);
This way "if" condition is not needed. The same goes for the function below.
> +}
> +
> +static void s3c64xx_iowrite16_32_rep(volatile void __iomem *addr,
> + const void *buffer, unsigned int count)
> +{
> + if (count) {
> + const u16 *buf = buffer;
> +
> + do {
> + __raw_writel(*buf++, addr);
> + } while (--count);
> + }
> +}
> +
> +static void s3c64xx_iowrite_rep(const struct s3c64xx_spi_driver_data *sdd,
> + struct spi_transfer *xfer)
> +{
> + void __iomem *regs = sdd->regs;
Suggest declaring aliases here, like this:
void __iomem *addr = sdd->regs + S3C64XX_SPI_TX_DATA;
const void *buf = xfer->tx_buf;
unsigned int len = xfer->len;
Using those in the code below makes it more compact and easier to read.
> +
> + switch (sdd->cur_bpw) {
> + case 32:
> + iowrite32_rep(regs + S3C64XX_SPI_TX_DATA,
> + xfer->tx_buf, xfer->len / 4);
> + break;
> + case 16:
> + if (sdd->port_conf->use_32bit_io)
> + s3c64xx_iowrite16_32_rep(regs + S3C64XX_SPI_TX_DATA,
> + xfer->tx_buf, xfer->len / 2);
> + else
> + iowrite16_rep(regs + S3C64XX_SPI_TX_DATA,
> + xfer->tx_buf, xfer->len / 2);
> + break;
> + default:
> + if (sdd->port_conf->use_32bit_io)
> + s3c64xx_iowrite8_32_rep(regs + S3C64XX_SPI_TX_DATA,
> + xfer->tx_buf, xfer->len);
> + else
> + iowrite8_rep(regs + S3C64XX_SPI_TX_DATA,
> + xfer->tx_buf, xfer->len);
> + break;
> + }
> +}
> +
> static int s3c64xx_enable_datapath(struct s3c64xx_spi_driver_data *sdd,
> struct spi_transfer *xfer, int dma_mode)
> {
> @@ -445,20 +500,7 @@ static int s3c64xx_enable_datapath(struct s3c64xx_spi_driver_data *sdd,
> modecfg |= S3C64XX_SPI_MODE_TXDMA_ON;
> ret = s3c64xx_prepare_dma(&sdd->tx_dma, &xfer->tx_sg);
> } else {
> - switch (sdd->cur_bpw) {
> - case 32:
> - iowrite32_rep(regs + S3C64XX_SPI_TX_DATA,
> - xfer->tx_buf, xfer->len / 4);
> - break;
> - case 16:
> - iowrite16_rep(regs + S3C64XX_SPI_TX_DATA,
> - xfer->tx_buf, xfer->len / 2);
> - break;
> - default:
> - iowrite8_rep(regs + S3C64XX_SPI_TX_DATA,
> - xfer->tx_buf, xfer->len);
> - break;
> - }
> + s3c64xx_iowrite_rep(sdd, xfer);
This extraction (with no functional change yet) could've been a
preceding separate patch, preparing things for this rework.
> }
> }
>
> --
> 2.43.0.594.gd9cf4e227d-goog
>
