Re: [PATCH 3/4] gpio: sifive: Add GPIO driver for SiFive SoCs

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
To: Linus Walleij <linus.walleij@linaro.org>
Cc: "mark.rutland@arm.com" <mark.rutland@arm.com>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"aou@eecs.berkeley.edu" <aou@eecs.berkeley.edu>,
	"jason@lakedaemon.net" <jason@lakedaemon.net>,
	"linux-gpio@vger.kernel.org" <linux-gpio@vger.kernel.org>,
	"maz@kernel.org" <maz@kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"atish.patra@wdc.com" <atish.patra@wdc.com>,
	Yash Shah <yash.shah@sifive.com>,
	"robh+dt@kernel.org" <robh+dt@kernel.org>,
	"palmer@dabbelt.com" <palmer@dabbelt.com>,
	Sagar Kadam <sagar.kadam@sifive.com>,
	"Paul Walmsley \( Sifive\)" <paul.walmsley@sifive.com>,
	"tglx@linutronix.de" <tglx@linutronix.de>,
	"bmeng.cn@gmail.com" <bmeng.cn@gmail.com>,
	"linux-riscv@lists.infradead.org"
	<linux-riscv@lists.infradead.org>,
	Sachin Ghadi <sachin.ghadi@sifive.com>
Subject: Re: [PATCH 3/4] gpio: sifive: Add GPIO driver for SiFive SoCs
Date: Tue, 19 Nov 2019 17:41:51 +0100	[thread overview]
Message-ID: <CAMpxmJXFK4VLgJU+P0ZMNkduGfFxAeQ_NguRHtedf7cRPav7LQ@mail.gmail.com> (raw)
In-Reply-To: <CACRpkdb9KKPsu7dkjVmHbgQcdo1Zx9uC_jtd6HFwM+RO2EA4nw@mail.gmail.com>

wt., 19 lis 2019 o 16:03 Linus Walleij <linus.walleij@linaro.org> napisał(a):
>
> On Mon, Nov 18, 2019 at 11:15 AM Bartosz Golaszewski
> <bgolaszewski@baylibre.com> wrote:
> > pon., 18 lis 2019 o 11:03 Yash Shah <yash.shah@sifive.com> napisał(a):
>
> > > As suggested in the comments received on the RFC version of this patch[0], I am trying to use regmap MMIO by looking at gpio-mvebu.c. I got your point regarding the usage of own locks is not making any sense.
> > > Here is what I will do in v2:
> > > 1. drop the usage of own locks
> > > 2. consistently use regmap_* apis for register access (replace all iowrites).
> > > Does this make sense now?
> >
> > The thing is: the gpio-mmio code you're (correctly) reusing uses a
> > different lock - namely: bgpio_lock in struct gpio_chip. If you want
> > to use regmap for register operations, then you need to set
> > disable_locking in regmap_config to true and then take this lock
> > manually on every access.
>
> Is it really so? The bgpio_lock does protect the registers used
> by regmap-mmio but unless the interrupt code is also using the
> same registers it is fine to have a different lock for those.
>
> Is the interrupt code really poking into the very same registers
> as passed to bgpio_init()?
>
> Of course it could be seen as a bit dirty to poke around in the
> same memory space with regmap and the bgpio_* accessors
> but in practice it's no problem if they never touch the same
> things.
>
> Yours,
> Linus Walleij

I'm wondering if it won't cause any inconsistencies when for example
interrupts are being triggered on input lines while we're also reading
their values? Seems to me it's just more clear to use a single lock
for a register range. Most drivers using gpio-mmio do just that in
their irq-related routines.

Anyway: even without using bgpio_lock this code is inconsistent: if
we're using regmap for interrupt registers, we should either decide to
rely on locking provided by regmap or disable it and use a locally
defined lock. Also: if we're using regmap, then let's use it
everywhere, not only when it's convenient for updating registers.

Bart

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv