linux-spi.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Oleksandr Suvorov <oleksandr.suvorov-2KBjVHiyJgBBDgjK7y7TUQ@public.gmane.org>
To: Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: "linux-spi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org"
	<linux-spi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>,
	"linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org"
	<linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>,
	Marcel Ziswiler
	<marcel.ziswiler-2KBjVHiyJgBBDgjK7y7TUQ@public.gmane.org>,
	Igor Opanyuk
	<igor.opanyuk-2KBjVHiyJgBBDgjK7y7TUQ@public.gmane.org>,
	Philippe Schenker
	<philippe.schenker-2KBjVHiyJgBBDgjK7y7TUQ@public.gmane.org>
Subject: Re: [PATCH 2/2] spi: spidev: fix a max speed setting
Date: Fri, 21 Feb 2020 12:43:20 +0000	[thread overview]
Message-ID: <CAGgjyvGQ3edNcBUUpcBmTaaNcYA3yvx19m+6MVyKMWgWPVabKQ@mail.gmail.com> (raw)
In-Reply-To: <20200220184201.GE3926-GFdadSzt00ze9xe1eoZjHA@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)

  parent reply	other threads:[~2020-02-21 12:43 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-20 14:11 [PATCH 0/2] trivial fixes for fsl-spi and spidev Oleksandr Suvorov
2020-02-20 14:11 ` [PATCH 2/2] spi: spidev: fix a max speed setting Oleksandr Suvorov
     [not found]   ` <20200220141143.3902922-3-oleksandr.suvorov-2KBjVHiyJgBBDgjK7y7TUQ@public.gmane.org>
2020-02-20 18:42     ` Mark Brown
     [not found]       ` <20200220184201.GE3926-GFdadSzt00ze9xe1eoZjHA@public.gmane.org>
2020-02-21 12:43         ` Oleksandr Suvorov [this message]
     [not found] ` <20200220141143.3902922-1-oleksandr.suvorov-2KBjVHiyJgBBDgjK7y7TUQ@public.gmane.org>
2020-02-20 14:11   ` [PATCH 1/2] spi: fsl-lpspi: remove unneeded array Oleksandr Suvorov
     [not found]     ` <20200220141143.3902922-2-oleksandr.suvorov-2KBjVHiyJgBBDgjK7y7TUQ@public.gmane.org>
2020-03-03 17:08       ` Applied "spi: fsl-lpspi: remove unneeded array" to the spi tree Mark Brown
2020-02-20 16:47   ` [PATCH 0/2] trivial fixes for fsl-spi and spidev Mark Brown

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAGgjyvGQ3edNcBUUpcBmTaaNcYA3yvx19m+6MVyKMWgWPVabKQ@mail.gmail.com \
    --to=oleksandr.suvorov-2kbjvhiyjgbbdgjk7y7tuq@public.gmane.org \
    --cc=broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org \
    --cc=igor.opanyuk-2KBjVHiyJgBBDgjK7y7TUQ@public.gmane.org \
    --cc=linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
    --cc=linux-spi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
    --cc=marcel.ziswiler-2KBjVHiyJgBBDgjK7y7TUQ@public.gmane.org \
    --cc=philippe.schenker-2KBjVHiyJgBBDgjK7y7TUQ@public.gmane.org \
    --subject='Re: [PATCH 2/2] spi: spidev: fix a max speed setting' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).