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:
> >
> > > 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 <andriy.shevchenko@xxxxxxxxxxxxxxx>
> > > + * Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
> > > + *
> > > + * 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 <linux/acpi.h>
> > > +#include <linux/ioport.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/module.h>
> > > +#include <linux/pm.h>
> > > +#include <linux/pm_runtime.h>
> > > +#include <linux/platform_device.h>
> >
> > [...]
> >
> > > + 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?
What do you mean by export? What's wrong with using this code
segment?
> > > + if (ret < 0)
> >
> > if (!ret)
>
> Do you mean a) if (ret) or b) if (!ret) return 0; ?
>
> Will be fixed for option a).
Right.
> > > +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'.
Ah, multiple devices, yes good point.
[...]
> > > +#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.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
