pinctrl: baytrail: explicitly set gpio chip base
diff mbox

Message ID 1430298162-20238-1-git-send-email-ao2@ao2.it
State New
Headers show

Commit Message

Antonio Ospite April 29, 2015, 9:02 a.m. UTC
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(-)

Comments

Mika Westerberg April 30, 2015, 11:02 a.m. UTC | #1
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
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Antonio Ospite April 30, 2015, 9:51 p.m. UTC | #2
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
Mika Westerberg May 4, 2015, 10:40 a.m. UTC | #3
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...
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Antonio Ospite May 4, 2015, 11:36 a.m. UTC | #4
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
Linus Walleij May 12, 2015, 7:48 a.m. UTC | #5
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
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Antonio Ospite May 12, 2015, 1:07 p.m. UTC | #6
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

Patch
diff mbox

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;