| Message ID | 1442339914-25843-10-git-send-email-dannenberg@ti.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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? > + 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? > +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? Best regards, Krzysztof -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Sep 16, 2015 at 02:54:53PM -0500, Andreas Dannenberg wrote: > On Wed, Sep 16, 2015 at 05:10:06PM +0900, Krzysztof Kozlowski wrote: > > On 16.09.2015 02:58, Andreas Dannenberg wrote: > > > 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. Actually I looked into this more and realized that the use of the pre-configured DEVICE_ATTR_* definitions imposes certain function names, which would mean renaming the functions including dropping the bq24257_ prefix those function names have, making the code inconsistent and decreasing the unique-ness of those function names in the global Kernel namespace. So I rather leave the more general DEVICE_ATTR() definitions in place. Regards, -- Andreas Dannenberg Texas Instruments Inc -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; + + 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); +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); static struct attribute *bq24257_charger_attr[] = { &dev_attr_ovp_voltage.attr, &dev_attr_in_dpm_voltage.attr, + &dev_attr_high_impedance_enable.attr, + &dev_attr_sysoff_enable.attr, NULL, };
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(+)