Re: [PATCH 2/3] bindings: hwmon: Add adi,adaq4224_temp as compatible string

From: Guenter Roeck <linux@roeck-us.net>
To: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>,
	Daniel Matyas <daniel.matyas@analog.com>
Cc: Jean Delvare <jdelvare@suse.com>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Jonathan Corbet <corbet@lwn.net>,
	linux-hwmon@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org
Subject: Re: [PATCH 2/3] bindings: hwmon: Add adi,adaq4224_temp as compatible string
Date: Fri, 15 Dec 2023 08:03:04 -0800	[thread overview]
Message-ID: <a5bc25d5-f59c-43ce-a44a-3eabc4b2d06c@roeck-us.net> (raw)
In-Reply-To: <43c3f6cb-aeb2-40c8-a79d-c2222414b49c@linaro.org>

On 12/15/23 00:49, Krzysztof Kozlowski wrote:
> On 14/12/2023 15:36, Daniel Matyas wrote:
>> In the device ada4224 the max31827 temperature sensor will be used, so
>> the default values corresponding to adaq4224_temp are the same for
>> max31827.
>>
>> Signed-off-by: Daniel Matyas <daniel.matyas@analog.com>
> 
> Please use subject prefixes matching the subsystem. You can get them for
> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
> your patch is touching.
> 
>> ---
>>   Documentation/devicetree/bindings/hwmon/adi,max31827.yaml | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/hwmon/adi,max31827.yaml b/Documentation/devicetree/bindings/hwmon/adi,max31827.yaml
>> index f60e06ab7d0a..9f3b0839aa46 100644
>> --- a/Documentation/devicetree/bindings/hwmon/adi,max31827.yaml
>> +++ b/Documentation/devicetree/bindings/hwmon/adi,max31827.yaml
>> @@ -20,6 +20,7 @@ properties:
>>         - const: adi,max31827
>>         - items:
>>             - enum:
>> +              - adi,adaq4224_temp
> 
> Underscores are not allowed
> 

That isn't the main problem with this patch.
https://github.com/analogdevicesinc/linux/tree/dev_adaq4224_dts
suggests that it may be a development system which utilizes the max31827.
 From there, we can see that there is a devicetree description of a board
with that name which uses

                 temperature1: temperature@5f {
                         compatible = "adi,adaq4224_temp";
                         reg = <0x5f>;
                         vref-supply = <&vio>;

                         adi,comp-int;
                         adi,alarm-pol = <0>;
                         adi,fault-q = <1>;
                         adi,timeout-enable;
                 };

That doesn't make sense to me. I don't know why they don't just reference max31827.
I am most definitely not going to accept a driver change just to map adi,adaq4224_temp
(or adi,adaq4224-temp) to the actually used temperature sensor chip. If we start
accepting that, we'd end up with no end of
"<vendor>,<board_name>-{temp,voltage,current,power,...}"
compatibles.

Looking more into the above mentioned repository/branch, an earlier version
of the dts file did reference adi,max31827 for that chip. It also looks like
there may be an adaq4224 ADC (per drivers/iio/adc/ad4630.c), but that would be
a SPI chip. It seems highly unlikely that a SPI ADC would have a separate I2C
interface connected to a temperature sensor. Confusing.

There is also some indication that this may some IP to be loaded into an FPGA.
which utilizes an FPGA implementation of max31827 (or maybe connects to one).
If that is the case, it should still be referenced as max31827.

All that wasted time because of "let's provide as little as possible information
about what we are actually doing" :-(.

Guenter