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

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

[Bjorn Andersson]

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

Bjorn Andersson (2):
  dt-bindings: power: supply: Add Lenovo Yoga C630 EC
  power: supply: Add Lenovo Yoga C630 EC driver

 .../power/supply/lenovo,yoga-c630-ec.yaml     |  88 +++
 drivers/power/supply/Kconfig                  |  11 +
 drivers/power/supply/Makefile                 |   1 +
 drivers/power/supply/yoga-c630-ec.c           | 547 ++++++++++++++++++
 4 files changed, 647 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/supply/lenovo,yoga-c630-ec.yaml
 create mode 100644 drivers/power/supply/yoga-c630-ec.c

-- 
2.37.1

[PATCH 1/2] dt-bindings: power: supply: Add Lenovo Yoga C630 EC

[Bjorn Andersson]

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

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 .../power/supply/lenovo,yoga-c630-ec.yaml     | 88 +++++++++++++++++++
 1 file changed, 88 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/supply/lenovo,yoga-c630-ec.yaml

diff --git a/Documentation/devicetree/bindings/power/supply/lenovo,yoga-c630-ec.yaml b/Documentation/devicetree/bindings/power/supply/lenovo,yoga-c630-ec.yaml
new file mode 100644
index 000000000000..2dceb57a56b1
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/supply/lenovo,yoga-c630-ec.yaml
@@ -0,0 +1,88 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/power/supply/lenovo,yoga-c630-ec.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Lenovo Yoga C630 Embedded Controller.
+
+maintainers:
+  - Bjorn Andersson <bjorn.andersson@linaro.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@\d$':
+    $ref: /schemas/connector/usb-connector.yaml#
+
+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";
+
+          ports {
+            #address-cells = <1>;
+            #size-cells = <0>;
+            port@1 {
+              reg = <1>;
+              lenovo_ec_dp_in: endpoint {
+                   remote-endpoint = <&mdss_dp_out>;
+              };
+            };
+          };
+        };
+      };
+    };
+...
-- 
2.37.1

[PATCH 2/2] power: supply: Add Lenovo Yoga C630 EC driver

[Bjorn Andersson]

The Qualcomm Snapdragon-based Lenovo Yoga C630 has some sort of EC
providing AC-adapter and battery status, as well as USB Type-C altmode
notifications for Displayport operation.

The Yoga C630 ships with Windows, where these operations primarily are
implemented in ACPI, but due to various issues with the hardware
representation therein it's not possible to run Linux on this
information. As such this is a best-effort re-implementation of these
operations, based on the register map expressed in ACPI and a fair
amount of trial and error.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 drivers/power/supply/Kconfig        |  11 +
 drivers/power/supply/Makefile       |   1 +
 drivers/power/supply/yoga-c630-ec.c | 547 ++++++++++++++++++++++++++++
 3 files changed, 559 insertions(+)
 create mode 100644 drivers/power/supply/yoga-c630-ec.c

diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
index 1aa8323ad9f6..6e706e948ad2 100644
--- a/drivers/power/supply/Kconfig
+++ b/drivers/power/supply/Kconfig
@@ -897,4 +897,15 @@ config BATTERY_UG3105
 	  device is off or suspended, the functionality of this driver is
 	  limited to reporting capacity only.
 
+config LENOVO_YOGA_C630_EC
+	tristate "Lenovo Yoga C630 EC battery driver"
+	depends on DRM
+	depends on I2C
+	help
+	  Driver for the Embedded Controller in the Qualcomm Snapdragon-based
+	  Lenovo Yoga C630, which provides battery information and USB Type-C
+	  altmode notifications.
+
+	  Say M or Y here to include this support.
+
 endif # POWER_SUPPLY
diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
index 7f02f36aea55..24a4129cc427 100644
--- a/drivers/power/supply/Makefile
+++ b/drivers/power/supply/Makefile
@@ -108,3 +108,4 @@ obj-$(CONFIG_BATTERY_ACER_A500)	+= acer_a500_battery.o
 obj-$(CONFIG_BATTERY_SURFACE)	+= surface_battery.o
 obj-$(CONFIG_CHARGER_SURFACE)	+= surface_charger.o
 obj-$(CONFIG_BATTERY_UG3105)	+= ug3105_battery.o
+obj-$(CONFIG_LENOVO_YOGA_C630_EC)	+= yoga-c630-ec.o
diff --git a/drivers/power/supply/yoga-c630-ec.c b/drivers/power/supply/yoga-c630-ec.c
new file mode 100644
index 000000000000..1fa0b5844e01
--- /dev/null
+++ b/drivers/power/supply/yoga-c630-ec.c
@@ -0,0 +1,547 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2022 Linaro Ltd.
+ */
+#include <linux/bitops.h>
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/power_supply.h>
+#include <linux/usb/typec_mux.h>
+
+#include <drm/drm_bridge.h>
+
+#define LENOVO_EC_CACHE_TIME		(10 * HZ)
+
+#define LENOVO_EC_RESPONSE_REG		0x1
+#define LENOVO_EC_REQUEST_REG		0x2
+
+#define LENOVO_EC_READ_REG		0xb0
+#define LENOVO_EC_QUERY_USB_EVENT	0x8
+#define LENOVO_EC_USB_EVENT_MUX		GENMASK(1, 0)
+#define LENOVO_EC_USB_EVENT_ORIENTATION	BIT(7)
+#define LENOVO_EC_ADPT_STATUS		0xa3
+#define LENOVO_EC_ADPT_PRESENT		BIT(7)
+#define LENOVO_EC_BAT_STATUS		0xc1
+#define LENOVO_EC_BAT_VOLTAGE		0xc6
+#define LENOVO_EC_BAT_CURRENT		0xd2
+#define LENOVO_EC_BAT_PRESENT		0xda
+#define LENOVO_EC_BAT_ATTRIBUTES	0xc0
+#define LENOVO_EC_BAT_ATTR_UNIT_IS_MA	BIT(1)
+#define LENOVO_EC_BAT_REMAIN_CAPACITY	0xc2
+#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_FULL_FACTORY	0xd6
+#define LENOVO_EC_BAT_FULL_REGISTER	0xdb
+
+#define LENOVO_EC_REQUEST_NEXT_EVENT	0x84
+#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
+
+struct yoga_c630_ec {
+	struct i2c_client *client;
+	struct mutex lock;
+
+	struct power_supply *adp_psy;
+	struct power_supply *bat_psy;
+
+	struct typec_switch *typec_switch;
+
+	unsigned long last_status_update;
+
+	bool adapter_online;
+
+	bool unit_ma;
+
+	unsigned int scale;
+
+	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 rate_now;
+
+	bool bridge_configured;
+	struct drm_bridge bridge;
+};
+
+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);
+}
+
+static int yoga_c630_ec_read8(struct yoga_c630_ec *ec, u8 addr)
+{
+	int ret;
+	u8 val;
+
+	ret = yoga_c630_ec_request(ec, (u8[]) { LENOVO_EC_READ_REG, addr }, 2,
+				   &val, 1);
+	if (ret < 0)
+		return ret;
+
+	return val;
+}
+
+static int yoga_c630_ec_read16(struct yoga_c630_ec *ec, u8 addr)
+{
+	int ret;
+	u8 msb;
+	u8 lsb;
+
+	ret = yoga_c630_ec_request(ec, (u8[]) { LENOVO_EC_READ_REG, addr }, 2,
+				   &lsb, 1);
+	if (ret < 0)
+		return ret;
+
+	ret = yoga_c630_ec_request(ec, (u8[]) { LENOVO_EC_READ_REG, addr + 1 }, 2,
+				   &msb, 1);
+	if (ret < 0)
+		return ret;
+
+	return msb << 8 | lsb;
+}
+
+static void yoga_c630_ec_update_bat_info(struct yoga_c630_ec *ec)
+{
+	int val;
+
+	mutex_lock(&ec->lock);
+
+	val = yoga_c630_ec_read8(ec, LENOVO_EC_BAT_PRESENT);
+	if (val < 0)
+		goto out_unlock;
+	ec->bat_present = !!(val & BIT(0));
+	if (!ec->bat_present)
+		goto out_unlock;
+
+	val = yoga_c630_ec_read8(ec, LENOVO_EC_BAT_ATTRIBUTES);
+	if (val < 0)
+		goto out_unlock;
+	ec->unit_ma = val & LENOVO_EC_BAT_ATTR_UNIT_IS_MA;
+	if (!ec->unit_ma)
+		ec->scale = 1000;
+	else
+		ec->scale = 1;
+
+	val = yoga_c630_ec_read16(ec, LENOVO_EC_BAT_DESIGN_CAPACITY);
+	if (val < 0)
+		goto out_unlock;
+	ec->design_capacity = val * ec->scale;
+
+	msleep(50);
+
+	val = yoga_c630_ec_read16(ec, LENOVO_EC_BAT_DESIGN_VOLTAGE);
+	if (val < 0)
+		goto out_unlock;
+	ec->design_voltage = val;
+
+	msleep(50);
+
+	val = yoga_c630_ec_read8(ec, LENOVO_EC_BAT_FULL_REGISTER);
+	if (val < 0)
+		goto out_unlock;
+	if (val & BIT(0))
+		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)
+		goto out_unlock;
+	ec->full_charge_capacity = val * ec->scale;
+
+out_unlock:
+	mutex_unlock(&ec->lock);
+}
+
+static void yoga_c630_ec_maybe_update_bat_status(struct yoga_c630_ec *ec)
+{
+	int val = 0;
+
+	mutex_lock(&ec->lock);
+
+	if (time_before(jiffies, ec->last_status_update + LENOVO_EC_CACHE_TIME))
+		goto out_unlock;
+
+	val = yoga_c630_ec_read8(ec, LENOVO_EC_BAT_STATUS);
+	if (val < 0)
+		goto out_unlock;
+	ec->bat_status = val;
+
+	val = yoga_c630_ec_read16(ec, LENOVO_EC_BAT_REMAIN_CAPACITY);
+	if (val < 0)
+		goto out_unlock;
+	ec->capacity_now = val * ec->scale;
+
+	val = yoga_c630_ec_read16(ec, LENOVO_EC_BAT_VOLTAGE);
+	if (val < 0)
+		goto out_unlock;
+	ec->voltage_now = val;
+
+	val = yoga_c630_ec_read16(ec, LENOVO_EC_BAT_CURRENT);
+	if (val < 0)
+		goto out_unlock;
+	ec->rate_now = sign_extend32(val, 15) * ec->scale;
+
+	if (ec->unit_ma)
+		ec->rate_now = ec->rate_now * ec->voltage_now / 1000;
+
+	ec->last_status_update = jiffies;
+
+out_unlock:
+	mutex_unlock(&ec->lock);
+}
+
+static int yoga_c630_ec_update_adapter_status(struct yoga_c630_ec *ec)
+{
+	int val;
+
+	mutex_lock(&ec->lock);
+
+	val = yoga_c630_ec_read8(ec, LENOVO_EC_ADPT_STATUS);
+	if (val > 0)
+		ec->adapter_online = FIELD_GET(LENOVO_EC_ADPT_PRESENT, val);
+
+	mutex_unlock(&ec->lock);
+
+	return val;
+}
+
+static bool yoga_c630_ec_is_charged(struct yoga_c630_ec *ec)
+{
+	if (ec->bat_status != 0)
+		return false;
+
+	if (ec->full_charge_capacity == ec->capacity_now)
+		return true;
+
+	if (ec->design_capacity == ec->capacity_now)
+		return true;
+
+	return false;
+}
+
+static int yoga_c630_ec_bat_get_property(struct power_supply *psy,
+					 enum power_supply_property psp,
+					 union power_supply_propval *val)
+{
+	struct yoga_c630_ec *ec = power_supply_get_drvdata(psy);
+	int rc = 0;
+
+	if (ec->bat_present)
+		yoga_c630_ec_maybe_update_bat_status(ec);
+	else if (psp != POWER_SUPPLY_PROP_PRESENT)
+		return -ENODEV;
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_STATUS:
+		if (ec->bat_status & BIT(0))
+			val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
+		else if (ec->bat_status & BIT(1))
+			val->intval = POWER_SUPPLY_STATUS_CHARGING;
+		else if (yoga_c630_ec_is_charged(ec))
+			val->intval = POWER_SUPPLY_STATUS_FULL;
+		else
+			val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING;
+		break;
+	case POWER_SUPPLY_PROP_PRESENT:
+		val->intval = ec->bat_present;
+		break;
+	case POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN:
+		val->intval = ec->design_voltage;
+		break;
+	case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
+		val->intval = ec->design_capacity;
+		break;
+	case POWER_SUPPLY_PROP_CHARGE_FULL:
+		val->intval = ec->full_charge_capacity;
+		break;
+	case POWER_SUPPLY_PROP_CHARGE_NOW:
+		val->intval = ec->capacity_now;
+		break;
+	case POWER_SUPPLY_PROP_CURRENT_NOW:
+		val->intval = ec->rate_now;
+		break;
+	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
+		val->intval = ec->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;
+	case POWER_SUPPLY_PROP_SERIAL_NUMBER:
+		val->strval = "05072018";
+		break;
+	default:
+		rc = -EINVAL;
+		break;
+	}
+
+	return rc;
+}
+
+static enum power_supply_property yoga_c630_ec_bat_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_VOLTAGE_NOW,
+	POWER_SUPPLY_PROP_TECHNOLOGY,
+	POWER_SUPPLY_PROP_MODEL_NAME,
+	POWER_SUPPLY_PROP_MANUFACTURER,
+	POWER_SUPPLY_PROP_SERIAL_NUMBER,
+};
+
+static const struct power_supply_desc yoga_c630_ec_bat_psy_desc = {
+	.name = "yoga-c630-battery",
+	.type = POWER_SUPPLY_TYPE_BATTERY,
+	.properties = yoga_c630_ec_bat_properties,
+	.num_properties = ARRAY_SIZE(yoga_c630_ec_bat_properties),
+	.get_property = yoga_c630_ec_bat_get_property,
+};
+
+static int yoga_c630_ec_adpt_get_property(struct power_supply *psy,
+					  enum power_supply_property psp,
+					  union power_supply_propval *val)
+{
+	struct yoga_c630_ec *ec = power_supply_get_drvdata(psy);
+	int rc = 0;
+
+	yoga_c630_ec_update_adapter_status(ec);
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_ONLINE:
+		val->intval = ec->adapter_online;
+		break;
+	default:
+		rc = -EINVAL;
+		break;
+	}
+
+	return rc;
+}
+
+static enum power_supply_property yoga_c630_ec_adpt_properties[] = {
+	POWER_SUPPLY_PROP_ONLINE,
+};
+
+static const struct power_supply_desc yoga_c630_ec_adpt_psy_desc = {
+	.name = "yoga-c630-adapter",
+	.type = POWER_SUPPLY_TYPE_USB_TYPE_C,
+	.properties = yoga_c630_ec_adpt_properties,
+	.num_properties = ARRAY_SIZE(yoga_c630_ec_adpt_properties),
+	.get_property = yoga_c630_ec_adpt_get_property,
+};
+
+static int yoga_c630_ec_attach(struct drm_bridge *bridge,
+			       enum drm_bridge_attach_flags flags)
+{
+	return flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR ? 0 : -EINVAL;
+}
+
+static const struct drm_bridge_funcs yoga_c630_ec_bridge_funcs = {
+	.attach = yoga_c630_ec_attach,
+};
+
+static int yoga_c630_ec_query_usb_event(struct yoga_c630_ec *ec)
+{
+	struct device *dev = &ec->client->dev;
+	enum typec_orientation orientation;
+	enum drm_connector_status status;
+	int event;
+
+	mutex_lock(&ec->lock);
+	event = yoga_c630_ec_read8(ec, LENOVO_EC_QUERY_USB_EVENT);
+	mutex_unlock(&ec->lock);
+	if (event < 0) {
+		dev_err(dev, "unable to query USB event\n");
+		return event;
+	}
+
+	if (FIELD_GET(LENOVO_EC_USB_EVENT_MUX, event))
+		status = connector_status_connected;
+	else
+		status = connector_status_disconnected;
+
+	if (FIELD_GET(LENOVO_EC_USB_EVENT_ORIENTATION, event))
+		orientation = TYPEC_ORIENTATION_REVERSE;
+	else
+		orientation = TYPEC_ORIENTATION_NORMAL;
+
+	typec_switch_set(ec->typec_switch, orientation);
+	if (ec->bridge_configured)
+		drm_bridge_hpd_notify(&ec->bridge, status);
+
+	return 0;
+}
+
+static irqreturn_t yoga_c630_ec_intr(int irq, void *data)
+{
+	struct yoga_c630_ec *ec = data;
+	u8 event;
+	int ret;
+
+	mutex_lock(&ec->lock);
+	ret = yoga_c630_ec_request(ec, (u8[]){ LENOVO_EC_REQUEST_NEXT_EVENT }, 1,
+				   &event, 1);
+	mutex_unlock(&ec->lock);
+	if (ret < 0)
+		return IRQ_HANDLED;
+
+	switch (event) {
+	case LENOVO_EC_EVENT_USB:
+	case LENOVO_EC_EVENT_HPD:
+		yoga_c630_ec_query_usb_event(ec);
+		break;
+	case LENOVO_EC_EVENT_BAT_INFO:
+		yoga_c630_ec_update_bat_info(ec);
+		break;
+	case LENOVO_EC_EVENT_BAT_ADPT_STATUS:
+		power_supply_changed(ec->adp_psy);
+		fallthrough;
+	case LENOVO_EC_EVENT_BAT_STATUS:
+		power_supply_changed(ec->bat_psy);
+		break;
+	}
+
+	return IRQ_HANDLED;
+}
+
+static void yoga_c630_ec_put_switch(void *data)
+{
+	typec_switch_put(data);
+}
+
+static int yoga_c630_ec_probe(struct i2c_client *client,
+			      const struct i2c_device_id *id)
+{
+	struct power_supply_config adp_cfg = {};
+	struct power_supply_config bat_cfg = {};
+	struct fwnode_handle *fwnode;
+	struct yoga_c630_ec *ec;
+	struct device *dev = &client->dev;
+	u32 port;
+	int ret;
+
+	ec = devm_kzalloc(dev, sizeof(*ec), GFP_KERNEL);
+	if (!ec)
+		return -ENOMEM;
+
+	mutex_init(&ec->lock);
+	ec->client = client;
+
+	adp_cfg.drv_data = ec;
+	adp_cfg.of_node = client->dev.of_node;
+	ec->adp_psy = devm_power_supply_register(dev, &yoga_c630_ec_adpt_psy_desc, &adp_cfg);
+	if (IS_ERR(ec->adp_psy)) {
+		dev_err(dev, "failed to register AC adapter supply\n");
+		return PTR_ERR(ec->adp_psy);
+	}
+
+	bat_cfg.drv_data = ec;
+	bat_cfg.of_node = client->dev.of_node;
+	ec->bat_psy = devm_power_supply_register(dev, &yoga_c630_ec_bat_psy_desc, &bat_cfg);
+	if (IS_ERR(ec->bat_psy)) {
+		dev_err(dev, "failed to register battery supply\n");
+		return PTR_ERR(ec->bat_psy);
+	}
+
+	device_for_each_child_node(dev, fwnode) {
+		ret = fwnode_property_read_u32(fwnode, "reg", &port);
+		if (ret < 0)
+			continue;
+
+		/* Got multiple ports, but altmode is only possible on port 1 */
+		if (port != 1)
+			continue;
+
+		ec->bridge.funcs = &yoga_c630_ec_bridge_funcs;
+		ec->bridge.of_node = to_of_node(fwnode);
+		ec->bridge.ops = DRM_BRIDGE_OP_HPD;
+		ec->bridge.type = DRM_MODE_CONNECTOR_USB;
+
+		ret = devm_drm_bridge_add(dev, &ec->bridge);
+		if (ret) {
+			dev_err(dev, "failed to register drm bridge\n");
+			fwnode_handle_put(fwnode);
+			return ret;
+		}
+
+		ec->bridge_configured = true;
+
+		ec->typec_switch = fwnode_typec_switch_get(fwnode);
+		if (IS_ERR(ec->typec_switch))
+			return dev_err_probe(dev, PTR_ERR(ec->typec_switch),
+					     "failed to acquire orientation-switch for port 1\n");
+
+		ret = devm_add_action_or_reset(dev, yoga_c630_ec_put_switch,
+					       ec->typec_switch);
+		if (ret)
+			return ret;
+	}
+
+	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");
+
+	yoga_c630_ec_update_bat_info(ec);
+
+	return 0;
+}
+
+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 EC driver");
+MODULE_LICENSE("GPL");
-- 
2.37.1

Re: [PATCH 0/2] power: supply: Lenovo Yoga C630 EC

[Steev Klimaszewski]

On 8/9/22 10:04 PM, Bjorn Andersson wrote:
> This adds binding and driver for the Lenovo Yoga C630 Embedded Controller, to
> provide battery information and DisplayPort support.
>
> Bjorn Andersson (2):
>    dt-bindings: power: supply: Add Lenovo Yoga C630 EC
>    power: supply: Add Lenovo Yoga C630 EC driver
>
>   .../power/supply/lenovo,yoga-c630-ec.yaml     |  88 +++
>   drivers/power/supply/Kconfig                  |  11 +
>   drivers/power/supply/Makefile                 |   1 +
>   drivers/power/supply/yoga-c630-ec.c           | 547 ++++++++++++++++++
>   4 files changed, 647 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/power/supply/lenovo,yoga-c630-ec.yaml
>   create mode 100644 drivers/power/supply/yoga-c630-ec.c
>
It will be so nice to drop the "some-battery" patches that I've been 
carrying in my kernel sources since 5.7 :D

Tested-by: Steev Klimaszewski <steev@kali.org>

Re: [PATCH 1/2] dt-bindings: power: supply: Add Lenovo Yoga C630 EC

[Krzysztof Kozlowski]

On 10/08/2022 06:04, Bjorn Andersson wrote:
> Add binding for the Embedded Controller found in the Qualcomm
> Snapdragon-based Lenovo Yoga C630.

Thank you for your patch. There is something to discuss/improve.

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

Just to clarify: the EC have physically two USB connectors?

> +
> +  '#size-cells':
> +    const: 0
> +
> +  interrupts:
> +    maxItems: 1
> +
> +patternProperties:
> +  '^connector@\d$':
> +    $ref: /schemas/connector/usb-connector.yaml#

unevaluatedProperties:false inside connector (on its level)

> +
> +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";
> +
> +          ports {
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +            port@1 {
> +              reg = <1>;
> +              lenovo_ec_dp_in: endpoint {
> +                   remote-endpoint = <&mdss_dp_out>;

You have inconsistent indentation. Use 4-spaces for entire DTS example.

Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: power: supply: Add Lenovo Yoga C630 EC

[Rob Herring]

On Tue, 09 Aug 2022 22:04:59 -0500, Bjorn Andersson wrote:
> Add binding for the Embedded Controller found in the Qualcomm
> Snapdragon-based Lenovo Yoga C630.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
>  .../power/supply/lenovo,yoga-c630-ec.yaml     | 88 +++++++++++++++++++
>  1 file changed, 88 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/supply/lenovo,yoga-c630-ec.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/power/supply/lenovo,yoga-c630-ec.example.dtb: embedded-controller@70: connector@1:ports: 'port@0' is a required property
	From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/power/supply/lenovo,yoga-c630-ec.yaml
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/power/supply/lenovo,yoga-c630-ec.example.dtb: connector@1: ports: 'port@0' is a required property
	From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/connector/usb-connector.yaml

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

Re: [PATCH 0/2] power: supply: Lenovo Yoga C630 EC

[Bjorn Andersson]

On Tue 09 Aug 21:04 PDT 2022, Steev Klimaszewski wrote:

> 
> On 8/9/22 10:04 PM, Bjorn Andersson wrote:
> > This adds binding and driver for the Lenovo Yoga C630 Embedded Controller, to
> > provide battery information and DisplayPort support.
> > 
> > Bjorn Andersson (2):
> >    dt-bindings: power: supply: Add Lenovo Yoga C630 EC
> >    power: supply: Add Lenovo Yoga C630 EC driver
> > 
> >   .../power/supply/lenovo,yoga-c630-ec.yaml     |  88 +++
> >   drivers/power/supply/Kconfig                  |  11 +
> >   drivers/power/supply/Makefile                 |   1 +
> >   drivers/power/supply/yoga-c630-ec.c           | 547 ++++++++++++++++++
> >   4 files changed, 647 insertions(+)
> >   create mode 100644 Documentation/devicetree/bindings/power/supply/lenovo,yoga-c630-ec.yaml
> >   create mode 100644 drivers/power/supply/yoga-c630-ec.c
> > 
> It will be so nice to drop the "some-battery" patches that I've been
> carrying in my kernel sources since 5.7 :D
> 

Not only that, this is the only patch I've been carrying ont op of
Torvalds' tree for a couple of releases now. As such, if this lands, I
expect to be able to run an unmodified v6.1 straight off on my laptop.

Regards,
Bjorn

> Tested-by: Steev Klimaszewski <steev@kali.org>
> 

Re: [PATCH 1/2] dt-bindings: power: supply: Add Lenovo Yoga C630 EC

[Bjorn Andersson]

On Wed 10 Aug 06:35 PDT 2022, Krzysztof Kozlowski wrote:

> On 10/08/2022 06:04, Bjorn Andersson wrote:
> > Add binding for the Embedded Controller found in the Qualcomm
> > Snapdragon-based Lenovo Yoga C630.
> 
> Thank you for your patch. There is something to discuss/improve.
> 
> > +
> > +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
> 
> Just to clarify: the EC have physically two USB connectors?
> 

Correct, while it's only possible to do Displayport on the second
connector, the EC is involved in both the connectors - based on the
events received from it.

> > +
> > +  '#size-cells':
> > +    const: 0
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +patternProperties:
> > +  '^connector@\d$':
> > +    $ref: /schemas/connector/usb-connector.yaml#
> 
> unevaluatedProperties:false inside connector (on its level)
> 

Okay, will update accordingly.

> > +
> > +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";
> > +
> > +          ports {
> > +            #address-cells = <1>;
> > +            #size-cells = <0>;
> > +            port@1 {
> > +              reg = <1>;
> > +              lenovo_ec_dp_in: endpoint {
> > +                   remote-endpoint = <&mdss_dp_out>;
> 
> You have inconsistent indentation. Use 4-spaces for entire DTS example.
> 

Odd, will fix.

Thanks,
Bjorn

> Best regards,
> Krzysztof

Re: [PATCH 2/2] power: supply: Add Lenovo Yoga C630 EC driver

[kernel test robot]

Hi Bjorn,

I love your patch! Yet something to improve:

[auto build test ERROR on sre-power-supply/for-next]
[also build test ERROR on robh/for-next krzk-dt/for-next linus/master v5.19 next-20220812]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Bjorn-Andersson/power-supply-Lenovo-Yoga-C630-EC/20220810-110737
base:   https://git.kernel.org/pub/scm/linux/kernel/git/sre/linux-power-supply.git for-next
config: powerpc-allyesconfig (https://download.01.org/0day-ci/archive/20220813/202208131821.HWmmofHI-lkp@intel.com/config)
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 5f1c7e2cc5a3c07cbc2412e851a7283c1841f520)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install powerpc cross compiling tool for clang build
        # apt-get install binutils-powerpc-linux-gnu
        # https://github.com/intel-lab-lkp/linux/commit/d194cf7394bbf827844847bbde97efa1283ccb08
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Bjorn-Andersson/power-supply-Lenovo-Yoga-C630-EC/20220810-110737
        git checkout d194cf7394bbf827844847bbde97efa1283ccb08
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=powerpc SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/power/supply/yoga-c630-ec.c:225:24: error: call to undeclared function 'FIELD_GET'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
                   ec->adapter_online = FIELD_GET(LENOVO_EC_ADPT_PRESENT, val);
                                        ^
   drivers/power/supply/yoga-c630-ec.c:391:6: error: call to undeclared function 'FIELD_GET'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
           if (FIELD_GET(LENOVO_EC_USB_EVENT_MUX, event))
               ^
>> drivers/power/supply/yoga-c630-ec.c:493:9: error: call to undeclared function 'devm_drm_bridge_add'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
                   ret = devm_drm_bridge_add(dev, &ec->bridge);
                         ^
   drivers/power/supply/yoga-c630-ec.c:493:9: note: did you mean 'drm_bridge_add'?
   include/drm/drm_bridge.h:798:6: note: 'drm_bridge_add' declared here
   void drm_bridge_add(struct drm_bridge *bridge);
        ^
   3 errors generated.


vim +/FIELD_GET +225 drivers/power/supply/yoga-c630-ec.c

   216	
   217	static int yoga_c630_ec_update_adapter_status(struct yoga_c630_ec *ec)
   218	{
   219		int val;
   220	
   221		mutex_lock(&ec->lock);
   222	
   223		val = yoga_c630_ec_read8(ec, LENOVO_EC_ADPT_STATUS);
   224		if (val > 0)
 > 225			ec->adapter_online = FIELD_GET(LENOVO_EC_ADPT_PRESENT, val);
   226	
   227		mutex_unlock(&ec->lock);
   228	
   229		return val;
   230	}
   231	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH 2/2] power: supply: Add Lenovo Yoga C630 EC driver

[Daniel Thompson]

On Tue, Aug 09, 2022 at 10:05:00PM -0500, Bjorn Andersson wrote:
> The Qualcomm Snapdragon-based Lenovo Yoga C630 has some sort of EC
> providing AC-adapter and battery status, as well as USB Type-C altmode
> notifications for Displayport operation.

There's a couple of minor review comments but before we get to that:
woo hoo!


> The Yoga C630 ships with Windows, where these operations primarily are
> implemented in ACPI, but due to various issues with the hardware
> representation therein it's not possible to run Linux on this
> information. As such this is a best-effort re-implementation of these
> operations, based on the register map expressed in ACPI and a fair
> amount of trial and error.
>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> Reported-by: kernel test robot <lkp@intel.com>
> ---
>  drivers/power/supply/Kconfig        |  11 +
>  drivers/power/supply/Makefile       |   1 +
>  drivers/power/supply/yoga-c630-ec.c | 547 ++++++++++++++++++++++++++++
>  3 files changed, 559 insertions(+)
>  create mode 100644 drivers/power/supply/yoga-c630-ec.c
>
> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
> index 1aa8323ad9f6..6e706e948ad2 100644
> --- a/drivers/power/supply/Kconfig
> +++ b/drivers/power/supply/Kconfig
> @@ -897,4 +897,15 @@ config BATTERY_UG3105
>  	  device is off or suspended, the functionality of this driver is
>  	  limited to reporting capacity only.
>
> +config LENOVO_YOGA_C630_EC
> +	tristate "Lenovo Yoga C630 EC battery driver"
> +	depends on DRM
> +	depends on I2C

This needs a "depends on TYPEC" in order to avoid linker errors.


> +	help
> +	  Driver for the Embedded Controller in the Qualcomm Snapdragon-based
> +	  Lenovo Yoga C630, which provides battery information and USB Type-C
> +	  altmode notifications.
> +
> +	  Say M or Y here to include this support.
> +
>  endif # POWER_SUPPLY
> diff --git a/drivers/power/supply/yoga-c630-ec.c b/drivers/power/supply/yoga-c630-ec.c
> new file mode 100644
> index 000000000000..1fa0b5844e01
> --- /dev/null
> +++ b/drivers/power/supply/yoga-c630-ec.c
> @@ -0,0 +1,547 @@
> <snip>
> +static int yoga_c630_ec_bat_get_property(struct power_supply *psy,
> +					 enum power_supply_property psp,
> +					 union power_supply_propval *val)
> +{
> +	struct yoga_c630_ec *ec = power_supply_get_drvdata(psy);
> +	int rc = 0;
> +
> +	if (ec->bat_present)
> +		yoga_c630_ec_maybe_update_bat_status(ec);
> +	else if (psp != POWER_SUPPLY_PROP_PRESENT)
> +		return -ENODEV;
> +
> +	switch (psp) {
>       <snip>
> +	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;
> +	case POWER_SUPPLY_PROP_SERIAL_NUMBER:
> +		val->strval = "05072018";
> +		break;

I'm a little sceptical that hardcoding a serial number into the
driver provides anybody any benefit (regardless of whether the
AML code does this). AFAICT this is not a commonly implemented property
in other power supplies so I'm not clear why this is needed.


Daniel.

Re: [PATCH 2/2] power: supply: Add Lenovo Yoga C630 EC driver

[Daniel Thompson]

On Mon, Sep 12, 2022 at 05:00:17PM +0000, Daniel Thompson wrote:
> On Tue, Aug 09, 2022 at 10:05:00PM -0500, Bjorn Andersson wrote:
> > The Qualcomm Snapdragon-based Lenovo Yoga C630 has some sort of EC
> > providing AC-adapter and battery status, as well as USB Type-C altmode
> > notifications for Displayport operation.
>
> There's a couple of minor review comments but before we get to that:
> woo hoo!

... and now with the correct address for Bjorn too (comments still below
and indented > one level).


Daniel.


> > The Yoga C630 ships with Windows, where these operations primarily are
> > implemented in ACPI, but due to various issues with the hardware
> > representation therein it's not possible to run Linux on this
> > information. As such this is a best-effort re-implementation of these
> > operations, based on the register map expressed in ACPI and a fair
> > amount of trial and error.
> >
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > Reported-by: kernel test robot <lkp@intel.com>
> > ---
> >  drivers/power/supply/Kconfig        |  11 +
> >  drivers/power/supply/Makefile       |   1 +
> >  drivers/power/supply/yoga-c630-ec.c | 547 ++++++++++++++++++++++++++++
> >  3 files changed, 559 insertions(+)
> >  create mode 100644 drivers/power/supply/yoga-c630-ec.c
> >
> > diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
> > index 1aa8323ad9f6..6e706e948ad2 100644
> > --- a/drivers/power/supply/Kconfig
> > +++ b/drivers/power/supply/Kconfig
> > @@ -897,4 +897,15 @@ config BATTERY_UG3105
> >  	  device is off or suspended, the functionality of this driver is
> >  	  limited to reporting capacity only.
> >
> > +config LENOVO_YOGA_C630_EC
> > +	tristate "Lenovo Yoga C630 EC battery driver"
> > +	depends on DRM
> > +	depends on I2C
>
> This needs a "depends on TYPEC" in order to avoid linker errors.
>
>
> > +	help
> > +	  Driver for the Embedded Controller in the Qualcomm Snapdragon-based
> > +	  Lenovo Yoga C630, which provides battery information and USB Type-C
> > +	  altmode notifications.
> > +
> > +	  Say M or Y here to include this support.
> > +
> >  endif # POWER_SUPPLY
> > diff --git a/drivers/power/supply/yoga-c630-ec.c b/drivers/power/supply/yoga-c630-ec.c
> > new file mode 100644
> > index 000000000000..1fa0b5844e01
> > --- /dev/null
> > +++ b/drivers/power/supply/yoga-c630-ec.c
> > @@ -0,0 +1,547 @@
> > <snip>
> > +static int yoga_c630_ec_bat_get_property(struct power_supply *psy,
> > +					 enum power_supply_property psp,
> > +					 union power_supply_propval *val)
> > +{
> > +	struct yoga_c630_ec *ec = power_supply_get_drvdata(psy);
> > +	int rc = 0;
> > +
> > +	if (ec->bat_present)
> > +		yoga_c630_ec_maybe_update_bat_status(ec);
> > +	else if (psp != POWER_SUPPLY_PROP_PRESENT)
> > +		return -ENODEV;
> > +
> > +	switch (psp) {
> >       <snip>
> > +	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;
> > +	case POWER_SUPPLY_PROP_SERIAL_NUMBER:
> > +		val->strval = "05072018";
> > +		break;
>
> I'm a little sceptical that hardcoding a serial number into the
> driver provides anybody any benefit (regardless of whether the
> AML code does this). AFAICT this is not a commonly implemented property
> in other power supplies so I'm not clear why this is needed.
>
>
> Daniel.

Re: [PATCH 2/2] power: supply: Add Lenovo Yoga C630 EC driver

[Sebastian Reichel]

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

Hi,

[+Cc Lee Jones, DRI devel]

On Tue, Aug 09, 2022 at 10:05:00PM -0500, Bjorn Andersson wrote:
> The Qualcomm Snapdragon-based Lenovo Yoga C630 has some sort of EC
> providing AC-adapter and battery status, as well as USB Type-C altmode
> notifications for Displayport operation.
> 
> The Yoga C630 ships with Windows, where these operations primarily are
> implemented in ACPI, but due to various issues with the hardware
> representation therein it's not possible to run Linux on this
> information. As such this is a best-effort re-implementation of these
> operations, based on the register map expressed in ACPI and a fair
> amount of trial and error.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
> [...]
> +	val = yoga_c630_ec_read8(ec, LENOVO_EC_BAT_ATTRIBUTES);
> +	if (val < 0)
> +		goto out_unlock;
> +	ec->unit_ma = val & LENOVO_EC_BAT_ATTR_UNIT_IS_MA;
> +	if (!ec->unit_ma)
> +		ec->scale = 1000;
> +	else
> +		ec->scale = 1;

Since I'm not sure how much of information was gained by reverse
engineering: Is this really milliamps vs microamps and not milliamps
vs milliwatt? SBS batteries usually report either mA or mW.

> [...]
> +	case POWER_SUPPLY_PROP_SERIAL_NUMBER:
> +		val->strval = "05072018";
> +		break;

why is this hardcoded? :)

> [...]
> +	device_for_each_child_node(dev, fwnode) {
> +		ret = fwnode_property_read_u32(fwnode, "reg", &port);
> +		if (ret < 0)
> +			continue;
> +
> +		/* Got multiple ports, but altmode is only possible on port 1 */
> +		if (port != 1)
> +			continue;
> +
> +		ec->bridge.funcs = &yoga_c630_ec_bridge_funcs;
> +		ec->bridge.of_node = to_of_node(fwnode);
> +		ec->bridge.ops = DRM_BRIDGE_OP_HPD;
> +		ec->bridge.type = DRM_MODE_CONNECTOR_USB;
> +
> +		ret = devm_drm_bridge_add(dev, &ec->bridge);
> +		if (ret) {
> +			dev_err(dev, "failed to register drm bridge\n");
> +			fwnode_handle_put(fwnode);
> +			return ret;
> +		}

I wonder if DRM people want to see this in drivers/gpu/drm/bridge.
Maybe it's better to make this a MFD driver?

> [...]

-- Sebastian

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

Re: [PATCH 2/2] power: supply: Add Lenovo Yoga C630 EC driver

[Sebastian Reichel]

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

Hi,

[+Cc Lee Jones, DRI devel]

On Tue, Aug 09, 2022 at 10:05:00PM -0500, Bjorn Andersson wrote:
> The Qualcomm Snapdragon-based Lenovo Yoga C630 has some sort of EC
> providing AC-adapter and battery status, as well as USB Type-C altmode
> notifications for Displayport operation.
> 
> The Yoga C630 ships with Windows, where these operations primarily are
> implemented in ACPI, but due to various issues with the hardware
> representation therein it's not possible to run Linux on this
> information. As such this is a best-effort re-implementation of these
> operations, based on the register map expressed in ACPI and a fair
> amount of trial and error.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
> [...]
> +	val = yoga_c630_ec_read8(ec, LENOVO_EC_BAT_ATTRIBUTES);
> +	if (val < 0)
> +		goto out_unlock;
> +	ec->unit_ma = val & LENOVO_EC_BAT_ATTR_UNIT_IS_MA;
> +	if (!ec->unit_ma)
> +		ec->scale = 1000;
> +	else
> +		ec->scale = 1;

Since I'm not sure how much of information was gained by reverse
engineering: Is this really milliamps vs microamps and not milliamps
vs milliwatt? SBS batteries usually report either mA or mW.

> [...]
> +	case POWER_SUPPLY_PROP_SERIAL_NUMBER:
> +		val->strval = "05072018";
> +		break;

why is this hardcoded? :)

> [...]
> +	device_for_each_child_node(dev, fwnode) {
> +		ret = fwnode_property_read_u32(fwnode, "reg", &port);
> +		if (ret < 0)
> +			continue;
> +
> +		/* Got multiple ports, but altmode is only possible on port 1 */
> +		if (port != 1)
> +			continue;
> +
> +		ec->bridge.funcs = &yoga_c630_ec_bridge_funcs;
> +		ec->bridge.of_node = to_of_node(fwnode);
> +		ec->bridge.ops = DRM_BRIDGE_OP_HPD;
> +		ec->bridge.type = DRM_MODE_CONNECTOR_USB;
> +
> +		ret = devm_drm_bridge_add(dev, &ec->bridge);
> +		if (ret) {
> +			dev_err(dev, "failed to register drm bridge\n");
> +			fwnode_handle_put(fwnode);
> +			return ret;
> +		}

I wonder if DRM people want to see this in drivers/gpu/drm/bridge.
Maybe it's better to make this a MFD driver?

> [...]

-- Sebastian

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

Re: [PATCH 2/2] power: supply: Add Lenovo Yoga C630 EC driver

[Bjorn Andersson]

On Tue, Sep 13, 2022 at 12:45:45PM +0200, Sebastian Reichel wrote:
> Hi,
> 
> [+Cc Lee Jones, DRI devel]
> 
> On Tue, Aug 09, 2022 at 10:05:00PM -0500, Bjorn Andersson wrote:
> > The Qualcomm Snapdragon-based Lenovo Yoga C630 has some sort of EC
> > providing AC-adapter and battery status, as well as USB Type-C altmode
> > notifications for Displayport operation.
> > 
> > The Yoga C630 ships with Windows, where these operations primarily are
> > implemented in ACPI, but due to various issues with the hardware
> > representation therein it's not possible to run Linux on this
> > information. As such this is a best-effort re-implementation of these
> > operations, based on the register map expressed in ACPI and a fair
> > amount of trial and error.
> > 
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > ---
> > [...]
> > +	val = yoga_c630_ec_read8(ec, LENOVO_EC_BAT_ATTRIBUTES);
> > +	if (val < 0)
> > +		goto out_unlock;
> > +	ec->unit_ma = val & LENOVO_EC_BAT_ATTR_UNIT_IS_MA;
> > +	if (!ec->unit_ma)
> > +		ec->scale = 1000;
> > +	else
> > +		ec->scale = 1;
> 
> Since I'm not sure how much of information was gained by reverse
> engineering: Is this really milliamps vs microamps and not milliamps
> vs milliwatt? SBS batteries usually report either mA or mW.
> 

Unfortunately I don't know the answer to this.

> > [...]
> > +	case POWER_SUPPLY_PROP_SERIAL_NUMBER:
> > +		val->strval = "05072018";
> > +		break;
> 
> why is this hardcoded? :)
> 

I don't know, but as Daniel suggests, it would make sense to just drop
it.

> > [...]
> > +	device_for_each_child_node(dev, fwnode) {
> > +		ret = fwnode_property_read_u32(fwnode, "reg", &port);
> > +		if (ret < 0)
> > +			continue;
> > +
> > +		/* Got multiple ports, but altmode is only possible on port 1 */
> > +		if (port != 1)
> > +			continue;
> > +
> > +		ec->bridge.funcs = &yoga_c630_ec_bridge_funcs;
> > +		ec->bridge.of_node = to_of_node(fwnode);
> > +		ec->bridge.ops = DRM_BRIDGE_OP_HPD;
> > +		ec->bridge.type = DRM_MODE_CONNECTOR_USB;
> > +
> > +		ret = devm_drm_bridge_add(dev, &ec->bridge);
> > +		if (ret) {
> > +			dev_err(dev, "failed to register drm bridge\n");
> > +			fwnode_handle_put(fwnode);
> > +			return ret;
> > +		}
> 
> I wonder if DRM people want to see this in drivers/gpu/drm/bridge.
> Maybe it's better to make this a MFD driver?
> 

I did consider it, but it adds a whole bunch of boiler plate code
without a lot of benefit.

There are a few other cases where I think it would make sense to have
drm bridges outside of drivers/gpu/drm, such as
drivers/usb/typec/altmodes/ and drivers/platform/chrome/...

> > [...]
> 
> -- Sebastian

Thanks,
Bjorn

Re: [PATCH 2/2] power: supply: Add Lenovo Yoga C630 EC driver

[Bjorn Andersson]

On Tue, Sep 13, 2022 at 12:45:45PM +0200, Sebastian Reichel wrote:
> Hi,
> 
> [+Cc Lee Jones, DRI devel]
> 
> On Tue, Aug 09, 2022 at 10:05:00PM -0500, Bjorn Andersson wrote:
> > The Qualcomm Snapdragon-based Lenovo Yoga C630 has some sort of EC
> > providing AC-adapter and battery status, as well as USB Type-C altmode
> > notifications for Displayport operation.
> > 
> > The Yoga C630 ships with Windows, where these operations primarily are
> > implemented in ACPI, but due to various issues with the hardware
> > representation therein it's not possible to run Linux on this
> > information. As such this is a best-effort re-implementation of these
> > operations, based on the register map expressed in ACPI and a fair
> > amount of trial and error.
> > 
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > ---
> > [...]
> > +	val = yoga_c630_ec_read8(ec, LENOVO_EC_BAT_ATTRIBUTES);
> > +	if (val < 0)
> > +		goto out_unlock;
> > +	ec->unit_ma = val & LENOVO_EC_BAT_ATTR_UNIT_IS_MA;
> > +	if (!ec->unit_ma)
> > +		ec->scale = 1000;
> > +	else
> > +		ec->scale = 1;
> 
> Since I'm not sure how much of information was gained by reverse
> engineering: Is this really milliamps vs microamps and not milliamps
> vs milliwatt? SBS batteries usually report either mA or mW.
> 

Unfortunately I don't know the answer to this.

> > [...]
> > +	case POWER_SUPPLY_PROP_SERIAL_NUMBER:
> > +		val->strval = "05072018";
> > +		break;
> 
> why is this hardcoded? :)
> 

I don't know, but as Daniel suggests, it would make sense to just drop
it.

> > [...]
> > +	device_for_each_child_node(dev, fwnode) {
> > +		ret = fwnode_property_read_u32(fwnode, "reg", &port);
> > +		if (ret < 0)
> > +			continue;
> > +
> > +		/* Got multiple ports, but altmode is only possible on port 1 */
> > +		if (port != 1)
> > +			continue;
> > +
> > +		ec->bridge.funcs = &yoga_c630_ec_bridge_funcs;
> > +		ec->bridge.of_node = to_of_node(fwnode);
> > +		ec->bridge.ops = DRM_BRIDGE_OP_HPD;
> > +		ec->bridge.type = DRM_MODE_CONNECTOR_USB;
> > +
> > +		ret = devm_drm_bridge_add(dev, &ec->bridge);
> > +		if (ret) {
> > +			dev_err(dev, "failed to register drm bridge\n");
> > +			fwnode_handle_put(fwnode);
> > +			return ret;
> > +		}
> 
> I wonder if DRM people want to see this in drivers/gpu/drm/bridge.
> Maybe it's better to make this a MFD driver?
> 

I did consider it, but it adds a whole bunch of boiler plate code
without a lot of benefit.

There are a few other cases where I think it would make sense to have
drm bridges outside of drivers/gpu/drm, such as
drivers/usb/typec/altmodes/ and drivers/platform/chrome/...

> > [...]
> 
> -- Sebastian

Thanks,
Bjorn

Re: [PATCH 2/2] power: supply: Add Lenovo Yoga C630 EC driver

[Dmitry Baryshkov]

On Fri, 16 Sept 2022 at 00:25, Bjorn Andersson <andersson@kernel.org> wrote:
>
> On Tue, Sep 13, 2022 at 12:45:45PM +0200, Sebastian Reichel wrote:
> > Hi,
> >
> > [+Cc Lee Jones, DRI devel]
> >
> > On Tue, Aug 09, 2022 at 10:05:00PM -0500, Bjorn Andersson wrote:
> > > The Qualcomm Snapdragon-based Lenovo Yoga C630 has some sort of EC
> > > providing AC-adapter and battery status, as well as USB Type-C altmode
> > > notifications for Displayport operation.
> > >
> > > The Yoga C630 ships with Windows, where these operations primarily are
> > > implemented in ACPI, but due to various issues with the hardware
> > > representation therein it's not possible to run Linux on this
> > > information. As such this is a best-effort re-implementation of these
> > > operations, based on the register map expressed in ACPI and a fair
> > > amount of trial and error.
> > >
> > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > > ---
> > > [...]
> > > +   val = yoga_c630_ec_read8(ec, LENOVO_EC_BAT_ATTRIBUTES);
> > > +   if (val < 0)
> > > +           goto out_unlock;
> > > +   ec->unit_ma = val & LENOVO_EC_BAT_ATTR_UNIT_IS_MA;
> > > +   if (!ec->unit_ma)
> > > +           ec->scale = 1000;
> > > +   else
> > > +           ec->scale = 1;
> >
> > Since I'm not sure how much of information was gained by reverse
> > engineering: Is this really milliamps vs microamps and not milliamps
> > vs milliwatt? SBS batteries usually report either mA or mW.
> >
>
> Unfortunately I don't know the answer to this.
>
> > > [...]
> > > +   case POWER_SUPPLY_PROP_SERIAL_NUMBER:
> > > +           val->strval = "05072018";
> > > +           break;
> >
> > why is this hardcoded? :)
> >
>
> I don't know, but as Daniel suggests, it would make sense to just drop
> it.
>
> > > [...]
> > > +   device_for_each_child_node(dev, fwnode) {
> > > +           ret = fwnode_property_read_u32(fwnode, "reg", &port);
> > > +           if (ret < 0)
> > > +                   continue;
> > > +
> > > +           /* Got multiple ports, but altmode is only possible on port 1 */
> > > +           if (port != 1)
> > > +                   continue;
> > > +
> > > +           ec->bridge.funcs = &yoga_c630_ec_bridge_funcs;
> > > +           ec->bridge.of_node = to_of_node(fwnode);
> > > +           ec->bridge.ops = DRM_BRIDGE_OP_HPD;
> > > +           ec->bridge.type = DRM_MODE_CONNECTOR_USB;
> > > +
> > > +           ret = devm_drm_bridge_add(dev, &ec->bridge);
> > > +           if (ret) {
> > > +                   dev_err(dev, "failed to register drm bridge\n");
> > > +                   fwnode_handle_put(fwnode);
> > > +                   return ret;
> > > +           }
> >
> > I wonder if DRM people want to see this in drivers/gpu/drm/bridge.
> > Maybe it's better to make this a MFD driver?
> >
>
> I did consider it, but it adds a whole bunch of boiler plate code
> without a lot of benefit.
>
> There are a few other cases where I think it would make sense to have
> drm bridges outside of drivers/gpu/drm, such as
> drivers/usb/typec/altmodes/ and drivers/platform/chrome/...

What about a solution which might sound simpler than MFD: from your
driver's probe() register a platform device which represents a drm
bridge. Add drm_bridge driver for it into drivers/gpu/drm/bridges/.



-- 
With best wishes
Dmitry

Re: [PATCH 2/2] power: supply: Add Lenovo Yoga C630 EC driver

[Dmitry Baryshkov]

On Fri, 16 Sept 2022 at 00:25, Bjorn Andersson <andersson@kernel.org> wrote:
>
> On Tue, Sep 13, 2022 at 12:45:45PM +0200, Sebastian Reichel wrote:
> > Hi,
> >
> > [+Cc Lee Jones, DRI devel]
> >
> > On Tue, Aug 09, 2022 at 10:05:00PM -0500, Bjorn Andersson wrote:
> > > The Qualcomm Snapdragon-based Lenovo Yoga C630 has some sort of EC
> > > providing AC-adapter and battery status, as well as USB Type-C altmode
> > > notifications for Displayport operation.
> > >
> > > The Yoga C630 ships with Windows, where these operations primarily are
> > > implemented in ACPI, but due to various issues with the hardware
> > > representation therein it's not possible to run Linux on this
> > > information. As such this is a best-effort re-implementation of these
> > > operations, based on the register map expressed in ACPI and a fair
> > > amount of trial and error.
> > >
> > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > > ---
> > > [...]
> > > +   val = yoga_c630_ec_read8(ec, LENOVO_EC_BAT_ATTRIBUTES);
> > > +   if (val < 0)
> > > +           goto out_unlock;
> > > +   ec->unit_ma = val & LENOVO_EC_BAT_ATTR_UNIT_IS_MA;
> > > +   if (!ec->unit_ma)
> > > +           ec->scale = 1000;
> > > +   else
> > > +           ec->scale = 1;
> >
> > Since I'm not sure how much of information was gained by reverse
> > engineering: Is this really milliamps vs microamps and not milliamps
> > vs milliwatt? SBS batteries usually report either mA or mW.
> >
>
> Unfortunately I don't know the answer to this.
>
> > > [...]
> > > +   case POWER_SUPPLY_PROP_SERIAL_NUMBER:
> > > +           val->strval = "05072018";
> > > +           break;
> >
> > why is this hardcoded? :)
> >
>
> I don't know, but as Daniel suggests, it would make sense to just drop
> it.
>
> > > [...]
> > > +   device_for_each_child_node(dev, fwnode) {
> > > +           ret = fwnode_property_read_u32(fwnode, "reg", &port);
> > > +           if (ret < 0)
> > > +                   continue;
> > > +
> > > +           /* Got multiple ports, but altmode is only possible on port 1 */
> > > +           if (port != 1)
> > > +                   continue;
> > > +
> > > +           ec->bridge.funcs = &yoga_c630_ec_bridge_funcs;
> > > +           ec->bridge.of_node = to_of_node(fwnode);
> > > +           ec->bridge.ops = DRM_BRIDGE_OP_HPD;
> > > +           ec->bridge.type = DRM_MODE_CONNECTOR_USB;
> > > +
> > > +           ret = devm_drm_bridge_add(dev, &ec->bridge);
> > > +           if (ret) {
> > > +                   dev_err(dev, "failed to register drm bridge\n");
> > > +                   fwnode_handle_put(fwnode);
> > > +                   return ret;
> > > +           }
> >
> > I wonder if DRM people want to see this in drivers/gpu/drm/bridge.
> > Maybe it's better to make this a MFD driver?
> >
>
> I did consider it, but it adds a whole bunch of boiler plate code
> without a lot of benefit.
>
> There are a few other cases where I think it would make sense to have
> drm bridges outside of drivers/gpu/drm, such as
> drivers/usb/typec/altmodes/ and drivers/platform/chrome/...

What about a solution which might sound simpler than MFD: from your
driver's probe() register a platform device which represents a drm
bridge. Add drm_bridge driver for it into drivers/gpu/drm/bridges/.



-- 
With best wishes
Dmitry

Re: [PATCH 2/2] power: supply: Add Lenovo Yoga C630 EC driver

[Bjorn Andersson]

On Fri, Sep 16, 2022 at 12:53:42AM +0300, Dmitry Baryshkov wrote:
> On Fri, 16 Sept 2022 at 00:25, Bjorn Andersson <andersson@kernel.org> wrote:
> >
> > On Tue, Sep 13, 2022 at 12:45:45PM +0200, Sebastian Reichel wrote:
> > > Hi,
> > >
> > > [+Cc Lee Jones, DRI devel]
> > >
> > > On Tue, Aug 09, 2022 at 10:05:00PM -0500, Bjorn Andersson wrote:
> > > > The Qualcomm Snapdragon-based Lenovo Yoga C630 has some sort of EC
> > > > providing AC-adapter and battery status, as well as USB Type-C altmode
> > > > notifications for Displayport operation.
> > > >
> > > > The Yoga C630 ships with Windows, where these operations primarily are
> > > > implemented in ACPI, but due to various issues with the hardware
> > > > representation therein it's not possible to run Linux on this
> > > > information. As such this is a best-effort re-implementation of these
> > > > operations, based on the register map expressed in ACPI and a fair
> > > > amount of trial and error.
> > > >
> > > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > > > ---
> > > > [...]
> > > > +   val = yoga_c630_ec_read8(ec, LENOVO_EC_BAT_ATTRIBUTES);
> > > > +   if (val < 0)
> > > > +           goto out_unlock;
> > > > +   ec->unit_ma = val & LENOVO_EC_BAT_ATTR_UNIT_IS_MA;
> > > > +   if (!ec->unit_ma)
> > > > +           ec->scale = 1000;
> > > > +   else
> > > > +           ec->scale = 1;
> > >
> > > Since I'm not sure how much of information was gained by reverse
> > > engineering: Is this really milliamps vs microamps and not milliamps
> > > vs milliwatt? SBS batteries usually report either mA or mW.
> > >
> >
> > Unfortunately I don't know the answer to this.
> >
> > > > [...]
> > > > +   case POWER_SUPPLY_PROP_SERIAL_NUMBER:
> > > > +           val->strval = "05072018";
> > > > +           break;
> > >
> > > why is this hardcoded? :)
> > >
> >
> > I don't know, but as Daniel suggests, it would make sense to just drop
> > it.
> >
> > > > [...]
> > > > +   device_for_each_child_node(dev, fwnode) {
> > > > +           ret = fwnode_property_read_u32(fwnode, "reg", &port);
> > > > +           if (ret < 0)
> > > > +                   continue;
> > > > +
> > > > +           /* Got multiple ports, but altmode is only possible on port 1 */
> > > > +           if (port != 1)
> > > > +                   continue;
> > > > +
> > > > +           ec->bridge.funcs = &yoga_c630_ec_bridge_funcs;
> > > > +           ec->bridge.of_node = to_of_node(fwnode);
> > > > +           ec->bridge.ops = DRM_BRIDGE_OP_HPD;
> > > > +           ec->bridge.type = DRM_MODE_CONNECTOR_USB;
> > > > +
> > > > +           ret = devm_drm_bridge_add(dev, &ec->bridge);
> > > > +           if (ret) {
> > > > +                   dev_err(dev, "failed to register drm bridge\n");
> > > > +                   fwnode_handle_put(fwnode);
> > > > +                   return ret;
> > > > +           }
> > >
> > > I wonder if DRM people want to see this in drivers/gpu/drm/bridge.
> > > Maybe it's better to make this a MFD driver?
> > >
> >
> > I did consider it, but it adds a whole bunch of boiler plate code
> > without a lot of benefit.
> >
> > There are a few other cases where I think it would make sense to have
> > drm bridges outside of drivers/gpu/drm, such as
> > drivers/usb/typec/altmodes/ and drivers/platform/chrome/...
> 
> What about a solution which might sound simpler than MFD: from your
> driver's probe() register a platform device which represents a drm
> bridge. Add drm_bridge driver for it into drivers/gpu/drm/bridges/.
> 

That is indeed simpler

But then rather than just calling drm_bridge_hpd_notify(status) we need
a new custom interface for this driver to call the other drive when
status changes and we waste 840 bytes just to track another struct
platform_device.

Perhaps that could be made generic, so we'd have "proxy_drm_bridge"
driver for drivers who implements a HPD drm_bridge but don't want to
implement that outside drivers/gpu/drm.

But this is just based on the assumption that the "DRM people" require
drm_bridges to only live in drivers/gpu/drm/. It's all one kernel and
the API is well defined, so I don't think there's a concrete problem to
solve.

Regards,
Bjorn

Re: [PATCH 2/2] power: supply: Add Lenovo Yoga C630 EC driver

[Bjorn Andersson]

On Fri, Sep 16, 2022 at 12:53:42AM +0300, Dmitry Baryshkov wrote:
> On Fri, 16 Sept 2022 at 00:25, Bjorn Andersson <andersson@kernel.org> wrote:
> >
> > On Tue, Sep 13, 2022 at 12:45:45PM +0200, Sebastian Reichel wrote:
> > > Hi,
> > >
> > > [+Cc Lee Jones, DRI devel]
> > >
> > > On Tue, Aug 09, 2022 at 10:05:00PM -0500, Bjorn Andersson wrote:
> > > > The Qualcomm Snapdragon-based Lenovo Yoga C630 has some sort of EC
> > > > providing AC-adapter and battery status, as well as USB Type-C altmode
> > > > notifications for Displayport operation.
> > > >
> > > > The Yoga C630 ships with Windows, where these operations primarily are
> > > > implemented in ACPI, but due to various issues with the hardware
> > > > representation therein it's not possible to run Linux on this
> > > > information. As such this is a best-effort re-implementation of these
> > > > operations, based on the register map expressed in ACPI and a fair
> > > > amount of trial and error.
> > > >
> > > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > > > ---
> > > > [...]
> > > > +   val = yoga_c630_ec_read8(ec, LENOVO_EC_BAT_ATTRIBUTES);
> > > > +   if (val < 0)
> > > > +           goto out_unlock;
> > > > +   ec->unit_ma = val & LENOVO_EC_BAT_ATTR_UNIT_IS_MA;
> > > > +   if (!ec->unit_ma)
> > > > +           ec->scale = 1000;
> > > > +   else
> > > > +           ec->scale = 1;
> > >
> > > Since I'm not sure how much of information was gained by reverse
> > > engineering: Is this really milliamps vs microamps and not milliamps
> > > vs milliwatt? SBS batteries usually report either mA or mW.
> > >
> >
> > Unfortunately I don't know the answer to this.
> >
> > > > [...]
> > > > +   case POWER_SUPPLY_PROP_SERIAL_NUMBER:
> > > > +           val->strval = "05072018";
> > > > +           break;
> > >
> > > why is this hardcoded? :)
> > >
> >
> > I don't know, but as Daniel suggests, it would make sense to just drop
> > it.
> >
> > > > [...]
> > > > +   device_for_each_child_node(dev, fwnode) {
> > > > +           ret = fwnode_property_read_u32(fwnode, "reg", &port);
> > > > +           if (ret < 0)
> > > > +                   continue;
> > > > +
> > > > +           /* Got multiple ports, but altmode is only possible on port 1 */
> > > > +           if (port != 1)
> > > > +                   continue;
> > > > +
> > > > +           ec->bridge.funcs = &yoga_c630_ec_bridge_funcs;
> > > > +           ec->bridge.of_node = to_of_node(fwnode);
> > > > +           ec->bridge.ops = DRM_BRIDGE_OP_HPD;
> > > > +           ec->bridge.type = DRM_MODE_CONNECTOR_USB;
> > > > +
> > > > +           ret = devm_drm_bridge_add(dev, &ec->bridge);
> > > > +           if (ret) {
> > > > +                   dev_err(dev, "failed to register drm bridge\n");
> > > > +                   fwnode_handle_put(fwnode);
> > > > +                   return ret;
> > > > +           }
> > >
> > > I wonder if DRM people want to see this in drivers/gpu/drm/bridge.
> > > Maybe it's better to make this a MFD driver?
> > >
> >
> > I did consider it, but it adds a whole bunch of boiler plate code
> > without a lot of benefit.
> >
> > There are a few other cases where I think it would make sense to have
> > drm bridges outside of drivers/gpu/drm, such as
> > drivers/usb/typec/altmodes/ and drivers/platform/chrome/...
> 
> What about a solution which might sound simpler than MFD: from your
> driver's probe() register a platform device which represents a drm
> bridge. Add drm_bridge driver for it into drivers/gpu/drm/bridges/.
> 

That is indeed simpler

But then rather than just calling drm_bridge_hpd_notify(status) we need
a new custom interface for this driver to call the other drive when
status changes and we waste 840 bytes just to track another struct
platform_device.

Perhaps that could be made generic, so we'd have "proxy_drm_bridge"
driver for drivers who implements a HPD drm_bridge but don't want to
implement that outside drivers/gpu/drm.

But this is just based on the assumption that the "DRM people" require
drm_bridges to only live in drivers/gpu/drm/. It's all one kernel and
the API is well defined, so I don't think there's a concrete problem to
solve.

Regards,
Bjorn