| Message ID | 20240610-cros_ec-charge-control-v3-3-135e37252094@weissschuh.net (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | ChromeOS Embedded Controller charge control driver | expand |
On Mon, Jun 10, 2024 at 05:51:08PM +0200, Thomas Weißschuh wrote: > If the command is not supported at all the EC returns > -EINVAL/EC_RES_INVALID_PARAMS. > > This error is translated into an empty version mask as that is easier to > handle for callers and they don't need to know about the error details. I'm not sure whether the behavior is what we want or not as existing EC_CMD_GET_CMD_VERSIONS usages don't have it.
On 2024-06-11 06:32:38+0000, Tzung-Bi Shih wrote: > On Mon, Jun 10, 2024 at 05:51:08PM +0200, Thomas Weißschuh wrote: > > If the command is not supported at all the EC returns > > -EINVAL/EC_RES_INVALID_PARAMS. > > > > This error is translated into an empty version mask as that is easier to > > handle for callers and they don't need to know about the error details. > > I'm not sure whether the behavior is what we want or not as existing > EC_CMD_GET_CMD_VERSIONS usages don't have it. At least the caller of cros_ec_get_host_command_version_mask() expects it: ret = cros_ec_get_host_command_version_mask(..., &ver_mask); if (ret < 0 || ver_mask == 0) ... ver_mask == 0 will never happen as in that case -EINVAL would have been returned. Others, like cros_ec_cec_get_write_cmd_version(), expect the current semantic of ver_mask != 0 but log spurious errors in case of -EINVAL. cros_pchg_cmd_ver_check(), works with both semantics, but currently also logs a spurious error message. To me the new semantic looks more obvious and much easier to handle. For each command version a bit is set. no command versions -> no bits.
On Tue, Jun 11, 2024 at 09:23:24AM +0200, Thomas Weißschuh wrote: > On 2024-06-11 06:32:38+0000, Tzung-Bi Shih wrote: > > On Mon, Jun 10, 2024 at 05:51:08PM +0200, Thomas Weißschuh wrote: > > > If the command is not supported at all the EC returns > > > -EINVAL/EC_RES_INVALID_PARAMS. > > > > > > This error is translated into an empty version mask as that is easier to > > > handle for callers and they don't need to know about the error details. > > > > I'm not sure whether the behavior is what we want or not as existing > > EC_CMD_GET_CMD_VERSIONS usages don't have it. > > At least the caller of cros_ec_get_host_command_version_mask() expects > it: > > ret = cros_ec_get_host_command_version_mask(..., &ver_mask); > if (ret < 0 || ver_mask == 0) > ... > > ver_mask == 0 will never happen as in that case -EINVAL would have been > returned. > > Others, like cros_ec_cec_get_write_cmd_version(), expect the current > semantic of ver_mask != 0 but log spurious errors in case of -EINVAL. > cros_pchg_cmd_ver_check(), works with both semantics, but currently also > logs a spurious error message. > > To me the new semantic looks more obvious and much easier to handle. > For each command version a bit is set. no command versions -> no bits. Ack.
On Mon, Jun 10, 2024 at 05:51:08PM +0200, Thomas Weißschuh wrote: > +/** > + * cros_ec_cmd_versions - Get supported version mask. I guess we would like to call it something like "cros_ec_get_cmd_versions". > + * > + * @ec_dev: EC device > + * @cmd: Command to test > + * > + * Return: version mask on success, negative error number on failure. > + */ > +int cros_ec_cmd_versions(struct cros_ec_device *ec_dev, u16 cmd) Could it support a "version" parameter as existing EC_CMD_GET_CMD_VERSIONS usages use both versions? An `u16 cmd` parameter and down-cast to u8 for v0 should be fine. (ec_params_get_cmd_versions vs. ec_params_get_cmd_versions_v1)
On 2024-06-11 11:35:39+0000, Tzung-Bi Shih wrote: > On Mon, Jun 10, 2024 at 05:51:08PM +0200, Thomas Weißschuh wrote: > > +/** > > + * cros_ec_cmd_versions - Get supported version mask. > > I guess we would like to call it something like "cros_ec_get_cmd_versions". Ack. > > + * > > + * @ec_dev: EC device > > + * @cmd: Command to test > > + * > > + * Return: version mask on success, negative error number on failure. > > + */ > > +int cros_ec_cmd_versions(struct cros_ec_device *ec_dev, u16 cmd) > > Could it support a "version" parameter as existing EC_CMD_GET_CMD_VERSIONS > usages use both versions? An `u16 cmd` parameter and down-cast to u8 for v0 > should be fine. (ec_params_get_cmd_versions vs. ec_params_get_cmd_versions_v1) Ack. Or we could automatically use v0 if cmd <= U8_MAX?
On Tue, Jun 11, 2024 at 02:14:33PM +0200, Thomas Weißschuh wrote: > On 2024-06-11 11:35:39+0000, Tzung-Bi Shih wrote: > > On Mon, Jun 10, 2024 at 05:51:08PM +0200, Thomas Weißschuh wrote: > > > + * > > > + * @ec_dev: EC device > > > + * @cmd: Command to test > > > + * > > > + * Return: version mask on success, negative error number on failure. > > > + */ > > > +int cros_ec_cmd_versions(struct cros_ec_device *ec_dev, u16 cmd) > > > > Could it support a "version" parameter as existing EC_CMD_GET_CMD_VERSIONS > > usages use both versions? An `u16 cmd` parameter and down-cast to u8 for v0 > > should be fine. (ec_params_get_cmd_versions vs. ec_params_get_cmd_versions_v1) > > Ack. > > Or we could automatically use v0 if cmd <= U8_MAX? Ack. Looks feasible.
diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c index fe68be66ee98..9cfe885a5301 100644 --- a/drivers/platform/chrome/cros_ec_proto.c +++ b/drivers/platform/chrome/cros_ec_proto.c @@ -1069,3 +1069,29 @@ int cros_ec_cmd_readmem(struct cros_ec_device *ec_dev, u8 offset, u8 size, void ¶ms, sizeof(params), dest, size); } EXPORT_SYMBOL_GPL(cros_ec_cmd_readmem); + +/** + * cros_ec_cmd_versions - Get supported version mask. + * + * @ec_dev: EC device + * @cmd: Command to test + * + * Return: version mask on success, negative error number on failure. + */ +int cros_ec_cmd_versions(struct cros_ec_device *ec_dev, u16 cmd) +{ + struct ec_params_get_cmd_versions_v1 req = {}; + struct ec_response_get_cmd_versions resp; + int ret; + + req.cmd = cmd; + ret = cros_ec_cmd(ec_dev, 1, EC_CMD_GET_CMD_VERSIONS, + &req, sizeof(req), &resp, sizeof(resp)); + if (ret == -EINVAL) + return 0; /* Command not implemented */ + else if (ret < 0) + return ret; + else + return resp.version_mask; +} +EXPORT_SYMBOL_GPL(cros_ec_cmd_versions); diff --git a/include/linux/platform_data/cros_ec_proto.h b/include/linux/platform_data/cros_ec_proto.h index 6e9225bdf903..98ab5986f543 100644 --- a/include/linux/platform_data/cros_ec_proto.h +++ b/include/linux/platform_data/cros_ec_proto.h @@ -263,6 +263,8 @@ int cros_ec_cmd(struct cros_ec_device *ec_dev, unsigned int version, int command int cros_ec_cmd_readmem(struct cros_ec_device *ec_dev, u8 offset, u8 size, void *dest); +int cros_ec_cmd_versions(struct cros_ec_device *ec_dev, u16 cmd); + /** * cros_ec_get_time_ns() - Return time in ns. *
Retrieving the supported versions of a command is a fairly common operation. Provide a helper for it. If the command is not supported at all the EC returns -EINVAL/EC_RES_INVALID_PARAMS. This error is translated into an empty version mask as that is easier to handle for callers and they don't need to know about the error details. Signed-off-by: Thomas Weißschuh <linux@weissschuh.net> --- drivers/platform/chrome/cros_ec_proto.c | 26 ++++++++++++++++++++++++++ include/linux/platform_data/cros_ec_proto.h | 2 ++ 2 files changed, 28 insertions(+)