* Re: [PATCH v4 1/4] HID: add driver for Valve Steam Controller
From: Rodrigo Rivas Costa @ 2018-03-01 19:13 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Jiri Kosina, Benjamin Tissoires, Pierre-Loup A. Griffais,
Cameron Gutman, Clément VUCHENER, Linux Kernel Mailing List,
linux-input
In-Reply-To: <CAHp75Vd9wgX8S=4hGFERN5bEN9q4=DxYcDea9Z=mvSRi2GcxeQ@mail.gmail.com>
On Thu, Mar 01, 2018 at 12:20:37PM +0200, Andy Shevchenko wrote:
> On Thu, Mar 1, 2018 at 12:49 AM, Rodrigo Rivas Costa
> <rodrigorivascosta@gmail.com> wrote:
> > On Wed, Feb 28, 2018 at 09:21:15PM +0200, Andy Shevchenko wrote:
> >> On Wed, Feb 28, 2018 at 8:43 PM, Rodrigo Rivas Costa
> >> <rodrigorivascosta@gmail.com> wrote:
>
> >> > + if (input) {
> >>
> >> if (!input)
> >> return;
> >>
> >> ?
> > That was because of symmetry, because further patches add more objects.
> > Then
> > if (input)
> > free(input);
> > if (battery)
> > free(battery);
> > /* in the future *(
> > if (input_gyro)
> > free(input_gyro);
> >
> > Sure, the last one can do an early return, but you break symmetry.
>
> The generic APIs when designed properly are NULL aware at freeing resources.
>
> So, it should look like
>
> free(input);
> free(battery);
> free(input_gyro);
My bad, I didn't mean a literal free() call. More like:
static void steam_unregister(struct steam_device *steam)
{
struct input_dev *input;
struct power_supply *battery;
rcu_read_lock();
input = rcu_dereference(steam->input);
battery = rcu_dereference(steam->battery);
input_gyro = rcu_dereference(steam->input_gyro);
rcu_read_unlock();
if (input) {
RCU_INIT_POINTER(steam->input, NULL);
synchronize_rcu();
hid_info(steam->hdev, "Steam Controller '%s' disconnected",
steam->serial_no);
input_unregister_device(input);
}
if (battery) {
RCU_INIT_POINTER(steam->battery, NULL);
synchronize_rcu();
power_supply_unregister(battery);
}
if (input_gyro) {
RCU_INIT_POINTER(steam->input_gyro, NULL);
synchronize_rcu();
input_unregister_device(input_gyro);
}
}
Also I think input_unregister_device() does not admit a NULL pointer.
Having a special `if (!input_gyro)return;` at the end looks just
wrong to me, it is no different from the other ones.
>
> >> > + if (steam) {
> >> > + cancel_work_sync(&steam->work_connect);
> >> > + steam_unregister(steam);
> >>
> >> > + hid_set_drvdata(hdev, NULL);
> >>
> >> Hmm.. Doesn't HID do this?
> >
> > Do you mean the hid_set_drvdata(hdev, NULL)? I'm not sure, I didn't see
> > that on hid-core.c or hid-generic.c. And a call like this is done in
> > many modules... so I did the same, just to be sure.
>
> What you are looking for is, AFAIU:
>
> drivers/base/dd.c:902: dev_set_drvdata(dev, NULL);
Ah, yes. I usually feel more comfortable setting the pointer to NULL
just after freeing the object. But currently I'm not freeing the steam
anymore, it is devm'ed, and the actual free happens from dd.c:900.
You convinced me, I'll remove that line.
>
> >>
> >> > + }
> >>
> >> if (steam) {
> >> ...
> >> hid_hw_stop(hdev);
> >> ...
> >> } else {
> >> hid_hw_stop(hdev);
> >> }
> >>
> >> ?
> >
> > I have no real preference. Your version has two 'if', mine has two 'if'.
> > What about:
> >
> > static void steam_remove(struct hid_device *hdev)
> > {
> > struct steam_device *steam = hid_get_drvdata(hdev);
> >
> > if (!steam) {
> > hid_hw_stop(hdev);
> > return;
> > }
> >
> > if (steam->quirks & STEAM_QUIRK_WIRELESS) {
> > hid_info(hdev, "Steam wireless receiver disconnected");
> > hid_hw_close(hdev);
> > }
> > hid_hw_stop(hdev);
> > cancel_work_sync(&steam->work_connect);
> > steam_unregister(steam);
>
> > hid_set_drvdata(hdev, NULL);
>
> w/o this one.
>
> > }
>
> For me my version looks more compact to read, but at the end it's up to you.
> Another option split if (steam) variant into helper, and thus
>
> if (steam)
> steam_non_null_device_remove();
> else
> hid_hw_stop();
>
> But again, up to you (that's why question mark in the first place above).
Well, I don't like code such as:
if (cond) {
/* a lot of code */
} else {
/* one line */
}
because at the time I get to the else I don't remember what the
condition was about.
Regards.
Rodrigo
> --
> With Best Regards,
> Andy Shevchenko
^ permalink raw reply
* [PATCH 2/2] HID: elecom: add support for EX-G M-XT4DRBK trackball
From: Tomasz Kramkowski @ 2018-03-01 17:22 UTC (permalink / raw)
To: Jiri Kosina
Cc: Benjamin Tissoires, linux-input, linux-kernel, Tomasz Kramkowski,
Mark Morgan Lloyd
In-Reply-To: <20180301172224.1276-1-tk@the-tk.com>
This patch enables the 6th button on the ELECOM EX-G M-XT4DRBK
trackball mouse.
This is a left handed EX-G variant which only comes in a wireless (D)
model. It has a total of 6 buttons but one of these is not available
because of how the HID descriptor is configured.
Signed-off-by: Tomasz Kramkowski <tk@the-tk.com>
---
This patch depends on patch 1 in this series.
This was requested on the list [1] and on reddit [2].
There's no "Requested-by:" field but the requesters might qualify for
a "Suggested-by:". I've notified both in case they qualify and would
like their names to be included.
I do not have the hardware to test this patch but I have been informed
that an equivalent change was tested and worked. I have asked both
requesters for their help testing the patch which means that they
might be able to vouch for its functionality. However, it's unlikely
for there to be a problem.
[1]: https://marc.info/?l=linux-input&m=151915950727292
[2]: https://www.reddit.com/r/Trackballs/comments/7ut6ht/
---
drivers/hid/hid-elecom.c | 4 +++-
drivers/hid/hid-ids.h | 1 +
drivers/hid/hid-quirks.c | 1 +
3 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/hid-elecom.c b/drivers/hid/hid-elecom.c
index 21e15dc91aac..ae8e9413c79d 100644
--- a/drivers/hid/hid-elecom.c
+++ b/drivers/hid/hid-elecom.c
@@ -1,7 +1,7 @@
/*
* HID driver for ELECOM devices:
* - BM084 Bluetooth Mouse
- * - EX-G Trackballs (M-XT3DRBK, M-XT3URBK)
+ * - EX-G Trackballs (M-XT3DRBK, M-XT3URBK, M-XT4DRBK)
* - DEFT Trackballs (M-DT1DRBK, M-DT1URBK, M-DT2DRBK, M-DT2URBK)
* - HUGE Trackballs (M-HT1DRBK, M-HT1URBK)
*
@@ -67,6 +67,7 @@ static __u8 *elecom_report_fixup(struct hid_device *hdev, __u8 *rdesc,
break;
case USB_DEVICE_ID_ELECOM_M_XT3URBK:
case USB_DEVICE_ID_ELECOM_M_XT3DRBK:
+ case USB_DEVICE_ID_ELECOM_M_XT4DRBK:
mouse_button_fixup(hdev, rdesc, *rsize, 6);
break;
case USB_DEVICE_ID_ELECOM_M_DT1URBK:
@@ -83,6 +84,7 @@ static const struct hid_device_id elecom_devices[] = {
{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_BM084) },
{ HID_USB_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_M_XT3URBK) },
{ HID_USB_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_M_XT3DRBK) },
+ { HID_USB_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_M_XT4DRBK) },
{ HID_USB_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_M_DT1URBK) },
{ HID_USB_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_M_DT1DRBK) },
{ HID_USB_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_M_HT1URBK) },
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 830c022c3624..c8902f2f0e23 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -373,6 +373,7 @@
#define USB_DEVICE_ID_ELECOM_BM084 0x0061
#define USB_DEVICE_ID_ELECOM_M_XT3URBK 0x00fb
#define USB_DEVICE_ID_ELECOM_M_XT3DRBK 0x00fc
+#define USB_DEVICE_ID_ELECOM_M_XT4DRBK 0x00fd
#define USB_DEVICE_ID_ELECOM_M_DT1URBK 0x00fe
#define USB_DEVICE_ID_ELECOM_M_DT1DRBK 0x00ff
#define USB_DEVICE_ID_ELECOM_M_HT1URBK 0x010c
diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c
index 9675cd500776..91049dff301f 100644
--- a/drivers/hid/hid-quirks.c
+++ b/drivers/hid/hid-quirks.c
@@ -337,6 +337,7 @@ static const struct hid_device_id hid_have_special_driver[] = {
{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_BM084) },
{ HID_USB_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_M_XT3URBK) },
{ HID_USB_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_M_XT3DRBK) },
+ { HID_USB_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_M_XT4DRBK) },
{ HID_USB_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_M_DT1URBK) },
{ HID_USB_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_M_DT1DRBK) },
{ HID_USB_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_M_HT1URBK) },
--
2.16.2
^ permalink raw reply related
* [PATCH 1/2] HID: elecom: refer to trackballs by model name instead of series
From: Tomasz Kramkowski @ 2018-03-01 17:22 UTC (permalink / raw)
To: Jiri Kosina
Cc: Benjamin Tissoires, linux-input, linux-kernel, Tomasz Kramkowski
This patch changes all references to ELECOM trackballs using their
series name to refer to them by their model name.
ELECOM provides multiple series of mice such as EX-G, HUGE, and DEFT.
Although it has not caused conflicts in the driver, there can be more
than one iteration of mice in each series. For example, there are 7
variants of EX-G trackballs but only three (M-XT3URBK, M-XT3DRBK, and
M-XT4DRBK) need a driver to work correctly.
There are also 4 DEFT series trackballs but two of them have the same
VID:PID as the other two. I picked the earlier model for the naming of
the PID macros.
Signed-off-by: Tomasz Kramkowski <tk@the-tk.com>
---
If this patch is too trivial to bother then feel free to ask and I'll
amend the second patch to use a different naming scheme.
I did a survey of mouse model names and VID:PID values here:
https://www.reddit.com/r/Trackballs/comments/7vqraz/
---
drivers/hid/Kconfig | 6 +++---
drivers/hid/hid-elecom.c | 30 +++++++++++++++---------------
drivers/hid/hid-ids.h | 12 ++++++------
drivers/hid/hid-quirks.c | 12 ++++++------
4 files changed, 30 insertions(+), 30 deletions(-)
diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 19c499f5623d..4989ec4028bf 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -280,9 +280,9 @@ config HID_ELECOM
---help---
Support for ELECOM devices:
- BM084 Bluetooth Mouse
- - EX-G Trackball (Wired and wireless)
- - DEFT Trackball (Wired and wireless)
- - HUGE Trackball (Wired and wireless)
+ - EX-G Trackballs (M-XT3DRBK, M-XT3URBK)
+ - DEFT Trackballs (M-DT1DRBK, M-DT1URBK, M-DT2DRBK, M-DT2URBK)
+ - HUGE Trackballs (M-HT1DRBK, M-HT1URBK)
config HID_ELO
tristate "ELO USB 4000/4500 touchscreen"
diff --git a/drivers/hid/hid-elecom.c b/drivers/hid/hid-elecom.c
index 1a1ecc491c02..21e15dc91aac 100644
--- a/drivers/hid/hid-elecom.c
+++ b/drivers/hid/hid-elecom.c
@@ -1,9 +1,9 @@
/*
* HID driver for ELECOM devices:
* - BM084 Bluetooth Mouse
- * - EX-G Trackball (Wired and wireless)
- * - DEFT Trackball (Wired and wireless)
- * - HUGE Trackball (Wired and wireless)
+ * - EX-G Trackballs (M-XT3DRBK, M-XT3URBK)
+ * - DEFT Trackballs (M-DT1DRBK, M-DT1URBK, M-DT2DRBK, M-DT2URBK)
+ * - HUGE Trackballs (M-HT1DRBK, M-HT1URBK)
*
* Copyright (c) 2010 Richard Nauber <Richard.Nauber@gmail.com>
* Copyright (c) 2016 Yuxuan Shui <yshuiv7@gmail.com>
@@ -65,14 +65,14 @@ static __u8 *elecom_report_fixup(struct hid_device *hdev, __u8 *rdesc,
rdesc[47] = 0x00;
}
break;
- case USB_DEVICE_ID_ELECOM_EX_G_WIRED:
- case USB_DEVICE_ID_ELECOM_EX_G_WIRELESS:
+ case USB_DEVICE_ID_ELECOM_M_XT3URBK:
+ case USB_DEVICE_ID_ELECOM_M_XT3DRBK:
mouse_button_fixup(hdev, rdesc, *rsize, 6);
break;
- case USB_DEVICE_ID_ELECOM_DEFT_WIRED:
- case USB_DEVICE_ID_ELECOM_DEFT_WIRELESS:
- case USB_DEVICE_ID_ELECOM_HUGE_WIRED:
- case USB_DEVICE_ID_ELECOM_HUGE_WIRELESS:
+ case USB_DEVICE_ID_ELECOM_M_DT1URBK:
+ case USB_DEVICE_ID_ELECOM_M_DT1DRBK:
+ case USB_DEVICE_ID_ELECOM_M_HT1URBK:
+ case USB_DEVICE_ID_ELECOM_M_HT1DRBK:
mouse_button_fixup(hdev, rdesc, *rsize, 8);
break;
}
@@ -81,12 +81,12 @@ static __u8 *elecom_report_fixup(struct hid_device *hdev, __u8 *rdesc,
static const struct hid_device_id elecom_devices[] = {
{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_BM084) },
- { HID_USB_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_EX_G_WIRED) },
- { HID_USB_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_EX_G_WIRELESS) },
- { HID_USB_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_DEFT_WIRED) },
- { HID_USB_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_DEFT_WIRELESS) },
- { HID_USB_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_HUGE_WIRED) },
- { HID_USB_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_HUGE_WIRELESS) },
+ { HID_USB_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_M_XT3URBK) },
+ { HID_USB_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_M_XT3DRBK) },
+ { HID_USB_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_M_DT1URBK) },
+ { HID_USB_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_M_DT1DRBK) },
+ { HID_USB_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_M_HT1URBK) },
+ { HID_USB_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_M_HT1DRBK) },
{ }
};
MODULE_DEVICE_TABLE(hid, elecom_devices);
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 43ddcdfbd0da..830c022c3624 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -371,12 +371,12 @@
#define USB_VENDOR_ID_ELECOM 0x056e
#define USB_DEVICE_ID_ELECOM_BM084 0x0061
-#define USB_DEVICE_ID_ELECOM_EX_G_WIRED 0x00fb
-#define USB_DEVICE_ID_ELECOM_EX_G_WIRELESS 0x00fc
-#define USB_DEVICE_ID_ELECOM_DEFT_WIRED 0x00fe
-#define USB_DEVICE_ID_ELECOM_DEFT_WIRELESS 0x00ff
-#define USB_DEVICE_ID_ELECOM_HUGE_WIRED 0x010c
-#define USB_DEVICE_ID_ELECOM_HUGE_WIRELESS 0x010d
+#define USB_DEVICE_ID_ELECOM_M_XT3URBK 0x00fb
+#define USB_DEVICE_ID_ELECOM_M_XT3DRBK 0x00fc
+#define USB_DEVICE_ID_ELECOM_M_DT1URBK 0x00fe
+#define USB_DEVICE_ID_ELECOM_M_DT1DRBK 0x00ff
+#define USB_DEVICE_ID_ELECOM_M_HT1URBK 0x010c
+#define USB_DEVICE_ID_ELECOM_M_HT1DRBK 0x010d
#define USB_VENDOR_ID_DREAM_CHEEKY 0x1d34
#define USB_DEVICE_ID_DREAM_CHEEKY_WN 0x0004
diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c
index 5f6035a5ce36..9675cd500776 100644
--- a/drivers/hid/hid-quirks.c
+++ b/drivers/hid/hid-quirks.c
@@ -335,12 +335,12 @@ static const struct hid_device_id hid_have_special_driver[] = {
#endif
#if IS_ENABLED(CONFIG_HID_ELECOM)
{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_BM084) },
- { HID_USB_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_EX_G_WIRED) },
- { HID_USB_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_EX_G_WIRELESS) },
- { HID_USB_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_DEFT_WIRED) },
- { HID_USB_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_DEFT_WIRELESS) },
- { HID_USB_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_HUGE_WIRED) },
- { HID_USB_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_HUGE_WIRELESS) },
+ { HID_USB_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_M_XT3URBK) },
+ { HID_USB_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_M_XT3DRBK) },
+ { HID_USB_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_M_DT1URBK) },
+ { HID_USB_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_M_DT1DRBK) },
+ { HID_USB_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_M_HT1URBK) },
+ { HID_USB_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_M_HT1DRBK) },
#endif
#if IS_ENABLED(CONFIG_HID_ELO)
{ HID_USB_DEVICE(USB_VENDOR_ID_ELO, 0x0009) },
--
2.16.2
^ permalink raw reply related
* [PATCH v2 2/2] dt-bindings: input: Add binding for RAVE SP input driver
From: Andrey Smirnov @ 2018-03-01 16:55 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Andrey Smirnov, linux-input, linux-kernel, Rob Herring,
Mark Rutland, devicetree, Guenter Roeck, Chris Healy, Lucas Stach
In-Reply-To: <20180301165527.22274-1-andrew.smirnov@gmail.com>
Add Device Tree bindings for RAVE SP input drvier - an MFD cell of
parent RAVE SP driver (documented in
Documentation/devicetree/bindings/mfd/zii,rave-sp.txt).
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: linux-input@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: devicetree@vger.kernel.org
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Chris Healy <cphealy@gmail.com>
Cc: Lucas Stach <l.stach@pengutronix.de>
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
.../bindings/input/zii,rave-sp-pwrbutton.txt | 24 ++++++++++++++++++++++
1 file changed, 24 insertions(+)
create mode 100644 Documentation/devicetree/bindings/input/zii,rave-sp-pwrbutton.txt
diff --git a/Documentation/devicetree/bindings/input/zii,rave-sp-pwrbutton.txt b/Documentation/devicetree/bindings/input/zii,rave-sp-pwrbutton.txt
new file mode 100644
index 000000000000..e14e12d27772
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/zii,rave-sp-pwrbutton.txt
@@ -0,0 +1,24 @@
+Zodiac Inflight Innovations RAVE Supervisory Processor Power Button Bindings
+
+RAVE SP input device is a "MFD cell" device corresponding to power
+button functionality of RAVE Supervisory Processor. It is expected
+that its Device Tree node is specified as a child of the node
+corresponding to the parent RAVE SP device (as documented in
+Documentation/devicetree/bindings/mfd/zii,rave-sp.txt)
+
+Required properties:
+
+- compatible: Should be "zii,rave-sp-pwrbutton"
+
+Example:
+
+ rave-sp {
+ compatible = "zii,rave-sp-rdu1";
+ current-speed = <38400>;
+
+ pwrbutton {
+ compatible = "zii,rave-sp-pwrbutton";
+ status = "okay";
+ };
+ }
+
--
2.14.3
^ permalink raw reply related
* [PATCH v2 1/2] input: Add RAVE SP Powerbutton driver
From: Andrey Smirnov @ 2018-03-01 16:55 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Andrey Smirnov, linux-input, linux-kernel, Rob Herring,
Mark Rutland, devicetree, Guenter Roeck, Chris Healy, Lucas Stach
Add driver that properly handles input event emitted by RAVE SP
devices.
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: linux-input@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: devicetree@vger.kernel.org
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Chris Healy <cphealy@gmail.com>
Cc: Lucas Stach <l.stach@pengutronix.de>
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
Changes since [v1]:
- Removed redundant dev.parent assignment
- Various cosmetic changes
[v1] lkml.kernel.org/r/20180226154130.25774-1-andrew.smirnov@gmail.com
drivers/input/misc/Kconfig | 9 ++++
drivers/input/misc/Makefile | 1 +
drivers/input/misc/rave-sp-pwrbutton.c | 94 ++++++++++++++++++++++++++++++++++
3 files changed, 104 insertions(+)
create mode 100644 drivers/input/misc/rave-sp-pwrbutton.c
diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
index 62a1312a7387..6a3c753b093b 100644
--- a/drivers/input/misc/Kconfig
+++ b/drivers/input/misc/Kconfig
@@ -841,4 +841,13 @@ config INPUT_HISI_POWERKEY
To compile this driver as a module, choose M here: the
module will be called hisi_powerkey.
+config INPUT_RAVE_SP_PWRBUTTON
+ tristate "RAVE SP Power button Driver"
+ depends on RAVE_SP_CORE
+ help
+ Say Y here if you want to enable power key reporting from RAVE SP
+
+ To compile this driver as a module, choose M here: the
+ module will be called rave-sp-pwrbutton.
+
endif
diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
index a8f61af865aa..8cc58f362bb8 100644
--- a/drivers/input/misc/Makefile
+++ b/drivers/input/misc/Makefile
@@ -60,6 +60,7 @@ obj-$(CONFIG_INPUT_PMIC8XXX_PWRKEY) += pmic8xxx-pwrkey.o
obj-$(CONFIG_INPUT_POWERMATE) += powermate.o
obj-$(CONFIG_INPUT_PWM_BEEPER) += pwm-beeper.o
obj-$(CONFIG_INPUT_PWM_VIBRA) += pwm-vibra.o
+obj-$(CONFIG_INPUT_RAVE_SP_PWRBUTTON) += rave-sp-pwrbutton.o
obj-$(CONFIG_INPUT_RB532_BUTTON) += rb532_button.o
obj-$(CONFIG_INPUT_REGULATOR_HAPTIC) += regulator-haptic.o
obj-$(CONFIG_INPUT_RETU_PWRBUTTON) += retu-pwrbutton.o
diff --git a/drivers/input/misc/rave-sp-pwrbutton.c b/drivers/input/misc/rave-sp-pwrbutton.c
new file mode 100644
index 000000000000..bcab3cdb7ebd
--- /dev/null
+++ b/drivers/input/misc/rave-sp-pwrbutton.c
@@ -0,0 +1,94 @@
+// SPDX-License-Identifier: GPL-2.0+
+//
+// Power Button driver for RAVE SP
+//
+// Copyright (C) 2017 Zodiac Inflight Innovations
+//
+//
+
+#include <linux/input.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mfd/rave-sp.h>
+#include <linux/platform_device.h>
+
+#define RAVE_SP_EVNT_BUTTON_PRESS (RAVE_SP_EVNT_BASE + 0x00)
+
+struct rave_sp_power_button {
+ struct input_dev *idev;
+ struct notifier_block nb;
+};
+
+static int rave_sp_power_button_event(struct notifier_block *nb,
+ unsigned long action, void *data)
+{
+ struct rave_sp_power_button *pb =
+ container_of(nb, struct rave_sp_power_button, nb);
+ const u8 event = rave_sp_action_unpack_event(action);
+ const u8 value = rave_sp_action_unpack_value(action);
+ struct input_dev *idev = pb->idev;
+
+ if (event == RAVE_SP_EVNT_BUTTON_PRESS) {
+ input_report_key(idev, KEY_POWER, value);
+ input_sync(idev);
+
+ return NOTIFY_STOP;
+ }
+
+ return NOTIFY_DONE;
+}
+
+static int rave_sp_pwrbutton_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct rave_sp_power_button *pb;
+ struct input_dev *idev;
+ int error;
+
+ pb = devm_kzalloc(dev, sizeof(*pb), GFP_KERNEL);
+ if (!pb)
+ return -ENOMEM;
+
+ idev = devm_input_allocate_device(dev);
+ if (!idev)
+ return -ENOMEM;
+
+ idev->name = pdev->name;
+
+ input_set_capability(idev, EV_KEY, KEY_POWER);
+
+ error = input_register_device(idev);
+ if (error)
+ return error;
+
+ pb->idev = idev;
+ pb->nb.notifier_call = rave_sp_power_button_event;
+ pb->nb.priority = 128;
+
+ error = devm_rave_sp_register_event_notifier(dev, &pb->nb);
+ if (error)
+ return error;
+
+ return 0;
+}
+
+static const struct of_device_id rave_sp_pwrbutton_of_match[] = {
+ { .compatible = "zii,rave-sp-pwrbutton" },
+ {}
+};
+
+static struct platform_driver rave_sp_pwrbutton_driver = {
+ .probe = rave_sp_pwrbutton_probe,
+ .driver = {
+ .name = KBUILD_MODNAME,
+ .of_match_table = rave_sp_pwrbutton_of_match,
+ },
+};
+module_platform_driver(rave_sp_pwrbutton_driver);
+
+MODULE_DEVICE_TABLE(of, rave_sp_pwrbutton_of_match);
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Andrey Vostrikov <andrey.vostrikov@cogentembedded.com>");
+MODULE_AUTHOR("Nikita Yushchenko <nikita.yoush@cogentembedded.com>");
+MODULE_AUTHOR("Andrey Smirnov <andrew.smirnov@gmail.com>");
+MODULE_DESCRIPTION("RAVE SP Power Button driver");
--
2.14.3
^ permalink raw reply related
* [RFC/RFT patch 3/7] Input: evdev - Conflate clock MONOTONIC and BOOTTIME
From: Thomas Gleixner @ 2018-03-01 16:33 UTC (permalink / raw)
To: LKML
Cc: Linus Torvalds, Ingo Molnar, Peter Zijlstra, Steven Rostedt,
John Stultz, Petr Mladek, Mark Salyzyn, Prarit Bhargava,
Sergey Senozhatsky, Dmitry Torokhov, Kevin Easton,
Michael Kerrisk, Jonathan Corbet, linux-input
In-Reply-To: <20180301163331.987775783@linutronix.de>
[-- Attachment #1: Input--evdev---Conflate-clock-MONOTONIC-and-BOOTTIME.patch --]
[-- Type: text/plain, Size: 1116 bytes --]
Now that clock MONOTONIC and BOOTTIME are indentical remove all the special
casing. The user space visible interfaces still support both clocks.
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: linux-input@vger.kernel.org
---
drivers/input/evdev.c | 7 +------
1 file changed, 1 insertion(+), 6 deletions(-)
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -31,7 +31,6 @@
enum evdev_clock_type {
EV_CLK_REAL = 0,
EV_CLK_MONO,
- EV_CLK_BOOT,
EV_CLK_MAX
};
@@ -198,12 +197,10 @@ static int evdev_set_clk_type(struct evd
case CLOCK_REALTIME:
clk_type = EV_CLK_REAL;
break;
+ case CLOCK_BOOTTIME:
case CLOCK_MONOTONIC:
clk_type = EV_CLK_MONO;
break;
- case CLOCK_BOOTTIME:
- clk_type = EV_CLK_BOOT;
- break;
default:
return -EINVAL;
}
@@ -314,8 +311,6 @@ static void evdev_events(struct input_ha
ev_time[EV_CLK_MONO] = ktime_get();
ev_time[EV_CLK_REAL] = ktime_mono_to_real(ev_time[EV_CLK_MONO]);
- ev_time[EV_CLK_BOOT] = ktime_mono_to_any(ev_time[EV_CLK_MONO],
- TK_OFFS_BOOT);
rcu_read_lock();
^ permalink raw reply
* Re: [PATCH 1/2] input: Add RAVE SP Powerbutton driver
From: Andrey Smirnov @ 2018-03-01 16:05 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: linux-input, linux-kernel, Rob Herring, Mark Rutland,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Guenter Roeck, Chris Healy, Lucas Stach
In-Reply-To: <20180228045935.GB151045@dtor-ws>
On Tue, Feb 27, 2018 at 8:59 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> Hi Andrey,
>
> On Mon, Feb 26, 2018 at 07:41:29AM -0800, Andrey Smirnov wrote:
>> Add driver that properly handles input event emitted by RAVE SP
>> devices.
>>
>> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> Cc: linux-input@vger.kernel.org
>> Cc: linux-kernel@vger.kernel.org
>> Cc: Rob Herring <robh+dt@kernel.org>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Cc: devicetree@vger.kernel.org
>> Cc: Guenter Roeck <linux@roeck-us.net>
>> Cc: Chris Healy <cphealy@gmail.com>
>> Cc: Lucas Stach <l.stach@pengutronix.de>
>> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
>> ---
>> drivers/input/misc/Kconfig | 9 ++++
>> drivers/input/misc/Makefile | 1 +
>> drivers/input/misc/rave-sp-pwrbutton.c | 92 ++++++++++++++++++++++++++++++++++
>> 3 files changed, 102 insertions(+)
>> create mode 100644 drivers/input/misc/rave-sp-pwrbutton.c
>>
>> diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
>> index 62a1312a7387..6a3c753b093b 100644
>> --- a/drivers/input/misc/Kconfig
>> +++ b/drivers/input/misc/Kconfig
>> @@ -841,4 +841,13 @@ config INPUT_HISI_POWERKEY
>> To compile this driver as a module, choose M here: the
>> module will be called hisi_powerkey.
>>
>> +config INPUT_RAVE_SP_PWRBUTTON
>> + tristate "RAVE SP Power button Driver"
>> + depends on RAVE_SP_CORE
>> + help
>> + Say Y here if you want to enable power key reporting from RAVE SP
>> +
>> + To compile this driver as a module, choose M here: the
>> + module will be called rave-sp-pwrbutton.
>> +
>> endif
>> diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
>> index a8f61af865aa..6ebeec1f4dd0 100644
>> --- a/drivers/input/misc/Makefile
>> +++ b/drivers/input/misc/Makefile
>> @@ -80,3 +80,4 @@ obj-$(CONFIG_INPUT_WM831X_ON) += wm831x-on.o
>> obj-$(CONFIG_INPUT_XEN_KBDDEV_FRONTEND) += xen-kbdfront.o
>> obj-$(CONFIG_INPUT_YEALINK) += yealink.o
>> obj-$(CONFIG_INPUT_IDEAPAD_SLIDEBAR) += ideapad_slidebar.o
>> +obj-$(CONFIG_INPUT_RAVE_SP_PWRBUTTON) += rave-sp-pwrbutton.o
>
> Please put it in [somewhat] alphabetical order (even though ideapad is
> out of order).
>
OK, will do in v2.
>
>> diff --git a/drivers/input/misc/rave-sp-pwrbutton.c b/drivers/input/misc/rave-sp-pwrbutton.c
>> new file mode 100644
>> index 000000000000..7511062c724b
>> --- /dev/null
>> +++ b/drivers/input/misc/rave-sp-pwrbutton.c
>> @@ -0,0 +1,92 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +
>> +/*
>> + * Power Button driver for RAVE SP
>> + *
>> + * Copyright (C) 2017 Zodiac Inflight Innovations
>> + *
>> + */
>
> Let's keep SPDX and copyright in the same // comment block.
>
This goes against kernel coding style, but sure.
>> +
>> +#include <linux/input.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/mfd/rave-sp.h>
>> +#include <linux/platform_device.h>
>> +
>> +#define RAVE_SP_EVNT_BUTTON_PRESS (RAVE_SP_EVNT_BASE + 0x00)
>> +
>> +struct rave_sp_power_button {
>> + struct input_dev *idev;
>> + struct notifier_block nb;
>> +};
>> +
>> +static int rave_sp_power_button_event(struct notifier_block *nb,
>> + unsigned long action, void *data)
>> +{
>> + struct rave_sp_power_button *pb =
>> + container_of(nb, struct rave_sp_power_button, nb);
>> + const u8 event = rave_sp_action_unpack_event(action);
>> + const u8 value = rave_sp_action_unpack_value(action);
>> + struct input_dev *idev = pb->idev;
>> +
>> + if (event == RAVE_SP_EVNT_BUTTON_PRESS) {
>> + input_report_key(idev, KEY_POWER, value);
>> + input_sync(idev);
>> +
>> + return NOTIFY_STOP;
>> + }
>> +
>> + return NOTIFY_DONE;
>> +}
>> +
>> +static int rave_sp_pwrbutton_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct rave_sp_power_button *pb;
>> + struct input_dev *idev;
>> + int ret;
>> +
>> + pb = devm_kzalloc(dev, sizeof(*pb), GFP_KERNEL);
>> + if (!pb)
>> + return -ENOMEM;
>> +
>> + idev = devm_input_allocate_device(dev);
>> + if (!idev)
>> + return -ENOMEM;
>> +
>> + idev->name = pdev->name;
>> + idev->dev.parent = dev;
>
> Not needed - done by devm_input_allocate_device().
>
Good point, will remove.
>> +
>> + input_set_capability(idev, EV_KEY, KEY_POWER);
>> +
>> + ret = input_register_device(idev);
>> + if (ret)
>> + return ret;
>
> Please call this "error".
Sure, will do.
>
>> +
>> + pb->idev = idev;
>> + pb->nb.notifier_call = rave_sp_power_button_event;
>> + pb->nb.priority = 128;
>> +
>> + return devm_rave_sp_register_event_notifier(dev, &pb->nb);
>
> Prefer
>
> error = devm_rave...();
> if (error)
> return error;
>
> return 0;
>
OK, will do in v2.
Thanks,
Andrey Smirnov
^ permalink raw reply
* Re: [PATCH v4] Input: gpio_keys: Add level trigger support for GPIO keys
From: Arnd Bergmann @ 2018-03-01 14:29 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Baolin Wang, Dmitry Torokhov, Rob Herring, Mark Rutland,
Greg Kroah-Hartman, stephen lu, Arvind Yadav, Joseph Lo,
Kate Stewart, Philippe Ombredanne, Thomas Gleixner, linux-input,
devicetree, Linux Kernel Mailing List, Mark Brown, Linus Walleij
In-Reply-To: <CAHp75VdOLUF8UQYkuK0Pmi9uefJgk4oZVBerFoBObOXM7zpCUQ@mail.gmail.com>
On Thu, Mar 1, 2018 at 2:59 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Thu, Mar 1, 2018 at 2:37 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Thu, Mar 1, 2018 at 1:09 PM, Andy Shevchenko
>> <andy.shevchenko@gmail.com> wrote:
>>> On Thu, Mar 1, 2018 at 9:36 AM, Baolin Wang <baolin.wang@linaro.org> wrote:
>>>> On some platforms (such as Spreadtrum platform), the GPIO keys can only
>>>> be triggered by level type. So this patch introduces one trigger_type to
>>>> indicate if the button's interrupt type is level trigger or edge trigger.
>>>
>>>> button->irq =
>>>> irq_of_parse_and_map(to_of_node(child), 0);
>>>
>>> AFAIU, 0 means NO_IRQ, thus,
>>>
>>>> + if (button->irq)
>>>> + button->trigger_type =
>>>> + irq_get_trigger_type(button->irq);
>>>
>>> irq_get_trigger_type(NO_IRQ) should return 0.
>>>
>>> Therefore, if (button->irq) is redundant.
>>> Did I miss anything?
>>
>> "irq_of_parse_and_map(to_of_node(child), 0);" is the first interrupt. A
>> more common way to express the same thing in modern drivers is
>> 'platform_get_irq(pdev, 0)'.
>
> Yes, though I'm talking about _returned_ value.
Got it, yes you are right.
Arnd
^ permalink raw reply
* Re: [PATCH v4] Input: gpio_keys: Add level trigger support for GPIO keys
From: Andy Shevchenko @ 2018-03-01 13:59 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Baolin Wang, Dmitry Torokhov, Rob Herring, Mark Rutland,
Greg Kroah-Hartman, stephen lu, Arvind Yadav, Joseph Lo,
Kate Stewart, Philippe Ombredanne, Thomas Gleixner, linux-input,
devicetree, Linux Kernel Mailing List, Mark Brown, Linus Walleij
In-Reply-To: <CAK8P3a3iK7Oo_XrkoqXyE0cXBm9Ngx18Yyvk+0UJNN1Gz5DYcQ@mail.gmail.com>
On Thu, Mar 1, 2018 at 2:37 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Thu, Mar 1, 2018 at 1:09 PM, Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>> On Thu, Mar 1, 2018 at 9:36 AM, Baolin Wang <baolin.wang@linaro.org> wrote:
>>> On some platforms (such as Spreadtrum platform), the GPIO keys can only
>>> be triggered by level type. So this patch introduces one trigger_type to
>>> indicate if the button's interrupt type is level trigger or edge trigger.
>>
>>> button->irq =
>>> irq_of_parse_and_map(to_of_node(child), 0);
>>
>> AFAIU, 0 means NO_IRQ, thus,
>>
>>> + if (button->irq)
>>> + button->trigger_type =
>>> + irq_get_trigger_type(button->irq);
>>
>> irq_get_trigger_type(NO_IRQ) should return 0.
>>
>> Therefore, if (button->irq) is redundant.
>> Did I miss anything?
>
> "irq_of_parse_and_map(to_of_node(child), 0);" is the first interrupt. A
> more common way to express the same thing in modern drivers is
> 'platform_get_irq(pdev, 0)'.
Yes, though I'm talking about _returned_ value.
> However, there is one more thing I'm missing: with my suggestion
> of emulating edge-triggered interrupts in the irqchip/gpio driver, the
> trigger type here should also be 'IRQF_TRIGGER_RISING |
> IRQF_TRIGGER_FALLING' in the DT description, so I don't think
> we need to change the gpio-keys driver at all, just the gpio driver.
Sounds even better.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: rmi4 defunct on T460p [was: [git pull] Input updates for v4.15-rc8]
From: Arkadiusz Hiler @ 2018-03-01 13:29 UTC (permalink / raw)
To: Marc-André Lureau
Cc: Jiri Slaby, Dmitry Torokhov, open list, linux-input,
Greg Kroah-Hartman
In-Reply-To: <CAJ+F1CLFE=02hBY8paC83c-GjcPBM3YekAeYwxaporw8QczLFw@mail.gmail.com>
On Thu, Mar 01, 2018 at 02:22:10PM +0100, Marc-André Lureau wrote:
> Hi
>
> On Tue, Feb 13, 2018 at 9:22 AM, Jiri Slaby <jslaby@suse.cz> wrote:
> > On 02/09/2018, 02:39 AM, Dmitry Torokhov wrote:
> >> On Thu, Feb 08, 2018 at 11:04:22AM +0100, Jiri Slaby wrote:
> >>> On 01/19/2018, 06:42 PM, Dmitry Torokhov wrote:
> >>>> Please pull from:
> >>>>
> >>>> git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git for-linus
> >>>>
> >>>> to receive updates for the input subsystem. You will get:
> >>>>
> >>> ...
> >>>> - touchpad on Lenovo T640p is switched over to SMbus/RMI
> >>> ...
> >>>
> >>>> 王振杰 (1):
> >>>> Input: synaptics - Lenovo Thinkpad T460p devices should use RMI
> >>>
> >>> Hi,
> >>>
> >>> one of openSUSE users with T460p reports that the touchpad is defunct
> >>> after resume in 4.15. psmouse.synaptics_intertouch=0 works around the
> >>> problem.
> >>>
> >>> The problem is:
> >>> kernel: rmi4_smbus 8-002c: failed to get SMBus version number!
> >>> kernel: rmi4_physical rmi4-00: rmi_driver_reset_handler: Failed to read
> >>> current IRQ mask.
> >>> kernel: rmi4_f01 rmi4-00.fn01: Failed to restore normal operation: -16.
> >>> kernel: rmi4_f01 rmi4-00.fn01: Resume failed with code -16.
> >>> kernel: rmi4_physical rmi4-00: Failed to suspend functions: -16
> >>> kernel: rmi4_smbus 8-002c: Failed to resume device: -16
> >>> kernel: rmi4_f03 rmi4-00.fn03: rmi_f03_pt_write: Failed to write to F03
> >>> TX register (-16).
> >>>
> >>> The downstream bug is at:
> >>> https://bugzilla.suse.com/show_bug.cgi?id=1079862
> >>>
> >>> Any ideas?
> >>
> >> Hmm, maybe the seuence between psmouse and rmi/smbus is not quite right
> >> on resume on that box. Can you ask the reporter to:
> >>
> >> echo 1 > /sys/power/pm_print_times
> >> suspend/resume
> >
> > He uploaded it here:
> > https://bugzilla.suse.com/attachment.cgi?id=759933
> >
>
> Same problem (with f27 and upstream kernel), is there anything else we
> can do to help? Should we open a bug on http://bugzilla.kernel.org ?
>
> thanks
There are already two reverts on this mailing list that fix the problem.
https://patchwork.kernel.org/patch/10239877/
and
https://patchwork.kernel.org/patch/10228565/
Since the regression made it to the stable kernel and this is not moving
I think it's appropiate to cc Greg here.
- Arek
^ permalink raw reply
* Re: rmi4 defunct on T460p [was: [git pull] Input updates for v4.15-rc8]
From: Marc-André Lureau @ 2018-03-01 13:22 UTC (permalink / raw)
To: Jiri Slaby; +Cc: Dmitry Torokhov, open list, linux-input
In-Reply-To: <6eb210d1-5b1b-d7ad-6188-e632fde2c6f7@suse.cz>
Hi
On Tue, Feb 13, 2018 at 9:22 AM, Jiri Slaby <jslaby@suse.cz> wrote:
> On 02/09/2018, 02:39 AM, Dmitry Torokhov wrote:
>> On Thu, Feb 08, 2018 at 11:04:22AM +0100, Jiri Slaby wrote:
>>> On 01/19/2018, 06:42 PM, Dmitry Torokhov wrote:
>>>> Please pull from:
>>>>
>>>> git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git for-linus
>>>>
>>>> to receive updates for the input subsystem. You will get:
>>>>
>>> ...
>>>> - touchpad on Lenovo T640p is switched over to SMbus/RMI
>>> ...
>>>
>>>> 王振杰 (1):
>>>> Input: synaptics - Lenovo Thinkpad T460p devices should use RMI
>>>
>>> Hi,
>>>
>>> one of openSUSE users with T460p reports that the touchpad is defunct
>>> after resume in 4.15. psmouse.synaptics_intertouch=0 works around the
>>> problem.
>>>
>>> The problem is:
>>> kernel: rmi4_smbus 8-002c: failed to get SMBus version number!
>>> kernel: rmi4_physical rmi4-00: rmi_driver_reset_handler: Failed to read
>>> current IRQ mask.
>>> kernel: rmi4_f01 rmi4-00.fn01: Failed to restore normal operation: -16.
>>> kernel: rmi4_f01 rmi4-00.fn01: Resume failed with code -16.
>>> kernel: rmi4_physical rmi4-00: Failed to suspend functions: -16
>>> kernel: rmi4_smbus 8-002c: Failed to resume device: -16
>>> kernel: rmi4_f03 rmi4-00.fn03: rmi_f03_pt_write: Failed to write to F03
>>> TX register (-16).
>>>
>>> The downstream bug is at:
>>> https://bugzilla.suse.com/show_bug.cgi?id=1079862
>>>
>>> Any ideas?
>>
>> Hmm, maybe the seuence between psmouse and rmi/smbus is not quite right
>> on resume on that box. Can you ask the reporter to:
>>
>> echo 1 > /sys/power/pm_print_times
>> suspend/resume
>
> He uploaded it here:
> https://bugzilla.suse.com/attachment.cgi?id=759933
>
Same problem (with f27 and upstream kernel), is there anything else we
can do to help? Should we open a bug on http://bugzilla.kernel.org ?
thanks
--
Marc-André Lureau
^ permalink raw reply
* Re: [PATCH v4] Input: gpio_keys: Add level trigger support for GPIO keys
From: Arnd Bergmann @ 2018-03-01 12:37 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Baolin Wang, Dmitry Torokhov, Rob Herring, Mark Rutland,
Greg Kroah-Hartman, stephen lu, Arvind Yadav, Joseph Lo,
Kate Stewart, Philippe Ombredanne, Thomas Gleixner, linux-input,
devicetree, Linux Kernel Mailing List, Mark Brown, Linus Walleij
In-Reply-To: <CAHp75VcgUmOoo-jRbM-Gyp1JTY-2rJri8e7u2miYSvRM_4HmPg@mail.gmail.com>
On Thu, Mar 1, 2018 at 1:09 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Thu, Mar 1, 2018 at 9:36 AM, Baolin Wang <baolin.wang@linaro.org> wrote:
>> On some platforms (such as Spreadtrum platform), the GPIO keys can only
>> be triggered by level type. So this patch introduces one trigger_type to
>> indicate if the button's interrupt type is level trigger or edge trigger.
>
>> button->irq =
>> irq_of_parse_and_map(to_of_node(child), 0);
>
> AFAIU, 0 means NO_IRQ, thus,
>
>> + if (button->irq)
>> + button->trigger_type =
>> + irq_get_trigger_type(button->irq);
>
> irq_get_trigger_type(NO_IRQ) should return 0.
>
> Therefore, if (button->irq) is redundant.
> Did I miss anything?
"irq_of_parse_and_map(to_of_node(child), 0);" is the first interrupt. A
more common way to express the same thing in modern drivers is
'platform_get_irq(pdev, 0)'.
However, there is one more thing I'm missing: with my suggestion
of emulating edge-triggered interrupts in the irqchip/gpio driver, the
trigger type here should also be 'IRQF_TRIGGER_RISING |
IRQF_TRIGGER_FALLING' in the DT description, so I don't think
we need to change the gpio-keys driver at all, just the gpio driver.
Arnd
^ permalink raw reply
* Re: [PATCH v4] Input: gpio_keys: Add level trigger support for GPIO keys
From: Andy Shevchenko @ 2018-03-01 12:09 UTC (permalink / raw)
To: Baolin Wang
Cc: Dmitry Torokhov, Rob Herring, Mark Rutland, Greg Kroah-Hartman,
stephen lu, Arvind Yadav, Joseph Lo, Kate Stewart,
Philippe Ombredanne, Thomas Gleixner, linux-input, devicetree,
Linux Kernel Mailing List, Mark Brown, Linus Walleij,
Arnd Bergmann
In-Reply-To: <9ea2faf9c1cf8439f3a8ad495c6a2b49dacad7bd.1519889208.git.baolin.wang@linaro.org>
On Thu, Mar 1, 2018 at 9:36 AM, Baolin Wang <baolin.wang@linaro.org> wrote:
> On some platforms (such as Spreadtrum platform), the GPIO keys can only
> be triggered by level type. So this patch introduces one trigger_type to
> indicate if the button's interrupt type is level trigger or edge trigger.
> button->irq =
> irq_of_parse_and_map(to_of_node(child), 0);
AFAIU, 0 means NO_IRQ, thus,
> + if (button->irq)
> + button->trigger_type =
> + irq_get_trigger_type(button->irq);
irq_get_trigger_type(NO_IRQ) should return 0.
Therefore, if (button->irq) is redundant.
Did I miss anything?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v4 1/4] HID: add driver for Valve Steam Controller
From: Andy Shevchenko @ 2018-03-01 10:20 UTC (permalink / raw)
To: Rodrigo Rivas Costa
Cc: Jiri Kosina, Benjamin Tissoires, Pierre-Loup A. Griffais,
Cameron Gutman, Clément VUCHENER, Linux Kernel Mailing List,
linux-input
In-Reply-To: <20180228224926.GA7009@casa>
On Thu, Mar 1, 2018 at 12:49 AM, Rodrigo Rivas Costa
<rodrigorivascosta@gmail.com> wrote:
> On Wed, Feb 28, 2018 at 09:21:15PM +0200, Andy Shevchenko wrote:
>> On Wed, Feb 28, 2018 at 8:43 PM, Rodrigo Rivas Costa
>> <rodrigorivascosta@gmail.com> wrote:
>> > + if (input) {
>>
>> if (!input)
>> return;
>>
>> ?
> That was because of symmetry, because further patches add more objects.
> Then
> if (input)
> free(input);
> if (battery)
> free(battery);
> /* in the future *(
> if (input_gyro)
> free(input_gyro);
>
> Sure, the last one can do an early return, but you break symmetry.
The generic APIs when designed properly are NULL aware at freeing resources.
So, it should look like
free(input);
free(battery);
free(input_gyro);
>> > + if (steam) {
>> > + cancel_work_sync(&steam->work_connect);
>> > + steam_unregister(steam);
>>
>> > + hid_set_drvdata(hdev, NULL);
>>
>> Hmm.. Doesn't HID do this?
>
> Do you mean the hid_set_drvdata(hdev, NULL)? I'm not sure, I didn't see
> that on hid-core.c or hid-generic.c. And a call like this is done in
> many modules... so I did the same, just to be sure.
What you are looking for is, AFAIU:
drivers/base/dd.c:902: dev_set_drvdata(dev, NULL);
>>
>> > + }
>>
>> if (steam) {
>> ...
>> hid_hw_stop(hdev);
>> ...
>> } else {
>> hid_hw_stop(hdev);
>> }
>>
>> ?
>
> I have no real preference. Your version has two 'if', mine has two 'if'.
> What about:
>
> static void steam_remove(struct hid_device *hdev)
> {
> struct steam_device *steam = hid_get_drvdata(hdev);
>
> if (!steam) {
> hid_hw_stop(hdev);
> return;
> }
>
> if (steam->quirks & STEAM_QUIRK_WIRELESS) {
> hid_info(hdev, "Steam wireless receiver disconnected");
> hid_hw_close(hdev);
> }
> hid_hw_stop(hdev);
> cancel_work_sync(&steam->work_connect);
> steam_unregister(steam);
> hid_set_drvdata(hdev, NULL);
w/o this one.
> }
For me my version looks more compact to read, but at the end it's up to you.
Another option split if (steam) variant into helper, and thus
if (steam)
steam_non_null_device_remove();
else
hid_hw_stop();
But again, up to you (that's why question mark in the first place above).
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v4 1/4] HID: add driver for Valve Steam Controller
From: Marcus Folkesson @ 2018-03-01 7:50 UTC (permalink / raw)
To: Rodrigo Rivas Costa
Cc: Andy Shevchenko, Jiri Kosina, Benjamin Tissoires,
Pierre-Loup A. Griffais, Cameron Gutman, Clément VUCHENER,
Linux Kernel Mailing List, linux-input
In-Reply-To: <20180228224926.GA7009@casa>
Rodrigo,
On Wed, Feb 28, 2018 at 11:49:26PM +0100, Rodrigo Rivas Costa wrote:
> On Wed, Feb 28, 2018 at 09:21:15PM +0200, Andy Shevchenko wrote:
> > On Wed, Feb 28, 2018 at 8:43 PM, Rodrigo Rivas Costa
> > <rodrigorivascosta@gmail.com> wrote:
> > > There are two ways to connect the Steam Controller: directly to the USB
> > > or with the USB wireless adapter. Both methods are similar, but the
> > > wireless adapter can connect up to 4 devices at the same time.
> > >
> > > The wired device will appear as 3 interfaces: a virtual mouse, a virtual
> > > keyboard and a custom HID device.
> > >
> > > The wireless device will appear as 5 interfaces: a virtual keyboard and
> > > 4 custom HID devices, that will remain silent until a device is actually
> > > connected.
> > >
> > > The custom HID device has a report descriptor with all vendor specific
> > > usages, so the hid-generic is not very useful. In a PC/SteamBox Valve
> > > Steam Client provices a software translation by using direct USB access
> > > and a creates a uinput virtual gamepad.
> > >
> > > This driver was reverse engineered to provide direct kernel support in
> > > case you cannot, or do not want to, use Valve Steam Client. It disables
> > > the virtual keyboard and mouse, as they are not so useful when you have
> > > a working gamepad.
> >
> >
> > > +// SPDX-License-Identifier: GPL-2.0
> >
> > > +MODULE_LICENSE("GPL");
> >
> > Not the same.
>
> Hmmm... I copied from usb-skeleton.c, IIRC...
> I'll change to GPL-2.0+, that would be correct, I think.
Yep, the usb-skeleton.c is wrong.
I have prepared a patch, just not submitted it yet..
GPL-2.0+ is "GPLv2 or later" if that is what you want.
Best regards
Marcus Folkesson
^ permalink raw reply
* Re: [PATCH v2] Input: gpio_keys: Add level trigger support for GPIO keys
From: Baolin Wang @ 2018-03-01 7:41 UTC (permalink / raw)
To: Linus Walleij
Cc: Dmitry Torokhov, Rob Herring, Mark Rutland, Greg KH, stephen lu,
Arvind Yadav, Joseph Lo, Kate Stewart, Philippe Ombredanne,
Thomas Gleixner, Linux Input, DTML, LKML, Mark Brown,
Arnd Bergmann
In-Reply-To: <CACRpkdad4ZYa27Xd73QAsxvhJmBZnFRhDk-10yiitKYSrvxzWQ@mail.gmail.com>
On 28 February 2018 at 20:53, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Mon, Feb 26, 2018 at 7:24 AM, Baolin Wang <baolin.wang@linaro.org> wrote:
>> On 21 February 2018 at 19:35, Baolin Wang <baolin.wang@linaro.org> wrote:
>>> On 20 February 2018 at 02:11, Rob Herring <robh@kernel.org> wrote:
>
>>>>> diff --git a/Documentation/devicetree/bindings/input/gpio-keys.txt b/Documentation/devicetree/bindings/input/gpio-keys.txt
>>>>> index a949404..e3104bd 100644
>>>>> --- a/Documentation/devicetree/bindings/input/gpio-keys.txt
>>>>> +++ b/Documentation/devicetree/bindings/input/gpio-keys.txt
>>>>> @@ -29,6 +29,8 @@ Optional subnode-properties:
>>>>> - linux,can-disable: Boolean, indicates that button is connected
>>>>> to dedicated (not shared) interrupt which can be disabled to
>>>>> suppress events from the button.
>>>>> + - gpio-key,level-trigger: Boolean, indicates that button's interrupt
>>>>> + type is level trigger. Otherwise it is edge trigger as default.
>>>>
>>>> No. Just use 'interrupts' instead of 'gpios' and specify the trigger
>>>> type. Or put both if you need to read the state.
>>>
>>> Okay, so something as below to get the level type from the
>>> 'interrupts' property.
>>> if (fwnode_property_read_u32(child, "interrupts", &button->level_type))
>>> button->level_type = IRQ_TYPE_NONE;
>>
>> After more thinking, if we use 'interrupts' to indicate the irq type
>> for this case, we cannot specify the irq number due to the irq number
>> should be get by gpiod_to_irq(). So the device nodes look weird, since
>> we should define the index of the interrupt controller instead of the
>> irq type if the #interrupt_cells is set to 1 according to the
>> interrupt controller documentation. What do you think about this?
>
> I think what you're ultimately seeing is a bad fit between this
> GPIO/irq-controller and the Linux gpio keys driver, it doesn't
> have very much to do with the device tree bindings.
>
> What I think is appropriate is to try to create a new input driver
> in Linux that just takes an interrupt, not a GPIO, and let the
> GPIO chip only act as an irqchip for this.
>
> This avoid the complicated step of converting a GPIO to an
> interrupt in order to use it, when all you really want to
> do is use an interrupt, you don't really care about the
> GPIO here, correct?
Sometimes we should set the GPIO debounce, and read GPIO value to
report the event, so I think we can not remove GPIO.
>
> So we would create
> drivers/input/keyboard/interrupt_keys.c
> however I suspect a bunch of code would need to be shared
> with gpio_keys.c so maybe it is necessary to break out the
> parts of gpio_keys.c into its own helper file.
>
> Or maybe even have the
> pure interrupt handling as part of gpio_keys.c, i.e. if the
> driver can't find any associated GPIO, it goes on to
> check if there is an interrupt assigned to the device node
> and use that directly instead.
>
> Either way, Dmitry must be involved.
>
> Yours,
> Linus Walleij
--
Baolin.wang
Best Regards
^ permalink raw reply
* [PATCH v4] Input: gpio_keys: Add level trigger support for GPIO keys
From: Baolin Wang @ 2018-03-01 7:36 UTC (permalink / raw)
To: dmitry.torokhov, robh+dt, mark.rutland
Cc: gregkh, lumotuwe, arvind.yadav.cs, josephl, kstewart, pombredanne,
tglx, linux-input, devicetree, linux-kernel, broonie,
linus.walleij, arnd, baolin.wang
On some platforms (such as Spreadtrum platform), the GPIO keys can only
be triggered by level type. So this patch introduces one trigger_type to
indicate if the button's interrupt type is level trigger or edge trigger.
Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
Changes since v3:
- Remove the reserse level logic into gpio irqchip.
- Add one trigger_type to indicate teh button's trigger type.
Changes since v2:
- Use 'interrupt' property to indicate the irq type.
Changes since v1:
- Diable the GPIO irq until reversing the GPIO level type.
---
drivers/input/keyboard/gpio_keys.c | 12 ++++++++++--
include/linux/gpio_keys.h | 2 ++
2 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index 87e613d..614ee29 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -566,7 +566,10 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
INIT_DELAYED_WORK(&bdata->work, gpio_keys_gpio_work_func);
isr = gpio_keys_gpio_isr;
- irqflags = IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING;
+ if (button->trigger_type)
+ irqflags = button->trigger_type;
+ else
+ irqflags = IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING;
} else {
if (!button->irq) {
@@ -696,10 +699,15 @@ static void gpio_keys_close(struct input_dev *input)
device_property_read_string(dev, "label", &pdata->name);
device_for_each_child_node(dev, child) {
- if (is_of_node(child))
+ if (is_of_node(child)) {
button->irq =
irq_of_parse_and_map(to_of_node(child), 0);
+ if (button->irq)
+ button->trigger_type =
+ irq_get_trigger_type(button->irq);
+ }
+
if (fwnode_property_read_u32(child, "linux,code",
&button->code)) {
dev_err(dev, "Button without keycode\n");
diff --git a/include/linux/gpio_keys.h b/include/linux/gpio_keys.h
index d06bf77..eae87e2 100644
--- a/include/linux/gpio_keys.h
+++ b/include/linux/gpio_keys.h
@@ -18,6 +18,7 @@
* disable button via sysfs
* @value: axis value for %EV_ABS
* @irq: Irq number in case of interrupt keys
+ * @trigger_type: indicate the button's interrupt type
*/
struct gpio_keys_button {
unsigned int code;
@@ -30,6 +31,7 @@ struct gpio_keys_button {
bool can_disable;
int value;
unsigned int irq;
+ unsigned int trigger_type;
};
/**
--
1.7.9.5
^ permalink raw reply related
* Re: [PATCH v3] Input: gpio_keys: Add level trigger support for GPIO keys
From: Baolin Wang @ 2018-03-01 7:35 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Dmitry Torokhov, Rob Herring, Mark Rutland, gregkh, stephen lu,
Arvind Yadav, Joseph Lo, Kate Stewart, Philippe Ombredanne,
Thomas Gleixner, open list:HID CORE LAYER, DTML,
Linux Kernel Mailing List, Mark Brown, Linus Walleij
In-Reply-To: <CAK8P3a1v2GanneaFKpXWjWLt1YzibJ9Bh7vUcd_SH-4jGN_1+A@mail.gmail.com>
On 28 February 2018 at 22:44, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wed, Feb 28, 2018 at 1:44 PM, Baolin Wang <baolin.wang@linaro.org> wrote:
>> On some platforms (such as Spreadtrum platform), the GPIO keys can only
>> be triggered by level type. So this patch introduces one property to
>> indicate if the GPIO trigger type is level trigger or edge trigger.
>>
>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>> ---
>> Changes since v2:
>> - Use 'interrupt' property to indicate the irq type.
>>
>> Changes since v1:
>> - Diable the GPIO irq until reversing the GPIO level type.
>
> I've looked at your patch in more detail now, and given it a bit more thought.
>
> I wonder if you could move that logic into your gpiochip/irqchip driver instead.
> It seems that what you do in the gpio-keys driver is to emulate edge triggered
> behavior on a level triggered irqchip.
>
> If you put the same logic into the gpio driver, you could simply make it
> pretend to support an edge trigger on both edges and call into the interrupt
> handler whenever the state changes.
>
That is really a good suggestion, which can avoid duplicate level
reverse logic in different drivers. So this patch can be simplified
just adding one trigger_type to indicate the interrupt type (not
always edge trigger). Thanks for your suggestion.
--
Baolin.wang
Best Regards
^ permalink raw reply
* Re: [PATCH v4 2/4] HID: steam: add serial number information.
From: Rodrigo Rivas Costa @ 2018-02-28 23:12 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Jiri Kosina, Benjamin Tissoires, Pierre-Loup A. Griffais,
Cameron Gutman, Clément VUCHENER, Linux Kernel Mailing List,
linux-input
In-Reply-To: <CAHp75VfxaxLJWK1Hk9PRiU0L2MuLPX5VoPsn6RRmmXg-apXo6Q@mail.gmail.com>
On Wed, Feb 28, 2018 at 10:17:50PM +0200, Andy Shevchenko wrote:
> On Wed, Feb 28, 2018 at 8:43 PM, Rodrigo Rivas Costa
> <rodrigorivascosta@gmail.com> wrote:
> > This device has a feature report to send and receive commands.
> > Use it to get the serial number and set the device's uniq value.
>
> > #include <linux/module.h>
> > #include <linux/workqueue.h>
> > #include <linux/rcupdate.h>
>
> > +#include <linux/delay.h>
>
> Better to keep it somehow sorted (yes, I see it's not originally, but
> better to squeeze new header to the most ordered part).
Do you mean alphabetically? Or by topic/submodule? I just added it to
the end of the include list.
>
>
> > @@ -41,8 +42,99 @@ struct steam_device {
> > unsigned long quirks;
> > struct work_struct work_connect;
> > bool connected;
>
> > + char serial_no[11];
>
> 11 is a magic.
Magic indeed, it is 10 because the Valve protocol says so, and +1 for
the NUL. I'll add a #define for that 10.
>
> > };
> >
> > +static int steam_recv_report(struct steam_device *steam,
> > + u8 *data, int size)
> > +{
> > + struct hid_report *r;
> > + u8 *buf;
> > + int ret;
> > +
> > + r = steam->hdev->report_enum[HID_FEATURE_REPORT].report_id_hash[0];
> > + if (hid_report_len(r) < 64)
> > + return -EINVAL;
>
> + empty line.
Ok.
>
> > + buf = hid_alloc_report_buf(r, GFP_KERNEL);
> > + if (!buf)
> > + return -ENOMEM;
> > +
> > + /*
> > + * The report ID is always 0, so strip the first byte from the output.
> > + * hid_report_len() is not counting the report ID, so +1 to the length
> > + * or else we get a EOVERFLOW. We are safe from a buffer overflow
> > + * because hid_alloc_report_buf() allocates +7 bytes.
> > + */
> > + ret = hid_hw_raw_request(steam->hdev, 0x00,
> > + buf, hid_report_len(r) + 1,
> > + HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
> > + if (ret > 0)
> > + memcpy(data, buf + 1, min(size, ret - 1));
> > + kfree(buf);
> > + return ret;
> > +}
> > +
> > +static int steam_send_report(struct steam_device *steam,
> > + u8 *cmd, int size)
> > +{
> > + struct hid_report *r;
> > + u8 *buf;
> > + int retry;
> > + int ret;
> > +
> > + r = steam->hdev->report_enum[HID_FEATURE_REPORT].report_id_hash[0];
> > + if (hid_report_len(r) < 64)
> > + return -EINVAL;
>
> +empty line.
Ok.
>
> > + buf = hid_alloc_report_buf(r, GFP_KERNEL);
> > + if (!buf)
> > + return -ENOMEM;
> > +
> > + /* The report ID is always 0 */
> > + memcpy(buf + 1, cmd, size);
> > +
> > + /*
> > + * Sometimes the wireless controller fails with EPIPE
> > + * when sending a feature report.
> > + * Doing a HID_REQ_GET_REPORT and waiting for a while
> > + * seems to fix that.
> > + */
>
> > + for (retry = 0; retry < 10; ++retry) {
> > + ret = hid_hw_raw_request(steam->hdev, 0,
> > + buf, size + 1,
> > + HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
> > + if (ret != -EPIPE)
> > + break;
> > + steam_recv_report(steam, NULL, 0);
> > + msleep(50);
> > + }
>
> Personally I consider do{}while in case of "timeout loops" much easier to parse.
>
> unsigned int retry = 10;
> ...
>
> do {
> ...
> } while (--retry);
>
Ok, it looks like it is done this way in most places. Also renamed to 'retries'.
> > + kfree(buf);
> > + if (ret < 0)
> > + hid_err(steam->hdev, "%s: error %d (%*ph)\n", __func__,
> > + ret, size, cmd);
> > + return ret;
> > +}
> > +
> > +static int steam_get_serial(struct steam_device *steam)
> > +{
> > + /*
> > + * Send: 0xae 0x15 0x01
> > + * Recv: 0xae 0x15 0x01 serialnumber (10 chars)
> > + */
> > + int ret;
> > + u8 cmd[] = {0xae, 0x15, 0x01};
>
> > + u8 reply[14];
> > +
> > + ret = steam_send_report(steam, cmd, sizeof(cmd));
> > + if (ret < 0)
> > + return ret;
> > + ret = steam_recv_report(steam, reply, sizeof(reply));
> > + if (ret < 0)
> > + return ret;
>
>
> > + reply[13] = 0;
> > + strcpy(steam->serial_no, reply + 3);
>
> strlcpy()
Well, I've set a NUL byte at the end so the overflow is impossible.
I'll change it anyway, for extra safety.
>
> > + return 0;
> > +}
>
>
> --
> With Best Regards,
> Andy Shevchenko
Regards.
Rodrigo
^ permalink raw reply
* Re: [PATCH v4 1/4] HID: add driver for Valve Steam Controller
From: Rodrigo Rivas Costa @ 2018-02-28 22:49 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Jiri Kosina, Benjamin Tissoires, Pierre-Loup A. Griffais,
Cameron Gutman, Clément VUCHENER, Linux Kernel Mailing List,
linux-input
In-Reply-To: <CAHp75VfPK=yAx9TLEX-W0SrK8xXQzrP4wAuFWS3zEjHGoPJRsw@mail.gmail.com>
On Wed, Feb 28, 2018 at 09:21:15PM +0200, Andy Shevchenko wrote:
> On Wed, Feb 28, 2018 at 8:43 PM, Rodrigo Rivas Costa
> <rodrigorivascosta@gmail.com> wrote:
> > There are two ways to connect the Steam Controller: directly to the USB
> > or with the USB wireless adapter. Both methods are similar, but the
> > wireless adapter can connect up to 4 devices at the same time.
> >
> > The wired device will appear as 3 interfaces: a virtual mouse, a virtual
> > keyboard and a custom HID device.
> >
> > The wireless device will appear as 5 interfaces: a virtual keyboard and
> > 4 custom HID devices, that will remain silent until a device is actually
> > connected.
> >
> > The custom HID device has a report descriptor with all vendor specific
> > usages, so the hid-generic is not very useful. In a PC/SteamBox Valve
> > Steam Client provices a software translation by using direct USB access
> > and a creates a uinput virtual gamepad.
> >
> > This driver was reverse engineered to provide direct kernel support in
> > case you cannot, or do not want to, use Valve Steam Client. It disables
> > the virtual keyboard and mouse, as they are not so useful when you have
> > a working gamepad.
>
>
> > +// SPDX-License-Identifier: GPL-2.0
>
> > +MODULE_LICENSE("GPL");
>
> Not the same.
Hmmm... I copied from usb-skeleton.c, IIRC...
I'll change to GPL-2.0+, that would be correct, I think.
>
> > +static void steam_unregister(struct steam_device *steam)
> > +{
> > + struct input_dev *input;
> > +
> > + rcu_read_lock();
> > + input = rcu_dereference(steam->input);
> > + rcu_read_unlock();
> > +
>
> > + if (input) {
>
> if (!input)
> return;
>
> ?
That was because of symmetry, because further patches add more objects.
Then
if (input)
free(input);
if (battery)
free(battery);
/* in the future *(
if (input_gyro)
free(input_gyro);
Sure, the last one can do an early return, but you break symmetry.
>
> > + RCU_INIT_POINTER(steam->input, NULL);
> > + synchronize_rcu();
> > + hid_info(steam->hdev, "Steam Controller disconnected");
> > + input_unregister_device(input);
> > + }
> > +}
>
> > +static bool steam_is_valve_interface(struct hid_device *hdev)
> > +{
> > + struct hid_report_enum *rep_enum;
> > + struct hid_report *hreport;
> > +
> > + /*
> > + * The wired device creates 3 interfaces:
> > + * 0: emulated mouse.
> > + * 1: emulated keyboard.
> > + * 2: the real game pad.
> > + * The wireless device creates 5 interfaces:
> > + * 0: emulated keyboard.
> > + * 1-4: slots where up to 4 real game pads will be connected to.
> > + * We know which one is the real gamepad interface because they are the
> > + * only ones with a feature report.
> > + */
> > + rep_enum = &hdev->report_enum[HID_FEATURE_REPORT];
>
> > + list_for_each_entry(hreport, &rep_enum->report_list, list) {
> > + /* should we check hreport->id == 0? */
> > + return true;
> > + }
> > + return false;
>
> So, for now it's just an equivalent of
>
> return !list_empty();
>
> ?
I was expecting to add a few more checks in the middle, but those
weren't needed at the end.
I'll change that.
>
> > +}
>
> > + /*
> > + * From this point on, if anything fails return 0 and ignores
> > + * the error, so that the default HID devices are still bound.
> > + */
> > + steam = devm_kzalloc(&hdev->dev,
> > + sizeof(struct steam_device), GFP_KERNEL);
>
> sizeof(*steam) saves a line.
Right, changed.
>
> > + if (!steam) {
> > + ret = -ENOMEM;
> > + goto mem_fail;
> > + }
>
> > +static void steam_remove(struct hid_device *hdev)
> > +{
> > + struct steam_device *steam = hid_get_drvdata(hdev);
> > +
> > + if (steam && (steam->quirks & STEAM_QUIRK_WIRELESS)) {
> > + hid_info(hdev, "Steam wireless receiver disconnected");
> > + hid_hw_close(hdev);
> > + }
> > +
> > + hid_hw_stop(hdev);
> > +
> > + if (steam) {
> > + cancel_work_sync(&steam->work_connect);
> > + steam_unregister(steam);
>
> > + hid_set_drvdata(hdev, NULL);
>
> Hmm.. Doesn't HID do this?
Do you mean the hid_set_drvdata(hdev, NULL)? I'm not sure, I didn't see
that on hid-core.c or hid-generic.c. And a call like this is done in
many modules... so I did the same, just to be sure.
>
> > + }
>
> if (steam) {
> ...
> hid_hw_stop(hdev);
> ...
> } else {
> hid_hw_stop(hdev);
> }
>
> ?
I have no real preference. Your version has two 'if', mine has two 'if'.
What about:
static void steam_remove(struct hid_device *hdev)
{
struct steam_device *steam = hid_get_drvdata(hdev);
if (!steam) {
hid_hw_stop(hdev);
return;
}
if (steam->quirks & STEAM_QUIRK_WIRELESS) {
hid_info(hdev, "Steam wireless receiver disconnected");
hid_hw_close(hdev);
}
hid_hw_stop(hdev);
cancel_work_sync(&steam->work_connect);
steam_unregister(steam);
hid_set_drvdata(hdev, NULL);
}
>
> > +}
>
> > +static void steam_do_input_event(struct steam_device *steam,
> > + struct input_dev *input, u8 *data)
> > +{
> > + /* 24 bits of buttons */
> > + u8 b8, b9, b10;
> > + bool lpad_touched, lpad_and_joy;
> > +
> > + b8 = data[8];
> > + b9 = data[9];
> > + b10 = data[10];
> > +
> > + input_report_abs(input, ABS_Z, data[11]);
> > + input_report_abs(input, ABS_RZ, data[12]);
> > +
> > + /*
> > + * These two bits tells how to interpret the values X and Y.
> > + * lpad_and_joy tells that the joystick and the lpad are used at the
> > + * same time.
> > + * lpad_touched tells whether X/Y are to be read as lpad coord or
> > + * joystick values.
> > + * (lpad_touched || lpad_and_joy) tells if the lpad is really touched.
> > + */
>
> > + lpad_touched = b10 & 0x08;
>
> BIT(3) ?
>
> > + lpad_and_joy = b10 & 0x80;
>
> BIT(7) ?
>
> > + input_event(input, EV_KEY, BTN_TR2, !!(b8 & 0x01));
> > + input_event(input, EV_KEY, BTN_TL2, !!(b8 & 0x02));
> > + input_event(input, EV_KEY, BTN_TR, !!(b8 & 0x04));
> > + input_event(input, EV_KEY, BTN_TL, !!(b8 & 0x08));
> > + input_event(input, EV_KEY, BTN_Y, !!(b8 & 0x10));
> > + input_event(input, EV_KEY, BTN_B, !!(b8 & 0x20));
> > + input_event(input, EV_KEY, BTN_X, !!(b8 & 0x40));
> > + input_event(input, EV_KEY, BTN_A, !!(b8 & 0x80));
> > + input_event(input, EV_KEY, BTN_SELECT, !!(b9 & 0x10));
> > + input_event(input, EV_KEY, BTN_MODE, !!(b9 & 0x20));
> > + input_event(input, EV_KEY, BTN_START, !!(b9 & 0x40));
> > + input_event(input, EV_KEY, BTN_GEAR_DOWN, !!(b9 & 0x80));
> > + input_event(input, EV_KEY, BTN_GEAR_UP, !!(b10 & 0x01));
> > + input_event(input, EV_KEY, BTN_THUMBR, !!(b10 & 0x04));
> > + input_event(input, EV_KEY, BTN_THUMBL, !!(b10 & 0x40));
> > + input_event(input, EV_KEY, BTN_THUMB, lpad_touched || lpad_and_joy);
> > + input_event(input, EV_KEY, BTN_THUMB2, !!(b10 & 0x10));
>
> BIT(x) ?
>
> > +
> > + input_report_abs(input, ABS_HAT0X,
> > + !!(b9 & 0x02) - !!(b9 & 0x04));
> > + input_report_abs(input, ABS_HAT0Y,
> > + !!(b9 & 0x08) - !!(b9 & 0x01));
>
> BIT(x) ?
That's certainly nicer. Changed.
>
> > +}
>
> > +static int steam_raw_event(struct hid_device *hdev,
> > + struct hid_report *report, u8 *data,
> > + int size)
> > +{
> > + struct steam_device *steam = hid_get_drvdata(hdev);
> > + struct input_dev *input;
> > +
>
> > + if (!steam)
> > + return 0;
>
> When it's possible?
That's because how the new automatic unbinding/rebinding of hid-generic
works. This driver binds all the interfaces of the device, including the
virtual mouse and keyboard (!steam_is_valve_interface()). Those
hid_devices do not have a steam_device, but they still generate
raw_events.
>
> > + return 0;
> > +}
>
> --
> With Best Regards,
> Andy Shevchenko
Regards.
Rodrigo.
^ permalink raw reply
* Re: [RFC 3/4] hwmon: add Gateworks System Controller support
From: Guenter Roeck @ 2018-02-28 22:36 UTC (permalink / raw)
To: Tim Harvey
Cc: Lee Jones, Rob Herring, Mark Rutland, Mark Brown, Dmitry Torokhov,
linux-kernel, devicetree, linux-arm-kernel, linux-hwmon,
linux-input
In-Reply-To: <CAJ+vNU3U85mwv9_U5O5Xq3o=D7k_63Dwy4xf+bbLiink7pyapQ@mail.gmail.com>
On Wed, Feb 28, 2018 at 01:44:36PM -0800, Tim Harvey wrote:
> On Tue, Feb 27, 2018 at 6:05 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> > On 02/27/2018 05:21 PM, Tim Harvey wrote:
> >>
> >> The Gateworks System Controller has a hwmon sub-component that exposes
> >> up to 16 ADC's, some of which are temperature sensors, others which are
> >> voltage inputs. The ADC configuration (register mapping and name) is
> >> configured via device-tree and varies board to board.
> >>
> >> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> >> ---
> >> drivers/hwmon/Kconfig | 6 +
> >> drivers/hwmon/Makefile | 1 +
> >> drivers/hwmon/gsc-hwmon.c | 299
> >> ++++++++++++++++++++++++++++++++++++++++++++++
> >> 3 files changed, 306 insertions(+)
> >> create mode 100644 drivers/hwmon/gsc-hwmon.c
> >>
> >> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> >> index 7ad0176..9cdc3cb 100644
> >> --- a/drivers/hwmon/Kconfig
> >> +++ b/drivers/hwmon/Kconfig
> >> @@ -475,6 +475,12 @@ config SENSORS_F75375S
> >> This driver can also be built as a module. If so, the module
> >> will be called f75375s.
> >> +config SENSORS_GSC
> >> + tristate "Gateworks System Controller ADC"
> >> + depends on MFD_GSC
> >> + help
> >> + Support for the Gateworks System Controller A/D converters.
> >> +
> >> config SENSORS_MC13783_ADC
> >> tristate "Freescale MC13783/MC13892 ADC"
> >> depends on MFD_MC13XXX
> >> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> >> index 0fe489f..835a536 100644
> >> --- a/drivers/hwmon/Makefile
> >> +++ b/drivers/hwmon/Makefile
> >> @@ -69,6 +69,7 @@ obj-$(CONFIG_SENSORS_G760A) += g760a.o
> >> obj-$(CONFIG_SENSORS_G762) += g762.o
> >> obj-$(CONFIG_SENSORS_GL518SM) += gl518sm.o
> >> obj-$(CONFIG_SENSORS_GL520SM) += gl520sm.o
> >> +obj-$(CONFIG_SENSORS_GSC) += gsc-hwmon.o
> >> obj-$(CONFIG_SENSORS_GPIO_FAN) += gpio-fan.o
> >> obj-$(CONFIG_SENSORS_HIH6130) += hih6130.o
> >> obj-$(CONFIG_SENSORS_ULTRA45) += ultra45_env.o
> >> diff --git a/drivers/hwmon/gsc-hwmon.c b/drivers/hwmon/gsc-hwmon.c
> >> new file mode 100644
> >> index 0000000..3e14bea
> >> --- /dev/null
> >> +++ b/drivers/hwmon/gsc-hwmon.c
> >> @@ -0,0 +1,299 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * Copyright (C) 2018 Gateworks Corporation
> >> + */
> >> +#define DEBUG
>
> Guenter,
>
> Thanks for the review!
>
> >
> >
> > Please no.
>
> oops - left that in by mistake.
>
> >
> >> +
> >> +#include <linux/hwmon.h>
> >> +#include <linux/hwmon-sysfs.h>
> >> +#include <linux/mfd/gsc.h>
> >> +#include <linux/module.h>
> >> +#include <linux/of.h>
> >> +#include <linux/platform_device.h>
> >> +#include <linux/regmap.h>
> >> +#include <linux/slab.h>
> >> +
> >> +/* map channel to channel info */
> >> +struct gsc_hwmon_ch {
> >> + u8 reg;
> >> + char name[32];
> >> +};
> >> +static struct gsc_hwmon_ch gsc_temp_ch[16];
> >
> >
> > 16 temperature channels ...
>
> It has 16x ADC channels where some can be temperatures and others can
> be voltage inputs (based on device tree).
>
That was just to point out that you use smaller arrays later on.
> >
> >
> >> +static struct gsc_hwmon_ch gsc_in_ch[16];
> >> +static struct gsc_hwmon_ch gsc_fan_ch[5];
> >> +
> >> +static int
> >> +gsc_hwmon_read(struct device *dev, enum hwmon_sensor_types type, u32
> >> attr,
> >> + int ch, long *val)
> >> +{
> >> + struct gsc_dev *gsc = dev_get_drvdata(dev);
> >> + int sz, ret;
> >> + u8 reg;
> >> + u8 buf[3];
> >> +
> >> + dev_dbg(dev, "%s type=%d attr=%d ch=%d\n", __func__, type, attr,
> >> ch);
> >> + switch (type) {
> >> + case hwmon_in:
> >> + sz = 3;
> >> + reg = gsc_in_ch[ch].reg;
> >> + break;
> >> + case hwmon_temp:
> >> + sz = 2;
> >> + reg = gsc_temp_ch[ch].reg;
> >> + break;
> >> + default:
> >> + return -EOPNOTSUPP;
> >> + }
> >> +
> >> + ret = regmap_bulk_read(gsc->regmap_hwmon, reg, &buf, sz);
> >> + if (!ret) {
> >> + *val = 0;
> >> + while (sz-- > 0)
> >> + *val |= (buf[sz] << (8*sz));
> >> + if ((type == hwmon_temp) && *val > 0x8000)
> >
> >
> > Excessive [and inconsistent] ( )
> >
> > Please make this
> > if (ret)
> > return ret;
> > ...
> > return 0;
>
> understood - a much cleaner pattern
>
> >
> >
> >> + *val -= 0xffff;
> >> + }
> >> +
> >> + return ret;
> >> +}
> >> +
> >> +static int
> >> +gsc_hwmon_read_string(struct device *dev, enum hwmon_sensor_types type,
> >> + u32 attr, int ch, const char **buf)
> >> +{
> >> + dev_dbg(dev, "%s type=%d attr=%d ch=%d\n", __func__, type, attr,
> >> ch);
> >> + switch (type) {
> >> + case hwmon_in:
> >> + case hwmon_temp:
> >> + case hwmon_fan:
> >> + switch (attr) {
> >> + case hwmon_in_label:
> >> + *buf = gsc_in_ch[ch].name;
> >> + return 0;
> >> + break;
> >> + case hwmon_temp_label:
> >> + *buf = gsc_temp_ch[ch].name;
> >> + return 0;
> >> + break;
> >> + case hwmon_fan_label:
> >> + *buf = gsc_fan_ch[ch].name;
> >> + return 0;
> >> + break;
> >
> >
> > return followed by break doesn't make sense.
>
> right - removed
>
> >
> >
> >> + default:
> >> + break;
> >> + }
> >> + break;
> >> + default:
> >> + break;
> >> + }
> >> +
> >> + return -ENOTSUPP;
> >> +}
> >> +
> >> +static int
> >> +gsc_hwmon_write(struct device *dev, enum hwmon_sensor_types type, u32
> >> attr,
> >> + int ch, long val)
> >> +{
> >> + struct gsc_dev *gsc = dev_get_drvdata(dev);
> >> + int ret;
> >> + u8 reg;
> >> + u8 buf[3];
> >> +
> >> + dev_dbg(dev, "%s type=%d attr=%d ch=%d\n", __func__, type, attr,
> >> ch);
> >> + switch (type) {
> >> + case hwmon_fan:
> >> + buf[0] = val & 0xff;
> >> + buf[1] = (val >> 8) & 0xff;
> >> + reg = gsc_fan_ch[ch].reg;
> >> + ret = regmap_bulk_write(gsc->regmap_hwmon, reg, &buf, 2);
> >
> >
> > &buf -> buf
>
> yikes - thanks for catching that
>
> >
> >> + break;
> >> + default:
> >> + ret = -EOPNOTSUPP;
> >> + break;
> >> + }
> >> +
> >> + return ret;
> >> +}
> >> +
> >> +static umode_t
> >> +gsc_hwmon_is_visible(const void *_data, enum hwmon_sensor_types type, u32
> >> attr,
> >> + int ch)
> >> +{
> >> + const struct gsc_dev *gsc = _data;
> >> + struct device *dev = gsc->dev;
> >> + umode_t mode = 0;
> >> +
> >> + switch (type) {
> >> + case hwmon_fan:
> >> + if (attr == hwmon_fan_input)
> >> + mode = (S_IRUGO | S_IWUSR);
> >
> >
> > Unnecessary ( )
>
> ok
>
> >
> >
> >> + break;
> >> + case hwmon_temp:
> >> + case hwmon_in:
> >> + mode = S_IRUGO;
> >> + break;
> >> + default:
> >> + break;
> >> + }
> >> + dev_dbg(dev, "%s type=%d attr=%d ch=%d mode=0x%x\n", __func__,
> >> type,
> >> + attr, ch, mode);
> >> +
> >> + return mode;
> >> +}
> >> +
> >> +static u32 gsc_in_config[] = {
> >> + HWMON_I_INPUT,
> >> + HWMON_I_INPUT,
> >> + HWMON_I_INPUT,
> >> + HWMON_I_INPUT,
> >> + HWMON_I_INPUT,
> >> + HWMON_I_INPUT,
> >> + HWMON_I_INPUT,
> >> + HWMON_I_INPUT,
> >> + HWMON_I_INPUT,
> >> + HWMON_I_INPUT,
> >> + HWMON_I_INPUT,
> >> + HWMON_I_INPUT,
> >> + HWMON_I_INPUT,
> >> + HWMON_I_INPUT,
> >> + HWMON_I_INPUT,
> >> + HWMON_I_INPUT,
> >> + 0
> >> +};
> >> +static const struct hwmon_channel_info gsc_in = {
> >> + .type = hwmon_in,
> >> + .config = gsc_in_config,
> >> +};
> >> +
> >> +static u32 gsc_temp_config[] = {
> >> + HWMON_T_INPUT,
> >> + HWMON_T_INPUT,
> >> + HWMON_T_INPUT,
> >> + HWMON_T_INPUT,
> >> + HWMON_T_INPUT,
> >> + HWMON_T_INPUT,
> >> + HWMON_T_INPUT,
> >> + HWMON_T_INPUT,
> >> + 0
> >
> >
> > ... but this array only has 8+1 elements. This seems inconsistent.
> > How about using some defines for array sizes ?
> >
> > Also, why initialize those arrays ? You are overwriting them below.
> > You could just use a static size array instead.
> >
> > I assume it is guaranteed that there is only exactly one instance
> > of this device in the system. Have you tried what happens if you
> > declare two instances anyway ? The result must be interesting,
> > with all those static variables.
>
> yes, that static arrays are not very forward-thinking and yes my
> arrays are not consistent. I'll convert to dynamically allocating the
> channels for v2
>
> >
> >> +};
> >> +static const struct hwmon_channel_info gsc_temp = {
> >> + .type = hwmon_temp,
> >> + .config = gsc_temp_config,
> >> +};
> >> +
> >> +static u32 gsc_fan_config[] = {
> >> + HWMON_F_INPUT,
> >> + HWMON_F_INPUT,
> >> + HWMON_F_INPUT,
> >> + HWMON_F_INPUT,
> >> + HWMON_F_INPUT,
> >> + HWMON_F_INPUT,
> >> + 0,
> >> +};
> >
> >
> > The matching gsc_fan_ch has only 5 entries.
>
> right - certainly an issue
>
> >
> >
> >> +static const struct hwmon_channel_info gsc_fan = {
> >> + .type = hwmon_fan,
> >> + .config = gsc_fan_config,
> >> +};
> >> +
> >> +static const struct hwmon_channel_info *gsc_info[] = {
> >> + &gsc_temp,
> >> + &gsc_in,
> >> + &gsc_fan,
> >> + NULL
> >> +};
> >> +
> >> +static const struct hwmon_ops gsc_hwmon_ops = {
> >> + .is_visible = gsc_hwmon_is_visible,
> >> + .read = gsc_hwmon_read,
> >> + .read_string = gsc_hwmon_read_string,
> >> + .write = gsc_hwmon_write,
> >> +};
> >> +
> >> +static const struct hwmon_chip_info gsc_chip_info = {
> >> + .ops = &gsc_hwmon_ops,
> >> + .info = gsc_info,
> >> +};
> >> +
> >> +static int gsc_hwmon_probe(struct platform_device *pdev)
> >> +{
> >> + struct gsc_dev *gsc = dev_get_drvdata(pdev->dev.parent);
> >> + struct device_node *np;
> >> + struct device *hwmon;
> >> + int temp_count = 0;
> >> + int in_count = 0;
> >> + int fan_count = 0;
> >> + int ret;
> >> +
> >> + dev_dbg(&pdev->dev, "%s\n", __func__);
> >
> >
> > You declare local 'dev' variables all over the place, except here,
> > where it would actually be used multiple times.
> >
> > Please either declare one here as well, or drop all the others.
>
> will do
>
> >
> >> + np = of_get_next_child(pdev->dev.of_node, NULL);
> >> + while (np) {
> >> + u32 reg, type;
> >> + const char *label;
> >> +
> >> + of_property_read_u32(np, "reg", ®);
> >> + of_property_read_u32(np, "type", &type);
> >> + label = of_get_property(np, "label", NULL);
> >
> >
> > It must be interesting to see what happens if no 'label' property
> > is provided. Have you tried ? Also, no validation of 'reg' and 'type' ?
> > Are you sure ?
>
> will add validation
>
> >
> >> + switch(type) {
> >> + case 0: /* temperature sensor */
> >> + gsc_temp_config[temp_count] = HWMON_T_INPUT |
> >> + HWMON_T_LABEL;
> >> + gsc_temp_ch[temp_count].reg = reg;
> >> + strncpy(gsc_temp_ch[temp_count].name, label, 32);
> >
> >
> > This leaves the string unterminated if it is too long. Have you tested
> > what happens in this situation ? Consider using strlcpy instead.
> >
> > Also please use sizeof() instead of '32'.
>
> ok
>
> >
> >> + if (temp_count < ARRAY_SIZE(gsc_temp_config))
> >> + temp_count++;
> >
> >
> > I would suggest to abort with EINVAL if this happens. Otherwise the last
> > entry
> > is overwritten, which doesn't make much sense. Also, this accepts up to
> > ARRAY_SIZE()
> > entries, leaving no termination.
> >
> >> + break;
> >> + case 1: /* voltage sensor */
> >> + gsc_in_config[in_count] = HWMON_I_INPUT |
> >> + HWMON_I_LABEL;
> >> + gsc_in_ch[in_count].reg = reg;
> >
> >
> > So a reg value of 0xXXyy is auto-converted to 0xYY ?
>
> Do you mean stuffing a u32 into a u8?
>
That is what you do, isn't it ? So reg=0xffff and reg=0xfeff will both
map to 0xff. Or, in other word, the code happily accepts invalid values
and converts them into something else.
> >
> >> + strncpy(gsc_in_ch[in_count].name, label, 32);
> >> + if (in_count < ARRAY_SIZE(gsc_in_config))
> >> + in_count++;
> >> + break;
> >> + case 2: /* fan controller setpoint */
> >> + gsc_fan_config[fan_count] = HWMON_F_INPUT |
> >> + HWMON_F_LABEL;
> >> + gsc_fan_ch[fan_count].reg = reg;
> >
> >
> > It is going to be interesting to see what happens if there are more than
> > 5 such entries.
>
> will fix
>
> >
> >> + strncpy(gsc_fan_ch[fan_count].name, label, 32);
> >> + if (fan_count < ARRAY_SIZE(gsc_fan_config))
> >> + fan_count++;
> >> + break;
> >
> >
> > All other types are silently ignored ?
>
> will fix
>
> >
> >> + }
> >> + np = of_get_next_child(pdev->dev.of_node, np);
> >> + }
> >> + /* terminate list */
> >> + gsc_in_config[in_count] = 0;
> >> + gsc_temp_config[temp_count] = 0;
> >> + gsc_fan_config[fan_count] = 0;
> >> +
> >
> > I would suggest to move above code into a separate function.
>
> will do
>
> >
> >> + hwmon = devm_hwmon_device_register_with_info(&pdev->dev,
> >> + KBUILD_MODNAME, gsc,
> >> + &gsc_chip_info,
> >> NULL);
> >> + if (IS_ERR(hwmon)) {
> >> + ret = PTR_ERR(hwmon);
> >> + dev_err(&pdev->dev, "Unable to register hwmon device:
> >> %d\n",
> >> + ret);
> >> + return ret;
> >> + }
> >
> >
> > The error would be ENOMEM. Is it necessary to report that again ?
>
> could also return -EINVAL but not with the args I'm passing in so I'll
> change it to:
> return PTR_ERR_OR_ZERO(hwmon);
>
Much preferred.
Thanks,
Guenter
^ permalink raw reply
* Re: [RFC 3/4] hwmon: add Gateworks System Controller support
From: Tim Harvey @ 2018-02-28 21:44 UTC (permalink / raw)
To: Guenter Roeck
Cc: Lee Jones, Rob Herring, Mark Rutland, Mark Brown, Dmitry Torokhov,
linux-kernel, devicetree, linux-arm-kernel, linux-hwmon,
linux-input
In-Reply-To: <69ea679a-91a7-761d-4fa8-2ac4c351fd43@roeck-us.net>
On Tue, Feb 27, 2018 at 6:05 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> On 02/27/2018 05:21 PM, Tim Harvey wrote:
>>
>> The Gateworks System Controller has a hwmon sub-component that exposes
>> up to 16 ADC's, some of which are temperature sensors, others which are
>> voltage inputs. The ADC configuration (register mapping and name) is
>> configured via device-tree and varies board to board.
>>
>> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
>> ---
>> drivers/hwmon/Kconfig | 6 +
>> drivers/hwmon/Makefile | 1 +
>> drivers/hwmon/gsc-hwmon.c | 299
>> ++++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 306 insertions(+)
>> create mode 100644 drivers/hwmon/gsc-hwmon.c
>>
>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>> index 7ad0176..9cdc3cb 100644
>> --- a/drivers/hwmon/Kconfig
>> +++ b/drivers/hwmon/Kconfig
>> @@ -475,6 +475,12 @@ config SENSORS_F75375S
>> This driver can also be built as a module. If so, the module
>> will be called f75375s.
>> +config SENSORS_GSC
>> + tristate "Gateworks System Controller ADC"
>> + depends on MFD_GSC
>> + help
>> + Support for the Gateworks System Controller A/D converters.
>> +
>> config SENSORS_MC13783_ADC
>> tristate "Freescale MC13783/MC13892 ADC"
>> depends on MFD_MC13XXX
>> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
>> index 0fe489f..835a536 100644
>> --- a/drivers/hwmon/Makefile
>> +++ b/drivers/hwmon/Makefile
>> @@ -69,6 +69,7 @@ obj-$(CONFIG_SENSORS_G760A) += g760a.o
>> obj-$(CONFIG_SENSORS_G762) += g762.o
>> obj-$(CONFIG_SENSORS_GL518SM) += gl518sm.o
>> obj-$(CONFIG_SENSORS_GL520SM) += gl520sm.o
>> +obj-$(CONFIG_SENSORS_GSC) += gsc-hwmon.o
>> obj-$(CONFIG_SENSORS_GPIO_FAN) += gpio-fan.o
>> obj-$(CONFIG_SENSORS_HIH6130) += hih6130.o
>> obj-$(CONFIG_SENSORS_ULTRA45) += ultra45_env.o
>> diff --git a/drivers/hwmon/gsc-hwmon.c b/drivers/hwmon/gsc-hwmon.c
>> new file mode 100644
>> index 0000000..3e14bea
>> --- /dev/null
>> +++ b/drivers/hwmon/gsc-hwmon.c
>> @@ -0,0 +1,299 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2018 Gateworks Corporation
>> + */
>> +#define DEBUG
Guenter,
Thanks for the review!
>
>
> Please no.
oops - left that in by mistake.
>
>> +
>> +#include <linux/hwmon.h>
>> +#include <linux/hwmon-sysfs.h>
>> +#include <linux/mfd/gsc.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +#include <linux/slab.h>
>> +
>> +/* map channel to channel info */
>> +struct gsc_hwmon_ch {
>> + u8 reg;
>> + char name[32];
>> +};
>> +static struct gsc_hwmon_ch gsc_temp_ch[16];
>
>
> 16 temperature channels ...
It has 16x ADC channels where some can be temperatures and others can
be voltage inputs (based on device tree).
>
>
>> +static struct gsc_hwmon_ch gsc_in_ch[16];
>> +static struct gsc_hwmon_ch gsc_fan_ch[5];
>> +
>> +static int
>> +gsc_hwmon_read(struct device *dev, enum hwmon_sensor_types type, u32
>> attr,
>> + int ch, long *val)
>> +{
>> + struct gsc_dev *gsc = dev_get_drvdata(dev);
>> + int sz, ret;
>> + u8 reg;
>> + u8 buf[3];
>> +
>> + dev_dbg(dev, "%s type=%d attr=%d ch=%d\n", __func__, type, attr,
>> ch);
>> + switch (type) {
>> + case hwmon_in:
>> + sz = 3;
>> + reg = gsc_in_ch[ch].reg;
>> + break;
>> + case hwmon_temp:
>> + sz = 2;
>> + reg = gsc_temp_ch[ch].reg;
>> + break;
>> + default:
>> + return -EOPNOTSUPP;
>> + }
>> +
>> + ret = regmap_bulk_read(gsc->regmap_hwmon, reg, &buf, sz);
>> + if (!ret) {
>> + *val = 0;
>> + while (sz-- > 0)
>> + *val |= (buf[sz] << (8*sz));
>> + if ((type == hwmon_temp) && *val > 0x8000)
>
>
> Excessive [and inconsistent] ( )
>
> Please make this
> if (ret)
> return ret;
> ...
> return 0;
understood - a much cleaner pattern
>
>
>> + *val -= 0xffff;
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static int
>> +gsc_hwmon_read_string(struct device *dev, enum hwmon_sensor_types type,
>> + u32 attr, int ch, const char **buf)
>> +{
>> + dev_dbg(dev, "%s type=%d attr=%d ch=%d\n", __func__, type, attr,
>> ch);
>> + switch (type) {
>> + case hwmon_in:
>> + case hwmon_temp:
>> + case hwmon_fan:
>> + switch (attr) {
>> + case hwmon_in_label:
>> + *buf = gsc_in_ch[ch].name;
>> + return 0;
>> + break;
>> + case hwmon_temp_label:
>> + *buf = gsc_temp_ch[ch].name;
>> + return 0;
>> + break;
>> + case hwmon_fan_label:
>> + *buf = gsc_fan_ch[ch].name;
>> + return 0;
>> + break;
>
>
> return followed by break doesn't make sense.
right - removed
>
>
>> + default:
>> + break;
>> + }
>> + break;
>> + default:
>> + break;
>> + }
>> +
>> + return -ENOTSUPP;
>> +}
>> +
>> +static int
>> +gsc_hwmon_write(struct device *dev, enum hwmon_sensor_types type, u32
>> attr,
>> + int ch, long val)
>> +{
>> + struct gsc_dev *gsc = dev_get_drvdata(dev);
>> + int ret;
>> + u8 reg;
>> + u8 buf[3];
>> +
>> + dev_dbg(dev, "%s type=%d attr=%d ch=%d\n", __func__, type, attr,
>> ch);
>> + switch (type) {
>> + case hwmon_fan:
>> + buf[0] = val & 0xff;
>> + buf[1] = (val >> 8) & 0xff;
>> + reg = gsc_fan_ch[ch].reg;
>> + ret = regmap_bulk_write(gsc->regmap_hwmon, reg, &buf, 2);
>
>
> &buf -> buf
yikes - thanks for catching that
>
>> + break;
>> + default:
>> + ret = -EOPNOTSUPP;
>> + break;
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static umode_t
>> +gsc_hwmon_is_visible(const void *_data, enum hwmon_sensor_types type, u32
>> attr,
>> + int ch)
>> +{
>> + const struct gsc_dev *gsc = _data;
>> + struct device *dev = gsc->dev;
>> + umode_t mode = 0;
>> +
>> + switch (type) {
>> + case hwmon_fan:
>> + if (attr == hwmon_fan_input)
>> + mode = (S_IRUGO | S_IWUSR);
>
>
> Unnecessary ( )
ok
>
>
>> + break;
>> + case hwmon_temp:
>> + case hwmon_in:
>> + mode = S_IRUGO;
>> + break;
>> + default:
>> + break;
>> + }
>> + dev_dbg(dev, "%s type=%d attr=%d ch=%d mode=0x%x\n", __func__,
>> type,
>> + attr, ch, mode);
>> +
>> + return mode;
>> +}
>> +
>> +static u32 gsc_in_config[] = {
>> + HWMON_I_INPUT,
>> + HWMON_I_INPUT,
>> + HWMON_I_INPUT,
>> + HWMON_I_INPUT,
>> + HWMON_I_INPUT,
>> + HWMON_I_INPUT,
>> + HWMON_I_INPUT,
>> + HWMON_I_INPUT,
>> + HWMON_I_INPUT,
>> + HWMON_I_INPUT,
>> + HWMON_I_INPUT,
>> + HWMON_I_INPUT,
>> + HWMON_I_INPUT,
>> + HWMON_I_INPUT,
>> + HWMON_I_INPUT,
>> + HWMON_I_INPUT,
>> + 0
>> +};
>> +static const struct hwmon_channel_info gsc_in = {
>> + .type = hwmon_in,
>> + .config = gsc_in_config,
>> +};
>> +
>> +static u32 gsc_temp_config[] = {
>> + HWMON_T_INPUT,
>> + HWMON_T_INPUT,
>> + HWMON_T_INPUT,
>> + HWMON_T_INPUT,
>> + HWMON_T_INPUT,
>> + HWMON_T_INPUT,
>> + HWMON_T_INPUT,
>> + HWMON_T_INPUT,
>> + 0
>
>
> ... but this array only has 8+1 elements. This seems inconsistent.
> How about using some defines for array sizes ?
>
> Also, why initialize those arrays ? You are overwriting them below.
> You could just use a static size array instead.
>
> I assume it is guaranteed that there is only exactly one instance
> of this device in the system. Have you tried what happens if you
> declare two instances anyway ? The result must be interesting,
> with all those static variables.
yes, that static arrays are not very forward-thinking and yes my
arrays are not consistent. I'll convert to dynamically allocating the
channels for v2
>
>> +};
>> +static const struct hwmon_channel_info gsc_temp = {
>> + .type = hwmon_temp,
>> + .config = gsc_temp_config,
>> +};
>> +
>> +static u32 gsc_fan_config[] = {
>> + HWMON_F_INPUT,
>> + HWMON_F_INPUT,
>> + HWMON_F_INPUT,
>> + HWMON_F_INPUT,
>> + HWMON_F_INPUT,
>> + HWMON_F_INPUT,
>> + 0,
>> +};
>
>
> The matching gsc_fan_ch has only 5 entries.
right - certainly an issue
>
>
>> +static const struct hwmon_channel_info gsc_fan = {
>> + .type = hwmon_fan,
>> + .config = gsc_fan_config,
>> +};
>> +
>> +static const struct hwmon_channel_info *gsc_info[] = {
>> + &gsc_temp,
>> + &gsc_in,
>> + &gsc_fan,
>> + NULL
>> +};
>> +
>> +static const struct hwmon_ops gsc_hwmon_ops = {
>> + .is_visible = gsc_hwmon_is_visible,
>> + .read = gsc_hwmon_read,
>> + .read_string = gsc_hwmon_read_string,
>> + .write = gsc_hwmon_write,
>> +};
>> +
>> +static const struct hwmon_chip_info gsc_chip_info = {
>> + .ops = &gsc_hwmon_ops,
>> + .info = gsc_info,
>> +};
>> +
>> +static int gsc_hwmon_probe(struct platform_device *pdev)
>> +{
>> + struct gsc_dev *gsc = dev_get_drvdata(pdev->dev.parent);
>> + struct device_node *np;
>> + struct device *hwmon;
>> + int temp_count = 0;
>> + int in_count = 0;
>> + int fan_count = 0;
>> + int ret;
>> +
>> + dev_dbg(&pdev->dev, "%s\n", __func__);
>
>
> You declare local 'dev' variables all over the place, except here,
> where it would actually be used multiple times.
>
> Please either declare one here as well, or drop all the others.
will do
>
>> + np = of_get_next_child(pdev->dev.of_node, NULL);
>> + while (np) {
>> + u32 reg, type;
>> + const char *label;
>> +
>> + of_property_read_u32(np, "reg", ®);
>> + of_property_read_u32(np, "type", &type);
>> + label = of_get_property(np, "label", NULL);
>
>
> It must be interesting to see what happens if no 'label' property
> is provided. Have you tried ? Also, no validation of 'reg' and 'type' ?
> Are you sure ?
will add validation
>
>> + switch(type) {
>> + case 0: /* temperature sensor */
>> + gsc_temp_config[temp_count] = HWMON_T_INPUT |
>> + HWMON_T_LABEL;
>> + gsc_temp_ch[temp_count].reg = reg;
>> + strncpy(gsc_temp_ch[temp_count].name, label, 32);
>
>
> This leaves the string unterminated if it is too long. Have you tested
> what happens in this situation ? Consider using strlcpy instead.
>
> Also please use sizeof() instead of '32'.
ok
>
>> + if (temp_count < ARRAY_SIZE(gsc_temp_config))
>> + temp_count++;
>
>
> I would suggest to abort with EINVAL if this happens. Otherwise the last
> entry
> is overwritten, which doesn't make much sense. Also, this accepts up to
> ARRAY_SIZE()
> entries, leaving no termination.
>
>> + break;
>> + case 1: /* voltage sensor */
>> + gsc_in_config[in_count] = HWMON_I_INPUT |
>> + HWMON_I_LABEL;
>> + gsc_in_ch[in_count].reg = reg;
>
>
> So a reg value of 0xXXyy is auto-converted to 0xYY ?
Do you mean stuffing a u32 into a u8?
>
>> + strncpy(gsc_in_ch[in_count].name, label, 32);
>> + if (in_count < ARRAY_SIZE(gsc_in_config))
>> + in_count++;
>> + break;
>> + case 2: /* fan controller setpoint */
>> + gsc_fan_config[fan_count] = HWMON_F_INPUT |
>> + HWMON_F_LABEL;
>> + gsc_fan_ch[fan_count].reg = reg;
>
>
> It is going to be interesting to see what happens if there are more than
> 5 such entries.
will fix
>
>> + strncpy(gsc_fan_ch[fan_count].name, label, 32);
>> + if (fan_count < ARRAY_SIZE(gsc_fan_config))
>> + fan_count++;
>> + break;
>
>
> All other types are silently ignored ?
will fix
>
>> + }
>> + np = of_get_next_child(pdev->dev.of_node, np);
>> + }
>> + /* terminate list */
>> + gsc_in_config[in_count] = 0;
>> + gsc_temp_config[temp_count] = 0;
>> + gsc_fan_config[fan_count] = 0;
>> +
>
> I would suggest to move above code into a separate function.
will do
>
>> + hwmon = devm_hwmon_device_register_with_info(&pdev->dev,
>> + KBUILD_MODNAME, gsc,
>> + &gsc_chip_info,
>> NULL);
>> + if (IS_ERR(hwmon)) {
>> + ret = PTR_ERR(hwmon);
>> + dev_err(&pdev->dev, "Unable to register hwmon device:
>> %d\n",
>> + ret);
>> + return ret;
>> + }
>
>
> The error would be ENOMEM. Is it necessary to report that again ?
could also return -EINVAL but not with the args I'm passing in so I'll
change it to:
return PTR_ERR_OR_ZERO(hwmon);
Thanks!
Tim
^ permalink raw reply
* Re: [RFC 2/4] mfd: add Gateworks System Controller core driver
From: Tim Harvey @ 2018-02-28 21:16 UTC (permalink / raw)
To: Andrew Lunn
Cc: Lee Jones, Rob Herring, Mark Rutland, Mark Brown, Dmitry Torokhov,
linux-hwmon, devicetree, linux-kernel, linux-arm-kernel,
linux-input
In-Reply-To: <20180228185302.GD1203@lunn.ch>
On Wed, Feb 28, 2018 at 10:53 AM, Andrew Lunn <andrew@lunn.ch> wrote:
>> + dev_err(&client->dev, ">> 0x%02x %d\n", reg, ret);
>> + return ret;
>> + }
>> + dev_dbg(&client->dev, ">> 0x%02x=0x%02x (%d)\n", reg, val, retry);
>> +
>> + return 0;
>
> Hi Tim
>
> There appears to be a few spaces vs tabs issues in this file.
>
> Andrew
>
Thanks. I'll run through checkpatch prior to v2.
Tim
^ permalink raw reply
* Re: [RFC 2/4] mfd: add Gateworks System Controller core driver
From: Tim Harvey @ 2018-02-28 21:14 UTC (permalink / raw)
To: Randy Dunlap
Cc: Lee Jones, Rob Herring, Mark Rutland, Mark Brown, Dmitry Torokhov,
linux-kernel, devicetree, linux-arm-kernel, linux-hwmon,
linux-input
In-Reply-To: <170c83e1-8a34-5103-3767-139b22081cf9@infradead.org>
On Tue, Feb 27, 2018 at 6:00 PM, Randy Dunlap <rdunlap@infradead.org> wrote:
> On 02/27/2018 05:21 PM, Tim Harvey wrote:
>> The Gateworks System Controller (GSC) is an I2C slave controller
>> implemented with an MSP430 micro-controller whose firmware embeds the
>> following features:
>> - I/O expander (16 GPIO's) using PCA955x protocol
>> - Real Time Clock using DS1672 protocol
>> - User EEPROM using AT24 protocol
>> - HWMON using custom protocol
>> - Interrupt controller with tamper detect, user pushbotton
>> - Watchdog controller capable of full board power-cycle
>> - Power Control capable of full board power-cycle
>>
>> see http://trac.gateworks.com/wiki/gsc for more details
>>
>> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
>> ---
>> drivers/mfd/Kconfig | 10 ++
>> drivers/mfd/Makefile | 1 +
>> drivers/mfd/gsc.c | 330 ++++++++++++++++++++++++++++++++++++++++++++++++
>> include/linux/mfd/gsc.h | 79 ++++++++++++
>> 4 files changed, 420 insertions(+)
>> create mode 100644 drivers/mfd/gsc.c
>> create mode 100644 include/linux/mfd/gsc.h
>>
>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
>> index 1d20a80..16dd486 100644
>> --- a/drivers/mfd/Kconfig
>> +++ b/drivers/mfd/Kconfig
>> @@ -341,6 +341,16 @@ config MFD_EXYNOS_LPASS
>> Select this option to enable support for Samsung Exynos Low Power
>> Audio Subsystem.
>>
>> +config MFD_GSC
>> + tristate "Gateworks System Controller"
>> + depends on (I2C && OF) || COMPILE_TEST
>
> Do both I2C and OF have stubs so that a driver will build when they are
> both disabled? I.e., only COMPILE_TEST is enabled?
Randy,
No, at this point it requires both I2C and OF. I may add platform data
to support an older non-device-tree family of boards but it still
would require I2C.
I will remove the || COMPILE_TEST
Thanks for catching that.
Tim
^ permalink raw reply
* Re: [PATCH v4 2/4] HID: steam: add serial number information.
From: Andy Shevchenko @ 2018-02-28 20:17 UTC (permalink / raw)
To: Rodrigo Rivas Costa
Cc: Jiri Kosina, Benjamin Tissoires, Pierre-Loup A. Griffais,
Cameron Gutman, Clément VUCHENER, Linux Kernel Mailing List,
linux-input
In-Reply-To: <20180228184322.29636-3-rodrigorivascosta@gmail.com>
On Wed, Feb 28, 2018 at 8:43 PM, Rodrigo Rivas Costa
<rodrigorivascosta@gmail.com> wrote:
> This device has a feature report to send and receive commands.
> Use it to get the serial number and set the device's uniq value.
> #include <linux/module.h>
> #include <linux/workqueue.h>
> #include <linux/rcupdate.h>
> +#include <linux/delay.h>
Better to keep it somehow sorted (yes, I see it's not originally, but
better to squeeze new header to the most ordered part).
> @@ -41,8 +42,99 @@ struct steam_device {
> unsigned long quirks;
> struct work_struct work_connect;
> bool connected;
> + char serial_no[11];
11 is a magic.
> };
>
> +static int steam_recv_report(struct steam_device *steam,
> + u8 *data, int size)
> +{
> + struct hid_report *r;
> + u8 *buf;
> + int ret;
> +
> + r = steam->hdev->report_enum[HID_FEATURE_REPORT].report_id_hash[0];
> + if (hid_report_len(r) < 64)
> + return -EINVAL;
+ empty line.
> + buf = hid_alloc_report_buf(r, GFP_KERNEL);
> + if (!buf)
> + return -ENOMEM;
> +
> + /*
> + * The report ID is always 0, so strip the first byte from the output.
> + * hid_report_len() is not counting the report ID, so +1 to the length
> + * or else we get a EOVERFLOW. We are safe from a buffer overflow
> + * because hid_alloc_report_buf() allocates +7 bytes.
> + */
> + ret = hid_hw_raw_request(steam->hdev, 0x00,
> + buf, hid_report_len(r) + 1,
> + HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
> + if (ret > 0)
> + memcpy(data, buf + 1, min(size, ret - 1));
> + kfree(buf);
> + return ret;
> +}
> +
> +static int steam_send_report(struct steam_device *steam,
> + u8 *cmd, int size)
> +{
> + struct hid_report *r;
> + u8 *buf;
> + int retry;
> + int ret;
> +
> + r = steam->hdev->report_enum[HID_FEATURE_REPORT].report_id_hash[0];
> + if (hid_report_len(r) < 64)
> + return -EINVAL;
+empty line.
> + buf = hid_alloc_report_buf(r, GFP_KERNEL);
> + if (!buf)
> + return -ENOMEM;
> +
> + /* The report ID is always 0 */
> + memcpy(buf + 1, cmd, size);
> +
> + /*
> + * Sometimes the wireless controller fails with EPIPE
> + * when sending a feature report.
> + * Doing a HID_REQ_GET_REPORT and waiting for a while
> + * seems to fix that.
> + */
> + for (retry = 0; retry < 10; ++retry) {
> + ret = hid_hw_raw_request(steam->hdev, 0,
> + buf, size + 1,
> + HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
> + if (ret != -EPIPE)
> + break;
> + steam_recv_report(steam, NULL, 0);
> + msleep(50);
> + }
Personally I consider do{}while in case of "timeout loops" much easier to parse.
unsigned int retry = 10;
...
do {
...
} while (--retry);
> + kfree(buf);
> + if (ret < 0)
> + hid_err(steam->hdev, "%s: error %d (%*ph)\n", __func__,
> + ret, size, cmd);
> + return ret;
> +}
> +
> +static int steam_get_serial(struct steam_device *steam)
> +{
> + /*
> + * Send: 0xae 0x15 0x01
> + * Recv: 0xae 0x15 0x01 serialnumber (10 chars)
> + */
> + int ret;
> + u8 cmd[] = {0xae, 0x15, 0x01};
> + u8 reply[14];
> +
> + ret = steam_send_report(steam, cmd, sizeof(cmd));
> + if (ret < 0)
> + return ret;
> + ret = steam_recv_report(steam, reply, sizeof(reply));
> + if (ret < 0)
> + return ret;
> + reply[13] = 0;
> + strcpy(steam->serial_no, reply + 3);
strlcpy()
> + return 0;
> +}
--
With Best Regards,
Andy Shevchenko
^ 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