* [RFC] gpio/stmpe: add bitmask to block requests to unavailable GPIOs @ 2010-08-16 15:14 Wolfram Sang 2010-08-18 10:09 ` Rabin VINCENT 0 siblings, 1 reply; 6+ messages in thread From: Wolfram Sang @ 2010-08-16 15:14 UTC (permalink / raw) To: linux-kernel; +Cc: Wolfram Sang, Rabin Vincent, Linus Walleij, Samuel Ortiz GPIOs on these controller are multi-functional. If you decided to use some of them e.g. as input channels for the ADC, you surely don't want those pins to be reassigned as simple GPIOs (which may be triggered even from userspace via 'export'). Same for the touchscreen controller pins. Since knowledge about the hardware is needed to decide which GPIOs to reserve, let this bitmask be inside platform_data and provide some defines to assist potential users. Signed-off-by: Wolfram Sang <w.sang@pengutronix.de> Cc: Rabin Vincent <rabin.vincent@stericsson.com> Cc: Linus Walleij <linus.walleij@stericsson.com> Cc: Samuel Ortiz <sameo@linux.intel.com> --- Tested on a custom board. I'm not sure if this is the best solution. Now, using 'export' might fail without the GPIO in question appearing in debugfs. That might be confusing. Still, it should be a lot safer than being able to change the pins via userspace. drivers/gpio/stmpe-gpio.c | 5 +++++ include/linux/mfd/stmpe.h | 6 ++++++ 2 files changed, 11 insertions(+), 0 deletions(-) diff --git a/drivers/gpio/stmpe-gpio.c b/drivers/gpio/stmpe-gpio.c index 4e1f1b9..65b9960 100644 --- a/drivers/gpio/stmpe-gpio.c +++ b/drivers/gpio/stmpe-gpio.c @@ -30,6 +30,7 @@ struct stmpe_gpio { struct mutex irq_lock; int irq_base; + unsigned norequest_mask; /* Caches of interrupt control registers for bus_lock */ u8 regs[CACHE_NR_REGS][CACHE_NR_BANKS]; @@ -103,6 +104,9 @@ static int stmpe_gpio_request(struct gpio_chip *chip, unsigned offset) struct stmpe_gpio *stmpe_gpio = to_stmpe_gpio(chip); struct stmpe *stmpe = stmpe_gpio->stmpe; + if (stmpe_gpio->norequest_mask & (1 << offset)) + return -EINVAL; + return stmpe_set_altfunc(stmpe, 1 << offset, STMPE_BLOCK_GPIO); } @@ -302,6 +306,7 @@ static int __devinit stmpe_gpio_probe(struct platform_device *pdev) stmpe_gpio->dev = &pdev->dev; stmpe_gpio->stmpe = stmpe; + stmpe_gpio->norequest_mask = pdata ? pdata->norequest_mask : 0; stmpe_gpio->chip = template_chip; stmpe_gpio->chip.ngpio = stmpe->num_gpios; diff --git a/include/linux/mfd/stmpe.h b/include/linux/mfd/stmpe.h index 90faa98..845abaa 100644 --- a/include/linux/mfd/stmpe.h +++ b/include/linux/mfd/stmpe.h @@ -112,13 +112,19 @@ struct stmpe_keypad_platform_data { bool no_autorepeat; }; +#define STMPE_GPIO_NOREQ_811_TOUCH (0xf0) + /** * struct stmpe_gpio_platform_data - STMPE GPIO platform data * @gpio_base: first gpio number assigned. A maximum of * %STMPE_NR_GPIOS GPIOs will be allocated. + * @norequest_mask: bitmask specifying which GPIOs should _not_ be + * requestable due to different usage (e.g. touch, keypad) + * STMPE_GPIO_NOREQ_* macros can be used here. */ struct stmpe_gpio_platform_data { int gpio_base; + unsigned norequest_mask; void (*setup)(struct stmpe *stmpe, unsigned gpio_base); void (*remove)(struct stmpe *stmpe, unsigned gpio_base); }; -- 1.7.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RFC] gpio/stmpe: add bitmask to block requests to unavailable GPIOs 2010-08-16 15:14 [RFC] gpio/stmpe: add bitmask to block requests to unavailable GPIOs Wolfram Sang @ 2010-08-18 10:09 ` Rabin VINCENT 2010-08-18 10:31 ` Wolfram Sang 0 siblings, 1 reply; 6+ messages in thread From: Rabin VINCENT @ 2010-08-18 10:09 UTC (permalink / raw) To: Wolfram Sang; +Cc: linux-kernel@vger.kernel.org, Linus WALLEIJ, Samuel Ortiz On Mon, Aug 16, 2010 at 17:14:44 +0200, Wolfram Sang wrote: > GPIOs on these controller are multi-functional. If you decided to use > some of them e.g. as input channels for the ADC, you surely don't want > those pins to be reassigned as simple GPIOs (which may be triggered even > from userspace via 'export'). Same for the touchscreen controller pins. > Since knowledge about the hardware is needed to decide which GPIOs to > reserve, let this bitmask be inside platform_data and provide some > defines to assist potential users. Could this be done without the platform data, say something like the below? (Though this does assume that nobody tries to request GPIOs before the STMPE subdrivers reserve their pins.) diff --git a/drivers/input/touchscreen/stmpe-ts.c b/drivers/input/touchscreen/stmpe-ts.c index 656148e..d1595f9 100644 --- a/drivers/input/touchscreen/stmpe-ts.c +++ b/drivers/input/touchscreen/stmpe-ts.c @@ -179,6 +179,12 @@ static int __devinit stmpe_init_hw(struct stmpe_touch *ts) return ret; } + ret = stmpe_set_altfunc(stmpe, 0xf0, STMPE_BLOCK_TOUCHSCREEN); + if (ret) { + dev_err(dev, "Could not enable alternate function for pins\n"); + return ret; + } + adc_ctrl1 = SAMPLE_TIME(ts->sample_time) | MOD_12B(ts->mod_12b) | REF_SEL(ts->ref_sel); adc_ctrl1_mask = SAMPLE_TIME(0xff) | MOD_12B(0xff) | REF_SEL(0xff); diff --git a/drivers/mfd/stmpe.c b/drivers/mfd/stmpe.c index 0754c5e9..4f748fa 100644 --- a/drivers/mfd/stmpe.c +++ b/drivers/mfd/stmpe.c @@ -253,6 +253,12 @@ int stmpe_set_altfunc(struct stmpe *stmpe, u32 pins, enum stmpe_block block) mutex_lock(&stmpe->lock); + if (block == STMPE_BLOCK_GPIO && (pins & stmpe->af_pins)) { + dev_dbg(stmpe->dev, "not allowing change of AF pins to GPIO\n"); + ret = -EBUSY; + goto out; + } + ret = __stmpe_enable(stmpe, STMPE_BLOCK_GPIO); if (ret < 0) goto out; @@ -274,6 +280,11 @@ int stmpe_set_altfunc(struct stmpe *stmpe, u32 pins, enum stmpe_block block) pins &= ~(1 << pin); } + if (block == STMPE_BLOCK_GPIO) + stmpe->af_pins &= ~pins; + else + stmpe->af_pins |= pins; + ret = __stmpe_block_write(stmpe, regaddr, numregs, regs); out: diff --git a/include/linux/mfd/stmpe.h b/include/linux/mfd/stmpe.h index 39ca758..ba5a847 100644 --- a/include/linux/mfd/stmpe.h +++ b/include/linux/mfd/stmpe.h @@ -64,6 +64,7 @@ struct stmpe_variant_info; * @num_gpios: number of gpios, differs for variants * @ier: cache of IER registers for bus_lock * @oldier: cache of IER registers for bus_lock + * @af_pins: mask of pins set in alternate function mode * @pdata: platform data */ struct stmpe { @@ -79,6 +80,7 @@ struct stmpe { int num_gpios; u8 ier[2]; u8 oldier[2]; + u32 af_pins; struct stmpe_platform_data *pdata; }; ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RFC] gpio/stmpe: add bitmask to block requests to unavailable GPIOs 2010-08-18 10:09 ` Rabin VINCENT @ 2010-08-18 10:31 ` Wolfram Sang 2010-08-19 13:06 ` Rabin Vincent 0 siblings, 1 reply; 6+ messages in thread From: Wolfram Sang @ 2010-08-18 10:31 UTC (permalink / raw) To: Rabin VINCENT; +Cc: linux-kernel@vger.kernel.org, Linus WALLEIJ, Samuel Ortiz [-- Attachment #1: Type: text/plain, Size: 3605 bytes --] On Wed, Aug 18, 2010 at 03:39:50PM +0530, Rabin VINCENT wrote: > On Mon, Aug 16, 2010 at 17:14:44 +0200, Wolfram Sang wrote: > > GPIOs on these controller are multi-functional. If you decided to use > > some of them e.g. as input channels for the ADC, you surely don't want > > those pins to be reassigned as simple GPIOs (which may be triggered even > > from userspace via 'export'). Same for the touchscreen controller pins. > > Since knowledge about the hardware is needed to decide which GPIOs to > > reserve, let this bitmask be inside platform_data and provide some > > defines to assist potential users. > > Could this be done without the platform data, say something like the > below? (Though this does assume that nobody tries to request GPIOs > before the STMPE subdrivers reserve their pins.) > While I'd also like to skip the additional platform_data entry, your last comment is in deed a drawback. Also, when thinking of a generic solution (ADC comes to my mind), you can't make an assumption which pins will have the alternate function as you can do with the touchscreen driver. If a pin is an ADC-input channel or a GPIO, that is board design. Regards, Wolfram > diff --git a/drivers/input/touchscreen/stmpe-ts.c b/drivers/input/touchscreen/stmpe-ts.c > index 656148e..d1595f9 100644 > --- a/drivers/input/touchscreen/stmpe-ts.c > +++ b/drivers/input/touchscreen/stmpe-ts.c > @@ -179,6 +179,12 @@ static int __devinit stmpe_init_hw(struct stmpe_touch *ts) > return ret; > } > > + ret = stmpe_set_altfunc(stmpe, 0xf0, STMPE_BLOCK_TOUCHSCREEN); > + if (ret) { > + dev_err(dev, "Could not enable alternate function for pins\n"); > + return ret; > + } > + > adc_ctrl1 = SAMPLE_TIME(ts->sample_time) | MOD_12B(ts->mod_12b) | > REF_SEL(ts->ref_sel); > adc_ctrl1_mask = SAMPLE_TIME(0xff) | MOD_12B(0xff) | REF_SEL(0xff); > diff --git a/drivers/mfd/stmpe.c b/drivers/mfd/stmpe.c > index 0754c5e9..4f748fa 100644 > --- a/drivers/mfd/stmpe.c > +++ b/drivers/mfd/stmpe.c > @@ -253,6 +253,12 @@ int stmpe_set_altfunc(struct stmpe *stmpe, u32 pins, enum stmpe_block block) > > mutex_lock(&stmpe->lock); > > + if (block == STMPE_BLOCK_GPIO && (pins & stmpe->af_pins)) { > + dev_dbg(stmpe->dev, "not allowing change of AF pins to GPIO\n"); > + ret = -EBUSY; > + goto out; > + } > + > ret = __stmpe_enable(stmpe, STMPE_BLOCK_GPIO); > if (ret < 0) > goto out; > @@ -274,6 +280,11 @@ int stmpe_set_altfunc(struct stmpe *stmpe, u32 pins, enum stmpe_block block) > pins &= ~(1 << pin); > } > > + if (block == STMPE_BLOCK_GPIO) > + stmpe->af_pins &= ~pins; > + else > + stmpe->af_pins |= pins; > + > ret = __stmpe_block_write(stmpe, regaddr, numregs, regs); > > out: > diff --git a/include/linux/mfd/stmpe.h b/include/linux/mfd/stmpe.h > index 39ca758..ba5a847 100644 > --- a/include/linux/mfd/stmpe.h > +++ b/include/linux/mfd/stmpe.h > @@ -64,6 +64,7 @@ struct stmpe_variant_info; > * @num_gpios: number of gpios, differs for variants > * @ier: cache of IER registers for bus_lock > * @oldier: cache of IER registers for bus_lock > + * @af_pins: mask of pins set in alternate function mode > * @pdata: platform data > */ > struct stmpe { > @@ -79,6 +80,7 @@ struct stmpe { > int num_gpios; > u8 ier[2]; > u8 oldier[2]; > + u32 af_pins; > struct stmpe_platform_data *pdata; > }; > -- Pengutronix e.K. | Wolfram Sang | Industrial Linux Solutions | http://www.pengutronix.de/ | [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 198 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC] gpio/stmpe: add bitmask to block requests to unavailable GPIOs 2010-08-18 10:31 ` Wolfram Sang @ 2010-08-19 13:06 ` Rabin Vincent 2010-08-19 13:26 ` Wolfram Sang 2010-08-20 22:25 ` Samuel Ortiz 0 siblings, 2 replies; 6+ messages in thread From: Rabin Vincent @ 2010-08-19 13:06 UTC (permalink / raw) To: Wolfram Sang; +Cc: linux-kernel@vger.kernel.org, Linus WALLEIJ, Samuel Ortiz On Wed, Aug 18, 2010 at 12:31:37 +0200, Wolfram Sang wrote: > On Wed, Aug 18, 2010 at 03:39:50PM +0530, Rabin VINCENT wrote: > > On Mon, Aug 16, 2010 at 17:14:44 +0200, Wolfram Sang wrote: > > > GPIOs on these controller are multi-functional. If you decided to use > > > some of them e.g. as input channels for the ADC, you surely don't want > > > those pins to be reassigned as simple GPIOs (which may be triggered even > > > from userspace via 'export'). Same for the touchscreen controller pins. > > > Since knowledge about the hardware is needed to decide which GPIOs to > > > reserve, let this bitmask be inside platform_data and provide some > > > defines to assist potential users. > > > > Could this be done without the platform data, say something like the > > below? (Though this does assume that nobody tries to request GPIOs > > before the STMPE subdrivers reserve their pins.) > > > > While I'd also like to skip the additional platform_data entry, your > last comment is in deed a drawback. Yes, especially if the subdrivers are dynamically loaded. Your patch is fine with me: Acked-by: Rabin Vincent <rabin.vincent@stericsson.com> The only minor comment is that the STMPE_GPIO_NOREQ_811_TOUCH name could perhaps be clearer. STMPE811_GPIO_NOREQ_TOUCHSCREEN, maybe? Rabin ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC] gpio/stmpe: add bitmask to block requests to unavailable GPIOs 2010-08-19 13:06 ` Rabin Vincent @ 2010-08-19 13:26 ` Wolfram Sang 2010-08-20 22:25 ` Samuel Ortiz 1 sibling, 0 replies; 6+ messages in thread From: Wolfram Sang @ 2010-08-19 13:26 UTC (permalink / raw) To: Rabin Vincent; +Cc: linux-kernel@vger.kernel.org, Linus WALLEIJ, Samuel Ortiz [-- Attachment #1: Type: text/plain, Size: 1766 bytes --] On Thu, Aug 19, 2010 at 06:36:05PM +0530, Rabin Vincent wrote: > On Wed, Aug 18, 2010 at 12:31:37 +0200, Wolfram Sang wrote: > > On Wed, Aug 18, 2010 at 03:39:50PM +0530, Rabin VINCENT wrote: > > > On Mon, Aug 16, 2010 at 17:14:44 +0200, Wolfram Sang wrote: > > > > GPIOs on these controller are multi-functional. If you decided to use > > > > some of them e.g. as input channels for the ADC, you surely don't want > > > > those pins to be reassigned as simple GPIOs (which may be triggered even > > > > from userspace via 'export'). Same for the touchscreen controller pins. > > > > Since knowledge about the hardware is needed to decide which GPIOs to > > > > reserve, let this bitmask be inside platform_data and provide some > > > > defines to assist potential users. > > > > > > Could this be done without the platform data, say something like the > > > below? (Though this does assume that nobody tries to request GPIOs > > > before the STMPE subdrivers reserve their pins.) > > > > > > > While I'd also like to skip the additional platform_data entry, your > > last comment is in deed a drawback. > > Yes, especially if the subdrivers are dynamically loaded. Your patch is > fine with me: > > Acked-by: Rabin Vincent <rabin.vincent@stericsson.com> Thanks. > The only minor comment is that the STMPE_GPIO_NOREQ_811_TOUCH name could > perhaps be clearer. STMPE811_GPIO_NOREQ_TOUCHSCREEN, maybe? I thought of STMPE_GPIO_NOREQ as common prefix, 811 as type, and TOUCH as function. But I don't insist on it; will do what it takes to get this patch upstream ;) -- Pengutronix e.K. | Wolfram Sang | Industrial Linux Solutions | http://www.pengutronix.de/ | [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 198 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC] gpio/stmpe: add bitmask to block requests to unavailable GPIOs 2010-08-19 13:06 ` Rabin Vincent 2010-08-19 13:26 ` Wolfram Sang @ 2010-08-20 22:25 ` Samuel Ortiz 1 sibling, 0 replies; 6+ messages in thread From: Samuel Ortiz @ 2010-08-20 22:25 UTC (permalink / raw) To: Rabin Vincent; +Cc: Wolfram Sang, linux-kernel@vger.kernel.org, Linus WALLEIJ Hi Rabin, On Thu, Aug 19, 2010 at 06:36:05PM +0530, Rabin Vincent wrote: > On Wed, Aug 18, 2010 at 12:31:37 +0200, Wolfram Sang wrote: > > On Wed, Aug 18, 2010 at 03:39:50PM +0530, Rabin VINCENT wrote: > > > On Mon, Aug 16, 2010 at 17:14:44 +0200, Wolfram Sang wrote: > > > > GPIOs on these controller are multi-functional. If you decided to use > > > > some of them e.g. as input channels for the ADC, you surely don't want > > > > those pins to be reassigned as simple GPIOs (which may be triggered even > > > > from userspace via 'export'). Same for the touchscreen controller pins. > > > > Since knowledge about the hardware is needed to decide which GPIOs to > > > > reserve, let this bitmask be inside platform_data and provide some > > > > defines to assist potential users. > > > > > > Could this be done without the platform data, say something like the > > > below? (Though this does assume that nobody tries to request GPIOs > > > before the STMPE subdrivers reserve their pins.) > > > > > > > While I'd also like to skip the additional platform_data entry, your > > last comment is in deed a drawback. > > Yes, especially if the subdrivers are dynamically loaded. Your patch is > fine with me: > > Acked-by: Rabin Vincent <rabin.vincent@stericsson.com> > > The only minor comment is that the STMPE_GPIO_NOREQ_811_TOUCH name could > perhaps be clearer. STMPE811_GPIO_NOREQ_TOUCHSCREEN, maybe? I applied Wolfram's patch. If he wants to change this name, he can send an additional patch later on. Thanks for the patch and the review. Cheers, Samuel. > Rabin -- Intel Open Source Technology Centre http://oss.intel.com/ ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-08-20 22:25 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-08-16 15:14 [RFC] gpio/stmpe: add bitmask to block requests to unavailable GPIOs Wolfram Sang 2010-08-18 10:09 ` Rabin VINCENT 2010-08-18 10:31 ` Wolfram Sang 2010-08-19 13:06 ` Rabin Vincent 2010-08-19 13:26 ` Wolfram Sang 2010-08-20 22:25 ` Samuel Ortiz
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).