| Message ID | 1382446042-27099-2-git-send-email-sre@debian.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi! > Add device tree support for twl4030 keypad driver and update the > Documentation with twl4030 keypad device tree binding information. > > Tested on Nokia N900. It looks pretty good. > +++ b/Documentation/devicetree/bindings/input/twl4030-keypad.txt > @@ -0,0 +1,31 @@ > +* TWL4030's Keypad Controller device tree bindings > + > +TWL4030's Keypad controller is used to interface a SoC with a matrix-type > +keypad device. The keypad controller supports multiple row and column lines. > +A key can be placed at each intersection of a unique row and a unique column. > +The keypad controller can sense a key-press and key-release and report the > +event using a interrupt to the cpu. > + > +This binding is based on the matrix-keymap binding with the following > +changes: > + > + * keypad,num-rows and keypad,num-columns are required. Is "keypad," prefix neccessary here? > +Optional Properties specific to linux: > +- linux,keypad-no-autorepeat: do no enable autorepeat feature. "do not autorepeat". Plus I do not see what is Linux specifc about not autorepeating... Other systems will likely know about autorepeat, too. > @@ -324,6 +326,31 @@ static int twl4030_kp_program(struct twl4030_keypad *kp) > return 0; > } > > +#if IS_ENABLED(CONFIG_OF) I'm probably missing something here, but why not #ifdef CONFIG_OF? > @@ -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); > Double negation is nasty to read. I believe code would be more readable if you switched logic to kp->autorepeat. Thanks, Pavel
Hi Pavel, On Sun, Oct 27, 2013 at 12:17:15PM +0100, Pavel Machek wrote: > > Add device tree support for twl4030 keypad driver and update the > > Documentation with twl4030 keypad device tree binding information. > > > > Tested on Nokia N900. > > It looks pretty good. Thanks. > > + * keypad,num-rows and keypad,num-columns are required. > Is "keypad," prefix neccessary here? See Documentation/devicetree/bindings/input/matrix-keymap.txt > > +Optional Properties specific to linux: > > +- linux,keypad-no-autorepeat: do no enable autorepeat feature. > > "do not autorepeat". Plus I do not see what is Linux specifc about not > autorepeating... Other systems will likely know about autorepeat, too. This property has already been used like this for samsung-keypad, stmpe-keypad, omap-keypad and gpio-matrix-keypad. > > +#if IS_ENABLED(CONFIG_OF) > I'm probably missing something here, but why not #ifdef CONFIG_OF? I have been told for other drivers, that IS_ENABLED() is the prefered way to check for configuration these days. > > @@ -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); > > > > Double negation is nasty to read. I believe code would be more > readable if you switched logic to kp->autorepeat. I was thinking about the same when writing it, but decided against it, since it will just move the double negation to the variable initialization. -- Sebastian
Hi! > > > + * keypad,num-rows and keypad,num-columns are required. > > Is "keypad," prefix neccessary here? > > See Documentation/devicetree/bindings/input/matrix-keymap.txt > > > > +Optional Properties specific to linux: > > > +- linux,keypad-no-autorepeat: do no enable autorepeat feature. > > > > "do not autorepeat". Plus I do not see what is Linux specifc about not > > autorepeating... Other systems will likely know about autorepeat, too. > > This property has already been used like this for > samsung-keypad, stmpe-keypad, omap-keypad and > gpio-matrix-keypad. Ok. But you still have a typo. "do no enable" => "do not enable". > > > +#if IS_ENABLED(CONFIG_OF) > > I'm probably missing something here, but why not #ifdef CONFIG_OF? > > I have been told for other drivers, that IS_ENABLED() is > the prefered way to check for configuration these days. CONFIG_OF can not be module, using IS_ENABLED() on it is just wrong. > > > @@ -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); > > > > > > > Double negation is nasty to read. I believe code would be more > > readable if you switched logic to kp->autorepeat. > > I was thinking about the same when writing it, but decided > against it, since it will just move the double negation to > the variable initialization. Well, the property should be linux,keypad-autorepeat in the first place, but it is too late to change that. Thanks, Pavel
* Pavel Machek <pavel@ucw.cz> [131027 04:48]: > > > > > +#if IS_ENABLED(CONFIG_OF) > > > I'm probably missing something here, but why not #ifdef CONFIG_OF? > > > > I have been told for other drivers, that IS_ENABLED() is > > the prefered way to check for configuration these days. > > CONFIG_OF can not be module, using IS_ENABLED() on it is just wrong. Good point. Looks like there's IS_BUILTIN that's for boolean options. Regards, Tony
On Sun, Oct 27, 2013 at 05:23:48AM -0700, Tony Lindgren wrote: > * Pavel Machek <pavel@ucw.cz> [131027 04:48]: > > > > > +#if IS_ENABLED(CONFIG_OF) > > > > I'm probably missing something here, but why not #ifdef CONFIG_OF? > > > > > > I have been told for other drivers, that IS_ENABLED() is > > > the prefered way to check for configuration these days. > > > > CONFIG_OF can not be module, using IS_ENABLED() on it is just wrong. > > Good point. Looks like there's IS_BUILTIN that's for boolean options. Using IS_ENABLED for boolean options is supposed to be supported according to the comment above IS_BUILTIN: /* * IS_BUILTIN(CONFIG_FOO) evaluates to 1 if CONFIG_FOO is set to 'y', 0 * otherwise. For boolean options, this is equivalent to * IS_ENABLED(CONFIG_FOO). */ #define IS_BUILTIN(option) config_enabled(option) -- Sebastian
On Oct 22, 2013, at 7:47 AM, Sebastian Reichel wrote: > Add device tree support for twl4030 keypad driver and update the > Documentation with twl4030 keypad device tree binding information. > > Tested on Nokia N900. > > Signed-off-by: Sebastian Reichel <sre@debian.org> > --- > .../devicetree/bindings/input/twl4030-keypad.txt | 31 ++++++++ > drivers/input/keyboard/twl4030_keypad.c | 91 ++++++++++++++++++---- > 2 files changed, 105 insertions(+), 17 deletions(-) > create mode 100644 Documentation/devicetree/bindings/input/twl4030-keypad.txt > > diff --git a/Documentation/devicetree/bindings/input/twl4030-keypad.txt b/Documentation/devicetree/bindings/input/twl4030-keypad.txt > new file mode 100644 > index 0000000..2b4bd7a > --- /dev/null > +++ b/Documentation/devicetree/bindings/input/twl4030-keypad.txt > @@ -0,0 +1,31 @@ > +* TWL4030's Keypad Controller device tree bindings > + > +TWL4030's Keypad controller is used to interface a SoC with a matrix-type > +keypad device. The keypad controller supports multiple row and column lines. > +A key can be placed at each intersection of a unique row and a unique column. > +The keypad controller can sense a key-press and key-release and report the > +event using a interrupt to the cpu. > + > +This binding is based on the matrix-keymap binding with the following > +changes: Maybe be a bit more specific and say 'based on the input/matrix-keymap.txt binding'.. > + > + * keypad,num-rows and keypad,num-columns are required. > + Is linux,keymap required from matrix-keymap.txt? > +Required SoC Specific Properties: > +- compatible: should be one of the following > + - "ti,twl4030-keypad": For controllers compatible with twl4030 keypad > + controller. > +- interrupt: should be one of the following > + - <1>: For controllers compatible with twl4030 keypad controller. > + > +Optional Properties specific to linux: > +- linux,keypad-no-autorepeat: do no enable autorepeat feature. Does it make sense to update the matrix-keymap.txt binding to add 'linux,keypad-no-autorepeat' there? > + > +Example: > + twl_keypad: keypad { > + compatible = "ti,twl4030-keypad"; > + interrupts = <1>; > + keypad,num-rows = <8>; > + keypad,num-columns = <8>; > + linux,keypad-no-autorepeat; > + };
On Mon, Oct 28, 2013 at 01:42:47AM -0500, Kumar Gala wrote: > > +This binding is based on the matrix-keymap binding with the following > > +changes: > > Maybe be a bit more specific and say 'based on the input/matrix-keymap.txt binding'.. OK. > > + * keypad,num-rows and keypad,num-columns are required. > > Is linux,keymap required from matrix-keymap.txt? Yes, matrix-keymap.txt contains descriptions for the following: required: - linux,keymap optional: - keypad,num-rows - keypad,num-columns > > +Optional Properties specific to linux: > > +- linux,keypad-no-autorepeat: do no enable autorepeat feature. > > Does it make sense to update the matrix-keymap.txt binding to add > 'linux,keypad-no-autorepeat' there? At least according to devicetree documentation there are keymap-matrix.txt based drivers, which do not support "linux,keypad-no-autorepeat". -- Sebastian
On Oct 28, 2013, at 8:06 PM, Sebastian Reichel wrote: > On Mon, Oct 28, 2013 at 01:42:47AM -0500, Kumar Gala wrote: >>> +This binding is based on the matrix-keymap binding with the following >>> +changes: >> >> Maybe be a bit more specific and say 'based on the input/matrix-keymap.txt binding'.. > > OK. > >>> + * keypad,num-rows and keypad,num-columns are required. >> >> Is linux,keymap required from matrix-keymap.txt? > > Yes, matrix-keymap.txt contains descriptions for the following: > > required: > - linux,keymap So you don't say that linux,keymap is required for twl4030-keypad (wasn't clear if you assumed that or not). > optional: > - keypad,num-rows > - keypad,num-columns > >>> +Optional Properties specific to linux: >>> +- linux,keypad-no-autorepeat: do no enable autorepeat feature. >> >> Does it make sense to update the matrix-keymap.txt binding to add >> 'linux,keypad-no-autorepeat' there? > > At least according to devicetree documentation there are > keymap-matrix.txt based drivers, which do not support > "linux,keypad-no-autorepeat". Which is why it could be optional in keymap-matrix.txt. I dont know anything about keymap/keypad's just asking the question? It seems as if linux,keypad-no-autorepeat is intended to mean the same thing (if relevant to the device) across all drivers. Is that correct? If so it seems like moving it to be specified in a generic input binding makes sense, just not sure if keymap-matrix.txt is that place or not. - k
On Tue, Oct 29, 2013 at 03:33:31AM -0500, Kumar Gala wrote: > On Oct 28, 2013, at 8:06 PM, Sebastian Reichel wrote: > > On Mon, Oct 28, 2013 at 01:42:47AM -0500, Kumar Gala wrote: > >>> +This binding is based on the matrix-keymap binding with the following > >>> +changes: > >> > >> Maybe be a bit more specific and say 'based on the input/matrix-keymap.txt binding'.. > > > > OK. > > > >>> + * keypad,num-rows and keypad,num-columns are required. > >> > >> Is linux,keymap required from matrix-keymap.txt? > > > > Yes, matrix-keymap.txt contains descriptions for the following: > > > > required: > > - linux,keymap > > So you don't say that linux,keymap is required for twl4030-keypad > (wasn't clear if you assumed that or not). It's already required by matrix-keypad, so I assumed the requirement is inherited by twl4030-keypad. The other matrix-keymap based bindings assume the same. keypad,num-rows and keypad,num-columns are only stated explicitly, because they are optional in matrix-keymap, but required by twl4030-keympad. > > optional: > > - keypad,num-rows > > - keypad,num-columns > > > >>> +Optional Properties specific to linux: > >>> +- linux,keypad-no-autorepeat: do no enable autorepeat feature. > >> > >> Does it make sense to update the matrix-keymap.txt binding to add > >> 'linux,keypad-no-autorepeat' there? > > > > At least according to devicetree documentation there are > > keymap-matrix.txt based drivers, which do not support > > "linux,keypad-no-autorepeat". > > Which is why it could be optional in keymap-matrix.txt. I dont > know anything about keymap/keypad's just asking the question? > > It seems as if linux,keypad-no-autorepeat is intended to mean the > same thing (if relevant to the device) across all drivers. Is > that correct? If so it seems like moving it to be specified in a > generic input binding makes sense, just not sure if > keymap-matrix.txt is that place or not. Yes, when supported it means the same. I can add it to keymap-matrix.txt. -- Sebastian
On Sun, 27 Oct 2013 12:47:53 +0100, Pavel Machek <pavel@ucw.cz> wrote: > > > > +#if IS_ENABLED(CONFIG_OF) > > > I'm probably missing something here, but why not #ifdef CONFIG_OF? > > > > I have been told for other drivers, that IS_ENABLED() is > > the prefered way to check for configuration these days. > > CONFIG_OF can not be module, using IS_ENABLED() on it is just wrong. That makes no sense. There is absolutely nothing wrong with using IS_ENABLED() for CONFIG_OF. g.
On Wed 2013-10-30 06:53:07, Grant Likely wrote: > On Sun, 27 Oct 2013 12:47:53 +0100, Pavel Machek <pavel@ucw.cz> wrote: > > > > > +#if IS_ENABLED(CONFIG_OF) > > > > I'm probably missing something here, but why not #ifdef CONFIG_OF? > > > > > > I have been told for other drivers, that IS_ENABLED() is > > > the prefered way to check for configuration these days. > > > > CONFIG_OF can not be module, using IS_ENABLED() on it is just wrong. > > That makes no sense. There is absolutely nothing wrong with using > IS_ENABLED() for CONFIG_OF. Besides being too long, confusing for a reader, and testing for an option that can't exist? include/linux/kconfig.h-/* include/linux/kconfig.h: * IS_ENABLED(CONFIG_FOO) evaluates to 1 if CONFIG_FOO is set to 'y' or 'm', include/linux/kconfig.h- * 0 otherwise. include/linux/kconfig.h- * include/linux/kconfig.h- */ include/linux/kconfig.h:#define IS_ENABLED(option) \ include/linux/kconfig.h- (config_enabled(option) || config_enabled(option##_MODULE)) include/linux/kconfig.h- include/linux/kconfig.h-/* include/linux/kconfig.h- * IS_BUILTIN(CONFIG_FOO) evaluates to 1 if CONFIG_FOO is set to 'y', 0 include/linux/kconfig.h- * otherwise. For boolean options, this is equivalent to include/linux/kconfig.h: * IS_ENABLED(CONFIG_FOO). include/linux/kconfig.h- */ include/linux/kconfig.h-#define IS_BUILTIN(option) config_enabled(option) include/linux/kconfig.h- Oops. And I made a mistake of looking up config_enabled(). Obfuscated C code contest. Just use #ifdef CONFIG_foo. Pavel
diff --git a/Documentation/devicetree/bindings/input/twl4030-keypad.txt b/Documentation/devicetree/bindings/input/twl4030-keypad.txt new file mode 100644 index 0000000..2b4bd7a --- /dev/null +++ b/Documentation/devicetree/bindings/input/twl4030-keypad.txt @@ -0,0 +1,31 @@ +* TWL4030's Keypad Controller device tree bindings + +TWL4030's Keypad controller is used to interface a SoC with a matrix-type +keypad device. The keypad controller supports multiple row and column lines. +A key can be placed at each intersection of a unique row and a unique column. +The keypad controller can sense a key-press and key-release and report the +event using a interrupt to the cpu. + +This binding is based on the matrix-keymap binding with the following +changes: + + * keypad,num-rows and keypad,num-columns are required. + +Required SoC Specific Properties: +- compatible: should be one of the following + - "ti,twl4030-keypad": For controllers compatible with twl4030 keypad + controller. +- interrupt: should be one of the following + - <1>: For controllers compatible with twl4030 keypad controller. + +Optional Properties specific to linux: +- linux,keypad-no-autorepeat: do no enable autorepeat feature. + +Example: + twl_keypad: keypad { + compatible = "ti,twl4030-keypad"; + interrupts = <1>; + keypad,num-rows = <8>; + keypad,num-columns = <8>; + linux,keypad-no-autorepeat; + }; 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 and update the Documentation with twl4030 keypad device tree binding information. Tested on Nokia N900. Signed-off-by: Sebastian Reichel <sre@debian.org> --- .../devicetree/bindings/input/twl4030-keypad.txt | 31 ++++++++ drivers/input/keyboard/twl4030_keypad.c | 91 ++++++++++++++++++---- 2 files changed, 105 insertions(+), 17 deletions(-) create mode 100644 Documentation/devicetree/bindings/input/twl4030-keypad.txt