Paul Walmsley <[email protected]> writes:
> 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 <[email protected]>
>> ---
>> 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...
>
Agreed, I thought the same when I copy-paste'd from pwrdm code.
Will rework.
>> +
>> /* 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.
Agreed.
> (And yes, I do think the "pwrdm_" is redundant in the struct
> powerdomain too ;-)
:)
Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
