Re: [PATCH v2 3/3] dt-bindings: hwmon: Add bindings for max31732

[Guenter Roeck <linux@roeck-us.net>]

On Wed, Dec 14, 2022 at 06:00:03PM +0100, Krzysztof Kozlowski wrote:
> On 14/12/2022 15:22, Sinan Divarci wrote:
> > Adding bindings for max31732 quad remote temperature sensor
> 
> Full stop.
> 
> Subject: drop second, redundant "bindings for".
> 
> > 
> > Signed-off-by: Sinan Divarci <Sinan.Divarci@analog.com>
> > ---
> >  .../bindings/hwmon/adi,max31732.yaml          | 83 +++++++++++++++++++
> >  1 file changed, 83 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/hwmon/adi,max31732.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/hwmon/adi,max31732.yaml b/Documentation/devicetree/bindings/hwmon/adi,max31732.yaml
> > new file mode 100644
> > index 000000000..c701cda95
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/hwmon/adi,max31732.yaml
> > @@ -0,0 +1,83 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +# Copyright 2022 Analog Devices Inc.
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/hwmon/adi,max31732.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Analog Devices MAX31732 Temperature Sensor Device Driver
> 
> Drop "Device Driver"
> 
> > +
> > +maintainers:
> > +  - Sinan Divarci <Sinan.Divarci@analog.com>
> > +
> > +description: Bindings for the Analog Devices MAX31732 Temperature Sensor Device.
> 
> Drop "Bindings for". Actually, either drop entire description or write
> something else than title.
> 
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - adi,max31732
> > +
> > +  reg:
> > +    description: I2C address of the Temperature Sensor Device.
> 
> Drop description.
> 
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    minItems: 1
> > +    maxItems: 2
> > +
> > +  interrupt-names:
> > +    description: Name of the interrupt pin of max31732 used for IRQ.
> 
> Drop description.
> 
> > +    minItems: 1
> > +    items:
> > +      - enum: [ALARM1, ALARM2]
> > +      - enum: [ALARM1, ALARM2]
> 
> This should be fixed, not flexible. Why it's flexible?
> 
> lowercase letters only
> 
> > +
> > +  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.

> > +    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.

Guenter