[01/10] spi: spi-axi-spi-engine: remove usage of delay_usecs
diff mbox series

Message ID 20210308145502.1075689-2-aardelean@deviqon.com
State Accepted
Commit 93c941448994a728e691f7dce9ea6475e352b09c
Headers show
Series
  • spi: finalize 'delay_usecs' removal/transition
Related show

Commit Message

Alexandru Ardelean March 8, 2021, 2:54 p.m. UTC
The 'delay_usecs' field was handled for backwards compatibility in case
there were some users that still configured SPI delay transfers with
this field.

They should all be removed by now.

Signed-off-by: Alexandru Ardelean <aardelean@deviqon.com>
---
 drivers/spi/spi-axi-spi-engine.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

Comments

Lars-Peter Clausen March 8, 2021, 4:42 p.m. UTC | #1
On 3/8/21 3:54 PM, Alexandru Ardelean wrote:
> The 'delay_usecs' field was handled for backwards compatibility in case
> there were some users that still configured SPI delay transfers with
> this field.
>
> They should all be removed by now.
>
> Signed-off-by: Alexandru Ardelean <aardelean@deviqon.com>
> ---
>   drivers/spi/spi-axi-spi-engine.c | 12 ++++--------
>   1 file changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/spi/spi-axi-spi-engine.c b/drivers/spi/spi-axi-spi-engine.c
> index af86e6d6e16b..80c3e38f5c1b 100644
> --- a/drivers/spi/spi-axi-spi-engine.c
> +++ b/drivers/spi/spi-axi-spi-engine.c
> @@ -170,14 +170,10 @@ static void spi_engine_gen_sleep(struct spi_engine_program *p, bool dry,
>   	unsigned int t;
>   	int delay;
>   
> -	if (xfer->delay_usecs) {
> -		delay = xfer->delay_usecs;
> -	} else {
> -		delay = spi_delay_to_ns(&xfer->delay, xfer);
> -		if (delay < 0)
> -			return;
> -		delay /= 1000;
> -	}
> +	delay = spi_delay_to_ns(&xfer->delay, xfer);
> +	if (delay < 0)
> +		return;

Bit of a nit, but this could be `delay <= 0` and then drop the check for 
`delay == 0` below.

> +	delay /= 1000;
>   
>   	if (delay == 0)
>   		return;
Alexandru Ardelean March 10, 2021, 7:16 a.m. UTC | #2
On Mon, 8 Mar 2021 at 18:42, Lars-Peter Clausen <lars@metafoo.de> wrote:
>
> On 3/8/21 3:54 PM, Alexandru Ardelean wrote:
> > The 'delay_usecs' field was handled for backwards compatibility in case
> > there were some users that still configured SPI delay transfers with
> > this field.
> >
> > They should all be removed by now.
> >
> > Signed-off-by: Alexandru Ardelean <aardelean@deviqon.com>
> > ---
> >   drivers/spi/spi-axi-spi-engine.c | 12 ++++--------
> >   1 file changed, 4 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/spi/spi-axi-spi-engine.c b/drivers/spi/spi-axi-spi-engine.c
> > index af86e6d6e16b..80c3e38f5c1b 100644
> > --- a/drivers/spi/spi-axi-spi-engine.c
> > +++ b/drivers/spi/spi-axi-spi-engine.c
> > @@ -170,14 +170,10 @@ static void spi_engine_gen_sleep(struct spi_engine_program *p, bool dry,
> >       unsigned int t;
> >       int delay;
> >
> > -     if (xfer->delay_usecs) {
> > -             delay = xfer->delay_usecs;
> > -     } else {
> > -             delay = spi_delay_to_ns(&xfer->delay, xfer);
> > -             if (delay < 0)
> > -                     return;
> > -             delay /= 1000;
> > -     }
> > +     delay = spi_delay_to_ns(&xfer->delay, xfer);
> > +     if (delay < 0)
> > +             return;
>
> Bit of a nit, but this could be `delay <= 0` and then drop the check for
> `delay == 0` below.

hmm, that's a bit debatable, because the `delay == 0` check comes
after `delay /= 1000` ;
to do what you're suggesting, it would probably need a
DIV_ROUND_UP(delay, 1000) to make sure that even sub-microsecond
delays don't become zero;

if you're acking this suggestion i'll implement it;
i'll wait a few more days to see if there are any other acks or
complaints on the set before sending a V2;

btw: this new spi_delay struct supports delays in microseconds,
nanoseconds and clock cycles;
at some point it may be interesting to use a
`spi_delay_to_clk_cycles()` for this driver and other similar;

>
> > +     delay /= 1000;
> >
> >       if (delay == 0)
> >               return;
>
>
Lars-Peter Clausen March 10, 2021, 7:21 a.m. UTC | #3
On 3/10/21 8:16 AM, Alexandru Ardelean wrote:
> On Mon, 8 Mar 2021 at 18:42, Lars-Peter Clausen <lars@metafoo.de> wrote:
>> On 3/8/21 3:54 PM, Alexandru Ardelean wrote:
>>> The 'delay_usecs' field was handled for backwards compatibility in case
>>> there were some users that still configured SPI delay transfers with
>>> this field.
>>>
>>> They should all be removed by now.
>>>
>>> Signed-off-by: Alexandru Ardelean <aardelean@deviqon.com>
>>> ---
>>>    drivers/spi/spi-axi-spi-engine.c | 12 ++++--------
>>>    1 file changed, 4 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/spi/spi-axi-spi-engine.c b/drivers/spi/spi-axi-spi-engine.c
>>> index af86e6d6e16b..80c3e38f5c1b 100644
>>> --- a/drivers/spi/spi-axi-spi-engine.c
>>> +++ b/drivers/spi/spi-axi-spi-engine.c
>>> @@ -170,14 +170,10 @@ static void spi_engine_gen_sleep(struct spi_engine_program *p, bool dry,
>>>        unsigned int t;
>>>        int delay;
>>>
>>> -     if (xfer->delay_usecs) {
>>> -             delay = xfer->delay_usecs;
>>> -     } else {
>>> -             delay = spi_delay_to_ns(&xfer->delay, xfer);
>>> -             if (delay < 0)
>>> -                     return;
>>> -             delay /= 1000;
>>> -     }
>>> +     delay = spi_delay_to_ns(&xfer->delay, xfer);
>>> +     if (delay < 0)
>>> +             return;
>> Bit of a nit, but this could be `delay <= 0` and then drop the check for
>> `delay == 0` below.
> hmm, that's a bit debatable, because the `delay == 0` check comes
> after `delay /= 1000` ;
> to do what you're suggesting, it would probably need a
> DIV_ROUND_UP(delay, 1000) to make sure that even sub-microsecond
> delays don't become zero;

Ah, true. Lets keep the code as it is.

On the other hand you could argue that we should round up to ensure the 
delay is at least as long as requested. But that is something that 
should be changed independently from this series.

Patch
diff mbox series

diff --git a/drivers/spi/spi-axi-spi-engine.c b/drivers/spi/spi-axi-spi-engine.c
index af86e6d6e16b..80c3e38f5c1b 100644
--- a/drivers/spi/spi-axi-spi-engine.c
+++ b/drivers/spi/spi-axi-spi-engine.c
@@ -170,14 +170,10 @@  static void spi_engine_gen_sleep(struct spi_engine_program *p, bool dry,
 	unsigned int t;
 	int delay;
 
-	if (xfer->delay_usecs) {
-		delay = xfer->delay_usecs;
-	} else {
-		delay = spi_delay_to_ns(&xfer->delay, xfer);
-		if (delay < 0)
-			return;
-		delay /= 1000;
-	}
+	delay = spi_delay_to_ns(&xfer->delay, xfer);
+	if (delay < 0)
+		return;
+	delay /= 1000;
 
 	if (delay == 0)
 		return;