Re: [PATCH 2/2] spi: spidev: fix a max speed setting

[Oleksandr Suvorov <oleksandr.suvorov-2KBjVHiyJgBBDgjK7y7TUQ@public.gmane.org>]

Hi Mark,

On Thu, Feb 20, 2020 at 8:42 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Thu, Feb 20, 2020 at 02:11:51PM +0000, Oleksandr Suvorov wrote:
> > SPI_IOC_WR_MAX_SPEED_HZ command always sets spi->max_speed_hz
> > the initial value come from DT.
>
> This is intentional.  It's doing a call to spi_setup() then restoring
> the original value, the goal being just to run spi_setup() with the new
> value - it's not really a good idea to change the maximum speed in the
> first place.

Now I see it.
>
> > It leads to set a wrong max speed with IOCTL call.
> In what way does it lead to the wrong speed being set?

After all, I reviewed the code and found out that the problem is not
in spidev_ioctl,
the problem is in spidev_message()'s debug message :)
The real code is ok:

drivers/spi/spidev.c: spidev_message():
...
                 k_tmp->speed_hz = u_tmp->speed_hz;
                 if (!k_tmp->speed_hz)
                        k_tmp->speed_hz = spidev->speed_hz;
...

but the debug message takes wrong value:
drivers/spi/spidev.c: spidev_message():
...
              dev_info(&spidev->spi->dev,
                      "  xfer len %u %s%s%s%dbits %u usec %uHz
(speed_hz=%d max_speed_hz=%d)\n",
                        u_tmp->len,
                        u_tmp->rx_buf ? "rx " : "",
                        u_tmp->tx_buf ? "tx " : "",
                        u_tmp->cs_change ? "cs " : "",
                        u_tmp->bits_per_word ? : spidev->spi->bits_per_word,
                        u_tmp->delay_usecs,
>>>                        u_tmp->speed_hz ? : spidev->spi->max_speed_hz);
...
It leads to debug messages like:

[ 1227.512806] spidev spi0.0: setup mode 0, 32 bits/w, 1000000 Hz max --> 0
[ 1227.541749] spidev spi0.0:   xfer len 4096 tx 32bits 0 usec 10000000Hz
..
[ 1227.616165] spidev spi0.0: setup mode 0, 32 bits/w, 2000000 Hz max --> 0
[ 1227.645095] spidev spi0.0:   xfer len 4096 tx 32bits 0 usec 10000000Hz
...
[ 1227.702714] spidev spi0.0: setup mode 0, 32 bits/w, 20000000 Hz max --> 0
[ 1227.731801] spidev spi0.0:   xfer len 4096 tx 32bits 0 usec 10000000Hz
...
So if one passes the message (using ioctl instead of write to fd) with
empty speed_hz,
the debug message tells the wrong real speed. It forced me to think in
the wrong direction.

> > Fix the logic of a max speed assignment.
>
> If the expectation is that the default speed should be changed for the
> device this should be handled at the spidev level rather than in the
> core.

Agree. I fixed the wrong place :)
I'll replace this patch with better one.

Thanx!

-- 
Best regards
Oleksandr Suvorov

Toradex AG
Altsagenstrasse 5 | 6048 Horw/Luzern | Switzerland | T: +41 41 500
4800 (main line)