| 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
| ||
|
Date: Wed, 29 May 2024 09:08:38 +0100
From: Bryan O'Donoghue <bryan.odonoghue@...aro.org>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
Cc: Sebastian Reichel <sre@...nel.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
<conor+dt@...nel.org>, Bjorn Andersson <andersson@...nel.org>,
Hans de Goede <hdegoede@...hat.com>,
Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Konrad Dybcio <konrad.dybcio@...aro.org>, linux-pm@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
platform-driver-x86@...r.kernel.org, linux-usb@...r.kernel.org,
linux-arm-msm@...r.kernel.org, Nikita Travkin <nikita@...n.ru>
Subject: Re: [PATCH v4 2/6] platform: arm64: add Lenovo Yoga C630 WOS EC
driver
On 29/05/2024 00:56, Dmitry Baryshkov wrote:
> On Wed, May 29, 2024 at 12:51:04AM +0100, Bryan O'Donoghue wrote:
>> On 28/05/2024 21:44, Dmitry Baryshkov wrote:
>>> Lenovo Yoga C630 WOS is a laptop using Snapdragon 850 SoC. Like many
>>> laptops it uses embedded controller (EC) to perform various platform
>>
>> an embedded controller
>>
>>> operations, including, but not limited, to Type-C port control or power
>>> supply handlng.
>>>
>>> Add the driver for the EC, that creates devices for UCSI and power
>>> supply devices.
>>>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
>>> ---
>>> drivers/platform/arm64/Kconfig | 14 ++
>>> drivers/platform/arm64/Makefile | 1 +
>>> drivers/platform/arm64/lenovo-yoga-c630.c | 279 +++++++++++++++++++++++++
>>> include/linux/platform_data/lenovo-yoga-c630.h | 42 ++++
>>> 4 files changed, 336 insertions(+)
>>>
>>> diff --git a/drivers/platform/arm64/Kconfig b/drivers/platform/arm64/Kconfig
>>> index 8fdca0f8e909..8c103b3150d1 100644
>>> --- a/drivers/platform/arm64/Kconfig
>>> +++ b/drivers/platform/arm64/Kconfig
>>> @@ -32,4 +32,18 @@ config EC_ACER_ASPIRE1
>>> laptop where this information is not properly exposed via the
>>> standard ACPI devices.
>>> +config EC_LENOVO_YOGA_C630
>>> + tristate "Lenovo Yoga C630 Embedded Controller driver"
>>> + depends on I2C
>>> + help
>>> + Driver for the Embedded Controller in the Qualcomm Snapdragon-based
>>> + Lenovo Yoga C630, which provides battery and power adapter
>>> + information.
>>> +
>>> + This driver provides battery and AC status support for the mentioned
>>> + laptop where this information is not properly exposed via the
>>> + standard ACPI devices.
>>> +
>>> + Say M or Y here to include this support.
>>> +
>>> endif # ARM64_PLATFORM_DEVICES
>>> diff --git a/drivers/platform/arm64/Makefile b/drivers/platform/arm64/Makefile
>>> index 4fcc9855579b..b2ae9114fdd8 100644
>>> --- a/drivers/platform/arm64/Makefile
>>> +++ b/drivers/platform/arm64/Makefile
>>> @@ -6,3 +6,4 @@
>>> #
>>> obj-$(CONFIG_EC_ACER_ASPIRE1) += acer-aspire1-ec.o
>>> +obj-$(CONFIG_EC_LENOVO_YOGA_C630) += lenovo-yoga-c630.o
>>> diff --git a/drivers/platform/arm64/lenovo-yoga-c630.c b/drivers/platform/arm64/lenovo-yoga-c630.c
>>> new file mode 100644
>>> index 000000000000..3d1d5acde807
>>> --- /dev/null
>>> +++ b/drivers/platform/arm64/lenovo-yoga-c630.c
>>> @@ -0,0 +1,279 @@
>>> +// SPDX-License-Identifier: GPL-2.0-only
>>> +/*
>>> + * Copyright (c) 2022-2024, Linaro Ltd
>>> + * Authors:
>>> + * Bjorn Andersson
>>> + * Dmitry Baryshkov
>>> + */
>>> +#include <linux/auxiliary_bus.h>
>>> +#include <linux/i2c.h>
>>> +#include <linux/module.h>
>>> +#include <linux/notifier.h>
>>> +#include <linux/platform_data/lenovo-yoga-c630.h>
>>> +
>>> +#define LENOVO_EC_RESPONSE_REG 0x01
>>> +#define LENOVO_EC_REQUEST_REG 0x02
>>> +
>>> +#define LENOVO_EC_UCSI_WRITE 0x20
>>> +#define LENOVO_EC_UCSI_READ 0x21
>>> +
>>> +#define LENOVO_EC_READ_REG 0xb0
>>> +#define LENOVO_EC_REQUEST_NEXT_EVENT 0x84
>>> +
>>> +struct yoga_c630_ec {
>>> + struct i2c_client *client;
>>> + struct mutex lock;
>>> + struct blocking_notifier_head notifier_list;
>>> +};
>>> +
>>> +static int yoga_c630_ec_request(struct yoga_c630_ec *ec, u8 *req, size_t req_len,
>>> + u8 *resp, size_t resp_len)
>>> +{
>>> + int ret;
>>> +
>>> + WARN_ON(!mutex_is_locked(&ec->lock));
>>> +
>>> + ret = i2c_smbus_write_i2c_block_data(ec->client, LENOVO_EC_REQUEST_REG,
>>> + req_len, req);
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + return i2c_smbus_read_i2c_block_data(ec->client, LENOVO_EC_RESPONSE_REG,
>>> + resp_len, resp);
>>> +}
>>> +
>>> +int yoga_c630_ec_read8(struct yoga_c630_ec *ec, u8 addr)
>>> +{
>>> + u8 req[2] = { LENOVO_EC_READ_REG, };
>>> + int ret;
>>> + u8 val;
>>> +
>>> + mutex_lock(&ec->lock);
>>> + req[1] = addr;
>>> + ret = yoga_c630_ec_request(ec, req, sizeof(req), &val, 1);
>>> + mutex_unlock(&ec->lock);
>>> +
>>> + return ret < 0 ? ret : val;
>>> +}
>>> +EXPORT_SYMBOL_GPL(yoga_c630_ec_read8);
>>> +
>>> +int yoga_c630_ec_read16(struct yoga_c630_ec *ec, u8 addr)
>>> +{
>>> + u8 req[2] = { LENOVO_EC_READ_REG, };
>>> + int ret;
>>> + u8 msb;
>>> + u8 lsb;
>>> +
>>> + mutex_lock(&ec->lock);
>>> +
>>> + req[1] = addr;
>>> + ret = yoga_c630_ec_request(ec, req, sizeof(req), &lsb, 1);
>>> + if (ret < 0)
>>> + goto out;
>>> +
>>> + req[1] = addr + 1;
>>> + ret = yoga_c630_ec_request(ec, req, sizeof(req), &msb, 1);
>>> +
>>> +out:
>>> + mutex_unlock(&ec->lock);
>>> +
>>> + return ret < 0 ? ret : msb << 8 | lsb;
>>> +}
>>> +EXPORT_SYMBOL_GPL(yoga_c630_ec_read16);
>>> +
>>> +u16 yoga_c630_ec_ucsi_get_version(struct yoga_c630_ec *ec)
>>> +{
>>> + u8 req[3] = { 0xb3, 0xf2, 0x20};
>>
>> You have a define above for the read_reg and write_reg commands, could you
>> not define 0xb3 as LENOVO_EC_GET_VERSION ?
>>
>> All of the other commands here seem to have a named define.
>
> Because unlike other registers it is not clear what other use cases does
> 0xb3 support
>
>>
>>> + int ret;
>>> + u8 msb;
>>> + u8 lsb;
>>> +
>>> + mutex_lock(&ec->lock);
>>> + ret = yoga_c630_ec_request(ec, req, sizeof(req), &lsb, 1);
>>> + if (ret < 0)
>>> + goto out;
>>> +
>>> + req[2]++;
>>
>> why not set reg[2] = 0x21;
>
> ack
>
>>
>> also is req[2] some kind of address ?
>
> Unfortunately no idea. This is totally based on the AML code in DSDT. I
> have no documentation on the EC or its programming interface.
>
>>
>>> + ret = yoga_c630_ec_request(ec, req, sizeof(req), &msb, 1);
>>> +
>>> +out:
>>> + mutex_unlock(&ec->lock);
>>> +
>>> + return ret < 0 ? ret : msb << 8 | lsb;
>>> +}
>>> +EXPORT_SYMBOL_GPL(yoga_c630_ec_ucsi_get_version);
>>> +
>>> +int yoga_c630_ec_ucsi_write(struct yoga_c630_ec *ec,
>>> + const u8 req[YOGA_C630_UCSI_WRITE_SIZE])
>>> +{
>>> + int ret;
>>> +
>>> + mutex_lock(&ec->lock);
>>> + ret = i2c_smbus_write_i2c_block_data(ec->client, LENOVO_EC_UCSI_WRITE,
>>> + YOGA_C630_UCSI_WRITE_SIZE, req);
>>> + mutex_unlock(&ec->lock);
>>> +
>>> + return ret < 0 ? ret : 0;
>>> +}
>>> +EXPORT_SYMBOL_GPL(yoga_c630_ec_ucsi_write);
>>> +
>>> +int yoga_c630_ec_ucsi_read(struct yoga_c630_ec *ec,
>>> + u8 resp[YOGA_C630_UCSI_READ_SIZE])
>>> +{
>>> + int ret;
>>> +
>>> + mutex_lock(&ec->lock);
>>> + ret = i2c_smbus_read_i2c_block_data(ec->client, LENOVO_EC_UCSI_READ,
>>> + YOGA_C630_UCSI_READ_SIZE, resp);
>>> + mutex_unlock(&ec->lock);
>>> +
>>> + return ret < 0 ? ret : 0;
>>> +}
>>> +EXPORT_SYMBOL_GPL(yoga_c630_ec_ucsi_read);
>>> +
>>> +static irqreturn_t yoga_c630_ec_intr(int irq, void *data)
>>> +{
>>> + u8 req[] = { LENOVO_EC_REQUEST_NEXT_EVENT };
>>> + struct yoga_c630_ec *ec = data;
>>> + u8 event;
>>> + int ret;
>>> +
>>> + mutex_lock(&ec->lock);
>>> + ret = yoga_c630_ec_request(ec, req, sizeof(req), &event, 1);
>>> + mutex_unlock(&ec->lock);
>>> + if (ret < 0)
>>> + return IRQ_HANDLED;
>>> +
>>> + pr_info("NOTIFY %x\n", event);
>>
>> why not dev_info() ?
>
> Argh, debugging code. I should drop it.
>
Assuming you do all of that in v5
Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@...aro.org>
Powered by blists - more mailing lists