Re: [PATCH] [v4] Input: Add "Select" button to Microsoft Xbox One controller.

[Bastien Nocera]

On Tue, 2021-04-13 at 01:02 +, Chris Ye wrote:
> Add "Select" button input capability and input event mapping for
> Microsoft Xbox One controller. From product site this is also
> referred as
> "Share" button.
> Fixed Microsoft Xbox One controller select button not working under
> USB
> connection.
> 
> Signed-off-by: Chris Ye 
> ---
>  drivers/input/joystick/xpad.c | 8 +++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/input/joystick/xpad.c
> b/drivers/input/joystick/xpad.c
> index 9f0d07dcbf06..99cb8bb78570 100644
> --- a/drivers/input/joystick/xpad.c
> +++ b/drivers/input/joystick/xpad.c
> @@ -79,6 +79,7 @@
>  #define MAP_DPAD_TO_BUTTONS(1 << 0)
>  #define MAP_TRIGGERS_TO_BUTTONS(1 << 1)
>  #define MAP_STICKS_TO_NULL (1 << 2)
> +#define MAP_SELECT_BUTTON  (1 << 3)
>  #define DANCEPAD_MAP_CONFIG(MAP_DPAD_TO_BUTTONS
> |  \
> MAP_TRIGGERS_TO_BUTTONS |
> MAP_STICKS_TO_NULL)
>  
> @@ -130,6 +131,7 @@ static const struct xpad_device {
> { 0x045e, 0x02e3, "Microsoft X-Box One Elite pad", 0,
> XTYPE_XBOXONE },
> { 0x045e, 0x02ea, "Microsoft X-Box One S pad", 0,
> XTYPE_XBOXONE },
> { 0x045e, 0x0719, "Xbox 360 Wireless Receiver",
> MAP_DPAD_TO_BUTTONS, XTYPE_XBOX360W },
> +   { 0x045e, 0x0b12, "Microsoft Xbox One X pad",
> MAP_SELECT_BUTTON, XTYPE_XBOXONE },
> { 0x046d, 0xc21d, "Logitech Gamepad F310", 0, XTYPE_XBOX360
> },
> { 0x046d, 0xc21e, "Logitech Gamepad F510", 0, XTYPE_XBOX360
> },
> { 0x046d, 0xc21f, "Logitech Gamepad F710", 0, XTYPE_XBOX360
> },
> @@ -862,6 +864,8 @@ static void xpadone_process_packet(struct
> usb_xpad *xpad, u16 cmd, unsigned char
> /* menu/view buttons */
> input_report_key(dev, BTN_START,  data[4] & 0x04);
> input_report_key(dev, BTN_SELECT, data[4] & 0x08);
> +   if (xpad->mapping & MAP_SELECT_BUTTON)
> +   input_report_key(dev, KEY_RECORD, data[22] & 0x01);
>  
> /* buttons A,B,X,Y */
> input_report_key(dev, BTN_A,data[4] & 0x10);
> @@ -1669,9 +1673,11 @@ static int xpad_init_input(struct usb_xpad
> *xpad)
>  
> /* set up model-specific ones */
> if (xpad->xtype == XTYPE_XBOX360 || xpad->xtype ==
> XTYPE_XBOX360W ||
> -   xpad->xtype == XTYPE_XBOXONE) {
> +   xpad->xtype == XTYPE_XBOXONE) {

Why the indentation change here?

> for (i = 0; xpad360_btn[i] >= 0; i++)
> input_set_capability(input_dev, EV_KEY,
> xpad360_btn[i]);
> +   if (xpad->mapping & MAP_SELECT_BUTTON)
> +   input_set_capability(input_dev, EV_KEY,
> KEY_RECORD);
> } else {
> for (i = 0; xpad_btn[i] >= 0; i++)
> input_set_capability(input_dev, EV_KEY,
> xpad_btn[i]);

Re: [PATCH] [v2] Input: Add "Share" button to Microsoft Xbox One controller.

[Bastien Nocera]

On Thu, 2021-03-04 at 20:42 -0800, Roderick Colenbrander wrote:
> (resend in plain text)
> +benjamin in a more explicit way
> 
> Hi Chris,
> 
> I see the need for the Record button. I wonder what makes sense from
> the Linux kernel perspective. For DualShock 4 and DualSense there is a
> Share button too (it introduced it). For DualShock 4 that was mapped
> to 'Select' I think per Linux gamepad spec. For DualSense in 5.12 we
> did the same... Though if there is a true 'Record' button we want to
> use moving forward. Maybe we still want to change the button
> definition for DualSensein 5.12 while we can...
> 
> It would be good to get some consensus on these buttons.

The XBox One Series X controller has 3 of those "middle" buttons, 2
equivalent to Start/Select on older controllers (that were already
present on older versions of the pad) and Share.
https://www.xbox.com/en-US/accessories/controllers/xbox-wireless-controller

The Options buttons on the DS4 replaces Start and Select:
https://manuals.playstation.net/document/en/ps4/basic/pn_controller.html
so Share got added, but Select taken away, which is why you had to
label it Select in the driver.

I don't really see how to fix that without retroactively re-adding
buttons to the DS4 controller ;)

> 
> Thanks,
> Roderick
> 
> 
> On Thu, Mar 4, 2021 at 6:25 PM Chris Ye  wrote:
> > 
> > Hi Bastien,  just want to follow up again on this.  I've checked
> > again
> > with the Xbox team that the "Share button" is given for the product,
> > the HID usage profile and mapping to RECORD is what Xbox team expects
> > and they want the same mapping for USB.
> > 
> > Thanks!
> > Chris
> > 
> > 
> > On Tue, Mar 2, 2021 at 3:57 PM Chris Ye  wrote:
> > > 
> > > Hi Bastien,
> > >     The "Share button" is a name Microsoft calls it, it actually
> > > has
> > > HID descriptor defined in the bluetooth interface, which the HID
> > > usage
> > > is:
> > > consumer 0xB2:
> > > 0x05, 0x0C,    //   Usage Page (Consumer)
> > > 0x0A, 0xB2, 0x00,  //   Usage (Record)
> > > Microsoft wants the same key code to be generated consistently for
> > > USB
> > > and bluetooth.
> > > Thanks!
> > > Chris
> > > 
> > > 
> > > On Tue, Mar 2, 2021 at 1:50 AM Bastien Nocera 
> > > wrote:
> > > > 
> > > > On Thu, 2021-02-25 at 05:32 +, Chris Ye wrote:
> > > > > Add "Share" button input capability and input event mapping for
> > > > > Microsoft Xbox One controller.
> > > > > Fixed Microsoft Xbox One controller share button not working
> > > > > under USB
> > > > > connection.
> > > > > 
> > > > > Signed-off-by: Chris Ye 
> > > > > ---
> > > > >  drivers/input/joystick/xpad.c | 9 -
> > > > >  1 file changed, 8 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/input/joystick/xpad.c
> > > > > b/drivers/input/joystick/xpad.c
> > > > > index 9f0d07dcbf06..0c3374091aff 100644
> > > > > --- a/drivers/input/joystick/xpad.c
> > > > > +++ b/drivers/input/joystick/xpad.c
> > > > > @@ -79,6 +79,7 @@
> > > > >  #define MAP_DPAD_TO_BUTTONS    (1 << 0)
> > > > >  #define MAP_TRIGGERS_TO_BUTTONS    (1 << 1)
> > > > >  #define MAP_STICKS_TO_NULL (1 << 2)
> > > > > +#define MAP_SHARE_BUTTON   (1 << 3)
> > > > >  #define DANCEPAD_MAP_CONFIG    (MAP_DPAD_TO_BUTTONS
> > > > > >  \
> > > > >     MAP_TRIGGERS_TO_BUTTONS |
> > > > > MAP_STICKS_TO_NULL)
> > > > > 
> > > > > @@ -130,6 +131,7 @@ static const struct xpad_device {
> > > > >     { 0x045e, 0x02e3, "Microsoft X-Box One Elite pad", 0,
> > > > > XTYPE_XBOXONE },
> > > > >     { 0x045e, 0x02ea, "Microsoft X-Box One S pad", 0,
> > > > > XTYPE_XBOXONE
> > > > > },
> > > > >     { 0x045e, 0x0719, "Xbox 360 Wireless Receiver",
> > > > > MAP_DPAD_TO_BUTTONS, XTYPE_XBOX360W },
> > > > > +   { 0x045e, 0x0b12, "Microsoft X-Box One X pad",
> > > > > MAP_SHARE_BUTTON, XTYPE_XBOXONE },
> > > > >     { 0x046d, 0xc21d, "Logitech Gamepad F310", 0,
> &

Re: [PATCH] [v2] Input: Add "Share" button to Microsoft Xbox One controller.

[Bastien Nocera]

On Thu, 2021-02-25 at 05:32 +, Chris Ye wrote:
> Add "Share" button input capability and input event mapping for
> Microsoft Xbox One controller.
> Fixed Microsoft Xbox One controller share button not working under USB
> connection.
> 
> Signed-off-by: Chris Ye 
> ---
>  drivers/input/joystick/xpad.c | 9 -
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/input/joystick/xpad.c
> b/drivers/input/joystick/xpad.c
> index 9f0d07dcbf06..0c3374091aff 100644
> --- a/drivers/input/joystick/xpad.c
> +++ b/drivers/input/joystick/xpad.c
> @@ -79,6 +79,7 @@
>  #define MAP_DPAD_TO_BUTTONS(1 << 0)
>  #define MAP_TRIGGERS_TO_BUTTONS(1 << 1)
>  #define MAP_STICKS_TO_NULL (1 << 2)
> +#define MAP_SHARE_BUTTON   (1 << 3)
>  #define DANCEPAD_MAP_CONFIG(MAP_DPAD_TO_BUTTONS
> |  \
> MAP_TRIGGERS_TO_BUTTONS |
> MAP_STICKS_TO_NULL)
>  
> @@ -130,6 +131,7 @@ static const struct xpad_device {
> { 0x045e, 0x02e3, "Microsoft X-Box One Elite pad", 0,
> XTYPE_XBOXONE },
> { 0x045e, 0x02ea, "Microsoft X-Box One S pad", 0, XTYPE_XBOXONE
> },
> { 0x045e, 0x0719, "Xbox 360 Wireless Receiver",
> MAP_DPAD_TO_BUTTONS, XTYPE_XBOX360W },
> +   { 0x045e, 0x0b12, "Microsoft X-Box One X pad",
> MAP_SHARE_BUTTON, XTYPE_XBOXONE },
> { 0x046d, 0xc21d, "Logitech Gamepad F310", 0, XTYPE_XBOX360 },
> { 0x046d, 0xc21e, "Logitech Gamepad F510", 0, XTYPE_XBOX360 },
> { 0x046d, 0xc21f, "Logitech Gamepad F710", 0, XTYPE_XBOX360 },
> @@ -862,6 +864,8 @@ static void xpadone_process_packet(struct usb_xpad
> *xpad, u16 cmd, unsigned char
> /* menu/view buttons */
> input_report_key(dev, BTN_START,  data[4] & 0x04);
> input_report_key(dev, BTN_SELECT, data[4] & 0x08);
> +   if (xpad->mapping & MAP_SHARE_BUTTON)
> +   input_report_key(dev, KEY_RECORD, data[22] & 0x01);
>  
> /* buttons A,B,X,Y */
> input_report_key(dev, BTN_A,data[4] & 0x10);
> @@ -1669,9 +1673,12 @@ static int xpad_init_input(struct usb_xpad
> *xpad)
>  
> /* set up model-specific ones */
> if (xpad->xtype == XTYPE_XBOX360 || xpad->xtype ==
> XTYPE_XBOX360W ||
> -   xpad->xtype == XTYPE_XBOXONE) {
> +   xpad->xtype == XTYPE_XBOXONE) {
> for (i = 0; xpad360_btn[i] >= 0; i++)
> input_set_capability(input_dev, EV_KEY,
> xpad360_btn[i]);
> +   if (xpad->mapping & MAP_SHARE_BUTTON) {
> +   input_set_capability(input_dev, EV_KEY,
> KEY_RECORD);

Is there not a better keycode to use than "Record"? Should a "share"
keycode be added?

I couldn't find a share button in the most recent USB HID Usage Tables:
https://www.usb.org/document-library/hid-usage-tables-121

> +   }
> } else {
> for (i = 0; xpad_btn[i] >= 0; i++)
> input_set_capability(input_dev, EV_KEY,
> xpad_btn[i]);

Re: linux-next: Signed-off-by missing for commit in the bluetooth tree

[Bastien Nocera]

On Tue, 2021-01-26 at 19:54 +1100, Stephen Rothwell wrote:
> Hi all,
> 
> Commit
> 
>   98d2c3e17310 ("Bluetooth: L2CAP: Try harder to accept device not
> knowing options")
> 
> is missing a Signed-off-by from its author.

Marcel, is it possible to amend this to add my Signed-off-by?

Signed-off-by: Bastien Nocera 

Cheers

Re: [PATCH 0/2] Add support for Goodix GT9286 chip

[Bastien Nocera]

On Sat, 2021-01-09 at 14:55 +0100, AngeloGioacchino Del Regno wrote:
> Add support for the GT9286 chip, tested on F(x)Tec Pro1 (MSM8998).

Can you please add this test information to the commit message for the
goodix.c patch?

Feel free to add my:
Reviewed-by: Bastien Nocera 
to both patches when you send a v2.

Cheers

> 
> AngeloGioacchino Del Regno (2):
>   input: goodix: Add support for Goodix GT9286 chip
>   dt-bindings: ts: goodix: Add binding for GT9286 IC
> 
>  Documentation/devicetree/bindings/input/touchscreen/goodix.yaml | 1
> +
>  drivers/input/touchscreen/goodix.c  | 2
> ++
>  2 files changed, 3 insertions(+)
> 

Re: [PATCH] HID: logitech-hidpp: add support for Unified Battery (1004) feature

[Bastien Nocera]

On Wed, 2021-01-06 at 18:48 +, Filipe Laíns wrote:
> On Wed, 2021-01-06 at 10:34 +0100, Bastien Nocera wrote:
> > On Mon, 2021-01-04 at 18:29 +, la...@archlinux.org wrote:
> > > From: Filipe Laíns 
> > > 
> > > This new feature present in new devices replaces the old Battery
> > > Level
> > > Status (0x1000) feature. It keeps essentially the same
> > > information
> > > for
> > > levels (reporting critical, low, good and full) but makes these
> > > levels
> > > optional, the device exports a capability setting which describes
> > > which
> > > levels it supports. In addition to this, there is an optional
> > > state_of_charge paramenter that exports the battery percentage.
> > > 
> > > This patch adds support for this new feature. There were some
> > > implementation choices, as described below and in the code.
> > > 
> > > If the device supports the state_of_charge parameter, we will
> > > just
> > > export the battery percentage and not the levels, which the
> > > device
> > > might
> > > still support.
> > 
> > I'm guessing that means no changes needed on the upower side?
> > 
> > Cheers
> > 
> 
> Yes :)
> I tested upower and all works as expected.
> 
> There will still be devices that only support battery voltage, so I
> might
> implement the battery voltage to charge percentage in a future patch.

I sent a WIP patch at the end of November for that, it wasn't even
compile-tested, but might be a good base to start from.

I don't think that I have any hardware that supports that feature in
any case.

Cheers

Re: [PATCH] HID: logitech-hidpp: add support for Unified Battery (1004) feature

[Bastien Nocera]

On Mon, 2021-01-04 at 18:29 +, la...@archlinux.org wrote:
> From: Filipe Laíns 
> 
> This new feature present in new devices replaces the old Battery
> Level
> Status (0x1000) feature. It keeps essentially the same information
> for
> levels (reporting critical, low, good and full) but makes these
> levels
> optional, the device exports a capability setting which describes
> which
> levels it supports. In addition to this, there is an optional
> state_of_charge paramenter that exports the battery percentage.
> 
> This patch adds support for this new feature. There were some
> implementation choices, as described below and in the code.
> 
> If the device supports the state_of_charge parameter, we will just
> export the battery percentage and not the levels, which the device
> might
> still support.

I'm guessing that means no changes needed on the upower side?

Cheers

Re: How to enable auto-suspend by default

[Bastien Nocera]

On Wed, 2020-11-11 at 17:32 +0100, Greg KH wrote:
> On Wed, Nov 11, 2020 at 04:03:30PM +, Limonciello, Mario wrote:
> > > > > Given we're effectively ending up with the combination of
> > > > > runtime PM turned
> > > > > on by udev rules, do we need something like this for that ID:
> > > > > 
> > > > > 
> > > https://github.com/torvalds/linux/commit/6a7c533d4a1854f54901a065d8c672e890400
> > > d8a
> > > > > 
> > > > > @Mika Westerberg should 8086:a0ed be quirked like the TCSS
> > > > > xHCI too?
> > > > 
> > > > I think this one is the TGL PCH xHCI. The quirk currently for
> > > > xHCI
> > > > controllers that are part of the TCSS (Type-C SubSystem) where
> > > > it is
> > > > important to put all devices into low power mode whenever
> > > > possible,
> > > > otherwise it keeps the whole block on.
> > > 
> > > Note that there are currently some IDs missing from the xHCIs
> > > which
> > > are part of the TCSS too. At least the id for the xHCI in the
> > > thunderbolt
> > > controller on the Lenovo T14 gen 1 is missing. I started a
> > > discussion
> > > about extending the kernel quirk list for this vs switching to
> > > hwdb
> > > a while a go:
> > > 
> > > https://lore.kernel.org/linux-usb/b8b21ba3-0a8a-ff54-5e12-
> > > cf8960651...@redhat.com/
> > > 
> > > The conclusion back then was to switch to hwdb, but I never got
> > > around to
> > > this.
> > 
> > I guess the problem I see with switching to a hwdb for this type of
> > thing is
> > that if there is a "bug" in your kernel driver around autosuspend
> > you will
> > then be potentially causing it to occur more regularly on a kernel
> > that didn't
> > necessarily pick up the fix but does have the newer hwdb.
> > 
> > I don't know how common that will really be though.
> > 
> > Since Mika mentioned the really light userspace scenario, what
> > about shipping
> > the hwdb "with" the kernel in tree?  This could allow evicting all
> > these quirk
> > scenarios from the kernel at the same time as switching to a hwdb
> > and also cover
> > the problem I suggested might happen with a bug in older kernel and
> > newer userspace.
> 
> We took things out of the kernel to put it in hwdb years ago as it
> was
> easier for people to update a "text file" than it was their kernel
> image.  I don't think you want to go backwards here :)

There are (unfortunately) a couple of Linux based OSes that don't use
systemd, which is one of the problems we see.

I think it might be a good idea to have a repository or directory
that's accessible to same contributions as the drivers, where this sort
of data is kept, as close to the drivers as possible.

You could always split off your quirks into separate "works with any
kernel" and "works from this version of the kernel" files, the goal
here would be to make sure that there is a canonical list of devices
that can be autosuspended, without user-space always playing catch-up
(especially as is the case now where systemd is being fed by ChromeOS
which is fed in some other way).

The Venn diagram of folks that contribute to hwdb quirks databases in
systemd and that contribute to kernel drivers has a pretty small
overlap. Moving much of those quirks to a kernel-controlled repository
(whatever format it ends up being in) would make sense so that the
"quirk enablement" and the "driver writing" sections overlap.

Re: How to enable auto-suspend by default

[Bastien Nocera]

On Tue, 2020-11-24 at 14:37 +0200, Mathias Nyman wrote:
> 
> I don't think we are ready to enable runtime pm as default for all
> Intel xHCI controllers.
> The risk of xHCI not waking up when user plugs a mouse/keyboard,
> making the system unusable
> just seems too high compared to the powersaving benefit.
> 
> The powersaving benefit from autosuspending the TCSS xHCI is a lot
> better, and we, (Mika mostly)
> has been able to verify they work.
> 
> So I propose we for now continue adding TCSS xHCI controllers to the
> allowlist in kernel.
> For others I think a userspace allow/denylist makes sense.
> 
> Long term goal would be default allow for all, with short denylist in
> kernel.

Is there any way to preemptively enable autosuspend for all the _TCSS_
xHCI controllers?

This was the problem the original post tried to tease out, whether it
would be easier/better to enable autosuspend by default, and not enable
it on systems where it breaks something, rather than default to sucking
battery until somebody notices that a device ID got missed.

How to enable auto-suspend by default

[Bastien Nocera]

Hey,

systemd has been shipping this script to enable auto-suspend on a
number of USB and PCI devices:
https://github.com/systemd/systemd/blob/master/tools/chromiumos/gen_autosuspend_rules.py

The problem here is twofold. First, the list of devices is updated from
ChromeOS, and the original list obviously won't be updated by ChromeOS
developers unless a device listed exists in a ChromeBook computer,
which means a number of devices that do support autosuspend aren't
listed.

The other problem is that this list needs to exist at all, and that it
doesn't seem possible for device driver developers (at various levels
of the stack) to opt-in to auto-suspend when all the variants of the
device (or at least detectable ones) support auto-suspend.

So the question is: how can we make it easier for device drivers to
implicitly allow autosuspend *unless they opt-out*, especially for
frameworks where the device's transport layer isn't directly available
(eg. HID devices)?

If that can't be done in the kernel drivers directly, would it be
possible for the kernel to ship with a somewhat canonical list that
systemd (or its replacement on other "Linuxes") could use to generate
those user-space quirks?

Ideally, for example, all new "iwlwifi" or all tested "iwlwifi" devices
should have autosuspend enabled by the developers adding support for
them, as in the script above, rather than downstreams (systemd upstream
included) having to chase new PCI IDs.

Cheers

Re: [External] Re: [PATCH] Documentation: Add documentation for new platform_profile sysfs attribute

[Bastien Nocera]

On Thu, 2020-10-29 at 10:46 +0100, Hans de Goede wrote:
> <
> 

> IMHO it does not belong in the sysfs API docs for the
> platform_profile
> stuff. But I guess it would be good to document it somewhere in some
> generic syfs API rules/expectations document (with a note that their
> might be exceptions).
> 
> Ideally we would already have such a file somewhere, but I don't know
> if we do (I did not look). So if you feel like it (and such a file
> does
> not exist yet) then I guess a patch adding such a doc file would be
> good.

I don't know enough about the helpers and the code around it to know
whether documenting this would be needed, but I'm fine with knowing
that we're not breaking new ground here.

Re: [PATCH] Documentation: Add documentation for new platform_profile sysfs attribute

[Bastien Nocera]

On Wed, 2020-10-28 at 18:23 +0100, Hans de Goede wrote:
> 
> > It's not meaningless, but rather ambiguous. For a range of 1 to 5,
> > is 1
> > high performance, and 5 low power, or vice-versa?
> 
> It is meaningless because the space we are trying to describe with
> the
> profile-names is not 1 dimensional. E.g. as discussed before cool and
> low-power are not necessarily the same thing. If you have a better
> way
> to word this I'm definitely in favor of improving the text here.

What do you think of:

> +Since numbers are a rather meaningless way to describe platform-
profiles

"Since numbers on their own cannot represent the multiple variables
that a profile will adjust (power consumption, heat generation, etc.)
..."

> +this API uses strings to describe the various profiles. To make sure that
> +userspace gets a consistent experience when using this API this API
> +document defines a fixed set of profile-names. Drivers *must* map their
> +internal profile representation/names onto this fixed set.

Re: [PATCH] Documentation: Add documentation for new platform_profile sysfs attribute

[Bastien Nocera]

Hey Hans, Mark,

On Tue, 2020-10-27 at 12:42 -0400, Mark Pearson wrote:
> From: Hans de Goede 
> 
> On modern systems the platform performance, temperature, fan and
> other
> hardware related characteristics are often dynamically configurable.
> The
> profile is often automatically adjusted to the load by somei
> automatic-mechanism (which may very well live outside the kernel).
> 
> These auto platform-adjustment mechanisms often can be configured
> with
> one of several 'platform-profiles', with either a bias towards low-
> power

Can you please make sure to quote 'platform-profile' and 'profile-name'
this way all through the document? They're not existing words, and
quoting them shows that they're attribute names, rather than English.

> consumption or towards performance (and higher power consumption and
> thermals).

s/thermal/temperature/

"A thermal" is something else (it's seasonal underwear for me ;)

> Introduce a new platform_profile sysfs API which offers a generic API
> for
> selecting the performance-profile of these automatic-mechanisms.
> 
> Co-developed-by: Mark Pearson 
> Signed-off-by: Mark Pearson 
> Signed-off-by: Hans de Goede 
> ---
> Changes in V1:
>  - Moved from RFC to proposed patch
>  - Added cool profile as requested
>  - removed extra-profiles as no longer relevant
> 
>  .../ABI/testing/sysfs-platform_profile    | 66
> +++
>  1 file changed, 66 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-platform_profile
> 
> diff --git a/Documentation/ABI/testing/sysfs-platform_profile
> b/Documentation/ABI/testing/sysfs-platform_profile
> new file mode 100644
> index ..240bd3d7532b
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-platform_profile
> @@ -0,0 +1,66 @@
> +Platform-profile selection (e.g.
> /sys/firmware/acpi/platform_profile)
> +
> +On modern systems the platform performance, temperature, fan and
> other
> +hardware related characteristics are often dynamically configurable.
> The
> +profile is often automatically adjusted to the load by some
> +automatic-mechanism (which may very well live outside the kernel).
> +
> +These auto platform-adjustment mechanisms often can be configured
> with
> +one of several 'platform-profiles', with either a bias towards low-
> power
> +consumption or towards performance (and higher power consumption and
> +thermals).
> +
> +The purpose of the platform_profile attribute is to offer a generic
> sysfs
> +API for selecting the platform-profile of these automatic-
> mechanisms.
> +
> +Note that this API is only for selecting the platform-profile, it is
> +NOT a goal of this API to allow monitoring the resulting performance
> +characteristics. Monitoring performance is best done with
> device/vendor
> +specific tools such as e.g. turbostat.
> +
> +Specifically when selecting a high-performance profile the actual
> achieved
> +performance may be limited by various factors such as: the heat
> generated
> +by other components, room temperature, free air flow at the bottom
> of a
> +laptop, etc. It is explicitly NOT a goal of this API to let
> userspace know
> +about any sub-optimal conditions which are impeding reaching the
> requested
> +performance level.
> +
> +Since numbers are a rather meaningless way to describe platform-
> profiles

It's not meaningless, but rather ambiguous. For a range of 1 to 5, is 1
high performance, and 5 low power, or vice-versa?

> +this API uses strings to describe the various profiles. To make sure
> that
> +userspace gets a consistent experience when using this API this API

you can remove "when using this API".

> +document defines a fixed set of profile-names. Drivers *must* map
> their
> +internal profile representation/names onto this fixed set.
> +
> +If for some reason there is no good match when mapping then a new
> profile-name
> +may be added.

"for some reason" can be removed.

>  Drivers which wish to introduce new profile-names must:
> +1. Have very good reasons to do so.

"1. Explain why the existing 'profile-names' cannot be used"

> +2. Add the new profile-name to this document, so that future drivers
> which also
> +   have a similar problem can use the same name.

"2. Add the new 'profile-name' to the documentation so that other
drivers can use it, as well as user-space knowing clearly what
behaviour the 'profile-name' corresponds to"

> +
> +What:  /sys/firmware/acpi/platform_profile_choices
> +Date:  October 2020
> +Contact:   Hans de Goede 
> +Description:
> +   Reading this file gives a space separated list of
> profiles
> +   supported for this device.

"This file contains a space-separated list of profiles..."

> +
> +   Drivers must use the following standard profile-
> names:
> +
> +   low-power:  Emphasises low power
> consumption
> +   cool:   Emphasises cooler operation
> +   quiet:  Emphasises 

Re: [RFC] Documentation: Add documentation for new performance_profile sysfs class

[Bastien Nocera]

On Wed, 2020-10-07 at 15:58 +, Limonciello, Mario wrote:
>  
> > On Mon, 2020-10-05 at 12:58 +, Limonciello, Mario wrote:
> > > > On modern systems CPU/GPU/... performance is often dynamically
> > > > configurable
> > > > in the form of e.g. variable clock-speeds and TPD. The
> > > > performance
> > > > is often
> > > > automatically adjusted to the load by some automatic-mechanism
> > > > (which may
> > > > very well live outside the kernel).
> > > > 
> > > > These auto performance-adjustment mechanisms often can be
> > > > configured with
> > > > one of several performance-profiles, with either a bias towards
> > > > low-power
> > > > consumption (and cool and quiet) or towards performance (and
> > > > higher
> > > > power
> > > > consumption and thermals).
> > > > 
> > > > Introduce a new performance_profile class/sysfs API which
> > > > offers a
> > > > generic
> > > > API for selecting the performance-profile of these automatic-
> > > > mechanisms.
> > > > 
> > > 
> > > If introducing an API for this - let me ask the question, why
> > > even let each
> > > driver offer a class interface and userspace need to change
> > > "each" driver's
> > > performance setting?
> > > 
> > > I would think that you could just offer something kernel-wide
> > > like
> > > /sys/power/performance-profile
> > > 
> > > Userspace can read and write to a single file.  All drivers can
> > > get notified
> > > on this sysfs file changing.
> > > 
> > > The systems that react in firmware (such as the two that prompted
> > > this discussion) can change at that time.  It leaves the
> > > possibility for a
> > > more open kernel implementation that can do the same thing though
> > > too by
> > > directly modifying device registers instead of ACPI devices.
> > 
> > The problem, as I've mentioned in previous discussions we had about
> > this, is that, as you've seen in replies to this mail, this would
> > suddenly be making the kernel apply policy.
> > 
> > There's going to be pushback as soon as policy is enacted in the
> > kernel, and you take away the different knobs for individual
> > components
> > (or you can control them centrally as well as individually). As
> > much as
> > I hate the quantity of knobs[1], I don't think that trying to
> > reduce
> > the number of knobs in the kernel is a good use of our time, and
> > easier
> > to enact, coordinated with design targets, in user-space.
> > 
> > Unless you can think of a way to implement this kernel wide setting
> > without adding one more exponent on the number of possibilities for
> > the
> > testing matrix, I'll +1 Hans' original API.
> > 
> Actually I offered two proposals in my reply.  So are you NAKing
> both?

No, this is only about the first portion of the email, which I quoted.
And I'm not NAK'ing it, but I don't see how it can work without being
antithetical to what kernel "users" expect, or what the folks consuming
those interfaces (presumably us both) would expect to be able to test
and maintain.

> The other one suggested to use the same firmware attributes class
> being
> introduced by the new Dell driver ( 
> https://patchwork.kernel.org/patch/11818343/)
> since this is actually a knob to a specific firmware setting.

This seemed to me like an implementation detail (eg. the same metadata
is being exported, but in a different way), and I don't feel strongly
about it either way.

Re: [RFC] Documentation: Add documentation for new performance_profile sysfs class

[Bastien Nocera]

On Mon, 2020-10-05 at 12:58 +, Limonciello, Mario wrote:
> > On modern systems CPU/GPU/... performance is often dynamically
> > configurable
> > in the form of e.g. variable clock-speeds and TPD. The performance
> > is often
> > automatically adjusted to the load by some automatic-mechanism
> > (which may
> > very well live outside the kernel).
> > 
> > These auto performance-adjustment mechanisms often can be
> > configured with
> > one of several performance-profiles, with either a bias towards
> > low-power
> > consumption (and cool and quiet) or towards performance (and higher
> > power
> > consumption and thermals).
> > 
> > Introduce a new performance_profile class/sysfs API which offers a
> > generic
> > API for selecting the performance-profile of these automatic-
> > mechanisms.
> > 
> 
> If introducing an API for this - let me ask the question, why even let each
> driver offer a class interface and userspace need to change "each" driver's
> performance setting?
> 
> I would think that you could just offer something kernel-wide like
> /sys/power/performance-profile
> 
> Userspace can read and write to a single file.  All drivers can get notified
> on this sysfs file changing.
> 
> The systems that react in firmware (such as the two that prompted
> this discussion) can change at that time.  It leaves the possibility for a
> more open kernel implementation that can do the same thing though too by
> directly modifying device registers instead of ACPI devices.

The problem, as I've mentioned in previous discussions we had about
this, is that, as you've seen in replies to this mail, this would
suddenly be making the kernel apply policy.

There's going to be pushback as soon as policy is enacted in the
kernel, and you take away the different knobs for individual components
(or you can control them centrally as well as individually). As much as
I hate the quantity of knobs[1], I don't think that trying to reduce
the number of knobs in the kernel is a good use of our time, and easier
to enact, coordinated with design targets, in user-space.

Unless you can think of a way to implement this kernel wide setting
without adding one more exponent on the number of possibilities for the
testing matrix, I'll +1 Hans' original API.

Cheers

[1]: https://ometer.com/preferences.html

Re: [PATCH] Revert "Bluetooth: Update resolving list when updating whitelist"

[Bastien Nocera]

On Sun, 2020-10-04 at 15:18 +0200, Greg Kroah-Hartman wrote:
> On Sun, Oct 04, 2020 at 02:17:06PM +0200, Bastien Nocera wrote:
> > On Sun, 2020-10-04 at 12:51 +0200, Greg Kroah-Hartman wrote:
> > > On Sat, Oct 03, 2020 at 08:33:18PM +0200, Marcel Holtmann wrote:
> > > > Hi Greg,
> > > > 
> > > > > > > This reverts commit
> > > > > > > 0eee35bdfa3b472cc986ecc6ad76293fdcda59e2
> > > > > > > as it
> > > > > > > breaks all bluetooth connections on my machine.
> > > > > > > 
> > > > > > > Cc: Marcel Holtmann 
> > > > > > > Cc: Sathish Narsimman 
> > > > > > > Fixes: 0eee35bdfa3b ("Bluetooth: Update resolving list
> > > > > > > when
> > > > > > > updating whitelist")
> > > > > > > Signed-off-by: Greg Kroah-Hartman
> > > > > > > 
> > > > > > > ---
> > > > > > > net/bluetooth/hci_request.c | 41 ++--
> > > > > > > 
> > > > > > > -
> > > > > > > 1 file changed, 2 insertions(+), 39 deletions(-)
> > > > > > > 
> > > > > > > This has been bugging me for since 5.9-rc1, when all
> > > > > > > bluetooth devices
> > > > > > > stopped working on my desktop system.  I finally got the
> > > > > > > time
> > > > > > > to do
> > > > > > > bisection today, and it came down to this patch. 
> > > > > > > Reverting
> > > > > > > it on top of
> > > > > > > 5.9-rc7 restored bluetooth devices and now my input
> > > > > > > devices
> > > > > > > properly
> > > > > > > work.
> > > > > > > 
> > > > > > > As it's almost 5.9-final, any chance this can be merged
> > > > > > > now
> > > > > > > to fix the
> > > > > > > issue?
> > > > > > 
> > > > > > can you be specific what breaks since our guys and I also
> > > > > > think
> > > > > > the
> > > > > > ChromeOS guys have been testing these series of patches
> > > > > > heavily.
> > > > > 
> > > > > My bluetooth trackball does not connect at all.  With this
> > > > > reverted, it
> > > > > all "just works".
> > > > > 
> > > > > Same I think for a Bluetooth headset, can check that again if
> > > > > you
> > > > > really
> > > > > need me to, but the trackball is reliable here.
> > > > > 
> > > > > > When you run btmon does it indicate any errors?
> > > > > 
> > > > > How do I run it and where are the errors displayed?
> > > > 
> > > > you can do btmon -w trace.log and just let it run like tcdpump.
> > > 
> > > Ok, attached.
> > > 
> > > The device is not connecting, and then I open the gnome bluetooth
> > > dialog
> > > and it scans for devices in the area, but does not connect to my
> > > existing devices at all.
> > > 
> > > Any ideas?
> > 
> > Use bluetoothctl instead, the Bluetooth Settings from GNOME also
> > run a
> > discovery the whole time the panel is opened, and this breaks a
> > fair
> > number of poor quality adapters. This is worked-around in the most
> > recent version, but using bluetoothctl is a better debugging option
> > in
> > all cases.
> 
> Ok, but how do I use that tool?  How do I shut down the gnome
> bluetooth
> stuff?

You close the settings window...

> I need newbie steps here please for what to run and what to show you.

bluetoothctl connect "bluetooth address"
eg.
bluetoothctl connect "12:34:56:78:90"

Re: [PATCH] Revert "Bluetooth: Update resolving list when updating whitelist"

[Bastien Nocera]

On Sun, 2020-10-04 at 12:51 +0200, Greg Kroah-Hartman wrote:
> On Sat, Oct 03, 2020 at 08:33:18PM +0200, Marcel Holtmann wrote:
> > Hi Greg,
> > 
> > > > > This reverts commit 0eee35bdfa3b472cc986ecc6ad76293fdcda59e2
> > > > > as it
> > > > > breaks all bluetooth connections on my machine.
> > > > > 
> > > > > Cc: Marcel Holtmann 
> > > > > Cc: Sathish Narsimman 
> > > > > Fixes: 0eee35bdfa3b ("Bluetooth: Update resolving list when
> > > > > updating whitelist")
> > > > > Signed-off-by: Greg Kroah-Hartman
> > > > > 
> > > > > ---
> > > > > net/bluetooth/hci_request.c | 41 ++--
> > > > > -
> > > > > 1 file changed, 2 insertions(+), 39 deletions(-)
> > > > > 
> > > > > This has been bugging me for since 5.9-rc1, when all
> > > > > bluetooth devices
> > > > > stopped working on my desktop system.  I finally got the time
> > > > > to do
> > > > > bisection today, and it came down to this patch.  Reverting
> > > > > it on top of
> > > > > 5.9-rc7 restored bluetooth devices and now my input devices
> > > > > properly
> > > > > work.
> > > > > 
> > > > > As it's almost 5.9-final, any chance this can be merged now
> > > > > to fix the
> > > > > issue?
> > > > 
> > > > can you be specific what breaks since our guys and I also think
> > > > the
> > > > ChromeOS guys have been testing these series of patches
> > > > heavily.
> > > 
> > > My bluetooth trackball does not connect at all.  With this
> > > reverted, it
> > > all "just works".
> > > 
> > > Same I think for a Bluetooth headset, can check that again if you
> > > really
> > > need me to, but the trackball is reliable here.
> > > 
> > > > When you run btmon does it indicate any errors?
> > > 
> > > How do I run it and where are the errors displayed?
> > 
> > you can do btmon -w trace.log and just let it run like tcdpump.
> 
> Ok, attached.
> 
> The device is not connecting, and then I open the gnome bluetooth
> dialog
> and it scans for devices in the area, but does not connect to my
> existing devices at all.
> 
> Any ideas?

Use bluetoothctl instead, the Bluetooth Settings from GNOME also run a
discovery the whole time the panel is opened, and this breaks a fair
number of poor quality adapters. This is worked-around in the most
recent version, but using bluetoothctl is a better debugging option in
all cases.

Cheers

Re: [PATCH] HID: udraw-ps3: Replace HTTP links with HTTPS ones

[Bastien Nocera]

On Sat, 2020-07-18 at 12:33 +0200, Alexander A. Klimov wrote:
> Rationale:
> Reduces attack surface on kernel devs opening the links for MITM
> as HTTPS traffic is much harder to manipulate.
> 
> Deterministic algorithm:
> For each file:
>   If not .svg:
> For each line:
>   If doesn't contain `\bxmlns\b`:
> For each link, `\bhttp://[^# \t\r\n]*(?:\w|/)`:
> If neither `\bgnu\.org/license`, nor `\bmozilla\.org/MPL\b`:
> If both the HTTP and HTTPS versions
> return 200 OK and serve the same content:
>   Replace HTTP with HTTPS.
> 
> Signed-off-by: Alexander A. Klimov 

Looks good!

Acked-by: Bastien Nocera 

Re: [PATCH 3/3] platform/x86: thinkpad_acpi: Map Clipping tool hotkey to KEY_SELECTIVE_SCREENSHOT

[Bastien Nocera]

On Fri, 2020-07-17 at 13:41 +0200, Hans de Goede wrote:
> Commit 696c6523ec8f ("platform/x86: thinkpad_acpi: add mapping for
> new
> hotkeys") added support for a bunch of new hotkeys, but the
> clipping/snipping tool hotkey got ignored because there was no good
> key-code to map it to.
> 
> Recently a new KEY_SELECTIVE_SCREENSHOT keycode was added by commit
> 3b059da9835c ("Input: allocate keycode for "Selective Screenshot"
> key")
> quoting from the commit message:
> 
> "New Chrome OS keyboards have a "snip" key that is basically a
> selective
> screenshot (allows a user to select an area of screen to be copied).
> Allocate a keycode for it."
> 
> Support for this "snip" key seems like it is also a good match for
> the
> clipping/snipping tool hotkey, so map this hotkey to the new
> KEY_SELECTIVE_SCREENSHOT key-code.
> 
> Signed-off-by: Hans de Goede 

Added 5 years ago for the Carbon X1 2014, and finally getting
a keycode ;)

Reviewed-by: Bastien Nocera 

> ---
>  drivers/platform/x86/thinkpad_acpi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/thinkpad_acpi.c
> b/drivers/platform/x86/thinkpad_acpi.c
> index 7fc44b6f8370..70d533b0c907 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -3436,7 +3436,7 @@ static int __init hotkey_init(struct
> ibm_init_struct *iibm)
>   KEY_UNKNOWN,
>  
>   KEY_BOOKMARKS,  /* Favorite app,
> 0x311 */
> - KEY_RESERVED,   /* Clipping tool */
> + KEY_SELECTIVE_SCREENSHOT,   /* Clipping tool */
>   KEY_CALC,   /* Calculator (above numpad,
> P52) */
>   KEY_BLUETOOTH,  /* Bluetooth */
>   KEY_KEYBOARD,   /* Keyboard, 0x315 */

Re: [PATCH 1/8] USB: rename USB quirk to USB_QUIRK_ENDPOINT_IGNORE

[Bastien Nocera]

On Fri, 2020-06-19 at 12:53 +0200, Hans de Goede wrote:
> A note for future reference, not sure what you mean with driver
> 
> API here. If you mean the in kernel API, the kernel rules are
> 
> that we are always free to change that (Linux does not have a
> 
> stable driver API).
> 
> 
> 
> So if a header does not sit under include/uapi (indicating that
> 
> it is an userspace API) then a change like this is fine.

I meant the internal driver API, which might break out-of-tree drivers.
I know that this API is fluid, and that there are no stability
guarantees, but I'd still expect at least a note in the commit message
to that effect.

Re: [PATCH 1/8] USB: rename USB quirk to USB_QUIRK_ENDPOINT_IGNORE

[Bastien Nocera]

On Thu, 2020-06-18 at 11:42 +0200, Greg Kroah-Hartman wrote:
> The USB core has a quirk flag to ignore specific endpoints, so rename
> it
> to be more obvious what this quirk does.
> 
> Cc: Johan Hovold 
> Cc: Alan Stern 
> Cc: Richard Dodd 
> Cc: Hans de Goede 
> Cc: Jonathan Cox 
> Cc: Bastien Nocera 
> Cc: "Thiébaud Weksteen" 
> Cc: Nishad Kamdar 
> Signed-off-by: Greg Kroah-Hartman 

If the driver API change below is agreeable, you can add my:
Reviewed-by: Bastien Nocera 

Good job.


> diff --git a/include/linux/usb/quirks.h b/include/linux/usb/quirks.h
> index 22c1f579afe3..5e4c497f54d6 100644
> --- a/include/linux/usb/quirks.h
> +++ b/include/linux/usb/quirks.h
> @@ -69,7 +69,7 @@
>  /* Hub needs extra delay after resetting its port. */
>  #define USB_QUIRK_HUB_SLOW_RESET BIT(14)
>  
> -/* device has blacklisted endpoints */
> -#define USB_QUIRK_ENDPOINT_BLACKLIST BIT(15)
> +/* device has endpoints that should be ignored */
> +#define USB_QUIRK_ENDPOINT_IGNOREBIT(15)
>  
>  #endif /* __LINUX_USB_QUIRKS_H */

Re: [PATCH v6] HID: sb0540: add support for Creative SB0540 IR receivers

[Bastien Nocera]

Hey,

On Tue, 2019-07-02 at 10:39 +0200, Bastien Nocera wrote:
> From: Bastien Nocera 
> 
> Add a new hid driver for the Creative SB0540 IR receiver. This
> receiver
> is usually coupled with an RM-1500 or an RM-1800 remote control.
> 
> The scrollwheels on the RM-1800 remote are not bound, as they are
> labelled for specific audio controls that don't usually exist on most
> systems. They can be remapped using standard Linux keyboard
> remapping tools.

I'm back from travelling, so ready to update/respin this patch if
needed.

Cheers

Re: [PATCH v5] hid-logitech-hidpp: read battery voltage from newer devices

[Bastien Nocera]

On Sat, 2019-08-31 at 13:56 -0400, Pedro Vanzella wrote:
> Newer Logitech mice report their battery voltage through feature
> 0x1001
> instead of the battery levels through feature 0x1000.
> 
> When the device is brought up and we try to query the battery, figure
> out if it supports the old or the new feature. If it supports the new
> feature, record the feature index and read the battery voltage and
> its status.

FWIW, it wasn't clear to me that there were 3 bytes, and the last one
would contain the battery status. I was under the impression reading
this that the only thing the mouse would send back would be the
voltage, so this might need a slight rewording.

Did you test this with upower? Did it work as you expected?

Cheers

[PATCH v6] HID: sb0540: add support for Creative SB0540 IR receivers

[Bastien Nocera]

From: Bastien Nocera 

Add a new hid driver for the Creative SB0540 IR receiver. This receiver
is usually coupled with an RM-1500 or an RM-1800 remote control.

The scrollwheels on the RM-1800 remote are not bound, as they are
labelled for specific audio controls that don't usually exist on most
systems. They can be remapped using standard Linux keyboard
remapping tools.

Signed-off-by: Bastien Nocera 
---
 MAINTAINERS   |   6 +
 drivers/hid/Kconfig   |   9 +
 drivers/hid/Makefile  |   1 +
 drivers/hid/hid-creative-sb0540.c | 268 ++
 drivers/hid/hid-ids.h |   1 +
 5 files changed, 285 insertions(+)
 create mode 100644 drivers/hid/hid-creative-sb0540.c

diff --git a/MAINTAINERS b/MAINTAINERS
index d0ed735994a5..6fc022d152c8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4227,6 +4227,12 @@ S:   Maintained
 F: Documentation/filesystems/cramfs.txt
 F: fs/cramfs/
 
+CREATIVE SB0540
+M: Bastien Nocera 
+L: linux-in...@vger.kernel.org
+S: Maintained
+F: drivers/hid/hid-creative-sb0540.c
+
 CRYPTO API
 M: Herbert Xu 
 M: "David S. Miller" 
diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 3872e03d9a59..a70999f9c639 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -273,6 +273,15 @@ config HID_CP2112
and gpiochip to expose these functions of the CP2112. The
customizable USB descriptor fields are exposed as sysfs attributes.
 
+config HID_CREATIVE_SB0540
+   tristate "Creative SB0540 infrared receiver"
+   depends on USB_HID
+   help
+   Support for Creative infrared SB0540-compatible remote controls, such
+   as the RM-1500 and RM-1800 remotes.
+
+   Say Y here if you want support for Creative SB0540 infrared receiver.
+
 config HID_CYPRESS
tristate "Cypress mouse and barcode readers"
depends on HID
diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
index cc5d827c9164..0c03308cfb08 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -27,6 +27,7 @@ obj-$(CONFIG_HID_ALPS)+= hid-alps.o
 obj-$(CONFIG_HID_ACRUX)+= hid-axff.o
 obj-$(CONFIG_HID_APPLE)+= hid-apple.o
 obj-$(CONFIG_HID_APPLEIR)  += hid-appleir.o
+obj-$(CONFIG_HID_CREATIVE_SB0540)  += hid-creative-sb0540.o
 obj-$(CONFIG_HID_ASUS) += hid-asus.o
 obj-$(CONFIG_HID_AUREAL)   += hid-aureal.o
 obj-$(CONFIG_HID_BELKIN)   += hid-belkin.o
diff --git a/drivers/hid/hid-creative-sb0540.c 
b/drivers/hid/hid-creative-sb0540.c
new file mode 100644
index ..b4c8e7a5d3e0
--- /dev/null
+++ b/drivers/hid/hid-creative-sb0540.c
@@ -0,0 +1,268 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * HID driver for the Creative SB0540 receiver
+ *
+ * Copyright (C) 2019 Red Hat Inc. All Rights Reserved
+ *
+ */
+
+#include 
+#include 
+#include 
+#include "hid-ids.h"
+
+MODULE_AUTHOR("Bastien Nocera ");
+MODULE_DESCRIPTION("HID Creative SB0540 receiver");
+MODULE_LICENSE("GPL");
+
+static const unsigned short creative_sb0540_key_table[] = {
+   KEY_POWER,
+   KEY_RESERVED,   /* text: 24bit */
+   KEY_RESERVED,   /* 24bit wheel up */
+   KEY_RESERVED,   /* 24bit wheel down */
+   KEY_RESERVED,   /* text: CMSS */
+   KEY_RESERVED,   /* CMSS wheel Up */
+   KEY_RESERVED,   /* CMSS wheel Down */
+   KEY_RESERVED,   /* text: EAX */
+   KEY_RESERVED,   /* EAX wheel up */
+   KEY_RESERVED,   /* EAX wheel down */
+   KEY_RESERVED,   /* text: 3D Midi */
+   KEY_RESERVED,   /* 3D Midi wheel up */
+   KEY_RESERVED,   /* 3D Midi wheel down */
+   KEY_MUTE,
+   KEY_VOLUMEUP,
+   KEY_VOLUMEDOWN,
+   KEY_UP,
+   KEY_LEFT,
+   KEY_RIGHT,
+   KEY_REWIND,
+   KEY_OK,
+   KEY_FASTFORWARD,
+   KEY_DOWN,
+   KEY_AGAIN,  /* text: Return, symbol: Jump to */
+   KEY_PLAY,   /* text: Start */
+   KEY_ESC,/* text: Cancel */
+   KEY_RECORD,
+   KEY_OPTION,
+   KEY_MENU,   /* text: Display */
+   KEY_PREVIOUS,
+   KEY_PLAYPAUSE,
+   KEY_NEXT,
+   KEY_SLOW,
+   KEY_STOP,
+   KEY_NUMERIC_1,
+   KEY_NUMERIC_2,
+   KEY_NUMERIC_3,
+   KEY_NUMERIC_4,
+   KEY_NUMERIC_5,
+   KEY_NUMERIC_6,
+   KEY_NUMERIC_7,
+   KEY_NUMERIC_8,
+   KEY_NUMERIC_9,
+   KEY_NUMERIC_0
+};
+
+/*
+ * Codes and keys from lirc's
+ * remotes/creative/lircd.conf.alsa_usb
+ * order and size must match creative_sb0540_key_table[] above
+ */
+static const unsigned short creative_sb0540_codes[] = {
+   0x619E,
+   0x916E,
+   0x926D,
+   0x936C,
+   0x718E,
+   0x946B,
+   0x956A,
+   0x8C73,
+   0x9669,
+   0x9768,
+   

Re: [PATCH v5] HID: sb0540: add support for Creative SB0540 IR receivers

[Bastien Nocera]

On Tue, 2019-07-02 at 10:29 +0200, Benjamin Tissoires wrote:
> drivers/hid/hid-creative-sb0540.c: In function
> 'creative_sb0540_raw_event':
> drivers/hid/hid-creative-sb0540.c:157:3: error: label 'out' used but
> not defined
>   157 |   goto out;
>   |   ^~~~
> 
> It would have been nice to at least try to compile it in a tree.
> You don't need to compile the whole tree: just clone it, apply your
> patch and then `make -j4 M=drivers/hid`

v4 _did_ build. Don't be surprised if after 4 versions on top of the
ones you did when the driver was out of tree, I get review fatigue, go
for expediency and some mistakes slip through.

Fixed in v6.

Re: [PATCH v4] HID: sb0540: add support for Creative SB0540 IR receivers

[Bastien Nocera]

On Mon, 2019-07-01 at 12:15 +0200, Benjamin Tissoires wrote:
> 

> I forgot to mention that sparse was complaining about:
> 
> scripts/Makefile.build:283: target 'drivers/hid/hid-creative-
> sb0540.c'
> doesn't match the target pattern
> 
> And it turns out your line should read `hid-creative-sb0540.o` not
> `hid-creative-sb0540.c`

It does show that I didn't fancy making my office warmer with a full
kernel compile, right?

v5 sent with the fix, thanks.

[PATCH v5] HID: sb0540: add support for Creative SB0540 IR receivers

[Bastien Nocera]

From: Bastien Nocera 

Add a new hid driver for the Creative SB0540 IR receiver. This receiver
is usually coupled with an RM-1500 or an RM-1800 remote control.

The scrollwheels on the RM-1800 remote are not bound, as they are
labelled for specific audio controls that don't usually exist on most
systems. They can be remapped using standard Linux keyboard
remapping tools.

Signed-off-by: Bastien Nocera 
---
 MAINTAINERS   |   6 +
 drivers/hid/Kconfig   |   9 +
 drivers/hid/Makefile  |   1 +
 drivers/hid/hid-creative-sb0540.c | 268 ++
 drivers/hid/hid-ids.h |   1 +
 5 files changed, 285 insertions(+)
 create mode 100644 drivers/hid/hid-creative-sb0540.c

diff --git a/MAINTAINERS b/MAINTAINERS
index d0ed735994a5..6fc022d152c8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4227,6 +4227,12 @@ S:   Maintained
 F: Documentation/filesystems/cramfs.txt
 F: fs/cramfs/
 
+CREATIVE SB0540
+M: Bastien Nocera 
+L: linux-in...@vger.kernel.org
+S: Maintained
+F: drivers/hid/hid-creative-sb0540.c
+
 CRYPTO API
 M: Herbert Xu 
 M: "David S. Miller" 
diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 3872e03d9a59..a70999f9c639 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -273,6 +273,15 @@ config HID_CP2112
and gpiochip to expose these functions of the CP2112. The
customizable USB descriptor fields are exposed as sysfs attributes.
 
+config HID_CREATIVE_SB0540
+   tristate "Creative SB0540 infrared receiver"
+   depends on USB_HID
+   help
+   Support for Creative infrared SB0540-compatible remote controls, such
+   as the RM-1500 and RM-1800 remotes.
+
+   Say Y here if you want support for Creative SB0540 infrared receiver.
+
 config HID_CYPRESS
tristate "Cypress mouse and barcode readers"
depends on HID
diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
index cc5d827c9164..0c03308cfb08 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -27,6 +27,7 @@ obj-$(CONFIG_HID_ALPS)+= hid-alps.o
 obj-$(CONFIG_HID_ACRUX)+= hid-axff.o
 obj-$(CONFIG_HID_APPLE)+= hid-apple.o
 obj-$(CONFIG_HID_APPLEIR)  += hid-appleir.o
+obj-$(CONFIG_HID_CREATIVE_SB0540)  += hid-creative-sb0540.o
 obj-$(CONFIG_HID_ASUS) += hid-asus.o
 obj-$(CONFIG_HID_AUREAL)   += hid-aureal.o
 obj-$(CONFIG_HID_BELKIN)   += hid-belkin.o
diff --git a/drivers/hid/hid-creative-sb0540.c 
b/drivers/hid/hid-creative-sb0540.c
new file mode 100644
index ..6b7c81ccf310
--- /dev/null
+++ b/drivers/hid/hid-creative-sb0540.c
@@ -0,0 +1,268 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * HID driver for the Creative SB0540 receiver
+ *
+ * Copyright (C) 2019 Red Hat Inc. All Rights Reserved
+ *
+ */
+
+#include 
+#include 
+#include 
+#include "hid-ids.h"
+
+MODULE_AUTHOR("Bastien Nocera ");
+MODULE_DESCRIPTION("HID Creative SB0540 receiver");
+MODULE_LICENSE("GPL");
+
+static const unsigned short creative_sb0540_key_table[] = {
+   KEY_POWER,
+   KEY_RESERVED,   /* text: 24bit */
+   KEY_RESERVED,   /* 24bit wheel up */
+   KEY_RESERVED,   /* 24bit wheel down */
+   KEY_RESERVED,   /* text: CMSS */
+   KEY_RESERVED,   /* CMSS wheel Up */
+   KEY_RESERVED,   /* CMSS wheel Down */
+   KEY_RESERVED,   /* text: EAX */
+   KEY_RESERVED,   /* EAX wheel up */
+   KEY_RESERVED,   /* EAX wheel down */
+   KEY_RESERVED,   /* text: 3D Midi */
+   KEY_RESERVED,   /* 3D Midi wheel up */
+   KEY_RESERVED,   /* 3D Midi wheel down */
+   KEY_MUTE,
+   KEY_VOLUMEUP,
+   KEY_VOLUMEDOWN,
+   KEY_UP,
+   KEY_LEFT,
+   KEY_RIGHT,
+   KEY_REWIND,
+   KEY_OK,
+   KEY_FASTFORWARD,
+   KEY_DOWN,
+   KEY_AGAIN,  /* text: Return, symbol: Jump to */
+   KEY_PLAY,   /* text: Start */
+   KEY_ESC,/* text: Cancel */
+   KEY_RECORD,
+   KEY_OPTION,
+   KEY_MENU,   /* text: Display */
+   KEY_PREVIOUS,
+   KEY_PLAYPAUSE,
+   KEY_NEXT,
+   KEY_SLOW,
+   KEY_STOP,
+   KEY_NUMERIC_1,
+   KEY_NUMERIC_2,
+   KEY_NUMERIC_3,
+   KEY_NUMERIC_4,
+   KEY_NUMERIC_5,
+   KEY_NUMERIC_6,
+   KEY_NUMERIC_7,
+   KEY_NUMERIC_8,
+   KEY_NUMERIC_9,
+   KEY_NUMERIC_0
+};
+
+/*
+ * Codes and keys from lirc's
+ * remotes/creative/lircd.conf.alsa_usb
+ * order and size must match creative_sb0540_key_table[] above
+ */
+static const unsigned short creative_sb0540_codes[] = {
+   0x619E,
+   0x916E,
+   0x926D,
+   0x936C,
+   0x718E,
+   0x946B,
+   0x956A,
+   0x8C73,
+   0x9669,
+   0x9768,
+   

Re: [PATCH v3] HID: sb0540: add support for Creative SB0540 IR receivers

[Bastien Nocera]

On Mon, 2019-07-01 at 11:45 +0200, Benjamin Tissoires wrote:
> Hi Bastien,
> 
> On Wed, Jun 26, 2019 at 4:07 PM Bastien Nocera 
> wrote:
> > From: Bastien Nocera 
> > 
> > Add a new hid driver for the Creative SB0540 IR receiver. This
> > receiver
> > is usually coupled with an RM-1500 or an RM-1800 remote control.
> > 
> > The scrollwheels on the RM-1800 remote are not bound, as they are
> > labelled for specific audio controls that don't usually exist on
> > most
> > systems. They can be remapped using standard Linux keyboard
> > remapping tools.
> 
> Much better commit message :)
> Thanks!
> 
> I have a few nitpicks however

Most of the checkpatch.pl warnings are now fixed. I ignored the one
about writing docs for the symbol (I have no idea what it wants, looks
like a false positive) and the too long line for the USB device match.


> > +   /* force input as some remotes bypass the input
> > registration */
> > +   hid->quirks |= HID_QUIRK_HIDINPUT_FORCE;
> 
> Does this still applies to your remote?

Yes, you mentioned it in your original review as well, and it was
necessary.

v4's been sent.

Thanks!

[PATCH v4] HID: sb0540: add support for Creative SB0540 IR receivers

[Bastien Nocera]

From: Bastien Nocera 

Add a new hid driver for the Creative SB0540 IR receiver. This receiver
is usually coupled with an RM-1500 or an RM-1800 remote control.

The scrollwheels on the RM-1800 remote are not bound, as they are
labelled for specific audio controls that don't usually exist on most
systems. They can be remapped using standard Linux keyboard
remapping tools.

Signed-off-by: Bastien Nocera 
---
 MAINTAINERS   |   6 +
 drivers/hid/Kconfig   |   9 +
 drivers/hid/Makefile  |   1 +
 drivers/hid/hid-creative-sb0540.c | 268 ++
 drivers/hid/hid-ids.h |   1 +
 5 files changed, 285 insertions(+)
 create mode 100644 drivers/hid/hid-creative-sb0540.c

diff --git a/MAINTAINERS b/MAINTAINERS
index d0ed735994a5..6fc022d152c8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4227,6 +4227,12 @@ S:   Maintained
 F: Documentation/filesystems/cramfs.txt
 F: fs/cramfs/
 
+CREATIVE SB0540
+M: Bastien Nocera 
+L: linux-in...@vger.kernel.org
+S: Maintained
+F: drivers/hid/hid-creative-sb0540.c
+
 CRYPTO API
 M: Herbert Xu 
 M: "David S. Miller" 
diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 3872e03d9a59..a70999f9c639 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -273,6 +273,15 @@ config HID_CP2112
and gpiochip to expose these functions of the CP2112. The
customizable USB descriptor fields are exposed as sysfs attributes.
 
+config HID_CREATIVE_SB0540
+   tristate "Creative SB0540 infrared receiver"
+   depends on USB_HID
+   help
+   Support for Creative infrared SB0540-compatible remote controls, such
+   as the RM-1500 and RM-1800 remotes.
+
+   Say Y here if you want support for Creative SB0540 infrared receiver.
+
 config HID_CYPRESS
tristate "Cypress mouse and barcode readers"
depends on HID
diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
index cc5d827c9164..1ad662fe37b6 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -27,6 +27,7 @@ obj-$(CONFIG_HID_ALPS)+= hid-alps.o
 obj-$(CONFIG_HID_ACRUX)+= hid-axff.o
 obj-$(CONFIG_HID_APPLE)+= hid-apple.o
 obj-$(CONFIG_HID_APPLEIR)  += hid-appleir.o
+obj-$(CONFIG_HID_CREATIVE_SB0540)  += hid-creative-sb0540.c
 obj-$(CONFIG_HID_ASUS) += hid-asus.o
 obj-$(CONFIG_HID_AUREAL)   += hid-aureal.o
 obj-$(CONFIG_HID_BELKIN)   += hid-belkin.o
diff --git a/drivers/hid/hid-creative-sb0540.c 
b/drivers/hid/hid-creative-sb0540.c
new file mode 100644
index ..6b7c81ccf310
--- /dev/null
+++ b/drivers/hid/hid-creative-sb0540.c
@@ -0,0 +1,268 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * HID driver for the Creative SB0540 receiver
+ *
+ * Copyright (C) 2019 Red Hat Inc. All Rights Reserved
+ *
+ */
+
+#include 
+#include 
+#include 
+#include "hid-ids.h"
+
+MODULE_AUTHOR("Bastien Nocera ");
+MODULE_DESCRIPTION("HID Creative SB0540 receiver");
+MODULE_LICENSE("GPL");
+
+static const unsigned short creative_sb0540_key_table[] = {
+   KEY_POWER,
+   KEY_RESERVED,   /* text: 24bit */
+   KEY_RESERVED,   /* 24bit wheel up */
+   KEY_RESERVED,   /* 24bit wheel down */
+   KEY_RESERVED,   /* text: CMSS */
+   KEY_RESERVED,   /* CMSS wheel Up */
+   KEY_RESERVED,   /* CMSS wheel Down */
+   KEY_RESERVED,   /* text: EAX */
+   KEY_RESERVED,   /* EAX wheel up */
+   KEY_RESERVED,   /* EAX wheel down */
+   KEY_RESERVED,   /* text: 3D Midi */
+   KEY_RESERVED,   /* 3D Midi wheel up */
+   KEY_RESERVED,   /* 3D Midi wheel down */
+   KEY_MUTE,
+   KEY_VOLUMEUP,
+   KEY_VOLUMEDOWN,
+   KEY_UP,
+   KEY_LEFT,
+   KEY_RIGHT,
+   KEY_REWIND,
+   KEY_OK,
+   KEY_FASTFORWARD,
+   KEY_DOWN,
+   KEY_AGAIN,  /* text: Return, symbol: Jump to */
+   KEY_PLAY,   /* text: Start */
+   KEY_ESC,/* text: Cancel */
+   KEY_RECORD,
+   KEY_OPTION,
+   KEY_MENU,   /* text: Display */
+   KEY_PREVIOUS,
+   KEY_PLAYPAUSE,
+   KEY_NEXT,
+   KEY_SLOW,
+   KEY_STOP,
+   KEY_NUMERIC_1,
+   KEY_NUMERIC_2,
+   KEY_NUMERIC_3,
+   KEY_NUMERIC_4,
+   KEY_NUMERIC_5,
+   KEY_NUMERIC_6,
+   KEY_NUMERIC_7,
+   KEY_NUMERIC_8,
+   KEY_NUMERIC_9,
+   KEY_NUMERIC_0
+};
+
+/*
+ * Codes and keys from lirc's
+ * remotes/creative/lircd.conf.alsa_usb
+ * order and size must match creative_sb0540_key_table[] above
+ */
+static const unsigned short creative_sb0540_codes[] = {
+   0x619E,
+   0x916E,
+   0x926D,
+   0x936C,
+   0x718E,
+   0x946B,
+   0x956A,
+   0x8C73,
+   0x9669,
+   0x9768,
+   

[PATCH v3] HID: sb0540: add support for Creative SB0540 IR receivers

[Bastien Nocera]

From: Bastien Nocera 

Add a new hid driver for the Creative SB0540 IR receiver. This receiver
is usually coupled with an RM-1500 or an RM-1800 remote control.

The scrollwheels on the RM-1800 remote are not bound, as they are
labelled for specific audio controls that don't usually exist on most
systems. They can be remapped using standard Linux keyboard
remapping tools.

Signed-off-by: Bastien Nocera 
---
 drivers/hid/Kconfig   |   9 ++
 drivers/hid/Makefile  |   1 +
 drivers/hid/hid-creative-sb0540.c | 254 ++
 drivers/hid/hid-ids.h |   1 +
 4 files changed, 265 insertions(+)
 create mode 100644 drivers/hid/hid-creative-sb0540.c

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 3872e03d9a59..f16c4bd822e4 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -273,6 +273,15 @@ config HID_CP2112
and gpiochip to expose these functions of the CP2112. The
customizable USB descriptor fields are exposed as sysfs attributes.
 
+config HID_CREATIVE_SB0540
+   tristate "Creative SB0540 infrared receiver"
+   depends on USB_HID
+   ---help---
+   Support for Creative infrared SB0540-compatible remote controls, such
+   as the RM-1500 and RM-1800 remotes.
+
+   Say Y here if you want support for Creative SB0540 infrared receiver.
+
 config HID_CYPRESS
tristate "Cypress mouse and barcode readers"
depends on HID
diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
index cc5d827c9164..1ad662fe37b6 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -27,6 +27,7 @@ obj-$(CONFIG_HID_ALPS)+= hid-alps.o
 obj-$(CONFIG_HID_ACRUX)+= hid-axff.o
 obj-$(CONFIG_HID_APPLE)+= hid-apple.o
 obj-$(CONFIG_HID_APPLEIR)  += hid-appleir.o
+obj-$(CONFIG_HID_CREATIVE_SB0540)  += hid-creative-sb0540.c
 obj-$(CONFIG_HID_ASUS) += hid-asus.o
 obj-$(CONFIG_HID_AUREAL)   += hid-aureal.o
 obj-$(CONFIG_HID_BELKIN)   += hid-belkin.o
diff --git a/drivers/hid/hid-creative-sb0540.c 
b/drivers/hid/hid-creative-sb0540.c
new file mode 100644
index ..a94542cbdd33
--- /dev/null
+++ b/drivers/hid/hid-creative-sb0540.c
@@ -0,0 +1,254 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * HID driver for the Creative SB0540 receiver
+ *
+ * Copyright (C) 2019 Red Hat Inc. All Rights Reserved
+ *
+ */
+
+#include 
+#include 
+#include 
+#include "hid-ids.h"
+
+MODULE_AUTHOR("Bastien Nocera ");
+MODULE_DESCRIPTION("HID Creative SB0540 receiver");
+MODULE_LICENSE("GPL");
+
+static const unsigned short creative_sb0540_key_table[] = {
+   KEY_POWER,
+   KEY_RESERVED,   /* text: 24bit */
+   KEY_RESERVED,   /* 24bit wheel up */
+   KEY_RESERVED,   /* 24bit wheel down */
+   KEY_RESERVED,   /* text: CMSS */
+   KEY_RESERVED,   /* CMSS wheel Up */
+   KEY_RESERVED,   /* CMSS wheel Down */
+   KEY_RESERVED,   /* text: EAX */
+   KEY_RESERVED,   /* EAX wheel up */
+   KEY_RESERVED,   /* EAX wheel down */
+   KEY_RESERVED,   /* text: 3D Midi */
+   KEY_RESERVED,   /* 3D Midi wheel up */
+   KEY_RESERVED,   /* 3D Midi wheel down */
+   KEY_MUTE,
+   KEY_VOLUMEUP,
+   KEY_VOLUMEDOWN,
+   KEY_UP,
+   KEY_LEFT,
+   KEY_RIGHT,
+   KEY_REWIND,
+   KEY_OK,
+   KEY_FASTFORWARD,
+   KEY_DOWN,
+   KEY_AGAIN,  /* text: Return, symbol: Jump to */
+   KEY_PLAY,   /* text: Start */
+   KEY_ESC,/* text: Cancel */
+   KEY_RECORD,
+   KEY_OPTION,
+   KEY_MENU,   /* text: Display */
+   KEY_PREVIOUS,
+   KEY_PLAYPAUSE,
+   KEY_NEXT,
+   KEY_SLOW,
+   KEY_STOP,
+   KEY_NUMERIC_1,
+   KEY_NUMERIC_2,
+   KEY_NUMERIC_3,
+   KEY_NUMERIC_4,
+   KEY_NUMERIC_5,
+   KEY_NUMERIC_6,
+   KEY_NUMERIC_7,
+   KEY_NUMERIC_8,
+   KEY_NUMERIC_9,
+   KEY_NUMERIC_0
+};
+
+/* Codes and keys from lirc's
+ * remotes/creative/lircd.conf.alsa_usb
+ * order and size must match creative_sb0540_key_table[] above */
+static const unsigned short creative_sb0540_codes[] = {
+   0x619E,
+   0x916E,
+   0x926D,
+   0x936C,
+   0x718E,
+   0x946B,
+   0x956A,
+   0x8C73,
+   0x9669,
+   0x9768,
+   0x9867,
+   0x9966,
+   0x9A65,
+   0x6E91,
+   0x629D,
+   0x639C,
+   0x7B84,
+   0x6B94,
+   0x728D,
+   0x8778,
+   0x817E,
+   0x758A,
+   0x8D72,
+   0x8E71,
+   0x8877,
+   0x7C83,
+   0x738C,
+   0x827D,
+   0x7689,
+   0x7F80,
+   0x7986,
+   0x7A85,
+   0x7D82,
+   0x857A,
+   0x8B74,
+   0x8F70,
+   0x906F,
+   0x8A75,
+   0x847B,
+   0x7887,
+ 

Re: [PATCH] Revert "Bluetooth: Align minimum encryption key size for LE and BR/EDR connections"

[Bastien Nocera]

On Wed, 2019-06-12 at 09:07 +0200, Greg Kroah-Hartman wrote:
> On Tue, Jun 11, 2019 at 11:36:26PM +0200, Marcel Holtmann wrote:
> > Hi Vasily,
> > 
> > > Can we get this revert merged into stable branches? Bluetooth HID
> > > has
> > > been broken for many devices for quite a while now and RFC patch
> > > that
> > > fixes the breakage hasn't seen any movement for almost a month.
> > 
> > lets send the RFC patch upstream since it got enough feedback that
> > it fixes the issue.
> 
> According to Hans, the workaround did not work.

Is it possible that those folks were running Fedora, and using a
version of bluetoothd without a fix for using dbus-broker as the D-Bus
daemon implementation?

I backported the fix in an update last week:
https://bugzilla.redhat.com/show_bug.cgi?id=1711594

> So can we just get this reverted so that people's machines go back to
> working?
> 
> thanks,
> 
> greg k-h

Re: [PATCH] staging: rtl8723bs: Fix Unneeded variable: "ret". Return "0"

[Bastien Nocera]

On Thu, 2019-06-06 at 20:10 -0700, Shobhit Kukreti wrote:
> coccicheck reported Unneeded variable ret at
> rtl8723bs/core/rtw_ap.c:1400.
> Function "rtw_acl_remove_sta" always returns 0. Modified return type
> of the
> function to void.
> 
> Signed-off-by: Shobhit Kukreti 

Looks good, thanks.

Reviewed-by: Bastien Nocera 

Re: [RFC v2] iio: input-bridge: optionally bridge iio acceleometers to create a /dev/input interface

[Bastien Nocera]

On Fri, 2019-05-10 at 11:33 +0200, H. Nikolaus Schaller wrote:
> > 

> It does through "Input device name:" starting with "iio-bridge:" as
> you can see in the commit message of [RFC v3]:

This makes it ABI, right?

Big fat warnings around the code that declares it would be appreciated.

Re: [RFC v2] iio: input-bridge: optionally bridge iio acceleometers to create a /dev/input interface

[Bastien Nocera]

On Sun, 2019-04-14 at 09:26 -0700, Roderick Colenbrander wrote:
> 

> We at the time were one of the first to expose acceleration and gyro
> data through /dev/input for DualShock 4 as supported by hid-sony. We
> report acceleration in 'g' and angular velocity in 'degree / s'. We
> set the resolution to respectively '1 g' and '1 degree / s'. The range
> we set to the range of the device e.g. for DS4 -4g to +4g for
> acceleration. I need to check though what coordinate system we use,
> but I think it is right handed (gyro counter clockwise relative to
> acceleration axes).

How do you export the gyro information through the input device?

FWIW, we needed to do extra work in iio-sensor-proxy so that the
accelerometer in the Sixaxis/DS4 joypads (and uDraw tablet) didn't
appear as though they were accelerometer for the system:
https://github.com/hadess/iio-sensor-proxy/commit/401d59e54b3123860180d4401e09df8a1e1bc6c3

> The two other drivers using INPUT_PROC_ACCELEROMETER are hid-wacom and
> hid-udraw-ps3 Wacom. Both seem to report resolution in 'g'  as well.

I wrote hid-udraw-ps3, and it's reporting accelerometer data through
input because the rest of the driver is input, and it didn't make much
sense to use another subsystem for just that small portion of the
events the device sends out.

Re: [RFC v2] iio: input-bridge: optionally bridge iio acceleometers to create a /dev/input interface

[Bastien Nocera]

On Mon, 2019-04-22 at 15:20 +0100, Jonathan Cameron wrote:
> > Different goals usually lead to different solution architectures.
> 
> Indeed, but in this case we have your proposal which is a subset of
> what
> I am suggesting.  One architecture can fulfil both requirements.
> 
> I'll leave it for the other thread, but Bastien has raised the case
> (that I'd forgotten) that there already userspace stacks that are
> capable of happily taking in both IIO and Input devices.  The
> confusion
> here is they will now discover 'both' without the existing userspace
> knowing that they are the same device.  We need to be very careful
> not
> to break those userspace programs.
> 
> So this is an illustration of why the simplistic case doesn't work
> 'now'.

I don't know what state the whole patch is, but, at the very least, I'd
expect that patch to export the fact that it's exporting synthesised
data from another driver, so that it can be easily ignored in user-
space that already supports IIO devices.

Re: [RFC v3] iio: input-bridge: optionally bridge iio acceleometers to create a /dev/input interface

[Bastien Nocera]

On Tue, 2019-04-16 at 21:33 +0200, H. Nikolaus Schaller wrote:
> Hi Bastien,
> 
> > Am 16.04.2019 um 18:04 schrieb Bastien Nocera :
> > This can be done in user-space, reading the data from the IIO driver,
> > and using uinput to feed it back. Why is doing this at the kernel level
> > better?
> 
> Well, I'd estimate that >80% of the current kernel could be done in user-space
> (but not at the same speed/quality).
> 
> E.g. TCP could most likely be done by directly accessing the Ethernet layer 
> and
> providing other processes access through named pipes instead of sockets.
> 
> But usually a user-space daemon feeding things back into the kernel is slower
> (because it is scheduled differently) and needs more resources for running the
> process and IPC and is less protected against hickups and deadlocks.

This is mostly irrelevant for the amount of data we're treating, but it
doesn't matter too much.

> Two more aspects come to my mind from reading your project page:
> 
> a) "It requires libgudev and systemd"
> b) "Note that a number of kernel bugs will prevent it from working correctly"
> 
> a) this makes quite significant assumptions about the user-space while a 
> kernel
>driver can be kept independent of this

It's made for modern desktop OSes/"traditional" Linux. I don't think
that those 2 libraries are problematic dependencies unless you're on
Android, where a replacement could be implemented or iio-sensor-proxy
modified for that use case.

> b) if it is in-kernel it will be kept in sync with kernel changes and such 
> bugs
>are less likely

No they're not. This warning was because 1) drivers sometimes have bugs
2) user-space sometimes has bugs 3) user-space sometimes causes the
kernel to have bugs.

The 2 significant breakages for iio-sensor-proxy were caused by runtime
PM bugs in the hid-sensor-hub driver, and in the USB core. I doubt a
kernel-space implementation would have been able to magically fix those
bugs unfortunately.

Re: [RFC v3] iio: input-bridge: optionally bridge iio acceleometers to create a /dev/input interface

[Bastien Nocera]

Having written a "bridge" myself (I called it a "proxy"[1]), I have a
few comments.

[1]: https://github.com/hadess/iio-sensor-proxy

Let's start with the easy ones ;) there's a typo in the subject line.

The subject line also says "optionally" but there doesn't seem to be
any ways to disable the feature if it's shipped by the kernel used.

On Mon, 2019-04-15 at 23:05 +0200, H. Nikolaus Schaller wrote:
> Some user spaces (e.g. some Android devices) use /dev/input/event*
> for handling
> the 3D position of the device with respect to the center of gravity
> (earth).
> This can be used for gaming input, auto-rotation of screens etc.
> 
> This interface should be the standard for such use cases because it
> is an abstraction
> of how orientation data is acquired from sensor chips. Sensor chips
> may be connected
> through different interfaces and in different positions. They may
> also have different
> parameters. And, if a chip is replaced by a different one, the values
> reported by
> the device position interface should remain the same, provided the
> device tree reflects
> the changed chip.

I don't understand this section of the commit message. The IIO drivers
are already that abstraction interface, no?

> This did initially lead to input accelerometer drivers like
> drivers/input/misc/bma150.c
> or drivers/misc/lis3lv02d/
> 
> But nowadays, new accelerometer chips mostly get iio drivers and
> rarely input drivers.
> 
> Therefore we need something like a protocol stack which bridges raw
> data and input devices.
> It can be seen as a similar layering like TCP/IP vs. bare Ethernet.
> Or keyboard
> input events vs. raw gpio or raw USB access.

This can be done in user-space, reading the data from the IIO driver,
and using uinput to feed it back. Why is doing this at the kernel level
better?

> This patch bridges the gap between raw iio data and the input device
> abstraction
> so that accelerometer measurements can additionally be presented as
> X/Y/Z accelerometer
> channels (INPUT_PROP_ACCELEROMETER) through /dev/input/event*.
> 
> There are no special requirements or changes needed for an iio
> driver.

The user-space daemon I wrote supports both IIO drivers and input
drivers for accelerometers. How do I know from user-space whether a
device is proxied or not?

> There is no need to define a mapping (e.g. in device tree).
> 
> This driver simply collects the first 3 accelerometer channels as X,
> Y and Z.
> If only 1 or 2 channels are available, they are used for X and Y
> only. Additional
> channels are ignored.

In what cases are 2 dimensional accelerometers used?

> Scaling is done automatically so that 1g is represented by value 256
> and
> range is assumed to be -511 .. +511 which gives a reasonable
> precision as an
> input device.
> 
> If a mount-matrix is provided by the iio driver, it is also taken
> into account
> so that the input event automatically gets the correct orientation
> with respect
> to the device.
>
> If this extension is not configured into the kernel it takes no
> resources (except
> source code).
> 
> If it is configured, but there is no accelerometer, there is only a
> tiny penalty
> for scanning for accelerometer channels once during probe of each iio
> device.
> 
> If it runs, the driver polls the device(s) once every 100 ms. A mode
> where the
> iio device defines the update rate is not implemented and for further
> study.
> 
> If there is no user-space client, polling is not running.

Is the bridge going to modify the IIO device's settings behind other
possible consumer's backs, such as threshold values, and triggers?

> The driver is capable to handle multiple iio accelerometers and they
> are
> presented by unique /dev/input/event* files. The iio chip name is
> used to define
> the input device name so that it can be identified (e.g. by udev
> rules or evtest).

As you can probably guess, I'm not overly enthusiastic about this piece
of code. If it had existed 5 years ago, I probably wouldn't have
written iio-sensor-proxy, but then somebody else would have had to for
the rest of the IIO sensors that can be consumed.

To me, this bridge has all the drawbacks of a simple user-space
implementation using uinput, without much of the benefits of being an
exclusive user of the IIO accelerometers, such as being able to change
the update rate, or using triggers depending on the usage.

What am I missing? Why shouldn't this live in user-space?

Cheers

Re: [PATCH v2] input: goodix - support Goodix gt5688

[Bastien Nocera]

On Tue, 2019-01-29 at 12:11 +0100, Guido Günther wrote:
> From what I've seen in vendor trees it's fine to treat this as gt1x¹.
> Tested on the Purism Librem 5 Devkit (Rocktech JH057N00900 panel).
> 
> [1]: 
> https://github.com/TadiT7/android_kernel_mtk-4.4/tree/master/drivers/input/touchscreen/mediatek/GT5688
> 
> Signed-off-by: Guido Günther 

Not super fond of the casual commit message, but the code and
explanation are fine.

Reviewed-by: Bastien Nocera 

> ---
> Changes from v1:
> * Add tested board to commit message
> 
>  Documentation/devicetree/bindings/input/touchscreen/goodix.txt | 1 +
>  drivers/input/touchscreen/goodix.c | 2
> ++
>  2 files changed, 3 insertions(+)
> 
> diff --git
> a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> index f7e95c52f3c7..57d3d8870a09 100644
> --- a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> +++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> @@ -3,6 +3,7 @@ Device tree bindings for Goodix GT9xx series
> touchscreen controller
>  Required properties:
>  
>   - compatible: Should be "goodix,gt1151"
> +  or "goodix,gt5688"
>or "goodix,gt911"
>or "goodix,gt9110"
>or "goodix,gt912"
> diff --git a/drivers/input/touchscreen/goodix.c
> b/drivers/input/touchscreen/goodix.c
> index f2d9c2c41885..47b1ced41576 100644
> --- a/drivers/input/touchscreen/goodix.c
> +++ b/drivers/input/touchscreen/goodix.c
> @@ -216,6 +216,7 @@ static const struct goodix_chip_data
> *goodix_get_chip_data(u16 id)
>  {
>   switch (id) {
>   case 1151:
> + case 5688:
>   return _chip_data;
>  
>   case 911:
> @@ -942,6 +943,7 @@ MODULE_DEVICE_TABLE(acpi, goodix_acpi_match);
>  #ifdef CONFIG_OF
>  static const struct of_device_id goodix_of_match[] = {
>   { .compatible = "goodix,gt1151" },
> + { .compatible = "goodix,gt5688" },
>   { .compatible = "goodix,gt911" },
>   { .compatible = "goodix,gt9110" },
>   { .compatible = "goodix,gt912" },

Re: [PATCH] input: goodix - support Goodix gt5688

[Bastien Nocera]

On Tue, 2019-01-29 at 10:16 +0100, Guido Günther wrote:
> Hi,
> On Mon, Jan 28, 2019 at 07:24:33PM +0100, Bastien Nocera wrote:
> > On Mon, 2019-01-28 at 18:03 +0100, Guido Günther wrote:
> > > From what I've seen in vendor trees it's fine to treat this as
> > > gt1x¹.
> > > 
> > > [1]: 
> > > https://github.com/TadiT7/android_kernel_mtk-4.4/tree/master/drivers/input/touchscreen/mediatek/GT5688
> > 
> > Can you please point to the exact line of code that makes you say
> > that?
> > I'm not saying it's not compatible, but it's not the same driver
> > that
> > the current goodix.c was based on, or even goodix.c.
> > 
> > Can you please elaborate?
> 
> I basically looked at the registers uses for the config update:
> 
> https://github.com/TadiT7/android_kernel_mtk-4.4/blob/master/drivers/input/touchscreen/mediatek/GT5688/include/gt1x_tpd_common.h#L152
> https://github.com/TadiT7/android_kernel_mtk-4.4/blob/master/drivers/input/touchscreen/mediatek/GT5688/gt1x_generic.c#L430

I'm not sure that's good enough to say that the touchscreen models are
compatible.

> That and the fact that the driver is doing it's job well made me
> believe
> that's good for base support.

If you've tested it, that's better. Can you please add a reference to
the device that you've tested this on in the commit message?

>  Things like hotknot, gesture wakeup and
> proximity sensor will need additional work.
> 
> > Finding that data in the specs would also be fine:
> > https://github.com/hadess/gt9xx/tree/master/specifications
> 
> https://github.com/hadess/gt9xx/pull/3

Merged that, thanks

Cheers

Re: [PATCH] input: goodix - support Goodix gt5688

[Bastien Nocera]

On Mon, 2019-01-28 at 18:03 +0100, Guido Günther wrote:
> From what I've seen in vendor trees it's fine to treat this as gt1x¹.
> 
> [1]: 
> https://github.com/TadiT7/android_kernel_mtk-4.4/tree/master/drivers/input/touchscreen/mediatek/GT5688

Can you please point to the exact line of code that makes you say that?
I'm not saying it's not compatible, but it's not the same driver that
the current goodix.c was based on, or even goodix.c.

Can you please elaborate?

Finding that data in the specs would also be fine:
https://github.com/hadess/gt9xx/tree/master/specifications

Cheers

[1]: original driver:
https://github.com/hadess/gt9xx/commit/82b141220e8bce00060e0de697735d0a70af2678

> Signed-off-by: Guido Günther 
> ---
>  Documentation/devicetree/bindings/input/touchscreen/goodix.txt | 1 +
>  drivers/input/touchscreen/goodix.c | 2
> ++
>  2 files changed, 3 insertions(+)
> 
> diff --git
> a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> index f7e95c52f3c7..57d3d8870a09 100644
> --- a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> +++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> @@ -3,6 +3,7 @@ Device tree bindings for Goodix GT9xx series
> touchscreen controller
>  Required properties:
>  
>   - compatible: Should be "goodix,gt1151"
> +  or "goodix,gt5688"
>or "goodix,gt911"
>or "goodix,gt9110"
>or "goodix,gt912"
> diff --git a/drivers/input/touchscreen/goodix.c
> b/drivers/input/touchscreen/goodix.c
> index f2d9c2c41885..47b1ced41576 100644
> --- a/drivers/input/touchscreen/goodix.c
> +++ b/drivers/input/touchscreen/goodix.c
> @@ -216,6 +216,7 @@ static const struct goodix_chip_data
> *goodix_get_chip_data(u16 id)
>  {
>   switch (id) {
>   case 1151:
> + case 5688:
>   return _chip_data;
>  
>   case 911:
> @@ -942,6 +943,7 @@ MODULE_DEVICE_TABLE(acpi, goodix_acpi_match);
>  #ifdef CONFIG_OF
>  static const struct of_device_id goodix_of_match[] = {
>   { .compatible = "goodix,gt1151" },
> + { .compatible = "goodix,gt5688" },
>   { .compatible = "goodix,gt911" },
>   { .compatible = "goodix,gt9110" },
>   { .compatible = "goodix,gt912" },

Re: [PATCH] Input: goodix - decouple irq and reset lines

[Bastien Nocera]

On Mon, 2019-01-07 at 16:56 +0100, Bastien Nocera wrote:
> Given that we do have access to the datasheet, it would also be
> useful
> for the patch to mention where in the datasheet it says that the
> reset
> line can be left pulled-up, or mention on which shipping device this
> setup is already used (and if so, what the DTS or ACPI snippet that
> declares those is).

Or alternatively, and as Andreas pointed out in another thread, find
the code in the vendor driver that does support this setup:
https://github.com/goodix/gt9xx_driver_android
https://github.com/goodix/goodix_gt9xx_public

If it doesn't work with the vendor code, then we might not want to make
it work with our driver either.

Cheers

Re: [PATCH] Input: goodix - decouple irq and reset lines

[Bastien Nocera]

On Sat, 2019-01-05 at 22:51 +, Dmitry Torokhov wrote:
> Hi Alex,
> 
> On Fri, Jan 04, 2019 at 05:00:48PM +0100, Alex Gonzalez wrote:
> > The Goodix touch controller allows the use of two optional GPIOs
> > (RESET
> > and INT) to reset the touch controller, select the I2C address of
> > the
> > device and exit the device from sleep mode.
> > 
> > The current implementation requires both GPIOs to be provided,
> > however,
> > it is possible to provide only the INT line and not to have the
> > RESET line
> > available but pulled-up.
> > 
> > Designs that only provide the INT line are able to operate the
> > touch on
> > the default I2C address but will not be able to reset the touch via
> > software or place the device in sleep mode.
> 
> I do not have a datasheet for the device, so I am not sure if reset
> line

Data sheets for a lot of the Goodix devices were shared a couple of
years ago on this list. You'll find the one for the GT911 in here:
https://drive.google.com/drive/folders/0BxCVOQS3ZymGfmJyY2RKbE5XbVlKNlktVTlwV0lxNEdxd2dzeWZER094cmJPVnMxN1F0Yzg?usp=sharing

You can probably translate it using Google Translate's Documents
upload, but I haven't had much luck at all...

> is actually needed to put the device into sleep mode. As far as I can
> see from the code we suspend it by pulsing INT line and then sending
> a
> command to the controller, and resuming by pulsing the INT line
> again.
> So it sounds to me INT only designs _could_ place device in sleep
> mode.
> 
> As far as the patch goes, if you do not need to execute reset or put
> device into low power mode, you do not need to specify any of the
> GPIOs
> as GPIO resources. Simply specify the INT GPIO as your interrupt
> source
> (GpioInt() in ACPI world or "interrupts = < NNN
> IRQF_TRIGGER_WHATEVER>" in DT world and be done with it.

Given that we do have access to the datasheet, it would also be useful
for the patch to mention where in the datasheet it says that the reset
line can be left pulled-up, or mention on which shipping device this
setup is already used (and if so, what the DTS or ACPI snippet that
declares those is).

Cheers

XBox One S controller, Bluetooth support (was Re: hid: microsoft: Add rumble support for Xbox One S controller)

[Bastien Nocera]

(Resend to the correct Bluetooth list, sorry for the dupes)

On Wed, 2018-09-26 at 15:45 +0200, Bastien Nocera wrote:
> Hey Florian,
> 
> On Wed, 2018-09-26 at 14:51 +0200, Dollinger Florian wrote:
> > From: Florian Dollinger 
> > 
> > Hi there! Why do you re-engineer the wheel? :) There is already a
> > fully functional and tested driver out there (
> > https://github.com/atar-axis/xpadneo). Would be much easier to help
> > me (the owner of xpadneo) to push it into the kernel.
> 
> Probably because he didn't know about it, and how would he? I also
> didn't know about it, because it didn't exist last I worked on those
> joypads.
> 
> I spent quite a bit of time trying to get the XBox One S controller
> working over Bluetooth, without success, and I see that you have a
> patch for that which you didn't send upstream either:
> 
https://github.com/atar-axis/xpadneo/blob/master/misc/kernel_patches/0001-fix_bluetooth_reconnect.patch

For this patch, after speaking with the Bluetooth maintainers, we would
need to:
- add relevant btmon outputs before and after the patch in the commit
message
- remove the mention of "many devices" (if you've seen other devices
with that problem in the wild, please mention them, otherwise mention
that it's for "XBox One S" controllers, and clones)
- run the patch against the "Profile Tuning Suite Tool" from the
Bluetooth SIG. This requires a Windows laptop, and a piece of hardware
from the Bluetooth SIG, see below.

The first 2 should be pretty easy to do, just send your patch using
"git send-email" to the linux-bluetooth list (CC:ed), with the data
attached.

For the qualification test, I was told Szymon has a test suite that he
could use to double-check the tests. If not, I should be able to buy
the adapter and run that test suite locally.

Cheers

XBox One S controller, Bluetooth support (was Re: hid: microsoft: Add rumble support for Xbox One S controller)

[Bastien Nocera]

(Resend to the correct Bluetooth list, sorry for the dupes)

On Wed, 2018-09-26 at 15:45 +0200, Bastien Nocera wrote:
> Hey Florian,
> 
> On Wed, 2018-09-26 at 14:51 +0200, Dollinger Florian wrote:
> > From: Florian Dollinger 
> > 
> > Hi there! Why do you re-engineer the wheel? :) There is already a
> > fully functional and tested driver out there (
> > https://github.com/atar-axis/xpadneo). Would be much easier to help
> > me (the owner of xpadneo) to push it into the kernel.
> 
> Probably because he didn't know about it, and how would he? I also
> didn't know about it, because it didn't exist last I worked on those
> joypads.
> 
> I spent quite a bit of time trying to get the XBox One S controller
> working over Bluetooth, without success, and I see that you have a
> patch for that which you didn't send upstream either:
> 
https://github.com/atar-axis/xpadneo/blob/master/misc/kernel_patches/0001-fix_bluetooth_reconnect.patch

For this patch, after speaking with the Bluetooth maintainers, we would
need to:
- add relevant btmon outputs before and after the patch in the commit
message
- remove the mention of "many devices" (if you've seen other devices
with that problem in the wild, please mention them, otherwise mention
that it's for "XBox One S" controllers, and clones)
- run the patch against the "Profile Tuning Suite Tool" from the
Bluetooth SIG. This requires a Windows laptop, and a piece of hardware
from the Bluetooth SIG, see below.

The first 2 should be pretty easy to do, just send your patch using
"git send-email" to the linux-bluetooth list (CC:ed), with the data
attached.

For the qualification test, I was told Szymon has a test suite that he
could use to double-check the tests. If not, I should be able to buy
the adapter and run that test suite locally.

Cheers

Re: AW: hid: microsoft: Add rumble support for Xbox One S controller

[Bastien Nocera]

On Wed, 2018-09-26 at 17:15 +0200, dollinger.flor...@gmx.de wrote:
> Hey Bastien,
> 
> > Probably because he didn't know about it, and how would he?
> 
> Hum, it's the first hit when you search for something like 'xbox one
> s driver linux' since half an year or longer 

Sure it is, but:
- it's not an "XBox One S controller", it's a variant of the "XBox One
controller" shipped with the XBox One S.
- we discussed this on the linux-bluetooth mailing-list in August 2016:
  https://www.spinics.net/lists/linux-bluetooth/msg68102.html
  revived the thread in 2017:
  https://www.spinics.net/lists/linux-bluetooth/msg72750.html
  and discussed the force feedback in particular in August this year:
  https://www.spinics.net/lists/linux-input/msg57744.html
- why would we look for a "driver" when there's already one in the
kernel that supports all the xbox controllers except this one? :)

Really, as much as it's nice to find working code for this device, it's
surprising you didn't contact any kernel developer before, rather than
us having to find you.

> > I can imagine that a large portion of the driver can be integrated
> > in the existing XBox pad driver, with each feature added in
> > individual patches.
> 
> Would be great! But ist the xpad driver really the right place?
> 
> First of all, the BT interface is using HID, USB does not.
> Furthermore, the driver is currently around 1,3k lines long - for a
> single device and only BT... (okay yes, there are a lot of comments
> in it).
> But anyway, is it really a good idea to put everything into hid-
> microsoft?
> Moreover, there is another interface which I am trying to support at
> the moment - the one which is used by the XBOX (WiFi).
> 
> I think in the end the whole hid-microsoft thingy will get blown-up a
> bit - right?
> Or, everything is scattered over many places (hid-microsoft, xpad,
> another driver fort he wifi-interface).
> 
> Isn't it possible to use "one driver per device" in the kernel too,
> like I did?

Yes, but that's not the way the drivers are usually arranged. They're
arranged by vendors, so this driver would need to be merged into the
"hid-microsoft.c" driver.

I'm pretty sure that the USB version can also be made to use HID.

I have no idea how the RF protocol would work though. I imagine it
requires a dongle, or does it actually use Wi-Fi? (that would be
surprising...).

Anyway, I don't have much time to work on it right this minute, but it
would be great if you could send your Bluetooth patch to start with,
and we can iterate on that, and fix the other problems as we clear the
priority list.

Cheers

Re: AW: hid: microsoft: Add rumble support for Xbox One S controller

[Bastien Nocera]

On Wed, 2018-09-26 at 17:15 +0200, dollinger.flor...@gmx.de wrote:
> Hey Bastien,
> 
> > Probably because he didn't know about it, and how would he?
> 
> Hum, it's the first hit when you search for something like 'xbox one
> s driver linux' since half an year or longer 

Sure it is, but:
- it's not an "XBox One S controller", it's a variant of the "XBox One
controller" shipped with the XBox One S.
- we discussed this on the linux-bluetooth mailing-list in August 2016:
  https://www.spinics.net/lists/linux-bluetooth/msg68102.html
  revived the thread in 2017:
  https://www.spinics.net/lists/linux-bluetooth/msg72750.html
  and discussed the force feedback in particular in August this year:
  https://www.spinics.net/lists/linux-input/msg57744.html
- why would we look for a "driver" when there's already one in the
kernel that supports all the xbox controllers except this one? :)

Really, as much as it's nice to find working code for this device, it's
surprising you didn't contact any kernel developer before, rather than
us having to find you.

> > I can imagine that a large portion of the driver can be integrated
> > in the existing XBox pad driver, with each feature added in
> > individual patches.
> 
> Would be great! But ist the xpad driver really the right place?
> 
> First of all, the BT interface is using HID, USB does not.
> Furthermore, the driver is currently around 1,3k lines long - for a
> single device and only BT... (okay yes, there are a lot of comments
> in it).
> But anyway, is it really a good idea to put everything into hid-
> microsoft?
> Moreover, there is another interface which I am trying to support at
> the moment - the one which is used by the XBOX (WiFi).
> 
> I think in the end the whole hid-microsoft thingy will get blown-up a
> bit - right?
> Or, everything is scattered over many places (hid-microsoft, xpad,
> another driver fort he wifi-interface).
> 
> Isn't it possible to use "one driver per device" in the kernel too,
> like I did?

Yes, but that's not the way the drivers are usually arranged. They're
arranged by vendors, so this driver would need to be merged into the
"hid-microsoft.c" driver.

I'm pretty sure that the USB version can also be made to use HID.

I have no idea how the RF protocol would work though. I imagine it
requires a dongle, or does it actually use Wi-Fi? (that would be
surprising...).

Anyway, I don't have much time to work on it right this minute, but it
would be great if you could send your Bluetooth patch to start with,
and we can iterate on that, and fix the other problems as we clear the
priority list.

Cheers

XBox One S controller, Bluetooth support (was Re: hid: microsoft: Add rumble support for Xbox One S controller)

[Bastien Nocera]

On Wed, 2018-09-26 at 15:45 +0200, Bastien Nocera wrote:
> Hey Florian,
> 
> On Wed, 2018-09-26 at 14:51 +0200, Dollinger Florian wrote:
> > From: Florian Dollinger 
> > 
> > Hi there! Why do you re-engineer the wheel? :) There is already a
> > fully functional and tested driver out there (
> > https://github.com/atar-axis/xpadneo). Would be much easier to help
> > me (the owner of xpadneo) to push it into the kernel.
> 
> Probably because he didn't know about it, and how would he? I also
> didn't know about it, because it didn't exist last I worked on those
> joypads.
> 
> I spent quite a bit of time trying to get the XBox One S controller
> working over Bluetooth, without success, and I see that you have a
> patch for that which you didn't send upstream either:
> https://github.com/atar-axis/xpadneo/blob/master/misc/kernel_patches/0001-fix_bluetooth_reconnect.patch

For this patch, after speaking with the Bluetooth maintainers, we would
need to:
- add relevant btmon outputs before and after the patch in the commit
message
- remove the mention of "many devices" (if you've seen other devices
with that problem in the wild, please mention them, otherwise mention
that it's for "XBox One S" controllers, and clones)
- run the patch against the "Profile Tuning Suite Tool" from the
Bluetooth SIG. This requires a Windows laptop, and a piece of hardware
from the Bluetooth SIG, see below.

The first 2 should be pretty easy to do, just send your patch using
"git send-email" to the linux-bluetooth list (CC:ed), with the data
attached.

For the qualification test, I was told Szymon has a test suite that he
could use to double-check the tests. If not, I should be able to buy
the adapter and run that test suite locally.

Cheers

XBox One S controller, Bluetooth support (was Re: hid: microsoft: Add rumble support for Xbox One S controller)

[Bastien Nocera]

On Wed, 2018-09-26 at 15:45 +0200, Bastien Nocera wrote:
> Hey Florian,
> 
> On Wed, 2018-09-26 at 14:51 +0200, Dollinger Florian wrote:
> > From: Florian Dollinger 
> > 
> > Hi there! Why do you re-engineer the wheel? :) There is already a
> > fully functional and tested driver out there (
> > https://github.com/atar-axis/xpadneo). Would be much easier to help
> > me (the owner of xpadneo) to push it into the kernel.
> 
> Probably because he didn't know about it, and how would he? I also
> didn't know about it, because it didn't exist last I worked on those
> joypads.
> 
> I spent quite a bit of time trying to get the XBox One S controller
> working over Bluetooth, without success, and I see that you have a
> patch for that which you didn't send upstream either:
> https://github.com/atar-axis/xpadneo/blob/master/misc/kernel_patches/0001-fix_bluetooth_reconnect.patch

For this patch, after speaking with the Bluetooth maintainers, we would
need to:
- add relevant btmon outputs before and after the patch in the commit
message
- remove the mention of "many devices" (if you've seen other devices
with that problem in the wild, please mention them, otherwise mention
that it's for "XBox One S" controllers, and clones)
- run the patch against the "Profile Tuning Suite Tool" from the
Bluetooth SIG. This requires a Windows laptop, and a piece of hardware
from the Bluetooth SIG, see below.

The first 2 should be pretty easy to do, just send your patch using
"git send-email" to the linux-bluetooth list (CC:ed), with the data
attached.

For the qualification test, I was told Szymon has a test suite that he
could use to double-check the tests. If not, I should be able to buy
the adapter and run that test suite locally.

Cheers

Re: hid: microsoft: Add rumble support for Xbox One S controller

[Bastien Nocera]

Hey Florian,

On Wed, 2018-09-26 at 14:51 +0200, Dollinger Florian wrote:
> From: Florian Dollinger 
> 
> Hi there! Why do you re-engineer the wheel? :) There is already a
> fully functional and tested driver out there (
> https://github.com/atar-axis/xpadneo). Would be much easier to help
> me (the owner of xpadneo) to push it into the kernel.

Probably because he didn't know about it, and how would he? I also
didn't know about it, because it didn't exist last I worked on those
joypads.

I spent quite a bit of time trying to get the XBox One S controller
working over Bluetooth, without success, and I see that you have a
patch for that which you didn't send upstream either:
https://github.com/atar-axis/xpadneo/blob/master/misc/kernel_patches/0001-fix_bluetooth_reconnect.patch

I can imagine that a large portion of the driver can be integrated in
the existing XBox pad driver, with each feature added in individual
patches.

If I get the time, there are good chances I will send a patch to
integrate the battery reporting in the existing driver at least, and
then add support for missing buttons if there's a problem there (I see
that mentioned in the README).

"Trigger Force Feedback" is likely something that would need to be
integrated at a lower level, this is probably not something we'd want
to have replicated in each driver.

Cheers

Re: hid: microsoft: Add rumble support for Xbox One S controller

[Bastien Nocera]

Hey Florian,

On Wed, 2018-09-26 at 14:51 +0200, Dollinger Florian wrote:
> From: Florian Dollinger 
> 
> Hi there! Why do you re-engineer the wheel? :) There is already a
> fully functional and tested driver out there (
> https://github.com/atar-axis/xpadneo). Would be much easier to help
> me (the owner of xpadneo) to push it into the kernel.

Probably because he didn't know about it, and how would he? I also
didn't know about it, because it didn't exist last I worked on those
joypads.

I spent quite a bit of time trying to get the XBox One S controller
working over Bluetooth, without success, and I see that you have a
patch for that which you didn't send upstream either:
https://github.com/atar-axis/xpadneo/blob/master/misc/kernel_patches/0001-fix_bluetooth_reconnect.patch

I can imagine that a large portion of the driver can be integrated in
the existing XBox pad driver, with each feature added in individual
patches.

If I get the time, there are good chances I will send a patch to
integrate the battery reporting in the existing driver at least, and
then add support for missing buttons if there's a problem there (I see
that mentioned in the README).

"Trigger Force Feedback" is likely something that would need to be
integrated at a lower level, this is probably not something we'd want
to have replicated in each driver.

Cheers

Re: [PATCH] hid: microsoft: Add rumble support for Xbox One S controller

[Bastien Nocera]

On Mon, 2018-08-13 at 07:37 -0700, Andrey Smirnov wrote:
> On Fri, Aug 10, 2018 at 4:38 AM Bastien Nocera 
> wrote:
> > 
> > On Thu, 2018-08-09 at 17:17 -0700, Andrey Smirnov wrote:
> > > Add HID quirk driver for Xbox One S controller over bluetooth.
> > > 
> > > This driver only adds support for rumble. Standard controller
> > > functionality is exposed by default HID driver.
> > 
> > Did you manage to make the joypad work without hacks in the
> > Bluetooth
> > stack[1]?
> 
> I was not aware this hack actually existed, but now that I see it I
> think it explains why doing
> 
> echo 1 > /sys/module/bluetooth/parameters/disable_ertm
> 
> was necessary to make the controller connect over Bluetooth on my
> machine. It looks like disabling ERTM just happens to have the same
> effect as the hack that you mention. I'd like/plan to look into
> finding a proper solution to replace that hack, but that'd probably
> be
> a separate patch.

I gave it a try a couple of months back, but without much success.
L2CAP is unfortunately a bit too low-level to know anything about the
device which requested this. I think that some level of "STREAMING"
support is needed for the Bluetooth audio portion of the device.

Marcel was also interested in looking into the problem, but didn't get
to it. Let me know if you manage to get anything working without the
big hammer of disabling ERTM support for everything.

Cheers

Re: [PATCH] hid: microsoft: Add rumble support for Xbox One S controller

[Bastien Nocera]

On Mon, 2018-08-13 at 07:37 -0700, Andrey Smirnov wrote:
> On Fri, Aug 10, 2018 at 4:38 AM Bastien Nocera 
> wrote:
> > 
> > On Thu, 2018-08-09 at 17:17 -0700, Andrey Smirnov wrote:
> > > Add HID quirk driver for Xbox One S controller over bluetooth.
> > > 
> > > This driver only adds support for rumble. Standard controller
> > > functionality is exposed by default HID driver.
> > 
> > Did you manage to make the joypad work without hacks in the
> > Bluetooth
> > stack[1]?
> 
> I was not aware this hack actually existed, but now that I see it I
> think it explains why doing
> 
> echo 1 > /sys/module/bluetooth/parameters/disable_ertm
> 
> was necessary to make the controller connect over Bluetooth on my
> machine. It looks like disabling ERTM just happens to have the same
> effect as the hack that you mention. I'd like/plan to look into
> finding a proper solution to replace that hack, but that'd probably
> be
> a separate patch.

I gave it a try a couple of months back, but without much success.
L2CAP is unfortunately a bit too low-level to know anything about the
device which requested this. I think that some level of "STREAMING"
support is needed for the Bluetooth audio portion of the device.

Marcel was also interested in looking into the problem, but didn't get
to it. Let me know if you manage to get anything working without the
big hammer of disabling ERTM support for everything.

Cheers

Re: [PATCH] hid: microsoft: Add rumble support for Xbox One S controller

[Bastien Nocera]

On Thu, 2018-08-09 at 17:17 -0700, Andrey Smirnov wrote:
> Add HID quirk driver for Xbox One S controller over bluetooth.
> 
> This driver only adds support for rumble. Standard controller
> functionality is exposed by default HID driver.

Did you manage to make the joypad work without hacks in the Bluetooth
stack[1]?

[1]: 
https://github.com/ValveSoftware/steamos_kernel/commit/549c3dc10fa3749b3999549a2672b14bf0e9786d

Re: [PATCH] hid: microsoft: Add rumble support for Xbox One S controller

[Bastien Nocera]

On Thu, 2018-08-09 at 17:17 -0700, Andrey Smirnov wrote:
> Add HID quirk driver for Xbox One S controller over bluetooth.
> 
> This driver only adds support for rumble. Standard controller
> functionality is exposed by default HID driver.

Did you manage to make the joypad work without hacks in the Bluetooth
stack[1]?

[1]: 
https://github.com/ValveSoftware/steamos_kernel/commit/549c3dc10fa3749b3999549a2672b14bf0e9786d

Re: staging: rtl8723bs: bug or pointless if else ?

[Bastien Nocera]

On Thu, 2018-06-28 at 10:22 +0200, Hans de Goede wrote:
> Hi,
> 
> On 28-06-18 09:43, Michael Straube wrote:
> > Hi,
> > 
> > I stumbled upon the following if else construct in
> > drivers/staging/rtl8723bs/os_dep/sdio_intf.c:618
> > 
> >  if (pwrpriv->bInternalAutoSuspend)
> >  {
> >  ret = rtw_resume_process(padapter);
> >  }
> >  else
> >  {
> >  if (pwrpriv->wowlan_mode || pwrpriv-
> > >wowlan_ap_mode)
> >  {
> >  ret = rtw_resume_process(padapter);
> >  }
> >  else
> >  {
> >  ret = rtw_resume_process(padapter);
> >  }
> >  }
> > 
> > It does not matter if the conditions are true or not,
> > ret is always set to:
> > 
> > ret = rtw_resume_process(padapter)
> > 
> > Is this a bug or is the if else construct just pointless?
> 
> It probably is just pointless, my guess would be that once
> upon a time there was a difference in the paths and at some
> point that difference went away.

Quite:
https://github.com/hadess/rtl8723bs/blob/7d36e26f78bbc709844c12ad0c62e3e8503fdbc5/os_dep/linux/sdio_intf.c#L1757

Re: staging: rtl8723bs: bug or pointless if else ?

[Bastien Nocera]

On Thu, 2018-06-28 at 10:22 +0200, Hans de Goede wrote:
> Hi,
> 
> On 28-06-18 09:43, Michael Straube wrote:
> > Hi,
> > 
> > I stumbled upon the following if else construct in
> > drivers/staging/rtl8723bs/os_dep/sdio_intf.c:618
> > 
> >  if (pwrpriv->bInternalAutoSuspend)
> >  {
> >  ret = rtw_resume_process(padapter);
> >  }
> >  else
> >  {
> >  if (pwrpriv->wowlan_mode || pwrpriv-
> > >wowlan_ap_mode)
> >  {
> >  ret = rtw_resume_process(padapter);
> >  }
> >  else
> >  {
> >  ret = rtw_resume_process(padapter);
> >  }
> >  }
> > 
> > It does not matter if the conditions are true or not,
> > ret is always set to:
> > 
> > ret = rtw_resume_process(padapter)
> > 
> > Is this a bug or is the if else construct just pointless?
> 
> It probably is just pointless, my guess would be that once
> upon a time there was a difference in the paths and at some
> point that difference went away.

Quite:
https://github.com/hadess/rtl8723bs/blob/7d36e26f78bbc709844c12ad0c62e3e8503fdbc5/os_dep/linux/sdio_intf.c#L1757

Re: [PATCH 1/2] platform/x86: asus-wmi: Call new led hw_changed API on kbd brightness change

[Bastien Nocera]

On Tue, 2018-06-05 at 12:05 +0200, Hans de Goede wrote:
> Hi,
> 
> On 05-06-18 11:58, Bastien Nocera wrote:
> > On Tue, 2018-06-05 at 09:37 +0200, Hans de Goede wrote:
> > > Hi,
> > > 
> > > On 05-06-18 05:18, Chris Chiu wrote:
> > > > On Tue, Jun 5, 2018 at 10:31 AM, Darren Hart  > > > org>
> > > > wrote:
> > > > > On Mon, Jun 04, 2018 at 04:23:04PM +0200, Hans de Goede
> > > > > wrote:
> > > > > > Hi,
> > > > > > 
> > > > > > On 04-06-18 15:51, Daniel Drake wrote:
> > > > > > > On Mon, Jun 4, 2018 at 7:22 AM, Hans de Goede  > > > > > > edha
> > > > > > > t.com> wrote:
> > > > > > > > Is this really a case of the hardware itself processing
> > > > > > > > the
> > > > > > > > keypress and then changing the brightness *itself* ?
> > > > > > > > 
> > > > > > > >From the "[PATCH 2/2] platform/x86: asus-wmi: Add
> > > > > > > > keyboard backlight
> > > > > > > > toggle support" patch I get the impression that the
> > > > > > > > driver
> > > > > > > > is
> > > > > > > > modifying the brightness from within the kernel rather
> > > > > > > > then
> > > > > > > > the
> > > > > > > > keyboard controller are ACPI embeddec-controller doing
> > > > > > > > it
> > > > > > > > itself.
> > > > > > > > 
> > > > > > > > If that is the case then the right fix is for the
> > > > > > > > driver to
> > > > > > > > stop
> > > > > > > > mucking with the brighness itself, it should simply
> > > > > > > > report
> > > > > > > > the
> > > > > > > > right keyboard events and export a led interface and
> > > > > > > > then
> > > > > > > > userspace
> > > > > > > > will do the right thing (and be able to offer flexible
> > > > > > > > policies
> > > > > > > > to the user).
> > > > > > > 
> > > > > > > Before this modification, the driver reports the
> > > > > > > brightness
> > > > > > > keypresses
> > > > > > > to userspace and then userspace can respond by changing
> > > > > > > the
> > > > > > > brightness
> > > > > > > level, as you describe.
> > > > > > > 
> > > > > > > You are right in that the hardware doesn't change the
> > > > > > > brightness
> > > > > > > directly itself, which is the normal usage of
> > > > > > > LED_BRIGHT_HW_CHANGED.
> > > > > > > 
> > > > > > > However this approach was suggested by Benjamin Berg and
> > > > > > > Bastien
> > > > > > > Nocera in the thread: Re: [PATCH v2] platform/x86: asus-
> > > > > > > wmi:
> > > > > > > Add
> > > > > > > keyboard backlight toggle support
> > > > > > > https://marc.info/?l=linux-kernel=152639169210655=2
> > > > > > > 
> > > > > > > The issue is that we need to support a new "keyboard
> > > > > > > backlight
> > > > > > > brightness cycle" key (in the patch that follows this
> > > > > > > one)
> > > > > > > which
> > > > > > > doesn't fit into any definitions of keys recognised by
> > > > > > > the
> > > > > > > kernel and
> > > > > > > likewise there's no userspace code to handle it.
> > > > > > > 
> > > > > > > If preferred we could leave the standard brightness keys
> > > > > > > behaving as
> > > > > > > they are (input events) and make the new special key type
> > > > > > > directly
> > > > > > > handled by the kernel?
> > > > > > 
> > > > > > I'm sorry that Benjamin and Bastien steered you in this
> > > >

Re: [PATCH 1/2] platform/x86: asus-wmi: Call new led hw_changed API on kbd brightness change

[Bastien Nocera]

On Tue, 2018-06-05 at 12:05 +0200, Hans de Goede wrote:
> Hi,
> 
> On 05-06-18 11:58, Bastien Nocera wrote:
> > On Tue, 2018-06-05 at 09:37 +0200, Hans de Goede wrote:
> > > Hi,
> > > 
> > > On 05-06-18 05:18, Chris Chiu wrote:
> > > > On Tue, Jun 5, 2018 at 10:31 AM, Darren Hart  > > > org>
> > > > wrote:
> > > > > On Mon, Jun 04, 2018 at 04:23:04PM +0200, Hans de Goede
> > > > > wrote:
> > > > > > Hi,
> > > > > > 
> > > > > > On 04-06-18 15:51, Daniel Drake wrote:
> > > > > > > On Mon, Jun 4, 2018 at 7:22 AM, Hans de Goede  > > > > > > edha
> > > > > > > t.com> wrote:
> > > > > > > > Is this really a case of the hardware itself processing
> > > > > > > > the
> > > > > > > > keypress and then changing the brightness *itself* ?
> > > > > > > > 
> > > > > > > >From the "[PATCH 2/2] platform/x86: asus-wmi: Add
> > > > > > > > keyboard backlight
> > > > > > > > toggle support" patch I get the impression that the
> > > > > > > > driver
> > > > > > > > is
> > > > > > > > modifying the brightness from within the kernel rather
> > > > > > > > then
> > > > > > > > the
> > > > > > > > keyboard controller are ACPI embeddec-controller doing
> > > > > > > > it
> > > > > > > > itself.
> > > > > > > > 
> > > > > > > > If that is the case then the right fix is for the
> > > > > > > > driver to
> > > > > > > > stop
> > > > > > > > mucking with the brighness itself, it should simply
> > > > > > > > report
> > > > > > > > the
> > > > > > > > right keyboard events and export a led interface and
> > > > > > > > then
> > > > > > > > userspace
> > > > > > > > will do the right thing (and be able to offer flexible
> > > > > > > > policies
> > > > > > > > to the user).
> > > > > > > 
> > > > > > > Before this modification, the driver reports the
> > > > > > > brightness
> > > > > > > keypresses
> > > > > > > to userspace and then userspace can respond by changing
> > > > > > > the
> > > > > > > brightness
> > > > > > > level, as you describe.
> > > > > > > 
> > > > > > > You are right in that the hardware doesn't change the
> > > > > > > brightness
> > > > > > > directly itself, which is the normal usage of
> > > > > > > LED_BRIGHT_HW_CHANGED.
> > > > > > > 
> > > > > > > However this approach was suggested by Benjamin Berg and
> > > > > > > Bastien
> > > > > > > Nocera in the thread: Re: [PATCH v2] platform/x86: asus-
> > > > > > > wmi:
> > > > > > > Add
> > > > > > > keyboard backlight toggle support
> > > > > > > https://marc.info/?l=linux-kernel=152639169210655=2
> > > > > > > 
> > > > > > > The issue is that we need to support a new "keyboard
> > > > > > > backlight
> > > > > > > brightness cycle" key (in the patch that follows this
> > > > > > > one)
> > > > > > > which
> > > > > > > doesn't fit into any definitions of keys recognised by
> > > > > > > the
> > > > > > > kernel and
> > > > > > > likewise there's no userspace code to handle it.
> > > > > > > 
> > > > > > > If preferred we could leave the standard brightness keys
> > > > > > > behaving as
> > > > > > > they are (input events) and make the new special key type
> > > > > > > directly
> > > > > > > handled by the kernel?
> > > > > > 
> > > > > > I'm sorry that Benjamin and Bastien steered you in this
> > > >

Re: [PATCH 1/2] platform/x86: asus-wmi: Call new led hw_changed API on kbd brightness change

[Bastien Nocera]

On Tue, 2018-06-05 at 09:37 +0200, Hans de Goede wrote:
> Hi,
> 
> On 05-06-18 05:18, Chris Chiu wrote:
> > On Tue, Jun 5, 2018 at 10:31 AM, Darren Hart 
> > wrote:
> > > On Mon, Jun 04, 2018 at 04:23:04PM +0200, Hans de Goede wrote:
> > > > Hi,
> > > > 
> > > > On 04-06-18 15:51, Daniel Drake wrote:
> > > > > On Mon, Jun 4, 2018 at 7:22 AM, Hans de Goede  > > > > t.com> wrote:
> > > > > > Is this really a case of the hardware itself processing the
> > > > > > keypress and then changing the brightness *itself* ?
> > > > > > 
> > > > > >   From the "[PATCH 2/2] platform/x86: asus-wmi: Add
> > > > > > keyboard backlight
> > > > > > toggle support" patch I get the impression that the driver
> > > > > > is
> > > > > > modifying the brightness from within the kernel rather then
> > > > > > the
> > > > > > keyboard controller are ACPI embeddec-controller doing it
> > > > > > itself.
> > > > > > 
> > > > > > If that is the case then the right fix is for the driver to
> > > > > > stop
> > > > > > mucking with the brighness itself, it should simply report
> > > > > > the
> > > > > > right keyboard events and export a led interface and then
> > > > > > userspace
> > > > > > will do the right thing (and be able to offer flexible
> > > > > > policies
> > > > > > to the user).
> > > > > 
> > > > > Before this modification, the driver reports the brightness
> > > > > keypresses
> > > > > to userspace and then userspace can respond by changing the
> > > > > brightness
> > > > > level, as you describe.
> > > > > 
> > > > > You are right in that the hardware doesn't change the
> > > > > brightness
> > > > > directly itself, which is the normal usage of
> > > > > LED_BRIGHT_HW_CHANGED.
> > > > > 
> > > > > However this approach was suggested by Benjamin Berg and
> > > > > Bastien
> > > > > Nocera in the thread: Re: [PATCH v2] platform/x86: asus-wmi:
> > > > > Add
> > > > > keyboard backlight toggle support
> > > > > https://marc.info/?l=linux-kernel=152639169210655=2
> > > > > 
> > > > > The issue is that we need to support a new "keyboard
> > > > > backlight
> > > > > brightness cycle" key (in the patch that follows this one)
> > > > > which
> > > > > doesn't fit into any definitions of keys recognised by the
> > > > > kernel and
> > > > > likewise there's no userspace code to handle it.
> > > > > 
> > > > > If preferred we could leave the standard brightness keys
> > > > > behaving as
> > > > > they are (input events) and make the new special key type
> > > > > directly
> > > > > handled by the kernel?
> > > > 
> > > > I'm sorry that Benjamin and Bastien steered you in this
> > > > direction,
> > > > IMHO none of it should be handled in the kernel.
> > > > 
> > > > Anytime any sort of input is directly responded to by the
> > > > kernel
> > > > it is a huge PITA to deal with from userspace. The kernel will
> > > > have
> > > > a simplistic implementation which almost always is wrong.
> > > > 
> > > > Benjamin, remember the pain we went through with rfkill hotkey
> > > > presses being handled in the kernel ?
> > > > 
> > > > And then there is the whole
> > > > acpi_video.brightness_switch_enabled
> > > > debacle, which is an option which defaults to true which causes
> > > > the kernel to handle LCD brightness key presses, which all
> > > > distros
> > > > have been patching to default to off for ages.
> > > > 
> > > > To give a concrete example, we may want to implement software
> > > > dimming / auto-off of the kbd backlight when the no keys are
> > > > touched for x seconds. This would seriously get in the way of
> > > > that.
> > > > 
> > > > So sorry, but NACK to this series.
> > > 
> > > So if instead of modifying the LED v

Re: [PATCH 1/2] platform/x86: asus-wmi: Call new led hw_changed API on kbd brightness change

[Bastien Nocera]

On Tue, 2018-06-05 at 09:37 +0200, Hans de Goede wrote:
> Hi,
> 
> On 05-06-18 05:18, Chris Chiu wrote:
> > On Tue, Jun 5, 2018 at 10:31 AM, Darren Hart 
> > wrote:
> > > On Mon, Jun 04, 2018 at 04:23:04PM +0200, Hans de Goede wrote:
> > > > Hi,
> > > > 
> > > > On 04-06-18 15:51, Daniel Drake wrote:
> > > > > On Mon, Jun 4, 2018 at 7:22 AM, Hans de Goede  > > > > t.com> wrote:
> > > > > > Is this really a case of the hardware itself processing the
> > > > > > keypress and then changing the brightness *itself* ?
> > > > > > 
> > > > > >   From the "[PATCH 2/2] platform/x86: asus-wmi: Add
> > > > > > keyboard backlight
> > > > > > toggle support" patch I get the impression that the driver
> > > > > > is
> > > > > > modifying the brightness from within the kernel rather then
> > > > > > the
> > > > > > keyboard controller are ACPI embeddec-controller doing it
> > > > > > itself.
> > > > > > 
> > > > > > If that is the case then the right fix is for the driver to
> > > > > > stop
> > > > > > mucking with the brighness itself, it should simply report
> > > > > > the
> > > > > > right keyboard events and export a led interface and then
> > > > > > userspace
> > > > > > will do the right thing (and be able to offer flexible
> > > > > > policies
> > > > > > to the user).
> > > > > 
> > > > > Before this modification, the driver reports the brightness
> > > > > keypresses
> > > > > to userspace and then userspace can respond by changing the
> > > > > brightness
> > > > > level, as you describe.
> > > > > 
> > > > > You are right in that the hardware doesn't change the
> > > > > brightness
> > > > > directly itself, which is the normal usage of
> > > > > LED_BRIGHT_HW_CHANGED.
> > > > > 
> > > > > However this approach was suggested by Benjamin Berg and
> > > > > Bastien
> > > > > Nocera in the thread: Re: [PATCH v2] platform/x86: asus-wmi:
> > > > > Add
> > > > > keyboard backlight toggle support
> > > > > https://marc.info/?l=linux-kernel=152639169210655=2
> > > > > 
> > > > > The issue is that we need to support a new "keyboard
> > > > > backlight
> > > > > brightness cycle" key (in the patch that follows this one)
> > > > > which
> > > > > doesn't fit into any definitions of keys recognised by the
> > > > > kernel and
> > > > > likewise there's no userspace code to handle it.
> > > > > 
> > > > > If preferred we could leave the standard brightness keys
> > > > > behaving as
> > > > > they are (input events) and make the new special key type
> > > > > directly
> > > > > handled by the kernel?
> > > > 
> > > > I'm sorry that Benjamin and Bastien steered you in this
> > > > direction,
> > > > IMHO none of it should be handled in the kernel.
> > > > 
> > > > Anytime any sort of input is directly responded to by the
> > > > kernel
> > > > it is a huge PITA to deal with from userspace. The kernel will
> > > > have
> > > > a simplistic implementation which almost always is wrong.
> > > > 
> > > > Benjamin, remember the pain we went through with rfkill hotkey
> > > > presses being handled in the kernel ?
> > > > 
> > > > And then there is the whole
> > > > acpi_video.brightness_switch_enabled
> > > > debacle, which is an option which defaults to true which causes
> > > > the kernel to handle LCD brightness key presses, which all
> > > > distros
> > > > have been patching to default to off for ages.
> > > > 
> > > > To give a concrete example, we may want to implement software
> > > > dimming / auto-off of the kbd backlight when the no keys are
> > > > touched for x seconds. This would seriously get in the way of
> > > > that.
> > > > 
> > > > So sorry, but NACK to this series.
> > > 
> > > So if instead of modifying the LED v

Re: [PATCH][staging-next] staging: rtl8723bs: spelling mistake: "dismatch" -> "mismatch"

[Bastien Nocera]

On Mon, 2018-04-30 at 15:19 +0100, Colin King wrote:
> +   P2P_STATE_RECV_INVITE_REQ_DISMATCH =
> 17,/*  receiving the P2P Inviation request and mismatch
> with the profile. */

Might as well fix the "inviation" as well, no? :)

Thanks

Re: [PATCH][staging-next] staging: rtl8723bs: spelling mistake: "dismatch" -> "mismatch"

[Bastien Nocera]

On Mon, 2018-04-30 at 15:19 +0100, Colin King wrote:
> +   P2P_STATE_RECV_INVITE_REQ_DISMATCH =
> 17,/*  receiving the P2P Inviation request and mismatch
> with the profile. */

Might as well fix the "inviation" as well, no? :)

Thanks

Re: [PATCH] Resolve RF Type mismatch

[Bastien Nocera]

On Wed, 2018-02-07 at 00:57 +0900, Kangmin Park wrote:
> From: pr0gr4m 

This needs a commit message. I don't understand what the code is trying
to do.

Re: [PATCH] Resolve RF Type mismatch

[Bastien Nocera]

On Wed, 2018-02-07 at 00:57 +0900, Kangmin Park wrote:
> From: pr0gr4m 

This needs a commit message. I don't understand what the code is trying
to do.

Re: [RFC PATCH] input: Add disable sysfs entry for every input device

[Bastien Nocera]

On Tue, 2018-01-02 at 22:54 +0100, Pali Rohár wrote:
> On Wednesday 04 January 2017 15:37:35 Bastien Nocera wrote:
> > I don't doubt that the use cases should be catered for, I
> > essentially
> > did that same work without kernel changes for GNOME. What I doubt
> > is
> > the fuzzy semantics, the fact that the device is kept opened but no
> > data is sent (that's not power saving), that whether users are
> > revoked
> > or should be revoked isn't clear, and that the goal is basically to
> > work around stupid input handling when at the console. When running
> > a
> > display manager, this is all avoided.
> > 
> > If this were to go through, then the semantics and behaviour needs
> > to
> > be better explained, power saving actually made possible, and make
> > sure
> > that libinput can proxy that state to the users on the console. Or
> > an
> > ioctl added to the evdev device to disable them.
> 
> So, do you mean to implement this "disable" action as ioctl for
> particular /dev/input/event* device (instead of sysfs entry)?

Yes, so the device can be powered down without the device node being
closed and made unavailable. I don't know whether that's something
that's already possible for all cases, but there's already
opportunistic in a lot of drivers and subsystems.

This opens up a whole new wave of potential problems, but it's a more
generally useful mechanism, I would think.

Re: [RFC PATCH] input: Add disable sysfs entry for every input device

[Bastien Nocera]

On Tue, 2018-01-02 at 22:54 +0100, Pali Rohár wrote:
> On Wednesday 04 January 2017 15:37:35 Bastien Nocera wrote:
> > I don't doubt that the use cases should be catered for, I
> > essentially
> > did that same work without kernel changes for GNOME. What I doubt
> > is
> > the fuzzy semantics, the fact that the device is kept opened but no
> > data is sent (that's not power saving), that whether users are
> > revoked
> > or should be revoked isn't clear, and that the goal is basically to
> > work around stupid input handling when at the console. When running
> > a
> > display manager, this is all avoided.
> > 
> > If this were to go through, then the semantics and behaviour needs
> > to
> > be better explained, power saving actually made possible, and make
> > sure
> > that libinput can proxy that state to the users on the console. Or
> > an
> > ioctl added to the evdev device to disable them.
> 
> So, do you mean to implement this "disable" action as ioctl for
> particular /dev/input/event* device (instead of sysfs entry)?

Yes, so the device can be powered down without the device node being
closed and made unavailable. I don't know whether that's something
that's already possible for all cases, but there's already
opportunistic in a lot of drivers and subsystems.

This opens up a whole new wave of potential problems, but it's a more
generally useful mechanism, I would think.

Re: [RFC PATCH] input: Add disable sysfs entry for every input device

[Bastien Nocera]

On Tue, 2018-01-02 at 22:48 +0100, Pali Rohár wrote:
> On Tuesday 03 January 2017 12:21:21 Bastien Nocera wrote:
> > On Mon, 2017-01-02 at 18:09 +0100, Pali Rohár wrote:
> > > On Monday 02 January 2017 16:27:05 Bastien Nocera wrote:
> > > > On Sun, 2016-12-25 at 11:04 +0100, Pali Rohár wrote:
> > > > > This patch allows user to disable events from any input
> > > > > device so
> > > > > events
> > > > > would not be delivered to userspace.
> > > > >  
> > > > > Currently there is no way to disable particular input device
> > > > > by
> > > > > kernel.
> > > > > User for different reasons would need it for integrated PS/2
> > > > > keyboard or
> > > > > touchpad in notebook or touchscreen on mobile device to
> > > > > prevent
> > > > > sending
> > > > > events. E.g. mobile phone in pocket or broken integrated PS/2
> > > > > keyboard.
> > > > >  
> > > > > This is just a RFC patch, not tested yet. Original post about
> > > > > motivation
> > > > > about this patch is there: https://lkml.org/lkml/2014/11/29/9
> > > > > 2
> > > > 
> > > >  
> > > > Having implemented something of that ilk in user-space (we
> > > > automatically disable touch devices when the associated screen
> > > > is
> > > > turned off/suspended), I think this might need more thought.
> > > 
> > > How to implement such thing in userspace? I think you cannot do
> > > that 
> > > without rewriting every one userspace application which uses
> > > input.
> > > 
> > > > What happens when a device is opened and the device disabled
> > > 
> > > through
> > > > sysfs, are the users revoked?
> > > 
> > > Applications will not receive events. Same as if input device
> > > does
> > > not 
> > > generates events.
> > > 
> > > > Does this put the device in suspend in the same way that
> > > > closing
> > > 
> > > the
> > > > device's last user does?
> > > 
> > > Current code not (this is just RFC prototype), but it should be
> > > possible 
> > > to implement.
> > > 
> > > > Is this not better implemented in user-space at the session
> > > > level,
> > > > where it knows about which output corresponds to which input
> > > 
> > > device?
> > > 
> > > How to do that without rewriting existing applications?
> > > 
> > > > Is this useful enough to disable misbehaving devices on
> > > > hardware,
> > > 
> > > so
> > > > that the device is not effective on boot?  
> > > 
> > > In case integrated device is absolutely unusable and generates
> > > always 
> > > random events, it does not solve problem at boot time.
> > > 
> > > But more real case is laptop with closed LID press buttons and
> > > here
> > > it 
> > > is useful.
> > 
> > There's usually a display manager in between the application and
> > the
> > input device.
> 
> But that is not always truth. In some cases there are applications
> which
> opens input device directly.

Which is why I said "usually", and not "always".

> > Whether it's X.org, or a Wayland compositor. Even David's
> >  https://github.com/dvdhrm/kmscon could help for console
> > applications.
> 
> That kmscon needs KMS, right? So it would not work on hardware which
> do
> not have KMS drivers.

Except that the use cases are getting smaller and smaller. At one
point, it might become easier to carry that patch in your tree, for
your use case.

It's not useful on desktop Linux, it's not useful for Android or Chrome
OS. It seems it's only really useful for the console when there's no
way to use a display manager between the console subsystem and the
"UI".

At this point I'd ask whether whatever is consuming those buttons can't
be modified instead of the kernel. You'd even get the benefit of being
able to close devices and save some power.

Re: [RFC PATCH] input: Add disable sysfs entry for every input device

[Bastien Nocera]

On Tue, 2018-01-02 at 22:48 +0100, Pali Rohár wrote:
> On Tuesday 03 January 2017 12:21:21 Bastien Nocera wrote:
> > On Mon, 2017-01-02 at 18:09 +0100, Pali Rohár wrote:
> > > On Monday 02 January 2017 16:27:05 Bastien Nocera wrote:
> > > > On Sun, 2016-12-25 at 11:04 +0100, Pali Rohár wrote:
> > > > > This patch allows user to disable events from any input
> > > > > device so
> > > > > events
> > > > > would not be delivered to userspace.
> > > > >  
> > > > > Currently there is no way to disable particular input device
> > > > > by
> > > > > kernel.
> > > > > User for different reasons would need it for integrated PS/2
> > > > > keyboard or
> > > > > touchpad in notebook or touchscreen on mobile device to
> > > > > prevent
> > > > > sending
> > > > > events. E.g. mobile phone in pocket or broken integrated PS/2
> > > > > keyboard.
> > > > >  
> > > > > This is just a RFC patch, not tested yet. Original post about
> > > > > motivation
> > > > > about this patch is there: https://lkml.org/lkml/2014/11/29/9
> > > > > 2
> > > > 
> > > >  
> > > > Having implemented something of that ilk in user-space (we
> > > > automatically disable touch devices when the associated screen
> > > > is
> > > > turned off/suspended), I think this might need more thought.
> > > 
> > > How to implement such thing in userspace? I think you cannot do
> > > that 
> > > without rewriting every one userspace application which uses
> > > input.
> > > 
> > > > What happens when a device is opened and the device disabled
> > > 
> > > through
> > > > sysfs, are the users revoked?
> > > 
> > > Applications will not receive events. Same as if input device
> > > does
> > > not 
> > > generates events.
> > > 
> > > > Does this put the device in suspend in the same way that
> > > > closing
> > > 
> > > the
> > > > device's last user does?
> > > 
> > > Current code not (this is just RFC prototype), but it should be
> > > possible 
> > > to implement.
> > > 
> > > > Is this not better implemented in user-space at the session
> > > > level,
> > > > where it knows about which output corresponds to which input
> > > 
> > > device?
> > > 
> > > How to do that without rewriting existing applications?
> > > 
> > > > Is this useful enough to disable misbehaving devices on
> > > > hardware,
> > > 
> > > so
> > > > that the device is not effective on boot?  
> > > 
> > > In case integrated device is absolutely unusable and generates
> > > always 
> > > random events, it does not solve problem at boot time.
> > > 
> > > But more real case is laptop with closed LID press buttons and
> > > here
> > > it 
> > > is useful.
> > 
> > There's usually a display manager in between the application and
> > the
> > input device.
> 
> But that is not always truth. In some cases there are applications
> which
> opens input device directly.

Which is why I said "usually", and not "always".

> > Whether it's X.org, or a Wayland compositor. Even David's
> >  https://github.com/dvdhrm/kmscon could help for console
> > applications.
> 
> That kmscon needs KMS, right? So it would not work on hardware which
> do
> not have KMS drivers.

Except that the use cases are getting smaller and smaller. At one
point, it might become easier to carry that patch in your tree, for
your use case.

It's not useful on desktop Linux, it's not useful for Android or Chrome
OS. It seems it's only really useful for the console when there's no
way to use a display manager between the console subsystem and the
"UI".

At this point I'd ask whether whatever is consuming those buttons can't
be modified instead of the kernel. You'd even get the benefit of being
able to close devices and save some power.

Re: [RFC PATCH] input: Add disable sysfs entry for every input device

[Bastien Nocera]

On Tue, 2017-01-17 at 12:07 +0100, Pavel Machek wrote:
> 

> We'd really like it to work on plain old text console, too, because
> that's what
> I'm using on n900, and with old maemo userspace, because that's what
> everyone
> else uses.
> 
> You mentioned you think X.org can do this kind of device disabling
> (and powersave).
> Would you have an idea how to activate that? I'd like to disable
> touchscreen
> and most of the buttons when the device is "in the pocket".

You need to change the "Device Enabled" property in X. What this does
depends on which Xorg driver it uses.

Re: [RFC PATCH] input: Add disable sysfs entry for every input device

[Bastien Nocera]

On Tue, 2017-01-17 at 12:07 +0100, Pavel Machek wrote:
> 

> We'd really like it to work on plain old text console, too, because
> that's what
> I'm using on n900, and with old maemo userspace, because that's what
> everyone
> else uses.
> 
> You mentioned you think X.org can do this kind of device disabling
> (and powersave).
> Would you have an idea how to activate that? I'd like to disable
> touchscreen
> and most of the buttons when the device is "in the pocket".

You need to change the "Device Enabled" property in X. What this does
depends on which Xorg driver it uses.

Re: [PATCH] Bluettoth: btusb: Prevent USB devices to autosuspend while setting interface

[Bastien Nocera]

On Wed, 2017-11-15 at 14:54 +0100, Marcel Holtmann wrote:
> Hi Abhijeet,
> 
> > Runtime resume USB device in order to ensure that PM framework
> > knows
> > that the we might be using the device in a short time and doesn't
> > autosuspend the device while we update it's interface.
> > 
> > Signed-off-by: Abhijeet Kumar 
> > ---
> > drivers/bluetooth/btusb.c | 6 ++
> > 1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > index 7a5c06aaa181..588aabf991be 100644
> > --- a/drivers/bluetooth/btusb.c
> > +++ b/drivers/bluetooth/btusb.c
> > @@ -1444,6 +1444,12 @@ static void btusb_work(struct work_struct
> > *work)
> > data->sco_skb = NULL;
> > spin_unlock_irqrestore(>rxlock, flags);
> > 
> > +/*
> > + *Letting runtime PM know that we wish to
> > use
> > + *the device in a short time.
> > + */
> > + pm_runtime_get(>udev->dev);
> > +
> 
> the indentation is wrong here and the comment coding style if off.
> Please fix.

And there's a typo in the commit message ("its interface").

Re: [PATCH] Bluettoth: btusb: Prevent USB devices to autosuspend while setting interface

[Bastien Nocera]

On Wed, 2017-11-15 at 14:54 +0100, Marcel Holtmann wrote:
> Hi Abhijeet,
> 
> > Runtime resume USB device in order to ensure that PM framework
> > knows
> > that the we might be using the device in a short time and doesn't
> > autosuspend the device while we update it's interface.
> > 
> > Signed-off-by: Abhijeet Kumar 
> > ---
> > drivers/bluetooth/btusb.c | 6 ++
> > 1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > index 7a5c06aaa181..588aabf991be 100644
> > --- a/drivers/bluetooth/btusb.c
> > +++ b/drivers/bluetooth/btusb.c
> > @@ -1444,6 +1444,12 @@ static void btusb_work(struct work_struct
> > *work)
> > data->sco_skb = NULL;
> > spin_unlock_irqrestore(>rxlock, flags);
> > 
> > +/*
> > + *Letting runtime PM know that we wish to
> > use
> > + *the device in a short time.
> > + */
> > + pm_runtime_get(>udev->dev);
> > +
> 
> the indentation is wrong here and the comment coding style if off.
> Please fix.

And there's a typo in the commit message ("its interface").

Re: [PATCH v2 4/5] platform/x86: intel-vbtn: support KEY_ROTATE_LOCK_TOGGLE

[Bastien Nocera]

On Fri, 2017-11-10 at 01:15 +0100, Stefan Brüns wrote:
> On Friday, November 10, 2017 12:30:46 AM CET Bastien Nocera wrote:
> > On Thu, 2017-11-09 at 23:44 +0100, Stefan Brüns wrote:
> > > The Rotate Lock button event is emitted on the XPS 12 (BIOS A8,
> > > but
> > > not
> > > on BIOS A2).
> > > 
> > > Signed-off-by: Stefan Brüns <stefan.bru...@rwth-aachen.de>
> > > ---
> > > 
> > > Changes in v2:
> > > - Emit KEY_ROTATE_LOCK_TOGGLE instead of KEY_ROTATE_DISPLAY
> > > - Use separate up/down events
> > > 
> > >  drivers/platform/x86/intel-vbtn.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/drivers/platform/x86/intel-vbtn.c
> > > b/drivers/platform/x86/intel-vbtn.c index
> > > e3f6375af85c..a484bcc6393b
> > > 100644
> > > --- a/drivers/platform/x86/intel-vbtn.c
> > > +++ b/drivers/platform/x86/intel-vbtn.c
> > > @@ -42,6 +42,8 @@ static const struct key_entry
> > > intel_vbtn_keymap[] = {
> > > 
> > >   { KE_IGNORE, 0xC5, { KEY_VOLUMEUP } },  /*
> > > volume-up key release */
> > >   { KE_KEY, 0xC6, { KEY_VOLUMEDOWN } },   /*
> > > volume-down key press */
> > >   { KE_IGNORE, 0xC7, { KEY_VOLUMEDOWN } },/*
> > > volume-down key release 
> 
> */
> > > 
> > > + { KE_KEY,0xC8, { KEY_ROTATE_LOCK_TOGGLE } },
> > > /* rotate-lock key
> > > press */ +{ KE_KEY,0xC9, { KEY_ROTATE_LOCK_TOGGLE }
> > > },/*
> > > rotate-lock key release */
> > 
> > How are those events sent? When pressing and releasing the key, do
> > you
> > receive 0xC8 followed by 0xC9? Or do you receive 0xC8 when pressing
> > and
> > releasing the first time, and 0xC9 when pressing and releasing a
> > second
> > time?
> > 
> > If the former, then it's not going to work. The release is supposed
> > to
> > be ignored, as you send the event with
> > sparse_keymap_report_event().
> > 
> > If the latter, and there's an actual state, does it disable a
> > device
> > on-board, or activate an LED? If so, then it would need to be a
> > switch,
> > not a key.
> 
> Do you think I don't test the patches before sending? Let me tell
> you, it 
> *does* work.
> 
> You could also read the cover letter, where you find more details,
> putting the 
> patches in relation to each other.

I guess this was so clear that Darren made the same assumption as me. I
also never said that it wouldn't work, which is why I used "pretty
sure", and asked questions.

> Just in case its not yet clear:
> The codes are emitted when pressing a button. It is a button, not a
> switch. 
> There is no state handled in hardware. On press (as noted by the
> code 
> comment), event code 0xc8 is emitted. On release, event code 0xc9 is
> emitted.

>From the looks of things, some devices only send the keypresses, and
never the key release, otherwise what would be the point of ignoring
the key release in that table, say for the volume buttons? It's clear
as mud, and there's no comments in the driver to explain that.

If you don't want to answer questions, or are going to be as pissy as
your replies have been, I'm happy to not continue reviewing your
patches. I find patch 2 unreadable, but it might be my tiny tiny little
brain.

Re: [PATCH v2 4/5] platform/x86: intel-vbtn: support KEY_ROTATE_LOCK_TOGGLE

[Bastien Nocera]

On Fri, 2017-11-10 at 01:15 +0100, Stefan Brüns wrote:
> On Friday, November 10, 2017 12:30:46 AM CET Bastien Nocera wrote:
> > On Thu, 2017-11-09 at 23:44 +0100, Stefan Brüns wrote:
> > > The Rotate Lock button event is emitted on the XPS 12 (BIOS A8,
> > > but
> > > not
> > > on BIOS A2).
> > > 
> > > Signed-off-by: Stefan Brüns 
> > > ---
> > > 
> > > Changes in v2:
> > > - Emit KEY_ROTATE_LOCK_TOGGLE instead of KEY_ROTATE_DISPLAY
> > > - Use separate up/down events
> > > 
> > >  drivers/platform/x86/intel-vbtn.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/drivers/platform/x86/intel-vbtn.c
> > > b/drivers/platform/x86/intel-vbtn.c index
> > > e3f6375af85c..a484bcc6393b
> > > 100644
> > > --- a/drivers/platform/x86/intel-vbtn.c
> > > +++ b/drivers/platform/x86/intel-vbtn.c
> > > @@ -42,6 +42,8 @@ static const struct key_entry
> > > intel_vbtn_keymap[] = {
> > > 
> > >   { KE_IGNORE, 0xC5, { KEY_VOLUMEUP } },  /*
> > > volume-up key release */
> > >   { KE_KEY, 0xC6, { KEY_VOLUMEDOWN } },   /*
> > > volume-down key press */
> > >   { KE_IGNORE, 0xC7, { KEY_VOLUMEDOWN } },/*
> > > volume-down key release 
> 
> */
> > > 
> > > + { KE_KEY,0xC8, { KEY_ROTATE_LOCK_TOGGLE } },
> > > /* rotate-lock key
> > > press */ +{ KE_KEY,0xC9, { KEY_ROTATE_LOCK_TOGGLE }
> > > },/*
> > > rotate-lock key release */
> > 
> > How are those events sent? When pressing and releasing the key, do
> > you
> > receive 0xC8 followed by 0xC9? Or do you receive 0xC8 when pressing
> > and
> > releasing the first time, and 0xC9 when pressing and releasing a
> > second
> > time?
> > 
> > If the former, then it's not going to work. The release is supposed
> > to
> > be ignored, as you send the event with
> > sparse_keymap_report_event().
> > 
> > If the latter, and there's an actual state, does it disable a
> > device
> > on-board, or activate an LED? If so, then it would need to be a
> > switch,
> > not a key.
> 
> Do you think I don't test the patches before sending? Let me tell
> you, it 
> *does* work.
> 
> You could also read the cover letter, where you find more details,
> putting the 
> patches in relation to each other.

I guess this was so clear that Darren made the same assumption as me. I
also never said that it wouldn't work, which is why I used "pretty
sure", and asked questions.

> Just in case its not yet clear:
> The codes are emitted when pressing a button. It is a button, not a
> switch. 
> There is no state handled in hardware. On press (as noted by the
> code 
> comment), event code 0xc8 is emitted. On release, event code 0xc9 is
> emitted.

>From the looks of things, some devices only send the keypresses, and
never the key release, otherwise what would be the point of ignoring
the key release in that table, say for the volume buttons? It's clear
as mud, and there's no comments in the driver to explain that.

If you don't want to answer questions, or are going to be as pissy as
your replies have been, I'm happy to not continue reviewing your
patches. I find patch 2 unreadable, but it might be my tiny tiny little
brain.

Re: [PATCH v2 5/5] platform/x86: intel-vbtn: support panel front button

[Bastien Nocera]

On Thu, 2017-11-09 at 23:44 +0100, Stefan Brüns wrote:
> The Lenovo Helix 2 and Dell XPS 12 (9Q33) have an extra button on the
> front showing a 'Windows' logo, both reporting event codes 0xC2/0xC3
> on press/release. On the Dell, both press/release are distinct events
> while on the Helix 2 both events are generated on release.
> 
> Tested on XPS 12, for info on the Helix 2 see:
> https://www.spinics.net/lists/ibm-acpi-devel/msg03982.html
> 
> Signed-off-by: Stefan Brüns 

Same problem as mentioned in patch 3. Pretty sure you need to set the
Windows key release to KEY_IGNORE.

Or better, teach the intel-vbtn driver which buttons should be
autoreleased, and which ones should send key presses and key releases
separately. This would allow handling long presses in the upper layers.

> ---
> 
> Changes in v2:
> - Emit KEY_LEFTMETA instead of KEY_MENU
> 
>  drivers/platform/x86/intel-vbtn.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/platform/x86/intel-vbtn.c
> b/drivers/platform/x86/intel-vbtn.c
> index a484bcc6393b..0861efe490d0 100644
> --- a/drivers/platform/x86/intel-vbtn.c
> +++ b/drivers/platform/x86/intel-vbtn.c
> @@ -38,6 +38,8 @@ static const struct acpi_device_id intel_vbtn_ids[]
> = {
>  static const struct key_entry intel_vbtn_keymap[] = {
>   { KE_KEY, 0xC0, { KEY_POWER } },/* power key press
> */
>   { KE_IGNORE, 0xC1, { KEY_POWER } }, /* power key
> release */
> + { KE_KEY, 0xC2, { KEY_LEFTMETA } }, /*
> 'Windows' key press */
> + { KE_KEY, 0xC3, { KEY_LEFTMETA } }, /*
> 'Windows' key release */
>   { KE_KEY, 0xC4, { KEY_VOLUMEUP } }, /*
> volume-up key press */
>   { KE_IGNORE, 0xC5, { KEY_VOLUMEUP } },  /*
> volume-up key release */
>   { KE_KEY, 0xC6, { KEY_VOLUMEDOWN } },   /*
> volume-down key press */

Re: [PATCH v2 5/5] platform/x86: intel-vbtn: support panel front button

[Bastien Nocera]

On Thu, 2017-11-09 at 23:44 +0100, Stefan Brüns wrote:
> The Lenovo Helix 2 and Dell XPS 12 (9Q33) have an extra button on the
> front showing a 'Windows' logo, both reporting event codes 0xC2/0xC3
> on press/release. On the Dell, both press/release are distinct events
> while on the Helix 2 both events are generated on release.
> 
> Tested on XPS 12, for info on the Helix 2 see:
> https://www.spinics.net/lists/ibm-acpi-devel/msg03982.html
> 
> Signed-off-by: Stefan Brüns 

Same problem as mentioned in patch 3. Pretty sure you need to set the
Windows key release to KEY_IGNORE.

Or better, teach the intel-vbtn driver which buttons should be
autoreleased, and which ones should send key presses and key releases
separately. This would allow handling long presses in the upper layers.

> ---
> 
> Changes in v2:
> - Emit KEY_LEFTMETA instead of KEY_MENU
> 
>  drivers/platform/x86/intel-vbtn.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/platform/x86/intel-vbtn.c
> b/drivers/platform/x86/intel-vbtn.c
> index a484bcc6393b..0861efe490d0 100644
> --- a/drivers/platform/x86/intel-vbtn.c
> +++ b/drivers/platform/x86/intel-vbtn.c
> @@ -38,6 +38,8 @@ static const struct acpi_device_id intel_vbtn_ids[]
> = {
>  static const struct key_entry intel_vbtn_keymap[] = {
>   { KE_KEY, 0xC0, { KEY_POWER } },/* power key press
> */
>   { KE_IGNORE, 0xC1, { KEY_POWER } }, /* power key
> release */
> + { KE_KEY, 0xC2, { KEY_LEFTMETA } }, /*
> 'Windows' key press */
> + { KE_KEY, 0xC3, { KEY_LEFTMETA } }, /*
> 'Windows' key release */
>   { KE_KEY, 0xC4, { KEY_VOLUMEUP } }, /*
> volume-up key press */
>   { KE_IGNORE, 0xC5, { KEY_VOLUMEUP } },  /*
> volume-up key release */
>   { KE_KEY, 0xC6, { KEY_VOLUMEDOWN } },   /*
> volume-down key press */

Re: [PATCH v2 3/5] Input: add KEY_ROTATE_LOCK_TOGGLE

[Bastien Nocera]

On Thu, 2017-11-09 at 23:44 +0100, Stefan Brüns wrote:
> The key has the same use as the SW_ROTATE_LOCK, but is used on
> devices
> where the state is not tracked by the hardware but has to be handled
> in software.

I'll let the input and hid subsystem maintainers have the final say
here, though I think that sending out Win+O would be easier, as this is
already something that desktops need to support for those devices that
send the actual Win+O key combination when those buttons are pressed
there.

If we're going with the separate keycode, could you also file bugs
against xkbd-common and xkeyboard-config to get the key added to the
list of XF86 keysyms and to the default evdev keymap? Otherwise Wayland
and X11 based desktops won't be able to use it.

Cheers

> Signed-off-by: Stefan Brüns 
> 
> ---
> 
> Changes in v2:
> - New patch, add support for KEY_ROTATE_LOCK_TOGGLE
> 
>  include/uapi/linux/input-event-codes.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/uapi/linux/input-event-codes.h
> b/include/uapi/linux/input-event-codes.h
> index f4058bd4c373..ba5d709a8923 100644
> --- a/include/uapi/linux/input-event-codes.h
> +++ b/include/uapi/linux/input-event-codes.h
> @@ -593,6 +593,7 @@
>  #define BTN_DPAD_RIGHT   0x223
>  
>  #define KEY_ALS_TOGGLE   0x230   /* Ambient light
> sensor */
> +#define KEY_ROTATE_LOCK_TOGGLE   0x231   /* Display
> rotation lock */
>  
>  #define KEY_BUTTONCONFIG 0x240   /* AL Button
> Configuration */
>  #define KEY_TASKMANAGER  0x241   /* AL
> Task/Project Manager */

Re: [PATCH v2 3/5] Input: add KEY_ROTATE_LOCK_TOGGLE

[Bastien Nocera]

On Thu, 2017-11-09 at 23:44 +0100, Stefan Brüns wrote:
> The key has the same use as the SW_ROTATE_LOCK, but is used on
> devices
> where the state is not tracked by the hardware but has to be handled
> in software.

I'll let the input and hid subsystem maintainers have the final say
here, though I think that sending out Win+O would be easier, as this is
already something that desktops need to support for those devices that
send the actual Win+O key combination when those buttons are pressed
there.

If we're going with the separate keycode, could you also file bugs
against xkbd-common and xkeyboard-config to get the key added to the
list of XF86 keysyms and to the default evdev keymap? Otherwise Wayland
and X11 based desktops won't be able to use it.

Cheers

> Signed-off-by: Stefan Brüns 
> 
> ---
> 
> Changes in v2:
> - New patch, add support for KEY_ROTATE_LOCK_TOGGLE
> 
>  include/uapi/linux/input-event-codes.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/uapi/linux/input-event-codes.h
> b/include/uapi/linux/input-event-codes.h
> index f4058bd4c373..ba5d709a8923 100644
> --- a/include/uapi/linux/input-event-codes.h
> +++ b/include/uapi/linux/input-event-codes.h
> @@ -593,6 +593,7 @@
>  #define BTN_DPAD_RIGHT   0x223
>  
>  #define KEY_ALS_TOGGLE   0x230   /* Ambient light
> sensor */
> +#define KEY_ROTATE_LOCK_TOGGLE   0x231   /* Display
> rotation lock */
>  
>  #define KEY_BUTTONCONFIG 0x240   /* AL Button
> Configuration */
>  #define KEY_TASKMANAGER  0x241   /* AL
> Task/Project Manager */

Re: [PATCH v2 4/5] platform/x86: intel-vbtn: support KEY_ROTATE_LOCK_TOGGLE

[Bastien Nocera]

On Thu, 2017-11-09 at 23:44 +0100, Stefan Brüns wrote:
> The Rotate Lock button event is emitted on the XPS 12 (BIOS A8, but
> not
> on BIOS A2).
> 
> Signed-off-by: Stefan Brüns 
> ---
> 
> Changes in v2:
> - Emit KEY_ROTATE_LOCK_TOGGLE instead of KEY_ROTATE_DISPLAY
> - Use separate up/down events
> 
>  drivers/platform/x86/intel-vbtn.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/platform/x86/intel-vbtn.c 
> b/drivers/platform/x86/intel-vbtn.c
> index e3f6375af85c..a484bcc6393b 100644
> --- a/drivers/platform/x86/intel-vbtn.c
> +++ b/drivers/platform/x86/intel-vbtn.c
> @@ -42,6 +42,8 @@ static const struct key_entry intel_vbtn_keymap[] = {
>   { KE_IGNORE, 0xC5, { KEY_VOLUMEUP } },  /* volume-up key 
> release */
>   { KE_KEY, 0xC6, { KEY_VOLUMEDOWN } },   /* volume-down key 
> press */
>   { KE_IGNORE, 0xC7, { KEY_VOLUMEDOWN } },/* volume-down key 
> release */
> + { KE_KEY,0xC8, { KEY_ROTATE_LOCK_TOGGLE } },/* rotate-lock 
> key press */
> + { KE_KEY,0xC9, { KEY_ROTATE_LOCK_TOGGLE } },/* rotate-lock 
> key release */

How are those events sent? When pressing and releasing the key, do you
receive 0xC8 followed by 0xC9? Or do you receive 0xC8 when pressing and
releasing the first time, and 0xC9 when pressing and releasing a second
time?

If the former, then it's not going to work. The release is supposed to
be ignored, as you send the event with sparse_keymap_report_event().

If the latter, and there's an actual state, does it disable a device
on-board, or activate an LED? If so, then it would need to be a switch,
not a key.

>   { KE_SW, 0xCC, { .sw = { SW_TABLET_MODE, 1 } } },   /* Tablet */
>   { KE_SW, 0xCD, { .sw = { SW_TABLET_MODE, 0 } } },   /* Laptop */
>   { KE_END },

Re: [PATCH v2 4/5] platform/x86: intel-vbtn: support KEY_ROTATE_LOCK_TOGGLE

[Bastien Nocera]

On Thu, 2017-11-09 at 23:44 +0100, Stefan Brüns wrote:
> The Rotate Lock button event is emitted on the XPS 12 (BIOS A8, but
> not
> on BIOS A2).
> 
> Signed-off-by: Stefan Brüns 
> ---
> 
> Changes in v2:
> - Emit KEY_ROTATE_LOCK_TOGGLE instead of KEY_ROTATE_DISPLAY
> - Use separate up/down events
> 
>  drivers/platform/x86/intel-vbtn.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/platform/x86/intel-vbtn.c 
> b/drivers/platform/x86/intel-vbtn.c
> index e3f6375af85c..a484bcc6393b 100644
> --- a/drivers/platform/x86/intel-vbtn.c
> +++ b/drivers/platform/x86/intel-vbtn.c
> @@ -42,6 +42,8 @@ static const struct key_entry intel_vbtn_keymap[] = {
>   { KE_IGNORE, 0xC5, { KEY_VOLUMEUP } },  /* volume-up key 
> release */
>   { KE_KEY, 0xC6, { KEY_VOLUMEDOWN } },   /* volume-down key 
> press */
>   { KE_IGNORE, 0xC7, { KEY_VOLUMEDOWN } },/* volume-down key 
> release */
> + { KE_KEY,0xC8, { KEY_ROTATE_LOCK_TOGGLE } },/* rotate-lock 
> key press */
> + { KE_KEY,0xC9, { KEY_ROTATE_LOCK_TOGGLE } },/* rotate-lock 
> key release */

How are those events sent? When pressing and releasing the key, do you
receive 0xC8 followed by 0xC9? Or do you receive 0xC8 when pressing and
releasing the first time, and 0xC9 when pressing and releasing a second
time?

If the former, then it's not going to work. The release is supposed to
be ignored, as you send the event with sparse_keymap_report_event().

If the latter, and there's an actual state, does it disable a device
on-board, or activate an LED? If so, then it would need to be a switch,
not a key.

>   { KE_SW, 0xCC, { .sw = { SW_TABLET_MODE, 1 } } },   /* Tablet */
>   { KE_SW, 0xCD, { .sw = { SW_TABLET_MODE, 0 } } },   /* Laptop */
>   { KE_END },

[PATCH] HID: sony: Fix SHANWAN pad rumbling on USB

[Bastien Nocera]

The SHANWAN PS3 clone joypad will start its rumble motors as soon as
it is plugged in via USB. As the additional USB interrupt does nothing on
the original PS3 Sixaxis joypads, and makes a number of other
clone joypads actually start sending data, disable that call for
the SHANWAN so the rumble motors aren't started on plug.

Signed-off-by: Bastien Nocera <had...@hadess.net>
---
 drivers/hid/hid-sony.c | 14 ++
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index d03203a82e8f..b9dc3ac4d4aa 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -1439,10 +1439,16 @@ static int sixaxis_set_operational_usb(struct 
hid_device *hdev)
goto out;
}
 
-   ret = hid_hw_output_report(hdev, buf, 1);
-   if (ret < 0) {
-   hid_info(hdev, "can't set operational mode: step 3, 
ignoring\n");
-   ret = 0;
+   /*
+* But the USB interrupt would cause SHANWAN controllers to
+* start rumbling non-stop.
+*/
+   if (strcmp(hdev->name, "SHANWAN PS3 GamePad")) {
+   ret = hid_hw_output_report(hdev, buf, 1);
+   if (ret < 0) {
+   hid_info(hdev, "can't set operational mode: step 3, 
ignoring\n");
+   ret = 0;
+   }
}
 
 out:
-- 
2.14.3

[PATCH] HID: sony: Fix SHANWAN pad rumbling on USB

[Bastien Nocera]

The SHANWAN PS3 clone joypad will start its rumble motors as soon as
it is plugged in via USB. As the additional USB interrupt does nothing on
the original PS3 Sixaxis joypads, and makes a number of other
clone joypads actually start sending data, disable that call for
the SHANWAN so the rumble motors aren't started on plug.

Signed-off-by: Bastien Nocera 
---
 drivers/hid/hid-sony.c | 14 ++
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index d03203a82e8f..b9dc3ac4d4aa 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -1439,10 +1439,16 @@ static int sixaxis_set_operational_usb(struct 
hid_device *hdev)
goto out;
}
 
-   ret = hid_hw_output_report(hdev, buf, 1);
-   if (ret < 0) {
-   hid_info(hdev, "can't set operational mode: step 3, 
ignoring\n");
-   ret = 0;
+   /*
+* But the USB interrupt would cause SHANWAN controllers to
+* start rumbling non-stop.
+*/
+   if (strcmp(hdev->name, "SHANWAN PS3 GamePad")) {
+   ret = hid_hw_output_report(hdev, buf, 1);
+   if (ret < 0) {
+   hid_info(hdev, "can't set operational mode: step 3, 
ignoring\n");
+   ret = 0;
+   }
}
 
 out:
-- 
2.14.3

Re: [PATCH 0/4] platform/x86: intel-vbtn: Add support for several more switches/buttons

[Bastien Nocera]

On Mon, 2017-11-06 at 15:25 +, Brüns, Stefan wrote:
> On Montag, 6. November 2017 13:54:25 CET Bastien Nocera wrote:
> > On Sun, 2017-11-05 at 14:31 +0200, Andy Shevchenko wrote:
> > > On Fri, Nov 3, 2017 at 9:03 PM, Stefan Brüns
> > > 
> > > <stefan.bru...@rwth-aachen.de> wrote:
> > > > Currently, intel-vbtn only supports the most relevant buttons,
> > > > although
> > > > there are several more events, mostly in use by convertibles.
> > > > 
> > > > This series adds support for three more events. One of these
> > > > events
> > > > is a switch (SW_*) event, which is currently not working when
> > > > using
> > > > sparse keymaps. The first patch fixes this combination.
> > > > 
> > > > The second patch adds support for the SW_TABLET_MODE switch,
> > > > which
> > > > is used by current convertibles.
> > > > 
> > > > The third patch adds support for the KEY_ROTATE_DISPLAY. On the
> > > > Dell
> > > > XPS 12 (9Q33), rotation lock is implemented as a button to
> > > > toggle
> > > > between locked and unlocked state. In locked state, the
> > > > accelerometer
> > > > should be ignored, while in unlocked the screen contents should
> > > > autorotate based on the tablet orientation. The same
> > > > functionality
> > > > is likely implemented as a switch (SW_ROTATE_LOCK event) on
> > > > different
> > > > hardware.
> > > > 
> > > > The fourth patch adds support for the "Windows logo" button/key
> > > > found on
> > > > the XPS 12 display (i.e. in tablet mode, it is the only key
> > > > reachable).
> > > > The Lenovo Helix 2 has an equivalent touch button. The event
> > > > currently
> > > > uses KEY_MENU, although a distinct key code may be a better
> > > > choice.
> > > 
> > > All, except first, are applied to my review and testing queue,
> > > thanks!
> > > 
> > > > Stefan Brüns (4):
> > > >   Input: sparse-keymap - send sync event for KE_SW/KW_VSW
> > > >   platform/x86: intel-vbtn: support SW_TABLET_MODE
> > > >   platform/x86: intel-vbtn: support KEY_ROTATE_DISPLAY
> > > >   platform/x86: intel-vbtn: support panel front button
> > 
> > KEY_MENU is the key for the contextual menu. You need to use
> > KEY_LEFTMETA. See 791738be57473fddaf393dcedcef31b577231aaa which
> > does
> > this for soc_button_array.
> 
> IMHO LEFTMETA is a bad idea for several reasons:
> 
> - LEFTMETA aka Windows aka RightGUI key is used as a modifier/flag
> (see e.g. 
> USB HID HUT, Keyboard Page 0x07), while on a tablet, it is the only
> regular 
> button (save special funtions like power, volume).

It isn't. There are a number of functions that can be triggered using
the Windows key + other buttons on the device, such as:

 ⊞ Win+PrtScr or ⊞ Win+Volume up instantly saves a screenshot to the
"Screenshots" folder in "Pictures" library. All screenshots are saved
as PNG files.

(from https://en.wikipedia.org/wiki/Windows_key#Windows_8)

In Windows, the button behaves the exact same way as the Windows key on
a keyboard would.

> - on a regular keyboard, I expect the LEFTMETA key to be
> handled/usable as a 
> modifier key. I would not expect it to be used as a shortcut key.
> 
> So if KEY_MENU is not acceptable, a new keycode IMHO is a much better
> option.
> 
> Kind regards,
> 
> Stefan
> 

Re: [PATCH 0/4] platform/x86: intel-vbtn: Add support for several more switches/buttons

[Bastien Nocera]

On Mon, 2017-11-06 at 15:25 +, Brüns, Stefan wrote:
> On Montag, 6. November 2017 13:54:25 CET Bastien Nocera wrote:
> > On Sun, 2017-11-05 at 14:31 +0200, Andy Shevchenko wrote:
> > > On Fri, Nov 3, 2017 at 9:03 PM, Stefan Brüns
> > > 
> > >  wrote:
> > > > Currently, intel-vbtn only supports the most relevant buttons,
> > > > although
> > > > there are several more events, mostly in use by convertibles.
> > > > 
> > > > This series adds support for three more events. One of these
> > > > events
> > > > is a switch (SW_*) event, which is currently not working when
> > > > using
> > > > sparse keymaps. The first patch fixes this combination.
> > > > 
> > > > The second patch adds support for the SW_TABLET_MODE switch,
> > > > which
> > > > is used by current convertibles.
> > > > 
> > > > The third patch adds support for the KEY_ROTATE_DISPLAY. On the
> > > > Dell
> > > > XPS 12 (9Q33), rotation lock is implemented as a button to
> > > > toggle
> > > > between locked and unlocked state. In locked state, the
> > > > accelerometer
> > > > should be ignored, while in unlocked the screen contents should
> > > > autorotate based on the tablet orientation. The same
> > > > functionality
> > > > is likely implemented as a switch (SW_ROTATE_LOCK event) on
> > > > different
> > > > hardware.
> > > > 
> > > > The fourth patch adds support for the "Windows logo" button/key
> > > > found on
> > > > the XPS 12 display (i.e. in tablet mode, it is the only key
> > > > reachable).
> > > > The Lenovo Helix 2 has an equivalent touch button. The event
> > > > currently
> > > > uses KEY_MENU, although a distinct key code may be a better
> > > > choice.
> > > 
> > > All, except first, are applied to my review and testing queue,
> > > thanks!
> > > 
> > > > Stefan Brüns (4):
> > > >   Input: sparse-keymap - send sync event for KE_SW/KW_VSW
> > > >   platform/x86: intel-vbtn: support SW_TABLET_MODE
> > > >   platform/x86: intel-vbtn: support KEY_ROTATE_DISPLAY
> > > >   platform/x86: intel-vbtn: support panel front button
> > 
> > KEY_MENU is the key for the contextual menu. You need to use
> > KEY_LEFTMETA. See 791738be57473fddaf393dcedcef31b577231aaa which
> > does
> > this for soc_button_array.
> 
> IMHO LEFTMETA is a bad idea for several reasons:
> 
> - LEFTMETA aka Windows aka RightGUI key is used as a modifier/flag
> (see e.g. 
> USB HID HUT, Keyboard Page 0x07), while on a tablet, it is the only
> regular 
> button (save special funtions like power, volume).

It isn't. There are a number of functions that can be triggered using
the Windows key + other buttons on the device, such as:

 ⊞ Win+PrtScr or ⊞ Win+Volume up instantly saves a screenshot to the
"Screenshots" folder in "Pictures" library. All screenshots are saved
as PNG files.

(from https://en.wikipedia.org/wiki/Windows_key#Windows_8)

In Windows, the button behaves the exact same way as the Windows key on
a keyboard would.

> - on a regular keyboard, I expect the LEFTMETA key to be
> handled/usable as a 
> modifier key. I would not expect it to be used as a shortcut key.
> 
> So if KEY_MENU is not acceptable, a new keycode IMHO is a much better
> option.
> 
> Kind regards,
> 
> Stefan
> 

Re: [PATCH 0/4] platform/x86: intel-vbtn: Add support for several more switches/buttons

[Bastien Nocera]

On Sun, 2017-11-05 at 14:31 +0200, Andy Shevchenko wrote:
> On Fri, Nov 3, 2017 at 9:03 PM, Stefan Brüns
>  wrote:
> > Currently, intel-vbtn only supports the most relevant buttons,
> > although
> > there are several more events, mostly in use by convertibles.
> > 
> > This series adds support for three more events. One of these events
> > is a switch (SW_*) event, which is currently not working when using
> > sparse keymaps. The first patch fixes this combination.
> > 
> > The second patch adds support for the SW_TABLET_MODE switch, which
> > is used by current convertibles.
> > 
> > The third patch adds support for the KEY_ROTATE_DISPLAY. On the
> > Dell
> > XPS 12 (9Q33), rotation lock is implemented as a button to toggle
> > between locked and unlocked state. In locked state, the
> > accelerometer
> > should be ignored, while in unlocked the screen contents should
> > autorotate based on the tablet orientation. The same functionality
> > is likely implemented as a switch (SW_ROTATE_LOCK event) on
> > different
> > hardware.
> > 
> > The fourth patch adds support for the "Windows logo" button/key
> > found on
> > the XPS 12 display (i.e. in tablet mode, it is the only key
> > reachable).
> > The Lenovo Helix 2 has an equivalent touch button. The event
> > currently
> > uses KEY_MENU, although a distinct key code may be a better choice.
> > 
> 
> All, except first, are applied to my review and testing queue,
> thanks!
> 
> > 
> > Stefan Brüns (4):
> >   Input: sparse-keymap - send sync event for KE_SW/KW_VSW
> >   platform/x86: intel-vbtn: support SW_TABLET_MODE
> >   platform/x86: intel-vbtn: support KEY_ROTATE_DISPLAY
> >   platform/x86: intel-vbtn: support panel front button

KEY_MENU is the key for the contextual menu. You need to use
KEY_LEFTMETA. See 791738be57473fddaf393dcedcef31b577231aaa which does
this for soc_button_array.

Cheers

Re: [PATCH 0/4] platform/x86: intel-vbtn: Add support for several more switches/buttons

[Bastien Nocera]

On Sun, 2017-11-05 at 14:31 +0200, Andy Shevchenko wrote:
> On Fri, Nov 3, 2017 at 9:03 PM, Stefan Brüns
>  wrote:
> > Currently, intel-vbtn only supports the most relevant buttons,
> > although
> > there are several more events, mostly in use by convertibles.
> > 
> > This series adds support for three more events. One of these events
> > is a switch (SW_*) event, which is currently not working when using
> > sparse keymaps. The first patch fixes this combination.
> > 
> > The second patch adds support for the SW_TABLET_MODE switch, which
> > is used by current convertibles.
> > 
> > The third patch adds support for the KEY_ROTATE_DISPLAY. On the
> > Dell
> > XPS 12 (9Q33), rotation lock is implemented as a button to toggle
> > between locked and unlocked state. In locked state, the
> > accelerometer
> > should be ignored, while in unlocked the screen contents should
> > autorotate based on the tablet orientation. The same functionality
> > is likely implemented as a switch (SW_ROTATE_LOCK event) on
> > different
> > hardware.
> > 
> > The fourth patch adds support for the "Windows logo" button/key
> > found on
> > the XPS 12 display (i.e. in tablet mode, it is the only key
> > reachable).
> > The Lenovo Helix 2 has an equivalent touch button. The event
> > currently
> > uses KEY_MENU, although a distinct key code may be a better choice.
> > 
> 
> All, except first, are applied to my review and testing queue,
> thanks!
> 
> > 
> > Stefan Brüns (4):
> >   Input: sparse-keymap - send sync event for KE_SW/KW_VSW
> >   platform/x86: intel-vbtn: support SW_TABLET_MODE
> >   platform/x86: intel-vbtn: support KEY_ROTATE_DISPLAY
> >   platform/x86: intel-vbtn: support panel front button

KEY_MENU is the key for the contextual menu. You need to use
KEY_LEFTMETA. See 791738be57473fddaf393dcedcef31b577231aaa which does
this for soc_button_array.

Cheers

Re: [PATCH 0/4] platform/x86: intel-vbtn: Add support for several more switches/buttons

[Bastien Nocera]

On Sun, 2017-11-05 at 14:31 +0200, Andy Shevchenko wrote:
> On Fri, Nov 3, 2017 at 9:03 PM, Stefan Brüns
>  wrote:
> > Currently, intel-vbtn only supports the most relevant buttons,
> > although
> > there are several more events, mostly in use by convertibles.
> > 
> > This series adds support for three more events. One of these events
> > is a switch (SW_*) event, which is currently not working when using
> > sparse keymaps. The first patch fixes this combination.
> > 
> > The second patch adds support for the SW_TABLET_MODE switch, which
> > is used by current convertibles.
> > 
> > The third patch adds support for the KEY_ROTATE_DISPLAY. On the
> > Dell
> > XPS 12 (9Q33), rotation lock is implemented as a button to toggle
> > between locked and unlocked state. In locked state, the
> > accelerometer
> > should be ignored, while in unlocked the screen contents should
> > autorotate based on the tablet orientation. The same functionality
> > is likely implemented as a switch (SW_ROTATE_LOCK event) on
> > different
> > hardware.

KEY_ROTATE_DISPLAY will resolve to XF86RotateWindows. This is a button
for devices without accelerometers (such as old Thinkpad tablet models
with Wacom builtin), to request the display to be rotated. It's not a
request to the OS to toggle the acceleration lock.

The acceleration lock is currently handled in user-space if "Win+O" is
pressed, as some BIOSes will generate this key combination directly,
which Windows will handle.

Please revert this patch, it should send out Win+O instead (or it needs
another keycode added).

(Also, even if the code ultimately lands in the platform tree, it would
be nice to send the linux-input list a copy of those types of patches,
where this problem would have been caught earlier).

Cheers

Re: [PATCH 0/4] platform/x86: intel-vbtn: Add support for several more switches/buttons

[Bastien Nocera]

On Sun, 2017-11-05 at 14:31 +0200, Andy Shevchenko wrote:
> On Fri, Nov 3, 2017 at 9:03 PM, Stefan Brüns
>  wrote:
> > Currently, intel-vbtn only supports the most relevant buttons,
> > although
> > there are several more events, mostly in use by convertibles.
> > 
> > This series adds support for three more events. One of these events
> > is a switch (SW_*) event, which is currently not working when using
> > sparse keymaps. The first patch fixes this combination.
> > 
> > The second patch adds support for the SW_TABLET_MODE switch, which
> > is used by current convertibles.
> > 
> > The third patch adds support for the KEY_ROTATE_DISPLAY. On the
> > Dell
> > XPS 12 (9Q33), rotation lock is implemented as a button to toggle
> > between locked and unlocked state. In locked state, the
> > accelerometer
> > should be ignored, while in unlocked the screen contents should
> > autorotate based on the tablet orientation. The same functionality
> > is likely implemented as a switch (SW_ROTATE_LOCK event) on
> > different
> > hardware.

KEY_ROTATE_DISPLAY will resolve to XF86RotateWindows. This is a button
for devices without accelerometers (such as old Thinkpad tablet models
with Wacom builtin), to request the display to be rotated. It's not a
request to the OS to toggle the acceleration lock.

The acceleration lock is currently handled in user-space if "Win+O" is
pressed, as some BIOSes will generate this key combination directly,
which Windows will handle.

Please revert this patch, it should send out Win+O instead (or it needs
another keycode added).

(Also, even if the code ultimately lands in the platform tree, it would
be nice to send the linux-input list a copy of those types of patches,
where this problem would have been caught earlier).

Cheers

Re: [PATCH] fbdev: Fix typo in vfb comment

[Bastien Nocera]

On Thu, 2017-10-12 at 15:31 +0200, Jiri Kosina wrote:
> On Tue, 20 Jun 2017, Bastien Nocera wrote:
> 
> > Signed-off-by: Bastien Nocera <had...@hadess.net>
> > ---
> >  drivers/video/fbdev/vfb.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/video/fbdev/vfb.c b/drivers/video/fbdev/vfb.c
> > index da653a080394..59cd17bd9de6 100644
> > --- a/drivers/video/fbdev/vfb.c
> > +++ b/drivers/video/fbdev/vfb.c
> > @@ -374,7 +374,7 @@ static int vfb_mmap(struct fb_info *info,
> >  
> >  #ifndef MODULE
> >  /*
> > - * The virtual framebuffer driver is only enabled if explicitly
> > + * The virtual framebuffer driver is only enabled if explicitely
> 
> Is this really an improvement? :)

Apparently not, feel free to drop that patch.

Re: [PATCH] fbdev: Fix typo in vfb comment

[Bastien Nocera]

On Thu, 2017-10-12 at 15:31 +0200, Jiri Kosina wrote:
> On Tue, 20 Jun 2017, Bastien Nocera wrote:
> 
> > Signed-off-by: Bastien Nocera 
> > ---
> >  drivers/video/fbdev/vfb.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/video/fbdev/vfb.c b/drivers/video/fbdev/vfb.c
> > index da653a080394..59cd17bd9de6 100644
> > --- a/drivers/video/fbdev/vfb.c
> > +++ b/drivers/video/fbdev/vfb.c
> > @@ -374,7 +374,7 @@ static int vfb_mmap(struct fb_info *info,
> >  
> >  #ifndef MODULE
> >  /*
> > - * The virtual framebuffer driver is only enabled if explicitly
> > + * The virtual framebuffer driver is only enabled if explicitely
> 
> Is this really an improvement? :)

Apparently not, feel free to drop that patch.

Re: [PATCH v3] Input: goodix: Add support for capacitive home button

[Bastien Nocera]

Hey,

On Tue, 2017-06-20 at 18:08 +0200, Bastien Nocera wrote:
> From: "Sergei A. Trusov" <sergei.a.tru...@ya.ru>
> 
> On some x86 tablets with a Goodix touchscreen, the Windows logo on
> the
> front is a capacitive home button. Touching this button results in a
> touch
> with bit 4 of the first byte set, while only the lower 4 bits (0-3)
> are
> used to indicate the number of touches.
> 
> Report a KEY_LEFTMETA press when this happens.
> 
> Note that the hardware might support more than one button, in which
> case the "id" byte of coor_data would identify the button in
> question.
> This is not implemented as we don't have access to hardware with
> multiple buttons.
> 
> Signed-off-by: Sergei A. Trusov <sergei.a.tru...@ya.ru>
> Acked-by: Bastien Nocera <had...@hadess.net>

Can we please get this merged? I didn't receive any reviews on it, and
it's been sitting there for 2 months...

Re: [PATCH v3] Input: goodix: Add support for capacitive home button

[Bastien Nocera]

Hey,

On Tue, 2017-06-20 at 18:08 +0200, Bastien Nocera wrote:
> From: "Sergei A. Trusov" 
> 
> On some x86 tablets with a Goodix touchscreen, the Windows logo on
> the
> front is a capacitive home button. Touching this button results in a
> touch
> with bit 4 of the first byte set, while only the lower 4 bits (0-3)
> are
> used to indicate the number of touches.
> 
> Report a KEY_LEFTMETA press when this happens.
> 
> Note that the hardware might support more than one button, in which
> case the "id" byte of coor_data would identify the button in
> question.
> This is not implemented as we don't have access to hardware with
> multiple buttons.
> 
> Signed-off-by: Sergei A. Trusov 
> Acked-by: Bastien Nocera 

Can we please get this merged? I didn't receive any reviews on it, and
it's been sitting there for 2 months...

Re: [RFT][PATCH] iio: hid-sensor-trigger: Fix the race with user space powering up sensors

[Bastien Nocera]

On Sat, 2017-08-12 at 13:16 +0100, Jonathan Cameron wrote:
> On Fri, 11 Aug 2017 16:04:30 +0200
> Bastien Nocera <had...@hadess.net> wrote:
> 
> > Woot!
> > 
> > On Thu, 2017-08-10 at 16:24 -0700, Srinivas Pandruvada wrote:
> > > It has been reported for a while that with iio-sensor-proxy
> > > service the
> > > rotation only works after one suspend/resume cycle. This required
> > > a wait
> > > in the systemd unit file to avoid race. I found a Yoga 900 where
> > > I could
> > > reproduce this.
> > > 
> > > The problem scenerio is:
> > > - During sensor driver init, enable run time PM and also set a
> > >   auto-suspend for 3 seconds.
> > >   This result in one runtime resume. But there is a check to
> > > avoid
> > > a powerup in this sequence, but rpm is active
> > > - User space iio-sensor-proxy tries to power up the sensor. Since
> > > rpm is
> > >   active it will simply return. But sensors were not actually
> > > powered up in the prior sequence, so actaully the sensors will
> > > not work
> > > - After 3 seconds the auto suspend kicks
> > > 
> > > If we add a wait in systemd service file to fire iio-sensor-proxy 
> > > after
> > > 3 seconds, then now everything will work as the runtime resume
> > > will
> > > actually powerup the sensor as this is a user request.
> > > 
> > > To avoid this:
> > > - Remove the check to match user requested state, this will cause
> > > a
> > >   brief powerup, but if the iio-sensor-proxy starts immediately
> > > it will
> > > still work as the sensors are ON.
> > > - Also move the autosuspend delay to place when user requested
> > > turn off
> > >   of sensors, like after user finished raw read or buffer disable
> > > 
> > > Signed-off-by: Srinivas Pandruvada <srinivas.pandruv...@linux.int
> > > el.com>  
> > 
> > Tested-by: Bastien Nocera <had...@hadess.net>
> > 
> > I'm still chasing a couple of bugs in the user-space side of things
> > caused by the removal of the timeout.
> > 
> > Thanks!
> 
> Is it worth me sitting on this for a week or so to see if it deals
> with
> all the reported issues around this?
> 
> Or are you happy that the test set you have is sufficient to verify
> it?

I'd rather have it merged ASAP. The fact that it didn't need to sleep
for 3 seconds allowed me to find a couple of problems in GNOME's use of
this feature, with the sensor showing up before the desktop has
started. Those will likely be taken care of next week as well.

I've tested it on a couple of machines, and it's working as expected.

> Definitely good to put this one to bed finally! 

And no one's happier than me in this one. I can concentrate on bugs I
wrote myself now ;)

Re: [RFT][PATCH] iio: hid-sensor-trigger: Fix the race with user space powering up sensors

[Bastien Nocera]

On Sat, 2017-08-12 at 13:16 +0100, Jonathan Cameron wrote:
> On Fri, 11 Aug 2017 16:04:30 +0200
> Bastien Nocera  wrote:
> 
> > Woot!
> > 
> > On Thu, 2017-08-10 at 16:24 -0700, Srinivas Pandruvada wrote:
> > > It has been reported for a while that with iio-sensor-proxy
> > > service the
> > > rotation only works after one suspend/resume cycle. This required
> > > a wait
> > > in the systemd unit file to avoid race. I found a Yoga 900 where
> > > I could
> > > reproduce this.
> > > 
> > > The problem scenerio is:
> > > - During sensor driver init, enable run time PM and also set a
> > >   auto-suspend for 3 seconds.
> > >   This result in one runtime resume. But there is a check to
> > > avoid
> > > a powerup in this sequence, but rpm is active
> > > - User space iio-sensor-proxy tries to power up the sensor. Since
> > > rpm is
> > >   active it will simply return. But sensors were not actually
> > > powered up in the prior sequence, so actaully the sensors will
> > > not work
> > > - After 3 seconds the auto suspend kicks
> > > 
> > > If we add a wait in systemd service file to fire iio-sensor-proxy 
> > > after
> > > 3 seconds, then now everything will work as the runtime resume
> > > will
> > > actually powerup the sensor as this is a user request.
> > > 
> > > To avoid this:
> > > - Remove the check to match user requested state, this will cause
> > > a
> > >   brief powerup, but if the iio-sensor-proxy starts immediately
> > > it will
> > > still work as the sensors are ON.
> > > - Also move the autosuspend delay to place when user requested
> > > turn off
> > >   of sensors, like after user finished raw read or buffer disable
> > > 
> > > Signed-off-by: Srinivas Pandruvada  > > el.com>  
> > 
> > Tested-by: Bastien Nocera 
> > 
> > I'm still chasing a couple of bugs in the user-space side of things
> > caused by the removal of the timeout.
> > 
> > Thanks!
> 
> Is it worth me sitting on this for a week or so to see if it deals
> with
> all the reported issues around this?
> 
> Or are you happy that the test set you have is sufficient to verify
> it?

I'd rather have it merged ASAP. The fact that it didn't need to sleep
for 3 seconds allowed me to find a couple of problems in GNOME's use of
this feature, with the sensor showing up before the desktop has
started. Those will likely be taken care of next week as well.

I've tested it on a couple of machines, and it's working as expected.

> Definitely good to put this one to bed finally! 

And no one's happier than me in this one. I can concentrate on bugs I
wrote myself now ;)

Re: [RFT][PATCH] iio: hid-sensor-trigger: Fix the race with user space powering up sensors

[Bastien Nocera]

Woot!

On Thu, 2017-08-10 at 16:24 -0700, Srinivas Pandruvada wrote:
> It has been reported for a while that with iio-sensor-proxy service the
> rotation only works after one suspend/resume cycle. This required a wait
> in the systemd unit file to avoid race. I found a Yoga 900 where I could
> reproduce this.
> 
> The problem scenerio is:
> - During sensor driver init, enable run time PM and also set a
>   auto-suspend for 3 seconds.
>   This result in one runtime resume. But there is a check to avoid
> a powerup in this sequence, but rpm is active
> - User space iio-sensor-proxy tries to power up the sensor. Since rpm is
>   active it will simply return. But sensors were not actually
> powered up in the prior sequence, so actaully the sensors will not work
> - After 3 seconds the auto suspend kicks
> 
> If we add a wait in systemd service file to fire iio-sensor-proxy after
> 3 seconds, then now everything will work as the runtime resume will
> actually powerup the sensor as this is a user request.
> 
> To avoid this:
> - Remove the check to match user requested state, this will cause a
>   brief powerup, but if the iio-sensor-proxy starts immediately it will
> still work as the sensors are ON.
> - Also move the autosuspend delay to place when user requested turn off
>   of sensors, like after user finished raw read or buffer disable
> 
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruv...@linux.intel.com>

Tested-by: Bastien Nocera <had...@hadess.net>

I'm still chasing a couple of bugs in the user-space side of things
caused by the removal of the timeout.

Thanks!

Re: [RFT][PATCH] iio: hid-sensor-trigger: Fix the race with user space powering up sensors

[Bastien Nocera]

Woot!

On Thu, 2017-08-10 at 16:24 -0700, Srinivas Pandruvada wrote:
> It has been reported for a while that with iio-sensor-proxy service the
> rotation only works after one suspend/resume cycle. This required a wait
> in the systemd unit file to avoid race. I found a Yoga 900 where I could
> reproduce this.
> 
> The problem scenerio is:
> - During sensor driver init, enable run time PM and also set a
>   auto-suspend for 3 seconds.
>   This result in one runtime resume. But there is a check to avoid
> a powerup in this sequence, but rpm is active
> - User space iio-sensor-proxy tries to power up the sensor. Since rpm is
>   active it will simply return. But sensors were not actually
> powered up in the prior sequence, so actaully the sensors will not work
> - After 3 seconds the auto suspend kicks
> 
> If we add a wait in systemd service file to fire iio-sensor-proxy after
> 3 seconds, then now everything will work as the runtime resume will
> actually powerup the sensor as this is a user request.
> 
> To avoid this:
> - Remove the check to match user requested state, this will cause a
>   brief powerup, but if the iio-sensor-proxy starts immediately it will
> still work as the sensors are ON.
> - Also move the autosuspend delay to place when user requested turn off
>   of sensors, like after user finished raw read or buffer disable
> 
> Signed-off-by: Srinivas Pandruvada 

Tested-by: Bastien Nocera 

I'm still chasing a couple of bugs in the user-space side of things
caused by the removal of the timeout.

Thanks!

Re: [PATCH] HID: Add quirk for Lenovo Yoga 910 with ITE Chips

[Bastien Nocera]

On Mon, 2017-07-17 at 13:19 -0700, Srinivas Pandruvada wrote:
> On Sat, 2017-07-15 at 22:05 +0200, Patrick wrote:
> > On Sat, Jul 15, 2017 at 07:58:10PM +0200, Bastien Nocera wrote:
> > > 
> > > On Sat, 2017-07-15 at 19:52 +0200, Patrick Pedersen wrote:
> > > > 
> > > > On Sat, Jul 15, 2017 at 4:29 PM, Bastien Nocera <hadess@hadess.
> > > > ne
> > > > t>
> > > > wrote:
> > > > > 
> > > > > On Sat, 2017-07-15 at 14:27 +0200, Patrick Pedersen wrote:
> > > > > > 
> > > > > > As with previous generations of this device (see https://pa
> > > > > > tc
> > > > > > hwor
> > > > > 
> > > > > k.ke
> > > > > > 
> > > > > > rnel.org/patch/7887361/), the ITE
> > > > > > HID Sensor Hub, responsible for the accelerometer and als
> > > > > > sensor,
> > > > > > requires a quirk entry.
> > > > > > 
> > > > > > Without the entry, the Sensor Hub can't be accessed and the
> > > > > 
> > > > > kernel
> > > > > > 
> > > > > > fails to report any movements. As a result
> > > > > > iio-sensor-proxy receives no new data.
> > > > > > 
> > > > > > It shall additionally be noted that the i2c-hid 'sleep' bug
> > > > > 
> > > > > (present
> > > > > > 
> > > > > > since kernel ver. 4.3)
> > > > > > still affects the driver. This means that the sensor hub
> > > > > > will
> > > > > > not
> > > > > > report any movement, until
> > > > > > the device is suspended and resumed.

Those are the same symptoms as the problem I reported here:
https://www.spinics.net/lists/linux-iio/msg29983.html

> Can you try some tests for this for test? I want to see if sensors
> are
> powered off or transport didn't wake up? So forcing the sensors to
> keep
> power on.

I tested the patch below using a HID sensor (ColorHug ALS) and it
failed in the same way with and without the patch you have below.

Hans also had an idea that this might be due to the PM suspend. The 3-
second sleep that you need to remove from iio-sensor-proxy matches the
default autosuspend delay at:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/iio/common/hid-sensors/hid-sensor-trigger.c#n285

But setting "->common_attributes.data_ready" to 1 in each of the
driver's original states didn't work out.

Knowing that it affects both i2c and USB is a good bit of information
though, meaning that the problem probably lies in the IIO subsystem.

Or possibly a change of behaviour in the PM subsystem which made the
IIO trigger stop working. If Rafael could do a review on the code
linked above, that'd be really helpful.

Note that in addition to the patches I mentioned in the kernel thread
above, you'll need to revert this patch in iio-sensor-proxy:
https://github.com/hadess/iio-sensor-proxy/commit/5468452f7a72566d2e6ddc44f9396d3d088fa9fb

Otherwise things work :/

> diff --git a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> index 16ade0a..a70df7e 100644
> --- a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> +++ b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> @@ -110,6 +110,9 @@ static int _hid_sensor_power_state(struct
> hid_sensor_common *st, bool state)
> int report_val;
> s32 poll_value = 0;
>  
> +   st->power_state.logical_minimum = 1;
> +   st->report_state.logical_minimum = 1;
> +
> if (state) {
> if (!atomic_read(>user_requested_state))
> return 0;
> @@ -146,6 +149,9 @@ static int _hid_sensor_power_state(struct
> hid_sensor_common *st, bool state)
> HID_USAGE_SENSOR_PROP_REPORTING_STATE_NO_EVEN
> TS
> _ENUM);
> }
>  
> +   state_val = 0;
> +   report_val = 0;
> +   
> if (state_val >= 0) {
> state_val += st->power_state.logical_minimum;
> sensor_hub_set_feature(st->hsdev, st-
> > power_state.report_id,

Re: [PATCH] HID: Add quirk for Lenovo Yoga 910 with ITE Chips

[Bastien Nocera]

On Mon, 2017-07-17 at 13:19 -0700, Srinivas Pandruvada wrote:
> On Sat, 2017-07-15 at 22:05 +0200, Patrick wrote:
> > On Sat, Jul 15, 2017 at 07:58:10PM +0200, Bastien Nocera wrote:
> > > 
> > > On Sat, 2017-07-15 at 19:52 +0200, Patrick Pedersen wrote:
> > > > 
> > > > On Sat, Jul 15, 2017 at 4:29 PM, Bastien Nocera  > > > ne
> > > > t>
> > > > wrote:
> > > > > 
> > > > > On Sat, 2017-07-15 at 14:27 +0200, Patrick Pedersen wrote:
> > > > > > 
> > > > > > As with previous generations of this device (see https://pa
> > > > > > tc
> > > > > > hwor
> > > > > 
> > > > > k.ke
> > > > > > 
> > > > > > rnel.org/patch/7887361/), the ITE
> > > > > > HID Sensor Hub, responsible for the accelerometer and als
> > > > > > sensor,
> > > > > > requires a quirk entry.
> > > > > > 
> > > > > > Without the entry, the Sensor Hub can't be accessed and the
> > > > > 
> > > > > kernel
> > > > > > 
> > > > > > fails to report any movements. As a result
> > > > > > iio-sensor-proxy receives no new data.
> > > > > > 
> > > > > > It shall additionally be noted that the i2c-hid 'sleep' bug
> > > > > 
> > > > > (present
> > > > > > 
> > > > > > since kernel ver. 4.3)
> > > > > > still affects the driver. This means that the sensor hub
> > > > > > will
> > > > > > not
> > > > > > report any movement, until
> > > > > > the device is suspended and resumed.

Those are the same symptoms as the problem I reported here:
https://www.spinics.net/lists/linux-iio/msg29983.html

> Can you try some tests for this for test? I want to see if sensors
> are
> powered off or transport didn't wake up? So forcing the sensors to
> keep
> power on.

I tested the patch below using a HID sensor (ColorHug ALS) and it
failed in the same way with and without the patch you have below.

Hans also had an idea that this might be due to the PM suspend. The 3-
second sleep that you need to remove from iio-sensor-proxy matches the
default autosuspend delay at:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/iio/common/hid-sensors/hid-sensor-trigger.c#n285

But setting "->common_attributes.data_ready" to 1 in each of the
driver's original states didn't work out.

Knowing that it affects both i2c and USB is a good bit of information
though, meaning that the problem probably lies in the IIO subsystem.

Or possibly a change of behaviour in the PM subsystem which made the
IIO trigger stop working. If Rafael could do a review on the code
linked above, that'd be really helpful.

Note that in addition to the patches I mentioned in the kernel thread
above, you'll need to revert this patch in iio-sensor-proxy:
https://github.com/hadess/iio-sensor-proxy/commit/5468452f7a72566d2e6ddc44f9396d3d088fa9fb

Otherwise things work :/

> diff --git a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> index 16ade0a..a70df7e 100644
> --- a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> +++ b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> @@ -110,6 +110,9 @@ static int _hid_sensor_power_state(struct
> hid_sensor_common *st, bool state)
> int report_val;
> s32 poll_value = 0;
>  
> +   st->power_state.logical_minimum = 1;
> +   st->report_state.logical_minimum = 1;
> +
> if (state) {
> if (!atomic_read(>user_requested_state))
> return 0;
> @@ -146,6 +149,9 @@ static int _hid_sensor_power_state(struct
> hid_sensor_common *st, bool state)
> HID_USAGE_SENSOR_PROP_REPORTING_STATE_NO_EVEN
> TS
> _ENUM);
> }
>  
> +   state_val = 0;
> +   report_val = 0;
> +   
> if (state_val >= 0) {
> state_val += st->power_state.logical_minimum;
> sensor_hub_set_feature(st->hsdev, st-
> > power_state.report_id,

Re: [PATCH] mfd: intel_soc_pmic_crc: Uniquify device name

[Bastien Nocera]

On Mon, 2017-07-17 at 12:58 +0100, Lee Jones wrote:
> The commit log should not be empty.

Fair.

> Please provide information as to why this change is required, etc.

SSIA.

I also sent another patch to the same addresses labelled:
[PATCH] gpio: crystalcove: Uniquify driver name

Re: [PATCH] mfd: intel_soc_pmic_crc: Uniquify device name

[Bastien Nocera]

On Mon, 2017-07-17 at 12:58 +0100, Lee Jones wrote:
> The commit log should not be empty.

Fair.

> Please provide information as to why this change is required, etc.

SSIA.

I also sent another patch to the same addresses labelled:
[PATCH] gpio: crystalcove: Uniquify driver name