From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753928AbbE1LRm (ORCPT ); Thu, 28 May 2015 07:17:42 -0400 Received: from mga01.intel.com ([192.55.52.88]:13366 "EHLO mga01.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753802AbbE1LRa (ORCPT ); Thu, 28 May 2015 07:17:30 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.13,512,1427785200"; d="scan'208";a="733045551" Message-ID: <1432811843.26331.18.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 Date: Thu, 28 May 2015 14:17:23 +0300 In-Reply-To: <20150527102241.GC11677@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> 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 Wed, 2015-05-27 at 11:22 +0100, Lee Jones wrote: > On Mon, 25 May 2015, Andy Shevchenko wrote: > > > The new coming Intel platforms such as Skylake will contain Sunrisepoint PCH. > > The main difference to the previous platforms is that the LPSS devices are > > compound devices where usually main (SPI, HSUART, or I2C) and DMA IPs are > > present. > > > > This patch brings the driver for such devices found on Sunrisepoint PCH. Thanks for comments. My answers below. > > --- /dev/null > > +++ b/drivers/mfd/intel-lpss-acpi.c > > @@ -0,0 +1,84 @@ > > +/* > > + * Intel LPSS ACPI support. > > + * > > + * Copyright (C) 2015, Intel Corporation > > + * > > + * Authors: Andy Shevchenko > > + * Mika Westerberg > > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License version 2 as > > + * published by the Free Software Foundation. > > + */ > > + > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > [...] > > > +#include "intel-lpss.h" > > +int intel_lpss_probe(struct device *dev, > > + const struct intel_lpss_platform_info *info) > > +{ > > + struct intel_lpss *lpss; > > + int ret; > > + > > + if (!info || !info->mem || info->irq <= 0) > > + return -EINVAL; > > + > > + lpss = devm_kzalloc(dev, sizeof(*lpss), GFP_KERNEL); > > + if (!lpss) > > + return -ENOMEM; > > + > > + lpss->priv = devm_ioremap(dev, info->mem->start + LPSS_PRIV_OFFSET, > > + LPSS_PRIV_SIZE); > > + if (!lpss->priv) > > + return -ENOMEM; > > + > > + lpss->info = info; > > + lpss->dev = dev; > > + lpss->caps = readl(lpss->priv + LPSS_PRIV_CAPS); > > + > > + dev_set_drvdata(dev, lpss); > > + > > + ret = intel_lpss_assign_devs(lpss); > > + if (ret) > > + return ret; > > + > > + intel_lpss_init_dev(lpss); > > [...] > > > + lpss->devid = ida_simple_get(&intel_lpss_devid_ida, 0, 0, GFP_KERNEL); > > + if (lpss->devid < 0) > > + return lpss->devid; > > + > > + ret = intel_lpss_register_clock(lpss); > > + if (ret < 0) > > + goto err_clk_register; > > Still not convinced by this. I'd like Mike (who you *still* have not > CC'ed), to review. I will include him on next iteration. > > + 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? > > + if (ret < 0) > > if (!ret) Do you mean a) if (ret) or b) if (!ret) return 0; ? Will be fixed for option a). > > +static int resume_lpss_device(struct device *dev, void *data) > > +{ > > + pm_runtime_resume(dev); > > + return 0; > > +} > > + > > +int intel_lpss_prepare(struct device *dev) > > +{ > > + /* > > + * Resume both child devices before entering system sleep. This > > + * ensures that they are in proper state before they get suspended. > > + */ > > + device_for_each_child_reverse(dev, NULL, resume_lpss_device); > > Why can't you do this in intel_lpss_suspend()? > > Then you can get rid of all the hand-rolled nonsense you have in the > header file and use SET_SYSTEM_SLEEP_PM_OPS instead. > > Does something happen after .prepare() and before .suspend() that > prevents this from working? I rely on what Mika answered you. > > +static int __init intel_lpss_init(void) > > +{ > > + intel_lpss_debugfs = debugfs_create_dir("intel_lpss", NULL); > > Any reason this can't be done in .probe()? ->probe is called per device, but we have one global folder for all of them. So, intel_lpss/ dev_name 1/ capabilities ... dev_name 2/ capabilities ... ... I doubt debugfs_create_dir() works like 'mkdir -p'. > > + return 0; > > +} > > +module_init(intel_lpss_init); > > + > > +static void __exit intel_lpss_exit(void) > > +{ > > + debugfs_remove(intel_lpss_debugfs); > > .remove()? See above. > > +++ b/drivers/mfd/intel-lpss.h [] > > +#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. -- Andy Shevchenko Intel Finland Oy