| Message ID | 20260518095708.2502537-3-chin-ting_kuo@aspeedtech.com |
|---|---|
| State | New |
| Headers | show |
| Series | spi: aspeed: Replace VLA parameter with flat pointer in calibration helper | expand |
On 5/18/26 11:57, Chin-Ting Kuo wrote: > aspeed_spi_ast2600_optimized_timing() declared its buffer argument as a > variable-length array parameter (u8 buf[rows][cols]), which causes a > sparse warning. Replace the VLA parameter with a plain u8 * and compute > the 2-D index manually. The corresponding call site is also updated. > > Reported-by: kernel test robot <lkp@intel.com> > Closes: https://lore.kernel.org/oe-kbuild-all/202605180441.uD3toFRJ-lkp@intel.com/ > Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com> > --- > drivers/spi/spi-aspeed-smc.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/drivers/spi/spi-aspeed-smc.c b/drivers/spi/spi-aspeed-smc.c > index 808659a1f460..c5275700de3d 100644 > --- a/drivers/spi/spi-aspeed-smc.c > +++ b/drivers/spi/spi-aspeed-smc.c > @@ -1467,8 +1467,7 @@ static int aspeed_spi_do_calibration(struct aspeed_spi_chip *chip) > * must contains the highest number of consecutive "pass" > * results and not span across multiple rows. > */ > -static u32 aspeed_spi_ast2600_optimized_timing(u32 rows, u32 cols, > - u8 buf[rows][cols]) > +static u32 aspeed_spi_ast2600_optimized_timing(u32 rows, u32 cols, u8 *buf) > { > int r = 0, c = 0; > int max = 0; > @@ -1478,7 +1477,7 @@ static u32 aspeed_spi_ast2600_optimized_timing(u32 rows, u32 cols, > for (j = 0; j < cols;) { > int k = j; > > - while (k < cols && buf[i][k]) > + while (k < cols && buf[i * cols + k]) > k++; > > if (k - j > max) { > @@ -1541,7 +1540,7 @@ static int aspeed_spi_ast2600_calibrate(struct aspeed_spi_chip *chip, u32 hdiv, > } > } > > - calib_point = aspeed_spi_ast2600_optimized_timing(6, 17, calib_res); > + calib_point = aspeed_spi_ast2600_optimized_timing(6, 17, &calib_res[0][0]); > /* No good setting for this frequency */ > if (calib_point == 0) > return -1; Reviewed-by: Cédric Le Goater <clg@kaod.org> Thanks, C.
On Mon, May 18, 2026 at 05:57:08PM +0800, Chin-Ting Kuo wrote: > - while (k < cols && buf[i][k]) > + while (k < cols && buf[i * cols + k]) This really needs () to make it clear what's going on; the precedence is well defined but not everyone is going to know that off the top of their head.
Hi Mark, Thanks for the review. > -----Original Message----- > From: Mark Brown <broonie@kernel.org> > Sent: Tuesday, May 19, 2026 7:04 PM > Subject: Re: [PATCH] spi: aspeed: Replace VLA parameter with flat pointer in > calibration helper > > On Mon, May 18, 2026 at 05:57:08PM +0800, Chin-Ting Kuo wrote: > > > - while (k < cols && buf[i][k]) > > + while (k < cols && buf[i * cols + k]) > > This really needs () to make it clear what's going on; the precedence is well > defined but not everyone is going to know that off the top of their head. Okay, below change will be added in the next patch version. while (k < cols && buf[(i * cols) + k]) Chin-Ting
On Tue, May 19, 2026 at 12:43:44PM +0000, Chin-Ting Kuo wrote: > > This really needs () to make it clear what's going on; the precedence is well > > defined but not everyone is going to know that off the top of their head. > Okay, below change will be added in the next patch version. > while (k < cols && buf[(i * cols) + k]) Sure, looks OK.
On Tue, 19 May 2026 12:03:51 +0100 Mark Brown <broonie@kernel.org> wrote: > On Mon, May 18, 2026 at 05:57:08PM +0800, Chin-Ting Kuo wrote: > > > - while (k < cols && buf[i][k]) > > + while (k < cols && buf[i * cols + k]) > > This really needs () to make it clear what's going on; the precedence is > well defined but not everyone is going to know that off the top of their > head. Come on, it's multiply and add - everyone is going to get that right. -- David
On Tue, May 19, 2026 at 06:13:48PM +0100, David Laight wrote: > Mark Brown <broonie@kernel.org> wrote: > > On Mon, May 18, 2026 at 05:57:08PM +0800, Chin-Ting Kuo wrote: > > > - while (k < cols && buf[i][k]) > > > + while (k < cols && buf[i * cols + k]) > > This really needs () to make it clear what's going on; the precedence is > > well defined but not everyone is going to know that off the top of their > > head. > Come on, it's multiply and add - everyone is going to get that right. No, I have to stop and think. It's not just "what is the rule" it's also "is that the same rule whoever wrote the code thought there was" - implicit precedence is the sort of thing that flags up as an alarm bell when scanning through code.
diff --git a/drivers/spi/spi-aspeed-smc.c b/drivers/spi/spi-aspeed-smc.c index 808659a1f460..c5275700de3d 100644 --- a/drivers/spi/spi-aspeed-smc.c +++ b/drivers/spi/spi-aspeed-smc.c @@ -1467,8 +1467,7 @@ static int aspeed_spi_do_calibration(struct aspeed_spi_chip *chip) * must contains the highest number of consecutive "pass" * results and not span across multiple rows. */ -static u32 aspeed_spi_ast2600_optimized_timing(u32 rows, u32 cols, - u8 buf[rows][cols]) +static u32 aspeed_spi_ast2600_optimized_timing(u32 rows, u32 cols, u8 *buf) { int r = 0, c = 0; int max = 0; @@ -1478,7 +1477,7 @@ static u32 aspeed_spi_ast2600_optimized_timing(u32 rows, u32 cols, for (j = 0; j < cols;) { int k = j; - while (k < cols && buf[i][k]) + while (k < cols && buf[i * cols + k]) k++; if (k - j > max) { @@ -1541,7 +1540,7 @@ static int aspeed_spi_ast2600_calibrate(struct aspeed_spi_chip *chip, u32 hdiv, } } - calib_point = aspeed_spi_ast2600_optimized_timing(6, 17, calib_res); + calib_point = aspeed_spi_ast2600_optimized_timing(6, 17, &calib_res[0][0]); /* No good setting for this frequency */ if (calib_point == 0) return -1;
aspeed_spi_ast2600_optimized_timing() declared its buffer argument as a variable-length array parameter (u8 buf[rows][cols]), which causes a sparse warning. Replace the VLA parameter with a plain u8 * and compute the 2-D index manually. The corresponding call site is also updated. Reported-by: kernel test robot <lkp@intel.com> Closes: https://lore.kernel.org/oe-kbuild-all/202605180441.uD3toFRJ-lkp@intel.com/ Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com> --- drivers/spi/spi-aspeed-smc.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)