[PATCH 0/7] mfd / platform: cros_ec: move cros_ec sysfs attributes to its own drivers.

[PATCH 0/7] mfd / platform: cros_ec: move cros_ec sysfs attributes to its own drivers.

[Enric Balletbo i Serra]

Hi,

This is another patchset to try to cleanup a bit more the crossed
references for cros-ec driver between the MFD and the platform/chrome
subsystems.

The purpose of these patches is get rid of the different cros-ec attributes
from mfd/cros_ec_dev to its own sub-driver in platform/chrome. cros_ec_dev
continues instantiating the sub-devices but the sysfs attributes are owned
by the platform driver.E.g. The lightbar driver should own his sysfs
attributes and be instantiated only if the Embedded Controller has a
lightbar.

The patchset also adds the documentation of the sysfs attributes.

Waiting for your feedback. Best regards,
 Enric


Enric Balletbo i Serra (7):
  mfd: cros_ec: use devm_mfd_add_devices.
  mfd / platform: cros_ec: move lightbar attributes to its own driver.
  mfd / platform: cros_ec: move vbc attributes to its own driver.
  mfd / platform: cros_ec: move debugfs attributes to its own driver.
  mfd / platform: cros_ec: move device sysfs attributes to its own
    driver.
  mfd / platform: cros_ec: instantiate only if th EC has a VBC NVRAM.
  platform/chrome: cros_ec_lightbar: instantiate only if the EC has a
    lightbar.

 .../ABI/testing/sysfs-class-chromeos          |  32 +++++
 ...sfs-class-chromeos-driver-cros-ec-lightbar |  74 ++++++++++
 .../sysfs-class-chromeos-driver-cros-ec-vbc   |   6 +
 drivers/mfd/Kconfig                           |   1 -
 drivers/mfd/cros_ec.c                         |  12 +-
 drivers/mfd/cros_ec_dev.c                     | 115 ++++++++--------
 drivers/mfd/cros_ec_dev.h                     |   6 -
 drivers/platform/chrome/Kconfig               |  44 +++++-
 drivers/platform/chrome/Makefile              |   7 +-
 drivers/platform/chrome/cros_ec_debugfs.c     |  49 +++++--
 drivers/platform/chrome/cros_ec_i2c.c         |  10 --
 drivers/platform/chrome/cros_ec_lightbar.c    | 127 ++++++++++++------
 drivers/platform/chrome/cros_ec_lpc.c         |   4 -
 drivers/platform/chrome/cros_ec_spi.c         |  11 --
 drivers/platform/chrome/cros_ec_sysfs.c       |  43 +++++-
 drivers/platform/chrome/cros_ec_vbc.c         |  62 ++++++---
 include/linux/mfd/cros_ec.h                   |  15 +--
 17 files changed, 434 insertions(+), 184 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-class-chromeos
 create mode 100644 Documentation/ABI/testing/sysfs-class-chromeos-driver-cros-ec-lightbar
 create mode 100644 Documentation/ABI/testing/sysfs-class-chromeos-driver-cros-ec-vbc

-- 
2.19.1

[PATCH 1/7] mfd: cros_ec: use devm_mfd_add_devices.

[Enric Balletbo i Serra]

Use devm_mfd_add_devices() for adding MFD child devices. This reduces
the need of remove callback for removing MFD child devices.

Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---

 drivers/mfd/cros_ec.c                 | 12 ++----------
 drivers/mfd/cros_ec_dev.c             |  8 ++++----
 drivers/platform/chrome/cros_ec_i2c.c | 10 ----------
 drivers/platform/chrome/cros_ec_lpc.c |  4 ----
 drivers/platform/chrome/cros_ec_spi.c | 11 -----------
 5 files changed, 6 insertions(+), 39 deletions(-)

diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
index fe6f83766144..5f18e279bfc3 100644
--- a/drivers/mfd/cros_ec.c
+++ b/drivers/mfd/cros_ec.c
@@ -129,7 +129,7 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
 		}
 	}
 
-	err = mfd_add_devices(ec_dev->dev, PLATFORM_DEVID_AUTO, &ec_cell, 1,
+	err = devm_mfd_add_devices(ec_dev->dev, PLATFORM_DEVID_AUTO, &ec_cell, 1,
 			      NULL, ec_dev->irq, NULL);
 	if (err) {
 		dev_err(dev,
@@ -147,7 +147,7 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
 		 * - the EC is responsive at init time (it is not true for a
 		 *   sensor hub.
 		 */
-		err = mfd_add_devices(ec_dev->dev, PLATFORM_DEVID_AUTO,
+		err = devm_mfd_add_devices(ec_dev->dev, PLATFORM_DEVID_AUTO,
 				      &ec_pd_cell, 1, NULL, ec_dev->irq, NULL);
 		if (err) {
 			dev_err(dev,
@@ -181,14 +181,6 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
 }
 EXPORT_SYMBOL(cros_ec_register);
 
-int cros_ec_remove(struct cros_ec_device *ec_dev)
-{
-	mfd_remove_devices(ec_dev->dev);
-
-	return 0;
-}
-EXPORT_SYMBOL(cros_ec_remove);
-
 #ifdef CONFIG_PM_SLEEP
 int cros_ec_suspend(struct cros_ec_device *ec_dev)
 {
diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
index 8f9d6964173e..3cde77dd17f6 100644
--- a/drivers/mfd/cros_ec_dev.c
+++ b/drivers/mfd/cros_ec_dev.c
@@ -366,7 +366,7 @@ static void cros_ec_sensors_register(struct cros_ec_dev *ec)
 		id++;
 	}
 
-	ret = mfd_add_devices(ec->dev, 0, sensor_cells, id,
+	ret = devm_mfd_add_devices(ec->dev, 0, sensor_cells, id,
 			      NULL, 0, NULL);
 	if (ret)
 		dev_err(ec->dev, "failed to add EC sensors\n");
@@ -430,7 +430,7 @@ static int ec_device_probe(struct platform_device *pdev)
 
 	/* Check whether this EC instance has CEC host command support */
 	if (cros_ec_check_features(ec, EC_FEATURE_CEC)) {
-		retval = mfd_add_devices(ec->dev, PLATFORM_DEVID_AUTO,
+		retval = devm_mfd_add_devices(ec->dev, PLATFORM_DEVID_AUTO,
 					 cros_ec_cec_cells,
 					 ARRAY_SIZE(cros_ec_cec_cells),
 					 NULL, 0, NULL);
@@ -442,7 +442,7 @@ static int ec_device_probe(struct platform_device *pdev)
 
 	/* Check whether this EC instance has RTC host command support */
 	if (cros_ec_check_features(ec, EC_FEATURE_RTC)) {
-		retval = mfd_add_devices(ec->dev, PLATFORM_DEVID_AUTO,
+		retval = devm_mfd_add_devices(ec->dev, PLATFORM_DEVID_AUTO,
 					 cros_ec_rtc_cells,
 					 ARRAY_SIZE(cros_ec_rtc_cells),
 					 NULL, 0, NULL);
@@ -454,7 +454,7 @@ static int ec_device_probe(struct platform_device *pdev)
 
 	/* Check whether this EC instance has the PD charge manager */
 	if (cros_ec_check_features(ec, EC_FEATURE_USB_PD)) {
-		retval = mfd_add_devices(ec->dev, PLATFORM_DEVID_AUTO,
+		retval = devm_mfd_add_devices(ec->dev, PLATFORM_DEVID_AUTO,
 					 cros_usbpd_charger_cells,
 					 ARRAY_SIZE(cros_usbpd_charger_cells),
 					 NULL, 0, NULL);
diff --git a/drivers/platform/chrome/cros_ec_i2c.c b/drivers/platform/chrome/cros_ec_i2c.c
index ef9b4763356f..9a009eaa4ada 100644
--- a/drivers/platform/chrome/cros_ec_i2c.c
+++ b/drivers/platform/chrome/cros_ec_i2c.c
@@ -317,15 +317,6 @@ static int cros_ec_i2c_probe(struct i2c_client *client,
 	return 0;
 }
 
-static int cros_ec_i2c_remove(struct i2c_client *client)
-{
-	struct cros_ec_device *ec_dev = i2c_get_clientdata(client);
-
-	cros_ec_remove(ec_dev);
-
-	return 0;
-}
-
 #ifdef CONFIG_PM_SLEEP
 static int cros_ec_i2c_suspend(struct device *dev)
 {
@@ -376,7 +367,6 @@ static struct i2c_driver cros_ec_driver = {
 		.pm	= &cros_ec_i2c_pm_ops,
 	},
 	.probe		= cros_ec_i2c_probe,
-	.remove		= cros_ec_i2c_remove,
 	.id_table	= cros_ec_i2c_id,
 };
 
diff --git a/drivers/platform/chrome/cros_ec_lpc.c b/drivers/platform/chrome/cros_ec_lpc.c
index e1b75775cd4a..14684a56e40f 100644
--- a/drivers/platform/chrome/cros_ec_lpc.c
+++ b/drivers/platform/chrome/cros_ec_lpc.c
@@ -327,7 +327,6 @@ static int cros_ec_lpc_probe(struct platform_device *pdev)
 
 static int cros_ec_lpc_remove(struct platform_device *pdev)
 {
-	struct cros_ec_device *ec_dev;
 	struct acpi_device *adev;
 
 	adev = ACPI_COMPANION(&pdev->dev);
@@ -335,9 +334,6 @@ static int cros_ec_lpc_remove(struct platform_device *pdev)
 		acpi_remove_notify_handler(adev->handle, ACPI_ALL_NOTIFY,
 					   cros_ec_lpc_acpi_notify);
 
-	ec_dev = platform_get_drvdata(pdev);
-	cros_ec_remove(ec_dev);
-
 	return 0;
 }
 
diff --git a/drivers/platform/chrome/cros_ec_spi.c b/drivers/platform/chrome/cros_ec_spi.c
index 2060d1483043..6cfbc2835beb 100644
--- a/drivers/platform/chrome/cros_ec_spi.c
+++ b/drivers/platform/chrome/cros_ec_spi.c
@@ -685,16 +685,6 @@ static int cros_ec_spi_probe(struct spi_device *spi)
 	return 0;
 }
 
-static int cros_ec_spi_remove(struct spi_device *spi)
-{
-	struct cros_ec_device *ec_dev;
-
-	ec_dev = spi_get_drvdata(spi);
-	cros_ec_remove(ec_dev);
-
-	return 0;
-}
-
 #ifdef CONFIG_PM_SLEEP
 static int cros_ec_spi_suspend(struct device *dev)
 {
@@ -733,7 +723,6 @@ static struct spi_driver cros_ec_driver_spi = {
 		.pm	= &cros_ec_spi_pm_ops,
 	},
 	.probe		= cros_ec_spi_probe,
-	.remove		= cros_ec_spi_remove,
 	.id_table	= cros_ec_spi_id,
 };
 
-- 
2.19.1

[PATCH 2/7] mfd / platform: cros_ec: move lightbar attributes to its own driver.

[Enric Balletbo i Serra]

The entire way how cros sysfs attibutes are created is broken.
cros_ec_lightbar should be its own driver and its attributes should be
associated with a lightbar driver not the mfd driver. In order to retain
the path, the lightbar attributes are attached to the cros_class.

The patch also adds the sysfs documentation.

Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---

 ...sfs-class-chromeos-driver-cros-ec-lightbar |  74 +++++++++++++
 drivers/mfd/cros_ec_dev.c                     |  37 +++++--
 drivers/mfd/cros_ec_dev.h                     |   6 --
 drivers/platform/chrome/Kconfig               |  10 ++
 drivers/platform/chrome/Makefile              |   3 +-
 drivers/platform/chrome/cros_ec_lightbar.c    | 100 ++++++++++++++----
 include/linux/mfd/cros_ec.h                   |   7 +-
 7 files changed, 196 insertions(+), 41 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-class-chromeos-driver-cros-ec-lightbar

diff --git a/Documentation/ABI/testing/sysfs-class-chromeos-driver-cros-ec-lightbar b/Documentation/ABI/testing/sysfs-class-chromeos-driver-cros-ec-lightbar
new file mode 100644
index 000000000000..4893fd2e956f
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-chromeos-driver-cros-ec-lightbar
@@ -0,0 +1,74 @@
+What:		/sys/class/chromeos/<cros-ec>/lightbar/brightness
+Date:		August 2015
+KernelVersion:	4.2
+Description:
+		Writing to this file adjusts the overall brightness of
+		the lightbar, separate from any color intensity. The
+		valid range is 0 (off) to 255 (maximum brightness).
+
+What:		/sys/class/chromeos/<cros-ec>/lightbar/interval_msec
+Date:		August 2015
+KernelVersion:	4.2
+Description:
+		The lightbar is controlled by an embedded controller (EC),
+		which also manages the keyboard, battery charging, fans,
+		and other system hardware. To prevent unprivileged users
+		from interfering with the other EC functions, the rate at
+		which the lightbar control files can be read or written is
+		limited.
+
+		Reading this file will return the number of milliseconds
+		that must elapse between accessing any of the lightbar
+		functions through this interface. Going faster will simply
+		block until the necessary interval has lapsed. The interval
+		applies uniformly to all accesses of any kind by any user.
+
+What:		/sys/class/chromeos/<cros-ec>/lightbar/led_rgb
+Date:		August 2015
+KernelVersion:	4.2
+Description:
+		This allows you to control each LED segment. If the
+		lightbar is already running one of the automatic
+		sequences, you probably won’t see anything change because
+		your color setting will be almost immediately replaced.
+		To get useful results, you should stop the lightbar
+		sequence first.
+
+		The values written to this file are sets of four integers,
+		indicating LED, RED, GREEN, BLUE. The LED number is 0 to 3
+		to select a single segment, or 4 to set all four segments
+		to the same value at once. The RED, GREEN, and BLUE
+		numbers should be in the range 0 (off) to 255 (maximum).
+		You can update more than one segment at a time by writing
+		more than one set of four integers.
+
+What:		/sys/class/chromeos/<cros-ec>/lightbar/program
+Date:		August 2015
+KernelVersion:	4.2
+Description:
+		This allows you to upload and run custom lightbar sequences.
+
+What:		/sys/class/chromeos/<cros-ec>/lightbar/sequence
+Date:		August 2015
+KernelVersion:	4.2
+Description:
+		The Pixel lightbar has a number of built-in sequences
+		that it displays under various conditions, such as at
+		power on, shut down, or while running. Reading from this
+		file displays the current sequence that the lightbar is
+		displaying. Writing to this file allows you to change the
+		sequence.
+
+What:		/sys/class/chromeos/<cros-ec>/lightbar/userspace_control
+Date:		August 2015
+KernelVersion:	4.2
+Description:
+		This allows you to take the control of the lightbar. This
+		prevents the kernel from going through its normal
+		sequences.
+
+What:		/sys/class/chromeos/<cros-ec>/lightbar/version
+Date:		August 2015
+KernelVersion:	4.2
+Description:
+		Show the information about the lightbar version.
diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
index 3cde77dd17f6..9bdd42561701 100644
--- a/drivers/mfd/cros_ec_dev.c
+++ b/drivers/mfd/cros_ec_dev.c
@@ -36,7 +36,6 @@ static int ec_major;
 
 static const struct attribute_group *cros_ec_groups[] = {
 	&cros_ec_attr_group,
-	&cros_ec_lightbar_attr_group,
 	&cros_ec_vbc_attr_group,
 	NULL,
 };
@@ -47,6 +46,20 @@ static struct class cros_class = {
 	.dev_groups     = cros_ec_groups,
 };
 
+int cros_ec_attach_attribute_group(struct cros_ec_dev *ec,
+				   struct attribute_group *attrs)
+{
+	return sysfs_create_group(&ec->class_dev.kobj, attrs);
+}
+EXPORT_SYMBOL(cros_ec_attach_attribute_group);
+
+void cros_ec_detach_attribute_group(struct cros_ec_dev *ec,
+				    struct attribute_group *attrs)
+{
+	sysfs_remove_group(&ec->class_dev.kobj, attrs);
+}
+EXPORT_SYMBOL(cros_ec_detach_attribute_group);
+
 /* Basic communication */
 static int ec_get_version(struct cros_ec_dev *ec, char *str, int maxlen)
 {
@@ -390,6 +403,10 @@ static const struct mfd_cell cros_usbpd_charger_cells[] = {
 	{ .name = "cros-usbpd-charger" }
 };
 
+static const struct mfd_cell cros_ec_platform_cells[] = {
+	{ .name = "cros-ec-lightbar" },
+};
+
 static int ec_device_probe(struct platform_device *pdev)
 {
 	int retval = -ENOMEM;
@@ -464,9 +481,6 @@ static int ec_device_probe(struct platform_device *pdev)
 				retval);
 	}
 
-	/* Take control of the lightbar from the EC. */
-	lb_manual_suspend_ctrl(ec, 1);
-
 	/* We can now add the sysfs class, we know which parameter to show */
 	retval = cdev_device_add(&ec->cdev, &ec->class_dev);
 	if (retval) {
@@ -474,6 +488,14 @@ static int ec_device_probe(struct platform_device *pdev)
 		goto failed;
 	}
 
+	retval = devm_mfd_add_devices(ec->dev, PLATFORM_DEVID_AUTO,
+				      cros_ec_platform_cells,
+				      ARRAY_SIZE(cros_ec_platform_cells),
+				      NULL, 0, NULL);
+	if (retval)
+		dev_err(ec->dev,
+			"failed to add cros-ec platform devices: %d\n", retval);
+
 	if (cros_ec_debugfs_init(ec))
 		dev_warn(dev, "failed to create debugfs directory\n");
 
@@ -488,9 +510,6 @@ static int ec_device_remove(struct platform_device *pdev)
 {
 	struct cros_ec_dev *ec = dev_get_drvdata(&pdev->dev);
 
-	/* Let the EC take over the lightbar again. */
-	lb_manual_suspend_ctrl(ec, 0);
-
 	cros_ec_debugfs_remove(ec);
 
 	cdev_del(&ec->cdev);
@@ -518,8 +537,6 @@ static __maybe_unused int ec_device_suspend(struct device *dev)
 
 	cros_ec_debugfs_suspend(ec);
 
-	lb_suspend(ec);
-
 	return 0;
 }
 
@@ -529,8 +546,6 @@ static __maybe_unused int ec_device_resume(struct device *dev)
 
 	cros_ec_debugfs_resume(ec);
 
-	lb_resume(ec);
-
 	return 0;
 }
 
diff --git a/drivers/mfd/cros_ec_dev.h b/drivers/mfd/cros_ec_dev.h
index 978d836a0248..ec750433455a 100644
--- a/drivers/mfd/cros_ec_dev.h
+++ b/drivers/mfd/cros_ec_dev.h
@@ -44,10 +44,4 @@ struct cros_ec_readmem {
 #define CROS_EC_DEV_IOCXCMD   _IOWR(CROS_EC_DEV_IOC, 0, struct cros_ec_command)
 #define CROS_EC_DEV_IOCRDMEM  _IOWR(CROS_EC_DEV_IOC, 1, struct cros_ec_readmem)
 
-/* Lightbar utilities */
-extern bool ec_has_lightbar(struct cros_ec_dev *ec);
-extern int lb_manual_suspend_ctrl(struct cros_ec_dev *ec, uint8_t enable);
-extern int lb_suspend(struct cros_ec_dev *ec);
-extern int lb_resume(struct cros_ec_dev *ec);
-
 #endif /* _CROS_EC_DEV_H_ */
diff --git a/drivers/platform/chrome/Kconfig b/drivers/platform/chrome/Kconfig
index 16b1615958aa..05bcefbe1926 100644
--- a/drivers/platform/chrome/Kconfig
+++ b/drivers/platform/chrome/Kconfig
@@ -111,4 +111,14 @@ config CROS_KBD_LED_BACKLIGHT
 	  To compile this driver as a module, choose M here: the
 	  module will be called cros_kbd_led_backlight.
 
+config CROS_EC_LIGHTBAR
+	tristate "Chromebook Pixel's lightbar support"
+	depends on MFD_CROS_EC_CHARDEV
+	help
+	  This option exposes the Chromebook Pixel's lightbar to
+	  userspace.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called cros_ec_lightbar.
+
 endif # CHROMEOS_PLATFORMS
diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile
index cd591bf872bb..3c29a4b405d5 100644
--- a/drivers/platform/chrome/Makefile
+++ b/drivers/platform/chrome/Makefile
@@ -3,7 +3,7 @@
 obj-$(CONFIG_CHROMEOS_LAPTOP)		+= chromeos_laptop.o
 obj-$(CONFIG_CHROMEOS_PSTORE)		+= chromeos_pstore.o
 obj-$(CONFIG_CHROMEOS_TBMC)		+= chromeos_tbmc.o
-cros_ec_ctl-objs			:= cros_ec_sysfs.o cros_ec_lightbar.o \
+cros_ec_ctl-objs			:= cros_ec_sysfs.o \
 					   cros_ec_vbc.o cros_ec_debugfs.o
 obj-$(CONFIG_CROS_EC_CTL)		+= cros_ec_ctl.o
 obj-$(CONFIG_CROS_EC_I2C)		+= cros_ec_i2c.o
@@ -13,3 +13,4 @@ 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
+obj-$(CONFIG_CROS_EC_LIGHTBAR)		+= cros_ec_lightbar.o
diff --git a/drivers/platform/chrome/cros_ec_lightbar.c b/drivers/platform/chrome/cros_ec_lightbar.c
index 68193bb53383..31d22f594fac 100644
--- a/drivers/platform/chrome/cros_ec_lightbar.c
+++ b/drivers/platform/chrome/cros_ec_lightbar.c
@@ -33,6 +33,8 @@
 #include <linux/uaccess.h>
 #include <linux/slab.h>
 
+#define DRV_NAME "cros-ec-lightbar"
+
 /* Rate-limit the lightbar interface to prevent DoS. */
 static unsigned long lb_interval_jiffies = 50 * HZ / 1000;
 
@@ -373,7 +375,7 @@ static int lb_send_empty_cmd(struct cros_ec_dev *ec, uint8_t cmd)
 	return ret;
 }
 
-int lb_manual_suspend_ctrl(struct cros_ec_dev *ec, uint8_t enable)
+static int lb_manual_suspend_ctrl(struct cros_ec_dev *ec, uint8_t enable)
 {
 	struct ec_params_lightbar *param;
 	struct cros_ec_command *msg;
@@ -408,25 +410,6 @@ int lb_manual_suspend_ctrl(struct cros_ec_dev *ec, uint8_t enable)
 
 	return ret;
 }
-EXPORT_SYMBOL(lb_manual_suspend_ctrl);
-
-int lb_suspend(struct cros_ec_dev *ec)
-{
-	if (userspace_control || ec != ec_with_lightbar)
-		return 0;
-
-	return lb_send_empty_cmd(ec, LIGHTBAR_CMD_SUSPEND);
-}
-EXPORT_SYMBOL(lb_suspend);
-
-int lb_resume(struct cros_ec_dev *ec)
-{
-	if (userspace_control || ec != ec_with_lightbar)
-		return 0;
-
-	return lb_send_empty_cmd(ec, LIGHTBAR_CMD_RESUME);
-}
-EXPORT_SYMBOL(lb_resume);
 
 static ssize_t sequence_store(struct device *dev, struct device_attribute *attr,
 			      const char *buf, size_t count)
@@ -584,7 +567,7 @@ static struct attribute *__lb_cmds_attrs[] = {
 	NULL,
 };
 
-bool ec_has_lightbar(struct cros_ec_dev *ec)
+static bool ec_has_lightbar(struct cros_ec_dev *ec)
 {
 	return !!get_lightbar_version(ec, NULL, NULL);
 }
@@ -616,4 +599,77 @@ struct attribute_group cros_ec_lightbar_attr_group = {
 	.attrs = __lb_cmds_attrs,
 	.is_visible = cros_ec_lightbar_attrs_are_visible,
 };
-EXPORT_SYMBOL(cros_ec_lightbar_attr_group);
+
+static int cros_ec_lightbar_probe(struct platform_device *pd)
+{
+	struct cros_ec_dev *ec_dev = dev_get_drvdata(pd->dev.parent);
+	struct device *dev = &pd->dev;
+	int ret;
+
+	if (!ec_dev) {
+		dev_err(dev, "No EC dev found\n");
+		return -EINVAL;
+	}
+
+	/* Take control of the lightbar from the EC. */
+	lb_manual_suspend_ctrl(ec_dev, 1);
+
+	ret = cros_ec_attach_attribute_group(ec_dev,
+					     &cros_ec_lightbar_attr_group);
+	if (ret < 0)
+		dev_err(dev, "failed to create %s attributes. err=%d\n",
+			cros_ec_lightbar_attr_group.name, ret);
+
+	return ret;
+}
+
+static int cros_ec_lightbar_remove(struct platform_device *pd)
+{
+	struct cros_ec_dev *ec_dev = dev_get_drvdata(pd->dev.parent);
+
+	cros_ec_detach_attribute_group(ec_dev,
+				       &cros_ec_lightbar_attr_group);
+
+	/* Let the EC take over the lightbar again. */
+	lb_manual_suspend_ctrl(ec_dev, 0);
+
+	return 0;
+}
+
+static int __maybe_unused cros_ec_lightbar_resume(struct device *dev)
+{
+	struct cros_ec_dev *ec_dev = dev_get_drvdata(dev);
+
+	if (userspace_control || ec_dev != ec_with_lightbar)
+		return 0;
+
+	return lb_send_empty_cmd(ec_dev, LIGHTBAR_CMD_RESUME);
+}
+
+static int __maybe_unused cros_ec_lightbar_suspend(struct device *dev)
+{
+	struct cros_ec_dev *ec_dev = dev_get_drvdata(dev);
+
+	if (userspace_control || ec_dev != ec_with_lightbar)
+		return 0;
+
+	return lb_send_empty_cmd(ec_dev, LIGHTBAR_CMD_SUSPEND);
+}
+
+static SIMPLE_DEV_PM_OPS(cros_ec_lightbar_pm_ops,
+			 cros_ec_lightbar_suspend, cros_ec_lightbar_resume);
+
+static struct platform_driver cros_ec_lightbar_driver = {
+	.driver = {
+		.name = DRV_NAME,
+		.pm = &cros_ec_lightbar_pm_ops,
+	},
+	.probe = cros_ec_lightbar_probe,
+	.remove = cros_ec_lightbar_remove,
+};
+
+module_platform_driver(cros_ec_lightbar_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Expose the Chromebook Pixel's lightbar to userspace");
+MODULE_ALIAS("platform:" DRV_NAME);
diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
index e44e3ec8a9c7..4431e46c5875 100644
--- a/include/linux/mfd/cros_ec.h
+++ b/include/linux/mfd/cros_ec.h
@@ -335,7 +335,6 @@ u32 cros_ec_get_host_event(struct cros_ec_device *ec_dev);
 
 /* sysfs stuff */
 extern struct attribute_group cros_ec_attr_group;
-extern struct attribute_group cros_ec_lightbar_attr_group;
 extern struct attribute_group cros_ec_vbc_attr_group;
 
 /* debugfs stuff */
@@ -344,4 +343,10 @@ void cros_ec_debugfs_remove(struct cros_ec_dev *ec);
 void cros_ec_debugfs_suspend(struct cros_ec_dev *ec);
 void cros_ec_debugfs_resume(struct cros_ec_dev *ec);
 
+/* Attach/detach attributes to the cros_class */
+extern int cros_ec_attach_attribute_group(struct cros_ec_dev *ec,
+					  struct attribute_group *attrs);
+extern void cros_ec_detach_attribute_group(struct cros_ec_dev *ec,
+					   struct attribute_group *attrs);
+
 #endif /* __LINUX_MFD_CROS_EC_H */
-- 
2.19.1

[PATCH 3/7] mfd / platform: cros_ec: move vbc attributes to its own driver.

[Enric Balletbo i Serra]

The entire way how cros sysfs attibutes are created is broken.
cros_ec_vbc should be its own driver and its attributes should be
associated with a vbc driver not the mfd driver. In order to retain
the path, the vbc attributes are attached to the cros_class.

The patch also adds the sysfs documentation.

Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---

 .../sysfs-class-chromeos-driver-cros-ec-vbc   |  6 +++
 drivers/mfd/cros_ec_dev.c                     |  2 +-
 drivers/platform/chrome/Kconfig               | 10 ++++
 drivers/platform/chrome/Makefile              |  3 +-
 drivers/platform/chrome/cros_ec_vbc.c         | 46 ++++++++++++++++++-
 include/linux/mfd/cros_ec.h                   |  1 -
 6 files changed, 64 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-class-chromeos-driver-cros-ec-vbc

diff --git a/Documentation/ABI/testing/sysfs-class-chromeos-driver-cros-ec-vbc b/Documentation/ABI/testing/sysfs-class-chromeos-driver-cros-ec-vbc
new file mode 100644
index 000000000000..c256a2febf37
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-chromeos-driver-cros-ec-vbc
@@ -0,0 +1,6 @@
+What:		/sys/class/chromeos/<cros-ec>/vbc/vboot_context
+Date:		October 2015
+KernelVersion:	4.4
+Description:
+		Read/write the verified boot context data included on a
+		small nvram space on some EC implementations.
diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
index 9bdd42561701..790dc28b93f0 100644
--- a/drivers/mfd/cros_ec_dev.c
+++ b/drivers/mfd/cros_ec_dev.c
@@ -36,7 +36,6 @@ static int ec_major;
 
 static const struct attribute_group *cros_ec_groups[] = {
 	&cros_ec_attr_group,
-	&cros_ec_vbc_attr_group,
 	NULL,
 };
 
@@ -405,6 +404,7 @@ static const struct mfd_cell cros_usbpd_charger_cells[] = {
 
 static const struct mfd_cell cros_ec_platform_cells[] = {
 	{ .name = "cros-ec-lightbar" },
+	{ .name = "cros-ec-vbc" },
 };
 
 static int ec_device_probe(struct platform_device *pdev)
diff --git a/drivers/platform/chrome/Kconfig b/drivers/platform/chrome/Kconfig
index 05bcefbe1926..29bd9837d520 100644
--- a/drivers/platform/chrome/Kconfig
+++ b/drivers/platform/chrome/Kconfig
@@ -121,4 +121,14 @@ config CROS_EC_LIGHTBAR
 	  To compile this driver as a module, choose M here: the
 	  module will be called cros_ec_lightbar.
 
+config CROS_EC_VBC
+	tristate "ChromeOS EC vboot context support"
+	depends on MFD_CROS_EC_CHARDEV && OF
+	help
+	  This option exposes the ChromeOS EC vboot context nvram to
+	  userspace.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called cros_ec_vbc.
+
 endif # CHROMEOS_PLATFORMS
diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile
index 3c29a4b405d5..4081b7179df7 100644
--- a/drivers/platform/chrome/Makefile
+++ b/drivers/platform/chrome/Makefile
@@ -4,7 +4,7 @@ obj-$(CONFIG_CHROMEOS_LAPTOP)		+= chromeos_laptop.o
 obj-$(CONFIG_CHROMEOS_PSTORE)		+= chromeos_pstore.o
 obj-$(CONFIG_CHROMEOS_TBMC)		+= chromeos_tbmc.o
 cros_ec_ctl-objs			:= cros_ec_sysfs.o \
-					   cros_ec_vbc.o cros_ec_debugfs.o
+					   cros_ec_debugfs.o
 obj-$(CONFIG_CROS_EC_CTL)		+= cros_ec_ctl.o
 obj-$(CONFIG_CROS_EC_I2C)		+= cros_ec_i2c.o
 obj-$(CONFIG_CROS_EC_SPI)		+= cros_ec_spi.o
@@ -14,3 +14,4 @@ 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
 obj-$(CONFIG_CROS_EC_LIGHTBAR)		+= cros_ec_lightbar.o
+obj-$(CONFIG_CROS_EC_VBC)		+= cros_ec_vbc.o
diff --git a/drivers/platform/chrome/cros_ec_vbc.c b/drivers/platform/chrome/cros_ec_vbc.c
index 5356f26bc022..374153e3dc13 100644
--- a/drivers/platform/chrome/cros_ec_vbc.c
+++ b/drivers/platform/chrome/cros_ec_vbc.c
@@ -22,8 +22,11 @@
 #include <linux/platform_device.h>
 #include <linux/mfd/cros_ec.h>
 #include <linux/mfd/cros_ec_commands.h>
+#include <linux/module.h>
 #include <linux/slab.h>
 
+#define DRV_NAME "cros-ec-vbc"
+
 static ssize_t vboot_context_read(struct file *filp, struct kobject *kobj,
 				  struct bin_attribute *att, char *buf,
 				  loff_t pos, size_t count)
@@ -132,4 +135,45 @@ struct attribute_group cros_ec_vbc_attr_group = {
 	.bin_attrs = cros_ec_vbc_bin_attrs,
 	.is_bin_visible = cros_ec_vbc_is_visible,
 };
-EXPORT_SYMBOL(cros_ec_vbc_attr_group);
+
+static int cros_ec_vbc_probe(struct platform_device *pd)
+{
+	struct cros_ec_dev *ec_dev = dev_get_drvdata(pd->dev.parent);
+	struct device *dev = &pd->dev;
+	int ret;
+
+	if (!ec_dev) {
+		dev_err(dev, "No EC dev found\n");
+		return -EINVAL;
+	}
+
+	ret = cros_ec_attach_attribute_group(ec_dev, &cros_ec_vbc_attr_group);
+	if (ret < 0)
+		dev_err(dev, "failed to create %s attributes. err=%d\n",
+			cros_ec_vbc_attr_group.name, ret);
+
+	return ret;
+}
+
+static int cros_ec_vbc_remove(struct platform_device *pd)
+{
+	struct cros_ec_dev *ec_dev = dev_get_drvdata(pd->dev.parent);
+
+	cros_ec_detach_attribute_group(ec_dev, &cros_ec_vbc_attr_group);
+
+	return 0;
+}
+
+static struct platform_driver cros_ec_vbc_driver = {
+	.driver = {
+		.name = DRV_NAME,
+	},
+	.probe = cros_ec_vbc_probe,
+	.remove = cros_ec_vbc_remove,
+};
+
+module_platform_driver(cros_ec_vbc_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Expose the vboot context nvram to userspace");
+MODULE_ALIAS("platform:" DRV_NAME);
diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
index 4431e46c5875..ddbb7e18a530 100644
--- a/include/linux/mfd/cros_ec.h
+++ b/include/linux/mfd/cros_ec.h
@@ -335,7 +335,6 @@ u32 cros_ec_get_host_event(struct cros_ec_device *ec_dev);
 
 /* sysfs stuff */
 extern struct attribute_group cros_ec_attr_group;
-extern struct attribute_group cros_ec_vbc_attr_group;
 
 /* debugfs stuff */
 int cros_ec_debugfs_init(struct cros_ec_dev *ec);
-- 
2.19.1

[PATCH 4/7] mfd / platform: cros_ec: move debugfs attributes to its own driver.

[Enric Balletbo i Serra]

The entire way how cros debugfs attibutes are created is broken.
cros_ec_debugfs should be its own driver and its attributes should be
associated with a debugfs driver not the mfd driver.

Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---

 drivers/mfd/cros_ec_dev.c                 | 41 +------------------
 drivers/platform/chrome/Kconfig           | 10 +++++
 drivers/platform/chrome/Makefile          |  4 +-
 drivers/platform/chrome/cros_ec_debugfs.c | 49 ++++++++++++++++++-----
 include/linux/mfd/cros_ec.h               |  6 ---
 5 files changed, 53 insertions(+), 57 deletions(-)

diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
index 790dc28b93f0..8c7ee2a0b50a 100644
--- a/drivers/mfd/cros_ec_dev.c
+++ b/drivers/mfd/cros_ec_dev.c
@@ -403,6 +403,7 @@ static const struct mfd_cell cros_usbpd_charger_cells[] = {
 };
 
 static const struct mfd_cell cros_ec_platform_cells[] = {
+	{ .name = "cros-ec-debugfs" },
 	{ .name = "cros-ec-lightbar" },
 	{ .name = "cros-ec-vbc" },
 };
@@ -496,9 +497,6 @@ static int ec_device_probe(struct platform_device *pdev)
 		dev_err(ec->dev,
 			"failed to add cros-ec platform devices: %d\n", retval);
 
-	if (cros_ec_debugfs_init(ec))
-		dev_warn(dev, "failed to create debugfs directory\n");
-
 	return 0;
 
 failed:
@@ -510,61 +508,24 @@ static int ec_device_remove(struct platform_device *pdev)
 {
 	struct cros_ec_dev *ec = dev_get_drvdata(&pdev->dev);
 
-	cros_ec_debugfs_remove(ec);
-
 	cdev_del(&ec->cdev);
 	device_unregister(&ec->class_dev);
 	return 0;
 }
 
-static void ec_device_shutdown(struct platform_device *pdev)
-{
-	struct cros_ec_dev *ec = dev_get_drvdata(&pdev->dev);
-
-	/* Be sure to clear up debugfs delayed works */
-	cros_ec_debugfs_remove(ec);
-}
-
 static const struct platform_device_id cros_ec_id[] = {
 	{ DRV_NAME, 0 },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(platform, cros_ec_id);
 
-static __maybe_unused int ec_device_suspend(struct device *dev)
-{
-	struct cros_ec_dev *ec = dev_get_drvdata(dev);
-
-	cros_ec_debugfs_suspend(ec);
-
-	return 0;
-}
-
-static __maybe_unused int ec_device_resume(struct device *dev)
-{
-	struct cros_ec_dev *ec = dev_get_drvdata(dev);
-
-	cros_ec_debugfs_resume(ec);
-
-	return 0;
-}
-
-static const struct dev_pm_ops cros_ec_dev_pm_ops = {
-#ifdef CONFIG_PM_SLEEP
-	.suspend = ec_device_suspend,
-	.resume = ec_device_resume,
-#endif
-};
-
 static struct platform_driver cros_ec_dev_driver = {
 	.driver = {
 		.name = DRV_NAME,
-		.pm = &cros_ec_dev_pm_ops,
 	},
 	.id_table = cros_ec_id,
 	.probe = ec_device_probe,
 	.remove = ec_device_remove,
-	.shutdown = ec_device_shutdown,
 };
 
 static int __init cros_ec_dev_init(void)
diff --git a/drivers/platform/chrome/Kconfig b/drivers/platform/chrome/Kconfig
index 29bd9837d520..a1239d48c46f 100644
--- a/drivers/platform/chrome/Kconfig
+++ b/drivers/platform/chrome/Kconfig
@@ -131,4 +131,14 @@ config CROS_EC_VBC
 	  To compile this driver as a module, choose M here: the
 	  module will be called cros_ec_vbc.
 
+config CROS_EC_DEBUGFS
+	tristate "Export ChromeOS EC internals in DebugFS"
+	depends on MFD_CROS_EC_CHARDEV && DEBUG_FS
+	help
+	  This option exposes the ChromeOS EC device internals to
+	  userspace.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called cros_ec_debugfs.
+
 endif # CHROMEOS_PLATFORMS
diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile
index 4081b7179df7..12a5c4d18c17 100644
--- a/drivers/platform/chrome/Makefile
+++ b/drivers/platform/chrome/Makefile
@@ -3,8 +3,7 @@
 obj-$(CONFIG_CHROMEOS_LAPTOP)		+= chromeos_laptop.o
 obj-$(CONFIG_CHROMEOS_PSTORE)		+= chromeos_pstore.o
 obj-$(CONFIG_CHROMEOS_TBMC)		+= chromeos_tbmc.o
-cros_ec_ctl-objs			:= cros_ec_sysfs.o \
-					   cros_ec_debugfs.o
+cros_ec_ctl-objs			:= cros_ec_sysfs.o
 obj-$(CONFIG_CROS_EC_CTL)		+= cros_ec_ctl.o
 obj-$(CONFIG_CROS_EC_I2C)		+= cros_ec_i2c.o
 obj-$(CONFIG_CROS_EC_SPI)		+= cros_ec_spi.o
@@ -15,3 +14,4 @@ obj-$(CONFIG_CROS_EC_PROTO)		+= cros_ec_proto.o
 obj-$(CONFIG_CROS_KBD_LED_BACKLIGHT)	+= cros_kbd_led_backlight.o
 obj-$(CONFIG_CROS_EC_LIGHTBAR)		+= cros_ec_lightbar.o
 obj-$(CONFIG_CROS_EC_VBC)		+= cros_ec_vbc.o
+obj-$(CONFIG_CROS_EC_DEBUGFS)		+= cros_ec_debugfs.o
diff --git a/drivers/platform/chrome/cros_ec_debugfs.c b/drivers/platform/chrome/cros_ec_debugfs.c
index c62ee8e610a0..2ae385a27a1d 100644
--- a/drivers/platform/chrome/cros_ec_debugfs.c
+++ b/drivers/platform/chrome/cros_ec_debugfs.c
@@ -23,12 +23,16 @@
 #include <linux/fs.h>
 #include <linux/mfd/cros_ec.h>
 #include <linux/mfd/cros_ec_commands.h>
+#include <linux/module.h>
 #include <linux/mutex.h>
+#include <linux/platform_device.h>
 #include <linux/poll.h>
 #include <linux/sched.h>
 #include <linux/slab.h>
 #include <linux/wait.h>
 
+#define DRV_NAME "cros-ec-debugfs"
+
 #define LOG_SHIFT		14
 #define LOG_SIZE		(1 << LOG_SHIFT)
 #define LOG_POLL_SEC		10
@@ -423,8 +427,9 @@ static int cros_ec_create_pdinfo(struct cros_ec_debugfs *debug_info)
 	return 0;
 }
 
-int cros_ec_debugfs_init(struct cros_ec_dev *ec)
+static int cros_ec_debugfs_probe(struct platform_device *pd)
 {
+	struct cros_ec_dev *ec = dev_get_drvdata(pd->dev.parent);
 	struct cros_ec_platform *ec_platform = dev_get_platdata(ec->dev);
 	const char *name = ec_platform->ec_name;
 	struct cros_ec_debugfs *debug_info;
@@ -459,20 +464,24 @@ int cros_ec_debugfs_init(struct cros_ec_dev *ec)
 	debugfs_remove_recursive(debug_info->dir);
 	return ret;
 }
-EXPORT_SYMBOL(cros_ec_debugfs_init);
 
-void cros_ec_debugfs_remove(struct cros_ec_dev *ec)
+static int cros_ec_debugfs_remove(struct platform_device *pd)
 {
+	struct cros_ec_dev *ec = dev_get_drvdata(pd->dev.parent);
+
 	if (!ec->debug_info)
-		return;
+		return 0;
 
 	debugfs_remove_recursive(ec->debug_info->dir);
 	cros_ec_cleanup_console_log(ec->debug_info);
+
+	return 0;
 }
-EXPORT_SYMBOL(cros_ec_debugfs_remove);
 
-void cros_ec_debugfs_suspend(struct cros_ec_dev *ec)
+static int __maybe_unused cros_ec_debugfs_suspend(struct device *dev)
 {
+	struct cros_ec_dev *ec = dev_get_drvdata(dev);
+
 	/*
 	 * cros_ec_debugfs_init() failures are non-fatal; it's also possible
 	 * that we initted things but decided that console log wasn't supported.
@@ -481,12 +490,34 @@ void cros_ec_debugfs_suspend(struct cros_ec_dev *ec)
 	 */
 	if (ec->debug_info && ec->debug_info->log_buffer.buf)
 		cancel_delayed_work_sync(&ec->debug_info->log_poll_work);
+
+	return 0;
 }
-EXPORT_SYMBOL(cros_ec_debugfs_suspend);
 
-void cros_ec_debugfs_resume(struct cros_ec_dev *ec)
+static int __maybe_unused cros_ec_debugfs_resume(struct device *dev)
 {
+	struct cros_ec_dev *ec = dev_get_drvdata(dev);
+
 	if (ec->debug_info && ec->debug_info->log_buffer.buf)
 		schedule_delayed_work(&ec->debug_info->log_poll_work, 0);
+
+	return 0;
 }
-EXPORT_SYMBOL(cros_ec_debugfs_resume);
+
+static SIMPLE_DEV_PM_OPS(cros_ec_debugfs_pm_ops,
+			 cros_ec_debugfs_suspend, cros_ec_debugfs_resume);
+
+static struct platform_driver cros_ec_debugfs_driver = {
+	.driver = {
+		.name = DRV_NAME,
+		.pm = &cros_ec_debugfs_pm_ops,
+	},
+	.probe = cros_ec_debugfs_probe,
+	.remove = cros_ec_debugfs_remove,
+};
+
+module_platform_driver(cros_ec_debugfs_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Debug logs for ChromeOS EC");
+MODULE_ALIAS("platform:" DRV_NAME);
diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
index ddbb7e18a530..1722c2c26143 100644
--- a/include/linux/mfd/cros_ec.h
+++ b/include/linux/mfd/cros_ec.h
@@ -336,12 +336,6 @@ u32 cros_ec_get_host_event(struct cros_ec_device *ec_dev);
 /* sysfs stuff */
 extern struct attribute_group cros_ec_attr_group;
 
-/* debugfs stuff */
-int cros_ec_debugfs_init(struct cros_ec_dev *ec);
-void cros_ec_debugfs_remove(struct cros_ec_dev *ec);
-void cros_ec_debugfs_suspend(struct cros_ec_dev *ec);
-void cros_ec_debugfs_resume(struct cros_ec_dev *ec);
-
 /* Attach/detach attributes to the cros_class */
 extern int cros_ec_attach_attribute_group(struct cros_ec_dev *ec,
 					  struct attribute_group *attrs);
-- 
2.19.1

[PATCH 5/7] mfd / platform: cros_ec: move device sysfs attributes to its own driver.

[Enric Balletbo i Serra]

The entire way how cros debugfs attibutes are created is broken.
cros_ec_sysfs should be its own driver and its attributes should be
associated with the sysfs driver not the mfd driver.

The patch also adds the sysfs documentation.

Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---

 .../ABI/testing/sysfs-class-chromeos          | 32 ++++++++++++++
 drivers/mfd/Kconfig                           |  1 -
 drivers/mfd/cros_ec_dev.c                     |  7 +--
 drivers/platform/chrome/Kconfig               | 14 ++++--
 drivers/platform/chrome/Makefile              |  3 +-
 drivers/platform/chrome/cros_ec_sysfs.c       | 43 ++++++++++++++++++-
 include/linux/mfd/cros_ec.h                   |  3 --
 7 files changed, 87 insertions(+), 16 deletions(-)
 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..a5399c4ca82d
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-chromeos
@@ -0,0 +1,32 @@
+What:		/sys/class/chromeos/<cros-ec>/flashinfo
+Date:		August 2015
+KernelVersion:	4.2
+Description:
+		Show the EC flash information.
+
+What:		/sys/class/chromeos/<cros-ec>/kb_wake_angle
+Date:		March 2018
+KernelVersion:	4.17
+Description:
+		Control the keyboard wake lid angle. Values are between
+		0 and 360. This file will also show the keyboard wake lid
+		angle by querying the hardware.
+
+What:		/sys/class/chromeos/<cros-ec>/reboot
+Date:		August 2015
+KernelVersion:	4.2
+Description:
+		Tell the EC to reboot in various ways. Options are:
+		"cancel": Cancel a pending reboot.
+		"ro": Jump to RO without rebooting.
+		"rw": Jump to RW without rebooting.
+		"cold": Cold reboot.
+		"disable-jump": Disable jump until next reboot.
+		"hibernate": Hibernate the EC.
+		"at-shutdown": Reboot after an AP shutdown.
+
+What:		/sys/class/chromeos/<cros-ec>/version
+Date:		August 2015
+KernelVersion:	4.2
+Description:
+		Show the information about the EC software and hardware.
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 8c5dfdce4326..089ffb408918 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -214,7 +214,6 @@ config MFD_CROS_EC
 config MFD_CROS_EC_CHARDEV
         tristate "Chrome OS Embedded Controller userspace device interface"
         depends on MFD_CROS_EC
-        select CROS_EC_CTL
         ---help---
           This driver adds support to talk with the ChromeOS EC from userspace.
 
diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
index 8c7ee2a0b50a..fc235af6da65 100644
--- a/drivers/mfd/cros_ec_dev.c
+++ b/drivers/mfd/cros_ec_dev.c
@@ -34,15 +34,9 @@
 #define CROS_MAX_DEV 128
 static int ec_major;
 
-static const struct attribute_group *cros_ec_groups[] = {
-	&cros_ec_attr_group,
-	NULL,
-};
-
 static struct class cros_class = {
 	.owner          = THIS_MODULE,
 	.name           = "chromeos",
-	.dev_groups     = cros_ec_groups,
 };
 
 int cros_ec_attach_attribute_group(struct cros_ec_dev *ec,
@@ -405,6 +399,7 @@ static const struct mfd_cell cros_usbpd_charger_cells[] = {
 static const struct mfd_cell cros_ec_platform_cells[] = {
 	{ .name = "cros-ec-debugfs" },
 	{ .name = "cros-ec-lightbar" },
+	{ .name = "cros-ec-sysfs" },
 	{ .name = "cros-ec-vbc" },
 };
 
diff --git a/drivers/platform/chrome/Kconfig b/drivers/platform/chrome/Kconfig
index a1239d48c46f..2ef51520766a 100644
--- a/drivers/platform/chrome/Kconfig
+++ b/drivers/platform/chrome/Kconfig
@@ -49,9 +49,6 @@ config CHROMEOS_TBMC
 	  To compile this driver as a module, choose M here: the
 	  module will be called chromeos_tbmc.
 
-config CROS_EC_CTL
-        tristate
-
 config CROS_EC_I2C
 	tristate "ChromeOS Embedded Controller (I2C)"
 	depends on MFD_CROS_EC && I2C
@@ -141,4 +138,15 @@ config CROS_EC_DEBUGFS
 	  To compile this driver as a module, choose M here: the
 	  module will be called cros_ec_debugfs.
 
+config CROS_EC_SYSFS
+	tristate "ChromeOS EC control and information through sysfs"
+	depends on MFD_CROS_EC_CHARDEV && SYSFS
+	default y
+	help
+	  This option exposes some sysfs attributes to control and get
+	  information from ChromeOS EC.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called cros_ec_sysfs.
+
 endif # CHROMEOS_PLATFORMS
diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile
index 12a5c4d18c17..fdbee501931b 100644
--- a/drivers/platform/chrome/Makefile
+++ b/drivers/platform/chrome/Makefile
@@ -3,8 +3,6 @@
 obj-$(CONFIG_CHROMEOS_LAPTOP)		+= chromeos_laptop.o
 obj-$(CONFIG_CHROMEOS_PSTORE)		+= chromeos_pstore.o
 obj-$(CONFIG_CHROMEOS_TBMC)		+= chromeos_tbmc.o
-cros_ec_ctl-objs			:= cros_ec_sysfs.o
-obj-$(CONFIG_CROS_EC_CTL)		+= cros_ec_ctl.o
 obj-$(CONFIG_CROS_EC_I2C)		+= cros_ec_i2c.o
 obj-$(CONFIG_CROS_EC_SPI)		+= cros_ec_spi.o
 cros_ec_lpcs-objs			:= cros_ec_lpc.o cros_ec_lpc_reg.o
@@ -15,3 +13,4 @@ obj-$(CONFIG_CROS_KBD_LED_BACKLIGHT)	+= cros_kbd_led_backlight.o
 obj-$(CONFIG_CROS_EC_LIGHTBAR)		+= cros_ec_lightbar.o
 obj-$(CONFIG_CROS_EC_VBC)		+= cros_ec_vbc.o
 obj-$(CONFIG_CROS_EC_DEBUGFS)		+= cros_ec_debugfs.o
+obj-$(CONFIG_CROS_EC_SYSFS)		+= cros_ec_sysfs.o
diff --git a/drivers/platform/chrome/cros_ec_sysfs.c b/drivers/platform/chrome/cros_ec_sysfs.c
index f34a50121064..47d08a8f0542 100644
--- a/drivers/platform/chrome/cros_ec_sysfs.c
+++ b/drivers/platform/chrome/cros_ec_sysfs.c
@@ -34,6 +34,8 @@
 #include <linux/types.h>
 #include <linux/uaccess.h>
 
+#define DRV_NAME "cros-ec-sysfs"
+
 /* Accessor functions */
 
 static ssize_t reboot_show(struct device *dev,
@@ -353,7 +355,46 @@ struct attribute_group cros_ec_attr_group = {
 	.attrs = __ec_attrs,
 	.is_visible = cros_ec_ctrl_visible,
 };
-EXPORT_SYMBOL(cros_ec_attr_group);
+
+static int cros_ec_sysfs_probe(struct platform_device *pd)
+{
+	struct cros_ec_dev *ec_dev = dev_get_drvdata(pd->dev.parent);
+	struct cros_ec_platform *ec_platform = dev_get_platdata(ec_dev->dev);
+	struct device *dev = &pd->dev;
+	int ret;
+
+	if (!ec_dev) {
+		dev_err(dev, "No EC dev found\n");
+		return -EINVAL;
+	}
+
+	ret = cros_ec_attach_attribute_group(ec_dev, &cros_ec_attr_group);
+	if (ret < 0)
+		dev_err(dev, "failed to create %s attributes. err=%d\n",
+			ec_platform->ec_name, ret);
+
+	return ret;
+}
+
+static int cros_ec_sysfs_remove(struct platform_device *pd)
+{
+	struct cros_ec_dev *ec_dev = dev_get_drvdata(pd->dev.parent);
+
+	cros_ec_detach_attribute_group(ec_dev, &cros_ec_attr_group);
+
+	return 0;
+}
+
+static struct platform_driver cros_ec_sysfs_driver = {
+	.driver = {
+		.name = DRV_NAME,
+	},
+	.probe = cros_ec_sysfs_probe,
+	.remove = cros_ec_sysfs_remove,
+};
+
+module_platform_driver(cros_ec_sysfs_driver);
 
 MODULE_LICENSE("GPL");
 MODULE_DESCRIPTION("ChromeOS EC control driver");
+MODULE_ALIAS("platform:" DRV_NAME);
diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
index 1722c2c26143..6d8618894a45 100644
--- a/include/linux/mfd/cros_ec.h
+++ b/include/linux/mfd/cros_ec.h
@@ -333,9 +333,6 @@ int cros_ec_get_next_event(struct cros_ec_device *ec_dev, bool *wake_event);
  */
 u32 cros_ec_get_host_event(struct cros_ec_device *ec_dev);
 
-/* sysfs stuff */
-extern struct attribute_group cros_ec_attr_group;
-
 /* Attach/detach attributes to the cros_class */
 extern int cros_ec_attach_attribute_group(struct cros_ec_dev *ec,
 					  struct attribute_group *attrs);
-- 
2.19.1

[PATCH 6/7] mfd / platform: cros_ec: instantiate only if th EC has a VBC NVRAM.

[Enric Balletbo i Serra]

The cros-ec-vbc driver is DT-only and there is a DT property that
indicates if the EC has the VCB NVRAM, in such case instantiate the
driver but don't instantiate on the other cases.

To do this move the check code to its parent instead of play with the
attribute group visibility. This changes a bit the actual behaviour.
Before the patch if an EC doesn't have a VBC NVRAM an empty vbc folder
is created in /sys/class/chromeos/<ec device>, after the patch the empty
folder is not created, so, the folder is only created if the vbc is set.

Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---

 drivers/mfd/cros_ec_dev.c             | 24 +++++++++++++++++++++++-
 drivers/platform/chrome/cros_ec_vbc.c | 16 ----------------
 2 files changed, 23 insertions(+), 17 deletions(-)

diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
index fc235af6da65..a63588953760 100644
--- a/drivers/mfd/cros_ec_dev.c
+++ b/drivers/mfd/cros_ec_dev.c
@@ -21,6 +21,7 @@
 #include <linux/mfd/core.h>
 #include <linux/module.h>
 #include <linux/mod_devicetable.h>
+#include <linux/of_platform.h>
 #include <linux/platform_device.h>
 #include <linux/pm.h>
 #include <linux/slab.h>
@@ -400,7 +401,10 @@ static const struct mfd_cell cros_ec_platform_cells[] = {
 	{ .name = "cros-ec-debugfs" },
 	{ .name = "cros-ec-lightbar" },
 	{ .name = "cros-ec-sysfs" },
-	{ .name = "cros-ec-vbc" },
+};
+
+static const struct mfd_cell cros_ec_vbc_cells[] = {
+	{ .name = "cros-ec-vbc" }
 };
 
 static int ec_device_probe(struct platform_device *pdev)
@@ -409,6 +413,7 @@ static int ec_device_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct cros_ec_platform *ec_platform = dev_get_platdata(dev);
 	struct cros_ec_dev *ec = devm_kzalloc(dev, sizeof(*ec), GFP_KERNEL);
+	struct device_node *node;
 
 	if (!ec)
 		return retval;
@@ -492,6 +497,23 @@ static int ec_device_probe(struct platform_device *pdev)
 		dev_err(ec->dev,
 			"failed to add cros-ec platform devices: %d\n", retval);
 
+	/* Check whether this EC instance has a VBC NVRAM */
+	node = ec->ec_dev->dev->of_node;
+	if (IS_ENABLED(CONFIG_OF) && node) {
+		if (of_property_read_bool(node, "google,has-vbc-nvram")) {
+			dev_err(dev, "device VBC found.\n");
+			retval = devm_mfd_add_devices(ec->dev,
+					PLATFORM_DEVID_AUTO,
+					cros_ec_vbc_cells,
+					ARRAY_SIZE(cros_ec_vbc_cells),
+					NULL, 0, NULL);
+			if (retval)
+				dev_err(ec->dev,
+					"failed to add VBC devices: %d\n",
+					retval);
+		}
+	}
+
 	return 0;
 
 failed:
diff --git a/drivers/platform/chrome/cros_ec_vbc.c b/drivers/platform/chrome/cros_ec_vbc.c
index 374153e3dc13..b7b81fe0fb25 100644
--- a/drivers/platform/chrome/cros_ec_vbc.c
+++ b/drivers/platform/chrome/cros_ec_vbc.c
@@ -108,21 +108,6 @@ static ssize_t vboot_context_write(struct file *filp, struct kobject *kobj,
 	return data_sz;
 }
 
-static umode_t cros_ec_vbc_is_visible(struct kobject *kobj,
-				      struct bin_attribute *a, int n)
-{
-	struct device *dev = container_of(kobj, struct device, kobj);
-	struct cros_ec_dev *ec = to_cros_ec_dev(dev);
-	struct device_node *np = ec->ec_dev->dev->of_node;
-
-	if (IS_ENABLED(CONFIG_OF) && np) {
-		if (of_property_read_bool(np, "google,has-vbc-nvram"))
-			return a->attr.mode;
-	}
-
-	return 0;
-}
-
 static BIN_ATTR_RW(vboot_context, 16);
 
 static struct bin_attribute *cros_ec_vbc_bin_attrs[] = {
@@ -133,7 +118,6 @@ static struct bin_attribute *cros_ec_vbc_bin_attrs[] = {
 struct attribute_group cros_ec_vbc_attr_group = {
 	.name = "vbc",
 	.bin_attrs = cros_ec_vbc_bin_attrs,
-	.is_bin_visible = cros_ec_vbc_is_visible,
 };
 
 static int cros_ec_vbc_probe(struct platform_device *pd)
-- 
2.19.1

[PATCH 7/7] platform/chrome: cros_ec_lightbar: instantiate only if the EC has a lightbar.

[Enric Balletbo i Serra]

Due to the way attribute groups visibility work, the function
cros_ec_lightbar_attrs_are_visible is called multiple times, once per
attribute, and each of these calls makes an EC transaction. For what is
worth the EC log reports multiple errors on boot when the lightbar is
not available. Instead, check if the EC has a lightbar in the probe
function and only instantiate the device.

Ideally we should have instantiate the driver only if the
EC_FEATURE_LIGHTBAR is defined, but that's not possible because that flag
is not in the very first Pixel Chromebook (Link), only on Samus. So, the
driver is instantiated by his parent always.

This patch changes a bit the actual behaviour. Before the patch if an EC
doesn't have a lightbar an empty lightbar folder is created in
/sys/class/chromeos/<ec device>, after the patch the empty folder is not
created, so, the folder is only created if the lightbar exists.

Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---

 drivers/platform/chrome/cros_ec_lightbar.c | 29 +++++++++-------------
 1 file changed, 12 insertions(+), 17 deletions(-)

diff --git a/drivers/platform/chrome/cros_ec_lightbar.c b/drivers/platform/chrome/cros_ec_lightbar.c
index 31d22f594fac..d255264eb082 100644
--- a/drivers/platform/chrome/cros_ec_lightbar.c
+++ b/drivers/platform/chrome/cros_ec_lightbar.c
@@ -567,37 +567,28 @@ static struct attribute *__lb_cmds_attrs[] = {
 	NULL,
 };
 
-static bool ec_has_lightbar(struct cros_ec_dev *ec)
+static bool cros_ec_has_lightbar(struct cros_ec_dev *ec_dev)
 {
-	return !!get_lightbar_version(ec, NULL, NULL);
-}
-
-static umode_t cros_ec_lightbar_attrs_are_visible(struct kobject *kobj,
-						  struct attribute *a, int n)
-{
-	struct device *dev = container_of(kobj, struct device, kobj);
-	struct cros_ec_dev *ec = to_cros_ec_dev(dev);
-	struct platform_device *pdev = to_platform_device(ec->dev);
+	struct platform_device *pdev = to_platform_device(ec_dev->dev);
 	struct cros_ec_platform *pdata = pdev->dev.platform_data;
 	int is_cros_ec;
 
 	is_cros_ec = strcmp(pdata->ec_name, CROS_EC_DEV_NAME);
 
 	if (is_cros_ec != 0)
-		return 0;
+		return false;
 
-	/* Only instantiate this stuff if the EC has a lightbar */
-	if (ec_has_lightbar(ec)) {
-		ec_with_lightbar = ec;
-		return a->mode;
+	if (!!get_lightbar_version(ec_dev, NULL, NULL)) {
+		ec_with_lightbar = ec_dev;
+		return true;
 	}
-	return 0;
+
+	return false;
 }
 
 struct attribute_group cros_ec_lightbar_attr_group = {
 	.name = "lightbar",
 	.attrs = __lb_cmds_attrs,
-	.is_visible = cros_ec_lightbar_attrs_are_visible,
 };
 
 static int cros_ec_lightbar_probe(struct platform_device *pd)
@@ -611,6 +602,10 @@ static int cros_ec_lightbar_probe(struct platform_device *pd)
 		return -EINVAL;
 	}
 
+	/* Only instantiate this stuff if the EC has a lightbar */
+	if (!cros_ec_has_lightbar(ec_dev))
+		return -ENODEV;
+
 	/* Take control of the lightbar from the EC. */
 	lb_manual_suspend_ctrl(ec_dev, 1);
 
-- 
2.19.1

Re: [PATCH 2/7] mfd / platform: cros_ec: move lightbar attributes to its own driver.

[Guenter Roeck]

Hi Enric,

On Thu, Nov 22, 2018 at 12:33:51PM +0100, Enric Balletbo i Serra wrote:
> The entire way how cros sysfs attibutes are created is broken.
> cros_ec_lightbar should be its own driver and its attributes should be
> associated with a lightbar driver not the mfd driver. In order to retain
> the path, the lightbar attributes are attached to the cros_class.
> 
> The patch also adds the sysfs documentation.
> 
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> ---
> 
...
>  
> +int cros_ec_attach_attribute_group(struct cros_ec_dev *ec,
> +				   struct attribute_group *attrs)
> +{
> +	return sysfs_create_group(&ec->class_dev.kobj, attrs);
> +}
> +EXPORT_SYMBOL(cros_ec_attach_attribute_group);
> +
> +void cros_ec_detach_attribute_group(struct cros_ec_dev *ec,
> +				    struct attribute_group *attrs)
> +{
> +	sysfs_remove_group(&ec->class_dev.kobj, attrs);
> +}
> +EXPORT_SYMBOL(cros_ec_detach_attribute_group);
> +

Are those two functions necessary ? Why not just call sysfs_create_group
and sysfs_remove_group directly from the calling code ?

Thanks,
Guenter

Re: [PATCH 4/7] mfd / platform: cros_ec: move debugfs attributes to its own driver.

[Guenter Roeck]

On Thu, Nov 22, 2018 at 12:33:53PM +0100, Enric Balletbo i Serra wrote:
> The entire way how cros debugfs attibutes are created is broken.
> cros_ec_debugfs should be its own driver and its attributes should be
> associated with a debugfs driver not the mfd driver.
> 
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> ---
> 
>  drivers/mfd/cros_ec_dev.c                 | 41 +------------------
>  drivers/platform/chrome/Kconfig           | 10 +++++
>  drivers/platform/chrome/Makefile          |  4 +-
>  drivers/platform/chrome/cros_ec_debugfs.c | 49 ++++++++++++++++++-----
>  include/linux/mfd/cros_ec.h               |  6 ---
>  5 files changed, 53 insertions(+), 57 deletions(-)
> 
> diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
> index 790dc28b93f0..8c7ee2a0b50a 100644
> --- a/drivers/mfd/cros_ec_dev.c
> +++ b/drivers/mfd/cros_ec_dev.c
> @@ -403,6 +403,7 @@ static const struct mfd_cell cros_usbpd_charger_cells[] = {
>  };
>  
>  static const struct mfd_cell cros_ec_platform_cells[] = {
> +	{ .name = "cros-ec-debugfs" },
>  	{ .name = "cros-ec-lightbar" },
>  	{ .name = "cros-ec-vbc" },
>  };
> @@ -496,9 +497,6 @@ static int ec_device_probe(struct platform_device *pdev)
>  		dev_err(ec->dev,
>  			"failed to add cros-ec platform devices: %d\n", retval);
>  
> -	if (cros_ec_debugfs_init(ec))
> -		dev_warn(dev, "failed to create debugfs directory\n");
> -
>  	return 0;
>  
>  failed:
> @@ -510,61 +508,24 @@ static int ec_device_remove(struct platform_device *pdev)
>  {
>  	struct cros_ec_dev *ec = dev_get_drvdata(&pdev->dev);
>  
> -	cros_ec_debugfs_remove(ec);
> -
>  	cdev_del(&ec->cdev);
>  	device_unregister(&ec->class_dev);
>  	return 0;
>  }
>  
> -static void ec_device_shutdown(struct platform_device *pdev)
> -{
> -	struct cros_ec_dev *ec = dev_get_drvdata(&pdev->dev);
> -
> -	/* Be sure to clear up debugfs delayed works */
> -	cros_ec_debugfs_remove(ec);
> -}
> -
>  static const struct platform_device_id cros_ec_id[] = {
>  	{ DRV_NAME, 0 },
>  	{ /* sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(platform, cros_ec_id);
>  
> -static __maybe_unused int ec_device_suspend(struct device *dev)
> -{
> -	struct cros_ec_dev *ec = dev_get_drvdata(dev);
> -
> -	cros_ec_debugfs_suspend(ec);
> -
> -	return 0;
> -}
> -
> -static __maybe_unused int ec_device_resume(struct device *dev)
> -{
> -	struct cros_ec_dev *ec = dev_get_drvdata(dev);
> -
> -	cros_ec_debugfs_resume(ec);
> -
> -	return 0;
> -}
> -
> -static const struct dev_pm_ops cros_ec_dev_pm_ops = {
> -#ifdef CONFIG_PM_SLEEP
> -	.suspend = ec_device_suspend,
> -	.resume = ec_device_resume,
> -#endif
> -};
> -
>  static struct platform_driver cros_ec_dev_driver = {
>  	.driver = {
>  		.name = DRV_NAME,
> -		.pm = &cros_ec_dev_pm_ops,
>  	},
>  	.id_table = cros_ec_id,
>  	.probe = ec_device_probe,
>  	.remove = ec_device_remove,
> -	.shutdown = ec_device_shutdown,
>  };
>  
>  static int __init cros_ec_dev_init(void)
> diff --git a/drivers/platform/chrome/Kconfig b/drivers/platform/chrome/Kconfig
> index 29bd9837d520..a1239d48c46f 100644
> --- a/drivers/platform/chrome/Kconfig
> +++ b/drivers/platform/chrome/Kconfig
> @@ -131,4 +131,14 @@ config CROS_EC_VBC
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called cros_ec_vbc.
>  
> +config CROS_EC_DEBUGFS
> +	tristate "Export ChromeOS EC internals in DebugFS"
> +	depends on MFD_CROS_EC_CHARDEV && DEBUG_FS

Maybe "default MFD_CROS_EC_CHARDEV" ?

> +	help
> +	  This option exposes the ChromeOS EC device internals to
> +	  userspace.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called cros_ec_debugfs.
> +
>  endif # CHROMEOS_PLATFORMS
> diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile
> index 4081b7179df7..12a5c4d18c17 100644
> --- a/drivers/platform/chrome/Makefile
> +++ b/drivers/platform/chrome/Makefile
> @@ -3,8 +3,7 @@
>  obj-$(CONFIG_CHROMEOS_LAPTOP)		+= chromeos_laptop.o
>  obj-$(CONFIG_CHROMEOS_PSTORE)		+= chromeos_pstore.o
>  obj-$(CONFIG_CHROMEOS_TBMC)		+= chromeos_tbmc.o
> -cros_ec_ctl-objs			:= cros_ec_sysfs.o \
> -					   cros_ec_debugfs.o
> +cros_ec_ctl-objs			:= cros_ec_sysfs.o
>  obj-$(CONFIG_CROS_EC_CTL)		+= cros_ec_ctl.o
>  obj-$(CONFIG_CROS_EC_I2C)		+= cros_ec_i2c.o
>  obj-$(CONFIG_CROS_EC_SPI)		+= cros_ec_spi.o
> @@ -15,3 +14,4 @@ obj-$(CONFIG_CROS_EC_PROTO)		+= cros_ec_proto.o
>  obj-$(CONFIG_CROS_KBD_LED_BACKLIGHT)	+= cros_kbd_led_backlight.o
>  obj-$(CONFIG_CROS_EC_LIGHTBAR)		+= cros_ec_lightbar.o
>  obj-$(CONFIG_CROS_EC_VBC)		+= cros_ec_vbc.o
> +obj-$(CONFIG_CROS_EC_DEBUGFS)		+= cros_ec_debugfs.o
> diff --git a/drivers/platform/chrome/cros_ec_debugfs.c b/drivers/platform/chrome/cros_ec_debugfs.c
> index c62ee8e610a0..2ae385a27a1d 100644
> --- a/drivers/platform/chrome/cros_ec_debugfs.c
> +++ b/drivers/platform/chrome/cros_ec_debugfs.c
> @@ -23,12 +23,16 @@
>  #include <linux/fs.h>
>  #include <linux/mfd/cros_ec.h>
>  #include <linux/mfd/cros_ec_commands.h>
> +#include <linux/module.h>
>  #include <linux/mutex.h>
> +#include <linux/platform_device.h>
>  #include <linux/poll.h>
>  #include <linux/sched.h>
>  #include <linux/slab.h>
>  #include <linux/wait.h>
>  
> +#define DRV_NAME "cros-ec-debugfs"
> +
>  #define LOG_SHIFT		14
>  #define LOG_SIZE		(1 << LOG_SHIFT)
>  #define LOG_POLL_SEC		10
> @@ -423,8 +427,9 @@ static int cros_ec_create_pdinfo(struct cros_ec_debugfs *debug_info)
>  	return 0;
>  }
>  
> -int cros_ec_debugfs_init(struct cros_ec_dev *ec)
> +static int cros_ec_debugfs_probe(struct platform_device *pd)
>  {
> +	struct cros_ec_dev *ec = dev_get_drvdata(pd->dev.parent);
>  	struct cros_ec_platform *ec_platform = dev_get_platdata(ec->dev);
>  	const char *name = ec_platform->ec_name;
>  	struct cros_ec_debugfs *debug_info;
> @@ -459,20 +464,24 @@ int cros_ec_debugfs_init(struct cros_ec_dev *ec)
>  	debugfs_remove_recursive(debug_info->dir);
>  	return ret;
>  }
> -EXPORT_SYMBOL(cros_ec_debugfs_init);
>  
> -void cros_ec_debugfs_remove(struct cros_ec_dev *ec)
> +static int cros_ec_debugfs_remove(struct platform_device *pd)
>  {
> +	struct cros_ec_dev *ec = dev_get_drvdata(pd->dev.parent);
> +
>  	if (!ec->debug_info)
> -		return;
> +		return 0;

This should no long be necessary.

>  
>  	debugfs_remove_recursive(ec->debug_info->dir);
>  	cros_ec_cleanup_console_log(ec->debug_info);
> +
> +	return 0;
>  }
> -EXPORT_SYMBOL(cros_ec_debugfs_remove);
>  
> -void cros_ec_debugfs_suspend(struct cros_ec_dev *ec)
> +static int __maybe_unused cros_ec_debugfs_suspend(struct device *dev)
>  {
> +	struct cros_ec_dev *ec = dev_get_drvdata(dev);
> +
>  	/*
>  	 * cros_ec_debugfs_init() failures are non-fatal; it's also possible
>  	 * that we initted things but decided that console log wasn't supported.

The above comment no longer applies. Also, since this is now its own driver,
the if statement should not be necessary anymore.

> @@ -481,12 +490,34 @@ void cros_ec_debugfs_suspend(struct cros_ec_dev *ec)
>  	 */
>  	if (ec->debug_info && ec->debug_info->log_buffer.buf)
>  		cancel_delayed_work_sync(&ec->debug_info->log_poll_work);
> +
> +	return 0;
>  }
> -EXPORT_SYMBOL(cros_ec_debugfs_suspend);
>  
> -void cros_ec_debugfs_resume(struct cros_ec_dev *ec)
> +static int __maybe_unused cros_ec_debugfs_resume(struct device *dev)
>  {
> +	struct cros_ec_dev *ec = dev_get_drvdata(dev);
> +
>  	if (ec->debug_info && ec->debug_info->log_buffer.buf)
>  		schedule_delayed_work(&ec->debug_info->log_poll_work, 0);

As aabove, the if statement should not be necessary anymore.

> +
> +	return 0;
>  }
> -EXPORT_SYMBOL(cros_ec_debugfs_resume);
> +
> +static SIMPLE_DEV_PM_OPS(cros_ec_debugfs_pm_ops,
> +			 cros_ec_debugfs_suspend, cros_ec_debugfs_resume);
> +
> +static struct platform_driver cros_ec_debugfs_driver = {
> +	.driver = {
> +		.name = DRV_NAME,
> +		.pm = &cros_ec_debugfs_pm_ops,
> +	},
> +	.probe = cros_ec_debugfs_probe,
> +	.remove = cros_ec_debugfs_remove,
> +};
> +
> +module_platform_driver(cros_ec_debugfs_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Debug logs for ChromeOS EC");
> +MODULE_ALIAS("platform:" DRV_NAME);
> diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
> index ddbb7e18a530..1722c2c26143 100644
> --- a/include/linux/mfd/cros_ec.h
> +++ b/include/linux/mfd/cros_ec.h
> @@ -336,12 +336,6 @@ u32 cros_ec_get_host_event(struct cros_ec_device *ec_dev);
>  /* sysfs stuff */
>  extern struct attribute_group cros_ec_attr_group;
>  
> -/* debugfs stuff */
> -int cros_ec_debugfs_init(struct cros_ec_dev *ec);
> -void cros_ec_debugfs_remove(struct cros_ec_dev *ec);
> -void cros_ec_debugfs_suspend(struct cros_ec_dev *ec);
> -void cros_ec_debugfs_resume(struct cros_ec_dev *ec);
> -
>  /* Attach/detach attributes to the cros_class */
>  extern int cros_ec_attach_attribute_group(struct cros_ec_dev *ec,
>  					  struct attribute_group *attrs);

Re: [PATCH 5/7] mfd / platform: cros_ec: move device sysfs attributes to its own driver.

[Guenter Roeck]

On Thu, Nov 22, 2018 at 12:33:54PM +0100, Enric Balletbo i Serra wrote:
> The entire way how cros debugfs attibutes are created is broken.
> cros_ec_sysfs should be its own driver and its attributes should be
> associated with the sysfs driver not the mfd driver.
> 
> The patch also adds the sysfs documentation.
> 
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> ---
> 
>  .../ABI/testing/sysfs-class-chromeos          | 32 ++++++++++++++
>  drivers/mfd/Kconfig                           |  1 -
>  drivers/mfd/cros_ec_dev.c                     |  7 +--
>  drivers/platform/chrome/Kconfig               | 14 ++++--
>  drivers/platform/chrome/Makefile              |  3 +-
>  drivers/platform/chrome/cros_ec_sysfs.c       | 43 ++++++++++++++++++-
>  include/linux/mfd/cros_ec.h                   |  3 --
>  7 files changed, 87 insertions(+), 16 deletions(-)
>  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..a5399c4ca82d
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-chromeos
> @@ -0,0 +1,32 @@
> +What:		/sys/class/chromeos/<cros-ec>/flashinfo
> +Date:		August 2015
> +KernelVersion:	4.2
> +Description:
> +		Show the EC flash information.
> +
> +What:		/sys/class/chromeos/<cros-ec>/kb_wake_angle
> +Date:		March 2018
> +KernelVersion:	4.17
> +Description:
> +		Control the keyboard wake lid angle. Values are between
> +		0 and 360. This file will also show the keyboard wake lid
> +		angle by querying the hardware.
> +
> +What:		/sys/class/chromeos/<cros-ec>/reboot
> +Date:		August 2015
> +KernelVersion:	4.2
> +Description:
> +		Tell the EC to reboot in various ways. Options are:
> +		"cancel": Cancel a pending reboot.
> +		"ro": Jump to RO without rebooting.
> +		"rw": Jump to RW without rebooting.
> +		"cold": Cold reboot.
> +		"disable-jump": Disable jump until next reboot.
> +		"hibernate": Hibernate the EC.
> +		"at-shutdown": Reboot after an AP shutdown.
> +
> +What:		/sys/class/chromeos/<cros-ec>/version
> +Date:		August 2015
> +KernelVersion:	4.2
> +Description:
> +		Show the information about the EC software and hardware.
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 8c5dfdce4326..089ffb408918 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -214,7 +214,6 @@ config MFD_CROS_EC
>  config MFD_CROS_EC_CHARDEV
>          tristate "Chrome OS Embedded Controller userspace device interface"
>          depends on MFD_CROS_EC
> -        select CROS_EC_CTL
>          ---help---
>            This driver adds support to talk with the ChromeOS EC from userspace.
>  
> diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
> index 8c7ee2a0b50a..fc235af6da65 100644
> --- a/drivers/mfd/cros_ec_dev.c
> +++ b/drivers/mfd/cros_ec_dev.c
> @@ -34,15 +34,9 @@
>  #define CROS_MAX_DEV 128
>  static int ec_major;
>  
> -static const struct attribute_group *cros_ec_groups[] = {
> -	&cros_ec_attr_group,
> -	NULL,
> -};
> -
>  static struct class cros_class = {
>  	.owner          = THIS_MODULE,
>  	.name           = "chromeos",
> -	.dev_groups     = cros_ec_groups,
>  };
>  
>  int cros_ec_attach_attribute_group(struct cros_ec_dev *ec,
> @@ -405,6 +399,7 @@ static const struct mfd_cell cros_usbpd_charger_cells[] = {
>  static const struct mfd_cell cros_ec_platform_cells[] = {
>  	{ .name = "cros-ec-debugfs" },
>  	{ .name = "cros-ec-lightbar" },
> +	{ .name = "cros-ec-sysfs" },
>  	{ .name = "cros-ec-vbc" },
>  };
>  
> diff --git a/drivers/platform/chrome/Kconfig b/drivers/platform/chrome/Kconfig
> index a1239d48c46f..2ef51520766a 100644
> --- a/drivers/platform/chrome/Kconfig
> +++ b/drivers/platform/chrome/Kconfig
> @@ -49,9 +49,6 @@ config CHROMEOS_TBMC
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called chromeos_tbmc.
>  
> -config CROS_EC_CTL
> -        tristate
> -
>  config CROS_EC_I2C
>  	tristate "ChromeOS Embedded Controller (I2C)"
>  	depends on MFD_CROS_EC && I2C
> @@ -141,4 +138,15 @@ config CROS_EC_DEBUGFS
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called cros_ec_debugfs.
>  
> +config CROS_EC_SYSFS
> +	tristate "ChromeOS EC control and information through sysfs"
> +	depends on MFD_CROS_EC_CHARDEV && SYSFS
> +	default y
> +	help
> +	  This option exposes some sysfs attributes to control and get
> +	  information from ChromeOS EC.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called cros_ec_sysfs.
> +
>  endif # CHROMEOS_PLATFORMS
> diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile
> index 12a5c4d18c17..fdbee501931b 100644
> --- a/drivers/platform/chrome/Makefile
> +++ b/drivers/platform/chrome/Makefile
> @@ -3,8 +3,6 @@
>  obj-$(CONFIG_CHROMEOS_LAPTOP)		+= chromeos_laptop.o
>  obj-$(CONFIG_CHROMEOS_PSTORE)		+= chromeos_pstore.o
>  obj-$(CONFIG_CHROMEOS_TBMC)		+= chromeos_tbmc.o
> -cros_ec_ctl-objs			:= cros_ec_sysfs.o
> -obj-$(CONFIG_CROS_EC_CTL)		+= cros_ec_ctl.o
>  obj-$(CONFIG_CROS_EC_I2C)		+= cros_ec_i2c.o
>  obj-$(CONFIG_CROS_EC_SPI)		+= cros_ec_spi.o
>  cros_ec_lpcs-objs			:= cros_ec_lpc.o cros_ec_lpc_reg.o
> @@ -15,3 +13,4 @@ obj-$(CONFIG_CROS_KBD_LED_BACKLIGHT)	+= cros_kbd_led_backlight.o
>  obj-$(CONFIG_CROS_EC_LIGHTBAR)		+= cros_ec_lightbar.o
>  obj-$(CONFIG_CROS_EC_VBC)		+= cros_ec_vbc.o
>  obj-$(CONFIG_CROS_EC_DEBUGFS)		+= cros_ec_debugfs.o
> +obj-$(CONFIG_CROS_EC_SYSFS)		+= cros_ec_sysfs.o
> diff --git a/drivers/platform/chrome/cros_ec_sysfs.c b/drivers/platform/chrome/cros_ec_sysfs.c
> index f34a50121064..47d08a8f0542 100644
> --- a/drivers/platform/chrome/cros_ec_sysfs.c
> +++ b/drivers/platform/chrome/cros_ec_sysfs.c
> @@ -34,6 +34,8 @@
>  #include <linux/types.h>
>  #include <linux/uaccess.h>
>  
> +#define DRV_NAME "cros-ec-sysfs"
> +
>  /* Accessor functions */
>  
>  static ssize_t reboot_show(struct device *dev,
> @@ -353,7 +355,46 @@ struct attribute_group cros_ec_attr_group = {
>  	.attrs = __ec_attrs,
>  	.is_visible = cros_ec_ctrl_visible,

I think the .is_visible evaluation should now happen in the probe fuction.

>  };
> -EXPORT_SYMBOL(cros_ec_attr_group);
> +
> +static int cros_ec_sysfs_probe(struct platform_device *pd)
> +{
> +	struct cros_ec_dev *ec_dev = dev_get_drvdata(pd->dev.parent);
> +	struct cros_ec_platform *ec_platform = dev_get_platdata(ec_dev->dev);
> +	struct device *dev = &pd->dev;
> +	int ret;
> +
> +	if (!ec_dev) {
> +		dev_err(dev, "No EC dev found\n");
> +		return -EINVAL;
> +	}

Above:

	struct cros_ec_platform *ec_platform = dev_get_platdata(ec_dev->dev);

so if ec_dev is ever NULL, we'll never get to this point (and static
analyzers will have a field day). I think the check is unnecessary.

> +
> +	ret = cros_ec_attach_attribute_group(ec_dev, &cros_ec_attr_group);
> +	if (ret < 0)
> +		dev_err(dev, "failed to create %s attributes. err=%d\n",
> +			ec_platform->ec_name, ret);

Does it really make sense to display ec_platform->ec_name here ?
"Failed to create sysfs attributes" with dev_err should be good enough.

Also, again, I think we should create the attribute directly and drop
cros_ec_attach_attribute_group().

> +
> +	return ret;
> +}
> +
> +static int cros_ec_sysfs_remove(struct platform_device *pd)
> +{
> +	struct cros_ec_dev *ec_dev = dev_get_drvdata(pd->dev.parent);
> +
> +	cros_ec_detach_attribute_group(ec_dev, &cros_ec_attr_group);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver cros_ec_sysfs_driver = {
> +	.driver = {
> +		.name = DRV_NAME,
> +	},
> +	.probe = cros_ec_sysfs_probe,
> +	.remove = cros_ec_sysfs_remove,
> +};
> +
> +module_platform_driver(cros_ec_sysfs_driver);
>  
>  MODULE_LICENSE("GPL");
>  MODULE_DESCRIPTION("ChromeOS EC control driver");
> +MODULE_ALIAS("platform:" DRV_NAME);
> diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
> index 1722c2c26143..6d8618894a45 100644
> --- a/include/linux/mfd/cros_ec.h
> +++ b/include/linux/mfd/cros_ec.h
> @@ -333,9 +333,6 @@ int cros_ec_get_next_event(struct cros_ec_device *ec_dev, bool *wake_event);
>   */
>  u32 cros_ec_get_host_event(struct cros_ec_device *ec_dev);
>  
> -/* sysfs stuff */
> -extern struct attribute_group cros_ec_attr_group;
> -
>  /* Attach/detach attributes to the cros_class */
>  extern int cros_ec_attach_attribute_group(struct cros_ec_dev *ec,
>  					  struct attribute_group *attrs);

Re: [PATCH 6/7] mfd / platform: cros_ec: instantiate only if th EC has a VBC NVRAM.

[Guenter Roeck]

On Thu, Nov 22, 2018 at 12:33:55PM +0100, Enric Balletbo i Serra wrote:
> The cros-ec-vbc driver is DT-only and there is a DT property that
> indicates if the EC has the VCB NVRAM, in such case instantiate the
> driver but don't instantiate on the other cases.
> 
> To do this move the check code to its parent instead of play with the
> attribute group visibility. This changes a bit the actual behaviour.
> Before the patch if an EC doesn't have a VBC NVRAM an empty vbc folder
> is created in /sys/class/chromeos/<ec device>, after the patch the empty
> folder is not created, so, the folder is only created if the vbc is set.
> 
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> ---
> 
>  drivers/mfd/cros_ec_dev.c             | 24 +++++++++++++++++++++++-
>  drivers/platform/chrome/cros_ec_vbc.c | 16 ----------------
>  2 files changed, 23 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
> index fc235af6da65..a63588953760 100644
> --- a/drivers/mfd/cros_ec_dev.c
> +++ b/drivers/mfd/cros_ec_dev.c
> @@ -21,6 +21,7 @@
>  #include <linux/mfd/core.h>
>  #include <linux/module.h>
>  #include <linux/mod_devicetable.h>
> +#include <linux/of_platform.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm.h>
>  #include <linux/slab.h>
> @@ -400,7 +401,10 @@ static const struct mfd_cell cros_ec_platform_cells[] = {
>  	{ .name = "cros-ec-debugfs" },
>  	{ .name = "cros-ec-lightbar" },
>  	{ .name = "cros-ec-sysfs" },
> -	{ .name = "cros-ec-vbc" },
> +};
> +
> +static const struct mfd_cell cros_ec_vbc_cells[] = {
> +	{ .name = "cros-ec-vbc" }
>  };
>  
>  static int ec_device_probe(struct platform_device *pdev)
> @@ -409,6 +413,7 @@ static int ec_device_probe(struct platform_device *pdev)
>  	struct device *dev = &pdev->dev;
>  	struct cros_ec_platform *ec_platform = dev_get_platdata(dev);
>  	struct cros_ec_dev *ec = devm_kzalloc(dev, sizeof(*ec), GFP_KERNEL);
> +	struct device_node *node;
>  
>  	if (!ec)
>  		return retval;
> @@ -492,6 +497,23 @@ static int ec_device_probe(struct platform_device *pdev)
>  		dev_err(ec->dev,
>  			"failed to add cros-ec platform devices: %d\n", retval);
>  
> +	/* Check whether this EC instance has a VBC NVRAM */
> +	node = ec->ec_dev->dev->of_node;
> +	if (IS_ENABLED(CONFIG_OF) && node) {

Is IS_ENABLED() necessary here ? Seems to me node should always be NULL
in this case.

> +		if (of_property_read_bool(node, "google,has-vbc-nvram")) {
> +			dev_err(dev, "device VBC found.\n");

dev_err ?

> +			retval = devm_mfd_add_devices(ec->dev,
> +					PLATFORM_DEVID_AUTO,
> +					cros_ec_vbc_cells,
> +					ARRAY_SIZE(cros_ec_vbc_cells),
> +					NULL, 0, NULL);
> +			if (retval)
> +				dev_err(ec->dev,
> +					"failed to add VBC devices: %d\n",
> +					retval);

Since the error is ignored, maybe better dev_warn() ?

> +		}
> +	}
> +
>  	return 0;
>  
>  failed:
> diff --git a/drivers/platform/chrome/cros_ec_vbc.c b/drivers/platform/chrome/cros_ec_vbc.c
> index 374153e3dc13..b7b81fe0fb25 100644
> --- a/drivers/platform/chrome/cros_ec_vbc.c
> +++ b/drivers/platform/chrome/cros_ec_vbc.c
> @@ -108,21 +108,6 @@ static ssize_t vboot_context_write(struct file *filp, struct kobject *kobj,
>  	return data_sz;
>  }
>  
> -static umode_t cros_ec_vbc_is_visible(struct kobject *kobj,
> -				      struct bin_attribute *a, int n)
> -{
> -	struct device *dev = container_of(kobj, struct device, kobj);
> -	struct cros_ec_dev *ec = to_cros_ec_dev(dev);
> -	struct device_node *np = ec->ec_dev->dev->of_node;
> -
> -	if (IS_ENABLED(CONFIG_OF) && np) {
> -		if (of_property_read_bool(np, "google,has-vbc-nvram"))
> -			return a->attr.mode;
> -	}
> -
> -	return 0;
> -}
> -
>  static BIN_ATTR_RW(vboot_context, 16);
>  
>  static struct bin_attribute *cros_ec_vbc_bin_attrs[] = {
> @@ -133,7 +118,6 @@ static struct bin_attribute *cros_ec_vbc_bin_attrs[] = {
>  struct attribute_group cros_ec_vbc_attr_group = {
>  	.name = "vbc",
>  	.bin_attrs = cros_ec_vbc_bin_attrs,
> -	.is_bin_visible = cros_ec_vbc_is_visible,
>  };
>  
>  static int cros_ec_vbc_probe(struct platform_device *pd)

Re: [PATCH 7/7] platform/chrome: cros_ec_lightbar: instantiate only if the EC has a lightbar.

[Guenter Roeck]

On Thu, Nov 22, 2018 at 12:33:56PM +0100, Enric Balletbo i Serra wrote:
> Due to the way attribute groups visibility work, the function
> cros_ec_lightbar_attrs_are_visible is called multiple times, once per
> attribute, and each of these calls makes an EC transaction. For what is
> worth the EC log reports multiple errors on boot when the lightbar is
> not available. Instead, check if the EC has a lightbar in the probe
> function and only instantiate the device.
> 
> Ideally we should have instantiate the driver only if the
> EC_FEATURE_LIGHTBAR is defined, but that's not possible because that flag
> is not in the very first Pixel Chromebook (Link), only on Samus. So, the
> driver is instantiated by his parent always.
> 
> This patch changes a bit the actual behaviour. Before the patch if an EC
> doesn't have a lightbar an empty lightbar folder is created in
> /sys/class/chromeos/<ec device>, after the patch the empty folder is not
> created, so, the folder is only created if the lightbar exists.
> 
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>

Guess this is the answer to the suggestion I had before. Maybe the two patches
should be merged together ? Or do  others think that they should be kept
separate ?

Additional comment below.

Thanks,
Guenter

> ---
> 
>  drivers/platform/chrome/cros_ec_lightbar.c | 29 +++++++++-------------
>  1 file changed, 12 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/platform/chrome/cros_ec_lightbar.c b/drivers/platform/chrome/cros_ec_lightbar.c
> index 31d22f594fac..d255264eb082 100644
> --- a/drivers/platform/chrome/cros_ec_lightbar.c
> +++ b/drivers/platform/chrome/cros_ec_lightbar.c
> @@ -567,37 +567,28 @@ static struct attribute *__lb_cmds_attrs[] = {
>  	NULL,
>  };
>  
> -static bool ec_has_lightbar(struct cros_ec_dev *ec)
> +static bool cros_ec_has_lightbar(struct cros_ec_dev *ec_dev)
>  {
> -	return !!get_lightbar_version(ec, NULL, NULL);
> -}
> -
> -static umode_t cros_ec_lightbar_attrs_are_visible(struct kobject *kobj,
> -						  struct attribute *a, int n)
> -{
> -	struct device *dev = container_of(kobj, struct device, kobj);
> -	struct cros_ec_dev *ec = to_cros_ec_dev(dev);
> -	struct platform_device *pdev = to_platform_device(ec->dev);
> +	struct platform_device *pdev = to_platform_device(ec_dev->dev);
>  	struct cros_ec_platform *pdata = pdev->dev.platform_data;
>  	int is_cros_ec;
>  
>  	is_cros_ec = strcmp(pdata->ec_name, CROS_EC_DEV_NAME);
>  
Can this now ever be false ?

>  	if (is_cros_ec != 0)
> -		return 0;
> +		return false;
>  
> -	/* Only instantiate this stuff if the EC has a lightbar */
> -	if (ec_has_lightbar(ec)) {
> -		ec_with_lightbar = ec;
> -		return a->mode;
> +	if (!!get_lightbar_version(ec_dev, NULL, NULL)) {
> +		ec_with_lightbar = ec_dev;

Is this variable (and the associated check in lb_manual_suspend_ctrl)
still necessary ?

> +		return true;
>  	}
> -	return 0;
> +
> +	return false;
>  }
>  
>  struct attribute_group cros_ec_lightbar_attr_group = {
>  	.name = "lightbar",
>  	.attrs = __lb_cmds_attrs,
> -	.is_visible = cros_ec_lightbar_attrs_are_visible,
>  };
>  
>  static int cros_ec_lightbar_probe(struct platform_device *pd)
> @@ -611,6 +602,10 @@ static int cros_ec_lightbar_probe(struct platform_device *pd)
>  		return -EINVAL;
>  	}
>  
> +	/* Only instantiate this stuff if the EC has a lightbar */
> +	if (!cros_ec_has_lightbar(ec_dev))
> +		return -ENODEV;
> +
>  	/* Take control of the lightbar from the EC. */
>  	lb_manual_suspend_ctrl(ec_dev, 1);
>  

Re: [PATCH 6/7] mfd / platform: cros_ec: instantiate only if th EC has a VBC NVRAM.

[Enric Balletbo i Serra]

Hi Guenter,

On 22/11/18 20:14, Guenter Roeck wrote:
> On Thu, Nov 22, 2018 at 12:33:55PM +0100, Enric Balletbo i Serra wrote:
>> The cros-ec-vbc driver is DT-only and there is a DT property that
>> indicates if the EC has the VCB NVRAM, in such case instantiate the
>> driver but don't instantiate on the other cases.
>>
>> To do this move the check code to its parent instead of play with the
>> attribute group visibility. This changes a bit the actual behaviour.
>> Before the patch if an EC doesn't have a VBC NVRAM an empty vbc folder
>> is created in /sys/class/chromeos/<ec device>, after the patch the empty
>> folder is not created, so, the folder is only created if the vbc is set.
>>
>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>> ---
>>
>>  drivers/mfd/cros_ec_dev.c             | 24 +++++++++++++++++++++++-
>>  drivers/platform/chrome/cros_ec_vbc.c | 16 ----------------
>>  2 files changed, 23 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
>> index fc235af6da65..a63588953760 100644
>> --- a/drivers/mfd/cros_ec_dev.c
>> +++ b/drivers/mfd/cros_ec_dev.c
>> @@ -21,6 +21,7 @@
>>  #include <linux/mfd/core.h>
>>  #include <linux/module.h>
>>  #include <linux/mod_devicetable.h>
>> +#include <linux/of_platform.h>
>>  #include <linux/platform_device.h>
>>  #include <linux/pm.h>
>>  #include <linux/slab.h>
>> @@ -400,7 +401,10 @@ static const struct mfd_cell cros_ec_platform_cells[] = {
>>  	{ .name = "cros-ec-debugfs" },
>>  	{ .name = "cros-ec-lightbar" },
>>  	{ .name = "cros-ec-sysfs" },
>> -	{ .name = "cros-ec-vbc" },
>> +};
>> +
>> +static const struct mfd_cell cros_ec_vbc_cells[] = {
>> +	{ .name = "cros-ec-vbc" }
>>  };
>>  
>>  static int ec_device_probe(struct platform_device *pdev)
>> @@ -409,6 +413,7 @@ static int ec_device_probe(struct platform_device *pdev)
>>  	struct device *dev = &pdev->dev;
>>  	struct cros_ec_platform *ec_platform = dev_get_platdata(dev);
>>  	struct cros_ec_dev *ec = devm_kzalloc(dev, sizeof(*ec), GFP_KERNEL);
>> +	struct device_node *node;
>>  
>>  	if (!ec)
>>  		return retval;
>> @@ -492,6 +497,23 @@ static int ec_device_probe(struct platform_device *pdev)
>>  		dev_err(ec->dev,
>>  			"failed to add cros-ec platform devices: %d\n", retval);
>>  
>> +	/* Check whether this EC instance has a VBC NVRAM */
>> +	node = ec->ec_dev->dev->of_node;
>> +	if (IS_ENABLED(CONFIG_OF) && node) {
> 
> Is IS_ENABLED() necessary here ? Seems to me node should always be NULL
> in this case.
> 
>> +		if (of_property_read_bool(node, "google,has-vbc-nvram")) {
>> +			dev_err(dev, "device VBC found.\n");
> 
> dev_err ?
> 

Ups, I added this message only for debugging puposes, I think doesn't make sense
here so I'll remove.

>> +			retval = devm_mfd_add_devices(ec->dev,
>> +					PLATFORM_DEVID_AUTO,
>> +					cros_ec_vbc_cells,
>> +					ARRAY_SIZE(cros_ec_vbc_cells),
>> +					NULL, 0, NULL);
>> +			if (retval)
>> +				dev_err(ec->dev,
>> +					"failed to add VBC devices: %d\n",
>> +					retval);
> 
> Since the error is ignored, maybe better dev_warn() ?
> 

Done in next version.

>> +		}
>> +	}
>> +
>>  	return 0;
>>  
>>  failed:
>> diff --git a/drivers/platform/chrome/cros_ec_vbc.c b/drivers/platform/chrome/cros_ec_vbc.c
>> index 374153e3dc13..b7b81fe0fb25 100644
>> --- a/drivers/platform/chrome/cros_ec_vbc.c
>> +++ b/drivers/platform/chrome/cros_ec_vbc.c
>> @@ -108,21 +108,6 @@ static ssize_t vboot_context_write(struct file *filp, struct kobject *kobj,
>>  	return data_sz;
>>  }
>>  
>> -static umode_t cros_ec_vbc_is_visible(struct kobject *kobj,
>> -				      struct bin_attribute *a, int n)
>> -{
>> -	struct device *dev = container_of(kobj, struct device, kobj);
>> -	struct cros_ec_dev *ec = to_cros_ec_dev(dev);
>> -	struct device_node *np = ec->ec_dev->dev->of_node;
>> -
>> -	if (IS_ENABLED(CONFIG_OF) && np) {
>> -		if (of_property_read_bool(np, "google,has-vbc-nvram"))
>> -			return a->attr.mode;
>> -	}
>> -
>> -	return 0;
>> -}
>> -
>>  static BIN_ATTR_RW(vboot_context, 16);
>>  
>>  static struct bin_attribute *cros_ec_vbc_bin_attrs[] = {
>> @@ -133,7 +118,6 @@ static struct bin_attribute *cros_ec_vbc_bin_attrs[] = {
>>  struct attribute_group cros_ec_vbc_attr_group = {
>>  	.name = "vbc",
>>  	.bin_attrs = cros_ec_vbc_bin_attrs,
>> -	.is_bin_visible = cros_ec_vbc_is_visible,
>>  };
>>  
>>  static int cros_ec_vbc_probe(struct platform_device *pd)

Re: [PATCH 7/7] platform/chrome: cros_ec_lightbar: instantiate only if the EC has a lightbar.

[Enric Balletbo i Serra]

Hi Guenter,

On 22/11/18 20:25, Guenter Roeck wrote:
> On Thu, Nov 22, 2018 at 12:33:56PM +0100, Enric Balletbo i Serra wrote:
>> Due to the way attribute groups visibility work, the function
>> cros_ec_lightbar_attrs_are_visible is called multiple times, once per
>> attribute, and each of these calls makes an EC transaction. For what is
>> worth the EC log reports multiple errors on boot when the lightbar is
>> not available. Instead, check if the EC has a lightbar in the probe
>> function and only instantiate the device.
>>
>> Ideally we should have instantiate the driver only if the
>> EC_FEATURE_LIGHTBAR is defined, but that's not possible because that flag
>> is not in the very first Pixel Chromebook (Link), only on Samus. So, the
>> driver is instantiated by his parent always.
>>
>> This patch changes a bit the actual behaviour. Before the patch if an EC
>> doesn't have a lightbar an empty lightbar folder is created in
>> /sys/class/chromeos/<ec device>, after the patch the empty folder is not
>> created, so, the folder is only created if the lightbar exists.
>>
>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> 
> Guess this is the answer to the suggestion I had before. Maybe the two patches
> should be merged together ? Or do  others think that they should be kept
> separate ?
> 

I did in a separate patch because it changes a bit the current behaviour (i.e
after that patch the lightbar directory will not appear if is not detected).
Having in a separate patch will allow us to revert cleanly if for some weird
reason we still want the old behaviour. So in general, first I moved the
attributes and then I did a follow up patch with the probe change. This also
happens with vbc driver.

Said that, I don't mind to merge the two patches.


> Additional comment below.
> 
> Thanks,
> Guenter
> 
>> ---
>>
>>  drivers/platform/chrome/cros_ec_lightbar.c | 29 +++++++++-------------
>>  1 file changed, 12 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/platform/chrome/cros_ec_lightbar.c b/drivers/platform/chrome/cros_ec_lightbar.c
>> index 31d22f594fac..d255264eb082 100644
>> --- a/drivers/platform/chrome/cros_ec_lightbar.c
>> +++ b/drivers/platform/chrome/cros_ec_lightbar.c
>> @@ -567,37 +567,28 @@ static struct attribute *__lb_cmds_attrs[] = {
>>  	NULL,
>>  };
>>  
>> -static bool ec_has_lightbar(struct cros_ec_dev *ec)
>> +static bool cros_ec_has_lightbar(struct cros_ec_dev *ec_dev)
>>  {
>> -	return !!get_lightbar_version(ec, NULL, NULL);
>> -}
>> -
>> -static umode_t cros_ec_lightbar_attrs_are_visible(struct kobject *kobj,
>> -						  struct attribute *a, int n)
>> -{
>> -	struct device *dev = container_of(kobj, struct device, kobj);
>> -	struct cros_ec_dev *ec = to_cros_ec_dev(dev);
>> -	struct platform_device *pdev = to_platform_device(ec->dev);
>> +	struct platform_device *pdev = to_platform_device(ec_dev->dev);
>>  	struct cros_ec_platform *pdata = pdev->dev.platform_data;
>>  	int is_cros_ec;
>>  
>>  	is_cros_ec = strcmp(pdata->ec_name, CROS_EC_DEV_NAME);
>>  
> Can this now ever be false ?
> 

Yes, this happens for example on Samus, where there are two ECs, the first one
is named "cros_ec" and the second one is "cros_pd", so will fail in the second
case. Gwendal, correct me if I am wrong, but AFAIK this is a bit of hack because
some initial versions of Samus (coded Link I guess) have no support for the
EC_LIGHTBAR_FEATURE so we can't really use the features thing.

>>  	if (is_cros_ec != 0)
>> -		return 0;
>> +		return false;
>>  
>> -	/* Only instantiate this stuff if the EC has a lightbar */
>> -	if (ec_has_lightbar(ec)) {
>> -		ec_with_lightbar = ec;
>> -		return a->mode;
>> +	if (!!get_lightbar_version(ec_dev, NULL, NULL)) {
>> +		ec_with_lightbar = ec_dev;
> 
> Is this variable (and the associated check in lb_manual_suspend_ctrl)
> still necessary ?
> 

Hmm, right, I will double check and remove in next version.

>> +		return true;
>>  	}
>> -	return 0;
>> +
>> +	return false;
>>  }
>>  
>>  struct attribute_group cros_ec_lightbar_attr_group = {
>>  	.name = "lightbar",
>>  	.attrs = __lb_cmds_attrs,
>> -	.is_visible = cros_ec_lightbar_attrs_are_visible,
>>  };
>>  
>>  static int cros_ec_lightbar_probe(struct platform_device *pd)
>> @@ -611,6 +602,10 @@ static int cros_ec_lightbar_probe(struct platform_device *pd)
>>  		return -EINVAL;
>>  	}
>>  
>> +	/* Only instantiate this stuff if the EC has a lightbar */
>> +	if (!cros_ec_has_lightbar(ec_dev))
>> +		return -ENODEV;
>> +
>>  	/* Take control of the lightbar from the EC. */
>>  	lb_manual_suspend_ctrl(ec_dev, 1);
>>  
> 

Re: [PATCH 7/7] platform/chrome: cros_ec_lightbar: instantiate only if the EC has a lightbar.

[Guenter Roeck]

On 11/23/18 3:10 AM, Enric Balletbo i Serra wrote:
> Hi Guenter,
> 
> On 22/11/18 20:25, Guenter Roeck wrote:
>> On Thu, Nov 22, 2018 at 12:33:56PM +0100, Enric Balletbo i Serra wrote:
>>> Due to the way attribute groups visibility work, the function
>>> cros_ec_lightbar_attrs_are_visible is called multiple times, once per
>>> attribute, and each of these calls makes an EC transaction. For what is
>>> worth the EC log reports multiple errors on boot when the lightbar is
>>> not available. Instead, check if the EC has a lightbar in the probe
>>> function and only instantiate the device.
>>>
>>> Ideally we should have instantiate the driver only if the
>>> EC_FEATURE_LIGHTBAR is defined, but that's not possible because that flag
>>> is not in the very first Pixel Chromebook (Link), only on Samus. So, the
>>> driver is instantiated by his parent always.
>>>
>>> This patch changes a bit the actual behaviour. Before the patch if an EC
>>> doesn't have a lightbar an empty lightbar folder is created in
>>> /sys/class/chromeos/<ec device>, after the patch the empty folder is not
>>> created, so, the folder is only created if the lightbar exists.
>>>
>>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>>
>> Guess this is the answer to the suggestion I had before. Maybe the two patches
>> should be merged together ? Or do  others think that they should be kept
>> separate ?
>>
> 
> I did in a separate patch because it changes a bit the current behaviour (i.e
> after that patch the lightbar directory will not appear if is not detected).
> Having in a separate patch will allow us to revert cleanly if for some weird
> reason we still want the old behaviour. So in general, first I moved the
> attributes and then I did a follow up patch with the probe change. This also
> happens with vbc driver.
> 

Good point, makes sense.

> Said that, I don't mind to merge the two patches.
> 
> 
>> Additional comment below.
>>
>> Thanks,
>> Guenter
>>
>>> ---
>>>
>>>   drivers/platform/chrome/cros_ec_lightbar.c | 29 +++++++++-------------
>>>   1 file changed, 12 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/drivers/platform/chrome/cros_ec_lightbar.c b/drivers/platform/chrome/cros_ec_lightbar.c
>>> index 31d22f594fac..d255264eb082 100644
>>> --- a/drivers/platform/chrome/cros_ec_lightbar.c
>>> +++ b/drivers/platform/chrome/cros_ec_lightbar.c
>>> @@ -567,37 +567,28 @@ static struct attribute *__lb_cmds_attrs[] = {
>>>   	NULL,
>>>   };
>>>   
>>> -static bool ec_has_lightbar(struct cros_ec_dev *ec)
>>> +static bool cros_ec_has_lightbar(struct cros_ec_dev *ec_dev)
>>>   {
>>> -	return !!get_lightbar_version(ec, NULL, NULL);
>>> -}
>>> -
>>> -static umode_t cros_ec_lightbar_attrs_are_visible(struct kobject *kobj,
>>> -						  struct attribute *a, int n)
>>> -{
>>> -	struct device *dev = container_of(kobj, struct device, kobj);
>>> -	struct cros_ec_dev *ec = to_cros_ec_dev(dev);
>>> -	struct platform_device *pdev = to_platform_device(ec->dev);
>>> +	struct platform_device *pdev = to_platform_device(ec_dev->dev);
>>>   	struct cros_ec_platform *pdata = pdev->dev.platform_data;
>>>   	int is_cros_ec;
>>>   
>>>   	is_cros_ec = strcmp(pdata->ec_name, CROS_EC_DEV_NAME);
>>>   
>> Can this now ever be false ?
>>
> 
> Yes, this happens for example on Samus, where there are two ECs, the first one
> is named "cros_ec" and the second one is "cros_pd", so will fail in the second
> case. Gwendal, correct me if I am wrong, but AFAIK this is a bit of hack because
> some initial versions of Samus (coded Link I guess) have no support for the
> EC_LIGHTBAR_FEATURE so we can't really use the features thing.
> 
Ok.

Thanks,
Guenter

>>>   	if (is_cros_ec != 0)
>>> -		return 0;
>>> +		return false;
>>>   
>>> -	/* Only instantiate this stuff if the EC has a lightbar */
>>> -	if (ec_has_lightbar(ec)) {
>>> -		ec_with_lightbar = ec;
>>> -		return a->mode;
>>> +	if (!!get_lightbar_version(ec_dev, NULL, NULL)) {
>>> +		ec_with_lightbar = ec_dev;
>>
>> Is this variable (and the associated check in lb_manual_suspend_ctrl)
>> still necessary ?
>>
> 
> Hmm, right, I will double check and remove in next version.
> 
>>> +		return true;
>>>   	}
>>> -	return 0;
>>> +
>>> +	return false;
>>>   }
>>>   
>>>   struct attribute_group cros_ec_lightbar_attr_group = {
>>>   	.name = "lightbar",
>>>   	.attrs = __lb_cmds_attrs,
>>> -	.is_visible = cros_ec_lightbar_attrs_are_visible,
>>>   };
>>>   
>>>   static int cros_ec_lightbar_probe(struct platform_device *pd)
>>> @@ -611,6 +602,10 @@ static int cros_ec_lightbar_probe(struct platform_device *pd)
>>>   		return -EINVAL;
>>>   	}
>>>   
>>> +	/* Only instantiate this stuff if the EC has a lightbar */
>>> +	if (!cros_ec_has_lightbar(ec_dev))
>>> +		return -ENODEV;
>>> +
>>>   	/* Take control of the lightbar from the EC. */
>>>   	lb_manual_suspend_ctrl(ec_dev, 1);
>>>   
>>
> 

Re: [PATCH 2/7] mfd / platform: cros_ec: move lightbar attributes to its own driver.

[Enric Balletbo i Serra]

Hi Guenter,

On 22/11/18 18:41, Guenter Roeck wrote:
> Hi Enric,
> 
> On Thu, Nov 22, 2018 at 12:33:51PM +0100, Enric Balletbo i Serra wrote:
>> The entire way how cros sysfs attibutes are created is broken.
>> cros_ec_lightbar should be its own driver and its attributes should be
>> associated with a lightbar driver not the mfd driver. In order to retain
>> the path, the lightbar attributes are attached to the cros_class.
>>
>> The patch also adds the sysfs documentation.
>>
>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>> ---
>>
> ...
>>  
>> +int cros_ec_attach_attribute_group(struct cros_ec_dev *ec,
>> +				   struct attribute_group *attrs)
>> +{
>> +	return sysfs_create_group(&ec->class_dev.kobj, attrs);
>> +}
>> +EXPORT_SYMBOL(cros_ec_attach_attribute_group);
>> +
>> +void cros_ec_detach_attribute_group(struct cros_ec_dev *ec,
>> +				    struct attribute_group *attrs)
>> +{
>> +	sysfs_remove_group(&ec->class_dev.kobj, attrs);
>> +}
>> +EXPORT_SYMBOL(cros_ec_detach_attribute_group);
>> +
> 
> Are those two functions necessary ? Why not just call sysfs_create_group
> and sysfs_remove_group directly from the calling code ?
> 

Actually we have cros_ec_dev which registers the cros_ec class, and sysfs/vbc
and lightbar using this cros_ec class. I had problems unloading the different
modules. For example, when I removed cros_ec_dev modules before
cros_ec_sysfs/cros_ec_vbc/cros_ec_lightbar I got a hang.

To solve the hang I did the easy solution that is make these drivers depend on
cros_ec_dev so you're not able to unload cros_ec_dev if first you don't unload
the sysfs/vbc/lightbar.

Thinking again about it, I don't really understand now why failed in the first
place, cros_ec_dev is the parent, so, on remove should call mfd_remove_devices
for the subdevices.

So, let me check again this and I'll back to you.

Thanks,
 Enric


> Thanks,
> Guenter
> 

Re: [PATCH 2/7] mfd / platform: cros_ec: move lightbar attributes to its own driver.

[Guenter Roeck]

On 11/23/18 3:52 AM, Enric Balletbo i Serra wrote:
> Hi Guenter,
> 
> On 22/11/18 18:41, Guenter Roeck wrote:
>> Hi Enric,
>>
>> On Thu, Nov 22, 2018 at 12:33:51PM +0100, Enric Balletbo i Serra wrote:
>>> The entire way how cros sysfs attibutes are created is broken.
>>> cros_ec_lightbar should be its own driver and its attributes should be
>>> associated with a lightbar driver not the mfd driver. In order to retain
>>> the path, the lightbar attributes are attached to the cros_class.
>>>
>>> The patch also adds the sysfs documentation.
>>>
>>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>>> ---
>>>
>> ...
>>>   
>>> +int cros_ec_attach_attribute_group(struct cros_ec_dev *ec,
>>> +				   struct attribute_group *attrs)
>>> +{
>>> +	return sysfs_create_group(&ec->class_dev.kobj, attrs);
>>> +}
>>> +EXPORT_SYMBOL(cros_ec_attach_attribute_group);
>>> +
>>> +void cros_ec_detach_attribute_group(struct cros_ec_dev *ec,
>>> +				    struct attribute_group *attrs)
>>> +{
>>> +	sysfs_remove_group(&ec->class_dev.kobj, attrs);
>>> +}
>>> +EXPORT_SYMBOL(cros_ec_detach_attribute_group);
>>> +
>>
>> Are those two functions necessary ? Why not just call sysfs_create_group
>> and sysfs_remove_group directly from the calling code ?
>>
> 
> Actually we have cros_ec_dev which registers the cros_ec class, and sysfs/vbc
> and lightbar using this cros_ec class. I had problems unloading the different
> modules. For example, when I removed cros_ec_dev modules before
> cros_ec_sysfs/cros_ec_vbc/cros_ec_lightbar I got a hang.
> 
> To solve the hang I did the easy solution that is make these drivers depend on
> cros_ec_dev so you're not able to unload cros_ec_dev if first you don't unload
> the sysfs/vbc/lightbar.
> 

That seems like a side effect of the callbacks, which may increase the use count
of cros_ec_dev. If the lack of these callbacks causes problems, we should identify
the root cause and fix it, and not depend on side effects of a callback.

Thanks,
Guenter

> Thinking again about it, I don't really understand now why failed in the first
> place, cros_ec_dev is the parent, so, on remove should call mfd_remove_devices
> for the subdevices.
> 
> So, let me check again this and I'll back to you.
> 
> Thanks,
>   Enric
> 
> 
>> Thanks,
>> Guenter
>>
> 

Re: [PATCH 5/7] mfd / platform: cros_ec: move device sysfs attributes to its own driver.

[Dan Carpenter]

Hi Enric,

I love your patch! Perhaps something to improve:

url:    https://github.com/0day-ci/linux/commits/Enric-Balletbo-i-Serra/mfd-cros_ec-use-devm_mfd_add_devices/20181123-025253

smatch warnings:
drivers/platform/chrome/cros_ec_sysfs.c:366 cros_ec_sysfs_probe() warn: variable dereferenced before check 'ec_dev' (see line 362)

# https://github.com/0day-ci/linux/commit/b3074e331f36fff8890e7bd5c1f5874f4c59d38f
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout b3074e331f36fff8890e7bd5c1f5874f4c59d38f
vim +/ec_dev +366 drivers/platform/chrome/cros_ec_sysfs.c

b3074e33 Enric Balletbo i Serra 2018-11-22  358  
b3074e33 Enric Balletbo i Serra 2018-11-22  359  static int cros_ec_sysfs_probe(struct platform_device *pd)
b3074e33 Enric Balletbo i Serra 2018-11-22  360  {
b3074e33 Enric Balletbo i Serra 2018-11-22  361  	struct cros_ec_dev *ec_dev = dev_get_drvdata(pd->dev.parent);
b3074e33 Enric Balletbo i Serra 2018-11-22 @362  	struct cros_ec_platform *ec_platform = dev_get_platdata(ec_dev->dev);
                                                                                                                ^^^^^^^^^^^
b3074e33 Enric Balletbo i Serra 2018-11-22  363  	struct device *dev = &pd->dev;
b3074e33 Enric Balletbo i Serra 2018-11-22  364  	int ret;
b3074e33 Enric Balletbo i Serra 2018-11-22  365  
b3074e33 Enric Balletbo i Serra 2018-11-22 @366  	if (!ec_dev) {
                                                             ^^^^^^
Presumable this test can be removed?

b3074e33 Enric Balletbo i Serra 2018-11-22  367  		dev_err(dev, "No EC dev found\n");
b3074e33 Enric Balletbo i Serra 2018-11-22  368  		return -EINVAL;
b3074e33 Enric Balletbo i Serra 2018-11-22  369  	}
b3074e33 Enric Balletbo i Serra 2018-11-22  370  

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

Re: [PATCH 5/7] mfd / platform: cros_ec: move device sysfs attributes to its own driver.

[Enric Balletbo i Serra]

Hi Dan,

On 29/11/18 12:21, Dan Carpenter wrote:
> Hi Enric,
> 
> I love your patch! Perhaps something to improve:
> 
> url:    https://github.com/0day-ci/linux/commits/Enric-Balletbo-i-Serra/mfd-cros_ec-use-devm_mfd_add_devices/20181123-025253
> 
> smatch warnings:
> drivers/platform/chrome/cros_ec_sysfs.c:366 cros_ec_sysfs_probe() warn: variable dereferenced before check 'ec_dev' (see line 362)
> 
> # https://github.com/0day-ci/linux/commit/b3074e331f36fff8890e7bd5c1f5874f4c59d38f
> git remote add linux-review https://github.com/0day-ci/linux
> git remote update linux-review
> git checkout b3074e331f36fff8890e7bd5c1f5874f4c59d38f
> vim +/ec_dev +366 drivers/platform/chrome/cros_ec_sysfs.c
> 
> b3074e33 Enric Balletbo i Serra 2018-11-22  358  
> b3074e33 Enric Balletbo i Serra 2018-11-22  359  static int cros_ec_sysfs_probe(struct platform_device *pd)
> b3074e33 Enric Balletbo i Serra 2018-11-22  360  {
> b3074e33 Enric Balletbo i Serra 2018-11-22  361  	struct cros_ec_dev *ec_dev = dev_get_drvdata(pd->dev.parent);
> b3074e33 Enric Balletbo i Serra 2018-11-22 @362  	struct cros_ec_platform *ec_platform = dev_get_platdata(ec_dev->dev);
>                                                                                                                 ^^^^^^^^^^^
> b3074e33 Enric Balletbo i Serra 2018-11-22  363  	struct device *dev = &pd->dev;
> b3074e33 Enric Balletbo i Serra 2018-11-22  364  	int ret;
> b3074e33 Enric Balletbo i Serra 2018-11-22  365  
> b3074e33 Enric Balletbo i Serra 2018-11-22 @366  	if (!ec_dev) {
>                                                              ^^^^^^
> Presumable this test can be removed?
> 

Yes, Guenter detected this and I already sent another patchset that removes
this. Thanks.

> b3074e33 Enric Balletbo i Serra 2018-11-22  367  		dev_err(dev, "No EC dev found\n");
> b3074e33 Enric Balletbo i Serra 2018-11-22  368  		return -EINVAL;
> b3074e33 Enric Balletbo i Serra 2018-11-22  369  	}
> b3074e33 Enric Balletbo i Serra 2018-11-22  370  
> 
> ---
> 0-DAY kernel test infrastructure                Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
> 

I had this question on my mind for some time but I didn't look for an answer, in
fact, I'm not sure if I already asked you this before, sorry about that if I
did, the question is. There is a easy way to run your smatch scripts locally to
check the patch before I send to the ML?

Best regards,
 Enric

Re: [PATCH 5/7] mfd / platform: cros_ec: move device sysfs attributes to its own driver.

[Dan Carpenter]

On Thu, Nov 29, 2018 at 03:43:14PM +0100, Enric Balletbo i Serra wrote:
> I had this question on my mind for some time but I didn't look for an answer, in
> fact, I'm not sure if I already asked you this before, sorry about that if I
> did, the question is. There is a easy way to run your smatch scripts locally to
> check the patch before I send to the ML?

Yeah.  Happy to help.  Run ~/smatch/smatch_scripts/kchecker path/to/module.c

It's slightly more limited because it doesn't have the cross function
DB.  Building the DB is easy, but takes four or five hours these days...
~/smatch/smatch_scripts/build_kernel_data.sh

regards,
dan carpenter