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

[Dimitri Fedrau <dima.fedrau@gmail.com>]

Am Mon, May 20, 2024 at 10:28:10PM -0400 schrieb Andrew Hepp:
> 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 for the explanation and the effort, must have taken some time. I
think you are right. I will remove the patch from the series, so that no
ABI change takes place.

Best regards,
Dimitri

> 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,
> > >   	},
> > >   };
> >