* Re: [PATCH 1/2] input: touchscreen: ar1021_i2c: enable touch mode during open
From: Christian Gmeiner @ 2018-02-05 10:07 UTC (permalink / raw)
To: Martin Kepplinger; +Cc: Dmitry Torokhov, linux-input, LKML
In-Reply-To: <1493295756-17812-1-git-send-email-martin.kepplinger@ginzinger.com>
Hi all.
2017-04-27 14:22 GMT+02:00 Martin Kepplinger <martin.kepplinger@ginzinger.com>:
> The device could as well be in command mode, in which this driver cannot
> handle the device. When opening the device, let's make sure the device
> will be in the mode we expect it to be for this driver.
>
I run into issues caused by this change. It turns out that the device
is non-functional
after some warm-reboots and as a result I am not able to use xorg's
evdev driver.
So I have some questions about this change:
* Should we enable irq before calling i2c_master_send(..) as the chip raises an
irq if the command was processed?
* Would it be enough to send this command only once during driver
lifetime? I can
see that on my system open gets called 3 times during boot-up.
* What are the circumstances the touch device would be in an other state? In the
official kernel driver the userspace can send commands via sysfs.
Also the driver
does set the touch enable mode as this patch does.
> Signed-off-by: Martin Kepplinger <martin.kepplinger@ginzinger.com>
> ---
> drivers/input/touchscreen/ar1021_i2c.c | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
>
> diff --git a/drivers/input/touchscreen/ar1021_i2c.c b/drivers/input/touchscreen/ar1021_i2c.c
> index 1a94d8b..2a76231 100644
> --- a/drivers/input/touchscreen/ar1021_i2c.c
> +++ b/drivers/input/touchscreen/ar1021_i2c.c
> @@ -18,6 +18,12 @@
> #define AR1021_MAX_X 4095
> #define AR1021_MAX_Y 4095
>
> +#define AR1021_CMD 0x55
> +#define AR1021_TOUCH 0x80
> +
> +#define AR1021_CMD_ENABLE_TOUCH 0x12
> +#define AR1021_CMD_DISABLE_TOUCH 0x13
> +
> struct ar1021_i2c {
> struct i2c_client *client;
> struct input_dev *input;
> @@ -58,6 +64,15 @@ static int ar1021_i2c_open(struct input_dev *dev)
> {
> struct ar1021_i2c *ar1021 = input_get_drvdata(dev);
> struct i2c_client *client = ar1021->client;
> + int error;
> + u8 cmd_enable_touch[3] = {AR1021_CMD,
> + 0x01, /* number of bytes after this */
> + AR1021_CMD_ENABLE_TOUCH };
> +
> + error = i2c_master_send(ar1021->client, cmd_enable_touch,
> + sizeof(cmd_enable_touch));
> + if (error < 0)
> + return error;
>
> enable_irq(client->irq);
>
> --
> 2.1.4
>
--
greets
--
Christian Gmeiner, MSc
https://christian-gmeiner.info
^ permalink raw reply
* Re: ALPS touchpad ot correctly recognized: GlidePoint vs DualPoint
From: Juanito @ 2018-02-05 7:48 UTC (permalink / raw)
To: Pali Rohár
Cc: Masaki Ota, Dmitry Torokhov, linux-input@vger.kernel.org,
Paul Donohue
In-Reply-To: <20180204202118.fmyhcqmteyu6jnit@pali>
[-- Attachment #1.1: Type: text/plain, Size: 3545 bytes --]
On 02/04/2018 09:21 PM, Pali Rohár wrote:
> On Sunday 04 February 2018 20:39:06 Juanito wrote:
>> On 02/04/2018 07:16 PM, Pali Rohár wrote:
>>> On Monday 11 September 2017 13:26:30 Juanito wrote:
>>>> Hi Masaki Ota,
>>>>
>>>> Thanks!
>>>>
>>>> On 11.09.2017 04:38, Masaki Ota wrote:
>>>>> Hi, Juanito,
>>>>>
>>>>> In my information, ALPS Touchpad is used on Thinkpad E series and L series.
>>>>> I don't know the device that is ALPS Touchpad + other vendor TrackStick.
>>>>> But Lenovo might use ALPS Touchpad on such a combination.
>>>>>
>>>>
>>>> Well, that is probably my fault, as it is not the original touchpad that
>>>> was delievered with the laptop. I bought the touchpad (that included
>>>> the three buttons) separately because I didn't like the clickpad that
>>>> came with my laptop.
>>>
>>> Hi Juanito,
>>>
>>
>> Hi Pali,
>>
>>> if you are still want to play with your touchpad hardware, I have a good
>>> news for your.
>>>
>>
>> Yeah! I'd love to get it to work! It's just I've been "working" on some
>> other projects and my last kernel builds didn't work at all so I gave up
>> and never got back to it.
>>
>> Thank you very much for that!
>>
>>> It looks like that at least ALPS rushmore touchpads allow to receive RAW
>>> PS/2 packets from trackstick to host kernel, without modifying them by
>>> touchpad. Plus I was able to tell ALPS touchpad to start "mixing" those
>>> RAW trackstick PS/2 packets with native touchpad packets.
>>>
>>> On my configuration trackstick in RAW PS/2 mode by default talks with
>>> standard bare 3 byte PS/2 protocol and touchpad in 6 byte ALPS protocol.
>>> alps.c/psmouse.ko is already able to process and parse such mixed
>>> packets. And trackstick can be switched to some extended 4 byte
>>> protocol...
>>>
>>> All this happen when passthrough mode is enabled.
>>>
>>> From my understanding it seems that in normal mode, touchpad and
>>> trackstick communicate with that 4 byte protocol and touchpad converts
>>> it into 6 byte ALPS protocol and then send to kernel.
>>>
>>> On thinkpads trackstick communicate with TPPS/2 protcol and above 4
>>> byte. So in my opinion ALPS touchpad by default cannot understand it.
>>> But you should be able to enter passthrough mode and then you would
>>> receive that TPPS/2 in alps kernel code.
>>>
>>
>> I don't really understand what you mean. Do I "just" have to set it to
>> passthrough mode? How exactly can I do that?
>
> Look at function alps_passthrough_mode_v3(). And try to call it after
> touchpad is initialized.
>
Cool, I'll give this a try!
> You can also look which alps_command_mode_write_reg() functions are
> called for your touchpad and maybe try to figure out what those the
> registers can enabled/disable.
>
> On http://www.cirque.com/gen4-dev-resources you can find document named
> GP-AN- 130823 INTERFACING TO GEN4 OVER I2C (PDF) which contains
> some description of those registers (in section 7).
>
> Register C2C8, bit 0 has description "PS2AuxControl.CommandPassThruEnabled"
>
> If trackstick is not detected, it seems you can "force" enable it by
> setting bit 7 "PS2AuxControl.AuxDevicePresent" in same register.
>
>> Again, thank you very much!
>
> Have you tried to read from that register which Masaki Ota talked in
> previous emails?
>
I tried but I just didn't manage to get it to work, at all and gave up
(I really am a newbie in this whole kernel world). Maybe a fresh start
helps here.
Cheers,
Juanito
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: ALPS touchpad ot correctly recognized: GlidePoint vs DualPoint
From: Pali Rohár @ 2018-02-04 20:21 UTC (permalink / raw)
To: Juanito
Cc: Masaki Ota, Dmitry Torokhov, linux-input@vger.kernel.org,
Paul Donohue
In-Reply-To: <f291e382-b90e-baba-fb3c-bdebc991a21d@posteo.net>
[-- Attachment #1: Type: text/plain, Size: 3279 bytes --]
On Sunday 04 February 2018 20:39:06 Juanito wrote:
> On 02/04/2018 07:16 PM, Pali Rohár wrote:
> > On Monday 11 September 2017 13:26:30 Juanito wrote:
> >> Hi Masaki Ota,
> >>
> >> Thanks!
> >>
> >> On 11.09.2017 04:38, Masaki Ota wrote:
> >>> Hi, Juanito,
> >>>
> >>> In my information, ALPS Touchpad is used on Thinkpad E series and L series.
> >>> I don't know the device that is ALPS Touchpad + other vendor TrackStick.
> >>> But Lenovo might use ALPS Touchpad on such a combination.
> >>>
> >>
> >> Well, that is probably my fault, as it is not the original touchpad that
> >> was delievered with the laptop. I bought the touchpad (that included
> >> the three buttons) separately because I didn't like the clickpad that
> >> came with my laptop.
> >
> > Hi Juanito,
> >
>
> Hi Pali,
>
> > if you are still want to play with your touchpad hardware, I have a good
> > news for your.
> >
>
> Yeah! I'd love to get it to work! It's just I've been "working" on some
> other projects and my last kernel builds didn't work at all so I gave up
> and never got back to it.
>
> Thank you very much for that!
>
> > It looks like that at least ALPS rushmore touchpads allow to receive RAW
> > PS/2 packets from trackstick to host kernel, without modifying them by
> > touchpad. Plus I was able to tell ALPS touchpad to start "mixing" those
> > RAW trackstick PS/2 packets with native touchpad packets.
> >
> > On my configuration trackstick in RAW PS/2 mode by default talks with
> > standard bare 3 byte PS/2 protocol and touchpad in 6 byte ALPS protocol.
> > alps.c/psmouse.ko is already able to process and parse such mixed
> > packets. And trackstick can be switched to some extended 4 byte
> > protocol...
> >
> > All this happen when passthrough mode is enabled.
> >
> > From my understanding it seems that in normal mode, touchpad and
> > trackstick communicate with that 4 byte protocol and touchpad converts
> > it into 6 byte ALPS protocol and then send to kernel.
> >
> > On thinkpads trackstick communicate with TPPS/2 protcol and above 4
> > byte. So in my opinion ALPS touchpad by default cannot understand it.
> > But you should be able to enter passthrough mode and then you would
> > receive that TPPS/2 in alps kernel code.
> >
>
> I don't really understand what you mean. Do I "just" have to set it to
> passthrough mode? How exactly can I do that?
Look at function alps_passthrough_mode_v3(). And try to call it after
touchpad is initialized.
You can also look which alps_command_mode_write_reg() functions are
called for your touchpad and maybe try to figure out what those
registers can enabled/disable.
On http://www.cirque.com/gen4-dev-resources you can find document named
GP-AN- 130823 INTERFACING TO GEN4 OVER I2C (PDF) which contains
some description of those registers (in section 7).
Register C2C8, bit 0 has description "PS2AuxControl.CommandPassThruEnabled"
If trackstick is not detected, it seems you can "force" enable it by
setting bit 7 "PS2AuxControl.AuxDevicePresent" in same register.
> Again, thank you very much!
Have you tried to read from that register which Masaki Ota talked in
previous emails?
--
Pali Rohár
pali.rohar@gmail.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]
^ permalink raw reply
* Re: Dell docking station & Dell Embedded Controller & PS/2 devices
From: Pali Rohár @ 2018-02-04 20:07 UTC (permalink / raw)
To: Mario.Limonciello; +Cc: dmitry.torokhov, linux-input, linux-kernel
In-Reply-To: <ffe4f35a6570401083d7032e261daa20@ausx13mpc120.AMER.DELL.COM>
[-- Attachment #1: Type: text/plain, Size: 6893 bytes --]
On Thursday 01 February 2018 21:23:40 Mario.Limonciello@dell.com wrote:
> > -----Original Message-----
> > From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> > Sent: Thursday, February 1, 2018 3:14 PM
> > To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> > Cc: Pali Rohár <pali.rohar@gmail.com>; linux-input@vger.kernel.org; lkml <linux-
> > kernel@vger.kernel.org>
> > Subject: Re: Dell docking station & Dell Embedded Controller & PS/2 devices
> >
> > On Thu, Feb 1, 2018 at 11:35 AM, <Mario.Limonciello@dell.com> wrote:
> > >
> > >
> > >> -----Original Message-----
> > >> From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> > >> Sent: Thursday, February 1, 2018 1:30 PM
> > >> To: Pali Rohár <pali.rohar@gmail.com>
> > >> Cc: Limonciello, Mario <Mario_Limonciello@Dell.com>; linux-
> > >> input@vger.kernel.org; linux-kernel@vger.kernel.org
> > >> Subject: Re: Dell docking station & Dell Embedded Controller & PS/2 devices
> > >>
> > >> Hi Pali,
> > >>
> > >> On Wed, Jan 24, 2018 at 11:41:21AM +0100, Pali Rohár wrote:
> > >> > Hi Dmitry!
> > >> >
> > >> > I'm observing a problem with internal touchpad (handled by psmouse.ko)
> > >> > on Dell laptops connected to Dell E docking station. When I connect
> > >> > external PS/2 keyboard to docking station then internal laptop touchpad
> > >> > switch from multitouch absolute mode to relative bare PS/2 mode.
> > >> >
> > >> > And because ALPS driver in psmouse.ko is capable to process interleaved
> > >> > bare 3-byte PS/2 packets with 6-byte ALPS packets (which handles
> > >> > trackstick data on some ALPS models), ALPS driver does not show any
> > >> > message about this "downgrade" from multitouch to bare mode. And
> > >> > continue working in bare mode.
> > >> >
> > >> > When I rmmod psmouse and modprobe it again, then touchpad switch back to
> > >> > multitouch mode.
> > >> >
> > >> > Mario told me that Dell Embedded Controller, which handle internal
> > >> > keyboard, internal touchpad and external PS/2 keyboard, automatically
> > >> > send RESET command to *all* those devices when external PS/2 keyboard is
> > >> > connected. Therefore this is reason why touchpad downgrade to to bare
> > >> > mode. And according to Mario, host system should issue vendor specific
> > >> > PS/2 commands to re-initialize all PS/2 devices when this situation
> > >> > happen. Mario also told me that Windows is doing this action.
> > >>
> > >> Yeah, I remember fun with Inspiron 8100 - when you dock it it woudl
> > >> silently switch Synaptics touchpad into standard mode and it would not
> > >> come back as Synaptics until you disconnect. And there was no
> > >> notification to the kernel as far as I could tell.
> > >>
> > >> It could be that we need to monitor dock events and then kick reconnect
> > >> of serio port, either from userspace via udev (I think that would be
> > >> preferred), or in kernel.
> > >>
> > >> >
> > >> > Every time when I connect external PS/2 keyboard to dock I see this
> > >> > message in dmesg:
> > >> >
> > >> > Spurious ACK... Some program might be trying to access hardware directly.
> > >> >
> > >> > I see it also every time when I dock laptop into docking station (to
> > >> > which is keyboard already connected). And it happens also when I connect
> > >> > external PS/2 mouse to dock.
> > >> >
> > >> > Dmitry, how to handle this situation to re-initialize psmouse.ko when
> > >> > external PS/2 device is connected to Dell E docking station? According
> > >> > to Mario, this is how Dell Embedded Controller is designed and suppose
> > >> > how OS should work with it.
> > >> >
> > >> > Manually rmmoding and modprobing for every docking/undocking laptop is
> > >> > not ideal solution.
> > >> >
> > >> > Could it be possible to use that Spurious ATKBD_RET_ACK from atkbd.c be
> > >> > handled on Dell systems (probably via DMI) as an event to reset and
> > >> > reinitialize all PS/2 devices?
> > >>
> > >> So we need to figure out what exactly we are getting from the docking
> > >> station in this case. We do try to handle the new device 0xaa 0x00
> > >> announcements:
> > >
> > > Docking station itself won't have sent any events. The PS2 ports in it
> > > are electrically wired to the external PS2 pins in the docking adapter on
> > > the system.
> > >
> > > System EC is what will be sending any PS2 events.
> >
> > OK, it does not matter from the input layer POW, if they are PS/2
> > events they should be coming from i8042.
Dell EC sends reset to touchpad, not to kernel.
> > >
> > >>
> > >> /* Check if this is a new device announcement (0xAA 0x00) */
> > >> if (unlikely(psmouse->packet[0] == PSMOUSE_RET_BAT && psmouse-
> > >> >pktcnt <= 2)) {
> > >> if (psmouse->pktcnt == 1) {
> > >> psmouse->last = jiffies;
> > >> goto out;
> > >> }
> > >>
> > >> if (psmouse->packet[1] == PSMOUSE_RET_ID ||
> > >> (psmouse->protocol->type == PSMOUSE_HGPK &&
> > >> psmouse->packet[1] == PSMOUSE_RET_BAT)) {
> > >> __psmouse_set_state(psmouse, PSMOUSE_IGNORE);
> > >> serio_reconnect(serio);
> > >> goto out;
> > >> }
> > >>
> > >> ...
> > >>
> > >> I am not sure where the "spurious ACK comes from". Can you enable i8042
> > >> debug before trying to dock and capture the data stream from the mouse?
> > >>
> > >
> > > The system EC has reset PS2 devices (but AFAIK kernel won't have seen this
> > > communication).
> >
> > So what kind of communication does EC send when docking? It sounds it
> > is completely out of band from PS/2 connection.
>
> Yes it's out of band from PS/2 connection OS sees. However as Pali has
> mentioned this issue will happen even if already docked but you plug in
> PS/2 keyboard.
>
> It will probably be clearer to you what OS has seen from this when Pali
> shares the captured data stream.
Currently I'm not near my docking station, so cannot do testing.
But in logs I can dig these lines which appeared every time after
putting notebook into dock (which has PS/2 keyboard connected):
kernel: [20127.033965] atkbd serio0: Spurious ACK on isa0060/serio0. Some program might be trying to access hardware directly.
kernel: [20127.076030] acpi PNP0401:00: Already enumerated
kernel: [20127.083088] acpi PNP0501:00: Still not present
kernel: [20127.087042] acpi device:41: Cannot transition to power state D3hot for parent in (unknown)
And sometimes after 1-5s I see Spurious event again:
kernel: [20128.325569] atkbd serio0: Spurious ACK on isa0060/serio0. Some program might be trying to access hardware directly.
--
Pali Rohár
pali.rohar@gmail.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]
^ permalink raw reply
* Re: ALPS touchpad ot correctly recognized: GlidePoint vs DualPoint
From: Pali Rohár @ 2018-02-04 18:16 UTC (permalink / raw)
To: Juanito
Cc: Masaki Ota, Dmitry Torokhov, linux-input@vger.kernel.org,
Paul Donohue
In-Reply-To: <7d5d4e88-b6d2-ce7b-a529-18f2b131fa7f@posteo.net>
[-- Attachment #1: Type: text/plain, Size: 5706 bytes --]
On Monday 11 September 2017 13:26:30 Juanito wrote:
> Hi Masaki Ota,
>
> Thanks!
>
> On 11.09.2017 04:38, Masaki Ota wrote:
> > Hi, Juanito,
> >
> > In my information, ALPS Touchpad is used on Thinkpad E series and L series.
> > I don't know the device that is ALPS Touchpad + other vendor TrackStick.
> > But Lenovo might use ALPS Touchpad on such a combination.
> >
>
> Well, that is probably my fault, as it is not the original touchpad that
> was delievered with the laptop. I bought the touchpad (that included
> the three buttons) separately because I didn't like the clickpad that
> came with my laptop.
Hi Juanito,
if you are still want to play with your touchpad hardware, I have a good
news for your.
It looks like that at least ALPS rushmore touchpads allow to receive RAW
PS/2 packets from trackstick to host kernel, without modifying them by
touchpad. Plus I was able to tell ALPS touchpad to start "mixing" those
RAW trackstick PS/2 packets with native touchpad packets.
On my configuration trackstick in RAW PS/2 mode by default talks with
standard bare 3 byte PS/2 protocol and touchpad in 6 byte ALPS protocol.
alps.c/psmouse.ko is already able to process and parse such mixed
packets. And trackstick can be switched to some extended 4 byte
protocol...
All this happen when passthrough mode is enabled.
From my understanding it seems that in normal mode, touchpad and
trackstick communicate with that 4 byte protocol and touchpad converts
it into 6 byte ALPS protocol and then send to kernel.
On thinkpads trackstick communicate with TPPS/2 protcol and above 4
byte. So in my opinion ALPS touchpad by default cannot understand it.
But you should be able to enter passthrough mode and then you would
receive that TPPS/2 in alps kernel code.
> > And I have found that the V7 protocol device has a special ID for Lenovo.
> > ALPS assigns the value that is the model type to 0xC399 address.
> >
> > Could you check 0xC399 address value such like a below code?
> > reg_val = alps_command_mode_read_reg(psmouse, 0xC399);
> >
> > If the value is 0x1 or 0x2 or 0x6 or 0x10 or 0xF, it has a Stick buttons.
> >
>
> I will check this as soon as I can (I don't have access to the laptop
> right now).
>
> > Best Regards,
> > Masaki Ota
> > -----Original Message-----
> > From: Juanito [mailto:juam+kernel@posteo.net]
> > Sent: Sunday, September 10, 2017 3:22 AM
> > To: Dmitry Torokhov <dmitry.torokhov@gmail.com>; Pali Rohár <pali.rohar@gmail.com>
> > Cc: 太田 真喜 Masaki Ota <masaki.ota@jp.alps.com>; linux-input@vger.kernel.org; Paul Donohue <linux-kernel@paulsd.com>
> > Subject: Re: ALPS touchpad ot correctly recognized: GlidePoint vs DualPoint
> >
> > On 09/09/2017 08:12 PM, Dmitry Torokhov wrote:
> >> On Sat, Sep 09, 2017 at 10:36:06AM +0200, Pali Rohár wrote:
> >>> On Saturday 09 September 2017 10:12:42 Juanito wrote:
> >>>> Hello,
> >>>>
> >>>> On 09/08/2017 08:48 AM, Pali Rohár wrote:
> >>>>> On Friday 08 September 2017 07:00:34 Juanito wrote:
> >>>>>>> ThinkPad with ALPS? Should not be it Synaptic? Maybe
> >>>>>>> miss-detection?
> >>>>>>
> >>>>>> Sorry, I forgot to mention this. The ThinkPad came with a clickpad
> >>>>>> I **really** disliked, so I bought this on the Internet.
> >>>>>
> >>>>> So, here is a problem. ThinkPads works with Synaptic touchpads, not
> >>>>> with ALPS.
> >>>>
> >>>> There definitely seems to be a problem here :)
> >>>>
> >>>> Do you mean that the alps code might not be detecting the trackpoint
> >>>> because probably the red thingie works with synaptics?
> >>>>
> >>>> So could this be the situation:
> >>>> ALPS driver: Hey touchpad! Do you have a trackstick?
> >>>> ALPS touchpad: No, I don't.
> >>>> AD: Ok! (and thinks 'I am going to have to ignore all trackstick
> >>>> packets that might arrive')
> >>>>
> >>>> So the driver understands that there can't possibly be any buttons
> >>>> because there is no trackstick?
> >>>
> >>> IIRC ALPS touchpad hardware itself does not work with non-ALPS
> >>> trackpoint.
> >>>
> >>> Masaki, any comments?
> >>>
> >>>>>> If by trackstick you mean the red thingie, it is **not** working.
> >>>>>
> >>>>> Ok. And it is working with your patch?
> >>>>
> >>>> No, it isn't :(
> >>>
> >>> I expected... You just created franken-hardware.
> >>>
> >>> Looks like with your hack patch it is possible to make buttons
> >>> working, but I suspect that trackstick would.
> >>>
> >>> Maybe Masaki can provide more information about this fact if we can
> >>> do anything.
> >>>
> >>> Dmitry, what you as maintainer going to do with this problem?
> >>
> >> It really depends on Ota-san response. If ALPS needs a special version
> >> of firmware flashed for proper trackstick identification, then I guess
> >> Juanito will have to carry a local patch. I do not think we want to
> >> complicate the driver any further for supporting such after-market
> >> modification.
> >>
> >
> > If this only happens in this after-market (or franken-hardware...
> > hahaha), I'd totally understand if you didn't want to patch it upstream, although it would be nice for me :)
> >
> >> Does booting with psmouse.proto=imps makes trackstick work?
> >>
> >
> > I am afraid I can't test it now as I don't have access to the laptop. I won't be able to test until two weeks. Will definitely do as soon as I can.
> >
> >> Thanks.
> >>
> >
> > Thank **you** (Dmitry and everybody esle) for taking the time to look at this.
> >
> > Juanito
> >
>
>
> Thank you very much!
>
> Best regards,
> Juanito
>
--
Pali Rohár
pali.rohar@gmail.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]
^ permalink raw reply
* ALPS Trackpoint & pressure
From: Pali Rohár @ 2018-02-04 15:08 UTC (permalink / raw)
To: Dmitry Torokhov, Masaki Ota; +Cc: linux-input, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 605 bytes --]
Hi! Now playing again with trackpoint connected to ALPS rushmore
touchpad and I'm seeking a nice feature. Via ALPS PS/2 protocol it
reports pressure of trackpoint. Parser for it is already implemented in
alps.c and value is assigned to variable "z". When I just move
trackpoint z is zero, when I push trackpoint while moving, then z is
number higher, maximally 32. Variable "z" is set, but unused.
Do we have some input interface which can be used to report this
pressure of trackpoint to userspace? I can use this feature e.g. as
additional button...
--
Pali Rohár
pali.rohar@gmail.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]
^ permalink raw reply
* Cirque/ALPS touchpad documentation
From: Pali Rohár @ 2018-02-04 13:58 UTC (permalink / raw)
To: Dmitry Torokhov, Masaki Ota, Jiri Kosina; +Cc: linux-input, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1182 bytes --]
Hi!
Now I find on cirque website that there is some documentation for
GlidePoint/ALPS touchpads/trackpads. The most interesting part is
"GP-AN- 130823 INTERFACING TO GEN4 OVER I2C (PDF)" on website:
http://www.cirque.com/gen4-dev-resources
There is basic I2C and HID protocol information with Memory Table
registers in section 7.
Looking at mouse/alps.c and hid/hid-alps.c source code, it seems that
those 0xC2C0 and other "random" numbers have real meaning. And it should
be possible to set more configuration options for ALPS touchpads. There
are some coordinate algorithms or configuration for sample rate, etc.
PS/2 protocol description is completely missing in that documentation,
but e.g. addresses passed to PS/2 function alps_command_mode_read_reg()
perfectly matches addresses in above memory table (e.g. AuxDevicePresent
= enable/disable trackstick, etc...).
I have not know that there is released such documentation for ALPS
devices, so I'm sending this email... maybe it can be useful for you
too.
On that page is also documentation with source code examples how to
update firmware in touchpads.
--
Pali Rohár
pali.rohar@gmail.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]
^ permalink raw reply
* Re: [PATCH] hid-sensor-accel-3d: Delete an error message for a failed memory allocation in hid_accel_3d_probe()
From: Jonathan Cameron @ 2018-02-04 11:23 UTC (permalink / raw)
To: SF Markus Elfring
Cc: linux-iio, linux-input, Hartmut Knaack, Jiri Kosina,
Lars-Peter Clausen, Peter Meerwald-Stadler, Srinivas Pandruvada,
LKML, kernel-janitors
In-Reply-To: <0406765c-bdd1-1a82-cf66-1c248063ae4f@users.sourceforge.net>
On Wed, 31 Jan 2018 22:26:14 +0100
SF Markus Elfring <elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 31 Jan 2018 22:20:56 +0100
>
> Omit an extra message for a memory allocation failure in this function.
>
> This issue was detected by using the Coccinelle software.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
Marcus,
If making changes like this I would suggest only sending one until
you have have a response from the relevant maintainer.
It would save you time as often these sorts of changes are
a matter of personal taste and weighing up of costs vs gains
- hence it is not obvious that they will be accepted.
Jonathan
> ---
> drivers/iio/accel/hid-sensor-accel-3d.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/iio/accel/hid-sensor-accel-3d.c b/drivers/iio/accel/hid-sensor-accel-3d.c
> index c066a3bdbff7..3d0acde40285 100644
> --- a/drivers/iio/accel/hid-sensor-accel-3d.c
> +++ b/drivers/iio/accel/hid-sensor-accel-3d.c
> @@ -383,11 +383,9 @@ static int hid_accel_3d_probe(struct platform_device *pdev)
> return ret;
> }
> indio_dev->channels = kmemdup(channel_spec, channel_size, GFP_KERNEL);
> -
> - if (!indio_dev->channels) {
> - dev_err(&pdev->dev, "failed to duplicate channels\n");
> + if (!indio_dev->channels)
> return -ENOMEM;
> - }
> +
> ret = accel_3d_parse_report(pdev, hsdev,
> (struct iio_chan_spec *)indio_dev->channels,
> hsdev->usage, accel_state);
^ permalink raw reply
* Re: [PATCH] iio/orientation: Delete an error message for a failed memory allocation in two functions
From: Jonathan Cameron @ 2018-02-04 11:01 UTC (permalink / raw)
To: SF Markus Elfring
Cc: linux-iio, linux-input, Hartmut Knaack, Jiri Kosina,
Lars-Peter Clausen, Peter Meerwald-Stadler, Srinivas Pandruvada,
LKML, kernel-janitors
In-Reply-To: <ebf6c923-d641-8bf8-5544-630a5ee39980@users.sourceforge.net>
On Mon, 29 Jan 2018 22:26:15 +0100
SF Markus Elfring <elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Mon, 29 Jan 2018 22:20:07 +0100
>
> Omit an extra message for a memory allocation failure in these functions.
>
> This issue was detected by using the Coccinelle software.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
Same response as previous patch - it might not be adding much information,
but it is adding some so we should keep it.
Jonathan
> ---
> drivers/iio/orientation/hid-sensor-incl-3d.c | 4 +---
> drivers/iio/orientation/hid-sensor-rotation.c | 4 +---
> 2 files changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/iio/orientation/hid-sensor-incl-3d.c b/drivers/iio/orientation/hid-sensor-incl-3d.c
> index 1e5451d1ff88..742c0e0eb4cf 100644
> --- a/drivers/iio/orientation/hid-sensor-incl-3d.c
> +++ b/drivers/iio/orientation/hid-sensor-incl-3d.c
> @@ -336,10 +336,8 @@ static int hid_incl_3d_probe(struct platform_device *pdev)
>
> indio_dev->channels = kmemdup(incl_3d_channels,
> sizeof(incl_3d_channels), GFP_KERNEL);
> - if (!indio_dev->channels) {
> - dev_err(&pdev->dev, "failed to duplicate channels\n");
> + if (!indio_dev->channels)
> return -ENOMEM;
> - }
>
> ret = incl_3d_parse_report(pdev, hsdev,
> (struct iio_chan_spec *)indio_dev->channels,
> diff --git a/drivers/iio/orientation/hid-sensor-rotation.c b/drivers/iio/orientation/hid-sensor-rotation.c
> index a69db2002414..cbf5e406b1ee 100644
> --- a/drivers/iio/orientation/hid-sensor-rotation.c
> +++ b/drivers/iio/orientation/hid-sensor-rotation.c
> @@ -277,10 +277,8 @@ static int hid_dev_rot_probe(struct platform_device *pdev)
> indio_dev->channels = devm_kmemdup(&pdev->dev, dev_rot_channels,
> sizeof(dev_rot_channels),
> GFP_KERNEL);
> - if (!indio_dev->channels) {
> - dev_err(&pdev->dev, "failed to duplicate channels\n");
> + if (!indio_dev->channels)
> return -ENOMEM;
> - }
>
> ret = dev_rot_parse_report(pdev, hsdev,
> (struct iio_chan_spec *)indio_dev->channels,
^ permalink raw reply
* Re: [PATCH] iio/pressure: Delete an error message for a failed memory allocation in hid_press_probe()
From: Jonathan Cameron @ 2018-02-04 10:58 UTC (permalink / raw)
To: SF Markus Elfring
Cc: linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-input-u79uwXL29TY76Z2rM5mHXA, Archana Patni, Hartmut Knaack,
Jiri Kosina, Lars-Peter Clausen, Peter Meerwald-Stadler,
Srinivas Pandruvada, LKML, kernel-janitors-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <e7dfe444-616a-e156-31d5-8ed626434d1f-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
On Mon, 29 Jan 2018 22:04:11 +0100
SF Markus Elfring <elfring-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org> wrote:
> From: Markus Elfring <elfring-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
> Date: Mon, 29 Jan 2018 21:55:08 +0100
>
> Omit an extra message for a memory allocation failure in this function.
>
> This issue was detected by using the Coccinelle software.
>
> Signed-off-by: Markus Elfring <elfring-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
Whilst the error message doesn't add a lot, it does add some information
over a simple memory copy failed error.
Hence I think we should keep it.
Jonathan
> ---
> drivers/iio/pressure/hid-sensor-press.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/iio/pressure/hid-sensor-press.c b/drivers/iio/pressure/hid-sensor-press.c
> index 4c437918f1d2..9de7590d50b0 100644
> --- a/drivers/iio/pressure/hid-sensor-press.c
> +++ b/drivers/iio/pressure/hid-sensor-press.c
> @@ -280,10 +280,8 @@ static int hid_press_probe(struct platform_device *pdev)
>
> indio_dev->channels = kmemdup(press_channels, sizeof(press_channels),
> GFP_KERNEL);
> - if (!indio_dev->channels) {
> - dev_err(&pdev->dev, "failed to duplicate channels\n");
> + if (!indio_dev->channels)
> return -ENOMEM;
> - }
>
> ret = press_parse_report(pdev, hsdev,
> (struct iio_chan_spec *)indio_dev->channels,
^ permalink raw reply
* [PATCH v3] Input: matrix_keypad - fix keypad does not response
From: Zhang Bo @ 2018-02-04 0:19 UTC (permalink / raw)
To: dmitry.torokhov
Cc: zbsdta, DRivshin, robh, linux-input, linux-kernel, zhang.bo19
If matrix_keypad_stop() is calling and the keypad interrupt is triggered,
disable_row_irqs() is called by both matrix_keypad_interrupt() and
matrix_keypad_stop() at the same time. then disable_row_irqs() is called
twice, and the device enter suspend state before keypad->work is executed.
At this condition the device will start keypad and enable irq once after
resume. and then irqs are disabled yet because irqs are disabled twice and
only enable once.
Take lock around keypad->stopped to ensure irqs operation is in atomic
operation.
Signed-off-by: Zhang Bo <zbsdta@126.com>
---
Changes in v3:
- delete no needed lock protection, only add lock in matrix_keypad_stop().
Changes in v2:
- Change commit message and full name in the signed-off-by tag.
drivers/input/keyboard/matrix_keypad.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/input/keyboard/matrix_keypad.c b/drivers/input/keyboard/matrix_keypad.c
index 1f316d66e6f7..41614c185918 100644
--- a/drivers/input/keyboard/matrix_keypad.c
+++ b/drivers/input/keyboard/matrix_keypad.c
@@ -218,8 +218,10 @@ static void matrix_keypad_stop(struct input_dev *dev)
{
struct matrix_keypad *keypad = input_get_drvdata(dev);
+ spin_lock_irq(&keypad->lock);
keypad->stopped = true;
- mb();
+ spin_unlock_irq(&keypad->lock);
+
flush_work(&keypad->work.work);
/*
* matrix_keypad_scan() will leave IRQs enabled;
--
2.14.3
^ permalink raw reply related
* Re: [PATCH v2] Input: matrix_keypad - fix keypad does not response
From: Zhang Bo @ 2018-02-03 23:57 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: robh, DRivshin, linux-input, linux-kernel, zhang.bo19,
andy.shevchenko
In-Reply-To: <20180203193504.pnn7qx6zymu5qk5n@dtor-ws>
On Sat, 2018-02-03 at 11:35 -0800, Dmitry Torokhov wrote:
Hi Dmitry Torokhov
> > If matrix_keypad_stop() is calling and the keypad interrupt is
> > triggered,
> > disable_row_irqs() is called by both matrix_keypad_interrupt() and
> > matrix_keypad_stop() at the same time. then disable_row_irqs() is
> > called
> > twice, and the device enter suspend state before keypad->work is
> > executed.
> > At this condition the device will start keypad and enable irq once
> > after
> > resume. and then irqs are disabled yet because irqs are disabled
> > twice and
> > only enable once.
> >
> > Take lock around keypad->stopped and queue delayed work in
> > matrix_keypad_start() and matrix_keypad_stop() to ensure irqs
> > operation and
> > scheduling scan work are in atomic operation.
> >
> > Signed-off-by: Zhang Bo <zbsdta@126.com>
> > ---
> > Changes in v2:
> > - Change commit message and full name in the signed-off-by tag.
> >
> > drivers/input/keyboard/matrix_keypad.c | 18 ++++++++++++------
> > 1 file changed, 12 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/input/keyboard/matrix_keypad.c
> > b/drivers/input/keyboard/matrix_keypad.c
> > index 1f316d66e6f7..13fe51824637 100644
> > --- a/drivers/input/keyboard/matrix_keypad.c
> > +++ b/drivers/input/keyboard/matrix_keypad.c
> > @@ -169,7 +169,8 @@ static void matrix_keypad_scan(struct
> > work_struct *work)
> > /* Enable IRQs again */
> > spin_lock_irq(&keypad->lock);
> > keypad->scan_pending = false;
> > - enable_row_irqs(keypad);
> > + if (keypad->stopped == false)
> > + enable_row_irqs(keypad);
> > spin_unlock_irq(&keypad->lock);
> > }
> >
> > @@ -202,14 +203,16 @@ static int matrix_keypad_start(struct
> > input_dev *dev)
> > {
> > struct matrix_keypad *keypad = input_get_drvdata(dev);
> >
> > + spin_lock_irq(&keypad->lock);
> > keypad->stopped = false;
> > - mb();
> >
> > /*
> > * Schedule an immediate key scan to capture current key
> > state;
> > * columns will be activated and IRQs be enabled after the
> > scan.
> > */
> > - schedule_delayed_work(&keypad->work, 0);
> > + if (keypad->scan_pending == false)
>
> How can we have the pending scan if the keypad was disabled.
>
> > + schedule_delayed_work(&keypad->work, 0);
> > + spin_unlock_irq(&keypad->lock);
>
> I do not think the change to matrix_keypad_start() is needed. If
> device
> is quiesced we do not have issue of ISR racing with us here.
>
you are right, irqs are disabled and worker is finished in
matrix_keypad_stop(), so
there is no pending scaning and the if condition here and lock are not
needed.
> >
> > return 0;
> > }
> > @@ -218,14 +221,17 @@ static void matrix_keypad_stop(struct
> > input_dev *dev)
> > {
> > struct matrix_keypad *keypad = input_get_drvdata(dev);
> >
> > + spin_lock_irq(&keypad->lock);
> > keypad->stopped = true;
> > - mb();
> > - flush_work(&keypad->work.work);
> > /*
> > * matrix_keypad_scan() will leave IRQs enabled;
> > * we should disable them now.
> > */
> > - disable_row_irqs(keypad);
> > + if (keypad->scan_pending == false)
> > + disable_row_irqs(keypad);
> > + spin_unlock_irq(&keypad->lock);
> > +
> > + flush_work(&keypad->work.work);
>
> This is wrong, you should not have moved the flush_work() here. The
> logic is as follows:
>
> - set the "stopped" flag
> - ensure that ISR has completed
> - ensure that work item has finished (by doing flush_work()) - this
> will
> make sure that interrupts are enabled (either ISR noticed "stopped
> flag" and did not touch them, or ISR scheduled work and work item
> re-enabled them)
> - finally disable IRQs
>
> Your change breaks this.
>
> As far as I can see, the only change that is needed is this at the
> beginning of matrix_keypad_stop():
>
> spin_lock_irq(&keypad->lock);
> keypad->stopped = true;
> spin_unlock_irq(&keypad->lock);
>
> Thanks.
>
yes, only protecting keypad->stopped ensures irqs are disabled only
once.
^ permalink raw reply
* [PATCH] HID: magicmouse: Delete an error message for a failed memory allocation in magicmouse_probe()
From: SF Markus Elfring @ 2018-02-03 21:20 UTC (permalink / raw)
To: linux-input, Benjamin Tissoires, Jiri Kosina; +Cc: LKML, kernel-janitors
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 3 Feb 2018 22:15:05 +0100
Omit an extra message for a memory allocation failure in this function.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
drivers/hid/hid-magicmouse.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
index 42ed887ba0be..8d17a812c5ba 100644
--- a/drivers/hid/hid-magicmouse.c
+++ b/drivers/hid/hid-magicmouse.c
@@ -501,10 +501,8 @@ static int magicmouse_probe(struct hid_device *hdev,
int ret;
msc = devm_kzalloc(&hdev->dev, sizeof(*msc), GFP_KERNEL);
- if (msc == NULL) {
- hid_err(hdev, "can't alloc magicmouse descriptor\n");
+ if (!msc)
return -ENOMEM;
- }
msc->scroll_accel = SCROLL_ACCEL_DEFAULT;
--
2.16.1
^ permalink raw reply related
* [PATCH 2/2] HID: ntrig: Improve a size determination in ntrig_probe()
From: SF Markus Elfring @ 2018-02-03 21:00 UTC (permalink / raw)
To: linux-input, Benjamin Tissoires, Jiri Kosina; +Cc: LKML, kernel-janitors
In-Reply-To: <86d02ef2-0ac2-5462-0ff0-a0cd87c3d1f7@users.sourceforge.net>
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 3 Feb 2018 21:48:39 +0100
Replace the specification of a data structure by a pointer dereference
as the parameter for the operator "sizeof" to make the corresponding size
determination a bit safer according to the Linux coding style convention.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
drivers/hid/hid-ntrig.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/hid/hid-ntrig.c b/drivers/hid/hid-ntrig.c
index 2a8dafaea2f0..c6878a6a147c 100644
--- a/drivers/hid/hid-ntrig.c
+++ b/drivers/hid/hid-ntrig.c
@@ -904,7 +904,7 @@ static int ntrig_probe(struct hid_device *hdev, const struct hid_device_id *id)
hdev->quirks |= HID_QUIRK_MULTI_INPUT
| HID_QUIRK_NO_INIT_REPORTS;
- nd = kmalloc(sizeof(struct ntrig_data), GFP_KERNEL);
+ nd = kmalloc(sizeof(*nd), GFP_KERNEL);
if (!nd)
return -ENOMEM;
--
2.16.1
^ permalink raw reply related
* [PATCH 1/2] HID: ntrig: Delete an error message for a failed memory allocation in ntrig_probe()
From: SF Markus Elfring @ 2018-02-03 20:58 UTC (permalink / raw)
To: linux-input, Benjamin Tissoires, Jiri Kosina; +Cc: LKML, kernel-janitors
In-Reply-To: <86d02ef2-0ac2-5462-0ff0-a0cd87c3d1f7@users.sourceforge.net>
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 3 Feb 2018 21:46:20 +0100
Omit an extra message for a memory allocation failure in this function.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
drivers/hid/hid-ntrig.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/hid/hid-ntrig.c b/drivers/hid/hid-ntrig.c
index 3d121d8ee980..2a8dafaea2f0 100644
--- a/drivers/hid/hid-ntrig.c
+++ b/drivers/hid/hid-ntrig.c
@@ -905,10 +905,8 @@ static int ntrig_probe(struct hid_device *hdev, const struct hid_device_id *id)
| HID_QUIRK_NO_INIT_REPORTS;
nd = kmalloc(sizeof(struct ntrig_data), GFP_KERNEL);
- if (!nd) {
- hid_err(hdev, "cannot allocate N-Trig data\n");
+ if (!nd)
return -ENOMEM;
- }
nd->reading_mt = 0;
nd->min_width = 0;
--
2.16.1
^ permalink raw reply related
* [PATCH 0/2] HID: N-Trig: Adjustments for ntrig_probe()
From: SF Markus Elfring @ 2018-02-03 20:56 UTC (permalink / raw)
To: linux-input, Benjamin Tissoires, Jiri Kosina; +Cc: LKML, kernel-janitors
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 3 Feb 2018 21:54:32 +0100
Two update suggestions were taken into account
from static source code analysis.
Markus Elfring (2):
Delete an error message for a failed memory allocation
Improve a size determination
drivers/hid/hid-ntrig.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
--
2.16.1
^ permalink raw reply
* [PATCH 2/2] HID: picoLCD: Improve a size determination in picolcd_probe()
From: SF Markus Elfring @ 2018-02-03 20:20 UTC (permalink / raw)
To: linux-input, Benjamin Tissoires, Bruno Prémont, Jiri Kosina
Cc: LKML, kernel-janitors
In-Reply-To: <1ef884c9-9fbe-b95f-d318-b806639e6065@users.sourceforge.net>
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 3 Feb 2018 21:05:55 +0100
Replace the specification of a data structure by a pointer dereference
as the parameter for the operator "sizeof" to make the corresponding size
determination a bit safer according to the Linux coding style convention.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
drivers/hid/hid-picolcd_core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/hid/hid-picolcd_core.c b/drivers/hid/hid-picolcd_core.c
index 5dc48ecc1b98..ff38016be6fb 100644
--- a/drivers/hid/hid-picolcd_core.c
+++ b/drivers/hid/hid-picolcd_core.c
@@ -547,7 +547,7 @@ static int picolcd_probe(struct hid_device *hdev,
* Let's allocate the picolcd data structure, set some reasonable
* defaults, and associate it with the device
*/
- data = kzalloc(sizeof(struct picolcd_data), GFP_KERNEL);
+ data = kzalloc(sizeof(*data), GFP_KERNEL);
if (data == NULL) {
error = -ENOMEM;
goto err_no_cleanup;
--
2.16.1
^ permalink raw reply related
* [PATCH 1/2] HID: picoLCD: Delete an error message for a failed memory allocation in three functions
From: SF Markus Elfring @ 2018-02-03 20:18 UTC (permalink / raw)
To: linux-input, Benjamin Tissoires, Bruno Prémont, Jiri Kosina
Cc: LKML, kernel-janitors
In-Reply-To: <1ef884c9-9fbe-b95f-d318-b806639e6065@users.sourceforge.net>
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 3 Feb 2018 21:00:37 +0100
Omit an extra message for a memory allocation failure in these functions.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
drivers/hid/hid-picolcd_core.c | 1 -
drivers/hid/hid-picolcd_fb.c | 5 ++---
drivers/hid/hid-picolcd_leds.c | 1 -
3 files changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/hid/hid-picolcd_core.c b/drivers/hid/hid-picolcd_core.c
index c1b29a9eb41a..5dc48ecc1b98 100644
--- a/drivers/hid/hid-picolcd_core.c
+++ b/drivers/hid/hid-picolcd_core.c
@@ -549,7 +549,6 @@ 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;
}
diff --git a/drivers/hid/hid-picolcd_fb.c b/drivers/hid/hid-picolcd_fb.c
index 7f965e231433..5fb0b67b8d59 100644
--- a/drivers/hid/hid-picolcd_fb.c
+++ b/drivers/hid/hid-picolcd_fb.c
@@ -548,10 +548,9 @@ int picolcd_init_framebuffer(struct picolcd_data *data)
fbdata->force = 1;
fbdata->vbitmap = info->par + sizeof(struct picolcd_fb_data);
fbdata->bitmap = vmalloc(PICOLCDFB_SIZE*8);
- if (fbdata->bitmap == NULL) {
- dev_err(dev, "can't get a free page for framebuffer\n");
+ if (!fbdata->bitmap)
goto err_nomem;
- }
+
info->screen_base = (char __force __iomem *)fbdata->bitmap;
info->fix.smem_start = (unsigned long)fbdata->bitmap;
memset(fbdata->vbitmap, 0xff, PICOLCDFB_SIZE);
diff --git a/drivers/hid/hid-picolcd_leds.c b/drivers/hid/hid-picolcd_leds.c
index a802b4f49c7b..871cad90730a 100644
--- a/drivers/hid/hid-picolcd_leds.c
+++ b/drivers/hid/hid-picolcd_leds.c
@@ -122,7 +122,6 @@ int picolcd_init_leds(struct picolcd_data *data, struct hid_report *report)
for (i = 0; i < 8; i++) {
led = kzalloc(sizeof(struct led_classdev)+name_sz, GFP_KERNEL);
if (!led) {
- dev_err(dev, "can't allocate memory for LED %d\n", i);
ret = -ENOMEM;
goto err;
}
--
2.16.1
^ permalink raw reply related
* [PATCH 0/2] HID-picoLCD: Adjustments for three function implementations
From: SF Markus Elfring @ 2018-02-03 20:16 UTC (permalink / raw)
To: linux-input, Benjamin Tissoires, Bruno Prémont, Jiri Kosina
Cc: LKML, kernel-janitors
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 3 Feb 2018 21:12:24 +0100
Two update suggestions were taken into account
from static source code analysis.
Markus Elfring (2):
Delete an error message for a failed memory allocation in three functions
Improve a size determination in picolcd_probe()
drivers/hid/hid-picolcd_core.c | 3 +--
drivers/hid/hid-picolcd_fb.c | 5 ++---
drivers/hid/hid-picolcd_leds.c | 1 -
3 files changed, 3 insertions(+), 6 deletions(-)
--
2.16.1
^ permalink raw reply
* Re: [PATCH v2] Input: matrix_keypad - fix keypad does not response
From: Dmitry Torokhov @ 2018-02-03 19:35 UTC (permalink / raw)
To: Zhang Bo
Cc: robh, DRivshin, linux-input, linux-kernel, zhang.bo19,
andy.shevchenko
In-Reply-To: <20180203155816.18221-1-zbsdta@126.com>
Hi Zhang,
On Sat, Feb 03, 2018 at 11:58:16PM +0800, Zhang Bo wrote:
> If matrix_keypad_stop() is calling and the keypad interrupt is triggered,
> disable_row_irqs() is called by both matrix_keypad_interrupt() and
> matrix_keypad_stop() at the same time. then disable_row_irqs() is called
> twice, and the device enter suspend state before keypad->work is executed.
> At this condition the device will start keypad and enable irq once after
> resume. and then irqs are disabled yet because irqs are disabled twice and
> only enable once.
>
> Take lock around keypad->stopped and queue delayed work in
> matrix_keypad_start() and matrix_keypad_stop() to ensure irqs operation and
> scheduling scan work are in atomic operation.
>
> Signed-off-by: Zhang Bo <zbsdta@126.com>
> ---
> Changes in v2:
> - Change commit message and full name in the signed-off-by tag.
>
> drivers/input/keyboard/matrix_keypad.c | 18 ++++++++++++------
> 1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/input/keyboard/matrix_keypad.c b/drivers/input/keyboard/matrix_keypad.c
> index 1f316d66e6f7..13fe51824637 100644
> --- a/drivers/input/keyboard/matrix_keypad.c
> +++ b/drivers/input/keyboard/matrix_keypad.c
> @@ -169,7 +169,8 @@ static void matrix_keypad_scan(struct work_struct *work)
> /* Enable IRQs again */
> spin_lock_irq(&keypad->lock);
> keypad->scan_pending = false;
> - enable_row_irqs(keypad);
> + if (keypad->stopped == false)
> + enable_row_irqs(keypad);
> spin_unlock_irq(&keypad->lock);
> }
>
> @@ -202,14 +203,16 @@ static int matrix_keypad_start(struct input_dev *dev)
> {
> struct matrix_keypad *keypad = input_get_drvdata(dev);
>
> + spin_lock_irq(&keypad->lock);
> keypad->stopped = false;
> - mb();
>
> /*
> * Schedule an immediate key scan to capture current key state;
> * columns will be activated and IRQs be enabled after the scan.
> */
> - schedule_delayed_work(&keypad->work, 0);
> + if (keypad->scan_pending == false)
How can we have the pending scan if the keypad was disabled.
> + schedule_delayed_work(&keypad->work, 0);
> + spin_unlock_irq(&keypad->lock);
I do not think the change to matrix_keypad_start() is needed. If device
is quiesced we do not have issue of ISR racing with us here.
>
> return 0;
> }
> @@ -218,14 +221,17 @@ static void matrix_keypad_stop(struct input_dev *dev)
> {
> struct matrix_keypad *keypad = input_get_drvdata(dev);
>
> + spin_lock_irq(&keypad->lock);
> keypad->stopped = true;
> - mb();
> - flush_work(&keypad->work.work);
> /*
> * matrix_keypad_scan() will leave IRQs enabled;
> * we should disable them now.
> */
> - disable_row_irqs(keypad);
> + if (keypad->scan_pending == false)
> + disable_row_irqs(keypad);
> + spin_unlock_irq(&keypad->lock);
> +
> + flush_work(&keypad->work.work);
This is wrong, you should not have moved the flush_work() here. The
logic is as follows:
- set the "stopped" flag
- ensure that ISR has completed
- ensure that work item has finished (by doing flush_work()) - this will
make sure that interrupts are enabled (either ISR noticed "stopped
flag" and did not touch them, or ISR scheduled work and work item
re-enabled them)
- finally disable IRQs
Your change breaks this.
As far as I can see, the only change that is needed is this at the
beginning of matrix_keypad_stop():
spin_lock_irq(&keypad->lock);
keypad->stopped = true;
spin_unlock_irq(&keypad->lock);
Thanks.
--
Dmitry
^ permalink raw reply
* [PATCH 6/6] hid-prodikeys: Rename a jump label in pcmidi_send_note()
From: SF Markus Elfring @ 2018-02-03 17:42 UTC (permalink / raw)
To: linux-input, Benjamin Tissoires, Jiri Kosina; +Cc: LKML, kernel-janitors
In-Reply-To: <c388b4d8-fa43-1bf2-e27d-1a3a8d636118@users.sourceforge.net>
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 3 Feb 2018 18:10:00 +0100
An other label fits better to the jump target. Thus rename it.
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
drivers/hid/hid-prodikeys.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/hid-prodikeys.c b/drivers/hid/hid-prodikeys.c
index 1c62dddedd40..4596d97b61eb 100644
--- a/drivers/hid/hid-prodikeys.c
+++ b/drivers/hid/hid-prodikeys.c
@@ -228,11 +228,11 @@ static void pcmidi_send_note(struct pcmidi_snd *pm,
if (!pm->in_substream ||
!test_bit(pm->in_substream->number, &pm->in_triggered))
- goto drop_note;
+ goto unlock;
snd_rawmidi_receive(pm->in_substream, buffer, 3);
-drop_note:
+unlock:
spin_unlock_irqrestore(&pm->rawmidi_in_lock, flags);
}
--
2.16.1
^ permalink raw reply related
* [PATCH 5/6] hid-prodikeys: Combine two condition checks into one statement in pcmidi_send_note()
From: SF Markus Elfring @ 2018-02-03 17:41 UTC (permalink / raw)
To: linux-input, Benjamin Tissoires, Jiri Kosina; +Cc: LKML, kernel-janitors
In-Reply-To: <c388b4d8-fa43-1bf2-e27d-1a3a8d636118@users.sourceforge.net>
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 3 Feb 2018 18:00:06 +0100
The same label was used by goto statements after two condition checks.
Thus use a single statement instead.
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
drivers/hid/hid-prodikeys.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/hid/hid-prodikeys.c b/drivers/hid/hid-prodikeys.c
index c9eae92ad429..1c62dddedd40 100644
--- a/drivers/hid/hid-prodikeys.c
+++ b/drivers/hid/hid-prodikeys.c
@@ -226,9 +226,8 @@ static void pcmidi_send_note(struct pcmidi_snd *pm,
spin_lock_irqsave(&pm->rawmidi_in_lock, flags);
- if (!pm->in_substream)
- goto drop_note;
- if (!test_bit(pm->in_substream->number, &pm->in_triggered))
+ if (!pm->in_substream ||
+ !test_bit(pm->in_substream->number, &pm->in_triggered))
goto drop_note;
snd_rawmidi_receive(pm->in_substream, buffer, 3);
--
2.16.1
^ permalink raw reply related
* [PATCH 4/6] hid-prodikeys: Delete an unnecessary return statement in pcmidi_send_note()
From: SF Markus Elfring @ 2018-02-03 17:40 UTC (permalink / raw)
To: linux-input, Benjamin Tissoires, Jiri Kosina; +Cc: LKML, kernel-janitors
In-Reply-To: <c388b4d8-fa43-1bf2-e27d-1a3a8d636118@users.sourceforge.net>
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 3 Feb 2018 17:33:40 +0100
The script "checkpatch.pl" pointed information out like the following.
WARNING: void function return statements are not generally useful
Thus remove such a statement in the affected function.
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
drivers/hid/hid-prodikeys.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/hid/hid-prodikeys.c b/drivers/hid/hid-prodikeys.c
index 7672f723b268..c9eae92ad429 100644
--- a/drivers/hid/hid-prodikeys.c
+++ b/drivers/hid/hid-prodikeys.c
@@ -235,8 +235,6 @@ static void pcmidi_send_note(struct pcmidi_snd *pm,
drop_note:
spin_unlock_irqrestore(&pm->rawmidi_in_lock, flags);
-
- return;
}
static void pcmidi_sustained_note_release(struct timer_list *t)
--
2.16.1
^ permalink raw reply related
* [PATCH 3/6] hid-prodikeys: Move an assignment in pk_probe()
From: SF Markus Elfring @ 2018-02-03 17:39 UTC (permalink / raw)
To: linux-input, Benjamin Tissoires, Jiri Kosina; +Cc: LKML, kernel-janitors
In-Reply-To: <c388b4d8-fa43-1bf2-e27d-1a3a8d636118@users.sourceforge.net>
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 3 Feb 2018 17:20:01 +0100
Move one assignment for a data structure member in one local variable
so that its setting will only be performed after a corresponding memory
allocation succeeded by this function.
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
drivers/hid/hid-prodikeys.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/hid/hid-prodikeys.c b/drivers/hid/hid-prodikeys.c
index 2e79dfc92162..7672f723b268 100644
--- a/drivers/hid/hid-prodikeys.c
+++ b/drivers/hid/hid-prodikeys.c
@@ -804,8 +804,6 @@ static int pk_probe(struct hid_device *hdev, const struct hid_device_id *id)
if (!pk)
return -ENOMEM;
- pk->hdev = hdev;
-
pm = kzalloc(sizeof(*pm), GFP_KERNEL);
if (pm == NULL) {
ret = -ENOMEM;
@@ -814,6 +812,7 @@ static int pk_probe(struct hid_device *hdev, const struct hid_device_id *id)
pm->pk = pk;
pk->pm = pm;
+ pk->hdev = hdev;
pm->ifnum = ifnum;
hid_set_drvdata(hdev, pk);
--
2.16.1
^ permalink raw reply related
* [PATCH 2/6] hid-prodikeys: Delete an unnecessary variable initialisation in pk_probe()
From: SF Markus Elfring @ 2018-02-03 17:38 UTC (permalink / raw)
To: linux-input, Benjamin Tissoires, Jiri Kosina; +Cc: LKML, kernel-janitors
In-Reply-To: <c388b4d8-fa43-1bf2-e27d-1a3a8d636118@users.sourceforge.net>
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 3 Feb 2018 14:45:24 +0100
The local variable "pm" will eventually be set to an appropriate pointer
a bit later. Thus omit the explicit initialisation at the beginning.
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
drivers/hid/hid-prodikeys.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/hid/hid-prodikeys.c b/drivers/hid/hid-prodikeys.c
index 9cad4973a67b..2e79dfc92162 100644
--- a/drivers/hid/hid-prodikeys.c
+++ b/drivers/hid/hid-prodikeys.c
@@ -798,6 +798,6 @@ static int pk_probe(struct hid_device *hdev, const struct hid_device_id *id)
unsigned short ifnum = intf->cur_altsetting->desc.bInterfaceNumber;
unsigned long quirks = id->driver_data;
struct pk_device *pk;
- struct pcmidi_snd *pm = NULL;
+ struct pcmidi_snd *pm;
pk = kzalloc(sizeof(*pk), GFP_KERNEL);
--
2.16.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox