Re: [PATCH v2 2/2] platform/x86: Add driver for Yoga Tablet Mode switch

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi


2023. március 23., csütörtök 3:52 keltezéssel, Andrew Kallmeyer <kallmeyeras@xxxxxxxxx> írta:

> From: Gergo Koteles <soyer@xxxxxx>
> 
> This WMI driver for the tablet mode control switch for Lenovo Yoga
> notebooks was originally written by Gergo Koteles. The mode is mapped to
> a SW_TABLET_MODE switch capable input device.
> 
> Andrew followed the suggestions that were posted in reply to Gergo's RFC
> patch, and on the v1 version of this patch to follow-up and get it
> merged.
> 
> Changes from Gergo's RFC:
> 
>  - Refactored obtaining a reference to the EC ACPI device needed for the
>    quirk implementation as suggested by Hans de Goede
>  - Applied small fixes and switched to devm_input_allocate_device() and
>    removing input_unregister_device() as suggested by Barnabás Pőcze.
>  - Merged the lenovo_ymc_trigger_ec function with the
>    ideapad_trigger_ymc_next_read function since it was no longer
>    external.
>  - Added the word "Tablet" to the driver description to hopefully make
>    it more clear.
>  - Fixed the LENOVO_YMC_QUERY_METHOD ID and the name string for the EC
>    APCI device trigged for the quirk
>  - Triggered the input event on probe so that the initial tablet mode
>    state when the driver is loaded is reported to userspace as suggested
>    by Armin Wolf.
> 
> We have tested this on the Yoga 7 14AIL7 for the non-quirk path and on
> the Yoga 7 14ARB7 which has the firmware bug that requires triggering
> the embedded controller to send the mode change events. This workaround
> is also used by the Windows drivers.
> 
> According to reports at https://github.com/lukas-w/yoga-usage-mode,
> which uses the same WMI devices, the following models should also work:
> Yoga C940, Ideapad flex 14API, Yoga 9 14IAP7, Yoga 7 14ARB7, etc.
> 
> Signed-off-by: Gergo Koteles <soyer@xxxxxx>
> Co-developed-by: Andrew Kallmeyer <kallmeyeras@xxxxxxxxx>
> Signed-off-by: Andrew Kallmeyer <kallmeyeras@xxxxxxxxx>
> Link: https://lore.kernel.org/r/20221004214332.35934-1-soyer@xxxxxx/
> Link: https://lore.kernel.org/platform-driver-x86/20230310041726.217447-1-kallmeyeras@xxxxxxxxx/
> ---
> [...]
> +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)
> +{
> +	int err;
> +
> +	if (!ec_trigger || !priv || !priv->ec_acpi_dev)

I think just the `!priv->ec_acpi_dev` check is sufficient.


> +		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);
> +}
> [...]
> +static int lenovo_ymc_probe(struct wmi_device *wdev, const void *ctx)
> +{
> +	struct input_dev *input_dev;
> +	struct lenovo_ymc_private *priv;
> +	int err;
> +
> +	ec_trigger |= dmi_check_system(ec_trigger_quirk_dmi_table);
> +
> +	priv = devm_kzalloc(&wdev->dev, sizeof(*priv), GFP_KERNEL);
> +	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);

The reference is leaked in case of an error. Use `devm_add_action_or_reset()`:

  static void acpi_dev_put_helper(void *p) { acpi_dev_put(p); }
  [...]
  priv->ec_acpi_dev = acpi_dev_get_first_match_dev("VPC2004", NULL, -1);
  if (!priv->ec_acpi_dev)
    ...
  err = devm_add_action_or_reset(&wdev->dev, acpi_dev_put_helper, priv->ec_acpi_dev)
  if (err)
    ...

And then you can remove `lenovo_ymc_remove()` altogether.


> +		if (!priv->ec_acpi_dev) {
> +			dev_err(&wdev->dev, "Could not find EC ACPI device.\n");
> +			return -ENODEV;
> +		}
> +	}
> +
> +	input_dev = devm_input_allocate_device(&wdev->dev);
> +	if (!input_dev)
> +		return -ENOMEM;
> +
> +	input_dev->name = "Lenovo Yoga Tablet Mode Control switch";
> +	input_dev->phys = LENOVO_YMC_EVENT_GUID "/input0";
> +	input_dev->id.bustype = BUS_HOST;
> +	input_dev->dev.parent = &wdev->dev;
> +
> +	input_set_capability(input_dev, EV_SW, SW_TABLET_MODE);

As far as I can tell `sparse_keymap_setup()` already sets the above.


> +
> +	err = sparse_keymap_setup(input_dev, lenovo_ymc_keymap, NULL);
> +	if (err) {
> +		dev_err(&wdev->dev,
> +			"Could not set up input device keymap: %d\n", err);
> +		return err;
> +	}
> +
> +	err = input_register_device(input_dev);
> +	if (err) {
> +		dev_err(&wdev->dev,
> +			"Could not register input device: %d\n", err);
> +		return err;
> +	}
> +
> +	priv->input_dev = input_dev;
> +	dev_set_drvdata(&wdev->dev, priv);
> +
> +	// Report the state for the first time on probe
> +	lenovo_ymc_trigger_ec(wdev, priv);
> +	lenovo_ymc_notify(wdev, NULL);
> +	return 0;
> +}
> [...]


Regards,
Barnabás Pőcze




[Index of Archives]     [Linux Kernel Development]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux