Message-ID: <CAPLW+4k+FkuNwsyvWH54gcLz0YFmh8OmvOWt_LdQvELRXMvOTQ@mail.gmail.com>
Date: Tue, 6 Feb 2024 12:44:13 -0600
From: Sam Protsenko <semen.protsenko@...aro.org>
To: Tudor Ambarus <tudor.ambarus@...aro.org>
Cc: broonie@...nel.org, andi.shyti@...nel.org, krzysztof.kozlowski@...aro.org, 
	alim.akhtar@...sung.com, linux-spi@...r.kernel.org, 
	linux-samsung-soc@...r.kernel.org, linux-arm-kernel@...ts.infradead.org, 
	linux-kernel@...r.kernel.org, andre.draszik@...aro.org, 
	peter.griffin@...aro.org, kernel-team@...roid.com, willmcvicker@...gle.com, 
	robh+dt@...nel.org, conor+dt@...nel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH 3/4] spi: s3c64xx: add s3c64xx_iowrite{8,16}_32_rep accessors

On Tue, Feb 6, 2024 at 2:52 AM Tudor Ambarus <tudor.ambarus@...aro.org> 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@...aro.org>
> ---
>  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
>

Powered by blists - more mailing lists