* Re: [Resubmit] Read battery voltage from Logitech Gaming mice
From: Pedro Vanzella @ 2019-08-23 15:46 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: open list:HID CORE LAYER, Filipe Laíns, Jiri Kosina, lkml
In-Reply-To: <CAO-hwJ+=dAyFnUfiPSmiGpzYTj=9BPDdeKQOY7UoCOvwQ5CH7Q@mail.gmail.com>
On 8/23/19 10:32 AM, Benjamin Tissoires wrote:
> On Fri, Aug 23, 2019 at 4:22 PM Pedro Vanzella <pedro@pedrovanzella.com> wrote:
>>
>> Hi Benjamin,
>>
>> On 8/23/19 4:25 AM, Benjamin Tissoires wrote:
>>> Hi Pedro,
>>>
>>> On Thu, Aug 22, 2019 at 10:19 PM Pedro Vanzella <pedro@pedrovanzella.com> wrote:
>>>>
>>>> Resumitting this after having rebased it against the latest changes.
>>>
>>> thanks for resubmitting. Sorry I wasn't able to provide feedback on
>>> the last revision
>>>
>>
>> No worries, I know how these things are.
>>
>>>>
>>>> The gaming line of Logitech devices doesn't use the old hidpp20
>>>> feature for battery level reporting. Instead, they report the
>>>> current voltage of the battery, in millivolts.
>>>>
>>>> This patch set handles this case by adding a quirk to the
>>>> devices we know to have this new feature, in both wired
>>>> and wireless mode.
>>>
>>> So the quirk is in the end a bad idea after all. I had some chats with
>>> Filipe that made me realize this.
>>
>> I actually resubmitted by Filipe's request, since the patches weren't
>> applying cleanly anymore. The idea was to apply these patches and in the
>> future refactor the code to use the feature discovery routines.
>>
>>> Re-reading our previous exchanges also made me understood why I wasn't
>>> happy with the initial submission: for every request the code was
>>> checking both features 0x1000 and 0x1001 when we can remember this
>>> once and for all during hidpp_initialize_battery().
>>
>> Yeah I wasn't too happy about this either, but since there was nothing
>> prohibiting some device to have both features enabled, I thought this
>> wasn't too horrible.
>
> I honestly don't think we will find one device that has both features
> enabled. It doesn't make sense as the "new" feature allows for fine
> tuning in the software, which would be moot if you also enable the
> percentage.
>
>>
>>>
>>> So I think we should remove the useless quirk in the end (bad idea
>>> from me, I concede), and instead during hidpp_initialize_battery() set
>>> the correct HIDPP_CAPABILITY_*.
>>> Not entirely sure if we should try to call 0x1000, or 0x1001 or if we
>>> should rely on the 0x0001 feature to know which feature is available,
>>> but this should be implementation detail.
>>
>> I like the idea of calling 0x0001 once on device boot much better. I
>> think we could easily just fetch the location for the features we know
>> about and want to deal with once only. This also makes sure we support
>> every single device that supports this feature, so that is a huge plus.
>>
>> In fact, I think we should _not_ call 0x0001 on battery init, but only
>> call battery init _after_ we called 0x0001 and discovered either 0x1000
>> or 0x1001 (or the solar battery feature, or any other one that might
>> crop up in the feature).
>
> ack for that
>
>>
>>>
>>>>
>>>> This version of the patch set is better split, as well as adding the
>>>> quirk to make sure we don't needlessly probe every device connected.
>>>
>>> It is for sure easy to review, but doesn't make much sense in the end.
>>> I think we should squash all the patches together as you are just
>>> adding one feature in the driver, and it is a little bit disturbing to
>>> first add the quirk that has no use, then set up the structs when they
>>> are not used, and so on, so forth.
>>
>> You're right. My first instinct was to send a single patch. As much as I
>> tested this, I always feel like breaking the patch up post-facto will
>> break a git bisect in the future and everyone will hate me :P
>
> as long as the patches are compiling and are not breaking, git bisect
> will not be a problem. However, we might end up with the last one,
> which is not very explicit in what it does as it just enables the
> features implemented previously.
>
>>
>> So we (you, me and Filipe) should probably come up with an action plan
>> here. The way I see it there are two issues here: one is adding this
>> feature, and the other is refactoring to use feature discovery for all
>> features. There are advantages and disadvantages to doing one or another
>> first and we might want to discuss that.
>>
>> By merging this first (probably after I resubmit it as a single squashed
>> patch) we get to test it a bit better and have a usable feature sooner.
>> Plenty of people have been requesting this and there is plenty of stuff
>> that can be built on top of it, but only once this is actually merged I
>> think.
>>
>> On the other hand, by first refactoring the rest of the code to use
>> 0x0001 we avoid some rework on this patch. It should be minor, as most
>> functions here do all the heavy lifting after the initial feature
>> discovery, and are thus mostly independent from how that is done.
>>
>> I'm happy either way, so just let me know what you guys decide.
>
> I think we should merge your v3 squashed series with a slight
> autodetection during battery init, like the method you used in the v1.
> This would remove the quirk, but keep the straightforward commands
> when addressing battery data.
Alright, I rebased against for-5.4/logitech to make sure, squashed
everything and restored the detection code from v1, removing the quirk.
Tested and it works on both wired and wireless on my G900.
>
> Relying on 0x0001 should be done separately and can come in in a later
> patch IMO (unless you plan to work on it, in which case you can send
> both at once).
0x0001 is quite the task and I think Filipe already has a good plan to
tackle it, so I'll leave that for him.
>
> The problem I have with quirks, and that I explained to Filipe on IRC
> is that this is kernel ABI. Even if there is a very low chance we have
> someone using this, re-using the same drv_data bit in the future might
> break someone's device.
>
>>
>> If you guys (or anyone else reading this on the public list, really) has
>> any input - naming things being notoriosly hard, I'm actually happy with
>> nitpicking - I'd appreciate it. On that note, come to think of it, I'm
>> not 100% sure reporting the voltage in milivolts is the standard way. I
>> looked through the docs, but found no solid guideline. It was either
>> that or a float, so I think I made the right call here, but still.
>
> I am not sure either. Adding Bastien as he has a lot more experience in upower.
>
> But I am under the impression that the kernel part is more "try to
> deal with whatever the hardware provides, and deal with it in user
> space".
>
I'll submit v4 as a single patch in the next couple of minutes.
- Pedro
^ permalink raw reply
* Re: [Resubmit] Read battery voltage from Logitech Gaming mice
From: Benjamin Tissoires @ 2019-08-23 15:32 UTC (permalink / raw)
To: Filipe Laíns
Cc: Pedro Vanzella, Bastien Nocera, open list:HID CORE LAYER,
Jiri Kosina, lkml
In-Reply-To: <3c37ccc992d7979358e8472fbf52a583c6e1068f.camel@archlinux.org>
On Fri, Aug 23, 2019 at 4:48 PM Filipe Laíns <lains@archlinux.org> wrote:
>
> On Fri, 2019-08-23 at 16:32 +0200, Benjamin Tissoires wrote:
> > The problem I have with quirks, and that I explained to Filipe on IRC
> > is that this is kernel ABI. Even if there is a very low chance we have
> > someone using this, re-using the same drv_data bit in the future might
> > break someone's device.
>
> But we can reserve those bits. I don't expect there to be a ton of
> devices that need to be quirked, so using the remaining bits should be
> perfectly fine. Do you agree?
Nope. If the code is not ready for upstream, it should not be included.
Cheers,
Benjamin
^ permalink raw reply
* Re: [Resubmit] Read battery voltage from Logitech Gaming mice
From: Filipe Laíns @ 2019-08-23 14:48 UTC (permalink / raw)
To: Benjamin Tissoires, Pedro Vanzella, Bastien Nocera
Cc: open list:HID CORE LAYER, Jiri Kosina, lkml
In-Reply-To: <CAO-hwJ+=dAyFnUfiPSmiGpzYTj=9BPDdeKQOY7UoCOvwQ5CH7Q@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 483 bytes --]
On Fri, 2019-08-23 at 16:32 +0200, Benjamin Tissoires wrote:
> The problem I have with quirks, and that I explained to Filipe on IRC
> is that this is kernel ABI. Even if there is a very low chance we have
> someone using this, re-using the same drv_data bit in the future might
> break someone's device.
But we can reserve those bits. I don't expect there to be a ton of
devices that need to be quirked, so using the remaining bits should be
perfectly fine. Do you agree?
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [Resubmit] Read battery voltage from Logitech Gaming mice
From: Benjamin Tissoires @ 2019-08-23 14:32 UTC (permalink / raw)
To: Pedro Vanzella, Bastien Nocera
Cc: open list:HID CORE LAYER, Filipe Laíns, Jiri Kosina, lkml
In-Reply-To: <e6014a01-1094-9ec7-9b37-2abdf70e305f@pedrovanzella.com>
On Fri, Aug 23, 2019 at 4:22 PM Pedro Vanzella <pedro@pedrovanzella.com> wrote:
>
> Hi Benjamin,
>
> On 8/23/19 4:25 AM, Benjamin Tissoires wrote:
> > Hi Pedro,
> >
> > On Thu, Aug 22, 2019 at 10:19 PM Pedro Vanzella <pedro@pedrovanzella.com> wrote:
> >>
> >> Resumitting this after having rebased it against the latest changes.
> >
> > thanks for resubmitting. Sorry I wasn't able to provide feedback on
> > the last revision
> >
>
> No worries, I know how these things are.
>
> >>
> >> The gaming line of Logitech devices doesn't use the old hidpp20
> >> feature for battery level reporting. Instead, they report the
> >> current voltage of the battery, in millivolts.
> >>
> >> This patch set handles this case by adding a quirk to the
> >> devices we know to have this new feature, in both wired
> >> and wireless mode.
> >
> > So the quirk is in the end a bad idea after all. I had some chats with
> > Filipe that made me realize this.
>
> I actually resubmitted by Filipe's request, since the patches weren't
> applying cleanly anymore. The idea was to apply these patches and in the
> future refactor the code to use the feature discovery routines.
>
> > Re-reading our previous exchanges also made me understood why I wasn't
> > happy with the initial submission: for every request the code was
> > checking both features 0x1000 and 0x1001 when we can remember this
> > once and for all during hidpp_initialize_battery().
>
> Yeah I wasn't too happy about this either, but since there was nothing
> prohibiting some device to have both features enabled, I thought this
> wasn't too horrible.
I honestly don't think we will find one device that has both features
enabled. It doesn't make sense as the "new" feature allows for fine
tuning in the software, which would be moot if you also enable the
percentage.
>
> >
> > So I think we should remove the useless quirk in the end (bad idea
> > from me, I concede), and instead during hidpp_initialize_battery() set
> > the correct HIDPP_CAPABILITY_*.
> > Not entirely sure if we should try to call 0x1000, or 0x1001 or if we
> > should rely on the 0x0001 feature to know which feature is available,
> > but this should be implementation detail.
>
> I like the idea of calling 0x0001 once on device boot much better. I
> think we could easily just fetch the location for the features we know
> about and want to deal with once only. This also makes sure we support
> every single device that supports this feature, so that is a huge plus.
>
> In fact, I think we should _not_ call 0x0001 on battery init, but only
> call battery init _after_ we called 0x0001 and discovered either 0x1000
> or 0x1001 (or the solar battery feature, or any other one that might
> crop up in the feature).
ack for that
>
> >
> >>
> >> This version of the patch set is better split, as well as adding the
> >> quirk to make sure we don't needlessly probe every device connected.
> >
> > It is for sure easy to review, but doesn't make much sense in the end.
> > I think we should squash all the patches together as you are just
> > adding one feature in the driver, and it is a little bit disturbing to
> > first add the quirk that has no use, then set up the structs when they
> > are not used, and so on, so forth.
>
> You're right. My first instinct was to send a single patch. As much as I
> tested this, I always feel like breaking the patch up post-facto will
> break a git bisect in the future and everyone will hate me :P
as long as the patches are compiling and are not breaking, git bisect
will not be a problem. However, we might end up with the last one,
which is not very explicit in what it does as it just enables the
features implemented previously.
>
> So we (you, me and Filipe) should probably come up with an action plan
> here. The way I see it there are two issues here: one is adding this
> feature, and the other is refactoring to use feature discovery for all
> features. There are advantages and disadvantages to doing one or another
> first and we might want to discuss that.
>
> By merging this first (probably after I resubmit it as a single squashed
> patch) we get to test it a bit better and have a usable feature sooner.
> Plenty of people have been requesting this and there is plenty of stuff
> that can be built on top of it, but only once this is actually merged I
> think.
>
> On the other hand, by first refactoring the rest of the code to use
> 0x0001 we avoid some rework on this patch. It should be minor, as most
> functions here do all the heavy lifting after the initial feature
> discovery, and are thus mostly independent from how that is done.
>
> I'm happy either way, so just let me know what you guys decide.
I think we should merge your v3 squashed series with a slight
autodetection during battery init, like the method you used in the v1.
This would remove the quirk, but keep the straightforward commands
when addressing battery data.
Relying on 0x0001 should be done separately and can come in in a later
patch IMO (unless you plan to work on it, in which case you can send
both at once).
The problem I have with quirks, and that I explained to Filipe on IRC
is that this is kernel ABI. Even if there is a very low chance we have
someone using this, re-using the same drv_data bit in the future might
break someone's device.
>
> If you guys (or anyone else reading this on the public list, really) has
> any input - naming things being notoriosly hard, I'm actually happy with
> nitpicking - I'd appreciate it. On that note, come to think of it, I'm
> not 100% sure reporting the voltage in milivolts is the standard way. I
> looked through the docs, but found no solid guideline. It was either
> that or a float, so I think I made the right call here, but still.
I am not sure either. Adding Bastien as he has a lot more experience in upower.
But I am under the impression that the kernel part is more "try to
deal with whatever the hardware provides, and deal with it in user
space".
^ permalink raw reply
* Re: [Resubmit] Read battery voltage from Logitech Gaming mice
From: Filipe Laíns @ 2019-08-23 14:29 UTC (permalink / raw)
To: Pedro Vanzella, Benjamin Tissoires
Cc: open list:HID CORE LAYER, Jiri Kosina, lkml
In-Reply-To: <e6014a01-1094-9ec7-9b37-2abdf70e305f@pedrovanzella.com>
[-- Attachment #1: Type: text/plain, Size: 1676 bytes --]
On Fri, 2019-08-23 at 10:22 -0400, Pedro Vanzella wrote:
> I actually resubmitted by Filipe's request, since the patches weren't
> applying cleanly anymore. The idea was to apply these patches and in the
> future refactor the code to use the feature discovery routines.
Yes, I want to refactor everything so I though there was no point in us
changing the patch set again. I did not review the first revision of
the patch set so if that works for you (Benjamin) we can just merge
that.
> So we (you, me and Filipe) should probably come up with an action plan
> here. The way I see it there are two issues here: one is adding this
> feature, and the other is refactoring to use feature discovery for all
> features. There are advantages and disadvantages to doing one or another
> first and we might want to discuss that.
>
> By merging this first (probably after I resubmit it as a single squashed
> patch) we get to test it a bit better and have a usable feature sooner.
> Plenty of people have been requesting this and there is plenty of stuff
> that can be built on top of it, but only once this is actually merged I
> think.
>
> On the other hand, by first refactoring the rest of the code to use
> 0x0001 we avoid some rework on this patch. It should be minor, as most
> functions here do all the heavy lifting after the initial feature
> discovery, and are thus mostly independent from how that is done.
>
> I'm happy either way, so just let me know what you guys decide.
I am also fine either way so I think we should just re-send the first
revision of your patch set as Benjamin requested.
Thank you,
Filipe Laíns
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [Resubmit] Read battery voltage from Logitech Gaming mice
From: Pedro Vanzella @ 2019-08-23 14:22 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: open list:HID CORE LAYER, Filipe Laíns, Jiri Kosina, lkml
In-Reply-To: <CAO-hwJKQcTpmk8cVf-YmKu2awXv_53=qfpy2yfmy2rgMu_DEug@mail.gmail.com>
Hi Benjamin,
On 8/23/19 4:25 AM, Benjamin Tissoires wrote:
> Hi Pedro,
>
> On Thu, Aug 22, 2019 at 10:19 PM Pedro Vanzella <pedro@pedrovanzella.com> wrote:
>>
>> Resumitting this after having rebased it against the latest changes.
>
> thanks for resubmitting. Sorry I wasn't able to provide feedback on
> the last revision
>
No worries, I know how these things are.
>>
>> The gaming line of Logitech devices doesn't use the old hidpp20
>> feature for battery level reporting. Instead, they report the
>> current voltage of the battery, in millivolts.
>>
>> This patch set handles this case by adding a quirk to the
>> devices we know to have this new feature, in both wired
>> and wireless mode.
>
> So the quirk is in the end a bad idea after all. I had some chats with
> Filipe that made me realize this.
I actually resubmitted by Filipe's request, since the patches weren't
applying cleanly anymore. The idea was to apply these patches and in the
future refactor the code to use the feature discovery routines.
> Re-reading our previous exchanges also made me understood why I wasn't
> happy with the initial submission: for every request the code was
> checking both features 0x1000 and 0x1001 when we can remember this
> once and for all during hidpp_initialize_battery().
Yeah I wasn't too happy about this either, but since there was nothing
prohibiting some device to have both features enabled, I thought this
wasn't too horrible.
>
> So I think we should remove the useless quirk in the end (bad idea
> from me, I concede), and instead during hidpp_initialize_battery() set
> the correct HIDPP_CAPABILITY_*.
> Not entirely sure if we should try to call 0x1000, or 0x1001 or if we
> should rely on the 0x0001 feature to know which feature is available,
> but this should be implementation detail.
I like the idea of calling 0x0001 once on device boot much better. I
think we could easily just fetch the location for the features we know
about and want to deal with once only. This also makes sure we support
every single device that supports this feature, so that is a huge plus.
In fact, I think we should _not_ call 0x0001 on battery init, but only
call battery init _after_ we called 0x0001 and discovered either 0x1000
or 0x1001 (or the solar battery feature, or any other one that might
crop up in the feature).
>
>>
>> This version of the patch set is better split, as well as adding the
>> quirk to make sure we don't needlessly probe every device connected.
>
> It is for sure easy to review, but doesn't make much sense in the end.
> I think we should squash all the patches together as you are just
> adding one feature in the driver, and it is a little bit disturbing to
> first add the quirk that has no use, then set up the structs when they
> are not used, and so on, so forth.
You're right. My first instinct was to send a single patch. As much as I
tested this, I always feel like breaking the patch up post-facto will
break a git bisect in the future and everyone will hate me :P
So we (you, me and Filipe) should probably come up with an action plan
here. The way I see it there are two issues here: one is adding this
feature, and the other is refactoring to use feature discovery for all
features. There are advantages and disadvantages to doing one or another
first and we might want to discuss that.
By merging this first (probably after I resubmit it as a single squashed
patch) we get to test it a bit better and have a usable feature sooner.
Plenty of people have been requesting this and there is plenty of stuff
that can be built on top of it, but only once this is actually merged I
think.
On the other hand, by first refactoring the rest of the code to use
0x0001 we avoid some rework on this patch. It should be minor, as most
functions here do all the heavy lifting after the initial feature
discovery, and are thus mostly independent from how that is done.
I'm happy either way, so just let me know what you guys decide.
If you guys (or anyone else reading this on the public list, really) has
any input - naming things being notoriosly hard, I'm actually happy with
nitpicking - I'd appreciate it. On that note, come to think of it, I'm
not 100% sure reporting the voltage in milivolts is the standard way. I
looked through the docs, but found no solid guideline. It was either
that or a float, so I think I made the right call here, but still.
- Pedro
>
> Cheers,
> Benjamin
>
>>
>>
^ permalink raw reply
* [PATCH] input: keyboard: snvs_pwrkey: Send press and release event for i.MX6 S,DL and Q
From: Robin van der Gracht @ 2019-08-23 12:30 UTC (permalink / raw)
To: Robin van der Gracht
Cc: Rob Herring, Mark Rutland, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
Dmitry Torokhov, devicetree, linux-arm-kernel, linux-kernel,
linux-input, Robin Gong, Adam Ford
The older generation i.MX6 processors send a powerdown request interrupt if
the powerkey is released before a hard shutdown (5 second press). This should
allow software to bring down the SoC safely.
For this driver to work as a regular powerkey with the older SoCs, we need to
send a keypress AND release when we get the powerdown request interrupt.
Signed-off-by: Robin van der Gracht <robin@protonic.nl>
---
arch/arm/boot/dts/imx6qdl.dtsi | 2 +-
arch/arm/boot/dts/imx6sll.dtsi | 2 +-
arch/arm/boot/dts/imx6sx.dtsi | 2 +-
arch/arm/boot/dts/imx6ul.dtsi | 2 +-
arch/arm/boot/dts/imx7s.dtsi | 2 +-
drivers/input/keyboard/Kconfig | 2 +-
drivers/input/keyboard/snvs_pwrkey.c | 59 +++++++++++++++++++++++-----
7 files changed, 56 insertions(+), 15 deletions(-)
diff --git a/arch/arm/boot/dts/imx6qdl.dtsi b/arch/arm/boot/dts/imx6qdl.dtsi
index b3a77bcf00d51..c10d12658743c 100644
--- a/arch/arm/boot/dts/imx6qdl.dtsi
+++ b/arch/arm/boot/dts/imx6qdl.dtsi
@@ -836,7 +836,7 @@
};
snvs_pwrkey: snvs-powerkey {
- compatible = "fsl,sec-v4.0-pwrkey";
+ compatible = "fsl,imx6qdl-sec-v4.0-pwrkey";
regmap = <&snvs>;
interrupts = <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>;
linux,keycode = <KEY_POWER>;
diff --git a/arch/arm/boot/dts/imx6sll.dtsi b/arch/arm/boot/dts/imx6sll.dtsi
index 1b4899f0fcded..91c7d5bdcc359 100644
--- a/arch/arm/boot/dts/imx6sll.dtsi
+++ b/arch/arm/boot/dts/imx6sll.dtsi
@@ -571,7 +571,7 @@
};
snvs_pwrkey: snvs-powerkey {
- compatible = "fsl,sec-v4.0-pwrkey";
+ compatible = "fsl,imx6sx-sec-v4.0-pwrkey";
regmap = <&snvs>;
interrupts = <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>;
linux,keycode = <KEY_POWER>;
diff --git a/arch/arm/boot/dts/imx6sx.dtsi b/arch/arm/boot/dts/imx6sx.dtsi
index b16a123990a26..b6736db65350f 100644
--- a/arch/arm/boot/dts/imx6sx.dtsi
+++ b/arch/arm/boot/dts/imx6sx.dtsi
@@ -733,7 +733,7 @@
};
snvs_pwrkey: snvs-powerkey {
- compatible = "fsl,sec-v4.0-pwrkey";
+ compatible = "fsl,imx6sx-sec-v4.0-pwrkey";
regmap = <&snvs>;
interrupts = <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>;
linux,keycode = <KEY_POWER>;
diff --git a/arch/arm/boot/dts/imx6ul.dtsi b/arch/arm/boot/dts/imx6ul.dtsi
index a7f6d1d58e20d..d4678c52b55db 100644
--- a/arch/arm/boot/dts/imx6ul.dtsi
+++ b/arch/arm/boot/dts/imx6ul.dtsi
@@ -644,7 +644,7 @@
};
snvs_pwrkey: snvs-powerkey {
- compatible = "fsl,sec-v4.0-pwrkey";
+ compatible = "fsl,imx6sx-sec-v4.0-pwrkey";
regmap = <&snvs>;
interrupts = <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>;
linux,keycode = <KEY_POWER>;
diff --git a/arch/arm/boot/dts/imx7s.dtsi b/arch/arm/boot/dts/imx7s.dtsi
index 106711d2c01b0..bb68c23beb199 100644
--- a/arch/arm/boot/dts/imx7s.dtsi
+++ b/arch/arm/boot/dts/imx7s.dtsi
@@ -604,7 +604,7 @@
};
snvs_pwrkey: snvs-powerkey {
- compatible = "fsl,sec-v4.0-pwrkey";
+ compatible = "fsl,imx6sx-sec-v4.0-pwrkey";
regmap = <&snvs>;
interrupts = <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>;
linux,keycode = <KEY_POWER>;
diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
index 7c4f19dab34fd..937e58da5ce17 100644
--- a/drivers/input/keyboard/Kconfig
+++ b/drivers/input/keyboard/Kconfig
@@ -436,7 +436,7 @@ config KEYBOARD_SNVS_PWRKEY
depends on OF
help
This is the snvs powerkey driver for the Freescale i.MX application
- processors that are newer than i.MX6 SX.
+ processors.
To compile this driver as a module, choose M here; the
module will be called snvs_pwrkey.
diff --git a/drivers/input/keyboard/snvs_pwrkey.c b/drivers/input/keyboard/snvs_pwrkey.c
index 5342d8d45f811..c321e5f2d1087 100644
--- a/drivers/input/keyboard/snvs_pwrkey.c
+++ b/drivers/input/keyboard/snvs_pwrkey.c
@@ -29,6 +29,11 @@
#define DEBOUNCE_TIME 30
#define REPEAT_INTERVAL 60
+enum imx_snvs_hwtype {
+ IMX6SX_SNVS, /* i.MX6 SoloX and newer */
+ IMX6QDL_SNVS, /* i.MX6 Solo, DualLite adn Quad */
+};
+
struct pwrkey_drv_data {
struct regmap *snvs;
int irq;
@@ -37,6 +42,19 @@ struct pwrkey_drv_data {
int wakeup;
struct timer_list check_timer;
struct input_dev *input;
+ enum imx_snvs_hwtype hwtype;
+};
+
+static const struct platform_device_id imx_snvs_devtype[] = {
+ {
+ .name = "imx6sx-snvs-pwrkey",
+ .driver_data = IMX6SX_SNVS,
+ }, {
+ .name = "imx6qdl-snvs-pwrkey",
+ .driver_data = IMX6QDL_SNVS,
+ }, {
+ /* sentinel */
+ }
};
static void imx_imx_snvs_check_for_events(struct timer_list *t)
@@ -67,13 +85,23 @@ static irqreturn_t imx_snvs_pwrkey_interrupt(int irq, void *dev_id)
{
struct platform_device *pdev = dev_id;
struct pwrkey_drv_data *pdata = platform_get_drvdata(pdev);
+ struct input_dev *input = pdata->input;
u32 lp_status;
- pm_wakeup_event(pdata->input->dev.parent, 0);
+ pm_wakeup_event(input->dev.parent, 0);
regmap_read(pdata->snvs, SNVS_LPSR_REG, &lp_status);
- if (lp_status & SNVS_LPSR_SPO)
- mod_timer(&pdata->check_timer, jiffies + msecs_to_jiffies(DEBOUNCE_TIME));
+ if (lp_status & SNVS_LPSR_SPO) {
+ if (pdata->hwtype == IMX6QDL_SNVS) {
+ input_report_key(input, pdata->keycode, 1);
+ input_report_key(input, pdata->keycode, 0);
+ input_sync(input);
+ pm_relax(input->dev.parent);
+ } else {
+ mod_timer(&pdata->check_timer,
+ jiffies + msecs_to_jiffies(DEBOUNCE_TIME));
+ }
+ }
/* clear SPO status */
regmap_write(pdata->snvs, SNVS_LPSR_REG, SNVS_LPSR_SPO);
@@ -88,11 +116,24 @@ static void imx_snvs_pwrkey_act(void *pdata)
del_timer_sync(&pd->check_timer);
}
+static const struct of_device_id imx_snvs_pwrkey_ids[] = {
+ {
+ .compatible = "fsl,imx6sx-sec-v4.0-pwrkey",
+ .data = &imx_snvs_devtype[IMX6SX_SNVS],
+ }, {
+ .compatible = "fsl,imx6qdl-sec-v4.0-pwrkey",
+ .data = &imx_snvs_devtype[IMX6QDL_SNVS],
+ },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, imx_snvs_pwrkey_ids);
+
static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
{
struct pwrkey_drv_data *pdata = NULL;
struct input_dev *input = NULL;
struct device_node *np;
+ const struct of_device_id *of_id;
int error;
/* Get SNVS register Page */
@@ -100,6 +141,11 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
if (!np)
return -ENODEV;
+ of_id = of_match_node(imx_snvs_pwrkey_ids, np);
+ if (!of_id)
+ return -ENODEV;
+ pdev->id_entry = of_id->data;
+
pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
if (!pdata)
return -ENOMEM;
@@ -116,6 +162,7 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
}
pdata->wakeup = of_property_read_bool(np, "wakeup-source");
+ pdata->hwtype = pdev->id_entry->driver_data;
pdata->irq = platform_get_irq(pdev, 0);
if (pdata->irq < 0) {
@@ -175,12 +222,6 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
return 0;
}
-static const struct of_device_id imx_snvs_pwrkey_ids[] = {
- { .compatible = "fsl,sec-v4.0-pwrkey" },
- { /* sentinel */ }
-};
-MODULE_DEVICE_TABLE(of, imx_snvs_pwrkey_ids);
-
static struct platform_driver imx_snvs_pwrkey_driver = {
.driver = {
.name = "snvs_pwrkey",
--
2.20.1
^ permalink raw reply related
* Re: [PATCH 00/11] Face lift for bu21013_ts driver
From: Linus Walleij @ 2019-08-23 11:30 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Linux Input, linux-kernel@vger.kernel.org
In-Reply-To: <20190821174257.GC76194@dtor-ws>
On Wed, Aug 21, 2019 at 7:43 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> The issue is this:
>
> static void bu21013_disable_chip(void *_ts)
> {
> struct bu21013_ts *ts = ts;
>
> which shuts up gcc about the fact that 'ts' is uninitialized, it should
> have said "ts = _ts". I guess it is a lesson for me to not call the voi
> d pointer argument almost the same name as the structure, as it is easy
> to miss in the review. The compiler would not care in either case, but a
> human might have noticed.
>
> Can you please try making this change (and the same in power off
> handler)?
Yes this works! :)
Tested-by: Linus Walleij <linus.walleij@linaro.org>
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH] Input: ili210x - switch to using threaded IRQ
From: Marek Vasut @ 2019-08-23 10:34 UTC (permalink / raw)
To: Dmitry Torokhov, linux-input; +Cc: linux-kernel
In-Reply-To: <20190811161104.GA43556@dtor-ws>
On 8/11/19 6:11 PM, Dmitry Torokhov wrote:
> Let's switch the driver to using threaded IRQ so that we do not need to
> manage the interrupt and work separately, and we do not acknowledge
> interrupt until we finished handling it completely.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
On ILI2117
Tested-by: Marek Vasut <marex@denx.de>
^ permalink raw reply
* Re: [PATCH v2] hid-logitech-dj: add the new Lightspeed receiver
From: Benjamin Tissoires @ 2019-08-23 10:06 UTC (permalink / raw)
To: Filipe Laíns
Cc: Nestor Lopez Casado, Jiri Kosina, open list:HID CORE LAYER, lkml
In-Reply-To: <20190730122458.5275-1-lains@archlinux.org>
On Tue, Jul 30, 2019 at 2:26 PM Filipe Laíns <lains@archlinux.org> wrote:
>
> This patchs adds the new Lightspeed receiver. Currently it seems to only
> be used in the G305.
>
> Signed-off-by: Filipe Laíns <lains@archlinux.org>
Applied to for-5.4/logitech
Cheers,
Benjamin
> ---
> drivers/hid/hid-ids.h | 3 ++-
> drivers/hid/hid-logitech-dj.c | 13 +++++++++++--
> 2 files changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> index fb19eefbc0b3..61b954fcfc2e 100644
> --- a/drivers/hid/hid-ids.h
> +++ b/drivers/hid/hid-ids.h
> @@ -768,7 +768,8 @@
> #define USB_DEVICE_ID_LOGITECH_NANO_RECEIVER 0xc52f
> #define USB_DEVICE_ID_LOGITECH_UNIFYING_RECEIVER_2 0xc532
> #define USB_DEVICE_ID_LOGITECH_NANO_RECEIVER_2 0xc534
> -#define USB_DEVICE_ID_LOGITECH_NANO_RECEIVER_LIGHTSPEED 0xc539
> +#define USB_DEVICE_ID_LOGITECH_NANO_RECEIVER_LIGHTSPEED_1 0xc539
> +#define USB_DEVICE_ID_LOGITECH_NANO_RECEIVER_LIGHTSPEED_1_1 0xc53f
> #define USB_DEVICE_ID_LOGITECH_NANO_RECEIVER_POWERPLAY 0xc53a
> #define USB_DEVICE_ID_SPACETRAVELLER 0xc623
> #define USB_DEVICE_ID_SPACENAVIGATOR 0xc626
> diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
> index 4a68960b131f..d718f01f56d3 100644
> --- a/drivers/hid/hid-logitech-dj.c
> +++ b/drivers/hid/hid-logitech-dj.c
> @@ -968,7 +968,12 @@ static void logi_hidpp_recv_queue_notif(struct hid_device *hdev,
> logi_hidpp_dev_conn_notif_equad(hdev, hidpp_report, &workitem);
> break;
> case 0x0c:
> - device_type = "eQUAD Lightspeed";
> + device_type = "eQUAD Lightspeed 1";
> + logi_hidpp_dev_conn_notif_equad(hdev, hidpp_report, &workitem);
> + workitem.reports_supported |= STD_KEYBOARD;
> + break;
> + case 0x0d:
> + device_type = "eQUAD Lightspeed 1_1";
> logi_hidpp_dev_conn_notif_equad(hdev, hidpp_report, &workitem);
> workitem.reports_supported |= STD_KEYBOARD;
> break;
> @@ -1832,7 +1837,11 @@ static const struct hid_device_id logi_dj_receivers[] = {
> .driver_data = recvr_type_hidpp},
> { /* Logitech lightspeed receiver (0xc539) */
> HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
> - USB_DEVICE_ID_LOGITECH_NANO_RECEIVER_LIGHTSPEED),
> + USB_DEVICE_ID_LOGITECH_NANO_RECEIVER_LIGHTSPEED_1),
> + .driver_data = recvr_type_gaming_hidpp},
> + { /* Logitech lightspeed receiver (0xc53f) */
> + HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
> + USB_DEVICE_ID_LOGITECH_NANO_RECEIVER_LIGHTSPEED_1_1),
> .driver_data = recvr_type_gaming_hidpp},
> { /* Logitech 27 MHz HID++ 1.0 receiver (0xc513) */
> HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_MX3000_RECEIVER),
> --
> 2.22.0
^ permalink raw reply
* Re: [PATCH] HID: logitech-dj: add support of the G700(s) receiver
From: Benjamin Tissoires @ 2019-08-23 10:06 UTC (permalink / raw)
To: Hans de Goede; +Cc: Jiri Kosina, open list:HID CORE LAYER, lkml
In-Reply-To: <f97403fd-4bf0-f82b-06e7-8bf4dcb2b2aa@redhat.com>
On Mon, Aug 12, 2019 at 6:26 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 12-08-19 18:08, Benjamin Tissoires wrote:
> > Both the G700 and the G700s are sharing the same receiver.
> > Include support for this receiver in hid-logitech-dj so that userspace
> > can differentiate both.
> >
> > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>
> Nice, looks good to me:
>
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
thanks
Applied to for-5.4/logitech
Cheers,
Benjamin
>
> Regards,
>
> Hans
>
>
>
> > ---
> > drivers/hid/hid-logitech-dj.c | 5 +++++
> > 1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
> > index c547cba05fbb..d6250b0cb9f8 100644
> > --- a/drivers/hid/hid-logitech-dj.c
> > +++ b/drivers/hid/hid-logitech-dj.c
> > @@ -959,6 +959,7 @@ static void logi_hidpp_recv_queue_notif(struct hid_device *hdev,
> > break;
> > case 0x07:
> > device_type = "eQUAD step 4 Gaming";
> > + logi_hidpp_dev_conn_notif_equad(hdev, hidpp_report, &workitem);
> > break;
> > case 0x08:
> > device_type = "eQUAD step 4 for gamepads";
> > @@ -1832,6 +1833,10 @@ static const struct hid_device_id logi_dj_receivers[] = {
> > HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
> > USB_DEVICE_ID_LOGITECH_NANO_RECEIVER_2),
> > .driver_data = recvr_type_hidpp},
> > + { /* Logitech G700(s) receiver (0xc531) */
> > + HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
> > + 0xc531),
> > + .driver_data = recvr_type_gaming_hidpp},
> > { /* Logitech lightspeed receiver (0xc539) */
> > HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
> > USB_DEVICE_ID_LOGITECH_NANO_RECEIVER_LIGHTSPEED),
> >
^ permalink raw reply
* Re: [Resubmit] Read battery voltage from Logitech Gaming mice
From: Benjamin Tissoires @ 2019-08-23 8:25 UTC (permalink / raw)
To: Pedro Vanzella
Cc: open list:HID CORE LAYER, Filipe Laíns, Jiri Kosina, lkml
In-Reply-To: <20190822201849.28924-1-pedro@pedrovanzella.com>
Hi Pedro,
On Thu, Aug 22, 2019 at 10:19 PM Pedro Vanzella <pedro@pedrovanzella.com> wrote:
>
> Resumitting this after having rebased it against the latest changes.
thanks for resubmitting. Sorry I wasn't able to provide feedback on
the last revision
>
> The gaming line of Logitech devices doesn't use the old hidpp20
> feature for battery level reporting. Instead, they report the
> current voltage of the battery, in millivolts.
>
> This patch set handles this case by adding a quirk to the
> devices we know to have this new feature, in both wired
> and wireless mode.
So the quirk is in the end a bad idea after all. I had some chats with
Filipe that made me realize this.
Re-reading our previous exchanges also made me understood why I wasn't
happy with the initial submission: for every request the code was
checking both features 0x1000 and 0x1001 when we can remember this
once and for all during hidpp_initialize_battery().
So I think we should remove the useless quirk in the end (bad idea
from me, I concede), and instead during hidpp_initialize_battery() set
the correct HIDPP_CAPABILITY_*.
Not entirely sure if we should try to call 0x1000, or 0x1001 or if we
should rely on the 0x0001 feature to know which feature is available,
but this should be implementation detail.
>
> This version of the patch set is better split, as well as adding the
> quirk to make sure we don't needlessly probe every device connected.
It is for sure easy to review, but doesn't make much sense in the end.
I think we should squash all the patches together as you are just
adding one feature in the driver, and it is a little bit disturbing to
first add the quirk that has no use, then set up the structs when they
are not used, and so on, so forth.
Cheers,
Benjamin
>
>
^ permalink raw reply
* Re: [PATCH v1 36/63] Input: atmel_mxt_ts - configure and use gpios as real gpios
From: Jiada Wang @ 2019-08-23 5:16 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: nick, linux-input, linux-kernel, george_davis
In-Reply-To: <20190816172422.GH121898@dtor-ws>
Hi Dmitry
On 2019/08/17 2:24, Dmitry Torokhov wrote:
> On Fri, Aug 16, 2019 at 05:34:58PM +0900, Jiada Wang wrote:
>> From: Kautuk Consul <kautuk_consul@mentor.com>
>>
>> The upstream Atmel mXT driver implementation seems to handle the
>> T19 GPIO/PWM object as a key pad. Keys can be defined in the
>> device tree ("linux,gpio-keymap") and will be transported as key
>> events to the Linux input device if GPIO state changes.
>>
>> With our hardware, the GPIO pins of the touch controller are
>> connected to a PWM/backlight controller and used as supervision
>> inputs. We like to read the status of the pins by a script or an
>> application in the sysfs.
>>
>> Adding newer sysfs entries which shall be placed in the input
>> class directory eg:
>> /sys/class/input/input<n>/backlight_error1
>
> No, if you want to export GPIO lines for external use create a gpiochip
> instance and register it with GPIO subsystem. No ad-hoc sysfs please.
>
Agree, I will drop this patch in v2 patch-set
Thanks,
Jiada
> Thanks.
>
^ permalink raw reply
* Re: [PATCH v1 31/63] Input: atmel_mxt_ts - add memory access interface via sysfs
From: Jiada Wang @ 2019-08-23 3:00 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: nick, linux-input, linux-kernel, george_davis
In-Reply-To: <20190816172920.GK121898@dtor-ws>
Hi Dmitry
On 2019/08/17 2:29, Dmitry Torokhov wrote:
> On Fri, Aug 16, 2019 at 05:34:19PM +0900, Jiada Wang wrote:
>> From: Nick Dyer <nick.dyer@itdev.co.uk>
>>
>> Atmel maXTouch chips can be addressed via an "Object Based Protocol" which
>> defines how i2c registers are mapped to different functions within the
>> chips. This interface exposes the register map and allows user-space
>> utilities to inspect and alter object configuration, and to view diagnostic
>> data, while the device is running.
>
> I'd say if we want this we should look into write support in regmap and
> switching the driver over.
>
since switch to regmap is non-trival, I will drop this patch in v2 patchset
Thanks,
Jiada
> Thanks.
>
^ permalink raw reply
* [PATCH v3 4/4] hid-logitech-hidpp: subscribe to battery voltage events
From: Pedro Vanzella @ 2019-08-22 20:18 UTC (permalink / raw)
To: linux-input
Cc: lains, Pedro Vanzella, Jiri Kosina, Benjamin Tissoires,
linux-kernel
In-Reply-To: <20190822201849.28924-1-pedro@pedrovanzella.com>
Like we do for other ways of interacting with the battery for other
devices, refresh the battery status and notify the power supply
subsystem of the changes in voltage and status.
Signed-off-by: Pedro Vanzella <pedro@pedrovanzella.com>
---
drivers/hid/hid-logitech-hidpp.c | 32 ++++++++++++++++++++++++++++++++
1 file changed, 32 insertions(+)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 06bee97d33b4..9f09ed6abbad 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -1310,6 +1310,35 @@ static int hidpp20_query_battery_voltage_info(struct hidpp_device *hidpp)
return 0;
}
+static int hidpp20_battery_voltage_event(struct hidpp_device *hidpp,
+ u8 *data, int size)
+{
+ struct hidpp_report *report = (struct hidpp_report *)data;
+ int status, voltage;
+ bool changed;
+
+ if (report->fap.feature_index != hidpp->battery.voltage_feature_index ||
+ report->fap.funcindex_clientid !=
+ EVENT_BATTERY_LEVEL_STATUS_BROADCAST)
+ return 0;
+
+ status = hidpp20_battery_map_status_voltage(report->fap.params,
+ &voltage);
+
+ hidpp->battery.online = status != POWER_SUPPLY_STATUS_NOT_CHARGING;
+
+ changed = voltage != hidpp->battery.voltage ||
+ status != hidpp->battery.status;
+
+ if (changed) {
+ hidpp->battery.voltage = voltage;
+ hidpp->battery.status = status;
+ if (hidpp->battery.ps)
+ power_supply_changed(hidpp->battery.ps);
+ }
+ return 0;
+}
+
static enum power_supply_property hidpp_battery_props[] = {
POWER_SUPPLY_PROP_ONLINE,
POWER_SUPPLY_PROP_STATUS,
@@ -3178,6 +3207,9 @@ static int hidpp_raw_hidpp_event(struct hidpp_device *hidpp, u8 *data,
ret = hidpp_solar_battery_event(hidpp, data, size);
if (ret != 0)
return ret;
+ ret = hidpp20_battery_voltage_event(hidpp, data, size);
+ if (ret != 0)
+ return ret;
}
if (hidpp->capabilities & HIDPP_CAPABILITY_HIDPP10_BATTERY) {
--
2.23.0
^ permalink raw reply related
* [PATCH v3 3/4] hid-logitech-hidpp: report battery voltage to the power supply
From: Pedro Vanzella @ 2019-08-22 20:18 UTC (permalink / raw)
To: linux-input
Cc: lains, Pedro Vanzella, Jiri Kosina, Benjamin Tissoires,
linux-kernel
In-Reply-To: <20190822201849.28924-1-pedro@pedrovanzella.com>
If we know the device supports reading its voltage, report that.
Note that the protocol only gives us the current voltage in millivolts.
Signed-off-by: Pedro Vanzella <pedro@pedrovanzella.com>
---
drivers/hid/hid-logitech-hidpp.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index e1ddc9cdcc8b..06bee97d33b4 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -1319,6 +1319,7 @@ static enum power_supply_property hidpp_battery_props[] = {
POWER_SUPPLY_PROP_SERIAL_NUMBER,
0, /* placeholder for POWER_SUPPLY_PROP_CAPACITY, */
0, /* placeholder for POWER_SUPPLY_PROP_CAPACITY_LEVEL, */
+ 0, /* placeholder for POWER_SUPPLY_PROP_VOLTAGE_NOW, */
};
static int hidpp_battery_get_property(struct power_supply *psy,
@@ -1356,6 +1357,9 @@ static int hidpp_battery_get_property(struct power_supply *psy,
case POWER_SUPPLY_PROP_SERIAL_NUMBER:
val->strval = hidpp->hid_dev->uniq;
break;
+ case POWER_SUPPLY_PROP_VOLTAGE_NOW:
+ val->intval = hidpp->battery.voltage;
+ break;
default:
ret = -EINVAL;
break;
@@ -3328,7 +3332,7 @@ static int hidpp_initialize_battery(struct hidpp_device *hidpp)
if (!battery_props)
return -ENOMEM;
- num_battery_props = ARRAY_SIZE(hidpp_battery_props) - 2;
+ num_battery_props = ARRAY_SIZE(hidpp_battery_props) - 3;
if (hidpp->capabilities & HIDPP_CAPABILITY_BATTERY_MILEAGE)
battery_props[num_battery_props++] =
@@ -3338,6 +3342,10 @@ static int hidpp_initialize_battery(struct hidpp_device *hidpp)
battery_props[num_battery_props++] =
POWER_SUPPLY_PROP_CAPACITY_LEVEL;
+ if (hidpp->capabilities & HIDPP_CAPABILITY_BATTERY_VOLTAGE)
+ battery_props[num_battery_props++] =
+ POWER_SUPPLY_PROP_VOLTAGE_NOW;
+
battery = &hidpp->battery;
n = atomic_inc_return(&battery_no) - 1;
--
2.23.0
^ permalink raw reply related
* [PATCH v3 2/4] hid-logitech-hidpp: add function to query battery voltage
From: Pedro Vanzella @ 2019-08-22 20:18 UTC (permalink / raw)
To: linux-input
Cc: lains, Pedro Vanzella, Jiri Kosina, Benjamin Tissoires,
linux-kernel
In-Reply-To: <20190822201849.28924-1-pedro@pedrovanzella.com>
When the device is brought up, if it's one of the devices we know
supports battery voltage checking, figure out the feature index
and get the battery voltage and status.
If everything went correctly, record the fact that we're capable
of querying battery voltage.
Signed-off-by: Pedro Vanzella <pedro@pedrovanzella.com>
---
drivers/hid/hid-logitech-hidpp.c | 95 ++++++++++++++++++++++++++++++++
1 file changed, 95 insertions(+)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 402ddba93adc..e1ddc9cdcc8b 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -88,6 +88,7 @@ MODULE_PARM_DESC(disable_tap_to_click,
#define HIDPP_CAPABILITY_HIDPP20_BATTERY BIT(1)
#define HIDPP_CAPABILITY_BATTERY_MILEAGE BIT(2)
#define HIDPP_CAPABILITY_BATTERY_LEVEL_STATUS BIT(3)
+#define HIDPP_CAPABILITY_BATTERY_VOLTAGE BIT(4)
/*
* There are two hidpp protocols in use, the first version hidpp10 is known
@@ -136,12 +137,14 @@ struct hidpp_report {
struct hidpp_battery {
u8 feature_index;
u8 solar_feature_index;
+ u8 voltage_feature_index;
struct power_supply_desc desc;
struct power_supply *ps;
char name[64];
int status;
int capacity;
int level;
+ int voltage; /* in millivolts */
bool online;
};
@@ -1220,6 +1223,93 @@ static int hidpp20_battery_event(struct hidpp_device *hidpp,
return 0;
}
+/* -------------------------------------------------------------------------- */
+/* 0x1001: Battery voltage */
+/* -------------------------------------------------------------------------- */
+
+#define HIDPP_PAGE_BATTERY_VOLTAGE 0x1001
+
+#define CMD_BATTERY_VOLTAGE_GET_BATTERY_VOLTAGE 0x00
+
+static int hidpp20_battery_map_status_voltage(u8 data[3], int *voltage)
+{
+ int status;
+
+ switch (data[2]) {
+ case 0x00: /* discharging */
+ status = POWER_SUPPLY_STATUS_DISCHARGING;
+ break;
+ case 0x10: /* wireless charging */
+ case 0x80: /* charging */
+ status = POWER_SUPPLY_STATUS_CHARGING;
+ break;
+ case 0x81: /* fully charged */
+ status = POWER_SUPPLY_STATUS_FULL;
+ break;
+ default:
+ status = POWER_SUPPLY_STATUS_NOT_CHARGING;
+ }
+
+ *voltage = (data[0] << 8) + data[1];
+
+ return status;
+}
+
+static int hidpp20_battery_get_battery_voltage(struct hidpp_device *hidpp,
+ u8 feature_index,
+ int *status, int *voltage)
+{
+ struct hidpp_report response;
+ int ret;
+ u8 *params = (u8 *)response.fap.params;
+
+ ret = hidpp_send_fap_command_sync(hidpp, feature_index,
+ CMD_BATTERY_VOLTAGE_GET_BATTERY_VOLTAGE,
+ NULL, 0, &response);
+
+ if (ret > 0) {
+ hid_err(hidpp->hid_dev, "%s: received protocol error 0x%02x\n",
+ __func__, ret);
+ return -EPROTO;
+ }
+ if (ret)
+ return ret;
+
+ hidpp->capabilities |= HIDPP_CAPABILITY_BATTERY_VOLTAGE;
+
+ *status = hidpp20_battery_map_status_voltage(params, voltage);
+
+ return 0;
+}
+
+static int hidpp20_query_battery_voltage_info(struct hidpp_device *hidpp)
+{
+ u8 feature_type;
+ int ret;
+ int status, voltage;
+
+ if (hidpp->battery.voltage_feature_index == 0xff) {
+ ret = hidpp_root_get_feature(hidpp, HIDPP_PAGE_BATTERY_VOLTAGE,
+ &hidpp->battery.voltage_feature_index,
+ &feature_type);
+ if (ret)
+ return ret;
+ }
+
+ ret = hidpp20_battery_get_battery_voltage(hidpp,
+ hidpp->battery.voltage_feature_index,
+ &status, &voltage);
+
+ if (ret)
+ return ret;
+
+ hidpp->battery.status = status;
+ hidpp->battery.voltage = voltage;
+ hidpp->battery.online = status != POWER_SUPPLY_STATUS_NOT_CHARGING;
+
+ return 0;
+}
+
static enum power_supply_property hidpp_battery_props[] = {
POWER_SUPPLY_PROP_ONLINE,
POWER_SUPPLY_PROP_STATUS,
@@ -3205,10 +3295,13 @@ static int hidpp_initialize_battery(struct hidpp_device *hidpp)
hidpp->battery.feature_index = 0xff;
hidpp->battery.solar_feature_index = 0xff;
+ hidpp->battery.voltage_feature_index = 0xff;
if (hidpp->protocol_major >= 2) {
if (hidpp->quirks & HIDPP_QUIRK_CLASS_K750)
ret = hidpp_solar_request_battery_event(hidpp);
+ else if (hidpp->quirks & HIDPP_QUIRK_BATTERY_VOLTAGE_X1001)
+ ret = hidpp20_query_battery_voltage_info(hidpp);
else
ret = hidpp20_query_battery_info(hidpp);
@@ -3409,6 +3502,8 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
hidpp10_query_battery_status(hidpp);
} else if (hidpp->capabilities & HIDPP_CAPABILITY_HIDPP20_BATTERY) {
hidpp20_query_battery_info(hidpp);
+ if (hidpp->capabilities & HIDPP_CAPABILITY_BATTERY_VOLTAGE)
+ hidpp20_query_battery_voltage_info(hidpp);
}
if (hidpp->battery.ps)
power_supply_changed(hidpp->battery.ps);
--
2.23.0
^ permalink raw reply related
* [PATCH v3 1/4] hid-logitech-hidpp: add quirk to handle battery voltage
From: Pedro Vanzella @ 2019-08-22 20:18 UTC (permalink / raw)
To: linux-input
Cc: lains, Pedro Vanzella, Jiri Kosina, Benjamin Tissoires,
linux-kernel
In-Reply-To: <20190822201849.28924-1-pedro@pedrovanzella.com>
This quirk allows us to pick which devices support the 0x1001 hidpp
feature to read the battery voltage.
Signed-off-by: Pedro Vanzella <pedro@pedrovanzella.com>
---
drivers/hid/hid-logitech-hidpp.c | 16 +++++++++++++---
1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 0179f7ed77e5..402ddba93adc 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -59,7 +59,8 @@ MODULE_PARM_DESC(disable_tap_to_click,
#define HIDPP_QUIRK_CLASS_G920 BIT(3)
#define HIDPP_QUIRK_CLASS_K750 BIT(4)
-/* bits 2..20 are reserved for classes */
+/* bits 2..1f are reserved for classes */
+#define HIDPP_QUIRK_BATTERY_VOLTAGE_X1001 BIT(20)
/* #define HIDPP_QUIRK_CONNECT_EVENTS BIT(21) disabled */
#define HIDPP_QUIRK_WTP_PHYSICAL_BUTTONS BIT(22)
#define HIDPP_QUIRK_NO_HIDINPUT BIT(23)
@@ -3732,6 +3733,13 @@ static const struct hid_device_id hidpp_devices[] = {
LDJ_DEVICE(0xb30b),
.driver_data = HIDPP_QUIRK_HIDPP_CONSUMER_VENDOR_KEYS },
+ { /* Logitech G403 Gaming Mouse over Lightspeed */
+ LDJ_DEVICE(0x405d),
+ .driver_data = HIDPP_QUIRK_BATTERY_VOLTAGE_X1001 },
+ { /* Logitech G900 Gaming Mouse over Lightspeed */
+ LDJ_DEVICE(0x4053),
+ .driver_data = HIDPP_QUIRK_BATTERY_VOLTAGE_X1001 },
+
{ LDJ_DEVICE(HID_ANY_ID) },
{ /* Keyboard LX501 (Y-RR53) */
@@ -3750,13 +3758,15 @@ static const struct hid_device_id hidpp_devices[] = {
{ L27MHZ_DEVICE(HID_ANY_ID) },
{ /* Logitech G403 Wireless Gaming Mouse over USB */
- HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC082) },
+ HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC082),
+ .driver_data = HIDPP_QUIRK_BATTERY_VOLTAGE_X1001 },
{ /* Logitech G703 Gaming Mouse over USB */
HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC087) },
{ /* Logitech G703 Hero Gaming Mouse over USB */
HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC090) },
{ /* Logitech G900 Gaming Mouse over USB */
- HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC081) },
+ HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC081),
+ .driver_data = HIDPP_QUIRK_BATTERY_VOLTAGE_X1001 },
{ /* Logitech G903 Gaming Mouse over USB */
HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC086) },
{ /* Logitech G903 Hero Gaming Mouse over USB */
--
2.23.0
^ permalink raw reply related
* [Resubmit] Read battery voltage from Logitech Gaming mice
From: Pedro Vanzella @ 2019-08-22 20:18 UTC (permalink / raw)
To: linux-input; +Cc: lains, Jiri Kosina, Benjamin Tissoires, linux-kernel
Resumitting this after having rebased it against the latest changes.
The gaming line of Logitech devices doesn't use the old hidpp20
feature for battery level reporting. Instead, they report the
current voltage of the battery, in millivolts.
This patch set handles this case by adding a quirk to the
devices we know to have this new feature, in both wired
and wireless mode.
This version of the patch set is better split, as well as adding the
quirk to make sure we don't needlessly probe every device connected.
^ permalink raw reply
* Re: [PATCH 2/3] input: Add SW_UNSUPPORT_INSERT define
From: Dmitry Torokhov @ 2019-08-22 18:16 UTC (permalink / raw)
To: bgoswami
Cc: alsa-devel, plai, tiwai, broonie, srinivas.kandagatla,
linux-input, Gopikrishnaiah Anandan
In-Reply-To: <1558730438-16524-1-git-send-email-bgoswami@codeaurora.org>
Hi Banajit,
On Fri, May 24, 2019 at 01:40:38PM -0700, bgoswami@codeaurora.org wrote:
> From: Banajit Goswami <bgoswami@codeaurora.org>
>
> Some devices may not support specific type of input devices. For example,
> when a headset or extension cable with GND/MIC swap is plugged into a
> headset jack that does not support the headset/cable, it needs to be
> reported with a corresponding input event. Also, increase the max values
> for INPUT_DEVICE_ID_SW_MAX and SW_MAX, to accommodate future extension of
> the number of event.
I do not think that adding this switch is a good idea as it will not
allow you to easily extend the reporting and to convey to the user why
something is not supported. I would look into alternate mechanism to
signal when and why a headset/peripheral was rejected.
Thanks.
--
Dmitry
^ permalink raw reply
* [PATCH] HID: hidraw: replace printk() with corresponding pr_xx() variant
From: Rishi Gupta @ 2019-08-22 16:43 UTC (permalink / raw)
To: jikos
Cc: benjamin.tissoires, dmitry.torokhov, linux-input, linux-kernel,
Rishi Gupta
This commit replaces direct invocations of printk with
their appropriate pr_info/warn() variant.
Signed-off-by: Rishi Gupta <gupt21@gmail.com>
---
drivers/hid/hidraw.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c
index 006bd6f..67b652b 100644
--- a/drivers/hid/hidraw.c
+++ b/drivers/hid/hidraw.c
@@ -197,14 +197,14 @@ static ssize_t hidraw_get_report(struct file *file, char __user *buffer, size_t
}
if (count > HID_MAX_BUFFER_SIZE) {
- printk(KERN_WARNING "hidraw: pid %d passed too large report\n",
+ pr_warn("hidraw: pid %d passed too large report\n",
task_pid_nr(current));
ret = -EINVAL;
goto out;
}
if (count < 2) {
- printk(KERN_WARNING "hidraw: pid %d passed too short report\n",
+ pr_warn("hidraw: pid %d passed too short report\n",
task_pid_nr(current));
ret = -EINVAL;
goto out;
@@ -597,7 +597,7 @@ int __init hidraw_init(void)
if (result < 0)
goto error_class;
- printk(KERN_INFO "hidraw: raw HID events driver (C) Jiri Kosina\n");
+ pr_info("hidraw: raw HID events driver (C) Jiri Kosina\n");
out:
return result;
--
2.7.4
^ permalink raw reply related
* Re: [PATCH 0/2] HID multitouch: small fixes
From: Benjamin Tissoires @ 2019-08-22 16:25 UTC (permalink / raw)
To: Matthias Fend, Jiri Kosina; +Cc: open list:HID CORE LAYER, lkml
In-Reply-To: <20190812162326.14253-1-benjamin.tissoires@redhat.com>
On Mon, Aug 12, 2019 at 6:23 PM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> First one should prevent us to add more quirks for Win8 devices
> that have a trackstick.
> Second one is a weird device that doesnt work properly in recent
> kernels.
Looks like there is not much traction for this series.
The test suite is still passing, so I applied the series in for-5.4/multitouch
Cheers,
Benjamin
>
> Cheers,
> Benjamin
>
> Benjamin Tissoires (2):
> HID: multitouch: do not filter mice nodes
> HID: multitouch: add support for the Smart Tech panel
>
> drivers/hid/hid-multitouch.c | 37 +++++++++++++++++++++++++++++-------
> 1 file changed, 30 insertions(+), 7 deletions(-)
>
> --
> 2.19.2
>
^ permalink raw reply
* Re: [PATCH 1/2] HID: do not call hid_set_drvdata(hdev, NULL) in drivers
From: Benjamin Tissoires @ 2019-08-22 15:50 UTC (permalink / raw)
To: Bruno Prémont
Cc: Jonathan Cameron, Srinivas Pandruvada, Ping Cheng, Jason Gerecke,
Jiri Kosina, open list:HID CORE LAYER, lkml
In-Reply-To: <20190813075358.2a3cbfbd@pluto.restena.lu>
On Tue, Aug 13, 2019 at 7:54 AM Bruno Prémont <bonbons@linux-vserver.org> wrote:
>
> On Mon, 12 Aug 2019 18:27:39 +0200 Benjamin Tissoires wrote:
> > This is a common pattern in the HID drivers to reset the drvdata. Some
> > do it properly, some do it only in case of failure.
> >
> > But, this is actually already handled by driver core, so there is no need
> > to do it manually.
> >
> > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>
> For hid-picolcd_core.c:
> Acked-by: Bruno Prémont <bonbons@linux-vserver.org>
Thanks for the acks.
Applied to for-5.4/cleanup
Cheers,
Benjamin
>
> > ---
> > drivers/hid/hid-cougar.c | 6 ++----
> > drivers/hid/hid-gfrm.c | 7 -------
> > drivers/hid/hid-lenovo.c | 2 --
> > drivers/hid/hid-picolcd_core.c | 7 +------
> > drivers/hid/hid-sensor-hub.c | 1 -
> > 5 files changed, 3 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/hid/hid-cougar.c b/drivers/hid/hid-cougar.c
> > index e0bb7b34f3a4..4ff3bc1d25e2 100644
> > --- a/drivers/hid/hid-cougar.c
> > +++ b/drivers/hid/hid-cougar.c
> > @@ -207,7 +207,7 @@ static int cougar_probe(struct hid_device *hdev,
> > error = hid_parse(hdev);
> > if (error) {
> > hid_err(hdev, "parse failed\n");
> > - goto fail;
> > + return error;
> > }
> >
> > if (hdev->collection->usage == COUGAR_VENDOR_USAGE) {
> > @@ -219,7 +219,7 @@ static int cougar_probe(struct hid_device *hdev,
> > error = hid_hw_start(hdev, connect_mask);
> > if (error) {
> > hid_err(hdev, "hw start failed\n");
> > - goto fail;
> > + return error;
> > }
> >
> > error = cougar_bind_shared_data(hdev, cougar);
> > @@ -249,8 +249,6 @@ static int cougar_probe(struct hid_device *hdev,
> >
> > fail_stop_and_cleanup:
> > hid_hw_stop(hdev);
> > -fail:
> > - hid_set_drvdata(hdev, NULL);
> > return error;
> > }
> >
> > diff --git a/drivers/hid/hid-gfrm.c b/drivers/hid/hid-gfrm.c
> > index 86c317320bf2..699186ff2349 100644
> > --- a/drivers/hid/hid-gfrm.c
> > +++ b/drivers/hid/hid-gfrm.c
> > @@ -123,12 +123,6 @@ static int gfrm_probe(struct hid_device *hdev, const struct hid_device_id *id)
> > return ret;
> > }
> >
> > -static void gfrm_remove(struct hid_device *hdev)
> > -{
> > - hid_hw_stop(hdev);
> > - hid_set_drvdata(hdev, NULL);
> > -}
> > -
> > static const struct hid_device_id gfrm_devices[] = {
> > { HID_BLUETOOTH_DEVICE(0x58, 0x2000),
> > .driver_data = GFRM100 },
> > @@ -142,7 +136,6 @@ static struct hid_driver gfrm_driver = {
> > .name = "gfrm",
> > .id_table = gfrm_devices,
> > .probe = gfrm_probe,
> > - .remove = gfrm_remove,
> > .input_mapping = gfrm_input_mapping,
> > .raw_event = gfrm_raw_event,
> > .input_configured = gfrm_input_configured,
> > diff --git a/drivers/hid/hid-lenovo.c b/drivers/hid/hid-lenovo.c
> > index 364bc7f11d9d..96fa2a2c2cd3 100644
> > --- a/drivers/hid/hid-lenovo.c
> > +++ b/drivers/hid/hid-lenovo.c
> > @@ -866,8 +866,6 @@ static void lenovo_remove_tpkbd(struct hid_device *hdev)
> >
> > led_classdev_unregister(&data_pointer->led_micmute);
> > led_classdev_unregister(&data_pointer->led_mute);
> > -
> > - hid_set_drvdata(hdev, NULL);
> > }
> >
> > static void lenovo_remove_cptkbd(struct hid_device *hdev)
> > diff --git a/drivers/hid/hid-picolcd_core.c b/drivers/hid/hid-picolcd_core.c
> > index 5f7a39a5d4af..1b5c63241af0 100644
> > --- a/drivers/hid/hid-picolcd_core.c
> > +++ b/drivers/hid/hid-picolcd_core.c
> > @@ -534,8 +534,7 @@ static int picolcd_probe(struct hid_device *hdev,
> > data = kzalloc(sizeof(struct picolcd_data), GFP_KERNEL);
> > if (data == NULL) {
> > hid_err(hdev, "can't allocate space for Minibox PicoLCD device data\n");
> > - error = -ENOMEM;
> > - goto err_no_cleanup;
> > + return -ENOMEM;
> > }
> >
> > spin_lock_init(&data->lock);
> > @@ -597,9 +596,6 @@ static int picolcd_probe(struct hid_device *hdev,
> > hid_hw_stop(hdev);
> > err_cleanup_data:
> > kfree(data);
> > -err_no_cleanup:
> > - hid_set_drvdata(hdev, NULL);
> > -
> > return error;
> > }
> >
> > @@ -635,7 +631,6 @@ static void picolcd_remove(struct hid_device *hdev)
> > picolcd_exit_cir(data);
> > picolcd_exit_keys(data);
> >
> > - hid_set_drvdata(hdev, NULL);
> > mutex_destroy(&data->mutex);
> > /* Finally, clean up the picolcd data itself */
> > kfree(data);
> > diff --git a/drivers/hid/hid-sensor-hub.c b/drivers/hid/hid-sensor-hub.c
> > index be92a6f79687..94c7398b5c27 100644
> > --- a/drivers/hid/hid-sensor-hub.c
> > +++ b/drivers/hid/hid-sensor-hub.c
> > @@ -742,7 +742,6 @@ static void sensor_hub_remove(struct hid_device *hdev)
> > }
> > spin_unlock_irqrestore(&data->lock, flags);
> > mfd_remove_devices(&hdev->dev);
> > - hid_set_drvdata(hdev, NULL);
> > mutex_destroy(&data->mutex);
> > }
> >
^ permalink raw reply
* Re: [PATCH 2/2] HID: wacom: do not call hid_set_drvdata(hdev, NULL)
From: Benjamin Tissoires @ 2019-08-22 15:49 UTC (permalink / raw)
To: Jason Gerecke
Cc: Bruno Prémont, Jonathan Cameron, Srinivas Pandruvada,
Ping Cheng, Jason Gerecke, Jiri Kosina, Linux Input, LKML
In-Reply-To: <CANRwn3QQcGVGx2KjoU0sG70gSjzwjDKUuL_8q-oYFHRYrn4qGQ@mail.gmail.com>
On Sat, Aug 17, 2019 at 12:04 AM Jason Gerecke <killertofu@gmail.com> wrote:
>
> On Mon, Aug 12, 2019 at 9:29 AM Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
> >
> > This is a common pattern in the HID drivers to reset the drvdata.
> > However, this is actually already handled by driver core, so there
> > is no need to do it manually.
> >
> > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>
> Acked-by: Jason Gerecke <jason.gerecke@wacom.com>
Thanks
Applied to for-5.4/wacom
Cheers,
Benjamin
>
> Jason
> ---
> Now instead of four in the eights place /
> you’ve got three, ‘Cause you added one /
> (That is to say, eight) to the two, /
> But you can’t take seven from three, /
> So you look at the sixty-fours....
>
>
> > ---
> > drivers/hid/wacom_sys.c | 18 +++++-------------
> > 1 file changed, 5 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> > index 53bddb50aeba..69ccfdd51a6f 100644
> > --- a/drivers/hid/wacom_sys.c
> > +++ b/drivers/hid/wacom_sys.c
> > @@ -2718,14 +2718,12 @@ static int wacom_probe(struct hid_device *hdev,
> > wacom_wac->features = *((struct wacom_features *)id->driver_data);
> > features = &wacom_wac->features;
> >
> > - if (features->check_for_hid_type && features->hid_type != hdev->type) {
> > - error = -ENODEV;
> > - goto fail;
> > - }
> > + if (features->check_for_hid_type && features->hid_type != hdev->type)
> > + return -ENODEV;
> >
> > error = kfifo_alloc(&wacom_wac->pen_fifo, WACOM_PKGLEN_MAX, GFP_KERNEL);
> > if (error)
> > - goto fail;
> > + return error;
> >
> > wacom_wac->hid_data.inputmode = -1;
> > wacom_wac->mode_report = -1;
> > @@ -2743,12 +2741,12 @@ static int wacom_probe(struct hid_device *hdev,
> > error = hid_parse(hdev);
> > if (error) {
> > hid_err(hdev, "parse failed\n");
> > - goto fail;
> > + return error;
> > }
> >
> > error = wacom_parse_and_register(wacom, false);
> > if (error)
> > - goto fail;
> > + return error;
> >
> > if (hdev->bus == BUS_BLUETOOTH) {
> > error = device_create_file(&hdev->dev, &dev_attr_speed);
> > @@ -2759,10 +2757,6 @@ static int wacom_probe(struct hid_device *hdev,
> > }
> >
> > return 0;
> > -
> > -fail:
> > - hid_set_drvdata(hdev, NULL);
> > - return error;
> > }
> >
> > static void wacom_remove(struct hid_device *hdev)
> > @@ -2791,8 +2785,6 @@ static void wacom_remove(struct hid_device *hdev)
> > wacom_release_resources(wacom);
> >
> > kfifo_free(&wacom_wac->pen_fifo);
> > -
> > - hid_set_drvdata(hdev, NULL);
> > }
> >
> > #ifdef CONFIG_PM
> > --
> > 2.19.2
> >
^ permalink raw reply
* Re: [PATCH] HID: quirks: Disable runtime suspend on Microsoft Corp. Basic Optical Mouse v2.0
From: Alan Stern @ 2019-08-22 14:49 UTC (permalink / raw)
To: Kai-Heng Feng
Cc: Oliver Neukum, jikos, benjamin.tissoires, linux-input,
linux-kernel, linux-usb
In-Reply-To: <D6E31CB0-BC2B-4B52-AF18-4BE990D3FDA5@canonical.com>
On Thu, 22 Aug 2019, Kai-Heng Feng wrote:
> at 18:38, Oliver Neukum <oneukum@suse.com> wrote:
>
> > Am Donnerstag, den 22.08.2019, 18:04 +0800 schrieb Kai-Heng Feng:
> >> Hi Oliver,
> >>
> >> at 17:45, Oliver Neukum <oneukum@suse.com> wrote:
> >>
> >>> Am Donnerstag, den 22.08.2019, 17:17 +0800 schrieb Kai-Heng Feng:
> >>>> The optical sensor of the mouse gets turned off when it's runtime
> >>>> suspended, so moving the mouse can't wake the mouse up, despite that
> >>>> USB remote wakeup is successfully set.
> >>>>
> >>>> Introduce a new quirk to prevent the mouse from getting runtime
> >>>> suspended.
> >>>
> >>> Hi,
> >>>
> >>> I am afraid this is a bad approach in principle. The device
> >>> behaves according to spec.
> >>
> >> Can you please point out which spec it is? Is it USB 2.0 spec?
> >
> > Well, sort of. The USB spec merely states how to enter and exit
> > a suspended state and that device state must not be lost.
> > It does not tell you what a suspended device must be able to do.
>
> But shouldn’t remote wakeup signaling wakes the device up and let it exit
> suspend state?
> Or it’s okay to let the device be suspended when remote wakeup is needed
> but broken?
>
> >
> >>> And it behaves like most hardware.
> >>
> >> So seems like most hardware are broken.
> >> Maybe a more appropriate solution is to disable RPM for all USB mice.
> >
> > That is a decision a distro certainly can make. However, the kernel
> > does not, by default, call usb_enable_autosuspend() for HID devices
> > for this very reason. It is enabled by default only for hubs,
> > BT dongles and UVC cameras (and some minor devices)
> >
> > In other words, if on your system it is on, you need to look
> > at udev, not the kernel.
>
> So if a device is broken when “power/control” is flipped by user, we should
> deal it at userspace? That doesn’t sound right to me.
>
> >
> >>> If you do not want runtime PM for such devices, do not switch
> >>> it on.
> >>
> >> A device should work regardless of runtime PM status.
> >
> > Well, no. Runtime PM is a trade off. You lose something if you use
> > it. If it worked just as well as full power, you would never use
> > full power, would you?
>
> I am not asking the suspended state to work as full power, but to prevent a
> device enters suspend state because of broken remote wakeup.
>
> >
> > Whether the loss of functionality or performance is worth the energy
> > savings is a policy decision. Hence it belongs into udev.
> > Ideally the kernel would tell user space what will work in a
> > suspended state. Unfortunately HID does not provide support for that.
>
> I really don’t think “loss of functionally” belongs to policy decision. But
> that’s just my opinion.
>
> >
> > This is a deficiency of user space. The kernel has an ioctl()
> > to let user space tell it, whether a device is fully needed.
> > X does not use them.
>
> Ok, I’ll take a look at other device drivers that use it.
>
> >
> >>> The refcounting needs to be done correctly.
> >>
> >> Will do.
> >
> > Well, I am afraid your patch breaks it and if you do not break
> > it, the patch is reduced to nothing.
>
> Maybe just calling usb_autopm_put_interface() in usbhid_close() to balance
> the refcount?
>
> >
> >>> This patch does something that udev should do and in a
> >>> questionable manner.
> >>
> >> IMO if the device doesn’t support runtime suspend, then it needs to be
> >> disabled in kernel but not workaround in userspace.
> >
> > You switch it on from user space. Of course the kernel default
> > must be safe, as you said. It already is.
>
> I’d also like to hear maintainers' opinion on this issue.
I agree with Oliver. There is no formal requirement on what actions
should cause a mouse to generate a remote wakeup request. Some mice
will do it when they are moved and some mice won't.
If you don't like the way a particular mouse behaves then you should
not allow it to go into runtime suspend. By default, the kernel
prevents _all_ USB mice from being runtime suspended; the only way a
mouse can be suspended is if some userspace program tells the kernel to
allow it.
It might be a udev script which does this, or a powertop setting, or
something else. Regardless, what the kernel does is correct.
Furthermore, the kernel has to accomodate users who don't mind pressing
a mouse button to wake up their mice. For their sake, the kernel
should not forbid a mouse from ever going into runtime suspend merely
because it won't generate a wakeup request when it is moved.
Alan Stern
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox