Hi
a few comments.
First, as Govindraj mentioned, this should be cc'ed to
linux-arm-kernel@xxxxxxxxxxxxxxxxxxx. It's surprising that the linux-arm
mailing list is still running...
On Fri, 16 Sep 2011, Tero Kristo wrote:
> This driver will eventually support OMAP soc PRM module features, e.g. PRCM
> chain interrupts.
This driver should be a separate series and should be posted to
linux-kernel@xxxxxxxxxxxxxxx also, since it will presumably need to be
acked or merged by someone else who is responsible for the appropriate
drivers directory. Since it should probably go into drivers/mfd (see
below), it should also be sent to the MFD tree maintainer, Samuel Ortiz
<sameo@xxxxxxxxxxxxxxx>.
> Signed-off-by: Tero Kristo <t-kristo@xxxxxx>
> Cc: Paul Walmsley <paul@xxxxxxxxx>
> Cc: Kevin Hilman <khilman@xxxxxx>
> ---
> drivers/power/Kconfig | 7 ++++
> drivers/power/Makefile | 1 +
> drivers/power/omap_prm.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++
A couple of comments here:
This driver is eventually going to need to create the VC and VP
subdevices, so it makes sense to me to place the main portion of the
driver into drivers/mfd, rather than drivers/power, to avoid churn.
Also, the driver should probably be renamed to something like
'omap2430_prm.c' or something like that. It looks to me like there are at
least three different hardware "PRM" IP block designs:
- OMAP2420 (integrated into the PRCM)
- OMAP2430/3430/3630/3517 (standalone PRM)
- OMAP4430/4460, possibly also the 816x (standalone PRM with very
different register layout)
The idea being that the hwmods for these would all have different names
and would therefore be associated with different drivers/mfd drivers.
Looking at your patch 6/16, it looks like there's some code that can be
shared between OMAP3/4 for interrupt handling, so maybe that can go into a
shared file, like omap_prm_common.c. The omap2430_prm.c or omap4430_prm.c
files could pass the number of registers and IRQ register offset arrays to
the common code in platform_data. drivers/mfd/wm8350-{core,irq}.c is what
I'm looking at here as a reasonable approach to compare to; see
wm8350_irq_init().
> 3 files changed, 91 insertions(+), 0 deletions(-)
> create mode 100644 drivers/power/omap_prm.c
>
> diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
> index 57de051..e735a95 100644
> --- a/drivers/power/Kconfig
> +++ b/drivers/power/Kconfig
> @@ -249,4 +249,11 @@ config CHARGER_MAX8998
> Say Y to enable support for the battery charger control sysfs and
> platform data of MAX8998/LP3974 PMICs.
>
> +config OMAP_PRM
> + bool "OMAP Power and Reset Management driver"
> + depends on (ARCH_OMAP) && PM
> + help
> + Say Y to enable support for the OMAP Power and Reset Management
> + driver.
> +
> endif # POWER_SUPPLY
> diff --git a/drivers/power/Makefile b/drivers/power/Makefile
> index b4af13d..8df93c2 100644
> --- a/drivers/power/Makefile
> +++ b/drivers/power/Makefile
> @@ -13,6 +13,7 @@ obj-$(CONFIG_WM831X_BACKUP) += wm831x_backup.o
> obj-$(CONFIG_WM831X_POWER) += wm831x_power.o
> obj-$(CONFIG_WM8350_POWER) += wm8350_power.o
> obj-$(CONFIG_TEST_POWER) += test_power.o
> +obj-$(CONFIG_OMAP_PRM) += omap_prm.o
>
> obj-$(CONFIG_BATTERY_DS2760) += ds2760_battery.o
> obj-$(CONFIG_BATTERY_DS2780) += ds2780_battery.o
> diff --git a/drivers/power/omap_prm.c b/drivers/power/omap_prm.c
> new file mode 100644
> index 0000000..dfc0920
> --- /dev/null
> +++ b/drivers/power/omap_prm.c
> @@ -0,0 +1,83 @@
> +/*
> + * OMAP Power and Reset Management (PRM) driver
> + *
> + * Copyright (C) 2011 Texas Instruments, Inc.
> + *
> + * Author: Tero Kristo <t-kristo@xxxxxx>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/ctype.h>
> +#include <linux/module.h>
> +#include <linux/io.h>
> +#include <linux/slab.h>
> +#include <linux/init.h>
> +#include <linux/err.h>
> +#include <linux/platform_device.h>
> +
> +#define DRIVER_NAME "omap-prm"
> +
> +struct omap_prm_device {
> + struct platform_device pdev;
> +};
Hmm. This doesn't look right. The platform_device should be created as
part of an omap_device_build() call in code that resides in
arch/arm/mach-omap2/ - see for example arch/arm/mach-omap2/hwspinlock.c
> +
> +static struct omap_prm_device prm_dev = {
> + .pdev = {
> + .name = DRIVER_NAME,
> + .id = -1,
> + },
> +};
> +
> +static int __init omap_prm_probe(struct platform_device *pdev)
> +{
> + return 0;
> +}
> +
> +static int __devexit omap_prm_remove(struct platform_device *pdev)
> +{
> + return 0;
> +}
> +
> +static struct platform_driver prm_driver = {
> + .remove = __exit_p(omap_prm_remove),
This should define a probe function that the LDM core will call when it
matches the driver's driver name with the hwmod's name (in the case of
OMAP).
> + .driver = {
> + .name = DRIVER_NAME,
> + },
> +};
> +
> +static int __init omap_prm_init(void)
> +{
> + int ret;
> +
> + ret = platform_device_register(&prm_dev.pdev);
> +
> + if (ret) {
> + printk(KERN_ERR "Unable to register PRM device: %d\n", ret);
> + return ret;
> + }
> +
> + ret = platform_driver_probe(&prm_driver, omap_prm_probe);
> +
> + if (ret)
> + printk(KERN_ERR "PRM driver probe failed: %d\n", ret);
> +
> + return ret;
> +}
> +subsys_initcall(omap_prm_init);
This function shouldn't be needed.
> +
> +static void __exit omap_prm_exit(void)
> +{
> + platform_device_unregister(&prm_dev.pdev);
> + platform_driver_unregister(&prm_driver);
> +}
> +module_exit(omap_prm_exit);
Nor should this one.
> +
> +MODULE_ALIAS("platform:"DRIVER_NAME);
> +MODULE_AUTHOR("Tero Kristo <t-kristo@xxxxxx>");
> +MODULE_DESCRIPTION("OMAP PRM driver");
> +MODULE_LICENSE("GPL");
> --
> 1.7.4.1
>
>
> Texas Instruments Oy, Tekniikantie 12, 02150 Espoo. Y-tunnus: 0115040-6. Kotipaikka: Helsinki
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
- Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
