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.