[PATCH 4/5 v2] input: tc3589x-keypad: support probing from device tree

[PATCH 4/5 v2] input: tc3589x-keypad: support probing from device tree

[Linus Walleij]

Implement device tree probing for the tc3589x keypad driver.
This is modeled on the STMPE keypad driver and tested on the
Ux500 TVK1281618 UIB.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v2:
- Fix rows/columns binding to read two u32's insead of two
  u8 /bits/ as noted by Mark Rutland.
---
 drivers/input/keyboard/tc3589x-keypad.c | 63 +++++++++++++++++++++++++++++++--
 1 file changed, 61 insertions(+), 2 deletions(-)

diff --git a/drivers/input/keyboard/tc3589x-keypad.c b/drivers/input/keyboard/tc3589x-keypad.c
index 208de7cbb7fa..7b6637d8cfbc 100644
--- a/drivers/input/keyboard/tc3589x-keypad.c
+++ b/drivers/input/keyboard/tc3589x-keypad.c
@@ -297,6 +297,62 @@ static void tc3589x_keypad_close(struct input_dev *input)
 	tc3589x_keypad_disable(keypad);
 }
 
+#ifdef CONFIG_OF
+static const struct tc3589x_keypad_platform_data *
+tc3589x_keypad_of_probe(struct device *dev)
+{
+	struct device_node *np = dev->of_node;
+	struct tc3589x_keypad_platform_data *plat;
+	u32 debounce_ms;
+	int proplen;
+
+	if (!np)
+		return ERR_PTR(-ENODEV);
+
+	plat = devm_kzalloc(dev, sizeof(*plat), GFP_KERNEL);
+	if (!plat)
+		return ERR_PTR(-ENOMEM);
+
+	of_property_read_u32(np, "keypad,num-columns", &plat->kcol);
+	of_property_read_u32(np, "keypad,num-rows", &plat->krow);
+	if (!plat->krow || !plat->kcol ||
+	     plat->krow > TC_KPD_ROWS || plat->kcol > TC_KPD_COLUMNS) {
+		dev_err(dev,
+			"keypad columns/rows not properly specified (%ux%u)\n",
+			plat->kcol, plat->krow);
+		return ERR_PTR(-EINVAL);
+	}
+
+	if (!of_get_property(np, "linux,keymap", &proplen)) {
+		dev_err(dev, "property linux,keymap not found\n");
+		return ERR_PTR(-ENOENT);
+	}
+
+	plat->no_autorepeat = of_property_read_bool(np, "linux,no-autorepeat");
+	plat->enable_wakeup = of_property_read_bool(np, "linux,wakeup");
+
+	/* The custom delay format is ms/16 */
+	of_property_read_u32(np, "debounce-delay-ms", &debounce_ms);
+	if (debounce_ms)
+		plat->debounce_period = debounce_ms * 16;
+	else
+		plat->debounce_period = TC_KPD_DEBOUNCE_PERIOD;
+
+	plat->settle_time = TC_KPD_SETTLE_TIME;
+	/* FIXME: should be property of the IRQ resource? */
+	plat->irqtype = IRQF_TRIGGER_FALLING;
+
+	return plat;
+}
+#else
+static inline const struct tc3589x_keypad_platform_data *
+tc3589x_keypad_of_probe(struct device *dev)
+{
+	return ERR_PTR(-ENODEV);
+}
+#endif
+
+
 static int tc3589x_keypad_probe(struct platform_device *pdev)
 {
 	struct tc3589x *tc3589x = dev_get_drvdata(pdev->dev.parent);
@@ -307,8 +363,11 @@ static int tc3589x_keypad_probe(struct platform_device *pdev)
 
 	plat = tc3589x->pdata->keypad;
 	if (!plat) {
-		dev_err(&pdev->dev, "invalid keypad platform data\n");
-		return -EINVAL;
+		plat = tc3589x_keypad_of_probe(&pdev->dev);
+		if (IS_ERR(plat)) {
+			dev_err(&pdev->dev, "invalid keypad platform data\n");
+			return PTR_ERR(plat);
+		}
 	}
 
 	irq = platform_get_irq(pdev, 0);
-- 
1.8.3.1

Re: [PATCH 4/5 v2] input: tc3589x-keypad: support probing from device tree

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 342 bytes --]

Hi,

On Tue, Nov 12, 2013 at 03:13:38PM +0100, Linus Walleij wrote:
> +	plat->no_autorepeat = of_property_read_bool(np, "linux,no-autorepeat");
> +	plat->enable_wakeup = of_property_read_bool(np, "linux,wakeup");

There is currently discussion going on for the property name of
autorepeat:

https://lkml.org/lkml/2013/11/11/680

-- Sebastian

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 4/5 v2] input: tc3589x-keypad: support probing from device tree

[Linus Walleij]

On Tue, Nov 12, 2013 at 4:30 PM, Sebastian Reichel <sre@ring0.de> wrote:
> On Tue, Nov 12, 2013 at 03:13:38PM +0100, Linus Walleij wrote:

>> +     plat->no_autorepeat = of_property_read_bool(np, "linux,no-autorepeat");
>> +     plat->enable_wakeup = of_property_read_bool(np, "linux,wakeup");
>
> There is currently discussion going on for the property name of
> autorepeat:
>
> https://lkml.org/lkml/2013/11/11/680

So this binding is documented for GPIO keys in:
Documentation/devicetree/bindings/input/gpio-matrix-keypad.txt

This is probably the most used binding as GPIO matrixes are
not uncommon. But I don't know, yes it is a mess. (As usual.)

Yours,
Linus Walleij

Re: [PATCH 4/5 v2] input: tc3589x-keypad: support probing from device tree

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1799 bytes --]

On Tue, Nov 12, 2013 at 06:06:49PM +0100, Linus Walleij wrote:
> On Tue, Nov 12, 2013 at 4:30 PM, Sebastian Reichel <sre@ring0.de> wrote:
> > On Tue, Nov 12, 2013 at 03:13:38PM +0100, Linus Walleij wrote:
> 
> >> +     plat->no_autorepeat = of_property_read_bool(np, "linux,no-autorepeat");
> >> +     plat->enable_wakeup = of_property_read_bool(np, "linux,wakeup");
> >
> > There is currently discussion going on for the property name of
> > autorepeat:
> >
> > https://lkml.org/lkml/2013/11/11/680
> 
> So this binding is documented for GPIO keys in:
> Documentation/devicetree/bindings/input/gpio-matrix-keypad.txt
> 
> This is probably the most used binding as GPIO matrixes are
> not uncommon. But I don't know, yes it is a mess. (As usual.)

I could find two boards using "gpio-matrix-keypad" in the mainline
kernel and not a single instance of "linux,no-autorepeat":

arch $ grep "gpio-matrix-keypad" **/*.dts **/*.dtsi
arm/boot/dts/am335x-evm.dts:               compatible = "gpio-matrix-keypad";
powerpc/boot/dts/ac14xx.dts:               compatible = "gpio-matrix-keypad";

arch $ grep "autorepeat" **/*.dts **/*.dtsi
arch/arm/boot/dts/am335x-evm.dts:          autorepeat;
arch/arm/boot/dts/ea3250.dts:              autorepeat;
arch/arm/boot/dts/exynos4210-smdkv310.dts: linux,keypad-no-autorepeat;
arch/arm/boot/dts/exynos4412-origen.dts:   linux,keypad-no-autorepeat;
arch/arm/boot/dts/exynos4412-smdk4412.dts: linux,keypad-no-autorepeat;
arch/arm/boot/dts/omap4-sdp.dts:           linux,input-no-autorepeat;
arch/arm/boot/dts/spear1310-evb.dts:       autorepeat;
arch/arm/boot/dts/spear1340-evb.dts:       autorepeat;
arch/arm/boot/dts/spear300-evb.dts:        autorepeat;
arch/arm/boot/dts/ste-stuib.dtsi:          st,no-autorepeat;

-- Sebastian

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 4/5 v2] input: tc3589x-keypad: support probing from device tree

[Linus Walleij]

On Tue, Nov 12, 2013 at 9:05 PM, Sebastian Reichel <sre@ring0.de> wrote:
> On Tue, Nov 12, 2013 at 06:06:49PM +0100, Linus Walleij wrote:

>> So this binding is documented for GPIO keys in:
>> Documentation/devicetree/bindings/input/gpio-matrix-keypad.txt
>>
>> This is probably the most used binding as GPIO matrixes are
>> not uncommon. But I don't know, yes it is a mess. (As usual.)
>
> I could find two boards using "gpio-matrix-keypad" in the mainline
> kernel and not a single instance of "linux,no-autorepeat":

In things connected to GPIO, I don't expect the in-kernel
device trees to be a good way so survey the usage of these
bindings. Anyone doing device trees on any system with a
few GPIOs can be using this.

But maybe we're lucky and won't break anyone's setup if
we change this?

Yours,
Linus Walleij

Re: [PATCH 4/5 v2] input: tc3589x-keypad: support probing from device tree

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1373 bytes --]

On Tue, Nov 12, 2013 at 09:11:40PM +0100, Linus Walleij wrote:
> > I could find two boards using "gpio-matrix-keypad" in the mainline
> > kernel and not a single instance of "linux,no-autorepeat":
> 
> In things connected to GPIO, I don't expect the in-kernel
> device trees to be a good way so survey the usage of these
> bindings. Anyone doing device trees on any system with a
> few GPIOs can be using this.
> 
> But maybe we're lucky and won't break anyone's setup if
> we change this?

AFAIK Device Tree property names are considered as ABI, so existing
property names must not be removed.

But I guess we can add the standardized property name in addition
to the deprecated one. New drivers can use the standardized property
name from the beginning.

Thus I guess we should not use the name, which has the most adopters
in kernel (or out of kernel). Instead the most fitting name should
be used. Current suggestions (taken from kernel) are:

* <<vendor>>,no-autorepeat
* keypad,autorepeat
* linux,keypad-no-autorepeat
* linux,input-no-autorepeat
* linux,no-autorepeat
* autorepeat

I do not really care, which one is chosen, except for two things:

* <<vendor>> seems wrong. This is not vendor specific.
* I would prefer "input-" over "keypad-", since then the same name
  can be used for single keys, buttons, etc.

-- Sebastian

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 4/5 v2] input: tc3589x-keypad: support probing from device tree

[Mark Rutland]

On Tue, Nov 12, 2013 at 08:40:14PM +0000, Sebastian Reichel wrote:
> On Tue, Nov 12, 2013 at 09:11:40PM +0100, Linus Walleij wrote:
> > > I could find two boards using "gpio-matrix-keypad" in the mainline
> > > kernel and not a single instance of "linux,no-autorepeat":
> > 
> > In things connected to GPIO, I don't expect the in-kernel
> > device trees to be a good way so survey the usage of these
> > bindings. Anyone doing device trees on any system with a
> > few GPIOs can be using this.
> > 
> > But maybe we're lucky and won't break anyone's setup if
> > we change this?
> 
> AFAIK Device Tree property names are considered as ABI, so existing
> property names must not be removed.

To an extent. We shouldn't pointlessly break existing bindings, and we
can typically support newer / "better" bindings without breaking support
for existing bindings.

There may be cases where we have to remove support for bindings (i.e.
when a binding is so badly used in practice that relying on it means
we're likely to damage hardware, or where it's so ambiguous that it
provides no useful information), but I don't think we've hit any of
those yet.

If a portions of a binding are no longer used in practice, then I see no
problem in removing support from them. However I'd expect them to remain
documented as deprecated / unsupported by Linux, and for those names to
not be re-used.

> 
> But I guess we can add the standardized property name in addition
> to the deprecated one. New drivers can use the standardized property
> name from the beginning.

This is precisely how I'd expect bindings to evolve. New properties can
be added, or existing properties can be extended, as long as those
modifications don't impair existing device trees which are in use.

If we need to change things substantially, then we can allocate a new
compatible string for the new binding. That doesn't seem to be necessary
here.

> 
> Thus I guess we should not use the name, which has the most adopters
> in kernel (or out of kernel). Instead the most fitting name should
> be used. Current suggestions (taken from kernel) are:
> 
> * <<vendor>>,no-autorepeat
> * keypad,autorepeat
> * linux,keypad-no-autorepeat
> * linux,input-no-autorepeat
> * linux,no-autorepeat
> * autorepeat
> 
> I do not really care, which one is chosen, except for two things:
> 
> * <<vendor>> seems wrong. This is not vendor specific.
> * I would prefer "input-" over "keypad-", since then the same name
>   can be used for single keys, buttons, etc.

Both of those sound valid to me, but I think it may make sense to keep
the "linux," prefix. As I understand it this is really telling the Linux
input subsystem to react to a device acting in a certain way, rather
than describing or configuring the device in a certain way.

Thanks,
Mark.

Re: [PATCH 4/5 v2] input: tc3589x-keypad: support probing from device tree

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 991 bytes --]

Hi,

On Wed, Nov 13, 2013 at 02:24:06PM +0000, Mark Rutland wrote:
> > Current suggestions (taken from kernel) are:
> > 
> > * <<vendor>>,no-autorepeat
> > * keypad,autorepeat
> > * linux,keypad-no-autorepeat
> > * linux,input-no-autorepeat
> > * linux,no-autorepeat
> > * autorepeat
> > 
> > I do not really care, which one is chosen, except for two things:
> > 
> > * <<vendor>> seems wrong. This is not vendor specific.
> > * I would prefer "input-" over "keypad-", since then the same name
> >   can be used for single keys, buttons, etc.
> 
> Both of those sound valid to me, but I think it may make sense to keep
> the "linux," prefix. As I understand it this is really telling the Linux
> input subsystem to react to a device acting in a certain way, rather
> than describing or configuring the device in a certain way.

That leaves the following two options:

* linux,input-no-autorepeat
* linux,no-autorepeat

Any suggestions for those?

-- Sebastian

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 4/5 v2] input: tc3589x-keypad: support probing from device tree

[Mark Rutland]

On Wed, Nov 13, 2013 at 11:29:24PM +0000, Sebastian Reichel wrote:
> Hi,
> 
> On Wed, Nov 13, 2013 at 02:24:06PM +0000, Mark Rutland wrote:
> > > Current suggestions (taken from kernel) are:
> > > 
> > > * <<vendor>>,no-autorepeat
> > > * keypad,autorepeat
> > > * linux,keypad-no-autorepeat
> > > * linux,input-no-autorepeat
> > > * linux,no-autorepeat
> > > * autorepeat
> > > 
> > > I do not really care, which one is chosen, except for two things:
> > > 
> > > * <<vendor>> seems wrong. This is not vendor specific.
> > > * I would prefer "input-" over "keypad-", since then the same name
> > >   can be used for single keys, buttons, etc.
> > 
> > Both of those sound valid to me, but I think it may make sense to keep
> > the "linux," prefix. As I understand it this is really telling the Linux
> > input subsystem to react to a device acting in a certain way, rather
> > than describing or configuring the device in a certain way.
> 
> That leaves the following two options:
> 
> * linux,input-no-autorepeat
> * linux,no-autorepeat
> 
> Any suggestions for those?

I'd go for "linux,input-no-autorepeat", it leaves less ambiguity as to
its purpose.

Mark.

Re: [PATCH 4/5 v2] input: tc3589x-keypad: support probing from device tree

[Pavel Machek]

Hi!

> > > I could find two boards using "gpio-matrix-keypad" in the mainline
> > > kernel and not a single instance of "linux,no-autorepeat":
> > 
> > In things connected to GPIO, I don't expect the in-kernel
> > device trees to be a good way so survey the usage of these
> > bindings. Anyone doing device trees on any system with a
> > few GPIOs can be using this.
> > 
> > But maybe we're lucky and won't break anyone's setup if
> > we change this?
> 
> AFAIK Device Tree property names are considered as ABI, so existing
> property names must not be removed.
> 
> But I guess we can add the standardized property name in addition
> to the deprecated one. New drivers can use the standardized property
> name from the beginning.
> 
> Thus I guess we should not use the name, which has the most adopters
> in kernel (or out of kernel). Instead the most fitting name should
> be used. Current suggestions (taken from kernel) are:
> 
> * <<vendor>>,no-autorepeat
> * keypad,autorepeat
> * linux,keypad-no-autorepeat
> * linux,input-no-autorepeat
> * linux,no-autorepeat
> * autorepeat
> 
> I do not really care, which one is chosen, except for two things:
> 
> * <<vendor>> seems wrong. This is not vendor specific.
> * I would prefer "input-" over "keypad-", since then the same name
>   can be used for single keys, buttons, etc.

Hmm, and it is not Linux-specific, either. So can we stick with simple "autorepeat"?


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 4/5 v2] input: tc3589x-keypad: support probing from device tree

[Pavel Machek]

> > Thus I guess we should not use the name, which has the most adopters
> > in kernel (or out of kernel). Instead the most fitting name should
> > be used. Current suggestions (taken from kernel) are:
> > 
> > * <<vendor>>,no-autorepeat
> > * keypad,autorepeat
> > * linux,keypad-no-autorepeat
> > * linux,input-no-autorepeat
> > * linux,no-autorepeat
> > * autorepeat
> > 
> > I do not really care, which one is chosen, except for two things:
> > 
> > * <<vendor>> seems wrong. This is not vendor specific.
> > * I would prefer "input-" over "keypad-", since then the same name
> >   can be used for single keys, buttons, etc.
> 
> Both of those sound valid to me, but I think it may make sense to keep
> the "linux," prefix. As I understand it this is really telling the Linux
> input subsystem to react to a device acting in a certain way, rather
> than describing or configuring the device in a certain way.

I'd say it is very much configuring device in certain way, and yes, other
operating systems will want to do autorepeat, too.

I believe we don't want to end up with

linux,input-no-autorepeat
bsd,keypad-autorepeat
windows-phone,disable-autorepeat

...and should avoid linux, prefix.
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 4/5 v2] input: tc3589x-keypad: support probing from device tree

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1742 bytes --]

On Sun, Nov 17, 2013 at 07:28:45PM +0100, Pavel Machek wrote:
> Hi!
> 
> > > > I could find two boards using "gpio-matrix-keypad" in the mainline
> > > > kernel and not a single instance of "linux,no-autorepeat":
> > > 
> > > In things connected to GPIO, I don't expect the in-kernel
> > > device trees to be a good way so survey the usage of these
> > > bindings. Anyone doing device trees on any system with a
> > > few GPIOs can be using this.
> > > 
> > > But maybe we're lucky and won't break anyone's setup if
> > > we change this?
> > 
> > AFAIK Device Tree property names are considered as ABI, so existing
> > property names must not be removed.
> > 
> > But I guess we can add the standardized property name in addition
> > to the deprecated one. New drivers can use the standardized property
> > name from the beginning.
> > 
> > Thus I guess we should not use the name, which has the most adopters
> > in kernel (or out of kernel). Instead the most fitting name should
> > be used. Current suggestions (taken from kernel) are:
> > 
> > * <<vendor>>,no-autorepeat
> > * keypad,autorepeat
> > * linux,keypad-no-autorepeat
> > * linux,input-no-autorepeat
> > * linux,no-autorepeat
> > * autorepeat
> > 
> > I do not really care, which one is chosen, except for two things:
> > 
> > * <<vendor>> seems wrong. This is not vendor specific.
> > * I would prefer "input-" over "keypad-", since then the same name
> >   can be used for single keys, buttons, etc.
> 
> Hmm, and it is not Linux-specific, either. So can we stick with simple "autorepeat"?

The advantage of the negated form is, that autorepeat is enabled by
default. So what do you think about

input-no-autorepeat

-- Sebastian

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 4/5 v2] input: tc3589x-keypad: support probing from device tree

[Mark Rutland]

On Sun, Nov 17, 2013 at 06:28:52PM +0000, Pavel Machek wrote:
> 
> > > Thus I guess we should not use the name, which has the most adopters
> > > in kernel (or out of kernel). Instead the most fitting name should
> > > be used. Current suggestions (taken from kernel) are:
> > > 
> > > * <<vendor>>,no-autorepeat
> > > * keypad,autorepeat
> > > * linux,keypad-no-autorepeat
> > > * linux,input-no-autorepeat
> > > * linux,no-autorepeat
> > > * autorepeat
> > > 
> > > I do not really care, which one is chosen, except for two things:
> > > 
> > > * <<vendor>> seems wrong. This is not vendor specific.
> > > * I would prefer "input-" over "keypad-", since then the same name
> > >   can be used for single keys, buttons, etc.
> > 
> > Both of those sound valid to me, but I think it may make sense to keep
> > the "linux," prefix. As I understand it this is really telling the Linux
> > input subsystem to react to a device acting in a certain way, rather
> > than describing or configuring the device in a certain way.
> 
> I'd say it is very much configuring device in certain way, and yes, other
> operating systems will want to do autorepeat, too.

Nothing is handled differently at the device with respect to this flag.
The Linux input subsystem behaves differently. Thus this is
configuration of the Linux input subsysytem, not the device.

> 
> I believe we don't want to end up with
> 
> linux,input-no-autorepeat
> bsd,keypad-autorepeat
> windows-phone,disable-autorepeat

I do not see a problem with this. This is only as bad as the current
situation, but has the benefit that the madness is constrained to
particular vendor prefixes, which we can uniquely identify and handle
differently if required.

> 
> ...and should avoid linux, prefix.

I disagree.

Mark.

Re: [PATCH 4/5 v2] input: tc3589x-keypad: support probing from device tree

[Pavel Machek]

On Mon 2013-12-02 11:37:20, Mark Rutland wrote:
> On Sun, Nov 17, 2013 at 06:28:52PM +0000, Pavel Machek wrote:
> > 
> > > > Thus I guess we should not use the name, which has the most adopters
> > > > in kernel (or out of kernel). Instead the most fitting name should
> > > > be used. Current suggestions (taken from kernel) are:
> > > > 
> > > > * <<vendor>>,no-autorepeat
> > > > * keypad,autorepeat
> > > > * linux,keypad-no-autorepeat
> > > > * linux,input-no-autorepeat
> > > > * linux,no-autorepeat
> > > > * autorepeat
> > > > 
> > > > I do not really care, which one is chosen, except for two things:
> > > > 
> > > > * <<vendor>> seems wrong. This is not vendor specific.
> > > > * I would prefer "input-" over "keypad-", since then the same name
> > > >   can be used for single keys, buttons, etc.
> > > 
> > > Both of those sound valid to me, but I think it may make sense to keep
> > > the "linux," prefix. As I understand it this is really telling the Linux
> > > input subsystem to react to a device acting in a certain way, rather
> > > than describing or configuring the device in a certain way.
> > 
> > I'd say it is very much configuring device in certain way, and yes, other
> > operating systems will want to do autorepeat, too.
> 
> Nothing is handled differently at the device with respect to this flag.
> The Linux input subsystem behaves differently. Thus this is
> configuration of the Linux input subsysytem, not the device.

This flag says "this is device where autorepeat makes sense". It does
make sense for qwerty keyboard, it does not make sense for power
button. There's nothing Linux specific here.

> > I believe we don't want to end up with
> > 
> > linux,input-no-autorepeat
> > bsd,keypad-autorepeat
> > windows-phone,disable-autorepeat
> 
> I do not see a problem with this. This is only as bad as the current

Not a problem? DTS bloat? Code bloat for the drivers? (Because that
way, we'll need to handle "windows-phone,disable-autorepeat" DTS for
compatibility one day).

> situation, but has the benefit that the madness is constrained to
> particular vendor prefixes, which we can uniquely identify and handle
> differently if required.

Madness? We want to avoid madness and improve current situation where
different driver use different attributes.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 4/5 v2] input: tc3589x-keypad: support probing from device tree

[Mark Rutland]

On Mon, Dec 02, 2013 at 11:54:42AM +0000, Pavel Machek wrote:
> On Mon 2013-12-02 11:37:20, Mark Rutland wrote:
> > On Sun, Nov 17, 2013 at 06:28:52PM +0000, Pavel Machek wrote:
> > > 
> > > > > Thus I guess we should not use the name, which has the most adopters
> > > > > in kernel (or out of kernel). Instead the most fitting name should
> > > > > be used. Current suggestions (taken from kernel) are:
> > > > > 
> > > > > * <<vendor>>,no-autorepeat
> > > > > * keypad,autorepeat
> > > > > * linux,keypad-no-autorepeat
> > > > > * linux,input-no-autorepeat
> > > > > * linux,no-autorepeat
> > > > > * autorepeat
> > > > > 
> > > > > I do not really care, which one is chosen, except for two things:
> > > > > 
> > > > > * <<vendor>> seems wrong. This is not vendor specific.
> > > > > * I would prefer "input-" over "keypad-", since then the same name
> > > > >   can be used for single keys, buttons, etc.
> > > > 
> > > > Both of those sound valid to me, but I think it may make sense to keep
> > > > the "linux," prefix. As I understand it this is really telling the Linux
> > > > input subsystem to react to a device acting in a certain way, rather
> > > > than describing or configuring the device in a certain way.
> > > 
> > > I'd say it is very much configuring device in certain way, and yes, other
> > > operating systems will want to do autorepeat, too.
> > 
> > Nothing is handled differently at the device with respect to this flag.
> > The Linux input subsystem behaves differently. Thus this is
> > configuration of the Linux input subsysytem, not the device.
> 
> This flag says "this is device where autorepeat makes sense". It does
> make sense for qwerty keyboard, it does not make sense for power
> button. There's nothing Linux specific here.

If you follow that argument, you don't need the property at all. We know
whether something is being handled as a keyboard or a power button, from
the rest of the data in the DT and how the driver is handling the
device. From that you can figure out if it's sensible to handle the
device as autorepeat or not.

> 
> > > I believe we don't want to end up with
> > > 
> > > linux,input-no-autorepeat
> > > bsd,keypad-autorepeat
> > > windows-phone,disable-autorepeat
> > 
> > I do not see a problem with this. This is only as bad as the current
> 
> Not a problem? DTS bloat? Code bloat for the drivers? (Because that
> way, we'll need to handle "windows-phone,disable-autorepeat" DTS for
> compatibility one day).

I would be extremely surprised if we ever discovered
"windows-phone,disable-autorepeat" in a DT ;)

I believe that namespacing the property is sensible for the moment, as
it currently is Linux specific. It also doesn't erroneously imply that
this is a hardware property. If another OS happen to pick it up, then
living with a "linux," prefix isn't that horrendous.

> 
> > situation, but has the benefit that the madness is constrained to
> > particular vendor prefixes, which we can uniquely identify and handle
> > differently if required.
> 
> Madness? We want to avoid madness and improve current situation where
> different driver use different attributes.

Then why not have a way of figuring this out automatically, and get rid
of the madness of having this in the DT at all? It's not a property of
the device, and it doesn't even make sense as a piece of static
configuration -- what if I prefer my keyboard to not autorepeat, but my
board vendor has decided autorepeat should be on?

Thanks,
Mark.

Re: [PATCH 4/5 v2] input: tc3589x-keypad: support probing from device tree

[Pavel Machek]

Hi!

> > This flag says "this is device where autorepeat makes sense". It does
> > make sense for qwerty keyboard, it does not make sense for power
> > button. There's nothing Linux specific here.
> 
> If you follow that argument, you don't need the property at all. We know
> whether something is being handled as a keyboard or a power button, from
> the rest of the data in the DT and how the driver is handling the
> device. From that you can figure out if it's sensible to handle the
> device as autorepeat or not.

gpio-button can be used for both power button and arrows... So yes,
driver could do the decision, but you 

Yes, dt "autorepeat" attribute is there to set up the defaults, so
that the device does not have to look at the list of keycodes.

(And it is still not linux specific).

> > > > I believe we don't want to end up with
> > > > 
> > > > linux,input-no-autorepeat
> > > > bsd,keypad-autorepeat
> > > > windows-phone,disable-autorepeat
> > > 
> > > I do not see a problem with this. This is only as bad as the current
> > 
> > Not a problem? DTS bloat? Code bloat for the drivers? (Because that
> > way, we'll need to handle "windows-phone,disable-autorepeat" DTS for
> > compatibility one day).
> 
> I would be extremely surprised if we ever discovered
> "windows-phone,disable-autorepeat" in a DT ;)

If you pretend that other operating systems do not exist, why bother
with "linux," prefix at all...

> I believe that namespacing the property is sensible for the moment, as
> it currently is Linux specific. It also doesn't erroneously imply that
> this is a hardware property. If another OS happen to pick it up, then
> living with a "linux," prefix isn't that horrendous.

I think that would be pretty bad. Also we would be expected to handle
"bsd,keyboard-autorepeat" and
"windows-phone,please-do-not-autorepeat".

And there's nothing really wrong with current "autorepeat" solution we
are using, is it?

> > > situation, but has the benefit that the madness is constrained to
> > > particular vendor prefixes, which we can uniquely identify and handle
> > > differently if required.
> > 
> > Madness? We want to avoid madness and improve current situation where
> > different driver use different attributes.
> 
> Then why not have a way of figuring this out automatically, and get rid
> of the madness of having this in the DT at all? 

We already have few drivers that use DT to do this, using different
variants of "[linux,][no]autorepeat". Yes, we could move the
complexity from DT to drivers, but that does not seem like a win.

> It's not a property of
> the device, and it doesn't even make sense as a piece of static
> configuration -- what if I prefer my keyboard to not autorepeat, but my
> board vendor has decided autorepeat should be on?

Input already has controls you can use.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html