Hi Maya,
Thank you for your patch. It is great to so people working on supporting
more ARM64 based laptop ECs.
Note not a full review just one remark from a quick scan of the driver.
On 27-Sep-24 8:53 PM, Maya Matuszczyk wrote:
> This patch adds a basic driver for the EC in Qualcomm Snapdragon X1
> Elite-based Lenovo Yoga Slim 7x.
>
> For now it supports only reporting that the AP is going to suspend and
> the microphone mute button, however the EC seems to also support reading
> fan information, other key combinations and thermal data.
>
> Signed-off-by: Maya Matuszczyk <maccraft123mc@xxxxxxxxx>
> ---
> MAINTAINERS | 1 +
> drivers/platform/arm64/Kconfig | 12 ++
> drivers/platform/arm64/Makefile | 1 +
> drivers/platform/arm64/lenovo-yoga-slim7x.c | 202 ++++++++++++++++++++
> 4 files changed, 216 insertions(+)
> create mode 100644 drivers/platform/arm64/lenovo-yoga-slim7x.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 0d4bfdde166d..f689cba80fa3 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12906,6 +12906,7 @@ LENOVO YOGA SLIM 7X EC DRIVER
> M: Maya Matuszczyk <maccraft123mc@xxxxxxxxx>
> S: Maintained
> F: Documentation/devicetree/bindings/platform/lenovo,yoga-slim7x-ec.yaml
> +F: drivers/platform/arm64/lenovo-yoga-slim7x.c
>
> LETSKETCH HID TABLET DRIVER
> M: Hans de Goede <hdegoede@xxxxxxxxxx>
> diff --git a/drivers/platform/arm64/Kconfig b/drivers/platform/arm64/Kconfig
> index f88395ea3376..9ceae50f8b4e 100644
> --- a/drivers/platform/arm64/Kconfig
> +++ b/drivers/platform/arm64/Kconfig
> @@ -49,4 +49,16 @@ config EC_LENOVO_YOGA_C630
>
> Say M or Y here to include this support.
>
> +config EC_LENOVO_YOGA_SLIM7X
> + tristate "Lenovo Yoga Slim 7x Embedded Controller driver"
> + depends on ARCH_QCOM || COMPILE_TEST
> + depends on I2C
> + help
> + Select this option to enable driver for the EC found in the Lenovo
> + Yoga Slim 7x.
> +
> + This driver currently supports reporting input event for microphone
> + mute button, and reporting device suspend to the EC so it can take
> + appropriate actions.
> +
> endif # ARM64_PLATFORM_DEVICES
> diff --git a/drivers/platform/arm64/Makefile b/drivers/platform/arm64/Makefile
> index b2ae9114fdd8..70dfc1fb979d 100644
> --- a/drivers/platform/arm64/Makefile
> +++ b/drivers/platform/arm64/Makefile
> @@ -7,3 +7,4 @@
>
> obj-$(CONFIG_EC_ACER_ASPIRE1) += acer-aspire1-ec.o
> obj-$(CONFIG_EC_LENOVO_YOGA_C630) += lenovo-yoga-c630.o
> +obj-$(CONFIG_EC_LENOVO_YOGA_SLIM7X) += lenovo-yoga-slim7x.o
> diff --git a/drivers/platform/arm64/lenovo-yoga-slim7x.c b/drivers/platform/arm64/lenovo-yoga-slim7x.c
> new file mode 100644
> index 000000000000..8f6d523395bc
> --- /dev/null
> +++ b/drivers/platform/arm64/lenovo-yoga-slim7x.c
> @@ -0,0 +1,202 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2024 Maya Matuszczyk <maya.matuszczyk@xxxxxxxxx>
> + */
> +#include <linux/auxiliary_bus.h>
> +#include <linux/cleanup.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/irqreturn.h>
> +#include <linux/lockdep.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/notifier.h>
> +#include <linux/slab.h>
> +#include <linux/input.h>
> +//#include <linux/platform_data/lenovo-yoga-slim7x.h>
> +
> +// These are the registers that i know about available from SMBUS
> +#define EC_IRQ_REASON_REG 0x05
> +#define EC_SUSPEND_RESUME_REG 0x23
> +#define EC_IRQ_ENABLE_REG 0x35
> +#define EC_BACKLIGHT_STATUS_REG 0x83
> +#define EC_MIC_MUTE_LED_REG 0x84
> +#define EC_AC_STATUS_REG 0x90
> +
> +// Valid values for EC_SUSPEND_RESUME_REG
> +#define EC_NOTIFY_SUSPEND_ENTER 0x01
> +#define EC_NOTIFY_SUSPEND_EXIT 0x00
> +#define EC_NOTIFY_SCREEN_OFF 0x03
> +#define EC_NOTIFY_SCREEN_ON 0x04
> +
> +// These are the values in EC_IRQ_REASON_REG that i could find in DSDT
> +#define EC_IRQ_MICMUTE_BUTTON 0x04
> +#define EC_IRQ_FAN1_STATUS_CHANGE 0x30
> +#define EC_IRQ_FAN2_STATUS_CHANGE 0x31
> +#define EC_IRQ_FAN1_SPEED_CHANGE 0x32
> +#define EC_IRQ_FAN2_SPEED_CHANGE 0x33
> +#define EC_IRQ_COMPLETED_LUT_UPDATE 0x34
> +#define EC_IRQ_COMPLETED_FAN_PROFILE_SWITCH 0x35
> +#define EC_IRQ_THERMISTOR_1_TEMP_THRESHOLD_CROSS 0x36
> +#define EC_IRQ_THERMISTOR_2_TEMP_THRESHOLD_CROSS 0x37
> +#define EC_IRQ_THERMISTOR_3_TEMP_THRESHOLD_CROSS 0x38
> +#define EC_IRQ_THERMISTOR_4_TEMP_THRESHOLD_CROSS 0x39
> +#define EC_IRQ_THERMISTOR_5_TEMP_THRESHOLD_CROSS 0x3a
> +#define EC_IRQ_THERMISTOR_6_TEMP_THRESHOLD_CROSS 0x3b
> +#define EC_IRQ_THERMISTOR_7_TEMP_THRESHOLD_CROSS 0x3c
> +#define EC_IRQ_RECOVERED_FROM_RESET 0x3d
> +#define EC_IRQ_LENOVO_SUPPORT_KEY 0x90
> +#define EC_IRQ_FN_Q 0x91
> +#define EC_IRQ_FN_M 0x92
> +#define EC_IRQ_FN_SPACE 0x93
> +#define EC_IRQ_FN_R 0x94
> +#define EC_IRQ_FNLOCK_ON 0x95
> +#define EC_IRQ_FNLOCK_OFF 0x96
> +#define EC_IRQ_FN_N 0x97
> +#define EC_IRQ_AI 0x9a
> +#define EC_IRQ_NPU 0x9b
> +
> +struct yoga_slim7x_ec {
> + struct i2c_client *client;
> + struct input_dev *idev;
> + struct mutex lock;
> +};
> +
> +static irqreturn_t yoga_slim7x_ec_irq(int irq, void *data)
> +{
> + struct yoga_slim7x_ec *ec = data;
> + struct device *dev = &ec->client->dev;
> + int val;
> +
> + guard(mutex)(&ec->lock);
> +
> + val = i2c_smbus_read_byte_data(ec->client, EC_IRQ_REASON_REG);
> + if (val < 0) {
> + dev_err(dev, "Failed to get EC IRQ reason: %d\n", val);
> + return IRQ_HANDLED;
> + }
> +
> + switch (val) {
> + case EC_IRQ_MICMUTE_BUTTON:
> + input_report_key(ec->idev, KEY_MICMUTE, 1);
> + input_sync(ec->idev);
> + input_report_key(ec->idev, KEY_MICMUTE, 0);
> + input_sync(ec->idev);
> + break;
> + default:
> + dev_info(dev, "Unhandled EC IRQ reason: %d\n", val);
> + }
I strongly suggest to use include/linux/input/sparse-keymap.h functionality
here. This will make adding new keys a lot easier and will allow re-mapping
codes from userspace.
E.g. replace the whole switch-case with:
if (!sparse_keymap_report_event(ec->idev, val, 1, true))
dev_info(dev, "Unhandled EC IRQ reason: %d\n", val);
This takes care of mapping val -> KEY_MICMUTE, and doing all
the reporting (after calling sparse_keymap_setup() with an appropriate
keymap from probe()).
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int yoga_slim7x_ec_probe(struct i2c_client *client)
> +{
> + struct device *dev = &client->dev;
> + struct yoga_slim7x_ec *ec;
> + int ret;
> +
> + ec = devm_kzalloc(dev, sizeof(*ec), GFP_KERNEL);
> + if (!ec)
> + return -ENOMEM;
> +
> + mutex_init(&ec->lock);
> + ec->client = client;
> +
> + ec->idev = devm_input_allocate_device(dev);
> + if (!ec->idev)
> + return -ENOMEM;
> + ec->idev->name = "yoga-slim7x-ec";
> + ec->idev->phys = "yoga-slim7x-ec/input0";
> + input_set_capability(ec->idev, EV_KEY, KEY_MICMUTE);
Same here, please use sparse_keymap_setup() here to have it
setup capabilities for all keys in your (to be defined)
const struct key_entry yoga_slim7x_ec_keymap[]
This way you only need to add new mappings in the keymap
and then both the capability setting as well as the reporting
in the irq-handler will be taken care of by the sparse-keymap
helpers.
Other then that the overall structure of the driver looks good
(again this is not a full review, just a quick scan).
Regards,
Hans
> +
> + ret = input_register_device(ec->idev);
> + if (ret < 0)
> + return dev_err_probe(dev, ret, "Failed to register input device\n");
> +
> + ret = devm_request_threaded_irq(dev, client->irq,
> + NULL, yoga_slim7x_ec_irq,
> + IRQF_ONESHOT, "yoga_slim7x_ec", ec);
> + if (ret < 0)
> + return dev_err_probe(dev, ret, "Unable to request irq\n");
> +
> + ret = i2c_smbus_write_byte_data(client, EC_IRQ_ENABLE_REG, 0x01);
> + if (ret < 0)
> + return dev_err_probe(dev, ret, "Failed to enable interrupts\n");
> +
> + return 0;
> +}
> +
> +static void yoga_slim7x_ec_remove(struct i2c_client *client)
> +{
> + struct device *dev = &client->dev;
> + int ret;
> +
> + ret = i2c_smbus_write_byte_data(client, EC_IRQ_ENABLE_REG, 0x00);
> + if (ret < 0)
> + dev_err(dev, "Failed to disable interrupts: %d\n", ret);
> +}
> +
> +static int yoga_slim7x_ec_suspend(struct device *dev)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + int ret;
> +
> + ret = i2c_smbus_write_byte_data(client, EC_SUSPEND_RESUME_REG, EC_NOTIFY_SCREEN_OFF);
> + if (ret)
> + return ret;
> +
> + ret = i2c_smbus_write_byte_data(client, EC_SUSPEND_RESUME_REG, EC_NOTIFY_SUSPEND_ENTER);
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
> +
> +static int yoga_slim7x_ec_resume(struct device *dev)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + int ret;
> +
> + ret = i2c_smbus_write_byte_data(client, EC_SUSPEND_RESUME_REG, EC_NOTIFY_SUSPEND_EXIT);
> + if (ret)
> + return ret;
> +
> + ret = i2c_smbus_write_byte_data(client, EC_SUSPEND_RESUME_REG, EC_NOTIFY_SCREEN_ON);
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
> +
> +static const struct of_device_id yoga_slim7x_ec_of_match[] = {
> + { .compatible = "lenovo,yoga-slim7x-ec" },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, yoga_slim7x_ec_of_match);
> +
> +static const struct i2c_device_id yoga_slim7x_ec_i2c_id_table[] = {
> + { "yoga-slim7x-ec", },
> + {}
> +};
> +MODULE_DEVICE_TABLE(i2c, yoga_slim7x_ec_i2c_id_table);
> +
> +static DEFINE_SIMPLE_DEV_PM_OPS(yoga_slim7x_ec_pm_ops,
> + yoga_slim7x_ec_suspend,
> + yoga_slim7x_ec_resume);
> +
> +static struct i2c_driver yoga_slim7x_ec_i2c_driver = {
> + .driver = {
> + .name = "yoga-slim7x-ec",
> + .of_match_table = yoga_slim7x_ec_of_match,
> + .pm = &yoga_slim7x_ec_pm_ops
> + },
> + .probe = yoga_slim7x_ec_probe,
> + .remove = yoga_slim7x_ec_remove,
> + .id_table = yoga_slim7x_ec_i2c_id_table,
> +};
> +module_i2c_driver(yoga_slim7x_ec_i2c_driver);
> +
> +MODULE_DESCRIPTION("Lenovo Yoga Slim 7x Embedded Controller");
> +MODULE_LICENSE("GPL");
