Re: [RFC/PATCH] gpio/pinctrl: baytrail: move gpio driver from pinctrl to gpio directory

[David Cohen]

Hi Mika,

Thanks for your feedback. See below my reply.

On Wed, Oct 15, 2014 at 10:08:12AM +0300, Mika Westerberg wrote:
> On Tue, Oct 14, 2014 at 10:45:35AM -0700, David Cohen wrote:
> > Hi Mathias,
> > 
> > On Tue, Oct 14, 2014 at 01:35:43PM +0300, Mathias Nyman wrote:
> > > On 13.10.2014 22:17, David Cohen wrote:
> > > > Even though GPIO module on Intel Bay Trail is able to control pin
> > > > functionality, it's unlikely Linux kernel driver will ever support it
> > > > since BIOS should handle all pin muxing itself.
> > > > 
> > > > Currently this driver does not register any pinctrl interface and
> > > > doesn't call any pinctrl interface. It just uses on internal functions
> > > > the 'struct pinctrl_gpio_range', which is a weak justification to not be
> > > > under gpio directory.
> > > > 
> > > 
> > > This discussion was held when gpio-baytrail was first submitted.
> > > These threads explain the gpio/pinctrl-baytrail history:
> > > 
> > > http://marc.info/?l=linux-kernel&m=136981432427668&w=2
> > > http://marc.info/?l=linux-kernel&m=137113578604763&w=2
> > > http://marc.info/?l=linux-kernel&m=137155497023054&w=2
> > 
> > Thanks for pointing that out.
> > 
> > > 
> > > A proper pinctrl driver for baytrail is still not yet ruled out
> > 
> > Having it inside pinctrl directory is creating some confusion because
> > ppl expect it to implement the actual pinctrl interface.
> 
> A typical pinctrl driver can implement both a pinctrl interface and a
> GPIO interface. So it is not uncommon to look the GPIO drivers under
> drivers/pinctrl/*.

Agreed :)
But pinctrl-baytrail has no pinctrl interface, just gpio.

> 
> Furthermore the driver announces that it is a GPIO driver in its Kconfig
> entry:
> 
> config PINCTRL_BAYTRAIL
>         bool "Intel Baytrail GPIO pin control"
> 
> so I don't quite get why this would confuse people.

How come? You just wrote the inconsistence :)
We're enabling PINCTRL_BAYTRAIL to build a gpio only driver.

> 
> We are also planning to add more Intel pinctrl (real) drivers in the
> future. drivers/pinctrl/intel/* should be the place where people find
> the pinctrl/GPIO drivers for newer Intel hardware.

I fully agree pinctrl/gpio drivers should stay under drivers/pinctrl.
But we're talking about a gpio driver. IMHO we should at least mention
in the driver it lacks pinctrl interface currently but it will come in
future.

A side discussion would be why a distro would want to have the pinctrl
interface on Bay Trail. I'd assume any driver being the consumer of it
is likely to be wrong. When Linux boots, all pins' functions should be
already set.

> 
> > Anyway, the threads above are over 1 year old. Perhaps it was making
> > sense during that time, but unless somebody is working on pinctrl
> > interface now, IMHO we're misplacing the driver for too long. It'd
> > better to move this driver to gpio directory and when/if pinctrl
> > interface is implemented, we move it back to current place.
> 
> I disagree.
> 
> What happens when people and distros have CONFIG_PINCTRL_BAYTRAIL=y and
> now the symbol is changed to something else? For one, it will break the
> existing working configs.

Well, it would happen the same situation when CONFIG_PINCTRL_BAYTRAIL
was implemented for the first time. I don't think we should move/rename
is pinctrl interface is being implemented in short or middle term. But
my point is whether it will be implemented at all for the reasons I
mentioned above.

Anyway, I sent this patch as RFC due to I was expecting it to not be a
consensus. But guess the majority prefer to let it stay where it is.

Br, David
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/