| Message ID | 20240120170001.3356-1-semen.protsenko@linaro.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 460efee706c2b6a4daba62ec143fea29c2e7b358 |
| Headers | show |
| Series | spi: s3c64xx: Extract FIFO depth calculation to a dedicated macro | expand |
Hi Sam, > void __iomem *regs = sdd->regs; > unsigned long val = 1; > u32 status; > - > - /* max fifo depth available */ > - u32 max_fifo = (FIFO_LVL_MASK(sdd) >> 1) + 1; > + u32 max_fifo = FIFO_DEPTH(sdd); Why have you removed the comment? Perhaps you could place it on the side in order to remove that awful space. Not a biding comment, though: Reviewed-by: Andi Shyti <andi.shyti@kernel.org> Andi
On Sun, Jan 21, 2024 at 2:24 PM Andi Shyti <andi.shyti@kernel.org> wrote: > > Hi Sam, > > > void __iomem *regs = sdd->regs; > > unsigned long val = 1; > > u32 status; > > - > > - /* max fifo depth available */ > > - u32 max_fifo = (FIFO_LVL_MASK(sdd) >> 1) + 1; > > + u32 max_fifo = FIFO_DEPTH(sdd); > > Why have you removed the comment? Perhaps you could place it on > the side in order to remove that awful space. > The fact that `max_fifo' contains max FIFO depth is already coded in the variable name itself. And with that new FIFO_DEPTH() macro, it would be basically stating the same thing the third time on the same string. Thought the removal of that comment only made the code easier to read. If you think I should bring the comment back, please let me know and I'll send v2. > Not a biding comment, though: > > Reviewed-by: Andi Shyti <andi.shyti@kernel.org> > > Andi
Hi Sam, On Sun, Jan 21, 2024 at 04:11:21PM -0600, Sam Protsenko wrote: > On Sun, Jan 21, 2024 at 2:24 PM Andi Shyti <andi.shyti@kernel.org> wrote: > > > > Hi Sam, > > > > > void __iomem *regs = sdd->regs; > > > unsigned long val = 1; > > > u32 status; > > > - > > > - /* max fifo depth available */ > > > - u32 max_fifo = (FIFO_LVL_MASK(sdd) >> 1) + 1; > > > + u32 max_fifo = FIFO_DEPTH(sdd); > > > > Why have you removed the comment? Perhaps you could place it on > > the side in order to remove that awful space. > > > > The fact that `max_fifo' contains max FIFO depth is already coded in > the variable name itself. And with that new FIFO_DEPTH() macro, it > would be basically stating the same thing the third time on the same > string. Thought the removal of that comment only made the code easier > to read. If you think I should bring the comment back, please let me > know and I'll send v2. No, that's fine... you have a point here :-) Andi
On Sun, Jan 21, 2024 at 5:27 PM Andi Shyti <andi.shyti@kernel.org> wrote: > > Hi Sam, > > On Sun, Jan 21, 2024 at 04:11:21PM -0600, Sam Protsenko wrote: > > On Sun, Jan 21, 2024 at 2:24 PM Andi Shyti <andi.shyti@kernel.org> wrote: > > > > > > Hi Sam, > > > > > > > void __iomem *regs = sdd->regs; > > > > unsigned long val = 1; > > > > u32 status; > > > > - > > > > - /* max fifo depth available */ > > > > - u32 max_fifo = (FIFO_LVL_MASK(sdd) >> 1) + 1; > > > > + u32 max_fifo = FIFO_DEPTH(sdd); > > > > > > Why have you removed the comment? Perhaps you could place it on > > > the side in order to remove that awful space. > > > > > > > The fact that `max_fifo' contains max FIFO depth is already coded in > > the variable name itself. And with that new FIFO_DEPTH() macro, it > > would be basically stating the same thing the third time on the same > > string. Thought the removal of that comment only made the code easier > > to read. If you think I should bring the comment back, please let me > > know and I'll send v2. > > No, that's fine... you have a point here :-) > Thanks for the review! :) > Andi
On Sat, 20 Jan 2024 11:00:01 -0600, Sam Protsenko wrote: > Simplify the code by extracting all cases of FIFO depth calculation into > a dedicated macro. No functional change. > > Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next Thanks! [1/1] spi: s3c64xx: Extract FIFO depth calculation to a dedicated macro commit: 460efee706c2b6a4daba62ec143fea29c2e7b358 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
diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c index f7d623ad6ac3..7f7eb8f742e4 100644 --- a/drivers/spi/spi-s3c64xx.c +++ b/drivers/spi/spi-s3c64xx.c @@ -109,6 +109,7 @@ #define TX_FIFO_LVL(v, i) (((v) >> 6) & FIFO_LVL_MASK(i)) #define RX_FIFO_LVL(v, i) (((v) >> (i)->port_conf->rx_lvl_offset) & \ 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 @@ -406,7 +407,7 @@ 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) { - return xfer->len > (FIFO_LVL_MASK(sdd) >> 1) + 1; + return xfer->len > FIFO_DEPTH(sdd); } else { return false; } @@ -495,9 +496,7 @@ static u32 s3c64xx_spi_wait_for_timeout(struct s3c64xx_spi_driver_data *sdd, void __iomem *regs = sdd->regs; unsigned long val = 1; u32 status; - - /* max fifo depth available */ - u32 max_fifo = (FIFO_LVL_MASK(sdd) >> 1) + 1; + u32 max_fifo = FIFO_DEPTH(sdd); if (timeout_ms) val = msecs_to_loops(timeout_ms); @@ -604,7 +603,7 @@ static int s3c64xx_wait_for_pio(struct s3c64xx_spi_driver_data *sdd, * For any size less than the fifo size the below code is * executed atleast once. */ - loops = xfer->len / ((FIFO_LVL_MASK(sdd) >> 1) + 1); + loops = xfer->len / FIFO_DEPTH(sdd); buf = xfer->rx_buf; do { /* wait for data to be received in the fifo */ @@ -741,7 +740,7 @@ static int s3c64xx_spi_transfer_one(struct spi_controller *host, struct spi_transfer *xfer) { struct s3c64xx_spi_driver_data *sdd = spi_controller_get_devdata(host); - const unsigned int fifo_len = (FIFO_LVL_MASK(sdd) >> 1) + 1; + const unsigned int fifo_len = FIFO_DEPTH(sdd); const void *tx_buf = NULL; void *rx_buf = NULL; int target_len = 0, origin_len = 0; @@ -1280,7 +1279,7 @@ static int s3c64xx_spi_probe(struct platform_device *pdev) dev_dbg(&pdev->dev, "Samsung SoC SPI Driver loaded for Bus SPI-%d with %d Targets attached\n", sdd->port_id, host->num_chipselect); dev_dbg(&pdev->dev, "\tIOmem=[%pR]\tFIFO %dbytes\n", - mem_res, (FIFO_LVL_MASK(sdd) >> 1) + 1); + mem_res, FIFO_DEPTH(sdd)); pm_runtime_mark_last_busy(&pdev->dev); pm_runtime_put_autosuspend(&pdev->dev);
Simplify the code by extracting all cases of FIFO depth calculation into a dedicated macro. No functional change. Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> --- drivers/spi/spi-s3c64xx.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)