Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [BUG] SPI broken for SPI based panel drivers
Mime-Version: 1.0 (Mac OS X Mail 9.3 \(3124\))
Content-Type: text/plain; charset=us-ascii
From:   "H. Nikolaus Schaller" <hns@goldelico.com>
In-Reply-To: <CAGngYiVd6d2pb3eOd5ZiCvTBOzZK6+dudRUVWj8neBRK0S42Pg@mail.gmail.com>
Date:   Tue, 1 Dec 2020 17:46:56 +0100
Cc:     Linus Walleij <linus.walleij@linaro.org>,
        Mark Brown <broonie@kernel.org>,
        kernel list <linux-kernel@vger.kernel.org>,
        Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
        Discussions about the Letux Kernel 
        <letux-kernel@openphoenux.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <FF708A45-7325-49D5-B41E-7D54942F5E23@goldelico.com>
References: <2D7916FA-678F-4236-B478-C953CADF2FFA@goldelico.com> <CAGngYiXgc_m2A7Wihxuhzm-u4qH-JZgxHjke653zvyT45jMU7Q@mail.gmail.com> <4AC29229-9542-4E77-B993-217E29C7E209@goldelico.com> <20201201121620.GB5239@sirena.org.uk> <A499CCB9-F2EC-4F24-AA79-5A7FA6A092A9@goldelico.com> <CACRpkdYf2dUF6PjYcvnsKDPoxXPWiWKKAqpik4-2AAQjRmatfw@mail.gmail.com> <6283C16F-549C-4463-BC08-E2C1A1D78B2F@goldelico.com> <CAGngYiVd6d2pb3eOd5ZiCvTBOzZK6+dudRUVWj8neBRK0S42Pg@mail.gmail.com>
To:     Sven Van Asbroeck <thesven73@gmail.com>
Precedence: bulk


> Am 01.12.2020 um 16:52 schrieb Sven Van Asbroeck =
<thesven73@gmail.com>:
>=20
> Nikolaus,
>=20
> On Tue, Dec 1, 2020 at 9:38 AM H. Nikolaus Schaller =
<hns@goldelico.com> wrote:
>>=20
>> Let's work on a fix for the fix now.
>=20
> I tested spi-gpio on my system, by converting a built-in or hardware =
spi,
> to a spi-gpio. Interestingly, the patch has the opposite effect on my =
system:
> before the patch, spi-gpio did not work, but after it's applied, it =
does work.
>=20
> Can you tell me the idle status of your chip-select gpio in debugfs?
> # mount -t debugfs none /sys/kernel/debug
> # cat /sys/kernel/debug/gpio
> Look for something like this:
> gpiochip0: GPIOs 0-31, parent: platform/209c000.gpio, 209c000.gpio:
> gpio-17  (                    |spi5 CS0            ) out hi ACTIVE LOW

root@letux:~# mount -t debugfs none /sys/kernel/debug
root@letux:~# cat /sys/kernel/debug/gpio|fgrep spi
 gpio-179 (                    |spi4 CS0            ) out lo=20
root@letux:~#=20

That is after the panel driver did send the commands.

>=20
> Also, apply the following patch, and tell me
> a) does this dev_err() get called on your system, and

yes. Many times.=20

> b) what is the value when your chip is de-selected

root@letux:~# dmesg|fgrep td028
[   14.530456] panel-tpo-td028ttec1 spi4.0: spi->mode =3D 00000003
[   14.599212] panel-tpo-td028ttec1 spi4.0: gpiod disable
[   14.817871] panel-tpo-td028ttec1 spi4.0: spi->mode =3D 00000003
[   14.817871] panel-tpo-td028ttec1 spi4.0: gpiod enable

So it is disabled first and then enabled. Which appears to be the =
opposite
of what it should be.

BTW: I have added another dev_err to print the spi->mode and like
you describe it is (overwritten? by SPI_MODE_3.

I can check what value it has in the driver before it is set to =
SPI_MODE_3.

Maybe, there the spi-cs-high gets lost?

BR and thanks,
Nikolaus


> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index 7e8804b02be9..b2f4cf5c9ffb 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -813,11 +813,12 @@ static void spi_set_cs(struct spi_device *spi,
> bool enable)
>=20
>        if (spi->cs_gpiod || gpio_is_valid(spi->cs_gpio)) {
>                if (!(spi->mode & SPI_NO_CS)) {
> -                       if (spi->cs_gpiod)
> +                       if (spi->cs_gpiod) {
> +                               dev_err(&spi->dev, "gpiod %s", enable1
> ? "enable" : "disable");
>                                /* polarity handled by gpiolib */
>                                gpiod_set_value_cansleep(spi->cs_gpiod,
>                                                         enable1);
> -                       else
> +                       } else
>                                /*
>                                 * invert the enable line, as active =
low is
>                                 * default for SPI.