[PATCH RESEND] pinctrl: intel: Turn Baytrail support to tristate

[PATCH RESEND] pinctrl: intel: Turn Baytrail support to tristate

[Jean Delvare]

The pinctrl-baytrail driver builds just fine as a module so give
users this option.

Signed-off-by: Jean Delvare <jdelvare@suse.de>
Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
---
This was discussed almost one year ago, with no clear conclusion, but
also no evidence that the driver can't be built as a module. Is there
any way to push this forward?

 drivers/pinctrl/intel/Kconfig |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- linux-4.7-rc7.orig/drivers/pinctrl/intel/Kconfig	2016-07-12 14:35:36.024835842 +0200
+++ linux-4.7-rc7/drivers/pinctrl/intel/Kconfig	2016-07-12 14:35:44.735904433 +0200
@@ -3,7 +3,7 @@
 #
 
 config PINCTRL_BAYTRAIL
-	bool "Intel Baytrail GPIO pin control"
+	tristate "Intel Baytrail GPIO pin control"
 	depends on GPIOLIB && ACPI
 	select GPIOLIB_IRQCHIP
 	select PINMUX


-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH RESEND] pinctrl: intel: Turn Baytrail support to tristate

[Andy Shevchenko]

On Wed, Jan 25, 2017 at 5:26 PM, Jean Delvare <jdelvare@suse.de> wrote:
> The pinctrl-baytrail driver builds just fine as a module so give
> users this option.
>
> Signed-off-by: Jean Delvare <jdelvare@suse.de>
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>

Looks okay to me.
FWIW:
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> ---
> This was discussed almost one year ago, with no clear conclusion, but
> also no evidence that the driver can't be built as a module. Is there
> any way to push this forward?
>
>  drivers/pinctrl/intel/Kconfig |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> --- linux-4.7-rc7.orig/drivers/pinctrl/intel/Kconfig    2016-07-12 14:35:36.024835842 +0200
> +++ linux-4.7-rc7/drivers/pinctrl/intel/Kconfig 2016-07-12 14:35:44.735904433 +0200
> @@ -3,7 +3,7 @@
>  #
>
>  config PINCTRL_BAYTRAIL
> -       bool "Intel Baytrail GPIO pin control"
> +       tristate "Intel Baytrail GPIO pin control"
>         depends on GPIOLIB && ACPI
>         select GPIOLIB_IRQCHIP
>         select PINMUX
>
>
> --
> Jean Delvare
> SUSE L3 Support



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH RESEND] pinctrl: intel: Turn Baytrail support to tristate

[Mika Westerberg]

On Wed, Jan 25, 2017 at 04:26:08PM +0100, Jean Delvare wrote:
> The pinctrl-baytrail driver builds just fine as a module so give
> users this option.
> 
> Signed-off-by: Jean Delvare <jdelvare@suse.de>
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>

Assuming you have checked that nothing breaks in Baytrail, I'm fine with
this change,

Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [PATCH RESEND] pinctrl: intel: Turn Baytrail support to tristate

[Jean Delvare]

Hi Mika and all,

On Wed, 25 Jan 2017 18:16:51 +0200, Mika Westerberg wrote:
> On Wed, Jan 25, 2017 at 04:26:08PM +0100, Jean Delvare wrote:
> > The pinctrl-baytrail driver builds just fine as a module so give
> > users this option.
> > 
> > Signed-off-by: Jean Delvare <jdelvare@suse.de>
> > Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> 
> Assuming you have checked that nothing breaks in Baytrail, I'm fine with
> this change,

I have not, as I do not have access to any Baytrail hardware. This is
the very reason why I'd like this code to be buildable as a module: I'm
not happy with a useless 50 kB driver being loaded on all my systems. I
was hopping someone at Intel would have access to the hardware to
perform the test.

Meanwhile I have found that my patch is not good, because since I wrote
it, Paul Gortmaker remove the module glue code from the driver itself:

commit 360943a8d26265825025b88da32961bd9ad4f7c6
Author: Paul Gortmaker <paul.gortmaker@windriver.com>
Date:   Mon Jun 6 22:43:01 2016 -0400

    pinctrl: baytrail: make it explicitly non-modular

This would have to be reverted first. Paul, do I understand it
correctly that the commit above was generated automatically and was not
specific to the pinctrl-baytrail driver?

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH RESEND] pinctrl: intel: Turn Baytrail support to tristate

[Mika Westerberg]

On Thu, Jan 26, 2017 at 10:05:06AM +0100, Jean Delvare wrote:
> Hi Mika and all,
> 
> On Wed, 25 Jan 2017 18:16:51 +0200, Mika Westerberg wrote:
> > On Wed, Jan 25, 2017 at 04:26:08PM +0100, Jean Delvare wrote:
> > > The pinctrl-baytrail driver builds just fine as a module so give
> > > users this option.
> > > 
> > > Signed-off-by: Jean Delvare <jdelvare@suse.de>
> > > Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> > 
> > Assuming you have checked that nothing breaks in Baytrail, I'm fine with
> > this change,
> 
> I have not, as I do not have access to any Baytrail hardware. This is
> the very reason why I'd like this code to be buildable as a module: I'm
> not happy with a useless 50 kB driver being loaded on all my systems. I
> was hopping someone at Intel would have access to the hardware to
> perform the test.

We do not have all the possible hardware here. I have one Minnowboard
MAX which I can test this on (like I commented in the other email) but
it does not have any ACPI GPIO OpRegion stuff that is present in many
Baytrail based laptops out there.

Re: [PATCH RESEND] pinctrl: intel: Turn Baytrail support to tristate

[Jean Delvare]

On Thu, 26 Jan 2017 11:29:11 +0200, Mika Westerberg wrote:
> On Thu, Jan 26, 2017 at 10:05:06AM +0100, Jean Delvare wrote:
> > I have not, as I do not have access to any Baytrail hardware. This is
> > the very reason why I'd like this code to be buildable as a module: I'm
> > not happy with a useless 50 kB driver being loaded on all my systems. I
> > was hopping someone at Intel would have access to the hardware to
> > perform the test.
> 
> We do not have all the possible hardware here. I have one Minnowboard
> MAX which I can test this on (like I commented in the other email) but
> it does not have any ACPI GPIO OpRegion stuff that is present in many
> Baytrail based laptops out there.

Takashi, do you have any such laptop or know anyone at SUSE who does
and could test?

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH RESEND] pinctrl: intel: Turn Baytrail support to tristate

[Heikki Krogerus]

On Wed, Jan 25, 2017 at 04:26:08PM +0100, Jean Delvare wrote:
> The pinctrl-baytrail driver builds just fine as a module so give
> users this option.
> 
> Signed-off-by: Jean Delvare <jdelvare@suse.de>
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>

OK by me. FWIW.

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>


Thanks,

-- 
heikki

Re: [PATCH RESEND] pinctrl: intel: Turn Baytrail support to tristate

[Linus Walleij]

On Wed, Jan 25, 2017 at 4:26 PM, Jean Delvare <jdelvare@suse.de> wrote:

> The pinctrl-baytrail driver builds just fine as a module so give
> users this option.
>
> Signed-off-by: Jean Delvare <jdelvare@suse.de>
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> ---
> This was discussed almost one year ago, with no clear conclusion, but
> also no evidence that the driver can't be built as a module. Is there
> any way to push this forward?

I see ACKs for this patch, but in my git I also have:

commit 360943a8d26265825025b88da32961bd9ad4f7c6
pinctrl: baytrail: make it explicitly non-modular

Acked by Mika.

So which one is it going to be?

If this should be applied, the previous patch from Paul Gortmaker
should be reverted first. Especially the runtime PM parts seem
important to get back.

Then I want a patch reverting that and adding this tristate in one.

Yours,
Linus Walleij

Re: [PATCH RESEND] pinctrl: intel: Turn Baytrail support to tristate

[Mika Westerberg]

On Thu, Jan 26, 2017 at 09:55:36AM +0100, Linus Walleij wrote:
> On Wed, Jan 25, 2017 at 4:26 PM, Jean Delvare <jdelvare@suse.de> wrote:
> 
> > The pinctrl-baytrail driver builds just fine as a module so give
> > users this option.
> >
> > Signed-off-by: Jean Delvare <jdelvare@suse.de>
> > Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> > Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > Cc: Linus Walleij <linus.walleij@linaro.org>
> > ---
> > This was discussed almost one year ago, with no clear conclusion, but
> > also no evidence that the driver can't be built as a module. Is there
> > any way to push this forward?
> 
> I see ACKs for this patch, but in my git I also have:
> 
> commit 360943a8d26265825025b88da32961bd9ad4f7c6
> pinctrl: baytrail: make it explicitly non-modular
> 
> Acked by Mika.

Heh, yeah we even removed possibility to unbind the driver with that
commit. Totally forgot that one.

> So which one is it going to be?

Good question. I'm fine with both but I would really like to get some
confirmation that turning the driver to module actually does not break
anything.

I have one Minnowboard MAX here but it does not do any ACPI magic for
GPIOs so testing on that one might not catch all possible issues.

> If this should be applied, the previous patch from Paul Gortmaker
> should be reverted first. Especially the runtime PM parts seem
> important to get back.

Runtime PM actually does not do anything - there is no way to power down
the GPIO controller in Baytrail.

> Then I want a patch reverting that and adding this tristate in one.

I agree + really good explanation in the changelog why this was done.

Re: [PATCH RESEND] pinctrl: intel: Turn Baytrail support to tristate

[Takashi Iwai]

On Thu, 26 Jan 2017 10:19:31 +0100,
Mika Westerberg wrote:
> 
> On Thu, Jan 26, 2017 at 09:55:36AM +0100, Linus Walleij wrote:
> > On Wed, Jan 25, 2017 at 4:26 PM, Jean Delvare <jdelvare@suse.de> wrote:
> > 
> > > The pinctrl-baytrail driver builds just fine as a module so give
> > > users this option.
> > >
> > > Signed-off-by: Jean Delvare <jdelvare@suse.de>
> > > Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > Cc: Linus Walleij <linus.walleij@linaro.org>
> > > ---
> > > This was discussed almost one year ago, with no clear conclusion, but
> > > also no evidence that the driver can't be built as a module. Is there
> > > any way to push this forward?
> > 
> > I see ACKs for this patch, but in my git I also have:
> > 
> > commit 360943a8d26265825025b88da32961bd9ad4f7c6
> > pinctrl: baytrail: make it explicitly non-modular
> > 
> > Acked by Mika.
> 
> Heh, yeah we even removed possibility to unbind the driver with that
> commit. Totally forgot that one.
> 
> > So which one is it going to be?
> 
> Good question. I'm fine with both but I would really like to get some
> confirmation that turning the driver to module actually does not break
> anything.

I guess it would break things on some machines if the module loading
order isn't setup properly.  For example, it's known that
pinctrl-cherrytrail breaks MMC or others if it's loaded too lately.
On distros, we often work around it by a specific module loading order
in initrd.

But this doesn't mean that the modularization itself is wrong.  It's
merely a setup issue.


thanks,

Takashi

Re: [PATCH RESEND] pinctrl: intel: Turn Baytrail support to tristate

[Mika Westerberg]

On Thu, Jan 26, 2017 at 10:26:56AM +0100, Takashi Iwai wrote:
> I guess it would break things on some machines if the module loading
> order isn't setup properly.  For example, it's known that
> pinctrl-cherrytrail breaks MMC or others if it's loaded too lately.

Yes, if you have rootfs on eMMC and it requires a GPIO then of course
you need to make sure your initrd includes the necessary drivers. Same
goes with other media that can be used as rootfs.