[v3,1/2] platform/chrome: wilco_ec: Add Boot on AC support
diff mbox series

Message ID 20190417012048.87977-1-ncrews@chromium.org
State New, archived
Headers show
Series
  • [v3,1/2] platform/chrome: wilco_ec: Add Boot on AC support
Related show

Commit Message

Nick Crews April 17, 2019, 1:20 a.m. UTC
Boot on AC is a policy which makes the device boot from S5 when AC
power is connected. This is useful for users who want to run their
device headless or with a dock.

v3 changes:
- Add docstring to wilco_ec_add_sysfs()
- Tweak a comment
- Use val > 1 instead of val != 0 && val != 1
v2 changes:
- Move documentation to Documentation/ABI/testing/sysfs-platform-wilco-ec

Signed-off-by: Nick Crews <ncrews@chromium.org>
---
 .../ABI/testing/sysfs-platform-wilco-ec       | 11 +++
 drivers/platform/chrome/wilco_ec/Makefile     |  2 +-
 drivers/platform/chrome/wilco_ec/core.c       |  9 +++
 drivers/platform/chrome/wilco_ec/sysfs.c      | 77 +++++++++++++++++++
 include/linux/platform_data/wilco-ec.h        | 12 +++
 5 files changed, 110 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/ABI/testing/sysfs-platform-wilco-ec
 create mode 100644 drivers/platform/chrome/wilco_ec/sysfs.c

Comments

Nick Crews April 18, 2019, 3:42 p.m. UTC | #1
There's one error that Guenter just found...

> +
> +int wilco_ec_add_sysfs(struct wilco_ec_device *ec)
> +{
> +       return sysfs_create_group(&ec->dev->kobj, &wilco_dev_attr_group);
> +}
> +
> +void wilco_ec_remove_sysfs(struct wilco_ec_device *ec)
> +{
> +       sysfs_create_group(&ec->dev->kobj, &wilco_dev_attr_group);

Should be sysfs_remove_group()
Nick Crews May 1, 2019, 5:06 p.m. UTC | #2
Hi Enric,

Are these two patches an acceptable use of sysfs? There were concerns
earlier about abusing sysfs, but I think that these two uses follow
other sysfs use-cases well.

Thanks,
Nick
Enric Balletbo Serra May 9, 2019, 9:31 p.m. UTC | #3
Hi Nick,

Thanks for the patch, some few comments below...

Missatge de Nick Crews <ncrews@chromium.org> del dia dc., 17 d’abr.
2019 a les 3:22:
>
> Boot on AC is a policy which makes the device boot from S5 when AC
> power is connected. This is useful for users who want to run their
> device headless or with a dock.
>
> v3 changes:
> - Add docstring to wilco_ec_add_sysfs()
> - Tweak a comment
> - Use val > 1 instead of val != 0 && val != 1
> v2 changes:
> - Move documentation to Documentation/ABI/testing/sysfs-platform-wilco-ec
>
> Signed-off-by: Nick Crews <ncrews@chromium.org>
> ---
>  .../ABI/testing/sysfs-platform-wilco-ec       | 11 +++
>  drivers/platform/chrome/wilco_ec/Makefile     |  2 +-
>  drivers/platform/chrome/wilco_ec/core.c       |  9 +++
>  drivers/platform/chrome/wilco_ec/sysfs.c      | 77 +++++++++++++++++++
>  include/linux/platform_data/wilco-ec.h        | 12 +++
>  5 files changed, 110 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/ABI/testing/sysfs-platform-wilco-ec
>  create mode 100644 drivers/platform/chrome/wilco_ec/sysfs.c
>
> diff --git a/Documentation/ABI/testing/sysfs-platform-wilco-ec b/Documentation/ABI/testing/sysfs-platform-wilco-ec
> new file mode 100644
> index 000000000000..e074c203cd32
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-platform-wilco-ec
> @@ -0,0 +1,11 @@
> +What:          /sys/bus/platform/devices/GOOG000C\:00/boot_on_ac
> +Date:          April 2019
> +KernelVersion: 5.2
> +Description:
> +               Boot on AC is a policy which makes the device boot from S5
> +               when AC power is connected. This is useful for users who
> +               want to run their device headless or with a dock.
> +
> +               Input should be parseable by kstrtou8() to 0 or 1.
> +               Output will be either "0\n" or "1\n".
> +
> diff --git a/drivers/platform/chrome/wilco_ec/Makefile b/drivers/platform/chrome/wilco_ec/Makefile
> index 063e7fb4ea17..1281dd7737c4 100644
> --- a/drivers/platform/chrome/wilco_ec/Makefile
> +++ b/drivers/platform/chrome/wilco_ec/Makefile
> @@ -1,6 +1,6 @@
>  # SPDX-License-Identifier: GPL-2.0
>
> -wilco_ec-objs                          := core.o mailbox.o
> +wilco_ec-objs                          := core.o mailbox.o sysfs.o
>  obj-$(CONFIG_WILCO_EC)                 += wilco_ec.o
>  wilco_ec_debugfs-objs                  := debugfs.o
>  obj-$(CONFIG_WILCO_EC_DEBUGFS)         += wilco_ec_debugfs.o
> diff --git a/drivers/platform/chrome/wilco_ec/core.c b/drivers/platform/chrome/wilco_ec/core.c
> index 05e1e2be1c91..abd15d04e57b 100644
> --- a/drivers/platform/chrome/wilco_ec/core.c
> +++ b/drivers/platform/chrome/wilco_ec/core.c
> @@ -89,8 +89,16 @@ static int wilco_ec_probe(struct platform_device *pdev)
>                 goto unregister_debugfs;
>         }
>
> +       ret = wilco_ec_add_sysfs(ec);
> +       if (ret < 0) {
> +               dev_err(dev, "Failed to create sysfs entries: %d", ret);
> +               goto unregister_rtc;
> +       }
> +
>         return 0;
>
> +unregister_rtc:
> +       platform_device_unregister(ec->rtc_pdev);
>  unregister_debugfs:
>         if (ec->debugfs_pdev)
>                 platform_device_unregister(ec->debugfs_pdev);
> @@ -102,6 +110,7 @@ static int wilco_ec_remove(struct platform_device *pdev)
>  {
>         struct wilco_ec_device *ec = platform_get_drvdata(pdev);
>
> +       wilco_ec_remove_sysfs(ec);
>         platform_device_unregister(ec->rtc_pdev);
>         if (ec->debugfs_pdev)
>                 platform_device_unregister(ec->debugfs_pdev);
> diff --git a/drivers/platform/chrome/wilco_ec/sysfs.c b/drivers/platform/chrome/wilco_ec/sysfs.c
> new file mode 100644
> index 000000000000..959b5da2eb16
> --- /dev/null
> +++ b/drivers/platform/chrome/wilco_ec/sysfs.c
> @@ -0,0 +1,77 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2019 Google LLC
> + *
> + * Sysfs properties to view and modify EC-controlled features on Wilco devices.
> + * The entries will appear under /sys/bus/platform/devices/GOOG000C:00/
> + *
> + * See Documentation/ABI/testing/sysfs-platform-wilco-ec for more information.
> + */
> +
> +#include <linux/platform_data/wilco-ec.h>
> +#include <linux/sysfs.h>
> +
> +#define CMD_KB_CMOS                    0x7C
> +#define SUB_CMD_KB_CMOS_AUTO_ON                0x03
> +
> +struct boot_on_ac_request {
> +       u8 cmd;                 /* Always CMD_KB_CMOS */
> +       u8 reserved1;
> +       u8 sub_cmd;             /* Always SUB_CMD_KB_CMOS_AUTO_ON */
> +       u8 reserved3to5[3];
> +       u8 val;                 /* Either 0 or 1 */
> +       u8 reserved7;
> +} __packed;
> +
> +static ssize_t boot_on_ac_store(struct device *dev,
> +                               struct device_attribute *attr,
> +                               const char *buf, size_t count)
> +{
> +       struct wilco_ec_device *ec = dev_get_drvdata(dev);
> +       struct boot_on_ac_request rq;
> +       struct wilco_ec_message msg;
> +       int ret;
> +       u8 val;
> +
> +       ret = kstrtou8(buf, 10, &val);
> +       if (ret < 0)
> +               return ret;
> +       if (val > 1)
> +               return -EINVAL;
> +
> +       memset(&rq, 0, sizeof(rq));
> +       rq.cmd = CMD_KB_CMOS;
> +       rq.sub_cmd = SUB_CMD_KB_CMOS_AUTO_ON;
> +       rq.val = val;
> +
> +       memset(&msg, 0, sizeof(msg));
> +       msg.type = WILCO_EC_MSG_LEGACY;
> +       msg.request_data = &rq;
> +       msg.request_size = sizeof(rq);
> +       ret = wilco_ec_mailbox(ec, &msg);
> +       if (ret < 0)
> +               return ret;
> +
> +       return count;
> +}
> +
> +static DEVICE_ATTR_WO(boot_on_ac);

Is not possible to read the flag? From the API description seems that it is.

> +
> +static struct attribute *wilco_dev_attrs[] = {
> +       &dev_attr_boot_on_ac.attr,
> +       NULL,
> +};
> +
> +static struct attribute_group wilco_dev_attr_group = {
> +       .attrs = wilco_dev_attrs,
> +};
> +
> +int wilco_ec_add_sysfs(struct wilco_ec_device *ec)
> +{
> +       return sysfs_create_group(&ec->dev->kobj, &wilco_dev_attr_group);
> +}
> +
> +void wilco_ec_remove_sysfs(struct wilco_ec_device *ec)
> +{
> +       sysfs_create_group(&ec->dev->kobj, &wilco_dev_attr_group);

As Guenter pointed:  sysfs_remove_group()

> +}
> diff --git a/include/linux/platform_data/wilco-ec.h b/include/linux/platform_data/wilco-ec.h
> index 1ff224793c99..7809b098ce27 100644
> --- a/include/linux/platform_data/wilco-ec.h
> +++ b/include/linux/platform_data/wilco-ec.h
> @@ -123,4 +123,16 @@ struct wilco_ec_message {
>   */
>  int wilco_ec_mailbox(struct wilco_ec_device *ec, struct wilco_ec_message *msg);
>
> +/**
> + * wilco_ec_add_sysfs() - Create sysfs entries
> + * @ec: Wilco EC device
> + *
> + * wilco_ec_remove_sysfs() needs to be called afterwards
> + * to perform the necessary cleanup.
> + *
> + * Return: 0 on success or negative error code on failure.
> + */
> +int wilco_ec_add_sysfs(struct wilco_ec_device *ec);
> +void wilco_ec_remove_sysfs(struct wilco_ec_device *ec);
> +
>  #endif /* WILCO_EC_H */
> --
> 2.20.1
>
Nick Crews May 10, 2019, 4:09 p.m. UTC | #4
Thanks for the review Enric!

I can resend the patch with the fixes, or if you think the fixes are
simple enough, you could tweak them as you apply them. Let
me know if you want me to resend a clean version.

> > +
> > +static DEVICE_ATTR_WO(boot_on_ac);
>
> Is not possible to read the flag? From the API description seems that it is.

It is not possible to read the flag. The API description is wrong,
I'll fix remove
the line about reading from the documentation.

> > +void wilco_ec_remove_sysfs(struct wilco_ec_device *ec)
> > +{
> > +       sysfs_create_group(&ec->dev->kobj, &wilco_dev_attr_group);
>
> As Guenter pointed:  sysfs_remove_group()

Yes, exactly.
Enric Balletbo i Serra May 15, 2019, 1 p.m. UTC | #5
Hi,

On 10/5/19 18:09, Nick Crews wrote:
> Thanks for the review Enric!
> 
> I can resend the patch with the fixes, or if you think the fixes are
> simple enough, you could tweak them as you apply them. Let
> me know if you want me to resend a clean version.
> 

No need to resend, I tweaked them.

The patch is queued to the for-next branch for the autobuilders to play with, if
all goes well I'll add the patch for 5.3 when current merge window closes.

Thanks,
 Enric


>>> +
>>> +static DEVICE_ATTR_WO(boot_on_ac);
>>
>> Is not possible to read the flag? From the API description seems that it is.
> 
> It is not possible to read the flag. The API description is wrong,
> I'll fix remove
> the line about reading from the documentation.
> 
>>> +void wilco_ec_remove_sysfs(struct wilco_ec_device *ec)
>>> +{
>>> +       sysfs_create_group(&ec->dev->kobj, &wilco_dev_attr_group);
>>
>> As Guenter pointed:  sysfs_remove_group()
> 
> Yes, exactly.
>

Patch
diff mbox series

diff --git a/Documentation/ABI/testing/sysfs-platform-wilco-ec b/Documentation/ABI/testing/sysfs-platform-wilco-ec
new file mode 100644
index 000000000000..e074c203cd32
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-platform-wilco-ec
@@ -0,0 +1,11 @@ 
+What:		/sys/bus/platform/devices/GOOG000C\:00/boot_on_ac
+Date:		April 2019
+KernelVersion:	5.2
+Description:
+		Boot on AC is a policy which makes the device boot from S5
+		when AC power is connected. This is useful for users who
+		want to run their device headless or with a dock.
+
+		Input should be parseable by kstrtou8() to 0 or 1.
+		Output will be either "0\n" or "1\n".
+
diff --git a/drivers/platform/chrome/wilco_ec/Makefile b/drivers/platform/chrome/wilco_ec/Makefile
index 063e7fb4ea17..1281dd7737c4 100644
--- a/drivers/platform/chrome/wilco_ec/Makefile
+++ b/drivers/platform/chrome/wilco_ec/Makefile
@@ -1,6 +1,6 @@ 
 # SPDX-License-Identifier: GPL-2.0
 
-wilco_ec-objs				:= core.o mailbox.o
+wilco_ec-objs				:= core.o mailbox.o sysfs.o
 obj-$(CONFIG_WILCO_EC)			+= wilco_ec.o
 wilco_ec_debugfs-objs			:= debugfs.o
 obj-$(CONFIG_WILCO_EC_DEBUGFS)		+= wilco_ec_debugfs.o
diff --git a/drivers/platform/chrome/wilco_ec/core.c b/drivers/platform/chrome/wilco_ec/core.c
index 05e1e2be1c91..abd15d04e57b 100644
--- a/drivers/platform/chrome/wilco_ec/core.c
+++ b/drivers/platform/chrome/wilco_ec/core.c
@@ -89,8 +89,16 @@  static int wilco_ec_probe(struct platform_device *pdev)
 		goto unregister_debugfs;
 	}
 
+	ret = wilco_ec_add_sysfs(ec);
+	if (ret < 0) {
+		dev_err(dev, "Failed to create sysfs entries: %d", ret);
+		goto unregister_rtc;
+	}
+
 	return 0;
 
+unregister_rtc:
+	platform_device_unregister(ec->rtc_pdev);
 unregister_debugfs:
 	if (ec->debugfs_pdev)
 		platform_device_unregister(ec->debugfs_pdev);
@@ -102,6 +110,7 @@  static int wilco_ec_remove(struct platform_device *pdev)
 {
 	struct wilco_ec_device *ec = platform_get_drvdata(pdev);
 
+	wilco_ec_remove_sysfs(ec);
 	platform_device_unregister(ec->rtc_pdev);
 	if (ec->debugfs_pdev)
 		platform_device_unregister(ec->debugfs_pdev);
diff --git a/drivers/platform/chrome/wilco_ec/sysfs.c b/drivers/platform/chrome/wilco_ec/sysfs.c
new file mode 100644
index 000000000000..959b5da2eb16
--- /dev/null
+++ b/drivers/platform/chrome/wilco_ec/sysfs.c
@@ -0,0 +1,77 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2019 Google LLC
+ *
+ * Sysfs properties to view and modify EC-controlled features on Wilco devices.
+ * The entries will appear under /sys/bus/platform/devices/GOOG000C:00/
+ *
+ * See Documentation/ABI/testing/sysfs-platform-wilco-ec for more information.
+ */
+
+#include <linux/platform_data/wilco-ec.h>
+#include <linux/sysfs.h>
+
+#define CMD_KB_CMOS			0x7C
+#define SUB_CMD_KB_CMOS_AUTO_ON		0x03
+
+struct boot_on_ac_request {
+	u8 cmd;			/* Always CMD_KB_CMOS */
+	u8 reserved1;
+	u8 sub_cmd;		/* Always SUB_CMD_KB_CMOS_AUTO_ON */
+	u8 reserved3to5[3];
+	u8 val;			/* Either 0 or 1 */
+	u8 reserved7;
+} __packed;
+
+static ssize_t boot_on_ac_store(struct device *dev,
+				struct device_attribute *attr,
+				const char *buf, size_t count)
+{
+	struct wilco_ec_device *ec = dev_get_drvdata(dev);
+	struct boot_on_ac_request rq;
+	struct wilco_ec_message msg;
+	int ret;
+	u8 val;
+
+	ret = kstrtou8(buf, 10, &val);
+	if (ret < 0)
+		return ret;
+	if (val > 1)
+		return -EINVAL;
+
+	memset(&rq, 0, sizeof(rq));
+	rq.cmd = CMD_KB_CMOS;
+	rq.sub_cmd = SUB_CMD_KB_CMOS_AUTO_ON;
+	rq.val = val;
+
+	memset(&msg, 0, sizeof(msg));
+	msg.type = WILCO_EC_MSG_LEGACY;
+	msg.request_data = &rq;
+	msg.request_size = sizeof(rq);
+	ret = wilco_ec_mailbox(ec, &msg);
+	if (ret < 0)
+		return ret;
+
+	return count;
+}
+
+static DEVICE_ATTR_WO(boot_on_ac);
+
+static struct attribute *wilco_dev_attrs[] = {
+	&dev_attr_boot_on_ac.attr,
+	NULL,
+};
+
+static struct attribute_group wilco_dev_attr_group = {
+	.attrs = wilco_dev_attrs,
+};
+
+int wilco_ec_add_sysfs(struct wilco_ec_device *ec)
+{
+	return sysfs_create_group(&ec->dev->kobj, &wilco_dev_attr_group);
+}
+
+void wilco_ec_remove_sysfs(struct wilco_ec_device *ec)
+{
+	sysfs_create_group(&ec->dev->kobj, &wilco_dev_attr_group);
+}
diff --git a/include/linux/platform_data/wilco-ec.h b/include/linux/platform_data/wilco-ec.h
index 1ff224793c99..7809b098ce27 100644
--- a/include/linux/platform_data/wilco-ec.h
+++ b/include/linux/platform_data/wilco-ec.h
@@ -123,4 +123,16 @@  struct wilco_ec_message {
  */
 int wilco_ec_mailbox(struct wilco_ec_device *ec, struct wilco_ec_message *msg);
 
+/**
+ * wilco_ec_add_sysfs() - Create sysfs entries
+ * @ec: Wilco EC device
+ *
+ * wilco_ec_remove_sysfs() needs to be called afterwards
+ * to perform the necessary cleanup.
+ *
+ * Return: 0 on success or negative error code on failure.
+ */
+int wilco_ec_add_sysfs(struct wilco_ec_device *ec);
+void wilco_ec_remove_sysfs(struct wilco_ec_device *ec);
+
 #endif /* WILCO_EC_H */