Re: [PATCH v2 4/4] da850-evm: add baseboard UI expander buttons, switches and LEDs

[Ben Gardiner <bengardiner@nanometrics.ca>]

Hi Sehkar,

On Fri, Nov 19, 2010 at 7:14 AM, Nori, Sekhar <nsekhar@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
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