* [PATCH 0/4] Add haptics support to Nexus 5 using pwm-vibra driver
From: Luca Weiss @ 2023-04-27 20:34 UTC (permalink / raw)
To: ~postmarketos/upstreaming, phone-devel, Dmitry Torokhov,
Rob Herring, Krzysztof Kozlowski, Sebastian Reichel, Andy Gross,
Bjorn Andersson, Konrad Dybcio, Brian Masney
Cc: linux-input, devicetree, linux-kernel, linux-arm-msm, Luca Weiss
A while ago Brian Masney sent some patches for a clk-vibrator which was
then succeeded by the idea of a clk-pwm driver that "converts" a clock
into a PWM and to use the existing pwm-vibra driver.
Since clk-pwm has landed last year we can finally add haptics support
upstream.
We just need to add support for an enable GPIO to the pwm-vibra driver
since that also needs to be high for the haptics to work on this device.
Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
---
Luca Weiss (4):
dt-bindings: input: pwm-vibrator: Add enable-gpio
Input: pwm-vibra - add newline to dev_err prints
Input: pwm-vibra - add support for enable GPIO
ARM: dts: qcom: msm8974-hammerhead: Add vibrator
.../devicetree/bindings/input/pwm-vibrator.yaml | 2 ++
.../dts/qcom-msm8974-lge-nexus5-hammerhead.dts | 35 +++++++++++++++++++++
drivers/input/misc/pwm-vibra.c | 36 ++++++++++++++++------
3 files changed, 63 insertions(+), 10 deletions(-)
---
base-commit: dec7f67a13c3270f9a38eba227a4fc15993f01b3
change-id: 20230427-hammerhead-vibra-06bd1bf771a3
Best regards,
--
Luca Weiss <luca@z3ntu.xyz>
^ permalink raw reply
* [PATCH 4/4] ARM: dts: qcom: msm8974-hammerhead: Add vibrator
From: Luca Weiss @ 2023-04-27 20:34 UTC (permalink / raw)
To: ~postmarketos/upstreaming, phone-devel, Dmitry Torokhov,
Rob Herring, Krzysztof Kozlowski, Sebastian Reichel, Andy Gross,
Bjorn Andersson, Konrad Dybcio, Brian Masney
Cc: linux-input, devicetree, linux-kernel, linux-arm-msm, Luca Weiss
In-Reply-To: <20230427-hammerhead-vibra-v1-0-e87eeb94da51@z3ntu.xyz>
The Nexus 5 has a vibrator connected to the clock output of GP1_CLK
which we can use with the clk-pwm driver, then we can use that pwm with
pwm-vibrator to get haptics functionality.
This patch is based on Brian Masney's previous patch with clk-vibrator.
Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
---
.../dts/qcom-msm8974-lge-nexus5-hammerhead.dts | 35 ++++++++++++++++++++++
1 file changed, 35 insertions(+)
diff --git a/arch/arm/boot/dts/qcom-msm8974-lge-nexus5-hammerhead.dts b/arch/arm/boot/dts/qcom-msm8974-lge-nexus5-hammerhead.dts
index ab35f2d644c0..fea8a6be9021 100644
--- a/arch/arm/boot/dts/qcom-msm8974-lge-nexus5-hammerhead.dts
+++ b/arch/arm/boot/dts/qcom-msm8974-lge-nexus5-hammerhead.dts
@@ -41,6 +41,25 @@ key-volume-down {
};
};
+ clk_pwm: pwm {
+ compatible = "clk-pwm";
+ clocks = <&mmcc CAMSS_GP1_CLK>;
+
+ pinctrl-0 = <&vibrator_pin>;
+ pinctrl-names = "default";
+
+ #pwm-cells = <2>;
+ };
+
+ vibrator {
+ compatible = "pwm-vibrator";
+ pwms = <&clk_pwm 0 100000>;
+ pwm-names = "enable";
+
+ vcc-supply = <&pm8941_l19>;
+ enable-gpios = <&tlmm 60 GPIO_ACTIVE_HIGH>;
+ };
+
vreg_wlan: wlan-regulator {
compatible = "regulator-fixed";
@@ -637,6 +656,22 @@ shutdown-pins {
function = "gpio";
};
};
+
+ vibrator_pin: vibrator-state {
+ core-pins {
+ pins = "gpio27";
+ function = "gp1_clk";
+ drive-strength = <6>;
+ bias-disable;
+ };
+
+ enable-pins {
+ pins = "gpio60";
+ function = "gpio";
+ drive-strength = <2>;
+ bias-disable;
+ };
+ };
};
&usb {
--
2.40.0
^ permalink raw reply related
* [PATCH 1/4] dt-bindings: input: pwm-vibrator: Add enable-gpio
From: Luca Weiss @ 2023-04-27 20:34 UTC (permalink / raw)
To: ~postmarketos/upstreaming, phone-devel, Dmitry Torokhov,
Rob Herring, Krzysztof Kozlowski, Sebastian Reichel, Andy Gross,
Bjorn Andersson, Konrad Dybcio, Brian Masney
Cc: linux-input, devicetree, linux-kernel, linux-arm-msm, Luca Weiss
In-Reply-To: <20230427-hammerhead-vibra-v1-0-e87eeb94da51@z3ntu.xyz>
Some pwm vibrators have a dedicated enable GPIO that needs to be set
high so that the vibrator works. Document that.
Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
---
Documentation/devicetree/bindings/input/pwm-vibrator.yaml | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/input/pwm-vibrator.yaml b/Documentation/devicetree/bindings/input/pwm-vibrator.yaml
index d32716c604fe..6398534b43c3 100644
--- a/Documentation/devicetree/bindings/input/pwm-vibrator.yaml
+++ b/Documentation/devicetree/bindings/input/pwm-vibrator.yaml
@@ -32,6 +32,8 @@ properties:
minItems: 1
maxItems: 2
+ enable-gpios: true
+
vcc-supply: true
direction-duty-cycle-ns:
--
2.40.0
^ permalink raw reply related
* Re: [PATCH] Fix freeze in lm8333 i2c keyboard driver
From: Jeff LaBundy @ 2023-04-27 18:54 UTC (permalink / raw)
To: Tomáš Mudruňka; +Cc: Dmitry Torokhov, linux-input, linux-kernel
In-Reply-To: <CAH2-hcJa_vL9iWTARUAD+adrvQAjzr1N4bQ=cN+8kbE0arVwZw@mail.gmail.com>
Hi Tomas,
On Thu, Apr 27, 2023 at 10:19:38AM +0200, Tomáš Mudruňka wrote:
> > Yes that's correct; what I mean to say is that depending on the nature of
> > the read-to-clear mechanism in the part, there is a chance that the IRQ
> > has not been deasserted by the time the threaded handler returns. On some
> > devices for example, the IRQ is not deasserted until some time after the
> > read's stop condition.
> >
> > For these cases, I consider it best practice to measure the I2C and IRQ
> > lines on a scope and if necessary, add a small delay before the interrupt
> > handler returns. This is especially true for open-drain interrupts that
> > may need a few hundred extra us for the pin to rise.
>
> Well before posting the patch i did some testing.
> I was watching the /proc/interrupts and checked that IRQ counter for
> lm8333 matches number of keypresses.
> Which i've only tested for like 20-30 times, but haven't seem any glitch.
>
> But i still recognize the fact that the gpio line getting stuck for
> some reason (short circuit on PCB?) might cause troubles by
> unnecessarily loading the CPU, while with edge trigger it's more
> likely to affect only the function of keyboard itself rather than
> bringing down whole system. But i am not sure if this case is supposed
> to be expected and handled in SW.
In the case of short circuit, the hardware has failed and there is nothing
we can do.
My point is that different devices deassert their IRQ at different points
in the read-to-clear operation. For some devices, the IRQ is deasserted
immediately after the register address is latched, so we can be confident
that the IRQ has already gone high by the time the I2C read operation and
hence the interrupt handler return.
On others however, the IRQ may still remain low for 10's or even 100's of
us after the read is complete. In some cases, the threaded handler could
have already returned by then. Since you did not find any unexplained IRQ
counts, perhaps that is not the case for this device.
Kind regards,
Jeff LaBundy
^ permalink raw reply
* Re: [PATCH v2] Fix freeze in lm8333 i2c keyboard driver
From: Jeff LaBundy @ 2023-04-27 18:47 UTC (permalink / raw)
To: Tomáš Mudruňka; +Cc: dmitry.torokhov, linux-input, linux-kernel
In-Reply-To: <CAH2-hcLOA0ECg7ubKh_r9BryRuGPjJ1Cv1ErUpMmQ+Yag5qnWA@mail.gmail.com>
Hi Tomas,
On Thu, Apr 27, 2023 at 10:13:22AM +0200, Tomáš Mudruňka wrote:
> Hello, thanks for your notes.
>
> > This is the right idea. I am sort of splitting hairs here, however I
> > think it makes sense to place this read before the IRQ is requested
> > and not after.
> >
> >
> > As written, there is room for an ever-so-tiny race condition wherein
> > the IRQ is asserted just after it is requested. Before the threaded
> > handler has run however, the new read in probe swallows the IRQ status
> > before the threaded handler can read it and react to errors.
>
> In fact i believe quite the opposite case to be true.
> If i read before registering IRQ there will be ever-so-tiny race condition that
> would allow to miss the edge (exactly the bug this patch is fixing,
> but limited).
I thought the original problem is that the IRQ is already low by the time
the driver loads. Since a high-to-low transition (i.e. falling edge) is
never witnessed, the handler is never called to read the status and allow
the IRQ to go high again. Therefore, key events are gone forever.
The concern you mention is simply that of not responding to key events
until the interrupt handler is registered; there is no way around that.
Any event that occurs before then is off the table. Instead, we can only
make sure that none of those prior events place us in a bad state.
>
> In the case you describe the worst scenario is likely that the interrupt handler
> will be called only to re-read status and immediately return on this condition:
>
> if (!status) return IRQ_NONE;
Right, I am simply saying this is one key press that could have been
preserved. As a matter of principle, once the interrupt handler is live,
you should not disturb the precious read-on-clear registers on your own
without concurrency protection. It is much more common to clear suprious
interrupts and _then_ make the handler go live.
>
> > Also, I think you should at least capture and evaluate lm8333_read8()'s
> > return value as is already done for the calls to lm8333_write8().
>
> Well. If you think this will bring any benefits, i might as well just call
> lm8333_irq_thread() instead of lm8333_read8()
> Would that be acceptable solution?
Looking at the datasheet, it seems this devices builds up scan codes in
a FIFO. To protect against the rare case in which this dummy read includes
actual data, perhaps it is better to call lm8333_irq_thread() instead of
lm8333_read8() so that the FIFO is flushed.
>
> Tom.
Kind regards,
Jeff LaBundy
^ permalink raw reply
* Re: [PATCH] HID: wacom: Added error check
From: Ping Cheng @ 2023-04-27 17:56 UTC (permalink / raw)
To: Denis Arefev
Cc: Ping Cheng, Jason Gerecke, Jiri Kosina, Benjamin Tissoires,
linux-input, linux-kernel, lvc-project, trufanov, vfh
In-Reply-To: <20230427114745.53136-1-arefev@swemel.ru>
On Thu, Apr 27, 2023 at 5:07 AM Denis Arefev <arefev@swemel.ru> wrote:
>
> Added a variable check and
> transition in case of an error
>
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>
> Signed-off-by: Denis Arefev <arefev@swemel.ru>
Thank you Denis for working on the patch! The patch LGTM.
Reviewed-by: Ping Cheng <ping.cheng@wacom.com>
Cheers,
Ping
> ---
> drivers/hid/wacom_sys.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> index fb538a6c4add..aff4a21a46b6 100644
> --- a/drivers/hid/wacom_sys.c
> +++ b/drivers/hid/wacom_sys.c
> @@ -2417,8 +2417,13 @@ static int wacom_parse_and_register(struct wacom *wacom, bool wireless)
> goto fail_quirks;
> }
>
> - if (features->device_type & WACOM_DEVICETYPE_WL_MONITOR)
> + if (features->device_type & WACOM_DEVICETYPE_WL_MONITOR) {
> error = hid_hw_open(hdev);
> + if (error) {
> + hid_err(hdev, "hw open failed\n");
> + goto fail_quirks;
> + }
> + }
>
> wacom_set_shared_values(wacom_wac);
> devres_close_group(&hdev->dev, wacom);
> --
> 2.25.1
>
^ permalink raw reply
* Re: [RFC v1 0/4] Input: support virtual objects on touchscreens
From: Jeff LaBundy @ 2023-04-27 17:45 UTC (permalink / raw)
To: Pavel Machek
Cc: Michael Riesch, Javier Carrasco, phone-devel, linux-input,
devicetree, linux-kernel, Dmitry Torokhov, Rob Herring,
Krzysztof Kozlowski, Henrik Rydberg, Ulf Hansson, Hans Verkuil,
Stephen Boyd, Sebastian Reichel, Linus Walleij, Jonathan Cameron,
Uwe Kleine-g, Bastian Hecht
In-Reply-To: <ZEp1Z08/CQ+waw8+@duo.ucw.cz>
Hi Pavel,
On Thu, Apr 27, 2023 at 03:15:19PM +0200, Pavel Machek wrote:
> Hi!
>
> > >
> > >> Some touchscreens are shipped with a physical layer on top of them where
> > >> a number of buttons and a resized touchscreen surface might be
> > >> available.
> > >
> > > Yes, it is quite comon, for example Motorola Droid 4 has 4 virtual
> > > buttons below touchscreen.
> >
> > Are those buttons configurable in some way? Or do they have a fixed purpose?
>
> Fixed.
>
> > How does Android handle those buttons, BTW?
>
> No idea.
>
> > > One question is if this should be handled inside the kernel. It will
> > > make it compatible with existing software, but it will also reduce
> > > flexibility.
That's a great question; I think there are arguments for both.
On one hand, we generally want the kernel to be responsible for nothing more
than handing off the raw coordinate and touch information to user space. Any
further translation of that represents policy which would not belong here.
On the other hand, the notion of what buttons exist and where is very much a
hardware statement for the use-case targeted by this RFC. It would be ideal
if both the kernel and user space did not need to know information about the
same piece of hardware. So I think it is OK for the driver to give some help
by doing some of its own interpretation, much like some hardware-accelerated
solutions already do.
While there are obviously exceptions in either case, I don't see any reason
to prohibit having a simple option like this in the kernel, especially since
it doesn't preclude having something in user space for more advanced cases.
> >
> > I would say that it should be described in device tree if the purpose is
> > fixed. For example, if there is no display behind the touch screen at a
> > certain point but a printed sheet (e.g., with a home or return symbol)
> > then it is clear that this button is not going to change. In such a case
> > I doubt that flexibility is required.
>
> I agree it should be in the device tree.
>
> AFAICT hardware can do drags between the buttons, and drag between the
> buttons and touchscreen. Turning it into buttons prevents that.
>
> Plus, real buttons can do simultaneous presses on all of them,
> touchscreens will have problems with that.
I interpreted the RFC and its example to accommodate multitouch support, so
I don't see any problem here unless the vendor built such a module without
a multitouch panel which would not make sense. Let me know in case I have
misunderstood the concern.
>
> Best regards,
> Pavel
> --
> People of Russia, stop Putin before his war on Ukraine escalates.
Kind regards,
Jeff LaBundy
^ permalink raw reply
* Re: [PATCH v4 1/2] dt-bindings: input: goodix: Add "goodix,no-reset-during-suspend" property
From: Rob Herring @ 2023-04-27 17:35 UTC (permalink / raw)
To: Fei Shao
Cc: Rob Herring, Benjamin Tissoires, Dmitry Torokhov,
Krzysztof Kozlowski, Matthias Brugger, linux-input, devicetree,
Douglas Anderson, Jeff LaBundy, linux-kernel, linux-mediatek
In-Reply-To: <20230427035656.1962698-2-fshao@chromium.org>
On Thu, 27 Apr 2023 11:56:55 +0800, Fei Shao wrote:
> We observed that on Chromebook device Steelix, if Goodix GT7375P
> touchscreen is powered in suspend (because, for example, it connects to
> an always-on regulator) and with the reset GPIO asserted, it will
> introduce about 14mW power leakage.
>
> To address that, we add this property to skip reset during suspend.
> If it's set, the driver will stop asserting the reset GPIO during
> power-down. Refer to the comments in the driver for details.
>
> Signed-off-by: Fei Shao <fshao@chromium.org>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>
> Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>
> Reviewed-by: Jeff LaBundy <jeff@labundy.com>
> ---
>
> (no changes since v2)
>
> Changes in v2:
> - Use a more accurate property name and with "goodix," prefix.
>
> .../devicetree/bindings/input/goodix,gt7375p.yaml | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
Acked-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* Re: [RFC v1 0/4] Input: support virtual objects on touchscreens
From: Jeff LaBundy @ 2023-04-27 17:23 UTC (permalink / raw)
To: Javier Carrasco
Cc: Thomas Weißschuh, linux-input, devicetree, linux-kernel,
Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Henrik Rydberg,
Ulf Hansson, Hans Verkuil, Stephen Boyd, Sebastian Reichel,
Linus Walleij, Jonathan Cameron, Uwe Kleine-g, Bastian Hecht,
Michael Riesch
In-Reply-To: <aa6125d9-1233-7aab-1811-29acd4ad49a5@wolfvision.net>
Hi Javier,
On Thu, Apr 27, 2023 at 05:59:42PM +0200, Javier Carrasco wrote:
> Hi,
>
> On 25.04.23 18:02, Jeff LaBundy wrote:
> > Hi Thomas,
> >
> > On Tue, Apr 25, 2023 at 05:29:39PM +0200, Thomas Weißschuh wrote:
> >> Hi Javier,
> >>
> >> On 2023-04-25 13:50:45+0200, Javier Carrasco wrote:
> >>> Some touchscreens are shipped with a physical layer on top of them where
> >>> a number of buttons and a resized touchscreen surface might be available.
> >>>
> >>> In order to generate proper key events by overlay buttons and adjust the
> >>> touch events to a clipped surface, these patches offer a documented,
> >>> device-tree-based solution by means of helper functions.
> >>> An implementation for a specific touchscreen driver is also included.
> >>>
> >>> The functions in ts-virtobj provide a simple workflow to acquire
> >>> physical objects from the device tree, map them into the device driver
> >>> structures as virtual objects and generate events according to
> >>> the object descriptions.
> >>>
> >>> This solution has been tested with a JT240MHQS-E3 display, which uses
> >>> the st1624 as a touchscreen and provides two overly buttons and a frame
> >>> that clips its effective surface.
> >>
> >> There are quite a few of notebooks from Asus that feature a printed
> >> numpad on their touchpad [0]. The mapping from the touch events to the
> >> numpad events needs to happen in software.
> >
> > That example seems a kind of fringe use-case in my opinion; I think the
> > gap filled by this RFC is the case where a touchscreen has a printed
> > overlay with a key that represents a fixed function.
>
> Exactly, this RFC addresses exactly such printed overlays.
> >
> > One problem I do see here is something like libinput or multitouch taking
> > hold of the input device, and swallowing the key presses because it sees
> > the device as a touchscreen and is not interested in these keys.
>
> Unfortunately I do not know libinput or multitouch and I might be
> getting you wrong, but I guess the same would apply to any event
> consumer that takes touchscreens as touch event producers and nothing else.
>
> Should they not check the supported events from the device instead of
> making such assumptions? This RFC adds key events defined in the device
> tree and they are therefore available and published as device
> capabilities. That is for example what evtest does to report the
> supported events and they are then notified accordingly. Is that not the
> right way to do it?
evtest is just that, a test tool. It's handy for ensuring the device emits
the appropriate input events in response to hardware inputs, but it is not
necessarily representative of how the input device may be used in practice.
I would encourage you to test this solution with a simple use-case such as
Raspbian, and the virtual keys mapped to easily recognizable functions like
volume up/down.
Here, you will find that libinput will grab the device and declare it to be
a touchscreen based on the input events it advertises. However, you will not
see volume up/down keys are handled.
If you break out the virtual keypad as a separate input device, however, you
will see libinput additionally recognize it as a keyboard and volume up/down
keys will be handled. It is for this reason that a handful of drivers with
this kind of mixed functionality (e.g. ad714x) already branch out multiple
input devices for each function.
As a matter of principle, I find it to be most flexible for logically separate
functions to be represented as logically separate input devices, even if those
input devices all stem from the same piece of hardware. Not only does it allow
you to attach different handlers to each device (i.e. file descriptor), but it
also allows user space to inhibit one device but not the other, etc.
Maybe the right approach, which your RFC already seems to support, is to simply
let the driver decide whether to pass the touchscreen input_dev or a different
input_dev. The driver would be responsible for allocating and registering the
keypad; your functions simply set the capabilities for, and report events from,
whichever input_dev is passed to them. This is something to consider for your
st1232 example.
>
> Thanks a lot for your feedback!
> >
> > Therefore, my first impression is that the virtual keypad may be better
> > served by registering its own input device.
> >
> > Great work by the way, Javier!
> >
> >>
> >> Do you think your solution is general enough to also support this
> >> usecase?
> >>
> >> The differences I see are
> >> * not device-tree based
> >> * touchpads instead of touchscreens
> >>
> >>> [..]
> >>
> >> [0] https://unix.stackexchange.com/q/494400
> >
> > Kind regards,
> > Jeff LaBundy
Kind regards,
Jeff LaBundy
^ permalink raw reply
* Re: [PATCH v2 2/2] dt-bindings: MFD: Convert STMPE to YAML schema
From: Rob Herring @ 2023-04-27 16:16 UTC (permalink / raw)
To: Linus Walleij
Cc: Bartosz Golaszewski, Krzysztof Kozlowski, Maxime Coquelin,
Alexandre Torgue, Dmitry Torokhov, Lee Jones, Philippe Schenker,
Stefan Agner, Marek Vasut, Steffen Trumtrar, linux-gpio,
devicetree, linux-stm32, linux-arm-kernel, linux-kernel,
linux-input
In-Reply-To: <20230426-stmpe-dt-bindings-v2-2-2f85a1fffcda@linaro.org>
On Wed, Apr 26, 2023 at 01:21:39PM +0200, Linus Walleij wrote:
> This converts the STMPE MFD device tree bindings to the YAML
> schema.
>
> Reference the existing schema for the ADC, just define the
> other subnode schemas directly in the MFD schema.
>
> Add two examples so we have examples covering both the simple
> GPIO expander and the more complex with ADC and touchscreen.
>
> Some in-tree users do not follow the naming conventions for nodes
> so these DTS files need to be augmented to use proper node names
> like "adc", "pwm", "gpio", "keyboard-controller" etc before the
> bindings take effect on them.
>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v1->v2:
> - Split off the GPIO bindings to their own schema, as the old
> bindings didn't even have any GPIO bindings. Put the GPIO
> schema before this schema so we can use GPIO in the examples.
> - Drop nodename and pattern as STMPE is not a generic name.
> - Add maxItems to the resets.
> - Make wakeup-source just :true, as it is a generic property.
> - Move unevaluatedProperties for subnodes right before properties
> as requested.
> - Name devices "port-expander" in the examples.
> - Use lowercase hex in line init.
> ---
> .../devicetree/bindings/input/stmpe-keypad.txt | 41 ---
> .../bindings/input/touchscreen/stmpe.txt | 108 --------
> .../devicetree/bindings/mfd/st,stmpe.yaml | 298 +++++++++++++++++++++
> Documentation/devicetree/bindings/mfd/stmpe.txt | 42 ---
> 4 files changed, 298 insertions(+), 191 deletions(-)
> diff --git a/Documentation/devicetree/bindings/mfd/st,stmpe.yaml b/Documentation/devicetree/bindings/mfd/st,stmpe.yaml
> new file mode 100644
> index 000000000000..dd24ae2d5fb4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/st,stmpe.yaml
> @@ -0,0 +1,298 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mfd/st,stmpe.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: STMicroelectonics Port Expander (STMPE)
> +
> +description: STMicroelectronics Port Expander (STMPE) is a series of slow
> + bus controllers for various expanded peripherals such as GPIO, keypad,
> + touchscreen, ADC, PWM or rotator. It can contain one or several different
> + peripherals connected to SPI or I2C.
> +
> +maintainers:
> + - Linus Walleij <linus.walleij@linaro.org>
> +
> +allOf:
> + - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +properties:
> + compatible:
> + enum:
> + - st,stmpe601
> + - st,stmpe801
> + - st,stmpe811
> + - st,stmpe1600
> + - st,stmpe1601
> + - st,stmpe2401
> + - st,stmpe2403
> +
> + reg:
> + maxItems: 1
> +
> + interrupts:
> + maxItems: 1
> +
> + vcc-supply: true
> +
> + vio-supply: true
> +
> + reset-gpios:
> + maxItems: 1
> +
> + wakeup-source: true
> +
> + st,autosleep-timeout:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + enum: [ 4, 16, 32, 64, 128, 256, 512, 1024 ]
> + description: Time idle before going to automatic sleep to save power
> +
> + st,sample-time:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + enum: [ 0, 1, 2, 3, 4, 5, 6 ]
> + description: |
> + Sample time per iteration
> + 0 = 36 clock ticks
> + 1 = 44 clock ticks
> + 2 = 56 clock ticks
> + 3 = 64 clock ticks
> + 4 = 80 clock ticks - recommended
> + 5 = 96 clock ticks
> + 6 = 124 clock ticks
> +
> + st,mod-12b:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + enum: [ 0, 1 ]
> + description: ADC bit mode 0 = 10bit ADC, 1 = 12bit ADC
> +
> + st,ref-sel:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + enum: [ 0, 1 ]
> + description: ADC reference source 0 = internal, 1 = external
> +
> + st,adc-freq:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + enum: [ 0, 1, 2, 3 ]
> + description: |
> + ADC clock speed
> + 0 = 1.625 MHz
> + 1 = 3.25 MHz
> + 2, 3 = 6.5 MHz
> +
> + adc:
> + type: object
> + $ref: /schemas/iio/adc/st,stmpe-adc.yaml#
> +
> + gpio:
> + type: object
> + $ref: /schemas/gpio/st,stmpe-gpio.yaml#
> +
> + keyboard-controller:
> + type: object
> + $ref: /schemas/input/matrix-keymap.yaml#
> +
> + unevaluatedProperties: false
> +
> + properties:
> + compatible:
> + const: st,stmpe-keypad
> +
> + debounce-interval:
> + description: Debouncing interval in milliseconds
> + $ref: /schemas/types.yaml#/definitions/uint32
> +
> + st,no-autorepeat:
> + description: If present, the keys will not autorepeat when pressed
> + $ref: /schemas/types.yaml#/definitions/flag
> +
> + st,scan-count:
> + description: Scanning cycles elapsed before key data is updated
> + $ref: /schemas/types.yaml#/definitions/uint32
> +
> + required:
> + - compatible
> + - linux,keymap
> +
> + pwm:
> + type: object
> + $ref: /schemas/pwm/pwm.yaml#
> +
> + unevaluatedProperties: false
> +
> + properties:
> + compatible:
> + const: st,stmpe-pwm
You need to define what value #pwm-cells should be.
> +
> + required:
> + - compatible
> + - "#pwm-cells"
pwm.yaml already requires this.
With those fixed,
Reviewed-by: Rob Herring <robh@kernel.org>
> +
> + touchscreen:
> + type: object
> + $ref: /schemas/input/touchscreen/touchscreen.yaml#
> +
> + unevaluatedProperties: false
> +
> + properties:
> + compatible:
> + const: st,stmpe-ts
> +
> + st,ave-ctrl:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + enum: [ 0, 1, 2, 3 ]
> + description: |
> + Sample average control
> + 0 = 1 sample
> + 1 = 2 samples
> + 2 = 4 samples
> + 3 = 8 samples
> +
> + st,touch-det-delay:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + enum: [ 0, 1, 2, 3, 4, 5, 6, 7 ]
> + description: |
> + Touch detection delay
> + 0 = 10 us
> + 1 = 50 us
> + 2 = 100 us
> + 3 = 500 us - recommended
> + 4 = 1 ms
> + 5 = 5 ms
> + 6 = 10 ms
> + 7 = 50 ms
> +
> + st,settling:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + enum: [ 0, 1, 2, 3, 4, 5, 6, 7 ]
> + description: |
> + Panel driver settling time
> + 0 = 10 us
> + 1 = 100 us
> + 2 = 500 us - recommended
> + 3 = 1 ms
> + 4 = 5 ms
> + 5 = 10 ms
> + 6 = 50 ms
> + 7 = 100 ms
> +
> + st,fraction-z:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + enum: [ 0, 1, 2, 3, 4, 5, 6, 7 ]
> + description: Length of the fractional part in z, recommended is 7
> + (fraction-z ([0..7]) = Count of the fractional part)
> +
> + st,i-drive:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + enum: [ 0, 1 ]
> + description: |
> + current limit value of the touchscreen drivers
> + 0 = 20 mA (typical 35 mA max)
> + 1 = 50 mA (typical 80 mA max)
> +
> + required:
> + - compatible
> +
> +additionalProperties: false
> +
> +required:
> + - compatible
> + - reg
> + - interrupts
> +
> +examples:
> + - |
> + #include <dt-bindings/gpio/gpio.h>
> + #include <dt-bindings/interrupt-controller/irq.h>
> + #include <dt-bindings/input/input.h>
> + i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port-expander@43 {
> + compatible = "st,stmpe2401";
> + reg = <0x43>;
> + reset-gpios = <&gpio 13 GPIO_ACTIVE_LOW>;
> + interrupts = <26 IRQ_TYPE_EDGE_FALLING>;
> + interrupt-parent = <&gpio>;
> + vcc-supply = <&db8500_vsmps2_reg>;
> + vio-supply = <&db8500_vsmps2_reg>;
> + wakeup-source;
> + st,autosleep-timeout = <1024>;
> +
> + gpio {
> + compatible = "st,stmpe-gpio";
> + gpio-controller;
> + #gpio-cells = <2>;
> + interrupt-controller;
> + #interrupt-cells = <2>;
> + st,norequest-mask = <0xf0f002>;
> + };
> +
> + keyboard-controller {
> + compatible = "st,stmpe-keypad";
> + debounce-interval = <64>;
> + st,scan-count = <8>;
> + st,no-autorepeat;
> + keypad,num-rows = <8>;
> + keypad,num-columns = <8>;
> + linux,keymap = <
> + MATRIX_KEY(0x00, 0x00, KEY_1)
> + MATRIX_KEY(0x00, 0x01, KEY_2)
> + MATRIX_KEY(0x00, 0x02, KEY_3)
> + MATRIX_KEY(0x00, 0x03, KEY_4)
> + MATRIX_KEY(0x00, 0x04, KEY_5)
> + MATRIX_KEY(0x00, 0x05, KEY_6)
> + MATRIX_KEY(0x00, 0x06, KEY_7)
> + MATRIX_KEY(0x00, 0x07, KEY_8)
> + MATRIX_KEY(0x00, 0x08, KEY_9)
> + MATRIX_KEY(0x00, 0x09, KEY_0)
> + >;
> + };
> +
> + pwm {
> + compatible = "st,stmpe-pwm";
> + #pwm-cells = <2>;
> + };
> + };
> +
> + port-expander@41 {
> + compatible = "st,stmpe811";
> + reg = <0x41>;
> + interrupts = <10 IRQ_TYPE_LEVEL_LOW>;
> + interrupt-parent = <&gpio>;
> + st,adc-freq = <1>;
> + st,mod-12b = <1>;
> + st,ref-sel = <0>;
> + st,sample-time = <4>;
> +
> + adc {
> + compatible = "st,stmpe-adc";
> + st,norequest-mask = <0x0f>;
> + #io-channel-cells = <1>;
> + };
> +
> + gpio {
> + compatible = "st,stmpe-gpio";
> + gpio-controller;
> + #gpio-cells = <2>;
> + interrupt-controller;
> + #interrupt-cells = <2>;
> + };
> +
> + pwm {
> + compatible = "st,stmpe-pwm";
> + #pwm-cells = <2>;
> + };
> +
> + touchscreen {
> + compatible = "st,stmpe-ts";
> + st,ave-ctrl = <3>;
> + st,touch-det-delay = <5>;
> + st,settling = <3>;
> + st,fraction-z = <7>;
> + st,i-drive = <1>;
> + };
> + };
> + };
> +...
^ permalink raw reply
* Re: [PATCH v2 1/2] dt-bindings: gpio: Add STMPE YAML DT schema
From: Rob Herring @ 2023-04-27 16:10 UTC (permalink / raw)
To: Linus Walleij
Cc: Bartosz Golaszewski, Krzysztof Kozlowski, Maxime Coquelin,
Alexandre Torgue, Dmitry Torokhov, Lee Jones, Philippe Schenker,
Stefan Agner, Marek Vasut, Steffen Trumtrar, linux-gpio,
devicetree, linux-stm32, linux-arm-kernel, linux-kernel,
linux-input
In-Reply-To: <20230426-stmpe-dt-bindings-v2-1-2f85a1fffcda@linaro.org>
On Wed, Apr 26, 2023 at 01:21:38PM +0200, Linus Walleij wrote:
> This adds a schema for the STMPE GPIO that while it is used a
> lot in the kernel tree is anyway missing its bindings.
>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v1->v2:
> - New patch split off from the MFD patch.
> ---
> .../devicetree/bindings/gpio/st,stmpe-gpio.yaml | 60 ++++++++++++++++++++++
> 1 file changed, 60 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/gpio/st,stmpe-gpio.yaml b/Documentation/devicetree/bindings/gpio/st,stmpe-gpio.yaml
> new file mode 100644
> index 000000000000..6e991ebbdf77
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/st,stmpe-gpio.yaml
> @@ -0,0 +1,60 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/gpio/st,stmpe-gpio.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: STMicroelectonics Port Expander (STMPE) GPIO Block
> +
> +description: STMicroelectronics Port Expander (STMPE) is a series of slow
> + bus controllers for various expanded peripherals such as GPIO, keypad,
> + touchscreen, ADC, PWM or rotator. It can contain one or several different
> + peripherals connected to SPI or I2C. These bindings pertain to the
> + GPIO portions of these expanders.
> +
> +maintainers:
> + - Linus Walleij <linus.walleij@linaro.org>
> +
> +properties:
> + compatible:
> + const: st,stmpe-gpio
> +
> + "#gpio-cells":
> + const: 2
> +
> + "#interrupt-cells":
> + const: 2
> +
> + gpio-controller: true
> +
> + interrupt-controller: true
> +
> + st,norequest-mask:
> + description: A bitmask of GPIO lines that cannot be requested because for
> + for example not being connected to anything on the system
> + $ref: /schemas/types.yaml#/definitions/uint32
> +
> +patternProperties:
> + "^.+-hog(-[0-9]+)?$":
> + type: object
> +
> + properties:
> + gpio-hog: true
> + gpios: true
> + input: true
> + output-high: true
> + output-low: true
> + line-name: true
> +
> + required:
> + - gpio-hog
> + - gpios
All you need for hog nodes is:
"^.+-hog(-[0-9]+)?$":
type: object
required:
- gpio-hog
The presence of 'gpio-hog' causes the hog schema to be applied.
Rob
^ permalink raw reply
* Re: [RFC v1 0/4] Input: support virtual objects on touchscreens
From: Javier Carrasco @ 2023-04-27 15:59 UTC (permalink / raw)
To: Jeff LaBundy, Thomas Weißschuh
Cc: linux-input, devicetree, linux-kernel, Dmitry Torokhov,
Rob Herring, Krzysztof Kozlowski, Henrik Rydberg, Ulf Hansson,
Hans Verkuil, Stephen Boyd, Sebastian Reichel, Linus Walleij,
Jonathan Cameron, Uwe Kleine-g, Bastian Hecht, Michael Riesch
In-Reply-To: <ZEf5rfzs22HtQivB@nixie71>
Hi,
On 25.04.23 18:02, Jeff LaBundy wrote:
> Hi Thomas,
>
> On Tue, Apr 25, 2023 at 05:29:39PM +0200, Thomas Weißschuh wrote:
>> Hi Javier,
>>
>> On 2023-04-25 13:50:45+0200, Javier Carrasco wrote:
>>> Some touchscreens are shipped with a physical layer on top of them where
>>> a number of buttons and a resized touchscreen surface might be available.
>>>
>>> In order to generate proper key events by overlay buttons and adjust the
>>> touch events to a clipped surface, these patches offer a documented,
>>> device-tree-based solution by means of helper functions.
>>> An implementation for a specific touchscreen driver is also included.
>>>
>>> The functions in ts-virtobj provide a simple workflow to acquire
>>> physical objects from the device tree, map them into the device driver
>>> structures as virtual objects and generate events according to
>>> the object descriptions.
>>>
>>> This solution has been tested with a JT240MHQS-E3 display, which uses
>>> the st1624 as a touchscreen and provides two overly buttons and a frame
>>> that clips its effective surface.
>>
>> There are quite a few of notebooks from Asus that feature a printed
>> numpad on their touchpad [0]. The mapping from the touch events to the
>> numpad events needs to happen in software.
>
> That example seems a kind of fringe use-case in my opinion; I think the
> gap filled by this RFC is the case where a touchscreen has a printed
> overlay with a key that represents a fixed function.
Exactly, this RFC addresses exactly such printed overlays.
>
> One problem I do see here is something like libinput or multitouch taking
> hold of the input device, and swallowing the key presses because it sees
> the device as a touchscreen and is not interested in these keys.
Unfortunately I do not know libinput or multitouch and I might be
getting you wrong, but I guess the same would apply to any event
consumer that takes touchscreens as touch event producers and nothing else.
Should they not check the supported events from the device instead of
making such assumptions? This RFC adds key events defined in the device
tree and they are therefore available and published as device
capabilities. That is for example what evtest does to report the
supported events and they are then notified accordingly. Is that not the
right way to do it?
Thanks a lot for your feedback!
>
> Therefore, my first impression is that the virtual keypad may be better
> served by registering its own input device.
>
> Great work by the way, Javier!
>
>>
>> Do you think your solution is general enough to also support this
>> usecase?
>>
>> The differences I see are
>> * not device-tree based
>> * touchpads instead of touchscreens
>>
>>> [..]
>>
>> [0] https://unix.stackexchange.com/q/494400
>
> Kind regards,
> Jeff LaBundy
^ permalink raw reply
* Re: [RFC v1 0/4] Input: support virtual objects on touchscreens
From: Pavel Machek @ 2023-04-27 13:15 UTC (permalink / raw)
To: Michael Riesch
Cc: Javier Carrasco, phone-devel, linux-input, devicetree,
linux-kernel, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Henrik Rydberg, Ulf Hansson, Hans Verkuil, Stephen Boyd,
Sebastian Reichel, Linus Walleij, Jonathan Cameron, Uwe Kleine-g,
Bastian Hecht
In-Reply-To: <de87d6ae-3449-5581-3e17-4aae72e8197a@wolfvision.net>
[-- Attachment #1: Type: text/plain, Size: 1352 bytes --]
Hi!
> >
> >> Some touchscreens are shipped with a physical layer on top of them where
> >> a number of buttons and a resized touchscreen surface might be
> >> available.
> >
> > Yes, it is quite comon, for example Motorola Droid 4 has 4 virtual
> > buttons below touchscreen.
>
> Are those buttons configurable in some way? Or do they have a fixed purpose?
Fixed.
> How does Android handle those buttons, BTW?
No idea.
> > One question is if this should be handled inside the kernel. It will
> > make it compatible with existing software, but it will also reduce
> > flexibility.
>
> I would say that it should be described in device tree if the purpose is
> fixed. For example, if there is no display behind the touch screen at a
> certain point but a printed sheet (e.g., with a home or return symbol)
> then it is clear that this button is not going to change. In such a case
> I doubt that flexibility is required.
I agree it should be in the device tree.
AFAICT hardware can do drags between the buttons, and drag between the
buttons and touchscreen. Turning it into buttons prevents that.
Plus, real buttons can do simultaneous presses on all of them,
touchscreens will have problems with that.
Best regards,
Pavel
--
People of Russia, stop Putin before his war on Ukraine escalates.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]
^ permalink raw reply
* [PATCH] HID: wacom: Added error check
From: Denis Arefev @ 2023-04-27 11:47 UTC (permalink / raw)
To: Ping Cheng
Cc: Jason Gerecke, Jiri Kosina, Benjamin Tissoires, linux-input,
linux-kernel, lvc-project, trufanov, vfh
Added a variable check and
transition in case of an error
Found by Linux Verification Center (linuxtesting.org) with SVACE.
Signed-off-by: Denis Arefev <arefev@swemel.ru>
---
drivers/hid/wacom_sys.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index fb538a6c4add..aff4a21a46b6 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -2417,8 +2417,13 @@ static int wacom_parse_and_register(struct wacom *wacom, bool wireless)
goto fail_quirks;
}
- if (features->device_type & WACOM_DEVICETYPE_WL_MONITOR)
+ if (features->device_type & WACOM_DEVICETYPE_WL_MONITOR) {
error = hid_hw_open(hdev);
+ if (error) {
+ hid_err(hdev, "hw open failed\n");
+ goto fail_quirks;
+ }
+ }
wacom_set_shared_values(wacom_wac);
devres_close_group(&hdev->dev, wacom);
--
2.25.1
^ permalink raw reply related
* [PATCH] HID: wacom: Added error check
From: Denis Arefev @ 2023-04-27 11:37 UTC (permalink / raw)
To: Ping Cheng
Cc: Jason Gerecke, Jiri Kosina, Benjamin Tissoires, linux-input,
linux-kernel, trufanov, vfh
Added a variable check and
transition in case of an error
Found by Linux Verification Center (linuxtesting.org) with SVACE.
Signed-off-by: Denis Arefev <arefev@swemel.ru>
---
drivers/hid/wacom_sys.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index fb538a6c4add..aff4a21a46b6 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -2417,8 +2417,13 @@ static int wacom_parse_and_register(struct wacom *wacom, bool wireless)
goto fail_quirks;
}
- if (features->device_type & WACOM_DEVICETYPE_WL_MONITOR)
+ if (features->device_type & WACOM_DEVICETYPE_WL_MONITOR) {
error = hid_hw_open(hdev);
+ if (error) {
+ hid_err(hdev, "hw open failed\n");
+ goto fail_quirks;
+ }
+ }
wacom_set_shared_values(wacom_wac);
devres_close_group(&hdev->dev, wacom);
--
2.25.1
^ permalink raw reply related
* Re: [RFC v1 0/4] Input: support virtual objects on touchscreens
From: Michael Riesch @ 2023-04-27 11:11 UTC (permalink / raw)
To: Pavel Machek, Javier Carrasco, phone-devel
Cc: linux-input, devicetree, linux-kernel, Dmitry Torokhov,
Rob Herring, Krzysztof Kozlowski, Henrik Rydberg, Ulf Hansson,
Hans Verkuil, Stephen Boyd, Sebastian Reichel, Linus Walleij,
Jonathan Cameron, Uwe Kleine-g, Bastian Hecht
In-Reply-To: <ZEpWrWpzkI9kNTkr@duo.ucw.cz>
Hi Pavel,
On 4/27/23 13:04, Pavel Machek wrote:
> Hi!
>
>> Some touchscreens are shipped with a physical layer on top of them where
>> a number of buttons and a resized touchscreen surface might be
>> available.
>
> Yes, it is quite comon, for example Motorola Droid 4 has 4 virtual
> buttons below touchscreen.
Are those buttons configurable in some way? Or do they have a fixed purpose?
How does Android handle those buttons, BTW?
> One question is if this should be handled inside the kernel. It will
> make it compatible with existing software, but it will also reduce
> flexibility.
I would say that it should be described in device tree if the purpose is
fixed. For example, if there is no display behind the touch screen at a
certain point but a printed sheet (e.g., with a home or return symbol)
then it is clear that this button is not going to change. In such a case
I doubt that flexibility is required.
Best regards, Michael
>
> Best regards,
> Pavel
^ permalink raw reply
* Re: [RFC v1 0/4] Input: support virtual objects on touchscreens
From: Pavel Machek @ 2023-04-27 11:04 UTC (permalink / raw)
To: Javier Carrasco, phone-devel
Cc: linux-input, devicetree, linux-kernel, Dmitry Torokhov,
Rob Herring, Krzysztof Kozlowski, Henrik Rydberg, Ulf Hansson,
Hans Verkuil, Stephen Boyd, Sebastian Reichel, Linus Walleij,
Jonathan Cameron, Uwe Kleine-g, Bastian Hecht, Michael Riesch
In-Reply-To: <20230425115049.870003-1-javier.carrasco@wolfvision.net>
[-- Attachment #1: Type: text/plain, Size: 519 bytes --]
Hi!
> Some touchscreens are shipped with a physical layer on top of them where
> a number of buttons and a resized touchscreen surface might be
> available.
Yes, it is quite comon, for example Motorola Droid 4 has 4 virtual
buttons below touchscreen.
One question is if this should be handled inside the kernel. It will
make it compatible with existing software, but it will also reduce
flexibility.
Best regards,
Pavel
--
People of Russia, stop Putin before his war on Ukraine escalates.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]
^ permalink raw reply
* Re: [PATCH] Fix freeze in lm8333 i2c keyboard driver
From: Tomáš Mudruňka @ 2023-04-27 8:19 UTC (permalink / raw)
To: Jeff LaBundy; +Cc: Dmitry Torokhov, linux-input, linux-kernel
In-Reply-To: <ZEnBZs9VYxriT1Or@nixie71>
> Yes that's correct; what I mean to say is that depending on the nature of
> the read-to-clear mechanism in the part, there is a chance that the IRQ
> has not been deasserted by the time the threaded handler returns. On some
> devices for example, the IRQ is not deasserted until some time after the
> read's stop condition.
>
> For these cases, I consider it best practice to measure the I2C and IRQ
> lines on a scope and if necessary, add a small delay before the interrupt
> handler returns. This is especially true for open-drain interrupts that
> may need a few hundred extra us for the pin to rise.
Well before posting the patch i did some testing.
I was watching the /proc/interrupts and checked that IRQ counter for
lm8333 matches number of keypresses.
Which i've only tested for like 20-30 times, but haven't seem any glitch.
But i still recognize the fact that the gpio line getting stuck for
some reason (short circuit on PCB?) might cause troubles by
unnecessarily loading the CPU, while with edge trigger it's more
likely to affect only the function of keyboard itself rather than
bringing down whole system. But i am not sure if this case is supposed
to be expected and handled in SW.
^ permalink raw reply
* Re: [PATCH v2] Fix freeze in lm8333 i2c keyboard driver
From: Tomáš Mudruňka @ 2023-04-27 8:13 UTC (permalink / raw)
To: Jeff LaBundy; +Cc: dmitry.torokhov, linux-input, linux-kernel
In-Reply-To: <ZEnErxfnSn2JOpvm@nixie71>
Hello, thanks for your notes.
> This is the right idea. I am sort of splitting hairs here, however I
> think it makes sense to place this read before the IRQ is requested
> and not after.
>
>
> As written, there is room for an ever-so-tiny race condition wherein
> the IRQ is asserted just after it is requested. Before the threaded
> handler has run however, the new read in probe swallows the IRQ status
> before the threaded handler can read it and react to errors.
In fact i believe quite the opposite case to be true.
If i read before registering IRQ there will be ever-so-tiny race condition that
would allow to miss the edge (exactly the bug this patch is fixing,
but limited).
In the case you describe the worst scenario is likely that the interrupt handler
will be called only to re-read status and immediately return on this condition:
if (!status) return IRQ_NONE;
> Also, I think you should at least capture and evaluate lm8333_read8()'s
> return value as is already done for the calls to lm8333_write8().
Well. If you think this will bring any benefits, i might as well just call
lm8333_irq_thread() instead of lm8333_read8()
Would that be acceptable solution?
Tom.
^ permalink raw reply
* [PATCH v4 2/2] HID: i2c-hid: goodix: Add support for "goodix,no-reset-during-suspend" property
From: Fei Shao @ 2023-04-27 3:56 UTC (permalink / raw)
To: Jeff LaBundy, Douglas Anderson, Benjamin Tissoires, Rob Herring
Cc: linux-mediatek, Fei Shao, Dmitry Torokhov, Jiri Kosina,
Matthias Kaehlcke, Stephen Kitt, linux-input, linux-kernel
In-Reply-To: <20230427035656.1962698-1-fshao@chromium.org>
In the beginning, commit 18eeef46d359 ("HID: i2c-hid: goodix: Tie the
reset line to true state of the regulator") introduced a change to tie
the reset line of the Goodix touchscreen to the state of the regulator
to fix a power leakage issue in suspend.
After some time, the change was deemed unnecessary and was reverted in
commit 557e05fa9fdd ("HID: i2c-hid: goodix: Stop tying the reset line to
the regulator") due to difficulties in managing regulator notifiers for
designs like Evoker, which provides a second power rail to touchscreen.
However, the revert caused a power regression on another Chromebook
device Steelix in the field, which has a dedicated always-on regulator
for touchscreen and was covered by the workaround in the first commit.
To address both cases, this patch adds the support for the new
"goodix,no-reset-during-suspend" property in the driver:
- When set to true, the driver does not assert the reset GPIO during
power-down.
Instead, the GPIO will be asserted during power-up to ensure the
touchscreen always has a clean start and consistent behavior after
resuming.
This is for designs with a dedicated always-on regulator.
- When set to false or unset, the driver uses the original control flow
and asserts GPIO and disables regulators normally.
This is for the two-regulator and shared-regulator designs.
Signed-off-by: Fei Shao <fshao@chromium.org>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Jeff LaBundy <jeff@labundy.com>
---
Changes in v4:
- Minor coding style improvement
Changes in v3:
- In power-down, only skip the GPIO but not the regulator calls if the
flag is set
Changes in v2:
- Do not change the regulator_enable logic during power-up.
drivers/hid/i2c-hid/i2c-hid-of-goodix.c | 16 +++++++++++++++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/i2c-hid/i2c-hid-of-goodix.c b/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
index 0060e3dcd775..db4639db9840 100644
--- a/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
+++ b/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
@@ -28,6 +28,7 @@ struct i2c_hid_of_goodix {
struct regulator *vdd;
struct regulator *vddio;
struct gpio_desc *reset_gpio;
+ bool no_reset_during_suspend;
const struct goodix_i2c_hid_timing_data *timings;
};
@@ -37,6 +38,14 @@ static int goodix_i2c_hid_power_up(struct i2chid_ops *ops)
container_of(ops, struct i2c_hid_of_goodix, ops);
int ret;
+ /*
+ * We assert reset GPIO here (instead of during power-down) to ensure
+ * the device will have a clean state after powering up, just like the
+ * normal scenarios will have.
+ */
+ if (ihid_goodix->no_reset_during_suspend)
+ gpiod_set_value_cansleep(ihid_goodix->reset_gpio, 1);
+
ret = regulator_enable(ihid_goodix->vdd);
if (ret)
return ret;
@@ -60,7 +69,9 @@ static void goodix_i2c_hid_power_down(struct i2chid_ops *ops)
struct i2c_hid_of_goodix *ihid_goodix =
container_of(ops, struct i2c_hid_of_goodix, ops);
- gpiod_set_value_cansleep(ihid_goodix->reset_gpio, 1);
+ if (!ihid_goodix->no_reset_during_suspend)
+ gpiod_set_value_cansleep(ihid_goodix->reset_gpio, 1);
+
regulator_disable(ihid_goodix->vddio);
regulator_disable(ihid_goodix->vdd);
}
@@ -91,6 +102,9 @@ static int i2c_hid_of_goodix_probe(struct i2c_client *client)
if (IS_ERR(ihid_goodix->vddio))
return PTR_ERR(ihid_goodix->vddio);
+ ihid_goodix->no_reset_during_suspend =
+ of_property_read_bool(client->dev.of_node, "goodix,no-reset-during-suspend");
+
ihid_goodix->timings = device_get_match_data(&client->dev);
return i2c_hid_core_probe(client, &ihid_goodix->ops, 0x0001, 0);
--
2.40.1.495.gc816e09b53d-goog
^ permalink raw reply related
* [PATCH v4 1/2] dt-bindings: input: goodix: Add "goodix,no-reset-during-suspend" property
From: Fei Shao @ 2023-04-27 3:56 UTC (permalink / raw)
To: Jeff LaBundy, Douglas Anderson, Benjamin Tissoires, Rob Herring
Cc: linux-mediatek, Fei Shao, Matthias Brugger, Dmitry Torokhov,
Krzysztof Kozlowski, devicetree, linux-input, linux-kernel
In-Reply-To: <20230427035656.1962698-1-fshao@chromium.org>
We observed that on Chromebook device Steelix, if Goodix GT7375P
touchscreen is powered in suspend (because, for example, it connects to
an always-on regulator) and with the reset GPIO asserted, it will
introduce about 14mW power leakage.
To address that, we add this property to skip reset during suspend.
If it's set, the driver will stop asserting the reset GPIO during
power-down. Refer to the comments in the driver for details.
Signed-off-by: Fei Shao <fshao@chromium.org>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>
Reviewed-by: Jeff LaBundy <jeff@labundy.com>
---
(no changes since v2)
Changes in v2:
- Use a more accurate property name and with "goodix," prefix.
.../devicetree/bindings/input/goodix,gt7375p.yaml | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml b/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml
index ce18d7dadae2..1edad1da1196 100644
--- a/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml
+++ b/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml
@@ -43,6 +43,15 @@ properties:
itself as long as it allows the main board to make signals compatible
with what the touchscreen is expecting for its IO rails.
+ goodix,no-reset-during-suspend:
+ description:
+ Set this to true to enforce the driver to not assert the reset GPIO
+ during suspend.
+ Due to potential touchscreen hardware flaw, back-powering could happen in
+ suspend if the power supply is on and with active-low reset GPIO asserted.
+ This property is used to avoid the back-powering issue.
+ type: boolean
+
required:
- compatible
- reg
--
2.40.1.495.gc816e09b53d-goog
^ permalink raw reply related
* [PATCH v4 0/2] Fix Goodix touchscreen power leakage for MT8186 boards
From: Fei Shao @ 2023-04-27 3:56 UTC (permalink / raw)
To: Jeff LaBundy, Douglas Anderson, Benjamin Tissoires, Rob Herring
Cc: linux-mediatek, Fei Shao, Dmitry Torokhov, Jiri Kosina,
Krzysztof Kozlowski, Matthias Kaehlcke, Stephen Kitt, devicetree,
linux-input, linux-kernel
These changes are based on the series in [1], which modified the
i2c-hid-of-goodix driver and removed the workaround for a power leakage
issue, so the issue revisits on Mediatek MT8186 boards (Steelix).
The root cause is that the touchscreen can be powered in different ways
depending on the hardware designs, and it's not as easy to come up with
a solution that is both simple and elegant for all the known designs.
To address the issue, I ended up adding a new boolean property for the
driver so that we can control the power up/down sequence depending on
that.
Adding a new property might not be the cleanest approach for this, but
at least the intention would be easy enough to understand, and it
introduces relatively small change to the code and fully preserves the
original control flow.
I hope this is something acceptable, and I'm open to any better
approaches.
[1] https://lore.kernel.org/all/20230207024816.525938-1-dianders@chromium.org/
Changes in v4:
- Minor coding style improvement
Changes in v3:
- In power-down, only skip the GPIO but not the regulator calls if the
flag is set
Changes in v2:
- Use a more accurate property name and with "goodix," prefix.
- Do not change the regulator_enable logic during power-up.
Fei Shao (2):
dt-bindings: input: goodix: Add "goodix,no-reset-during-suspend"
property
HID: i2c-hid: goodix: Add support for "goodix,no-reset-during-suspend"
property
.../bindings/input/goodix,gt7375p.yaml | 9 +++++++++
drivers/hid/i2c-hid/i2c-hid-of-goodix.c | 16 +++++++++++++++-
2 files changed, 24 insertions(+), 1 deletion(-)
--
2.40.1.495.gc816e09b53d-goog
^ permalink raw reply
* Re: [PATCH v3 2/2] HID: i2c-hid: goodix: Add support for "goodix,no-reset-during-suspend" property
From: Fei Shao @ 2023-04-27 3:45 UTC (permalink / raw)
To: Jeff LaBundy
Cc: Douglas Anderson, Benjamin Tissoires, Rob Herring, linux-mediatek,
Dmitry Torokhov, Jiri Kosina, Matthias Kaehlcke, Stephen Kitt,
linux-input, linux-kernel
In-Reply-To: <ZEnK7bQkl+fkcHkb@nixie71>
Hi,
On Thu, Apr 27, 2023 at 9:08 AM Jeff LaBundy <jeff@labundy.com> wrote:
>
> Hi Fei,
>
> On Wed, Apr 26, 2023 at 10:44:22PM +0800, Fei Shao wrote:
> > In the beginning, commit 18eeef46d359 ("HID: i2c-hid: goodix: Tie the
> > reset line to true state of the regulator") introduced a change to tie
> > the reset line of the Goodix touchscreen to the state of the regulator
> > to fix a power leakage issue in suspend.
> >
> > After some time, the change was deemed unnecessary and was reverted in
> > commit 557e05fa9fdd ("HID: i2c-hid: goodix: Stop tying the reset line to
> > the regulator") due to difficulties in managing regulator notifiers for
> > designs like Evoker, which provides a second power rail to touchscreen.
> >
> > However, the revert caused a power regression on another Chromebook
> > device Steelix in the field, which has a dedicated always-on regulator
> > for touchscreen and was covered by the workaround in the first commit.
> >
> > To address both cases, this patch adds the support for the new
> > "goodix,no-reset-during-suspend" property in the driver:
> > - When set to true, the driver does not assert the reset GPIO during
> > power-down.
> > Instead, the GPIO will be asserted during power-up to ensure the
> > touchscreen always has a clean start and consistent behavior after
> > resuming.
> > This is for designs with a dedicated always-on regulator.
> > - When set to false or unset, the driver uses the original control flow
> > and asserts GPIO and disables regulators normally.
> > This is for the two-regulator and shared-regulator designs.
> >
> > Signed-off-by: Fei Shao <fshao@chromium.org>
> > Reviewed-by: Douglas Anderson <dianders@chromium.org>
>
> Great work; one tiny nit below. If you do not agree with it or have found
> precedent that suggests it is OK, feel free to ignore it. Either way:
>
> Reviewed-by: Jeff LaBundy <jeff@labundy.com>
>
> >
> > ---
> >
> > Changes in v3:
> > - In power-down, only skip the GPIO but not the regulator calls if the
> > flag is set
> >
> > Changes in v2:
> > - Do not change the regulator_enable logic during power-up.
> >
> > drivers/hid/i2c-hid/i2c-hid-of-goodix.c | 16 +++++++++++++++-
> > 1 file changed, 15 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/hid/i2c-hid/i2c-hid-of-goodix.c b/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
> > index 0060e3dcd775..3ed365b50432 100644
> > --- a/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
> > +++ b/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
> > @@ -28,6 +28,7 @@ struct i2c_hid_of_goodix {
> > struct regulator *vdd;
> > struct regulator *vddio;
> > struct gpio_desc *reset_gpio;
> > + bool no_reset_during_suspend;
> > const struct goodix_i2c_hid_timing_data *timings;
> > };
> >
> > @@ -37,6 +38,14 @@ static int goodix_i2c_hid_power_up(struct i2chid_ops *ops)
> > container_of(ops, struct i2c_hid_of_goodix, ops);
> > int ret;
> >
> > + if (ihid_goodix->no_reset_during_suspend) {
> > + /*
> > + * We assert reset GPIO here (instead of during power-down) to
> > + * ensure the device will have a clean state after powering up,
> > + * just like the normal scenarios will have.
> > + */
> > + gpiod_set_value_cansleep(ihid_goodix->reset_gpio, 1);
> > + }
>
> I don't think curly braces are technically required here, as there is only
> one statement within the conditional despite the comments making it appear
> otherwise. Maybe this would work too:
>
> /* ... */
> if (...)
> gpiod_set_value_cansleep(...);
>
> Again however, I do not feel strongly about this.
Oops, I just carried it over from v1 and didn't think that much at the
moment, but indeed I should have cleaned that up.
V4 is on the way, thanks for pointing that out. :)
Regards,
Fei
>
> > ret = regulator_enable(ihid_goodix->vdd);
> > if (ret)
> > return ret;
> > @@ -60,7 +69,9 @@ static void goodix_i2c_hid_power_down(struct i2chid_ops *ops)
> > struct i2c_hid_of_goodix *ihid_goodix =
> > container_of(ops, struct i2c_hid_of_goodix, ops);
> >
> > - gpiod_set_value_cansleep(ihid_goodix->reset_gpio, 1);
> > + if (!ihid_goodix->no_reset_during_suspend)
> > + gpiod_set_value_cansleep(ihid_goodix->reset_gpio, 1);
> > +
> > regulator_disable(ihid_goodix->vddio);
> > regulator_disable(ihid_goodix->vdd);
> > }
> > @@ -91,6 +102,9 @@ static int i2c_hid_of_goodix_probe(struct i2c_client *client)
> > if (IS_ERR(ihid_goodix->vddio))
> > return PTR_ERR(ihid_goodix->vddio);
> >
> > + ihid_goodix->no_reset_during_suspend =
> > + of_property_read_bool(client->dev.of_node, "goodix,no-reset-during-suspend");
> > +
> > ihid_goodix->timings = device_get_match_data(&client->dev);
> >
> > return i2c_hid_core_probe(client, &ihid_goodix->ops, 0x0001, 0);
> > --
> > 2.40.1.495.gc816e09b53d-goog
> >
>
> Kind regards,
> Jeff LaBundy
^ permalink raw reply
* Re: Re: [PATCH] Input: vmmouse - add macros to enable vmmouse relative mode
From: Zack Rusin @ 2023-04-27 2:35 UTC (permalink / raw)
To: dmitry.torokhov@gmail.com, Pv-drivers, zhouzongmin@kylinos.cn,
Linux-graphics-maintainer
Cc: linux-input@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <rk3ip31xbz-rk4smwgz5s@nsmail7.0.0--kylin--1>
On Thu, 2023-04-20 at 09:49 +0800, 周宗敏 wrote:
> Dear zack:
>
> As far as I know, I think in the current design for vmmouse device,
> the mouse mode can only choose one,can't request both two mode.
>
> The flowchart for vmmouse device between host and guest roughly as follows：
> QEMU VMMouse emulation code defined variables of 's->absolute'
> to identify the mouse mode requested by the guest driver.
> Based on the value of 's->absolute',qemu will add different spice-input VD-
> Interface,
> spice-server will notify client to use the correct mouse mode,
> and then spice client will update mouse motion/position based on the mouse mode.
>
> About the VMMOUSE_RELATIVE_PACKET events,I guess that designer may want use it
> to distinguish relative from absolute of the process events.But doesn't mark them
> as such
> on QEMU's vmmouse device code.In fact, the status corresponds to the following
> buttons value on QEMU：
>
> From the screenshot we can know it didn't mark the mouse mode status on original
> design,
> only set the actual button state. So I think have to mark it here according to the
> value of 's->absolute'.
Hi,
we had a quick chat about this internally and I don't think we can make this work as
is. The core issue is that this driver is for VMware's mouse, if qemu emulation of
that device diverges from that then we can't make it work, i.e. if the device that
qemu emulates doesn't match the behaviour of vmmouse then it's not a vmmouse.
I think there are three possible solutions here:
1) Fix qemu so that it properly flags relative vs absolute events
2) Implement some capabilities mechanism so that we can shoehorn what effectively is
a different device that happens to have the same pci id into this driver (not ideal)
3) Fork the driver, make any changes you wish and just remember to change the device
detection for the qemu mouse so it differs from vmmouse. The driver is fairly simple
and is not too big, if fixing qemu is not an option then you effectively have a
different device and then just properly treat it as such. From your initial patch
where you wanted to have the relative flag as a define, it seems that your kernel is
custom anyway (because otherwise you wouldn't be able to set that define) so this
should be trivially doable on your config. And long term you can submit the new
driver to the kernel (as long as you can make sure the mouse detection code doesn't
detect the actual vmmouse of course to conflict with this driver).
z
^ permalink raw reply
* Re: [PATCH v3 2/2] HID: i2c-hid: goodix: Add support for "goodix,no-reset-during-suspend" property
From: Jeff LaBundy @ 2023-04-27 1:07 UTC (permalink / raw)
To: Fei Shao
Cc: Douglas Anderson, Benjamin Tissoires, Rob Herring, linux-mediatek,
Dmitry Torokhov, Jiri Kosina, Matthias Kaehlcke, Stephen Kitt,
linux-input, linux-kernel
In-Reply-To: <20230426144423.2820826-3-fshao@chromium.org>
Hi Fei,
On Wed, Apr 26, 2023 at 10:44:22PM +0800, Fei Shao wrote:
> In the beginning, commit 18eeef46d359 ("HID: i2c-hid: goodix: Tie the
> reset line to true state of the regulator") introduced a change to tie
> the reset line of the Goodix touchscreen to the state of the regulator
> to fix a power leakage issue in suspend.
>
> After some time, the change was deemed unnecessary and was reverted in
> commit 557e05fa9fdd ("HID: i2c-hid: goodix: Stop tying the reset line to
> the regulator") due to difficulties in managing regulator notifiers for
> designs like Evoker, which provides a second power rail to touchscreen.
>
> However, the revert caused a power regression on another Chromebook
> device Steelix in the field, which has a dedicated always-on regulator
> for touchscreen and was covered by the workaround in the first commit.
>
> To address both cases, this patch adds the support for the new
> "goodix,no-reset-during-suspend" property in the driver:
> - When set to true, the driver does not assert the reset GPIO during
> power-down.
> Instead, the GPIO will be asserted during power-up to ensure the
> touchscreen always has a clean start and consistent behavior after
> resuming.
> This is for designs with a dedicated always-on regulator.
> - When set to false or unset, the driver uses the original control flow
> and asserts GPIO and disables regulators normally.
> This is for the two-regulator and shared-regulator designs.
>
> Signed-off-by: Fei Shao <fshao@chromium.org>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>
Great work; one tiny nit below. If you do not agree with it or have found
precedent that suggests it is OK, feel free to ignore it. Either way:
Reviewed-by: Jeff LaBundy <jeff@labundy.com>
>
> ---
>
> Changes in v3:
> - In power-down, only skip the GPIO but not the regulator calls if the
> flag is set
>
> Changes in v2:
> - Do not change the regulator_enable logic during power-up.
>
> drivers/hid/i2c-hid/i2c-hid-of-goodix.c | 16 +++++++++++++++-
> 1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hid/i2c-hid/i2c-hid-of-goodix.c b/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
> index 0060e3dcd775..3ed365b50432 100644
> --- a/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
> +++ b/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
> @@ -28,6 +28,7 @@ struct i2c_hid_of_goodix {
> struct regulator *vdd;
> struct regulator *vddio;
> struct gpio_desc *reset_gpio;
> + bool no_reset_during_suspend;
> const struct goodix_i2c_hid_timing_data *timings;
> };
>
> @@ -37,6 +38,14 @@ static int goodix_i2c_hid_power_up(struct i2chid_ops *ops)
> container_of(ops, struct i2c_hid_of_goodix, ops);
> int ret;
>
> + if (ihid_goodix->no_reset_during_suspend) {
> + /*
> + * We assert reset GPIO here (instead of during power-down) to
> + * ensure the device will have a clean state after powering up,
> + * just like the normal scenarios will have.
> + */
> + gpiod_set_value_cansleep(ihid_goodix->reset_gpio, 1);
> + }
I don't think curly braces are technically required here, as there is only
one statement within the conditional despite the comments making it appear
otherwise. Maybe this would work too:
/* ... */
if (...)
gpiod_set_value_cansleep(...);
Again however, I do not feel strongly about this.
> ret = regulator_enable(ihid_goodix->vdd);
> if (ret)
> return ret;
> @@ -60,7 +69,9 @@ static void goodix_i2c_hid_power_down(struct i2chid_ops *ops)
> struct i2c_hid_of_goodix *ihid_goodix =
> container_of(ops, struct i2c_hid_of_goodix, ops);
>
> - gpiod_set_value_cansleep(ihid_goodix->reset_gpio, 1);
> + if (!ihid_goodix->no_reset_during_suspend)
> + gpiod_set_value_cansleep(ihid_goodix->reset_gpio, 1);
> +
> regulator_disable(ihid_goodix->vddio);
> regulator_disable(ihid_goodix->vdd);
> }
> @@ -91,6 +102,9 @@ static int i2c_hid_of_goodix_probe(struct i2c_client *client)
> if (IS_ERR(ihid_goodix->vddio))
> return PTR_ERR(ihid_goodix->vddio);
>
> + ihid_goodix->no_reset_during_suspend =
> + of_property_read_bool(client->dev.of_node, "goodix,no-reset-during-suspend");
> +
> ihid_goodix->timings = device_get_match_data(&client->dev);
>
> return i2c_hid_core_probe(client, &ihid_goodix->ops, 0x0001, 0);
> --
> 2.40.1.495.gc816e09b53d-goog
>
Kind regards,
Jeff LaBundy
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox