Re: [PATCH v3 5/8] platform/chrome: Introduce cros_ec_device_alloc()

[Tzung-Bi Shih <tzungbi@kernel.org>]

On Mon, Jul 21, 2025 at 08:15:07AM +0200, Greg KH wrote:
> On Mon, Jul 21, 2025 at 04:44:53AM +0000, Tzung-Bi Shih wrote:
> > Prepare to decouple the lifecycle of struct cros_ec_device from specific
> > device by introducing a kref.
> 
> Ick, are you sure?  This is a device, so use struct device for it.
> That's what it is there for.
> 
> So shouldn't this be its own 'struct device' and let the driver core
> handle it correctly instead of trying to have a "child" structure with a
> reference count that is not shown in sysfs at all?

Thanks for the review.

To make sure I understand, do the following patches (draft and simplified) make
sense?  Short summary:
- cros_ec_device_alloc() allocates resources and setup the destructor.
- cros_ec_register() adds the device to the device hierarchy.
- cros_ec_unregister() removes the device.
- cros_ec_chardev_open() gets a refcount of the device.
- cros_ec_chardev_release() puts the refcount.

diff --git a/drivers/platform/chrome/cros_ec.c b/drivers/platform/chrome/cros_ec.c
index f8ffdc1e1b16..4d0f1ffc3387 100644
--- a/drivers/platform/chrome/cros_ec.c
+++ b/drivers/platform/chrome/cros_ec.c
@@ -11,0 +12 @@
+#include <linux/idr.h>
@@ -22,0 +24,6 @@
+static DEFINE_IDA(cros_ec_devid_ida);
+
+static struct class cros_ec_class = {
+	.name = "cros_ec",
+};
+
@@ -101,0 +109,44 @@ EXPORT_SYMBOL(cros_ec_irq_thread);
+static void cros_ec_device_release(struct device *dev)
+{
+	struct cros_ec_device *ec_dev = container_of(dev, struct cros_ec_device, this_dev);
+
+	ida_free(&cros_ec_devid_ida, ec_dev->devid);
+	kfree(ec_dev);
+}
+
+/**
+ * cros_ec_device_alloc - Allocate a new struct cros_ec_device.
+ *
+ * @parent: The parent device.
+ */
+struct cros_ec_device *cros_ec_device_alloc(struct device *parent)
+{
+	struct cros_ec_device *ec_dev = NULL;
+	int id = -1;
+
+	ec_dev = kzalloc(sizeof(*ec_dev), GFP_KERNEL);
+	if (!ec_dev)
+		goto err;
+
+	id = ida_alloc(&cros_ec_devid_ida, GFP_KERNEL);
+	if (id < 0)
+		goto err;
+
+	ec_dev->dev = &ec_dev->this_dev;
+	ec_dev->this_dev.parent = parent;
+	ec_dev->this_dev.release = cros_ec_device_release;
+	ec_dev->this_dev.class = &cros_ec_class;
+	ec_dev->devid = id;
+
+	device_initialize(&ec_dev->this_dev);
+	dev_set_name(&ec_dev->this_dev, "cros_ec_transport.%d", id);
+	dev_set_drvdata(&ec_dev->this_dev, ec_dev);
+	return ec_dev;
+err:
+	if (id >= 0)
+		ida_free(&cros_ec_devid_ida, id);
+	kfree(ec_dev);
+	return NULL;
+}
+EXPORT_SYMBOL(cros_ec_device_alloc);
+
@@ -206,0 +258,4 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
+	err = device_add(dev);
+	if (err)
+		return err;
+
@@ -334,0 +390,2 @@ void cros_ec_unregister(struct cros_ec_device *ec_dev)
+
+	device_unregister(&ec_dev->this_dev);
@@ -500,0 +558,19 @@ EXPORT_SYMBOL(cros_ec_resume);
+static int __init cros_ec_init(void)
+{
+	int ret;
+
+	ret = class_register(&cros_ec_class);
+	if (ret)
+		pr_err("cros_ec: Failed to register device class\n");
+
+	return ret;
+}
+
+static void __exit cros_ec_exit(void)
+{
+	class_unregister(&cros_ec_class);
+}
+
+module_init(cros_ec_init);
+module_exit(cros_ec_exit);
+

diff --git a/drivers/platform/chrome/cros_ec_spi.c b/drivers/platform/chrome/cros_ec_spi.c
index 8ca0f854e7ac..6851e466e1dc 100644
--- a/drivers/platform/chrome/cros_ec_spi.c
+++ b/drivers/platform/chrome/cros_ec_spi.c
@@ -752 +752 @@ static int cros_ec_spi_probe(struct spi_device *spi)
-	ec_dev = devm_kzalloc(dev, sizeof(*ec_dev), GFP_KERNEL);
+	ec_dev = cros_ec_device_alloc(dev);
@@ -760 +759,0 @@ static int cros_ec_spi_probe(struct spi_device *spi)
-	ec_dev->dev = dev;

diff --git a/drivers/platform/chrome/cros_ec_chardev.c b/drivers/platform/chrome/cros_ec_chardev.c
index c9d80ad5b57e..13109ef6e452 100644
--- a/drivers/platform/chrome/cros_ec_chardev.c
+++ b/drivers/platform/chrome/cros_ec_chardev.c
@@ -183,0 +184 @@ static int cros_ec_chardev_open(struct inode *inode, struct file *filp)
+	get_device(ec_dev->dev);
@@ -258,0 +260 @@ static int cros_ec_chardev_release(struct inode *inode, struct file *filp)
+	put_device(ec_dev->dev);