| Message ID | 20210310033618.29354-1-chris.packham@alliedtelesis.co.nz (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | hwmon: (adm9240): Don't re-read config/limits | expand |
On 3/9/21 7:36 PM, Chris Packham wrote: > The hwmon chip is configured either via sysfs or by an earlier boot > stage (e.g. bootloader/bios). In the sysfs case we already store the > configuration values before it's written to the device. Reading in the > configuration only needs to be done once at probe time to cover the > second case. > > Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> > --- > > This doesn't resolve my ongoing i2c issues[0] but avoiding unnecessary > i2c reads will help a bit (it'll certainly avoid errors where the > threshold spontaneously changes). > > [0] - https://lore.kernel.org/lkml/8e0a88ba-01e9-9bc1-c78b-20f26ce27d12@alliedtelesis.co.nz/ > > drivers/hwmon/adm9240.c | 25 +++++++++++-------------- > 1 file changed, 11 insertions(+), 14 deletions(-) > > diff --git a/drivers/hwmon/adm9240.c b/drivers/hwmon/adm9240.c > index cc3e0184e720..7e1258b20b35 100644 > --- a/drivers/hwmon/adm9240.c > +++ b/drivers/hwmon/adm9240.c > @@ -128,7 +128,6 @@ struct adm9240_data { > struct mutex update_lock; > char valid; > unsigned long last_updated_measure; > - unsigned long last_updated_config; > > u8 in[6]; /* ro in0_input */ > u8 in_max[6]; /* rw in0_max */ > @@ -282,21 +281,11 @@ static struct adm9240_data *adm9240_update_device(struct device *dev) > return ERR_PTR(err); > } > data->last_updated_measure = jiffies; > - } > - > - /* minimum config reading cycle: 300 seconds */ > - if (time_after(jiffies, data->last_updated_config + (HZ * 300)) > - || !data->valid) { This is a bit more invasive than desirable. Problem is that it takes away the "!data->valid" part of the code as well, and there may be valid reasons to re-read the limits after an error. I'd suggest to just drop the time_after and last_updated_config and just check for data->valid. This also makes the change in the probe function unnecessary. I you _really_ want to make a difference, you could drop caching from the driver entirely and let regmap handle it (ie tell regmap which registers are hot). Caching isn't really that valuable, and is often more expensive than worth it. Even better, of course, would be to convert the driver to use devm_hwmon_device_register_with_info and drop all the sysfs code from it. Or, rather, almost all of it, since we'd have that non-standard aout_output attribute. Unfortunately, neither of those changes would solve your problem. > - err = adm9240_update_config(data); > - if (err < 0) { > - data->valid = 0; > - mutex_unlock(&data->update_lock); > - return ERR_PTR(err); > - } > - data->last_updated_config = jiffies; > data->valid = 1; > } > + > mutex_unlock(&data->update_lock); > + Please refrain from making whitespace changes. You may prefer the code that way, but next week I'll get someone else submitting a different patch and removing the added empty lines. > return data; > } > > @@ -855,7 +844,15 @@ static int adm9240_probe(struct i2c_client *new_client) > new_client->name, > data, > adm9240_groups); > - return PTR_ERR_OR_ZERO(hwmon_dev); > + if (IS_ERR(hwmon_dev)) > + return PTR_ERR(hwmon_dev); > + > + /* pull in configuration from an earlier boot stage */ > + err = adm9240_update_config(data); > + if (err < 0) > + return err; > + > + return 0; If at all, this call should be made before registering the hwmon device. > } > > static const struct i2c_device_id adm9240_id[] = { >
diff --git a/drivers/hwmon/adm9240.c b/drivers/hwmon/adm9240.c index cc3e0184e720..7e1258b20b35 100644 --- a/drivers/hwmon/adm9240.c +++ b/drivers/hwmon/adm9240.c @@ -128,7 +128,6 @@ struct adm9240_data { struct mutex update_lock; char valid; unsigned long last_updated_measure; - unsigned long last_updated_config; u8 in[6]; /* ro in0_input */ u8 in_max[6]; /* rw in0_max */ @@ -282,21 +281,11 @@ static struct adm9240_data *adm9240_update_device(struct device *dev) return ERR_PTR(err); } data->last_updated_measure = jiffies; - } - - /* minimum config reading cycle: 300 seconds */ - if (time_after(jiffies, data->last_updated_config + (HZ * 300)) - || !data->valid) { - err = adm9240_update_config(data); - if (err < 0) { - data->valid = 0; - mutex_unlock(&data->update_lock); - return ERR_PTR(err); - } - data->last_updated_config = jiffies; data->valid = 1; } + mutex_unlock(&data->update_lock); + return data; } @@ -855,7 +844,15 @@ static int adm9240_probe(struct i2c_client *new_client) new_client->name, data, adm9240_groups); - return PTR_ERR_OR_ZERO(hwmon_dev); + if (IS_ERR(hwmon_dev)) + return PTR_ERR(hwmon_dev); + + /* pull in configuration from an earlier boot stage */ + err = adm9240_update_config(data); + if (err < 0) + return err; + + return 0; } static const struct i2c_device_id adm9240_id[] = {
The hwmon chip is configured either via sysfs or by an earlier boot stage (e.g. bootloader/bios). In the sysfs case we already store the configuration values before it's written to the device. Reading in the configuration only needs to be done once at probe time to cover the second case. Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> --- This doesn't resolve my ongoing i2c issues[0] but avoiding unnecessary i2c reads will help a bit (it'll certainly avoid errors where the threshold spontaneously changes). [0] - https://lore.kernel.org/lkml/8e0a88ba-01e9-9bc1-c78b-20f26ce27d12@alliedtelesis.co.nz/ drivers/hwmon/adm9240.c | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-)