RE: [PATCH 9/9] OMAP:GPIO: Implement GPIO as a platform device
-Original Message-
From: Kevin Hilman [mailto:khil...@deeprootsystems.com]
Sent: Saturday, May 01, 2010 5:34 AM
To: Varadarajan, Charulatha
Cc: linux-omap@vger.kernel.org; Nayak, Rajendra; p...@pwsan.com;
t...@atomide.com
Subject: Re: [PATCH 9/9] OMAP:GPIO: Implement GPIO as a platform device
[...]
+static int init_gpio_info(void)
+{
+ gpio_bank_bits = 32;
+
+ if (cpu_is_omap15xx()) {
+ gpio_bank_count = 2;
+ gpio_bank_bits = 16;
+ } else if (cpu_is_omap16xx()) {
+ gpio_bank_count = 5;
+ gpio_bank_bits = 16;
+ } else if (cpu_is_omap7xx())
+ gpio_bank_count = 7;
+ else if (cpu_is_omap242x())
+ gpio_bank_count = 4;
+ else if (cpu_is_omap243x())
+ gpio_bank_count = 5;
+ else if (cpu_is_omap34xx() || cpu_is_omap44xx())
+ gpio_bank_count = OMAP34XX_NR_GPIOS;
Both the bank count and bank bits could be part of platform_data
and set in the SoC specific init. This is the GPIO driver part
and we're trying to make this as SoC independent as possible. Anytime
you need to add a cpu_is* or #ifdef in this code indicates something
that should be part of SoC specific init and passed in.
bank count and bank bits are not specific to each device, but SoC specific.
Hence I did not consider passing it as part of platform_data, because this
information would be duplicated across all devices in that SoC.
It would be good if we have a way to pass the SoC specific data which is
common for all the devices rather than duplicating them by sending them via
platform_data.
Anyways, I can move it to platform_data if there is no other way.
Probably the right place for the SoC specifics is attached to the SoC
specific hwmod using the dev_attr pointer. That struct can then
be passed in via platform_data.
You can see how Thara did this for SmartReflex as an example.
Using dev_attr for OMAP2PLUS is fine. How about OMAP1?
1. To be implemented in a uniform way, may I get this info as
part of platform_data?
2. Use dev_attr for OMAP2PLUS and use platform_data for OMAP1?
Any other way? Please suggest.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 9/9] OMAP:GPIO: Implement GPIO as a platform device
-Original Message- From: Kevin Hilman [mailto:khil...@deeprootsystems.com] Sent: Thursday, May 06, 2010 2:30 AM To: Varadarajan, Charulatha Cc: linux-omap@vger.kernel.org; Nayak, Rajendra; p...@pwsan.com; t...@atomide.com Subject: Re: [PATCH 9/9] OMAP:GPIO: Implement GPIO as a platform device Varadarajan, Charulatha ch...@ti.com writes: -Original Message- From: Kevin Hilman [mailto:khil...@deeprootsystems.com] Sent: Saturday, May 01, 2010 5:34 AM To: Varadarajan, Charulatha Cc: linux-omap@vger.kernel.org; Nayak, Rajendra; p...@pwsan.com; t...@atomide.com Subject: Re: [PATCH 9/9] OMAP:GPIO: Implement GPIO as a platform device Charulatha V ch...@ti.com writes: This patch implements GPIO as a early platform device. Also it implements OMAP2PLUS specific GPIO as HWMOD FW adapted driver. Should include a summary explanation of why you're converting to an early platform device as well. Okay. Inorder to convert GPIO as platform device, modifications are required in clock_data.c files so that device names can be used to obtain clock instead of getting clocks by name/NULL ptr. ok Currently early platform device register does not do device_pm_init. Hence pm_runtime functions are not used to enable the GPIO device since gpio is early platform device. OK for now, since this isn't using runtime PM, but maybe we need a late_initcall() here to do the device_pm_init() + pm_runtime_enable() This change log needs more description of the intended init sequence. Right now it seems that there are multiple init paths. Now that GPIO is an early_platform_device, we should be able to at least make omap_gpio_init() static and remove its usage from all the board files. This was the implementation that I initially did in my previous patch series. The following needs to be considered: 1. omap2_init_devices() might be too late for early_gpio_init() to be placed in it. 2. omap_init_irq() needs to be completed before calling early_gpio_init(). So, if early_gpio_init() is called from omap2_init_common_hw(), we need to have omap_init_irq() called before omap2_init_common_hw(). But Tony objected this approach mentioning that board might not boot up as omap2_init_common_hw() has to be done asap. That's why, I had not moved the omap_gpio_init() usage from board files. OK... for now. I'd still like to see GPIO init consolidated as there's no (good) reason why every board file has to init GPIOs when it's common for all SoCs, but this doesn't necessarily have to be done in your series. Although, if you do it for OMAP1 (as proposed below) you should do similar for OMAP2+. Okay. Will use arch/subsys_initcall for starting the GPIO as per Tony's suggestion. Also, the driver and device separation and init is totally mixed together and very confusing. The platform_driver is in Agreed. Please see my comment at the end of this patch in reply to your other similar comment. plat-omap/gpio.c and should be doing the driver init: [early_]platform_driver_register() and _probe(). The platform_device setup is in mach-omapX/gpio*.c and where the device init should be, in this case early_platform_add_devices(). In early_gpio_init, omap_device_build() (device specific) and early_platform_driver_register_all() (driver specific), _probe() are done in a sequence. This is because early_gpio_init needs to be called asap and if we try to split this device specific and driver specific early_inits, we will end up having two init calls for each early drivers. I feel that multiple function calls for early_init (one for driver probe and one for device register) should be avoided as we need to find the right place to call them. But your driver probe is already in a separate init path (via arch_initcall) from your device init path omap_gpio_init(). My problem is that there is a 2nd driver init path: board file - [driver]omap_gpio_init() - [device] soc_gpio_init() The fact that it goes through the driver is what I don't like. Okay. Signed-off-by: Charulatha V ch...@ti.com Signed-off-by: Rajendra Nayak rna...@ti.com --- arch/arm/mach-omap1/Makefile |6 + arch/arm/mach-omap1/clock_data.c |2 +- arch/arm/mach-omap2/Makefile |2 +- arch/arm/mach-omap2/clock2420_data.c | 10 +- arch/arm/mach-omap2/clock2430_data.c | 14 +- arch/arm/mach-omap2/clock3xxx_data.c | 24 +- arch/arm/mach-omap2/clock44xx_data.c | 24 +- arch/arm/plat-omap/gpio.c | 405 -- -- arch/arm/plat-omap/include/plat/gpio.h | 21 ++ 9 files changed, 220 insertions(+), 288 deletions(-) [...] @@ -1621,6 +1501,34 @@ static void __init omap_gpio_show_rev(void) */ static struct lock_class_key gpio_lock_class; +static int init_gpio_info(void
Re: [PATCH 9/9] OMAP:GPIO: Implement GPIO as a platform device
* Kevin Hilman khil...@deeprootsystems.com [100505 13:54]: Varadarajan, Charulatha ch...@ti.com writes: -Original Message- From: Kevin Hilman [mailto:khil...@deeprootsystems.com] 2. omap_init_irq() needs to be completed before calling early_gpio_init(). So, if early_gpio_init() is called from omap2_init_common_hw(), we need to have omap_init_irq() called before omap2_init_common_hw(). But Tony objected this approach mentioning that board might not boot up as omap2_init_common_hw() has to be done asap. That's why, I had not moved the omap_gpio_init() usage from board files. OK... for now. I'd still like to see GPIO init consolidated as there's no (good) reason why every board file has to init GPIOs when it's common for all SoCs, but this doesn't necessarily have to be done in your series. Although, if you do it for OMAP1 (as proposed below) you should do similar for OMAP2+. Let's try to use just arch/subsys_initcall for starting the GPIO. AFAIK we don't need it earlier than that. See also my comments to the patch 5/9. Regards, Tony -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 9/9] OMAP:GPIO: Implement GPIO as a platform device
-Original Message-
From: Kevin Hilman [mailto:khil...@deeprootsystems.com]
Sent: Saturday, May 01, 2010 5:34 AM
To: Varadarajan, Charulatha
Cc: linux-omap@vger.kernel.org; Nayak, Rajendra; p...@pwsan.com;
t...@atomide.com
Subject: Re: [PATCH 9/9] OMAP:GPIO: Implement GPIO as a platform device
Charulatha V ch...@ti.com writes:
This patch implements GPIO as a early platform device. Also it
implements OMAP2PLUS specific GPIO as HWMOD FW adapted driver.
Should include a summary explanation of why you're converting to an
early platform device as well.
Okay.
Inorder to convert GPIO as platform device, modifications are
required in clock_data.c files so that device names can be
used to obtain clock instead of getting clocks by name/NULL ptr.
ok
Currently early platform device register does not do device_pm_init.
Hence pm_runtime functions are not used to enable the GPIO device
since gpio is early platform device.
OK for now, since this isn't using runtime PM, but maybe we need a
late_initcall() here to do the device_pm_init() + pm_runtime_enable()
This change log needs more description of the intended init sequence.
Right now it seems that there are multiple init paths. Now that GPIO
is an early_platform_device, we should be able to at least make
omap_gpio_init() static and remove its usage from all the board files.
This was the implementation that I initially did in my previous patch
series. The following needs to be considered:
1. omap2_init_devices() might be too late for early_gpio_init() to
be placed in it.
2. omap_init_irq() needs to be completed before calling early_gpio_init().
So, if early_gpio_init() is called from omap2_init_common_hw(), we need to
have omap_init_irq() called before omap2_init_common_hw(). But Tony
objected this approach mentioning that board might not boot up as
omap2_init_common_hw() has to be done asap.
That's why, I had not moved the omap_gpio_init() usage from board files.
Also, the driver and device separation and init is totally mixed
together and very confusing. The platform_driver is in
Agreed. Please see my comment at the end of this patch in reply to your
other similar comment.
plat-omap/gpio.c and should be doing the driver init:
[early_]platform_driver_register() and _probe(). The platform_device
setup is in mach-omapX/gpio*.c and where the device init should be, in
this case early_platform_add_devices().
In early_gpio_init, omap_device_build() (device specific) and
early_platform_driver_register_all() (driver specific), _probe() are
done in a sequence. This is because early_gpio_init needs to be called
asap and if we try to split this device specific and driver specific
early_inits, we will end up having two init calls for each early drivers.
I feel that multiple function calls for early_init (one for driver probe and
one for device register) should be avoided as we need to find the right place
to call them.
Signed-off-by: Charulatha V ch...@ti.com
Signed-off-by: Rajendra Nayak rna...@ti.com
---
arch/arm/mach-omap1/Makefile |6 +
arch/arm/mach-omap1/clock_data.c |2 +-
arch/arm/mach-omap2/Makefile |2 +-
arch/arm/mach-omap2/clock2420_data.c | 10 +-
arch/arm/mach-omap2/clock2430_data.c | 14 +-
arch/arm/mach-omap2/clock3xxx_data.c | 24 +-
arch/arm/mach-omap2/clock44xx_data.c | 24 +-
arch/arm/plat-omap/gpio.c | 405
arch/arm/plat-omap/include/plat/gpio.h | 21 ++
9 files changed, 220 insertions(+), 288 deletions(-)
[...]
@@ -1621,6 +1501,34 @@ static void __init omap_gpio_show_rev(void)
*/
static struct lock_class_key gpio_lock_class;
+static int init_gpio_info(void)
+{
+ gpio_bank_bits = 32;
+
+ if (cpu_is_omap15xx()) {
+ gpio_bank_count = 2;
+ gpio_bank_bits = 16;
+ } else if (cpu_is_omap16xx()) {
+ gpio_bank_count = 5;
+ gpio_bank_bits = 16;
+ } else if (cpu_is_omap7xx())
+ gpio_bank_count = 7;
+ else if (cpu_is_omap242x())
+ gpio_bank_count = 4;
+ else if (cpu_is_omap243x())
+ gpio_bank_count = 5;
+ else if (cpu_is_omap34xx() || cpu_is_omap44xx())
+ gpio_bank_count = OMAP34XX_NR_GPIOS;
Both the bank count and bank bits could be part of platform_data
and set in the SoC specific init. This is the GPIO driver part
and we're trying to make this as SoC independent as possible. Anytime
you need to add a cpu_is* or #ifdef in this code indicates something
that should be part of SoC specific init and passed in.
bank count and bank bits are not specific to each device, but SoC specific.
Hence I did not consider passing it as part of platform_data, because this
information would be duplicated across all devices in that SoC.
It would be good if we have a way to pass the SoC specific data which is
common for all
Re: [PATCH 9/9] OMAP:GPIO: Implement GPIO as a platform device
Charulatha V ch...@ti.com writes:
This patch implements GPIO as a early platform device. Also it
implements OMAP2PLUS specific GPIO as HWMOD FW adapted driver.
Should include a summary explanation of why you're converting to an
early platform device as well.
Inorder to convert GPIO as platform device, modifications are
required in clock_data.c files so that device names can be
used to obtain clock instead of getting clocks by name/NULL ptr.
ok
Currently early platform device register does not do device_pm_init.
Hence pm_runtime functions are not used to enable the GPIO device
since gpio is early platform device.
OK for now, since this isn't using runtime PM, but maybe we need a
late_initcall() here to do the device_pm_init() + pm_runtime_enable()
This change log needs more description of the intended init sequence.
Right now it seems that there are multiple init paths. Now that GPIO
is an early_platform_device, we should be able to at least make
omap_gpio_init() static and remove its usage from all the board files.
Also, the driver and device separation and init is totally mixed
together and very confusing. The platform_driver is in
plat-omap/gpio.c and should be doing the driver init:
[early_]platform_driver_register() and _probe(). The platform_device
setup is in mach-omapX/gpio*.c and where the device init should be, in
this case early_platform_add_devices().
Signed-off-by: Charulatha V ch...@ti.com
Signed-off-by: Rajendra Nayak rna...@ti.com
---
arch/arm/mach-omap1/Makefile |6 +
arch/arm/mach-omap1/clock_data.c |2 +-
arch/arm/mach-omap2/Makefile |2 +-
arch/arm/mach-omap2/clock2420_data.c | 10 +-
arch/arm/mach-omap2/clock2430_data.c | 14 +-
arch/arm/mach-omap2/clock3xxx_data.c | 24 +-
arch/arm/mach-omap2/clock44xx_data.c | 24 +-
arch/arm/plat-omap/gpio.c | 405
arch/arm/plat-omap/include/plat/gpio.h | 21 ++
9 files changed, 220 insertions(+), 288 deletions(-)
[...]
@@ -1621,6 +1501,34 @@ static void __init omap_gpio_show_rev(void)
*/
static struct lock_class_key gpio_lock_class;
+static int init_gpio_info(void)
+{
+ gpio_bank_bits = 32;
+
+ if (cpu_is_omap15xx()) {
+ gpio_bank_count = 2;
+ gpio_bank_bits = 16;
+ } else if (cpu_is_omap16xx()) {
+ gpio_bank_count = 5;
+ gpio_bank_bits = 16;
+ } else if (cpu_is_omap7xx())
+ gpio_bank_count = 7;
+ else if (cpu_is_omap242x())
+ gpio_bank_count = 4;
+ else if (cpu_is_omap243x())
+ gpio_bank_count = 5;
+ else if (cpu_is_omap34xx() || cpu_is_omap44xx())
+ gpio_bank_count = OMAP34XX_NR_GPIOS;
Both the bank count and bank bits could be part of platform_data
and set in the SoC specific init. This is the GPIO driver part
and we're trying to make this as SoC independent as possible. Anytime
you need to add a cpu_is* or #ifdef in this code indicates something
that should be part of SoC specific init and passed in.
+ gpio_bank = kzalloc(gpio_bank_count * sizeof(struct gpio_bank),
+ GFP_KERNEL);
+ if (!gpio_bank) {
+ pr_err(Memory allocation failed for gpio_bank\n);
+ return -ENOMEM;
+ }
+ return 0;
+}
+
static void omap_gpio_mod_init(struct gpio_bank *bank, int id)
{
if (cpu_class_is_omap2()) {
@@ -1686,16 +1594,9 @@ static void omap_gpio_mod_init(struct gpio_bank *bank,
int id)
static void __init omap_gpio_chip_init(struct gpio_bank *bank)
{
- int j, gpio_bank_bits = 16;
+ int j;
static int gpio;
- if (cpu_is_omap7xx() bank-method == METHOD_GPIO_7XX)
- gpio_bank_bits = 32; /* 7xx has 32-bit GPIOs */
-
- if ((bank-method == METHOD_GPIO_24XX) ||
- (bank-method == METHOD_GPIO_44XX))
- gpio_bank_bits = 32;
-
bank-mod_usage = 0;
/* REVISIT eventually switch from OMAP-specific gpio structs
* over to the generic ones
@@ -1737,140 +1638,103 @@ static void __init omap_gpio_chip_init(struct
gpio_bank *bank)
set_irq_data(bank-irq, bank);
}
-static int __init _omap_gpio_init(void)
+static inline void get_gpio_dbck(struct platform_device *pdev,
+ struct gpio_bank *bank)
{
- int i;
- int gpio = 0;
+ if (cpu_is_omap34xx() || cpu_is_omap44xx()) {
+ bank-dbck = clk_get(pdev-dev, dbck);
+ if (IS_ERR(bank-dbck))
+ pr_err(GPIO: Could not get dbck\n);
+ }
+}
+static int __devinit omap_gpio_probe(struct platform_device *pdev)
+{
+ static int gpio_init_done;
+ struct omap_gpio_platform_data *pdata;
+ int id;
struct gpio_bank *bank;
- int bank_size = SZ_8K; /* Module 4KB + L4 4KB except on omap1 */
- char clk_name[11];
+ struct resource
