On Thu, Dec 29, 2022 at 05:39:30PM +0100, Krzysztof Kozlowski wrote: > On 29/12/2022 16:52, Guenter Roeck wrote: > >>> + adi,alarm1-interrupt-mode: > >>> + description: | > >>> + Enables the ALARM1 output to function in interrupt mode. > >>> + Default ALARM1 output function is comparator mode. > >> > >> Why this is a property of DT/hardware? Don't encode policy in DT. > >> > > > > I would not call this "policy". Normally it is an implementation > > question or decision, since interrupts behave differently depending > > on the mode. Impact is difficult to see, though, since the chip > > documentation is not available to the public. > > Some more background would be useful here from the author... > Indeed. Oddly enough I found the datasheet (as Google employee), but it is under NDA which means that I can't say much but "this is wrong" or "this doesn't work" in some or even all situations. I am not even sure if I can say that much because saying so implies providing information about the chip which I am bound to not expose. Sigh. Anyway, from public information and the driver. - The chip has one internal and four external channels. Channel configuration is per channel (there is an enable flag for each channel). This means devicetree configuration must be nested and include per-channel information (at the very least information if a channel is enabled). Other configuration information, if it exists, may also be per channel. That includes both resistance cancellation, beta compensation, and possibly other configuration data. One thing that came to mind was diode linearity factor. Examples for existing properties in other drivers: adi,ideal-factor-value (ltc2983) transistor-ideality (max6697) ti,n-factor (tmp421) extended-range-enable (may be dynamic; the driver currently disables it) ti,extended-range-enable (lm90) resistance-cancellation (max6697) beta-compensation-enable (max6697) ti,beta-compensation (tmp401) smbus-timeout-disable (max6697) alert-mask, over-temperature-mask (max6697) I am really happy with those; I think it should be automatic based on enabled channels temperature-offset-millicelsius (lm90) Then there is the question if temperature limits should be provided in devicetree, in addition to the above where appropriate. After all, those are thermal system properties. Examples for channel based devicetree property descriptions: devicetree/bindings/hwmon/ ti,tmp421.yaml nuvoton,nct7802.yaml national,lm90.yaml Looking into interrupts and interrupt modes: For the interrupt mode, I'll assume for the time being that its implementation is similar to that of other chips. I'll use MAX31730 as example, and subsequently refer to it as if MAX31732 would implement the same mechanism. In interrupt mode, interrupts are cleared by writing into the status register. If the condition still exists, the interrupt will be re-asserted after the next conversion. Clearly, that does not work with the driver as written - it would fill the log with messages, and send an endless amount of notifications to userspace. In comparator mode, interrupts are asserted whenever a temperature measurement exceeds a threshold value, and is deasserted when the temperature is back in the acceptable range. This would be equivalent to an edge triggered interrupt. The example below configures interrupts in interrupt mode but with IRQ_TYPE_EDGE_BOTH. No idea how that is supposed to work, because in interrupt mode the interrupt would have to be level triggered, not edge triggered, to make sense. Edge triggered interrupt only makes sense if the chip is in comparator mode, since only in that mode the edges have any meaning. The driver would need to know if the interrupt is edge triggered or level triggered, and that should decide how to handle it. Edge triggered should result in comparator mode, and level triggered should result in interrupt mode. Overall I am not sure if comparator mode even makes sense, since the interrupt line(s) would become active if a single temperature is out of range, and only become inactive if all temperatures are within range. Effecively this mode could only be used on a subset of channels, but that would require the ability to enable alerts on a per-channel basis. Either case, the interrupt handler would need to cache any previously reported status, since it should only generate a notification if the status changes, and not for each interrupt and each channel. With that, enough for now. Guenter > > > >>> + type: boolean > >>> + > >>> + adi,alarm2-interrupt-mode: > >>> + description: | > >>> + Enables the ALARM2 output to function in interrupt mode. > >>> + Default ALARM2 output function is comparator mode. > >> > >> Same question. > >> > >>> + type: boolean > >>> + > >>> + adi,alarm1-fault-queue: > >>> + description: The number of consecutive faults required to assert ALARM1. > >> > >> Same question - why this number differs with hardware? > >> > > > > Noisier hardware will require more samples to avoid spurious faults. > > Trade-off is speed of reporting a fault. Normally the board designer > > would determine a value which is low enough to avoid spurious faults. > > > > Note that the chip (according to patch 2/3) supports resistance > > cancellation as well as beta compensation, which are also board specific. > > I don't have access to the datasheet, so I don't know for sure if those > > are configurable (typically they are). If they are configurable, I would > > expect to see respective properties. > > If that's the argument behind property, then it's fine. > > Best regards, > Krzysztof >
