On Mon, 21 Oct 2019 20:22:44 -0700 Guenter Roeck <linux@xxxxxxxxxxxx> wrote: > On 10/21/19 7:26 PM, Jeff LaBundy wrote: > > Hi Guenter, > > > > Thank you for your prompt review. > > > > On Mon, Oct 21, 2019 at 08:38:25AM -0700, Guenter Roeck wrote: > >> On Sun, Oct 20, 2019 at 11:11:19PM -0500, Jeff LaBundy wrote: > >>> This patch adds support for the Azoteq IQS620AT temperature sensor, > >>> capable of reporting its absolute die temperature. > >>> > >>> Signed-off-by: Jeff LaBundy <jeff@xxxxxxxxxxx> > >> > >> Seems to me this might be more feasible as iio driver. > >> Jonathan, what do you think ? > >> > > > > Interestingly enough, this actually started as an iio driver; however the > > "When to Use" slide of [0] made me suspect that conventional devices with > > the temperature sensing element integrated on the die belong in hwmon. > > > > I then found the highly similar ad7314, which Jonathan himself appears to > > have converted from iio to hwmon. Therefore, I placed this where existing > > drivers seemed to match the most, especially since the temperature sensors > > in iio generally use IR or a thermocouple. > > > > That being said, I would be happy to move this into iio so long as Jonathan > > does not mind, as it would limit the blast radius of this patch series. > > > > I don't recall why the ad7314 driver was moved. With a conversion time of 40uS > it is most definitely not a typical use case for a hwmon sensor. I'll be honest, I can't remember either ;) > > Anyway, not worth arguing about. Just don't complain later. There is an > iio->hwmon bridge, but no hwmon->iio bridge, so the decision does have some > impact. Specifically, userspace will have to implement both hwmon and iio > access to handle the chip. So I had a very quick look at one of the data sheets. The temperature sensor here is described as: "The IQS620(A) provides temperature monitoring capabilities which can be used for temperature change detection in order to ensure the integrity of other sensing technology". Superficially this sounds like it's probably inappropriate for any sort of system temperature monitoring. It's really just there to allow for clever compensation algorithms for the other bits on this chip (much like the temperature sensors we almost always get on a decent IMU). Normally we'd just tack an extra channel for the temperature sensor on to the the the sensor it is integrated with. This is a bit more complex though as we have 3 different IIO sensors that are present in particular part numbers and for some cases we have no IIO device at all, but do have a temperature sensor. So if people are going to actually use this to compensate outputs (not sure which ones are actually temperature sensitive btw ;) then if those are IIO supported devices, then probably makes sense for this to be an IIO device. It may make sense anyway if there is any chance of adding temperature compensation to the drivers in kernel. I suspect the only use that would actually be made is as a trip point if something has gone horribly wrong, but I might be wrong! Conclusion. I also don't feel strongly on this one as it kind of sits between IIO and hwmon, but probably ever so slightly on the IIO side as monitoring a sensor chip, not some other device. Thanks, Jonathan > > Guenter
