Add device tree support for the Nokia N900 keyboard.
Changes since v1:
* create "DTS: ARM: TWL4030: Add keypad node" patch, which
was part of "Input: twl4030-keypad - add device tree support"
before.
* use "IS_ENABLED(CONFIG_OF)" to check if DT is enabled
Currently non-DT boot crashes for me very early, so I was not able
to check this patch using non-DT boot.
-- Sebastian
Sebastian Reichel (3):
Input: twl4030-keypad - add device tree support
DTS: ARM: TWL4030: Add keypad node
ARM: dts: N900: TWL4030 Keypad Matrix definition
.../devicetree/bindings/input/twl4030-keypad.txt | 31 ++++++++
arch/arm/boot/dts/omap3-n900.dts | 55 +++++++++++++
arch/arm/boot/dts/twl4030.dtsi | 7 ++
drivers/input/keyboard/twl4030_keypad.c | 91 ++++++++++++++++++----
4 files changed, 167 insertions(+), 17 deletions(-)
create mode 100644 Documentation/devicetree/bindings/input/twl4030-keypad.txt
--
1.8.4.rc3
Add Keyboard Matrix information to N900's DTS file.
This patch maps the keys exactly as the original
board code.
Signed-off-by: Sebastian Reichel <[email protected]>
---
arch/arm/boot/dts/omap3-n900.dts | 55 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 55 insertions(+)
diff --git a/arch/arm/boot/dts/omap3-n900.dts b/arch/arm/boot/dts/omap3-n900.dts
index c2b92e8..30ac779 100644
--- a/arch/arm/boot/dts/omap3-n900.dts
+++ b/arch/arm/boot/dts/omap3-n900.dts
@@ -109,6 +109,61 @@
#include "twl4030.dtsi"
#include "twl4030_omap3.dtsi"
+&twl_keypad {
+ linux,keymap = < 0x00000010 /* KEY_Q */
+ 0x00010018 /* KEY_O */
+ 0x00020019 /* KEY_P */
+ 0x00030033 /* KEY_COMMA */
+ 0x0004000e /* KEY_BACKSPACE */
+ 0x0006001e /* KEY_A */
+ 0x0007001f /* KEY_S */
+
+ 0x01000011 /* KEY_W */
+ 0x01010020 /* KEY_D */
+ 0x01020021 /* KEY_F */
+ 0x01030022 /* KEY_G */
+ 0x01040023 /* KEY_H */
+ 0x01050024 /* KEY_J */
+ 0x01060025 /* KEY_K */
+ 0x01070026 /* KEY_L */
+
+ 0x02000012 /* KEY_E */
+ 0x02010034 /* KEY_DOT */
+ 0x02020067 /* KEY_UP */
+ 0x0203001c /* KEY_ENTER */
+ 0x0205002c /* KEY_Z */
+ 0x0206002d /* KEY_X */
+ 0x0207002e /* KEY_C */
+ 0x02080043 /* KEY_F9 */
+
+ 0x03000013 /* KEY_R */
+ 0x0301002f /* KEY_V */
+ 0x03020030 /* KEY_B */
+ 0x03030031 /* KEY_N */
+ 0x03040032 /* KEY_M */
+ 0x03050039 /* KEY_SPACE */
+ 0x03060039 /* KEY_SPACE */
+ 0x03070069 /* KEY_LEFT */
+
+ 0x04000014 /* KEY_T */
+ 0x0401006c /* KEY_DOWN */
+ 0x0402006a /* KEY_RIGHT */
+ 0x0404001d /* KEY_LEFTCTRL */
+ 0x04050064 /* KEY_RIGHTALT */
+ 0x0406002a /* KEY_LEFTSHIFT */
+ 0x04080044 /* KEY_F10 */
+
+ 0x05000015 /* KEY_Y */
+ 0x05080057 /* KEY_F11 */
+
+ 0x06000016 /* KEY_U */
+
+ 0x07000017 /* KEY_I */
+ 0x07010041 /* KEY_F7 */
+ 0x07020042 /* KEY_F8 */
+ >;
+};
+
&twl_gpio {
ti,pullups = <0x0>;
ti,pulldowns = <0x03ff3f>; /* BIT(0..5) | BIT(8..17) */
--
1.8.4.rc3
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 <[email protected]>
---
.../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:
+
+ * 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);
--
1.8.4.rc3
Add keypad node to twl4030, so that board DTS
files can just add the keymap.
Signed-off-by: Sebastian Reichel <[email protected]>
---
arch/arm/boot/dts/twl4030.dtsi | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/arch/arm/boot/dts/twl4030.dtsi b/arch/arm/boot/dts/twl4030.dtsi
index ae6a17a..773c94c 100644
--- a/arch/arm/boot/dts/twl4030.dtsi
+++ b/arch/arm/boot/dts/twl4030.dtsi
@@ -97,4 +97,11 @@
compatible = "ti,twl4030-pwmled";
#pwm-cells = <2>;
};
+
+ twl_keypad: keypad {
+ compatible = "ti,twl4030-keypad";
+ interrupts = <1>;
+ keypad,num-rows = <8>;
+ keypad,num-columns = <8>;
+ };
};
--
1.8.4.rc3
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
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
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
* Pavel Machek <[email protected]> [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 <[email protected]> [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 <[email protected]>
> ---
> .../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;
> + };
--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
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
--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
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 <[email protected]> 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 <[email protected]> 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