Message-ID: <20221229183034.GA29265@roeck-us.net>
Date:   Thu, 29 Dec 2022 10:30:34 -0800
From:   Guenter Roeck <linux@...ck-us.net>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
Cc:     Sinan Divarci <Sinan.Divarci@...log.com>, jdelvare@...e.com,
        robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org,
        linux-hwmon@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 3/3] dt-bindings: hwmon: Add bindings for max31732

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
> 

Powered by blists - more mailing lists