Re: [PATCH 2/3] platform: arm64: Add driver for Lenovo Yoga Slim 7x's EC

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

 



Hi, thanks for taking a look at this driver.

sob., 28 wrz 2024 o 17:53 Hans de Goede <hdegoede@xxxxxxxxxx> 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@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())

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");
>





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

  Powered by Linux