Re: [PATCH 0/5] pinctrl_gpio_get_direction & ingenic fixes

From: Paul Cercueil <paul@crapouillou.net>
To: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Linus Walleij <linus.walleij@linaro.org>,
	"open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 0/5] pinctrl_gpio_get_direction & ingenic fixes
Date: Thu, 28 Jun 2018 21:11:06 +0200	[thread overview]
Message-ID: <1530213066.3755.0@smtp.crapouillou.net> (raw)
In-Reply-To: <CAHp75Vc3QTYKp6oS6kW6EezsBbiLf9E=NBErHMbpxRU9EXMxWg@mail.gmail.com>

Hi Andy,

Le mer. 27 juin 2018 à 19:18, Andy Shevchenko 
<andy.shevchenko@gmail.com> a écrit :
> On Wed, Jun 27, 2018 at 2:48 PM, Paul Cercueil <paul@crapouillou.net> 
> wrote:
>>  Hi Linus,
>> 
>>  Here's a set of (rather RFC) patches, to implement
>>  pinctrl_gpio_get_direction(). I did that, because my gpio-ingenic 
>> driver
>>  calls pinctrl_gpio_set_direction() within its gpio_chip's 
>> .set_direction
>>  callback, but there was no corresponding function to implement the
>>  .get_direction callback. If that's not the right way to do it, 
>> please
>>  advise.
>> 
>>  If not merging the whole series, patch [3/5] is a real fix that 
>> should
>>  go through.
>> 
>>  Note that it doesn't make checkpatch.pl happy, I wasn't sure 
>> whether I
>>  should try to comply to checkpatch.pl or match the coding style in 
>> the
>>  pinctrl subsystem, I chose the latter.
> 
> I dunno what Linus would going to say about this, but I would like to
> see a schematics for this piece of IP.

ftp://ftp.ingenic.com/SOC/JZ4780/JZ4780_pm.pdf

> Even if GPIO and pin muxing has only one set of buffers to indicate
> input or output (same registers in use) it's a GPIO driver business to
> get direction from GPIO part of IP.

If I follow that logic it's also a GPIO driver business to set the 
direction
of a GPIO, right? Truth is the pinctrl subsystem takes care of that. So 
why
have "set direction" and no "get direction"?

> Looking into the existing code I would rather say that
> pinctrl-ingenic.c should incorporate gpio-ingenic.c as they are
> (partially) sharing same registers.
> To ->get_direction() implementation it's pretty straight forward, just
> read necessary registers in the gpio-ingenic.c directly. No need to
> have pin control or pin muxing to be involved.

Sure, it'd be pretty straightforward to do it from the GPIO driver, but 
I'd
still like to hear Linus' point of view about this.

As for merging pinctrl-ingenic.c and gpio-ingenic.c... I wouldn't 
disagree more,
even if they share registers, they belong to different subsystems. 
Besides,
your platform might need the pinctrl driver but not the GPIO one, or 
you might
want to provide the GPIO driver as a loadable module, etc.

Regards,
-Paul