* Re: [PATCHv6 1/3] Input: twl4030-pwrbutton - add device tree support
From: Sebastian Reichel @ 2013-10-25 14:44 UTC (permalink / raw)
To: Peter Ujfalusi
Cc: Dmitry Torokhov, Grant Likely, Rob Herring, Sachin Kamat,
Florian Vaussard, linux-input, linux-kernel, devicetree
In-Reply-To: <526A680A.8090308@ti.com>
[-- Attachment #1: Type: text/plain, Size: 1058 bytes --]
Hi Péter,
On Fri, Oct 25, 2013 at 03:46:02PM +0300, Peter Ujfalusi wrote:
> > [...]
> > +#if IS_ENABLED(CONFIG_OF)
>
> You don't need to do this.
It's done like this in all the other drivers.
> > +static const struct of_device_id twl4030_pwrbutton_dt_match_table[] = {
> > + { .compatible = "ti,twl4030-pwrbutton" },
> > + {},
> > +};
> > +MODULE_DEVICE_TABLE(of, twl4030_pwrbutton_dt_match_table);
> > +#endif
> > +
> > static struct platform_driver twl4030_pwrbutton_driver = {
> > + .probe = twl4030_pwrbutton_probe,
> > .remove = __exit_p(twl4030_pwrbutton_remove),
> > .driver = {
> > .name = "twl4030_pwrbutton",
> > .owner = THIS_MODULE,
> > + .of_match_table = of_match_ptr(twl4030_pwrbutton_dt_match_table),
>
> If you try to compile this driver with config !CONFIG_OF it will not work in
> this way.
For !CONFIG_OF of_match_ptr is defined as follows (in "include/linux/of.h"):
#define of_match_ptr(_ptr) NULL
So the preprocessor will remove the undefined symbol.
-- Sebastian
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: changes to ati_remote to support ATI Remote Wonder Plus RF Remote Control
From: Torrey Hoffman @ 2013-10-25 14:28 UTC (permalink / raw)
To: sfo; +Cc: linux-input
In-Reply-To: <34245.198.84.133.226.1382707162.squirrel@cargocult.ca>
I should mention that of course, I don't get to 'decide' who the
official maintainer of anything is :-)
But given that Stephen is willing to work on it, he has my blessing,
for what that's worth.
^ permalink raw reply
* changes to ati_remote to support ATI Remote Wonder Plus RF Remote Control
From: Stephen Oberski @ 2013-10-25 13:19 UTC (permalink / raw)
To: linux-input; +Cc: sfo
[-- Attachment #1: Type: text/plain, Size: 2469 bytes --]
Attached is a text document describing proposed changes to the ati_remote kernel module to support
the ATI Remote Wonder Plus RF Remote Control.
I've forwarded these proposed changes to the listed authors of the module for review (Anssi
Hannula <anssi.hannula@XXX.YYY> and Torrey Hoffman <thoffman@XXX.YYY>) and have not heard back
from Anssi Hannul and I include Torrey Hoffman's response below.
Thanks,
--
Stephen Oberski sfo@cargocult.ca
289 430 5076
http://ca.linkedin.com/in/stephenoberski/
---------------------------------------- Original Message ----------------------------------------
Subject: Re: [Fwd: changes to ati_remote to support ATI Remote Wonder Plus RF Remote Control]
From: "Torrey Hoffman" <thoffman@XXX.YYY>
Date: Mon, October 21, 2013 12:21 pm
To: sfo@cargocult.ca
--------------------------------------------------------------------------------------------------
Thanks for the heads up, and for working on this.
Unfortunately I no longer have any relevant hardware to test with. If you
would like to be the official maintainer of this module, that would be
excellent.
Best wishes,
Torrey
On Mon, Oct 21, 2013 at 5:18 PM, Stephen Oberski <sfo@cargocult.ca> wrote:
> Hi Torrey,
>
> Attached is a text document describing proposed changes to the ati_remote
> kernel module to support
> the ATI Remote Wonder Plus RF Remote Control.
>
> I am sending this to you as the author of the module before posting to the
> linux-input mailing
> list and would appreciate any feedback.
>
> Thanks,
>
> --
> Stephen Oberski sfo@cargocult.ca
> 289 430 5076
> http://ca.linkedin.com/in/stephenoberski/
>
>
> ---------------------------------------- Original Message
> ----------------------------------------
> Subject: changes to ati_remote to support ATI Remote Wonder Plus RF Remote
> Control
> From: "Stephen Oberski" <sfo@cargocult.ca>
> Date: Sat, October 12, 2013 9:53 pm
> To: anssi.hannula@XXX.YYY
> Cc: sfo@cargocult.ca
>
> --------------------------------------------------------------------------------------------------
>
> Hi Anssi,
>
> Attached is a text document describing proposed changes to the ati_remote
> kernel module to support
> the ATI Remote Wonder Plus RF Remote Control.
>
> I am sending this to you as the author of the module before posting to the
> linux-input mailing
> list and would appreciated any feedback.
>
> Thanks,
>
> --
> Stephen Oberski sfo@cargocult.ca
> 289 430 5076
>
[-- Attachment #2: linux-input_ati_remote-problem.txt --]
[-- Type: text/plain, Size: 12887 bytes --]
Proposed changes to the ati_remote kernel loadable module (drivers/media/rc/ati_remote.c) to support the ATI Remote Wonder Plus RF Remote Control
-------------------------------------------------------------------------------------------------------------------------------------------------
Date: Thu Oct 10 10:53:59 EDT 2013
By: Stephen Oberski (sfo@cargocult.ca)
Overview
========
The ati_remote.c USB ATI Remote support loadable kernel module (Version 2.2.0, Linux 3.8.0-31-generic)
supports the SnapStream Media Firefly PC Remote Wireless Receiver Model R1000-1 receiver (FF receiver) with the Firefly RF Remote Control Model R1000 (FF remote).
It also supports the ATI USB RF Wireless Remote Receiver Part # 5000027000G receiver (ATI receiver) with the Firefly RF Remote Control Model R1000.
However neither the SnapStream Media Firefly PC Remote Wireless Receiver Model R1000-1 receiver nor the ATI USB RF Wireless Remote Receiver Part # 5000027000G receiver
are supported with the ATI Remote Wonder Plus RF Remote Control Part # 5000026900G (ATI remote).
A simple change to the ati_remote.c module results in the support of all combinations of the FF and ATI RF receivers and remotes.
The RF receiver initialization sequence is modified and the processing of the 4 byte remote inputs by the driver is modified to support a leading byte value of 0x15 as well as the current 0x14.
These changes appear to be backward compatible and do no seem to impact the existing behaviour of the module when presented with data from currently supported RF receiver and remote combinations
while allowing currently unsupported combinations to be used.
Problem Scope
=============
It appears that data generated by the ATI remote is never being received by the ati_remote kernel module:
============================== CUT HERE ==============================
# terminal window 1
# modprobe usbmon
# cd /sys/kernel/debug/usb/usbmon
# lsusb
...
Bus 004 Device 008: ID 0bc7:0004 X10 Wireless Technology, Inc. X10 Receiver
...
# rmmod ati_remote
# modprobe ati_remote
# terminal window 2
# grep --line-buffered '4:008' 4u
ffff88018a00d3c0 1361036384 C Ii:4:008:1 -108:8 0
ffff88018a00d240 1385420709 S Io:4:008:2 -115:8 5 = 80010020 14
ffff88018a00d240 1385424393 C Io:4:008:2 0:8 5 >
ffff88018a00d240 1385424401 S Io:4:008:2 -115:8 8 = 80010020 14202020
ffff88018a00d240 1385432392 C Io:4:008:2 0:8 8 >
ffff88018a00da80 1385432519 S Ii:4:008:1 -115:8 8 <
ffff88018a00da80 1385440394 C Ii:4:008:1 0:8 1 = ff
ffff88018a00da80 1385440399 S Ii:4:008:1 -115:8 8 <
ffff88018a00da80 1415136406 C Ii:4:008:1 0:8 4 = 14658010
ffff88018a00da80 1415136432 S Ii:4:008:1 -115:8 8 <
ffff88018a00da80 1415176406 C Ii:4:008:1 0:8 4 = 14658010
ffff88018a00da80 1415176413 S Ii:4:008:1 -115:8 8 <
ffff88018a00da80 1415216405 C Ii:4:008:1 0:8 4 = 14658010
ffff88018a00da80 1415216411 S Ii:4:008:1 -115:8 8 <
ffff88018a00da80 1415256404 C Ii:4:008:1 0:8 4 = 14658010
ffff88018a00da80 1415256410 S Ii:4:008:1 -115:8 8 <
ffff88018a00da80 1415296405 C Ii:4:008:1 0:8 4 = 14658010
ffff88018a00da80 1415296412 S Ii:4:008:1 -115:8 8 <
ffff88018a00da80 1415336404 C Ii:4:008:1 0:8 4 = 14658010
ffff88018a00da80 1415336410 S Ii:4:008:1 -115:8 8 <
============================== CUT HERE ==============================
We log USB data for the RF receiver in window 2.
In window 1 we enable USB monitoring and unload and reload the ati_remote module.
We can see the 2 initialization strings being sent to the RF receiver via the interrupt out (Io) lines
and then when we press the Firefly key on the FF remote we see the repeated USB packets sent by the RF remote (14658010).
However when we press any keys on the ATI remote no data is logged by usbmon.
This indicates that the RF receiver is ignoring data sent by the ATI remote when initialized by the ati_remote modules.
However when we load the modified version of the ati_remote module we see:
============================== CUT HERE ==============================
# rmmod ati_remote
# insmod ./ati_remote_plus.ko
# grep --line-buffered '4:008' 4u
ffff88018a00da80 2183228699 C Ii:4:008:1 -108:8 0
ffff880181305a80 2197203596 S Io:4:008:2 -115:8 9 = 8080051b 15142024 15
ffff880181305a80 2197216703 C Io:4:008:2 0:8 9 >
ffff880181305a80 2197216761 S Io:4:008:2 -115:8 3 = 808303
ffff880181305a80 2197224702 C Io:4:008:2 0:8 3 >
ffff880181305a80 2197224714 S Io:4:008:2 -115:8 4 = 8084d720
ffff880181305a80 2197232701 C Io:4:008:2 0:8 4 >
ffff8801813050c0 2197232835 S Ii:4:008:1 -115:8 8 <
ffff8801813050c0 2197240704 C Ii:4:008:1 0:8 1 = 00
ffff8801813050c0 2197240711 S Ii:4:008:1 -115:8 8 <
ffff8801813050c0 2219904713 C Ii:4:008:1 0:8 4 = 14658010
ffff8801813050c0 2219904740 S Ii:4:008:1 -115:8 8 <
ffff8801813050c0 2219944712 C Ii:4:008:1 0:8 4 = 14658010
ffff8801813050c0 2219944720 S Ii:4:008:1 -115:8 8 <
ffff8801813050c0 2219984712 C Ii:4:008:1 0:8 4 = 14658010
ffff8801813050c0 2219984718 S Ii:4:008:1 -115:8 8 <
ffff8801813050c0 2220024715 C Ii:4:008:1 0:8 4 = 14658010
ffff8801813050c0 2220024722 S Ii:4:008:1 -115:8 8 <
ffff8801813050c0 2220064715 C Ii:4:008:1 0:8 4 = 14658010
ffff8801813050c0 2220064721 S Ii:4:008:1 -115:8 8 <
ffff8801813050c0 2221952713 C Ii:4:008:1 0:8 4 = 15222d20
ffff8801813050c0 2221952740 S Ii:4:008:1 -115:8 8 <
ffff8801813050c0 2221992712 C Ii:4:008:1 0:8 4 = 15222d20
ffff8801813050c0 2221992720 S Ii:4:008:1 -115:8 8 <
ffff8801813050c0 2222032713 C Ii:4:008:1 0:8 4 = 15222d20
ffff8801813050c0 2222032720 S Ii:4:008:1 -115:8 8 <
ffff8801813050c0 2222072715 C Ii:4:008:1 0:8 4 = 15222d20
ffff8801813050c0 2222072721 S Ii:4:008:1 -115:8 8 <
ffff8801813050c0 2222120710 C Ii:4:008:1 0:8 4 = 15222d20
ffff8801813050c0 2222120716 S Ii:4:008:1 -115:8 8 <
ffff8801813050c0 2222160714 C Ii:4:008:1 0:8 4 = 15222d20
ffff8801813050c0 2222160720 S Ii:4:008:1 -115:8 8 <
============================== CUT HERE ==============================
Here we can see the modified initialization sequence being sent to the RF remote and when we press the Firefly key on the FF remote we see the repeated USB packets sent by the FF remote (14658010)
and when we press the ATI key on the ATI remote we see the repeated USB packets sent by the ATI remote (15222d20).
The conclusion is that the RF receiver initialization must be changed in the ati_remote module in order to support the ATI remote.
History
=======
The changes made to the ati_remote.c module are based on changes made to the LIRC lirc_atiusb.c code [1]
by Cymen Vig in Feb 2006, who used a USB packet snooping program on Windows, these changes now seems to have disappeared from the current version of LIRC (0.9.0)
and a patch made to the drivers/media/rc/ati_remote.c file for the 2.6.31 kernel [2] which was never committed into the Linux source tree.
Hardware
========
- ATI USB RF Wireless Remote Receiver Part # 5000027000G:
Oct 10 16:07:08 acer-Veriton-N282G kernel: [695612.752080] usb 2-2: new low-speed USB device number 4 using uhci_hcd
Oct 10 16:07:08 acer-Veriton-N282G kernel: [695612.929218] usb 2-2: New USB device found, idVendor=0bc7, idProduct=0004
Oct 10 16:07:08 acer-Veriton-N282G kernel: [695612.929227] usb 2-2: New USB device strings: Mfr=1, Product=2, SerialNumber=0
Oct 10 16:07:08 acer-Veriton-N282G kernel: [695612.929233] usb 2-2: Product: USB Receiver
Oct 10 16:07:08 acer-Veriton-N282G kernel: [695612.929239] usb 2-2: Manufacturer: X10 Wireless Technology Inc
Oct 10 16:07:08 acer-Veriton-N282G kernel: [695613.044063] Registered IR keymap rc-ati-x10
- SnapStream Media Firefly PC Remote Wireless Receiver Model R1000-1:
Oct 10 19:14:59 OptiPlex-755 kernel: [534490.804065] usb 4-1: new low-speed USB device number 6 using uhci_hcd
Oct 10 19:14:59 OptiPlex-755 kernel: [534490.974451] usb 4-1: New USB device found, idVendor=0bc7, idProduct=0008
Oct 10 19:14:59 OptiPlex-755 kernel: [534490.974455] usb 4-1: New USB device strings: Mfr=1, Product=2, SerialNumber=0
Oct 10 19:14:59 OptiPlex-755 kernel: [534490.974458] usb 4-1: Product: USB Receiver
Oct 10 19:14:59 OptiPlex-755 kernel: [534490.974461] usb 4-1: Manufacturer: X10 Wireless Technology Inc
Oct 10 19:15:00 OptiPlex-755 kernel: [534491.032019] Registered IR keymap rc-snapstream-firefly
- Firefly RF Remote Control Model R1000
- ATI Remote Wonder Plus RF Remote Control Part # 5000026900G
Product ID Test Coverage
========================
+--------------------------+------+-----+------+-----------------------+---------------------+
|Product Name +ID +FF RC+ATI RC+3.8.0-31-generic/x86_64+3.8.0-31-generic/i686|
+--------------------------+------+-----+------+-----------------------+---------------------+
|LOLA_REMOTE_PRODUCT_ID |0x0002|No |No |No |No |
|LOLA2_REMOTE_PRODUCT_ID |0x0003|No |No |No |No |
|ATI_REMOTE_PRODUCT_ID |0x0004|Yes |Yes |Yes |Yes |
|NVIDIA_REMOTE_PRODUCT_ID |0x0005|No |No |No |No |
|MEDION_REMOTE_PRODUCT_ID |0x0006|No |No |No |No |
|FIREFLY_REMOTE_PRODUCT_ID |0x0008|Yes |Yes |Yes |Yes |
+--------------------------+------+-----+------+-----------------------+---------------------+
FF RC: Firefly Remote Control
ATI RC: ATI Remote Control
3.8.0-31-generic/x86_64: 64 bit kernel
3.8.0-31-generic/i686: 32 bit kernel
Yes: Successful test
No: Not tested
Userspace Test
==============
A userspace test program [3] was developed to test the Remote receiver and remote control combinations.
It locates the X10 receiver by searching the USB devices for a Vendor ID of 0xbc7, any Product ID is allowed.
It then initializes the X10 receiver via interrupt writes using the new initialization strings discovered by Cymen Vig [1]:
static char init1[] = { 0x80, 0x05, 0x1b, 0x15, 0x14, 0x20, 0x24, 0x15 };
static char init2[] = { 0x83, 0x03 };
static char init3[] = { 0x84, 0xd7, 0x020 };
It then enters a loop and displays all packets received via interrupt reads, along with the computed checksum.
It is assumed that the libusb-dev: "userspace USB programming library development files" package is installed.
Compile and link as follows:
$ gcc -c testlibusb1.c
$ gcc testlibusb1.o -o testlibusb1 -lusb
Unload the "ati_remote" module if loaded:
$ sudo rmmod ati_remote
Run the test program:
$ sudo ./testlibusb1
...
found X10 0xbc7:0x8
found 1 interfaces
found 2 endpoints
endpoint 0 direction IN 0x81
endpoint 1 direction OUT 0x2
X10 open() OK
X10 set configuration OK
X10 claim interface 0 OK
X10 write 8 bytes to endpoint 0x2 OK with 8 bytes written
X10 write 2 bytes to endpoint 0x2 OK with 2 bytes written
X10 write 3 bytes to endpoint 0x2 OK with 3 bytes written
X10 read from endpoint 0x1 returned 4
0:14 1:e5 2:00 3:10 checksum e5
X10 read from endpoint 0x1 returned 4
0:14 1:e5 2:00 3:10 checksum e5
X10 read from endpoint 0x1 returned 4
0:14 1:e5 2:00 3:10 checksum e5
X10 read from endpoint 0x1 returned 4
X10 read from endpoint 0x1 returned 4
0:15 1:a2 2:ad 3:20 checksum a2
X10 read from endpoint 0x1 returned 4
0:15 1:a2 2:ad 3:20 checksum a2
X10 read from endpoint 0x1 returned 4
0:15 1:a2 2:ad 3:20 checksum a2
X10 read from endpoint 0x1 returned 4
0:15 1:a2 2:ad 3:20 checksum a2
...
^C
In the sample output above the SnapStream Media Firefly PC Remote Wireless Receiver Model R1000-1 was connected as can be seen by the Vendor ID value of 0x8.
Each remote key press generates 4 usb packets, each USB packet is always 4 bytes in length.
The first 4 packets were from the Firefly button on the Firefly RF Remote Control Model R1000, byte 0 is always 0x14 for this remote.
The second 4 packets were from the ATI button on the ATI Remote Wonder Plus RF Remote Control Part # 5000026900G, byte 0 is always 0x15 for this remote.
Other than the value of byte 0 the format of the the USB packets generated by the Firefly and ATI remotes are identical, including the channel mask (byte 3) and checksum calculation.
Exactly the same results are obtained when the ATI USB RF Wireless Remote Receiver Part # 5000027000G is used.
Conclusions
===========
The modified version of the ati_remote driver [4] and the corresponding patch file [5] are provided.
The untested Product ID and remote control combinations need to be tested.
I have no access to the untested RF receivers corresponding to the Product IDs so perhaps users on the linux-input and linux-media
could test this patch using other RF receiver and remote combinations.
References
==========
[1] http://www.mythtv.org/pipermail/mythtv-users/2006-February/121861.html
[2] http://www.mythtv.org/wiki/ATI_Remote_Wonder_Plus
[3] http://cargocult.ca/testlibusb1.c
[4] http://cargocult.ca/ati_remote_plus.c
[5] http://cargocult.ca/ati_remote.patch
^ permalink raw reply
* Re: [PATCHv6 1/3] Input: twl4030-pwrbutton - add device tree support
From: Peter Ujfalusi @ 2013-10-25 12:46 UTC (permalink / raw)
To: Sebastian Reichel, Sebastian Reichel, Dmitry Torokhov
Cc: Grant Likely, Rob Herring, Sachin Kamat, Florian Vaussard,
linux-input, linux-kernel, devicetree
In-Reply-To: <1382626126-12565-2-git-send-email-sre@debian.org>
On 10/24/2013 05:48 PM, Sebastian Reichel wrote:
> Add device tree support for twl4030 power button driver.
>
> Signed-off-by: Sebastian Reichel <sre@debian.org>
> ---
> .../devicetree/bindings/input/twl4030-pwrbutton.txt | 21 +++++++++++++++++++++
> drivers/input/misc/twl4030-pwrbutton.c | 16 ++++++++++++----
> 2 files changed, 33 insertions(+), 4 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/input/twl4030-pwrbutton.txt
>
> diff --git a/Documentation/devicetree/bindings/input/twl4030-pwrbutton.txt b/Documentation/devicetree/bindings/input/twl4030-pwrbutton.txt
> new file mode 100644
> index 0000000..4375646
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/twl4030-pwrbutton.txt
> @@ -0,0 +1,21 @@
> +Texas Instruments TWL family (twl4030) pwrbutton module
> +
> +This module is part of the TWL4030. For more details about the whole
> +chip see Documentation/devicetree/bindings/mfd/twl-familly.txt.
> +
> +This module provides a simple power button event via an Interrupt.
> +
> +Required properties:
> +- compatible: should be one of the following
> + - "ti,twl4030-pwrbutton": For controllers compatible with twl4030
> +- interrupt: should be one of the following
> + - <8>: For controllers compatible with twl4030
> +
> +Example:
> +
> +&twl {
> + twl_pwrbutton: pwrbutton {
> + compatible = "ti,twl4030-pwrbutton";
> + interrupts = <8>;
> + };
> +};
> diff --git a/drivers/input/misc/twl4030-pwrbutton.c b/drivers/input/misc/twl4030-pwrbutton.c
> index b9a05fd..a3a0fe3 100644
> --- a/drivers/input/misc/twl4030-pwrbutton.c
> +++ b/drivers/input/misc/twl4030-pwrbutton.c
> @@ -52,7 +52,7 @@ static irqreturn_t powerbutton_irq(int irq, void *_pwr)
> return IRQ_HANDLED;
> }
>
> -static int __init twl4030_pwrbutton_probe(struct platform_device *pdev)
> +static int twl4030_pwrbutton_probe(struct platform_device *pdev)
> {
> struct input_dev *pwr;
> int irq = platform_get_irq(pdev, 0);
> @@ -106,16 +106,24 @@ static int __exit twl4030_pwrbutton_remove(struct platform_device *pdev)
> return 0;
> }
>
> +#if IS_ENABLED(CONFIG_OF)
You don't need to do this.
> +static const struct of_device_id twl4030_pwrbutton_dt_match_table[] = {
> + { .compatible = "ti,twl4030-pwrbutton" },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, twl4030_pwrbutton_dt_match_table);
> +#endif
> +
> static struct platform_driver twl4030_pwrbutton_driver = {
> + .probe = twl4030_pwrbutton_probe,
> .remove = __exit_p(twl4030_pwrbutton_remove),
> .driver = {
> .name = "twl4030_pwrbutton",
> .owner = THIS_MODULE,
> + .of_match_table = of_match_ptr(twl4030_pwrbutton_dt_match_table),
If you try to compile this driver with config !CONFIG_OF it will not work in
this way.
> },
> };
> -
> -module_platform_driver_probe(twl4030_pwrbutton_driver,
> - twl4030_pwrbutton_probe);
> +module_platform_driver(twl4030_pwrbutton_driver);
>
> MODULE_ALIAS("platform:twl4030_pwrbutton");
> MODULE_DESCRIPTION("Triton2 Power Button");
>
--
Péter
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCHv6 1/3] Input: twl4030-pwrbutton - add device tree support
From: Florian Vaussard @ 2013-10-25 12:40 UTC (permalink / raw)
To: Sebastian Reichel, Sebastian Reichel, Dmitry Torokhov
Cc: Grant Likely, Rob Herring, Peter Ujfalusi, Sachin Kamat,
linux-input, linux-kernel, devicetree
In-Reply-To: <1382626126-12565-2-git-send-email-sre@debian.org>
Hello,
On 10/24/2013 04:48 PM, Sebastian Reichel wrote:
> Add device tree support for twl4030 power button driver.
>
> Signed-off-by: Sebastian Reichel <sre@debian.org>
I tested on my OMAP3 board, the button event works as expected. So for
the DT boot, feel free to add my:
Tested-by: Florian Vaussard <florian.vaussard@epfl.ch>
Best regards,
Florian
^ permalink raw reply
* Re: [PATCH] HID: hid-multitouch: add support for SiS panels
From: Forest Bond @ 2013-10-25 11:56 UTC (permalink / raw)
To: Jiri Kosina; +Cc: linux-input
In-Reply-To: <alpine.LNX.2.00.1310250947340.30926@pobox.suse.cz>
[-- Attachment #1: Type: text/plain, Size: 427 bytes --]
Hi Jiri,
On Fri, Oct 25, 2013 at 09:47:55AM +0100, Jiri Kosina wrote:
> On Mon, 21 Oct 2013, Forest Bond wrote:
>
> > From: Forest Bond <forest.bond@rapidrollout.com>
> >
> > Signed-off-by: Forest Bond <forest.bond@rapidrollout.com>
>
> Added changelog (I don't like commits without changelog) and applied.
Noted. Thanks!
-Forest
--
Forest Bond
http://www.forestbond.com/
http://www.rapidrollout.com/
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* Re: [PATCH] HID: Added Holtek USB ID 04d9:a072 LEETGION Hellion Gaming Mouse
From: Jiri Kosina @ 2013-10-25 9:28 UTC (permalink / raw)
To: Anders F. U. Kiær; +Cc: linux-input, linux-kernel
In-Reply-To: <1382391742-31335-1-git-send-email-ablacksheep@gmail.com>
On Mon, 21 Oct 2013, Anders F. U. Kiær wrote:
> Added id, bindings and comments for Holtek USB ID 04d9:a072
> LEETGION Hellion Gaming mouse to use the same corrections of the report
> descriptor as Holtek 04d9:a067. As the mouse exceed HID_MAX_USAGES at the
> same offsets in the reported descriptor.
> Tested on the hardware.
>
> Signed-off-by: Anders F. U. Kiær <ablacksheep@gmail.com>
Applied, thanks.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH] HID: hid-sensor-hub: fix report size
From: Jiri Kosina @ 2013-10-25 9:01 UTC (permalink / raw)
To: Srinivas Pandruvada; +Cc: linux-input
In-Reply-To: <1382114924-25654-1-git-send-email-srinivas.pandruvada@linux.intel.com>
On Fri, 18 Oct 2013, Srinivas Pandruvada wrote:
> The number of bytes in a field needs to take account of report_count
> field. Need to multiply report_size and report_count to get total
> number of bytes.
Srinivas,
could you please elaborate a little bit more in the changelog what
user-visible bug this is fixing? Thanks.
>
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> ---
> drivers/hid/hid-sensor-hub.c | 13 ++++++++-----
> 1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/hid/hid-sensor-hub.c b/drivers/hid/hid-sensor-hub.c
> index 88fc5ae..a184e19 100644
> --- a/drivers/hid/hid-sensor-hub.c
> +++ b/drivers/hid/hid-sensor-hub.c
> @@ -326,7 +326,8 @@ int sensor_hub_input_get_attribute_info(struct hid_sensor_hub_device *hsdev,
> field->logical == attr_usage_id) {
> sensor_hub_fill_attr_info(info, i, report->id,
> field->unit, field->unit_exponent,
> - field->report_size);
> + field->report_size *
> + field->report_count);
> ret = 0;
> } else {
> for (j = 0; j < field->maxusage; ++j) {
> @@ -338,7 +339,8 @@ int sensor_hub_input_get_attribute_info(struct hid_sensor_hub_device *hsdev,
> i, report->id,
> field->unit,
> field->unit_exponent,
> - field->report_size);
> + field->report_size *
> + field->report_count);
> ret = 0;
> break;
> }
> @@ -425,9 +427,10 @@ static int sensor_hub_raw_event(struct hid_device *hdev,
> hid_dbg(hdev, "%d collection_index:%x hid:%x sz:%x\n",
> i, report->field[i]->usage->collection_index,
> report->field[i]->usage->hid,
> - report->field[i]->report_size/8);
> -
> - sz = report->field[i]->report_size/8;
> + (report->field[i]->report_size *
> + report->field[i]->report_count)/8);
> + sz = (report->field[i]->report_size *
> + report->field[i]->report_count)/8;
> if (pdata->pending.status && pdata->pending.attr_usage_id ==
> report->field[i]->usage->hid) {
> hid_dbg(hdev, "data was pending ...\n");
> --
> 1.8.3.2
>
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH] HID: hid-multitouch: add support for SiS panels
From: Jiri Kosina @ 2013-10-25 8:47 UTC (permalink / raw)
To: Forest Bond; +Cc: linux-input
In-Reply-To: <20131021163849.GC30240@alittletooquiet.net>
On Mon, 21 Oct 2013, Forest Bond wrote:
> From: Forest Bond <forest.bond@rapidrollout.com>
>
> Signed-off-by: Forest Bond <forest.bond@rapidrollout.com>
Added changelog (I don't like commits without changelog) and applied.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCHv7][ 1/4] Input: tsc2007: Add device tree support.
From: Thierry Reding @ 2013-10-25 8:33 UTC (permalink / raw)
To: Lothar Waßmann
Cc: Denis Carikli, Rob Herring, Shawn Guo, Eric Bénard,
Sascha Hauer, linux-arm-kernel, Grant Likely, Pawel Moll,
Mark Rutland, Stephen Warren, Ian Campbell, devicetree,
Dmitry Torokhov, linux-input, Linus Walleij
In-Reply-To: <20131025083951.18675574@ipc1.ka-ro>
[-- Attachment #1: Type: text/plain, Size: 3967 bytes --]
On Fri, Oct 25, 2013 at 08:39:51AM +0200, Lothar Waßmann wrote:
> Hi,
>
> Thierry Reding wrote:
> > On Thu, Oct 24, 2013 at 02:42:13PM +0200, Denis Carikli wrote:
> > > Cc: Rob Herring <rob.herring@calxeda.com>
> > > Cc: Pawel Moll <pawel.moll@arm.com>
> > > Cc: Mark Rutland <mark.rutland@arm.com>
> > > Cc: Stephen Warren <swarren@wwwdotorg.org>
> > > Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
> > > Cc: devicetree@vger.kernel.org
> > > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > > Cc: linux-input@vger.kernel.org
> > > Cc: Sascha Hauer <kernel@pengutronix.de>
> > > Cc: linux-arm-kernel@lists.infradead.org
> > > Cc: Lothar Waßmann <LW@KARO-electronics.de>
> > > Cc: Eric Bénard <eric@eukrea.com>
> > > Signed-off-by: Denis Carikli <denis@eukrea.com>
> > > ---
> > > ChangeLog v6->v7:
> > > - One small whitespace cleanup.
> > > - The properties specific to that driver are now prefixed with "ti,".
> > > - The ti,fuzzy property has now better documentation.
> > > ---
> > > .../bindings/input/touchscreen/tsc2007.txt | 45 +++++
> > > drivers/input/touchscreen/tsc2007.c | 194 +++++++++++++++-----
> > > 2 files changed, 198 insertions(+), 41 deletions(-)
> > > create mode 100644 Documentation/devicetree/bindings/input/touchscreen/tsc2007.txt
> > >
> > > diff --git a/Documentation/devicetree/bindings/input/touchscreen/tsc2007.txt b/Documentation/devicetree/bindings/input/touchscreen/tsc2007.txt
> > > new file mode 100644
> > > index 0000000..516b63b
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/input/touchscreen/tsc2007.txt
> > > @@ -0,0 +1,45 @@
> > > +* Texas Instruments tsc2007 touchscreen controller
> > > +
> > > +Required properties:
> > > +- compatible: must be "ti,tsc2007".
> > > +- reg: I2C address of the chip.
> > > +- ti,x-plate-ohms: X-plate resistance in ohms.
> > > +
> > > +Optional properties:
> > > +- gpios: the interrupt gpio the chip is connected to (trough the penirq pin)
> > > + (see GPIO binding[2] for more details).
> > > +- interrupt-parent: the phandle for the gpio controller
> > > + (see interrupt binding[1]).
> > > +- interrupts: (gpio) interrupt to which the chip is connected
> > > + (see interrupt binding[1]).
> > > +- pinctrl-0: Should specify pin control groups used for the gpio
> > > + (see pinctrl bindings[0]).
> > > +- pinctrl-names: Should contain only one value - "default"
> > > + (see pinctrl bindings[0]).
> >
> > Also I haven't seen a response as to why this can't be handled by the
> > GPIO driver. Adding Linus Walleij, perhaps he knows a more definitive
> > answer.
> >
> > Linus, the issue here is that the pinctrl properties for this chip are
> > supposed to pinmux the pendown GPIO for this chip. I was under the
> > impression that this should be handled by the GPIO controller itself, so
> > that when gpio_request() was called on a pin it would be the GPIO
> > controller driver's responsibility to pinmux it appropriately.
> >
> When a GPIO is requested the GPIO layer cannot know what it is
> requested for and how the pinmux for the pin function should be
> configured (pullups, drive strength, ...). A pin may have multiple
> functions depending on what modules are plugged into a baseboard.
But GPIO is one specific function of a given pin, isn't it?
> Thus the pinctrl binding needs to be with the driver that uses the pin.
I wonder if this really needs to be dynamically configured depending on
the driver. Properties like pullups and driver strength can potentially
harm hardware when they're wrong, so relying on the driver's probe to
configure them correctly doesn't sound very safe. It's always possible
to disable the device (either via modifying the DTS or by having the
bootloader patch it). That means the driver won't be probed at all, and
therefore none of the pinctrl or pinmux settings will be applied.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH v4] Input: add regulator haptic driver
From: Mark Brown @ 2013-10-25 8:29 UTC (permalink / raw)
To: Hyunhee Kim
Cc: dmitry.torokhov, peter.ujfalusi, wfp5p, linux-input, linux-kernel,
akpm, kyungmin.park
In-Reply-To: <002601ced14b$9c412330$d4c36990$%kim@samsung.com>
[-- Attachment #1: Type: text/plain, Size: 492 bytes --]
On Fri, Oct 25, 2013 at 03:29:51PM +0900, Hyunhee Kim wrote:
> From: Hyunhee Kim <hyunhee.kim@samsung.com>
> Date: Wed, 9 Oct 2013 16:21:36 +0900
> Subject: [PATCH] Input: add regulator haptic driver
This is OK from a regulator point of view though there's still a lot of
use of regulator devm_ and input devm_ that could happen - it wasn't
just the kzalloc(), it was most of the allocations in the driver.
You also have some word wrapping in your e-mail so the patch probably
won't apply.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCHv7][ 1/4] Input: tsc2007: Add device tree support.
From: Lothar Waßmann @ 2013-10-25 6:39 UTC (permalink / raw)
To: Thierry Reding
Cc: Mark Rutland, devicetree, Dmitry Torokhov, Eric Bénard,
Pawel Moll, Stephen Warren, Linus Walleij, Ian Campbell,
Rob Herring, Grant Likely, Denis Carikli, Sascha Hauer,
linux-input, Shawn Guo, linux-arm-kernel
In-Reply-To: <20131024151736.GE9044@ulmo.nvidia.com>
Hi,
Thierry Reding wrote:
> On Thu, Oct 24, 2013 at 02:42:13PM +0200, Denis Carikli wrote:
> > Cc: Rob Herring <rob.herring@calxeda.com>
> > Cc: Pawel Moll <pawel.moll@arm.com>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Cc: Stephen Warren <swarren@wwwdotorg.org>
> > Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
> > Cc: devicetree@vger.kernel.org
> > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > Cc: linux-input@vger.kernel.org
> > Cc: Sascha Hauer <kernel@pengutronix.de>
> > Cc: linux-arm-kernel@lists.infradead.org
> > Cc: Lothar Waßmann <LW@KARO-electronics.de>
> > Cc: Eric Bénard <eric@eukrea.com>
> > Signed-off-by: Denis Carikli <denis@eukrea.com>
> > ---
> > ChangeLog v6->v7:
> > - One small whitespace cleanup.
> > - The properties specific to that driver are now prefixed with "ti,".
> > - The ti,fuzzy property has now better documentation.
> > ---
> > .../bindings/input/touchscreen/tsc2007.txt | 45 +++++
> > drivers/input/touchscreen/tsc2007.c | 194 +++++++++++++++-----
> > 2 files changed, 198 insertions(+), 41 deletions(-)
> > create mode 100644 Documentation/devicetree/bindings/input/touchscreen/tsc2007.txt
> >
> > diff --git a/Documentation/devicetree/bindings/input/touchscreen/tsc2007.txt b/Documentation/devicetree/bindings/input/touchscreen/tsc2007.txt
> > new file mode 100644
> > index 0000000..516b63b
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/input/touchscreen/tsc2007.txt
> > @@ -0,0 +1,45 @@
> > +* Texas Instruments tsc2007 touchscreen controller
> > +
> > +Required properties:
> > +- compatible: must be "ti,tsc2007".
> > +- reg: I2C address of the chip.
> > +- ti,x-plate-ohms: X-plate resistance in ohms.
> > +
> > +Optional properties:
> > +- gpios: the interrupt gpio the chip is connected to (trough the penirq pin)
> > + (see GPIO binding[2] for more details).
> > +- interrupt-parent: the phandle for the gpio controller
> > + (see interrupt binding[1]).
> > +- interrupts: (gpio) interrupt to which the chip is connected
> > + (see interrupt binding[1]).
> > +- pinctrl-0: Should specify pin control groups used for the gpio
> > + (see pinctrl bindings[0]).
> > +- pinctrl-names: Should contain only one value - "default"
> > + (see pinctrl bindings[0]).
>
> Also I haven't seen a response as to why this can't be handled by the
> GPIO driver. Adding Linus Walleij, perhaps he knows a more definitive
> answer.
>
> Linus, the issue here is that the pinctrl properties for this chip are
> supposed to pinmux the pendown GPIO for this chip. I was under the
> impression that this should be handled by the GPIO controller itself, so
> that when gpio_request() was called on a pin it would be the GPIO
> controller driver's responsibility to pinmux it appropriately.
>
When a GPIO is requested the GPIO layer cannot know what it is
requested for and how the pinmux for the pin function should be
configured (pullups, drive strength, ...). A pin may have multiple
functions depending on what modules are plugged into a baseboard.
Thus the pinctrl binding needs to be with the driver that uses the pin.
Lothar Waßmann
--
___________________________________________________________
Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996
www.karo-electronics.de | info@karo-electronics.de
___________________________________________________________
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH v4] Input: add regulator haptic driver
From: Hyunhee Kim @ 2013-10-25 6:29 UTC (permalink / raw)
To: 'Mark Brown', dmitry.torokhov, peter.ujfalusi, wfp5p,
linux-input, linux-kernel, akpm
Cc: 'hyunhee.kim', kyungmin.park
In-Reply-To: <20131024133836.GG18506@sirena.org.uk>
From: Hyunhee Kim <hyunhee.kim@samsung.com>
Date: Wed, 9 Oct 2013 16:21:36 +0900
Subject: [PATCH] Input: add regulator haptic driver
Signed-off-by: Hyunhee Kim <hyunhee.kim@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
Acked-by: Aristeu Rozanski <aris@ruivo.org>
---
drivers/input/misc/Kconfig | 10 ++
drivers/input/misc/Makefile | 1 +
drivers/input/misc/regulator-haptic.c | 187
+++++++++++++++++++++++++++++++++
3 files changed, 198 insertions(+)
create mode 100644 drivers/input/misc/regulator-haptic.c
diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
index bb698e1..21b4d5b 100644
--- a/drivers/input/misc/Kconfig
+++ b/drivers/input/misc/Kconfig
@@ -82,6 +82,16 @@ config INPUT_ARIZONA_HAPTICS
To compile this driver as a module, choose M here: the
module will be called arizona-haptics.
+config INPUT_REGULATOR_HAPTIC
+ tristate "support haptics on/off using regulator"
+ select INPUT_FF_MEMLESS
+ help
+ Say Y to enable support for the haptic module. Haptic can be
+ enabled/disabled by regulator.
+
+ To compile this driver as a module, choose M here: the
+ module will be called regulator-haptic.
+
config INPUT_BMA150
tristate "BMA150/SMB380 acceleration sensor support"
depends on I2C
diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
index d7fc17f..106f0bc 100644
--- a/drivers/input/misc/Makefile
+++ b/drivers/input/misc/Makefile
@@ -15,6 +15,7 @@ obj-$(CONFIG_INPUT_ADXL34X_I2C) +=
adxl34x-i2c.o
obj-$(CONFIG_INPUT_ADXL34X_SPI) += adxl34x-spi.o
obj-$(CONFIG_INPUT_APANEL) += apanel.o
obj-$(CONFIG_INPUT_ARIZONA_HAPTICS) += arizona-haptics.o
+obj-$(CONFIG_INPUT_REGULATOR_HAPTIC) += regulator-haptic.o
obj-$(CONFIG_INPUT_ATI_REMOTE2) += ati_remote2.o
obj-$(CONFIG_INPUT_ATLAS_BTNS) += atlas_btns.o
obj-$(CONFIG_INPUT_BFIN_ROTARY) += bfin_rotary.o
diff --git a/drivers/input/misc/regulator-haptic.c
b/drivers/input/misc/regulator-haptic.c
new file mode 100644
index 0000000..bab2ad9
--- /dev/null
+++ b/drivers/input/misc/regulator-haptic.c
@@ -0,0 +1,187 @@
+/*
+ * Regulator haptic driver
+ *
+ * Copyright (c) 2013 Samsung Electronics Co., Ltd.
+ *
+ * Author: Hyunhee Kim <hyunhee.kim@samsung.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/input.h>
+#include <linux/slab.h>
+#include <linux/regulator/driver.h>
+
+struct regulator_haptic {
+ struct device *dev;
+ struct input_dev *input_dev;
+ struct work_struct work;
+ bool enabled;
+ struct regulator *regulator;
+ struct mutex mutex;
+ int level;
+};
+
+static void regulator_haptic_enable(struct regulator_haptic *haptic)
+{
+ int ret;
+
+ if (!haptic->enabled) {
+ ret = regulator_enable(haptic->regulator);
+ if (ret)
+ dev_err(haptic->dev, "failed to enable
regulator\n");
+ haptic->enabled = true;
+ }
+}
+
+static void regulator_haptic_disable(struct regulator_haptic *haptic)
+{
+ int ret;
+
+ if (haptic->enabled) {
+ ret = regulator_disable(haptic->regulator);
+ if (ret)
+ dev_err(haptic->dev, "failed to disable
regulator\n");
+ haptic->enabled = false;
+ }
+}
+
+static void regulator_haptic_work(struct work_struct *work)
+{
+ struct regulator_haptic *haptic = container_of(work,
+ struct
regulator_haptic,
+ work);
+ mutex_lock(&haptic->mutex);
+ if (haptic->level)
+ regulator_haptic_enable(haptic);
+ else
+ regulator_haptic_disable(haptic);
+ mutex_unlock(&haptic->mutex);
+}
+
+static int regulator_haptic_play(struct input_dev *input, void *data,
+ struct ff_effect *effect)
+{
+ struct regulator_haptic *haptic = input_get_drvdata(input);
+
+ haptic->level = effect->u.rumble.strong_magnitude;
+ if (!haptic->level)
+ haptic->level = effect->u.rumble.weak_magnitude;
+ schedule_work(&haptic->work);
+
+ return 0;
+}
+
+static void regulator_haptic_close(struct input_dev *input)
+{
+ struct regulator_haptic *haptic = input_get_drvdata(input);
+
+ cancel_work_sync(&haptic->work);
+ regulator_haptic_disable(haptic);
+}
+
+static int regulator_haptic_probe(struct platform_device *pdev)
+{
+ struct regulator_haptic *haptic;
+ struct input_dev *input_dev;
+ int error;
+
+ haptic = devm_kzalloc(&pdev->dev, sizeof(*haptic), GFP_KERNEL);
+ if (!haptic) {
+ dev_err(&pdev->dev, "unable to allocate memory for
haptic\n");
+ return -ENOMEM;
+ }
+
+ input_dev = input_allocate_device();
+
+ if (!input_dev) {
+ dev_err(&pdev->dev, "unable to allocate memory\n");
+ return -ENOMEM;
+ }
+
+ INIT_WORK(&haptic->work, regulator_haptic_work);
+ mutex_init(&haptic->mutex);
+ haptic->input_dev = input_dev;
+ haptic->dev = &pdev->dev;
+ haptic->regulator = regulator_get(&pdev->dev, "haptic");
+
+ if (IS_ERR(haptic->regulator)) {
+ error = PTR_ERR(haptic->regulator);
+ dev_err(&pdev->dev, "unable to get regulator, err: %d\n",
+ error);
+ goto err_ifree_mem;
+ }
+
+ haptic->input_dev->name = "regulator:haptic";
+ haptic->input_dev->dev.parent = &pdev->dev;
+ haptic->input_dev->close = regulator_haptic_close;
+ haptic->enabled = false;
+ input_set_drvdata(haptic->input_dev, haptic);
+ input_set_capability(haptic->input_dev, EV_FF, FF_RUMBLE);
+
+ error = input_ff_create_memless(input_dev, NULL,
+ regulator_haptic_play);
+ if (error) {
+ dev_err(&pdev->dev,
+ "input_ff_create_memless() failed: %d\n",
+ error);
+ goto err_put_regulator;
+ }
+
+ error = input_register_device(haptic->input_dev);
+ if (error) {
+ dev_err(&pdev->dev,
+ "couldn't register input device: %d\n",
+ error);
+ goto err_destroy_ff;
+ }
+
+ platform_set_drvdata(pdev, haptic);
+
+ return 0;
+
+err_destroy_ff:
+ input_ff_destroy(haptic->input_dev);
+err_put_regulator:
+ regulator_put(haptic->regulator);
+err_ifree_mem:
+ input_free_device(haptic->input_dev);
+
+ return error;
+}
+
+static int regulator_haptic_remove(struct platform_device *pdev)
+{
+ struct regulator_haptic *haptic = platform_get_drvdata(pdev);
+
+ input_unregister_device(haptic->input_dev);
+ regulator_put(haptic->regulator);
+
+ return 0;
+}
+
+static struct of_device_id regulator_haptic_dt_match[] = {
+ { .compatible = "linux,regulator-haptic" },
+ {},
+};
+
+static struct platform_driver regulator_haptic_driver = {
+ .driver = {
+ .name = "regulator-haptic",
+ .owner = THIS_MODULE,
+ .of_match_table = regulator_haptic_dt_match,
+ },
+
+ .probe = regulator_haptic_probe,
+ .remove = regulator_haptic_remove,
+};
+module_platform_driver(regulator_haptic_driver);
+
+MODULE_ALIAS("platform:regulator-haptic");
+MODULE_DESCRIPTION("Regulator haptic driver");
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Hyunhee Kim <hyunhee.kim@samsung.com>");
--
1.7.9.5
^ permalink raw reply related
* Re: [PATCHv7][ 1/4] Input: tsc2007: Add device tree support.
From: Lothar Waßmann @ 2013-10-25 5:56 UTC (permalink / raw)
To: Grant Likely
Cc: Mark Rutland, devicetree, Dmitry Torokhov, Eric BXXnard,
Pawel Moll, Stephen Warren, Ian Campbell, Rob Herring,
Denis Carikli, Thierry Reding, Sascha Hauer, linux-input,
Shawn Guo, linux-arm-kernel
In-Reply-To: <20131024164746.3CB36C403B6@trevor.secretlab.ca>
Hi,
Grant Likely wrote:
> On Thu, 24 Oct 2013 14:42:13 +0200, Denis Carikli <denis@eukrea.com> wrote:
> > Cc: Rob Herring <rob.herring@calxeda.com>
> > Cc: Pawel Moll <pawel.moll@arm.com>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Cc: Stephen Warren <swarren@wwwdotorg.org>
> > Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
> > Cc: devicetree@vger.kernel.org
> > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > Cc: linux-input@vger.kernel.org
> > Cc: Sascha Hauer <kernel@pengutronix.de>
> > Cc: linux-arm-kernel@lists.infradead.org
> > Cc: Lothar Waßmann <LW@KARO-electronics.de>
> > Cc: Eric Bénard <eric@eukrea.com>
> > Signed-off-by: Denis Carikli <denis@eukrea.com>
> > ---
> > ChangeLog v6->v7:
> > - One small whitespace cleanup.
> > - The properties specific to that driver are now prefixed with "ti,".
> > - The ti,fuzzy property has now better documentation.
> > ---
> > .../bindings/input/touchscreen/tsc2007.txt | 45 +++++
> > drivers/input/touchscreen/tsc2007.c | 194 +++++++++++++++-----
> > 2 files changed, 198 insertions(+), 41 deletions(-)
> > create mode 100644 Documentation/devicetree/bindings/input/touchscreen/tsc2007.txt
> >
> > diff --git a/Documentation/devicetree/bindings/input/touchscreen/tsc2007.txt b/Documentation/devicetree/bindings/input/touchscreen/tsc2007.txt
> > new file mode 100644
> > index 0000000..516b63b
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/input/touchscreen/tsc2007.txt
> > @@ -0,0 +1,45 @@
> > +* Texas Instruments tsc2007 touchscreen controller
> > +
> > +Required properties:
> > +- compatible: must be "ti,tsc2007".
> > +- reg: I2C address of the chip.
> > +- ti,x-plate-ohms: X-plate resistance in ohms.
> > +
> > +Optional properties:
> > +- gpios: the interrupt gpio the chip is connected to (trough the penirq pin)
> > + (see GPIO binding[2] for more details).
>
> Hmmm, why is this needed? Is the line used for anything other than
> finding the irq number? If it is just an interrupt then an interrupts
> property is all that should be specified.
>
It's also the pendown detect GPIO. It asserts an interrupt when first
activated and is then polled in the driver until it goes inactive again.
Lothar Waßmann
--
___________________________________________________________
Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996
www.karo-electronics.de | info@karo-electronics.de
___________________________________________________________
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 4/8] input: Remove OOM message after input_allocate_device
From: Joe Perches @ 2013-10-24 19:20 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: linux-kernel, Wan ZongShun, Andrey Moiseev, Henrik Rydberg,
Josh Wu, Ferruh Yigit, Pau Oliva Fora, Ben Dooks, Kukjin Kim,
linux-input, linux-arm-kernel, linux-samsung-soc
In-Reply-To: <20131024191032.GA5553@core.coreip.homeip.net>
On Thu, 2013-10-24 at 12:10 -0700, Dmitry Torokhov wrote:
> On Thu, Oct 24, 2013 at 11:45:39AM -0700, Joe Perches wrote:
> > On Thu, 2013-10-24 at 11:37 -0700, Dmitry Torokhov wrote:
> > > Hi Joe,
> > >
> > > On Wed, Oct 23, 2013 at 12:14:50PM -0700, Joe Perches wrote:
> > > > Emitting an OOM message isn't necessary after input_allocate_device
> > > > as there's a generic OOM and a dump_stack already done.
> > >
> > > No, please don't. The kzalloc may get changed in the future to not dump
> > > stack (that was added originally because not everyone was handling OOM
> > > properly, right?), input core might get changed to use something else
> > > than kzalloc, etc, etc.
> > >
> > > The majority of errors use dev_err so we also get idea what device
> > > failed (if there are several), and more.
> >
> > I think that's not valuable as input_allocate_device already has
> > dozens of locations that don't emit a specific OOM and centralizing
> > the location for any generic message would work anyway.
>
> Not having diagnostic messages in some of the drivers is hardly a
> justification to remove them from everywhere else.
But standardization is.
If a diagnostic message is really desired
without the already existing dump_stack(),
it's a small matter of adding something like:
drivers/input/input.c | 26 +++++++++++++++-----------
1 file changed, 15 insertions(+), 11 deletions(-)
diff --git a/drivers/input/input.c b/drivers/input/input.c
index c044699..98570c2 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -1736,19 +1736,23 @@ struct input_dev *input_allocate_device(void)
{
struct input_dev *dev;
- dev = kzalloc(sizeof(struct input_dev), GFP_KERNEL);
- if (dev) {
- dev->dev.type = &input_dev_type;
- dev->dev.class = &input_class;
- device_initialize(&dev->dev);
- mutex_init(&dev->mutex);
- spin_lock_init(&dev->event_lock);
- INIT_LIST_HEAD(&dev->h_list);
- INIT_LIST_HEAD(&dev->node);
-
- __module_get(THIS_MODULE);
+ dev = kzalloc(sizeof(struct input_dev), GFP_KERNEL | __GFP_NOWARN);
+ if (!dev) {
+ pr_err("%pf: input_allocate_device failed\n",
+ __builtin_return_address(1));
+ return NULL;
}
+ dev->dev.type = &input_dev_type;
+ dev->dev.class = &input_class;
+ device_initialize(&dev->dev);
+ mutex_init(&dev->mutex);
+ spin_lock_init(&dev->event_lock);
+ INIT_LIST_HEAD(&dev->h_list);
+ INIT_LIST_HEAD(&dev->node);
+
+ __module_get(THIS_MODULE);
+
return dev;
}
EXPORT_SYMBOL(input_allocate_device);
^ permalink raw reply related
* Re: [PATCH 4/8] input: Remove OOM message after input_allocate_device
From: Uwe Kleine-König @ 2013-10-24 19:15 UTC (permalink / raw)
To: Joe Perches
Cc: Kukjin Kim, Ferruh Yigit, Wan ZongShun, Andrey Moiseev,
Dmitry Torokhov, linux-kernel, Josh Wu, linux-samsung-soc,
Henrik Rydberg, Ben Dooks, linux-input, Pau Oliva Fora,
linux-arm-kernel
In-Reply-To: <1382640528.22433.75.camel@joe-AO722>
On Thu, Oct 24, 2013 at 11:48:48AM -0700, Joe Perches wrote:
> On Thu, 2013-10-24 at 20:46 +0200, Uwe Kleine-König wrote:
> > On Thu, Oct 24, 2013 at 11:43:38AM -0700, Joe Perches wrote:
> []
> > > Any k.alloc without __GFP_NOWARN does a generic OOM message
> > > and a dump_stack() so there could already be 2 messages anyway.
> > Then mention that in the commit log if you still want this patch?!
>
> How do you think this commit message should be changed:
>
> Emitting an OOM message isn't necessary after input_allocate_device
> as there's a generic OOM and a dump_stack already done.
after input_allocate_device and/or k.alloc ...
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* Re: [PATCH 4/8] input: Remove OOM message after input_allocate_device
From: Dmitry Torokhov @ 2013-10-24 19:10 UTC (permalink / raw)
To: Joe Perches
Cc: linux-kernel, Wan ZongShun, Andrey Moiseev, Henrik Rydberg,
Josh Wu, Ferruh Yigit, Pau Oliva Fora, Ben Dooks, Kukjin Kim,
linux-input, linux-arm-kernel, linux-samsung-soc
In-Reply-To: <1382640339.22433.73.camel@joe-AO722>
On Thu, Oct 24, 2013 at 11:45:39AM -0700, Joe Perches wrote:
> On Thu, 2013-10-24 at 11:37 -0700, Dmitry Torokhov wrote:
> > Hi Joe,
> >
> > On Wed, Oct 23, 2013 at 12:14:50PM -0700, Joe Perches wrote:
> > > Emitting an OOM message isn't necessary after input_allocate_device
> > > as there's a generic OOM and a dump_stack already done.
> >
> > No, please don't. The kzalloc may get changed in the future to not dump
> > stack (that was added originally because not everyone was handling OOM
> > properly, right?), input core might get changed to use something else
> > than kzalloc, etc, etc.
> >
> > The majority of errors use dev_err so we also get idea what device
> > failed (if there are several), and more.
>
> I think that's not valuable as input_allocate_device already has
> dozens of locations that don't emit a specific OOM and centralizing
> the location for any generic message would work anyway.
Not having diagnostic messages in some of the drivers is hardly a
justification to remove them from everywhere else.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 4/8] input: Remove OOM message after input_allocate_device
From: Joe Perches @ 2013-10-24 18:48 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Kukjin Kim, Ferruh Yigit, Wan ZongShun, Andrey Moiseev,
Dmitry Torokhov, linux-kernel, Josh Wu, linux-samsung-soc,
Henrik Rydberg, Ben Dooks, linux-input, Pau Oliva Fora,
linux-arm-kernel
In-Reply-To: <20131024184612.GC3512@pengutronix.de>
On Thu, 2013-10-24 at 20:46 +0200, Uwe Kleine-König wrote:
> On Thu, Oct 24, 2013 at 11:43:38AM -0700, Joe Perches wrote:
[]
> > Any k.alloc without __GFP_NOWARN does a generic OOM message
> > and a dump_stack() so there could already be 2 messages anyway.
> Then mention that in the commit log if you still want this patch?!
How do you think this commit message should be changed:
Emitting an OOM message isn't necessary after input_allocate_device
as there's a generic OOM and a dump_stack already done.
^ permalink raw reply
* Re: [PATCH 4/8] input: Remove OOM message after input_allocate_device
From: Uwe Kleine-König @ 2013-10-24 18:46 UTC (permalink / raw)
To: Joe Perches
Cc: Kukjin Kim, Ferruh Yigit, Wan ZongShun, Andrey Moiseev,
Dmitry Torokhov, linux-kernel, Josh Wu, linux-samsung-soc,
Henrik Rydberg, Ben Dooks, linux-input, Pau Oliva Fora,
linux-arm-kernel
In-Reply-To: <1382640218.22433.71.camel@joe-AO722>
On Thu, Oct 24, 2013 at 11:43:38AM -0700, Joe Perches wrote:
> On Thu, 2013-10-24 at 20:26 +0200, Uwe Kleine-König wrote:
> > Hello Joe,
> >
> > On Wed, Oct 23, 2013 at 12:14:50PM -0700, Joe Perches wrote:
> > > Emitting an OOM message isn't necessary after input_allocate_device
> > > as there's a generic OOM and a dump_stack already done.
> > >
> > > [...]
> > > Signed-off-by: Joe Perches <joe@perches.com>
> > > diff --git a/drivers/input/joystick/as5011.c b/drivers/input/joystick/as5011.c
> > > index 005d852..3b9c709 100644
> > > --- a/drivers/input/joystick/as5011.c
> > > +++ b/drivers/input/joystick/as5011.c
> > > @@ -254,8 +254,6 @@ static int as5011_probe(struct i2c_client *client,
> > > as5011 = kmalloc(sizeof(struct as5011_device), GFP_KERNEL);
> > > input_dev = input_allocate_device();
> > > if (!as5011 || !input_dev) {
> > > - dev_err(&client->dev,
> > > - "Can't allocate memory for device structure\n");
> > Don't know if that can happen, but if as5011 is NULL but input_dev isn't
> > the message would still be sensible, wouldn't it? There are several more
> > that suffer the same "problem".
>
> Any k.alloc without __GFP_NOWARN does a generic OOM message
> and a dump_stack() so there could already be 2 messages anyway.
Then mention that in the commit log if you still want this patch?!
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* Re: [PATCH 4/8] input: Remove OOM message after input_allocate_device
From: Joe Perches @ 2013-10-24 18:45 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: linux-kernel, Wan ZongShun, Andrey Moiseev, Henrik Rydberg,
Josh Wu, Ferruh Yigit, Pau Oliva Fora, Ben Dooks, Kukjin Kim,
linux-input, linux-arm-kernel, linux-samsung-soc
In-Reply-To: <20131024183707.GA5281@core.coreip.homeip.net>
On Thu, 2013-10-24 at 11:37 -0700, Dmitry Torokhov wrote:
> Hi Joe,
>
> On Wed, Oct 23, 2013 at 12:14:50PM -0700, Joe Perches wrote:
> > Emitting an OOM message isn't necessary after input_allocate_device
> > as there's a generic OOM and a dump_stack already done.
>
> No, please don't. The kzalloc may get changed in the future to not dump
> stack (that was added originally because not everyone was handling OOM
> properly, right?), input core might get changed to use something else
> than kzalloc, etc, etc.
>
> The majority of errors use dev_err so we also get idea what device
> failed (if there are several), and more.
I think that's not valuable as input_allocate_device already has
dozens of locations that don't emit a specific OOM and centralizing
the location for any generic message would work anyway.
^ permalink raw reply
* Re: [PATCH 4/8] input: Remove OOM message after input_allocate_device
From: Joe Perches @ 2013-10-24 18:43 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: linux-kernel, Kukjin Kim, Ferruh Yigit, Wan ZongShun,
Andrey Moiseev, Dmitry Torokhov, Josh Wu, linux-samsung-soc,
Henrik Rydberg, Ben Dooks, linux-input, Pau Oliva Fora,
linux-arm-kernel
In-Reply-To: <20131024182611.GA3512@pengutronix.de>
On Thu, 2013-10-24 at 20:26 +0200, Uwe Kleine-König wrote:
> Hello Joe,
>
> On Wed, Oct 23, 2013 at 12:14:50PM -0700, Joe Perches wrote:
> > Emitting an OOM message isn't necessary after input_allocate_device
> > as there's a generic OOM and a dump_stack already done.
> >
> > [...]
> > Signed-off-by: Joe Perches <joe@perches.com>
> > diff --git a/drivers/input/joystick/as5011.c b/drivers/input/joystick/as5011.c
> > index 005d852..3b9c709 100644
> > --- a/drivers/input/joystick/as5011.c
> > +++ b/drivers/input/joystick/as5011.c
> > @@ -254,8 +254,6 @@ static int as5011_probe(struct i2c_client *client,
> > as5011 = kmalloc(sizeof(struct as5011_device), GFP_KERNEL);
> > input_dev = input_allocate_device();
> > if (!as5011 || !input_dev) {
> > - dev_err(&client->dev,
> > - "Can't allocate memory for device structure\n");
> Don't know if that can happen, but if as5011 is NULL but input_dev isn't
> the message would still be sensible, wouldn't it? There are several more
> that suffer the same "problem".
Any k.alloc without __GFP_NOWARN does a generic OOM message
and a dump_stack() so there could already be 2 messages anyway.
cheers, Joe
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 4/8] input: Remove OOM message after input_allocate_device
From: Dmitry Torokhov @ 2013-10-24 18:37 UTC (permalink / raw)
To: Joe Perches
Cc: linux-kernel, Wan ZongShun, Andrey Moiseev, Henrik Rydberg,
Josh Wu, Ferruh Yigit, Pau Oliva Fora, Ben Dooks, Kukjin Kim,
linux-input, linux-arm-kernel, linux-samsung-soc
In-Reply-To: <cc5e6a317bc9ef33cfdfae3ed9d3a47b5319a47b.1382555436.git.joe@perches.com>
Hi Joe,
On Wed, Oct 23, 2013 at 12:14:50PM -0700, Joe Perches wrote:
> Emitting an OOM message isn't necessary after input_allocate_device
> as there's a generic OOM and a dump_stack already done.
No, please don't. The kzalloc may get changed in the future to not dump
stack (that was added originally because not everyone was handling OOM
properly, right?), input core might get changed to use something else
than kzalloc, etc, etc.
The majority of errors use dev_err so we also get idea what device
failed (if there are several), and more.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 4/8] input: Remove OOM message after input_allocate_device
From: Uwe Kleine-König @ 2013-10-24 18:26 UTC (permalink / raw)
To: Joe Perches
Cc: linux-kernel, Kukjin Kim, Ferruh Yigit, Wan ZongShun,
Andrey Moiseev, Dmitry Torokhov, Josh Wu, linux-samsung-soc,
Henrik Rydberg, Ben Dooks, linux-input, Pau Oliva Fora,
linux-arm-kernel
In-Reply-To: <cc5e6a317bc9ef33cfdfae3ed9d3a47b5319a47b.1382555436.git.joe@perches.com>
Hello Joe,
On Wed, Oct 23, 2013 at 12:14:50PM -0700, Joe Perches wrote:
> Emitting an OOM message isn't necessary after input_allocate_device
> as there's a generic OOM and a dump_stack already done.
>
> [...]
> Signed-off-by: Joe Perches <joe@perches.com>
> diff --git a/drivers/input/joystick/as5011.c b/drivers/input/joystick/as5011.c
> index 005d852..3b9c709 100644
> --- a/drivers/input/joystick/as5011.c
> +++ b/drivers/input/joystick/as5011.c
> @@ -254,8 +254,6 @@ static int as5011_probe(struct i2c_client *client,
> as5011 = kmalloc(sizeof(struct as5011_device), GFP_KERNEL);
> input_dev = input_allocate_device();
> if (!as5011 || !input_dev) {
> - dev_err(&client->dev,
> - "Can't allocate memory for device structure\n");
Don't know if that can happen, but if as5011 is NULL but input_dev isn't
the message would still be sensible, wouldn't it? There are several more
that suffer the same "problem".
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* Re: [PATCH V4 1/2] tps6507x-ts: Add DT support
From: Mark Rutland @ 2013-10-24 17:27 UTC (permalink / raw)
To: Manish Badarkhe
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
davinci-linux-open-source-VycZQUHpC/PFrsHnngEfi1aTQe2KTcn/@public.gmane.org,
linux-doc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org,
dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org,
grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org,
rob-VoJi6FS/r0vR7s880joybQ@public.gmane.org,
linux-input-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <CAKDJKT4+ma+KdNX0uemZUQVKHp8giGUX8EgWkY40vYOR-KR0_A-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Thu, Oct 24, 2013 at 06:05:53PM +0100, Manish Badarkhe wrote:
> Hi Mark,
>
> Thank you for your reply.
>
> On Wed, Oct 23, 2013 at 10:15 PM, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:
>
> On Wed, Oct 23, 2013 at 05:28:52PM +0100, Manish Badarkhe wrote:
> > Add device tree based support for TI's tps6507x touchscreen.
> >
> > Signed-off-by: Manish Badarkhe <badarkhe.manish-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > ---
> > Changes since V3:
> > - Rebased on top of Dmitry's changes
> > - Removed error handling for optional DT properties
> >
> > Changes since V2:
> > - Removed unnecessary code.
> > - Updated Documentation to provide proper names of
> > devicetree properties.
> >
> > Changes since V1:
> > - Updated documentation to specify tps6507x as multifunctional
> > device.
> > - return proper error value in absence of platform and DT
> > data for touchscreen.
> > - Updated commit message.
> >
> > :100755 100755 8fffa3c... e1b9cfd... M Documentation/devicetree/
> bindings/mfd/tps6507x.txt
> > :100644 100644 db604e0... 0cf0eb1... M drivers/input/touchscreen/
> tps6507x-ts.c
> > Documentation/devicetree/bindings/mfd/tps6507x.txt | 24 ++++++-
> > drivers/input/touchscreen/tps6507x-ts.c | 72
> ++++++++++++--------
> > 2 files changed, 67 insertions(+), 29 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/mfd/tps6507x.txt b/
> Documentation/devicetree/bindings/mfd/tps6507x.txt
> > index 8fffa3c..e1b9cfd 100755
> > --- a/Documentation/devicetree/bindings/mfd/tps6507x.txt
> > +++ b/Documentation/devicetree/bindings/mfd/tps6507x.txt
> > @@ -1,4 +1,8 @@
> > -TPS6507x Power Management Integrated Circuit
> > +TPS6507x Multifunctional Device.
> > +
> > +Features provided by TPS6507x:
> > + 1.Power Management Integrated Circuit.
> > + 2.Touch-Screen.
> >
> > Required properties:
> > - compatible: "ti,tps6507x"
> > @@ -23,6 +27,9 @@ Required properties:
> > vindcdc1_2-supply: VDCDC1 and VDCDC2 input.
> > vindcdc3-supply : VDCDC3 input.
> > vldo1_2-supply : VLDO1 and VLDO2 input.
> > +- tsc: This node specifies touch screen data.
>
> This is a node, but is listed un "Required properties". That seems odd.
>
> When is it required?
>
> Why is the data under the tsc subnode? Why not directly under the main
> node?
>
>
> Yes, It seems odd to add a subnode properties here. I gone through other
> documentation
> for MFD devices and observed that separate documentation file is present for
> sub node modules.
I was trying to say that nodes != properties rather than that this should be
split into separate files.
>
> TPS6507x is multifunctional device and having touch screen submodule. As it is
> child node for
> TPS6507x multifunctional device, I have created child node as "tsc".
>
>
>
> > + ti,poll-period : Time at which touch input is getting sampled in
> ms.
>
> Is this for debounce? Other bindings have similar but differently named
> properties.
>
>
> This is not debounce time. This time is required for input poll devices.
>
>
> Why is this necessary? Can the driver not choose a sane value?
>
>
> poll-period is necessary to sample touch inputs. Driver will chose value
> default poll
> period if this value is not present. I was bit confused here, whether to add
> this property
> as optional or required. As with default period touch not achieve performance.
> Can I make this property optional?
Please elaborate. Why will the default period be sub-optimal? What's the
tradeoff here?
>
>
> > + ti,min-pressure: Minimum pressure value to trigger touch.
>
> What type are these? Single (u32) cells?
>
> What units is ti,min-pressure in?
>
>
> No, its a u16 cell. It is measured in ohm. Again default value for min-pressure
> is available
> in driver code. Can I make this property optional?
Why is it a u16, it's very uncommon to have u16 properties.
What _physical_ units is this in, and what order of magnitude? e.g. Pascals,
millipascals.
>
>
> >
> > Regulator Optional properties:
> > - defdcdc_default: It's property of DCDC2 and DCDC3 regulators.
> > @@ -30,6 +37,14 @@ Regulator Optional properties:
> > 1: If defdcdc pin of DCDC2/DCDC3 is driven HIGH.
> > If this property is not defined, it defaults to 0 (not enabled).
> >
> > +Touchscreen Optional properties:
> > +- ti,vendor : Touchscreen vendor id to populate
> > + in sysclass interface.
> > +- ti,product: Touchscreen product id to populate
> > + in sysclass interface.
> > +- ti,version: Touchscreen version id to populate
> > + in sysclass interface.
>
> Are these integers, strings, or something else?
>
>
> These are unsigned short(u16) values.
Why?
>
>
> What values make sense?
>
>
> These values are only to tell about chip details.
That does not describe the set of valid values.
Is ti,vendor = <4> valid? What does this mean?
Is there some standard for assignment of vendor IDs that this follows?
>
>
> What's the sysclass interface? That sounds like a Linux detail. The
> properties
> might be fine but the description shouldn't need to refer to Linux
> internals.
>
>
> Okay, I will update description for these properties.
>
>
> > +
> > Example:
> >
> > pmu: tps6507x@48 {
> > @@ -88,4 +103,11 @@ Example:
> > };
> > };
> >
> > + tsc {
> > + ti,poll-period = <30>;
> > + ti,min-pressure = <0x30>;
> > + ti,vendor = <0>;
> > + ti,product = <65070>;
> > + ti,version = <0x100>;
> > + };
> > };
> > diff --git a/drivers/input/touchscreen/tps6507x-ts.c b/drivers/input/
> touchscreen/tps6507x-ts.c
> > index db604e0..0cf0eb1 100644
> > --- a/drivers/input/touchscreen/tps6507x-ts.c
> > +++ b/drivers/input/touchscreen/tps6507x-ts.c
> > @@ -22,6 +22,8 @@
> > #include <linux/mfd/tps6507x.h>
> > #include <linux/input/tps6507x-ts.h>
> > #include <linux/delay.h>
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> >
> > #define TSC_DEFAULT_POLL_PERIOD 30 /* ms */
> > #define TPS_DEFAULT_MIN_PRESSURE 0x30
> > @@ -206,33 +208,54 @@ done:
> > tps6507x_adc_standby(tsc);
> > }
> >
> > +static void tsc_init_data(struct tps6507x_ts *tsc,
> > + struct input_dev *input_dev)
> > +{
> > + struct device_node *node = tsc->dev->of_node;
> > + const struct tps6507x_board *tps_board =
> > + dev_get_platdata(tsc->dev);
> > + const struct touchscreen_init_data *init_data = NULL;
> > +
> > + if (node)
> > + node = of_find_node_by_name(node, "tsc");
>
> As of_find_node_by_name traverses the list of all nodes (not just
> children),
> this may match nodes other than what you expect.
>
>
> I think, It traverse nodes from parent node (i.e. tps6507x) and find child.
> Please correct me if I am wrong?
No. As I said, it traverses the list of all nodes. Is there is no matching
child, it will go on and possibly match a node that is not a direct child (e.g.
a child of another node).
Perhaps of_get_child_by_name is what you want.
>
>
>
> > + if (tps_board)
> > + /*
> > + * init_data points to array of touchscreen_init structure
> > + * coming from the board-evm file.
> > + */
> > + init_data = tps_board->tps6507x_ts_init_data;
> > +
> > + if (node == NULL || init_data == NULL) {
> > + tsc->poll_dev->poll_interval = TSC_DEFAULT_POLL_PERIOD;
> > + tsc->min_pressure = TPS_DEFAULT_MIN_PRESSURE;
> > + } else if (init_data) {
> > + tsc->poll_dev->poll_interval = init_data->poll_period;
> > + tsc->min_pressure = init_data->min_pressure;
> > + input_dev->id.vendor = init_data->vendor;
> > + input_dev->id.product = init_data->product;
> > + input_dev->id.version = init_data->version;
> > + } else if (node) {
> > + of_property_read_u32(node, "ti,poll-period",
> > + &tsc->poll_dev->poll_interval);
> > + of_property_read_u16(node, "ti,min-pressure",
> > + &tsc->min_pressure);
>
> You didn't mention these were 16-bit values in the binding.
>
> As DTB is encoded big-endian, and you didn't use a /bits/ 16 declaration
> (making the property a u32 cell), won't this read 0 in all cases you have a
> value in the expected range (as in your example)?
>
>
> I will mention these values as 16bit. values in binding.
Please explain _why_ they are 16-bit values. Even if they are 16-bit it may
make sense to have them as u32 values for general consistency and least
surprise.
Thanks,
Mark.
^ permalink raw reply
* Re: [PATCH V4 1/2] tps6507x-ts: Add DT support
From: Manish Badarkhe @ 2013-10-24 17:05 UTC (permalink / raw)
To: Mark Rutland
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
davinci-linux-open-source-VycZQUHpC/PFrsHnngEfi1aTQe2KTcn/@public.gmane.org,
linux-doc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org,
dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org,
grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org,
rob-VoJi6FS/r0vR7s880joybQ@public.gmane.org,
linux-input-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <20131023164548.GB6042@kartoffel>
[-- Attachment #1.1: Type: text/plain, Size: 8199 bytes --]
Hi Mark,
Thank you for your reply.
On Wed, Oct 23, 2013 at 10:15 PM, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:
> On Wed, Oct 23, 2013 at 05:28:52PM +0100, Manish Badarkhe wrote:
> > Add device tree based support for TI's tps6507x touchscreen.
> >
> > Signed-off-by: Manish Badarkhe <badarkhe.manish-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > ---
> > Changes since V3:
> > - Rebased on top of Dmitry's changes
> > - Removed error handling for optional DT properties
> >
> > Changes since V2:
> > - Removed unnecessary code.
> > - Updated Documentation to provide proper names of
> > devicetree properties.
> >
> > Changes since V1:
> > - Updated documentation to specify tps6507x as multifunctional
> > device.
> > - return proper error value in absence of platform and DT
> > data for touchscreen.
> > - Updated commit message.
> >
> > :100755 100755 8fffa3c... e1b9cfd... M
> Documentation/devicetree/bindings/mfd/tps6507x.txt
> > :100644 100644 db604e0... 0cf0eb1... M
> drivers/input/touchscreen/tps6507x-ts.c
> > Documentation/devicetree/bindings/mfd/tps6507x.txt | 24 ++++++-
> > drivers/input/touchscreen/tps6507x-ts.c | 72
> ++++++++++++--------
> > 2 files changed, 67 insertions(+), 29 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/mfd/tps6507x.txt
> b/Documentation/devicetree/bindings/mfd/tps6507x.txt
> > index 8fffa3c..e1b9cfd 100755
> > --- a/Documentation/devicetree/bindings/mfd/tps6507x.txt
> > +++ b/Documentation/devicetree/bindings/mfd/tps6507x.txt
> > @@ -1,4 +1,8 @@
> > -TPS6507x Power Management Integrated Circuit
> > +TPS6507x Multifunctional Device.
> > +
> > +Features provided by TPS6507x:
> > + 1.Power Management Integrated Circuit.
> > + 2.Touch-Screen.
> >
> > Required properties:
> > - compatible: "ti,tps6507x"
> > @@ -23,6 +27,9 @@ Required properties:
> > vindcdc1_2-supply: VDCDC1 and VDCDC2 input.
> > vindcdc3-supply : VDCDC3 input.
> > vldo1_2-supply : VLDO1 and VLDO2 input.
> > +- tsc: This node specifies touch screen data.
>
> This is a node, but is listed un "Required properties". That seems odd.
>
> When is it required?
>
> Why is the data under the tsc subnode? Why not directly under the main
> node?
>
Yes, It seems odd to add a subnode properties here. I gone through other
documentation
for MFD devices and observed that separate documentation file is present
for sub node modules.
TPS6507x is multifunctional device and having touch screen submodule. As it
is child node for
TPS6507x multifunctional device, I have created child node as "tsc".
>
> > + ti,poll-period : Time at which touch input is getting sampled in
> ms.
>
> Is this for debounce? Other bindings have similar but differently named
> properties.
>
> This is not debounce time. This time is required for input poll devices.
> Why is this necessary? Can the driver not choose a sane value?
>
> poll-period is necessary to sample touch inputs. Driver will chose value
default poll
period if this value is not present. I was bit confused here, whether to
add this property
as optional or required. As with default period touch not achieve
performance.
Can I make this property optional?
> > + ti,min-pressure: Minimum pressure value to trigger touch.
>
> What type are these? Single (u32) cells?
>
> What units is ti,min-pressure in?
>
> No, its a u16 cell. It is measured in ohm. Again default value for
min-pressure is available
in driver code. Can I make this property optional?
>
> > Regulator Optional properties:
> > - defdcdc_default: It's property of DCDC2 and DCDC3 regulators.
> > @@ -30,6 +37,14 @@ Regulator Optional properties:
> > 1: If defdcdc pin of DCDC2/DCDC3 is driven HIGH.
> > If this property is not defined, it defaults to 0 (not enabled).
> >
> > +Touchscreen Optional properties:
> > +- ti,vendor : Touchscreen vendor id to populate
> > + in sysclass interface.
> > +- ti,product: Touchscreen product id to populate
> > + in sysclass interface.
> > +- ti,version: Touchscreen version id to populate
> > + in sysclass interface.
>
> Are these integers, strings, or something else?
>
> These are unsigned short(u16) values.
> What values make sense?
>
> These values are only to tell about chip details.
> What's the sysclass interface? That sounds like a Linux detail. The
> properties
> might be fine but the description shouldn't need to refer to Linux
> internals.
>
> Okay, I will update description for these properties.
> +
> > Example:
> >
> > pmu: tps6507x@48 {
> > @@ -88,4 +103,11 @@ Example:
> > };
> > };
> >
> > + tsc {
> > + ti,poll-period = <30>;
> > + ti,min-pressure = <0x30>;
> > + ti,vendor = <0>;
> > + ti,product = <65070>;
> > + ti,version = <0x100>;
> > + };
> > };
> > diff --git a/drivers/input/touchscreen/tps6507x-ts.c
> b/drivers/input/touchscreen/tps6507x-ts.c
> > index db604e0..0cf0eb1 100644
> > --- a/drivers/input/touchscreen/tps6507x-ts.c
> > +++ b/drivers/input/touchscreen/tps6507x-ts.c
> > @@ -22,6 +22,8 @@
> > #include <linux/mfd/tps6507x.h>
> > #include <linux/input/tps6507x-ts.h>
> > #include <linux/delay.h>
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> >
> > #define TSC_DEFAULT_POLL_PERIOD 30 /* ms */
> > #define TPS_DEFAULT_MIN_PRESSURE 0x30
> > @@ -206,33 +208,54 @@ done:
> > tps6507x_adc_standby(tsc);
> > }
> >
> > +static void tsc_init_data(struct tps6507x_ts *tsc,
> > + struct input_dev *input_dev)
> > +{
> > + struct device_node *node = tsc->dev->of_node;
> > + const struct tps6507x_board *tps_board =
> > + dev_get_platdata(tsc->dev);
> > + const struct touchscreen_init_data *init_data = NULL;
> > +
> > + if (node)
> > + node = of_find_node_by_name(node, "tsc");
>
> As of_find_node_by_name traverses the list of all nodes (not just
> children),
> this may match nodes other than what you expect.
>
I think, It traverse nodes from parent node (i.e. tps6507x) and find child.
Please correct me if I am wrong?
>
> > + if (tps_board)
> > + /*
> > + * init_data points to array of touchscreen_init structure
> > + * coming from the board-evm file.
> > + */
> > + init_data = tps_board->tps6507x_ts_init_data;
> > +
> > + if (node == NULL || init_data == NULL) {
> > + tsc->poll_dev->poll_interval = TSC_DEFAULT_POLL_PERIOD;
> > + tsc->min_pressure = TPS_DEFAULT_MIN_PRESSURE;
> > + } else if (init_data) {
> > + tsc->poll_dev->poll_interval = init_data->poll_period;
> > + tsc->min_pressure = init_data->min_pressure;
> > + input_dev->id.vendor = init_data->vendor;
> > + input_dev->id.product = init_data->product;
> > + input_dev->id.version = init_data->version;
> > + } else if (node) {
> > + of_property_read_u32(node, "ti,poll-period",
> > + &tsc->poll_dev->poll_interval);
> > + of_property_read_u16(node, "ti,min-pressure",
> > + &tsc->min_pressure);
>
> You didn't mention these were 16-bit values in the binding.
>
> As DTB is encoded big-endian, and you didn't use a /bits/ 16 declaration
> (making the property a u32 cell), won't this read 0 in all cases you have a
> value in the expected range (as in your example)?
>
I will mention these values as 16bit. values in binding.
>
> > + of_property_read_u16(node, "ti,vendor",
> > + &input_dev->id.vendor);
> > + of_property_read_u16(node, "ti,product",
> > + &input_dev->id.product);
> > + of_property_read_u16(node, "ti,version",
> > + &input_dev->id.version);
>
> Similarly for these three.
>
Also, I will mention these values as 16bit in binding.
>
> Thanks,
> Mark.
>
Thanks,
Manish Badarkhe
[-- Attachment #1.2: Type: text/html, Size: 11889 bytes --]
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox