Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20190607210528.248042-1-evgreen@chromium.org> <797d3c44-cf30-678d-2622-6bd4a2e89b70@collabora.com>
In-Reply-To: <797d3c44-cf30-678d-2622-6bd4a2e89b70@collabora.com>
From:   Evan Green <evgreen@chromium.org>
Date:   Thu, 13 Jun 2019 13:56:03 -0700
Message-ID: <CAE=gft427=fMGatFUkkOHpBvJa3n=Usi09aToqu=69bJZ_0uTQ@mail.gmail.com>
Subject: Re: [PATCH] platform/chrome: Expose resume result via sysfs
To:     Enric Balletbo i Serra <enric.balletbo@collabora.com>
Cc:     Lee Jones <lee.jones@linaro.org>,
        Ravi Chandra Sadineni <ravisadineni@chromium.org>,
        Rajat Jain <rajatja@chromium.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Guenter Roeck <groeck@chromium.org>,
        Benson Leung <bleung@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Jun 12, 2019 at 9:37 AM Enric Balletbo i Serra
<enric.balletbo@collabora.com> wrote:
>
> Hi Evan,
>
> On 7/6/19 23:05, Evan Green wrote:
> > For ECs that support it, the EC returns the number of slp_s0
> > transitions and whether or not there was a timeout in the resume
> > response. Expose the last resume result to usermode via sysfs so
> > that usermode can detect and report S0ix timeouts.
>
> Looks more for a debugfs attribute instead of sysfs?
>
> I'd prefer have it in debugfs unless you have a good reason. As you probably
> know I'm not a big fan of having private interfaces and I'd like to maintain the
> minimum needed in sysfs.

That seems fine with me. It's easy enough to move it from debugfs to
sysfs later if needed, but much more difficult to go the other way.
I'll spin it for debugfs.

>
> >
> > Signed-off-by: Evan Green <evgreen@chromium.org>
> > ---
> >
> > Enric, I'm not sure if this conflicts with your giant refactoring.
> > I can rebase this on your series, but patchwork doesn't have patch
> > 2 of your series, so you'd have to point me to a tree.
> >
>
> Probably, but it's fine for now, so don't worry about it. The refactoring still
> needs more reviews from other people.

Ok. I'll base it on linux-next.

>
> > ---
> >  drivers/mfd/cros_ec.c                   |  6 +++++-
> >  drivers/platform/chrome/cros_ec_sysfs.c | 17 +++++++++++++++++
> >  include/linux/mfd/cros_ec.h             |  1 +
> >  3 files changed, 23 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
> > index bd2bcdd4718b..64a2d3adc729 100644
> > --- a/drivers/mfd/cros_ec.c
> > +++ b/drivers/mfd/cros_ec.c
> > @@ -110,12 +110,16 @@ static int cros_ec_sleep_event(struct cros_ec_device *ec_dev, u8 sleep_event)
> >
> >       /* For now, report failure to transition to S0ix with a warning. */
> >       if (ret >= 0 && ec_dev->host_sleep_v1 &&
> > -         (sleep_event == HOST_SLEEP_EVENT_S0IX_RESUME))
> > +         (sleep_event == HOST_SLEEP_EVENT_S0IX_RESUME)) {
> > +             ec_dev->last_resume_result =
> > +                     buf.u.resp1.resume_response.sleep_transitions;
> > +
> >               WARN_ONCE(buf.u.resp1.resume_response.sleep_transitions &
> >                         EC_HOST_RESUME_SLEEP_TIMEOUT,
> >                         "EC detected sleep transition timeout. Total slp_s0 transitions: %d",
> >                         buf.u.resp1.resume_response.sleep_transitions &
> >                         EC_HOST_RESUME_SLEEP_TRANSITIONS_MASK);
> > +     }
> >
> >       return ret;
> >  }
> > diff --git a/drivers/platform/chrome/cros_ec_sysfs.c b/drivers/platform/chrome/cros_ec_sysfs.c
> > index 3edb237bf8ed..2deca98c7a7a 100644
> > --- a/drivers/platform/chrome/cros_ec_sysfs.c
> > +++ b/drivers/platform/chrome/cros_ec_sysfs.c
> > @@ -308,18 +308,35 @@ static ssize_t kb_wake_angle_store(struct device *dev,
> >       return count;
> >  }
> >
> > +/* Last resume result */
> > +static ssize_t last_resume_result_show(struct device *dev,
> > +                                    struct device_attribute *attr, char *buf)
> > +{
> > +     struct cros_ec_dev *ec = to_cros_ec_dev(dev);
> > +     int ret;
> > +
> > +     ret = scnprintf(buf,
> > +                     PAGE_SIZE,
> > +                     "0x%x\n",
> > +                     ec->ec_dev->last_resume_result);
> > +
> > +     return ret;
> > +}
> > +
> >  /* Module initialization */
> >
> >  static DEVICE_ATTR_RW(reboot);
> >  static DEVICE_ATTR_RO(version);
> >  static DEVICE_ATTR_RO(flashinfo);
> >  static DEVICE_ATTR_RW(kb_wake_angle);
> > +static DEVICE_ATTR_RO(last_resume_result);
> >
>
> The attribute should be documented in
>
> Documentation/ABI/testing/sysfs-class-chromeos
>
> or
>
> Documentation/ABI/testing/debugfs-cros-ec ; yes I know this file does not exist,
> but we should add it :-)

Ok. Thanks for taking a look.
-Evan