Message ID | 20250423-spi-tegra114-v1-1-2d608bcc12f9@gmail.com |
---|---|
State | New |
Headers | show |
Series | spi: tegra114: Don't fail set_cs_timing when delays are zero | expand |
On Wed, 23 Apr 2025 21:03:03 -0500, Aaron Kling wrote: > The original code would skip null delay pointers, but when the pointers > were converted to point within the spi_device struct, the check was not > updated to skip delays of zero. Hence all spi devices that didn't set > delays would fail to probe. > > Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next Thanks! [1/1] spi: tegra114: Don't fail set_cs_timing when delays are zero commit: 4426e6b4ecf632bb75d973051e1179b8bfac2320 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
On 24/04/2025 03:03, Aaron Kling via B4 Relay wrote: > From: Aaron Kling <webgeek1234@gmail.com> > > The original code would skip null delay pointers, but when the pointers > were converted to point within the spi_device struct, the check was not > updated to skip delays of zero. Hence all spi devices that didn't set > delays would fail to probe. > > Fixes: 04e6bb0d6bb1 ("spi: modify set_cs_timing parameter") > Cc: stable@vger.kernel.org > Signed-off-by: Aaron Kling <webgeek1234@gmail.com> > --- > drivers/spi/spi-tegra114.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/spi/spi-tegra114.c b/drivers/spi/spi-tegra114.c > index 3822d7c8d8edb9730e937df50d1c75e095dd18ec..2a8bb798e95b954fe573f1c50445ed2e7fcbfd78 100644 > --- a/drivers/spi/spi-tegra114.c > +++ b/drivers/spi/spi-tegra114.c > @@ -728,9 +728,9 @@ static int tegra_spi_set_hw_cs_timing(struct spi_device *spi) > u32 inactive_cycles; > u8 cs_state; > > - if (setup->unit != SPI_DELAY_UNIT_SCK || > - hold->unit != SPI_DELAY_UNIT_SCK || > - inactive->unit != SPI_DELAY_UNIT_SCK) { > + if ((setup->unit && setup->unit != SPI_DELAY_UNIT_SCK) || > + (hold->unit && hold->unit != SPI_DELAY_UNIT_SCK) || > + (inactive->unit && inactive->unit != SPI_DELAY_UNIT_SCK)) { The above does not look correct to me. For example, if 'setup->unit' is 0, this means that the unit is 'SPI_DELAY_UNIT_USECS' and does not indicate that the delay is 0. Shouldn't the above be ... if ((setup && setup->unit != SPI_DELAY_UNIT_SCK) || (hold && hold->unit != SPI_DELAY_UNIT_SCK) || (inactive && inactive->unit != SPI_DELAY_UNIT_SCK)) { Jon
On Tue, May 6, 2025 at 4:27 AM Jon Hunter <jonathanh@nvidia.com> wrote: > > > On 24/04/2025 03:03, Aaron Kling via B4 Relay wrote: > > From: Aaron Kling <webgeek1234@gmail.com> > > > > The original code would skip null delay pointers, but when the pointers > > were converted to point within the spi_device struct, the check was not > > updated to skip delays of zero. Hence all spi devices that didn't set > > delays would fail to probe. > > > > Fixes: 04e6bb0d6bb1 ("spi: modify set_cs_timing parameter") > > Cc: stable@vger.kernel.org > > Signed-off-by: Aaron Kling <webgeek1234@gmail.com> > > --- > > drivers/spi/spi-tegra114.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/spi/spi-tegra114.c b/drivers/spi/spi-tegra114.c > > index 3822d7c8d8edb9730e937df50d1c75e095dd18ec..2a8bb798e95b954fe573f1c50445ed2e7fcbfd78 100644 > > --- a/drivers/spi/spi-tegra114.c > > +++ b/drivers/spi/spi-tegra114.c > > @@ -728,9 +728,9 @@ static int tegra_spi_set_hw_cs_timing(struct spi_device *spi) > > u32 inactive_cycles; > > u8 cs_state; > > > > - if (setup->unit != SPI_DELAY_UNIT_SCK || > > - hold->unit != SPI_DELAY_UNIT_SCK || > > - inactive->unit != SPI_DELAY_UNIT_SCK) { > > + if ((setup->unit && setup->unit != SPI_DELAY_UNIT_SCK) || > > + (hold->unit && hold->unit != SPI_DELAY_UNIT_SCK) || > > + (inactive->unit && inactive->unit != SPI_DELAY_UNIT_SCK)) { > > The above does not look correct to me. For example, if 'setup->unit' is > 0, this means that the unit is 'SPI_DELAY_UNIT_USECS' and does not > indicate that the delay is 0. > > Shouldn't the above be ... > > if ((setup && setup->unit != SPI_DELAY_UNIT_SCK) || > (hold && hold->unit != SPI_DELAY_UNIT_SCK) || > (inactive && inactive->unit != SPI_DELAY_UNIT_SCK)) { This is what the code looked like before 373c36b [0], which dropped that check because the pointers can never be NULL. Should this check if ->value is not 0 instead? Sincerely, Aaron Kling [0] https://github.com/torvalds/linux/commit/373c36bf7914e3198ac2654dede499f340c52950
On 06/05/2025 10:50, Aaron Kling wrote: ... >>> - if (setup->unit != SPI_DELAY_UNIT_SCK || >>> - hold->unit != SPI_DELAY_UNIT_SCK || >>> - inactive->unit != SPI_DELAY_UNIT_SCK) { >>> + if ((setup->unit && setup->unit != SPI_DELAY_UNIT_SCK) || >>> + (hold->unit && hold->unit != SPI_DELAY_UNIT_SCK) || >>> + (inactive->unit && inactive->unit != SPI_DELAY_UNIT_SCK)) { >> >> The above does not look correct to me. For example, if 'setup->unit' is >> 0, this means that the unit is 'SPI_DELAY_UNIT_USECS' and does not >> indicate that the delay is 0. >> >> Shouldn't the above be ... >> >> if ((setup && setup->unit != SPI_DELAY_UNIT_SCK) || >> (hold && hold->unit != SPI_DELAY_UNIT_SCK) || >> (inactive && inactive->unit != SPI_DELAY_UNIT_SCK)) { > > This is what the code looked like before 373c36b [0], which dropped > that check because the pointers can never be NULL. Should this check > if ->value is not 0 instead? What the code does now does not match what you describe and does not appear to be correct. Yes checking ->value is not 0 would make sense. Jon
On Tue, May 6, 2025 at 5:06 AM Jon Hunter <jonathanh@nvidia.com> wrote: > > > On 06/05/2025 10:50, Aaron Kling wrote: > > ... > > >>> - if (setup->unit != SPI_DELAY_UNIT_SCK || > >>> - hold->unit != SPI_DELAY_UNIT_SCK || > >>> - inactive->unit != SPI_DELAY_UNIT_SCK) { > >>> + if ((setup->unit && setup->unit != SPI_DELAY_UNIT_SCK) || > >>> + (hold->unit && hold->unit != SPI_DELAY_UNIT_SCK) || > >>> + (inactive->unit && inactive->unit != SPI_DELAY_UNIT_SCK)) { > >> > >> The above does not look correct to me. For example, if 'setup->unit' is > >> 0, this means that the unit is 'SPI_DELAY_UNIT_USECS' and does not > >> indicate that the delay is 0. > >> > >> Shouldn't the above be ... > >> > >> if ((setup && setup->unit != SPI_DELAY_UNIT_SCK) || > >> (hold && hold->unit != SPI_DELAY_UNIT_SCK) || > >> (inactive && inactive->unit != SPI_DELAY_UNIT_SCK)) { > > > > This is what the code looked like before 373c36b [0], which dropped > > that check because the pointers can never be NULL. Should this check > > if ->value is not 0 instead? > > What the code does now does not match what you describe and does not > appear to be correct. Yes checking ->value is not 0 would make sense. Alright, I will send in a new revision once I can verify the change. Since this was already picked up, is there anything I need to do to get the bad patch pulled? Sincerely, Aaron Kling
On 06/05/2025 11:12, Aaron Kling wrote: ... > Alright, I will send in a new revision once I can verify the change. > Since this was already picked up, is there anything I need to do to > get the bad patch pulled? You can either send a revert of the bad patch or just fix up the existing patch and reference the bad patch in the 'Fixes:' tag. Jon
diff --git a/drivers/spi/spi-tegra114.c b/drivers/spi/spi-tegra114.c index 3822d7c8d8edb9730e937df50d1c75e095dd18ec..2a8bb798e95b954fe573f1c50445ed2e7fcbfd78 100644 --- a/drivers/spi/spi-tegra114.c +++ b/drivers/spi/spi-tegra114.c @@ -728,9 +728,9 @@ static int tegra_spi_set_hw_cs_timing(struct spi_device *spi) u32 inactive_cycles; u8 cs_state; - if (setup->unit != SPI_DELAY_UNIT_SCK || - hold->unit != SPI_DELAY_UNIT_SCK || - inactive->unit != SPI_DELAY_UNIT_SCK) { + if ((setup->unit && setup->unit != SPI_DELAY_UNIT_SCK) || + (hold->unit && hold->unit != SPI_DELAY_UNIT_SCK) || + (inactive->unit && inactive->unit != SPI_DELAY_UNIT_SCK)) { dev_err(&spi->dev, "Invalid delay unit %d, should be SPI_DELAY_UNIT_SCK\n", SPI_DELAY_UNIT_SCK);