Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1566679 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH RESEND] pinctrl: intel: Turn Baytrail support to tristate Jean Delvare <jdelvare@suse.de> - 2017-01-25 16:30 +0100
    Re: [PATCH RESEND] pinctrl: intel: Turn Baytrail support to tristate Mika Westerberg <mika.westerberg@linux.intel.com> - 2017-01-25 17:20 +0100
      Re: [PATCH RESEND] pinctrl: intel: Turn Baytrail support to  tristate Jean Delvare <jdelvare@suse.de> - 2017-01-26 10:10 +0100
        Re: [PATCH RESEND] pinctrl: intel: Turn Baytrail support to tristate Mika Westerberg <mika.westerberg@linux.intel.com> - 2017-01-26 10:30 +0100
          Re: [PATCH RESEND] pinctrl: intel: Turn Baytrail support to  tristate Jean Delvare <jdelvare@suse.de> - 2017-01-27 10:20 +0100
    Re: [PATCH RESEND] pinctrl: intel: Turn Baytrail support to tristate Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-01-25 17:20 +0100
    Re: [PATCH RESEND] pinctrl: intel: Turn Baytrail support to tristate Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2017-01-26 09:40 +0100
    Re: [PATCH RESEND] pinctrl: intel: Turn Baytrail support to tristate Linus Walleij <linus.walleij@linaro.org> - 2017-01-26 10:00 +0100
      Re: [PATCH RESEND] pinctrl: intel: Turn Baytrail support to tristate Mika Westerberg <mika.westerberg@linux.intel.com> - 2017-01-26 10:20 +0100
        Re: [PATCH RESEND] pinctrl: intel: Turn Baytrail support to tristate Takashi Iwai <tiwai@suse.de> - 2017-01-26 10:30 +0100
          Re: [PATCH RESEND] pinctrl: intel: Turn Baytrail support to tristate Mika Westerberg <mika.westerberg@linux.intel.com> - 2017-01-26 10:40 +0100

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

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

[toc] | [next] | [standalone]

#1566720

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>

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1567219

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.

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1566724

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

[toc] | [prev] | [next] | [standalone]

#1567180

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

[toc] | [prev] | [next] | [standalone]

#1567187

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

[toc] | [prev] | [next] | [standalone]

#1567208

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.

[toc] | [prev] | [next] | [standalone]

#1567216

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

[toc] | [prev] | [next] | [standalone]

#1567223

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.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web