[RESEND PATCH v2 0/8] Input: sx8654 - reset-gpio, sx865[056] support, etc.

[RESEND PATCH v2 0/8] Input: sx8654 - reset-gpio, sx865[056] support, etc.

[Richard Leitner]

Add reset-gpio, sx8654[056] and common of_touchscreen functions support
for the sx8654 driver.

Changes RESEND v2:
	- added "Reviewed-by: Rob Herring <robh@kernel.org>" tags

Changes v2:
	- use devm_gpiod_get_optional in probe instead of in #ifdef CONFIG_OF
	- convert flags to BIT() in a separate patch
	- replace hrtimer with "regular" timer
	- use of_device_get_match_data instead of of_match_device
	- add driver data to i2c_device_id table for non-DT fallback
	- fix sequence of common touchscreen initialization
	- div. minor stlye changes

Richard Leitner (8):
  dt-bindings: input: touchscreen: sx8654: add reset-gpio property
  Input: sx8654 - add reset-gpio support
  dt-bindings: input: touchscreen: sx8654: add compatible models
  Input: sx8654 - add sx8655 and sx8656 to compatibles
  dt-bindings: input: touchscreen: sx8654: add sx8650 to comatibles
  Input: sx8654 - add sx8650 support
  Input: sx8654 - use common of_touchscreen functions
  Input: sx8654 - convert #defined flags to BIT(x)

 .../bindings/input/touchscreen/sx8654.txt          |  10 +-
 drivers/input/touchscreen/sx8654.c                 | 245 ++++++++++++++++++---
 2 files changed, 229 insertions(+), 26 deletions(-)

-- 
2.11.0

[RESEND PATCH v2 1/8] dt-bindings: input: touchscreen: sx8654: add reset-gpio property

[Richard Leitner]

Document the reset-gpio property for the sx8654 touchscreen controller
driver.

Signed-off-by: Richard Leitner <richard.leitner@skidata.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 Documentation/devicetree/bindings/input/touchscreen/sx8654.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/input/touchscreen/sx8654.txt b/Documentation/devicetree/bindings/input/touchscreen/sx8654.txt
index 4886c4aa2906..ca521d8f7d65 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/sx8654.txt
+++ b/Documentation/devicetree/bindings/input/touchscreen/sx8654.txt
@@ -5,6 +5,9 @@ Required properties:
 - reg: i2c slave address
 - interrupts: touch controller interrupt
 
+Optional properties:
+ - reset-gpios: GPIO specification for the NRST input
+
 Example:
 
 	sx8654@48 {
@@ -12,4 +15,5 @@ Example:
 		reg = <0x48>;
 		interrupt-parent = <&gpio6>;
 		interrupts = <3 IRQ_TYPE_EDGE_FALLING>;
+		reset-gpios = <&gpio4 2 GPIO_ACTIVE_LOW>;
 	};
-- 
2.11.0

[RESEND PATCH v2 2/8] Input: sx8654 - add reset-gpio support

[Richard Leitner]

The sx8654 features a NRST input which may be connected to a GPIO.
Therefore add support for hard-resetting the sx8654 via this NRST.

If the reset-gpio property is provided the sx8654 is resetted via NRST
instead of the soft-reset via I2C.

Signed-off-by: Richard Leitner <richard.leitner@skidata.com>
---
 drivers/input/touchscreen/sx8654.c | 40 +++++++++++++++++++++++++++++++++++---
 1 file changed, 37 insertions(+), 3 deletions(-)

diff --git a/drivers/input/touchscreen/sx8654.c b/drivers/input/touchscreen/sx8654.c
index ed29db3ec731..238f56b1581b 100644
--- a/drivers/input/touchscreen/sx8654.c
+++ b/drivers/input/touchscreen/sx8654.c
@@ -33,6 +33,8 @@
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
 #include <linux/irq.h>
+#include <linux/gpio/consumer.h>
+#include <linux/delay.h>
 
 /* register addresses */
 #define I2C_REG_TOUCH0			0x00
@@ -74,6 +76,7 @@
 struct sx8654 {
 	struct input_dev *input;
 	struct i2c_client *client;
+	struct gpio_desc *gpio_reset;
 };
 
 static irqreturn_t sx8654_irq(int irq, void *handle)
@@ -124,6 +127,27 @@ static irqreturn_t sx8654_irq(int irq, void *handle)
 	return IRQ_HANDLED;
 }
 
+static int sx8654_reset(struct sx8654 *ts)
+{
+	int err;
+
+	if (ts->gpio_reset) {
+		gpiod_set_value_cansleep(ts->gpio_reset, 1);
+		udelay(2); /* Tpulse > 1µs */
+		gpiod_set_value_cansleep(ts->gpio_reset, 0);
+
+		return 0;
+	}
+
+	dev_dbg(&ts->client->dev, "NRST unavailable, try softreset\n");
+	err = i2c_smbus_write_byte_data(ts->client, I2C_REG_SOFTRESET,
+					SOFTRESET_VALUE);
+	if (err)
+		return err;
+	else
+		return 0;
+}
+
 static int sx8654_open(struct input_dev *dev)
 {
 	struct sx8654 *sx8654 = input_get_drvdata(dev);
@@ -186,6 +210,17 @@ static int sx8654_probe(struct i2c_client *client,
 	if (!sx8654)
 		return -ENOMEM;
 
+	sx8654->gpio_reset = devm_gpiod_get_optional(&client->dev, "reset",
+						     GPIOD_OUT_HIGH);
+	if (IS_ERR(sx8654->gpio_reset)) {
+		error = PTR_ERR(sx8654->gpio_reset);
+		if (error != -EPROBE_DEFER)
+			dev_err(&client->dev, "unable to get reset-gpio: %d\n",
+				error);
+		return error;
+	}
+	dev_dbg(&client->dev, "got GPIO reset pin\n");
+
 	input = devm_input_allocate_device(&client->dev);
 	if (!input)
 		return -ENOMEM;
@@ -206,10 +241,9 @@ static int sx8654_probe(struct i2c_client *client,
 
 	input_set_drvdata(sx8654->input, sx8654);
 
-	error = i2c_smbus_write_byte_data(client, I2C_REG_SOFTRESET,
-					  SOFTRESET_VALUE);
+	error = sx8654_reset(sx8654);
 	if (error) {
-		dev_err(&client->dev, "writing softreset value failed");
+		dev_err(&client->dev, "reset failed");
 		return error;
 	}
 
-- 
2.11.0

Re: [RESEND PATCH v2 2/8] Input: sx8654 - add reset-gpio support

[Dmitry Torokhov]

On Tue, Dec 18, 2018 at 09:36:00AM +0100, Richard Leitner wrote:
> The sx8654 features a NRST input which may be connected to a GPIO.
> Therefore add support for hard-resetting the sx8654 via this NRST.
> 
> If the reset-gpio property is provided the sx8654 is resetted via NRST
> instead of the soft-reset via I2C.
> 
> Signed-off-by: Richard Leitner <richard.leitner@skidata.com>
> ---
>  drivers/input/touchscreen/sx8654.c | 40 +++++++++++++++++++++++++++++++++++---
>  1 file changed, 37 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/sx8654.c b/drivers/input/touchscreen/sx8654.c
> index ed29db3ec731..238f56b1581b 100644
> --- a/drivers/input/touchscreen/sx8654.c
> +++ b/drivers/input/touchscreen/sx8654.c
> @@ -33,6 +33,8 @@
>  #include <linux/i2c.h>
>  #include <linux/interrupt.h>
>  #include <linux/irq.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/delay.h>
>  
>  /* register addresses */
>  #define I2C_REG_TOUCH0			0x00
> @@ -74,6 +76,7 @@
>  struct sx8654 {
>  	struct input_dev *input;
>  	struct i2c_client *client;
> +	struct gpio_desc *gpio_reset;
>  };
>  
>  static irqreturn_t sx8654_irq(int irq, void *handle)
> @@ -124,6 +127,27 @@ static irqreturn_t sx8654_irq(int irq, void *handle)
>  	return IRQ_HANDLED;
>  }
>  
> +static int sx8654_reset(struct sx8654 *ts)
> +{
> +	int err;
> +
> +	if (ts->gpio_reset) {
> +		gpiod_set_value_cansleep(ts->gpio_reset, 1);
> +		udelay(2); /* Tpulse > 1µs */
> +		gpiod_set_value_cansleep(ts->gpio_reset, 0);
> +
> +		return 0;

I rearranged code a bit to avoid this early return and applied. Thank
you.

> +	}
> +
> +	dev_dbg(&ts->client->dev, "NRST unavailable, try softreset\n");
> +	err = i2c_smbus_write_byte_data(ts->client, I2C_REG_SOFTRESET,
> +					SOFTRESET_VALUE);
> +	if (err)
> +		return err;
> +	else
> +		return 0;
> +}
> +
>  static int sx8654_open(struct input_dev *dev)
>  {
>  	struct sx8654 *sx8654 = input_get_drvdata(dev);
> @@ -186,6 +210,17 @@ static int sx8654_probe(struct i2c_client *client,
>  	if (!sx8654)
>  		return -ENOMEM;
>  
> +	sx8654->gpio_reset = devm_gpiod_get_optional(&client->dev, "reset",
> +						     GPIOD_OUT_HIGH);
> +	if (IS_ERR(sx8654->gpio_reset)) {
> +		error = PTR_ERR(sx8654->gpio_reset);
> +		if (error != -EPROBE_DEFER)
> +			dev_err(&client->dev, "unable to get reset-gpio: %d\n",
> +				error);
> +		return error;
> +	}
> +	dev_dbg(&client->dev, "got GPIO reset pin\n");
> +
>  	input = devm_input_allocate_device(&client->dev);
>  	if (!input)
>  		return -ENOMEM;
> @@ -206,10 +241,9 @@ static int sx8654_probe(struct i2c_client *client,
>  
>  	input_set_drvdata(sx8654->input, sx8654);
>  
> -	error = i2c_smbus_write_byte_data(client, I2C_REG_SOFTRESET,
> -					  SOFTRESET_VALUE);
> +	error = sx8654_reset(sx8654);
>  	if (error) {
> -		dev_err(&client->dev, "writing softreset value failed");
> +		dev_err(&client->dev, "reset failed");
>  		return error;
>  	}
>  
> -- 
> 2.11.0
> 

-- 
Dmitry

[RESEND PATCH v2 3/8] dt-bindings: input: touchscreen: sx8654: add compatible models

[Richard Leitner]

As the sx865[456] share the same datasheet and differ only in the
presence of a "capacitive proximity detection circuit" and a "haptics
motor driver for LRA/ERM" add them to the compatbiles. As the driver
doesn't implement these features it should be no problem.

Signed-off-by: Richard Leitner <richard.leitner@skidata.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 Documentation/devicetree/bindings/input/touchscreen/sx8654.txt | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/input/touchscreen/sx8654.txt b/Documentation/devicetree/bindings/input/touchscreen/sx8654.txt
index ca521d8f7d65..a538678424dd 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/sx8654.txt
+++ b/Documentation/devicetree/bindings/input/touchscreen/sx8654.txt
@@ -1,7 +1,10 @@
 * Semtech SX8654 I2C Touchscreen Controller
 
 Required properties:
-- compatible: must be "semtech,sx8654"
+- compatible: must be one of the following, depending on the model:
+	"semtech,sx8654"
+	"semtech,sx8655"
+	"semtech,sx8656"
 - reg: i2c slave address
 - interrupts: touch controller interrupt
 
-- 
2.11.0

[RESEND PATCH v2 4/8] Input: sx8654 - add sx8655 and sx8656 to compatibles

[Richard Leitner]

As the sx865[456] share the same datasheet and differ only in the
presence of a "capacitive proximity detection circuit" and a "haptics
motor driver for LRA/ERM" add them to the compatbiles. As the driver
doesn't implement these features it should be no problem.

Signed-off-by: Richard Leitner <richard.leitner@skidata.com>
---
 drivers/input/touchscreen/sx8654.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/input/touchscreen/sx8654.c b/drivers/input/touchscreen/sx8654.c
index 238f56b1581b..afa4da138fe9 100644
--- a/drivers/input/touchscreen/sx8654.c
+++ b/drivers/input/touchscreen/sx8654.c
@@ -293,6 +293,8 @@ static int sx8654_probe(struct i2c_client *client,
 #ifdef CONFIG_OF
 static const struct of_device_id sx8654_of_match[] = {
 	{ .compatible = "semtech,sx8654", },
+	{ .compatible = "semtech,sx8655", },
+	{ .compatible = "semtech,sx8656", },
 	{ },
 };
 MODULE_DEVICE_TABLE(of, sx8654_of_match);
@@ -300,6 +302,8 @@ MODULE_DEVICE_TABLE(of, sx8654_of_match);
 
 static const struct i2c_device_id sx8654_id_table[] = {
 	{ "semtech_sx8654", 0 },
+	{ "semtech_sx8655", 0 },
+	{ "semtech_sx8656", 0 },
 	{ },
 };
 MODULE_DEVICE_TABLE(i2c, sx8654_id_table);
-- 
2.11.0

Re: [RESEND PATCH v2 4/8] Input: sx8654 - add sx8655 and sx8656 to compatibles

[Dmitry Torokhov]

On Tue, Dec 18, 2018 at 09:36:02AM +0100, Richard Leitner wrote:
> As the sx865[456] share the same datasheet and differ only in the
> presence of a "capacitive proximity detection circuit" and a "haptics
> motor driver for LRA/ERM" add them to the compatbiles. As the driver
> doesn't implement these features it should be no problem.
> 
> Signed-off-by: Richard Leitner <richard.leitner@skidata.com>

Applied, thank you.

> ---
>  drivers/input/touchscreen/sx8654.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/input/touchscreen/sx8654.c b/drivers/input/touchscreen/sx8654.c
> index 238f56b1581b..afa4da138fe9 100644
> --- a/drivers/input/touchscreen/sx8654.c
> +++ b/drivers/input/touchscreen/sx8654.c
> @@ -293,6 +293,8 @@ static int sx8654_probe(struct i2c_client *client,
>  #ifdef CONFIG_OF
>  static const struct of_device_id sx8654_of_match[] = {
>  	{ .compatible = "semtech,sx8654", },
> +	{ .compatible = "semtech,sx8655", },
> +	{ .compatible = "semtech,sx8656", },
>  	{ },
>  };
>  MODULE_DEVICE_TABLE(of, sx8654_of_match);
> @@ -300,6 +302,8 @@ MODULE_DEVICE_TABLE(of, sx8654_of_match);
>  
>  static const struct i2c_device_id sx8654_id_table[] = {
>  	{ "semtech_sx8654", 0 },
> +	{ "semtech_sx8655", 0 },
> +	{ "semtech_sx8656", 0 },
>  	{ },
>  };
>  MODULE_DEVICE_TABLE(i2c, sx8654_id_table);
> -- 
> 2.11.0
> 

-- 
Dmitry

[RESEND PATCH v2 5/8] dt-bindings: input: touchscreen: sx8654: add sx8650 to comatibles

[Richard Leitner]

As the sx8650 is quite similar to the sx8654 support for it will be
added in the driver. Therefore add it to the compatibles.

Signed-off-by: Richard Leitner <richard.leitner@skidata.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 Documentation/devicetree/bindings/input/touchscreen/sx8654.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/input/touchscreen/sx8654.txt b/Documentation/devicetree/bindings/input/touchscreen/sx8654.txt
index a538678424dd..0ebe6dd043c7 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/sx8654.txt
+++ b/Documentation/devicetree/bindings/input/touchscreen/sx8654.txt
@@ -2,6 +2,7 @@
 
 Required properties:
 - compatible: must be one of the following, depending on the model:
+	"semtech,sx8650"
 	"semtech,sx8654"
 	"semtech,sx8655"
 	"semtech,sx8656"
-- 
2.11.0

[RESEND PATCH v2 6/8] Input: sx8654 - add sx8650 support

[Richard Leitner]

The sx8654 and sx8650 are quite similar, therefore add support for the
sx8650 within the sx8654 driver.

Signed-off-by: Richard Leitner <richard.leitner@skidata.com>
---
 drivers/input/touchscreen/sx8654.c | 193 +++++++++++++++++++++++++++++++++----
 1 file changed, 173 insertions(+), 20 deletions(-)

diff --git a/drivers/input/touchscreen/sx8654.c b/drivers/input/touchscreen/sx8654.c
index afa4da138fe9..4939863efbef 100644
--- a/drivers/input/touchscreen/sx8654.c
+++ b/drivers/input/touchscreen/sx8654.c
@@ -29,7 +29,7 @@
 
 #include <linux/input.h>
 #include <linux/module.h>
-#include <linux/of.h>
+#include <linux/of_device.h>
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
 #include <linux/irq.h>
@@ -44,9 +44,11 @@
 #define I2C_REG_IRQSRC			0x23
 #define I2C_REG_SOFTRESET		0x3f
 
+#define I2C_REG_SX8650_STAT		0x05
+#define SX8650_STAT_CONVIRQ		0x80
+
 /* commands */
 #define CMD_READ_REGISTER		0x40
-#define CMD_MANUAL			0xc0
 #define CMD_PENTRG			0xe0
 
 /* value for I2C_REG_SOFTRESET */
@@ -58,6 +60,7 @@
 
 /* bits for RegTouch1 */
 #define CONDIRQ				0x20
+#define RPDNT_100K			0x00
 #define FILT_7SA			0x03
 
 /* bits for I2C_REG_CHANMASK */
@@ -71,14 +74,122 @@
 /* power delay: lower nibble of CTRL0 register */
 #define POWDLY_1_1MS			0x0b
 
+/* for sx8650, as we have no pen release IRQ there: timeout in ns following the
+ * last PENIRQ after which we assume the pen is lifted.
+ */
+#define SX8650_PENIRQ_TIMEOUT		msecs_to_jiffies(10)
+
 #define MAX_12BIT			((1 << 12) - 1)
+#define MAX_I2C_READ_LEN		10 /* see datasheet section 5.1.5 */
+
+/* channel definition */
+#define CH_X				0x00
+#define CH_Y				0x01
+
+struct sx865x_data {
+	u8 cmd_manual;
+	u8 chan_mask;
+	u8 has_irq_penrelease;
+	u8 has_reg_irqmask;
+	irq_handler_t irqh;
+};
 
 struct sx8654 {
 	struct input_dev *input;
 	struct i2c_client *client;
 	struct gpio_desc *gpio_reset;
+
+	spinlock_t		lock; /* for input reporting from irq/timer */
+	struct timer_list	timer;
+
+	const struct sx865x_data *data;
 };
 
+static inline void sx865x_penrelease(struct sx8654 *ts)
+{
+	struct input_dev *input_dev = ts->input;
+
+	input_report_key(input_dev, BTN_TOUCH, 0);
+	input_sync(input_dev);
+}
+
+static void sx865x_penrelease_timer_handler(struct timer_list *t)
+{
+	struct sx8654 *ts = from_timer(ts, t, timer);
+	unsigned long flags;
+
+	spin_lock_irqsave(&ts->lock, flags);
+	sx865x_penrelease(ts);
+	spin_unlock_irqrestore(&ts->lock, flags);
+	dev_dbg(&ts->client->dev, "penrelease by timer\n");
+}
+
+static irqreturn_t sx8650_irq(int irq, void *handle)
+{
+	struct sx8654 *ts = handle;
+	struct device *dev = &ts->client->dev;
+	int len, i;
+	unsigned long flags;
+	u8 stat;
+	u16 x, y;
+	u8 data[MAX_I2C_READ_LEN];
+	u16 ch;
+	u16 chdata;
+	u8 readlen = hweight32(ts->data->chan_mask) * 2;
+
+	stat = i2c_smbus_read_byte_data(ts->client, CMD_READ_REGISTER
+						    | I2C_REG_SX8650_STAT);
+
+	if (!(stat & SX8650_STAT_CONVIRQ)) {
+		dev_dbg(dev, "%s ignore stat [0x%02x]", __func__, stat);
+		return IRQ_HANDLED;
+	}
+
+	len = i2c_master_recv(ts->client, data, readlen);
+	if (len != readlen) {
+		dev_dbg(dev, "ignore short recv (%d)\n", len);
+		return IRQ_HANDLED;
+	}
+
+	spin_lock_irqsave(&ts->lock, flags);
+
+	x = 0;
+	y = 0;
+	for (i = 0; (i + 1) < len; i++) {
+		chdata = data[i] << 8;
+		i++;
+		chdata += data[i];
+
+		if (unlikely(chdata == 0xFFFF)) {
+			dev_dbg(dev, "invalid qualified data @ %d\n", i);
+			continue;
+		} else if (unlikely(chdata & 0x8000)) {
+			dev_warn(dev, "hibit @ %d [0x%04x]\n", i, chdata);
+			continue;
+		}
+
+		ch = chdata >> 12;
+		if      (ch == CH_X)
+			x = chdata & MAX_12BIT;
+		else if (ch == CH_Y)
+			y = chdata & MAX_12BIT;
+		else
+			dev_warn(dev, "unknown channel %d [0x%04x]\n", ch,
+				 chdata);
+	}
+
+	input_report_abs(ts->input, ABS_X, x);
+	input_report_abs(ts->input, ABS_Y, y);
+	input_report_key(ts->input, BTN_TOUCH, 1);
+	input_sync(ts->input);
+	dev_dbg(dev, "point(%4d,%4d)\n", x, y);
+
+	mod_timer(&ts->timer, jiffies + SX8650_PENIRQ_TIMEOUT);
+	spin_unlock_irqrestore(&ts->lock, flags);
+
+	return IRQ_HANDLED;
+}
+
 static irqreturn_t sx8654_irq(int irq, void *handle)
 {
 	struct sx8654 *sx8654 = handle;
@@ -127,6 +238,22 @@ static irqreturn_t sx8654_irq(int irq, void *handle)
 	return IRQ_HANDLED;
 }
 
+static const struct sx865x_data sx8650_data = {
+	.cmd_manual = 0xb0,
+	.has_irq_penrelease = 0,
+	.has_reg_irqmask = 0,
+	.chan_mask = (CONV_X | CONV_Y),
+	.irqh = sx8650_irq,
+};
+
+static const struct sx865x_data sx8654_data = {
+	.cmd_manual = 0xc0,
+	.has_irq_penrelease = 1,
+	.has_reg_irqmask = 1,
+	.chan_mask = (CONV_X | CONV_Y),
+	.irqh = sx8654_irq,
+};
+
 static int sx8654_reset(struct sx8654 *ts)
 {
 	int err;
@@ -182,13 +309,13 @@ static void sx8654_close(struct input_dev *dev)
 	disable_irq(client->irq);
 
 	/* enable manual mode mode */
-	error = i2c_smbus_write_byte(client, CMD_MANUAL);
+	error = i2c_smbus_write_byte(client, sx8654->data->cmd_manual);
 	if (error) {
 		dev_err(&client->dev, "writing command CMD_MANUAL failed");
 		return;
 	}
 
-	error = i2c_smbus_write_byte_data(client, I2C_REG_TOUCH0, 0);
+	error = i2c_smbus_write_byte_data(client, I2C_REG_TOUCH0, RATE_MANUAL);
 	if (error) {
 		dev_err(&client->dev, "writing to I2C_REG_TOUCH0 failed");
 		return;
@@ -221,6 +348,20 @@ static int sx8654_probe(struct i2c_client *client,
 	}
 	dev_dbg(&client->dev, "got GPIO reset pin\n");
 
+	sx8654->data = of_device_get_match_data(&client->dev);
+	if (!sx8654->data)
+		sx8654->data = (const struct sx865x_data *)id->driver_data;
+	if (!sx8654->data) {
+		dev_err(&client->dev, "invalid or missing device data\n");
+		return -EINVAL;
+	}
+
+	if (!sx8654->data->has_irq_penrelease) {
+		dev_dbg(&client->dev, "use timer for penrelease\n");
+		timer_setup(&sx8654->timer, sx865x_penrelease_timer_handler, 0);
+		spin_lock_init(&sx8654->lock);
+	}
+
 	input = devm_input_allocate_device(&client->dev);
 	if (!input)
 		return -ENOMEM;
@@ -248,29 +389,31 @@ static int sx8654_probe(struct i2c_client *client,
 	}
 
 	error = i2c_smbus_write_byte_data(client, I2C_REG_CHANMASK,
-					  CONV_X | CONV_Y);
+					  sx8654->data->chan_mask);
 	if (error) {
 		dev_err(&client->dev, "writing to I2C_REG_CHANMASK failed");
 		return error;
 	}
 
-	error = i2c_smbus_write_byte_data(client, I2C_REG_IRQMASK,
-					  IRQ_PENTOUCH_TOUCHCONVDONE |
-						IRQ_PENRELEASE);
-	if (error) {
-		dev_err(&client->dev, "writing to I2C_REG_IRQMASK failed");
-		return error;
+	if (sx8654->data->has_reg_irqmask) {
+		error = i2c_smbus_write_byte_data(client, I2C_REG_IRQMASK,
+						  IRQ_PENTOUCH_TOUCHCONVDONE |
+							IRQ_PENRELEASE);
+		if (error) {
+			dev_err(&client->dev, "writing I2C_REG_IRQMASK failed");
+			return error;
+		}
 	}
 
 	error = i2c_smbus_write_byte_data(client, I2C_REG_TOUCH1,
-					  CONDIRQ | FILT_7SA);
+					  CONDIRQ | RPDNT_100K | FILT_7SA);
 	if (error) {
 		dev_err(&client->dev, "writing to I2C_REG_TOUCH1 failed");
 		return error;
 	}
 
 	error = devm_request_threaded_irq(&client->dev, client->irq,
-					  NULL, sx8654_irq,
+					  NULL, sx8654->data->irqh,
 					  IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
 					  client->name, sx8654);
 	if (error) {
@@ -292,18 +435,28 @@ static int sx8654_probe(struct i2c_client *client,
 
 #ifdef CONFIG_OF
 static const struct of_device_id sx8654_of_match[] = {
-	{ .compatible = "semtech,sx8654", },
-	{ .compatible = "semtech,sx8655", },
-	{ .compatible = "semtech,sx8656", },
-	{ },
+	{
+		.compatible = "semtech,sx8650",
+		.data = &sx8650_data,
+	}, {
+		.compatible = "semtech,sx8654",
+		.data = &sx8654_data,
+	}, {
+		.compatible = "semtech,sx8655",
+		.data = &sx8654_data,
+	}, {
+		.compatible = "semtech,sx8656",
+		.data = &sx8654_data,
+	}, {},
 };
 MODULE_DEVICE_TABLE(of, sx8654_of_match);
 #endif
 
 static const struct i2c_device_id sx8654_id_table[] = {
-	{ "semtech_sx8654", 0 },
-	{ "semtech_sx8655", 0 },
-	{ "semtech_sx8656", 0 },
+	{ .name = "semtech_sx8650", .driver_data = (long)&sx8650_data },
+	{ .name = "semtech_sx8654", .driver_data = (long)&sx8654_data },
+	{ .name = "semtech_sx8655", .driver_data = (long)&sx8654_data },
+	{ .name = "semtech_sx8656", .driver_data = (long)&sx8654_data },
 	{ },
 };
 MODULE_DEVICE_TABLE(i2c, sx8654_id_table);
-- 
2.11.0

Re: [RESEND PATCH v2 6/8] Input: sx8654 - add sx8650 support

[Dmitry Torokhov]

Hi Richard,

On Tue, Dec 18, 2018 at 09:39:31AM +0100, Richard Leitner wrote:
> The sx8654 and sx8650 are quite similar, therefore add support for the
> sx8650 within the sx8654 driver.
> 
> Signed-off-by: Richard Leitner <richard.leitner@skidata.com>
> ---
>  drivers/input/touchscreen/sx8654.c | 193 +++++++++++++++++++++++++++++++++----
>  1 file changed, 173 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/sx8654.c b/drivers/input/touchscreen/sx8654.c
> index afa4da138fe9..4939863efbef 100644
> --- a/drivers/input/touchscreen/sx8654.c
> +++ b/drivers/input/touchscreen/sx8654.c
> @@ -29,7 +29,7 @@
>  
>  #include <linux/input.h>
>  #include <linux/module.h>
> -#include <linux/of.h>
> +#include <linux/of_device.h>
>  #include <linux/i2c.h>
>  #include <linux/interrupt.h>
>  #include <linux/irq.h>
> @@ -44,9 +44,11 @@
>  #define I2C_REG_IRQSRC			0x23
>  #define I2C_REG_SOFTRESET		0x3f
>  
> +#define I2C_REG_SX8650_STAT		0x05
> +#define SX8650_STAT_CONVIRQ		0x80
> +
>  /* commands */
>  #define CMD_READ_REGISTER		0x40
> -#define CMD_MANUAL			0xc0
>  #define CMD_PENTRG			0xe0
>  
>  /* value for I2C_REG_SOFTRESET */
> @@ -58,6 +60,7 @@
>  
>  /* bits for RegTouch1 */
>  #define CONDIRQ				0x20
> +#define RPDNT_100K			0x00
>  #define FILT_7SA			0x03
>  
>  /* bits for I2C_REG_CHANMASK */
> @@ -71,14 +74,122 @@
>  /* power delay: lower nibble of CTRL0 register */
>  #define POWDLY_1_1MS			0x0b
>  
> +/* for sx8650, as we have no pen release IRQ there: timeout in ns following the
> + * last PENIRQ after which we assume the pen is lifted.
> + */
> +#define SX8650_PENIRQ_TIMEOUT		msecs_to_jiffies(10)
> +
>  #define MAX_12BIT			((1 << 12) - 1)
> +#define MAX_I2C_READ_LEN		10 /* see datasheet section 5.1.5 */
> +
> +/* channel definition */
> +#define CH_X				0x00
> +#define CH_Y				0x01
> +
> +struct sx865x_data {
> +	u8 cmd_manual;
> +	u8 chan_mask;
> +	u8 has_irq_penrelease;
> +	u8 has_reg_irqmask;

I changed these 2 to bools.

> +	irq_handler_t irqh;
> +};
>  
>  struct sx8654 {
>  	struct input_dev *input;
>  	struct i2c_client *client;
>  	struct gpio_desc *gpio_reset;
> +
> +	spinlock_t		lock; /* for input reporting from irq/timer */
> +	struct timer_list	timer;
> +
> +	const struct sx865x_data *data;
>  };
>  
> +static inline void sx865x_penrelease(struct sx8654 *ts)
> +{
> +	struct input_dev *input_dev = ts->input;
> +
> +	input_report_key(input_dev, BTN_TOUCH, 0);
> +	input_sync(input_dev);
> +}
> +
> +static void sx865x_penrelease_timer_handler(struct timer_list *t)
> +{
> +	struct sx8654 *ts = from_timer(ts, t, timer);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&ts->lock, flags);
> +	sx865x_penrelease(ts);
> +	spin_unlock_irqrestore(&ts->lock, flags);
> +	dev_dbg(&ts->client->dev, "penrelease by timer\n");
> +}
> +
> +static irqreturn_t sx8650_irq(int irq, void *handle)
> +{
> +	struct sx8654 *ts = handle;
> +	struct device *dev = &ts->client->dev;
> +	int len, i;
> +	unsigned long flags;
> +	u8 stat;
> +	u16 x, y;
> +	u8 data[MAX_I2C_READ_LEN];
> +	u16 ch;
> +	u16 chdata;
> +	u8 readlen = hweight32(ts->data->chan_mask) * 2;
> +
> +	stat = i2c_smbus_read_byte_data(ts->client, CMD_READ_REGISTER
> +						    | I2C_REG_SX8650_STAT);
> +
> +	if (!(stat & SX8650_STAT_CONVIRQ)) {
> +		dev_dbg(dev, "%s ignore stat [0x%02x]", __func__, stat);
> +		return IRQ_HANDLED;
> +	}
> +
> +	len = i2c_master_recv(ts->client, data, readlen);
> +	if (len != readlen) {
> +		dev_dbg(dev, "ignore short recv (%d)\n", len);
> +		return IRQ_HANDLED;
> +	}
> +
> +	spin_lock_irqsave(&ts->lock, flags);
> +
> +	x = 0;
> +	y = 0;
> +	for (i = 0; (i + 1) < len; i++) {
> +		chdata = data[i] << 8;
> +		i++;
> +		chdata += data[i];

So you reading be16 form the wire. I annotated data array as such and
used be16_to_cpu() here.

> +
> +		if (unlikely(chdata == 0xFFFF)) {
> +			dev_dbg(dev, "invalid qualified data @ %d\n", i);
> +			continue;
> +		} else if (unlikely(chdata & 0x8000)) {
> +			dev_warn(dev, "hibit @ %d [0x%04x]\n", i, chdata);
> +			continue;
> +		}
> +
> +		ch = chdata >> 12;
> +		if      (ch == CH_X)
> +			x = chdata & MAX_12BIT;
> +		else if (ch == CH_Y)
> +			y = chdata & MAX_12BIT;
> +		else
> +			dev_warn(dev, "unknown channel %d [0x%04x]\n", ch,
> +				 chdata);
> +	}
> +
> +	input_report_abs(ts->input, ABS_X, x);
> +	input_report_abs(ts->input, ABS_Y, y);
> +	input_report_key(ts->input, BTN_TOUCH, 1);
> +	input_sync(ts->input);
> +	dev_dbg(dev, "point(%4d,%4d)\n", x, y);
> +
> +	mod_timer(&ts->timer, jiffies + SX8650_PENIRQ_TIMEOUT);
> +	spin_unlock_irqrestore(&ts->lock, flags);
> +
> +	return IRQ_HANDLED;
> +}
> +
>  static irqreturn_t sx8654_irq(int irq, void *handle)
>  {
>  	struct sx8654 *sx8654 = handle;
> @@ -127,6 +238,22 @@ static irqreturn_t sx8654_irq(int irq, void *handle)
>  	return IRQ_HANDLED;
>  }
>  
> +static const struct sx865x_data sx8650_data = {
> +	.cmd_manual = 0xb0,
> +	.has_irq_penrelease = 0,
> +	.has_reg_irqmask = 0,

Changed both to false.

> +	.chan_mask = (CONV_X | CONV_Y),
> +	.irqh = sx8650_irq,
> +};
> +
> +static const struct sx865x_data sx8654_data = {
> +	.cmd_manual = 0xc0,
> +	.has_irq_penrelease = 1,
> +	.has_reg_irqmask = 1,

Changed both to true.

> +	.chan_mask = (CONV_X | CONV_Y),
> +	.irqh = sx8654_irq,
> +};
> +

Moved the structures closer to where they are used.

>  static int sx8654_reset(struct sx8654 *ts)
>  {
>  	int err;
> @@ -182,13 +309,13 @@ static void sx8654_close(struct input_dev *dev)
>  	disable_irq(client->irq);
>  
>  	/* enable manual mode mode */
> -	error = i2c_smbus_write_byte(client, CMD_MANUAL);
> +	error = i2c_smbus_write_byte(client, sx8654->data->cmd_manual);
>  	if (error) {
>  		dev_err(&client->dev, "writing command CMD_MANUAL failed");
>  		return;
>  	}
>  
> -	error = i2c_smbus_write_byte_data(client, I2C_REG_TOUCH0, 0);
> +	error = i2c_smbus_write_byte_data(client, I2C_REG_TOUCH0, RATE_MANUAL);
>  	if (error) {
>  		dev_err(&client->dev, "writing to I2C_REG_TOUCH0 failed");
>  		return;
> @@ -221,6 +348,20 @@ static int sx8654_probe(struct i2c_client *client,
>  	}
>  	dev_dbg(&client->dev, "got GPIO reset pin\n");
>  
> +	sx8654->data = of_device_get_match_data(&client->dev);

Changed to device_get_match_data() in case someone would use it on
non-OF platform.

> +	if (!sx8654->data)
> +		sx8654->data = (const struct sx865x_data *)id->driver_data;
> +	if (!sx8654->data) {
> +		dev_err(&client->dev, "invalid or missing device data\n");
> +		return -EINVAL;
> +	}
> +
> +	if (!sx8654->data->has_irq_penrelease) {
> +		dev_dbg(&client->dev, "use timer for penrelease\n");
> +		timer_setup(&sx8654->timer, sx865x_penrelease_timer_handler, 0);
> +		spin_lock_init(&sx8654->lock);

You should also make sure the timer is not running when you are tearing
down the device. I added del_timer_sync() call to sx8654_close().

> +	}
> +
>  	input = devm_input_allocate_device(&client->dev);
>  	if (!input)
>  		return -ENOMEM;
> @@ -248,29 +389,31 @@ static int sx8654_probe(struct i2c_client *client,
>  	}
>  
>  	error = i2c_smbus_write_byte_data(client, I2C_REG_CHANMASK,
> -					  CONV_X | CONV_Y);
> +					  sx8654->data->chan_mask);
>  	if (error) {
>  		dev_err(&client->dev, "writing to I2C_REG_CHANMASK failed");
>  		return error;
>  	}
>  
> -	error = i2c_smbus_write_byte_data(client, I2C_REG_IRQMASK,
> -					  IRQ_PENTOUCH_TOUCHCONVDONE |
> -						IRQ_PENRELEASE);
> -	if (error) {
> -		dev_err(&client->dev, "writing to I2C_REG_IRQMASK failed");
> -		return error;
> +	if (sx8654->data->has_reg_irqmask) {
> +		error = i2c_smbus_write_byte_data(client, I2C_REG_IRQMASK,
> +						  IRQ_PENTOUCH_TOUCHCONVDONE |
> +							IRQ_PENRELEASE);
> +		if (error) {
> +			dev_err(&client->dev, "writing I2C_REG_IRQMASK failed");
> +			return error;
> +		}
>  	}
>  
>  	error = i2c_smbus_write_byte_data(client, I2C_REG_TOUCH1,
> -					  CONDIRQ | FILT_7SA);
> +					  CONDIRQ | RPDNT_100K | FILT_7SA);
>  	if (error) {
>  		dev_err(&client->dev, "writing to I2C_REG_TOUCH1 failed");
>  		return error;
>  	}
>  
>  	error = devm_request_threaded_irq(&client->dev, client->irq,
> -					  NULL, sx8654_irq,
> +					  NULL, sx8654->data->irqh,
>  					  IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
>  					  client->name, sx8654);
>  	if (error) {
> @@ -292,18 +435,28 @@ static int sx8654_probe(struct i2c_client *client,
>  
>  #ifdef CONFIG_OF
>  static const struct of_device_id sx8654_of_match[] = {
> -	{ .compatible = "semtech,sx8654", },
> -	{ .compatible = "semtech,sx8655", },
> -	{ .compatible = "semtech,sx8656", },
> -	{ },
> +	{
> +		.compatible = "semtech,sx8650",
> +		.data = &sx8650_data,
> +	}, {
> +		.compatible = "semtech,sx8654",
> +		.data = &sx8654_data,
> +	}, {
> +		.compatible = "semtech,sx8655",
> +		.data = &sx8654_data,
> +	}, {
> +		.compatible = "semtech,sx8656",
> +		.data = &sx8654_data,
> +	}, {},
>  };
>  MODULE_DEVICE_TABLE(of, sx8654_of_match);
>  #endif
>  
>  static const struct i2c_device_id sx8654_id_table[] = {
> -	{ "semtech_sx8654", 0 },
> -	{ "semtech_sx8655", 0 },
> -	{ "semtech_sx8656", 0 },
> +	{ .name = "semtech_sx8650", .driver_data = (long)&sx8650_data },
> +	{ .name = "semtech_sx8654", .driver_data = (long)&sx8654_data },
> +	{ .name = "semtech_sx8655", .driver_data = (long)&sx8654_data },
> +	{ .name = "semtech_sx8656", .driver_data = (long)&sx8654_data },
>  	{ },
>  };
>  MODULE_DEVICE_TABLE(i2c, sx8654_id_table);
> -- 
> 2.11.0
> 

Applied with above mentioned changes, please take a look in case I
messed up.

Thanks.

-- 
Dmitry

[RESEND PATCH v2 7/8] Input: sx8654 - use common of_touchscreen functions

[Richard Leitner]

of_touchscreen.c provides a common interface for a axis invertion and
swapping of touchscreens. Therefore use it in the sx8654 driver.

Signed-off-by: Richard Leitner <richard.leitner@skidata.com>
---
 drivers/input/touchscreen/sx8654.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/input/touchscreen/sx8654.c b/drivers/input/touchscreen/sx8654.c
index 4939863efbef..b7b263ed52af 100644
--- a/drivers/input/touchscreen/sx8654.c
+++ b/drivers/input/touchscreen/sx8654.c
@@ -35,6 +35,7 @@
 #include <linux/irq.h>
 #include <linux/gpio/consumer.h>
 #include <linux/delay.h>
+#include <linux/input/touchscreen.h>
 
 /* register addresses */
 #define I2C_REG_TOUCH0			0x00
@@ -101,6 +102,7 @@ struct sx8654 {
 
 	spinlock_t		lock; /* for input reporting from irq/timer */
 	struct timer_list	timer;
+	struct touchscreen_properties props;
 
 	const struct sx865x_data *data;
 };
@@ -178,8 +180,7 @@ static irqreturn_t sx8650_irq(int irq, void *handle)
 				 chdata);
 	}
 
-	input_report_abs(ts->input, ABS_X, x);
-	input_report_abs(ts->input, ABS_Y, y);
+	touchscreen_report_pos(ts->input, &ts->props, x, y, false);
 	input_report_key(ts->input, BTN_TOUCH, 1);
 	input_sync(ts->input);
 	dev_dbg(dev, "point(%4d,%4d)\n", x, y);
@@ -226,8 +227,8 @@ static irqreturn_t sx8654_irq(int irq, void *handle)
 		x = ((data[0] & 0xf) << 8) | (data[1]);
 		y = ((data[2] & 0xf) << 8) | (data[3]);
 
-		input_report_abs(sx8654->input, ABS_X, x);
-		input_report_abs(sx8654->input, ABS_Y, y);
+		touchscreen_report_pos(sx8654->input, &sx8654->props, x, y,
+				       false);
 		input_report_key(sx8654->input, BTN_TOUCH, 1);
 		input_sync(sx8654->input);
 
@@ -377,6 +378,8 @@ static int sx8654_probe(struct i2c_client *client,
 	input_set_abs_params(input, ABS_X, 0, MAX_12BIT, 0, 0);
 	input_set_abs_params(input, ABS_Y, 0, MAX_12BIT, 0, 0);
 
+	touchscreen_parse_properties(input, false, &sx8654->props);
+
 	sx8654->client = client;
 	sx8654->input = input;
 
-- 
2.11.0

Re: [RESEND PATCH v2 7/8] Input: sx8654 - use common of_touchscreen functions

[Dmitry Torokhov]

On Tue, Dec 18, 2018 at 09:39:46AM +0100, Richard Leitner wrote:
> of_touchscreen.c provides a common interface for a axis invertion and
> swapping of touchscreens. Therefore use it in the sx8654 driver.
> 
> Signed-off-by: Richard Leitner <richard.leitner@skidata.com>

Applied, thank you.

> ---
>  drivers/input/touchscreen/sx8654.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/sx8654.c b/drivers/input/touchscreen/sx8654.c
> index 4939863efbef..b7b263ed52af 100644
> --- a/drivers/input/touchscreen/sx8654.c
> +++ b/drivers/input/touchscreen/sx8654.c
> @@ -35,6 +35,7 @@
>  #include <linux/irq.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/delay.h>
> +#include <linux/input/touchscreen.h>
>  
>  /* register addresses */
>  #define I2C_REG_TOUCH0			0x00
> @@ -101,6 +102,7 @@ struct sx8654 {
>  
>  	spinlock_t		lock; /* for input reporting from irq/timer */
>  	struct timer_list	timer;
> +	struct touchscreen_properties props;
>  
>  	const struct sx865x_data *data;
>  };
> @@ -178,8 +180,7 @@ static irqreturn_t sx8650_irq(int irq, void *handle)
>  				 chdata);
>  	}
>  
> -	input_report_abs(ts->input, ABS_X, x);
> -	input_report_abs(ts->input, ABS_Y, y);
> +	touchscreen_report_pos(ts->input, &ts->props, x, y, false);
>  	input_report_key(ts->input, BTN_TOUCH, 1);
>  	input_sync(ts->input);
>  	dev_dbg(dev, "point(%4d,%4d)\n", x, y);
> @@ -226,8 +227,8 @@ static irqreturn_t sx8654_irq(int irq, void *handle)
>  		x = ((data[0] & 0xf) << 8) | (data[1]);
>  		y = ((data[2] & 0xf) << 8) | (data[3]);
>  
> -		input_report_abs(sx8654->input, ABS_X, x);
> -		input_report_abs(sx8654->input, ABS_Y, y);
> +		touchscreen_report_pos(sx8654->input, &sx8654->props, x, y,
> +				       false);
>  		input_report_key(sx8654->input, BTN_TOUCH, 1);
>  		input_sync(sx8654->input);
>  
> @@ -377,6 +378,8 @@ static int sx8654_probe(struct i2c_client *client,
>  	input_set_abs_params(input, ABS_X, 0, MAX_12BIT, 0, 0);
>  	input_set_abs_params(input, ABS_Y, 0, MAX_12BIT, 0, 0);
>  
> +	touchscreen_parse_properties(input, false, &sx8654->props);
> +
>  	sx8654->client = client;
>  	sx8654->input = input;
>  
> -- 
> 2.11.0
> 

-- 
Dmitry

[RESEND PATCH v2 8/8] Input: sx8654 - convert #defined flags to BIT(x)

[Richard Leitner]

Some of the #defined register values are one-bit flags. Convert them to
use the BIT(x) macro instead of 1 byte hexadecimal values. This improves
readability and clarifies the intent.

Signed-off-by: Richard Leitner <richard.leitner@skidata.com>
---
 drivers/input/touchscreen/sx8654.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/input/touchscreen/sx8654.c b/drivers/input/touchscreen/sx8654.c
index b7b263ed52af..3746ea855f94 100644
--- a/drivers/input/touchscreen/sx8654.c
+++ b/drivers/input/touchscreen/sx8654.c
@@ -36,6 +36,7 @@
 #include <linux/gpio/consumer.h>
 #include <linux/delay.h>
 #include <linux/input/touchscreen.h>
+#include <linux/bitops.h>
 
 /* register addresses */
 #define I2C_REG_TOUCH0			0x00
@@ -46,7 +47,7 @@
 #define I2C_REG_SOFTRESET		0x3f
 
 #define I2C_REG_SX8650_STAT		0x05
-#define SX8650_STAT_CONVIRQ		0x80
+#define SX8650_STAT_CONVIRQ		BIT(7)
 
 /* commands */
 #define CMD_READ_REGISTER		0x40
@@ -56,8 +57,8 @@
 #define SOFTRESET_VALUE			0xde
 
 /* bits for I2C_REG_IRQSRC */
-#define IRQ_PENTOUCH_TOUCHCONVDONE	0x08
-#define IRQ_PENRELEASE			0x04
+#define IRQ_PENTOUCH_TOUCHCONVDONE	BIT(7)
+#define IRQ_PENRELEASE			BIT(6)
 
 /* bits for RegTouch1 */
 #define CONDIRQ				0x20
@@ -65,8 +66,8 @@
 #define FILT_7SA			0x03
 
 /* bits for I2C_REG_CHANMASK */
-#define CONV_X				0x80
-#define CONV_Y				0x40
+#define CONV_X				BIT(7)
+#define CONV_Y				BIT(6)
 
 /* coordinates rate: higher nibble of CTRL0 register */
 #define RATE_MANUAL			0x00
-- 
2.11.0

Re: [RESEND PATCH v2 8/8] Input: sx8654 - convert #defined flags to BIT(x)

[Dmitry Torokhov]

On Tue, Dec 18, 2018 at 09:40:02AM +0100, Richard Leitner wrote:
> Some of the #defined register values are one-bit flags. Convert them to
> use the BIT(x) macro instead of 1 byte hexadecimal values. This improves
> readability and clarifies the intent.
> 
> Signed-off-by: Richard Leitner <richard.leitner@skidata.com>

Applied, thank you.

> ---
>  drivers/input/touchscreen/sx8654.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/sx8654.c b/drivers/input/touchscreen/sx8654.c
> index b7b263ed52af..3746ea855f94 100644
> --- a/drivers/input/touchscreen/sx8654.c
> +++ b/drivers/input/touchscreen/sx8654.c
> @@ -36,6 +36,7 @@
>  #include <linux/gpio/consumer.h>
>  #include <linux/delay.h>
>  #include <linux/input/touchscreen.h>
> +#include <linux/bitops.h>
>  
>  /* register addresses */
>  #define I2C_REG_TOUCH0			0x00
> @@ -46,7 +47,7 @@
>  #define I2C_REG_SOFTRESET		0x3f
>  
>  #define I2C_REG_SX8650_STAT		0x05
> -#define SX8650_STAT_CONVIRQ		0x80
> +#define SX8650_STAT_CONVIRQ		BIT(7)
>  
>  /* commands */
>  #define CMD_READ_REGISTER		0x40
> @@ -56,8 +57,8 @@
>  #define SOFTRESET_VALUE			0xde
>  
>  /* bits for I2C_REG_IRQSRC */
> -#define IRQ_PENTOUCH_TOUCHCONVDONE	0x08
> -#define IRQ_PENRELEASE			0x04
> +#define IRQ_PENTOUCH_TOUCHCONVDONE	BIT(7)
> +#define IRQ_PENRELEASE			BIT(6)
>  
>  /* bits for RegTouch1 */
>  #define CONDIRQ				0x20
> @@ -65,8 +66,8 @@
>  #define FILT_7SA			0x03
>  
>  /* bits for I2C_REG_CHANMASK */
> -#define CONV_X				0x80
> -#define CONV_Y				0x40
> +#define CONV_X				BIT(7)
> +#define CONV_Y				BIT(6)
>  
>  /* coordinates rate: higher nibble of CTRL0 register */
>  #define RATE_MANUAL			0x00
> -- 
> 2.11.0
> 

-- 
Dmitry

Re: [RESEND PATCH v2 8/8] Input: sx8654 - convert #defined flags to BIT(x)

[Joe Perches]

On Mon, 2019-01-28 at 16:25 -0800, Dmitry Torokhov wrote:
> On Tue, Dec 18, 2018 at 09:40:02AM +0100, Richard Leitner wrote:
> > Some of the #defined register values are one-bit flags. Convert them to
> > use the BIT(x) macro instead of 1 byte hexadecimal values. This improves
> > readability and clarifies the intent.
> > 
> > Signed-off-by: Richard Leitner <richard.leitner@skidata.com>
> 
> Applied, thank you.

Not so sure this should be applied.

> > diff --git a/drivers/input/touchscreen/sx8654.c b/drivers/input/touchscreen/sx8654.c
[]
> > @@ -46,7 +47,7 @@
[]
> >  /* bits for I2C_REG_IRQSRC */
> > -#define IRQ_PENTOUCH_TOUCHCONVDONE	0x08
> > -#define IRQ_PENRELEASE			0x04
> > +#define IRQ_PENTOUCH_TOUCHCONVDONE	BIT(7)
> > +#define IRQ_PENRELEASE			BIT(6)

Shouldn't this be BIT(3) and BIT(2)
or did you mean to change the values too?

If so, this change should be noted in the commit message.

Re: [RESEND PATCH v2 8/8] Input: sx8654 - convert #defined flags to BIT(x)

[Richard Leitner]

Hi Joe,

On 29/01/2019 06:40, Joe Perches wrote:
> On Mon, 2019-01-28 at 16:25 -0800, Dmitry Torokhov wrote:
>> On Tue, Dec 18, 2018 at 09:40:02AM +0100, Richard Leitner wrote:
>>> Some of the #defined register values are one-bit flags. Convert them to
>>> use the BIT(x) macro instead of 1 byte hexadecimal values. This improves
>>> readability and clarifies the intent.
>>>
>>> Signed-off-by: Richard Leitner <richard.leitner@skidata.com>
>>
>> Applied, thank you.
> 
> Not so sure this should be applied.
> 
>>> diff --git a/drivers/input/touchscreen/sx8654.c b/drivers/input/touchscreen/sx8654.c
> []
>>> @@ -46,7 +47,7 @@
> []
>>>   /* bits for I2C_REG_IRQSRC */
>>> -#define IRQ_PENTOUCH_TOUCHCONVDONE	0x08
>>> -#define IRQ_PENRELEASE			0x04
>>> +#define IRQ_PENTOUCH_TOUCHCONVDONE	BIT(7)
>>> +#define IRQ_PENRELEASE			BIT(6)
> 
> Shouldn't this be BIT(3) and BIT(2)
> or did you mean to change the values too?
> 
> If so, this change should be noted in the commit message.
> 

That's true, those values should stay the same. Thanks for the catch!

@Dimitry: Should I send an updated version or do you fix it yourself?

regards;Richard.L

Re: [RESEND PATCH v2 8/8] Input: sx8654 - convert #defined flags to BIT(x)

[Dmitry Torokhov]

On Tue, Jan 29, 2019 at 12:23:01PM +0100, Richard Leitner wrote:
> Hi Joe,
> 
> On 29/01/2019 06:40, Joe Perches wrote:
> > On Mon, 2019-01-28 at 16:25 -0800, Dmitry Torokhov wrote:
> > > On Tue, Dec 18, 2018 at 09:40:02AM +0100, Richard Leitner wrote:
> > > > Some of the #defined register values are one-bit flags. Convert them to
> > > > use the BIT(x) macro instead of 1 byte hexadecimal values. This improves
> > > > readability and clarifies the intent.
> > > > 
> > > > Signed-off-by: Richard Leitner <richard.leitner@skidata.com>
> > > 
> > > Applied, thank you.
> > 
> > Not so sure this should be applied.
> > 
> > > > diff --git a/drivers/input/touchscreen/sx8654.c b/drivers/input/touchscreen/sx8654.c
> > []
> > > > @@ -46,7 +47,7 @@
> > []
> > > >   /* bits for I2C_REG_IRQSRC */
> > > > -#define IRQ_PENTOUCH_TOUCHCONVDONE	0x08
> > > > -#define IRQ_PENRELEASE			0x04
> > > > +#define IRQ_PENTOUCH_TOUCHCONVDONE	BIT(7)
> > > > +#define IRQ_PENRELEASE			BIT(6)
> > 
> > Shouldn't this be BIT(3) and BIT(2)
> > or did you mean to change the values too?
> > 
> > If so, this change should be noted in the commit message.
> > 
> 
> That's true, those values should stay the same. Thanks for the catch!

Thanks Joe!

> 
> @Dimitry: Should I send an updated version or do you fix it yourself?
> 

I updated the values and pushed out the new version.

Thanks.

-- 
Dmitry

Re: [RESEND PATCH v2 0/8] Input: sx8654 - reset-gpio, sx865[056] support, etc.

[Richard Leitner]

Hi,
another 3 weeks and still no response :-(

The last comment on this patchset was in October by Dimitry/Rob. In the
meantime I did a "ping" and a resend...

So what's up? I'm perfectly fine with a "we don't want this is mainline
because XXX" or "I have no time and will come back to it in XXX"...

But please give at least some kind of response... Thanks!

regards;Richard.L

On 18/12/2018 09:35, Richard Leitner wrote:
> Add reset-gpio, sx8654[056] and common of_touchscreen functions support
> for the sx8654 driver.
> 
> Changes RESEND v2:
> 	- added "Reviewed-by: Rob Herring <robh@kernel.org>" tags
> 
> Changes v2:
> 	- use devm_gpiod_get_optional in probe instead of in #ifdef CONFIG_OF
> 	- convert flags to BIT() in a separate patch
> 	- replace hrtimer with "regular" timer
> 	- use of_device_get_match_data instead of of_match_device
> 	- add driver data to i2c_device_id table for non-DT fallback
> 	- fix sequence of common touchscreen initialization
> 	- div. minor stlye changes
> 
> Richard Leitner (8):
>    dt-bindings: input: touchscreen: sx8654: add reset-gpio property
>    Input: sx8654 - add reset-gpio support
>    dt-bindings: input: touchscreen: sx8654: add compatible models
>    Input: sx8654 - add sx8655 and sx8656 to compatibles
>    dt-bindings: input: touchscreen: sx8654: add sx8650 to comatibles
>    Input: sx8654 - add sx8650 support
>    Input: sx8654 - use common of_touchscreen functions
>    Input: sx8654 - convert #defined flags to BIT(x)
> 
>   .../bindings/input/touchscreen/sx8654.txt          |  10 +-
>   drivers/input/touchscreen/sx8654.c                 | 245 ++++++++++++++++++---
>   2 files changed, 229 insertions(+), 26 deletions(-)
>