[PATCH v4 0/6] power: supply: Lenovo Yoga C630 EC

[PATCH v4 0/6] power: supply: Lenovo Yoga C630 EC

[Dmitry Baryshkov]

This adds binding, driver and the DT support for the Lenovo Yoga C630
Embedded Controller, to provide battery information.

Support for this EC was implemented by Bjorn, who later could not work
on this driver. I've picked this patchset up and updated it following
the pending review comments.

DisplayPort support is still not a part of this patchset. It uses EC
messages to provide AltMode information rather than implementing
corresponding UCSI commands. However to have a cleaner uAPI story, the
AltMode should be handled via the same Type-C port.

Merge strategy: the driver bits depend on the platform/arm64 patch,
which adds interface for the subdrivers. I'd either ask to get that
patch merged to the immutable branch, which then can be picked up by
power/supply and USB trees or, to make life simpler, ack merging all
driver bits e.g. through USB subsystem (I'm biased here since I plan to
send more cleanups for the UCSI subsystem, which would otherwise result
in cross-subsystem conflicts).

---
Changes in v4:
- Moved bindings to platform/ to follow example of other Acer Aspire1 EC
  (Nikita Travkin)
- Fixed dt validation for EC interrupt pin (Rob Herring)
- Dropped separate 'scale' property (Oliver Neukum)
- Link to v3: https://lore.kernel.org/r/20240527-yoga-ec-driver-v3-0-327a9851dad5@linaro.org

Changes in v3:
- Split the driver into core and power supply drivers,
- Added UCSI driver part, handling USB connections,
- Fixed Bjorn's address in DT bindings (Brian Masney)
- Changed power-role for both ports to be "dual" per UCSI
- Link to v2: https://lore.kernel.org/linux-arm-msm/20230205152809.2233436-1-dmitry.baryshkov@linaro.org/

Changes in v2:
- Dropped DP support for now, as the bindings are in process of being
  discussed separately,
- Merged dt patch into the same patchseries,
- Removed the fixed serial number battery property,
- Fixed indentation of dt bindings example,
- Added property: reg and unevaluatedProperties to the connector
  bindings.
- Link to v1: https://lore.kernel.org/linux-arm-msm/20220810035424.2796777-1-bjorn.andersson@linaro.org/

---
Bjorn Andersson (2):
      dt-bindings: platform: Add Lenovo Yoga C630 EC
      arm64: dts: qcom: c630: Add Embedded Controller node

Dmitry Baryshkov (4):
      platform: arm64: add Lenovo Yoga C630 WOS EC driver
      usb: typec: ucsi: add Lenovo Yoga C630 glue driver
      power: supply: lenovo_yoga_c630_battery: add Lenovo C630 driver
      arm64: dts: qcom: sdm845: describe connections of USB/DP port

 .../bindings/platform/lenovo,yoga-c630-ec.yaml     |  83 ++++
 arch/arm64/boot/dts/qcom/sdm845.dtsi               |  53 ++-
 .../boot/dts/qcom/sdm850-lenovo-yoga-c630.dts      |  75 ++++
 drivers/platform/arm64/Kconfig                     |  14 +
 drivers/platform/arm64/Makefile                    |   1 +
 drivers/platform/arm64/lenovo-yoga-c630.c          | 279 ++++++++++++
 drivers/power/supply/Kconfig                       |   9 +
 drivers/power/supply/Makefile                      |   1 +
 drivers/power/supply/lenovo_yoga_c630_battery.c    | 479 +++++++++++++++++++++
 drivers/usb/typec/ucsi/Kconfig                     |   9 +
 drivers/usb/typec/ucsi/Makefile                    |   1 +
 drivers/usb/typec/ucsi/ucsi_yoga_c630.c            | 189 ++++++++
 include/linux/platform_data/lenovo-yoga-c630.h     |  42 ++
 13 files changed, 1234 insertions(+), 1 deletion(-)
---
base-commit: 6dc544b66971c7f9909ff038b62149105272d26a
change-id: 20240527-yoga-ec-driver-76fd7f5ddae8

Best regards,
-- 
Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

[PATCH v4 1/6] dt-bindings: platform: Add Lenovo Yoga C630 EC

[Dmitry Baryshkov]

From: Bjorn Andersson <andersson@kernel.org>

Add binding for the Embedded Controller found in the Qualcomm
Snapdragon-based Lenovo Yoga C630.

Signed-off-by: Bjorn Andersson <andersson@kernel.org>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 .../bindings/platform/lenovo,yoga-c630-ec.yaml     | 83 ++++++++++++++++++++++
 1 file changed, 83 insertions(+)

diff --git a/Documentation/devicetree/bindings/platform/lenovo,yoga-c630-ec.yaml b/Documentation/devicetree/bindings/platform/lenovo,yoga-c630-ec.yaml
new file mode 100644
index 000000000000..3180ce1a22d4
--- /dev/null
+++ b/Documentation/devicetree/bindings/platform/lenovo,yoga-c630-ec.yaml
@@ -0,0 +1,83 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/platform/lenovo,yoga-c630-ec.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Lenovo Yoga C630 Embedded Controller.
+
+maintainers:
+  - Bjorn Andersson <andersson@kernel.org>
+
+description:
+  The Qualcomm Snapdragon-based Lenovo Yoga C630 has an Embedded Controller
+  (EC) which handles things such as battery and USB Type-C. This binding
+  describes the interface, on an I2C bus, to this EC.
+
+properties:
+  compatible:
+    const: lenovo,yoga-c630-ec
+
+  reg:
+    const: 0x70
+
+  '#address-cells':
+    const: 1
+
+  '#size-cells':
+    const: 0
+
+  interrupts:
+    maxItems: 1
+
+patternProperties:
+  '^connector@[01]$':
+    $ref: /schemas/connector/usb-connector.yaml#
+
+    properties:
+      reg:
+        maxItems: 1
+
+    unevaluatedProperties: false
+
+required:
+  - compatible
+  - reg
+  - interrupts
+
+additionalProperties: false
+
+examples:
+  - |+
+    #include <dt-bindings/interrupt-controller/irq.h>
+    i2c1 {
+        clock-frequency = <400000>;
+
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        embedded-controller@70 {
+            compatible = "lenovo,yoga-c630-ec";
+            reg = <0x70>;
+
+            interrupts-extended = <&tlmm 20 IRQ_TYPE_LEVEL_HIGH>;
+
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            connector@0 {
+                compatible = "usb-c-connector";
+                reg = <0>;
+                power-role = "source";
+                data-role = "host";
+            };
+
+            connector@1 {
+                compatible = "usb-c-connector";
+                reg = <1>;
+                power-role = "source";
+                data-role = "host";
+            };
+        };
+    };
+...

-- 
2.39.2

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

[Dmitry Baryshkov]

Lenovo Yoga C630 WOS is a laptop using Snapdragon 850 SoC. Like many
laptops it uses 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.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.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};
+	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]++;
+	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);
+
+	blocking_notifier_call_chain(&ec->notifier_list, event, ec);
+
+	return IRQ_HANDLED;
+}
+
+/**
+ * yoga_c630_ec_register_notify - Register a notifier callback for EC events.
+ * @ec: Yoga C630 EC
+ * @nb: Notifier block pointer to register
+ *
+ * Return: 0 on success or negative error code.
+ */
+int yoga_c630_ec_register_notify(struct yoga_c630_ec *ec, struct notifier_block *nb)
+{
+	return blocking_notifier_chain_register(&ec->notifier_list,
+						nb);
+}
+EXPORT_SYMBOL_GPL(yoga_c630_ec_register_notify);
+
+/**
+ * yoga_c630_ec_unregister_notify - Unregister notifier callback for EC events.
+ * @ec: Yoga C630 EC
+ * @nb: Notifier block pointer to unregister
+ *
+ * Unregister a notifier callback that was previously registered with
+ * yoga_c630_ec_register_notify().
+ */
+void yoga_c630_ec_unregister_notify(struct yoga_c630_ec *ec, struct notifier_block *nb)
+{
+	blocking_notifier_chain_unregister(&ec->notifier_list, nb);
+}
+EXPORT_SYMBOL_GPL(yoga_c630_ec_unregister_notify);
+
+static void yoga_c630_aux_release(struct device *dev)
+{
+	struct auxiliary_device *adev = to_auxiliary_dev(dev);
+
+	kfree(adev);
+}
+
+static void yoga_c630_aux_remove(void *data)
+{
+	struct auxiliary_device *adev = data;
+
+	auxiliary_device_delete(adev);
+	auxiliary_device_uninit(adev);
+}
+
+static int yoga_c630_aux_init(struct device *parent, const char *name,
+			      struct yoga_c630_ec *ec)
+{
+	struct auxiliary_device *adev;
+	int ret;
+
+	adev = kzalloc(sizeof(*adev), GFP_KERNEL);
+	if (!adev)
+		return -ENOMEM;
+
+	adev->name = name;
+	adev->id = 0;
+	adev->dev.parent = parent;
+	adev->dev.release = yoga_c630_aux_release;
+	adev->dev.platform_data = ec;
+
+	ret = auxiliary_device_init(adev);
+	if (ret) {
+		kfree(adev);
+		return ret;
+	}
+
+	ret = auxiliary_device_add(adev);
+	if (ret) {
+		auxiliary_device_uninit(adev);
+		return ret;
+	}
+
+	return devm_add_action_or_reset(parent, yoga_c630_aux_remove, adev);
+}
+
+static int yoga_c630_ec_probe(struct i2c_client *client)
+{
+	struct yoga_c630_ec *ec;
+	struct device *dev = &client->dev;
+	int ret;
+
+	ec = devm_kzalloc(dev, sizeof(*ec), GFP_KERNEL);
+	if (!ec)
+		return -ENOMEM;
+
+	mutex_init(&ec->lock);
+	ec->client = client;
+	BLOCKING_INIT_NOTIFIER_HEAD(&ec->notifier_list);
+
+	ret = devm_request_threaded_irq(dev, client->irq,
+					NULL, yoga_c630_ec_intr,
+					IRQF_ONESHOT, "yoga_c630_ec", ec);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "unable to request irq\n");
+
+	ret = yoga_c630_aux_init(dev, YOGA_C630_DEV_PSY, ec);
+	if (ret)
+		return ret;
+
+	return yoga_c630_aux_init(dev, YOGA_C630_DEV_UCSI, ec);
+}
+
+
+static const struct of_device_id yoga_c630_ec_of_match[] = {
+	{ .compatible = "lenovo,yoga-c630-ec" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, yoga_c630_ec_of_match);
+
+static const struct i2c_device_id yoga_c630_ec_i2c_id_table[] = {
+	{ "yoga-c630-ec", },
+	{}
+};
+MODULE_DEVICE_TABLE(i2c, yoga_c630_ec_i2c_id_table);
+
+static struct i2c_driver yoga_c630_ec_i2c_driver = {
+	.driver = {
+		.name = "yoga-c630-ec",
+		.of_match_table = yoga_c630_ec_of_match
+	},
+	.probe = yoga_c630_ec_probe,
+	.id_table = yoga_c630_ec_i2c_id_table,
+};
+module_i2c_driver(yoga_c630_ec_i2c_driver);
+
+MODULE_DESCRIPTION("Lenovo Yoga C630 Embedded Controller");
+MODULE_LICENSE("GPL");
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..2b893dbeb124
--- /dev/null
+++ b/include/linux/platform_data/lenovo-yoga-c630.h
@@ -0,0 +1,42 @@
+// 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;
+
+#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)
+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

-- 
2.39.2

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

[Bryan O'Donoghue]

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@linaro.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.

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

also is req[2] some kind of address ?

> +	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() ?

---
bod

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

[Dmitry Baryshkov]

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@linaro.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.

-- 
With best wishes
Dmitry

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

[Bryan O'Donoghue]

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@linaro.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@linaro.org>

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

[Ilpo Järvinen]

On Tue, 28 May 2024, 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
> 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@linaro.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;

Add include for struct mutex.

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

There some lockdep way to assert the same thing.

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

Please use the scoped_guard from linux/cleanup.h for the mutex so you can 
immediately return instead of adding the out label.

> +
> +	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};
> +	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]++;
> +	ret = yoga_c630_ec_request(ec, req, sizeof(req), &msb, 1);
> +
> +out:
> +	mutex_unlock(&ec->lock);

Ditto.

> +	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);
> +
> +	blocking_notifier_call_chain(&ec->notifier_list, event, ec);
> +
> +	return IRQ_HANDLED;

Add include for IRQ_HANDLED and irqreturn_t.

> +}
> +
> +/**
> + * yoga_c630_ec_register_notify - Register a notifier callback for EC events.
> + * @ec: Yoga C630 EC
> + * @nb: Notifier block pointer to register
> + *
> + * Return: 0 on success or negative error code.
> + */
> +int yoga_c630_ec_register_notify(struct yoga_c630_ec *ec, struct notifier_block *nb)
> +{
> +	return blocking_notifier_chain_register(&ec->notifier_list,
> +						nb);

Fits to one line.

> +}
> +EXPORT_SYMBOL_GPL(yoga_c630_ec_register_notify);
> +
> +/**
> + * yoga_c630_ec_unregister_notify - Unregister notifier callback for EC events.
> + * @ec: Yoga C630 EC
> + * @nb: Notifier block pointer to unregister
> + *
> + * Unregister a notifier callback that was previously registered with
> + * yoga_c630_ec_register_notify().
> + */
> +void yoga_c630_ec_unregister_notify(struct yoga_c630_ec *ec, struct notifier_block *nb)
> +{
> +	blocking_notifier_chain_unregister(&ec->notifier_list, nb);
> +}
> +EXPORT_SYMBOL_GPL(yoga_c630_ec_unregister_notify);
> +
> +static void yoga_c630_aux_release(struct device *dev)
> +{
> +	struct auxiliary_device *adev = to_auxiliary_dev(dev);
> +
> +	kfree(adev);
> +}
> +
> +static void yoga_c630_aux_remove(void *data)
> +{
> +	struct auxiliary_device *adev = data;
> +
> +	auxiliary_device_delete(adev);
> +	auxiliary_device_uninit(adev);
> +}
> +
> +static int yoga_c630_aux_init(struct device *parent, const char *name,
> +			      struct yoga_c630_ec *ec)
> +{
> +	struct auxiliary_device *adev;
> +	int ret;
> +
> +	adev = kzalloc(sizeof(*adev), GFP_KERNEL);

Add include for kzalloc.

> +	if (!adev)
> +		return -ENOMEM;

Add include for ENOMEM.

I might have missed some other includes your code is using which are not 
directly included currently, please add them as well.

> +	adev->name = name;
> +	adev->id = 0;
> +	adev->dev.parent = parent;
> +	adev->dev.release = yoga_c630_aux_release;
> +	adev->dev.platform_data = ec;
> +
> +	ret = auxiliary_device_init(adev);
> +	if (ret) {
> +		kfree(adev);
> +		return ret;
> +	}
> +
> +	ret = auxiliary_device_add(adev);
> +	if (ret) {
> +		auxiliary_device_uninit(adev);
> +		return ret;
> +	}
> +
> +	return devm_add_action_or_reset(parent, yoga_c630_aux_remove, adev);
> +}
> +
> +static int yoga_c630_ec_probe(struct i2c_client *client)
> +{
> +	struct yoga_c630_ec *ec;
> +	struct device *dev = &client->dev;

Reverse the order of these two.

> +	int ret;
> +
> +	ec = devm_kzalloc(dev, sizeof(*ec), GFP_KERNEL);
> +	if (!ec)
> +		return -ENOMEM;
> +
> +	mutex_init(&ec->lock);
> +	ec->client = client;
> +	BLOCKING_INIT_NOTIFIER_HEAD(&ec->notifier_list);
> +
> +	ret = devm_request_threaded_irq(dev, client->irq,
> +					NULL, yoga_c630_ec_intr,

Could you please rename the handler function such that it's immediately 
obvious you're using irq thread (I had to check this from here when 
reviewing your handler since you used mutex inside it).

> +					IRQF_ONESHOT, "yoga_c630_ec", ec);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "unable to request irq\n");
> +
> +	ret = yoga_c630_aux_init(dev, YOGA_C630_DEV_PSY, ec);
> +	if (ret)
> +		return ret;
> +
> +	return yoga_c630_aux_init(dev, YOGA_C630_DEV_UCSI, ec);
> +}
> +
> +
> +static const struct of_device_id yoga_c630_ec_of_match[] = {
> +	{ .compatible = "lenovo,yoga-c630-ec" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, yoga_c630_ec_of_match);
> +
> +static const struct i2c_device_id yoga_c630_ec_i2c_id_table[] = {
> +	{ "yoga-c630-ec", },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(i2c, yoga_c630_ec_i2c_id_table);
> +
> +static struct i2c_driver yoga_c630_ec_i2c_driver = {
> +	.driver = {
> +		.name = "yoga-c630-ec",
> +		.of_match_table = yoga_c630_ec_of_match
> +	},
> +	.probe = yoga_c630_ec_probe,
> +	.id_table = yoga_c630_ec_i2c_id_table,
> +};
> +module_i2c_driver(yoga_c630_ec_i2c_driver);
> +
> +MODULE_DESCRIPTION("Lenovo Yoga C630 Embedded Controller");
> +MODULE_LICENSE("GPL");
> 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..2b893dbeb124
> --- /dev/null
> +++ b/include/linux/platform_data/lenovo-yoga-c630.h
> @@ -0,0 +1,42 @@
> +// 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;
> +
> +#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);

You need a forward declaration for struct notifier_block like you already 
have for yoga_c630_ec.

> +#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)
> +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.

[PATCH v4 3/6] usb: typec: ucsi: add Lenovo Yoga C630 glue driver

[Dmitry Baryshkov]

The Lenovo Yoga C630 WOS laptop provides implements UCSI interface in
the onboard EC. Add glue driver to interface the platform's UCSI
implementation.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/usb/typec/ucsi/Kconfig          |   9 ++
 drivers/usb/typec/ucsi/Makefile         |   1 +
 drivers/usb/typec/ucsi/ucsi_yoga_c630.c | 189 ++++++++++++++++++++++++++++++++
 3 files changed, 199 insertions(+)

diff --git a/drivers/usb/typec/ucsi/Kconfig b/drivers/usb/typec/ucsi/Kconfig
index bdcb1764cfae..680e1b87b152 100644
--- a/drivers/usb/typec/ucsi/Kconfig
+++ b/drivers/usb/typec/ucsi/Kconfig
@@ -69,4 +69,13 @@ config UCSI_PMIC_GLINK
 	  To compile the driver as a module, choose M here: the module will be
 	  called ucsi_glink.
 
+config UCSI_LENOVO_YOGA_C630
+	tristate "UCSI Interface Driver for Lenovo Yoga C630"
+	depends on EC_LENOVO_YOGA_C630
+	help
+	  This driver enables UCSI support on the Lenovo Yoga C630 laptop.
+
+	  To compile the driver as a module, choose M here: the module will be
+	  called ucsi_yoga_c630.
+
 endif
diff --git a/drivers/usb/typec/ucsi/Makefile b/drivers/usb/typec/ucsi/Makefile
index b4679f94696b..aed41d23887b 100644
--- a/drivers/usb/typec/ucsi/Makefile
+++ b/drivers/usb/typec/ucsi/Makefile
@@ -21,3 +21,4 @@ obj-$(CONFIG_UCSI_ACPI)			+= ucsi_acpi.o
 obj-$(CONFIG_UCSI_CCG)			+= ucsi_ccg.o
 obj-$(CONFIG_UCSI_STM32G0)		+= ucsi_stm32g0.o
 obj-$(CONFIG_UCSI_PMIC_GLINK)		+= ucsi_glink.o
+obj-$(CONFIG_UCSI_LENOVO_YOGA_C630)	+= ucsi_yoga_c630.o
diff --git a/drivers/usb/typec/ucsi/ucsi_yoga_c630.c b/drivers/usb/typec/ucsi/ucsi_yoga_c630.c
new file mode 100644
index 000000000000..ca1ab5c81b87
--- /dev/null
+++ b/drivers/usb/typec/ucsi/ucsi_yoga_c630.c
@@ -0,0 +1,189 @@
+// 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/module.h>
+#include <linux/platform_data/lenovo-yoga-c630.h>
+
+#include "ucsi.h"
+
+struct yoga_c630_ucsi {
+	struct yoga_c630_ec *ec;
+	struct ucsi *ucsi;
+	struct notifier_block nb;
+	struct completion complete;
+	unsigned long flags;
+#define UCSI_C630_COMMAND_PENDING	0
+#define UCSI_C630_ACK_PENDING		1
+	u16 version;
+};
+
+static  int yoga_c630_ucsi_read(struct ucsi *ucsi, unsigned int offset,
+				void *val, size_t val_len)
+{
+	struct yoga_c630_ucsi *uec = ucsi_get_drvdata(ucsi);
+	u8 buf[YOGA_C630_UCSI_READ_SIZE];
+	int ret;
+
+	ret = yoga_c630_ec_ucsi_read(uec->ec, buf);
+	if (ret)
+		return ret;
+
+	if (offset == UCSI_VERSION) {
+		memcpy(val, &uec->version, min(val_len, sizeof(uec->version)));
+		return 0;
+	}
+
+	if (offset == UCSI_CCI)
+		memcpy(val, buf,
+		       min(val_len, YOGA_C630_UCSI_CCI_SIZE));
+	else if (offset == UCSI_MESSAGE_IN)
+		memcpy(val, buf + YOGA_C630_UCSI_CCI_SIZE,
+		       min(val_len, YOGA_C630_UCSI_DATA_SIZE));
+	else
+		return -EINVAL;
+
+	return 0;
+}
+
+static  int yoga_c630_ucsi_async_write(struct ucsi *ucsi, unsigned int offset,
+				       const void *val, size_t val_len)
+{
+	struct yoga_c630_ucsi *uec = ucsi_get_drvdata(ucsi);
+
+	if (offset != UCSI_CONTROL ||
+	    val_len != YOGA_C630_UCSI_WRITE_SIZE)
+		return -EINVAL;
+
+	return yoga_c630_ec_ucsi_write(uec->ec, val);
+}
+
+static  int yoga_c630_ucsi_sync_write(struct ucsi *ucsi, unsigned int offset,
+				      const void *val, size_t val_len)
+{
+	struct yoga_c630_ucsi *uec = ucsi_get_drvdata(ucsi);
+	bool ack = UCSI_COMMAND(*(u64 *)val) == UCSI_ACK_CC_CI;
+	int ret;
+
+	if (ack)
+		set_bit(UCSI_C630_ACK_PENDING, &uec->flags);
+	else
+		set_bit(UCSI_C630_COMMAND_PENDING, &uec->flags);
+
+	reinit_completion(&uec->complete);
+
+	ret = yoga_c630_ucsi_async_write(ucsi, offset, val, val_len);
+	if (ret)
+		goto out_clear_bit;
+
+	if (!wait_for_completion_timeout(&uec->complete, 5 * HZ))
+		ret = -ETIMEDOUT;
+
+out_clear_bit:
+	if (ack)
+		clear_bit(UCSI_C630_ACK_PENDING, &uec->flags);
+	else
+		clear_bit(UCSI_C630_COMMAND_PENDING, &uec->flags);
+
+	return ret;
+}
+
+const struct ucsi_operations yoga_c630_ucsi_ops = {
+	.read = yoga_c630_ucsi_read,
+	.sync_write = yoga_c630_ucsi_sync_write,
+	.async_write = yoga_c630_ucsi_async_write,
+};
+
+static int yoga_c630_ucsi_notify(struct notifier_block *nb,
+				 unsigned long action, void *data)
+{
+	struct yoga_c630_ucsi *uec = container_of(nb, struct yoga_c630_ucsi, nb);
+	u32 cci;
+	int ret;
+
+	if (action == LENOVO_EC_EVENT_USB || action == LENOVO_EC_EVENT_HPD) {
+		ucsi_connector_change(uec->ucsi, 1);
+		return NOTIFY_OK;
+	}
+
+	if (action != LENOVO_EC_EVENT_UCSI)
+		return NOTIFY_DONE;
+
+	ret = uec->ucsi->ops->read(uec->ucsi, UCSI_CCI, &cci, sizeof(cci));
+	if (ret)
+		return NOTIFY_DONE;
+
+	if (UCSI_CCI_CONNECTOR(cci))
+		ucsi_connector_change(uec->ucsi, UCSI_CCI_CONNECTOR(cci));
+
+	if (cci & UCSI_CCI_ACK_COMPLETE &&
+	    test_bit(UCSI_C630_ACK_PENDING, &uec->flags))
+		complete(&uec->complete);
+	if (cci & UCSI_CCI_COMMAND_COMPLETE &&
+	    test_bit(UCSI_C630_COMMAND_PENDING, &uec->flags))
+		complete(&uec->complete);
+
+	return NOTIFY_OK;
+}
+
+static int yoga_c630_ucsi_probe(struct auxiliary_device *adev,
+				const struct auxiliary_device_id *id)
+{
+	struct yoga_c630_ec *ec = adev->dev.platform_data;
+	struct yoga_c630_ucsi *uec;
+	int ret;
+
+	uec = devm_kzalloc(&adev->dev, sizeof(*uec), GFP_KERNEL);
+	if (!uec)
+		return -ENOMEM;
+
+	uec->ec = ec;
+	init_completion(&uec->complete);
+	uec->nb.notifier_call = yoga_c630_ucsi_notify;
+
+	uec->ucsi = ucsi_create(&adev->dev, &yoga_c630_ucsi_ops);
+	if (IS_ERR(uec->ucsi))
+		return PTR_ERR(uec->ucsi);
+
+	ucsi_set_drvdata(uec->ucsi, uec);
+
+	uec->version = yoga_c630_ec_ucsi_get_version(uec->ec);
+
+	auxiliary_set_drvdata(adev, uec);
+
+	ret = yoga_c630_ec_register_notify(ec, &uec->nb);
+	if (ret)
+		return ret;
+
+	return ucsi_register(uec->ucsi);
+}
+
+static void yoga_c630_ucsi_remove(struct auxiliary_device *adev)
+{
+	struct yoga_c630_ucsi *uec = auxiliary_get_drvdata(adev);
+
+	yoga_c630_ec_unregister_notify(uec->ec, &uec->nb);
+	ucsi_unregister(uec->ucsi);
+}
+
+static const struct auxiliary_device_id yoga_c630_ucsi_id_table[] = {
+	{ .name = YOGA_C630_MOD_NAME "." YOGA_C630_DEV_UCSI, },
+	{}
+};
+MODULE_DEVICE_TABLE(auxiliary, yoga_c630_ucsi_id_table);
+
+static struct auxiliary_driver yoga_c630_ucsi_driver = {
+	.name = YOGA_C630_DEV_UCSI,
+	.id_table = yoga_c630_ucsi_id_table,
+	.probe = yoga_c630_ucsi_probe,
+	.remove = yoga_c630_ucsi_remove,
+};
+
+module_auxiliary_driver(yoga_c630_ucsi_driver);
+
+MODULE_DESCRIPTION("Lenovo Yoga C630 UCSI");
+MODULE_LICENSE("GPL");

-- 
2.39.2

Re: [PATCH v4 3/6] usb: typec: ucsi: add Lenovo Yoga C630 glue driver

[Ilpo Järvinen]

On Tue, 28 May 2024, Dmitry Baryshkov wrote:

> The Lenovo Yoga C630 WOS laptop provides implements UCSI interface in
> the onboard EC. Add glue driver to interface the platform's UCSI
> implementation.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/usb/typec/ucsi/Kconfig          |   9 ++
>  drivers/usb/typec/ucsi/Makefile         |   1 +
>  drivers/usb/typec/ucsi/ucsi_yoga_c630.c | 189 ++++++++++++++++++++++++++++++++
>  3 files changed, 199 insertions(+)
> 
> diff --git a/drivers/usb/typec/ucsi/Kconfig b/drivers/usb/typec/ucsi/Kconfig
> index bdcb1764cfae..680e1b87b152 100644
> --- a/drivers/usb/typec/ucsi/Kconfig
> +++ b/drivers/usb/typec/ucsi/Kconfig
> @@ -69,4 +69,13 @@ config UCSI_PMIC_GLINK
>  	  To compile the driver as a module, choose M here: the module will be
>  	  called ucsi_glink.
>  
> +config UCSI_LENOVO_YOGA_C630
> +	tristate "UCSI Interface Driver for Lenovo Yoga C630"
> +	depends on EC_LENOVO_YOGA_C630
> +	help
> +	  This driver enables UCSI support on the Lenovo Yoga C630 laptop.
> +
> +	  To compile the driver as a module, choose M here: the module will be
> +	  called ucsi_yoga_c630.
> +
>  endif
> diff --git a/drivers/usb/typec/ucsi/Makefile b/drivers/usb/typec/ucsi/Makefile
> index b4679f94696b..aed41d23887b 100644
> --- a/drivers/usb/typec/ucsi/Makefile
> +++ b/drivers/usb/typec/ucsi/Makefile
> @@ -21,3 +21,4 @@ obj-$(CONFIG_UCSI_ACPI)			+= ucsi_acpi.o
>  obj-$(CONFIG_UCSI_CCG)			+= ucsi_ccg.o
>  obj-$(CONFIG_UCSI_STM32G0)		+= ucsi_stm32g0.o
>  obj-$(CONFIG_UCSI_PMIC_GLINK)		+= ucsi_glink.o
> +obj-$(CONFIG_UCSI_LENOVO_YOGA_C630)	+= ucsi_yoga_c630.o
> diff --git a/drivers/usb/typec/ucsi/ucsi_yoga_c630.c b/drivers/usb/typec/ucsi/ucsi_yoga_c630.c
> new file mode 100644
> index 000000000000..ca1ab5c81b87
> --- /dev/null
> +++ b/drivers/usb/typec/ucsi/ucsi_yoga_c630.c
> @@ -0,0 +1,189 @@
> +// 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/module.h>
> +#include <linux/platform_data/lenovo-yoga-c630.h>
> +
> +#include "ucsi.h"
> +
> +struct yoga_c630_ucsi {
> +	struct yoga_c630_ec *ec;
> +	struct ucsi *ucsi;
> +	struct notifier_block nb;
> +	struct completion complete;

Add includes for what you used here.

> +	unsigned long flags;
> +#define UCSI_C630_COMMAND_PENDING	0
> +#define UCSI_C630_ACK_PENDING		1
> +	u16 version;
> +};
> +
> +static  int yoga_c630_ucsi_read(struct ucsi *ucsi, unsigned int offset,

extra space

> +				void *val, size_t val_len)
> +{
> +	struct yoga_c630_ucsi *uec = ucsi_get_drvdata(ucsi);

Missing include for ucsi_get_drvdata

> +	u8 buf[YOGA_C630_UCSI_READ_SIZE];
> +	int ret;
> +
> +	ret = yoga_c630_ec_ucsi_read(uec->ec, buf);
> +	if (ret)
> +		return ret;
> +
> +	if (offset == UCSI_VERSION) {
> +		memcpy(val, &uec->version, min(val_len, sizeof(uec->version)));
> +		return 0;
> +	}
> +
> +	if (offset == UCSI_CCI)
> +		memcpy(val, buf,
> +		       min(val_len, YOGA_C630_UCSI_CCI_SIZE));

Fits to one line.

> +	else if (offset == UCSI_MESSAGE_IN)
> +		memcpy(val, buf + YOGA_C630_UCSI_CCI_SIZE,
> +		       min(val_len, YOGA_C630_UCSI_DATA_SIZE));
> +	else
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static  int yoga_c630_ucsi_async_write(struct ucsi *ucsi, unsigned int offset,

extra space, there seems to be more of them below but I won't mark them.

> +				       const void *val, size_t val_len)
> +{
> +	struct yoga_c630_ucsi *uec = ucsi_get_drvdata(ucsi);
> +
> +	if (offset != UCSI_CONTROL ||
> +	    val_len != YOGA_C630_UCSI_WRITE_SIZE)
> +		return -EINVAL;
> +
> +	return yoga_c630_ec_ucsi_write(uec->ec, val);
> +}
> +
> +static  int yoga_c630_ucsi_sync_write(struct ucsi *ucsi, unsigned int offset,
> +				      const void *val, size_t val_len)
> +{
> +	struct yoga_c630_ucsi *uec = ucsi_get_drvdata(ucsi);
> +	bool ack = UCSI_COMMAND(*(u64 *)val) == UCSI_ACK_CC_CI;
> +	int ret;
> +
> +	if (ack)
> +		set_bit(UCSI_C630_ACK_PENDING, &uec->flags);
> +	else
> +		set_bit(UCSI_C630_COMMAND_PENDING, &uec->flags);

Include for set_bit()

> +	reinit_completion(&uec->complete);
> +
> +	ret = yoga_c630_ucsi_async_write(ucsi, offset, val, val_len);
> +	if (ret)
> +		goto out_clear_bit;
> +
> +	if (!wait_for_completion_timeout(&uec->complete, 5 * HZ))
> +		ret = -ETIMEDOUT;
> +
> +out_clear_bit:
> +	if (ack)
> +		clear_bit(UCSI_C630_ACK_PENDING, &uec->flags);
> +	else
> +		clear_bit(UCSI_C630_COMMAND_PENDING, &uec->flags);
> +
> +	return ret;
> +}
> +
> +const struct ucsi_operations yoga_c630_ucsi_ops = {

Include for ucsi_operations.

> +	.read = yoga_c630_ucsi_read,
> +	.sync_write = yoga_c630_ucsi_sync_write,
> +	.async_write = yoga_c630_ucsi_async_write,
> +};
> +
> +static int yoga_c630_ucsi_notify(struct notifier_block *nb,
> +				 unsigned long action, void *data)
> +{
> +	struct yoga_c630_ucsi *uec = container_of(nb, struct yoga_c630_ucsi, nb);

Include for container_of

-- 
 i.

Re: [PATCH v4 3/6] usb: typec: ucsi: add Lenovo Yoga C630 glue driver

[Dmitry Baryshkov]

On Wed, 29 May 2024 at 17:20, Ilpo Järvinen
<ilpo.jarvinen@linux.intel.com> wrote:
>
> On Tue, 28 May 2024, Dmitry Baryshkov wrote:
>
> > The Lenovo Yoga C630 WOS laptop provides implements UCSI interface in
> > the onboard EC. Add glue driver to interface the platform's UCSI
> > implementation.
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >  drivers/usb/typec/ucsi/Kconfig          |   9 ++
> >  drivers/usb/typec/ucsi/Makefile         |   1 +
> >  drivers/usb/typec/ucsi/ucsi_yoga_c630.c | 189 ++++++++++++++++++++++++++++++++
> >  3 files changed, 199 insertions(+)
> >
> > diff --git a/drivers/usb/typec/ucsi/Kconfig b/drivers/usb/typec/ucsi/Kconfig
> > index bdcb1764cfae..680e1b87b152 100644
> > --- a/drivers/usb/typec/ucsi/Kconfig
> > +++ b/drivers/usb/typec/ucsi/Kconfig
> > @@ -69,4 +69,13 @@ config UCSI_PMIC_GLINK
> >         To compile the driver as a module, choose M here: the module will be
> >         called ucsi_glink.
> >
> > +config UCSI_LENOVO_YOGA_C630
> > +     tristate "UCSI Interface Driver for Lenovo Yoga C630"
> > +     depends on EC_LENOVO_YOGA_C630
> > +     help
> > +       This driver enables UCSI support on the Lenovo Yoga C630 laptop.
> > +
> > +       To compile the driver as a module, choose M here: the module will be
> > +       called ucsi_yoga_c630.
> > +
> >  endif
> > diff --git a/drivers/usb/typec/ucsi/Makefile b/drivers/usb/typec/ucsi/Makefile
> > index b4679f94696b..aed41d23887b 100644
> > --- a/drivers/usb/typec/ucsi/Makefile
> > +++ b/drivers/usb/typec/ucsi/Makefile
> > @@ -21,3 +21,4 @@ obj-$(CONFIG_UCSI_ACPI)                     += ucsi_acpi.o
> >  obj-$(CONFIG_UCSI_CCG)                       += ucsi_ccg.o
> >  obj-$(CONFIG_UCSI_STM32G0)           += ucsi_stm32g0.o
> >  obj-$(CONFIG_UCSI_PMIC_GLINK)                += ucsi_glink.o
> > +obj-$(CONFIG_UCSI_LENOVO_YOGA_C630)  += ucsi_yoga_c630.o
> > diff --git a/drivers/usb/typec/ucsi/ucsi_yoga_c630.c b/drivers/usb/typec/ucsi/ucsi_yoga_c630.c
> > new file mode 100644
> > index 000000000000..ca1ab5c81b87
> > --- /dev/null
> > +++ b/drivers/usb/typec/ucsi/ucsi_yoga_c630.c
> > @@ -0,0 +1,189 @@
> > +// 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/module.h>
> > +#include <linux/platform_data/lenovo-yoga-c630.h>
> > +
> > +#include "ucsi.h"
> > +
> > +struct yoga_c630_ucsi {
> > +     struct yoga_c630_ec *ec;
> > +     struct ucsi *ucsi;
> > +     struct notifier_block nb;
> > +     struct completion complete;
>
> Add includes for what you used here.
>
> > +     unsigned long flags;
> > +#define UCSI_C630_COMMAND_PENDING    0
> > +#define UCSI_C630_ACK_PENDING                1
> > +     u16 version;
> > +};
> > +
> > +static  int yoga_c630_ucsi_read(struct ucsi *ucsi, unsigned int offset,
>
> extra space
>
> > +                             void *val, size_t val_len)
> > +{
> > +     struct yoga_c630_ucsi *uec = ucsi_get_drvdata(ucsi);
>
> Missing include for ucsi_get_drvdata

I'll review my includes, but this comment and the comment for
ucsi_operations are clearly wrong. There is a corresponding include.

>
> > +     u8 buf[YOGA_C630_UCSI_READ_SIZE];
> > +     int ret;
> > +
> > +     ret = yoga_c630_ec_ucsi_read(uec->ec, buf);
> > +     if (ret)
> > +             return ret;
> > +
> > +     if (offset == UCSI_VERSION) {
> > +             memcpy(val, &uec->version, min(val_len, sizeof(uec->version)));
> > +             return 0;
> > +     }
> > +
> > +     if (offset == UCSI_CCI)
> > +             memcpy(val, buf,
> > +                    min(val_len, YOGA_C630_UCSI_CCI_SIZE));
>
> Fits to one line.
>
> > +     else if (offset == UCSI_MESSAGE_IN)
> > +             memcpy(val, buf + YOGA_C630_UCSI_CCI_SIZE,
> > +                    min(val_len, YOGA_C630_UCSI_DATA_SIZE));
> > +     else
> > +             return -EINVAL;
> > +
> > +     return 0;
> > +}
> > +
> > +static  int yoga_c630_ucsi_async_write(struct ucsi *ucsi, unsigned int offset,
>
> extra space, there seems to be more of them below but I won't mark them.
>
> > +                                    const void *val, size_t val_len)
> > +{
> > +     struct yoga_c630_ucsi *uec = ucsi_get_drvdata(ucsi);
> > +
> > +     if (offset != UCSI_CONTROL ||
> > +         val_len != YOGA_C630_UCSI_WRITE_SIZE)
> > +             return -EINVAL;
> > +
> > +     return yoga_c630_ec_ucsi_write(uec->ec, val);
> > +}
> > +
> > +static  int yoga_c630_ucsi_sync_write(struct ucsi *ucsi, unsigned int offset,
> > +                                   const void *val, size_t val_len)
> > +{
> > +     struct yoga_c630_ucsi *uec = ucsi_get_drvdata(ucsi);
> > +     bool ack = UCSI_COMMAND(*(u64 *)val) == UCSI_ACK_CC_CI;
> > +     int ret;
> > +
> > +     if (ack)
> > +             set_bit(UCSI_C630_ACK_PENDING, &uec->flags);
> > +     else
> > +             set_bit(UCSI_C630_COMMAND_PENDING, &uec->flags);
>
> Include for set_bit()
>
> > +     reinit_completion(&uec->complete);
> > +
> > +     ret = yoga_c630_ucsi_async_write(ucsi, offset, val, val_len);
> > +     if (ret)
> > +             goto out_clear_bit;
> > +
> > +     if (!wait_for_completion_timeout(&uec->complete, 5 * HZ))
> > +             ret = -ETIMEDOUT;
> > +
> > +out_clear_bit:
> > +     if (ack)
> > +             clear_bit(UCSI_C630_ACK_PENDING, &uec->flags);
> > +     else
> > +             clear_bit(UCSI_C630_COMMAND_PENDING, &uec->flags);
> > +
> > +     return ret;
> > +}
> > +
> > +const struct ucsi_operations yoga_c630_ucsi_ops = {
>
> Include for ucsi_operations.
>
> > +     .read = yoga_c630_ucsi_read,
> > +     .sync_write = yoga_c630_ucsi_sync_write,
> > +     .async_write = yoga_c630_ucsi_async_write,
> > +};
> > +
> > +static int yoga_c630_ucsi_notify(struct notifier_block *nb,
> > +                              unsigned long action, void *data)
> > +{
> > +     struct yoga_c630_ucsi *uec = container_of(nb, struct yoga_c630_ucsi, nb);
>
> Include for container_of
>
> --
>  i.
>


-- 
With best wishes
Dmitry

Re: [PATCH v4 3/6] usb: typec: ucsi: add Lenovo Yoga C630 glue driver

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 3652 bytes --]

On Wed, 29 May 2024, Dmitry Baryshkov wrote:

> On Wed, 29 May 2024 at 17:20, Ilpo Järvinen
> <ilpo.jarvinen@linux.intel.com> wrote:
> >
> > On Tue, 28 May 2024, Dmitry Baryshkov wrote:
> >
> > > The Lenovo Yoga C630 WOS laptop provides implements UCSI interface in
> > > the onboard EC. Add glue driver to interface the platform's UCSI
> > > implementation.
> > >
> > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > ---
> > >  drivers/usb/typec/ucsi/Kconfig          |   9 ++
> > >  drivers/usb/typec/ucsi/Makefile         |   1 +
> > >  drivers/usb/typec/ucsi/ucsi_yoga_c630.c | 189 ++++++++++++++++++++++++++++++++
> > >  3 files changed, 199 insertions(+)
> > >
> > > diff --git a/drivers/usb/typec/ucsi/Kconfig b/drivers/usb/typec/ucsi/Kconfig
> > > index bdcb1764cfae..680e1b87b152 100644
> > > --- a/drivers/usb/typec/ucsi/Kconfig
> > > +++ b/drivers/usb/typec/ucsi/Kconfig
> > > @@ -69,4 +69,13 @@ config UCSI_PMIC_GLINK
> > >         To compile the driver as a module, choose M here: the module will be
> > >         called ucsi_glink.
> > >
> > > +config UCSI_LENOVO_YOGA_C630
> > > +     tristate "UCSI Interface Driver for Lenovo Yoga C630"
> > > +     depends on EC_LENOVO_YOGA_C630
> > > +     help
> > > +       This driver enables UCSI support on the Lenovo Yoga C630 laptop.
> > > +
> > > +       To compile the driver as a module, choose M here: the module will be
> > > +       called ucsi_yoga_c630.
> > > +
> > >  endif
> > > diff --git a/drivers/usb/typec/ucsi/Makefile b/drivers/usb/typec/ucsi/Makefile
> > > index b4679f94696b..aed41d23887b 100644
> > > --- a/drivers/usb/typec/ucsi/Makefile
> > > +++ b/drivers/usb/typec/ucsi/Makefile
> > > @@ -21,3 +21,4 @@ obj-$(CONFIG_UCSI_ACPI)                     += ucsi_acpi.o
> > >  obj-$(CONFIG_UCSI_CCG)                       += ucsi_ccg.o
> > >  obj-$(CONFIG_UCSI_STM32G0)           += ucsi_stm32g0.o
> > >  obj-$(CONFIG_UCSI_PMIC_GLINK)                += ucsi_glink.o
> > > +obj-$(CONFIG_UCSI_LENOVO_YOGA_C630)  += ucsi_yoga_c630.o
> > > diff --git a/drivers/usb/typec/ucsi/ucsi_yoga_c630.c b/drivers/usb/typec/ucsi/ucsi_yoga_c630.c
> > > new file mode 100644
> > > index 000000000000..ca1ab5c81b87
> > > --- /dev/null
> > > +++ b/drivers/usb/typec/ucsi/ucsi_yoga_c630.c
> > > @@ -0,0 +1,189 @@
> > > +// 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/module.h>
> > > +#include <linux/platform_data/lenovo-yoga-c630.h>
> > > +
> > > +#include "ucsi.h"
> > > +
> > > +struct yoga_c630_ucsi {
> > > +     struct yoga_c630_ec *ec;
> > > +     struct ucsi *ucsi;
> > > +     struct notifier_block nb;
> > > +     struct completion complete;
> >
> > Add includes for what you used here.
> >
> > > +     unsigned long flags;
> > > +#define UCSI_C630_COMMAND_PENDING    0
> > > +#define UCSI_C630_ACK_PENDING                1
> > > +     u16 version;
> > > +};
> > > +
> > > +static  int yoga_c630_ucsi_read(struct ucsi *ucsi, unsigned int offset,
> >
> > extra space
> >
> > > +                             void *val, size_t val_len)
> > > +{
> > > +     struct yoga_c630_ucsi *uec = ucsi_get_drvdata(ucsi);
> >
> > Missing include for ucsi_get_drvdata
> 
> I'll review my includes, but this comment and the comment for
> ucsi_operations are clearly wrong. There is a corresponding include.

Ah, sorry about that. I completely missed there was that local include.

-- 
 i.

Re: [PATCH v4 3/6] usb: typec: ucsi: add Lenovo Yoga C630 glue driver

[Bryan O'Donoghue]

On 28/05/2024 21:44, Dmitry Baryshkov wrote:
> The Lenovo Yoga C630 WOS laptop provides implements UCSI interface in
> the onboard EC. Add glue driver to interface the platform's UCSI
> implementation.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/usb/typec/ucsi/Kconfig          |   9 ++
>   drivers/usb/typec/ucsi/Makefile         |   1 +
>   drivers/usb/typec/ucsi/ucsi_yoga_c630.c | 189 ++++++++++++++++++++++++++++++++
>   3 files changed, 199 insertions(+)
> 
> diff --git a/drivers/usb/typec/ucsi/Kconfig b/drivers/usb/typec/ucsi/Kconfig
> index bdcb1764cfae..680e1b87b152 100644
> --- a/drivers/usb/typec/ucsi/Kconfig
> +++ b/drivers/usb/typec/ucsi/Kconfig
> @@ -69,4 +69,13 @@ config UCSI_PMIC_GLINK
>   	  To compile the driver as a module, choose M here: the module will be
>   	  called ucsi_glink.
>   
> +config UCSI_LENOVO_YOGA_C630
> +	tristate "UCSI Interface Driver for Lenovo Yoga C630"
> +	depends on EC_LENOVO_YOGA_C630
> +	help
> +	  This driver enables UCSI support on the Lenovo Yoga C630 laptop.
> +
> +	  To compile the driver as a module, choose M here: the module will be
> +	  called ucsi_yoga_c630.
> +
>   endif
> diff --git a/drivers/usb/typec/ucsi/Makefile b/drivers/usb/typec/ucsi/Makefile
> index b4679f94696b..aed41d23887b 100644
> --- a/drivers/usb/typec/ucsi/Makefile
> +++ b/drivers/usb/typec/ucsi/Makefile
> @@ -21,3 +21,4 @@ obj-$(CONFIG_UCSI_ACPI)			+= ucsi_acpi.o
>   obj-$(CONFIG_UCSI_CCG)			+= ucsi_ccg.o
>   obj-$(CONFIG_UCSI_STM32G0)		+= ucsi_stm32g0.o
>   obj-$(CONFIG_UCSI_PMIC_GLINK)		+= ucsi_glink.o
> +obj-$(CONFIG_UCSI_LENOVO_YOGA_C630)	+= ucsi_yoga_c630.o
> diff --git a/drivers/usb/typec/ucsi/ucsi_yoga_c630.c b/drivers/usb/typec/ucsi/ucsi_yoga_c630.c
> new file mode 100644
> index 000000000000..ca1ab5c81b87
> --- /dev/null
> +++ b/drivers/usb/typec/ucsi/ucsi_yoga_c630.c
> @@ -0,0 +1,189 @@
> +// 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/module.h>
> +#include <linux/platform_data/lenovo-yoga-c630.h>
> +
> +#include "ucsi.h"
> +
> +struct yoga_c630_ucsi {
> +	struct yoga_c630_ec *ec;
> +	struct ucsi *ucsi;
> +	struct notifier_block nb;
> +	struct completion complete;
> +	unsigned long flags;
> +#define UCSI_C630_COMMAND_PENDING	0
> +#define UCSI_C630_ACK_PENDING		1
> +	u16 version;
> +};
> +
> +static  int yoga_c630_ucsi_read(struct ucsi *ucsi, unsigned int offset,
> +				void *val, size_t val_len)
> +{
> +	struct yoga_c630_ucsi *uec = ucsi_get_drvdata(ucsi);
> +	u8 buf[YOGA_C630_UCSI_READ_SIZE];
> +	int ret;
> +
> +	ret = yoga_c630_ec_ucsi_read(uec->ec, buf);
> +	if (ret)
> +		return ret;
> +
> +	if (offset == UCSI_VERSION) {
> +		memcpy(val, &uec->version, min(val_len, sizeof(uec->version)));
> +		return 0;
> +	}
> +
> +	if (offset == UCSI_CCI)
> +		memcpy(val, buf,
> +		       min(val_len, YOGA_C630_UCSI_CCI_SIZE));
> +	else if (offset == UCSI_MESSAGE_IN)
> +		memcpy(val, buf + YOGA_C630_UCSI_CCI_SIZE,
> +		       min(val_len, YOGA_C630_UCSI_DATA_SIZE));

For some reason I believe multi-lines like this, including function 
calls that are split over lines should be encapsulated with {}

else if(x) {
     memcpy(x,y,
            z);
}

If checkpatch doesn't complain about it feel free not to do that though.

> +	else
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static  int yoga_c630_ucsi_async_write(struct ucsi *ucsi, unsigned int offset,
> +				       const void *val, size_t val_len)
> +{
> +	struct yoga_c630_ucsi *uec = ucsi_get_drvdata(ucsi);
> +
> +	if (offset != UCSI_CONTROL ||
> +	    val_len != YOGA_C630_UCSI_WRITE_SIZE)
> +		return -EINVAL;
> +
> +	return yoga_c630_ec_ucsi_write(uec->ec, val);
> +}
> +
> +static  int yoga_c630_ucsi_sync_write(struct ucsi *ucsi, unsigned int offset,
> +				      const void *val, size_t val_len)
> +{
> +	struct yoga_c630_ucsi *uec = ucsi_get_drvdata(ucsi);
> +	bool ack = UCSI_COMMAND(*(u64 *)val) == UCSI_ACK_CC_CI;
> +	int ret;
> +
> +	if (ack)
> +		set_bit(UCSI_C630_ACK_PENDING, &uec->flags);
> +	else
> +		set_bit(UCSI_C630_COMMAND_PENDING, &uec->flags);
> +
> +	reinit_completion(&uec->complete);
> +
> +	ret = yoga_c630_ucsi_async_write(ucsi, offset, val, val_len);
> +	if (ret)
> +		goto out_clear_bit;
> +
> +	if (!wait_for_completion_timeout(&uec->complete, 5 * HZ))
> +		ret = -ETIMEDOUT;
> +
> +out_clear_bit:
> +	if (ack)
> +		clear_bit(UCSI_C630_ACK_PENDING, &uec->flags);
> +	else
> +		clear_bit(UCSI_C630_COMMAND_PENDING, &uec->flags);
> +
> +	return ret;
> +}
> +
> +const struct ucsi_operations yoga_c630_ucsi_ops = {
> +	.read = yoga_c630_ucsi_read,
> +	.sync_write = yoga_c630_ucsi_sync_write,
> +	.async_write = yoga_c630_ucsi_async_write,
> +};
> +
> +static int yoga_c630_ucsi_notify(struct notifier_block *nb,
> +				 unsigned long action, void *data)
> +{
> +	struct yoga_c630_ucsi *uec = container_of(nb, struct yoga_c630_ucsi, nb);
> +	u32 cci;
> +	int ret;
> +
> +	if (action == LENOVO_EC_EVENT_USB || action == LENOVO_EC_EVENT_HPD) {
> +		ucsi_connector_change(uec->ucsi, 1);
> +		return NOTIFY_OK;
> +	}
> +
> +	if (action != LENOVO_EC_EVENT_UCSI)
> +		return NOTIFY_DONE;

Is this disjunction on action a good candidate for a switch(){}


> +
> +	ret = uec->ucsi->ops->read(uec->ucsi, UCSI_CCI, &cci, sizeof(cci));
> +	if (ret)
> +		return NOTIFY_DONE;
> +
> +	if (UCSI_CCI_CONNECTOR(cci))
> +		ucsi_connector_change(uec->ucsi, UCSI_CCI_CONNECTOR(cci));
> +
> +	if (cci & UCSI_CCI_ACK_COMPLETE &&
> +	    test_bit(UCSI_C630_ACK_PENDING, &uec->flags))
> +		complete(&uec->complete);
> +	if (cci & UCSI_CCI_COMMAND_COMPLETE &&
> +	    test_bit(UCSI_C630_COMMAND_PENDING, &uec->flags))
> +		complete(&uec->complete);

IMO these multi-line clauses should end up with a {} around the complete 
even though its not required.

Emphasis on the O.

Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

Re: [PATCH v4 3/6] usb: typec: ucsi: add Lenovo Yoga C630 glue driver

[Dmitry Baryshkov]

On Wed, May 29, 2024 at 04:41:40PM +0100, Bryan O'Donoghue wrote:
> On 28/05/2024 21:44, Dmitry Baryshkov wrote:
> > The Lenovo Yoga C630 WOS laptop provides implements UCSI interface in
> > the onboard EC. Add glue driver to interface the platform's UCSI
> > implementation.
> > 
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >   drivers/usb/typec/ucsi/Kconfig          |   9 ++
> >   drivers/usb/typec/ucsi/Makefile         |   1 +
> >   drivers/usb/typec/ucsi/ucsi_yoga_c630.c | 189 ++++++++++++++++++++++++++++++++
> >   3 files changed, 199 insertions(+)
> > 
> > diff --git a/drivers/usb/typec/ucsi/Kconfig b/drivers/usb/typec/ucsi/Kconfig
> > index bdcb1764cfae..680e1b87b152 100644
> > --- a/drivers/usb/typec/ucsi/Kconfig
> > +++ b/drivers/usb/typec/ucsi/Kconfig
> > @@ -69,4 +69,13 @@ config UCSI_PMIC_GLINK
> >   	  To compile the driver as a module, choose M here: the module will be
> >   	  called ucsi_glink.
> > +config UCSI_LENOVO_YOGA_C630
> > +	tristate "UCSI Interface Driver for Lenovo Yoga C630"
> > +	depends on EC_LENOVO_YOGA_C630
> > +	help
> > +	  This driver enables UCSI support on the Lenovo Yoga C630 laptop.
> > +
> > +	  To compile the driver as a module, choose M here: the module will be
> > +	  called ucsi_yoga_c630.
> > +
> >   endif
> > diff --git a/drivers/usb/typec/ucsi/Makefile b/drivers/usb/typec/ucsi/Makefile
> > index b4679f94696b..aed41d23887b 100644
> > --- a/drivers/usb/typec/ucsi/Makefile
> > +++ b/drivers/usb/typec/ucsi/Makefile
> > @@ -21,3 +21,4 @@ obj-$(CONFIG_UCSI_ACPI)			+= ucsi_acpi.o
> >   obj-$(CONFIG_UCSI_CCG)			+= ucsi_ccg.o
> >   obj-$(CONFIG_UCSI_STM32G0)		+= ucsi_stm32g0.o
> >   obj-$(CONFIG_UCSI_PMIC_GLINK)		+= ucsi_glink.o
> > +obj-$(CONFIG_UCSI_LENOVO_YOGA_C630)	+= ucsi_yoga_c630.o
> > diff --git a/drivers/usb/typec/ucsi/ucsi_yoga_c630.c b/drivers/usb/typec/ucsi/ucsi_yoga_c630.c
> > new file mode 100644
> > index 000000000000..ca1ab5c81b87
> > --- /dev/null
> > +++ b/drivers/usb/typec/ucsi/ucsi_yoga_c630.c
> > @@ -0,0 +1,189 @@
> > +// 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/module.h>
> > +#include <linux/platform_data/lenovo-yoga-c630.h>
> > +
> > +#include "ucsi.h"
> > +
> > +struct yoga_c630_ucsi {
> > +	struct yoga_c630_ec *ec;
> > +	struct ucsi *ucsi;
> > +	struct notifier_block nb;
> > +	struct completion complete;
> > +	unsigned long flags;
> > +#define UCSI_C630_COMMAND_PENDING	0
> > +#define UCSI_C630_ACK_PENDING		1
> > +	u16 version;
> > +};
> > +
> > +static  int yoga_c630_ucsi_read(struct ucsi *ucsi, unsigned int offset,
> > +				void *val, size_t val_len)
> > +{
> > +	struct yoga_c630_ucsi *uec = ucsi_get_drvdata(ucsi);
> > +	u8 buf[YOGA_C630_UCSI_READ_SIZE];
> > +	int ret;
> > +
> > +	ret = yoga_c630_ec_ucsi_read(uec->ec, buf);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (offset == UCSI_VERSION) {
> > +		memcpy(val, &uec->version, min(val_len, sizeof(uec->version)));
> > +		return 0;
> > +	}
> > +
> > +	if (offset == UCSI_CCI)
> > +		memcpy(val, buf,
> > +		       min(val_len, YOGA_C630_UCSI_CCI_SIZE));
> > +	else if (offset == UCSI_MESSAGE_IN)
> > +		memcpy(val, buf + YOGA_C630_UCSI_CCI_SIZE,
> > +		       min(val_len, YOGA_C630_UCSI_DATA_SIZE));
> 
> For some reason I believe multi-lines like this, including function calls
> that are split over lines should be encapsulated with {}
> 
> else if(x) {
>     memcpy(x,y,
>            z);
> }
> 
> If checkpatch doesn't complain about it feel free not to do that though.

No, checkpatch --strict doesn't complain

> 
> > +	else
> > +		return -EINVAL;
> > +
> > +	return 0;
> > +}
> > +
> > +static  int yoga_c630_ucsi_async_write(struct ucsi *ucsi, unsigned int offset,
> > +				       const void *val, size_t val_len)
> > +{
> > +	struct yoga_c630_ucsi *uec = ucsi_get_drvdata(ucsi);
> > +
> > +	if (offset != UCSI_CONTROL ||
> > +	    val_len != YOGA_C630_UCSI_WRITE_SIZE)
> > +		return -EINVAL;
> > +
> > +	return yoga_c630_ec_ucsi_write(uec->ec, val);
> > +}
> > +
> > +static  int yoga_c630_ucsi_sync_write(struct ucsi *ucsi, unsigned int offset,
> > +				      const void *val, size_t val_len)
> > +{
> > +	struct yoga_c630_ucsi *uec = ucsi_get_drvdata(ucsi);
> > +	bool ack = UCSI_COMMAND(*(u64 *)val) == UCSI_ACK_CC_CI;
> > +	int ret;
> > +
> > +	if (ack)
> > +		set_bit(UCSI_C630_ACK_PENDING, &uec->flags);
> > +	else
> > +		set_bit(UCSI_C630_COMMAND_PENDING, &uec->flags);
> > +
> > +	reinit_completion(&uec->complete);
> > +
> > +	ret = yoga_c630_ucsi_async_write(ucsi, offset, val, val_len);
> > +	if (ret)
> > +		goto out_clear_bit;
> > +
> > +	if (!wait_for_completion_timeout(&uec->complete, 5 * HZ))
> > +		ret = -ETIMEDOUT;
> > +
> > +out_clear_bit:
> > +	if (ack)
> > +		clear_bit(UCSI_C630_ACK_PENDING, &uec->flags);
> > +	else
> > +		clear_bit(UCSI_C630_COMMAND_PENDING, &uec->flags);
> > +
> > +	return ret;
> > +}
> > +
> > +const struct ucsi_operations yoga_c630_ucsi_ops = {
> > +	.read = yoga_c630_ucsi_read,
> > +	.sync_write = yoga_c630_ucsi_sync_write,
> > +	.async_write = yoga_c630_ucsi_async_write,
> > +};
> > +
> > +static int yoga_c630_ucsi_notify(struct notifier_block *nb,
> > +				 unsigned long action, void *data)
> > +{
> > +	struct yoga_c630_ucsi *uec = container_of(nb, struct yoga_c630_ucsi, nb);
> > +	u32 cci;
> > +	int ret;
> > +
> > +	if (action == LENOVO_EC_EVENT_USB || action == LENOVO_EC_EVENT_HPD) {
> > +		ucsi_connector_change(uec->ucsi, 1);
> > +		return NOTIFY_OK;
> > +	}
> > +
> > +	if (action != LENOVO_EC_EVENT_UCSI)
> > +		return NOTIFY_DONE;
> 
> Is this disjunction on action a good candidate for a switch(){}

Ack, refactored the function by extracting the UCSI notification code
and then using the switch-case.

> > +
> > +	ret = uec->ucsi->ops->read(uec->ucsi, UCSI_CCI, &cci, sizeof(cci));
> > +	if (ret)
> > +		return NOTIFY_DONE;
> > +
> > +	if (UCSI_CCI_CONNECTOR(cci))
> > +		ucsi_connector_change(uec->ucsi, UCSI_CCI_CONNECTOR(cci));
> > +
> > +	if (cci & UCSI_CCI_ACK_COMPLETE &&
> > +	    test_bit(UCSI_C630_ACK_PENDING, &uec->flags))
> > +		complete(&uec->complete);
> > +	if (cci & UCSI_CCI_COMMAND_COMPLETE &&
> > +	    test_bit(UCSI_C630_COMMAND_PENDING, &uec->flags))
> > +		complete(&uec->complete);
> 
> IMO these multi-line clauses should end up with a {} around the complete
> even though its not required.
> 
> Emphasis on the O.

I added an empty line inbetween, then it's easier to comprehent event
without curly brackets.

> 
> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

-- 
With best wishes
Dmitry

[PATCH v4 4/6] power: supply: lenovo_yoga_c630_battery: add Lenovo C630 driver

[Dmitry Baryshkov]

On the Lenovo Yoga C630 WOS laptop the EC provides access to the adapter
and battery status. Add the driver to read power supply status on the
laptop.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/power/supply/Kconfig                    |   9 +
 drivers/power/supply/Makefile                   |   1 +
 drivers/power/supply/lenovo_yoga_c630_battery.c | 479 ++++++++++++++++++++++++
 3 files changed, 489 insertions(+)

diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
index 3e31375491d5..55ab8e90747d 100644
--- a/drivers/power/supply/Kconfig
+++ b/drivers/power/supply/Kconfig
@@ -167,6 +167,15 @@ config BATTERY_LEGO_EV3
 	help
 	  Say Y here to enable support for the LEGO MINDSTORMS EV3 battery.
 
+config BATTERY_LENOVO_YOGA_C630
+	tristate "Lenovo Yoga C630 battery"
+	depends on OF && EC_LENOVO_YOGA_C630
+	help
+	  This driver enables battery support on the Lenovo Yoga C630 laptop.
+
+	  To compile the driver as a module, choose M here: the module will be
+	  called lenovo_yoga_c630_battery.
+
 config BATTERY_PMU
 	tristate "Apple PMU battery"
 	depends on PPC32 && ADB_PMU
diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
index 58b567278034..8ebbdcf92dac 100644
--- a/drivers/power/supply/Makefile
+++ b/drivers/power/supply/Makefile
@@ -32,6 +32,7 @@ obj-$(CONFIG_BATTERY_DS2782)	+= ds2782_battery.o
 obj-$(CONFIG_BATTERY_GAUGE_LTC2941)	+= ltc2941-battery-gauge.o
 obj-$(CONFIG_BATTERY_GOLDFISH)	+= goldfish_battery.o
 obj-$(CONFIG_BATTERY_LEGO_EV3)	+= lego_ev3_battery.o
+obj-$(CONFIG_BATTERY_LENOVO_YOGA_C630) += lenovo_yoga_c630_battery.o
 obj-$(CONFIG_BATTERY_PMU)	+= pmu_battery.o
 obj-$(CONFIG_BATTERY_QCOM_BATTMGR)	+= qcom_battmgr.o
 obj-$(CONFIG_BATTERY_OLPC)	+= olpc_battery.o
diff --git a/drivers/power/supply/lenovo_yoga_c630_battery.c b/drivers/power/supply/lenovo_yoga_c630_battery.c
new file mode 100644
index 000000000000..76152ad38d46
--- /dev/null
+++ b/drivers/power/supply/lenovo_yoga_c630_battery.c
@@ -0,0 +1,479 @@
+// 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/delay.h>
+#include <linux/module.h>
+#include <linux/platform_data/lenovo-yoga-c630.h>
+#include <linux/power_supply.h>
+
+struct yoga_c630_psy {
+	struct yoga_c630_ec *ec;
+	struct device *dev;
+	struct device_node *of_node;
+	struct notifier_block nb;
+	struct mutex lock;
+
+	struct power_supply *adp_psy;
+	struct power_supply *bat_psy;
+
+	unsigned long last_status_update;
+
+	bool adapter_online;
+
+	bool unit_mA;
+
+	bool bat_present;
+	unsigned int bat_status;
+	unsigned int design_capacity;
+	unsigned int design_voltage;
+	unsigned int full_charge_capacity;
+
+	unsigned int capacity_now;
+	unsigned int voltage_now;
+
+	int current_now;
+	int rate_now;
+};
+
+#define LENOVO_EC_CACHE_TIME		(10 * HZ)
+
+#define LENOVO_EC_ADPT_STATUS		0xa3
+#define LENOVO_EC_ADPT_PRESENT		BIT(7)
+#define LENOVO_EC_BAT_ATTRIBUTES	0xc0
+#define LENOVO_EC_BAT_ATTR_UNIT_IS_MA	BIT(1)
+#define LENOVO_EC_BAT_STATUS		0xc1
+#define LENOVO_EC_BAT_REMAIN_CAPACITY	0xc2
+#define LENOVO_EC_BAT_VOLTAGE		0xc6
+#define LENOVO_EC_BAT_DESIGN_VOLTAGE	0xc8
+#define LENOVO_EC_BAT_DESIGN_CAPACITY	0xca
+#define LENOVO_EC_BAT_FULL_CAPACITY	0xcc
+#define LENOVO_EC_BAT_CURRENT		0xd2
+#define LENOVO_EC_BAT_FULL_FACTORY	0xd6
+#define LENOVO_EC_BAT_PRESENT		0xda
+#define LENOVO_EC_BAT_FULL_REGISTER	0xdb
+#define LENOVO_EC_BAT_FULL_IS_FACTORY	BIT(0)
+
+/* the mutex should already be locked */
+static int yoga_c630_psy_update_bat_info(struct yoga_c630_psy *ecbat)
+{
+	struct yoga_c630_ec *ec = ecbat->ec;
+	int val;
+
+	val = yoga_c630_ec_read8(ec, LENOVO_EC_BAT_PRESENT);
+	if (val < 0)
+		return val;
+	ecbat->bat_present = !!(val & BIT(0));
+	if (!ecbat->bat_present)
+		return val;
+
+	val = yoga_c630_ec_read8(ec, LENOVO_EC_BAT_ATTRIBUTES);
+	if (val < 0)
+		return val;
+	ecbat->unit_mA = val & LENOVO_EC_BAT_ATTR_UNIT_IS_MA;
+
+	val = yoga_c630_ec_read16(ec, LENOVO_EC_BAT_DESIGN_CAPACITY);
+	if (val < 0)
+		return val;
+	ecbat->design_capacity = val * 1000;
+
+	msleep(50);
+
+	val = yoga_c630_ec_read16(ec, LENOVO_EC_BAT_DESIGN_VOLTAGE);
+	if (val < 0)
+		return val;
+	ecbat->design_voltage = val;
+
+	msleep(50);
+
+	val = yoga_c630_ec_read8(ec, LENOVO_EC_BAT_FULL_REGISTER);
+	if (val < 0)
+		return val;
+	if (val & LENOVO_EC_BAT_FULL_IS_FACTORY)
+		val = yoga_c630_ec_read16(ec, LENOVO_EC_BAT_FULL_FACTORY);
+	else
+		val = yoga_c630_ec_read16(ec, LENOVO_EC_BAT_FULL_CAPACITY);
+	if (val < 0)
+		return val;
+
+	ecbat->full_charge_capacity = val * 1000;
+
+	if (!ecbat->unit_mA) {
+		ecbat->design_capacity *= 10;
+		ecbat->full_charge_capacity *= 10;
+	}
+
+	return 0;
+}
+
+/* the mutex should already be locked */
+static int yoga_c630_psy_maybe_update_bat_status(struct yoga_c630_psy *ecbat)
+{
+	struct yoga_c630_ec *ec = ecbat->ec;
+	int current_mA;
+	int val;
+
+	if (time_before(jiffies, ecbat->last_status_update + LENOVO_EC_CACHE_TIME))
+		return 0;
+
+	val = yoga_c630_ec_read8(ec, LENOVO_EC_BAT_STATUS);
+	if (val < 0)
+		return val;
+	ecbat->bat_status = val;
+
+	msleep(50);
+
+	val = yoga_c630_ec_read16(ec, LENOVO_EC_BAT_REMAIN_CAPACITY);
+	if (val < 0)
+		return val;
+	ecbat->capacity_now = val * 1000;
+
+	msleep(50);
+
+	val = yoga_c630_ec_read16(ec, LENOVO_EC_BAT_VOLTAGE);
+	if (val < 0)
+		return val;
+	ecbat->voltage_now = val * 1000;
+
+	msleep(50);
+
+	val = yoga_c630_ec_read16(ec, LENOVO_EC_BAT_CURRENT);
+	if (val < 0)
+		return val;
+	current_mA = sign_extend32(val, 15);
+	ecbat->current_now = current_mA * 1000;
+	ecbat->rate_now = current_mA * (ecbat->voltage_now / 1000);
+
+	msleep(50);
+
+	if (!ecbat->unit_mA)
+		ecbat->capacity_now *= 10;
+
+	ecbat->last_status_update = jiffies;
+
+	return 0;
+}
+
+static int yoga_c630_psy_update_adapter_status(struct yoga_c630_psy *ecbat)
+{
+	struct yoga_c630_ec *ec = ecbat->ec;
+	int val;
+
+	mutex_lock(&ecbat->lock);
+
+	val = yoga_c630_ec_read8(ec, LENOVO_EC_ADPT_STATUS);
+	if (val > 0)
+		ecbat->adapter_online = FIELD_GET(LENOVO_EC_ADPT_PRESENT, val);
+
+	mutex_unlock(&ecbat->lock);
+
+	return val;
+}
+
+static bool yoga_c630_psy_is_charged(struct yoga_c630_psy *ecbat)
+{
+	if (ecbat->bat_status != 0)
+		return false;
+
+	if (ecbat->full_charge_capacity <= ecbat->capacity_now)
+		return true;
+
+	if (ecbat->design_capacity <= ecbat->capacity_now)
+		return true;
+
+	return false;
+}
+
+static int yoga_c630_psy_bat_get_property(struct power_supply *psy,
+					 enum power_supply_property psp,
+					 union power_supply_propval *val)
+{
+	struct yoga_c630_psy *ecbat = power_supply_get_drvdata(psy);
+	int rc = 0;
+
+	if (!ecbat->bat_present &&
+	    psp != POWER_SUPPLY_PROP_PRESENT)
+		return -ENODEV;
+
+	mutex_lock(&ecbat->lock);
+	rc = yoga_c630_psy_maybe_update_bat_status(ecbat);
+	mutex_unlock(&ecbat->lock);
+
+	if (rc)
+		return rc;
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_STATUS:
+		if (ecbat->bat_status & BIT(0))
+			val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
+		else if (ecbat->bat_status & BIT(1))
+			val->intval = POWER_SUPPLY_STATUS_CHARGING;
+		else if (yoga_c630_psy_is_charged(ecbat))
+			val->intval = POWER_SUPPLY_STATUS_FULL;
+		else
+			val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING;
+		break;
+	case POWER_SUPPLY_PROP_PRESENT:
+		val->intval = ecbat->bat_present;
+		break;
+	case POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN:
+		val->intval = ecbat->design_voltage;
+		break;
+	case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
+	case POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN:
+		val->intval = ecbat->design_capacity;
+		break;
+	case POWER_SUPPLY_PROP_CHARGE_FULL:
+	case POWER_SUPPLY_PROP_ENERGY_FULL:
+		val->intval = ecbat->full_charge_capacity;
+		break;
+	case POWER_SUPPLY_PROP_CHARGE_NOW:
+	case POWER_SUPPLY_PROP_ENERGY_NOW:
+		val->intval = ecbat->capacity_now;
+		break;
+	case POWER_SUPPLY_PROP_CURRENT_NOW:
+		val->intval = ecbat->current_now;
+		break;
+	case POWER_SUPPLY_PROP_POWER_NOW:
+		val->intval = ecbat->rate_now;
+		break;
+	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
+		val->intval = ecbat->voltage_now;
+		break;
+	case POWER_SUPPLY_PROP_TECHNOLOGY:
+		val->intval = POWER_SUPPLY_TECHNOLOGY_LION;
+		break;
+	case POWER_SUPPLY_PROP_MODEL_NAME:
+		val->strval = "PABAS0241231";
+		break;
+	case POWER_SUPPLY_PROP_MANUFACTURER:
+		val->strval = "Compal";
+		break;
+	default:
+		rc = -EINVAL;
+		break;
+	}
+
+	return rc;
+}
+
+static enum power_supply_property yoga_c630_psy_bat_mA_properties[] = {
+	POWER_SUPPLY_PROP_STATUS,
+	POWER_SUPPLY_PROP_PRESENT,
+	POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN,
+	POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN,
+	POWER_SUPPLY_PROP_CHARGE_FULL,
+	POWER_SUPPLY_PROP_CHARGE_NOW,
+	POWER_SUPPLY_PROP_CURRENT_NOW,
+	POWER_SUPPLY_PROP_POWER_NOW,
+	POWER_SUPPLY_PROP_VOLTAGE_NOW,
+	POWER_SUPPLY_PROP_TECHNOLOGY,
+	POWER_SUPPLY_PROP_MODEL_NAME,
+	POWER_SUPPLY_PROP_MANUFACTURER,
+};
+
+static enum power_supply_property yoga_c630_psy_bat_mWh_properties[] = {
+	POWER_SUPPLY_PROP_STATUS,
+	POWER_SUPPLY_PROP_PRESENT,
+	POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN,
+	POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN,
+	POWER_SUPPLY_PROP_ENERGY_FULL,
+	POWER_SUPPLY_PROP_ENERGY_NOW,
+	POWER_SUPPLY_PROP_CURRENT_NOW,
+	POWER_SUPPLY_PROP_POWER_NOW,
+	POWER_SUPPLY_PROP_VOLTAGE_NOW,
+	POWER_SUPPLY_PROP_TECHNOLOGY,
+	POWER_SUPPLY_PROP_MODEL_NAME,
+	POWER_SUPPLY_PROP_MANUFACTURER,
+};
+
+static const struct power_supply_desc yoga_c630_psy_bat_psy_desc_mA = {
+	.name = "yoga-c630-battery",
+	.type = POWER_SUPPLY_TYPE_BATTERY,
+	.properties = yoga_c630_psy_bat_mA_properties,
+	.num_properties = ARRAY_SIZE(yoga_c630_psy_bat_mA_properties),
+	.get_property = yoga_c630_psy_bat_get_property,
+};
+
+static const struct power_supply_desc yoga_c630_psy_bat_psy_desc_mWh = {
+	.name = "yoga-c630-battery",
+	.type = POWER_SUPPLY_TYPE_BATTERY,
+	.properties = yoga_c630_psy_bat_mWh_properties,
+	.num_properties = ARRAY_SIZE(yoga_c630_psy_bat_mWh_properties),
+	.get_property = yoga_c630_psy_bat_get_property,
+};
+
+static int yoga_c630_psy_adpt_get_property(struct power_supply *psy,
+					  enum power_supply_property psp,
+					  union power_supply_propval *val)
+{
+	struct yoga_c630_psy *ecbat = power_supply_get_drvdata(psy);
+	int rc = 0;
+
+	yoga_c630_psy_update_adapter_status(ecbat);
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_ONLINE:
+		val->intval = ecbat->adapter_online;
+		break;
+	default:
+		rc = -EINVAL;
+		break;
+	}
+
+	return rc;
+}
+
+static enum power_supply_property yoga_c630_psy_adpt_properties[] = {
+	POWER_SUPPLY_PROP_ONLINE,
+};
+
+static const struct power_supply_desc yoga_c630_psy_adpt_psy_desc = {
+	.name = "yoga-c630-adapter",
+	.type = POWER_SUPPLY_TYPE_USB_TYPE_C,
+	.properties = yoga_c630_psy_adpt_properties,
+	.num_properties = ARRAY_SIZE(yoga_c630_psy_adpt_properties),
+	.get_property = yoga_c630_psy_adpt_get_property,
+};
+
+static int yoga_c630_psy_register_bat_psy(struct yoga_c630_psy *ecbat)
+{
+	struct power_supply_config bat_cfg = {};
+
+	bat_cfg.drv_data = ecbat;
+	bat_cfg.of_node = ecbat->of_node;
+	if (ecbat->unit_mA)
+		ecbat->bat_psy = power_supply_register_no_ws(ecbat->dev, &yoga_c630_psy_bat_psy_desc_mA, &bat_cfg);
+	else
+		ecbat->bat_psy = power_supply_register_no_ws(ecbat->dev, &yoga_c630_psy_bat_psy_desc_mWh, &bat_cfg);
+	if (IS_ERR(ecbat->bat_psy)) {
+		dev_err(ecbat->dev, "failed to register battery supply\n");
+		return PTR_ERR(ecbat->bat_psy);
+	}
+
+	return 0;
+}
+
+static void yoga_c630_ec_refresh_bat_info(struct yoga_c630_psy *ecbat)
+{
+	bool current_unit;
+
+	mutex_lock(&ecbat->lock);
+	current_unit = ecbat->unit_mA;
+
+	yoga_c630_psy_update_bat_info(ecbat);
+
+	if (current_unit != ecbat->unit_mA) {
+		power_supply_unregister(ecbat->bat_psy);
+		yoga_c630_psy_register_bat_psy(ecbat);
+	}
+
+	mutex_unlock(&ecbat->lock);
+}
+
+static int yoga_c630_psy_notify(struct notifier_block *nb,
+				unsigned long action, void *data)
+{
+	struct yoga_c630_psy *ecbat = container_of(nb, struct yoga_c630_psy, nb);
+
+	switch (action) {
+	case LENOVO_EC_EVENT_BAT_INFO:
+		yoga_c630_ec_refresh_bat_info(ecbat);
+		break;
+	case LENOVO_EC_EVENT_BAT_ADPT_STATUS:
+		power_supply_changed(ecbat->adp_psy);
+		fallthrough;
+	case LENOVO_EC_EVENT_BAT_STATUS:
+		power_supply_changed(ecbat->bat_psy);
+		break;
+	}
+
+	return NOTIFY_OK;
+}
+
+static int yoga_c630_psy_probe(struct auxiliary_device *adev,
+				   const struct auxiliary_device_id *id)
+{
+	struct yoga_c630_ec *ec = adev->dev.platform_data;
+	struct power_supply_config adp_cfg = {};
+	struct device *dev = &adev->dev;
+	struct yoga_c630_psy *ecbat;
+	int ret;
+
+	ecbat = devm_kzalloc(&adev->dev, sizeof(*ecbat), GFP_KERNEL);
+	if (!ecbat)
+		return -ENOMEM;
+
+	ecbat->ec = ec;
+	ecbat->dev = dev;
+	mutex_init(&ecbat->lock);
+	ecbat->of_node = adev->dev.parent->of_node;
+	ecbat->nb.notifier_call = yoga_c630_psy_notify;
+
+	auxiliary_set_drvdata(adev, ecbat);
+
+	adp_cfg.drv_data = ecbat;
+	adp_cfg.of_node = ecbat->of_node;
+	adp_cfg.supplied_to = (char **)&yoga_c630_psy_bat_psy_desc_mA.name;
+	adp_cfg.num_supplicants = 1;
+	ecbat->adp_psy = devm_power_supply_register_no_ws(dev, &yoga_c630_psy_adpt_psy_desc, &adp_cfg);
+	if (IS_ERR(ecbat->adp_psy)) {
+		dev_err(dev, "failed to register AC adapter supply\n");
+		return PTR_ERR(ecbat->adp_psy);
+	}
+
+	mutex_lock(&ecbat->lock);
+
+	ret = yoga_c630_psy_update_bat_info(ecbat);
+	if (ret)
+		goto err_unlock;
+
+	ret = yoga_c630_psy_register_bat_psy(ecbat);
+	if (ret)
+		goto err_unlock;
+
+	mutex_unlock(&ecbat->lock);
+
+	ret = yoga_c630_ec_register_notify(ecbat->ec, &ecbat->nb);
+	if (ret)
+		goto err_unreg_bat;
+
+	return 0;
+
+err_unlock:
+	mutex_unlock(&ecbat->lock);
+
+err_unreg_bat:
+	power_supply_unregister(ecbat->bat_psy);
+	return ret;
+}
+
+static void yoga_c630_psy_remove(struct auxiliary_device *adev)
+{
+	struct yoga_c630_psy *ecbat = auxiliary_get_drvdata(adev);
+
+	yoga_c630_ec_unregister_notify(ecbat->ec, &ecbat->nb);
+	power_supply_unregister(ecbat->bat_psy);
+}
+
+static const struct auxiliary_device_id yoga_c630_psy_id_table[] = {
+	{ .name = YOGA_C630_MOD_NAME "." YOGA_C630_DEV_PSY, },
+	{}
+};
+MODULE_DEVICE_TABLE(auxiliary, yoga_c630_psy_id_table);
+
+static struct auxiliary_driver yoga_c630_psy_driver = {
+	.name = YOGA_C630_DEV_PSY,
+	.id_table = yoga_c630_psy_id_table,
+	.probe = yoga_c630_psy_probe,
+	.remove = yoga_c630_psy_remove,
+};
+
+module_auxiliary_driver(yoga_c630_psy_driver);
+
+MODULE_DESCRIPTION("Lenovo Yoga C630 psy");
+MODULE_LICENSE("GPL");

-- 
2.39.2

Re: [PATCH v4 4/6] power: supply: lenovo_yoga_c630_battery: add Lenovo C630 driver

[Ilpo Järvinen]

On Tue, 28 May 2024, Dmitry Baryshkov wrote:

> On the Lenovo Yoga C630 WOS laptop the EC provides access to the adapter
> and battery status. Add the driver to read power supply status on the
> laptop.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/power/supply/Kconfig                    |   9 +
>  drivers/power/supply/Makefile                   |   1 +
>  drivers/power/supply/lenovo_yoga_c630_battery.c | 479 ++++++++++++++++++++++++
>  3 files changed, 489 insertions(+)
> 
> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
> index 3e31375491d5..55ab8e90747d 100644
> --- a/drivers/power/supply/Kconfig
> +++ b/drivers/power/supply/Kconfig
> @@ -167,6 +167,15 @@ config BATTERY_LEGO_EV3
>  	help
>  	  Say Y here to enable support for the LEGO MINDSTORMS EV3 battery.
>  
> +config BATTERY_LENOVO_YOGA_C630
> +	tristate "Lenovo Yoga C630 battery"
> +	depends on OF && EC_LENOVO_YOGA_C630
> +	help
> +	  This driver enables battery support on the Lenovo Yoga C630 laptop.
> +
> +	  To compile the driver as a module, choose M here: the module will be
> +	  called lenovo_yoga_c630_battery.
> +
>  config BATTERY_PMU
>  	tristate "Apple PMU battery"
>  	depends on PPC32 && ADB_PMU
> diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
> index 58b567278034..8ebbdcf92dac 100644
> --- a/drivers/power/supply/Makefile
> +++ b/drivers/power/supply/Makefile
> @@ -32,6 +32,7 @@ obj-$(CONFIG_BATTERY_DS2782)	+= ds2782_battery.o
>  obj-$(CONFIG_BATTERY_GAUGE_LTC2941)	+= ltc2941-battery-gauge.o
>  obj-$(CONFIG_BATTERY_GOLDFISH)	+= goldfish_battery.o
>  obj-$(CONFIG_BATTERY_LEGO_EV3)	+= lego_ev3_battery.o
> +obj-$(CONFIG_BATTERY_LENOVO_YOGA_C630) += lenovo_yoga_c630_battery.o
>  obj-$(CONFIG_BATTERY_PMU)	+= pmu_battery.o
>  obj-$(CONFIG_BATTERY_QCOM_BATTMGR)	+= qcom_battmgr.o
>  obj-$(CONFIG_BATTERY_OLPC)	+= olpc_battery.o
> diff --git a/drivers/power/supply/lenovo_yoga_c630_battery.c b/drivers/power/supply/lenovo_yoga_c630_battery.c
> new file mode 100644
> index 000000000000..76152ad38d46
> --- /dev/null
> +++ b/drivers/power/supply/lenovo_yoga_c630_battery.c
> @@ -0,0 +1,479 @@
> +// 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/delay.h>
> +#include <linux/module.h>
> +#include <linux/platform_data/lenovo-yoga-c630.h>
> +#include <linux/power_supply.h>
> +
> +struct yoga_c630_psy {
> +	struct yoga_c630_ec *ec;
> +	struct device *dev;
> +	struct device_node *of_node;
> +	struct notifier_block nb;
> +	struct mutex lock;

Missing a few includes, please check them here as well.

> +	struct power_supply *adp_psy;
> +	struct power_supply *bat_psy;
> +
> +	unsigned long last_status_update;
> +
> +	bool adapter_online;
> +
> +	bool unit_mA;
> +
> +	bool bat_present;
> +	unsigned int bat_status;
> +	unsigned int design_capacity;
> +	unsigned int design_voltage;
> +	unsigned int full_charge_capacity;
> +
> +	unsigned int capacity_now;
> +	unsigned int voltage_now;
> +
> +	int current_now;
> +	int rate_now;
> +};
> +
> +#define LENOVO_EC_CACHE_TIME		(10 * HZ)
> +
> +#define LENOVO_EC_ADPT_STATUS		0xa3
> +#define LENOVO_EC_ADPT_PRESENT		BIT(7)

Add include for BIT()

> +#define LENOVO_EC_BAT_ATTRIBUTES	0xc0
> +#define LENOVO_EC_BAT_ATTR_UNIT_IS_MA	BIT(1)
> +#define LENOVO_EC_BAT_STATUS		0xc1
> +#define LENOVO_EC_BAT_REMAIN_CAPACITY	0xc2
> +#define LENOVO_EC_BAT_VOLTAGE		0xc6
> +#define LENOVO_EC_BAT_DESIGN_VOLTAGE	0xc8
> +#define LENOVO_EC_BAT_DESIGN_CAPACITY	0xca
> +#define LENOVO_EC_BAT_FULL_CAPACITY	0xcc
> +#define LENOVO_EC_BAT_CURRENT		0xd2
> +#define LENOVO_EC_BAT_FULL_FACTORY	0xd6
> +#define LENOVO_EC_BAT_PRESENT		0xda
> +#define LENOVO_EC_BAT_FULL_REGISTER	0xdb
> +#define LENOVO_EC_BAT_FULL_IS_FACTORY	BIT(0)
> +
> +/* the mutex should already be locked */
> +static int yoga_c630_psy_update_bat_info(struct yoga_c630_psy *ecbat)
> +{
> +	struct yoga_c630_ec *ec = ecbat->ec;
> +	int val;
> +
> +	val = yoga_c630_ec_read8(ec, LENOVO_EC_BAT_PRESENT);
> +	if (val < 0)
> +		return val;
> +	ecbat->bat_present = !!(val & BIT(0));

Name the bit with a define.

> +	if (!ecbat->bat_present)
> +		return val;
> +
> +	val = yoga_c630_ec_read8(ec, LENOVO_EC_BAT_ATTRIBUTES);
> +	if (val < 0)
> +		return val;
> +	ecbat->unit_mA = val & LENOVO_EC_BAT_ATTR_UNIT_IS_MA;
> +
> +	val = yoga_c630_ec_read16(ec, LENOVO_EC_BAT_DESIGN_CAPACITY);
> +	if (val < 0)
> +		return val;
> +	ecbat->design_capacity = val * 1000;

Check linux/units.h if some WATT related one matches to that literal 1000.

> +	msleep(50);
> +
> +	val = yoga_c630_ec_read16(ec, LENOVO_EC_BAT_DESIGN_VOLTAGE);
> +	if (val < 0)
> +		return val;
> +	ecbat->design_voltage = val;
> +
> +	msleep(50);
> +
> +	val = yoga_c630_ec_read8(ec, LENOVO_EC_BAT_FULL_REGISTER);
> +	if (val < 0)
> +		return val;
> +	if (val & LENOVO_EC_BAT_FULL_IS_FACTORY)
> +		val = yoga_c630_ec_read16(ec, LENOVO_EC_BAT_FULL_FACTORY);
> +	else
> +		val = yoga_c630_ec_read16(ec, LENOVO_EC_BAT_FULL_CAPACITY);

You could consider doing this instead of branching:

	val = yoga_c630_ec_read16(ec, val & LENOVO_EC_BAT_FULL_IS_FACTORY ?
				      LENOVO_EC_BAT_FULL_FACTORY :
				      LENOVO_EC_BAT_FULL_CAPACITY);

> +	if (val < 0)
> +		return val;
> +
> +	ecbat->full_charge_capacity = val * 1000;

Something from linux/units.h ?

> +	if (!ecbat->unit_mA) {
> +		ecbat->design_capacity *= 10;
> +		ecbat->full_charge_capacity *= 10;
> +	}
> +
> +	return 0;
> +}
> +
> +/* the mutex should already be locked */
> +static int yoga_c630_psy_maybe_update_bat_status(struct yoga_c630_psy *ecbat)
> +{
> +	struct yoga_c630_ec *ec = ecbat->ec;
> +	int current_mA;
> +	int val;
> +
> +	if (time_before(jiffies, ecbat->last_status_update + LENOVO_EC_CACHE_TIME))
> +		return 0;
> +
> +	val = yoga_c630_ec_read8(ec, LENOVO_EC_BAT_STATUS);
> +	if (val < 0)
> +		return val;
> +	ecbat->bat_status = val;
> +
> +	msleep(50);
> +
> +	val = yoga_c630_ec_read16(ec, LENOVO_EC_BAT_REMAIN_CAPACITY);
> +	if (val < 0)
> +		return val;
> +	ecbat->capacity_now = val * 1000;

units.h ?

> +	msleep(50);
> +
> +	val = yoga_c630_ec_read16(ec, LENOVO_EC_BAT_VOLTAGE);
> +	if (val < 0)
> +		return val;
> +	ecbat->voltage_now = val * 1000;

Ditto.

> +	msleep(50);
> +
> +	val = yoga_c630_ec_read16(ec, LENOVO_EC_BAT_CURRENT);
> +	if (val < 0)
> +		return val;
> +	current_mA = sign_extend32(val, 15);
> +	ecbat->current_now = current_mA * 1000;
> +	ecbat->rate_now = current_mA * (ecbat->voltage_now / 1000);

Ditto.

> +	msleep(50);
> +
> +	if (!ecbat->unit_mA)
> +		ecbat->capacity_now *= 10;
> +
> +	ecbat->last_status_update = jiffies;

Add jiffies.h

> +	return 0;
> +}
> +
> +static int yoga_c630_psy_update_adapter_status(struct yoga_c630_psy *ecbat)
> +{
> +	struct yoga_c630_ec *ec = ecbat->ec;
> +	int val;
> +
> +	mutex_lock(&ecbat->lock);

This kind of functions coul use guard to take the mutex so unlock will be 
handled for you by the cleanup automation.

> +	val = yoga_c630_ec_read8(ec, LENOVO_EC_ADPT_STATUS);
> +	if (val > 0)
> +		ecbat->adapter_online = FIELD_GET(LENOVO_EC_ADPT_PRESENT, val);
> +
> +	mutex_unlock(&ecbat->lock);
> +
> +	return val;
> +}
> +
> +static bool yoga_c630_psy_is_charged(struct yoga_c630_psy *ecbat)
> +{
> +	if (ecbat->bat_status != 0)
> +		return false;
> +
> +	if (ecbat->full_charge_capacity <= ecbat->capacity_now)
> +		return true;
> +
> +	if (ecbat->design_capacity <= ecbat->capacity_now)
> +		return true;
> +
> +	return false;
> +}
> +
> +static int yoga_c630_psy_bat_get_property(struct power_supply *psy,
> +					 enum power_supply_property psp,
> +					 union power_supply_propval *val)
> +{
> +	struct yoga_c630_psy *ecbat = power_supply_get_drvdata(psy);
> +	int rc = 0;
> +
> +	if (!ecbat->bat_present &&
> +	    psp != POWER_SUPPLY_PROP_PRESENT)

Fits to one line.

> +		return -ENODEV;
> +
> +	mutex_lock(&ecbat->lock);
> +	rc = yoga_c630_psy_maybe_update_bat_status(ecbat);
> +	mutex_unlock(&ecbat->lock);
> +
> +	if (rc)

Remove empty line in between since this is the error handling.

> +		return rc;
> +
> +	switch (psp) {
> +	case POWER_SUPPLY_PROP_STATUS:
> +		if (ecbat->bat_status & BIT(0))

Name bits with defines.

> +			val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
> +		else if (ecbat->bat_status & BIT(1))

Ditto.

> +			val->intval = POWER_SUPPLY_STATUS_CHARGING;
> +		else if (yoga_c630_psy_is_charged(ecbat))
> +			val->intval = POWER_SUPPLY_STATUS_FULL;
> +		else
> +			val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING;
> +		break;
> +	case POWER_SUPPLY_PROP_PRESENT:
> +		val->intval = ecbat->bat_present;
> +		break;
> +	case POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN:
> +		val->intval = ecbat->design_voltage;
> +		break;
> +	case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
> +	case POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN:
> +		val->intval = ecbat->design_capacity;
> +		break;
> +	case POWER_SUPPLY_PROP_CHARGE_FULL:
> +	case POWER_SUPPLY_PROP_ENERGY_FULL:
> +		val->intval = ecbat->full_charge_capacity;
> +		break;
> +	case POWER_SUPPLY_PROP_CHARGE_NOW:
> +	case POWER_SUPPLY_PROP_ENERGY_NOW:
> +		val->intval = ecbat->capacity_now;
> +		break;
> +	case POWER_SUPPLY_PROP_CURRENT_NOW:
> +		val->intval = ecbat->current_now;
> +		break;
> +	case POWER_SUPPLY_PROP_POWER_NOW:
> +		val->intval = ecbat->rate_now;
> +		break;
> +	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> +		val->intval = ecbat->voltage_now;
> +		break;
> +	case POWER_SUPPLY_PROP_TECHNOLOGY:
> +		val->intval = POWER_SUPPLY_TECHNOLOGY_LION;
> +		break;
> +	case POWER_SUPPLY_PROP_MODEL_NAME:
> +		val->strval = "PABAS0241231";
> +		break;
> +	case POWER_SUPPLY_PROP_MANUFACTURER:
> +		val->strval = "Compal";
> +		break;
> +	default:
> +		rc = -EINVAL;
> +		break;
> +	}
> +
> +	return rc;
> +}


> +static int yoga_c630_psy_register_bat_psy(struct yoga_c630_psy *ecbat)
> +{
> +	struct power_supply_config bat_cfg = {};
> +
> +	bat_cfg.drv_data = ecbat;
> +	bat_cfg.of_node = ecbat->of_node;
> +	if (ecbat->unit_mA)
> +		ecbat->bat_psy = power_supply_register_no_ws(ecbat->dev, &yoga_c630_psy_bat_psy_desc_mA, &bat_cfg);
> +	else
> +		ecbat->bat_psy = power_supply_register_no_ws(ecbat->dev, &yoga_c630_psy_bat_psy_desc_mWh, &bat_cfg);

Again, it might be easier to see what's different in here if the relevant 
parameter just uses ?: instead of full blown if/else.

> +	if (IS_ERR(ecbat->bat_psy)) {
> +		dev_err(ecbat->dev, "failed to register battery supply\n");
> +		return PTR_ERR(ecbat->bat_psy);
> +	}
> +
> +	return 0;
> +}


-- 
 i.

Re: [PATCH v4 4/6] power: supply: lenovo_yoga_c630_battery: add Lenovo C630 driver

[Dmitry Baryshkov]

On Wed, May 29, 2024 at 06:41:36PM +0300, Ilpo Järvinen wrote:
> On Tue, 28 May 2024, Dmitry Baryshkov wrote:
> 
> > On the Lenovo Yoga C630 WOS laptop the EC provides access to the adapter
> > and battery status. Add the driver to read power supply status on the
> > laptop.
> > 
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >  drivers/power/supply/Kconfig                    |   9 +
> >  drivers/power/supply/Makefile                   |   1 +
> >  drivers/power/supply/lenovo_yoga_c630_battery.c | 479 ++++++++++++++++++++++++
> >  3 files changed, 489 insertions(+)
> > 

> > +
> > +	val = yoga_c630_ec_read16(ec, LENOVO_EC_BAT_DESIGN_CAPACITY);
> > +	if (val < 0)
> > +		return val;
> > +	ecbat->design_capacity = val * 1000;
> 
> Check linux/units.h if some WATT related one matches to that literal 1000.

I'd rather not do that. The capacity might be either in microWatt-hours
or in microAmp-hours. Using WATT will be confusing in the second case.

> > +	msleep(50);
> > +
> > +	val = yoga_c630_ec_read16(ec, LENOVO_EC_BAT_VOLTAGE);
> > +	if (val < 0)
> > +		return val;
> > +	ecbat->voltage_now = val * 1000;
> 
> Ditto.

No, Volts and Amps don't have units in <linux/units.h>

> 
> > +	msleep(50);
> > +
> > +	val = yoga_c630_ec_read16(ec, LENOVO_EC_BAT_CURRENT);
> > +	if (val < 0)
> > +		return val;
> > +	current_mA = sign_extend32(val, 15);
> > +	ecbat->current_now = current_mA * 1000;
> > +	ecbat->rate_now = current_mA * (ecbat->voltage_now / 1000);
> 
> Ditto.

The same

> 
> > +	msleep(50);
> > +
> > +	if (!ecbat->unit_mA)
> > +		ecbat->capacity_now *= 10;
> > +
> > +	ecbat->last_status_update = jiffies;

-- 
With best wishes
Dmitry

Re: [PATCH v4 4/6] power: supply: lenovo_yoga_c630_battery: add Lenovo C630 driver

[Bryan O'Donoghue]

On 28/05/2024 21:44, Dmitry Baryshkov wrote:
> On the Lenovo Yoga C630 WOS laptop the EC provides access to the adapter
> and battery status. Add the driver to read power supply status on the
> laptop.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/power/supply/Kconfig                    |   9 +
>   drivers/power/supply/Makefile                   |   1 +
>   drivers/power/supply/lenovo_yoga_c630_battery.c | 479 ++++++++++++++++++++++++
>   3 files changed, 489 insertions(+)
> 
> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
> index 3e31375491d5..55ab8e90747d 100644
> --- a/drivers/power/supply/Kconfig
> +++ b/drivers/power/supply/Kconfig
> @@ -167,6 +167,15 @@ config BATTERY_LEGO_EV3
>   	help
>   	  Say Y here to enable support for the LEGO MINDSTORMS EV3 battery.
>   
> +config BATTERY_LENOVO_YOGA_C630
> +	tristate "Lenovo Yoga C630 battery"
> +	depends on OF && EC_LENOVO_YOGA_C630
> +	help
> +	  This driver enables battery support on the Lenovo Yoga C630 laptop.
> +
> +	  To compile the driver as a module, choose M here: the module will be
> +	  called lenovo_yoga_c630_battery.
> +
>   config BATTERY_PMU
>   	tristate "Apple PMU battery"
>   	depends on PPC32 && ADB_PMU
> diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
> index 58b567278034..8ebbdcf92dac 100644
> --- a/drivers/power/supply/Makefile
> +++ b/drivers/power/supply/Makefile
> @@ -32,6 +32,7 @@ obj-$(CONFIG_BATTERY_DS2782)	+= ds2782_battery.o
>   obj-$(CONFIG_BATTERY_GAUGE_LTC2941)	+= ltc2941-battery-gauge.o
>   obj-$(CONFIG_BATTERY_GOLDFISH)	+= goldfish_battery.o
>   obj-$(CONFIG_BATTERY_LEGO_EV3)	+= lego_ev3_battery.o
> +obj-$(CONFIG_BATTERY_LENOVO_YOGA_C630) += lenovo_yoga_c630_battery.o
>   obj-$(CONFIG_BATTERY_PMU)	+= pmu_battery.o
>   obj-$(CONFIG_BATTERY_QCOM_BATTMGR)	+= qcom_battmgr.o
>   obj-$(CONFIG_BATTERY_OLPC)	+= olpc_battery.o
> diff --git a/drivers/power/supply/lenovo_yoga_c630_battery.c b/drivers/power/supply/lenovo_yoga_c630_battery.c
> new file mode 100644
> index 000000000000..76152ad38d46
> --- /dev/null
> +++ b/drivers/power/supply/lenovo_yoga_c630_battery.c
> @@ -0,0 +1,479 @@
> +// 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/delay.h>
> +#include <linux/module.h>
> +#include <linux/platform_data/lenovo-yoga-c630.h>
> +#include <linux/power_supply.h>
> +
> +struct yoga_c630_psy {
> +	struct yoga_c630_ec *ec;
> +	struct device *dev;
> +	struct device_node *of_node;
> +	struct notifier_block nb;
> +	struct mutex lock;

Do locks still not require a

struct mutex lock; /* this mutex locks this thing */

> +
> +	struct power_supply *adp_psy;
> +	struct power_supply *bat_psy;
> +
> +	unsigned long last_status_update;
> +
> +	bool adapter_online;
> +
> +	bool unit_mA;
> +
> +	bool bat_present;
> +	unsigned int bat_status;
> +	unsigned int design_capacity;
> +	unsigned int design_voltage;
> +	unsigned int full_charge_capacity;
> +
> +	unsigned int capacity_now;
> +	unsigned int voltage_now;
> +
> +	int current_now;
> +	int rate_now;
> +};
> +
> +#define LENOVO_EC_CACHE_TIME		(10 * HZ)
> +
> +#define LENOVO_EC_ADPT_STATUS		0xa3
> +#define LENOVO_EC_ADPT_PRESENT		BIT(7)
> +#define LENOVO_EC_BAT_ATTRIBUTES	0xc0
> +#define LENOVO_EC_BAT_ATTR_UNIT_IS_MA	BIT(1)
> +#define LENOVO_EC_BAT_STATUS		0xc1
> +#define LENOVO_EC_BAT_REMAIN_CAPACITY	0xc2
> +#define LENOVO_EC_BAT_VOLTAGE		0xc6
> +#define LENOVO_EC_BAT_DESIGN_VOLTAGE	0xc8
> +#define LENOVO_EC_BAT_DESIGN_CAPACITY	0xca
> +#define LENOVO_EC_BAT_FULL_CAPACITY	0xcc
> +#define LENOVO_EC_BAT_CURRENT		0xd2
> +#define LENOVO_EC_BAT_FULL_FACTORY	0xd6
> +#define LENOVO_EC_BAT_PRESENT		0xda
> +#define LENOVO_EC_BAT_FULL_REGISTER	0xdb
> +#define LENOVO_EC_BAT_FULL_IS_FACTORY	BIT(0)
> +
> +/* the mutex should already be locked */
> +static int yoga_c630_psy_update_bat_info(struct yoga_c630_psy *ecbat)
> +{
> +	struct yoga_c630_ec *ec = ecbat->ec;
> +	int val;
> +
> +	val = yoga_c630_ec_read8(ec, LENOVO_EC_BAT_PRESENT);
> +	if (val < 0)
> +		return val;
> +	ecbat->bat_present = !!(val & BIT(0));
> +	if (!ecbat->bat_present)
> +		return val;
> +
> +	val = yoga_c630_ec_read8(ec, LENOVO_EC_BAT_ATTRIBUTES);
> +	if (val < 0)
> +		return val;
> +	ecbat->unit_mA = val & LENOVO_EC_BAT_ATTR_UNIT_IS_MA;
> +
> +	val = yoga_c630_ec_read16(ec, LENOVO_EC_BAT_DESIGN_CAPACITY);
> +	if (val < 0)
> +		return val;
> +	ecbat->design_capacity = val * 1000;
> +
> +	msleep(50);

What's this for ? Also do you really want to hold a mutex for 50 
milliseconds ?

> +
> +	val = yoga_c630_ec_read16(ec, LENOVO_EC_BAT_DESIGN_VOLTAGE);
> +	if (val < 0)
> +		return val;
> +	ecbat->design_voltage = val;
> +
> +	msleep(50);

And again ?

I guess it doesn't really matter how long you hold your mutex but, some 
description of the delay in the code would be nice from a reader's 
perspective.

Same comment for the rest of the msleeps();

> +
> +	val = yoga_c630_ec_read8(ec, LENOVO_EC_BAT_FULL_REGISTER);
> +	if (val < 0)
> +		return val;
> +	if (val & LENOVO_EC_BAT_FULL_IS_FACTORY)
> +		val = yoga_c630_ec_read16(ec, LENOVO_EC_BAT_FULL_FACTORY);
> +	else
> +		val = yoga_c630_ec_read16(ec, LENOVO_EC_BAT_FULL_CAPACITY);
> +	if (val < 0)
> +		return val;
> +
> +	ecbat->full_charge_capacity = val * 1000;
> +
> +	if (!ecbat->unit_mA) {
> +		ecbat->design_capacity *= 10;
> +		ecbat->full_charge_capacity *= 10;
> +	}
> +
> +	return 0;
> +}
> +
> +/* the mutex should already be locked */
> +static int yoga_c630_psy_maybe_update_bat_status(struct yoga_c630_psy *ecbat)
> +{
> +	struct yoga_c630_ec *ec = ecbat->ec;
> +	int current_mA;
> +	int val;
> +
> +	if (time_before(jiffies, ecbat->last_status_update + LENOVO_EC_CACHE_TIME))
> +		return 0;
> +
> +	val = yoga_c630_ec_read8(ec, LENOVO_EC_BAT_STATUS);
> +	if (val < 0)
> +		return val;
> +	ecbat->bat_status = val;
> +
> +	msleep(50);
> +
> +	val = yoga_c630_ec_read16(ec, LENOVO_EC_BAT_REMAIN_CAPACITY);
> +	if (val < 0)
> +		return val;
> +	ecbat->capacity_now = val * 1000;
> +
> +	msleep(50);
> +
> +	val = yoga_c630_ec_read16(ec, LENOVO_EC_BAT_VOLTAGE);
> +	if (val < 0)
> +		return val;
> +	ecbat->voltage_now = val * 1000;
> +
> +	msleep(50);
> +
> +	val = yoga_c630_ec_read16(ec, LENOVO_EC_BAT_CURRENT);
> +	if (val < 0)
> +		return val;
> +	current_mA = sign_extend32(val, 15);
> +	ecbat->current_now = current_mA * 1000;
> +	ecbat->rate_now = current_mA * (ecbat->voltage_now / 1000);
> +
> +	msleep(50);
> +
> +	if (!ecbat->unit_mA)
> +		ecbat->capacity_now *= 10;
> +
> +	ecbat->last_status_update = jiffies;
> +
> +	return 0;
> +}
> +
> +static int yoga_c630_psy_update_adapter_status(struct yoga_c630_psy *ecbat)
> +{
> +	struct yoga_c630_ec *ec = ecbat->ec;
> +	int val;
> +
> +	mutex_lock(&ecbat->lock);
> +
> +	val = yoga_c630_ec_read8(ec, LENOVO_EC_ADPT_STATUS);
> +	if (val > 0)
> +		ecbat->adapter_online = FIELD_GET(LENOVO_EC_ADPT_PRESENT, val);
> +
> +	mutex_unlock(&ecbat->lock);
> +
> +	return val;
> +}
> +
> +static bool yoga_c630_psy_is_charged(struct yoga_c630_psy *ecbat)
> +{
> +	if (ecbat->bat_status != 0)
> +		return false;
> +
> +	if (ecbat->full_charge_capacity <= ecbat->capacity_now)
> +		return true;
> +
> +	if (ecbat->design_capacity <= ecbat->capacity_now)
> +		return true;
> +
> +	return false;
> +}
> +
> +static int yoga_c630_psy_bat_get_property(struct power_supply *psy,
> +					 enum power_supply_property psp,
> +					 union power_supply_propval *val)
> +{
> +	struct yoga_c630_psy *ecbat = power_supply_get_drvdata(psy);
> +	int rc = 0;
> +
> +	if (!ecbat->bat_present &&
> +	    psp != POWER_SUPPLY_PROP_PRESENT)
> +		return -ENODEV;
> +
> +	mutex_lock(&ecbat->lock);
> +	rc = yoga_c630_psy_maybe_update_bat_status(ecbat);
> +	mutex_unlock(&ecbat->lock);
> +
> +	if (rc)
> +		return rc;
> +
> +	switch (psp) {
> +	case POWER_SUPPLY_PROP_STATUS:
> +		if (ecbat->bat_status & BIT(0))
> +			val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
> +		else if (ecbat->bat_status & BIT(1))

For preference I'd name these bits.

> +			val->intval = POWER_SUPPLY_STATUS_CHARGING;
> +		else if (yoga_c630_psy_is_charged(ecbat))
> +			val->intval = POWER_SUPPLY_STATUS_FULL;
> +		else
> +			val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING;
> +		break;
> +	case POWER_SUPPLY_PROP_PRESENT:
> +		val->intval = ecbat->bat_present;
> +		break;
> +	case POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN:
> +		val->intval = ecbat->design_voltage;
> +		break;
> +	case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
> +	case POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN:
> +		val->intval = ecbat->design_capacity;
> +		break;
> +	case POWER_SUPPLY_PROP_CHARGE_FULL:
> +	case POWER_SUPPLY_PROP_ENERGY_FULL:
> +		val->intval = ecbat->full_charge_capacity;
> +		break;
> +	case POWER_SUPPLY_PROP_CHARGE_NOW:
> +	case POWER_SUPPLY_PROP_ENERGY_NOW:
> +		val->intval = ecbat->capacity_now;
> +		break;
> +	case POWER_SUPPLY_PROP_CURRENT_NOW:
> +		val->intval = ecbat->current_now;
> +		break;
> +	case POWER_SUPPLY_PROP_POWER_NOW:
> +		val->intval = ecbat->rate_now;
> +		break;
> +	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> +		val->intval = ecbat->voltage_now;
> +		break;
> +	case POWER_SUPPLY_PROP_TECHNOLOGY:
> +		val->intval = POWER_SUPPLY_TECHNOLOGY_LION;
> +		break;
> +	case POWER_SUPPLY_PROP_MODEL_NAME:
> +		val->strval = "PABAS0241231";
> +		break;
> +	case POWER_SUPPLY_PROP_MANUFACTURER:
> +		val->strval = "Compal";
> +		break;
> +	default:
> +		rc = -EINVAL;
> +		break;
> +	}
> +
> +	return rc;
> +}
> +
> +static enum power_supply_property yoga_c630_psy_bat_mA_properties[] = {
> +	POWER_SUPPLY_PROP_STATUS,
> +	POWER_SUPPLY_PROP_PRESENT,
> +	POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN,
> +	POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN,
> +	POWER_SUPPLY_PROP_CHARGE_FULL,
> +	POWER_SUPPLY_PROP_CHARGE_NOW,
> +	POWER_SUPPLY_PROP_CURRENT_NOW,
> +	POWER_SUPPLY_PROP_POWER_NOW,
> +	POWER_SUPPLY_PROP_VOLTAGE_NOW,
> +	POWER_SUPPLY_PROP_TECHNOLOGY,
> +	POWER_SUPPLY_PROP_MODEL_NAME,
> +	POWER_SUPPLY_PROP_MANUFACTURER,
> +};
> +
> +static enum power_supply_property yoga_c630_psy_bat_mWh_properties[] = {
> +	POWER_SUPPLY_PROP_STATUS,
> +	POWER_SUPPLY_PROP_PRESENT,
> +	POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN,
> +	POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN,
> +	POWER_SUPPLY_PROP_ENERGY_FULL,
> +	POWER_SUPPLY_PROP_ENERGY_NOW,
> +	POWER_SUPPLY_PROP_CURRENT_NOW,
> +	POWER_SUPPLY_PROP_POWER_NOW,
> +	POWER_SUPPLY_PROP_VOLTAGE_NOW,
> +	POWER_SUPPLY_PROP_TECHNOLOGY,
> +	POWER_SUPPLY_PROP_MODEL_NAME,
> +	POWER_SUPPLY_PROP_MANUFACTURER,
> +};
> +
> +static const struct power_supply_desc yoga_c630_psy_bat_psy_desc_mA = {
> +	.name = "yoga-c630-battery",
> +	.type = POWER_SUPPLY_TYPE_BATTERY,
> +	.properties = yoga_c630_psy_bat_mA_properties,
> +	.num_properties = ARRAY_SIZE(yoga_c630_psy_bat_mA_properties),
> +	.get_property = yoga_c630_psy_bat_get_property,
> +};
> +
> +static const struct power_supply_desc yoga_c630_psy_bat_psy_desc_mWh = {
> +	.name = "yoga-c630-battery",
> +	.type = POWER_SUPPLY_TYPE_BATTERY,
> +	.properties = yoga_c630_psy_bat_mWh_properties,
> +	.num_properties = ARRAY_SIZE(yoga_c630_psy_bat_mWh_properties),
> +	.get_property = yoga_c630_psy_bat_get_property,
> +};
> +
> +static int yoga_c630_psy_adpt_get_property(struct power_supply *psy,
> +					  enum power_supply_property psp,
> +					  union power_supply_propval *val)
> +{
> +	struct yoga_c630_psy *ecbat = power_supply_get_drvdata(psy);
> +	int rc = 0;
> +
> +	yoga_c630_psy_update_adapter_status(ecbat);
> +
> +	switch (psp) {
> +	case POWER_SUPPLY_PROP_ONLINE:
> +		val->intval = ecbat->adapter_online;
> +		break;
> +	default:
> +		rc = -EINVAL;
> +		break;
> +	}
> +
> +	return rc;
> +}
> +
> +static enum power_supply_property yoga_c630_psy_adpt_properties[] = {
> +	POWER_SUPPLY_PROP_ONLINE,
> +};
> +
> +static const struct power_supply_desc yoga_c630_psy_adpt_psy_desc = {
> +	.name = "yoga-c630-adapter",
> +	.type = POWER_SUPPLY_TYPE_USB_TYPE_C,
> +	.properties = yoga_c630_psy_adpt_properties,
> +	.num_properties = ARRAY_SIZE(yoga_c630_psy_adpt_properties),
> +	.get_property = yoga_c630_psy_adpt_get_property,
> +};
> +
> +static int yoga_c630_psy_register_bat_psy(struct yoga_c630_psy *ecbat)
> +{
> +	struct power_supply_config bat_cfg = {};
> +
> +	bat_cfg.drv_data = ecbat;
> +	bat_cfg.of_node = ecbat->of_node;
> +	if (ecbat->unit_mA)
> +		ecbat->bat_psy = power_supply_register_no_ws(ecbat->dev, &yoga_c630_psy_bat_psy_desc_mA, &bat_cfg);
> +	else
> +		ecbat->bat_psy = power_supply_register_no_ws(ecbat->dev, &yoga_c630_psy_bat_psy_desc_mWh, &bat_cfg);

These look a bit long, in the other driver in this series you're capping 
at whatever it is 75 or 80 characters.

TBH I think I prefer the longer line above maybe consider elongating the 
length of the line in the previous driver or curtailing to 80 here.

But I think you should have a consistent line lenght in the same series.

> +	if (IS_ERR(ecbat->bat_psy)) {
> +		dev_err(ecbat->dev, "failed to register battery supply\n");
> +		return PTR_ERR(ecbat->bat_psy);
> +	}
> +
> +	return 0;
> +}
> +
> +static void yoga_c630_ec_refresh_bat_info(struct yoga_c630_psy *ecbat)
> +{
> +	bool current_unit;
> +
> +	mutex_lock(&ecbat->lock);
> +	current_unit = ecbat->unit_mA;
> +
> +	yoga_c630_psy_update_bat_info(ecbat);
> +
> +	if (current_unit != ecbat->unit_mA) {
> +		power_supply_unregister(ecbat->bat_psy);
> +		yoga_c630_psy_register_bat_psy(ecbat);
> +	}
> +
> +	mutex_unlock(&ecbat->lock);
> +}
> +
> +static int yoga_c630_psy_notify(struct notifier_block *nb,
> +				unsigned long action, void *data)
> +{
> +	struct yoga_c630_psy *ecbat = container_of(nb, struct yoga_c630_psy, nb);
> +
> +	switch (action) {
> +	case LENOVO_EC_EVENT_BAT_INFO:
> +		yoga_c630_ec_refresh_bat_info(ecbat);
> +		break;
> +	case LENOVO_EC_EVENT_BAT_ADPT_STATUS:
> +		power_supply_changed(ecbat->adp_psy);
> +		fallthrough;
> +	case LENOVO_EC_EVENT_BAT_STATUS:
> +		power_supply_changed(ecbat->bat_psy);
> +		break;
> +	}
> +
> +	return NOTIFY_OK;
> +}
> +
> +static int yoga_c630_psy_probe(struct auxiliary_device *adev,
> +				   const struct auxiliary_device_id *id)
> +{
> +	struct yoga_c630_ec *ec = adev->dev.platform_data;
> +	struct power_supply_config adp_cfg = {};
> +	struct device *dev = &adev->dev;
> +	struct yoga_c630_psy *ecbat;
> +	int ret;
> +
> +	ecbat = devm_kzalloc(&adev->dev, sizeof(*ecbat), GFP_KERNEL);
> +	if (!ecbat)
> +		return -ENOMEM;
> +
> +	ecbat->ec = ec;
> +	ecbat->dev = dev;
> +	mutex_init(&ecbat->lock);
> +	ecbat->of_node = adev->dev.parent->of_node;
> +	ecbat->nb.notifier_call = yoga_c630_psy_notify;
> +
> +	auxiliary_set_drvdata(adev, ecbat);
> +
> +	adp_cfg.drv_data = ecbat;
> +	adp_cfg.of_node = ecbat->of_node;
> +	adp_cfg.supplied_to = (char **)&yoga_c630_psy_bat_psy_desc_mA.name;
> +	adp_cfg.num_supplicants = 1;
> +	ecbat->adp_psy = devm_power_supply_register_no_ws(dev, &yoga_c630_psy_adpt_psy_desc, &adp_cfg);
> +	if (IS_ERR(ecbat->adp_psy)) {
> +		dev_err(dev, "failed to register AC adapter supply\n");
> +		return PTR_ERR(ecbat->adp_psy);
> +	}
> +
> +	mutex_lock(&ecbat->lock);

Do you really need this lock here in your probe() function ? What's the 
parallel path of execution you are mitigating against here ?

> +
> +	ret = yoga_c630_psy_update_bat_info(ecbat);
> +	if (ret)
> +		goto err_unlock;
> +
> +	ret = yoga_c630_psy_register_bat_psy(ecbat);
> +	if (ret)
> +		goto err_unlock;
> +
> +	mutex_unlock(&ecbat->lock);
> +
> +	ret = yoga_c630_ec_register_notify(ecbat->ec, &ecbat->nb);
> +	if (ret)
> +		goto err_unreg_bat;
> +
> +	return 0;
> +
> +err_unlock:
> +	mutex_unlock(&ecbat->lock);
> +
> +err_unreg_bat:
> +	power_supply_unregister(ecbat->bat_psy);
> +	return ret;
> +}
> +
> +static void yoga_c630_psy_remove(struct auxiliary_device *adev)
> +{
> +	struct yoga_c630_psy *ecbat = auxiliary_get_drvdata(adev);
> +
> +	yoga_c630_ec_unregister_notify(ecbat->ec, &ecbat->nb);
> +	power_supply_unregister(ecbat->bat_psy);
> +}
> +
> +static const struct auxiliary_device_id yoga_c630_psy_id_table[] = {
> +	{ .name = YOGA_C630_MOD_NAME "." YOGA_C630_DEV_PSY, },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(auxiliary, yoga_c630_psy_id_table);
> +
> +static struct auxiliary_driver yoga_c630_psy_driver = {
> +	.name = YOGA_C630_DEV_PSY,
> +	.id_table = yoga_c630_psy_id_table,
> +	.probe = yoga_c630_psy_probe,
> +	.remove = yoga_c630_psy_remove,
> +};
> +
> +module_auxiliary_driver(yoga_c630_psy_driver);
> +
> +MODULE_DESCRIPTION("Lenovo Yoga C630 psy");
> +MODULE_LICENSE("GPL");
> 

Re: [PATCH v4 4/6] power: supply: lenovo_yoga_c630_battery: add Lenovo C630 driver

[Dmitry Baryshkov]

On Wed, May 29, 2024 at 04:51:54PM +0100, Bryan O'Donoghue wrote:
> On 28/05/2024 21:44, Dmitry Baryshkov wrote:
> > On the Lenovo Yoga C630 WOS laptop the EC provides access to the adapter
> > and battery status. Add the driver to read power supply status on the
> > laptop.
> > 
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >   drivers/power/supply/Kconfig                    |   9 +
> >   drivers/power/supply/Makefile                   |   1 +
> >   drivers/power/supply/lenovo_yoga_c630_battery.c | 479 ++++++++++++++++++++++++
> >   3 files changed, 489 insertions(+)
> > 
> > diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
> > index 3e31375491d5..55ab8e90747d 100644
> > --- a/drivers/power/supply/Kconfig
> > +++ b/drivers/power/supply/Kconfig
> > @@ -167,6 +167,15 @@ config BATTERY_LEGO_EV3
> >   	help
> >   	  Say Y here to enable support for the LEGO MINDSTORMS EV3 battery.
> > +config BATTERY_LENOVO_YOGA_C630
> > +	tristate "Lenovo Yoga C630 battery"
> > +	depends on OF && EC_LENOVO_YOGA_C630
> > +	help
> > +	  This driver enables battery support on the Lenovo Yoga C630 laptop.
> > +
> > +	  To compile the driver as a module, choose M here: the module will be
> > +	  called lenovo_yoga_c630_battery.
> > +
> >   config BATTERY_PMU
> >   	tristate "Apple PMU battery"
> >   	depends on PPC32 && ADB_PMU
> > diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
> > index 58b567278034..8ebbdcf92dac 100644
> > --- a/drivers/power/supply/Makefile
> > +++ b/drivers/power/supply/Makefile
> > @@ -32,6 +32,7 @@ obj-$(CONFIG_BATTERY_DS2782)	+= ds2782_battery.o
> >   obj-$(CONFIG_BATTERY_GAUGE_LTC2941)	+= ltc2941-battery-gauge.o
> >   obj-$(CONFIG_BATTERY_GOLDFISH)	+= goldfish_battery.o
> >   obj-$(CONFIG_BATTERY_LEGO_EV3)	+= lego_ev3_battery.o
> > +obj-$(CONFIG_BATTERY_LENOVO_YOGA_C630) += lenovo_yoga_c630_battery.o
> >   obj-$(CONFIG_BATTERY_PMU)	+= pmu_battery.o
> >   obj-$(CONFIG_BATTERY_QCOM_BATTMGR)	+= qcom_battmgr.o
> >   obj-$(CONFIG_BATTERY_OLPC)	+= olpc_battery.o
> > diff --git a/drivers/power/supply/lenovo_yoga_c630_battery.c b/drivers/power/supply/lenovo_yoga_c630_battery.c
> > new file mode 100644
> > index 000000000000..76152ad38d46
> > --- /dev/null
> > +++ b/drivers/power/supply/lenovo_yoga_c630_battery.c
> > @@ -0,0 +1,479 @@
> > +// 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/delay.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_data/lenovo-yoga-c630.h>
> > +#include <linux/power_supply.h>
> > +
> > +struct yoga_c630_psy {
> > +	struct yoga_c630_ec *ec;
> > +	struct device *dev;
> > +	struct device_node *of_node;
> > +	struct notifier_block nb;
> > +	struct mutex lock;
> 
> Do locks still not require a
> 
> struct mutex lock; /* this mutex locks this thing */

Not required, but let me add the doc.

> 
> > +
> > +	struct power_supply *adp_psy;
> > +	struct power_supply *bat_psy;
> > +
> > +	unsigned long last_status_update;
> > +
> > +	bool adapter_online;
> > +
> > +	bool unit_mA;
> > +
> > +	bool bat_present;
> > +	unsigned int bat_status;
> > +	unsigned int design_capacity;
> > +	unsigned int design_voltage;
> > +	unsigned int full_charge_capacity;
> > +
> > +	unsigned int capacity_now;
> > +	unsigned int voltage_now;
> > +
> > +	int current_now;
> > +	int rate_now;
> > +};
> > +
> > +#define LENOVO_EC_CACHE_TIME		(10 * HZ)
> > +
> > +#define LENOVO_EC_ADPT_STATUS		0xa3
> > +#define LENOVO_EC_ADPT_PRESENT		BIT(7)
> > +#define LENOVO_EC_BAT_ATTRIBUTES	0xc0
> > +#define LENOVO_EC_BAT_ATTR_UNIT_IS_MA	BIT(1)
> > +#define LENOVO_EC_BAT_STATUS		0xc1
> > +#define LENOVO_EC_BAT_REMAIN_CAPACITY	0xc2
> > +#define LENOVO_EC_BAT_VOLTAGE		0xc6
> > +#define LENOVO_EC_BAT_DESIGN_VOLTAGE	0xc8
> > +#define LENOVO_EC_BAT_DESIGN_CAPACITY	0xca
> > +#define LENOVO_EC_BAT_FULL_CAPACITY	0xcc
> > +#define LENOVO_EC_BAT_CURRENT		0xd2
> > +#define LENOVO_EC_BAT_FULL_FACTORY	0xd6
> > +#define LENOVO_EC_BAT_PRESENT		0xda
> > +#define LENOVO_EC_BAT_FULL_REGISTER	0xdb
> > +#define LENOVO_EC_BAT_FULL_IS_FACTORY	BIT(0)
> > +
> > +/* the mutex should already be locked */
> > +static int yoga_c630_psy_update_bat_info(struct yoga_c630_psy *ecbat)
> > +{
> > +	struct yoga_c630_ec *ec = ecbat->ec;
> > +	int val;
> > +
> > +	val = yoga_c630_ec_read8(ec, LENOVO_EC_BAT_PRESENT);
> > +	if (val < 0)
> > +		return val;
> > +	ecbat->bat_present = !!(val & BIT(0));
> > +	if (!ecbat->bat_present)
> > +		return val;
> > +
> > +	val = yoga_c630_ec_read8(ec, LENOVO_EC_BAT_ATTRIBUTES);
> > +	if (val < 0)
> > +		return val;
> > +	ecbat->unit_mA = val & LENOVO_EC_BAT_ATTR_UNIT_IS_MA;
> > +
> > +	val = yoga_c630_ec_read16(ec, LENOVO_EC_BAT_DESIGN_CAPACITY);
> > +	if (val < 0)
> > +		return val;
> > +	ecbat->design_capacity = val * 1000;
> > +
> > +	msleep(50);
> 
> What's this for ? Also do you really want to hold a mutex for 50
> milliseconds ?

DSDT has these delays after each read, so I can only assume it is required.
Sleeping outside of the mutex() would mean that a concurrent thread
might break into this delay and query the EC.

[skipped]


> > +static int yoga_c630_psy_probe(struct auxiliary_device *adev,
> > +				   const struct auxiliary_device_id *id)
> > +{
> > +	struct yoga_c630_ec *ec = adev->dev.platform_data;
> > +	struct power_supply_config adp_cfg = {};
> > +	struct device *dev = &adev->dev;
> > +	struct yoga_c630_psy *ecbat;
> > +	int ret;
> > +
> > +	ecbat = devm_kzalloc(&adev->dev, sizeof(*ecbat), GFP_KERNEL);
> > +	if (!ecbat)
> > +		return -ENOMEM;
> > +
> > +	ecbat->ec = ec;
> > +	ecbat->dev = dev;
> > +	mutex_init(&ecbat->lock);
> > +	ecbat->of_node = adev->dev.parent->of_node;
> > +	ecbat->nb.notifier_call = yoga_c630_psy_notify;
> > +
> > +	auxiliary_set_drvdata(adev, ecbat);
> > +
> > +	adp_cfg.drv_data = ecbat;
> > +	adp_cfg.of_node = ecbat->of_node;
> > +	adp_cfg.supplied_to = (char **)&yoga_c630_psy_bat_psy_desc_mA.name;
> > +	adp_cfg.num_supplicants = 1;
> > +	ecbat->adp_psy = devm_power_supply_register_no_ws(dev, &yoga_c630_psy_adpt_psy_desc, &adp_cfg);
> > +	if (IS_ERR(ecbat->adp_psy)) {
> > +		dev_err(dev, "failed to register AC adapter supply\n");
> > +		return PTR_ERR(ecbat->adp_psy);
> > +	}
> > +
> > +	mutex_lock(&ecbat->lock);
> 
> Do you really need this lock here in your probe() function ? What's the
> parallel path of execution you are mitigating against here ?

Notifications from the battery driver can already happen at this point.
Also once the fist power supply is registered, userspace can potentially
access it, triggering EC access and updates of the PSY registration.

> 
> > +
> > +	ret = yoga_c630_psy_update_bat_info(ecbat);
> > +	if (ret)
> > +		goto err_unlock;
> > +
> > +	ret = yoga_c630_psy_register_bat_psy(ecbat);
> > +	if (ret)
> > +		goto err_unlock;
> > +
> > +	mutex_unlock(&ecbat->lock);
> > +


-- 
With best wishes
Dmitry

Re: [PATCH v4 4/6] power: supply: lenovo_yoga_c630_battery: add Lenovo C630 driver

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 2130 bytes --]

Hi,

Thanks for your patch. I have a few more comments.

On Tue, May 28, 2024 at 11:44:49PM +0300, Dmitry Baryshkov wrote:
...
> +struct yoga_c630_psy {
> +       struct yoga_c630_ec *ec;
> +       struct device *dev;
> +       struct device_node *of_node;

Please use fwnode

> +static enum power_supply_property yoga_c630_psy_bat_mA_properties[] = {
> +	POWER_SUPPLY_PROP_STATUS,
> +	POWER_SUPPLY_PROP_PRESENT,
> +	POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN,
> +	POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN,
> +	POWER_SUPPLY_PROP_CHARGE_FULL,
> +	POWER_SUPPLY_PROP_CHARGE_NOW,
> +	POWER_SUPPLY_PROP_CURRENT_NOW,
> +	POWER_SUPPLY_PROP_POWER_NOW,
> +	POWER_SUPPLY_PROP_VOLTAGE_NOW,
> +	POWER_SUPPLY_PROP_TECHNOLOGY,
> +	POWER_SUPPLY_PROP_MODEL_NAME,
> +	POWER_SUPPLY_PROP_MANUFACTURER,
> +};
> +
> +static enum power_supply_property yoga_c630_psy_bat_mWh_properties[] = {
> +	POWER_SUPPLY_PROP_STATUS,
> +	POWER_SUPPLY_PROP_PRESENT,
> +	POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN,
> +	POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN,
> +	POWER_SUPPLY_PROP_ENERGY_FULL,
> +	POWER_SUPPLY_PROP_ENERGY_NOW,
> +	POWER_SUPPLY_PROP_CURRENT_NOW,
> +	POWER_SUPPLY_PROP_POWER_NOW,
> +	POWER_SUPPLY_PROP_VOLTAGE_NOW,
> +	POWER_SUPPLY_PROP_TECHNOLOGY,
> +	POWER_SUPPLY_PROP_MODEL_NAME,
> +	POWER_SUPPLY_PROP_MANUFACTURER,
> +};

Please also add

POWER_SUPPLY_PROP_SCOPE = POWER_SUPPLY_SCOPE_SYSTEM

> +static enum power_supply_property yoga_c630_psy_adpt_properties[] = {
> +       POWER_SUPPLY_PROP_ONLINE,
> +};

Please also add

POWER_SUPPLY_PROP_USB_TYPE = POWER_SUPPLY_USB_TYPE_C

> +static const struct power_supply_desc yoga_c630_psy_adpt_psy_desc = {
> +	.name = "yoga-c630-adapter",
> +	.type = POWER_SUPPLY_TYPE_USB_TYPE_C,

static const enum power_supply_usb_type yoga_c630_psy_adpt_usb_type[] = {
	POWER_SUPPLY_USB_TYPE_C,
};

.type = POWER_SUPPLY_TYPE_USB
.usb_types = yoga_c630_psy_adpt_usb_type
.num_usb_types = ARRAY_SIZE(yoga_c630_psy_adpt_usb_type),

> +	.properties = yoga_c630_psy_adpt_properties,
> +	.num_properties = ARRAY_SIZE(yoga_c630_psy_adpt_properties),
> +	.get_property = yoga_c630_psy_adpt_get_property,
> +};
> +

Greetings,

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v4 5/6] arm64: dts: qcom: sdm845: describe connections of USB/DP port

[Dmitry Baryshkov]

Describe links between the first USB3 host and the DisplayPort that is
routed to the same pins.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 arch/arm64/boot/dts/qcom/sdm845.dtsi | 53 +++++++++++++++++++++++++++++++++++-
 1 file changed, 52 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index 26b1638c76f9..5276ab0433b6 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -4030,6 +4030,35 @@ usb_1_qmpphy: phy@88e8000 {
 
 			#clock-cells = <1>;
 			#phy-cells = <1>;
+			orientation-switch;
+
+			ports {
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				port@0 {
+					reg = <0>;
+
+					usb_1_qmpphy_out: endpoint {
+					};
+				};
+
+				port@1 {
+					reg = <1>;
+
+					usb_1_qmpphy_usb_ss_in: endpoint {
+						remote-endpoint = <&usb_1_dwc3_ss>;
+					};
+				};
+
+				port@2 {
+					reg = <2>;
+
+					usb_1_qmpphy_dp_in: endpoint {
+						remote-endpoint = <&dp_out>;
+					};
+				};
+			};
 		};
 
 		usb_2_qmpphy: phy@88eb000 {
@@ -4110,6 +4139,26 @@ usb_1_dwc3: usb@a600000 {
 				snps,dis_enblslpm_quirk;
 				phys = <&usb_1_hsphy>, <&usb_1_qmpphy QMP_USB43DP_USB3_PHY>;
 				phy-names = "usb2-phy", "usb3-phy";
+
+				ports {
+					#address-cells = <1>;
+					#size-cells = <0>;
+
+					port@0 {
+						reg = <0>;
+
+						usb_1_dwc3_hs: endpoint {
+						};
+					};
+
+					port@1 {
+						reg = <1>;
+
+						usb_1_dwc3_ss: endpoint {
+							remote-endpoint = <&usb_1_qmpphy_usb_ss_in>;
+						};
+					};
+				};
 			};
 		};
 
@@ -4600,7 +4649,9 @@ dp_in: endpoint {
 
 					port@1 {
 						reg = <1>;
-						dp_out: endpoint { };
+						dp_out: endpoint {
+							remote-endpoint = <&usb_1_qmpphy_dp_in>;
+						};
 					};
 				};
 

-- 
2.39.2

Re: [PATCH v4 5/6] arm64: dts: qcom: sdm845: describe connections of USB/DP port

[Bryan O'Donoghue]

On 28/05/2024 21:44, Dmitry Baryshkov wrote:
> Describe links between the first USB3 host and the DisplayPort that is
> routed to the same pins.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   arch/arm64/boot/dts/qcom/sdm845.dtsi | 53 +++++++++++++++++++++++++++++++++++-
>   1 file changed, 52 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> index 26b1638c76f9..5276ab0433b6 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi

Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

[PATCH v4 6/6] arm64: dts: qcom: c630: Add Embedded Controller node

[Dmitry Baryshkov]

From: Bjorn Andersson <andersson@kernel.org>

The Embedded Controller in the Lenovo Yoga C630 is accessible on &i2c1
and provides battery and adapter status, as well as altmode
notifications for the second USB Type-C port.

Add a definition for the EC.

Signed-off-by: Bjorn Andersson <andersson@kernel.org>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 .../boot/dts/qcom/sdm850-lenovo-yoga-c630.dts      | 75 ++++++++++++++++++++++
 1 file changed, 75 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts b/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts
index 47dc42f6e936..07bff1c5a7ab 100644
--- a/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts
+++ b/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts
@@ -370,6 +370,66 @@ zap-shader {
 &i2c1 {
 	status = "okay";
 	clock-frequency = <400000>;
+
+	embedded-controller@70 {
+		compatible = "lenovo,yoga-c630-ec";
+		reg = <0x70>;
+
+		interrupts-extended = <&tlmm 20 IRQ_TYPE_LEVEL_HIGH>;
+
+		pinctrl-names = "default";
+		pinctrl-0 = <&ec_int_state>;
+
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		connector@0 {
+			compatible = "usb-c-connector";
+			reg = <0>;
+			power-role = "dual";
+			data-role = "host";
+
+			ports {
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				port@0 {
+					reg = <0>;
+
+					ucsi0_hs_in: endpoint {
+						remote-endpoint = <&usb_1_dwc3_hs>;
+					};
+				};
+
+				port@1 {
+					reg = <1>;
+
+					ucsi0_ss_in: endpoint {
+						remote-endpoint = <&usb_1_qmpphy_out>;
+					};
+				};
+
+				port@2 {
+					reg = <2>;
+
+					ucsi0_sbu: endpoint {
+					};
+				};
+			};
+		};
+
+		connector@1 {
+			compatible = "usb-c-connector";
+			reg = <1>;
+			power-role = "dual";
+			data-role = "host";
+
+			/*
+			 * connected to the onboard USB hub, orientation is
+			 * handled by the controller
+			 */
+		};
+	};
 };
 
 &i2c3 {
@@ -694,6 +754,13 @@ mode_pin_active: mode-pin-state {
 
 		bias-disable;
 	};
+
+	ec_int_state: ec-int-state {
+		pins = "gpio20";
+		function = "gpio";
+
+		bias-disable;
+	};
 };
 
 &uart6 {
@@ -741,6 +808,10 @@ &usb_1_dwc3 {
 	dr_mode = "host";
 };
 
+&usb_1_dwc3_hs {
+	remote-endpoint = <&ucsi0_hs_in>;
+};
+
 &usb_1_hsphy {
 	status = "okay";
 
@@ -761,6 +832,10 @@ &usb_1_qmpphy {
 	vdda-pll-supply = <&vdda_usb1_ss_core>;
 };
 
+&usb_1_qmpphy_out {
+	remote-endpoint = <&ucsi0_ss_in>;
+};
+
 &usb_2 {
 	status = "okay";
 };

-- 
2.39.2