[PATCH 1/4] input: of_touchscreen: Preserve flags passed into touchscreen_parse_properties

touchscreen_parse_properties preserves the exisiting max and fuzz
values for axis if not specified as a device_property.

But it would set invert_x / invert_y / swap_x_y to false when
not specified as a device_property, rather then preserving them,
this is not consistent.

All current users of touchscreen_parse_properties pass in a kzalloc-ed
struct touchscreen_properties (or NULL), so preserving the existing
value for these flags preserves existing behavior.

Allowing a caller of touchscreen_parse_properties to set one of these
flags beforehand is useful on ACPI based tablets, where the ACPI
touchscreen node often only contains info on the gpio and the irq
and is missing any info on the axis. In this case drivers may want
to fill in some of these values based on e.g. DMI identification
if a specific model tablet before calling touchscreen_parse_properties.

Signed-off-by: Hans de Goede <redacted>
---
 drivers/input/touchscreen/of_touchscreen.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/input/touchscreen/of_touchscreen.c b/drivers/input/touchscreen/of_touchscreen.c
index 8d7f9c8..180a334 100644
--- a/drivers/input/touchscreen/of_touchscreen.c
+++ b/drivers/input/touchscreen/of_touchscreen.c

@@ -116,12 +116,12 @@ void touchscreen_parse_properties(struct input_dev *input, bool multitouch,
 
 	prop->max_x = input_abs_get_max(input, axis);
 	prop->max_y = input_abs_get_max(input, axis + 1);
-	prop->invert_x =
-		device_property_read_bool(dev, "touchscreen-inverted-x");
-	prop->invert_y =
-		device_property_read_bool(dev, "touchscreen-inverted-y");
-	prop->swap_x_y =
-		device_property_read_bool(dev, "touchscreen-swapped-x-y");
+	if (device_property_read_bool(dev, "touchscreen-inverted-x"))
+		prop->invert_x = true;
+	if (device_property_read_bool(dev, "touchscreen-inverted-y"))
+		prop->invert_y = true;
+	if (device_property_read_bool(dev, "touchscreen-swapped-x-y"))
+		prop->swap_x_y = true;
 
 	if (prop->swap_x_y)
 		swap(input->absinfo[axis], input->absinfo[axis + 1]);

-- 
2.9.3

ACPI gpios may return -EBUSY this means that the gpio is owned by the
ACPI code, and will be set / cleared as needed by the ACPI code.

Treat gpiod_get returning -EBUSY as not having a gpio, fixing the
driver not loading on tablets where this happens.

Signed-off-by: Hans de Goede <redacted>
---
 drivers/input/touchscreen/silead.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/input/touchscreen/silead.c b/drivers/input/touchscreen/silead.c
index f502c84..4387cd8 100644
--- a/drivers/input/touchscreen/silead.c
+++ b/drivers/input/touchscreen/silead.c

@@ -467,6 +467,14 @@ static int silead_ts_probe(struct i2c_client *client,
 
 	/* Power GPIO pin */
 	data->gpio_power = devm_gpiod_get_optional(dev, "power", GPIOD_OUT_LOW);
+#ifdef CONFIG_ACPI
+	/*
+	 * ACPI gpios may return -EBUSY this means that the gpio is owned by
+	 * the ACPI code, and will be set / cleared by the ACPI code.
+	 */
+	if (IS_ERR(data->gpio_power) && PTR_ERR(data->gpio_power) == -EBUSY)
+		data->gpio_power = NULL;
+#endif
 	if (IS_ERR(data->gpio_power)) {
 		if (PTR_ERR(data->gpio_power) != -EPROBE_DEFER)
 			dev_err(dev, "Shutdown GPIO request failed\n");

-- 
2.9.3

Set the default max_x / max_y in prop.max_? before calling
silead_ts_read_props, so that silead_ts_read_props can override
them. This will be used to fill in DMI based touchscreen info on
ACPI based tablets, since the APCI touchscreen node does not
contain resolution info.

Signed-off-by: Hans de Goede <redacted>
---
 drivers/input/touchscreen/silead.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/input/touchscreen/silead.c b/drivers/input/touchscreen/silead.c
index 4387cd8..d6593bb 100644
--- a/drivers/input/touchscreen/silead.c
+++ b/drivers/input/touchscreen/silead.c

@@ -99,8 +99,10 @@ static int silead_ts_request_input_dev(struct silead_ts_data *data)
 		return -ENOMEM;
 	}
 
-	input_set_abs_params(data->input, ABS_MT_POSITION_X, 0, 4095, 0, 0);
-	input_set_abs_params(data->input, ABS_MT_POSITION_Y, 0, 4095, 0, 0);
+	input_set_abs_params(data->input, ABS_MT_POSITION_X, 0,
+			     data->prop.max_x, 0, 0);
+	input_set_abs_params(data->input, ABS_MT_POSITION_Y, 0,
+			     data->prop.max_y, 0, 0);
 	touchscreen_parse_properties(data->input, true, &data->prop);
 
 	input_mt_init_slots(data->input, data->max_fingers,

@@ -454,6 +456,8 @@ static int silead_ts_probe(struct i2c_client *client,
 
 	i2c_set_clientdata(client, data);
 	data->client = client;
+	data->prop.max_x = 4095;
+	data->prop.max_y = 4095;
 
 	error = silead_ts_set_default_fw_name(data, id);
 	if (error)

-- 
2.9.3

On ACPI based tablets, the ACPI touchscreen node only contains info on
the gpio and the irq, and is missing any info on the axis. This info is
expected to be built into the tablet model specific version of the driver
shipped with the os-image for the device.

Add support for getting the missing info from a table built into the
driver, using dmi data to identify which entry of the table to use and
add info for the CUBE iwork8 Air tablet on which this code was tested /
developed.

BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=187531
Signed-off-by: Hans de Goede <redacted>
---
 drivers/input/touchscreen/silead.c | 51 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 50 insertions(+), 1 deletion(-)

diff --git a/drivers/input/touchscreen/silead.c b/drivers/input/touchscreen/silead.c
index d6593bb..f32b029 100644
--- a/drivers/input/touchscreen/silead.c
+++ b/drivers/input/touchscreen/silead.c

@@ -20,6 +20,7 @@
 #include <linux/i2c.h>
 #include <linux/module.h>
 #include <linux/acpi.h>
+#include <linux/dmi.h>
 #include <linux/interrupt.h>
 #include <linux/gpio/consumer.h>
 #include <linux/delay.h>

@@ -87,6 +88,38 @@ struct silead_fw_data {
 	u32 val;
 };
 
+#ifdef CONFIG_DMI
+struct silead_driver_data {
+	struct touchscreen_properties prop;
+	const char *fw_name;
+	u32 max_fingers;
+};
+
+static struct silead_driver_data cube_iwork8_air_driver_data = {
+	.prop = {
+		.max_x = 1659,
+		.max_y = 899,
+		.swap_x_y = true,
+	},
+	.fw_name = "gsl3670-cube-iwork8-air.fw",
+	.max_fingers = 5,
+};
+
+static const struct dmi_system_id silead_ts_dmi_table[] = {
+	{
+	 .ident = "CUBE iwork8 Air",
+	 .driver_data = &cube_iwork8_air_driver_data,
+	 .matches = {
+		DMI_MATCH(DMI_SYS_VENDOR, "cube"),
+		DMI_MATCH(DMI_PRODUCT_NAME, "i1-TF"),
+		DMI_MATCH(DMI_BOARD_NAME, "Cherry Trail CR"),
+		},
+	},
+
+	{ },
+};
+#endif
+
 static int silead_ts_request_input_dev(struct silead_ts_data *data)
 {
 	struct device *dev = &data->client->dev;

@@ -385,11 +418,27 @@ static void silead_ts_read_props(struct i2c_client *client)
 	const char *str;
 	int error;
 
+#ifdef CONFIG_DMI
+	const struct dmi_system_id *dmi_id;
+
+	dmi_id = dmi_first_match(silead_ts_dmi_table);
+	if (dmi_id) {
+		struct silead_driver_data *driver_data = dmi_id->driver_data;
+
+		data->prop = driver_data->prop;
+		snprintf(data->fw_name, sizeof(data->fw_name),
+			 "silead/%s", driver_data->fw_name);
+		data->max_fingers = driver_data->max_fingers;
+	}
+#endif
+
 	error = device_property_read_u32(dev, "silead,max-fingers",
 					 &data->max_fingers);
 	if (error) {
 		dev_dbg(dev, "Max fingers read error %d\n", error);
-		data->max_fingers = 5; /* Most devices handle up-to 5 fingers */
+		/* Most devices handle up-to 5 fingers */
+		if (data->max_fingers == 0)
+			data->max_fingers = 5;
 	}
 
 	error = device_property_read_string(dev, "firmware-name", &str);

-- 
2.9.3

On Fri, Dec 09, 2016 at 11:35:20AM +0100, Hans de Goede wrote:

ACPI gpios may return -EBUSY this means that the gpio is owned by the
ACPI code, and will be set / cleared as needed by the ACPI code.

Treat gpiod_get returning -EBUSY as not having a gpio, fixing the
driver not loading on tablets where this happens.

Hmm, I'd say ACPI should not be exposing existence of GPIO to the
drivers if it decides to manage it itself. Can we hide this in gpiolib
ACPI code?

Thanks.

-- 
Dmitry

On Fri, Dec 09, 2016 at 11:35:22AM +0100, Hans de Goede wrote:

On ACPI based tablets, the ACPI touchscreen node only contains info on
the gpio and the irq, and is missing any info on the axis. This info is
expected to be built into the tablet model specific version of the driver
shipped with the os-image for the device.

Add support for getting the missing info from a table built into the
driver, using dmi data to identify which entry of the table to use and
add info for the CUBE iwork8 Air tablet on which this code was tested /
developed.

BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=187531
Signed-off-by: Hans de Goede <redacted>

Instead of doing DMI stuff in the driver, I wonder if we could use
device_add_properties() API to add missing properties in DMI case.

You'd probably need to hide it all in drivers/platform/x86.. and
probably add ACPI bus callback to make sure we attache the properties
before the device is instantiated.

Thanks.

-- 
Dmitry

Hi,

On 27-12-16 23:14, Dmitry Torokhov wrote:

On Fri, Dec 09, 2016 at 11:35:20AM +0100, Hans de Goede wrote:

quoted

ACPI gpios may return -EBUSY this means that the gpio is owned by the
ACPI code, and will be set / cleared as needed by the ACPI code.

Treat gpiod_get returning -EBUSY as not having a gpio, fixing the
driver not loading on tablets where this happens.

Hmm, I'd say ACPI should not be exposing existence of GPIO to the
drivers if it decides to manage it itself. Can we hide this in gpiolib
ACPI code?

I don't think we really can, the -EBUSY comes from the chipset driver,
where there is a special bit in the gpio-cfg reg signalling whether
the gpio is owned by the host or by the firmware. So only the chipset
specific implementation knows about this.

And for other drivers not being able to access the gpio is not acceptable.

We could simply make the gpio completely optional by doing:

	data->gpio_power = devm_gpiod_get(dev, "power", GPIOD_OUT_LOW);
	if (IS_ERR(data->gpio_power)) {
		error = PTR_ERR(data->gpio_power);
		if (error == -EPROBE_DEFER)
			return error;
		dev_info(dev, "Not using power gpio: %d\n", error);
		data->gpio_power = NULL;
	}

Then we don't end up ugly-fying the silead.c code...

Regards,

Hans

quoted

Signed-off-by: Hans de Goede <redacted>
---
 drivers/input/touchscreen/silead.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/input/touchscreen/silead.c b/drivers/input/touchscreen/silead.c
index f502c84..4387cd8 100644
--- a/drivers/input/touchscreen/silead.c
+++ b/drivers/input/touchscreen/silead.c

@@ -467,6 +467,14 @@ static int silead_ts_probe(struct i2c_client *client,

 	/* Power GPIO pin */
 	data->gpio_power = devm_gpiod_get_optional(dev, "power", GPIOD_OUT_LOW);
+#ifdef CONFIG_ACPI
+	/*
+	 * ACPI gpios may return -EBUSY this means that the gpio is owned by
+	 * the ACPI code, and will be set / cleared by the ACPI code.
+	 */
+	if (IS_ERR(data->gpio_power) && PTR_ERR(data->gpio_power) == -EBUSY)
+		data->gpio_power = NULL;
+#endif
 	if (IS_ERR(data->gpio_power)) {
 		if (PTR_ERR(data->gpio_power) != -EPROBE_DEFER)
 			dev_err(dev, "Shutdown GPIO request failed\n");
--

2.9.3

Thanks.

We could simply make the gpio completely optional by doing:

Then we don't end up ugly-fying the silead.c code...

I remember vaguely that this will make the driver useless in many cases.

Some DSDTs I've seen have ACPI PM functions that handle the GPIO lines, and I 
tried calling them instead of managing the GPIO directly. This did not appear 
to work, but maybe I did it wrong.

See here: 
https://github.com/onitake/gslx680-acpi/blob/master/gslx680_ts_acpi.c#L571
And this is what's supposed to be called: 
https://github.com/onitake/gslx680-acpi/blob/master/acpi/gsl-dsdt.aml#L12

Hi,

On 27-12-16 23:27, Dmitry Torokhov wrote:

On Fri, Dec 09, 2016 at 11:35:22AM +0100, Hans de Goede wrote:

quoted

On ACPI based tablets, the ACPI touchscreen node only contains info on
the gpio and the irq, and is missing any info on the axis. This info is
expected to be built into the tablet model specific version of the driver
shipped with the os-image for the device.

Add support for getting the missing info from a table built into the
driver, using dmi data to identify which entry of the table to use and
add info for the CUBE iwork8 Air tablet on which this code was tested /
developed.

BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=187531
Signed-off-by: Hans de Goede <redacted>

Instead of doing DMI stuff in the driver, I wonder if we could use
device_add_properties() API to add missing properties in DMI case.

You'd probably need to hide it all in drivers/platform/x86.. and
probably add ACPI bus callback to make sure we attache the properties
before the device is instantiated.

We would still end up basing what properties to apply based on DMI
data, we would just be doing it in a circumvent way, with tricky
ordering issues, I do not really see any advantage in this.

AFAICT every other driver which needs to do DMI based quirks / info
is doing it directly, e.g. all of the following input drivers are
already adjusting to hardware variance using dmi-matching:

drivers/input/misc/wistron_btns.c
drivers/input/touchscreen/atmel_mxt_ts.c
drivers/input/touchscreen/goodix.c
drivers/input/keyboard/atkbd.c
drivers/input/mouse/alps.c
drivers/input/mouse/synaptics.c
drivers/input/mouse/elantech.c
drivers/input/mouse/lifebook.c

I will grant you that the dmi table potentially may become quite big.

We could put it in a new

drivers/input/touchscreen/silead-x86.c

File, and make that add device properties instead of my current
implementation, then all silead.c would gain is the following few
lines:

	error = silead_initialize_device_properties(...);
	if (error)
		return error;

And the rest would sit in drivers/input/touchscreen/silead-x86.c, so
it would be (mostly) isolated from the core silead code. I believe
this would be better then doing something similar with some code
under drivers/platfrom/x86 as that will introduce ordering issues
and just make things needlessly complicated in general IMHO.

Regards,

Hans

Thanks.

quoted

---
 drivers/input/touchscreen/silead.c | 51 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 50 insertions(+), 1 deletion(-)

diff --git a/drivers/input/touchscreen/silead.c b/drivers/input/touchscreen/silead.c
index d6593bb..f32b029 100644
--- a/drivers/input/touchscreen/silead.c
+++ b/drivers/input/touchscreen/silead.c

@@ -20,6 +20,7 @@
 #include <linux/i2c.h>
 #include <linux/module.h>
 #include <linux/acpi.h>
+#include <linux/dmi.h>
 #include <linux/interrupt.h>
 #include <linux/gpio/consumer.h>
 #include <linux/delay.h>

@@ -87,6 +88,38 @@ struct silead_fw_data {
 	u32 val;
 };

+#ifdef CONFIG_DMI
+struct silead_driver_data {
+	struct touchscreen_properties prop;
+	const char *fw_name;
+	u32 max_fingers;
+};
+
+static struct silead_driver_data cube_iwork8_air_driver_data = {
+	.prop = {
+		.max_x = 1659,
+		.max_y = 899,
+		.swap_x_y = true,
+	},
+	.fw_name = "gsl3670-cube-iwork8-air.fw",
+	.max_fingers = 5,
+};
+
+static const struct dmi_system_id silead_ts_dmi_table[] = {
+	{
+	 .ident = "CUBE iwork8 Air",
+	 .driver_data = &cube_iwork8_air_driver_data,
+	 .matches = {
+		DMI_MATCH(DMI_SYS_VENDOR, "cube"),
+		DMI_MATCH(DMI_PRODUCT_NAME, "i1-TF"),
+		DMI_MATCH(DMI_BOARD_NAME, "Cherry Trail CR"),
+		},
+	},
+
+	{ },
+};
+#endif
+
 static int silead_ts_request_input_dev(struct silead_ts_data *data)
 {
 	struct device *dev = &data->client->dev;

@@ -385,11 +418,27 @@ static void silead_ts_read_props(struct i2c_client *client)
 	const char *str;
 	int error;

+#ifdef CONFIG_DMI
+	const struct dmi_system_id *dmi_id;
+
+	dmi_id = dmi_first_match(silead_ts_dmi_table);
+	if (dmi_id) {
+		struct silead_driver_data *driver_data = dmi_id->driver_data;
+
+		data->prop = driver_data->prop;
+		snprintf(data->fw_name, sizeof(data->fw_name),
+			 "silead/%s", driver_data->fw_name);
+		data->max_fingers = driver_data->max_fingers;
+	}
+#endif
+
 	error = device_property_read_u32(dev, "silead,max-fingers",
 					 &data->max_fingers);
 	if (error) {
 		dev_dbg(dev, "Max fingers read error %d\n", error);
-		data->max_fingers = 5; /* Most devices handle up-to 5 fingers */
+		/* Most devices handle up-to 5 fingers */
+		if (data->max_fingers == 0)
+			data->max_fingers = 5;
 	}

 	error = device_property_read_string(dev, "firmware-name", &str);
--

2.9.3

Hi,

On 31-12-16 18:06, Gregor Riepl wrote:

quoted

We could simply make the gpio completely optional by doing:

Then we don't end up ugly-fying the silead.c code...

I remember vaguely that this will make the driver useless in many cases.

Some DSDTs I've seen have ACPI PM functions that handle the GPIO lines, and I tried calling them instead of managing the GPIO directly. This did not appear to work, but maybe I did it wrong.

See here: https://github.com/onitake/gslx680-acpi/blob/master/gslx680_ts_acpi.c#L571

Hmm, I've just written (and deleted) a blurb about how the platform-bus takes
care of power states for us. But this is an i2c driver, so that is not
relevant. For i2c drivers we should indeed do this ourselves. I'll write
a patch based on the code blurb you pointed to and test that on the
2 different model cherrytrail (x86) tablets I've access to.

Regards,

Hans