| Message ID | 20210210173403.667482-1-vincent.knecht@mailoo.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v4,1/2] dt-bindings: input/touchscreen: add bindings for msg2638 | expand |
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. -- Dmitry
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 ?
diff --git a/Documentation/devicetree/bindings/input/touchscreen/mstar,msg2638.yaml b/Documentation/devicetree/bindings/input/touchscreen/mstar,msg2638.yaml new file mode 100644 index 000000000000..12f44d9e4d41 --- /dev/null +++ b/Documentation/devicetree/bindings/input/touchscreen/mstar,msg2638.yaml @@ -0,0 +1,69 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/input/touchscreen/mstar,msg2638.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: MStar msg2638 touchscreen controller Bindings + +maintainers: + - Vincent Knecht <vincent.knecht@mailoo.org> + +allOf: + - $ref: touchscreen.yaml# + +properties: + compatible: + const: mstar,msg2638 + + reg: + const: 0x26 + + interrupts: + maxItems: 1 + + reset-gpios: + maxItems: 1 + + vdd-supply: + description: Power supply regulator for the chip + + vddio-supply: + description: Power supply regulator for the I2C bus + + touchscreen-size-x: true + touchscreen-size-y: true + +additionalProperties: false + +required: + - compatible + - reg + - interrupts + - reset-gpios + - touchscreen-size-x + - touchscreen-size-y + +examples: + - | + #include <dt-bindings/gpio/gpio.h> + #include <dt-bindings/interrupt-controller/irq.h> + i2c { + #address-cells = <1>; + #size-cells = <0>; + touchscreen@26 { + compatible = "mstar,msg2638"; + reg = <0x26>; + interrupt-parent = <&msmgpio>; + interrupts = <13 IRQ_TYPE_EDGE_FALLING>; + reset-gpios = <&msmgpio 100 GPIO_ACTIVE_LOW>; + pinctrl-names = "default"; + pinctrl-0 = <&ts_int_reset_default>; + vdd-supply = <&pm8916_l17>; + vddio-supply = <&pm8916_l5>; + touchscreen-size-x = <720>; + touchscreen-size-y = <1280>; + }; + }; + +...
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