mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
From: Rong Zhang <i@rong.moe>
To: "Marco Giunta" <marco_giunta@outlook.it>,
	"Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>,
	"Mark Pearson" <mpearson-lenovo@squebb.ca>,
	"Hans de Goede" <hansg@kernel.org>
Cc: Huang Wei <huangwei@kylinos.cn>,
	"Derek J . Clark" <derekjohn.clark@gmail.com>,
	Ike Panhc <ikepanhc@gmail.com>,
	 platform-driver-x86@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] platform/x86: ideapad-laptop: Report camera switch as SW_CAMERA_LENS_COVER
Date: Mon, 21 Sep 2026 23:03:04 +0800	[thread overview]
Message-ID: <7ff9f813abc2ebba8a7a9618b68ed3fde7022a60.camel@rong.moe> (raw)
In-Reply-To: <d867a4f6b798809776164542972e68d724f29fd5.camel@rong.moe>

Hi Marco,

On Mon, 2026-09-21 at 22:50 +0800, Rong Zhang wrote:
> Hi Macro,

Whoops, I mistyped your name. Sorry for that. My fingers definitely have
their own thoughts.

Thanks,
Rong

> 
> On Sun, 2026-09-20 at 22:28 +0200, Marco Giunta wrote:
> > On certain 2025 Lenovo laptops, such as the Yoga Pro 7 14ASP10 and Legion
> > Pro 7 16AFR10H, a switch on the side disables the camera. This is handled
> > by the firmware, which also sends event 0x0d (camera disabled) or 0x0c
> > (camera enabled) through the WMI event GUID handled with
> > ideapad_wmi_context_fn_keys. These events are currently reported as
> > KEY_UNKNOWN.
> > 
> > Other Lenovo laptops expose a similar camera switch through a separate WMI
> > GUID, which lenovo-wmi-camera reports as SW_CAMERA_LENS_COVER since
> > commit d98bf6a6ed61 ("platform/x86: lenovo-wmi-camera: Use
> > SW_CAMERA_LENS_COVER instead of KEY_CAMERA_ACESS"). That driver does not
> > bind on these laptops, so do the same here: as the switch state cannot be
> > read from the firmware, register a separate input device with the switch
> > on the first event, and report the state carried by each event.
> > 
> > Signed-off-by: Marco Giunta <marco_giunta@outlook.it>
> > ---
> > Changes in v2:
> > 
> > * Add lockdep_assert_held(&ideapad_shared_mutex) and a comment at the top
> >   of ideapad_camera_switch_report() to document and check that the WMI
> >   notify path is serialized through ideapad_shared_mutex (suggested by
> >   Huang Wei)
> > * Replace dev_warn() with dev_warn_once() in
> >   ideapad_camera_switch_report() so that a persistent registration failure
> >   does not log a warning on every toggle (suggested by Huang Wei)
> > 
> > Link to v1: https://lore.kernel.org/platform-driver-x86/SN6PR19MB23039910DD1918BEE3825BAFFC862@SN6PR19MB2303.namprd19.prod.outlook.com/
> > 
> > Notes (copied verbatim from v1, to keep the context easily accessible):
> > 
> > * I considered a couple of alternatives to the approach in this patch:
> > 
> >   1.  Simply map 0x0c and 0x0d to KEY_CAMERA_ACCESS_ENABLE/DISABLE:
> > 
> >       { KE_KEY,	0x0c | IDEAPAD_WMI_KEY, { KEY_CAMERA_ACCESS_ENABLE } },
> >       { KE_KEY,	0x0d | IDEAPAD_WMI_KEY, { KEY_CAMERA_ACCESS_DISABLE } },
> > 
> >       similar to the touchpad off/on hkey events (66/67) this driver emits
> >       after the firmware has toggled the touchpad state. However, HUTRR72
> >       describes these hkey events as asserting a camera access state for
> >       the host to apply, rather than reporting one the firmware has
> >       already applied. 
> > 
> 
> Well, it really doesn't matter how the specification says; what matters
> is how devices in reality do: whenever the HID Usage Table doesn't define
> a usage for hardware/firmware to report the already-applied value but
> defines a usage for notifying the host to apply a specific value, a lot
> of devices will emerge, using the latter even when the former is
> intended.
> 
> For example, many USB Audio Class devices come with volume control
> buttons or knobs. Some of them tune the volume themselves as well as
> emitting Volume Increment/Decrement HID events.
> 
> In this case, the audio stack, in response to the HID events,
> intentionally overrides the volume set by hardware, effectively
> synchronizing the hardware and software volume value.
> 
> The pattern has become so common that many devices like that also
> implement an internal timeout mechanism. They only set the volume
> themselves when reaching the timeout with no UAC volume control request
> received.
> 
> IOW, it doesn't matter whether the hardware/firmware has done its job as
> long as the subsequent software control is harmless. That's why we can
> safely use KEY_TOUCHPAD_ON/OFF even if the firmware has toggled the
> touchpad state. They cause the desktop environment to mask/unmask
> touchpad input, which is essentially a no-op (thus a safe operation) when
> the firmware has already disabled/enabled the touchpad.
> 
> Fundamentally speaking, using KEY_CAMERA_ACCESS_ENABLE/DISABLE is also
> OK. It's just...
> 
> > Furthermore, lenovo-wmi-camera moved away from
> >       these hkey events in favor of SW_CAMERA_LENS_COVER in the quoted
> >       commit, which is arguably the closest relevant precedent.
> 
> ...relatively new and isn't widely used compared to SW_CAMERA_LENS_COVER.
> 
> Thus, I agreed that using SW_CAMERA_LENS_COVER is a better approach as it
> aligns with other drivers' behavior. The patch seems over-engineered
> though, see below.
> 
> >     
> >   2.  Ignore these events like the firmware-handled FnLock events:
> > 
> > 			{ KE_IGNORE,	0x0c | IDEAPAD_WMI_KEY },
> > 			{ KE_IGNORE,	0x0d | IDEAPAD_WMI_KEY },
> > 
> >       This fixes the KEY_UNKNOWN issue, but userspace gets no information.
> >     
> >   Overall, in the end I copied the approach of the lenovo-wmi-camera
> >   driver. Kindly let me know if there are other/better solutions.
> >     
> > * Unlike the touchpad, whose state this driver reads with VPCCMD_R_TOUCHPAD
> >   at probe, on resume and on each event, 
> > 
> 
> At least for the input device, the synchronization on probe or on resume
> does not matter, as no input event is emitted due to !send_events.
> 
> > the camera switch state is only
> >   available in EC-private fields, and the two tested laptops even use
> >   different ones. Hence the lenovo-wmi-camera approach: the switch appears
> >   on the first event, and a change made while suspended is not reported,
> >   leaving the state wrong until the switch is toggled again.
> 
> So the difference compared to the lenovo-wmi-camera approach is only
> about when and how the input device is registered.
> 
> On recent models, the firmware neither emits VPCCMD_R_TOUCHPAD nor
> toggles the touchpad. It leaves the job to software by emitting WMI key
> event 0x29, which is converted to KEY_TOUCHPAD_TOGGLE via ideapad_keymap.
> It'd be tidier to take that approach and reuse the existing input device.
> Sparse keymap supports KEY_SW, so it's viable to map the two WMI events
> to SW_CAMERA_LENS_COVER with appropriate switch value.
> 
> I've attached a patch implementing the approach. Could you test if it
> works on your devices? If so, you may submit my patch yourself by adding
> a From: pseudo header with my identity and swapping the two Signed-off-
> by: tags. I can also submit it myself as long as you permit me to do so.
> 
> > 
> > * For reference, on KDE Plasma 6.7.5, using the Legion's touchpad toggle
> >   button shows an OSD, but nothing on a SW_CAMERA_LENS_COVER change or if
> >   sending KEY_CAMERA_ACCESS_ENABLE/DISABLE events. I am not sure whether
> >   this is simply a case of "not implemented yet" or if I misunderstood how
> >   these events are supposed to be used to communicate with userspace.
> >   Please feel free to correct me here.
> 
> I believe KDE Plasma has not implemented it yet. You may raise an issue
> on their Bugzilla.
> 
> Thanks,
> Rong
> 
> >   
> > Best regards,
> > Marco
> > ---
> >  drivers/platform/x86/lenovo/ideapad-laptop.c | 59 ++++++++++++++++++++
> >  1 file changed, 59 insertions(+)
> > 
> > diff --git a/drivers/platform/x86/lenovo/ideapad-laptop.c b/drivers/platform/x86/lenovo/ideapad-laptop.c
> > index 207368f5d489..0066638ee332 100644
> > --- a/drivers/platform/x86/lenovo/ideapad-laptop.c
> > +++ b/drivers/platform/x86/lenovo/ideapad-laptop.c
> > @@ -178,6 +178,7 @@ struct ideapad_private {
> >  	struct ideapad_rfk_priv rfk_priv[IDEAPAD_RFKILL_DEV_NUM];
> >  	struct platform_device *platform_device;
> >  	struct input_dev *inputdev;
> > +	struct input_dev *camera_switch_idev;
> >  	struct backlight_device *blightdev;
> >  	struct ideapad_dytc_priv *dytc;
> >  	struct dentry *debug;
> > @@ -1410,10 +1411,43 @@ static int ideapad_input_init(struct ideapad_private *priv)
> >  	return err;
> >  }
> >  
> > +static int ideapad_camera_switch_init(struct ideapad_private *priv, bool covered)
> > +{
> > +	struct input_dev *idev;
> > +	int err;
> > +
> > +	idev = input_allocate_device();
> > +	if (!idev)
> > +		return -ENOMEM;
> > +
> > +	idev->name = "Ideapad Camera Switch";
> > +	idev->phys = "ideapad/input1";
> > +	idev->id.bustype = BUS_HOST;
> > +	idev->dev.parent = &priv->platform_device->dev;
> > +
> > +	input_set_capability(idev, EV_SW, SW_CAMERA_LENS_COVER);
> > +	input_report_switch(idev, SW_CAMERA_LENS_COVER, covered);
> > +	input_sync(idev);
> > +
> > +	err = input_register_device(idev);
> > +	if (err) {
> > +		input_free_device(idev);
> > +		return err;
> > +	}
> > +
> > +	priv->camera_switch_idev = idev;
> > +	return 0;
> > +}
> > +
> >  static void ideapad_input_exit(struct ideapad_private *priv)
> >  {
> >  	input_unregister_device(priv->inputdev);
> >  	priv->inputdev = NULL;
> > +
> > +	if (priv->camera_switch_idev) {
> > +		input_unregister_device(priv->camera_switch_idev);
> > +		priv->camera_switch_idev = NULL;
> > +	}
> >  }
> >  
> >  static void ideapad_input_report(struct ideapad_private *priv,
> > @@ -1422,6 +1456,22 @@ static void ideapad_input_report(struct ideapad_private *priv,
> >  	sparse_keymap_report_event(priv->inputdev, scancode, 1, true);
> >  }
> >  
> > +static void ideapad_camera_switch_report(struct ideapad_private *priv, bool covered)
> > +{
> > +	/* The WMI notify path is serialized through ideapad_shared_mutex */
> > +	lockdep_assert_held(&ideapad_shared_mutex);
> > +
> > +	if (!priv->camera_switch_idev) {
> > +		if (ideapad_camera_switch_init(priv, covered))
> > +			dev_warn_once(&priv->platform_device->dev,
> > +				      "Failed to register camera switch input device\n");
> > +		return;
> > +	}
> > +
> > +	input_report_switch(priv->camera_switch_idev, SW_CAMERA_LENS_COVER, covered);
> > +	input_sync(priv->camera_switch_idev);
> > +}
> > +
> >  static void ideapad_input_novokey(struct ideapad_private *priv)
> >  {
> >  	unsigned long long_pressed;
> > @@ -2312,6 +2362,15 @@ static void ideapad_wmi_notify(struct wmi_device *wdev, union acpi_object *data)
> >  			break;
> >  		}
> >  
> > +		/*
> > +		 * Camera switch, handled by the firmware:
> > +		 * 0x0c camera enabled, 0x0d camera disabled
> > +		 */
> > +		if (data->integer.value == 0x0c || data->integer.value == 0x0d) {
> > +			ideapad_camera_switch_report(priv, data->integer.value == 0x0d);
> > +			break;
> > +		}
> > +
> >  		/* 0x02 FnLock, 0x03 Esc */
> >  		if (data->integer.value == 0x02 || data->integer.value == 0x03)
> >  			ideapad_fn_lock_led_notify(priv, data->integer.value == 0x02);
> > 
> > base-commit: f475845eaf3d749114a63270bf2efea459e14dd2

  reply	other threads:[~2026-09-21 15:08 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-09-19  8:41 [PATCH] " Marco Giunta
2026-09-20  6:42 ` Huang Wei
2026-09-20  9:58   ` Marco Giunta
2026-09-20 10:29     ` Huang Wei
2026-09-20 10:58       ` Marco Giunta
2026-09-20 20:28 ` [PATCH v2] " Marco Giunta
2026-09-21  2:36   ` Huang Wei
2026-09-21 14:50   ` Rong Zhang
2026-09-21 15:03     ` Rong Zhang [this message]
2026-09-21 19:00     ` Marco Giunta
2026-09-22 11:08       ` Huang Wei
2026-09-22 12:18       ` Rong Zhang
2026-09-22 15:52         ` Mark Pearson
2026-09-22 15:58           ` Rong Zhang
2026-09-22 16:42             ` Rong Zhang
2026-09-22 17:32               ` Mark Pearson
2026-09-22 18:47                 ` Rong Zhang
2026-09-23 11:17                   ` Marco Giunta
2026-09-23 12:39                     ` Rong Zhang
2026-09-23 13:11                       ` Marco Giunta

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=7ff9f813abc2ebba8a7a9618b68ed3fde7022a60.camel@rong.moe \
    --to=i@rong.moe \
    --cc=derekjohn.clark@gmail.com \
    --cc=hansg@kernel.org \
    --cc=huangwei@kylinos.cn \
    --cc=ikepanhc@gmail.com \
    --cc=ilpo.jarvinen@linux.intel.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=marco_giunta@outlook.it \
    --cc=mpearson-lenovo@squebb.ca \
    --cc=platform-driver-x86@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®