Linux Input/HID development
 help / color / mirror / Atom feed
* AW: [PATCH 2/3] Input: st1232 - add support for st1633
From: Matthias Fend @ 2019-01-29  7:07 UTC (permalink / raw)
  To: Martin Kepplinger, Dmitry Torokhov
  Cc: devicetree@vger.kernel.org, linux-input@vger.kernel.org,
	robh+dt@kernel.org, mark.rutland@arm.com,
	linux-kernel@vger.kernel.org, Martin Kepplinger
In-Reply-To: <894cb7d4749835325c4e0f9c65d31a63@posteo.de>

Hi Martin,


Matthias Fend
R&D Electronics 

Wolfvision GmbH
Oberes Ried 14 | 6833 Klaus | Austria 
Tel: +43 5523 52250 | Mail: Matthias.Fend@wolfvision.net 

Webpage: www.wolfvision.com | www.wolfvision.com/green
Firmenbuch / Commercial Register: FN283521v Feldkirch/Austria

> -----Ursprüngliche Nachricht-----
> Von: linux-input-owner@vger.kernel.org <linux-input-
> owner@vger.kernel.org> Im Auftrag von Martin Kepplinger
> Gesendet: Montag, 28. Jänner 2019 20:10
> An: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: devicetree@vger.kernel.org; linux-input@vger.kernel.org;
> robh+dt@kernel.org; mark.rutland@arm.com; linux-kernel@vger.kernel.org;
> Martin Kepplinger <martin.kepplinger@ginzinger.com>
> Betreff: Re: [PATCH 2/3] Input: st1232 - add support for st1633
> 
> Am 28.01.2019 20:03 schrieb Dmitry Torokhov:
> > Hi Martin,
> >
> > On Mon, Jan 28, 2019 at 09:44:48AM +0100, Martin Kepplinger wrote:
> >> From: Martin Kepplinger <martin.kepplinger@ginzinger.com>
> >>
> >> Add support for the Sitronix ST1633 touchscreen controller to the
> >> st1232
> >> driver. A protocol spec can be found here:
> >>
> https://emea01.safelinks.protection.outlook.com/?url=www.ampdisplay.co
> m%2Fdocuments%2Fpdf%2FAM-320480B6TZQW-
> TC0H.pdf&amp;data=01%7C01%7CMatthias.Fend%40wolfvision.net%7C4f9d
> 56475f674b1e3b4008d685543b35%7Ce94ec9da9183471e83b351baa8eb804f%
> 7C0&amp;sdata=YTw33BPKKpdN%2BBFrufVk8e4YqXOzR6VQKuzRMOjcwWk
> %3D&amp;reserved=0
> >>
> >> Signed-off-by: Martin Kepplinger <martin.kepplinger@ginzinger.com>
> >> ---
> >>  drivers/input/touchscreen/Kconfig  |   6 +-
> >>  drivers/input/touchscreen/st1232.c | 122
> >> +++++++++++++++++++++--------
> >>  2 files changed, 94 insertions(+), 34 deletions(-)
> >>
> >> diff --git a/drivers/input/touchscreen/Kconfig
> >> b/drivers/input/touchscreen/Kconfig
> >> index 068dbbc610fc..7c597a49c265 100644
> >> --- a/drivers/input/touchscreen/Kconfig
> >> +++ b/drivers/input/touchscreen/Kconfig
> >> @@ -1168,11 +1168,11 @@ config TOUCHSCREEN_SIS_I2C
> >>  	  module will be called sis_i2c.
> >>
> >>  config TOUCHSCREEN_ST1232
> >> -	tristate "Sitronix ST1232 touchscreen controllers"
> >> +	tristate "Sitronix ST1232 or ST1633 touchscreen controllers"
> >>  	depends on I2C
> >>  	help
> >> -	  Say Y here if you want to support Sitronix ST1232
> >> -	  touchscreen controller.
> >> +	  Say Y here if you want to support the Sitronix ST1232
> >> +	  or ST1633 touchscreen controller.
> >>
> >>  	  If unsure, say N.
> >>
> >> diff --git a/drivers/input/touchscreen/st1232.c
> >> b/drivers/input/touchscreen/st1232.c
> >> index 11ff32c68025..19a665d48dad 100644
> >> --- a/drivers/input/touchscreen/st1232.c
> >> +++ b/drivers/input/touchscreen/st1232.c
> >> @@ -23,13 +23,15 @@
> >>  #include <linux/types.h>
> >>
> >>  #define ST1232_TS_NAME	"st1232-ts"
> >> +#define ST1633_TS_NAME	"st1633-ts"
> >> +
> >> +enum {
> >> +	st1232,
> >> +	st1633,
> >> +};
> >
> > This enum seems no longer needed, I dropped it.
> >
> >>
> >>  #define MIN_X		0x00
> >>  #define MIN_Y		0x00
> >
> > Given we no longer have constant MAX_X/Y I simply used 0 in
> > input_set_abs_params() and dropped MIN-X/Y.
> >
> >> -#define MAX_X		0x31f	/* (800 - 1) */
> >> -#define MAX_Y		0x1df	/* (480 - 1) */
> >> -#define MAX_AREA	0xff
> >> -#define MAX_FINGERS	2
> >>
> >>  struct st1232_ts_finger {
> >>  	u16 x;
> >> @@ -38,12 +40,24 @@ struct st1232_ts_finger {
> >>  	bool is_valid;
> >>  };
> >>
> >> +struct st_chip_info {
> >> +	bool	have_z;
> >> +	u16	max_x;
> >> +	u16	max_y;
> >> +	u16	max_area;
> >> +	u16	max_fingers;
> >> +	u8	start_reg;
> >> +};
> >> +
> >>  struct st1232_ts_data {
> >>  	struct i2c_client *client;
> >>  	struct input_dev *input_dev;
> >> -	struct st1232_ts_finger finger[MAX_FINGERS];
> >>  	struct dev_pm_qos_request low_latency_req;
> >>  	int reset_gpio;
> >
> > Could you please create an additional patch converting this to gpiod?
> > Instead of doing of_get_gpio()/gpio_is_valid()/devm_gpio_request()
> > smply
> > do
> >
> > 	ts->reset_gpio = devm_gpiod_get_optional();
> > 	if (IS_ERR(ts->reset_gpio)) {
> > 		...
> > 	}
> >
> > and later
> >
> > 	if (ts->reset_gpio)
> > 		...
> >
> > Looking at the code it looks like reset is as usual active low, so you
> > will need to invert the logic as gpiod takes care of convertion logical
> > value to proper level (active low or high).
> 
> I'll test your applied changes and get back to this tomorrow.
> 
> thanks.
> 
> >
> >> +	const struct st_chip_info *chip_info;
> >> +	int read_buf_len;
> >> +	u8 *read_buf;
> >> +	struct st1232_ts_finger *finger;
> >>  };
> >>
> >>  static int st1232_ts_read_data(struct st1232_ts_data *ts)
> >> @@ -52,40 +66,35 @@ static int st1232_ts_read_data(struct
> >> st1232_ts_data *ts)
> >>  	struct i2c_client *client = ts->client;
> >>  	struct i2c_msg msg[2];
> >>  	int error;
> >> -	u8 start_reg;
> >> -	u8 buf[10];
> >> +	int i, y;
> >> +	u8 start_reg = ts->chip_info->start_reg;
> >> +	u8 *buf = ts->read_buf;
> >>
> >> -	/* read touchscreen data from ST1232 */
> >> +	/* read touchscreen data */
> >>  	msg[0].addr = client->addr;
> >>  	msg[0].flags = 0;
> >>  	msg[0].len = 1;
> >>  	msg[0].buf = &start_reg;
> >> -	start_reg = 0x10;
> >>
> >>  	msg[1].addr = ts->client->addr;
> >>  	msg[1].flags = I2C_M_RD;
> >> -	msg[1].len = sizeof(buf);
> >> +	msg[1].len = ts->read_buf_len;
> >>  	msg[1].buf = buf;
> >>
> >>  	error = i2c_transfer(client->adapter, msg, 2);
> >>  	if (error < 0)
> >>  		return error;
> >>
> >> -	/* get "valid" bits */
> >> -	finger[0].is_valid = buf[2] >> 7;
> >> -	finger[1].is_valid = buf[5] >> 7;
> >> +	for (i = 0, y = 0; i < ts->chip_info->max_fingers; i++, y += 3) {
> >> +		finger[i].is_valid = buf[i + y] >> 7;
> >> +		if (finger[i].is_valid) {
> >> +			finger[i].x = ((buf[i + y] & 0x0070) << 4) | buf[i + 1];
> >> +			finger[i].y = ((buf[i + y] & 0x0007) << 8) | buf[i + 2];
> >>
> >> -	/* get xy coordinate */
> >> -	if (finger[0].is_valid) {
> >> -		finger[0].x = ((buf[2] & 0x0070) << 4) | buf[3];
> >> -		finger[0].y = ((buf[2] & 0x0007) << 8) | buf[4];
> >> -		finger[0].t = buf[8];
> >> -	}
> >> -
> >> -	if (finger[1].is_valid) {
> >> -		finger[1].x = ((buf[5] & 0x0070) << 4) | buf[6];
> >> -		finger[1].y = ((buf[5] & 0x0007) << 8) | buf[7];
> >> -		finger[1].t = buf[9];
> >> +			/* st1232 includes a z-axis / touch strength */
> >> +			if (ts->chip_info->have_z)
> >> +				finger[i].t = buf[i + 6];
> >> +		}
> >>  	}
> >>
> >>  	return 0;
> >> @@ -104,11 +113,14 @@ static irqreturn_t st1232_ts_irq_handler(int
> >> irq, void *dev_id)
> >>  		goto end;
> >>
> >>  	/* multi touch protocol */
> >> -	for (i = 0; i < MAX_FINGERS; i++) {
> >> +	for (i = 0; i < ts->chip_info->max_fingers; i++) {
> >>  		if (!finger[i].is_valid)
> >>  			continue;
> >>
> >> -		input_report_abs(input_dev, ABS_MT_TOUCH_MAJOR,
> finger[i].t);
> >> +		if (ts->chip_info->have_z)
> >> +			input_report_abs(input_dev,
> ABS_MT_TOUCH_MAJOR,
> >> +					 finger[i].t);
> >> +
> >>  		input_report_abs(input_dev, ABS_MT_POSITION_X,
> finger[i].x);
> >>  		input_report_abs(input_dev, ABS_MT_POSITION_Y,
> finger[i].y);
> >>  		input_mt_sync(input_dev);
> >> @@ -142,12 +154,40 @@ static void st1232_ts_power(struct
> >> st1232_ts_data *ts, bool poweron)
> >>  		gpio_direction_output(ts->reset_gpio, poweron);
> >>  }
> >>
> >> +static const struct st_chip_info st1232_chip_info = {
> >> +		.have_z = true,
> >> +		.max_x = 0x31f, /* 800 - 1 */
> >> +		.max_y = 0x1df, /* 480 -1 */
> >> +		.max_area = 0xff,
> >> +		.max_fingers = 2,
> >> +		.start_reg = 0x12,
> >> +};
> >> +
> >> +static const struct st_chip_info st1633_chip_info = {
> >> +		.have_z = false,
> >> +		.max_x = 0x13f, /* 320 - 1 */
> >> +		.max_y = 0x1df, /* 480 -1 */

I guess these values are the dimensions of the referenced TFT display and not any limits of the touch controller. I wonder which values are correct here?
Maybe it would also be easier to just use the decimal representation and drop the comment, what do you think?

Thanks,
 ~Matthias

> >> +		.max_area = 0x00,
> >> +		.max_fingers = 5,
> >> +		.start_reg = 0x12,
> >> +};
> >> +
> >>  static int st1232_ts_probe(struct i2c_client *client,
> >>  			   const struct i2c_device_id *id)
> >>  {
> >>  	struct st1232_ts_data *ts;
> >> +	struct st1232_ts_finger *finger;
> >>  	struct input_dev *input_dev;
> >>  	int error;
> >> +	const struct st_chip_info *match = NULL;
> >
> > There is no need to initialize with NULL given that we do unconditional
> > assignment below. I removed initialization.
> >
> >> +
> >> +	match = device_get_match_data(&client->dev);
> >> +	if (!match && id)
> >> +		match = (const void *)id->driver_data;
> >> +	if (!match) {
> >> +		dev_err(&client->dev, "unknown device model\n");
> >> +		return -ENODEV;
> >> +	}
> >>
> >>  	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
> >>  		dev_err(&client->dev, "need I2C_FUNC_I2C\n");
> >> @@ -163,6 +203,19 @@ static int st1232_ts_probe(struct i2c_client
> >> *client,
> >>  	if (!ts)
> >>  		return -ENOMEM;
> >>
> >> +	ts->chip_info = match;
> >> +	ts->finger = devm_kzalloc(&client->dev,
> >> +				  sizeof(*finger) * ts->chip_info-
> >max_fingers,
> >> +				  GFP_KERNEL);
> >
> > I replaced it with devm_kcalloc(&client->dev,
> > 				ts->chip_info->max_fingers, sizeof(*finger),
> > 				GFP_KERNEL);
> >
> > and applied.
> >
> >> +	if (!ts->finger)
> >> +		return -ENOMEM;
> >> +
> >> +	/* allocate a buffer according to the number of registers to read */
> >> +	ts->read_buf_len = ts->chip_info->max_fingers * 4;
> >> +	ts->read_buf = devm_kzalloc(&client->dev, ts->read_buf_len,
> >> GFP_KERNEL);
> >> +	if (!ts->read_buf)
> >> +		return -ENOMEM;
> >> +
> >>  	input_dev = devm_input_allocate_device(&client->dev);
> >>  	if (!input_dev)
> >>  		return -ENOMEM;
> >> @@ -192,9 +245,14 @@ static int st1232_ts_probe(struct i2c_client
> >> *client,
> >>  	__set_bit(EV_KEY, input_dev->evbit);
> >>  	__set_bit(EV_ABS, input_dev->evbit);
> >>
> >> -	input_set_abs_params(input_dev, ABS_MT_TOUCH_MAJOR, 0,
> MAX_AREA, 0,
> >> 0);
> >> -	input_set_abs_params(input_dev, ABS_MT_POSITION_X, MIN_X,
> MAX_X, 0,
> >> 0);
> >> -	input_set_abs_params(input_dev, ABS_MT_POSITION_Y, MIN_Y,
> MAX_Y, 0,
> >> 0);
> >> +	if (ts->chip_info->have_z)
> >> +		input_set_abs_params(input_dev,
> ABS_MT_TOUCH_MAJOR, 0,
> >> +				     ts->chip_info->max_area, 0, 0);
> >> +
> >> +	input_set_abs_params(input_dev, ABS_MT_POSITION_X,
> >> +			     MIN_X, ts->chip_info->max_x, 0, 0);
> >> +	input_set_abs_params(input_dev, ABS_MT_POSITION_Y,
> >> +			     MIN_Y, ts->chip_info->max_y, 0, 0);
> >>
> >>  	error = devm_request_threaded_irq(&client->dev, client->irq,
> >>  					  NULL, st1232_ts_irq_handler,
> >> @@ -261,13 +319,15 @@ static
> SIMPLE_DEV_PM_OPS(st1232_ts_pm_ops,
> >>  			 st1232_ts_suspend, st1232_ts_resume);
> >>
> >>  static const struct i2c_device_id st1232_ts_id[] = {
> >> -	{ ST1232_TS_NAME, 0 },
> >> +	{ ST1232_TS_NAME, (unsigned long)&st1232_chip_info },
> >> +	{ ST1633_TS_NAME, (unsigned long)&st1633_chip_info },
> >>  	{ }
> >>  };
> >>  MODULE_DEVICE_TABLE(i2c, st1232_ts_id);
> >>
> >>  static const struct of_device_id st1232_ts_dt_ids[] = {
> >> -	{ .compatible = "sitronix,st1232", },
> >> +	{ .compatible = "sitronix,st1232", .data = &st1232_chip_info },
> >> +	{ .compatible = "sitronix,st1633", .data = &st1633_chip_info },
> >>  	{ }
> >>  };
> >>  MODULE_DEVICE_TABLE(of, st1232_ts_dt_ids);
> >> --
> >> 2.20.1
> >>
> >
> > Thanks.

^ permalink raw reply

* Re: [RESEND PATCH v2 8/8] Input: sx8654 - convert #defined flags to BIT(x)
From: Joe Perches @ 2019-01-29  5:40 UTC (permalink / raw)
  To: Dmitry Torokhov, Richard Leitner
  Cc: mark.rutland, robh+dt, linux-input, devicetree, linux-kernel
In-Reply-To: <20190129002518.GH34692@dtor-ws>

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.

^ permalink raw reply

* [PATCH] Input: sx8654 - do not override interrupt trigger
From: Dmitry Torokhov @ 2019-01-29  0:37 UTC (permalink / raw)
  To: linux-input; +Cc: Richard Leitner, linux-kernel

We should rely on the interrupt trigger (level vs edge) set up by the
firmware or board code instead of forcing what we consider appropriate.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/touchscreen/sx8654.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/input/touchscreen/sx8654.c b/drivers/input/touchscreen/sx8654.c
index 1cc0e4116d3b..b792320d628e 100644
--- a/drivers/input/touchscreen/sx8654.c
+++ b/drivers/input/touchscreen/sx8654.c
@@ -403,7 +403,7 @@ static int sx8654_probe(struct i2c_client *client,
 
 	error = devm_request_threaded_irq(&client->dev, client->irq,
 					  NULL, sx8654->data->irqh,
-					  IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+					  IRQF_ONESHOT,
 					  client->name, sx8654);
 	if (error) {
 		dev_err(&client->dev,
-- 
2.20.1.495.gaa96b0ce6b-goog


-- 
Dmitry

^ permalink raw reply related

* Re: [RESEND PATCH v2 4/8] Input: sx8654 - add sx8655 and sx8656 to compatibles
From: Dmitry Torokhov @ 2019-01-29  0:26 UTC (permalink / raw)
  To: Richard Leitner
  Cc: mark.rutland, robh+dt, linux-input, devicetree, linux-kernel
In-Reply-To: <20181218083606.25795-5-richard.leitner@skidata.com>

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

^ permalink raw reply

* Re: [RESEND PATCH v2 2/8] Input: sx8654 - add reset-gpio support
From: Dmitry Torokhov @ 2019-01-29  0:26 UTC (permalink / raw)
  To: Richard Leitner
  Cc: mark.rutland, robh+dt, linux-input, devicetree, linux-kernel
In-Reply-To: <20181218083606.25795-3-richard.leitner@skidata.com>

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

^ permalink raw reply

* Re: [RESEND PATCH v2 7/8] Input: sx8654 - use common of_touchscreen functions
From: Dmitry Torokhov @ 2019-01-29  0:25 UTC (permalink / raw)
  To: Richard Leitner
  Cc: mark.rutland, robh+dt, linux-input, devicetree, linux-kernel
In-Reply-To: <20181218083946.13823-1-richard.leitner@skidata.com>

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

^ permalink raw reply

* Re: [RESEND PATCH v2 8/8] Input: sx8654 - convert #defined flags to BIT(x)
From: Dmitry Torokhov @ 2019-01-29  0:25 UTC (permalink / raw)
  To: Richard Leitner
  Cc: mark.rutland, robh+dt, linux-input, devicetree, linux-kernel
In-Reply-To: <20181218084002.19454-1-richard.leitner@skidata.com>

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

^ permalink raw reply

* Re: [RESEND PATCH v2 6/8] Input: sx8654 - add sx8650 support
From: Dmitry Torokhov @ 2019-01-29  0:12 UTC (permalink / raw)
  To: Richard Leitner
  Cc: mark.rutland, robh+dt, linux-input, devicetree, linux-kernel
In-Reply-To: <20181218083931.11549-1-richard.leitner@skidata.com>

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

^ permalink raw reply

* Re: [PATCH][RESEND] HID: elan: Make array buf static, shrinks object size
From: Jiri Kosina @ 2019-01-28 20:49 UTC (permalink / raw)
  To: Colin King; +Cc: Benjamin Tissoires, linux-input, kernel-janitors, linux-kernel
In-Reply-To: <20190125160546.8732-1-colin.king@canonical.com>

On Fri, 25 Jan 2019, Colin King wrote:

> From: Colin Ian King <colin.king@canonical.com>
> 
> Don't populate the array buf on the stack but instead make it
> static. Makes the object code smaller by 43 bytes:
> 
> Before:
>    text	   data	    bss	    dec	    hex	filename
>    7769	   1520	      0	   9289	   2449	drivers/hid/hid-elan.o
> 
> After:
>    text	   data	    bss	    dec	    hex	filename
>    7662	   1584	      0	   9246	   241e	drivers/hid/hid-elan.o
> 
> (gcc version 8.2.0 x86_64)

Applied to for-5.1/hid-elan.

-- 
Jiri Kosina
SUSE Labs

^ permalink raw reply

* Re: [PATCH 3/3] input: dt-bindings: Add binding for bma150 sensor
From: Dmitry Torokhov @ 2019-01-28 19:31 UTC (permalink / raw)
  To: Jonathan Bakker
  Cc: Paweł Chmiel, robh+dt@kernel.org, mark.rutland@arm.com,
	devicetree@vger.kernel.org, linux-input@vger.kernel.org,
	linux-kernel@vger.kernel.org
In-Reply-To: <BYAPR02MB54950EDEEE82D5F06C7131CEA3940@BYAPR02MB5495.namprd02.prod.outlook.com>

On Sat, Jan 26, 2019 at 03:39:17AM +0000, Jonathan Bakker wrote:
> 
> 
> On 2019-01-25 5:28 p.m., Dmitry Torokhov wrote:
> > On Fri, Jan 25, 2019 at 07:44:00PM +0100, Paweł Chmiel wrote:
> >> From: Jonathan Bakker <xc-racer2@live.ca>
> >>
> >> Add device tree bindings for Bosch BMA150 Accelerometer Sensor
> >>
> >> Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
> >> Signed-off-by: Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com>
> >> ---
> >>  .../bindings/input/bosch,bma150.txt           | 20 +++++++++++++++++++
> >>  1 file changed, 20 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/input/bosch,bma150.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/input/bosch,bma150.txt b/Documentation/devicetree/bindings/input/bosch,bma150.txt
> >> new file mode 100644
> >> index 000000000000..290c60e38c70
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/input/bosch,bma150.txt
> >> @@ -0,0 +1,20 @@
> >> +* Bosch BMA150 Accelerometer Sensor
> >> +
> >> +Also works for the SMB380 and BMA023 accelerometers
> >> +
> >> +Required properties:
> >> +- compatible : Should be "bosch,bma150"
> >> +- reg : The I2C address of the sensor
> >> +
> >> +Optional properties:
> >> +- interrupt-parent : should be the phandle for the interrupt controller
> >> +- interrupts : Interrupt mapping for IRQ.  If not present device will be polled
> >> +
> >> +Example:
> >> +
> >> +bma150@38 {
> >> +	compatible = "bosch,bma150";
> >> +	reg = <0x38>;
> >> +	interrupt-parent = <&gph0>;
> >> +	interrupts = <1 IRQ_TYPE_LEVEL_HIGH>;
> > 
> > Hmm, here you say that IRQ_TYPE_LEVEL_HIGH, so it is level interrupts,
> > but the driver overrides to rising edge unconditionally. Since you are
> > the first to add DT support please make separate patch to driver to drop
> > the ORQ trigger from request_theraded_irq() leaving only IRQF_ONESHOT.
> > 
> This was simply an oversight on my part, my device was using the the polled method as opposed to an interrupt.  I'll correct this in v2.
> > Also please create patch removing platform data support as noone is
> > using it upstream.
> > 
> Will do.
> > What about the rest of config parameters from bma150_cfg? They should be
> > handled as device properties too.
> > 
> Ok, I can add them as well.  I didn't bother as the comment in the source code says that the default values are the ones recommended by Bosch.

OK, if you do not need to handle them then we can leave this task to the
next user ;)

Thanks.

-- 
Dmitry

^ permalink raw reply

* Re: [PATCH 4/4] dt-bindings: input: Add documentation for gp2a sensor
From: Dmitry Torokhov @ 2019-01-28 19:30 UTC (permalink / raw)
  To: Jonathan Bakker
  Cc: Paweł Chmiel, robh+dt@kernel.org, mark.rutland@arm.com,
	mchehab+samsung@kernel.org, colyli@suse.de,
	ckeepax@opensource.wolfsonmicro.com, andrew.smirnov@gmail.com,
	arnd@arndb.de, xiaotong.lu@spreadtrum.com,
	linux-input@vger.kernel.org, linux-kernel@vger.kernel.org,
	devicetree@vger.kernel.org
In-Reply-To: <BYAPR02MB54959FD8A7CF5977B0D4B5B7A3940@BYAPR02MB5495.namprd02.prod.outlook.com>

On Sat, Jan 26, 2019 at 03:14:14AM +0000, Jonathan Bakker wrote:
> Hi Dmitry,
> 
> Thanks for your review of the patches.
> 
> Considering that the light sensor part should be in IIO, should the entire driver be rewritten as an IIO driver?  There's already the driver for gp2ap020a00f there which is presumably the gp2ap002a00f's successor and does the same functions.

I'd be fine with that.

> 
> On 2019-01-25 5:32 p.m., Dmitry Torokhov wrote:
> > On Fri, Jan 25, 2019 at 06:50:45PM +0100, Paweł Chmiel wrote:
> >> From: Jonathan Bakker <xc-racer2@live.ca>
> >>
> >> This commit adds documentation for Sharp GP2AP002A00F.
> >> It's Proximity/Opto Sensor connected over i2c.
> >>
> >> Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
> >> Signed-off-by: Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com>
> >> ---
> >>  .../bindings/input/sharp,gp2ap002a00f.txt     | 29 +++++++++++++++++++
> >>  1 file changed, 29 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/input/sharp,gp2ap002a00f.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/input/sharp,gp2ap002a00f.txt b/Documentation/devicetree/bindings/input/sharp,gp2ap002a00f.txt
> >> new file mode 100644
> >> index 000000000000..c524eb7d3d60
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/input/sharp,gp2ap002a00f.txt
> >> @@ -0,0 +1,29 @@
> >> +* Sharp GP2AP002A00F I2C Proximity/Opto Sensor
> >> +
> >> +Required properties:
> >> +- compatible : Should be "sharp,gp2ap002a00f"
> >> +- reg : The I2C address of the sensor
> >> +- vout-gpio : The gpio connected to the vout pin
> > 
> > Do you know what it is for?
> > 
> It's the control of the main power supply to the chip.

In this case it should be a power supply (regulator), not gpio.

Thanks.

-- 
Dmitry

^ permalink raw reply

* Re: [PATCH 12/13] input: max77650: add onkey support
From: Dmitry Torokhov @ 2019-01-28 19:22 UTC (permalink / raw)
  To: Bartosz Golaszewski
  Cc: Rob Herring, Mark Rutland, Linus Walleij, Jacek Anaszewski,
	Pavel Machek, Lee Jones, Sebastian Reichel, Liam Girdwood,
	Mark Brown, Greg Kroah-Hartman, Linux Kernel Mailing List,
	open list:GPIO SUBSYSTEM, devicetree, linux-input, linux-leds,
	linux-pm, Bartosz Golaszewski
In-Reply-To: <CAMRc=MfOWWK+nfYLt03EFe-ekTnbDr6oCoqd=n3dpMjz1sELMQ@mail.gmail.com>

On Mon, Jan 21, 2019 at 11:52:50AM +0100, Bartosz Golaszewski wrote:
> sob., 19 sty 2019 o 10:03 Dmitry Torokhov <dmitry.torokhov@gmail.com>
> napisał(a):
> >
> > Hi Bartosz,
> >
> > On Fri, Jan 18, 2019 at 02:42:43PM +0100, Bartosz Golaszewski wrote:
> > > +     input_set_capability(onkey->input, EV_KEY, onkey->code);
> > > +
> > > +     rv = devm_request_threaded_irq(dev, irq_f, NULL,
> >
> > Why threaded interrupt with only hard interrupt handler? If parent
> > interrupt is threaded use "any_context_irq" here.
> >
> 
> Hi Dmitry,
> 
> actually it's the other way around. Take a look at the function
> prototype for  devm_request_threaded_irq()[1].
> 
> The third parameter is the hard-irq handler (NULL in my patch), the
> fourth is the thread function. Actually even if I did what you're
> saying - it would never work as this is a nested irq for which the
> hard-irq handler is never called.

Sorry, my eyes must have crossed. Still, from the driver POV the
interrupt does not have to be threaded, this is dictated by the
constraints beyond the driver control. For these cases we have
devm_request_any_context_irq() that takes essentially only "hard" IRQ
handler, but internally either does request_irq() or
request_threaded_irq(), depending on the context (whether the interrupt
is nested or not). Using devm_request_any_context_irq() should not have
any behavioral changes, but documents the logic better.

Thanks.

-- 
Dmitry

^ permalink raw reply

* Re: [PATCH 2/3] Input: st1232 - add support for st1633
From: Martin Kepplinger @ 2019-01-28 19:10 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: devicetree, linux-input, robh+dt, mark.rutland, linux-kernel,
	Martin Kepplinger
In-Reply-To: <20190128190334.GA34692@dtor-ws>

Am 28.01.2019 20:03 schrieb Dmitry Torokhov:
> Hi Martin,
> 
> On Mon, Jan 28, 2019 at 09:44:48AM +0100, Martin Kepplinger wrote:
>> From: Martin Kepplinger <martin.kepplinger@ginzinger.com>
>> 
>> Add support for the Sitronix ST1633 touchscreen controller to the 
>> st1232
>> driver. A protocol spec can be found here:
>> www.ampdisplay.com/documents/pdf/AM-320480B6TZQW-TC0H.pdf
>> 
>> Signed-off-by: Martin Kepplinger <martin.kepplinger@ginzinger.com>
>> ---
>>  drivers/input/touchscreen/Kconfig  |   6 +-
>>  drivers/input/touchscreen/st1232.c | 122 
>> +++++++++++++++++++++--------
>>  2 files changed, 94 insertions(+), 34 deletions(-)
>> 
>> diff --git a/drivers/input/touchscreen/Kconfig 
>> b/drivers/input/touchscreen/Kconfig
>> index 068dbbc610fc..7c597a49c265 100644
>> --- a/drivers/input/touchscreen/Kconfig
>> +++ b/drivers/input/touchscreen/Kconfig
>> @@ -1168,11 +1168,11 @@ config TOUCHSCREEN_SIS_I2C
>>  	  module will be called sis_i2c.
>> 
>>  config TOUCHSCREEN_ST1232
>> -	tristate "Sitronix ST1232 touchscreen controllers"
>> +	tristate "Sitronix ST1232 or ST1633 touchscreen controllers"
>>  	depends on I2C
>>  	help
>> -	  Say Y here if you want to support Sitronix ST1232
>> -	  touchscreen controller.
>> +	  Say Y here if you want to support the Sitronix ST1232
>> +	  or ST1633 touchscreen controller.
>> 
>>  	  If unsure, say N.
>> 
>> diff --git a/drivers/input/touchscreen/st1232.c 
>> b/drivers/input/touchscreen/st1232.c
>> index 11ff32c68025..19a665d48dad 100644
>> --- a/drivers/input/touchscreen/st1232.c
>> +++ b/drivers/input/touchscreen/st1232.c
>> @@ -23,13 +23,15 @@
>>  #include <linux/types.h>
>> 
>>  #define ST1232_TS_NAME	"st1232-ts"
>> +#define ST1633_TS_NAME	"st1633-ts"
>> +
>> +enum {
>> +	st1232,
>> +	st1633,
>> +};
> 
> This enum seems no longer needed, I dropped it.
> 
>> 
>>  #define MIN_X		0x00
>>  #define MIN_Y		0x00
> 
> Given we no longer have constant MAX_X/Y I simply used 0 in
> input_set_abs_params() and dropped MIN-X/Y.
> 
>> -#define MAX_X		0x31f	/* (800 - 1) */
>> -#define MAX_Y		0x1df	/* (480 - 1) */
>> -#define MAX_AREA	0xff
>> -#define MAX_FINGERS	2
>> 
>>  struct st1232_ts_finger {
>>  	u16 x;
>> @@ -38,12 +40,24 @@ struct st1232_ts_finger {
>>  	bool is_valid;
>>  };
>> 
>> +struct st_chip_info {
>> +	bool	have_z;
>> +	u16	max_x;
>> +	u16	max_y;
>> +	u16	max_area;
>> +	u16	max_fingers;
>> +	u8	start_reg;
>> +};
>> +
>>  struct st1232_ts_data {
>>  	struct i2c_client *client;
>>  	struct input_dev *input_dev;
>> -	struct st1232_ts_finger finger[MAX_FINGERS];
>>  	struct dev_pm_qos_request low_latency_req;
>>  	int reset_gpio;
> 
> Could you please create an additional patch converting this to gpiod?
> Instead of doing of_get_gpio()/gpio_is_valid()/devm_gpio_request() 
> smply
> do
> 
> 	ts->reset_gpio = devm_gpiod_get_optional();
> 	if (IS_ERR(ts->reset_gpio)) {
> 		...
> 	}
> 
> and later
> 
> 	if (ts->reset_gpio)
> 		...
> 
> Looking at the code it looks like reset is as usual active low, so you
> will need to invert the logic as gpiod takes care of convertion logical
> value to proper level (active low or high).

I'll test your applied changes and get back to this tomorrow.

thanks.

> 
>> +	const struct st_chip_info *chip_info;
>> +	int read_buf_len;
>> +	u8 *read_buf;
>> +	struct st1232_ts_finger *finger;
>>  };
>> 
>>  static int st1232_ts_read_data(struct st1232_ts_data *ts)
>> @@ -52,40 +66,35 @@ static int st1232_ts_read_data(struct 
>> st1232_ts_data *ts)
>>  	struct i2c_client *client = ts->client;
>>  	struct i2c_msg msg[2];
>>  	int error;
>> -	u8 start_reg;
>> -	u8 buf[10];
>> +	int i, y;
>> +	u8 start_reg = ts->chip_info->start_reg;
>> +	u8 *buf = ts->read_buf;
>> 
>> -	/* read touchscreen data from ST1232 */
>> +	/* read touchscreen data */
>>  	msg[0].addr = client->addr;
>>  	msg[0].flags = 0;
>>  	msg[0].len = 1;
>>  	msg[0].buf = &start_reg;
>> -	start_reg = 0x10;
>> 
>>  	msg[1].addr = ts->client->addr;
>>  	msg[1].flags = I2C_M_RD;
>> -	msg[1].len = sizeof(buf);
>> +	msg[1].len = ts->read_buf_len;
>>  	msg[1].buf = buf;
>> 
>>  	error = i2c_transfer(client->adapter, msg, 2);
>>  	if (error < 0)
>>  		return error;
>> 
>> -	/* get "valid" bits */
>> -	finger[0].is_valid = buf[2] >> 7;
>> -	finger[1].is_valid = buf[5] >> 7;
>> +	for (i = 0, y = 0; i < ts->chip_info->max_fingers; i++, y += 3) {
>> +		finger[i].is_valid = buf[i + y] >> 7;
>> +		if (finger[i].is_valid) {
>> +			finger[i].x = ((buf[i + y] & 0x0070) << 4) | buf[i + 1];
>> +			finger[i].y = ((buf[i + y] & 0x0007) << 8) | buf[i + 2];
>> 
>> -	/* get xy coordinate */
>> -	if (finger[0].is_valid) {
>> -		finger[0].x = ((buf[2] & 0x0070) << 4) | buf[3];
>> -		finger[0].y = ((buf[2] & 0x0007) << 8) | buf[4];
>> -		finger[0].t = buf[8];
>> -	}
>> -
>> -	if (finger[1].is_valid) {
>> -		finger[1].x = ((buf[5] & 0x0070) << 4) | buf[6];
>> -		finger[1].y = ((buf[5] & 0x0007) << 8) | buf[7];
>> -		finger[1].t = buf[9];
>> +			/* st1232 includes a z-axis / touch strength */
>> +			if (ts->chip_info->have_z)
>> +				finger[i].t = buf[i + 6];
>> +		}
>>  	}
>> 
>>  	return 0;
>> @@ -104,11 +113,14 @@ static irqreturn_t st1232_ts_irq_handler(int 
>> irq, void *dev_id)
>>  		goto end;
>> 
>>  	/* multi touch protocol */
>> -	for (i = 0; i < MAX_FINGERS; i++) {
>> +	for (i = 0; i < ts->chip_info->max_fingers; i++) {
>>  		if (!finger[i].is_valid)
>>  			continue;
>> 
>> -		input_report_abs(input_dev, ABS_MT_TOUCH_MAJOR, finger[i].t);
>> +		if (ts->chip_info->have_z)
>> +			input_report_abs(input_dev, ABS_MT_TOUCH_MAJOR,
>> +					 finger[i].t);
>> +
>>  		input_report_abs(input_dev, ABS_MT_POSITION_X, finger[i].x);
>>  		input_report_abs(input_dev, ABS_MT_POSITION_Y, finger[i].y);
>>  		input_mt_sync(input_dev);
>> @@ -142,12 +154,40 @@ static void st1232_ts_power(struct 
>> st1232_ts_data *ts, bool poweron)
>>  		gpio_direction_output(ts->reset_gpio, poweron);
>>  }
>> 
>> +static const struct st_chip_info st1232_chip_info = {
>> +		.have_z = true,
>> +		.max_x = 0x31f, /* 800 - 1 */
>> +		.max_y = 0x1df, /* 480 -1 */
>> +		.max_area = 0xff,
>> +		.max_fingers = 2,
>> +		.start_reg = 0x12,
>> +};
>> +
>> +static const struct st_chip_info st1633_chip_info = {
>> +		.have_z = false,
>> +		.max_x = 0x13f, /* 320 - 1 */
>> +		.max_y = 0x1df, /* 480 -1 */
>> +		.max_area = 0x00,
>> +		.max_fingers = 5,
>> +		.start_reg = 0x12,
>> +};
>> +
>>  static int st1232_ts_probe(struct i2c_client *client,
>>  			   const struct i2c_device_id *id)
>>  {
>>  	struct st1232_ts_data *ts;
>> +	struct st1232_ts_finger *finger;
>>  	struct input_dev *input_dev;
>>  	int error;
>> +	const struct st_chip_info *match = NULL;
> 
> There is no need to initialize with NULL given that we do unconditional
> assignment below. I removed initialization.
> 
>> +
>> +	match = device_get_match_data(&client->dev);
>> +	if (!match && id)
>> +		match = (const void *)id->driver_data;
>> +	if (!match) {
>> +		dev_err(&client->dev, "unknown device model\n");
>> +		return -ENODEV;
>> +	}
>> 
>>  	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
>>  		dev_err(&client->dev, "need I2C_FUNC_I2C\n");
>> @@ -163,6 +203,19 @@ static int st1232_ts_probe(struct i2c_client 
>> *client,
>>  	if (!ts)
>>  		return -ENOMEM;
>> 
>> +	ts->chip_info = match;
>> +	ts->finger = devm_kzalloc(&client->dev,
>> +				  sizeof(*finger) * ts->chip_info->max_fingers,
>> +				  GFP_KERNEL);
> 
> I replaced it with devm_kcalloc(&client->dev,
> 				ts->chip_info->max_fingers, sizeof(*finger),
> 				GFP_KERNEL);
> 
> and applied.
> 
>> +	if (!ts->finger)
>> +		return -ENOMEM;
>> +
>> +	/* allocate a buffer according to the number of registers to read */
>> +	ts->read_buf_len = ts->chip_info->max_fingers * 4;
>> +	ts->read_buf = devm_kzalloc(&client->dev, ts->read_buf_len, 
>> GFP_KERNEL);
>> +	if (!ts->read_buf)
>> +		return -ENOMEM;
>> +
>>  	input_dev = devm_input_allocate_device(&client->dev);
>>  	if (!input_dev)
>>  		return -ENOMEM;
>> @@ -192,9 +245,14 @@ static int st1232_ts_probe(struct i2c_client 
>> *client,
>>  	__set_bit(EV_KEY, input_dev->evbit);
>>  	__set_bit(EV_ABS, input_dev->evbit);
>> 
>> -	input_set_abs_params(input_dev, ABS_MT_TOUCH_MAJOR, 0, MAX_AREA, 0, 
>> 0);
>> -	input_set_abs_params(input_dev, ABS_MT_POSITION_X, MIN_X, MAX_X, 0, 
>> 0);
>> -	input_set_abs_params(input_dev, ABS_MT_POSITION_Y, MIN_Y, MAX_Y, 0, 
>> 0);
>> +	if (ts->chip_info->have_z)
>> +		input_set_abs_params(input_dev, ABS_MT_TOUCH_MAJOR, 0,
>> +				     ts->chip_info->max_area, 0, 0);
>> +
>> +	input_set_abs_params(input_dev, ABS_MT_POSITION_X,
>> +			     MIN_X, ts->chip_info->max_x, 0, 0);
>> +	input_set_abs_params(input_dev, ABS_MT_POSITION_Y,
>> +			     MIN_Y, ts->chip_info->max_y, 0, 0);
>> 
>>  	error = devm_request_threaded_irq(&client->dev, client->irq,
>>  					  NULL, st1232_ts_irq_handler,
>> @@ -261,13 +319,15 @@ static SIMPLE_DEV_PM_OPS(st1232_ts_pm_ops,
>>  			 st1232_ts_suspend, st1232_ts_resume);
>> 
>>  static const struct i2c_device_id st1232_ts_id[] = {
>> -	{ ST1232_TS_NAME, 0 },
>> +	{ ST1232_TS_NAME, (unsigned long)&st1232_chip_info },
>> +	{ ST1633_TS_NAME, (unsigned long)&st1633_chip_info },
>>  	{ }
>>  };
>>  MODULE_DEVICE_TABLE(i2c, st1232_ts_id);
>> 
>>  static const struct of_device_id st1232_ts_dt_ids[] = {
>> -	{ .compatible = "sitronix,st1232", },
>> +	{ .compatible = "sitronix,st1232", .data = &st1232_chip_info },
>> +	{ .compatible = "sitronix,st1633", .data = &st1633_chip_info },
>>  	{ }
>>  };
>>  MODULE_DEVICE_TABLE(of, st1232_ts_dt_ids);
>> --
>> 2.20.1
>> 
> 
> Thanks.

^ permalink raw reply

* Re: [PATCH AUTOSEL 4.20 148/304] Input: rotary-encoder - don't log EPROBE_DEFER to kernel log
From: Sasha Levin @ 2019-01-28 19:08 UTC (permalink / raw)
  To: Dmitry Torokhov; +Cc: linux-kernel, stable, Uwe Kleine-König, linux-input
In-Reply-To: <20190128182405.GA30191@dtor-ws>

On Mon, Jan 28, 2019 at 10:24:05AM -0800, Dmitry Torokhov wrote:
>Hi Sasha,
>
>On Mon, Jan 28, 2019 at 10:41:05AM -0500, Sasha Levin wrote:
>> From: Uwe Kleine-König <uwe@kleine-koenig.org>
>>
>> [ Upstream commit 0832e93632c61987d504e251b927a2be769dd21a ]
>>
>> When a driver fails to bind because a resource it still missing it's not
>> helpful to report this as (usually) probing is repeated later.
>
>I do not think that patch that merely suppresses a harmless warning in
>dmesg belongs in stable.

I'll drop it.

FWIW, my experience with users is that unexplained warnings in dmesg
sometimes end up being even more trouble than an actual bug :)

--
Thanks,
Sasha

^ permalink raw reply

* Re: [PATCH 3/3] Input: st1232 - add Martin as module author
From: Dmitry Torokhov @ 2019-01-28 19:04 UTC (permalink / raw)
  To: Martin Kepplinger
  Cc: devicetree, linux-input, robh+dt, mark.rutland, linux-kernel,
	Martin Kepplinger
In-Reply-To: <20190128084449.16070-3-martink@posteo.de>

On Mon, Jan 28, 2019 at 09:44:49AM +0100, Martin Kepplinger wrote:
> From: Martin Kepplinger <martin.kepplinger@ginzinger.com>
> 
> This adds myself as an author of the st1232 driver module as Tony's
> email address doesn't seem to work anymore.
> 
> Signed-off-by: Martin Kepplinger <martin.kepplinger@ginzinger.com>

Applied, thank you.

> ---
>  drivers/input/touchscreen/st1232.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/input/touchscreen/st1232.c b/drivers/input/touchscreen/st1232.c
> index 19a665d48dad..906b233970aa 100644
> --- a/drivers/input/touchscreen/st1232.c
> +++ b/drivers/input/touchscreen/st1232.c
> @@ -346,5 +346,6 @@ static struct i2c_driver st1232_ts_driver = {
>  module_i2c_driver(st1232_ts_driver);
>  
>  MODULE_AUTHOR("Tony SIM <chinyeow.sim.xt@renesas.com>");
> +MODULE_AUTHOR("Martin Kepplinger <martin.kepplinger@ginzinger.com>");
>  MODULE_DESCRIPTION("SITRONIX ST1232 Touchscreen Controller Driver");
>  MODULE_LICENSE("GPL v2");
> -- 
> 2.20.1
> 

-- 
Dmitry

^ permalink raw reply

* Re: [PATCH 2/3] Input: st1232 - add support for st1633
From: Dmitry Torokhov @ 2019-01-28 19:03 UTC (permalink / raw)
  To: Martin Kepplinger
  Cc: devicetree, linux-input, robh+dt, mark.rutland, linux-kernel,
	Martin Kepplinger
In-Reply-To: <20190128084449.16070-2-martink@posteo.de>

Hi Martin,

On Mon, Jan 28, 2019 at 09:44:48AM +0100, Martin Kepplinger wrote:
> From: Martin Kepplinger <martin.kepplinger@ginzinger.com>
> 
> Add support for the Sitronix ST1633 touchscreen controller to the st1232
> driver. A protocol spec can be found here:
> www.ampdisplay.com/documents/pdf/AM-320480B6TZQW-TC0H.pdf
> 
> Signed-off-by: Martin Kepplinger <martin.kepplinger@ginzinger.com>
> ---
>  drivers/input/touchscreen/Kconfig  |   6 +-
>  drivers/input/touchscreen/st1232.c | 122 +++++++++++++++++++++--------
>  2 files changed, 94 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> index 068dbbc610fc..7c597a49c265 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -1168,11 +1168,11 @@ config TOUCHSCREEN_SIS_I2C
>  	  module will be called sis_i2c.
>  
>  config TOUCHSCREEN_ST1232
> -	tristate "Sitronix ST1232 touchscreen controllers"
> +	tristate "Sitronix ST1232 or ST1633 touchscreen controllers"
>  	depends on I2C
>  	help
> -	  Say Y here if you want to support Sitronix ST1232
> -	  touchscreen controller.
> +	  Say Y here if you want to support the Sitronix ST1232
> +	  or ST1633 touchscreen controller.
>  
>  	  If unsure, say N.
>  
> diff --git a/drivers/input/touchscreen/st1232.c b/drivers/input/touchscreen/st1232.c
> index 11ff32c68025..19a665d48dad 100644
> --- a/drivers/input/touchscreen/st1232.c
> +++ b/drivers/input/touchscreen/st1232.c
> @@ -23,13 +23,15 @@
>  #include <linux/types.h>
>  
>  #define ST1232_TS_NAME	"st1232-ts"
> +#define ST1633_TS_NAME	"st1633-ts"
> +
> +enum {
> +	st1232,
> +	st1633,
> +};

This enum seems no longer needed, I dropped it.

>  
>  #define MIN_X		0x00
>  #define MIN_Y		0x00

Given we no longer have constant MAX_X/Y I simply used 0 in
input_set_abs_params() and dropped MIN-X/Y.

> -#define MAX_X		0x31f	/* (800 - 1) */
> -#define MAX_Y		0x1df	/* (480 - 1) */
> -#define MAX_AREA	0xff
> -#define MAX_FINGERS	2
>  
>  struct st1232_ts_finger {
>  	u16 x;
> @@ -38,12 +40,24 @@ struct st1232_ts_finger {
>  	bool is_valid;
>  };
>  
> +struct st_chip_info {
> +	bool	have_z;
> +	u16	max_x;
> +	u16	max_y;
> +	u16	max_area;
> +	u16	max_fingers;
> +	u8	start_reg;
> +};
> +
>  struct st1232_ts_data {
>  	struct i2c_client *client;
>  	struct input_dev *input_dev;
> -	struct st1232_ts_finger finger[MAX_FINGERS];
>  	struct dev_pm_qos_request low_latency_req;
>  	int reset_gpio;

Could you please create an additional patch converting this to gpiod?
Instead of doing of_get_gpio()/gpio_is_valid()/devm_gpio_request() smply
do

	ts->reset_gpio = devm_gpiod_get_optional();
	if (IS_ERR(ts->reset_gpio)) {
		...
	}

and later

	if (ts->reset_gpio)
		...

Looking at the code it looks like reset is as usual active low, so you
will need to invert the logic as gpiod takes care of convertion logical
value to proper level (active low or high).

> +	const struct st_chip_info *chip_info;
> +	int read_buf_len;
> +	u8 *read_buf;
> +	struct st1232_ts_finger *finger;
>  };
>  
>  static int st1232_ts_read_data(struct st1232_ts_data *ts)
> @@ -52,40 +66,35 @@ static int st1232_ts_read_data(struct st1232_ts_data *ts)
>  	struct i2c_client *client = ts->client;
>  	struct i2c_msg msg[2];
>  	int error;
> -	u8 start_reg;
> -	u8 buf[10];
> +	int i, y;
> +	u8 start_reg = ts->chip_info->start_reg;
> +	u8 *buf = ts->read_buf;
>  
> -	/* read touchscreen data from ST1232 */
> +	/* read touchscreen data */
>  	msg[0].addr = client->addr;
>  	msg[0].flags = 0;
>  	msg[0].len = 1;
>  	msg[0].buf = &start_reg;
> -	start_reg = 0x10;
>  
>  	msg[1].addr = ts->client->addr;
>  	msg[1].flags = I2C_M_RD;
> -	msg[1].len = sizeof(buf);
> +	msg[1].len = ts->read_buf_len;
>  	msg[1].buf = buf;
>  
>  	error = i2c_transfer(client->adapter, msg, 2);
>  	if (error < 0)
>  		return error;
>  
> -	/* get "valid" bits */
> -	finger[0].is_valid = buf[2] >> 7;
> -	finger[1].is_valid = buf[5] >> 7;
> +	for (i = 0, y = 0; i < ts->chip_info->max_fingers; i++, y += 3) {
> +		finger[i].is_valid = buf[i + y] >> 7;
> +		if (finger[i].is_valid) {
> +			finger[i].x = ((buf[i + y] & 0x0070) << 4) | buf[i + 1];
> +			finger[i].y = ((buf[i + y] & 0x0007) << 8) | buf[i + 2];
>  
> -	/* get xy coordinate */
> -	if (finger[0].is_valid) {
> -		finger[0].x = ((buf[2] & 0x0070) << 4) | buf[3];
> -		finger[0].y = ((buf[2] & 0x0007) << 8) | buf[4];
> -		finger[0].t = buf[8];
> -	}
> -
> -	if (finger[1].is_valid) {
> -		finger[1].x = ((buf[5] & 0x0070) << 4) | buf[6];
> -		finger[1].y = ((buf[5] & 0x0007) << 8) | buf[7];
> -		finger[1].t = buf[9];
> +			/* st1232 includes a z-axis / touch strength */
> +			if (ts->chip_info->have_z)
> +				finger[i].t = buf[i + 6];
> +		}
>  	}
>  
>  	return 0;
> @@ -104,11 +113,14 @@ static irqreturn_t st1232_ts_irq_handler(int irq, void *dev_id)
>  		goto end;
>  
>  	/* multi touch protocol */
> -	for (i = 0; i < MAX_FINGERS; i++) {
> +	for (i = 0; i < ts->chip_info->max_fingers; i++) {
>  		if (!finger[i].is_valid)
>  			continue;
>  
> -		input_report_abs(input_dev, ABS_MT_TOUCH_MAJOR, finger[i].t);
> +		if (ts->chip_info->have_z)
> +			input_report_abs(input_dev, ABS_MT_TOUCH_MAJOR,
> +					 finger[i].t);
> +
>  		input_report_abs(input_dev, ABS_MT_POSITION_X, finger[i].x);
>  		input_report_abs(input_dev, ABS_MT_POSITION_Y, finger[i].y);
>  		input_mt_sync(input_dev);
> @@ -142,12 +154,40 @@ static void st1232_ts_power(struct st1232_ts_data *ts, bool poweron)
>  		gpio_direction_output(ts->reset_gpio, poweron);
>  }
>  
> +static const struct st_chip_info st1232_chip_info = {
> +		.have_z = true,
> +		.max_x = 0x31f, /* 800 - 1 */
> +		.max_y = 0x1df, /* 480 -1 */
> +		.max_area = 0xff,
> +		.max_fingers = 2,
> +		.start_reg = 0x12,
> +};
> +
> +static const struct st_chip_info st1633_chip_info = {
> +		.have_z = false,
> +		.max_x = 0x13f, /* 320 - 1 */
> +		.max_y = 0x1df, /* 480 -1 */
> +		.max_area = 0x00,
> +		.max_fingers = 5,
> +		.start_reg = 0x12,
> +};
> +
>  static int st1232_ts_probe(struct i2c_client *client,
>  			   const struct i2c_device_id *id)
>  {
>  	struct st1232_ts_data *ts;
> +	struct st1232_ts_finger *finger;
>  	struct input_dev *input_dev;
>  	int error;
> +	const struct st_chip_info *match = NULL;

There is no need to initialize with NULL given that we do unconditional
assignment below. I removed initialization.

> +
> +	match = device_get_match_data(&client->dev);
> +	if (!match && id)
> +		match = (const void *)id->driver_data;
> +	if (!match) {
> +		dev_err(&client->dev, "unknown device model\n");
> +		return -ENODEV;
> +	}
>  
>  	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
>  		dev_err(&client->dev, "need I2C_FUNC_I2C\n");
> @@ -163,6 +203,19 @@ static int st1232_ts_probe(struct i2c_client *client,
>  	if (!ts)
>  		return -ENOMEM;
>  
> +	ts->chip_info = match;
> +	ts->finger = devm_kzalloc(&client->dev,
> +				  sizeof(*finger) * ts->chip_info->max_fingers,
> +				  GFP_KERNEL);

I replaced it with devm_kcalloc(&client->dev,
				ts->chip_info->max_fingers, sizeof(*finger),
				GFP_KERNEL);

and applied.

> +	if (!ts->finger)
> +		return -ENOMEM;
> +
> +	/* allocate a buffer according to the number of registers to read */
> +	ts->read_buf_len = ts->chip_info->max_fingers * 4;
> +	ts->read_buf = devm_kzalloc(&client->dev, ts->read_buf_len, GFP_KERNEL);
> +	if (!ts->read_buf)
> +		return -ENOMEM;
> +
>  	input_dev = devm_input_allocate_device(&client->dev);
>  	if (!input_dev)
>  		return -ENOMEM;
> @@ -192,9 +245,14 @@ static int st1232_ts_probe(struct i2c_client *client,
>  	__set_bit(EV_KEY, input_dev->evbit);
>  	__set_bit(EV_ABS, input_dev->evbit);
>  
> -	input_set_abs_params(input_dev, ABS_MT_TOUCH_MAJOR, 0, MAX_AREA, 0, 0);
> -	input_set_abs_params(input_dev, ABS_MT_POSITION_X, MIN_X, MAX_X, 0, 0);
> -	input_set_abs_params(input_dev, ABS_MT_POSITION_Y, MIN_Y, MAX_Y, 0, 0);
> +	if (ts->chip_info->have_z)
> +		input_set_abs_params(input_dev, ABS_MT_TOUCH_MAJOR, 0,
> +				     ts->chip_info->max_area, 0, 0);
> +
> +	input_set_abs_params(input_dev, ABS_MT_POSITION_X,
> +			     MIN_X, ts->chip_info->max_x, 0, 0);
> +	input_set_abs_params(input_dev, ABS_MT_POSITION_Y,
> +			     MIN_Y, ts->chip_info->max_y, 0, 0);
>  
>  	error = devm_request_threaded_irq(&client->dev, client->irq,
>  					  NULL, st1232_ts_irq_handler,
> @@ -261,13 +319,15 @@ static SIMPLE_DEV_PM_OPS(st1232_ts_pm_ops,
>  			 st1232_ts_suspend, st1232_ts_resume);
>  
>  static const struct i2c_device_id st1232_ts_id[] = {
> -	{ ST1232_TS_NAME, 0 },
> +	{ ST1232_TS_NAME, (unsigned long)&st1232_chip_info },
> +	{ ST1633_TS_NAME, (unsigned long)&st1633_chip_info },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(i2c, st1232_ts_id);
>  
>  static const struct of_device_id st1232_ts_dt_ids[] = {
> -	{ .compatible = "sitronix,st1232", },
> +	{ .compatible = "sitronix,st1232", .data = &st1232_chip_info },
> +	{ .compatible = "sitronix,st1633", .data = &st1633_chip_info },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(of, st1232_ts_dt_ids);
> -- 
> 2.20.1
> 

Thanks.

-- 
Dmitry

^ permalink raw reply

* Re: [PATCH 1/3] input: keyboard: allow to select IMX SNVS Power Key driver for i.MX 7D
From: Dmitry Torokhov @ 2019-01-28 18:26 UTC (permalink / raw)
  To: Stefan Agner
  Cc: arnd, peng.fan, linux-imx, kernel, fabio.estevam, shawnguo,
	linux-input, linux-kernel
In-Reply-To: <20190128160305.22398-1-stefan@agner.ch>

On Mon, Jan 28, 2019 at 05:03:05PM +0100, Stefan Agner wrote:
> The i.MX SNVS Power Key driver supports the i.MX 7D SoC family too.
> Allow to enable the i.MX SNVS Power Key driver even if only i.MX 7D
> SoC is selected.
> 
> Signed-off-by: Stefan Agner <stefan@agner.ch>

Applied, thank you.

> ---
>  drivers/input/keyboard/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
> index 4713957b0cbb..a878351f1643 100644
> --- a/drivers/input/keyboard/Kconfig
> +++ b/drivers/input/keyboard/Kconfig
> @@ -420,7 +420,7 @@ config KEYBOARD_MPR121
>  
>  config KEYBOARD_SNVS_PWRKEY
>  	tristate "IMX SNVS Power Key Driver"
> -	depends on SOC_IMX6SX
> +	depends on SOC_IMX6SX || SOC_IMX7D
>  	depends on OF
>  	help
>  	  This is the snvs powerkey driver for the Freescale i.MX application
> -- 
> 2.20.1
> 

-- 
Dmitry

^ permalink raw reply

* Re: [PATCH] input: goodix - support Goodix gt5688
From: Bastien Nocera @ 2019-01-28 18:24 UTC (permalink / raw)
  To: Guido Günther, Dmitry Torokhov, Rob Herring, Mark Rutland,
	Matthias Brugger, linux-input, devicetree, linux-kernel,
	linux-arm-kernel, linux-mediatek
In-Reply-To: <20190128170310.GA305@bogon.m.sigxcpu.org>

On Mon, 2019-01-28 at 18:03 +0100, Guido Günther wrote:
> From what I've seen in vendor trees it's fine to treat this as gt1x¹.
> 
> [1]: https://github.com/TadiT7/android_kernel_mtk-4.4/tree/master/drivers/input/touchscreen/mediatek/GT5688

Can you please point to the exact line of code that makes you say that?
I'm not saying it's not compatible, but it's not the same driver that
the current goodix.c was based on, or even goodix.c.

Can you please elaborate?

Finding that data in the specs would also be fine:
https://github.com/hadess/gt9xx/tree/master/specifications

Cheers

[1]: original driver:
https://github.com/hadess/gt9xx/commit/82b141220e8bce00060e0de697735d0a70af2678

> Signed-off-by: Guido Günther <agx@sigxcpu.org>
> ---
>  Documentation/devicetree/bindings/input/touchscreen/goodix.txt | 1 +
>  drivers/input/touchscreen/goodix.c                             | 2
> ++
>  2 files changed, 3 insertions(+)
> 
> diff --git
> a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> index f7e95c52f3c7..57d3d8870a09 100644
> --- a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> +++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> @@ -3,6 +3,7 @@ Device tree bindings for Goodix GT9xx series
> touchscreen controller
>  Required properties:
>  
>   - compatible		: Should be "goodix,gt1151"
> +				 or "goodix,gt5688"
>  				 or "goodix,gt911"
>  				 or "goodix,gt9110"
>  				 or "goodix,gt912"
> diff --git a/drivers/input/touchscreen/goodix.c
> b/drivers/input/touchscreen/goodix.c
> index f2d9c2c41885..47b1ced41576 100644
> --- a/drivers/input/touchscreen/goodix.c
> +++ b/drivers/input/touchscreen/goodix.c
> @@ -216,6 +216,7 @@ static const struct goodix_chip_data
> *goodix_get_chip_data(u16 id)
>  {
>  	switch (id) {
>  	case 1151:
> +	case 5688:
>  		return &gt1x_chip_data;
>  
>  	case 911:
> @@ -942,6 +943,7 @@ MODULE_DEVICE_TABLE(acpi, goodix_acpi_match);
>  #ifdef CONFIG_OF
>  static const struct of_device_id goodix_of_match[] = {
>  	{ .compatible = "goodix,gt1151" },
> +	{ .compatible = "goodix,gt5688" },
>  	{ .compatible = "goodix,gt911" },
>  	{ .compatible = "goodix,gt9110" },
>  	{ .compatible = "goodix,gt912" },

^ permalink raw reply

* Re: [PATCH AUTOSEL 4.20 148/304] Input: rotary-encoder - don't log EPROBE_DEFER to kernel log
From: Dmitry Torokhov @ 2019-01-28 18:24 UTC (permalink / raw)
  To: Sasha Levin; +Cc: linux-kernel, stable, Uwe Kleine-König, linux-input
In-Reply-To: <20190128154341.47195-148-sashal@kernel.org>

Hi Sasha,

On Mon, Jan 28, 2019 at 10:41:05AM -0500, Sasha Levin wrote:
> From: Uwe Kleine-König <uwe@kleine-koenig.org>
> 
> [ Upstream commit 0832e93632c61987d504e251b927a2be769dd21a ]
> 
> When a driver fails to bind because a resource it still missing it's not
> helpful to report this as (usually) probing is repeated later.

I do not think that patch that merely suppresses a harmless warning in
dmesg belongs in stable.

> 
> Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Signed-off-by: Sasha Levin <sashal@kernel.org>
> ---
>  drivers/input/misc/rotary_encoder.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/input/misc/rotary_encoder.c b/drivers/input/misc/rotary_encoder.c
> index 30ec77ad32c6..d748897bf5e9 100644
> --- a/drivers/input/misc/rotary_encoder.c
> +++ b/drivers/input/misc/rotary_encoder.c
> @@ -240,8 +240,10 @@ static int rotary_encoder_probe(struct platform_device *pdev)
>  
>  	encoder->gpios = devm_gpiod_get_array(dev, NULL, GPIOD_IN);
>  	if (IS_ERR(encoder->gpios)) {
> -		dev_err(dev, "unable to get gpios\n");
> -		return PTR_ERR(encoder->gpios);
> +		err = PTR_ERR(encoder->gpios);
> +		if (err != -EPROBE_DEFER)
> +			dev_err(dev, "unable to get gpios: %d\n", err);
> +		return err;
>  	}
>  	if (encoder->gpios->ndescs < 2) {
>  		dev_err(dev, "not enough gpios found\n");
> -- 
> 2.19.1
> 

Thanks.

-- 
Dmitry

^ permalink raw reply

* [PATCH] input: goodix - support Goodix gt5688
From: Guido Günther @ 2019-01-28 17:03 UTC (permalink / raw)
  To: Dmitry Torokhov, Rob Herring, Mark Rutland, Bastien Nocera,
	Matthias Brugger, Guido Günther, linux-input, devicetree,
	linux-kernel, linux-arm-kernel, linux-mediatek

>From what I've seen in vendor trees it's fine to treat this as gt1x¹.

[1]: https://github.com/TadiT7/android_kernel_mtk-4.4/tree/master/drivers/input/touchscreen/mediatek/GT5688

Signed-off-by: Guido Günther <agx@sigxcpu.org>
---
 Documentation/devicetree/bindings/input/touchscreen/goodix.txt | 1 +
 drivers/input/touchscreen/goodix.c                             | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
index f7e95c52f3c7..57d3d8870a09 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
+++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
@@ -3,6 +3,7 @@ Device tree bindings for Goodix GT9xx series touchscreen controller
 Required properties:
 
  - compatible		: Should be "goodix,gt1151"
+				 or "goodix,gt5688"
 				 or "goodix,gt911"
 				 or "goodix,gt9110"
 				 or "goodix,gt912"
diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index f2d9c2c41885..47b1ced41576 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -216,6 +216,7 @@ static const struct goodix_chip_data *goodix_get_chip_data(u16 id)
 {
 	switch (id) {
 	case 1151:
+	case 5688:
 		return &gt1x_chip_data;
 
 	case 911:
@@ -942,6 +943,7 @@ MODULE_DEVICE_TABLE(acpi, goodix_acpi_match);
 #ifdef CONFIG_OF
 static const struct of_device_id goodix_of_match[] = {
 	{ .compatible = "goodix,gt1151" },
+	{ .compatible = "goodix,gt5688" },
 	{ .compatible = "goodix,gt911" },
 	{ .compatible = "goodix,gt9110" },
 	{ .compatible = "goodix,gt912" },
-- 
2.20.1

^ permalink raw reply related

* [PATCH AUTOSEL 4.4 77/80] HID: lenovo: Add checks to fix of_led_classdev_register
From: Sasha Levin @ 2019-01-28 16:23 UTC (permalink / raw)
  To: linux-kernel, stable; +Cc: Aditya Pakki, Jiri Kosina, Sasha Levin, linux-input
In-Reply-To: <20190128162401.58841-1-sashal@kernel.org>

From: Aditya Pakki <pakki001@umn.edu>

[ Upstream commit 6ae16dfb61bce538d48b7fe98160fada446056c5 ]

In lenovo_probe_tpkbd(), the function of_led_classdev_register() could
return an error value that is unchecked. The fix adds these checks.

Signed-off-by: Aditya Pakki <pakki001@umn.edu>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 drivers/hid/hid-lenovo.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-lenovo.c b/drivers/hid/hid-lenovo.c
index 8979f1fd5208..24a4a23bdc90 100644
--- a/drivers/hid/hid-lenovo.c
+++ b/drivers/hid/hid-lenovo.c
@@ -703,7 +703,9 @@ static int lenovo_probe_tpkbd(struct hid_device *hdev)
 	data_pointer->led_mute.brightness_get = lenovo_led_brightness_get_tpkbd;
 	data_pointer->led_mute.brightness_set = lenovo_led_brightness_set_tpkbd;
 	data_pointer->led_mute.dev = dev;
-	led_classdev_register(dev, &data_pointer->led_mute);
+	ret = led_classdev_register(dev, &data_pointer->led_mute);
+	if (ret < 0)
+		goto err;
 
 	data_pointer->led_micmute.name = name_micmute;
 	data_pointer->led_micmute.brightness_get =
@@ -711,7 +713,11 @@ static int lenovo_probe_tpkbd(struct hid_device *hdev)
 	data_pointer->led_micmute.brightness_set =
 		lenovo_led_brightness_set_tpkbd;
 	data_pointer->led_micmute.dev = dev;
-	led_classdev_register(dev, &data_pointer->led_micmute);
+	ret = led_classdev_register(dev, &data_pointer->led_micmute);
+	if (ret < 0) {
+		led_classdev_unregister(&data_pointer->led_mute);
+		goto err;
+	}
 
 	lenovo_features_set_tpkbd(hdev);
 
-- 
2.19.1

^ permalink raw reply related

* [PATCH AUTOSEL 4.9 102/107] HID: lenovo: Add checks to fix of_led_classdev_register
From: Sasha Levin @ 2019-01-28 16:19 UTC (permalink / raw)
  To: linux-kernel, stable; +Cc: Aditya Pakki, Jiri Kosina, Sasha Levin, linux-input
In-Reply-To: <20190128161947.57405-1-sashal@kernel.org>

From: Aditya Pakki <pakki001@umn.edu>

[ Upstream commit 6ae16dfb61bce538d48b7fe98160fada446056c5 ]

In lenovo_probe_tpkbd(), the function of_led_classdev_register() could
return an error value that is unchecked. The fix adds these checks.

Signed-off-by: Aditya Pakki <pakki001@umn.edu>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 drivers/hid/hid-lenovo.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-lenovo.c b/drivers/hid/hid-lenovo.c
index 1ac4ff4d57a6..d409cc8759fc 100644
--- a/drivers/hid/hid-lenovo.c
+++ b/drivers/hid/hid-lenovo.c
@@ -713,7 +713,9 @@ static int lenovo_probe_tpkbd(struct hid_device *hdev)
 	data_pointer->led_mute.brightness_get = lenovo_led_brightness_get_tpkbd;
 	data_pointer->led_mute.brightness_set = lenovo_led_brightness_set_tpkbd;
 	data_pointer->led_mute.dev = dev;
-	led_classdev_register(dev, &data_pointer->led_mute);
+	ret = led_classdev_register(dev, &data_pointer->led_mute);
+	if (ret < 0)
+		goto err;
 
 	data_pointer->led_micmute.name = name_micmute;
 	data_pointer->led_micmute.brightness_get =
@@ -721,7 +723,11 @@ static int lenovo_probe_tpkbd(struct hid_device *hdev)
 	data_pointer->led_micmute.brightness_set =
 		lenovo_led_brightness_set_tpkbd;
 	data_pointer->led_micmute.dev = dev;
-	led_classdev_register(dev, &data_pointer->led_micmute);
+	ret = led_classdev_register(dev, &data_pointer->led_micmute);
+	if (ret < 0) {
+		led_classdev_unregister(&data_pointer->led_mute);
+		goto err;
+	}
 
 	lenovo_features_set_tpkbd(hdev);
 
-- 
2.19.1

^ permalink raw reply related

* [PATCH AUTOSEL 4.9 047/107] Input: rotary-encoder - don't log EPROBE_DEFER to kernel log
From: Sasha Levin @ 2019-01-28 16:18 UTC (permalink / raw)
  To: linux-kernel, stable
  Cc: Uwe Kleine-König, Dmitry Torokhov, Sasha Levin, linux-input
In-Reply-To: <20190128161947.57405-1-sashal@kernel.org>

From: Uwe Kleine-König <uwe@kleine-koenig.org>

[ Upstream commit 0832e93632c61987d504e251b927a2be769dd21a ]

When a driver fails to bind because a resource it still missing it's not
helpful to report this as (usually) probing is repeated later.

Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 drivers/input/misc/rotary_encoder.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/input/misc/rotary_encoder.c b/drivers/input/misc/rotary_encoder.c
index 1588aecafff7..72eee6d55527 100644
--- a/drivers/input/misc/rotary_encoder.c
+++ b/drivers/input/misc/rotary_encoder.c
@@ -240,8 +240,10 @@ static int rotary_encoder_probe(struct platform_device *pdev)
 
 	encoder->gpios = devm_gpiod_get_array(dev, NULL, GPIOD_IN);
 	if (IS_ERR(encoder->gpios)) {
-		dev_err(dev, "unable to get gpios\n");
-		return PTR_ERR(encoder->gpios);
+		err = PTR_ERR(encoder->gpios);
+		if (err != -EPROBE_DEFER)
+			dev_err(dev, "unable to get gpios: %d\n", err);
+		return err;
 	}
 	if (encoder->gpios->ndescs < 2) {
 		dev_err(dev, "not enough gpios found\n");
-- 
2.19.1

^ permalink raw reply related

* [PATCH AUTOSEL 4.14 163/170] HID: lenovo: Add checks to fix of_led_classdev_register
From: Sasha Levin @ 2019-01-28 16:11 UTC (permalink / raw)
  To: linux-kernel, stable; +Cc: Aditya Pakki, Jiri Kosina, Sasha Levin, linux-input
In-Reply-To: <20190128161200.55107-1-sashal@kernel.org>

From: Aditya Pakki <pakki001@umn.edu>

[ Upstream commit 6ae16dfb61bce538d48b7fe98160fada446056c5 ]

In lenovo_probe_tpkbd(), the function of_led_classdev_register() could
return an error value that is unchecked. The fix adds these checks.

Signed-off-by: Aditya Pakki <pakki001@umn.edu>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 drivers/hid/hid-lenovo.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-lenovo.c b/drivers/hid/hid-lenovo.c
index 643b6eb54442..eacc76d2ab96 100644
--- a/drivers/hid/hid-lenovo.c
+++ b/drivers/hid/hid-lenovo.c
@@ -743,7 +743,9 @@ static int lenovo_probe_tpkbd(struct hid_device *hdev)
 	data_pointer->led_mute.brightness_get = lenovo_led_brightness_get_tpkbd;
 	data_pointer->led_mute.brightness_set = lenovo_led_brightness_set_tpkbd;
 	data_pointer->led_mute.dev = dev;
-	led_classdev_register(dev, &data_pointer->led_mute);
+	ret = led_classdev_register(dev, &data_pointer->led_mute);
+	if (ret < 0)
+		goto err;
 
 	data_pointer->led_micmute.name = name_micmute;
 	data_pointer->led_micmute.brightness_get =
@@ -751,7 +753,11 @@ static int lenovo_probe_tpkbd(struct hid_device *hdev)
 	data_pointer->led_micmute.brightness_set =
 		lenovo_led_brightness_set_tpkbd;
 	data_pointer->led_micmute.dev = dev;
-	led_classdev_register(dev, &data_pointer->led_micmute);
+	ret = led_classdev_register(dev, &data_pointer->led_micmute);
+	if (ret < 0) {
+		led_classdev_unregister(&data_pointer->led_mute);
+		goto err;
+	}
 
 	lenovo_features_set_tpkbd(hdev);
 
-- 
2.19.1

^ permalink raw reply related

* [PATCH AUTOSEL 4.14 081/170] Input: rotary-encoder - don't log EPROBE_DEFER to kernel log
From: Sasha Levin @ 2019-01-28 16:10 UTC (permalink / raw)
  To: linux-kernel, stable
  Cc: Uwe Kleine-König, Dmitry Torokhov, Sasha Levin, linux-input
In-Reply-To: <20190128161200.55107-1-sashal@kernel.org>

From: Uwe Kleine-König <uwe@kleine-koenig.org>

[ Upstream commit 0832e93632c61987d504e251b927a2be769dd21a ]

When a driver fails to bind because a resource it still missing it's not
helpful to report this as (usually) probing is repeated later.

Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 drivers/input/misc/rotary_encoder.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/input/misc/rotary_encoder.c b/drivers/input/misc/rotary_encoder.c
index 1588aecafff7..72eee6d55527 100644
--- a/drivers/input/misc/rotary_encoder.c
+++ b/drivers/input/misc/rotary_encoder.c
@@ -240,8 +240,10 @@ static int rotary_encoder_probe(struct platform_device *pdev)
 
 	encoder->gpios = devm_gpiod_get_array(dev, NULL, GPIOD_IN);
 	if (IS_ERR(encoder->gpios)) {
-		dev_err(dev, "unable to get gpios\n");
-		return PTR_ERR(encoder->gpios);
+		err = PTR_ERR(encoder->gpios);
+		if (err != -EPROBE_DEFER)
+			dev_err(dev, "unable to get gpios: %d\n", err);
+		return err;
 	}
 	if (encoder->gpios->ndescs < 2) {
 		dev_err(dev, "not enough gpios found\n");
-- 
2.19.1

^ permalink raw reply related


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox