| Message ID | 20141031132005.GB1273@saruman |
|---|---|
| State | Not Applicable |
| Headers | show |
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 -- 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 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 > > -- 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 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. -- 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, 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.
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. -- 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 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 -- 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, 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.
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. -- 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, 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.
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. -- 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, 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.
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. -- 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, 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. -- 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, 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? -- 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, 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.
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 -- 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, 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. -- 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, 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 -- 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, 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. -- 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, 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 -- 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, 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... -- 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, 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 -- 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, 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 -- 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, 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 -- 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
+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 -- 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 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 -- 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
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, }, };