Re: [PATCH 4/6] input: cyapa: enable/disable trackpad device based on LID state

2014-05-20 Thread Rafael J. Wysocki
On Tuesday, May 20, 2014 08:25:51 AM Dmitry Torokhov wrote:
> On Tue, May 20, 2014 at 02:40:12PM +0200, Rafael J. Wysocki wrote:
> > On Monday, May 19, 2014 08:43:02 PM Dmitry Torokhov wrote:
> > > Hi Dudley,
> > > 
> > > On Wed, Apr 16, 2014 at 08:39:34AM +, Dudley Du wrote:
> > > > Rely on EV_SW and SW_LID bits to identify a LID device, and hook
> > > > up our filter to listen for SW_LID events to enable/disable touchpad 
> > > > when
> > > > LID is open/closed.
> > > > TEST=test on Chomebooks.
> > > 
> > > This is a policy and it does not belong in the kernel. Please work with
> > > Rafael to establish generic interface to put devices into low power mode
> > > (like accelerating runtime PM idle timeout)
> > 
> > I'm not really sure what you mean here, care to be more specific?
> 
> I think we chatted about this before - we need a uniform interface for
> userspace to put devices into low power mode on demand.

I'm still not sure what you mean exactly.

If you mean an interface for user space to *force* transitions into low-power
states, we can't have it, because user space doesn't know when it is safe to
do that.  The /sys/devices/.../power/control interface is the best we can
give to user space to this end.

Adding Alan Stern to the recipient list, because he was inovlved in discussions
about that.

But if you mean an interface for user space to poke at pm_runtime_idle() to
possibly trigger a runtime suspend, that we can add I think.  Alan?

> As implementation detail I thought we could require runtime PM for that and
> simply pretend that the PM timeout expired early when userspace invokes
> that API.

That'd be overly complicated IMHO.  Calling pm_runtime_idle() should suffice.

> > 
> > > and use it when userspace detects that lid is closed.
> > 
> > I guess we get an event then, don't we?
> 
> Right, userspace gets EV_SW/SW_LID input event and needs to react. In
> this particular case the desire is to power down touchpad (since it is
> unaccessible). I am not sure why system suspend (which I expect happen
> in reaction to lid closing) is not enough, but that's question for
> Dudley.

OK

Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH 4/6] input: cyapa: enable/disable trackpad device based on LID state

2014-05-20 Thread Benson Leung
Hi Dmitry and Rafael,

I can provide more insight here since I implemented an early version
of this in the atmel and cyapa drivers that are used in the chromeos
kernel.

On Tue, May 20, 2014 at 8:25 AM, Dmitry Torokhov
 wrote:
> On Tue, May 20, 2014 at 02:40:12PM +0200, Rafael J. Wysocki wrote:
>> On Monday, May 19, 2014 08:43:02 PM Dmitry Torokhov wrote:
>> > Hi Dudley,
>> >
>> > On Wed, Apr 16, 2014 at 08:39:34AM +, Dudley Du wrote:
>> > > Rely on EV_SW and SW_LID bits to identify a LID device, and hook
>> > > up our filter to listen for SW_LID events to enable/disable touchpad when
>> > > LID is open/closed.
>> > > TEST=test on Chomebooks.
>> >
>> > This is a policy and it does not belong in the kernel. Please work with
>> > Rafael to establish generic interface to put devices into low power mode
>> > (like accelerating runtime PM idle timeout)
>>
>> I'm not really sure what you mean here, care to be more specific?
>
> I think we chatted about this before - we need a uniform interface for
> userspace to put devices into low power mode on demand. As
> implementation detail I thought we could require runtime PM for that and
> simply pretend that the PM timeout expired early when userspace invokes
> that API.
>
>>
>> > and use it when userspace detects that lid is closed.
>>
>> I guess we get an event then, don't we?
>
> Right, userspace gets EV_SW/SW_LID input event and needs to react. In
> this particular case the desire is to power down touchpad (since it is
> unaccessible). I am not sure why system suspend (which I expect happen
> in reaction to lid closing) is not enough, but that's question for
> Dudley.


Waiting for system suspend isn't sufficient here because system
suspend takes some time to trigger from user space, and in that time,
the B panel of the laptop may couple with the active trackpad, or the
C panel with the active touchscreen. This may generate stray input
events, which may in turn cancel the suspend if the drivers use
pm_wakeup_event(), and those input events may trigger something
unwanted in the UI (think, accidentally one-click buy something on
Amazon, etc).

I came up with a requirement: When the lid is closed, as soon as
possible, the devices must be off. That's part of the reason why
Dudley's patch does it in the kernel rather than doing the round trip
to user space.

Furthermore, the policy on lid close is not always to enter suspend.
We have something called Docked Mode, which allows the system to not
suspend while the lid is closed if there's an external monitor
attached. In this case, the touch devices must be disabled as well as
again they risk generating stray events with the lid closed.

I'd prefer if there was some way to establish a relationship between
the lid and devices that must be power managed differently depending
on the state of the lid, rather than leaving it to user space to
manage the power management of these devices in real time and open up
the possibility of race conditions there if user space is too slow.

Thanks!

-- 
Benson Leung
Software Engineer, Chrome OS
Google Inc.
ble...@google.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH 4/6] input: cyapa: enable/disable trackpad device based on LID state

2014-05-20 Thread Dmitry Torokhov
On Tue, May 20, 2014 at 02:40:12PM +0200, Rafael J. Wysocki wrote:
> On Monday, May 19, 2014 08:43:02 PM Dmitry Torokhov wrote:
> > Hi Dudley,
> > 
> > On Wed, Apr 16, 2014 at 08:39:34AM +, Dudley Du wrote:
> > > Rely on EV_SW and SW_LID bits to identify a LID device, and hook
> > > up our filter to listen for SW_LID events to enable/disable touchpad when
> > > LID is open/closed.
> > > TEST=test on Chomebooks.
> > 
> > This is a policy and it does not belong in the kernel. Please work with
> > Rafael to establish generic interface to put devices into low power mode
> > (like accelerating runtime PM idle timeout)
> 
> I'm not really sure what you mean here, care to be more specific?

I think we chatted about this before - we need a uniform interface for
userspace to put devices into low power mode on demand. As
implementation detail I thought we could require runtime PM for that and
simply pretend that the PM timeout expired early when userspace invokes
that API.

> 
> > and use it when userspace detects that lid is closed.
> 
> I guess we get an event then, don't we?

Right, userspace gets EV_SW/SW_LID input event and needs to react. In
this particular case the desire is to power down touchpad (since it is
unaccessible). I am not sure why system suspend (which I expect happen
in reaction to lid closing) is not enough, but that's question for
Dudley.

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH 4/6] input: cyapa: enable/disable trackpad device based on LID state

2014-05-20 Thread Rafael J. Wysocki
On Monday, May 19, 2014 08:43:02 PM Dmitry Torokhov wrote:
> Hi Dudley,
> 
> On Wed, Apr 16, 2014 at 08:39:34AM +, Dudley Du wrote:
> > Rely on EV_SW and SW_LID bits to identify a LID device, and hook
> > up our filter to listen for SW_LID events to enable/disable touchpad when
> > LID is open/closed.
> > TEST=test on Chomebooks.
> 
> This is a policy and it does not belong in the kernel. Please work with
> Rafael to establish generic interface to put devices into low power mode
> (like accelerating runtime PM idle timeout)

I'm not really sure what you mean here, care to be more specific?

> and use it when userspace detects that lid is closed.

I guess we get an event then, don't we?

Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


RE: [PATCH 4/6] input: cyapa: enable/disable trackpad device based on LID state

2014-05-20 Thread Dudley Du
Hi Rafael,

Could you help give some advice on how to apply the function to put the 
trackpad device into low power mode with LID in current kernel?
or it's just a policy and should not be submitted to kernel right now?

Thanks,
Dudley

>
> -Original Message-
> From: Dmitry Torokhov [mailto:dmitry.torok...@gmail.com]
> Sent: Tuesday, May 20, 2014 11:43 AM
> To: Dudley Du
> Cc: Benson Leung; Daniel Kurtz; David Solda; linux-in...@vger.kernel.org; 
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 4/6] input: cyapa: enable/disable trackpad device based 
> on LID state
>
> Hi Dudley,
>
> On Wed, Apr 16, 2014 at 08:39:34AM +, Dudley Du wrote:
> > Rely on EV_SW and SW_LID bits to identify a LID device, and hook up
> > our filter to listen for SW_LID events to enable/disable touchpad when
> > LID is open/closed.
> > TEST=test on Chomebooks.
>
> This is a policy and it does not belong in the kernel. Please work with 
> Rafael to establish generic interface to put devices into low power mode 
> (like accelerating runtime PM idle timeout) and use it when userspace detects 
> that lid is closed.
>
> Thanks.
>
> >
> > Signed-off-by: Du, Dudley 
> > ---
> > diff --git a/drivers/input/mouse/cyapa.c b/drivers/input/mouse/cyapa.c
> > index 6820b3f..da03427 100644
> > --- a/drivers/input/mouse/cyapa.c
> > +++ b/drivers/input/mouse/cyapa.c
> > @@ -523,6 +523,9 @@ struct cyapa {
> > int physical_size_x;
> > int physical_size_y;
> >
> > +   bool lid_handler_registered;
> > +   struct input_handler lid_handler;
> > +
> > /* used in ttsp and truetouch based trackpad devices. */
> > u8 x_origin;  /* X Axis Origin: 0 = left side; 1 = rigth side. */
> > u8 y_origin;  /* Y Axis Origin: 0 = top; 1 = bottom. */ @@
> > -3107,6 +3110,125 @@ static void cyapa_start_runtime(struct cyapa
> > *cyapa)  static void cyapa_start_runtime(struct cyapa *cyapa) {}
> > #endif /* CONFIG_PM_RUNTIME */
> >
> > +
> > +/*
> > + * We rely on EV_SW and SW_LID bits to identify a LID device, and
> > +hook
> > + * up our filter to listen for SW_LID events to enable/disable
> > +touchpad when
> > + * LID is open/closed.
> > + */
> > +static const struct input_device_id lid_device_ids[] = {
> > +   {
> > +   .flags = INPUT_DEVICE_ID_MATCH_EVBIT |
> > +INPUT_DEVICE_ID_MATCH_SWBIT,
> > +   .evbit = { BIT_MASK(EV_SW) },
> > +   .swbit = { BIT_MASK(SW_LID) },
> > +   },
> > +   { },
> > +};
> > +
> > +static int lid_device_connect(struct input_handler *handler,
> > + struct input_dev *dev,
> > + const struct input_device_id *id) {
> > +   struct input_handle *lid_handle;
> > +   int error;
> > +
> > +   lid_handle = kzalloc(sizeof(struct input_handle), GFP_KERNEL);
> > +   if (!lid_handle)
> > +   return -ENOMEM;
> > +
> > +   lid_handle->dev = dev;
> > +   lid_handle->handler = handler;
> > +   lid_handle->name = "lid_event_handler";
> > +   lid_handle->private = handler->private;
> > +
> > +   error = input_register_handle(lid_handle);
> > +   if (error)
> > +   goto err_free;
> > +
> > +   error = input_open_device(lid_handle);
> > +   if (error)
> > +   goto err_unregister;
> > +
> > +   return 0;
> > +err_unregister:
> > +   input_unregister_handle(lid_handle);
> > +err_free:
> > +   kfree(lid_handle);
> > +   return error;
> > +}
> > +
> > +static void lid_device_disconnect(struct input_handle *handle) {
> > +   input_close_device(handle);
> > +   input_unregister_handle(handle);
> > +   kfree(handle);
> > +}
> > +
> > +static bool lid_event_filter(struct input_handle *handle,
> > +unsigned int type, unsigned int code, int
> > +value) {
> > +   struct cyapa *cyapa = handle->private;
> > +   struct device *dev = >client->dev;
> > +
> > +   if (type == EV_SW && code == SW_LID) {
> > +   if (cyapa->suspended) {
> > +   /*
> > +* If the lid event filter is called while 
> > suspended,
> > +* there is no guarantee that the underlying i2cs 
> > are
> > 

RE: [PATCH 4/6] input: cyapa: enable/disable trackpad device based on LID state

2014-05-20 Thread Dudley Du
Hi Rafael,

Could you help give some advice on how to apply the function to put the 
trackpad device into low power mode with LID in current kernel?
or it's just a policy and should not be submitted to kernel right now?

Thanks,
Dudley


 -Original Message-
 From: Dmitry Torokhov [mailto:dmitry.torok...@gmail.com]
 Sent: Tuesday, May 20, 2014 11:43 AM
 To: Dudley Du
 Cc: Benson Leung; Daniel Kurtz; David Solda; linux-in...@vger.kernel.org; 
 linux-kernel@vger.kernel.org
 Subject: Re: [PATCH 4/6] input: cyapa: enable/disable trackpad device based 
 on LID state

 Hi Dudley,

 On Wed, Apr 16, 2014 at 08:39:34AM +, Dudley Du wrote:
  Rely on EV_SW and SW_LID bits to identify a LID device, and hook up
  our filter to listen for SW_LID events to enable/disable touchpad when
  LID is open/closed.
  TEST=test on Chomebooks.

 This is a policy and it does not belong in the kernel. Please work with 
 Rafael to establish generic interface to put devices into low power mode 
 (like accelerating runtime PM idle timeout) and use it when userspace detects 
 that lid is closed.

 Thanks.

 
  Signed-off-by: Du, Dudley d...@cypress.com
  ---
  diff --git a/drivers/input/mouse/cyapa.c b/drivers/input/mouse/cyapa.c
  index 6820b3f..da03427 100644
  --- a/drivers/input/mouse/cyapa.c
  +++ b/drivers/input/mouse/cyapa.c
  @@ -523,6 +523,9 @@ struct cyapa {
  int physical_size_x;
  int physical_size_y;
 
  +   bool lid_handler_registered;
  +   struct input_handler lid_handler;
  +
  /* used in ttsp and truetouch based trackpad devices. */
  u8 x_origin;  /* X Axis Origin: 0 = left side; 1 = rigth side. */
  u8 y_origin;  /* Y Axis Origin: 0 = top; 1 = bottom. */ @@
  -3107,6 +3110,125 @@ static void cyapa_start_runtime(struct cyapa
  *cyapa)  static void cyapa_start_runtime(struct cyapa *cyapa) {}
  #endif /* CONFIG_PM_RUNTIME */
 
  +
  +/*
  + * We rely on EV_SW and SW_LID bits to identify a LID device, and
  +hook
  + * up our filter to listen for SW_LID events to enable/disable
  +touchpad when
  + * LID is open/closed.
  + */
  +static const struct input_device_id lid_device_ids[] = {
  +   {
  +   .flags = INPUT_DEVICE_ID_MATCH_EVBIT |
  +INPUT_DEVICE_ID_MATCH_SWBIT,
  +   .evbit = { BIT_MASK(EV_SW) },
  +   .swbit = { BIT_MASK(SW_LID) },
  +   },
  +   { },
  +};
  +
  +static int lid_device_connect(struct input_handler *handler,
  + struct input_dev *dev,
  + const struct input_device_id *id) {
  +   struct input_handle *lid_handle;
  +   int error;
  +
  +   lid_handle = kzalloc(sizeof(struct input_handle), GFP_KERNEL);
  +   if (!lid_handle)
  +   return -ENOMEM;
  +
  +   lid_handle-dev = dev;
  +   lid_handle-handler = handler;
  +   lid_handle-name = lid_event_handler;
  +   lid_handle-private = handler-private;
  +
  +   error = input_register_handle(lid_handle);
  +   if (error)
  +   goto err_free;
  +
  +   error = input_open_device(lid_handle);
  +   if (error)
  +   goto err_unregister;
  +
  +   return 0;
  +err_unregister:
  +   input_unregister_handle(lid_handle);
  +err_free:
  +   kfree(lid_handle);
  +   return error;
  +}
  +
  +static void lid_device_disconnect(struct input_handle *handle) {
  +   input_close_device(handle);
  +   input_unregister_handle(handle);
  +   kfree(handle);
  +}
  +
  +static bool lid_event_filter(struct input_handle *handle,
  +unsigned int type, unsigned int code, int
  +value) {
  +   struct cyapa *cyapa = handle-private;
  +   struct device *dev = cyapa-client-dev;
  +
  +   if (type == EV_SW  code == SW_LID) {
  +   if (cyapa-suspended) {
  +   /*
  +* If the lid event filter is called while 
  suspended,
  +* there is no guarantee that the underlying i2cs 
  are
  +* resumed at this point, so it is not safe to issue
  +* the command to change power modes.
  +* Instead, rely on cyapa_resume to set us back to
  +* PWR_MODE_FULL_ACTIVE.
  +*/
  +   return false;
  +   }
  +   if (value == 0) {
  +   if (cyapa-cyapa_set_power_mode)
  +   cyapa-cyapa_set_power_mode(cyapa,
  +   PWR_MODE_FULL_ACTIVE, 0);
  +   pm_runtime_set_active(dev);
  +   pm_runtime_enable(dev);
  +   } else {
  +   pm_runtime_disable(dev);
  +   if (cyapa-cyapa_set_power_mode)
  +   cyapa-cyapa_set_power_mode(cyapa

Re: [PATCH 4/6] input: cyapa: enable/disable trackpad device based on LID state

2014-05-20 Thread Rafael J. Wysocki
On Monday, May 19, 2014 08:43:02 PM Dmitry Torokhov wrote:
 Hi Dudley,
 
 On Wed, Apr 16, 2014 at 08:39:34AM +, Dudley Du wrote:
  Rely on EV_SW and SW_LID bits to identify a LID device, and hook
  up our filter to listen for SW_LID events to enable/disable touchpad when
  LID is open/closed.
  TEST=test on Chomebooks.
 
 This is a policy and it does not belong in the kernel. Please work with
 Rafael to establish generic interface to put devices into low power mode
 (like accelerating runtime PM idle timeout)

I'm not really sure what you mean here, care to be more specific?

 and use it when userspace detects that lid is closed.

I guess we get an event then, don't we?

Rafael

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH 4/6] input: cyapa: enable/disable trackpad device based on LID state

2014-05-20 Thread Dmitry Torokhov
On Tue, May 20, 2014 at 02:40:12PM +0200, Rafael J. Wysocki wrote:
 On Monday, May 19, 2014 08:43:02 PM Dmitry Torokhov wrote:
  Hi Dudley,
  
  On Wed, Apr 16, 2014 at 08:39:34AM +, Dudley Du wrote:
   Rely on EV_SW and SW_LID bits to identify a LID device, and hook
   up our filter to listen for SW_LID events to enable/disable touchpad when
   LID is open/closed.
   TEST=test on Chomebooks.
  
  This is a policy and it does not belong in the kernel. Please work with
  Rafael to establish generic interface to put devices into low power mode
  (like accelerating runtime PM idle timeout)
 
 I'm not really sure what you mean here, care to be more specific?

I think we chatted about this before - we need a uniform interface for
userspace to put devices into low power mode on demand. As
implementation detail I thought we could require runtime PM for that and
simply pretend that the PM timeout expired early when userspace invokes
that API.

 
  and use it when userspace detects that lid is closed.
 
 I guess we get an event then, don't we?

Right, userspace gets EV_SW/SW_LID input event and needs to react. In
this particular case the desire is to power down touchpad (since it is
unaccessible). I am not sure why system suspend (which I expect happen
in reaction to lid closing) is not enough, but that's question for
Dudley.

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH 4/6] input: cyapa: enable/disable trackpad device based on LID state

2014-05-20 Thread Benson Leung
Hi Dmitry and Rafael,

I can provide more insight here since I implemented an early version
of this in the atmel and cyapa drivers that are used in the chromeos
kernel.

On Tue, May 20, 2014 at 8:25 AM, Dmitry Torokhov
dmitry.torok...@gmail.com wrote:
 On Tue, May 20, 2014 at 02:40:12PM +0200, Rafael J. Wysocki wrote:
 On Monday, May 19, 2014 08:43:02 PM Dmitry Torokhov wrote:
  Hi Dudley,
 
  On Wed, Apr 16, 2014 at 08:39:34AM +, Dudley Du wrote:
   Rely on EV_SW and SW_LID bits to identify a LID device, and hook
   up our filter to listen for SW_LID events to enable/disable touchpad when
   LID is open/closed.
   TEST=test on Chomebooks.
 
  This is a policy and it does not belong in the kernel. Please work with
  Rafael to establish generic interface to put devices into low power mode
  (like accelerating runtime PM idle timeout)

 I'm not really sure what you mean here, care to be more specific?

 I think we chatted about this before - we need a uniform interface for
 userspace to put devices into low power mode on demand. As
 implementation detail I thought we could require runtime PM for that and
 simply pretend that the PM timeout expired early when userspace invokes
 that API.


  and use it when userspace detects that lid is closed.

 I guess we get an event then, don't we?

 Right, userspace gets EV_SW/SW_LID input event and needs to react. In
 this particular case the desire is to power down touchpad (since it is
 unaccessible). I am not sure why system suspend (which I expect happen
 in reaction to lid closing) is not enough, but that's question for
 Dudley.


Waiting for system suspend isn't sufficient here because system
suspend takes some time to trigger from user space, and in that time,
the B panel of the laptop may couple with the active trackpad, or the
C panel with the active touchscreen. This may generate stray input
events, which may in turn cancel the suspend if the drivers use
pm_wakeup_event(), and those input events may trigger something
unwanted in the UI (think, accidentally one-click buy something on
Amazon, etc).

I came up with a requirement: When the lid is closed, as soon as
possible, the devices must be off. That's part of the reason why
Dudley's patch does it in the kernel rather than doing the round trip
to user space.

Furthermore, the policy on lid close is not always to enter suspend.
We have something called Docked Mode, which allows the system to not
suspend while the lid is closed if there's an external monitor
attached. In this case, the touch devices must be disabled as well as
again they risk generating stray events with the lid closed.

I'd prefer if there was some way to establish a relationship between
the lid and devices that must be power managed differently depending
on the state of the lid, rather than leaving it to user space to
manage the power management of these devices in real time and open up
the possibility of race conditions there if user space is too slow.

Thanks!

-- 
Benson Leung
Software Engineer, Chrome OS
Google Inc.
ble...@google.com
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH 4/6] input: cyapa: enable/disable trackpad device based on LID state

2014-05-20 Thread Rafael J. Wysocki
On Tuesday, May 20, 2014 08:25:51 AM Dmitry Torokhov wrote:
 On Tue, May 20, 2014 at 02:40:12PM +0200, Rafael J. Wysocki wrote:
  On Monday, May 19, 2014 08:43:02 PM Dmitry Torokhov wrote:
   Hi Dudley,
   
   On Wed, Apr 16, 2014 at 08:39:34AM +, Dudley Du wrote:
Rely on EV_SW and SW_LID bits to identify a LID device, and hook
up our filter to listen for SW_LID events to enable/disable touchpad 
when
LID is open/closed.
TEST=test on Chomebooks.
   
   This is a policy and it does not belong in the kernel. Please work with
   Rafael to establish generic interface to put devices into low power mode
   (like accelerating runtime PM idle timeout)
  
  I'm not really sure what you mean here, care to be more specific?
 
 I think we chatted about this before - we need a uniform interface for
 userspace to put devices into low power mode on demand.

I'm still not sure what you mean exactly.

If you mean an interface for user space to *force* transitions into low-power
states, we can't have it, because user space doesn't know when it is safe to
do that.  The /sys/devices/.../power/control interface is the best we can
give to user space to this end.

Adding Alan Stern to the recipient list, because he was inovlved in discussions
about that.

But if you mean an interface for user space to poke at pm_runtime_idle() to
possibly trigger a runtime suspend, that we can add I think.  Alan?

 As implementation detail I thought we could require runtime PM for that and
 simply pretend that the PM timeout expired early when userspace invokes
 that API.

That'd be overly complicated IMHO.  Calling pm_runtime_idle() should suffice.

  
   and use it when userspace detects that lid is closed.
  
  I guess we get an event then, don't we?
 
 Right, userspace gets EV_SW/SW_LID input event and needs to react. In
 this particular case the desire is to power down touchpad (since it is
 unaccessible). I am not sure why system suspend (which I expect happen
 in reaction to lid closing) is not enough, but that's question for
 Dudley.

OK

Rafael

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH 4/6] input: cyapa: enable/disable trackpad device based on LID state

2014-05-19 Thread Dmitry Torokhov
Hi Dudley,

On Wed, Apr 16, 2014 at 08:39:34AM +, Dudley Du wrote:
> Rely on EV_SW and SW_LID bits to identify a LID device, and hook
> up our filter to listen for SW_LID events to enable/disable touchpad when
> LID is open/closed.
> TEST=test on Chomebooks.

This is a policy and it does not belong in the kernel. Please work with
Rafael to establish generic interface to put devices into low power mode
(like accelerating runtime PM idle timeout) and use it when userspace
detects that lid is closed.

Thanks.

> 
> Signed-off-by: Du, Dudley 
> ---
> diff --git a/drivers/input/mouse/cyapa.c b/drivers/input/mouse/cyapa.c
> index 6820b3f..da03427 100644
> --- a/drivers/input/mouse/cyapa.c
> +++ b/drivers/input/mouse/cyapa.c
> @@ -523,6 +523,9 @@ struct cyapa {
> int physical_size_x;
> int physical_size_y;
> 
> +   bool lid_handler_registered;
> +   struct input_handler lid_handler;
> +
> /* used in ttsp and truetouch based trackpad devices. */
> u8 x_origin;  /* X Axis Origin: 0 = left side; 1 = rigth side. */
> u8 y_origin;  /* Y Axis Origin: 0 = top; 1 = bottom. */
> @@ -3107,6 +3110,125 @@ static void cyapa_start_runtime(struct cyapa *cyapa)
>  static void cyapa_start_runtime(struct cyapa *cyapa) {}
>  #endif /* CONFIG_PM_RUNTIME */
> 
> +
> +/*
> + * We rely on EV_SW and SW_LID bits to identify a LID device, and hook
> + * up our filter to listen for SW_LID events to enable/disable touchpad when
> + * LID is open/closed.
> + */
> +static const struct input_device_id lid_device_ids[] = {
> +   {
> +   .flags = INPUT_DEVICE_ID_MATCH_EVBIT |
> +INPUT_DEVICE_ID_MATCH_SWBIT,
> +   .evbit = { BIT_MASK(EV_SW) },
> +   .swbit = { BIT_MASK(SW_LID) },
> +   },
> +   { },
> +};
> +
> +static int lid_device_connect(struct input_handler *handler,
> + struct input_dev *dev,
> + const struct input_device_id *id)
> +{
> +   struct input_handle *lid_handle;
> +   int error;
> +
> +   lid_handle = kzalloc(sizeof(struct input_handle), GFP_KERNEL);
> +   if (!lid_handle)
> +   return -ENOMEM;
> +
> +   lid_handle->dev = dev;
> +   lid_handle->handler = handler;
> +   lid_handle->name = "lid_event_handler";
> +   lid_handle->private = handler->private;
> +
> +   error = input_register_handle(lid_handle);
> +   if (error)
> +   goto err_free;
> +
> +   error = input_open_device(lid_handle);
> +   if (error)
> +   goto err_unregister;
> +
> +   return 0;
> +err_unregister:
> +   input_unregister_handle(lid_handle);
> +err_free:
> +   kfree(lid_handle);
> +   return error;
> +}
> +
> +static void lid_device_disconnect(struct input_handle *handle)
> +{
> +   input_close_device(handle);
> +   input_unregister_handle(handle);
> +   kfree(handle);
> +}
> +
> +static bool lid_event_filter(struct input_handle *handle,
> +unsigned int type, unsigned int code, int value)
> +{
> +   struct cyapa *cyapa = handle->private;
> +   struct device *dev = >client->dev;
> +
> +   if (type == EV_SW && code == SW_LID) {
> +   if (cyapa->suspended) {
> +   /*
> +* If the lid event filter is called while suspended,
> +* there is no guarantee that the underlying i2cs are
> +* resumed at this point, so it is not safe to issue
> +* the command to change power modes.
> +* Instead, rely on cyapa_resume to set us back to
> +* PWR_MODE_FULL_ACTIVE.
> +*/
> +   return false;
> +   }
> +   if (value == 0) {
> +   if (cyapa->cyapa_set_power_mode)
> +   cyapa->cyapa_set_power_mode(cyapa,
> +   PWR_MODE_FULL_ACTIVE, 0);
> +   pm_runtime_set_active(dev);
> +   pm_runtime_enable(dev);
> +   } else {
> +   pm_runtime_disable(dev);
> +   if (cyapa->cyapa_set_power_mode)
> +   cyapa->cyapa_set_power_mode(cyapa,
> +   PWR_MODE_OFF, 0);
> +   }
> +   }
> +
> +   return false;
> +}
> +
> +static void lid_event_register_handler(struct cyapa *cyapa)
> +{
> +   int error;
> +   struct input_handler *lid_handler = >lid_handler;
> +
> +   if (cyapa->lid_handler_registered)
> +   return;
> +
> +   lid_handler->filter = lid_event_filter;
> +   lid_handler->connect= lid_device_connect;
> +   lid_handler->disconnect = lid_device_disconnect;
> +   lid_handler->name   = "cyapa_lid_event_handler";

Re: [PATCH 4/6] input: cyapa: enable/disable trackpad device based on LID state

2014-05-19 Thread Dmitry Torokhov
Hi Dudley,

On Wed, Apr 16, 2014 at 08:39:34AM +, Dudley Du wrote:
 Rely on EV_SW and SW_LID bits to identify a LID device, and hook
 up our filter to listen for SW_LID events to enable/disable touchpad when
 LID is open/closed.
 TEST=test on Chomebooks.

This is a policy and it does not belong in the kernel. Please work with
Rafael to establish generic interface to put devices into low power mode
(like accelerating runtime PM idle timeout) and use it when userspace
detects that lid is closed.

Thanks.

 
 Signed-off-by: Du, Dudley d...@cypress.com
 ---
 diff --git a/drivers/input/mouse/cyapa.c b/drivers/input/mouse/cyapa.c
 index 6820b3f..da03427 100644
 --- a/drivers/input/mouse/cyapa.c
 +++ b/drivers/input/mouse/cyapa.c
 @@ -523,6 +523,9 @@ struct cyapa {
 int physical_size_x;
 int physical_size_y;
 
 +   bool lid_handler_registered;
 +   struct input_handler lid_handler;
 +
 /* used in ttsp and truetouch based trackpad devices. */
 u8 x_origin;  /* X Axis Origin: 0 = left side; 1 = rigth side. */
 u8 y_origin;  /* Y Axis Origin: 0 = top; 1 = bottom. */
 @@ -3107,6 +3110,125 @@ static void cyapa_start_runtime(struct cyapa *cyapa)
  static void cyapa_start_runtime(struct cyapa *cyapa) {}
  #endif /* CONFIG_PM_RUNTIME */
 
 +
 +/*
 + * We rely on EV_SW and SW_LID bits to identify a LID device, and hook
 + * up our filter to listen for SW_LID events to enable/disable touchpad when
 + * LID is open/closed.
 + */
 +static const struct input_device_id lid_device_ids[] = {
 +   {
 +   .flags = INPUT_DEVICE_ID_MATCH_EVBIT |
 +INPUT_DEVICE_ID_MATCH_SWBIT,
 +   .evbit = { BIT_MASK(EV_SW) },
 +   .swbit = { BIT_MASK(SW_LID) },
 +   },
 +   { },
 +};
 +
 +static int lid_device_connect(struct input_handler *handler,
 + struct input_dev *dev,
 + const struct input_device_id *id)
 +{
 +   struct input_handle *lid_handle;
 +   int error;
 +
 +   lid_handle = kzalloc(sizeof(struct input_handle), GFP_KERNEL);
 +   if (!lid_handle)
 +   return -ENOMEM;
 +
 +   lid_handle-dev = dev;
 +   lid_handle-handler = handler;
 +   lid_handle-name = lid_event_handler;
 +   lid_handle-private = handler-private;
 +
 +   error = input_register_handle(lid_handle);
 +   if (error)
 +   goto err_free;
 +
 +   error = input_open_device(lid_handle);
 +   if (error)
 +   goto err_unregister;
 +
 +   return 0;
 +err_unregister:
 +   input_unregister_handle(lid_handle);
 +err_free:
 +   kfree(lid_handle);
 +   return error;
 +}
 +
 +static void lid_device_disconnect(struct input_handle *handle)
 +{
 +   input_close_device(handle);
 +   input_unregister_handle(handle);
 +   kfree(handle);
 +}
 +
 +static bool lid_event_filter(struct input_handle *handle,
 +unsigned int type, unsigned int code, int value)
 +{
 +   struct cyapa *cyapa = handle-private;
 +   struct device *dev = cyapa-client-dev;
 +
 +   if (type == EV_SW  code == SW_LID) {
 +   if (cyapa-suspended) {
 +   /*
 +* If the lid event filter is called while suspended,
 +* there is no guarantee that the underlying i2cs are
 +* resumed at this point, so it is not safe to issue
 +* the command to change power modes.
 +* Instead, rely on cyapa_resume to set us back to
 +* PWR_MODE_FULL_ACTIVE.
 +*/
 +   return false;
 +   }
 +   if (value == 0) {
 +   if (cyapa-cyapa_set_power_mode)
 +   cyapa-cyapa_set_power_mode(cyapa,
 +   PWR_MODE_FULL_ACTIVE, 0);
 +   pm_runtime_set_active(dev);
 +   pm_runtime_enable(dev);
 +   } else {
 +   pm_runtime_disable(dev);
 +   if (cyapa-cyapa_set_power_mode)
 +   cyapa-cyapa_set_power_mode(cyapa,
 +   PWR_MODE_OFF, 0);
 +   }
 +   }
 +
 +   return false;
 +}
 +
 +static void lid_event_register_handler(struct cyapa *cyapa)
 +{
 +   int error;
 +   struct input_handler *lid_handler = cyapa-lid_handler;
 +
 +   if (cyapa-lid_handler_registered)
 +   return;
 +
 +   lid_handler-filter = lid_event_filter;
 +   lid_handler-connect= lid_device_connect;
 +   lid_handler-disconnect = lid_device_disconnect;
 +   lid_handler-name   = cyapa_lid_event_handler;
 +   lid_handler-id_table   = lid_device_ids;
 +   lid_handler-private= cyapa;
 +
 +   error =