* Re: [RFC PATCH v2 6/7] dt-bindings: arm: mediatek: Remove SKU specific compatibles for Google Krane
From: Doug Anderson @ 2023-11-11 0:29 UTC (permalink / raw)
To: Rob Herring
Cc: Chen-Yu Tsai, Frank Rowand, Krzysztof Kozlowski, Conor Dooley,
Matthias Brugger, AngeloGioacchino Del Regno, Hsin-Yi Wang,
Dmitry Torokhov, andriy.shevchenko, Jiri Kosina, linus.walleij,
broonie, gregkh, hdegoede, james.clark, james, keescook,
petr.tesarik.ext, rafael, tglx, Jeff LaBundy, linux-input,
devicetree, linux-arm-kernel, linux-mediatek, linux-kernel,
Johan Hovold
In-Reply-To: <20231110210443.GA419831-robh@kernel.org>
Hi,
On Fri, Nov 10, 2023 at 1:04 PM Rob Herring <robh@kernel.org> wrote:
>
> On Thu, Nov 09, 2023 at 06:06:03PM +0800, Chen-Yu Tsai wrote:
> > In cases where the same Chromebook model is manufactured with different
> > components (MIPI DSI panels, MIPI CSI camera sensors, or trackpad /
> > touchscreens with conflicting addresses), a different SKU ID is
> > allocated to each specific combination. This SKU ID is exported by the
> > bootloader into the device tree, and can be used to "discover" which
> > combination is present on the current machine. Thus we no longer have
> > to specify separate compatible strings for each of them.
>
> You just broke an existing kernel with a new DT having this change.
>
> Just because you come up with a new way to do things, doesn't mean you
> can remove the old way.
I was wondering about that, actually. My understanding was that what
Chen-Yu was doing here was correct, but I'm happy to be educated.
Specifically, I think that after his series old device trees will
continue to boot just fine. ...so if someone took a device tree from
before his series and booted it on a kernel after his series that
everything would be hunky dory. If that doesn't work then, I agree,
that should be fixed.
However, here, he is documenting what the "latest and greatest" device
tree should look at and that matches what's checked into the "dts"
directory. In general, I thought that yaml files didn't necessarily
always document old/deprecated ways of doing things and just focused
on documenting the new/best way.
Now, obviously, if someone took a new device tree and tried to put it
on an old kernel then it wouldn't work, but I was always under the
impression that wasn't a requirement.
-Doug
^ permalink raw reply
* Re: [RFC PATCH v2 0/7] of: Introduce hardware prober driver
From: Doug Anderson @ 2023-11-11 0:22 UTC (permalink / raw)
To: Chen-Yu Tsai
Cc: Rob Herring, Frank Rowand, Krzysztof Kozlowski, Conor Dooley,
Matthias Brugger, AngeloGioacchino Del Regno, Hsin-Yi Wang,
Dmitry Torokhov, andriy.shevchenko, Jiri Kosina, linus.walleij,
broonie, gregkh, hdegoede, james.clark, james, keescook,
petr.tesarik.ext, rafael, tglx, Jeff LaBundy, linux-input,
devicetree, linux-arm-kernel, linux-mediatek, linux-kernel,
Johan Hovold
In-Reply-To: <20231109100606.1245545-1-wenst@chromium.org>
Hi,
On Thu, Nov 9, 2023 at 2:06 AM Chen-Yu Tsai <wenst@chromium.org> wrote:
>
> The driver as
> implemented currently doesn't deal with regulators or GPIO pins,
> since in the existing device trees they are either always on for
> regulators, or have GPIO hogs or pinmux and pinconfig directly
> tied to the pin controller.
I guess I won't object too much about this limitation for v1, but IMO
it would be good to get this sorted out since I think part of the
power of having the HW Prober is specifically that it can handle this
type of use case. You have a little bit of board-specific code that
knows how to turn on the regulators / GPIOs and can then probe the
devices.
Note: even if this is "board specific", it doesn't mean you couldn't
share code between boards. For instance, you could have a helper
function that would turn on regulators/GPIOs based on some type of
table and that helper function could be used across a whole pile of
Chromebooks. If a Chromebook is sufficiently different that it
couldn't use the helper function then it could call its own function,
but presumably it wouldn't be hard to support a bunch of boards
without much code.
As part of this, I think that your main "HW Prober" for Chromebooks
should be in "drivers/platform/chrome/". I think that the only things
that should be in the "drivers/of" directory should be helper
functions used by the Chromebook HW Probers.
-Doug
^ permalink raw reply
* Re: [RFC PATCH v2 0/7] of: Introduce hardware prober driver
From: Doug Anderson @ 2023-11-11 0:12 UTC (permalink / raw)
To: Rob Herring
Cc: AngeloGioacchino Del Regno, Chen-Yu Tsai, Frank Rowand,
Krzysztof Kozlowski, Conor Dooley, Matthias Brugger, Hsin-Yi Wang,
Dmitry Torokhov, andriy.shevchenko, Jiri Kosina, linus.walleij,
broonie, gregkh, hdegoede, james.clark, james, keescook,
petr.tesarik.ext, rafael, tglx, Jeff LaBundy, linux-input,
devicetree, linux-arm-kernel, linux-mediatek, linux-kernel,
Johan Hovold
In-Reply-To: <CAL_JsqK64w3+r_LJZoh50PzAUcsvH6ahSDCqgSiKrD3LBAXE9g@mail.gmail.com>
Hi,
On Thu, Nov 9, 2023 at 5:52 AM Rob Herring <robh+dt@kernel.org> wrote:
>
> > > End of background from Doug's cover letter.
> >
> > I think that using "status" is not a good idea, I find that confusing.
>
> "status" is what defines a device's state in terms of enabled,
> present, available. That's exactly what we're expressing here.
>
> Now, I do not think we should be mixing the device class (e.g.
> touchscreen) into status. I said this on v1, but apparently that was
> not listened to.
Interesting. I must have missed the "don't mix device class into
status" part. Do you have a link to your post about that? Maybe
there's other stuff I missed... Having the device class stuck at the
end there was at least part of my last post [1] which gathered no
response.
I think one of the reasons that I felt we needed to mux the device
class into status was that it was going to make the code a lot less
fragile. Everything I've seen indicates that you don't want us to
create a "HW prober" node that could be used to provide relevant
phandles for different classes of devices, so the "HW prober" code
needs to either search through the whole device tree for a status of
"failed-needs-probe" or needs to contain per-board, hardcoded,
fully-qualified paths.
I don't think we want to include hardcoded, fully-qualified paths in
the code. That would mean that if someone changed a node name
somewhere in the path to one of the devices that we're dealing with
then it would break.
So if we're searching the whole device tree for "failed-needs-probe"
then we need to figure out which devices are related to each other. If
a given board has second sources for MIPI panels, touchscreens, and
trackpads then we need to know which of the "failed-needs-probe"
devices are trackpads, which are touchscreens, and which are MIPI
panels. Do you have any suggestions for how we should do that? Maybe
it was in some other thread that I missed? I guess we could have a
board-specific table mapping (compatible + node name + reg) to a
class, but that feels awkward.
[1] https://lore.kernel.org/r/CAD=FV=UjVAgT-febtj4=UZ2GQp01D-ern2Ff9+ODcHeQBOsdTQ@mail.gmail.com
^ permalink raw reply
* Re: [RFC PATCH v2 5/7] of: hw_prober: Support Chromebook SKU ID based component selection
From: Rob Herring @ 2023-11-10 21:07 UTC (permalink / raw)
To: Chen-Yu Tsai
Cc: Frank Rowand, Krzysztof Kozlowski, Conor Dooley, Matthias Brugger,
AngeloGioacchino Del Regno, Hsin-Yi Wang, Dmitry Torokhov,
andriy.shevchenko, Jiri Kosina, linus.walleij, broonie, gregkh,
hdegoede, james.clark, james, keescook, petr.tesarik.ext, rafael,
tglx, Jeff LaBundy, linux-input, devicetree, linux-arm-kernel,
linux-mediatek, linux-kernel, Douglas Anderson, Johan Hovold
In-Reply-To: <20231109100606.1245545-6-wenst@chromium.org>
On Thu, Nov 09, 2023 at 06:06:02PM +0800, Chen-Yu Tsai wrote:
> In cases where the same Chromebook model is manufactured with different
> components (MIPI DSI panels, MIPI CSI camera sensors, or trackpad /
> touchscreens with conflicting addresses), a different SKU ID is
> allocated to each specific combination. This SKU ID is exported by the
> bootloader into the device tree, and can be used to "discover" which
> combination is present on the current machine.
>
> This change adds a hardware prober that will match the SKU ID against
> a provided table, and enable the component for the matched entry based
> on the given compatible string. In the MIPI DSI panel and MIPI CSI
> camera sensor cases which have OF graphs, it will also update the
> remote endpoint to point to the enabled component. This assumes a single
> endpoint only.
>
> This will provide a path to reducing the number of Chromebook device
> trees.
>
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> ---
> drivers/of/hw_prober.c | 160 +++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 160 insertions(+)
This certainly does not belong in drivers/of/.
> diff --git a/drivers/of/hw_prober.c b/drivers/of/hw_prober.c
> index 442da6eff896..4345e5aed6d8 100644
> --- a/drivers/of/hw_prober.c
> +++ b/drivers/of/hw_prober.c
> @@ -8,6 +8,7 @@
> #include <linux/array_size.h>
> #include <linux/i2c.h>
> #include <linux/of.h>
> +#include <linux/of_graph.h>
> #include <linux/platform_device.h>
>
> #define DRV_NAME "hw_prober"
> @@ -108,9 +109,168 @@ static int i2c_component_prober(struct platform_device *pdev, const void *data)
> return ret;
> }
>
> +static int cros_get_coreboot_sku_id(struct device *dev, u32 *sku_id)
> +{
> + struct device_node *node = NULL;
> + int ret;
> +
> + node = of_find_node_by_path("/firmware/coreboot");
> + if (!node)
> + return dev_err_probe(dev, -EINVAL, "Cannot find coreboot firmware node\n");
> +
> + ret = of_property_read_u32(node, "sku-id", sku_id);
Not documented.
Rob
^ permalink raw reply
* Re: [RFC PATCH v2 6/7] dt-bindings: arm: mediatek: Remove SKU specific compatibles for Google Krane
From: Rob Herring @ 2023-11-10 21:04 UTC (permalink / raw)
To: Chen-Yu Tsai
Cc: Frank Rowand, Krzysztof Kozlowski, Conor Dooley, Matthias Brugger,
AngeloGioacchino Del Regno, Hsin-Yi Wang, Dmitry Torokhov,
andriy.shevchenko, Jiri Kosina, linus.walleij, broonie, gregkh,
hdegoede, james.clark, james, keescook, petr.tesarik.ext, rafael,
tglx, Jeff LaBundy, linux-input, devicetree, linux-arm-kernel,
linux-mediatek, linux-kernel, Douglas Anderson, Johan Hovold
In-Reply-To: <20231109100606.1245545-7-wenst@chromium.org>
On Thu, Nov 09, 2023 at 06:06:03PM +0800, Chen-Yu Tsai wrote:
> In cases where the same Chromebook model is manufactured with different
> components (MIPI DSI panels, MIPI CSI camera sensors, or trackpad /
> touchscreens with conflicting addresses), a different SKU ID is
> allocated to each specific combination. This SKU ID is exported by the
> bootloader into the device tree, and can be used to "discover" which
> combination is present on the current machine. Thus we no longer have
> to specify separate compatible strings for each of them.
You just broke an existing kernel with a new DT having this change.
Just because you come up with a new way to do things, doesn't mean you
can remove the old way.
>
> Remove the SKU specific compatible strings for Google Krane.
>
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> ---
> Documentation/devicetree/bindings/arm/mediatek.yaml | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/arm/mediatek.yaml b/Documentation/devicetree/bindings/arm/mediatek.yaml
> index a4541855a838..ef3dfb286814 100644
> --- a/Documentation/devicetree/bindings/arm/mediatek.yaml
> +++ b/Documentation/devicetree/bindings/arm/mediatek.yaml
> @@ -186,9 +186,6 @@ properties:
> - const: mediatek,mt8183
> - description: Google Krane (Lenovo IdeaPad Duet, 10e,...)
> items:
> - - enum:
> - - google,krane-sku0
> - - google,krane-sku176
> - const: google,krane
> - const: mediatek,mt8183
> - description: Google Willow (Acer Chromebook 311 C722/C722T)
> --
> 2.42.0.869.gea05f2083d-goog
>
^ permalink raw reply
* Re: PSA: This list is being migrated (no action required)
From: Konstantin Ryabitsev @ 2023-11-10 19:35 UTC (permalink / raw)
To: linux-embedded, linux-ext4, linux-fbdev, linux-fpga,
linux-fscrypt, linux-gcc, linux-gpio, linux-hams, linux-hexagon,
linux-hotplug, linux-hwmon, linux-i2c, linux-ia64, linux-ide,
linux-iio, linux-input, linux-integrity, linux-kbuild,
linux-kselftest, linux-leds, linux-m68k, linux-man, linux-media,
linux-mips, linux-mmc, linux-msdos
In-Reply-To: <cfriwrxovqzcrptf74ccq52lcqj2nsergucufsz6wlh45fdnz3@z5e5y2lowbq2>
On Fri, Nov 10, 2023 at 01:51:44PM -0500, Konstantin Ryabitsev wrote:
> This list is being migrated to new vger infrastructure. No action is required
> on your part and there will be no change in how you interact with this list
> after the migration is completed.
>
> There will be a short 30-minute delay to the list archives on lore.kernel.org.
> Once the backend work is done, I will follow up with another message.
This work is completed now. This message acts as a test to make sure archives
are working at their new place.
If anything is not working or looking right, please reach out to
helpdesk@kernel.org.
-K
^ permalink raw reply
* PSA: This list is being migrated (no action required)
From: Konstantin Ryabitsev @ 2023-11-10 18:51 UTC (permalink / raw)
To: linux-embedded, linux-ext4, linux-fbdev, linux-fpga,
linux-fscrypt, linux-gcc, linux-gpio, linux-hams, linux-hexagon,
linux-hotplug, linux-hwmon, linux-i2c, linux-ia64, linux-ide,
linux-iio, linux-input, linux-integrity, linux-kbuild,
linux-kselftest, linux-leds, linux-m68k, linux-man, linux-media,
linux-mips, linux-mmc, linux-msdos
Hello, all:
This list is being migrated to new vger infrastructure. No action is required
on your part and there will be no change in how you interact with this list
after the migration is completed.
There will be a short 30-minute delay to the list archives on lore.kernel.org.
Once the backend work is done, I will follow up with another message.
-K
^ permalink raw reply
* Re: [PATCH v5 1/3] dt-bindings: input: microchip,cap11xx: add advanced sensitivity settings
From: Krzysztof Kozlowski @ 2023-11-10 8:22 UTC (permalink / raw)
To: Jiri Valek - 2N, krzysztof.kozlowski+dt, dmitry.torokhov
Cc: devicetree, linux-input, linux-kernel, robh+dt, u.kleine-koenig
In-Reply-To: <20231108155647.1812835-2-jiriv@axis.com>
On 08/11/2023 16:56, Jiri Valek - 2N wrote:
> Add support for advanced sensitivity settings and signal guard feature.
>
> Signed-off-by: Jiri Valek - 2N <jiriv@axis.com>
> ---
> .../bindings/input/microchip,cap11xx.yaml | 76 ++++++++++++++++++-
> 1 file changed, 73 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/input/microchip,cap11xx.yaml b/Documentation/devicetree/bindings/input/microchip,cap11xx.yaml
> index 5b5d4f7d3482..aa97702c43ef 100644
> --- a/Documentation/devicetree/bindings/input/microchip,cap11xx.yaml
> +++ b/Documentation/devicetree/bindings/input/microchip,cap11xx.yaml
> @@ -45,13 +45,13 @@ properties:
> Enables the Linux input system's autorepeat feature on the input device.
>
> linux,keycodes:
> - minItems: 6
> - maxItems: 6
> + minItems: 3
> + maxItems: 8
> description: |
> Specifies an array of numeric keycode values to
> be used for the channels. If this property is
> omitted, KEY_A, KEY_B, etc are used as defaults.
> - The array must have exactly six entries.
> + The number of entries must correspond to the number of channels.
>
> microchip,sensor-gain:
> $ref: /schemas/types.yaml#/definitions/uint32
> @@ -70,6 +70,55 @@ properties:
> open drain. This property allows using the active
> high push-pull output.
>
> + microchip,sensitivity-delta-sense:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + default: 32
> + enum: [1, 2, 4, 8, 16, 32, 64, 128]
> + description:
> + Optional parameter. Controls the sensitivity multiplier of a touch detection.
> + At the more sensitive settings, touches are detected for a smaller delta
Which values are more sensitive?
> + capacitance corresponding to a “lighter” touch.
Looks like you use some non-ASCII characters for ".
> +
> + microchip,signal-guard:
> + $ref: /schemas/types.yaml#/definitions/uint32-array
> + minItems: 3
> + maxItems: 8
> + items:
> + minimum: 0
> + maximum: 1
> + description: |
> + Optional parameter supported only for CAP129x.
> + 0 - off
> + 1 - on
> + The signal guard isolates the signal from virtual grounds.
> + If enabled then the behavior of the channel is changed to signal guard.
> + The number of entries must correspond to the number of channels.
> +
> + microchip,input-treshold:
typo: threshold
> + $ref: /schemas/types.yaml#/definitions/uint32-array
> + minItems: 3
> + maxItems: 8
> + items:
> + minimum: 0
> + maximum: 127
> + description:
> + Optional parameter. Specifies the delta threshold that is used to
Drop everywhere the "optional parameter". It's redundant. required:
block tells what is / is not optional.
> + determine if a touch has been detected.
In what units are the values?
> + The number of entries must correspond to the number of channels.
> +
> + microchip,calib-sensitivity:
> + $ref: /schemas/types.yaml#/definitions/uint32-array
> + minItems: 3
> + maxItems: 8
> + items:
> + minimum: 1
> + maximum: 4
> + description:
> + Optional parameter supported only for CAP129x. Specifies an array of
> + numeric values that controls the gain used by the calibration routine to
> + enable sensor inputs to be more sensitive for proximity detection.
Gain is usually in dB, isn't it?
> + The number of entries must correspond to the number of channels.
> +
> patternProperties:
> "^led@[0-7]$":
Best regards,
Krzysztof
^ permalink raw reply
* Re: Requesting your attention and expertise regarding a Tablet/Kernel issue
From: Benjamin Tissoires @ 2023-11-10 10:19 UTC (permalink / raw)
To: David Revoy
Cc: José Expósito, Eric GOUYER, Illia Ostapyshyn, jkosina,
jason.gerecke, linux-input, linux-kernel, Peter Hutterer
In-Reply-To: <7wmtNlKuYResf5cFQ7M2QTalzIUtw0I6ohvPcz69Jo1c8flezyIlnJu1IwAgXhJ-u0NlRL3IV7HnL0Kza6fVBqd7X7jhc-Z6QCi3oqHEvpY=@protonmail.com>
Hi David,
On Thu, Nov 9, 2023 at 11:04 PM David Revoy <davidrevoy@protonmail.com> wrote:
>
> Hi Benjamin,
>
> Thank you it works! 🎉 🎉 🎉
\o/ /o\ \o/
>
> > I've pushed an update of the file[0], turns out I made several mistakes.
> > As a general rule of thumb, you can follow the MR I've opened at [1],
> > click on the pipeline, open the last job ("make release"), then browse
> > the artifacts and pull the file from there.
> > [...]
> > > But just to be sure, you don't have a custom configuration in place
> > > for that tablet device?
> > [...]
> > [0] https://gitlab.freedesktop.org/libevdev/udev-hid-bpf/-/jobs/51399392/artifacts/file/udev-hid-bpf_0.1.0.tar.xz
> > [1] https://gitlab.freedesktop.org/libevdev/udev-hid-bpf/-/merge_requests/27
>
> I tested the latest artifact on kernel 6.5.8-200.fc38.x86_64 and I also removed my custom configuration at startup (I had not much: an hwdb files for the 24 Pro −mainly for frame buttons− and an xsetwacom bash script for each tablet).
>
> During the tests, the styluses of both 24 Pro and 16 Pro Gen2 worked perfectly: right-click on upper button out-of-the-box, and the eraser tip of the 16 Pro Gen2 continued to erase as expected.
>
> I could also target with xsetwacom this 'button 3' of the styluses, and I tested random available shortcuts (but I'll keep default right-click).
>
> So, good job, and many thanks!
>
> I want now to write a follow-up after the first blog-post. I see it is a MR [1], maybe it means if it get merged it will be part of libevdev? What would you advice to write for the ones who want to benefit from the fix?
This MR will be merged into udev-hid-bpf. It's not "libevdev", the
library, per se, but we piggyback on the libevdev project some tools
that are more or less official. Right now, udev-hid-bpf is not
integrated in any distribution. It's a relatively new project.
As for the blog post, feel free to copy/extract/rewrite/link the
following. I've tried to address the couple of elements that worried
me on your initial blog post (mostly the why), and gave a rough
overview of what we have done here:
---
Here is a little bit of history of why you were encountering this bug:
First, HID is a quite old protocol and has the benefit of being "plug
and play" [0] [1].
But plug and play often means for a hardware maker: "let's do trial
and error until Windows seems to behave in a correct way".
In some other cases, Microsoft puts more restrictions on the HID part
(Windows 7 enforced touchscreens to follow a specific pattern, and
then Windows 8 did it for touchpads). And they also sometimes provide
a test suite that hardware makers have to pass to be "certified".
They have to pass the test suite by using the Windows provided generic
driver, but Windows also allows them to create a virtual HID device
and let a custom driver create that virtual HID device. Which means,
we sometimes have devices behaving badly but working perfectly fine on
Windows because there are bits missing in the device itself that are
fixed by adding an extra software layer. Sigh.
But I digress and we need to go back to the pen bits, and AFAIK, there
is no such test suite and certification.
So basically, hardware makers follow the empiric rule of "if Windows
is happy, I am too".
To do so, they have to use several events from HID [2] (quoting them):
- *Tip Switch* -> A switch located at the tip of a stylus, indicating
contact of the stylus with a surface. A pen-based system or system
extension would use this switch to enable the input of handwriting or
gesture data. The system typically maps Tip Switch to a primary button
in a non-pen context.
- *In Range* -> Indicates that a transducer is located within the
region where digitizing is possible. In Range is a bit quantity
- *Barrel Switch* -> A non-tip button located on the barrel of a
stylus. Its function is typically mapped to a system secondary button
or to a Shift key modifier that changes the Tip Switch function from
primary button to secondary button.
- *Secondary Barrel Switch* -> A second non-tip button located on the
barrel of a stylus further from the tip than the Barrel Switch. Its
function is typically mapped to a system secondary or tertiary button.
- *Eraser* -> This control is used for erasing objects. Following the
metaphor of a pencil, it is typically located opposite the writing end
of a stylus. It may be a bit switch or a pressure quantity.
- *Invert* -> A bit that indicates that the currently sensed position
originates from the end of a stylus opposite the tip.
I'm sure that by reading those, everybody should be able to
immediately know how to write a Pen HID device, and how the
interactions between the events should be. :) (If you are, please
contact me ASAP, we have plenty of kernel work to do).
So for years the state of pen devices in the Linux kernel was 2 fold:
- Wacom shipped an in-kernel driver for their own devices, that they
tested and that defined the de-facto "standard" in Linux
- the rest was trying to catch up by luck or with the help of projects
like DiGiMend, by relying on the generic HID processing of the Linux
kernel
However, they were no specification for how the events should come:
basically in the hid generic input processing each event was mapped to
a single input keycode and we had situations were we would get both
`BTN_TOOL_PEN` and `BTN_TOOL_ERASER` at the same time (because the `In
Range` and the `Eraser` bits were sent by the device at the same
time). Which means "hey, the user is interacting with a pen with both
the tail and the tip at the same time. Good luck with that!"
This led to a complex situation where userspace (libinput mostly) had
to do guesses on what is the intent of the user. But the problem is
that when you are in userspace, you don't know all of the events of
the device at the same time, so it was messy to deal with. Again,
Wacom devices were unaffected because they controlled all of the
stack: a kernel driver to fix/interpret their devices and a userspace
component, xf86-drv-wacom, in the X.org world.
Once, as you mentioned in your blog, Microsoft decided to use the
second barrel button as the "rubber" mode. The reason was practical:
people liked the rubber capability on the styluses, but they wanted to
have a separate button on the tail end of the styluses. And I suppose
that at the time, given that no other hardware vendors were capable of
having no-battery styluses but Wacom (IP protection and capabilities
of the hardware IIRC), you still had to put the battery somewhere. And
that tail end is convenient for storing such a battery. But that makes
it harder to have an eraser end because you need to link both ends of
the pen on the same IC, with a battery in the middle that is roughly
the same size as your pen's barrel. So having just 2 wires for the
battery allows you to have a separate bluetooth button on one end, and
the normal stylus on the other end, and keep the width of the pen
reasonable.
So that choice of using the second button as an eraser was made, and
the hardware makers followed: on the XP-Pen Artist Pro 24, the device
reports `Tip Switch`, `Barrel Switch`, `Eraser`, `In Range`.
Which is "correct" according to the HID Usage Table [2], but which
doesn't adhere to the recommendation Microsoft is doing [3]: the
device should report an extra `Invert` when the pen is in range with
the intent to erase...
But you can see that XP-Pen tried to adhere to it in some ways because
if you look carefully at the events coming from the device with
hid-recorder[4], you'll notice that when you are in range of the
sensor and press this button, you'll get an extra "In Range = 0" event
to notify that you went out of proximity of the sensor.
In kernel 5.18, with commit 87562fcd1342 ("HID: input: remove the need
for HID_QUIRK_INVERT"), I tried to remove those multiple tool states
to have a straightforward state provided by the kernel that userspace
can deal with easily. However, given that there were no regression
tests at the time for generic tablets, I wrote some based on
Microsoft's recommendation [3] and also tested on a Huion device I
have locally. And it was working fine. But I didn't have the devices
that were not sending `Invert`, which explained why it was bogus on
those devices.
This was "fixed" in kernel 6.6 with commit 276e14e6c399 ("HID: input:
Support devices sending Eraser without Invert"). Putting quotes around
"fixed" because I'm still not happy about this patch.
But the point is, from kernel 5.18, the Pen processing in the kernel
became a state machine, which means we can not have external actors
tampering with it.
Why using the ioctl EVIOCSKEYCODE is bad to remap `Eraser` to
`BTN_STYLUS2` (through tools like evmap):
Having the ability to do something doesn't mean it's the right thing
to do. And in that case, this is definitely wrong because you have to
call the ioctl after the kernel presents the device to userspace.
Which means userspace (and the kernel) already made assumptions on the
device itself. There is a high chance libinput (or the Wacom driver)
opens the device before evmap, and that it is considering that the
device doesn't have `BTN_STYLUS2`. So sending that event would break
userspace.
And in our case here, the kernel expects some state between the input
layer and its internal HID state, and remapping that HID event to
something else confuses it.
There is another side effect of this: usually end users configuring
their devices with such tools do not report back their configuration
to the kernel community. In some cases this is valid (this is my
preference and my choice), but in other cases it's not (there is a bug
here and I'm papering over it).
So, what can be done?
Basically 2 options:
1. write a kernel patch to fix that problem once and for all
2. use the brand new HID-BPF[5] capability introduced in kernel v6.3
and send me back the BPF program so I can eventually integrate the
source in the kernel tree itself and fix that problem once and for all
as well
For 1., you need:
- to be able to dig into the kernel code
- to be able to write a patch with the correct kernel standard (with a
regression test in `tools/testing/selftests/hid`, please)
- to be able to compile your own kernel and test it
- to be able to submit your contribution by email (I can suggest using
b4 for that, very nice tool)
- to be able to take reviews into account, and learn `git rebase -i`
to submit v2, v3, and potentially v10 or more in some complex cases
- to wait for the patch to be integrated into Linus' tree
- to wait for Greg to backport your patch into a stable kernel tree
- to wait for your distribution to pick up the stable tree with your patch
That's relatively easy, no? :)
OTOTH, we have 2.: HID-BPF [5]
Very quickly, eBPF [6] is a state machine inside the kernel that
allows user space to include a verified code path in the kernel to
tweak its behavior. And I adapted this for HID so you can:
- change the report descriptor of the device: this
disconnects/reconnects the device, meaning the kernel works on the new
report descriptor and is consistent with its state
- change the event flow of the device: to fix the spurious out-of-prox
event for example
- more to come
What is interesting in BPF (or eBPF), is that nowadays, libbpf
implements something named CORE (Compile Once Run Everywhere). Which
means that if I compile locally an eBPF program on my machine with my
development kernel, as long as I only use functions available from
kernel v6.3 for instance, the same compilation output (that changes
the event flow of your HID device) will work on any kernel from v6.3
unless there are some later API breakages[7].
Which means, anybody can modify the event flow of an HID device, put
the file in the filesystem, and have the device still fixed even if
they upgrade their kernel.
In the long run, I intend to include those HID-BPF fixes in the kernel
tree to centralize them, but also to be able to automatically load
them from the kernel when those devices appear.
Which means, for the reporter of such a bug you:
- can now rely on someone else to write the code, compile it and
provide the compilation result [10]
- just put that result in the filesystem to have the device tested and fixed
Behind the scenes, that other knowledgeable person can take the heavy
task of submitting the kernel patch for you, but given that the code
has been tested, it's way easier to do (and to eventually re-test).
Currently, the "let's integrate that bpf program in the kernel" is not
completely done, so we use udev-hid-bpf[8][9] to give it a jump start.
And that's exactly what happened in your case David. Which is why I'm
so happy (also because I fixed the tools from an author I like and
already had the books at home :-P):
You want your device to be fixed now, but going through a regular
kernel patch means months before it's fixed in your distribution.
But with HID-BPF, I fixed it now, and you can safely upgrade the
kernel, because even if I do changes in the kernel, the HID-BPF fix
will still convert the device into something valid from the HID point
of view, and it has a regression test now. When your device will be
fixed in the future in the kernel, there is a high chance the `probe`
function of the HID-BPF program will say that it's not the correct
device, and so the program will not even load and rely on the fixed
kernel only. Transparently for you, without you having to change your
filesystem.
On my side, what's left to be done:
- First, I need to fix the tablets not sending the `Invert` usage.
Commit 276e14e6c399 ("HID: input: Support devices sending Eraser
without Invert") is IMO not good enough, and we might as well simply
say that if there is no `Invert` usage, we can convert the `Eraser`
usage into `Secondary Barrel Switch`
- then I need to fix the XP-Pen Artist Pro 16 gen 2 from the kernel
too, by replacing the `Eraser` usage with `Secondary Barrel Switch`.
Ideally I would just dump the HID-BPF program in the kernel, but this
is not available yet, so I'll probably write a small kernel driver
using the same code path as the HID-BPF program.
- then Peter and I need to write a more generic HID-BPF program to
convert "eraser mode buttons" into `Secondary Barrel Switch`,
basically unwinding what the hardware does. This can only happen when
libinput will be able to do the opposite transformation so we don't
regress. But we can rely on libwacom to tell us if this pen has a tail
end eraser or not, and then have userspace choose if they want the
button to be a button, or an eraser mode.
I think that's pretty much it.
Thanks for reading through everything :)
Cheers,
Benjamin
[0] https://who-t.blogspot.com/2018/12/understanding-hid-report-descriptors.html
[1] https://docs.kernel.org/hid/hidintro.html
[2] https://usb.org/sites/default/files/hut1_4.pdf
[3] https://learn.microsoft.com/en-us/windows-hardware/design/component-guidelines/windows-pen-states
[4] https://gitlab.freedesktop.org/libevdev/hid-tools
[5] https://docs.kernel.org/hid/hid-bpf.html
[6] https://docs.kernel.org/bpf/index.html
[7] but if API breakage happens, all that will happen is that the
HID-BPF program will not be loaded. No kernel crash involved.
[8] https://gitlab.freedesktop.org/libevdev/udev-hid-bpf
[9] https://libevdev.pages.freedesktop.org/udev-hid-bpf/tutorial.html
[10] https://gitlab.freedesktop.org/libevdev/udev-hid-bpf/-/merge_requests/27
>
> Thanks again,
> David
>
> [1] https://gitlab.freedesktop.org/libevdev/udev-hid-bpf/-/merge_requests/27
>
^ permalink raw reply
* [PATCH V1] Input: synaptics-rmi4 - Enable support for F3A by default.
From: Marge Yang @ 2023-11-10 8:21 UTC (permalink / raw)
To: dmitry.torokhov, linux-input, linux-kernel, marge.yang
Cc: david.chiu, derek.cheng, sam.tsai, vincent.huang
RMI4 F3A supports the touchpad GPIO function, it's designed to
support more GPIOs and used on new touchpad. This patch will
enable support of touchpad button.
Signed-off-by: Marge Yang <marge.yang@tw.synaptics.com>
---
drivers/hid/Kconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index e11c1c8..f3a989e 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -1082,6 +1082,7 @@ config HID_RMI
select RMI4_F11
select RMI4_F12
select RMI4_F30
+ select RMI4_F3A
help
Support for Synaptics RMI4 touchpads.
Say Y here if you have a Synaptics RMI4 touchpads over i2c-hid or usbhid
--
2.7.4
^ permalink raw reply related
* Your Order Update843541926
From: Thank You#8999071396 @ 2023-11-10 12:40 UTC (permalink / raw)
To: linux-input
[-- Attachment #1: Type: text/plain, Size: 185 bytes --]
Please feel free to reach out again if you have any questions. We have
successfully processed your request to change delivery address. You
have received the confirmation email as well.
[-- Attachment #2: Bill843541926.pdf --]
[-- Type: application/octet-stream, Size: 277724 bytes --]
^ permalink raw reply
* Re: [git pull] Input updates for v6.7-rc0
From: pr-tracker-bot @ 2023-11-09 22:28 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Linus Torvalds, linux-kernel, linux-input
In-Reply-To: <ZUyey2l2Cfri8715@google.com>
The pull request you sent on Thu, 9 Nov 2023 00:56:43 -0800:
> git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git tags/input-for-v6.7-rc0
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/a12deb44f9734dc25970c266249b272e44d3d1b5
Thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html
^ permalink raw reply
* Re: Requesting your attention and expertise regarding a Tablet/Kernel issue
From: David Revoy @ 2023-11-09 22:04 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: José Expósito, Eric GOUYER, Illia Ostapyshyn, jkosina,
jason.gerecke, linux-input, linux-kernel
In-Reply-To: <CAO-hwJKJW5jGDdaaS8eB7kcLQUvWO_1XkOzJG4HAcaRzw1cGnQ@mail.gmail.com>
Hi Benjamin,
Thank you it works! 🎉 🎉 🎉
> I've pushed an update of the file[0], turns out I made several mistakes.
> As a general rule of thumb, you can follow the MR I've opened at [1],
> click on the pipeline, open the last job ("make release"), then browse
> the artifacts and pull the file from there.
> [...]
> > But just to be sure, you don't have a custom configuration in place
> > for that tablet device?
> [...]
> [0] https://gitlab.freedesktop.org/libevdev/udev-hid-bpf/-/jobs/51399392/artifacts/file/udev-hid-bpf_0.1.0.tar.xz
> [1] https://gitlab.freedesktop.org/libevdev/udev-hid-bpf/-/merge_requests/27
I tested the latest artifact on kernel 6.5.8-200.fc38.x86_64 and I also removed my custom configuration at startup (I had not much: an hwdb files for the 24 Pro −mainly for frame buttons− and an xsetwacom bash script for each tablet).
During the tests, the styluses of both 24 Pro and 16 Pro Gen2 worked perfectly: right-click on upper button out-of-the-box, and the eraser tip of the 16 Pro Gen2 continued to erase as expected.
I could also target with xsetwacom this 'button 3' of the styluses, and I tested random available shortcuts (but I'll keep default right-click).
So, good job, and many thanks!
I want now to write a follow-up after the first blog-post. I see it is a MR [1], maybe it means if it get merged it will be part of libevdev? What would you advice to write for the ones who want to benefit from the fix?
Thanks again,
David
[1] https://gitlab.freedesktop.org/libevdev/udev-hid-bpf/-/merge_requests/27
^ permalink raw reply
* Re: [RFC PATCH v2 2/7] of: Introduce hardware prober driver
From: Andy Shevchenko @ 2023-11-09 17:54 UTC (permalink / raw)
To: Chen-Yu Tsai
Cc: Rob Herring, Frank Rowand, Krzysztof Kozlowski, Conor Dooley,
Matthias Brugger, AngeloGioacchino Del Regno, Hsin-Yi Wang,
Dmitry Torokhov, Jiri Kosina, linus.walleij, broonie, gregkh,
hdegoede, james.clark, james, keescook, petr.tesarik.ext, rafael,
tglx, Jeff LaBundy, linux-input, devicetree, linux-arm-kernel,
linux-mediatek, linux-kernel, Douglas Anderson, Johan Hovold
In-Reply-To: <20231109100606.1245545-3-wenst@chromium.org>
On Thu, Nov 09, 2023 at 06:05:59PM +0800, Chen-Yu Tsai wrote:
> Some devices are designed and manufactured with some components having
> multiple drop-in replacement options. These components are often
> connected to the mainboard via ribbon cables, having the same signals
> and pin assignments across all options. These may include the display
> panel and touchscreen on laptops and tablets, and the trackpad on
> laptops. Sometimes which component option is used in a particular device
> can be detected by some firmware provided identifier, other times that
> information is not available, and the kernel has to try to probe each
> device.
>
> This change attempts to make the "probe each device" case cleaner. The
> current approach is to have all options added and enabled in the device
> tree. The kernel would then bind each device and run each driver's probe
> function. This works, but has been broken before due to the introduction
> of asynchronous probing, causing multiple instances requesting "shared"
> resources, such as pinmuxes, GPIO pins, interrupt lines, at the same
> time, with only one instance succeeding. Work arounds for these include
> moving the pinmux to the parent I2C controller, using GPIO hogs or
> pinmux settings to keep the GPIO pins in some fixed configuration, and
> requesting the interrupt line very late. Such configurations can be seen
> on the MT8183 Krane Chromebook tablets, and the Qualcomm sc8280xp-based
> Lenovo Thinkpad 13S.
>
> Instead of this delicate dance between drivers and device tree quirks,
> this change introduces a simple I2C component prober. For any given
> class of devices on the same I2C bus, it will go through all of them,
> doing a simple I2C read transfer and see which one of them responds.
> It will then enable the device that responds.
>
> This requires some minor modifications in the existing device tree.
> The status for all the device nodes for the component options must be
> set to "failed-needs-probe-xxx". This makes it clear that some mechanism
> is needed to enable one of them, and also prevents the prober and device
> drivers running at the same time.
...
> +config HW_PROBER
config OF_HW_PROBER // or anything with explicit OF
Don't give a false impression that this is something that may works without
OF support.
...
> + bool "Hardware Prober driver"
Ditto.
...
> +/*
> + * hw_prober.c - Hardware prober driver
Do not include filename into the file itself.
> + *
> + * Copyright (c) 2023 Google LLC
> + */
...
> + node = of_find_node_by_name(NULL, node_name);
> + if (!node)
> + return dev_err_probe(&pdev->dev, -ENODEV, "Could not find %s device node\n",
> + node_name);
With
struct device *dev = &pdev->dev;
this and other lines can be made neater.
...
For better maintenance it's good to have ret assignment be placed here
ret = 0;
> + for_each_child_of_node(i2c_node, node) {
> + struct property *prop;
> + union i2c_smbus_data data;
> + u32 addr;
> +
> + if (!of_node_name_prefix(node, node_name))
> + continue;
> + if (of_property_read_u32(node, "reg", &addr))
> + continue;
> + if (i2c_smbus_xfer(i2c, addr, 0, I2C_SMBUS_READ, 0, I2C_SMBUS_BYTE, &data) < 0)
> + continue;
> +
> + dev_info(&pdev->dev, "Enabling %pOF\n", node);
> +
> + prop = kzalloc(sizeof(*prop), GFP_KERNEL);
> + if (!prop) {
> + ret = -ENOMEM;
> + of_node_put(node);
> + break;
> + }
> +
> + prop->name = "status";
> + prop->length = 5;
> + prop->value = "okay";
> +
> + /* Found a device that is responding */
> + ret = of_update_property(node, prop);
> + if (ret)
> + kfree(prop);
> +
> + of_node_put(node);
> + break;
> + }
...
> +static const struct hw_prober_entry hw_prober_platforms[] = {
> + { .compatible = "google,hana", .prober = i2c_component_prober, .data = "touchscreen" },
> + { .compatible = "google,hana", .prober = i2c_component_prober, .data = "trackpad" },
> +};
Why can't OF ID table be used for this?
...
> + for (int i = 0; i < ARRAY_SIZE(hw_prober_platforms); i++)
unsigned?
> + if (of_machine_is_compatible(hw_prober_platforms[i].compatible)) {
> + int ret;
> +
> + ret = hw_prober_platforms[i].prober(pdev, hw_prober_platforms[i].data);
> + if (ret)
> + return ret;
> + }
...
> + pdev = platform_device_register_simple(DRV_NAME, -1, NULL, 0);
-1 is defined in the header, use that definition.
> + if (!IS_ERR(pdev))
> + return 0;
> +
> + platform_driver_unregister(&hw_prober_driver);
> +
> + return PTR_ERR(pdev);
Can you use standard pattern, i.e. checking for the _error_ condition?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: Requesting your attention and expertise regarding a Tablet/Kernel issue
From: Benjamin Tissoires @ 2023-11-09 16:13 UTC (permalink / raw)
To: David Revoy
Cc: José Expósito, Eric GOUYER, Illia Ostapyshyn, jkosina,
jason.gerecke, linux-input, linux-kernel
In-Reply-To: <CAO-hwJ+zm=R7NwrALaLVmfPDtMNXpj0eoQgLkiS1wa6wd+hu+A@mail.gmail.com>
On Thu, Nov 9, 2023 at 12:56 PM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> Hi David,
>
> On Thu, Nov 9, 2023 at 1:32 AM David Revoy <davidrevoy@protonmail.com> wrote:
> >
> > Hi Benjamin,
> >
> > > Alright, I made quite some progress so far:
> > > - regressions tests have been written (branch wip/xp-pen of my fork on
> > > freedesktop[0])
> > > that branch can not go in directly as it just adds the tests, and
> > > thus is failing
> > > - I made the fixes through HID-BPF[1]
> > >
> > > Anyone using those 2 tablets and using Fedora should be able to just
> > > grab the artifact at [2], uncompress it and run `sudo ./install.sh --verbose`.
> > > This will install the bpf programs in /lib/firmware/hid/bpf/ and will
> > > automatically load them when the device is connected.
> > >
> > > For those not using Fedora, the binary might work (or not, not sure),
> > > but you can always decompress it, and check if running
> > > `udev-hid-bpf_0.1.0/bin/udev-hid-bpf --version` returns the correct
> > > version or just fails. If you get "udev-hid-bpf 0.1.0", then running
> > > `sudo ./install.sh --verbose` should work, as long as the kernel has
> > > CONFIG_HID_BPF set to 'Y'.
> > > [...]
> > > [0] https://gitlab.freedesktop.org/bentiss/hid/-/tree/wip/xp-pen?ref_type=heads
> > > [1] https://gitlab.freedesktop.org/libevdev/udev-hid-bpf/-/merge_requests/27
> > > [2] https://gitlab.freedesktop.org/bentiss/udev-hid-bpf/-/jobs/51350589/artifacts/raw/udev-hid-bpf_0.1.0.tar.xz
> >
> > Thank you for this package.
> >
> > I was able to test it even though the link in (2) at the bottom of your email returned a blank page. I was able to find my way after manually visiting gitlab.freedesktop.org [1] and then manually downloading the article from 51350589. I unzipped it and ran `sudo ./install.sh --verbose`. Everything looks like it was successful [2]. I then rebooted my Fedora 38 'Linux workstation 6.5.8-200.fc38.x86_64' kernel (the one I blamed in my post) and tested both tablets.
>
> Weird that you had to manually retrieve it. It works here, but maybe
> because I am logged in on gitlab.fd.o.
>
> Also, just FYI, you shouldn't have to reboot. Just unplug/replug and
> you are good. In the same way, if you uninstall the package, you can
> just unplug/replug to not have the programs loaded.
I've pushed an update of the file[0], turns out I made several mistakes.
As a general rule of thumb, you can follow the MR I've opened at [1],
click on the pipeline, open the last job ("make release"), then browse
the artifacts and pull the file from there.
>
> >
> > Here are my observation:
> >
> > XPPEN Artist Pro 24
> > ===================
> > Nothing changed for this device (it's the one with two buttons and no 'eraser tip'). Nor my hwdb/udev rules or `xsetwacom set "UGTABLET 24 inch PenDisplay eraser" button 1 3` affects the upper button of the stylus: if I hold it hover the canvas, Krita switch the tool and cursor for an eraser. If I click on the canvas with the pen tip while holding the upper button pressed, I get the right-click Pop-up Palette (but not all the time, probably Krita has hard time to triage Eraser or Right-click).
>
> As I mentioned in another reply, the more I think of it, the more I
> think I should get rid of the "eraser mode". In that Artist Pro 24 I
> can detect it through the same mechanics as the HID_QUIRK_NOINVERT
> from Illia's patch. But instead of trying to force the device into the
> eraser mode, we should just say "this is actually BUTTON_STYLUS_2".
>
> So I'm going to amend the bpf program to do this and hopefully you
> won't need the hwdb/udev rule at all.
I've fixed that one normally. There were a couple of issues:
- the PID in use was the one from the pro 16 gen2, which explained why
no change was appearing
- I've now decided to not export the second button as an eraser, as
mentioned above.
>
> >
> > XPPEN Artist Pro 16 (Gen2)
> > ==========================
> > Something changed. `xsetwacom set "UGTABLET Artist Pro 16 (Gen2) eraser" button 1 3` successfully affected the upper button of the stylus. Now if I click it while hovering the canvas, Krita shows the right click Pop-up Palette.
>
> I'm surprised you need to teach the wacom driver that BTN_STYLUS_2 is
> the right click.
>
> > On the downside; the real eraser tip when I flip the stylus bugs. When I flip the stylus on eraser hovering the canvas, Krita shows the Eraser icon and switch tool. As soon as I draw with the eraser tip, Krita will also show a right-click color palette and with also not a 100% consistency, as if the event were mixed.
>
> I'll investigate. Maybe I messed up with my event flow patch.
Definitely my mistake: both the bpf programs I wrote were attached to
the same device. Thus, the 2 fixes were stacking on each other,
leading to some interesting side effects.
You can check that the bpf are properly loaded by having a look at the
report descriptor when you replug the device:
if you see "Secondary Barrel Switch" at offset 16 instead of "Eraser"
on both of your tablets (with hid-recorder), you should have
successfully patched your devices.
Cheers,
Benjamin
>
> But just to be sure, you don't have a custom configuration in place
> for that tablet device?
>
[0] https://gitlab.freedesktop.org/libevdev/udev-hid-bpf/-/jobs/51399392/artifacts/file/udev-hid-bpf_0.1.0.tar.xz
[1] https://gitlab.freedesktop.org/libevdev/udev-hid-bpf/-/merge_requests/27
^ permalink raw reply
* Re: [PATCH] Add support for touch screens using the General Touch ST6001S controller.
From: Gareth Randall @ 2023-11-09 16:13 UTC (permalink / raw)
To: Benjamin Tissoires, Dmitry Torokhov; +Cc: linux-input
In-Reply-To: <4c792c2e-9de3-4faa-9948-47ddae77640e@virgin.net>
Dear Benjamin and Dmitry,
I wonder if you have had a chance to look at this patch. It was resubmitted on 3rd Oct 2023 then
again to make it appear in Patchwork properly. This is my first patch so any feedback on both the
patch and whether I'm following the process properly would be very useful.
Thanks very much.
Yours,
Gareth
On 25/10/2023 16:51, Gareth Randall wrote:
> Add support for touch screens using the General Touch ST6001S controller,
> as found in the GPEG model AOD22WZ-ST monitor. This controller can output
> the ELO 10-byte protocol, but requires different initialisation.
>
> Signed-off-by: Gareth Randall <gareth@garethrandall.com>
> ---
> drivers/input/touchscreen/elo.c | 81 ++++++++++++++++++++++++++++++++-
> 1 file changed, 80 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/input/touchscreen/elo.c b/drivers/input/touchscreen/elo.c
> index 96173232e53f..b233869ffa2a 100644
[snip]
^ permalink raw reply
* Re: [RFC PATCH v2 2/7] of: Introduce hardware prober driver
From: Rob Herring @ 2023-11-09 15:13 UTC (permalink / raw)
To: Chen-Yu Tsai
Cc: Frank Rowand, Krzysztof Kozlowski, Conor Dooley, Matthias Brugger,
AngeloGioacchino Del Regno, Hsin-Yi Wang, Dmitry Torokhov,
andriy.shevchenko, Jiri Kosina, linus.walleij, broonie, gregkh,
hdegoede, james.clark, james, keescook, rafael, tglx,
Jeff LaBundy, linux-input, devicetree, linux-arm-kernel,
linux-mediatek, linux-kernel, Douglas Anderson, Johan Hovold
In-Reply-To: <20231109100606.1245545-3-wenst@chromium.org>
On Thu, Nov 9, 2023 at 4:06 AM Chen-Yu Tsai <wenst@chromium.org> wrote:
>
> Some devices are designed and manufactured with some components having
> multiple drop-in replacement options. These components are often
> connected to the mainboard via ribbon cables, having the same signals
> and pin assignments across all options. These may include the display
> panel and touchscreen on laptops and tablets, and the trackpad on
> laptops. Sometimes which component option is used in a particular device
> can be detected by some firmware provided identifier, other times that
> information is not available, and the kernel has to try to probe each
> device.
>
> This change attempts to make the "probe each device" case cleaner. The
> current approach is to have all options added and enabled in the device
> tree. The kernel would then bind each device and run each driver's probe
> function. This works, but has been broken before due to the introduction
> of asynchronous probing, causing multiple instances requesting "shared"
> resources, such as pinmuxes, GPIO pins, interrupt lines, at the same
> time, with only one instance succeeding. Work arounds for these include
> moving the pinmux to the parent I2C controller, using GPIO hogs or
> pinmux settings to keep the GPIO pins in some fixed configuration, and
> requesting the interrupt line very late. Such configurations can be seen
> on the MT8183 Krane Chromebook tablets, and the Qualcomm sc8280xp-based
> Lenovo Thinkpad 13S.
>
> Instead of this delicate dance between drivers and device tree quirks,
> this change introduces a simple I2C component prober. For any given
> class of devices on the same I2C bus, it will go through all of them,
> doing a simple I2C read transfer and see which one of them responds.
> It will then enable the device that responds.
>
> This requires some minor modifications in the existing device tree.
> The status for all the device nodes for the component options must be
> set to "failed-needs-probe-xxx". This makes it clear that some mechanism
> is needed to enable one of them, and also prevents the prober and device
> drivers running at the same time.
>
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> ---
> drivers/of/Kconfig | 13 ++++
> drivers/of/Makefile | 1 +
> drivers/of/hw_prober.c | 154 +++++++++++++++++++++++++++++++++++++++++
Not sure about having this in drivers/of/, but fine for now... Really,
the I2C bus stuff should be in the I2C core with the rest of the code
that knows how to parse I2C bus nodes.
> 3 files changed, 168 insertions(+)
> create mode 100644 drivers/of/hw_prober.c
>
> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> index da9826accb1b..269d20d51936 100644
> --- a/drivers/of/Kconfig
> +++ b/drivers/of/Kconfig
> @@ -102,4 +102,17 @@ config OF_OVERLAY
> config OF_NUMA
> bool
>
> +config HW_PROBER
> + bool "Hardware Prober driver"
> + select I2C
You should not select I2C, but enable/disable I2C functionality based
on it being enabled.
> + select OF_DYNAMIC
> + help
> + Some devices will have multiple drop-in options for one component.
> + In many cases the different options are indistinguishable by the
> + kernel without actually probing each possible option.
> +
> + This driver is meant to handle the probing of such components, and
> + update the running device tree such that the correct variant is
> + made available.
> +
> endif # OF
> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> index eff624854575..ed3875cdc554 100644
> --- a/drivers/of/Makefile
> +++ b/drivers/of/Makefile
> @@ -12,6 +12,7 @@ obj-$(CONFIG_OF_RESERVED_MEM) += of_reserved_mem.o
> obj-$(CONFIG_OF_RESOLVE) += resolver.o
> obj-$(CONFIG_OF_OVERLAY) += overlay.o
> obj-$(CONFIG_OF_NUMA) += of_numa.o
> +obj-$(CONFIG_HW_PROBER) += hw_prober.o
>
> ifdef CONFIG_KEXEC_FILE
> ifdef CONFIG_OF_FLATTREE
> diff --git a/drivers/of/hw_prober.c b/drivers/of/hw_prober.c
> new file mode 100644
> index 000000000000..442da6eff896
> --- /dev/null
> +++ b/drivers/of/hw_prober.c
> @@ -0,0 +1,154 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * hw_prober.c - Hardware prober driver
> + *
> + * Copyright (c) 2023 Google LLC
> + */
> +
> +#include <linux/array_size.h>
> +#include <linux/i2c.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +
> +#define DRV_NAME "hw_prober"
> +
> +/**
> + * struct hw_prober_entry - Holds an entry for the hardware prober
> + *
> + * @compatible: compatible string to match against the machine
> + * @prober: prober function to call when machine matches
> + * @data: extra data for the prober function
> + */
> +struct hw_prober_entry {
> + const char *compatible;
> + int (*prober)(struct platform_device *pdev, const void *data);
> + const void *data;
> +};
> +
> +/*
> + * Some devices, such as Google Hana Chromebooks, are produced by multiple
> + * vendors each using their preferred components. This prober assumes such
> + * drop-in parts are on dedicated I2C busses, have non-conflicting addresses,
> + * and can be directly probed by seeing which address responds without needing
> + * regulators or GPIOs being enabled or toggled.
> + */
> +static int i2c_component_prober(struct platform_device *pdev, const void *data)
> +{
> + const char *node_name = data;
> + struct device_node *node, *i2c_node;
> + struct i2c_adapter *i2c;
> + int ret = 0;
> +
> + node = of_find_node_by_name(NULL, node_name);
> + if (!node)
> + return dev_err_probe(&pdev->dev, -ENODEV, "Could not find %s device node\n",
> + node_name);
> +
> + i2c_node = of_get_next_parent(node);
> + if (strcmp(i2c_node->name, "i2c")) {
We have functions for comparing node names, use them and don't access
->name directly.
> + of_node_put(i2c_node);
> + return dev_err_probe(&pdev->dev, -EINVAL, "%s device isn't on I2C bus\n",
> + node_name);
> + }
> +
> + for_each_child_of_node(i2c_node, node) {
> + if (!of_node_name_prefix(node, node_name))
> + continue;
> + if (!of_device_is_fail(node)) {
> + /* device tree has component already enabled */
This isn't quite right if there's a disabled device. To check 'is
enabled', you just need to use of_device_is_available().
> + of_node_put(node);
> + of_node_put(i2c_node);
> + return 0;
> + }
> + }
> +
> + i2c = of_get_i2c_adapter_by_node(i2c_node);
> + if (!i2c) {
> + of_node_put(i2c_node);
> + return dev_err_probe(&pdev->dev, -EPROBE_DEFER, "Couldn't get I2C adapter\n");
> + }
> +
> + for_each_child_of_node(i2c_node, node) {
The I2C core will walk the devices too. Perhaps if that saves off a
list of failed devices, then we don't need to walk the nodes again.
> + struct property *prop;
> + union i2c_smbus_data data;
> + u32 addr;
> +
> + if (!of_node_name_prefix(node, node_name))
> + continue;
> + if (of_property_read_u32(node, "reg", &addr))
> + continue;
> + if (i2c_smbus_xfer(i2c, addr, 0, I2C_SMBUS_READ, 0, I2C_SMBUS_BYTE, &data) < 0)
> + continue;
> +
> + dev_info(&pdev->dev, "Enabling %pOF\n", node);
> +
> + prop = kzalloc(sizeof(*prop), GFP_KERNEL);
> + if (!prop) {
> + ret = -ENOMEM;
> + of_node_put(node);
> + break;
> + }
> +
> + prop->name = "status";
> + prop->length = 5;
> + prop->value = "okay";
> +
> + /* Found a device that is responding */
> + ret = of_update_property(node, prop);
Use the changeset API instead and make an update flavor of
of_changeset_add_prop_string().
> + if (ret)
> + kfree(prop);
> +
> + of_node_put(node);
> + break;
> + }
> +
> + i2c_put_adapter(i2c);
> + of_node_put(i2c_node);
> +
> + return ret;
> +}
> +
> +static const struct hw_prober_entry hw_prober_platforms[] = {
> + { .compatible = "google,hana", .prober = i2c_component_prober, .data = "touchscreen" },
> + { .compatible = "google,hana", .prober = i2c_component_prober, .data = "trackpad" },
Not generic code. Needs to be somewhere else.
> +};
> +
> +static int hw_prober_probe(struct platform_device *pdev)
> +{
> + for (int i = 0; i < ARRAY_SIZE(hw_prober_platforms); i++)
> + if (of_machine_is_compatible(hw_prober_platforms[i].compatible)) {
> + int ret;
> +
> + ret = hw_prober_platforms[i].prober(pdev, hw_prober_platforms[i].data);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static struct platform_driver hw_prober_driver = {
> + .probe = hw_prober_probe,
> + .driver = {
> + .name = DRV_NAME,
> + },
> +};
> +
> +static int __init hw_prober_driver_init(void)
> +{
> + struct platform_device *pdev;
> + int ret;
> +
> + ret = platform_driver_register(&hw_prober_driver);
> + if (ret)
> + return ret;
> +
> + pdev = platform_device_register_simple(DRV_NAME, -1, NULL, 0);
This should be dependent on platforms that need it, not everyone. IOW,
this is where checking for "google,hana" belongs.
Rob
^ permalink raw reply
* Re: [RFC PATCH v2 0/7] of: Introduce hardware prober driver
From: Rob Herring @ 2023-11-09 13:51 UTC (permalink / raw)
To: AngeloGioacchino Del Regno
Cc: Chen-Yu Tsai, Frank Rowand, Krzysztof Kozlowski, Conor Dooley,
Matthias Brugger, Hsin-Yi Wang, Dmitry Torokhov,
andriy.shevchenko, Jiri Kosina, linus.walleij, broonie, gregkh,
hdegoede, james.clark, james, keescook, petr.tesarik.ext, rafael,
tglx, Jeff LaBundy, linux-input, devicetree, linux-arm-kernel,
linux-mediatek, linux-kernel, Douglas Anderson, Johan Hovold
In-Reply-To: <859ac058-c50a-4eb8-99b6-3011ef4e7529@collabora.com>
On Thu, Nov 9, 2023 at 4:54 AM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
>
> Il 09/11/23 11:05, Chen-Yu Tsai ha scritto:
> > Hi everyone,
> >
> > This v2 series continues Doug's "of: device: Support 2nd sources of
> > probeable but undiscoverable devices" [1] series, but follows the scheme
> > suggested by Rob, marking all second source component device nodes
> > as "fail-needs-probe-XXX", and having a hardware prober driver enable
> > the one of them. I tried to include everyone from the original Cc: list.
> > Please let me know if you would like to be dropped from future
> > submissions.
> >
> >
> > For the I2C component (touchscreens and trackpads) case from the
> > original series, the hardware prober driver finds the particular
> > class of device in the device tree, gets its parent I2C adapter,
> > and tries to initiate a simple I2C read for each device under that
> > I2C bus. When it finds one that responds, it considers that one
> > present, marks it as "okay", and returns, letting the driver core
> > actually probe the device.
> >
> > This works fine in most cases since these components are connected
> > via ribbon cable and always have the same resources. The driver as
> > implemented currently doesn't deal with regulators or GPIO pins,
> > since in the existing device trees they are either always on for
> > regulators, or have GPIO hogs or pinmux and pinconfig directly
> > tied to the pin controller.
> >
> >
> > Another case this driver could handle is selecting components based
> > on some identifier passed in by the firmware. On Chromebooks we have
> > a SKU ID which is inserted by the bootloader at
> > /firmware/coreboot/sku-id. When a new combination of components is
> > introduced, a new SKU ID is allocated to it. To have SKU ID based
> > device trees, we would need to have one per SKU ID. This ends up
> > increasing the number of device trees we have a lot. The recent
> > MT8186 devices already have 10+10 SKUs [2], with possibly more to come.
> >
> > Instead, we could have just one device tree for each device, with
> > component options listed and marked as "fail-needs-probe-XXX", and
> > let the hardware prober enable one of them based on the given SKU ID.
> > The driver will also fix up OF graph remote endpoints to point to the
> > enabled component.
> >
> > The MT8186 Corsola series [2] can also benefit from this, though I
> > haven't implemented anything yet.
> >
> >
> > Patch 1 adds of_device_is_fail() for the new driver to use.
> >
> > Patch 2 implements the first case, probing the I2C bus for presence
> > of components. This initial version targets the Hana Chromebooks.
> >
> > Patch 3 modifies the Hana device tree and marks the touchscreens
> > and trackpads as "fail-needs-probe-XXX", ready for the driver to
> > probe.
> >
> > Patch 4 adds a missing touchscreen variant to Hana.
> >
> > Patch 5 implements the second case, selectively enabling components
> > based on the SKU ID. This initial version targets the Krane ChromeOS
> > tablet, which has two possible MIPI DSI display panel options.
> >
> > Patch 6 drops Krane's SKU-specific compatible strings from the bindings.
> >
> > Patch 7 merges Krane's SKU-specific device trees into one, with the
> > device tree now containing two possible panels. This unfortunately
> > introduces a dtc warning:
> >
> > arch/arm64/boot/dts/mediatek/mt8183-kukui-krane.dts:81.13-83.6:
> > Warning (graph_endpoint): /soc/dsi@14014000/panel2@0/port/endpoint:
> > graph connection to node '/soc/dsi@14014000/ports/port/endpoint'
> > is not bidirectional
> >
> >
> > Please take a look.
> >
> > Johan, I'm not sure if this works as is for the Lenovo Thinkpad 13S
> > case, since it looks like the trackpad shares the I2C bus with the
> > keyboard.
> >
> >
> > Thanks
> > ChenYu
> >
> >
> > Background as given in Doug's cover letter:
> >
> > Support for multiple "equivalent" sources for components (also known
> > as second sourcing components) is a standard practice that helps keep
> > cost down and also makes sure that if one component is unavailable due
> > to a shortage that we don't need to stop production for the whole
> > product.
> >
> > Some components are very easy to second source. eMMC, for instance, is
> > fully discoverable and probable so you can stuff a wide variety of
> > similar eMMC chips on your board and things will work without a hitch.
> >
> > Some components are more difficult to second source, specifically
> > because it's difficult for software to probe what component is present
> > on any given board. In cases like this software is provided
> > supplementary information to help it, like a GPIO strap or a SKU ID
> > programmed into an EEPROM. This helpful information can allow the
> > bootloader to select a different device tree. The various different
> > "SKUs" of different Chromebooks are examples of this.
> >
> > Some components are somewhere in between. These in-between components
> > are the subject of this patch. Specifically, these components are
> > easily "probeable" but not easily "discoverable".
> >
> > A good example of a probeable but undiscoverable device is an
> > i2c-connected touchscreen or trackpad. Two separate components may be
> > electrically compatible with each other and may have compatible power
> > sequencing requirements but may require different software. If
> > software is told about the different possible components (because it
> > can't discover them), it can safely probe them to figure out which
> > ones are present.
> >
> > On systems using device tree, if we want to tell the OS about all of
> > the different components we need to list them all in the device
> > tree. This leads to a problem. The multiple sources for components
> > likely use the same resources (GPIOs, interrupts, regulators). If the
> > OS tries to probe all of these components at the same time then it
> > will detect a resource conflict and that's a fatal error.
> >
> > The fact that Linux can't handle these probeable but undiscoverable
> > devices well has had a few consequences:
> > 1. In some cases, we've abandoned the idea of second sourcing
> > components for a given board, which increases cost / generates
> > manufacturing headaches.
> > 2. In some cases, we've been forced to add some sort of strapping /
> > EEPROM to indicate which component is present. This adds difficulty
> > to manufacturing / refurb processes.
> > 3. In some cases, we've managed to make things work by the skin of our
> > teeth through slightly hacky solutions. Specifically, if we remove
> > the "pinctrl" entry from the various options then it won't
> > conflict. Regulators inherently can have more than one consumer, so
> > as long as there are no GPIOs involved in power sequencing and
> > probing devices then things can work. This is how
> > "sc8280xp-lenovo-thinkpad-x13s" works and also how
> > "mt8173-elm-hana" works.
> >
> > End of background from Doug's cover letter.
>
> I think that using "status" is not a good idea, I find that confusing.
"status" is what defines a device's state in terms of enabled,
present, available. That's exactly what we're expressing here.
Now, I do not think we should be mixing the device class (e.g.
touchscreen) into status. I said this on v1, but apparently that was
not listened to.
>
> Perhaps we could have a node like
>
> something {
> device-class-one = <&device1>, <&device2>, <&device3>;
> device-class-two = <&device4>, <&device5>, <&device6>;
> }
>
> so that'd be more or less
>
> hw-prober {
> trackpads = <&tp1>, <&tp2>;
> keyboards = <&kb1>, <&kb2>;
> touchscreens = <&ts1>, <&ts2>;
> }
No. That's more or less what v1 had.
Rob
^ permalink raw reply
* Re: Requesting your attention and expertise regarding a Tablet/Kernel issue
From: Benjamin Tissoires @ 2023-11-09 11:56 UTC (permalink / raw)
To: David Revoy
Cc: José Expósito, Eric GOUYER, Illia Ostapyshyn, jkosina,
jason.gerecke, linux-input, linux-kernel
In-Reply-To: <_DEF7tHL1p_ExY7GJlJvT5gRA7ZvNnVMJuURb8_WCV-0fbYXkLN2p5zHloi6wiJPNzGEjFAkq2sjbCU633_eNF_cGm0rAbmCOOIOfwe1jWo=@protonmail.com>
Hi David,
On Thu, Nov 9, 2023 at 1:32 AM David Revoy <davidrevoy@protonmail.com> wrote:
>
> Hi Benjamin,
>
> > Alright, I made quite some progress so far:
> > - regressions tests have been written (branch wip/xp-pen of my fork on
> > freedesktop[0])
> > that branch can not go in directly as it just adds the tests, and
> > thus is failing
> > - I made the fixes through HID-BPF[1]
> >
> > Anyone using those 2 tablets and using Fedora should be able to just
> > grab the artifact at [2], uncompress it and run `sudo ./install.sh --verbose`.
> > This will install the bpf programs in /lib/firmware/hid/bpf/ and will
> > automatically load them when the device is connected.
> >
> > For those not using Fedora, the binary might work (or not, not sure),
> > but you can always decompress it, and check if running
> > `udev-hid-bpf_0.1.0/bin/udev-hid-bpf --version` returns the correct
> > version or just fails. If you get "udev-hid-bpf 0.1.0", then running
> > `sudo ./install.sh --verbose` should work, as long as the kernel has
> > CONFIG_HID_BPF set to 'Y'.
> > [...]
> > [0] https://gitlab.freedesktop.org/bentiss/hid/-/tree/wip/xp-pen?ref_type=heads
> > [1] https://gitlab.freedesktop.org/libevdev/udev-hid-bpf/-/merge_requests/27
> > [2] https://gitlab.freedesktop.org/bentiss/udev-hid-bpf/-/jobs/51350589/artifacts/raw/udev-hid-bpf_0.1.0.tar.xz
>
> Thank you for this package.
>
> I was able to test it even though the link in (2) at the bottom of your email returned a blank page. I was able to find my way after manually visiting gitlab.freedesktop.org [1] and then manually downloading the article from 51350589. I unzipped it and ran `sudo ./install.sh --verbose`. Everything looks like it was successful [2]. I then rebooted my Fedora 38 'Linux workstation 6.5.8-200.fc38.x86_64' kernel (the one I blamed in my post) and tested both tablets.
Weird that you had to manually retrieve it. It works here, but maybe
because I am logged in on gitlab.fd.o.
Also, just FYI, you shouldn't have to reboot. Just unplug/replug and
you are good. In the same way, if you uninstall the package, you can
just unplug/replug to not have the programs loaded.
>
> Here are my observation:
>
> XPPEN Artist Pro 24
> ===================
> Nothing changed for this device (it's the one with two buttons and no 'eraser tip'). Nor my hwdb/udev rules or `xsetwacom set "UGTABLET 24 inch PenDisplay eraser" button 1 3` affects the upper button of the stylus: if I hold it hover the canvas, Krita switch the tool and cursor for an eraser. If I click on the canvas with the pen tip while holding the upper button pressed, I get the right-click Pop-up Palette (but not all the time, probably Krita has hard time to triage Eraser or Right-click).
As I mentioned in another reply, the more I think of it, the more I
think I should get rid of the "eraser mode". In that Artist Pro 24 I
can detect it through the same mechanics as the HID_QUIRK_NOINVERT
from Illia's patch. But instead of trying to force the device into the
eraser mode, we should just say "this is actually BUTTON_STYLUS_2".
So I'm going to amend the bpf program to do this and hopefully you
won't need the hwdb/udev rule at all.
>
> XPPEN Artist Pro 16 (Gen2)
> ==========================
> Something changed. `xsetwacom set "UGTABLET Artist Pro 16 (Gen2) eraser" button 1 3` successfully affected the upper button of the stylus. Now if I click it while hovering the canvas, Krita shows the right click Pop-up Palette.
I'm surprised you need to teach the wacom driver that BTN_STYLUS_2 is
the right click.
> On the downside; the real eraser tip when I flip the stylus bugs. When I flip the stylus on eraser hovering the canvas, Krita shows the Eraser icon and switch tool. As soon as I draw with the eraser tip, Krita will also show a right-click color palette and with also not a 100% consistency, as if the event were mixed.
I'll investigate. Maybe I messed up with my event flow patch.
But just to be sure, you don't have a custom configuration in place
for that tablet device?
Cheers,
Benjamin
>
> [1] https://gitlab.freedesktop.org/bentiss/udev-hid-bpf/-/artifacts
> [2] https://www.peppercarrot.com/extras/temp/2023-11-09_screenshot_005214_net.jpg
>
>
> On Wednesday, November 8th, 2023 at 19:21, Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote:
>
>
> > On Wed, Nov 8, 2023 at 10:34 AM Benjamin Tissoires
> > benjamin.tissoires@redhat.com wrote:
> >
> > > On Wed, Nov 8, 2023 at 10:23 AM José Expósito jose.exposito89@gmail.com wrote:
> > >
> > > > Hi Benjamin,
> > > >
> > > > On Wed, Nov 08, 2023 at 10:04:30AM +0100, Benjamin Tissoires wrote:
> > > > [...]
> > > >
> > > > > > So, the behavior probably breaks the specs, but sincerely I'm happy to
> > > > > > have the "eraser" button independent of the "rubber eraser", which
> > > > > > makes the stylus a somewhat 4-buttons stylus (tip, button1, button2,
> > > > > > rubber), and I would like to keep this.
> > > > >
> > > > > Yes, and I'd like to keep it that way, even if 6.6 and 6.5.8
> > > > > apparently broke it.
> > > > >
> > > > > So, to me:
> > > > > - 276e14e6c3993317257e1787e93b7166fbc30905 is wrong: this is a
> > > > > firmware bug (reporting invert through eraser) and should not be
> > > > > tackled at the generic level, thus it should be reverted
> > > > > - both of these tablets are forwarding the useful information, but not
> > > > > correctly, which confuses the kernel
> > > > > - I should now be able to write regression tests
> > > > > - I should be able to provide HID-BPF fixes for those tablets so that
> > > > > we can keep them working with or without
> > > > > 276e14e6c3993317257e1787e93b7166fbc30905
> > > > > reverted (hopefully)
> > > > > - problem is I still don't have the mechanics to integrate the HID-BPF
> > > > > fixes directly in the kernel tree, so maybe I'll have to write a
> > > > > driver for XP-Pen while these internals are set (it shouldn't
> > > > > interfere with the HID-BPF out of the tree).
> > > >
> > > > I already added support for a few XP-Pen devices on the UCLogic driver
> > > > and I was planning to start working on this one during the weekend in
> > > > my DIGImend fork (to simplify testing).
> > > >
> > > > Let me know if you prefer to add it yourself or if you want me to ping
> > > > you in the DIGImend discussion.
> > >
> > > So far, I really have to work on this now. It's a good use case for
> > > HID-BPF and it's a regression that I'd like to be fixed ASAP.
> > > I'd appreciate any reviews :)
> >
> >
> > Alright, I made quite some progress so far:
> > - regressions tests have been written (branch wip/xp-pen of my fork on
> > freedesktop[0])
> > that branch can not go in directly as it just adds the tests, and
> > thus is failing
> > - I made the fixes through HID-BPF[1]
> >
> > Anyone using those 2 tablets and using Fedora should be able to just
> > grab the artifact at [2], uncompress it and run `sudo ./install.sh --verbose`.
> > This will install the bpf programs in /lib/firmware/hid/bpf/ and will
> > automatically load them when the device is connected.
> >
> > For those not using Fedora, the binary might work (or not, not sure),
> > but you can always decompress it, and check if running
> > `udev-hid-bpf_0.1.0/bin/udev-hid-bpf --version` returns the correct
> > version or just fails. If you get "udev-hid-bpf 0.1.0", then running
> > `sudo ./install.sh --verbose` should work, as long as the kernel has
> > CONFIG_HID_BPF set to 'Y'.
> >
> > > Also, good to know that I can probably piggyback on hid-uclogic for
> > > fixing those 2 devices in the kernel.
> >
> >
> > Next step will be to fix them using a kernel driver, but it seems that
> > the uclogic driver is doing more than just report descriptor fixups,
> > so maybe I'll have to use a different driver.
> > But the point is, in theory, if you are affected by those bugs, using
> > the udev-hid-bpf should fix the issue, and this should also be
> > resilient to further kernel updates.
> >
> > I'd appreciate testing when I got the full kernel series up and ready,
> > of course.
> >
> > Cheers,
> > Benjamin
> >
> > [0] https://gitlab.freedesktop.org/bentiss/hid/-/tree/wip/xp-pen?ref_type=heads
> > [1] https://gitlab.freedesktop.org/libevdev/udev-hid-bpf/-/merge_requests/27
> > [2] https://gitlab.freedesktop.org/bentiss/udev-hid-bpf/-/jobs/51350589/artifacts/raw/udev-hid-bpf_0.1.0.tar.xz
>
^ permalink raw reply
* Re: Requesting your attention and expertise regarding a Tablet/Kernel issue
From: Benjamin Tissoires @ 2023-11-09 11:47 UTC (permalink / raw)
To: David Revoy
Cc: Illia Ostapyshyn, jkosina, jason.gerecke, jose.exposito89,
linux-input, linux-kernel, nils, peter.hutterer, ping.cheng,
bagasdotme
In-Reply-To: <A01KgwZWh8vP1ux3J92E572eCVMfYPzBcHLuuGfSTYntQMVErkqIcPhJtWRxJsinbI_AfHvD_GcnGvQ1kFtxR36ozCPj_VH8Ys8OlA02MZQ=@protonmail.com>
On Thu, Nov 9, 2023 at 12:19 AM David Revoy <davidrevoy@protonmail.com> wrote:
>
> > BTW, David, were you able to do a revert of 276e14e6c3?
>
> I'm sorry Benjamin: I did some research on how to build a kernel [1], on how to revert a commit (easy, I know a bit of Git), and started following it step by step. Result: I failed and concluded that it probably requires too much computer knowledge compared to what I can do now. I'm afraid I won't be able to build a custom kernel for testing.
No worries. And I'm actually happy, because you definitely fit into
the HID-BPF model where I want to fix a user's device without
requiring kernel compilation, and fixing the device in a reliable way
that we can do the general fix without impacting the reporter.
Cheers,
Benjamin
>
> [1] https://docs.fedoraproject.org/en-US/quick-docs/kernel-build-custom/#_building_a_vanilla_upstream_kernel
>
>
> On Tuesday, November 7th, 2023 at 08:59, Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote:
>
>
> > On Mon, Nov 6, 2023 at 9:06 PM Illia Ostapyshyn
> > ostapyshyn@sra.uni-hannover.de wrote:
> >
> > > On 11/6/23 17:59, Benjamin Tissoires wrote:
> > >
> > > > If the pen has 2 buttons, and an eraser side, it would be a serious
> > > > design flow for XPPEN to report both as eraser.
> > > >
> > > > Could you please use sudo hid-recorder from hid-tools[1] on any kernel
> > > > version and send us the logs here?
> > > > I'll be able to replay the events locally, and understand why the
> > > > kernel doesn't work properly.
> > > >
> > > > And if there is a design flaw that can be fixed, we might even be able
> > > > to use hid-bpf to change it :)
> > >
> > > My wild guess is that XP-Pen 16 Artist Pro reports an Eraser usage
> > > without Invert for the upper button and Eraser with Invert for the
> > > eraser tip. A device-specific driver could work with that, but there
> > > seems to be no way to incorporate two different erasers (thus, allowing
> > > userspace to map them to different actions arbitrarily) in the generic
> > > driver currently.
> >
> >
> > That's exactly why I want to see the exact event flow. We can not do
> > "wild guesses" unfortunately (not meaning any offenses).
> > And I am very suspicious about the fact that the stylus reports 2
> > identical erasers. Because in the past David seemed to be able to have
> > 2 distincts behaviors for the 2 "buttons" (physical button and eraser
> > tail).
> >
> > > > Generally speaking, relying on X to fix your hardware is going to be a
> > > > dead end. When you switch to wayland, you'll lose all of your fixes,
> > > > which isn't great.
> > >
> > > > AFAIU, the kernel now "merges" both buttons, which is a problem. It
> > > > seems to be a serious regression. This case is also worrying because I
> > > > added regression tests on hid, but I don't have access to all of the
> > > > various tablets, so I implemented them from the Microsoft
> > > > specification[0]. We need a special case for you here.
> > >
> > > The issue preventing David from mapping HID_DG_ERASER to BTN_STYLUS2 is
> > > that the hidinput_hid_event is not compatible with hidinput_setkeycode.
> > > If usage->code is no longer BTN_TOUCH after remapping, it won't be
> > > released when Eraser reports 0. A simple fix is:
> >
> >
> > I must confess, being the one who refactored everything, I still don't
> > believe this is as simple as it may seem. I paged out all of the
> > special cases, and now, without seeing the event flow I just can not
> > understand why this would fix the situation.
> >
> > And BTW, if you have a tool affected by 276e14e6c3, I'd be curious to
> > get a hid-recorder sample for it so I can get regression tests for it.
> >
> > > --- a/drivers/hid/hid-input.c
> > > +++ b/drivers/hid/hid-input.c
> > > @@ -1589,7 +1589,7 @@ void hidinput_hid_event(struct hid_device *hid,
> > > struct hid_field field, struct
> > > / value is off, tool is not rubber, ignore */
> > > return;
> > > else if (*quirks & HID_QUIRK_NOINVERT &&
> > > - !test_bit(BTN_TOUCH, input->key)) {
> > > + !test_bit(usage->code, input->key)) {
> >
> >
> > I don't want to be rude, but this feels very much like black magic,
> > especially because there is a comment just below and it is not
> > updated. So either the explanation was wrong, or it's not explaining
> > the situation (I also understand that this is not a formal submission,
> > so maybe that's the reason why the comment is not updated).
> >
> > > /*
> > > * There is no invert to release the tool, let hid_input
> > > * send BTN_TOUCH with scancode and release the tool after.
> > >
> > > This change alone fixes David's problem and the right-click mapping in
> > > hwdb works again. However, the tool switches to rubber for the remapped
> > > eraser (here BTN_STYLUS2) events, both for devices with and without
> > > Invert. This does no harm but is not useful either. A cleaner solution
> > > for devices without Invert would be to omit the whole tool switching
> > > logic in this case:
> > >
> > > @@ -1577,6 +1577,9 @@ void hidinput_hid_event(struct hid_device *hid,
> > > struct hid_field *field, struct
> > >
> > > switch (usage->hid) {
> > > case HID_DG_ERASER:
> > > + if (*quirks & HID_QUIRK_NOINVERT && usage->code != BTN_TOUCH)
> > > + break;
> > > +
> > > report->tool_active |= !!value;
> > >
> > > Remapping Invert does not work anyway as the Invert tool is hardcoded in
> > > hidinput_hid_event. Even worse, I guess (not tested) trying to do so
> > > would mask BTN_TOOL_RUBBER from dev->keybit and could cause weird
> > > behavior similar to one between 87562fcd1342 and 276e14e6c3. This
> > > raises the question: should users be able to remap Invert after all?
> >
> >
> > The kernel is supposed to transfer what the device is. So if it says
> > this is an eraser, we should not try to change it. Users can then
> > tweak their own device if they wish through hid-bpf or through
> > libinput quirks, but when you install a fresh kernel without tweaks,
> > we should be as accurate as possible.
> >
> > My main concern is that now we have a device which exports 2 different
> > interactions as being the same. So either the firmware is wrong, and
> > we need to quirk it, or the kernel is wrong and merges both, and this
> > needs fixes as well.
> >
> > Once every interaction on the device gets its own behavior, userspace
> > can do whatever they want. It's not the kernel's concern anymore.
> >
> > BTW, David, were you able to do a revert of 276e14e6c3?
> >
> > Cheers,
> > Benjamin
>
^ permalink raw reply
* Re: Requesting your attention and expertise regarding a Tablet/Kernel issue
From: Benjamin Tissoires @ 2023-11-09 11:46 UTC (permalink / raw)
To: ostapyshyn
Cc: Nils Fuhler, davidrevoy, folays, jason.gerecke, jkosina,
jose.exposito89, linux-input, linux-kernel
In-Reply-To: <87zfzndghj.fsf@sra.uni-hannover.de>
On Wed, Nov 8, 2023 at 11:32 PM <ostapyshyn@sra.uni-hannover.de> wrote:
>
> On 11/8/23 21:34, Benjamin Tissoires wrote:
>
> > Again, you convinced me that this commit was wrong. If people needs to
> > also use an ioctl to "fix" it, then there is something wrong.
>
> I don't think we're on the same page here. Nobody needs to use an ioctl
> to fix 276e14e6c3. Rather, the _exact opposite_: the bug reporter used
> an ioctl to remap Eraser to BTN_STYLUS2. It stopped working after
> 276e14e6c3 and broke his workflow. He reported it as a regression,
> starting this whole thread.
After more thoughts about Niels' email, the whole thread and a
not-so-good night's sleep, I think I now understand what is the
problem.
And yes, most of the problem comes from that remap *after* the kernel
parsed the device and made a decision based on what was provided to
it.
>
> > Sorry but I tend to disagree. Relying on the ioctl EVIOCSKEYCODE for
> > tuning the behavior of a state machine is just plain wrong. When
> > people do that, they are doing it at the wrong level and introducing
> > further bugs.
> >
> > The whole pen and touch HID protocols rely on a state machine. You
> > just can not change the meaning of it because your hardware maker
> > produced a faulty hardware.
> >
> > [...]
> >
> > In the same way, if you remap Tip Switch to KEY-A, you won't get
> > clicks from your pen. Assuming you can do that with any event on any
> > HID device is just plain wrong.
> > That ioctl is OK-ish for "remapping" simple things like keys. In our
> > case, the whole firmware follows a state machine, so we can not change
> > it. It has to be remapped in a later layer, in libinput, your
> > compositor, or in your end user application.
>
> I don't disagree. Forbidding EVIOCSKEYCODE ioctls for pen and touch HID
> is a legitimate way to resolve this (an appealing one too -- accounting
> for it in hidinput_hid_event might be a hellish task).
I think it would be best not to require the need for the ioctl in the
first place.
Looking at David's blog, I'm starting to wonder if we actually need to
report BTN_TOOL_RUBBER after all in the case where there is no Invert
usage.
>
> Should we forbid remapping Eraser too? If your answer is yes, then we
> can finish this conversation here and leave the code as it is now,
> because __the regression__ is a user not being able to use an ioctl to
> remap Eraser after 276e14e6c3. Otherwise, if we do make an exemption for
> David's Eraser, the fix is as simple as replacing BTN_TOUCH with
> usage->code on line 1594 of hid-input.c.
>
> > How many of such devices do we have? Are they all UGTablet like the
> > XP-PEN? Are they behaving properly on Windows without a proprietary
> > driver?
> >
> > [...]
> >
> > I might buy the "invertless" devices are a thing if I can get more
> > data about it. So far there are only 2 of them, and they add extra
> > complexity in the code when we can just patch the devices to do the
> > right thing.
>
> There might or might not be more devices like this in the wild. It looks
> like BarrelSwitch2 was added only 2013 [1], which is why so many styli
> use Eraser for the second button. Setting two bits for a single button
> just to adhere to Microsoft's *recommendation* is nice for compatibility,
> but I can imagine vendors taking a shortcut and omitting Invert
> altogether. The HID specification alone just lists the usages and says
> nothing about how they relate to each other.
Right. So maybe instead of trying to force the "no Invert" pens into
the "oh, this looks like an eraser", maybe we should remap in that
case the eraser usage into a secondary barrel switch.
We then need to filter the proximity out event that is sent when the
user presses it, but all in all it should be doable (hopefully).
>
> XP-Pen Artist 24 does work on Windows with the generic driver. The
> Eraser engages as soon as the button is pressed, without touching the
> screen.
OK, thanks for the confirmation.
I just had a meeting with Peter Hutterer, and he told me it would be
best if the kernel doesn't follow the entire "this button is an eraser
mode". But that requires some filtering of the events because some
hardware (like the Artist 24 here) partially implements the
"specification" by sending a proximity out event when the button is
pressed.
So my idea would be to do that change in HID-BPF, so that it's only
included when libinput supports it (no regressions then), and we can
actually change the heuristics more easily than having to patch the
kernel.
I'd also need to get the behavior of:
- stylus is in range -> second button is pressed -> stylus touches the
surface with the button still pressed -> button is released -> stylus
leaves the surface and goes out of proximity.
- stylus is in range -> stylus touches the surface without any button
pressed -> second button is pressed -> stylus leaves the surface and
goes out of proximity
- stylus is in range -> stylus touches the surface without any button
pressed -> second button is pressed -> second button is released ->
stylus leaves the surface and goes out of proximity
And probably some other weird corner cases.
If we get the "eraser" event being set to 0/1 when the button is
pressed whether the stylus touches the surface or not, it would be
simple enough to change the purpose of the button in HID-BPF and
filter the eventual prox out events.
>
> > New hardware isn't supposed to be supported on an old kernel and is
> > not considered as a regression. However, David mentioned that the
> > device was "working" on 6.5.0 but broke in 6.5.8 with the patch
> > mentioned above. This is a regression that needs to be tackled.
> > Especially because it was introduced in 6.6 but backported into 6.5.
>
> To make sure we're talking about the same thing:
>
> 1. "Broke" in this context means that the ioctl remapping from Eraser to
> right-click stopped working.
Yeah, you're correct. This isn't a regression, it's a user tempering
with the kernel and the kernel can't deal with it.
But the use case is still valid. It's the way it was done that was wrong.
>
> 2. XPPen 16 Pro Gen2 is a whole different topic, untouched by 276e14e6c3.
I still need to figure out what is wrong after my HID-BPF changes. But
yeah, it is orthogonal.
Cheers,
Benjamin
>
> [1] https://www.usb.org/sites/default/files/hutrr46e.txt
>
^ permalink raw reply
* Re: [RFC PATCH v2 0/7] of: Introduce hardware prober driver
From: AngeloGioacchino Del Regno @ 2023-11-09 10:54 UTC (permalink / raw)
To: Chen-Yu Tsai, Rob Herring, Frank Rowand, Krzysztof Kozlowski,
Conor Dooley, Matthias Brugger
Cc: Hsin-Yi Wang, Dmitry Torokhov, andriy.shevchenko, Jiri Kosina,
linus.walleij, broonie, gregkh, hdegoede, james.clark, james,
keescook, petr.tesarik.ext, rafael, tglx, Jeff LaBundy,
linux-input, devicetree, linux-arm-kernel, linux-mediatek,
linux-kernel, Douglas Anderson, Johan Hovold
In-Reply-To: <20231109100606.1245545-1-wenst@chromium.org>
Il 09/11/23 11:05, Chen-Yu Tsai ha scritto:
> Hi everyone,
>
> This v2 series continues Doug's "of: device: Support 2nd sources of
> probeable but undiscoverable devices" [1] series, but follows the scheme
> suggested by Rob, marking all second source component device nodes
> as "fail-needs-probe-XXX", and having a hardware prober driver enable
> the one of them. I tried to include everyone from the original Cc: list.
> Please let me know if you would like to be dropped from future
> submissions.
>
>
> For the I2C component (touchscreens and trackpads) case from the
> original series, the hardware prober driver finds the particular
> class of device in the device tree, gets its parent I2C adapter,
> and tries to initiate a simple I2C read for each device under that
> I2C bus. When it finds one that responds, it considers that one
> present, marks it as "okay", and returns, letting the driver core
> actually probe the device.
>
> This works fine in most cases since these components are connected
> via ribbon cable and always have the same resources. The driver as
> implemented currently doesn't deal with regulators or GPIO pins,
> since in the existing device trees they are either always on for
> regulators, or have GPIO hogs or pinmux and pinconfig directly
> tied to the pin controller.
>
>
> Another case this driver could handle is selecting components based
> on some identifier passed in by the firmware. On Chromebooks we have
> a SKU ID which is inserted by the bootloader at
> /firmware/coreboot/sku-id. When a new combination of components is
> introduced, a new SKU ID is allocated to it. To have SKU ID based
> device trees, we would need to have one per SKU ID. This ends up
> increasing the number of device trees we have a lot. The recent
> MT8186 devices already have 10+10 SKUs [2], with possibly more to come.
>
> Instead, we could have just one device tree for each device, with
> component options listed and marked as "fail-needs-probe-XXX", and
> let the hardware prober enable one of them based on the given SKU ID.
> The driver will also fix up OF graph remote endpoints to point to the
> enabled component.
>
> The MT8186 Corsola series [2] can also benefit from this, though I
> haven't implemented anything yet.
>
>
> Patch 1 adds of_device_is_fail() for the new driver to use.
>
> Patch 2 implements the first case, probing the I2C bus for presence
> of components. This initial version targets the Hana Chromebooks.
>
> Patch 3 modifies the Hana device tree and marks the touchscreens
> and trackpads as "fail-needs-probe-XXX", ready for the driver to
> probe.
>
> Patch 4 adds a missing touchscreen variant to Hana.
>
> Patch 5 implements the second case, selectively enabling components
> based on the SKU ID. This initial version targets the Krane ChromeOS
> tablet, which has two possible MIPI DSI display panel options.
>
> Patch 6 drops Krane's SKU-specific compatible strings from the bindings.
>
> Patch 7 merges Krane's SKU-specific device trees into one, with the
> device tree now containing two possible panels. This unfortunately
> introduces a dtc warning:
>
> arch/arm64/boot/dts/mediatek/mt8183-kukui-krane.dts:81.13-83.6:
> Warning (graph_endpoint): /soc/dsi@14014000/panel2@0/port/endpoint:
> graph connection to node '/soc/dsi@14014000/ports/port/endpoint'
> is not bidirectional
>
>
> Please take a look.
>
> Johan, I'm not sure if this works as is for the Lenovo Thinkpad 13S
> case, since it looks like the trackpad shares the I2C bus with the
> keyboard.
>
>
> Thanks
> ChenYu
>
>
> Background as given in Doug's cover letter:
>
> Support for multiple "equivalent" sources for components (also known
> as second sourcing components) is a standard practice that helps keep
> cost down and also makes sure that if one component is unavailable due
> to a shortage that we don't need to stop production for the whole
> product.
>
> Some components are very easy to second source. eMMC, for instance, is
> fully discoverable and probable so you can stuff a wide variety of
> similar eMMC chips on your board and things will work without a hitch.
>
> Some components are more difficult to second source, specifically
> because it's difficult for software to probe what component is present
> on any given board. In cases like this software is provided
> supplementary information to help it, like a GPIO strap or a SKU ID
> programmed into an EEPROM. This helpful information can allow the
> bootloader to select a different device tree. The various different
> "SKUs" of different Chromebooks are examples of this.
>
> Some components are somewhere in between. These in-between components
> are the subject of this patch. Specifically, these components are
> easily "probeable" but not easily "discoverable".
>
> A good example of a probeable but undiscoverable device is an
> i2c-connected touchscreen or trackpad. Two separate components may be
> electrically compatible with each other and may have compatible power
> sequencing requirements but may require different software. If
> software is told about the different possible components (because it
> can't discover them), it can safely probe them to figure out which
> ones are present.
>
> On systems using device tree, if we want to tell the OS about all of
> the different components we need to list them all in the device
> tree. This leads to a problem. The multiple sources for components
> likely use the same resources (GPIOs, interrupts, regulators). If the
> OS tries to probe all of these components at the same time then it
> will detect a resource conflict and that's a fatal error.
>
> The fact that Linux can't handle these probeable but undiscoverable
> devices well has had a few consequences:
> 1. In some cases, we've abandoned the idea of second sourcing
> components for a given board, which increases cost / generates
> manufacturing headaches.
> 2. In some cases, we've been forced to add some sort of strapping /
> EEPROM to indicate which component is present. This adds difficulty
> to manufacturing / refurb processes.
> 3. In some cases, we've managed to make things work by the skin of our
> teeth through slightly hacky solutions. Specifically, if we remove
> the "pinctrl" entry from the various options then it won't
> conflict. Regulators inherently can have more than one consumer, so
> as long as there are no GPIOs involved in power sequencing and
> probing devices then things can work. This is how
> "sc8280xp-lenovo-thinkpad-x13s" works and also how
> "mt8173-elm-hana" works.
>
> End of background from Doug's cover letter.
I think that using "status" is not a good idea, I find that confusing.
Perhaps we could have a node like
something {
device-class-one = <&device1>, <&device2>, <&device3>;
device-class-two = <&device4>, <&device5>, <&device6>;
}
so that'd be more or less
hw-prober {
trackpads = <&tp1>, <&tp2>;
keyboards = <&kb1>, <&kb2>;
touchscreens = <&ts1>, <&ts2>;
}
Besides, something else I can suggest here is to make this more generic: actually,
this issue is spread across way more devices than you maybe think... for example,
I know of some smartphones that may have the same situation with DSI displays and
they're sometimes distinguished by an ADC value, sometimes by reading back the
manufacturer ID (or panel id) through DSI.
Also, if Chromebooks really need something "special", such as that coreboot sku-id
parameter, I think that this should be registered externally into the hw prober
and not embedded inside of the *generic* hw prober driver.
We can even reuse of_device_id instead of inventing a new hw_prober_entry struct...
Idea:
drivers/platform/chrome/cros_of_hw_prober.c
static int cros_sku_hw_prober(struct platform_device *pdev, const void *data)
{
...this is your cros_sku_component_selector() function, anyway...
}
static const struct of_device_id cros_hw_prober_ids[] = {
{ .compatible = "google,hana", .data = something },
{ /* sentinel */ }
};
static int some_kind_of_early_init_function(something)
{
int a,b,c,ret,something;
.. some logic if necessary ..
return of_hw_prober_register(cros_sku_hw_prober, cros_hw_prober_ids);
}
Btw, thanks for starting that. If this will be done the right way, it's going to
be useful to many, many people.
Regards,
Angelo
>
> [1] https://lore.kernel.org/all/20230921102420.RFC.1.I9dddd99ccdca175e3ceb1b9fa1827df0928c5101@changeid/
> [2] https://lore.kernel.org/linux-mediatek/20231012230237.2676469-1-wenst@chromium.org/
>
> Chen-Yu Tsai (7):
> of: base: Add of_device_is_fail
> of: Introduce hardware prober driver
> arm64: dts: mediatek: mt8173-elm-hana: Mark touchscreens and trackpads
> as fail
> arm64: dts: mediatek: mt8173-elm-hana: Add G2touch G7500 touchscreen
> of: hw_prober: Support Chromebook SKU ID based component selection
> dt-bindings: arm: mediatek: Remove SKU specific compatibles for Google
> Krane
> arm64: dts: mediatek: mt8183-kukui: Merge Krane device trees
>
> .../devicetree/bindings/arm/mediatek.yaml | 3 -
> arch/arm64/boot/dts/mediatek/Makefile | 3 +-
> .../boot/dts/mediatek/mt8173-elm-hana.dtsi | 20 ++
> .../dts/mediatek/mt8183-kukui-krane-sku0.dts | 24 --
> .../mediatek/mt8183-kukui-krane-sku176.dts | 24 --
> ...ukui-krane.dtsi => mt8183-kukui-krane.dts} | 47 ++-
> drivers/of/Kconfig | 13 +
> drivers/of/Makefile | 1 +
> drivers/of/base.c | 20 ++
> drivers/of/hw_prober.c | 314 ++++++++++++++++++
> include/linux/of.h | 6 +
> 11 files changed, 418 insertions(+), 57 deletions(-)
> delete mode 100644 arch/arm64/boot/dts/mediatek/mt8183-kukui-krane-sku0.dts
> delete mode 100644 arch/arm64/boot/dts/mediatek/mt8183-kukui-krane-sku176.dts
> rename arch/arm64/boot/dts/mediatek/{mt8183-kukui-krane.dtsi => mt8183-kukui-krane.dts} (86%)
> create mode 100644 drivers/of/hw_prober.c
>
^ permalink raw reply
* [RFC PATCH v2 7/7] arm64: dts: mediatek: mt8183-kukui: Merge Krane device trees
From: Chen-Yu Tsai @ 2023-11-09 10:06 UTC (permalink / raw)
To: Rob Herring, Frank Rowand, Krzysztof Kozlowski, Conor Dooley,
Matthias Brugger, AngeloGioacchino Del Regno
Cc: Hsin-Yi Wang, Dmitry Torokhov, andriy.shevchenko, Jiri Kosina,
linus.walleij, broonie, gregkh, hdegoede, james.clark, james,
keescook, petr.tesarik.ext, rafael, tglx, Jeff LaBundy,
linux-input, Chen-Yu Tsai, devicetree, linux-arm-kernel,
linux-mediatek, linux-kernel, Douglas Anderson, Johan Hovold
In-Reply-To: <20231109100606.1245545-1-wenst@chromium.org>
In cases where the same Chromebook model is manufactured with different
components (MIPI DSI panels, MIPI CSI camera sensors, or trackpad /
touchscreens with conflicting addresses), a different SKU ID is
allocated to each specific combination. This SKU ID is exported by the
bootloader into the device tree, and can be used to "discover" which
combination is present on the current machine.
Merge the separate Krane dtsi/dts files into one shared for all SKUs.
A new device node is added for the alternative panel. Both it and the
original panel are marked as "fail-needs-probe-panel" to let the
hardware prober handle it.
Also move the cros_ec node so that all node references are ordered
alphabetically.
Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
arch/arm64/boot/dts/mediatek/Makefile | 3 +-
.../dts/mediatek/mt8183-kukui-krane-sku0.dts | 24 ----------
.../mediatek/mt8183-kukui-krane-sku176.dts | 24 ----------
...ukui-krane.dtsi => mt8183-kukui-krane.dts} | 47 +++++++++++++++++--
4 files changed, 44 insertions(+), 54 deletions(-)
delete mode 100644 arch/arm64/boot/dts/mediatek/mt8183-kukui-krane-sku0.dts
delete mode 100644 arch/arm64/boot/dts/mediatek/mt8183-kukui-krane-sku176.dts
rename arch/arm64/boot/dts/mediatek/{mt8183-kukui-krane.dtsi => mt8183-kukui-krane.dts} (86%)
diff --git a/arch/arm64/boot/dts/mediatek/Makefile b/arch/arm64/boot/dts/mediatek/Makefile
index 7e365e9516ab..d4d97b315b2f 100644
--- a/arch/arm64/boot/dts/mediatek/Makefile
+++ b/arch/arm64/boot/dts/mediatek/Makefile
@@ -40,8 +40,7 @@ dtb-$(CONFIG_ARCH_MEDIATEK) += mt8183-kukui-kodama-sku16.dtb
dtb-$(CONFIG_ARCH_MEDIATEK) += mt8183-kukui-kodama-sku272.dtb
dtb-$(CONFIG_ARCH_MEDIATEK) += mt8183-kukui-kodama-sku288.dtb
dtb-$(CONFIG_ARCH_MEDIATEK) += mt8183-kukui-kodama-sku32.dtb
-dtb-$(CONFIG_ARCH_MEDIATEK) += mt8183-kukui-krane-sku0.dtb
-dtb-$(CONFIG_ARCH_MEDIATEK) += mt8183-kukui-krane-sku176.dtb
+dtb-$(CONFIG_ARCH_MEDIATEK) += mt8183-kukui-krane.dtb
dtb-$(CONFIG_ARCH_MEDIATEK) += mt8183-pumpkin.dtb
dtb-$(CONFIG_ARCH_MEDIATEK) += mt8186-corsola-magneton-sku393216.dtb
dtb-$(CONFIG_ARCH_MEDIATEK) += mt8186-corsola-magneton-sku393217.dtb
diff --git a/arch/arm64/boot/dts/mediatek/mt8183-kukui-krane-sku0.dts b/arch/arm64/boot/dts/mediatek/mt8183-kukui-krane-sku0.dts
deleted file mode 100644
index 4ac75806fa94..000000000000
--- a/arch/arm64/boot/dts/mediatek/mt8183-kukui-krane-sku0.dts
+++ /dev/null
@@ -1,24 +0,0 @@
-// SPDX-License-Identifier: (GPL-2.0 OR MIT)
-/*
- * Copyright 2019 Google LLC
- *
- * Device-tree for Krane sku0.
- *
- * SKU is a 8-bit value (0x00 == 0):
- * - Bits 7..4: Panel ID: 0x0 (AUO)
- * - Bits 3..0: SKU ID: 0x0 (default)
- */
-
-/dts-v1/;
-#include "mt8183-kukui-krane.dtsi"
-
-/ {
- model = "MediaTek krane sku0 board";
- chassis-type = "tablet";
- compatible = "google,krane-sku0", "google,krane", "mediatek,mt8183";
-};
-
-&panel {
- status = "okay";
- compatible = "auo,kd101n80-45na";
-};
diff --git a/arch/arm64/boot/dts/mediatek/mt8183-kukui-krane-sku176.dts b/arch/arm64/boot/dts/mediatek/mt8183-kukui-krane-sku176.dts
deleted file mode 100644
index 095279e55d50..000000000000
--- a/arch/arm64/boot/dts/mediatek/mt8183-kukui-krane-sku176.dts
+++ /dev/null
@@ -1,24 +0,0 @@
-// SPDX-License-Identifier: (GPL-2.0 OR MIT)
-/*
- * Copyright 2019 Google LLC
- *
- * Device-tree for Krane sku176.
- *
- * SKU is a 8-bit value (0xb0 == 176):
- * - Bits 7..4: Panel ID: 0xb (BOE)
- * - Bits 3..0: SKU ID: 0x0 (default)
- */
-
-/dts-v1/;
-#include "mt8183-kukui-krane.dtsi"
-
-/ {
- model = "MediaTek krane sku176 board";
- chassis-type = "tablet";
- compatible = "google,krane-sku176", "google,krane", "mediatek,mt8183";
-};
-
-&panel {
- status = "okay";
- compatible = "boe,tv101wum-nl6";
-};
diff --git a/arch/arm64/boot/dts/mediatek/mt8183-kukui-krane.dtsi b/arch/arm64/boot/dts/mediatek/mt8183-kukui-krane.dts
similarity index 86%
rename from arch/arm64/boot/dts/mediatek/mt8183-kukui-krane.dtsi
rename to arch/arm64/boot/dts/mediatek/mt8183-kukui-krane.dts
index d5f41c6c9881..75a734c0fbcc 100644
--- a/arch/arm64/boot/dts/mediatek/mt8183-kukui-krane.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8183-kukui-krane.dts
@@ -1,12 +1,24 @@
// SPDX-License-Identifier: (GPL-2.0 OR MIT)
/*
* Copyright 2019 Google LLC
+ *
+ * Device tree for Krane Chromebook family.
+ *
+ * SKU ID is a 8-bit value (0x00 == 0):
+ * - Bits 7..4: Panel ID: 0x0 (AUO)
+ * 0xb (BOE)
+ * - Bits 3..0: SKU ID: 0x0 (default)
*/
+/dts-v1/;
#include "mt8183-kukui.dtsi"
#include "mt8183-kukui-audio-max98357a.dtsi"
/ {
+ model = "Google Krane Chromebook";
+ chassis-type = "tablet";
+ compatible = "google,krane", "mediatek,mt8183";
+
ppvarn_lcd: ppvarn-lcd {
compatible = "regulator-fixed";
regulator-name = "ppvarn_lcd";
@@ -45,6 +57,34 @@ &bluetooth {
firmware-name = "nvm_00440302_i2s_eu.bin";
};
+&cros_ec {
+ keyboard-controller {
+ compatible = "google,cros-ec-keyb-switches";
+ };
+};
+
+&dsi0 {
+ panel2@0 {
+ compatible = "boe,tv101wum-nl6";
+ reg = <0>;
+ enable-gpios = <&pio 45 0>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&panel_pins_default>;
+ avdd-supply = <&ppvarn_lcd>;
+ avee-supply = <&ppvarp_lcd>;
+ pp1800-supply = <&pp1800_lcd>;
+ backlight = <&backlight_lcd0>;
+ rotation = <270>;
+ status = "fail-needs-probe-panel";
+
+ port {
+ endpoint {
+ remote-endpoint = <&dsi_out>;
+ };
+ };
+ };
+};
+
&i2c0 {
status = "okay";
@@ -343,10 +383,9 @@ rst_pin {
};
};
-&cros_ec {
- keyboard-controller {
- compatible = "google,cros-ec-keyb-switches";
- };
+&panel {
+ compatible = "auo,kd101n80-45na";
+ status = "fail-needs-probe-panel";
};
&qca_wifi {
--
2.42.0.869.gea05f2083d-goog
^ permalink raw reply related
* [RFC PATCH v2 6/7] dt-bindings: arm: mediatek: Remove SKU specific compatibles for Google Krane
From: Chen-Yu Tsai @ 2023-11-09 10:06 UTC (permalink / raw)
To: Rob Herring, Frank Rowand, Krzysztof Kozlowski, Conor Dooley,
Matthias Brugger, AngeloGioacchino Del Regno
Cc: Hsin-Yi Wang, Dmitry Torokhov, andriy.shevchenko, Jiri Kosina,
linus.walleij, broonie, gregkh, hdegoede, james.clark, james,
keescook, petr.tesarik.ext, rafael, tglx, Jeff LaBundy,
linux-input, Chen-Yu Tsai, devicetree, linux-arm-kernel,
linux-mediatek, linux-kernel, Douglas Anderson, Johan Hovold
In-Reply-To: <20231109100606.1245545-1-wenst@chromium.org>
In cases where the same Chromebook model is manufactured with different
components (MIPI DSI panels, MIPI CSI camera sensors, or trackpad /
touchscreens with conflicting addresses), a different SKU ID is
allocated to each specific combination. This SKU ID is exported by the
bootloader into the device tree, and can be used to "discover" which
combination is present on the current machine. Thus we no longer have
to specify separate compatible strings for each of them.
Remove the SKU specific compatible strings for Google Krane.
Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
Documentation/devicetree/bindings/arm/mediatek.yaml | 3 ---
1 file changed, 3 deletions(-)
diff --git a/Documentation/devicetree/bindings/arm/mediatek.yaml b/Documentation/devicetree/bindings/arm/mediatek.yaml
index a4541855a838..ef3dfb286814 100644
--- a/Documentation/devicetree/bindings/arm/mediatek.yaml
+++ b/Documentation/devicetree/bindings/arm/mediatek.yaml
@@ -186,9 +186,6 @@ properties:
- const: mediatek,mt8183
- description: Google Krane (Lenovo IdeaPad Duet, 10e,...)
items:
- - enum:
- - google,krane-sku0
- - google,krane-sku176
- const: google,krane
- const: mediatek,mt8183
- description: Google Willow (Acer Chromebook 311 C722/C722T)
--
2.42.0.869.gea05f2083d-goog
^ permalink raw reply related
* [RFC PATCH v2 5/7] of: hw_prober: Support Chromebook SKU ID based component selection
From: Chen-Yu Tsai @ 2023-11-09 10:06 UTC (permalink / raw)
To: Rob Herring, Frank Rowand, Krzysztof Kozlowski, Conor Dooley,
Matthias Brugger, AngeloGioacchino Del Regno
Cc: Hsin-Yi Wang, Dmitry Torokhov, andriy.shevchenko, Jiri Kosina,
linus.walleij, broonie, gregkh, hdegoede, james.clark, james,
keescook, petr.tesarik.ext, rafael, tglx, Jeff LaBundy,
linux-input, Chen-Yu Tsai, devicetree, linux-arm-kernel,
linux-mediatek, linux-kernel, Douglas Anderson, Johan Hovold
In-Reply-To: <20231109100606.1245545-1-wenst@chromium.org>
In cases where the same Chromebook model is manufactured with different
components (MIPI DSI panels, MIPI CSI camera sensors, or trackpad /
touchscreens with conflicting addresses), a different SKU ID is
allocated to each specific combination. This SKU ID is exported by the
bootloader into the device tree, and can be used to "discover" which
combination is present on the current machine.
This change adds a hardware prober that will match the SKU ID against
a provided table, and enable the component for the matched entry based
on the given compatible string. In the MIPI DSI panel and MIPI CSI
camera sensor cases which have OF graphs, it will also update the
remote endpoint to point to the enabled component. This assumes a single
endpoint only.
This will provide a path to reducing the number of Chromebook device
trees.
Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
drivers/of/hw_prober.c | 160 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 160 insertions(+)
diff --git a/drivers/of/hw_prober.c b/drivers/of/hw_prober.c
index 442da6eff896..4345e5aed6d8 100644
--- a/drivers/of/hw_prober.c
+++ b/drivers/of/hw_prober.c
@@ -8,6 +8,7 @@
#include <linux/array_size.h>
#include <linux/i2c.h>
#include <linux/of.h>
+#include <linux/of_graph.h>
#include <linux/platform_device.h>
#define DRV_NAME "hw_prober"
@@ -108,9 +109,168 @@ static int i2c_component_prober(struct platform_device *pdev, const void *data)
return ret;
}
+static int cros_get_coreboot_sku_id(struct device *dev, u32 *sku_id)
+{
+ struct device_node *node = NULL;
+ int ret;
+
+ node = of_find_node_by_path("/firmware/coreboot");
+ if (!node)
+ return dev_err_probe(dev, -EINVAL, "Cannot find coreboot firmware node\n");
+
+ ret = of_property_read_u32(node, "sku-id", sku_id);
+ if (ret)
+ dev_err_probe(dev, ret, "Cannot get SKU ID\n");
+
+ of_node_put(node);
+ return ret;
+}
+
+struct cros_sku_option {
+ u32 sku_id_val;
+ u32 sku_id_mask;
+ const char *compatible;
+};
+
+struct cros_sku_component_data {
+ const struct cros_sku_option *options;
+ int num_options;
+};
+
+/*
+ * cros_sku_component_selector - Selectively enable a component based on SKU ID
+ *
+ * Based on the list of component options and SKU ID read back from the device
+ * tree, enable the matching component. Also update the OF graph if it exists,
+ * so that the enabled component's remote endpoint correctly points to it. This
+ * assumes a single local endpoint, which should be the case for panels and
+ * camera sensors.
+ */
+static int cros_sku_component_selector(struct platform_device *pdev, const void *data)
+{
+ const struct cros_sku_component_data *pdata = data;
+ const char *compatible;
+ struct device_node *node = NULL, *endpoint = NULL, *remote = NULL;
+ struct property *status_prop = NULL, *endpoint_prop = NULL;
+ struct of_changeset *ocs = NULL;
+ __be32 *val = NULL;
+ int ret, i;
+ u32 sku_id;
+
+ if (!data)
+ return dev_err_probe(&pdev->dev, -EINVAL, "No data given\n");
+
+ ret = cros_get_coreboot_sku_id(&pdev->dev, &sku_id);
+ if (ret)
+ return ret;
+
+ for (i = 0; i < pdata->num_options; i++)
+ if ((sku_id & pdata->options[i].sku_id_mask) == pdata->options[i].sku_id_val) {
+ compatible = pdata->options->compatible;
+ break;
+ }
+
+ if (i == pdata->num_options)
+ return dev_err_probe(&pdev->dev, -EINVAL, "Unknown SKU ID: 0x%x\n", sku_id);
+
+ node = of_find_compatible_node(NULL, NULL, compatible);
+ if (!node)
+ return dev_err_probe(&pdev->dev, -ENODEV, "Cannot find matching device node\n");
+
+ /* device node not marked as fail; don't mess with the device tree */
+ if (!of_device_is_fail(node))
+ goto err_free;
+
+ dev_info(&pdev->dev, "Enabling %pOF for SKU 0x%x\n", node, sku_id);
+
+ ret = -ENOMEM;
+ ocs = kzalloc(sizeof(*ocs), GFP_KERNEL);
+ if (!ocs)
+ goto err_free;
+
+ status_prop = kzalloc(sizeof(*status_prop), GFP_KERNEL);
+ if (!status_prop)
+ goto err_free;
+
+ status_prop->name = "status";
+ status_prop->length = 5;
+ status_prop->value = "okay";
+
+ /* Create changeset to apply DT changes atomically */
+ of_changeset_init(ocs);
+
+ if (of_graph_is_present(node)) {
+ ret = -EINVAL;
+
+ /* This currently assumes a single port on the component. */
+ endpoint = of_graph_get_next_endpoint(node, NULL);
+ if (!endpoint) {
+ dev_err(&pdev->dev, "No endpoint found for %pOF\n", node);
+ goto err_destroy_ocs;
+ }
+
+ remote = of_graph_get_remote_endpoint(endpoint);
+ if (!remote) {
+ dev_err(&pdev->dev, "No remote endpoint node found for %pOF\n", endpoint);
+ goto err_destroy_ocs;
+ }
+
+ endpoint_prop = kzalloc(sizeof(*endpoint_prop), GFP_KERNEL);
+ if (!endpoint_prop)
+ goto err_destroy_ocs;
+
+ val = kzalloc(sizeof(*val), GFP_KERNEL);
+ if (!val)
+ goto err_destroy_ocs;
+
+ *val = cpu_to_be32(endpoint->phandle);
+ endpoint_prop->name = "remote-endpoint";
+ endpoint_prop->length = sizeof(*val);
+ endpoint_prop->value = val;
+
+ ret = of_changeset_update_property(ocs, node, endpoint_prop);
+ if (ret)
+ goto err_destroy_ocs;
+ }
+
+ ret = of_changeset_update_property(ocs, node, status_prop);
+ if (ret)
+ goto err_destroy_ocs;
+ ret = of_changeset_apply(ocs);
+ if (ret)
+ goto err_destroy_ocs;
+
+ of_node_put(node);
+
+ return 0;
+
+err_destroy_ocs:
+ of_node_put(remote);
+ of_node_put(endpoint);
+ kfree(val);
+ kfree(endpoint_prop);
+ of_changeset_destroy(ocs);
+err_free:
+ kfree(ocs);
+ kfree(status_prop);
+ of_node_put(node);
+ return ret;
+}
+
+static const struct cros_sku_option cros_krane_panel_options[] = {
+ { .sku_id_val = 0x00, .sku_id_mask = 0xf0, .compatible = "auo,kd101n80-45na" },
+ { .sku_id_val = 0xb0, .sku_id_mask = 0xf0, .compatible = "boe,tv101wum-nl6" },
+};
+
+static const struct cros_sku_component_data cros_krane_panel_data = {
+ .options = cros_krane_panel_options,
+ .num_options = ARRAY_SIZE(cros_krane_panel_options),
+};
+
static const struct hw_prober_entry hw_prober_platforms[] = {
{ .compatible = "google,hana", .prober = i2c_component_prober, .data = "touchscreen" },
{ .compatible = "google,hana", .prober = i2c_component_prober, .data = "trackpad" },
+ { .compatible = "google,krane", .prober = cros_sku_component_selector, .data = &cros_krane_panel_data },
};
static int hw_prober_probe(struct platform_device *pdev)
--
2.42.0.869.gea05f2083d-goog
^ permalink raw reply related
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