Date:   Mon, 17 Dec 2018 17:45:47 +0100
From:   Enric Balletbo Serra <eballetbo@...il.com>
To:     Nick Crews <ncrews@...gle.com>
Cc:     linux-kernel <linux-kernel@...r.kernel.org>,
        Duncan Laurie <dlaurie@...gle.com>,
        Olof Johansson <olof@...om.net>,
        Benson Leung <bleung@...omium.org>,
        Guenter Roeck <groeck@...omium.org>
Subject: Re: [RFC PATCH 03/10] CHROMIUM: wilco_ec: Add sysfs attributes

Hi Nick,

Many thanks for sending this upstream. Adding Guenter as also might be
interested, and some comments and questions below.

Missatge de Nick Crews <ncrews@...gle.com> del dia ds., 15 de des.
2018 a les 1:20:
>
> From: Duncan Laurie <dlaurie@...gle.com>
>
> Add some sample sysfs attributes for the Wilco EC that show how
> the mailbox interface works.
>
> > cat /sys/bus/platform/devices/GOOG000C\:00/version
> Label        : 99.99.99
> SVN Revision : 738ed.99
> Model Number : 08;8
> Build Date   : 08/30/18
>
> Signed-off-by: Duncan Laurie <dlaurie@...gle.com>
> Signed-off-by: Nick Crews <ncrews@...gle.com>
> ---
>
>  drivers/platform/chrome/Makefile           |   3 +-
>  drivers/platform/chrome/wilco_ec.h         |  14 +++
>  drivers/platform/chrome/wilco_ec_mailbox.c |  12 ++
>  drivers/platform/chrome/wilco_ec_sysfs.c   | 121 +++++++++++++++++++++

I am wondering if this should have its own driver (like we recently
did for cros_ec) or if it should be integrated into the wilco core.
Also, I am wondering if this should be attached to the cros_ec_class
or not ( /sys/class/chromeos/wilco_ec ? ). Guenter and Benson, any
opinion here?

Anyway, you should add the documentation of the sysfs entries, please.

>  4 files changed, 148 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/platform/chrome/wilco_ec_sysfs.c
>
> diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile
> index b132ba5b3e3d..e8603bc5b095 100644
> --- a/drivers/platform/chrome/Makefile
> +++ b/drivers/platform/chrome/Makefile
> @@ -13,6 +13,5 @@ cros_ec_lpcs-$(CONFIG_CROS_EC_LPC_MEC)        += cros_ec_lpc_mec.o
>  obj-$(CONFIG_CROS_EC_LPC)              += cros_ec_lpcs.o
>  obj-$(CONFIG_CROS_EC_PROTO)            += cros_ec_proto.o
>
> -obj-$(CONFIG_CROS_KBD_LED_BACKLIGHT)   += cros_kbd_led_backlight.o

I guess that you don't want to do this :)

> -wilco_ec-objs                          := wilco_ec_mailbox.o
> +wilco_ec-objs                          := wilco_ec_mailbox.o wilco_ec_sysfs.o
>  obj-$(CONFIG_WILCO_EC)                 += wilco_ec.o
> diff --git a/drivers/platform/chrome/wilco_ec.h b/drivers/platform/chrome/wilco_ec.h
> index ba16fcff87c4..699f4cf744dc 100644
> --- a/drivers/platform/chrome/wilco_ec.h
> +++ b/drivers/platform/chrome/wilco_ec.h
> @@ -94,4 +94,18 @@ struct wilco_ec_message {
>   */
>  int wilco_ec_mailbox(struct wilco_ec_device *ec, struct wilco_ec_message *msg);
>
> +/**
> + * wilco_ec_sysfs_init() - Create sysfs attributes.
> + * @ec: EC device.
> + *
> + * Return: 0 for success or negative error code on failure.
> + */
> +int wilco_ec_sysfs_init(struct wilco_ec_device *ec);
> +
> +/**
> + * wilco_ec_sysfs_remove() - Remove sysfs attributes.
> + * @ec: EC device.
> + */
> +void wilco_ec_sysfs_remove(struct wilco_ec_device *ec);
> +
>  #endif /* WILCO_EC_H */
> diff --git a/drivers/platform/chrome/wilco_ec_mailbox.c b/drivers/platform/chrome/wilco_ec_mailbox.c
> index 6613c18c2a82..414ea0a8ad03 100644
> --- a/drivers/platform/chrome/wilco_ec_mailbox.c
> +++ b/drivers/platform/chrome/wilco_ec_mailbox.c
> @@ -361,11 +361,23 @@ static int wilco_ec_probe(struct platform_device *pdev)
>         cros_ec_lpc_mec_init(ec->io_packet->start,
>                              ec->io_packet->start + EC_MAILBOX_DATA_SIZE);
>
> +       /* Create sysfs attributes for userspace interaction */
> +       if (wilco_ec_sysfs_init(ec) < 0) {
> +               dev_err(dev, "Failed to create sysfs attributes\n");
> +               cros_ec_lpc_mec_destroy();
> +               return -ENODEV;
> +       }
> +
>         return 0;
>  }
>
>  static int wilco_ec_remove(struct platform_device *pdev)
>  {
> +       struct wilco_ec_device *ec = platform_get_drvdata(pdev);
> +
> +       /* Remove sysfs attributes */
> +       wilco_ec_sysfs_remove(ec);
> +
>         /* Teardown cros_ec interface */
>         cros_ec_lpc_mec_destroy();
>
> diff --git a/drivers/platform/chrome/wilco_ec_sysfs.c b/drivers/platform/chrome/wilco_ec_sysfs.c
> new file mode 100644
> index 000000000000..f9ae6cef6169
> --- /dev/null
> +++ b/drivers/platform/chrome/wilco_ec_sysfs.c
> @@ -0,0 +1,121 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * wilco_ec_sysfs - Sysfs attributes for Wilco Embedded Controller

Remove 'wilco_ec_sysfs -'.

> + *
> + * Copyright 2018 Google LLC
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.

Remove the license boiler-plate.

> + */
> +
> +#include <linux/ctype.h>
> +#include <linux/platform_device.h>
> +#include <linux/sysfs.h>
> +#include "wilco_ec.h"
> +
> +#define EC_COMMAND_EC_INFO             0x38
> +#define EC_INFO_SIZE                    9
> +#define EC_COMMAND_STEALTH_MODE                0xfc
> +
> +struct ec_info {
> +       u8 index;
> +       const char *label;
> +};
> +
> +static ssize_t wilco_ec_show_info(struct wilco_ec_device *ec, char *buf,
> +                                 ssize_t count, struct ec_info *info)
> +{
> +       char result[EC_INFO_SIZE];
> +       struct wilco_ec_message msg = {
> +               .type = WILCO_EC_MSG_LEGACY,
> +               .command = EC_COMMAND_EC_INFO,
> +               .request_data = &info->index,
> +               .request_size = sizeof(info->index),
> +               .response_data = result,
> +               .response_size = EC_INFO_SIZE,
> +       };
> +       int ret;
> +
> +       ret = wilco_ec_mailbox(ec, &msg);
> +       if (ret != EC_INFO_SIZE)
> +               return scnprintf(buf + count, PAGE_SIZE - count,
> +                                "%-12s : ERROR %d\n", info->label, ret);
> +
> +       return scnprintf(buf + count, PAGE_SIZE - count,
> +                        "%-12s : %s\n", info->label, result);
> +}
> +
> +static ssize_t version_show(struct device *dev,
> +                           struct device_attribute *attr, char *buf)
> +{
> +       struct wilco_ec_device *ec = dev_get_drvdata(dev);
> +       struct ec_info wilco_ec_info[] = {
> +               { 0, "Label" },
> +               { 1, "SVN Revision" },
> +               { 2, "Model Number" },
> +               { 3, "Build Date" },
> +               { 0xff, NULL },
> +       };
> +       struct ec_info *info = wilco_ec_info;
> +       ssize_t c = 0;
> +
> +       for (info = wilco_ec_info; info->label; info++)
> +               c += wilco_ec_show_info(ec, buf, c, info);
> +
> +       return c;
> +}
> +
> +static ssize_t stealth_mode_store(struct device *dev,
> +                                 struct device_attribute *attr,
> +                                 const char *buf, size_t count)
> +{
> +       struct wilco_ec_device *ec = dev_get_drvdata(dev);
> +       u8 param;
> +       struct wilco_ec_message msg = {
> +               .type = WILCO_EC_MSG_LEGACY,
> +               .command = EC_COMMAND_STEALTH_MODE,
> +               .request_data = &param,
> +               .request_size = sizeof(param),
> +       };
> +       int ret;
> +       bool enable;
> +
> +       ret = kstrtobool(buf, &enable);
> +       if (ret)
> +               return ret;
> +
> +       /* Invert input parameter, EC expects 0=on and 1=off */
> +       param = enable ? 0 : 1;
> +
> +       ret = wilco_ec_mailbox(ec, &msg);
> +       if (ret < 0)
> +               return ret;
> +
> +       return count;
> +}
> +
> +static DEVICE_ATTR_RO(version);
> +static DEVICE_ATTR_WO(stealth_mode);
> +
> +static struct attribute *wilco_ec_attrs[] = {
> +       &dev_attr_version.attr,
> +       &dev_attr_stealth_mode.attr,
> +       NULL
> +};
> +ATTRIBUTE_GROUPS(wilco_ec);
> +
> +int wilco_ec_sysfs_init(struct wilco_ec_device *ec)
> +{
> +       return sysfs_create_groups(&ec->dev->kobj, wilco_ec_groups);
> +}
> +
> +void wilco_ec_sysfs_remove(struct wilco_ec_device *ec)
> +{
> +       sysfs_remove_groups(&ec->dev->kobj, wilco_ec_groups);
> +}
> --
> 2.20.0.405.gbc1bbc6f85-goog
>

Thanks,
  Enric

Powered by blists - more mailing lists