* Re: [PATCH] GPIO button wth wakeup attribute is supposed to wake the system up
From: Dmitry Torokhov @ 2014-07-09 0:15 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Li, Aubrey, linux-input@vger.kernel.org, LKML,
One Thousand Gnomes, Linux PM list
In-Reply-To: <2213522.hTnsy9lMxO@vostro.rjw.lan>
On Wed, Jul 09, 2014 at 01:06:07AM +0200, Rafael J. Wysocki wrote:
> On Tuesday, July 08, 2014 03:11:14 PM Dmitry Torokhov wrote:
> > On Tue, Jul 08, 2014 at 11:47:01PM +0200, Rafael J. Wysocki wrote:
> > > On Tuesday, July 08, 2014 02:12:59 PM Dmitry Torokhov wrote:
> > > > On Tue, Jul 08, 2014 at 11:06:17PM +0200, Rafael J. Wysocki wrote:
> > > > > On Tuesday, July 08, 2014 01:45:30 PM Dmitry Torokhov wrote:
> > > > > > On Tue, Jul 08, 2014 at 10:52:52PM +0200, Rafael J. Wysocki wrote:
> > > > > > > On Thursday, June 19, 2014 08:51:25 AM Li, Aubrey wrote:
> > > > > > > > When the wakeup attribute is set, the GPIO button is capable of
> > > > > > > > waking up the system from sleep states, including the "freeze"
> > > > > > > > sleep state. For that to work, its driver needs to pass the
> > > > > > > > IRQF_NO_SUSPEND flag to devm_request_any_context_irq(), or the
> > > > > > > > interrupt will be disabled by suspend_device_irqs() and the
> > > > > > > > system won't be woken up by it from the "freeze" sleep state.
> > > > > > > >
> > > > > > > > The suspend_device_irqs() routine is a workaround for drivers
> > > > > > > > that mishandle interrupts triggered when the devices handled
> > > > > > > > by them are suspended, so it is safe to use IRQF_NO_SUSPEND in
> > > > > > > > all drivers that don't have that problem.
> > > > > > > >
> > > > > > > > The affected/tested machines include Dell Venue 11 Pro and Asus T100TA.
> > > > > > > >
> > > > > > > > Signed-off-by: Aubrey Li <aubrey.li@linux.intel.com>
> > > > > > > > Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > > > >
> > > > > > > OK
> > > > > > >
> > > > > > > Due to the lack of response (ie. no objections) and because the issue
> > > > > > > addressed by this patch is real, I'm queuing it up as a PM-related fix
> > > > > > > for 3.17.
> > > > > >
> > > > > > Please do not. The response is till the same: board code should make sure
> > > > > > that enable_irq_wake() does the right thing and keeps interrupts enabled.
> > > > >
> > > > > Which board code? That's nothing like that for the platforms in question.
> > > >
> > > > Then it needs to be written.
> > >
> > > Well, excuse me, but I don't get it. Why would I need to write any board code
> > > for an ACPI-based system?
> >
> > Why would not you? What is the difference between ACPI systems and all
> > other systems? If ACPI-based systems need certain behavior they need to
> > implement it, the same as DT-based systems or custom boards.
>
> I'm not sure what you're talking about.
>
> This isn't an ACPI-based system that needs certain behavior. That's certain
> behavior we want from GPIO buttons that can wake up *in* *general*.
>
> Regardless of the platform, we want interrupts from those buttons to happen
> after calling suspend_device_irqs(). Why? Because we want them to be able
> to happen while freeze_enter() is being executed and *that* is platform
> independent.
Tell me this: do we always call suspend_device_irqs()?
>
> > > > > > It is wrong to patch drivers for this.
> > > > >
> > > > > Why is it? Only drivers know if they can handle incoming interrupts after
> > > > > having suspended their devices.
> > > >
> > > > The driver correctly used enable_irq_wake() to indicate that interrupt should
> > > > be a wakeup source, the now the core/board needs to make sure the interrupt
> > > > gets delivered to the driver properly. We should not be patching every driver
> > > > that uses enable_irq_wake() with IRQF_NO_SUSPEND.
> > >
> > > Interrupts that can wake up from the "freeze" sleep state need not be set up
> > > with enable_irq_wake() and the flag doesn't say "this is a wakeup interrupt".
> > > It says "do not suspend this interrupt, I can handle it after the device has
> > > been suspended" (as I said).
> >
> > And the driver does not really care about this and whether the sleep
> > state is suspend or freeze or something else, it is your platform that
> > cares and has certain limitations that require interrupts to be not
> > suspended in certain cases.
>
> Not in "certain cases", but actually never and it is not about any platform
> limitations. I'm not sure what other words I need to use to make it more
> clear.
>
> > From the driver POV it says that the device can be a waekup source
> > (again it does not care about details as to which sleep state we woudl
> > be waking from) and it expects the PM core to handle the things
> > properly. If certain sleep state requires interrupts to be kept on then
> > PM core should make them as such, not driver.
>
> That would be the case in an ideal world, but the real one is not ideal,
> unfortunately. The problem is that the PM core actually cannot decide
> which interrupts to keep on, because it doesn't know which drivers can
> cope with interrupts that happen after their devices have been suspended.
> Using IRQF_NO_SUSPEND is the way to experess that by a driver.
When device driver marks IRQ as a wakeup source I believe it is prepared
to handle it (or it would shut it off explicitly).
>
> And again, that has nothing to do with platform limitations or requirements.
> All this is about is whether or not to allow interrupts to be *handled* by
> drivers after certain point in the suspend sequence, which is
> suspend_device_irqs(). By using IRQF_NO_SUSPEND drivers say, basically "it is
> OK to leave this IRQ as is, I promise to take care of it going forward" and
> that covers *all* suspend/hibernate transitions.
>
> The reason why the "freeze" sleep state is somewhat special is that it doesn't
> do any platform-specific magic and needs *normal* device interrupts to work
> after suspend_device_irqs(). And I would *love* *to* drop suspend_device_irqs()
> at this point, but I can't, because there still are drivers that would have
> broken had I done that.
>
> And I'm saying "somewhat" above, because that behavior is actually needed to
> prevent wakeup events occuring *during* suspend transitions from being lost
> for all kinds of those transitions (if someone actually cares).
>
> > >
> > > And if it is OK for a driver to set IRQF_SHARED, it is equally OK for it to
> > > set IRQF_NO_SUSPEND, because, in fact, those two flags are related.
> >
> > Are you proposing for IRQ core to automatically set IRQF_NO_SUSPEND for
> > IRQF_SHARED interrupts? That wold be fine with me.
>
> No, I'm not.
Then they are not really related that closely, are they?
>
> The first choice basically is to go through all drivers that don't use IRQF_NO_SUSPEND
> and which have _noirq suspend callbacks (that covers all PCI drivers and some
> non-PCI ones too IIRC) and audit their interrupt handlers to check whether or not
> they can cope with interrupts coming after their devices have been suspended. Fix
> the ones that can't and we can drop suspend_device_irqs().
>
> But I guess I may be forgiven for regarding that as rather unattractive. And we
> actually could have done that to start with, guess why we didn't?
>
> The second choice is to use IRQF_NO_SUSPEND in the drivers that are OK. That
> isn't too attractive either but has been practice for quite a while.
>
> > > > If you look at the earlier patch discussion Tegra folks managed to implement
> > > > this behavior just fine.
> > >
> > > I'm not sure whose idea it was that IRQF_NO_SUSPEND was not to be set by drivers,
> > > but it is not a correct one. I know why suspend_device_irqs() was introduced
> > > and I'm telling you this has nothing to do with setting up the IRQ chip to do
> > > system wakeup.
> >
> > I do not believe I asked why suspend_device_irqs() was introduced.
>
> But you should, because suspend_device_irqs() is the very reason why
> IRQF_NO_SUSPEND exists. :-)
Then you should have shared this knowledge instead of asserting that you
possess it.
>
> > >
> > > And please grep for IRQF_NO_SUSPEND to see how drivers generally use it.
> >
> > I see that just handful of them use IRQF_NO_SUSPEND (not sure how many
> > are actully required), I see that a lot more drivers use
> > enable_irq_wake() and do not bother setting IRQF_NO_SUSPEND.
>
> And they will have problems with the "freeze" sleep state.
>
> enable_irq_wake() is *not* a replacement for IRQF_NO_SUSPEND, nor the other
> way around. They are different things.
However what I hear is that one has to use one when using another, and
form this fact comes my request: when entering freeze sleep state
interrupts that are marked as wakeup sources should be automatically
excluded form the list of IRQs that need to be suspended.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] GPIO button wth wakeup attribute is supposed to wake the system up
From: Rafael J. Wysocki @ 2014-07-09 0:59 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Li, Aubrey, linux-input@vger.kernel.org, LKML,
One Thousand Gnomes, Linux PM list
In-Reply-To: <20140709001506.GA14935@core.coreip.homeip.net>
On Tuesday, July 08, 2014 05:15:06 PM Dmitry Torokhov wrote:
> On Wed, Jul 09, 2014 at 01:06:07AM +0200, Rafael J. Wysocki wrote:
> > On Tuesday, July 08, 2014 03:11:14 PM Dmitry Torokhov wrote:
> > > On Tue, Jul 08, 2014 at 11:47:01PM +0200, Rafael J. Wysocki wrote:
> > > > On Tuesday, July 08, 2014 02:12:59 PM Dmitry Torokhov wrote:
> > > > > On Tue, Jul 08, 2014 at 11:06:17PM +0200, Rafael J. Wysocki wrote:
> > > > > > On Tuesday, July 08, 2014 01:45:30 PM Dmitry Torokhov wrote:
> > > > > > > On Tue, Jul 08, 2014 at 10:52:52PM +0200, Rafael J. Wysocki wrote:
> > > > > > > > On Thursday, June 19, 2014 08:51:25 AM Li, Aubrey wrote:
> > > > > > > > > When the wakeup attribute is set, the GPIO button is capable of
> > > > > > > > > waking up the system from sleep states, including the "freeze"
> > > > > > > > > sleep state. For that to work, its driver needs to pass the
> > > > > > > > > IRQF_NO_SUSPEND flag to devm_request_any_context_irq(), or the
> > > > > > > > > interrupt will be disabled by suspend_device_irqs() and the
> > > > > > > > > system won't be woken up by it from the "freeze" sleep state.
> > > > > > > > >
> > > > > > > > > The suspend_device_irqs() routine is a workaround for drivers
> > > > > > > > > that mishandle interrupts triggered when the devices handled
> > > > > > > > > by them are suspended, so it is safe to use IRQF_NO_SUSPEND in
> > > > > > > > > all drivers that don't have that problem.
> > > > > > > > >
> > > > > > > > > The affected/tested machines include Dell Venue 11 Pro and Asus T100TA.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Aubrey Li <aubrey.li@linux.intel.com>
> > > > > > > > > Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > > > > >
> > > > > > > > OK
> > > > > > > >
> > > > > > > > Due to the lack of response (ie. no objections) and because the issue
> > > > > > > > addressed by this patch is real, I'm queuing it up as a PM-related fix
> > > > > > > > for 3.17.
> > > > > > >
> > > > > > > Please do not. The response is till the same: board code should make sure
> > > > > > > that enable_irq_wake() does the right thing and keeps interrupts enabled.
> > > > > >
> > > > > > Which board code? That's nothing like that for the platforms in question.
> > > > >
> > > > > Then it needs to be written.
> > > >
> > > > Well, excuse me, but I don't get it. Why would I need to write any board code
> > > > for an ACPI-based system?
> > >
> > > Why would not you? What is the difference between ACPI systems and all
> > > other systems? If ACPI-based systems need certain behavior they need to
> > > implement it, the same as DT-based systems or custom boards.
> >
> > I'm not sure what you're talking about.
> >
> > This isn't an ACPI-based system that needs certain behavior. That's certain
> > behavior we want from GPIO buttons that can wake up *in* *general*.
> >
> > Regardless of the platform, we want interrupts from those buttons to happen
> > after calling suspend_device_irqs(). Why? Because we want them to be able
> > to happen while freeze_enter() is being executed and *that* is platform
> > independent.
>
> Tell me this: do we always call suspend_device_irqs()?
Yes, we do.
> > > > > > > It is wrong to patch drivers for this.
> > > > > >
> > > > > > Why is it? Only drivers know if they can handle incoming interrupts after
> > > > > > having suspended their devices.
> > > > >
> > > > > The driver correctly used enable_irq_wake() to indicate that interrupt should
> > > > > be a wakeup source, the now the core/board needs to make sure the interrupt
> > > > > gets delivered to the driver properly. We should not be patching every driver
> > > > > that uses enable_irq_wake() with IRQF_NO_SUSPEND.
> > > >
> > > > Interrupts that can wake up from the "freeze" sleep state need not be set up
> > > > with enable_irq_wake() and the flag doesn't say "this is a wakeup interrupt".
> > > > It says "do not suspend this interrupt, I can handle it after the device has
> > > > been suspended" (as I said).
> > >
> > > And the driver does not really care about this and whether the sleep
> > > state is suspend or freeze or something else, it is your platform that
> > > cares and has certain limitations that require interrupts to be not
> > > suspended in certain cases.
> >
> > Not in "certain cases", but actually never and it is not about any platform
> > limitations. I'm not sure what other words I need to use to make it more
> > clear.
> >
> > > From the driver POV it says that the device can be a waekup source
> > > (again it does not care about details as to which sleep state we woudl
> > > be waking from) and it expects the PM core to handle the things
> > > properly. If certain sleep state requires interrupts to be kept on then
> > > PM core should make them as such, not driver.
> >
> > That would be the case in an ideal world, but the real one is not ideal,
> > unfortunately. The problem is that the PM core actually cannot decide
> > which interrupts to keep on, because it doesn't know which drivers can
> > cope with interrupts that happen after their devices have been suspended.
> > Using IRQF_NO_SUSPEND is the way to experess that by a driver.
>
> When device driver marks IRQ as a wakeup source I believe it is prepared
> to handle it (or it would shut it off explicitly).
I can agree with that.
Are you suggesting that __disable_irq() should check irq_data for
IRQD_WAKEUP_STATE and skip the IRQ (in the 'suspend' case) if that is set?
That might work too.
> > And again, that has nothing to do with platform limitations or requirements.
> > All this is about is whether or not to allow interrupts to be *handled* by
> > drivers after certain point in the suspend sequence, which is
> > suspend_device_irqs(). By using IRQF_NO_SUSPEND drivers say, basically "it is
> > OK to leave this IRQ as is, I promise to take care of it going forward" and
> > that covers *all* suspend/hibernate transitions.
> >
> > The reason why the "freeze" sleep state is somewhat special is that it doesn't
> > do any platform-specific magic and needs *normal* device interrupts to work
> > after suspend_device_irqs(). And I would *love* *to* drop suspend_device_irqs()
> > at this point, but I can't, because there still are drivers that would have
> > broken had I done that.
> >
> > And I'm saying "somewhat" above, because that behavior is actually needed to
> > prevent wakeup events occuring *during* suspend transitions from being lost
> > for all kinds of those transitions (if someone actually cares).
> >
> > > >
> > > > And if it is OK for a driver to set IRQF_SHARED, it is equally OK for it to
> > > > set IRQF_NO_SUSPEND, because, in fact, those two flags are related.
> > >
> > > Are you proposing for IRQ core to automatically set IRQF_NO_SUSPEND for
> > > IRQF_SHARED interrupts? That wold be fine with me.
> >
> > No, I'm not.
>
> Then they are not really related that closely, are they?
Yes, they are. The original underlying problem was that some drivers with
IRQF_SHARED set mishandled interrupts triggered by the other device sharing the
IRQ line (so they did not expect the interrupt to trigger at that point, but it did).
> > The first choice basically is to go through all drivers that don't use IRQF_NO_SUSPEND
> > and which have _noirq suspend callbacks (that covers all PCI drivers and some
> > non-PCI ones too IIRC) and audit their interrupt handlers to check whether or not
> > they can cope with interrupts coming after their devices have been suspended. Fix
> > the ones that can't and we can drop suspend_device_irqs().
> >
> > But I guess I may be forgiven for regarding that as rather unattractive. And we
> > actually could have done that to start with, guess why we didn't?
> >
> > The second choice is to use IRQF_NO_SUSPEND in the drivers that are OK. That
> > isn't too attractive either but has been practice for quite a while.
> >
> > > > > If you look at the earlier patch discussion Tegra folks managed to implement
> > > > > this behavior just fine.
> > > >
> > > > I'm not sure whose idea it was that IRQF_NO_SUSPEND was not to be set by drivers,
> > > > but it is not a correct one. I know why suspend_device_irqs() was introduced
> > > > and I'm telling you this has nothing to do with setting up the IRQ chip to do
> > > > system wakeup.
> > >
> > > I do not believe I asked why suspend_device_irqs() was introduced.
> >
> > But you should, because suspend_device_irqs() is the very reason why
> > IRQF_NO_SUSPEND exists. :-)
>
> Then you should have shared this knowledge instead of asserting that you
> possess it.
Well, I just thought you knew that too.
> > > >
> > > > And please grep for IRQF_NO_SUSPEND to see how drivers generally use it.
> > >
> > > I see that just handful of them use IRQF_NO_SUSPEND (not sure how many
> > > are actully required), I see that a lot more drivers use
> > > enable_irq_wake() and do not bother setting IRQF_NO_SUSPEND.
> >
> > And they will have problems with the "freeze" sleep state.
> >
> > enable_irq_wake() is *not* a replacement for IRQF_NO_SUSPEND, nor the other
> > way around. They are different things.
>
> However what I hear is that one has to use one when using another, and
> form this fact comes my request:
That isn't exactly the case. IRQF_NO_SUSPEND need not imply enable_irq_wake().
> when entering freeze sleep state
> interrupts that are marked as wakeup sources should be automatically
> excluded form the list of IRQs that need to be suspended.
That seems to be the case for all sleep states, doesn't it?
Rafael
^ permalink raw reply
* Re: [PATCH] GPIO button wth wakeup attribute is supposed to wake the system up
From: Dmitry Torokhov @ 2014-07-09 0:54 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Li, Aubrey, linux-input@vger.kernel.org, LKML,
One Thousand Gnomes, Linux PM list
In-Reply-To: <18261725.84uqcyZ8CE@vostro.rjw.lan>
On Wed, Jul 09, 2014 at 02:59:33AM +0200, Rafael J. Wysocki wrote:
> On Tuesday, July 08, 2014 05:15:06 PM Dmitry Torokhov wrote:
> > On Wed, Jul 09, 2014 at 01:06:07AM +0200, Rafael J. Wysocki wrote:
> > > On Tuesday, July 08, 2014 03:11:14 PM Dmitry Torokhov wrote:
> > > > On Tue, Jul 08, 2014 at 11:47:01PM +0200, Rafael J. Wysocki wrote:
> > > > > On Tuesday, July 08, 2014 02:12:59 PM Dmitry Torokhov wrote:
> > > > > > On Tue, Jul 08, 2014 at 11:06:17PM +0200, Rafael J. Wysocki wrote:
> > > > > > > On Tuesday, July 08, 2014 01:45:30 PM Dmitry Torokhov wrote:
> > > > > > > > On Tue, Jul 08, 2014 at 10:52:52PM +0200, Rafael J. Wysocki wrote:
> > > > > > > > > On Thursday, June 19, 2014 08:51:25 AM Li, Aubrey wrote:
> > > > > > > > > > When the wakeup attribute is set, the GPIO button is capable of
> > > > > > > > > > waking up the system from sleep states, including the "freeze"
> > > > > > > > > > sleep state. For that to work, its driver needs to pass the
> > > > > > > > > > IRQF_NO_SUSPEND flag to devm_request_any_context_irq(), or the
> > > > > > > > > > interrupt will be disabled by suspend_device_irqs() and the
> > > > > > > > > > system won't be woken up by it from the "freeze" sleep state.
> > > > > > > > > >
> > > > > > > > > > The suspend_device_irqs() routine is a workaround for drivers
> > > > > > > > > > that mishandle interrupts triggered when the devices handled
> > > > > > > > > > by them are suspended, so it is safe to use IRQF_NO_SUSPEND in
> > > > > > > > > > all drivers that don't have that problem.
> > > > > > > > > >
> > > > > > > > > > The affected/tested machines include Dell Venue 11 Pro and Asus T100TA.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Aubrey Li <aubrey.li@linux.intel.com>
> > > > > > > > > > Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > > > > > >
> > > > > > > > > OK
> > > > > > > > >
> > > > > > > > > Due to the lack of response (ie. no objections) and because the issue
> > > > > > > > > addressed by this patch is real, I'm queuing it up as a PM-related fix
> > > > > > > > > for 3.17.
> > > > > > > >
> > > > > > > > Please do not. The response is till the same: board code should make sure
> > > > > > > > that enable_irq_wake() does the right thing and keeps interrupts enabled.
> > > > > > >
> > > > > > > Which board code? That's nothing like that for the platforms in question.
> > > > > >
> > > > > > Then it needs to be written.
> > > > >
> > > > > Well, excuse me, but I don't get it. Why would I need to write any board code
> > > > > for an ACPI-based system?
> > > >
> > > > Why would not you? What is the difference between ACPI systems and all
> > > > other systems? If ACPI-based systems need certain behavior they need to
> > > > implement it, the same as DT-based systems or custom boards.
> > >
> > > I'm not sure what you're talking about.
> > >
> > > This isn't an ACPI-based system that needs certain behavior. That's certain
> > > behavior we want from GPIO buttons that can wake up *in* *general*.
> > >
> > > Regardless of the platform, we want interrupts from those buttons to happen
> > > after calling suspend_device_irqs(). Why? Because we want them to be able
> > > to happen while freeze_enter() is being executed and *that* is platform
> > > independent.
> >
> > Tell me this: do we always call suspend_device_irqs()?
>
> Yes, we do.
OK, I see.
>
> > > > > > > > It is wrong to patch drivers for this.
> > > > > > >
> > > > > > > Why is it? Only drivers know if they can handle incoming interrupts after
> > > > > > > having suspended their devices.
> > > > > >
> > > > > > The driver correctly used enable_irq_wake() to indicate that interrupt should
> > > > > > be a wakeup source, the now the core/board needs to make sure the interrupt
> > > > > > gets delivered to the driver properly. We should not be patching every driver
> > > > > > that uses enable_irq_wake() with IRQF_NO_SUSPEND.
> > > > >
> > > > > Interrupts that can wake up from the "freeze" sleep state need not be set up
> > > > > with enable_irq_wake() and the flag doesn't say "this is a wakeup interrupt".
> > > > > It says "do not suspend this interrupt, I can handle it after the device has
> > > > > been suspended" (as I said).
> > > >
> > > > And the driver does not really care about this and whether the sleep
> > > > state is suspend or freeze or something else, it is your platform that
> > > > cares and has certain limitations that require interrupts to be not
> > > > suspended in certain cases.
> > >
> > > Not in "certain cases", but actually never and it is not about any platform
> > > limitations. I'm not sure what other words I need to use to make it more
> > > clear.
> > >
> > > > From the driver POV it says that the device can be a waekup source
> > > > (again it does not care about details as to which sleep state we woudl
> > > > be waking from) and it expects the PM core to handle the things
> > > > properly. If certain sleep state requires interrupts to be kept on then
> > > > PM core should make them as such, not driver.
> > >
> > > That would be the case in an ideal world, but the real one is not ideal,
> > > unfortunately. The problem is that the PM core actually cannot decide
> > > which interrupts to keep on, because it doesn't know which drivers can
> > > cope with interrupts that happen after their devices have been suspended.
> > > Using IRQF_NO_SUSPEND is the way to experess that by a driver.
> >
> > When device driver marks IRQ as a wakeup source I believe it is prepared
> > to handle it (or it would shut it off explicitly).
>
> I can agree with that.
>
> Are you suggesting that __disable_irq() should check irq_data for
> IRQD_WAKEUP_STATE and skip the IRQ (in the 'suspend' case) if that is set?
Yes, something like that.
>
> That might work too.
>
> > > And again, that has nothing to do with platform limitations or requirements.
> > > All this is about is whether or not to allow interrupts to be *handled* by
> > > drivers after certain point in the suspend sequence, which is
> > > suspend_device_irqs(). By using IRQF_NO_SUSPEND drivers say, basically "it is
> > > OK to leave this IRQ as is, I promise to take care of it going forward" and
> > > that covers *all* suspend/hibernate transitions.
> > >
> > > The reason why the "freeze" sleep state is somewhat special is that it doesn't
> > > do any platform-specific magic and needs *normal* device interrupts to work
> > > after suspend_device_irqs(). And I would *love* *to* drop suspend_device_irqs()
> > > at this point, but I can't, because there still are drivers that would have
> > > broken had I done that.
> > >
> > > And I'm saying "somewhat" above, because that behavior is actually needed to
> > > prevent wakeup events occuring *during* suspend transitions from being lost
> > > for all kinds of those transitions (if someone actually cares).
> > >
> > > > >
> > > > > And if it is OK for a driver to set IRQF_SHARED, it is equally OK for it to
> > > > > set IRQF_NO_SUSPEND, because, in fact, those two flags are related.
> > > >
> > > > Are you proposing for IRQ core to automatically set IRQF_NO_SUSPEND for
> > > > IRQF_SHARED interrupts? That wold be fine with me.
> > >
> > > No, I'm not.
> >
> > Then they are not really related that closely, are they?
>
> Yes, they are. The original underlying problem was that some drivers with
> IRQF_SHARED set mishandled interrupts triggered by the other device sharing the
> IRQ line (so they did not expect the interrupt to trigger at that point, but it did).
Right, well-behaved driver using IRQF_SHARED should expect IRQ at any
time, even if the device supposed to be quiesced at the time. But I
guess we are still not comfortable setting IRQF_NO_SUSPEND even though
it should be OK.
>
> > > The first choice basically is to go through all drivers that don't use IRQF_NO_SUSPEND
> > > and which have _noirq suspend callbacks (that covers all PCI drivers and some
> > > non-PCI ones too IIRC) and audit their interrupt handlers to check whether or not
> > > they can cope with interrupts coming after their devices have been suspended. Fix
> > > the ones that can't and we can drop suspend_device_irqs().
> > >
> > > But I guess I may be forgiven for regarding that as rather unattractive. And we
> > > actually could have done that to start with, guess why we didn't?
> > >
> > > The second choice is to use IRQF_NO_SUSPEND in the drivers that are OK. That
> > > isn't too attractive either but has been practice for quite a while.
> > >
> > > > > > If you look at the earlier patch discussion Tegra folks managed to implement
> > > > > > this behavior just fine.
> > > > >
> > > > > I'm not sure whose idea it was that IRQF_NO_SUSPEND was not to be set by drivers,
> > > > > but it is not a correct one. I know why suspend_device_irqs() was introduced
> > > > > and I'm telling you this has nothing to do with setting up the IRQ chip to do
> > > > > system wakeup.
> > > >
> > > > I do not believe I asked why suspend_device_irqs() was introduced.
> > >
> > > But you should, because suspend_device_irqs() is the very reason why
> > > IRQF_NO_SUSPEND exists. :-)
> >
> > Then you should have shared this knowledge instead of asserting that you
> > possess it.
>
> Well, I just thought you knew that too.
>
> > > > >
> > > > > And please grep for IRQF_NO_SUSPEND to see how drivers generally use it.
> > > >
> > > > I see that just handful of them use IRQF_NO_SUSPEND (not sure how many
> > > > are actully required), I see that a lot more drivers use
> > > > enable_irq_wake() and do not bother setting IRQF_NO_SUSPEND.
> > >
> > > And they will have problems with the "freeze" sleep state.
> > >
> > > enable_irq_wake() is *not* a replacement for IRQF_NO_SUSPEND, nor the other
> > > way around. They are different things.
> >
> > However what I hear is that one has to use one when using another, and
> > form this fact comes my request:
>
> That isn't exactly the case. IRQF_NO_SUSPEND need not imply enable_irq_wake().
Right, but I am hoping that enable_irq_wake() can imply IRQF_NO_SUSPEND.
>
> > when entering freeze sleep state
> > interrupts that are marked as wakeup sources should be automatically
> > excluded form the list of IRQs that need to be suspended.
>
> That seems to be the case for all sleep states, doesn't it?
I was under impression that some of sleep paths were skipping freezer.
If not then they all need this.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] Input: s3c2410_ts: Move to clk_prepare_enable/clk_disable_unprepare
From: Dmitry Torokhov @ 2014-07-09 1:00 UTC (permalink / raw)
To: Vasily Khoruzhick
Cc: Ben Dooks, Kukjin Kim, linux-arm-kernel, linux-samsung-soc,
linux-input
In-Reply-To: <1404155377-2862-1-git-send-email-anarsoul@gmail.com>
On Mon, Jun 30, 2014 at 10:09:37PM +0300, Vasily Khoruzhick wrote:
> Use clk_prepare_enable/clk_disable_unprepare to make the driver
> work properly with common clock framework.
>
> Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> ---
> drivers/input/touchscreen/s3c2410_ts.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/input/touchscreen/s3c2410_ts.c b/drivers/input/touchscreen/s3c2410_ts.c
> index 19cb247..c0e0baa 100644
> --- a/drivers/input/touchscreen/s3c2410_ts.c
> +++ b/drivers/input/touchscreen/s3c2410_ts.c
> @@ -264,7 +264,7 @@ static int s3c2410ts_probe(struct platform_device *pdev)
> return -ENOENT;
> }
>
> - clk_enable(ts.clock);
> + clk_prepare_enable(ts.clock);
> dev_dbg(dev, "got and enabled clocks\n");
>
> ts.irq_tc = ret = platform_get_irq(pdev, 0);
> @@ -369,7 +369,7 @@ static int s3c2410ts_remove(struct platform_device *pdev)
> free_irq(ts.irq_tc, ts.input);
> del_timer_sync(&touch_timer);
>
> - clk_disable(ts.clock);
> + clk_disable_unprepare(ts.clock);
> clk_put(ts.clock);
>
> input_unregister_device(ts.input);
> @@ -383,7 +383,7 @@ static int s3c2410ts_suspend(struct device *dev)
> {
> writel(TSC_SLEEP, ts.io + S3C2410_ADCTSC);
> disable_irq(ts.irq_tc);
> - clk_disable(ts.clock);
> + clk_disable_unprepare(ts.clock);
Do we really need to unprepare on suspend? Why simply disabling is not
enough here?
Thanks.
>
> return 0;
> }
> @@ -393,7 +393,7 @@ static int s3c2410ts_resume(struct device *dev)
> struct platform_device *pdev = to_platform_device(dev);
> struct s3c2410_ts_mach_info *info = dev_get_platdata(&pdev->dev);
>
> - clk_enable(ts.clock);
> + clk_prepare_enable(ts.clock);
> enable_irq(ts.irq_tc);
>
> /* Initialise registers */
> --
> 2.0.0
>
--
Dmitry
^ permalink raw reply
* Re: [PATCH] soc_button_array: fix the issue that button device can't be enumerated since 3.16-rc1
From: Zhang Rui @ 2014-07-09 2:27 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: dmitry.torokhov, yao.jin, linux-input
In-Reply-To: <2556427.cdnVoQoSXY@vostro.rjw.lan>
On Tue, 2014-07-08 at 22:50 +0200, Rafael J. Wysocki wrote:
> On Wednesday, July 02, 2014 10:01:53 PM Zhang Rui wrote:
> > From c2ee1886ba230d9d93d2ea2f350b1dc1a2d5ead5 Mon Sep 17 00:00:00 2001
> > From: Jin Yao <yao.jin@linux.intel.com>
> > Date: Thu, 26 Jun 2014 10:26:44 +0800
> > Subject: [PATCH] soc_button_array: fix the issue that button device can't be
> > enumerated since 3.16-rc1
>
> Hi Rui,
>
> For 3.16 I'm afraid we need to add the missing device ID to the PNP list.
> It is too late to do the conversion at this point IMO and we can do it later.
Okay, then please take the patch below.
>From 846c16d9d863723ef9a2d9722778bce0633ea24c Mon Sep 17 00:00:00 2001
From: Zhang Rui <rui.zhang@intel.com>
Date: Wed, 9 Jul 2014 10:19:38 +0800
Subject: [PATCH] ACPI: add soc_button_array device id in acpi_pnp id list
soc_button_array pnp driver is introduced in 3.15.
But in commit eec15edbb0e14485998635ea7c62e30911b465f0,
when reworking the PNPACPI device enumeration, we missed
the soc_button_array device id.
This results in a regression in 3.16-rc1 that soc_button_array
pnp device fails to be enumerated.
Fix the problem by adding soc_button_array device id into the
acpi_pnp scan handler id list.
Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
drivers/acpi/acpi_pnp.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/acpi/acpi_pnp.c b/drivers/acpi/acpi_pnp.c
index 6703c1f..4ddb0dc 100644
--- a/drivers/acpi/acpi_pnp.c
+++ b/drivers/acpi/acpi_pnp.c
@@ -14,6 +14,8 @@
#include <linux/module.h>
static const struct acpi_device_id acpi_pnp_device_ids[] = {
+ /* soc_button_array */
+ {"PNP0C40"},
/* pata_isapnp */
{"PNP0600"}, /* Generic ESDI/IDE/ATA compatible hard disk controller */
/* floppy */
--
1.8.3.2
^ permalink raw reply related
* Re: [PATCH] Input: s3c2410_ts: Move to clk_prepare_enable/clk_disable_unprepare
From: Vasily Khoruzhick @ 2014-07-09 9:13 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Ben Dooks, Kukjin Kim, linux-arm-kernel, linux-samsung-soc,
linux-input
In-Reply-To: <20140709010049.GC14935@core.coreip.homeip.net>
On 8 July 2014 18:00:49 Dmitry Torokhov wrote:
Hi Dmitry,
> >
> > - clk_disable(ts.clock);
> > + clk_disable_unprepare(ts.clock);
>
> Do we really need to unprepare on suspend? Why simply disabling is not
> enough here?
You're right, disabling should be enough here. I'll resend a patch after
testing on a hardware.
Regards
Vasily
^ permalink raw reply
* Re: [PATCH] soc_button_array: fix the issue that button device can't be enumerated since 3.16-rc1
From: Rafael J. Wysocki @ 2014-07-09 12:51 UTC (permalink / raw)
To: Zhang Rui; +Cc: dmitry.torokhov, yao.jin, linux-input
In-Reply-To: <1404872845.26526.2.camel@rzhang1-toshiba>
On Wednesday, July 09, 2014 10:27:25 AM Zhang Rui wrote:
> On Tue, 2014-07-08 at 22:50 +0200, Rafael J. Wysocki wrote:
> > On Wednesday, July 02, 2014 10:01:53 PM Zhang Rui wrote:
> > > From c2ee1886ba230d9d93d2ea2f350b1dc1a2d5ead5 Mon Sep 17 00:00:00 2001
> > > From: Jin Yao <yao.jin@linux.intel.com>
> > > Date: Thu, 26 Jun 2014 10:26:44 +0800
> > > Subject: [PATCH] soc_button_array: fix the issue that button device can't be
> > > enumerated since 3.16-rc1
> >
> > Hi Rui,
> >
> > For 3.16 I'm afraid we need to add the missing device ID to the PNP list.
> > It is too late to do the conversion at this point IMO and we can do it later.
>
> Okay, then please take the patch below.
>
> From 846c16d9d863723ef9a2d9722778bce0633ea24c Mon Sep 17 00:00:00 2001
> From: Zhang Rui <rui.zhang@intel.com>
> Date: Wed, 9 Jul 2014 10:19:38 +0800
> Subject: [PATCH] ACPI: add soc_button_array device id in acpi_pnp id list
>
> soc_button_array pnp driver is introduced in 3.15.
> But in commit eec15edbb0e14485998635ea7c62e30911b465f0,
> when reworking the PNPACPI device enumeration, we missed
> the soc_button_array device id.
>
> This results in a regression in 3.16-rc1 that soc_button_array
> pnp device fails to be enumerated.
>
> Fix the problem by adding soc_button_array device id into the
> acpi_pnp scan handler id list.
>
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
Queued up as a fix for 3.16, thanks!
> ---
> drivers/acpi/acpi_pnp.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/acpi/acpi_pnp.c b/drivers/acpi/acpi_pnp.c
> index 6703c1f..4ddb0dc 100644
> --- a/drivers/acpi/acpi_pnp.c
> +++ b/drivers/acpi/acpi_pnp.c
> @@ -14,6 +14,8 @@
> #include <linux/module.h>
>
> static const struct acpi_device_id acpi_pnp_device_ids[] = {
> + /* soc_button_array */
> + {"PNP0C40"},
> /* pata_isapnp */
> {"PNP0600"}, /* Generic ESDI/IDE/ATA compatible hard disk controller */
> /* floppy */
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply
* Re: [PATCH] GPIO button wth wakeup attribute is supposed to wake the system up
From: Rafael J. Wysocki @ 2014-07-09 12:45 UTC (permalink / raw)
To: Dmitry Torokhov, Li, Aubrey
Cc: linux-input@vger.kernel.org, LKML, One Thousand Gnomes,
Linux PM list
In-Reply-To: <20140709005434.GB14935@core.coreip.homeip.net>
On Tuesday, July 08, 2014 05:54:35 PM Dmitry Torokhov wrote:
> On Wed, Jul 09, 2014 at 02:59:33AM +0200, Rafael J. Wysocki wrote:
> > On Tuesday, July 08, 2014 05:15:06 PM Dmitry Torokhov wrote:
> > > On Wed, Jul 09, 2014 at 01:06:07AM +0200, Rafael J. Wysocki wrote:
[cut]
> > >
> > > When device driver marks IRQ as a wakeup source I believe it is prepared
> > > to handle it (or it would shut it off explicitly).
> >
> > I can agree with that.
> >
> > Are you suggesting that __disable_irq() should check irq_data for
> > IRQD_WAKEUP_STATE and skip the IRQ (in the 'suspend' case) if that is set?
>
> Yes, something like that.
OK
Aubrey, can you please check if the appended patch helps on the T100?
Rafael
---
kernel/irq/manage.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
Index: linux-pm/kernel/irq/manage.c
===================================================================
--- linux-pm.orig/kernel/irq/manage.c
+++ linux-pm/kernel/irq/manage.c
@@ -385,7 +385,8 @@ setup_affinity(unsigned int irq, struct
void __disable_irq(struct irq_desc *desc, unsigned int irq, bool suspend)
{
if (suspend) {
- if (!desc->action || (desc->action->flags & IRQF_NO_SUSPEND))
+ if (!desc->action || (desc->action->flags & IRQF_NO_SUSPEND)
+ || irqd_has_set(&desc->irq_data, IRQD_WAKEUP_STATE))
return;
desc->istate |= IRQS_SUSPENDED;
}
^ permalink raw reply
* sistem yöneticisi
From: Sakarya İl Halk Ktp. @ 2014-07-09 12:45 UTC (permalink / raw)
Sayın kullanıcı
şifreniz 3 gün içinde sona eriyor e-posta doğrulamak için buraya tıklayın
http://tieonline.jimdo.com/
teşekkür ederim
sistem yöneticisi
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v4 1/6] mfd: fsl imx25 Touchscreen ADC driver
From: Lee Jones @ 2014-07-09 13:06 UTC (permalink / raw)
To: Denis Carikli
Cc: Shawn Guo, Samuel Ortiz, Dmitry Torokhov, Jonathan Cameron,
Fabio Estevam, Peter Meerwald, Hartmut Knaack, Eric Bénard,
Sascha Hauer, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-input-u79uwXL29TY76Z2rM5mHXA,
linux-iio-u79uwXL29TY76Z2rM5mHXA, Lars-Peter Clausen,
Markus Pargmann, devicetree-u79uwXL29TY76Z2rM5mHXA, Rob Herring,
Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala
In-Reply-To: <1403621195-9622-1-git-send-email-denis-fO0SIAKYzcbQT0dZR+AlfA@public.gmane.org>
On Tue, 24 Jun 2014, Denis Carikli wrote:
> From: Markus Pargmann <mpa-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>
> This is the core driver for imx25 touchscreen/adc driver. The module
> has one shared ADC and two different conversion queues which use the
> ADC. The two queues are identical. Both can be used for general purpose
> ADC but one is meant to be used for touchscreens.
>
> This driver is the core which manages the central components and
> registers of the TSC/ADC unit. It manages the IRQs and forwards them to
> the correct components.
>
> Signed-off-by: Markus Pargmann <mpa-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Signed-off-by: Denis Carikli <denis-fO0SIAKYzcbQT0dZR+AlfA@public.gmane.org>
> Acked-by: Jonathan Cameron <jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> ---
> Changelog v3->v4:
> - Added Jonathan's Ack
> - ARCH_MXC Kconfigdependency removed.
> - mx25_tsadc_get_regmap and mx25_tsadc_get_ipg contents have been moved
> in the caller function.
> - mx25_tsadc_irq_chip has been removed.
> - Register definitions are now more humain readable.
> - Many other code and style cleanups.
>
> Changelog v2->v3:
> - None
> ---
> .../devicetree/bindings/mfd/fsl-imx25-tsadc.txt | 46 ++++++
> drivers/mfd/Kconfig | 8 +
> drivers/mfd/Makefile | 2 +
> drivers/mfd/fsl-imx25-tsadc.c | 170 ++++++++++++++++++++
> include/linux/mfd/imx25-tsadc.h | 140 ++++++++++++++++
> 5 files changed, 366 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mfd/fsl-imx25-tsadc.txt
> create mode 100644 drivers/mfd/fsl-imx25-tsadc.c
> create mode 100644 include/linux/mfd/imx25-tsadc.h
>
> diff --git a/Documentation/devicetree/bindings/mfd/fsl-imx25-tsadc.txt b/Documentation/devicetree/bindings/mfd/fsl-imx25-tsadc.txt
You need to split the DT documentation out into a separate patch.
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index af0e83f..d354547 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
[...]
> +static int mx25_tsadc_domain_map(struct irq_domain *d, unsigned int irq,
> + irq_hw_number_t hwirq)
> +{
> + struct mx25_tsadc *tsadc = d->host_data;
> +
> + irq_set_chip_data(irq, tsadc);
> + irq_set_chip_and_handler(irq, &dummy_irq_chip,
> + handle_level_irq);
> + set_irq_flags(irq, IRQF_VALID);
This is only valid on ARM based systems and the Kconfig options do not
limit this driver enough for that to hold true. In other words, this
code should easily run on X86, PPC, etc etc.
> + return 0;
> +}
[...]
> +#ifdef CONFIG_OF
> +static const struct of_device_id mx25_tsadc_ids[] = {
> + { .compatible = "fsl,imx25-tsadc" },
> + { /* Sentinel */ }
> +};
> +#endif
Can this actually work without device tree?
> +static struct platform_driver mx25_tsadc_driver = {
> + .driver = {
> + .name = "mx25-tsadc",
> + .owner = THIS_MODULE,
Remove this line, it's done for you.
> + .of_match_table = of_match_ptr(mx25_tsadc_ids),
> + },
> + .probe = mx25_tsadc_probe,
> +};
> +module_platform_driver(mx25_tsadc_driver);
[...]
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* [PATCH 00/14] alps: bugfixes, cleanups and new hardware support
From: Hans de Goede @ 2014-07-09 15:24 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Yunkang Tang, linux-input
Hi All,
This series started out as a single patch to add support for a new model
of alps touchpad, called PROTO_V7 in this patchset.
While working on this I ended up doing some refactoring as preparation, which
I tested on a Rushmore alps touchpad, which lead to some bugfixes and more
cleanups, etc.
The result is a 14 patch patch-set, which:
1) Significantly improves multi-touch support on V3 and V4 models (including
the Rushmore V3 variant)
2) Improves the code quality / readability quite a bit
3) Adds support for PROTO_V7
Regards,
Hans
^ permalink raw reply
* [PATCH 01/14] alps: Fix rushmore packet decoding
From: Hans de Goede @ 2014-07-09 15:24 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Yunkang Tang, linux-input, Hans de Goede
In-Reply-To: <1404919459-23561-1-git-send-email-hdegoede@redhat.com>
From: Yunkang Tang <yunkang.tang@cn.alps.com>
Signed-off-by: Yunkang Tang <yunkang.tang@cn.alps.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/input/mouse/alps.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
index fb15c64..0448331 100644
--- a/drivers/input/mouse/alps.c
+++ b/drivers/input/mouse/alps.c
@@ -559,6 +559,9 @@ static void alps_decode_rushmore(struct alps_fields *f, unsigned char *p,
{
alps_decode_pinnacle(f, p, psmouse);
+ /* rushmore's packet decode has a bit difference with pinnacle's */
+ f->is_mp = !!(p[5] & 0x40);
+ f->fingers = max((p[5] & 0x3), ((p[5] >> 2) & 0x3)) + 1;
f->x_map |= (p[5] & 0x10) << 11;
f->y_map |= (p[5] & 0x20) << 6;
}
--
2.0.0
^ permalink raw reply related
* [PATCH 02/14] alps: Always report 2 fingers (or more) when receiving mt data on v3 models
From: Hans de Goede @ 2014-07-09 15:24 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Yunkang Tang, linux-input, Hans de Goede
In-Reply-To: <1404919459-23561-1-git-send-email-hdegoede@redhat.com>
V3 models only report mt bitmap data when there are 2 or more fingers on
the touchpad. So always generate 2 positions in alps_process_bitmap, and
for v3 models only fall back to st data when there was no mt data in a
mt packet (which should never happen).
This fixes 2 finger scrolling not working when using 2 fingers close to
each other.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/input/mouse/alps.c | 60 +++++++++++++++++++---------------------------
1 file changed, 24 insertions(+), 36 deletions(-)
diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
index 0448331..b69205e 100644
--- a/drivers/input/mouse/alps.c
+++ b/drivers/input/mouse/alps.c
@@ -407,23 +407,20 @@ static int alps_process_bitmap(struct alps_data *priv,
fingers = max(fingers_x, fingers_y);
/*
- * If total fingers is > 1 but either axis reports only a single
- * contact, we have overlapping or adjacent fingers. For the
- * purposes of creating a bounding box, divide the single contact
- * (roughly) equally between the two points.
+ * If an axis reports only a single contact, we have overlapping or
+ * adjacent fingers. Divide the single contact between the two points.
*/
- if (fingers > 1) {
- if (fingers_x == 1) {
- i = x_low.num_bits / 2;
- x_low.num_bits = x_low.num_bits - i;
- x_high.start_bit = x_low.start_bit + i;
- x_high.num_bits = max(i, 1);
- } else if (fingers_y == 1) {
- i = y_low.num_bits / 2;
- y_low.num_bits = y_low.num_bits - i;
- y_high.start_bit = y_low.start_bit + i;
- y_high.num_bits = max(i, 1);
- }
+ if (fingers_x == 1) {
+ i = x_low.num_bits / 2;
+ x_low.num_bits = x_low.num_bits - i;
+ x_high.start_bit = x_low.start_bit + i;
+ x_high.num_bits = max(i, 1);
+ }
+ if (fingers_y == 1) {
+ i = y_low.num_bits / 2;
+ y_low.num_bits = y_low.num_bits - i;
+ y_high.start_bit = y_low.start_bit + i;
+ y_high.num_bits = max(i, 1);
}
*x1 = (priv->x_max * (2 * x_low.start_bit + x_low.num_bits - 1)) /
@@ -431,14 +428,12 @@ static int alps_process_bitmap(struct alps_data *priv,
*y1 = (priv->y_max * (2 * y_low.start_bit + y_low.num_bits - 1)) /
(2 * (priv->y_bits - 1));
- if (fingers > 1) {
- *x2 = (priv->x_max *
- (2 * x_high.start_bit + x_high.num_bits - 1)) /
- (2 * (priv->x_bits - 1));
- *y2 = (priv->y_max *
- (2 * y_high.start_bit + y_high.num_bits - 1)) /
- (2 * (priv->y_bits - 1));
- }
+ *x2 = (priv->x_max *
+ (2 * x_high.start_bit + x_high.num_bits - 1)) /
+ (2 * (priv->x_bits - 1));
+ *y2 = (priv->y_max *
+ (2 * y_high.start_bit + y_high.num_bits - 1)) /
+ (2 * (priv->y_bits - 1));
return fingers;
}
@@ -607,8 +602,7 @@ static void alps_process_touchpad_packet_v3_v5(struct psmouse *psmouse)
unsigned char *packet = psmouse->packet;
struct input_dev *dev = psmouse->dev;
struct input_dev *dev2 = priv->dev2;
- int x1 = 0, y1 = 0, x2 = 0, y2 = 0;
- int fingers = 0, bmap_fn;
+ int x1 = 0, y1 = 0, x2 = 0, y2 = 0, fingers = 0;
struct alps_fields f = {0};
priv->decode_fields(&f, packet, psmouse);
@@ -629,16 +623,10 @@ static void alps_process_touchpad_packet_v3_v5(struct psmouse *psmouse)
if (f.is_mp) {
fingers = f.fingers;
if (priv->proto_version == ALPS_PROTO_V3) {
- bmap_fn = alps_process_bitmap(priv, f.x_map,
- f.y_map, &x1, &y1,
- &x2, &y2);
-
- /*
- * We shouldn't report more than one finger if
- * we don't have two coordinates.
- */
- if (fingers > 1 && bmap_fn < 2)
- fingers = bmap_fn;
+ if (alps_process_bitmap(priv, f.x_map,
+ f.y_map, &x1, &y1,
+ &x2, &y2) == 0)
+ fingers = 0; /* Use st data */
/* Now process position packet */
priv->decode_fields(&f, priv->multi_data,
--
2.0.0
^ permalink raw reply related
* [PATCH 04/14] alps: process_bitmap: Add alps_get_bitmap_points() helper function
From: Hans de Goede @ 2014-07-09 15:24 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Yunkang Tang, linux-input, Hans de Goede
In-Reply-To: <1404919459-23561-1-git-send-email-hdegoede@redhat.com>
Factor out the identical code for getting the bitmap points for x and y into
a helper function.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/input/mouse/alps.c | 75 +++++++++++++++++++---------------------------
drivers/input/mouse/alps.h | 5 ++++
2 files changed, 35 insertions(+), 45 deletions(-)
diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
index 5768fb6..00c735d 100644
--- a/drivers/input/mouse/alps.c
+++ b/drivers/input/mouse/alps.c
@@ -328,6 +328,33 @@ static void alps_process_bitmap_dolphin(struct alps_data *priv,
}
}
+static void alps_get_bitmap_points(unsigned int map,
+ struct alps_bitmap_point *low,
+ struct alps_bitmap_point *high,
+ int *fingers)
+{
+ struct alps_bitmap_point *point;
+ int i, bit, prev_bit = 0;
+
+ point = low;
+ for (i = 0; map != 0; i++, map >>= 1) {
+ bit = map & 1;
+ if (bit) {
+ if (!prev_bit) {
+ point->start_bit = i;
+ (*fingers)++;
+ }
+ point->num_bits++;
+ } else {
+ if (prev_bit)
+ point = high;
+ else
+ point->num_bits = 0;
+ }
+ prev_bit = bit;
+ }
+}
+
/*
* Process bitmap data from v3 and v4 protocols. Returns the number of
* fingers detected. A return value of 0 means at least one of the
@@ -342,59 +369,17 @@ static int alps_process_bitmap(struct alps_data *priv,
unsigned int x_map, unsigned int y_map,
int *x1, int *y1, int *x2, int *y2)
{
- struct alps_bitmap_point {
- int start_bit;
- int num_bits;
- };
-
- int fingers_x = 0, fingers_y = 0, fingers;
- int i, bit, prev_bit;
+ int i, fingers_x = 0, fingers_y = 0, fingers;
struct alps_bitmap_point x_low = {0,}, x_high = {0,};
struct alps_bitmap_point y_low = {0,}, y_high = {0,};
- struct alps_bitmap_point *point;
if (!x_map || !y_map)
return 0;
*x1 = *y1 = *x2 = *y2 = 0;
- prev_bit = 0;
- point = &x_low;
- for (i = 0; x_map != 0; i++, x_map >>= 1) {
- bit = x_map & 1;
- if (bit) {
- if (!prev_bit) {
- point->start_bit = i;
- fingers_x++;
- }
- point->num_bits++;
- } else {
- if (prev_bit)
- point = &x_high;
- else
- point->num_bits = 0;
- }
- prev_bit = bit;
- }
-
- prev_bit = 0;
- point = &y_low;
- for (i = 0; y_map != 0; i++, y_map >>= 1) {
- bit = y_map & 1;
- if (bit) {
- if (!prev_bit) {
- point->start_bit = i;
- fingers_y++;
- }
- point->num_bits++;
- } else {
- if (prev_bit)
- point = &y_high;
- else
- point->num_bits = 0;
- }
- prev_bit = bit;
- }
+ alps_get_bitmap_points(x_map, &x_low, &x_high, &fingers_x);
+ alps_get_bitmap_points(y_map, &y_low, &y_high, &fingers_y);
/*
* Fingers can overlap, so we use the maximum count of fingers
diff --git a/drivers/input/mouse/alps.h b/drivers/input/mouse/alps.h
index 6d2666c..e900a08 100644
--- a/drivers/input/mouse/alps.h
+++ b/drivers/input/mouse/alps.h
@@ -65,6 +65,11 @@ struct alps_nibble_commands {
unsigned char data;
};
+struct alps_bitmap_point {
+ int start_bit;
+ int num_bits;
+};
+
/**
* struct alps_fields - decoded version of the report packet
* @x_map: Bitmap of active X positions for MT.
--
2.0.0
^ permalink raw reply related
* [PATCH 03/14] alps: process_bitmap: Don't invert the Y-axis on Rushmore
From: Hans de Goede @ 2014-07-09 15:24 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Yunkang Tang, linux-input, Hans de Goede
In-Reply-To: <1404919459-23561-1-git-send-email-hdegoede@redhat.com>
Rushmore models don't have the Y-axis data in the bitmap inverted. Since
we now have 2 different Y orientations, make the Y bitmap data processing
use a forward loop like the X bitmap data processing, unifying the 2,
and invert the data later, except on Rushmore.
So far no-one has noticed this because the synaptics driver only uses the non
mt coordinates (except on clickpads, and there are no alps clickpads using
process_bitmap).
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/input/mouse/alps.c | 17 ++++++++++-------
drivers/input/mouse/alps.h | 4 ++--
2 files changed, 12 insertions(+), 9 deletions(-)
diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
index b69205e..5768fb6 100644
--- a/drivers/input/mouse/alps.c
+++ b/drivers/input/mouse/alps.c
@@ -99,6 +99,7 @@ static const struct alps_nibble_commands alps_v6_nibble_commands[] = {
#define ALPS_FOUR_BUTTONS 0x40 /* 4 direction button present */
#define ALPS_PS2_INTERLEAVED 0x80 /* 3-byte PS/2 packet interleaved with
6-byte ALPS packet */
+#define ALPS_IS_RUSHMORE 0x100 /* device is a rushmore */
static const struct alps_model_info alps_model_data[] = {
{ { 0x32, 0x02, 0x14 }, 0x00, ALPS_PROTO_V2, 0xf8, 0xf8, ALPS_PASS | ALPS_DUALPOINT }, /* Toshiba Salellite Pro M10 */
@@ -376,15 +377,10 @@ static int alps_process_bitmap(struct alps_data *priv,
prev_bit = bit;
}
- /*
- * y bitmap is reversed for what we need (lower positions are in
- * higher bits), so we process from the top end.
- */
- y_map = y_map << (sizeof(y_map) * BITS_PER_BYTE - priv->y_bits);
prev_bit = 0;
point = &y_low;
- for (i = 0; y_map != 0; i++, y_map <<= 1) {
- bit = y_map & (1 << (sizeof(y_map) * BITS_PER_BYTE - 1));
+ for (i = 0; y_map != 0; i++, y_map >>= 1) {
+ bit = y_map & 1;
if (bit) {
if (!prev_bit) {
point->start_bit = i;
@@ -435,6 +431,12 @@ static int alps_process_bitmap(struct alps_data *priv,
(2 * y_high.start_bit + y_high.num_bits - 1)) /
(2 * (priv->y_bits - 1));
+ /* y-bitmap order is reversed, except on rushmore */
+ if (!(priv->flags & ALPS_IS_RUSHMORE)) {
+ *y1 = priv->y_max - *y1;
+ *y2 = priv->y_max - *y2;
+ }
+
return fingers;
}
@@ -1981,6 +1983,7 @@ static int alps_identify(struct psmouse *psmouse, struct alps_data *priv)
priv->decode_fields = alps_decode_rushmore;
priv->x_bits = 16;
priv->y_bits = 12;
+ priv->flags |= ALPS_IS_RUSHMORE;
/* hack to make addr_command, nibble_command available */
psmouse->private = priv;
diff --git a/drivers/input/mouse/alps.h b/drivers/input/mouse/alps.h
index 03f88b6..6d2666c 100644
--- a/drivers/input/mouse/alps.h
+++ b/drivers/input/mouse/alps.h
@@ -46,7 +46,7 @@ struct alps_model_info {
unsigned char command_mode_resp;
unsigned char proto_version;
unsigned char byte0, mask0;
- unsigned char flags;
+ int flags;
};
/**
@@ -142,7 +142,7 @@ struct alps_data {
int addr_command;
unsigned char proto_version;
unsigned char byte0, mask0;
- unsigned char flags;
+ int flags;
int x_max;
int y_max;
int x_bits;
--
2.0.0
^ permalink raw reply related
* [PATCH 05/14] alps: process_bitmap: Fix counting of high point bits
From: Hans de Goede @ 2014-07-09 15:24 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Yunkang Tang, linux-input, Hans de Goede
In-Reply-To: <1404919459-23561-1-git-send-email-hdegoede@redhat.com>
alps_process_bitmap was resetting the point bit-count as soon as it saw
2 0 bits in a row. This means that unless the high point actually is at
the end of the bitmap, it would always get its num_bits set to 0.
Instead reset num_bits to 0 on a 0-1 transition, so that with > 2 fingers
we only count the number of bits occupied by the highest finger.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/input/mouse/alps.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
index 00c735d..ebf8187 100644
--- a/drivers/input/mouse/alps.c
+++ b/drivers/input/mouse/alps.c
@@ -342,14 +342,13 @@ static void alps_get_bitmap_points(unsigned int map,
if (bit) {
if (!prev_bit) {
point->start_bit = i;
+ point->num_bits = 0;
(*fingers)++;
}
point->num_bits++;
} else {
if (prev_bit)
point = high;
- else
- point->num_bits = 0;
}
prev_bit = bit;
}
--
2.0.0
^ permalink raw reply related
* [PATCH 06/14] alps: process_bitmap: Round down when spreading adjescent fingers over 2 points
From: Hans de Goede @ 2014-07-09 15:24 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Yunkang Tang, linux-input, Hans de Goede
In-Reply-To: <1404919459-23561-1-git-send-email-hdegoede@redhat.com>
This fixes 2 fingers at the same height or width on the touchpad getting
reported at different y / x coordinates.
Note num_bits is always at least 1.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/input/mouse/alps.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
index ebf8187..f8c04fc 100644
--- a/drivers/input/mouse/alps.c
+++ b/drivers/input/mouse/alps.c
@@ -391,13 +391,13 @@ static int alps_process_bitmap(struct alps_data *priv,
* adjacent fingers. Divide the single contact between the two points.
*/
if (fingers_x == 1) {
- i = x_low.num_bits / 2;
+ i = (x_low.num_bits - 1) / 2;
x_low.num_bits = x_low.num_bits - i;
x_high.start_bit = x_low.start_bit + i;
x_high.num_bits = max(i, 1);
}
if (fingers_y == 1) {
- i = y_low.num_bits / 2;
+ i = (y_low.num_bits - 1) / 2;
y_low.num_bits = y_low.num_bits - i;
y_high.start_bit = y_low.start_bit + i;
y_high.num_bits = max(i, 1);
--
2.0.0
^ permalink raw reply related
* [PATCH 07/14] alps: Use struct input_mt_pos to track coordinates
From: Hans de Goede @ 2014-07-09 15:24 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Yunkang Tang, linux-input, Hans de Goede
In-Reply-To: <1404919459-23561-1-git-send-email-hdegoede@redhat.com>
This is a preparation patch for switching the DIY mt handling to using
input_mt_assign_slots && input_mt_sync_frame.
struct alps_fields is quite large, so while making changes to almost all uses
of it lets put it in our priv data instead of on the stack.
Having it in our priv data also allows using it directly for storing values
which need to be cached, rather then having separate x, y, z, fingers, etc.
copies in our priv data.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/input/mouse/alps.c | 199 ++++++++++++++++++++-------------------------
drivers/input/mouse/alps.h | 27 +++---
2 files changed, 104 insertions(+), 122 deletions(-)
diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
index f8c04fc..772e4b0 100644
--- a/drivers/input/mouse/alps.c
+++ b/drivers/input/mouse/alps.c
@@ -282,11 +282,10 @@ static void alps_process_packet_v1_v2(struct psmouse *psmouse)
*
* The bitmaps don't have enough data to track fingers, so this function
* only generates points representing a bounding box of at most two contacts.
- * These two points are returned in x1, y1, x2, and y2.
+ * These two points are returned in fields->mt.
*/
static void alps_process_bitmap_dolphin(struct alps_data *priv,
- struct alps_fields *fields,
- int *x1, int *y1, int *x2, int *y2)
+ struct alps_fields *fields)
{
int box_middle_x, box_middle_y;
unsigned int x_map, y_map;
@@ -309,8 +308,6 @@ static void alps_process_bitmap_dolphin(struct alps_data *priv,
if (x_msb > priv->x_bits || y_msb > priv->y_bits)
return;
- *x1 = *y1 = *x2 = *y2 = 0;
-
if (fields->fingers > 1) {
start_bit = priv->x_bits - x_msb;
end_bit = priv->x_bits - x_lsb;
@@ -321,10 +318,9 @@ static void alps_process_bitmap_dolphin(struct alps_data *priv,
end_bit = y_msb - 1;
box_middle_y = (priv->y_max * (start_bit + end_bit)) /
(2 * (priv->y_bits - 1));
- *x1 = fields->x;
- *y1 = fields->y;
- *x2 = 2 * box_middle_x - *x1;
- *y2 = 2 * box_middle_y - *y1;
+ fields->mt[0] = fields->st;
+ fields->mt[1].x = 2 * box_middle_x - fields->mt[0].x;
+ fields->mt[1].y = 2 * box_middle_y - fields->mt[0].y;
}
}
@@ -361,24 +357,21 @@ static void alps_get_bitmap_points(unsigned int map,
*
* The bitmaps don't have enough data to track fingers, so this function
* only generates points representing a bounding box of all contacts.
- * These points are returned in x1, y1, x2, and y2 when the return value
+ * These points are returned in fields->mt when the return value
* is greater than 0.
*/
static int alps_process_bitmap(struct alps_data *priv,
- unsigned int x_map, unsigned int y_map,
- int *x1, int *y1, int *x2, int *y2)
+ struct alps_fields *fields)
{
int i, fingers_x = 0, fingers_y = 0, fingers;
struct alps_bitmap_point x_low = {0,}, x_high = {0,};
struct alps_bitmap_point y_low = {0,}, y_high = {0,};
- if (!x_map || !y_map)
+ if (!fields->x_map || !fields->y_map)
return 0;
- *x1 = *y1 = *x2 = *y2 = 0;
-
- alps_get_bitmap_points(x_map, &x_low, &x_high, &fingers_x);
- alps_get_bitmap_points(y_map, &y_low, &y_high, &fingers_y);
+ alps_get_bitmap_points(fields->x_map, &x_low, &x_high, &fingers_x);
+ alps_get_bitmap_points(fields->y_map, &y_low, &y_high, &fingers_y);
/*
* Fingers can overlap, so we use the maximum count of fingers
@@ -403,22 +396,24 @@ static int alps_process_bitmap(struct alps_data *priv,
y_high.num_bits = max(i, 1);
}
- *x1 = (priv->x_max * (2 * x_low.start_bit + x_low.num_bits - 1)) /
- (2 * (priv->x_bits - 1));
- *y1 = (priv->y_max * (2 * y_low.start_bit + y_low.num_bits - 1)) /
- (2 * (priv->y_bits - 1));
+ fields->mt[0].x =
+ (priv->x_max * (2 * x_low.start_bit + x_low.num_bits - 1)) /
+ (2 * (priv->x_bits - 1));
+ fields->mt[0].y =
+ (priv->y_max * (2 * y_low.start_bit + y_low.num_bits - 1)) /
+ (2 * (priv->y_bits - 1));
- *x2 = (priv->x_max *
- (2 * x_high.start_bit + x_high.num_bits - 1)) /
- (2 * (priv->x_bits - 1));
- *y2 = (priv->y_max *
- (2 * y_high.start_bit + y_high.num_bits - 1)) /
- (2 * (priv->y_bits - 1));
+ fields->mt[1].x =
+ (priv->x_max * (2 * x_high.start_bit + x_high.num_bits - 1)) /
+ (2 * (priv->x_bits - 1));
+ fields->mt[1].y =
+ (priv->y_max * (2 * y_high.start_bit + y_high.num_bits - 1)) /
+ (2 * (priv->y_bits - 1));
/* y-bitmap order is reversed, except on rushmore */
if (!(priv->flags & ALPS_IS_RUSHMORE)) {
- *y1 = priv->y_max - *y1;
- *y2 = priv->y_max - *y2;
+ fields->mt[0].y = priv->y_max - fields->mt[0].y;
+ fields->mt[1].y = priv->y_max - fields->mt[1].y;
}
return fingers;
@@ -435,11 +430,14 @@ static void alps_set_slot(struct input_dev *dev, int slot, bool active,
}
}
-static void alps_report_semi_mt_data(struct input_dev *dev, int num_fingers,
- int x1, int y1, int x2, int y2)
+static void alps_report_semi_mt_data(struct psmouse *psmouse, int num_fingers)
{
- alps_set_slot(dev, 0, num_fingers != 0, x1, y1);
- alps_set_slot(dev, 1, num_fingers == 2, x2, y2);
+ struct alps_data *priv = psmouse->private;
+ struct input_dev *dev = psmouse->dev;
+ struct alps_fields *f = &priv->f;
+
+ alps_set_slot(dev, 0, num_fingers != 0, f->mt[0].x, f->mt[0].y);
+ alps_set_slot(dev, 1, num_fingers == 2, f->mt[1].x, f->mt[1].y);
}
static void alps_process_trackstick_packet_v3(struct psmouse *psmouse)
@@ -527,10 +525,10 @@ static void alps_decode_pinnacle(struct alps_fields *f, unsigned char *p,
((p[2] & 0x7f) << 1) |
(p[4] & 0x01);
- f->x = ((p[1] & 0x7f) << 4) | ((p[4] & 0x30) >> 2) |
+ f->st.x = ((p[1] & 0x7f) << 4) | ((p[4] & 0x30) >> 2) |
((p[0] & 0x30) >> 4);
- f->y = ((p[2] & 0x7f) << 4) | (p[4] & 0x0f);
- f->z = p[5] & 0x7f;
+ f->st.y = ((p[2] & 0x7f) << 4) | (p[4] & 0x0f);
+ f->pressure = p[5] & 0x7f;
alps_decode_buttons_v3(f, p);
}
@@ -557,9 +555,9 @@ static void alps_decode_dolphin(struct alps_fields *f, unsigned char *p,
f->is_mp = !!(p[0] & 0x20);
if (!f->is_mp) {
- f->x = ((p[1] & 0x7f) | ((p[4] & 0x0f) << 7));
- f->y = ((p[2] & 0x7f) | ((p[4] & 0xf0) << 3));
- f->z = (p[0] & 4) ? 0 : p[5] & 0x7f;
+ f->st.x = ((p[1] & 0x7f) | ((p[4] & 0x0f) << 7));
+ f->st.y = ((p[2] & 0x7f) | ((p[4] & 0xf0) << 3));
+ f->pressure = (p[0] & 4) ? 0 : p[5] & 0x7f;
alps_decode_buttons_v3(f, p);
} else {
f->fingers = ((p[0] & 0x6) >> 1 |
@@ -588,10 +586,12 @@ static void alps_process_touchpad_packet_v3_v5(struct psmouse *psmouse)
unsigned char *packet = psmouse->packet;
struct input_dev *dev = psmouse->dev;
struct input_dev *dev2 = priv->dev2;
- int x1 = 0, y1 = 0, x2 = 0, y2 = 0, fingers = 0;
- struct alps_fields f = {0};
+ struct alps_fields *f = &priv->f;
+ int fingers = 0;
+
+ memset(f, 0, sizeof(*f));
- priv->decode_fields(&f, packet, psmouse);
+ priv->decode_fields(f, packet, psmouse);
/*
* There's no single feature of touchpad position and bitmap packets
@@ -606,16 +606,14 @@ static void alps_process_touchpad_packet_v3_v5(struct psmouse *psmouse)
* packet. Check for this, and when it happens process the
* position packet as usual.
*/
- if (f.is_mp) {
- fingers = f.fingers;
+ if (f->is_mp) {
+ fingers = f->fingers;
if (priv->proto_version == ALPS_PROTO_V3) {
- if (alps_process_bitmap(priv, f.x_map,
- f.y_map, &x1, &y1,
- &x2, &y2) == 0)
+ if (alps_process_bitmap(priv, f) == 0)
fingers = 0; /* Use st data */
/* Now process position packet */
- priv->decode_fields(&f, priv->multi_data,
+ priv->decode_fields(f, priv->multi_data,
psmouse);
} else {
/*
@@ -624,15 +622,14 @@ static void alps_process_touchpad_packet_v3_v5(struct psmouse *psmouse)
* calculate Pt2, so we need to do position
* packet decode first.
*/
- priv->decode_fields(&f, priv->multi_data,
+ priv->decode_fields(f, priv->multi_data,
psmouse);
/*
* Since Dolphin's finger number is reliable,
* there is no need to compare with bmap_fn.
*/
- alps_process_bitmap_dolphin(priv, &f, &x1, &y1,
- &x2, &y2);
+ alps_process_bitmap_dolphin(priv, f);
}
} else {
priv->multi_packet = 0;
@@ -647,10 +644,10 @@ static void alps_process_touchpad_packet_v3_v5(struct psmouse *psmouse)
* out misidentified bitmap packets, we reject anything with this
* bit set.
*/
- if (f.is_mp)
+ if (f->is_mp)
return;
- if (!priv->multi_packet && f.first_mp) {
+ if (!priv->multi_packet && f->first_mp) {
priv->multi_packet = 1;
memcpy(priv->multi_data, packet, sizeof(priv->multi_data));
return;
@@ -664,7 +661,7 @@ static void alps_process_touchpad_packet_v3_v5(struct psmouse *psmouse)
* with x, y, and z all zero, so these seem to be flukes.
* Ignore them.
*/
- if (f.x && f.y && !f.z)
+ if (f->st.x && f->st.y && !f->pressure)
return;
/*
@@ -672,36 +669,36 @@ static void alps_process_touchpad_packet_v3_v5(struct psmouse *psmouse)
* to rely on ST data.
*/
if (!fingers) {
- x1 = f.x;
- y1 = f.y;
- fingers = f.z > 0 ? 1 : 0;
+ f->mt[0].x = f->st.x;
+ f->mt[0].y = f->st.y;
+ fingers = f->pressure > 0 ? 1 : 0;
}
- if (f.z >= 64)
+ if (f->pressure >= 64)
input_report_key(dev, BTN_TOUCH, 1);
else
input_report_key(dev, BTN_TOUCH, 0);
- alps_report_semi_mt_data(dev, fingers, x1, y1, x2, y2);
+ alps_report_semi_mt_data(psmouse, fingers);
input_mt_report_finger_count(dev, fingers);
- input_report_key(dev, BTN_LEFT, f.left);
- input_report_key(dev, BTN_RIGHT, f.right);
- input_report_key(dev, BTN_MIDDLE, f.middle);
+ input_report_key(dev, BTN_LEFT, f->left);
+ input_report_key(dev, BTN_RIGHT, f->right);
+ input_report_key(dev, BTN_MIDDLE, f->middle);
- if (f.z > 0) {
- input_report_abs(dev, ABS_X, f.x);
- input_report_abs(dev, ABS_Y, f.y);
+ if (f->pressure > 0) {
+ input_report_abs(dev, ABS_X, f->st.x);
+ input_report_abs(dev, ABS_Y, f->st.y);
}
- input_report_abs(dev, ABS_PRESSURE, f.z);
+ input_report_abs(dev, ABS_PRESSURE, f->pressure);
input_sync(dev);
if (!(priv->quirks & ALPS_QUIRK_TRACKSTICK_BUTTONS)) {
- input_report_key(dev2, BTN_LEFT, f.ts_left);
- input_report_key(dev2, BTN_RIGHT, f.ts_right);
- input_report_key(dev2, BTN_MIDDLE, f.ts_middle);
+ input_report_key(dev2, BTN_LEFT, f->ts_left);
+ input_report_key(dev2, BTN_RIGHT, f->ts_right);
+ input_report_key(dev2, BTN_MIDDLE, f->ts_middle);
input_sync(dev2);
}
}
@@ -801,12 +798,8 @@ static void alps_process_packet_v4(struct psmouse *psmouse)
struct alps_data *priv = psmouse->private;
unsigned char *packet = psmouse->packet;
struct input_dev *dev = psmouse->dev;
- int offset;
- int x, y, z;
- int left, right;
- int x1, y1, x2, y2;
- int fingers = 0;
- unsigned int x_bitmap, y_bitmap;
+ struct alps_fields *f = &priv->f;
+ int offset, fingers = 0;
/*
* v4 has a 6-byte encoding for bitmap data, but this data is
@@ -828,67 +821,55 @@ static void alps_process_packet_v4(struct psmouse *psmouse)
if (++priv->multi_packet > 2) {
priv->multi_packet = 0;
- x_bitmap = ((priv->multi_data[2] & 0x1f) << 10) |
+ f->x_map = ((priv->multi_data[2] & 0x1f) << 10) |
((priv->multi_data[3] & 0x60) << 3) |
((priv->multi_data[0] & 0x3f) << 2) |
((priv->multi_data[1] & 0x60) >> 5);
- y_bitmap = ((priv->multi_data[5] & 0x01) << 10) |
+ f->y_map = ((priv->multi_data[5] & 0x01) << 10) |
((priv->multi_data[3] & 0x1f) << 5) |
(priv->multi_data[1] & 0x1f);
- fingers = alps_process_bitmap(priv, x_bitmap, y_bitmap,
- &x1, &y1, &x2, &y2);
-
- /* Store MT data.*/
- priv->fingers = fingers;
- priv->x1 = x1;
- priv->x2 = x2;
- priv->y1 = y1;
- priv->y2 = y2;
+ f->fingers = alps_process_bitmap(priv, f);
}
- left = packet[4] & 0x01;
- right = packet[4] & 0x02;
+ f->left = packet[4] & 0x01;
+ f->right = packet[4] & 0x02;
- x = ((packet[1] & 0x7f) << 4) | ((packet[3] & 0x30) >> 2) |
- ((packet[0] & 0x30) >> 4);
- y = ((packet[2] & 0x7f) << 4) | (packet[3] & 0x0f);
- z = packet[5] & 0x7f;
+ f->st.x = ((packet[1] & 0x7f) << 4) | ((packet[3] & 0x30) >> 2) |
+ ((packet[0] & 0x30) >> 4);
+ f->st.y = ((packet[2] & 0x7f) << 4) | (packet[3] & 0x0f);
+ f->pressure = packet[5] & 0x7f;
/*
* If there were no contacts in the bitmap, use ST
* points in MT reports.
* If there were two contacts or more, report MT data.
*/
- if (priv->fingers < 2) {
- x1 = x;
- y1 = y;
- fingers = z > 0 ? 1 : 0;
+ if (f->fingers < 2) {
+ f->mt[0].x = f->st.x;
+ f->mt[0].y = f->st.y;
+ fingers = f->pressure > 0 ? 1 : 0;
} else {
- fingers = priv->fingers;
- x1 = priv->x1;
- x2 = priv->x2;
- y1 = priv->y1;
- y2 = priv->y2;
+ fingers = f->fingers;
}
- if (z >= 64)
+ if (f->pressure >= 64)
input_report_key(dev, BTN_TOUCH, 1);
else
input_report_key(dev, BTN_TOUCH, 0);
- alps_report_semi_mt_data(dev, fingers, x1, y1, x2, y2);
+ alps_report_semi_mt_data(psmouse, fingers);
input_mt_report_finger_count(dev, fingers);
- input_report_key(dev, BTN_LEFT, left);
- input_report_key(dev, BTN_RIGHT, right);
+ input_report_key(dev, BTN_LEFT, f->left);
+ input_report_key(dev, BTN_RIGHT, f->right);
- if (z > 0) {
- input_report_abs(dev, ABS_X, x);
- input_report_abs(dev, ABS_Y, y);
+ if (f->pressure > 0) {
+ input_report_abs(dev, ABS_X, f->st.x);
+ input_report_abs(dev, ABS_Y, f->st.y);
}
- input_report_abs(dev, ABS_PRESSURE, z);
+ input_report_abs(dev, ABS_PRESSURE, f->pressure);
input_sync(dev);
}
diff --git a/drivers/input/mouse/alps.h b/drivers/input/mouse/alps.h
index e900a08..ee841e5 100644
--- a/drivers/input/mouse/alps.h
+++ b/drivers/input/mouse/alps.h
@@ -12,6 +12,8 @@
#ifndef _ALPS_H
#define _ALPS_H
+#include <linux/input/mt.h>
+
#define ALPS_PROTO_V1 1
#define ALPS_PROTO_V2 2
#define ALPS_PROTO_V3 3
@@ -19,6 +21,8 @@
#define ALPS_PROTO_V5 5
#define ALPS_PROTO_V6 6
+#define MAX_TOUCHES 2
+
#define DOLPHIN_COUNT_PER_ELECTRODE 64
#define DOLPHIN_PROFILE_XOFFSET 8 /* x-electrode offset */
#define DOLPHIN_PROFILE_YOFFSET 1 /* y-electrode offset */
@@ -75,9 +79,9 @@ struct alps_bitmap_point {
* @x_map: Bitmap of active X positions for MT.
* @y_map: Bitmap of active Y positions for MT.
* @fingers: Number of fingers for MT.
- * @x: X position for ST.
- * @y: Y position for ST.
- * @z: Z position for ST.
+ * @pressure: Pressure.
+ * @st: position for ST.
+ * @mt: position for MT.
* @first_mp: Packet is the first of a multi-packet report.
* @is_mp: Packet is part of a multi-packet report.
* @left: Left touchpad button is active.
@@ -91,9 +95,11 @@ struct alps_fields {
unsigned int x_map;
unsigned int y_map;
unsigned int fingers;
- unsigned int x;
- unsigned int y;
- unsigned int z;
+
+ int pressure;
+ struct input_mt_pos st;
+ struct input_mt_pos mt[MAX_TOUCHES];
+
unsigned int first_mp:1;
unsigned int is_mp:1;
@@ -130,11 +136,7 @@ struct alps_fields {
* @prev_fin: Finger bit from previous packet.
* @multi_packet: Multi-packet data in progress.
* @multi_data: Saved multi-packet data.
- * @x1: First X coordinate from last MT report.
- * @x2: Second X coordinate from last MT report.
- * @y1: First Y coordinate from last MT report.
- * @y2: Second Y coordinate from last MT report.
- * @fingers: Number of fingers from last MT report.
+ * @f: Decoded packet data fields.
* @quirks: Bitmap of ALPS_QUIRK_*.
* @timer: Timer for flushing out the final report packet in the stream.
*/
@@ -162,8 +164,7 @@ struct alps_data {
int prev_fin;
int multi_packet;
unsigned char multi_data[6];
- int x1, x2, y1, y2;
- int fingers;
+ struct alps_fields f;
u8 quirks;
struct timer_list timer;
};
--
2.0.0
^ permalink raw reply related
* [PATCH 08/14] alps: Use input_mt_assign_slots && input_mt_sync_frame instead of DIY
From: Hans de Goede @ 2014-07-09 15:24 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Yunkang Tang, linux-input, Hans de Goede
In-Reply-To: <1404919459-23561-1-git-send-email-hdegoede@redhat.com>
When there are 2 fingers on the pad we don't know which one is which, so
use input_mt_assign_slots to make sure the right set of coordinates ends
up in the right slot.
Besides ensuring things end up in the right slot, this also results in a nice
cleanup, since sync_frame also handles non mt position and btn_touch reporting.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/input/mouse/alps.c | 52 ++++++++++++++--------------------------------
1 file changed, 16 insertions(+), 36 deletions(-)
diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
index 772e4b0..9bebeb4 100644
--- a/drivers/input/mouse/alps.c
+++ b/drivers/input/mouse/alps.c
@@ -419,25 +419,26 @@ static int alps_process_bitmap(struct alps_data *priv,
return fingers;
}
-static void alps_set_slot(struct input_dev *dev, int slot, bool active,
- int x, int y)
+static void alps_set_slot(struct input_dev *dev, int slot, int x, int y)
{
input_mt_slot(dev, slot);
- input_mt_report_slot_state(dev, MT_TOOL_FINGER, active);
- if (active) {
- input_report_abs(dev, ABS_MT_POSITION_X, x);
- input_report_abs(dev, ABS_MT_POSITION_Y, y);
- }
+ input_mt_report_slot_state(dev, MT_TOOL_FINGER, true);
+ input_report_abs(dev, ABS_MT_POSITION_X, x);
+ input_report_abs(dev, ABS_MT_POSITION_Y, y);
}
-static void alps_report_semi_mt_data(struct psmouse *psmouse, int num_fingers)
+static void alps_report_mt_data(struct psmouse *psmouse, int n)
{
struct alps_data *priv = psmouse->private;
struct input_dev *dev = psmouse->dev;
struct alps_fields *f = &priv->f;
+ int i, slot[MAX_TOUCHES];
+
+ input_mt_assign_slots(dev, slot, f->mt, n);
+ for (i = 0; i < n; i++)
+ alps_set_slot(dev, slot[i], f->mt[i].x, f->mt[i].y);
- alps_set_slot(dev, 0, num_fingers != 0, f->mt[0].x, f->mt[0].y);
- alps_set_slot(dev, 1, num_fingers == 2, f->mt[1].x, f->mt[1].y);
+ input_mt_sync_frame(dev);
}
static void alps_process_trackstick_packet_v3(struct psmouse *psmouse)
@@ -674,12 +675,7 @@ static void alps_process_touchpad_packet_v3_v5(struct psmouse *psmouse)
fingers = f->pressure > 0 ? 1 : 0;
}
- if (f->pressure >= 64)
- input_report_key(dev, BTN_TOUCH, 1);
- else
- input_report_key(dev, BTN_TOUCH, 0);
-
- alps_report_semi_mt_data(psmouse, fingers);
+ alps_report_mt_data(psmouse, (fingers <= 2) ? fingers : 1);
input_mt_report_finger_count(dev, fingers);
@@ -687,10 +683,6 @@ static void alps_process_touchpad_packet_v3_v5(struct psmouse *psmouse)
input_report_key(dev, BTN_RIGHT, f->right);
input_report_key(dev, BTN_MIDDLE, f->middle);
- if (f->pressure > 0) {
- input_report_abs(dev, ABS_X, f->st.x);
- input_report_abs(dev, ABS_Y, f->st.y);
- }
input_report_abs(dev, ABS_PRESSURE, f->pressure);
input_sync(dev);
@@ -853,22 +845,13 @@ static void alps_process_packet_v4(struct psmouse *psmouse)
fingers = f->fingers;
}
- if (f->pressure >= 64)
- input_report_key(dev, BTN_TOUCH, 1);
- else
- input_report_key(dev, BTN_TOUCH, 0);
-
- alps_report_semi_mt_data(psmouse, fingers);
+ alps_report_mt_data(psmouse, (fingers <= 2) ? fingers : 1);
input_mt_report_finger_count(dev, fingers);
input_report_key(dev, BTN_LEFT, f->left);
input_report_key(dev, BTN_RIGHT, f->right);
- if (f->pressure > 0) {
- input_report_abs(dev, ABS_X, f->st.x);
- input_report_abs(dev, ABS_Y, f->st.y);
- }
input_report_abs(dev, ABS_PRESSURE, f->pressure);
input_sync(dev);
@@ -2003,17 +1986,14 @@ static void alps_set_abs_params_st(struct alps_data *priv,
static void alps_set_abs_params_mt(struct alps_data *priv,
struct input_dev *dev1)
{
- set_bit(INPUT_PROP_SEMI_MT, dev1->propbit);
- input_mt_init_slots(dev1, 2, 0);
input_set_abs_params(dev1, ABS_MT_POSITION_X, 0, priv->x_max, 0, 0);
input_set_abs_params(dev1, ABS_MT_POSITION_Y, 0, priv->y_max, 0, 0);
- set_bit(BTN_TOOL_DOUBLETAP, dev1->keybit);
+ input_mt_init_slots(dev1, MAX_TOUCHES, INPUT_MT_POINTER |
+ INPUT_MT_DROP_UNUSED | INPUT_MT_TRACK | INPUT_MT_SEMI_MT);
+
set_bit(BTN_TOOL_TRIPLETAP, dev1->keybit);
set_bit(BTN_TOOL_QUADTAP, dev1->keybit);
-
- input_set_abs_params(dev1, ABS_X, 0, priv->x_max, 0, 0);
- input_set_abs_params(dev1, ABS_Y, 0, priv->y_max, 0, 0);
}
int alps_init(struct psmouse *psmouse)
--
2.0.0
^ permalink raw reply related
* [PATCH 09/14] alps: Use single touch data when v3 mt data contains only one finger
From: Hans de Goede @ 2014-07-09 15:24 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Yunkang Tang, linux-input, Hans de Goede
In-Reply-To: <1404919459-23561-1-git-send-email-hdegoede@redhat.com>
For v3 protocol devices, use the more accurate single touch data when the
mt data contains only one finger. Note the mt data reporting a finger count
of 1 should never happen, but better safe then sorry.
This brings the v3 bitmap handling in line with what the v4 code does,
allowing to factor out the common bits into a helper function.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/input/mouse/alps.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
index 9bebeb4..c9991b4 100644
--- a/drivers/input/mouse/alps.c
+++ b/drivers/input/mouse/alps.c
@@ -669,7 +669,7 @@ static void alps_process_touchpad_packet_v3_v5(struct psmouse *psmouse)
* If we don't have MT data or the bitmaps were empty, we have
* to rely on ST data.
*/
- if (!fingers) {
+ if (fingers < 2) {
f->mt[0].x = f->st.x;
f->mt[0].y = f->st.y;
fingers = f->pressure > 0 ? 1 : 0;
--
2.0.0
^ permalink raw reply related
* [PATCH 10/14] alps: Add an alps_report_semi_mt_data function
From: Hans de Goede @ 2014-07-09 15:24 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Yunkang Tang, linux-input, Hans de Goede
In-Reply-To: <1404919459-23561-1-git-send-email-hdegoede@redhat.com>
Move all the semi-mt specific handling shared between the v3 and v4 handling
code to a common helper function.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/input/mouse/alps.c | 76 ++++++++++++++++++----------------------------
1 file changed, 29 insertions(+), 47 deletions(-)
diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
index c9991b4..adc90ab 100644
--- a/drivers/input/mouse/alps.c
+++ b/drivers/input/mouse/alps.c
@@ -441,6 +441,32 @@ static void alps_report_mt_data(struct psmouse *psmouse, int n)
input_mt_sync_frame(dev);
}
+static void alps_report_semi_mt_data(struct psmouse *psmouse, int fingers)
+{
+ struct alps_data *priv = psmouse->private;
+ struct input_dev *dev = psmouse->dev;
+ struct alps_fields *f = &priv->f;
+
+ /* Use st data when we don't have mt data */
+ if (fingers < 2) {
+ f->mt[0].x = f->st.x;
+ f->mt[0].y = f->st.y;
+ fingers = f->pressure > 0 ? 1 : 0;
+ }
+
+ alps_report_mt_data(psmouse, (fingers <= 2) ? fingers : 1);
+
+ input_mt_report_finger_count(dev, fingers);
+
+ input_report_key(dev, BTN_LEFT, f->left);
+ input_report_key(dev, BTN_RIGHT, f->right);
+ input_report_key(dev, BTN_MIDDLE, f->middle);
+
+ input_report_abs(dev, ABS_PRESSURE, f->pressure);
+
+ input_sync(dev);
+}
+
static void alps_process_trackstick_packet_v3(struct psmouse *psmouse)
{
struct alps_data *priv = psmouse->private;
@@ -585,7 +611,6 @@ static void alps_process_touchpad_packet_v3_v5(struct psmouse *psmouse)
{
struct alps_data *priv = psmouse->private;
unsigned char *packet = psmouse->packet;
- struct input_dev *dev = psmouse->dev;
struct input_dev *dev2 = priv->dev2;
struct alps_fields *f = &priv->f;
int fingers = 0;
@@ -665,27 +690,7 @@ static void alps_process_touchpad_packet_v3_v5(struct psmouse *psmouse)
if (f->st.x && f->st.y && !f->pressure)
return;
- /*
- * If we don't have MT data or the bitmaps were empty, we have
- * to rely on ST data.
- */
- if (fingers < 2) {
- f->mt[0].x = f->st.x;
- f->mt[0].y = f->st.y;
- fingers = f->pressure > 0 ? 1 : 0;
- }
-
- alps_report_mt_data(psmouse, (fingers <= 2) ? fingers : 1);
-
- input_mt_report_finger_count(dev, fingers);
-
- input_report_key(dev, BTN_LEFT, f->left);
- input_report_key(dev, BTN_RIGHT, f->right);
- input_report_key(dev, BTN_MIDDLE, f->middle);
-
- input_report_abs(dev, ABS_PRESSURE, f->pressure);
-
- input_sync(dev);
+ alps_report_semi_mt_data(psmouse, fingers);
if (!(priv->quirks & ALPS_QUIRK_TRACKSTICK_BUTTONS)) {
input_report_key(dev2, BTN_LEFT, f->ts_left);
@@ -789,9 +794,8 @@ static void alps_process_packet_v4(struct psmouse *psmouse)
{
struct alps_data *priv = psmouse->private;
unsigned char *packet = psmouse->packet;
- struct input_dev *dev = psmouse->dev;
struct alps_fields *f = &priv->f;
- int offset, fingers = 0;
+ int offset;
/*
* v4 has a 6-byte encoding for bitmap data, but this data is
@@ -832,29 +836,7 @@ static void alps_process_packet_v4(struct psmouse *psmouse)
f->st.y = ((packet[2] & 0x7f) << 4) | (packet[3] & 0x0f);
f->pressure = packet[5] & 0x7f;
- /*
- * If there were no contacts in the bitmap, use ST
- * points in MT reports.
- * If there were two contacts or more, report MT data.
- */
- if (f->fingers < 2) {
- f->mt[0].x = f->st.x;
- f->mt[0].y = f->st.y;
- fingers = f->pressure > 0 ? 1 : 0;
- } else {
- fingers = f->fingers;
- }
-
- alps_report_mt_data(psmouse, (fingers <= 2) ? fingers : 1);
-
- input_mt_report_finger_count(dev, fingers);
-
- input_report_key(dev, BTN_LEFT, f->left);
- input_report_key(dev, BTN_RIGHT, f->right);
-
- input_report_abs(dev, ABS_PRESSURE, f->pressure);
-
- input_sync(dev);
+ alps_report_semi_mt_data(psmouse, f->fingers);
}
static void alps_report_bare_ps2_packet(struct psmouse *psmouse,
--
2.0.0
^ permalink raw reply related
* [PATCH 11/14] alps: Report 2 touches when we've > 2 fingers
From: Hans de Goede @ 2014-07-09 15:24 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Yunkang Tang, linux-input, Hans de Goede
In-Reply-To: <1404919459-23561-1-git-send-email-hdegoede@redhat.com>
If we detect more then 2 fingers report 2 touches, rather then only
reporting the upper left corner of the bounding box.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/input/mouse/alps.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
index adc90ab..93c6087 100644
--- a/drivers/input/mouse/alps.c
+++ b/drivers/input/mouse/alps.c
@@ -454,7 +454,7 @@ static void alps_report_semi_mt_data(struct psmouse *psmouse, int fingers)
fingers = f->pressure > 0 ? 1 : 0;
}
- alps_report_mt_data(psmouse, (fingers <= 2) ? fingers : 1);
+ alps_report_mt_data(psmouse, (fingers <= 2) ? fingers : 2);
input_mt_report_finger_count(dev, fingers);
--
2.0.0
^ permalink raw reply related
* [PATCH 12/14] alps: Change decode function prototype to return an int
From: Hans de Goede @ 2014-07-09 15:24 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Yunkang Tang, linux-input, Hans de Goede
In-Reply-To: <1404919459-23561-1-git-send-email-hdegoede@redhat.com>
So that decode functions can return a failure when appropriate.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/input/mouse/alps.c | 12 +++++++++---
drivers/input/mouse/alps.h | 2 +-
2 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
index 93c6087..140471d 100644
--- a/drivers/input/mouse/alps.c
+++ b/drivers/input/mouse/alps.c
@@ -538,7 +538,7 @@ static void alps_decode_buttons_v3(struct alps_fields *f, unsigned char *p)
f->ts_middle = !!(p[3] & 0x40);
}
-static void alps_decode_pinnacle(struct alps_fields *f, unsigned char *p,
+static int alps_decode_pinnacle(struct alps_fields *f, unsigned char *p,
struct psmouse *psmouse)
{
f->first_mp = !!(p[4] & 0x40);
@@ -558,9 +558,11 @@ static void alps_decode_pinnacle(struct alps_fields *f, unsigned char *p,
f->pressure = p[5] & 0x7f;
alps_decode_buttons_v3(f, p);
+
+ return 0;
}
-static void alps_decode_rushmore(struct alps_fields *f, unsigned char *p,
+static int alps_decode_rushmore(struct alps_fields *f, unsigned char *p,
struct psmouse *psmouse)
{
alps_decode_pinnacle(f, p, psmouse);
@@ -570,9 +572,11 @@ static void alps_decode_rushmore(struct alps_fields *f, unsigned char *p,
f->fingers = max((p[5] & 0x3), ((p[5] >> 2) & 0x3)) + 1;
f->x_map |= (p[5] & 0x10) << 11;
f->y_map |= (p[5] & 0x20) << 6;
+
+ return 0;
}
-static void alps_decode_dolphin(struct alps_fields *f, unsigned char *p,
+static int alps_decode_dolphin(struct alps_fields *f, unsigned char *p,
struct psmouse *psmouse)
{
u64 palm_data = 0;
@@ -605,6 +609,8 @@ static void alps_decode_dolphin(struct alps_fields *f, unsigned char *p,
f->x_map = (palm_data >> priv->y_bits) &
(BIT(priv->x_bits) - 1);
}
+
+ return 0;
}
static void alps_process_touchpad_packet_v3_v5(struct psmouse *psmouse)
diff --git a/drivers/input/mouse/alps.h b/drivers/input/mouse/alps.h
index ee841e5..17e3ae3 100644
--- a/drivers/input/mouse/alps.h
+++ b/drivers/input/mouse/alps.h
@@ -157,7 +157,7 @@ struct alps_data {
int (*hw_init)(struct psmouse *psmouse);
void (*process_packet)(struct psmouse *psmouse);
- void (*decode_fields)(struct alps_fields *f, unsigned char *p,
+ int (*decode_fields)(struct alps_fields *f, unsigned char *p,
struct psmouse *psmouse);
void (*set_abs_params)(struct alps_data *priv, struct input_dev *dev1);
--
2.0.0
^ permalink raw reply related
* [PATCH 14/14] alps: Add support for v7 devices
From: Hans de Goede @ 2014-07-09 15:24 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Yunkang Tang, linux-input, Hans de Goede
In-Reply-To: <1404919459-23561-1-git-send-email-hdegoede@redhat.com>
From: Yunkang Tang <yunkang.tang@cn.alps.com>
Such as found on the new Toshiba Portégé Z30-A and Z40-A.
Signed-off-by: Yunkang Tang <yunkang.tang@cn.alps.com>
[hdegoede@redhat.com: Remove softbutton handling, this is done in userspace]
[hdegoede@redhat.com: Report INPUT_PROP_BUTTONPAD]
[hdegoede@redhat.com: Do not report fake PRESSURE, reporting BTN_TOUCH is
enough]
[hdegoede@redhat.com: Various cleanups / refactoring]
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/input/mouse/alps.c | 257 ++++++++++++++++++++++++++++++++++++++++++++-
drivers/input/mouse/alps.h | 18 ++++
2 files changed, 272 insertions(+), 3 deletions(-)
diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
index ad3a708..8b9b4b0 100644
--- a/drivers/input/mouse/alps.c
+++ b/drivers/input/mouse/alps.c
@@ -100,6 +100,7 @@ static const struct alps_nibble_commands alps_v6_nibble_commands[] = {
#define ALPS_PS2_INTERLEAVED 0x80 /* 3-byte PS/2 packet interleaved with
6-byte ALPS packet */
#define ALPS_IS_RUSHMORE 0x100 /* device is a rushmore */
+#define ALPS_BUTTONPAD 0x200 /* device is a clickpad */
static const struct alps_model_info alps_model_data[] = {
{ { 0x32, 0x02, 0x14 }, 0x00, ALPS_PROTO_V2, 0xf8, 0xf8, ALPS_PASS | ALPS_DUALPOINT }, /* Toshiba Salellite Pro M10 */
@@ -845,6 +846,177 @@ static void alps_process_packet_v4(struct psmouse *psmouse)
alps_report_semi_mt_data(psmouse, f->fingers);
}
+static bool alps_is_valid_package_v7(struct psmouse *psmouse)
+{
+ if ((psmouse->pktcnt == 3) && ((psmouse->packet[2] & 0x40) != 0x40))
+ return false;
+ if ((psmouse->pktcnt == 4) && ((psmouse->packet[3] & 0x48) != 0x48))
+ return false;
+ if ((psmouse->pktcnt == 6) && ((psmouse->packet[5] & 0x40) != 0x0))
+ return false;
+ return true;
+}
+
+static unsigned char alps_get_packet_id_v7(char *byte)
+{
+ unsigned char packet_id;
+
+ if (byte[4] & 0x40)
+ packet_id = V7_PACKET_ID_TWO;
+ else if (byte[4] & 0x01)
+ packet_id = V7_PACKET_ID_MULTI;
+ else if ((byte[0] & 0x10) && !(byte[4] & 0x43))
+ packet_id = V7_PACKET_ID_NEW;
+ else if (byte[1] == 0x00 && byte[4] == 0x00)
+ packet_id = V7_PACKET_ID_IDLE;
+ else
+ packet_id = V7_PACKET_ID_UNKNOWN;
+
+ return packet_id;
+}
+
+static void alps_get_finger_coordinate_v7(struct input_mt_pos *mt,
+ unsigned char *pkt,
+ unsigned char pkt_id)
+{
+ mt[0].x = ((pkt[2] & 0x80) << 4);
+ mt[0].x |= ((pkt[2] & 0x3F) << 5);
+ mt[0].x |= ((pkt[3] & 0x30) >> 1);
+ mt[0].x |= (pkt[3] & 0x07);
+ mt[0].y = (pkt[1] << 3) | (pkt[0] & 0x07);
+
+ mt[1].x = ((pkt[3] & 0x80) << 4);
+ mt[1].x |= ((pkt[4] & 0x80) << 3);
+ mt[1].x |= ((pkt[4] & 0x3F) << 4);
+ mt[1].y = ((pkt[5] & 0x80) << 3);
+ mt[1].y |= ((pkt[5] & 0x3F) << 4);
+
+ if (pkt_id == V7_PACKET_ID_TWO) {
+ mt[1].x &= ~0x000F;
+ mt[1].y |= 0x000F;
+ } else if (pkt_id == V7_PACKET_ID_MULTI) {
+ mt[1].x &= ~0x003F;
+ mt[1].y &= ~0x0020;
+ mt[1].y |= ((pkt[4] & 0x02) << 4);
+ mt[1].y |= 0x001F;
+ } else if (pkt_id == V7_PACKET_ID_NEW) {
+ mt[1].x &= ~0x003F;
+ mt[1].x |= (pkt[0] & 0x20);
+ mt[1].y |= 0x000F;
+ }
+
+ mt[0].y = 0x7FF - mt[0].y;
+ mt[1].y = 0x7FF - mt[1].y;
+}
+
+static int alps_get_mt_count(struct input_mt_pos *mt)
+{
+ int i;
+
+ for (i = 0; i < MAX_TOUCHES && mt[i].x != 0 && mt[i].y != 0; i++)
+ ;
+
+ return i;
+}
+
+static int alps_decode_packet_v7(struct alps_fields *f,
+ unsigned char *p,
+ struct psmouse *psmouse)
+{
+ unsigned char pkt_id;
+
+ pkt_id = alps_get_packet_id_v7(p);
+ if (pkt_id == V7_PACKET_ID_IDLE)
+ return 0;
+ if (pkt_id == V7_PACKET_ID_UNKNOWN)
+ return -1;
+
+ alps_get_finger_coordinate_v7(f->mt, p, pkt_id);
+
+ if (pkt_id == V7_PACKET_ID_TWO || pkt_id == V7_PACKET_ID_MULTI) {
+ f->left = (p[0] & 0x80) >> 7;
+ f->right = (p[0] & 0x20) >> 5;
+ f->middle = (p[0] & 0x10) >> 4;
+ }
+
+ if (pkt_id == V7_PACKET_ID_TWO)
+ f->fingers = alps_get_mt_count(f->mt);
+ else if (pkt_id == V7_PACKET_ID_MULTI)
+ f->fingers = 3 + (p[5] & 0x03);
+
+ return 0;
+}
+
+static void alps_process_trackstick_packet_v7(struct psmouse *psmouse)
+{
+ struct alps_data *priv = psmouse->private;
+ unsigned char *packet = psmouse->packet;
+ struct input_dev *dev2 = priv->dev2;
+ int x, y, z, left, right, middle;
+
+ /*
+ * b7 b6 b5 b4 b3 b2 b1 b0
+ * Byte0 0 1 0 0 1 0 0 0
+ * Byte1 1 1 * * 1 M R L
+ * Byte2 X7 1 X5 X4 X3 X2 X1 X0
+ * Byte3 Z6 1 Y6 X6 1 Y2 Y1 Y0
+ * Byte4 Y7 0 Y5 Y4 Y3 1 1 0
+ * Byte5 T&P 0 Z5 Z4 Z3 Z2 Z1 Z0
+ * M / R / L: Middle / Right / Left button
+ */
+
+ x = ((packet[2] & 0xbf)) | ((packet[3] & 0x10) << 2);
+ y = (packet[3] & 0x07) | (packet[4] & 0xb8) |
+ ((packet[3] & 0x20) << 1);
+ z = (packet[5] & 0x3f) | ((packet[3] & 0x80) >> 1);
+
+ left = (packet[1] & 0x01);
+ right = (packet[1] & 0x02) >> 1;
+ middle = (packet[1] & 0x04) >> 2;
+
+ /* Divide 2 since trackpoint's speed is too fast */
+ input_report_rel(dev2, REL_X, (char)x / 2);
+ input_report_rel(dev2, REL_Y, -((char)y / 2));
+
+ input_report_key(dev2, BTN_LEFT, left);
+ input_report_key(dev2, BTN_RIGHT, right);
+ input_report_key(dev2, BTN_MIDDLE, middle);
+
+ input_sync(dev2);
+}
+
+static void alps_process_touchpad_packet_v7(struct psmouse *psmouse)
+{
+ struct alps_data *priv = psmouse->private;
+ struct input_dev *dev = psmouse->dev;
+ struct alps_fields *f = &priv->f;
+
+ memset(f, 0, sizeof(*f));
+
+ if (priv->decode_fields(f, psmouse->packet, psmouse))
+ return;
+
+ alps_report_mt_data(psmouse, alps_get_mt_count(f->mt));
+
+ input_mt_report_finger_count(dev, f->fingers);
+
+ input_report_key(dev, BTN_LEFT, f->left);
+ input_report_key(dev, BTN_RIGHT, f->right);
+ input_report_key(dev, BTN_MIDDLE, f->middle);
+
+ input_sync(dev);
+}
+
+static void alps_process_packet_v7(struct psmouse *psmouse)
+{
+ unsigned char *packet = psmouse->packet;
+
+ if ((packet[0] == 0x48) && ((packet[4] & 0x47) == 0x06))
+ alps_process_trackstick_packet_v7(psmouse);
+ else
+ alps_process_touchpad_packet_v7(psmouse);
+}
+
static void alps_report_bare_ps2_packet(struct psmouse *psmouse,
unsigned char packet[],
bool report_buttons)
@@ -1009,6 +1181,14 @@ static psmouse_ret_t alps_process_byte(struct psmouse *psmouse)
return PSMOUSE_BAD_DATA;
}
+ if (priv->proto_version == ALPS_PROTO_V7 &&
+ !alps_is_valid_package_v7(psmouse)) {
+ psmouse_dbg(psmouse, "refusing packet[%i] = %x\n",
+ psmouse->pktcnt - 1,
+ psmouse->packet[psmouse->pktcnt - 1]);
+ return PSMOUSE_BAD_DATA;
+ }
+
if (psmouse->pktcnt == psmouse->pktsize) {
priv->process_packet(psmouse);
return PSMOUSE_FULL_PACKET;
@@ -1121,6 +1301,22 @@ static int alps_rpt_cmd(struct psmouse *psmouse, int init_command,
return 0;
}
+static int alps_check_valid_firmware_id(unsigned char id[])
+{
+ int valid = 1;
+
+ if (id[0] == 0x73)
+ valid = 1;
+ else if (id[0] == 0x88) {
+ if ((id[1] == 0x07) ||
+ (id[1] == 0x08) ||
+ ((id[1] & 0xf0) == 0xB0))
+ valid = 1;
+ }
+
+ return valid;
+}
+
static int alps_enter_command_mode(struct psmouse *psmouse)
{
unsigned char param[4];
@@ -1130,8 +1326,7 @@ static int alps_enter_command_mode(struct psmouse *psmouse)
return -1;
}
- if ((param[0] != 0x88 || (param[1] != 0x07 && param[1] != 0x08)) &&
- param[0] != 0x73) {
+ if (!alps_check_valid_firmware_id(param)) {
psmouse_dbg(psmouse,
"unknown response while entering command mode\n");
return -1;
@@ -1785,6 +1980,32 @@ static int alps_hw_init_dolphin_v1(struct psmouse *psmouse)
return 0;
}
+static int alps_hw_init_v7(struct psmouse *psmouse)
+{
+ struct ps2dev *ps2dev = &psmouse->ps2dev;
+ int reg_val, ret = -1;
+
+ if (alps_enter_command_mode(psmouse) ||
+ alps_command_mode_read_reg(psmouse, 0xc2d9) == -1)
+ goto error;
+
+ if (alps_command_mode_write_reg(psmouse, 0xc2c9, 0x64))
+ goto error;
+
+ reg_val = alps_command_mode_read_reg(psmouse, 0xc2c4);
+ if (reg_val == -1)
+ goto error;
+ if (__alps_command_mode_write_reg(psmouse, reg_val | 0x02))
+ goto error;
+
+ alps_exit_command_mode(psmouse);
+ return ps2_command(ps2dev, NULL, PSMOUSE_CMD_ENABLE);
+
+error:
+ alps_exit_command_mode(psmouse);
+ return ret;
+}
+
static void alps_set_defaults(struct alps_data *priv)
{
priv->byte0 = 0x8f;
@@ -1843,6 +2064,21 @@ static void alps_set_defaults(struct alps_data *priv)
priv->x_max = 2047;
priv->y_max = 1535;
break;
+ case ALPS_PROTO_V7:
+ priv->hw_init = alps_hw_init_v7;
+ priv->process_packet = alps_process_packet_v7;
+ priv->decode_fields = alps_decode_packet_v7;
+ priv->set_abs_params = alps_set_abs_params_mt;
+ priv->nibble_commands = alps_v3_nibble_commands;
+ priv->addr_command = PSMOUSE_CMD_RESET_WRAP;
+ priv->x_max = 0xfff;
+ priv->y_max = 0x7ff;
+ priv->byte0 = 0x48;
+ priv->mask0 = 0x48;
+
+ if (priv->fw_ver[1] != 0xba)
+ priv->flags |= ALPS_BUTTONPAD;
+ break;
}
}
@@ -1914,6 +2150,12 @@ static int alps_identify(struct psmouse *psmouse, struct alps_data *priv)
return -EIO;
else
return 0;
+ } else if (ec[0] == 0x88 &&
+ ((ec[1] & 0xf0) == 0xb0 || (ec[1] & 0xf0) == 0xc0)) {
+ priv->proto_version = ALPS_PROTO_V7;
+ alps_set_defaults(priv);
+
+ return 0;
} else if (ec[0] == 0x88 && ec[1] == 0x08) {
priv->proto_version = ALPS_PROTO_V3;
alps_set_defaults(priv);
@@ -1985,6 +2227,10 @@ static void alps_set_abs_params_mt(struct alps_data *priv,
set_bit(BTN_TOOL_TRIPLETAP, dev1->keybit);
set_bit(BTN_TOOL_QUADTAP, dev1->keybit);
+
+ /* V7 is real multi-touch */
+ if (priv->proto_version == ALPS_PROTO_V7)
+ clear_bit(INPUT_PROP_SEMI_MT, dev1->propbit);
}
int alps_init(struct psmouse *psmouse)
@@ -2030,7 +2276,9 @@ int alps_init(struct psmouse *psmouse)
dev1->evbit[BIT_WORD(EV_ABS)] |= BIT_MASK(EV_ABS);
priv->set_abs_params(priv, dev1);
- input_set_abs_params(dev1, ABS_PRESSURE, 0, 127, 0, 0);
+ /* No pressure on V7 */
+ if (priv->proto_version != ALPS_PROTO_V7)
+ input_set_abs_params(dev1, ABS_PRESSURE, 0, 127, 0, 0);
if (priv->flags & ALPS_WHEEL) {
dev1->evbit[BIT_WORD(EV_REL)] |= BIT_MASK(EV_REL);
@@ -2047,6 +2295,9 @@ int alps_init(struct psmouse *psmouse)
dev1->keybit[BIT_WORD(BTN_1)] |= BIT_MASK(BTN_1);
dev1->keybit[BIT_WORD(BTN_2)] |= BIT_MASK(BTN_2);
dev1->keybit[BIT_WORD(BTN_3)] |= BIT_MASK(BTN_3);
+ } else if (priv->flags & ALPS_BUTTONPAD) {
+ set_bit(INPUT_PROP_BUTTONPAD, dev1->propbit);
+ clear_bit(BTN_RIGHT, dev1->keybit);
} else {
dev1->keybit[BIT_WORD(BTN_MIDDLE)] |= BIT_MASK(BTN_MIDDLE);
}
diff --git a/drivers/input/mouse/alps.h b/drivers/input/mouse/alps.h
index e3d0f09..a98ac9b 100644
--- a/drivers/input/mouse/alps.h
+++ b/drivers/input/mouse/alps.h
@@ -20,6 +20,7 @@
#define ALPS_PROTO_V4 4
#define ALPS_PROTO_V5 5
#define ALPS_PROTO_V6 6
+#define ALPS_PROTO_V7 7 /* t3btl t4s */
#define MAX_TOUCHES 2
@@ -27,6 +28,23 @@
#define DOLPHIN_PROFILE_XOFFSET 8 /* x-electrode offset */
#define DOLPHIN_PROFILE_YOFFSET 1 /* y-electrode offset */
+/*
+ * enum V7_PACKET_ID - defines the packet type for V7
+ * V7_PACKET_ID_IDLE: There's no finger and no button activity.
+ * V7_PACKET_ID_TWO: There's one or two non-resting fingers on touchpad
+ * or there's button activities.
+ * V7_PACKET_ID_MULTI: There are at least three non-resting fingers.
+ * V7_PACKET_ID_NEW: The finger position in slot is not continues from
+ * previous packet.
+*/
+enum V7_PACKET_ID {
+ V7_PACKET_ID_IDLE,
+ V7_PACKET_ID_TWO,
+ V7_PACKET_ID_MULTI,
+ V7_PACKET_ID_NEW,
+ V7_PACKET_ID_UNKNOWN,
+};
+
/**
* struct alps_model_info - touchpad ID table
* @signature: E7 response string to match.
--
2.0.0
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* [PATCH 13/14] alps: Cache firmware version
From: Hans de Goede @ 2014-07-09 15:24 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Yunkang Tang, linux-input, Hans de Goede
In-Reply-To: <1404919459-23561-1-git-send-email-hdegoede@redhat.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/input/mouse/alps.c | 3 +++
drivers/input/mouse/alps.h | 2 ++
2 files changed, 5 insertions(+)
diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
index 140471d..ad3a708 100644
--- a/drivers/input/mouse/alps.c
+++ b/drivers/input/mouse/alps.c
@@ -1901,6 +1901,9 @@ static int alps_identify(struct psmouse *psmouse, struct alps_data *priv)
alps_exit_command_mode(psmouse))
return -EIO;
+ /* Save the Firmware version */
+ memcpy(priv->fw_ver, ec, 3);
+
if (alps_match_table(psmouse, priv, e7, ec) == 0) {
return 0;
} else if (e7[0] == 0x73 && e7[1] == 0x03 && e7[2] == 0x50 &&
diff --git a/drivers/input/mouse/alps.h b/drivers/input/mouse/alps.h
index 17e3ae3..e3d0f09 100644
--- a/drivers/input/mouse/alps.h
+++ b/drivers/input/mouse/alps.h
@@ -124,6 +124,7 @@ struct alps_fields {
* known format for this model. The first byte of the report, ANDed with
* mask0, should match byte0.
* @mask0: The mask used to check the first byte of the report.
+ * @fw_ver: cached copy of firmware version (EC report)
* @flags: Additional device capabilities (passthrough port, trackstick, etc.).
* @x_max: Largest possible X position value.
* @y_max: Largest possible Y position value.
@@ -149,6 +150,7 @@ struct alps_data {
int addr_command;
unsigned char proto_version;
unsigned char byte0, mask0;
+ unsigned char fw_ver[3];
int flags;
int x_max;
int y_max;
--
2.0.0
^ 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