Re: [PATCH] Add generic driver for Silead tochscreens

Re: [PATCH] Add generic driver for Silead tochscreens

[Gregor Riepl]

Hello.

You beat me to it - I noticed the lack of a generic Silead driver that works
with Baytrail tablets (and ACPI) and started writing one myself, adapting the
code by Joe Burmeister (https://github.com/jabjoe/sunxi-gslx680). You can find
my endeavors (so far) here: https://github.com/onitake/gslx680-acpi

While writing the driver, I noticed that my controller or the firmware (which
I got from the Windows 8 driver for the tablet) does not seem to support
finger tracking. If I look at the data packets, almost all of them carry 0 as
the finger ID, while some carry 4. Lifting a finger simply removes the record,
without providing a way to correlate it with the next data set.

The code I worked with seems to support some kind of finger tracking, but it
still uses the ID field of each touch record as finger ID. And that does not
work with my touchscreen.

In the meantime, I just implemented type A multitouch support, leaving the
finger tracking out.

Can you help me look into the problem?

Re: [PATCH] Add generic driver for Silead tochscreens

[Dmitry Torokhov]

On Sat, Jul 11, 2015 at 11:23:37PM +0200, Gregor Riepl wrote:
> Hello.
> 
> You beat me to it - I noticed the lack of a generic Silead driver that works
> with Baytrail tablets (and ACPI) and started writing one myself, adapting the
> code by Joe Burmeister (https://github.com/jabjoe/sunxi-gslx680). You can find
> my endeavors (so far) here: https://github.com/onitake/gslx680-acpi
> 
> While writing the driver, I noticed that my controller or the firmware (which
> I got from the Windows 8 driver for the tablet) does not seem to support
> finger tracking. If I look at the data packets, almost all of them carry 0 as
> the finger ID, while some carry 4. Lifting a finger simply removes the record,
> without providing a way to correlate it with the next data set.
> 
> The code I worked with seems to support some kind of finger tracking, but it
> still uses the ID field of each touch record as finger ID. And that does not
> work with my touchscreen.
> 
> In the meantime, I just implemented type A multitouch support, leaving the
> finger tracking out.
> 
> Can you help me look into the problem?

Input core does have facilities to perform contact tracking, see
input_mt_assign_slots().

Thanks.

-- 
Dmitry

Re: [PATCH] Add generic driver for Silead tochscreens

[Robert Dolca]

On Sun, Jul 12, 2015 at 12:23 AM, Gregor Riepl <onitake@gmail.com> wrote:
> Hello.
>
> You beat me to it - I noticed the lack of a generic Silead driver that works
> with Baytrail tablets (and ACPI) and started writing one myself, adapting the
> code by Joe Burmeister (https://github.com/jabjoe/sunxi-gslx680). You can find
> my endeavors (so far) here: https://github.com/onitake/gslx680-acpi
>
> While writing the driver, I noticed that my controller or the firmware (which
> I got from the Windows 8 driver for the tablet) does not seem to support
> finger tracking. If I look at the data packets, almost all of them carry 0 as
> the finger ID, while some carry 4. Lifting a finger simply removes the record,
> without providing a way to correlate it with the next data set.
>
> The code I worked with seems to support some kind of finger tracking, but it
> still uses the ID field of each touch record as finger ID. And that does not
> work with my touchscreen.
>
> In the meantime, I just implemented type A multitouch support, leaving the
> finger tracking out.
>
> Can you help me look into the problem?

The firmware has to be calibrated by the vendor for each chip-panel
combination. If you change the panel even if you keep the same chip it
is very likely to need some calibration. This might be the reason why
the finger ID is not working properly.

Re: [PATCH] Add generic driver for Silead tochscreens

[Gregor Riepl]

> The firmware has to be calibrated by the vendor for each chip-panel
> combination. If you change the panel even if you keep the same chip it
> is very likely to need some calibration. This might be the reason why
> the finger ID is not working properly.

I see.

So, while your new driver is sufficiently generic, it would still need a
device specific firmware image and there is no way to work around that?

That's kind of a bummer.

Would it be possible to modify the driver in a way that it would work with
different types of panels/chips, even those where finger tracking does not
work correctly?

Also, I see one particular problem concerning existing hardware.
You're retrieving the dimensions, maximum number of trackable fingers and the
name of the firmware image from the DSDT or DTS using device_property_read_*.
The Baytrail tablet I'm testing on does not include such information, and
injecting a modified DSDT is maybe not the best option. Also, the DSDT shipped
I have here does not use GPIO definitions for the control pins, but instead
executes the shutdown/wakeup sequences through _PS3/_PS0 methods. At least
that was my assumption when I looked at it. There are GPIO definitions as
well, but it's not clear what those are used for. Please take a look at the
stanza yourself, if you can:
https://github.com/onitake/gslx680-acpi/blob/master/acpi/gsl-dsdt.aml
I can also send you the complete decompiled DSDT if it helps.

Re: [PATCH] Add generic driver for Silead tochscreens

[Gregor Riepl]

> The firmware has to be calibrated by the vendor for each chip-panel
> combination. If you change the panel even if you keep the same chip it
> is very likely to need some calibration. This might be the reason why
> the finger ID is not working properly.

Thinking about the device parameters... Wouldn't it be possible to read them
from the Silead controller? According to comments in the firmware shipped with
the old Android platform driver by Basewin, all the configurable parameters
are contained in the firmware.[1]

I don't have a complete data sheet, but maybe we could just read back the
panel width and height and number of tracking points from a hardware register?

That would not solve the problem of transferring the correct, platform
specific firmware image to the device, however.


[1]
https://code.google.com/p/yuandao-n90-window-dual-core-2/source/browse/drivers/input/touchscreen/gslx680_ts.h
- the comments are in Chinese, a translation can be found here:
https://github.com/jabjoe/sunxi-gslx680/blob/master/firmware/firmware_information.txt

Re: [PATCH] Add generic driver for Silead tochscreens

[Robert Dolca]

On Mon, Jul 13, 2015 at 5:24 PM, Gregor Riepl <onitake@gmail.com> wrote:
>> The firmware has to be calibrated by the vendor for each chip-panel
>> combination. If you change the panel even if you keep the same chip it
>> is very likely to need some calibration. This might be the reason why
>> the finger ID is not working properly.
>
> Thinking about the device parameters... Wouldn't it be possible to read them
> from the Silead controller? According to comments in the firmware shipped with
> the old Android platform driver by Basewin, all the configurable parameters
> are contained in the firmware.[1]
>
> I don't have a complete data sheet, but maybe we could just read back the
> panel width and height and number of tracking points from a hardware register?
>
> That would not solve the problem of transferring the correct, platform
> specific firmware image to the device, however.

In the datasheet there is no register map. Silead gave us a partial
register map in order to implement the driver. In the map they
provided there is no register for resolution or anything that would
help with this issue. The information in [1] is incomplete and can not
be considered reliable since the vendor does not say anything about
it.

Regards,
Robert

>
>
> [1]
> https://code.google.com/p/yuandao-n90-window-dual-core-2/source/browse/drivers/input/touchscreen/gslx680_ts.h
> - the comments are in Chinese, a translation can be found here:
> https://github.com/jabjoe/sunxi-gslx680/blob/master/firmware/firmware_information.txt

Re: [PATCH] Add generic driver for Silead tochscreens

[Gregor Riepl]

> In the datasheet there is no register map. Silead gave us a partial
> register map in order to implement the driver. In the map they
> provided there is no register for resolution or anything that would
> help with this issue. The information in [1] is incomplete and can not
> be considered reliable since the vendor does not say anything about
> it.

That is indeed true.
Relying on parameters read from the ACPI DSDT or the DeviceTree would
certainly be the "clean" way to go on new hardware, but it will not work on
existing hardware where the device parameters are fixed.

Do you think it would make sense to also allow passing the parameters as
module options, or via sysfs for cases where they can't be autodetected?

Also, while going over your code, I found something that might be incorrect.
When you evaluate the point data from the device, you are interpreting it with
this code:

#define SILEAD_POINT_DATA_LEN	0x04
#define SILEAD_POINT_X_OFF	0x02
#define SILEAD_POINT_ID_OFF	0x03
#define SILEAD_X_HSB_MASK	0xF0
#define SILEAD_TOUCH_ID_MASK	0x0F
for (i = 1; i <= touch_nr; i++) {
	offset = i * SILEAD_POINT_DATA_LEN;

	/* The last 4 bits are the touch id */
	id = buf[offset + SILEAD_POINT_ID_OFF] & SILEAD_TOUCH_ID_MASK;

	/* The 1st 4 bits are part of X */
	buf[offset + SILEAD_POINT_ID_OFF] =
		(buf[offset + SILEAD_POINT_ID_OFF] & SILEAD_X_HSB_MASK)
		>> 4;

	y = le16_to_cpup((__le16 *)(buf + offset));
	x = le16_to_cpup((__le16 *)(buf + offset + SILEAD_POINT_X_OFF));
}

The Basewin driver and all the ones derived from it interpreted it like this:

ts->dd = {
	.x_index = 6,
	.y_index = 4,
	.z_index = 5,
	.id_index = 7,
}
for(i= 0;i < (touches > MAX_FINGERS ? MAX_FINGERS : touches);i ++) {
	x = join_bytes( ( ts->touch_data[ts->dd->x_index + 4 * i + 1] & 0xf),
	ts->touch_data[ts->dd->x_index + 4 * i]);
	y = join_bytes(ts->touch_data[ts->dd->y_index + 4 * i + 1],
	ts->touch_data[ts->dd->y_index + 4 * i ]);
	id = ts->touch_data[ts->dd->id_index + 4 * i] >> 4;
}


Or, in other words, your code uses the second-to-last nibble of each record as
the touch id, while the Basewin code uses the last nibble (in little-endian
order).
Please correct me if I am wrong.

Re: [PATCH] Add generic driver for Silead tochscreens

[Gregor Riepl]

Please excuse me if I'm being to brash. I'd love to see complete Silead
touchscreen controller support in the mainline kernel, but with the hardware
being what it is and the lack of complete public documentation, I understand
this is a bit difficult.

> /* The last 4 bits are the touch id */
> id = buf[offset + SILEAD_POINT_ID_OFF] & SILEAD_TOUCH_ID_MASK;
>
> /* The 1st 4 bits are part of X */
> buf[offset + SILEAD_POINT_ID_OFF] =
>	(buf[offset + SILEAD_POINT_ID_OFF] & SILEAD_X_HSB_MASK)
>	>> 4;
>
> y = le16_to_cpup((__le16 *)(buf + offset));
> x = le16_to_cpup((__le16 *)(buf + offset + SILEAD_POINT_X_OFF));

Can you confirm that your data sheet/hardware does in fact use the lower 4
bits of byte 3 of each point record as the touch id, and not the upper 4 bits?

I just checked, both the Basewin driver (and all drivers derived from it) and
my testing hardware attribute it to the high nibble. The lower nibble is part
of the x coordinate.

Aside from this, would you be interested in making your driver compatible with
legacy/"broken" hardware? I'd start working on a patch if this is ok to you.

This would encompass the following:
- Adding fallbacks for unavailable information (width, height, number of touch
points, finger tracking availability, axis swapping and mirroring, firmware
name), maybe keeping the device uninitialized until all parameters are set
- Adding a sysfs interface and/or module options to set these attributes, or
possibly adding platform quirks that autoconfigurate depending on DMI data
(better ideas are welcome)
- Adding code paths for the missing functionality (finger tracking,
swapping/mirroring, other quirks)

Re: [PATCH] Add generic driver for Silead tochscreens

[Robert Dolca]

On Thu, Jul 16, 2015 at 4:59 PM, Gregor Riepl <onitake@gmail.com> wrote:
> Please excuse me if I'm being to brash. I'd love to see complete Silead
> touchscreen controller support in the mainline kernel, but with the hardware
> being what it is and the lack of complete public documentation, I understand
> this is a bit difficult.
>
>> /* The last 4 bits are the touch id */
>> id = buf[offset + SILEAD_POINT_ID_OFF] & SILEAD_TOUCH_ID_MASK;
>>
>> /* The 1st 4 bits are part of X */
>> buf[offset + SILEAD_POINT_ID_OFF] =
>>       (buf[offset + SILEAD_POINT_ID_OFF] & SILEAD_X_HSB_MASK)
>>       >> 4;
>>
>> y = le16_to_cpup((__le16 *)(buf + offset));
>> x = le16_to_cpup((__le16 *)(buf + offset + SILEAD_POINT_X_OFF));
>
> Can you confirm that your data sheet/hardware does in fact use the lower 4
> bits of byte 3 of each point record as the touch id, and not the upper 4 bits?
>
> I just checked, both the Basewin driver (and all drivers derived from it) and
> my testing hardware attribute it to the high nibble. The lower nibble is part
> of the x coordinate.
>
> Aside from this, would you be interested in making your driver compatible with
> legacy/"broken" hardware? I'd start working on a patch if this is ok to you.
>
> This would encompass the following:
> - Adding fallbacks for unavailable information (width, height, number of touch
> points, finger tracking availability, axis swapping and mirroring, firmware
> name), maybe keeping the device uninitialized until all parameters are set

axis swapping, mirroring sound like a good addon.
there is currently support for width, height, number of touch points
and firmware name

> - Adding a sysfs interface and/or module options to set these attributes, or
> possibly adding platform quirks that autoconfigurate depending on DMI data
> (better ideas are welcome)

IMHO it's not a good idea to do the config using sysfs.

> - Adding code paths for the missing functionality (finger tracking,
> swapping/mirroring, other quirks)

I am not sure I think  Dmitry could answer this one.

Re: [PATCH] Add generic driver for Silead tochscreens

[Robert Dolca]

Hi Gregor,

On Mon, Jul 13, 2015 at 6:18 PM, Gregor Riepl <onitake@gmail.com> wrote:
>> In the datasheet there is no register map. Silead gave us a partial
>> register map in order to implement the driver. In the map they
>> provided there is no register for resolution or anything that would
>> help with this issue. The information in [1] is incomplete and can not
>> be considered reliable since the vendor does not say anything about
>> it.
>
> That is indeed true.
> Relying on parameters read from the ACPI DSDT or the DeviceTree would
> certainly be the "clean" way to go on new hardware, but it will not work on
> existing hardware where the device parameters are fixed.
>
> Do you think it would make sense to also allow passing the parameters as
> module options, or via sysfs for cases where they can't be autodetected?

For the existing devices you can override the ACPI tables using initrd.
https://www.kernel.org/doc/Documentation/acpi/initrd_table_override.txt

There is some work being done that will allow to amend (no need to
completely override) the ACPI tables with custom data.
https://lkml.org/lkml/2015/6/18/947

Device tree already has support for overlays.

Giving this I think that we should keep the driver as simple as possible.

> Also, while going over your code, I found something that might be incorrect.
> When you evaluate the point data from the device, you are interpreting it with
> this code:
>
> #define SILEAD_POINT_DATA_LEN   0x04
> #define SILEAD_POINT_X_OFF      0x02
> #define SILEAD_POINT_ID_OFF     0x03
> #define SILEAD_X_HSB_MASK       0xF0
> #define SILEAD_TOUCH_ID_MASK    0x0F
> for (i = 1; i <= touch_nr; i++) {
>         offset = i * SILEAD_POINT_DATA_LEN;
>
>         /* The last 4 bits are the touch id */
>         id = buf[offset + SILEAD_POINT_ID_OFF] & SILEAD_TOUCH_ID_MASK;
>
>         /* The 1st 4 bits are part of X */
>         buf[offset + SILEAD_POINT_ID_OFF] =
>                 (buf[offset + SILEAD_POINT_ID_OFF] & SILEAD_X_HSB_MASK)
>                 >> 4;
>
>         y = le16_to_cpup((__le16 *)(buf + offset));
>         x = le16_to_cpup((__le16 *)(buf + offset + SILEAD_POINT_X_OFF));
> }
>
> The Basewin driver and all the ones derived from it interpreted it like this:
>
> ts->dd = {
>         .x_index = 6,
>         .y_index = 4,
>         .z_index = 5,
>         .id_index = 7,
> }
> for(i= 0;i < (touches > MAX_FINGERS ? MAX_FINGERS : touches);i ++) {
>         x = join_bytes( ( ts->touch_data[ts->dd->x_index + 4 * i + 1] & 0xf),
>         ts->touch_data[ts->dd->x_index + 4 * i]);
>         y = join_bytes(ts->touch_data[ts->dd->y_index + 4 * i + 1],
>         ts->touch_data[ts->dd->y_index + 4 * i ]);
>         id = ts->touch_data[ts->dd->id_index + 4 * i] >> 4;
> }
>
>
> Or, in other words, your code uses the second-to-last nibble of each record as
> the touch id, while the Basewin code uses the last nibble (in little-endian
> order).
> Please correct me if I am wrong.


I will come back with the info on the touch id after I check the docs.

Re: [PATCH] Add generic driver for Silead tochscreens

[Gregor Riepl]

Hi Robert

> For the existing devices you can override the ACPI tables using initrd.
> https://www.kernel.org/doc/Documentation/acpi/initrd_table_override.txt
> 
> There is some work being done that will allow to amend (no need to
> completely override) the ACPI tables with custom data.
> https://lkml.org/lkml/2015/6/18/947

I see.
I was under the impression that modifying the DSDT is generally frowned upon
and will result in a tainted kernel.

> Device tree already has support for overlays.
> 
> Giving this I think that we should keep the driver as simple as possible.

I absolutely agree. However, as long as there is no alternative way to pass
the necessary parameters, a driver will not work on all machines, particularly
those that were designed to be shipped with Microsoft Windows. I suspect that
vendors will be reluctant to change their (UEFI) BIOSes to accommodate full
Linux support in such devices.

> I will come back with the info on the touch id after I check the docs.

Thanks.

> axis swapping, mirroring sound like a good addon.
> there is currently support for width, height, number of touch points and
> firmware name

Those features are fairly easy to implement, and thanks to the great work by
the linux-input team, even finger tracking is not difficult to integrate.

Some example code is included below.

> IMHO it's not a good idea to do the config using sysfs.

You are probably right, yes.

In the meantime, I had a different idea. Instead of passing the panel and
firmware parameters (width, height, number of touch areas, mirroring,
swapping, tracking support) through device_property_read, they could be
contained in the firmware, in a special header.
As practically every single device seems to require its own firmware, users
and platform builders would need to obtain it anyway. Why not include all the
information the driver requires with the firmware image itself?

Reading the firmware name from the DSDT/DT like all the other parameters is an
option, of course. However... If you have, let's say, 100 different devices on
the market, all with different firmware, you'd have to install 100 different
firmware images to make automatic detection and device startup possible. This
doesn't seem very reasonable. On the other hand, just logging an error that a
specific firmware is required and requiring the user to manually download and
install that firmware isn't much better than logging vendor/device data from
the DMI, then provide a tool or website that provides the appropriate firmware
for that device.

Another obvious approach would be device quirks. Many drivers in the Linux
kernel follow this approach, but I'm not sure if it's a preferable solution.
Again, assuming there are a 100 different devices with a limited DSDT out
there, one would need 100 different device quirks.

That number is, obviously, purely hypothetical. But I hope you get the picture.



Code example: Swapping, mirroring and finger tracking support (error checking
omitted for simplicity):

/* 0 = unlimited distance */
#define MAX_DISTANCE 0
#define MAX_CONTACTS 10
struct gsl_ts_data {
	struct input_dev *input;
	/* ... */
	unsigned int x_max;
	unsigned int y_max;
	unsigned int multi_touches;
	bool x_reversed;
	bool y_reversed;
	bool xy_swapped;
	bool soft_tracking;
};
static int gsl_ts_probe(struct i2c_client *client, const struct i2c_device_id *id)
{
	struct gsl_ts_data *ts;
	/* ... */
	ts = devm_kzalloc(&client->dev, sizeof(struct gsl_ts_data), GFP_KERNEL);
	ts->input = devm_input_allocate_device(&client->dev);
	if (ts->soft_tracking) {
		input_mt_init_slots(ts->input, ts->multi_touches, INPUT_MT_DIRECT |
INPUT_MT_DROP_UNUSED | INPUT_MT_TRACK);
	} else {
		input_mt_init_slots(ts->input, ts->multi_touches, INPUT_MT_DIRECT |
INPUT_MT_DROP_UNUSED);
	}
	/* ... */
}

static void gsl_ts_mt_event(struct gsl_ts_data *ts, u8 *buf)
{
	size_t i;
	struct input_mt_pos positions[MAX_CONTACTS];
	int slots[MAX_CONTACTS];
	/* ... */
	if (num_touches > MAX_CONTACTS) {
		num_touches = MAX_CONTACTS;
	}
	for (i = 0; i < num_touches; i++) {
		/* ... */
		if (ts->xy_swapped) {
			swap(x, y);
		}
		if (ts->x_reversed) {
			x = ts->x_max - x;
		}
		if (ts->y_reversed) {
			y = ts->y_max - y;
		}
		positions[i].x = x;
		positions[i].y = y;
		if (!ts->soft_tracking) {
			slots[i] = id;
		}
	}
	if (ts->soft_tracking) {
#if LINUX_VERSION_CODE < KERNEL_VERSION(4,0,0)
		rc = input_mt_assign_slots(input, slots, positions, touches);
#else
		rc = input_mt_assign_slots(input, slots, positions, touches, MAX_DISTANCE);
#endif
		if (rc < 0) {
			dev_err(dev, "%s: input_mt_assign_slots returned %d\n", __func__, rc);
			return;
		}
	}
	for (i = 0; i < num_touches; i++) {
		input_mt_slot(input, slots[i]);
		input_mt_report_slot_state(input, MT_TOOL_FINGER, true);
		input_report_abs(input, ABS_MT_POSITION_X, positions[i].x);
		input_report_abs(input, ABS_MT_POSITION_Y, positions[i].y);
	}
	input_mt_sync_frame(input);
	input_sync(input);
}

[PATCH] Add generic driver for Silead tochscreens

[Robert Dolca]

This driver adds support for Silead touchscreens. It has been tested
with GSL1680 and GSL3680 touch panels.

It supports ACPI and device tree enumeration. For ACPI you need ACPI 5.1+
in order to be able to use named GPIOs. In device tree you can use 'interrupts'
or a named GPIO for IRQ.

Screen resolution, the maximum number of fingers supported and firmware name
are configurable using ACPI/DT properties.

Signed-off-by: Robert Dolca <robert.dolca@intel.com>
---
 drivers/input/touchscreen/Kconfig  |  12 +
 drivers/input/touchscreen/Makefile |   1 +
 drivers/input/touchscreen/silead.c | 596 +++++++++++++++++++++++++++++++++++++
 3 files changed, 609 insertions(+)
 create mode 100644 drivers/input/touchscreen/silead.c

diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
index 547f67d65..dd12f36 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -1025,4 +1025,16 @@ config TOUCHSCREEN_ZFORCE
 	  To compile this driver as a module, choose M here: the
 	  module will be called zforce_ts.
 
+config TOUCHSCREEN_SILEAD
+	tristate "Silead I2C touchscreen"
+	depends on I2C
+	help
+	  Say Y here if you have the Silead touchscreen connected to
+	  your system.
+
+	  If unsure, say N.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called silead.
+
 endif
diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
index 44deea7..2c6beaa 100644
--- a/drivers/input/touchscreen/Makefile
+++ b/drivers/input/touchscreen/Makefile
@@ -84,3 +84,4 @@ obj-$(CONFIG_TOUCHSCREEN_W90X900)	+= w90p910_ts.o
 obj-$(CONFIG_TOUCHSCREEN_SX8654)	+= sx8654.o
 obj-$(CONFIG_TOUCHSCREEN_TPS6507X)	+= tps6507x-ts.o
 obj-$(CONFIG_TOUCHSCREEN_ZFORCE)	+= zforce_ts.o
+obj-$(CONFIG_TOUCHSCREEN_SILEAD)	+= silead.o
diff --git a/drivers/input/touchscreen/silead.c b/drivers/input/touchscreen/silead.c
new file mode 100644
index 0000000..b7da344
--- /dev/null
+++ b/drivers/input/touchscreen/silead.c
@@ -0,0 +1,596 @@
+/* -------------------------------------------------------------------------
+ * Copyright (C) 2014-2015, Intel Corporation
+ *
+ * Derived from:
+ *  gslX68X.c
+ *  Copyright (C) 2010-2015, Shanghai Sileadinc Co.Ltd
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ * ------------------------------------------------------------------------- */
+
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/acpi.h>
+#include <linux/interrupt.h>
+#include <linux/gpio/consumer.h>
+#include <linux/delay.h>
+#include <linux/firmware.h>
+#include <linux/input.h>
+#include <linux/input/mt.h>
+#include <linux/pm.h>
+#include <linux/irq.h>
+
+#define SILEAD_TS_NAME "silead_ts"
+
+#define SILEAD_REG_RESET	0xE0
+#define SILEAD_REG_DATA		0x80
+#define SILEAD_REG_TOUCH_NR	0x80
+#define SILEAD_REG_POWER	0xBC
+#define SILEAD_REG_CLOCK	0xE4
+#define SILEAD_REG_STATUS	0xB0
+#define SILEAD_REG_ID		0xFC
+#define SILEAD_REG_MEM_CHECK	0xB0
+
+#define SILEAD_STATUS_OK	0x5A5A5A5A
+#define SILEAD_TS_DATA_LEN	44
+#define SILEAD_CLOCK		0x04
+
+#define SILEAD_CMD_RESET	0x88
+#define SILEAD_CMD_START	0x00
+
+#define SILEAD_POINT_DATA_LEN	0x04
+#define SILEAD_POINT_X_OFF	0x02
+#define SILEAD_POINT_ID_OFF	0x03
+#define SILEAD_X_HSB_MASK	0xF0
+#define SILEAD_TOUCH_ID_MASK	0x0F
+
+#define SILEAD_DP_X_MAX		"resolution-x"
+#define SILEAD_DP_Y_MAX		"resolution-y"
+#define SILEAD_DP_MAX_FINGERS	"max-fingers"
+#define SILEAD_DP_FW_NAME	"fw-name"
+#define SILEAD_PWR_GPIO_NAME	"power"
+
+#define SILEAD_CMD_SLEEP_MIN	10000
+#define SILEAD_CMD_SLEEP_MAX	20000
+#define SILEAD_POWER_SLEEP	20
+#define SILEAD_STARTUP_SLEEP	30
+
+enum silead_ts_power {
+	SILEAD_POWER_ON  = 1,
+	SILEAD_POWER_OFF = 0
+};
+
+struct silead_ts_data {
+	struct i2c_client *client;
+	struct gpio_desc *gpio_power;
+	struct input_dev *input;
+	const char *custom_fw_name;
+	char fw_name[I2C_NAME_SIZE];
+	u16 x_max;
+	u16 y_max;
+	u8 max_fingers;
+	u32 chip_id;
+};
+
+struct silead_fw_data {
+	u32 offset;
+	u32 val;
+};
+
+static int silead_ts_request_input_dev(struct silead_ts_data *data)
+{
+	struct device *dev = &data->client->dev;
+	int ret;
+
+	data->input = devm_input_allocate_device(dev);
+	if (!data->input) {
+		dev_err(dev,
+			"Failed to allocate input device\n");
+		return -ENOMEM;
+	}
+
+	input_set_capability(data->input, EV_ABS, ABS_X);
+	input_set_capability(data->input, EV_ABS, ABS_Y);
+
+	input_set_abs_params(data->input, ABS_MT_POSITION_X, 0,
+			     data->x_max, 0, 0);
+	input_set_abs_params(data->input, ABS_MT_POSITION_Y, 0,
+			     data->y_max, 0, 0);
+
+	input_mt_init_slots(data->input, data->max_fingers,
+			    INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED);
+
+	data->input->name = SILEAD_TS_NAME;
+	data->input->phys = "input/ts";
+	data->input->id.bustype = BUS_I2C;
+
+	ret = input_register_device(data->input);
+	if (ret) {
+		dev_err(dev, "Failed to register input device: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static void silead_ts_report_touch(struct silead_ts_data *data, u16 x, u16 y,
+				   u8 id)
+{
+	input_mt_slot(data->input, id);
+	input_mt_report_slot_state(data->input, MT_TOOL_FINGER, true);
+	input_report_abs(data->input, ABS_MT_POSITION_X, x);
+	input_report_abs(data->input, ABS_MT_POSITION_Y, y);
+}
+
+static void silead_ts_set_power(struct i2c_client *client,
+				enum silead_ts_power state)
+{
+	struct silead_ts_data *data = i2c_get_clientdata(client);
+
+	gpiod_set_value_cansleep(data->gpio_power, state);
+	msleep(SILEAD_POWER_SLEEP);
+}
+
+static void silead_ts_read_data(struct i2c_client *client)
+{
+	struct silead_ts_data *data = i2c_get_clientdata(client);
+	struct device *dev = &client->dev;
+	u8 buf[SILEAD_TS_DATA_LEN];
+	int x, y, id, touch_nr, ret, i, offset;
+
+	ret = i2c_smbus_read_i2c_block_data(client, SILEAD_REG_DATA,
+					    SILEAD_TS_DATA_LEN, buf);
+	if (ret < 0) {
+		dev_err(dev, "Data read error %d\n", ret);
+		return;
+	}
+
+	touch_nr = buf[0];
+
+	if (touch_nr < 0)
+		return;
+
+	dev_dbg(dev, "Touch number: %d\n", touch_nr);
+
+	for (i = 1; i <= touch_nr; i++) {
+		offset = i * SILEAD_POINT_DATA_LEN;
+
+		/* The last 4 bits are the touch id */
+		id = buf[offset + SILEAD_POINT_ID_OFF] & SILEAD_TOUCH_ID_MASK;
+
+		/* The 1st 4 bits are part of X */
+		buf[offset + SILEAD_POINT_ID_OFF] =
+			(buf[offset + SILEAD_POINT_ID_OFF] & SILEAD_X_HSB_MASK)
+			>> 4;
+
+		y = le16_to_cpup((__le16 *)(buf + offset));
+		x = le16_to_cpup((__le16 *)(buf + offset + SILEAD_POINT_X_OFF));
+
+		dev_dbg(dev, "x=%d y=%d id=%d\n", x, y, id);
+		silead_ts_report_touch(data, x, y, id);
+	}
+
+	input_mt_sync_frame(data->input);
+	input_sync(data->input);
+}
+
+static int silead_ts_init(struct i2c_client *client)
+{
+	struct silead_ts_data *data = i2c_get_clientdata(client);
+	int ret;
+
+	ret = i2c_smbus_write_byte_data(client, SILEAD_REG_RESET,
+					SILEAD_CMD_RESET);
+	if (ret)
+		goto i2c_write_err;
+	usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
+
+	ret = i2c_smbus_write_byte_data(client, SILEAD_REG_TOUCH_NR,
+					data->max_fingers);
+	if (ret)
+		goto i2c_write_err;
+	usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
+
+	ret = i2c_smbus_write_byte_data(client, SILEAD_REG_CLOCK, SILEAD_CLOCK);
+	if (ret)
+		goto i2c_write_err;
+	usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
+
+	ret = i2c_smbus_write_byte_data(client, SILEAD_REG_RESET,
+					SILEAD_CMD_START);
+	if (ret)
+		goto i2c_write_err;
+	usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
+
+	return 0;
+
+i2c_write_err:
+	dev_err(&client->dev, "Registers clear error %d\n", ret);
+	return ret;
+}
+
+static int silead_ts_reset(struct i2c_client *client)
+{
+	int ret;
+
+	ret = i2c_smbus_write_byte_data(client, SILEAD_REG_RESET,
+					SILEAD_CMD_RESET);
+	if (ret)
+		goto i2c_write_err;
+	usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
+
+	ret = i2c_smbus_write_byte_data(client, SILEAD_REG_CLOCK, SILEAD_CLOCK);
+	if (ret)
+		goto i2c_write_err;
+	usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
+
+	ret = i2c_smbus_write_byte_data(client, SILEAD_REG_POWER,
+					SILEAD_CMD_START);
+	if (ret)
+		goto i2c_write_err;
+	usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
+
+	return 0;
+
+i2c_write_err:
+	dev_err(&client->dev, "Chip reset error %d\n", ret);
+	return ret;
+}
+
+static int silead_ts_startup(struct i2c_client *client)
+{
+	int ret;
+
+	ret = i2c_smbus_write_byte_data(client, SILEAD_REG_RESET, 0x00);
+	if (ret) {
+		dev_err(&client->dev, "Startup error %d\n", ret);
+		return ret;
+	}
+	msleep(SILEAD_STARTUP_SLEEP);
+
+	return 0;
+}
+
+static int silead_ts_load_fw(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct silead_ts_data *data = i2c_get_clientdata(client);
+	unsigned int fw_size, i;
+	const struct firmware *fw;
+	struct silead_fw_data *fw_data;
+	int ret;
+
+	dev_dbg(dev, "Firmware file name: %s", data->fw_name);
+
+	if (data->custom_fw_name)
+		ret = request_firmware(&fw, data->custom_fw_name, dev);
+	else
+		ret = request_firmware(&fw, data->fw_name, dev);
+
+	if (ret) {
+		dev_err(dev, "Firmware request error %d\n", ret);
+		return ret;
+	}
+
+	fw_size = fw->size / sizeof(*fw_data);
+	fw_data = (struct silead_fw_data *)fw->data;
+
+	for (i = 0; i < fw_size; i++) {
+		ret = i2c_smbus_write_i2c_block_data(client, fw_data[i].offset,
+						     4, (u8 *)&fw_data[i].val);
+		if (ret) {
+			dev_err(dev, "Firmware load error %d\n", ret);
+			goto release_fw_err;
+		}
+	}
+
+	release_firmware(fw);
+	return 0;
+
+release_fw_err:
+	release_firmware(fw);
+	return ret;
+}
+
+static u32 silead_ts_get_status(struct i2c_client *client)
+{
+	int ret;
+	u32 status;
+
+	ret = i2c_smbus_read_i2c_block_data(client, SILEAD_REG_STATUS, 4,
+					    (u8 *)&status);
+	if (ret < 0) {
+		dev_err(&client->dev, "Status read error %d\n", ret);
+		return ret;
+	}
+
+	return status;
+}
+
+static int silead_ts_get_id(struct i2c_client *client)
+{
+	struct silead_ts_data *data = i2c_get_clientdata(client);
+	int ret;
+
+	ret = i2c_smbus_read_i2c_block_data(client, SILEAD_REG_ID, 4,
+					    (u8 *)&data->chip_id);
+	if (ret < 0) {
+		dev_err(&client->dev, "Chip ID read error %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int silead_ts_setup(struct i2c_client *client)
+{
+	struct silead_ts_data *data = i2c_get_clientdata(client);
+	struct device *dev = &client->dev;
+	int ret;
+	u32 status;
+
+	silead_ts_set_power(client, SILEAD_POWER_OFF);
+	silead_ts_set_power(client, SILEAD_POWER_ON);
+
+	ret = silead_ts_get_id(client);
+	if (ret)
+		return ret;
+	dev_dbg(dev, "Chip ID: 0x%8X", data->chip_id);
+
+	ret = silead_ts_init(client);
+	if (ret)
+		return ret;
+
+	ret = silead_ts_reset(client);
+	if (ret)
+		return ret;
+
+	ret = silead_ts_load_fw(client);
+	if (ret)
+		return ret;
+
+	ret = silead_ts_startup(client);
+	if (ret)
+		return ret;
+
+	status = silead_ts_get_status(client);
+	if (status != SILEAD_STATUS_OK) {
+		dev_err(dev, "Initialization error, status: 0x%X\n", status);
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
+static irqreturn_t silead_ts_threaded_irq_handler(int irq, void *id)
+{
+	struct silead_ts_data *data = (struct silead_ts_data *)id;
+	struct i2c_client *client = data->client;
+
+	silead_ts_read_data(client);
+
+	return IRQ_HANDLED;
+}
+
+static int silead_ts_read_props(struct i2c_client *client)
+{
+	struct silead_ts_data *data = i2c_get_clientdata(client);
+	struct device *dev = &client->dev;
+	int ret;
+
+	ret = device_property_read_u16(dev, SILEAD_DP_X_MAX, &data->x_max);
+	if (ret) {
+		dev_err(dev, "Resolution X read error %d\n", ret);
+		goto error;
+	}
+
+	ret = device_property_read_u16(dev, SILEAD_DP_Y_MAX, &data->y_max);
+	if (ret) {
+		dev_err(dev, "Resolution Y read error %d\n", ret);
+		goto error;
+	}
+
+	ret = device_property_read_u8(dev, SILEAD_DP_MAX_FINGERS,
+				      &data->max_fingers);
+	if (ret) {
+		dev_err(dev, "Max fingers read error %d\n", ret);
+		goto error;
+	}
+
+	ret = device_property_read_string(dev, SILEAD_DP_FW_NAME,
+					  &data->custom_fw_name);
+	if (ret)
+		dev_info(dev, "Firmware file name read error. Using default.");
+
+	dev_dbg(dev, "X max = %d, Y max = %d, max fingers = %d",
+		data->x_max, data->y_max, data->max_fingers);
+
+	return 0;
+
+error:
+	return ret;
+}
+
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id silead_ts_acpi_match[];
+
+static int silead_ts_set_default_fw_name(struct silead_ts_data *data,
+					 const struct i2c_device_id *id)
+{
+	const struct acpi_device_id *acpi_id;
+	struct device *dev = &data->client->dev;
+	int i;
+
+	if (ACPI_HANDLE(dev)) {
+		acpi_id = acpi_match_device(silead_ts_acpi_match, dev);
+		if (!acpi_id)
+			return -ENODEV;
+
+		sprintf(data->fw_name, "%s.fw", acpi_id->id);
+
+		for (i = 0; i < strlen(data->fw_name); i++)
+			data->fw_name[i] = tolower(data->fw_name[i]);
+	} else {
+		sprintf(data->fw_name, "%s.fw", id->name);
+	}
+
+	return 0;
+}
+#else
+static int silead_ts_set_default_fw_name(struct silead_ts_data *data,
+					 const struct i2c_device_id *id)
+{
+	sprintf(data->fw_name, "%s.fw", id->name);
+	return 0;
+}
+#endif
+
+static int silead_ts_probe(struct i2c_client *client,
+			   const struct i2c_device_id *id)
+{
+	struct silead_ts_data *data;
+	struct device *dev = &client->dev;
+	int ret;
+
+	if (!i2c_check_functionality(client->adapter,
+				     I2C_FUNC_I2C |
+				     I2C_FUNC_SMBUS_READ_I2C_BLOCK |
+				     I2C_FUNC_SMBUS_WRITE_I2C_BLOCK)) {
+		dev_err(dev, "I2C functionality check failed\n");
+		return -ENXIO;
+	}
+
+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	i2c_set_clientdata(client, data);
+	data->client = client;
+
+	ret = silead_ts_set_default_fw_name(data, id);
+	if (ret)
+		return ret;
+
+	/* If the IRQ is not filled by DT or ACPI subsytem
+	 * we can't continue without it */
+	if (client->irq <= 0)
+		return -ENODEV;
+
+	/* Power GPIO pin */
+	data->gpio_power = devm_gpiod_get(dev, SILEAD_PWR_GPIO_NAME,
+					  GPIOD_OUT_LOW);
+	if (IS_ERR(data->gpio_power)) {
+		dev_err(dev, "Shutdown GPIO request failed\n");
+		return PTR_ERR(data->gpio_power);
+	}
+
+	ret = silead_ts_read_props(client);
+	if (ret)
+		return ret;
+
+	ret = silead_ts_setup(client);
+	if (ret)
+		return ret;
+
+	ret = silead_ts_request_input_dev(data);
+	if (ret)
+		return ret;
+
+	ret = devm_request_threaded_irq(dev, client->irq, NULL,
+					silead_ts_threaded_irq_handler,
+					IRQF_ONESHOT | IRQ_TYPE_EDGE_RISING,
+					client->name, data);
+	if (ret) {
+		dev_err(dev, "IRQ request failed %d\n", ret);
+		return ret;
+	}
+
+	dev_dbg(dev, "Probing succeded\n");
+	return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int silead_ts_suspend(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+
+	silead_ts_set_power(client, SILEAD_POWER_OFF);
+	return 0;
+}
+
+static int silead_ts_resume(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	int ret, status;
+
+	silead_ts_set_power(client, SILEAD_POWER_ON);
+
+	ret = silead_ts_reset(client);
+	if (ret)
+		return ret;
+
+	ret = silead_ts_startup(client);
+	if (ret)
+		return ret;
+
+	status = silead_ts_get_status(client);
+	if (status != SILEAD_STATUS_OK) {
+		dev_err(dev, "Resume error, status: 0x%X\n", status);
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(silead_ts_pm, silead_ts_suspend, silead_ts_resume);
+#endif
+
+static const struct i2c_device_id silead_ts_id[] = {
+	{ "gsl1680", 0 },
+	{ "gsl1688", 0 },
+	{ "gsl3670", 0 },
+	{ "gsl3675", 0 },
+	{ "gsl3692", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, silead_ts_id);
+
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id silead_ts_acpi_match[] = {
+	{ "GSL1680", 0 },
+	{ "GSL1688", 0 },
+	{ "GSL3670", 0 },
+	{ "GSL3675", 0 },
+	{ "GSL3692", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, silead_ts_acpi_match);
+#endif
+
+static struct i2c_driver silead_ts_driver = {
+	.probe = silead_ts_probe,
+	.id_table = silead_ts_id,
+	.driver = {
+		.name = SILEAD_TS_NAME,
+		.owner = THIS_MODULE,
+#ifdef CONFIG_ACPI
+		.acpi_match_table = ACPI_PTR(silead_ts_acpi_match),
+#endif
+#ifdef CONFIG_PM_SLEEP
+		.pm = &silead_ts_pm,
+#endif
+	},
+};
+module_i2c_driver(silead_ts_driver);
+
+MODULE_AUTHOR("Robert Dolca <robert.dolca@intel.com>");
+MODULE_DESCRIPTION("Silead I2C touchscreen driver");
+MODULE_LICENSE("GPL");
-- 
1.9.1

Re: [PATCH] Add generic driver for Silead tochscreens

[Paul Bolle]

On vr, 2015-07-10 at 18:11 +0300, Robert Dolca wrote:
> +static struct i2c_driver silead_ts_driver = {
> +	[...]
> +	.driver = {
> +		[...]
> +#ifdef CONFIG_ACPI
> +		.acpi_match_table = ACPI_PTR(silead_ts_acpi_match),
> +#endif

A nit: for the !CONFIG_ACPI case include/linux/acpi.h contains
    #define ACPI_PTR(_ptr)  (NULL)

So I think the above CONFIG_ACPI guard is superfluous and can be dropped.

> +[...]
> +	},
> +};

Thanks,


Paul Bolle

Re: [PATCH] Add generic driver for Silead tochscreens

[Dmitry Torokhov]

Hi Robert,

On Fri, Jul 10, 2015 at 06:11:04PM +0300, Robert Dolca wrote:
> This driver adds support for Silead touchscreens. It has been tested
> with GSL1680 and GSL3680 touch panels.
> 
> It supports ACPI and device tree enumeration. For ACPI you need ACPI 5.1+
> in order to be able to use named GPIOs. In device tree you can use 'interrupts'
> or a named GPIO for IRQ.
> 
> Screen resolution, the maximum number of fingers supported and firmware name
> are configurable using ACPI/DT properties.
> 
> Signed-off-by: Robert Dolca <robert.dolca@intel.com>
> ---
>  drivers/input/touchscreen/Kconfig  |  12 +
>  drivers/input/touchscreen/Makefile |   1 +
>  drivers/input/touchscreen/silead.c | 596 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 609 insertions(+)
>  create mode 100644 drivers/input/touchscreen/silead.c
> 
> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> index 547f67d65..dd12f36 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -1025,4 +1025,16 @@ config TOUCHSCREEN_ZFORCE
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called zforce_ts.
>  
> +config TOUCHSCREEN_SILEAD
> +	tristate "Silead I2C touchscreen"
> +	depends on I2C
> +	help
> +	  Say Y here if you have the Silead touchscreen connected to
> +	  your system.
> +
> +	  If unsure, say N.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called silead.
> +
>  endif
> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> index 44deea7..2c6beaa 100644
> --- a/drivers/input/touchscreen/Makefile
> +++ b/drivers/input/touchscreen/Makefile
> @@ -84,3 +84,4 @@ obj-$(CONFIG_TOUCHSCREEN_W90X900)	+= w90p910_ts.o
>  obj-$(CONFIG_TOUCHSCREEN_SX8654)	+= sx8654.o
>  obj-$(CONFIG_TOUCHSCREEN_TPS6507X)	+= tps6507x-ts.o
>  obj-$(CONFIG_TOUCHSCREEN_ZFORCE)	+= zforce_ts.o
> +obj-$(CONFIG_TOUCHSCREEN_SILEAD)	+= silead.o
> diff --git a/drivers/input/touchscreen/silead.c b/drivers/input/touchscreen/silead.c
> new file mode 100644
> index 0000000..b7da344
> --- /dev/null
> +++ b/drivers/input/touchscreen/silead.c
> @@ -0,0 +1,596 @@
> +/* -------------------------------------------------------------------------
> + * Copyright (C) 2014-2015, Intel Corporation
> + *
> + * Derived from:
> + *  gslX68X.c
> + *  Copyright (C) 2010-2015, Shanghai Sileadinc Co.Ltd
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation; either version 2 of the License, or
> + *  (at your option) any later version.
> + *
> + *  This program is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + * ------------------------------------------------------------------------- */
> +
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/acpi.h>
> +#include <linux/interrupt.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/delay.h>
> +#include <linux/firmware.h>
> +#include <linux/input.h>
> +#include <linux/input/mt.h>
> +#include <linux/pm.h>
> +#include <linux/irq.h>
> +
> +#define SILEAD_TS_NAME "silead_ts"
> +
> +#define SILEAD_REG_RESET	0xE0
> +#define SILEAD_REG_DATA		0x80
> +#define SILEAD_REG_TOUCH_NR	0x80
> +#define SILEAD_REG_POWER	0xBC
> +#define SILEAD_REG_CLOCK	0xE4
> +#define SILEAD_REG_STATUS	0xB0
> +#define SILEAD_REG_ID		0xFC
> +#define SILEAD_REG_MEM_CHECK	0xB0
> +
> +#define SILEAD_STATUS_OK	0x5A5A5A5A
> +#define SILEAD_TS_DATA_LEN	44
> +#define SILEAD_CLOCK		0x04
> +
> +#define SILEAD_CMD_RESET	0x88
> +#define SILEAD_CMD_START	0x00
> +
> +#define SILEAD_POINT_DATA_LEN	0x04
> +#define SILEAD_POINT_X_OFF	0x02
> +#define SILEAD_POINT_ID_OFF	0x03
> +#define SILEAD_X_HSB_MASK	0xF0
> +#define SILEAD_TOUCH_ID_MASK	0x0F
> +
> +#define SILEAD_DP_X_MAX		"resolution-x"
> +#define SILEAD_DP_Y_MAX		"resolution-y"
> +#define SILEAD_DP_MAX_FINGERS	"max-fingers"
> +#define SILEAD_DP_FW_NAME	"fw-name"
> +#define SILEAD_PWR_GPIO_NAME	"power"
> +
> +#define SILEAD_CMD_SLEEP_MIN	10000
> +#define SILEAD_CMD_SLEEP_MAX	20000
> +#define SILEAD_POWER_SLEEP	20
> +#define SILEAD_STARTUP_SLEEP	30
> +
> +enum silead_ts_power {
> +	SILEAD_POWER_ON  = 1,
> +	SILEAD_POWER_OFF = 0
> +};
> +
> +struct silead_ts_data {
> +	struct i2c_client *client;
> +	struct gpio_desc *gpio_power;
> +	struct input_dev *input;
> +	const char *custom_fw_name;
> +	char fw_name[I2C_NAME_SIZE];
> +	u16 x_max;
> +	u16 y_max;
> +	u8 max_fingers;
> +	u32 chip_id;
> +};
> +
> +struct silead_fw_data {
> +	u32 offset;
> +	u32 val;
> +};
> +
> +static int silead_ts_request_input_dev(struct silead_ts_data *data)
> +{
> +	struct device *dev = &data->client->dev;
> +	int ret;
> +
> +	data->input = devm_input_allocate_device(dev);
> +	if (!data->input) {
> +		dev_err(dev,
> +			"Failed to allocate input device\n");
> +		return -ENOMEM;
> +	}
> +
> +	input_set_capability(data->input, EV_ABS, ABS_X);
> +	input_set_capability(data->input, EV_ABS, ABS_Y);

No need as you do input_set_abs_params() below.

> +
> +	input_set_abs_params(data->input, ABS_MT_POSITION_X, 0,
> +			     data->x_max, 0, 0);
> +	input_set_abs_params(data->input, ABS_MT_POSITION_Y, 0,
> +			     data->y_max, 0, 0);
> +
> +	input_mt_init_slots(data->input, data->max_fingers,
> +			    INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED);
> +
> +	data->input->name = SILEAD_TS_NAME;
> +	data->input->phys = "input/ts";
> +	data->input->id.bustype = BUS_I2C;
> +
> +	ret = input_register_device(data->input);
> +	if (ret) {
> +		dev_err(dev, "Failed to register input device: %d\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static void silead_ts_report_touch(struct silead_ts_data *data, u16 x, u16 y,
> +				   u8 id)
> +{
> +	input_mt_slot(data->input, id);
> +	input_mt_report_slot_state(data->input, MT_TOOL_FINGER, true);
> +	input_report_abs(data->input, ABS_MT_POSITION_X, x);
> +	input_report_abs(data->input, ABS_MT_POSITION_Y, y);
> +}
> +
> +static void silead_ts_set_power(struct i2c_client *client,
> +				enum silead_ts_power state)
> +{
> +	struct silead_ts_data *data = i2c_get_clientdata(client);
> +
> +	gpiod_set_value_cansleep(data->gpio_power, state);
> +	msleep(SILEAD_POWER_SLEEP);
> +}
> +
> +static void silead_ts_read_data(struct i2c_client *client)
> +{
> +	struct silead_ts_data *data = i2c_get_clientdata(client);
> +	struct device *dev = &client->dev;
> +	u8 buf[SILEAD_TS_DATA_LEN];
> +	int x, y, id, touch_nr, ret, i, offset;
> +
> +	ret = i2c_smbus_read_i2c_block_data(client, SILEAD_REG_DATA,
> +					    SILEAD_TS_DATA_LEN, buf);
> +	if (ret < 0) {
> +		dev_err(dev, "Data read error %d\n", ret);
> +		return;
> +	}
> +
> +	touch_nr = buf[0];
> +
> +	if (touch_nr < 0)
> +		return;
> +
> +	dev_dbg(dev, "Touch number: %d\n", touch_nr);
> +
> +	for (i = 1; i <= touch_nr; i++) {
> +		offset = i * SILEAD_POINT_DATA_LEN;
> +
> +		/* The last 4 bits are the touch id */
> +		id = buf[offset + SILEAD_POINT_ID_OFF] & SILEAD_TOUCH_ID_MASK;
> +
> +		/* The 1st 4 bits are part of X */
> +		buf[offset + SILEAD_POINT_ID_OFF] =
> +			(buf[offset + SILEAD_POINT_ID_OFF] & SILEAD_X_HSB_MASK)
> +			>> 4;
> +
> +		y = le16_to_cpup((__le16 *)(buf + offset));
> +		x = le16_to_cpup((__le16 *)(buf + offset + SILEAD_POINT_X_OFF));
> +
> +		dev_dbg(dev, "x=%d y=%d id=%d\n", x, y, id);
> +		silead_ts_report_touch(data, x, y, id);
> +	}
> +
> +	input_mt_sync_frame(data->input);
> +	input_sync(data->input);
> +}
> +
> +static int silead_ts_init(struct i2c_client *client)
> +{
> +	struct silead_ts_data *data = i2c_get_clientdata(client);
> +	int ret;
> +
> +	ret = i2c_smbus_write_byte_data(client, SILEAD_REG_RESET,
> +					SILEAD_CMD_RESET);
> +	if (ret)
> +		goto i2c_write_err;
> +	usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
> +
> +	ret = i2c_smbus_write_byte_data(client, SILEAD_REG_TOUCH_NR,
> +					data->max_fingers);
> +	if (ret)
> +		goto i2c_write_err;
> +	usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
> +
> +	ret = i2c_smbus_write_byte_data(client, SILEAD_REG_CLOCK, SILEAD_CLOCK);
> +	if (ret)
> +		goto i2c_write_err;
> +	usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
> +
> +	ret = i2c_smbus_write_byte_data(client, SILEAD_REG_RESET,
> +					SILEAD_CMD_START);
> +	if (ret)
> +		goto i2c_write_err;
> +	usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
> +
> +	return 0;
> +
> +i2c_write_err:
> +	dev_err(&client->dev, "Registers clear error %d\n", ret);
> +	return ret;
> +}
> +
> +static int silead_ts_reset(struct i2c_client *client)
> +{
> +	int ret;
> +
> +	ret = i2c_smbus_write_byte_data(client, SILEAD_REG_RESET,
> +					SILEAD_CMD_RESET);
> +	if (ret)
> +		goto i2c_write_err;
> +	usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
> +
> +	ret = i2c_smbus_write_byte_data(client, SILEAD_REG_CLOCK, SILEAD_CLOCK);
> +	if (ret)
> +		goto i2c_write_err;
> +	usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
> +
> +	ret = i2c_smbus_write_byte_data(client, SILEAD_REG_POWER,
> +					SILEAD_CMD_START);
> +	if (ret)
> +		goto i2c_write_err;
> +	usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
> +
> +	return 0;
> +
> +i2c_write_err:
> +	dev_err(&client->dev, "Chip reset error %d\n", ret);
> +	return ret;
> +}
> +
> +static int silead_ts_startup(struct i2c_client *client)
> +{
> +	int ret;
> +
> +	ret = i2c_smbus_write_byte_data(client, SILEAD_REG_RESET, 0x00);
> +	if (ret) {
> +		dev_err(&client->dev, "Startup error %d\n", ret);
> +		return ret;
> +	}
> +	msleep(SILEAD_STARTUP_SLEEP);
> +
> +	return 0;
> +}
> +
> +static int silead_ts_load_fw(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct silead_ts_data *data = i2c_get_clientdata(client);
> +	unsigned int fw_size, i;
> +	const struct firmware *fw;
> +	struct silead_fw_data *fw_data;
> +	int ret;
> +
> +	dev_dbg(dev, "Firmware file name: %s", data->fw_name);
> +
> +	if (data->custom_fw_name)
> +		ret = request_firmware(&fw, data->custom_fw_name, dev);
> +	else
> +		ret = request_firmware(&fw, data->fw_name, dev);
> +
> +	if (ret) {
> +		dev_err(dev, "Firmware request error %d\n", ret);
> +		return ret;
> +	}
> +
> +	fw_size = fw->size / sizeof(*fw_data);
> +	fw_data = (struct silead_fw_data *)fw->data;
> +
> +	for (i = 0; i < fw_size; i++) {
> +		ret = i2c_smbus_write_i2c_block_data(client, fw_data[i].offset,
> +						     4, (u8 *)&fw_data[i].val);
> +		if (ret) {
> +			dev_err(dev, "Firmware load error %d\n", ret);
> +			goto release_fw_err;
> +		}
> +	}
> +
> +	release_firmware(fw);
> +	return 0;
> +
> +release_fw_err:
> +	release_firmware(fw);
> +	return ret;
> +}
> +
> +static u32 silead_ts_get_status(struct i2c_client *client)
> +{
> +	int ret;
> +	u32 status;
> +
> +	ret = i2c_smbus_read_i2c_block_data(client, SILEAD_REG_STATUS, 4,
> +					    (u8 *)&status);

Endianness.

> +	if (ret < 0) {
> +		dev_err(&client->dev, "Status read error %d\n", ret);
> +		return ret;
> +	}
> +
> +	return status;
> +}
> +
> +static int silead_ts_get_id(struct i2c_client *client)
> +{
> +	struct silead_ts_data *data = i2c_get_clientdata(client);
> +	int ret;
> +
> +	ret = i2c_smbus_read_i2c_block_data(client, SILEAD_REG_ID, 4,
> +					    (u8 *)&data->chip_id);

Endianness.

> +	if (ret < 0) {
> +		dev_err(&client->dev, "Chip ID read error %d\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int silead_ts_setup(struct i2c_client *client)
> +{
> +	struct silead_ts_data *data = i2c_get_clientdata(client);
> +	struct device *dev = &client->dev;
> +	int ret;
> +	u32 status;
> +
> +	silead_ts_set_power(client, SILEAD_POWER_OFF);
> +	silead_ts_set_power(client, SILEAD_POWER_ON);
> +
> +	ret = silead_ts_get_id(client);
> +	if (ret)
> +		return ret;
> +	dev_dbg(dev, "Chip ID: 0x%8X", data->chip_id);
> +
> +	ret = silead_ts_init(client);
> +	if (ret)
> +		return ret;
> +
> +	ret = silead_ts_reset(client);
> +	if (ret)
> +		return ret;
> +
> +	ret = silead_ts_load_fw(client);
> +	if (ret)
> +		return ret;
> +
> +	ret = silead_ts_startup(client);
> +	if (ret)
> +		return ret;
> +
> +	status = silead_ts_get_status(client);
> +	if (status != SILEAD_STATUS_OK) {
> +		dev_err(dev, "Initialization error, status: 0x%X\n", status);
> +		return -ENODEV;
> +	}
> +
> +	return 0;
> +}
> +
> +static irqreturn_t silead_ts_threaded_irq_handler(int irq, void *id)
> +{
> +	struct silead_ts_data *data = (struct silead_ts_data *)id;

No need to cast from void *.

> +	struct i2c_client *client = data->client;
> +
> +	silead_ts_read_data(client);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int silead_ts_read_props(struct i2c_client *client)
> +{
> +	struct silead_ts_data *data = i2c_get_clientdata(client);
> +	struct device *dev = &client->dev;
> +	int ret;
> +
> +	ret = device_property_read_u16(dev, SILEAD_DP_X_MAX, &data->x_max);
> +	if (ret) {
> +		dev_err(dev, "Resolution X read error %d\n", ret);
> +		goto error;
> +	}

Can we use the standard touchscreen properties as described in
Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt

> +
> +	ret = device_property_read_u16(dev, SILEAD_DP_Y_MAX, &data->y_max);
> +	if (ret) {
> +		dev_err(dev, "Resolution Y read error %d\n", ret);
> +		goto error;
> +	}
> +
> +	ret = device_property_read_u8(dev, SILEAD_DP_MAX_FINGERS,
> +				      &data->max_fingers);
> +	if (ret) {
> +		dev_err(dev, "Max fingers read error %d\n", ret);
> +		goto error;
> +	}

Can you query the controller capabilities to get number of fingers
supported? What controls it? The chip? Firmware?

> +
> +	ret = device_property_read_string(dev, SILEAD_DP_FW_NAME,
> +					  &data->custom_fw_name);
> +	if (ret)
> +		dev_info(dev, "Firmware file name read error. Using default.");

Why do we need this?

> +
> +	dev_dbg(dev, "X max = %d, Y max = %d, max fingers = %d",
> +		data->x_max, data->y_max, data->max_fingers);
> +
> +	return 0;
> +
> +error:
> +	return ret;
> +}
> +
> +#ifdef CONFIG_ACPI
> +static const struct acpi_device_id silead_ts_acpi_match[];
> +
> +static int silead_ts_set_default_fw_name(struct silead_ts_data *data,
> +					 const struct i2c_device_id *id)
> +{
> +	const struct acpi_device_id *acpi_id;
> +	struct device *dev = &data->client->dev;
> +	int i;
> +
> +	if (ACPI_HANDLE(dev)) {
> +		acpi_id = acpi_match_device(silead_ts_acpi_match, dev);
> +		if (!acpi_id)
> +			return -ENODEV;
> +
> +		sprintf(data->fw_name, "%s.fw", acpi_id->id);
> +
> +		for (i = 0; i < strlen(data->fw_name); i++)
> +			data->fw_name[i] = tolower(data->fw_name[i]);
> +	} else {
> +		sprintf(data->fw_name, "%s.fw", id->name);
> +	}
> +
> +	return 0;
> +}
> +#else
> +static int silead_ts_set_default_fw_name(struct silead_ts_data *data,
> +					 const struct i2c_device_id *id)
> +{
> +	sprintf(data->fw_name, "%s.fw", id->name);
> +	return 0;
> +}
> +#endif
> +
> +static int silead_ts_probe(struct i2c_client *client,
> +			   const struct i2c_device_id *id)
> +{
> +	struct silead_ts_data *data;
> +	struct device *dev = &client->dev;
> +	int ret;
> +
> +	if (!i2c_check_functionality(client->adapter,
> +				     I2C_FUNC_I2C |
> +				     I2C_FUNC_SMBUS_READ_I2C_BLOCK |
> +				     I2C_FUNC_SMBUS_WRITE_I2C_BLOCK)) {
> +		dev_err(dev, "I2C functionality check failed\n");
> +		return -ENXIO;
> +	}
> +
> +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	i2c_set_clientdata(client, data);
> +	data->client = client;
> +
> +	ret = silead_ts_set_default_fw_name(data, id);
> +	if (ret)
> +		return ret;
> +
> +	/* If the IRQ is not filled by DT or ACPI subsytem
> +	 * we can't continue without it */
> +	if (client->irq <= 0)
> +		return -ENODEV;
> +
> +	/* Power GPIO pin */
> +	data->gpio_power = devm_gpiod_get(dev, SILEAD_PWR_GPIO_NAME,
> +					  GPIOD_OUT_LOW);
> +	if (IS_ERR(data->gpio_power)) {
> +		dev_err(dev, "Shutdown GPIO request failed\n");
> +		return PTR_ERR(data->gpio_power);
> +	}
> +
> +	ret = silead_ts_read_props(client);
> +	if (ret)
> +		return ret;
> +
> +	ret = silead_ts_setup(client);
> +	if (ret)
> +		return ret;
> +
> +	ret = silead_ts_request_input_dev(data);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_request_threaded_irq(dev, client->irq, NULL,
> +					silead_ts_threaded_irq_handler,
> +					IRQF_ONESHOT | IRQ_TYPE_EDGE_RISING,
> +					client->name, data);
> +	if (ret) {
> +		dev_err(dev, "IRQ request failed %d\n", ret);
> +		return ret;
> +	}
> +
> +	dev_dbg(dev, "Probing succeded\n");
> +	return 0;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int silead_ts_suspend(struct device *dev)

Instead of guarding with ifdef let's annotate with __maybe_unused. This
provides better compile coverage.

> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +
> +	silead_ts_set_power(client, SILEAD_POWER_OFF);
> +	return 0;
> +}
> +
> +static int silead_ts_resume(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	int ret, status;

Personal preference: can we call here and elsewhere variables that store
error codes only "error" and not "ret"?

> +
> +	silead_ts_set_power(client, SILEAD_POWER_ON);
> +
> +	ret = silead_ts_reset(client);
> +	if (ret)
> +		return ret;
> +
> +	ret = silead_ts_startup(client);
> +	if (ret)
> +		return ret;
> +
> +	status = silead_ts_get_status(client);
> +	if (status != SILEAD_STATUS_OK) {
> +		dev_err(dev, "Resume error, status: 0x%X\n", status);
> +		return -ENODEV;
> +	}
> +
> +	return 0;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(silead_ts_pm, silead_ts_suspend, silead_ts_resume);

Pull it out of ifdef and then you do not need #ifdef in driver
initializer.

> +#endif
> +
> +static const struct i2c_device_id silead_ts_id[] = {
> +	{ "gsl1680", 0 },
> +	{ "gsl1688", 0 },
> +	{ "gsl3670", 0 },
> +	{ "gsl3675", 0 },
> +	{ "gsl3692", 0 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, silead_ts_id);
> +
> +#ifdef CONFIG_ACPI
> +static const struct acpi_device_id silead_ts_acpi_match[] = {
> +	{ "GSL1680", 0 },
> +	{ "GSL1688", 0 },
> +	{ "GSL3670", 0 },
> +	{ "GSL3675", 0 },
> +	{ "GSL3692", 0 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(acpi, silead_ts_acpi_match);
> +#endif
> +
> +static struct i2c_driver silead_ts_driver = {
> +	.probe = silead_ts_probe,
> +	.id_table = silead_ts_id,
> +	.driver = {
> +		.name = SILEAD_TS_NAME,
> +		.owner = THIS_MODULE,
> +#ifdef CONFIG_ACPI
> +		.acpi_match_table = ACPI_PTR(silead_ts_acpi_match),
> +#endif

No need for ifdef if you are using ACPI_PTR.

> +#ifdef CONFIG_PM_SLEEP
> +		.pm = &silead_ts_pm,
> +#endif
> +	},
> +};
> +module_i2c_driver(silead_ts_driver);
> +
> +MODULE_AUTHOR("Robert Dolca <robert.dolca@intel.com>");
> +MODULE_DESCRIPTION("Silead I2C touchscreen driver");
> +MODULE_LICENSE("GPL");
> -- 
> 1.9.1
> 

Thanks.

-- 
Dmitry

Re: [PATCH] Add generic driver for Silead tochscreens

[Robert Dolca]

Hi Dmitry,

On Mon, Jul 20, 2015 at 9:51 AM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Fri, Jul 10, 2015 at 06:11:04PM +0300, Robert Dolca wrote:
> > This driver adds support for Silead touchscreens. It has been tested
> > with GSL1680 and GSL3680 touch panels.
> >
> > It supports ACPI and device tree enumeration. For ACPI you need ACPI 5.1+
> > in order to be able to use named GPIOs. In device tree you can use 'interrupts'
> > or a named GPIO for IRQ.
> >
> > Screen resolution, the maximum number of fingers supported and firmware name
> > are configurable using ACPI/DT properties.
> >
> > Signed-off-by: Robert Dolca <robert.dolca@intel.com>
> > ---
> >  drivers/input/touchscreen/Kconfig  |  12 +
> >  drivers/input/touchscreen/Makefile |   1 +
> >  drivers/input/touchscreen/silead.c | 596 +++++++++++++++++++++++++++++++++++++
> >  3 files changed, 609 insertions(+)
> >  create mode 100644 drivers/input/touchscreen/silead.c
> >
> > diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> > index 547f67d65..dd12f36 100644
> > --- a/drivers/input/touchscreen/Kconfig
> > +++ b/drivers/input/touchscreen/Kconfig
> > @@ -1025,4 +1025,16 @@ config TOUCHSCREEN_ZFORCE
> >         To compile this driver as a module, choose M here: the
> >         module will be called zforce_ts.
> >
> > +config TOUCHSCREEN_SILEAD
> > +     tristate "Silead I2C touchscreen"
> > +     depends on I2C
> > +     help
> > +       Say Y here if you have the Silead touchscreen connected to
> > +       your system.
> > +
> > +       If unsure, say N.
> > +
> > +       To compile this driver as a module, choose M here: the
> > +       module will be called silead.
> > +
> >  endif
> > diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> > index 44deea7..2c6beaa 100644
> > --- a/drivers/input/touchscreen/Makefile
> > +++ b/drivers/input/touchscreen/Makefile
> > @@ -84,3 +84,4 @@ obj-$(CONFIG_TOUCHSCREEN_W90X900)   += w90p910_ts.o
> >  obj-$(CONFIG_TOUCHSCREEN_SX8654)     += sx8654.o
> >  obj-$(CONFIG_TOUCHSCREEN_TPS6507X)   += tps6507x-ts.o
> >  obj-$(CONFIG_TOUCHSCREEN_ZFORCE)     += zforce_ts.o
> > +obj-$(CONFIG_TOUCHSCREEN_SILEAD)     += silead.o
> > diff --git a/drivers/input/touchscreen/silead.c b/drivers/input/touchscreen/silead.c
> > new file mode 100644
> > index 0000000..b7da344
> > --- /dev/null
> > +++ b/drivers/input/touchscreen/silead.c
> > @@ -0,0 +1,596 @@
> > +/* -------------------------------------------------------------------------
> > + * Copyright (C) 2014-2015, Intel Corporation
> > + *
> > + * Derived from:
> > + *  gslX68X.c
> > + *  Copyright (C) 2010-2015, Shanghai Sileadinc Co.Ltd
> > + *
> > + *  This program is free software; you can redistribute it and/or modify
> > + *  it under the terms of the GNU General Public License as published by
> > + *  the Free Software Foundation; either version 2 of the License, or
> > + *  (at your option) any later version.
> > + *
> > + *  This program is distributed in the hope that it will be useful,
> > + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + *  GNU General Public License for more details.
> > + * ------------------------------------------------------------------------- */
> > +
> > +#include <linux/i2c.h>
> > +#include <linux/module.h>
> > +#include <linux/acpi.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/delay.h>
> > +#include <linux/firmware.h>
> > +#include <linux/input.h>
> > +#include <linux/input/mt.h>
> > +#include <linux/pm.h>
> > +#include <linux/irq.h>
> > +
> > +#define SILEAD_TS_NAME "silead_ts"
> > +
> > +#define SILEAD_REG_RESET     0xE0
> > +#define SILEAD_REG_DATA              0x80
> > +#define SILEAD_REG_TOUCH_NR  0x80
> > +#define SILEAD_REG_POWER     0xBC
> > +#define SILEAD_REG_CLOCK     0xE4
> > +#define SILEAD_REG_STATUS    0xB0
> > +#define SILEAD_REG_ID                0xFC
> > +#define SILEAD_REG_MEM_CHECK 0xB0
> > +
> > +#define SILEAD_STATUS_OK     0x5A5A5A5A
> > +#define SILEAD_TS_DATA_LEN   44
> > +#define SILEAD_CLOCK         0x04
> > +
> > +#define SILEAD_CMD_RESET     0x88
> > +#define SILEAD_CMD_START     0x00
> > +
> > +#define SILEAD_POINT_DATA_LEN        0x04
> > +#define SILEAD_POINT_X_OFF   0x02
> > +#define SILEAD_POINT_ID_OFF  0x03
> > +#define SILEAD_X_HSB_MASK    0xF0
> > +#define SILEAD_TOUCH_ID_MASK 0x0F
> > +
> > +#define SILEAD_DP_X_MAX              "resolution-x"
> > +#define SILEAD_DP_Y_MAX              "resolution-y"
> > +#define SILEAD_DP_MAX_FINGERS        "max-fingers"
> > +#define SILEAD_DP_FW_NAME    "fw-name"
> > +#define SILEAD_PWR_GPIO_NAME "power"
> > +
> > +#define SILEAD_CMD_SLEEP_MIN 10000
> > +#define SILEAD_CMD_SLEEP_MAX 20000
> > +#define SILEAD_POWER_SLEEP   20
> > +#define SILEAD_STARTUP_SLEEP 30
> > +
> > +enum silead_ts_power {
> > +     SILEAD_POWER_ON  = 1,
> > +     SILEAD_POWER_OFF = 0
> > +};
> > +
> > +struct silead_ts_data {
> > +     struct i2c_client *client;
> > +     struct gpio_desc *gpio_power;
> > +     struct input_dev *input;
> > +     const char *custom_fw_name;
> > +     char fw_name[I2C_NAME_SIZE];
> > +     u16 x_max;
> > +     u16 y_max;
> > +     u8 max_fingers;
> > +     u32 chip_id;
> > +};
> > +
> > +struct silead_fw_data {
> > +     u32 offset;
> > +     u32 val;
> > +};
> > +
> > +static int silead_ts_request_input_dev(struct silead_ts_data *data)
> > +{
> > +     struct device *dev = &data->client->dev;
> > +     int ret;
> > +
> > +     data->input = devm_input_allocate_device(dev);
> > +     if (!data->input) {
> > +             dev_err(dev,
> > +                     "Failed to allocate input device\n");
> > +             return -ENOMEM;
> > +     }
> > +
> > +     input_set_capability(data->input, EV_ABS, ABS_X);
> > +     input_set_capability(data->input, EV_ABS, ABS_Y);
>
> No need as you do input_set_abs_params() below.

It was put here to filter out the invalid coordinates. Why should it be removed?

> > +
> > +     input_set_abs_params(data->input, ABS_MT_POSITION_X, 0,
> > +                          data->x_max, 0, 0);
> > +     input_set_abs_params(data->input, ABS_MT_POSITION_Y, 0,
> > +                          data->y_max, 0, 0);
> > +
> > +     input_mt_init_slots(data->input, data->max_fingers,
> > +                         INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED);
> > +
> > +     data->input->name = SILEAD_TS_NAME;
> > +     data->input->phys = "input/ts";
> > +     data->input->id.bustype = BUS_I2C;
> > +
> > +     ret = input_register_device(data->input);
> > +     if (ret) {
> > +             dev_err(dev, "Failed to register input device: %d\n", ret);
> > +             return ret;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static void silead_ts_report_touch(struct silead_ts_data *data, u16 x, u16 y,
> > +                                u8 id)
> > +{
> > +     input_mt_slot(data->input, id);
> > +     input_mt_report_slot_state(data->input, MT_TOOL_FINGER, true);
> > +     input_report_abs(data->input, ABS_MT_POSITION_X, x);
> > +     input_report_abs(data->input, ABS_MT_POSITION_Y, y);
> > +}
> > +
> > +static void silead_ts_set_power(struct i2c_client *client,
> > +                             enum silead_ts_power state)
> > +{
> > +     struct silead_ts_data *data = i2c_get_clientdata(client);
> > +
> > +     gpiod_set_value_cansleep(data->gpio_power, state);
> > +     msleep(SILEAD_POWER_SLEEP);
> > +}
> > +
> > +static void silead_ts_read_data(struct i2c_client *client)
> > +{
> > +     struct silead_ts_data *data = i2c_get_clientdata(client);
> > +     struct device *dev = &client->dev;
> > +     u8 buf[SILEAD_TS_DATA_LEN];
> > +     int x, y, id, touch_nr, ret, i, offset;
> > +
> > +     ret = i2c_smbus_read_i2c_block_data(client, SILEAD_REG_DATA,
> > +                                         SILEAD_TS_DATA_LEN, buf);
> > +     if (ret < 0) {
> > +             dev_err(dev, "Data read error %d\n", ret);
> > +             return;
> > +     }
> > +
> > +     touch_nr = buf[0];
> > +
> > +     if (touch_nr < 0)
> > +             return;
> > +
> > +     dev_dbg(dev, "Touch number: %d\n", touch_nr);
> > +
> > +     for (i = 1; i <= touch_nr; i++) {
> > +             offset = i * SILEAD_POINT_DATA_LEN;
> > +
> > +             /* The last 4 bits are the touch id */
> > +             id = buf[offset + SILEAD_POINT_ID_OFF] & SILEAD_TOUCH_ID_MASK;
> > +
> > +             /* The 1st 4 bits are part of X */
> > +             buf[offset + SILEAD_POINT_ID_OFF] =
> > +                     (buf[offset + SILEAD_POINT_ID_OFF] & SILEAD_X_HSB_MASK)
> > +                     >> 4;
> > +
> > +             y = le16_to_cpup((__le16 *)(buf + offset));
> > +             x = le16_to_cpup((__le16 *)(buf + offset + SILEAD_POINT_X_OFF));
> > +
> > +             dev_dbg(dev, "x=%d y=%d id=%d\n", x, y, id);
> > +             silead_ts_report_touch(data, x, y, id);
> > +     }
> > +
> > +     input_mt_sync_frame(data->input);
> > +     input_sync(data->input);
> > +}
> > +
> > +static int silead_ts_init(struct i2c_client *client)
> > +{
> > +     struct silead_ts_data *data = i2c_get_clientdata(client);
> > +     int ret;
> > +
> > +     ret = i2c_smbus_write_byte_data(client, SILEAD_REG_RESET,
> > +                                     SILEAD_CMD_RESET);
> > +     if (ret)
> > +             goto i2c_write_err;
> > +     usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
> > +
> > +     ret = i2c_smbus_write_byte_data(client, SILEAD_REG_TOUCH_NR,
> > +                                     data->max_fingers);
> > +     if (ret)
> > +             goto i2c_write_err;
> > +     usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
> > +
> > +     ret = i2c_smbus_write_byte_data(client, SILEAD_REG_CLOCK, SILEAD_CLOCK);
> > +     if (ret)
> > +             goto i2c_write_err;
> > +     usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
> > +
> > +     ret = i2c_smbus_write_byte_data(client, SILEAD_REG_RESET,
> > +                                     SILEAD_CMD_START);
> > +     if (ret)
> > +             goto i2c_write_err;
> > +     usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
> > +
> > +     return 0;
> > +
> > +i2c_write_err:
> > +     dev_err(&client->dev, "Registers clear error %d\n", ret);
> > +     return ret;
> > +}
> > +
> > +static int silead_ts_reset(struct i2c_client *client)
> > +{
> > +     int ret;
> > +
> > +     ret = i2c_smbus_write_byte_data(client, SILEAD_REG_RESET,
> > +                                     SILEAD_CMD_RESET);
> > +     if (ret)
> > +             goto i2c_write_err;
> > +     usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
> > +
> > +     ret = i2c_smbus_write_byte_data(client, SILEAD_REG_CLOCK, SILEAD_CLOCK);
> > +     if (ret)
> > +             goto i2c_write_err;
> > +     usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
> > +
> > +     ret = i2c_smbus_write_byte_data(client, SILEAD_REG_POWER,
> > +                                     SILEAD_CMD_START);
> > +     if (ret)
> > +             goto i2c_write_err;
> > +     usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
> > +
> > +     return 0;
> > +
> > +i2c_write_err:
> > +     dev_err(&client->dev, "Chip reset error %d\n", ret);
> > +     return ret;
> > +}
> > +
> > +static int silead_ts_startup(struct i2c_client *client)
> > +{
> > +     int ret;
> > +
> > +     ret = i2c_smbus_write_byte_data(client, SILEAD_REG_RESET, 0x00);
> > +     if (ret) {
> > +             dev_err(&client->dev, "Startup error %d\n", ret);
> > +             return ret;
> > +     }
> > +     msleep(SILEAD_STARTUP_SLEEP);
> > +
> > +     return 0;
> > +}
> > +
> > +static int silead_ts_load_fw(struct i2c_client *client)
> > +{
> > +     struct device *dev = &client->dev;
> > +     struct silead_ts_data *data = i2c_get_clientdata(client);
> > +     unsigned int fw_size, i;
> > +     const struct firmware *fw;
> > +     struct silead_fw_data *fw_data;
> > +     int ret;
> > +
> > +     dev_dbg(dev, "Firmware file name: %s", data->fw_name);
> > +
> > +     if (data->custom_fw_name)
> > +             ret = request_firmware(&fw, data->custom_fw_name, dev);
> > +     else
> > +             ret = request_firmware(&fw, data->fw_name, dev);
> > +
> > +     if (ret) {
> > +             dev_err(dev, "Firmware request error %d\n", ret);
> > +             return ret;
> > +     }
> > +
> > +     fw_size = fw->size / sizeof(*fw_data);
> > +     fw_data = (struct silead_fw_data *)fw->data;
> > +
> > +     for (i = 0; i < fw_size; i++) {
> > +             ret = i2c_smbus_write_i2c_block_data(client, fw_data[i].offset,
> > +                                                  4, (u8 *)&fw_data[i].val);
> > +             if (ret) {
> > +                     dev_err(dev, "Firmware load error %d\n", ret);
> > +                     goto release_fw_err;
> > +             }
> > +     }
> > +
> > +     release_firmware(fw);
> > +     return 0;
> > +
> > +release_fw_err:
> > +     release_firmware(fw);
> > +     return ret;
> > +}
> > +
> > +static u32 silead_ts_get_status(struct i2c_client *client)
> > +{
> > +     int ret;
> > +     u32 status;
> > +
> > +     ret = i2c_smbus_read_i2c_block_data(client, SILEAD_REG_STATUS, 4,
> > +                                         (u8 *)&status);
>
> Endianness.
>
> > +     if (ret < 0) {
> > +             dev_err(&client->dev, "Status read error %d\n", ret);
> > +             return ret;
> > +     }
> > +
> > +     return status;
> > +}
> > +
> > +static int silead_ts_get_id(struct i2c_client *client)
> > +{
> > +     struct silead_ts_data *data = i2c_get_clientdata(client);
> > +     int ret;
> > +
> > +     ret = i2c_smbus_read_i2c_block_data(client, SILEAD_REG_ID, 4,
> > +                                         (u8 *)&data->chip_id);
>
> Endianness.
>
> > +     if (ret < 0) {
> > +             dev_err(&client->dev, "Chip ID read error %d\n", ret);
> > +             return ret;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static int silead_ts_setup(struct i2c_client *client)
> > +{
> > +     struct silead_ts_data *data = i2c_get_clientdata(client);
> > +     struct device *dev = &client->dev;
> > +     int ret;
> > +     u32 status;
> > +
> > +     silead_ts_set_power(client, SILEAD_POWER_OFF);
> > +     silead_ts_set_power(client, SILEAD_POWER_ON);
> > +
> > +     ret = silead_ts_get_id(client);
> > +     if (ret)
> > +             return ret;
> > +     dev_dbg(dev, "Chip ID: 0x%8X", data->chip_id);
> > +
> > +     ret = silead_ts_init(client);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = silead_ts_reset(client);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = silead_ts_load_fw(client);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = silead_ts_startup(client);
> > +     if (ret)
> > +             return ret;
> > +
> > +     status = silead_ts_get_status(client);
> > +     if (status != SILEAD_STATUS_OK) {
> > +             dev_err(dev, "Initialization error, status: 0x%X\n", status);
> > +             return -ENODEV;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static irqreturn_t silead_ts_threaded_irq_handler(int irq, void *id)
> > +{
> > +     struct silead_ts_data *data = (struct silead_ts_data *)id;
>
> No need to cast from void *.
>
> > +     struct i2c_client *client = data->client;
> > +
> > +     silead_ts_read_data(client);
> > +
> > +     return IRQ_HANDLED;
> > +}
> > +
> > +static int silead_ts_read_props(struct i2c_client *client)
> > +{
> > +     struct silead_ts_data *data = i2c_get_clientdata(client);
> > +     struct device *dev = &client->dev;
> > +     int ret;
> > +
> > +     ret = device_property_read_u16(dev, SILEAD_DP_X_MAX, &data->x_max);
> > +     if (ret) {
> > +             dev_err(dev, "Resolution X read error %d\n", ret);
> > +             goto error;
> > +     }
>
> Can we use the standard touchscreen properties as described in
> Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt
>
> > +
> > +     ret = device_property_read_u16(dev, SILEAD_DP_Y_MAX, &data->y_max);
> > +     if (ret) {
> > +             dev_err(dev, "Resolution Y read error %d\n", ret);
> > +             goto error;
> > +     }
> > +
> > +     ret = device_property_read_u8(dev, SILEAD_DP_MAX_FINGERS,
> > +                                   &data->max_fingers);
> > +     if (ret) {
> > +             dev_err(dev, "Max fingers read error %d\n", ret);
> > +             goto error;
> > +     }
>
> Can you query the controller capabilities to get number of fingers
> supported? What controls it? The chip? Firmware?

In the documentation from the vendor there is no register for this. I
will ask them.

> > +
> > +     ret = device_property_read_string(dev, SILEAD_DP_FW_NAME,
> > +                                       &data->custom_fw_name);
> > +     if (ret)
> > +             dev_info(dev, "Firmware file name read error. Using default.");
>
> Why do we need this?

Even if there is the same chip, if the panel is different you need a
different firmware. The firmware file name defaults to the chip name
but can be overwritten.

> > +
> > +     dev_dbg(dev, "X max = %d, Y max = %d, max fingers = %d",
> > +             data->x_max, data->y_max, data->max_fingers);
> > +
> > +     return 0;
> > +
> > +error:
> > +     return ret;
> > +}
> > +
> > +#ifdef CONFIG_ACPI
> > +static const struct acpi_device_id silead_ts_acpi_match[];
> > +
> > +static int silead_ts_set_default_fw_name(struct silead_ts_data *data,
> > +                                      const struct i2c_device_id *id)
> > +{
> > +     const struct acpi_device_id *acpi_id;
> > +     struct device *dev = &data->client->dev;
> > +     int i;
> > +
> > +     if (ACPI_HANDLE(dev)) {
> > +             acpi_id = acpi_match_device(silead_ts_acpi_match, dev);
> > +             if (!acpi_id)
> > +                     return -ENODEV;
> > +
> > +             sprintf(data->fw_name, "%s.fw", acpi_id->id);
> > +
> > +             for (i = 0; i < strlen(data->fw_name); i++)
> > +                     data->fw_name[i] = tolower(data->fw_name[i]);
> > +     } else {
> > +             sprintf(data->fw_name, "%s.fw", id->name);
> > +     }
> > +
> > +     return 0;
> > +}
> > +#else
> > +static int silead_ts_set_default_fw_name(struct silead_ts_data *data,
> > +                                      const struct i2c_device_id *id)
> > +{
> > +     sprintf(data->fw_name, "%s.fw", id->name);
> > +     return 0;
> > +}
> > +#endif
> > +
> > +static int silead_ts_probe(struct i2c_client *client,
> > +                        const struct i2c_device_id *id)
> > +{
> > +     struct silead_ts_data *data;
> > +     struct device *dev = &client->dev;
> > +     int ret;
> > +
> > +     if (!i2c_check_functionality(client->adapter,
> > +                                  I2C_FUNC_I2C |
> > +                                  I2C_FUNC_SMBUS_READ_I2C_BLOCK |
> > +                                  I2C_FUNC_SMBUS_WRITE_I2C_BLOCK)) {
> > +             dev_err(dev, "I2C functionality check failed\n");
> > +             return -ENXIO;
> > +     }
> > +
> > +     data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> > +     if (!data)
> > +             return -ENOMEM;
> > +
> > +     i2c_set_clientdata(client, data);
> > +     data->client = client;
> > +
> > +     ret = silead_ts_set_default_fw_name(data, id);
> > +     if (ret)
> > +             return ret;
> > +
> > +     /* If the IRQ is not filled by DT or ACPI subsytem
> > +      * we can't continue without it */
> > +     if (client->irq <= 0)
> > +             return -ENODEV;
> > +
> > +     /* Power GPIO pin */
> > +     data->gpio_power = devm_gpiod_get(dev, SILEAD_PWR_GPIO_NAME,
> > +                                       GPIOD_OUT_LOW);
> > +     if (IS_ERR(data->gpio_power)) {
> > +             dev_err(dev, "Shutdown GPIO request failed\n");
> > +             return PTR_ERR(data->gpio_power);
> > +     }
> > +
> > +     ret = silead_ts_read_props(client);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = silead_ts_setup(client);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = silead_ts_request_input_dev(data);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = devm_request_threaded_irq(dev, client->irq, NULL,
> > +                                     silead_ts_threaded_irq_handler,
> > +                                     IRQF_ONESHOT | IRQ_TYPE_EDGE_RISING,
> > +                                     client->name, data);
> > +     if (ret) {
> > +             dev_err(dev, "IRQ request failed %d\n", ret);
> > +             return ret;
> > +     }
> > +
> > +     dev_dbg(dev, "Probing succeded\n");
> > +     return 0;
> > +}
> > +
> > +#ifdef CONFIG_PM_SLEEP
> > +static int silead_ts_suspend(struct device *dev)
>
> Instead of guarding with ifdef let's annotate with __maybe_unused. This
> provides better compile coverage.
>
> > +{
> > +     struct i2c_client *client = to_i2c_client(dev);
> > +
> > +     silead_ts_set_power(client, SILEAD_POWER_OFF);
> > +     return 0;
> > +}
> > +
> > +static int silead_ts_resume(struct device *dev)
> > +{
> > +     struct i2c_client *client = to_i2c_client(dev);
> > +     int ret, status;
>
> Personal preference: can we call here and elsewhere variables that store
> error codes only "error" and not "ret"?
>
> > +
> > +     silead_ts_set_power(client, SILEAD_POWER_ON);
> > +
> > +     ret = silead_ts_reset(client);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = silead_ts_startup(client);
> > +     if (ret)
> > +             return ret;
> > +
> > +     status = silead_ts_get_status(client);
> > +     if (status != SILEAD_STATUS_OK) {
> > +             dev_err(dev, "Resume error, status: 0x%X\n", status);
> > +             return -ENODEV;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static SIMPLE_DEV_PM_OPS(silead_ts_pm, silead_ts_suspend, silead_ts_resume);
>
> Pull it out of ifdef and then you do not need #ifdef in driver
> initializer.
>
> > +#endif
> > +
> > +static const struct i2c_device_id silead_ts_id[] = {
> > +     { "gsl1680", 0 },
> > +     { "gsl1688", 0 },
> > +     { "gsl3670", 0 },
> > +     { "gsl3675", 0 },
> > +     { "gsl3692", 0 },
> > +     { }
> > +};
> > +MODULE_DEVICE_TABLE(i2c, silead_ts_id);
> > +
> > +#ifdef CONFIG_ACPI
> > +static const struct acpi_device_id silead_ts_acpi_match[] = {
> > +     { "GSL1680", 0 },
> > +     { "GSL1688", 0 },
> > +     { "GSL3670", 0 },
> > +     { "GSL3675", 0 },
> > +     { "GSL3692", 0 },
> > +     { }
> > +};
> > +MODULE_DEVICE_TABLE(acpi, silead_ts_acpi_match);
> > +#endif
> > +
> > +static struct i2c_driver silead_ts_driver = {
> > +     .probe = silead_ts_probe,
> > +     .id_table = silead_ts_id,
> > +     .driver = {
> > +             .name = SILEAD_TS_NAME,
> > +             .owner = THIS_MODULE,
> > +#ifdef CONFIG_ACPI
> > +             .acpi_match_table = ACPI_PTR(silead_ts_acpi_match),
> > +#endif
>
> No need for ifdef if you are using ACPI_PTR.
>
> > +#ifdef CONFIG_PM_SLEEP
> > +             .pm = &silead_ts_pm,
> > +#endif
> > +     },
> > +};
> > +module_i2c_driver(silead_ts_driver);
> > +
> > +MODULE_AUTHOR("Robert Dolca <robert.dolca@intel.com>");
> > +MODULE_DESCRIPTION("Silead I2C touchscreen driver");
> > +MODULE_LICENSE("GPL");
> > --
> > 1.9.1
> >
>
> Thanks.
>
> --
> Dmitry
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Add generic driver for Silead tochscreens

[Dmitry Torokhov]

On Mon, Jul 20, 2015 at 03:05:44PM +0300, Robert Dolca wrote:
> Hi Dmitry,
> 
> On Mon, Jul 20, 2015 at 9:51 AM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > On Fri, Jul 10, 2015 at 06:11:04PM +0300, Robert Dolca wrote:
> > > This driver adds support for Silead touchscreens. It has been tested
> > > with GSL1680 and GSL3680 touch panels.
> > >
> > > It supports ACPI and device tree enumeration. For ACPI you need ACPI 5.1+
> > > in order to be able to use named GPIOs. In device tree you can use 'interrupts'
> > > or a named GPIO for IRQ.
> > >
> > > Screen resolution, the maximum number of fingers supported and firmware name
> > > are configurable using ACPI/DT properties.
> > >
> > > Signed-off-by: Robert Dolca <robert.dolca@intel.com>
> > > ---
> > >  drivers/input/touchscreen/Kconfig  |  12 +
> > >  drivers/input/touchscreen/Makefile |   1 +
> > >  drivers/input/touchscreen/silead.c | 596 +++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 609 insertions(+)
> > >  create mode 100644 drivers/input/touchscreen/silead.c
> > >
> > > diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> > > index 547f67d65..dd12f36 100644
> > > --- a/drivers/input/touchscreen/Kconfig
> > > +++ b/drivers/input/touchscreen/Kconfig
> > > @@ -1025,4 +1025,16 @@ config TOUCHSCREEN_ZFORCE
> > >         To compile this driver as a module, choose M here: the
> > >         module will be called zforce_ts.
> > >
> > > +config TOUCHSCREEN_SILEAD
> > > +     tristate "Silead I2C touchscreen"
> > > +     depends on I2C
> > > +     help
> > > +       Say Y here if you have the Silead touchscreen connected to
> > > +       your system.
> > > +
> > > +       If unsure, say N.
> > > +
> > > +       To compile this driver as a module, choose M here: the
> > > +       module will be called silead.
> > > +
> > >  endif
> > > diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> > > index 44deea7..2c6beaa 100644
> > > --- a/drivers/input/touchscreen/Makefile
> > > +++ b/drivers/input/touchscreen/Makefile
> > > @@ -84,3 +84,4 @@ obj-$(CONFIG_TOUCHSCREEN_W90X900)   += w90p910_ts.o
> > >  obj-$(CONFIG_TOUCHSCREEN_SX8654)     += sx8654.o
> > >  obj-$(CONFIG_TOUCHSCREEN_TPS6507X)   += tps6507x-ts.o
> > >  obj-$(CONFIG_TOUCHSCREEN_ZFORCE)     += zforce_ts.o
> > > +obj-$(CONFIG_TOUCHSCREEN_SILEAD)     += silead.o
> > > diff --git a/drivers/input/touchscreen/silead.c b/drivers/input/touchscreen/silead.c
> > > new file mode 100644
> > > index 0000000..b7da344
> > > --- /dev/null
> > > +++ b/drivers/input/touchscreen/silead.c
> > > @@ -0,0 +1,596 @@
> > > +/* -------------------------------------------------------------------------
> > > + * Copyright (C) 2014-2015, Intel Corporation
> > > + *
> > > + * Derived from:
> > > + *  gslX68X.c
> > > + *  Copyright (C) 2010-2015, Shanghai Sileadinc Co.Ltd
> > > + *
> > > + *  This program is free software; you can redistribute it and/or modify
> > > + *  it under the terms of the GNU General Public License as published by
> > > + *  the Free Software Foundation; either version 2 of the License, or
> > > + *  (at your option) any later version.
> > > + *
> > > + *  This program is distributed in the hope that it will be useful,
> > > + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > > + *  GNU General Public License for more details.
> > > + * ------------------------------------------------------------------------- */
> > > +
> > > +#include <linux/i2c.h>
> > > +#include <linux/module.h>
> > > +#include <linux/acpi.h>
> > > +#include <linux/interrupt.h>
> > > +#include <linux/gpio/consumer.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/firmware.h>
> > > +#include <linux/input.h>
> > > +#include <linux/input/mt.h>
> > > +#include <linux/pm.h>
> > > +#include <linux/irq.h>
> > > +
> > > +#define SILEAD_TS_NAME "silead_ts"
> > > +
> > > +#define SILEAD_REG_RESET     0xE0
> > > +#define SILEAD_REG_DATA              0x80
> > > +#define SILEAD_REG_TOUCH_NR  0x80
> > > +#define SILEAD_REG_POWER     0xBC
> > > +#define SILEAD_REG_CLOCK     0xE4
> > > +#define SILEAD_REG_STATUS    0xB0
> > > +#define SILEAD_REG_ID                0xFC
> > > +#define SILEAD_REG_MEM_CHECK 0xB0
> > > +
> > > +#define SILEAD_STATUS_OK     0x5A5A5A5A
> > > +#define SILEAD_TS_DATA_LEN   44
> > > +#define SILEAD_CLOCK         0x04
> > > +
> > > +#define SILEAD_CMD_RESET     0x88
> > > +#define SILEAD_CMD_START     0x00
> > > +
> > > +#define SILEAD_POINT_DATA_LEN        0x04
> > > +#define SILEAD_POINT_X_OFF   0x02
> > > +#define SILEAD_POINT_ID_OFF  0x03
> > > +#define SILEAD_X_HSB_MASK    0xF0
> > > +#define SILEAD_TOUCH_ID_MASK 0x0F
> > > +
> > > +#define SILEAD_DP_X_MAX              "resolution-x"
> > > +#define SILEAD_DP_Y_MAX              "resolution-y"
> > > +#define SILEAD_DP_MAX_FINGERS        "max-fingers"
> > > +#define SILEAD_DP_FW_NAME    "fw-name"
> > > +#define SILEAD_PWR_GPIO_NAME "power"
> > > +
> > > +#define SILEAD_CMD_SLEEP_MIN 10000
> > > +#define SILEAD_CMD_SLEEP_MAX 20000
> > > +#define SILEAD_POWER_SLEEP   20
> > > +#define SILEAD_STARTUP_SLEEP 30
> > > +
> > > +enum silead_ts_power {
> > > +     SILEAD_POWER_ON  = 1,
> > > +     SILEAD_POWER_OFF = 0
> > > +};
> > > +
> > > +struct silead_ts_data {
> > > +     struct i2c_client *client;
> > > +     struct gpio_desc *gpio_power;
> > > +     struct input_dev *input;
> > > +     const char *custom_fw_name;
> > > +     char fw_name[I2C_NAME_SIZE];
> > > +     u16 x_max;
> > > +     u16 y_max;
> > > +     u8 max_fingers;
> > > +     u32 chip_id;
> > > +};
> > > +
> > > +struct silead_fw_data {
> > > +     u32 offset;
> > > +     u32 val;
> > > +};
> > > +
> > > +static int silead_ts_request_input_dev(struct silead_ts_data *data)
> > > +{
> > > +     struct device *dev = &data->client->dev;
> > > +     int ret;
> > > +
> > > +     data->input = devm_input_allocate_device(dev);
> > > +     if (!data->input) {
> > > +             dev_err(dev,
> > > +                     "Failed to allocate input device\n");
> > > +             return -ENOMEM;
> > > +     }
> > > +
> > > +     input_set_capability(data->input, EV_ABS, ABS_X);
> > > +     input_set_capability(data->input, EV_ABS, ABS_Y);
> >
> > No need as you do input_set_abs_params() below.
> 
> It was put here to filter out the invalid coordinates. Why should it be removed?

input_set_abs_params() takes care of setting capability bits so
input_set_capability() is not really needed.

> 
> > > +
> > > +     input_set_abs_params(data->input, ABS_MT_POSITION_X, 0,
> > > +                          data->x_max, 0, 0);
> > > +     input_set_abs_params(data->input, ABS_MT_POSITION_Y, 0,
> > > +                          data->y_max, 0, 0);
> > > +
> > > +     input_mt_init_slots(data->input, data->max_fingers,
> > > +                         INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED);
> > > +
> > > +     data->input->name = SILEAD_TS_NAME;
> > > +     data->input->phys = "input/ts";
> > > +     data->input->id.bustype = BUS_I2C;
> > > +
> > > +     ret = input_register_device(data->input);
> > > +     if (ret) {
> > > +             dev_err(dev, "Failed to register input device: %d\n", ret);
> > > +             return ret;
> > > +     }
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +static void silead_ts_report_touch(struct silead_ts_data *data, u16 x, u16 y,
> > > +                                u8 id)
> > > +{
> > > +     input_mt_slot(data->input, id);
> > > +     input_mt_report_slot_state(data->input, MT_TOOL_FINGER, true);
> > > +     input_report_abs(data->input, ABS_MT_POSITION_X, x);
> > > +     input_report_abs(data->input, ABS_MT_POSITION_Y, y);
> > > +}
> > > +
> > > +static void silead_ts_set_power(struct i2c_client *client,
> > > +                             enum silead_ts_power state)
> > > +{
> > > +     struct silead_ts_data *data = i2c_get_clientdata(client);
> > > +
> > > +     gpiod_set_value_cansleep(data->gpio_power, state);
> > > +     msleep(SILEAD_POWER_SLEEP);
> > > +}
> > > +
> > > +static void silead_ts_read_data(struct i2c_client *client)
> > > +{
> > > +     struct silead_ts_data *data = i2c_get_clientdata(client);
> > > +     struct device *dev = &client->dev;
> > > +     u8 buf[SILEAD_TS_DATA_LEN];
> > > +     int x, y, id, touch_nr, ret, i, offset;
> > > +
> > > +     ret = i2c_smbus_read_i2c_block_data(client, SILEAD_REG_DATA,
> > > +                                         SILEAD_TS_DATA_LEN, buf);
> > > +     if (ret < 0) {
> > > +             dev_err(dev, "Data read error %d\n", ret);
> > > +             return;
> > > +     }
> > > +
> > > +     touch_nr = buf[0];
> > > +
> > > +     if (touch_nr < 0)
> > > +             return;
> > > +
> > > +     dev_dbg(dev, "Touch number: %d\n", touch_nr);
> > > +
> > > +     for (i = 1; i <= touch_nr; i++) {
> > > +             offset = i * SILEAD_POINT_DATA_LEN;
> > > +
> > > +             /* The last 4 bits are the touch id */
> > > +             id = buf[offset + SILEAD_POINT_ID_OFF] & SILEAD_TOUCH_ID_MASK;
> > > +
> > > +             /* The 1st 4 bits are part of X */
> > > +             buf[offset + SILEAD_POINT_ID_OFF] =
> > > +                     (buf[offset + SILEAD_POINT_ID_OFF] & SILEAD_X_HSB_MASK)
> > > +                     >> 4;
> > > +
> > > +             y = le16_to_cpup((__le16 *)(buf + offset));
> > > +             x = le16_to_cpup((__le16 *)(buf + offset + SILEAD_POINT_X_OFF));
> > > +
> > > +             dev_dbg(dev, "x=%d y=%d id=%d\n", x, y, id);
> > > +             silead_ts_report_touch(data, x, y, id);
> > > +     }
> > > +
> > > +     input_mt_sync_frame(data->input);
> > > +     input_sync(data->input);
> > > +}
> > > +
> > > +static int silead_ts_init(struct i2c_client *client)
> > > +{
> > > +     struct silead_ts_data *data = i2c_get_clientdata(client);
> > > +     int ret;
> > > +
> > > +     ret = i2c_smbus_write_byte_data(client, SILEAD_REG_RESET,
> > > +                                     SILEAD_CMD_RESET);
> > > +     if (ret)
> > > +             goto i2c_write_err;
> > > +     usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
> > > +
> > > +     ret = i2c_smbus_write_byte_data(client, SILEAD_REG_TOUCH_NR,
> > > +                                     data->max_fingers);
> > > +     if (ret)
> > > +             goto i2c_write_err;
> > > +     usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
> > > +
> > > +     ret = i2c_smbus_write_byte_data(client, SILEAD_REG_CLOCK, SILEAD_CLOCK);
> > > +     if (ret)
> > > +             goto i2c_write_err;
> > > +     usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
> > > +
> > > +     ret = i2c_smbus_write_byte_data(client, SILEAD_REG_RESET,
> > > +                                     SILEAD_CMD_START);
> > > +     if (ret)
> > > +             goto i2c_write_err;
> > > +     usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
> > > +
> > > +     return 0;
> > > +
> > > +i2c_write_err:
> > > +     dev_err(&client->dev, "Registers clear error %d\n", ret);
> > > +     return ret;
> > > +}
> > > +
> > > +static int silead_ts_reset(struct i2c_client *client)
> > > +{
> > > +     int ret;
> > > +
> > > +     ret = i2c_smbus_write_byte_data(client, SILEAD_REG_RESET,
> > > +                                     SILEAD_CMD_RESET);
> > > +     if (ret)
> > > +             goto i2c_write_err;
> > > +     usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
> > > +
> > > +     ret = i2c_smbus_write_byte_data(client, SILEAD_REG_CLOCK, SILEAD_CLOCK);
> > > +     if (ret)
> > > +             goto i2c_write_err;
> > > +     usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
> > > +
> > > +     ret = i2c_smbus_write_byte_data(client, SILEAD_REG_POWER,
> > > +                                     SILEAD_CMD_START);
> > > +     if (ret)
> > > +             goto i2c_write_err;
> > > +     usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
> > > +
> > > +     return 0;
> > > +
> > > +i2c_write_err:
> > > +     dev_err(&client->dev, "Chip reset error %d\n", ret);
> > > +     return ret;
> > > +}
> > > +
> > > +static int silead_ts_startup(struct i2c_client *client)
> > > +{
> > > +     int ret;
> > > +
> > > +     ret = i2c_smbus_write_byte_data(client, SILEAD_REG_RESET, 0x00);
> > > +     if (ret) {
> > > +             dev_err(&client->dev, "Startup error %d\n", ret);
> > > +             return ret;
> > > +     }
> > > +     msleep(SILEAD_STARTUP_SLEEP);
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +static int silead_ts_load_fw(struct i2c_client *client)
> > > +{
> > > +     struct device *dev = &client->dev;
> > > +     struct silead_ts_data *data = i2c_get_clientdata(client);
> > > +     unsigned int fw_size, i;
> > > +     const struct firmware *fw;
> > > +     struct silead_fw_data *fw_data;
> > > +     int ret;
> > > +
> > > +     dev_dbg(dev, "Firmware file name: %s", data->fw_name);
> > > +
> > > +     if (data->custom_fw_name)
> > > +             ret = request_firmware(&fw, data->custom_fw_name, dev);
> > > +     else
> > > +             ret = request_firmware(&fw, data->fw_name, dev);
> > > +
> > > +     if (ret) {
> > > +             dev_err(dev, "Firmware request error %d\n", ret);
> > > +             return ret;
> > > +     }
> > > +
> > > +     fw_size = fw->size / sizeof(*fw_data);
> > > +     fw_data = (struct silead_fw_data *)fw->data;
> > > +
> > > +     for (i = 0; i < fw_size; i++) {
> > > +             ret = i2c_smbus_write_i2c_block_data(client, fw_data[i].offset,
> > > +                                                  4, (u8 *)&fw_data[i].val);
> > > +             if (ret) {
> > > +                     dev_err(dev, "Firmware load error %d\n", ret);
> > > +                     goto release_fw_err;
> > > +             }
> > > +     }
> > > +
> > > +     release_firmware(fw);
> > > +     return 0;
> > > +
> > > +release_fw_err:
> > > +     release_firmware(fw);
> > > +     return ret;
> > > +}
> > > +
> > > +static u32 silead_ts_get_status(struct i2c_client *client)
> > > +{
> > > +     int ret;
> > > +     u32 status;
> > > +
> > > +     ret = i2c_smbus_read_i2c_block_data(client, SILEAD_REG_STATUS, 4,
> > > +                                         (u8 *)&status);
> >
> > Endianness.
> >
> > > +     if (ret < 0) {
> > > +             dev_err(&client->dev, "Status read error %d\n", ret);
> > > +             return ret;
> > > +     }
> > > +
> > > +     return status;
> > > +}
> > > +
> > > +static int silead_ts_get_id(struct i2c_client *client)
> > > +{
> > > +     struct silead_ts_data *data = i2c_get_clientdata(client);
> > > +     int ret;
> > > +
> > > +     ret = i2c_smbus_read_i2c_block_data(client, SILEAD_REG_ID, 4,
> > > +                                         (u8 *)&data->chip_id);
> >
> > Endianness.
> >
> > > +     if (ret < 0) {
> > > +             dev_err(&client->dev, "Chip ID read error %d\n", ret);
> > > +             return ret;
> > > +     }
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +static int silead_ts_setup(struct i2c_client *client)
> > > +{
> > > +     struct silead_ts_data *data = i2c_get_clientdata(client);
> > > +     struct device *dev = &client->dev;
> > > +     int ret;
> > > +     u32 status;
> > > +
> > > +     silead_ts_set_power(client, SILEAD_POWER_OFF);
> > > +     silead_ts_set_power(client, SILEAD_POWER_ON);
> > > +
> > > +     ret = silead_ts_get_id(client);
> > > +     if (ret)
> > > +             return ret;
> > > +     dev_dbg(dev, "Chip ID: 0x%8X", data->chip_id);
> > > +
> > > +     ret = silead_ts_init(client);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     ret = silead_ts_reset(client);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     ret = silead_ts_load_fw(client);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     ret = silead_ts_startup(client);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     status = silead_ts_get_status(client);
> > > +     if (status != SILEAD_STATUS_OK) {
> > > +             dev_err(dev, "Initialization error, status: 0x%X\n", status);
> > > +             return -ENODEV;
> > > +     }
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +static irqreturn_t silead_ts_threaded_irq_handler(int irq, void *id)
> > > +{
> > > +     struct silead_ts_data *data = (struct silead_ts_data *)id;
> >
> > No need to cast from void *.
> >
> > > +     struct i2c_client *client = data->client;
> > > +
> > > +     silead_ts_read_data(client);
> > > +
> > > +     return IRQ_HANDLED;
> > > +}
> > > +
> > > +static int silead_ts_read_props(struct i2c_client *client)
> > > +{
> > > +     struct silead_ts_data *data = i2c_get_clientdata(client);
> > > +     struct device *dev = &client->dev;
> > > +     int ret;
> > > +
> > > +     ret = device_property_read_u16(dev, SILEAD_DP_X_MAX, &data->x_max);
> > > +     if (ret) {
> > > +             dev_err(dev, "Resolution X read error %d\n", ret);
> > > +             goto error;
> > > +     }
> >
> > Can we use the standard touchscreen properties as described in
> > Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt
> >
> > > +
> > > +     ret = device_property_read_u16(dev, SILEAD_DP_Y_MAX, &data->y_max);
> > > +     if (ret) {
> > > +             dev_err(dev, "Resolution Y read error %d\n", ret);
> > > +             goto error;
> > > +     }
> > > +
> > > +     ret = device_property_read_u8(dev, SILEAD_DP_MAX_FINGERS,
> > > +                                   &data->max_fingers);
> > > +     if (ret) {
> > > +             dev_err(dev, "Max fingers read error %d\n", ret);
> > > +             goto error;
> > > +     }
> >
> > Can you query the controller capabilities to get number of fingers
> > supported? What controls it? The chip? Firmware?
> 
> In the documentation from the vendor there is no register for this. I
> will ask them.
> 
> > > +
> > > +     ret = device_property_read_string(dev, SILEAD_DP_FW_NAME,
> > > +                                       &data->custom_fw_name);
> > > +     if (ret)
> > > +             dev_info(dev, "Firmware file name read error. Using default.");
> >
> > Why do we need this?
> 
> Even if there is the same chip, if the panel is different you need a
> different firmware. The firmware file name defaults to the chip name
> but can be overwritten.

But when you compose your userland image you can make sure you are
putting the needed firmware into the right place. There is no really
need to add the code to kernel when you can rename in userspace.

> 
> > > +
> > > +     dev_dbg(dev, "X max = %d, Y max = %d, max fingers = %d",
> > > +             data->x_max, data->y_max, data->max_fingers);
> > > +
> > > +     return 0;
> > > +
> > > +error:
> > > +     return ret;
> > > +}
> > > +
> > > +#ifdef CONFIG_ACPI
> > > +static const struct acpi_device_id silead_ts_acpi_match[];
> > > +
> > > +static int silead_ts_set_default_fw_name(struct silead_ts_data *data,
> > > +                                      const struct i2c_device_id *id)
> > > +{
> > > +     const struct acpi_device_id *acpi_id;
> > > +     struct device *dev = &data->client->dev;
> > > +     int i;
> > > +
> > > +     if (ACPI_HANDLE(dev)) {
> > > +             acpi_id = acpi_match_device(silead_ts_acpi_match, dev);
> > > +             if (!acpi_id)
> > > +                     return -ENODEV;
> > > +
> > > +             sprintf(data->fw_name, "%s.fw", acpi_id->id);
> > > +
> > > +             for (i = 0; i < strlen(data->fw_name); i++)
> > > +                     data->fw_name[i] = tolower(data->fw_name[i]);
> > > +     } else {
> > > +             sprintf(data->fw_name, "%s.fw", id->name);
> > > +     }
> > > +
> > > +     return 0;
> > > +}
> > > +#else
> > > +static int silead_ts_set_default_fw_name(struct silead_ts_data *data,
> > > +                                      const struct i2c_device_id *id)
> > > +{
> > > +     sprintf(data->fw_name, "%s.fw", id->name);
> > > +     return 0;
> > > +}
> > > +#endif
> > > +
> > > +static int silead_ts_probe(struct i2c_client *client,
> > > +                        const struct i2c_device_id *id)
> > > +{
> > > +     struct silead_ts_data *data;
> > > +     struct device *dev = &client->dev;
> > > +     int ret;
> > > +
> > > +     if (!i2c_check_functionality(client->adapter,
> > > +                                  I2C_FUNC_I2C |
> > > +                                  I2C_FUNC_SMBUS_READ_I2C_BLOCK |
> > > +                                  I2C_FUNC_SMBUS_WRITE_I2C_BLOCK)) {
> > > +             dev_err(dev, "I2C functionality check failed\n");
> > > +             return -ENXIO;
> > > +     }
> > > +
> > > +     data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> > > +     if (!data)
> > > +             return -ENOMEM;
> > > +
> > > +     i2c_set_clientdata(client, data);
> > > +     data->client = client;
> > > +
> > > +     ret = silead_ts_set_default_fw_name(data, id);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     /* If the IRQ is not filled by DT or ACPI subsytem
> > > +      * we can't continue without it */
> > > +     if (client->irq <= 0)
> > > +             return -ENODEV;
> > > +
> > > +     /* Power GPIO pin */
> > > +     data->gpio_power = devm_gpiod_get(dev, SILEAD_PWR_GPIO_NAME,
> > > +                                       GPIOD_OUT_LOW);
> > > +     if (IS_ERR(data->gpio_power)) {
> > > +             dev_err(dev, "Shutdown GPIO request failed\n");
> > > +             return PTR_ERR(data->gpio_power);
> > > +     }
> > > +
> > > +     ret = silead_ts_read_props(client);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     ret = silead_ts_setup(client);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     ret = silead_ts_request_input_dev(data);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     ret = devm_request_threaded_irq(dev, client->irq, NULL,
> > > +                                     silead_ts_threaded_irq_handler,
> > > +                                     IRQF_ONESHOT | IRQ_TYPE_EDGE_RISING,
> > > +                                     client->name, data);
> > > +     if (ret) {
> > > +             dev_err(dev, "IRQ request failed %d\n", ret);
> > > +             return ret;
> > > +     }
> > > +
> > > +     dev_dbg(dev, "Probing succeded\n");
> > > +     return 0;
> > > +}
> > > +
> > > +#ifdef CONFIG_PM_SLEEP
> > > +static int silead_ts_suspend(struct device *dev)
> >
> > Instead of guarding with ifdef let's annotate with __maybe_unused. This
> > provides better compile coverage.
> >
> > > +{
> > > +     struct i2c_client *client = to_i2c_client(dev);
> > > +
> > > +     silead_ts_set_power(client, SILEAD_POWER_OFF);
> > > +     return 0;
> > > +}
> > > +
> > > +static int silead_ts_resume(struct device *dev)
> > > +{
> > > +     struct i2c_client *client = to_i2c_client(dev);
> > > +     int ret, status;
> >
> > Personal preference: can we call here and elsewhere variables that store
> > error codes only "error" and not "ret"?
> >
> > > +
> > > +     silead_ts_set_power(client, SILEAD_POWER_ON);
> > > +
> > > +     ret = silead_ts_reset(client);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     ret = silead_ts_startup(client);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     status = silead_ts_get_status(client);
> > > +     if (status != SILEAD_STATUS_OK) {
> > > +             dev_err(dev, "Resume error, status: 0x%X\n", status);
> > > +             return -ENODEV;
> > > +     }
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +static SIMPLE_DEV_PM_OPS(silead_ts_pm, silead_ts_suspend, silead_ts_resume);
> >
> > Pull it out of ifdef and then you do not need #ifdef in driver
> > initializer.
> >
> > > +#endif
> > > +
> > > +static const struct i2c_device_id silead_ts_id[] = {
> > > +     { "gsl1680", 0 },
> > > +     { "gsl1688", 0 },
> > > +     { "gsl3670", 0 },
> > > +     { "gsl3675", 0 },
> > > +     { "gsl3692", 0 },
> > > +     { }
> > > +};
> > > +MODULE_DEVICE_TABLE(i2c, silead_ts_id);
> > > +
> > > +#ifdef CONFIG_ACPI
> > > +static const struct acpi_device_id silead_ts_acpi_match[] = {
> > > +     { "GSL1680", 0 },
> > > +     { "GSL1688", 0 },
> > > +     { "GSL3670", 0 },
> > > +     { "GSL3675", 0 },
> > > +     { "GSL3692", 0 },
> > > +     { }
> > > +};
> > > +MODULE_DEVICE_TABLE(acpi, silead_ts_acpi_match);
> > > +#endif
> > > +
> > > +static struct i2c_driver silead_ts_driver = {
> > > +     .probe = silead_ts_probe,
> > > +     .id_table = silead_ts_id,
> > > +     .driver = {
> > > +             .name = SILEAD_TS_NAME,
> > > +             .owner = THIS_MODULE,
> > > +#ifdef CONFIG_ACPI
> > > +             .acpi_match_table = ACPI_PTR(silead_ts_acpi_match),
> > > +#endif
> >
> > No need for ifdef if you are using ACPI_PTR.
> >
> > > +#ifdef CONFIG_PM_SLEEP
> > > +             .pm = &silead_ts_pm,
> > > +#endif
> > > +     },
> > > +};
> > > +module_i2c_driver(silead_ts_driver);
> > > +
> > > +MODULE_AUTHOR("Robert Dolca <robert.dolca@intel.com>");
> > > +MODULE_DESCRIPTION("Silead I2C touchscreen driver");
> > > +MODULE_LICENSE("GPL");
> > > --
> > > 1.9.1
> > >

Thanks.

-- 
Dmitry

Re: [PATCH] Add generic driver for Silead tochscreens

[Robert Dolca]

On Tue, Jul 28, 2015 at 12:30 AM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Mon, Jul 20, 2015 at 03:05:44PM +0300, Robert Dolca wrote:
>> Hi Dmitry,
>>
>> On Mon, Jul 20, 2015 at 9:51 AM, Dmitry Torokhov
>> <dmitry.torokhov@gmail.com> wrote:
>> > On Fri, Jul 10, 2015 at 06:11:04PM +0300, Robert Dolca wrote:
>> > > This driver adds support for Silead touchscreens. It has been tested
>> > > with GSL1680 and GSL3680 touch panels.
>> > >
>> > > It supports ACPI and device tree enumeration. For ACPI you need ACPI 5.1+
>> > > in order to be able to use named GPIOs. In device tree you can use 'interrupts'
>> > > or a named GPIO for IRQ.
>> > >
>> > > Screen resolution, the maximum number of fingers supported and firmware name
>> > > are configurable using ACPI/DT properties.
>> > >
>> > > Signed-off-by: Robert Dolca <robert.dolca@intel.com>
>> > > ---
>> > >  drivers/input/touchscreen/Kconfig  |  12 +
>> > >  drivers/input/touchscreen/Makefile |   1 +
>> > >  drivers/input/touchscreen/silead.c | 596 +++++++++++++++++++++++++++++++++++++
>> > >  3 files changed, 609 insertions(+)
>> > >  create mode 100644 drivers/input/touchscreen/silead.c
>> > >
>> > > diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
>> > > index 547f67d65..dd12f36 100644
>> > > --- a/drivers/input/touchscreen/Kconfig
>> > > +++ b/drivers/input/touchscreen/Kconfig
>> > > @@ -1025,4 +1025,16 @@ config TOUCHSCREEN_ZFORCE
>> > >         To compile this driver as a module, choose M here: the
>> > >         module will be called zforce_ts.
>> > >
>> > > +config TOUCHSCREEN_SILEAD
>> > > +     tristate "Silead I2C touchscreen"
>> > > +     depends on I2C
>> > > +     help
>> > > +       Say Y here if you have the Silead touchscreen connected to
>> > > +       your system.
>> > > +
>> > > +       If unsure, say N.
>> > > +
>> > > +       To compile this driver as a module, choose M here: the
>> > > +       module will be called silead.
>> > > +
>> > >  endif
>> > > diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
>> > > index 44deea7..2c6beaa 100644
>> > > --- a/drivers/input/touchscreen/Makefile
>> > > +++ b/drivers/input/touchscreen/Makefile
>> > > @@ -84,3 +84,4 @@ obj-$(CONFIG_TOUCHSCREEN_W90X900)   += w90p910_ts.o
>> > >  obj-$(CONFIG_TOUCHSCREEN_SX8654)     += sx8654.o
>> > >  obj-$(CONFIG_TOUCHSCREEN_TPS6507X)   += tps6507x-ts.o
>> > >  obj-$(CONFIG_TOUCHSCREEN_ZFORCE)     += zforce_ts.o
>> > > +obj-$(CONFIG_TOUCHSCREEN_SILEAD)     += silead.o
>> > > diff --git a/drivers/input/touchscreen/silead.c b/drivers/input/touchscreen/silead.c
>> > > new file mode 100644
>> > > index 0000000..b7da344
>> > > --- /dev/null
>> > > +++ b/drivers/input/touchscreen/silead.c
>> > > @@ -0,0 +1,596 @@
>> > > +/* -------------------------------------------------------------------------
>> > > + * Copyright (C) 2014-2015, Intel Corporation
>> > > + *
>> > > + * Derived from:
>> > > + *  gslX68X.c
>> > > + *  Copyright (C) 2010-2015, Shanghai Sileadinc Co.Ltd
>> > > + *
>> > > + *  This program is free software; you can redistribute it and/or modify
>> > > + *  it under the terms of the GNU General Public License as published by
>> > > + *  the Free Software Foundation; either version 2 of the License, or
>> > > + *  (at your option) any later version.
>> > > + *
>> > > + *  This program is distributed in the hope that it will be useful,
>> > > + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
>> > > + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> > > + *  GNU General Public License for more details.
>> > > + * ------------------------------------------------------------------------- */
>> > > +
>> > > +#include <linux/i2c.h>
>> > > +#include <linux/module.h>
>> > > +#include <linux/acpi.h>
>> > > +#include <linux/interrupt.h>
>> > > +#include <linux/gpio/consumer.h>
>> > > +#include <linux/delay.h>
>> > > +#include <linux/firmware.h>
>> > > +#include <linux/input.h>
>> > > +#include <linux/input/mt.h>
>> > > +#include <linux/pm.h>
>> > > +#include <linux/irq.h>
>> > > +
>> > > +#define SILEAD_TS_NAME "silead_ts"
>> > > +
>> > > +#define SILEAD_REG_RESET     0xE0
>> > > +#define SILEAD_REG_DATA              0x80
>> > > +#define SILEAD_REG_TOUCH_NR  0x80
>> > > +#define SILEAD_REG_POWER     0xBC
>> > > +#define SILEAD_REG_CLOCK     0xE4
>> > > +#define SILEAD_REG_STATUS    0xB0
>> > > +#define SILEAD_REG_ID                0xFC
>> > > +#define SILEAD_REG_MEM_CHECK 0xB0
>> > > +
>> > > +#define SILEAD_STATUS_OK     0x5A5A5A5A
>> > > +#define SILEAD_TS_DATA_LEN   44
>> > > +#define SILEAD_CLOCK         0x04
>> > > +
>> > > +#define SILEAD_CMD_RESET     0x88
>> > > +#define SILEAD_CMD_START     0x00
>> > > +
>> > > +#define SILEAD_POINT_DATA_LEN        0x04
>> > > +#define SILEAD_POINT_X_OFF   0x02
>> > > +#define SILEAD_POINT_ID_OFF  0x03
>> > > +#define SILEAD_X_HSB_MASK    0xF0
>> > > +#define SILEAD_TOUCH_ID_MASK 0x0F
>> > > +
>> > > +#define SILEAD_DP_X_MAX              "resolution-x"
>> > > +#define SILEAD_DP_Y_MAX              "resolution-y"
>> > > +#define SILEAD_DP_MAX_FINGERS        "max-fingers"
>> > > +#define SILEAD_DP_FW_NAME    "fw-name"
>> > > +#define SILEAD_PWR_GPIO_NAME "power"
>> > > +
>> > > +#define SILEAD_CMD_SLEEP_MIN 10000
>> > > +#define SILEAD_CMD_SLEEP_MAX 20000
>> > > +#define SILEAD_POWER_SLEEP   20
>> > > +#define SILEAD_STARTUP_SLEEP 30
>> > > +
>> > > +enum silead_ts_power {
>> > > +     SILEAD_POWER_ON  = 1,
>> > > +     SILEAD_POWER_OFF = 0
>> > > +};
>> > > +
>> > > +struct silead_ts_data {
>> > > +     struct i2c_client *client;
>> > > +     struct gpio_desc *gpio_power;
>> > > +     struct input_dev *input;
>> > > +     const char *custom_fw_name;
>> > > +     char fw_name[I2C_NAME_SIZE];
>> > > +     u16 x_max;
>> > > +     u16 y_max;
>> > > +     u8 max_fingers;
>> > > +     u32 chip_id;
>> > > +};
>> > > +
>> > > +struct silead_fw_data {
>> > > +     u32 offset;
>> > > +     u32 val;
>> > > +};
>> > > +
>> > > +static int silead_ts_request_input_dev(struct silead_ts_data *data)
>> > > +{
>> > > +     struct device *dev = &data->client->dev;
>> > > +     int ret;
>> > > +
>> > > +     data->input = devm_input_allocate_device(dev);
>> > > +     if (!data->input) {
>> > > +             dev_err(dev,
>> > > +                     "Failed to allocate input device\n");
>> > > +             return -ENOMEM;
>> > > +     }
>> > > +
>> > > +     input_set_capability(data->input, EV_ABS, ABS_X);
>> > > +     input_set_capability(data->input, EV_ABS, ABS_Y);
>> >
>> > No need as you do input_set_abs_params() below.
>>
>> It was put here to filter out the invalid coordinates. Why should it be removed?
>
> input_set_abs_params() takes care of setting capability bits so
> input_set_capability() is not really needed.
>
>>
>> > > +
>> > > +     input_set_abs_params(data->input, ABS_MT_POSITION_X, 0,
>> > > +                          data->x_max, 0, 0);
>> > > +     input_set_abs_params(data->input, ABS_MT_POSITION_Y, 0,
>> > > +                          data->y_max, 0, 0);
>> > > +
>> > > +     input_mt_init_slots(data->input, data->max_fingers,
>> > > +                         INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED);
>> > > +
>> > > +     data->input->name = SILEAD_TS_NAME;
>> > > +     data->input->phys = "input/ts";
>> > > +     data->input->id.bustype = BUS_I2C;
>> > > +
>> > > +     ret = input_register_device(data->input);
>> > > +     if (ret) {
>> > > +             dev_err(dev, "Failed to register input device: %d\n", ret);
>> > > +             return ret;
>> > > +     }
>> > > +
>> > > +     return 0;
>> > > +}
>> > > +
>> > > +static void silead_ts_report_touch(struct silead_ts_data *data, u16 x, u16 y,
>> > > +                                u8 id)
>> > > +{
>> > > +     input_mt_slot(data->input, id);
>> > > +     input_mt_report_slot_state(data->input, MT_TOOL_FINGER, true);
>> > > +     input_report_abs(data->input, ABS_MT_POSITION_X, x);
>> > > +     input_report_abs(data->input, ABS_MT_POSITION_Y, y);
>> > > +}
>> > > +
>> > > +static void silead_ts_set_power(struct i2c_client *client,
>> > > +                             enum silead_ts_power state)
>> > > +{
>> > > +     struct silead_ts_data *data = i2c_get_clientdata(client);
>> > > +
>> > > +     gpiod_set_value_cansleep(data->gpio_power, state);
>> > > +     msleep(SILEAD_POWER_SLEEP);
>> > > +}
>> > > +
>> > > +static void silead_ts_read_data(struct i2c_client *client)
>> > > +{
>> > > +     struct silead_ts_data *data = i2c_get_clientdata(client);
>> > > +     struct device *dev = &client->dev;
>> > > +     u8 buf[SILEAD_TS_DATA_LEN];
>> > > +     int x, y, id, touch_nr, ret, i, offset;
>> > > +
>> > > +     ret = i2c_smbus_read_i2c_block_data(client, SILEAD_REG_DATA,
>> > > +                                         SILEAD_TS_DATA_LEN, buf);
>> > > +     if (ret < 0) {
>> > > +             dev_err(dev, "Data read error %d\n", ret);
>> > > +             return;
>> > > +     }
>> > > +
>> > > +     touch_nr = buf[0];
>> > > +
>> > > +     if (touch_nr < 0)
>> > > +             return;
>> > > +
>> > > +     dev_dbg(dev, "Touch number: %d\n", touch_nr);
>> > > +
>> > > +     for (i = 1; i <= touch_nr; i++) {
>> > > +             offset = i * SILEAD_POINT_DATA_LEN;
>> > > +
>> > > +             /* The last 4 bits are the touch id */
>> > > +             id = buf[offset + SILEAD_POINT_ID_OFF] & SILEAD_TOUCH_ID_MASK;
>> > > +
>> > > +             /* The 1st 4 bits are part of X */
>> > > +             buf[offset + SILEAD_POINT_ID_OFF] =
>> > > +                     (buf[offset + SILEAD_POINT_ID_OFF] & SILEAD_X_HSB_MASK)
>> > > +                     >> 4;
>> > > +
>> > > +             y = le16_to_cpup((__le16 *)(buf + offset));
>> > > +             x = le16_to_cpup((__le16 *)(buf + offset + SILEAD_POINT_X_OFF));
>> > > +
>> > > +             dev_dbg(dev, "x=%d y=%d id=%d\n", x, y, id);
>> > > +             silead_ts_report_touch(data, x, y, id);
>> > > +     }
>> > > +
>> > > +     input_mt_sync_frame(data->input);
>> > > +     input_sync(data->input);
>> > > +}
>> > > +
>> > > +static int silead_ts_init(struct i2c_client *client)
>> > > +{
>> > > +     struct silead_ts_data *data = i2c_get_clientdata(client);
>> > > +     int ret;
>> > > +
>> > > +     ret = i2c_smbus_write_byte_data(client, SILEAD_REG_RESET,
>> > > +                                     SILEAD_CMD_RESET);
>> > > +     if (ret)
>> > > +             goto i2c_write_err;
>> > > +     usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
>> > > +
>> > > +     ret = i2c_smbus_write_byte_data(client, SILEAD_REG_TOUCH_NR,
>> > > +                                     data->max_fingers);
>> > > +     if (ret)
>> > > +             goto i2c_write_err;
>> > > +     usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
>> > > +
>> > > +     ret = i2c_smbus_write_byte_data(client, SILEAD_REG_CLOCK, SILEAD_CLOCK);
>> > > +     if (ret)
>> > > +             goto i2c_write_err;
>> > > +     usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
>> > > +
>> > > +     ret = i2c_smbus_write_byte_data(client, SILEAD_REG_RESET,
>> > > +                                     SILEAD_CMD_START);
>> > > +     if (ret)
>> > > +             goto i2c_write_err;
>> > > +     usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
>> > > +
>> > > +     return 0;
>> > > +
>> > > +i2c_write_err:
>> > > +     dev_err(&client->dev, "Registers clear error %d\n", ret);
>> > > +     return ret;
>> > > +}
>> > > +
>> > > +static int silead_ts_reset(struct i2c_client *client)
>> > > +{
>> > > +     int ret;
>> > > +
>> > > +     ret = i2c_smbus_write_byte_data(client, SILEAD_REG_RESET,
>> > > +                                     SILEAD_CMD_RESET);
>> > > +     if (ret)
>> > > +             goto i2c_write_err;
>> > > +     usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
>> > > +
>> > > +     ret = i2c_smbus_write_byte_data(client, SILEAD_REG_CLOCK, SILEAD_CLOCK);
>> > > +     if (ret)
>> > > +             goto i2c_write_err;
>> > > +     usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
>> > > +
>> > > +     ret = i2c_smbus_write_byte_data(client, SILEAD_REG_POWER,
>> > > +                                     SILEAD_CMD_START);
>> > > +     if (ret)
>> > > +             goto i2c_write_err;
>> > > +     usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
>> > > +
>> > > +     return 0;
>> > > +
>> > > +i2c_write_err:
>> > > +     dev_err(&client->dev, "Chip reset error %d\n", ret);
>> > > +     return ret;
>> > > +}
>> > > +
>> > > +static int silead_ts_startup(struct i2c_client *client)
>> > > +{
>> > > +     int ret;
>> > > +
>> > > +     ret = i2c_smbus_write_byte_data(client, SILEAD_REG_RESET, 0x00);
>> > > +     if (ret) {
>> > > +             dev_err(&client->dev, "Startup error %d\n", ret);
>> > > +             return ret;
>> > > +     }
>> > > +     msleep(SILEAD_STARTUP_SLEEP);
>> > > +
>> > > +     return 0;
>> > > +}
>> > > +
>> > > +static int silead_ts_load_fw(struct i2c_client *client)
>> > > +{
>> > > +     struct device *dev = &client->dev;
>> > > +     struct silead_ts_data *data = i2c_get_clientdata(client);
>> > > +     unsigned int fw_size, i;
>> > > +     const struct firmware *fw;
>> > > +     struct silead_fw_data *fw_data;
>> > > +     int ret;
>> > > +
>> > > +     dev_dbg(dev, "Firmware file name: %s", data->fw_name);
>> > > +
>> > > +     if (data->custom_fw_name)
>> > > +             ret = request_firmware(&fw, data->custom_fw_name, dev);
>> > > +     else
>> > > +             ret = request_firmware(&fw, data->fw_name, dev);
>> > > +
>> > > +     if (ret) {
>> > > +             dev_err(dev, "Firmware request error %d\n", ret);
>> > > +             return ret;
>> > > +     }
>> > > +
>> > > +     fw_size = fw->size / sizeof(*fw_data);
>> > > +     fw_data = (struct silead_fw_data *)fw->data;
>> > > +
>> > > +     for (i = 0; i < fw_size; i++) {
>> > > +             ret = i2c_smbus_write_i2c_block_data(client, fw_data[i].offset,
>> > > +                                                  4, (u8 *)&fw_data[i].val);
>> > > +             if (ret) {
>> > > +                     dev_err(dev, "Firmware load error %d\n", ret);
>> > > +                     goto release_fw_err;
>> > > +             }
>> > > +     }
>> > > +
>> > > +     release_firmware(fw);
>> > > +     return 0;
>> > > +
>> > > +release_fw_err:
>> > > +     release_firmware(fw);
>> > > +     return ret;
>> > > +}
>> > > +
>> > > +static u32 silead_ts_get_status(struct i2c_client *client)
>> > > +{
>> > > +     int ret;
>> > > +     u32 status;
>> > > +
>> > > +     ret = i2c_smbus_read_i2c_block_data(client, SILEAD_REG_STATUS, 4,
>> > > +                                         (u8 *)&status);
>> >
>> > Endianness.
>> >
>> > > +     if (ret < 0) {
>> > > +             dev_err(&client->dev, "Status read error %d\n", ret);
>> > > +             return ret;
>> > > +     }
>> > > +
>> > > +     return status;
>> > > +}
>> > > +
>> > > +static int silead_ts_get_id(struct i2c_client *client)
>> > > +{
>> > > +     struct silead_ts_data *data = i2c_get_clientdata(client);
>> > > +     int ret;
>> > > +
>> > > +     ret = i2c_smbus_read_i2c_block_data(client, SILEAD_REG_ID, 4,
>> > > +                                         (u8 *)&data->chip_id);
>> >
>> > Endianness.
>> >
>> > > +     if (ret < 0) {
>> > > +             dev_err(&client->dev, "Chip ID read error %d\n", ret);
>> > > +             return ret;
>> > > +     }
>> > > +
>> > > +     return 0;
>> > > +}
>> > > +
>> > > +static int silead_ts_setup(struct i2c_client *client)
>> > > +{
>> > > +     struct silead_ts_data *data = i2c_get_clientdata(client);
>> > > +     struct device *dev = &client->dev;
>> > > +     int ret;
>> > > +     u32 status;
>> > > +
>> > > +     silead_ts_set_power(client, SILEAD_POWER_OFF);
>> > > +     silead_ts_set_power(client, SILEAD_POWER_ON);
>> > > +
>> > > +     ret = silead_ts_get_id(client);
>> > > +     if (ret)
>> > > +             return ret;
>> > > +     dev_dbg(dev, "Chip ID: 0x%8X", data->chip_id);
>> > > +
>> > > +     ret = silead_ts_init(client);
>> > > +     if (ret)
>> > > +             return ret;
>> > > +
>> > > +     ret = silead_ts_reset(client);
>> > > +     if (ret)
>> > > +             return ret;
>> > > +
>> > > +     ret = silead_ts_load_fw(client);
>> > > +     if (ret)
>> > > +             return ret;
>> > > +
>> > > +     ret = silead_ts_startup(client);
>> > > +     if (ret)
>> > > +             return ret;
>> > > +
>> > > +     status = silead_ts_get_status(client);
>> > > +     if (status != SILEAD_STATUS_OK) {
>> > > +             dev_err(dev, "Initialization error, status: 0x%X\n", status);
>> > > +             return -ENODEV;
>> > > +     }
>> > > +
>> > > +     return 0;
>> > > +}
>> > > +
>> > > +static irqreturn_t silead_ts_threaded_irq_handler(int irq, void *id)
>> > > +{
>> > > +     struct silead_ts_data *data = (struct silead_ts_data *)id;
>> >
>> > No need to cast from void *.
>> >
>> > > +     struct i2c_client *client = data->client;
>> > > +
>> > > +     silead_ts_read_data(client);
>> > > +
>> > > +     return IRQ_HANDLED;
>> > > +}
>> > > +
>> > > +static int silead_ts_read_props(struct i2c_client *client)
>> > > +{
>> > > +     struct silead_ts_data *data = i2c_get_clientdata(client);
>> > > +     struct device *dev = &client->dev;
>> > > +     int ret;
>> > > +
>> > > +     ret = device_property_read_u16(dev, SILEAD_DP_X_MAX, &data->x_max);
>> > > +     if (ret) {
>> > > +             dev_err(dev, "Resolution X read error %d\n", ret);
>> > > +             goto error;
>> > > +     }
>> >
>> > Can we use the standard touchscreen properties as described in
>> > Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt
>> >
>> > > +
>> > > +     ret = device_property_read_u16(dev, SILEAD_DP_Y_MAX, &data->y_max);
>> > > +     if (ret) {
>> > > +             dev_err(dev, "Resolution Y read error %d\n", ret);
>> > > +             goto error;
>> > > +     }
>> > > +
>> > > +     ret = device_property_read_u8(dev, SILEAD_DP_MAX_FINGERS,
>> > > +                                   &data->max_fingers);
>> > > +     if (ret) {
>> > > +             dev_err(dev, "Max fingers read error %d\n", ret);
>> > > +             goto error;
>> > > +     }
>> >
>> > Can you query the controller capabilities to get number of fingers
>> > supported? What controls it? The chip? Firmware?
>>
>> In the documentation from the vendor there is no register for this. I
>> will ask them.
>>
>> > > +
>> > > +     ret = device_property_read_string(dev, SILEAD_DP_FW_NAME,
>> > > +                                       &data->custom_fw_name);
>> > > +     if (ret)
>> > > +             dev_info(dev, "Firmware file name read error. Using default.");
>> >
>> > Why do we need this?
>>
>> Even if there is the same chip, if the panel is different you need a
>> different firmware. The firmware file name defaults to the chip name
>> but can be overwritten.
>
> But when you compose your userland image you can make sure you are
> putting the needed firmware into the right place. There is no really
> need to add the code to kernel when you can rename in userspace.

For example when you have a generic Android system image you have all
the firmware images for all the flavors of the device that can be used
with that build. This can also apply for a linux system that supports
multiple touch screens using the same image.

>>
>> > > +
>> > > +     dev_dbg(dev, "X max = %d, Y max = %d, max fingers = %d",
>> > > +             data->x_max, data->y_max, data->max_fingers);
>> > > +
>> > > +     return 0;
>> > > +
>> > > +error:
>> > > +     return ret;
>> > > +}
>> > > +
>> > > +#ifdef CONFIG_ACPI
>> > > +static const struct acpi_device_id silead_ts_acpi_match[];
>> > > +
>> > > +static int silead_ts_set_default_fw_name(struct silead_ts_data *data,
>> > > +                                      const struct i2c_device_id *id)
>> > > +{
>> > > +     const struct acpi_device_id *acpi_id;
>> > > +     struct device *dev = &data->client->dev;
>> > > +     int i;
>> > > +
>> > > +     if (ACPI_HANDLE(dev)) {
>> > > +             acpi_id = acpi_match_device(silead_ts_acpi_match, dev);
>> > > +             if (!acpi_id)
>> > > +                     return -ENODEV;
>> > > +
>> > > +             sprintf(data->fw_name, "%s.fw", acpi_id->id);
>> > > +
>> > > +             for (i = 0; i < strlen(data->fw_name); i++)
>> > > +                     data->fw_name[i] = tolower(data->fw_name[i]);
>> > > +     } else {
>> > > +             sprintf(data->fw_name, "%s.fw", id->name);
>> > > +     }
>> > > +
>> > > +     return 0;
>> > > +}
>> > > +#else
>> > > +static int silead_ts_set_default_fw_name(struct silead_ts_data *data,
>> > > +                                      const struct i2c_device_id *id)
>> > > +{
>> > > +     sprintf(data->fw_name, "%s.fw", id->name);
>> > > +     return 0;
>> > > +}
>> > > +#endif
>> > > +
>> > > +static int silead_ts_probe(struct i2c_client *client,
>> > > +                        const struct i2c_device_id *id)
>> > > +{
>> > > +     struct silead_ts_data *data;
>> > > +     struct device *dev = &client->dev;
>> > > +     int ret;
>> > > +
>> > > +     if (!i2c_check_functionality(client->adapter,
>> > > +                                  I2C_FUNC_I2C |
>> > > +                                  I2C_FUNC_SMBUS_READ_I2C_BLOCK |
>> > > +                                  I2C_FUNC_SMBUS_WRITE_I2C_BLOCK)) {
>> > > +             dev_err(dev, "I2C functionality check failed\n");
>> > > +             return -ENXIO;
>> > > +     }
>> > > +
>> > > +     data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>> > > +     if (!data)
>> > > +             return -ENOMEM;
>> > > +
>> > > +     i2c_set_clientdata(client, data);
>> > > +     data->client = client;
>> > > +
>> > > +     ret = silead_ts_set_default_fw_name(data, id);
>> > > +     if (ret)
>> > > +             return ret;
>> > > +
>> > > +     /* If the IRQ is not filled by DT or ACPI subsytem
>> > > +      * we can't continue without it */
>> > > +     if (client->irq <= 0)
>> > > +             return -ENODEV;
>> > > +
>> > > +     /* Power GPIO pin */
>> > > +     data->gpio_power = devm_gpiod_get(dev, SILEAD_PWR_GPIO_NAME,
>> > > +                                       GPIOD_OUT_LOW);
>> > > +     if (IS_ERR(data->gpio_power)) {
>> > > +             dev_err(dev, "Shutdown GPIO request failed\n");
>> > > +             return PTR_ERR(data->gpio_power);
>> > > +     }
>> > > +
>> > > +     ret = silead_ts_read_props(client);
>> > > +     if (ret)
>> > > +             return ret;
>> > > +
>> > > +     ret = silead_ts_setup(client);
>> > > +     if (ret)
>> > > +             return ret;
>> > > +
>> > > +     ret = silead_ts_request_input_dev(data);
>> > > +     if (ret)
>> > > +             return ret;
>> > > +
>> > > +     ret = devm_request_threaded_irq(dev, client->irq, NULL,
>> > > +                                     silead_ts_threaded_irq_handler,
>> > > +                                     IRQF_ONESHOT | IRQ_TYPE_EDGE_RISING,
>> > > +                                     client->name, data);
>> > > +     if (ret) {
>> > > +             dev_err(dev, "IRQ request failed %d\n", ret);
>> > > +             return ret;
>> > > +     }
>> > > +
>> > > +     dev_dbg(dev, "Probing succeded\n");
>> > > +     return 0;
>> > > +}
>> > > +
>> > > +#ifdef CONFIG_PM_SLEEP
>> > > +static int silead_ts_suspend(struct device *dev)
>> >
>> > Instead of guarding with ifdef let's annotate with __maybe_unused. This
>> > provides better compile coverage.
>> >
>> > > +{
>> > > +     struct i2c_client *client = to_i2c_client(dev);
>> > > +
>> > > +     silead_ts_set_power(client, SILEAD_POWER_OFF);
>> > > +     return 0;
>> > > +}
>> > > +
>> > > +static int silead_ts_resume(struct device *dev)
>> > > +{
>> > > +     struct i2c_client *client = to_i2c_client(dev);
>> > > +     int ret, status;
>> >
>> > Personal preference: can we call here and elsewhere variables that store
>> > error codes only "error" and not "ret"?
>> >
>> > > +
>> > > +     silead_ts_set_power(client, SILEAD_POWER_ON);
>> > > +
>> > > +     ret = silead_ts_reset(client);
>> > > +     if (ret)
>> > > +             return ret;
>> > > +
>> > > +     ret = silead_ts_startup(client);
>> > > +     if (ret)
>> > > +             return ret;
>> > > +
>> > > +     status = silead_ts_get_status(client);
>> > > +     if (status != SILEAD_STATUS_OK) {
>> > > +             dev_err(dev, "Resume error, status: 0x%X\n", status);
>> > > +             return -ENODEV;
>> > > +     }
>> > > +
>> > > +     return 0;
>> > > +}
>> > > +
>> > > +static SIMPLE_DEV_PM_OPS(silead_ts_pm, silead_ts_suspend, silead_ts_resume);
>> >
>> > Pull it out of ifdef and then you do not need #ifdef in driver
>> > initializer.
>> >
>> > > +#endif
>> > > +
>> > > +static const struct i2c_device_id silead_ts_id[] = {
>> > > +     { "gsl1680", 0 },
>> > > +     { "gsl1688", 0 },
>> > > +     { "gsl3670", 0 },
>> > > +     { "gsl3675", 0 },
>> > > +     { "gsl3692", 0 },
>> > > +     { }
>> > > +};
>> > > +MODULE_DEVICE_TABLE(i2c, silead_ts_id);
>> > > +
>> > > +#ifdef CONFIG_ACPI
>> > > +static const struct acpi_device_id silead_ts_acpi_match[] = {
>> > > +     { "GSL1680", 0 },
>> > > +     { "GSL1688", 0 },
>> > > +     { "GSL3670", 0 },
>> > > +     { "GSL3675", 0 },
>> > > +     { "GSL3692", 0 },
>> > > +     { }
>> > > +};
>> > > +MODULE_DEVICE_TABLE(acpi, silead_ts_acpi_match);
>> > > +#endif
>> > > +
>> > > +static struct i2c_driver silead_ts_driver = {
>> > > +     .probe = silead_ts_probe,
>> > > +     .id_table = silead_ts_id,
>> > > +     .driver = {
>> > > +             .name = SILEAD_TS_NAME,
>> > > +             .owner = THIS_MODULE,
>> > > +#ifdef CONFIG_ACPI
>> > > +             .acpi_match_table = ACPI_PTR(silead_ts_acpi_match),
>> > > +#endif
>> >
>> > No need for ifdef if you are using ACPI_PTR.
>> >
>> > > +#ifdef CONFIG_PM_SLEEP
>> > > +             .pm = &silead_ts_pm,
>> > > +#endif
>> > > +     },
>> > > +};
>> > > +module_i2c_driver(silead_ts_driver);
>> > > +
>> > > +MODULE_AUTHOR("Robert Dolca <robert.dolca@intel.com>");
>> > > +MODULE_DESCRIPTION("Silead I2C touchscreen driver");
>> > > +MODULE_LICENSE("GPL");
>> > > --

Robert