All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] Input: cros_ec_keyb: Add support for a front proximity switch
@ 2020-12-05  0:47 Stephen Boyd
  2021-01-07  1:16 ` Doug Anderson
  0 siblings, 1 reply; 8+ messages in thread
From: Stephen Boyd @ 2020-12-05  0:47 UTC (permalink / raw)
  To: Enric Balletbo i Serra
  Cc: linux-kernel, linux-input, Dmitry Torokhov, Benson Leung, Guenter Roeck

Some cros ECs support a front proximity MKBP event via
'EC_MKBP_FRONT_PROXIMITY'. Map this to the 'SW_FRONT_PROXIMITY' input
event code so it can be reported up to userspace.

Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Benson Leung <bleung@chromium.org>
Cc: Guenter Roeck <groeck@chromium.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/input/keyboard/cros_ec_keyb.c          | 5 +++++
 include/linux/platform_data/cros_ec_commands.h | 1 +
 2 files changed, 6 insertions(+)

diff --git a/drivers/input/keyboard/cros_ec_keyb.c b/drivers/input/keyboard/cros_ec_keyb.c
index 15d17c717081..9d05e9192a10 100644
--- a/drivers/input/keyboard/cros_ec_keyb.c
+++ b/drivers/input/keyboard/cros_ec_keyb.c
@@ -107,6 +107,11 @@ static const struct cros_ec_bs_map cros_ec_keyb_bs[] = {
 		.code		= SW_TABLET_MODE,
 		.bit		= EC_MKBP_TABLET_MODE,
 	},
+	{
+		.ev_type	= EV_SW,
+		.code		= SW_FRONT_PROXIMITY,
+		.bit		= EC_MKBP_FRONT_PROXIMITY,
+	},
 };
 
 /*
diff --git a/include/linux/platform_data/cros_ec_commands.h b/include/linux/platform_data/cros_ec_commands.h
index a3a9a878415f..d9dc5e30e59e 100644
--- a/include/linux/platform_data/cros_ec_commands.h
+++ b/include/linux/platform_data/cros_ec_commands.h
@@ -3455,6 +3455,7 @@ struct ec_response_get_next_event_v1 {
 #define EC_MKBP_LID_OPEN	0
 #define EC_MKBP_TABLET_MODE	1
 #define EC_MKBP_BASE_ATTACHED	2
+#define EC_MKBP_FRONT_PROXIMITY	3
 
 /* Run keyboard factory test scanning */
 #define EC_CMD_KEYBOARD_FACTORY_TEST 0x0068

base-commit: b65054597872ce3aefbc6a666385eabdf9e288da
-- 
https://chromeos.dev


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] Input: cros_ec_keyb: Add support for a front proximity switch
  2020-12-05  0:47 [PATCH] Input: cros_ec_keyb: Add support for a front proximity switch Stephen Boyd
@ 2021-01-07  1:16 ` Doug Anderson
  2021-01-07  2:21   ` Dmitry Torokhov
       [not found]   ` <CABXOdTcT4f_mg=ukPd0sD90o-aKg3qgiuLDRNPU8SUuFnFbRxA@mail.gmail.com>
  0 siblings, 2 replies; 8+ messages in thread
From: Doug Anderson @ 2021-01-07  1:16 UTC (permalink / raw)
  To: Stephen Boyd, Dmitry Torokhov, Enric Balletbo i Serra, Benson Leung
  Cc: LKML, open list:HID CORE LAYER, Guenter Roeck

Hi,

On Fri, Dec 4, 2020 at 4:48 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Some cros ECs support a front proximity MKBP event via
> 'EC_MKBP_FRONT_PROXIMITY'. Map this to the 'SW_FRONT_PROXIMITY' input
> event code so it can be reported up to userspace.
>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Benson Leung <bleung@chromium.org>
> Cc: Guenter Roeck <groeck@chromium.org>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
>  drivers/input/keyboard/cros_ec_keyb.c          | 5 +++++
>  include/linux/platform_data/cros_ec_commands.h | 1 +
>  2 files changed, 6 insertions(+)

This seems really straightforward.

Reviewed-by: Douglas Anderson <dianders@chromium.org>

Given that it touches a header file owned by the Chrome OS maintainers
and a driver owned by input, how should it land?  One maintainer Acks
and the other lands?

-Doug

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Input: cros_ec_keyb: Add support for a front proximity switch
  2021-01-07  1:16 ` Doug Anderson
@ 2021-01-07  2:21   ` Dmitry Torokhov
  2021-01-07 14:57     ` Doug Anderson
       [not found]   ` <CABXOdTcT4f_mg=ukPd0sD90o-aKg3qgiuLDRNPU8SUuFnFbRxA@mail.gmail.com>
  1 sibling, 1 reply; 8+ messages in thread
From: Dmitry Torokhov @ 2021-01-07  2:21 UTC (permalink / raw)
  To: Doug Anderson
  Cc: Stephen Boyd, Enric Balletbo i Serra, Benson Leung, LKML,
	open list:HID CORE LAYER, Guenter Roeck

Hi Doug, Stephen,

On Wed, Jan 06, 2021 at 05:16:10PM -0800, Doug Anderson wrote:
> Hi,
> 
> On Fri, Dec 4, 2020 at 4:48 PM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > Some cros ECs support a front proximity MKBP event via
> > 'EC_MKBP_FRONT_PROXIMITY'. Map this to the 'SW_FRONT_PROXIMITY' input
> > event code so it can be reported up to userspace.
> >
> > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > Cc: Benson Leung <bleung@chromium.org>
> > Cc: Guenter Roeck <groeck@chromium.org>
> > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> > ---
> >  drivers/input/keyboard/cros_ec_keyb.c          | 5 +++++
> >  include/linux/platform_data/cros_ec_commands.h | 1 +
> >  2 files changed, 6 insertions(+)
> 
> This seems really straightforward.
> 
> Reviewed-by: Douglas Anderson <dianders@chromium.org>
> 
> Given that it touches a header file owned by the Chrome OS maintainers
> and a driver owned by input, how should it land?  One maintainer Acks
> and the other lands?

Sorry about missing this one, however the "front proximity" switch has
been introduced for the benefit of phone devices, to be emitted when a
device is raised to user's ear, and I do not think we should be using
this here.

We have just recently had similar discussion with regard to palm- and
lap-mode sensors and whether they should be reported over input or IIO
as true proximity sensors:

https://lore.kernel.org/linux-iio/9f9b0ff6-3bf1-63c4-eb36-901cecd7c4d9@redhat.com/

Based on what we are doing for other Chrome OS devices that expose
proximity sensors (for example trogdor) we have decided that we all
should be using IIO as it will allow not only on/off, but true proximity
reporting with potential of implementing smarter policies by userspace.

Because of that we should do the same here and export this as IIO
proximity sensor as well.

Thanks.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Input: cros_ec_keyb: Add support for a front proximity switch
  2021-01-07  2:21   ` Dmitry Torokhov
@ 2021-01-07 14:57     ` Doug Anderson
  2021-01-07 19:46       ` Dmitry Torokhov
  0 siblings, 1 reply; 8+ messages in thread
From: Doug Anderson @ 2021-01-07 14:57 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Stephen Boyd, Enric Balletbo i Serra, Benson Leung, LKML,
	open list:HID CORE LAYER, Guenter Roeck

Hi,

On Wed, Jan 6, 2021 at 6:22 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> Hi Doug, Stephen,
>
> On Wed, Jan 06, 2021 at 05:16:10PM -0800, Doug Anderson wrote:
> > Hi,
> >
> > On Fri, Dec 4, 2020 at 4:48 PM Stephen Boyd <swboyd@chromium.org> wrote:
> > >
> > > Some cros ECs support a front proximity MKBP event via
> > > 'EC_MKBP_FRONT_PROXIMITY'. Map this to the 'SW_FRONT_PROXIMITY' input
> > > event code so it can be reported up to userspace.
> > >
> > > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > > Cc: Benson Leung <bleung@chromium.org>
> > > Cc: Guenter Roeck <groeck@chromium.org>
> > > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> > > ---
> > >  drivers/input/keyboard/cros_ec_keyb.c          | 5 +++++
> > >  include/linux/platform_data/cros_ec_commands.h | 1 +
> > >  2 files changed, 6 insertions(+)
> >
> > This seems really straightforward.
> >
> > Reviewed-by: Douglas Anderson <dianders@chromium.org>
> >
> > Given that it touches a header file owned by the Chrome OS maintainers
> > and a driver owned by input, how should it land?  One maintainer Acks
> > and the other lands?
>
> Sorry about missing this one, however the "front proximity" switch has
> been introduced for the benefit of phone devices, to be emitted when a
> device is raised to user's ear, and I do not think we should be using
> this here.
>
> We have just recently had similar discussion with regard to palm- and
> lap-mode sensors and whether they should be reported over input or IIO
> as true proximity sensors:
>
> https://lore.kernel.org/linux-iio/9f9b0ff6-3bf1-63c4-eb36-901cecd7c4d9@redhat.com/
>
> Based on what we are doing for other Chrome OS devices that expose
> proximity sensors (for example trogdor) we have decided that we all
> should be using IIO as it will allow not only on/off, but true proximity
> reporting with potential of implementing smarter policies by userspace.
>
> Because of that we should do the same here and export this as IIO
> proximity sensor as well.

For devices with a true proximity sensor that's exactly what we're
doing.  I've only been involved in the periphery of the discussion,
but as I understand it there are some models of laptop for which we
don't have a true proximity sensor.  On these devices, the EC is in
charge of deciding about proximity based on a number of factors.
Unfortunately, it's not a public bug, but since you're at Google I
think you can look at b/168714440 to find the relevant discussion.
These are the models that Stephen is trying to add support for here...

-Doug

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Input: cros_ec_keyb: Add support for a front proximity switch
  2021-01-07 14:57     ` Doug Anderson
@ 2021-01-07 19:46       ` Dmitry Torokhov
  2021-01-15 22:45         ` Stephen Boyd
  2021-02-02 13:43         ` Pavel Machek
  0 siblings, 2 replies; 8+ messages in thread
From: Dmitry Torokhov @ 2021-01-07 19:46 UTC (permalink / raw)
  To: Doug Anderson
  Cc: Stephen Boyd, Enric Balletbo i Serra, Benson Leung, LKML,
	open list:HID CORE LAYER, Guenter Roeck

On Thu, Jan 07, 2021 at 06:57:10AM -0800, Doug Anderson wrote:
> Hi,
> 
> On Wed, Jan 6, 2021 at 6:22 PM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> >
> > Hi Doug, Stephen,
> >
> > On Wed, Jan 06, 2021 at 05:16:10PM -0800, Doug Anderson wrote:
> > > Hi,
> > >
> > > On Fri, Dec 4, 2020 at 4:48 PM Stephen Boyd <swboyd@chromium.org> wrote:
> > > >
> > > > Some cros ECs support a front proximity MKBP event via
> > > > 'EC_MKBP_FRONT_PROXIMITY'. Map this to the 'SW_FRONT_PROXIMITY' input
> > > > event code so it can be reported up to userspace.
> > > >
> > > > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > > > Cc: Benson Leung <bleung@chromium.org>
> > > > Cc: Guenter Roeck <groeck@chromium.org>
> > > > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> > > > ---
> > > >  drivers/input/keyboard/cros_ec_keyb.c          | 5 +++++
> > > >  include/linux/platform_data/cros_ec_commands.h | 1 +
> > > >  2 files changed, 6 insertions(+)
> > >
> > > This seems really straightforward.
> > >
> > > Reviewed-by: Douglas Anderson <dianders@chromium.org>
> > >
> > > Given that it touches a header file owned by the Chrome OS maintainers
> > > and a driver owned by input, how should it land?  One maintainer Acks
> > > and the other lands?
> >
> > Sorry about missing this one, however the "front proximity" switch has
> > been introduced for the benefit of phone devices, to be emitted when a
> > device is raised to user's ear, and I do not think we should be using
> > this here.
> >
> > We have just recently had similar discussion with regard to palm- and
> > lap-mode sensors and whether they should be reported over input or IIO
> > as true proximity sensors:
> >
> > https://lore.kernel.org/linux-iio/9f9b0ff6-3bf1-63c4-eb36-901cecd7c4d9@redhat.com/
> >
> > Based on what we are doing for other Chrome OS devices that expose
> > proximity sensors (for example trogdor) we have decided that we all
> > should be using IIO as it will allow not only on/off, but true proximity
> > reporting with potential of implementing smarter policies by userspace.
> >
> > Because of that we should do the same here and export this as IIO
> > proximity sensor as well.
> 
> For devices with a true proximity sensor that's exactly what we're
> doing.  I've only been involved in the periphery of the discussion,
> but as I understand it there are some models of laptop for which we
> don't have a true proximity sensor.  On these devices, the EC is in
> charge of deciding about proximity based on a number of factors.

Yes, I understand that on some devices the proximity sensors are not
true sensors but rather on/off signals, potentially derived from a
multitude of sources. However there is still a benefit in exposing them
as IIO proximity devices with limited reporting representing
[near, infinity] range/values. This will mean that userspace needs to
monitor only one set of devices (IIO) instead of both IIO and input, and
will not require constantly expanding EV_SW set to account for
ever-growing number of proximity sensors (lap, palm, general presence,
etc).

Thanks.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Input: cros_ec_keyb: Add support for a front proximity switch
       [not found]   ` <CABXOdTcT4f_mg=ukPd0sD90o-aKg3qgiuLDRNPU8SUuFnFbRxA@mail.gmail.com>
@ 2021-01-15 22:43     ` Stephen Boyd
  0 siblings, 0 replies; 8+ messages in thread
From: Stephen Boyd @ 2021-01-15 22:43 UTC (permalink / raw)
  To: Doug Anderson, Guenter Roeck
  Cc: Dmitry Torokhov, Enric Balletbo i Serra, Benson Leung, LKML,
	HID CORE LAYER, Guenter Roeck,

Quoting Guenter Roeck (2021-01-06 18:30:20)
> On Wed, Jan 6, 2021 at 5:16 PM Doug Anderson <dianders@chromium.org> wrote:
> 
>     Hi,
> 
>     On Fri, Dec 4, 2020 at 4:48 PM Stephen Boyd <swboyd@chromium.org> wrote:
>     >
>     > Some cros ECs support a front proximity MKBP event via
>     > 'EC_MKBP_FRONT_PROXIMITY'. Map this to the 'SW_FRONT_PROXIMITY' input
>     > event code so it can be reported up to userspace.
>     >
>     > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>     > Cc: Benson Leung <bleung@chromium.org>
>     > Cc: Guenter Roeck <groeck@chromium.org>
>     > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
>     > ---
>     >  drivers/input/keyboard/cros_ec_keyb.c          | 5 +++++
>     >  include/linux/platform_data/cros_ec_commands.h | 1 +
> 
> 
> For a year or two people kept changes to cros_ec_commands.h separate from
> functional changes. Unfortunately, we are now reverting to the old
> "change cros_ec_commands.h together with some functional change" approach which
> caused an endless amount of trouble and pain for Chrome OS rebases a couple of
> years ago.
> 
> I won't NACK this patch, but it is close.

I'm fine to split it into two, or keep it together. Just let me know
what is preferred. Thanks!

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Input: cros_ec_keyb: Add support for a front proximity switch
  2021-01-07 19:46       ` Dmitry Torokhov
@ 2021-01-15 22:45         ` Stephen Boyd
  2021-02-02 13:43         ` Pavel Machek
  1 sibling, 0 replies; 8+ messages in thread
From: Stephen Boyd @ 2021-01-15 22:45 UTC (permalink / raw)
  To: Dmitry Torokhov, Doug Anderson
  Cc: Enric Balletbo i Serra, Benson Leung, LKML, HID CORE LAYER,
	Guenter Roeck,

Quoting Dmitry Torokhov (2021-01-07 11:46:48)
> 
> Yes, I understand that on some devices the proximity sensors are not
> true sensors but rather on/off signals, potentially derived from a
> multitude of sources. However there is still a benefit in exposing them
> as IIO proximity devices with limited reporting representing
> [near, infinity] range/values. This will mean that userspace needs to
> monitor only one set of devices (IIO) instead of both IIO and input, and
> will not require constantly expanding EV_SW set to account for
> ever-growing number of proximity sensors (lap, palm, general presence,
> etc).

Ok, no problem. I think I'll have to spin up a small driver to handle
this as an IIO proximity device and then register the IIO child device
from the cros ec driver. Let me see how that goes.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Input: cros_ec_keyb: Add support for a front proximity switch
  2021-01-07 19:46       ` Dmitry Torokhov
  2021-01-15 22:45         ` Stephen Boyd
@ 2021-02-02 13:43         ` Pavel Machek
  1 sibling, 0 replies; 8+ messages in thread
From: Pavel Machek @ 2021-02-02 13:43 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Doug Anderson, Stephen Boyd, Enric Balletbo i Serra,
	Benson Leung, LKML, open list:HID CORE LAYER, Guenter Roeck

[-- Attachment #1: Type: text/plain, Size: 2459 bytes --]

Hi!

> > > > Reviewed-by: Douglas Anderson <dianders@chromium.org>
> > > >
> > > > Given that it touches a header file owned by the Chrome OS maintainers
> > > > and a driver owned by input, how should it land?  One maintainer Acks
> > > > and the other lands?
> > >
> > > Sorry about missing this one, however the "front proximity" switch has
> > > been introduced for the benefit of phone devices, to be emitted when a
> > > device is raised to user's ear, and I do not think we should be using
> > > this here.
> > >
> > > We have just recently had similar discussion with regard to palm- and
> > > lap-mode sensors and whether they should be reported over input or IIO
> > > as true proximity sensors:
> > >
> > > https://lore.kernel.org/linux-iio/9f9b0ff6-3bf1-63c4-eb36-901cecd7c4d9@redhat.com/
> > >
> > > Based on what we are doing for other Chrome OS devices that expose
> > > proximity sensors (for example trogdor) we have decided that we all
> > > should be using IIO as it will allow not only on/off, but true proximity
> > > reporting with potential of implementing smarter policies by userspace.
> > >
> > > Because of that we should do the same here and export this as IIO
> > > proximity sensor as well.
> > 
> > For devices with a true proximity sensor that's exactly what we're
> > doing.  I've only been involved in the periphery of the discussion,
> > but as I understand it there are some models of laptop for which we
> > don't have a true proximity sensor.  On these devices, the EC is in
> > charge of deciding about proximity based on a number of factors.
> 
> Yes, I understand that on some devices the proximity sensors are not
> true sensors but rather on/off signals, potentially derived from a
> multitude of sources. However there is still a benefit in exposing them
> as IIO proximity devices with limited reporting representing
> [near, infinity] range/values. This will mean that userspace needs to
> monitor only one set of devices (IIO) instead of both IIO and input, and
> will not require constantly expanding EV_SW set to account for
> ever-growing number of proximity sensors (lap, palm, general presence,
> etc).

While I believe one set of devices is good goal, I don't think IIO is
good solution here. It is being used for user input after
all... Routing on/off values to IIO is strange.

Best regards,
									Pavel

-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2021-02-02 13:47 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-05  0:47 [PATCH] Input: cros_ec_keyb: Add support for a front proximity switch Stephen Boyd
2021-01-07  1:16 ` Doug Anderson
2021-01-07  2:21   ` Dmitry Torokhov
2021-01-07 14:57     ` Doug Anderson
2021-01-07 19:46       ` Dmitry Torokhov
2021-01-15 22:45         ` Stephen Boyd
2021-02-02 13:43         ` Pavel Machek
     [not found]   ` <CABXOdTcT4f_mg=ukPd0sD90o-aKg3qgiuLDRNPU8SUuFnFbRxA@mail.gmail.com>
2021-01-15 22:43     ` Stephen Boyd

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.