| Message ID | 20171102113116.14791-1-romain.porte@nokia.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
On 11/02/2017 04:31 AM, Romain Porte wrote: > By default the time before really updating a device is hardcoded to one second > in the pmbus_core.c file (HZ). This patch proposes a new configuration entry > that allows to tune the timeout value before updating a device. It defaults > to one second in order to stay compatible with the previous behavior, but can > now be changed if needed. > > Performing faster than one second sensor reads in userspace is what motivates > this patch. > > Reviewed-by: Alexander Sverdlin <alexander.sverdlin@nokia.com> NACK, I'd rather drop the caching entirely, and possibly convert the driver to regmap for caching non-volatile registers. Guenter > --- > drivers/hwmon/pmbus/Kconfig | 15 +++++++++++++++ > drivers/hwmon/pmbus/pmbus_core.c | 5 ++++- > 2 files changed, 19 insertions(+), 1 deletion(-) > > diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig > index 40019325b517..5d865e3d0cb1 100644 > --- a/drivers/hwmon/pmbus/Kconfig > +++ b/drivers/hwmon/pmbus/Kconfig > @@ -14,6 +14,21 @@ menuconfig PMBUS > > if PMBUS > > +config PMBUS_DEV_CACHE_MS > + int "PMBus device read cache in milliseconds" > + default 1000 > + help > + This options allows you to control how much time should a device > + be stored in cache before being updated. If you attempt to read > + a sensor value from the device before this cache time is elapsed, > + then you will get a cached value and the driver will not update > + the device values. > + > + This option is using jiffies under the hood and thus is not very > + precise. Setting this option to more than zero will perform a delay > + of at least one jiffie. Setting this option to zero will lead to > + no delay, thus forcing to perform an update on every read. > + > config SENSORS_PMBUS > tristate "Generic PMBus devices" > default y > diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c > index 302f0aef59de..2df0bc5a46f1 100644 > --- a/drivers/hwmon/pmbus/pmbus_core.c > +++ b/drivers/hwmon/pmbus/pmbus_core.c > @@ -414,9 +414,12 @@ static struct pmbus_data *pmbus_update_device(struct device *dev) > struct pmbus_data *data = i2c_get_clientdata(client); > const struct pmbus_driver_info *info = data->info; > struct pmbus_sensor *sensor; > + unsigned long timeout; > > mutex_lock(&data->update_lock); > - if (time_after(jiffies, data->last_updated + HZ) || !data->valid) { > + timeout = data->last_updated + > + (HZ * CONFIG_PMBUS_DEV_CACHE_MS + 999) / 1000; > + if (time_after(jiffies, timeout) || !data->valid) { > int i, j; > > for (i = 0; i < info->pages; i++) { > -- 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 02/11/2017 14:58, Guenter Roeck wrote: > NACK, I'd rather drop the caching entirely, and possibly convert the > driver to > regmap for caching non-volatile registers. I need to familiarize myself with regmap then, it looks like a nice abstraction to have. Do you think I can propose an intermediate patch for removing the cache support? (both time-based and data->valid based, so that every read actually performs a read, everytime) 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 Thu, Nov 02, 2017 at 05:17:21PM +0100, Romain Porte wrote: > On 02/11/2017 14:58, Guenter Roeck wrote: > >NACK, I'd rather drop the caching entirely, and possibly convert the > >driver to > >regmap for caching non-volatile registers. > I need to familiarize myself with regmap then, it looks like a nice > abstraction to have. > > Do you think I can propose an intermediate patch for removing the cache > support? (both time-based and data->valid based, so that every read actually > performs a read, everytime) > If you manage to do that, yes. That should limit reads to required reads, not just drop ->valid and re-read every register for each attribute access. Essentially, pmbus_update_device() should be completely gone. That isn't easy to implement, which is why I never got to doing it. 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..5d865e3d0cb1 100644 --- a/drivers/hwmon/pmbus/Kconfig +++ b/drivers/hwmon/pmbus/Kconfig @@ -14,6 +14,21 @@ menuconfig PMBUS if PMBUS +config PMBUS_DEV_CACHE_MS + int "PMBus device read cache in milliseconds" + default 1000 + help + This options allows you to control how much time should a device + be stored in cache before being updated. If you attempt to read + a sensor value from the device before this cache time is elapsed, + then you will get a cached value and the driver will not update + the device values. + + This option is using jiffies under the hood and thus is not very + precise. Setting this option to more than zero will perform a delay + of at least one jiffie. Setting this option to zero will lead to + no delay, thus forcing to perform an update on every read. + config SENSORS_PMBUS tristate "Generic PMBus devices" default y diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c index 302f0aef59de..2df0bc5a46f1 100644 --- a/drivers/hwmon/pmbus/pmbus_core.c +++ b/drivers/hwmon/pmbus/pmbus_core.c @@ -414,9 +414,12 @@ static struct pmbus_data *pmbus_update_device(struct device *dev) struct pmbus_data *data = i2c_get_clientdata(client); const struct pmbus_driver_info *info = data->info; struct pmbus_sensor *sensor; + unsigned long timeout; mutex_lock(&data->update_lock); - if (time_after(jiffies, data->last_updated + HZ) || !data->valid) { + timeout = data->last_updated + + (HZ * CONFIG_PMBUS_DEV_CACHE_MS + 999) / 1000; + if (time_after(jiffies, timeout) || !data->valid) { int i, j; for (i = 0; i < info->pages; i++) {
By default the time before really updating a device is hardcoded to one second in the pmbus_core.c file (HZ). This patch proposes a new configuration entry that allows to tune the timeout value before updating a device. It defaults to one second in order to stay compatible with the previous behavior, but can now be changed if needed. Performing faster than one second sensor reads in userspace is what motivates this patch. Reviewed-by: Alexander Sverdlin <alexander.sverdlin@nokia.com> --- drivers/hwmon/pmbus/Kconfig | 15 +++++++++++++++ drivers/hwmon/pmbus/pmbus_core.c | 5 ++++- 2 files changed, 19 insertions(+), 1 deletion(-)