Re: [PATCH v4 2/2] Input: add MStar MSG2638 touchscreen driver

2021-02-22 Thread Vincent Knecht
Le samedi 20 février 2021 à 15:23 -0800, Dmitry Torokhov a écrit :
> Hi Vincent,

Hi Dmitry, thank you for the review !

> On Wed, Feb 10, 2021 at 06:33:52PM +0100, Vincent Knecht wrote:
> > +
> > +   for (i = 0; i < MAX_SUPPORTED_FINGER_NUM; i++) {
> > +   p = _event.pkt[i];
> > +   /* Ignore non-pressed finger data */
> > +   if (p->xy_hi == 0xFF && p->x_low == 0xFF && p->y_low == 
> > 0xFF)
> > +   continue;
> > +
> > +   coord.x = (((p->xy_hi & 0xF0) << 4) | p->x_low) * 
> > msg2638->prop.max_x / TPD_WIDTH;
> > +   coord.y = (((p->xy_hi & 0x0F) << 8) | p->y_low) * 
> > msg2638->prop.max_y / TPD_HEIGHT;
> > +   msg2638_report_finger(msg2638, i, );
> 
> We do not scale the coordinates in the kernel. Rather we provide
> resolution, if known, and min/max coordinates reported by the hardware,
> and let userspace handle the rest.

Ok, will remove scaling... I was able to test it's ok by setting 
touchscreen-size-{x,y} = <2048>;

> > +static int __maybe_unused msg2638_suspend(struct device *dev)
> > +{
> > +   struct i2c_client *client = to_i2c_client(dev);
> > +   struct msg2638_ts_data *msg2638 = i2c_get_clientdata(client);
> > +
> > +   mutex_lock(>input_dev->mutex);
> > +
> > +   if (input_device_enabled(msg2638->input_dev))
> > +   msg2638_stop(msg2638);
> 
> I believe that you should power down the device only if it is not
> configures as wakeup source. In fact (and I think most drivers are
> wrong in this), you may want to power up the device if it is a wakeup
> source and it does not have any users.

I don't know much on this subject ; from downstream code, it seems
the touchscreen supports "wakeup gestures" (like "double click",
up/down/left/right directions and some characters/letters) and apparently
an irq-gpio which would be used to wakeup.
I don't handle that currently, is it required ?







Re: [PATCH v4 2/2] Input: add MStar MSG2638 touchscreen driver

2021-02-20 Thread Dmitry Torokhov
Hi Vincent,

On Wed, Feb 10, 2021 at 06:33:52PM +0100, Vincent Knecht wrote:
> +
> + for (i = 0; i < MAX_SUPPORTED_FINGER_NUM; i++) {
> + p = _event.pkt[i];
> + /* Ignore non-pressed finger data */
> + if (p->xy_hi == 0xFF && p->x_low == 0xFF && p->y_low == 0xFF)
> + continue;
> +
> + coord.x = (((p->xy_hi & 0xF0) << 4) | p->x_low) * 
> msg2638->prop.max_x / TPD_WIDTH;
> + coord.y = (((p->xy_hi & 0x0F) << 8) | p->y_low) * 
> msg2638->prop.max_y / TPD_HEIGHT;
> + msg2638_report_finger(msg2638, i, );

We do not scale the coordinates in the kernel. Rather we provide
resolution, if known, and min/max coordinates reported by the hardware,
and let userspace handle the rest.

> +static int __maybe_unused msg2638_suspend(struct device *dev)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + struct msg2638_ts_data *msg2638 = i2c_get_clientdata(client);
> +
> + mutex_lock(>input_dev->mutex);
> +
> + if (input_device_enabled(msg2638->input_dev))
> + msg2638_stop(msg2638);

I believe that you should power down the device only if it is not
configures as wakeup source. In fact (and I think most drivers are
wrong in this), you may want to power up the device if it is a wakeup
source and it does not have any users.

Thanks.

-- 
Dmitry


[PATCH v4 2/2] Input: add MStar MSG2638 touchscreen driver

2021-02-10 Thread Vincent Knecht
Add support for the msg2638 touchscreen IC from MStar.
This driver reuses zinitix.c structure, while the checksum and irq handler
functions are based on out-of-tree driver for Alcatel Idol 3 (4.7").

Signed-off-by: Vincent Knecht 
---
Changed in v4:
- rename from msg26xx to msg2638, following compatible string change
- rename mstar_* functions to msg2638_* for consistency
- add RESET_DELAY define and use it in msg2638_power_on()
- change a few dev_err() calls to be on one line only
- add missing \n in a few dev_err() strings
Changed in v3:
- no change
Changed in v2:
- don't use bitfields in packet struct, to prevent endian-ness related problems 
(Dmitry)
- fix reset gpiod calls order in mstar_power_on() (Dmitry)
---
 drivers/input/touchscreen/Kconfig   |  12 +
 drivers/input/touchscreen/Makefile  |   1 +
 drivers/input/touchscreen/msg2638.c | 362 
 3 files changed, 375 insertions(+)
 create mode 100644 drivers/input/touchscreen/msg2638.c

diff --git a/drivers/input/touchscreen/Kconfig 
b/drivers/input/touchscreen/Kconfig
index f012fe746df0..fefbe1c1bb10 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -1334,4 +1334,16 @@ config TOUCHSCREEN_ZINITIX
  To compile this driver as a module, choose M here: the
  module will be called zinitix.
 
+config TOUCHSCREEN_MSG2638
+   tristate "MStar msg2638 touchscreen support"
+   depends on I2C
+   depends on GPIOLIB || COMPILE_TEST
+   help
+ Say Y here if you have an I2C touchscreen using MStar msg2638.
+
+ If unsure, say N.
+
+ To compile this driver as a module, choose M here: the
+ module will be called msg2638.
+
 endif
diff --git a/drivers/input/touchscreen/Makefile 
b/drivers/input/touchscreen/Makefile
index 6233541e9173..83e516cb3d33 100644
--- a/drivers/input/touchscreen/Makefile
+++ b/drivers/input/touchscreen/Makefile
@@ -112,3 +112,4 @@ obj-$(CONFIG_TOUCHSCREEN_ROHM_BU21023)  += 
rohm_bu21023.o
 obj-$(CONFIG_TOUCHSCREEN_RASPBERRYPI_FW)   += raspberrypi-ts.o
 obj-$(CONFIG_TOUCHSCREEN_IQS5XX)   += iqs5xx.o
 obj-$(CONFIG_TOUCHSCREEN_ZINITIX)  += zinitix.o
+obj-$(CONFIG_TOUCHSCREEN_MSG2638)  += msg2638.o
diff --git a/drivers/input/touchscreen/msg2638.c 
b/drivers/input/touchscreen/msg2638.c
new file mode 100644
index ..e00cff923fe2
--- /dev/null
+++ b/drivers/input/touchscreen/msg2638.c
@@ -0,0 +1,362 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Driver for MStar msg2638 touchscreens
+ *
+ * Copyright (c) 2021 Vincent Knecht 
+ *
+ * Checksum and IRQ handler based on mstar_drv_common.c and 
mstar_drv_mutual_fw_control.c
+ * Copyright (c) 2006-2012 MStar Semiconductor, Inc.
+ *
+ * Driver structure based on zinitix.c by Michael Srba 
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define MODE_DATA_RAW  0x5A
+
+#define TPD_WIDTH  2048
+#define TPD_HEIGHT 2048
+
+#define MAX_SUPPORTED_FINGER_NUM   5
+
+#define CHIP_ON_DELAY  15 // ms
+#define FIRMWARE_ON_DELAY  50 // ms
+#define RESET_DELAY10 // ms
+
+struct point_coord {
+   u16 x;
+   u16 y;
+};
+
+struct packet {
+   u8  xy_hi; /* higher bits of x and y coordinates */
+   u8  x_low;
+   u8  y_low;
+   u8  pressure;
+};
+
+struct touch_event {
+   u8  mode;
+   struct  packet pkt[MAX_SUPPORTED_FINGER_NUM];
+   u8  proximity;
+   u8  checksum;
+};
+
+struct msg2638_ts_data {
+   struct i2c_client *client;
+   struct input_dev *input_dev;
+   struct touchscreen_properties prop;
+   struct regulator_bulk_data supplies[2];
+   struct gpio_desc *reset_gpiod;
+};
+
+static int msg2638_init_regulators(struct msg2638_ts_data *msg2638)
+{
+   struct i2c_client *client = msg2638->client;
+   int error;
+
+   msg2638->supplies[0].supply = "vdd";
+   msg2638->supplies[1].supply = "vddio";
+   error = devm_regulator_bulk_get(>dev,
+   ARRAY_SIZE(msg2638->supplies),
+   msg2638->supplies);
+   if (error < 0) {
+   dev_err(>dev, "Failed to get regulators: %d\n", error);
+   return error;
+   }
+
+   return 0;
+}
+
+static void msg2638_power_on(struct msg2638_ts_data *msg2638)
+{
+   gpiod_set_value(msg2638->reset_gpiod, 1);
+   mdelay(RESET_DELAY);
+   gpiod_set_value(msg2638->reset_gpiod, 0);
+   mdelay(FIRMWARE_ON_DELAY);
+
+   enable_irq(msg2638->client->irq);
+}
+
+static void msg2638_report_finger(struct msg2638_ts_data *msg2638, int slot,
+   const struct point_coord *pc)
+{
+   input_mt_slot(msg2638->input_dev, slot);
+