Re: [PATCH v3 1/3] omap: opp: add OMAP3 OPP table data and common init
Thomas Petazzoni had written, on 11/16/2010 07:20 AM, the following: would prevent you from having no OPP table (the case where a NULL OPP table is passed is tested *before* in omapX_init_opp()). HUH?? NULL table to a static function - what code are you talking about?? why are you so behind BUG_ON, when there are valid reasons for reentry into code. In the current design, yes, there are indeed valid reasons for reentry into the omapX_init_opp() function, and that's exactly the point I'm critizicing here. how about: if (omap_table_init) return -EEXIST; does that make it better? it still retains the ability to handle both kinds of platforms as well as not BUG out. -- Regards, Nishanth Menon -- 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 v3 1/3] omap: opp: add OMAP3 OPP table data and common init
On Tue, 16 Nov 2010 08:16:07 -0800 Kevin Hilman khil...@deeprootsystems.com wrote: Yes, I'm not a big fan of the init function called multiple times either, but I really want to minimize what board files have to do. Historically, we tend to add all the init functions to every board file, and this is getting cumbersome to understand and maintain. What we need is for common code to take care of sensible defaults for all boards, and then only boards with non-default behavior have to have custom code. The other way is to have the board code register its customization into the OPP subsystem, and then when the OPP subsystem is initialized, it takes those customizations into account. But in that specific case, it's not clear how those customizations could easily be expressed, so maybe that multiple call strategy is the simplest solution. Thomas -- Thomas Petazzoni, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com -- 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 v3 1/3] omap: opp: add OMAP3 OPP table data and common init
Hello, On Mon, 15 Nov 2010 13:27:39 -0600 Nishanth Menon n...@ti.com wrote: + /* + * Allow multiple calls, but initialize only if not already initalized Minor: s/initalized/initialized/. + * even if the previous call failed, coz, no reason we'd succeed again + */ + if (omap_table_init) + return 0; + omap_table_init = 1; Do we really need this ? I personaly don't really like this quite of Hey, I'm already initialized, let's do nothing silently then. Unless there are strong reasons for which this function could be called twice, I'd rather not have this, or turn this into a BUG_ON(omap_table_init == 1). Regards, Thomas -- Thomas Petazzoni, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com -- 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 v3 1/3] omap: opp: add OMAP3 OPP table data and common init
Thomas Petazzoni wrote, on 11/16/2010 05:21 AM: Hello, On Mon, 15 Nov 2010 13:27:39 -0600 Nishanth Menonn...@ti.com wrote: + /* +* Allow multiple calls, but initialize only if not already initalized Minor: s/initalized/initialized/. aah thanks :) +* even if the previous call failed, coz, no reason we'd succeed again +*/ + if (omap_table_init) + return 0; + omap_table_init = 1; Do we really need this ? I personaly don't really like this quite of Hey, I'm already initialized, let's do nothing silently then. Unless there are strong reasons for which this function could be called twice, I'd rather not have this, or turn this into a BUG_ON(omap_table_init == 1). Yes, it is needed. The intent here is different. See the documentation that I put along with this patch - At times, board files may need to do customization to opps - like enable 1GHz on that platform alone - it can do it *only if* the defaults are registered, following which it can call opp_enable. when device_initcall follows this at a later point, it is still valid. btw, BUG_ON is a strict NO NO for me here - if I dont have OPP table, ok fine, system can still survive without cpufreq, no need to stop system operations because of that. -- Regards, Nishanth Menon -- 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 v3 1/3] omap: opp: add OMAP3 OPP table data and common init
Hello, On Tue, 16 Nov 2010 05:54:50 -0600 Nishanth Menon n...@ti.com wrote: Do we really need this ? I personaly don't really like this quite of Hey, I'm already initialized, let's do nothing silently then. Unless there are strong reasons for which this function could be called twice, I'd rather not have this, or turn this into a BUG_ON(omap_table_init == 1). Yes, it is needed. The intent here is different. See the documentation that I put along with this patch - At times, board files may need to do customization to opps - like enable 1GHz on that platform alone - it can do it *only if* the defaults are registered, following which it can call opp_enable. when device_initcall follows this at a later point, it is still valid. Ah, right, I didn't catch that omapX_init_opp() could be called first from the board init function, and then later on as a device_initcall() callback. But, sorry, I find this even uglier than I thought it was :) What about adding the obligation to boards file to call the omapX_init_opp() function and then do their customization (if needed), then no call to omapX_init_opp() would be needed as a device_initcall() callback. Or, add a mechanism that allows the board file to register its customizations, that are later taken into account by the omapX_init_opp() function. Maybe it's just a matter of personal taste, but I really don't like this kind of let's call this function once, do some stuff, then call it again, since it'll know that it shouldn't do anything. btw, BUG_ON is a strict NO NO for me here - if I dont have OPP table, ok fine, system can still survive without cpufreq, no need to stop system operations because of that. I don't see why replacing: + if (omap_table_init) + return 0; + omap_table_init = 1; by BUG_ON(omap_table_init == 1); omap_table_init = 1; would prevent you from having no OPP table (the case where a NULL OPP table is passed is tested *before* in omapX_init_opp()). Regards, Thomas -- Thomas Petazzoni, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com -- 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 v3 1/3] omap: opp: add OMAP3 OPP table data and common init
Thomas Petazzoni wrote, on 11/16/2010 06:42 AM: Hello, On Tue, 16 Nov 2010 05:54:50 -0600 Nishanth Menonn...@ti.com wrote: Do we really need this ? I personaly don't really like this quite of Hey, I'm already initialized, let's do nothing silently then. Unless there are strong reasons for which this function could be called twice, I'd rather not have this, or turn this into a BUG_ON(omap_table_init == 1). Yes, it is needed. The intent here is different. See the documentation that I put along with this patch - At times, board files may need to do customization to opps - like enable 1GHz on that platform alone - it can do it *only if* the defaults are registered, following which it can call opp_enable. when device_initcall follows this at a later point, it is still valid. Ah, right, I didn't catch that omapX_init_opp() could be called first from the board init function, and then later on as a device_initcall() callback. But, sorry, I find this even uglier than I thought it was :) What about adding the obligation to boards file to call the omapX_init_opp() function and then do their customization (if needed), then no call to omapX_init_opp() would be needed as a device_initcall() callback. Or, add a mechanism that allows the board file to register its customizations, that are later taken into account by the omapX_init_opp() function. Maybe it's just a matter of personal taste, but I really don't like this kind of let's call this function once, do some stuff, then call it again, since it'll know that it shouldn't do anything., I feel you may have misunderstood the code, we DONOT oblige all boards to *have* to call omapX_init_opp. It is a device_initcall - so for the boards that dont call it, device_initcall will trigger and initialzie it. the hooks for the customization of the OPPs is in OPP layer itself. the need we satisfy is this: if you need to support two sets of boards: a) boards that are happy with the defaults - most of the boards - dont do anything in the board file. (device_init_call with auto register the defaults) b) boards that need customization - these guys need to call omapX_init_opp(to register the defaults) before customizing the defaults. Does this explain the code and reason for the logic? if you do have a better mechanism, lets know. btw, BUG_ON is a strict NO NO for me here - if I dont have OPP table, ok fine, system can still survive without cpufreq, no need to stop system operations because of that. I don't see why replacing: + if (omap_table_init) + return 0; + omap_table_init = 1; by BUG_ON(omap_table_init == 1); omap_table_init = 1; would prevent you from having no OPP table (the case where a NULL OPP table is passed is tested *before* in omapX_init_opp()). HUH?? NULL table to a static function - what code are you talking about?? why are you so behind BUG_ON, when there are valid reasons for reentry into code. -- Regards, Nishanth Menon -- 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 v3 1/3] omap: opp: add OMAP3 OPP table data and common init
Hello, On Tue, 16 Nov 2010 07:10:36 -0600 Nishanth Menon n...@ti.com wrote: I feel you may have misunderstood the code, we DONOT oblige all boards to *have* to call omapX_init_opp. It is a device_initcall - so for the boards that dont call it, device_initcall will trigger and initialzie it. the hooks for the customization of the OPPs is in OPP layer itself. This is exactly what I have understood. the need we satisfy is this: if you need to support two sets of boards: a) boards that are happy with the defaults - most of the boards - dont do anything in the board file. (device_init_call with auto register the defaults) b) boards that need customization - these guys need to call omapX_init_opp(to register the defaults) before customizing the defaults. Does this explain the code and reason for the logic? if you do have a better mechanism, lets know. Yes, it explains the code and reason for the logic, but still doesn't make it pretty :-) would prevent you from having no OPP table (the case where a NULL OPP table is passed is tested *before* in omapX_init_opp()). HUH?? NULL table to a static function - what code are you talking about?? why are you so behind BUG_ON, when there are valid reasons for reentry into code. In the current design, yes, there are indeed valid reasons for reentry into the omapX_init_opp() function, and that's exactly the point I'm critizicing here. Regards! Thomas -- Thomas Petazzoni, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com -- 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 v3 1/3] omap: opp: add OMAP3 OPP table data and common init
Thomas Petazzoni had written, on 11/16/2010 06:42 AM, the following: But, sorry, I find this even uglier than I thought it was :) What about adding the obligation to boards file to call the omapX_init_opp() function and then do their customization (if needed), then no call to I knew I had discussed this before! Apologies, I should have dug this thread up earlier in the discussion. my initial implementation had forced board files to call the opp_init_table, then changed that here: http://marc.info/?l=linux-omapm=127431810922704w=2 Kevin seemed happy with the change here: http://marc.info/?l=linux-omapm=127507237109393w=2 -- Regards, Nishanth Menon -- 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 v3 1/3] omap: opp: add OMAP3 OPP table data and common init
On Tue, 16 Nov 2010 09:23:06 -0600 Nishanth Menon n...@ti.com wrote: my initial implementation had forced board files to call the opp_init_table, then changed that here: http://marc.info/?l=linux-omapm=127431810922704w=2 Kevin seemed happy with the change here: http://marc.info/?l=linux-omapm=127507237109393w=2 Ok, if Kevin is happy with this solution, fair enough. Sorry for the noise, and thanks for your answers. Regards, Thomas -- Thomas Petazzoni, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com -- 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 v3 1/3] omap: opp: add OMAP3 OPP table data and common init
Thomas Petazzoni had written, on 11/16/2010 09:50 AM, the following: On Tue, 16 Nov 2010 09:23:06 -0600 Nishanth Menon n...@ti.com wrote: my initial implementation had forced board files to call the opp_init_table, then changed that here: http://marc.info/?l=linux-omapm=127431810922704w=2 Kevin seemed happy with the change here: http://marc.info/?l=linux-omapm=127507237109393w=2 Ok, if Kevin is happy with this solution, fair enough. Sorry for the noise, and thanks for your answers. Thanks for taking the time to review and the comments. It is always a good idea to evolve to a better solution. If there are no further comments, I will post a v4 later today incorporating comments: a) return error instead of 0 if opp table is already initialized b) change the .h to .c -- Regards, Nishanth Menon -- 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 v3 1/3] omap: opp: add OMAP3 OPP table data and common init
Thomas Petazzoni thomas.petazz...@free-electrons.com writes: On Tue, 16 Nov 2010 09:23:06 -0600 Nishanth Menon n...@ti.com wrote: my initial implementation had forced board files to call the opp_init_table, then changed that here: http://marc.info/?l=linux-omapm=127431810922704w=2 Kevin seemed happy with the change here: http://marc.info/?l=linux-omapm=127507237109393w=2 Ok, if Kevin is happy with this solution, fair enough. Sorry for the noise, and thanks for your answers. Yes, I'm not a big fan of the init function called multiple times either, but I really want to minimize what board files have to do. Historically, we tend to add all the init functions to every board file, and this is getting cumbersome to understand and maintain. What we need is for common code to take care of sensible defaults for all boards, and then only boards with non-default behavior have to have custom code. Kevin -- 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 v3 1/3] omap: opp: add OMAP3 OPP table data and common init
* Nishanth Menon n...@ti.com [101115 16:43]:
Thomas Petazzoni had written, on 11/15/2010 04:51 PM, the following:
Hello,
On Mon, 15 Nov 2010 13:27:39 -0600
Nishanth Menon n...@ti.com wrote:
+++ b/arch/arm/mach-omap2/opp3xxx_data.h
+
+static struct omap_opp_def __initdata omap34xx_opp_def_list[] = {
+
+static struct omap_opp_def __initdata omap36xx_opp_def_list[] = {
Do we really want to have structure definitions in an header file ?
Unless I'm wrong, this means that if the opp3xxx_data.h file is
included in two different C files, the structures will be present twice.
The intent here - DONT DO precisely THAT!
As far as I could see, most of the kernel instantiate structure in C
files instead.
The intent here though was that opp3xx.h and opp4xx.h are private to
just opp.c to prevent misuse elsewhere. hmm.. thinking a bit,
find drivers/ -iname *.c|xargs grep #include| grep -v \.h
shows numerous examples of .c files being included in c files. I
dont have an issue of renaming these headers as .c file instead (I
had carried them over as .h from old implementation, but we can
change it), main point being, I just dont want folks mucking around
and hacking stuff with the defines.
What usually works best is to have common opp.c, then have opp34xx.c
that has initcall that registers the data in opp.c.
That leaves out if cpu_is_omapxxx else if stuff in opp.c and then
adding support for new omaps is just a matter of doing opp.c.
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 v3 1/3] omap: opp: add OMAP3 OPP table data and common init
* Kevin Hilman khil...@deeprootsystems.com [101116 08:06]: Thomas Petazzoni thomas.petazz...@free-electrons.com writes: On Tue, 16 Nov 2010 09:23:06 -0600 Nishanth Menon n...@ti.com wrote: my initial implementation had forced board files to call the opp_init_table, then changed that here: http://marc.info/?l=linux-omapm=127431810922704w=2 Kevin seemed happy with the change here: http://marc.info/?l=linux-omapm=127507237109393w=2 Ok, if Kevin is happy with this solution, fair enough. Sorry for the noise, and thanks for your answers. Yes, I'm not a big fan of the init function called multiple times either, but I really want to minimize what board files have to do. Historically, we tend to add all the init functions to every board file, and this is getting cumbersome to understand and maintain. What we need is for common code to take care of sensible defaults for all boards, and then only boards with non-default behavior have to have custom code. Yeah. The initial comment from Thomas with data in .h file should be fixed though. 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 v3 1/3] omap: opp: add OMAP3 OPP table data and common init
Tony Lindgren had written, on 11/16/2010 02:35 PM, the following:
* Nishanth Menon n...@ti.com [101115 16:43]:
Thomas Petazzoni had written, on 11/15/2010 04:51 PM, the following:
Hello,
On Mon, 15 Nov 2010 13:27:39 -0600
Nishanth Menon n...@ti.com wrote:
+++ b/arch/arm/mach-omap2/opp3xxx_data.h
+
+static struct omap_opp_def __initdata omap34xx_opp_def_list[] = {
+
+static struct omap_opp_def __initdata omap36xx_opp_def_list[] = {
Do we really want to have structure definitions in an header file ?
Unless I'm wrong, this means that if the opp3xxx_data.h file is
included in two different C files, the structures will be present twice.
The intent here - DONT DO precisely THAT!
As far as I could see, most of the kernel instantiate structure in C
files instead.
The intent here though was that opp3xx.h and opp4xx.h are private to
just opp.c to prevent misuse elsewhere. hmm.. thinking a bit,
find drivers/ -iname *.c|xargs grep #include| grep -v \.h
shows numerous examples of .c files being included in c files. I
dont have an issue of renaming these headers as .c file instead (I
had carried them over as .h from old implementation, but we can
change it), main point being, I just dont want folks mucking around
and hacking stuff with the defines.
What usually works best is to have common opp.c, then have opp34xx.c
that has initcall that registers the data in opp.c.
That leaves out if cpu_is_omapxxx else if stuff in opp.c and then
adding support for new omaps is just a matter of doing opp.c.
Series rev4 already posted here:
http://marc.info/?l=linux-omapm=128993367212640w=2
I believe I have taken care of the comments there - do let me know if I
screwed anything up here..
--
Regards,
Nishanth Menon
--
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 v3 1/3] omap: opp: add OMAP3 OPP table data and common init
Hello,
On Mon, 15 Nov 2010 13:27:39 -0600
Nishanth Menon n...@ti.com wrote:
+++ b/arch/arm/mach-omap2/opp3xxx_data.h
+
+static struct omap_opp_def __initdata omap34xx_opp_def_list[] = {
+
+static struct omap_opp_def __initdata omap36xx_opp_def_list[] = {
Do we really want to have structure definitions in an header file ?
Unless I'm wrong, this means that if the opp3xxx_data.h file is
included in two different C files, the structures will be present twice.
As far as I could see, most of the kernel instantiate structure in C
files instead.
Regards,
Thomas
--
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
--
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 v3 1/3] omap: opp: add OMAP3 OPP table data and common init
Thomas Petazzoni had written, on 11/15/2010 04:51 PM, the following:
Hello,
On Mon, 15 Nov 2010 13:27:39 -0600
Nishanth Menon n...@ti.com wrote:
+++ b/arch/arm/mach-omap2/opp3xxx_data.h
+
+static struct omap_opp_def __initdata omap34xx_opp_def_list[] = {
+
+static struct omap_opp_def __initdata omap36xx_opp_def_list[] = {
Do we really want to have structure definitions in an header file ?
Unless I'm wrong, this means that if the opp3xxx_data.h file is
included in two different C files, the structures will be present twice.
The intent here - DONT DO precisely THAT!
As far as I could see, most of the kernel instantiate structure in C
files instead.
The intent here though was that opp3xx.h and opp4xx.h are private to
just opp.c to prevent misuse elsewhere. hmm.. thinking a bit,
find drivers/ -iname *.c|xargs grep #include| grep -v \.h
shows numerous examples of .c files being included in c files. I dont
have an issue of renaming these headers as .c file instead (I had
carried them over as .h from old implementation, but we can change it),
main point being, I just dont want folks mucking around and hacking
stuff with the defines.
--
Regards,
Nishanth Menon
--
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
