[PATCH 0/3] Introduce new driver cros-ec-charge-state

[PATCH 0/3] Introduce new driver cros-ec-charge-state

[Sung-Chi, Li]

There is a battery charger IC connect to the ChromeOS Embedded
Controller (EC) if that device is configured to work with batteries, and
EC exposed the application processor with a set of control to limit the
overall current used for charging and current flow into the system.

We have seen that the battery charger is one of major thermal budget
consumer on the device. As such, it would be great that we can limit the
current running through the battery charger IC, and reserve thermal
budget for application processor to have more room running in higher
frequencies.

There are some existing drivers that can limit the current flow into the
system, but they either require certain features (e.g., needing ACPI
supports for cros_charge-control.c, which is only available on x86
system), and these current limiting mechanisms are achieved via indirect
configurations (e.g., cros_usbpd-charger.c via setting certain USB PD
charging profiles). As such, I introduced a new driver that can directly
manipulate the battery charging current and the system input current.

Signed-off-by: Sung-Chi, Li
---
Sung-Chi, Li (3):
      platform/chrome: cros_ec_charge_state: add new driver to control charge
      dt-bindings: chrome: add new binding google,cros-ec-chrage-state
      mfd: cros_ec: Add charge state control cell

 .../bindings/chrome/google,cros-charge-state.yaml  |  62 ++++++
 .../devicetree/bindings/mfd/google,cros-ec.yaml    |   4 +
 drivers/mfd/cros_ec_dev.c                          |   9 +
 drivers/platform/chrome/Kconfig                    |  11 ++
 drivers/platform/chrome/Makefile                   |   1 +
 drivers/platform/chrome/cros_ec_charge_state.c     | 215 +++++++++++++++++++++
 6 files changed, 302 insertions(+)
---
base-commit: 744cf71b8bdfcdd77aaf58395e068b7457634b2c
change-id: 20241118-add_charger_state-8c0d6e9a5e45

Best regards,
-- 
Sung-Chi, Li <lschyi@chromium.org>

[PATCH 1/3] platform/chrome: cros_ec_charge_state: add new driver to control charge

[Sung-Chi, Li]

Implement the new platform driver cros_ec_charge_state to have low finer
control over the charge current flow through the charge chip connected
on ChromeOS Embedded Controller (EC).

The driver reads configured charge chip configurations from the device
tree, and register these chip controls as thermal zone devices, so they
are controllable from the thermal subsystem.

As such, corresponding DTS changes are needed, and here is a sample DTS
configuration:

```
&cros_ec {
	charge-chip-battery {
		compatible = "google,cros-ec-charge-state";
		type = "charge";
		min-milliamp = <150>;
		max-milliamp = <5000>;
	};
};
```

Signed-off-by: Sung-Chi, Li <lschyi@chromium.org>
---
 drivers/platform/chrome/Kconfig                |  11 ++
 drivers/platform/chrome/Makefile               |   1 +
 drivers/platform/chrome/cros_ec_charge_state.c | 215 +++++++++++++++++++++++++
 3 files changed, 227 insertions(+)

diff --git a/drivers/platform/chrome/Kconfig b/drivers/platform/chrome/Kconfig
index 7dbeb786352a..34d00d8823cb 100644
--- a/drivers/platform/chrome/Kconfig
+++ b/drivers/platform/chrome/Kconfig
@@ -297,6 +297,17 @@ config CROS_TYPEC_SWITCH
 	  To compile this driver as a module, choose M here: the module will be
 	  called cros_typec_switch.
 
+config CROS_CHARGE_STATE
+	tristate "ChromeOS EC Charger Chip  Control"
+	depends on MFD_CROS_EC_DEV
+	default MFD_CROS_EC_DEV
+	help
+	  If you say Y here, you get support for configuring the battery
+	  charging and system input current.
+
+	  To compile this driver as a module, choose M here: the module will be
+	  called cros-ec-charge-state.
+
 source "drivers/platform/chrome/wilco_ec/Kconfig"
 
 # Kunit test cases
diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile
index 2dcc6ccc2302..01c7154ae119 100644
--- a/drivers/platform/chrome/Makefile
+++ b/drivers/platform/chrome/Makefile
@@ -32,6 +32,7 @@ obj-$(CONFIG_CROS_EC_SYSFS)		+= cros_ec_sysfs.o
 obj-$(CONFIG_CROS_HPS_I2C)		+= cros_hps_i2c.o
 obj-$(CONFIG_CROS_USBPD_LOGGER)		+= cros_usbpd_logger.o
 obj-$(CONFIG_CROS_USBPD_NOTIFY)		+= cros_usbpd_notify.o
+obj-$(CONFIG_CROS_CHARGE_STATE)		+= cros_ec_charge_state.o
 
 obj-$(CONFIG_WILCO_EC)			+= wilco_ec/
 
diff --git a/drivers/platform/chrome/cros_ec_charge_state.c b/drivers/platform/chrome/cros_ec_charge_state.c
new file mode 100644
index 000000000000..3fed5b48bc92
--- /dev/null
+++ b/drivers/platform/chrome/cros_ec_charge_state.c
@@ -0,0 +1,215 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Charge state driver for ChromeOS Embedded Controller
+ *
+ * Copyright 2024 Google LLC
+ *
+ * This driver exports the low level control over charge chip connected to EC
+ * which allows to manipulate the current used to charge the battery, and also
+ * manipulate the current input to the whole system.
+ * This driver also registers that charge chip as a thermal cooling device.
+ */
+
+#include <linux/of.h>
+#include <linux/platform_data/cros_ec_commands.h>
+#include <linux/platform_data/cros_ec_proto.h>
+#include <linux/platform_device.h>
+#include <linux/thermal.h>
+
+#define DRV_NAME "cros-ec-charge-state"
+#define CHARGE_TYPE_CHARGE "charge"
+#define CHARGE_TYPE_INPUT "input"
+
+struct cros_ec_charge_state_data {
+	struct cros_ec_device *ec_dev;
+	struct device *dev;
+	enum charge_state_params charge_type;
+	uint32_t min_milliamp;
+	uint32_t max_milliamp;
+};
+
+static int
+cros_ec_charge_state_get_current_limit(struct cros_ec_device *ec_dev,
+				       enum charge_state_params charge_type,
+				       uint32_t *limit)
+{
+	struct ec_params_charge_state param;
+	struct ec_response_charge_state state;
+	int ret;
+
+	param.cmd = CHARGE_STATE_CMD_GET_PARAM;
+	param.get_param.param = charge_type;
+	ret = cros_ec_cmd(ec_dev, 0, EC_CMD_CHARGE_STATE, &param, sizeof(param),
+			  &state, sizeof(state));
+	if (ret < 0)
+		return ret;
+
+	*limit = cpu_to_le32(state.get_param.value);
+	return 0;
+}
+
+static int
+cros_ec_charge_state_set_current_limit(struct cros_ec_device *ec_dev,
+				       enum charge_state_params charge_type,
+				       uint32_t limit)
+{
+	struct ec_params_charge_state param;
+	int ret;
+
+	param.cmd = CHARGE_STATE_CMD_SET_PARAM;
+	param.set_param.param = charge_type;
+	param.set_param.value = cpu_to_le32(limit);
+	ret = cros_ec_cmd(ec_dev, 0, EC_CMD_CHARGE_STATE, &param, sizeof(param),
+			  NULL, 0);
+	return (ret < 0) ? ret : 0;
+}
+
+static int
+cros_ec_charge_state_get_max_state(struct thermal_cooling_device *cdev,
+				   unsigned long *state)
+{
+	struct cros_ec_charge_state_data *data = cdev->devdata;
+	*state = data->max_milliamp;
+	return 0;
+}
+
+static int
+cros_ec_charge_state_get_cur_state(struct thermal_cooling_device *cdev,
+				   unsigned long *state)
+{
+	struct cros_ec_charge_state_data *data = cdev->devdata;
+	uint32_t limit;
+	int ret;
+
+	ret = cros_ec_charge_state_get_current_limit(data->ec_dev,
+						     data->charge_type, &limit);
+	if (ret) {
+		dev_err(data->dev, "failed to get current state: %d", ret);
+		return ret;
+	}
+
+	*state = data->max_milliamp - limit;
+	return 0;
+}
+
+static int
+cros_ec_charge_state_set_cur_state(struct thermal_cooling_device *cdev,
+				   unsigned long state)
+{
+	struct cros_ec_charge_state_data *data = cdev->devdata;
+	uint32_t limit = data->max_milliamp - state;
+
+	if (limit < data->min_milliamp) {
+		dev_warn(
+			data->dev,
+			"failed to set current %u lower than minimum %d; set to minimum",
+			limit, data->min_milliamp);
+		limit = data->min_milliamp;
+	}
+
+	state = data->max_milliamp - limit;
+	return cros_ec_charge_state_set_current_limit(
+		data->ec_dev, data->charge_type, (uint32_t)state);
+}
+
+static const struct thermal_cooling_device_ops
+	cros_ec_charge_state_cooling_device_ops = {
+		.get_max_state = cros_ec_charge_state_get_max_state,
+		.get_cur_state = cros_ec_charge_state_get_cur_state,
+		.set_cur_state = cros_ec_charge_state_set_cur_state,
+	};
+
+static int
+cros_ec_charge_state_register_charge_chip(struct device *dev,
+					  struct device_node *node,
+					  struct cros_ec_device *cros_ec)
+{
+	struct cros_ec_charge_state_data *data;
+	struct thermal_cooling_device *cdev;
+	const char *type_val = NULL;
+	int ret;
+
+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	ret = of_property_read_string(node, "type", &type_val);
+	if (ret) {
+		dev_err(dev, "failed to get charge type: %d", ret);
+		return ret;
+	}
+
+	if (!strcmp(type_val, CHARGE_TYPE_CHARGE)) {
+		data->charge_type = CS_PARAM_CHG_CURRENT;
+	} else if (!strcmp(type_val, CHARGE_TYPE_INPUT)) {
+		data->charge_type = CS_PARAM_CHG_INPUT_CURRENT;
+	} else {
+		dev_err(dev, "unknown charge type: %s", type_val);
+		return -1;
+	}
+
+	ret = of_property_read_u32(node, "min-milliamp", &data->min_milliamp);
+	if (ret) {
+		dev_err(dev, "failed to get min-milliamp data: %d", ret);
+		return ret;
+	}
+
+	ret = of_property_read_u32(node, "max-milliamp", &data->max_milliamp);
+	if (ret) {
+		dev_err(dev, "failed to get max-milliamp data: %d", ret);
+		return ret;
+	}
+
+	data->ec_dev = cros_ec;
+	data->dev = dev;
+
+	cdev = devm_thermal_of_cooling_device_register(
+		dev, node, node->name, data,
+		&cros_ec_charge_state_cooling_device_ops);
+	if (IS_ERR_VALUE(cdev)) {
+		dev_err(dev,
+			"failed to register charge chip %s as cooling device: %pe",
+			node->name, cdev);
+		return PTR_ERR(cdev);
+	}
+
+	return 0;
+}
+
+static int cros_ec_charge_state_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct cros_ec_dev *ec_dev = dev_get_drvdata(dev->parent);
+	struct cros_ec_device *cros_ec = ec_dev->ec_dev;
+	struct device_node *child;
+
+	for_each_available_child_of_node(cros_ec->dev->of_node, child) {
+		if (!of_device_is_compatible(child,
+					     "google,cros-ec-charge-state"))
+			continue;
+		if (cros_ec_charge_state_register_charge_chip(dev, child,
+							      cros_ec))
+			continue;
+	}
+
+	return 0;
+}
+
+static const struct platform_device_id cros_ec_charge_state_id[] = {
+	{ DRV_NAME,  0 },
+	{}
+};
+
+static struct platform_driver cros_ec_chargedev_driver = {
+	.driver = {
+		.name = DRV_NAME,
+	},
+	.probe = cros_ec_charge_state_probe,
+};
+
+module_platform_driver(cros_ec_chargedev_driver);
+
+MODULE_DEVICE_TABLE(platform, cros_ec_charge_state_id);
+MODULE_DESCRIPTION("ChromeOS EC Charge State Driver");
+MODULE_AUTHOR("Sung-Chi, Li <lschyi@chromium.org>");
+MODULE_LICENSE("GPL");

-- 
2.47.0.338.g60cca15819-goog

[PATCH 2/3] dt-bindings: chrome: add new binding google,cros-ec-chrage-state

[Sung-Chi, Li]

Add new dt bindings for charge chip control. The charge chip control
dt configuration is used by the driver 'cros-ec-charge-state', which is
added in the commit "platform/chrome: cros_ec_charge_state: add new
driver to control charge".

As these charge chip controls are connected under the ChromeOS Embedded
Controller (EC), also add the patternProperties to the
mfd/google,cros-ec bindings.

Signed-off-by: Sung-Chi, Li <lschyi@chromium.org>
---
 .../bindings/chrome/google,cros-charge-state.yaml  | 62 ++++++++++++++++++++++
 .../devicetree/bindings/mfd/google,cros-ec.yaml    |  4 ++
 2 files changed, 66 insertions(+)

diff --git a/Documentation/devicetree/bindings/chrome/google,cros-charge-state.yaml b/Documentation/devicetree/bindings/chrome/google,cros-charge-state.yaml
new file mode 100644
index 000000000000..40e8f6988769
--- /dev/null
+++ b/Documentation/devicetree/bindings/chrome/google,cros-charge-state.yaml
@@ -0,0 +1,62 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/chrome/google,cros-charge-state.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Google Chrome OS EC(Embedded Controller) charge state driver.
+
+maintainers:
+  - Sung-Chi, Li <lschyi@chromium.org>
+
+description:
+  Chrome OS devices have an Embedded Controller(EC) which has access to
+  battery charger IC. This node is intended to allow the host to read and
+  control the charger current. The node for this device should be under a
+  cros-ec node like google,cros-ec-spi.
+
+properties:
+  compatible:
+    const: google,cros-ec-charge-state
+
+  min-milliamp:
+    description: min current in milliamp.
+    $ref: /schemas/types.yaml#/definitions/uint32
+
+  max-milliamp:
+    description: max current in milliamp.
+    $ref: /schemas/types.yaml#/definitions/uint32
+
+  type:
+    description: current limit type.
+    enum:
+      - charge
+      - input
+
+required:
+  - compatible
+  - min-milliamp
+  - man-milliamp
+  - type
+
+additionalProperties: false
+
+examples:
+  - |+
+    spi {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      cros_ec: ec@0 {
+        compatible = "google,cros-ec-spi";
+        reg = <0>;
+        interrupts = <35 0>;
+
+        charge_chip_battery_current: charge-chip-battery {
+          compatible = "google,cros-ec-charge";
+          type = "charge";
+          min-milliamp = <150>;
+          max-milliamp = <5000>;
+        };
+      };
+    };
diff --git a/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml b/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml
index aac8819bd00b..3db4a48d5176 100644
--- a/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml
+++ b/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml
@@ -166,6 +166,10 @@ patternProperties:
     type: object
     $ref: /schemas/extcon/extcon-usbc-cros-ec.yaml#
 
+  "^charge-chip-*":
+    type: object
+    $ref: /schemas/chrome/google,cros-charge-state.yaml#
+
 required:
   - compatible
 

-- 
2.47.0.338.g60cca15819-goog

[PATCH 3/3] mfd: cros_ec: Add charge state control cell

[Sung-Chi, Li]

The driver of controlling the charge chip connected on the ChromeOS
Embedded Controller (EC) is added in the commit "platform/chrome:
cros_ec_charge_state: add new driver to control charge". To register
the charge state sub-devices, add mfd cells in the cros-ec-dev mfd
driver, and register charge state sub-devices based on whether the EC
supports battery feature.

Signed-off-by: Sung-Chi, Li <lschyi@chromium.org>
---
 drivers/mfd/cros_ec_dev.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
index 9f84a52b48d6..fcb4deac3bf3 100644
--- a/drivers/mfd/cros_ec_dev.c
+++ b/drivers/mfd/cros_ec_dev.c
@@ -112,6 +112,10 @@ static const struct mfd_cell cros_ec_ucsi_cells[] = {
 	{ .name = "cros_ec_ucsi", },
 };
 
+static const struct mfd_cell cros_ec_charge_state_cells[] = {
+	{ .name = "cros-ec-charge-state", },
+};
+
 static const struct cros_feature_to_cells cros_subdevices[] = {
 	{
 		.id		= EC_FEATURE_CEC,
@@ -148,6 +152,11 @@ static const struct cros_feature_to_cells cros_subdevices[] = {
 		.mfd_cells	= cros_ec_keyboard_leds_cells,
 		.num_cells	= ARRAY_SIZE(cros_ec_keyboard_leds_cells),
 	},
+	{
+		.id		= EC_FEATURE_BATTERY,
+		.mfd_cells	= cros_ec_charge_state_cells,
+		.num_cells	= ARRAY_SIZE(cros_ec_charge_state_cells),
+	},
 };
 
 static const struct mfd_cell cros_ec_platform_cells[] = {

-- 
2.47.0.338.g60cca15819-goog

Re: [PATCH 2/3] dt-bindings: chrome: add new binding google,cros-ec-chrage-state

[Rob Herring (Arm)]

On Mon, 18 Nov 2024 17:33:47 +0800, Sung-Chi, Li wrote:
> Add new dt bindings for charge chip control. The charge chip control
> dt configuration is used by the driver 'cros-ec-charge-state', which is
> added in the commit "platform/chrome: cros_ec_charge_state: add new
> driver to control charge".
> 
> As these charge chip controls are connected under the ChromeOS Embedded
> Controller (EC), also add the patternProperties to the
> mfd/google,cros-ec bindings.
> 
> Signed-off-by: Sung-Chi, Li <lschyi@chromium.org>
> ---
>  .../bindings/chrome/google,cros-charge-state.yaml  | 62 ++++++++++++++++++++++
>  .../devicetree/bindings/mfd/google,cros-ec.yaml    |  4 ++
>  2 files changed, 66 insertions(+)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/chrome/google,cros-charge-state.example.dtb: ec@0: charge-chip-battery:compatible:0: 'google,cros-ec-charge-state' was expected
	from schema $id: http://devicetree.org/schemas/mfd/google,cros-ec.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/chrome/google,cros-charge-state.example.dtb: ec@0: charge-chip-battery: 'man-milliamp' is a required property
	from schema $id: http://devicetree.org/schemas/mfd/google,cros-ec.yaml#
Documentation/devicetree/bindings/chrome/google,cros-charge-state.example.dtb: /example-0/spi/ec@0/charge-chip-battery: failed to match any schema with compatible: ['google,cros-ec-charge']

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20241118-add_charger_state-v1-2-94997079f35a@chromium.org

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

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 after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH 2/3] dt-bindings: chrome: add new binding google,cros-ec-chrage-state

[Rob Herring]

On Mon, Nov 18, 2024 at 05:33:47PM +0800, Sung-Chi, Li wrote:
> Add new dt bindings for charge chip control. The charge chip control
> dt configuration is used by the driver 'cros-ec-charge-state', which is
> added in the commit "platform/chrome: cros_ec_charge_state: add new
> driver to control charge".
> 
> As these charge chip controls are connected under the ChromeOS Embedded
> Controller (EC), also add the patternProperties to the
> mfd/google,cros-ec bindings.
> 
> Signed-off-by: Sung-Chi, Li <lschyi@chromium.org>
> ---
>  .../bindings/chrome/google,cros-charge-state.yaml  | 62 ++++++++++++++++++++++
>  .../devicetree/bindings/mfd/google,cros-ec.yaml    |  4 ++
>  2 files changed, 66 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/chrome/google,cros-charge-state.yaml b/Documentation/devicetree/bindings/chrome/google,cros-charge-state.yaml
> new file mode 100644
> index 000000000000..40e8f6988769
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/chrome/google,cros-charge-state.yaml
> @@ -0,0 +1,62 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/chrome/google,cros-charge-state.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Google Chrome OS EC(Embedded Controller) charge state driver.
> +
> +maintainers:
> +  - Sung-Chi, Li <lschyi@chromium.org>
> +
> +description:
> +  Chrome OS devices have an Embedded Controller(EC) which has access to
> +  battery charger IC. This node is intended to allow the host to read and
> +  control the charger current. The node for this device should be under a
> +  cros-ec node like google,cros-ec-spi.
> +
> +properties:
> +  compatible:
> +    const: google,cros-ec-charge-state
> +
> +  min-milliamp:
> +    description: min current in milliamp.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +
> +  max-milliamp:
> +    description: max current in milliamp.
> +    $ref: /schemas/types.yaml#/definitions/uint32

Use standard units defined in property-units.yaml.

No constraints? 4000000 amps is okay?

> +
> +  type:

Too generic. Property types are global. You need a vendor prefix for 
starters.

> +    description: current limit type.
> +    enum:
> +      - charge
> +      - input

What if you need to describe both?

> +
> +required:
> +  - compatible
> +  - min-milliamp
> +  - man-milliamp
> +  - type
> +
> +additionalProperties: false
> +
> +examples:
> +  - |+
> +    spi {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      cros_ec: ec@0 {
> +        compatible = "google,cros-ec-spi";
> +        reg = <0>;
> +        interrupts = <35 0>;
> +
> +        charge_chip_battery_current: charge-chip-battery {
> +          compatible = "google,cros-ec-charge";
> +          type = "charge";
> +          min-milliamp = <150>;
> +          max-milliamp = <5000>;
> +        };
> +      };
> +    };
> diff --git a/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml b/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml
> index aac8819bd00b..3db4a48d5176 100644
> --- a/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml
> +++ b/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml
> @@ -166,6 +166,10 @@ patternProperties:
>      type: object
>      $ref: /schemas/extcon/extcon-usbc-cros-ec.yaml#
>  
> +  "^charge-chip-*":
> +    type: object
> +    $ref: /schemas/chrome/google,cros-charge-state.yaml#
> +
>  required:
>    - compatible
>  
> 
> -- 
> 2.47.0.338.g60cca15819-goog
> 

Re: [PATCH 2/3] dt-bindings: chrome: add new binding google,cros-ec-chrage-state

[Sung-Chi, Li]

On Mon, Nov 18, 2024 at 02:25:20PM -0600, Rob Herring wrote:
> > +properties:
> > +  compatible:
> > +    const: google,cros-ec-charge-state
> > +
> > +  min-milliamp:
> > +    description: min current in milliamp.
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +
> > +  max-milliamp:
> > +    description: max current in milliamp.
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> 
> Use standard units defined in property-units.yaml.
> 
> No constraints? 4000000 amps is okay?
> 

Hi, I cannot find a good value as the max value, the will depend on what charge
chip is used on that device. This is like a upper bound set from the kernel
side, so setting it to the max uint32 value is acceptable (from the driver side
when loading this config), not the desired current value. It is equivalent to
kernel side do not specify any constraint. Surely, if kernel set a value that is
larger than the max value, the EC will reject that request, either. The real
current is bounded by the EC and the charge chip, so it will not damage any
hardware.
If we can find a meaningful constraint value to document it here, that would be
great.

Would it be sufficient that I add the explanation to the description?

> > +
> > +  type:
> 
> Too generic. Property types are global. You need a vendor prefix for 
> starters.
> 

Thank you, I will use a more specific name in the following patches.

> > +    description: current limit type.
> > +    enum:
> > +      - charge
> > +      - input
> 
> What if you need to describe both?
> 

We need to declare different DTS nods for each. This node is representing the
constraint, not the charge chip itself.
The voltage, min and max milliamp on each current type are different on a single
charge chip. For example, I have a device that uses the charge chip rt9490, and
it has the following set up:

- Input current
  - min-milliamp: 100
  - max-milliamp: 3300
- Charge current
  - min-milliamp: 150
  - max-milliamp: 5000

I cannot find a clean way to merge different current type, max, and min milliamp
just in a single DTS node.
Also, we need to split different constraints into its own DTS node. It is
because the a cooling device in the thermal framework need its own DTS node, so
we can use it in the trip section.

Re: [PATCH 1/3] platform/chrome: cros_ec_charge_state: add new driver to control charge

[Tzung-Bi Shih]

On Mon, Nov 18, 2024 at 05:33:46PM +0800, Sung-Chi, Li wrote:
> diff --git a/drivers/platform/chrome/cros_ec_charge_state.c b/drivers/platform/chrome/cros_ec_charge_state.c
[...]
> +#define DRV_NAME "cros-ec-charge-state"
> +#define CHARGE_TYPE_CHARGE "charge"
> +#define CHARGE_TYPE_INPUT "input"

I'm not a big fan of these kind of macros and would prefer to remove them.

> +static int
> +cros_ec_charge_state_get_current_limit(struct cros_ec_device *ec_dev,
> +				       enum charge_state_params charge_type,
> +				       uint32_t *limit)
> +{
[...]
> +	*limit = cpu_to_le32(state.get_param.value);
> +	return 0;
> +}
> +
> +static int
> +cros_ec_charge_state_set_current_limit(struct cros_ec_device *ec_dev,
> +				       enum charge_state_params charge_type,
> +				       uint32_t limit)
> +{
[...]
> +	param.set_param.value = cpu_to_le32(limit);

Looks weird to me.  Both getter and setter use cpu_to_le32()?  Should one
of them be le32_to_cpu()?

> +static int
> +cros_ec_charge_state_get_cur_state(struct thermal_cooling_device *cdev,
> +				   unsigned long *state)
> +{
> +	struct cros_ec_charge_state_data *data = cdev->devdata;
> +	uint32_t limit;
> +	int ret;
> +
> +	ret = cros_ec_charge_state_get_current_limit(data->ec_dev,
> +						     data->charge_type, &limit);
> +	if (ret) {
> +		dev_err(data->dev, "failed to get current state: %d", ret);

If something went wrong, and cros_ec_charge_state_get_current_limit() keeps
returning errors, would it somehow flood the kernel logs?

> +		return ret;
> +	}
> +
> +	*state = data->max_milliamp - limit;

Would it happen: data->max_milliamp - limit < data->min_milliamp == true?

> +static int
> +cros_ec_charge_state_register_charge_chip(struct device *dev,
> +					  struct device_node *node,
> +					  struct cros_ec_device *cros_ec)
> +{
[...]
> +
> +	if (!strcmp(type_val, CHARGE_TYPE_CHARGE)) {
> +		data->charge_type = CS_PARAM_CHG_CURRENT;
> +	} else if (!strcmp(type_val, CHARGE_TYPE_INPUT)) {
> +		data->charge_type = CS_PARAM_CHG_INPUT_CURRENT;
> +	} else {
> +		dev_err(dev, "unknown charge type: %s", type_val);
> +		return -1;

How about -EINVAL?

> +	}
> +
> +	ret = of_property_read_u32(node, "min-milliamp", &data->min_milliamp);
> +	if (ret) {
> +		dev_err(dev, "failed to get min-milliamp data: %d", ret);
> +		return ret;
> +	}
> +
> +	ret = of_property_read_u32(node, "max-milliamp", &data->max_milliamp);
> +	if (ret) {
> +		dev_err(dev, "failed to get max-milliamp data: %d", ret);
> +		return ret;
> +	}

Would it happen: min-milliamp > max-milliamp == true?

> +
> +	data->ec_dev = cros_ec;
> +	data->dev = dev;
> +
> +	cdev = devm_thermal_of_cooling_device_register(
> +		dev, node, node->name, data,
> +		&cros_ec_charge_state_cooling_device_ops);
> +	if (IS_ERR_VALUE(cdev)) {

Any reasons to not use IS_ERR()?

> +static int cros_ec_charge_state_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct cros_ec_dev *ec_dev = dev_get_drvdata(dev->parent);
> +	struct cros_ec_device *cros_ec = ec_dev->ec_dev;
> +	struct device_node *child;
> +
> +	for_each_available_child_of_node(cros_ec->dev->of_node, child) {
> +		if (!of_device_is_compatible(child,
> +					     "google,cros-ec-charge-state"))
> +			continue;
> +		if (cros_ec_charge_state_register_charge_chip(dev, child,
> +							      cros_ec))
> +			continue;
> +	}

There should be a way to use the compatible string in struct mfd_cell for
matching the node.  See also [1].

[1]: https://elixir.bootlin.com/linux/v6.12/source/drivers/mfd/mfd-core.c#L184

> +
> +static const struct platform_device_id cros_ec_charge_state_id[] = {
> +	{ DRV_NAME,  0 },
                    ^

> +static struct platform_driver cros_ec_chargedev_driver = {

The whole file uses "cros_ec_charge_state_" as a prefix for all symbols.  Any
reasons to not make this consistent?

Re: [PATCH 2/3] dt-bindings: chrome: add new binding google,cros-ec-chrage-state

[Krzysztof Kozlowski]

On 19/11/2024 03:23, Sung-Chi, Li wrote:
> 
>>> +
>>> +  type:
>>
>> Too generic. Property types are global. You need a vendor prefix for 
>> starters.
>>
> 
> Thank you, I will use a more specific name in the following patches.
> 
>>> +    description: current limit type.
>>> +    enum:
>>> +      - charge
>>> +      - input
>>
>> What if you need to describe both?
>>
> 
> We need to declare different DTS nods for each. This node is representing the
> constraint, not the charge chip itself.

Looks like you are re-implementing charger manager. Even title says: driver.

Use standard psy properties including battery. All this is supposed to
describe hardware, not your driver.

> The voltage, min and max milliamp on each current type are different on a single
> charge chip. For example, I have a device that uses the charge chip rt9490, and
> it has the following set up:

> 
> - Input current
>   - min-milliamp: 100
>   - max-milliamp: 3300
> - Charge current
>   - min-milliamp: 150
>   - max-milliamp: 5000
> 
> I cannot find a clean way to merge different current type, max, and min milliamp
> just in a single DTS node.

Well, all other bindings were able, so I really do not get why this one
is so special.

> Also, we need to split different constraints into its own DTS node. It is
> because the a cooling device in the thermal framework need its own DTS node, so
> we can use it in the trip section.

So fix thermal framework.

Best regards,
Krzysztof

Re: [PATCH 2/3] dt-bindings: chrome: add new binding google,cros-ec-chrage-state

[Krzysztof Kozlowski]

On 18/11/2024 10:33, Sung-Chi, Li wrote:
> Add new dt bindings for charge chip control. The charge chip control
> dt configuration is used by the driver 'cros-ec-charge-state', which is
> added in the commit "platform/chrome: cros_ec_charge_state: add new
> driver to control charge".
> 
> As these charge chip controls are connected under the ChromeOS Embedded
> Controller (EC), also add the patternProperties to the
> mfd/google,cros-ec bindings.
> 
> Signed-off-by: Sung-Chi, Li <lschyi@chromium.org>
> ---
>  .../bindings/chrome/google,cros-charge-state.yaml  | 62 ++++++++++++++++++++++
>  .../devicetree/bindings/mfd/google,cros-ec.yaml    |  4 ++
>  2 files changed, 66 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/chrome/google,cros-charge-state.yaml b/Documentation/devicetree/bindings/chrome/google,cros-charge-state.yaml
> new file mode 100644
> index 000000000000..40e8f6988769
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/chrome/google,cros-charge-state.yaml
> @@ -0,0 +1,62 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/chrome/google,cros-charge-state.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Google Chrome OS EC(Embedded Controller) charge state driver.

Capitalize, drop driver, drop full stop.


...

> +examples:
> +  - |+

No need for +

> +    spi {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      cros_ec: ec@0 {
> +        compatible = "google,cros-ec-spi";
> +        reg = <0>;
> +        interrupts = <35 0>;
> +
> +        charge_chip_battery_current: charge-chip-battery {

1. Drop unused label.
2. So this is a battery? Then just "battery"... or this is a charger?
Please look how power supplies are done. This should not be different.

Best regards,
Krzysztof

Re: [PATCH 1/3] platform/chrome: cros_ec_charge_state: add new driver to control charge

[kernel test robot]

Hi Li,

kernel test robot noticed the following build warnings:

[auto build test WARNING on lee-mfd/for-mfd-next]
[also build test WARNING on robh/for-next groeck-staging/hwmon-next chrome-platform/for-next chrome-platform/for-firmware-next lee-mfd/for-mfd-fixes linus/master v6.12 next-20241121]
[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/Sung-Chi-Li/platform-chrome-cros_ec_charge_state-add-new-driver-to-control-charge/20241121-112442
base:   https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git for-mfd-next
patch link:    https://lore.kernel.org/r/20241118-add_charger_state-v1-1-94997079f35a%40chromium.org
patch subject: [PATCH 1/3] platform/chrome: cros_ec_charge_state: add new driver to control charge
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20241121/202411212036.M5ujDUNV-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241121/202411212036.M5ujDUNV-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202411212036.M5ujDUNV-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/platform/chrome/cros_ec_charge_state.c:198:40: warning: 'cros_ec_charge_state_id' defined but not used [-Wunused-const-variable=]
     198 | static const struct platform_device_id cros_ec_charge_state_id[] = {
         |                                        ^~~~~~~~~~~~~~~~~~~~~~~


vim +/cros_ec_charge_state_id +198 drivers/platform/chrome/cros_ec_charge_state.c

   197	
 > 198	static const struct platform_device_id cros_ec_charge_state_id[] = {
   199		{ DRV_NAME,  0 },
   200		{}
   201	};
   202	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 1/3] platform/chrome: cros_ec_charge_state: add new driver to control charge

[Thomas Weißschuh]

On 2024-11-18 17:33:46+0800, Sung-Chi, Li wrote:
> Implement the new platform driver cros_ec_charge_state to have low finer
> control over the charge current flow through the charge chip connected
> on ChromeOS Embedded Controller (EC).
> 
> The driver reads configured charge chip configurations from the device
> tree, and register these chip controls as thermal zone devices, so they
> are controllable from the thermal subsystem.
> 
> As such, corresponding DTS changes are needed, and here is a sample DTS
> configuration:
> 
> ```
> &cros_ec {
> 	charge-chip-battery {
> 		compatible = "google,cros-ec-charge-state";
> 		type = "charge";
> 		min-milliamp = <150>;
> 		max-milliamp = <5000>;
> 	};
> };
> ```
> 
> Signed-off-by: Sung-Chi, Li <lschyi@chromium.org>
> ---
>  drivers/platform/chrome/Kconfig                |  11 ++
>  drivers/platform/chrome/Makefile               |   1 +
>  drivers/platform/chrome/cros_ec_charge_state.c | 215 +++++++++++++++++++++++++
>  3 files changed, 227 insertions(+)
> 
> diff --git a/drivers/platform/chrome/Kconfig b/drivers/platform/chrome/Kconfig
> index 7dbeb786352a..34d00d8823cb 100644
> --- a/drivers/platform/chrome/Kconfig
> +++ b/drivers/platform/chrome/Kconfig
> @@ -297,6 +297,17 @@ config CROS_TYPEC_SWITCH
>  	  To compile this driver as a module, choose M here: the module will be
>  	  called cros_typec_switch.
>  
> +config CROS_CHARGE_STATE
> +	tristate "ChromeOS EC Charger Chip  Control"
> +	depends on MFD_CROS_EC_DEV

Should depend on THERMAL_OF.
Otherwise the driver will be built and loaded on non-OF platforms but
probing can never succeed.

> +	default MFD_CROS_EC_DEV
> +	help
> +	  If you say Y here, you get support for configuring the battery
> +	  charging and system input current.
> +
> +	  To compile this driver as a module, choose M here: the module will be
> +	  called cros-ec-charge-state.
> +
>  source "drivers/platform/chrome/wilco_ec/Kconfig"
>  
>  # Kunit test cases
> diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile
> index 2dcc6ccc2302..01c7154ae119 100644
> --- a/drivers/platform/chrome/Makefile
> +++ b/drivers/platform/chrome/Makefile
> @@ -32,6 +32,7 @@ obj-$(CONFIG_CROS_EC_SYSFS)		+= cros_ec_sysfs.o
>  obj-$(CONFIG_CROS_HPS_I2C)		+= cros_hps_i2c.o
>  obj-$(CONFIG_CROS_USBPD_LOGGER)		+= cros_usbpd_logger.o
>  obj-$(CONFIG_CROS_USBPD_NOTIFY)		+= cros_usbpd_notify.o
> +obj-$(CONFIG_CROS_CHARGE_STATE)		+= cros_ec_charge_state.o
>  
>  obj-$(CONFIG_WILCO_EC)			+= wilco_ec/
>  
> diff --git a/drivers/platform/chrome/cros_ec_charge_state.c b/drivers/platform/chrome/cros_ec_charge_state.c
> new file mode 100644
> index 000000000000..3fed5b48bc92
> --- /dev/null
> +++ b/drivers/platform/chrome/cros_ec_charge_state.c
> @@ -0,0 +1,215 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Charge state driver for ChromeOS Embedded Controller
> + *
> + * Copyright 2024 Google LLC
> + *
> + * This driver exports the low level control over charge chip connected to EC
> + * which allows to manipulate the current used to charge the battery, and also
> + * manipulate the current input to the whole system.
> + * This driver also registers that charge chip as a thermal cooling device.
> + */
> +
> +#include <linux/of.h>
> +#include <linux/platform_data/cros_ec_commands.h>
> +#include <linux/platform_data/cros_ec_proto.h>
> +#include <linux/platform_device.h>
> +#include <linux/thermal.h>
> +
> +#define DRV_NAME "cros-ec-charge-state"
> +#define CHARGE_TYPE_CHARGE "charge"
> +#define CHARGE_TYPE_INPUT "input"
> +
> +struct cros_ec_charge_state_data {
> +	struct cros_ec_device *ec_dev;
> +	struct device *dev;
> +	enum charge_state_params charge_type;
> +	uint32_t min_milliamp;
> +	uint32_t max_milliamp;
> +};
> +
> +static int
> +cros_ec_charge_state_get_current_limit(struct cros_ec_device *ec_dev,
> +				       enum charge_state_params charge_type,
> +				       uint32_t *limit)
> +{
> +	struct ec_params_charge_state param;
> +	struct ec_response_charge_state state;
> +	int ret;
> +
> +	param.cmd = CHARGE_STATE_CMD_GET_PARAM;
> +	param.get_param.param = charge_type;
> +	ret = cros_ec_cmd(ec_dev, 0, EC_CMD_CHARGE_STATE, &param, sizeof(param),
> +			  &state, sizeof(state));
> +	if (ret < 0)
> +		return ret;
> +
> +	*limit = cpu_to_le32(state.get_param.value);

The cros_ec core itself does not handle BE systems.
So I'm not sure if it's worth trying to handle it in the driver.

> +	return 0;
> +}
> +
> +static int
> +cros_ec_charge_state_set_current_limit(struct cros_ec_device *ec_dev,
> +				       enum charge_state_params charge_type,
> +				       uint32_t limit)
> +{
> +	struct ec_params_charge_state param;
> +	int ret;
> +
> +	param.cmd = CHARGE_STATE_CMD_SET_PARAM;
> +	param.set_param.param = charge_type;
> +	param.set_param.value = cpu_to_le32(limit);
> +	ret = cros_ec_cmd(ec_dev, 0, EC_CMD_CHARGE_STATE, &param, sizeof(param),
> +			  NULL, 0);
> +	return (ret < 0) ? ret : 0;
> +}
> +
> +static int
> +cros_ec_charge_state_get_max_state(struct thermal_cooling_device *cdev,
> +				   unsigned long *state)
> +{
> +	struct cros_ec_charge_state_data *data = cdev->devdata;
> +	*state = data->max_milliamp;
> +	return 0;
> +}
> +
> +static int
> +cros_ec_charge_state_get_cur_state(struct thermal_cooling_device *cdev,
> +				   unsigned long *state)
> +{
> +	struct cros_ec_charge_state_data *data = cdev->devdata;
> +	uint32_t limit;
> +	int ret;
> +
> +	ret = cros_ec_charge_state_get_current_limit(data->ec_dev,
> +						     data->charge_type, &limit);
> +	if (ret) {
> +		dev_err(data->dev, "failed to get current state: %d", ret);
> +		return ret;
> +	}
> +
> +	*state = data->max_milliamp - limit;
> +	return 0;
> +}
> +
> +static int
> +cros_ec_charge_state_set_cur_state(struct thermal_cooling_device *cdev,
> +				   unsigned long state)
> +{
> +	struct cros_ec_charge_state_data *data = cdev->devdata;
> +	uint32_t limit = data->max_milliamp - state;
> +
> +	if (limit < data->min_milliamp) {
> +		dev_warn(
> +			data->dev,
> +			"failed to set current %u lower than minimum %d; set to minimum",
> +			limit, data->min_milliamp);
> +		limit = data->min_milliamp;
> +	}
> +
> +	state = data->max_milliamp - limit;
> +	return cros_ec_charge_state_set_current_limit(
> +		data->ec_dev, data->charge_type, (uint32_t)state);
> +}
> +
> +static const struct thermal_cooling_device_ops
> +	cros_ec_charge_state_cooling_device_ops = {
> +		.get_max_state = cros_ec_charge_state_get_max_state,
> +		.get_cur_state = cros_ec_charge_state_get_cur_state,
> +		.set_cur_state = cros_ec_charge_state_set_cur_state,
> +	};
> +
> +static int
> +cros_ec_charge_state_register_charge_chip(struct device *dev,
> +					  struct device_node *node,
> +					  struct cros_ec_device *cros_ec)
> +{
> +	struct cros_ec_charge_state_data *data;
> +	struct thermal_cooling_device *cdev;
> +	const char *type_val = NULL;
> +	int ret;
> +
> +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	ret = of_property_read_string(node, "type", &type_val);
> +	if (ret) {
> +		dev_err(dev, "failed to get charge type: %d", ret);
> +		return ret;

return dev_err_probe(dev, ...)

> +	}
> +
> +	if (!strcmp(type_val, CHARGE_TYPE_CHARGE)) {
> +		data->charge_type = CS_PARAM_CHG_CURRENT;
> +	} else if (!strcmp(type_val, CHARGE_TYPE_INPUT)) {
> +		data->charge_type = CS_PARAM_CHG_INPUT_CURRENT;
> +	} else {
> +		dev_err(dev, "unknown charge type: %s", type_val);
> +		return -1;
> +	}
> +
> +	ret = of_property_read_u32(node, "min-milliamp", &data->min_milliamp);
> +	if (ret) {
> +		dev_err(dev, "failed to get min-milliamp data: %d", ret);
> +		return ret;
> +	}
> +
> +	ret = of_property_read_u32(node, "max-milliamp", &data->max_milliamp);
> +	if (ret) {
> +		dev_err(dev, "failed to get max-milliamp data: %d", ret);
> +		return ret;
> +	}
> +
> +	data->ec_dev = cros_ec;
> +	data->dev = dev;
> +
> +	cdev = devm_thermal_of_cooling_device_register(
> +		dev, node, node->name, data,
> +		&cros_ec_charge_state_cooling_device_ops);
> +	if (IS_ERR_VALUE(cdev)) {
> +		dev_err(dev,
> +			"failed to register charge chip %s as cooling device: %pe",
> +			node->name, cdev);
> +		return PTR_ERR(cdev);
> +	}
> +
> +	return 0;
> +}
> +
> +static int cros_ec_charge_state_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct cros_ec_dev *ec_dev = dev_get_drvdata(dev->parent);
> +	struct cros_ec_device *cros_ec = ec_dev->ec_dev;
> +	struct device_node *child;
> +
> +	for_each_available_child_of_node(cros_ec->dev->of_node, child) {
> +		if (!of_device_is_compatible(child,
> +					     "google,cros-ec-charge-state"))
> +			continue;
> +		if (cros_ec_charge_state_register_charge_chip(dev, child,
> +							      cros_ec))
> +			continue;
> +	}

If no chips are matched -ENODEV would be better.
And errors should be reported from probe().

Given that this is only useable with OF configuration, would it make
more sense to probe it via OF instead of MFD?

> +
> +	return 0;
> +}
> +
> +static const struct platform_device_id cros_ec_charge_state_id[] = {
> +	{ DRV_NAME,  0 },
> +	{}
> +};

Reference this in the platform_driver below.

> +static struct platform_driver cros_ec_chargedev_driver = {
> +	.driver = {
> +		.name = DRV_NAME,
> +	},
> +	.probe = cros_ec_charge_state_probe,
> +};
> +
> +module_platform_driver(cros_ec_chargedev_driver);
> +
> +MODULE_DEVICE_TABLE(platform, cros_ec_charge_state_id);
> +MODULE_DESCRIPTION("ChromeOS EC Charge State Driver");
> +MODULE_AUTHOR("Sung-Chi, Li <lschyi@chromium.org>");
> +MODULE_LICENSE("GPL");
> 
> -- 
> 2.47.0.338.g60cca15819-goog
> 

Re: [PATCH 1/3] platform/chrome: cros_ec_charge_state: add new driver to control charge

[Krzysztof Kozlowski]

On 21/11/2024 14:47, Thomas Weißschuh wrote:
> 
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct platform_device_id cros_ec_charge_state_id[] = {
>> +	{ DRV_NAME,  0 },
>> +	{}
>> +};
> 
> Reference this in the platform_driver below.

And missing module device table... This wasn't ever tested as module.

Best regards,
Krzysztof

Re: [PATCH 1/3] platform/chrome: cros_ec_charge_state: add new driver to control charge

[Thomas Weißschuh]

On 2024-11-21 15:00:13+0100, Krzysztof Kozlowski wrote:
> On 21/11/2024 14:47, Thomas Weißschuh wrote:
> > 
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static const struct platform_device_id cros_ec_charge_state_id[] = {
> >> +	{ DRV_NAME,  0 },
> >> +	{}
> >> +};
> > 
> > Reference this in the platform_driver below.
> 
> And missing module device table... This wasn't ever tested as module.

It has one in the general MODULE_*() macro soup at the end of the file.
But yes, it should be moved where it can be found, right after
cros_ec_charge_state_id.

Re: [PATCH 1/3] platform/chrome: cros_ec_charge_state: add new driver to control charge

[Sung-Chi, Li]

On Thu, Nov 21, 2024 at 03:11:30PM +0100, Thomas Weißschuh wrote:
> On 2024-11-21 15:00:13+0100, Krzysztof Kozlowski wrote:
> > On 21/11/2024 14:47, Thomas Weißschuh wrote:
> > > 
> > >> +
> > >> +	return 0;
> > >> +}
> > >> +
> > >> +static const struct platform_device_id cros_ec_charge_state_id[] = {
> > >> +	{ DRV_NAME,  0 },
> > >> +	{}
> > >> +};
> > > 
> > > Reference this in the platform_driver below.
> > 
> > And missing module device table... This wasn't ever tested as module.
> 
> It has one in the general MODULE_*() macro soup at the end of the file.
> But yes, it should be moved where it can be found, right after
> cros_ec_charge_state_id.

Thank you all for spending time reviewing my changes, and I am very sorry that
I made so such careless mistakes. All these input are very valuable, and I
learnt a lot from them, and I will prevent these mistakes in future commits.

As we have seen lots of inputs from the DTS change commit, and I spent lot of
time coming up a better solution for achieving my goal (export certain
mechanisms, such that we can limit the charger chip current as a cooling
device), I think maybe extending functionalities in the 
driver/power/supply/cros_usbpd-charger.c would be a better approach. As a
result, I will stop the development on this series. So anyone is helping on this
series can stop review these changes.

However, because I am kind of new to developing the kernel driver module, any
inputs are welcome, and I have to say I really learnt a lot from mistakes
pointed by all of you, and I shall not make same mistakes in future
contributions.

Best,
Sung-Chi Li