linux-kernel - Re: [PATCH v2 8/8] mfd: Add support for Intel Sunrisepoint LPSS devices

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 29 May 2015 13:03:15 +0300
From:	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:	Lee Jones <lee.jones@...aro.org>
Cc:	"Rafael J. Wysocki" <rjw@...ysocki.net>,
	linux-acpi@...r.kernel.org, linux-pm@...r.kernel.org,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Vinod Koul <vinod.koul@...el.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Mika Westerberg <mika.westerberg@...ux.intel.com>,
	linux-kernel@...r.kernel.org, dmaengine@...r.kernel.org,
	Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
	Jarkko Nikula <jarkko.nikula@...ux.intel.com>,
	"Wysocki, Rafael J" <rafael.j.wysocki@...el.com>
Subject: Re: [PATCH v2 8/8] mfd: Add support for Intel Sunrisepoint LPSS
 devices

On Thu, 2015-05-28 at 14:10 +0100, Lee Jones wrote:
> On Thu, 28 May 2015, Andy Shevchenko wrote:
> > On Wed, 2015-05-27 at 11:22 +0100, Lee Jones wrote:
> > > On Mon, 25 May 2015, Andy Shevchenko wrote:

[]

> > > > +	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:
> > > 
> > > > +	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.

> > > > +#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		\
> > 
> > > 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 <andriy.shevchenko@...el.com>
Intel Finland Oy

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/