From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932504AbbE2KD3 (ORCPT ); Fri, 29 May 2015 06:03:29 -0400 Received: from mga14.intel.com ([192.55.52.115]:23198 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932362AbbE2KDW (ORCPT ); Fri, 29 May 2015 06:03:22 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.13,516,1427785200"; d="scan'208";a="733619309" Message-ID: <1432893795.26331.21.camel@linux.intel.com> Subject: Re: [PATCH v2 8/8] mfd: Add support for Intel Sunrisepoint LPSS devices From: Andy Shevchenko To: Lee Jones Cc: "Rafael J. Wysocki" , linux-acpi@vger.kernel.org, linux-pm@vger.kernel.org, Greg Kroah-Hartman , Vinod Koul , Andrew Morton , Mika Westerberg , linux-kernel@vger.kernel.org, dmaengine@vger.kernel.org, Heikki Krogerus , Jarkko Nikula , "Wysocki, Rafael J" Date: Fri, 29 May 2015 13:03:15 +0300 In-Reply-To: <20150528131051.GR11677@x1> References: <1432570172-86963-1-git-send-email-andriy.shevchenko@linux.intel.com> <1432570172-86963-9-git-send-email-andriy.shevchenko@linux.intel.com> <20150527102241.GC11677@x1> <1432811843.26331.18.camel@linux.intel.com> <20150528131051.GR11677@x1> Organization: Intel Finland Oy Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.12.9-1+b1 Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 Intel Finland Oy