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 <[email protected]>
Cc: Rabin Vincent <[email protected]>
Cc: Linus Walleij <[email protected]>
Cc: Samuel Ortiz <[email protected]>
---
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
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;
};
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/ |
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 <[email protected]>
The only minor comment is that the STMPE_GPIO_NOREQ_811_TOUCH name could
perhaps be clearer. STMPE811_GPIO_NOREQ_TOUCHSCREEN, maybe?
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 <[email protected]>
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/ |
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 <[email protected]>
>
> 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/