[v4,1/2] dt-bindings: input/touchscreen: add bindings for msg2638

This adds dts bindings for the mstar msg2638 touchscreen.

Signed-off-by: Vincent Knecht <vincent.knecht@mailoo.org>
---
Changed in v4:
- don't use wildcards in compatible strings (Rob H)
- rename from msg26xx to msg2638
- rename example pinctrl-0 to &ts_int_reset_default for consistency
Changed in v3:
- added `touchscreen-size-x: true` and `touchscreen-size-y: true` properties
Changed in v2:
- changed M-Star to MStar in title line
- changed reset gpio to active-low in example section
---
 .../input/touchscreen/mstar,msg2638.yaml      | 69 +++++++++++++++++++
 1 file changed, 69 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/input/touchscreen/mstar,msg2638.yaml

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 = &touch_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, &coord);

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(&msg2638->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.

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 = &touch_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, &coord);
> 
> 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(&msg2638->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 ?