| Message ID | 20171020103916.3309-2-romain.porte@nokia.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
On Fri, Oct 20, 2017 at 12:39:15PM +0200, Romain Porte wrote: > Trimming is a manufacturer-specific command provided by TPS544C20. > Since there is no generic interface for trimming yet this patch > proposes a new driver with dedicated sysfs entries for trimming > support for the component, according to its datasheet. This > allows users to access the VREF_TRIM and STORE_USER_ALL commands. This should be part of device programming in manufacturing, just like, say, current calibration. It should not be user programmable, even less so runtime programmable. On top of that, we definitely don't want to make STORE_USER_ALL available to user space. Both can too easily result in a bricked device (bad enough that the register values are writable using i2cset). Guenter > --- > drivers/hwmon/pmbus/Kconfig | 10 +++ > drivers/hwmon/pmbus/Makefile | 1 + > drivers/hwmon/pmbus/tps544c20.c | 164 ++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 175 insertions(+) > create mode 100644 drivers/hwmon/pmbus/tps544c20.c > > diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig > index 40019325b517..6f46e17d2004 100644 > --- a/drivers/hwmon/pmbus/Kconfig > +++ b/drivers/hwmon/pmbus/Kconfig > @@ -153,6 +153,16 @@ config SENSORS_TPS53679 > This driver can also be built as a module. If so, the module will > be called tps53679. > > +config SENSORS_TPS544C20 > + tristate "TI TPS544C20" > + default n > + help > + If you say yes here you get hardware monitoring support for TI > + TPS544C20 with trimming support. > + > + This driver can also be built as a module. If so, the module will > + be called tps544c20. > + > config SENSORS_UCD9000 > tristate "TI UCD90120, UCD90124, UCD90160, UCD9090, UCD90910" > default n > diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile > index 459a6be3390e..5b8e0a37551a 100644 > --- a/drivers/hwmon/pmbus/Makefile > +++ b/drivers/hwmon/pmbus/Makefile > @@ -16,6 +16,7 @@ obj-$(CONFIG_SENSORS_MAX34440) += max34440.o > obj-$(CONFIG_SENSORS_MAX8688) += max8688.o > obj-$(CONFIG_SENSORS_TPS40422) += tps40422.o > obj-$(CONFIG_SENSORS_TPS53679) += tps53679.o > +obj-$(CONFIG_SENSORS_TPS544C20) += tps544c20.o > obj-$(CONFIG_SENSORS_UCD9000) += ucd9000.o > obj-$(CONFIG_SENSORS_UCD9200) += ucd9200.o > obj-$(CONFIG_SENSORS_ZL6100) += zl6100.o > diff --git a/drivers/hwmon/pmbus/tps544c20.c b/drivers/hwmon/pmbus/tps544c20.c > new file mode 100644 > index 000000000000..241c030fc216 > --- /dev/null > +++ b/drivers/hwmon/pmbus/tps544c20.c > @@ -0,0 +1,164 @@ > +/* > + * Hardware monitoring driver for TI TPS544C20 with trim support > + * > + * Copyright (c) 2017 Nokia Solutions and Networks. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > + > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/init.h> > +#include <linux/err.h> > +#include <linux/i2c.h> > +#include <linux/delay.h> > +#include "pmbus.h" > + > +#define VREF_TRIM_ADDR 0xD4 > +#define STORE_USER_ALL_ADDR 0x15 > + > +static struct pmbus_driver_info tps544c20_info = { > + .pages = 1, > + .format[PSC_VOLTAGE_IN] = linear, > + .format[PSC_VOLTAGE_OUT] = linear, > + .format[PSC_TEMPERATURE] = linear, > + .func[0] = PMBUS_HAVE_VOUT | PMBUS_HAVE_TEMP2 > + | PMBUS_HAVE_STATUS_VOUT | PMBUS_HAVE_STATUS_TEMP > + | PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT, > +}; > + > +static ssize_t show_vref_trim(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + int ret; > + struct i2c_client *client = to_i2c_client(dev); > + > + ret = pmbus_read_word_data(client, 0, VREF_TRIM_ADDR); > + if (ret < 0) { > + dev_err(dev, "error reading vref_trim (%d)\n", ret); > + return ret; > + } > + > + ret = snprintf(buf, PAGE_SIZE, "%d\n", (int16_t) ret); > + return ret; > +} > + > +static ssize_t store_vref_trim(struct device *dev, > + struct device_attribute *attr, const char *buf, > + size_t count) > +{ > + int ret; > + int value; > + struct i2c_client *client = to_i2c_client(dev); > + > + ret = sscanf(buf, "%d", &value); Side note: There are API functions for that. > + if (ret != 1) { > + dev_err(dev, "bad value, please write integer\n"); Easy way to clog the kernel log. > + return -EINVAL; > + } > + > + ret = pmbus_write_word_data(client, 0, VREF_TRIM_ADDR, (int16_t) value); > + if (ret < 0) { > + dev_err(dev, "error writing vref_trim\n"); > + return ret; > + } > + > + /* Delay of at least 15 us/mV of change (0.067 mV/us), lets wait 10ms */ > + msleep(10); > + > + return count; > +} > + > +static ssize_t set_store_user_all(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + int ret; > + int read_value; > + struct i2c_client *client = to_i2c_client(dev); > + > + ret = sscanf(buf, "%d", &read_value); > + if (ret != 1 || read_value != 1) { > + dev_err(dev, "bad value, write 1 to store. abort\n"); > + return -EINVAL; > + } > + > + ret = pmbus_write_byte(client, 0, STORE_USER_ALL_ADDR); > + if (ret < 0) { > + dev_err(dev, "error writing store_user_all\n"); > + return ret; > + } > + > + return count; > +} > + > +DEVICE_ATTR(vref_trim, S_IWUSR | S_IRUGO, show_vref_trim, store_vref_trim); > +DEVICE_ATTR(store_user_all, S_IWUSR, NULL, set_store_user_all); > + > +static struct attribute *tps544c20_attributes[] = { > + &dev_attr_vref_trim.attr, > + &dev_attr_store_user_all.attr, > + NULL > +}; > + > +static const struct attribute_group tps544c20_group = { > + .attrs = tps544c20_attributes, > +}; > + > +static int tps544c20_probe(struct i2c_client *client, > + const struct i2c_device_id *id) > +{ > + int ret; > + > + ret = pmbus_do_probe(client, id, &tps544c20_info); > + if (ret < 0) { > + pr_err("error during pmbus probe\n"); > + return ret; > + } > + > + ret = sysfs_create_group(&client->dev.kobj, &tps544c20_group); > + if (ret < 0) { > + pr_err("error creating sysfs\n"); > + return ret; > + } > + > + return 0; > +} > + > +static int tps544c20_remove(struct i2c_client *client) > +{ > + sysfs_remove_group(&client->dev.kobj, &tps544c20_group); > + > + return pmbus_do_remove(client); > +} > + > +static const struct i2c_device_id tps544c20_id[] = { > + {"tps544c20", 0}, > + {} > +}; > + > +MODULE_DEVICE_TABLE(i2c, tps544c20_id); > + > +/* This is the driver that will be inserted */ > +static struct i2c_driver tps544c20_driver = { > + .driver = { > + .name = "tps544c20", > + }, > + .probe = tps544c20_probe, > + .remove = tps544c20_remove, > + .id_table = tps544c20_id, > +}; > + > +module_i2c_driver(tps544c20_driver); > + > +MODULE_AUTHOR("Romain Porte <romain.porte@nokia.com>"); > +MODULE_DESCRIPTION("PMBus driver for TI TPS544C20 with trim support"); > +MODULE_LICENSE("GPL"); -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 21/10/2017 18:20, Guenter Roeck wrote: > This should be part of device programming in manufacturing, just like, > say, current calibration. It should not be user programmable, even less so > runtime programmable. On top of that, we definitely don't want to make > STORE_USER_ALL available to user space. Both can too easily result in > a bricked device (bad enough that the register values are writable using > i2cset). My point is that I am exactly using Linux's pmbus implementation in order to perform a manufacturing calibration which is indeed a one-time operation. I agree that exposing these sysfs entries to end users can be dangerous. It can be useful if you actually want to perform a component calibration using Linux. Actually I am running an userspace application that does the calibration by writing to these sysfs entries. This driver is useful for manufacturing calibration and I think can be useful for other Linux users who wants to perform this kind of operation too. If this driver is dangerous for the end-user, how can we keep the features of this driver for manufacturing calibration using Linux? Maybe keep the generic driver for normal use and propose this specific driver as 'Advanced TPS544C20 driver [DANGER]'? Romain. -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 21/10/2017 18:20, Guenter Roeck wrote: > This should be part of device programming in manufacturing, just like, > say, current calibration. It should not be user programmable, even less so > runtime programmable. On top of that, we definitely don't want to make > STORE_USER_ALL available to user space. Both can too easily result in > a bricked device (bad enough that the register values are writable using > i2cset). After some work, I have tested accessing these registers using i2cget and i2cset and I can reproduce all the features I have implemented in this driver. I think am going to re-implement my calibration procedure using i2cget and i2cset in userspace. I thought it could be a good idea to make this accessible by sysfs entries. Since one must read the datasheet for calibrating the component, I guess it is not a big deal to have users to find register addresses/sizes and write to them using userspace commands instead of (over?)simplifying it by providing dedicated sysfs entries. Romain. -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Oct 23, 2017 at 09:54:26AM +0200, Romain Porte wrote: > On 21/10/2017 18:20, Guenter Roeck wrote: > >This should be part of device programming in manufacturing, just like, > >say, current calibration. It should not be user programmable, even less so > >runtime programmable. On top of that, we definitely don't want to make > >STORE_USER_ALL available to user space. Both can too easily result in > >a bricked device (bad enough that the register values are writable using > >i2cset). > My point is that I am exactly using Linux's pmbus implementation in order to > perform a manufacturing calibration which is indeed a one-time operation. > > I agree that exposing these sysfs entries to end users can be dangerous. It > can be useful if you actually want to perform a component calibration using > Linux. Actually I am running an userspace application that does the > calibration by writing to these sysfs entries. This driver is useful for > manufacturing calibration and I think can be useful for other Linux users > who wants to perform this kind of operation too. > > If this driver is dangerous for the end-user, how can we keep the features > of this driver for manufacturing calibration using Linux? Maybe keep the > generic driver for normal use and propose this specific driver as 'Advanced > TPS544C20 driver [DANGER]'? > You can use i2cset for your purpose and still use Linux. There is no need to have kernel support for it. With that, you can program all registers, not just a single one. Guenter -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Oct 23, 2017 at 03:34:24PM +0200, Romain Porte wrote: > On 21/10/2017 18:20, Guenter Roeck wrote: > >This should be part of device programming in manufacturing, just like, > >say, current calibration. It should not be user programmable, even less so > >runtime programmable. On top of that, we definitely don't want to make > >STORE_USER_ALL available to user space. Both can too easily result in > >a bricked device (bad enough that the register values are writable using > >i2cset). > After some work, I have tested accessing these registers using i2cget and > i2cset and I can reproduce all the features I have implemented in this > driver. I think am going to re-implement my calibration procedure using > i2cget and i2cset in userspace. > > I thought it could be a good idea to make this accessible by sysfs entries. > Since one must read the datasheet for calibrating the component, I guess it > is not a big deal to have users to find register addresses/sizes and write > to them using userspace commands instead of (over?)simplifying it by > providing dedicated sysfs entries. > Anyone tasked with calibration type work on any chip should _really_ read the datasheet. Guenter -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" 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/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig index 40019325b517..6f46e17d2004 100644 --- a/drivers/hwmon/pmbus/Kconfig +++ b/drivers/hwmon/pmbus/Kconfig @@ -153,6 +153,16 @@ config SENSORS_TPS53679 This driver can also be built as a module. If so, the module will be called tps53679. +config SENSORS_TPS544C20 + tristate "TI TPS544C20" + default n + help + If you say yes here you get hardware monitoring support for TI + TPS544C20 with trimming support. + + This driver can also be built as a module. If so, the module will + be called tps544c20. + config SENSORS_UCD9000 tristate "TI UCD90120, UCD90124, UCD90160, UCD9090, UCD90910" default n diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile index 459a6be3390e..5b8e0a37551a 100644 --- a/drivers/hwmon/pmbus/Makefile +++ b/drivers/hwmon/pmbus/Makefile @@ -16,6 +16,7 @@ obj-$(CONFIG_SENSORS_MAX34440) += max34440.o obj-$(CONFIG_SENSORS_MAX8688) += max8688.o obj-$(CONFIG_SENSORS_TPS40422) += tps40422.o obj-$(CONFIG_SENSORS_TPS53679) += tps53679.o +obj-$(CONFIG_SENSORS_TPS544C20) += tps544c20.o obj-$(CONFIG_SENSORS_UCD9000) += ucd9000.o obj-$(CONFIG_SENSORS_UCD9200) += ucd9200.o obj-$(CONFIG_SENSORS_ZL6100) += zl6100.o diff --git a/drivers/hwmon/pmbus/tps544c20.c b/drivers/hwmon/pmbus/tps544c20.c new file mode 100644 index 000000000000..241c030fc216 --- /dev/null +++ b/drivers/hwmon/pmbus/tps544c20.c @@ -0,0 +1,164 @@ +/* + * Hardware monitoring driver for TI TPS544C20 with trim support + * + * Copyright (c) 2017 Nokia Solutions and Networks. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/init.h> +#include <linux/err.h> +#include <linux/i2c.h> +#include <linux/delay.h> +#include "pmbus.h" + +#define VREF_TRIM_ADDR 0xD4 +#define STORE_USER_ALL_ADDR 0x15 + +static struct pmbus_driver_info tps544c20_info = { + .pages = 1, + .format[PSC_VOLTAGE_IN] = linear, + .format[PSC_VOLTAGE_OUT] = linear, + .format[PSC_TEMPERATURE] = linear, + .func[0] = PMBUS_HAVE_VOUT | PMBUS_HAVE_TEMP2 + | PMBUS_HAVE_STATUS_VOUT | PMBUS_HAVE_STATUS_TEMP + | PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT, +}; + +static ssize_t show_vref_trim(struct device *dev, + struct device_attribute *attr, char *buf) +{ + int ret; + struct i2c_client *client = to_i2c_client(dev); + + ret = pmbus_read_word_data(client, 0, VREF_TRIM_ADDR); + if (ret < 0) { + dev_err(dev, "error reading vref_trim (%d)\n", ret); + return ret; + } + + ret = snprintf(buf, PAGE_SIZE, "%d\n", (int16_t) ret); + return ret; +} + +static ssize_t store_vref_trim(struct device *dev, + struct device_attribute *attr, const char *buf, + size_t count) +{ + int ret; + int value; + struct i2c_client *client = to_i2c_client(dev); + + ret = sscanf(buf, "%d", &value); + if (ret != 1) { + dev_err(dev, "bad value, please write integer\n"); + return -EINVAL; + } + + ret = pmbus_write_word_data(client, 0, VREF_TRIM_ADDR, (int16_t) value); + if (ret < 0) { + dev_err(dev, "error writing vref_trim\n"); + return ret; + } + + /* Delay of at least 15 us/mV of change (0.067 mV/us), lets wait 10ms */ + msleep(10); + + return count; +} + +static ssize_t set_store_user_all(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t count) +{ + int ret; + int read_value; + struct i2c_client *client = to_i2c_client(dev); + + ret = sscanf(buf, "%d", &read_value); + if (ret != 1 || read_value != 1) { + dev_err(dev, "bad value, write 1 to store. abort\n"); + return -EINVAL; + } + + ret = pmbus_write_byte(client, 0, STORE_USER_ALL_ADDR); + if (ret < 0) { + dev_err(dev, "error writing store_user_all\n"); + return ret; + } + + return count; +} + +DEVICE_ATTR(vref_trim, S_IWUSR | S_IRUGO, show_vref_trim, store_vref_trim); +DEVICE_ATTR(store_user_all, S_IWUSR, NULL, set_store_user_all); + +static struct attribute *tps544c20_attributes[] = { + &dev_attr_vref_trim.attr, + &dev_attr_store_user_all.attr, + NULL +}; + +static const struct attribute_group tps544c20_group = { + .attrs = tps544c20_attributes, +}; + +static int tps544c20_probe(struct i2c_client *client, + const struct i2c_device_id *id) +{ + int ret; + + ret = pmbus_do_probe(client, id, &tps544c20_info); + if (ret < 0) { + pr_err("error during pmbus probe\n"); + return ret; + } + + ret = sysfs_create_group(&client->dev.kobj, &tps544c20_group); + if (ret < 0) { + pr_err("error creating sysfs\n"); + return ret; + } + + return 0; +} + +static int tps544c20_remove(struct i2c_client *client) +{ + sysfs_remove_group(&client->dev.kobj, &tps544c20_group); + + return pmbus_do_remove(client); +} + +static const struct i2c_device_id tps544c20_id[] = { + {"tps544c20", 0}, + {} +}; + +MODULE_DEVICE_TABLE(i2c, tps544c20_id); + +/* This is the driver that will be inserted */ +static struct i2c_driver tps544c20_driver = { + .driver = { + .name = "tps544c20", + }, + .probe = tps544c20_probe, + .remove = tps544c20_remove, + .id_table = tps544c20_id, +}; + +module_i2c_driver(tps544c20_driver); + +MODULE_AUTHOR("Romain Porte <romain.porte@nokia.com>"); +MODULE_DESCRIPTION("PMBus driver for TI TPS544C20 with trim support"); +MODULE_LICENSE("GPL");