On Fri, Nov 4, 2016 at 4:09 PM, Guenter Roeck <linux@xxxxxxxxxxxx> wrote: > On Fri, Nov 04, 2016 at 09:24:25AM +0100, Andrea Merello wrote: >> On Thu, Nov 3, 2016 at 10:16 PM, Guenter Roeck <linux@xxxxxxxxxxxx> wrote: >> > On Thu, Nov 03, 2016 at 08:35:27AM +0100, Andrea Merello wrote: >> >> On Wed, Nov 2, 2016 at 11:48 PM, Guenter Roeck <linux@xxxxxxxxxxxx> wrote: >> >> > On Wed, Nov 02, 2016 at 09:11:35AM +0100, Andrea Merello wrote: >> >> >> On Mon, Oct 31, 2016 at 5:18 PM, Guenter Roeck <linux@xxxxxxxxxxxx> wrote: >> >> >> > On 10/31/2016 08:27 AM, Andrea Merello wrote: >> >> >> > >> >> >> >>>> >> >> >> >>>>>> + if (ret < 0) >> >> >> >>>>>> + return ret; >> >> >> >>>>>> + >> >> >> >>>>>> + priv->max_alert = priv->max_alert || !!(ret & >> >> >> >>>>>> STTS751_STATUS_TRIPH); >> >> >> >>>>>> + priv->min_alert = priv->min_alert || !!(ret & >> >> >> >>>>>> STTS751_STATUS_TRIPL); >> >> >> >>>>>> + >> >> >> >>>>> >> >> >> >>>>> >> >> >> >>>>> I am not really happy about the caching of alert values. This isn't >> >> >> >>>>> really >> >> >> >>>>> common. Usually we report current alert values when reading. I don't >> >> >> >>>>> really >> >> >> >>>>> see why this driver should be different to all the others. >> >> >> >>>> >> >> >> >>>> >> >> >> >>>> >> >> >> >>>> OK >> >> >> >> >> >> >> >> >> >> >> >> I've tried to avoid caching, but as soon as I read the status register >> >> >> >> in the alert handler, the device clears that reg, and it will remain >> >> >> >> cleared up to the next conversion. >> >> >> >> >> >> >> >> So for example if the reader is blocked on poll over the alert >> >> >> >> attribute, it gets unblocked on alert, and it will very likely read >> >> >> >> zero.. >> >> >> >> >> >> >> >> The only way I can see for avoiding caching, and for changing the API >> >> >> >> about clearing events as you asked below, is performing a temperature >> >> >> >> read from the temperature registers and doing software compare with >> >> >> >> the limits in show_[max/min]_alert. >> >> >> >> >> >> >> >> Does this sounds OK for you ? >> >> >> >> >> >> >> > >> >> >> > Not really. That could be done from user space by just reading the >> >> >> > temperature >> >> >> > and comparing it against limits. >> >> >> >> >> >> Sorry, I don't see the point here.. Why the fact that something could >> >> >> be done in userspace should suggest we don't want to do it in the >> >> >> driver? We do, for example, a simple calculation to give an absolute >> >> >> value for the hysteresis; we could do this also in userspace, but we >> >> >> do this in the driver in order to support the proper API.. >> >> >> >> >> > Datasheet says: >> >> > >> >> > T HIGH : [6] Bit = 1 indicates temperature high limit has been exceeded (T A > >> >> > high limit). T HIGH is cleared when the status register is read, provided >> >> > the condition no longer exists. >> >> > T LOW : [5] Bit = 1 indicates the is at or below the low limit (T A ≤ low >> >> > limit). T LOW is cleared when the status register is read, provided the >> >> > condition no longer exists. >> >> > >> >> > This isn't exactly what you are claiming, since it says "cleared ... provided >> >> > that the condition no longer exists", which would be more in line with other >> >> > chips. >> >> >> >> Yes, I know.. I've also read that on the datasheet.. And I tried >> >> reading the status register in both the alert handler and the show >> >> function, but it didn't worked :/ >> >> >> >> So I started to suspect the status register got actually clear after >> >> reading up to the next conversion, and to test this, I simply read it >> >> twice in the alert handler. The first read always read 1, as expected; >> >> the second time I always got zero, even if the temperature was still >> >> over-limit.. And I soon got another event, that behaved in the same >> >> manner. >> >> >> > Question is if the event bit is set again after the next conversion; this would >> > be relatively easy to handle (cache the status register for one conversion >> > time). >> >> Yes, I meant the HW does exactly this. But it seems to me that caching >> could end up in subtle races here.. >> >> For example suppose that user is reading the lower limit alarm file, >> or therm, and this will trigger read of the status reg; but a >> conversion is just ended and set the high limit alarm just before >> reading the status reg. Then the event handler is invoked but it will >> find out the status reg to be (incorrectly) zero, (or rely on a value >> cached when reading the lower limit, that was zero); thus a thread >> blocked on poll on the high alarm would not be woken up. (ok, this >> probably will happen in next conversions, but this doesn't sound quite >> sane to me..). >> > Good catch and thinking, but there is a workaround. If the cache time > has not expired, just logically or the cached status value with the current > value in the alert function, and update the time stamp. This way nothing > gets lost. Hmm, it seems OK; thanks for suggesting this :) I'll try to go this way.. BTW I would say that the cache time for the alert should be coservatively a bit longer than the conversion time (to avoid early expiration when the status register has been not restored yet). On the other hand for the temperature reading I'm using a cache time that is shorter (I think it is 1/4) than the conversion interval, so that the user gets a reasonably fresh value (becasue the chip sampling freq is asyncrhronous wrt the reads we do). Do you think it's worth to keep doing this (so we have two different cache time for alarms and temp value), or do you think that it's pointless, and better to unify it (at the longer one, I would say)? >> >> >> (also I'm thinking about a way to mask events once we know that we are >> >> in a out-of-limit condition, to avoid having a lot of them, but simply >> >> disabling events and re-enabling them after userspace reads the alarm >> >> and find it to be zero, like lm90 driver does, IMHO wouldn't be good >> >> here, because we have a single bit to disable both high and low >> >> limits..) >> >> >> >> > The THRM bit doesn't seem to self-clean either, at least not according to the >> >> > datasheet. >> >> >> >> I need to test this.. But this doesn't generate any event, it seems >> >> only inteded to drive an external circuit, say a fan, and not to >> >> notify SW.. And currently I even have no show function to read it's >> >> status (now that I'm on it, I think I should add it...).. >> >> >> > Yes. Note that this pin might be wired to an interrupt. >> >> IMHO it isn't straightforward to handle this.. >> >> AFAIK what is done by the smbus code to allow handling smbus events >> lines with regular interrupt lines is to briefly disabling the IRQ >> line until the event can be acknowledged with the ARA (in non atomic >> context). >> >> But here is even worse because there is no ARA; it seems that the >> therm line can't be "cleared" at all. >> >> Maybe it's possible to do hacks similar to what the smbus code does, >> but keeping the IRQ line disabled (given that it must not be shared).. >> Maybe if the IRQ controller could be set in edge-triggered mode it >> would be easier.. >> >> But does it really worth struggling supporting this scenario? There's >> the event pin explicitly designed for events, and there is code that >> make it possible to use this even with regular IRQs.. Using also the >> therm pin as interrupt seems honestly a bit pointless and >> inappropriate.. >> > Correct, one would use an edge triggered interrupt in this case. But > you are right, this should only be implemented if that use case is > actually encountered. > > Thanks, > Guenter -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
