* [PATCH] Revert "Input: trackpoint - add new trackpoint firmware ID"
From: Sebastian Schmidt @ 2017-12-30 15:22 UTC (permalink / raw)
To: linux-input, dmitry.torokhov; +Cc: Greg KH, Aaron Ma
This reverts commit ec667683c532c93fb41e100e5d61a518971060e2, which
breaks the Trackpoint on ThinkPad X1 Carbon Gen5 (Model 20HR). That
commit intended to add support for later firmware versions to the
trackpoint driver, however, the version is reported in the second byte
whereas the change was made to the magic byte preceding that version.
The update package linked by Lenovo suggests that 20HR models use an
ALPS Touchpad instead.
Signed-off-by: Sebastian Schmidt <yath@yath.de>
Acked-by: Greg KH <gregkh@linuxfoundation.org>
Cc: stable@vger.kernel.org
---
drivers/input/mouse/trackpoint.c | 3 +--
drivers/input/mouse/trackpoint.h | 3 +--
2 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/input/mouse/trackpoint.c b/drivers/input/mouse/trackpoint.c
index 0871010f18d5..20b5b21c1bba 100644
--- a/drivers/input/mouse/trackpoint.c
+++ b/drivers/input/mouse/trackpoint.c
@@ -265,8 +265,7 @@ static int trackpoint_start_protocol(struct psmouse *psmouse, unsigned char *fir
if (ps2_command(&psmouse->ps2dev, param, MAKE_PS2_CMD(0, 2, TP_READ_ID)))
return -1;
- /* add new TP ID. */
- if (!(param[0] & TP_MAGIC_IDENT))
+ if (param[0] != TP_MAGIC_IDENT)
return -1;
if (firmware_id)
diff --git a/drivers/input/mouse/trackpoint.h b/drivers/input/mouse/trackpoint.h
index 88055755f82e..5617ed3a7d7a 100644
--- a/drivers/input/mouse/trackpoint.h
+++ b/drivers/input/mouse/trackpoint.h
@@ -21,9 +21,8 @@
#define TP_COMMAND 0xE2 /* Commands start with this */
#define TP_READ_ID 0xE1 /* Sent for device identification */
-#define TP_MAGIC_IDENT 0x03 /* Sent after a TP_READ_ID followed */
+#define TP_MAGIC_IDENT 0x01 /* Sent after a TP_READ_ID followed */
/* by the firmware ID */
- /* Firmware ID includes 0x1, 0x2, 0x3 */
/*
--
2.15.1
^ permalink raw reply related
* Re: PROBLEM: Changing speed on ThinkPad X1 Carbon 5th trackpoint causes "failed to enable mouse"
From: Aaron Ma @ 2017-12-30 15:08 UTC (permalink / raw)
To: Greg KH, Sebastian Schmidt; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20171230150235.GA19890@kroah.com>
Greg, Please check my last e-mail.
My patch will enable the synaptic trackpoint features like scroll mode
with middle button and trackpoint stick.
Even there is a firmware issue that it can not be set speed on sysfs.
But it is not driver issue.
We can NOT to make wrong code than facing the real issue.
Regards,
Aaron
On 12/30/2017 11:02 PM, Greg KH wrote:
> On Sat, Dec 30, 2017 at 03:40:40PM +0100, Sebastian Schmidt wrote:
>> Because reverting your commit fixes the issue for me.
>
> Great, it should be reverted then. Care to send the patch that does the
> revert to make it easy for Dmitry to apply it?
>
> thanks,
>
> greg k-h
>
^ permalink raw reply
* Re: PROBLEM: Changing speed on ThinkPad X1 Carbon 5th trackpoint causes "failed to enable mouse"
From: Greg KH @ 2017-12-30 15:02 UTC (permalink / raw)
To: Sebastian Schmidt; +Cc: Aaron Ma, dmitry.torokhov, linux-input
In-Reply-To: <20171230144040.GB23284@marax.lan.yath.de>
On Sat, Dec 30, 2017 at 03:40:40PM +0100, Sebastian Schmidt wrote:
> Because reverting your commit fixes the issue for me.
Great, it should be reverted then. Care to send the patch that does the
revert to make it easy for Dmitry to apply it?
thanks,
greg k-h
^ permalink raw reply
* Re: PROBLEM: Changing speed on ThinkPad X1 Carbon 5th trackpoint causes "failed to enable mouse"
From: Aaron Ma @ 2017-12-30 15:00 UTC (permalink / raw)
To: Sebastian Schmidt; +Cc: gregkh, dmitry.torokhov, linux-input
In-Reply-To: <20171230144040.GB23284@marax.lan.yath.de>
If X1C5 use alps, my patch will not be used. And this patch had been
verified on laptops with alps/elan sticks.
And on your laptop psmouse module already tried alps protocol, it failed
and fallback to PS/2.
My commit is to enable scroll mode with middle button and stick and
other trackpoint features.
Yes, like you said maybe you don't use these features, but other people
like to use.
I suggest you to use Ubuntu kernel that is built from mainline kernel
just for your convenience that you won't need to rebuild the kernel with
RMI4_SMB config enabled. *NOT* for you to always use.
Indeed I didn't know the evbug is enabled. Even for now I don't think
this kernel with evbug will hack or harm you system even I am not a
security guy.
Regards,
Aaron
On 12/30/2017 10:40 PM, Sebastian Schmidt wrote:
> On Sat, Dec 30, 2017 at 10:17:55PM +0800, Aaron Ma wrote:
>> Sorry, I don't know anything about the firmware software.
>> So you can NOT expect me to fix the firmware issue.
>
> You changed trackpoint.c in ec667683c532c93fb41e100e5d61a518971060e2 to,
> according to the commit message, “support version 2 and 3”. Even though
> I don’t understand the change, because according to the comment next to
> TP_MAGIC_IDENT (and the code in trackpoint.c), the version is reported
> in param[1], not in param[0]. Also it’s called “MAGIC_IDENT” and not
> “SUPPORTED_FIRMWARE_VERSIONS”.
>
>> I have helped answer all the question I can.
>> I don't know why you blame me like this.
>
> Because reverting your commit fixes the issue for me. In fact, I was
> just starting to reverse engineer the differences between firmware
> versions 3 and 4, when I noticed a file called
> “Alps_Pointing-Device_Updater_amd64_1_4.exe”, and all the other binaries
> also saying only “ALPS” and not “Synaptics”, in the
> trackpoint_fw_updater_1.0.0.9.zip update package (for model 20HR). Are
> you actually certain that Gen5 X1s do always contain a Synaptics
> Trackpoint and not some models an ALPS one? Does changing the speed on
> your FW version 0x03 or 0x02 work at all?
>
> I’m suspecting that by changing the TP_MAGIC_IDENT to supposedly newer
> firmware versions you just made that driver wrongly recognize an ALPS
> trackpoint as whatever trackpoint.c is for.
>
> And I am, in fact, upset, since you don’t appear to be supporting the
> code you have written, even though it causes regressions. Then you ask
> me to install a kernel that includes a keylogger without any warning
> whatsoever and now “yeah, sysfs is barely used, just use GNOME”. Who
> else should I blame, please?
>
> Thanks,
> Sebastian
>
^ permalink raw reply
* Re: PROBLEM: Changing speed on ThinkPad X1 Carbon 5th trackpoint causes "failed to enable mouse"
From: Sebastian Schmidt @ 2017-12-30 14:40 UTC (permalink / raw)
To: Aaron Ma; +Cc: gregkh, dmitry.torokhov, linux-input
In-Reply-To: <d8770041-baf4-205e-7818-7ea477f69a29@canonical.com>
On Sat, Dec 30, 2017 at 10:17:55PM +0800, Aaron Ma wrote:
> Sorry, I don't know anything about the firmware software.
> So you can NOT expect me to fix the firmware issue.
You changed trackpoint.c in ec667683c532c93fb41e100e5d61a518971060e2 to,
according to the commit message, “support version 2 and 3”. Even though
I don’t understand the change, because according to the comment next to
TP_MAGIC_IDENT (and the code in trackpoint.c), the version is reported
in param[1], not in param[0]. Also it’s called “MAGIC_IDENT” and not
“SUPPORTED_FIRMWARE_VERSIONS”.
> I have helped answer all the question I can.
> I don't know why you blame me like this.
Because reverting your commit fixes the issue for me. In fact, I was
just starting to reverse engineer the differences between firmware
versions 3 and 4, when I noticed a file called
“Alps_Pointing-Device_Updater_amd64_1_4.exe”, and all the other binaries
also saying only “ALPS” and not “Synaptics”, in the
trackpoint_fw_updater_1.0.0.9.zip update package (for model 20HR). Are
you actually certain that Gen5 X1s do always contain a Synaptics
Trackpoint and not some models an ALPS one? Does changing the speed on
your FW version 0x03 or 0x02 work at all?
I’m suspecting that by changing the TP_MAGIC_IDENT to supposedly newer
firmware versions you just made that driver wrongly recognize an ALPS
trackpoint as whatever trackpoint.c is for.
And I am, in fact, upset, since you don’t appear to be supporting the
code you have written, even though it causes regressions. Then you ask
me to install a kernel that includes a keylogger without any warning
whatsoever and now “yeah, sysfs is barely used, just use GNOME”. Who
else should I blame, please?
Thanks,
Sebastian
^ permalink raw reply
* Re: PROBLEM: Changing speed on ThinkPad X1 Carbon 5th trackpoint causes "failed to enable mouse"
From: Aaron Ma @ 2017-12-30 14:17 UTC (permalink / raw)
To: Sebastian Schmidt, gregkh; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20171230141138.GA23284@marax.lan.yath.de>
Sorry, I don't know anything about the firmware software.
So you can NOT expect me to fix the firmware issue.
I have helped answer all the question I can.
I don't know why you blame me like this.
Regards,
Aaron
On 12/30/2017 10:11 PM, Sebastian Schmidt wrote:
> On Sat, Dec 30, 2017 at 09:54:39PM +0800, Aaron Ma wrote:
>> I believe it should be a firmware issue on trackpoint.
>
> I’ve just checked yesterday and updated the Windows Synaptics drivers to
> the newest version, but the reported firmware version (4) stays
> unchanged. Did you actually verify your change on firmware version 4 or
> only 2 and 3, as the commit message and the comment indicate?
>
>> There are several ways to set the speed & sensitivity:
>> 1, udev hwdb/rules;
>> 2, xorg conf;
>> 3, GUI;
>>
>> It seems users are rarely to use sysfs directly, so the bug is still there.
>
> Sorry. Are you saying “Oops, thanks for the report, I’ll fix it”
> or “I’ve just given you three ways to tell libinput my preferred
> acceleration, so sysfs is going to stay broken for firmware version
> 0x04”? I doubt that sysfs is “rarely” used and, mind you, not everyone
> is using GNOME and Wayland.
>
> Adding Greg KH since he’s asked for objections in
> <20170828080530.607134002@linuxfoundation.org>.
>
^ permalink raw reply
* Re: PROBLEM: Changing speed on ThinkPad X1 Carbon 5th trackpoint causes "failed to enable mouse"
From: Sebastian Schmidt @ 2017-12-30 14:11 UTC (permalink / raw)
To: Aaron Ma, gregkh; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <4ca11d3d-4e7c-5bef-1811-9bc95b6e901a@canonical.com>
On Sat, Dec 30, 2017 at 09:54:39PM +0800, Aaron Ma wrote:
> I believe it should be a firmware issue on trackpoint.
I’ve just checked yesterday and updated the Windows Synaptics drivers to
the newest version, but the reported firmware version (4) stays
unchanged. Did you actually verify your change on firmware version 4 or
only 2 and 3, as the commit message and the comment indicate?
> There are several ways to set the speed & sensitivity:
> 1, udev hwdb/rules;
> 2, xorg conf;
> 3, GUI;
>
> It seems users are rarely to use sysfs directly, so the bug is still there.
Sorry. Are you saying “Oops, thanks for the report, I’ll fix it”
or “I’ve just given you three ways to tell libinput my preferred
acceleration, so sysfs is going to stay broken for firmware version
0x04”? I doubt that sysfs is “rarely” used and, mind you, not everyone
is using GNOME and Wayland.
Adding Greg KH since he’s asked for objections in
<20170828080530.607134002@linuxfoundation.org>.
^ permalink raw reply
* Re: PROBLEM: Changing speed on ThinkPad X1 Carbon 5th trackpoint causes "failed to enable mouse"
From: Aaron Ma @ 2017-12-30 13:54 UTC (permalink / raw)
To: Sebastian Schmidt; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20171230095705.GA1861@marax.lan.yath.de>
I believe it should be a firmware issue on trackpoint.
There are several ways to set the speed & sensitivity:
1, udev hwdb/rules;
2, xorg conf;
3, GUI;
It seems users are rarely to use sysfs directly, so the bug is still there.
Regards,
Aaron
On 12/30/2017 05:57 PM, Sebastian Schmidt wrote:
> On Sat, Dec 30, 2017 at 02:43:16PM +0800, Aaron Ma wrote:
>> Please try:
>> $ xinput set-prop "TPPS/2 IBM TrackPoint" "libinput Accel Speed" -1
>>
>> The last value of speed can be -1 to 1.
>
> Ah. That indeed changes the speed, thanks!
>
> However, please let me ask again: Isn’t the sysfs interface supposed to
> work, too? I’ve got my problem resolved for Xorg now, but about every
> article on the internet refers to the sysfs interface for changing the
> speed, cf. <https://www.google.com/search?q=trackpoint+speed>.
>
> I’m also fine with the assertion that Lenovo shipped a broken firmware,
> but I’d expect some warning in dmesg then and a reasonable fallback (to
> a regular PS/2 mouse?) then. Can we at least agree that there is a
> driver bug that ought to be fixed or a firmware bug that ought to be
> worked around?
>
> Thanks,
> Sebastian
>
^ permalink raw reply
* dmesg asked me to email about touchpad
From: Leonardo Fontenelle @ 2017-12-30 12:33 UTC (permalink / raw)
To: linux-input
I got this on my dmesg:
[ 8.757452] psmouse serio4: synaptics: Your touchpad (PNP: SYN019e SYN0100 SYN0002 PNP0f13) says it can support a different bus. If i2c-hid and hid-rmi are not used, you might want to try setting psmouse.synaptics_intertouch to 1 and report this to linux-input@vger.kernel.org.
[ 8.824695] psmouse serio4: synaptics: Touchpad model: 1, fw: 8.1, id: 0x1e2b1, caps: 0xd00123/0x840300/0x27c00/0x0, board id: 2251, fw id: 1241283
[ 8.868370] input: SynPS/2 Synaptics TouchPad as /devices/platform/i8042/serio4/input/input24
After setting the boot parameter, dmesg says:
[ 8.484085] psmouse serio4: synaptics: queried max coordinates: x [..5660], y [..4742]
[ 8.521340] psmouse serio4: synaptics: queried min coordinates: x [1282..], y [1110..]
[ 8.521344] psmouse serio4: synaptics: Trying to set up SMBus access
[ 8.535868] rmi4_smbus 9-002c: registering SMbus-connected sensor
Both before and after setting psmouse.synaptics_intertouch, I had:
$ lsmod | fgrep hid
mac_hid 16384 0
Despite the name, this is an HP Folio 9470m running Arch Linux with Linux 4.14.9. The touchpad works fine either way.
I do not subscribe to the list, please cc me so that I can provide further necessary information.
Thanks for making things work.
Sincerely,
Leonardo Ferreira Fontenelle
^ permalink raw reply
* Re: PROBLEM: Changing speed on ThinkPad X1 Carbon 5th trackpoint causes "failed to enable mouse"
From: Sebastian Schmidt @ 2017-12-30 9:57 UTC (permalink / raw)
To: Aaron Ma; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <f8795209-1b95-81a5-5fbe-f3e3fdc0d18f@canonical.com>
On Sat, Dec 30, 2017 at 02:43:16PM +0800, Aaron Ma wrote:
> Please try:
> $ xinput set-prop "TPPS/2 IBM TrackPoint" "libinput Accel Speed" -1
>
> The last value of speed can be -1 to 1.
Ah. That indeed changes the speed, thanks!
However, please let me ask again: Isn’t the sysfs interface supposed to
work, too? I’ve got my problem resolved for Xorg now, but about every
article on the internet refers to the sysfs interface for changing the
speed, cf. <https://www.google.com/search?q=trackpoint+speed>.
I’m also fine with the assertion that Lenovo shipped a broken firmware,
but I’d expect some warning in dmesg then and a reasonable fallback (to
a regular PS/2 mouse?) then. Can we at least agree that there is a
driver bug that ought to be fixed or a firmware bug that ought to be
worked around?
Thanks,
Sebastian
^ permalink raw reply
* Re: PROBLEM: Changing speed on ThinkPad X1 Carbon 5th trackpoint causes "failed to enable mouse"
From: Aaron Ma @ 2017-12-30 6:43 UTC (permalink / raw)
To: Sebastian Schmidt; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20171229190529.GA2108@marax.lan.yath.de>
Please try:
$ xinput set-prop "TPPS/2 IBM TrackPoint" "libinput Accel Speed" -1
The last value of speed can be -1 to 1.
Regards,
Aaron
On 12/30/2017 03:05 AM, Sebastian Schmidt wrote:
> On Thu, Dec 28, 2017 at 11:53:44PM +0800, Aaron Ma wrote:
>> The set on sysfs "speed" will execute the following code:
>> 1, set #define PSMOUSE_CMD_DISABLE 0x00f5;
>> 2, set #define TP_SPEED 0x60 /* Speed of TP Cursor */
>> with the value in speed.
>> 3, set #define PSMOUSE_CMD_ENABLE 0x00f4
>>
>> When set 3rd step, it fails to write the cmd to ps2dev.
>> This issue should be related to trackpoint firmware that response on all
>> commands.
>
> Sorry, I’m not sure I can follow. Is this a bug in my Trackpoint
> firmware and I need to update it?
>
>> And for the trackpoint is too fast or sensitive, please change it in
>> setting of gnome (I assume you are using), the setting will use the
>> libinput(wayland) and xerver-input-synaptics driver to set the mouse
>> speed. It should be worked as expected.
>
> Are you saying the sysfs settings are not supposed to work? From a
> user’s perspective I’ve upgraded my kernel (to a stable version, into
> which that patch was merged), found my mouse too fast and a knob called
> “speed” in sysfs. Just touching that knob (by echoing the same value)
> broke my mouse entirely, and that’s the problem I’m reporting.
>
> I’m using i3 on “plain” Xorg, no Wayland or any desktop environment.
> synclient only appears to have options for the touchpad, as far as I can
> tell from the names:
> % synclient
> Parameter settings:
> LeftEdge = 77
> RightEdge = 1859
> TopEdge = 57
> BottomEdge = 1000
> FingerLow = 25
> FingerHigh = 30
> MaxTapTime = 180
> MaxTapMove = 97
> MaxDoubleTapTime = 180
> SingleTapTimeout = 180
> ClickTime = 100
> EmulateMidButtonTime = 0
> EmulateTwoFingerMinZ = 282
> EmulateTwoFingerMinW = 7
> VertScrollDelta = 44
> HorizScrollDelta = 44
> VertEdgeScroll = 0
> HorizEdgeScroll = 0
> CornerCoasting = 0
> VertTwoFingerScroll = 1
> HorizTwoFingerScroll = 0
> MinSpeed = 1
> MaxSpeed = 1.75
> AccelFactor = 0.090703
> TouchpadOff = 0
> LockedDrags = 0
> LockedDragTimeout = 5000
> RTCornerButton = 0
> RBCornerButton = 0
> LTCornerButton = 0
> LBCornerButton = 0
> TapButton1 = 0
> TapButton2 = 0
> TapButton3 = 0
> ClickFinger1 = 1
> ClickFinger2 = 3
> ClickFinger3 = 2
> CircularScrolling = 0
> CircScrollDelta = 0.1
> CircScrollTrigger = 0
> CircularPad = 0
> PalmDetect = 0
> PalmMinWidth = 10
> PalmMinZ = 200
> CoastingSpeed = 20
> CoastingFriction = 50
> PressureMotionMinZ = 30
> PressureMotionMaxZ = 160
> PressureMotionMinFactor = 1
> PressureMotionMaxFactor = 1
> GrabEventDevice = 0
> TapAndDragGesture = 1
> AreaLeftEdge = 0
> AreaRightEdge = 0
> AreaTopEdge = 0
> AreaBottomEdge = 0
> HorizHysteresis = 11
> VertHysteresis = 11
> ClickPad = 1
> RightButtonAreaLeft = 968
> RightButtonAreaRight = 0
> RightButtonAreaTop = 866
> RightButtonAreaBottom = 0
> MiddleButtonAreaLeft = 0
> MiddleButtonAreaRight = 0
> MiddleButtonAreaTop = 0
> MiddleButtonAreaBottom = 0
> %
>
> The xinput properties don’t show a (non-zero) speed either:
> % xinput list-props "TPPS/2 IBM TrackPoint"
> Device 'TPPS/2 IBM TrackPoint':
> Device Enabled (142): 1
> Coordinate Transformation Matrix (144): 1.000000, 0.000000, 0.000000, 0.000000, 1.000000, 0.000000, 0.000000, 0.000000, 1.000000
> libinput Natural Scrolling Enabled (326): 0
> libinput Natural Scrolling Enabled Default (327): 0
> libinput Left Handed Enabled (328): 0
> libinput Left Handed Enabled Default (329): 0
> libinput Accel Speed (330): 0.000000
> libinput Accel Speed Default (331): 0.000000
> libinput Accel Profiles Available (332): 1, 1
> libinput Accel Profile Enabled (333): 1, 0
> libinput Accel Profile Enabled Default (334): 1, 0
> libinput Scroll Methods Available (335): 0, 0, 1
> libinput Scroll Method Enabled (336): 0, 0, 1
> libinput Scroll Method Enabled Default (337): 0, 0, 1
> libinput Button Scrolling Button (338): 2
> libinput Button Scrolling Button Default (339): 2
> libinput Middle Emulation Enabled (340): 0
> libinput Middle Emulation Enabled Default (341): 0
> libinput Send Events Modes Available (265): 1, 0
> libinput Send Events Mode Enabled (266): 0, 0
> libinput Send Events Mode Enabled Default (267): 0, 0
> Device Node (268): "/dev/input/event17"
> Device Product ID (269): 2, 10
> libinput Drag Lock Buttons (342): <no items>
> libinput Horizontal Scroll Enabled (343): 1
> %
>
> Thanks,
> Sebastian
>
^ permalink raw reply
* [PATCH] Input: fix semicolon.cocci warnings
From: kbuild test robot @ 2017-12-30 3:15 UTC (permalink / raw)
Cc: kbuild-all, dmitry.torokhov, robh+dt, mark.rutland, linux,
maxime.ripard, wens, linux-arm-kernel, linux-input, devicetree,
linux-kernel, mylene.josserand, thomas.petazzoni, quentin.schulz
In-Reply-To: <20171228163336.28131-2-mylene.josserand@free-electrons.com>
From: Fengguang Wu <fengguang.wu@intel.com>
drivers/input/touchscreen/edt-ft5x06.c:1004:2-3: Unneeded semicolon
Remove unneeded semicolon.
Generated by: scripts/coccinelle/misc/semicolon.cocci
Fixes: 5969d946e8aa ("Input: edt-ft5x06 - Add support for regulator")
CC: Mylène Josserand <mylene.josserand@free-electrons.com>
Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
---
edt-ft5x06.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--- a/drivers/input/touchscreen/edt-ft5x06.c
+++ b/drivers/input/touchscreen/edt-ft5x06.c
@@ -1001,7 +1001,7 @@ static int edt_ft5x06_ts_probe(struct i2
dev_err(&client->dev, "failed to request regulator: %d\n",
error);
return error;
- };
+ }
if (tsdata->vcc) {
error = regulator_enable(tsdata->vcc);
^ permalink raw reply
* Re: [PATCH v2 1/2] Input: edt-ft5x06 - Add support for regulator
From: kbuild test robot @ 2017-12-30 3:15 UTC (permalink / raw)
Cc: kbuild-all, dmitry.torokhov, robh+dt, mark.rutland, linux,
maxime.ripard, wens, linux-arm-kernel, linux-input, devicetree,
linux-kernel, mylene.josserand, thomas.petazzoni, quentin.schulz
In-Reply-To: <20171228163336.28131-2-mylene.josserand@free-electrons.com>
Hi Mylène,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on robh/for-next]
[also build test WARNING on v4.15-rc5 next-20171222]
[cannot apply to input/next]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Myl-ne-Josserand/sun8i-a83t-Add-touchscreen-support-on-TBS-A711/20171230-091331
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
coccinelle warnings: (new ones prefixed by >>)
>> drivers/input/touchscreen/edt-ft5x06.c:1004:2-3: Unneeded semicolon
Please review and possibly fold the followup patch.
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
^ permalink raw reply
* RE: ATTENTION!!!
From: Loretta Robles @ 2017-12-30 0:28 UTC (permalink / raw)
To: Loretta Robles
In-Reply-To: <35D4F79A8B8138489F38DE5CF6200454E570015C@HCI-EX-MB2.hci.utah.edu>
________________________________
From: Loretta Robles
Sent: Friday, December 29, 2017 1:01 PM
To: Loretta Robles
Subject: ATTENTION!!!
You have been randomly selected for a donation. Contact soriz4040@gmail.com for claims.
^ permalink raw reply
* Re: PROBLEM: Changing speed on ThinkPad X1 Carbon 5th trackpoint causes "failed to enable mouse"
From: Sebastian Schmidt @ 2017-12-29 19:05 UTC (permalink / raw)
To: Aaron Ma; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <f1fb1612-2d16-25a4-b57e-c56a01b729c2@canonical.com>
On Thu, Dec 28, 2017 at 11:53:44PM +0800, Aaron Ma wrote:
> The set on sysfs "speed" will execute the following code:
> 1, set #define PSMOUSE_CMD_DISABLE 0x00f5;
> 2, set #define TP_SPEED 0x60 /* Speed of TP Cursor */
> with the value in speed.
> 3, set #define PSMOUSE_CMD_ENABLE 0x00f4
>
> When set 3rd step, it fails to write the cmd to ps2dev.
> This issue should be related to trackpoint firmware that response on all
> commands.
Sorry, I’m not sure I can follow. Is this a bug in my Trackpoint
firmware and I need to update it?
> And for the trackpoint is too fast or sensitive, please change it in
> setting of gnome (I assume you are using), the setting will use the
> libinput(wayland) and xerver-input-synaptics driver to set the mouse
> speed. It should be worked as expected.
Are you saying the sysfs settings are not supposed to work? From a
user’s perspective I’ve upgraded my kernel (to a stable version, into
which that patch was merged), found my mouse too fast and a knob called
“speed” in sysfs. Just touching that knob (by echoing the same value)
broke my mouse entirely, and that’s the problem I’m reporting.
I’m using i3 on “plain” Xorg, no Wayland or any desktop environment.
synclient only appears to have options for the touchpad, as far as I can
tell from the names:
% synclient
Parameter settings:
LeftEdge = 77
RightEdge = 1859
TopEdge = 57
BottomEdge = 1000
FingerLow = 25
FingerHigh = 30
MaxTapTime = 180
MaxTapMove = 97
MaxDoubleTapTime = 180
SingleTapTimeout = 180
ClickTime = 100
EmulateMidButtonTime = 0
EmulateTwoFingerMinZ = 282
EmulateTwoFingerMinW = 7
VertScrollDelta = 44
HorizScrollDelta = 44
VertEdgeScroll = 0
HorizEdgeScroll = 0
CornerCoasting = 0
VertTwoFingerScroll = 1
HorizTwoFingerScroll = 0
MinSpeed = 1
MaxSpeed = 1.75
AccelFactor = 0.090703
TouchpadOff = 0
LockedDrags = 0
LockedDragTimeout = 5000
RTCornerButton = 0
RBCornerButton = 0
LTCornerButton = 0
LBCornerButton = 0
TapButton1 = 0
TapButton2 = 0
TapButton3 = 0
ClickFinger1 = 1
ClickFinger2 = 3
ClickFinger3 = 2
CircularScrolling = 0
CircScrollDelta = 0.1
CircScrollTrigger = 0
CircularPad = 0
PalmDetect = 0
PalmMinWidth = 10
PalmMinZ = 200
CoastingSpeed = 20
CoastingFriction = 50
PressureMotionMinZ = 30
PressureMotionMaxZ = 160
PressureMotionMinFactor = 1
PressureMotionMaxFactor = 1
GrabEventDevice = 0
TapAndDragGesture = 1
AreaLeftEdge = 0
AreaRightEdge = 0
AreaTopEdge = 0
AreaBottomEdge = 0
HorizHysteresis = 11
VertHysteresis = 11
ClickPad = 1
RightButtonAreaLeft = 968
RightButtonAreaRight = 0
RightButtonAreaTop = 866
RightButtonAreaBottom = 0
MiddleButtonAreaLeft = 0
MiddleButtonAreaRight = 0
MiddleButtonAreaTop = 0
MiddleButtonAreaBottom = 0
%
The xinput properties don’t show a (non-zero) speed either:
% xinput list-props "TPPS/2 IBM TrackPoint"
Device 'TPPS/2 IBM TrackPoint':
Device Enabled (142): 1
Coordinate Transformation Matrix (144): 1.000000, 0.000000, 0.000000, 0.000000, 1.000000, 0.000000, 0.000000, 0.000000, 1.000000
libinput Natural Scrolling Enabled (326): 0
libinput Natural Scrolling Enabled Default (327): 0
libinput Left Handed Enabled (328): 0
libinput Left Handed Enabled Default (329): 0
libinput Accel Speed (330): 0.000000
libinput Accel Speed Default (331): 0.000000
libinput Accel Profiles Available (332): 1, 1
libinput Accel Profile Enabled (333): 1, 0
libinput Accel Profile Enabled Default (334): 1, 0
libinput Scroll Methods Available (335): 0, 0, 1
libinput Scroll Method Enabled (336): 0, 0, 1
libinput Scroll Method Enabled Default (337): 0, 0, 1
libinput Button Scrolling Button (338): 2
libinput Button Scrolling Button Default (339): 2
libinput Middle Emulation Enabled (340): 0
libinput Middle Emulation Enabled Default (341): 0
libinput Send Events Modes Available (265): 1, 0
libinput Send Events Mode Enabled (266): 0, 0
libinput Send Events Mode Enabled Default (267): 0, 0
Device Node (268): "/dev/input/event17"
Device Product ID (269): 2, 10
libinput Drag Lock Buttons (342): <no items>
libinput Horizontal Scroll Enabled (343): 1
%
Thanks,
Sebastian
^ permalink raw reply
* Re: [PATCH 03/14] dt-bindings: iio: add binding support for iio trigger provider/consumer
From: Jonathan Cameron @ 2017-12-29 17:24 UTC (permalink / raw)
To: Rob Herring
Cc: Eugen Hristev, nicolas.ferre, ludovic.desroches,
alexandre.belloni, linux-iio, linux-arm-kernel, devicetree,
linux-kernel, linux-input, dmitry.torokhov
In-Reply-To: <20171226223500.dxbp26bsx2ojbicr@rob-hp-laptop>
On Tue, 26 Dec 2017 16:35:00 -0600
Rob Herring <robh@kernel.org> wrote:
> On Fri, Dec 22, 2017 at 05:07:10PM +0200, Eugen Hristev wrote:
> > Add bindings for producer/consumer for iio triggers.
> >
> > Similar with iio channels, the iio triggers can be connected between drivers:
> > one driver will be a producer by registering iio triggers, and another driver
> > will connect as a consumer.
> >
> > Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
In cases where the connectivity is entirely known to the various drivers
(battery chargers integrated in SoCs that use ADC channels for example) we
have always kept the map in driver.
I'm not yet entirely clear if we can do this here. Might make sense if
we can...
> > ---
> > .../devicetree/bindings/iio/iio-bindings.txt | 52 +++++++++++++++++++++-
> > 1 file changed, 51 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/devicetree/bindings/iio/iio-bindings.txt b/Documentation/devicetree/bindings/iio/iio-bindings.txt
> > index 68d6f8c..d861f0df 100644
> > --- a/Documentation/devicetree/bindings/iio/iio-bindings.txt
> > +++ b/Documentation/devicetree/bindings/iio/iio-bindings.txt
> > @@ -11,6 +11,10 @@ value of a #io-channel-cells property in the IIO provider node.
> >
> > [1] http://marc.info/?l=linux-iio&m=135902119507483&w=2
> >
> > +Moreover, the provider can have a set of triggers that can be attached to
> > +from the consumer drivers.
> > +
> > +
> > ==IIO providers==
> >
> > Required properties:
> > @@ -18,6 +22,11 @@ Required properties:
> > with a single IIO output and 1 for nodes with multiple
> > IIO outputs.
> >
> > +Optional properties:
> > +#io-trigger-cells: Number of cells for the IIO trigger specifier. Typically 0
> > + for nodes with a single IIO trigger and 1 for nodes with
> > + multiple IIO triggers.
> > +
> > Example for a simple configuration with no trigger:
> >
> > adc: voltage-sensor@35 {
> > @@ -26,7 +35,7 @@ Example for a simple configuration with no trigger:
> > #io-channel-cells = <1>;
> > };
> >
> > -Example for a configuration with trigger:
> > +Example for a configuration with channels provided by trigger:
> >
> > adc@35 {
> > compatible = "some-vendor,some-adc";
> > @@ -42,6 +51,17 @@ Example for a configuration with trigger:
> > };
> > };
> >
> > +Example for a configuration for a trigger provider:
> > +
> > + adc: sensor-with-trigger@35 {
> > + compatible = "some-vendor,some-adc";
> > + reg = <0x35>;
> > + #io-channel-cells = <1>;
> > + #io-trigger-cells = <1>;
> > + /* other properties */
> > + };
> > +
> > +
> > ==IIO consumers==
> >
> > Required properties:
> > @@ -61,16 +81,38 @@ io-channel-ranges:
> > IIO channels from this node. Useful for bus nodes to provide
> > and IIO channel to their children.
> >
> > +io-triggers: List of phandle and IIO specifier pairs, one pair
> > + for each trigger input to the device. Note: if the
> > + IIO trigger provider specifies '0' for #io-trigger-cells,
> > + then only the phandle portion of the pair will appear.
> > +
> > +io-trigger-names:
> > + List of IIO trigger input name strings sorted in the same
> > + order as the io-triggers property. Consumers drivers
> > + will use io-trigger-names to match IIO trigger input names
> > + with IIO specifiers.
> > +
> > +io-trigger-ranges:
> > + Empty property indicating that child nodes can inherit named
> > + IIO triggers from this node. Useful for bus nodes to provide
> > + IIO triggers to their children.
>
> I think it would be better to be explicit in the child nodes. What's the
> use you had in mind?
>
> Rob
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" 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 07/14] iio: triggers: on pollfunc attach, complete iio_dev if NULL
From: Jonathan Cameron @ 2017-12-29 17:23 UTC (permalink / raw)
To: Eugen Hristev
Cc: nicolas.ferre, ludovic.desroches, alexandre.belloni, linux-iio,
linux-arm-kernel, devicetree, linux-kernel, linux-input,
dmitry.torokhov
In-Reply-To: <1513955241-10985-8-git-send-email-eugen.hristev@microchip.com>
On Fri, 22 Dec 2017 17:07:14 +0200
Eugen Hristev <eugen.hristev@microchip.com> wrote:
> When attaching a pollfunc to a trigger, if the pollfunc does not
> have an associated iio_dev pointer, just use the private data
> iio_dev pointer from the trigger to fill in the poll func required
> iio_dev reference.
>
> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
I'm yet to be convinced this is necessary rather than using a callback
buffer. It's also decidedly unsafe as there is no particular reason
in general to assume the private data is an iio_dev.
> ---
> drivers/iio/industrialio-trigger.c | 9 +++++++++
> include/linux/iio/trigger_consumer.h | 2 ++
> 2 files changed, 11 insertions(+)
>
> diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c
> index 8565c92..ab180bd 100644
> --- a/drivers/iio/industrialio-trigger.c
> +++ b/drivers/iio/industrialio-trigger.c
> @@ -272,6 +272,15 @@ int iio_trigger_attach_poll_func(struct iio_trigger *trig,
> bool notinuse
> = bitmap_empty(trig->pool, CONFIG_IIO_CONSUMERS_PER_TRIGGER);
>
> + /*
> + * If we did not get a iio_dev in the poll func, attempt to
> + * obtain the trigger's owner's device struct
> + */
> + if (!pf->indio_dev)
> + pf->indio_dev = iio_trigger_get_drvdata(trig);
That isn't always valid. Triggers don't always have a iio_dev associated
with them at all.
> + if (!pf->indio_dev)
> + return -EINVAL;
> +
> /* Prevent the module from being removed whilst attached to a trigger */
> __module_get(pf->indio_dev->driver_module);
>
> diff --git a/include/linux/iio/trigger_consumer.h b/include/linux/iio/trigger_consumer.h
> index aeefcdb..36e2a02 100644
> --- a/include/linux/iio/trigger_consumer.h
> +++ b/include/linux/iio/trigger_consumer.h
> @@ -63,6 +63,8 @@ int iio_triggered_buffer_predisable(struct iio_dev *indio_dev);
> /*
> * Two functions for the uncommon case when we need to attach or detach
> * a specific pollfunc to and from a trigger
> + * If the pollfunc has a NULL iio_dev pointer, it will be filled from the
> + * trigger struct.
> */
> int iio_trigger_attach_poll_func(struct iio_trigger *trig,
> struct iio_poll_func *pf);
^ permalink raw reply
* Re: [PATCH 09/14] iio: inkern: triggers: create helpers for OF trigger retrieval
From: Jonathan Cameron @ 2017-12-29 17:20 UTC (permalink / raw)
To: Eugen Hristev
Cc: nicolas.ferre-UWL1GkI3JZL3oGB3hsPCZA,
ludovic.desroches-UWL1GkI3JZL3oGB3hsPCZA,
alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-input-u79uwXL29TY76Z2rM5mHXA,
dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w
In-Reply-To: <1513955241-10985-10-git-send-email-eugen.hristev-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org>
On Fri, 22 Dec 2017 17:07:16 +0200
Eugen Hristev <eugen.hristev-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org> wrote:
> Create helper API to get trigger information from OF regarding
> trigger producer/consumer for iio triggers.
> The functions will search for matching trigger by name, similar
> with channel retrieval
>
> Signed-off-by: Eugen Hristev <eugen.hristev-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org>
This makes sense once we make the touchscreen driver truely generic.
If it wasn't generic this could be rolled up as data within that driver.
A few small comments inline.
> ---
> drivers/iio/inkern.c | 91 ++++++++++++++++++++++++++++++++++++
> include/linux/iio/trigger_consumer.h | 1 +
> 2 files changed, 92 insertions(+)
>
> diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> index 069defc..58bd18d 100644
> --- a/drivers/iio/inkern.c
> +++ b/drivers/iio/inkern.c
> @@ -14,9 +14,13 @@
>
> #include <linux/iio/iio.h>
> #include "iio_core.h"
> +#include "iio_core_trigger.h"
> #include <linux/iio/machine.h>
> #include <linux/iio/driver.h>
> #include <linux/iio/consumer.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/trigger_consumer.h>
> +
Don't add white space here.
>
> struct iio_map_internal {
> struct iio_dev *indio_dev;
> @@ -262,6 +266,39 @@ static struct iio_channel *of_iio_channel_get_all(struct device *dev)
> return ERR_PTR(ret);
> }
>
> +#ifdef CONFIG_IIO_TRIGGER
Hmm. Generally frowned upon to have ifdef blocks inline within c files.
Perhaps worth pulling these out to inkern-trigger.c and using Kconfig magic
to build if relevant - not worth splitting the header though.
> +
> +static struct iio_trigger *of_iio_trigger_get(struct device_node *np, int index)
> +{
> + struct device *idev;
> + struct iio_dev *indio_dev;
> + int err;
> + struct of_phandle_args iiospec;
> + struct iio_trigger *trig;
> +
> + err = of_parse_phandle_with_args(np, "io-triggers",
> + "#io-trigger-cells",
> + index, &iiospec);
> + if (err)
> + return ERR_PTR(err);
> +
> + idev = bus_find_device(&iio_bus_type, NULL, iiospec.np,
> + iio_dev_node_match);
> + of_node_put(iiospec.np);
> + if (!idev)
> + return ERR_PTR(-EPROBE_DEFER);
> +
> + indio_dev = dev_to_iio_dev(idev);
> +
> + trig = iio_trigger_find_from_device(indio_dev, iiospec.args[0]);
> +
> + if (!trig)
> + return ERR_PTR(-ENODEV);
> +
> + return trig;
> +}
> +#endif /* CONFIG_IIO_TRIGGER */
> +
> #else /* CONFIG_OF */
>
> static inline struct iio_channel *
> @@ -275,6 +312,12 @@ static inline struct iio_channel *of_iio_channel_get_all(struct device *dev)
> return NULL;
> }
>
> +static inline struct iio_trigger *of_iio_trigger_get(struct device_node *np,
> + int index)
> +{
> + return NULL;
> +}
> +
> #endif /* CONFIG_OF */
>
> static struct iio_channel *iio_channel_get_sys(const char *name,
> @@ -927,3 +970,51 @@ ssize_t iio_write_channel_ext_info(struct iio_channel *chan, const char *attr,
> chan->channel, buf, len);
> }
> EXPORT_SYMBOL_GPL(iio_write_channel_ext_info);
> +
> +#ifdef CONFIG_IIO_TRIGGER
> +struct iio_trigger *iio_trigger_find(struct device *dev, char *name)
> +{
> + struct device_node *np = dev->of_node;
> + struct iio_trigger *trig = NULL;
> +
> + /* Walk up the tree of devices looking for a matching iio trigger */
> + while (np) {
> + int index = 0;
> +
> + /*
> + * For named iio triggers, first look up the name in the
> + * "io-trigger-names" property. If it cannot be found, the
> + * index will be an error code, and of_iio_trigger_get()
> + * will fail.
> + */
> + if (name)
> + index = of_property_match_string(np, "io-trigger-names",
> + name);
> + trig = of_iio_trigger_get(np, index);
> + if (!IS_ERR(trig) || PTR_ERR(trig) == -EPROBE_DEFER) {
> + break;
> + } else if (name && index >= 0) {
> + pr_err("ERROR: could not get IIO trigger %pOF:%s(%i)\n",
> + np, name ? name : "", index);
> + return trig;
> + }
> +
> + /*
> + * No matching IIO trigger found on this node.
> + * If the parent node has a "io-trigger-ranges" property,
> + * then we can try one of its channels.
> + */
> + np = np->parent;
> + if (np && !of_get_property(np, "io-trigger-ranges", NULL))
> + return trig;
> + }
> +
> + if (!trig || (IS_ERR(trig) && PTR_ERR(trig) != -EPROBE_DEFER))
> + dev_dbg(dev, "error retrieving trigger information\n");
> + else
> + dev_dbg(dev, "trigger found: %s\n", name);
> +
> + return trig;
> +}
> +EXPORT_SYMBOL_GPL(iio_trigger_find);
> +#endif /* CONFIG_IIO_TRIGGER */
> diff --git a/include/linux/iio/trigger_consumer.h b/include/linux/iio/trigger_consumer.h
> index 13be595..b2fc485 100644
> --- a/include/linux/iio/trigger_consumer.h
> +++ b/include/linux/iio/trigger_consumer.h
> @@ -62,6 +62,7 @@ irqreturn_t iio_pollfunc_store_time(int irq, void *p);
>
> void iio_trigger_notify_done(struct iio_trigger *trig);
>
> +struct iio_trigger *iio_trigger_find(struct device *dev, char *name);
> /*
> * Two functions for common case where all that happens is a pollfunc
> * is attached and detached from a trigger
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 13/14] input: touchscreen: sama5d2_rts: SAMA5D2 Resistive touchscreen driver
From: Jonathan Cameron @ 2017-12-29 17:16 UTC (permalink / raw)
To: Eugen Hristev
Cc: nicolas.ferre, ludovic.desroches, alexandre.belloni, linux-iio,
linux-arm-kernel, devicetree, linux-kernel, linux-input,
dmitry.torokhov
In-Reply-To: <1513955241-10985-14-git-send-email-eugen.hristev@microchip.com>
On Fri, 22 Dec 2017 17:07:20 +0200
Eugen Hristev <eugen.hristev@microchip.com> wrote:
> This is the implementation of the Microchip SAMA5D2 SOC resistive
> touchscreen driver.
> The driver registers an input device and connects to the give IIO device
> from devicetree. It requires an IIO trigger (acting as a consumer) and
> three IIO channels : one for X position, one for Y position and one
> for pressure.
> It the reports the values to the input subsystem.
>
> Some parts of this driver are based on the initial original work by
> Mohamed Jamsheeth Hajanajubudeen and Bandaru Venkateswara Swamy
>
> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
I've suggested some difference in implementation, but this is very nearly
a generic resistive touch screen driver which is rather nice.
It does use somewhat magic trigger which is effectively a filtered
periodic trigger (only fires when touch has occurred) which would not
be particularly hard to implement in other resistive touch screen ADCs.
I'm not totally sure that is a strong requirement though - any periodic
trigger would work.
Anyhow, the big stuff to my mind is whether we can use the buffer_cb
code to do this. I originally wrote that for an accelerometer to input
bridge driver (which I never got around to finishing upstreaming). It's
existing usecases are rather esoteric but it should work here I think.
Jonathan
> ---
> drivers/input/touchscreen/Kconfig | 13 ++
> drivers/input/touchscreen/Makefile | 1 +
> drivers/input/touchscreen/sama5d2_rts.c | 287 ++++++++++++++++++++++++++++++++
> 3 files changed, 301 insertions(+)
> create mode 100644 drivers/input/touchscreen/sama5d2_rts.c
>
> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> index 64b30fe..db8f541 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -126,6 +126,19 @@ config TOUCHSCREEN_ATMEL_MXT_T37
> Say Y here if you want support to output data from the T37
> Diagnostic Data object using a V4L device.
>
> +config TOUCHSCREEN_SAMA5D2
> + tristate "Microchip SAMA5D2 resistive touchscreen support"
> + depends on ARCH_AT91
> + depends on AT91_SAMA5D2_ADC
> + help
> + Say Y here if you have 4-wire touchscreen connected
> + to ADC Controller on your SAMA5D2 Microchip SoC.
> +
> + If unsure, say N.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called sama5d2_rts.
> +
> config TOUCHSCREEN_AUO_PIXCIR
> tristate "AUO in-cell touchscreen using Pixcir ICs"
> depends on I2C
> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> index 850c156..9a2772e 100644
> --- a/drivers/input/touchscreen/Makefile
> +++ b/drivers/input/touchscreen/Makefile
> @@ -16,6 +16,7 @@ obj-$(CONFIG_TOUCHSCREEN_AD7879_SPI) += ad7879-spi.o
> obj-$(CONFIG_TOUCHSCREEN_ADS7846) += ads7846.o
> obj-$(CONFIG_TOUCHSCREEN_AR1021_I2C) += ar1021_i2c.o
> obj-$(CONFIG_TOUCHSCREEN_ATMEL_MXT) += atmel_mxt_ts.o
> +obj-$(CONFIG_TOUCHSCREEN_SAMA5D2) += sama5d2_rts.o
> obj-$(CONFIG_TOUCHSCREEN_AUO_PIXCIR) += auo-pixcir-ts.o
> obj-$(CONFIG_TOUCHSCREEN_BU21013) += bu21013_ts.o
> obj-$(CONFIG_TOUCHSCREEN_CHIPONE_ICN8318) += chipone_icn8318.o
> diff --git a/drivers/input/touchscreen/sama5d2_rts.c b/drivers/input/touchscreen/sama5d2_rts.c
> new file mode 100644
> index 0000000..e2ae413
> --- /dev/null
> +++ b/drivers/input/touchscreen/sama5d2_rts.c
> @@ -0,0 +1,287 @@
> +/*
> + * Microchip resistive touchscreen (RTS) driver for SAMA5D2.
> + *
> + * Copyright (C) 2017 Microchip Technology,
> + * Author: Eugen Hristev <eugen.hristev@microchip.com>
> + *
> + * SPDX-License-Identifier: GPL-2.0
> + */
> +#include <linux/input.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/iio/consumer.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/trigger_consumer.h>
> +
> +#define DRIVER_NAME "sama5d2_rts"
> +#define MAX_POS_MASK GENMASK(11, 0)
> +#define AT91_RTS_DEFAULT_PRESSURE_THRESHOLD 10000
> +
> +/**
> + * at91_rts - at91 resistive touchscreen information struct
> + * @input: the input device structure that we register
> + * @chan_x: X channel to IIO device to get position on X axis
> + * @chan_y: Y channel to IIO device to get position on Y axis
> + * @chan_pressure: pressure channel to IIO device to get pressure
> + * @trig: trigger to IIO device to register to for polling
> + * @rts_pf: pollfunc for the trigger to be called by IIO dev
> + * @pressure_threshold: number representing the threshold for the pressure
> + * @adc_connected: to know if adc device is connected
> + * @workq: to defer computations to this work queue for reporting
> + */
> +struct at91_rts {
> + struct input_dev *input;
> + struct iio_channel *chan_x, *chan_y, *chan_pressure;
> + struct iio_trigger *trig;
> + struct iio_poll_func *rts_pf;
> + u32 pressure_threshold;
> + bool adc_connected;
> + struct work_struct workq;
> +};
> +
> +static irqreturn_t at91_rts_trigger_handler(int irq, void *p)
> +{
> + struct at91_rts *st = ((struct iio_poll_func *)p)->p;
> +
> + schedule_work(&st->workq);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static void at91_rts_workq_handler(struct work_struct *workq)
> +{
> + struct at91_rts *st = container_of(workq, struct at91_rts, workq);
> + unsigned int x, y, press;
> + int ret;
> +
If we were instead to use a callback buffer this would all be
in the ADC driver as a simple 3 element channel scan.
> + /* read the channels, if all good, report touch */
> + ret = iio_read_channel_raw(st->chan_x, &x);
> + if (ret < 0)
> + goto at91_rts_workq_handler_end_touch;
> +
> + ret = iio_read_channel_raw(st->chan_y, &y);
> + if (ret < 0)
> + goto at91_rts_workq_handler_end_touch;
> +
> + ret = iio_read_channel_raw(st->chan_pressure, &press);
> + if (ret < 0)
> + goto at91_rts_workq_handler_end_touch;
> +
At this point our callback would be called and provided the above data.
> + /* if pressure too low, don't report */
> + if (press > st->pressure_threshold)
> + goto at91_rts_workq_handler_exit;
> +
> + input_report_abs(st->input, ABS_X, x);
> + input_report_abs(st->input, ABS_Y, y);
> + input_report_abs(st->input, ABS_PRESSURE, press);
> + input_report_key(st->input, BTN_TOUCH, 1);
> + input_sync(st->input);
> +
This would be back in the callback buffer code once the
callback has run.
Has the interesting side effect of making this code effectively generic.
It no longer has any ties to the at91 at all that I can spot.
Just a selection of channels and a request for a particular trigger -
both of which could come from device tree.
> + iio_trigger_notify_done(st->trig);
> + return;
> +
> +at91_rts_workq_handler_end_touch:
> + /* report end of touch */
> + input_report_key(st->input, BTN_TOUCH, 0);
> + input_sync(st->input);
> +at91_rts_workq_handler_exit:
> + iio_trigger_notify_done(st->trig);
> +}
> +
> +static int at91_rts_open(struct input_dev *dev)
> +{
> + int ret;
> + struct at91_rts *st = input_get_drvdata(dev);
> +
> + /* avoid multiple initialization in case touchscreen is opened again */
> + if (st->adc_connected)
> + return 0;
> +
> + /*
> + * First, look for the channels. It is possible that the ADC device
> + * did not probe yet, but we already probed, so we returning probe defer
> + * doesn't make much sense.
Quite - so why not do the obvious and request these in the probe and hold
them until remove? Then if they aren't ready defer the probe. This driver
is useless until the ADC is there so why let it successfully probe before
that point?
> + */
> + st->chan_x = iio_channel_get(dev->dev.parent, "x");
> + if (IS_ERR_OR_NULL(st->chan_x)) {
> + dev_err(dev->dev.parent, "cannot get X channel from ADC");
> + ret = PTR_ERR(st->chan_x);
> + goto at91_rts_open_free_chan;
> + }
> +
> + st->chan_y = iio_channel_get(dev->dev.parent, "y");
> + if (IS_ERR_OR_NULL(st->chan_y)) {
> + dev_err(dev->dev.parent, "cannot get Y channel from ADC");
> + ret = PTR_ERR(st->chan_y);
> + goto at91_rts_open_free_chan;
> + }
> +
> + st->chan_pressure = iio_channel_get(dev->dev.parent, "pressure");
> + if (IS_ERR_OR_NULL(st->chan_pressure)) {
> + dev_err(dev->dev.parent, "cannot get pressure channel from ADC");
> + ret = PTR_ERR(st->chan_pressure);
> + goto at91_rts_open_free_chan;
> + }
> +
> + /* look for the trigger in device tree */
> + st->trig = iio_trigger_find(dev->dev.parent, NULL);
> + if (IS_ERR_OR_NULL(st->trig)) {
> + dev_err(dev->dev.parent, "cannot get trigger from ADC");
> + ret = PTR_ERR(st->trig)
This also feels like it should be retrieved during the probe and we should
defer if that fails. Can't do anything useful without it!
> + goto at91_rts_open_free_chan;
> + }
> +
> + /* allocate a pollfunc for the trigger */
> + st->rts_pf = iio_alloc_pollfunc(at91_rts_trigger_handler, NULL,
> + IRQF_ONESHOT, NULL,
> + dev->dev.parent->of_node->name);
> + if (!st->rts_pf) {
> + ret = -ENOMEM;
> + dev_err(dev->dev.parent, "cannot allocate trigger pollfunc");
> + goto at91_rts_open_free_chan;
> + }
> +
> + iio_pollfunc_set_private_data(st->rts_pf, st);
> +
> + /*
> + * Attach the pollfunc to the trigger. This will also call the
> + * configure function to enable the trigger
> + */
> + ret = iio_trigger_attach_poll_func(st->trig, st->rts_pf);
> + if (ret)
> + goto at91_rts_open_dealloc_pf;
Ah. Now I think I see why you needed the poll function rather than
doing this with a callback buffer.
I'd rather see a consumer interface that requests the whole ADC runs on
a particular trigger.
> +
> + dev_dbg(dev->dev.parent, "channels found, attached to trigger");
> +
> + st->adc_connected = true;
> + return 0;
> +
> +at91_rts_open_dealloc_pf:
> + iio_dealloc_pollfunc(st->rts_pf);
> +at91_rts_open_free_chan:
> + if (!IS_ERR_OR_NULL(st->chan_x))
> + iio_channel_release(st->chan_x);
> + if (!IS_ERR_OR_NULL(st->chan_y))
> + iio_channel_release(st->chan_y);
> + if (!IS_ERR_OR_NULL(st->chan_pressure))
> + iio_channel_release(st->chan_pressure);
> + /*
> + * Avoid keeping old values in channel pointers. in case some channel
> + * failed and we reopen them, and now fail, we will have invalid values
> + * to release. So write them as NULL now.
> + */
> + st->chan_x = NULL;
> + st->chan_y = NULL;
> + st->chan_pressure = NULL;
> + return ret;
> +}
> +
> +static void at91_rts_close(struct input_dev *dev)
> +{
> + struct at91_rts *st = input_get_drvdata(dev);
> +
> + if (!st->adc_connected)
> + return;
> +
> + iio_trigger_detach_poll_func(st->trig, st->rts_pf);
> + iio_dealloc_pollfunc(st->rts_pf);
> +
> + if (!IS_ERR_OR_NULL(st->chan_x))
> + iio_channel_release(st->chan_x);
> + if (!IS_ERR_OR_NULL(st->chan_y))
> + iio_channel_release(st->chan_y);
> + if (!IS_ERR_OR_NULL(st->chan_pressure))
> + iio_channel_release(st->chan_pressure);
> +
> + st->adc_connected = false;
> +}
> +
> +static int at91_rts_probe(struct platform_device *pdev)
> +{
> + int ret;
> + struct at91_rts *st;
> + struct input_dev *input;
> + struct device *dev = &pdev->dev;
> + struct device_node *node = dev->of_node;
> +
> + st = devm_kzalloc(dev, sizeof(struct at91_rts), GFP_KERNEL);
> + if (!st)
> + return -ENOMEM;
> + st->adc_connected = false;
> +
> + INIT_WORK(&st->workq, at91_rts_workq_handler);
> +
> + input = devm_input_allocate_device(dev);
> + if (!input) {
> + dev_err(dev, "failed to allocate input device\n");
> + return -ENOMEM;
> + }
> +
> + ret = of_property_read_u32(node, "microchip,pressure-threshold",
> + &st->pressure_threshold);
> + if (ret < 0) {
> + dev_dbg(dev, "can't get touchscreen pressure threshold property.\n");
> + st->pressure_threshold = AT91_RTS_DEFAULT_PRESSURE_THRESHOLD;
> + }
> +
> + input->name = DRIVER_NAME;
> + input->id.bustype = BUS_HOST;
> + input->dev.parent = &pdev->dev;
> + input->open = at91_rts_open;
> + input->close = at91_rts_close;
> +
> + input_set_abs_params(input, ABS_X, 0, MAX_POS_MASK - 1, 0, 0);
> + input_set_abs_params(input, ABS_Y, 0, MAX_POS_MASK, 0, 0);
> + input_set_abs_params(input, ABS_PRESSURE, 0, 0xffffff, 0, 0);
> +
> + input->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_ABS);
> + input->keybit[BIT_WORD(BTN_TOUCH)] = BIT_MASK(BTN_TOUCH);
> +
> + st->input = input;
> + input_set_drvdata(input, st);
> +
> + ret = input_register_device(input);
> + if (ret) {
> + dev_err(dev, "failed to register input device: %d", ret);
> + return ret;
> + }
> +
> + platform_set_drvdata(pdev, st);
> +
> + dev_info(dev, "probed successfully\n");
Not useful. There are many ways to find this out without looking at dmesg.
Please remove.
> + return 0;
> +}
> +
> +static int at91_rts_remove(struct platform_device *pdev)
> +{
> + struct at91_rts *st = platform_get_drvdata(pdev);
> +
> + input_unregister_device(st->input);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id at91_rts_of_match[] = {
> + {
> + .compatible = "microchip,sama5d2-resistive-touch",
> + }, {
> + /* sentinel */
> + },
> +};
> +MODULE_DEVICE_TABLE(of, at91_rts_of_match);
> +
> +static struct platform_driver atmel_rts_driver = {
> + .probe = at91_rts_probe,
> + .remove = at91_rts_remove,
> + .driver = {
> + .name = DRIVER_NAME,
> + .of_match_table = of_match_ptr(at91_rts_of_match),
> + },
> +};
> +
> +module_platform_driver(atmel_rts_driver);
> +
> +MODULE_AUTHOR("Eugen Hristev <eugen.hristev@microchip.com>");
> +MODULE_DESCRIPTION("Microchip SAMA5D2 Resistive Touch Driver");
> +MODULE_LICENSE("GPL v2");
^ permalink raw reply
* Re: [PATCH 12/14] iio: adc: at91-sama5d2_adc: support for position and pressure channels
From: Jonathan Cameron @ 2017-12-29 17:02 UTC (permalink / raw)
To: Eugen Hristev
Cc: nicolas.ferre-UWL1GkI3JZL3oGB3hsPCZA,
ludovic.desroches-UWL1GkI3JZL3oGB3hsPCZA,
alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-input-u79uwXL29TY76Z2rM5mHXA,
dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w
In-Reply-To: <1513955241-10985-13-git-send-email-eugen.hristev-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org>
On Fri, 22 Dec 2017 17:07:19 +0200
Eugen Hristev <eugen.hristev-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org> wrote:
> The ADC IP supports position and pressure measurements for a touchpad
> connected on channels 0,1,2,3 for a 4-wire touchscreen with pressure
> measurement support.
> Using the inkern API, a driver can request a trigger and read the
> channel values from the ADC.
> The implementation provides a trigger named "touch" which can be
> connected to a consumer driver.
> Once a driver connects and attaches a pollfunc to this trigger, the
> configure trigger callback is called, and then the ADC driver will
> initialize pad measurement.
> First step is to enable touchscreen 4wire support and enable
> pen detect IRQ.
> Once a pen is detected, a periodic trigger is setup to trigger every
> 2 ms (e.g.) and sample the resistive touchscreen values. The trigger poll
> is called, and the consumer driver is then woke up, and it can read the
> respective channels for the values : X, and Y for position and pressure
> channel.
> Because only one trigger can be active in hardware in the same time,
> while touching the pad, the ADC will block any attempt to use the
> triggered buffer. Same, conversions using the software trigger are also
> impossible (since the periodic trigger is setup).
> If some driver wants to attach while the trigger is in use, it will
> also fail.
> Once the pen is not detected anymore, the trigger is free for use (hardware
> or software trigger, with or without DMA).
> Channels 0,1,2 and 3 are unavailable if a touchscreen is enabled.
>
> Some parts of this patch are based on initial original work by
> Mohamed Jamsheeth Hajanajubudeen and Bandaru Venkateswara Swamy
>
OK, so comments inline.
What I'm missing currently though is an explanation of why the slightly
more standard arrangement of using a callback buffer doesn't work here.
The only addition I think you need to do that is to allow a consumer to
request a particular trigger. I also think some of the other provisions
could be handled using standard features and slightly reducing the flexibility.
I don't know for example if it's useful to allow other channels to be
read when touch is not in progress or not.
So restrictions:
1. Touch screen channels can only be read when touch is enabled.
- use the available_scan_masks to control this. Or the callback that lets
you do the same dynamically.
2. You need to push these channels to your consumer driver.
- register a callback buffer rather than jumping through the hoops to
insert your own pollfunc. That will call a function in your
consumer, providing the data from the 3 channels directly.
3. You need to make sure it is using the right driver. For that you
will I think need a new interface.
Various other comments inline. I may well be missing something as this is
a fair bit of complex code to read - if so then next version should have
a clear cover letter describing why this more standard approach can't be
used.
> Signed-off-by: Eugen Hristev <eugen.hristev-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org>
> ---
> drivers/iio/adc/at91-sama5d2_adc.c | 455 ++++++++++++++++++++++++++++++++++++-
> 1 file changed, 446 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
> index 9610393..79eb197 100644
> --- a/drivers/iio/adc/at91-sama5d2_adc.c
> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
> @@ -102,14 +102,26 @@
> #define AT91_SAMA5D2_LCDR 0x20
> /* Interrupt Enable Register */
> #define AT91_SAMA5D2_IER 0x24
> +/* Interrupt Enable Register - TS X measurement ready */
> +#define AT91_SAMA5D2_IER_XRDY BIT(20)
> +/* Interrupt Enable Register - TS Y measurement ready */
> +#define AT91_SAMA5D2_IER_YRDY BIT(21)
> +/* Interrupt Enable Register - TS pressure measurement ready */
> +#define AT91_SAMA5D2_IER_PRDY BIT(22)
> /* Interrupt Enable Register - general overrun error */
> #define AT91_SAMA5D2_IER_GOVRE BIT(25)
> +/* Interrupt Enable Register - Pen detect */
> +#define AT91_SAMA5D2_IER_PEN BIT(29)
> +/* Interrupt Enable Register - No pen detect */
> +#define AT91_SAMA5D2_IER_NOPEN BIT(30)
> /* Interrupt Disable Register */
> #define AT91_SAMA5D2_IDR 0x28
> /* Interrupt Mask Register */
> #define AT91_SAMA5D2_IMR 0x2c
> /* Interrupt Status Register */
> #define AT91_SAMA5D2_ISR 0x30
> +/* Interrupt Status Register - Pen touching sense status */
> +#define AT91_SAMA5D2_ISR_PENS BIT(31)
> /* Last Channel Trigger Mode Register */
> #define AT91_SAMA5D2_LCTMR 0x34
> /* Last Channel Compare Window Register */
> @@ -131,8 +143,37 @@
> #define AT91_SAMA5D2_CDR0 0x50
> /* Analog Control Register */
> #define AT91_SAMA5D2_ACR 0x94
> +/* Analog Control Register - Pen detect sensitivity mask */
> +#define AT91_SAMA5D2_ACR_PENDETSENS_MASK GENMASK(0, 1)
> /* Touchscreen Mode Register */
> #define AT91_SAMA5D2_TSMR 0xb0
> +/* Touchscreen Mode Register - No touch mode */
> +#define AT91_SAMA5D2_TSMR_TSMODE_NONE 0
> +/* Touchscreen Mode Register - 4 wire screen, no pressure measurement */
> +#define AT91_SAMA5D2_TSMR_TSMODE_4WIRE_NO_PRESS 1
> +/* Touchscreen Mode Register - 4 wire screen, pressure measurement */
> +#define AT91_SAMA5D2_TSMR_TSMODE_4WIRE_PRESS 2
> +/* Touchscreen Mode Register - 5 wire screen */
> +#define AT91_SAMA5D2_TSMR_TSMODE_5WIRE 3
> +/* Touchscreen Mode Register - Average samples mask */
> +#define AT91_SAMA5D2_TSMR_TSAV_MASK (3 << 4)
> +/* Touchscreen Mode Register - Average samples */
> +#define AT91_SAMA5D2_TSMR_TSAV(x) ((x) << 4)
> +/* Touchscreen Mode Register - Touch/trigger frequency ratio mask */
> +#define AT91_SAMA5D2_TSMR_TSFREQ_MASK (0xf << 8)
> +/* Touchscreen Mode Register - Touch/trigger freqency ratio */
> +#define AT91_SAMA5D2_TSMR_TSFREQ(x) ((x) << 8)
> +/* Touchscreen Mode Register - Pen Debounce Time mask */
> +#define AT91_SAMA5D2_TSMR_PENDBC_MASK (0xf << 28)
> +/* Touchscreen Mode Register - Pen Debounce Time */
> +#define AT91_SAMA5D2_TSMR_PENDBC(x) ((x) << 28)
> +/* Touchscreen Mode Register - No DMA for touch measurements */
> +#define AT91_SAMA5D2_TSMR_NOTSDMA BIT(22)
> +/* Touchscreen Mode Register - Disable pen detection */
> +#define AT91_SAMA5D2_TSMR_PENDET_DIS (0 << 24)
> +/* Touchscreen Mode Register - Enable pen detection */
> +#define AT91_SAMA5D2_TSMR_PENDET_ENA BIT(24)
> +
> /* Touchscreen X Position Register */
> #define AT91_SAMA5D2_XPOSR 0xb4
> /* Touchscreen Y Position Register */
> @@ -151,7 +192,12 @@
> #define AT91_SAMA5D2_TRGR_TRGMOD_EXT_TRIG_FALL 2
> /* Trigger Mode external trigger any edge */
> #define AT91_SAMA5D2_TRGR_TRGMOD_EXT_TRIG_ANY 3
> -
> +/* Trigger Mode internal periodic */
> +#define AT91_SAMA5D2_TRGR_TRGMOD_PERIODIC 5
> +/* Trigger Mode - trigger period mask */
> +#define AT91_SAMA5D2_TRGR_TRGPER_MASK (0xffff << 16)
> +/* Trigger Mode - trigger period */
> +#define AT91_SAMA5D2_TRGR_TRGPER(x) ((x) << 16)
> /* Correction Select Register */
> #define AT91_SAMA5D2_COSR 0xd0
> /* Correction Value Register */
> @@ -169,6 +215,21 @@
> #define AT91_SAMA5D2_SINGLE_CHAN_CNT 12
> #define AT91_SAMA5D2_DIFF_CHAN_CNT 6
>
> +#define AT91_SAMA5D2_TIMESTAMP_CHAN_IDX (AT91_SAMA5D2_SINGLE_CHAN_CNT + \
> + AT91_SAMA5D2_DIFF_CHAN_CNT + 1)
> +
> +#define AT91_SAMA5D2_TOUCH_X_CHAN_IDX (AT91_SAMA5D2_TIMESTAMP_CHAN_IDX + 1)
> +#define AT91_SAMA5D2_TOUCH_Y_CHAN_IDX (AT91_SAMA5D2_TOUCH_X_CHAN_IDX + 1)
> +#define AT91_SAMA5D2_TOUCH_P_CHAN_IDX (AT91_SAMA5D2_TOUCH_Y_CHAN_IDX + 1)
> +
> +#define TOUCH_SAMPLE_PERIOD_US 2000 /* 2ms */
These all need the AT91_SAMA5D2 prefix.
> +#define TOUCH_PEN_DETECT_DEBOUNCE_US 200
> +
> +#define XYZ_MASK GENMASK(11, 0)
> +
> +#define MAX_POS_BITS 12
> +
> +#define AT91_ADC_TOUCH_TRIG_SHORTNAME "touch"
> /*
> * Maximum number of bytes to hold conversion from all channels
> * without the timestamp.
> @@ -222,6 +283,37 @@
> .indexed = 1, \
> }
>
> +#define AT91_SAMA5D2_CHAN_TOUCH(num, name, mod) \
> + { \
> + .type = IIO_POSITION, \
> + .modified = 1, \
> + .channel = num, \
> + .channel2 = mod, \
> + .scan_index = num, \
> + .scan_type = { \
> + .sign = 'u', \
> + .realbits = 12, \
> + .storagebits = 16, \
> + }, \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),\
> + .datasheet_name = name, \
> + }
> +#define AT91_SAMA5D2_CHAN_PRESSURE(num, name) \
> + { \
> + .type = IIO_PRESSURE, \
> + .channel = num, \
> + .scan_index = num, \
> + .scan_type = { \
> + .sign = 'u', \
> + .realbits = 12, \
> + .storagebits = 16, \
> + }, \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),\
> + .datasheet_name = name, \
> + }
> +
> #define at91_adc_readl(st, reg) readl_relaxed(st->base + reg)
> #define at91_adc_writel(st, reg, val) writel_relaxed(val, st->base + reg)
>
> @@ -239,6 +331,20 @@ struct at91_adc_trigger {
> };
>
> /**
> + * at91_adc_touch - at91-sama5d2 touchscreen information struct
> + * @trig: hold the start timestamp of dma operation
> + * @sample_period_val: the value for periodic trigger interval
> + * @touching: is the pen touching the screen or not
> + * @x_pos: temporary placeholder for pressure computation
> + */
> +struct at91_adc_touch {
> + struct iio_trigger *trig;
> + u16 sample_period_val;
> + bool touching;
> + u32 x_pos;
> +};
> +
> +/**
> * at91_adc_dma - at91-sama5d2 dma information struct
> * @dma_chan: the dma channel acquired
> * @rx_buf: dma coherent allocated area
> @@ -267,18 +373,22 @@ struct at91_adc_state {
> struct regulator *reg;
> struct regulator *vref;
> int vref_uv;
> + unsigned int current_sample_rate;
> struct iio_trigger *trig;
> const struct at91_adc_trigger *selected_trig;
> const struct iio_chan_spec *chan;
> bool conversion_done;
> u32 conversion_value;
> + bool touch_requested;
> struct at91_adc_soc_info soc_info;
> wait_queue_head_t wq_data_available;
> struct at91_adc_dma dma_st;
> + struct at91_adc_touch touch_st;
> u16 buffer[AT91_BUFFER_MAX_HWORDS];
> /*
> * lock to prevent concurrent 'single conversion' requests through
> - * sysfs.
> + * sysfs. Also protects when enabling or disabling touchscreen
> + * producer mode and checking if this mode is enabled or not.
> */
> struct mutex lock;
> };
> @@ -310,6 +420,7 @@ static const struct at91_adc_trigger at91_adc_trigger_list[] = {
> },
> };
>
> +/* channel order is not subject to change. inkern consumers rely on this */
> static const struct iio_chan_spec at91_adc_channels[] = {
> AT91_SAMA5D2_CHAN_SINGLE(0, 0x50),
> AT91_SAMA5D2_CHAN_SINGLE(1, 0x54),
> @@ -329,10 +440,103 @@ static const struct iio_chan_spec at91_adc_channels[] = {
> AT91_SAMA5D2_CHAN_DIFF(6, 7, 0x68),
> AT91_SAMA5D2_CHAN_DIFF(8, 9, 0x70),
> AT91_SAMA5D2_CHAN_DIFF(10, 11, 0x78),
> - IIO_CHAN_SOFT_TIMESTAMP(AT91_SAMA5D2_SINGLE_CHAN_CNT
> - + AT91_SAMA5D2_DIFF_CHAN_CNT + 1),
> + IIO_CHAN_SOFT_TIMESTAMP(AT91_SAMA5D2_TIMESTAMP_CHAN_IDX),
> + AT91_SAMA5D2_CHAN_TOUCH(AT91_SAMA5D2_TOUCH_X_CHAN_IDX, "x", IIO_MOD_X),
> + AT91_SAMA5D2_CHAN_TOUCH(AT91_SAMA5D2_TOUCH_Y_CHAN_IDX, "y", IIO_MOD_Y),
> + AT91_SAMA5D2_CHAN_PRESSURE(AT91_SAMA5D2_TOUCH_P_CHAN_IDX, "pressure"),
> };
>
> +static int at91_adc_configure_touch(struct at91_adc_state *st, bool state)
> +{
> + u32 clk_khz = st->current_sample_rate / 1000;
> + int i = 0;
> + u16 pendbc;
> + u32 tsmr, acr;
> +
> + if (!state) {
> + /* disabling touch IRQs and setting mode to no touch enabled */
> + at91_adc_writel(st, AT91_SAMA5D2_IDR,
> + AT91_SAMA5D2_IER_PEN | AT91_SAMA5D2_IER_NOPEN);
> + at91_adc_writel(st, AT91_SAMA5D2_TSMR, 0);
> + return 0;
> + }
> + /*
> + * debounce time is in microseconds, we need it in milliseconds to
> + * multiply with kilohertz, so, divide by 1000, but after the multiply.
> + * round up to make sure pendbc is at least 1
> + */
> + pendbc = round_up(TOUCH_PEN_DETECT_DEBOUNCE_US * clk_khz / 1000, 1);
> +
> + /* get the required exponent */
> + while (pendbc >> i++)
> + ;
This is related to the first 0? There are cleaner ways of doing this
with ffs and friends.
> +
> + pendbc = i;
> +
> + tsmr = AT91_SAMA5D2_TSMR_TSMODE_4WIRE_PRESS;
> +
> + tsmr |= AT91_SAMA5D2_TSMR_TSAV(1) & AT91_SAMA5D2_TSMR_TSAV_MASK;
> + tsmr |= AT91_SAMA5D2_TSMR_PENDBC(pendbc) &
> + AT91_SAMA5D2_TSMR_PENDBC_MASK;
> + tsmr |= AT91_SAMA5D2_TSMR_NOTSDMA;
> + tsmr |= AT91_SAMA5D2_TSMR_PENDET_ENA;
> + tsmr |= AT91_SAMA5D2_TSMR_TSFREQ(1) & AT91_SAMA5D2_TSMR_TSFREQ_MASK;
> +
> + at91_adc_writel(st, AT91_SAMA5D2_TSMR, tsmr);
> +
> + acr = at91_adc_readl(st, AT91_SAMA5D2_ACR);
> + acr &= ~AT91_SAMA5D2_ACR_PENDETSENS_MASK;
> + acr |= 0x02 & AT91_SAMA5D2_ACR_PENDETSENS_MASK;
> + at91_adc_writel(st, AT91_SAMA5D2_ACR, acr);
> +
> + /* Sample Period Time = (TRGPER + 1) / ADCClock */
> + st->touch_st.sample_period_val = round_up((TOUCH_SAMPLE_PERIOD_US *
> + clk_khz / 1000) - 1, 1);
> + /* enable pen detect IRQ */
> + at91_adc_writel(st, AT91_SAMA5D2_IER, AT91_SAMA5D2_IER_PEN);
> +
> + return 0;
> +}
> +
> +static int at91_adc_touch_trigger_validate_device(struct iio_trigger *trig,
> + struct iio_dev *indio_dev)
> +{
> + /* the touch trigger cannot be used with a buffer */
> + return -EBUSY;
> +}
> +
> +static int at91_adc_configure_touch_trigger(struct iio_trigger *trig,
> + bool state)
> +{
> + struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> + struct at91_adc_state *st = iio_priv(indio_dev);
> + int ret = 0;
> +
> + /*
> + * If we configure this with the IRQ enabled, the pen detected IRQ
> + * might fire before we finish setting all up, and the IRQ handler
> + * might misbehave. Better to reenable the IRQ after we are done
> + */
> + disable_irq_nosync(st->irq);
> +
> + mutex_lock(&st->lock);
> + if (state) {
> + ret = iio_buffer_enabled(indio_dev);
> + if (ret) {
> + dev_dbg(&indio_dev->dev, "trigger is currently in use\n");
> + ret = -EBUSY;
> + goto configure_touch_unlock_exit;
> + }
> + }
> + at91_adc_configure_touch(st, state);
> + st->touch_requested = state;
> +
> +configure_touch_unlock_exit:
> + enable_irq(st->irq);
> + mutex_unlock(&st->lock);
> + return ret;
> +}
> +
> static int at91_adc_configure_trigger(struct iio_trigger *trig, bool state)
> {
> struct iio_dev *indio = iio_trigger_get_drvdata(trig);
> @@ -390,12 +594,27 @@ static int at91_adc_reenable_trigger(struct iio_trigger *trig)
> return 0;
> }
>
> +static int at91_adc_reenable_touch_trigger(struct iio_trigger *trig)
> +{
> + struct iio_dev *indio = iio_trigger_get_drvdata(trig);
> + struct at91_adc_state *st = iio_priv(indio);
> +
> + enable_irq(st->irq);
> +
> + return 0;
> +}
> static const struct iio_trigger_ops at91_adc_trigger_ops = {
> .set_trigger_state = &at91_adc_configure_trigger,
> .try_reenable = &at91_adc_reenable_trigger,
> .validate_device = iio_trigger_validate_own_device,
> };
>
> +static const struct iio_trigger_ops at91_adc_touch_trigger_ops = {
> + .set_trigger_state = &at91_adc_configure_touch_trigger,
> + .try_reenable = &at91_adc_reenable_touch_trigger,
> + .validate_device = &at91_adc_touch_trigger_validate_device,
> +};
> +
> static int at91_adc_dma_size_done(struct at91_adc_state *st)
> {
> struct dma_tx_state state;
> @@ -490,6 +709,23 @@ static int at91_adc_dma_start(struct iio_dev *indio_dev)
> return 0;
> }
>
> +static int at91_adc_buffer_preenable(struct iio_dev *indio_dev)
> +{
> + struct at91_adc_state *st = iio_priv(indio_dev);
> + int ret;
> +
> + /* have to make sure nobody is requesting the trigger right now */
This needs some more explanation as I don't totally follow what this
is designed to protect against.
Realistically a device is only useful if it has one trigger at a time
feeding a valid set of channels to however many consumers (whether
in the driver or not).
> + mutex_lock(&st->lock);
> + ret = st->touch_requested;
> + mutex_unlock(&st->lock);
> +
> + /*
> + * if the trigger is used by the touchscreen,
> + * we must return an error
> + */
> + return ret ? -EBUSY : 0;
> +}
> +
> static int at91_adc_buffer_postenable(struct iio_dev *indio_dev)
> {
> int ret;
> @@ -538,6 +774,7 @@ static int at91_adc_buffer_predisable(struct iio_dev *indio_dev)
> }
>
> static const struct iio_buffer_setup_ops at91_buffer_setup_ops = {
> + .preenable = &at91_adc_buffer_preenable,
> .postenable = &at91_adc_buffer_postenable,
> .predisable = &at91_adc_buffer_predisable,
> };
> @@ -555,7 +792,11 @@ static struct iio_trigger *at91_adc_allocate_trigger(struct iio_dev *indio,
>
> trig->dev.parent = indio->dev.parent;
> iio_trigger_set_drvdata(trig, indio);
> - trig->ops = &at91_adc_trigger_ops;
> +
> + if (strcmp(trigger_name, AT91_ADC_TOUCH_TRIG_SHORTNAME))
Pass this is as a parameter to the function and avoid the strcmp nastiness.
> + trig->ops = &at91_adc_trigger_ops;
> + else
> + trig->ops = &at91_adc_touch_trigger_ops;
>
> ret = devm_iio_trigger_register(&indio->dev, trig);
> if (ret)
> @@ -571,7 +812,16 @@ static int at91_adc_trigger_init(struct iio_dev *indio)
> st->trig = at91_adc_allocate_trigger(indio, st->selected_trig->name);
> if (IS_ERR(st->trig)) {
> dev_err(&indio->dev,
> - "could not allocate trigger\n");
> + "could not allocate trigger %s\n",
> + st->selected_trig->name);
> + return PTR_ERR(st->trig);
> + }
> +
> + st->touch_st.trig = at91_adc_allocate_trigger(indio,
> + AT91_ADC_TOUCH_TRIG_SHORTNAME);
> + if (IS_ERR(st->trig)) {
> + dev_err(&indio->dev, "could not allocate trigger"
> + AT91_ADC_TOUCH_TRIG_SHORTNAME "\n");
> return PTR_ERR(st->trig);
> }
>
> @@ -703,6 +953,8 @@ static void at91_adc_setup_samp_freq(struct at91_adc_state *st, unsigned freq)
>
> dev_dbg(&indio_dev->dev, "freq: %u, startup: %u, prescal: %u\n",
> freq, startup, prescal);
> +
> + st->current_sample_rate = freq;
> }
>
> static unsigned at91_adc_get_sample_freq(struct at91_adc_state *st)
> @@ -718,23 +970,77 @@ static unsigned at91_adc_get_sample_freq(struct at91_adc_state *st)
> return f_adc;
> }
>
> +static irqreturn_t at91_adc_pen_detect_interrupt(struct at91_adc_state *st)
> +{
> + at91_adc_writel(st, AT91_SAMA5D2_IDR, AT91_SAMA5D2_IER_PEN);
> + at91_adc_writel(st, AT91_SAMA5D2_IER, AT91_SAMA5D2_IER_NOPEN |
> + AT91_SAMA5D2_IER_XRDY | AT91_SAMA5D2_IER_YRDY |
> + AT91_SAMA5D2_IER_PRDY);
> + at91_adc_writel(st, AT91_SAMA5D2_TRGR,
> + AT91_SAMA5D2_TRGR_TRGMOD_PERIODIC |
> + AT91_SAMA5D2_TRGR_TRGPER(st->touch_st.sample_period_val));
> + st->touch_st.touching = true;
> +
> + return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t at91_adc_no_pen_detect_interrupt(struct at91_adc_state *st)
> +{
> + at91_adc_writel(st, AT91_SAMA5D2_TRGR, 0);
> + at91_adc_writel(st, AT91_SAMA5D2_IDR, AT91_SAMA5D2_IER_NOPEN |
> + AT91_SAMA5D2_IER_XRDY | AT91_SAMA5D2_IER_YRDY |
> + AT91_SAMA5D2_IER_PRDY);
> + st->touch_st.touching = false;
Hmm. I think we are unfortunately racing here. There is nothing preventing
this running concurrently with the read_raw calls that check the same variable.
If this is fine (because we will always get valid data anyway (if stale)
then a comment is needed to explain that.
> +
> + disable_irq_nosync(st->irq);
> + iio_trigger_poll(st->touch_st.trig);
Comment to explain why a poll here is fine, but not on the pen on would be
good (I can guess but better to state it!)
> +
> + at91_adc_writel(st, AT91_SAMA5D2_IER, AT91_SAMA5D2_IER_PEN);
> +
> + return IRQ_HANDLED;
> +}
> +
> static irqreturn_t at91_adc_interrupt(int irq, void *private)
> {
> struct iio_dev *indio = private;
> struct at91_adc_state *st = iio_priv(indio);
> u32 status = at91_adc_readl(st, AT91_SAMA5D2_ISR);
> u32 imr = at91_adc_readl(st, AT91_SAMA5D2_IMR);
> + u32 rdy_mask = AT91_SAMA5D2_IER_XRDY | AT91_SAMA5D2_IER_YRDY |
> + AT91_SAMA5D2_IER_PRDY;
>
> if (!(status & imr))
> return IRQ_NONE;
>
> - if (iio_buffer_enabled(indio) && !st->dma_st.dma_chan) {
> + if (st->touch_requested && (status & AT91_SAMA5D2_IER_PEN)) {
> + /* pen detected IRQ */
> + return at91_adc_pen_detect_interrupt(st);
> + } else if (st->touch_requested && (status & AT91_SAMA5D2_IER_NOPEN)) {
> + /* nopen detected IRQ */
> + return at91_adc_no_pen_detect_interrupt(st);
> + } else if (st->touch_requested && (status & AT91_SAMA5D2_ISR_PENS) &&
> + ((status & rdy_mask) == rdy_mask)) {
> + /* periodic trigger IRQ - during pen sense */
> + disable_irq_nosync(irq);
> + iio_trigger_poll(st->touch_st.trig);
> + } else if ((st->touch_requested && (status & AT91_SAMA5D2_ISR_PENS))) {
> + /*
> + * touching, but the measurements are not ready yet.
> + * read and ignore.
> + */
> + status = at91_adc_readl(st, AT91_SAMA5D2_XPOSR);
> + status = at91_adc_readl(st, AT91_SAMA5D2_YPOSR);
> + status = at91_adc_readl(st, AT91_SAMA5D2_PRESSR);
> + } else if (iio_buffer_enabled(indio) && !st->dma_st.dma_chan) {
> + /* buffered trigger without DMA */
> disable_irq_nosync(irq);
> iio_trigger_poll(indio->trig);
> } else if (iio_buffer_enabled(indio) && st->dma_st.dma_chan) {
> + /* buffered trigger with DMA - should not happen */
> disable_irq_nosync(irq);
> WARN(true, "Unexpected irq occurred\n");
> } else if (!iio_buffer_enabled(indio)) {
> + /* software requested conversion */
> st->conversion_value = at91_adc_readl(st, st->chan->address);
> st->conversion_done = true;
> wake_up_interruptible(&st->wq_data_available);
> @@ -742,6 +1048,96 @@ static irqreturn_t at91_adc_interrupt(int irq, void *private)
> return IRQ_HANDLED;
> }
>
> +static u32 at91_adc_touch_x_pos(struct at91_adc_state *st)
> +{
> + u32 xscale, val;
> + u32 x, xpos;
> +
> + /* x position = (x / xscale) * max, max = 2^MAX_POS_BITS - 1 */
> + val = at91_adc_readl(st, AT91_SAMA5D2_XPOSR);
> + if (!val)
> + dev_dbg(&iio_priv_to_dev(st)->dev, "x_pos is 0\n");
> +
> + xpos = val & XYZ_MASK;
> + x = (xpos << MAX_POS_BITS) - xpos;
> + xscale = (val >> 16) & XYZ_MASK;
> + if (xscale == 0) {
> + dev_err(&iio_priv_to_dev(st)->dev, "xscale is 0\n");
> + return 0;
> + }
> + x /= xscale;
> + st->touch_st.x_pos = x;
> +
> + return x;
> +}
> +
> +static u32 at91_adc_touch_y_pos(struct at91_adc_state *st)
> +{
> + u32 yscale, val;
> + u32 y, ypos;
> +
> + /* y position = (y / yscale) * max, max = 2^MAX_POS_BITS - 1 */
> + val = at91_adc_readl(st, AT91_SAMA5D2_YPOSR);
> + ypos = val & XYZ_MASK;
> + y = (ypos << MAX_POS_BITS) - ypos;
> + yscale = (val >> 16) & XYZ_MASK;
> +
> + if (yscale == 0)
> + return 0;
> +
> + y /= yscale;
> +
> + return y;
> +}
> +
> +static u32 at91_adc_touch_pressure(struct at91_adc_state *st)
> +{
> + u32 val, z1, z2;
> + u32 pres;
> + u32 rxp = 1;
> + u32 factor = 1000;
> +
> + /* calculate the pressure */
> + val = at91_adc_readl(st, AT91_SAMA5D2_PRESSR);
> + z1 = val & XYZ_MASK;
XYZ_MASK seems oddly named given what this seems to be doing...
> + z2 = (val >> 16) & XYZ_MASK;
> +
> + if (z1 != 0)
> + pres = rxp * (st->touch_st.x_pos * factor / 1024) *
> + (z2 * factor / z1 - factor) /
> + factor;
> + else
> + pres = 0xFFFFFFFF; /* no pen contact */
> +
> + return pres;
> +}
> +
> +static int at91_adc_read_position(struct at91_adc_state *st, int chan, int *val)
> +{
> + if (!st->touch_st.touching)
> + return -ENODATA;
> + if (chan == AT91_SAMA5D2_TOUCH_X_CHAN_IDX)
> + *val = at91_adc_touch_x_pos(st);
> + else if (chan == AT91_SAMA5D2_TOUCH_Y_CHAN_IDX)
> + *val = at91_adc_touch_y_pos(st);
> + else
> + return -ENODATA;
> +
> + return IIO_VAL_INT;
> +}
> +
> +static int at91_adc_read_pressure(struct at91_adc_state *st, int chan, int *val)
> +{
> + if (!st->touch_st.touching)
> + return -ENODATA;
> + if (chan == AT91_SAMA5D2_TOUCH_P_CHAN_IDX)
General code flow simpler if you check error first
if (chan != AT91_SAMA5D2_TOUCH_P_CHAN_IDX)
return -ENODATA;
*val =...
> + *val = at91_adc_touch_pressure(st);
> + else
> + return -ENODATA;
> +
> + return IIO_VAL_INT;
> +}
> +
> static int at91_adc_read_raw(struct iio_dev *indio_dev,
> struct iio_chan_spec const *chan,
> int *val, int *val2, long mask)
> @@ -752,11 +1148,38 @@ static int at91_adc_read_raw(struct iio_dev *indio_dev,
>
> switch (mask) {
> case IIO_CHAN_INFO_RAW:
> + mutex_lock(&st->lock);
> +
> + if (chan->type == IIO_POSITION) {
Switch or else if as only one is true at a time.
Hmm. So you allow sysfs reads of these channels if touch in progress.
Do we actually have a use for this? Seems we could have simpler code
by just not providing direct reads for them if not.
> + ret = at91_adc_read_position(st, chan->channel, val);
> + mutex_unlock(&st->lock);
> + return ret;
> + }
> + if (chan->type == IIO_PRESSURE) {
> + ret = at91_adc_read_pressure(st, chan->channel, val);
> + mutex_unlock(&st->lock);
> + return ret;
> + }
> + /* if we using touch, channels 0, 1, 2, 3 are unavailable */
> + if (st->touch_requested && chan->channel <= 3) {
> + mutex_unlock(&st->lock);
> + return -EBUSY;
> + }
> + /*
> + * if we have the periodic trigger set up, we can't use
> + * software trigger either.
> + */
> + if (st->touch_st.touching) {
> + mutex_unlock(&st->lock);
> + return -ENODATA;
> + }
> +
> /* we cannot use software trigger if hw trigger enabled */
> ret = iio_device_claim_direct_mode(indio_dev);
> - if (ret)
> + if (ret) {
> + mutex_unlock(&st->lock);
> return ret;
> - mutex_lock(&st->lock);
> + }
>
> st->chan = chan;
>
> @@ -785,6 +1208,11 @@ static int at91_adc_read_raw(struct iio_dev *indio_dev,
>
> at91_adc_writel(st, AT91_SAMA5D2_IDR, BIT(chan->channel));
> at91_adc_writel(st, AT91_SAMA5D2_CHDR, BIT(chan->channel));
> + /*
> + * It is possible that after this conversion, we reuse these
> + * channels for the touchscreen. So, reset the COR now.
> + */
> + at91_adc_writel(st, AT91_SAMA5D2_COR, 0);
>
> /* Needed to ACK the DRDY interruption */
> at91_adc_readl(st, AT91_SAMA5D2_LCDR);
> @@ -1180,6 +1608,10 @@ static int at91_adc_remove(struct platform_device *pdev)
> struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> struct at91_adc_state *st = iio_priv(indio_dev);
>
> + mutex_lock(&st->lock);
> + devm_iio_trigger_unregister(&indio_dev->dev, st->touch_st.trig);
As before this needs detailed explanation. It should not be necessary.
> + mutex_unlock(&st->lock);
> +
> if (st->selected_trig->hw_trig)
> devm_iio_trigger_unregister(&indio_dev->dev, st->trig);
>
> @@ -1245,6 +1677,11 @@ static __maybe_unused int at91_adc_resume(struct device *dev)
> if (iio_buffer_enabled(indio_dev))
> at91_adc_configure_trigger(st->trig, true);
>
> + mutex_lock(&st->lock);
> + if (st->touch_requested)
> + at91_adc_configure_touch_trigger(st->touch_st.trig, true);
> + mutex_unlock(&st->lock);
> +
> return 0;
>
> vref_disable_resume:
^ permalink raw reply
* Re: [PATCH 11/14] iio: adc: at91-sama5d2_adc: optimize scan index for diff channels
From: Jonathan Cameron @ 2017-12-29 16:24 UTC (permalink / raw)
To: Eugen Hristev
Cc: nicolas.ferre-UWL1GkI3JZL3oGB3hsPCZA,
ludovic.desroches-UWL1GkI3JZL3oGB3hsPCZA,
alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-input-u79uwXL29TY76Z2rM5mHXA,
dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w
In-Reply-To: <1513955241-10985-12-git-send-email-eugen.hristev-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org>
On Fri, 22 Dec 2017 17:07:18 +0200
Eugen Hristev <eugen.hristev-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org> wrote:
> Optimize the scan index for the differential channels. Before, it
> was single channel count + index of the first single channel
> number of the differential pair. (e.g. 11+0, +2, +4, etc.)
> Divide that number by two (since it's always even), and add it up
> as a scan index to have consecutive numbered channels in the
> index.
Why? This is odd as it stands, but that isn't a strong enough reason
to fix it.
This is making a userspace ABI change. We need a very strong
argument for why it is necessary and also why existing userspace
won't care.
>
> Signed-off-by: Eugen Hristev <eugen.hristev-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org>
> ---
> drivers/iio/adc/at91-sama5d2_adc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
> index 7b9febc..9610393 100644
> --- a/drivers/iio/adc/at91-sama5d2_adc.c
> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
> @@ -209,7 +209,7 @@
> .channel = num, \
> .channel2 = num2, \
> .address = addr, \
> - .scan_index = num + AT91_SAMA5D2_SINGLE_CHAN_CNT, \
> + .scan_index = (num >> 1) + AT91_SAMA5D2_SINGLE_CHAN_CNT,\
> .scan_type = { \
> .sign = 's', \
> .realbits = 12, \
^ permalink raw reply
* Re: [PATCH 10/14] iio: adc: at91-sama5d2_adc: force trigger removal on module remove
From: Jonathan Cameron @ 2017-12-29 16:22 UTC (permalink / raw)
To: Eugen Hristev
Cc: nicolas.ferre-UWL1GkI3JZL3oGB3hsPCZA,
ludovic.desroches-UWL1GkI3JZL3oGB3hsPCZA,
alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-input-u79uwXL29TY76Z2rM5mHXA,
dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w
In-Reply-To: <1513955241-10985-11-git-send-email-eugen.hristev-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org>
On Fri, 22 Dec 2017 17:07:17 +0200
Eugen Hristev <eugen.hristev-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org> wrote:
> On module remove, if we do not call trigger remove, the trigger
> stays in the subsystem, and on further module insert, we will have
> multiple triggers, and the old one is not usable.
> Have to call the remove function on module remove to solve this.
>
> Signed-off-by: Eugen Hristev <eugen.hristev-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org>
This needs more explanation. I can't see why the managed removal
isn't sufficient. On removal the dev should have gone away taking
the trigger with it.
If it isn't then it looks like a straight forward bug that needs fixing.
Jonathan
> ---
> drivers/iio/adc/at91-sama5d2_adc.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
> index 4eff835..7b9febc 100644
> --- a/drivers/iio/adc/at91-sama5d2_adc.c
> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
> @@ -1180,6 +1180,9 @@ static int at91_adc_remove(struct platform_device *pdev)
> struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> struct at91_adc_state *st = iio_priv(indio_dev);
>
> + if (st->selected_trig->hw_trig)
> + devm_iio_trigger_unregister(&indio_dev->dev, st->trig);
> +
> iio_device_unregister(indio_dev);
>
> at91_adc_dma_disable(pdev);
^ permalink raw reply
* Re: [PATCH 02/14] iio: Add channel for Position
From: Jonathan Cameron @ 2017-12-29 16:09 UTC (permalink / raw)
To: Eugen Hristev
Cc: nicolas.ferre-UWL1GkI3JZL3oGB3hsPCZA,
ludovic.desroches-UWL1GkI3JZL3oGB3hsPCZA,
alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-input-u79uwXL29TY76Z2rM5mHXA,
dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w
In-Reply-To: <1513955241-10985-3-git-send-email-eugen.hristev-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org>
On Fri, 22 Dec 2017 17:07:09 +0200
Eugen Hristev <eugen.hristev-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org> wrote:
> Add new channel type for position on a pad.
>
> These type of analog sensor represents the position of a pen
> on a touchpad, and is represented as a voltage, which can be
> converted to a position on X and Y axis on the pad.
>
> The channel can then be consumed by a touchscreen driver or
> read as-is for a raw indication of the touchpen on a touchpad.
>
> Signed-off-by: Eugen Hristev <eugen.hristev-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org>
> ---
> Documentation/ABI/testing/sysfs-bus-iio | 11 +++++++++++
> drivers/iio/industrialio-core.c | 1 +
> include/uapi/linux/iio/types.h | 1 +
> tools/iio/iio_event_monitor.c | 2 ++
> 4 files changed, 15 insertions(+)
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> index a478740..d2b9e2f 100644
> --- a/Documentation/ABI/testing/sysfs-bus-iio
> +++ b/Documentation/ABI/testing/sysfs-bus-iio
> @@ -190,6 +190,17 @@ Description:
> but should match other such assignments on device).
> Units after application of scale and offset are m/s^2.
>
> +What: /sys/bus/iio/devices/iio:deviceX/in_position_x_raw
> +What: /sys/bus/iio/devices/iio:deviceX/in_position_y_raw
> +KernelVersion: 4.16
> +Contact: linux-iio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> +Description:
> + Position in direction x or y on a pad (may be arbitrarily
> + assigned but should match other such assignments on device).
> + Units after application of scale and offset are millipercents
> + from the pad's size in both directions. Should be calibrated by
> + the consumer.
Hmm. The units are an issues as to be consistent with the existing ABI position
should be in meters. Perhaps the trick is to do similar to we have done for
relative humidity and call this in_positionrelative_x_raw etc.
That leaves position open for absolute position devices (who knows what)
in the future.
> +
> What: /sys/bus/iio/devices/iio:deviceX/in_anglvel_x_raw
> What: /sys/bus/iio/devices/iio:deviceX/in_anglvel_y_raw
> What: /sys/bus/iio/devices/iio:deviceX/in_anglvel_z_raw
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index 2e8e36f..a4fa49b 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -85,6 +85,7 @@ static const char * const iio_chan_type_name_spec[] = {
> [IIO_COUNT] = "count",
> [IIO_INDEX] = "index",
> [IIO_GRAVITY] = "gravity",
> + [IIO_POSITION] = "position",
> };
>
> static const char * const iio_modifier_names[] = {
> diff --git a/include/uapi/linux/iio/types.h b/include/uapi/linux/iio/types.h
> index 4213cdf..35e17da 100644
> --- a/include/uapi/linux/iio/types.h
> +++ b/include/uapi/linux/iio/types.h
> @@ -44,6 +44,7 @@ enum iio_chan_type {
> IIO_COUNT,
> IIO_INDEX,
> IIO_GRAVITY,
> + IIO_POSITION,
> };
>
> enum iio_modifier {
> diff --git a/tools/iio/iio_event_monitor.c b/tools/iio/iio_event_monitor.c
> index b61245e..0c2b317 100644
> --- a/tools/iio/iio_event_monitor.c
> +++ b/tools/iio/iio_event_monitor.c
> @@ -58,6 +58,7 @@ static const char * const iio_chan_type_name_spec[] = {
> [IIO_PH] = "ph",
> [IIO_UVINDEX] = "uvindex",
> [IIO_GRAVITY] = "gravity",
> + [IIO_POSITION] = "position",
> };
>
> static const char * const iio_ev_type_text[] = {
> @@ -151,6 +152,7 @@ static bool event_is_known(struct iio_event_data *event)
> case IIO_PH:
> case IIO_UVINDEX:
> case IIO_GRAVITY:
> + case IIO_POSITION:
> break;
> default:
> return false;
^ permalink raw reply
* Re: [PATCH v2 0/2] sun8i-a83t: Add touchscreen support on TBS A711
From: Mylene JOSSERAND @ 2017-12-28 16:46 UTC (permalink / raw)
To: dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
linux-I+IVW8TIWO2tmTQ+vhA3Yw,
maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8, wens-jdAy2FN1RRM
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-input-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8,
quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8
In-Reply-To: <20171228163336.28131-1-mylene.josserand-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
Hello,
Le Thu, 28 Dec 2017 17:33:34 +0100,
Mylène Josserand <mylene.josserand-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> a écrit :
> Hello everyone,
>
> This is a V2 of the patch series that adds touchscreen support
> (FocalTech EDT-FT5x06 Polytouch) for TBS A711 (Allwinner sun8i-a83t SoC).
> Based on last linux-next (next-20171222).
>
> Changes since v1:
> - Remove patches 01 and 02 as Chen-Yu Tsai sent a similar patch:
> https://patchwork.kernel.org/patch/10111431/
> and it is merged on last next-20171222.
> (See commit f066f46ce5a5 "ARM: dts: sun8i: a83t: Add I2C device nodes and pinmux settings")
> - Update regulator according to Dmitry Torokhov's review: remove "optional"
> suffix while retrieving the regulator, rename it into "vcc" instead of
> "power" and add bindings documentation.
I notice that I forgot the second review of Dmitry about reset/wake
gpios so I will send a V3 with the modifications.
Thanks,
Mylène
> - Update device tree according to Maxime Ripard's review: remove the
> label and rename the node.
> - Squash patch 03 with patch 05 to add I2C0 and touchscreen's node
> in one patch (see patch 02).
>
> Patch 01: Add support for regulator in the FocalTech touchscreen driver
> because A711 tablet is using a regulator to power-up the touchscreen.
> Patch 02: Add i2c0 and touchscreen's node for A711 TBS tablet.
>
> Thank you in advance for any review.
> Best regards,
> Mylène
>
> Mylène Josserand (2):
> Input: edt-ft5x06 - Add support for regulator
> arm: dts: sun8i: a83t: a711: Add touchscreen node
>
> .../bindings/input/touchscreen/edt-ft5x06.txt | 1 +
> arch/arm/boot/dts/sun8i-a83t-tbs-a711.dts | 16 +++++++++++
> drivers/input/touchscreen/edt-ft5x06.c | 33 ++++++++++++++++++++++
> 3 files changed, 50 insertions(+)
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH v2 2/2] arm: dts: sun8i: a83t: a711: Add touchscreen node
From: Mylène Josserand @ 2017-12-28 16:33 UTC (permalink / raw)
To: dmitry.torokhov, robh+dt, mark.rutland, linux, maxime.ripard,
wens
Cc: thomas.petazzoni, devicetree, linux-kernel, quentin.schulz,
linux-input, mylene.josserand, linux-arm-kernel
In-Reply-To: <20171228163336.28131-1-mylene.josserand@free-electrons.com>
Tha A711 tablet has a FocalTech EDT-FT5x06 Polytouch touchscreen.
It is connected via I2C0. The reset line is PD5, the interrupt
line is PL7 and the VCC supply is the ldo_io0 regulator.
Signed-off-by: Mylène Josserand <mylene.josserand@free-electrons.com>
---
arch/arm/boot/dts/sun8i-a83t-tbs-a711.dts | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/arch/arm/boot/dts/sun8i-a83t-tbs-a711.dts b/arch/arm/boot/dts/sun8i-a83t-tbs-a711.dts
index a021ee6da396..7840f9aa9094 100644
--- a/arch/arm/boot/dts/sun8i-a83t-tbs-a711.dts
+++ b/arch/arm/boot/dts/sun8i-a83t-tbs-a711.dts
@@ -105,6 +105,22 @@
status = "okay";
};
+&i2c0 {
+ clock-frequency = <400000>;
+ status = "okay";
+
+ touchscreen@38 {
+ compatible = "edt,edt-ft5x06";
+ reg = <0x38>;
+ interrupt-parent = <&r_pio>;
+ interrupts = <0 7 IRQ_TYPE_EDGE_FALLING>;
+ reset-gpios = <&pio 3 5 GPIO_ACTIVE_LOW>;
+ vcc-supply = <®_ldo_io0>;
+ touchscreen-size-x = <1024>;
+ touchscreen-size-y = <600>;
+ };
+};
+
&mmc0 {
vmmc-supply = <®_dcdc1>;
pinctrl-names = "default";
--
2.11.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
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