| Message ID | 20220802113957.v3.1.I2c8c550183162e7594309b66d19af696b8d84552@changeid (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] platform/chrome: cros_ec: Send host event for prepare/complete | expand |
LGTM On Tue, Aug 2, 2022 at 11:40 AM Tim Van Patten <timvp@google.com> wrote: > > Update cros_ec_lpc_pm_ops to call cros_ec_lpc_prepare() during PM > .prepare() and cros_ec_lpc_complete() during .complete(). This allows the > EC to log entry/exit of AP's suspend/resume more accurately. > > Signed-off-by: Tim Van Patten <timvp@google.com> > --- > > Changes in v3: > - Update cros_ec_lpc_suspend() to cros_ec_lpc_prepare() > - Update cros_ec_lpc_resume() to cros_ec_lpc_complete() > > Changes in v2: > - Include cros_ec_resume() return value in dev_info() output. > - Guard setting .prepare/.complete with #ifdef CONFIG_PM_SLEEP. > > drivers/platform/chrome/cros_ec_lpc.c | 16 ++++++++++++---- > 1 file changed, 12 insertions(+), 4 deletions(-) > > diff --git a/drivers/platform/chrome/cros_ec_lpc.c b/drivers/platform/chrome/cros_ec_lpc.c > index 7677ab3c0ead9..4158bdeee197b 100644 > --- a/drivers/platform/chrome/cros_ec_lpc.c > +++ b/drivers/platform/chrome/cros_ec_lpc.c > @@ -530,23 +530,31 @@ static const struct dmi_system_id cros_ec_lpc_dmi_table[] __initconst = { > MODULE_DEVICE_TABLE(dmi, cros_ec_lpc_dmi_table); > > #ifdef CONFIG_PM_SLEEP > -static int cros_ec_lpc_suspend(struct device *dev) > +static int cros_ec_lpc_prepare(struct device *dev) > { > struct cros_ec_device *ec_dev = dev_get_drvdata(dev); > > + dev_info(dev, "Prepare EC suspend\n"); > + > return cros_ec_suspend(ec_dev); > } > > -static int cros_ec_lpc_resume(struct device *dev) > +static void cros_ec_lpc_complete(struct device *dev) > { > struct cros_ec_device *ec_dev = dev_get_drvdata(dev); > + int ret; > + > + ret = cros_ec_resume(ec_dev); > > - return cros_ec_resume(ec_dev); > + dev_info(dev, "EC resume completed: ret = %d\n", ret); > } > #endif > > static const struct dev_pm_ops cros_ec_lpc_pm_ops = { > - SET_LATE_SYSTEM_SLEEP_PM_OPS(cros_ec_lpc_suspend, cros_ec_lpc_resume) > +#ifdef CONFIG_PM_SLEEP > + .prepare = cros_ec_lpc_prepare, > + .complete = cros_ec_lpc_complete > +#endif > }; > > static struct platform_driver cros_ec_lpc_driver = { > -- > 2.37.1.455.g008518b4e5-goog > Signed-off-by: Raul E Rangel <rrangel@chromium.org>
On Tue, Aug 02, 2022 at 11:40:08AM -0600, Tim Van Patten wrote: > Update cros_ec_lpc_pm_ops to call cros_ec_lpc_prepare() during PM > .prepare() and cros_ec_lpc_complete() during .complete(). This allows the > EC to log entry/exit of AP's suspend/resume more accurately. As what I commented on [1], the term "host event" in the commit title is confusing. Also, as this is a cros_ec_lpc specific patch, please change the prefix. [1]: https://patchwork.kernel.org/project/chrome-platform/patch/20220706205136.v2.1.Ic7a7c81f880ab31533652e0928aa6e687bb268b5@changeid/#24934911 > -static int cros_ec_lpc_resume(struct device *dev) > +static void cros_ec_lpc_complete(struct device *dev) > { > struct cros_ec_device *ec_dev = dev_get_drvdata(dev); > + int ret; > + > + ret = cros_ec_resume(ec_dev); > > - return cros_ec_resume(ec_dev); > + dev_info(dev, "EC resume completed: ret = %d\n", ret); cros_ec_resume() always returns 0.
On Wed, Aug 17, 2022 at 10:08 PM Tzung-Bi Shih <tzungbi@kernel.org> wrote: > > On Tue, Aug 02, 2022 at 11:40:08AM -0600, Tim Van Patten wrote: > > Update cros_ec_lpc_pm_ops to call cros_ec_lpc_prepare() during PM > > .prepare() and cros_ec_lpc_complete() during .complete(). This allows the > > EC to log entry/exit of AP's suspend/resume more accurately. > > As what I commented on [1], the term "host event" in the commit title is > confusing. Also, as this is a cros_ec_lpc specific patch, please change > the prefix. I've updated the prefix to "cros_ec_lpc" and the title/description to indicate that this CL moves when the host event is sent to .prepare()/.complete(). > [1]: https://patchwork.kernel.org/project/chrome-platform/patch/20220706205136.v2.1.Ic7a7c81f880ab31533652e0928aa6e687bb268b5@changeid/#24934911 > > > -static int cros_ec_lpc_resume(struct device *dev) > > +static void cros_ec_lpc_complete(struct device *dev) > > { > > struct cros_ec_device *ec_dev = dev_get_drvdata(dev); > > + int ret; > > + > > + ret = cros_ec_resume(ec_dev); > > > > - return cros_ec_resume(ec_dev); > > + dev_info(dev, "EC resume completed: ret = %d\n", ret); > > cros_ec_resume() always returns 0. Yes, it always returns 0 today, but that may not be the case forever. While "ret" is not returned by cros_ec_resume() today, it's possible for it to be non-zero and someone may update cros_ec_resume() to return that status. https://github.com/torvalds/linux/blob/1c23f9e627a7b412978b4e852793c5e3c3efc555/drivers/platform/chrome/cros_ec.c#L378 Additionally, everyone passes along the return code of cros_ec_resume(): drivers/platform/chrome/cros_ec_i2c.c 331: return cros_ec_resume(ec_dev); drivers/platform/chrome/cros_ec_rpmsg.c 285: return cros_ec_resume(ec_dev); drivers/platform/chrome/cros_ec_ishtp.c 766: return cros_ec_resume(client_data->ec_dev); drivers/platform/chrome/cros_ec_spi.c 808: return cros_ec_resume(ec_dev); drivers/platform/chrome/cros_ec_uart.c 390: return cros_ec_resume(ec_dev); drivers/platform/chrome/cros_ec_lpc.c 530: return cros_ec_resume(ec_dev); I'm going to leave this as-is, since it's good practice to check return values (and log bad status) and so we don't need to update this code in case cros_ec_resume() does return a non-zero value in the future.
On Mon, Aug 22, 2022 at 10:21:47AM -0600, Tim Van Patten wrote: > On Wed, Aug 17, 2022 at 10:08 PM Tzung-Bi Shih <tzungbi@kernel.org> wrote: > > > > On Tue, Aug 02, 2022 at 11:40:08AM -0600, Tim Van Patten wrote: > > > Update cros_ec_lpc_pm_ops to call cros_ec_lpc_prepare() during PM > > > .prepare() and cros_ec_lpc_complete() during .complete(). This allows the > > > EC to log entry/exit of AP's suspend/resume more accurately. > > > > As what I commented on [1], the term "host event" in the commit title is > > confusing. Also, as this is a cros_ec_lpc specific patch, please change > > the prefix. > > I've updated the prefix to "cros_ec_lpc" and the title/description to > indicate that this CL moves when the host event is sent to > .prepare()/.complete(). > > > [1]: https://patchwork.kernel.org/project/chrome-platform/patch/20220706205136.v2.1.Ic7a7c81f880ab31533652e0928aa6e687bb268b5@changeid/#24934911 I'm not sure if any unclear. "host event" is a terminology for CrOS EC. The usage here is confusing. > > > > > -static int cros_ec_lpc_resume(struct device *dev) > > > +static void cros_ec_lpc_complete(struct device *dev) > > > { > > > struct cros_ec_device *ec_dev = dev_get_drvdata(dev); > > > + int ret; > > > + > > > + ret = cros_ec_resume(ec_dev); > > > > > > - return cros_ec_resume(ec_dev); > > > + dev_info(dev, "EC resume completed: ret = %d\n", ret); > > > > cros_ec_resume() always returns 0. > > Yes, it always returns 0 today, but that may not be the case forever. > While "ret" is not returned by cros_ec_resume() today, it's possible > for it to be non-zero and someone may update cros_ec_resume() to > return that status. Does it really need to print if `ret` is always 0?
On Mon, Aug 22, 2022 at 8:49 PM Tzung-Bi Shih <tzungbi@kernel.org> wrote: > > On Mon, Aug 22, 2022 at 10:21:47AM -0600, Tim Van Patten wrote: > > On Wed, Aug 17, 2022 at 10:08 PM Tzung-Bi Shih <tzungbi@kernel.org> wrote: > > > > > > On Tue, Aug 02, 2022 at 11:40:08AM -0600, Tim Van Patten wrote: > > > > Update cros_ec_lpc_pm_ops to call cros_ec_lpc_prepare() during PM > > > > .prepare() and cros_ec_lpc_complete() during .complete(). This allows the > > > > EC to log entry/exit of AP's suspend/resume more accurately. > > > > > > As what I commented on [1], the term "host event" in the commit title is > > > confusing. Also, as this is a cros_ec_lpc specific patch, please change > > > the prefix. > > > > I've updated the prefix to "cros_ec_lpc" and the title/description to > > indicate that this CL moves when the host event is sent to > > .prepare()/.complete(). > > > > > [1]: https://patchwork.kernel.org/project/chrome-platform/patch/20220706205136.v2.1.Ic7a7c81f880ab31533652e0928aa6e687bb268b5@changeid/#24934911 > > I'm not sure if any unclear. "host event" is a terminology for CrOS EC. > The usage here is confusing. I updated the title/description to "host command". > > > > > > > > -static int cros_ec_lpc_resume(struct device *dev) > > > > +static void cros_ec_lpc_complete(struct device *dev) > > > > { > > > > struct cros_ec_device *ec_dev = dev_get_drvdata(dev); > > > > + int ret; > > > > + > > > > + ret = cros_ec_resume(ec_dev); > > > > > > > > - return cros_ec_resume(ec_dev); > > > > + dev_info(dev, "EC resume completed: ret = %d\n", ret); > > > > > > cros_ec_resume() always returns 0. > > > > Yes, it always returns 0 today, but that may not be the case forever. > > While "ret" is not returned by cros_ec_resume() today, it's possible > > for it to be non-zero and someone may update cros_ec_resume() to > > return that status. > > Does it really need to print if `ret` is always 0? Yes, we should always print the return value in case it changes in the future.
diff --git a/drivers/platform/chrome/cros_ec_lpc.c b/drivers/platform/chrome/cros_ec_lpc.c index 7677ab3c0ead9..4158bdeee197b 100644 --- a/drivers/platform/chrome/cros_ec_lpc.c +++ b/drivers/platform/chrome/cros_ec_lpc.c @@ -530,23 +530,31 @@ static const struct dmi_system_id cros_ec_lpc_dmi_table[] __initconst = { MODULE_DEVICE_TABLE(dmi, cros_ec_lpc_dmi_table); #ifdef CONFIG_PM_SLEEP -static int cros_ec_lpc_suspend(struct device *dev) +static int cros_ec_lpc_prepare(struct device *dev) { struct cros_ec_device *ec_dev = dev_get_drvdata(dev); + dev_info(dev, "Prepare EC suspend\n"); + return cros_ec_suspend(ec_dev); } -static int cros_ec_lpc_resume(struct device *dev) +static void cros_ec_lpc_complete(struct device *dev) { struct cros_ec_device *ec_dev = dev_get_drvdata(dev); + int ret; + + ret = cros_ec_resume(ec_dev); - return cros_ec_resume(ec_dev); + dev_info(dev, "EC resume completed: ret = %d\n", ret); } #endif static const struct dev_pm_ops cros_ec_lpc_pm_ops = { - SET_LATE_SYSTEM_SLEEP_PM_OPS(cros_ec_lpc_suspend, cros_ec_lpc_resume) +#ifdef CONFIG_PM_SLEEP + .prepare = cros_ec_lpc_prepare, + .complete = cros_ec_lpc_complete +#endif }; static struct platform_driver cros_ec_lpc_driver = {
Update cros_ec_lpc_pm_ops to call cros_ec_lpc_prepare() during PM .prepare() and cros_ec_lpc_complete() during .complete(). This allows the EC to log entry/exit of AP's suspend/resume more accurately. Signed-off-by: Tim Van Patten <timvp@google.com> --- Changes in v3: - Update cros_ec_lpc_suspend() to cros_ec_lpc_prepare() - Update cros_ec_lpc_resume() to cros_ec_lpc_complete() Changes in v2: - Include cros_ec_resume() return value in dev_info() output. - Guard setting .prepare/.complete with #ifdef CONFIG_PM_SLEEP. drivers/platform/chrome/cros_ec_lpc.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)