All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/4] input: of_touchscreen: Preserve flags passed into touchscreen_parse_properties
@ 2016-12-09 10:35 Hans de Goede
  2016-12-09 10:35 ` [PATCH 2/4] Input: silead_gsl1680: gpiod_get returning -EBUSY is not an error Hans de Goede
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Hans de Goede @ 2016-12-09 10:35 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: russianneuromancer @ ya . ru, Gregor Riepl, linux-input, Hans de Goede

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 <hdegoede@redhat.com>
---
 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


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 2/4] Input: silead_gsl1680: gpiod_get returning -EBUSY is not an error
  2016-12-09 10:35 [PATCH 1/4] input: of_touchscreen: Preserve flags passed into touchscreen_parse_properties Hans de Goede
@ 2016-12-09 10:35 ` Hans de Goede
  2016-12-27 22:14   ` Dmitry Torokhov
  2016-12-09 10:35 ` [PATCH 3/4] Input: silead_gsl1680: Allow silead_ts_read_props to override default resolution Hans de Goede
  2016-12-09 10:35 ` [PATCH 4/4] Input: silead_gsl1680: Add support for setting resolution based on dmi data Hans de Goede
  2 siblings, 1 reply; 10+ messages in thread
From: Hans de Goede @ 2016-12-09 10:35 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: russianneuromancer @ ya . ru, Gregor Riepl, linux-input, Hans de Goede

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 <hdegoede@redhat.com>
---
 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


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 3/4] Input: silead_gsl1680: Allow silead_ts_read_props to override default resolution
  2016-12-09 10:35 [PATCH 1/4] input: of_touchscreen: Preserve flags passed into touchscreen_parse_properties Hans de Goede
  2016-12-09 10:35 ` [PATCH 2/4] Input: silead_gsl1680: gpiod_get returning -EBUSY is not an error Hans de Goede
@ 2016-12-09 10:35 ` Hans de Goede
  2016-12-09 10:35 ` [PATCH 4/4] Input: silead_gsl1680: Add support for setting resolution based on dmi data Hans de Goede
  2 siblings, 0 replies; 10+ messages in thread
From: Hans de Goede @ 2016-12-09 10:35 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: russianneuromancer @ ya . ru, Gregor Riepl, linux-input, Hans de Goede

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 <hdegoede@redhat.com>
---
 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


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 4/4] Input: silead_gsl1680: Add support for setting resolution based on dmi data
  2016-12-09 10:35 [PATCH 1/4] input: of_touchscreen: Preserve flags passed into touchscreen_parse_properties Hans de Goede
  2016-12-09 10:35 ` [PATCH 2/4] Input: silead_gsl1680: gpiod_get returning -EBUSY is not an error Hans de Goede
  2016-12-09 10:35 ` [PATCH 3/4] Input: silead_gsl1680: Allow silead_ts_read_props to override default resolution Hans de Goede
@ 2016-12-09 10:35 ` Hans de Goede
  2016-12-27 22:27   ` Dmitry Torokhov
  2 siblings, 1 reply; 10+ messages in thread
From: Hans de Goede @ 2016-12-09 10:35 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: russianneuromancer @ ya . ru, Gregor Riepl, linux-input, Hans de Goede

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 <hdegoede@redhat.com>
---
 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


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/4] Input: silead_gsl1680: gpiod_get returning -EBUSY is not an error
  2016-12-09 10:35 ` [PATCH 2/4] Input: silead_gsl1680: gpiod_get returning -EBUSY is not an error Hans de Goede
@ 2016-12-27 22:14   ` Dmitry Torokhov
  2016-12-31 16:45     ` Hans de Goede
  0 siblings, 1 reply; 10+ messages in thread
From: Dmitry Torokhov @ 2016-12-27 22:14 UTC (permalink / raw)
  To: Hans de Goede; +Cc: russianneuromancer @ ya . ru, Gregor Riepl, linux-input

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?

> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  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.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 4/4] Input: silead_gsl1680: Add support for setting resolution based on dmi data
  2016-12-09 10:35 ` [PATCH 4/4] Input: silead_gsl1680: Add support for setting resolution based on dmi data Hans de Goede
@ 2016-12-27 22:27   ` Dmitry Torokhov
  2016-12-31 17:09     ` Hans de Goede
  0 siblings, 1 reply; 10+ messages in thread
From: Dmitry Torokhov @ 2016-12-27 22:27 UTC (permalink / raw)
  To: Hans de Goede; +Cc: russianneuromancer @ ya . ru, Gregor Riepl, linux-input

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 <hdegoede@redhat.com>

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.

> ---
>  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
> 

-- 
Dmitry

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/4] Input: silead_gsl1680: gpiod_get returning -EBUSY is not an error
  2016-12-27 22:14   ` Dmitry Torokhov
@ 2016-12-31 16:45     ` Hans de Goede
  2016-12-31 17:06       ` Gregor Riepl
  0 siblings, 1 reply; 10+ messages in thread
From: Hans de Goede @ 2016-12-31 16:45 UTC (permalink / raw)
  To: Dmitry Torokhov; +Cc: russianneuromancer @ ya . ru, Gregor Riepl, linux-input

Hi,

On 27-12-16 23:14, Dmitry Torokhov wrote:
> 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?

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



>
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  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.
>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/4] Input: silead_gsl1680: gpiod_get returning -EBUSY is not an error
  2016-12-31 16:45     ` Hans de Goede
@ 2016-12-31 17:06       ` Gregor Riepl
  2016-12-31 18:57         ` Hans de Goede
  0 siblings, 1 reply; 10+ messages in thread
From: Gregor Riepl @ 2016-12-31 17:06 UTC (permalink / raw)
  To: Hans de Goede, Dmitry Torokhov; +Cc: russianneuromancer @ ya . ru, linux-input

> 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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 4/4] Input: silead_gsl1680: Add support for setting resolution based on dmi data
  2016-12-27 22:27   ` Dmitry Torokhov
@ 2016-12-31 17:09     ` Hans de Goede
  0 siblings, 0 replies; 10+ messages in thread
From: Hans de Goede @ 2016-12-31 17:09 UTC (permalink / raw)
  To: Dmitry Torokhov; +Cc: russianneuromancer @ ya . ru, Gregor Riepl, linux-input

Hi,

On 27-12-16 23:27, Dmitry Torokhov wrote:
> 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 <hdegoede@redhat.com>
>
> 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.
>
>> ---
>>  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
>>
>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/4] Input: silead_gsl1680: gpiod_get returning -EBUSY is not an error
  2016-12-31 17:06       ` Gregor Riepl
@ 2016-12-31 18:57         ` Hans de Goede
  0 siblings, 0 replies; 10+ messages in thread
From: Hans de Goede @ 2016-12-31 18:57 UTC (permalink / raw)
  To: Gregor Riepl, Dmitry Torokhov; +Cc: russianneuromancer @ ya . ru, linux-input

Hi,

On 31-12-16 18:06, Gregor Riepl wrote:
>> 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

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2016-12-31 18:57 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-12-09 10:35 [PATCH 1/4] input: of_touchscreen: Preserve flags passed into touchscreen_parse_properties Hans de Goede
2016-12-09 10:35 ` [PATCH 2/4] Input: silead_gsl1680: gpiod_get returning -EBUSY is not an error Hans de Goede
2016-12-27 22:14   ` Dmitry Torokhov
2016-12-31 16:45     ` Hans de Goede
2016-12-31 17:06       ` Gregor Riepl
2016-12-31 18:57         ` Hans de Goede
2016-12-09 10:35 ` [PATCH 3/4] Input: silead_gsl1680: Allow silead_ts_read_props to override default resolution Hans de Goede
2016-12-09 10:35 ` [PATCH 4/4] Input: silead_gsl1680: Add support for setting resolution based on dmi data Hans de Goede
2016-12-27 22:27   ` Dmitry Torokhov
2016-12-31 17:09     ` Hans de Goede

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.