[PATCH] spi: tegra114: Don't fail set_cs_timing when delays are zero

[PATCH] spi: tegra114: Don't fail set_cs_timing when delays are zero

[Aaron Kling via B4 Relay]

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)) {
 		dev_err(&spi->dev,
 			"Invalid delay unit %d, should be SPI_DELAY_UNIT_SCK\n",
 			SPI_DELAY_UNIT_SCK);

---
base-commit: a79be02bba5c31f967885c7f3bf3a756d77d11d9
change-id: 20250423-spi-tegra114-b88828c5c0f3

Best regards,
-- 
Aaron Kling <webgeek1234@gmail.com>

Re: [PATCH] spi: tegra114: Don't fail set_cs_timing when delays are zero

[Mark Brown]

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

Re: [PATCH] spi: tegra114: Don't fail set_cs_timing when delays are zero

[Jon Hunter]

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

-- 
nvpublic

Re: [PATCH] spi: tegra114: Don't fail set_cs_timing when delays are zero

[Aaron Kling]

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

Re: [PATCH] spi: tegra114: Don't fail set_cs_timing when delays are zero

[Jon Hunter]

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

-- 
nvpublic

Re: [PATCH] spi: tegra114: Don't fail set_cs_timing when delays are zero

[Aaron Kling]

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

Re: [PATCH] spi: tegra114: Don't fail set_cs_timing when delays are zero

[Jon Hunter]

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

-- 
nvpublic