RE: [PATCH v2 4/4] da850-evm: add baseboard UI expander buttons, switches and LEDs
Hi Ben, On Mon, Nov 22, 2010 at 19:45:46, Ben Gardiner wrote: Hi Sekhar, On Mon, Nov 22, 2010 at 7:00 AM, Nori, Sekhar nsek...@ti.com wrote: Thanks for the explanation. I should have probably asked earlier, why do we need to prevent sysfs access of deep sleep enable and sw reset pins? We don't seem to be using them in the kernel either. You're welcome. I was assuming that those pins were not exported as gpio pins on purpose; I was taking the prudent approach to prevent haphazard toggling of sw_rst and deep_sleep_en from userspace. sw_rst because it could initiate a reset of the cpu when toggled and deep_sleep_en because it can override the behaviour of davinci_pm_enter(). I'm not sure how they would be used by existing kernel classes either. The sw_rst pin could be used for reset but since it is on the other end of an i2c bus and there is an existing implementation of reset using the on chip watchdog I don't think it would be benficial to switch. Deep_sleep_en could override the behaviour in davinci_pm_enter() -- _maybe_ (I don't really know) it could be used as a hardware-assisted suspend-blocker? But I totally guessing here. My preference would be to leave these pins as is (don't call a gpio_request() on them) till someone comes up with a use case for them. From what you described, sysfs access cannot happen accidently so someone accessing these pins from sysfs surely knows what he is doing. Thanks, Sekhar ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH v2 4/4] da850-evm: add baseboard UI expander buttons, switches and LEDs
Hi Sekhar, On Tue, Nov 23, 2010 at 7:42 AM, Nori, Sekhar nsek...@ti.com wrote: On Mon, Nov 22, 2010 at 19:45:46, Ben Gardiner wrote: [...] I was assuming that those pins were not exported as gpio pins on purpose; I was taking the prudent approach to prevent haphazard toggling of sw_rst and deep_sleep_en from userspace. sw_rst because it could initiate a reset of the cpu when toggled and deep_sleep_en because it can override the behaviour of davinci_pm_enter(). I'm not sure how they would be used by existing kernel classes either. The sw_rst pin could be used for reset but since it is on the other end of an i2c bus and there is an existing implementation of reset using the on chip watchdog I don't think it would be benficial to switch. Deep_sleep_en could override the behaviour in davinci_pm_enter() -- _maybe_ (I don't really know) it could be used as a hardware-assisted suspend-blocker? But I totally guessing here. My preference would be to leave these pins as is (don't call a gpio_request() on them) till someone comes up with a use case for them. From what you described, sysfs access cannot happen accidently so someone accessing these pins from sysfs surely knows what he is doing. No problem. I will re-spin the patch shortly with the deep_sleep_en and sw_rst pins free for use by client code. Best Regards, Ben Gardiner --- Nanometrics Inc. http://www.nanometrics.ca ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
RE: [PATCH v2 4/4] da850-evm: add baseboard UI expander buttons, switches and LEDs
Hi Ben, On Fri, Nov 19, 2010 at 21:10:50, Ben Gardiner wrote: [...] How does gpio_request prevent sysfs control? To obtain access to a gpio through the sysfs interface the user must first write the gpio number to 'export'. When the gpio has been gpio_request()'d the result of writing to 'export' is nothing; whereas writing to export would normally result in the appearance of a named gpio line alongside 'export'. I hope the following commands and their output illustrate my point: $ cd /sys/class/gpio/ $ ls export gpiochip128 gpiochip160 gpiochip64 unexport gpiochip0gpiochip144 gpiochip32 gpiochip96 $ echo 160 export $ ls export gpiochip128 gpiochip160 gpiochip64 unexport gpiochip0gpiochip144 gpiochip32 gpiochip96 $ echo 163 export $ ls export gpiochip128 gpiochip160 gpiochip64 tp_22 gpiochip0gpiochip144 gpiochip32 gpiochip96 unexport Thanks for the explanation. I should have probably asked earlier, why do we need to prevent sysfs access of deep sleep enable and sw reset pins? We don't seem to be using them in the kernel either. Thanks, Sekhar ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH v2 4/4] da850-evm: add baseboard UI expander buttons, switches and LEDs
Hi Sekhar, On Mon, Nov 22, 2010 at 7:00 AM, Nori, Sekhar nsek...@ti.com wrote: Thanks for the explanation. I should have probably asked earlier, why do we need to prevent sysfs access of deep sleep enable and sw reset pins? We don't seem to be using them in the kernel either. You're welcome. I was assuming that those pins were not exported as gpio pins on purpose; I was taking the prudent approach to prevent haphazard toggling of sw_rst and deep_sleep_en from userspace. sw_rst because it could initiate a reset of the cpu when toggled and deep_sleep_en because it can override the behaviour of davinci_pm_enter(). I'm not sure how they would be used by existing kernel classes either. The sw_rst pin could be used for reset but since it is on the other end of an i2c bus and there is an existing implementation of reset using the on chip watchdog I don't think it would be benficial to switch. Deep_sleep_en could override the behaviour in davinci_pm_enter() -- _maybe_ (I don't really know) it could be used as a hardware-assisted suspend-blocker? But I totally guessing here. Best Regards, Ben Gardiner --- Nanometrics Inc. http://www.nanometrics.ca ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
RE: [PATCH v2 4/4] da850-evm: add baseboard UI expander buttons, switches and LEDs
Hi Ben,
The board patches look good to me overall. Some minor comments below:
On Wed, Nov 17, 2010 at 01:09:37, Ben Gardiner wrote:
This patch adds a pca953x platform device for the tca6416 found on the evm
baseboard. The tca6416 is a GPIO expander, also found on the UI board at a
separate I2C address. The pins of the baseboard IO expander are connected to
software reset, deep sleep enable, test points, a push button, DIP switches
and
LEDs.
Add support for the push button, DIP switches and LEDs and test points (as
free GPIOs). The reset and deep sleep enable connections are reserved by the
setup routine so that userspace can't toggle those lines.
The existing tca6416-keypad driver was not employed because there was no
apararent way to register the LEDs connected to gpio's on the tca6416 while
simultaneously registering the tca6416-keypad instance.
Signed-off-by: Ben Gardiner bengardi...@nanometrics.ca
Reviewed-by: Chris Cordahi christophercord...@nanometrics.ca
CC: Govindarajan, Sriramakrishnan s...@ti.com
---
Changes since v1:
* adding note about why the tca6416-keypad driver was not used.
* adding Govindarajan, Sriramakrishnan, the author of the tca6416-keypad
driver
---
arch/arm/mach-davinci/board-da850-evm.c | 217
++-
1 files changed, 213 insertions(+), 4 deletions(-)
diff --git a/arch/arm/mach-davinci/board-da850-evm.c
b/arch/arm/mach-davinci/board-da850-evm.c
index dcf21e5..79f2c95 100644
--- a/arch/arm/mach-davinci/board-da850-evm.c
+++ b/arch/arm/mach-davinci/board-da850-evm.c
@@ -410,6 +410,208 @@ static int da850_evm_ui_expander_teardown(struct
i2c_client *client,
return 0;
}
+/* assign the baseboard expander's GPIOs after the UI board's */
+#define DA850_UI_EXPANDER_N_GPIOS ARRAY_SIZE(ui_expander_names)
+#define DA850_BB_EXPANDER_GPIO_BASE (DAVINCI_N_GPIO +
DA850_UI_EXPANDER_N_GPIOS)
+
+static const char const *baseboard_expander_names[] = {
+ deep_sleep_en, sw_rst, tp_23, tp_22, tp_21, user_pb1,
+ user_led2, user_led1, user_sw_1, user_sw_2, user_sw_3,
+ user_sw_4, user_sw_5, user_sw_6, user_sw_7, user_sw_8
+};
+
+#define DA850_DEEP_SLEEP_EN_OFFSET 0
+#define DA850_SW_RST_OFFSET 1
+#define DA850_PB1_OFFSET 5
+#define DA850_USER_LED2_OFFSET 6
+#define DA850_USER_SW_1_OFFSET 8
Again, I think index initialized array will work much
better here. Currently it is error prone to keep the defines
and the array of names in sync.
+
+#define DA850_N_USER_SW 8
+#define DA850_N_USER_LED 2
+
+static struct gpio_keys_button user_pb_gpio_key = {
da850evm_bb_pb might be a better name?
+ .code = KEY_PROG1,
+ .type = EV_KEY,
+ .active_low = 1,
+ .wakeup = 0,
+ .debounce_interval = DA850_PB_DEBOUNCE_MS,
+ .gpio = -1, /* assigned at runtime */
+ .desc = NULL, /* assigned at runtime */
+};
+
+static struct gpio_keys_platform_data user_pb_gpio_key_platform_data = {
Similarly da850evm_bb_pb_pdata instead of the long name?
+ .buttons = user_pb_gpio_key,
+ .nbuttons = 1,
+ .rep = 0, /* disable auto-repeat */
+ .poll_interval = DA850_PB_POLL_MS,
+};
+
+static struct platform_device user_pb_gpio_key_device = {
+ .name = gpio-keys,
+ .id = 1,
+ .dev = {
+ .platform_data = user_pb_gpio_key_platform_data
+ }
+};
+
+static struct gpio_keys_button user_sw_gpio_keys[DA850_N_USER_SW];
You could initialize most static fields here itself using:
static struct gpio_keys_button user_sw_gpio_keys[] = {
[0 ... DA850_N_USER_SW] = {
...
...
...
},
};
This way your runtime initialization will come down.
+
+static struct gpio_keys_platform_data user_sw_gpio_key_platform_data = {
+ .buttons = user_sw_gpio_keys,
+ .nbuttons = ARRAY_SIZE(user_sw_gpio_keys),
+ .rep = 0, /* disable auto-repeat */
+ .poll_interval = DA850_SW_POLL_MS,
+};
I wonder if we really have create to separate platform data
for switches and push buttons. If it is only the debounce period
that is different, it can be handled by initializing that field
differently.
+
+static struct platform_device user_sw_gpio_key_device = {
+ .name = gpio-keys,
+ .id = 2,
+ .dev = {
+ .platform_data = user_sw_gpio_key_platform_data
End with a ',' here.
+ }
+};
+
+static void da850_user_switches_init(unsigned gpio)
+{
+ int i;
+ struct gpio_keys_button *button;
+
+ for (i = 0; i DA850_N_USER_SW; i++) {
+ button = user_sw_gpio_keys[i];
+
+ button-code = SW_LID + i;
+ button-type = EV_SW;
+ button-active_low = 1;
+ button-wakeup = 0;
+ button-debounce_interval = DA850_PB_DEBOUNCE_MS;
+ button-desc
Re: [PATCH v2 4/4] da850-evm: add baseboard UI expander buttons, switches and LEDs
Hi Sehkar,
On Fri, Nov 19, 2010 at 7:14 AM, Nori, Sekhar nsek...@ti.com wrote:
The board patches look good to me overall. Some minor comments below:
Thanks -- and I appreciate your input.
On Wed, Nov 17, 2010 at 01:09:37, Ben Gardiner wrote:
[...]
+static const char const *baseboard_expander_names[] = {
+ deep_sleep_en, sw_rst, tp_23, tp_22, tp_21, user_pb1,
+ user_led2, user_led1, user_sw_1, user_sw_2, user_sw_3,
+ user_sw_4, user_sw_5, user_sw_6, user_sw_7, user_sw_8
+};
+
+#define DA850_DEEP_SLEEP_EN_OFFSET 0
+#define DA850_SW_RST_OFFSET 1
+#define DA850_PB1_OFFSET 5
+#define DA850_USER_LED2_OFFSET 6
+#define DA850_USER_SW_1_OFFSET 8
Again, I think index initialized array will work much
better here. Currently it is error prone to keep the defines
and the array of names in sync.
Agreed. Now that I've seen what you did with the previous patch I am
eager to apply that pattern also to the definitions in this patch.
[...]
+static struct gpio_keys_platform_data user_pb_gpio_key_platform_data = {
Similarly da850evm_bb_pb_pdata instead of the long name?
Will do.
[...]
+static struct gpio_keys_button user_sw_gpio_keys[DA850_N_USER_SW];
You could initialize most static fields here itself using:
static struct gpio_keys_button user_sw_gpio_keys[] = {
[0 ... DA850_N_USER_SW] = {
...
...
...
},
};
This way your runtime initialization will come down.
Indeed; I am eager to extend the pattern you introduced to this
initialization also.
+
+static struct gpio_keys_platform_data user_sw_gpio_key_platform_data = {
+ .buttons = user_sw_gpio_keys,
+ .nbuttons = ARRAY_SIZE(user_sw_gpio_keys),
+ .rep = 0, /* disable auto-repeat */
+ .poll_interval = DA850_SW_POLL_MS,
+};
I wonder if we really have create to separate platform data
for switches and push buttons. If it is only the debounce period
that is different, it can be handled by initializing that field
differently.
I see. Good idea; we can declare an array of gpio_keys_platform_data.
Note; it is the polling interval which differs, not the debounce interval.
+
+static struct platform_device user_sw_gpio_key_device = {
+ .name = gpio-keys,
+ .id = 2,
+ .dev = {
+ .platform_data = user_sw_gpio_key_platform_data
End with a ',' here.
Will do.
[...]
+static void da850_user_switches_init(unsigned gpio)
+{
+ int i;
+ struct gpio_keys_button *button;
+
+ for (i = 0; i DA850_N_USER_SW; i++) {
+ button = user_sw_gpio_keys[i];
+
+ button-code = SW_LID + i;
+ button-type = EV_SW;
+ button-active_low = 1;
+ button-wakeup = 0;
+ button-debounce_interval = DA850_PB_DEBOUNCE_MS;
+ button-desc = (char *)
+ baseboard_expander_names[DA850_USER_SW_1_OFFSET + i];
You could use some shorter names here. In the context of EVM file, 'bb'
will fit for base board, exp works for expander. Also, here it is
clear the macro is used as an array offset, so _OFFSET can be dropped
altogether. Similarly with _names. Also, the global and static symbols
should be pre-fixed with da850evm_ so it is easy to look up the symbol
file or object dump.
I see your points; 1) exp and bb for expander and baseboard variable
names, respectively 2) drop _OFFSET and _names 3) prefix statics and
globals with da850evm.
[...]
How does gpio_request prevent sysfs control?
To obtain access to a gpio through the sysfs interface the user must
first write the gpio number to 'export'. When the gpio has been
gpio_request()'d the result of writing to 'export' is nothing; whereas
writing to export would normally result in the appearance of a named
gpio line alongside 'export'.
I hope the following commands and their output illustrate my point:
$ cd /sys/class/gpio/
$ ls
export gpiochip128 gpiochip160 gpiochip64 unexport
gpiochip0gpiochip144 gpiochip32 gpiochip96
$ echo 160 export
$ ls
export gpiochip128 gpiochip160 gpiochip64 unexport
gpiochip0gpiochip144 gpiochip32 gpiochip96
$ echo 163 export
$ ls
export gpiochip128 gpiochip160 gpiochip64 tp_22
gpiochip0gpiochip144 gpiochip32 gpiochip96 unexport
Best Regards,
Ben Gardiner
---
Nanometrics Inc.
http://www.nanometrics.ca
___
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH v2 4/4] da850-evm: add baseboard UI expander buttons, switches and LEDs
Hi Ben,
On Fri, Nov 19, 2010 at 10:40:50AM -0500, Ben Gardiner wrote:
+
+static struct gpio_keys_platform_data user_sw_gpio_key_platform_data = {
+ .buttons = user_sw_gpio_keys,
+ .nbuttons = ARRAY_SIZE(user_sw_gpio_keys),
+ .rep = 0, /* disable auto-repeat */
+ .poll_interval = DA850_SW_POLL_MS,
+};
I wonder if we really have create to separate platform data
for switches and push buttons. If it is only the debounce period
that is different, it can be handled by initializing that field
differently.
I see. Good idea; we can declare an array of gpio_keys_platform_data.
Note; it is the polling interval which differs, not the debounce interval.
Another question is why do you want to poll the same device at
different intervals? The processor is already woken up so it makes sense
to do as much as possible instead of going to sleep. In your case
200/700 for every 1400ms interval you wake an extra time to poll
switches so unless polling switches takes very long time it is better to
combine them together (and into one input device, probably).
--
Dmitry
___
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH v2 4/4] da850-evm: add baseboard UI expander buttons, switches and LEDs
Hi Dmitry, On Fri, Nov 19, 2010 at 12:02 PM, Dmitry Torokhov dmitry.torok...@gmail.com wrote: Another question is why do you want to poll the same device at different intervals? The processor is already woken up so it makes sense to do as much as possible instead of going to sleep. In your case 200/700 for every 1400ms interval you wake an extra time to poll switches so unless polling switches takes very long time it is better to combine them together (and into one input device, probably). I see. That is a good question :) I didn't realize that splitting into two separate input devices with different polling intervals would have the opposite effect than what I intended. Thank you for pointing this out. I will definitely integrate them into one input device polled at 200ms. Best Regards, Ben Gardiner --- Nanometrics Inc. http://www.nanometrics.ca ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
