Re: [PATCH v3 12/16] pinctrl: starfive: Add pinctrl driver for StarFive SoCs

From: Andy Shevchenko <andy.shevchenko@gmail.com>
To: Emil Renner Berthing <kernel@esmil.dk>
Cc: Linus Walleij <linus.walleij@linaro.org>,
	linux-riscv <linux-riscv@lists.infradead.org>,
	devicetree <devicetree@vger.kernel.org>,
	linux-clk <linux-clk@vger.kernel.org>,
	"open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
	"open list:SERIAL DRIVERS" <linux-serial@vger.kernel.org>,
	Palmer Dabbelt <palmer@dabbelt.com>,
	Paul Walmsley <paul.walmsley@sifive.com>,
	Rob Herring <robh+dt@kernel.org>,
	Michael Turquette <mturquette@baylibre.com>,
	Stephen Boyd <sboyd@kernel.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Marc Zyngier <maz@kernel.org>,
	Philipp Zabel <p.zabel@pengutronix.de>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Daniel Lezcano <daniel.lezcano@linaro.org>,
	Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
	Jiri Slaby <jirislaby@kernel.org>,
	Maximilian Luz <luzmaximilian@gmail.com>,
	Sagar Kadam <sagar.kadam@sifive.com>,
	Drew Fustini <drew@beagleboard.org>,
	Geert Uytterhoeven <geert@linux-m68k.org>,
	Michael Zhu <michael.zhu@starfivetech.com>,
	Fu Wei <tekkamanninja@gmail.com>, Anup Patel <anup.patel@wdc.com>,
	Atish Patra <atish.patra@wdc.com>,
	Matteo Croce <mcroce@microsoft.com>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Huan Feng <huan.feng@starfivetech.com>
Subject: Re: [PATCH v3 12/16] pinctrl: starfive: Add pinctrl driver for StarFive SoCs
Date: Tue, 9 Nov 2021 11:33:18 +0200	[thread overview]
Message-ID: <CAHp75VdBaKZVeA7dasHWP4E3c8F2phaGz-90FErj3bB8FJOS9w@mail.gmail.com> (raw)
In-Reply-To: <CANBLGcyo3YjygkjDmdjt4C_H=MZdHQwqumsxnatuObeP2LADAg@mail.gmail.com>

On Tue, Nov 9, 2021 at 11:21 AM Emil Renner Berthing <kernel@esmil.dk> wrote:
> On Tue, 9 Nov 2021 at 02:01, Linus Walleij <linus.walleij@linaro.org> wrote:
> > On Tue, Nov 2, 2021 at 9:08 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:

...

> > > Linus any comments on this code (sorry if I missed your reply)? The
> > > idea behind above is to skip all settings from the same category and
> > > apply only the last one, e.g. if we have "bias set to X", ..., "bias
> > > disable", ..., "bias set to Y", the hardware will see only the last
> > > operation, i.e. "bias set to Y". I think it may not be the best
> > > approach (theoretically?) since the hardware definitely may behave
> > > differently on the other side in case of such series of the
> > > configurations (yes, I have seen some interesting implementations of
> > > the touchpad / touchscreen GPIOs that may be affected).
> >
> > That sounds weird. I think we need to look at how other drivers
> > deal with this.
> >
> > To me it seems more natural that
> > starfive_padctl_rmw(sfp, group->pins[i], mask, value);
> > would get called at the end of each iteration of the
> > for (i = 0; i < num_configs; i++) loop.
>
> That would work, but when the loop is done the end result would be
> exactly the same.

It seems we interpret the term "result" differently. The result when
we talking about GPIOs is the series of pin state changes incl.
configuration. This is how it should be recognized when programming
hardware.

>  The only difference is that the above would rapidly
> "blink" the different states during the loop until it arrives at the
> result. This would certainly be different, but it can never be the
> intended behaviour and only a side-effect on how the pinctrl framework
> works.

Is it? That's what I'm trying to get an answer to. If you may
guarantee this (the keywords "intended behaviour" and "side effect"),
I wouldn't object.

> The order the different states are blinked depends entirely on
> how the pinctrl framework parses the device tree. I still think it
> would be more natural to cleanly go to the end result without this
> blinking.

-- 
With Best Regards,
Andy Shevchenko