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

[Antonio Ospite <ao2@xxxxxx>]

On Mon, 4 May 2015 13:40:00 +0300
Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx> 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@xxxxxx>
> > > > Cc: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
> > > > Cc: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> > > > Cc: Linus Walleij <linus.walleij@xxxxxxxxxx>
> > > > Cc: linux-gpio@xxxxxxxxxxxxxxx
> > > > ---
> > > >  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?
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html