Hi Kevin,
a couple of comments,
On Fri, 18 Mar 2011, Kevin Hilman wrote:
> When a powerdomain is registered and it has an associated voltage domain,
> add the powerdomain to the voltagedomain using voltdm_add_pwrdm().
>
> Modeled after a similar relationship between clockdomains and powerdomains.
>
> Signed-off-by: Kevin Hilman <khilman@xxxxxx>
> ---
> arch/arm/mach-omap2/powerdomain.c | 1 +
> arch/arm/mach-omap2/voltage.c | 77 +++++++++++++++++++++++++++++++++++++
> arch/arm/mach-omap2/voltage.h | 11 +++++
> 3 files changed, 89 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/powerdomain.c b/arch/arm/mach-omap2/powerdomain.c
> index da86c72..2f16b43 100644
> --- a/arch/arm/mach-omap2/powerdomain.c
> +++ b/arch/arm/mach-omap2/powerdomain.c
> @@ -103,6 +103,7 @@ static int _pwrdm_register(struct powerdomain *pwrdm)
> return -EINVAL;
> }
> pwrdm->voltdm.ptr = voltdm;
> + voltdm_add_pwrdm(voltdm, pwrdm);
> }
>
> list_add(&pwrdm->node, &pwrdm_list);
> diff --git a/arch/arm/mach-omap2/voltage.c b/arch/arm/mach-omap2/voltage.c
> index 1dc6967..c25df81 100644
> --- a/arch/arm/mach-omap2/voltage.c
> +++ b/arch/arm/mach-omap2/voltage.c
> @@ -36,6 +36,7 @@
> #include "control.h"
>
> #include "voltage.h"
> +#include "powerdomain.h"
>
> #include "vc.h"
> #include "vp.h"
> @@ -1081,6 +1082,82 @@ static struct voltagedomain *_voltdm_lookup(const char *name)
> return voltdm;
> }
>
> +/**
> + * voltdm_add_pwrdm - add a powerdomain to a voltagedomain
> + * @voltdm: struct voltagedomain * to add the powerdomain to
> + * @pwrdm: struct powerdomain * to associate with a voltagedomain
> + *
> + * Associate the powerdomain @pwrdm with a voltagedomain @voltdm. This
> + * enables the use of voltdm_for_each_pwrdm(). Returns -EINVAL if
> + * presented with invalid pointers; -ENOMEM if memory could not be allocated;
> + * or 0 upon success.
> + */
> +int voltdm_add_pwrdm(struct voltagedomain *voltdm, struct powerdomain *pwrdm)
> +{
> + int i;
> + int ret = -EINVAL;
> +
> + if (!voltdm || !pwrdm)
> + return -EINVAL;
> +
> + pr_debug("voltagedomain: associating powerdomain %s with voltagedomain "
> + "%s\n", pwrdm->name, voltdm->name);
> +
> + for (i = 0; i < VOLTDM_MAX_PWRDMS; i++) {
> + if (!voltdm->voltdm_pwrdms[i])
> + break;
> +#ifdef DEBUG
> + if (voltdm->voltdm_pwrdms[i] == pwrdm) {
> + ret = -EINVAL;
> + goto vap_exit;
> + }
> +#endif
> + }
> +
> + if (i == VOLTDM_MAX_PWRDMS) {
> + pr_debug("voltagedomain: increase VOLTDM_MAX_PWRDMS for "
> + "voltdm %s pwrdm %s\n", voltdm->name, pwrdm->name);
> + WARN_ON(1);
> + ret = -ENOMEM;
> + goto vap_exit;
> + }
> +
> + voltdm->voltdm_pwrdms[i] = pwrdm;
> +
> + ret = 0;
> +
> +vap_exit:
> + return ret;
> +}
> +
> +/**
> + * voltdm_for_each_pwrdm - call function on each pwrdm in a voltdm
> + * @voltdm: struct voltagedomain * to iterate over
> + * @fn: callback function *
> + *
> + * Call the supplied function @fn for each powerdomain in the voltagedomain
> + * @voltdm. The callback function can return anything but 0 to bail
> + * out early from the iterator. Returns -EINVAL if presented with
> + * invalid pointers; or passes along the last return value of the
> + * callback function, which should be 0 for success or anything else
> + * to indicate failure.
> + */
> +int voltdm_for_each_pwrdm(struct voltagedomain *voltdm,
> + int (*fn)(struct voltagedomain *voltdm,
> + struct powerdomain *pwrdm))
> +{
> + int ret = 0;
> + int i;
> +
> + if (!fn)
> + return -EINVAL;
> +
> + for (i = 0; i < VOLTDM_MAX_PWRDMS && !ret; i++)
> + ret = (*fn)(voltdm, voltdm->voltdm_pwrdms[i]);
> +
> + return ret;
> +}
> +
> static int _voltdm_register(struct voltagedomain *voltdm)
> {
> if (!voltdm || !voltdm->name)
> diff --git a/arch/arm/mach-omap2/voltage.h b/arch/arm/mach-omap2/voltage.h
> index 13e5ed9..8e25126 100644
> --- a/arch/arm/mach-omap2/voltage.h
> +++ b/arch/arm/mach-omap2/voltage.h
> @@ -19,6 +19,11 @@
> #include "vc.h"
> #include "vp.h"
>
> +/*
> + * Maximum number of powerdomains that can be associated with a voltagedomain.
> + */
> +#define VOLTDM_MAX_PWRDMS 10
Since there are so many, I'd suggest using a list instead...
> +
> /* XXX document */
> #define VOLTSCALE_VPFORCEUPDATE 1
> #define VOLTSCALE_VCBYPASS 2
> @@ -55,11 +60,14 @@ struct omap_vfsm_instance_data {
> * @name: Name of the voltage domain which can be used as a unique identifier.
> * @node: list_head linking all voltage domains
> * @vdd: to be removed
> + * @voltdm_pwrdms: powerdomains in this voltagedomain
> */
> struct voltagedomain {
> char *name;
> struct list_head node;
> struct omap_vdd_info *vdd;
> +
> + struct powerdomain *voltdm_pwrdms[VOLTDM_MAX_PWRDMS];
... and just a minor nit in naming, I don't think the "voltdm_" part of
the name is necessary, it seems redundant to me. (And yes, I do think the
"pwrdm_" is redundant in the struct powerdomain too ;-)
> };
>
> /**
> @@ -183,4 +191,7 @@ extern void omap44xx_voltagedomains_init(void);
> struct voltagedomain *voltdm_lookup(const char *name);
> void voltdm_init(struct voltagedomain **voltdm_list);
> int voltdm_add_pwrdm(struct voltagedomain *voltdm, struct powerdomain *pwrdm);
> +int voltdm_for_each_pwrdm(struct voltagedomain *voltdm,
> + int (*fn)(struct voltagedomain *voltdm,
> + struct powerdomain *pwrdm));
> #endif
> --
> 1.7.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
- Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
