Re: [PATCH 05/33] gpio: add generic single-register fixed-direction GPIO driver
On Thu, Sep 8, 2016 at 3:21 PM, Linus Walleijwrote: > On Mon, Sep 5, 2016 at 2:26 PM, Russell King - ARM Linux > wrote: >> On Mon, Sep 05, 2016 at 11:06:28AM +0200, Linus Walleij wrote: >>> I couldn't resist testing on the Compaq iPAQ h3600. It works the >>> same as before so: >>> Tested-by: Linus Walleij [for Compaq iPAQ H3600] >> >> Great news. I've been thinking about digging out my h3600, but it's >> very old, and hasn't been turned on for many years. I'm not sure what >> state it's in. >> >> I've been hoping to try booting some kernels with qemu-system-arm, but >> so far I've completely failed to get qemu-system-arm to do anything >> useful - it just sits there doing apparently nothing, irrespective of >> which platform I choose or which kernel I give it. >> >>> The only news in the bootlog is this: >>> sa11x0-pcmcia: probe of sa11x0-pcmcia failed with error -2 >> >> Not so great news - that's -ENOENT. Did that happen before these >> changes? That could be that the gpiod lookup table isn't found. >> However, if that were the case, I'd have expected an error message >> along the lines of: >> >> Failed to get GPIO for xxx: -nnn >> >> from soc_pcmcia_request_gpiods(). The other possibility is that >> we're not getting to sa11x0_drv_pcmcia_legacy_probe() but instead >> trying to initialise it as a generic sa11x0 socket, and >> sa11x0_pcmcia_hw_init() is failing as a result. >> >> We should be using the legacy probe on H3600, so sa11x0_pcmcia_hw_init() >> should never be reached. > > However that is what happens, this is my callstack after > adding some prints: > > sa11x0_pcmcia_init > sa11x0_drv_pcmcia_probe > soc_pcmcia_init_one > soc_pcmcia_add_one > soc_pcmcia_hw_init > sa11x0_pcmcia_hw_init > soc_pcmcia_add_one: pcmcia HW init failed > sa11x0-pcmcia: probe of sa11x0-pcmcia failed with error -2 Bah I found the cause, just a simple oneliner typo in one of the patches. I will comment on the patch in question so you can fix it up on your branch. Yours, Linus Walleij ___ Linux PCMCIA reimplementation list http://lists.infradead.org/mailman/listinfo/linux-pcmcia
Re: [PATCH 05/33] gpio: add generic single-register fixed-direction GPIO driver
On Mon, Sep 5, 2016 at 2:26 PM, Russell King - ARM Linuxwrote: > On Mon, Sep 05, 2016 at 11:06:28AM +0200, Linus Walleij wrote: >> I couldn't resist testing on the Compaq iPAQ h3600. It works the >> same as before so: >> Tested-by: Linus Walleij [for Compaq iPAQ H3600] > > Great news. I've been thinking about digging out my h3600, but it's > very old, and hasn't been turned on for many years. I'm not sure what > state it's in. > > I've been hoping to try booting some kernels with qemu-system-arm, but > so far I've completely failed to get qemu-system-arm to do anything > useful - it just sits there doing apparently nothing, irrespective of > which platform I choose or which kernel I give it. > >> The only news in the bootlog is this: >> sa11x0-pcmcia: probe of sa11x0-pcmcia failed with error -2 > > Not so great news - that's -ENOENT. Did that happen before these > changes? That could be that the gpiod lookup table isn't found. > However, if that were the case, I'd have expected an error message > along the lines of: > > Failed to get GPIO for xxx: -nnn > > from soc_pcmcia_request_gpiods(). The other possibility is that > we're not getting to sa11x0_drv_pcmcia_legacy_probe() but instead > trying to initialise it as a generic sa11x0 socket, and > sa11x0_pcmcia_hw_init() is failing as a result. > > We should be using the legacy probe on H3600, so sa11x0_pcmcia_hw_init() > should never be reached. However that is what happens, this is my callstack after adding some prints: sa11x0_pcmcia_init sa11x0_drv_pcmcia_probe soc_pcmcia_init_one soc_pcmcia_add_one soc_pcmcia_hw_init sa11x0_pcmcia_hw_init soc_pcmcia_add_one: pcmcia HW init failed sa11x0-pcmcia: probe of sa11x0-pcmcia failed with error -2 It bails out when trying to get the "reset" gpio. I'm trying to figure out how this can happen. On the plus side: this minor snag is all the problems I have. All other GPIOs work exactly as they should. Yours, Linus Walleij ___ Linux PCMCIA reimplementation list http://lists.infradead.org/mailman/listinfo/linux-pcmcia
Re: [PATCH 05/33] gpio: add generic single-register fixed-direction GPIO driver
On Mon, Sep 05, 2016 at 11:06:28AM +0200, Linus Walleij wrote: > I couldn't resist testing on the Compaq iPAQ h3600. It works the > same as before so: > Tested-by: Linus Walleij[for Compaq iPAQ H3600] Great news. I've been thinking about digging out my h3600, but it's very old, and hasn't been turned on for many years. I'm not sure what state it's in. I've been hoping to try booting some kernels with qemu-system-arm, but so far I've completely failed to get qemu-system-arm to do anything useful - it just sits there doing apparently nothing, irrespective of which platform I choose or which kernel I give it. > The only news in the bootlog is this: > sa11x0-pcmcia: probe of sa11x0-pcmcia failed with error -2 Not so great news - that's -ENOENT. Did that happen before these changes? That could be that the gpiod lookup table isn't found. However, if that were the case, I'd have expected an error message along the lines of: Failed to get GPIO for xxx: -nnn from soc_pcmcia_request_gpiods(). The other possibility is that we're not getting to sa11x0_drv_pcmcia_legacy_probe() but instead trying to initialise it as a generic sa11x0 socket, and sa11x0_pcmcia_hw_init() is failing as a result. We should be using the legacy probe on H3600, so sa11x0_pcmcia_hw_init() should never be reached. > This device does have a PCMCIA "sleeve" where I slotted in an > ethernet card at one time to see if I could network this thing. > But I never got the PCMCIA working. Now it seems like I could > actually start looking into that as the driver gives its first sign > of life. > > I don't think PCMCIA ever worked on this thing upstream, > but I have a copy of the old linux-handheld kernel tree where > it supposedly was working at one point. Well, the H3600 PCMCIA driver raises lots of "that can't be right" questions for me, so I decided to leave most of the GPIO stuff alone there, and just convert only the bits which made sense to me - the detect and ready(irq) bits. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ___ Linux PCMCIA reimplementation list http://lists.infradead.org/mailman/listinfo/linux-pcmcia
Re: [PATCH 05/33] gpio: add generic single-register fixed-direction GPIO driver
On Wed, Aug 31, 2016 at 10:49 AM, Russell King - ARM Linuxwrote: > git://git.armlinux.org.uk/~rmk/linux-arm.git sa1100 > > It would be great to have this tested on Lubbock, and get the PCMCIA > issues fixed. Maybe we can look at converting mainstone as well? I couldn't resist testing on the Compaq iPAQ h3600. It works the same as before so: Tested-by: Linus Walleij [for Compaq iPAQ H3600] The only news in the bootlog is this: sa11x0-pcmcia: probe of sa11x0-pcmcia failed with error -2 This device does have a PCMCIA "sleeve" where I slotted in an ethernet card at one time to see if I could network this thing. But I never got the PCMCIA working. Now it seems like I could actually start looking into that as the driver gives its first sign of life. I don't think PCMCIA ever worked on this thing upstream, but I have a copy of the old linux-handheld kernel tree where it supposedly was working at one point. Yours, Linus Walleij ___ Linux PCMCIA reimplementation list http://lists.infradead.org/mailman/listinfo/linux-pcmcia
Re: [PATCH 05/33] gpio: add generic single-register fixed-direction GPIO driver
On Fri, Sep 2, 2016 at 7:00 PM, Russell King - ARM Linuxwrote: > Linus, > > There's a change I'd like to merge into patch 5 - overall it looks > like the below, and allows us to use gpio-reg with the PXA mainstone > MST_PCMCIA[01] registers. Some of these GPIO signals have hardware > interrupts associated with them, but not all. > > Do you approve? Yes of course. Acked-by: Linus Walleij Yours, Linus Walleij ___ Linux PCMCIA reimplementation list http://lists.infradead.org/mailman/listinfo/linux-pcmcia
Re: [PATCH 05/33] gpio: add generic single-register fixed-direction GPIO driver
On Sun, Sep 04, 2016 at 09:04:59PM +0200, Robert Jarzmik wrote: > And retested with my 3 patches on top of it. Everything works fine : > - CF insertion is correctly detected now ! > - AT/2 keyboard interrupts fire, keys are there, etc ... > - the AT/2 warning is now gone > > The pxa_cplds_irqs patch will make it work as before, better than today for > sure. I think edge type interrupts (such as sa), if they come close > enough, > might be lost with the current implementation, but anyway, that's for another > time. Good news, but I suspect CF probably isn't working as it should do (see my reply to the clock alias patch.) > There is one patch in my serie that I left up to you, as I wasn't very sure > about the accuracy of my commit message, nor been able to write down all the > requirements implied on sa with this additional test. That's basically what I have here at the moment, but since I made the suggestion, I've been thinking about moving it into the IRQ initialisation instead. Also, I think it's a genirq problem that you can install a chained handler on an interrupt without a registered chip - and then when an irq chip comes along later, the IRQ isn't unmasked. We should either not allow a chained handler to be installed in that situation, /or/ we should unmask chained IRQs when the chip comes along later. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ___ Linux PCMCIA reimplementation list http://lists.infradead.org/mailman/listinfo/linux-pcmcia
Re: [PATCH 05/33] gpio: add generic single-register fixed-direction GPIO driver
Russell King - ARM Linuxwrites: > On Wed, Aug 31, 2016 at 09:49:38AM +0100, Russell King - ARM Linux wrote: >> On Tue, Aug 30, 2016 at 11:32:16PM +0200, Robert Jarzmik wrote: >> > Russell King - ARM Linux writes: >> > >> > > If you can wait a day or two, I'll push a branch out for everything in >> > > all these multiple series. >> > Sure, just ping me when you have something. >> >> git://git.armlinux.org.uk/~rmk/linux-arm.git sa1100 >> >> should get you something suitable to test. It's based on 4.7-rc3 plus >> my fixes branch. > > Branch updated, hopefully with everything that we've agreed on so far, > all the bug fixes, plus a few extra bits... And retested with my 3 patches on top of it. Everything works fine : - CF insertion is correctly detected now ! - AT/2 keyboard interrupts fire, keys are there, etc ... - the AT/2 warning is now gone The pxa_cplds_irqs patch will make it work as before, better than today for sure. I think edge type interrupts (such as sa), if they come close enough, might be lost with the current implementation, but anyway, that's for another time. There is one patch in my serie that I left up to you, as I wasn't very sure about the accuracy of my commit message, nor been able to write down all the requirements implied on sa with this additional test. Cheers. -- Robert ___ Linux PCMCIA reimplementation list http://lists.infradead.org/mailman/listinfo/linux-pcmcia
Re: [PATCH 05/33] gpio: add generic single-register fixed-direction GPIO driver
Russell King - ARM Linuxwrites: > On Wed, Aug 31, 2016 at 09:49:38AM +0100, Russell King - ARM Linux wrote: >> On Tue, Aug 30, 2016 at 11:32:16PM +0200, Robert Jarzmik wrote: >> > Russell King - ARM Linux writes: >> > >> > > If you can wait a day or two, I'll push a branch out for everything in >> > > all these multiple series. >> > Sure, just ping me when you have something. >> >> git://git.armlinux.org.uk/~rmk/linux-arm.git sa1100 >> >> should get you something suitable to test. It's based on 4.7-rc3 plus >> my fixes branch. > > Branch updated, hopefully with everything that we've agreed on so far, > all the bug fixes, plus a few extra bits... Good, I'll retest within the weekend. >> It would be great to have this tested on Lubbock, and get the PCMCIA >> issues fixed. Maybe we can look at converting mainstone as well? > > ... like converting mainstone and pxaficp_ir on mainstone. > > I'm not going to be around for most of the rest of this weekend. Okay. I'll repost on Monday for the latest branch test results and your idea about the irq_get_chip() workaround. Good week-end. -- Robert ___ Linux PCMCIA reimplementation list http://lists.infradead.org/mailman/listinfo/linux-pcmcia
Re: [PATCH 05/33] gpio: add generic single-register fixed-direction GPIO driver
On Fri, Sep 02, 2016 at 11:21:12PM +0200, Robert Jarzmik wrote: > It looks that I have an ordering problem : > - I want gpio-pxa.probe() to be called at device initcall time > - pxa_cplds_irqs.probe() cannot complete before gpio-pxa.probe() because it >needs GPIO0 as its interrupt source >=> it might need to honor -EPROBE_DEFER > - sa.sa_probe() cannot complete before pxa_cplds_irqs.probe() because >it needs IRQ305 as its source >=> it might need to honor -EPROBE_DEFER I wonder if we can work around that by adding this to sa_probe(): if (!irq_get_chip(irq)) return -EPROBE_DEFER; It's not particularly nice, but it should at least ensure that things happen in the right order. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ___ Linux PCMCIA reimplementation list http://lists.infradead.org/mailman/listinfo/linux-pcmcia
Re: [PATCH 05/33] gpio: add generic single-register fixed-direction GPIO driver
On Thu, Sep 01, 2016 at 09:19:13AM +0200, Robert Jarzmik wrote: > >From 977c16201a752aac8a8fb2da1f4271795f0b2122 Mon Sep 17 00:00:00 2001 > From: Robert Jarzmik> Date: Thu, 1 Sep 2016 08:31:08 +0200 > Subject: [PATCH] pcmcia: lubbock: fix sockets configuration > > On lubbock board, the probe of the driver crashes by dereferencing very > early a platform_data structure which is not set, in > pxa2xx_configure_sockets(). Patch applied, with some fixups for the error code propagation changes and edited the patch description a little. Thanks. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ___ Linux PCMCIA reimplementation list http://lists.infradead.org/mailman/listinfo/linux-pcmcia
Re: [PATCH 05/33] gpio: add generic single-register fixed-direction GPIO driver
On Fri, Sep 02, 2016 at 07:50:35PM +0200, Robert Jarzmik wrote: > Russell King - ARM Linuxwrites: > >> Moreover, I have a bit of homework as I also see : > >> - no SA interrupts at all, especially nothing when I insert/remove my > >>CompactFlash card > >>This might be an effect of pxa_cplds_irqs.c I created, I must have a > >> look. > > > > Do you get other SA interrupts (eg, the PS/2 interrupts) delivered? > I see no other interrupts at all. > Actually I see no interrupt claimed in /proc/interrupts, where I would have > expected interrupt 305. > cat /proc/interrupts > CPU0 >24: 1419SC 8 Edge gpio-0 >25: 0SC 9 Edge gpio-1 >26: 0SC 10 Edge gpio-mux >38:118SC 22 Edge UART1 >41: 0SC 25 Edge DMA >42: 40224SC 26 Edge ost0 > 112: 1419 GPIO 0 Edge pxa_cplds_irqs > 307: 1419 pxa_cplds 3 Edge eth0 > 387: 0 SA-h Edge SA PCMCIA card detect > 388: 0 SA-h Edge SA CF card detect > 389: 0 SA-h Edge SA PCMCIA BVD1 > 390: 0 SA-h Edge SA CF BVD1 > Err: 0 > > Actually this leads me to think that this interrupt 305 is not "requested" nor > activated. I see in sa.c:506 : > "irq_set_chained_handler_and_data(sachip->irq, sa_irq_handler, ..." > This puts in the handler and data, but I don't this is "enables" the > interrupt, > right ? It should enable the interrupt - the end of that does: if (handle != handle_bad_irq && is_chained) { irq_settings_set_noprobe(desc); irq_settings_set_norequest(desc); irq_settings_set_nothread(desc); desc->action = _action; irq_startup(desc, true); } and indeed, I see that it gets enabled on Assabet. That irq_startup() should result in the irqchip's irq_startup, irq_enable, or irq_unmask method being called. So, that should result in the IRQ to which the sa is connected being unmasked. Chained interrupts don't appear in /proc/interrupts. > I traced the code in the interrupt controller (pxa_cplds_irqs), the SA > physical line is not set (even with an AT/2 keyboard connected, and I don't > think anybody tried this AT/2 on a lubbock for a long time). Hmm. Looking at pxa_cplds_irqs, that's trying to support the CPLD interrupts using a very very old and inefficient technique. The whole point of the chained interrupt system is to avoid several overheads in the system... I'm curious why PXA moved away from that. One of the problems is that we end up nesting irq_entry()/irq_exit() which contains a lot of code, eg trying to run softirqs. All that stuff is pure overhead because we'll never get to run anything like that in this path. I'm also very concerned that the conversion is wrong. The old code has this comment in the irq_unmask function: - /* the irq can be acknowledged only if deasserted, so it's done here */ - LUB_IRQ_SET_CLR &= ~(1 << lubbock_irq); In other words, we "acknowledge" (really clear the latched status) of the interrupt _after_ we've serviced the peripheral, not before. The new code tries to clear down the interrupt when masking it - in other words, before the peripheral handler has had a chance to clear down its interrupt. I suspect you're seeing about twice as many interrupt counts on your ethernet interface because of this - you'll be entering the handler once for the real interrupt, but because the clear-down was ineffective, you end up re-entering it a second time when hopefully the peripheral is no longer asserting the interrupt. > The interrupt is for sure masked, and therefore the SA interrupts are not > fired. Even if they would have been enabled, the "pending interrupts register" > doesn't show any sa interrupt pending, so there is something else. Masked where though - in the SA or FPGA? > As I don't know if "enable_irq()" in sa.c would be an option as I have no > sight on sa.c requirements, I would take any advice here. It shouldn't be required, as I say above, irq_set_chained_handler_and_data() deals with unmasking the IRQ already. > > Hmm, on Neponset with a CF card inserted, I can cat that, and it reports > > the CIS. Any error messages in the kernel log? > None. > I have an ever more suprising thing. > I tried again this morning, without changing a single line of code (excepting > in > pxa_cplds_irqs.c) nor touching the CF card, and now it really rocks !!! : > hexdump -C /sys/class/pcmcia_socket/pcmcia_socket1/cis > hexdump -C /sys/class/pcmcia_socket/pcmcia_socket1/cis > 01 04 df 79 01 ff 1c 04 02 db 01 ff 18 02 df 01 |...y| > 0010 20 04 45 00 01 04 15 0b
Re: [PATCH 05/33] gpio: add generic single-register fixed-direction GPIO driver
Russell King - ARM Linuxwrites: >> You're right, I submitted a patch for that, and I confirm it actually >> happens on >> lubbock. > > That'll work fine for lubbock, but not the others (we can have several > of the others enabled on sa11x0 platforms - eg, badge4 with neponset.) Ah, I see. Let's drop the patch then, I won't be able to know what to return in this function if 2 initialisations fail, nor if the first failure should be fatal or not, nor if a rollback is possible after 1 success (badge4) and 1 failure (neponset). > Ignore those for now - the old ARM IRQ stuff was silent on that, but genirq > is more noisy. I should probably make the sa irqchip handle the both- > edge case itself. Ok. >> Moreover, I have a bit of homework as I also see : >> - no SA interrupts at all, especially nothing when I insert/remove my >>CompactFlash card >>This might be an effect of pxa_cplds_irqs.c I created, I must have a look. > > Do you get other SA interrupts (eg, the PS/2 interrupts) delivered? I see no other interrupts at all. Actually I see no interrupt claimed in /proc/interrupts, where I would have expected interrupt 305. cat /proc/interrupts CPU0 24: 1419SC 8 Edge gpio-0 25: 0SC 9 Edge gpio-1 26: 0SC 10 Edge gpio-mux 38:118SC 22 Edge UART1 41: 0SC 25 Edge DMA 42: 40224SC 26 Edge ost0 112: 1419 GPIO 0 Edge pxa_cplds_irqs 307: 1419 pxa_cplds 3 Edge eth0 387: 0 SA-h Edge SA PCMCIA card detect 388: 0 SA-h Edge SA CF card detect 389: 0 SA-h Edge SA PCMCIA BVD1 390: 0 SA-h Edge SA CF BVD1 Err: 0 Actually this leads me to think that this interrupt 305 is not "requested" nor activated. I see in sa.c:506 : "irq_set_chained_handler_and_data(sachip->irq, sa_irq_handler, ..." This puts in the handler and data, but I don't this is "enables" the interrupt, right ? I traced the code in the interrupt controller (pxa_cplds_irqs), the SA physical line is not set (even with an AT/2 keyboard connected, and I don't think anybody tried this AT/2 on a lubbock for a long time). The interrupt is for sure masked, and therefore the SA interrupts are not fired. Even if they would have been enabled, the "pending interrupts register" doesn't show any sa interrupt pending, so there is something else. As I don't know if "enable_irq()" in sa.c would be an option as I have no sight on sa.c requirements, I would take any advice here. > Hmm, on Neponset with a CF card inserted, I can cat that, and it reports > the CIS. Any error messages in the kernel log? None. I have an ever more suprising thing. I tried again this morning, without changing a single line of code (excepting in pxa_cplds_irqs.c) nor touching the CF card, and now it really rocks !!! : hexdump -C /sys/class/pcmcia_socket/pcmcia_socket1/cis hexdump -C /sys/class/pcmcia_socket/pcmcia_socket1/cis 01 04 df 79 01 ff 1c 04 02 db 01 ff 18 02 df 01 |...y| 0010 20 04 45 00 01 04 15 0b 04 01 00 43 46 43 41 52 | .ECFCAR| 0020 44 00 ff 21 02 04 01 22 02 01 01 22 03 02 0c 0f |D..!..."..."| 0030 1a 05 01 03 00 02 0f 1b 08 c0 40 a1 01 55 08 00 |..@..U..| 0040 20 1b 06 00 01 21 b5 1e 4d 1b 0a c1 41 99 01 55 | !..M...A..U| 0050 64 f0 ff ff 20 1b 06 01 01 21 b5 1e 4d 1b 0f c2 |d... !..M...| 0060 41 99 01 55 ea 61 f0 01 07 f6 03 01 ee 20 1b 06 |A..U.a... ..| 0070 02 01 21 b5 1e 4d 1b 0f c3 41 99 01 55 ea 61 70 |..!..M...A..U.ap| 0080 01 07 76 03 01 ee 20 1b 06 03 01 21 b5 1e 4d 14 |..v... !..M.| 0090 00|.| I think this proves that your patches related to lubbock pcmcia conversion to max1602 is fully functional ! Only the interrupt part to fight a bit, and then I'll try to make a couple of tests on the IRDA part. Cheers. -- Robert ___ Linux PCMCIA reimplementation list http://lists.infradead.org/mailman/listinfo/linux-pcmcia
Re: [PATCH 05/33] gpio: add generic single-register fixed-direction GPIO driver
Linus, There's a change I'd like to merge into patch 5 - overall it looks like the below, and allows us to use gpio-reg with the PXA mainstone MST_PCMCIA[01] registers. Some of these GPIO signals have hardware interrupts associated with them, but not all. Do you approve? Thanks. 8< From: Russell KingSubject: [PATCH] gpio: gpio-reg: add irq mapping for gpio-reg users Add support for mapping gpio-reg gpios to interrupts. This may be a non-linear mapping - some gpios in the register may not even have corresponding interrupts associated with them, so we need to pass an array. Signed-off-by: Russell King --- arch/arm/mach-pxa/lubbock.c | 2 +- arch/arm/mach-sa1100/assabet.c | 2 +- arch/arm/mach-sa1100/neponset.c | 2 +- drivers/gpio/gpio-reg.c | 25 +++-- include/linux/gpio/gpio-reg.h | 3 ++- 5 files changed, 28 insertions(+), 6 deletions(-) diff --git a/arch/arm/mach-pxa/lubbock.c b/arch/arm/mach-pxa/lubbock.c index 1f7bfabfa4eb..cd401546cea8 100644 --- a/arch/arm/mach-pxa/lubbock.c +++ b/arch/arm/mach-pxa/lubbock.c @@ -477,7 +477,7 @@ static void __init lubbock_init(void) lubbock_misc_wr_gc = gpio_reg_init(NULL, (void *)_MISC_WR, -1, 16, "lubbock", 0, LUB_MISC_WR, - NULL); + NULL, NULL, NULL); if (IS_ERR(lubbock_misc_wr_gc)) { pr_err("Lubbock: unable to register lubbock GPIOs: %ld\n", PTR_ERR(lubbock_misc_wr_gc)); diff --git a/arch/arm/mach-sa1100/assabet.c b/arch/arm/mach-sa1100/assabet.c index 51950454afac..e086c609551c 100644 --- a/arch/arm/mach-sa1100/assabet.c +++ b/arch/arm/mach-sa1100/assabet.c @@ -120,7 +120,7 @@ static int __init assabet_init_gpio(void __iomem *reg, u32 def_val) writel_relaxed(def_val, reg); gc = gpio_reg_init(NULL, reg, -1, 32, "assabet", 0xff00, def_val, - assabet_names); + assabet_names, NULL, NULL); if (IS_ERR(gc)) return PTR_ERR(gc); diff --git a/arch/arm/mach-sa1100/neponset.c b/arch/arm/mach-sa1100/neponset.c index 5abc04fb196d..b16de1f0d5d0 100644 --- a/arch/arm/mach-sa1100/neponset.c +++ b/arch/arm/mach-sa1100/neponset.c @@ -240,7 +240,7 @@ static int neponset_init_gpio(struct gpio_chip **gcp, struct gpio_chip *gc; gc = gpio_reg_init(dev, reg, -1, num, label, in ? 0x : 0, - readl_relaxed(reg), names); + readl_relaxed(reg), names, NULL, NULL); if (IS_ERR(gc)) return PTR_ERR(gc); diff --git a/drivers/gpio/gpio-reg.c b/drivers/gpio/gpio-reg.c index 561e28b4d3aa..39175d373871 100644 --- a/drivers/gpio/gpio-reg.c +++ b/drivers/gpio/gpio-reg.c @@ -19,6 +19,8 @@ struct gpio_reg { u32 direction; u32 out; void __iomem *reg; + struct irq_domain *irqdomain; + const int *irqs; }; #define to_gpio_reg(x) container_of(x, struct gpio_reg, gc) @@ -96,6 +98,17 @@ static void gpio_reg_set_multiple(struct gpio_chip *gc, unsigned long *mask, spin_unlock_irqrestore(>lock, flags); } +static int gpio_reg_to_irq(struct gpio_chip *gc, unsigned offset) +{ + struct gpio_reg *r = to_gpio_reg(gc); + int irq = r->irqs[offset]; + + if (irq >= 0 && r->irqdomain) + irq = irq_find_mapping(r->irqdomain, irq); + + return irq; +} + /** * gpio_reg_init - add a fixed in/out register as gpio * @dev: optional struct device associated with this register @@ -104,7 +117,12 @@ static void gpio_reg_set_multiple(struct gpio_chip *gc, unsigned long *mask, * @label: GPIO chip label * @direction: bitmask of fixed direction, one per GPIO signal, 1 = in * @def_out: initial GPIO output value - * @names: array of %num strings describing each GPIO signal + * @names: array of %num strings describing each GPIO signal or %NULL + * @irqdom: irq domain or %NULL + * @irqs: array of %num ints describing the interrupt mapping for each + *GPIO signal, or %NULL. If @irqdom is %NULL, then this + *describes the Linux interrupt number, otherwise it describes + *the hardware interrupt number in the specified irq domain. * * Add a single-register GPIO device containing up to 32 GPIO signals, * where each GPIO has a fixed input or output configuration. Only @@ -114,7 +132,7 @@ static void gpio_reg_set_multiple(struct gpio_chip *gc, unsigned long *mask, */ struct gpio_chip *gpio_reg_init(struct device *dev, void __iomem *reg, int base, int num, const char *label, u32 direction, u32 def_out, - const char *const *names) + const char *const *names, struct irq_domain *irqdom, const int *irqs) { struct gpio_reg *r; int ret; @@ -136,12 +154,15 @@ struct gpio_chip *gpio_reg_init(struct device
Re: [PATCH 05/33] gpio: add generic single-register fixed-direction GPIO driver
On Thu, Sep 01, 2016 at 11:58:28PM +0200, Robert Jarzmik wrote: > Russell King - ARM Linuxwrites: > > On Thu, Sep 01, 2016 at 09:19:13AM +0200, Robert Jarzmik wrote: > > It looks like: > > > > (a) pcmcia_probe() in drivers/pcmcia/sa_generic.c doesn't check the > > return value from the platform specific init functions, meaning if > > they fail, the driver still binds. (note: they return -ENODEV to > > indicate that they should skip to the next platform.) > You're right, I submitted a patch for that, and I confirm it actually happens > on > lubbock. That'll work fine for lubbock, but not the others (we can have several of the others enabled on sa11x0 platforms - eg, badge4 with neponset.) > > (b) there is no clock provided for the sa pcmcia device (aka "1800"). > > This should be the same clock as pxa2xx-pcmcia. > Again right in the spot. > I added temporarily a clock until I have a more complete understanding in > lubbock.c : > + clk_add_alias(NULL, "1800", "SA_CLK", NULL); > > With this, things look way better : > [1.507480] pcmcia_socket pcmcia_socket1: pccard: PCMCIA card inserted > into slot 1 Yay! > I'm still investigating the new message errors: > [0.479157] genirq: Setting trigger mode 3 for irq 387 failed > (sa_type_highirq+0x0/0x6c) > [0.488213] genirq: Setting trigger mode 3 for irq 389 failed > (sa_type_highirq+0x0/0x6c) > [0.507449] genirq: Setting trigger mode 3 for irq 388 failed > (sa_type_highirq+0x0/0x6c) > [0.516492] genirq: Setting trigger mode 3 for irq 390 failed > (sa_type_highirq+0x0/0x6c) Ignore those for now - the old ARM IRQ stuff was silent on that, but genirq is more noisy. I should probably make the sa irqchip handle the both- edge case itself. > Moreover, I have a bit of homework as I also see : > - no SA interrupts at all, especially nothing when I insert/remove my >CompactFlash card >This might be an effect of pxa_cplds_irqs.c I created, I must have a look. Do you get other SA interrupts (eg, the PS/2 interrupts) delivered? > - cat /sys/class/pcmcia_socket/pcmcia_socket1/cis >cat: read error: Input/output error >That will cost me a review of the memory timings registers MCIO1/MECR/xxx, >the power lines, etc ... Hmm, on Neponset with a CF card inserted, I can cat that, and it reports the CIS. Any error messages in the kernel log? > - cat /sys/class/pcmcia_socket/pcmcia_socket1/status > slot : 1 > status : SS_READY SS_DETECT SS_POWERON SS_3VCARD > csc_mask : SS_DETECT > cs_flags : SS_OUTPUT_ENA > Vcc : 33 > Vpp : 33 > IRQ : 0 (386) That looks hopeful, but the IRQ hasn't been properly assigned (probably because no driver has bound to the card.) > >> As your gpios are contiguous (0 .. 31), why an array instead of a simple > >> offset > >> so that your translation is only a linear irq = gpio + offset ? > > > > There isn't a linear translation here: > ...zip... > > MST_PCMCIA_nCD => MAINSTONE_S0_CD_IRQ or MAINSTONE_S1_CD_IRQ > > MST_PCMCIA_nSTSCHG_BVD1 => MAINSTONE_S0_STSCHG_IRQ or > > MAINSTONE_S1_STSCHG_IRQ > > MST_PCMCIA_nIRQ => MAINSTONE_S0_IRQ or MAINSTONE_S1_IRQ > > > > So they aren't linear, and every "gpio" doesn't have a corresponding > > interrupt. > Ah yes, too bad, it would have been so much simpler. Indeed, but a tabular approach isn't that painful, especially if we also insist on knowing an irqdomain as well, which relieves us of having to know absolute interrupt numbers, which may end up being dynamically allocated eventually. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ___ Linux PCMCIA reimplementation list http://lists.infradead.org/mailman/listinfo/linux-pcmcia
Re: [PATCH 05/33] gpio: add generic single-register fixed-direction GPIO driver
On Wed, Aug 31, 2016 at 09:49:38AM +0100, Russell King - ARM Linux wrote: > On Tue, Aug 30, 2016 at 11:32:16PM +0200, Robert Jarzmik wrote: > > Russell King - ARM Linuxwrites: > > > > > If you can wait a day or two, I'll push a branch out for everything in > > > all these multiple series. > > Sure, just ping me when you have something. > > git://git.armlinux.org.uk/~rmk/linux-arm.git sa1100 > > should get you something suitable to test. It's based on 4.7-rc3 plus > my fixes branch. > > It would be great to have this tested on Lubbock, and get the PCMCIA > issues fixed. Maybe we can look at converting mainstone as well? Yes, looking at mainstone's PCMCIA, it uses a MAX1602 device, which is supported by the new max1600.c code. #define MST_PCMCIA_PWR_VPP_00x0/* voltage VPP = 0V */ #define MST_PCMCIA_PWR_VPP_120 0x2/* voltage VPP = 12V*/ #define MST_PCMCIA_PWR_VPP_VCC 0x1/* voltage VPP = VCC */ #define MST_PCMCIA_PWR_VCC_00x0/* voltage VCC = 0V */ #define MST_PCMCIA_PWR_VCC_33 0x8/* voltage VCC = 3.3V */ #define MST_PCMCIA_PWR_VCC_50 0x4/* voltage VCC = 5.0V */ This follows the Cirrus code (also used by Lubbock.) So, if we represent the MST_PCMCIA[01] registers as GPIOs, we can switch pxa2xx_mainstone.c to use the max1600.c code for power control. With the other signals in MST_PCMCIA[01] represented as GPIOs (we'd need to add the VS* to soc_common), we'd then have those read by generic code, which means mst_pcmcia_socket_state() becomes just the hack for STSCHG. With gpio-reg, we can represent these registers easily as GPIOs, eg: gc = gpio_reg_init(NULL, (void __iomem *)_PCMCIA0, -1, 11, "mst-pcmcia0", ~MST_PCMCIA_PWR_MASK, 0, mst_pcmcia_names); There is a slight issue, which is that the interrupts can't be translated to an interrupt by gpio-reg, which will currently cause soc_common problems - but that's an easy fix, though leaves us with more code than I'd desire in pxa2xx_mainstone.c. Maybe a solution there would be to have gpio-reg also take an array of interrupt numbers... not sure yet. For IrDA, it looks like it has the same transceiver as the assabet, so I've (already) patches to split out the gpio-based transceiver control from sa1100_ir - maybe we can re-use that in pxaficp_ir too. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ___ Linux PCMCIA reimplementation list http://lists.infradead.org/mailman/listinfo/linux-pcmcia
Re: [PATCH 05/33] gpio: add generic single-register fixed-direction GPIO driver
On Tue, Aug 30, 2016 at 11:32:16PM +0200, Robert Jarzmik wrote: > Russell King - ARM Linuxwrites: > > > If you can wait a day or two, I'll push a branch out for everything in > > all these multiple series. > Sure, just ping me when you have something. git://git.armlinux.org.uk/~rmk/linux-arm.git sa1100 should get you something suitable to test. It's based on 4.7-rc3 plus my fixes branch. It would be great to have this tested on Lubbock, and get the PCMCIA issues fixed. Maybe we can look at converting mainstone as well? -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ___ Linux PCMCIA reimplementation list http://lists.infradead.org/mailman/listinfo/linux-pcmcia
Re: [PATCH 05/33] gpio: add generic single-register fixed-direction GPIO driver
On Tue, Aug 30, 2016 at 11:42 PM, Russell King - ARM Linuxwrote: > On Tue, Aug 30, 2016 at 11:25:19PM +0200, Linus Walleij wrote: >> > +#define to_gpio_reg(x) container_of(x, struct gpio_reg, gc) >> >> You don't need that trickery anymore, just: >> >> > +static int gpio_reg_get_direction(struct gpio_chip *gc, unsigned offset) >> > +{ >> > + struct gpio_reg *r = to_gpio_reg(gc); >> >> struct gpio_reg *r = gpiochip_get_data(gc); >> >> (applied everywhere) > > I prefer my method by a long shot Sure it's no strong preference. Keep it like this if you like. I'm very happy with the series either way! Yours, Linus Walleij ___ Linux PCMCIA reimplementation list http://lists.infradead.org/mailman/listinfo/linux-pcmcia
Re: [PATCH 05/33] gpio: add generic single-register fixed-direction GPIO driver
On Tue, Aug 30, 2016 at 11:25:19PM +0200, Linus Walleij wrote: > On Mon, Aug 29, 2016 at 12:24 PM, Russell King >wrote: > > > Add a simple, generic, single register fixed-direction GPIO driver. > > This is able to support a single register where a fixed number of > > bits are used for input and a fixed number of bits used for output. > > > > Signed-off-by: Russell King > > Clever, I like it! > > > include/linux/gpio-reg.h | 12 > > Can we put this in include/linux/gpio/gpio-reg.h? > > I try to do some scopeing to there. Sure, I'll just have to hunt through all the patches to find an occurance of the include to fix them all up... > > +#define to_gpio_reg(x) container_of(x, struct gpio_reg, gc) > > You don't need that trickery anymore, just: > > > +static int gpio_reg_get_direction(struct gpio_chip *gc, unsigned offset) > > +{ > > + struct gpio_reg *r = to_gpio_reg(gc); > > struct gpio_reg *r = gpiochip_get_data(gc); > > (applied everywhere) I prefer my method by a long shot - it's always going to work because the gpiochip is embedded within the gpio_reg structure, and the compiler is inteligent enough to keep a single pointer around. With your suggestion, the compiler has no idea that 'r' and 'gc' are actually the same pointer, but a different type, and we end up having to carry around identical pointers in two registers rather than just one. It makes more sense to use gpiochip_get_data() if gpio_chip were a const data structure that was never embedded, but the way *gpiochip_add*() writes to the structure and the presence of members like "base" prevents that. > > > + if (dev) > > + ret = devm_gpiochip_add_data(dev, >gc, r); > > + else > > + ret = gpiochip_add_data(>gc, r); > > Aha both device and device-less, I see. Yes, to avoid problems with the transition to it - the legacy APIs (such as ASSABET_BCR_frob(), etc) can be called really early, so we need the gpiochip available early as well so we can keep the legacy APIs working until they can be killed off. There's some corner cases in the assabet code which make that difficult at the moment. This whole patch set is still very much a work-in-progress - there's more that needs doing, but I wanted to get _this_ out there so that people can reviewing it, and hopefully get it queued for the next merge window. > Apart from that it looks nice, any other questionmarks were > fixed in the other replies. > > Yours, > Linus Walleij -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ___ Linux PCMCIA reimplementation list http://lists.infradead.org/mailman/listinfo/linux-pcmcia
Re: [PATCH 05/33] gpio: add generic single-register fixed-direction GPIO driver
Russell King - ARM Linuxwrites: > If you can wait a day or two, I'll push a branch out for everything in > all these multiple series. Sure, just ping me when you have something. Cheers. -- Robert ___ Linux PCMCIA reimplementation list http://lists.infradead.org/mailman/listinfo/linux-pcmcia
Re: [PATCH 05/33] gpio: add generic single-register fixed-direction GPIO driver
On Mon, Aug 29, 2016 at 12:24 PM, Russell Kingwrote: > Add a simple, generic, single register fixed-direction GPIO driver. > This is able to support a single register where a fixed number of > bits are used for input and a fixed number of bits used for output. > > Signed-off-by: Russell King Clever, I like it! > include/linux/gpio-reg.h | 12 Can we put this in include/linux/gpio/gpio-reg.h? I try to do some scopeing to there. > +#define to_gpio_reg(x) container_of(x, struct gpio_reg, gc) You don't need that trickery anymore, just: > +static int gpio_reg_get_direction(struct gpio_chip *gc, unsigned offset) > +{ > + struct gpio_reg *r = to_gpio_reg(gc); struct gpio_reg *r = gpiochip_get_data(gc); (applied everywhere) > + if (dev) > + ret = devm_gpiochip_add_data(dev, >gc, r); > + else > + ret = gpiochip_add_data(>gc, r); Aha both device and device-less, I see. Apart from that it looks nice, any other questionmarks were fixed in the other replies. Yours, Linus Walleij ___ Linux PCMCIA reimplementation list http://lists.infradead.org/mailman/listinfo/linux-pcmcia
Re: [PATCH 05/33] gpio: add generic single-register fixed-direction GPIO driver
On Tue, Aug 30, 2016 at 06:42:03PM +0200, Robert Jarzmik wrote: > Russell King - ARM Linuxwrites: > > > On Mon, Aug 29, 2016 at 09:39:54PM +0200, Robert Jarzmik wrote: > >> Maybe this one would deserve a doxygen comment ? > > > > Does this solve it? > Oh yes, that's very nice, especially the "1 = in" for which I have always a > doubt. > > Reviewed-by: Robert Jarzmik > > I hope to have an "attempt" to test all your series on the lubbock board in > the > next days. The "attempt" part is because last time I checked pcmcia support > was > broken on lubbock (ie. CONFIG_PCMCIA_SA activated implies an Oops), and I > don't remember having repaired it (and it's disabled on my test farm which is > a > sign I was not very confident about this part). > > If you happen to have somewhere a branch I can pull, with all your series (at > least "[RFC PATCH 00/33] SA11x0/PXA GPIO rework (Core + PCMCIA only)" and > "[PATCH 0/8] SA11x0/PXA remainder & cleanups"), that would spare me the > multiple > git-am to make the test(s). If you can wait a day or two, I'll push a branch out for everything in all these multiple series. Thanks. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ___ Linux PCMCIA reimplementation list http://lists.infradead.org/mailman/listinfo/linux-pcmcia
Re: [PATCH 05/33] gpio: add generic single-register fixed-direction GPIO driver
Russell King - ARM Linuxwrites: > On Mon, Aug 29, 2016 at 09:39:54PM +0200, Robert Jarzmik wrote: >> Maybe this one would deserve a doxygen comment ? > > Does this solve it? Oh yes, that's very nice, especially the "1 = in" for which I have always a doubt. Reviewed-by: Robert Jarzmik I hope to have an "attempt" to test all your series on the lubbock board in the next days. The "attempt" part is because last time I checked pcmcia support was broken on lubbock (ie. CONFIG_PCMCIA_SA activated implies an Oops), and I don't remember having repaired it (and it's disabled on my test farm which is a sign I was not very confident about this part). If you happen to have somewhere a branch I can pull, with all your series (at least "[RFC PATCH 00/33] SA11x0/PXA GPIO rework (Core + PCMCIA only)" and "[PATCH 0/8] SA11x0/PXA remainder & cleanups"), that would spare me the multiple git-am to make the test(s). Cheers. -- Robert ___ Linux PCMCIA reimplementation list http://lists.infradead.org/mailman/listinfo/linux-pcmcia
Re: [PATCH 05/33] gpio: add generic single-register fixed-direction GPIO driver
On Tue, Aug 30, 2016 at 09:08:03AM +0300, Alexander Shiyan wrote: > >Вторник, 30 августа 2016, 2:12 +03:00 от Russell King - ARM Linux > >: > > > >On Mon, Aug 29, 2016 at 09:39:54PM +0200, Robert Jarzmik wrote: > >> Hi Russell, > >> > >> Russell King < rmk+ker...@armlinux.org.uk > writes: > >> > >> > Add a simple, generic, single register fixed-direction GPIO driver. > >> > This is able to support a single register where a fixed number of > >> > bits are used for input and a fixed number of bits used for output. > >> > > >> > Signed-off-by: Russell King < rmk+ker...@armlinux.org.uk > > > There is a GPIO driver which already performs these tasks. > Plaease take a look on the gpio-74xx-mmio driver. I did, and no it doesn't, because: 1. It is either all-in or all-out, it doesn't support a mixture of fixed-directions for a single register. 2. It is DT-only, I need it for legacy platforms. 3. It uses the bgpio stuff, which is unsuitable for reasons already covered in a previous reply. So, gpio-74xx-mmio is unsuitable. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ___ Linux PCMCIA reimplementation list http://lists.infradead.org/mailman/listinfo/linux-pcmcia
Re: [PATCH 05/33] gpio: add generic single-register fixed-direction GPIO driver
>Вторник, 30 августа 2016, 2:12 +03:00 от Russell King - ARM Linux >: > >On Mon, Aug 29, 2016 at 09:39:54PM +0200, Robert Jarzmik wrote: >> Hi Russell, >> >> Russell King < rmk+ker...@armlinux.org.uk > writes: >> >> > Add a simple, generic, single register fixed-direction GPIO driver. >> > This is able to support a single register where a fixed number of >> > bits are used for input and a fixed number of bits used for output. >> > >> > Signed-off-by: Russell King < rmk+ker...@armlinux.org.uk > There is a GPIO driver which already performs these tasks. Plaease take a look on the gpio-74xx-mmio driver. --- ___ Linux PCMCIA reimplementation list http://lists.infradead.org/mailman/listinfo/linux-pcmcia
Re: [PATCH 05/33] gpio: add generic single-register fixed-direction GPIO driver
On Mon, Aug 29, 2016 at 09:39:54PM +0200, Robert Jarzmik wrote: > Hi Russell, > > Russell Kingwrites: > > > Add a simple, generic, single register fixed-direction GPIO driver. > > This is able to support a single register where a fixed number of > > bits are used for input and a fixed number of bits used for output. > > > > Signed-off-by: Russell King > > --- > > drivers/gpio/Kconfig | 6 ++ > > drivers/gpio/Makefile| 1 + > > drivers/gpio/gpio-reg.c | 139 > > +++ > > include/linux/gpio-reg.h | 12 > > 4 files changed, 158 insertions(+) > > create mode 100644 drivers/gpio/gpio-reg.c > > create mode 100644 include/linux/gpio-reg.h > > > > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig > > index 98dd47a30fc7..49bd8b89712e 100644 > > --- a/drivers/gpio/Kconfig > > +++ b/drivers/gpio/Kconfig > > @@ -365,6 +365,12 @@ config GPIO_RCAR > > help > > Say yes here to support GPIO on Renesas R-Car SoCs. > > > > +config GPIO_REG > > + bool > So I suppose it is on purpose you forbid it to be a module. Is there a way to > write either in the commit message or in the Kconfig this purpose, so that > nobody on "purpose" changes this bool to tristate ? The only forbidding is that it's used from code early at boot, so it has to be available to the kernel for some of these platforms to function. Even if it's changed to tristate, the select from these platforms would force it to be built-in. > > + help > > + A 32-bit single register GPIO fixed in/out implementation. This > > + can be used to represent any register as a set of GPIO signals. > Another question I was asking myself was how it differenciated itself from > gpio-mmio, ie. what brought the need for this driver that isn't available with > gpio-mmio ? gpio-mmio doesn't allow the fixed direction - it assumes that you always have some form of direction register. It also doesn't do the double-read that's necessary for some platforms to get the "current" state of inputs. It also doesn't do the "use 32-bit accessors even for smaller numbers of GPIOs". Lastly, it assumes that the current output state can be read from the registers - this is not always the case (and is not the case for Assabet.) > I seem to understand that this is mainly for platform code, hence the > "builtin only" necessity, and if I'm right a part of your cover letter > could very well fit within this patch's commit message. Apart from the missing MODULE_* tags and symbol exports, there's nothing whic prohibits it becoming a module. > > +static void gpio_reg_set_multiple(struct gpio_chip *gc, unsigned long > > *mask, > > + unsigned long *bits) > > +{ > > + struct gpio_reg *r = to_gpio_reg(gc); > > + unsigned long flags; > > + > > + spin_lock_irqsave(>lock, flags); > > + r->out = (r->out & ~*mask) | *bits; > Shouldn't this be : > r->out = (r->out & ~*mask) | (*bits & *mask); Possibly, but the latter is redundant when used through gpiolib. > > diff --git a/include/linux/gpio-reg.h b/include/linux/gpio-reg.h > > new file mode 100644 > > index ..0352bec7319a > > --- /dev/null > > +++ b/include/linux/gpio-reg.h > > @@ -0,0 +1,12 @@ > > +#ifndef GPIO_REG_H > > +#define GPIO_REG_H > > + > > +struct device; > > + > > +struct gpio_chip *gpio_reg_init(struct device *dev, void __iomem *reg, > > + int base, int num, const char *label, u32 direction, u32 def_out, > > + const char *const *names); > Maybe this one would deserve a doxygen comment ? Maybe, but I've been told not to put such comments in header files. I've already spent the last two weeks on this stuff (at the expense of reading mail), I've not got around to thinking about any kind of documentation. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ___ Linux PCMCIA reimplementation list http://lists.infradead.org/mailman/listinfo/linux-pcmcia
Re: [PATCH 05/33] gpio: add generic single-register fixed-direction GPIO driver
Hi Russell, Russell Kingwrites: > Add a simple, generic, single register fixed-direction GPIO driver. > This is able to support a single register where a fixed number of > bits are used for input and a fixed number of bits used for output. > > Signed-off-by: Russell King > --- > drivers/gpio/Kconfig | 6 ++ > drivers/gpio/Makefile| 1 + > drivers/gpio/gpio-reg.c | 139 > +++ > include/linux/gpio-reg.h | 12 > 4 files changed, 158 insertions(+) > create mode 100644 drivers/gpio/gpio-reg.c > create mode 100644 include/linux/gpio-reg.h > > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig > index 98dd47a30fc7..49bd8b89712e 100644 > --- a/drivers/gpio/Kconfig > +++ b/drivers/gpio/Kconfig > @@ -365,6 +365,12 @@ config GPIO_RCAR > help > Say yes here to support GPIO on Renesas R-Car SoCs. > > +config GPIO_REG > + bool So I suppose it is on purpose you forbid it to be a module. Is there a way to write either in the commit message or in the Kconfig this purpose, so that nobody on "purpose" changes this bool to tristate ? > + help > + A 32-bit single register GPIO fixed in/out implementation. This > + can be used to represent any register as a set of GPIO signals. Another question I was asking myself was how it differenciated itself from gpio-mmio, ie. what brought the need for this driver that isn't available with gpio-mmio ? I seem to understand that this is mainly for platform code, hence the "builtin only" necessity, and if I'm right a part of your cover letter could very well fit within this patch's commit message. > diff --git a/drivers/gpio/gpio-reg.c b/drivers/gpio/gpio-reg.c > new file mode 100644 > index ..fc7e0a395f9f > --- /dev/null > +++ b/drivers/gpio/gpio-reg.c ...zip... > +static void gpio_reg_set_multiple(struct gpio_chip *gc, unsigned long *mask, > + unsigned long *bits) > +{ > + struct gpio_reg *r = to_gpio_reg(gc); > + unsigned long flags; > + > + spin_lock_irqsave(>lock, flags); > + r->out = (r->out & ~*mask) | *bits; Shouldn't this be : r->out = (r->out & ~*mask) | (*bits & *mask); > diff --git a/include/linux/gpio-reg.h b/include/linux/gpio-reg.h > new file mode 100644 > index ..0352bec7319a > --- /dev/null > +++ b/include/linux/gpio-reg.h > @@ -0,0 +1,12 @@ > +#ifndef GPIO_REG_H > +#define GPIO_REG_H > + > +struct device; > + > +struct gpio_chip *gpio_reg_init(struct device *dev, void __iomem *reg, > + int base, int num, const char *label, u32 direction, u32 def_out, > + const char *const *names); Maybe this one would deserve a doxygen comment ? Cheers. -- Robert ___ Linux PCMCIA reimplementation list http://lists.infradead.org/mailman/listinfo/linux-pcmcia