Re: [PATCHv2 1/3] Input: twl4030-keypad - add device tree support
On Sun, 27 Oct 2013 12:47:53 +0100, Pavel Machek pa...@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. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 1/3] Input: twl4030-keypad - add device tree support
On Wed 2013-10-30 06:53:07, Grant Likely wrote: On Sun, 27 Oct 2013 12:47:53 +0100, Pavel Machek pa...@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 -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 1/3] Input: twl4030-keypad - add device tree support
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 -- Employee of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 1/3] Input: twl4030-keypad - add device tree support
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 signature.asc Description: Digital signature
Re: [PATCHv2 1/3] Input: twl4030-keypad - add device tree support
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 s...@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 000..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;
+ };
--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by
The Linux Foundation
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 1/3] Input: twl4030-keypad - add device tree support
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 signature.asc Description: Digital signature
Re: [PATCHv2 1/3] Input: twl4030-keypad - add device tree support
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 -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 1/3] Input: twl4030-keypad - add device tree support
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 signature.asc Description: Digital signature
Re: [PATCHv2 1/3] Input: twl4030-keypad - add device tree support
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 -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 1/3] Input: twl4030-keypad - add device tree support
* Pavel Machek pa...@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 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 1/3] Input: twl4030-keypad - add device tree support
On Sun, Oct 27, 2013 at 05:23:48AM -0700, Tony Lindgren wrote: * Pavel Machek pa...@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 signature.asc Description: Digital signature
[PATCHv2 1/3] Input: twl4030-keypad - add device tree support
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 s...@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 000..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];
+ boolno_autorepeat;
unsignedn_rows;
unsignedn_cols;
unsignedirq;
@@ -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
