| 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: <CAO_MupJTdo8+cpx2DHtC47WDQsFRVnB7xQFOmHiEQQzVmkFywg@mail.gmail.com>
Date: Sat, 28 Sep 2024 20:04:29 +0200
From: Maya Matuszczyk <maccraft123mc@...il.com>
To: Hans de Goede <hdegoede@...hat.com>
Cc: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
"Bryan O'Donoghue" <bryan.odonoghue@...aro.org>, 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, thanks for taking a look at this driver.
sob., 28 wrz 2024 o 17:53 Hans de Goede <hdegoede@...hat.com> napisał(a):
>
> 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())
Thank you for the suggestion. I'm not sure how to handle the non-key
related IRQs, like fan status changes.
Do you think they should be filtered out like this:
if (val == 0x04 || (val >= 0x90 && val <= 0x97))
Best Regards,
Maya Matuszczyk.
>
>
>
> > +
> > + 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