Re: [PATCH] power_supply: Use attribute groups

[Daniel Mack <daniel@caiaq.de>]

Hi Anton,

On Mon, May 17, 2010 at 11:40:16PM +0400, Anton Vorontsov wrote:
> This fixes a race between power supply device and initial
> attributes creation, plus makes it possible to implement
> writable properties.

Thanks a lot. This works for me, but I have two minor comments below ...

[...]

> diff --git a/drivers/power/power_supply_sysfs.c b/drivers/power/power_supply_sysfs.c
> index 5b6e352..f95d934 100644
> --- a/drivers/power/power_supply_sysfs.c
> +++ b/drivers/power/power_supply_sysfs.c
> @@ -41,6 +41,9 @@ static struct device_attribute power_supply_attrs[];
>  static ssize_t power_supply_show_property(struct device *dev,
>  					  struct device_attribute *attr,
>  					  char *buf) {
> +	static char *type_text[] = {
> +		"Battery", "UPS", "Mains", "USB"
> +	};
>  	static char *status_text[] = {
>  		"Unknown", "Charging", "Discharging", "Not charging", "Full"
>  	};
> @@ -58,12 +61,15 @@ static ssize_t power_supply_show_property(struct device *dev,
>  	static char *capacity_level_text[] = {
>  		"Unknown", "Critical", "Low", "Normal", "High", "Full"
>  	};
> -	ssize_t ret;
> +	ssize_t ret = 0;
>  	struct power_supply *psy = dev_get_drvdata(dev);
>  	const ptrdiff_t off = attr - power_supply_attrs;
>  	union power_supply_propval value;
>  
> -	ret = psy->get_property(psy, off, &value);
> +	if (off == POWER_SUPPLY_PROP_TYPE)
> +		value.intval = psy->type;
> +	else
> +		ret = psy->get_property(psy, off, &value);
>  
>  	if (ret < 0) {
>  		if (ret == -ENODATA)
> @@ -85,10 +91,13 @@ static ssize_t power_supply_show_property(struct device *dev,
>  		return sprintf(buf, "%s\n", technology_text[value.intval]);
>  	else if (off == POWER_SUPPLY_PROP_CAPACITY_LEVEL)
>  		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_MODEL_NAME)
>  		return sprintf(buf, "%s\n", value.strval);
>  
>  	return sprintf(buf, "%d\n", value.intval);
> +return 0;

This return is superflous :)

And with this approach, we don't need to initialize the .mode field in
the POWER_SUPPLY_ATTR anmore.

I'll then rebase my patches for writeable properties and resend them.

Thanks,
Daniel