Re: Questions about adt7470 driver

[Jean Delvare <jdelvare@suse.de>]

On Thu, 28 May 2020 06:52:37 -0700, Guenter Roeck wrote:
> On 5/28/20 3:02 AM, Jean Delvare wrote:
> > Well it's still in use and apparently at least one user cares enough to
> > report a bug and propose a candidate fix.
>
> ... but at the same time that user doesn't have any temperature sensors
> installed and won't be able to test any changes.

They would be able to test the rest of the driver still. Plus I have a
register dump which I am feeding i2c-stub with, and that lets me test
the driver to some extent. God bless Mark M. Hoffman!

What I can't test are timing issues, and hardware misbehavior as
described by Darrick.

> > (...)
> > 4* Why are you calling msleep_interruptible() in
> > adt7470_read_temperatures() to wait for the temperature conversions? We
> > return -EAGAIN if that happens, but then ignore that error code, and we
> > log a cryptic error message. Do I understand correctly that the only
> > case where this should happen is when the user unloads the kernel
> > driver, in which case we do not care about having been interrupted? I
> > can't actually get the error message to be logged when rmmod'ing the
> > module so I don't know what it would take to trigger it.
> 
> Not sure if rmmod generates a signal.

I tested and it doesn't seem so. I expected rmmod to call
adt7470_remove(), in turn calling kthread_stop() and I thought this
would interrupt the thread. But the interrupt message never gets logged.

"modprobe adt7470 && rmmod adt7470" takes 2 seconds, so I assume that
rmmod gives the thread all the time it wants to exit on its own
(through kthread_should_stop()).

> In theory you could possibly
> keep writing -1 into the num_temp_sensors attribute, execute the
> sensors command (or just read a temperature), and interrupt the
> sequence.

Interrupt how? I tried Ctrl+C while "sensors" is waiting for the driver
to update the values, but it waits for completion before actually
exiting.

So far I couldn't find any way to get this msleep_interruptible() to
actually get interrupted.

> (...)
> The datasheet says nothing about the need to run such a thread for
> automatic mode either.

But that at least makes some sense due to the external nature of the
thermal sensors. The data needs to be fetched from the slaves somehow
before you can read it from the ADT7470's registers.

On the other hand, having to change PWM configuration registers for
temperature readings to be correct (/if/ that's what is happening
here... not sure) is highly unexpected.

> As I understand it, the chip is supposed to
> repeat temperature measurements automatically once configuration
> register 1 bit 7 is set. Of course that is difficult if not
> impossible to confirm without access to the chip.

Well there's clearly a huge design mistake for that chip, sharing a
pin between FULL_SPEED and TMP_START makes it pretty much impossible
for automatic temperature monitoring to be implemented without a
software loop. And a hardware monitor device that can't run on its own
is just asking for trouble. Oh well.

-- 
Jean Delvare
SUSE L3 Support