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: <CABXOdTdvMKGHoE5JQBLca_6V+ihRiUedgnyb2or8GraBDsKYWQ@mail.gmail.com>
 <20190305005300.129464-1-ravisadineni@chromium.org> <404dd7fe-23de-e5c4-48c2-5cd77a14e72b@collabora.com>
In-Reply-To: <404dd7fe-23de-e5c4-48c2-5cd77a14e72b@collabora.com>
From:   Ravi Chandra Sadineni <ravisadineni@chromium.org>
Date:   Fri, 8 Mar 2019 17:46:32 -0800
Message-ID: <CAEZbON6xzRnxSLB8r-KGEtWWB2nv33eT7QTWwFwt3Hf+xPFYpw@mail.gmail.com>
Subject: Re: [PATCH V3] cros_ec: Expose sysfile to force battery cut-off on shutdown.
To:     Enric Balletbo i Serra <enric.balletbo@collabora.com>
Cc:     Benson Leung <bleung@chromium.org>,
        Guenter Roeck <groeck@chromium.org>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        Todd Broch <tbroch@google.com>,
        Daisuke Nojiri <dnojiri@google.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Enric,

Thanks for the review.

On Thu, Mar 7, 2019 at 3:57 AM Enric Balletbo i Serra
<enric.balletbo@collabora.com> wrote:
>
> Hi RaviChandra,
>
> Some few comments below ...
>
>
> On 5/3/19 1:53, RaviChandra Sadineni wrote:
> > On chromebooks, power_manager daemon normally shutsdown(S5) the device
> > when the battery charge falls below 4% threshold. Chromeos EC then
> > normally spends an hour in S5 before hibernating. If the battery charge
> > falls below critical threshold in the mean time, EC does a battery cutoff
>
> Be coherent using the same spelling along all the patch for cutoff.
Done.
>
>
> > instead of hibernating. On some chromebooks, S5 is optimal enough
> > resulting in EC hibernating without battery cut-off. This results in
>
> s/cut-off/cutoff/
Done.
>
> > battery deep-discharging. This is a bad user experience as battery
> > has to trickle charge before booting when the A.C is plugged in the next
> > time.
> >
> > This patch exposes a sysfs file for an userland daemon to suggest EC if it
> > has to do a battery cut-off instead of hibernating when the system enters
>
> s/cut-off/cutoff/
Done.
>
> > S5 next time.
> >
> > Signed-off-by: RaviChandra Sadineni <ravisadineni@chromium.org>
> > ---
> >
> > V3: Make battery-cutoff generic and expose 'at-shutdown' flag.
> > V2: Use kstrtobool() instead of kstrtou8() and add documentation.
> >
> >
> > .../ABI/testing/sysfs-class-chromeos          | 10 ++++
> >  drivers/platform/chrome/cros_ec_sysfs.c       | 48 +++++++++++++++++++
> >  2 files changed, 58 insertions(+)
> >  create mode 100644 Documentation/ABI/testing/sysfs-class-chromeos
> >
> > diff --git a/Documentation/ABI/testing/sysfs-class-chromeos b/Documentation/ABI/testing/sysfs-class-chromeos
> > new file mode 100644
> > index 000000000000..d5ab22c44977
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-class-chromeos
>
> Please rebase the patch on top of chrome-platform for-next [1] or 5.1-rc1 when
> released, so it applies cleanly.
Done.
>
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/chrome-platform/linux.git/log/?h=for-next
>
> > @@ -0,0 +1,10 @@
> > +What:           /sys/class/chromeos/cros_ec/battery-cuttoff
>
> The name should match with the attribute name, so battery_cutoff in this case as
> you defined the attribute name as battery_cutoff
>
> Is this a common property for all the EC devices?
>
> Note that we have
>
> /sys/class/chromeos/<ec-device-name>/...
>
> where <ec-device-name> can be cros_ec|cros_pd and many others in the future
>
> On those devices that more than one EC is present we will have. E.g
>
>  cros_ec/battery_cutoff and cros_pd/battery_cutoff
>  cros_ec/battery_cutoff and cros_ish/battery_cutoff
>
> Which I think is not what you want. Ideally I'd like to see visible the
> attribute only on those ECs that support that feature. Is this command
> associated with an EC feature?
Yes with EC_FEATURE_BATTERY.  Now checking if EC supports EC_FEATURE_BATTERY.
>
> If that's not possible I can assume writing to battery_cutoff for ECs that don't
> implement it would return a protocol/command error.

>
>
> > +Date:           February 2019
> > +Contact:        Ravi Chandra Sadineni <ravisadineni@chromium.org>
> > +Description:
> > +                cros_ec battery cuttoff configuration. Only option
> > +                currently exposed is 'at-shutdown'.
> > +
> > +                'at-shutdown' sends a host command to EC requesting
> > +                battery cutoff on next shutdown. If AC is plugged
> > +                in before next shutdown, EC ignores the request.
> > diff --git a/drivers/platform/chrome/cros_ec_sysfs.c b/drivers/platform/chrome/cros_ec_sysfs.c
> > index f34a50121064..6ef6b860c818 100644
> > --- a/drivers/platform/chrome/cros_ec_sysfs.c
> > +++ b/drivers/platform/chrome/cros_ec_sysfs.c
> > @@ -322,14 +322,62 @@ static ssize_t kb_wake_angle_store(struct device *dev,
> >       return count;
> >  }
> >
> > +/* Battery cut-off control */
>
> s/cut-off/cutoff/
Done.
>
> > +static ssize_t battery_cutoff_store(struct device *dev,
> > +                                 struct device_attribute *attr,
> > +                                 const char *buf, size_t count)
> > +{
> > +     struct ec_params_battery_cutoff *param;
> > +     struct cros_ec_command *msg;
> > +     int ret;
> > +     struct cros_ec_dev *ec =
> > +             container_of(dev, struct cros_ec_dev, class_dev);
>
> use to_cros_ec_dev instead of container_of
>
> > +     char *p;
> > +     int len;
> > +
> > +     msg = kmalloc(sizeof(*msg) + EC_HOST_PARAM_SIZE, GFP_KERNEL);
> > +     if (!msg)
> > +             return -ENOMEM;
> > +
> > +     param = (struct ec_params_battery_cutoff *)msg->data;
> > +     msg->command = EC_CMD_BATTERY_CUT_OFF + ec->cmd_offset;
> > +     msg->version = 1;
> > +     msg->outsize = sizeof(*param);
> > +     msg->insize = 0;
> > +
> > +     p = memchr(buf, '\n', count);
> > +     len = p ? p - buf : count;
> > +
> > +     if (len == 11 && !strncmp(buf, "at-shutdown", len)) {
> > +             param->flags = EC_BATTERY_CUTOFF_FLAG_AT_SHUTDOWN;
> > +     } else {
> > +             kfree(msg);
>
> Use only one kfree, please. Remove this.
>
> > +             return -EINVAL;
>
>                 count = -EINVAL;
>                 goto exit;
>
> > +     }
> > +
> > +     ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
> > +     kfree(msg);
>
> Only one point for kfree, please. Remove this.
>
> > +     if (ret < 0)
> > +             return ret;
>             count = ret;
>
> exit:
>         kfree(msg);
> > +     return count;
> > +}
> > +
> >  /* Module initialization */
> >
> >  static DEVICE_ATTR_RW(reboot);
> >  static DEVICE_ATTR_RO(version);
> >  static DEVICE_ATTR_RO(flashinfo);
> >  static DEVICE_ATTR_RW(kb_wake_angle);
> > +/*
> > + * Currently EC does not expose a host command to read the status of
> > + * battery cut-off configuration. Also there is no requirement to read
>
> s/cut-off/cutoff/
>
> > + * the status of these flags from userland. So marking this attribute as
> > + *  write-only.
> > + */
>
> Can you also specify that is WO in the sysfs documentation. I understand
> correctly that this command only applies when the device runs on battery? And
> that if AC is plugged the flag is reset?
Done.


Thanks,
Ravi


>
> > +static DEVICE_ATTR_WO(battery_cutoff);
> >
>
> Note that the name should match with the sysfs documentation.
>
> >  static struct attribute *__ec_attrs[] = {
> > +     &dev_attr_battery_cutoff.attr,
> >       &dev_attr_kb_wake_angle.attr,
> >       &dev_attr_reboot.attr,
> >       &dev_attr_version.attr,
> >
>
> Thanks,
>  Enric