[PATCH 00/11] Platform drivers, provide a way to add sysfs groups easily

[PATCH 00/11] Platform drivers, provide a way to add sysfs groups easily

[Greg Kroah-Hartman]

If a platform driver wants to add a sysfs group, it has to do so in a
racy way, adding it after the driver is bound.  To resolve this issue,
have the platform driver core do this for the driver, making the
individual drivers logic smaller and simpler, and solving the race at
the same time.

All of these patches depend on the first patch.  I'll take the first one
through my driver-core tree, and any subsystem maintainer can either ack
their individul patch and I will be glad to also merge it, or they can
wait until after 5.3-rc1 when the core patch hits Linus's tree and then
take it, it's up to them.

Thank to Richard Gong for the idea and the testing of the platform
driver patch.

Greg Kroah-Hartman (11):
  Platform: add a dev_groups pointer to struct platform_driver
  uio: uio_fsl_elbc_gpcm: convert platform driver to use dev_groups
  serial: sh-sci: use driver core functions, not sysfs ones.
  firmware: arm_scpi: convert platform driver to use dev_groups
  olpc: x01: convert platform driver to use dev_groups
  platform: x86: hp-wmi: convert platform driver to use dev_groups
  video: fbdev: wm8505fb: convert platform driver to use dev_groups
  video: fbdev: w100fb: convert platform driver to use dev_groups
  video: fbdev: sm501fb: convert platform driver to use dev_groups
  input: keyboard: gpio_keys: convert platform driver to use dev_groups
  input: axp20x-pek: convert platform driver to use dev_groups

 arch/x86/platform/olpc/olpc-xo1-sci.c | 17 ++++------
 drivers/base/platform.c               | 40 +++++++++++++++--------
 drivers/firmware/arm_scpi.c           |  5 +--
 drivers/input/keyboard/gpio_keys.c    | 13 ++------
 drivers/input/misc/axp20x-pek.c       | 15 ++-------
 drivers/platform/x86/hp-wmi.c         | 47 +++++++--------------------
 drivers/tty/serial/sh-sci.c           | 22 +++++--------
 drivers/uio/uio_fsl_elbc_gpcm.c       | 23 +++++--------
 drivers/video/fbdev/sm501fb.c         | 37 +++++----------------
 drivers/video/fbdev/w100fb.c          | 23 ++++++-------
 drivers/video/fbdev/wm8505fb.c        | 13 ++++----
 include/linux/platform_device.h       |  1 +
 12 files changed, 94 insertions(+), 162 deletions(-)

-- 
2.22.0

[PATCH 00/11] Platform drivers, provide a way to add sysfs groups easily

[Greg Kroah-Hartman]

If a platform driver wants to add a sysfs group, it has to do so in a
racy way, adding it after the driver is bound.  To resolve this issue,
have the platform driver core do this for the driver, making the
individual drivers logic smaller and simpler, and solving the race at
the same time.

All of these patches depend on the first patch.  I'll take the first one
through my driver-core tree, and any subsystem maintainer can either ack
their individul patch and I will be glad to also merge it, or they can
wait until after 5.3-rc1 when the core patch hits Linus's tree and then
take it, it's up to them.

Thank to Richard Gong for the idea and the testing of the platform
driver patch.

Greg Kroah-Hartman (11):
  Platform: add a dev_groups pointer to struct platform_driver
  uio: uio_fsl_elbc_gpcm: convert platform driver to use dev_groups
  serial: sh-sci: use driver core functions, not sysfs ones.
  firmware: arm_scpi: convert platform driver to use dev_groups
  olpc: x01: convert platform driver to use dev_groups
  platform: x86: hp-wmi: convert platform driver to use dev_groups
  video: fbdev: wm8505fb: convert platform driver to use dev_groups
  video: fbdev: w100fb: convert platform driver to use dev_groups
  video: fbdev: sm501fb: convert platform driver to use dev_groups
  input: keyboard: gpio_keys: convert platform driver to use dev_groups
  input: axp20x-pek: convert platform driver to use dev_groups

 arch/x86/platform/olpc/olpc-xo1-sci.c | 17 ++++------
 drivers/base/platform.c               | 40 +++++++++++++++--------
 drivers/firmware/arm_scpi.c           |  5 +--
 drivers/input/keyboard/gpio_keys.c    | 13 ++------
 drivers/input/misc/axp20x-pek.c       | 15 ++-------
 drivers/platform/x86/hp-wmi.c         | 47 +++++++--------------------
 drivers/tty/serial/sh-sci.c           | 22 +++++--------
 drivers/uio/uio_fsl_elbc_gpcm.c       | 23 +++++--------
 drivers/video/fbdev/sm501fb.c         | 37 +++++----------------
 drivers/video/fbdev/w100fb.c          | 23 ++++++-------
 drivers/video/fbdev/wm8505fb.c        | 13 ++++----
 include/linux/platform_device.h       |  1 +
 12 files changed, 94 insertions(+), 162 deletions(-)

-- 
2.22.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 00/11] Platform drivers, provide a way to add sysfs groups easily

[Greg Kroah-Hartman]

If a platform driver wants to add a sysfs group, it has to do so in a
racy way, adding it after the driver is bound.  To resolve this issue,
have the platform driver core do this for the driver, making the
individual drivers logic smaller and simpler, and solving the race at
the same time.

All of these patches depend on the first patch.  I'll take the first one
through my driver-core tree, and any subsystem maintainer can either ack
their individul patch and I will be glad to also merge it, or they can
wait until after 5.3-rc1 when the core patch hits Linus's tree and then
take it, it's up to them.

Thank to Richard Gong for the idea and the testing of the platform
driver patch.

Greg Kroah-Hartman (11):
  Platform: add a dev_groups pointer to struct platform_driver
  uio: uio_fsl_elbc_gpcm: convert platform driver to use dev_groups
  serial: sh-sci: use driver core functions, not sysfs ones.
  firmware: arm_scpi: convert platform driver to use dev_groups
  olpc: x01: convert platform driver to use dev_groups
  platform: x86: hp-wmi: convert platform driver to use dev_groups
  video: fbdev: wm8505fb: convert platform driver to use dev_groups
  video: fbdev: w100fb: convert platform driver to use dev_groups
  video: fbdev: sm501fb: convert platform driver to use dev_groups
  input: keyboard: gpio_keys: convert platform driver to use dev_groups
  input: axp20x-pek: convert platform driver to use dev_groups

 arch/x86/platform/olpc/olpc-xo1-sci.c | 17 ++++------
 drivers/base/platform.c               | 40 +++++++++++++++--------
 drivers/firmware/arm_scpi.c           |  5 +--
 drivers/input/keyboard/gpio_keys.c    | 13 ++------
 drivers/input/misc/axp20x-pek.c       | 15 ++-------
 drivers/platform/x86/hp-wmi.c         | 47 +++++++--------------------
 drivers/tty/serial/sh-sci.c           | 22 +++++--------
 drivers/uio/uio_fsl_elbc_gpcm.c       | 23 +++++--------
 drivers/video/fbdev/sm501fb.c         | 37 +++++----------------
 drivers/video/fbdev/w100fb.c          | 23 ++++++-------
 drivers/video/fbdev/wm8505fb.c        | 13 ++++----
 include/linux/platform_device.h       |  1 +
 12 files changed, 94 insertions(+), 162 deletions(-)

-- 
2.22.0

[PATCH 00/11] Platform drivers, provide a way to add sysfs groups easily

[Greg Kroah-Hartman]

If a platform driver wants to add a sysfs group, it has to do so in a
racy way, adding it after the driver is bound.  To resolve this issue,
have the platform driver core do this for the driver, making the
individual drivers logic smaller and simpler, and solving the race at
the same time.

All of these patches depend on the first patch.  I'll take the first one
through my driver-core tree, and any subsystem maintainer can either ack
their individul patch and I will be glad to also merge it, or they can
wait until after 5.3-rc1 when the core patch hits Linus's tree and then
take it, it's up to them.

Thank to Richard Gong for the idea and the testing of the platform
driver patch.

Greg Kroah-Hartman (11):
  Platform: add a dev_groups pointer to struct platform_driver
  uio: uio_fsl_elbc_gpcm: convert platform driver to use dev_groups
  serial: sh-sci: use driver core functions, not sysfs ones.
  firmware: arm_scpi: convert platform driver to use dev_groups
  olpc: x01: convert platform driver to use dev_groups
  platform: x86: hp-wmi: convert platform driver to use dev_groups
  video: fbdev: wm8505fb: convert platform driver to use dev_groups
  video: fbdev: w100fb: convert platform driver to use dev_groups
  video: fbdev: sm501fb: convert platform driver to use dev_groups
  input: keyboard: gpio_keys: convert platform driver to use dev_groups
  input: axp20x-pek: convert platform driver to use dev_groups

 arch/x86/platform/olpc/olpc-xo1-sci.c | 17 ++++------
 drivers/base/platform.c               | 40 +++++++++++++++--------
 drivers/firmware/arm_scpi.c           |  5 +--
 drivers/input/keyboard/gpio_keys.c    | 13 ++------
 drivers/input/misc/axp20x-pek.c       | 15 ++-------
 drivers/platform/x86/hp-wmi.c         | 47 +++++++--------------------
 drivers/tty/serial/sh-sci.c           | 22 +++++--------
 drivers/uio/uio_fsl_elbc_gpcm.c       | 23 +++++--------
 drivers/video/fbdev/sm501fb.c         | 37 +++++----------------
 drivers/video/fbdev/w100fb.c          | 23 ++++++-------
 drivers/video/fbdev/wm8505fb.c        | 13 ++++----
 include/linux/platform_device.h       |  1 +
 12 files changed, 94 insertions(+), 162 deletions(-)

-- 
2.22.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 01/11] Platform: add a dev_groups pointer to struct platform_driver

[Greg Kroah-Hartman]

Platform drivers like to add sysfs groups to their device, but right now
they have to do it "by hand".  The driver core should handle this for
them, but there is no way to get to the bus-default attribute groups as
all platform devices are "special and unique" one-off drivers/devices.

To combat this, add a dev_groups pointer to platform_driver which allows
a platform driver to set up a list of default attributes that will be
properly created and removed by the platform driver core when a probe()
function is successful and removed right before the device is unbound.

Cc: Richard Gong <richard.gong@linux.intel.com>
Cc: Romain Izard <romain.izard.pro@gmail.com>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Mans Rullgard <mans@mansr.com>
Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Cc: Randy Dunlap <rdunlap@infradead.org>
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/base/platform.c         | 40 +++++++++++++++++++++------------
 include/linux/platform_device.h |  1 +
 2 files changed, 27 insertions(+), 14 deletions(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 713903290385..d81fcd435b52 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -598,6 +598,21 @@ struct platform_device *platform_device_register_full(
 }
 EXPORT_SYMBOL_GPL(platform_device_register_full);
 
+static int platform_drv_remove(struct device *_dev)
+{
+	struct platform_driver *drv = to_platform_driver(_dev->driver);
+	struct platform_device *dev = to_platform_device(_dev);
+	int ret = 0;
+
+	if (drv->remove)
+		ret = drv->remove(dev);
+	if (drv->dev_groups)
+		device_remove_groups(_dev, drv->dev_groups);
+	dev_pm_domain_detach(_dev, true);
+
+	return ret;
+}
+
 static int platform_drv_probe(struct device *_dev)
 {
 	struct platform_driver *drv = to_platform_driver(_dev->driver);
@@ -614,8 +629,18 @@ static int platform_drv_probe(struct device *_dev)
 
 	if (drv->probe) {
 		ret = drv->probe(dev);
-		if (ret)
+		if (ret) {
 			dev_pm_domain_detach(_dev, true);
+			goto out;
+		}
+	}
+	if (drv->dev_groups) {
+		ret = device_add_groups(_dev, drv->dev_groups);
+		if (ret) {
+			platform_drv_remove(_dev);
+			return ret;
+		}
+		kobject_uevent(&_dev->kobj, KOBJ_CHANGE);
 	}
 
 out:
@@ -632,19 +657,6 @@ static int platform_drv_probe_fail(struct device *_dev)
 	return -ENXIO;
 }
 
-static int platform_drv_remove(struct device *_dev)
-{
-	struct platform_driver *drv = to_platform_driver(_dev->driver);
-	struct platform_device *dev = to_platform_device(_dev);
-	int ret = 0;
-
-	if (drv->remove)
-		ret = drv->remove(dev);
-	dev_pm_domain_detach(_dev, true);
-
-	return ret;
-}
-
 static void platform_drv_shutdown(struct device *_dev)
 {
 	struct platform_driver *drv = to_platform_driver(_dev->driver);
diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
index cc464850b71e..027f1e1d7af8 100644
--- a/include/linux/platform_device.h
+++ b/include/linux/platform_device.h
@@ -190,6 +190,7 @@ struct platform_driver {
 	int (*resume)(struct platform_device *);
 	struct device_driver driver;
 	const struct platform_device_id *id_table;
+	const struct attribute_group **dev_groups;
 	bool prevent_deferred_probe;
 };
 
-- 
2.22.0

[PATCH 02/11] uio: uio_fsl_elbc_gpcm: convert platform driver to use dev_groups

[Greg Kroah-Hartman]

Platform drivers now have the option to have the platform core create
and remove any needed sysfs attribute files.  So take advantage of that
and do not register "by hand" a sysfs group of attributes.

Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/uio/uio_fsl_elbc_gpcm.c | 23 ++++++++---------------
 1 file changed, 8 insertions(+), 15 deletions(-)

diff --git a/drivers/uio/uio_fsl_elbc_gpcm.c b/drivers/uio/uio_fsl_elbc_gpcm.c
index 450e2f5c9b43..05a9cb10a419 100644
--- a/drivers/uio/uio_fsl_elbc_gpcm.c
+++ b/drivers/uio/uio_fsl_elbc_gpcm.c
@@ -71,6 +71,13 @@ static ssize_t reg_store(struct device *dev, struct device_attribute *attr,
 static DEVICE_ATTR(reg_br, 0664, reg_show, reg_store);
 static DEVICE_ATTR(reg_or, 0664, reg_show, reg_store);
 
+static struct attribute *uio_fsl_elbc_gpcm_attrs[] = {
+	&dev_attr_reg_br.attr,
+	&dev_attr_reg_or.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(uio_fsl_elbc_gpcm);
+
 static ssize_t reg_show(struct device *dev, struct device_attribute *attr,
 			char *buf)
 {
@@ -411,25 +418,12 @@ static int uio_fsl_elbc_gpcm_probe(struct platform_device *pdev)
 	/* store private data */
 	platform_set_drvdata(pdev, info);
 
-	/* create sysfs files */
-	ret = device_create_file(priv->dev, &dev_attr_reg_br);
-	if (ret)
-		goto out_err3;
-	ret = device_create_file(priv->dev, &dev_attr_reg_or);
-	if (ret)
-		goto out_err4;
-
 	dev_info(priv->dev,
 		 "eLBC/GPCM device (%s) at 0x%llx, bank %d, irq=%d\n",
 		 priv->name, (unsigned long long)res.start, priv->bank,
 		 irq != NO_IRQ ? irq : -1);
 
 	return 0;
-out_err4:
-	device_remove_file(priv->dev, &dev_attr_reg_br);
-out_err3:
-	platform_set_drvdata(pdev, NULL);
-	uio_unregister_device(info);
 out_err2:
 	if (priv->shutdown)
 		priv->shutdown(info, true);
@@ -448,8 +442,6 @@ static int uio_fsl_elbc_gpcm_remove(struct platform_device *pdev)
 	struct uio_info *info = platform_get_drvdata(pdev);
 	struct fsl_elbc_gpcm *priv = info->priv;
 
-	device_remove_file(priv->dev, &dev_attr_reg_or);
-	device_remove_file(priv->dev, &dev_attr_reg_br);
 	platform_set_drvdata(pdev, NULL);
 	uio_unregister_device(info);
 	if (priv->shutdown)
@@ -477,6 +469,7 @@ static struct platform_driver uio_fsl_elbc_gpcm_driver = {
 	},
 	.probe = uio_fsl_elbc_gpcm_probe,
 	.remove = uio_fsl_elbc_gpcm_remove,
+	.dev_groups = uio_fsl_elbc_gpcm_groups,
 };
 module_platform_driver(uio_fsl_elbc_gpcm_driver);
 
-- 
2.22.0

[PATCH 03/11] serial: sh-sci: use driver core functions, not sysfs ones.

[Greg Kroah-Hartman]

This is a driver, do not call "raw" sysfs functions, instead call driver
core ones.  Specifically convert the use of sysfs_create_file() and
sysfs_remove_file() to use device_create_file() and device_remove_file()

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jslaby@suse.com>
Cc: linux-serial@vger.kernel.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/tty/serial/sh-sci.c | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index abc705716aa0..69f9072505f7 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -3137,14 +3137,10 @@ static int sci_remove(struct platform_device *dev)
 
 	sci_cleanup_single(port);
 
-	if (port->port.fifosize > 1) {
-		sysfs_remove_file(&dev->dev.kobj,
-				  &dev_attr_rx_fifo_trigger.attr);
-	}
-	if (type == PORT_SCIFA || type == PORT_SCIFB || type == PORT_HSCIF) {
-		sysfs_remove_file(&dev->dev.kobj,
-				  &dev_attr_rx_fifo_timeout.attr);
-	}
+	if (port->port.fifosize > 1)
+		device_remove_file(&dev->dev, &dev_attr_rx_fifo_trigger);
+	if (type == PORT_SCIFA || type == PORT_SCIFB || type == PORT_HSCIF)
+		device_remove_file(&dev->dev, &dev_attr_rx_fifo_timeout);
 
 	return 0;
 }
@@ -3332,19 +3328,17 @@ static int sci_probe(struct platform_device *dev)
 		return ret;
 
 	if (sp->port.fifosize > 1) {
-		ret = sysfs_create_file(&dev->dev.kobj,
-				&dev_attr_rx_fifo_trigger.attr);
+		ret = device_create_file(&dev->dev, &dev_attr_rx_fifo_trigger);
 		if (ret)
 			return ret;
 	}
 	if (sp->port.type == PORT_SCIFA || sp->port.type == PORT_SCIFB ||
 	    sp->port.type == PORT_HSCIF) {
-		ret = sysfs_create_file(&dev->dev.kobj,
-				&dev_attr_rx_fifo_timeout.attr);
+		ret = device_create_file(&dev->dev, &dev_attr_rx_fifo_timeout);
 		if (ret) {
 			if (sp->port.fifosize > 1) {
-				sysfs_remove_file(&dev->dev.kobj,
-					&dev_attr_rx_fifo_trigger.attr);
+				device_remove_file(&dev->dev,
+						   &dev_attr_rx_fifo_trigger);
 			}
 			return ret;
 		}
-- 
2.22.0

[PATCH 04/11] firmware: arm_scpi: convert platform driver to use dev_groups

[Greg Kroah-Hartman]

Platform drivers now have the option to have the platform core create
and remove any needed sysfs attribute files.  So take advantage of that
and do not register "by hand" a sysfs group of attributes.

Cc: Sudeep Holla <sudeep.holla@arm.com>
Cc: linux-arm-kernel@lists.infradead.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/firmware/arm_scpi.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
index 725164b83242..2774ec886d60 100644
--- a/drivers/firmware/arm_scpi.c
+++ b/drivers/firmware/arm_scpi.c
@@ -1011,10 +1011,6 @@ static int scpi_probe(struct platform_device *pdev)
 				   scpi_info->firmware_version));
 	scpi_info->scpi_ops = &scpi_ops;
 
-	ret = devm_device_add_groups(dev, versions_groups);
-	if (ret)
-		dev_err(dev, "unable to create sysfs version group\n");
-
 	return devm_of_platform_populate(dev);
 }
 
@@ -1033,6 +1029,7 @@ static struct platform_driver scpi_driver = {
 	},
 	.probe = scpi_probe,
 	.remove = scpi_remove,
+	.dev_groups = versions_groups,
 };
 module_platform_driver(scpi_driver);
 
-- 
2.22.0

[PATCH 04/11] firmware: arm_scpi: convert platform driver to use dev_groups

[Greg Kroah-Hartman]

Platform drivers now have the option to have the platform core create
and remove any needed sysfs attribute files.  So take advantage of that
and do not register "by hand" a sysfs group of attributes.

Cc: Sudeep Holla <sudeep.holla@arm.com>
Cc: linux-arm-kernel@lists.infradead.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/firmware/arm_scpi.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
index 725164b83242..2774ec886d60 100644
--- a/drivers/firmware/arm_scpi.c
+++ b/drivers/firmware/arm_scpi.c
@@ -1011,10 +1011,6 @@ static int scpi_probe(struct platform_device *pdev)
 				   scpi_info->firmware_version));
 	scpi_info->scpi_ops = &scpi_ops;
 
-	ret = devm_device_add_groups(dev, versions_groups);
-	if (ret)
-		dev_err(dev, "unable to create sysfs version group\n");
-
 	return devm_of_platform_populate(dev);
 }
 
@@ -1033,6 +1029,7 @@ static struct platform_driver scpi_driver = {
 	},
 	.probe = scpi_probe,
 	.remove = scpi_remove,
+	.dev_groups = versions_groups,
 };
 module_platform_driver(scpi_driver);
 
-- 
2.22.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 05/11] olpc: x01: convert platform driver to use dev_groups

[Greg Kroah-Hartman]

Platform drivers now have the option to have the platform core create
and remove any needed sysfs attribute files.  So take advantage of that
and do not register "by hand" a lid sysfs file.

Cc: Darren Hart <dvhart@infradead.org>
Cc: Andy Shevchenko <andy@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: x86@kernel.org
Cc: platform-driver-x86@vger.kernel.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 arch/x86/platform/olpc/olpc-xo1-sci.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/arch/x86/platform/olpc/olpc-xo1-sci.c b/arch/x86/platform/olpc/olpc-xo1-sci.c
index 25ce1b3b0732..ce1948918dd2 100644
--- a/arch/x86/platform/olpc/olpc-xo1-sci.c
+++ b/arch/x86/platform/olpc/olpc-xo1-sci.c
@@ -157,6 +157,12 @@ static ssize_t lid_wake_mode_set(struct device *dev,
 static DEVICE_ATTR(lid_wake_mode, S_IWUSR | S_IRUGO, lid_wake_mode_show,
 		   lid_wake_mode_set);
 
+static struct attribute *lid_attrs[] = {
+	&dev_attr_lid_wake_mode.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(lid);
+
 /*
  * Process all items in the EC's SCI queue.
  *
@@ -510,17 +516,8 @@ static int setup_lid_switch(struct platform_device *pdev)
 		goto err_register;
 	}
 
-	r = device_create_file(&lid_switch_idev->dev, &dev_attr_lid_wake_mode);
-	if (r) {
-		dev_err(&pdev->dev, "failed to create wake mode attr: %d\n", r);
-		goto err_create_attr;
-	}
-
 	return 0;
 
-err_create_attr:
-	input_unregister_device(lid_switch_idev);
-	lid_switch_idev = NULL;
 err_register:
 	input_free_device(lid_switch_idev);
 	return r;
@@ -528,7 +525,6 @@ static int setup_lid_switch(struct platform_device *pdev)
 
 static void free_lid_switch(void)
 {
-	device_remove_file(&lid_switch_idev->dev, &dev_attr_lid_wake_mode);
 	input_unregister_device(lid_switch_idev);
 }
 
@@ -629,6 +625,7 @@ static struct platform_driver xo1_sci_driver = {
 	.remove = xo1_sci_remove,
 	.suspend = xo1_sci_suspend,
 	.resume = xo1_sci_resume,
+	.dev_groups = lid_groups,
 };
 
 static int __init xo1_sci_init(void)
-- 
2.22.0

[PATCH 06/11] platform: x86: hp-wmi: convert platform driver to use dev_groups

[Greg Kroah-Hartman]

Platform drivers now have the option to have the platform core create
and remove any needed sysfs attribute files.  So take advantage of that
and do not register "by hand" a bunch of sysfs files.

Cc: Darren Hart <dvhart@infradead.org>
Cc: Andy Shevchenko <andy@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: x86@kernel.org
Cc: platform-driver-x86@vger.kernel.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/platform/x86/hp-wmi.c | 47 +++++++++--------------------------
 1 file changed, 12 insertions(+), 35 deletions(-)

diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c
index 2521e45280b8..b4ed5902737a 100644
--- a/drivers/platform/x86/hp-wmi.c
+++ b/drivers/platform/x86/hp-wmi.c
@@ -502,6 +502,17 @@ static DEVICE_ATTR_RO(dock);
 static DEVICE_ATTR_RO(tablet);
 static DEVICE_ATTR_RW(postcode);
 
+static struct attribute *hp_wmi_attrs[] = {
+	&dev_attr_display.attr,
+	&dev_attr_hddtemp.attr,
+	&dev_attr_als.attr,
+	&dev_attr_dock.attr,
+	&dev_attr_tablet.attr,
+	&dev_attr_postcode.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(hp_wmi);
+
 static void hp_wmi_notify(u32 value, void *context)
 {
 	struct acpi_buffer response = { ACPI_ALLOCATE_BUFFER, NULL };
@@ -678,16 +689,6 @@ static void hp_wmi_input_destroy(void)
 	input_unregister_device(hp_wmi_input_dev);
 }
 
-static void cleanup_sysfs(struct platform_device *device)
-{
-	device_remove_file(&device->dev, &dev_attr_display);
-	device_remove_file(&device->dev, &dev_attr_hddtemp);
-	device_remove_file(&device->dev, &dev_attr_als);
-	device_remove_file(&device->dev, &dev_attr_dock);
-	device_remove_file(&device->dev, &dev_attr_tablet);
-	device_remove_file(&device->dev, &dev_attr_postcode);
-}
-
 static int __init hp_wmi_rfkill_setup(struct platform_device *device)
 {
 	int err, wireless;
@@ -858,8 +859,6 @@ static int __init hp_wmi_rfkill2_setup(struct platform_device *device)
 
 static int __init hp_wmi_bios_setup(struct platform_device *device)
 {
-	int err;
-
 	/* clear detected rfkill devices */
 	wifi_rfkill = NULL;
 	bluetooth_rfkill = NULL;
@@ -869,35 +868,12 @@ static int __init hp_wmi_bios_setup(struct platform_device *device)
 	if (hp_wmi_rfkill_setup(device))
 		hp_wmi_rfkill2_setup(device);
 
-	err = device_create_file(&device->dev, &dev_attr_display);
-	if (err)
-		goto add_sysfs_error;
-	err = device_create_file(&device->dev, &dev_attr_hddtemp);
-	if (err)
-		goto add_sysfs_error;
-	err = device_create_file(&device->dev, &dev_attr_als);
-	if (err)
-		goto add_sysfs_error;
-	err = device_create_file(&device->dev, &dev_attr_dock);
-	if (err)
-		goto add_sysfs_error;
-	err = device_create_file(&device->dev, &dev_attr_tablet);
-	if (err)
-		goto add_sysfs_error;
-	err = device_create_file(&device->dev, &dev_attr_postcode);
-	if (err)
-		goto add_sysfs_error;
 	return 0;
-
-add_sysfs_error:
-	cleanup_sysfs(device);
-	return err;
 }
 
 static int __exit hp_wmi_bios_remove(struct platform_device *device)
 {
 	int i;
-	cleanup_sysfs(device);
 
 	for (i = 0; i < rfkill2_count; i++) {
 		rfkill_unregister(rfkill2[i].rfkill);
@@ -968,6 +944,7 @@ static struct platform_driver hp_wmi_driver = {
 		.pm = &hp_wmi_pm_ops,
 	},
 	.remove = __exit_p(hp_wmi_bios_remove),
+	.dev_groups = hp_wmi_groups,
 };
 
 static int __init hp_wmi_init(void)
-- 
2.22.0

[PATCH 07/11] video: fbdev: wm8505fb: convert platform driver to use dev_groups

[Greg Kroah-Hartman]

Platform drivers now have the option to have the platform core create
and remove any needed sysfs attribute files.  So take advantage of that
and do not register "by hand" a sysfs file.

Cc: Darren Hart <dvhart@infradead.org>
Cc: Andy Shevchenko <andy@infradead.org>
Cc: platform-driver-x86@vger.kernel.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/video/fbdev/wm8505fb.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/video/fbdev/wm8505fb.c b/drivers/video/fbdev/wm8505fb.c
index 8f0d5379861d..3b826da97035 100644
--- a/drivers/video/fbdev/wm8505fb.c
+++ b/drivers/video/fbdev/wm8505fb.c
@@ -184,6 +184,12 @@ static ssize_t contrast_store(struct device *dev,
 
 static DEVICE_ATTR_RW(contrast);
 
+static struct attribute *wm8505fb_attrs[] = {
+	&dev_attr_contrast.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(wm8505fb);
+
 static inline u_int chan_to_field(u_int chan, struct fb_bitfield *bf)
 {
 	chan &= 0xffff;
@@ -369,10 +375,6 @@ static int wm8505fb_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	ret = device_create_file(&pdev->dev, &dev_attr_contrast);
-	if (ret < 0)
-		fb_warn(&fbi->fb, "failed to register attributes (%d)\n", ret);
-
 	fb_info(&fbi->fb, "%s frame buffer at 0x%lx-0x%lx\n",
 		fbi->fb.fix.id, fbi->fb.fix.smem_start,
 		fbi->fb.fix.smem_start + fbi->fb.fix.smem_len - 1);
@@ -384,8 +386,6 @@ static int wm8505fb_remove(struct platform_device *pdev)
 {
 	struct wm8505fb_info *fbi = platform_get_drvdata(pdev);
 
-	device_remove_file(&pdev->dev, &dev_attr_contrast);
-
 	unregister_framebuffer(&fbi->fb);
 
 	writel(0, fbi->regbase);
@@ -402,6 +402,7 @@ static const struct of_device_id wmt_dt_ids[] = {
 };
 
 static struct platform_driver wm8505fb_driver = {
+	.dev_groups	= wm8505fb_groups,
 	.probe		= wm8505fb_probe,
 	.remove		= wm8505fb_remove,
 	.driver		= {
-- 
2.22.0

[PATCH 08/11] video: fbdev: w100fb: convert platform driver to use dev_groups

[Greg Kroah-Hartman]

Platform drivers now have the option to have the platform core create
and remove any needed sysfs attribute files.  So take advantage of that
and do not register "by hand" a bunch of sysfs files.

Cc: Tony Prisk <linux@prisktech.co.nz>
Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: dri-devel@lists.freedesktop.org
Cc: linux-fbdev@vger.kernel.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/video/fbdev/w100fb.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/drivers/video/fbdev/w100fb.c b/drivers/video/fbdev/w100fb.c
index 696106ecdff0..4be3afcc1c93 100644
--- a/drivers/video/fbdev/w100fb.c
+++ b/drivers/video/fbdev/w100fb.c
@@ -168,6 +168,15 @@ static ssize_t fastpllclk_store(struct device *dev, struct device_attribute *att
 
 static DEVICE_ATTR_RW(fastpllclk);
 
+static struct attribute *w100fb_attrs[] = {
+	&dev_attr_fastpllclk.attr,
+	&dev_attr_reg_read.attr,
+	&dev_attr_reg_write.attr,
+	&dev_attr_flip.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(w100fb);
+
 /*
  * Some touchscreens need hsync information from the video driver to
  * function correctly. We export it here.
@@ -756,14 +765,6 @@ int w100fb_probe(struct platform_device *pdev)
 		goto out;
 	}
 
-	err = device_create_file(&pdev->dev, &dev_attr_fastpllclk);
-	err |= device_create_file(&pdev->dev, &dev_attr_reg_read);
-	err |= device_create_file(&pdev->dev, &dev_attr_reg_write);
-	err |= device_create_file(&pdev->dev, &dev_attr_flip);
-
-	if (err != 0)
-		fb_warn(info, "failed to register attributes (%d)\n", err);
-
 	fb_info(info, "%s frame buffer device\n", info->fix.id);
 	return 0;
 out:
@@ -788,11 +789,6 @@ static int w100fb_remove(struct platform_device *pdev)
 	struct fb_info *info = platform_get_drvdata(pdev);
 	struct w100fb_par *par=info->par;
 
-	device_remove_file(&pdev->dev, &dev_attr_fastpllclk);
-	device_remove_file(&pdev->dev, &dev_attr_reg_read);
-	device_remove_file(&pdev->dev, &dev_attr_reg_write);
-	device_remove_file(&pdev->dev, &dev_attr_flip);
-
 	unregister_framebuffer(info);
 
 	vfree(par->saved_intmem);
@@ -1630,6 +1626,7 @@ static struct platform_driver w100fb_driver = {
 	.driver		= {
 		.name	= "w100fb",
 	},
+	.dev_groups	= w100fb_groups,
 };
 
 module_platform_driver(w100fb_driver);
-- 
2.22.0

[PATCH 08/11] video: fbdev: w100fb: convert platform driver to use dev_groups

[Greg Kroah-Hartman]

Platform drivers now have the option to have the platform core create
and remove any needed sysfs attribute files.  So take advantage of that
and do not register "by hand" a bunch of sysfs files.

Cc: Tony Prisk <linux@prisktech.co.nz>
Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: dri-devel@lists.freedesktop.org
Cc: linux-fbdev@vger.kernel.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/video/fbdev/w100fb.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/drivers/video/fbdev/w100fb.c b/drivers/video/fbdev/w100fb.c
index 696106ecdff0..4be3afcc1c93 100644
--- a/drivers/video/fbdev/w100fb.c
+++ b/drivers/video/fbdev/w100fb.c
@@ -168,6 +168,15 @@ static ssize_t fastpllclk_store(struct device *dev, struct device_attribute *att
 
 static DEVICE_ATTR_RW(fastpllclk);
 
+static struct attribute *w100fb_attrs[] = {
+	&dev_attr_fastpllclk.attr,
+	&dev_attr_reg_read.attr,
+	&dev_attr_reg_write.attr,
+	&dev_attr_flip.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(w100fb);
+
 /*
  * Some touchscreens need hsync information from the video driver to
  * function correctly. We export it here.
@@ -756,14 +765,6 @@ int w100fb_probe(struct platform_device *pdev)
 		goto out;
 	}
 
-	err = device_create_file(&pdev->dev, &dev_attr_fastpllclk);
-	err |= device_create_file(&pdev->dev, &dev_attr_reg_read);
-	err |= device_create_file(&pdev->dev, &dev_attr_reg_write);
-	err |= device_create_file(&pdev->dev, &dev_attr_flip);
-
-	if (err != 0)
-		fb_warn(info, "failed to register attributes (%d)\n", err);
-
 	fb_info(info, "%s frame buffer device\n", info->fix.id);
 	return 0;
 out:
@@ -788,11 +789,6 @@ static int w100fb_remove(struct platform_device *pdev)
 	struct fb_info *info = platform_get_drvdata(pdev);
 	struct w100fb_par *par=info->par;
 
-	device_remove_file(&pdev->dev, &dev_attr_fastpllclk);
-	device_remove_file(&pdev->dev, &dev_attr_reg_read);
-	device_remove_file(&pdev->dev, &dev_attr_reg_write);
-	device_remove_file(&pdev->dev, &dev_attr_flip);
-
 	unregister_framebuffer(info);
 
 	vfree(par->saved_intmem);
@@ -1630,6 +1626,7 @@ static struct platform_driver w100fb_driver = {
 	.driver		= {
 		.name	= "w100fb",
 	},
+	.dev_groups	= w100fb_groups,
 };
 
 module_platform_driver(w100fb_driver);
-- 
2.22.0

[PATCH 08/11] video: fbdev: w100fb: convert platform driver to use dev_groups

[Greg Kroah-Hartman]

Platform drivers now have the option to have the platform core create
and remove any needed sysfs attribute files.  So take advantage of that
and do not register "by hand" a bunch of sysfs files.

Cc: Tony Prisk <linux@prisktech.co.nz>
Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: dri-devel@lists.freedesktop.org
Cc: linux-fbdev@vger.kernel.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/video/fbdev/w100fb.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/drivers/video/fbdev/w100fb.c b/drivers/video/fbdev/w100fb.c
index 696106ecdff0..4be3afcc1c93 100644
--- a/drivers/video/fbdev/w100fb.c
+++ b/drivers/video/fbdev/w100fb.c
@@ -168,6 +168,15 @@ static ssize_t fastpllclk_store(struct device *dev, struct device_attribute *att
 
 static DEVICE_ATTR_RW(fastpllclk);
 
+static struct attribute *w100fb_attrs[] = {
+	&dev_attr_fastpllclk.attr,
+	&dev_attr_reg_read.attr,
+	&dev_attr_reg_write.attr,
+	&dev_attr_flip.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(w100fb);
+
 /*
  * Some touchscreens need hsync information from the video driver to
  * function correctly. We export it here.
@@ -756,14 +765,6 @@ int w100fb_probe(struct platform_device *pdev)
 		goto out;
 	}
 
-	err = device_create_file(&pdev->dev, &dev_attr_fastpllclk);
-	err |= device_create_file(&pdev->dev, &dev_attr_reg_read);
-	err |= device_create_file(&pdev->dev, &dev_attr_reg_write);
-	err |= device_create_file(&pdev->dev, &dev_attr_flip);
-
-	if (err != 0)
-		fb_warn(info, "failed to register attributes (%d)\n", err);
-
 	fb_info(info, "%s frame buffer device\n", info->fix.id);
 	return 0;
 out:
@@ -788,11 +789,6 @@ static int w100fb_remove(struct platform_device *pdev)
 	struct fb_info *info = platform_get_drvdata(pdev);
 	struct w100fb_par *par=info->par;
 
-	device_remove_file(&pdev->dev, &dev_attr_fastpllclk);
-	device_remove_file(&pdev->dev, &dev_attr_reg_read);
-	device_remove_file(&pdev->dev, &dev_attr_reg_write);
-	device_remove_file(&pdev->dev, &dev_attr_flip);
-
 	unregister_framebuffer(info);
 
 	vfree(par->saved_intmem);
@@ -1630,6 +1626,7 @@ static struct platform_driver w100fb_driver = {
 	.driver		= {
 		.name	= "w100fb",
 	},
+	.dev_groups	= w100fb_groups,
 };
 
 module_platform_driver(w100fb_driver);
-- 
2.22.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 08/11] video: fbdev: w100fb: convert platform driver to use dev_groups

[Greg Kroah-Hartman]

Platform drivers now have the option to have the platform core create
and remove any needed sysfs attribute files.  So take advantage of that
and do not register "by hand" a bunch of sysfs files.

Cc: Tony Prisk <linux@prisktech.co.nz>
Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: dri-devel@lists.freedesktop.org
Cc: linux-fbdev@vger.kernel.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/video/fbdev/w100fb.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/drivers/video/fbdev/w100fb.c b/drivers/video/fbdev/w100fb.c
index 696106ecdff0..4be3afcc1c93 100644
--- a/drivers/video/fbdev/w100fb.c
+++ b/drivers/video/fbdev/w100fb.c
@@ -168,6 +168,15 @@ static ssize_t fastpllclk_store(struct device *dev, struct device_attribute *att
 
 static DEVICE_ATTR_RW(fastpllclk);
 
+static struct attribute *w100fb_attrs[] = {
+	&dev_attr_fastpllclk.attr,
+	&dev_attr_reg_read.attr,
+	&dev_attr_reg_write.attr,
+	&dev_attr_flip.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(w100fb);
+
 /*
  * Some touchscreens need hsync information from the video driver to
  * function correctly. We export it here.
@@ -756,14 +765,6 @@ int w100fb_probe(struct platform_device *pdev)
 		goto out;
 	}
 
-	err = device_create_file(&pdev->dev, &dev_attr_fastpllclk);
-	err |= device_create_file(&pdev->dev, &dev_attr_reg_read);
-	err |= device_create_file(&pdev->dev, &dev_attr_reg_write);
-	err |= device_create_file(&pdev->dev, &dev_attr_flip);
-
-	if (err != 0)
-		fb_warn(info, "failed to register attributes (%d)\n", err);
-
 	fb_info(info, "%s frame buffer device\n", info->fix.id);
 	return 0;
 out:
@@ -788,11 +789,6 @@ static int w100fb_remove(struct platform_device *pdev)
 	struct fb_info *info = platform_get_drvdata(pdev);
 	struct w100fb_par *par=info->par;
 
-	device_remove_file(&pdev->dev, &dev_attr_fastpllclk);
-	device_remove_file(&pdev->dev, &dev_attr_reg_read);
-	device_remove_file(&pdev->dev, &dev_attr_reg_write);
-	device_remove_file(&pdev->dev, &dev_attr_flip);
-
 	unregister_framebuffer(info);
 
 	vfree(par->saved_intmem);
@@ -1630,6 +1626,7 @@ static struct platform_driver w100fb_driver = {
 	.driver		= {
 		.name	= "w100fb",
 	},
+	.dev_groups	= w100fb_groups,
 };
 
 module_platform_driver(w100fb_driver);
-- 
2.22.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 09/11] video: fbdev: sm501fb: convert platform driver to use dev_groups

[Greg Kroah-Hartman]

Platform drivers now have the option to have the platform core create
and remove any needed sysfs attribute files.  So take advantage of that
and do not register "by hand" a bunch of sysfs files.

Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Cc: dri-devel@lists.freedesktop.org
Cc: linux-fbdev@vger.kernel.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/video/fbdev/sm501fb.c | 37 +++++++++--------------------------
 1 file changed, 9 insertions(+), 28 deletions(-)

diff --git a/drivers/video/fbdev/sm501fb.c b/drivers/video/fbdev/sm501fb.c
index dde52d027416..c633ee76a73d 100644
--- a/drivers/video/fbdev/sm501fb.c
+++ b/drivers/video/fbdev/sm501fb.c
@@ -1274,6 +1274,14 @@ static ssize_t sm501fb_debug_show_pnl(struct device *dev,
 
 static DEVICE_ATTR(fbregs_pnl, 0444, sm501fb_debug_show_pnl, NULL);
 
+static struct attribute *sm501fb_attrs[] = {
+	&dev_attr_crt_src.attr,
+	&dev_attr_fbregs_pnl.attr,
+	&dev_attr_fbregs_crt.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(sm501fb);
+
 /* acceleration operations */
 static int sm501fb_sync(struct fb_info *info)
 {
@@ -2016,33 +2024,9 @@ static int sm501fb_probe(struct platform_device *pdev)
 		goto err_started_crt;
 	}
 
-	/* create device files */
-
-	ret = device_create_file(dev, &dev_attr_crt_src);
-	if (ret)
-		goto err_started_panel;
-
-	ret = device_create_file(dev, &dev_attr_fbregs_pnl);
-	if (ret)
-		goto err_attached_crtsrc_file;
-
-	ret = device_create_file(dev, &dev_attr_fbregs_crt);
-	if (ret)
-		goto err_attached_pnlregs_file;
-
 	/* we registered, return ok */
 	return 0;
 
-err_attached_pnlregs_file:
-	device_remove_file(dev, &dev_attr_fbregs_pnl);
-
-err_attached_crtsrc_file:
-	device_remove_file(dev, &dev_attr_crt_src);
-
-err_started_panel:
-	unregister_framebuffer(info->fb[HEAD_PANEL]);
-	sm501_free_init_fb(info, HEAD_PANEL);
-
 err_started_crt:
 	unregister_framebuffer(info->fb[HEAD_CRT]);
 	sm501_free_init_fb(info, HEAD_CRT);
@@ -2072,10 +2056,6 @@ static int sm501fb_remove(struct platform_device *pdev)
 	struct fb_info	   *fbinfo_crt = info->fb[0];
 	struct fb_info	   *fbinfo_pnl = info->fb[1];
 
-	device_remove_file(&pdev->dev, &dev_attr_fbregs_crt);
-	device_remove_file(&pdev->dev, &dev_attr_fbregs_pnl);
-	device_remove_file(&pdev->dev, &dev_attr_crt_src);
-
 	sm501_free_init_fb(info, HEAD_CRT);
 	sm501_free_init_fb(info, HEAD_PANEL);
 
@@ -2233,6 +2213,7 @@ static int sm501fb_resume(struct platform_device *pdev)
 #endif
 
 static struct platform_driver sm501fb_driver = {
+	.dev_groups	= sm501fb_groups,
 	.probe		= sm501fb_probe,
 	.remove		= sm501fb_remove,
 	.suspend	= sm501fb_suspend,
-- 
2.22.0

[PATCH 09/11] video: fbdev: sm501fb: convert platform driver to use dev_groups

[Greg Kroah-Hartman]

Platform drivers now have the option to have the platform core create
and remove any needed sysfs attribute files.  So take advantage of that
and do not register "by hand" a bunch of sysfs files.

Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Cc: dri-devel@lists.freedesktop.org
Cc: linux-fbdev@vger.kernel.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/video/fbdev/sm501fb.c | 37 +++++++++--------------------------
 1 file changed, 9 insertions(+), 28 deletions(-)

diff --git a/drivers/video/fbdev/sm501fb.c b/drivers/video/fbdev/sm501fb.c
index dde52d027416..c633ee76a73d 100644
--- a/drivers/video/fbdev/sm501fb.c
+++ b/drivers/video/fbdev/sm501fb.c
@@ -1274,6 +1274,14 @@ static ssize_t sm501fb_debug_show_pnl(struct device *dev,
 
 static DEVICE_ATTR(fbregs_pnl, 0444, sm501fb_debug_show_pnl, NULL);
 
+static struct attribute *sm501fb_attrs[] = {
+	&dev_attr_crt_src.attr,
+	&dev_attr_fbregs_pnl.attr,
+	&dev_attr_fbregs_crt.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(sm501fb);
+
 /* acceleration operations */
 static int sm501fb_sync(struct fb_info *info)
 {
@@ -2016,33 +2024,9 @@ static int sm501fb_probe(struct platform_device *pdev)
 		goto err_started_crt;
 	}
 
-	/* create device files */
-
-	ret = device_create_file(dev, &dev_attr_crt_src);
-	if (ret)
-		goto err_started_panel;
-
-	ret = device_create_file(dev, &dev_attr_fbregs_pnl);
-	if (ret)
-		goto err_attached_crtsrc_file;
-
-	ret = device_create_file(dev, &dev_attr_fbregs_crt);
-	if (ret)
-		goto err_attached_pnlregs_file;
-
 	/* we registered, return ok */
 	return 0;
 
-err_attached_pnlregs_file:
-	device_remove_file(dev, &dev_attr_fbregs_pnl);
-
-err_attached_crtsrc_file:
-	device_remove_file(dev, &dev_attr_crt_src);
-
-err_started_panel:
-	unregister_framebuffer(info->fb[HEAD_PANEL]);
-	sm501_free_init_fb(info, HEAD_PANEL);
-
 err_started_crt:
 	unregister_framebuffer(info->fb[HEAD_CRT]);
 	sm501_free_init_fb(info, HEAD_CRT);
@@ -2072,10 +2056,6 @@ static int sm501fb_remove(struct platform_device *pdev)
 	struct fb_info	   *fbinfo_crt = info->fb[0];
 	struct fb_info	   *fbinfo_pnl = info->fb[1];
 
-	device_remove_file(&pdev->dev, &dev_attr_fbregs_crt);
-	device_remove_file(&pdev->dev, &dev_attr_fbregs_pnl);
-	device_remove_file(&pdev->dev, &dev_attr_crt_src);
-
 	sm501_free_init_fb(info, HEAD_CRT);
 	sm501_free_init_fb(info, HEAD_PANEL);
 
@@ -2233,6 +2213,7 @@ static int sm501fb_resume(struct platform_device *pdev)
 #endif
 
 static struct platform_driver sm501fb_driver = {
+	.dev_groups	= sm501fb_groups,
 	.probe		= sm501fb_probe,
 	.remove		= sm501fb_remove,
 	.suspend	= sm501fb_suspend,
-- 
2.22.0

[PATCH 09/11] video: fbdev: sm501fb: convert platform driver to use dev_groups

[Greg Kroah-Hartman]

Platform drivers now have the option to have the platform core create
and remove any needed sysfs attribute files.  So take advantage of that
and do not register "by hand" a bunch of sysfs files.

Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Cc: dri-devel@lists.freedesktop.org
Cc: linux-fbdev@vger.kernel.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/video/fbdev/sm501fb.c | 37 +++++++++--------------------------
 1 file changed, 9 insertions(+), 28 deletions(-)

diff --git a/drivers/video/fbdev/sm501fb.c b/drivers/video/fbdev/sm501fb.c
index dde52d027416..c633ee76a73d 100644
--- a/drivers/video/fbdev/sm501fb.c
+++ b/drivers/video/fbdev/sm501fb.c
@@ -1274,6 +1274,14 @@ static ssize_t sm501fb_debug_show_pnl(struct device *dev,
 
 static DEVICE_ATTR(fbregs_pnl, 0444, sm501fb_debug_show_pnl, NULL);
 
+static struct attribute *sm501fb_attrs[] = {
+	&dev_attr_crt_src.attr,
+	&dev_attr_fbregs_pnl.attr,
+	&dev_attr_fbregs_crt.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(sm501fb);
+
 /* acceleration operations */
 static int sm501fb_sync(struct fb_info *info)
 {
@@ -2016,33 +2024,9 @@ static int sm501fb_probe(struct platform_device *pdev)
 		goto err_started_crt;
 	}
 
-	/* create device files */
-
-	ret = device_create_file(dev, &dev_attr_crt_src);
-	if (ret)
-		goto err_started_panel;
-
-	ret = device_create_file(dev, &dev_attr_fbregs_pnl);
-	if (ret)
-		goto err_attached_crtsrc_file;
-
-	ret = device_create_file(dev, &dev_attr_fbregs_crt);
-	if (ret)
-		goto err_attached_pnlregs_file;
-
 	/* we registered, return ok */
 	return 0;
 
-err_attached_pnlregs_file:
-	device_remove_file(dev, &dev_attr_fbregs_pnl);
-
-err_attached_crtsrc_file:
-	device_remove_file(dev, &dev_attr_crt_src);
-
-err_started_panel:
-	unregister_framebuffer(info->fb[HEAD_PANEL]);
-	sm501_free_init_fb(info, HEAD_PANEL);
-
 err_started_crt:
 	unregister_framebuffer(info->fb[HEAD_CRT]);
 	sm501_free_init_fb(info, HEAD_CRT);
@@ -2072,10 +2056,6 @@ static int sm501fb_remove(struct platform_device *pdev)
 	struct fb_info	   *fbinfo_crt = info->fb[0];
 	struct fb_info	   *fbinfo_pnl = info->fb[1];
 
-	device_remove_file(&pdev->dev, &dev_attr_fbregs_crt);
-	device_remove_file(&pdev->dev, &dev_attr_fbregs_pnl);
-	device_remove_file(&pdev->dev, &dev_attr_crt_src);
-
 	sm501_free_init_fb(info, HEAD_CRT);
 	sm501_free_init_fb(info, HEAD_PANEL);
 
@@ -2233,6 +2213,7 @@ static int sm501fb_resume(struct platform_device *pdev)
 #endif
 
 static struct platform_driver sm501fb_driver = {
+	.dev_groups	= sm501fb_groups,
 	.probe		= sm501fb_probe,
 	.remove		= sm501fb_remove,
 	.suspend	= sm501fb_suspend,
-- 
2.22.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 10/11] input: keyboard: gpio_keys: convert platform driver to use dev_groups

[Greg Kroah-Hartman]

Platform drivers now have the option to have the platform core create
and remove any needed sysfs attribute files.  So take advantage of that
and do not register "by hand" a bunch of sysfs files.

Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Cc: dri-devel@lists.freedesktop.org
Cc: linux-fbdev@vger.kernel.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/input/keyboard/gpio_keys.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index 6cd199e8a370..1f6547440edb 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -354,10 +354,7 @@ static struct attribute *gpio_keys_attrs[] = {
 	&dev_attr_disabled_switches.attr,
 	NULL,
 };
-
-static const struct attribute_group gpio_keys_attr_group = {
-	.attrs = gpio_keys_attrs,
-};
+ATTRIBUTE_GROUPS(gpio_keys);
 
 static void gpio_keys_gpio_report_event(struct gpio_button_data *bdata)
 {
@@ -856,13 +853,6 @@ static int gpio_keys_probe(struct platform_device *pdev)
 
 	fwnode_handle_put(child);
 
-	error = devm_device_add_group(dev, &gpio_keys_attr_group);
-	if (error) {
-		dev_err(dev, "Unable to export keys/switches, error: %d\n",
-			error);
-		return error;
-	}
-
 	error = input_register_device(input);
 	if (error) {
 		dev_err(dev, "Unable to register input device, error: %d\n",
@@ -1025,6 +1015,7 @@ static void gpio_keys_shutdown(struct platform_device *pdev)
 }
 
 static struct platform_driver gpio_keys_device_driver = {
+	.dev_groups	= gpio_keys_groups,
 	.probe		= gpio_keys_probe,
 	.shutdown	= gpio_keys_shutdown,
 	.driver		= {
-- 
2.22.0

[PATCH 10/11] input: keyboard: gpio_keys: convert platform driver to use dev_groups

[Greg Kroah-Hartman]

Platform drivers now have the option to have the platform core create
and remove any needed sysfs attribute files.  So take advantage of that
and do not register "by hand" a bunch of sysfs files.

Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Cc: dri-devel@lists.freedesktop.org
Cc: linux-fbdev@vger.kernel.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/input/keyboard/gpio_keys.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index 6cd199e8a370..1f6547440edb 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -354,10 +354,7 @@ static struct attribute *gpio_keys_attrs[] = {
 	&dev_attr_disabled_switches.attr,
 	NULL,
 };
-
-static const struct attribute_group gpio_keys_attr_group = {
-	.attrs = gpio_keys_attrs,
-};
+ATTRIBUTE_GROUPS(gpio_keys);
 
 static void gpio_keys_gpio_report_event(struct gpio_button_data *bdata)
 {
@@ -856,13 +853,6 @@ static int gpio_keys_probe(struct platform_device *pdev)
 
 	fwnode_handle_put(child);
 
-	error = devm_device_add_group(dev, &gpio_keys_attr_group);
-	if (error) {
-		dev_err(dev, "Unable to export keys/switches, error: %d\n",
-			error);
-		return error;
-	}
-
 	error = input_register_device(input);
 	if (error) {
 		dev_err(dev, "Unable to register input device, error: %d\n",
@@ -1025,6 +1015,7 @@ static void gpio_keys_shutdown(struct platform_device *pdev)
 }
 
 static struct platform_driver gpio_keys_device_driver = {
+	.dev_groups	= gpio_keys_groups,
 	.probe		= gpio_keys_probe,
 	.shutdown	= gpio_keys_shutdown,
 	.driver		= {
-- 
2.22.0

[PATCH 11/11] input: axp20x-pek: convert platform driver to use dev_groups

[Greg Kroah-Hartman]

Platform drivers now have the option to have the platform core create
and remove any needed sysfs attribute files.  So take advantage of that
and do not register "by hand" a sysfs group of attributes.

Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: linux-input@vger.kernel.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/input/misc/axp20x-pek.c | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/drivers/input/misc/axp20x-pek.c b/drivers/input/misc/axp20x-pek.c
index debeeaeb8812..baff6dcbe392 100644
--- a/drivers/input/misc/axp20x-pek.c
+++ b/drivers/input/misc/axp20x-pek.c
@@ -195,15 +195,12 @@ DEVICE_ATTR(startup, 0644, axp20x_show_attr_startup, axp20x_store_attr_startup);
 DEVICE_ATTR(shutdown, 0644, axp20x_show_attr_shutdown,
 	    axp20x_store_attr_shutdown);
 
-static struct attribute *axp20x_attributes[] = {
+static struct attribute *axp20x_attrs[] = {
 	&dev_attr_startup.attr,
 	&dev_attr_shutdown.attr,
 	NULL,
 };
-
-static const struct attribute_group axp20x_attribute_group = {
-	.attrs = axp20x_attributes,
-};
+ATTRIBUTE_GROUPS(axp20x);
 
 static irqreturn_t axp20x_pek_irq(int irq, void *pwr)
 {
@@ -356,13 +353,6 @@ static int axp20x_pek_probe(struct platform_device *pdev)
 
 	axp20x_pek->info = (struct axp20x_info *)match->driver_data;
 
-	error = devm_device_add_group(&pdev->dev, &axp20x_attribute_group);
-	if (error) {
-		dev_err(&pdev->dev, "Failed to create sysfs attributes: %d\n",
-			error);
-		return error;
-	}
-
 	platform_set_drvdata(pdev, axp20x_pek);
 
 	return 0;
@@ -406,6 +396,7 @@ static const struct platform_device_id axp_pek_id_match[] = {
 MODULE_DEVICE_TABLE(platform, axp_pek_id_match);
 
 static struct platform_driver axp20x_pek_driver = {
+	.dev_groups	= axp20x_groups,
 	.probe		= axp20x_pek_probe,
 	.id_table	= axp_pek_id_match,
 	.driver		= {
-- 
2.22.0

Re: [PATCH 04/11] firmware: arm_scpi: convert platform driver to use dev_groups

[Sudeep Holla]

On Thu, Jul 04, 2019 at 10:46:10AM +0200, Greg Kroah-Hartman wrote:
> Platform drivers now have the option to have the platform core create
> and remove any needed sysfs attribute files.  So take advantage of that
> and do not register "by hand" a sysfs group of attributes.
> 
> Cc: Sudeep Holla <sudeep.holla@arm.com>

Assuming you plan to take this series as a whole,

Acked-by: Sudeep Holla <sudeep.holla@arm.com>

--
Regards,
Sudeep

Re: [PATCH 04/11] firmware: arm_scpi: convert platform driver to use dev_groups

[Sudeep Holla]

On Thu, Jul 04, 2019 at 10:46:10AM +0200, Greg Kroah-Hartman wrote:
> Platform drivers now have the option to have the platform core create
> and remove any needed sysfs attribute files.  So take advantage of that
> and do not register "by hand" a sysfs group of attributes.
> 
> Cc: Sudeep Holla <sudeep.holla@arm.com>

Assuming you plan to take this series as a whole,

Acked-by: Sudeep Holla <sudeep.holla@arm.com>

--
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 01/11] Platform: add a dev_groups pointer to struct platform_driver

[Johan Hovold]

On Thu, Jul 04, 2019 at 10:46:07AM +0200, Greg Kroah-Hartman wrote:
> Platform drivers like to add sysfs groups to their device, but right now
> they have to do it "by hand".  The driver core should handle this for
> them, but there is no way to get to the bus-default attribute groups as
> all platform devices are "special and unique" one-off drivers/devices.
> 
> To combat this, add a dev_groups pointer to platform_driver which allows
> a platform driver to set up a list of default attributes that will be
> properly created and removed by the platform driver core when a probe()
> function is successful and removed right before the device is unbound.
> 
> Cc: Richard Gong <richard.gong@linux.intel.com>
> Cc: Romain Izard <romain.izard.pro@gmail.com>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Mans Rullgard <mans@mansr.com>
> Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> Cc: Randy Dunlap <rdunlap@infradead.org>
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  drivers/base/platform.c         | 40 +++++++++++++++++++++------------
>  include/linux/platform_device.h |  1 +
>  2 files changed, 27 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index 713903290385..d81fcd435b52 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -598,6 +598,21 @@ struct platform_device *platform_device_register_full(
>  }
>  EXPORT_SYMBOL_GPL(platform_device_register_full);
>  
> +static int platform_drv_remove(struct device *_dev)
> +{
> +	struct platform_driver *drv = to_platform_driver(_dev->driver);
> +	struct platform_device *dev = to_platform_device(_dev);
> +	int ret = 0;
> +
> +	if (drv->remove)
> +		ret = drv->remove(dev);
> +	if (drv->dev_groups)
> +		device_remove_groups(_dev, drv->dev_groups);

Shouldn't you remove the groups before calling driver remove(), which
could be releasing resources used by the attribute implementations?

This would also be the reverse of how what you do at probe.

> +	dev_pm_domain_detach(_dev, true);
> +
> +	return ret;
> +}
> +
>  static int platform_drv_probe(struct device *_dev)
>  {
>  	struct platform_driver *drv = to_platform_driver(_dev->driver);
> @@ -614,8 +629,18 @@ static int platform_drv_probe(struct device *_dev)
>  
>  	if (drv->probe) {
>  		ret = drv->probe(dev);
> -		if (ret)
> +		if (ret) {
>  			dev_pm_domain_detach(_dev, true);
> +			goto out;
> +		}
> +	}
> +	if (drv->dev_groups) {
> +		ret = device_add_groups(_dev, drv->dev_groups);
> +		if (ret) {
> +			platform_drv_remove(_dev);

This would lead to device_remove_groups() being called for the never
added attribute groups. Looks like that may trigger a warning in the
sysfs code judging from a quick look.

> +			return ret;
> +		}
> +		kobject_uevent(&_dev->kobj, KOBJ_CHANGE);
>  	}

Johan

Re: [PATCH 01/11] Platform: add a dev_groups pointer to struct platform_driver

[Greg Kroah-Hartman]

On Thu, Jul 04, 2019 at 11:32:00AM +0200, Johan Hovold wrote:
> On Thu, Jul 04, 2019 at 10:46:07AM +0200, Greg Kroah-Hartman wrote:
> > Platform drivers like to add sysfs groups to their device, but right now
> > they have to do it "by hand".  The driver core should handle this for
> > them, but there is no way to get to the bus-default attribute groups as
> > all platform devices are "special and unique" one-off drivers/devices.
> > 
> > To combat this, add a dev_groups pointer to platform_driver which allows
> > a platform driver to set up a list of default attributes that will be
> > properly created and removed by the platform driver core when a probe()
> > function is successful and removed right before the device is unbound.
> > 
> > Cc: Richard Gong <richard.gong@linux.intel.com>
> > Cc: Romain Izard <romain.izard.pro@gmail.com>
> > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Cc: Mans Rullgard <mans@mansr.com>
> > Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > Cc: Randy Dunlap <rdunlap@infradead.org>
> > Cc: linux-kernel@vger.kernel.org
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > ---
> >  drivers/base/platform.c         | 40 +++++++++++++++++++++------------
> >  include/linux/platform_device.h |  1 +
> >  2 files changed, 27 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> > index 713903290385..d81fcd435b52 100644
> > --- a/drivers/base/platform.c
> > +++ b/drivers/base/platform.c
> > @@ -598,6 +598,21 @@ struct platform_device *platform_device_register_full(
> >  }
> >  EXPORT_SYMBOL_GPL(platform_device_register_full);
> >  
> > +static int platform_drv_remove(struct device *_dev)
> > +{
> > +	struct platform_driver *drv = to_platform_driver(_dev->driver);
> > +	struct platform_device *dev = to_platform_device(_dev);
> > +	int ret = 0;
> > +
> > +	if (drv->remove)
> > +		ret = drv->remove(dev);
> > +	if (drv->dev_groups)
> > +		device_remove_groups(_dev, drv->dev_groups);
> 
> Shouldn't you remove the groups before calling driver remove(), which
> could be releasing resources used by the attribute implementations?
> 
> This would also be the reverse of how what you do at probe.

Good point, probably doesn't really matter, but I'll reverse it.

> > +	dev_pm_domain_detach(_dev, true);
> > +
> > +	return ret;
> > +}
> > +
> >  static int platform_drv_probe(struct device *_dev)
> >  {
> >  	struct platform_driver *drv = to_platform_driver(_dev->driver);
> > @@ -614,8 +629,18 @@ static int platform_drv_probe(struct device *_dev)
> >  
> >  	if (drv->probe) {
> >  		ret = drv->probe(dev);
> > -		if (ret)
> > +		if (ret) {
> >  			dev_pm_domain_detach(_dev, true);
> > +			goto out;
> > +		}
> > +	}
> > +	if (drv->dev_groups) {
> > +		ret = device_add_groups(_dev, drv->dev_groups);
> > +		if (ret) {
> > +			platform_drv_remove(_dev);
> 
> This would lead to device_remove_groups() being called for the never
> added attribute groups. Looks like that may trigger a warning in the
> sysfs code judging from a quick look.

Hm, let me look at this some more...

[PATCH 01/12 v2] Platform: add a dev_groups pointer to struct platform_driver

[Greg Kroah-Hartman]

Platform drivers like to add sysfs groups to their device, but right now
they have to do it "by hand".  The driver core should handle this for
them, but there is no way to get to the bus-default attribute groups as
all platform devices are "special and unique" one-off drivers/devices.

To combat this, add a dev_groups pointer to platform_driver which allows
a platform driver to set up a list of default attributes that will be
properly created and removed by the platform driver core when a probe()
function is successful and removed right before the device is unbound.

Cc: Richard Gong <richard.gong@linux.intel.com>
Cc: Romain Izard <romain.izard.pro@gmail.com>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Mans Rullgard <mans@mansr.com>
Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Cc: Randy Dunlap <rdunlap@infradead.org>
Cc: Johan Hovold <johan@kernel.org>
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
v2: addressed Johan's comments by reordering when we remove the files
    from the device, and clean up on an error in a nicer way.  Ended up
    making the patch smaller overall, always nice.

 drivers/base/platform.c         | 16 +++++++++++++++-
 include/linux/platform_device.h |  1 +
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 713903290385..74428a1e03f3 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -614,8 +614,20 @@ static int platform_drv_probe(struct device *_dev)
 
 	if (drv->probe) {
 		ret = drv->probe(dev);
-		if (ret)
+		if (ret) {
+			dev_pm_domain_detach(_dev, true);
+			goto out;
+		}
+	}
+	if (drv->dev_groups) {
+		ret = device_add_groups(_dev, drv->dev_groups);
+		if (ret) {
+			if (drv->remove)
+				drv->remove(dev);
 			dev_pm_domain_detach(_dev, true);
+			return ret;
+		}
+		kobject_uevent(&_dev->kobj, KOBJ_CHANGE);
 	}
 
 out:
@@ -638,6 +650,8 @@ static int platform_drv_remove(struct device *_dev)
 	struct platform_device *dev = to_platform_device(_dev);
 	int ret = 0;
 
+	if (drv->dev_groups)
+		device_remove_groups(_dev, drv->dev_groups);
 	if (drv->remove)
 		ret = drv->remove(dev);
 	dev_pm_domain_detach(_dev, true);
diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
index cc464850b71e..027f1e1d7af8 100644
--- a/include/linux/platform_device.h
+++ b/include/linux/platform_device.h
@@ -190,6 +190,7 @@ struct platform_driver {
 	int (*resume)(struct platform_device *);
 	struct device_driver driver;
 	const struct platform_device_id *id_table;
+	const struct attribute_group **dev_groups;
 	bool prevent_deferred_probe;
 };
 
-- 
2.22.0

Re: [PATCH 05/11] olpc: x01: convert platform driver to use dev_groups

[Andy Shevchenko]

On Thu, Jul 4, 2019 at 11:47 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> Platform drivers now have the option to have the platform core create
> and remove any needed sysfs attribute files.  So take advantage of that
> and do not register "by hand" a lid sysfs file.
>

Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Cc: Darren Hart <dvhart@infradead.org>
> Cc: Andy Shevchenko <andy@infradead.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: x86@kernel.org
> Cc: platform-driver-x86@vger.kernel.org
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  arch/x86/platform/olpc/olpc-xo1-sci.c | 17 +++++++----------
>  1 file changed, 7 insertions(+), 10 deletions(-)
>
> diff --git a/arch/x86/platform/olpc/olpc-xo1-sci.c b/arch/x86/platform/olpc/olpc-xo1-sci.c
> index 25ce1b3b0732..ce1948918dd2 100644
> --- a/arch/x86/platform/olpc/olpc-xo1-sci.c
> +++ b/arch/x86/platform/olpc/olpc-xo1-sci.c
> @@ -157,6 +157,12 @@ static ssize_t lid_wake_mode_set(struct device *dev,
>  static DEVICE_ATTR(lid_wake_mode, S_IWUSR | S_IRUGO, lid_wake_mode_show,
>                    lid_wake_mode_set);
>
> +static struct attribute *lid_attrs[] = {
> +       &dev_attr_lid_wake_mode.attr,
> +       NULL,
> +};
> +ATTRIBUTE_GROUPS(lid);
> +
>  /*
>   * Process all items in the EC's SCI queue.
>   *
> @@ -510,17 +516,8 @@ static int setup_lid_switch(struct platform_device *pdev)
>                 goto err_register;
>         }
>
> -       r = device_create_file(&lid_switch_idev->dev, &dev_attr_lid_wake_mode);
> -       if (r) {
> -               dev_err(&pdev->dev, "failed to create wake mode attr: %d\n", r);
> -               goto err_create_attr;
> -       }
> -
>         return 0;
>
> -err_create_attr:
> -       input_unregister_device(lid_switch_idev);
> -       lid_switch_idev = NULL;
>  err_register:
>         input_free_device(lid_switch_idev);
>         return r;
> @@ -528,7 +525,6 @@ static int setup_lid_switch(struct platform_device *pdev)
>
>  static void free_lid_switch(void)
>  {
> -       device_remove_file(&lid_switch_idev->dev, &dev_attr_lid_wake_mode);
>         input_unregister_device(lid_switch_idev);
>  }
>
> @@ -629,6 +625,7 @@ static struct platform_driver xo1_sci_driver = {
>         .remove = xo1_sci_remove,
>         .suspend = xo1_sci_suspend,
>         .resume = xo1_sci_resume,
> +       .dev_groups = lid_groups,
>  };
>
>  static int __init xo1_sci_init(void)
> --
> 2.22.0
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 06/11] platform: x86: hp-wmi: convert platform driver to use dev_groups

[Andy Shevchenko]

On Thu, Jul 4, 2019 at 11:47 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> Platform drivers now have the option to have the platform core create
> and remove any needed sysfs attribute files.  So take advantage of that
> and do not register "by hand" a bunch of sysfs files.
>

Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Cc: Darren Hart <dvhart@infradead.org>
> Cc: Andy Shevchenko <andy@infradead.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: x86@kernel.org
> Cc: platform-driver-x86@vger.kernel.org
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  drivers/platform/x86/hp-wmi.c | 47 +++++++++--------------------------
>  1 file changed, 12 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c
> index 2521e45280b8..b4ed5902737a 100644
> --- a/drivers/platform/x86/hp-wmi.c
> +++ b/drivers/platform/x86/hp-wmi.c
> @@ -502,6 +502,17 @@ static DEVICE_ATTR_RO(dock);
>  static DEVICE_ATTR_RO(tablet);
>  static DEVICE_ATTR_RW(postcode);
>
> +static struct attribute *hp_wmi_attrs[] = {
> +       &dev_attr_display.attr,
> +       &dev_attr_hddtemp.attr,
> +       &dev_attr_als.attr,
> +       &dev_attr_dock.attr,
> +       &dev_attr_tablet.attr,
> +       &dev_attr_postcode.attr,
> +       NULL,
> +};
> +ATTRIBUTE_GROUPS(hp_wmi);
> +
>  static void hp_wmi_notify(u32 value, void *context)
>  {
>         struct acpi_buffer response = { ACPI_ALLOCATE_BUFFER, NULL };
> @@ -678,16 +689,6 @@ static void hp_wmi_input_destroy(void)
>         input_unregister_device(hp_wmi_input_dev);
>  }
>
> -static void cleanup_sysfs(struct platform_device *device)
> -{
> -       device_remove_file(&device->dev, &dev_attr_display);
> -       device_remove_file(&device->dev, &dev_attr_hddtemp);
> -       device_remove_file(&device->dev, &dev_attr_als);
> -       device_remove_file(&device->dev, &dev_attr_dock);
> -       device_remove_file(&device->dev, &dev_attr_tablet);
> -       device_remove_file(&device->dev, &dev_attr_postcode);
> -}
> -
>  static int __init hp_wmi_rfkill_setup(struct platform_device *device)
>  {
>         int err, wireless;
> @@ -858,8 +859,6 @@ static int __init hp_wmi_rfkill2_setup(struct platform_device *device)
>
>  static int __init hp_wmi_bios_setup(struct platform_device *device)
>  {
> -       int err;
> -
>         /* clear detected rfkill devices */
>         wifi_rfkill = NULL;
>         bluetooth_rfkill = NULL;
> @@ -869,35 +868,12 @@ static int __init hp_wmi_bios_setup(struct platform_device *device)
>         if (hp_wmi_rfkill_setup(device))
>                 hp_wmi_rfkill2_setup(device);
>
> -       err = device_create_file(&device->dev, &dev_attr_display);
> -       if (err)
> -               goto add_sysfs_error;
> -       err = device_create_file(&device->dev, &dev_attr_hddtemp);
> -       if (err)
> -               goto add_sysfs_error;
> -       err = device_create_file(&device->dev, &dev_attr_als);
> -       if (err)
> -               goto add_sysfs_error;
> -       err = device_create_file(&device->dev, &dev_attr_dock);
> -       if (err)
> -               goto add_sysfs_error;
> -       err = device_create_file(&device->dev, &dev_attr_tablet);
> -       if (err)
> -               goto add_sysfs_error;
> -       err = device_create_file(&device->dev, &dev_attr_postcode);
> -       if (err)
> -               goto add_sysfs_error;
>         return 0;
> -
> -add_sysfs_error:
> -       cleanup_sysfs(device);
> -       return err;
>  }
>
>  static int __exit hp_wmi_bios_remove(struct platform_device *device)
>  {
>         int i;
> -       cleanup_sysfs(device);
>
>         for (i = 0; i < rfkill2_count; i++) {
>                 rfkill_unregister(rfkill2[i].rfkill);
> @@ -968,6 +944,7 @@ static struct platform_driver hp_wmi_driver = {
>                 .pm = &hp_wmi_pm_ops,
>         },
>         .remove = __exit_p(hp_wmi_bios_remove),
> +       .dev_groups = hp_wmi_groups,
>  };
>
>  static int __init hp_wmi_init(void)
> --
> 2.22.0
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 07/11] video: fbdev: wm8505fb: convert platform driver to use dev_groups

[Andy Shevchenko]

On Thu, Jul 4, 2019 at 11:47 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> Platform drivers now have the option to have the platform core create
> and remove any needed sysfs attribute files.  So take advantage of that
> and do not register "by hand" a sysfs file.
>
> Cc: Darren Hart <dvhart@infradead.org>
> Cc: Andy Shevchenko <andy@infradead.org>
> Cc: platform-driver-x86@vger.kernel.org

Is it correct Cc list? Looks like a typo to me.

> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  drivers/video/fbdev/wm8505fb.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/video/fbdev/wm8505fb.c b/drivers/video/fbdev/wm8505fb.c
> index 8f0d5379861d..3b826da97035 100644
> --- a/drivers/video/fbdev/wm8505fb.c
> +++ b/drivers/video/fbdev/wm8505fb.c
> @@ -184,6 +184,12 @@ static ssize_t contrast_store(struct device *dev,
>
>  static DEVICE_ATTR_RW(contrast);
>
> +static struct attribute *wm8505fb_attrs[] = {
> +       &dev_attr_contrast.attr,
> +       NULL,
> +};
> +ATTRIBUTE_GROUPS(wm8505fb);
> +
>  static inline u_int chan_to_field(u_int chan, struct fb_bitfield *bf)
>  {
>         chan &= 0xffff;
> @@ -369,10 +375,6 @@ static int wm8505fb_probe(struct platform_device *pdev)
>                 return ret;
>         }
>
> -       ret = device_create_file(&pdev->dev, &dev_attr_contrast);
> -       if (ret < 0)
> -               fb_warn(&fbi->fb, "failed to register attributes (%d)\n", ret);
> -
>         fb_info(&fbi->fb, "%s frame buffer at 0x%lx-0x%lx\n",
>                 fbi->fb.fix.id, fbi->fb.fix.smem_start,
>                 fbi->fb.fix.smem_start + fbi->fb.fix.smem_len - 1);
> @@ -384,8 +386,6 @@ static int wm8505fb_remove(struct platform_device *pdev)
>  {
>         struct wm8505fb_info *fbi = platform_get_drvdata(pdev);
>
> -       device_remove_file(&pdev->dev, &dev_attr_contrast);
> -
>         unregister_framebuffer(&fbi->fb);
>
>         writel(0, fbi->regbase);
> @@ -402,6 +402,7 @@ static const struct of_device_id wmt_dt_ids[] = {
>  };
>
>  static struct platform_driver wm8505fb_driver = {
> +       .dev_groups     = wm8505fb_groups,
>         .probe          = wm8505fb_probe,
>         .remove         = wm8505fb_remove,
>         .driver         = {
> --
> 2.22.0
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 07/11] video: fbdev: wm8505fb: convert platform driver to use dev_groups

[Greg Kroah-Hartman]

On Thu, Jul 04, 2019 at 04:29:40PM +0300, Andy Shevchenko wrote:
> On Thu, Jul 4, 2019 at 11:47 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > Platform drivers now have the option to have the platform core create
> > and remove any needed sysfs attribute files.  So take advantage of that
> > and do not register "by hand" a sysfs file.
> >
> > Cc: Darren Hart <dvhart@infradead.org>
> > Cc: Andy Shevchenko <andy@infradead.org>
> > Cc: platform-driver-x86@vger.kernel.org
> 
> Is it correct Cc list? Looks like a typo to me.

Ugh, wrong mapping of patch to maintainers, my scripts failed me :(

I'll resend this with the proper one, thanks.

greg k-h

[PATCH 07/11] video: fbdev: wm8505fb: convert platform driver to use dev_groups

[Greg Kroah-Hartman]

Platform drivers now have the option to have the platform core create
and remove any needed sysfs attribute files.  So take advantage of that
and do not register "by hand" a sysfs file.

Cc: Tony Prisk <linux@prisktech.co.nz>
Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: dri-devel@lists.freedesktop.org
Cc: linux-fbdev@vger.kernel.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/video/fbdev/wm8505fb.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/video/fbdev/wm8505fb.c b/drivers/video/fbdev/wm8505fb.c
index 8f0d5379861d..3b826da97035 100644
--- a/drivers/video/fbdev/wm8505fb.c
+++ b/drivers/video/fbdev/wm8505fb.c
@@ -184,6 +184,12 @@ static ssize_t contrast_store(struct device *dev,
 
 static DEVICE_ATTR_RW(contrast);
 
+static struct attribute *wm8505fb_attrs[] = {
+	&dev_attr_contrast.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(wm8505fb);
+
 static inline u_int chan_to_field(u_int chan, struct fb_bitfield *bf)
 {
 	chan &= 0xffff;
@@ -369,10 +375,6 @@ static int wm8505fb_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	ret = device_create_file(&pdev->dev, &dev_attr_contrast);
-	if (ret < 0)
-		fb_warn(&fbi->fb, "failed to register attributes (%d)\n", ret);
-
 	fb_info(&fbi->fb, "%s frame buffer at 0x%lx-0x%lx\n",
 		fbi->fb.fix.id, fbi->fb.fix.smem_start,
 		fbi->fb.fix.smem_start + fbi->fb.fix.smem_len - 1);
@@ -384,8 +386,6 @@ static int wm8505fb_remove(struct platform_device *pdev)
 {
 	struct wm8505fb_info *fbi = platform_get_drvdata(pdev);
 
-	device_remove_file(&pdev->dev, &dev_attr_contrast);
-
 	unregister_framebuffer(&fbi->fb);
 
 	writel(0, fbi->regbase);
@@ -402,6 +402,7 @@ static const struct of_device_id wmt_dt_ids[] = {
 };
 
 static struct platform_driver wm8505fb_driver = {
+	.dev_groups	= wm8505fb_groups,
 	.probe		= wm8505fb_probe,
 	.remove		= wm8505fb_remove,
 	.driver		= {
-- 
2.22.0

[PATCH 07/11] video: fbdev: wm8505fb: convert platform driver to use dev_groups

[Greg Kroah-Hartman]

Platform drivers now have the option to have the platform core create
and remove any needed sysfs attribute files.  So take advantage of that
and do not register "by hand" a sysfs file.

Cc: Tony Prisk <linux@prisktech.co.nz>
Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: dri-devel@lists.freedesktop.org
Cc: linux-fbdev@vger.kernel.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/video/fbdev/wm8505fb.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/video/fbdev/wm8505fb.c b/drivers/video/fbdev/wm8505fb.c
index 8f0d5379861d..3b826da97035 100644
--- a/drivers/video/fbdev/wm8505fb.c
+++ b/drivers/video/fbdev/wm8505fb.c
@@ -184,6 +184,12 @@ static ssize_t contrast_store(struct device *dev,
 
 static DEVICE_ATTR_RW(contrast);
 
+static struct attribute *wm8505fb_attrs[] = {
+	&dev_attr_contrast.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(wm8505fb);
+
 static inline u_int chan_to_field(u_int chan, struct fb_bitfield *bf)
 {
 	chan &= 0xffff;
@@ -369,10 +375,6 @@ static int wm8505fb_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	ret = device_create_file(&pdev->dev, &dev_attr_contrast);
-	if (ret < 0)
-		fb_warn(&fbi->fb, "failed to register attributes (%d)\n", ret);
-
 	fb_info(&fbi->fb, "%s frame buffer at 0x%lx-0x%lx\n",
 		fbi->fb.fix.id, fbi->fb.fix.smem_start,
 		fbi->fb.fix.smem_start + fbi->fb.fix.smem_len - 1);
@@ -384,8 +386,6 @@ static int wm8505fb_remove(struct platform_device *pdev)
 {
 	struct wm8505fb_info *fbi = platform_get_drvdata(pdev);
 
-	device_remove_file(&pdev->dev, &dev_attr_contrast);
-
 	unregister_framebuffer(&fbi->fb);
 
 	writel(0, fbi->regbase);
@@ -402,6 +402,7 @@ static const struct of_device_id wmt_dt_ids[] = {
 };
 
 static struct platform_driver wm8505fb_driver = {
+	.dev_groups	= wm8505fb_groups,
 	.probe		= wm8505fb_probe,
 	.remove		= wm8505fb_remove,
 	.driver		= {
-- 
2.22.0

[PATCH 07/11] video: fbdev: wm8505fb: convert platform driver to use dev_groups

[Greg Kroah-Hartman]

Platform drivers now have the option to have the platform core create
and remove any needed sysfs attribute files.  So take advantage of that
and do not register "by hand" a sysfs file.

Cc: Tony Prisk <linux@prisktech.co.nz>
Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: dri-devel@lists.freedesktop.org
Cc: linux-fbdev@vger.kernel.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/video/fbdev/wm8505fb.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/video/fbdev/wm8505fb.c b/drivers/video/fbdev/wm8505fb.c
index 8f0d5379861d..3b826da97035 100644
--- a/drivers/video/fbdev/wm8505fb.c
+++ b/drivers/video/fbdev/wm8505fb.c
@@ -184,6 +184,12 @@ static ssize_t contrast_store(struct device *dev,
 
 static DEVICE_ATTR_RW(contrast);
 
+static struct attribute *wm8505fb_attrs[] = {
+	&dev_attr_contrast.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(wm8505fb);
+
 static inline u_int chan_to_field(u_int chan, struct fb_bitfield *bf)
 {
 	chan &= 0xffff;
@@ -369,10 +375,6 @@ static int wm8505fb_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	ret = device_create_file(&pdev->dev, &dev_attr_contrast);
-	if (ret < 0)
-		fb_warn(&fbi->fb, "failed to register attributes (%d)\n", ret);
-
 	fb_info(&fbi->fb, "%s frame buffer at 0x%lx-0x%lx\n",
 		fbi->fb.fix.id, fbi->fb.fix.smem_start,
 		fbi->fb.fix.smem_start + fbi->fb.fix.smem_len - 1);
@@ -384,8 +386,6 @@ static int wm8505fb_remove(struct platform_device *pdev)
 {
 	struct wm8505fb_info *fbi = platform_get_drvdata(pdev);
 
-	device_remove_file(&pdev->dev, &dev_attr_contrast);
-
 	unregister_framebuffer(&fbi->fb);
 
 	writel(0, fbi->regbase);
@@ -402,6 +402,7 @@ static const struct of_device_id wmt_dt_ids[] = {
 };
 
 static struct platform_driver wm8505fb_driver = {
+	.dev_groups	= wm8505fb_groups,
 	.probe		= wm8505fb_probe,
 	.remove		= wm8505fb_remove,
 	.driver		= {
-- 
2.22.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 01/12 v2] Platform: add a dev_groups pointer to struct platform_driver

[Dmitry Torokhov]

Hi Greg,

On Thu, Jul 4, 2019 at 5:15 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> Platform drivers like to add sysfs groups to their device, but right now
> they have to do it "by hand".  The driver core should handle this for
> them, but there is no way to get to the bus-default attribute groups as
> all platform devices are "special and unique" one-off drivers/devices.
>
> To combat this, add a dev_groups pointer to platform_driver which allows
> a platform driver to set up a list of default attributes that will be
> properly created and removed by the platform driver core when a probe()
> function is successful and removed right before the device is unbound.

Why is this limited to platform bus? Drivers for other buses also
often want to augment list of their attributes during probe(). I'd
move it to generic probe handling.

>
> Cc: Richard Gong <richard.gong@linux.intel.com>
> Cc: Romain Izard <romain.izard.pro@gmail.com>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Mans Rullgard <mans@mansr.com>
> Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> Cc: Randy Dunlap <rdunlap@infradead.org>
> Cc: Johan Hovold <johan@kernel.org>
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
> v2: addressed Johan's comments by reordering when we remove the files
>     from the device, and clean up on an error in a nicer way.  Ended up
>     making the patch smaller overall, always nice.
>
>  drivers/base/platform.c         | 16 +++++++++++++++-
>  include/linux/platform_device.h |  1 +
>  2 files changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index 713903290385..74428a1e03f3 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -614,8 +614,20 @@ static int platform_drv_probe(struct device *_dev)
>
>         if (drv->probe) {
>                 ret = drv->probe(dev);
> -               if (ret)
> +               if (ret) {
> +                       dev_pm_domain_detach(_dev, true);
> +                       goto out;
> +               }
> +       }
> +       if (drv->dev_groups) {
> +               ret = device_add_groups(_dev, drv->dev_groups);
> +               if (ret) {
> +                       if (drv->remove)
> +                               drv->remove(dev);
>                         dev_pm_domain_detach(_dev, true);
> +                       return ret;
> +               }
> +               kobject_uevent(&_dev->kobj, KOBJ_CHANGE);

We already emit KOBJ_BIND when we finish binding device to a driver,
regardless of the bus. I know we still need to teach systemd to handle
it properly, but I think it is better than sprinkling KOBJ_CHANGE
around.

Thanks.

-- 
Dmitry

Re: [PATCH 07/11] video: fbdev: wm8505fb: convert platform driver to use dev_groups

[Bartlomiej Zolnierkiewicz]

On 7/4/19 4:26 PM, Greg Kroah-Hartman wrote:
> Platform drivers now have the option to have the platform core create
> and remove any needed sysfs attribute files.  So take advantage of that
> and do not register "by hand" a sysfs file.
> 
> Cc: Tony Prisk <linux@prisktech.co.nz>
> Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: dri-devel@lists.freedesktop.org
> Cc: linux-fbdev@vger.kernel.org
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Acked-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Re: [PATCH 07/11] video: fbdev: wm8505fb: convert platform driver to use dev_groups

[Bartlomiej Zolnierkiewicz]

On 7/4/19 4:26 PM, Greg Kroah-Hartman wrote:
> Platform drivers now have the option to have the platform core create
> and remove any needed sysfs attribute files.  So take advantage of that
> and do not register "by hand" a sysfs file.
> 
> Cc: Tony Prisk <linux@prisktech.co.nz>
> Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: dri-devel@lists.freedesktop.org
> Cc: linux-fbdev@vger.kernel.org
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Acked-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Re: [PATCH 07/11] video: fbdev: wm8505fb: convert platform driver to use dev_groups

[Bartlomiej Zolnierkiewicz]

On 7/4/19 4:26 PM, Greg Kroah-Hartman wrote:
> Platform drivers now have the option to have the platform core create
> and remove any needed sysfs attribute files.  So take advantage of that
> and do not register "by hand" a sysfs file.
> 
> Cc: Tony Prisk <linux@prisktech.co.nz>
> Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: dri-devel@lists.freedesktop.org
> Cc: linux-fbdev@vger.kernel.org
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Acked-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 08/11] video: fbdev: w100fb: convert platform driver to use dev_groups

[Bartlomiej Zolnierkiewicz]

On 7/4/19 10:46 AM, Greg Kroah-Hartman wrote:
> Platform drivers now have the option to have the platform core create
> and remove any needed sysfs attribute files.  So take advantage of that
> and do not register "by hand" a bunch of sysfs files.
> 
> Cc: Tony Prisk <linux@prisktech.co.nz>
> Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: dri-devel@lists.freedesktop.org
> Cc: linux-fbdev@vger.kernel.org
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Acked-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Re: [PATCH 08/11] video: fbdev: w100fb: convert platform driver to use dev_groups

[Bartlomiej Zolnierkiewicz]

On 7/4/19 10:46 AM, Greg Kroah-Hartman wrote:
> Platform drivers now have the option to have the platform core create
> and remove any needed sysfs attribute files.  So take advantage of that
> and do not register "by hand" a bunch of sysfs files.
> 
> Cc: Tony Prisk <linux@prisktech.co.nz>
> Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: dri-devel@lists.freedesktop.org
> Cc: linux-fbdev@vger.kernel.org
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Acked-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Re: [PATCH 08/11] video: fbdev: w100fb: convert platform driver to use dev_groups

[Bartlomiej Zolnierkiewicz]

On 7/4/19 10:46 AM, Greg Kroah-Hartman wrote:
> Platform drivers now have the option to have the platform core create
> and remove any needed sysfs attribute files.  So take advantage of that
> and do not register "by hand" a bunch of sysfs files.
> 
> Cc: Tony Prisk <linux@prisktech.co.nz>
> Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: dri-devel@lists.freedesktop.org
> Cc: linux-fbdev@vger.kernel.org
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Acked-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 08/11] video: fbdev: w100fb: convert platform driver to use dev_groups

[Bartlomiej Zolnierkiewicz]

On 7/4/19 10:46 AM, Greg Kroah-Hartman wrote:
> Platform drivers now have the option to have the platform core create
> and remove any needed sysfs attribute files.  So take advantage of that
> and do not register "by hand" a bunch of sysfs files.
> 
> Cc: Tony Prisk <linux@prisktech.co.nz>
> Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: dri-devel@lists.freedesktop.org
> Cc: linux-fbdev@vger.kernel.org
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Acked-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 09/11] video: fbdev: sm501fb: convert platform driver to use dev_groups

[Bartlomiej Zolnierkiewicz]

On 7/4/19 10:46 AM, Greg Kroah-Hartman wrote:
> Platform drivers now have the option to have the platform core create
> and remove any needed sysfs attribute files.  So take advantage of that
> and do not register "by hand" a bunch of sysfs files.
> 
> Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> Cc: dri-devel@lists.freedesktop.org
> Cc: linux-fbdev@vger.kernel.org
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Acked-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Re: [PATCH 09/11] video: fbdev: sm501fb: convert platform driver to use dev_groups

[Bartlomiej Zolnierkiewicz]

On 7/4/19 10:46 AM, Greg Kroah-Hartman wrote:
> Platform drivers now have the option to have the platform core create
> and remove any needed sysfs attribute files.  So take advantage of that
> and do not register "by hand" a bunch of sysfs files.
> 
> Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> Cc: dri-devel@lists.freedesktop.org
> Cc: linux-fbdev@vger.kernel.org
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Acked-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Re: [PATCH 01/12 v2] Platform: add a dev_groups pointer to struct platform_driver

[Greg Kroah-Hartman]

On Thu, Jul 04, 2019 at 02:17:22PM -0700, Dmitry Torokhov wrote:
> Hi Greg,
> 
> On Thu, Jul 4, 2019 at 5:15 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > Platform drivers like to add sysfs groups to their device, but right now
> > they have to do it "by hand".  The driver core should handle this for
> > them, but there is no way to get to the bus-default attribute groups as
> > all platform devices are "special and unique" one-off drivers/devices.
> >
> > To combat this, add a dev_groups pointer to platform_driver which allows
> > a platform driver to set up a list of default attributes that will be
> > properly created and removed by the platform driver core when a probe()
> > function is successful and removed right before the device is unbound.
> 
> Why is this limited to platform bus? Drivers for other buses also
> often want to augment list of their attributes during probe(). I'd
> move it to generic probe handling.

This is not limited to the platform at all, the driver core supports
this for any bus type today, but it's then up to the bus-specific code
to pass that on to the driver core.  That's usually set for the
bus-specific attributes that they want exposed for all devices of that
bus type (see the bus_groups, dev_groups, and drv_groups pointers in
struct bus_type).

For the platform devices, the problem is that this is something that the
individual drivers want after they bind to the device.  And as all
platform devices are "different" they can't be a "common" set of
attributes, so they need to be created after the device is bound to the
driver.

> > Cc: Richard Gong <richard.gong@linux.intel.com>
> > Cc: Romain Izard <romain.izard.pro@gmail.com>
> > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Cc: Mans Rullgard <mans@mansr.com>
> > Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > Cc: Randy Dunlap <rdunlap@infradead.org>
> > Cc: Johan Hovold <johan@kernel.org>
> > Cc: linux-kernel@vger.kernel.org
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > ---
> > v2: addressed Johan's comments by reordering when we remove the files
> >     from the device, and clean up on an error in a nicer way.  Ended up
> >     making the patch smaller overall, always nice.
> >
> >  drivers/base/platform.c         | 16 +++++++++++++++-
> >  include/linux/platform_device.h |  1 +
> >  2 files changed, 16 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> > index 713903290385..74428a1e03f3 100644
> > --- a/drivers/base/platform.c
> > +++ b/drivers/base/platform.c
> > @@ -614,8 +614,20 @@ static int platform_drv_probe(struct device *_dev)
> >
> >         if (drv->probe) {
> >                 ret = drv->probe(dev);
> > -               if (ret)
> > +               if (ret) {
> > +                       dev_pm_domain_detach(_dev, true);
> > +                       goto out;
> > +               }
> > +       }
> > +       if (drv->dev_groups) {
> > +               ret = device_add_groups(_dev, drv->dev_groups);
> > +               if (ret) {
> > +                       if (drv->remove)
> > +                               drv->remove(dev);
> >                         dev_pm_domain_detach(_dev, true);
> > +                       return ret;
> > +               }
> > +               kobject_uevent(&_dev->kobj, KOBJ_CHANGE);
> 
> We already emit KOBJ_BIND when we finish binding device to a driver,
> regardless of the bus. I know we still need to teach systemd to handle
> it properly, but I think it is better than sprinkling KOBJ_CHANGE
> around.

But the object's attributes did just change, which is what KOBJ_CHANGE
tells userspace, so this should be the correct thing to say to
userspace.

And yes, ideally KOBJ_BIND would be handled, and it will be sent once
the device's probe function succeeds, but we have to deal with old
userspaces as well, right?

thanks,

greg k-h

Re: [PATCH 01/12 v2] Platform: add a dev_groups pointer to struct platform_driver

[Dmitry Torokhov]

Hi Greg,

On Sat, Jul 6, 2019 at 1:32 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Thu, Jul 04, 2019 at 02:17:22PM -0700, Dmitry Torokhov wrote:
> > Hi Greg,
> >
> > On Thu, Jul 4, 2019 at 5:15 AM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > >
> > > Platform drivers like to add sysfs groups to their device, but right now
> > > they have to do it "by hand".  The driver core should handle this for
> > > them, but there is no way to get to the bus-default attribute groups as
> > > all platform devices are "special and unique" one-off drivers/devices.
> > >
> > > To combat this, add a dev_groups pointer to platform_driver which allows
> > > a platform driver to set up a list of default attributes that will be
> > > properly created and removed by the platform driver core when a probe()
> > > function is successful and removed right before the device is unbound.
> >
> > Why is this limited to platform bus? Drivers for other buses also
> > often want to augment list of their attributes during probe(). I'd
> > move it to generic probe handling.
>
> This is not limited to the platform at all, the driver core supports
> this for any bus type today, but it's then up to the bus-specific code
> to pass that on to the driver core.  That's usually set for the
> bus-specific attributes that they want exposed for all devices of that
> bus type (see the bus_groups, dev_groups, and drv_groups pointers in
> struct bus_type).
>
> For the platform devices, the problem is that this is something that the
> individual drivers want after they bind to the device.  And as all
> platform devices are "different" they can't be a "common" set of
> attributes, so they need to be created after the device is bound to the
> driver.

I believe that your assertion that only platform devices want to
install custom attributes is incorrect. Drivers for devices attached
to serio, i2c, USB, spi, etc, etc, all have additional attributes:

dtor@dtor-ws:~/kernel/work (master *)$ grep -l '\(i2c\|usb\|spi\)'
`git grep -l '\(device_add_group\|sysfs_create_group\)' -- drivers` |
wc -l
170

I am pretty sure some of this count is false positives, but majority
is actually proper hits.

...

> >
> > We already emit KOBJ_BIND when we finish binding device to a driver,
> > regardless of the bus. I know we still need to teach systemd to handle
> > it properly, but I think it is better than sprinkling KOBJ_CHANGE
> > around.
>
> But the object's attributes did just change, which is what KOBJ_CHANGE
> tells userspace, so this should be the correct thing to say to
> userspace.
>
> And yes, ideally KOBJ_BIND would be handled, and it will be sent once
> the device's probe function succeeds, but we have to deal with old
> userspaces as well, right?

Not for the new functionality, I do not think so. Newer kernels should
be compatible with older userspace as it not breaking it, but new
functionality is not guaranteed to be available with older userspace.

Thanks.

-- 
Dmitry

Re: [PATCH 01/12 v2] Platform: add a dev_groups pointer to struct platform_driver

[Greg Kroah-Hartman]

On Sat, Jul 06, 2019 at 10:04:39AM -0700, Dmitry Torokhov wrote:
> Hi Greg,
> 
> On Sat, Jul 6, 2019 at 1:32 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Thu, Jul 04, 2019 at 02:17:22PM -0700, Dmitry Torokhov wrote:
> > > Hi Greg,
> > >
> > > On Thu, Jul 4, 2019 at 5:15 AM Greg Kroah-Hartman
> > > <gregkh@linuxfoundation.org> wrote:
> > > >
> > > > Platform drivers like to add sysfs groups to their device, but right now
> > > > they have to do it "by hand".  The driver core should handle this for
> > > > them, but there is no way to get to the bus-default attribute groups as
> > > > all platform devices are "special and unique" one-off drivers/devices.
> > > >
> > > > To combat this, add a dev_groups pointer to platform_driver which allows
> > > > a platform driver to set up a list of default attributes that will be
> > > > properly created and removed by the platform driver core when a probe()
> > > > function is successful and removed right before the device is unbound.
> > >
> > > Why is this limited to platform bus? Drivers for other buses also
> > > often want to augment list of their attributes during probe(). I'd
> > > move it to generic probe handling.
> >
> > This is not limited to the platform at all, the driver core supports
> > this for any bus type today, but it's then up to the bus-specific code
> > to pass that on to the driver core.  That's usually set for the
> > bus-specific attributes that they want exposed for all devices of that
> > bus type (see the bus_groups, dev_groups, and drv_groups pointers in
> > struct bus_type).
> >
> > For the platform devices, the problem is that this is something that the
> > individual drivers want after they bind to the device.  And as all
> > platform devices are "different" they can't be a "common" set of
> > attributes, so they need to be created after the device is bound to the
> > driver.
> 
> I believe that your assertion that only platform devices want to
> install custom attributes is incorrect.

Sorry, I didn't mean to imply that only platform drivers want to do
this, as you say, many other drivers do as well.

> Drivers for devices attached
> to serio, i2c, USB, spi, etc, etc, all have additional attributes:
> 
> dtor@dtor-ws:~/kernel/work (master *)$ grep -l '\(i2c\|usb\|spi\)'
> `git grep -l '\(device_add_group\|sysfs_create_group\)' -- drivers` |
> wc -l
> 170
> 
> I am pretty sure some of this count is false positives, but majority
> is actually proper hits.

Yeah, I know, we need to add this type of functionality to those busses
as well.  I don't see a way of doing it other than this bus-by-bus
conversion, do you?

> > > We already emit KOBJ_BIND when we finish binding device to a driver,
> > > regardless of the bus. I know we still need to teach systemd to handle
> > > it properly, but I think it is better than sprinkling KOBJ_CHANGE
> > > around.
> >
> > But the object's attributes did just change, which is what KOBJ_CHANGE
> > tells userspace, so this should be the correct thing to say to
> > userspace.
> >
> > And yes, ideally KOBJ_BIND would be handled, and it will be sent once
> > the device's probe function succeeds, but we have to deal with old
> > userspaces as well, right?
> 
> Not for the new functionality, I do not think so. Newer kernels should
> be compatible with older userspace as it not breaking it, but new
> functionality is not guaranteed to be available with older userspace.

I agree, but again, this is a kobject change (adding attributes), so
I think the event type I picked here is the correct one.

thanks,

greg k-h

Re: [PATCH 01/12 v2] Platform: add a dev_groups pointer to struct platform_driver

[Dmitry Torokhov]

On Sat, Jul 6, 2019 at 10:19 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Sat, Jul 06, 2019 at 10:04:39AM -0700, Dmitry Torokhov wrote:
> > Hi Greg,
> >
> > On Sat, Jul 6, 2019 at 1:32 AM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Thu, Jul 04, 2019 at 02:17:22PM -0700, Dmitry Torokhov wrote:
> > > > Hi Greg,
> > > >
> > > > On Thu, Jul 4, 2019 at 5:15 AM Greg Kroah-Hartman
> > > > <gregkh@linuxfoundation.org> wrote:
> > > > >
> > > > > Platform drivers like to add sysfs groups to their device, but right now
> > > > > they have to do it "by hand".  The driver core should handle this for
> > > > > them, but there is no way to get to the bus-default attribute groups as
> > > > > all platform devices are "special and unique" one-off drivers/devices.
> > > > >
> > > > > To combat this, add a dev_groups pointer to platform_driver which allows
> > > > > a platform driver to set up a list of default attributes that will be
> > > > > properly created and removed by the platform driver core when a probe()
> > > > > function is successful and removed right before the device is unbound.
> > > >
> > > > Why is this limited to platform bus? Drivers for other buses also
> > > > often want to augment list of their attributes during probe(). I'd
> > > > move it to generic probe handling.
> > >
> > > This is not limited to the platform at all, the driver core supports
> > > this for any bus type today, but it's then up to the bus-specific code
> > > to pass that on to the driver core.  That's usually set for the
> > > bus-specific attributes that they want exposed for all devices of that
> > > bus type (see the bus_groups, dev_groups, and drv_groups pointers in
> > > struct bus_type).
> > >
> > > For the platform devices, the problem is that this is something that the
> > > individual drivers want after they bind to the device.  And as all
> > > platform devices are "different" they can't be a "common" set of
> > > attributes, so they need to be created after the device is bound to the
> > > driver.
> >
> > I believe that your assertion that only platform devices want to
> > install custom attributes is incorrect.
>
> Sorry, I didn't mean to imply that only platform drivers want to do
> this, as you say, many other drivers do as well.
>
> > Drivers for devices attached
> > to serio, i2c, USB, spi, etc, etc, all have additional attributes:
> >
> > dtor@dtor-ws:~/kernel/work (master *)$ grep -l '\(i2c\|usb\|spi\)'
> > `git grep -l '\(device_add_group\|sysfs_create_group\)' -- drivers` |
> > wc -l
> > 170
> >
> > I am pretty sure some of this count is false positives, but majority
> > is actually proper hits.
>
> Yeah, I know, we need to add this type of functionality to those busses
> as well.  I don't see a way of doing it other than this bus-by-bus
> conversion, do you?

Can't you push the **dev_groups from platform driver down to the
generic driver structure and handle them in driver_sysfs_add()?

>
> > > > We already emit KOBJ_BIND when we finish binding device to a driver,
> > > > regardless of the bus. I know we still need to teach systemd to handle
> > > > it properly, but I think it is better than sprinkling KOBJ_CHANGE
> > > > around.
> > >
> > > But the object's attributes did just change, which is what KOBJ_CHANGE
> > > tells userspace, so this should be the correct thing to say to
> > > userspace.
> > >
> > > And yes, ideally KOBJ_BIND would be handled, and it will be sent once
> > > the device's probe function succeeds, but we have to deal with old
> > > userspaces as well, right?
> >
> > Not for the new functionality, I do not think so. Newer kernels should
> > be compatible with older userspace as it not breaking it, but new
> > functionality is not guaranteed to be available with older userspace.
>
> I agree, but again, this is a kobject change (adding attributes), so
> I think the event type I picked here is the correct one.

I guess once you push it all into core you'll end up with 2 uevents
being emitted back-to-back and this seems inefficient.

If you really want KOBJ_CHANGE maybe have some additional attribute
like "CHANGE=driver-specific-attrs" in it? It's all quite ugly though.

Thanks.

-- 
Dmitry

Re: [PATCH 01/12 v2] Platform: add a dev_groups pointer to struct platform_driver

[Greg Kroah-Hartman]

On Sat, Jul 06, 2019 at 10:39:38AM -0700, Dmitry Torokhov wrote:
> On Sat, Jul 6, 2019 at 10:19 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Sat, Jul 06, 2019 at 10:04:39AM -0700, Dmitry Torokhov wrote:
> > > Hi Greg,
> > >
> > > On Sat, Jul 6, 2019 at 1:32 AM Greg Kroah-Hartman
> > > <gregkh@linuxfoundation.org> wrote:
> > > >
> > > > On Thu, Jul 04, 2019 at 02:17:22PM -0700, Dmitry Torokhov wrote:
> > > > > Hi Greg,
> > > > >
> > > > > On Thu, Jul 4, 2019 at 5:15 AM Greg Kroah-Hartman
> > > > > <gregkh@linuxfoundation.org> wrote:
> > > > > >
> > > > > > Platform drivers like to add sysfs groups to their device, but right now
> > > > > > they have to do it "by hand".  The driver core should handle this for
> > > > > > them, but there is no way to get to the bus-default attribute groups as
> > > > > > all platform devices are "special and unique" one-off drivers/devices.
> > > > > >
> > > > > > To combat this, add a dev_groups pointer to platform_driver which allows
> > > > > > a platform driver to set up a list of default attributes that will be
> > > > > > properly created and removed by the platform driver core when a probe()
> > > > > > function is successful and removed right before the device is unbound.
> > > > >
> > > > > Why is this limited to platform bus? Drivers for other buses also
> > > > > often want to augment list of their attributes during probe(). I'd
> > > > > move it to generic probe handling.
> > > >
> > > > This is not limited to the platform at all, the driver core supports
> > > > this for any bus type today, but it's then up to the bus-specific code
> > > > to pass that on to the driver core.  That's usually set for the
> > > > bus-specific attributes that they want exposed for all devices of that
> > > > bus type (see the bus_groups, dev_groups, and drv_groups pointers in
> > > > struct bus_type).
> > > >
> > > > For the platform devices, the problem is that this is something that the
> > > > individual drivers want after they bind to the device.  And as all
> > > > platform devices are "different" they can't be a "common" set of
> > > > attributes, so they need to be created after the device is bound to the
> > > > driver.
> > >
> > > I believe that your assertion that only platform devices want to
> > > install custom attributes is incorrect.
> >
> > Sorry, I didn't mean to imply that only platform drivers want to do
> > this, as you say, many other drivers do as well.
> >
> > > Drivers for devices attached
> > > to serio, i2c, USB, spi, etc, etc, all have additional attributes:
> > >
> > > dtor@dtor-ws:~/kernel/work (master *)$ grep -l '\(i2c\|usb\|spi\)'
> > > `git grep -l '\(device_add_group\|sysfs_create_group\)' -- drivers` |
> > > wc -l
> > > 170
> > >
> > > I am pretty sure some of this count is false positives, but majority
> > > is actually proper hits.
> >
> > Yeah, I know, we need to add this type of functionality to those busses
> > as well.  I don't see a way of doing it other than this bus-by-bus
> > conversion, do you?
> 
> Can't you push the **dev_groups from platform driver down to the
> generic driver structure and handle them in driver_sysfs_add()?

Sorry for the delay, got busy with the merge window...

Anyway, no, we can't call this then, because driver_sysfs_add() is
called before probe() is called.  So if probe() fails, we don't bind the
device to the driver.  We also should not be creating sysfs files for a
driver that has not had probe() called yet, as internal structures will
not be set up at that time.

> > > > > We already emit KOBJ_BIND when we finish binding device to a driver,
> > > > > regardless of the bus. I know we still need to teach systemd to handle
> > > > > it properly, but I think it is better than sprinkling KOBJ_CHANGE
> > > > > around.
> > > >
> > > > But the object's attributes did just change, which is what KOBJ_CHANGE
> > > > tells userspace, so this should be the correct thing to say to
> > > > userspace.
> > > >
> > > > And yes, ideally KOBJ_BIND would be handled, and it will be sent once
> > > > the device's probe function succeeds, but we have to deal with old
> > > > userspaces as well, right?
> > >
> > > Not for the new functionality, I do not think so. Newer kernels should
> > > be compatible with older userspace as it not breaking it, but new
> > > functionality is not guaranteed to be available with older userspace.
> >
> > I agree, but again, this is a kobject change (adding attributes), so
> > I think the event type I picked here is the correct one.
> 
> I guess once you push it all into core you'll end up with 2 uevents
> being emitted back-to-back and this seems inefficient.

It's not the nicest, I agree.  But, this is not all that common for
drivers to do, so it should not happen often enough to cause many
issues.  Not all devices in the system will have this happen for them.

> If you really want KOBJ_CHANGE maybe have some additional attribute
> like "CHANGE=driver-specific-attrs" in it? It's all quite ugly though.

What would that addition help out with?  You still need to rescan the
device attributes no matter what.  Trying to compare a list of
attributes with what is "new" is probably slower than just reading them
all over again "from scratch".

thanks,

greg k-h

Re: [PATCH 01/12 v2] Platform: add a dev_groups pointer to struct platform_driver

[Dmitry Torokhov]

On Fri, Jul 19, 2019 at 08:52:20PM +0900, Greg Kroah-Hartman wrote:
> On Sat, Jul 06, 2019 at 10:39:38AM -0700, Dmitry Torokhov wrote:
> > On Sat, Jul 6, 2019 at 10:19 AM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Sat, Jul 06, 2019 at 10:04:39AM -0700, Dmitry Torokhov wrote:
> > > > Hi Greg,
> > > >
> > > > On Sat, Jul 6, 2019 at 1:32 AM Greg Kroah-Hartman
> > > > <gregkh@linuxfoundation.org> wrote:
> > > > >
> > > > > On Thu, Jul 04, 2019 at 02:17:22PM -0700, Dmitry Torokhov wrote:
> > > > > > Hi Greg,
> > > > > >
> > > > > > On Thu, Jul 4, 2019 at 5:15 AM Greg Kroah-Hartman
> > > > > > <gregkh@linuxfoundation.org> wrote:
> > > > > > >
> > > > > > > Platform drivers like to add sysfs groups to their device, but right now
> > > > > > > they have to do it "by hand".  The driver core should handle this for
> > > > > > > them, but there is no way to get to the bus-default attribute groups as
> > > > > > > all platform devices are "special and unique" one-off drivers/devices.
> > > > > > >
> > > > > > > To combat this, add a dev_groups pointer to platform_driver which allows
> > > > > > > a platform driver to set up a list of default attributes that will be
> > > > > > > properly created and removed by the platform driver core when a probe()
> > > > > > > function is successful and removed right before the device is unbound.
> > > > > >
> > > > > > Why is this limited to platform bus? Drivers for other buses also
> > > > > > often want to augment list of their attributes during probe(). I'd
> > > > > > move it to generic probe handling.
> > > > >
> > > > > This is not limited to the platform at all, the driver core supports
> > > > > this for any bus type today, but it's then up to the bus-specific code
> > > > > to pass that on to the driver core.  That's usually set for the
> > > > > bus-specific attributes that they want exposed for all devices of that
> > > > > bus type (see the bus_groups, dev_groups, and drv_groups pointers in
> > > > > struct bus_type).
> > > > >
> > > > > For the platform devices, the problem is that this is something that the
> > > > > individual drivers want after they bind to the device.  And as all
> > > > > platform devices are "different" they can't be a "common" set of
> > > > > attributes, so they need to be created after the device is bound to the
> > > > > driver.
> > > >
> > > > I believe that your assertion that only platform devices want to
> > > > install custom attributes is incorrect.
> > >
> > > Sorry, I didn't mean to imply that only platform drivers want to do
> > > this, as you say, many other drivers do as well.
> > >
> > > > Drivers for devices attached
> > > > to serio, i2c, USB, spi, etc, etc, all have additional attributes:
> > > >
> > > > dtor@dtor-ws:~/kernel/work (master *)$ grep -l '\(i2c\|usb\|spi\)'
> > > > `git grep -l '\(device_add_group\|sysfs_create_group\)' -- drivers` |
> > > > wc -l
> > > > 170
> > > >
> > > > I am pretty sure some of this count is false positives, but majority
> > > > is actually proper hits.
> > >
> > > Yeah, I know, we need to add this type of functionality to those busses
> > > as well.  I don't see a way of doing it other than this bus-by-bus
> > > conversion, do you?
> > 
> > Can't you push the **dev_groups from platform driver down to the
> > generic driver structure and handle them in driver_sysfs_add()?
> 
> Sorry for the delay, got busy with the merge window...
> 
> Anyway, no, we can't call this then, because driver_sysfs_add() is
> called before probe() is called.  So if probe() fails, we don't bind the
> device to the driver.  We also should not be creating sysfs files for a
> driver that has not had probe() called yet, as internal structures will
> not be set up at that time.

Ah, yes, I got confused by the fact that driver_sysfs_remove is called
early. Anyway, I think you want something like this:

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 0df9b4461766..61d9d650d890 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -515,9 +515,17 @@ static int really_probe(struct device *dev, struct device_driver *drv)
 			goto probe_failed;
 	}
 
+	if (device_add_groups(dev, drv->dev_groups)) {
+		printk(KERN_ERR "%s: device_add_groups(%s) failed\n",
+			__func__, dev_name(dev));
+		goto dev_groups_failed;
+	}
+
 	if (test_remove) {
 		test_remove = false;
 
+		device_remove_groups(dev, drv->dev_groups);
+
 		if (dev->bus->remove)
 			dev->bus->remove(dev);
 		else if (drv->remove)
@@ -545,6 +553,11 @@ static int really_probe(struct device *dev, struct device_driver *drv)
 		 drv->bus->name, __func__, dev_name(dev), drv->name);
 	goto done;
 
+dev_groups_failed:
+	if (dev->bus->remove)
+		dev->bus->remove(dev);
+	else if (drv->remove)
+		drv->remove(dev);
 probe_failed:
 	if (dev->bus)
 		blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
@@ -1075,6 +1088,8 @@ static void __device_release_driver(struct device *dev, struct device *parent)
 
 		pm_runtime_put_sync(dev);
 
+		device_remove_groups(dev, drv->dev_groups);
+
 		if (dev->bus && dev->bus->remove)
 			dev->bus->remove(dev);
 		else if (drv->remove)
diff --git a/include/linux/device.h b/include/linux/device.h
index 4a295e324ac5..12aa8c687404 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -259,6 +259,8 @@ enum probe_type {
  * @resume:	Called to bring a device from sleep mode.
  * @groups:	Default attributes that get created by the driver core
  *		automatically.
+ * @dev_groups:	Additional attributes attached to device instance once the
+ *		it is bound to the driver.
  * @pm:		Power management operations of the device which matched
  *		this driver.
  * @coredump:	Called when sysfs entry is written to. The device driver
@@ -293,6 +295,7 @@ struct device_driver {
 	int (*suspend) (struct device *dev, pm_message_t state);
 	int (*resume) (struct device *dev);
 	const struct attribute_group **groups;
+	const struct attribute_group **dev_groups;
 
 	const struct dev_pm_ops *pm;
 	void (*coredump) (struct device *dev);


Thanks.

-- 
Dmitry

Re: [PATCH 01/12 v2] Platform: add a dev_groups pointer to struct platform_driver

[Greg Kroah-Hartman]

On Sat, Jul 20, 2019 at 07:38:57AM +0300, Dmitry Torokhov wrote:
> On Fri, Jul 19, 2019 at 08:52:20PM +0900, Greg Kroah-Hartman wrote:
> > On Sat, Jul 06, 2019 at 10:39:38AM -0700, Dmitry Torokhov wrote:
> > > On Sat, Jul 6, 2019 at 10:19 AM Greg Kroah-Hartman
> > > <gregkh@linuxfoundation.org> wrote:
> > > >
> > > > On Sat, Jul 06, 2019 at 10:04:39AM -0700, Dmitry Torokhov wrote:
> > > > > Hi Greg,
> > > > >
> > > > > On Sat, Jul 6, 2019 at 1:32 AM Greg Kroah-Hartman
> > > > > <gregkh@linuxfoundation.org> wrote:
> > > > > >
> > > > > > On Thu, Jul 04, 2019 at 02:17:22PM -0700, Dmitry Torokhov wrote:
> > > > > > > Hi Greg,
> > > > > > >
> > > > > > > On Thu, Jul 4, 2019 at 5:15 AM Greg Kroah-Hartman
> > > > > > > <gregkh@linuxfoundation.org> wrote:
> > > > > > > >
> > > > > > > > Platform drivers like to add sysfs groups to their device, but right now
> > > > > > > > they have to do it "by hand".  The driver core should handle this for
> > > > > > > > them, but there is no way to get to the bus-default attribute groups as
> > > > > > > > all platform devices are "special and unique" one-off drivers/devices.
> > > > > > > >
> > > > > > > > To combat this, add a dev_groups pointer to platform_driver which allows
> > > > > > > > a platform driver to set up a list of default attributes that will be
> > > > > > > > properly created and removed by the platform driver core when a probe()
> > > > > > > > function is successful and removed right before the device is unbound.
> > > > > > >
> > > > > > > Why is this limited to platform bus? Drivers for other buses also
> > > > > > > often want to augment list of their attributes during probe(). I'd
> > > > > > > move it to generic probe handling.
> > > > > >
> > > > > > This is not limited to the platform at all, the driver core supports
> > > > > > this for any bus type today, but it's then up to the bus-specific code
> > > > > > to pass that on to the driver core.  That's usually set for the
> > > > > > bus-specific attributes that they want exposed for all devices of that
> > > > > > bus type (see the bus_groups, dev_groups, and drv_groups pointers in
> > > > > > struct bus_type).
> > > > > >
> > > > > > For the platform devices, the problem is that this is something that the
> > > > > > individual drivers want after they bind to the device.  And as all
> > > > > > platform devices are "different" they can't be a "common" set of
> > > > > > attributes, so they need to be created after the device is bound to the
> > > > > > driver.
> > > > >
> > > > > I believe that your assertion that only platform devices want to
> > > > > install custom attributes is incorrect.
> > > >
> > > > Sorry, I didn't mean to imply that only platform drivers want to do
> > > > this, as you say, many other drivers do as well.
> > > >
> > > > > Drivers for devices attached
> > > > > to serio, i2c, USB, spi, etc, etc, all have additional attributes:
> > > > >
> > > > > dtor@dtor-ws:~/kernel/work (master *)$ grep -l '\(i2c\|usb\|spi\)'
> > > > > `git grep -l '\(device_add_group\|sysfs_create_group\)' -- drivers` |
> > > > > wc -l
> > > > > 170
> > > > >
> > > > > I am pretty sure some of this count is false positives, but majority
> > > > > is actually proper hits.
> > > >
> > > > Yeah, I know, we need to add this type of functionality to those busses
> > > > as well.  I don't see a way of doing it other than this bus-by-bus
> > > > conversion, do you?
> > > 
> > > Can't you push the **dev_groups from platform driver down to the
> > > generic driver structure and handle them in driver_sysfs_add()?
> > 
> > Sorry for the delay, got busy with the merge window...
> > 
> > Anyway, no, we can't call this then, because driver_sysfs_add() is
> > called before probe() is called.  So if probe() fails, we don't bind the
> > device to the driver.  We also should not be creating sysfs files for a
> > driver that has not had probe() called yet, as internal structures will
> > not be set up at that time.
> 
> Ah, yes, I got confused by the fact that driver_sysfs_remove is called
> early. Anyway, I think you want something like this:

Ah, nice, this looks good.  Let me try this and see how it goes...

> 
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 0df9b4461766..61d9d650d890 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -515,9 +515,17 @@ static int really_probe(struct device *dev, struct device_driver *drv)
>  			goto probe_failed;
>  	}
>  
> +	if (device_add_groups(dev, drv->dev_groups)) {
> +		printk(KERN_ERR "%s: device_add_groups(%s) failed\n",
> +			__func__, dev_name(dev));

dev_err() of course :)

thanks for the review, much appreciated.

greg k-h

Re: [PATCH 01/12 v2] Platform: add a dev_groups pointer to struct platform_driver

[Richard Gong]

Hi Greg,

On 7/25/19 8:44 AM, Greg Kroah-Hartman wrote:
> On Sat, Jul 20, 2019 at 07:38:57AM +0300, Dmitry Torokhov wrote:
>> On Fri, Jul 19, 2019 at 08:52:20PM +0900, Greg Kroah-Hartman wrote:
>>> On Sat, Jul 06, 2019 at 10:39:38AM -0700, Dmitry Torokhov wrote:
>>>> On Sat, Jul 6, 2019 at 10:19 AM Greg Kroah-Hartman
>>>> <gregkh@linuxfoundation.org> wrote:
>>>>>
>>>>> On Sat, Jul 06, 2019 at 10:04:39AM -0700, Dmitry Torokhov wrote:
>>>>>> Hi Greg,
>>>>>>
>>>>>> On Sat, Jul 6, 2019 at 1:32 AM Greg Kroah-Hartman
>>>>>> <gregkh@linuxfoundation.org> wrote:
>>>>>>>
>>>>>>> On Thu, Jul 04, 2019 at 02:17:22PM -0700, Dmitry Torokhov wrote:
>>>>>>>> Hi Greg,
>>>>>>>>
>>>>>>>> On Thu, Jul 4, 2019 at 5:15 AM Greg Kroah-Hartman
>>>>>>>> <gregkh@linuxfoundation.org> wrote:
>>>>>>>>>
>>>>>>>>> Platform drivers like to add sysfs groups to their device, but right now
>>>>>>>>> they have to do it "by hand".  The driver core should handle this for
>>>>>>>>> them, but there is no way to get to the bus-default attribute groups as
>>>>>>>>> all platform devices are "special and unique" one-off drivers/devices.
>>>>>>>>>
>>>>>>>>> To combat this, add a dev_groups pointer to platform_driver which allows
>>>>>>>>> a platform driver to set up a list of default attributes that will be
>>>>>>>>> properly created and removed by the platform driver core when a probe()
>>>>>>>>> function is successful and removed right before the device is unbound.
>>>>>>>>
>>>>>>>> Why is this limited to platform bus? Drivers for other buses also
>>>>>>>> often want to augment list of their attributes during probe(). I'd
>>>>>>>> move it to generic probe handling.
>>>>>>>
>>>>>>> This is not limited to the platform at all, the driver core supports
>>>>>>> this for any bus type today, but it's then up to the bus-specific code
>>>>>>> to pass that on to the driver core.  That's usually set for the
>>>>>>> bus-specific attributes that they want exposed for all devices of that
>>>>>>> bus type (see the bus_groups, dev_groups, and drv_groups pointers in
>>>>>>> struct bus_type).
>>>>>>>
>>>>>>> For the platform devices, the problem is that this is something that the
>>>>>>> individual drivers want after they bind to the device.  And as all
>>>>>>> platform devices are "different" they can't be a "common" set of
>>>>>>> attributes, so they need to be created after the device is bound to the
>>>>>>> driver.
>>>>>>
>>>>>> I believe that your assertion that only platform devices want to
>>>>>> install custom attributes is incorrect.
>>>>>
>>>>> Sorry, I didn't mean to imply that only platform drivers want to do
>>>>> this, as you say, many other drivers do as well.
>>>>>
>>>>>> Drivers for devices attached
>>>>>> to serio, i2c, USB, spi, etc, etc, all have additional attributes:
>>>>>>
>>>>>> dtor@dtor-ws:~/kernel/work (master *)$ grep -l '\(i2c\|usb\|spi\)'
>>>>>> `git grep -l '\(device_add_group\|sysfs_create_group\)' -- drivers` |
>>>>>> wc -l
>>>>>> 170
>>>>>>
>>>>>> I am pretty sure some of this count is false positives, but majority
>>>>>> is actually proper hits.
>>>>>
>>>>> Yeah, I know, we need to add this type of functionality to those busses
>>>>> as well.  I don't see a way of doing it other than this bus-by-bus
>>>>> conversion, do you?
>>>>
>>>> Can't you push the **dev_groups from platform driver down to the
>>>> generic driver structure and handle them in driver_sysfs_add()?
>>>
>>> Sorry for the delay, got busy with the merge window...
>>>
>>> Anyway, no, we can't call this then, because driver_sysfs_add() is
>>> called before probe() is called.  So if probe() fails, we don't bind the
>>> device to the driver.  We also should not be creating sysfs files for a
>>> driver that has not had probe() called yet, as internal structures will
>>> not be set up at that time.
>>
>> Ah, yes, I got confused by the fact that driver_sysfs_remove is called
>> early. Anyway, I think you want something like this:
> 
> Ah, nice, this looks good.  Let me try this and see how it goes...
> 

I tried Dmitry's patch on Intel Stratix10 platform and it works.

I added one minor change on the top of Dmitry's patch, since I think we 
need add one additional check prior to device_add_groups(). To align 
with Dmitry's patch, I also change my code to use the new dev_groups 
pointer in the struct of device_driver.

My changes are below,

diff --git a/include/linux/device.h b/include/linux/device.h
index 6717ade..9207aea3 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -296,6 +296,7 @@ struct device_driver {
         int (*suspend) (struct device *dev, pm_message_t state);
         int (*resume) (struct device *dev);
         const struct attribute_group **groups;
+       const struct attribute_group **dev_groups;

         const struct dev_pm_ops *pm;

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 994a907..a91e69f 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -554,9 +554,19 @@ static int really_probe(struct device *dev, struct 
device_driver *drv)
                         goto probe_failed;
         }

+       if (drv->dev_groups) {
+               if (device_add_groups(dev, drv->dev_groups)) {
+                       dev_err(dev, "device_add_groups(%s) failed\n",
+                               dev_name(dev));
+                       goto dev_groups_failed;
+               }
+       }
+
         if (test_remove) {
                 test_remove = false;

+               device_remove_groups(dev, drv->dev_groups);
+
                 if (dev->bus->remove)
                         dev->bus->remove(dev);
                 else if (drv->remove)
@@ -584,6 +594,12 @@ static int really_probe(struct device *dev, struct 
device_driver *drv)
                  drv->bus->name, __func__, dev_name(dev), drv->name);
         goto done;

+dev_groups_failed:
+       if (dev->bus->remove)
+               dev->bus->remove(dev);
+       else if (drv->remove)
+               drv->remove(dev);
+
  probe_failed:
         if (dev->bus)
                 blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
@@ -1114,6 +1130,8 @@ static void __device_release_driver(struct device 
*dev, struct device *parent)

                 pm_runtime_put_sync(dev);

+               device_remove_groups(dev, drv->dev_groups);
+
                 if (dev->bus && dev->bus->remove)
                         dev->bus->remove(dev);
                 else if (drv->remove)


diff --git a/drivers/firmware/stratix10-rsu.c 
b/drivers/firmware/stratix10-rsu.c
index 98d8030..93b44e9 100644
--- a/drivers/firmware/stratix10-rsu.c
+++ b/drivers/firmware/stratix10-rsu.c
@@ -391,9 +391,9 @@ static int stratix10_rsu_remove(struct 
platform_device *pdev)
  static struct platform_driver stratix10_rsu_driver = {
         .probe = stratix10_rsu_probe,
         .remove = stratix10_rsu_remove,
         .driver = {
                 .name = "stratix10-rsu",
+               .dev_groups = rsu_groups,
         },
  };

Regards,
Richard

>>
>> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
>> index 0df9b4461766..61d9d650d890 100644
>> --- a/drivers/base/dd.c
>> +++ b/drivers/base/dd.c
>> @@ -515,9 +515,17 @@ static int really_probe(struct device *dev, struct device_driver *drv)
>>   			goto probe_failed;
>>   	}
>>   
>> +	if (device_add_groups(dev, drv->dev_groups)) {
>> +		printk(KERN_ERR "%s: device_add_groups(%s) failed\n",
>> +			__func__, dev_name(dev));
> 
> dev_err() of course :)
> 
> thanks for the review, much appreciated.
> 
> greg k-h
> 

Re: [PATCH 01/12 v2] Platform: add a dev_groups pointer to struct platform_driver

[Greg Kroah-Hartman]

On Thu, Jul 25, 2019 at 02:02:03PM -0500, Richard Gong wrote:
> Hi Greg,
> 
> On 7/25/19 8:44 AM, Greg Kroah-Hartman wrote:
> > On Sat, Jul 20, 2019 at 07:38:57AM +0300, Dmitry Torokhov wrote:
> > > On Fri, Jul 19, 2019 at 08:52:20PM +0900, Greg Kroah-Hartman wrote:
> > > > On Sat, Jul 06, 2019 at 10:39:38AM -0700, Dmitry Torokhov wrote:
> > > > > On Sat, Jul 6, 2019 at 10:19 AM Greg Kroah-Hartman
> > > > > <gregkh@linuxfoundation.org> wrote:
> > > > > > 
> > > > > > On Sat, Jul 06, 2019 at 10:04:39AM -0700, Dmitry Torokhov wrote:
> > > > > > > Hi Greg,
> > > > > > > 
> > > > > > > On Sat, Jul 6, 2019 at 1:32 AM Greg Kroah-Hartman
> > > > > > > <gregkh@linuxfoundation.org> wrote:
> > > > > > > > 
> > > > > > > > On Thu, Jul 04, 2019 at 02:17:22PM -0700, Dmitry Torokhov wrote:
> > > > > > > > > Hi Greg,
> > > > > > > > > 
> > > > > > > > > On Thu, Jul 4, 2019 at 5:15 AM Greg Kroah-Hartman
> > > > > > > > > <gregkh@linuxfoundation.org> wrote:
> > > > > > > > > > 
> > > > > > > > > > Platform drivers like to add sysfs groups to their device, but right now
> > > > > > > > > > they have to do it "by hand".  The driver core should handle this for
> > > > > > > > > > them, but there is no way to get to the bus-default attribute groups as
> > > > > > > > > > all platform devices are "special and unique" one-off drivers/devices.
> > > > > > > > > > 
> > > > > > > > > > To combat this, add a dev_groups pointer to platform_driver which allows
> > > > > > > > > > a platform driver to set up a list of default attributes that will be
> > > > > > > > > > properly created and removed by the platform driver core when a probe()
> > > > > > > > > > function is successful and removed right before the device is unbound.
> > > > > > > > > 
> > > > > > > > > Why is this limited to platform bus? Drivers for other buses also
> > > > > > > > > often want to augment list of their attributes during probe(). I'd
> > > > > > > > > move it to generic probe handling.
> > > > > > > > 
> > > > > > > > This is not limited to the platform at all, the driver core supports
> > > > > > > > this for any bus type today, but it's then up to the bus-specific code
> > > > > > > > to pass that on to the driver core.  That's usually set for the
> > > > > > > > bus-specific attributes that they want exposed for all devices of that
> > > > > > > > bus type (see the bus_groups, dev_groups, and drv_groups pointers in
> > > > > > > > struct bus_type).
> > > > > > > > 
> > > > > > > > For the platform devices, the problem is that this is something that the
> > > > > > > > individual drivers want after they bind to the device.  And as all
> > > > > > > > platform devices are "different" they can't be a "common" set of
> > > > > > > > attributes, so they need to be created after the device is bound to the
> > > > > > > > driver.
> > > > > > > 
> > > > > > > I believe that your assertion that only platform devices want to
> > > > > > > install custom attributes is incorrect.
> > > > > > 
> > > > > > Sorry, I didn't mean to imply that only platform drivers want to do
> > > > > > this, as you say, many other drivers do as well.
> > > > > > 
> > > > > > > Drivers for devices attached
> > > > > > > to serio, i2c, USB, spi, etc, etc, all have additional attributes:
> > > > > > > 
> > > > > > > dtor@dtor-ws:~/kernel/work (master *)$ grep -l '\(i2c\|usb\|spi\)'
> > > > > > > `git grep -l '\(device_add_group\|sysfs_create_group\)' -- drivers` |
> > > > > > > wc -l
> > > > > > > 170
> > > > > > > 
> > > > > > > I am pretty sure some of this count is false positives, but majority
> > > > > > > is actually proper hits.
> > > > > > 
> > > > > > Yeah, I know, we need to add this type of functionality to those busses
> > > > > > as well.  I don't see a way of doing it other than this bus-by-bus
> > > > > > conversion, do you?
> > > > > 
> > > > > Can't you push the **dev_groups from platform driver down to the
> > > > > generic driver structure and handle them in driver_sysfs_add()?
> > > > 
> > > > Sorry for the delay, got busy with the merge window...
> > > > 
> > > > Anyway, no, we can't call this then, because driver_sysfs_add() is
> > > > called before probe() is called.  So if probe() fails, we don't bind the
> > > > device to the driver.  We also should not be creating sysfs files for a
> > > > driver that has not had probe() called yet, as internal structures will
> > > > not be set up at that time.
> > > 
> > > Ah, yes, I got confused by the fact that driver_sysfs_remove is called
> > > early. Anyway, I think you want something like this:
> > 
> > Ah, nice, this looks good.  Let me try this and see how it goes...
> > 
> 
> I tried Dmitry's patch on Intel Stratix10 platform and it works.
> 
> I added one minor change on the top of Dmitry's patch, since I think we need
> add one additional check prior to device_add_groups(). To align with
> Dmitry's patch, I also change my code to use the new dev_groups pointer in
> the struct of device_driver.

Thanks for testing!

> My changes are below,

<snip>

> --- a/drivers/firmware/stratix10-rsu.c
> +++ b/drivers/firmware/stratix10-rsu.c
> @@ -391,9 +391,9 @@ static int stratix10_rsu_remove(struct platform_device
> *pdev)
>  static struct platform_driver stratix10_rsu_driver = {
>         .probe = stratix10_rsu_probe,
>         .remove = stratix10_rsu_remove,
>         .driver = {
>                 .name = "stratix10-rsu",
> +               .dev_groups = rsu_groups,

I'd prefer to leave the dev_groups in the platform driver code, as no
one should have to do this crazy "sub structure definition" that
platform drivers seem to love to do.

Here's the patch that I currently have on top of Dmitry's that is
getting run through 0-day right now.

I'll resend the whole patch series once it passes (hopefully tomorrow).

thanks,

greg k-h


From 6ad595541f81407f401d992b89ae1269e88cb3be Mon Sep 17 00:00:00 2001
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Date: Thu, 25 Jul 2019 15:54:24 +0200
Subject: [PATCH 02/13] platform: add a dev_groups pointer to struct
 platform_driver

As the driver core now provides the ability to directly add/remove
device groups when a driver is bound/unbound to a device, just pass that
pointer along to the driver core.

This allows us to fix up platform drivers to not need to create/remove
groups "by hand" anymore.

Cc: Richard Gong <richard.gong@linux.intel.com>
Cc: Romain Izard <romain.izard.pro@gmail.com>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Mans Rullgard <mans@mansr.com>
Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Cc: Randy Dunlap <rdunlap@infradead.org>
Cc: linux-kernel@vger.kernel.org
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/base/platform.c         | 1 +
 include/linux/platform_device.h | 1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 506a0175a5a7..21b3817569cd 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -667,6 +667,7 @@ int __platform_driver_register(struct platform_driver *drv,
 	drv->driver.probe = platform_drv_probe;
 	drv->driver.remove = platform_drv_remove;
 	drv->driver.shutdown = platform_drv_shutdown;
+	drv->driver.dev_groups = drv->dev_groups;
 
 	return driver_register(&drv->driver);
 }
diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
index 9bc36b589827..9945a08b872a 100644
--- a/include/linux/platform_device.h
+++ b/include/linux/platform_device.h
@@ -189,6 +189,7 @@ struct platform_driver {
 	int (*resume)(struct platform_device *);
 	struct device_driver driver;
 	const struct platform_device_id *id_table;
+	const struct attribute_group **dev_groups;
 	bool prevent_deferred_probe;
 };
 
-- 
2.22.0

Re: [PATCH 01/12 v2] Platform: add a dev_groups pointer to struct platform_driver

[Dmitry Torokhov]

On Thu, Jul 25, 2019 at 09:04:43PM +0200, Greg Kroah-Hartman wrote:
> On Thu, Jul 25, 2019 at 02:02:03PM -0500, Richard Gong wrote:
> > Hi Greg,
> > 
> > On 7/25/19 8:44 AM, Greg Kroah-Hartman wrote:
> > > On Sat, Jul 20, 2019 at 07:38:57AM +0300, Dmitry Torokhov wrote:
> > > > On Fri, Jul 19, 2019 at 08:52:20PM +0900, Greg Kroah-Hartman wrote:
> > > > > On Sat, Jul 06, 2019 at 10:39:38AM -0700, Dmitry Torokhov wrote:
> > > > > > On Sat, Jul 6, 2019 at 10:19 AM Greg Kroah-Hartman
> > > > > > <gregkh@linuxfoundation.org> wrote:
> > > > > > > 
> > > > > > > On Sat, Jul 06, 2019 at 10:04:39AM -0700, Dmitry Torokhov wrote:
> > > > > > > > Hi Greg,
> > > > > > > > 
> > > > > > > > On Sat, Jul 6, 2019 at 1:32 AM Greg Kroah-Hartman
> > > > > > > > <gregkh@linuxfoundation.org> wrote:
> > > > > > > > > 
> > > > > > > > > On Thu, Jul 04, 2019 at 02:17:22PM -0700, Dmitry Torokhov wrote:
> > > > > > > > > > Hi Greg,
> > > > > > > > > > 
> > > > > > > > > > On Thu, Jul 4, 2019 at 5:15 AM Greg Kroah-Hartman
> > > > > > > > > > <gregkh@linuxfoundation.org> wrote:
> > > > > > > > > > > 
> > > > > > > > > > > Platform drivers like to add sysfs groups to their device, but right now
> > > > > > > > > > > they have to do it "by hand".  The driver core should handle this for
> > > > > > > > > > > them, but there is no way to get to the bus-default attribute groups as
> > > > > > > > > > > all platform devices are "special and unique" one-off drivers/devices.
> > > > > > > > > > > 
> > > > > > > > > > > To combat this, add a dev_groups pointer to platform_driver which allows
> > > > > > > > > > > a platform driver to set up a list of default attributes that will be
> > > > > > > > > > > properly created and removed by the platform driver core when a probe()
> > > > > > > > > > > function is successful and removed right before the device is unbound.
> > > > > > > > > > 
> > > > > > > > > > Why is this limited to platform bus? Drivers for other buses also
> > > > > > > > > > often want to augment list of their attributes during probe(). I'd
> > > > > > > > > > move it to generic probe handling.
> > > > > > > > > 
> > > > > > > > > This is not limited to the platform at all, the driver core supports
> > > > > > > > > this for any bus type today, but it's then up to the bus-specific code
> > > > > > > > > to pass that on to the driver core.  That's usually set for the
> > > > > > > > > bus-specific attributes that they want exposed for all devices of that
> > > > > > > > > bus type (see the bus_groups, dev_groups, and drv_groups pointers in
> > > > > > > > > struct bus_type).
> > > > > > > > > 
> > > > > > > > > For the platform devices, the problem is that this is something that the
> > > > > > > > > individual drivers want after they bind to the device.  And as all
> > > > > > > > > platform devices are "different" they can't be a "common" set of
> > > > > > > > > attributes, so they need to be created after the device is bound to the
> > > > > > > > > driver.
> > > > > > > > 
> > > > > > > > I believe that your assertion that only platform devices want to
> > > > > > > > install custom attributes is incorrect.
> > > > > > > 
> > > > > > > Sorry, I didn't mean to imply that only platform drivers want to do
> > > > > > > this, as you say, many other drivers do as well.
> > > > > > > 
> > > > > > > > Drivers for devices attached
> > > > > > > > to serio, i2c, USB, spi, etc, etc, all have additional attributes:
> > > > > > > > 
> > > > > > > > dtor@dtor-ws:~/kernel/work (master *)$ grep -l '\(i2c\|usb\|spi\)'
> > > > > > > > `git grep -l '\(device_add_group\|sysfs_create_group\)' -- drivers` |
> > > > > > > > wc -l
> > > > > > > > 170
> > > > > > > > 
> > > > > > > > I am pretty sure some of this count is false positives, but majority
> > > > > > > > is actually proper hits.
> > > > > > > 
> > > > > > > Yeah, I know, we need to add this type of functionality to those busses
> > > > > > > as well.  I don't see a way of doing it other than this bus-by-bus
> > > > > > > conversion, do you?
> > > > > > 
> > > > > > Can't you push the **dev_groups from platform driver down to the
> > > > > > generic driver structure and handle them in driver_sysfs_add()?
> > > > > 
> > > > > Sorry for the delay, got busy with the merge window...
> > > > > 
> > > > > Anyway, no, we can't call this then, because driver_sysfs_add() is
> > > > > called before probe() is called.  So if probe() fails, we don't bind the
> > > > > device to the driver.  We also should not be creating sysfs files for a
> > > > > driver that has not had probe() called yet, as internal structures will
> > > > > not be set up at that time.
> > > > 
> > > > Ah, yes, I got confused by the fact that driver_sysfs_remove is called
> > > > early. Anyway, I think you want something like this:
> > > 
> > > Ah, nice, this looks good.  Let me try this and see how it goes...
> > > 
> > 
> > I tried Dmitry's patch on Intel Stratix10 platform and it works.
> > 
> > I added one minor change on the top of Dmitry's patch, since I think we need
> > add one additional check prior to device_add_groups(). To align with
> > Dmitry's patch, I also change my code to use the new dev_groups pointer in
> > the struct of device_driver.
> 
> Thanks for testing!
> 
> > My changes are below,
> 
> <snip>
> 
> > --- a/drivers/firmware/stratix10-rsu.c
> > +++ b/drivers/firmware/stratix10-rsu.c
> > @@ -391,9 +391,9 @@ static int stratix10_rsu_remove(struct platform_device
> > *pdev)
> >  static struct platform_driver stratix10_rsu_driver = {
> >         .probe = stratix10_rsu_probe,
> >         .remove = stratix10_rsu_remove,
> >         .driver = {
> >                 .name = "stratix10-rsu",
> > +               .dev_groups = rsu_groups,
> 
> I'd prefer to leave the dev_groups in the platform driver code, as no
> one should have to do this crazy "sub structure definition" that
> platform drivers seem to love to do.

Heh.

dtor@penguin:~/kernel/work $ git grep -A15 "static struct spi_driver" --
drivers/ | grep "\.driver.*=" | wc -l
272

You will find the similar counts for i2c, and other buses, mostly
because we need to specify driver name, OF/ACPI match tables, and
pointer to PM ops.

So please just keep it in the generic driver structure.

Thanks.

-- 
Dmitry

Re: [PATCH 01/12 v2] Platform: add a dev_groups pointer to struct platform_driver

[Dmitry Torokhov]

Hi Richard,

On Thu, Jul 25, 2019 at 02:02:03PM -0500, Richard Gong wrote:
> Hi Greg,
> 
> On 7/25/19 8:44 AM, Greg Kroah-Hartman wrote:
> > On Sat, Jul 20, 2019 at 07:38:57AM +0300, Dmitry Torokhov wrote:
> > > On Fri, Jul 19, 2019 at 08:52:20PM +0900, Greg Kroah-Hartman wrote:
> > > > On Sat, Jul 06, 2019 at 10:39:38AM -0700, Dmitry Torokhov wrote:
> > > > > On Sat, Jul 6, 2019 at 10:19 AM Greg Kroah-Hartman
> > > > > <gregkh@linuxfoundation.org> wrote:
> > > > > > 
> > > > > > On Sat, Jul 06, 2019 at 10:04:39AM -0700, Dmitry Torokhov wrote:
> > > > > > > Hi Greg,
> > > > > > > 
> > > > > > > On Sat, Jul 6, 2019 at 1:32 AM Greg Kroah-Hartman
> > > > > > > <gregkh@linuxfoundation.org> wrote:
> > > > > > > > 
> > > > > > > > On Thu, Jul 04, 2019 at 02:17:22PM -0700, Dmitry Torokhov wrote:
> > > > > > > > > Hi Greg,
> > > > > > > > > 
> > > > > > > > > On Thu, Jul 4, 2019 at 5:15 AM Greg Kroah-Hartman
> > > > > > > > > <gregkh@linuxfoundation.org> wrote:
> > > > > > > > > > 
> > > > > > > > > > Platform drivers like to add sysfs groups to their device, but right now
> > > > > > > > > > they have to do it "by hand".  The driver core should handle this for
> > > > > > > > > > them, but there is no way to get to the bus-default attribute groups as
> > > > > > > > > > all platform devices are "special and unique" one-off drivers/devices.
> > > > > > > > > > 
> > > > > > > > > > To combat this, add a dev_groups pointer to platform_driver which allows
> > > > > > > > > > a platform driver to set up a list of default attributes that will be
> > > > > > > > > > properly created and removed by the platform driver core when a probe()
> > > > > > > > > > function is successful and removed right before the device is unbound.
> > > > > > > > > 
> > > > > > > > > Why is this limited to platform bus? Drivers for other buses also
> > > > > > > > > often want to augment list of their attributes during probe(). I'd
> > > > > > > > > move it to generic probe handling.
> > > > > > > > 
> > > > > > > > This is not limited to the platform at all, the driver core supports
> > > > > > > > this for any bus type today, but it's then up to the bus-specific code
> > > > > > > > to pass that on to the driver core.  That's usually set for the
> > > > > > > > bus-specific attributes that they want exposed for all devices of that
> > > > > > > > bus type (see the bus_groups, dev_groups, and drv_groups pointers in
> > > > > > > > struct bus_type).
> > > > > > > > 
> > > > > > > > For the platform devices, the problem is that this is something that the
> > > > > > > > individual drivers want after they bind to the device.  And as all
> > > > > > > > platform devices are "different" they can't be a "common" set of
> > > > > > > > attributes, so they need to be created after the device is bound to the
> > > > > > > > driver.
> > > > > > > 
> > > > > > > I believe that your assertion that only platform devices want to
> > > > > > > install custom attributes is incorrect.
> > > > > > 
> > > > > > Sorry, I didn't mean to imply that only platform drivers want to do
> > > > > > this, as you say, many other drivers do as well.
> > > > > > 
> > > > > > > Drivers for devices attached
> > > > > > > to serio, i2c, USB, spi, etc, etc, all have additional attributes:
> > > > > > > 
> > > > > > > dtor@dtor-ws:~/kernel/work (master *)$ grep -l '\(i2c\|usb\|spi\)'
> > > > > > > `git grep -l '\(device_add_group\|sysfs_create_group\)' -- drivers` |
> > > > > > > wc -l
> > > > > > > 170
> > > > > > > 
> > > > > > > I am pretty sure some of this count is false positives, but majority
> > > > > > > is actually proper hits.
> > > > > > 
> > > > > > Yeah, I know, we need to add this type of functionality to those busses
> > > > > > as well.  I don't see a way of doing it other than this bus-by-bus
> > > > > > conversion, do you?
> > > > > 
> > > > > Can't you push the **dev_groups from platform driver down to the
> > > > > generic driver structure and handle them in driver_sysfs_add()?
> > > > 
> > > > Sorry for the delay, got busy with the merge window...
> > > > 
> > > > Anyway, no, we can't call this then, because driver_sysfs_add() is
> > > > called before probe() is called.  So if probe() fails, we don't bind the
> > > > device to the driver.  We also should not be creating sysfs files for a
> > > > driver that has not had probe() called yet, as internal structures will
> > > > not be set up at that time.
> > > 
> > > Ah, yes, I got confused by the fact that driver_sysfs_remove is called
> > > early. Anyway, I think you want something like this:
> > 
> > Ah, nice, this looks good.  Let me try this and see how it goes...
> > 
> 
> I tried Dmitry's patch on Intel Stratix10 platform and it works.
> 
> I added one minor change on the top of Dmitry's patch, since I think we need
> add one additional check prior to device_add_groups().

sysfs_create_groups() returns success when NULL is passed, and
device_add_groups() is a simple wrapper around it.

If anything I'd prefer to codify the behavior of
device_add_groups()/sysfs_create_groups() via comments rather than
adding unneeded checks.

Thanks.

-- 
Dmitry

Re: [PATCH 04/11] firmware: arm_scpi: convert platform driver to use dev_groups

[Greg Kroah-Hartman]

On Thu, Jul 04, 2019 at 10:10:26AM +0100, Sudeep Holla wrote:
> On Thu, Jul 04, 2019 at 10:46:10AM +0200, Greg Kroah-Hartman wrote:
> > Platform drivers now have the option to have the platform core create
> > and remove any needed sysfs attribute files.  So take advantage of that
> > and do not register "by hand" a sysfs group of attributes.
> > 
> > Cc: Sudeep Holla <sudeep.holla@arm.com>
> 
> Assuming you plan to take this series as a whole,
> 
> Acked-by: Sudeep Holla <sudeep.holla@arm.com>

Thanks, there will be one more series, but ideally we can take the whole
thing as-is.

greg k-h

Re: [PATCH 04/11] firmware: arm_scpi: convert platform driver to use dev_groups

[Greg Kroah-Hartman]

On Thu, Jul 04, 2019 at 10:10:26AM +0100, Sudeep Holla wrote:
> On Thu, Jul 04, 2019 at 10:46:10AM +0200, Greg Kroah-Hartman wrote:
> > Platform drivers now have the option to have the platform core create
> > and remove any needed sysfs attribute files.  So take advantage of that
> > and do not register "by hand" a sysfs group of attributes.
> > 
> > Cc: Sudeep Holla <sudeep.holla@arm.com>
> 
> Assuming you plan to take this series as a whole,
> 
> Acked-by: Sudeep Holla <sudeep.holla@arm.com>

Thanks, there will be one more series, but ideally we can take the whole
thing as-is.

greg k-h

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel