| Message ID | CACRpkdbk24kR75fanvXVAocnuozNq_=jWDSPHPCWhbZCOrN5_Q@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 23 April 2013 15:26, Linus Walleij <linus.walleij@linaro.org> wrote: > On Mon, Apr 22, 2013 at 2:58 AM, Haojian Zhuang > <haojian.zhuang@linaro.org> wrote: >> On 22 April 2013 06:23, Mike Dunn <mikedunn@newsguy.com> wrote: >>> Will the big guy see this? Only posted to linux-arm-kernel ML. >>> >>> Thanks Haojian, >>> Mike > > I guess Haojian is referring to me, not Torvalds ... > >> Sorry, I forgot to loop him in. >> >> Linus, >> Should I send the revert commit to you? Or will you revert it directly? > > How hard is it to fix the real problem? > > I'm looking at arch/arm/mach-pxa/palmtreo.c, would > something like this solve the problem: > Your fix could resolve the issue on palmtreo. After grep gpio_request() in arch-pxa, I found that there're a lot of files using gpio_request() in .init_machine(). Regards Haojian
On 04/23/2013 12:26 AM, Linus Walleij wrote: > On Mon, Apr 22, 2013 at 2:58 AM, Haojian Zhuang > <haojian.zhuang@linaro.org> wrote: >> On 22 April 2013 06:23, Mike Dunn <mikedunn@newsguy.com> wrote: >>> Will the big guy see this? Only posted to linux-arm-kernel ML. >>> >>> Thanks Haojian, >>> Mike > > I guess Haojian is referring to me, not Torvalds ... Oops, sorry Linus. Note that the other Linus has the patch in his official tree. [...] > > How hard is it to fix the real problem? > > I'm looking at arch/arm/mach-pxa/palmtreo.c, would > something like this solve the problem: > >>From 5acf99a576f563d48ca5d25b9e5a1bcdd09331eb Mon Sep 17 00:00:00 2001 > From: Linus Walleij <linus.walleij@linaro.org> > Date: Tue, 23 Apr 2013 09:24:43 +0200 > Subject: [PATCH] ARM: pxa: set up Treo GPIOs using device initcall > > This augments the Treo setup to grab LCD GPIOs at device_initcall() > level instead of at init_machine() time, and propagates any > returned errors so that deferred probe will work. Thanks for the patch. I am not having initial success, but will troubleshoot tomorrow morning. On a more general note... I'm surprised that gpiolib can not run earlier, for something so low-level. The kernel documentation on gpio talks about running early. I'm not involved in nor knowledgeable of the pinctrl/gpio work so I won't question the need. I'll probably be able to educate myself some more tomorrow. Thanks again, Mike
On Tue, Apr 23, 2013 at 9:42 PM, Mike Dunn <mikedunn@newsguy.com> wrote: > On a more general note... I'm surprised that gpiolib can not run earlier, for > something so low-level. Well I think the patch we're discussing is making it run less early, and it can actually run as arch_initcall() or similar if you wish (it's up to the driver). However that is beside the point, Haojian is still doing the right thing trying to push this to driver init (==module_init) level. The reason is that basically all hardware drivers should be initialized at this level because the other init levels are basically for other things and driver deps are just shoehorned into them. Instead of relying on things to be set up early one shall rely on deferred probe. Alas, it is not an easy change, and not expected to happen quickly or painlessly. Yours, Linus Walleij
Linus Walleij <linus.walleij@linaro.org> writes: > On Tue, Apr 23, 2013 at 9:42 PM, Mike Dunn <mikedunn@newsguy.com> wrote: > >> On a more general note... I'm surprised that gpiolib can not run earlier, for >> something so low-level. > > Well I think the patch we're discussing is making it run less early, and it > can actually run as arch_initcall() or similar if you wish (it's up to the > driver). > > However that is beside the point, Haojian is still doing the right thing > trying to push this to driver init (==module_init) level. The reason is > that basically all hardware drivers should be initialized at this level > because the other init levels are basically for other things and driver > deps are just shoehorned into them. > > Instead of relying on things to be set up early one shall rely on > deferred probe. Hi Linus, Even if that is the long term goal, and I agree with that goal, let me quote Documentation/gpio.txt : "However, for spinlock-safe GPIOs it's OK to use them before tasking is enabled, as part of early board setup.". When gpio abstraction was written, it was assumed GPIO usage was usable in early platform setup code. As this was granted, we (board maintainers) coded accordingly. Now this patch breaks boards. I disagree in having my board code broken without notice. What I would find less intrusive would be to : (a) revert the patch (b) inform board maintainers that they must fix their board code (for example give them 1 window) (c) put back the patch. Board maintainers who did not amend their board code cannot say anymore they didn't know it. Board maintainers will also have time to rise objections if they think they cannot change their board code (which I do not believe is possible, but who knows ...) Ah and change the Documentation too I think, if you want GPIO to be only usable from device initcall level. > Alas, it is not an easy change, and not expected to happen quickly > or painlessly. Sure, so let's do it in right order, shall we ? Cheers.
On Thu, Apr 25, 2013 at 9:36 PM, Robert Jarzmik <robert.jarzmik@free.fr> wrote: > Now this patch breaks boards. I disagree in having my board code broken without > notice. What I would find less intrusive would be to : > (a) revert the patch This was done by pull request to Torvalds yesterday ... > (b) inform board maintainers that they must fix their board code (for example > give them 1 window) > (c) put back the patch. Board maintainers who did not amend their board > code cannot say anymore they didn't know it. Board maintainers will also have > time to rise objections if they think they cannot change their board code > (which I do not believe is possible, but who knows ...) So when it comes to the PXA there is maybe this list of things that would be nice to change, and this is likely on top of it ... moving the PXA machines over to device tree would be more important if people who like them want them so stay around, because non-device tree platforms will at some point become deprecated, and fixing the platforms to support device tree will inevitably lead to this problem being fixed as well. Yours, Linus Walleij
On 04/25/2013 12:36 PM, Robert Jarzmik wrote: > Linus Walleij <linus.walleij@linaro.org> writes: > >> On Tue, Apr 23, 2013 at 9:42 PM, Mike Dunn <mikedunn@newsguy.com> wrote: >> >>> On a more general note... I'm surprised that gpiolib can not run earlier, for >>> something so low-level. >> >> Well I think the patch we're discussing is making it run less early, and it >> can actually run as arch_initcall() or similar if you wish (it's up to the >> driver). >> >> However that is beside the point, Haojian is still doing the right thing >> trying to push this to driver init (==module_init) level. The reason is >> that basically all hardware drivers should be initialized at this level >> because the other init levels are basically for other things and driver >> deps are just shoehorned into them. >> >> Instead of relying on things to be set up early one shall rely on >> deferred probe. > Hi Linus, > > Even if that is the long term goal, and I agree with that goal, let me quote > Documentation/gpio.txt : "However, for spinlock-safe GPIOs it's OK to use them > before tasking is enabled, as part of early board setup.". > > When gpio abstraction was written, it was assumed GPIO usage was usable in early > platform setup code. As this was granted, we (board maintainers) coded > accordingly. > > Now this patch breaks boards. I disagree in having my board code broken without > notice. What I would find less intrusive would be to : > (a) revert the patch > (b) inform board maintainers that they must fix their board code (for example > give them 1 window) > (c) put back the patch. Board maintainers who did not amend their board > code cannot say anymore they didn't know it. Board maintainers will also have > time to rise objections if they think they cannot change their board code > (which I do not believe is possible, but who knows ...) Yes, thank you Robert. More than a few boards were broken. Old architectures get no respect ;) Some drivers may need rework, not just board support code. For the pxa27x lcd driver (pxafb), I am thinking that the board will have to pass a callback function to the driver by way of platform_data, which the driver will call from its probe function. Advice gratefully received! > Ah and change the Documentation too I think, if you want GPIO to be only usable > from device initcall level. As Linus mentioned, it depends on the driver. There is no standard initcall level for them. Do 'grep initcall drivers/gpio/*.c' and you'll see the variety. This is a problem with the gpio abstraction that should be addressed, at least in the documentation. I wouldn't mind helping, but I'm pretty clueless at this point. Thanks, Mike
Linus Walleij <linus.walleij@linaro.org> writes: > On Thu, Apr 25, 2013 at 9:36 PM, Robert Jarzmik <robert.jarzmik@free.fr> wrote: > This was done by pull request to Torvalds yesterday ... Thanks, that's a relief. > >> (b) inform board maintainers that they must fix their board code (for example >> give them 1 window) >> (c) put back the patch. Board maintainers who did not amend their board >> code cannot say anymore they didn't know it. Board maintainers will also have >> time to rise objections if they think they cannot change their board code >> (which I do not believe is possible, but who knows ...) > > So when it comes to the PXA there is maybe this list of things that > would be nice to change, and this is likely on top of it ... moving the > PXA machines over to device tree would be more important if people > who like them want them so stay around, because non-device tree > platforms will at some point become deprecated, and fixing the platforms > to support device tree will inevitably lead to this problem being fixed > as well. Yeah, probably. That's a good long term goal too. Now you reverted, I'm under pressure to fix mioa701 to get rid of gpio calls in its board setup code :) As for device tree conversion, well that's another story. For my case for example I'll have to redevelop the bootloader (IPL+SPL) in order for suspend to RAM to work ... no world is perfecgt. Cheers.
diff --git a/arch/arm/mach-pxa/palmtreo.c b/arch/arm/mach-pxa/palmtreo.c index d82a50b..669b609 100644 --- a/arch/arm/mach-pxa/palmtreo.c +++ b/arch/arm/mach-pxa/palmtreo.c @@ -455,16 +455,20 @@ static void __init palmphone_common_init(void) } #ifdef CONFIG_MACH_TREO680 -void __init treo680_gpio_init(void) +static int __init treo680_gpio_init(void) { unsigned int gpio; + int ret; /* drive all three lcd gpios high initially */ const unsigned long lcd_flags = GPIOF_INIT_HIGH | GPIOF_DIR_OUT; /* * LCD GPIO initialization... + * only run this on the Treo680 */ + if (!machine_is_treo680()) + return 0; /* * This is likely the power to the lcd. Toggling it low/high appears to @@ -474,8 +478,9 @@ void __init treo680_gpio_init(void) * treo680 configures it as gpio. */ gpio = GPIO_NR_TREO680_LCD_POWER; - if (gpio_request_one(gpio, lcd_flags, "LCD power") < 0) - goto fail; + ret = gpio_request_one(gpio, lcd_flags, "LCD power"); + if (ret < 0) + goto fail_lcd_power; /* * These two are called "enables", for lack of a better understanding. @@ -486,28 +491,33 @@ void __init treo680_gpio_init(void) * revisited. */ gpio = GPIO_NR_TREO680_LCD_EN; - if (gpio_request_one(gpio, lcd_flags, "LCD enable") < 0) - goto fail; + ret = gpio_request_one(gpio, lcd_flags, "LCD enable"); + if (ret < 0) + goto fail_lcd_en; gpio = GPIO_NR_TREO680_LCD_EN_N; - if (gpio_request_one(gpio, lcd_flags, "LCD enable_n") < 0) - goto fail; + ret = gpio_request_one(gpio, lcd_flags, "LCD enable_n"); + if (ret < 0) + goto fail_lcd_en_n; /* driving this low turns LCD on */ gpio_set_value(GPIO_NR_TREO680_LCD_EN_N, 0); - return; - fail: - pr_err("gpio %d initialization failed\n", gpio); - gpio_free(GPIO_NR_TREO680_LCD_POWER); + return 0; + +fail_lcd_en_n: gpio_free(GPIO_NR_TREO680_LCD_EN); - gpio_free(GPIO_NR_TREO680_LCD_EN_N); +fail_lcd_en: + gpio_free(GPIO_NR_TREO680_LCD_POWER); +fail_lcd_power: + pr_err("gpio %d initialization failed\n", gpio); + return ret; } +device_initcall(treo680_gpio_init); static void __init treo680_init(void) { pxa2xx_mfp_config(ARRAY_AND_SIZE(treo680_pin_config)); palmphone_common_init(); - treo680_gpio_init(); palm27x_mmc_init(GPIO_NR_TREO_SD_DETECT_N, GPIO_NR_TREO680_SD_READONLY, GPIO_NR_TREO680_SD_POWER, 0); treo680_docg4_flash_init();