Re: [PATCH v6] input: touchscreen: Add generic driver for Silead touchscreens

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On Tue, Jul 12, 2016 at 10:10:24AM +0200, Hans de Goede wrote:
> Hi,
> 
> On 12-07-16 02:01, Dmitry Torokhov wrote:
> >Hi Hans,
> >
> >On Mon, Jul 11, 2016 at 05:51:44PM +0200, Hans de Goede wrote:
> >>From: Robert Dolca <robert.dolca@xxxxxxxxx>
> >>
> >>This driver adds support for Silead touchscreens. It has been tested
> >>with GSL1680 and GSL3680 touch panels.
> >>
> >>It supports ACPI and device tree enumeration. Screen resolution,
> >>the maximum number of fingers supported and firmware name are
> >>configurable.
> >>
> >>Signed-off-by: Robert Dolca <robert.dolca@xxxxxxxxx>
> >>Signed-off-by: Daniel Jansen <djaniboe@xxxxxxxxx>
> >>Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> >>---
> >>Changes in v6:
> >>-Default to 5 fingers if max-fingers is not specified
> >
> >From the conversations with Robert, IIRC, the number of touches is not
> >really property of a given controller, but rather firmware that is
> >currently loaded into it and theoretically can change. Given this fact,
> >can we simply expect up to 10 contacts and do not use the property at
> >all?
> 
> We actually write the max-fingers value to a register in the
> chip:
> 
> 	error = i2c_smbus_write_byte_data(client, SILEAD_REG_TOUCH_NR,
> 					data->max_fingers);
> 
> So we need the property, because AFAIK we cannot just write 10 for
> "firmware" which only supports 5.

I see.

> 
> Also note there really is not firmware, the "firmware" files contain
> pin configuration data and lookup tables to go from analog pin
> measurements to reported values. One can upgrade a touchscreen from
> say 800x480 resolution to 1024x600 by using another "firmware" as long
> as the 2 use the same pin config (same pins connected to the same
> senselines of the touchscreen).

OK then.

> 
> 
> 
> >
> >>-Improve devicetree binding doc: improve wake-gpios description, use
> >> "See touchscreen.txt" where applicable
> >>---
> >> .../bindings/input/touchscreen/silead_gsl1680.txt  |  36 ++
> >> drivers/input/touchscreen/Kconfig                  |  13 +
> >> drivers/input/touchscreen/Makefile                 |   1 +
> >> drivers/input/touchscreen/silead.c                 | 652 +++++++++++++++++++++
> >> 4 files changed, 702 insertions(+)
> >> create mode 100644 Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt
> >> create mode 100644 drivers/input/touchscreen/silead.c
> >>
> >>diff --git a/Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt b/Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt
> >>new file mode 100644
> >>index 0000000..1e58d37
> >>--- /dev/null
> >>+++ b/Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt
> >>@@ -0,0 +1,36 @@
> >>+* GSL 1680 touchscreen controller
> >>+
> >>+Required properties:
> >>+- compatible		  : "silead,gsl1680"
> >>+- reg			  : I2C slave address of the chip (0x40)
> >>+- interrupt-parent	  : a phandle pointing to the interrupt controller
> >>+			    serving the interrupt for this chip
> >>+- interrupts		  : interrupt specification for the gsl1680 interrupt
> >>+- power-gpios		  : Specification for the pin connected to the gsl1680's
> >>+			    shutdown input. This needs to be driven high to take the
> >>+			    gsl1680 out of its low power state
> >>+- touchscreen-size-x	  : See touchscreen.txt
> >>+- touchscreen-size-y	  : See touchscreen.txt
> >>+- touchscreen-max-fingers : maximum number of fingers the touchscreen can detect
> >>+
> >>+Optional properties:
> >>+- touchscreen-inverted-x  : See touchscreen.txt
> >>+- touchscreen-inverted-y  : See touchscreen.txt
> >>+- touchscreen-swapped-x-y : See touchscreen.txt
> >>+
> >>+Example:
> >>+
> >>+i2c@00000000 {
> >>+	gsl1680: touchscreen@40 {
> >>+		compatible = "silead,gsl1680";
> >>+		reg = <0x40>;
> >>+		interrupt-parent = <&pio>;
> >>+		interrupts = <6 11 IRQ_TYPE_EDGE_FALLING>;
> >>+		power-gpios = <&pio 1 3 GPIO_ACTIVE_HIGH>;
> >>+		touchscreen-size-x = <480>;
> >>+		touchscreen-size-y = <800>;
> >>+		touchscreen-max-fingers = <5>;
> >>+		touchscreen-inverted-x;
> >>+		touchscreen-swapped-x-y;
> >>+	};
> >>+};
> >>diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> >>index 8ecdc38..cdd7c4c 100644
> >>--- a/drivers/input/touchscreen/Kconfig
> >>+++ b/drivers/input/touchscreen/Kconfig
> >>@@ -1155,4 +1155,17 @@ config TOUCHSCREEN_ROHM_BU21023
> >> 	  To compile this driver as a module, choose M here: the
> >> 	  module will be called bu21023_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 f42975e..8ffa0fd 100644
> >>--- a/drivers/input/touchscreen/Makefile
> >>+++ b/drivers/input/touchscreen/Makefile
> >>@@ -95,3 +95,4 @@ obj-$(CONFIG_TOUCHSCREEN_TPS6507X)	+= tps6507x-ts.o
> >> obj-$(CONFIG_TOUCHSCREEN_ZFORCE)	+= zforce_ts.o
> >> obj-$(CONFIG_TOUCHSCREEN_COLIBRI_VF50)	+= colibri-vf50-ts.o
> >> obj-$(CONFIG_TOUCHSCREEN_ROHM_BU21023)	+= rohm_bu21023.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..22d3546
> >>--- /dev/null
> >>+++ b/drivers/input/touchscreen/silead.c
> >>@@ -0,0 +1,652 @@
> >>+/* -------------------------------------------------------------------------
> >>+ * 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_Y_OFF      0x00
> >>+#define SILEAD_POINT_Y_MSB_OFF	0x01
> >>+#define SILEAD_POINT_X_OFF	0x02
> >>+#define SILEAD_POINT_X_MSB_OFF	0x03
> >>+#define SILEAD_POINT_HSB_MASK	0x0F
> >>+#define SILEAD_TOUCH_ID_MASK	0xF0
> >>+
> >>+#define SILEAD_CMD_SLEEP_MIN	10000
> >>+#define SILEAD_CMD_SLEEP_MAX	20000
> >>+#define SILEAD_POWER_SLEEP	20
> >>+#define SILEAD_STARTUP_SLEEP	30
> >>+
> >>+#define SILEAD_MAX_FINGERS	10
> >>+#define SILEAD_MAX_X		4096
> >>+#define SILEAD_MAX_Y		4096
> >>+
> >>+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];
> >>+	u32 x_max;
> >>+	u32 y_max;
> >>+	u32 max_fingers;
> >>+	bool x_invert;
> >>+	bool y_invert;
> >>+	bool xy_swap;
> >>+	u32 chip_id;
> >>+	struct input_mt_pos pos[SILEAD_MAX_FINGERS];
> >>+	int slots[SILEAD_MAX_FINGERS];
> >>+	int id[SILEAD_MAX_FINGERS];
> >>+};
> >>+
> >>+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 error;
> >>+
> >>+	data->input = devm_input_allocate_device(dev);
> >>+	if (!data->input) {
> >>+		dev_err(dev,
> >>+			"Failed to allocate input device\n");
> >>+		return -ENOMEM;
> >>+	}
> >>+
> >>+	if (!data->xy_swap) {
> >>+		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);
> >>+	} else {
> >>+		input_set_abs_params(data->input, ABS_MT_POSITION_X, 0,
> >>+				     data->y_max, 0, 0);
> >>+		input_set_abs_params(data->input, ABS_MT_POSITION_Y, 0,
> >>+				     data->x_max, 0, 0);
> >>+	}
> >>+
> >>+	input_mt_init_slots(data->input, data->max_fingers,
> >>+			    INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED |
> >>+			    INPUT_MT_TRACK);
> >>+
> >>+	data->input->name = SILEAD_TS_NAME;
> >>+	data->input->phys = "input/ts";
> >>+	data->input->id.bustype = BUS_I2C;
> >>+
> >>+	error = input_register_device(data->input);
> >>+	if (error) {
> >>+		dev_err(dev, "Failed to register input device: %d\n", error);
> >>+		return error;
> >>+	}
> >>+
> >>+	return 0;
> >>+}
> >>+
> >>+static void silead_ts_report_touch(struct silead_ts_data *data, u16 x, u16 y,
> >>+				   u8 id)
> >>+{
> >>+	if (data->x_invert)
> >>+		x = data->x_max - x;
> >>+
> >>+	if (data->y_invert)
> >>+		y = data->y_max - y;
> >>+
> >>+	if (data->xy_swap)
> >>+		swap(x, y);
> >>+
> >>+	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);
> >>+
> >>+	if (data->gpio_power) {
> >>+		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, touch_nr, error, i, offset;
> >>+
> >>+	error = i2c_smbus_read_i2c_block_data(client, SILEAD_REG_DATA,
> >>+					    SILEAD_TS_DATA_LEN, buf);
> >>+	if (error < 0) {
> >>+		dev_err(dev, "Data read error %d\n", error);
> >>+		return;
> >>+	}
> >>+
> >>+	touch_nr = buf[0];
> >
> >What is touch NR? I.e. can we use it as slot ID instead of resorting to
> >in-kernel tracking?
> 
> On some models yes, the problem is we do not really know which models so
> we're keeping this info here to try and enable using the hardware
> tracking on later models and use in kernel tracking everywhere now.

OK, I guess we can improve that later if we figure out more about
different models.

> 
> >
> >>+
> >Extra blank line.
> 
> Will fix for the next version (which I will do when we've agreement on
> how to deal with your other remarks).
> 
> >
> >>+	if (touch_nr < 0)
> >>+		return;
> >>+
> >>+	if (touch_nr > data->max_fingers) {
> >>+		dev_warn(dev, "More touches reported then supported %d > %d\n",
> >>+			 touch_nr, data->max_fingers);
> >>+		touch_nr = data->max_fingers;
> >>+	}
> >>+
> >>+	dev_dbg(dev, "Touch number: %d\n", touch_nr);
> >>+
> >>+	for (i = 0; i < touch_nr; i++) {
> >>+		offset = (i + 1) * SILEAD_POINT_DATA_LEN;
> >>+
> >>+		/* Bits 4-7 are the touch id */
> >>+		data->id[i] = (buf[offset + SILEAD_POINT_X_MSB_OFF] &
> >>+			      SILEAD_TOUCH_ID_MASK) >> 4;
> >>+
> >>+		/* Bits 0-3 are MSB of X */
> >>+		buf[offset + SILEAD_POINT_X_MSB_OFF] =
> >>+					buf[offset + SILEAD_POINT_X_MSB_OFF] &
> >>+					SILEAD_POINT_HSB_MASK;
> >>+
> >>+		/* Bits 0-3 are MSB of Y */
> >>+		buf[offset + SILEAD_POINT_Y_MSB_OFF] =
> >>+					buf[offset + SILEAD_POINT_Y_MSB_OFF] &
> >>+					SILEAD_POINT_HSB_MASK;

Why don't you mask off the result of le16_to_pcup() instread og mangling the
buffer?

> >>+
> >>+		y = le16_to_cpup((__le16 *)(buf + offset + SILEAD_POINT_Y_OFF));
> >>+		x = le16_to_cpup((__le16 *)(buf + offset + SILEAD_POINT_X_OFF));
> >>+
> >>+		data->pos[i].x = x;
> >>+		data->pos[i].y = y;

I.e. do

		data->pos[i].x = x & 0x0fff;

> >>+	}
> >>+
> >>+	input_mt_assign_slots(data->input, data->slots, data->pos, touch_nr, 0);
> >>+
> >>+	for (i = 0; i < touch_nr; i++) {
> >>+		silead_ts_report_touch(data, data->pos[i].x, data->pos[i].y,
> >>+				       data->slots[i]);
> >>+
> >>+		dev_dbg(dev, "x=%d y=%d hw_id=%d sw_id=%d\n", data->pos[i].x,
> >>+			data->pos[i].y, data->id[i], data->slots[i]);
> >>+	}
> >>+
> >>+	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 error;
> >>+
> >>+	error = i2c_smbus_write_byte_data(client, SILEAD_REG_RESET,
> >>+					SILEAD_CMD_RESET);
> >>+	if (error)
> >>+		goto i2c_write_err;
> >>+	usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
> >>+
> >>+	error = i2c_smbus_write_byte_data(client, SILEAD_REG_TOUCH_NR,
> >>+					data->max_fingers);
> >>+	if (error)
> >>+		goto i2c_write_err;
> >>+	usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
> >>+
> >>+	error = i2c_smbus_write_byte_data(client, SILEAD_REG_CLOCK,
> >>+					  SILEAD_CLOCK);
> >>+	if (error)
> >>+		goto i2c_write_err;
> >>+	usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
> >>+
> >>+	error = i2c_smbus_write_byte_data(client, SILEAD_REG_RESET,
> >>+					SILEAD_CMD_START);
> >>+	if (error)
> >>+		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", error);
> >>+	return error;
> >>+}
> >>+
> >>+static int silead_ts_reset(struct i2c_client *client)
> >>+{
> >>+	int error;
> >>+
> >>+	error = i2c_smbus_write_byte_data(client, SILEAD_REG_RESET,
> >>+					SILEAD_CMD_RESET);
> >>+	if (error)
> >>+		goto i2c_write_err;
> >>+	usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
> >>+
> >>+	error = i2c_smbus_write_byte_data(client, SILEAD_REG_CLOCK,
> >>+					  SILEAD_CLOCK);
> >>+	if (error)
> >>+		goto i2c_write_err;
> >>+	usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
> >>+
> >>+	error = i2c_smbus_write_byte_data(client, SILEAD_REG_POWER,
> >>+					SILEAD_CMD_START);
> >>+	if (error)
> >>+		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", error);
> >>+	return error;
> >>+}
> >>+
> >>+static int silead_ts_startup(struct i2c_client *client)
> >>+{
> >>+	int error;
> >>+
> >>+	error = i2c_smbus_write_byte_data(client, SILEAD_REG_RESET, 0x00);
> >>+	if (error) {
> >>+		dev_err(&client->dev, "Startup error %d\n", error);
> >>+		return error;
> >>+	}
> >>+	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 error;
> >>+
> >>+	dev_dbg(dev, "Firmware file name: %s", data->fw_name);
> >>+
> >>+	if (data->custom_fw_name)
> >>+		error = request_firmware(&fw, data->custom_fw_name, dev);
> >>+	else
> >>+		error = request_firmware(&fw, data->fw_name, dev);
> >>+
> >>+	if (error) {
> >>+		dev_err(dev, "Firmware request error %d\n", error);
> >>+		return error;
> >>+	}
> >>+
> >>+	fw_size = fw->size / sizeof(*fw_data);
> >>+	fw_data = (struct silead_fw_data *)fw->data;
> >>+
> >>+	for (i = 0; i < fw_size; i++) {
> >>+		error = i2c_smbus_write_i2c_block_data(client,
> >>+						       fw_data[i].offset,
> >>+						       4,
> >>+						       (u8 *)&fw_data[i].val);
> >>+		if (error) {
> >>+			dev_err(dev, "Firmware load error %d\n", error);
> >>+			goto release_fw_err;
> >>+		}
> >>+	}
> >>+
> >>+	release_firmware(fw);
> >>+	return 0;
> >>+
> >>+release_fw_err:
> >>+	release_firmware(fw);

Would love to see firmware being released in one place.

> >>+	return error;
> >>+}
> >>+
> >>+static u32 silead_ts_get_status(struct i2c_client *client)
> >>+{
> >>+	int error;
> >>+	u32 status;
> >>+
> >>+	error = i2c_smbus_read_i2c_block_data(client, SILEAD_REG_STATUS, 4,
> >>+					    (u8 *)&status);
> >>+	if (error < 0) {
> >>+		dev_err(&client->dev, "Status read error %d\n", error);
> >>+		return error;
> >>+	}
> >>+
> >>+	return le32_to_cpu(status);
> >>+}
> >>+
> >>+static int silead_ts_get_id(struct i2c_client *client)
> >>+{
> >>+	struct silead_ts_data *data = i2c_get_clientdata(client);
> >>+	int error;
> >>+
> >>+	error = i2c_smbus_read_i2c_block_data(client, SILEAD_REG_ID, 4,
> >>+					    (u8 *)&data->chip_id);
> >>+
> >>+	data->chip_id = le32_to_cpu(data->chip_id);
> >>+
> >>+	if (error < 0) {
> >>+		dev_err(&client->dev, "Chip ID read error %d\n", error);
> >>+		return error;
> >>+	}
> >>+
> >>+	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 error;
> >>+	u32 status;
> >>+
> >>+	silead_ts_set_power(client, SILEAD_POWER_OFF);
> >>+	silead_ts_set_power(client, SILEAD_POWER_ON);
> >>+
> >>+	error = silead_ts_get_id(client);
> >>+	if (error)
> >>+		return error;
> >>+	dev_info(dev, "Silead chip ID: 0x%8X", data->chip_id);
> >>+
> >>+	error = silead_ts_init(client);
> >>+	if (error)
> >>+		return error;
> >>+
> >>+	error = silead_ts_reset(client);
> >>+	if (error)
> >>+		return error;
> >>+
> >>+	error = silead_ts_load_fw(client);
> >>+	if (error)
> >>+		return error;
> >>+
> >>+	error = silead_ts_startup(client);
> >>+	if (error)
> >>+		return error;
> >>+
> >>+	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 = 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 error;
> >>+
> >>+	error = device_property_read_u32(dev, "touchscreen-size-x",
> >>+					&data->x_max);
> >>+	if (error) {
> >>+		dev_dbg(dev, "Resolution X read error %d\n", error);
> >>+		data->x_max = SILEAD_MAX_X;
> >>+	}
> >>+	data->x_max--; /* Property contains size not max */
> >>+
> >>+	error = device_property_read_u32(dev, "touchscreen-size-y",
> >>+					&data->y_max);
> >>+	if (error) {
> >>+		dev_dbg(dev, "Resolution Y read error %d\n", error);
> >>+		data->y_max = SILEAD_MAX_Y;
> >>+	}
> >>+	data->y_max--; /* Property contains size not max */
> >>+
> >>+	error = device_property_read_u32(dev, "touchscreen-max-fingers",
> >>+					 &data->max_fingers);
> >>+	if (error) {
> >>+		dev_dbg(dev, "Max fingers read error %d\n", error);
> >>+		data->max_fingers = 5; /* Most devices handle up-to 5 fingers */
> >>+	}
> >>+
> >>+	error = device_property_read_string(dev, "touchscreen-fw-name",
> >>+					  &data->custom_fw_name);
> >>+	if (error)
> >>+		dev_dbg(dev, "Firmware file name read error. Using default.");
> >
> >Why do we need separate variable for this?
> 
> As said the firmware is not really firmware, but configuration data and
> each different digitizer (or the same digitizer wired up differently)
> needs different configuration data.

Right, but can you store this info in data->fw_name? You will probably have to
move call to silead_ts_set_default_fw_name() after parsing DT and check for
data->fw_name being non-null... 

> 
> >>+
> >>+	data->x_invert = device_property_read_bool(dev,
> >>+						"touchscreen-inverted-x");
> >>+	data->y_invert = device_property_read_bool(dev,
> >>+						"touchscreen-inverted-y");
> >>+	data->xy_swap = device_property_read_bool(dev,
> >>+						"touchscreen-swapped-x-y");
> >>+
> >>+	dev_dbg(dev, "x_max = %d, y_max = %d, max_fingers = %d, x_invert = %d, y_invert = %d, xy_swap = %d",
> >>+		data->x_max, data->y_max, data->max_fingers, data->x_invert,
> >>+		data->y_invert, data->xy_swap);
> >>+
> >>+	return 0;
> >>+}
> >>+
> >>+#ifdef CONFIG_ACPI
> >>+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(dev->driver->acpi_match_table, dev);
> >>+		if (!acpi_id)
> >>+			return -ENODEV;
> >>+
> >>+		snprintf(data->fw_name, sizeof(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 {
> >>+		snprintf(data->fw_name, sizeof(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)
> >>+{
> >>+	snprintf(data->fw_name, sizeof(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 error;
> >>+
> >>+	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;
> >>+
> >>+	error = silead_ts_set_default_fw_name(data, id);
> >>+	if (error)
> >>+		return error;
> >>+
> >>+	/* We must have the IRQ provided by DT or ACPI subsytem */
> >>+	if (client->irq <= 0)
> >>+		return -ENODEV;
> >>+
> >>+	/* Power GPIO pin */
> >>+	data->gpio_power = devm_gpiod_get_index(dev, "power",
> >>+						0, GPIOD_OUT_LOW);
> >
> >Why "get index" and not devm_gpiod_get_optional()?
> >
> >>+	if (IS_ERR(data->gpio_power)) {
> >>+		dev_dbg(dev, "Shutdown GPIO request failed\n");
> >>+		data->gpio_power = NULL;
> >
> >You want to abort on errors, especially if you get -EPROBE_DEFER.
> >
> >>+	}
> >>+
> >>+	error = silead_ts_read_props(client);
> >>+	if (error)
> >>+		return error;
> >>+
> >>+	error = silead_ts_setup(client);
> >>+	if (error)
> >>+		return error;
> >>+
> >>+	error = silead_ts_request_input_dev(data);
> >>+	if (error)
> >>+		return error;
> >>+
> >>+	error = devm_request_threaded_irq(dev, client->irq, NULL,
> >>+					silead_ts_threaded_irq_handler,
> >>+					IRQF_ONESHOT | IRQ_TYPE_EDGE_RISING,
> >
> >Please just use IRQF_ONESHOT and rely on platform code (ACPI. DT, etc)
> >to specify interrupt trigger.
> 
> Ack, will fix.
> 
> >>+					client->name, data);
> >>+	if (error) {
> >>+		dev_err(dev, "IRQ request failed %d\n", error);
> >>+		return error;
> >>+	}
> >>+
> >>+	dev_dbg(dev, "Probing succeded\n");
> >
> >You'd see if it failed... Just drop.
> 
> Ack, will fix.
> 
> >>+	return 0;
> >>+}
> >>+
> >>+static int __maybe_unused 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 __maybe_unused silead_ts_resume(struct device *dev)
> >>+{
> >>+	struct i2c_client *client = to_i2c_client(dev);
> >>+	int error, status;
> >>+
> >>+	silead_ts_set_power(client, SILEAD_POWER_ON);
> >>+
> >>+	error = silead_ts_reset(client);
> >>+	if (error)
> >>+		return error;
> >>+
> >>+	error = silead_ts_startup(client);
> >>+	if (error)
> >>+		return error;
> >>+
> >>+	status = silead_ts_get_status(client);
> >>+	if (status != SILEAD_STATUS_OK) {
> >>+		dev_err(dev, "Resume error, status: 0x%X\n", status);
> >
> >"%#02X"
> 
> Ack, will fix.
> 
> >>+		return -ENODEV;
> >>+	}
> >>+
> >>+	return 0;
> >>+}
> >>+
> >>+static SIMPLE_DEV_PM_OPS(silead_ts_pm, silead_ts_suspend, silead_ts_resume);
> >>+
> >>+static const struct i2c_device_id silead_ts_id[] = {
> >>+	{ "gsl1680", 0 },
> >>+	{ "gsl1688", 0 },
> >>+	{ "gsl3670", 0 },
> >>+	{ "gsl3675", 0 },
> >>+	{ "gsl3692", 0 },
> >>+	{ "mssl1680", 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 },
> >>+	{ "MSSL1680", 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,
> >>+		.acpi_match_table = ACPI_PTR(silead_ts_acpi_match),
> >>+		.pm = &silead_ts_pm,
> >>+	},
> >>+};
> >>+module_i2c_driver(silead_ts_driver);
> >>+
> >>+MODULE_AUTHOR("Robert Dolca <robert.dolca@xxxxxxxxx>");
> >>+MODULE_DESCRIPTION("Silead I2C touchscreen driver");
> >>+MODULE_LICENSE("GPL");
> >>--
> >>2.7.4
> >>
> >
> >Thanks.
> 
> Please let me know if you're happy with my answers to your remarks /
> questions which I've not marked as "will fix" and then I'll do a v7.

Yes I am for the most part, plus I had a couple of more comments above.

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux