Re: [PATCH 06/33] ARM: pxa/lubbock: add GPIO driver for LUB_MISC_WR register

2 messages, 2 authors, 2016-08-29 · open the first message on its own page

Re: [PATCH 06/33] ARM: pxa/lubbock: add GPIO driver for LUB_MISC_WR register

From: Robert Jarzmik <robert.jarzmik@free.fr>
Date: 2016-08-29 19:57:20

Russell King [off-list ref] writes:

..zip...

quoted hunk

diff --git a/arch/arm/mach-pxa/lubbock.c b/arch/arm/mach-pxa/lubbock.c
index 7245f3359564..e974d1eb0f88 100644
--- a/arch/arm/mach-pxa/lubbock.c
+++ b/arch/arm/mach-pxa/lubbock.c

...zip...

quoted hunk

@@ -110,20 +111,18 @@ static unsigned long lubbock_pin_config[] __initdata = {
 };
 
 #define LUB_HEXLED		__LUB_REG(LUBBOCK_FPGA_PHYS + 0x010)
-#define LUB_MISC_WR		__LUB_REG(LUBBOCK_FPGA_PHYS + 0x080)
 
 void lubbock_set_hexled(uint32_t value)
 {
 	LUB_HEXLED = value;
 }
 
+static struct gpio_chip *lubbock_misc_wr_gc;
+
 void lubbock_set_misc_wr(unsigned int mask, unsigned int set)
 {
-	unsigned long flags;
-
-	local_irq_save(flags);
-	LUB_MISC_WR = (LUB_MISC_WR & ~mask) | (set & mask);
-	local_irq_restore(flags);
+	unsigned long m = mask, v = set;
+	lubbock_misc_wr_gc->set_multiple(lubbock_misc_wr_gc, &m, &v);

If gpio_reg_init() failed (and I know, the probability of a lack of memory at
that stage of the kernel boot is ridiculous), this will end up as an NULL
pointer dereference if either IRDA or PCMCIA is used.

If it's expected, then the the pr_err() below would deserve a pr_crit(). I would
as well take an option on a "panic()" if lubbock_misc_wr_gc allocation fails. If
not, maybe a not-NULL test on lubbock_misc_wr_gc is in order, even if that would
be "hidding under the carpet" and rather difficult to debug later.

Apart this detail point, it's good for me.

Cheers.

-- 
Robert

Re: [PATCH 06/33] ARM: pxa/lubbock: add GPIO driver for LUB_MISC_WR register

From: Russell King - ARM Linux <linux@armlinux.org.uk>
Date: 2016-08-29 22:58:48

On Mon, Aug 29, 2016 at 09:57:20PM +0200, Robert Jarzmik wrote:

Russell King [off-list ref] writes:
If gpio_reg_init() failed (and I know, the probability of a lack of memory at
that stage of the kernel boot is ridiculous), this will end up as an NULL
pointer dereference if either IRDA or PCMCIA is used.

If it's expected, then the the pr_err() below would deserve a pr_crit(). I would
as well take an option on a "panic()" if lubbock_misc_wr_gc allocation fails. If
not, maybe a not-NULL test on lubbock_misc_wr_gc is in order, even if that would
be "hidding under the carpet" and rather difficult to debug later.

Apart this detail point, it's good for me.

I'd prefer to kill the function entirely.  The PCMCIA user of it goes
later in the series, and all that's left are the three users in lubbock.c,
those being IrDA and ads7846.  I'd like to see pxaficp go the same way
as sa1100_ir - converting to using gpiolib gpiod* APIs to manage the
FIR mode select GPIO itself internally.

That leaves ads7846_cs(), and I'd be surprised if SPI didn't already of
wiring GPIOs up as chip selects.

-- 
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.

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help