Hi,
On 28-Sep-24 8:04 PM, Maya Matuszczyk wrote:
> 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))
Yes drivers like this typically first check for special values
like above and then either do an early "return" or have
a structure like this:
if (val == special)
...
else if (val == other-special)
...
else {
if (!sparse_keymap_report_event(ec->idev, val, 1, true))
dev_info(dev, "Unhandled EC IRQ reason: %d\n", val);
}
The top-level if () ... else if () ... else may also
be replaced with a switch () { case : ... case : ... }.
Note that if you just want to ignore the values you can also
do this in the keymap and generally speaking that is better,
only adding special cases like above if the driver actually
needs to undertake some special action in response to the IRQ.
static const struct key_entry keymap[] = {
{ KE_KEY, EC_IRQ_MICMUTE_BUTTON, { KEY_MICMUTE } },
{ KE_IGNORE, 0x04, { KEY_RESERVED } }, /* Fan status change */
{ KE_IGNORE, 0x90, { KEY_RESERVED } }, /* Fan status change */
{ KE_IGNORE, 0x91, { KEY_RESERVED } }, /* Fan status change */
{ KE_IGNORE, 0x92, { KEY_RESERVED } }, /* Fan status change */
{ KE_IGNORE, 0x93, { KEY_RESERVED } }, /* Fan status change */
{ KE_IGNORE, 0x94, { KEY_RESERVED } }, /* Fan status change */
{ KE_IGNORE, 0x95, { KEY_RESERVED } }, /* Fan status change */
{ KE_IGNORE, 0x96, { KEY_RESERVED } }, /* Fan status change */
{ KE_IGNORE, 0x97, { KEY_RESERVED } }, /* Fan status change */
{ KE_END }
};
In this case with the 0x90 - 0x97 range that is not really pretty,
but it probably still is best to keep the code consistent.
Regards,
Hans
