Hi Sehkar,
On Fri, Nov 19, 2010 at 7:14 AM, Nori, Sekhar <nsekhar@xxxxxx> 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
gpiochip0 gpiochip144 gpiochip32 gpiochip96
$ echo 160 > export
$ ls
export gpiochip128 gpiochip160 gpiochip64 unexport
gpiochip0 gpiochip144 gpiochip32 gpiochip96
$ echo 163 > export
$ ls
export gpiochip128 gpiochip160 gpiochip64 tp_22
gpiochip0 gpiochip144 gpiochip32 gpiochip96 unexport
Best Regards,
Ben Gardiner
---
Nanometrics Inc.
http://www.nanometrics.ca
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
