On 10/19/22 15:03, Martin Blumenstingl wrote:
Hi Guenter,
Thank you for the quick feedback!
On Wed, Oct 19, 2022 at 11:51 PM Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
[...]
+ if (data->valid || data->temp[t_min])
This contradicts "with applying the previous values by only configuring
them if they are known valid". It explicitly applies the values if they
are marked as not valid, and it also applies the values if they are 0
(I don't really see the value of doing that).
Sorry, I don't understand the logic. Did you mean to use "&&" instead
of "||" ?
My understanding is that that:
1) data->valid = true is only set in jc42_update_device() (which is
only called when reading back the values from the registers)
2) jc42_write() can write values without setting data->value = true
In other words: if jc42_read() is never called but jc42_write() is
then we still have some setting to apply while data->valid is false.
Whether that's possible in reality is something that I'm not sure
about.
The above only means that the code is not optimized for the problem
you are trying to solve.
- Calling jc42_update_device() would solve the valid problem.
- Calling jc42_update_device() from the write function would
solve it as well.
Relying on a previous call to the write function is most definitely
wrong.
Also, for optimization, jc42_update_device() should really read the limit
registers only once. The best solution would probably be to convert the
driver to use regmap and let regmap handle the caching.
Guenter
If your suggestion is to simplify this to use data->valid only then I
can do that.
It would be great if you could also comment on whether
jc42_update_device() should be called from jc42_suspend() to give the
driver the chance to at least read the data once (and set data->valid)
if this has not happened before.
Best regards,
Martin
