On 03/25/2013 08:24 PM, Rhyland Klein wrote:
> This patch adds support for supplies to register a list of char *'s
> which represent the list of supplies which supply them. This is the
> opposite as the supplied_to list.
>
> This change maintains support for supplied_to until all drivers which
> make use of it already are converted.
> diff --git a/drivers/power/power_supply_core.c b/drivers/power/power_supply_core.c
> +static int __power_supply_is_supplied_by(struct power_supply *supplier,
> + struct power_supply *supply)
Shouldn't this function return a Boolean since it's "is" something? At
least, 1 for yes 0 for no would be more comprehensible than 0 for yes
and error for no?
> +{
> + int i;
> +
> + if (!supply->supplied_from && !supplier->supplied_to)
> + return -EINVAL;
> +
> + /* Support both supplied_to and supplied_from modes */
> + if (supply->supplied_from) {
> + for (i = 0; i < supply->num_supplies; i++) {
> + if (!supplier->name)
> + continue;
That test is loop invariant. Why put it inside the loop?
Why wouldn't a supply have a name? The loop in
__power_supply_changed_work() that this function replaces doesn't test
for NULL names.
> + if (!strcmp(supplier->name, supply->supplied_from[i]))
> + return 0;
Don't you want to return something true here, so that the if block
inside __power_supply_changed_work() is executed in this case?
Similar comment for the else block.
> static int __power_supply_changed_work(struct device *dev, void *data)
> - for (i = 0; i < psy->num_supplicants; i++)
> - if (!strcmp(psy->supplied_to[i], pst->name)) {
> - if (pst->external_power_changed)
> - pst->external_power_changed(pst);
> - }
> + if (__power_supply_is_supplied_by(psy, pst)) {
> + if (pst->external_power_changed)
> + pst->external_power_changed(pst);
> + }
> +
> return 0;
> }
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
