From: Sam Protsenko <semen.protsenko@linaro.org>
To: Tudor Ambarus <tudor.ambarus@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
Subject: Re: [PATCH 3/4] spi: s3c64xx: add s3c64xx_iowrite{8,16}_32_rep accessors
Date: Tue, 6 Feb 2024 12:44:13 -0600 [thread overview]
Message-ID: <CAPLW+4k+FkuNwsyvWH54gcLz0YFmh8OmvOWt_LdQvELRXMvOTQ@mail.gmail.com> (raw)
In-Reply-To: <20240206085238.1208256-4-tudor.ambarus@linaro.org>
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.
> *
> * 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
>
next prev parent reply other threads:[~2024-02-06 18:44 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-06 8:52 [PATCH 0/4] spi: s3c64xx: add support for google,gs101-spi Tudor Ambarus
2024-02-06 8:52 ` [PATCH 1/4] spi: s3c64xx: explicitly include <linux/types.h> Tudor Ambarus
2024-02-06 9:56 ` Peter Griffin
2024-02-06 18:02 ` Sam Protsenko
2024-02-07 6:20 ` Tudor Ambarus
2024-02-07 9:58 ` Greg Kroah-Hartman
2024-02-07 10:14 ` Tudor Ambarus
2024-02-07 15:42 ` Sam Protsenko
2024-02-07 16:07 ` Mark Brown
2024-02-06 8:52 ` [PATCH 2/4] spi: dt-bindings: samsung: add google,gs101-spi compatible Tudor Ambarus
2024-02-06 9:57 ` Peter Griffin
2024-02-06 8:52 ` [PATCH 3/4] spi: s3c64xx: add s3c64xx_iowrite{8,16}_32_rep accessors Tudor Ambarus
2024-02-06 18:44 ` Sam Protsenko [this message]
2024-02-07 7:04 ` Tudor Ambarus
2024-02-06 8:52 ` [PATCH 4/4] spi: s3c64xx: add support for google,gs101-spi Tudor Ambarus
2024-02-06 10:12 ` Peter Griffin
2024-02-06 11:04 ` Mark Brown
2024-02-06 11:19 ` Tudor Ambarus
2024-02-06 12:00 ` Peter Griffin
2024-02-06 18:51 ` Sam Protsenko
2024-02-06 18:59 ` [PATCH 0/4] " Sam Protsenko
2024-02-07 7:50 ` Tudor Ambarus
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAPLW+4k+FkuNwsyvWH54gcLz0YFmh8OmvOWt_LdQvELRXMvOTQ@mail.gmail.com \
--to=semen.protsenko@linaro.org \
--cc=alim.akhtar@samsung.com \
--cc=andi.shyti@kernel.org \
--cc=andre.draszik@linaro.org \
--cc=broonie@kernel.org \
--cc=conor+dt@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=kernel-team@android.com \
--cc=krzysztof.kozlowski@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-samsung-soc@vger.kernel.org \
--cc=linux-spi@vger.kernel.org \
--cc=peter.griffin@linaro.org \
--cc=robh+dt@kernel.org \
--cc=tudor.ambarus@linaro.org \
--cc=willmcvicker@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).