On Wed, 27 May 2020 16:33:34 -0700, Darrick J. Wong wrote: > On Wed, May 27, 2020 at 03:42:52PM -0700, Guenter Roeck wrote: > > On Tue, May 26, 2020 at 11:22:59AM +0200, Jean Delvare wrote: > > > Hi all, > > > > > > In the context of bug #207771, I got to look into the adt7470 driver. > > > I'm slowing understanding the logic of the background temperature > > > registers update thread, still there are 2 things I do not understand: > > > > > > 1* Function adt7470_read_temperatures() sets data->num_temp_sensors, > > > however this value seems to be only used to limit the wait time of > > > future calls to the same function. In the general update function we > > > still read ALL temperature sensors regardless of its value: > > > > > > for (i = 0; i < ADT7470_TEMP_COUNT; i++) > > > data->temp[i] = i2c_smbus_read_byte_data(client, > > > ADT7470_TEMP_REG(i)); > > > > > > Shouldn't this loop be bounded with data->num_temp_sensors instead of > > > ADT7470_TEMP_COUNT? > > > > > Guess so. > > > > > 2* Function adt7470_read_temperatures() also sets > > > data->temperatures_probed to 1, and this boolean is then used to skip > > > further calls to that function. But do we really need a separate > > > variable for this, given that num_temp_sensors >= 0 matches the same > > > condition as far as I can see? > > > > Agreed. On the other side, those are optimizations. I am not really sure > > if that driver is worth spending time on, given the age of the chip. Well it's still in use and apparently at least one user cares enough to report a bug and propose a candidate fix. > I /think/ the answer to both questions is yes, but I don't have the > hardware anymore so I have no way to QA that... :/ Thanks for both of you for your answers. Darrick, I have 3 more questions for you if you remember... 3* I understand that the temperatures need to be read periodically in order for automatic fan speed control to work. What I do not understand is why you bother switching PWM outputs to manual mode each time? What bad would happen if we did not do that? I see nothing in the datasheet justifying it. 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. 5* Is there any reason why the update thread is being started unconditionally? As I understand it, it is only needed if at least one PWM output is configured in automatic mode, which (I think) is not the default. It is odd that the bug reporter hits a problem with the polling thread when they are not using automatic fan speed control in the first place so they do not need the polling thread to run. I was wondering if it would be possible to start and stop the polling thread depending on whether automatic PWM is enabled or not. Thanks again, -- Jean Delvare SUSE L3 Support
