Re: [RFC][PATCH 2/3] OMAP PM: Add support for bypassing VP/VC in Voltage layer

["Menon, Nishanth" <nm@ti.com>]

On Tue, Mar 15, 2011 at 20:03, Vishwanath BS <vishwanath.bs@ti.com> wrote:
> Currently Voltage layer assumes that all PMICs use VP/VC for Voltage scaling.
> There can be some instances where PMIC would want to bypass VP/VC for voltage
> scaling. So make VOltage layer flexible enough to handle this.
please fix VOltage.

The commit message does'nt actually explain enough. it is too vague.

>
> Signed-off-by: Vishwanath BS <vishwanath.bs@ti.com>
> ---
>  arch/arm/mach-omap2/omap_twl.c |    5 +++++
>  arch/arm/mach-omap2/voltage.c  |   12 +++++++-----
>  arch/arm/mach-omap2/voltage.h  |    6 ++++++
>  3 files changed, 18 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/omap_twl.c b/arch/arm/mach-omap2/omap_twl.c
> index f96d4b2..42f66c6 100644
> --- a/arch/arm/mach-omap2/omap_twl.c
> +++ b/arch/arm/mach-omap2/omap_twl.c
> @@ -157,6 +157,7 @@ static struct omap_volt_pmic_info omap3_mpu_volt_info = {
>        .onlp_cmd               = twl4030_uv_to_vsel,
>        .ret_cmd                = twl4030_uv_to_vsel,
>        .off_cmd                = twl4030_uv_to_vsel,
> +       .voltage_class  = VP_VC_CLASS,

Do you need this all unassigned params in a static struct is 0 which
is basically VP_VC_CLASS.

>  };
>
>  static struct omap_volt_pmic_info omap3_core_volt_info = {
> @@ -176,6 +177,7 @@ static struct omap_volt_pmic_info omap3_core_volt_info = {
>        .onlp_cmd               = twl4030_uv_to_vsel,
>        .ret_cmd                = twl4030_uv_to_vsel,
>        .off_cmd                = twl4030_uv_to_vsel,
> +       .voltage_class  = VP_VC_CLASS,
here as well

>  };
>
>  static struct omap_volt_pmic_info omap4_mpu_volt_info = {
> @@ -195,6 +197,7 @@ static struct omap_volt_pmic_info omap4_mpu_volt_info = {
>        .onlp_cmd               = twl4030_uv_to_vsel,
>        .ret_cmd                = twl4030_uv_to_vsel,
>        .off_cmd                = twl4030_uv_to_vsel,
> +       .voltage_class  = VP_VC_CLASS,
again

>  };
>
>  static struct omap_volt_pmic_info omap4_iva_volt_info = {
> @@ -214,6 +217,7 @@ static struct omap_volt_pmic_info omap4_iva_volt_info = {
>        .onlp_cmd               = twl4030_uv_to_vsel,
>        .ret_cmd                = twl4030_uv_to_vsel,
>        .off_cmd                = twl4030_uv_to_vsel,
> +       .voltage_class  = VP_VC_CLASS,
and again

>  };
>
>  static struct omap_volt_pmic_info omap4_core_volt_info = {
> @@ -233,6 +237,7 @@ static struct omap_volt_pmic_info omap4_core_volt_info = {
>        .onlp_cmd               = twl4030_uv_to_vsel,
>        .ret_cmd                = twl4030_uv_to_vsel,
>        .off_cmd                = twl4030_uv_to_vsel,
> +       .voltage_class  = VP_VC_CLASS,
and here
>  };
>
>  int __init omap4_twl_init(void)
> diff --git a/arch/arm/mach-omap2/voltage.c b/arch/arm/mach-omap2/voltage.c
> index 2ac990f..73fa3ee 100644
> --- a/arch/arm/mach-omap2/voltage.c
> +++ b/arch/arm/mach-omap2/voltage.c
> @@ -1194,11 +1194,13 @@ int __init omap_voltage_late_init(void)
>                pr_err("%s: Unable to create voltage debugfs main dir\n",
>                        __func__);
>        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]);
> -               vdd_debugfs_init(vdd_info[i]);
> +               if (vdd_info[i]->pmic_info->voltage_class == VP_VC_CLASS) {
> +                       if (omap_vdd_data_configure(vdd_info[i]))
> +                               continue;
> +                       omap_vc_init(vdd_info[i]);
> +                       vp_init(vdd_info[i]);
> +                       vdd_debugfs_init(vdd_info[i]);
> +               }

>        }
>
>        return 0;
> diff --git a/arch/arm/mach-omap2/voltage.h b/arch/arm/mach-omap2/voltage.h
> index de4e09b..17b1e10
> --- a/arch/arm/mach-omap2/voltage.h
> +++ b/arch/arm/mach-omap2/voltage.h
> @@ -23,6 +23,9 @@
>  #define VOLTSCALE_VPFORCEUPDATE                1
>  #define VOLTSCALE_VCBYPASS             2
>
> +#define VP_VC_CLASS 0
> +#define REGULATOR_CLASS 1
please - could you ensure namespace is proper in macro usage?
I dont understand usage difference between VP_VC_CLASS and REGULATOR_CLASS

> +
>  /**
>  * struct omap_vfsm_instance_data - per-voltage manager FSM register/bitfield
>  * data
> @@ -87,6 +90,8 @@ struct omap_volt_data {
>  * @onlp_cmd:  PMIC API to send onlp command instruction
>  * @ret_cmd:   PMIC API to send ret command instruction
>  * @off_cmd:   PMIC API to send off command instruction
> + * @voltage_class: Voltage class used for voltage scaling (can be VP/VC method
> + *                             or regulator based method for now).
>  */
>  struct omap_volt_pmic_info {
>        int slew_rate;
> @@ -105,6 +110,7 @@ struct omap_volt_pmic_info {
>        unsigned char (*onlp_cmd)(unsigned long uV);
>        unsigned char (*ret_cmd)(unsigned long uV);
>        unsigned char (*off_cmd)(unsigned long uV);
> +       u8 voltage_class;
>  };
>
>  /**
> --
> 1.7.0.4
overall, other than deciding to initialize or not initialize VP/VC
based on class, I dont see how you have achieved $subject or $commit
message in terms of bring in bypass ability to VP/VC by introducing
class.

A) the $subject is wrong. you have just introduced voltage_class - I
suggest you state that instead.
B) how your patch achieves support for bypassing vp/vc is not clear
unfortunately.

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