[PATCH] pinctrl: baytrail: show output gpio state correctly on Intel Baytrail

[PATCH] pinctrl: baytrail: show output gpio state correctly on Intel Baytrail

[David Cohen]

Even if a gpio pin is set to output, we still need to set INPUT_EN bit
to be able to read its value. Without this change, we'll always read low
level state.

Cc: <stable@vger.kernel.org> # v3.14+
Signed-off-by: David Cohen <david.a.cohen@linux.intel.com>
---

Hi,

I'm resending same v1 patch but now copying linux stable and linux gpio.
This patch is meant for all linux stable trees >= v3.14.

Br, David
---

 drivers/pinctrl/pinctrl-baytrail.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pinctrl/pinctrl-baytrail.c b/drivers/pinctrl/pinctrl-baytrail.c
index e12e5b07f6d7..c23d8ded936d 100644
--- a/drivers/pinctrl/pinctrl-baytrail.c
+++ b/drivers/pinctrl/pinctrl-baytrail.c
@@ -318,7 +318,7 @@ static int byt_gpio_direction_output(struct gpio_chip *chip,
 		"Potential Error: Setting GPIO with direct_irq_en to output");
 
 	reg_val = readl(reg) | BYT_DIR_MASK;
-	reg_val &= ~BYT_OUTPUT_EN;
+	reg_val &= ~(BYT_OUTPUT_EN | BYT_INPUT_EN);
 
 	if (value)
 		writel(reg_val | BYT_LEVEL, reg);
-- 
2.1.0

Re: [PATCH] pinctrl: baytrail: show output gpio state correctly on Intel Baytrail

[Felipe Balbi]

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

On Mon, Oct 13, 2014 at 11:23:59AM -0700, David Cohen wrote:
> Even if a gpio pin is set to output, we still need to set INPUT_EN bit

here you say you're setting that bit.

> to be able to read its value. Without this change, we'll always read low
> level state.
> 
> Cc: <stable@vger.kernel.org> # v3.14+
> Signed-off-by: David Cohen <david.a.cohen@linux.intel.com>
> ---
> 
> Hi,
> 
> I'm resending same v1 patch but now copying linux stable and linux gpio.
> This patch is meant for all linux stable trees >= v3.14.
> 
> Br, David
> ---
> 
>  drivers/pinctrl/pinctrl-baytrail.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-baytrail.c b/drivers/pinctrl/pinctrl-baytrail.c
> index e12e5b07f6d7..c23d8ded936d 100644
> --- a/drivers/pinctrl/pinctrl-baytrail.c
> +++ b/drivers/pinctrl/pinctrl-baytrail.c
> @@ -318,7 +318,7 @@ static int byt_gpio_direction_output(struct gpio_chip *chip,
>  		"Potential Error: Setting GPIO with direct_irq_en to output");
>  
>  	reg_val = readl(reg) | BYT_DIR_MASK;
> -	reg_val &= ~BYT_OUTPUT_EN;
> +	reg_val &= ~(BYT_OUTPUT_EN | BYT_INPUT_EN);

but code is clearing it. Also, you're not patching byt_gpio_get(), so
you can't be 'reading its value' either. Quite frankly, this looks very
fishy to me. Comments on BYT_OUTPUT_EN and BYT_INPUT_EN states that
those bits are active low, meaning that clearing them enables that
particular feature.

How can you enable a pin to be both OUTPUT and INPUT ? Look like what
you really want is:

diff --git a/drivers/pinctrl/pinctrl-baytrail.c b/drivers/pinctrl/pinctrl-baytrail.c
index e12e5b0..69f882d 100644
--- a/drivers/pinctrl/pinctrl-baytrail.c
+++ b/drivers/pinctrl/pinctrl-baytrail.c
@@ -290,6 +290,7 @@ static int byt_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
 
 	value = readl(reg) | BYT_DIR_MASK;
 	value &= ~BYT_INPUT_EN;		/* active low */
+	value |= BYT_OUTPUT_EN;		/* disable output */
 	writel(value, reg);
 
 	spin_unlock_irqrestore(&vg->lock, flags);
@@ -319,6 +320,7 @@ static int byt_gpio_direction_output(struct gpio_chip *chip,
 
 	reg_val = readl(reg) | BYT_DIR_MASK;
 	reg_val &= ~BYT_OUTPUT_EN;
+	value |= BYT_INPUT_EN;		/* disable input */
 
 	if (value)
 		writel(reg_val | BYT_LEVEL, reg);

completely untested however. Care to further explain why clearing both
bits - thus enabling both INPUT and OUTPUT - is correct ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] pinctrl: baytrail: show output gpio state correctly on Intel Baytrail

[David Cohen]

On Mon, Oct 13, 2014 at 02:14:05PM -0500, Felipe Balbi wrote:
> On Mon, Oct 13, 2014 at 11:23:59AM -0700, David Cohen wrote:
> > Even if a gpio pin is set to output, we still need to set INPUT_EN bit
> 
> here you say you're setting that bit.
> 
> > to be able to read its value. Without this change, we'll always read low
> > level state.
> > 
> > Cc: <stable@vger.kernel.org> # v3.14+
> > Signed-off-by: David Cohen <david.a.cohen@linux.intel.com>
> > ---
> > 
> > Hi,
> > 
> > I'm resending same v1 patch but now copying linux stable and linux gpio.
> > This patch is meant for all linux stable trees >= v3.14.
> > 
> > Br, David
> > ---
> > 
> >  drivers/pinctrl/pinctrl-baytrail.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pinctrl/pinctrl-baytrail.c b/drivers/pinctrl/pinctrl-baytrail.c
> > index e12e5b07f6d7..c23d8ded936d 100644
> > --- a/drivers/pinctrl/pinctrl-baytrail.c
> > +++ b/drivers/pinctrl/pinctrl-baytrail.c
> > @@ -318,7 +318,7 @@ static int byt_gpio_direction_output(struct gpio_chip *chip,
> >  		"Potential Error: Setting GPIO with direct_irq_en to output");
> >  
> >  	reg_val = readl(reg) | BYT_DIR_MASK;
> > -	reg_val &= ~BYT_OUTPUT_EN;
> > +	reg_val &= ~(BYT_OUTPUT_EN | BYT_INPUT_EN);
> 
> but code is clearing it. Also, you're not patching byt_gpio_get(), so
> you can't be 'reading its value' either. Quite frankly, this looks very
> fishy to me. Comments on BYT_OUTPUT_EN and BYT_INPUT_EN states that
> those bits are active low, meaning that clearing them enables that
> particular feature.
> 
> How can you enable a pin to be both OUTPUT and INPUT ? Look like what
> you really want is:
> 
> diff --git a/drivers/pinctrl/pinctrl-baytrail.c b/drivers/pinctrl/pinctrl-baytrail.c
> index e12e5b0..69f882d 100644
> --- a/drivers/pinctrl/pinctrl-baytrail.c
> +++ b/drivers/pinctrl/pinctrl-baytrail.c
> @@ -290,6 +290,7 @@ static int byt_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
>  
>  	value = readl(reg) | BYT_DIR_MASK;
>  	value &= ~BYT_INPUT_EN;		/* active low */
> +	value |= BYT_OUTPUT_EN;		/* disable output */
>  	writel(value, reg);
>  
>  	spin_unlock_irqrestore(&vg->lock, flags);
> @@ -319,6 +320,7 @@ static int byt_gpio_direction_output(struct gpio_chip *chip,
>  
>  	reg_val = readl(reg) | BYT_DIR_MASK;
>  	reg_val &= ~BYT_OUTPUT_EN;
> +	value |= BYT_INPUT_EN;		/* disable input */
>  
>  	if (value)
>  		writel(reg_val | BYT_LEVEL, reg);
> 
> completely untested however. Care to further explain why clearing both
> bits - thus enabling both INPUT and OUTPUT - is correct ?

Actually I really meant what I sent, but perhaps I misspoke during
explanation. We need to clear the bit to enable the functionality.

!BYT_INPUT_EN allows us to read the value. !BYT_OUTPUT_EN allows us to
set a value. If we want to set a value (acting as output) and still be
able to read via sysfs, we need to set both functionality (i.e. clear
both bits).

I'll resend the patch with a better comment.

Br, David

> 
> -- 
> balbi

Re: [PATCH] pinctrl: baytrail: show output gpio state correctly on Intel Baytrail

[Felipe Balbi]

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

Hi,

On Mon, Oct 13, 2014 at 12:24:04PM -0700, David Cohen wrote:
> On Mon, Oct 13, 2014 at 02:14:05PM -0500, Felipe Balbi wrote:
> > On Mon, Oct 13, 2014 at 11:23:59AM -0700, David Cohen wrote:
> > > Even if a gpio pin is set to output, we still need to set INPUT_EN bit
> > 
> > here you say you're setting that bit.
> > 
> > > to be able to read its value. Without this change, we'll always read low
> > > level state.
> > > 
> > > Cc: <stable@vger.kernel.org> # v3.14+
> > > Signed-off-by: David Cohen <david.a.cohen@linux.intel.com>
> > > ---
> > > 
> > > Hi,
> > > 
> > > I'm resending same v1 patch but now copying linux stable and linux gpio.
> > > This patch is meant for all linux stable trees >= v3.14.
> > > 
> > > Br, David
> > > ---
> > > 
> > >  drivers/pinctrl/pinctrl-baytrail.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/pinctrl/pinctrl-baytrail.c b/drivers/pinctrl/pinctrl-baytrail.c
> > > index e12e5b07f6d7..c23d8ded936d 100644
> > > --- a/drivers/pinctrl/pinctrl-baytrail.c
> > > +++ b/drivers/pinctrl/pinctrl-baytrail.c
> > > @@ -318,7 +318,7 @@ static int byt_gpio_direction_output(struct gpio_chip *chip,
> > >  		"Potential Error: Setting GPIO with direct_irq_en to output");
> > >  
> > >  	reg_val = readl(reg) | BYT_DIR_MASK;
> > > -	reg_val &= ~BYT_OUTPUT_EN;
> > > +	reg_val &= ~(BYT_OUTPUT_EN | BYT_INPUT_EN);
> > 
> > but code is clearing it. Also, you're not patching byt_gpio_get(), so
> > you can't be 'reading its value' either. Quite frankly, this looks very
> > fishy to me. Comments on BYT_OUTPUT_EN and BYT_INPUT_EN states that
> > those bits are active low, meaning that clearing them enables that
> > particular feature.
> > 
> > How can you enable a pin to be both OUTPUT and INPUT ? Look like what
> > you really want is:
> > 
> > diff --git a/drivers/pinctrl/pinctrl-baytrail.c b/drivers/pinctrl/pinctrl-baytrail.c
> > index e12e5b0..69f882d 100644
> > --- a/drivers/pinctrl/pinctrl-baytrail.c
> > +++ b/drivers/pinctrl/pinctrl-baytrail.c
> > @@ -290,6 +290,7 @@ static int byt_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
> >  
> >  	value = readl(reg) | BYT_DIR_MASK;
> >  	value &= ~BYT_INPUT_EN;		/* active low */
> > +	value |= BYT_OUTPUT_EN;		/* disable output */
> >  	writel(value, reg);
> >  
> >  	spin_unlock_irqrestore(&vg->lock, flags);
> > @@ -319,6 +320,7 @@ static int byt_gpio_direction_output(struct gpio_chip *chip,
> >  
> >  	reg_val = readl(reg) | BYT_DIR_MASK;
> >  	reg_val &= ~BYT_OUTPUT_EN;
> > +	value |= BYT_INPUT_EN;		/* disable input */
> >  
> >  	if (value)
> >  		writel(reg_val | BYT_LEVEL, reg);
> > 
> > completely untested however. Care to further explain why clearing both
> > bits - thus enabling both INPUT and OUTPUT - is correct ?
> 
> Actually I really meant what I sent, but perhaps I misspoke during
> explanation. We need to clear the bit to enable the functionality.
> 
> !BYT_INPUT_EN allows us to read the value. !BYT_OUTPUT_EN allows us to
> set a value. If we want to set a value (acting as output) and still be
> able to read via sysfs, we need to set both functionality (i.e. clear
> both bits).
> 
> I'll resend the patch with a better comment.

Alright, just make sure that exposing a pin through sysfs and making it
output high, then later switching it to input works. It looks like
byt_gpio_direction_input() still needs to make sure OUTPUT is set.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] pinctrl: baytrail: show output gpio state correctly on Intel Baytrail

[Felipe Balbi]

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

On Mon, Oct 13, 2014 at 02:26:32PM -0500, Felipe Balbi wrote:
> Hi,
> 
> On Mon, Oct 13, 2014 at 12:24:04PM -0700, David Cohen wrote:
> > On Mon, Oct 13, 2014 at 02:14:05PM -0500, Felipe Balbi wrote:
> > > On Mon, Oct 13, 2014 at 11:23:59AM -0700, David Cohen wrote:
> > > > Even if a gpio pin is set to output, we still need to set INPUT_EN bit
> > > 
> > > here you say you're setting that bit.
> > > 
> > > > to be able to read its value. Without this change, we'll always read low
> > > > level state.
> > > > 
> > > > Cc: <stable@vger.kernel.org> # v3.14+
> > > > Signed-off-by: David Cohen <david.a.cohen@linux.intel.com>
> > > > ---
> > > > 
> > > > Hi,
> > > > 
> > > > I'm resending same v1 patch but now copying linux stable and linux gpio.
> > > > This patch is meant for all linux stable trees >= v3.14.
> > > > 
> > > > Br, David
> > > > ---
> > > > 
> > > >  drivers/pinctrl/pinctrl-baytrail.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/pinctrl/pinctrl-baytrail.c b/drivers/pinctrl/pinctrl-baytrail.c
> > > > index e12e5b07f6d7..c23d8ded936d 100644
> > > > --- a/drivers/pinctrl/pinctrl-baytrail.c
> > > > +++ b/drivers/pinctrl/pinctrl-baytrail.c
> > > > @@ -318,7 +318,7 @@ static int byt_gpio_direction_output(struct gpio_chip *chip,
> > > >  		"Potential Error: Setting GPIO with direct_irq_en to output");
> > > >  
> > > >  	reg_val = readl(reg) | BYT_DIR_MASK;
> > > > -	reg_val &= ~BYT_OUTPUT_EN;
> > > > +	reg_val &= ~(BYT_OUTPUT_EN | BYT_INPUT_EN);
> > > 
> > > but code is clearing it. Also, you're not patching byt_gpio_get(), so
> > > you can't be 'reading its value' either. Quite frankly, this looks very
> > > fishy to me. Comments on BYT_OUTPUT_EN and BYT_INPUT_EN states that
> > > those bits are active low, meaning that clearing them enables that
> > > particular feature.
> > > 
> > > How can you enable a pin to be both OUTPUT and INPUT ? Look like what
> > > you really want is:
> > > 
> > > diff --git a/drivers/pinctrl/pinctrl-baytrail.c b/drivers/pinctrl/pinctrl-baytrail.c
> > > index e12e5b0..69f882d 100644
> > > --- a/drivers/pinctrl/pinctrl-baytrail.c
> > > +++ b/drivers/pinctrl/pinctrl-baytrail.c
> > > @@ -290,6 +290,7 @@ static int byt_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
> > >  
> > >  	value = readl(reg) | BYT_DIR_MASK;
> > >  	value &= ~BYT_INPUT_EN;		/* active low */
> > > +	value |= BYT_OUTPUT_EN;		/* disable output */
> > >  	writel(value, reg);
> > >  
> > >  	spin_unlock_irqrestore(&vg->lock, flags);
> > > @@ -319,6 +320,7 @@ static int byt_gpio_direction_output(struct gpio_chip *chip,
> > >  
> > >  	reg_val = readl(reg) | BYT_DIR_MASK;
> > >  	reg_val &= ~BYT_OUTPUT_EN;
> > > +	value |= BYT_INPUT_EN;		/* disable input */
> > >  
> > >  	if (value)
> > >  		writel(reg_val | BYT_LEVEL, reg);
> > > 
> > > completely untested however. Care to further explain why clearing both
> > > bits - thus enabling both INPUT and OUTPUT - is correct ?
> > 
> > Actually I really meant what I sent, but perhaps I misspoke during
> > explanation. We need to clear the bit to enable the functionality.
> > 
> > !BYT_INPUT_EN allows us to read the value. !BYT_OUTPUT_EN allows us to
> > set a value. If we want to set a value (acting as output) and still be
> > able to read via sysfs, we need to set both functionality (i.e. clear
> > both bits).
> > 
> > I'll resend the patch with a better comment.
> 
> Alright, just make sure that exposing a pin through sysfs and making it
> output high, then later switching it to input works. It looks like
> byt_gpio_direction_input() still needs to make sure OUTPUT is set.

I also noticed that this is missing:

diff --git a/drivers/pinctrl/pinctrl-baytrail.c b/drivers/pinctrl/pinctrl-baytrail.c
index e12e5b0..7db5ab9 100644
--- a/drivers/pinctrl/pinctrl-baytrail.c
+++ b/drivers/pinctrl/pinctrl-baytrail.c
@@ -614,3 +614,9 @@ static int __init byt_gpio_init(void)
 }
 
 subsys_initcall(byt_gpio_init);
+
+static void __exit byt_gpio_exit(void)
+{
+	platform_driver_unregister(&byt_gpio_driver);
+}
+module_exit(byt_gpio_exit);

(also untested)

ps: is BayTrail documentation available publicly ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] pinctrl: baytrail: show output gpio state correctly on Intel Baytrail

[David Cohen]

On Mon, Oct 13, 2014 at 02:36:18PM -0500, Felipe Balbi wrote:
> On Mon, Oct 13, 2014 at 02:26:32PM -0500, Felipe Balbi wrote:
> > Hi,
> > 
> > On Mon, Oct 13, 2014 at 12:24:04PM -0700, David Cohen wrote:
> > > On Mon, Oct 13, 2014 at 02:14:05PM -0500, Felipe Balbi wrote:
> > > > On Mon, Oct 13, 2014 at 11:23:59AM -0700, David Cohen wrote:
> > > > > Even if a gpio pin is set to output, we still need to set INPUT_EN bit
> > > > 
> > > > here you say you're setting that bit.
> > > > 
> > > > > to be able to read its value. Without this change, we'll always read low
> > > > > level state.
> > > > > 
> > > > > Cc: <stable@vger.kernel.org> # v3.14+
> > > > > Signed-off-by: David Cohen <david.a.cohen@linux.intel.com>
> > > > > ---
> > > > > 
> > > > > Hi,
> > > > > 
> > > > > I'm resending same v1 patch but now copying linux stable and linux gpio.
> > > > > This patch is meant for all linux stable trees >= v3.14.
> > > > > 
> > > > > Br, David
> > > > > ---
> > > > > 
> > > > >  drivers/pinctrl/pinctrl-baytrail.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/pinctrl/pinctrl-baytrail.c b/drivers/pinctrl/pinctrl-baytrail.c
> > > > > index e12e5b07f6d7..c23d8ded936d 100644
> > > > > --- a/drivers/pinctrl/pinctrl-baytrail.c
> > > > > +++ b/drivers/pinctrl/pinctrl-baytrail.c
> > > > > @@ -318,7 +318,7 @@ static int byt_gpio_direction_output(struct gpio_chip *chip,
> > > > >  		"Potential Error: Setting GPIO with direct_irq_en to output");
> > > > >  
> > > > >  	reg_val = readl(reg) | BYT_DIR_MASK;
> > > > > -	reg_val &= ~BYT_OUTPUT_EN;
> > > > > +	reg_val &= ~(BYT_OUTPUT_EN | BYT_INPUT_EN);
> > > > 
> > > > but code is clearing it. Also, you're not patching byt_gpio_get(), so
> > > > you can't be 'reading its value' either. Quite frankly, this looks very
> > > > fishy to me. Comments on BYT_OUTPUT_EN and BYT_INPUT_EN states that
> > > > those bits are active low, meaning that clearing them enables that
> > > > particular feature.
> > > > 
> > > > How can you enable a pin to be both OUTPUT and INPUT ? Look like what
> > > > you really want is:
> > > > 
> > > > diff --git a/drivers/pinctrl/pinctrl-baytrail.c b/drivers/pinctrl/pinctrl-baytrail.c
> > > > index e12e5b0..69f882d 100644
> > > > --- a/drivers/pinctrl/pinctrl-baytrail.c
> > > > +++ b/drivers/pinctrl/pinctrl-baytrail.c
> > > > @@ -290,6 +290,7 @@ static int byt_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
> > > >  
> > > >  	value = readl(reg) | BYT_DIR_MASK;
> > > >  	value &= ~BYT_INPUT_EN;		/* active low */
> > > > +	value |= BYT_OUTPUT_EN;		/* disable output */
> > > >  	writel(value, reg);
> > > >  
> > > >  	spin_unlock_irqrestore(&vg->lock, flags);
> > > > @@ -319,6 +320,7 @@ static int byt_gpio_direction_output(struct gpio_chip *chip,
> > > >  
> > > >  	reg_val = readl(reg) | BYT_DIR_MASK;
> > > >  	reg_val &= ~BYT_OUTPUT_EN;
> > > > +	value |= BYT_INPUT_EN;		/* disable input */
> > > >  
> > > >  	if (value)
> > > >  		writel(reg_val | BYT_LEVEL, reg);
> > > > 
> > > > completely untested however. Care to further explain why clearing both
> > > > bits - thus enabling both INPUT and OUTPUT - is correct ?
> > > 
> > > Actually I really meant what I sent, but perhaps I misspoke during
> > > explanation. We need to clear the bit to enable the functionality.
> > > 
> > > !BYT_INPUT_EN allows us to read the value. !BYT_OUTPUT_EN allows us to
> > > set a value. If we want to set a value (acting as output) and still be
> > > able to read via sysfs, we need to set both functionality (i.e. clear
> > > both bits).
> > > 
> > > I'll resend the patch with a better comment.
> > 
> > Alright, just make sure that exposing a pin through sysfs and making it
> > output high, then later switching it to input works. It looks like
> > byt_gpio_direction_input() still needs to make sure OUTPUT is set.
> 
> I also noticed that this is missing:
> 
> diff --git a/drivers/pinctrl/pinctrl-baytrail.c b/drivers/pinctrl/pinctrl-baytrail.c
> index e12e5b0..7db5ab9 100644
> --- a/drivers/pinctrl/pinctrl-baytrail.c
> +++ b/drivers/pinctrl/pinctrl-baytrail.c
> @@ -614,3 +614,9 @@ static int __init byt_gpio_init(void)
>  }
>  
>  subsys_initcall(byt_gpio_init);
> +
> +static void __exit byt_gpio_exit(void)
> +{
> +	platform_driver_unregister(&byt_gpio_driver);
> +}
> +module_exit(byt_gpio_exit);
> 
> (also untested)

It sure does. Want to send a formal patch and I Ack?

> 
> ps: is BayTrail documentation available publicly ?

Check Minnowboard Max :) It's baytrail.

BR, David

> 
> -- 
> balbi

Re: [PATCH] pinctrl: baytrail: show output gpio state correctly on Intel Baytrail

[Linus Walleij]

On Mon, Oct 13, 2014 at 9:36 PM, Felipe Balbi <balbi@ti.com> wrote:
> On Mon, Oct 13, 2014 at 02:26:32PM -0500, Felipe Balbi wrote:

> I also noticed that this is missing:
>
> diff --git a/drivers/pinctrl/pinctrl-baytrail.c b/drivers/pinctrl/pinctrl-baytrail.c
> index e12e5b0..7db5ab9 100644
> --- a/drivers/pinctrl/pinctrl-baytrail.c
> +++ b/drivers/pinctrl/pinctrl-baytrail.c
> @@ -614,3 +614,9 @@ static int __init byt_gpio_init(void)
>  }
>
>  subsys_initcall(byt_gpio_init);
> +
> +static void __exit byt_gpio_exit(void)
> +{
> +       platform_driver_unregister(&byt_gpio_driver);
> +}
> +module_exit(byt_gpio_exit);

But the Baytrail driver is not a loadable module, it is bool:

config PINCTRL_BAYTRAIL
        bool "Intel Baytrail GPIO pin control"
        depends on GPIOLIB && ACPI && X86

(...)

So I guess it won't need handling for removal, as it can only
be compiled-in.

Yours,
Linus Walleij

Re: [PATCH] pinctrl: baytrail: show output gpio state correctly on Intel Baytrail

[Felipe Balbi]

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

On Tue, Oct 28, 2014 at 11:15:20AM +0100, Linus Walleij wrote:
> On Mon, Oct 13, 2014 at 9:36 PM, Felipe Balbi <balbi@ti.com> wrote:
> > On Mon, Oct 13, 2014 at 02:26:32PM -0500, Felipe Balbi wrote:
> 
> > I also noticed that this is missing:
> >
> > diff --git a/drivers/pinctrl/pinctrl-baytrail.c b/drivers/pinctrl/pinctrl-baytrail.c
> > index e12e5b0..7db5ab9 100644
> > --- a/drivers/pinctrl/pinctrl-baytrail.c
> > +++ b/drivers/pinctrl/pinctrl-baytrail.c
> > @@ -614,3 +614,9 @@ static int __init byt_gpio_init(void)
> >  }
> >
> >  subsys_initcall(byt_gpio_init);
> > +
> > +static void __exit byt_gpio_exit(void)
> > +{
> > +       platform_driver_unregister(&byt_gpio_driver);
> > +}
> > +module_exit(byt_gpio_exit);
> 
> But the Baytrail driver is not a loadable module, it is bool:
> 
> config PINCTRL_BAYTRAIL
>         bool "Intel Baytrail GPIO pin control"
>         depends on GPIOLIB && ACPI && X86
> 
> (...)
> 
> So I guess it won't need handling for removal, as it can only
> be compiled-in.

you can still unbind it through sysfs, right ? The thing also already
provides a ->remove() method anyway.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] pinctrl: baytrail: show output gpio state correctly on Intel Baytrail

[Linus Walleij]

On Tue, Oct 28, 2014 at 3:42 PM, Felipe Balbi <balbi@ti.com> wrote:
> On Tue, Oct 28, 2014 at 11:15:20AM +0100, Linus Walleij wrote:
>> On Mon, Oct 13, 2014 at 9:36 PM, Felipe Balbi <balbi@ti.com> wrote:
>> > On Mon, Oct 13, 2014 at 02:26:32PM -0500, Felipe Balbi wrote:
>>
>> > I also noticed that this is missing:
>> >
>> > diff --git a/drivers/pinctrl/pinctrl-baytrail.c b/drivers/pinctrl/pinctrl-baytrail.c
>> > index e12e5b0..7db5ab9 100644
>> > --- a/drivers/pinctrl/pinctrl-baytrail.c
>> > +++ b/drivers/pinctrl/pinctrl-baytrail.c
>> > @@ -614,3 +614,9 @@ static int __init byt_gpio_init(void)
>> >  }
>> >
>> >  subsys_initcall(byt_gpio_init);
>> > +
>> > +static void __exit byt_gpio_exit(void)
>> > +{
>> > +       platform_driver_unregister(&byt_gpio_driver);
>> > +}
>> > +module_exit(byt_gpio_exit);
>>
>> But the Baytrail driver is not a loadable module, it is bool:
>>
>> config PINCTRL_BAYTRAIL
>>         bool "Intel Baytrail GPIO pin control"
>>         depends on GPIOLIB && ACPI && X86
>>
>> (...)
>>
>> So I guess it won't need handling for removal, as it can only
>> be compiled-in.
>
> you can still unbind it through sysfs, right ? The thing also already
> provides a ->remove() method anyway.

Yes you're right of course...

But another way to get rid of the dilemma is to set
.suppress_bind_attrs = true on the .driver field of the
device driver. The one can't unbind it through sysfs anymore.

        .driver = {
                .name   = "foo",
                .suppress_bind_attrs = true,
        },

So one of them need to be done.

I suspect this is a kind of common problem...

Yours,
Linus Walleij

Re: [PATCH] pinctrl: baytrail: show output gpio state correctly on Intel Baytrail

[Felipe Balbi]

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

Hi,

On Fri, Oct 31, 2014 at 09:12:16AM +0100, Linus Walleij wrote:
> On Tue, Oct 28, 2014 at 3:42 PM, Felipe Balbi <balbi@ti.com> wrote:
> > On Tue, Oct 28, 2014 at 11:15:20AM +0100, Linus Walleij wrote:
> >> On Mon, Oct 13, 2014 at 9:36 PM, Felipe Balbi <balbi@ti.com> wrote:
> >> > On Mon, Oct 13, 2014 at 02:26:32PM -0500, Felipe Balbi wrote:
> >>
> >> > I also noticed that this is missing:
> >> >
> >> > diff --git a/drivers/pinctrl/pinctrl-baytrail.c b/drivers/pinctrl/pinctrl-baytrail.c
> >> > index e12e5b0..7db5ab9 100644
> >> > --- a/drivers/pinctrl/pinctrl-baytrail.c
> >> > +++ b/drivers/pinctrl/pinctrl-baytrail.c
> >> > @@ -614,3 +614,9 @@ static int __init byt_gpio_init(void)
> >> >  }
> >> >
> >> >  subsys_initcall(byt_gpio_init);
> >> > +
> >> > +static void __exit byt_gpio_exit(void)
> >> > +{
> >> > +       platform_driver_unregister(&byt_gpio_driver);
> >> > +}
> >> > +module_exit(byt_gpio_exit);
> >>
> >> But the Baytrail driver is not a loadable module, it is bool:
> >>
> >> config PINCTRL_BAYTRAIL
> >>         bool "Intel Baytrail GPIO pin control"
> >>         depends on GPIOLIB && ACPI && X86
> >>
> >> (...)
> >>
> >> So I guess it won't need handling for removal, as it can only
> >> be compiled-in.
> >
> > you can still unbind it through sysfs, right ? The thing also already
> > provides a ->remove() method anyway.
> 
> Yes you're right of course...
> 
> But another way to get rid of the dilemma is to set
> .suppress_bind_attrs = true on the .driver field of the
> device driver. The one can't unbind it through sysfs anymore.
> 
>         .driver = {
>                 .name   = "foo",
>                 .suppress_bind_attrs = true,
>         },
> 
> So one of them need to be done.
> 
> I suspect this is a kind of common problem...

so instead of taking of taking a three-liner which just makes sure this
can be used as "intended" you prefer to:

diff --git a/drivers/pinctrl/pinctrl-baytrail.c b/drivers/pinctrl/pinctrl-baytrail.c
index e12e5b0..254ba81 100644
--- a/drivers/pinctrl/pinctrl-baytrail.c
+++ b/drivers/pinctrl/pinctrl-baytrail.c
@@ -587,16 +587,6 @@ static const struct acpi_device_id byt_gpio_acpi_match[] = {
 };
 MODULE_DEVICE_TABLE(acpi, byt_gpio_acpi_match);
 
-static int byt_gpio_remove(struct platform_device *pdev)
-{
-	struct byt_gpio *vg = platform_get_drvdata(pdev);
-
-	pm_runtime_disable(&pdev->dev);
-	gpiochip_remove(&vg->chip);
-
-	return 0;
-}
-
 static struct platform_driver byt_gpio_driver = {
 	.probe          = byt_gpio_probe,
 	.remove         = byt_gpio_remove,
@@ -605,6 +595,7 @@ static struct platform_driver byt_gpio_driver = {
 		.owner  = THIS_MODULE,
 		.pm	= &byt_gpio_pm_ops,
 		.acpi_match_table = ACPI_PTR(byt_gpio_acpi_match),
+		.suppress_bind_attrs = true,
 	},
 };
 

I don't quite care since this is not an architecture I work for, but I
prefer drivers which can be unbound one way or another. Not to mention
that there's already a ->remove callback on the platform_driver anyway.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] pinctrl: baytrail: show output gpio state correctly on Intel Baytrail

[David Cohen]

On Fri, Oct 31, 2014 at 08:20:05AM -0500, Felipe Balbi wrote:
> Hi,
> 
> On Fri, Oct 31, 2014 at 09:12:16AM +0100, Linus Walleij wrote:
> > On Tue, Oct 28, 2014 at 3:42 PM, Felipe Balbi <balbi@ti.com> wrote:
> > > On Tue, Oct 28, 2014 at 11:15:20AM +0100, Linus Walleij wrote:
> > >> On Mon, Oct 13, 2014 at 9:36 PM, Felipe Balbi <balbi@ti.com> wrote:
> > >> > On Mon, Oct 13, 2014 at 02:26:32PM -0500, Felipe Balbi wrote:
> > >>
> > >> > I also noticed that this is missing:
> > >> >
> > >> > diff --git a/drivers/pinctrl/pinctrl-baytrail.c b/drivers/pinctrl/pinctrl-baytrail.c
> > >> > index e12e5b0..7db5ab9 100644
> > >> > --- a/drivers/pinctrl/pinctrl-baytrail.c
> > >> > +++ b/drivers/pinctrl/pinctrl-baytrail.c
> > >> > @@ -614,3 +614,9 @@ static int __init byt_gpio_init(void)
> > >> >  }
> > >> >
> > >> >  subsys_initcall(byt_gpio_init);
> > >> > +
> > >> > +static void __exit byt_gpio_exit(void)
> > >> > +{
> > >> > +       platform_driver_unregister(&byt_gpio_driver);
> > >> > +}
> > >> > +module_exit(byt_gpio_exit);
> > >>
> > >> But the Baytrail driver is not a loadable module, it is bool:
> > >>
> > >> config PINCTRL_BAYTRAIL
> > >>         bool "Intel Baytrail GPIO pin control"
> > >>         depends on GPIOLIB && ACPI && X86
> > >>
> > >> (...)
> > >>
> > >> So I guess it won't need handling for removal, as it can only
> > >> be compiled-in.
> > >
> > > you can still unbind it through sysfs, right ? The thing also already
> > > provides a ->remove() method anyway.
> > 
> > Yes you're right of course...
> > 
> > But another way to get rid of the dilemma is to set
> > .suppress_bind_attrs = true on the .driver field of the
> > device driver. The one can't unbind it through sysfs anymore.
> > 
> >         .driver = {
> >                 .name   = "foo",
> >                 .suppress_bind_attrs = true,
> >         },
> > 
> > So one of them need to be done.
> > 
> > I suspect this is a kind of common problem...
> 
> so instead of taking of taking a three-liner which just makes sure this
> can be used as "intended" you prefer to:
> 
> diff --git a/drivers/pinctrl/pinctrl-baytrail.c b/drivers/pinctrl/pinctrl-baytrail.c
> index e12e5b0..254ba81 100644
> --- a/drivers/pinctrl/pinctrl-baytrail.c
> +++ b/drivers/pinctrl/pinctrl-baytrail.c
> @@ -587,16 +587,6 @@ static const struct acpi_device_id byt_gpio_acpi_match[] = {
>  };
>  MODULE_DEVICE_TABLE(acpi, byt_gpio_acpi_match);
>  
> -static int byt_gpio_remove(struct platform_device *pdev)
> -{
> -	struct byt_gpio *vg = platform_get_drvdata(pdev);
> -
> -	pm_runtime_disable(&pdev->dev);
> -	gpiochip_remove(&vg->chip);
> -
> -	return 0;
> -}
> -
>  static struct platform_driver byt_gpio_driver = {
>  	.probe          = byt_gpio_probe,
>  	.remove         = byt_gpio_remove,
> @@ -605,6 +595,7 @@ static struct platform_driver byt_gpio_driver = {
>  		.owner  = THIS_MODULE,
>  		.pm	= &byt_gpio_pm_ops,
>  		.acpi_match_table = ACPI_PTR(byt_gpio_acpi_match),
> +		.suppress_bind_attrs = true,
>  	},
>  };
>  
> 
> I don't quite care since this is not an architecture I work for, but I
> prefer drivers which can be unbound one way or another. Not to mention
> that there's already a ->remove callback on the platform_driver anyway.

I think adding the module exit + allowing this driver to be a module
would be a good approach. Then we don't need to force generic x86 kernel
binaries to always have this driver. Unless Mathias or Mika knows a
constraint to force this driver to be builtin only.

Br, David

> 
> -- 
> balbi

Re: [PATCH] pinctrl: baytrail: show output gpio state correctly on Intel Baytrail

[David Cohen]

On Fri, Oct 31, 2014 at 09:23:39AM -0700, David Cohen wrote:
> On Fri, Oct 31, 2014 at 08:20:05AM -0500, Felipe Balbi wrote:
> > Hi,
> > 
> > On Fri, Oct 31, 2014 at 09:12:16AM +0100, Linus Walleij wrote:
> > > On Tue, Oct 28, 2014 at 3:42 PM, Felipe Balbi <balbi@ti.com> wrote:
> > > > On Tue, Oct 28, 2014 at 11:15:20AM +0100, Linus Walleij wrote:
> > > >> On Mon, Oct 13, 2014 at 9:36 PM, Felipe Balbi <balbi@ti.com> wrote:
> > > >> > On Mon, Oct 13, 2014 at 02:26:32PM -0500, Felipe Balbi wrote:
> > > >>
> > > >> > I also noticed that this is missing:
> > > >> >
> > > >> > diff --git a/drivers/pinctrl/pinctrl-baytrail.c b/drivers/pinctrl/pinctrl-baytrail.c
> > > >> > index e12e5b0..7db5ab9 100644
> > > >> > --- a/drivers/pinctrl/pinctrl-baytrail.c
> > > >> > +++ b/drivers/pinctrl/pinctrl-baytrail.c
> > > >> > @@ -614,3 +614,9 @@ static int __init byt_gpio_init(void)
> > > >> >  }
> > > >> >
> > > >> >  subsys_initcall(byt_gpio_init);
> > > >> > +
> > > >> > +static void __exit byt_gpio_exit(void)
> > > >> > +{
> > > >> > +       platform_driver_unregister(&byt_gpio_driver);
> > > >> > +}
> > > >> > +module_exit(byt_gpio_exit);
> > > >>
> > > >> But the Baytrail driver is not a loadable module, it is bool:
> > > >>
> > > >> config PINCTRL_BAYTRAIL
> > > >>         bool "Intel Baytrail GPIO pin control"
> > > >>         depends on GPIOLIB && ACPI && X86
> > > >>
> > > >> (...)
> > > >>
> > > >> So I guess it won't need handling for removal, as it can only
> > > >> be compiled-in.
> > > >
> > > > you can still unbind it through sysfs, right ? The thing also already
> > > > provides a ->remove() method anyway.
> > > 
> > > Yes you're right of course...
> > > 
> > > But another way to get rid of the dilemma is to set
> > > .suppress_bind_attrs = true on the .driver field of the
> > > device driver. The one can't unbind it through sysfs anymore.
> > > 
> > >         .driver = {
> > >                 .name   = "foo",
> > >                 .suppress_bind_attrs = true,
> > >         },
> > > 
> > > So one of them need to be done.
> > > 
> > > I suspect this is a kind of common problem...
> > 
> > so instead of taking of taking a three-liner which just makes sure this
> > can be used as "intended" you prefer to:
> > 
> > diff --git a/drivers/pinctrl/pinctrl-baytrail.c b/drivers/pinctrl/pinctrl-baytrail.c
> > index e12e5b0..254ba81 100644
> > --- a/drivers/pinctrl/pinctrl-baytrail.c
> > +++ b/drivers/pinctrl/pinctrl-baytrail.c
> > @@ -587,16 +587,6 @@ static const struct acpi_device_id byt_gpio_acpi_match[] = {
> >  };
> >  MODULE_DEVICE_TABLE(acpi, byt_gpio_acpi_match);
> >  
> > -static int byt_gpio_remove(struct platform_device *pdev)
> > -{
> > -	struct byt_gpio *vg = platform_get_drvdata(pdev);
> > -
> > -	pm_runtime_disable(&pdev->dev);
> > -	gpiochip_remove(&vg->chip);
> > -
> > -	return 0;
> > -}
> > -
> >  static struct platform_driver byt_gpio_driver = {
> >  	.probe          = byt_gpio_probe,
> >  	.remove         = byt_gpio_remove,
> > @@ -605,6 +595,7 @@ static struct platform_driver byt_gpio_driver = {
> >  		.owner  = THIS_MODULE,
> >  		.pm	= &byt_gpio_pm_ops,
> >  		.acpi_match_table = ACPI_PTR(byt_gpio_acpi_match),
> > +		.suppress_bind_attrs = true,
> >  	},
> >  };
> >  
> > 
> > I don't quite care since this is not an architecture I work for, but I
> > prefer drivers which can be unbound one way or another. Not to mention
> > that there's already a ->remove callback on the platform_driver anyway.
> 
> I think adding the module exit + allowing this driver to be a module
> would be a good approach. Then we don't need to force generic x86 kernel
> binaries to always have this driver. Unless Mathias or Mika knows a
> constraint to force this driver to be builtin only.

It helps if I CC them when asking for feedback :)

Mathias, Mika, do you know any constraint that forces pinctrl-baytrail
to be bool?

Br, David

> 
> Br, David
> 
> > 
> > -- 
> > balbi
> 
> 

Re: [PATCH] pinctrl: baytrail: show output gpio state correctly on Intel Baytrail

[Mika Westerberg]

On Fri, Oct 31, 2014 at 11:45:09AM -0700, David Cohen wrote:
> > I think adding the module exit + allowing this driver to be a module
> > would be a good approach. Then we don't need to force generic x86 kernel
> > binaries to always have this driver. Unless Mathias or Mika knows a
> > constraint to force this driver to be builtin only.
> 
> It helps if I CC them when asking for feedback :)
> 
> Mathias, Mika, do you know any constraint that forces pinctrl-baytrail
> to be bool?

The only constraint that has been keeping this driver as bool is that
some machines like, Asus T100, uses ACPI GPIO operation regions for
toggling GPIOs to get things like sensor hub powered on. The GPIO
operation region code does not yet handle -EPROBE_DEFER so only way to
ensure that the operation region is there is to have the driver compiled
in to the kernel.

Re: [PATCH] pinctrl: baytrail: show output gpio state correctly on Intel Baytrail

[Felipe Balbi]

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

On Mon, Nov 03, 2014 at 11:24:02AM +0200, Mika Westerberg wrote:
> On Fri, Oct 31, 2014 at 11:45:09AM -0700, David Cohen wrote:
> > > I think adding the module exit + allowing this driver to be a module
> > > would be a good approach. Then we don't need to force generic x86 kernel
> > > binaries to always have this driver. Unless Mathias or Mika knows a
> > > constraint to force this driver to be builtin only.
> > 
> > It helps if I CC them when asking for feedback :)
> > 
> > Mathias, Mika, do you know any constraint that forces pinctrl-baytrail
> > to be bool?
> 
> The only constraint that has been keeping this driver as bool is that
> some machines like, Asus T100, uses ACPI GPIO operation regions for
> toggling GPIOs to get things like sensor hub powered on. The GPIO
> operation region code does not yet handle -EPROBE_DEFER so only way to
> ensure that the operation region is there is to have the driver compiled
> in to the kernel.

But that's not enough excuse to have every single x86 in the market
shipping with this driver. Think about a distro kernel, most likely this
gets enabled and it's wrong in 80% of the cases.

It would be nicer to add EPROBE_DEFER support, convert this into
tristate and have default = M if BAYTRAIL, or something.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] pinctrl: baytrail: show output gpio state correctly on Intel Baytrail

[Mika Westerberg]

On Mon, Nov 03, 2014 at 09:00:48AM -0600, Felipe Balbi wrote:
> On Mon, Nov 03, 2014 at 11:24:02AM +0200, Mika Westerberg wrote:
> > On Fri, Oct 31, 2014 at 11:45:09AM -0700, David Cohen wrote:
> > > > I think adding the module exit + allowing this driver to be a module
> > > > would be a good approach. Then we don't need to force generic x86 kernel
> > > > binaries to always have this driver. Unless Mathias or Mika knows a
> > > > constraint to force this driver to be builtin only.
> > > 
> > > It helps if I CC them when asking for feedback :)
> > > 
> > > Mathias, Mika, do you know any constraint that forces pinctrl-baytrail
> > > to be bool?
> > 
> > The only constraint that has been keeping this driver as bool is that
> > some machines like, Asus T100, uses ACPI GPIO operation regions for
> > toggling GPIOs to get things like sensor hub powered on. The GPIO
> > operation region code does not yet handle -EPROBE_DEFER so only way to
> > ensure that the operation region is there is to have the driver compiled
> > in to the kernel.
> 
> But that's not enough excuse to have every single x86 in the market
> shipping with this driver. Think about a distro kernel, most likely this
> gets enabled and it's wrong in 80% of the cases.

True, but see below.

> It would be nicer to add EPROBE_DEFER support, convert this into
> tristate and have default = M if BAYTRAIL, or something.

If it were simple as that we would have done that already. Please check
drivers/gpio/gpiolib-acpi.c:acpi_gpio_adr_space_handler() and tell me
how we can do that.

The problem is that it is *firmware* code that decides to use the GPIO
at some random point in time and we have no way to tell it to retry
later when the GPIO is available.

Re: [PATCH] pinctrl: baytrail: show output gpio state correctly on Intel Baytrail

[Felipe Balbi]

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

Hi,

On Mon, Nov 03, 2014 at 05:27:43PM +0200, Mika Westerberg wrote:
> On Mon, Nov 03, 2014 at 09:00:48AM -0600, Felipe Balbi wrote:
> > On Mon, Nov 03, 2014 at 11:24:02AM +0200, Mika Westerberg wrote:
> > > On Fri, Oct 31, 2014 at 11:45:09AM -0700, David Cohen wrote:
> > > > > I think adding the module exit + allowing this driver to be a module
> > > > > would be a good approach. Then we don't need to force generic x86 kernel
> > > > > binaries to always have this driver. Unless Mathias or Mika knows a
> > > > > constraint to force this driver to be builtin only.
> > > > 
> > > > It helps if I CC them when asking for feedback :)
> > > > 
> > > > Mathias, Mika, do you know any constraint that forces pinctrl-baytrail
> > > > to be bool?
> > > 
> > > The only constraint that has been keeping this driver as bool is that
> > > some machines like, Asus T100, uses ACPI GPIO operation regions for
> > > toggling GPIOs to get things like sensor hub powered on. The GPIO
> > > operation region code does not yet handle -EPROBE_DEFER so only way to
> > > ensure that the operation region is there is to have the driver compiled
> > > in to the kernel.
> > 
> > But that's not enough excuse to have every single x86 in the market
> > shipping with this driver. Think about a distro kernel, most likely this
> > gets enabled and it's wrong in 80% of the cases.
> 
> True, but see below.
> 
> > It would be nicer to add EPROBE_DEFER support, convert this into
> > tristate and have default = M if BAYTRAIL, or something.
> 
> If it were simple as that we would have done that already. Please check
> drivers/gpio/gpiolib-acpi.c:acpi_gpio_adr_space_handler() and tell me
> how we can do that.
> 
> The problem is that it is *firmware* code that decides to use the GPIO
> at some random point in time and we have no way to tell it to retry
> later when the GPIO is available.

which means that even with the driver built-in, there is still the
possibility that firmware will try to access it before
pinctrl-baytrail's init function is called and you'd end up in the same
situation.

The fact is that currently you're forcing every x86 (even non-Intel) to
ship with this driver statically linked into it just because a small
percent of x86 systems might need to have this ready-to-go early enough.

Unfortunately I don't know ACPI enough to tell you if there is a way to
tell firmware "hey, you can use GPIOs now", so I'l refrain from
commenting on that. But that doesn't change the fact that this is wrong.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] pinctrl: baytrail: show output gpio state correctly on Intel Baytrail

[Mika Westerberg]

On Mon, Nov 03, 2014 at 05:27:43PM +0200, Mika Westerberg wrote:
> On Mon, Nov 03, 2014 at 09:00:48AM -0600, Felipe Balbi wrote:
> > On Mon, Nov 03, 2014 at 11:24:02AM +0200, Mika Westerberg wrote:
> > > On Fri, Oct 31, 2014 at 11:45:09AM -0700, David Cohen wrote:
> > > > > I think adding the module exit + allowing this driver to be a module
> > > > > would be a good approach. Then we don't need to force generic x86 kernel
> > > > > binaries to always have this driver. Unless Mathias or Mika knows a
> > > > > constraint to force this driver to be builtin only.
> > > > 
> > > > It helps if I CC them when asking for feedback :)
> > > > 
> > > > Mathias, Mika, do you know any constraint that forces pinctrl-baytrail
> > > > to be bool?
> > > 
> > > The only constraint that has been keeping this driver as bool is that
> > > some machines like, Asus T100, uses ACPI GPIO operation regions for
> > > toggling GPIOs to get things like sensor hub powered on. The GPIO
> > > operation region code does not yet handle -EPROBE_DEFER so only way to
> > > ensure that the operation region is there is to have the driver compiled
> > > in to the kernel.
> > 
> > But that's not enough excuse to have every single x86 in the market
> > shipping with this driver. Think about a distro kernel, most likely this
> > gets enabled and it's wrong in 80% of the cases.
> 
> True, but see below.
> 
> > It would be nicer to add EPROBE_DEFER support, convert this into
> > tristate and have default = M if BAYTRAIL, or something.
> 
> If it were simple as that we would have done that already. Please check
> drivers/gpio/gpiolib-acpi.c:acpi_gpio_adr_space_handler() and tell me
> how we can do that.

Actually the above is not the problem because we already have registered
the GPIO chip and hence we have the GPIO available to the firmware code.

The real problem is that if the ACPI GPIO operation handler is not there
at the time firmware decides to do something it will just skip things
that depend on the operation region. So if it has a GPIO that is used to
turn on sensor hub or touch panel or whatever, this will not be done and
it results that the device in question might not work properly.

Re: [PATCH] pinctrl: baytrail: show output gpio state correctly on Intel Baytrail

[Felipe Balbi]

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

Hi,

On Mon, Nov 03, 2014 at 05:42:07PM +0200, Mika Westerberg wrote:
> On Mon, Nov 03, 2014 at 05:27:43PM +0200, Mika Westerberg wrote:
> > On Mon, Nov 03, 2014 at 09:00:48AM -0600, Felipe Balbi wrote:
> > > On Mon, Nov 03, 2014 at 11:24:02AM +0200, Mika Westerberg wrote:
> > > > On Fri, Oct 31, 2014 at 11:45:09AM -0700, David Cohen wrote:
> > > > > > I think adding the module exit + allowing this driver to be a module
> > > > > > would be a good approach. Then we don't need to force generic x86 kernel
> > > > > > binaries to always have this driver. Unless Mathias or Mika knows a
> > > > > > constraint to force this driver to be builtin only.
> > > > > 
> > > > > It helps if I CC them when asking for feedback :)
> > > > > 
> > > > > Mathias, Mika, do you know any constraint that forces pinctrl-baytrail
> > > > > to be bool?
> > > > 
> > > > The only constraint that has been keeping this driver as bool is that
> > > > some machines like, Asus T100, uses ACPI GPIO operation regions for
> > > > toggling GPIOs to get things like sensor hub powered on. The GPIO
> > > > operation region code does not yet handle -EPROBE_DEFER so only way to
> > > > ensure that the operation region is there is to have the driver compiled
> > > > in to the kernel.
> > > 
> > > But that's not enough excuse to have every single x86 in the market
> > > shipping with this driver. Think about a distro kernel, most likely this
> > > gets enabled and it's wrong in 80% of the cases.
> > 
> > True, but see below.
> > 
> > > It would be nicer to add EPROBE_DEFER support, convert this into
> > > tristate and have default = M if BAYTRAIL, or something.
> > 
> > If it were simple as that we would have done that already. Please check
> > drivers/gpio/gpiolib-acpi.c:acpi_gpio_adr_space_handler() and tell me
> > how we can do that.
> 
> Actually the above is not the problem because we already have registered
> the GPIO chip and hence we have the GPIO available to the firmware code.

what happens before you registered the gpio chip ? It takes some time
from head.S to gpiochip_irqchip_add(). Anywhere between that time,
firmware could try to access gpios and the same problem would occur.

> The real problem is that if the ACPI GPIO operation handler is not there
> at the time firmware decides to do something it will just skip things
> that depend on the operation region. So if it has a GPIO that is used to
> turn on sensor hub or touch panel or whatever, this will not be done and
> it results that the device in question might not work properly.

that's an issue that needs solving, but forcing every x86 kernel to ship
with this driver, is not a proper solution.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] pinctrl: baytrail: show output gpio state correctly on Intel Baytrail

[Mika Westerberg]

On Mon, Nov 03, 2014 at 09:50:11AM -0600, Felipe Balbi wrote:
> Hi,
> 
> On Mon, Nov 03, 2014 at 05:42:07PM +0200, Mika Westerberg wrote:
> > On Mon, Nov 03, 2014 at 05:27:43PM +0200, Mika Westerberg wrote:
> > > On Mon, Nov 03, 2014 at 09:00:48AM -0600, Felipe Balbi wrote:
> > > > On Mon, Nov 03, 2014 at 11:24:02AM +0200, Mika Westerberg wrote:
> > > > > On Fri, Oct 31, 2014 at 11:45:09AM -0700, David Cohen wrote:
> > > > > > > I think adding the module exit + allowing this driver to be a module
> > > > > > > would be a good approach. Then we don't need to force generic x86 kernel
> > > > > > > binaries to always have this driver. Unless Mathias or Mika knows a
> > > > > > > constraint to force this driver to be builtin only.
> > > > > > 
> > > > > > It helps if I CC them when asking for feedback :)
> > > > > > 
> > > > > > Mathias, Mika, do you know any constraint that forces pinctrl-baytrail
> > > > > > to be bool?
> > > > > 
> > > > > The only constraint that has been keeping this driver as bool is that
> > > > > some machines like, Asus T100, uses ACPI GPIO operation regions for
> > > > > toggling GPIOs to get things like sensor hub powered on. The GPIO
> > > > > operation region code does not yet handle -EPROBE_DEFER so only way to
> > > > > ensure that the operation region is there is to have the driver compiled
> > > > > in to the kernel.
> > > > 
> > > > But that's not enough excuse to have every single x86 in the market
> > > > shipping with this driver. Think about a distro kernel, most likely this
> > > > gets enabled and it's wrong in 80% of the cases.
> > > 
> > > True, but see below.
> > > 
> > > > It would be nicer to add EPROBE_DEFER support, convert this into
> > > > tristate and have default = M if BAYTRAIL, or something.
> > > 
> > > If it were simple as that we would have done that already. Please check
> > > drivers/gpio/gpiolib-acpi.c:acpi_gpio_adr_space_handler() and tell me
> > > how we can do that.
> > 
> > Actually the above is not the problem because we already have registered
> > the GPIO chip and hence we have the GPIO available to the firmware code.
> 
> what happens before you registered the gpio chip ? It takes some time
> from head.S to gpiochip_irqchip_add(). Anywhere between that time,
> firmware could try to access gpios and the same problem would occur.

The operation region is not ready and the firmware does not try to use
it. However, the subsys_initcall() is there just to be sure that the
GPIO driver gets loaded before anything that is going to use GPIOs from
firmware.

> > The real problem is that if the ACPI GPIO operation handler is not there
> > at the time firmware decides to do something it will just skip things
> > that depend on the operation region. So if it has a GPIO that is used to
> > turn on sensor hub or touch panel or whatever, this will not be done and
> > it results that the device in question might not work properly.
> 
> that's an issue that needs solving, but forcing every x86 kernel to ship
> with this driver, is not a proper solution.

I would rather have the driver build in to the kernel now (and btw it
has been already in mainline quite some time so I suspect many distros
have already enabled it), than turning it module and render some devices
that have been working previously, fail suddenly.

There is a mechanism in ACPI to solve these issues, called _DEP, but it
is still very much work in progress.

Re: [PATCH] pinctrl: baytrail: show output gpio state correctly on Intel Baytrail

[Felipe Balbi]

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

Hi,

On Mon, Nov 03, 2014 at 08:42:47PM +0200, Mika Westerberg wrote:
> > > > > > > > I think adding the module exit + allowing this driver to be a module
> > > > > > > > would be a good approach. Then we don't need to force generic x86 kernel
> > > > > > > > binaries to always have this driver. Unless Mathias or Mika knows a
> > > > > > > > constraint to force this driver to be builtin only.
> > > > > > > 
> > > > > > > It helps if I CC them when asking for feedback :)
> > > > > > > 
> > > > > > > Mathias, Mika, do you know any constraint that forces pinctrl-baytrail
> > > > > > > to be bool?
> > > > > > 
> > > > > > The only constraint that has been keeping this driver as bool is that
> > > > > > some machines like, Asus T100, uses ACPI GPIO operation regions for
> > > > > > toggling GPIOs to get things like sensor hub powered on. The GPIO
> > > > > > operation region code does not yet handle -EPROBE_DEFER so only way to
> > > > > > ensure that the operation region is there is to have the driver compiled
> > > > > > in to the kernel.
> > > > > 
> > > > > But that's not enough excuse to have every single x86 in the market
> > > > > shipping with this driver. Think about a distro kernel, most likely this
> > > > > gets enabled and it's wrong in 80% of the cases.
> > > > 
> > > > True, but see below.
> > > > 
> > > > > It would be nicer to add EPROBE_DEFER support, convert this into
> > > > > tristate and have default = M if BAYTRAIL, or something.
> > > > 
> > > > If it were simple as that we would have done that already. Please check
> > > > drivers/gpio/gpiolib-acpi.c:acpi_gpio_adr_space_handler() and tell me
> > > > how we can do that.
> > > 
> > > Actually the above is not the problem because we already have registered
> > > the GPIO chip and hence we have the GPIO available to the firmware code.
> > 
> > what happens before you registered the gpio chip ? It takes some time
> > from head.S to gpiochip_irqchip_add(). Anywhere between that time,
> > firmware could try to access gpios and the same problem would occur.
> 
> The operation region is not ready and the firmware does not try to use
> it. However, the subsys_initcall() is there just to be sure that the
> GPIO driver gets loaded before anything that is going to use GPIOs from
> firmware.

alright, so how does the firmware know that the operation region is
ready and why can't that be deferred until pinctrl-baytrail (module or
built-in) has finished probing ? That would sort both issues, would it
not ?

> > > The real problem is that if the ACPI GPIO operation handler is not there
> > > at the time firmware decides to do something it will just skip things
> > > that depend on the operation region. So if it has a GPIO that is used to
> > > turn on sensor hub or touch panel or whatever, this will not be done and
> > > it results that the device in question might not work properly.
> > 
> > that's an issue that needs solving, but forcing every x86 kernel to ship
> > with this driver, is not a proper solution.
> 
> I would rather have the driver build in to the kernel now (and btw it
> has been already in mainline quite some time so I suspect many distros
> have already enabled it), than turning it module and render some devices
> that have been working previously, fail suddenly.

that's why I said you should default to Y if BAYTRAIL or make it
tristate and always default to M.

> There is a mechanism in ACPI to solve these issues, called _DEP, but it
> is still very much work in progress.

alright.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] pinctrl: baytrail: show output gpio state correctly on Intel Baytrail

[Mika Westerberg]

On Mon, Nov 03, 2014 at 02:40:38PM -0600, Felipe Balbi wrote:
> Hi,
> 
> On Mon, Nov 03, 2014 at 08:42:47PM +0200, Mika Westerberg wrote:
> > > > > > > > > I think adding the module exit + allowing this driver to be a module
> > > > > > > > > would be a good approach. Then we don't need to force generic x86 kernel
> > > > > > > > > binaries to always have this driver. Unless Mathias or Mika knows a
> > > > > > > > > constraint to force this driver to be builtin only.
> > > > > > > > 
> > > > > > > > It helps if I CC them when asking for feedback :)
> > > > > > > > 
> > > > > > > > Mathias, Mika, do you know any constraint that forces pinctrl-baytrail
> > > > > > > > to be bool?
> > > > > > > 
> > > > > > > The only constraint that has been keeping this driver as bool is that
> > > > > > > some machines like, Asus T100, uses ACPI GPIO operation regions for
> > > > > > > toggling GPIOs to get things like sensor hub powered on. The GPIO
> > > > > > > operation region code does not yet handle -EPROBE_DEFER so only way to
> > > > > > > ensure that the operation region is there is to have the driver compiled
> > > > > > > in to the kernel.
> > > > > > 
> > > > > > But that's not enough excuse to have every single x86 in the market
> > > > > > shipping with this driver. Think about a distro kernel, most likely this
> > > > > > gets enabled and it's wrong in 80% of the cases.
> > > > > 
> > > > > True, but see below.
> > > > > 
> > > > > > It would be nicer to add EPROBE_DEFER support, convert this into
> > > > > > tristate and have default = M if BAYTRAIL, or something.
> > > > > 
> > > > > If it were simple as that we would have done that already. Please check
> > > > > drivers/gpio/gpiolib-acpi.c:acpi_gpio_adr_space_handler() and tell me
> > > > > how we can do that.
> > > > 
> > > > Actually the above is not the problem because we already have registered
> > > > the GPIO chip and hence we have the GPIO available to the firmware code.
> > > 
> > > what happens before you registered the gpio chip ? It takes some time
> > > from head.S to gpiochip_irqchip_add(). Anywhere between that time,
> > > firmware could try to access gpios and the same problem would occur.
> > 
> > The operation region is not ready and the firmware does not try to use
> > it. However, the subsys_initcall() is there just to be sure that the
> > GPIO driver gets loaded before anything that is going to use GPIOs from
> > firmware.
> 
> alright, so how does the firmware know that the operation region is
> ready and why can't that be deferred until pinctrl-baytrail (module or
> built-in) has finished probing ? That would sort both issues, would it
> not ?

The firmware checks the dependent object for AVBL or similar variable.
If it is != 0 it assumes that the operation region is there. The problem
is how can you tell the firmware that it should somehow try again?

Here _DEP is the right solution as it forces the dependencies to be
loaded first.

> > > > The real problem is that if the ACPI GPIO operation handler is not there
> > > > at the time firmware decides to do something it will just skip things
> > > > that depend on the operation region. So if it has a GPIO that is used to
> > > > turn on sensor hub or touch panel or whatever, this will not be done and
> > > > it results that the device in question might not work properly.
> > > 
> > > that's an issue that needs solving, but forcing every x86 kernel to ship
> > > with this driver, is not a proper solution.
> > 
> > I would rather have the driver build in to the kernel now (and btw it
> > has been already in mainline quite some time so I suspect many distros
> > have already enabled it), than turning it module and render some devices
> > that have been working previously, fail suddenly.
> 
> that's why I said you should default to Y if BAYTRAIL or make it
> tristate and always default to M.

So you are saying it is better to add config option BAYTRAIL and then
make the driver Y? Well, now generic distros need to select that as Y
which doesn't solve anything.

Re: [PATCH] pinctrl: baytrail: show output gpio state correctly on Intel Baytrail

[Felipe Balbi]

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

Hi,

On Tue, Nov 04, 2014 at 09:51:35AM +0200, Mika Westerberg wrote:
> > > > > > > > > > I think adding the module exit + allowing this driver to be a module
> > > > > > > > > > would be a good approach. Then we don't need to force generic x86 kernel
> > > > > > > > > > binaries to always have this driver. Unless Mathias or Mika knows a
> > > > > > > > > > constraint to force this driver to be builtin only.
> > > > > > > > > 
> > > > > > > > > It helps if I CC them when asking for feedback :)
> > > > > > > > > 
> > > > > > > > > Mathias, Mika, do you know any constraint that forces pinctrl-baytrail
> > > > > > > > > to be bool?
> > > > > > > > 
> > > > > > > > The only constraint that has been keeping this driver as bool is that
> > > > > > > > some machines like, Asus T100, uses ACPI GPIO operation regions for
> > > > > > > > toggling GPIOs to get things like sensor hub powered on. The GPIO
> > > > > > > > operation region code does not yet handle -EPROBE_DEFER so only way to
> > > > > > > > ensure that the operation region is there is to have the driver compiled
> > > > > > > > in to the kernel.
> > > > > > > 
> > > > > > > But that's not enough excuse to have every single x86 in the market
> > > > > > > shipping with this driver. Think about a distro kernel, most likely this
> > > > > > > gets enabled and it's wrong in 80% of the cases.
> > > > > > 
> > > > > > True, but see below.
> > > > > > 
> > > > > > > It would be nicer to add EPROBE_DEFER support, convert this into
> > > > > > > tristate and have default = M if BAYTRAIL, or something.
> > > > > > 
> > > > > > If it were simple as that we would have done that already. Please check
> > > > > > drivers/gpio/gpiolib-acpi.c:acpi_gpio_adr_space_handler() and tell me
> > > > > > how we can do that.
> > > > > 
> > > > > Actually the above is not the problem because we already have registered
> > > > > the GPIO chip and hence we have the GPIO available to the firmware code.
> > > > 
> > > > what happens before you registered the gpio chip ? It takes some time
> > > > from head.S to gpiochip_irqchip_add(). Anywhere between that time,
> > > > firmware could try to access gpios and the same problem would occur.
> > > 
> > > The operation region is not ready and the firmware does not try to use
> > > it. However, the subsys_initcall() is there just to be sure that the
> > > GPIO driver gets loaded before anything that is going to use GPIOs from
> > > firmware.
> > 
> > alright, so how does the firmware know that the operation region is
> > ready and why can't that be deferred until pinctrl-baytrail (module or
> > built-in) has finished probing ? That would sort both issues, would it
> > not ?
> 
> The firmware checks the dependent object for AVBL or similar variable.
> If it is != 0 it assumes that the operation region is there. The problem
> is how can you tell the firmware that it should somehow try again?
> 
> Here _DEP is the right solution as it forces the dependencies to be
> loaded first.

alright.

> > > > > The real problem is that if the ACPI GPIO operation handler is not there
> > > > > at the time firmware decides to do something it will just skip things
> > > > > that depend on the operation region. So if it has a GPIO that is used to
> > > > > turn on sensor hub or touch panel or whatever, this will not be done and
> > > > > it results that the device in question might not work properly.
> > > > 
> > > > that's an issue that needs solving, but forcing every x86 kernel to ship
> > > > with this driver, is not a proper solution.
> > > 
> > > I would rather have the driver build in to the kernel now (and btw it
> > > has been already in mainline quite some time so I suspect many distros
> > > have already enabled it), than turning it module and render some devices
> > > that have been working previously, fail suddenly.
> > 
> > that's why I said you should default to Y if BAYTRAIL or make it
> > tristate and always default to M.
> 
> So you are saying it is better to add config option BAYTRAIL and then
> make the driver Y? Well, now generic distros need to select that as Y
> which doesn't solve anything.

ok, so there's no differentiation for when you're building a baytrail
kernel, that's fine. Then it gets really difficult to do anything. I
guess you really need to to wait for _DEP.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] pinctrl: baytrail: show output gpio state correctly on Intel Baytrail

[David Cohen]

Hi Mika,

Thanks for your feedbacks :)

On Mon, Nov 03, 2014 at 08:42:47PM +0200, Mika Westerberg wrote:
> On Mon, Nov 03, 2014 at 09:50:11AM -0600, Felipe Balbi wrote:
> > Hi,
> > 
> > On Mon, Nov 03, 2014 at 05:42:07PM +0200, Mika Westerberg wrote:
> > > On Mon, Nov 03, 2014 at 05:27:43PM +0200, Mika Westerberg wrote:
> > > > On Mon, Nov 03, 2014 at 09:00:48AM -0600, Felipe Balbi wrote:
> > > > > On Mon, Nov 03, 2014 at 11:24:02AM +0200, Mika Westerberg wrote:
> > > > > > On Fri, Oct 31, 2014 at 11:45:09AM -0700, David Cohen wrote:
> > > > > > > > I think adding the module exit + allowing this driver to be a module
> > > > > > > > would be a good approach. Then we don't need to force generic x86 kernel
> > > > > > > > binaries to always have this driver. Unless Mathias or Mika knows a
> > > > > > > > constraint to force this driver to be builtin only.
> > > > > > > 
> > > > > > > It helps if I CC them when asking for feedback :)
> > > > > > > 
> > > > > > > Mathias, Mika, do you know any constraint that forces pinctrl-baytrail
> > > > > > > to be bool?
> > > > > > 
> > > > > > The only constraint that has been keeping this driver as bool is that
> > > > > > some machines like, Asus T100, uses ACPI GPIO operation regions for
> > > > > > toggling GPIOs to get things like sensor hub powered on. The GPIO
> > > > > > operation region code does not yet handle -EPROBE_DEFER so only way to
> > > > > > ensure that the operation region is there is to have the driver compiled
> > > > > > in to the kernel.
> > > > > 
> > > > > But that's not enough excuse to have every single x86 in the market
> > > > > shipping with this driver. Think about a distro kernel, most likely this
> > > > > gets enabled and it's wrong in 80% of the cases.
> > > > 
> > > > True, but see below.
> > > > 
> > > > > It would be nicer to add EPROBE_DEFER support, convert this into
> > > > > tristate and have default = M if BAYTRAIL, or something.
> > > > 
> > > > If it were simple as that we would have done that already. Please check
> > > > drivers/gpio/gpiolib-acpi.c:acpi_gpio_adr_space_handler() and tell me
> > > > how we can do that.
> > > 
> > > Actually the above is not the problem because we already have registered
> > > the GPIO chip and hence we have the GPIO available to the firmware code.
> > 
> > what happens before you registered the gpio chip ? It takes some time
> > from head.S to gpiochip_irqchip_add(). Anywhere between that time,
> > firmware could try to access gpios and the same problem would occur.
> 
> The operation region is not ready and the firmware does not try to use
> it. However, the subsys_initcall() is there just to be sure that the
> GPIO driver gets loaded before anything that is going to use GPIOs from
> firmware.

That sounds hackish and dangerous. There are shared registers between
different GPIOs.

> 
> > > The real problem is that if the ACPI GPIO operation handler is not there
> > > at the time firmware decides to do something it will just skip things
> > > that depend on the operation region. So if it has a GPIO that is used to
> > > turn on sensor hub or touch panel or whatever, this will not be done and
> > > it results that the device in question might not work properly.
> > 
> > that's an issue that needs solving, but forcing every x86 kernel to ship
> > with this driver, is not a proper solution.
> 
> I would rather have the driver build in to the kernel now (and btw it
> has been already in mainline quite some time so I suspect many distros
> have already enabled it), than turning it module and render some devices
> that have been working previously, fail suddenly.

Hm.... it sounds like we're saying: we got it working somehow and from
now on we'll avoid change it to not create regression.

Correct if I'm wrong, but there's nothing really preventing this driver
to be a module beside the buggy ACPI interface that allows kernel and fw
to play freely (and silently) with same piece of hw.

IMHO we could allow this driver be M, but default to Y and an
explanation to set to Y in case FW needs to use GPIO before module gets
loaded. Since we've development board (Minnow Max), we should let kernel
more free for other developers as well.

Br, David

> 
> There is a mechanism in ACPI to solve these issues, called _DEP, but it
> is still very much work in progress.

Re: [PATCH] pinctrl: baytrail: show output gpio state correctly on Intel Baytrail

[Mika Westerberg]

On Mon, Nov 03, 2014 at 02:19:03PM -0800, David Cohen wrote:
> Hi Mika,
> 
> Thanks for your feedbacks :)
> 
> On Mon, Nov 03, 2014 at 08:42:47PM +0200, Mika Westerberg wrote:
> > On Mon, Nov 03, 2014 at 09:50:11AM -0600, Felipe Balbi wrote:
> > > Hi,
> > > 
> > > On Mon, Nov 03, 2014 at 05:42:07PM +0200, Mika Westerberg wrote:
> > > > On Mon, Nov 03, 2014 at 05:27:43PM +0200, Mika Westerberg wrote:
> > > > > On Mon, Nov 03, 2014 at 09:00:48AM -0600, Felipe Balbi wrote:
> > > > > > On Mon, Nov 03, 2014 at 11:24:02AM +0200, Mika Westerberg wrote:
> > > > > > > On Fri, Oct 31, 2014 at 11:45:09AM -0700, David Cohen wrote:
> > > > > > > > > I think adding the module exit + allowing this driver to be a module
> > > > > > > > > would be a good approach. Then we don't need to force generic x86 kernel
> > > > > > > > > binaries to always have this driver. Unless Mathias or Mika knows a
> > > > > > > > > constraint to force this driver to be builtin only.
> > > > > > > > 
> > > > > > > > It helps if I CC them when asking for feedback :)
> > > > > > > > 
> > > > > > > > Mathias, Mika, do you know any constraint that forces pinctrl-baytrail
> > > > > > > > to be bool?
> > > > > > > 
> > > > > > > The only constraint that has been keeping this driver as bool is that
> > > > > > > some machines like, Asus T100, uses ACPI GPIO operation regions for
> > > > > > > toggling GPIOs to get things like sensor hub powered on. The GPIO
> > > > > > > operation region code does not yet handle -EPROBE_DEFER so only way to
> > > > > > > ensure that the operation region is there is to have the driver compiled
> > > > > > > in to the kernel.
> > > > > > 
> > > > > > But that's not enough excuse to have every single x86 in the market
> > > > > > shipping with this driver. Think about a distro kernel, most likely this
> > > > > > gets enabled and it's wrong in 80% of the cases.
> > > > > 
> > > > > True, but see below.
> > > > > 
> > > > > > It would be nicer to add EPROBE_DEFER support, convert this into
> > > > > > tristate and have default = M if BAYTRAIL, or something.
> > > > > 
> > > > > If it were simple as that we would have done that already. Please check
> > > > > drivers/gpio/gpiolib-acpi.c:acpi_gpio_adr_space_handler() and tell me
> > > > > how we can do that.
> > > > 
> > > > Actually the above is not the problem because we already have registered
> > > > the GPIO chip and hence we have the GPIO available to the firmware code.
> > > 
> > > what happens before you registered the gpio chip ? It takes some time
> > > from head.S to gpiochip_irqchip_add(). Anywhere between that time,
> > > firmware could try to access gpios and the same problem would occur.
> > 
> > The operation region is not ready and the firmware does not try to use
> > it. However, the subsys_initcall() is there just to be sure that the
> > GPIO driver gets loaded before anything that is going to use GPIOs from
> > firmware.
> 
> That sounds hackish and dangerous. There are shared registers between
> different GPIOs.

For one it certainly is not hackish and dangerous. It is well defined
in the ACPI specification as interfaces between firmware and OS.

> > > > The real problem is that if the ACPI GPIO operation handler is not there
> > > > at the time firmware decides to do something it will just skip things
> > > > that depend on the operation region. So if it has a GPIO that is used to
> > > > turn on sensor hub or touch panel or whatever, this will not be done and
> > > > it results that the device in question might not work properly.
> > > 
> > > that's an issue that needs solving, but forcing every x86 kernel to ship
> > > with this driver, is not a proper solution.
> > 
> > I would rather have the driver build in to the kernel now (and btw it
> > has been already in mainline quite some time so I suspect many distros
> > have already enabled it), than turning it module and render some devices
> > that have been working previously, fail suddenly.
> 
> Hm.... it sounds like we're saying: we got it working somehow and from
> now on we'll avoid change it to not create regression.

That's exactly what I'm saying :-)

> Correct if I'm wrong, but there's nothing really preventing this driver
> to be a module beside the buggy ACPI interface that allows kernel and fw
> to play freely (and silently) with same piece of hw.

Not a buggy ACPI interface. Lack of implementation from Linux side.

> IMHO we could allow this driver be M, but default to Y and an
> explanation to set to Y in case FW needs to use GPIO before module gets
> loaded. Since we've development board (Minnow Max), we should let kernel
> more free for other developers as well.

All I'm saying both to you guys that this is the reasoning we have the
driver as a bool. Which is what you asked from me in the first place.

Since you both insist to turn it module, please send a patch with the
corresponding changelog telling why you think it does not need to build
in and let's see if we are getting any regressions. If yes, then we just
revert the patch. Does that work for you?

Re: [PATCH] pinctrl: baytrail: show output gpio state correctly on Intel Baytrail

[David Cohen]

On Tue, Nov 04, 2014 at 09:59:36AM +0200, Mika Westerberg wrote:
> On Mon, Nov 03, 2014 at 02:19:03PM -0800, David Cohen wrote:
> > Hi Mika,
> > 
> > Thanks for your feedbacks :)
> > 
> > On Mon, Nov 03, 2014 at 08:42:47PM +0200, Mika Westerberg wrote:
> > > On Mon, Nov 03, 2014 at 09:50:11AM -0600, Felipe Balbi wrote:
> > > > Hi,
> > > > 
> > > > On Mon, Nov 03, 2014 at 05:42:07PM +0200, Mika Westerberg wrote:
> > > > > On Mon, Nov 03, 2014 at 05:27:43PM +0200, Mika Westerberg wrote:
> > > > > > On Mon, Nov 03, 2014 at 09:00:48AM -0600, Felipe Balbi wrote:
> > > > > > > On Mon, Nov 03, 2014 at 11:24:02AM +0200, Mika Westerberg wrote:
> > > > > > > > On Fri, Oct 31, 2014 at 11:45:09AM -0700, David Cohen wrote:
> > > > > > > > > > I think adding the module exit + allowing this driver to be a module
> > > > > > > > > > would be a good approach. Then we don't need to force generic x86 kernel
> > > > > > > > > > binaries to always have this driver. Unless Mathias or Mika knows a
> > > > > > > > > > constraint to force this driver to be builtin only.
> > > > > > > > > 
> > > > > > > > > It helps if I CC them when asking for feedback :)
> > > > > > > > > 
> > > > > > > > > Mathias, Mika, do you know any constraint that forces pinctrl-baytrail
> > > > > > > > > to be bool?
> > > > > > > > 
> > > > > > > > The only constraint that has been keeping this driver as bool is that
> > > > > > > > some machines like, Asus T100, uses ACPI GPIO operation regions for
> > > > > > > > toggling GPIOs to get things like sensor hub powered on. The GPIO
> > > > > > > > operation region code does not yet handle -EPROBE_DEFER so only way to
> > > > > > > > ensure that the operation region is there is to have the driver compiled
> > > > > > > > in to the kernel.
> > > > > > > 
> > > > > > > But that's not enough excuse to have every single x86 in the market
> > > > > > > shipping with this driver. Think about a distro kernel, most likely this
> > > > > > > gets enabled and it's wrong in 80% of the cases.
> > > > > > 
> > > > > > True, but see below.
> > > > > > 
> > > > > > > It would be nicer to add EPROBE_DEFER support, convert this into
> > > > > > > tristate and have default = M if BAYTRAIL, or something.
> > > > > > 
> > > > > > If it were simple as that we would have done that already. Please check
> > > > > > drivers/gpio/gpiolib-acpi.c:acpi_gpio_adr_space_handler() and tell me
> > > > > > how we can do that.
> > > > > 
> > > > > Actually the above is not the problem because we already have registered
> > > > > the GPIO chip and hence we have the GPIO available to the firmware code.
> > > > 
> > > > what happens before you registered the gpio chip ? It takes some time
> > > > from head.S to gpiochip_irqchip_add(). Anywhere between that time,
> > > > firmware could try to access gpios and the same problem would occur.
> > > 
> > > The operation region is not ready and the firmware does not try to use
> > > it. However, the subsys_initcall() is there just to be sure that the
> > > GPIO driver gets loaded before anything that is going to use GPIOs from
> > > firmware.
> > 
> > That sounds hackish and dangerous. There are shared registers between
> > different GPIOs.
> 
> For one it certainly is not hackish and dangerous. It is well defined
> in the ACPI specification as interfaces between firmware and OS.

It looks we have an implicit dependency to GPIO driver in Bay Trail, and
having this window until load the module is not acceptable to fulfill
this implicit dependency.

But IMHO all dependency to a driver should be explicitly described
(e.g. on Kconfigs, or maybe failing probe). With current situation if we
do not select pinctrl_baytrail, instead of affecting just the drivers
that explicitly depend on that, it affects others which we are unable to
easily identify.

Maybe I don't like or fully agreee/understand ACPI, but I'd call that
dangerous.

> 
> > > > > The real problem is that if the ACPI GPIO operation handler is not there
> > > > > at the time firmware decides to do something it will just skip things
> > > > > that depend on the operation region. So if it has a GPIO that is used to
> > > > > turn on sensor hub or touch panel or whatever, this will not be done and
> > > > > it results that the device in question might not work properly.
> > > > 
> > > > that's an issue that needs solving, but forcing every x86 kernel to ship
> > > > with this driver, is not a proper solution.
> > > 
> > > I would rather have the driver build in to the kernel now (and btw it
> > > has been already in mainline quite some time so I suspect many distros
> > > have already enabled it), than turning it module and render some devices
> > > that have been working previously, fail suddenly.
> > 
> > Hm.... it sounds like we're saying: we got it working somehow and from
> > now on we'll avoid change it to not create regression.
> 
> That's exactly what I'm saying :-)

In this case, I think we need to carefully document our concerns about
the driver to help others not involved in original implementation to
easily contribute to it.

> 
> > Correct if I'm wrong, but there's nothing really preventing this driver
> > to be a module beside the buggy ACPI interface that allows kernel and fw
> > to play freely (and silently) with same piece of hw.
> 
> Not a buggy ACPI interface. Lack of implementation from Linux side.
> 
> > IMHO we could allow this driver be M, but default to Y and an
> > explanation to set to Y in case FW needs to use GPIO before module gets
> > loaded. Since we've development board (Minnow Max), we should let kernel
> > more free for other developers as well.
> 
> All I'm saying both to you guys that this is the reasoning we have the
> driver as a bool. Which is what you asked from me in the first place.

I apology for my frustration :)
You've been more than helpful clarifying our questions.

> 
> Since you both insist to turn it module, please send a patch with the
> corresponding changelog telling why you think it does not need to build
> in and let's see if we are getting any regressions. If yes, then we just
> revert the patch. Does that work for you?

I'm in favor of not hardcoding kernel features based on possible hidden
things that may happen on closed source FWs.
Having the module option does help further development of this driver
and others that depend on it. A clear explanation on Kconfig why we
recommend Y for production and why M is still experimental should be
enough to guide ppl working on distributions and at same time help
kernel developers involved with the driver.

Br, David

Re: [PATCH] pinctrl: baytrail: show output gpio state correctly on Intel Baytrail

[Mika Westerberg]

On Tue, Nov 04, 2014 at 10:05:26AM -0800, David Cohen wrote:
> On Tue, Nov 04, 2014 at 09:59:36AM +0200, Mika Westerberg wrote:
> > On Mon, Nov 03, 2014 at 02:19:03PM -0800, David Cohen wrote:
> > > Hi Mika,
> > > 
> > > Thanks for your feedbacks :)
> > > 
> > > On Mon, Nov 03, 2014 at 08:42:47PM +0200, Mika Westerberg wrote:
> > > > On Mon, Nov 03, 2014 at 09:50:11AM -0600, Felipe Balbi wrote:
> > > > > Hi,
> > > > > 
> > > > > On Mon, Nov 03, 2014 at 05:42:07PM +0200, Mika Westerberg wrote:
> > > > > > On Mon, Nov 03, 2014 at 05:27:43PM +0200, Mika Westerberg wrote:
> > > > > > > On Mon, Nov 03, 2014 at 09:00:48AM -0600, Felipe Balbi wrote:
> > > > > > > > On Mon, Nov 03, 2014 at 11:24:02AM +0200, Mika Westerberg wrote:
> > > > > > > > > On Fri, Oct 31, 2014 at 11:45:09AM -0700, David Cohen wrote:
> > > > > > > > > > > I think adding the module exit + allowing this driver to be a module
> > > > > > > > > > > would be a good approach. Then we don't need to force generic x86 kernel
> > > > > > > > > > > binaries to always have this driver. Unless Mathias or Mika knows a
> > > > > > > > > > > constraint to force this driver to be builtin only.
> > > > > > > > > > 
> > > > > > > > > > It helps if I CC them when asking for feedback :)
> > > > > > > > > > 
> > > > > > > > > > Mathias, Mika, do you know any constraint that forces pinctrl-baytrail
> > > > > > > > > > to be bool?
> > > > > > > > > 
> > > > > > > > > The only constraint that has been keeping this driver as bool is that
> > > > > > > > > some machines like, Asus T100, uses ACPI GPIO operation regions for
> > > > > > > > > toggling GPIOs to get things like sensor hub powered on. The GPIO
> > > > > > > > > operation region code does not yet handle -EPROBE_DEFER so only way to
> > > > > > > > > ensure that the operation region is there is to have the driver compiled
> > > > > > > > > in to the kernel.
> > > > > > > > 
> > > > > > > > But that's not enough excuse to have every single x86 in the market
> > > > > > > > shipping with this driver. Think about a distro kernel, most likely this
> > > > > > > > gets enabled and it's wrong in 80% of the cases.
> > > > > > > 
> > > > > > > True, but see below.
> > > > > > > 
> > > > > > > > It would be nicer to add EPROBE_DEFER support, convert this into
> > > > > > > > tristate and have default = M if BAYTRAIL, or something.
> > > > > > > 
> > > > > > > If it were simple as that we would have done that already. Please check
> > > > > > > drivers/gpio/gpiolib-acpi.c:acpi_gpio_adr_space_handler() and tell me
> > > > > > > how we can do that.
> > > > > > 
> > > > > > Actually the above is not the problem because we already have registered
> > > > > > the GPIO chip and hence we have the GPIO available to the firmware code.
> > > > > 
> > > > > what happens before you registered the gpio chip ? It takes some time
> > > > > from head.S to gpiochip_irqchip_add(). Anywhere between that time,
> > > > > firmware could try to access gpios and the same problem would occur.
> > > > 
> > > > The operation region is not ready and the firmware does not try to use
> > > > it. However, the subsys_initcall() is there just to be sure that the
> > > > GPIO driver gets loaded before anything that is going to use GPIOs from
> > > > firmware.
> > > 
> > > That sounds hackish and dangerous. There are shared registers between
> > > different GPIOs.
> > 
> > For one it certainly is not hackish and dangerous. It is well defined
> > in the ACPI specification as interfaces between firmware and OS.
> 
> It looks we have an implicit dependency to GPIO driver in Bay Trail, and
> having this window until load the module is not acceptable to fulfill
> this implicit dependency.

It is not implicit at all.

The user of the GPIO in ACPI DSDT table says something like:

	Name (_DEP, Package () { \_SB.GPO2 })

or similar. That is *explicit* dependency. Here \_SB.GPO2 is one of the
GPIO banks.

> But IMHO all dependency to a driver should be explicitly described
> (e.g. on Kconfigs, or maybe failing probe). With current situation if we
> do not select pinctrl_baytrail, instead of affecting just the drivers
> that explicitly depend on that, it affects others which we are unable to
> easily identify.

So how do you propose we describe the dependency? It is completely in
firmware. Should we make i2c-hid.c dependent on pinctrl-baytrail.c just
because some underlying firmware method (_PSx for example) needs the
GPIO but the driver itself does not?

> Maybe I don't like or fully agreee/understand ACPI, but I'd call that
> dangerous.
>
> > 
> > > > > > The real problem is that if the ACPI GPIO operation handler is not there
> > > > > > at the time firmware decides to do something it will just skip things
> > > > > > that depend on the operation region. So if it has a GPIO that is used to
> > > > > > turn on sensor hub or touch panel or whatever, this will not be done and
> > > > > > it results that the device in question might not work properly.
> > > > > 
> > > > > that's an issue that needs solving, but forcing every x86 kernel to ship
> > > > > with this driver, is not a proper solution.
> > > > 
> > > > I would rather have the driver build in to the kernel now (and btw it
> > > > has been already in mainline quite some time so I suspect many distros
> > > > have already enabled it), than turning it module and render some devices
> > > > that have been working previously, fail suddenly.
> > > 
> > > Hm.... it sounds like we're saying: we got it working somehow and from
> > > now on we'll avoid change it to not create regression.
> > 
> > That's exactly what I'm saying :-)
> 
> In this case, I think we need to carefully document our concerns about
> the driver to help others not involved in original implementation to
> easily contribute to it.
>
> > 
> > > Correct if I'm wrong, but there's nothing really preventing this driver
> > > to be a module beside the buggy ACPI interface that allows kernel and fw
> > > to play freely (and silently) with same piece of hw.
> > 
> > Not a buggy ACPI interface. Lack of implementation from Linux side.
> > 
> > > IMHO we could allow this driver be M, but default to Y and an
> > > explanation to set to Y in case FW needs to use GPIO before module gets
> > > loaded. Since we've development board (Minnow Max), we should let kernel
> > > more free for other developers as well.
> > 
> > All I'm saying both to you guys that this is the reasoning we have the
> > driver as a bool. Which is what you asked from me in the first place.
> 
> I apology for my frustration :)
> You've been more than helpful clarifying our questions.
> 
> > 
> > Since you both insist to turn it module, please send a patch with the
> > corresponding changelog telling why you think it does not need to build
> > in and let's see if we are getting any regressions. If yes, then we just
> > revert the patch. Does that work for you?
> 
> I'm in favor of not hardcoding kernel features based on possible hidden
> things that may happen on closed source FWs.
> Having the module option does help further development of this driver
> and others that depend on it. A clear explanation on Kconfig why we
> recommend Y for production and why M is still experimental should be
> enough to guide ppl working on distributions and at same time help
> kernel developers involved with the driver.

Like I said, send a patch describing why you think it needs to be module
and make it tristate instead. If it causes problems we can revert it.

Re: [PATCH] pinctrl: baytrail: show output gpio state correctly on Intel Baytrail

[David Cohen]

On Tue, Nov 04, 2014 at 08:57:02PM +0200, Mika Westerberg wrote:
> On Tue, Nov 04, 2014 at 10:05:26AM -0800, David Cohen wrote:
> > On Tue, Nov 04, 2014 at 09:59:36AM +0200, Mika Westerberg wrote:
> > > On Mon, Nov 03, 2014 at 02:19:03PM -0800, David Cohen wrote:
> > > > Hi Mika,
> > > > 
> > > > Thanks for your feedbacks :)
> > > > 
> > > > On Mon, Nov 03, 2014 at 08:42:47PM +0200, Mika Westerberg wrote:
> > > > > On Mon, Nov 03, 2014 at 09:50:11AM -0600, Felipe Balbi wrote:
> > > > > > Hi,
> > > > > > 
> > > > > > On Mon, Nov 03, 2014 at 05:42:07PM +0200, Mika Westerberg wrote:
> > > > > > > On Mon, Nov 03, 2014 at 05:27:43PM +0200, Mika Westerberg wrote:
> > > > > > > > On Mon, Nov 03, 2014 at 09:00:48AM -0600, Felipe Balbi wrote:
> > > > > > > > > On Mon, Nov 03, 2014 at 11:24:02AM +0200, Mika Westerberg wrote:
> > > > > > > > > > On Fri, Oct 31, 2014 at 11:45:09AM -0700, David Cohen wrote:
> > > > > > > > > > > > I think adding the module exit + allowing this driver to be a module
> > > > > > > > > > > > would be a good approach. Then we don't need to force generic x86 kernel
> > > > > > > > > > > > binaries to always have this driver. Unless Mathias or Mika knows a
> > > > > > > > > > > > constraint to force this driver to be builtin only.
> > > > > > > > > > > 
> > > > > > > > > > > It helps if I CC them when asking for feedback :)
> > > > > > > > > > > 
> > > > > > > > > > > Mathias, Mika, do you know any constraint that forces pinctrl-baytrail
> > > > > > > > > > > to be bool?
> > > > > > > > > > 
> > > > > > > > > > The only constraint that has been keeping this driver as bool is that
> > > > > > > > > > some machines like, Asus T100, uses ACPI GPIO operation regions for
> > > > > > > > > > toggling GPIOs to get things like sensor hub powered on. The GPIO
> > > > > > > > > > operation region code does not yet handle -EPROBE_DEFER so only way to
> > > > > > > > > > ensure that the operation region is there is to have the driver compiled
> > > > > > > > > > in to the kernel.
> > > > > > > > > 
> > > > > > > > > But that's not enough excuse to have every single x86 in the market
> > > > > > > > > shipping with this driver. Think about a distro kernel, most likely this
> > > > > > > > > gets enabled and it's wrong in 80% of the cases.
> > > > > > > > 
> > > > > > > > True, but see below.
> > > > > > > > 
> > > > > > > > > It would be nicer to add EPROBE_DEFER support, convert this into
> > > > > > > > > tristate and have default = M if BAYTRAIL, or something.
> > > > > > > > 
> > > > > > > > If it were simple as that we would have done that already. Please check
> > > > > > > > drivers/gpio/gpiolib-acpi.c:acpi_gpio_adr_space_handler() and tell me
> > > > > > > > how we can do that.
> > > > > > > 
> > > > > > > Actually the above is not the problem because we already have registered
> > > > > > > the GPIO chip and hence we have the GPIO available to the firmware code.
> > > > > > 
> > > > > > what happens before you registered the gpio chip ? It takes some time
> > > > > > from head.S to gpiochip_irqchip_add(). Anywhere between that time,
> > > > > > firmware could try to access gpios and the same problem would occur.
> > > > > 
> > > > > The operation region is not ready and the firmware does not try to use
> > > > > it. However, the subsys_initcall() is there just to be sure that the
> > > > > GPIO driver gets loaded before anything that is going to use GPIOs from
> > > > > firmware.
> > > > 
> > > > That sounds hackish and dangerous. There are shared registers between
> > > > different GPIOs.
> > > 
> > > For one it certainly is not hackish and dangerous. It is well defined
> > > in the ACPI specification as interfaces between firmware and OS.
> > 
> > It looks we have an implicit dependency to GPIO driver in Bay Trail, and
> > having this window until load the module is not acceptable to fulfill
> > this implicit dependency.
> 
> It is not implicit at all.
> 
> The user of the GPIO in ACPI DSDT table says something like:
> 
> 	Name (_DEP, Package () { \_SB.GPO2 })
> 
> or similar. That is *explicit* dependency. Here \_SB.GPO2 is one of the
> GPIO banks.

Either kernel knows on-the-fly or statically the required dependency.
The static dependency is well described by Kconfig. An on-the-fly
dependency could be a probe execution failing because it couldn't access
part of required resources. If the dependency is temporarily not
described this way, it would still be acceptable a documentation
somewhere explaining that we do have this hidden thing going on.

> 
> > But IMHO all dependency to a driver should be explicitly described
> > (e.g. on Kconfigs, or maybe failing probe). With current situation if we
> > do not select pinctrl_baytrail, instead of affecting just the drivers
> > that explicitly depend on that, it affects others which we are unable to
> > easily identify.
> 
> So how do you propose we describe the dependency? It is completely in
> firmware. Should we make i2c-hid.c dependent on pinctrl-baytrail.c just
> because some underlying firmware method (_PSx for example) needs the
> GPIO but the driver itself does not?

i2c-hid.c should fail, WARN, yell, scream or whatever :)
This way one could say: hey, we needed GPIO.

> 
> > Maybe I don't like or fully agreee/understand ACPI, but I'd call that
> > dangerous.
> >
> > > 
> > > > > > > The real problem is that if the ACPI GPIO operation handler is not there
> > > > > > > at the time firmware decides to do something it will just skip things
> > > > > > > that depend on the operation region. So if it has a GPIO that is used to
> > > > > > > turn on sensor hub or touch panel or whatever, this will not be done and
> > > > > > > it results that the device in question might not work properly.
> > > > > > 
> > > > > > that's an issue that needs solving, but forcing every x86 kernel to ship
> > > > > > with this driver, is not a proper solution.
> > > > > 
> > > > > I would rather have the driver build in to the kernel now (and btw it
> > > > > has been already in mainline quite some time so I suspect many distros
> > > > > have already enabled it), than turning it module and render some devices
> > > > > that have been working previously, fail suddenly.
> > > > 
> > > > Hm.... it sounds like we're saying: we got it working somehow and from
> > > > now on we'll avoid change it to not create regression.
> > > 
> > > That's exactly what I'm saying :-)
> > 
> > In this case, I think we need to carefully document our concerns about
> > the driver to help others not involved in original implementation to
> > easily contribute to it.
> >
> > > 
> > > > Correct if I'm wrong, but there's nothing really preventing this driver
> > > > to be a module beside the buggy ACPI interface that allows kernel and fw
> > > > to play freely (and silently) with same piece of hw.
> > > 
> > > Not a buggy ACPI interface. Lack of implementation from Linux side.
> > > 
> > > > IMHO we could allow this driver be M, but default to Y and an
> > > > explanation to set to Y in case FW needs to use GPIO before module gets
> > > > loaded. Since we've development board (Minnow Max), we should let kernel
> > > > more free for other developers as well.
> > > 
> > > All I'm saying both to you guys that this is the reasoning we have the
> > > driver as a bool. Which is what you asked from me in the first place.
> > 
> > I apology for my frustration :)
> > You've been more than helpful clarifying our questions.
> > 
> > > 
> > > Since you both insist to turn it module, please send a patch with the
> > > corresponding changelog telling why you think it does not need to build
> > > in and let's see if we are getting any regressions. If yes, then we just
> > > revert the patch. Does that work for you?
> > 
> > I'm in favor of not hardcoding kernel features based on possible hidden
> > things that may happen on closed source FWs.
> > Having the module option does help further development of this driver
> > and others that depend on it. A clear explanation on Kconfig why we
> > recommend Y for production and why M is still experimental should be
> > enough to guide ppl working on distributions and at same time help
> > kernel developers involved with the driver.
> 
> Like I said, send a patch describing why you think it needs to be module
> and make it tristate instead. If it causes problems we can revert it.

Thanks :)

BR, David

Re: [PATCH] pinctrl: baytrail: show output gpio state correctly on Intel Baytrail

[Mika Westerberg]

On Tue, Nov 04, 2014 at 11:11:16AM -0800, David Cohen wrote:
> > It is not implicit at all.
> > 
> > The user of the GPIO in ACPI DSDT table says something like:
> > 
> > 	Name (_DEP, Package () { \_SB.GPO2 })
> > 
> > or similar. That is *explicit* dependency. Here \_SB.GPO2 is one of the
> > GPIO banks.
> 
> Either kernel knows on-the-fly or statically the required dependency.
> The static dependency is well described by Kconfig. An on-the-fly
> dependency could be a probe execution failing because it couldn't access
> part of required resources. If the dependency is temporarily not
> described this way, it would still be acceptable a documentation
> somewhere explaining that we do have this hidden thing going on.

The only thing kernel knows about this is when it finds that the
device in question has _DEP object. Once that happens and it evaluates
to a list of devices we depend on, we can defer this particular driver
going further in probe until all the dependencies listed in _DEP are
resolved.

The documentation you are after is ACPI 5.1 specification downloadable
freely at uefi.org/acpi.

> > > But IMHO all dependency to a driver should be explicitly described
> > > (e.g. on Kconfigs, or maybe failing probe). With current situation if we
> > > do not select pinctrl_baytrail, instead of affecting just the drivers
> > > that explicitly depend on that, it affects others which we are unable to
> > > easily identify.
> > 
> > So how do you propose we describe the dependency? It is completely in
> > firmware. Should we make i2c-hid.c dependent on pinctrl-baytrail.c just
> > because some underlying firmware method (_PSx for example) needs the
> > GPIO but the driver itself does not?
> 
> i2c-hid.c should fail, WARN, yell, scream or whatever :)
> This way one could say: hey, we needed GPIO.

But i2c-hid.c does not know anything about GPIOS in the first place.
Like I said the dependency is in the firmware level. It may need GPIOs
to do something or not but the driver never sees those GPIOs.

Re: [PATCH] pinctrl: baytrail: show output gpio state correctly on Intel Baytrail

[David Cohen]

On Tue, Nov 04, 2014 at 09:34:24PM +0200, Mika Westerberg wrote:
> On Tue, Nov 04, 2014 at 11:11:16AM -0800, David Cohen wrote:
> > > It is not implicit at all.
> > > 
> > > The user of the GPIO in ACPI DSDT table says something like:
> > > 
> > > 	Name (_DEP, Package () { \_SB.GPO2 })
> > > 
> > > or similar. That is *explicit* dependency. Here \_SB.GPO2 is one of the
> > > GPIO banks.
> > 
> > Either kernel knows on-the-fly or statically the required dependency.
> > The static dependency is well described by Kconfig. An on-the-fly
> > dependency could be a probe execution failing because it couldn't access
> > part of required resources. If the dependency is temporarily not
> > described this way, it would still be acceptable a documentation
> > somewhere explaining that we do have this hidden thing going on.
> 
> The only thing kernel knows about this is when it finds that the
> device in question has _DEP object. Once that happens and it evaluates

That's all kernel needs to know.

> to a list of devices we depend on, we can defer this particular driver
> going further in probe until all the dependencies listed in _DEP are
> resolved.

That's the best way to prevent this issue IMHO, but looks like it's
already being addressed:
https://lkml.org/lkml/2014/10/27/455

> 
> The documentation you are after is ACPI 5.1 specification downloadable
> freely at uefi.org/acpi.

Nope. The documentation I am referring to is the one that doesn't exist.
It supposed to be a simple comment on pinctrl_baytrail.c's file, or
maybe a comment on Kconfig. Wherever it could tell ppl not involved with
original development that there might be an implicit dependency to this
gpio driver by other drivers. And because of that, it's recommended to
let this driver probed as soon as possible on Bay Trail platforms.

BTW if we can't find the dependency either on Kconfig or on kernel
codes, if no dependent is checking for this resource when probing, if
all we have to define that this dependency exists is by looking random
ACPI tables (which are neither generic, nor part of kernel), it is then
implicit WRT kernel. Kernel cannot find it, nor developers can foresee
it.

> 
> > > > But IMHO all dependency to a driver should be explicitly described
> > > > (e.g. on Kconfigs, or maybe failing probe). With current situation if we
> > > > do not select pinctrl_baytrail, instead of affecting just the drivers
> > > > that explicitly depend on that, it affects others which we are unable to
> > > > easily identify.
> > > 
> > > So how do you propose we describe the dependency? It is completely in
> > > firmware. Should we make i2c-hid.c dependent on pinctrl-baytrail.c just
> > > because some underlying firmware method (_PSx for example) needs the
> > > GPIO but the driver itself does not?
> > 
> > i2c-hid.c should fail, WARN, yell, scream or whatever :)
> > This way one could say: hey, we needed GPIO.
> 
> But i2c-hid.c does not know anything about GPIOS in the first place.
> Like I said the dependency is in the firmware level. It may need GPIOs
> to do something or not but the driver never sees those GPIOs.

AFAIU any gpio usage by FW would start from a call from i2c-hid.c
driver. Maybe this call could fail, and then nothing implicit happens.
But i2c-hid.c does know about the dependency if checks its ACPI table
during the probe. But that's being addressed by the thread I posted
above.

Br, David

Re: [PATCH] pinctrl: baytrail: show output gpio state correctly on Intel Baytrail

[Mika Westerberg]

On Tue, Nov 04, 2014 at 01:51:53PM -0800, David Cohen wrote:
> On Tue, Nov 04, 2014 at 09:34:24PM +0200, Mika Westerberg wrote:
> > On Tue, Nov 04, 2014 at 11:11:16AM -0800, David Cohen wrote:
> > > > It is not implicit at all.
> > > > 
> > > > The user of the GPIO in ACPI DSDT table says something like:
> > > > 
> > > > 	Name (_DEP, Package () { \_SB.GPO2 })
> > > > 
> > > > or similar. That is *explicit* dependency. Here \_SB.GPO2 is one of the
> > > > GPIO banks.
> > > 
> > > Either kernel knows on-the-fly or statically the required dependency.
> > > The static dependency is well described by Kconfig. An on-the-fly
> > > dependency could be a probe execution failing because it couldn't access
> > > part of required resources. If the dependency is temporarily not
> > > described this way, it would still be acceptable a documentation
> > > somewhere explaining that we do have this hidden thing going on.
> > 
> > The only thing kernel knows about this is when it finds that the
> > device in question has _DEP object. Once that happens and it evaluates
> 
> That's all kernel needs to know.
> 
> > to a list of devices we depend on, we can defer this particular driver
> > going further in probe until all the dependencies listed in _DEP are
> > resolved.
> 
> That's the best way to prevent this issue IMHO, but looks like it's
> already being addressed:
> https://lkml.org/lkml/2014/10/27/455

Yes, that's the patch series.

> > 
> > The documentation you are after is ACPI 5.1 specification downloadable
> > freely at uefi.org/acpi.
> 
> Nope. The documentation I am referring to is the one that doesn't exist.
> It supposed to be a simple comment on pinctrl_baytrail.c's file, or
> maybe a comment on Kconfig. Wherever it could tell ppl not involved with
> original development that there might be an implicit dependency to this
> gpio driver by other drivers. And because of that, it's recommended to
> let this driver probed as soon as possible on Bay Trail platforms.

Are we going to add similar to other drivers as well? I2C core for
example installs I2C operation region handlers for every I2C controller
that happens to have an ACPI handle.

> BTW if we can't find the dependency either on Kconfig or on kernel
> codes, if no dependent is checking for this resource when probing, if
> all we have to define that this dependency exists is by looking random
> ACPI tables (which are neither generic, nor part of kernel), it is then
> implicit WRT kernel. Kernel cannot find it, nor developers can foresee
> it.

OK.

> > > > > But IMHO all dependency to a driver should be explicitly described
> > > > > (e.g. on Kconfigs, or maybe failing probe). With current situation if we
> > > > > do not select pinctrl_baytrail, instead of affecting just the drivers
> > > > > that explicitly depend on that, it affects others which we are unable to
> > > > > easily identify.
> > > > 
> > > > So how do you propose we describe the dependency? It is completely in
> > > > firmware. Should we make i2c-hid.c dependent on pinctrl-baytrail.c just
> > > > because some underlying firmware method (_PSx for example) needs the
> > > > GPIO but the driver itself does not?
> > > 
> > > i2c-hid.c should fail, WARN, yell, scream or whatever :)
> > > This way one could say: hey, we needed GPIO.
> > 
> > But i2c-hid.c does not know anything about GPIOS in the first place.
> > Like I said the dependency is in the firmware level. It may need GPIOs
> > to do something or not but the driver never sees those GPIOs.
> 
> AFAIU any gpio usage by FW would start from a call from i2c-hid.c

Not necesarily. Even before we enter the probe of the i2c-hid.c, the
ACPI power domain might try to power on the device. That might involve
toggling GPIOs from the AML code.

Same thing during system suspend/resume.

The driver is not involved in this but it is the ACPI device related to
the i2c-hid.c that has the GPIO dependency.

> driver. Maybe this call could fail, and then nothing implicit happens.
> But i2c-hid.c does know about the dependency if checks its ACPI table
> during the probe. But that's being addressed by the thread I posted
> above.

Indeed.

However, now that I look at the _DEP implementation closely it seems to
leave this to the drivers and not automatically prevent probing a driver
if the device in questions has unmet dependencies...

Re: [PATCH] pinctrl: baytrail: show output gpio state correctly on Intel Baytrail

[Linus Walleij]

On Tue, Nov 4, 2014 at 10:51 PM, David Cohen
<david.a.cohen@linux.intel.com> wrote:
> On Tue, Nov 04, 2014 at 09:34:24PM +0200, Mika Westerberg wrote:

>> to a list of devices we depend on, we can defer this particular driver
>> going further in probe until all the dependencies listed in _DEP are
>> resolved.
>
> That's the best way to prevent this issue IMHO, but looks like it's
> already being addressed:
> https://lkml.org/lkml/2014/10/27/455

-EPROVE_DEFER will solve (hackishly) the probing problem.

It will not solve remove() or power up/down sequencing.

Yours,
Linus Walleij

Re: [PATCH] pinctrl: baytrail: show output gpio state correctly on Intel Baytrail

[Linus Walleij]

On Tue, Nov 4, 2014 at 7:57 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Tue, Nov 04, 2014 at 10:05:26AM -0800, David Cohen wrote:

>> It looks we have an implicit dependency to GPIO driver in Bay Trail, and
>> having this window until load the module is not acceptable to fulfill
>> this implicit dependency.
>
> It is not implicit at all.
>
> The user of the GPIO in ACPI DSDT table says something like:
>
>         Name (_DEP, Package () { \_SB.GPO2 })
>
> or similar. That is *explicit* dependency. Here \_SB.GPO2 is one of the
> GPIO banks.

That's very nice for ACPI. But what do you expect the Linux kernel to
do with that?

Basically that is just like getting an -EPROBE_DEFER from the
gpiochip when the gpiod_get() call is issued, and you have to wait
because the gpiochip is not probed yet. We can solve that at runtime
right?

I had a discussion with Greg the other day that we have no way of
expressing inside the kernel that a resource such as a GPIO, a pin,
a clk or a regulator is used by some module. It's just a synchronous
gpiod_get() or whatever call, then there is a warning if you remove
a gpiochip with gpios still in use.

What is needed to make use of such a dependency mechanism is
a way to graph the dependencies between kernel drivers and
the resources (gpios, clocks, regulators...) they provide to other
drivers, so this information can be used when probing, removing,
powering up/down the cluster.

That problem needs to be solved in the device core, until then there
is not way to actually use that ACPI _DEP property for what I can
tell.

(On a side note: whoever came up with the idea that ACPI props
be 4 characters wide and start with an underscore and this
backslash obfuscation needs to... think differently.)

Yours,
Linus Walleij

Re: [PATCH] pinctrl: baytrail: show output gpio state correctly on Intel Baytrail

[Mika Westerberg]

+Rafael

On Fri, Nov 14, 2014 at 10:39:07AM +0100, Linus Walleij wrote:
> On Tue, Nov 4, 2014 at 7:57 PM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > On Tue, Nov 04, 2014 at 10:05:26AM -0800, David Cohen wrote:
> 
> >> It looks we have an implicit dependency to GPIO driver in Bay Trail, and
> >> having this window until load the module is not acceptable to fulfill
> >> this implicit dependency.
> >
> > It is not implicit at all.
> >
> > The user of the GPIO in ACPI DSDT table says something like:
> >
> >         Name (_DEP, Package () { \_SB.GPO2 })
> >
> > or similar. That is *explicit* dependency. Here \_SB.GPO2 is one of the
> > GPIO banks.
> 
> That's very nice for ACPI. But what do you expect the Linux kernel to
> do with that?

It should prevent the driver from probing until all the devices listed
in _DEP have drivers probed.

However, it turned out that this is not that straightforward after all
:-( For one, it looks like _DEP is used also for non-operation region
dependencies. This is not in the ACPI spec but we have seen this in real
machines out there.

Other thing I heard, is that handling all these dependencies in driver
core might be nightmare to maintain.

> Basically that is just like getting an -EPROBE_DEFER from the
> gpiochip when the gpiod_get() call is issued, and you have to wait
> because the gpiochip is not probed yet. We can solve that at runtime
> right?

Yes we can if the driver core prevents probing the driver.

> I had a discussion with Greg the other day that we have no way of
> expressing inside the kernel that a resource such as a GPIO, a pin,
> a clk or a regulator is used by some module. It's just a synchronous
> gpiod_get() or whatever call, then there is a warning if you remove
> a gpiochip with gpios still in use.
> 
> What is needed to make use of such a dependency mechanism is
> a way to graph the dependencies between kernel drivers and
> the resources (gpios, clocks, regulators...) they provide to other
> drivers, so this information can be used when probing, removing,
> powering up/down the cluster.
> 
> That problem needs to be solved in the device core, until then there
> is not way to actually use that ACPI _DEP property for what I can
> tell.

I agree.

> (On a side note: whoever came up with the idea that ACPI props
> be 4 characters wide and start with an underscore and this
> backslash obfuscation needs to... think differently.)
> 
> Yours,
> Linus Walleij

Re: [PATCH] pinctrl: baytrail: show output gpio state correctly on Intel Baytrail

[Rafael J. Wysocki]

On Friday, November 14, 2014 11:53:40 AM Mika Westerberg wrote:
> +Rafael
> 
> On Fri, Nov 14, 2014 at 10:39:07AM +0100, Linus Walleij wrote:
> > On Tue, Nov 4, 2014 at 7:57 PM, Mika Westerberg
> > <mika.westerberg@linux.intel.com> wrote:
> > > On Tue, Nov 04, 2014 at 10:05:26AM -0800, David Cohen wrote:
> > 
> > >> It looks we have an implicit dependency to GPIO driver in Bay Trail, and
> > >> having this window until load the module is not acceptable to fulfill
> > >> this implicit dependency.
> > >
> > > It is not implicit at all.
> > >
> > > The user of the GPIO in ACPI DSDT table says something like:
> > >
> > >         Name (_DEP, Package () { \_SB.GPO2 })
> > >
> > > or similar. That is *explicit* dependency. Here \_SB.GPO2 is one of the
> > > GPIO banks.
> > 
> > That's very nice for ACPI. But what do you expect the Linux kernel to
> > do with that?
> 
> It should prevent the driver from probing until all the devices listed
> in _DEP have drivers probed.

Not only or not precisely that.

By the spec, _DEP is supposed to indicate an "operation region dependency",
which means that device A is providing an operation region handler that will
be invoked when AML executed for device B accesses certain things.

In other words, the operation region handler provided by A should be functional
when said AML is invoked for device B.  There are ACPI objects that are required
to not depend on any operation regions with such dependencies and those can be
safely evaluated at any time, but currently we evaluate more than that during
device enumeration alone.  So this affects more than just driver probing even
with the restricted spec-compliant interpretation.

> However, it turned out that this is not that straightforward after all
> :-( For one, it looks like _DEP is used also for non-operation region
> dependencies. This is not in the ACPI spec but we have seen this in real
> machines out there.

Yeah.  _DEP is apparently used to indicate any functional dependencies as
in "device B depends on device A somehow" which has much more far reaching
consequences.  In particular, it implies that A needs to be functional
whenever B is in use, so for example A may not be suspended when B is
active and so on.

> Other thing I heard, is that handling all these dependencies in driver
> core might be nightmare to maintain.

That if we can come up with a way to represent those dependencies in a
sensible way, which hasn't happened yet so far.

> > Basically that is just like getting an -EPROBE_DEFER from the
> > gpiochip when the gpiod_get() call is issued, and you have to wait
> > because the gpiochip is not probed yet. We can solve that at runtime
> > right?
> 
> Yes we can if the driver core prevents probing the driver.
> 
> > I had a discussion with Greg the other day that we have no way of
> > expressing inside the kernel that a resource such as a GPIO, a pin,
> > a clk or a regulator is used by some module. It's just a synchronous
> > gpiod_get() or whatever call, then there is a warning if you remove
> > a gpiochip with gpios still in use.
> > 
> > What is needed to make use of such a dependency mechanism is
> > a way to graph the dependencies between kernel drivers and
> > the resources (gpios, clocks, regulators...) they provide to other
> > drivers, so this information can be used when probing, removing,
> > powering up/down the cluster.
> > 
> > That problem needs to be solved in the device core, until then there
> > is not way to actually use that ACPI _DEP property for what I can
> > tell.
> 
> I agree.

Yup.

> > (On a side note: whoever came up with the idea that ACPI props
> > be 4 characters wide and start with an underscore and this
> > backslash obfuscation needs to... think differently.)

That happened 20+ years ago and the goal was to squeeze an object name
into sizeof(int) amount of memory (on a 32-bit system).  That way names
could be represented by ints.  Yes, people were thinking differently at
that time. :-)

Rafael

Re: [PATCH] pinctrl: baytrail: show output gpio state correctly on Intel Baytrail

[Linus Walleij]

On Mon, Nov 3, 2014 at 7:42 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Mon, Nov 03, 2014 at 09:50:11AM -0600, Felipe Balbi wrote:

>> that's an issue that needs solving, but forcing every x86 kernel to ship
>> with this driver, is not a proper solution.
>
> I would rather have the driver build in to the kernel now (and btw it
> has been already in mainline quite some time so I suspect many distros
> have already enabled it), than turning it module and render some devices
> that have been working previously, fail suddenly.

We have an analogous problem with large device tree kernels for ARM
(supporting a multitude of ARM subarchitectures): a lot of statically
compiled-in drivers that just idle around after boot.

What would be nice was for the kernel to have a way of marking some
platform drivers such that if it has not been probed by init_late(),
the entire driver would be discarded, like a module that gets unloaded.

When I posed this idea in some forum it was considered "pretty hard
to achieve" (plus I guess it runs into the dilemma that we can never
discard strings) but I still think it'd be the right thing to do. It'd be very
straight-forward for driver authors to annotate their on-chip platform
drivers with some MODULE_LATE_DISCARD() or whatever.

Yours,
Linus Walleij

Re: [PATCH] pinctrl: baytrail: show output gpio state correctly on Intel Baytrail

[Linus Walleij]

On Fri, Oct 31, 2014 at 2:20 PM, Felipe Balbi <balbi@ti.com> wrote:
> [Me]

>> But another way to get rid of the dilemma is to set
>> .suppress_bind_attrs = true on the .driver field of the
>> device driver. The one can't unbind it through sysfs anymore.
>>
>>         .driver = {
>>                 .name   = "foo",
>>                 .suppress_bind_attrs = true,
>>         },
>>
>> So one of them need to be done.
>>
>> I suspect this is a kind of common problem...
>
> so instead of taking of taking a three-liner which just makes sure this
> can be used as "intended" you prefer to:
>
> diff --git a/drivers/pinctrl/pinctrl-baytrail.c b/drivers/pinctrl/pinctrl-baytrail.c
> index e12e5b0..254ba81 100644
> --- a/drivers/pinctrl/pinctrl-baytrail.c
> +++ b/drivers/pinctrl/pinctrl-baytrail.c
> @@ -587,16 +587,6 @@ static const struct acpi_device_id byt_gpio_acpi_match[] = {
>  };
>  MODULE_DEVICE_TABLE(acpi, byt_gpio_acpi_match);
>
> -static int byt_gpio_remove(struct platform_device *pdev)
> -{
> -       struct byt_gpio *vg = platform_get_drvdata(pdev);
> -
> -       pm_runtime_disable(&pdev->dev);
> -       gpiochip_remove(&vg->chip);
> -
> -       return 0;
> -}
> -
>  static struct platform_driver byt_gpio_driver = {
>         .probe          = byt_gpio_probe,
>         .remove         = byt_gpio_remove,
> @@ -605,6 +595,7 @@ static struct platform_driver byt_gpio_driver = {
>                 .owner  = THIS_MODULE,
>                 .pm     = &byt_gpio_pm_ops,
>                 .acpi_match_table = ACPI_PTR(byt_gpio_acpi_match),
> +               .suppress_bind_attrs = true,
>         },
>  };

I don't know, if the driver *really* cannot be removed from sysfs
it is actually the right solution don't you think?

Else we're just leaving a nicely designed self-destruct
mechanism around.

Yours,
Linus Walleij

Re: [PATCH] pinctrl: baytrail: show output gpio state correctly on Intel Baytrail

[David Cohen]

On Mon, Oct 13, 2014 at 02:26:32PM -0500, Felipe Balbi wrote:
> Hi,
> 
> On Mon, Oct 13, 2014 at 12:24:04PM -0700, David Cohen wrote:
> > On Mon, Oct 13, 2014 at 02:14:05PM -0500, Felipe Balbi wrote:
> > > On Mon, Oct 13, 2014 at 11:23:59AM -0700, David Cohen wrote:
> > > > Even if a gpio pin is set to output, we still need to set INPUT_EN bit
> > > 
> > > here you say you're setting that bit.
> > > 
> > > > to be able to read its value. Without this change, we'll always read low
> > > > level state.
> > > > 
> > > > Cc: <stable@vger.kernel.org> # v3.14+
> > > > Signed-off-by: David Cohen <david.a.cohen@linux.intel.com>
> > > > ---
> > > > 
> > > > Hi,
> > > > 
> > > > I'm resending same v1 patch but now copying linux stable and linux gpio.
> > > > This patch is meant for all linux stable trees >= v3.14.
> > > > 
> > > > Br, David
> > > > ---
> > > > 
> > > >  drivers/pinctrl/pinctrl-baytrail.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/pinctrl/pinctrl-baytrail.c b/drivers/pinctrl/pinctrl-baytrail.c
> > > > index e12e5b07f6d7..c23d8ded936d 100644
> > > > --- a/drivers/pinctrl/pinctrl-baytrail.c
> > > > +++ b/drivers/pinctrl/pinctrl-baytrail.c
> > > > @@ -318,7 +318,7 @@ static int byt_gpio_direction_output(struct gpio_chip *chip,
> > > >  		"Potential Error: Setting GPIO with direct_irq_en to output");
> > > >  
> > > >  	reg_val = readl(reg) | BYT_DIR_MASK;
> > > > -	reg_val &= ~BYT_OUTPUT_EN;
> > > > +	reg_val &= ~(BYT_OUTPUT_EN | BYT_INPUT_EN);
> > > 
> > > but code is clearing it. Also, you're not patching byt_gpio_get(), so
> > > you can't be 'reading its value' either. Quite frankly, this looks very
> > > fishy to me. Comments on BYT_OUTPUT_EN and BYT_INPUT_EN states that
> > > those bits are active low, meaning that clearing them enables that
> > > particular feature.
> > > 
> > > How can you enable a pin to be both OUTPUT and INPUT ? Look like what
> > > you really want is:
> > > 
> > > diff --git a/drivers/pinctrl/pinctrl-baytrail.c b/drivers/pinctrl/pinctrl-baytrail.c
> > > index e12e5b0..69f882d 100644
> > > --- a/drivers/pinctrl/pinctrl-baytrail.c
> > > +++ b/drivers/pinctrl/pinctrl-baytrail.c
> > > @@ -290,6 +290,7 @@ static int byt_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
> > >  
> > >  	value = readl(reg) | BYT_DIR_MASK;
> > >  	value &= ~BYT_INPUT_EN;		/* active low */
> > > +	value |= BYT_OUTPUT_EN;		/* disable output */
> > >  	writel(value, reg);
> > >  
> > >  	spin_unlock_irqrestore(&vg->lock, flags);
> > > @@ -319,6 +320,7 @@ static int byt_gpio_direction_output(struct gpio_chip *chip,
> > >  
> > >  	reg_val = readl(reg) | BYT_DIR_MASK;
> > >  	reg_val &= ~BYT_OUTPUT_EN;
> > > +	value |= BYT_INPUT_EN;		/* disable input */
> > >  
> > >  	if (value)
> > >  		writel(reg_val | BYT_LEVEL, reg);
> > > 
> > > completely untested however. Care to further explain why clearing both
> > > bits - thus enabling both INPUT and OUTPUT - is correct ?
> > 
> > Actually I really meant what I sent, but perhaps I misspoke during
> > explanation. We need to clear the bit to enable the functionality.
> > 
> > !BYT_INPUT_EN allows us to read the value. !BYT_OUTPUT_EN allows us to
> > set a value. If we want to set a value (acting as output) and still be
> > able to read via sysfs, we need to set both functionality (i.e. clear
> > both bits).
> > 
> > I'll resend the patch with a better comment.
> 
> Alright, just make sure that exposing a pin through sysfs and making it
> output high, then later switching it to input works. It looks like

It does. I'm sending this patch based on internal fix I did to read the
value via sysfs. It's already tested and validated :)

> byt_gpio_direction_input() still needs to make sure OUTPUT is set.

That's handled by BYT_DIR_MASK here:

        value = readl(reg) | BYT_DIR_MASK;
        value &= ~BYT_INPUT_EN;         /* active low */

Br, David

[PATCH v2] pinctrl: baytrail: show output gpio state correctly on Intel Baytrail

[David Cohen]

Even if a gpio pin is set to output, we still need to set INPUT_EN
functionality (by clearing INPUT_EN bit) to be able to read the pin's
level.

E.g. without this change, we'll always read low level state from sysfs.

Cc: <stable@vger.kernel.org> # v3.14+
Cc: Mathias Nyman <mathias.nyman@linux.intel.com>
Signed-off-by: David Cohen <david.a.cohen@linux.intel.com>
---

v1 to v2: improved patch's body description as requested by Felipe Balbi
---

 drivers/pinctrl/pinctrl-baytrail.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pinctrl/pinctrl-baytrail.c b/drivers/pinctrl/pinctrl-baytrail.c
index e12e5b07f6d7..c23d8ded936d 100644
--- a/drivers/pinctrl/pinctrl-baytrail.c
+++ b/drivers/pinctrl/pinctrl-baytrail.c
@@ -318,7 +318,7 @@ static int byt_gpio_direction_output(struct gpio_chip *chip,
 		"Potential Error: Setting GPIO with direct_irq_en to output");
 
 	reg_val = readl(reg) | BYT_DIR_MASK;
-	reg_val &= ~BYT_OUTPUT_EN;
+	reg_val &= ~(BYT_OUTPUT_EN | BYT_INPUT_EN);
 
 	if (value)
 		writel(reg_val | BYT_LEVEL, reg);
-- 
2.1.0

Re: [PATCH v2] pinctrl: baytrail: show output gpio state correctly on Intel Baytrail

[Felipe Balbi]

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

On Tue, Oct 14, 2014 at 10:54:37AM -0700, David Cohen wrote:
> Even if a gpio pin is set to output, we still need to set INPUT_EN
> functionality (by clearing INPUT_EN bit) to be able to read the pin's
> level.
> 
> E.g. without this change, we'll always read low level state from sysfs.
> 
> Cc: <stable@vger.kernel.org> # v3.14+
> Cc: Mathias Nyman <mathias.nyman@linux.intel.com>
> Signed-off-by: David Cohen <david.a.cohen@linux.intel.com>

Reviewed-by: Felipe Balbi <balbi@ti.com>

> ---
> 
> v1 to v2: improved patch's body description as requested by Felipe Balbi
> ---
> 
>  drivers/pinctrl/pinctrl-baytrail.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-baytrail.c b/drivers/pinctrl/pinctrl-baytrail.c
> index e12e5b07f6d7..c23d8ded936d 100644
> --- a/drivers/pinctrl/pinctrl-baytrail.c
> +++ b/drivers/pinctrl/pinctrl-baytrail.c
> @@ -318,7 +318,7 @@ static int byt_gpio_direction_output(struct gpio_chip *chip,
>  		"Potential Error: Setting GPIO with direct_irq_en to output");
>  
>  	reg_val = readl(reg) | BYT_DIR_MASK;
> -	reg_val &= ~BYT_OUTPUT_EN;
> +	reg_val &= ~(BYT_OUTPUT_EN | BYT_INPUT_EN);
>  
>  	if (value)
>  		writel(reg_val | BYT_LEVEL, reg);
> -- 
> 2.1.0
> 

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2] pinctrl: baytrail: show output gpio state correctly on Intel Baytrail

[Linus Walleij]

On Tue, Oct 14, 2014 at 7:54 PM, David Cohen
<david.a.cohen@linux.intel.com> wrote:

> Even if a gpio pin is set to output, we still need to set INPUT_EN
> functionality (by clearing INPUT_EN bit) to be able to read the pin's
> level.
>
> E.g. without this change, we'll always read low level state from sysfs.
>
> Cc: <stable@vger.kernel.org> # v3.14+
> Cc: Mathias Nyman <mathias.nyman@linux.intel.com>
> Signed-off-by: David Cohen <david.a.cohen@linux.intel.com>
> ---
>
> v1 to v2: improved patch's body description as requested by Felipe Balbi

This v2 version applied for fixes with Felipe's review tag.

Sorry for slow catching up with email :(

Yours,
Linus Walleij