Re: [PATCH v3 1/2] dt-bindings: hwmon: add support for MCP998X

[<Victor.Duicu@microchip.com>]

On Tue, 2026-02-03 at 11:15 -0800, Guenter Roeck wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> On Tue, Jan 27, 2026 at 05:18:21PM +0200,
> victor.duicu@microchip.com wrote:
> > From: Victor Duicu <victor.duicu@microchip.com>
> > 
> > This is the devicetree schema for Microchip MCP998X/33 and
> > MCP998XD/33D Multichannel Automotive Temperature Monitor Family.
> > 
> > Acked-by: Conor Dooley <conor.dooley@microchip.com>
> > Signed-off-by: Victor Duicu <victor.duicu@microchip.com>
> 
> Some AI generated feedback inline, generated using Gemini 3 and Chris
> Mason's
> prompts as base. I don't know much if anything about devicetree
> properties,
> but it does seem to me that the AI has a point.
> 
> > ---
> >  .../bindings/hwmon/microchip,mcp9982.yaml     | 205
> > ++++++++++++++++++
> >  MAINTAINERS                                   |   6 +
> >  2 files changed, 211 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/hwmon/microchip,mcp9982.yaml
> > 
> > diff --git
> > a/Documentation/devicetree/bindings/hwmon/microchip,mcp9982.yaml
> > b/Documentation/devicetree/bindings/hwmon/microchip,mcp9982.yaml
> > new file mode 100644
> > index 000000000000..05ea3c6a5618
> > --- /dev/null
> > +++
> > b/Documentation/devicetree/bindings/hwmon/microchip,mcp9982.yaml
> > @@ -0,0 +1,205 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/hwmon/microchip,mcp9982.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Microchip MCP998X/33 and MCP998XD/33D Temperature Monitor
> > +
> > +maintainers:
> > +  - Victor Duicu <victor.duicu@microchip.com>
> > +
> > +description: |
> > +  The MCP998X/33 and MCP998XD/33D family is a high-accuracy 2-wire
> > +  multichannel automotive temperature monitor.
> > +  The datasheet can be found here:
> > +   
> > https://ww1.microchip.com/downloads/aemDocuments/documents/MSLD/ProductDocuments/DataSheets/MCP998X-Family-Data-Sheet-DS20006827.pdf
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - microchip,mcp9933
> > +      - microchip,mcp9933d
> > +      - microchip,mcp9982
> > +      - microchip,mcp9982d
> > +      - microchip,mcp9983
> > +      - microchip,mcp9983d
> > +      - microchip,mcp9984
> > +      - microchip,mcp9984d
> > +      - microchip,mcp9985
> > +      - microchip,mcp9985d
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    items:
> > +      - description: Signal coming from ALERT/THERM pin.
> > +      - description: Signal coming from THERM/ADDR pin.
> > +      - description: Signal coming from SYS_SHDN pin.
> > +
> > +  interrupt-names:
> > +    items:
> > +      - const: alert-therm
> > +      - const: therm-addr
> > +      - const: sys-shutdown
> 
> The top-level definition of interrupt-names specifies exactly 3
> items.
> How does this interact with variants that only have 2 interrupts?
> 

The chips with "D" in the family have the sys-shutdown and alert-therm
interrupt pins. The rest have alert-therm and therm-addr interrupt
pins. The conditional assigns the interrupt names depending on the
chip.

...

> 
> > +          items:
> > +            - const: alert-therm
> > +            - const: therm-addr
> > +        microchip,power-state: true
> > +
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            pattern: '^microchip,mcp998(2|3)$'
> > +    then:
> > +      properties:
> > +        microchip,enable-anti-parallel: false
> 
> If "D" variants only support Run mode as described in the property
> description, why is this property required in the devicetree?
> 
> Second feedback:
> 
> Since the description says "D" versions can only be set in Run mode
> (where True sets Run state), should this property also have a const:
> true
> constraint here?
> 

The property that sets the operation mode is microchip,power-state.
Chips with "D" can work only in Run mode, but the other ones can
work in Run or Standby mode.
In the conditions we force Run mode on the chips that require it
and accept either mode in the other half of the family.

> > +
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            pattern: '^microchip,mcp998(2|3)d$'
> > +    then:
> > +      properties:
> > +        microchip,enable-anti-parallel: false
> > +      required:
> > +        - microchip,parasitic-res-on-channel1-2
> > +        - microchip,parasitic-res-on-channel3-4
> 
> Should the parasitic resistance compensation properties be required?
> These
> represent board-specific parasitics and may not be present on all
> designs using these chip variants.
> 

As noted in the documentation, for chips with "D" parasitic resistance
error correction must be enabled so that the hardware shutdown feature
can't be overridden.

>