* [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
* 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 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 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
* [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 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 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
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 a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).