[PATCH 1/2] Adding platform level cpuidle driver for i.MX SoCs.

[Sascha Hauer]

On Fri, Aug 26, 2011 at 09:56:31AM -0500, Rob Lee wrote:
> Sascha and all,
> 
> Just FYI, this is my first submission to the community so I'm sure that I
> have much to learn about community style beyond what is given in
> the CodingStyle and Submit* documents.  Please give
> "community newbie" level details in your feedback.
> 
> My comments are below.
> 
> On 26 August 2011 02:27, Sascha Hauer <s.hauer at pengutronix.de> wrote:
> >
> > On Thu, Aug 25, 2011 at 09:33:14PM -0500, Robert Lee wrote:
> > > Signed-off-by: Robert Lee <rob.lee at linaro.org>
> > > ---
> > > --- /dev/null
> > > +++ b/arch/arm/plat-mxc/cpuidle.c
> > > @@ -0,0 +1,112 @@
> > > +/*
> > > + * This file is licensed under the terms of the GNU General Public
> > > + * License version 2.  This program is licensed "as is" without any
> > > + * warranty of any kind, whether express or implied.
> > > + */
> > > +
> > > +#include <linux/io.h>
> > > +#include <linux/cpuidle.h>
> > > +#include <asm/proc-fns.h>
> > > +#include <mach/hardware.h>
> > > +#include <mach/system.h>
> > > +#include <mach/cpuidle.h>
> > > +
> > > +
> > > +#ifndef MXC_OVERRIDE_DEFAULT_CPUIDLE_PARAMS
> >
> > Either there is a possibility to overwrite the cpuidle parameters
> > or there is none, but we don't need a define for this. I'm not
> > convinced we need this possibility at all though.
> >
> 
> This was simply to avoid the unnecessary memory usage by creating
> the default values if someone decided to override the default cpuidle
> parameters for their build.

Is a board known that wants to override this? If not, just drop it
and we'll wait until someone has valid reasons to do so. I think
this is a SoC only feature and the board has nothing to change here.

> 
> > > +
> > > +#define MXC_X_MACRO(a, b, c) {c}
> > > +static struct imx_cpuidle_params default_cpuidle_params[] = \
> > > +     MXC_CPUIDLE_TABLE;
> > > +#undef MXC_X_MACRO
> >
> > Hell! This is one of the worst unnecessary preprocessor abuses I've ever
> > seen. Do not show this in public again.
> >
> 
> Based on your response, it appears that standard C X-macros are not Linux
> kernel community friendly.

I never heard of standard C X-macros. You have an array of structs here,
there are no macros required to handle them.

> 
> > > +
> > > +static struct imx_cpuidle_params *imx_cpuidle_params = default_cpuidle_params;
> > > +
> > > +#else
> > > +
> > > +static struct imx_cpuidle_params *imx_cpuidle_params;
> > > +
> > > +#endif
> > > +
> > > +
> > > +
> > > +/* in case you want to override the mach level params at the board level */
> > > +void imx_cpuidle_board_params(struct imx_cpuidle_params *cpuidle_params)
> > > +{
> > > +     imx_cpuidle_params = cpuidle_params;
> > > +}
> > > +
> > > +static int imx_enter_idle(struct cpuidle_device *dev,
> > > +                            struct cpuidle_state *state)
> > > +{
> > > +     struct timeval before, after;
> > > +     int idle_time, i;
> > > +
> > > +     /* We only need to pass an index to the mach level so here we
> > > +      * find the index of the name contained in the cpuidle_state
> > > +      * to pass. */
> > > +     for (i = 0; i < MXC_NUM_CPUIDLE_STATES && i < CPUIDLE_STATE_MAX; i++)

btw please use cpuidle_set_statedata() / cpuidle_get_statedata instead
of looping around the states until you found the right one.

> >
> > A define for MXC_NUM_CPUIDLE_STATES looks wrong, it is not constant for
> > different SoCs.
> >
> 
> The CPUIDLE states will be constant among a family of SoCs such as mach-mx5, but
> the way I've written the driver, I've assumed a mach level defines a
> family which now
> that I think about it, obviously isn't the case for mach-imx.
> 
> If you have time, please give your thoughts on the organization of the mach
> directories with regards to mach-imx and mach-mx5 keeping in mind that
> i.MX6 will be coming soon.  This will help me in trying to make this driver
> more acceptable and I can pass this info on to others to discuss and learn
> from as well.

Generally:

- Put everything you need into a single driver (a platform driver)
- Do not call any functions imx_ or mx5_ functions outside your driver
- It seems that the cpuidle driver handles the same resources as
  platform_suspend_ops. I don't know enough of powermanagement to tell
  how both relate to each other, but as both use the same resources,
  both should be handled in the same place. That could mean that there
  are changes necessary to the current mx5 suspend ops.

When this is done, we can still decide whether we need a completely new
driver on i.MX6 or whether we can reuse the old one.

BTW. you don't need to think about i.MX6, you can also think about
i.MX3, i.MX2,..

Sascha



-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |