| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <4365cae6-33e2-4b86-aab9-0b9ad112e6b0@redhat.com>
Date: Sat, 28 Sep 2024 17:53:44 +0200
From: Hans de Goede <hdegoede@...hat.com>
To: Maya Matuszczyk <maccraft123mc@...il.com>,
Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
Bryan O'Donoghue <bryan.odonoghue@...aro.org>
Cc: linux-kernel@...r.kernel.org, platform-driver-x86@...r.kernel.org
Subject: Re: [PATCH 2/3] platform: arm64: Add driver for Lenovo Yoga Slim 7x's
EC
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@...il.com>
> ---
> 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@...il.com>
> 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@...hat.com>
> 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@...il.com>
> + */
> +#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");
Powered by blists - more mailing lists