Re: [GIT PULL] power_supply: add power supply scope

[Anton Vorontsov]

A few more minor nits...

On Wed, Dec 07, 2011 at 05:41:37PM -0800, Jeremy Fitzhardinge wrote:
[...]
> diff --git a/drivers/power/power_supply_core.c b/drivers/power/power_supply_core.c
> index 329b46b..bacf327 100644
> --- a/drivers/power/power_supply_core.c
> +++ b/drivers/power/power_supply_core.c
> @@ -147,6 +147,12 @@ struct power_supply *power_supply_get_by_name(char *name)
>  }
>  EXPORT_SYMBOL_GPL(power_supply_get_by_name);
>  
> +

Needless newline. :-)

> +int power_supply_powers(struct power_supply *psy, struct device *dev)
> +{
> +	return sysfs_create_link_nowarn(&psy->dev->kobj, &dev->kobj, "powers");
> +}

- This surely creates an important ABI to the userland. This has to be
  documented via Documentation/ABI (or at least via commit message that
  I don't see in this email :-).

- This functionality isn't used anywhere as of now (i.e. I don't see
  anyone calling this function), so let's omit it for now?

- I still don't think that this should be a single symlink, to me the
  more universal (so that we don't have to break ABI in the future)
  way would be 'powers' directory with symlinks in it.
  But maybe I'm not following where exactly the link will be created?
  Documentation or an example of the new sysfs structure would help.

> +
>  static void power_supply_dev_release(struct device *dev)
>  {
>  	pr_debug("device: '%s': %s\n", dev_name(dev), __func__);
> @@ -202,6 +208,7 @@ EXPORT_SYMBOL_GPL(power_supply_register);
>  void power_supply_unregister(struct power_supply *psy)
>  {
>  	cancel_work_sync(&psy->changed_work);
> +	sysfs_remove_link(&psy->dev->kobj, "powers");
>  	power_supply_remove_triggers(psy);
>  	device_unregister(psy->dev);
>  }
> diff --git a/drivers/power/power_supply_sysfs.c b/drivers/power/power_supply_sysfs.c
> index e15d4c9..21178eb 100644
> --- a/drivers/power/power_supply_sysfs.c
> +++ b/drivers/power/power_supply_sysfs.c
> @@ -63,6 +63,9 @@ static ssize_t power_supply_show_property(struct device *dev,
>  	static char *capacity_level_text[] = {
>  		"Unknown", "Critical", "Low", "Normal", "High", "Full"
>  	};
> +	static char *scope_text[] = {
> +		"Unknown", "System", "Device"
> +	};
>  	ssize_t ret = 0;
>  	struct power_supply *psy = dev_get_drvdata(dev);
>  	const ptrdiff_t off = attr - power_supply_attrs;
> @@ -95,6 +98,8 @@ static ssize_t power_supply_show_property(struct device *dev,
>  		return sprintf(buf, "%s\n", capacity_level_text[value.intval]);
>  	else if (off == POWER_SUPPLY_PROP_TYPE)
>  		return sprintf(buf, "%s\n", type_text[value.intval]);
> +	else if (off == POWER_SUPPLY_PROP_SCOPE)
> +		return sprintf(buf, "%s\n", scope_text[value.intval]);

Should we really handle the PROP_SCOPE as a dynamic property?
Maybe do it similar to PROP_TYPE, so that drivers will only need to
specity the scope during registration, and not bother w/ handling
it in theirs get_property() callbacks?

Thanks!

-- 
Anton Vorontsov
Email: cbouatmailru@xxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/