Re: [PATCH 3/3] gpio: syscon: Add support for the Xylon LogiCVC GPIOs

From: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
To: Linus Walleij <linus.walleij@linaro.org>
Cc: "open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
	"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
	<devicetree@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Bartosz Golaszewski <bgolaszewski@baylibre.com>,
	Rob Herring <robh+dt@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Subject: Re: [PATCH 3/3] gpio: syscon: Add support for the Xylon LogiCVC GPIOs
Date: Mon, 23 Sep 2019 15:33:25 +0200	[thread overview]
Message-ID: <20190923133325.GA57525@aptenodytes> (raw)
In-Reply-To: <CACRpkdY40PZc9R-yFwooR4-WMgn3LH7K+yTx00ZNxyq6OOnw6A@mail.gmail.com>

[-- Attachment #1: Type: text/plain, Size: 1836 bytes --]

Hi,

On Thu 12 Sep 19, 10:17, Linus Walleij wrote:
> On Tue, Sep 10, 2019 at 4:29 PM Paul Kocialkowski
> <paul.kocialkowski@bootlin.com> wrote:
> 
> > The LogiCVC display hardware block comes with GPIO capabilities
> > that must be exposed separately from the main driver (as GPIOs) for
> > use with regulators and panels. A syscon is used to share the same
> > regmap across the two drivers.
> >
> > Since the GPIO capabilities are pretty simple, add them to the syscon
> > GPIO driver.
> >
> > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> 
> I'm fine with this for now, but the gpio-syscon driver is now growing
> big and when you use it you are getting support for a whole bunch
> of systems you're not running on included in your binary.
> 
> We need to think about possibly creating drivers/gpio/syscon
> and split subdrivers into separate files and config options
> so that people can slim down to what they actually need.

Thanks for the review!

I understand your concern about more devices getting pulled-in and built
unconditionally. Though I do like the idea of having a single driver for only
exposing the GPIO part of bigger components instead of specific drivers with
< 100 lines of useful code.

Maybe a first step would be to introduce Kconfig options for each device and
ifdef around in the code, as to solve the "built unconditionally" aspect?

Either way, I'll send v2 still adding my driver to gpio-syscon but feel free to
have me in the loop when that driver needs to be changed.

> > +       *bit = 1 << offset;
> 
> Please do this:
> 
> #include <linux/bits.h>
> 
> *bit = BIT(offset);

Sure thing and sorry I missed that, thanks!

Cheers,

Paul

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]