* Re: [PATCH] HID: i2c-hid: ensure various commands do not interfere with each other
From: Jiri Kosina @ 2024-09-11 13:27 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: . Benjamin Tissoires, Douglas Anderson, Hans de Goede,
Kenny Levinsen, linux-input, linux-kernel
In-Reply-To: <Zt9clAu04BinzIcd@google.com>
On Mon, 9 Sep 2024, Dmitry Torokhov wrote:
> i2c-hid uses 2 shared buffers: command and "raw" input buffer for
> sending requests to peripherals and read data from peripherals when
> executing variety of commands. Such commands include reading of HID
> registers, requesting particular power mode, getting and setting
> reports and so on. Because all such requests use the same 2 buffers
> they should not execute simultaneously.
>
> Fix this by introducing "cmd_lock" mutex and acquire it whenever
> we needs to access ihid->cmdbuf or idid->rawbuf.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Thanks for the fix, Dmitry. Out of curiosity, did you find it by code
inspection, or have you actually seen it happening for real, making the
driver misbehave?
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH v5 0/10] Touch Bar support for T2 Macs
From: Jiri Kosina @ 2024-09-11 12:21 UTC (permalink / raw)
To: Aditya Garg
Cc: tzimmermann@suse.de, maarten.lankhorst@linux.intel.com,
mripard@kernel.org, airlied@gmail.com, daniel@ffwll.ch,
bentiss@kernel.org, Thomas Weißschuh, Orlando Chamberlain,
Kerem Karabay, Linux Kernel Mailing List,
linux-input@vger.kernel.org, dri-devel@lists.freedesktop.org,
torvalds@linux-foundation.org
In-Reply-To: <MA0P287MB02176175318B96135BE3E320B8902@MA0P287MB0217.INDP287.PROD.OUTLOOK.COM>
On Sat, 31 Aug 2024, Aditya Garg wrote:
> Hi Maintainers
>
> It has been 2 weeks but I still haven't received a single reply on this
> version of the patch series. Consider this email as a friendly reminder.
I think it makes most sense to take this whole set through hid.git, but
for that, I'd like to get Acked-by/Reviewed-by for patches 9 and 10 (drm
bits).
Dave, Daniel, .. ?
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH] Adding Support for Thinkpad X12 Gen 2 Kbd Portfolio with 0x61AE as PID
From: Jiri Kosina @ 2024-09-11 12:18 UTC (permalink / raw)
To: Vishnu Sankar
Cc: bentiss, linux-input, linux-kernel, mpearson-lenovo, vsankar
In-Reply-To: <CABxCQKtfFttYVpfZE0jsjt=xgO4EJ0vNeb4Wf-==xOr3XnKnxQ@mail.gmail.com>
On Tue, 10 Sep 2024, Vishnu Sankar wrote:
> Do we have any feedback or comments about this patch?
Sorry, this slipped in between cracks.
Now applied, thanks.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH] HID: i2c-hid: ensure various commands do not interfere with each other
From: Hans de Goede @ 2024-09-11 12:16 UTC (permalink / raw)
To: Dmitry Torokhov, Jiri Kosina, . Benjamin Tissoires
Cc: Douglas Anderson, Kenny Levinsen, linux-input, linux-kernel
In-Reply-To: <Zt9clAu04BinzIcd@google.com>
Hi,
On 9/9/24 10:37 PM, Dmitry Torokhov wrote:
> i2c-hid uses 2 shared buffers: command and "raw" input buffer for
> sending requests to peripherals and read data from peripherals when
> executing variety of commands. Such commands include reading of HID
> registers, requesting particular power mode, getting and setting
> reports and so on. Because all such requests use the same 2 buffers
> they should not execute simultaneously.
>
> Fix this by introducing "cmd_lock" mutex and acquire it whenever
> we needs to access ihid->cmdbuf or idid->rawbuf.
Typo: s/idid/ihid/
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Thanks, patch looks good to me:
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Regards,
Hans
> ---
> drivers/hid/i2c-hid/i2c-hid-core.c | 42 +++++++++++++++++++-----------
> 1 file changed, 27 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
> index 632eaf9e11a6..2f8a9d3f1e86 100644
> --- a/drivers/hid/i2c-hid/i2c-hid-core.c
> +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
> @@ -105,6 +105,7 @@ struct i2c_hid {
>
> wait_queue_head_t wait; /* For waiting the interrupt */
>
> + struct mutex cmd_lock; /* protects cmdbuf and rawbuf */
> struct mutex reset_lock;
>
> struct i2chid_ops *ops;
> @@ -220,6 +221,8 @@ static int i2c_hid_xfer(struct i2c_hid *ihid,
> static int i2c_hid_read_register(struct i2c_hid *ihid, __le16 reg,
> void *buf, size_t len)
> {
> + guard(mutex)(&ihid->cmd_lock);
> +
> *(__le16 *)ihid->cmdbuf = reg;
>
> return i2c_hid_xfer(ihid, ihid->cmdbuf, sizeof(__le16), buf, len);
> @@ -252,6 +255,8 @@ static int i2c_hid_get_report(struct i2c_hid *ihid,
>
> i2c_hid_dbg(ihid, "%s\n", __func__);
>
> + guard(mutex)(&ihid->cmd_lock);
> +
> /* Command register goes first */
> *(__le16 *)ihid->cmdbuf = ihid->hdesc.wCommandRegister;
> length += sizeof(__le16);
> @@ -342,6 +347,8 @@ static int i2c_hid_set_or_send_report(struct i2c_hid *ihid,
> if (!do_set && le16_to_cpu(ihid->hdesc.wMaxOutputLength) == 0)
> return -ENOSYS;
>
> + guard(mutex)(&ihid->cmd_lock);
> +
> if (do_set) {
> /* Command register goes first */
> *(__le16 *)ihid->cmdbuf = ihid->hdesc.wCommandRegister;
> @@ -384,6 +391,8 @@ static int i2c_hid_set_power_command(struct i2c_hid *ihid, int power_state)
> {
> size_t length;
>
> + guard(mutex)(&ihid->cmd_lock);
> +
> /* SET_POWER uses command register */
> *(__le16 *)ihid->cmdbuf = ihid->hdesc.wCommandRegister;
> length = sizeof(__le16);
> @@ -440,25 +449,27 @@ static int i2c_hid_start_hwreset(struct i2c_hid *ihid)
> if (ret)
> return ret;
>
> - /* Prepare reset command. Command register goes first. */
> - *(__le16 *)ihid->cmdbuf = ihid->hdesc.wCommandRegister;
> - length += sizeof(__le16);
> - /* Next is RESET command itself */
> - length += i2c_hid_encode_command(ihid->cmdbuf + length,
> - I2C_HID_OPCODE_RESET, 0, 0);
> + scoped_guard(mutex, &ihid->cmd_lock) {
> + /* Prepare reset command. Command register goes first. */
> + *(__le16 *)ihid->cmdbuf = ihid->hdesc.wCommandRegister;
> + length += sizeof(__le16);
> + /* Next is RESET command itself */
> + length += i2c_hid_encode_command(ihid->cmdbuf + length,
> + I2C_HID_OPCODE_RESET, 0, 0);
>
> - set_bit(I2C_HID_RESET_PENDING, &ihid->flags);
> + set_bit(I2C_HID_RESET_PENDING, &ihid->flags);
>
> - ret = i2c_hid_xfer(ihid, ihid->cmdbuf, length, NULL, 0);
> - if (ret) {
> - dev_err(&ihid->client->dev,
> - "failed to reset device: %d\n", ret);
> - goto err_clear_reset;
> - }
> + ret = i2c_hid_xfer(ihid, ihid->cmdbuf, length, NULL, 0);
> + if (ret) {
> + dev_err(&ihid->client->dev,
> + "failed to reset device: %d\n", ret);
> + break;
> + }
>
> - return 0;
> + return 0;
> + }
>
> -err_clear_reset:
> + /* Clean up if sending reset command failed */
> clear_bit(I2C_HID_RESET_PENDING, &ihid->flags);
> i2c_hid_set_power(ihid, I2C_HID_PWR_SLEEP);
> return ret;
> @@ -1200,6 +1211,7 @@ int i2c_hid_core_probe(struct i2c_client *client, struct i2chid_ops *ops,
> ihid->is_panel_follower = drm_is_panel_follower(&client->dev);
>
> init_waitqueue_head(&ihid->wait);
> + mutex_init(&ihid->cmd_lock);
> mutex_init(&ihid->reset_lock);
> INIT_WORK(&ihid->panel_follower_prepare_work, ihid_core_panel_prepare_work);
>
^ permalink raw reply
* Re: hid-lenovo breaks middle mouse button on tpIIkbd
From: Harald Welte @ 2024-09-11 6:22 UTC (permalink / raw)
To: ValdikSS; +Cc: linux-input
In-Reply-To: <8359e8a4-c8bb-4ff7-aa70-c1e2d19f93c6@valdikss.org.ru>
Hi ValdikSS,
thanks for your follow-up and investigation. Sorry for pointing the finger
in the wrong direction. It's weird to me that the problem can be worked around
reliably by unloading / blacklisting hid-lenovo. But then, as I said, I have
no real knowledge about either bluetoth or the input layer..
On Wed, Sep 11, 2024 at 02:51:27AM +0300, ValdikSS wrote:
> On 11.09.2024 01:07, ValdikSS wrote:
> > You found a REGRESSION (and not in hid_lenovo)! Look below.
> >
> > [...]
>
> The regression is caused by bluez:
> https://github.com/bluez/bluez/issues/949
Thanks, let's ee what the bluez maintainers have to say...
--
- Harald Welte <laforge@gnumonks.org> https://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
^ permalink raw reply
* Re: hid-lenovo breaks middle mouse button on tpIIkbd
From: ValdikSS @ 2024-09-10 23:51 UTC (permalink / raw)
To: Harald Welte; +Cc: linux-input
In-Reply-To: <8e4a1afd-f82c-4262-b05b-ff5a1c18bee6@valdikss.org.ru>
On 11.09.2024 01:07, ValdikSS wrote:
> You found a REGRESSION (and not in hid_lenovo)! Look below.
>
> The keyboard correctly enters native mode only after pairing or full
> Bluetooth reloading. Middle button does not work for me if the keyboard
> is just disconnected or powered off/on.
>
> This seems to be because Bluetooth layer does not disconnect the input
> device upon disconnecting Bluetooth, and reconnecting the keyboard just
> "resumes" the input device, but does not re-initialize it properly. You
> can see that by not-lid left LED indicator (ESC button). It should be
> lid by default if initialized properly.
>
> Known good configuration from Fedora 39:
> kernel 6.5.6
> bluez 5.69
The regression is caused by bluez:
https://github.com/bluez/bluez/issues/949
^ permalink raw reply
* Re: [PATCH] Input: ims-pcu - fix calling interruptible mutex
From: Dmitry Torokhov @ 2024-09-10 23:15 UTC (permalink / raw)
To: David Lechner; +Cc: linux-input, linux-kernel
In-Reply-To: <20240910-input-misc-ims-pcu-fix-mutex-intr-v1-1-bdd983685c43@baylibre.com>
On Tue, Sep 10, 2024 at 04:58:47PM -0500, David Lechner wrote:
> Fix calling scoped_cond_guard() with mutex instead of mutex_intr.
>
> scoped_cond_guard(mutex, ...) will call mutex_lock() instead of
> mutex_lock_interruptible().
>
> Fixes: 703f12672e1f ("Input: ims-pcu - switch to using cleanup functions")
> Signed-off-by: David Lechner <dlechner@baylibre.com>
Applied, thank you.
Too bad it does not warn when incorrect type of guard object is being
used with scoped_cond_guard()...
--
Dmitry
^ permalink raw reply
* Re: [PATCH v7 2/2] dt-bindings: input: Goodix SPI HID Touchscreen
From: Rob Herring @ 2024-09-10 22:41 UTC (permalink / raw)
To: Charles Wang
Cc: dmitry.torokhov, dianders, dan.carpenter, conor, krzk+dt, jikos,
bentiss, hbarnor, linux-input, devicetree, linux-kernel,
Conor Dooley
In-Reply-To: <CAL_Jsq+QfTtRj_JCqXzktQ49H8VUnztVuaBjvvkg3fwEHniUHw@mail.gmail.com>
On Fri, Sep 6, 2024 at 3:28 PM Rob Herring <robh@kernel.org> wrote:
>
> On Tue, Aug 13, 2024 at 9:45 PM Charles Wang <charles.goodix@gmail.com> wrote:
> >
> > The Goodix GT7986U touch controller report touch data according to the
> > HID protocol through the SPI bus. However, it is incompatible with
> > Microsoft's HID-over-SPI protocol.
> >
> > Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> > Signed-off-by: Charles Wang <charles.goodix@gmail.com>
> > ---
> > .../bindings/input/goodix,gt7986u.yaml | 71 +++++++++++++++++++
> > 1 file changed, 71 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/input/goodix,gt7986u.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/input/goodix,gt7986u.yaml b/Documentation/devicetree/bindings/input/goodix,gt7986u.yaml
> > new file mode 100644
> > index 000000000..a7d42a5d6
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/input/goodix,gt7986u.yaml
> > @@ -0,0 +1,71 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/input/goodix,gt7986u.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: GOODIX GT7986U SPI HID Touchscreen
> > +
> > +maintainers:
> > + - Charles Wang <charles.goodix@gmail.com>
> > +
> > +description: Supports the Goodix GT7986U touchscreen.
> > + This touch controller reports data packaged according to the HID protocol,
> > + but is incompatible with Microsoft's HID-over-SPI protocol.
> > +
> > +allOf:
> > + - $ref: /schemas/spi/spi-peripheral-props.yaml#
> > +
> > +properties:
> > + compatible:
> > + enum:
> > + - goodix,gt7986u
>
> This is already documented in goodix,gt7375p.yaml. Now linux-next has warnings:
>
> /builds/robherring/linux-dt/Documentation/devicetree/bindings/input/goodix,gt7986u.example.dtb:
> touchscreen@0: compatible: 'oneOf' conditional failed, one must be
> fixed:
> ['goodix,gt7986u'] is too short
> 'goodix,gt7375p' was expected
> from schema $id:
> http://devicetree.org/schemas/input/goodix,gt7375p.yaml#
> /builds/robherring/linux-dt/Documentation/devicetree/bindings/input/goodix,gt7986u.example.dtb:
> touchscreen@0: reg:0:0: 0 is not one of [93, 20]
> from schema $id:
> http://devicetree.org/schemas/input/goodix,gt7375p.yaml#
> /builds/robherring/linux-dt/Documentation/devicetree/bindings/input/goodix,gt7986u.example.dtb:
> touchscreen@0: 'vdd-supply' is a required property
> from schema $id:
> http://devicetree.org/schemas/input/goodix,gt7375p.yaml#
> /builds/robherring/linux-dt/Documentation/devicetree/bindings/input/goodix,gt7986u.example.dtb:
> touchscreen@0: 'goodix,hid-report-addr', 'spi-max-frequency' do not
> match any of the regexes: 'pinctrl-[0-9]+'
> from schema $id:
> http://devicetree.org/schemas/input/goodix,gt7375p.yaml#
>
> Please sort this out and send a fix.
I should add that it is intermittent whether you see this error or
not. The tools select a single schema based on the compatible string
and it is undefined which of the 2 schemas you will get.
Rob
^ permalink raw reply
* Re: hid-lenovo breaks middle mouse button on tpIIkbd
From: ValdikSS @ 2024-09-10 22:07 UTC (permalink / raw)
To: Harald Welte; +Cc: linux-input
In-Reply-To: <Ztwoai8_1L0rJkYp@nataraja>
On 07.09.2024 13:18, Harald Welte wrote:
> Dear ValdikSS,
> Dear Linux Input maintainers,
Hello Harald,
Nice to meet you, I know you from Osmocom project!
You found a REGRESSION (and not in hid_lenovo)! Look below.
> The keyboard has a small slider swithc to select between Android and
> Windows mode. I've set it to "windows" mode, but couldn't observe any
> difference in the bug in both modes.
You need to use Windows mode only. Android mode does not have all the
capabilities.
> The keyboard shows up as /sys/kernel/debug/hid/0005:17EF:60E1.000F
Same for me.
>
> == btmon
>
> * the events are visible on bluetooth with "BTMON". Middle mouse button
> press+release looks like this:
>
>> ACL Data RX: Handle 10 flags 0x02 dlen 11
> ATT: Handle Value Notification (0x1b) len 6
> Handle: 0x001c
> Data[4]: 04000000
>> ACL Data RX: Handle 10 flags 0x02 dlen 11
> ATT: Handle Value Notification (0x1b) len 6
> Handle: 0x001c
> Data[4]: 00000000
Hrm, these events are generated when the keyboard is not in "native"
mode (when the kernel module is not loaded for example).
Here's what the middle button should generate in "native" mode (over
Bluetooth):
> ACL Data RX: Handle 2048 flags 0x02 dlen 15
ATT: Handle Value Notification (0x1b) len 10
Handle: 0x0024
Data[8]: 0004020000000000
> ACL Data RX: Handle 2048 flags 0x02 dlen 15
ATT: Handle Value Notification (0x1b) len 10
Handle: 0x0024
Data[8]: 0000020000000000
But you're right, something does not work correctly.
The keyboard correctly enters native mode only after pairing or full
Bluetooth reloading. Middle button does not work for me if the keyboard
is just disconnected or powered off/on.
This seems to be because Bluetooth layer does not disconnect the input
device upon disconnecting Bluetooth, and reconnecting the keyboard just
"resumes" the input device, but does not re-initialize it properly. You
can see that by not-lid left LED indicator (ESC button). It should be
lid by default if initialized properly.
I haven't thoroughly used the keyboard for about a year, and last time I
used it, I don't remember such issue.
Just booted up Fedora 39 from ISO (not updated, release from November
2023) on 2 of my computers, and yes, it works correctly: the keyboard is
detected as new input device (in dmesg) on every power off/power on
cycle, and initialized correctly every time, middle button works.
Fedora 40 ISO (from 15 Apr 2024) — does not work, power off/power on
results in broken middle button, keyboard is NOT detected as new input
device in dmesg upon off/on (it "resumes" old input device).
Known good configuration from Fedora 39:
kernel 6.5.6
bluez 5.69
Known bad configuration from Fedora 40:
kernel 6.8.5
bluez 5.73
Old Fedora 39 doesn't ever detect the keyboard as "bluez-hog-device",
maybe that could be the clue.
All I can say is that lenovo_hid is not the culprit — I tried to use my
old module and it doesn't work on a new system either.
I don't have much time to run bisect right now, sorry.
^ permalink raw reply
* [PATCH] Input: ims-pcu - fix calling interruptible mutex
From: David Lechner @ 2024-09-10 21:58 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, linux-kernel, David Lechner
Fix calling scoped_cond_guard() with mutex instead of mutex_intr.
scoped_cond_guard(mutex, ...) will call mutex_lock() instead of
mutex_lock_interruptible().
Fixes: 703f12672e1f ("Input: ims-pcu - switch to using cleanup functions")
Signed-off-by: David Lechner <dlechner@baylibre.com>
---
drivers/input/misc/ims-pcu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c
index c086dadb45e3..058f3470b7ae 100644
--- a/drivers/input/misc/ims-pcu.c
+++ b/drivers/input/misc/ims-pcu.c
@@ -1067,7 +1067,7 @@ static ssize_t ims_pcu_attribute_store(struct device *dev,
if (data_len > attr->field_length)
return -EINVAL;
- scoped_cond_guard(mutex, return -EINTR, &pcu->cmd_mutex) {
+ scoped_cond_guard(mutex_intr, return -EINTR, &pcu->cmd_mutex) {
memset(field, 0, attr->field_length);
memcpy(field, buf, data_len);
---
base-commit: 6708132e80a2ced620bde9b9c36e426183544a23
change-id: 20240910-input-misc-ims-pcu-fix-mutex-intr-9bb7b7a6dda2
Best regards,
--
David Lechner <dlechner@baylibre.com>
^ permalink raw reply related
* Re: [PATCH 1/2] HID: wacom: Support sequence numbers smaller than 16-bit
From: Jiri Kosina @ 2024-09-10 21:48 UTC (permalink / raw)
To: Jason Gerecke
Cc: linux-input, Benjamin Tissoires, Ping Cheng, Joshua Dickens,
Jason Gerecke, Joshua Dickens
In-Reply-To: <CANRwn3ROebV4gj2w9Q2q7xDSKL0qEiH+Knx_jF55FqQLZ1YRcA@mail.gmail.com>
On Tue, 10 Sep 2024, Jason Gerecke wrote:
> These are pretty minor fixes, so I agree that there is no urgent need
> to get them into 6.11. Feel free to queue them for 6.12 instead.
Thanks. Both now queued in hid.git#for-6.12/wacom.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH 1/2] HID: wacom: Support sequence numbers smaller than 16-bit
From: Jason Gerecke @ 2024-09-10 18:58 UTC (permalink / raw)
To: Jiri Kosina
Cc: linux-input, Benjamin Tissoires, Ping Cheng, Joshua Dickens,
Jason Gerecke, Joshua Dickens
In-Reply-To: <nycvar.YFH.7.76.2409092238300.31206@cbobk.fhfr.pm>
On Mon, Sep 9, 2024 at 1:40 PM Jiri Kosina <jikos@kernel.org> wrote:
>
> On Mon, 9 Sep 2024, Gerecke, Jason wrote:
>
> > From: Jason Gerecke <jason.gerecke@wacom.com>
> >
> > The current dropped packet reporting assumes that all sequence numbers
> > are 16 bits in length. This results in misleading "Dropped" messages if
> > the hardware uses fewer bits. For example, if a tablet uses only 8 bits
> > to store its sequence number, once it rolls over from 255 -> 0, the
> > driver will still be expecting a packet "256". This patch adjusts the
> > logic to reset the next expected packet to logical_minimum whenever
> > it overflows beyond logical_maximum.
> >
> > Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
> > Tested-by: Joshua Dickens <joshua.dickens@wacom.com>
> > Fixes: 6d09085b38e5 ("HID: wacom: Adding Support for new usages")
>
> Hi Jason,
>
> thanks for both fixes.
>
> Looking at them, it seems like it'd normally be 6.11 material, but given
>
> - how far we currently are in a 6.11 development cycle
> - how long this issue has been present in the code
> - the fact that it actually does change the event processing behavior
>
> I have to ask you for a judgement -- would you like to absolutely see
> these in 6.11 as last-minute fixes, or do you consider this to be 6.12
> material?
>
> Thanks,
>
> --
> Jiri Kosina
> SUSE Labs
>
Hi Jiri,
These are pretty minor fixes, so I agree that there is no urgent need
to get them into 6.11. Feel free to queue them for 6.12 instead.
Jason (she/they)
---
Now instead of four in the eights place /
you’ve got three, ‘Cause you added one /
(That is to say, eight) to the two, /
But you can’t take seven from three, /
So you look at the sixty-fours....
^ permalink raw reply
* Re: [PATCH v6 2/2] input: add driver for Hynitron CST816X touchscreen
From: Jeff LaBundy @ 2024-09-10 15:47 UTC (permalink / raw)
To: Oleh Kuzhylnyi
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
linux-input, devicetree, linux-kernel, Conor Dooley, igor.opaniuk,
Neil Armstrong
In-Reply-To: <20240910115158.74502-2-kuzhylol@gmail.com>
Hi Oleh,
Great work! Just a few comments throughout.
On Tue, Sep 10, 2024 at 01:51:58PM +0200, Oleh Kuzhylnyi wrote:
> Introduce support for the Hynitron CST816X touchscreen controller
> used for 240×240 1.28-inch Round LCD Display Module manufactured
> by Waveshare Electronics. The driver is designed based on an Arduino
> implementation marked as under MIT License. This driver is written
> for a particular round display based on the CST816S controller, which
> is not compatiable with existing driver for Hynitron controllers.
>
> Signed-off-by: Oleh Kuzhylnyi <kuzhylol@gmail.com>
> ---
>
> Changes in v6:
> - No code changes
>
> Changes in v5:
> - Update commit based on Dmitry's feedback:
> - Make GPIO reset optional
> - Combine declaration and initialization for i2c_xfer
> - Return 0 explicitly where possible
> - Rename rc (return code) to error
> - Make Touch processing call return boolean
> - Improve error handling for i2c_transfer
> - Use get_unaligned_be16 for getting coordinates
> - Move touch event completeness upper to irq callback
>
> Changes in v4:
> - Update commit based on Dmitry's feedback:
> - Move abs_x and abs_y to u16
> - Remove __packed qualifier for touch_info struct
> - Hide tiny touch irq context to stack
> - Extend cst816x_i2c_read_register() with buf and buf_size
> - Remove loop from event lookup
>
> Changes in v3:
> - Drop timer and delayed work
> - Process touch in threaded IRQ context
> - Fix chip reset sequence
> - Move input_register() before devm_request_threaded_irq()
> - Re-arrange and document input reporting
> - Set u16 data type for event_code
> - Remove double tap event to prevent continuous double side sliding
>
> Changes in v2:
> - Apply dev_err_probe() for better error handling
> - Remove redundant printing, remove dev_warn() message spamming
> - Get rid of PM since the touchscreen goes into sleep mode automatically
> - Get rid of IRQ control and IRQF_NO_AUTOEN flag
> - Reduce timer timeout up to 10ms to handle touch events faster
> - Skip registering of non-gesture CST816X_SWIPE event
> - Shift input_register_device() as a final call in probe() callback
> - Specify name of i2c_device_id explicitly
> - Update module description and fix typo
> - Add necessary spaces between lines
> drivers/input/touchscreen/Kconfig | 12 +
> drivers/input/touchscreen/Makefile | 1 +
> drivers/input/touchscreen/hynitron-cst816x.c | 259 +++++++++++++++++++
> 3 files changed, 272 insertions(+)
> create mode 100644 drivers/input/touchscreen/hynitron-cst816x.c
>
> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> index c821fe3ee794..02f40d0fbac0 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -481,6 +481,18 @@ config TOUCHSCREEN_HYNITRON_CSTXXX
> To compile this driver as a module, choose M here: the
> module will be called hynitron-cstxxx.
>
> +config TOUCHSCREEN_HYNITRON_CST816X
> + tristate "Hynitron CST816X touchscreen support"
> + depends on I2C
> + help
> + Say Y here if you have a touchscreen using a Hynitron
> + CST816X touchscreen controller.
> +
> + If unsure, say N.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called hynitron-cst816x.
> +
> config TOUCHSCREEN_ILI210X
> tristate "Ilitek ILI210X based touchscreen"
> depends on I2C
> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> index a81cb5aa21a5..a92a87417a97 100644
> --- a/drivers/input/touchscreen/Makefile
> +++ b/drivers/input/touchscreen/Makefile
> @@ -51,6 +51,7 @@ obj-$(CONFIG_TOUCHSCREEN_GOODIX_BERLIN_CORE) += goodix_berlin_core.o
> obj-$(CONFIG_TOUCHSCREEN_GOODIX_BERLIN_I2C) += goodix_berlin_i2c.o
> obj-$(CONFIG_TOUCHSCREEN_GOODIX_BERLIN_SPI) += goodix_berlin_spi.o
> obj-$(CONFIG_TOUCHSCREEN_HIDEEP) += hideep.o
> +obj-$(CONFIG_TOUCHSCREEN_HYNITRON_CST816X) += hynitron-cst816x.o
> obj-$(CONFIG_TOUCHSCREEN_HYNITRON_CSTXXX) += hynitron_cstxxx.o
> obj-$(CONFIG_TOUCHSCREEN_ILI210X) += ili210x.o
> obj-$(CONFIG_TOUCHSCREEN_ILITEK) += ilitek_ts_i2c.o
> diff --git a/drivers/input/touchscreen/hynitron-cst816x.c b/drivers/input/touchscreen/hynitron-cst816x.c
> new file mode 100644
> index 000000000000..3886617e6a71
> --- /dev/null
> +++ b/drivers/input/touchscreen/hynitron-cst816x.c
> @@ -0,0 +1,259 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Driver for I2C connected Hynitron CST816X Touchscreen
> + *
> + * Copyright (C) 2024 Oleh Kuzhylnyi <kuzhylol@gmail.com>
> + */
> +#include <asm/unaligned.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/i2c.h>
> +#include <linux/input.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +
> +enum cst816x_registers {
> + CST816X_FRAME = 0x01,
> + CST816X_MOTION = 0xEC,
> +};
> +
> +enum cst816x_gestures {
> + CST816X_SWIPE_UP = 0x01,
> + CST816X_SWIPE_DOWN = 0x02,
> + CST816X_SWIPE_LEFT = 0x03,
> + CST816X_SWIPE_RIGHT = 0x04,
> + CST816X_SINGLE_TAP = 0x05,
> + CST816X_LONG_PRESS = 0x0C,
> + CST816X_RESERVED = 0xFF,
> +};
> +
> +struct cst816x_touch_info {
> + u8 gesture;
> + u8 touch;
> + u16 abs_x;
> + u16 abs_y;
> +};
> +
> +struct cst816x_priv {
> + struct device *dev;
> + struct i2c_client *client;
I don't see any value in storing both the device and i2c_client pointers
in the private struct; I think you can simply drop the former.
> + struct gpio_desc *reset;
> + struct input_dev *input;
> +};
> +
> +struct cst816x_event_mapping {
> + enum cst816x_gestures gesture;
> + u16 code;
> +};
> +
> +static const struct cst816x_event_mapping event_map[16] = {
> + {CST816X_SWIPE_UP, BTN_FORWARD},
> + {CST816X_SWIPE_DOWN, BTN_BACK},
> + {CST816X_SWIPE_LEFT, BTN_LEFT},
> + {CST816X_SWIPE_RIGHT, BTN_RIGHT},
> + {CST816X_SINGLE_TAP, BTN_TOUCH},
> + {CST816X_LONG_PRESS, BTN_TOOL_TRIPLETAP},
> + {CST816X_RESERVED, KEY_RESERVED},
> + {CST816X_RESERVED, KEY_RESERVED},
> + {CST816X_RESERVED, KEY_RESERVED},
> + {CST816X_RESERVED, KEY_RESERVED},
> + {CST816X_RESERVED, KEY_RESERVED},
> + {CST816X_RESERVED, KEY_RESERVED},
> + {CST816X_RESERVED, KEY_RESERVED},
> + {CST816X_RESERVED, KEY_RESERVED},
> + {CST816X_RESERVED, KEY_RESERVED},
> + {CST816X_RESERVED, KEY_RESERVED},
> +};
These really should be configurable via device tree and not hard coded
in the driver. At the very least, if the touchscreen is installed 180
degrees for certain platforms, the concept of "left" and "right" changes.
> +
> +static int cst816x_i2c_read_register(struct cst816x_priv *priv, u8 reg,
> + void *buf, size_t len)
> +{
> + int rc;
> + struct i2c_msg xfer[] = {
> + {
> + .addr = priv->client->addr,
> + .flags = 0,
> + .buf = ®,
> + .len = sizeof(reg),
> + },
> + {
> + .addr = priv->client->addr,
> + .flags = I2C_M_RD,
> + .buf = buf,
> + .len = len,
> + },
> + };
> +
> + rc = i2c_transfer(priv->client->adapter, xfer, ARRAY_SIZE(xfer));
> + if (rc != ARRAY_SIZE(xfer)) {
> + rc = rc < 0 ? rc : -EIO;
> + dev_err(&priv->client->dev, "i2c rx err: %d\n", rc);
Case in point: you already have priv->dev, but it's not being used.
> + return rc;
> + }
> +
> + return 0;
> +}
> +
> +static bool cst816x_process_touch(struct cst816x_priv *priv,
> + struct cst816x_touch_info *info)
> +{
> + u8 raw[8];
> +
> + if (cst816x_i2c_read_register(priv, CST816X_FRAME, raw, sizeof(raw)))
> + return false;
> +
> + info->gesture = raw[0];
> + info->touch = raw[1];
> + info->abs_x = get_unaligned_be16(&raw[2]) & GENMASK(11, 0);
> + info->abs_y = get_unaligned_be16(&raw[4]) & GENMASK(11, 0);
This seems like a good case to make cst816x_touch_info a __packed struct
with abs_x and abs_y declared as__be16 members. You can then read into an
instance of this struct and unpack as necessary, as opposed to essentially
having two buffers and copying one into the other manually.
> +
> + dev_dbg(priv->dev, "x: %d, y: %d, t: %d, g: 0x%x\n", info->abs_x,
> + info->abs_y, info->touch, info->gesture);
> +
> + return true;
> +}
> +
> +static int cst816x_register_input(struct cst816x_priv *priv)
> +{
> + priv->input = devm_input_allocate_device(priv->dev);
> + if (!priv->input)
> + return -ENOMEM;
> +
> + priv->input->name = "Hynitron CST816X Touchscreen";
> + priv->input->phys = "input/ts";
> + priv->input->id.bustype = BUS_I2C;
> + input_set_drvdata(priv->input, priv);
> +
> + for (unsigned int i = 0; i < ARRAY_SIZE(event_map); i++)
> + input_set_capability(priv->input, EV_KEY, event_map[i].code);
Nit: it's much more common in kernel code for iterators to be of type int.
> +
> + input_set_abs_params(priv->input, ABS_X, 0, 240, 0, 0);
> + input_set_abs_params(priv->input, ABS_Y, 0, 240, 0, 0);
I see the binding includes the touchscreen helper binding, but you're not
actually using any of the helpers. Assuming that's intentional, please drop
the reference to touchscreen.yaml in the binding.
> +
> + return input_register_device(priv->input);
> +}
> +
> +static void cst816x_reset(struct cst816x_priv *priv)
> +{
> + if (priv->reset) {
> + gpiod_set_value_cansleep(priv->reset, 1);
> + msleep(50);
> + gpiod_set_value_cansleep(priv->reset, 0);
> + msleep(100);
> + }
This is a style choice, but you can save some indentation by doing
the following:
if (!priv->reset)
return;
gpiod_set_value_cansleep(...);
> +}
> +
> +static void report_gesture_event(const struct cst816x_priv *priv,
> + enum cst816x_gestures gesture, bool touch)
> +{
> + u16 key = event_map[gesture & 0x0F].code;
> +
> + if (key != KEY_RESERVED)
> + input_report_key(priv->input, key, touch);
There is no need to manually filter KEY_RESERVED; the input core
automatically marks this key code as unsupported upon registration.
> +}
> +
> +/*
> + * Supports five gestures: TOUCH, LEFT, RIGHT, FORWARD, BACK, and LONG_PRESS.
> + * Reports surface interaction, sliding coordinates and finger detachment.
> + *
> + * 1. TOUCH Gesture Scenario:
> + *
> + * [x/y] [touch] [gesture] [Action] [Report ABS] [Report Key]
> + * x y true 0x00 Touch ABS_X_Y BTN_TOUCH
> + * x y true 0x00 Slide ABS_X_Y
> + * x y false 0x05 Gesture BTN_TOUCH
> + *
> + * 2. LEFT, RIGHT, FORWARD, BACK, and LONG_PRESS Gestures Scenario:
> + *
> + * [x/y] [touch] [gesture] [Action] [Report ABS] [Report Key]
> + * x y true 0x00 Touch ABS_X_Y BTN_TOUCH
> + * x y true 0x01 Gesture ABS_X_Y BTN_FORWARD
> + * x y true 0x01 Slide ABS_X_Y
> + * x y false 0x01 Detach BTN_FORWARD | BTN_TOUCH
> + */
This is one very specific implementation, and too restrictive to be
hard coded into the driver. As mentioned above, please consider making
gesture key codes configurable by way of the device tree.
> +static irqreturn_t cst816x_irq_cb(int irq, void *cookie)
> +{
> + struct cst816x_priv *priv = cookie;
> + struct cst816x_touch_info info;
> +
> + if (!cst816x_process_touch(priv, &info))
> + goto out;
This is mostly a style choice, but it would also be fine to simply
return directly here since there is no clean-up to do.
> +
> + if (info.touch) {
> + input_report_abs(priv->input, ABS_X, info.abs_x);
> + input_report_abs(priv->input, ABS_Y, info.abs_y);
> + input_report_key(priv->input, BTN_TOUCH, 1);
> + }
> +
> + if (info.gesture) {
> + report_gesture_event(priv, info.gesture, info.touch);
> +
> + if (!info.touch)
> + input_report_key(priv->input, BTN_TOUCH, 0);
> + }
> +
> + input_sync(priv->input);
> +
> +out:
> + return IRQ_HANDLED;
> +}
> +
> +static int cst816x_probe(struct i2c_client *client)
> +{
> + struct cst816x_priv *priv;
> + struct device *dev = &client->dev;
> + int error;
> +
> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + priv->dev = dev;
> + priv->client = client;
> +
> + priv->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> + if (IS_ERR(priv->reset))
> + return dev_err_probe(dev, PTR_ERR(priv->reset),
> + "reset gpio not found\n");
> +
> + cst816x_reset(priv);
> +
> + error = cst816x_register_input(priv);
> + if (error)
> + return dev_err_probe(dev, error, "input register failed\n");
> +
> + error = devm_request_threaded_irq(dev, client->irq, NULL, cst816x_irq_cb,
> + IRQF_ONESHOT, dev->driver->name, priv);
> + if (error)
> + return dev_err_probe(dev, error, "irq request failed\n");
> +
> + return 0;
> +}
> +
> +static const struct i2c_device_id cst816x_id[] = {
> + { .name = "cst816s", 0 },
> + { }
> +};
> +MODULE_DEVICE_TABLE(i2c, cst816x_id);
> +
> +static const struct of_device_id cst816x_of_match[] = {
> + { .compatible = "hynitron,cst816s", },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, cst816x_of_match);
> +
> +static struct i2c_driver cst816x_driver = {
> + .driver = {
> + .name = "cst816x",
> + .of_match_table = cst816x_of_match,
> + },
> + .id_table = cst816x_id,
> + .probe = cst816x_probe,
> +};
> +
> +module_i2c_driver(cst816x_driver);
> +
> +MODULE_AUTHOR("Oleh Kuzhylnyi <kuzhylol@gmail.com>");
> +MODULE_DESCRIPTION("Hynitron CST816X Touchscreen Driver");
> +MODULE_LICENSE("GPL");
> --
> 2.34.1
>
Kind regards,
Jeff LaBundy
^ permalink raw reply
* [PATCH -next v3 05/15] HID: mcp2221: Use devm_hid_hw_start_and_open in mcp2221_probe()
From: Li Zetao @ 2024-09-10 15:45 UTC (permalink / raw)
To: jikos, bentiss, michael.zaidman, djogorchock, rrameshbabu,
bonbons, roderick.colenbrander, david, savicaleksa83, me,
jdelvare, linux, mail, wilken.gottwalt, jonas, mezin.alexander
Cc: lizetao1, linux-input, linux-i2c, linux-hwmon
In-Reply-To: <20240910154545.736786-1-lizetao1@huawei.com>
Currently, the mcp2221 module use devm_add_action_or_reset() to manage
device resource for HID unregistration, now that a universal interface
has been provided, use a universal interface to replace it.
Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
v2 -> v3: None
v2: https://lore.kernel.org/all/20240909012313.500341-6-lizetao1@huawei.com/
v1 -> v2: None
v1: https://lore.kernel.org/all/20240904123607.3407364-6-lizetao1@huawei.com/
drivers/hid/hid-mcp2221.c | 26 ++------------------------
1 file changed, 2 insertions(+), 24 deletions(-)
diff --git a/drivers/hid/hid-mcp2221.c b/drivers/hid/hid-mcp2221.c
index 0f93c22a479f..3b8269f7e923 100644
--- a/drivers/hid/hid-mcp2221.c
+++ b/drivers/hid/hid-mcp2221.c
@@ -932,15 +932,6 @@ static int mcp2221_raw_event(struct hid_device *hdev,
return 1;
}
-/* Device resource managed function for HID unregistration */
-static void mcp2221_hid_unregister(void *ptr)
-{
- struct hid_device *hdev = ptr;
-
- hid_hw_close(hdev);
- hid_hw_stop(hdev);
-}
-
/* This is needed to be sure hid_hw_stop() isn't called twice by the subsystem */
static void mcp2221_remove(struct hid_device *hdev)
{
@@ -1141,31 +1132,18 @@ static int mcp2221_probe(struct hid_device *hdev,
* This driver uses the .raw_event callback and therefore does not need any
* HID_CONNECT_xxx flags.
*/
- ret = hid_hw_start(hdev, 0);
- if (ret) {
- hid_err(hdev, "can't start hardware\n");
+ ret = devm_hid_hw_start_and_open(hdev, 0);
+ if (ret)
return ret;
- }
hid_info(hdev, "USB HID v%x.%02x Device [%s] on %s\n", hdev->version >> 8,
hdev->version & 0xff, hdev->name, hdev->phys);
- ret = hid_hw_open(hdev);
- if (ret) {
- hid_err(hdev, "can't open device\n");
- hid_hw_stop(hdev);
- return ret;
- }
-
mutex_init(&mcp->lock);
init_completion(&mcp->wait_in_report);
hid_set_drvdata(hdev, mcp);
mcp->hdev = hdev;
- ret = devm_add_action_or_reset(&hdev->dev, mcp2221_hid_unregister, hdev);
- if (ret)
- return ret;
-
hid_device_io_start(hdev);
/* Set I2C bus clock diviser */
--
2.34.1
^ permalink raw reply related
* [PATCH -next v3 04/15] HID: mcp2200: Use devm_hid_hw_start_and_open in mcp2200_probe()
From: Li Zetao @ 2024-09-10 15:45 UTC (permalink / raw)
To: jikos, bentiss, michael.zaidman, djogorchock, rrameshbabu,
bonbons, roderick.colenbrander, david, savicaleksa83, me,
jdelvare, linux, mail, wilken.gottwalt, jonas, mezin.alexander
Cc: lizetao1, linux-input, linux-i2c, linux-hwmon
In-Reply-To: <20240910154545.736786-1-lizetao1@huawei.com>
Currently, the mcp2200 module needs to maintain hid resources
by itself. Use devm_hid_hw_start_and_open helper to ensure that hid
resources are consistent with the device life cycle, and release
hid resources before device is released. So there is no need to close and
stop hid when an error occurs. At the same time, since there is no need to
do any operations in mcp2200_remove() now, so delete .remote operation.
Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
v2 -> v3: None
v2: https://lore.kernel.org/all/20240909012313.500341-5-lizetao1@huawei.com/
v1 -> v2: Adjust commit information
v1: https://lore.kernel.org/all/20240904123607.3407364-5-lizetao1@huawei.com/
drivers/hid/hid-mcp2200.c | 22 ++--------------------
1 file changed, 2 insertions(+), 20 deletions(-)
diff --git a/drivers/hid/hid-mcp2200.c b/drivers/hid/hid-mcp2200.c
index bf57f7f6caa0..56d72fc5623d 100644
--- a/drivers/hid/hid-mcp2200.c
+++ b/drivers/hid/hid-mcp2200.c
@@ -329,22 +329,13 @@ static int mcp2200_probe(struct hid_device *hdev, const struct hid_device_id *id
return ret;
}
- ret = hid_hw_start(hdev, 0);
- if (ret) {
- hid_err(hdev, "can't start hardware\n");
+ ret = devm_hid_hw_start_and_open(hdev, 0);
+ if (ret)
return ret;
- }
hid_info(hdev, "USB HID v%x.%02x Device [%s] on %s\n", hdev->version >> 8,
hdev->version & 0xff, hdev->name, hdev->phys);
- ret = hid_hw_open(hdev);
- if (ret) {
- hid_err(hdev, "can't open device\n");
- hid_hw_stop(hdev);
- return ret;
- }
-
mutex_init(&mcp->lock);
init_completion(&mcp->wait_in_report);
hid_set_drvdata(hdev, mcp);
@@ -356,20 +347,12 @@ static int mcp2200_probe(struct hid_device *hdev, const struct hid_device_id *id
ret = devm_gpiochip_add_data(&hdev->dev, &mcp->gc, mcp);
if (ret < 0) {
hid_err(hdev, "Unable to register gpiochip\n");
- hid_hw_close(hdev);
- hid_hw_stop(hdev);
return ret;
}
return 0;
}
-static void mcp2200_remove(struct hid_device *hdev)
-{
- hid_hw_close(hdev);
- hid_hw_stop(hdev);
-}
-
static const struct hid_device_id mcp2200_devices[] = {
{ HID_USB_DEVICE(USB_VENDOR_ID_MICROCHIP, USB_DEVICE_ID_MCP2200) },
{ }
@@ -380,7 +363,6 @@ static struct hid_driver mcp2200_driver = {
.name = "mcp2200",
.id_table = mcp2200_devices,
.probe = mcp2200_probe,
- .remove = mcp2200_remove,
.raw_event = mcp2200_raw_event,
};
--
2.34.1
^ permalink raw reply related
* [PATCH -next v3 12/15] hwmon: (gigabyte_waterforce) Use devm_hid_hw_start_and_open in waterforce_probe()
From: Li Zetao @ 2024-09-10 15:45 UTC (permalink / raw)
To: jikos, bentiss, michael.zaidman, djogorchock, rrameshbabu,
bonbons, roderick.colenbrander, david, savicaleksa83, me,
jdelvare, linux, mail, wilken.gottwalt, jonas, mezin.alexander
Cc: lizetao1, linux-input, linux-i2c, linux-hwmon
In-Reply-To: <20240910154545.736786-1-lizetao1@huawei.com>
Currently, the waterforce module needs to maintain hid resources
by itself. Use devm_hid_hw_start_and_open helper to ensure that hid
resources are consistent with the device life cycle, and release
hid resources before device is released. At the same time, it can avoid
the goto-release encoding, drop the fail_and_close and fail_and_stop
lables, and directly return the error code when an error occurs.
Reviewed-by: Aleksa Savic <savicaleksa83@gmail.com>
Acked-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
v2 -> v3: None
v2: https://lore.kernel.org/all/20240909012313.500341-13-lizetao1@huawei.com/
v1 -> v2: Adjust commit information
v1: https://lore.kernel.org/all/20240904123607.3407364-16-lizetao1@huawei.com/
drivers/hwmon/gigabyte_waterforce.c | 29 +++++------------------------
1 file changed, 5 insertions(+), 24 deletions(-)
diff --git a/drivers/hwmon/gigabyte_waterforce.c b/drivers/hwmon/gigabyte_waterforce.c
index 8129d7b3ceaf..9052d1c3d5aa 100644
--- a/drivers/hwmon/gigabyte_waterforce.c
+++ b/drivers/hwmon/gigabyte_waterforce.c
@@ -337,23 +337,13 @@ static int waterforce_probe(struct hid_device *hdev, const struct hid_device_id
/*
* Enable hidraw so existing user-space tools can continue to work.
*/
- ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
- if (ret) {
- hid_err(hdev, "hid hw start failed with %d\n", ret);
+ ret = devm_hid_hw_start_and_open(hdev, HID_CONNECT_HIDRAW);
+ if (ret)
return ret;
- }
-
- ret = hid_hw_open(hdev);
- if (ret) {
- hid_err(hdev, "hid hw open failed with %d\n", ret);
- goto fail_and_stop;
- }
priv->buffer = devm_kzalloc(&hdev->dev, MAX_REPORT_LENGTH, GFP_KERNEL);
- if (!priv->buffer) {
- ret = -ENOMEM;
- goto fail_and_close;
- }
+ if (!priv->buffer)
+ return -ENOMEM;
mutex_init(&priv->status_report_request_mutex);
mutex_init(&priv->buffer_lock);
@@ -371,18 +361,12 @@ static int waterforce_probe(struct hid_device *hdev, const struct hid_device_id
if (IS_ERR(priv->hwmon_dev)) {
ret = PTR_ERR(priv->hwmon_dev);
hid_err(hdev, "hwmon registration failed with %d\n", ret);
- goto fail_and_close;
+ return ret;
}
waterforce_debugfs_init(priv);
return 0;
-
-fail_and_close:
- hid_hw_close(hdev);
-fail_and_stop:
- hid_hw_stop(hdev);
- return ret;
}
static void waterforce_remove(struct hid_device *hdev)
@@ -391,9 +375,6 @@ static void waterforce_remove(struct hid_device *hdev)
debugfs_remove_recursive(priv->debugfs);
hwmon_device_unregister(priv->hwmon_dev);
-
- hid_hw_close(hdev);
- hid_hw_stop(hdev);
}
static const struct hid_device_id waterforce_table[] = {
--
2.34.1
^ permalink raw reply related
* [PATCH -next v3 13/15] hwmon: (nzxt-kraken2) Use devm_hid_hw_start_and_open in kraken2_probe()
From: Li Zetao @ 2024-09-10 15:45 UTC (permalink / raw)
To: jikos, bentiss, michael.zaidman, djogorchock, rrameshbabu,
bonbons, roderick.colenbrander, david, savicaleksa83, me,
jdelvare, linux, mail, wilken.gottwalt, jonas, mezin.alexander
Cc: lizetao1, linux-input, linux-i2c, linux-hwmon
In-Reply-To: <20240910154545.736786-1-lizetao1@huawei.com>
Currently, the nzxt-kraken2 module needs to maintain hid resources
by itself. Use devm_hid_hw_start_and_open helper to ensure that hid
resources are consistent with the device life cycle, and release
hid resources before device is released. At the same time, it can avoid
the goto-release encoding, drop the fail_and_close and fail_and_stop
lables, and directly return the error code when an error occurs.
Further optimization, use devm_hwmon_device_register_with_info to replace
hwmon_device_register_with_info, the remote operation can be completely
deleted, and the kraken2_priv_data no longer needs to hold hwmon device.
Acked-by: Jonas Malaco <jonas@protocubo.io>
Acked-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
v2 -> v3: None
v2: https://lore.kernel.org/all/20240909012313.500341-14-lizetao1@huawei.com/
v1 -> v2:
1) Further optimize using devm_hwmon_device_register_with_info
and remove the .remove operation
2) Adjust commit information
v1: https://lore.kernel.org/all/20240904123607.3407364-18-lizetao1@huawei.com/
drivers/hwmon/nzxt-kraken2.c | 45 ++++++------------------------------
1 file changed, 7 insertions(+), 38 deletions(-)
diff --git a/drivers/hwmon/nzxt-kraken2.c b/drivers/hwmon/nzxt-kraken2.c
index 7caf387eb144..5b618fc2c17f 100644
--- a/drivers/hwmon/nzxt-kraken2.c
+++ b/drivers/hwmon/nzxt-kraken2.c
@@ -29,7 +29,6 @@ static const char *const kraken2_fan_label[] = {
struct kraken2_priv_data {
struct hid_device *hid_dev;
- struct device *hwmon_dev;
s32 temp_input[1];
u16 fan_input[2];
unsigned long updated; /* jiffies */
@@ -133,6 +132,7 @@ static int kraken2_probe(struct hid_device *hdev,
const struct hid_device_id *id)
{
struct kraken2_priv_data *priv;
+ struct device *hwmon_dev;
int ret;
priv = devm_kzalloc(&hdev->dev, sizeof(*priv), GFP_KERNEL);
@@ -158,44 +158,14 @@ static int kraken2_probe(struct hid_device *hdev,
/*
* Enable hidraw so existing user-space tools can continue to work.
*/
- ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
- if (ret) {
- hid_err(hdev, "hid hw start failed with %d\n", ret);
+ ret = devm_hid_hw_start_and_open(hdev, HID_CONNECT_HIDRAW);
+ if (ret)
return ret;
- }
-
- ret = hid_hw_open(hdev);
- if (ret) {
- hid_err(hdev, "hid hw open failed with %d\n", ret);
- goto fail_and_stop;
- }
-
- priv->hwmon_dev = hwmon_device_register_with_info(&hdev->dev, "kraken2",
- priv, &kraken2_chip_info,
- NULL);
- if (IS_ERR(priv->hwmon_dev)) {
- ret = PTR_ERR(priv->hwmon_dev);
- hid_err(hdev, "hwmon registration failed with %d\n", ret);
- goto fail_and_close;
- }
-
- return 0;
-
-fail_and_close:
- hid_hw_close(hdev);
-fail_and_stop:
- hid_hw_stop(hdev);
- return ret;
-}
-
-static void kraken2_remove(struct hid_device *hdev)
-{
- struct kraken2_priv_data *priv = hid_get_drvdata(hdev);
-
- hwmon_device_unregister(priv->hwmon_dev);
- hid_hw_close(hdev);
- hid_hw_stop(hdev);
+ hwmon_dev = devm_hwmon_device_register_with_info(&hdev->dev, "kraken2",
+ priv, &kraken2_chip_info,
+ NULL);
+ return PTR_ERR_OR_ZERO(hwmon_dev);
}
static const struct hid_device_id kraken2_table[] = {
@@ -209,7 +179,6 @@ static struct hid_driver kraken2_driver = {
.name = "nzxt-kraken2",
.id_table = kraken2_table,
.probe = kraken2_probe,
- .remove = kraken2_remove,
.raw_event = kraken2_raw_event,
};
--
2.34.1
^ permalink raw reply related
* [PATCH -next v3 11/15] hwmon: (corsair-psu) Use devm_hid_hw_start_and_open in corsairpsu_probe()
From: Li Zetao @ 2024-09-10 15:45 UTC (permalink / raw)
To: jikos, bentiss, michael.zaidman, djogorchock, rrameshbabu,
bonbons, roderick.colenbrander, david, savicaleksa83, me,
jdelvare, linux, mail, wilken.gottwalt, jonas, mezin.alexander
Cc: lizetao1, linux-input, linux-i2c, linux-hwmon
In-Reply-To: <20240910154545.736786-1-lizetao1@huawei.com>
Currently, the corsair-psu module needs to maintain hid resources
by itself. Use devm_hid_hw_start_and_open helper to ensure that hid
resources are consistent with the device life cycle, and release
hid resources before device is released. At the same time, it can avoid
the goto-release encoding, drop the fail_and_close and fail_and_stop
lables, and directly return the error code when an error occurs.
Reviewed-by: Wilken Gottwalt <wilken.gottwalt@posteo.net>
Acked-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
v2 -> v3: None
v2: https://lore.kernel.org/all/20240909012313.500341-12-lizetao1@huawei.com/
v1 -> v2: Adjust commit information
v1: https://lore.kernel.org/all/20240904123607.3407364-15-lizetao1@huawei.com/
drivers/hwmon/corsair-psu.c | 24 +++++-------------------
1 file changed, 5 insertions(+), 19 deletions(-)
diff --git a/drivers/hwmon/corsair-psu.c b/drivers/hwmon/corsair-psu.c
index f8f22b8a67cd..b574ec9cd00f 100644
--- a/drivers/hwmon/corsair-psu.c
+++ b/drivers/hwmon/corsair-psu.c
@@ -787,14 +787,10 @@ static int corsairpsu_probe(struct hid_device *hdev, const struct hid_device_id
if (ret)
return ret;
- ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
+ ret = devm_hid_hw_start_and_open(hdev, HID_CONNECT_HIDRAW);
if (ret)
return ret;
- ret = hid_hw_open(hdev);
- if (ret)
- goto fail_and_stop;
-
priv->hdev = hdev;
hid_set_drvdata(hdev, priv);
mutex_init(&priv->lock);
@@ -805,13 +801,13 @@ static int corsairpsu_probe(struct hid_device *hdev, const struct hid_device_id
ret = corsairpsu_init(priv);
if (ret < 0) {
dev_err(&hdev->dev, "unable to initialize device (%d)\n", ret);
- goto fail_and_stop;
+ return ret;
}
ret = corsairpsu_fwinfo(priv);
if (ret < 0) {
dev_err(&hdev->dev, "unable to query firmware (%d)\n", ret);
- goto fail_and_stop;
+ return ret;
}
corsairpsu_get_criticals(priv);
@@ -820,20 +816,12 @@ static int corsairpsu_probe(struct hid_device *hdev, const struct hid_device_id
priv->hwmon_dev = hwmon_device_register_with_info(&hdev->dev, "corsairpsu", priv,
&corsairpsu_chip_info, NULL);
- if (IS_ERR(priv->hwmon_dev)) {
- ret = PTR_ERR(priv->hwmon_dev);
- goto fail_and_close;
- }
+ if (IS_ERR(priv->hwmon_dev))
+ return PTR_ERR(priv->hwmon_dev);
corsairpsu_debugfs_init(priv);
return 0;
-
-fail_and_close:
- hid_hw_close(hdev);
-fail_and_stop:
- hid_hw_stop(hdev);
- return ret;
}
static void corsairpsu_remove(struct hid_device *hdev)
@@ -842,8 +830,6 @@ static void corsairpsu_remove(struct hid_device *hdev)
debugfs_remove_recursive(priv->debugfs);
hwmon_device_unregister(priv->hwmon_dev);
- hid_hw_close(hdev);
- hid_hw_stop(hdev);
}
static int corsairpsu_raw_event(struct hid_device *hdev, struct hid_report *report, u8 *data,
--
2.34.1
^ permalink raw reply related
* [PATCH -next v3 14/15] hwmon: (nzxt-kraken3) Use devm_hid_hw_start_and_open in kraken3_probe()
From: Li Zetao @ 2024-09-10 15:45 UTC (permalink / raw)
To: jikos, bentiss, michael.zaidman, djogorchock, rrameshbabu,
bonbons, roderick.colenbrander, david, savicaleksa83, me,
jdelvare, linux, mail, wilken.gottwalt, jonas, mezin.alexander
Cc: lizetao1, linux-input, linux-i2c, linux-hwmon
In-Reply-To: <20240910154545.736786-1-lizetao1@huawei.com>
Currently, the nzxt-kraken3 module needs to maintain hid resources
by itself. Use devm_hid_hw_start_and_open helper to ensure that hid
resources are consistent with the device life cycle, and release
hid resources before device is released. At the same time, it can avoid
the goto-release encoding, drop the fail_and_close and fail_and_stop
lables, and directly return the error code when an error occurs.
Acked-by: Guenter Roeck <linux@roeck-us.net>
Reviewed-by: Aleksa Savic <savicaleksa83@gmail.com>
Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
v2 -> v3: Change "nzxt-kraken2" to "nzxt-kraken3" in the commit message.
v2: https://lore.kernel.org/all/20240909012313.500341-15-lizetao1@huawei.com/
v1 -> v2: Adjust commit information
v1: https://lore.kernel.org/all/20240904123607.3407364-19-lizetao1@huawei.com/
drivers/hwmon/nzxt-kraken3.c | 34 +++++++---------------------------
1 file changed, 7 insertions(+), 27 deletions(-)
diff --git a/drivers/hwmon/nzxt-kraken3.c b/drivers/hwmon/nzxt-kraken3.c
index 00f3ac90a290..71b8c21cfe1b 100644
--- a/drivers/hwmon/nzxt-kraken3.c
+++ b/drivers/hwmon/nzxt-kraken3.c
@@ -897,17 +897,9 @@ static int kraken3_probe(struct hid_device *hdev, const struct hid_device_id *id
}
/* Enable hidraw so existing user-space tools can continue to work */
- ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
- if (ret) {
- hid_err(hdev, "hid hw start failed with %d\n", ret);
+ ret = devm_hid_hw_start_and_open(hdev, HID_CONNECT_HIDRAW);
+ if (ret)
return ret;
- }
-
- ret = hid_hw_open(hdev);
- if (ret) {
- hid_err(hdev, "hid hw open failed with %d\n", ret);
- goto fail_and_stop;
- }
switch (hdev->product) {
case USB_PRODUCT_ID_X53:
@@ -928,15 +920,12 @@ static int kraken3_probe(struct hid_device *hdev, const struct hid_device_id *id
device_name = "kraken2023elite";
break;
default:
- ret = -ENODEV;
- goto fail_and_close;
+ return -ENODEV;
}
priv->buffer = devm_kzalloc(&hdev->dev, MAX_REPORT_LENGTH, GFP_KERNEL);
- if (!priv->buffer) {
- ret = -ENOMEM;
- goto fail_and_close;
- }
+ if (!priv->buffer)
+ return -ENOMEM;
mutex_init(&priv->buffer_lock);
mutex_init(&priv->z53_status_request_lock);
@@ -948,7 +937,7 @@ static int kraken3_probe(struct hid_device *hdev, const struct hid_device_id *id
ret = kraken3_init_device(hdev);
if (ret < 0) {
hid_err(hdev, "device init failed with %d\n", ret);
- goto fail_and_close;
+ return ret;
}
ret = kraken3_get_fw_ver(hdev);
@@ -960,18 +949,12 @@ static int kraken3_probe(struct hid_device *hdev, const struct hid_device_id *id
if (IS_ERR(priv->hwmon_dev)) {
ret = PTR_ERR(priv->hwmon_dev);
hid_err(hdev, "hwmon registration failed with %d\n", ret);
- goto fail_and_close;
+ return ret;
}
kraken3_debugfs_init(priv, device_name);
return 0;
-
-fail_and_close:
- hid_hw_close(hdev);
-fail_and_stop:
- hid_hw_stop(hdev);
- return ret;
}
static void kraken3_remove(struct hid_device *hdev)
@@ -980,9 +963,6 @@ static void kraken3_remove(struct hid_device *hdev)
debugfs_remove_recursive(priv->debugfs);
hwmon_device_unregister(priv->hwmon_dev);
-
- hid_hw_close(hdev);
- hid_hw_stop(hdev);
}
static const struct hid_device_id kraken3_table[] = {
--
2.34.1
^ permalink raw reply related
* [PATCH -next v3 15/15] hwmon: (nzxt-smart2) Use devm_hid_hw_start_and_open in nzxt_smart2_hid_probe()
From: Li Zetao @ 2024-09-10 15:45 UTC (permalink / raw)
To: jikos, bentiss, michael.zaidman, djogorchock, rrameshbabu,
bonbons, roderick.colenbrander, david, savicaleksa83, me,
jdelvare, linux, mail, wilken.gottwalt, jonas, mezin.alexander
Cc: lizetao1, linux-input, linux-i2c, linux-hwmon
In-Reply-To: <20240910154545.736786-1-lizetao1@huawei.com>
Currently, the nzxt-smart2 module needs to maintain hid resources
by itself. Use devm_hid_hw_start_and_open helper to ensure that hid
resources are consistent with the device life cycle, and release
hid resources before device is released. At the same time, it can avoid
the goto-release encoding, drop the out_hw_close and out_hw_stop
lables, and directly return the error code when an error occurs.
Further optimization, use devm_hwmon_device_register_with_info to replace
hwmon_device_register_with_info, the remote operation can be completely
deleted, and the drvdata no longer needs to hold hwmon device
Acked-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
v2 -> v3: None
v2: https://lore.kernel.org/all/20240909012313.500341-16-lizetao1@huawei.com/
v1 -> v2:
1) Further optimize using devm_hwmon_device_register_with_info
and remove the .remove operation
2) Adjust commit information
v1: https://lore.kernel.org/all/20240904123607.3407364-20-lizetao1@huawei.com/
drivers/hwmon/nzxt-smart2.c | 38 +++++--------------------------------
1 file changed, 5 insertions(+), 33 deletions(-)
diff --git a/drivers/hwmon/nzxt-smart2.c b/drivers/hwmon/nzxt-smart2.c
index df6fa72a6b59..9c6d020ac896 100644
--- a/drivers/hwmon/nzxt-smart2.c
+++ b/drivers/hwmon/nzxt-smart2.c
@@ -172,7 +172,6 @@ struct set_fan_speed_report {
struct drvdata {
struct hid_device *hid;
- struct device *hwmon;
u8 fan_duty_percent[FAN_CHANNELS];
u16 fan_rpm[FAN_CHANNELS];
@@ -730,6 +729,7 @@ static int nzxt_smart2_hid_probe(struct hid_device *hdev,
const struct hid_device_id *id)
{
struct drvdata *drvdata;
+ struct device *hwmon;
int ret;
drvdata = devm_kzalloc(&hdev->dev, sizeof(struct drvdata), GFP_KERNEL);
@@ -750,44 +750,17 @@ static int nzxt_smart2_hid_probe(struct hid_device *hdev,
if (ret)
return ret;
- ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
+ ret = devm_hid_hw_start_and_open(hdev, HID_CONNECT_HIDRAW);
if (ret)
return ret;
- ret = hid_hw_open(hdev);
- if (ret)
- goto out_hw_stop;
-
hid_device_io_start(hdev);
init_device(drvdata, UPDATE_INTERVAL_DEFAULT_MS);
- drvdata->hwmon =
- hwmon_device_register_with_info(&hdev->dev, "nzxtsmart2", drvdata,
- &nzxt_smart2_chip_info, NULL);
- if (IS_ERR(drvdata->hwmon)) {
- ret = PTR_ERR(drvdata->hwmon);
- goto out_hw_close;
- }
-
- return 0;
-
-out_hw_close:
- hid_hw_close(hdev);
-
-out_hw_stop:
- hid_hw_stop(hdev);
- return ret;
-}
-
-static void nzxt_smart2_hid_remove(struct hid_device *hdev)
-{
- struct drvdata *drvdata = hid_get_drvdata(hdev);
-
- hwmon_device_unregister(drvdata->hwmon);
-
- hid_hw_close(hdev);
- hid_hw_stop(hdev);
+ hwmon = devm_hwmon_device_register_with_info(&hdev->dev, "nzxtsmart2", drvdata,
+ &nzxt_smart2_chip_info, NULL);
+ return PTR_ERR_OR_ZERO(hwmon);
}
static const struct hid_device_id nzxt_smart2_hid_id_table[] = {
@@ -807,7 +780,6 @@ static struct hid_driver nzxt_smart2_hid_driver = {
.name = "nzxt-smart2",
.id_table = nzxt_smart2_hid_id_table,
.probe = nzxt_smart2_hid_probe,
- .remove = nzxt_smart2_hid_remove,
.raw_event = nzxt_smart2_hid_raw_event,
#ifdef CONFIG_PM
.reset_resume = nzxt_smart2_hid_reset_resume,
--
2.34.1
^ permalink raw reply related
* [PATCH -next v3 09/15] hwmon: (asus_rog_ryujin) Use devm_hid_hw_start_and_open in rog_ryujin_probe()
From: Li Zetao @ 2024-09-10 15:45 UTC (permalink / raw)
To: jikos, bentiss, michael.zaidman, djogorchock, rrameshbabu,
bonbons, roderick.colenbrander, david, savicaleksa83, me,
jdelvare, linux, mail, wilken.gottwalt, jonas, mezin.alexander
Cc: lizetao1, linux-input, linux-i2c, linux-hwmon
In-Reply-To: <20240910154545.736786-1-lizetao1@huawei.com>
Currently, the rog_ryujin module needs to maintain hid resources
by itself. Use devm_hid_hw_start_and_open helper to ensure that hid
resources are consistent with the device life cycle, and release
hid resources before device is released. At the same time, it can avoid
the goto-release encoding, drop the fail_and_close and fail_and_stop
lables, and directly return the error code when an error occurs.
Further optimization, use devm_hwmon_device_register_with_info to replace
hwmon_device_register_with_info, the remote operation can be completely
deleted, and the rog_ryujin_data structure no longer needs to hold
hwmon device.
Acked-by: Guenter Roeck <linux@roeck-us.net>
Reviewed-by: Aleksa Savic <savicaleksa83@gmail.com>
Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
v2 -> v3: Added asus_rog_ryujin to prefix subject.
v2: https://lore.kernel.org/all/580835b6-928e-4cef-b083-1b48caa1c046@roeck-us.net/
v1 -> v2:
1) Further optimize using devm_hwmon_device_register_with_info
and remove the .remove operation
2) Adjust commit information
v1: https://lore.kernel.org/all/20240904123607.3407364-14-lizetao1@huawei.com/
drivers/hwmon/asus_rog_ryujin.c | 47 +++++----------------------------
1 file changed, 7 insertions(+), 40 deletions(-)
diff --git a/drivers/hwmon/asus_rog_ryujin.c b/drivers/hwmon/asus_rog_ryujin.c
index f8b20346a995..ef0d9b72a824 100644
--- a/drivers/hwmon/asus_rog_ryujin.c
+++ b/drivers/hwmon/asus_rog_ryujin.c
@@ -80,7 +80,6 @@ static const char *const rog_ryujin_speed_label[] = {
struct rog_ryujin_data {
struct hid_device *hdev;
- struct device *hwmon_dev;
/* For locking access to buffer */
struct mutex buffer_lock;
/* For queueing multiple readers */
@@ -497,6 +496,7 @@ static int rog_ryujin_raw_event(struct hid_device *hdev, struct hid_report *repo
static int rog_ryujin_probe(struct hid_device *hdev, const struct hid_device_id *id)
{
struct rog_ryujin_data *priv;
+ struct device *hwmon_dev;
int ret;
priv = devm_kzalloc(&hdev->dev, sizeof(*priv), GFP_KERNEL);
@@ -520,23 +520,13 @@ static int rog_ryujin_probe(struct hid_device *hdev, const struct hid_device_id
}
/* Enable hidraw so existing user-space tools can continue to work */
- ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
- if (ret) {
- hid_err(hdev, "hid hw start failed with %d\n", ret);
+ ret = devm_hid_hw_start_and_open(hdev, HID_CONNECT_HIDRAW);
+ if (ret)
return ret;
- }
-
- ret = hid_hw_open(hdev);
- if (ret) {
- hid_err(hdev, "hid hw open failed with %d\n", ret);
- goto fail_and_stop;
- }
priv->buffer = devm_kzalloc(&hdev->dev, MAX_REPORT_LENGTH, GFP_KERNEL);
- if (!priv->buffer) {
- ret = -ENOMEM;
- goto fail_and_close;
- }
+ if (!priv->buffer)
+ return -ENOMEM;
mutex_init(&priv->status_report_request_mutex);
mutex_init(&priv->buffer_lock);
@@ -548,31 +538,9 @@ static int rog_ryujin_probe(struct hid_device *hdev, const struct hid_device_id
init_completion(&priv->cooler_duty_set);
init_completion(&priv->controller_duty_set);
- priv->hwmon_dev = hwmon_device_register_with_info(&hdev->dev, "rog_ryujin",
+ hwmon_dev = devm_hwmon_device_register_with_info(&hdev->dev, "rog_ryujin",
priv, &rog_ryujin_chip_info, NULL);
- if (IS_ERR(priv->hwmon_dev)) {
- ret = PTR_ERR(priv->hwmon_dev);
- hid_err(hdev, "hwmon registration failed with %d\n", ret);
- goto fail_and_close;
- }
-
- return 0;
-
-fail_and_close:
- hid_hw_close(hdev);
-fail_and_stop:
- hid_hw_stop(hdev);
- return ret;
-}
-
-static void rog_ryujin_remove(struct hid_device *hdev)
-{
- struct rog_ryujin_data *priv = hid_get_drvdata(hdev);
-
- hwmon_device_unregister(priv->hwmon_dev);
-
- hid_hw_close(hdev);
- hid_hw_stop(hdev);
+ return PTR_ERR_OR_ZERO(hwmon_dev);
}
static const struct hid_device_id rog_ryujin_table[] = {
@@ -586,7 +554,6 @@ static struct hid_driver rog_ryujin_driver = {
.name = "rog_ryujin",
.id_table = rog_ryujin_table,
.probe = rog_ryujin_probe,
- .remove = rog_ryujin_remove,
.raw_event = rog_ryujin_raw_event,
};
--
2.34.1
^ permalink raw reply related
* [PATCH -next v3 10/15] hwmon: (corsair-cpro) Use devm_hid_hw_start_and_open in ccp_probe()
From: Li Zetao @ 2024-09-10 15:45 UTC (permalink / raw)
To: jikos, bentiss, michael.zaidman, djogorchock, rrameshbabu,
bonbons, roderick.colenbrander, david, savicaleksa83, me,
jdelvare, linux, mail, wilken.gottwalt, jonas, mezin.alexander
Cc: lizetao1, linux-input, linux-i2c, linux-hwmon
In-Reply-To: <20240910154545.736786-1-lizetao1@huawei.com>
Currently, the corsair-cpro module needs to maintain hid resources
by itself. Use devm_hid_hw_start_and_open helper to ensure that hid
resources are consistent with the device life cycle, and release
hid resources before device is released. At the same time, it can avoid
the goto-release encoding, drop the out_hw_close and hid_hw_stop
lables, and directly return the error code when an error occurs.
Acked-by: Marius Zachmann <mail@mariuszachmann.de>
Acked-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
v2 -> v3: None
v2: https://lore.kernel.org/all/20240909012313.500341-11-lizetao1@huawei.com/
v1 -> v2: Adjust commit information
v1: https://lore.kernel.org/all/20240904123607.3407364-15-lizetao1@huawei.com/
drivers/hwmon/corsair-cpro.c | 24 +++++-------------------
1 file changed, 5 insertions(+), 19 deletions(-)
diff --git a/drivers/hwmon/corsair-cpro.c b/drivers/hwmon/corsair-cpro.c
index e1a7f7aa7f80..7bba30840f32 100644
--- a/drivers/hwmon/corsair-cpro.c
+++ b/drivers/hwmon/corsair-cpro.c
@@ -601,14 +601,10 @@ static int ccp_probe(struct hid_device *hdev, const struct hid_device_id *id)
if (ret)
return ret;
- ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
+ ret = devm_hid_hw_start_and_open(hdev, HID_CONNECT_HIDRAW);
if (ret)
return ret;
- ret = hid_hw_open(hdev);
- if (ret)
- goto out_hw_stop;
-
ccp->hdev = hdev;
hid_set_drvdata(hdev, ccp);
@@ -621,28 +617,20 @@ static int ccp_probe(struct hid_device *hdev, const struct hid_device_id *id)
/* temp and fan connection status only updates when device is powered on */
ret = get_temp_cnct(ccp);
if (ret)
- goto out_hw_close;
+ return ret;
ret = get_fan_cnct(ccp);
if (ret)
- goto out_hw_close;
+ return ret;
ccp_debugfs_init(ccp);
ccp->hwmon_dev = hwmon_device_register_with_info(&hdev->dev, "corsaircpro",
ccp, &ccp_chip_info, NULL);
- if (IS_ERR(ccp->hwmon_dev)) {
- ret = PTR_ERR(ccp->hwmon_dev);
- goto out_hw_close;
- }
+ if (IS_ERR(ccp->hwmon_dev))
+ return PTR_ERR(ccp->hwmon_dev);
return 0;
-
-out_hw_close:
- hid_hw_close(hdev);
-out_hw_stop:
- hid_hw_stop(hdev);
- return ret;
}
static void ccp_remove(struct hid_device *hdev)
@@ -651,8 +639,6 @@ static void ccp_remove(struct hid_device *hdev)
debugfs_remove_recursive(ccp->debugfs);
hwmon_device_unregister(ccp->hwmon_dev);
- hid_hw_close(hdev);
- hid_hw_stop(hdev);
}
static const struct hid_device_id ccp_devices[] = {
--
2.34.1
^ permalink raw reply related
* [PATCH -next v3 08/15] hwmon: (aquacomputer_d5next) Use devm_hid_hw_start_and_open in aqc_probe()
From: Li Zetao @ 2024-09-10 15:45 UTC (permalink / raw)
To: jikos, bentiss, michael.zaidman, djogorchock, rrameshbabu,
bonbons, roderick.colenbrander, david, savicaleksa83, me,
jdelvare, linux, mail, wilken.gottwalt, jonas, mezin.alexander
Cc: lizetao1, linux-input, linux-i2c, linux-hwmon
In-Reply-To: <20240910154545.736786-1-lizetao1@huawei.com>
Currently, the aquacomputer_d5next module needs to maintain hid resources
by itself. Use devm_hid_hw_start_and_open helper to ensure that hid
resources are consistent with the device life cycle, and release
hid resources before device is released. At the same time, it can avoid
the goto-release encoding, drop the fail_and_close and fail_and_stop
lables, and directly return the error code when an error occurs.
Reviewed-by: Aleksa Savic <savicaleksa83@gmail.com>
Acked-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
v2 -> v3: None
v2: https://lore.kernel.org/all/20240909012313.500341-9-lizetao1@huawei.com/
v1 -> v2: Adjust commit information
v1: https://lore.kernel.org/all/20240904123607.3407364-13-lizetao1@huawei.com/
drivers/hwmon/aquacomputer_d5next.c | 39 +++++++----------------------
1 file changed, 9 insertions(+), 30 deletions(-)
diff --git a/drivers/hwmon/aquacomputer_d5next.c b/drivers/hwmon/aquacomputer_d5next.c
index 8e55cd2f46f5..9b66ff0fe6e1 100644
--- a/drivers/hwmon/aquacomputer_d5next.c
+++ b/drivers/hwmon/aquacomputer_d5next.c
@@ -1556,14 +1556,10 @@ static int aqc_probe(struct hid_device *hdev, const struct hid_device_id *id)
if (ret)
return ret;
- ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
+ ret = devm_hid_hw_start_and_open(hdev, HID_CONNECT_HIDRAW);
if (ret)
return ret;
- ret = hid_hw_open(hdev);
- if (ret)
- goto fail_and_stop;
-
switch (hdev->product) {
case USB_PRODUCT_ID_AQUAERO:
/*
@@ -1577,10 +1573,8 @@ static int aqc_probe(struct hid_device *hdev, const struct hid_device_id *id)
* they present. The two other devices have the type of the second element in
* their respective collections set to 1, while the real device has it set to 0.
*/
- if (hdev->collection[1].type != 0) {
- ret = -ENODEV;
- goto fail_and_close;
- }
+ if (hdev->collection[1].type != 0)
+ return -ENODEV;
priv->kind = aquaero;
@@ -1740,10 +1734,8 @@ static int aqc_probe(struct hid_device *hdev, const struct hid_device_id *id)
* Choose the right Leakshield device, because
* the other one acts as a keyboard
*/
- if (hdev->type != 2) {
- ret = -ENODEV;
- goto fail_and_close;
- }
+ if (hdev->type != 2)
+ return -ENODEV;
priv->kind = leakshield;
@@ -1865,30 +1857,20 @@ static int aqc_probe(struct hid_device *hdev, const struct hid_device_id *id)
priv->name = aqc_device_names[priv->kind];
priv->buffer = devm_kzalloc(&hdev->dev, priv->buffer_size, GFP_KERNEL);
- if (!priv->buffer) {
- ret = -ENOMEM;
- goto fail_and_close;
- }
+ if (!priv->buffer)
+ return -ENOMEM;
mutex_init(&priv->mutex);
priv->hwmon_dev = hwmon_device_register_with_info(&hdev->dev, priv->name, priv,
&aqc_chip_info, NULL);
- if (IS_ERR(priv->hwmon_dev)) {
- ret = PTR_ERR(priv->hwmon_dev);
- goto fail_and_close;
- }
+ if (IS_ERR(priv->hwmon_dev))
+ return PTR_ERR(priv->hwmon_dev);
aqc_debugfs_init(priv);
return 0;
-
-fail_and_close:
- hid_hw_close(hdev);
-fail_and_stop:
- hid_hw_stop(hdev);
- return ret;
}
static void aqc_remove(struct hid_device *hdev)
@@ -1897,9 +1879,6 @@ static void aqc_remove(struct hid_device *hdev)
debugfs_remove_recursive(priv->debugfs);
hwmon_device_unregister(priv->hwmon_dev);
-
- hid_hw_close(hdev);
- hid_hw_stop(hdev);
}
static const struct hid_device_id aqc_table[] = {
--
2.34.1
^ permalink raw reply related
* [PATCH -next v3 06/15] HID: nintendo: Use devm_hid_hw_start_and_open in nintendo_hid_probe()
From: Li Zetao @ 2024-09-10 15:45 UTC (permalink / raw)
To: jikos, bentiss, michael.zaidman, djogorchock, rrameshbabu,
bonbons, roderick.colenbrander, david, savicaleksa83, me,
jdelvare, linux, mail, wilken.gottwalt, jonas, mezin.alexander
Cc: lizetao1, linux-input, linux-i2c, linux-hwmon
In-Reply-To: <20240910154545.736786-1-lizetao1@huawei.com>
Currently, the nintendo module needs to maintain hid resources
by itself. Use devm_hid_hw_start_and_open helper to ensure that hid
resources are consistent with the device life cycle, and release
hid resources before device is released. At the same time, it can avoid
the goto-release encoding, drop the err_close and err_stop lables.
Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
v2 -> v3: None
v2: https://lore.kernel.org/all/20240909012313.500341-7-lizetao1@huawei.com/
v1 -> v2: Adjust commit information
v1: https://lore.kernel.org/all/20240904123607.3407364-7-lizetao1@huawei.com/
drivers/hid/hid-nintendo.c | 23 ++++-------------------
1 file changed, 4 insertions(+), 19 deletions(-)
diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
index 58cd0506e431..45ac4fd3c7ea 100644
--- a/drivers/hid/hid-nintendo.c
+++ b/drivers/hid/hid-nintendo.c
@@ -2673,31 +2673,23 @@ static int nintendo_hid_probe(struct hid_device *hdev,
*/
hdev->version |= 0x8000;
- ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
- if (ret) {
- hid_err(hdev, "HW start failed\n");
+ ret = devm_hid_hw_start_and_open(hdev, HID_CONNECT_HIDRAW);
+ if (ret)
goto err_wq;
- }
-
- ret = hid_hw_open(hdev);
- if (ret) {
- hid_err(hdev, "cannot start hardware I/O\n");
- goto err_stop;
- }
hid_device_io_start(hdev);
ret = joycon_init(hdev);
if (ret) {
hid_err(hdev, "Failed to initialize controller; ret=%d\n", ret);
- goto err_close;
+ goto err_wq;
}
/* Initialize the leds */
ret = joycon_leds_create(ctlr);
if (ret) {
hid_err(hdev, "Failed to create leds; ret=%d\n", ret);
- goto err_close;
+ goto err_wq;
}
/* Initialize the battery power supply */
@@ -2720,10 +2712,6 @@ static int nintendo_hid_probe(struct hid_device *hdev,
err_ida:
ida_free(&nintendo_player_id_allocator, ctlr->player_id);
-err_close:
- hid_hw_close(hdev);
-err_stop:
- hid_hw_stop(hdev);
err_wq:
destroy_workqueue(ctlr->rumble_queue);
err:
@@ -2745,9 +2733,6 @@ static void nintendo_hid_remove(struct hid_device *hdev)
destroy_workqueue(ctlr->rumble_queue);
ida_free(&nintendo_player_id_allocator, ctlr->player_id);
-
- hid_hw_close(hdev);
- hid_hw_stop(hdev);
}
#ifdef CONFIG_PM
--
2.34.1
^ permalink raw reply related
* [PATCH -next v3 07/15] HID: playstation: Use devm_hid_hw_start_and_open in ps_probe()
From: Li Zetao @ 2024-09-10 15:45 UTC (permalink / raw)
To: jikos, bentiss, michael.zaidman, djogorchock, rrameshbabu,
bonbons, roderick.colenbrander, david, savicaleksa83, me,
jdelvare, linux, mail, wilken.gottwalt, jonas, mezin.alexander
Cc: lizetao1, linux-input, linux-i2c, linux-hwmon
In-Reply-To: <20240910154545.736786-1-lizetao1@huawei.com>
Currently, the playstation module needs to maintain hid resources
by itself. Use devm_hid_hw_start_and_open helper to ensure that hid
resources are consistent with the device life cycle, and release
hid resources before device is released. At the same time, it can avoid
the goto-release encoding, drop the err_close and err_stop lables, and
directly return the error code when an error occurs.
Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
v2 -> v3: None
v2: https://lore.kernel.org/all/20240909012313.500341-8-lizetao1@huawei.com/
v1 -> v2: Adjust commit information
v1: https://lore.kernel.org/all/20240904123607.3407364-10-lizetao1@huawei.com/
drivers/hid/hid-playstation.c | 27 ++++-----------------------
1 file changed, 4 insertions(+), 23 deletions(-)
diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
index 0d90d7ee693c..6dddb9451a37 100644
--- a/drivers/hid/hid-playstation.c
+++ b/drivers/hid/hid-playstation.c
@@ -2704,41 +2704,25 @@ static int ps_probe(struct hid_device *hdev, const struct hid_device_id *id)
return ret;
}
- ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
- if (ret) {
- hid_err(hdev, "Failed to start HID device\n");
+ ret = devm_hid_hw_start_and_open(hdev, HID_CONNECT_HIDRAW);
+ if (ret)
return ret;
- }
-
- ret = hid_hw_open(hdev);
- if (ret) {
- hid_err(hdev, "Failed to open HID device\n");
- goto err_stop;
- }
if (id->driver_data == PS_TYPE_PS4_DUALSHOCK4) {
dev = dualshock4_create(hdev);
if (IS_ERR(dev)) {
hid_err(hdev, "Failed to create dualshock4.\n");
- ret = PTR_ERR(dev);
- goto err_close;
+ return PTR_ERR(dev);
}
} else if (id->driver_data == PS_TYPE_PS5_DUALSENSE) {
dev = dualsense_create(hdev);
if (IS_ERR(dev)) {
hid_err(hdev, "Failed to create dualsense.\n");
- ret = PTR_ERR(dev);
- goto err_close;
+ return PTR_ERR(dev);
}
}
return ret;
-
-err_close:
- hid_hw_close(hdev);
-err_stop:
- hid_hw_stop(hdev);
- return ret;
}
static void ps_remove(struct hid_device *hdev)
@@ -2750,9 +2734,6 @@ static void ps_remove(struct hid_device *hdev)
if (dev->remove)
dev->remove(dev);
-
- hid_hw_close(hdev);
- hid_hw_stop(hdev);
}
static const struct hid_device_id ps_devices[] = {
--
2.34.1
^ 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