On Thu, 2015-05-28 at 14:10 +0100, Lee Jones wrote:

On Thu, 28 May 2015, Andy Shevchenko wrote:

quoted

On Wed, 2015-05-27 at 11:22 +0100, Lee Jones wrote:

quoted

On Mon, 25 May 2015, Andy Shevchenko wrote:

[]

quoted

quoted

quoted

+	intel_lpss_ltr_expose(lpss);
+
+	ret = intel_lpss_debugfs_add(lpss);
+	if (ret)
+		dev_warn(lpss->dev, "Failed to create debugfs entries\n");
+
+	if (intel_lpss_has_idma(lpss)) {
+		/*
+		 * Ensure the DMA driver is loaded before the host
+		 * controller device appears, so that the host controller
+		 * driver can request its DMA channels as early as
+		 * possible.
+		 *
+		 * If the DMA module is not there that's OK as well.
+		 */
+		intel_lpss_request_dma_module(LPSS_IDMA_DRIVER_NAME);
+
+		ret = mfd_add_devices(dev, lpss->devid, lpss->devs, 2,
+				      info->mem, info->irq, NULL);
+	} else {
+		ret = mfd_add_devices(dev, lpss->devid, lpss->devs + 1, 1,
+				      info->mem, info->irq, NULL);
+	}

I'm still not happy with the mfd_cells being manipulated in this way,
or with the duplication you have within them.  Why don't you place the
IDMA device it its own mfd_cell, then:

quoted

+	if (intel_lpss_has_idma(lpss)) {
+		intel_lpss_request_dma_module(LPSS_IDMA_DRIVER_NAME);
+
+		ret = mfd_add_devices(dev, TBC, idma_dev, ARRAY_SIZE(idma_dev),
+				      info->mem, info->irq, NULL);
+             /* Error check */
+	}
+
+	ret = mfd_add_devices(dev, TBC, proto_dev, ARRAY_SIZE(proto_dev),
+				      info->mem, info->irq, NULL);

Would be nicer to export mfd_add_device() in that case?

What do you mean by export?  What's wrong with using this code
segment?

I took a closer look into this, indeed, we can call mfd_add_devices() as
many time as we want to add a new child device.

Will refactor this piece of code.

quoted

quoted

quoted

+#ifdef CONFIG_PM_SLEEP
+#define INTEL_LPSS_SLEEP_PM_OPS			\
+	.prepare = intel_lpss_prepare,		\
+	.suspend = intel_lpss_suspend,		\
+	.resume = intel_lpss_resume,		\
+	.freeze = intel_lpss_suspend,		\
+	.thaw = intel_lpss_resume,		\
+	.poweroff = intel_lpss_suspend,		\
+	.restore = intel_lpss_resume,
+#endif
+
+#define INTEL_LPSS_RUNTIME_PM_OPS		\
+	.runtime_suspend = intel_lpss_suspend,	\
+	.runtime_resume = intel_lpss_resume,
+
+#else /* !CONFIG_PM */
+#define INTEL_LPSS_SLEEP_PM_OPS
+#define INTEL_LPSS_RUNTIME_PM_OPS
+#endif /* CONFIG_PM */
+
+#define INTEL_LPSS_PM_OPS(name)			\
+const struct dev_pm_ops name = {		\
+	INTEL_LPSS_SLEEP_PM_OPS			\
+	INTEL_LPSS_RUNTIME_PM_OPS		\

quoted

If you _really_ need .prepare, then it's likely that some other
platform might too.  It will be the same amount of code to just make
this generic, so do that instead please.

In 'linux/pm.h' ->prepare() is excluded since it's quite exotic to be 
in device drivers. That is my understanding why it makes not much sense
to provide a generic definition for that.

$ git grep -n '\.prepare[ \t]*=.*pm' drivers/ | wc -l
33
$ git grep -n SET_SYSTEM_SLEEP_PM_OPS drivers/ | wc -l
114
$ git grep -n UNIVERSAL_DEV_PM_OPS drivers/ | wc -l
9
…and there are a lot of drivers (hundreds+) that do
not use mentioned macros, and has no ->prepare() callback defined.

I can try to summon up Rafael to clarify this.

Yes, let's do that, as I'd like a second opinion on this, thanks.

Rafael, it would be nice to have your input here.

-- 
Andy Shevchenko [off-list ref]
Intel Finland Oy

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html