Re: [PATCH v2 1/2] iio: temperature: mcp9600: Provide index for both channels

[Andrew Hepp <andrew.hepp@ahepp.dev>]

Hi all,

I attempted to send this yesterday, but I guess I leaked some HTML into 
the message and it was rejected from the lists. I am resending it now as 
plain text. Apologies for any inconvenience or confusion.

On 5/19/24 12:14 PM, Jonathan Cameron wrote:
> On Fri, 17 May 2024 10:10:49 +0200
> Dimitri Fedrau <dima.fedrau@gmail.com> wrote:
> 
>> The mapping from cold junction to ambient temperature is inaccurate. We
>> provide an index for hot and cold junction temperatures.
>>
>> Suggested-by: Jonathan Cameron <jic23@kernel.org>
>> Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com>
> Hi Dmitri,
> 
> I'm not sure you replied to the question in previous review of what
> sysfs files exist for this device.  Whilst I am at least a little
> open to changing the ABI, I'd like to fully understand what
> is currently presented and why iio_info is having trouble with it.
> 
> I also want an ack from Andrew on this one given might break it existing
> usage.

I’m not actively using the cold junction temperature reading, so I would 
be happy to see any deficiencies in the ABI corrected.

> 
> The current interface is perhaps less than ideal, but I don't think it
> is wrong as such. Whilst I wasn't particularly keen on the cold junction
> == ambient I'm not sure moving to just indexed is an improvement.
> Hence looking for input from Andrew. +CC Nuno as someone who is both
> active in IIO and has written thermocouple front end drivers in
> the past.

The ABI docs state

     The ambient and object modifiers distinguish between ambient 
(reference) and distant temperatures for contactless measurements
Reading more of the Linux Driver API docs, those say that .modified is 
"used to indicate a physically unique characteristic of the channel”, 
and that .indexed is "simply another instance”.

I’m not sure whether measuring temperature at a different location meets 
the bar of a “physically unique characteristic”. Maybe it does. But I 
don’t think of the cold junction temperature as “simply another 
instance”. Perhaps that’s a mistake on my behalf.

Reviewing temperature drivers using IIO_MOD_TEMP_AMBIENT, they all seem 
to be reporting die temperatures. Some are IR sensors, but there are a 
couple other thermocouples like the MCP9600.

Reviewing drivers using “.indexed”, one is an IR sensor and one is a 
thermocouple. In both cases, the indexed channels seem to represent a 
“full featured” channel. The IR sensor also reports 
IIO_MOD_TEMP_AMBIENT, so they chose not to make it an additional index.

It seems to me that using IIO_MOD_TEMP_AMBIENT is more in line with what 
has been done in the past. But I may be misunderstanding something and I 
am not opposed to using and index if it’s determined that is more correct.

Thanks,
Andrew

> 
> Jonathan
> 
> 
>> ---
>>   drivers/iio/temperature/mcp9600.c | 9 +++++++--
>>   1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/iio/temperature/mcp9600.c b/drivers/iio/temperature/mcp9600.c
>> index 46845804292b..22451d1d9e1f 100644
>> --- a/drivers/iio/temperature/mcp9600.c
>> +++ b/drivers/iio/temperature/mcp9600.c
>> @@ -14,6 +14,9 @@
>>   
>>   #include <linux/iio/iio.h>
>>   
>> +#define MCP9600_CHAN_HOT_JUNCTION	0
>> +#define MCP9600_CHAN_COLD_JUNCTION	1
>> +
>>   /* MCP9600 registers */
>>   #define MCP9600_HOT_JUNCTION 0x0
>>   #define MCP9600_COLD_JUNCTION 0x2
>> @@ -25,17 +28,19 @@
>>   static const struct iio_chan_spec mcp9600_channels[] = {
>>   	{
>>   		.type = IIO_TEMP,
>> +		.channel = MCP9600_CHAN_HOT_JUNCTION,
>>   		.address = MCP9600_HOT_JUNCTION,
>>   		.info_mask_separate =
>>   			BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
>> +		.indexed = 1,
>>   	},
>>   	{
>>   		.type = IIO_TEMP,
>> +		.channel = MCP9600_CHAN_COLD_JUNCTION,
>>   		.address = MCP9600_COLD_JUNCTION,
>> -		.channel2 = IIO_MOD_TEMP_AMBIENT,
>> -		.modified = 1,
>>   		.info_mask_separate =
>>   			BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
>> +		.indexed = 1,
>>   	},
>>   };
>>   
>