Re: [PATCH 3/4] platform/x86: ideapad-laptop: move ymc_trigger_ec from lenovo-ymc

[Ilpo Järvinen]

On Tue, 9 Jul 2024, Gergo Koteles wrote:

> Some models need to trigger the EC for the yoga mode control to work
> properly. EC triggering consists of a VPC call that needs to be
> synchronized. The vpc_mutex is in the ideapad-laptop module, so
> synchronization is easier there.
> 
> Move the ymc_trigger_ec function to the ideapad-laptop module.
> 
> Signed-off-by: Gergo Koteles <soyer@xxxxxx>
> ---
>  drivers/platform/x86/Kconfig          |  1 +
>  drivers/platform/x86/ideapad-laptop.c | 18 ++++++++++++++
>  drivers/platform/x86/ideapad-laptop.h |  2 ++
>  drivers/platform/x86/lenovo-ymc.c     | 34 ++++-----------------------
>  4 files changed, 26 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 665fa9524986..ddfccc226751 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -477,6 +477,7 @@ config LENOVO_YMC
>  	tristate "Lenovo Yoga Tablet Mode Control"
>  	depends on ACPI_WMI
>  	depends on INPUT
> +	depends on IDEAPAD_LAPTOP
>  	select INPUT_SPARSEKMAP
>  	help
>  	  This driver maps the Tablet Mode Control switch to SW_TABLET_MODE input
> diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
> index 464cc5fd0088..4dcdbb153dda 100644
> --- a/drivers/platform/x86/ideapad-laptop.c
> +++ b/drivers/platform/x86/ideapad-laptop.c
> @@ -1615,6 +1615,24 @@ static void ideapad_sync_touchpad_state(struct ideapad_private *priv, bool send_
>  	priv->r_touchpad_val = value;
>  }
>  
> +void ideapad_ymc_trigger_ec(void)
> +{
> +	struct ideapad_private *priv;
> +	int ret;
> +
> +	guard(mutex)(&ideapad_shared_mutex);
> +
> +	priv = ideapad_shared;
> +	if (!priv)
> +		return;
> +
> +	scoped_guard(mutex, &priv->vpc_mutex)

It feels the ordering in this series is off because patch 2 add the 
mutex and here you add it into yet another function. If this patch 
would be before the other, the mutex changes could all go into the same 
patch.

> +		ret = write_ec_cmd(priv->adev->handle, VPCCMD_W_YMC, 1);
> +	if (ret)
> +		dev_warn(&priv->platform_device->dev, "Could not write YMC: %d\n", ret);
> +}
> +EXPORT_SYMBOL_GPL(ideapad_ymc_trigger_ec);

This export should be namespaced.

I'll have to say I can accept this approach only grudgingly... it feels 
off even if I understand why you have to do this cross driver thing.

-- 
 i.

>  static void ideapad_acpi_notify(acpi_handle handle, u32 event, void *data)
>  {
>  	struct ideapad_private *priv = data;
> diff --git a/drivers/platform/x86/ideapad-laptop.h b/drivers/platform/x86/ideapad-laptop.h
> index 4498a96de597..4dcbed842b0b 100644
> --- a/drivers/platform/x86/ideapad-laptop.h
> +++ b/drivers/platform/x86/ideapad-laptop.h
> @@ -13,6 +13,8 @@
>  #include <linux/jiffies.h>
>  #include <linux/errno.h>
>  
> +void ideapad_ymc_trigger_ec(void);
> +
>  enum {
>  	VPCCMD_R_VPC1 = 0x10,
>  	VPCCMD_R_BL_MAX,
> diff --git a/drivers/platform/x86/lenovo-ymc.c b/drivers/platform/x86/lenovo-ymc.c
> index e1fbc35504d4..0819e9823ca9 100644
> --- a/drivers/platform/x86/lenovo-ymc.c
> +++ b/drivers/platform/x86/lenovo-ymc.c
> @@ -62,19 +62,14 @@ static const struct dmi_system_id allowed_chasis_types_dmi_table[] = {
>  
>  struct lenovo_ymc_private {
>  	struct input_dev *input_dev;
> -	struct acpi_device *ec_acpi_dev;
>  };
>  
> -static void lenovo_ymc_trigger_ec(struct wmi_device *wdev, struct lenovo_ymc_private *priv)
> +static void lenovo_ymc_trigger_ec(void)
>  {
> -	int err;
> -
> -	if (!priv->ec_acpi_dev)
> +	if (!ec_trigger)
>  		return;
>  
> -	err = write_ec_cmd(priv->ec_acpi_dev->handle, VPCCMD_W_YMC, 1);
> -	if (err)
> -		dev_warn(&wdev->dev, "Could not write YMC: %d\n", err);
> +	ideapad_ymc_trigger_ec();
>  }
>  
>  static const struct key_entry lenovo_ymc_keymap[] = {
> @@ -125,11 +120,9 @@ static void lenovo_ymc_notify(struct wmi_device *wdev, union acpi_object *data)
>  
>  free_obj:
>  	kfree(obj);
> -	lenovo_ymc_trigger_ec(wdev, priv);
> +	lenovo_ymc_trigger_ec();
>  }
>  
> -static void acpi_dev_put_helper(void *p) { acpi_dev_put(p); }
> -
>  static int lenovo_ymc_probe(struct wmi_device *wdev, const void *ctx)
>  {
>  	struct lenovo_ymc_private *priv;
> @@ -149,23 +142,6 @@ static int lenovo_ymc_probe(struct wmi_device *wdev, const void *ctx)
>  	if (!priv)
>  		return -ENOMEM;
>  
> -	if (ec_trigger) {
> -		pr_debug("Lenovo YMC enable EC triggering.\n");
> -		priv->ec_acpi_dev = acpi_dev_get_first_match_dev("VPC2004", NULL, -1);
> -
> -		if (!priv->ec_acpi_dev) {
> -			dev_err(&wdev->dev, "Could not find EC ACPI device.\n");
> -			return -ENODEV;
> -		}
> -		err = devm_add_action_or_reset(&wdev->dev,
> -				acpi_dev_put_helper, priv->ec_acpi_dev);
> -		if (err) {
> -			dev_err(&wdev->dev,
> -				"Could not clean up EC ACPI device: %d\n", err);
> -			return err;
> -		}
> -	}
> -
>  	input_dev = devm_input_allocate_device(&wdev->dev);
>  	if (!input_dev)
>  		return -ENOMEM;
> @@ -192,7 +168,7 @@ static int lenovo_ymc_probe(struct wmi_device *wdev, const void *ctx)
>  	dev_set_drvdata(&wdev->dev, priv);
>  
>  	/* Report the state for the first time on probe */
> -	lenovo_ymc_trigger_ec(wdev, priv);
> +	lenovo_ymc_trigger_ec();
>  	lenovo_ymc_notify(wdev, NULL);
>  	return 0;
>  }
>