Re: [PATCH 1/5] dt-bindings: iio: mcp9600: Add compatible for microchip,mcp9601

[Ben Collins <ben.collins@linux.dev>]

[-- Attachment #1: Type: text/plain, Size: 5768 bytes --]

On Sun, Aug 17, 2025 at 12:59:48PM -0500, David Lechner wrote:
> On 8/17/25 12:34 PM, Ben Collins wrote:
> > On Sun, Aug 17, 2025 at 11:51:22AM -0500, David Lechner wrote:
> >> On 8/17/25 11:37 AM, Ben Collins wrote:
> >>> On Sat, Aug 16, 2025 at 01:55:31PM -0500, David Lechner wrote:
> >>>> On 8/16/25 4:58 AM, Jonathan Cameron wrote:
> >>>>> On Fri, 15 Aug 2025 16:46:03 +0000
> >>>>> Ben Collins <bcollins@watter.com> wrote:
> >>>>>
> >>>>>> The mcp9600 driver supports the mcp9601 chip, but complains about not
> >>>>>> recognizing the device id on probe. A separate patch...
> >>>>>>
> >>>>>> 	iio: mcp9600: Recognize chip id for mcp9601
> >>>>>>
> >>>>>> ...addresses this. This patch updates the dt-bindings for this chip to
> >>>>>> reflect the change to allow explicitly setting microchip,mcp9601 as
> >>>>>> the expected chip type.
> >>>>>>
> >>>>>> The mcp9601 also supports features not found on the mcp9600, so this
> >>>>>> will also allow the driver to differentiate the support of these
> >>>>>> features.
> >>>>>
> >>>>> If it's additional features only then you can still use a fallback
> >>>>> compatible.  Intent being that a new DT vs old kernel still 'works'.
> >>>>>
> >>>>> Then for the driver on new kernels we match on the new compatible and
> >>>>> support those new features.  Old kernel users get to keep the ID
> >>>>> mismatch warning - they can upgrade if they want that to go away ;)
> >>>>>
> >>>>> Krzysztof raised the same point on v2 but I'm not seeing it addressed
> >>>>> in that discussion.
> >>>>
> >>>> One could make the argument that these are not entirely fallback
> >>>> compatible since bit 4 of the STATUS register has a different
> >>>> meaning depending on if the chip is MCP9601/L01/RL01 or not.
> >>>
> >>> There are some nuances to this register between the two, but it can be
> >>> used generically as "not in range" for both.
> >>>
> >>> My understanding from the docs is if VSENSE is connected on mcp9601,
> >>> then it is explicitly open-circuit detection vs. short-circuit, which
> >>> is bit 5.
> >>>
> >>>> Interestingly, the existing bindings include interrupts for
> >>>> open circuit and short circuit alert pins. But these pins
> >>>> also only exist on MCP9601/L01/RL01. If we decide these aren't
> >>>> fallback compatible, then those properties should have the
> >>>> proper constraints added as well.
> >>>
> >>> In my v4 patch, I'm going to remove the short/open circuit interrupts
> >>> since they are not implemented, yet.
> >>
> >> Don't remove them from the devicetree bindings. Even if the Linux driver
> >> doesn't use it, the bindings should be as complete as possible.
> >>
> >> https://docs.kernel.org/devicetree/bindings/writing-bindings.html
> >>
> > 
> > I couldn't find anything that would easily describe this type of layout:
> > 
> > properties:
> > ...
> >   interrupts:
> >     minItems: 1
> >     maxItems: 4
> >   interrupt-names:
> >     minItems: 1
> >     items:
> >       - const: alert1
> >       - const: alert2
> >       - const: alert3
> >       - const: alert4
> > 
> > allOf:
> >   - if:
> >       properties:
> >         compatible:
> >           contains:
> >             const: microchip,mcp9601
> >     then:
> >       # Override maxItems
> >       interrupts:
> >         maxItems: 6
> >       # XXX Add items to existing list???
> >       interrupt-names:
> >         items:
> >           - const: open-circuit
> >           - const: short-circuit
> > 
> 
> We usually do this the other way around. The base binding lists
> all of the possibilities then an -if: constraint limits them
> if needed.
> 
> 
> So don't change what is there already and then add:
> 
> 
> allOf:
>   - if:
>       properties:
>         compatible:
>           not:
>             contains:
>               const: microchip,mcp9601
>     then:
>       properties:
>         interrupts:
>           maxItems: 4
>         interrupt-names:
>           maxItems: 4
>           enum:
>             - alert1
>             - alert2
>             - alert3
>             - alert4

This might be a little more complicated. I want to add a boolean for
microchip,vsense so the SC/OC aren't even available without that flag
being true (default false).

I could just assume that having the interrupts means this flag is true,
but that doesn't cover the case where the interrupts might not be used
or even wired up, but the SC/OC detection in the status register can be
used.

I was going with this:

  interrupts:
    minItems: 1
    maxItems: 4

  interrupt-names:
    minItems: 1
    items:
      - const: alert1
      - const: alert2
      - const: alert3
      - const: alert4

  microchip,vsense:
    default: false
    description:
      This flag indicates that the chip has been wired with VSENSE to
      enable open and short circuit detect. By default, this is false,
      since there's no way to detect that the chip is wired correctly.
    type: boolean

...

allOf:
  - if:
      properties:
        # XXX Does this work like logical AND? Passes dt_binding_check
        microchip,vsense: true
        compatible:
          contains:
            const: microchip,mcp9601
    then:
      properties:
        interrupts:
          minItems: 1
          maxItems: 6
        interrupt-names:
          items:
            - const: alert1
            - const: alert2
            - const: alert3
            - const: alert4
            - const: open-circuit
            - const: short-circuit
-- 
 Ben Collins
 https://libjwt.io
 https://github.com/benmcollins
 --
 3EC9 7598 1672 961A 1139  173A 5D5A 57C7 242B 22CF

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]