Re: [PATCH v4 09/10] power: bq24257: Add various device-specific sysfs properties

[Andreas Dannenberg <dannenberg@ti.com>]

On Wed, Sep 16, 2015 at 05:10:06PM +0900, Krzysztof Kozlowski wrote:
> On 16.09.2015 02:58, Andreas Dannenberg wrote:
> > This patch adds support for enabling/disabling optional device specific
> > features through sysfs properties at runtime.
> > 
> > * High-impedance mode enable/disable
> > * Sysoff enable/disable
> > 
> > Refer to the respective device datasheets for more information:
> > 
> > http://www.ti.com/product/bq24250
> > http://www.ti.com/product/bq24251
> > http://www.ti.com/product/bq24257
> > 
> > Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
> > ---
> >  drivers/power/bq24257_charger.c | 53 +++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 53 insertions(+)
> > 
> > diff --git a/drivers/power/bq24257_charger.c b/drivers/power/bq24257_charger.c
> > index 517a522..a0cb33c 100644
> > --- a/drivers/power/bq24257_charger.c
> > +++ b/drivers/power/bq24257_charger.c
> > @@ -794,12 +794,65 @@ static ssize_t bq24257_show_in_dpm_voltage(struct device *dev,
> >  			 bq24257_vindpm_map[bq->init_data.vindpm]);
> >  }
> >  
> > +static ssize_t bq24257_sysfs_show_enable(struct device *dev,
> > +					 struct device_attribute *attr,
> > +					 char *buf)
> > +{
> > +	struct power_supply *psy = dev_get_drvdata(dev);
> > +	struct bq24257_device *bq = power_supply_get_drvdata(psy);
> > +	int ret;
> > +
> > +	if (strcmp(attr->attr.name, "high_impedance_enable") == 0)
> > +		ret = bq24257_field_read(bq, F_HZ_MODE);
> > +	else if (strcmp(attr->attr.name, "sysoff_enable") == 0)
> > +		ret = bq24257_field_read(bq, F_SYSOFF);
> > +	else
> > +		return -EINVAL;
> > +
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	return scnprintf(buf, PAGE_SIZE, "%d\n", ret);
> > +}
> > +
> > +static ssize_t bq24257_sysfs_set_enable(struct device *dev,
> > +					struct device_attribute *attr,
> > +					const char *buf,
> > +					size_t count)
> > +{
> > +	struct power_supply *psy = dev_get_drvdata(dev);
> > +	struct bq24257_device *bq = power_supply_get_drvdata(psy);
> > +	long val;
> > +	int ret;
> > +
> > +	if (kstrtol(buf, 10, &val) < 0)
> > +		return -EINVAL;
> > +
> 
> There is no validation for input number. Although this is harmless but
> one may expect that writing value of 1, 2 or 3 would give the same
> result. But it would not. Value of 2 would be equal to 0, right?

You are right 2 would result in 0. Changing where the value gets passed
into bq24257_field_write() from "val" to "(bool)val" will make this more
forgiving.

> > +	if (strcmp(attr->attr.name, "high_impedance_enable") == 0)
> > +		ret = bq24257_field_write(bq, F_HZ_MODE, val);
> > +	else if (strcmp(attr->attr.name, "sysoff_enable") == 0)
> > +		ret = bq24257_field_write(bq, F_SYSOFF, val);
> > +	else
> > +		return -EINVAL;
> > +
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	return count;
> > +}
> > +
> >  static DEVICE_ATTR(ovp_voltage, S_IRUGO, bq24257_show_ovp_voltage, NULL);
> >  static DEVICE_ATTR(in_dpm_voltage, S_IRUGO, bq24257_show_in_dpm_voltage, NULL);
> 
> This applies to previous patches actually: DEVICE_ATTR_RO?

Ok. Will simplify.

> 
> > +static DEVICE_ATTR(high_impedance_enable, S_IWUSR | S_IRUGO,
> > +		   bq24257_sysfs_show_enable, bq24257_sysfs_set_enable);
> > +static DEVICE_ATTR(sysoff_enable, S_IWUSR | S_IRUGO,
> > +		   bq24257_sysfs_show_enable, bq24257_sysfs_set_enable);
> 
> DEVICE_ATTR_RW?

Ditto.

Regards,

--
Andreas Dannenberg
Texas Instruments Inc