Make gpio_keys accept board descriptors
Subsystems (7)
cover Make gpio_keys accept board descriptors
The goal I'm working toward is to rid the kernel of the global GPIO numberspace.
This means GPIO lines should be references by the local offset on the GPIO chip.
This patch set starts to move gpio_keys toward using GPIO look-up tables instead of global GPIO numbers to find their GPIOs.
As an example I did (I think) the necessary patches to convert DaVinci and i.MX to use this. There are several users also x86 platform devices.
Strategy:
- Merge the patch to input/keyboard/gpio_keys* to support using descriptors from board files.
- Refactor the whole universe to get proper names on GPIO chips, and reference the GPIO lines with local offsets using tables, as in the example patches in this series. These can trickle in after the first patch is merged in Input. The work to be done can be seen if you type:
git grep gpio_keys_platform_data
- When all are refactored, remove the "gpio" and "active low" members from the platform data and delete the associated legacy GPIO handling in the input driver for gpio_keys.
Posting this with the examples to get buy-in from Dmitry before I go and spend time refactoring the world.
I.e. I'm happy if patch 1 gets merged, the rest I will push through ARM SoC etc, or if nothing else works (like nonresponsive maintainers) I can push it through the GPIO tree without bothering Dmitry.
Linus Walleij (5):
Input: gpio-keys: Support getting descriptors from board
gpio: pca953x: Name the gpiochip after the I2C address
ARM: davinci: Switch DA850EVM to use GPIO descriptors
ARM: imx: Give all GPIO chips a unique name
ARM: imx: Use GPIO descriptors for gpio_keys
arch/arm/mach-davinci/board-da850-evm.c | 57 ++++++++++++++--- arch/arm/mach-imx/mach-armadillo5x0.c | 19 ++++-- arch/arm/mach-imx/mach-imx27_visstrim_m10.c | 26 ++++++-- arch/arm/mach-imx/mach-pcm037_eet.c | 52 +++++++++++---- arch/arm/mach-imx/mach-vpr200.c | 99 ++++++++++++++++++++++++----- arch/arm/mach-imx/mm-imx21.c | 12 ++-- arch/arm/mach-imx/mm-imx27.c | 12 ++-- arch/arm/mach-imx/mm-imx3.c | 12 ++-- drivers/gpio/gpio-pca953x.c | 17 ++++- drivers/input/keyboard/gpio_keys.c | 14 +++- drivers/input/keyboard/gpio_keys_polled.c | 10 +++ 11 files changed, 259 insertions(+), 71 deletions(-)
Signature · 1 line
2.14.3
PATCH 1/5 Input: gpio-keys: Support getting descriptors from board
There are any number of board files in the kernel providing keys/pushbuttons/switches using simple GPIO. In order to transfer these smoothly to use GPIO descriptors with descriptor tables in the board files and no static GPIO numbers, we need to accept that the board files provide named GPIOs associated with the GPIO chip device.
Luckily gpio_keys of both polled and interrupt-enabled type will optionally pass a "desc" field which we can use as the name of the GPIO to look up and request a GPIO line for a certain key defined in a per-board descriptor lookup table.
--- drivers/input/keyboard/gpio_keys.c | 14 +++++++++++++- drivers/input/keyboard/gpio_keys_polled.c | 10 ++++++++++ 2 files changed, 23 insertions(+), 1 deletion(-)
drivers/input/keyboard/gpio_keys.c +13 -1
@@ -520,7 +520,9 @@ static int gpio_keys_setup_key(struct platform_device *pdev, }else if (gpio_is_valid (button->gpio)) {/* * Legacy GPIO number, so request the GPIO here and -* convert it to descriptor. +* convert it to descriptor. This code goes away once +* we convert all old board files to provide descriptor +* tables. */ unsigned flags =GPIOF_IN ;
@@ -537,6 +539,16 @@ static int gpio_keys_setup_key(struct platform_device *pdev, bdata->gpiod =gpio_to_desc (button->gpio);if (!bdata->gpiod)return -EINVAL ; + }else { +/* +* Try to look up from the descriptor table, includes +* letting gpiolib handle inversion semantics. This is +* optional since we have interrupt-only keys as well. +*/ + bdata->gpiod =devm_gpiod_get_optional (dev, desc, +GPIOD_IN ); +if (IS_ERR (bdata->gpiod)) +return PTR_ERR (bdata->gpiod); }if (bdata->gpiod) {
drivers/input/keyboard/gpio_keys_polled.c +10 -0
@@ -342,6 +342,16 @@ static int gpio_keys_polled_probe(struct platform_device *pdev) button->gpio);return -EINVAL ; } + }else { +/* +* Try to look up from the descriptor table, includes +* letting gpiolib handle inversion semantics. +*/ + bdata->gpiod =devm_gpiod_get_optional (dev, + button->desc, +GPIOD_IN ); +if (IS_ERR (bdata->gpiod)) +return PTR_ERR (bdata->gpiod); } bdata->last_state =-1 ;
Signature · 1 line
2.14.3
PATCH 2/5 gpio: pca953x: Name the gpiochip after the I2C address
In reply to Linus WalleijJust putting the name of the I2C device as name for the GPIO chip (label) is ambigous, and makes it hard for us to use GPIO descriptor tables on systems such as DaVinci DA850EVM which has two chips but on I2C address 0x20 and 0x21.
Instead, append "-XX" to the GPIOchip name using the I2C address so we get a unique chip name that can be used in descriptor tables, such as "tca6416-20" and "tca6416-21" on the DaVinci DA850EVM.
--- drivers/gpio/gpio-pca953x.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-)
drivers/gpio/gpio-pca953x.c +14 -3
@@ -413,10 +413,17 @@ static void pca953x_gpio_set_multiple(struct gpio_chip *gc,mutex_unlock (&chip->i2c_lock); } -static void pca953x_setup_gpio (struct pca953x_chip *chip,int gpios) +static int pca953x_setup_gpio (struct pca953x_chip *chip,int gpios) { +struct i2c_client *client = chip->client;struct gpio_chip *gc; +static char *chipname; +/* Gives a chip name from the chip name and I2C address */ + chipname =devm_kasprintf (&client->dev,GFP_KERNEL ,"%s-%02x" , + client->name, client->addr); +if (!chipname) +return -ENOMEM ; gc = &chip->gpio_chip; gc->direction_input = pca953x_gpio_direction_input;
@@ -429,10 +436,12 @@ static void pca953x_setup_gpio(struct pca953x_chip *chip, int gpios) gc->base = chip->gpio_start; gc->ngpio = gpios; - gc->label = chip->client->name; + gc->label = chipname; gc->parent = &chip->client->dev; gc->owner =THIS_MODULE ; gc->names = chip->names; + +return 0 ; }#ifdef CONFIG_GPIO_PCA953X_IRQ
@@ -848,7 +857,9 @@ static int pca953x_probe(struct i2c_client *client,/* initialize cached registers from their original values. * we can't share this chip with another i2c master. */ -pca953x_setup_gpio (chip, chip->driver_data &PCA_GPIO_MASK ); + ret =pca953x_setup_gpio (chip, chip->driver_data &PCA_GPIO_MASK ); +if (ret) + goto err_exit;if (chip->gpio_chip.ngpio <=8 ) { chip->write_regs = pca953x_write_regs_8;
Signature · 1 line
2.14.3
PATCH 2/5 gpio: pca953x: Name the gpiochip after the I2C address
Quoted 9 lines · Linus Walleij
Just putting the name of the I2C device as name for the GPIO chip (label) is ambigous, and makes it hard for us to use GPIO descriptor tables on systems such as DaVinci DA850EVM which has two chips but on I2C address
0x20and0x21.Instead, append "-XX" to the GPIOchip name using the I2C address so we get a unique chip name that can be used in descriptor tables, such as "tca6416-20" and "tca6416-21" on the DaVinci DA850EVM.
Hi Linus
Are you making the assumption that there are not two devices at the same address but on different I2C busses?
How safe is that assumption? Can you include the adapter ID in the string?
Andrew
PATCH 2/5 gpio: pca953x: Name the gpiochip after the I2C address
Quoted 11 lines · Andrew Lunn
Quoted 9 lines · Linus Walleij
Just putting the name of the I2C device as name for the GPIO chip (label) is ambigous, and makes it hard for us to use GPIO descriptor tables on systems such as DaVinci DA850EVM which has two chips but on I2C address
0x20and0x21.Instead, append "-XX" to the GPIOchip name using the I2C address so we get a unique chip name that can be used in descriptor tables, such as "tca6416-20" and "tca6416-21" on the DaVinci DA850EVM.
Quoted 4 lines
Hi Linus
Are you making the assumption that there are not two devices at the same address but on different I2C busses?
Yes.
Quoted 2 lines
How safe is that assumption? Can you include the adapter ID in the string?
Sure, I just had to come up with something.
The universe of board files is not very big, so the number of potentially clashing things is not eternal. This is just a transient "contain and deprecate" thing so I think it's unlikely that they clash but usually when I say something like that the next day it happens...
So I can add the adapter number :)
I am waiting for more feedback on the series before resending.
Yours,
Linus Walleij
PATCH 3/5 ARM: davinci: Switch DA850EVM to use GPIO descriptors
In reply to Linus WalleijTo get rid of the global GPIO numberspace we need to move to using descriptor tables with local offsets on the GPIO chip to reference the GPIO lines used by kernel drivers.
This moves the DaVinci DA850EVM board over to using GPIO descriptors for passing GPIOs to the GPIO keys.
--- arch/arm/mach-davinci/board-da850-evm.c | 57 ++++++++++++++++++++++++++++----- 1 file changed, 49 insertions(+), 8 deletions(-)
arch/arm/mach-davinci/board-da850-evm.c +49 -8
@@ -404,11 +404,9 @@ static const char * const da850_evm_ui_exp[] = {static struct gpio_keys_button da850_evm_ui_keys[] = { [0 ...DA850_N_UI_PB -1 ] = { .type =EV_KEY , - .active_low =1 , .wakeup =0 , .debounce_interval =DA850_KEYS_DEBOUNCE_MS , .code =-1 ,/* assigned at runtime */ - .gpio =-1 ,/* assigned at runtime */ .desc =NULL ,/* assigned at runtime */ }, };
@@ -427,6 +425,29 @@ static struct platform_device da850_evm_ui_keys_device = { }, }; +static struct gpiod_lookup_table da850_evm_ui_keys_gpios_table = { + .dev_id ="gpio-keys-polled" , + .table = { +GPIO_LOOKUP ("tca6416-20" ,DA850_EVM_UI_EXP_PB8 , +"pb8" ,GPIO_ACTIVE_LOW ), +GPIO_LOOKUP ("tca6416-20" ,DA850_EVM_UI_EXP_PB7 , +"pb7" ,GPIO_ACTIVE_LOW ), +GPIO_LOOKUP ("tca6416-20" ,DA850_EVM_UI_EXP_PB6 , +"pb6" ,GPIO_ACTIVE_LOW ), +GPIO_LOOKUP ("tca6416-20" ,DA850_EVM_UI_EXP_PB5 , +"pb5" ,GPIO_ACTIVE_LOW ), +GPIO_LOOKUP ("tca6416-20" ,DA850_EVM_UI_EXP_PB4 , +"pb4" ,GPIO_ACTIVE_LOW ), +GPIO_LOOKUP ("tca6416-20" ,DA850_EVM_UI_EXP_PB3 , +"pb3" ,GPIO_ACTIVE_LOW ), +GPIO_LOOKUP ("tca6416-20" ,DA850_EVM_UI_EXP_PB2 , +"pb2" ,GPIO_ACTIVE_LOW ), +GPIO_LOOKUP ("tca6416-20" ,DA850_EVM_UI_EXP_PB1 , +"pb1" ,GPIO_ACTIVE_LOW ), + + }, +}; +static void da850_evm_ui_keys_init (unsigned gpio) {int i;
@@ -436,8 +457,8 @@ static void da850_evm_ui_keys_init(unsigned gpio) button = &da850_evm_ui_keys[i]; button->code =KEY_F8 - i; button->desc = da850_evm_ui_exp[DA850_EVM_UI_EXP_PB8 + i]; - button->gpio = gpio +DA850_EVM_UI_EXP_PB8 + i; } +gpiod_add_lookup_table (&da850_evm_ui_keys_gpios_table); }#ifdef CONFIG_DA850_UI_SD_VIDEO_PORT
@@ -572,21 +593,17 @@ static const char * const da850_evm_bb_exp[] = {static struct gpio_keys_button da850_evm_bb_keys[] = { [0 ] = { .type =EV_KEY , - .active_low =1 , .wakeup =0 , .debounce_interval =DA850_KEYS_DEBOUNCE_MS , .code =KEY_PROG1 , .desc =NULL ,/* assigned at runtime */ - .gpio =-1 ,/* assigned at runtime */ }, [1 ...DA850_N_BB_USER_SW ] = { .type =EV_SW , - .active_low =1 , .wakeup =0 , .debounce_interval =DA850_KEYS_DEBOUNCE_MS , .code =-1 ,/* assigned at runtime */ .desc =NULL ,/* assigned at runtime */ - .gpio =-1 ,/* assigned at runtime */ }, };
@@ -604,6 +621,30 @@ static struct platform_device da850_evm_bb_keys_device = { }, }; +static struct gpiod_lookup_table da850_evm_bb_keys_gpios_table = { + .dev_id ="gpio-keys-polled" , + .table = { +GPIO_LOOKUP ("tca6416-21" ,DA850_EVM_BB_EXP_USER_PB1 , +"user_pb1" ,GPIO_ACTIVE_LOW ), +GPIO_LOOKUP ("tca6416-21" ,DA850_EVM_BB_EXP_USER_SW1 , +"user_sw1" ,GPIO_ACTIVE_LOW ), +GPIO_LOOKUP ("tca6416-21" ,DA850_EVM_BB_EXP_USER_SW2 , +"user_sw2" ,GPIO_ACTIVE_LOW ), +GPIO_LOOKUP ("tca6416-21" ,DA850_EVM_BB_EXP_USER_SW3 , +"user_sw3" ,GPIO_ACTIVE_LOW ), +GPIO_LOOKUP ("tca6416-21" ,DA850_EVM_BB_EXP_USER_SW4 , +"user_sw4" ,GPIO_ACTIVE_LOW ), +GPIO_LOOKUP ("tca6416-21" ,DA850_EVM_BB_EXP_USER_SW5 , +"user_sw5" ,GPIO_ACTIVE_LOW ), +GPIO_LOOKUP ("tca6416-21" ,DA850_EVM_BB_EXP_USER_SW6 , +"user_sw6" ,GPIO_ACTIVE_LOW ), +GPIO_LOOKUP ("tca6416-21" ,DA850_EVM_BB_EXP_USER_SW7 , +"user_sw7" ,GPIO_ACTIVE_LOW ), +GPIO_LOOKUP ("tca6416-21" ,DA850_EVM_BB_EXP_USER_SW8 , +"user_sw8" ,GPIO_ACTIVE_LOW ), + }, +}; +static void da850_evm_bb_keys_init (unsigned gpio) {int i;
@@ -617,8 +658,8 @@ static void da850_evm_bb_keys_init(unsigned gpio) button = &da850_evm_bb_keys[i +1 ]; button->code =SW_LID + i; button->desc = da850_evm_bb_exp[DA850_EVM_BB_EXP_USER_SW1 + i]; - button->gpio = gpio +DA850_EVM_BB_EXP_USER_SW1 + i; } +gpiod_add_lookup_table (&da850_evm_bb_keys_gpios_table); }#define DA850_N_BB_USER_LED 2
Signature · 1 line
2.14.3
PATCH 4/5 ARM: imx: Give all GPIO chips a unique name
In reply to Linus WalleijThe name/label of the GPIO chips is used in GPIO lookup tables so these need to be unique per chip. Suffix each GPIO chip with an instance number so these lookups will work as expected.
--- arch/arm/mach-imx/mm-imx21.c | 12 ++++++------ arch/arm/mach-imx/mm-imx27.c | 12 ++++++------ arch/arm/mach-imx/mm-imx3.c | 12 ++++++------ 3 files changed, 18 insertions(+), 18 deletions(-)
arch/arm/mach-imx/mm-imx21.c +6 -6
@@ -84,12 +84,12 @@ void __init imx21_soc_init(void)mxc_arch_reset_init (MX21_IO_ADDRESS (MX21_WDOG_BASE_ADDR ));mxc_device_init (); -mxc_register_gpio ("imx21-gpio" ,0 ,MX21_GPIO1_BASE_ADDR ,SZ_256 ,MX21_INT_GPIO ,0 ); -mxc_register_gpio ("imx21-gpio" ,1 ,MX21_GPIO2_BASE_ADDR ,SZ_256 ,MX21_INT_GPIO ,0 ); -mxc_register_gpio ("imx21-gpio" ,2 ,MX21_GPIO3_BASE_ADDR ,SZ_256 ,MX21_INT_GPIO ,0 ); -mxc_register_gpio ("imx21-gpio" ,3 ,MX21_GPIO4_BASE_ADDR ,SZ_256 ,MX21_INT_GPIO ,0 ); -mxc_register_gpio ("imx21-gpio" ,4 ,MX21_GPIO5_BASE_ADDR ,SZ_256 ,MX21_INT_GPIO ,0 ); -mxc_register_gpio ("imx21-gpio" ,5 ,MX21_GPIO6_BASE_ADDR ,SZ_256 ,MX21_INT_GPIO ,0 ); +mxc_register_gpio ("imx21-gpio-0" ,0 ,MX21_GPIO1_BASE_ADDR ,SZ_256 ,MX21_INT_GPIO ,0 ); +mxc_register_gpio ("imx21-gpio-1" ,1 ,MX21_GPIO2_BASE_ADDR ,SZ_256 ,MX21_INT_GPIO ,0 ); +mxc_register_gpio ("imx21-gpio-2" ,2 ,MX21_GPIO3_BASE_ADDR ,SZ_256 ,MX21_INT_GPIO ,0 ); +mxc_register_gpio ("imx21-gpio-3" ,3 ,MX21_GPIO4_BASE_ADDR ,SZ_256 ,MX21_INT_GPIO ,0 ); +mxc_register_gpio ("imx21-gpio-4" ,4 ,MX21_GPIO5_BASE_ADDR ,SZ_256 ,MX21_INT_GPIO ,0 ); +mxc_register_gpio ("imx21-gpio-5" ,5 ,MX21_GPIO6_BASE_ADDR ,SZ_256 ,MX21_INT_GPIO ,0 );pinctrl_provide_dummies (); imx_add_imx_dma("imx21-dma", MX21_DMA_BASE_ADDR,
arch/arm/mach-imx/mm-imx27.c +6 -6
@@ -85,12 +85,12 @@ void __init imx27_soc_init(void)mxc_device_init ();/* i.mx27 has the i.mx21 type gpio */ -mxc_register_gpio ("imx21-gpio" ,0 ,MX27_GPIO1_BASE_ADDR ,SZ_256 ,MX27_INT_GPIO ,0 ); -mxc_register_gpio ("imx21-gpio" ,1 ,MX27_GPIO2_BASE_ADDR ,SZ_256 ,MX27_INT_GPIO ,0 ); -mxc_register_gpio ("imx21-gpio" ,2 ,MX27_GPIO3_BASE_ADDR ,SZ_256 ,MX27_INT_GPIO ,0 ); -mxc_register_gpio ("imx21-gpio" ,3 ,MX27_GPIO4_BASE_ADDR ,SZ_256 ,MX27_INT_GPIO ,0 ); -mxc_register_gpio ("imx21-gpio" ,4 ,MX27_GPIO5_BASE_ADDR ,SZ_256 ,MX27_INT_GPIO ,0 ); -mxc_register_gpio ("imx21-gpio" ,5 ,MX27_GPIO6_BASE_ADDR ,SZ_256 ,MX27_INT_GPIO ,0 ); +mxc_register_gpio ("imx21-gpio-0" ,0 ,MX27_GPIO1_BASE_ADDR ,SZ_256 ,MX27_INT_GPIO ,0 ); +mxc_register_gpio ("imx21-gpio-1" ,1 ,MX27_GPIO2_BASE_ADDR ,SZ_256 ,MX27_INT_GPIO ,0 ); +mxc_register_gpio ("imx21-gpio-2" ,2 ,MX27_GPIO3_BASE_ADDR ,SZ_256 ,MX27_INT_GPIO ,0 ); +mxc_register_gpio ("imx21-gpio-3" ,3 ,MX27_GPIO4_BASE_ADDR ,SZ_256 ,MX27_INT_GPIO ,0 ); +mxc_register_gpio ("imx21-gpio-4" ,4 ,MX27_GPIO5_BASE_ADDR ,SZ_256 ,MX27_INT_GPIO ,0 ); +mxc_register_gpio ("imx21-gpio-5" ,5 ,MX27_GPIO6_BASE_ADDR ,SZ_256 ,MX27_INT_GPIO ,0 );pinctrl_provide_dummies (); imx_add_imx_dma("imx27-dma", MX27_DMA_BASE_ADDR,
arch/arm/mach-imx/mm-imx3.c +6 -6
@@ -188,9 +188,9 @@ void __init imx31_soc_init(void)mxc_arch_reset_init (MX31_IO_ADDRESS (MX31_WDOG_BASE_ADDR ));mxc_device_init (); -mxc_register_gpio ("imx31-gpio" ,0 ,MX31_GPIO1_BASE_ADDR ,SZ_16K ,MX31_INT_GPIO1 ,0 ); -mxc_register_gpio ("imx31-gpio" ,1 ,MX31_GPIO2_BASE_ADDR ,SZ_16K ,MX31_INT_GPIO2 ,0 ); -mxc_register_gpio ("imx31-gpio" ,2 ,MX31_GPIO3_BASE_ADDR ,SZ_16K ,MX31_INT_GPIO3 ,0 ); +mxc_register_gpio ("imx31-gpio-0" ,0 ,MX31_GPIO1_BASE_ADDR ,SZ_16K ,MX31_INT_GPIO1 ,0 ); +mxc_register_gpio ("imx31-gpio-1" ,1 ,MX31_GPIO2_BASE_ADDR ,SZ_16K ,MX31_INT_GPIO2 ,0 ); +mxc_register_gpio ("imx31-gpio-2" ,2 ,MX31_GPIO3_BASE_ADDR ,SZ_16K ,MX31_INT_GPIO3 ,0 );pinctrl_provide_dummies ();
@@ -298,9 +298,9 @@ void __init imx35_soc_init(void)mxc_arch_reset_init (MX35_IO_ADDRESS (MX35_WDOG_BASE_ADDR ));mxc_device_init (); -mxc_register_gpio ("imx35-gpio" ,0 ,MX35_GPIO1_BASE_ADDR ,SZ_16K ,MX35_INT_GPIO1 ,0 ); -mxc_register_gpio ("imx35-gpio" ,1 ,MX35_GPIO2_BASE_ADDR ,SZ_16K ,MX35_INT_GPIO2 ,0 ); -mxc_register_gpio ("imx35-gpio" ,2 ,MX35_GPIO3_BASE_ADDR ,SZ_16K ,MX35_INT_GPIO3 ,0 ); +mxc_register_gpio ("imx35-gpio-0" ,0 ,MX35_GPIO1_BASE_ADDR ,SZ_16K ,MX35_INT_GPIO1 ,0 ); +mxc_register_gpio ("imx35-gpio-1" ,1 ,MX35_GPIO2_BASE_ADDR ,SZ_16K ,MX35_INT_GPIO2 ,0 ); +mxc_register_gpio ("imx35-gpio-2" ,2 ,MX35_GPIO3_BASE_ADDR ,SZ_16K ,MX35_INT_GPIO3 ,0 );pinctrl_provide_dummies ();if (to_version ==1 ) {
Signature · 1 line
2.14.3
PATCH 5/5 ARM: imx: Use GPIO descriptors for gpio_keys
In reply to Linus WalleijThis switches all relevant i.MX boards still using boardfiles to using GPIO descriptors to look up the GPIO lines for gpio_keys.
Most conversions were straight-forward, I dropped comments on how the offsets are looked up where appropriate.
The VPR200 board was seemingly incorrect. It assigned the members of struct gpio_keys_button with the direct, non-C99 syntax like so:
#define VPR_KEY_DEBOUNCE 500
static struct gpio_keys_button vpr200_gpio_keys_table[] = {
{KEY_F2, GPIO_BUTTON1, 1, "vpr-keys: F2", 0, VPR_KEY_DEBOUNCE},
{KEY_F3, GPIO_BUTTON2, 1, "vpr-keys: F3", 0, VPR_KEY_DEBOUNCE},
(...)This is confusing as it seems to have been wrong already when the patch was merged. The members are assigned in order:
struct gpio_keys_button {
unsigned int code;
int gpio;
int active_low;
const char *desc;
unsigned int type;
int wakeup;
int debounce_interval;
bool can_disable;(...)
Meaning KEY_XX, GPIO_BUTTON1, 1, "key name", type us right, then comes wakeup capability and this should be 0 or 1, but is instead assigned VPR_KEY_DEBOUNCE which is 500. I have corrected the code to do what I think the author intended. I consider this a non-critical fix because I bet it works (or the board is completely unused), but add the fixing tag for reference.
--- arch/arm/mach-imx/mach-armadillo5x0.c | 19 ++++-- arch/arm/mach-imx/mach-imx27_visstrim_m10.c | 26 ++++++-- arch/arm/mach-imx/mach-pcm037_eet.c | 52 +++++++++++---- arch/arm/mach-imx/mach-vpr200.c | 99 ++++++++++++++++++++++++----- 4 files changed, 155 insertions(+), 41 deletions(-)
arch/arm/mach-imx/mach-armadillo5x0.c +15 -4
@@ -28,6 +28,7 @@#include <linux/clk.h> #include <linux/platform_device.h> #include <linux/gpio.h> +#include <linux/gpio/machine.h> #include <linux/smsc911x.h> #include <linux/interrupt.h> #include <linux/irq.h>
@@ -265,14 +266,10 @@ static struct i2c_board_info armadillo5x0_i2c_rtc = {static struct gpio_keys_button armadillo5x0_buttons[] = { { .code =KEY_ENTER ,/*28*/ - .gpio =IOMUX_TO_GPIO (MX31_PIN_SCLK0 ), - .active_low =1 , .desc ="menu" , .wakeup =1 , }, { .code =KEY_BACK ,/*158*/ - .gpio =IOMUX_TO_GPIO (MX31_PIN_SRST0 ), - .active_low =1 , .desc ="back" , .wakeup =1 , }
@@ -284,6 +281,19 @@ static const struct gpio_keys_platform_data .nbuttons = ARRAY_SIZE(armadillo5x0_buttons), }; +static struct gpiod_lookup_table armadillo5x0_buttons_gpios_table = { + .dev_id ="gpio-keys" , + .table = { +/* +* These are GPIOs M31_PIN_SCLK0 and MX31_PIN_SRSR0, +* global GPIO numbers 66 and 67, with 32 GPIOs per chip +* these are chip 2 offsets 2 and 3 +*/ +GPIO_LOOKUP ("imx31-gpio-2" ,2 ,"menu" ,GPIO_ACTIVE_LOW ), +GPIO_LOOKUP ("imx31-gpio-2" ,3 ,"back" ,GPIO_ACTIVE_LOW ), + }, +}; +/* * NAND Flash */
@@ -525,6 +535,7 @@ static void __init armadillo5x0_late(void)gpio_to_irq (IOMUX_TO_GPIO (MX31_PIN_GPIO1_0 ));platform_add_devices (devices,ARRAY_SIZE (devices)); +gpiod_add_lookup_table (&armadillo5x0_buttons_gpios_table);imx_add_gpio_keys (&armadillo5x0_button_data);/* SMSC9118 IRQ pin */
arch/arm/mach-imx/mach-imx27_visstrim_m10.c +20 -6
@@ -29,6 +29,7 @@#include <linux/platform_data/pca953x.h> #include <linux/input.h> #include <linux/gpio.h> +#include <linux/gpio/machine.h> #include <linux/delay.h> #include <linux/dma-mapping.h> #include <linux/leds.h>
@@ -274,25 +275,19 @@ static struct gpio_keys_button visstrim_gpio_keys[] = { { .type = EV_KEY, .code = KEY_RESTART, - .gpio = (GPIO_PORTC + 15), .desc = "Default config", - .active_low = 0, .wakeup = 1, }, { .type = EV_KEY, .code = KEY_RECORD, - .gpio = (GPIO_PORTF + 14), .desc = "Record", - .active_low = 0, .wakeup = 1, }, { .type = EV_KEY, .code = KEY_STOP, - .gpio = (GPIO_PORTF + 13), .desc = "Stop", - .active_low = 0, .wakeup = 1, } };
@@ -303,6 +298,24 @@ static const struct gpio_keys_platform_data .nbuttons = ARRAY_SIZE(visstrim_gpio_keys), }; +static struct gpiod_lookup_table visstrim_gpio_keys_gpios_table = { + .dev_id ="gpio-keys" , + .table = { +/* +* These keys are on "PORT E" offset 15 and "PORT F" +* offsets 14 and 13. This corresponds to imx21-gpio-4 +* and imx21-gpio-5, as this SoC has the i.MX21 GPIO +* block. +*/ +GPIO_LOOKUP ("imx21-gpio-4" ,15 ,"Default config" , +GPIO_ACTIVE_HIGH ), +GPIO_LOOKUP ("imx21-gpio-5" ,14 ,"Record" , +GPIO_ACTIVE_HIGH ), +GPIO_LOOKUP ("imx21-gpio-5" ,13 ,"Stop" , +GPIO_ACTIVE_HIGH ), + }, +}; +/* led */ static const struct gpio_led visstrim_m10_leds[]__initconst = { {
@@ -563,6 +576,7 @@ static void __init visstrim_m10_late_init(void)if (ret)pr_err ("Failed to request gpios (%d)\n" , ret); +gpiod_add_lookup_table (&visstrim_gpio_keys_gpios_table);imx_add_gpio_keys (&visstrim_gpio_keys_platform_data); imx_add_platform_device("mx27vis", 0, NULL, 0, &snd_mx27vis_pdata,
arch/arm/mach-imx/mach-pcm037_eet.c +38 -14
141 lines not on this page · raw message
arch/arm/mach-imx/mach-vpr200.c +82 -17
131 lines not on this page · raw message
Signature · 1 line
2.14.3
Dmitry Torokhov
maintainercover Make gpio_keys accept board descriptors
In reply to Linus WalleijHi Linus,
Quoted 26 lines · Linus Walleij
The goal I'm working toward is to rid the kernel of the global GPIO numberspace.
This means GPIO lines should be references by the local offset on the GPIO chip.
This patch set starts to move
gpio_keystoward using GPIO look-up tables instead of global GPIO numbers to find their GPIOs.As an example I did (I think) the necessary patches to convert DaVinci and i.MX to use this. There are several users also x86 platform devices.
Strategy:
- Merge the patch to input/keyboard/gpio_keys* to support using descriptors from board files.
- Refactor the whole universe to get proper names on GPIO chips, and reference the GPIO lines with local offsets using tables, as in the example patches in this series. These can trickle in after the first patch is merged in Input. The work to be done can be seen if you type:
git grepgpio_keys_platform_data
Quoted 6 lines
- When all are refactored, remove the "gpio" and "active low" members from the platform data and delete the associated legacy GPIO handling in the input driver for
gpio_keys.Posting this with the examples to get buy-in from Dmitry before I go and spend time refactoring the world.
I think this is a worthy goal, but I wonder if we could get static GPIO descriptors work with fwnode_get_named_gpiod() so we could retire the platform data parsing altogether. We'd need to extend static device properties to have notion of children though.
Thanks.
Signature · 1 line
Dmitry
cover Make gpio_keys accept board descriptors
Quoted 2 lines · Dmitry Torokhov
I think this is a worthy goal, but I wonder if we could get static GPIO descriptors work with
fwnode_get_named_gpiod()
I think that will just work, I can try it on something and see.
Quoted 3 lines
so we could retire the
platform data parsing altogether. We'd need to extend static device properties to have notion of children though.
So we are talking about these static device properties I assume?
/**
* struct gpiod_lookup - lookup table
* @chip_label: name of the chip the GPIO belongs to
* @chip_hwnum: hardware number (i.e. relative to the chip) of the GPIO
* @con_id: name of the GPIO from the device's point of view
* @idx: index of the GPIO in case several GPIOs share the same name
* @flags: mask of GPIO_* values
*
* gpiod_lookup is a lookup table for associating GPIOs to specific devices and
* functions using platform data.
*/
struct gpiod_lookup {
const char *chip_label;
u16 chip_hwnum;
const char *con_id;
unsigned int idx;
enum gpio_lookup_flags flags;
};struct gpiod_lookup_table {
struct list_head list;
const char *dev_id;
struct gpiod_lookup table[];
};The fact that keys are modeled as children in DTS/ACPI DSDT more of a pecularity to these description languages providing opaque configuration nodes.
E.g. these children reflects struct gpio_keys_button being a child of struct gpio_keys_platform_data in the board files. But
it is only one device, "gpio-keys" in the Linux device model. The buttons/keys are not devices.
Likewise, in device tree the buttons are not really devices under the "gpio-keys" device, they are just property nodes in the device tree conveniently set in a hierarchy to contain the information. I bet ACPI works the same.
Since GPIO descriptors (like clocks, regulators ...) are associated with the device, this becomes problematic: these children are not devices. So they need to be associated with the device to begin with.
In that case, I would opt to just use the unsigned int idx field for determining child number from the descriptor table, use con_id for naming the child and we are back at essentially the same structure as a backend for fwnode.
Due to the complexity of fwnode requireing an entire parser for fwnode_operations etc it's a bit of heavy lifting so I need to think and experiment some more.
Yours,
Linus Walleij
Hi Dmitry,
Quoted 15 lines · Dmitry Torokhov
Quoted 13 lines · Linus Walleij
The goal I'm working toward is to rid the kernel of the global GPIO numberspace.
This means GPIO lines should be references by the local offset on the GPIO chip.
This patch set starts to move
gpio_keystoward using GPIO look-up tables instead of global GPIO numbers to find their GPIOs.As an example I did (I think) the necessary patches to convert DaVinci and i.MX to use this. There are several users also x86 platform devices.
(...)
Quoted 4 lines
I think this is a worthy goal, but I wonder if we could get static GPIO descriptors work with
fwnode_get_named_gpiod()so we could retire the platform data parsing altogether. We'd need to extend static device properties to have notion of children though.
Do we have this now? I've looked at Heikki's et al work on software nodes but I cannot see whether we are there now.
We have fwnode_create_software_node() and friends, but I haven't seen if this can be used with input and GPIO descriptors are still a bit on the side. I can create a lot of properties but not really add a descriptor table as a software node as far as I can tell. I'm also a bit lost on whether it will be possible to get there sadly :/
Yours,
Linus Walleij
Quoted 28 lines · Linus Walleij
Hi Dmitry,
Quoted 15 lines · Dmitry Torokhov
Quoted 13 lines · Linus Walleij
The goal I'm working toward is to rid the kernel of the global GPIO numberspace.
This means GPIO lines should be references by the local offset on the GPIO chip.
This patch set starts to move
gpio_keystoward using GPIO look-up tables instead of global GPIO numbers to find their GPIOs.As an example I did (I think) the necessary patches to convert DaVinci and i.MX to use this. There are several users also x86 platform devices.
(...)
Quoted 4 lines
I think this is a worthy goal, but I wonder if we could get static GPIO descriptors work with
fwnode_get_named_gpiod()so we could retire the platform data parsing altogether. We'd need to extend static device properties to have notion of children though.Do we have this now? I've looked at Heikki's et al work on software nodes but I cannot see whether we are there now.
Quoted 6 lines
We have
fwnode_create_software_node()and friends, but I haven't seen if this can be used with input and GPIO descriptors are still a bit on the side. I can create a lot of properties but not really add a descriptor table as a software node as far as I can tell. I'm also a bit lost on whether it will be possible to get there sadly :/
I'm sorry but I'm not completely sure what is this about? Are software nodes still missing something that would prevent us from for example using them to describe the GPIO information exactly the same way it is described in DT? I don't know if that is what we want, but I'm just trying to understand what is still missing? Dmitry?
If there is still something missing, then let's fix that.
thanks,
Signature · 1 line
heikki
Dmitry Torokhov
maintainerQuoted 36 lines · Heikki Krogerus
Quoted 28 lines · Linus Walleij
Hi Dmitry,
Quoted 15 lines · Dmitry Torokhov
Quoted 13 lines · Linus Walleij
The goal I'm working toward is to rid the kernel of the global GPIO numberspace.
This means GPIO lines should be references by the local offset on the GPIO chip.
This patch set starts to move
gpio_keystoward using GPIO look-up tables instead of global GPIO numbers to find their GPIOs.As an example I did (I think) the necessary patches to convert DaVinci and i.MX to use this. There are several users also x86 platform devices.
(...)
Quoted 4 lines
I think this is a worthy goal, but I wonder if we could get static GPIO descriptors work with
fwnode_get_named_gpiod()so we could retire the platform data parsing altogether. We'd need to extend static device properties to have notion of children though.Do we have this now? I've looked at Heikki's et al work on software nodes but I cannot see whether we are there now.
Quoted 6 lines
We have
fwnode_create_software_node()and friends, but I haven't seen if this can be used with input and GPIO descriptors are still a bit on the side. I can create a lot of properties but not really add a descriptor table as a software node as far as I can tell. I'm also a bit lost on whether it will be possible to get there sadly :/
Quoted 5 lines
I'm sorry but I'm not completely sure what is this about? Are software nodes still missing something that would prevent us from for example using them to describe the GPIO information exactly the same way it is described in DT? I don't know if that is what we want, but I'm just trying to understand what is still missing? Dmitry?
No, my changes to improve the fwnode handling of references have landed, a while ago, so now I need to refresh the series of patches to gpiolib I was working on around Plumbers time last year. Linus seemed mostly OK with them, so it just a matter of finding time and picking this up again.
Thanks.
Signature · 1 line
Dmitry
Quoted 5 lines · Dmitry Torokhov
my changes to improve the fwnode handling of references have landed, a while ago, so now I need to refresh the series of patches to gpiolib I was working on around Plumbers time last year. Linus seemed mostly OK with them, so it just a matter of finding time and picking this up again.
OK I'll be happy to help of course, I am fixing board files to use GPIO descriptors a bit all over the place but then I am only fixing the GPIO problem, and this approach is more ambitious so I am excited if we can move it forward.
Yours,
Linus Walleij