| Message ID | 1430298162-20238-1-git-send-email-ao2@ao2.it |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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
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
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;
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(-)