| Message ID | 1383948866-32672-2-git-send-email-sre@debian.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi! > + if (of_get_property(np, "linux,input-no-autorepeat", NULL)) > + keypad_data->no_autorepeat = true; From 2/2: +Optional Properties specific to linux: +- linux,keypad-no-autorepeat: do not enable autorepeat feature. I'm confused now.
Hi, On Mon, Nov 11, 2013 at 11:19:41PM +0100, Pavel Machek wrote: > > + if (of_get_property(np, "linux,input-no-autorepeat", NULL)) > > + keypad_data->no_autorepeat = true; > > From 2/2: > > +Optional Properties specific to linux: > +- linux,keypad-no-autorepeat: do not enable autorepeat feature. > > I'm confused now. good catch! That happens when one tries to mimic other drivers :/ I just checked all DT input drivers for autorepeat keyword: DRIVER CODE DOCUMENTATION twl4030-keypad linux,input-no-autorepeat linux,keypad-no-autorepeat omap4-keypad linux,input-no-autorepeat linux,keypad-no-autorepeat samsung-keypad linux,input-no-autorepeat linux,keypad-no-autorepeat stmpe-keypad st,no-autorepeat st,no-autorepeat spear-keyboard autorepeat autorepeat tca8418-keypad keypad,autorepeat --- not documented --- gpio-matrix-keypad linux,no-autorepeat linux,no-autorepeat gpio-keys-polled autorepeat autorepeat gpio-keys autorepeat --- no documentation --- Any suggestions how to continue fixing this mess? I guess first of all the documentation of omap4-keypad, samsung-keypad and of course the new twl4030-keypad driver should be fixed. Next it would be nice to choose one standard property name for this and use it for twl4030-keypad. I suggest to use "linux,input-no-autorepeat". -- Sebastian
Hi! > On Mon, Nov 11, 2013 at 11:19:41PM +0100, Pavel Machek wrote: > > > + if (of_get_property(np, "linux,input-no-autorepeat", NULL)) > > > + keypad_data->no_autorepeat = true; > > > > From 2/2: > > > > +Optional Properties specific to linux: > > +- linux,keypad-no-autorepeat: do not enable autorepeat feature. > > > > I'm confused now. > > good catch! That happens when one tries to mimic other drivers :/ > > I just checked all DT input drivers for autorepeat keyword: > > DRIVER CODE DOCUMENTATION > twl4030-keypad linux,input-no-autorepeat linux,keypad-no-autorepeat > omap4-keypad linux,input-no-autorepeat linux,keypad-no-autorepeat > samsung-keypad linux,input-no-autorepeat linux,keypad-no-autorepeat > stmpe-keypad st,no-autorepeat st,no-autorepeat > spear-keyboard autorepeat autorepeat > tca8418-keypad keypad,autorepeat --- not documented --- > gpio-matrix-keypad linux,no-autorepeat linux,no-autorepeat > gpio-keys-polled autorepeat autorepeat > gpio-keys autorepeat --- no documentation --- > > Any suggestions how to continue fixing this mess? I guess first of > all the documentation of omap4-keypad, samsung-keypad and of course > the new twl4030-keypad driver should be fixed. > > Next it would be nice to choose one standard property name for this > and use it for twl4030-keypad. I suggest to use "linux,input-no-autorepeat". I'd suggest just simple "autorepeat", so that we get rid of ugly double-negation. But I guess this is not too important as long as it is consistent... And I'm not even sure consistency is huge issue. Pavel
On Sun, Nov 17, 2013 at 07:28:40PM +0100, Pavel Machek wrote: > Hi! > > > On Mon, Nov 11, 2013 at 11:19:41PM +0100, Pavel Machek wrote: > > > > + if (of_get_property(np, "linux,input-no-autorepeat", NULL)) > > > > + keypad_data->no_autorepeat = true; > > > > > > From 2/2: > > > > > > +Optional Properties specific to linux: > > > +- linux,keypad-no-autorepeat: do not enable autorepeat feature. > > > > > > I'm confused now. > > > > good catch! That happens when one tries to mimic other drivers :/ > > > > I just checked all DT input drivers for autorepeat keyword: > > > > DRIVER CODE DOCUMENTATION > > twl4030-keypad linux,input-no-autorepeat linux,keypad-no-autorepeat > > omap4-keypad linux,input-no-autorepeat linux,keypad-no-autorepeat > > samsung-keypad linux,input-no-autorepeat linux,keypad-no-autorepeat > > stmpe-keypad st,no-autorepeat st,no-autorepeat > > spear-keyboard autorepeat autorepeat > > tca8418-keypad keypad,autorepeat --- not documented --- > > gpio-matrix-keypad linux,no-autorepeat linux,no-autorepeat > > gpio-keys-polled autorepeat autorepeat > > gpio-keys autorepeat --- no documentation --- > > > > Any suggestions how to continue fixing this mess? I guess first of > > all the documentation of omap4-keypad, samsung-keypad and of course > > the new twl4030-keypad driver should be fixed. > > > > Next it would be nice to choose one standard property name for this > > and use it for twl4030-keypad. I suggest to use "linux,input-no-autorepeat". > > I'd suggest just simple "autorepeat", so that we get rid of ugly double-negation. The idea was that majority of setups want autorepeat so in the absence of the property autorepeat is turned on. Thanks.
Hi! > > > DRIVER CODE DOCUMENTATION > > > twl4030-keypad linux,input-no-autorepeat linux,keypad-no-autorepeat > > > omap4-keypad linux,input-no-autorepeat linux,keypad-no-autorepeat > > > samsung-keypad linux,input-no-autorepeat linux,keypad-no-autorepeat > > > stmpe-keypad st,no-autorepeat st,no-autorepeat > > > spear-keyboard autorepeat autorepeat > > > tca8418-keypad keypad,autorepeat --- not documented --- > > > gpio-matrix-keypad linux,no-autorepeat linux,no-autorepeat > > > gpio-keys-polled autorepeat autorepeat > > > gpio-keys autorepeat --- no documentation --- > > > > > > Any suggestions how to continue fixing this mess? I guess first of > > > all the documentation of omap4-keypad, samsung-keypad and of course > > > the new twl4030-keypad driver should be fixed. > > > > > > Next it would be nice to choose one standard property name for this > > > and use it for twl4030-keypad. I suggest to use "linux,input-no-autorepeat". > > > > I'd suggest just simple "autorepeat", so that we get rid of ugly double-negation. > The idea was that majority of setups want autorepeat so in the absence > of the property autorepeat is turned on. I see... but autorepeat on by default makes sense on devices that are usually querty keybaords, but does not make sense on devices that are usually phone keypads or power buttons. So my proposal is: 1) driver decides if it makes sense to autorepeat by default or not. 2) dts says autorepeat=0 or autorepeat=1 That way, we get dts that get chance to work on other OSes, get rid of double negations, and get right defaults when autorepeat is not specified. Pavel
Hi, > > > I'd suggest just simple "autorepeat", so that we get rid of ugly double-negation. > > The idea was that majority of setups want autorepeat so in the absence > > of the property autorepeat is turned on. > > I see... but autorepeat on by default makes sense on devices that are > usually querty keybaords, but does not make sense on devices that are > usually phone keypads or power buttons. > > So my proposal is: > > 1) driver decides if it makes sense to autorepeat by default or not. > > 2) dts says autorepeat=0 or autorepeat=1 > > That way, we get dts that get chance to work on other OSes, get rid of > double negations, and get right defaults when autorepeat is not > specified. Sounds fine to me. Any objections? -- Sebastian
diff --git a/drivers/input/keyboard/twl4030_keypad.c b/drivers/input/keyboard/twl4030_keypad.c index d2d178c..034c312 100644 --- a/drivers/input/keyboard/twl4030_keypad.c +++ b/drivers/input/keyboard/twl4030_keypad.c @@ -33,6 +33,7 @@ #include <linux/platform_device.h> #include <linux/i2c/twl.h> #include <linux/slab.h> +#include <linux/of.h> /* * The TWL4030 family chips include a keypad controller that supports @@ -60,6 +61,7 @@ struct twl4030_keypad { unsigned short keymap[TWL4030_KEYMAP_SIZE]; u16 kp_state[TWL4030_MAX_ROWS]; + bool no_autorepeat; unsigned n_rows; unsigned n_cols; unsigned irq; @@ -324,6 +326,31 @@ static int twl4030_kp_program(struct twl4030_keypad *kp) return 0; } +#if IS_ENABLED(CONFIG_OF) +static int twl4030_keypad_parse_dt(struct device *dev, + struct twl4030_keypad *keypad_data) +{ + struct device_node *np = dev->of_node; + int err; + + err = matrix_keypad_parse_of_params(dev, &keypad_data->n_rows, + &keypad_data->n_cols); + if (err) + return err; + + if (of_get_property(np, "linux,input-no-autorepeat", NULL)) + keypad_data->no_autorepeat = true; + + return 0; +} +#else +static inline int twl4030_keypad_parse_dt(struct device *dev, + struct twl4030_keypad *keypad_data) +{ + return -ENOSYS; +} +#endif + /* * Registers keypad device with input subsystem * and configures TWL4030 keypad registers @@ -331,20 +358,12 @@ static int twl4030_kp_program(struct twl4030_keypad *kp) static int twl4030_kp_probe(struct platform_device *pdev) { struct twl4030_keypad_data *pdata = pdev->dev.platform_data; - const struct matrix_keymap_data *keymap_data; + const struct matrix_keymap_data *keymap_data = NULL; struct twl4030_keypad *kp; struct input_dev *input; u8 reg; int error; - if (!pdata || !pdata->rows || !pdata->cols || !pdata->keymap_data || - pdata->rows > TWL4030_MAX_ROWS || pdata->cols > TWL4030_MAX_COLS) { - dev_err(&pdev->dev, "Invalid platform_data\n"); - return -EINVAL; - } - - keymap_data = pdata->keymap_data; - kp = kzalloc(sizeof(*kp), GFP_KERNEL); input = input_allocate_device(); if (!kp || !input) { @@ -352,13 +371,9 @@ static int twl4030_kp_probe(struct platform_device *pdev) goto err1; } - /* Get the debug Device */ - kp->dbg_dev = &pdev->dev; - kp->input = input; - - kp->n_rows = pdata->rows; - kp->n_cols = pdata->cols; - kp->irq = platform_get_irq(pdev, 0); + /* get the debug device */ + kp->dbg_dev = &pdev->dev; + kp->input = input; /* setup input device */ input->name = "TWL4030 Keypad"; @@ -370,6 +385,36 @@ static int twl4030_kp_probe(struct platform_device *pdev) input->id.product = 0x0001; input->id.version = 0x0003; + if (pdata) { + if (!pdata->rows || !pdata->cols || !pdata->keymap_data) { + dev_err(&pdev->dev, "Missing platform_data\n"); + error = -EINVAL; + goto err1; + } + + kp->n_rows = pdata->rows; + kp->n_cols = pdata->cols; + kp->no_autorepeat = !pdata->rep; + keymap_data = pdata->keymap_data; + } else { + error = twl4030_keypad_parse_dt(&pdev->dev, kp); + if (error) + goto err1; + } + + if (kp->n_rows > TWL4030_MAX_ROWS || kp->n_cols > TWL4030_MAX_COLS) { + dev_err(&pdev->dev, "Invalid rows/cols amount specified in platform/devicetree data\n"); + error = -EINVAL; + goto err1; + } + + kp->irq = platform_get_irq(pdev, 0); + if (!kp->irq) { + dev_err(&pdev->dev, "no keyboard irq assigned\n"); + error = -EINVAL; + goto err1; + } + error = matrix_keypad_build_keymap(keymap_data, NULL, TWL4030_MAX_ROWS, 1 << TWL4030_ROW_SHIFT, @@ -381,7 +426,7 @@ static int twl4030_kp_probe(struct platform_device *pdev) input_set_capability(input, EV_MSC, MSC_SCAN); /* Enable auto repeat feature of Linux input subsystem */ - if (pdata->rep) + if (!kp->no_autorepeat) __set_bit(EV_REP, input->evbit); error = input_register_device(input); @@ -443,6 +488,17 @@ static int twl4030_kp_remove(struct platform_device *pdev) return 0; } +#if IS_ENABLED(CONFIG_OF) +static const struct of_device_id twl4030_keypad_dt_match_table[] = { + { .compatible = "ti,twl4030-keypad" }, + {}, +}; +MODULE_DEVICE_TABLE(of, twl4030_keypad_dt_match_table); +#define twl4030_keypad_dt_match of_match_ptr(twl4030_keypad_dt_match_table) +#else +#define twl4030_keypad_dt_match NULL +#endif + /* * NOTE: twl4030 are multi-function devices connected via I2C. * So this device is a child of an I2C parent, thus it needs to @@ -455,6 +511,7 @@ static struct platform_driver twl4030_kp_driver = { .driver = { .name = "twl4030_keypad", .owner = THIS_MODULE, + .of_match_table = twl4030_keypad_dt_match, }, }; module_platform_driver(twl4030_kp_driver);
Add device tree support for twl4030 keypad driver. Tested on Nokia N900. Signed-off-by: Sebastian Reichel <sre@debian.org> --- drivers/input/keyboard/twl4030_keypad.c | 91 +++++++++++++++++++++++++++------ 1 file changed, 74 insertions(+), 17 deletions(-)