[PATCH] pinctrl: baytrail: explicitly set gpio chip base

[PATCH] pinctrl: baytrail: explicitly set gpio chip base

[Antonio Ospite]

Having the gpio chip base set explicitly makes it easier to compare the
GPIOs definitions with the ones found on some Android kernels.

Signed-off-by: Antonio Ospite <ao2@ao2.it>
Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: linux-gpio@vger.kernel.org
---
 drivers/pinctrl/intel/pinctrl-baytrail.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/pinctrl/intel/pinctrl-baytrail.c b/drivers/pinctrl/intel/pinctrl-baytrail.c
index 2062c22..4b2f594 100644
--- a/drivers/pinctrl/intel/pinctrl-baytrail.c
+++ b/drivers/pinctrl/intel/pinctrl-baytrail.c
@@ -548,6 +548,7 @@ static int byt_gpio_probe(struct platform_device *pdev)
 	struct acpi_device *acpi_dev;
 	struct pinctrl_gpio_range *range;
 	acpi_handle handle = ACPI_HANDLE(dev);
+	int base_offset;
 	int ret;
 
 	if (acpi_bus_get_device(handle, &acpi_dev))
@@ -559,12 +560,14 @@ static int byt_gpio_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	}
 
+	base_offset = 0;
 	for (range = byt_ranges; range->name; range++) {
 		if (!strcmp(acpi_dev->pnp.unique_id, range->name)) {
 			vg->chip.ngpio = range->npins;
 			vg->range = range;
 			break;
 		}
+		base_offset += range->npins;
 	}
 
 	if (!vg->chip.ngpio || !vg->range)
@@ -590,7 +593,7 @@ static int byt_gpio_probe(struct platform_device *pdev)
 	gc->get = byt_gpio_get;
 	gc->set = byt_gpio_set;
 	gc->dbg_show = byt_gpio_dbg_show;
-	gc->base = -1;
+	gc->base = base_offset;
 	gc->can_sleep = false;
 	gc->dev = dev;
 
-- 
2.1.4

Re: [PATCH] pinctrl: baytrail: explicitly set gpio chip base

[Mika Westerberg]

On Wed, Apr 29, 2015 at 11:02:42AM +0200, Antonio Ospite wrote:
> Having the gpio chip base set explicitly makes it easier to compare the
> GPIOs definitions with the ones found on some Android kernels.
> 
> Signed-off-by: Antonio Ospite <ao2@ao2.it>
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: linux-gpio@vger.kernel.org
> ---
>  drivers/pinctrl/intel/pinctrl-baytrail.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pinctrl/intel/pinctrl-baytrail.c b/drivers/pinctrl/intel/pinctrl-baytrail.c
> index 2062c22..4b2f594 100644
> --- a/drivers/pinctrl/intel/pinctrl-baytrail.c
> +++ b/drivers/pinctrl/intel/pinctrl-baytrail.c
> @@ -548,6 +548,7 @@ static int byt_gpio_probe(struct platform_device *pdev)
>  	struct acpi_device *acpi_dev;
>  	struct pinctrl_gpio_range *range;
>  	acpi_handle handle = ACPI_HANDLE(dev);
> +	int base_offset;
>  	int ret;
>  
>  	if (acpi_bus_get_device(handle, &acpi_dev))
> @@ -559,12 +560,14 @@ static int byt_gpio_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  	}
>  
> +	base_offset = 0;
>  	for (range = byt_ranges; range->name; range++) {
>  		if (!strcmp(acpi_dev->pnp.unique_id, range->name)) {
>  			vg->chip.ngpio = range->npins;
>  			vg->range = range;
>  			break;
>  		}
> +		base_offset += range->npins;
>  	}
>  
>  	if (!vg->chip.ngpio || !vg->range)
> @@ -590,7 +593,7 @@ static int byt_gpio_probe(struct platform_device *pdev)
>  	gc->get = byt_gpio_get;
>  	gc->set = byt_gpio_set;
>  	gc->dbg_show = byt_gpio_dbg_show;
> -	gc->base = -1;
> +	gc->base = base_offset;

But this changes the base from being dynamically allocated to always
start from 0, right?

>  	gc->can_sleep = false;
>  	gc->dev = dev;
>  
> -- 
> 2.1.4

Re: [PATCH] pinctrl: baytrail: explicitly set gpio chip base

[Antonio Ospite]

Hi Mika,

On Thu, 30 Apr 2015 14:02:37 +0300
Mika Westerberg <mika.westerberg@linux.intel.com> wrote:

> On Wed, Apr 29, 2015 at 11:02:42AM +0200, Antonio Ospite wrote:
> > Having the gpio chip base set explicitly makes it easier to compare the
> > GPIOs definitions with the ones found on some Android kernels.
> > 
> > Signed-off-by: Antonio Ospite <ao2@ao2.it>
> > Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> > Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > Cc: Linus Walleij <linus.walleij@linaro.org>
> > Cc: linux-gpio@vger.kernel.org
> > ---
> >  drivers/pinctrl/intel/pinctrl-baytrail.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pinctrl/intel/pinctrl-baytrail.c b/drivers/pinctrl/intel/pinctrl-baytrail.c
> > index 2062c22..4b2f594 100644
> > --- a/drivers/pinctrl/intel/pinctrl-baytrail.c
> > +++ b/drivers/pinctrl/intel/pinctrl-baytrail.c
> > @@ -548,6 +548,7 @@ static int byt_gpio_probe(struct platform_device *pdev)
> >  	struct acpi_device *acpi_dev;
> >  	struct pinctrl_gpio_range *range;
> >  	acpi_handle handle = ACPI_HANDLE(dev);
> > +	int base_offset;
> >  	int ret;
> >  
> >  	if (acpi_bus_get_device(handle, &acpi_dev))
> > @@ -559,12 +560,14 @@ static int byt_gpio_probe(struct platform_device *pdev)
> >  		return -ENOMEM;
> >  	}
> >  
> > +	base_offset = 0;
> >  	for (range = byt_ranges; range->name; range++) {
> >  		if (!strcmp(acpi_dev->pnp.unique_id, range->name)) {
> >  			vg->chip.ngpio = range->npins;
> >  			vg->range = range;
> >  			break;
> >  		}
> > +		base_offset += range->npins;
> >  	}
> >  
> >  	if (!vg->chip.ngpio || !vg->range)
> > @@ -590,7 +593,7 @@ static int byt_gpio_probe(struct platform_device *pdev)
> >  	gc->get = byt_gpio_get;
> >  	gc->set = byt_gpio_set;
> >  	gc->dbg_show = byt_gpio_dbg_show;
> > -	gc->base = -1;
> > +	gc->base = base_offset;
> 
> But this changes the base from being dynamically allocated to always
> start from 0, right?
>

Yes, that's the point: to have exactly the same bases as in the Android
kernels found in the wild, that is:

  $ ls -1 /sys/class/gpio
  export
  gpiochip0
  gpiochip102
  gpiochip130
  unexport

This makes it easier for me to verify the mappings when trying to make
my tablet work with the mainline kernel.

I might as well keep the change local, but I thought that others may
find it useful.

Are there drawbacks of such fixed scheme?

Ciao,
   Antonio

> >  	gc->can_sleep = false;
> >  	gc->dev = dev;
> >  
> > -- 
> > 2.1.4


-- 
Antonio Ospite
http://ao2.it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

Re: [PATCH] pinctrl: baytrail: explicitly set gpio chip base

[Mika Westerberg]

On Thu, Apr 30, 2015 at 11:51:41PM +0200, Antonio Ospite wrote:
> Hi Mika,
> 
> On Thu, 30 Apr 2015 14:02:37 +0300
> Mika Westerberg <mika.westerberg@linux.intel.com> wrote:
> 
> > On Wed, Apr 29, 2015 at 11:02:42AM +0200, Antonio Ospite wrote:
> > > Having the gpio chip base set explicitly makes it easier to compare the
> > > GPIOs definitions with the ones found on some Android kernels.
> > > 
> > > Signed-off-by: Antonio Ospite <ao2@ao2.it>
> > > Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > Cc: Linus Walleij <linus.walleij@linaro.org>
> > > Cc: linux-gpio@vger.kernel.org
> > > ---
> > >  drivers/pinctrl/intel/pinctrl-baytrail.c | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/pinctrl/intel/pinctrl-baytrail.c b/drivers/pinctrl/intel/pinctrl-baytrail.c
> > > index 2062c22..4b2f594 100644
> > > --- a/drivers/pinctrl/intel/pinctrl-baytrail.c
> > > +++ b/drivers/pinctrl/intel/pinctrl-baytrail.c
> > > @@ -548,6 +548,7 @@ static int byt_gpio_probe(struct platform_device *pdev)
> > >  	struct acpi_device *acpi_dev;
> > >  	struct pinctrl_gpio_range *range;
> > >  	acpi_handle handle = ACPI_HANDLE(dev);
> > > +	int base_offset;
> > >  	int ret;
> > >  
> > >  	if (acpi_bus_get_device(handle, &acpi_dev))
> > > @@ -559,12 +560,14 @@ static int byt_gpio_probe(struct platform_device *pdev)
> > >  		return -ENOMEM;
> > >  	}
> > >  
> > > +	base_offset = 0;
> > >  	for (range = byt_ranges; range->name; range++) {
> > >  		if (!strcmp(acpi_dev->pnp.unique_id, range->name)) {
> > >  			vg->chip.ngpio = range->npins;
> > >  			vg->range = range;
> > >  			break;
> > >  		}
> > > +		base_offset += range->npins;
> > >  	}
> > >  
> > >  	if (!vg->chip.ngpio || !vg->range)
> > > @@ -590,7 +593,7 @@ static int byt_gpio_probe(struct platform_device *pdev)
> > >  	gc->get = byt_gpio_get;
> > >  	gc->set = byt_gpio_set;
> > >  	gc->dbg_show = byt_gpio_dbg_show;
> > > -	gc->base = -1;
> > > +	gc->base = base_offset;
> > 
> > But this changes the base from being dynamically allocated to always
> > start from 0, right?
> >
> 
> Yes, that's the point: to have exactly the same bases as in the Android
> kernels found in the wild, that is:
> 
>   $ ls -1 /sys/class/gpio
>   export
>   gpiochip0
>   gpiochip102
>   gpiochip130
>   unexport
> 
> This makes it easier for me to verify the mappings when trying to make
> my tablet work with the mainline kernel.
> 
> I might as well keep the change local, but I thought that others may
> find it useful.
> 
> Are there drawbacks of such fixed scheme?

Well, if you happen to have another GPIO chip (a GPIO expander for
example) and it somehow gets loaded before this driver. It may take the
range you have reserved for the BYT driver.

Not sure how realistic case that is, though...

Re: [PATCH] pinctrl: baytrail: explicitly set gpio chip base

[Antonio Ospite]

On Mon, 4 May 2015 13:40:00 +0300
Mika Westerberg <mika.westerberg@linux.intel.com> wrote:

> On Thu, Apr 30, 2015 at 11:51:41PM +0200, Antonio Ospite wrote:
> > Hi Mika,
> > 
> > On Thu, 30 Apr 2015 14:02:37 +0300
> > Mika Westerberg <mika.westerberg@linux.intel.com> wrote:
> > 
> > > On Wed, Apr 29, 2015 at 11:02:42AM +0200, Antonio Ospite wrote:
> > > > Having the gpio chip base set explicitly makes it easier to compare the
> > > > GPIOs definitions with the ones found on some Android kernels.
> > > > 
> > > > Signed-off-by: Antonio Ospite <ao2@ao2.it>
> > > > Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > > Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > > Cc: Linus Walleij <linus.walleij@linaro.org>
> > > > Cc: linux-gpio@vger.kernel.org
> > > > ---
> > > >  drivers/pinctrl/intel/pinctrl-baytrail.c | 5 ++++-
> > > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/pinctrl/intel/pinctrl-baytrail.c b/drivers/pinctrl/intel/pinctrl-baytrail.c
> > > > index 2062c22..4b2f594 100644
> > > > --- a/drivers/pinctrl/intel/pinctrl-baytrail.c
> > > > +++ b/drivers/pinctrl/intel/pinctrl-baytrail.c
> > > > @@ -548,6 +548,7 @@ static int byt_gpio_probe(struct platform_device *pdev)
> > > >  	struct acpi_device *acpi_dev;
> > > >  	struct pinctrl_gpio_range *range;
> > > >  	acpi_handle handle = ACPI_HANDLE(dev);
> > > > +	int base_offset;
> > > >  	int ret;
> > > >  
> > > >  	if (acpi_bus_get_device(handle, &acpi_dev))
> > > > @@ -559,12 +560,14 @@ static int byt_gpio_probe(struct platform_device *pdev)
> > > >  		return -ENOMEM;
> > > >  	}
> > > >  
> > > > +	base_offset = 0;
> > > >  	for (range = byt_ranges; range->name; range++) {
> > > >  		if (!strcmp(acpi_dev->pnp.unique_id, range->name)) {
> > > >  			vg->chip.ngpio = range->npins;
> > > >  			vg->range = range;
> > > >  			break;
> > > >  		}
> > > > +		base_offset += range->npins;
> > > >  	}
> > > >  
> > > >  	if (!vg->chip.ngpio || !vg->range)
> > > > @@ -590,7 +593,7 @@ static int byt_gpio_probe(struct platform_device *pdev)
> > > >  	gc->get = byt_gpio_get;
> > > >  	gc->set = byt_gpio_set;
> > > >  	gc->dbg_show = byt_gpio_dbg_show;
> > > > -	gc->base = -1;
> > > > +	gc->base = base_offset;
> > > 
> > > But this changes the base from being dynamically allocated to always
> > > start from 0, right?
> > >
> > 
> > Yes, that's the point: to have exactly the same bases as in the Android
> > kernels found in the wild, that is:
> > 
> >   $ ls -1 /sys/class/gpio
> >   export
> >   gpiochip0
> >   gpiochip102
> >   gpiochip130
> >   unexport
> > 
> > This makes it easier for me to verify the mappings when trying to make
> > my tablet work with the mainline kernel.
> > 
> > I might as well keep the change local, but I thought that others may
> > find it useful.
> > 
> > Are there drawbacks of such fixed scheme?
> 
> Well, if you happen to have another GPIO chip (a GPIO expander for
> example) and it somehow gets loaded before this driver. It may take the
> range you have reserved for the BYT driver.
> 
> Not sure how realistic case that is, though...

Indeed, being this for the SoC gpio controller I thought it was
unlikely, however I do not have a huge experience on these matters.

I have no strong opinion on this, Mika, so whether or not you merge the
change it'll be fine by me.

Thanks,
   Antonio

-- 
Antonio Ospite
http://ao2.it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

Re: [PATCH] pinctrl: baytrail: explicitly set gpio chip base

[Linus Walleij]

On Mon, May 4, 2015 at 1:36 PM, Antonio Ospite <ao2@ao2.it> wrote:
> On Mon, 4 May 2015 13:40:00 +0300 Mika Westerberg <mika.westerberg@linux.intel.com> wrote:

>> Well, if you happen to have another GPIO chip (a GPIO expander for
>> example) and it somehow gets loaded before this driver. It may take the
>> range you have reserved for the BYT driver.
>>
>> Not sure how realistic case that is, though...
>
> Indeed, being this for the SoC gpio controller I thought it was
> unlikely, however I do not have a huge experience on these matters.
>
> I have no strong opinion on this, Mika, so whether or not you merge the
> change it'll be fine by me.

The ability to set .base is basically there for legacy reasons,
and the critical legacy case is usually when setting it to 0
for the on-SoC GPIO.

In the long run we want to get rid of static GPIO numbers
altogether so I prefer if you construct your userspace to
traverse sysfs to get the GPIOs you need to get at,
sysfs is horrible and unreliable for GPIO.

Yours,
Linus Walleij

Re: [PATCH] pinctrl: baytrail: explicitly set gpio chip base

[Antonio Ospite]

On Tue, 12 May 2015 09:48:12 +0200
Linus Walleij <linus.walleij@linaro.org> wrote:

> On Mon, May 4, 2015 at 1:36 PM, Antonio Ospite <ao2@ao2.it> wrote:
> > On Mon, 4 May 2015 13:40:00 +0300 Mika Westerberg <mika.westerberg@linux.intel.com> wrote:
> 
> >> Well, if you happen to have another GPIO chip (a GPIO expander for
> >> example) and it somehow gets loaded before this driver. It may take the
> >> range you have reserved for the BYT driver.
> >>
> >> Not sure how realistic case that is, though...
> >
> > Indeed, being this for the SoC gpio controller I thought it was
> > unlikely, however I do not have a huge experience on these matters.
> >
> > I have no strong opinion on this, Mika, so whether or not you merge the
> > change it'll be fine by me.
> 
> The ability to set .base is basically there for legacy reasons,
> and the critical legacy case is usually when setting it to 0
> for the on-SoC GPIO.
>
> In the long run we want to get rid of static GPIO numbers
> altogether so I prefer if you construct your userspace to
> traverse sysfs to get the GPIOs you need to get at,
> sysfs is horrible and unreliable for GPIO.
> 

OK, if the use of static .base is deprecated my patch is inappropriate,
I self-NACK it and we can close this thread.

Thanks for the replies,
   Antonio

-- 
Antonio Ospite
http://ao2.it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?