Re: [PATCH v5 2/6] platform: arm64: add Lenovo Yoga C630 WOS EC driver

From: Ilpo Järvinen
Date: Fri Jun 07 2024 - 07:46:51 EST


On Fri, 7 Jun 2024, Dmitry Baryshkov wrote:

> Lenovo Yoga C630 WOS is a laptop using Snapdragon 850 SoC. Like many
> laptops it uses an embedded controller (EC) to perform various platform
> 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.
>
> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@xxxxxxxxxx>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> ---
> drivers/platform/arm64/Kconfig | 14 ++
> drivers/platform/arm64/Makefile | 1 +
> drivers/platform/arm64/lenovo-yoga-c630.c | 283 +++++++++++++++++++++++++
> include/linux/platform_data/lenovo-yoga-c630.h | 43 ++++
> 4 files changed, 341 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..ffad8c443a13
> --- /dev/null
> +++ b/drivers/platform/arm64/lenovo-yoga-c630.c

> +int yoga_c630_ec_read8(struct yoga_c630_ec *ec, u8 addr)
> +{
> + u8 req[2] = { LENOVO_EC_READ_REG, };
> + int ret;
> + u8 val;
> +
> + scoped_guard(mutex, &ec->lock) {
> + req[1] = addr;
> + ret = yoga_c630_ec_request(ec, req, sizeof(req), &val, 1);
> + if (ret < 0)
> + return ret;
> + }
> +
> + return val;

For simple cases like this which don't do logic after the unlock, guard()
would be enough (I don't mind scoped_guard() myself though).

> +}
> +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;

addr + 1 could overflow below so it would be good the return -EINVAL if
0xff addr is given as a parameter.

> + scoped_guard(mutex, &ec->lock) {
> + req[1] = addr;
> + ret = yoga_c630_ec_request(ec, req, sizeof(req), &lsb, 1);
> + if (ret < 0)
> + return ret;
> +
> + req[1] = addr + 1;
> + ret = yoga_c630_ec_request(ec, req, sizeof(req), &msb, 1);
> + if (ret < 0)
> + return ret;
> + }
> +
> + return 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};
> + int ret;
> + u8 msb;
> + u8 lsb;
> +
> + scoped_guard(mutex, &ec->lock) {
> + ret = yoga_c630_ec_request(ec, req, sizeof(req), &lsb, 1);
> + if (ret < 0)
> + return ret;
> +
> + req[2] = 0x21;

Could you name 0x20 with a define and use it above and with + 1 here?

> + ret = yoga_c630_ec_request(ec, req, sizeof(req), &msb, 1);
> + if (ret < 0)
> + return ret;
> + }
> +
> + return msb << 8 | lsb;
> +}
> +EXPORT_SYMBOL_GPL(yoga_c630_ec_ucsi_get_version);

> diff --git a/include/linux/platform_data/lenovo-yoga-c630.h b/include/linux/platform_data/lenovo-yoga-c630.h
> new file mode 100644
> index 000000000000..5571dd65ce08
> --- /dev/null
> +++ b/include/linux/platform_data/lenovo-yoga-c630.h
> @@ -0,0 +1,43 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2022-2024, Linaro Ltd
> + * Authors:
> + * Bjorn Andersson
> + * Dmitry Baryshkov
> + */
> +
> +#ifndef _LENOVO_YOGA_C630_DATA_H
> +#define _LENOVO_YOGA_C630_DATA_H
> +
> +struct yoga_c630_ec;
> +struct notifier_block;
> +
> +#define YOGA_C630_MOD_NAME "lenovo_yoga_c630"
> +
> +#define YOGA_C630_DEV_UCSI "ucsi"
> +#define YOGA_C630_DEV_PSY "psy"
> +
> +int yoga_c630_ec_read8(struct yoga_c630_ec *ec, u8 addr);
> +int yoga_c630_ec_read16(struct yoga_c630_ec *ec, u8 addr);
> +
> +int yoga_c630_ec_register_notify(struct yoga_c630_ec *ec, struct notifier_block *nb);
> +void yoga_c630_ec_unregister_notify(struct yoga_c630_ec *ec, struct notifier_block *nb);
> +
> +#define YOGA_C630_UCSI_WRITE_SIZE 8
> +#define YOGA_C630_UCSI_CCI_SIZE 4
> +#define YOGA_C630_UCSI_DATA_SIZE 16
> +#define YOGA_C630_UCSI_READ_SIZE (YOGA_C630_UCSI_CCI_SIZE + YOGA_C630_UCSI_DATA_SIZE)

Add newline here.

> +u16 yoga_c630_ec_ucsi_get_version(struct yoga_c630_ec *ec);
> +int yoga_c630_ec_ucsi_write(struct yoga_c630_ec *ec,
> + const u8 req[YOGA_C630_UCSI_WRITE_SIZE]);
> +int yoga_c630_ec_ucsi_read(struct yoga_c630_ec *ec,
> + u8 resp[YOGA_C630_UCSI_READ_SIZE]);
> +
> +#define LENOVO_EC_EVENT_USB 0x20
> +#define LENOVO_EC_EVENT_UCSI 0x21
> +#define LENOVO_EC_EVENT_HPD 0x22
> +#define LENOVO_EC_EVENT_BAT_STATUS 0x24
> +#define LENOVO_EC_EVENT_BAT_INFO 0x25
> +#define LENOVO_EC_EVENT_BAT_ADPT_STATUS 0x37
> +
> +#endif
>
>

--
i.