Straightforward patches that clean the driver. Just compiled tested.
Stands up just the patch that updates the driver to use the bitfield
access macros. The bit operations shall be identical after the patch.
Sam and Andi have some concerns on whether using the bitfield access
macros are just a matter of taste, or they are actually necessary.
I think they are necessary. Here are the concerns/discussions:
https://lore.kernel.org/linux-arm-kernel/[email protected]/
https://lore.kernel.org/linux-arm-kernel/ri7gerw4ov4jnmmkhtumhhtgfgxtr6kpsopdxjlx6fylbqznna@3qgvejyhjirw/
Cheers,
ta
v3:
- reworked the bitfied access macros patch so that the bit operations
are the same as before the patch. Fix S3C64XX_SPI_PSR_MASK value,
drop S3C64XX_SPI_CS_NSC_CNT_MASK.
- add a new patches to explicitly remove a duplicated definition and to
drop a superfluous bitwise NOT operation.
- collect R-b tags
v2:
https://lore.kernel.org/linux-arm-kernel/[email protected]/
Tudor Ambarus (17):
spi: s3c64xx: explicitly include <linux/io.h>
spi: s3c64xx: explicitly include <linux/bits.h>
spi: s3c64xx: avoid possible negative array index
spi: s3c64xx: fix typo, s/configuartion/configuration
spi: s3c64xx: sort headers alphabetically
spi: s3c64xx: remove unneeded (void *) casts in of_match_table
spi: s3c64xx: remove else after return
spi: s3c64xx: move common code outside if else
spi: s3c64xx: check return code of dmaengine_slave_config()
spi: s3c64xx: propagate the dma_submit_error() error code
spi: s3c64xx: rename prepare_dma() to s3c64xx_prepare_dma()
spi: s3c64xx: return ETIMEDOUT for wait_for_completion_timeout()
spi: s3c64xx: drop blank line between declarations
spi: s3c64xx: downgrade dev_warn to dev_dbg for optional dt props
spi: s3c64xx: remove duplicated definition
spi: s3c64xx: drop a superfluous bitwise NOT operation
spi: s3c64xx: use bitfield access macros
drivers/spi/spi-s3c64xx.c | 298 ++++++++++++++++++++------------------
1 file changed, 158 insertions(+), 140 deletions(-)
--
2.43.0.429.g432eaa2c6b-goog
The driver uses readl() but does not include <linux/io.h>.
It is good practice to directly include all headers used, it avoids
implicit dependencies and spurious breakage if someone rearranges
headers and causes the implicit include to vanish.
Include the missing header.
Fixes: 230d42d422e7 ("spi: Add s3c64xx SPI Controller driver")
Signed-off-by: Tudor Ambarus <[email protected]>
---
drivers/spi/spi-s3c64xx.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 7f7eb8f742e4..c1cbc4780a3b 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -10,6 +10,7 @@
#include <linux/clk.h>
#include <linux/dma-mapping.h>
#include <linux/dmaengine.h>
+#include <linux/io.h>
#include <linux/platform_device.h>
#include <linux/pm_runtime.h>
#include <linux/spi/spi.h>
--
2.43.0.429.g432eaa2c6b-goog
The driver uses GENMASK() but does not include <linux/bits.h>.
It is good practice to directly include all headers used, it avoids
implicit dependencies and spurious breakage if someone rearranges
headers and causes the implicit include to vanish.
Include the missing header.
Fixes: 1224e29572f6 ("spi: s3c64xx: Fix large transfers with DMA")
Signed-off-by: Tudor Ambarus <[email protected]>
---
drivers/spi/spi-s3c64xx.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index c1cbc4780a3b..2b5bb7604526 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -3,6 +3,7 @@
// Copyright (c) 2009 Samsung Electronics Co., Ltd.
// Jaswinder Singh <[email protected]>
+#include <linux/bits.h>
#include <linux/init.h>
#include <linux/module.h>
#include <linux/interrupt.h>
--
2.43.0.429.g432eaa2c6b-goog
The platform id is used as an index into the fifo_lvl_mask array.
Platforms can come with a negative device ID, PLATFORM_DEVID_NONE (-1),
thus we risked a negative array index. Catch such cases and fail to
probe.
Fixes: 2b90807549e5 ("spi: s3c64xx: add device tree support")
Reviewed-by: Sam Protsenko <[email protected]>
Signed-off-by: Tudor Ambarus <[email protected]>
---
drivers/spi/spi-s3c64xx.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 2b5bb7604526..c3176a510643 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -1189,6 +1189,9 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)
"Failed to get alias id\n");
sdd->port_id = ret;
} else {
+ if (pdev->id < 0)
+ return dev_err_probe(&pdev->dev, -EINVAL,
+ "Negative platform ID is not allowed\n");
sdd->port_id = pdev->id;
}
--
2.43.0.429.g432eaa2c6b-goog
Fix typo, s/configuartion/configuration.
Fixes: 6b8d1e4739f4 ("spi: spi-s3c64xx: Add missing entries for structs 's3c64xx_spi_dma_data' and 's3c64xx_spi_dma_data'")
Reviewed-by: Sam Protsenko <[email protected]>
Signed-off-by: Tudor Ambarus <[email protected]>
---
drivers/spi/spi-s3c64xx.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index c3176a510643..3df4906bba34 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -180,7 +180,7 @@ struct s3c64xx_spi_port_config {
* @cur_speed: Current clock speed
* @rx_dma: Local receive DMA data (e.g. chan and direction)
* @tx_dma: Local transmit DMA data (e.g. chan and direction)
- * @port_conf: Local SPI port configuartion data
+ * @port_conf: Local SPI port configuration data
* @port_id: Port identification number
*/
struct s3c64xx_spi_driver_data {
--
2.43.0.429.g432eaa2c6b-goog
of_device_id::data is an opaque pointer. No explicit cast is needed.
Remove unneeded (void *) casts in of_match_table. While here align the
compatible and data members.
Reviewed-by: Andi Shyti <[email protected]>
Signed-off-by: Tudor Ambarus <[email protected]>
---
drivers/spi/spi-s3c64xx.c | 45 +++++++++++++++++++++++----------------
1 file changed, 27 insertions(+), 18 deletions(-)
diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index ccb700312d64..9bf54c1044b3 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -1511,32 +1511,41 @@ static const struct platform_device_id s3c64xx_spi_driver_ids[] = {
};
static const struct of_device_id s3c64xx_spi_dt_match[] = {
- { .compatible = "samsung,s3c2443-spi",
- .data = (void *)&s3c2443_spi_port_config,
+ {
+ .compatible = "samsung,s3c2443-spi",
+ .data = &s3c2443_spi_port_config,
},
- { .compatible = "samsung,s3c6410-spi",
- .data = (void *)&s3c6410_spi_port_config,
+ {
+ .compatible = "samsung,s3c6410-spi",
+ .data = &s3c6410_spi_port_config,
},
- { .compatible = "samsung,s5pv210-spi",
- .data = (void *)&s5pv210_spi_port_config,
+ {
+ .compatible = "samsung,s5pv210-spi",
+ .data = &s5pv210_spi_port_config,
},
- { .compatible = "samsung,exynos4210-spi",
- .data = (void *)&exynos4_spi_port_config,
+ {
+ .compatible = "samsung,exynos4210-spi",
+ .data = &exynos4_spi_port_config,
},
- { .compatible = "samsung,exynos7-spi",
- .data = (void *)&exynos7_spi_port_config,
+ {
+ .compatible = "samsung,exynos7-spi",
+ .data = &exynos7_spi_port_config,
},
- { .compatible = "samsung,exynos5433-spi",
- .data = (void *)&exynos5433_spi_port_config,
+ {
+ .compatible = "samsung,exynos5433-spi",
+ .data = &exynos5433_spi_port_config,
},
- { .compatible = "samsung,exynos850-spi",
- .data = (void *)&exynos850_spi_port_config,
+ {
+ .compatible = "samsung,exynos850-spi",
+ .data = &exynos850_spi_port_config,
},
- { .compatible = "samsung,exynosautov9-spi",
- .data = (void *)&exynosautov9_spi_port_config,
+ {
+ .compatible = "samsung,exynosautov9-spi",
+ .data = &exynosautov9_spi_port_config,
},
- { .compatible = "tesla,fsd-spi",
- .data = (void *)&fsd_spi_port_config,
+ {
+ .compatible = "tesla,fsd-spi",
+ .data = &fsd_spi_port_config,
},
{ },
};
--
2.43.0.429.g432eaa2c6b-goog
Sorting headers alphabetically helps locating duplicates,
and makes it easier to figure out where to insert new headers.
Reviewed-by: Andi Shyti <[email protected]>
Signed-off-by: Tudor Ambarus <[email protected]>
---
drivers/spi/spi-s3c64xx.c | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 3df4906bba34..ccb700312d64 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -4,20 +4,19 @@
// Jaswinder Singh <[email protected]>
#include <linux/bits.h>
-#include <linux/init.h>
-#include <linux/module.h>
-#include <linux/interrupt.h>
-#include <linux/delay.h>
#include <linux/clk.h>
+#include <linux/delay.h>
#include <linux/dma-mapping.h>
#include <linux/dmaengine.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_data/spi-s3c64xx.h>
#include <linux/platform_device.h>
#include <linux/pm_runtime.h>
#include <linux/spi/spi.h>
-#include <linux/of.h>
-
-#include <linux/platform_data/spi-s3c64xx.h>
#define MAX_SPI_PORTS 12
#define S3C64XX_SPI_QUIRK_CS_AUTO (1 << 1)
--
2.43.0.429.g432eaa2c6b-goog
Else case is not needed after a return, remove it.
Reviewed-by: Andi Shyti <[email protected]>
Reviewed-by: Sam Protsenko <[email protected]>
Signed-off-by: Tudor Ambarus <[email protected]>
---
drivers/spi/spi-s3c64xx.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 9bf54c1044b3..bd2ac875af59 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -407,12 +407,10 @@ static bool s3c64xx_spi_can_dma(struct spi_controller *host,
{
struct s3c64xx_spi_driver_data *sdd = spi_controller_get_devdata(host);
- if (sdd->rx_dma.ch && sdd->tx_dma.ch) {
+ if (sdd->rx_dma.ch && sdd->tx_dma.ch)
return xfer->len > FIFO_DEPTH(sdd);
- } else {
- return false;
- }
+ return false;
}
static int s3c64xx_enable_datapath(struct s3c64xx_spi_driver_data *sdd,
--
2.43.0.429.g432eaa2c6b-goog
Check the return code of dmaengine_slave_config().
Reviewed-by: Sam Protsenko <[email protected]>
Signed-off-by: Tudor Ambarus <[email protected]>
---
drivers/spi/spi-s3c64xx.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index bbbc4795bcbf..6268790bbcff 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -302,7 +302,9 @@ static int prepare_dma(struct s3c64xx_spi_dma_data *dma,
config.dst_maxburst = 1;
}
config.direction = dma->direction;
- dmaengine_slave_config(dma->ch, &config);
+ ret = dmaengine_slave_config(dma->ch, &config);
+ if (ret)
+ return ret;
desc = dmaengine_prep_slave_sg(dma->ch, sgt->sgl, sgt->nents,
dma->direction, DMA_PREP_INTERRUPT);
--
2.43.0.429.g432eaa2c6b-goog
Move common code outside if else to avoid code duplication.
Reviewed-by: Sam Protsenko <[email protected]>
Signed-off-by: Tudor Ambarus <[email protected]>
---
drivers/spi/spi-s3c64xx.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index bd2ac875af59..bbbc4795bcbf 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -291,20 +291,18 @@ static int prepare_dma(struct s3c64xx_spi_dma_data *dma,
if (dma->direction == DMA_DEV_TO_MEM) {
sdd = container_of((void *)dma,
struct s3c64xx_spi_driver_data, rx_dma);
- config.direction = dma->direction;
config.src_addr = sdd->sfr_start + S3C64XX_SPI_RX_DATA;
config.src_addr_width = sdd->cur_bpw / 8;
config.src_maxburst = 1;
- dmaengine_slave_config(dma->ch, &config);
} else {
sdd = container_of((void *)dma,
struct s3c64xx_spi_driver_data, tx_dma);
- config.direction = dma->direction;
config.dst_addr = sdd->sfr_start + S3C64XX_SPI_TX_DATA;
config.dst_addr_width = sdd->cur_bpw / 8;
config.dst_maxburst = 1;
- dmaengine_slave_config(dma->ch, &config);
}
+ config.direction = dma->direction;
+ dmaengine_slave_config(dma->ch, &config);
desc = dmaengine_prep_slave_sg(dma->ch, sgt->sgl, sgt->nents,
dma->direction, DMA_PREP_INTERRUPT);
--
2.43.0.429.g432eaa2c6b-goog
Don't monopolize the name. Prepend the driver prefix to the function
name.
Reviewed-by: Sam Protsenko <[email protected]>
Signed-off-by: Tudor Ambarus <[email protected]>
---
drivers/spi/spi-s3c64xx.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 64daf944b245..76fa378ab5ab 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -278,8 +278,8 @@ static void s3c64xx_spi_dmacb(void *data)
spin_unlock_irqrestore(&sdd->lock, flags);
}
-static int prepare_dma(struct s3c64xx_spi_dma_data *dma,
- struct sg_table *sgt)
+static int s3c64xx_prepare_dma(struct s3c64xx_spi_dma_data *dma,
+ struct sg_table *sgt)
{
struct s3c64xx_spi_driver_data *sdd;
struct dma_slave_config config;
@@ -444,7 +444,7 @@ static int s3c64xx_enable_datapath(struct s3c64xx_spi_driver_data *sdd,
chcfg |= S3C64XX_SPI_CH_TXCH_ON;
if (dma_mode) {
modecfg |= S3C64XX_SPI_MODE_TXDMA_ON;
- ret = prepare_dma(&sdd->tx_dma, &xfer->tx_sg);
+ ret = s3c64xx_prepare_dma(&sdd->tx_dma, &xfer->tx_sg);
} else {
switch (sdd->cur_bpw) {
case 32:
@@ -476,7 +476,7 @@ static int s3c64xx_enable_datapath(struct s3c64xx_spi_driver_data *sdd,
writel(((xfer->len * 8 / sdd->cur_bpw) & 0xffff)
| S3C64XX_SPI_PACKET_CNT_EN,
regs + S3C64XX_SPI_PACKET_CNT);
- ret = prepare_dma(&sdd->rx_dma, &xfer->rx_sg);
+ ret = s3c64xx_prepare_dma(&sdd->rx_dma, &xfer->rx_sg);
}
}
--
2.43.0.429.g432eaa2c6b-goog
DMA submit should just add the dma descriptor to a queue, without firing
it. EIO is misleading and hides what happens in DMA. Propagate the
dma_submit_error() error code, don't overwrite it.
Signed-off-by: Tudor Ambarus <[email protected]>
---
drivers/spi/spi-s3c64xx.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 6268790bbcff..64daf944b245 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -321,7 +321,7 @@ static int prepare_dma(struct s3c64xx_spi_dma_data *dma,
ret = dma_submit_error(dma->cookie);
if (ret) {
dev_err(&sdd->pdev->dev, "DMA submission failed");
- return -EIO;
+ return ret;
}
dma_async_issue_pending(dma->ch);
--
2.43.0.429.g432eaa2c6b-goog
Drop the blank line and move the logical operation in the body of the
function rather than in initialization list.
Reviewed-by: Sam Protsenko <[email protected]>
Signed-off-by: Tudor Ambarus <[email protected]>
---
drivers/spi/spi-s3c64xx.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 2f2c4ad35df4..08ba14adb428 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -1320,8 +1320,9 @@ static int s3c64xx_spi_suspend(struct device *dev)
{
struct spi_controller *host = dev_get_drvdata(dev);
struct s3c64xx_spi_driver_data *sdd = spi_controller_get_devdata(host);
+ int ret;
- int ret = spi_controller_suspend(host);
+ ret = spi_controller_suspend(host);
if (ret)
return ret;
--
2.43.0.429.g432eaa2c6b-goog
ETIMEDOUT is more specific than EIO, use it for
wait_for_completion_timeout().
Signed-off-by: Tudor Ambarus <[email protected]>
---
drivers/spi/spi-s3c64xx.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 76fa378ab5ab..2f2c4ad35df4 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -526,7 +526,7 @@ static int s3c64xx_wait_for_dma(struct s3c64xx_spi_driver_data *sdd,
/*
* If the previous xfer was completed within timeout, then
- * proceed further else return -EIO.
+ * proceed further else return -ETIMEDOUT.
* DmaTx returns after simply writing data in the FIFO,
* w/o waiting for real transmission on the bus to finish.
* DmaRx returns only after Dma read data from FIFO which
@@ -547,7 +547,7 @@ static int s3c64xx_wait_for_dma(struct s3c64xx_spi_driver_data *sdd,
/* If timed out while checking rx/tx status return error */
if (!val)
- return -EIO;
+ return -ETIMEDOUT;
return 0;
}
@@ -577,7 +577,7 @@ static int s3c64xx_wait_for_pio(struct s3c64xx_spi_driver_data *sdd,
if (use_irq) {
val = msecs_to_jiffies(ms);
if (!wait_for_completion_timeout(&sdd->xfer_completion, val))
- return -EIO;
+ return -ETIMEDOUT;
}
val = msecs_to_loops(ms);
--
2.43.0.429.g432eaa2c6b-goog
"samsung,spi-src-clk" and "num-cs" are optional dt properties. Downgrade
the message from warning to debug message.
Reviewed-by: Sam Protsenko <[email protected]>
Signed-off-by: Tudor Ambarus <[email protected]>
---
drivers/spi/spi-s3c64xx.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 08ba14adb428..dc779d5305a5 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -1109,14 +1109,14 @@ static struct s3c64xx_spi_info *s3c64xx_spi_parse_dt(struct device *dev)
return ERR_PTR(-ENOMEM);
if (of_property_read_u32(dev->of_node, "samsung,spi-src-clk", &temp)) {
- dev_warn(dev, "spi bus clock parent not specified, using clock at index 0 as parent\n");
+ dev_dbg(dev, "spi bus clock parent not specified, using clock at index 0 as parent\n");
sci->src_clk_nr = 0;
} else {
sci->src_clk_nr = temp;
}
if (of_property_read_u32(dev->of_node, "num-cs", &temp)) {
- dev_warn(dev, "number of chip select lines not specified, assuming 1 chip select line\n");
+ dev_dbg(dev, "number of chip select lines not specified, assuming 1 chip select line\n");
sci->num_cs = 1;
} else {
sci->num_cs = temp;
--
2.43.0.429.g432eaa2c6b-goog
S3C64XX_SPI_TRAILCNT brings no benefit in terms of name over
S3C64XX_SPI_MAX_TRAILCNT. Remove the duplicated definition.
Signed-off-by: Tudor Ambarus <[email protected]>
---
drivers/spi/spi-s3c64xx.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index dc779d5305a5..e9344fe71e56 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -115,8 +115,6 @@
#define S3C64XX_SPI_MAX_TRAILCNT 0x3ff
#define S3C64XX_SPI_TRAILCNT_OFF 19
-#define S3C64XX_SPI_TRAILCNT S3C64XX_SPI_MAX_TRAILCNT
-
#define S3C64XX_SPI_POLLING_SIZE 32
#define msecs_to_loops(t) (loops_per_jiffy / 1000 * HZ * t)
@@ -1092,7 +1090,7 @@ static void s3c64xx_spi_hwinit(struct s3c64xx_spi_driver_data *sdd)
val = readl(regs + S3C64XX_SPI_MODE_CFG);
val &= ~S3C64XX_SPI_MODE_4BURST;
val &= ~(S3C64XX_SPI_MAX_TRAILCNT << S3C64XX_SPI_TRAILCNT_OFF);
- val |= (S3C64XX_SPI_TRAILCNT << S3C64XX_SPI_TRAILCNT_OFF);
+ val |= (S3C64XX_SPI_MAX_TRAILCNT << S3C64XX_SPI_TRAILCNT_OFF);
writel(val, regs + S3C64XX_SPI_MODE_CFG);
s3c64xx_flush_fifo(sdd);
--
2.43.0.429.g432eaa2c6b-goog
val &= ~mask;
val |= mask;
is equivalent to:
val |= mask;
Drop the superfluous bitwise NOT operation.
Signed-off-by: Tudor Ambarus <[email protected]>
---
drivers/spi/spi-s3c64xx.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index e9344fe71e56..43b888c8812e 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -1089,7 +1089,6 @@ static void s3c64xx_spi_hwinit(struct s3c64xx_spi_driver_data *sdd)
val = readl(regs + S3C64XX_SPI_MODE_CFG);
val &= ~S3C64XX_SPI_MODE_4BURST;
- val &= ~(S3C64XX_SPI_MAX_TRAILCNT << S3C64XX_SPI_TRAILCNT_OFF);
val |= (S3C64XX_SPI_MAX_TRAILCNT << S3C64XX_SPI_TRAILCNT_OFF);
writel(val, regs + S3C64XX_SPI_MODE_CFG);
--
2.43.0.429.g432eaa2c6b-goog
Use the bitfield access macros in order to clean and to make the driver
easier to read. Introduce S3C64XX_SPI_MAX_TRAILCNT_MASK to replace value
and offset equivalents (S3C64XX_SPI_MAX_TRAILCNT,
S3C64XX_SPI_TRAILCNT_OFF). While touching the register definitions, align
their values to the same offset.
No functional change intended, the bit operations shall be equivalent.
Signed-off-by: Tudor Ambarus <[email protected]>
---
drivers/spi/spi-s3c64xx.c | 193 ++++++++++++++++++++------------------
1 file changed, 101 insertions(+), 92 deletions(-)
diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 43b888c8812e..7f052d6cd2ba 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -4,6 +4,7 @@
// Jaswinder Singh <[email protected]>
#include <linux/bits.h>
+#include <linux/bitfield.h>
#include <linux/clk.h>
#include <linux/delay.h>
#include <linux/dma-mapping.h>
@@ -18,91 +19,96 @@
#include <linux/pm_runtime.h>
#include <linux/spi/spi.h>
-#define MAX_SPI_PORTS 12
-#define S3C64XX_SPI_QUIRK_CS_AUTO (1 << 1)
-#define AUTOSUSPEND_TIMEOUT 2000
+#define MAX_SPI_PORTS 12
+#define S3C64XX_SPI_QUIRK_CS_AUTO BIT(1)
+#define AUTOSUSPEND_TIMEOUT 2000
/* Registers and bit-fields */
-#define S3C64XX_SPI_CH_CFG 0x00
-#define S3C64XX_SPI_CLK_CFG 0x04
-#define S3C64XX_SPI_MODE_CFG 0x08
-#define S3C64XX_SPI_CS_REG 0x0C
-#define S3C64XX_SPI_INT_EN 0x10
-#define S3C64XX_SPI_STATUS 0x14
-#define S3C64XX_SPI_TX_DATA 0x18
-#define S3C64XX_SPI_RX_DATA 0x1C
-#define S3C64XX_SPI_PACKET_CNT 0x20
-#define S3C64XX_SPI_PENDING_CLR 0x24
-#define S3C64XX_SPI_SWAP_CFG 0x28
-#define S3C64XX_SPI_FB_CLK 0x2C
-
-#define S3C64XX_SPI_CH_HS_EN (1<<6) /* High Speed Enable */
-#define S3C64XX_SPI_CH_SW_RST (1<<5)
-#define S3C64XX_SPI_CH_SLAVE (1<<4)
-#define S3C64XX_SPI_CPOL_L (1<<3)
-#define S3C64XX_SPI_CPHA_B (1<<2)
-#define S3C64XX_SPI_CH_RXCH_ON (1<<1)
-#define S3C64XX_SPI_CH_TXCH_ON (1<<0)
-
-#define S3C64XX_SPI_CLKSEL_SRCMSK (3<<9)
-#define S3C64XX_SPI_CLKSEL_SRCSHFT 9
-#define S3C64XX_SPI_ENCLK_ENABLE (1<<8)
-#define S3C64XX_SPI_PSR_MASK 0xff
-
-#define S3C64XX_SPI_MODE_CH_TSZ_BYTE (0<<29)
-#define S3C64XX_SPI_MODE_CH_TSZ_HALFWORD (1<<29)
-#define S3C64XX_SPI_MODE_CH_TSZ_WORD (2<<29)
-#define S3C64XX_SPI_MODE_CH_TSZ_MASK (3<<29)
-#define S3C64XX_SPI_MODE_BUS_TSZ_BYTE (0<<17)
-#define S3C64XX_SPI_MODE_BUS_TSZ_HALFWORD (1<<17)
-#define S3C64XX_SPI_MODE_BUS_TSZ_WORD (2<<17)
-#define S3C64XX_SPI_MODE_BUS_TSZ_MASK (3<<17)
+#define S3C64XX_SPI_CH_CFG 0x00
+#define S3C64XX_SPI_CLK_CFG 0x04
+#define S3C64XX_SPI_MODE_CFG 0x08
+#define S3C64XX_SPI_CS_REG 0x0C
+#define S3C64XX_SPI_INT_EN 0x10
+#define S3C64XX_SPI_STATUS 0x14
+#define S3C64XX_SPI_TX_DATA 0x18
+#define S3C64XX_SPI_RX_DATA 0x1C
+#define S3C64XX_SPI_PACKET_CNT 0x20
+#define S3C64XX_SPI_PENDING_CLR 0x24
+#define S3C64XX_SPI_SWAP_CFG 0x28
+#define S3C64XX_SPI_FB_CLK 0x2C
+
+#define S3C64XX_SPI_CH_HS_EN BIT(6) /* High Speed Enable */
+#define S3C64XX_SPI_CH_SW_RST BIT(5)
+#define S3C64XX_SPI_CH_SLAVE BIT(4)
+#define S3C64XX_SPI_CPOL_L BIT(3)
+#define S3C64XX_SPI_CPHA_B BIT(2)
+#define S3C64XX_SPI_CH_RXCH_ON BIT(1)
+#define S3C64XX_SPI_CH_TXCH_ON BIT(0)
+
+#define S3C64XX_SPI_CLKSEL_SRCMSK GENMASK(10, 9)
+#define S3C64XX_SPI_ENCLK_ENABLE BIT(8)
+#define S3C64XX_SPI_PSR_MASK GENMASK(7, 0)
+
+#define S3C64XX_SPI_MODE_CH_TSZ_MASK GENMASK(30, 29)
+#define S3C64XX_SPI_MODE_CH_TSZ_BYTE 0
+#define S3C64XX_SPI_MODE_CH_TSZ_HALFWORD 1
+#define S3C64XX_SPI_MODE_CH_TSZ_WORD 2
+#define S3C64XX_SPI_MAX_TRAILCNT_MASK GENMASK(28, 19)
+#define S3C64XX_SPI_MODE_BUS_TSZ_MASK GENMASK(18, 17)
+#define S3C64XX_SPI_MODE_BUS_TSZ_BYTE 0
+#define S3C64XX_SPI_MODE_BUS_TSZ_HALFWORD 1
+#define S3C64XX_SPI_MODE_BUS_TSZ_WORD 2
#define S3C64XX_SPI_MODE_RX_RDY_LVL GENMASK(16, 11)
-#define S3C64XX_SPI_MODE_RX_RDY_LVL_SHIFT 11
-#define S3C64XX_SPI_MODE_SELF_LOOPBACK (1<<3)
-#define S3C64XX_SPI_MODE_RXDMA_ON (1<<2)
-#define S3C64XX_SPI_MODE_TXDMA_ON (1<<1)
-#define S3C64XX_SPI_MODE_4BURST (1<<0)
-
-#define S3C64XX_SPI_CS_NSC_CNT_2 (2<<4)
-#define S3C64XX_SPI_CS_AUTO (1<<1)
-#define S3C64XX_SPI_CS_SIG_INACT (1<<0)
-
-#define S3C64XX_SPI_INT_TRAILING_EN (1<<6)
-#define S3C64XX_SPI_INT_RX_OVERRUN_EN (1<<5)
-#define S3C64XX_SPI_INT_RX_UNDERRUN_EN (1<<4)
-#define S3C64XX_SPI_INT_TX_OVERRUN_EN (1<<3)
-#define S3C64XX_SPI_INT_TX_UNDERRUN_EN (1<<2)
-#define S3C64XX_SPI_INT_RX_FIFORDY_EN (1<<1)
-#define S3C64XX_SPI_INT_TX_FIFORDY_EN (1<<0)
-
-#define S3C64XX_SPI_ST_RX_OVERRUN_ERR (1<<5)
-#define S3C64XX_SPI_ST_RX_UNDERRUN_ERR (1<<4)
-#define S3C64XX_SPI_ST_TX_OVERRUN_ERR (1<<3)
-#define S3C64XX_SPI_ST_TX_UNDERRUN_ERR (1<<2)
-#define S3C64XX_SPI_ST_RX_FIFORDY (1<<1)
-#define S3C64XX_SPI_ST_TX_FIFORDY (1<<0)
-
-#define S3C64XX_SPI_PACKET_CNT_EN (1<<16)
+#define S3C64XX_SPI_MODE_SELF_LOOPBACK BIT(3)
+#define S3C64XX_SPI_MODE_RXDMA_ON BIT(2)
+#define S3C64XX_SPI_MODE_TXDMA_ON BIT(1)
+#define S3C64XX_SPI_MODE_4BURST BIT(0)
+
+/*
+ * S3C64XX_SPI_CS_NSC_CNT_2 is a value into the NCS_TIME_COUNT field. In newer
+ * datasheets this field is defined as GENMASK(9, 4). We don't know if this mask
+ * applies to all the versions of the IP, thus we can't yet define
+ * S3C64XX_SPI_CS_NSC_CNT_2 as a value and the register field as a mask.
+ */
+#define S3C64XX_SPI_CS_NSC_CNT_2 (2 << 4)
+#define S3C64XX_SPI_CS_AUTO BIT(1)
+#define S3C64XX_SPI_CS_SIG_INACT BIT(0)
+
+#define S3C64XX_SPI_INT_TRAILING_EN BIT(6)
+#define S3C64XX_SPI_INT_RX_OVERRUN_EN BIT(5)
+#define S3C64XX_SPI_INT_RX_UNDERRUN_EN BIT(4)
+#define S3C64XX_SPI_INT_TX_OVERRUN_EN BIT(3)
+#define S3C64XX_SPI_INT_TX_UNDERRUN_EN BIT(2)
+#define S3C64XX_SPI_INT_RX_FIFORDY_EN BIT(1)
+#define S3C64XX_SPI_INT_TX_FIFORDY_EN BIT(0)
+
+#define S3C64XX_SPI_ST_RX_OVERRUN_ERR BIT(5)
+#define S3C64XX_SPI_ST_RX_UNDERRUN_ERR BIT(4)
+#define S3C64XX_SPI_ST_TX_OVERRUN_ERR BIT(3)
+#define S3C64XX_SPI_ST_TX_UNDERRUN_ERR BIT(2)
+#define S3C64XX_SPI_ST_RX_FIFORDY BIT(1)
+#define S3C64XX_SPI_ST_TX_FIFORDY BIT(0)
+
+#define S3C64XX_SPI_PACKET_CNT_EN BIT(16)
#define S3C64XX_SPI_PACKET_CNT_MASK GENMASK(15, 0)
-#define S3C64XX_SPI_PND_TX_UNDERRUN_CLR (1<<4)
-#define S3C64XX_SPI_PND_TX_OVERRUN_CLR (1<<3)
-#define S3C64XX_SPI_PND_RX_UNDERRUN_CLR (1<<2)
-#define S3C64XX_SPI_PND_RX_OVERRUN_CLR (1<<1)
-#define S3C64XX_SPI_PND_TRAILING_CLR (1<<0)
+#define S3C64XX_SPI_PND_TX_UNDERRUN_CLR BIT(4)
+#define S3C64XX_SPI_PND_TX_OVERRUN_CLR BIT(3)
+#define S3C64XX_SPI_PND_RX_UNDERRUN_CLR BIT(2)
+#define S3C64XX_SPI_PND_RX_OVERRUN_CLR BIT(1)
+#define S3C64XX_SPI_PND_TRAILING_CLR BIT(0)
-#define S3C64XX_SPI_SWAP_RX_HALF_WORD (1<<7)
-#define S3C64XX_SPI_SWAP_RX_BYTE (1<<6)
-#define S3C64XX_SPI_SWAP_RX_BIT (1<<5)
-#define S3C64XX_SPI_SWAP_RX_EN (1<<4)
-#define S3C64XX_SPI_SWAP_TX_HALF_WORD (1<<3)
-#define S3C64XX_SPI_SWAP_TX_BYTE (1<<2)
-#define S3C64XX_SPI_SWAP_TX_BIT (1<<1)
-#define S3C64XX_SPI_SWAP_TX_EN (1<<0)
+#define S3C64XX_SPI_SWAP_RX_HALF_WORD BIT(7)
+#define S3C64XX_SPI_SWAP_RX_BYTE BIT(6)
+#define S3C64XX_SPI_SWAP_RX_BIT BIT(5)
+#define S3C64XX_SPI_SWAP_RX_EN BIT(4)
+#define S3C64XX_SPI_SWAP_TX_HALF_WORD BIT(3)
+#define S3C64XX_SPI_SWAP_TX_BYTE BIT(2)
+#define S3C64XX_SPI_SWAP_TX_BIT BIT(1)
+#define S3C64XX_SPI_SWAP_TX_EN BIT(0)
-#define S3C64XX_SPI_FBCLK_MSK (3<<0)
+#define S3C64XX_SPI_FBCLK_MASK GENMASK(1, 0)
#define FIFO_LVL_MASK(i) ((i)->port_conf->fifo_lvl_mask[i->port_id])
#define S3C64XX_SPI_ST_TX_DONE(v, i) (((v) & \
@@ -112,16 +118,13 @@
FIFO_LVL_MASK(i))
#define FIFO_DEPTH(i) ((FIFO_LVL_MASK(i) >> 1) + 1)
-#define S3C64XX_SPI_MAX_TRAILCNT 0x3ff
-#define S3C64XX_SPI_TRAILCNT_OFF 19
-
#define S3C64XX_SPI_POLLING_SIZE 32
#define msecs_to_loops(t) (loops_per_jiffy / 1000 * HZ * t)
#define is_polling(x) (x->cntrlr_info->polling)
-#define RXBUSY (1<<2)
-#define TXBUSY (1<<3)
+#define RXBUSY BIT(2)
+#define TXBUSY BIT(3)
struct s3c64xx_spi_dma_data {
struct dma_chan *ch;
@@ -664,16 +667,22 @@ static int s3c64xx_spi_config(struct s3c64xx_spi_driver_data *sdd)
switch (sdd->cur_bpw) {
case 32:
- val |= S3C64XX_SPI_MODE_BUS_TSZ_WORD;
- val |= S3C64XX_SPI_MODE_CH_TSZ_WORD;
+ val |= FIELD_PREP(S3C64XX_SPI_MODE_BUS_TSZ_MASK,
+ S3C64XX_SPI_MODE_BUS_TSZ_WORD) |
+ FIELD_PREP(S3C64XX_SPI_MODE_CH_TSZ_MASK,
+ S3C64XX_SPI_MODE_CH_TSZ_WORD);
break;
case 16:
- val |= S3C64XX_SPI_MODE_BUS_TSZ_HALFWORD;
- val |= S3C64XX_SPI_MODE_CH_TSZ_HALFWORD;
+ val |= FIELD_PREP(S3C64XX_SPI_MODE_BUS_TSZ_MASK,
+ S3C64XX_SPI_MODE_BUS_TSZ_HALFWORD) |
+ FIELD_PREP(S3C64XX_SPI_MODE_CH_TSZ_MASK,
+ S3C64XX_SPI_MODE_CH_TSZ_HALFWORD);
break;
default:
- val |= S3C64XX_SPI_MODE_BUS_TSZ_BYTE;
- val |= S3C64XX_SPI_MODE_CH_TSZ_BYTE;
+ val |= FIELD_PREP(S3C64XX_SPI_MODE_BUS_TSZ_MASK,
+ S3C64XX_SPI_MODE_BUS_TSZ_BYTE) |
+ FIELD_PREP(S3C64XX_SPI_MODE_CH_TSZ_MASK,
+ S3C64XX_SPI_MODE_CH_TSZ_BYTE);
break;
}
@@ -799,7 +808,7 @@ static int s3c64xx_spi_transfer_one(struct spi_controller *host,
val = readl(sdd->regs + S3C64XX_SPI_MODE_CFG);
val &= ~S3C64XX_SPI_MODE_RX_RDY_LVL;
- val |= (rdy_lv << S3C64XX_SPI_MODE_RX_RDY_LVL_SHIFT);
+ val |= FIELD_PREP(S3C64XX_SPI_MODE_RX_RDY_LVL, rdy_lv);
writel(val, sdd->regs + S3C64XX_SPI_MODE_CFG);
/* Enable FIFO_RDY_EN IRQ */
@@ -1072,8 +1081,8 @@ static void s3c64xx_spi_hwinit(struct s3c64xx_spi_driver_data *sdd)
writel(0, regs + S3C64XX_SPI_INT_EN);
if (!sdd->port_conf->clk_from_cmu)
- writel(sci->src_clk_nr << S3C64XX_SPI_CLKSEL_SRCSHFT,
- regs + S3C64XX_SPI_CLK_CFG);
+ writel(FIELD_PREP(S3C64XX_SPI_CLKSEL_SRCMSK, sci->src_clk_nr),
+ regs + S3C64XX_SPI_CLK_CFG);
writel(0, regs + S3C64XX_SPI_MODE_CFG);
writel(0, regs + S3C64XX_SPI_PACKET_CNT);
@@ -1089,7 +1098,7 @@ static void s3c64xx_spi_hwinit(struct s3c64xx_spi_driver_data *sdd)
val = readl(regs + S3C64XX_SPI_MODE_CFG);
val &= ~S3C64XX_SPI_MODE_4BURST;
- val |= (S3C64XX_SPI_MAX_TRAILCNT << S3C64XX_SPI_TRAILCNT_OFF);
+ val |= S3C64XX_SPI_MAX_TRAILCNT_MASK;
writel(val, regs + S3C64XX_SPI_MODE_CFG);
s3c64xx_flush_fifo(sdd);
--
2.43.0.429.g432eaa2c6b-goog
On Fri, Jan 26, 2024 at 11:16 AM Tudor Ambarus <[email protected]> wrote:
>
> S3C64XX_SPI_TRAILCNT brings no benefit in terms of name over
> S3C64XX_SPI_MAX_TRAILCNT. Remove the duplicated definition.
>
> Signed-off-by: Tudor Ambarus <[email protected]>
> ---
Reviewed-by: Sam Protsenko <[email protected]>
> drivers/spi/spi-s3c64xx.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
> index dc779d5305a5..e9344fe71e56 100644
> --- a/drivers/spi/spi-s3c64xx.c
> +++ b/drivers/spi/spi-s3c64xx.c
> @@ -115,8 +115,6 @@
> #define S3C64XX_SPI_MAX_TRAILCNT 0x3ff
> #define S3C64XX_SPI_TRAILCNT_OFF 19
>
> -#define S3C64XX_SPI_TRAILCNT S3C64XX_SPI_MAX_TRAILCNT
> -
> #define S3C64XX_SPI_POLLING_SIZE 32
>
> #define msecs_to_loops(t) (loops_per_jiffy / 1000 * HZ * t)
> @@ -1092,7 +1090,7 @@ static void s3c64xx_spi_hwinit(struct s3c64xx_spi_driver_data *sdd)
> val = readl(regs + S3C64XX_SPI_MODE_CFG);
> val &= ~S3C64XX_SPI_MODE_4BURST;
> val &= ~(S3C64XX_SPI_MAX_TRAILCNT << S3C64XX_SPI_TRAILCNT_OFF);
> - val |= (S3C64XX_SPI_TRAILCNT << S3C64XX_SPI_TRAILCNT_OFF);
> + val |= (S3C64XX_SPI_MAX_TRAILCNT << S3C64XX_SPI_TRAILCNT_OFF);
> writel(val, regs + S3C64XX_SPI_MODE_CFG);
>
> s3c64xx_flush_fifo(sdd);
> --
> 2.43.0.429.g432eaa2c6b-goog
>
On Fri, Jan 26, 2024 at 11:16 AM Tudor Ambarus <[email protected]> wrote:
>
> val &= ~mask;
> val |= mask;
>
> is equivalent to:
> val |= mask;
>
> Drop the superfluous bitwise NOT operation.
>
> Signed-off-by: Tudor Ambarus <[email protected]>
> ---
That's much more clear. Also I'm pretty sure if you compare .o file
before and after the patch, they would be identical -- another way to
argue the patch has no functional change.
Reviewed-by: Sam Protsenko <[email protected]>
> drivers/spi/spi-s3c64xx.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
> index e9344fe71e56..43b888c8812e 100644
> --- a/drivers/spi/spi-s3c64xx.c
> +++ b/drivers/spi/spi-s3c64xx.c
> @@ -1089,7 +1089,6 @@ static void s3c64xx_spi_hwinit(struct s3c64xx_spi_driver_data *sdd)
>
> val = readl(regs + S3C64XX_SPI_MODE_CFG);
> val &= ~S3C64XX_SPI_MODE_4BURST;
> - val &= ~(S3C64XX_SPI_MAX_TRAILCNT << S3C64XX_SPI_TRAILCNT_OFF);
> val |= (S3C64XX_SPI_MAX_TRAILCNT << S3C64XX_SPI_TRAILCNT_OFF);
> writel(val, regs + S3C64XX_SPI_MODE_CFG);
>
> --
> 2.43.0.429.g432eaa2c6b-goog
>
On Fri, Jan 26, 2024 at 11:16 AM Tudor Ambarus <[email protected]> wrote:
>
> Use the bitfield access macros in order to clean and to make the driver
> easier to read. Introduce S3C64XX_SPI_MAX_TRAILCNT_MASK to replace value
> and offset equivalents (S3C64XX_SPI_MAX_TRAILCNT,
> S3C64XX_SPI_TRAILCNT_OFF). While touching the register definitions, align
> their values to the same offset.
>
> No functional change intended, the bit operations shall be equivalent.
>
> Signed-off-by: Tudor Ambarus <[email protected]>
> ---
> drivers/spi/spi-s3c64xx.c | 193 ++++++++++++++++++++------------------
> 1 file changed, 101 insertions(+), 92 deletions(-)
>
> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
> index 43b888c8812e..7f052d6cd2ba 100644
> --- a/drivers/spi/spi-s3c64xx.c
> +++ b/drivers/spi/spi-s3c64xx.c
> @@ -4,6 +4,7 @@
> // Jaswinder Singh <[email protected]>
>
> #include <linux/bits.h>
> +#include <linux/bitfield.h>
> #include <linux/clk.h>
> #include <linux/delay.h>
> #include <linux/dma-mapping.h>
> @@ -18,91 +19,96 @@
> #include <linux/pm_runtime.h>
> #include <linux/spi/spi.h>
>
> -#define MAX_SPI_PORTS 12
> -#define S3C64XX_SPI_QUIRK_CS_AUTO (1 << 1)
> -#define AUTOSUSPEND_TIMEOUT 2000
> +#define MAX_SPI_PORTS 12
> +#define S3C64XX_SPI_QUIRK_CS_AUTO BIT(1)
> +#define AUTOSUSPEND_TIMEOUT 2000
>
> /* Registers and bit-fields */
>
> -#define S3C64XX_SPI_CH_CFG 0x00
> -#define S3C64XX_SPI_CLK_CFG 0x04
> -#define S3C64XX_SPI_MODE_CFG 0x08
> -#define S3C64XX_SPI_CS_REG 0x0C
> -#define S3C64XX_SPI_INT_EN 0x10
> -#define S3C64XX_SPI_STATUS 0x14
> -#define S3C64XX_SPI_TX_DATA 0x18
> -#define S3C64XX_SPI_RX_DATA 0x1C
> -#define S3C64XX_SPI_PACKET_CNT 0x20
> -#define S3C64XX_SPI_PENDING_CLR 0x24
> -#define S3C64XX_SPI_SWAP_CFG 0x28
> -#define S3C64XX_SPI_FB_CLK 0x2C
> -
> -#define S3C64XX_SPI_CH_HS_EN (1<<6) /* High Speed Enable */
> -#define S3C64XX_SPI_CH_SW_RST (1<<5)
> -#define S3C64XX_SPI_CH_SLAVE (1<<4)
> -#define S3C64XX_SPI_CPOL_L (1<<3)
> -#define S3C64XX_SPI_CPHA_B (1<<2)
> -#define S3C64XX_SPI_CH_RXCH_ON (1<<1)
> -#define S3C64XX_SPI_CH_TXCH_ON (1<<0)
> -
> -#define S3C64XX_SPI_CLKSEL_SRCMSK (3<<9)
> -#define S3C64XX_SPI_CLKSEL_SRCSHFT 9
> -#define S3C64XX_SPI_ENCLK_ENABLE (1<<8)
> -#define S3C64XX_SPI_PSR_MASK 0xff
> -
> -#define S3C64XX_SPI_MODE_CH_TSZ_BYTE (0<<29)
> -#define S3C64XX_SPI_MODE_CH_TSZ_HALFWORD (1<<29)
> -#define S3C64XX_SPI_MODE_CH_TSZ_WORD (2<<29)
> -#define S3C64XX_SPI_MODE_CH_TSZ_MASK (3<<29)
> -#define S3C64XX_SPI_MODE_BUS_TSZ_BYTE (0<<17)
> -#define S3C64XX_SPI_MODE_BUS_TSZ_HALFWORD (1<<17)
> -#define S3C64XX_SPI_MODE_BUS_TSZ_WORD (2<<17)
> -#define S3C64XX_SPI_MODE_BUS_TSZ_MASK (3<<17)
> +#define S3C64XX_SPI_CH_CFG 0x00
> +#define S3C64XX_SPI_CLK_CFG 0x04
> +#define S3C64XX_SPI_MODE_CFG 0x08
> +#define S3C64XX_SPI_CS_REG 0x0C
> +#define S3C64XX_SPI_INT_EN 0x10
> +#define S3C64XX_SPI_STATUS 0x14
> +#define S3C64XX_SPI_TX_DATA 0x18
> +#define S3C64XX_SPI_RX_DATA 0x1C
> +#define S3C64XX_SPI_PACKET_CNT 0x20
> +#define S3C64XX_SPI_PENDING_CLR 0x24
> +#define S3C64XX_SPI_SWAP_CFG 0x28
> +#define S3C64XX_SPI_FB_CLK 0x2C
> +
> +#define S3C64XX_SPI_CH_HS_EN BIT(6) /* High Speed Enable */
> +#define S3C64XX_SPI_CH_SW_RST BIT(5)
> +#define S3C64XX_SPI_CH_SLAVE BIT(4)
> +#define S3C64XX_SPI_CPOL_L BIT(3)
> +#define S3C64XX_SPI_CPHA_B BIT(2)
> +#define S3C64XX_SPI_CH_RXCH_ON BIT(1)
> +#define S3C64XX_SPI_CH_TXCH_ON BIT(0)
> +
> +#define S3C64XX_SPI_CLKSEL_SRCMSK GENMASK(10, 9)
> +#define S3C64XX_SPI_ENCLK_ENABLE BIT(8)
> +#define S3C64XX_SPI_PSR_MASK GENMASK(7, 0)
> +
> +#define S3C64XX_SPI_MODE_CH_TSZ_MASK GENMASK(30, 29)
> +#define S3C64XX_SPI_MODE_CH_TSZ_BYTE 0
> +#define S3C64XX_SPI_MODE_CH_TSZ_HALFWORD 1
> +#define S3C64XX_SPI_MODE_CH_TSZ_WORD 2
> +#define S3C64XX_SPI_MAX_TRAILCNT_MASK GENMASK(28, 19)
> +#define S3C64XX_SPI_MODE_BUS_TSZ_MASK GENMASK(18, 17)
> +#define S3C64XX_SPI_MODE_BUS_TSZ_BYTE 0
> +#define S3C64XX_SPI_MODE_BUS_TSZ_HALFWORD 1
> +#define S3C64XX_SPI_MODE_BUS_TSZ_WORD 2
> #define S3C64XX_SPI_MODE_RX_RDY_LVL GENMASK(16, 11)
> -#define S3C64XX_SPI_MODE_RX_RDY_LVL_SHIFT 11
> -#define S3C64XX_SPI_MODE_SELF_LOOPBACK (1<<3)
> -#define S3C64XX_SPI_MODE_RXDMA_ON (1<<2)
> -#define S3C64XX_SPI_MODE_TXDMA_ON (1<<1)
> -#define S3C64XX_SPI_MODE_4BURST (1<<0)
> -
> -#define S3C64XX_SPI_CS_NSC_CNT_2 (2<<4)
> -#define S3C64XX_SPI_CS_AUTO (1<<1)
> -#define S3C64XX_SPI_CS_SIG_INACT (1<<0)
> -
> -#define S3C64XX_SPI_INT_TRAILING_EN (1<<6)
> -#define S3C64XX_SPI_INT_RX_OVERRUN_EN (1<<5)
> -#define S3C64XX_SPI_INT_RX_UNDERRUN_EN (1<<4)
> -#define S3C64XX_SPI_INT_TX_OVERRUN_EN (1<<3)
> -#define S3C64XX_SPI_INT_TX_UNDERRUN_EN (1<<2)
> -#define S3C64XX_SPI_INT_RX_FIFORDY_EN (1<<1)
> -#define S3C64XX_SPI_INT_TX_FIFORDY_EN (1<<0)
> -
> -#define S3C64XX_SPI_ST_RX_OVERRUN_ERR (1<<5)
> -#define S3C64XX_SPI_ST_RX_UNDERRUN_ERR (1<<4)
> -#define S3C64XX_SPI_ST_TX_OVERRUN_ERR (1<<3)
> -#define S3C64XX_SPI_ST_TX_UNDERRUN_ERR (1<<2)
> -#define S3C64XX_SPI_ST_RX_FIFORDY (1<<1)
> -#define S3C64XX_SPI_ST_TX_FIFORDY (1<<0)
> -
> -#define S3C64XX_SPI_PACKET_CNT_EN (1<<16)
> +#define S3C64XX_SPI_MODE_SELF_LOOPBACK BIT(3)
> +#define S3C64XX_SPI_MODE_RXDMA_ON BIT(2)
> +#define S3C64XX_SPI_MODE_TXDMA_ON BIT(1)
> +#define S3C64XX_SPI_MODE_4BURST BIT(0)
> +
> +/*
> + * S3C64XX_SPI_CS_NSC_CNT_2 is a value into the NCS_TIME_COUNT field. In newer
> + * datasheets this field is defined as GENMASK(9, 4). We don't know if this mask
> + * applies to all the versions of the IP, thus we can't yet define
> + * S3C64XX_SPI_CS_NSC_CNT_2 as a value and the register field as a mask.
> + */
> +#define S3C64XX_SPI_CS_NSC_CNT_2 (2 << 4)
> +#define S3C64XX_SPI_CS_AUTO BIT(1)
> +#define S3C64XX_SPI_CS_SIG_INACT BIT(0)
> +
> +#define S3C64XX_SPI_INT_TRAILING_EN BIT(6)
> +#define S3C64XX_SPI_INT_RX_OVERRUN_EN BIT(5)
> +#define S3C64XX_SPI_INT_RX_UNDERRUN_EN BIT(4)
> +#define S3C64XX_SPI_INT_TX_OVERRUN_EN BIT(3)
> +#define S3C64XX_SPI_INT_TX_UNDERRUN_EN BIT(2)
> +#define S3C64XX_SPI_INT_RX_FIFORDY_EN BIT(1)
> +#define S3C64XX_SPI_INT_TX_FIFORDY_EN BIT(0)
> +
> +#define S3C64XX_SPI_ST_RX_OVERRUN_ERR BIT(5)
> +#define S3C64XX_SPI_ST_RX_UNDERRUN_ERR BIT(4)
> +#define S3C64XX_SPI_ST_TX_OVERRUN_ERR BIT(3)
> +#define S3C64XX_SPI_ST_TX_UNDERRUN_ERR BIT(2)
> +#define S3C64XX_SPI_ST_RX_FIFORDY BIT(1)
> +#define S3C64XX_SPI_ST_TX_FIFORDY BIT(0)
> +
> +#define S3C64XX_SPI_PACKET_CNT_EN BIT(16)
> #define S3C64XX_SPI_PACKET_CNT_MASK GENMASK(15, 0)
>
> -#define S3C64XX_SPI_PND_TX_UNDERRUN_CLR (1<<4)
> -#define S3C64XX_SPI_PND_TX_OVERRUN_CLR (1<<3)
> -#define S3C64XX_SPI_PND_RX_UNDERRUN_CLR (1<<2)
> -#define S3C64XX_SPI_PND_RX_OVERRUN_CLR (1<<1)
> -#define S3C64XX_SPI_PND_TRAILING_CLR (1<<0)
> +#define S3C64XX_SPI_PND_TX_UNDERRUN_CLR BIT(4)
> +#define S3C64XX_SPI_PND_TX_OVERRUN_CLR BIT(3)
> +#define S3C64XX_SPI_PND_RX_UNDERRUN_CLR BIT(2)
> +#define S3C64XX_SPI_PND_RX_OVERRUN_CLR BIT(1)
> +#define S3C64XX_SPI_PND_TRAILING_CLR BIT(0)
>
> -#define S3C64XX_SPI_SWAP_RX_HALF_WORD (1<<7)
> -#define S3C64XX_SPI_SWAP_RX_BYTE (1<<6)
> -#define S3C64XX_SPI_SWAP_RX_BIT (1<<5)
> -#define S3C64XX_SPI_SWAP_RX_EN (1<<4)
> -#define S3C64XX_SPI_SWAP_TX_HALF_WORD (1<<3)
> -#define S3C64XX_SPI_SWAP_TX_BYTE (1<<2)
> -#define S3C64XX_SPI_SWAP_TX_BIT (1<<1)
> -#define S3C64XX_SPI_SWAP_TX_EN (1<<0)
> +#define S3C64XX_SPI_SWAP_RX_HALF_WORD BIT(7)
> +#define S3C64XX_SPI_SWAP_RX_BYTE BIT(6)
> +#define S3C64XX_SPI_SWAP_RX_BIT BIT(5)
> +#define S3C64XX_SPI_SWAP_RX_EN BIT(4)
> +#define S3C64XX_SPI_SWAP_TX_HALF_WORD BIT(3)
> +#define S3C64XX_SPI_SWAP_TX_BYTE BIT(2)
> +#define S3C64XX_SPI_SWAP_TX_BIT BIT(1)
> +#define S3C64XX_SPI_SWAP_TX_EN BIT(0)
>
> -#define S3C64XX_SPI_FBCLK_MSK (3<<0)
> +#define S3C64XX_SPI_FBCLK_MASK GENMASK(1, 0)
>
> #define FIFO_LVL_MASK(i) ((i)->port_conf->fifo_lvl_mask[i->port_id])
> #define S3C64XX_SPI_ST_TX_DONE(v, i) (((v) & \
> @@ -112,16 +118,13 @@
> FIFO_LVL_MASK(i))
> #define FIFO_DEPTH(i) ((FIFO_LVL_MASK(i) >> 1) + 1)
>
> -#define S3C64XX_SPI_MAX_TRAILCNT 0x3ff
> -#define S3C64XX_SPI_TRAILCNT_OFF 19
> -
> #define S3C64XX_SPI_POLLING_SIZE 32
>
> #define msecs_to_loops(t) (loops_per_jiffy / 1000 * HZ * t)
> #define is_polling(x) (x->cntrlr_info->polling)
>
> -#define RXBUSY (1<<2)
> -#define TXBUSY (1<<3)
> +#define RXBUSY BIT(2)
> +#define TXBUSY BIT(3)
>
> struct s3c64xx_spi_dma_data {
> struct dma_chan *ch;
> @@ -664,16 +667,22 @@ static int s3c64xx_spi_config(struct s3c64xx_spi_driver_data *sdd)
>
> switch (sdd->cur_bpw) {
> case 32:
> - val |= S3C64XX_SPI_MODE_BUS_TSZ_WORD;
> - val |= S3C64XX_SPI_MODE_CH_TSZ_WORD;
> + val |= FIELD_PREP(S3C64XX_SPI_MODE_BUS_TSZ_MASK,
> + S3C64XX_SPI_MODE_BUS_TSZ_WORD) |
> + FIELD_PREP(S3C64XX_SPI_MODE_CH_TSZ_MASK,
> + S3C64XX_SPI_MODE_CH_TSZ_WORD);
> break;
> case 16:
> - val |= S3C64XX_SPI_MODE_BUS_TSZ_HALFWORD;
> - val |= S3C64XX_SPI_MODE_CH_TSZ_HALFWORD;
> + val |= FIELD_PREP(S3C64XX_SPI_MODE_BUS_TSZ_MASK,
> + S3C64XX_SPI_MODE_BUS_TSZ_HALFWORD) |
> + FIELD_PREP(S3C64XX_SPI_MODE_CH_TSZ_MASK,
> + S3C64XX_SPI_MODE_CH_TSZ_HALFWORD);
Two people complained it makes the code harder to read. Yet it's not
addressed in v3. Please see my comments for your previous submission
explaining what can be done, and also Andi's comment on that matter.
Also I think new patch series are being submitted a bit too fast,
people might not have enough time to provide the review.
> break;
> default:
> - val |= S3C64XX_SPI_MODE_BUS_TSZ_BYTE;
> - val |= S3C64XX_SPI_MODE_CH_TSZ_BYTE;
> + val |= FIELD_PREP(S3C64XX_SPI_MODE_BUS_TSZ_MASK,
> + S3C64XX_SPI_MODE_BUS_TSZ_BYTE) |
> + FIELD_PREP(S3C64XX_SPI_MODE_CH_TSZ_MASK,
> + S3C64XX_SPI_MODE_CH_TSZ_BYTE);
> break;
> }
>
> @@ -799,7 +808,7 @@ static int s3c64xx_spi_transfer_one(struct spi_controller *host,
>
> val = readl(sdd->regs + S3C64XX_SPI_MODE_CFG);
> val &= ~S3C64XX_SPI_MODE_RX_RDY_LVL;
> - val |= (rdy_lv << S3C64XX_SPI_MODE_RX_RDY_LVL_SHIFT);
> + val |= FIELD_PREP(S3C64XX_SPI_MODE_RX_RDY_LVL, rdy_lv);
> writel(val, sdd->regs + S3C64XX_SPI_MODE_CFG);
>
> /* Enable FIFO_RDY_EN IRQ */
> @@ -1072,8 +1081,8 @@ static void s3c64xx_spi_hwinit(struct s3c64xx_spi_driver_data *sdd)
> writel(0, regs + S3C64XX_SPI_INT_EN);
>
> if (!sdd->port_conf->clk_from_cmu)
> - writel(sci->src_clk_nr << S3C64XX_SPI_CLKSEL_SRCSHFT,
> - regs + S3C64XX_SPI_CLK_CFG);
> + writel(FIELD_PREP(S3C64XX_SPI_CLKSEL_SRCMSK, sci->src_clk_nr),
> + regs + S3C64XX_SPI_CLK_CFG);
> writel(0, regs + S3C64XX_SPI_MODE_CFG);
> writel(0, regs + S3C64XX_SPI_PACKET_CNT);
>
> @@ -1089,7 +1098,7 @@ static void s3c64xx_spi_hwinit(struct s3c64xx_spi_driver_data *sdd)
>
> val = readl(regs + S3C64XX_SPI_MODE_CFG);
> val &= ~S3C64XX_SPI_MODE_4BURST;
> - val |= (S3C64XX_SPI_MAX_TRAILCNT << S3C64XX_SPI_TRAILCNT_OFF);
> + val |= S3C64XX_SPI_MAX_TRAILCNT_MASK;
> writel(val, regs + S3C64XX_SPI_MODE_CFG);
>
> s3c64xx_flush_fifo(sdd);
> --
> 2.43.0.429.g432eaa2c6b-goog
>
On Fri, Jan 26, 2024 at 11:16 AM Tudor Ambarus <[email protected]> wrote:
>
> ETIMEDOUT is more specific than EIO, use it for
> wait_for_completion_timeout().
>
> Signed-off-by: Tudor Ambarus <[email protected]>
> ---
Looks like you missed my R-b tag I added to this patch in your
previous submission.
> drivers/spi/spi-s3c64xx.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
> index 76fa378ab5ab..2f2c4ad35df4 100644
> --- a/drivers/spi/spi-s3c64xx.c
> +++ b/drivers/spi/spi-s3c64xx.c
> @@ -526,7 +526,7 @@ static int s3c64xx_wait_for_dma(struct s3c64xx_spi_driver_data *sdd,
>
> /*
> * If the previous xfer was completed within timeout, then
> - * proceed further else return -EIO.
> + * proceed further else return -ETIMEDOUT.
> * DmaTx returns after simply writing data in the FIFO,
> * w/o waiting for real transmission on the bus to finish.
> * DmaRx returns only after Dma read data from FIFO which
> @@ -547,7 +547,7 @@ static int s3c64xx_wait_for_dma(struct s3c64xx_spi_driver_data *sdd,
>
> /* If timed out while checking rx/tx status return error */
> if (!val)
> - return -EIO;
> + return -ETIMEDOUT;
>
> return 0;
> }
> @@ -577,7 +577,7 @@ static int s3c64xx_wait_for_pio(struct s3c64xx_spi_driver_data *sdd,
> if (use_irq) {
> val = msecs_to_jiffies(ms);
> if (!wait_for_completion_timeout(&sdd->xfer_completion, val))
> - return -EIO;
> + return -ETIMEDOUT;
> }
>
> val = msecs_to_loops(ms);
> --
> 2.43.0.429.g432eaa2c6b-goog
>
On Fri, Jan 26, 2024 at 11:15 AM Tudor Ambarus <[email protected]> wrote:
>
> of_device_id::data is an opaque pointer. No explicit cast is needed.
> Remove unneeded (void *) casts in of_match_table. While here align the
> compatible and data members.
>
> Reviewed-by: Andi Shyti <[email protected]>
> Signed-off-by: Tudor Ambarus <[email protected]>
> ---
> drivers/spi/spi-s3c64xx.c | 45 +++++++++++++++++++++++----------------
> 1 file changed, 27 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
> index ccb700312d64..9bf54c1044b3 100644
> --- a/drivers/spi/spi-s3c64xx.c
> +++ b/drivers/spi/spi-s3c64xx.c
> @@ -1511,32 +1511,41 @@ static const struct platform_device_id s3c64xx_spi_driver_ids[] = {
> };
>
> static const struct of_device_id s3c64xx_spi_dt_match[] = {
> - { .compatible = "samsung,s3c2443-spi",
> - .data = (void *)&s3c2443_spi_port_config,
> + {
> + .compatible = "samsung,s3c2443-spi",
> + .data = &s3c2443_spi_port_config,
> },
> - { .compatible = "samsung,s3c6410-spi",
> - .data = (void *)&s3c6410_spi_port_config,
> + {
The braces style is not fixed. Yet that's a style patch, which on one
hand fixes the issue (unnecessary void * cast), but OTOH brings
another issue (non-canonical braces placement). Please see my comments
for your previous submission.
> + .compatible = "samsung,s3c6410-spi",
> + .data = &s3c6410_spi_port_config,
> },
> - { .compatible = "samsung,s5pv210-spi",
> - .data = (void *)&s5pv210_spi_port_config,
> + {
> + .compatible = "samsung,s5pv210-spi",
> + .data = &s5pv210_spi_port_config,
> },
> - { .compatible = "samsung,exynos4210-spi",
> - .data = (void *)&exynos4_spi_port_config,
> + {
> + .compatible = "samsung,exynos4210-spi",
> + .data = &exynos4_spi_port_config,
> },
> - { .compatible = "samsung,exynos7-spi",
> - .data = (void *)&exynos7_spi_port_config,
> + {
> + .compatible = "samsung,exynos7-spi",
> + .data = &exynos7_spi_port_config,
> },
> - { .compatible = "samsung,exynos5433-spi",
> - .data = (void *)&exynos5433_spi_port_config,
> + {
> + .compatible = "samsung,exynos5433-spi",
> + .data = &exynos5433_spi_port_config,
> },
> - { .compatible = "samsung,exynos850-spi",
> - .data = (void *)&exynos850_spi_port_config,
> + {
> + .compatible = "samsung,exynos850-spi",
> + .data = &exynos850_spi_port_config,
> },
> - { .compatible = "samsung,exynosautov9-spi",
> - .data = (void *)&exynosautov9_spi_port_config,
> + {
> + .compatible = "samsung,exynosautov9-spi",
> + .data = &exynosautov9_spi_port_config,
> },
> - { .compatible = "tesla,fsd-spi",
> - .data = (void *)&fsd_spi_port_config,
> + {
> + .compatible = "tesla,fsd-spi",
> + .data = &fsd_spi_port_config,
> },
> { },
> };
> --
> 2.43.0.429.g432eaa2c6b-goog
>
Hi, Sam,
Thanks for the review feedback!
On 1/26/24 20:12, Sam Protsenko wrote:
>> - val |= S3C64XX_SPI_MODE_BUS_TSZ_HALFWORD;
>> - val |= S3C64XX_SPI_MODE_CH_TSZ_HALFWORD;
>> + val |= FIELD_PREP(S3C64XX_SPI_MODE_BUS_TSZ_MASK,
>> + S3C64XX_SPI_MODE_BUS_TSZ_HALFWORD) |
>> + FIELD_PREP(S3C64XX_SPI_MODE_CH_TSZ_MASK,
>> + S3C64XX_SPI_MODE_CH_TSZ_HALFWORD);
> Two people complained it makes the code harder to read. Yet it's not
> addressed in v3. Please see my comments for your previous submission
> explaining what can be done, and also Andi's comment on that matter.
I kept these intentionally. Please read my reply on that matter or the
cover letter to this patch set.
> Also I think new patch series are being submitted a bit too fast,
> people might not have enough time to provide the review.
This patch set contains patches that are already reviewed or too simple
to being lagged.
Cheers,
ta
On 1/26/24 20:13, Sam Protsenko wrote:
>> ETIMEDOUT is more specific than EIO, use it for
>> wait_for_completion_timeout().
>>
>> Signed-off-by: Tudor Ambarus <[email protected]>
>> ---
> Looks like you missed my R-b tag I added to this patch in your
> previous submission.
My apologies. Adding it here should do the trick, here it is:
Reviewed-by: Sam Protsenko <[email protected]>
On Fri, Jan 26, 2024 at 9:23 PM Tudor Ambarus <tudor.ambarus@linaroorg> wrote:
>
> Hi, Sam,
>
> Thanks for the review feedback!
>
> On 1/26/24 20:12, Sam Protsenko wrote:
> >> - val |= S3C64XX_SPI_MODE_BUS_TSZ_HALFWORD;
> >> - val |= S3C64XX_SPI_MODE_CH_TSZ_HALFWORD;
> >> + val |= FIELD_PREP(S3C64XX_SPI_MODE_BUS_TSZ_MASK,
> >> + S3C64XX_SPI_MODE_BUS_TSZ_HALFWORD) |
> >> + FIELD_PREP(S3C64XX_SPI_MODE_CH_TSZ_MASK,
> >> + S3C64XX_SPI_MODE_CH_TSZ_HALFWORD);
> > Two people complained it makes the code harder to read. Yet it's not
> > addressed in v3. Please see my comments for your previous submission
> > explaining what can be done, and also Andi's comment on that matter.
>
> I kept these intentionally. Please read my reply on that matter or the
> cover letter to this patch set.
>
I read it. But still don't like it :) I'm sure it's possible to do
this modification, but at the same time keep the code clean an easy to
read. The code above -- I don't like at all, sorry. It was much better
before this patch, IMHO.
> > Also I think new patch series are being submitted a bit too fast,
> > people might not have enough time to provide the review.
>
> This patch set contains patches that are already reviewed or too simple
> to being lagged.
>
Ok, agreed.
> Cheers,
> ta
Hi, Sam,
Thanks for the review feedback.
On 1/26/24 20:17, Sam Protsenko wrote:
>> static const struct of_device_id s3c64xx_spi_dt_match[] = {
>> - { .compatible = "samsung,s3c2443-spi",
>> - .data = (void *)&s3c2443_spi_port_config,
>> + {
>> + .compatible = "samsung,s3c2443-spi",
>> + .data = &s3c2443_spi_port_config,
>> },
>> - { .compatible = "samsung,s3c6410-spi",
>> - .data = (void *)&s3c6410_spi_port_config,
>> + {
> The braces style is not fixed. Yet that's a style patch, which on one
> hand fixes the issue (unnecessary void * cast), but OTOH brings
> another issue (non-canonical braces placement). Please see my comments
> for your previous submission.
I've read your email and replied there that the braces were one on top
of the other even before my patch and since I don't have a preference on
whether to choose a style or the other, I kept the style as it were.
But I learnt my lesson. Mark can ignore this path when applying and I'll
submit a new version of the patch where I'll refrain myself making
alignments or changing the style.
Cheers,
ta
On 1/27/24 03:38, Sam Protsenko wrote:
>>>> - val |= S3C64XX_SPI_MODE_BUS_TSZ_HALFWORD;
>>>> - val |= S3C64XX_SPI_MODE_CH_TSZ_HALFWORD;
>>>> + val |= FIELD_PREP(S3C64XX_SPI_MODE_BUS_TSZ_MASK,
>>>> + S3C64XX_SPI_MODE_BUS_TSZ_HALFWORD) |
>>>> + FIELD_PREP(S3C64XX_SPI_MODE_CH_TSZ_MASK,
>>>> + S3C64XX_SPI_MODE_CH_TSZ_HALFWORD);
>>> Two people complained it makes the code harder to read. Yet it's not
>>> addressed in v3. Please see my comments for your previous submission
>>> explaining what can be done, and also Andi's comment on that matter.
>> I kept these intentionally. Please read my reply on that matter or the
>> cover letter to this patch set.
>>
> I read it. But still don't like it ???? I'm sure it's possible to do
> this modification, but at the same time keep the code clean an easy to
> read. The code above -- I don't like at all, sorry. It was much better
> before this patch, IMHO.
Yeah, I guess Mark will tip the scale.
Cheers,
ta
On Sat, Jan 27, 2024 at 03:44:24AM +0000, Tudor Ambarus wrote:
> On 1/27/24 03:38, Sam Protsenko wrote:
> >>>> - val |= S3C64XX_SPI_MODE_BUS_TSZ_HALFWORD;
> >>>> - val |= S3C64XX_SPI_MODE_CH_TSZ_HALFWORD;
> >>>> + val |= FIELD_PREP(S3C64XX_SPI_MODE_BUS_TSZ_MASK,
> >>>> + S3C64XX_SPI_MODE_BUS_TSZ_HALFWORD) |
> >>>> + FIELD_PREP(S3C64XX_SPI_MODE_CH_TSZ_MASK,
> >>>> + S3C64XX_SPI_MODE_CH_TSZ_HALFWORD);
> >>> Two people complained it makes the code harder to read. Yet it's not
> >>> addressed in v3. Please see my comments for your previous submission
> >>> explaining what can be done, and also Andi's comment on that matter.
> >> I kept these intentionally. Please read my reply on that matter or the
> >> cover letter to this patch set.
> > I read it. But still don't like it ???? I'm sure it's possible to do
> > this modification, but at the same time keep the code clean an easy to
> > read. The code above -- I don't like at all, sorry. It was much better
> > before this patch, IMHO.
> Yeah, I guess Mark will tip the scale.
All other things being equal I tend to try not to get too involved with
minor coding style stuff in drivers. People do seem to like
FIELD_PREP() but I have a hard time getting *too* excited.
On 1/29/24 16:42, Mark Brown wrote:
> On Sat, Jan 27, 2024 at 03:44:24AM +0000, Tudor Ambarus wrote:
>> On 1/27/24 03:38, Sam Protsenko wrote:
>
>>>>>> - val |= S3C64XX_SPI_MODE_BUS_TSZ_HALFWORD;
>>>>>> - val |= S3C64XX_SPI_MODE_CH_TSZ_HALFWORD;
>>>>>> + val |= FIELD_PREP(S3C64XX_SPI_MODE_BUS_TSZ_MASK,
>>>>>> + S3C64XX_SPI_MODE_BUS_TSZ_HALFWORD) |
>>>>>> + FIELD_PREP(S3C64XX_SPI_MODE_CH_TSZ_MASK,
>>>>>> + S3C64XX_SPI_MODE_CH_TSZ_HALFWORD);
>
>>>>> Two people complained it makes the code harder to read. Yet it's not
>>>>> addressed in v3. Please see my comments for your previous submission
>>>>> explaining what can be done, and also Andi's comment on that matter.
>
>>>> I kept these intentionally. Please read my reply on that matter or the
>>>> cover letter to this patch set.
>
>>> I read it. But still don't like it ???? I'm sure it's possible to do
>>> this modification, but at the same time keep the code clean an easy to
>>> read. The code above -- I don't like at all, sorry. It was much better
>>> before this patch, IMHO.
>
>> Yeah, I guess Mark will tip the scale.
>
> All other things being equal I tend to try not to get too involved with
> minor coding style stuff in drivers. People do seem to like
> FIELD_PREP() but I have a hard time getting *too* excited.
Ok, I'll remove FIELD_PREP. Would you please consider the other patches,
all are simple. There's another "controversy" on 6/17. You can ignore
that as well maybe, and I'll resend it where I refrain myself to just
removing the cast.