Re: [PATCH 00/27] add pm_runtime_last_busy_and_autosuspend() helper

From: Felipe Balbi <balbi@ti.com>
To: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: linux-wireless@vger.kernel.org, Baruch Siach <baruch@tkos.co.il>,
	patches@opensource.wolfsonmicro.com, linux-doc@vger.kernel.org,
	Seungwon Jeon <tgih.jun@samsung.com>,
	Daniel Vetter <daniel.vetter@ffwll.ch>,
	Chris Ball <chris@printf.net>,
	dri-devel@lists.freedesktop.org, Marcin Wojtas <mw@semihalf.com>,
	"Rafael J. Wysocki" <rafael.j.wysocki@intel.com>,
	Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	Shinya Kuribayashi <skuribay@pobox.com>,
	Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>,
	Vinod Koul <vinod.koul@intel.com>,
	Jaehoon Chung <jh80.chung@samsung.com>,
	Jani Nikula <jani.nikula@intel.com>,
	Tomi Valkeinen <tomi.valkeinen@ti.com>,
	Alan Stern <stern@rowland.harvard.edu>,
	Ben Skeggs <bskeggs@redhat.com>,
	Bjorn Andersson <bjorn.andersson@sonymobile.com>,
	Wolfram Sang <wsa@the-dreams.de>,
	Dave Airlie <airlied@redhat.com>, Ulf Hansson <ulf.hansson@li>
Subject: Re: [PATCH 00/27] add pm_runtime_last_busy_and_autosuspend() helper
Date: Wed, 24 Sep 2014 15:32:19 -0500	[thread overview]
Message-ID: <20140924203219.GZ17997@saruman> (raw)
In-Reply-To: <2322320.9rETJijE2n@vostro.rjw.lan>

[-- Attachment #1.1: Type: text/plain, Size: 6265 bytes --]

On Wed, Sep 24, 2014 at 10:46:17PM +0200, Rafael J. Wysocki wrote:
> On Wednesday, September 24, 2014 03:15:58 PM Felipe Balbi wrote:
> > On Wed, Sep 24, 2014 at 10:28:07PM +0200, Rafael J. Wysocki wrote:
> > > On Wednesday, September 24, 2014 09:44:50 PM Vinod Koul wrote:
> > > > This patch series adds a simple macro pm_runtime_last_busy_and_autosuspend()
> > > > which invokes pm_runtime_mark_last_busy() and pm_runtime_put_autosuspend()
> > > > sequentially. Then we do a tree wide update of current patterns which are
> > > > present. As evident from log below this pattern is frequent in the
> > > > kernel.
> > > > 
> > > > This series can be found at
> > > > git://git.kernel.org/pub/scm/linux/kernel/git/vkoul/slave-dma.git
> > > > topic/pm_runtime_last_busy_and_autosuspend
> > > > 
> > > > Fengguang's kbuild has tested it so it shouldn't break things for anyone.
> > > > Barring one patch (explictyly mentioned in its changelog) rest are simple
> > > > replacements.
> > > > 
> > > > If all are okay, this should be merged thru PM tree as it depends on macro
> > > > addition.
> > > > 
> > > > Subhransu S. Prusty (1):
> > > >   PM: Add helper pm_runtime_last_busy_and_autosuspend()
> > > > 
> > > > Vinod Koul (26):
> > > >   dmaengine: ste_dma: use pm_runtime_last_busy_and_autosuspend helper
> > > >   extcon: arizona: use pm_runtime_last_busy_and_autosuspend helper
> > > >   drm/i915: use pm_runtime_last_busy_and_autosuspend helper
> > > >   drm/nouveau: use pm_runtime_last_busy_and_autosuspend helper
> > > >   drm/radeon: use pm_runtime_last_busy_and_autosuspend helper
> > > >   vga_switcheroo: use pm_runtime_last_busy_and_autosuspend helper
> > > >   i2c: designware: use pm_runtime_last_busy_and_autosuspend helper
> > > >   i2c: omap: use pm_runtime_last_busy_and_autosuspend helper
> > > >   i2c: qup: use pm_runtime_last_busy_and_autosuspend helper
> > > >   mfd: ab8500-gpadc: use pm_runtime_last_busy_and_autosuspend helper
> > > >   mfd: arizona: use pm_runtime_last_busy_and_autosuspend helper
> > > >   mei: use pm_runtime_last_busy_and_autosuspend helper
> > > >   mmc: use pm_runtime_last_busy_and_autosuspend helper
> > > >   mmc: mmci: use pm_runtime_last_busy_and_autosuspend helper
> > > >   mmc: omap_hsmmc: use pm_runtime_last_busy_and_autosuspend helper
> > > >   mmc: sdhci-pxav3: use pm_runtime_last_busy_and_autosuspend helper
> > > >   mmc: sdhci: use pm_runtime_last_busy_and_autosuspend helper
> > > >   NFC: trf7970a: use pm_runtime_last_busy_and_autosuspend helper
> > > >   pm2301-charger: use pm_runtime_last_busy_and_autosuspend helper
> > > >   spi: omap2-mcspi: use pm_runtime_last_busy_and_autosuspend helper
> > > >   spi: orion: use pm_runtime_last_busy_and_autosuspend helper
> > > >   spi: ti-qspi: use pm_runtime_last_busy_and_autosuspend helper
> > > >   spi: core: use pm_runtime_last_busy_and_autosuspend helper
> > > >   tty: serial: omap: use pm_runtime_last_busy_and_autosuspend helper
> > > >   usb: musb: omap2430: use pm_runtime_last_busy_and_autosuspend helper
> > > >   video: fbdev: use pm_runtime_last_busy_and_autosuspend helper
> > > > 
> > > >  Documentation/power/runtime_pm.txt          |    4 ++
> > > >  drivers/dma/ste_dma40.c                     |   30 ++++---------
> > > >  drivers/extcon/extcon-arizona.c             |    6 +--
> > > >  drivers/gpu/drm/i915/intel_pm.c             |    3 +-
> > > >  drivers/gpu/drm/nouveau/nouveau_connector.c |    3 +-
> > > >  drivers/gpu/drm/nouveau/nouveau_drm.c       |    9 +---
> > > >  drivers/gpu/drm/radeon/radeon_connectors.c  |   15 ++----
> > > >  drivers/gpu/drm/radeon/radeon_drv.c         |    5 +-
> > > >  drivers/gpu/drm/radeon/radeon_kms.c         |    6 +--
> > > >  drivers/gpu/vga/vga_switcheroo.c            |    7 +--
> > > >  drivers/i2c/busses/i2c-designware-core.c    |    3 +-
> > > >  drivers/i2c/busses/i2c-omap.c               |    6 +--
> > > >  drivers/i2c/busses/i2c-qup.c                |    3 +-
> > > >  drivers/mfd/ab8500-gpadc.c                  |    6 +--
> > > >  drivers/mfd/arizona-irq.c                   |    3 +-
> > > >  drivers/misc/mei/client.c                   |   12 ++----
> > > >  drivers/mmc/core/core.c                     |    3 +-
> > > >  drivers/mmc/host/mmci.c                     |   12 ++----
> > > >  drivers/mmc/host/omap_hsmmc.c               |   19 ++-------
> > > >  drivers/mmc/host/sdhci-pxav3.c              |    6 +--
> > > >  drivers/mmc/host/sdhci.c                    |    3 +-
> > > >  drivers/nfc/trf7970a.c                      |    3 +-
> > > >  drivers/power/pm2301_charger.c              |    3 +-
> > > >  drivers/spi/spi-omap2-mcspi.c               |    9 +---
> > > >  drivers/spi/spi-orion.c                     |    3 +-
> > > >  drivers/spi/spi-ti-qspi.c                   |    5 +-
> > > >  drivers/spi/spi.c                           |    6 +--
> > > >  drivers/tty/serial/omap-serial.c            |   60 +++++++++------------------
> > > >  drivers/usb/musb/omap2430.c                 |    6 +--
> > > >  drivers/video/fbdev/auo_k190x.c             |    9 +---
> > > >  include/linux/pm_runtime.h                  |    6 +++
> > > >  31 files changed, 97 insertions(+), 177 deletions(-)
> > > 
> > > OK, I guess this is as good as it gets.
> > > 
> > > What tree would you like it go through?
> > 
> > Do we really need this new helper ? I mean, the very moment when we
> > decide to implement ->runtime_idle() we will need to get rid of this
> > change. I wonder if it's really valid...
> 
> I'm not sure I'm following?  This seems to simply implement what drivers
> have been doing already as one function.  Why would it be invalid to reduce
> code duplication?

For two reasons:

1) the helper has no inteligence whatsoever. It just calls the same
functions.

2) the duplication will vanish whenever someone implements
->runtime_idle() and have that call pm_runtime_autosuspend() (like PCI
and USB buses are doing today). This will just be yet another line that
needs to change.

Frankly though, no strong feelings, I just think it's a commit that
doesn't bring that any benefits other than looking like one line was
removed.

-- 
balbi

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel