[PATCH 1/3] spi: sprd: Pass offset instead of physical address to adi_read/_write()

[PATCH 1/3] spi: sprd: Pass offset instead of physical address to adi_read/_write()

[Chunyan Zhang]

From: Chunyan Zhang <chunyan.zhang@unisoc.com>

The register offset would be added a physical address base and then pass to
the function sprd_adt_read()/_write() each time before calling them. So we
can do that within these two functions instead, that would make the code
more clear.

Signed-off-by: Chunyan Zhang <chunyan.zhang@unisoc.com>
---
 drivers/spi/spi-sprd-adi.c | 105 ++++++++++++++++---------------------
 1 file changed, 44 insertions(+), 61 deletions(-)

diff --git a/drivers/spi/spi-sprd-adi.c b/drivers/spi/spi-sprd-adi.c
index ab19068be867..abdad1ea7b38 100644
--- a/drivers/spi/spi-sprd-adi.c
+++ b/drivers/spi/spi-sprd-adi.c
@@ -117,24 +117,18 @@ struct sprd_adi {
 	struct notifier_block	restart_handler;
 };
 
-static int sprd_adi_check_paddr(struct sprd_adi *sadi, u32 paddr)
+static int sprd_adi_check_addr(struct sprd_adi *sadi, u32 reg)
 {
-	if (paddr < sadi->slave_pbase || paddr >
-	    (sadi->slave_pbase + ADI_SLAVE_ADDR_SIZE)) {
+	if (reg > ADI_SLAVE_ADDR_SIZE) {
 		dev_err(sadi->dev,
-			"slave physical address is incorrect, addr = 0x%x\n",
-			paddr);
+			"slave address offset is incorrect, reg = 0x%x\n",
+			reg);
 		return -EINVAL;
 	}
 
 	return 0;
 }
 
-static unsigned long sprd_adi_to_vaddr(struct sprd_adi *sadi, u32 paddr)
-{
-	return (paddr - sadi->slave_pbase + sadi->slave_vbase);
-}
-
 static int sprd_adi_drain_fifo(struct sprd_adi *sadi)
 {
 	u32 timeout = ADI_FIFO_DRAIN_TIMEOUT;
@@ -161,11 +155,11 @@ static int sprd_adi_fifo_is_full(struct sprd_adi *sadi)
 	return readl_relaxed(sadi->base + REG_ADI_ARM_FIFO_STS) & BIT_FIFO_FULL;
 }
 
-static int sprd_adi_read(struct sprd_adi *sadi, u32 reg_paddr, u32 *read_val)
+static int sprd_adi_read(struct sprd_adi *sadi, u32 reg, u32 *read_val)
 {
 	int read_timeout = ADI_READ_TIMEOUT;
 	unsigned long flags;
-	u32 val, rd_addr;
+	u32 val, rd_addr, paddr;
 	int ret = 0;
 
 	if (sadi->hwlock) {
@@ -178,11 +172,16 @@ static int sprd_adi_read(struct sprd_adi *sadi, u32 reg_paddr, u32 *read_val)
 		}
 	}
 
+	ret = sprd_adi_check_addr(sadi, reg);
+	if (ret)
+		goto out;
+
 	/*
 	 * Set the physical register address need to read into RD_CMD register,
 	 * then ADI controller will start to transfer automatically.
 	 */
-	writel_relaxed(reg_paddr, sadi->base + REG_ADI_RD_CMD);
+	paddr = sadi->slave_pbase + reg;
+	writel_relaxed(paddr, sadi->base + REG_ADI_RD_CMD);
 
 	/*
 	 * Wait read operation complete, the BIT_RD_CMD_BUSY will be set
@@ -212,9 +211,9 @@ static int sprd_adi_read(struct sprd_adi *sadi, u32 reg_paddr, u32 *read_val)
 	 */
 	rd_addr = (val & RD_ADDR_MASK) >> RD_ADDR_SHIFT;
 
-	if (rd_addr != (reg_paddr & REG_ADDR_LOW_MASK)) {
+	if (rd_addr != (paddr & REG_ADDR_LOW_MASK)) {
 		dev_err(sadi->dev, "read error, reg addr = 0x%x, val = 0x%x\n",
-			reg_paddr, val);
+			paddr, val);
 		ret = -EIO;
 		goto out;
 	}
@@ -227,9 +226,8 @@ static int sprd_adi_read(struct sprd_adi *sadi, u32 reg_paddr, u32 *read_val)
 	return ret;
 }
 
-static int sprd_adi_write(struct sprd_adi *sadi, u32 reg_paddr, u32 val)
+static int sprd_adi_write(struct sprd_adi *sadi, u32 reg, u32 val)
 {
-	unsigned long reg = sprd_adi_to_vaddr(sadi, reg_paddr);
 	u32 timeout = ADI_FIFO_DRAIN_TIMEOUT;
 	unsigned long flags;
 	int ret;
@@ -244,6 +242,10 @@ static int sprd_adi_write(struct sprd_adi *sadi, u32 reg_paddr, u32 val)
 		}
 	}
 
+	ret = sprd_adi_check_addr(sadi, reg);
+	if (ret)
+		goto out;
+
 	ret = sprd_adi_drain_fifo(sadi);
 	if (ret < 0)
 		goto out;
@@ -254,7 +256,8 @@ static int sprd_adi_write(struct sprd_adi *sadi, u32 reg_paddr, u32 val)
 	 */
 	do {
 		if (!sprd_adi_fifo_is_full(sadi)) {
-			writel_relaxed(val, (void __iomem *)reg);
+			/* we need virtual register address to write. */
+			writel_relaxed(val, (void __iomem *)(sadi->slave_vbase + reg));
 			break;
 		}
 
@@ -277,44 +280,24 @@ static int sprd_adi_transfer_one(struct spi_controller *ctlr,
 				 struct spi_transfer *t)
 {
 	struct sprd_adi *sadi = spi_controller_get_devdata(ctlr);
-	u32 phy_reg, val;
+	u32 reg, val;
 	int ret;
 
 	if (t->rx_buf) {
-		phy_reg = *(u32 *)t->rx_buf + sadi->slave_pbase;
-
-		ret = sprd_adi_check_paddr(sadi, phy_reg);
-		if (ret)
-			return ret;
-
-		ret = sprd_adi_read(sadi, phy_reg, &val);
-		if (ret)
-			return ret;
-
+		reg = *(u32 *)t->rx_buf;
+		ret = sprd_adi_read(sadi, reg, &val);
 		*(u32 *)t->rx_buf = val;
 	} else if (t->tx_buf) {
 		u32 *p = (u32 *)t->tx_buf;
-
-		/*
-		 * Get the physical register address need to write and convert
-		 * the physical address to virtual address. Since we need
-		 * virtual register address to write.
-		 */
-		phy_reg = *p++ + sadi->slave_pbase;
-		ret = sprd_adi_check_paddr(sadi, phy_reg);
-		if (ret)
-			return ret;
-
+		reg = *p++;
 		val = *p;
-		ret = sprd_adi_write(sadi, phy_reg, val);
-		if (ret)
-			return ret;
+		ret = sprd_adi_write(sadi, reg, val);
 	} else {
 		dev_err(sadi->dev, "no buffer for transfer\n");
-		return -EINVAL;
+		ret = -EINVAL;
 	}
 
-	return 0;
+	return ret;
 }
 
 static void sprd_adi_set_wdt_rst_mode(struct sprd_adi *sadi)
@@ -323,9 +306,9 @@ static void sprd_adi_set_wdt_rst_mode(struct sprd_adi *sadi)
 	u32 val;
 
 	/* Set default watchdog reboot mode */
-	sprd_adi_read(sadi, sadi->slave_pbase + PMIC_RST_STATUS, &val);
+	sprd_adi_read(sadi, PMIC_RST_STATUS, &val);
 	val |= HWRST_STATUS_WATCHDOG;
-	sprd_adi_write(sadi, sadi->slave_pbase + PMIC_RST_STATUS, val);
+	sprd_adi_write(sadi, PMIC_RST_STATUS, val);
 #endif
 }
 
@@ -366,40 +349,40 @@ static int sprd_adi_restart_handler(struct notifier_block *this,
 		reboot_mode = HWRST_STATUS_NORMAL;
 
 	/* Record the reboot mode */
-	sprd_adi_read(sadi, sadi->slave_pbase + PMIC_RST_STATUS, &val);
+	sprd_adi_read(sadi, PMIC_RST_STATUS, &val);
 	val &= ~HWRST_STATUS_WATCHDOG;
 	val |= reboot_mode;
-	sprd_adi_write(sadi, sadi->slave_pbase + PMIC_RST_STATUS, val);
+	sprd_adi_write(sadi, PMIC_RST_STATUS, val);
 
 	/* Enable the interface clock of the watchdog */
-	sprd_adi_read(sadi, sadi->slave_pbase + PMIC_MODULE_EN, &val);
+	sprd_adi_read(sadi, PMIC_MODULE_EN, &val);
 	val |= BIT_WDG_EN;
-	sprd_adi_write(sadi, sadi->slave_pbase + PMIC_MODULE_EN, val);
+	sprd_adi_write(sadi, PMIC_MODULE_EN, val);
 
 	/* Enable the work clock of the watchdog */
-	sprd_adi_read(sadi, sadi->slave_pbase + PMIC_CLK_EN, &val);
+	sprd_adi_read(sadi, PMIC_CLK_EN, &val);
 	val |= BIT_WDG_EN;
-	sprd_adi_write(sadi, sadi->slave_pbase + PMIC_CLK_EN, val);
+	sprd_adi_write(sadi, PMIC_CLK_EN, val);
 
 	/* Unlock the watchdog */
-	sprd_adi_write(sadi, sadi->slave_pbase + REG_WDG_LOCK, WDG_UNLOCK_KEY);
+	sprd_adi_write(sadi, REG_WDG_LOCK, WDG_UNLOCK_KEY);
 
-	sprd_adi_read(sadi, sadi->slave_pbase + REG_WDG_CTRL, &val);
+	sprd_adi_read(sadi, REG_WDG_CTRL, &val);
 	val |= BIT_WDG_NEW;
-	sprd_adi_write(sadi, sadi->slave_pbase + REG_WDG_CTRL, val);
+	sprd_adi_write(sadi, REG_WDG_CTRL, val);
 
 	/* Load the watchdog timeout value, 50ms is always enough. */
-	sprd_adi_write(sadi, sadi->slave_pbase + REG_WDG_LOAD_HIGH, 0);
-	sprd_adi_write(sadi, sadi->slave_pbase + REG_WDG_LOAD_LOW,
+	sprd_adi_write(sadi, REG_WDG_LOAD_HIGH, 0);
+	sprd_adi_write(sadi, REG_WDG_LOAD_LOW,
 		       WDG_LOAD_VAL & WDG_LOAD_MASK);
 
 	/* Start the watchdog to reset system */
-	sprd_adi_read(sadi, sadi->slave_pbase + REG_WDG_CTRL, &val);
+	sprd_adi_read(sadi, REG_WDG_CTRL, &val);
 	val |= BIT_WDG_RUN | BIT_WDG_RST;
-	sprd_adi_write(sadi, sadi->slave_pbase + REG_WDG_CTRL, val);
+	sprd_adi_write(sadi, REG_WDG_CTRL, val);
 
 	/* Lock the watchdog */
-	sprd_adi_write(sadi, sadi->slave_pbase + REG_WDG_LOCK, ~WDG_UNLOCK_KEY);
+	sprd_adi_write(sadi, REG_WDG_LOCK, ~WDG_UNLOCK_KEY);
 
 	mdelay(1000);
 
-- 
2.25.1

[PATCH 2/3] spi: sprd: Make sure offset not equal to slave address size

[Chunyan Zhang]

From: Chunyan Zhang <chunyan.zhang@unisoc.com>

The slave register offset shouldn't equal to the max slave address
which ADI can support to access.

Signed-off-by: Chunyan Zhang <chunyan.zhang@unisoc.com>
---
 drivers/spi/spi-sprd-adi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/spi/spi-sprd-adi.c b/drivers/spi/spi-sprd-adi.c
index abdad1ea7b38..06af519c0b21 100644
--- a/drivers/spi/spi-sprd-adi.c
+++ b/drivers/spi/spi-sprd-adi.c
@@ -119,7 +119,7 @@ struct sprd_adi {
 
 static int sprd_adi_check_addr(struct sprd_adi *sadi, u32 reg)
 {
-	if (reg > ADI_SLAVE_ADDR_SIZE) {
+	if (reg >= ADI_SLAVE_ADDR_SIZE) {
 		dev_err(sadi->dev,
 			"slave address offset is incorrect, reg = 0x%x\n",
 			reg);
-- 
2.25.1

[PATCH 3/3] spi: sprd: fill offset only to RD_CMD register for reading from slave device

[Chunyan Zhang]

From: Chunyan Zhang <chunyan.zhang@unisoc.com>

RD_CMD can accept slave address offset only, higher bits are reserved.
Writing the whole slave address including slave base seems unnecessary.

Signed-off-by: Chunyan Zhang <chunyan.zhang@unisoc.com>
---
 drivers/spi/spi-sprd-adi.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/spi/spi-sprd-adi.c b/drivers/spi/spi-sprd-adi.c
index 06af519c0b21..07f11b17bf20 100644
--- a/drivers/spi/spi-sprd-adi.c
+++ b/drivers/spi/spi-sprd-adi.c
@@ -159,7 +159,7 @@ static int sprd_adi_read(struct sprd_adi *sadi, u32 reg, u32 *read_val)
 {
 	int read_timeout = ADI_READ_TIMEOUT;
 	unsigned long flags;
-	u32 val, rd_addr, paddr;
+	u32 val, rd_addr;
 	int ret = 0;
 
 	if (sadi->hwlock) {
@@ -177,11 +177,10 @@ static int sprd_adi_read(struct sprd_adi *sadi, u32 reg, u32 *read_val)
 		goto out;
 
 	/*
-	 * Set the physical register address need to read into RD_CMD register,
+	 * Set the slave address offset need to read into RD_CMD register,
 	 * then ADI controller will start to transfer automatically.
 	 */
-	paddr = sadi->slave_pbase + reg;
-	writel_relaxed(paddr, sadi->base + REG_ADI_RD_CMD);
+	writel_relaxed(reg, sadi->base + REG_ADI_RD_CMD);
 
 	/*
 	 * Wait read operation complete, the BIT_RD_CMD_BUSY will be set
@@ -211,9 +210,9 @@ static int sprd_adi_read(struct sprd_adi *sadi, u32 reg, u32 *read_val)
 	 */
 	rd_addr = (val & RD_ADDR_MASK) >> RD_ADDR_SHIFT;
 
-	if (rd_addr != (paddr & REG_ADDR_LOW_MASK)) {
+	if (rd_addr != (reg & REG_ADDR_LOW_MASK)) {
 		dev_err(sadi->dev, "read error, reg addr = 0x%x, val = 0x%x\n",
-			paddr, val);
+			reg, val);
 		ret = -EIO;
 		goto out;
 	}
-- 
2.25.1

Re: [PATCH 1/3] spi: sprd: Pass offset instead of physical address to adi_read/_write()

[Mark Brown]

On Tue, 24 Aug 2021 15:02:10 +0800, Chunyan Zhang wrote:
> From: Chunyan Zhang <chunyan.zhang@unisoc.com>
> 
> The register offset would be added a physical address base and then pass to
> the function sprd_adt_read()/_write() each time before calling them. So we
> can do that within these two functions instead, that would make the code
> more clear.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[1/3] spi: sprd: Pass offset instead of physical address to adi_read/_write()
      commit: 5dc349ec131c6d40aeb2545064e285f0025fbb39
[2/3] spi: sprd: Make sure offset not equal to slave address size
      commit: 2b961c51f4d35c45116b21936b563cbb78fba540
[3/3] spi: sprd: fill offset only to RD_CMD register for reading from slave device
      commit: f674aacd5005184acf3cf7b851a299573d64fdd6

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark