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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20221229155227.GA22937@roeck-us.net>
Date:   Thu, 29 Dec 2022 07:52:27 -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 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@...log.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@...log.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