[PATCH] omap:pm: Fix boot-time errors with debugfs disabled

[PATCH] omap:pm: Fix boot-time errors with debugfs disabled

[Sanjeev Premi]

There is an implicit assumption in current implementation that
debugfs is always enabled.

When debugfs is disabled, these errors are noticed during boot:
  omap_voltage_late_init: Unable to create voltage debugfs main dir
  vdd_debugfs_init: Unable to create debugfs directory for vdd_mpu
  vdd_debugfs_init: Unable to create debugfs directory for vdd_core

This patch fixes these errors by enclosing code related to debugfs
in #ifdef CONFIG_DEBUG_FS..#endif.

Boot tested on OMAP3EVM.

Signed-off-by: Sanjeev Premi <premi@ti.com>
---
 Patch was created and tested on the pm branch at commit:
   d695569 : rebuild PM from branches

 I am still finding my way around new code sructure. Haven't
 been able to verify if smartreflex and voltage layer are
 properly initialized. Though I do understand that smartreflex
 won't work without debugfs.

 arch/arm/mach-omap2/smartreflex.c |   15 +++++++++++++++
 arch/arm/mach-omap2/voltage.c     |    8 ++++++++
 2 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-omap2/smartreflex.c b/arch/arm/mach-omap2/smartreflex.c
index 2ce2fb7..b757632 100644
--- a/arch/arm/mach-omap2/smartreflex.c
+++ b/arch/arm/mach-omap2/smartreflex.c
@@ -54,7 +54,9 @@ struct omap_sr {
 	struct list_head		node;
 	struct omap_sr_nvalue_table	*nvalue_table;
 	struct voltagedomain		*voltdm;
+#ifdef CONFIG_DEBUG_FS
 	struct dentry			*dbg_dir;
+#endif
 };
 
 /* sr_list contains all the instances of smartreflex module */
@@ -826,7 +828,9 @@ static int __init omap_sr_probe(struct platform_device *pdev)
 	struct omap_sr *sr_info = kzalloc(sizeof(struct omap_sr), GFP_KERNEL);
 	struct omap_sr_data *pdata = pdev->dev.platform_data;
 	struct resource *mem, *irq;
+#ifdef CONFIG_DEBUG_FS
 	struct dentry *vdd_dbg_dir, *nvalue_dir;
+#endif
 	struct omap_volt_data *volt_data;
 	int i, ret = 0;
 
@@ -903,6 +907,7 @@ static int __init omap_sr_probe(struct platform_device *pdev)
 	 * If the voltage domain debugfs directory is not created, do
 	 * not try to create rest of the debugfs entries.
 	 */
+#ifdef CONFIG_DEBUG_FS
 	vdd_dbg_dir = omap_voltage_get_dbgdir(sr_info->voltdm);
 	if (!vdd_dbg_dir) {
 		ret = -EINVAL;
@@ -952,14 +957,22 @@ static int __init omap_sr_probe(struct platform_device *pdev)
 		(void) debugfs_create_x32(name, S_IRUGO | S_IWUSR, nvalue_dir,
 				&(sr_info->nvalue_table[i].nvalue));
 	}
+#else
+	omap_voltage_get_volttable(sr_info->voltdm, &volt_data);
+	if (!volt_data) {
+		ret = -ENODATA;
+	}
+#endif /* CONFIG_DEBUG_FS */
 
 	return ret;
 
+#ifdef CONFIG_DEBUG_FS
 err_debugfs:
 	debugfs_remove_recursive(sr_info->dbg_dir);
 err_iounmap:
 	list_del(&sr_info->node);
 	iounmap(sr_info->base);
+#endif
 err_release_region:
 	release_mem_region(mem->start, resource_size(mem));
 err_free_devinfo:
@@ -988,8 +1001,10 @@ static int __devexit omap_sr_remove(struct platform_device *pdev)
 
 	if (sr_info->autocomp_active)
 		sr_stop_vddautocomp(sr_info);
+#ifdef CONFIG_DEBUG_FS
 	if (sr_info->dbg_dir)
 		debugfs_remove_recursive(sr_info->dbg_dir);
+#endif
 
 	list_del(&sr_info->node);
 	iounmap(sr_info->base);
diff --git a/arch/arm/mach-omap2/voltage.c b/arch/arm/mach-omap2/voltage.c
index 9ef3789..6153211 100644
--- a/arch/arm/mach-omap2/voltage.c
+++ b/arch/arm/mach-omap2/voltage.c
@@ -250,6 +250,7 @@ static void __init vp_init(struct omap_vdd_info *vdd)
 	vdd->write_reg(vp_val, prm_mod_offs, vdd->vp_data->vlimitto);
 }
 
+#ifdef CONFIG_DEBUG_FS
 static void __init vdd_debugfs_init(struct omap_vdd_info *vdd)
 {
 	char *name;
@@ -297,6 +298,7 @@ static void __init vdd_debugfs_init(struct omap_vdd_info *vdd)
 				vdd->debug_dir, (void *) vdd,
 				&nom_volt_debug_fops);
 }
+#endif
 
 /* Voltage scale and accessory APIs */
 static int _pre_volt_scale(struct omap_vdd_info *vdd,
@@ -980,6 +982,7 @@ int omap_voltage_register_pmic(struct voltagedomain *voltdm,
  * add any debug entry for a particular voltage domain. Returns NULL
  * in case of error.
  */
+#ifdef CONFIG_DEBUG_FS
 struct dentry *omap_voltage_get_dbgdir(struct voltagedomain *voltdm)
 {
 	struct omap_vdd_info *vdd;
@@ -993,6 +996,7 @@ struct dentry *omap_voltage_get_dbgdir(struct voltagedomain *voltdm)
 
 	return vdd->debug_dir;
 }
+#endif
 
 /**
  * omap_change_voltscale_method() - API to change the voltage scaling method.
@@ -1078,16 +1082,20 @@ int __init omap_voltage_late_init(void)
 		return -EINVAL;
 	}
 
+#ifdef CONFIG_DEBUG_FS
 	voltage_dir = debugfs_create_dir("voltage", NULL);
 	if (IS_ERR(voltage_dir))
 		pr_err("%s: Unable to create voltage debugfs main dir\n",
 			__func__);
+#endif
 	for (i = 0; i < nr_scalable_vdd; i++) {
 		if (omap_vdd_data_configure(vdd_info[i]))
 			continue;
 		omap_vc_init(vdd_info[i]);
 		vp_init(vdd_info[i]);
+#ifdef CONFIG_DEBUG_FS
 		vdd_debugfs_init(vdd_info[i]);
+#endif
 	}
 
 	return 0;
-- 
1.7.2.2

Re: [PATCH] omap:pm: Fix boot-time errors with debugfs disabled

[Menon, Nishanth]

On Thu, May 12, 2011 at 12:36, Sanjeev Premi <premi@ti.com> wrote:
> There is an implicit assumption in current implementation that
> debugfs is always enabled.
thanks for doing this.
>
> When debugfs is disabled, these errors are noticed during boot:
>  omap_voltage_late_init: Unable to create voltage debugfs main dir
>  vdd_debugfs_init: Unable to create debugfs directory for vdd_mpu
>  vdd_debugfs_init: Unable to create debugfs directory for vdd_core
>
> This patch fixes these errors by enclosing code related to debugfs
> in #ifdef CONFIG_DEBUG_FS..#endif.

generic - couple of comments - I think the #defs should be isolated
off to headers - maybe the right approach may be to move the debugfs
entries off to a separate file? or pm-debug.c??

>
> Boot tested on OMAP3EVM.
>
> Signed-off-by: Sanjeev Premi <premi@ti.com>
> ---
>  Patch was created and tested on the pm branch at commit:
>   d695569 : rebuild PM from branches

Could I suggest Kevin's volt cleanup series - based off _c branch if
there are cleanups to be done?

>
>  I am still finding my way around new code sructure. Haven't
>  been able to verify if smartreflex and voltage layer are
>  properly initialized. Though I do understand that smartreflex
>  won't work without debugfs.
>

Last I tried, with Vishwa's dvfs branch on panda without voltage
registrations, things could crash :( if I get some time I will try to
port Vishwa's series onto kevin's branch as well and test and provide
any patches necessary.


>  arch/arm/mach-omap2/smartreflex.c |   15 +++++++++++++++
>  arch/arm/mach-omap2/voltage.c     |    8 ++++++++
>  2 files changed, 23 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/smartreflex.c b/arch/arm/mach-omap2/smartreflex.c
> index 2ce2fb7..b757632 100644
> --- a/arch/arm/mach-omap2/smartreflex.c
> +++ b/arch/arm/mach-omap2/smartreflex.c
> @@ -54,7 +54,9 @@ struct omap_sr {
>        struct list_head                node;
>        struct omap_sr_nvalue_table     *nvalue_table;
>        struct voltagedomain            *voltdm;
> +#ifdef CONFIG_DEBUG_FS
>        struct dentry                   *dbg_dir;
> +#endif
>  };
>
>  /* sr_list contains all the instances of smartreflex module */
> @@ -826,7 +828,9 @@ static int __init omap_sr_probe(struct platform_device *pdev)
>        struct omap_sr *sr_info = kzalloc(sizeof(struct omap_sr), GFP_KERNEL);
>        struct omap_sr_data *pdata = pdev->dev.platform_data;
>        struct resource *mem, *irq;
> +#ifdef CONFIG_DEBUG_FS
>        struct dentry *vdd_dbg_dir, *nvalue_dir;
> +#endif
>        struct omap_volt_data *volt_data;
>        int i, ret = 0;
>
> @@ -903,6 +907,7 @@ static int __init omap_sr_probe(struct platform_device *pdev)
>         * If the voltage domain debugfs directory is not created, do
>         * not try to create rest of the debugfs entries.
>         */
> +#ifdef CONFIG_DEBUG_FS
>        vdd_dbg_dir = omap_voltage_get_dbgdir(sr_info->voltdm);
>        if (!vdd_dbg_dir) {
>                ret = -EINVAL;
> @@ -952,14 +957,22 @@ static int __init omap_sr_probe(struct platform_device *pdev)
>                (void) debugfs_create_x32(name, S_IRUGO | S_IWUSR, nvalue_dir,
>                                &(sr_info->nvalue_table[i].nvalue));
>        }
> +#else
> +       omap_voltage_get_volttable(sr_info->voltdm, &volt_data);
> +       if (!volt_data) {
> +               ret = -ENODATA;
> +       }
> +#endif /* CONFIG_DEBUG_FS */
>
>        return ret;
>
> +#ifdef CONFIG_DEBUG_FS
>  err_debugfs:
>        debugfs_remove_recursive(sr_info->dbg_dir);
>  err_iounmap:
>        list_del(&sr_info->node);
>        iounmap(sr_info->base);
> +#endif
>  err_release_region:
>        release_mem_region(mem->start, resource_size(mem));
>  err_free_devinfo:
> @@ -988,8 +1001,10 @@ static int __devexit omap_sr_remove(struct platform_device *pdev)
>
>        if (sr_info->autocomp_active)
>                sr_stop_vddautocomp(sr_info);
> +#ifdef CONFIG_DEBUG_FS
>        if (sr_info->dbg_dir)
>                debugfs_remove_recursive(sr_info->dbg_dir);
> +#endif
>
>        list_del(&sr_info->node);
>        iounmap(sr_info->base);
> diff --git a/arch/arm/mach-omap2/voltage.c b/arch/arm/mach-omap2/voltage.c
> index 9ef3789..6153211 100644
> --- a/arch/arm/mach-omap2/voltage.c
> +++ b/arch/arm/mach-omap2/voltage.c
> @@ -250,6 +250,7 @@ static void __init vp_init(struct omap_vdd_info *vdd)
>        vdd->write_reg(vp_val, prm_mod_offs, vdd->vp_data->vlimitto);
>  }
>
> +#ifdef CONFIG_DEBUG_FS
>  static void __init vdd_debugfs_init(struct omap_vdd_info *vdd)
>  {
>        char *name;
> @@ -297,6 +298,7 @@ static void __init vdd_debugfs_init(struct omap_vdd_info *vdd)
>                                vdd->debug_dir, (void *) vdd,
>                                &nom_volt_debug_fops);
>  }
> +#endif
>
>  /* Voltage scale and accessory APIs */
>  static int _pre_volt_scale(struct omap_vdd_info *vdd,
> @@ -980,6 +982,7 @@ int omap_voltage_register_pmic(struct voltagedomain *voltdm,
>  * add any debug entry for a particular voltage domain. Returns NULL
>  * in case of error.
>  */
> +#ifdef CONFIG_DEBUG_FS
>  struct dentry *omap_voltage_get_dbgdir(struct voltagedomain *voltdm)
>  {
>        struct omap_vdd_info *vdd;
> @@ -993,6 +996,7 @@ struct dentry *omap_voltage_get_dbgdir(struct voltagedomain *voltdm)
>
>        return vdd->debug_dir;
>  }
> +#endif
>
>  /**
>  * omap_change_voltscale_method() - API to change the voltage scaling method.
> @@ -1078,16 +1082,20 @@ int __init omap_voltage_late_init(void)
>                return -EINVAL;
>        }
>
> +#ifdef CONFIG_DEBUG_FS
>        voltage_dir = debugfs_create_dir("voltage", NULL);
>        if (IS_ERR(voltage_dir))
>                pr_err("%s: Unable to create voltage debugfs main dir\n",
>                        __func__);
> +#endif
>        for (i = 0; i < nr_scalable_vdd; i++) {
>                if (omap_vdd_data_configure(vdd_info[i]))
>                        continue;
>                omap_vc_init(vdd_info[i]);
>                vp_init(vdd_info[i]);
> +#ifdef CONFIG_DEBUG_FS
>                vdd_debugfs_init(vdd_info[i]);
> +#endif
>        }
>
>        return 0;
> --
> 1.7.2.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>


Regards,
Nishanth Menon
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] omap:pm: Fix boot-time errors with debugfs disabled

[Premi, Sanjeev]

 

> -----Original Message-----
> From: Menon, Nishanth 
> Sent: Thursday, May 12, 2011 11:32 PM
> To: Premi, Sanjeev
> Cc: linux-omap@vger.kernel.org
> Subject: Re: [PATCH] omap:pm: Fix boot-time errors with 
> debugfs disabled
> 
> On Thu, May 12, 2011 at 12:36, Sanjeev Premi <premi@ti.com> wrote:
> > There is an implicit assumption in current implementation that
> > debugfs is always enabled.
> thanks for doing this.
> >
> > When debugfs is disabled, these errors are noticed during boot:
> >  omap_voltage_late_init: Unable to create voltage debugfs main dir
> >  vdd_debugfs_init: Unable to create debugfs directory for vdd_mpu
> >  vdd_debugfs_init: Unable to create debugfs directory for vdd_core
> >
> > This patch fixes these errors by enclosing code related to debugfs
> > in #ifdef CONFIG_DEBUG_FS..#endif.
> 
> generic - couple of comments - I think the #defs should be isolated
> off to headers - maybe the right approach may be to move the debugfs

[sp] None of the code encapsulated here is "header" material.
     I guess you have seen the patch below.

> entries off to a separate file? or pm-debug.c??

[sp] I wish it was all debug code. It is "operational" code.
     Check the Kconfig in plat-omap. Smartreflex is not even expected
     to work without debugfs. Though I believe much of the data put in
     debugfs can be in local lists/structures.

     That was supposed to be my next set of patches - as I understand 
     the implementation better.
> 
> >
> > Boot tested on OMAP3EVM.
> >
> > Signed-off-by: Sanjeev Premi <premi@ti.com>
> > ---
> >  Patch was created and tested on the pm branch at commit:
> >   d695569 : rebuild PM from branches
> 
> Could I suggest Kevin's volt cleanup series - based off _c branch if
> there are cleanups to be done?

[sp] I didn't see it updated for 5 weeks. hence used this.
     but patch should apply cleanly...

> 
> >
> >  I am still finding my way around new code sructure. Haven't
> >  been able to verify if smartreflex and voltage layer are
> >  properly initialized. Though I do understand that smartreflex
> >  won't work without debugfs.
> >
> 
> Last I tried, with Vishwa's dvfs branch on panda without voltage
> registrations, things could crash :( if I get some time I will try to
> port Vishwa's series onto kevin's branch as well and test and provide
> any patches necessary.

[sp] I can try this on Panda. Will be able to get one tomorrow.

~sanjeev

[snip]...[snip]
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] omap:pm: Fix boot-time errors with debugfs disabled

[Todd Poynor]

On Fri, May 13, 2011 at 12:46:17AM +0530, Premi, Sanjeev wrote:
> > -----Original Message-----
> > From: Menon, Nishanth 
...
> > Last I tried, with Vishwa's dvfs branch on panda without voltage
> > registrations, things could crash :( if I get some time I will try to
> > port Vishwa's series onto kevin's branch as well and test and provide
> > any patches necessary.
> 
> [sp] I can try this on Panda. Will be able to get one tomorrow.

In case it helps, below is what I use for Panda.

---
ARM: OMAP2 voltage: Don't die if board does not register voltage params

This allows the DVFS patches to boot on boards not yet modified to
register voltage params.

Signed-off-by: Todd Poynor <toddpoynor@google.com>
---
 arch/arm/mach-omap2/voltage.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-omap2/voltage.c b/arch/arm/mach-omap2/voltage.c
index dbc1cfe..8f23d95 100644
--- a/arch/arm/mach-omap2/voltage.c
+++ b/arch/arm/mach-omap2/voltage.c
@@ -617,6 +617,12 @@ static void __init omap4_vc_init(struct omap_vdd_info *vdd)
 	struct clk *sys_ck;
 	u32 sys_clk_speed;
 
+	if (!vdd->board_data) {
+		pr_warning("%s: No voltage board params registered for vdd_%s\n",
+			   __func__, vdd->voltdm.name);
+		return;
+	}
+
 	sys_ck = clk_get(NULL, "sys_clkin_ck");
 	if (IS_ERR(sys_ck)) {
 		pr_warning("%s: Could not get the sys clk to calculate"
-- 
1.7.3.1

Re: [PATCH] omap:pm: Fix boot-time errors with debugfs disabled

[Nishanth Menon]

On 12:58-20110512, Todd Poynor wrote:
> On Fri, May 13, 2011 at 12:46:17AM +0530, Premi, Sanjeev wrote:
> > > -----Original Message-----
> > > From: Menon, Nishanth 
> ...
> > > Last I tried, with Vishwa's dvfs branch on panda without voltage
> > > registrations, things could crash :( if I get some time I will try to
> > > port Vishwa's series onto kevin's branch as well and test and provide
> > > any patches necessary.
> > 
> > [sp] I can try this on Panda. Will be able to get one tomorrow.
> 
> In case it helps, below is what I use for Panda.
> 
> ---
> ARM: OMAP2 voltage: Don't die if board does not register voltage params
> 
> This allows the DVFS patches to boot on boards not yet modified to
> register voltage params.
> 
> Signed-off-by: Todd Poynor <toddpoynor@google.com>
> ---
>  arch/arm/mach-omap2/voltage.c |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/voltage.c b/arch/arm/mach-omap2/voltage.c
> index dbc1cfe..8f23d95 100644
> --- a/arch/arm/mach-omap2/voltage.c
> +++ b/arch/arm/mach-omap2/voltage.c
> @@ -617,6 +617,12 @@ static void __init omap4_vc_init(struct omap_vdd_info *vdd)
>  	struct clk *sys_ck;
>  	u32 sys_clk_speed;
>  
> +	if (!vdd->board_data) {
> +		pr_warning("%s: No voltage board params registered for vdd_%s\n",
> +			   __func__, vdd->voltdm.name);
> +		return;
> +	}
> +
>  	sys_ck = clk_get(NULL, "sys_clkin_ck");
>  	if (IS_ERR(sys_ck)) {
>  		pr_warning("%s: Could not get the sys clk to calculate"
looks around the right place.. but, which branch is this based on?
cant see board_data on linus master..

-- 
Regards,
Nishanth Menon

RE: [PATCH] omap:pm: Fix boot-time errors with debugfs disabled

[Premi, Sanjeev]

> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org 
> [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of Premi, Sanjeev
> Sent: Friday, May 13, 2011 12:46 AM
> To: Menon, Nishanth
> Cc: linux-omap@vger.kernel.org
> Subject: RE: [PATCH] omap:pm: Fix boot-time errors with 
> debugfs disabled
> 

[sp]

> 
> > Last I tried, with Vishwa's dvfs branch on panda without voltage
> > registrations, things could crash :( if I get some time I 
> will try to
> > port Vishwa's series onto kevin's branch as well and test 
> and provide
> > any patches necessary.
> 
> [sp] I can try this on Panda. Will be able to get one tomorrow.


Tested same uImage on Panda as well. Just pasting the top header
here...

[    0.000000] Linux version 2.6.39-rc4-13782-gd695569-dirty (premi@mylinux) (gcc version 4.5.1 (Sourcery G++ Lite 2010.09-50) ) #6 SMP Thu May 12 21:40:58 IST 2011
[    0.000000] CPU: ARMv7 Processor [411fc092] revision 2 (ARMv7), cr=10c53c7f
[    0.000000] CPU: VIPT nonaliasing data cache, VIPT aliasing instruction cache
[    0.000000] Machine: OMAP4 Panda board
[    0.000000] Memory policy: ECC disabled, Data cache writealloc
[    0.000000] Truncating RAM at 80000000-bfffffff to -afffffff (vmalloc region overlap).
[    0.000000] OMAP4430 ES2.1

> 
> ~sanjeev
> 
> [snip]...[snip]
> --
> To unsubscribe from this list: send the line "unsubscribe 
> linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

RE: [PATCH] omap:pm: Fix boot-time errors with debugfs disabled

[Premi, Sanjeev]

> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org 
> [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of Premi, Sanjeev
> Sent: Friday, May 13, 2011 6:18 PM
> To: linux-omap@vger.kernel.org
> Cc: Menon, Nishanth
> Subject: RE: [PATCH] omap:pm: Fix boot-time errors with 
> debugfs disabled

[snip]...[snip]

> > 
> > [sp] I can try this on Panda. Will be able to get one tomorrow.
> 
> 
> Tested same uImage on Panda as well. Just pasting the top header
> here...
> 
> [    0.000000] Linux version 2.6.39-rc4-13782-gd695569-dirty 
> (premi@mylinux) (gcc version 4.5.1 (Sourcery G++ Lite 
> 2010.09-50) ) #6 SMP Thu May 12 21:40:58 IST 2011
> [    0.000000] CPU: ARMv7 Processor [411fc092] revision 2 
> (ARMv7), cr=10c53c7f
> [    0.000000] CPU: VIPT nonaliasing data cache, VIPT 
> aliasing instruction cache
> [    0.000000] Machine: OMAP4 Panda board
> [    0.000000] Memory policy: ECC disabled, Data cache writealloc
> [    0.000000] Truncating RAM at 80000000-bfffffff to 
> -afffffff (vmalloc region overlap).
> [    0.000000] OMAP4430 ES2.1
> 

Kevin,

Wanted to check if you have look at this patch?

~sanjeev

[snip]...[snip]

Re: [PATCH] omap:pm: Fix boot-time errors with debugfs disabled

[Menon, Nishanth]

On Thu, May 12, 2011 at 14:16, Premi, Sanjeev <premi@ti.com> wrote:
[...]
>> > This patch fixes these errors by enclosing code related to debugfs
>> > in #ifdef CONFIG_DEBUG_FS..#endif.
>>
>> generic - couple of comments - I think the #defs should be isolated
>> off to headers - maybe the right approach may be to move the debugfs
>
> [sp] None of the code encapsulated here is "header" material.
>     I guess you have seen the patch below.
>
>> entries off to a separate file? or pm-debug.c??
>
> [sp] I wish it was all debug code. It is "operational" code.
>     Check the Kconfig in plat-omap. Smartreflex is not even expected
>     to work without debugfs. Though I believe much of the data put in
>     debugfs can be in local lists/structures.
>
>     That was supposed to be my next set of patches - as I understand
>     the implementation better.
[...]
>> Could I suggest Kevin's volt cleanup series - based off _c branch if
>> there are cleanups to be done?
>
> [sp] I didn't see it updated for 5 weeks. hence used this.
>     but patch should apply cleanly...


While cleaning up voltdm_c set earlier this week, I think your changes
apply better there.
btw, I could incorporate a bit of your code into my patch, esp the one
Tony commented on http://marc.info/?l=linux-omap&m=130570559515977&w=2
but, overall, on the topic of SR, either:
a)  move SR autocomp into sysfs (and dump the rest of the debugfs - it
is useful for validation, but does'nt really provide additional info)
- given that it used to reside in /sys/power/sr_vddx_autocomp and then
moved to debugfs, I am not sure if this is the right path
b) move SR autocomp into a board defined configuration.. more
intrusive, but folks would really want to enable SR as an option at
times from userspace - many distros and devices do this (e.g. N900)..

Regards,
Nishanth Menon
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] omap:pm: Fix boot-time errors with debugfs disabled

[Premi, Sanjeev]

> -----Original Message-----
> From: Menon, Nishanth 
> Sent: Wednesday, May 18, 2011 1:55 PM
> To: Premi, Sanjeev
> Cc: linux-omap@vger.kernel.org
> Subject: Re: [PATCH] omap:pm: Fix boot-time errors with 
> debugfs disabled
> 
> On Thu, May 12, 2011 at 14:16, Premi, Sanjeev <premi@ti.com> wrote:
> [...]
> >> > This patch fixes these errors by enclosing code related 
> to debugfs
> >> > in #ifdef CONFIG_DEBUG_FS..#endif.
> >>
> >> generic - couple of comments - I think the #defs should be isolated
> >> off to headers - maybe the right approach may be to move 
> the debugfs
> >
> > [sp] None of the code encapsulated here is "header" material.
> >     I guess you have seen the patch below.
> >
> >> entries off to a separate file? or pm-debug.c??
> >
> > [sp] I wish it was all debug code. It is "operational" code.
> >     Check the Kconfig in plat-omap. Smartreflex is not even expected
> >     to work without debugfs. Though I believe much of the 
> data put in
> >     debugfs can be in local lists/structures.
> >
> >     That was supposed to be my next set of patches - as I understand
> >     the implementation better.
> [...]
> >> Could I suggest Kevin's volt cleanup series - based off _c 
> branch if
> >> there are cleanups to be done?
> >
> > [sp] I didn't see it updated for 5 weeks. hence used this.
> >     but patch should apply cleanly...
> 
> 
> While cleaning up voltdm_c set earlier this week, I think your changes
> apply better there.
> btw, I could incorporate a bit of your code into my patch, esp the one
> Tony commented on http://marc.info/?l=linux-omap&m=130570559515977&w=2
> but, overall, on the topic of SR, either:
> a)  move SR autocomp into sysfs (and dump the rest of the debugfs - it
> is useful for validation, but does'nt really provide additional info)
> - given that it used to reside in /sys/power/sr_vddx_autocomp and then
> moved to debugfs, I am not sure if this is the right path
> b) move SR autocomp into a board defined configuration.. more
> intrusive, but folks would really want to enable SR as an option at
> times from userspace - many distros and devices do this (e.g. N900)..

[sp] I am not sure what you are suggesting. Can't we take in the patch and
     then do the movements? ... the ones that doesn't seem to be implemented
     so far (based on your comments).

~sanjeev

> 
> Regards,
> Nishanth Menon
> --
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] omap:pm: Fix boot-time errors with debugfs disabled

[Menon, Nishanth]

On Wed, May 18, 2011 at 04:00, Premi, Sanjeev <premi@ti.com> wrote:
>> While cleaning up voltdm_c set earlier this week, I think your changes
>> apply better there.
>> btw, I could incorporate a bit of your code into my patch, esp the one
>> Tony commented on http://marc.info/?l=linux-omap&m=130570559515977&w=2
ok this part got rejected pending regulator framework for scalable
voltage domains..

>> but, overall, on the topic of SR, either:
>> a)  move SR autocomp into sysfs (and dump the rest of the debugfs - it
>> is useful for validation, but does'nt really provide additional info)
>> - given that it used to reside in /sys/power/sr_vddx_autocomp and then
>> moved to debugfs, I am not sure if this is the right path
>> b) move SR autocomp into a board defined configuration.. more
>> intrusive, but folks would really want to enable SR as an option at
>> times from userspace - many distros and devices do this (e.g. N900)..
>
> [sp] I am not sure what you are suggesting. Can't we take in the patch and
>     then do the movements? ... the ones that doesn't seem to be implemented
>     so far (based on your comments).
depends on what Kevin thinks is the future of voltdm(in terms of which
.4x target) - might be good to focus our attention into a single
branch and cleanit up for upstream.. I am personally not sure what
should autocomp's future should be - I think option of having boards
be able to define it - maybe as part of regulator framework + debugfs
cleanup similar(as voltdm_c has removed I believe all of voltage
debugfs - so your patch can be much smaller and effective there)..

Regards,
Nishanth Menon
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] omap:pm: Fix boot-time errors with debugfs disabled

[Premi, Sanjeev]

> -----Original Message-----
> From: Menon, Nishanth 
> Sent: Wednesday, May 18, 2011 2:36 PM
> To: Premi, Sanjeev
> Cc: linux-omap@vger.kernel.org
> Subject: Re: [PATCH] omap:pm: Fix boot-time errors with 
> debugfs disabled
> 
> On Wed, May 18, 2011 at 04:00, Premi, Sanjeev <premi@ti.com> wrote:
> >> While cleaning up voltdm_c set earlier this week, I think 
> your changes
> >> apply better there.
> >> btw, I could incorporate a bit of your code into my patch, 
> esp the one
> >> Tony commented on 
> http://marc.info/?l=linux-omap&m=130570559515977&w=2
> ok this part got rejected pending regulator framework for scalable
> voltage domains..
> 
> >> but, overall, on the topic of SR, either:
> >> a)  move SR autocomp into sysfs (and dump the rest of the 
> debugfs - it
> >> is useful for validation, but does'nt really provide 
> additional info)
> >> - given that it used to reside in 
> /sys/power/sr_vddx_autocomp and then
> >> moved to debugfs, I am not sure if this is the right path
> >> b) move SR autocomp into a board defined configuration.. more
> >> intrusive, but folks would really want to enable SR as an option at
> >> times from userspace - many distros and devices do this 
> (e.g. N900)..
> >
> > [sp] I am not sure what you are suggesting. Can't we take 
> in the patch and
> >     then do the movements? ... the ones that doesn't seem 
> to be implemented
> >     so far (based on your comments).
> depends on what Kevin thinks is the future of voltdm(in terms of which
> .4x target) - might be good to focus our attention into a single
> branch and cleanit up for upstream.. I am personally not sure what
> should autocomp's future should be - I think option of having boards
> be able to define it - maybe as part of regulator framework + debugfs
> cleanup similar(as voltdm_c has removed I believe all of voltage
> debugfs - so your patch can be much smaller and effective there)..

[sp] Reworking the patch for different branch is fine.
     But what about the pm branch today.

> 
> Regards,
> Nishanth Menon
> --
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] omap:pm: Fix boot-time errors with debugfs disabled

[Kevin Hilman]

Hi Sanjeev,

Sanjeev Premi <premi@ti.com> writes:

> There is an implicit assumption in current implementation that
> debugfs is always enabled.
>
> When debugfs is disabled, these errors are noticed during boot:
>   omap_voltage_late_init: Unable to create voltage debugfs main dir
>   vdd_debugfs_init: Unable to create debugfs directory for vdd_mpu
>   vdd_debugfs_init: Unable to create debugfs directory for vdd_core
>
> This patch fixes these errors by enclosing code related to debugfs
> in #ifdef CONFIG_DEBUG_FS..#endif.
>
> Boot tested on OMAP3EVM.
>
> Signed-off-by: Sanjeev Premi <premi@ti.com>

Sorry for the delay here, I've been on the road and finally catching up
on the list.

Basically, I'm in the process of a pretty major cleanup of the VC/VP and
SR layers.  For example, in my pm-wip/voltdm_* branches, the debugfs
interface to the voltage layer has been completely removed.  I'm also
thinking of removing the SR debugfs interface also, as I don't really
think we need a userspace interface for this.  A board-level interface
is probably enough (/me waits for flame from Nishanth :)

That being said, your approach below isn't quite right in its usage of
#ifdefs.  Use of #ifdefs like this is frowned upon for many reasons.
For starters, take a look the '#ifdefs are ugly' section of
Documentation/SubmittingPatches.

Kevin

> ---
>  Patch was created and tested on the pm branch at commit:
>    d695569 : rebuild PM from branches
>
>  I am still finding my way around new code sructure. Haven't
>  been able to verify if smartreflex and voltage layer are
>  properly initialized. Though I do understand that smartreflex
>  won't work without debugfs.
>
>  arch/arm/mach-omap2/smartreflex.c |   15 +++++++++++++++
>  arch/arm/mach-omap2/voltage.c     |    8 ++++++++
>  2 files changed, 23 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/smartreflex.c b/arch/arm/mach-omap2/smartreflex.c
> index 2ce2fb7..b757632 100644
> --- a/arch/arm/mach-omap2/smartreflex.c
> +++ b/arch/arm/mach-omap2/smartreflex.c
> @@ -54,7 +54,9 @@ struct omap_sr {
>  	struct list_head		node;
>  	struct omap_sr_nvalue_table	*nvalue_table;
>  	struct voltagedomain		*voltdm;
> +#ifdef CONFIG_DEBUG_FS
>  	struct dentry			*dbg_dir;
> +#endif
>  };
>  
>  /* sr_list contains all the instances of smartreflex module */
> @@ -826,7 +828,9 @@ static int __init omap_sr_probe(struct platform_device *pdev)
>  	struct omap_sr *sr_info = kzalloc(sizeof(struct omap_sr), GFP_KERNEL);
>  	struct omap_sr_data *pdata = pdev->dev.platform_data;
>  	struct resource *mem, *irq;
> +#ifdef CONFIG_DEBUG_FS
>  	struct dentry *vdd_dbg_dir, *nvalue_dir;
> +#endif
>  	struct omap_volt_data *volt_data;
>  	int i, ret = 0;
>  
> @@ -903,6 +907,7 @@ static int __init omap_sr_probe(struct platform_device *pdev)
>  	 * If the voltage domain debugfs directory is not created, do
>  	 * not try to create rest of the debugfs entries.
>  	 */
> +#ifdef CONFIG_DEBUG_FS
>  	vdd_dbg_dir = omap_voltage_get_dbgdir(sr_info->voltdm);
>  	if (!vdd_dbg_dir) {
>  		ret = -EINVAL;
> @@ -952,14 +957,22 @@ static int __init omap_sr_probe(struct platform_device *pdev)
>  		(void) debugfs_create_x32(name, S_IRUGO | S_IWUSR, nvalue_dir,
>  				&(sr_info->nvalue_table[i].nvalue));
>  	}
> +#else
> +	omap_voltage_get_volttable(sr_info->voltdm, &volt_data);
> +	if (!volt_data) {
> +		ret = -ENODATA;
> +	}
> +#endif /* CONFIG_DEBUG_FS */
>  
>  	return ret;
>  
> +#ifdef CONFIG_DEBUG_FS
>  err_debugfs:
>  	debugfs_remove_recursive(sr_info->dbg_dir);
>  err_iounmap:
>  	list_del(&sr_info->node);
>  	iounmap(sr_info->base);
> +#endif
>  err_release_region:
>  	release_mem_region(mem->start, resource_size(mem));
>  err_free_devinfo:
> @@ -988,8 +1001,10 @@ static int __devexit omap_sr_remove(struct platform_device *pdev)
>  
>  	if (sr_info->autocomp_active)
>  		sr_stop_vddautocomp(sr_info);
> +#ifdef CONFIG_DEBUG_FS
>  	if (sr_info->dbg_dir)
>  		debugfs_remove_recursive(sr_info->dbg_dir);
> +#endif
>  
>  	list_del(&sr_info->node);
>  	iounmap(sr_info->base);
> diff --git a/arch/arm/mach-omap2/voltage.c b/arch/arm/mach-omap2/voltage.c
> index 9ef3789..6153211 100644
> --- a/arch/arm/mach-omap2/voltage.c
> +++ b/arch/arm/mach-omap2/voltage.c
> @@ -250,6 +250,7 @@ static void __init vp_init(struct omap_vdd_info *vdd)
>  	vdd->write_reg(vp_val, prm_mod_offs, vdd->vp_data->vlimitto);
>  }
>  
> +#ifdef CONFIG_DEBUG_FS
>  static void __init vdd_debugfs_init(struct omap_vdd_info *vdd)
>  {
>  	char *name;
> @@ -297,6 +298,7 @@ static void __init vdd_debugfs_init(struct omap_vdd_info *vdd)
>  				vdd->debug_dir, (void *) vdd,
>  				&nom_volt_debug_fops);
>  }
> +#endif
>  
>  /* Voltage scale and accessory APIs */
>  static int _pre_volt_scale(struct omap_vdd_info *vdd,
> @@ -980,6 +982,7 @@ int omap_voltage_register_pmic(struct voltagedomain *voltdm,
>   * add any debug entry for a particular voltage domain. Returns NULL
>   * in case of error.
>   */
> +#ifdef CONFIG_DEBUG_FS
>  struct dentry *omap_voltage_get_dbgdir(struct voltagedomain *voltdm)
>  {
>  	struct omap_vdd_info *vdd;
> @@ -993,6 +996,7 @@ struct dentry *omap_voltage_get_dbgdir(struct voltagedomain *voltdm)
>  
>  	return vdd->debug_dir;
>  }
> +#endif
>  
>  /**
>   * omap_change_voltscale_method() - API to change the voltage scaling method.
> @@ -1078,16 +1082,20 @@ int __init omap_voltage_late_init(void)
>  		return -EINVAL;
>  	}
>  
> +#ifdef CONFIG_DEBUG_FS
>  	voltage_dir = debugfs_create_dir("voltage", NULL);
>  	if (IS_ERR(voltage_dir))
>  		pr_err("%s: Unable to create voltage debugfs main dir\n",
>  			__func__);
> +#endif
>  	for (i = 0; i < nr_scalable_vdd; i++) {
>  		if (omap_vdd_data_configure(vdd_info[i]))
>  			continue;
>  		omap_vc_init(vdd_info[i]);
>  		vp_init(vdd_info[i]);
> +#ifdef CONFIG_DEBUG_FS
>  		vdd_debugfs_init(vdd_info[i]);
> +#endif
>  	}
>  
>  	return 0;

Re: [PATCH] omap:pm: Fix boot-time errors with debugfs disabled

[Menon, Nishanth]

On Wed, May 18, 2011 at 11:34, Kevin Hilman <khilman@ti.com> wrote:
> Basically, I'm in the process of a pretty major cleanup of the VC/VP and
> SR layers.  For example, in my pm-wip/voltdm_* branches, the debugfs
> interface to the voltage layer has been completely removed.  I'm also
> thinking of removing the SR debugfs interface also, as I don't really
> think we need a userspace interface for this.  A board-level interface
> is probably enough (/me waits for flame from Nishanth :)
/me obliges with my 2cents ;) :

on board level interface - yes - we should have it.

on plugging out userspace:  This is too risky an approach - having a
mechanism to control SR from userspace is critical - yes folks can do
a regwrite over /dev/mem to plug it out, but with multiple SR classes,
it hardly ever going to be right. further, SR is a touchy beast to bad
clk and system configurations - if we view debugfs as what it was
meant to be: debug interface, SR deserves it. I agree that asking for
userspace to have a sysfs is not worth it if board level interface has
it. (and adding custom patches for every board developer out there...
well.. it is not really worth the pain).

Regards,
Nishanth Menon
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] omap:pm: Fix boot-time errors with debugfs disabled

[Premi, Sanjeev]

> -----Original Message-----
> From: Hilman, Kevin 
> Sent: Wednesday, May 18, 2011 10:04 PM
> To: Premi, Sanjeev
> Cc: linux-omap@vger.kernel.org
> Subject: Re: [PATCH] omap:pm: Fix boot-time errors with 
> debugfs disabled
> 
> Hi Sanjeev,
> 
> Sanjeev Premi <premi@ti.com> writes:
> 
> > There is an implicit assumption in current implementation that
> > debugfs is always enabled.
> >
> > When debugfs is disabled, these errors are noticed during boot:
> >   omap_voltage_late_init: Unable to create voltage debugfs main dir
> >   vdd_debugfs_init: Unable to create debugfs directory for vdd_mpu
> >   vdd_debugfs_init: Unable to create debugfs directory for vdd_core
> >
> > This patch fixes these errors by enclosing code related to debugfs
> > in #ifdef CONFIG_DEBUG_FS..#endif.
> >
> > Boot tested on OMAP3EVM.
> >
> > Signed-off-by: Sanjeev Premi <premi@ti.com>
> 
> Sorry for the delay here, I've been on the road and finally 
> catching up
> on the list.
> 
> Basically, I'm in the process of a pretty major cleanup of 
> the VC/VP and
> SR layers.  For example, in my pm-wip/voltdm_* branches, the debugfs
> interface to the voltage layer has been completely removed.  I'm also
> thinking of removing the SR debugfs interface also, as I don't really
> think we need a userspace interface for this.  A board-level interface
> is probably enough (/me waits for flame from Nishanth :)
> 
> That being said, your approach below isn't quite right in its usage of
> #ifdefs.  Use of #ifdefs like this is frowned upon for many reasons.
> For starters, take a look the '#ifdefs are ugly' section of
> Documentation/SubmittingPatches.

[sp] I don't like #ifdefs either but each time we cannot create
     a new file changes like this.

     The current code is a mess with debugfs used too frequently.
     And - all of it is not for debug. The location of ifdefs in
     in the patch illustrates it quite well.

     BTW, this isn't the only use of ifdefs in a C file in Linux.

~sanjeev

> 
> Kevin
> 
> > ---
> >  Patch was created and tested on the pm branch at commit:
> >    d695569 : rebuild PM from branches
> >
> >  I am still finding my way around new code sructure. Haven't
> >  been able to verify if smartreflex and voltage layer are
> >  properly initialized. Though I do understand that smartreflex
> >  won't work without debugfs.
> >
> >  arch/arm/mach-omap2/smartreflex.c |   15 +++++++++++++++
> >  arch/arm/mach-omap2/voltage.c     |    8 ++++++++
> >  2 files changed, 23 insertions(+), 0 deletions(-)
> >
> > diff --git a/arch/arm/mach-omap2/smartreflex.c 
> b/arch/arm/mach-omap2/smartreflex.c
> > index 2ce2fb7..b757632 100644
> > --- a/arch/arm/mach-omap2/smartreflex.c
> > +++ b/arch/arm/mach-omap2/smartreflex.c
> > @@ -54,7 +54,9 @@ struct omap_sr {
> >  	struct list_head		node;
> >  	struct omap_sr_nvalue_table	*nvalue_table;
> >  	struct voltagedomain		*voltdm;
> > +#ifdef CONFIG_DEBUG_FS
> >  	struct dentry			*dbg_dir;
> > +#endif
> >  };
> >  
> >  /* sr_list contains all the instances of smartreflex module */
> > @@ -826,7 +828,9 @@ static int __init omap_sr_probe(struct 
> platform_device *pdev)
> >  	struct omap_sr *sr_info = kzalloc(sizeof(struct 
> omap_sr), GFP_KERNEL);
> >  	struct omap_sr_data *pdata = pdev->dev.platform_data;
> >  	struct resource *mem, *irq;
> > +#ifdef CONFIG_DEBUG_FS
> >  	struct dentry *vdd_dbg_dir, *nvalue_dir;
> > +#endif
> >  	struct omap_volt_data *volt_data;
> >  	int i, ret = 0;
> >  
> > @@ -903,6 +907,7 @@ static int __init omap_sr_probe(struct 
> platform_device *pdev)
> >  	 * If the voltage domain debugfs directory is not created, do
> >  	 * not try to create rest of the debugfs entries.
> >  	 */
> > +#ifdef CONFIG_DEBUG_FS
> >  	vdd_dbg_dir = omap_voltage_get_dbgdir(sr_info->voltdm);
> >  	if (!vdd_dbg_dir) {
> >  		ret = -EINVAL;
> > @@ -952,14 +957,22 @@ static int __init 
> omap_sr_probe(struct platform_device *pdev)
> >  		(void) debugfs_create_x32(name, S_IRUGO | 
> S_IWUSR, nvalue_dir,
> >  				&(sr_info->nvalue_table[i].nvalue));
> >  	}
> > +#else
> > +	omap_voltage_get_volttable(sr_info->voltdm, &volt_data);
> > +	if (!volt_data) {
> > +		ret = -ENODATA;
> > +	}
> > +#endif /* CONFIG_DEBUG_FS */
> >  
> >  	return ret;
> >  
> > +#ifdef CONFIG_DEBUG_FS
> >  err_debugfs:
> >  	debugfs_remove_recursive(sr_info->dbg_dir);
> >  err_iounmap:
> >  	list_del(&sr_info->node);
> >  	iounmap(sr_info->base);
> > +#endif
> >  err_release_region:
> >  	release_mem_region(mem->start, resource_size(mem));
> >  err_free_devinfo:
> > @@ -988,8 +1001,10 @@ static int __devexit 
> omap_sr_remove(struct platform_device *pdev)
> >  
> >  	if (sr_info->autocomp_active)
> >  		sr_stop_vddautocomp(sr_info);
> > +#ifdef CONFIG_DEBUG_FS
> >  	if (sr_info->dbg_dir)
> >  		debugfs_remove_recursive(sr_info->dbg_dir);
> > +#endif
> >  
> >  	list_del(&sr_info->node);
> >  	iounmap(sr_info->base);
> > diff --git a/arch/arm/mach-omap2/voltage.c 
> b/arch/arm/mach-omap2/voltage.c
> > index 9ef3789..6153211 100644
> > --- a/arch/arm/mach-omap2/voltage.c
> > +++ b/arch/arm/mach-omap2/voltage.c
> > @@ -250,6 +250,7 @@ static void __init vp_init(struct 
> omap_vdd_info *vdd)
> >  	vdd->write_reg(vp_val, prm_mod_offs, vdd->vp_data->vlimitto);
> >  }
> >  
> > +#ifdef CONFIG_DEBUG_FS
> >  static void __init vdd_debugfs_init(struct omap_vdd_info *vdd)
> >  {
> >  	char *name;
> > @@ -297,6 +298,7 @@ static void __init 
> vdd_debugfs_init(struct omap_vdd_info *vdd)
> >  				vdd->debug_dir, (void *) vdd,
> >  				&nom_volt_debug_fops);
> >  }
> > +#endif
> >  
> >  /* Voltage scale and accessory APIs */
> >  static int _pre_volt_scale(struct omap_vdd_info *vdd,
> > @@ -980,6 +982,7 @@ int omap_voltage_register_pmic(struct 
> voltagedomain *voltdm,
> >   * add any debug entry for a particular voltage domain. 
> Returns NULL
> >   * in case of error.
> >   */
> > +#ifdef CONFIG_DEBUG_FS
> >  struct dentry *omap_voltage_get_dbgdir(struct 
> voltagedomain *voltdm)
> >  {
> >  	struct omap_vdd_info *vdd;
> > @@ -993,6 +996,7 @@ struct dentry 
> *omap_voltage_get_dbgdir(struct voltagedomain *voltdm)
> >  
> >  	return vdd->debug_dir;
> >  }
> > +#endif
> >  
> >  /**
> >   * omap_change_voltscale_method() - API to change the 
> voltage scaling method.
> > @@ -1078,16 +1082,20 @@ int __init omap_voltage_late_init(void)
> >  		return -EINVAL;
> >  	}
> >  
> > +#ifdef CONFIG_DEBUG_FS
> >  	voltage_dir = debugfs_create_dir("voltage", NULL);
> >  	if (IS_ERR(voltage_dir))
> >  		pr_err("%s: Unable to create voltage debugfs 
> main dir\n",
> >  			__func__);
> > +#endif
> >  	for (i = 0; i < nr_scalable_vdd; i++) {
> >  		if (omap_vdd_data_configure(vdd_info[i]))
> >  			continue;
> >  		omap_vc_init(vdd_info[i]);
> >  		vp_init(vdd_info[i]);
> > +#ifdef CONFIG_DEBUG_FS
> >  		vdd_debugfs_init(vdd_info[i]);
> > +#endif
> >  	}
> >  
> >  	return 0;
> 

Re: [PATCH] omap:pm: Fix boot-time errors with debugfs disabled

[Menon, Nishanth]

On Thu, May 19, 2011 at 05:30, Premi, Sanjeev <premi@ti.com> wrote:
>> -----Original Message-----
>> From: Hilman, Kevin
>> Sent: Wednesday, May 18, 2011 10:04 PM
>> To: Premi, Sanjeev
>> Cc: linux-omap@vger.kernel.org
>> Subject: Re: [PATCH] omap:pm: Fix boot-time errors with
>> debugfs disabled
>>
>> Hi Sanjeev,
>>
>> Sanjeev Premi <premi@ti.com> writes:
>>
>> > There is an implicit assumption in current implementation that
>> > debugfs is always enabled.
>> >
>> > When debugfs is disabled, these errors are noticed during boot:
>> >   omap_voltage_late_init: Unable to create voltage debugfs main dir
>> >   vdd_debugfs_init: Unable to create debugfs directory for vdd_mpu
>> >   vdd_debugfs_init: Unable to create debugfs directory for vdd_core
>> >
>> > This patch fixes these errors by enclosing code related to debugfs
>> > in #ifdef CONFIG_DEBUG_FS..#endif.
>> >
>> > Boot tested on OMAP3EVM.
>> >
>> > Signed-off-by: Sanjeev Premi <premi@ti.com>
>>
>> Sorry for the delay here, I've been on the road and finally
>> catching up
>> on the list.
>>
>> Basically, I'm in the process of a pretty major cleanup of
>> the VC/VP and
>> SR layers.  For example, in my pm-wip/voltdm_* branches, the debugfs
>> interface to the voltage layer has been completely removed.  I'm also
>> thinking of removing the SR debugfs interface also, as I don't really
>> think we need a userspace interface for this.  A board-level interface
>> is probably enough (/me waits for flame from Nishanth :)
>>
>> That being said, your approach below isn't quite right in its usage of
>> #ifdefs.  Use of #ifdefs like this is frowned upon for many reasons.
>> For starters, take a look the '#ifdefs are ugly' section of
>> Documentation/SubmittingPatches.
>
> [sp] I don't like #ifdefs either but each time we cannot create
>     a new file changes like this.
>
>     The current code is a mess with debugfs used too frequently.
>     And - all of it is not for debug. The location of ifdefs in
>     in the patch illustrates it quite well.
>
>     BTW, this isn't the only use of ifdefs in a C file in Linux.
in reality the only reason you've had to do this patch was because we
had a wicked handling of debugfs entries in voltage layer - with
voltdm_c these are all gone. further any entries remaining (e.g. SR)
are:
dentry for debugfs file -> just a minor overhead not deserving a #ifdef
all other functions of debugfs (as per include/linux/debugfs.h) when
debugfs is disabled in .config will be static inlined and we will not
need any #ifdefs at all

The real pending question is about functional SR debugfs entries that
need to loose it's critical functionality.
Regards,
Nishanth Menon
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] omap:pm: Fix boot-time errors with debugfs disabled

[Premi, Sanjeev]

> -----Original Message-----
> From: Menon, Nishanth 
> Sent: Thursday, May 19, 2011 7:28 PM
> To: Premi, Sanjeev
> Cc: Hilman, Kevin; linux-omap@vger.kernel.org
> Subject: Re: [PATCH] omap:pm: Fix boot-time errors with 
> debugfs disabled
> 
> On Thu, May 19, 2011 at 05:30, Premi, Sanjeev <premi@ti.com> wrote:
> >> -----Original Message-----
> >> From: Hilman, Kevin
> >> Sent: Wednesday, May 18, 2011 10:04 PM
> >> To: Premi, Sanjeev
> >> Cc: linux-omap@vger.kernel.org
> >> Subject: Re: [PATCH] omap:pm: Fix boot-time errors with
> >> debugfs disabled
> >>
> >> Hi Sanjeev,
> >>
> >> Sanjeev Premi <premi@ti.com> writes:
> >>
> >> > There is an implicit assumption in current implementation that
> >> > debugfs is always enabled.
> >> >
> >> > When debugfs is disabled, these errors are noticed during boot:
> >> >   omap_voltage_late_init: Unable to create voltage 
> debugfs main dir
> >> >   vdd_debugfs_init: Unable to create debugfs directory 
> for vdd_mpu
> >> >   vdd_debugfs_init: Unable to create debugfs directory 
> for vdd_core
> >> >
> >> > This patch fixes these errors by enclosing code related 
> to debugfs
> >> > in #ifdef CONFIG_DEBUG_FS..#endif.
> >> >
> >> > Boot tested on OMAP3EVM.
> >> >
> >> > Signed-off-by: Sanjeev Premi <premi@ti.com>
> >>
> >> Sorry for the delay here, I've been on the road and finally
> >> catching up
> >> on the list.
> >>
> >> Basically, I'm in the process of a pretty major cleanup of
> >> the VC/VP and
> >> SR layers.  For example, in my pm-wip/voltdm_* branches, 
> the debugfs
> >> interface to the voltage layer has been completely 
> removed.  I'm also
> >> thinking of removing the SR debugfs interface also, as I 
> don't really
> >> think we need a userspace interface for this.  A 
> board-level interface
> >> is probably enough (/me waits for flame from Nishanth :)
> >>
> >> That being said, your approach below isn't quite right in 
> its usage of
> >> #ifdefs.  Use of #ifdefs like this is frowned upon for 
> many reasons.
> >> For starters, take a look the '#ifdefs are ugly' section of
> >> Documentation/SubmittingPatches.
> >
> > [sp] I don't like #ifdefs either but each time we cannot create
> >     a new file changes like this.
> >
> >     The current code is a mess with debugfs used too frequently.
> >     And - all of it is not for debug. The location of ifdefs in
> >     in the patch illustrates it quite well.
> >
> >     BTW, this isn't the only use of ifdefs in a C file in Linux.
> in reality the only reason you've had to do this patch was because we
> had a wicked handling of debugfs entries in voltage layer - with
> voltdm_c these are all gone. further any entries remaining (e.g. SR)
> are:
> dentry for debugfs file -> just a minor overhead not 
> deserving a #ifdef
> all other functions of debugfs (as per include/linux/debugfs.h) when
> debugfs is disabled in .config will be static inlined and we will not
> need any #ifdefs at all
> 
> The real pending question is about functional SR debugfs entries that
> need to loose it's critical functionality.

[sp] good argument for future not the present and past. Fact is that
     "wicked handling of debugfs entries" made their way.

     I am not worried about the patch in or out - few folks who were
     stuck on the issue would have used it anyway. But, whether each
     fix for today can be postponed for future restructuring.

     #ifdefs were just easy target for the postponement.

~sanjeev

> Regards,
> Nishanth Menon
> --
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] omap:pm: Fix boot-time errors with debugfs disabled

[Kevin Hilman]

"Premi, Sanjeev" <premi@ti.com> writes:

[...]

>> > [sp] I don't like #ifdefs either but each time we cannot create
>> >     a new file changes like this.
>> >
>> >     The current code is a mess with debugfs used too frequently.
>> >     And - all of it is not for debug. The location of ifdefs in
>> >     in the patch illustrates it quite well.
>> >
>> >     BTW, this isn't the only use of ifdefs in a C file in Linux.
>> in reality the only reason you've had to do this patch was because we
>> had a wicked handling of debugfs entries in voltage layer - with
>> voltdm_c these are all gone. further any entries remaining (e.g. SR)
>> are:
>> dentry for debugfs file -> just a minor overhead not 
>> deserving a #ifdef
>> all other functions of debugfs (as per include/linux/debugfs.h) when
>> debugfs is disabled in .config will be static inlined and we will not
>> need any #ifdefs at all
>> 
>> The real pending question is about functional SR debugfs entries that
>> need to loose it's critical functionality.
>
> [sp] good argument for future not the present and past. Fact is that
>      "wicked handling of debugfs entries" made their way.

Correct.  As maintainers, we do not always catch every problem.  Also,
we have not been as strict about some things in the past as we are now.

However, the fact that bad coding style exists in the kernel already is
not a good argument for accepting more.

>      I am not worried about the patch in or out - few folks who were
>      stuck on the issue would have used it anyway. But, whether each
>      fix for today can be postponed for future restructuring.
>
>      #ifdefs were just easy target for the postponement.

Nothing has to be postponed for future restructuring.

The reason $SUBJECT patch was not merged, was because the approach was
not correct.

If you submit an acceptable fix to this problem, I'll merge it today
even if I'm in the process of removing those features in parallel
restructuring work, because I don't know when my removal work will be
ready.

Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html