* [RFC PATCH v3 5/5] MAINTAINERS: add myself for Marvell 88PM886 PMIC
From: Karel Balej @ 2024-03-03 10:04 UTC (permalink / raw)
To: Karel Balej, Lee Jones, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Dmitry Torokhov, Liam Girdwood, Mark Brown,
devicetree, linux-kernel, linux-input
Cc: Duje Mihanović, ~postmarketos/upstreaming, phone-devel
In-Reply-To: <20240303101506.4187-1-karelb@gimli.ms.mff.cuni.cz>
From: Karel Balej <balejk@matfyz.cz>
Add an entry to MAINTAINERS for the Marvell 88PM886 PMIC MFD, onkey and
regulator drivers.
Signed-off-by: Karel Balej <balejk@matfyz.cz>
---
Notes:
RFC v3:
- Remove onkey bindings file.
RFC v2:
- Only mention 88PM886 in the commit message.
- Add regulator driver.
- Rename the entry.
MAINTAINERS | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 960512bec428..944f88c92df6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12949,6 +12949,15 @@ F: drivers/net/dsa/mv88e6xxx/
F: include/linux/dsa/mv88e6xxx.h
F: include/linux/platform_data/mv88e6xxx.h
+MARVELL 88PM886 PMIC DRIVER
+M: Karel Balej <balejk@matfyz.cz>
+S: Maintained
+F: Documentation/devicetree/bindings/mfd/marvell,88pm886-a1.yaml
+F: drivers/input/misc/88pm886-onkey.c
+F: drivers/mfd/88pm886.c
+F: drivers/regulators/88pm886-regulator.c
+F: include/linux/mfd/88pm886.h
+
MARVELL ARMADA 3700 PHY DRIVERS
M: Miquel Raynal <miquel.raynal@bootlin.com>
S: Maintained
--
2.44.0
^ permalink raw reply related
* Re: [RFC PATCH v3 4/5] input: add onkey driver for Marvell 88PM886 PMIC
From: Dmitry Torokhov @ 2024-03-03 20:39 UTC (permalink / raw)
To: Karel Balej
Cc: Karel Balej, Lee Jones, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Liam Girdwood, Mark Brown, devicetree, linux-kernel,
linux-input, Duje Mihanović, ~postmarketos/upstreaming,
phone-devel
In-Reply-To: <20240303101506.4187-5-karelb@gimli.ms.mff.cuni.cz>
Hi Karel,
On Sun, Mar 03, 2024 at 11:04:25AM +0100, Karel Balej wrote:
> From: Karel Balej <balejk@matfyz.cz>
>
> Marvell 88PM886 PMIC provides onkey among other things. Add client
> driver to handle it. The driver currently only provides a basic support
> omitting additional functions found in the vendor version, such as long
> onkey and GPIO integration.
>
> Signed-off-by: Karel Balej <balejk@matfyz.cz>
> ---
>
> Notes:
> RFC v3:
> - Drop wakeup-source.
> RFC v2:
> - Address Dmitry's feedback:
> - Sort includes alphabetically.
> - Drop onkey->irq.
> - ret -> err in irq_handler and no initialization.
> - Break long lines and other formatting.
> - Do not clobber platform_get_irq error.
> - Do not set device parent manually.
> - Use input_set_capability.
> - Use the wakeup-source DT property.
> - Drop of_match_table.
I only said that you should not be using of_match_ptr(), but you still
need to have of_match_table set and have MODULE_DEVICE_TABLE() for the
proper module loading support.
With that fixed:
Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] Input: iqs269a - Use common error handling code in iqs269_parse_chan()
From: Dmitry Torokhov @ 2024-03-03 22:31 UTC (permalink / raw)
To: Markus Elfring
Cc: linux-input, kernel-janitors, Jeff LaBundy, Mattijs Korpershoek,
Uwe Kleine-König, ye xingchen, LKML
In-Reply-To: <6bf9f962-cf75-459d-89f4-2546063fc154@web.de>
Hi Markus,
On Sat, Mar 02, 2024 at 09:24:53AM +0100, Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 2 Mar 2024 09:15:20 +0100
>
> Add a jump target so that a bit of exception handling can be better reused
> at the end of this function implementation.
No, I do not think this is needed. However if you can introduce a fwnode
cleanup/free function (see include/linux/cleanup.h) maybe it would be
more useful and we could apply it to various drivers.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] Input: leds: Change config symbol dependency for audio mute trigger
From: Dmitry Torokhov @ 2024-03-03 22:36 UTC (permalink / raw)
To: Heiner Kallweit
Cc: open list:HID CORE LAYER, linux-leds@vger.kernel.org, Lee Jones
In-Reply-To: <6011ca63-187c-42dd-a5fd-7dd733d6257c@gmail.com>
On Sat, Mar 02, 2024 at 04:24:28PM +0100, Heiner Kallweit wrote:
> In a follow-up patch implementation of the LED audio trigger will be
> moved to sound/core/snd_ctl_led, including removal of config symbol
> LEDS_AUDIO_TRIGGER. Also as of today the audio mute LED trigger
> is effectively a no-op w/o config symbol SND_CTL_LED being defined.
> Therefore switch the dependency to this config symbol.
>
> Suggested-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
Applied, thank you.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] Input: xpad - Add additional HyperX Controller Identifiers
From: Dmitry Torokhov @ 2024-03-03 22:45 UTC (permalink / raw)
To: Nguyen, Max; +Cc: linux-input
In-Reply-To: <44ad5ffa-76d8-4046-94ee-2ef171930ed2@gmail.com>
Hi Max,
On Fri, Mar 01, 2024 at 06:15:42PM -0800, Nguyen, Max wrote:
> Add additional HyperX device identifiers to xpad_device and xpad_table. Cc:
> stable@vger.kernel.org Suggested-by: Chris Toledanes<chris.toledanes@hp.com>
> Reviewed-by: Carl Ng<carl.ng@hp.com>
> Signed-off-by: Max Nguyen<maxwell.nguyen@hp.com>
Your client messes up patches :(
> ---
>
> diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
> index 7c4b2a5cc1b5..a7e001a1f6dc 100644
> --- a/drivers/input/joystick/xpad.c
> +++ b/drivers/input/joystick/xpad.c
> @@ -131,6 +131,11 @@ static const struct xpad_device {
> { 0x03eb, 0xff01, "Wooting One (Legacy)", 0, XTYPE_XBOX360 },
> { 0x03eb, 0xff02, "Wooting Two (Legacy)", 0, XTYPE_XBOX360 },
> { 0x03f0, 0x0495, "HyperX Clutch Gladiate", 0, XTYPE_XBOXONE },
> + { 0x03f0, 0x08B6, "HyperX Clutch Gladiate", 0, XTYPE_XBOXONE }, /* v2 */
> + { 0x03f0, 0x07A0, "HyperX Clutch Gladiate RGB", 0, XTYPE_XBOXONE },
> + { 0x03f0, 0x038D, "HyperX Clutch", 0, XTYPE_XBOX360 }, /* wired */
> + { 0x03f0, 0x048D, "HyperX Clutch", 0, XTYPE_XBOX360 }, /* wireless */
> + { 0x03f0, 0x09B4, "HyperX Clutch Tanto", 0, XTYPE_XBOXONE },
These need to be sorted by VID/PID.
I fixed it up and applied.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [RESEND PATCH v5 0/5] input/touchscreen: imagis: add support for IST3032C
From: Dmitry Torokhov @ 2024-03-03 22:58 UTC (permalink / raw)
To: Karel Balej
Cc: Markuss Broks, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Henrik Rydberg, linux-input, devicetree, linux-kernel,
Duje Mihanović, ~postmarketos/upstreaming, phone-devel
In-Reply-To: <20240301164659.13240-1-karelb@gimli.ms.mff.cuni.cz>
On Fri, Mar 01, 2024 at 05:40:59PM +0100, Karel Balej wrote:
> From: Karel Balej <balejk@matfyz.cz>
>
> Hello,
>
> this patch series generalizes the Imagis touchscreen driver to support
> other Imagis chips, namely IST3038B and IST3032C.
>
> The motivation for IST3032C is the samsung,coreprimevelte smartphone
> with which this series has been tested. However, the support for this
> device is not yet in-tree, the effort is happening at [1]. Preliminary
> version of the regulator driver needed to use the touchscreen on this
> phone can be found here [2].
>
> Note that this is a prerequisite for (at least a part of) this series
> [3] which among other things implements support for touch keys for
> Imagis touchscreens that have it.
>
> [1] https://lore.kernel.org/all/20240110-pxa1908-lkml-v8-0-fea768a59474@skole.hr/
> [2] https://lore.kernel.org/all/20240211094609.2223-1-karelb@gimli.ms.mff.cuni.cz/
> [3] https://lore.kernel.org/all/20240120-b4-imagis-keys-v2-0-d7fc16f2e106@skole.hr/
Applied the lot, thank you.
--
Dmitry
^ permalink raw reply
* Re: [PATCH -next] Input: Fix kernel-doc for xps2_of_probe function
From: Dmitry Torokhov @ 2024-03-03 23:03 UTC (permalink / raw)
To: Yang Li; +Cc: michal.simek, linux-input, linux-arm-kernel, linux-kernel
In-Reply-To: <20240301092115.123092-1-yang.lee@linux.alibaba.com>
On Fri, Mar 01, 2024 at 05:21:15PM +0800, Yang Li wrote:
> The existing comment block above the xps2_of_probe function
> does not conform to the kernel-doc standard. This patch fixes the
> documentation to match the expected kernel-doc format, which includes
> a structured documentation header with param and return value.
>
> Signed-off-by: Yang Li <yang.lee@linux.alibaba.com>
Applied, thank you.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 1/2] Input: synaptics-rmi4 - make rmi_bus_type const
From: Dmitry Torokhov @ 2024-03-03 23:07 UTC (permalink / raw)
To: Ricardo B. Marliere; +Cc: linux-input, linux-kernel, Greg Kroah-Hartman
In-Reply-To: <20240210-bus_cleanup-input2-v1-1-0daef7e034e0@marliere.net>
On Sat, Feb 10, 2024 at 12:15:01PM -0300, Ricardo B. Marliere wrote:
> Now that the driver core can properly handle constant struct bus_type,
> move the variable rmi_bus_type to be a constant structure as well,
> placing it into read-only memory which can not be modified at runtime.
>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Ricardo B. Marliere <ricardo@marliere.net>
Applied, thank you.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 2/2] Input: serio - make serio_bus const
From: Dmitry Torokhov @ 2024-03-03 23:07 UTC (permalink / raw)
To: Ricardo B. Marliere; +Cc: linux-input, linux-kernel, Greg Kroah-Hartman
In-Reply-To: <20240210-bus_cleanup-input2-v1-2-0daef7e034e0@marliere.net>
On Sat, Feb 10, 2024 at 12:15:02PM -0300, Ricardo B. Marliere wrote:
> Now that the driver core can properly handle constant struct bus_type,
> move the serio_bus variable to be a constant structure as well,
> placing it into read-only memory which can not be modified at runtime.
>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Ricardo B. Marliere <ricardo@marliere.net>
Applied, thank you.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] Input: iqs626a - Use common error handling code in iqs626_parse_events()
From: Jeff LaBundy @ 2024-03-04 3:12 UTC (permalink / raw)
To: Markus Elfring
Cc: linux-input, kernel-janitors, Dmitry Torokhov, Rob Herring,
Uwe Kleine-König, LKML
In-Reply-To: <8a7607f8-d634-415e-8269-e26dcc0f9fdc@web.de>
Hi Markus,
On Sat, Mar 02, 2024 at 12:42:08PM +0100, Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 2 Mar 2024 11:44:17 +0100
>
> Add a jump target so that a bit of exception handling can be better reused
> at the end of this function implementation.
>
> This issue was transformed by using the Coccinelle software.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
> drivers/input/misc/iqs626a.c | 13 +++++++------
> 1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/input/misc/iqs626a.c b/drivers/input/misc/iqs626a.c
> index 0dab54d3a060..fa9570755f7b 100644
> --- a/drivers/input/misc/iqs626a.c
> +++ b/drivers/input/misc/iqs626a.c
> @@ -530,8 +530,7 @@ iqs626_parse_events(struct iqs626_private *iqs626,
> dev_err(&client->dev,
> "Invalid input type: %u\n",
> val);
> - fwnode_handle_put(ev_node);
> - return -EINVAL;
> + goto put_fwnode;
> }
>
> iqs626->kp_type[ch_id][i] = val;
> @@ -545,8 +544,7 @@ iqs626_parse_events(struct iqs626_private *iqs626,
> dev_err(&client->dev,
> "Invalid %s channel hysteresis: %u\n",
> fwnode_get_name(ch_node), val);
> - fwnode_handle_put(ev_node);
> - return -EINVAL;
> + goto put_fwnode;
> }
>
> if (i == IQS626_EVENT_DEEP_DN ||
> @@ -566,8 +564,7 @@ iqs626_parse_events(struct iqs626_private *iqs626,
> dev_err(&client->dev,
> "Invalid %s channel threshold: %u\n",
> fwnode_get_name(ch_node), val);
> - fwnode_handle_put(ev_node);
> - return -EINVAL;
> + goto put_fwnode;
> }
>
> if (ch_id == IQS626_CH_HALL)
> @@ -580,6 +577,10 @@ iqs626_parse_events(struct iqs626_private *iqs626,
> }
>
> return 0;
> +
> +put_fwnode:
> + fwnode_handle_put(ev_node);
> + return -EINVAL;
> }
>
> static noinline_for_stack int
> --
> 2.44.0
>
Thank you for this patch, but it seems like a NAK to me. I think this is
a matter of personal preference, and according to mine, it is much more
confusing to insert a goto label after a function's primary return path
than to have 2-3 calls to fwnode_handle_put().
If you feel strongly otherwise, then I would suggest a helper function as
recommended by Dmitry in another thread. However, maybe that helper should
live in the driver core, as I suspect this driver is not the only place we
can avoid calling fwnode_handle_put() in an error path that returns an int.
Kind regards,
Jeff LaBundy
^ permalink raw reply
* Re: [PATCH v7 2/4] Input: touch-overlay - Add touchscreen overlay handling
From: Jeff LaBundy @ 2024-03-04 3:42 UTC (permalink / raw)
To: Javier Carrasco
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Henrik Rydberg, Bastian Hecht, Michael Riesch, linux-kernel,
linux-input, devicetree
In-Reply-To: <20240119-feature-ts_virtobj_patch-v7-2-eda70985808f@wolfvision.net>
Hi Javier,
On Fri, Jan 19, 2024 at 08:43:34AM +0100, Javier Carrasco wrote:
> Some touch devices provide mechanical overlays with different objects
> like buttons or clipped touchscreen surfaces.
>
> In order to support these objects, add a series of helper functions
> to the input subsystem to transform them into overlay objects via
> device tree nodes.
>
> These overlay objects consume the raw touch events and report the
> expected input events depending on the object properties.
>
> Note that the current implementation allows for multiple definitions
> of touchscreen areas (regions that report touch events), but only the
> first one will be used for the touchscreen device that the consumers
> typically provide.
> Should the need for multiple touchscreen areas arise, additional
> touchscreen devices would be required at the consumer side.
> There is no limitation in the number of touch areas defined as buttons.
>
> Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
Apologies for the delay, but I think this looks great now. It can always
be extended later in case another touchscreen driver needs additional
functionality or visibility. You've addressed all of my feedback, and so:
Reviewed-by: Jeff LaBundy <jeff@labundy.com>
> ---
> MAINTAINERS | 7 +
> drivers/input/Makefile | 2 +-
> drivers/input/touch-overlay.c | 250 ++++++++++++++++++++++++++++++++++++
> include/linux/input/touch-overlay.h | 22 ++++
> 4 files changed, 280 insertions(+), 1 deletion(-)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index a7c4cf8201e0..668687bf94df 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -21961,6 +21961,13 @@ L: platform-driver-x86@vger.kernel.org
> S: Maintained
> F: drivers/platform/x86/toshiba-wmi.c
>
> +TOUCH OVERLAY
> +M: Javier Carrasco <javier.carrasco@wolfvision.net>
> +L: linux-input@vger.kernel.org
> +S: Maintained
> +F: drivers/input/touch-overlay.c
> +F: include/linux/input/touch-overlay.h
> +
> TPM DEVICE DRIVER
> M: Peter Huewe <peterhuewe@gmx.de>
> M: Jarkko Sakkinen <jarkko@kernel.org>
> diff --git a/drivers/input/Makefile b/drivers/input/Makefile
> index c78753274921..393e9f4d00dc 100644
> --- a/drivers/input/Makefile
> +++ b/drivers/input/Makefile
> @@ -7,7 +7,7 @@
>
> obj-$(CONFIG_INPUT) += input-core.o
> input-core-y := input.o input-compat.o input-mt.o input-poller.o ff-core.o
> -input-core-y += touchscreen.o
> +input-core-y += touchscreen.o touch-overlay.o
>
> obj-$(CONFIG_INPUT_FF_MEMLESS) += ff-memless.o
> obj-$(CONFIG_INPUT_SPARSEKMAP) += sparse-keymap.o
> diff --git a/drivers/input/touch-overlay.c b/drivers/input/touch-overlay.c
> new file mode 100644
> index 000000000000..42b6ad753a00
> --- /dev/null
> +++ b/drivers/input/touch-overlay.c
> @@ -0,0 +1,250 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Helper functions for overlay objects on touchscreens
> + *
> + * Copyright (c) 2023 Javier Carrasco <javier.carrasco@wolfvision.net>
> + */
> +
> +#include <linux/input.h>
> +#include <linux/input/mt.h>
> +#include <linux/input/touch-overlay.h>
> +#include <linux/list.h>
> +#include <linux/module.h>
> +#include <linux/property.h>
> +
> +struct touch_overlay_segment {
> + struct list_head list;
> + u32 x_origin;
> + u32 y_origin;
> + u32 x_size;
> + u32 y_size;
> + u32 key;
> + bool pressed;
> + int slot;
> +};
> +
> +static int touch_overlay_get_segment(struct fwnode_handle *segment_node,
> + struct touch_overlay_segment *segment,
> + struct input_dev *input)
> +{
> + int error;
> +
> + error = fwnode_property_read_u32(segment_node, "x-origin",
> + &segment->x_origin);
> + if (error)
> + return error;
> +
> + error = fwnode_property_read_u32(segment_node, "y-origin",
> + &segment->y_origin);
> + if (error)
> + return error;
> +
> + error = fwnode_property_read_u32(segment_node, "x-size",
> + &segment->x_size);
> + if (error)
> + return error;
> +
> + error = fwnode_property_read_u32(segment_node, "y-size",
> + &segment->y_size);
> + if (error)
> + return error;
> +
> + error = fwnode_property_read_u32(segment_node, "linux,code",
> + &segment->key);
> + if (!error)
> + input_set_capability(input, EV_KEY, segment->key);
> + else if (error != -EINVAL)
> + return error;
> +
> + return 0;
> +}
> +
> +/**
> + * touch_overlay_map - map overlay objects from the device tree and set
> + * key capabilities if buttons are defined.
> + * @list: pointer to the list that will hold the segments
> + * @input: pointer to the already allocated input_dev
> + *
> + * Returns 0 on success and error number otherwise.
> + *
> + * If buttons are defined, key capabilities are set accordingly.
> + */
> +int touch_overlay_map(struct list_head *list, struct input_dev *input)
> +{
> + struct fwnode_handle *overlay, *fw_segment;
> + struct device *dev = input->dev.parent;
> + struct touch_overlay_segment *segment;
> + int error;
> +
> + overlay = device_get_named_child_node(dev, "touch-overlay");
> + if (!overlay)
> + return 0;
> +
> + fwnode_for_each_child_node(overlay, fw_segment) {
> + segment = devm_kzalloc(dev, sizeof(*segment), GFP_KERNEL);
> + if (!segment) {
> + error = -ENOMEM;
> + fwnode_handle_put(overlay);
> + break;
> + }
> + error = touch_overlay_get_segment(fw_segment, segment, input);
> + if (error) {
> + fwnode_handle_put(overlay);
> + break;
> + }
> + list_add_tail(&segment->list, list);
> + }
> +
> + return error;
> +}
> +EXPORT_SYMBOL(touch_overlay_map);
> +
> +/**
> + * touch_overlay_get_touchscreen_abs - get abs size from the touchscreen area.
> + * @list: pointer to the list that holds the segments
> + * @x: horizontal abs
> + * @y: vertical abs
> + */
> +void touch_overlay_get_touchscreen_abs(struct list_head *list, u16 *x, u16 *y)
> +{
> + struct touch_overlay_segment *segment;
> + struct list_head *ptr;
> +
> + list_for_each(ptr, list) {
> + segment = list_entry(ptr, struct touch_overlay_segment, list);
> + if (!segment->key) {
> + *x = segment->x_size - 1;
> + *y = segment->y_size - 1;
> + break;
> + }
> + }
> +}
> +EXPORT_SYMBOL(touch_overlay_get_touchscreen_abs);
> +
> +static bool touch_overlay_segment_event(struct touch_overlay_segment *seg,
> + u32 x, u32 y)
> +{
> + if (!seg)
> + return false;
> +
> + if (x >= seg->x_origin && x < (seg->x_origin + seg->x_size) &&
> + y >= seg->y_origin && y < (seg->y_origin + seg->y_size))
> + return true;
> +
> + return false;
> +}
> +
> +/**
> + * touch_overlay_mapped_touchscreen - check if a touchscreen area is mapped
> + * @list: pointer to the list that holds the segments
> + *
> + * Returns true if a touchscreen area is mapped or false otherwise.
> + */
> +bool touch_overlay_mapped_touchscreen(struct list_head *list)
> +{
> + struct touch_overlay_segment *segment;
> + struct list_head *ptr;
> +
> + list_for_each(ptr, list) {
> + segment = list_entry(ptr, struct touch_overlay_segment, list);
> + if (!segment->key)
> + return true;
> + }
> +
> + return false;
> +}
> +EXPORT_SYMBOL(touch_overlay_mapped_touchscreen);
> +
> +static bool touch_overlay_event_on_ts(struct list_head *list, u32 *x, u32 *y)
> +{
> + struct touch_overlay_segment *segment;
> + struct list_head *ptr;
> + bool valid_touch = true;
> +
> + if (!x || !y)
> + return false;
> +
> + list_for_each(ptr, list) {
> + segment = list_entry(ptr, struct touch_overlay_segment, list);
> + if (segment->key)
> + continue;
> +
> + if (touch_overlay_segment_event(segment, *x, *y)) {
> + *x -= segment->x_origin;
> + *y -= segment->y_origin;
> + return true;
> + }
> + /* ignore touch events outside the defined area */
> + valid_touch = false;
> + }
> +
> + return valid_touch;
> +}
> +
> +static bool touch_overlay_button_event(struct input_dev *input,
> + struct touch_overlay_segment *segment,
> + const u32 *x, const u32 *y, u32 slot)
> +{
> + bool contact = x && y;
> +
> + if (!contact && segment->pressed && segment->slot == slot) {
> + segment->pressed = false;
> + input_report_key(input, segment->key, false);
> + input_sync(input);
> + return true;
> + } else if (contact && touch_overlay_segment_event(segment, *x, *y)) {
> + segment->pressed = true;
> + segment->slot = slot;
> + input_report_key(input, segment->key, true);
> + input_sync(input);
> + return true;
> + }
> +
> + return false;
> +}
> +
> +/**
> + * touch_overlay_process_event - process input events according to the overlay
> + * mapping. This function acts as a filter to release the calling driver from
> + * the events that are either related to overlay buttons or out of the overlay
> + * touchscreen area, if defined.
> + * @list: pointer to the list that holds the segments
> + * @input: pointer to the input device associated to the event
> + * @x: pointer to the x coordinate (NULL if not available - no contact)
> + * @y: pointer to the y coordinate (NULL if not available - no contact)
> + * @slot: slot associated to the event
> + *
> + * Returns true if the event was processed (reported for valid key events
> + * and dropped for events outside the overlay touchscreen area) or false
> + * if the event must be processed by the caller. In that case this function
> + * shifts the (x,y) coordinates to the overlay touchscreen axis if required.
> + */
> +bool touch_overlay_process_event(struct list_head *list,
> + struct input_dev *input,
> + u32 *x, u32 *y, u32 slot)
> +{
> + struct touch_overlay_segment *segment;
> + struct list_head *ptr;
> +
> + /*
> + * buttons must be prioritized over overlay touchscreens to account for
> + * overlappings e.g. a button inside the touchscreen area.
> + */
> + list_for_each(ptr, list) {
> + segment = list_entry(ptr, struct touch_overlay_segment, list);
> + if (segment->key &&
> + touch_overlay_button_event(input, segment, x, y, slot)) {
> + return true;
> + }
> + }
> +
> + /*
> + * valid touch events on the overlay touchscreen are left for the client
> + * to be processed/reported according to its (possibly) unique features.
> + */
> + return !touch_overlay_event_on_ts(list, x, y);
> +}
> +EXPORT_SYMBOL(touch_overlay_process_event);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Helper functions for overlay objects on touch devices");
> diff --git a/include/linux/input/touch-overlay.h b/include/linux/input/touch-overlay.h
> new file mode 100644
> index 000000000000..cef05c46000d
> --- /dev/null
> +++ b/include/linux/input/touch-overlay.h
> @@ -0,0 +1,22 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (c) 2023 Javier Carrasco <javier.carrasco@wolfvision.net>
> + */
> +
> +#ifndef _TOUCH_OVERLAY
> +#define _TOUCH_OVERLAY
> +
> +#include <linux/types.h>
> +
> +struct input_dev;
> +
> +int touch_overlay_map(struct list_head *list, struct input_dev *input);
> +
> +void touch_overlay_get_touchscreen_abs(struct list_head *list, u16 *x, u16 *y);
> +
> +bool touch_overlay_mapped_touchscreen(struct list_head *list);
> +
> +bool touch_overlay_process_event(struct list_head *list, struct input_dev *input,
> + u32 *x, u32 *y, u32 slot);
> +
> +#endif
>
> --
> 2.39.2
>
Kind regards,
Jeff LaBundy
^ permalink raw reply
* Re: [PATCH v7 4/4] Input: st1232 - add touch overlays handling
From: Jeff LaBundy @ 2024-03-04 3:43 UTC (permalink / raw)
To: Javier Carrasco
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Henrik Rydberg, Bastian Hecht, Michael Riesch, linux-kernel,
linux-input, devicetree
In-Reply-To: <20240119-feature-ts_virtobj_patch-v7-4-eda70985808f@wolfvision.net>
Hi Javier,
On Fri, Jan 19, 2024 at 08:43:36AM +0100, Javier Carrasco wrote:
> Use touch-overlay to support overlay objects such as buttons and a resized
> frame defined in the device tree.
>
> If buttons are provided, register an additional device to report key
> events separately. A key event will be generated if the coordinates
> of a touch event are within the area defined by the button properties.
>
> Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
Reviewed-by: Jeff LaBundy <jeff@labundy.com>
> ---
> drivers/input/touchscreen/st1232.c | 48 +++++++++++++++++++++++++++-----------
> 1 file changed, 34 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/input/touchscreen/st1232.c b/drivers/input/touchscreen/st1232.c
> index 6475084aee1b..4fa31447dbc1 100644
> --- a/drivers/input/touchscreen/st1232.c
> +++ b/drivers/input/touchscreen/st1232.c
> @@ -22,6 +22,7 @@
> #include <linux/pm_qos.h>
> #include <linux/slab.h>
> #include <linux/types.h>
> +#include <linux/input/touch-overlay.h>
>
> #define ST1232_TS_NAME "st1232-ts"
> #define ST1633_TS_NAME "st1633-ts"
> @@ -57,6 +58,7 @@ struct st1232_ts_data {
> struct dev_pm_qos_request low_latency_req;
> struct gpio_desc *reset_gpio;
> const struct st_chip_info *chip_info;
> + struct list_head touch_overlay_list;
> int read_buf_len;
> u8 *read_buf;
> };
> @@ -138,14 +140,20 @@ static int st1232_ts_parse_and_report(struct st1232_ts_data *ts)
>
> for (i = 0; i < ts->chip_info->max_fingers; i++) {
> u8 *buf = &ts->read_buf[i * 4];
> + bool contact = buf[0] & BIT(7);
> + unsigned int x, y;
>
> - if (buf[0] & BIT(7)) {
> - unsigned int x = ((buf[0] & 0x70) << 4) | buf[1];
> - unsigned int y = ((buf[0] & 0x07) << 8) | buf[2];
> -
> - touchscreen_set_mt_pos(&pos[n_contacts],
> - &ts->prop, x, y);
> + if (contact) {
> + x = ((buf[0] & 0x70) << 4) | buf[1];
> + y = ((buf[0] & 0x07) << 8) | buf[2];
> + }
> + if (touch_overlay_process_event(&ts->touch_overlay_list, input,
> + contact ? &x : NULL,
> + contact ? &y : NULL, i))
> + continue;
>
> + if (contact) {
> + touchscreen_set_mt_pos(&pos[n_contacts], &ts->prop, x, y);
> /* st1232 includes a z-axis / touch strength */
> if (ts->chip_info->have_z)
> z[n_contacts] = ts->read_buf[i + 6];
> @@ -292,18 +300,30 @@ static int st1232_ts_probe(struct i2c_client *client)
> if (error)
> return error;
>
> - /* Read resolution from the chip */
> - error = st1232_ts_read_resolution(ts, &max_x, &max_y);
> - if (error) {
> - dev_err(&client->dev,
> - "Failed to read resolution: %d\n", error);
> - return error;
> - }
> -
> if (ts->chip_info->have_z)
> input_set_abs_params(input_dev, ABS_MT_TOUCH_MAJOR, 0,
> ts->chip_info->max_area, 0, 0);
>
> + /* map overlay objects if defined in the device tree */
> + INIT_LIST_HEAD(&ts->touch_overlay_list);
> + error = touch_overlay_map(&ts->touch_overlay_list, input_dev);
> + if (error)
> + return error;
> +
> + if (touch_overlay_mapped_touchscreen(&ts->touch_overlay_list)) {
> + /* Read resolution from the overlay touchscreen if defined */
> + touch_overlay_get_touchscreen_abs(&ts->touch_overlay_list,
> + &max_x, &max_y);
> + } else {
> + /* Read resolution from the chip */
> + error = st1232_ts_read_resolution(ts, &max_x, &max_y);
> + if (error) {
> + dev_err(&client->dev,
> + "Failed to read resolution: %d\n", error);
> + return error;
> + }
> + }
> +
> input_set_abs_params(input_dev, ABS_MT_POSITION_X,
> 0, max_x, 0, 0);
> input_set_abs_params(input_dev, ABS_MT_POSITION_Y,
>
> --
> 2.39.2
>
Kind regards,
Jeff LaBundy
^ permalink raw reply
* Re: [PATCH] Input: iqs626a - Use common error handling code in iqs626_parse_events()
From: Dmitry Torokhov @ 2024-03-04 4:29 UTC (permalink / raw)
To: Jeff LaBundy
Cc: Markus Elfring, linux-input, kernel-janitors, Rob Herring,
Uwe Kleine-König, LKML
In-Reply-To: <ZeU8ENmnPj3sKxAv@nixie71>
On Sun, Mar 03, 2024 at 09:12:16PM -0600, Jeff LaBundy wrote:
> Hi Markus,
>
> On Sat, Mar 02, 2024 at 12:42:08PM +0100, Markus Elfring wrote:
> > From: Markus Elfring <elfring@users.sourceforge.net>
> > Date: Sat, 2 Mar 2024 11:44:17 +0100
> >
> > Add a jump target so that a bit of exception handling can be better reused
> > at the end of this function implementation.
> >
> > This issue was transformed by using the Coccinelle software.
> >
> > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> > ---
> > drivers/input/misc/iqs626a.c | 13 +++++++------
> > 1 file changed, 7 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/input/misc/iqs626a.c b/drivers/input/misc/iqs626a.c
> > index 0dab54d3a060..fa9570755f7b 100644
> > --- a/drivers/input/misc/iqs626a.c
> > +++ b/drivers/input/misc/iqs626a.c
> > @@ -530,8 +530,7 @@ iqs626_parse_events(struct iqs626_private *iqs626,
> > dev_err(&client->dev,
> > "Invalid input type: %u\n",
> > val);
> > - fwnode_handle_put(ev_node);
> > - return -EINVAL;
> > + goto put_fwnode;
> > }
> >
> > iqs626->kp_type[ch_id][i] = val;
> > @@ -545,8 +544,7 @@ iqs626_parse_events(struct iqs626_private *iqs626,
> > dev_err(&client->dev,
> > "Invalid %s channel hysteresis: %u\n",
> > fwnode_get_name(ch_node), val);
> > - fwnode_handle_put(ev_node);
> > - return -EINVAL;
> > + goto put_fwnode;
> > }
> >
> > if (i == IQS626_EVENT_DEEP_DN ||
> > @@ -566,8 +564,7 @@ iqs626_parse_events(struct iqs626_private *iqs626,
> > dev_err(&client->dev,
> > "Invalid %s channel threshold: %u\n",
> > fwnode_get_name(ch_node), val);
> > - fwnode_handle_put(ev_node);
> > - return -EINVAL;
> > + goto put_fwnode;
> > }
> >
> > if (ch_id == IQS626_CH_HALL)
> > @@ -580,6 +577,10 @@ iqs626_parse_events(struct iqs626_private *iqs626,
> > }
> >
> > return 0;
> > +
> > +put_fwnode:
> > + fwnode_handle_put(ev_node);
> > + return -EINVAL;
> > }
> >
> > static noinline_for_stack int
> > --
> > 2.44.0
> >
>
> Thank you for this patch, but it seems like a NAK to me. I think this is
> a matter of personal preference, and according to mine, it is much more
> confusing to insert a goto label after a function's primary return path
> than to have 2-3 calls to fwnode_handle_put().
>
> If you feel strongly otherwise, then I would suggest a helper function as
> recommended by Dmitry in another thread. However, maybe that helper should
> live in the driver core, as I suspect this driver is not the only place we
> can avoid calling fwnode_handle_put() in an error path that returns an int.
Yes, it should go into include/linux/fwnode.h, something like
DEFINE_FREE(fwnode, struct fwnode_handle *, if (_T) fwnode_hanlde_put(_T));
Then drivers can do:
struct fwnode_handle *ev_node __free(fwnode) =
fwnode_get_named_child_node(ch_node, ev_name);
and have it automatically be "put" once execution leaves the variable
scope.
Ah, we actually already have it defined in include/linux/property.h, all
the better.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [RFC PATCH v3 1/5] dt-bindings: mfd: add entry for Marvell 88PM886 PMIC
From: Krzysztof Kozlowski @ 2024-03-04 8:03 UTC (permalink / raw)
To: Karel Balej, Karel Balej, Lee Jones, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Dmitry Torokhov, Liam Girdwood,
Mark Brown, devicetree, linux-kernel, linux-input
Cc: Duje Mihanović, ~postmarketos/upstreaming, phone-devel
In-Reply-To: <20240303101506.4187-2-karelb@gimli.ms.mff.cuni.cz>
On 03/03/2024 11:04, Karel Balej wrote:
> From: Karel Balej <balejk@matfyz.cz>
>
> +examples:
> + - |
> + #include <dt-bindings/interrupt-controller/irq.h>
> + i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + pmic@30 {
> + compatible = "marvell,88pm886-a1";
> + reg = <0x30>;
> + interrupts = <0 4 IRQ_TYPE_LEVEL_HIGH>;
> + interrupt-parent = <&gic>;
> + wakeup-source;
> +
> + regulators {
> + ldo2: ldo2 {
> + regulator-min-microvolt = <3100000>;
> + regulator-max-microvolt = <3300000>;
> + };
Messed indentation here and in following lines..
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH] Input: iqs626a - Use common error handling code in iqs626_parse_events()
From: Dan Carpenter @ 2024-03-04 8:18 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Jeff LaBundy, Markus Elfring, linux-input, kernel-janitors,
Rob Herring, Uwe Kleine-König, LKML
In-Reply-To: <ZeVOPSt0L1D4BxuZ@google.com>
On Sun, Mar 03, 2024 at 08:29:49PM -0800, Dmitry Torokhov wrote:
> On Sun, Mar 03, 2024 at 09:12:16PM -0600, Jeff LaBundy wrote:
> > Hi Markus,
> >
> > On Sat, Mar 02, 2024 at 12:42:08PM +0100, Markus Elfring wrote:
> > > From: Markus Elfring <elfring@users.sourceforge.net>
> > > Date: Sat, 2 Mar 2024 11:44:17 +0100
> > >
> > > Add a jump target so that a bit of exception handling can be better reused
> > > at the end of this function implementation.
> > >
> > > This issue was transformed by using the Coccinelle software.
> > >
> > > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> > > ---
> > > drivers/input/misc/iqs626a.c | 13 +++++++------
> > > 1 file changed, 7 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/input/misc/iqs626a.c b/drivers/input/misc/iqs626a.c
> > > index 0dab54d3a060..fa9570755f7b 100644
> > > --- a/drivers/input/misc/iqs626a.c
> > > +++ b/drivers/input/misc/iqs626a.c
> > > @@ -530,8 +530,7 @@ iqs626_parse_events(struct iqs626_private *iqs626,
> > > dev_err(&client->dev,
> > > "Invalid input type: %u\n",
> > > val);
> > > - fwnode_handle_put(ev_node);
> > > - return -EINVAL;
> > > + goto put_fwnode;
> > > }
> > >
> > > iqs626->kp_type[ch_id][i] = val;
> > > @@ -545,8 +544,7 @@ iqs626_parse_events(struct iqs626_private *iqs626,
> > > dev_err(&client->dev,
> > > "Invalid %s channel hysteresis: %u\n",
> > > fwnode_get_name(ch_node), val);
> > > - fwnode_handle_put(ev_node);
> > > - return -EINVAL;
> > > + goto put_fwnode;
> > > }
> > >
> > > if (i == IQS626_EVENT_DEEP_DN ||
> > > @@ -566,8 +564,7 @@ iqs626_parse_events(struct iqs626_private *iqs626,
> > > dev_err(&client->dev,
> > > "Invalid %s channel threshold: %u\n",
> > > fwnode_get_name(ch_node), val);
> > > - fwnode_handle_put(ev_node);
> > > - return -EINVAL;
> > > + goto put_fwnode;
> > > }
> > >
> > > if (ch_id == IQS626_CH_HALL)
> > > @@ -580,6 +577,10 @@ iqs626_parse_events(struct iqs626_private *iqs626,
> > > }
> > >
> > > return 0;
> > > +
> > > +put_fwnode:
> > > + fwnode_handle_put(ev_node);
> > > + return -EINVAL;
> > > }
> > >
> > > static noinline_for_stack int
> > > --
> > > 2.44.0
> > >
> >
> > Thank you for this patch, but it seems like a NAK to me. I think this is
> > a matter of personal preference, and according to mine, it is much more
> > confusing to insert a goto label after a function's primary return path
> > than to have 2-3 calls to fwnode_handle_put().
> >
> > If you feel strongly otherwise, then I would suggest a helper function as
> > recommended by Dmitry in another thread. However, maybe that helper should
> > live in the driver core, as I suspect this driver is not the only place we
> > can avoid calling fwnode_handle_put() in an error path that returns an int.
>
> Yes, it should go into include/linux/fwnode.h, something like
>
> DEFINE_FREE(fwnode, struct fwnode_handle *, if (_T) fwnode_hanlde_put(_T));
>
> Then drivers can do:
>
> struct fwnode_handle *ev_node __free(fwnode) =
> fwnode_get_named_child_node(ch_node, ev_name);
>
> and have it automatically be "put" once execution leaves the variable
> scope.
>
> Ah, we actually already have it defined in include/linux/property.h, all
> the better.
It's already there.
DEFINE_FREE(fwnode_handle, struct fwnode_handle *, fwnode_handle_put(_T))
I can send a patch for this. You need to be a bit carefull to move
the declaration into the correct scope for this to work. I should write
some Smatch rules for this...
regards,
dan carpenter
^ permalink raw reply
* [REGRESSION] Missing bcm5974 touchpad on Macbooks
From: Takashi Iwai @ 2024-03-04 8:35 UTC (permalink / raw)
To: Javier Carrasco; +Cc: Dmitry Torokhov, linux-input, linux-kernel, regressions
Hi,
we've received a few regression reports for openSUSE Leap about the
missing touchpad on Macbooks. After debugging, this turned out to be
the backport of the commit 2b9c3eb32a699acdd4784d6b93743271b4970899
Input: bcm5974 - check endpoint type before starting traffic
And, the same regression was confirmed on the upstream 6.8-rc6
kernel.
Reverting the commit above fixes the problem, the touchpad reappears.
The detailed hardware info is found at:
https://bugzilla.suse.com/show_bug.cgi?id=1220030
Feel free to join the bugzilla above, or let me know if you need
something for debugging, then I'll delegate on the bugzilla.
thanks,
Takashi
^ permalink raw reply
* [PATCH v2] Input: iqs269a - Use scope-based resource management in iqs269_parse_chan()
From: Markus Elfring @ 2024-03-04 9:55 UTC (permalink / raw)
To: linux-input, kernel-janitors, Dmitry Torokhov, Jeff LaBundy,
Mattijs Korpershoek, Rob Herring, Uwe Kleine-König,
ye xingchen
Cc: LKML, Jonathan Cameron
In-Reply-To: <ZeT6UUFNq1ujMW17@google.com>
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 4 Mar 2024 10:30:52 +0100
Scope-based resource management became supported also for this software
area by contributions of Jonathan Cameron on 2024-02-17.
device property: Add cleanup.h based fwnode_handle_put() scope based cleanup.
https://lore.kernel.org/r/20240217164249.921878-3-jic23@kernel.org
* Thus use the attribute “__free(fwnode_handle)”.
* Reduce the scope for the local variable “ev_node” into a for loop.
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
v2:
An other cleanup technique was applied as requested by Dmitry Torokhov.
drivers/input/misc/iqs269a.c | 73 ++++++++++++++++++------------------
1 file changed, 37 insertions(+), 36 deletions(-)
diff --git a/drivers/input/misc/iqs269a.c b/drivers/input/misc/iqs269a.c
index cd14ff9f57cf..9caee936927b 100644
--- a/drivers/input/misc/iqs269a.c
+++ b/drivers/input/misc/iqs269a.c
@@ -557,7 +557,6 @@ static int iqs269_parse_chan(struct iqs269_private *iqs269,
const struct fwnode_handle *ch_node)
{
struct i2c_client *client = iqs269->client;
- struct fwnode_handle *ev_node;
struct iqs269_ch_reg *ch_reg;
u16 engine_a, engine_b;
unsigned int reg, val;
@@ -734,47 +733,49 @@ static int iqs269_parse_chan(struct iqs269_private *iqs269,
}
for (i = 0; i < ARRAY_SIZE(iqs269_events); i++) {
- ev_node = fwnode_get_named_child_node(ch_node,
- iqs269_events[i].name);
- if (!ev_node)
- continue;
-
- if (!fwnode_property_read_u32(ev_node, "azoteq,thresh", &val)) {
- if (val > IQS269_CHx_THRESH_MAX) {
- dev_err(&client->dev,
- "Invalid channel %u threshold: %u\n",
- reg, val);
- fwnode_handle_put(ev_node);
- return -EINVAL;
+ {
+ struct fwnode_handle *ev_node __free(fwnode_handle)
+ = fwnode_get_named_child_node(ch_node,
+ iqs269_events[i].name);
+
+ if (!ev_node)
+ continue;
+
+ if (!fwnode_property_read_u32(ev_node, "azoteq,thresh", &val)) {
+ if (val > IQS269_CHx_THRESH_MAX) {
+ dev_err(&client->dev,
+ "Invalid channel %u threshold: %u\n",
+ reg, val);
+ return -EINVAL;
+ }
+
+ ch_reg->thresh[iqs269_events[i].th_offs] = val;
}
- ch_reg->thresh[iqs269_events[i].th_offs] = val;
- }
-
- if (!fwnode_property_read_u32(ev_node, "azoteq,hyst", &val)) {
- u8 *hyst = &ch_reg->hyst;
-
- if (val > IQS269_CHx_HYST_MAX) {
- dev_err(&client->dev,
- "Invalid channel %u hysteresis: %u\n",
- reg, val);
- fwnode_handle_put(ev_node);
- return -EINVAL;
+ if (!fwnode_property_read_u32(ev_node, "azoteq,hyst", &val)) {
+ u8 *hyst = &ch_reg->hyst;
+
+ if (val > IQS269_CHx_HYST_MAX) {
+ dev_err(&client->dev,
+ "Invalid channel %u hysteresis: %u\n",
+ reg, val);
+ return -EINVAL;
+ }
+
+ if (i == IQS269_EVENT_DEEP_DN ||
+ i == IQS269_EVENT_DEEP_UP) {
+ *hyst &= ~IQS269_CHx_HYST_DEEP_MASK;
+ *hyst |= (val << IQS269_CHx_HYST_DEEP_SHIFT);
+ } else if (i == IQS269_EVENT_TOUCH_DN ||
+ i == IQS269_EVENT_TOUCH_UP) {
+ *hyst &= ~IQS269_CHx_HYST_TOUCH_MASK;
+ *hyst |= val;
+ }
}
- if (i == IQS269_EVENT_DEEP_DN ||
- i == IQS269_EVENT_DEEP_UP) {
- *hyst &= ~IQS269_CHx_HYST_DEEP_MASK;
- *hyst |= (val << IQS269_CHx_HYST_DEEP_SHIFT);
- } else if (i == IQS269_EVENT_TOUCH_DN ||
- i == IQS269_EVENT_TOUCH_UP) {
- *hyst &= ~IQS269_CHx_HYST_TOUCH_MASK;
- *hyst |= val;
- }
+ error = fwnode_property_read_u32(ev_node, "linux,code", &val);
}
- error = fwnode_property_read_u32(ev_node, "linux,code", &val);
- fwnode_handle_put(ev_node);
if (error == -EINVAL) {
continue;
} else if (error) {
--
2.44.0
^ permalink raw reply related
* [PATCH v2] Input: iqs626a - Use scope-based resource management in iqs626_parse_events()
From: Markus Elfring @ 2024-03-04 10:52 UTC (permalink / raw)
To: linux-input, kernel-janitors, Dmitry Torokhov, Jeff LaBundy,
Rob Herring, Uwe Kleine-König
Cc: LKML, Jonathan Cameron
In-Reply-To: <ZeVOPSt0L1D4BxuZ@google.com>
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 4 Mar 2024 11:40:04 +0100
Scope-based resource management became supported also for this software
area by contributions of Jonathan Cameron on 2024-02-17.
device property: Add cleanup.h based fwnode_handle_put() scope based cleanup.
https://lore.kernel.org/r/20240217164249.921878-3-jic23@kernel.org
* Thus use the attribute “__free(fwnode_handle)”.
* Reduce the scope for the local variable “ev_node” into a for loop.
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
v2:
An other cleanup technique was applied as requested by Dmitry Torokhov
and Jeff LaBundy.
drivers/input/misc/iqs626a.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/input/misc/iqs626a.c b/drivers/input/misc/iqs626a.c
index 0dab54d3a060..86fcb5134f45 100644
--- a/drivers/input/misc/iqs626a.c
+++ b/drivers/input/misc/iqs626a.c
@@ -462,7 +462,6 @@ iqs626_parse_events(struct iqs626_private *iqs626,
{
struct iqs626_sys_reg *sys_reg = &iqs626->sys_reg;
struct i2c_client *client = iqs626->client;
- struct fwnode_handle *ev_node;
const char *ev_name;
u8 *thresh, *hyst;
unsigned int val;
@@ -501,6 +500,8 @@ iqs626_parse_events(struct iqs626_private *iqs626,
if (!iqs626_channels[ch_id].events[i])
continue;
+ struct fwnode_handle *ev_node __free(fwnode_handle);
+
if (ch_id == IQS626_CH_TP_2 || ch_id == IQS626_CH_TP_3) {
/*
* Trackpad touch events are simply described under the
@@ -530,7 +531,6 @@ iqs626_parse_events(struct iqs626_private *iqs626,
dev_err(&client->dev,
"Invalid input type: %u\n",
val);
- fwnode_handle_put(ev_node);
return -EINVAL;
}
@@ -545,7 +545,6 @@ iqs626_parse_events(struct iqs626_private *iqs626,
dev_err(&client->dev,
"Invalid %s channel hysteresis: %u\n",
fwnode_get_name(ch_node), val);
- fwnode_handle_put(ev_node);
return -EINVAL;
}
@@ -566,7 +565,6 @@ iqs626_parse_events(struct iqs626_private *iqs626,
dev_err(&client->dev,
"Invalid %s channel threshold: %u\n",
fwnode_get_name(ch_node), val);
- fwnode_handle_put(ev_node);
return -EINVAL;
}
@@ -575,8 +573,6 @@ iqs626_parse_events(struct iqs626_private *iqs626,
else
*(thresh + iqs626_events[i].th_offs) = val;
}
-
- fwnode_handle_put(ev_node);
}
return 0;
--
2.44.0
^ permalink raw reply related
* Re: [PATCH v2] Input: iqs626a - Use scope-based resource management in iqs626_parse_events()
From: Julia Lawall @ 2024-03-04 10:55 UTC (permalink / raw)
To: Markus Elfring
Cc: linux-input, kernel-janitors, Dmitry Torokhov, Jeff LaBundy,
Rob Herring, Uwe Kleine-König, LKML, Jonathan Cameron
In-Reply-To: <e8a2b63f-4f9a-463b-b419-c5f673191111@web.de>
[-- Attachment #1: Type: text/plain, Size: 2651 bytes --]
On Mon, 4 Mar 2024, Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Mon, 4 Mar 2024 11:40:04 +0100
>
> Scope-based resource management became supported also for this software
> area by contributions of Jonathan Cameron on 2024-02-17.
>
> device property: Add cleanup.h based fwnode_handle_put() scope based cleanup.
> https://lore.kernel.org/r/20240217164249.921878-3-jic23@kernel.org
>
>
> * Thus use the attribute “__free(fwnode_handle)”.
>
> * Reduce the scope for the local variable “ev_node” into a for loop.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>
> v2:
> An other cleanup technique was applied as requested by Dmitry Torokhov
> and Jeff LaBundy.
>
>
> drivers/input/misc/iqs626a.c | 8 ++------
> 1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/input/misc/iqs626a.c b/drivers/input/misc/iqs626a.c
> index 0dab54d3a060..86fcb5134f45 100644
> --- a/drivers/input/misc/iqs626a.c
> +++ b/drivers/input/misc/iqs626a.c
> @@ -462,7 +462,6 @@ iqs626_parse_events(struct iqs626_private *iqs626,
> {
> struct iqs626_sys_reg *sys_reg = &iqs626->sys_reg;
> struct i2c_client *client = iqs626->client;
> - struct fwnode_handle *ev_node;
> const char *ev_name;
> u8 *thresh, *hyst;
> unsigned int val;
> @@ -501,6 +500,8 @@ iqs626_parse_events(struct iqs626_private *iqs626,
> if (!iqs626_channels[ch_id].events[i])
> continue;
>
> + struct fwnode_handle *ev_node __free(fwnode_handle);
Doesn't this need to be initialized?
julia
> +
> if (ch_id == IQS626_CH_TP_2 || ch_id == IQS626_CH_TP_3) {
> /*
> * Trackpad touch events are simply described under the
> @@ -530,7 +531,6 @@ iqs626_parse_events(struct iqs626_private *iqs626,
> dev_err(&client->dev,
> "Invalid input type: %u\n",
> val);
> - fwnode_handle_put(ev_node);
> return -EINVAL;
> }
>
> @@ -545,7 +545,6 @@ iqs626_parse_events(struct iqs626_private *iqs626,
> dev_err(&client->dev,
> "Invalid %s channel hysteresis: %u\n",
> fwnode_get_name(ch_node), val);
> - fwnode_handle_put(ev_node);
> return -EINVAL;
> }
>
> @@ -566,7 +565,6 @@ iqs626_parse_events(struct iqs626_private *iqs626,
> dev_err(&client->dev,
> "Invalid %s channel threshold: %u\n",
> fwnode_get_name(ch_node), val);
> - fwnode_handle_put(ev_node);
> return -EINVAL;
> }
>
> @@ -575,8 +573,6 @@ iqs626_parse_events(struct iqs626_private *iqs626,
> else
> *(thresh + iqs626_events[i].th_offs) = val;
> }
> -
> - fwnode_handle_put(ev_node);
> }
>
> return 0;
> --
> 2.44.0
>
>
>
^ permalink raw reply
* Re: [REGRESSION] Missing bcm5974 touchpad on Macbooks
From: Javier Carrasco @ 2024-03-04 11:26 UTC (permalink / raw)
To: Takashi Iwai, Javier Carrasco
Cc: Dmitry Torokhov, linux-input, linux-kernel, regressions
In-Reply-To: <87sf161jjc.wl-tiwai@suse.de>
On 04.03.24 09:35, Takashi Iwai wrote:
> Hi,
>
> we've received a few regression reports for openSUSE Leap about the
> missing touchpad on Macbooks. After debugging, this turned out to be
> the backport of the commit 2b9c3eb32a699acdd4784d6b93743271b4970899
> Input: bcm5974 - check endpoint type before starting traffic
>
> And, the same regression was confirmed on the upstream 6.8-rc6
> kernel.
>
> Reverting the commit above fixes the problem, the touchpad reappears.
>
> The detailed hardware info is found at:
> https://bugzilla.suse.com/show_bug.cgi?id=1220030
>
> Feel free to join the bugzilla above, or let me know if you need
> something for debugging, then I'll delegate on the bugzilla.
>
>
> thanks,
>
> Takashi
>
Hi Takashi,
The commit adds a check to ensure that the endpoint type is interrupt.
According to that report, the issue arose with a MacBook Pro 5.1 (no
button, only trackpad endpoint), so the check on the tp_ep address
(0x81) returns false. I assume that you see an error message
("Unexpected non-int endpoint) and the probe function fails returning
-ENODEV.
Do you see any warning in the logs when you revert the commit? It was
added to prevent using wrong endpoint types, which will display the
following warning: "BOGUS urb xfer, pipe "some_number" != type
"another_number""
I am just wondering if for some reason the check on interrupt type is
wrong here.
Best regards,
Javier Carrasco
^ permalink raw reply
* Re: [PATCH v2] Input: iqs626a - Use scope-based resource management in iqs626_parse_events()
From: Dan Carpenter @ 2024-03-04 11:32 UTC (permalink / raw)
To: Julia Lawall
Cc: Markus Elfring, linux-input, kernel-janitors, Dmitry Torokhov,
Jeff LaBundy, Rob Herring, Uwe Kleine-König, LKML,
Jonathan Cameron
In-Reply-To: <b91fe21-fe2-eac8-d1ee-ea8922a08861@inria.fr>
On Mon, Mar 04, 2024 at 11:55:23AM +0100, Julia Lawall wrote:
>
>
> On Mon, 4 Mar 2024, Markus Elfring wrote:
>
> > From: Markus Elfring <elfring@users.sourceforge.net>
> > Date: Mon, 4 Mar 2024 11:40:04 +0100
> >
> > Scope-based resource management became supported also for this software
> > area by contributions of Jonathan Cameron on 2024-02-17.
> >
> > device property: Add cleanup.h based fwnode_handle_put() scope based cleanup.
> > https://lore.kernel.org/r/20240217164249.921878-3-jic23@kernel.org
> >
> >
> > * Thus use the attribute “__free(fwnode_handle)”.
> >
> > * Reduce the scope for the local variable “ev_node” into a for loop.
> >
> > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> > ---
> >
> > v2:
> > An other cleanup technique was applied as requested by Dmitry Torokhov
> > and Jeff LaBundy.
> >
> >
> > drivers/input/misc/iqs626a.c | 8 ++------
> > 1 file changed, 2 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/input/misc/iqs626a.c b/drivers/input/misc/iqs626a.c
> > index 0dab54d3a060..86fcb5134f45 100644
> > --- a/drivers/input/misc/iqs626a.c
> > +++ b/drivers/input/misc/iqs626a.c
> > @@ -462,7 +462,6 @@ iqs626_parse_events(struct iqs626_private *iqs626,
> > {
> > struct iqs626_sys_reg *sys_reg = &iqs626->sys_reg;
> > struct i2c_client *client = iqs626->client;
> > - struct fwnode_handle *ev_node;
> > const char *ev_name;
> > u8 *thresh, *hyst;
> > unsigned int val;
> > @@ -501,6 +500,8 @@ iqs626_parse_events(struct iqs626_private *iqs626,
> > if (!iqs626_channels[ch_id].events[i])
> > continue;
> >
> > + struct fwnode_handle *ev_node __free(fwnode_handle);
>
> Doesn't this need to be initialized?
>
Only if you declared before the continue; statement. Generally kernel
style is that you have to declare variables at the start of the block...
But that's becoming less universal now that it's not a compile error.
regards,
dan carpenter
^ permalink raw reply
* Re: [PATCH v2] Input: iqs626a - Use scope-based resource management in iqs626_parse_events()
From: Markus Elfring @ 2024-03-04 12:10 UTC (permalink / raw)
To: Julia Lawall, linux-input, kernel-janitors, Dmitry Torokhov,
Jeff LaBundy, Rob Herring, Uwe Kleine-König
Cc: LKML, Jonathan Cameron
In-Reply-To: <b91fe21-fe2-eac8-d1ee-ea8922a08861@inria.fr>
>> Scope-based resource management became supported also for this software
>> area by contributions of Jonathan Cameron on 2024-02-17.
>>
>> device property: Add cleanup.h based fwnode_handle_put() scope based cleanup.
>> https://lore.kernel.org/r/20240217164249.921878-3-jic23@kernel.org
>>
>>
>> * Thus use the attribute “__free(fwnode_handle)”.
>>
>> * Reduce the scope for the local variable “ev_node” into a for loop.
…
>> +++ b/drivers/input/misc/iqs626a.c
>> @@ -462,7 +462,6 @@ iqs626_parse_events(struct iqs626_private *iqs626,
>> {
>> struct iqs626_sys_reg *sys_reg = &iqs626->sys_reg;
>> struct i2c_client *client = iqs626->client;
>> - struct fwnode_handle *ev_node;
>> const char *ev_name;
>> u8 *thresh, *hyst;
>> unsigned int val;
>> @@ -501,6 +500,8 @@ iqs626_parse_events(struct iqs626_private *iqs626,
>> if (!iqs626_channels[ch_id].events[i])
>> continue;
>>
>> + struct fwnode_handle *ev_node __free(fwnode_handle);
>
> Doesn't this need to be initialized?
This variable should usually be set in both branches of the subsequent if statement,
shouldn't it?
Please take another look at the proposed scope reduction
for the affected variable.
May additional curly brackets be omitted for this source code transformation?
Regards,
Markus
^ permalink raw reply
* Re: [REGRESSION] Missing bcm5974 touchpad on Macbooks
From: Linux regression tracking #adding (Thorsten Leemhuis) @ 2024-03-04 12:18 UTC (permalink / raw)
To: regressions; +Cc: linux-input, linux-kernel
In-Reply-To: <87sf161jjc.wl-tiwai@suse.de>
[TLDR: I'm adding this report to the list of tracked Linux kernel
regressions; the text you find below is based on a few templates
paragraphs you might have encountered already in similar form.
See link in footer if these mails annoy you.]
On 04.03.24 09:35, Takashi Iwai wrote:
>
> we've received a few regression reports for openSUSE Leap about the
> missing touchpad on Macbooks. After debugging, this turned out to be
> the backport of the commit 2b9c3eb32a699acdd4784d6b93743271b4970899
> Input: bcm5974 - check endpoint type before starting traffic
>
> And, the same regression was confirmed on the upstream 6.8-rc6
> kernel.
>
> Reverting the commit above fixes the problem, the touchpad reappears.
>
> The detailed hardware info is found at:
> https://bugzilla.suse.com/show_bug.cgi?id=1220030
Thanks for the report. To be sure the issue doesn't fall through the
cracks unnoticed, I'm adding it to regzbot, the Linux kernel regression
tracking bot:
#regzbot ^introduced 2b9c3eb32a699ac
#regzbot title Input: missing bcm5974 touchpad on Macbooks
#regzbot duplicate: https://bugzilla.suse.com/show_bug.cgi?id=1220030
#regzbot ignore-activity
Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
That page also explains what to do if mails like this annoy you.
^ permalink raw reply
* Re: Input: iqs626a - Use common error handling code in iqs626_parse_events()
From: Markus Elfring @ 2024-03-04 12:31 UTC (permalink / raw)
To: Dan Carpenter, Dmitry Torokhov, Jeff LaBundy, linux-input,
kernel-janitors
Cc: Rob Herring, Uwe Kleine-König, LKML
In-Reply-To: <11e5db31-2a8f-458d-a249-7205e37aa20f@moroto.mountain>
> DEFINE_FREE(fwnode_handle, struct fwnode_handle *, fwnode_handle_put(_T))
>
> I can send a patch for this. You need to be a bit carefull to move
> the declaration into the correct scope for this to work. I should write
> some Smatch rules for this...
I became also curious how available development tools will evolve further
for improved handling of scope-based resource management.
Regards,
Markus
^ permalink raw reply
* Re: [REGRESSION] Missing bcm5974 touchpad on Macbooks
From: Takashi Iwai @ 2024-03-04 12:45 UTC (permalink / raw)
To: Javier Carrasco
Cc: Takashi Iwai, Javier Carrasco, Dmitry Torokhov, linux-input,
linux-kernel, regressions
In-Reply-To: <6ef6c5bf-e6e5-4711-81c6-6ae41de2e61e@wolfvision.net>
On Mon, 04 Mar 2024 12:26:48 +0100,
Javier Carrasco wrote:
>
> On 04.03.24 09:35, Takashi Iwai wrote:
> > Hi,
> >
> > we've received a few regression reports for openSUSE Leap about the
> > missing touchpad on Macbooks. After debugging, this turned out to be
> > the backport of the commit 2b9c3eb32a699acdd4784d6b93743271b4970899
> > Input: bcm5974 - check endpoint type before starting traffic
> >
> > And, the same regression was confirmed on the upstream 6.8-rc6
> > kernel.
> >
> > Reverting the commit above fixes the problem, the touchpad reappears.
> >
> > The detailed hardware info is found at:
> > https://bugzilla.suse.com/show_bug.cgi?id=1220030
> >
> > Feel free to join the bugzilla above, or let me know if you need
> > something for debugging, then I'll delegate on the bugzilla.
> >
> >
> > thanks,
> >
> > Takashi
> >
>
> Hi Takashi,
>
> The commit adds a check to ensure that the endpoint type is interrupt.
>
> According to that report, the issue arose with a MacBook Pro 5.1 (no
> button, only trackpad endpoint), so the check on the tp_ep address
> (0x81) returns false. I assume that you see an error message
> ("Unexpected non-int endpoint) and the probe function fails returning
> -ENODEV.
Right, there is the message.
> Do you see any warning in the logs when you revert the commit? It was
> added to prevent using wrong endpoint types, which will display the
> following warning: "BOGUS urb xfer, pipe "some_number" != type
> "another_number""
The revert was tested on the downstream kernel, but it has also the
check of bogus pipe, and there was no such warning, as far as I see
the report.
> I am just wondering if for some reason the check on interrupt type is
> wrong here.
I'll ask reporters to give the lsusb -v output so that we can take a
deeper look. Also, I'm building a test kernel based on 6.8-rc7 with
the revert, and ask reporters to test with it, just to be sure.
thanks,
Takashi
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox