Let's try to read more information out of more modern cros_ec devices by
using the v2 format first and then fall back to the v1 format. This
gives us more information about things such as DP mode of the typec pins
and the CC state, along with some more things.
Cc: Gwendal Grignou <[email protected]>
Cc: Prashant Malani <[email protected]>
Cc: Guenter Roeck <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
Should we move read_buf to the heap?
drivers/platform/chrome/cros_ec_debugfs.c | 40 +++++++++++++++++------
1 file changed, 30 insertions(+), 10 deletions(-)
diff --git a/drivers/platform/chrome/cros_ec_debugfs.c b/drivers/platform/chrome/cros_ec_debugfs.c
index 272c89837d74..4f8c902c0de6 100644
--- a/drivers/platform/chrome/cros_ec_debugfs.c
+++ b/drivers/platform/chrome/cros_ec_debugfs.c
@@ -195,28 +195,31 @@ static ssize_t cros_ec_pdinfo_read(struct file *file,
size_t count,
loff_t *ppos)
{
- char read_buf[EC_USB_PD_MAX_PORTS * 40], *p = read_buf;
+ char read_buf[EC_USB_PD_MAX_PORTS * 64], *p = read_buf;
struct cros_ec_debugfs *debug_info = file->private_data;
struct cros_ec_device *ec_dev = debug_info->ec->ec_dev;
struct {
struct cros_ec_command msg;
union {
- struct ec_response_usb_pd_control_v1 resp;
+ struct ec_response_usb_pd_control_v2 resp_v2;
+ struct ec_response_usb_pd_control_v1 resp_v1;
struct ec_params_usb_pd_control params;
};
} __packed ec_buf;
struct cros_ec_command *msg;
- struct ec_response_usb_pd_control_v1 *resp;
+ struct ec_response_usb_pd_control_v1 *resp_v1;
+ struct ec_response_usb_pd_control_v2 *resp_v2;
struct ec_params_usb_pd_control *params;
int i;
msg = &ec_buf.msg;
params = (struct ec_params_usb_pd_control *)msg->data;
- resp = (struct ec_response_usb_pd_control_v1 *)msg->data;
+ resp_v1 = (struct ec_response_usb_pd_control_v1 *)msg->data;
+ resp_v2 = (struct ec_response_usb_pd_control_v2 *)msg->data;
msg->command = EC_CMD_USB_PD_CONTROL;
- msg->version = 1;
- msg->insize = sizeof(*resp);
+ msg->version = 2;
+ msg->insize = sizeof(*resp_v2);
msg->outsize = sizeof(*params);
/*
@@ -229,13 +232,30 @@ static ssize_t cros_ec_pdinfo_read(struct file *file,
params->mux = 0;
params->swap = 0;
- if (cros_ec_cmd_xfer_status(ec_dev, msg) < 0)
+ if (cros_ec_cmd_xfer_status(ec_dev, msg) < 0) {
+ if (i == 0 && msg->version == 2) {
+ /* Try again with version 1 */
+ msg->version = 1;
+ msg->insize = sizeof(*resp_v1);
+ i = 0;
+ continue;
+ }
+
break;
+ }
p += scnprintf(p, sizeof(read_buf) + read_buf - p,
- "p%d: %s en:%.2x role:%.2x pol:%.2x\n", i,
- resp->state, resp->enabled, resp->role,
- resp->polarity);
+ "p%d: %s en:%.2x role:%.2x pol:%.2x", i,
+ resp_v1->state, resp_v1->enabled, resp_v1->role,
+ resp_v1->polarity);
+ if (msg->version == 2) {
+ p += scnprintf(p, sizeof(read_buf) + read_buf - p,
+ " cc:%.2x dp:%.2x ctrl:%.2x cs:%.2x gen:%.2x",
+ resp_v2->cc_state, resp_v2->dp_mode,
+ resp_v2->control_flags, resp_v2->cable_speed,
+ resp_v2->cable_gen);
+ }
+ p += scnprintf(p, sizeof(read_buf) + read_buf - p, "\n");
}
return simple_read_from_buffer(user_buf, count, ppos,
--
Sent by a computer, using git, on the internet
Hi Stephen, Prashant,
On 9/9/20 6:04, Stephen Boyd wrote:
> Let's try to read more information out of more modern cros_ec devices by
> using the v2 format first and then fall back to the v1 format. This
> gives us more information about things such as DP mode of the typec pins
> and the CC state, along with some more things.
>
> Cc: Gwendal Grignou <[email protected]>
> Cc: Prashant Malani <[email protected]>
> Cc: Guenter Roeck <[email protected]>
> Signed-off-by: Stephen Boyd <[email protected]>
> ---
>
I saw some discussion going on in gerrit (a pity the discussion didn't happen in
mainline) Did you end with a conclusion? Can I remove this patch from my backlog?
Thanks,
Enric
> Should we move read_buf to the heap?
>
> drivers/platform/chrome/cros_ec_debugfs.c | 40 +++++++++++++++++------
> 1 file changed, 30 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/platform/chrome/cros_ec_debugfs.c b/drivers/platform/chrome/cros_ec_debugfs.c
> index 272c89837d74..4f8c902c0de6 100644
> --- a/drivers/platform/chrome/cros_ec_debugfs.c
> +++ b/drivers/platform/chrome/cros_ec_debugfs.c
> @@ -195,28 +195,31 @@ static ssize_t cros_ec_pdinfo_read(struct file *file,
> size_t count,
> loff_t *ppos)
> {
> - char read_buf[EC_USB_PD_MAX_PORTS * 40], *p = read_buf;
> + char read_buf[EC_USB_PD_MAX_PORTS * 64], *p = read_buf;
> struct cros_ec_debugfs *debug_info = file->private_data;
> struct cros_ec_device *ec_dev = debug_info->ec->ec_dev;
> struct {
> struct cros_ec_command msg;
> union {
> - struct ec_response_usb_pd_control_v1 resp;
> + struct ec_response_usb_pd_control_v2 resp_v2;
> + struct ec_response_usb_pd_control_v1 resp_v1;
> struct ec_params_usb_pd_control params;
> };
> } __packed ec_buf;
> struct cros_ec_command *msg;
> - struct ec_response_usb_pd_control_v1 *resp;
> + struct ec_response_usb_pd_control_v1 *resp_v1;
> + struct ec_response_usb_pd_control_v2 *resp_v2;
> struct ec_params_usb_pd_control *params;
> int i;
>
> msg = &ec_buf.msg;
> params = (struct ec_params_usb_pd_control *)msg->data;
> - resp = (struct ec_response_usb_pd_control_v1 *)msg->data;
> + resp_v1 = (struct ec_response_usb_pd_control_v1 *)msg->data;
> + resp_v2 = (struct ec_response_usb_pd_control_v2 *)msg->data;
>
> msg->command = EC_CMD_USB_PD_CONTROL;
> - msg->version = 1;
> - msg->insize = sizeof(*resp);
> + msg->version = 2;
> + msg->insize = sizeof(*resp_v2);
> msg->outsize = sizeof(*params);
>
> /*
> @@ -229,13 +232,30 @@ static ssize_t cros_ec_pdinfo_read(struct file *file,
> params->mux = 0;
> params->swap = 0;
>
> - if (cros_ec_cmd_xfer_status(ec_dev, msg) < 0)
> + if (cros_ec_cmd_xfer_status(ec_dev, msg) < 0) {
> + if (i == 0 && msg->version == 2) {
> + /* Try again with version 1 */
> + msg->version = 1;
> + msg->insize = sizeof(*resp_v1);
> + i = 0;
> + continue;
> + }
> +
> break;
> + }
>
> p += scnprintf(p, sizeof(read_buf) + read_buf - p,
> - "p%d: %s en:%.2x role:%.2x pol:%.2x\n", i,
> - resp->state, resp->enabled, resp->role,
> - resp->polarity);
> + "p%d: %s en:%.2x role:%.2x pol:%.2x", i,
> + resp_v1->state, resp_v1->enabled, resp_v1->role,
> + resp_v1->polarity);
> + if (msg->version == 2) {
> + p += scnprintf(p, sizeof(read_buf) + read_buf - p,
> + " cc:%.2x dp:%.2x ctrl:%.2x cs:%.2x gen:%.2x",
> + resp_v2->cc_state, resp_v2->dp_mode,
> + resp_v2->control_flags, resp_v2->cable_speed,
> + resp_v2->cable_gen);
> + }
> + p += scnprintf(p, sizeof(read_buf) + read_buf - p, "\n");
> }
>
> return simple_read_from_buffer(user_buf, count, ppos,
>
HI Enric,
On Tue, Sep 15, 2020 at 5:48 AM Enric Balletbo i Serra
<[email protected]> wrote:
>
> Hi Stephen, Prashant,
>
> On 9/9/20 6:04, Stephen Boyd wrote:
> > Let's try to read more information out of more modern cros_ec devices by
> > using the v2 format first and then fall back to the v1 format. This
> > gives us more information about things such as DP mode of the typec pins
> > and the CC state, along with some more things.
> >
> > Cc: Gwendal Grignou <[email protected]>
> > Cc: Prashant Malani <[email protected]>
> > Cc: Guenter Roeck <[email protected]>
> > Signed-off-by: Stephen Boyd <[email protected]>
> > ---
> >
>
> I saw some discussion going on in gerrit (a pity the discussion didn't happen in
> mainline) Did you end with a conclusion? Can I remove this patch from my backlog?
My apologies for not posting the comment here.
To summarize: the userspace EC utility ectool [1] can offer the
equivalent output, but with better formatting. So I believe the
decision is to use that instead of this patch.
I also posed a counter-question: can we remove this debugfs pdinfo
file entirely, since we can pull this information using ectool?
[1]: https://chromium.googlesource.com/chromiumos/platform/ec/+/refs/heads/master/util/ectool.c
Best regards,
Hi Prashant,
On 15/9/20 17:34, Prashant Malani wrote:
> HI Enric,
>
> On Tue, Sep 15, 2020 at 5:48 AM Enric Balletbo i Serra
> <[email protected]> wrote:
>>
>> Hi Stephen, Prashant,
>>
>> On 9/9/20 6:04, Stephen Boyd wrote:
>>> Let's try to read more information out of more modern cros_ec devices by
>>> using the v2 format first and then fall back to the v1 format. This
>>> gives us more information about things such as DP mode of the typec pins
>>> and the CC state, along with some more things.
>>>
>>> Cc: Gwendal Grignou <[email protected]>
>>> Cc: Prashant Malani <[email protected]>
>>> Cc: Guenter Roeck <[email protected]>
>>> Signed-off-by: Stephen Boyd <[email protected]>
>>> ---
>>>
>>
>> I saw some discussion going on in gerrit (a pity the discussion didn't happen in
>> mainline) Did you end with a conclusion? Can I remove this patch from my backlog?
>
> My apologies for not posting the comment here.
> To summarize: the userspace EC utility ectool [1] can offer the
> equivalent output, but with better formatting. So I believe the
> decision is to use that instead of this patch.
> I also posed a counter-question: can we remove this debugfs pdinfo
> file entirely, since we can pull this information using ectool?
>
If I am not mistaken pdinfo is used by your userspace, so, assuming we don't
break (or we have a plan to not break) things I'm fine with it. In general,
delegating things to userspace and get rid of kernel code is good for everyone.
Thanks,
Enric
> [1]: https://chromium.googlesource.com/chromiumos/platform/ec/+/refs/heads/master/util/ectool.c
>
> Best regards,
>
Hi Enric,
On Wed, Sep 16, 2020 at 2:39 AM Enric Balletbo i Serra
<[email protected]> wrote:
>
> Hi Prashant,
>
>
> If I am not mistaken pdinfo is used by your userspace, so, assuming we don't
> break (or we have a plan to not break) things I'm fine with it. In general,
> delegating things to userspace and get rid of kernel code is good for everyone.
Sounds good, thanks! I will look into this and if it isn't breaking
anything I'll send a patch which does away with pdinfo.
Best regards,