Messages in this thread | | | Date | Tue, 11 Dec 2012 10:20:51 +0900 | Subject | Re: [PATCH RFC] PM/Devfreq: Add Exynos5-bus devfreq driver for Exynos5250. | From | MyungJoo Ham <> |
| |
On Fri, Nov 30, 2012 at 5:42 PM, Abhilash Kesavan <a.kesavan@samsung.com> wrote: > Exynos5-bus device devfreq driver monitors PPMU counters and > adjusts operating frequencies and voltages with OPP. ASV should > be used to provide appropriate voltages as per the speed group > of the SoC rather than using a constant 1.025V. > > Signed-off-by: Abhilash Kesavan <a.kesavan@samsung.com> > Cc: Jonghwan Choi <jhbird.choi@samsung.com> > Cc: Kukjin Kim <kgene.kim@samsung.com>
I've got a few general comments and questions on your patch.
> --- > This code is based on Jonghwan Choi's <jhbird.choi@samsung.com> devfreq work > for Exynos5250. This requires corresponding machine specific changes which > will be posted once the driver is reviewed. > > drivers/devfreq/Kconfig | 10 + > drivers/devfreq/Makefile | 1 + > drivers/devfreq/exynos5_bus.c | 595 ++++++++++++++++++++++++++++++++++++++++ > drivers/devfreq/exynos5_ppmu.c | 395 ++++++++++++++++++++++++++ > drivers/devfreq/exynos5_ppmu.h | 26 ++ > drivers/devfreq/exynos_ppmu.c | 56 ++++ > drivers/devfreq/exynos_ppmu.h | 79 ++++++ > 7 files changed, 1162 insertions(+), 0 deletions(-) > create mode 100644 drivers/devfreq/exynos5_bus.c > create mode 100644 drivers/devfreq/exynos5_ppmu.c > create mode 100644 drivers/devfreq/exynos5_ppmu.h > create mode 100644 drivers/devfreq/exynos_ppmu.c > create mode 100644 drivers/devfreq/exynos_ppmu.h
I understand that Exynos PPMU drivers seem not to be used (at least in mainline Linux) widely and it'd be convinent for a bus driver to have ppmu driver located in the same source directory.
However, I don't feel very comfortable to have ppmu drivers explicitly landing in devfreq directory. Would it be possible to place them somewhere else? (in drivers/misc, arch/arm/mach-exynos, or somewhere appropriate?) If PPMU drivers really have nowhere to relocate, they may be located along with its sole user (exynos5_bus.c) anyway.
> + > +struct exynos5_bus_int_handle { > + struct list_head node; > + struct delayed_work work; > + bool boost; > + bool poll; > + unsigned long min; > +};
It appears that "boost" is something may be handled by per-dev QoS. It looks like that you are reimplementing the pm-qos infrastructure in the driver.
Could you please implement this w/ per-dev QoS? Or explain why this is required instead of per-dev PM-QoS?
Cheers, MyungJoo
-- MyungJoo Ham, Ph.D. Mobile Software Platform Lab, DMC Business, Samsung Electronics
| |