Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1582757 > unrolled thread

[PATCH v2 0/3] Bind RMI4 over SMBus from PS/2

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 0/3] Bind RMI4 over SMBus from PS/2 Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2017-02-16 19:00 +0100
    [PATCH v2 2/3] Input: synaptics - allocate a Synaptics Intertouch device Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2017-02-16 19:00 +0100
      Re: [PATCH v2 2/3] Input: synaptics - allocate a Synaptics  Intertouch device kbuild test robot <lkp@intel.com> - 2017-02-16 22:50 +0100
      [PATCH] Input: fix ptr_ret.cocci warnings kbuild test robot <lkp@intel.com> - 2017-02-16 22:50 +0100
    [PATCH v2 4/3] Input: ps2smbus - force PS/2 disable before SMBus gets resumed Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2017-02-17 12:50 +0100

#1582757 — [PATCH v2 0/3] Bind RMI4 over SMBus from PS/2

Hi Dmitry,

quoting your last email about this topic:

> I think that by trying to "unwind" unsuccessful SMbus initialization you
> make the code much more complicated and fragile. I think we should
> select a path (PS/2 or SMbus) and commit to it. If we commit to SMBus
> then we need to communicate that fact to psmouse core so that it does
> not create input devices or psmouse attributes, and "short-circuit" the
> reconnect() routines to simply ignore requests and always report
> success.

I have tried in the past to unwind the psmouse input devices, and it was a pain.
So I thought of using a dummy serio driver that basically just calls
PSMOUSE_CMD_DISABLE at connect and returns 0 everywhere else.

I tried this in the past without much luck but I think I found a reliable
way today.

The good thing is that it seems that in that case, the rmi4_smbus driver
doesn't need any changes now that most serio states are ignored.

The new ps2smbus driver gets a little bit more complex, especially because
of kseriod. We need to wait for the .connect() of the serio driver to end
and have a stable PS/2 connection before starting the smbus work.

From the tests I made today, it seems reliable: cold boot & suspend/resume
works, various "rescan" with drvctl with different synaptics_intertouch
parameter state work too.

I hope you'll find this solution acceptable.

Cheers,
Benjamin

Benjamin Tissoires (3):
  input: serio - allow others to specify a driver for a serio device
  Input: synaptics - allocate a Synaptics Intertouch device
  Input: add a PS/2 to SMBus platform module

 drivers/input/misc/Kconfig      |  11 ++
 drivers/input/misc/Makefile     |   1 +
 drivers/input/misc/ps2_smbus.c  | 382 ++++++++++++++++++++++++++++++++++++++++
 drivers/input/mouse/synaptics.c | 106 +++++++++++
 drivers/input/mouse/synaptics.h |   1 +
 drivers/input/rmi4/Kconfig      |   1 +
 drivers/input/serio/serio.c     |  20 +++
 include/linux/serio.h           |   5 +
 8 files changed, 527 insertions(+)
 create mode 100644 drivers/input/misc/ps2_smbus.c

-- 
2.9.3

[toc] | [next] | [standalone]

#1582761 — [PATCH v2 2/3] Input: synaptics - allocate a Synaptics Intertouch device

Most of the Synaptics devices are connected through PS/2 and a different
bus (SMBus or HID over I2C).
The secondary bus capability is indicated by the InterTouch bit in
extended capability 0x0C.

When we encounter such a device, we can create a platform device with
the information gathered through the PS/2 enumeration as some information
might be missing through the other bus. Using a platform device allows
to not add any dependency on the psmouse driver.

We only enable the InterTouch device to be created for the laptops
registered with the top software button property or those we know
that are functional.

In the future, we might change the default to always rely on the
InterTouch bus. Currently, users can enable/disable the feature
with the psmouse parameter synaptics_intertouch.

The SMBus devices keep their PS/2 connection alive. If the initialization
process goes too far (psmouse_activate called), the device disconnects
from the I2C bus and stays on the PS/2 bus. We need to be sure the psmouse
driver will stop communicating with the device (and the pass-through
trackstick too). This part is not addressed here but will be in a
following patch.

The HID over I2C devices are enumerated through the ACPI DSDT, and
their PS/2 device also exports the InterTouch bit in the extended
capability 0x0C. However, the firmware keeps its I2C connection open
even after going further in the PS/2 initialization. We don't need
to take extra precautions with those device, especially because they
block their PS/2 communication when HID over I2C is used.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

---

changes in v2:
- do not store the platform device as psmouse will be disconnected
  to be replaced by a dummy serio driver.
---
 drivers/input/mouse/synaptics.c | 106 ++++++++++++++++++++++++++++++++++++++++
 drivers/input/mouse/synaptics.h |   1 +
 2 files changed, 107 insertions(+)

diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index 597ee4b..c71a500 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -29,6 +29,8 @@
 #include <linux/input/mt.h>
 #include <linux/serio.h>
 #include <linux/libps2.h>
+#include <linux/platform_device.h>
+#include <linux/rmi.h>
 #include <linux/slab.h>
 #include "psmouse.h"
 #include "synaptics.h"
@@ -70,6 +72,21 @@
 /* maximum ABS_MT_POSITION displacement (in mm) */
 #define DMAX 10
 
+/*
+ * The newest Synaptics device can use a secondary bus (called InterTouch) which
+ * provides a better bandwidth and allow a better control of the touchpads.
+ * This is used to decide if we need to use this bus or not.
+ */
+enum {
+	SYNAPTICS_INTERTOUCH_NOT_SET = -1,
+	SYNAPTICS_INTERTOUCH_OFF,
+	SYNAPTICS_INTERTOUCH_ON,
+};
+
+static int synaptics_intertouch = SYNAPTICS_INTERTOUCH_NOT_SET;
+module_param_named(synaptics_intertouch, synaptics_intertouch, int, 0644);
+MODULE_PARM_DESC(synaptics_intertouch, "Use a secondary bus for the Synaptics device.");
+
 /*****************************************************************************
  *	Stuff we need even when we do not want native Synaptics support
  ****************************************************************************/
@@ -218,6 +235,92 @@ static const char * const forcepad_pnp_ids[] = {
 	NULL
 };
 
+static const char * const smbus_pnp_ids[] = {
+	/* all of the topbuttonpad_pnp_ids are valid, we just add some extras */
+	"LEN0048", /* X1 Carbon 3 */
+	"LEN0046", /* X250 */
+	"LEN004a", /* W541 */
+	"LEN200f", /* T450s */
+};
+
+static int rmi4_id;
+
+static int synaptics_create_intertouch(struct psmouse *psmouse)
+{
+	struct synaptics_data *priv = psmouse->private;
+	struct platform_device_info pdevinfo;
+	struct platform_device *pdev;
+	struct rmi_device_platform_data pdata = {
+		.sensor_pdata = {
+			.sensor_type = rmi_sensor_touchpad,
+			.axis_align.flip_y = true,
+			/* to prevent cursors jumps: */
+			.kernel_tracking = true,
+		},
+		.f30_data = {
+			.buttonpad = SYN_CAP_CLICKPAD(priv->ext_cap_0c),
+			.trackstick_buttons =
+			  !!SYN_CAP_EXT_BUTTONS_STICK(priv->ext_cap_10),
+		},
+	};
+
+	pdata.sensor_pdata.topbuttonpad =
+			psmouse_matches_pnp_id(psmouse, topbuttonpad_pnp_ids) &&
+			!SYN_CAP_EXT_BUTTONS_STICK(priv->ext_cap_10);
+
+	memset(&pdevinfo, 0, sizeof(pdevinfo));
+	pdevinfo.name = "rmi4";
+	pdevinfo.id = rmi4_id++;
+	pdevinfo.data = &pdata;
+	pdevinfo.size_data = sizeof(pdata);
+	pdevinfo.parent = &psmouse->ps2dev.serio->dev;
+
+	pdev = platform_device_register_full(&pdevinfo);
+	if (IS_ERR(pdev))
+		return PTR_ERR(pdev);
+
+	return 0;
+}
+
+static int synaptics_remove_intertouch_device(struct device *dev, void *data)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+
+	if (strncmp("rmi4", dev_name(dev), 4))
+		return 0;
+
+	platform_device_unregister(pdev);
+
+	return 0;
+}
+
+/**
+ * synaptics_setup_intertouch - called once the PS/2 devices are enumerated
+ * and decides to instantiate a SMBus InterTouch device.
+ */
+static void synaptics_setup_intertouch(struct psmouse *psmouse)
+{
+	int ret;
+
+	/* first remove any remnant platform intertouch devices */
+	bus_for_each_dev(&platform_bus_type, NULL, NULL,
+			 synaptics_remove_intertouch_device);
+
+	if (synaptics_intertouch == SYNAPTICS_INTERTOUCH_OFF)
+		return;
+
+	if (synaptics_intertouch == SYNAPTICS_INTERTOUCH_NOT_SET) {
+		if (!psmouse_matches_pnp_id(psmouse, topbuttonpad_pnp_ids) &&
+		    !psmouse_matches_pnp_id(psmouse, smbus_pnp_ids))
+			return;
+	}
+
+	psmouse_info(psmouse, "device also supported by an other bus.\n");
+	ret = synaptics_create_intertouch(psmouse);
+	if (ret)
+		psmouse_info(psmouse,
+			     "unable to create intertouch device.\n");
+}
 /*****************************************************************************
  *	Synaptics communications functions
  ****************************************************************************/
@@ -1546,6 +1649,9 @@ static int __synaptics_init(struct psmouse *psmouse, bool absolute_mode)
 		}
 	}
 
+	if (SYN_CAP_INTERTOUCH(priv->ext_cap_0c))
+		synaptics_setup_intertouch(psmouse);
+
 	return 0;
 
  init_fail:
diff --git a/drivers/input/mouse/synaptics.h b/drivers/input/mouse/synaptics.h
index 116ae25..ccbd3c5 100644
--- a/drivers/input/mouse/synaptics.h
+++ b/drivers/input/mouse/synaptics.h
@@ -90,6 +90,7 @@
 #define SYN_CAP_ADV_GESTURE(ex0c)	((ex0c) & 0x080000)
 #define SYN_CAP_REDUCED_FILTERING(ex0c)	((ex0c) & 0x000400)
 #define SYN_CAP_IMAGE_SENSOR(ex0c)	((ex0c) & 0x000800)
+#define SYN_CAP_INTERTOUCH(ex0c)	((ex0c) & 0x004000)
 
 /*
  * The following descibes response for the 0x10 query.
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1582958 — Re: [PATCH v2 2/3] Input: synaptics - allocate a Synaptics Intertouch device

Hi Benjamin,

[auto build test WARNING on input/next]
[also build test WARNING on v4.10-rc8 next-20170216]
[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/Benjamin-Tissoires/Bind-RMI4-over-SMBus-from-PS-2/20170217-043234
base:   https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git next


coccinelle warnings: (new ones prefixed by >>)

>> drivers/input/mouse/synaptics.c:279:1-3: WARNING: PTR_ERR_OR_ZERO can be used

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

[toc] | [prev] | [next] | [standalone]

#1582961 — [PATCH] Input: fix ptr_ret.cocci warnings

drivers/input/mouse/synaptics.c:279:1-3: WARNING: PTR_ERR_OR_ZERO can be used


 Use PTR_ERR_OR_ZERO rather than if(IS_ERR(...)) + PTR_ERR

Generated by: scripts/coccinelle/api/ptr_ret.cocci

CC: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
---

 synaptics.c |    5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -276,10 +276,7 @@ static int synaptics_create_intertouch(s
 	pdevinfo.parent = &psmouse->ps2dev.serio->dev;
 
 	pdev = platform_device_register_full(&pdevinfo);
-	if (IS_ERR(pdev))
-		return PTR_ERR(pdev);
-
-	return 0;
+	return PTR_ERR_OR_ZERO(pdev);
 }
 
 static int synaptics_remove_intertouch_device(struct device *dev, void *data)

[toc] | [prev] | [next] | [standalone]

#1583363 — [PATCH v2 4/3] Input: ps2smbus - force PS/2 disable before SMBus gets resumed

On some cases, the touchpad can be reset during resume. We need to
send the PS/2 command PSMOUSE_CMD_DISABLE before attempting to contact
the touchpad over SMBus. Given that the .connect() callback is called
in a separate thread in kseriod, we need to wait for it in the main
thread before leaving the resume of the platform device.

From what I can see, the I2C client is then blocked until the platform
device gets resumed, even if the I2C client is not a child of the platform
device.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---

Hi Dmitry,

this morning the touchpad was dead after the resume. So we need to
actually be sure the PS/2 state is disabled before attempting to
use the SMBus connection.

I am not 100% sure the I2C client will be waiting for the platform
device to be resumed given that I can't find a way to mark the I2C
as a child of the other one. However, it seems that the ordering
is correct nevertheless.

Cheers,
Benjamin


new in v2

 drivers/input/misc/ps2_smbus.c | 42 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/drivers/input/misc/ps2_smbus.c b/drivers/input/misc/ps2_smbus.c
index b58c113..0b03224 100644
--- a/drivers/input/misc/ps2_smbus.c
+++ b/drivers/input/misc/ps2_smbus.c
@@ -49,6 +49,7 @@ struct ps2smbus_work {
 
 struct ps2smbus_serio {
 	struct ps2dev ps2dev;
+	bool suspended;
 };
 
 static struct serio_device_id ps2smbus_serio_ids[] = {
@@ -121,8 +122,26 @@ static int ps2smbus_connect(struct serio *serio, struct serio_driver *drv)
 	return error;
 }
 
+static void ps2smbus_cleanup(struct serio *serio)
+{
+	struct ps2smbus_serio *ps2smbus = serio_get_drvdata(serio);
+
+	ps2smbus->suspended = true;
+}
+
 static int ps2smbus_reconnect(struct serio *serio)
 {
+	struct ps2smbus_serio *ps2smbus = serio_get_drvdata(serio);
+	int error;
+
+	error = ps2_command(&ps2smbus->ps2dev, NULL, PSMOUSE_CMD_DISABLE);
+	if (error)
+		dev_warn(&serio->dev, "Failed to deactivate PS/2 mouse on %s\n",
+			 serio->phys);
+
+	ps2smbus->suspended = false;
+	wake_up_interruptible(&ps2smbus_serio_wait);
+
 	return 0;
 }
 
@@ -144,6 +163,7 @@ static struct serio_driver ps2smbus_serio_drv = {
 	.id_table	= ps2smbus_serio_ids,
 	.interrupt	= ps2smbus_interrupt,
 	.connect	= ps2smbus_connect,
+	.cleanup	= ps2smbus_cleanup,
 	.reconnect	= ps2smbus_reconnect,
 	.disconnect	= ps2smbus_disconnect,
 	.manual_bind	= true,
@@ -328,6 +348,27 @@ static int ps2smbus_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static int __maybe_unused ps2smbus_resume(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct ps2smbus *ps2smbus = platform_get_drvdata(pdev);
+	struct serio *serio = ps2smbus->serio;
+	struct ps2smbus_serio *ps2smbus_serio = serio_get_drvdata(serio);
+	int error;
+
+	error = wait_event_interruptible_timeout(ps2smbus_serio_wait,
+				ps2smbus_serio->suspended == false,
+				msecs_to_jiffies(1000));
+	if (error <= 10)
+		dev_warn(&serio->dev,
+			 "error while waiting for the PS/2 node to be ready: %d\n",
+			 error);
+
+	return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(ps2smbus_pm_ops, NULL, ps2smbus_resume);
+
 static const struct platform_device_id ps2smbus_id_table[] = {
 	{ .name = "rmi4", .driver_data = PS2SMBUS_SYNAPTICS_RMI4 },
 	{ }
@@ -337,6 +378,7 @@ MODULE_DEVICE_TABLE(platform, ps2smbus_id_table);
 static struct platform_driver ps2smbus_drv = {
 	.driver		= {
 		.name	= "ps2smbus",
+		.pm	= &ps2smbus_pm_ops,
 	},
 	.probe		= ps2smbus_probe,
 	.remove		= ps2smbus_remove,
-- 
2.9.3

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web