Received-SPF: pass (google.com: domain of linux-kernel+bounces-56054-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <82358d4c-f100-472a-a4ed-88f28c460698@linaro.org>
Date: Wed, 7 Feb 2024 07:04:24 +0000
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 3/4] spi: s3c64xx: add s3c64xx_iowrite{8,16}_32_rep
 accessors
Content-Language: en-US
To: Sam Protsenko <semen.protsenko@linaro.org>
Cc: broonie@kernel.org, andi.shyti@kernel.org,
 krzysztof.kozlowski@linaro.org, alim.akhtar@samsung.com,
 linux-spi@vger.kernel.org, linux-samsung-soc@vger.kernel.org,
 linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
 andre.draszik@linaro.org, peter.griffin@linaro.org, kernel-team@android.com,
 willmcvicker@google.com, robh+dt@kernel.org, conor+dt@kernel.org,
 devicetree@vger.kernel.org
References: <20240206085238.1208256-1-tudor.ambarus@linaro.org>
 <20240206085238.1208256-4-tudor.ambarus@linaro.org>
 <CAPLW+4k+FkuNwsyvWH54gcLz0YFmh8OmvOWt_LdQvELRXMvOTQ@mail.gmail.com>
From: Tudor Ambarus <tudor.ambarus@linaro.org>
In-Reply-To: <CAPLW+4k+FkuNwsyvWH54gcLz0YFmh8OmvOWt_LdQvELRXMvOTQ@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit


On 2/6/24 18:44, Sam Protsenko wrote:
> On Tue, Feb 6, 2024 at 2:52 AM Tudor Ambarus <tudor.ambarus@linaro.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@linaro.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.

ok, will change if I send a new version.
> 
>>   *
>>   * 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.

count will overflow, but shouldn't matter as it's not used afterwards.
But I would keep the style as it is, if you don't mind, it follows other
similar implementations from include/asm-generic/io.h. I'd like to be
consistent with the coding style of the generic implementations.

> 
>> +}
>> +
>> +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.

Ok, will add the local variables if I send again. I hope you're fine
with adding the local variables when introducing the method.
> 
>> +
>> +       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.
> 

I'm not a fan of having preparation patches for small changes like this,
it hides the scope. As the patch is now one can see that the logic
extends and would have made the indentation horrible if not introducing
a dedicated method. No preparation patch, please.