Change in coreboot[master]: ec/google/chromeec: Add wrappers to get/set the voltage

[Yidi Lin (Code Review)]

Yidi Lin has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/46404 )

Change subject: ec/google/chromeec: Add wrappers to get/set the voltage
......................................................................

ec/google/chromeec: Add wrappers to get/set the voltage

Add APIs to get and set the voltage for the target regulator.

BUG=b:147789962
BRANCH=none
TEST=emerge-asurada coreboot

Change-Id: I0e56df45fc3309c387b9949534334eadefb616b2
Signed-off-by: Yidi Lin 
---
M src/ec/google/chromeec/ec.c
M src/ec/google/chromeec/ec.h
2 files changed, 64 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/46404/1

diff --git a/src/ec/google/chromeec/ec.c b/src/ec/google/chromeec/ec.c
index 82de088..39cf895 100644
--- a/src/ec/google/chromeec/ec.c
+++ b/src/ec/google/chromeec/ec.c
@@ -1616,3 +1616,50 @@

return 0;
 }
+
+int google_chromeec_regulator_set_voltage(uint32_t index, uint32_t min_mv,
+					  uint32_t max_mv)
+{
+	struct ec_params_regulator_set_voltage params = {
+		.index = index,
+		.min_mv = min_mv,
+		.max_mv = max_mv,
+	};
+	struct chromeec_command cmd = {
+		.cmd_code = EC_CMD_REGULATOR_SET_VOLTAGE,
+		.cmd_version = 0,
+		.cmd_data_in = &params,
+		.cmd_size_in = sizeof(params),
+		.cmd_data_out = NULL,
+		.cmd_size_out = 0,
+		.cmd_dev_index = 0,
+	};
+
+	if (google_chromeec_command(&cmd))
+		return -1;
+
+	return 0;
+}
+
+int google_chromeec_regulator_get_voltage(uint32_t index, uint32_t *voltage_mv)
+{
+	struct ec_params_regulator_get_voltage params = {
+		.index = index,
+	};
+	struct ec_response_regulator_get_voltage resp = {};
+	struct chromeec_command cmd = {
+		.cmd_code = EC_CMD_REGULATOR_GET_VOLTAGE,
+		.cmd_version = 0,
+		.cmd_data_in = &params,
+		.cmd_size_in = sizeof(params),
+		.cmd_data_out = &resp,
+		.cmd_size_out = sizeof(resp),
+		.cmd_dev_index = 0,
+	};
+
+	if (google_chromeec_command(&cmd))
+		return -1;
+
+	*voltage_mv = resp.voltage_mv;
+	return 0;
+}
diff --git a/src/ec/google/chromeec/ec.h b/src/ec/google/chromeec/ec.h
index ad3768c..0e28344 100644
--- a/src/ec/google/chromeec/ec.h
+++ b/src/ec/google/chromeec/ec.h
@@ -353,6 +353,23 @@
  */
 int google_chromeec_ap_reset(void);

+/**
+ * Set voltage for the voltage regulator within the range specified.
+ * @param index		Regulator ID
+ * @param min_mv	Minimum voltage
+ * @param max_mv	Maximum voltage
+ * @return		0 on success, -1 on error
+ */
+int google_chromeec_regulator_set_voltage(uint32_t index, uint32_t min_mv,
+					  uint32_t max_mv);
+/**
+ * Get the currently configured voltage for the voltage regulator.
+ * @param index		Regulator ID
+ * @param *voltage_mv	If successful, voltage_mv is filled with current voltage
+ * @return		0 on success, -1 on error
+ */
+int google_chromeec_regulator_get_voltage(uint32_t index, uint32_t *voltage_mv);
+
 #if CONFIG(HAVE_ACPI_TABLES)
 /**
  * Writes USB Type-C PD related information to the SSDT

[Hung-Te Lin (Code Review)]

Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46404 )

Change subject: ec/google/chromeec: Add wrappers to get/set the voltage
......................................................................

Patch Set 2: Code-Review+2

[Yu-Ping Wu (Code Review)]

Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46404 )

Change subject: ec/google/chromeec: Add wrappers to get/set the voltage
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/46404/3/src/ec/google/chromeec/ec.h 
File src/ec/google/chromeec/ec.h:

https://review.coreboot.org/c/coreboot/+/46404/3/src/ec/google/chromeec/ec.h... 
PS3, Line 365: /**
One blank line before this.

[Martin Roth (Code Review)]

Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46404 )

Change subject: ec/google/chromeec: Add wrappers to get/set the voltage
......................................................................

Patch Set 3:

Please don't rebase the entire patch train at once.  From the gerrit guidelines:

Don’t submit patch trains longer than around 20 patches unless you understand how to manage long patch trains. Long patch trains can become difficult to handle and tie up the build servers for long periods of time if not managed well. Rebasing a patch train over and over as you fix earlier patches in the train can hide comments, and make people review the code multiple times to see if anything has changed between revisions. When pushing long patch trains, it is recommended to only push the full patch train once - the initial time, and only to rebase three or four patches at a time.

https://doc.coreboot.org/getting_started/gerrit_guidelines.html

Thanks.

[Yidi Lin (Code Review)]

Hello Hung-Te Lin, build bot (Jenkins), Yu-Ping Wu,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/46404

to look at the new patch set (#4).

Change subject: ec/google/chromeec: Add wrappers to get/set the voltage
......................................................................

ec/google/chromeec: Add wrappers to get/set the voltage

Add APIs to get and set the voltage for the target regulator.

BUG=b:147789962
BRANCH=none
TEST=emerge-asurada coreboot

Change-Id: I0e56df45fc3309c387b9949534334eadefb616b2
Signed-off-by: Yidi Lin 
---
M src/ec/google/chromeec/ec.c
M src/ec/google/chromeec/ec.h
2 files changed, 65 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/46404/4

[Yidi Lin (Code Review)]

Yidi Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46404 )

Change subject: ec/google/chromeec: Add wrappers to get/set the voltage
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/46404/3/src/ec/google/chromeec/ec.h 
File src/ec/google/chromeec/ec.h:

https://review.coreboot.org/c/coreboot/+/46404/3/src/ec/google/chromeec/ec.h... 
PS3, Line 365: /**
> One blank line before this.

Ack

[Yu-Ping Wu (Code Review)]

Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46404 )

Change subject: ec/google/chromeec: Add wrappers to get/set the voltage
......................................................................

Patch Set 4: Code-Review+2

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46404 )

Change subject: ec/google/chromeec: Add wrappers to get/set the voltage
......................................................................

Patch Set 4:

(2 comments)

https://review.coreboot.org/c/coreboot/+/46404/4/src/ec/google/chromeec/ec.h 
File src/ec/google/chromeec/ec.h:

https://review.coreboot.org/c/coreboot/+/46404/4/src/ec/google/chromeec/ec.h... 
PS4, Line 361:  * @return		0 on success, -1 on error
Please use CB_SUCCESS and friends.

https://review.coreboot.org/c/coreboot/+/46404/4/src/ec/google/chromeec/ec.h... 
PS4, Line 372: int google_chromeec_regulator_get_voltage(uint32_t index, uint32_t *voltage_mv);
Why not return the voltage?

[Hung-Te Lin (Code Review)]

Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46404 )

Change subject: ec/google/chromeec: Add wrappers to get/set the voltage
......................................................................

Patch Set 4:

(2 comments)

https://review.coreboot.org/c/coreboot/+/46404/4/src/ec/google/chromeec/ec.h 
File src/ec/google/chromeec/ec.h:

https://review.coreboot.org/c/coreboot/+/46404/4/src/ec/google/chromeec/ec.h... 
PS4, Line 361:  * @return		0 on success, -1 on error
> Please use CB_SUCCESS and friends.

This is file is directly shared across cros_ec repo and coreboot, so it's better to keep 0/-1 since there's no CB_* definition there.

https://review.coreboot.org/c/coreboot/+/46404/4/src/ec/google/chromeec/ec.h... 
PS4, Line 372: int google_chromeec_regulator_get_voltage(uint32_t index, uint32_t *voltage_mv);
> Why not return the voltage?

The voltage is uint so if we return voltage directly (and not introducing another pointer to indicate success or not), then there must be a special value for defining failure, which is harder to maintain. Also, there's no guarantee the call will success (especially when wrong index is provided) so returning success or not is important.

[Hung-Te Lin (Code Review)]

Hung-Te Lin has submitted this change. ( https://review.coreboot.org/c/coreboot/+/46404 )

Change subject: ec/google/chromeec: Add wrappers to get/set the voltage
......................................................................

ec/google/chromeec: Add wrappers to get/set the voltage

Add APIs to get and set the voltage for the target regulator.

BUG=b:147789962
BRANCH=none
TEST=emerge-asurada coreboot

Change-Id: I0e56df45fc3309c387b9949534334eadefb616b2
Signed-off-by: Yidi Lin 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/46404
Tested-by: build bot (Jenkins) 
Reviewed-by: Yu-Ping Wu 
---
M src/ec/google/chromeec/ec.c
M src/ec/google/chromeec/ec.h
2 files changed, 65 insertions(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Yu-Ping Wu: Looks good to me, approved

diff --git a/src/ec/google/chromeec/ec.c b/src/ec/google/chromeec/ec.c
index 82de088..39cf895 100644
--- a/src/ec/google/chromeec/ec.c
+++ b/src/ec/google/chromeec/ec.c
@@ -1616,3 +1616,50 @@

return 0;
 }
+
+int google_chromeec_regulator_set_voltage(uint32_t index, uint32_t min_mv,
+					  uint32_t max_mv)
+{
+	struct ec_params_regulator_set_voltage params = {
+		.index = index,
+		.min_mv = min_mv,
+		.max_mv = max_mv,
+	};
+	struct chromeec_command cmd = {
+		.cmd_code = EC_CMD_REGULATOR_SET_VOLTAGE,
+		.cmd_version = 0,
+		.cmd_data_in = &params,
+		.cmd_size_in = sizeof(params),
+		.cmd_data_out = NULL,
+		.cmd_size_out = 0,
+		.cmd_dev_index = 0,
+	};
+
+	if (google_chromeec_command(&cmd))
+		return -1;
+
+	return 0;
+}
+
+int google_chromeec_regulator_get_voltage(uint32_t index, uint32_t *voltage_mv)
+{
+	struct ec_params_regulator_get_voltage params = {
+		.index = index,
+	};
+	struct ec_response_regulator_get_voltage resp = {};
+	struct chromeec_command cmd = {
+		.cmd_code = EC_CMD_REGULATOR_GET_VOLTAGE,
+		.cmd_version = 0,
+		.cmd_data_in = &params,
+		.cmd_size_in = sizeof(params),
+		.cmd_data_out = &resp,
+		.cmd_size_out = sizeof(resp),
+		.cmd_dev_index = 0,
+	};
+
+	if (google_chromeec_command(&cmd))
+		return -1;
+
+	*voltage_mv = resp.voltage_mv;
+	return 0;
+}
diff --git a/src/ec/google/chromeec/ec.h b/src/ec/google/chromeec/ec.h
index ad3768c..c2ceff8 100644
--- a/src/ec/google/chromeec/ec.h
+++ b/src/ec/google/chromeec/ec.h
@@ -353,6 +353,24 @@
  */
 int google_chromeec_ap_reset(void);

+/**
+ * Set voltage for the voltage regulator within the range specified.
+ * @param index		Regulator ID
+ * @param min_mv	Minimum voltage
+ * @param max_mv	Maximum voltage
+ * @return		0 on success, -1 on error
+ */
+int google_chromeec_regulator_set_voltage(uint32_t index, uint32_t min_mv,
+					  uint32_t max_mv);
+
+/**
+ * Get the currently configured voltage for the voltage regulator.
+ * @param index		Regulator ID
+ * @param *voltage_mv	If successful, voltage_mv is filled with current voltage
+ * @return		0 on success, -1 on error
+ */
+int google_chromeec_regulator_get_voltage(uint32_t index, uint32_t *voltage_mv);
+
 #if CONFIG(HAVE_ACPI_TABLES)
 /**
  * Writes USB Type-C PD related information to the SSDT