Re: [RFC v8 4/7] platform: x86: Add generic Intel IPC driver

From: sathya
Date: Sat Jan 20 2018 - 23:59:36 EST


Hi Heikki,


On 11/23/2017 05:29 AM, Heikki Krogerus wrote:
Hi,

On Sun, Oct 29, 2017 at 02:49:57AM -0700, sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx wrote:
+/**
+ * devm_intel_ipc_dev_create() - Create IPC device
+ * @dev : IPC parent device.
+ * @devname : Name of the IPC device.
+ * @cfg : IPC device configuration.
+ * @ops : IPC device ops.
+ *
+ * Resource managed API to create IPC device with
+ * given configuration.
+ *
+ * Return : IPC device pointer or ERR_PTR(error code).
+ */
+struct intel_ipc_dev *devm_intel_ipc_dev_create(struct device *dev,
+ const char *devname,
+ struct intel_ipc_dev_cfg *cfg,
+ struct intel_ipc_dev_ops *ops)
+{
+ struct intel_ipc_dev **ptr, *ipc_dev;
+ int ret;
+
+ if (!dev && !devname && !cfg)
+ return ERR_PTR(-EINVAL);
+
+ if (intel_ipc_dev_get(devname)) {
+ dev_err(dev, "IPC device %s already exist\n", devname);
+ return ERR_PTR(-EINVAL);
+ }
+
+ ptr = devres_alloc(devm_intel_ipc_dev_release, sizeof(*ptr),
+ GFP_KERNEL);
+ if (!ptr)
+ return ERR_PTR(-ENOMEM);
+
+ ipc_dev = kzalloc(sizeof(*ipc_dev), GFP_KERNEL);
+ if (!ipc_dev) {
+ ret = -ENOMEM;
+ goto err_dev_create;
+ }
+
+ ipc_dev->dev.class = &intel_ipc_class;
+ ipc_dev->dev.parent = dev;
+ ipc_dev->dev.groups = ipc_dev_groups;
+ ipc_dev->cfg = cfg;
+ ipc_dev->ops = ops;
+
+ mutex_init(&ipc_dev->lock);
+ init_completion(&ipc_dev->cmd_complete);
+ dev_set_drvdata(&ipc_dev->dev, ipc_dev);
+ dev_set_name(&ipc_dev->dev, devname);
+ device_initialize(&ipc_dev->dev);
+
+ ret = device_add(&ipc_dev->dev);
+ if (ret < 0) {
+ dev_err(&ipc_dev->dev, "%s device create failed\n",
+ __func__);
+ ret = -ENODEV;
+ goto err_dev_add;
+ }
+
+ if (ipc_dev->cfg->mode == IPC_DEV_MODE_IRQ) {
+ if (devm_request_irq(&ipc_dev->dev,
+ ipc_dev->cfg->irq,
+ ipc_dev_irq_handler,
+ ipc_dev->cfg->irqflags,
+ dev_name(&ipc_dev->dev),
+ ipc_dev)) {
+ dev_err(&ipc_dev->dev,
+ "Failed to request irq\n");
+ goto err_irq_request;
+ }
+ }
That looks really wrong to me. This is the class driver, so why are
you handling the interrupts of the devices in it? You are clearly
making assumption that the interrupt will always be used only for
command completion, but that may not be the case. No assumptions.
Yes. I only considered the current usage. But I agree with your
comment. I will fix it in next version.

Just define completion callbacks, and let the drivers handle the
actual interrupts.

+ *ptr = ipc_dev;
+
+ devres_add(dev, ptr);
+ return ipc_dev;
+
+err_irq_request:
+ device_del(&ipc_dev->dev);
+err_dev_add:
+ kfree(ipc_dev);
+err_dev_create:
+ devres_free(ptr);
+ return ERR_PTR(ret);
+}
+EXPORT_SYMBOL_GPL(devm_intel_ipc_dev_create);
+
+static int __init intel_ipc_init(void)
+{
+ ipc_channel_lock_init();
+ return class_register(&intel_ipc_class);
+}
+
+static void __exit intel_ipc_exit(void)
+{
+ class_unregister(&intel_ipc_class);
+}
+subsys_initcall(intel_ipc_init);
+module_exit(intel_ipc_exit);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Kuppuswamy Sathyanarayanan<sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx>");
+MODULE_DESCRIPTION("Intel IPC device class driver");
To be honest, creating an extra logical device for the IPC hosts, and
the entire class, all feel unnecessarily complex to me.

I want to avoid client drivers using different APIs to call everyone of these IPC drivers. One
solution for this issue is to use name of the device to get the device pointer and then use
that pointer in common API to make IPC calls. This solution is similar to whats used in
extcon framework.

To implement this model either we have to use class type to group all these devices or
implement a custom list to keep the references for all these devices.

I preferred to go with class based grouping, so that I can use class_find_device() API to
get the device pointer.


The goal of this patch should be possible to achieve in a much simpler
and flexible way. IMO this patch needs to be redesigned.
I am open for suggestions. I will send a separate mail to you discuss about the further
details.


Thanks,