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

[David Lechner <dlechner@baylibre.com>]

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