[PATCH v2 0/2] Add threshold events support

[PATCH v2 0/2] Add threshold events support

[Dimitri Fedrau]

Add patch for providing index for both channels and add patch for threshold
events support for the MCP9600 device.

Changes in V2:
  - Remove pretty printing patches from series
  - Add patch for providing index for both channels(ABI change)!
    Suggested by Jonathan, hope this okay.
  - Remove formatting in a precursor patch
  - Add lock documentation
  - Add define MCP9600_TEMP_SCALE_NUM and use it instead of MICRO. MICRO is
    type unsigned long which we have to cast to int when using
    multiplication or division, because we are handling negative values.
  - Use switch statement in mcp9600_write_thresh
  - Replaced generic interrupt handler with four separate interrupt handler
  - Use one lock instead of four
  - Added error check for mcp9600_probe_alerts
     

Dimitri Fedrau (2):
  iio: temperature: mcp9600: Provide index for both channels
  iio: temperature: mcp9600: add threshold events support

 drivers/iio/temperature/mcp9600.c | 398 +++++++++++++++++++++++++++++-
 1 file changed, 396 insertions(+), 2 deletions(-)

-- 
2.39.2

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

[Dimitri Fedrau]

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

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

[Markus Elfring]

I suggest to reconsider the distribution of email addresses over recipient lists
once more.


…
> We provide an index for …

Please improve the change description with an imperative wording.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.9#n94

Regards,
Markus

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

[Jonathan Cameron]

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.

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.

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

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

[Dimitri Fedrau]

Am Sun, May 19, 2024 at 05:14:38PM +0100 schrieb Jonathan Cameron:
> 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,
>
Hi Jonathan,

> 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 did, see: https://lore.kernel.org/linux-iio/20240509193125.GA3614@debian/T/#u

But maybe not to the point. Sysfs is working correct and iio_info
probably not. There is only one channel found, the temp_ambient. I would
have expected two channels. Instead there are four attributes, I would
have expected two. It seems to me that they are just duplicated. I also
added the output when setting channel2 member of channel 0 to
IIO_MOD_TEMP_OBJECT. This time iio_info works fine.

> I also want an ack from Andrew on this one given might break it existing
> usage.
> 
> 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.
> 
I just thought the setting of channel2 member to IIO_MOD_TEMP_OBJECT was
missing. But it turned out that it is not set for a reason. I'm fine
with the existing mapping, but would be still interesting to know how
others think about the mapping.

Dimitri

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

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

[Jonathan Cameron]

On Sun, 19 May 2024 22:32:50 +0200
Dimitri Fedrau <dima.fedrau@gmail.com> wrote:

> Am Sun, May 19, 2024 at 05:14:38PM +0100 schrieb Jonathan Cameron:
> > 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,
> >  
> Hi Jonathan,
> 
> > 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 did, see: https://lore.kernel.org/linux-iio/20240509193125.GA3614@debian/T/#u

Ah thanks!  Oddly seem to have missed that on my other email account, but
have it here.  Thanks for the link.

> 
> But maybe not to the point. Sysfs is working correct and iio_info
> probably not. There is only one channel found, the temp_ambient. I would
> have expected two channels. Instead there are four attributes, I would
> have expected two. It seems to me that they are just duplicated. I also
> added the output when setting channel2 member of channel 0 to
> IIO_MOD_TEMP_OBJECT. This time iio_info works fine.

I'd be tempted to look at whether iio_info can be easily 'fixed' as a starting point.
This corner case may well occur for other devices in future.
It is 'stretching' the ABI definition but I don't think the current documentation
rules this case out.

> 
> > I also want an ack from Andrew on this one given might break it existing
> > usage.
> > 
> > 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.
> >   
> I just thought the setting of channel2 member to IIO_MOD_TEMP_OBJECT was
> missing. But it turned out that it is not set for a reason. I'm fine
> with the existing mapping, but would be still interesting to know how
> others think about the mapping.

Agreed - I'm not sure on the right thing to do here.
I suspect leaving the modifier and add the index may be the best we can do
but let us wait to see what others think.

Jonathan

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

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

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

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

[Dimitri Fedrau]

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

[PATCH v2 2/2] iio: temperature: mcp9600: add threshold events support

[Dimitri Fedrau]

The device has four programmable temperature alert outputs which can be
used to monitor hot or cold-junction temperatures and detect falling and
rising temperatures. It supports up to 255 degree celsius programmable
hysteresis. Each alert can be individually configured by setting following
options in the associated alert configuration register:
- monitor hot or cold junction temperature
- monitor rising or falling temperature
- set comparator or interrupt mode
- set output polarity
- enable alert

This patch binds alert outputs to iio events:
- alert1: hot junction, rising temperature
- alert2: hot junction, falling temperature
- alert3: cold junction, rising temperature
- alert4: cold junction, falling temperature

All outputs are set in comparator mode and polarity depends on interrupt
configuration.

Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com>
---
 drivers/iio/temperature/mcp9600.c | 389 ++++++++++++++++++++++++++++++
 1 file changed, 389 insertions(+)

diff --git a/drivers/iio/temperature/mcp9600.c b/drivers/iio/temperature/mcp9600.c
index 22451d1d9e1f..91d811fe9371 100644
--- a/drivers/iio/temperature/mcp9600.c
+++ b/drivers/iio/temperature/mcp9600.c
@@ -6,12 +6,21 @@
  * Author: <andrew.hepp@ahepp.dev>
  */
 
+#include <linux/bitfield.h>
+#include <linux/bitops.h>
+#include <linux/bits.h>
 #include <linux/err.h>
 #include <linux/i2c.h>
 #include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/math.h>
+#include <linux/minmax.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 
+#include <linux/iio/events.h>
 #include <linux/iio/iio.h>
 
 #define MCP9600_CHAN_HOT_JUNCTION	0
@@ -20,11 +29,65 @@
 /* MCP9600 registers */
 #define MCP9600_HOT_JUNCTION 0x0
 #define MCP9600_COLD_JUNCTION 0x2
+#define MCP9600_STATUS			0x4
+#define MCP9600_STATUS_ALERT(x)		BIT(x)
+#define MCP9600_ALERT_CFG1		0x8
+#define MCP9600_ALERT_CFG(x)		(MCP9600_ALERT_CFG1 + (x - 1))
+#define MCP9600_ALERT_CFG_ENABLE	BIT(0)
+#define MCP9600_ALERT_CFG_ACTIVE_HIGH	BIT(2)
+#define MCP9600_ALERT_CFG_FALLING	BIT(3)
+#define MCP9600_ALERT_CFG_COLD_JUNCTION	BIT(4)
+#define MCP9600_ALERT_HYSTERESIS1	0xc
+#define MCP9600_ALERT_HYSTERESIS(x)	(MCP9600_ALERT_HYSTERESIS1 + (x - 1))
+#define MCP9600_ALERT_LIMIT1		0x10
+#define MCP9600_ALERT_LIMIT(x)		(MCP9600_ALERT_LIMIT1 + (x - 1))
+#define MCP9600_ALERT_LIMIT_MASK	GENMASK(15, 2)
 #define MCP9600_DEVICE_ID 0x20
 
 /* MCP9600 device id value */
 #define MCP9600_DEVICE_ID_MCP9600 0x40
 
+#define MCP9600_ALERT_COUNT		4
+
+#define MCP9600_TEMP_SCALE_NUM		1000000
+
+#define MCP9600_MIN_TEMP_HOT_JUNCTION	-200
+#define MCP9600_MAX_TEMP_HOT_JUNCTION	1800
+
+#define MCP9600_MIN_TEMP_COLD_JUNCTION	-40
+#define MCP9600_MAX_TEMP_COLD_JUNCTION	125
+
+enum mcp9600_alert {
+	MCP9600_ALERT1,
+	MCP9600_ALERT2,
+	MCP9600_ALERT3,
+	MCP9600_ALERT4
+};
+
+static const char * const mcp9600_alert_name[MCP9600_ALERT_COUNT] = {
+	[MCP9600_ALERT1] = "alert1",
+	[MCP9600_ALERT2] = "alert2",
+	[MCP9600_ALERT3] = "alert3",
+	[MCP9600_ALERT4] = "alert4",
+};
+
+static const struct iio_event_spec mcp9600_events[] = {
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_RISING,
+		.mask_separate = BIT(IIO_EV_INFO_ENABLE) |
+				 BIT(IIO_EV_INFO_VALUE) |
+				 BIT(IIO_EV_INFO_HYSTERESIS),
+	},
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_FALLING,
+		.mask_separate = BIT(IIO_EV_INFO_ENABLE) |
+				 BIT(IIO_EV_INFO_VALUE) |
+				 BIT(IIO_EV_INFO_HYSTERESIS),
+	},
+};
+
 static const struct iio_chan_spec mcp9600_channels[] = {
 	{
 		.type = IIO_TEMP,
@@ -33,6 +96,8 @@ static const struct iio_chan_spec mcp9600_channels[] = {
 		.info_mask_separate =
 			BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
 		.indexed = 1,
+		.event_spec = mcp9600_events,
+		.num_event_specs = ARRAY_SIZE(mcp9600_events),
 	},
 	{
 		.type = IIO_TEMP,
@@ -41,11 +106,18 @@ static const struct iio_chan_spec mcp9600_channels[] = {
 		.info_mask_separate =
 			BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
 		.indexed = 1,
+		.event_spec = mcp9600_events,
+		.num_event_specs = ARRAY_SIZE(mcp9600_events),
 	},
 };
 
 struct mcp9600_data {
 	struct i2c_client *client;
+	/*
+	 * Serializes access to threshold and hysteresis values, since the
+	 * latter are stored as offsets from thresholds on the device.
+	 */
+	struct mutex lock;
 };
 
 static int mcp9600_read(struct mcp9600_data *data,
@@ -84,10 +156,321 @@ static int mcp9600_read_raw(struct iio_dev *indio_dev,
 	}
 }
 
+static int mcp9600_get_alert_index(int channel, enum iio_event_direction dir)
+{
+	if (channel == MCP9600_CHAN_HOT_JUNCTION) {
+		if (dir == IIO_EV_DIR_RISING)
+			return MCP9600_ALERT1;
+		else
+			return MCP9600_ALERT2;
+	} else {
+		if (dir == IIO_EV_DIR_RISING)
+			return MCP9600_ALERT3;
+		else
+			return MCP9600_ALERT4;
+	}
+}
+
+static int mcp9600_read_event_config(struct iio_dev *indio_dev,
+				     const struct iio_chan_spec *chan,
+				     enum iio_event_type type,
+				     enum iio_event_direction dir)
+{
+	struct mcp9600_data *data = iio_priv(indio_dev);
+	struct i2c_client *client = data->client;
+	int i, ret;
+
+	i = mcp9600_get_alert_index(chan->channel, dir);
+	ret = i2c_smbus_read_byte_data(client, MCP9600_ALERT_CFG(i + 1));
+	if (ret < 0)
+		return ret;
+
+	return FIELD_GET(MCP9600_ALERT_CFG_ENABLE, ret);
+}
+
+static int mcp9600_write_event_config(struct iio_dev *indio_dev,
+				      const struct iio_chan_spec *chan,
+				      enum iio_event_type type,
+				      enum iio_event_direction dir,
+				      int state)
+{
+	struct mcp9600_data *data = iio_priv(indio_dev);
+	struct i2c_client *client = data->client;
+	int i, ret;
+
+	i = mcp9600_get_alert_index(chan->channel, dir);
+	ret = i2c_smbus_read_byte_data(client, MCP9600_ALERT_CFG(i + 1));
+	if (ret < 0)
+		return ret;
+
+	if (state)
+		ret |= MCP9600_ALERT_CFG_ENABLE;
+	else
+		ret &= ~MCP9600_ALERT_CFG_ENABLE;
+
+	return i2c_smbus_write_byte_data(client, MCP9600_ALERT_CFG(i + 1), ret);
+}
+
+static int mcp9600_read_thresh(struct iio_dev *indio_dev,
+			       const struct iio_chan_spec *chan,
+			       enum iio_event_type type,
+			       enum iio_event_direction dir,
+			       enum iio_event_info info, int *val, int *val2)
+{
+	struct mcp9600_data *data = iio_priv(indio_dev);
+	struct i2c_client *client = data->client;
+	s32 ret;
+	int i;
+
+	i = mcp9600_get_alert_index(chan->channel, dir);
+	guard(mutex)(&data->lock);
+	ret = i2c_smbus_read_word_swapped(client, MCP9600_ALERT_LIMIT(i + 1));
+	if (ret < 0)
+		return ret;
+
+	/*
+	 * Temperature is stored in two’s complement format in bits(15:2),
+	 * LSB is 0.25 degree celsius.
+	 */
+	*val = sign_extend32(FIELD_GET(MCP9600_ALERT_LIMIT_MASK, ret), 13);
+	*val2 = 4;
+	if (info == IIO_EV_INFO_VALUE)
+		return IIO_VAL_FRACTIONAL;
+
+	ret = i2c_smbus_read_byte_data(client, MCP9600_ALERT_HYSTERESIS(i + 1));
+	if (ret < 0)
+		return ret;
+
+	/*
+	 * Hysteresis is stored as unsigned offset from threshold. The alert
+	 * direction bit in the alert configuration register defines whether the
+	 * value is below or above the corresponding threshold.
+	 */
+	if (dir == IIO_EV_DIR_RISING)
+		*val -= (*val2 * ret);
+	else
+		*val += (*val2 * ret);
+
+	return IIO_VAL_FRACTIONAL;
+}
+
+static int mcp9600_write_thresh(struct iio_dev *indio_dev,
+				const struct iio_chan_spec *chan,
+				enum iio_event_type type,
+				enum iio_event_direction dir,
+				enum iio_event_info info, int val, int val2)
+{
+	struct mcp9600_data *data = iio_priv(indio_dev);
+	struct i2c_client *client = data->client;
+	int s_val, s_thresh, i;
+	s16 thresh;
+	s32 ret;
+	u8 hyst;
+
+	/* Scale value to include decimal part into calculations */
+	s_val = (val < 0) ? ((val * MCP9600_TEMP_SCALE_NUM) - val2) :
+			    ((val * MCP9600_TEMP_SCALE_NUM) + val2);
+
+	/* Hot junction temperature range is from –200 to 1800 degree celsius */
+	if (chan->channel == MCP9600_CHAN_HOT_JUNCTION &&
+	   (s_val < (MCP9600_MIN_TEMP_HOT_JUNCTION * MCP9600_TEMP_SCALE_NUM) ||
+	    s_val > (MCP9600_MAX_TEMP_HOT_JUNCTION * MCP9600_TEMP_SCALE_NUM)))
+		return -EINVAL;
+
+	/* Cold junction temperature range is from –40 to 125 degree celsius */
+	if (chan->channel == MCP9600_CHAN_COLD_JUNCTION &&
+	   (s_val < (MCP9600_MIN_TEMP_COLD_JUNCTION * MCP9600_TEMP_SCALE_NUM) ||
+	    s_val > (MCP9600_MAX_TEMP_COLD_JUNCTION * MCP9600_TEMP_SCALE_NUM)))
+		return -EINVAL;
+
+	i = mcp9600_get_alert_index(chan->channel, dir);
+	guard(mutex)(&data->lock);
+	switch (info) {
+	case IIO_EV_INFO_VALUE:
+		/*
+		 * Shift length 4 bits = 2(15:2) + 2(0.25 LSB), temperature is
+		 * stored in two’s complement format.
+		 */
+		thresh = (s16)(s_val / (MCP9600_TEMP_SCALE_NUM >> 4));
+		return i2c_smbus_write_word_swapped(client,
+						    MCP9600_ALERT_LIMIT(i + 1),
+						    thresh);
+	case IIO_EV_INFO_HYSTERESIS:
+		/* Read out threshold, hysteresis is stored as offset */
+		ret = i2c_smbus_read_word_swapped(client, MCP9600_ALERT_LIMIT(i + 1));
+		if (ret < 0)
+			return ret;
+
+		/* Shift length 4 bits = 2(15:2) + 2(0.25 LSB), see above. */
+		s_thresh = sign_extend32(ret, 15) * (MCP9600_TEMP_SCALE_NUM >> 4);
+
+		/*
+		 * Hysteresis is stored as offset, for rising temperatures, the
+		 * hysteresis range is below the alert limit where, as for falling
+		 * temperatures, the hysteresis range is above the alert limit.
+		 */
+		hyst = min(255, abs(s_thresh - s_val) / MCP9600_TEMP_SCALE_NUM);
+
+		return i2c_smbus_write_byte_data(client,
+						 MCP9600_ALERT_HYSTERESIS(i + 1),
+						 hyst);
+	default:
+		return -EINVAL;
+	}
+}
+
 static const struct iio_info mcp9600_info = {
 	.read_raw = mcp9600_read_raw,
+	.read_event_config = mcp9600_read_event_config,
+	.write_event_config = mcp9600_write_event_config,
+	.read_event_value = mcp9600_read_thresh,
+	.write_event_value = mcp9600_write_thresh,
+};
+
+static irqreturn_t mcp9600_alert1_handler(int irq, void *private)
+{
+	struct iio_dev *indio_dev = private;
+	struct mcp9600_data *data = iio_priv(indio_dev);
+	int ret;
+
+	ret = i2c_smbus_read_byte_data(data->client, MCP9600_STATUS);
+	if (ret < 0)
+		return IRQ_HANDLED;
+
+	if (!(ret & MCP9600_STATUS_ALERT(MCP9600_ALERT1)))
+		return IRQ_NONE;
+
+	iio_push_event(indio_dev,
+		       IIO_MOD_EVENT_CODE(IIO_TEMP, MCP9600_CHAN_HOT_JUNCTION,
+					  IIO_NO_MOD, IIO_EV_TYPE_THRESH,
+					  IIO_EV_DIR_RISING),
+		       iio_get_time_ns(indio_dev));
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t mcp9600_alert2_handler(int irq, void *private)
+{
+	struct iio_dev *indio_dev = private;
+	struct mcp9600_data *data = iio_priv(indio_dev);
+	int ret;
+
+	ret = i2c_smbus_read_byte_data(data->client, MCP9600_STATUS);
+	if (ret < 0)
+		return IRQ_HANDLED;
+
+	if (!(ret & MCP9600_STATUS_ALERT(MCP9600_ALERT2)))
+		return IRQ_NONE;
+
+	iio_push_event(indio_dev,
+		       IIO_MOD_EVENT_CODE(IIO_TEMP, MCP9600_CHAN_HOT_JUNCTION,
+					  IIO_NO_MOD, IIO_EV_TYPE_THRESH,
+					  IIO_EV_DIR_FALLING),
+		       iio_get_time_ns(indio_dev));
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t mcp9600_alert3_handler(int irq, void *private)
+{
+	struct iio_dev *indio_dev = private;
+	struct mcp9600_data *data = iio_priv(indio_dev);
+	int ret;
+
+	ret = i2c_smbus_read_byte_data(data->client, MCP9600_STATUS);
+	if (ret < 0)
+		return IRQ_HANDLED;
+
+	if (!(ret & MCP9600_STATUS_ALERT(MCP9600_ALERT3)))
+		return IRQ_NONE;
+
+	iio_push_event(indio_dev,
+		       IIO_MOD_EVENT_CODE(IIO_TEMP, MCP9600_CHAN_COLD_JUNCTION,
+					  IIO_NO_MOD, IIO_EV_TYPE_THRESH,
+					  IIO_EV_DIR_RISING),
+		       iio_get_time_ns(indio_dev));
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t mcp9600_alert4_handler(int irq, void *private)
+{
+	struct iio_dev *indio_dev = private;
+	struct mcp9600_data *data = iio_priv(indio_dev);
+	int ret;
+
+	ret = i2c_smbus_read_byte_data(data->client, MCP9600_STATUS);
+	if (ret < 0)
+		return IRQ_HANDLED;
+
+	if (!(ret & MCP9600_STATUS_ALERT(MCP9600_ALERT4)))
+		return IRQ_NONE;
+
+	iio_push_event(indio_dev,
+		       IIO_MOD_EVENT_CODE(IIO_TEMP, MCP9600_CHAN_COLD_JUNCTION,
+					  IIO_NO_MOD, IIO_EV_TYPE_THRESH,
+					  IIO_EV_DIR_FALLING),
+		       iio_get_time_ns(indio_dev));
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t (*mcp9600_alert_handler[MCP9600_ALERT_COUNT]) (int, void *) = {
+	mcp9600_alert1_handler,
+	mcp9600_alert2_handler,
+	mcp9600_alert3_handler,
+	mcp9600_alert4_handler,
 };
 
+static int mcp9600_probe_alerts(struct iio_dev *indio_dev)
+{
+	struct mcp9600_data *data = iio_priv(indio_dev);
+	struct i2c_client *client = data->client;
+	struct device *dev = &client->dev;
+	struct fwnode_handle *fwnode = dev_fwnode(dev);
+	unsigned int irq_type;
+	int ret, irq, i;
+	u8 val;
+
+	/*
+	 * alert1: hot junction, rising temperature
+	 * alert2: hot junction, falling temperature
+	 * alert3: cold junction, rising temperature
+	 * alert4: cold junction, falling temperature
+	 */
+	for (i = 0; i < MCP9600_ALERT_COUNT; i++) {
+		irq = fwnode_irq_get_byname(fwnode, mcp9600_alert_name[i]);
+		if (irq <= 0)
+			continue;
+
+		val = 0;
+		irq_type = irq_get_trigger_type(irq);
+		if (irq_type == IRQ_TYPE_EDGE_RISING)
+			val |= MCP9600_ALERT_CFG_ACTIVE_HIGH;
+
+		if (i == MCP9600_ALERT2 || i == MCP9600_ALERT4)
+			val |= MCP9600_ALERT_CFG_FALLING;
+
+		if (i == MCP9600_ALERT3 || i == MCP9600_ALERT4)
+			val |= MCP9600_ALERT_CFG_COLD_JUNCTION;
+
+		ret = i2c_smbus_write_byte_data(client,
+						MCP9600_ALERT_CFG(i + 1),
+						val);
+		if (ret < 0)
+			return ret;
+
+		ret = devm_request_threaded_irq(dev, irq, NULL,
+						mcp9600_alert_handler[i],
+						IRQF_ONESHOT, "mcp9600",
+						indio_dev);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 static int mcp9600_probe(struct i2c_client *client)
 {
 	struct iio_dev *indio_dev;
@@ -107,6 +490,11 @@ static int mcp9600_probe(struct i2c_client *client)
 
 	data = iio_priv(indio_dev);
 	data->client = client;
+	mutex_init(&data->lock);
+
+	ret = mcp9600_probe_alerts(indio_dev);
+	if (ret)
+		return ret;
 
 	indio_dev->info = &mcp9600_info;
 	indio_dev->name = "mcp9600";
@@ -139,6 +527,7 @@ static struct i2c_driver mcp9600_driver = {
 };
 module_i2c_driver(mcp9600_driver);
 
+MODULE_AUTHOR("Dimitri Fedrau <dima.fedrau@gmail.com>");
 MODULE_AUTHOR("Andrew Hepp <andrew.hepp@ahepp.dev>");
 MODULE_DESCRIPTION("Microchip MCP9600 thermocouple EMF converter driver");
 MODULE_LICENSE("GPL");
-- 
2.39.2

Re: [PATCH v2 2/2] iio: temperature: mcp9600: add threshold events support

[Jonathan Cameron]

On Fri, 17 May 2024 10:10:50 +0200
Dimitri Fedrau <dima.fedrau@gmail.com> wrote:

> The device has four programmable temperature alert outputs which can be
> used to monitor hot or cold-junction temperatures and detect falling and
> rising temperatures. It supports up to 255 degree celsius programmable
> hysteresis. Each alert can be individually configured by setting following
> options in the associated alert configuration register:
> - monitor hot or cold junction temperature
> - monitor rising or falling temperature
> - set comparator or interrupt mode
> - set output polarity
> - enable alert
> 
> This patch binds alert outputs to iio events:
> - alert1: hot junction, rising temperature
> - alert2: hot junction, falling temperature
> - alert3: cold junction, rising temperature
> - alert4: cold junction, falling temperature
> 
> All outputs are set in comparator mode and polarity depends on interrupt
> configuration.
> 
> Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com>
Hi Dmitri
Please make sure to address all questions in earlier reviews, either by
changing the code, or directly answering the question.

The hysteresis handling in here is completely different from normal
and the diagrams in figure 5-10 suggest it should not be.

Your thresholds should not include hysteresis at all as it has nothing to
do with event triggering. The hysteresis is presented to userspace so it
knows when a 'reset' of event detection logic occurs. It is expressed
as an offset (in the obvious direction) from the current threshold.

It is always positive as negative hysteresis would be very odd. It's just
magnitude of how far back beyond the threshold the signal must go for
a reset of the signal detection logic to occur, allowing new transitions etc.

As long as you are using an edge interrupt that just means you won't get
another interrupt until getting well away from what triggered the interrupt
last time.

Jonathan

> ---
>  drivers/iio/temperature/mcp9600.c | 389 ++++++++++++++++++++++++++++++
>  1 file changed, 389 insertions(+)
> 
> diff --git a/drivers/iio/temperature/mcp9600.c b/drivers/iio/temperature/mcp9600.c
> index 22451d1d9e1f..91d811fe9371 100644
> --- a/drivers/iio/temperature/mcp9600.c
> +++ b/drivers/iio/temperature/mcp9600.c
> @@ -6,12 +6,21 @@
>   * Author: <andrew.hepp@ahepp.dev>
>   */
>  
> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>
> +#include <linux/bits.h>
>  #include <linux/err.h>
>  #include <linux/i2c.h>
>  #include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/math.h>
> +#include <linux/minmax.h>
>  #include <linux/mod_devicetable.h>
>  #include <linux/module.h>
> +#include <linux/mutex.h>
>  
> +#include <linux/iio/events.h>
>  #include <linux/iio/iio.h>
>  
>  #define MCP9600_CHAN_HOT_JUNCTION	0
> @@ -20,11 +29,65 @@
>  /* MCP9600 registers */
>  #define MCP9600_HOT_JUNCTION 0x0
>  #define MCP9600_COLD_JUNCTION 0x2
> +#define MCP9600_STATUS			0x4
> +#define MCP9600_STATUS_ALERT(x)		BIT(x)
> +#define MCP9600_ALERT_CFG1		0x8
> +#define MCP9600_ALERT_CFG(x)		(MCP9600_ALERT_CFG1 + (x - 1))
> +#define MCP9600_ALERT_CFG_ENABLE	BIT(0)
> +#define MCP9600_ALERT_CFG_ACTIVE_HIGH	BIT(2)
> +#define MCP9600_ALERT_CFG_FALLING	BIT(3)
> +#define MCP9600_ALERT_CFG_COLD_JUNCTION	BIT(4)
> +#define MCP9600_ALERT_HYSTERESIS1	0xc
> +#define MCP9600_ALERT_HYSTERESIS(x)	(MCP9600_ALERT_HYSTERESIS1 + (x - 1))
> +#define MCP9600_ALERT_LIMIT1		0x10
> +#define MCP9600_ALERT_LIMIT(x)		(MCP9600_ALERT_LIMIT1 + (x - 1))
> +#define MCP9600_ALERT_LIMIT_MASK	GENMASK(15, 2)
>  #define MCP9600_DEVICE_ID 0x20
>  
>  /* MCP9600 device id value */
>  #define MCP9600_DEVICE_ID_MCP9600 0x40
>  
> +#define MCP9600_ALERT_COUNT		4
> +
> +#define MCP9600_TEMP_SCALE_NUM		1000000

MICRO or just use that inline.

> +
> +#define MCP9600_MIN_TEMP_HOT_JUNCTION	-200
> +#define MCP9600_MAX_TEMP_HOT_JUNCTION	1800
Give these units in the naming and  you can include the * MICRO here.
e.g.
#define MCP9600_MIN_TEMP_HOT_JUNC_MICROCELCIUS -200 * MICRO
etc


> +
> +static int mcp9600_read_thresh(struct iio_dev *indio_dev,
> +			       const struct iio_chan_spec *chan,
> +			       enum iio_event_type type,
> +			       enum iio_event_direction dir,
> +			       enum iio_event_info info, int *val, int *val2)
> +{
> +	struct mcp9600_data *data = iio_priv(indio_dev);
> +	struct i2c_client *client = data->client;
> +	s32 ret;
> +	int i;
> +
> +	i = mcp9600_get_alert_index(chan->channel, dir);
> +	guard(mutex)(&data->lock);
> +	ret = i2c_smbus_read_word_swapped(client, MCP9600_ALERT_LIMIT(i + 1));
> +	if (ret < 0)
> +		return ret;
> +
> +	/*
> +	 * Temperature is stored in two’s complement format in bits(15:2),
> +	 * LSB is 0.25 degree celsius.
> +	 */
> +	*val = sign_extend32(FIELD_GET(MCP9600_ALERT_LIMIT_MASK, ret), 13);
> +	*val2 = 4;
> +	if (info == IIO_EV_INFO_VALUE)
> +		return IIO_VAL_FRACTIONAL;
> +
> +	ret = i2c_smbus_read_byte_data(client, MCP9600_ALERT_HYSTERESIS(i + 1));
> +	if (ret < 0)
> +		return ret;
> +
> +	/*
> +	 * Hysteresis is stored as unsigned offset from threshold. The alert
> +	 * direction bit in the alert configuration register defines whether the
> +	 * value is below or above the corresponding threshold.

I'm still very very confused by this.  I raised a question in the first review
and you didn't provide more information.
This is not how hysteresis is normally defined.  It should not affect the
threshold at all, but instead affect when a reset occurs of the threshold detection
logic. It also does not correspond to the diagrams in Figure 5-10 which look
exactly like normal hysteresis controls.


> +	 */
> +	if (dir == IIO_EV_DIR_RISING)
> +		*val -= (*val2 * ret);
> +	else
> +		*val += (*val2 * ret);
> +
> +	return IIO_VAL_FRACTIONAL;
> +}
> +
> +static int mcp9600_write_thresh(struct iio_dev *indio_dev,
> +				const struct iio_chan_spec *chan,
> +				enum iio_event_type type,
> +				enum iio_event_direction dir,
> +				enum iio_event_info info, int val, int val2)
> +{
> +	struct mcp9600_data *data = iio_priv(indio_dev);
> +	struct i2c_client *client = data->client;
> +	int s_val, s_thresh, i;
> +	s16 thresh;
> +	s32 ret;
> +	u8 hyst;
> +
> +	/* Scale value to include decimal part into calculations */
> +	s_val = (val < 0) ? ((val * MCP9600_TEMP_SCALE_NUM) - val2) :
> +			    ((val * MCP9600_TEMP_SCALE_NUM) + val2);
> +
> +	/* Hot junction temperature range is from –200 to 1800 degree celsius */
> +	if (chan->channel == MCP9600_CHAN_HOT_JUNCTION &&
> +	   (s_val < (MCP9600_MIN_TEMP_HOT_JUNCTION * MCP9600_TEMP_SCALE_NUM) ||
> +	    s_val > (MCP9600_MAX_TEMP_HOT_JUNCTION * MCP9600_TEMP_SCALE_NUM)))

As above, change the units of the defines to simplify this or perhaps
just treat these as numbers and put them here rather than using defines at all.

> +		return -EINVAL;
> +
> +	/* Cold junction temperature range is from –40 to 125 degree celsius */
> +	if (chan->channel == MCP9600_CHAN_COLD_JUNCTION &&
> +	   (s_val < (MCP9600_MIN_TEMP_COLD_JUNCTION * MCP9600_TEMP_SCALE_NUM) ||
> +	    s_val > (MCP9600_MAX_TEMP_COLD_JUNCTION * MCP9600_TEMP_SCALE_NUM)))
> +		return -EINVAL;
> +
> +	i = mcp9600_get_alert_index(chan->channel, dir);
> +	guard(mutex)(&data->lock);
> +	switch (info) {
> +	case IIO_EV_INFO_VALUE:
> +		/*
> +		 * Shift length 4 bits = 2(15:2) + 2(0.25 LSB), temperature is
> +		 * stored in two’s complement format.
> +		 */
> +		thresh = (s16)(s_val / (MCP9600_TEMP_SCALE_NUM >> 4));
> +		return i2c_smbus_write_word_swapped(client,
> +						    MCP9600_ALERT_LIMIT(i + 1),
> +						    thresh);
> +	case IIO_EV_INFO_HYSTERESIS:
> +		/* Read out threshold, hysteresis is stored as offset */
> +		ret = i2c_smbus_read_word_swapped(client, MCP9600_ALERT_LIMIT(i + 1));
> +		if (ret < 0)
> +			return ret;
> +
> +		/* Shift length 4 bits = 2(15:2) + 2(0.25 LSB), see above. */
> +		s_thresh = sign_extend32(ret, 15) * (MCP9600_TEMP_SCALE_NUM >> 4);
> +
> +		/*
> +		 * Hysteresis is stored as offset, for rising temperatures, the
> +		 * hysteresis range is below the alert limit where, as for falling
> +		 * temperatures, the hysteresis range is above the alert limit.
> +		 */

I don't understand this comment.
Hysteresis as a parameter in sysfs in IIO is also an offset, so why is the current
threshold relevant?

Normally hysteresis is about allowing repeat events. I.e. you have to drop below
threshold - hysteresis  before rising again to trigger a rising event when passing
threshold.


> +		hyst = min(255, abs(s_thresh - s_val) / MCP9600_TEMP_SCALE_NUM);
> +
> +		return i2c_smbus_write_byte_data(client,
> +						 MCP9600_ALERT_HYSTERESIS(i + 1),
> +						 hyst);
> +	default:
> +		return -EINVAL;
> +	}
> +}


> +static irqreturn_t mcp9600_alert3_handler(int irq, void *private)
> +{
> +	struct iio_dev *indio_dev = private;
> +	struct mcp9600_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = i2c_smbus_read_byte_data(data->client, MCP9600_STATUS);
> +	if (ret < 0)
> +		return IRQ_HANDLED;
> +
> +	if (!(ret & MCP9600_STATUS_ALERT(MCP9600_ALERT3)))

This duplicates far too much all these call a function that takes
a) This bit,
b) the related channel index.
C) the direction

and call that from all these separate handlers.
Each individual handler become simply:

	return mcp9600_alert_handler(private, MCP9600_ALERT3,
				     MCP9600_CHAN_COLD_JUNCTION,
				     IIO_EV_DIR_RISING);

etc.

> +		return IRQ_NONE;
> +
> +	iio_push_event(indio_dev,
> +		       IIO_MOD_EVENT_CODE(IIO_TEMP, MCP9600_CHAN_COLD_JUNCTION,
> +					  IIO_NO_MOD, IIO_EV_TYPE_THRESH,
> +					  IIO_EV_DIR_RISING),
> +		       iio_get_time_ns(indio_dev));
> +
> +	return IRQ_HANDLED;
> +}
> +

> +
> +static irqreturn_t (*mcp9600_alert_handler[MCP9600_ALERT_COUNT]) (int, void *) = {
> +	mcp9600_alert1_handler,
> +	mcp9600_alert2_handler,
> +	mcp9600_alert3_handler,
> +	mcp9600_alert4_handler,
>  };

Re: [PATCH v2 2/2] iio: temperature: mcp9600: add threshold events support

[Dimitri Fedrau]

Am Sun, May 19, 2024 at 05:42:48PM +0100 schrieb Jonathan Cameron:
> On Fri, 17 May 2024 10:10:50 +0200
> Dimitri Fedrau <dima.fedrau@gmail.com> wrote:
> 
> > The device has four programmable temperature alert outputs which can be
> > used to monitor hot or cold-junction temperatures and detect falling and
> > rising temperatures. It supports up to 255 degree celsius programmable
> > hysteresis. Each alert can be individually configured by setting following
> > options in the associated alert configuration register:
> > - monitor hot or cold junction temperature
> > - monitor rising or falling temperature
> > - set comparator or interrupt mode
> > - set output polarity
> > - enable alert
> > 
> > This patch binds alert outputs to iio events:
> > - alert1: hot junction, rising temperature
> > - alert2: hot junction, falling temperature
> > - alert3: cold junction, rising temperature
> > - alert4: cold junction, falling temperature
> > 
> > All outputs are set in comparator mode and polarity depends on interrupt
> > configuration.
> > 
> > Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com>
> Hi Dmitri

Hi Jonathan,

> Please make sure to address all questions in earlier reviews, either by
> changing the code, or directly answering the question.
>
I did, see: https://lore.kernel.org/linux-iio/20240509204559.GB3614@debian/T/#u
or did I miss anything ? I'm a little bit confused.

> The hysteresis handling in here is completely different from normal
> and the diagrams in figure 5-10 suggest it should not be.
> 
> Your thresholds should not include hysteresis at all as it has nothing to
> do with event triggering. The hysteresis is presented to userspace so it
> knows when a 'reset' of event detection logic occurs. It is expressed
> as an offset (in the obvious direction) from the current threshold.
> 
> It is always positive as negative hysteresis would be very odd. It's just
> magnitude of how far back beyond the threshold the signal must go for
> a reset of the signal detection logic to occur, allowing new transitions etc.
> 
> As long as you are using an edge interrupt that just means you won't get
> another interrupt until getting well away from what triggered the interrupt
> last time.
> 
You are right. Thanks a lot for your explanation. I just didn't know it,
assumed the hysteresis is represented same way as the threshold.

> Jonathan
> 
> > ---
> >  drivers/iio/temperature/mcp9600.c | 389 ++++++++++++++++++++++++++++++
> >  1 file changed, 389 insertions(+)
> > 
> > diff --git a/drivers/iio/temperature/mcp9600.c b/drivers/iio/temperature/mcp9600.c
> > index 22451d1d9e1f..91d811fe9371 100644
> > --- a/drivers/iio/temperature/mcp9600.c
> > +++ b/drivers/iio/temperature/mcp9600.c
> > @@ -6,12 +6,21 @@
> >   * Author: <andrew.hepp@ahepp.dev>
> >   */
> >  
> > +#include <linux/bitfield.h>
> > +#include <linux/bitops.h>
> > +#include <linux/bits.h>
> >  #include <linux/err.h>
> >  #include <linux/i2c.h>
> >  #include <linux/init.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/irq.h>
> > +#include <linux/math.h>
> > +#include <linux/minmax.h>
> >  #include <linux/mod_devicetable.h>
> >  #include <linux/module.h>
> > +#include <linux/mutex.h>
> >  
> > +#include <linux/iio/events.h>
> >  #include <linux/iio/iio.h>
> >  
> >  #define MCP9600_CHAN_HOT_JUNCTION	0
> > @@ -20,11 +29,65 @@
> >  /* MCP9600 registers */
> >  #define MCP9600_HOT_JUNCTION 0x0
> >  #define MCP9600_COLD_JUNCTION 0x2
> > +#define MCP9600_STATUS			0x4
> > +#define MCP9600_STATUS_ALERT(x)		BIT(x)
> > +#define MCP9600_ALERT_CFG1		0x8
> > +#define MCP9600_ALERT_CFG(x)		(MCP9600_ALERT_CFG1 + (x - 1))
> > +#define MCP9600_ALERT_CFG_ENABLE	BIT(0)
> > +#define MCP9600_ALERT_CFG_ACTIVE_HIGH	BIT(2)
> > +#define MCP9600_ALERT_CFG_FALLING	BIT(3)
> > +#define MCP9600_ALERT_CFG_COLD_JUNCTION	BIT(4)
> > +#define MCP9600_ALERT_HYSTERESIS1	0xc
> > +#define MCP9600_ALERT_HYSTERESIS(x)	(MCP9600_ALERT_HYSTERESIS1 + (x - 1))
> > +#define MCP9600_ALERT_LIMIT1		0x10
> > +#define MCP9600_ALERT_LIMIT(x)		(MCP9600_ALERT_LIMIT1 + (x - 1))
> > +#define MCP9600_ALERT_LIMIT_MASK	GENMASK(15, 2)
> >  #define MCP9600_DEVICE_ID 0x20
> >  
> >  /* MCP9600 device id value */
> >  #define MCP9600_DEVICE_ID_MCP9600 0x40
> >  
> > +#define MCP9600_ALERT_COUNT		4
> > +
> > +#define MCP9600_TEMP_SCALE_NUM		1000000
> 
> MICRO or just use that inline.
> 
> > +
> > +#define MCP9600_MIN_TEMP_HOT_JUNCTION	-200
> > +#define MCP9600_MAX_TEMP_HOT_JUNCTION	1800
> Give these units in the naming and  you can include the * MICRO here.
> e.g.
> #define MCP9600_MIN_TEMP_HOT_JUNC_MICROCELCIUS -200 * MICRO
> etc
> 
Ok.

> 
> > +
> > +static int mcp9600_read_thresh(struct iio_dev *indio_dev,
> > +			       const struct iio_chan_spec *chan,
> > +			       enum iio_event_type type,
> > +			       enum iio_event_direction dir,
> > +			       enum iio_event_info info, int *val, int *val2)
> > +{
> > +	struct mcp9600_data *data = iio_priv(indio_dev);
> > +	struct i2c_client *client = data->client;
> > +	s32 ret;
> > +	int i;
> > +
> > +	i = mcp9600_get_alert_index(chan->channel, dir);
> > +	guard(mutex)(&data->lock);
> > +	ret = i2c_smbus_read_word_swapped(client, MCP9600_ALERT_LIMIT(i + 1));
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	/*
> > +	 * Temperature is stored in two’s complement format in bits(15:2),
> > +	 * LSB is 0.25 degree celsius.
> > +	 */
> > +	*val = sign_extend32(FIELD_GET(MCP9600_ALERT_LIMIT_MASK, ret), 13);
> > +	*val2 = 4;
> > +	if (info == IIO_EV_INFO_VALUE)
> > +		return IIO_VAL_FRACTIONAL;
> > +
> > +	ret = i2c_smbus_read_byte_data(client, MCP9600_ALERT_HYSTERESIS(i + 1));
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	/*
> > +	 * Hysteresis is stored as unsigned offset from threshold. The alert
> > +	 * direction bit in the alert configuration register defines whether the
> > +	 * value is below or above the corresponding threshold.
> 
> I'm still very very confused by this.  I raised a question in the first review
> and you didn't provide more information.
> This is not how hysteresis is normally defined.  It should not affect the
> threshold at all, but instead affect when a reset occurs of the threshold detection
> logic. It also does not correspond to the diagrams in Figure 5-10 which look
> exactly like normal hysteresis controls.
> 
You are right. I got it wrong here. After your explanation and having a
look at https://www.kernel.org/doc/Documentation/ABI/testing/sysfs-bus-iio
it should be easy to fix this. Thanks again.

> 
> > +	 */
> > +	if (dir == IIO_EV_DIR_RISING)
> > +		*val -= (*val2 * ret);
> > +	else
> > +		*val += (*val2 * ret);
> > +
> > +	return IIO_VAL_FRACTIONAL;
> > +}
> > +
> > +static int mcp9600_write_thresh(struct iio_dev *indio_dev,
> > +				const struct iio_chan_spec *chan,
> > +				enum iio_event_type type,
> > +				enum iio_event_direction dir,
> > +				enum iio_event_info info, int val, int val2)
> > +{
> > +	struct mcp9600_data *data = iio_priv(indio_dev);
> > +	struct i2c_client *client = data->client;
> > +	int s_val, s_thresh, i;
> > +	s16 thresh;
> > +	s32 ret;
> > +	u8 hyst;
> > +
> > +	/* Scale value to include decimal part into calculations */
> > +	s_val = (val < 0) ? ((val * MCP9600_TEMP_SCALE_NUM) - val2) :
> > +			    ((val * MCP9600_TEMP_SCALE_NUM) + val2);
> > +
> > +	/* Hot junction temperature range is from –200 to 1800 degree celsius */
> > +	if (chan->channel == MCP9600_CHAN_HOT_JUNCTION &&
> > +	   (s_val < (MCP9600_MIN_TEMP_HOT_JUNCTION * MCP9600_TEMP_SCALE_NUM) ||
> > +	    s_val > (MCP9600_MAX_TEMP_HOT_JUNCTION * MCP9600_TEMP_SCALE_NUM)))
> 
> As above, change the units of the defines to simplify this or perhaps
> just treat these as numbers and put them here rather than using defines at all.
> 
Ok.

> > +		return -EINVAL;
> > +
> > +	/* Cold junction temperature range is from –40 to 125 degree celsius */
> > +	if (chan->channel == MCP9600_CHAN_COLD_JUNCTION &&
> > +	   (s_val < (MCP9600_MIN_TEMP_COLD_JUNCTION * MCP9600_TEMP_SCALE_NUM) ||
> > +	    s_val > (MCP9600_MAX_TEMP_COLD_JUNCTION * MCP9600_TEMP_SCALE_NUM)))
> > +		return -EINVAL;
> > +
> > +	i = mcp9600_get_alert_index(chan->channel, dir);
> > +	guard(mutex)(&data->lock);
> > +	switch (info) {
> > +	case IIO_EV_INFO_VALUE:
> > +		/*
> > +		 * Shift length 4 bits = 2(15:2) + 2(0.25 LSB), temperature is
> > +		 * stored in two’s complement format.
> > +		 */
> > +		thresh = (s16)(s_val / (MCP9600_TEMP_SCALE_NUM >> 4));
> > +		return i2c_smbus_write_word_swapped(client,
> > +						    MCP9600_ALERT_LIMIT(i + 1),
> > +						    thresh);
> > +	case IIO_EV_INFO_HYSTERESIS:
> > +		/* Read out threshold, hysteresis is stored as offset */
> > +		ret = i2c_smbus_read_word_swapped(client, MCP9600_ALERT_LIMIT(i + 1));
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		/* Shift length 4 bits = 2(15:2) + 2(0.25 LSB), see above. */
> > +		s_thresh = sign_extend32(ret, 15) * (MCP9600_TEMP_SCALE_NUM >> 4);
> > +
> > +		/*
> > +		 * Hysteresis is stored as offset, for rising temperatures, the
> > +		 * hysteresis range is below the alert limit where, as for falling
> > +		 * temperatures, the hysteresis range is above the alert limit.
> > +		 */
> 
> I don't understand this comment.
> Hysteresis as a parameter in sysfs in IIO is also an offset, so why is the current
> threshold relevant?
> 
> Normally hysteresis is about allowing repeat events. I.e. you have to drop below
> threshold - hysteresis  before rising again to trigger a rising event when passing
> threshold.
> 
As above, I just didn't know better.

> 
> > +		hyst = min(255, abs(s_thresh - s_val) / MCP9600_TEMP_SCALE_NUM);
> > +
> > +		return i2c_smbus_write_byte_data(client,
> > +						 MCP9600_ALERT_HYSTERESIS(i + 1),
> > +						 hyst);
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> 
> 
> > +static irqreturn_t mcp9600_alert3_handler(int irq, void *private)
> > +{
> > +	struct iio_dev *indio_dev = private;
> > +	struct mcp9600_data *data = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	ret = i2c_smbus_read_byte_data(data->client, MCP9600_STATUS);
> > +	if (ret < 0)
> > +		return IRQ_HANDLED;
> > +
> > +	if (!(ret & MCP9600_STATUS_ALERT(MCP9600_ALERT3)))
> 
> This duplicates far too much all these call a function that takes
> a) This bit,
> b) the related channel index.
> C) the direction
> 
> and call that from all these separate handlers.
> Each individual handler become simply:
> 
> 	return mcp9600_alert_handler(private, MCP9600_ALERT3,
> 				     MCP9600_CHAN_COLD_JUNCTION,
> 				     IIO_EV_DIR_RISING);
> 
> etc.
>
Yes, will use a helper function to avoid code duplication.

> > +		return IRQ_NONE;
> > +
> > +	iio_push_event(indio_dev,
> > +		       IIO_MOD_EVENT_CODE(IIO_TEMP, MCP9600_CHAN_COLD_JUNCTION,
> > +					  IIO_NO_MOD, IIO_EV_TYPE_THRESH,
> > +					  IIO_EV_DIR_RISING),
> > +		       iio_get_time_ns(indio_dev));
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> 
> > +
> > +static irqreturn_t (*mcp9600_alert_handler[MCP9600_ALERT_COUNT]) (int, void *) = {
> > +	mcp9600_alert1_handler,
> > +	mcp9600_alert2_handler,
> > +	mcp9600_alert3_handler,
> > +	mcp9600_alert4_handler,
> >  };
>

Dimitri

Re: [PATCH v2 2/2] iio: temperature: mcp9600: add threshold events support

[Jonathan Cameron]

On Sun, 19 May 2024 23:00:36 +0200
Dimitri Fedrau <dima.fedrau@gmail.com> wrote:

> Am Sun, May 19, 2024 at 05:42:48PM +0100 schrieb Jonathan Cameron:
> > On Fri, 17 May 2024 10:10:50 +0200
> > Dimitri Fedrau <dima.fedrau@gmail.com> wrote:
> >   
> > > The device has four programmable temperature alert outputs which can be
> > > used to monitor hot or cold-junction temperatures and detect falling and
> > > rising temperatures. It supports up to 255 degree celsius programmable
> > > hysteresis. Each alert can be individually configured by setting following
> > > options in the associated alert configuration register:
> > > - monitor hot or cold junction temperature
> > > - monitor rising or falling temperature
> > > - set comparator or interrupt mode
> > > - set output polarity
> > > - enable alert
> > > 
> > > This patch binds alert outputs to iio events:
> > > - alert1: hot junction, rising temperature
> > > - alert2: hot junction, falling temperature
> > > - alert3: cold junction, rising temperature
> > > - alert4: cold junction, falling temperature
> > > 
> > > All outputs are set in comparator mode and polarity depends on interrupt
> > > configuration.
> > > 
> > > Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com>  
> > Hi Dmitri  
> 
> Hi Jonathan,
> 
> > Please make sure to address all questions in earlier reviews, either by
> > changing the code, or directly answering the question.
> >  
> I did, see: https://lore.kernel.org/linux-iio/20240509204559.GB3614@debian/T/#u
> or did I miss anything ? I'm a little bit confused.

I think some emails went astray :( Sorry I didn't check the archive.

Anyhow, thanks for providing the links.

Jonathan

Re: [PATCH v2 2/2] iio: temperature: mcp9600: add threshold events support

[Dimitri Fedrau]

Am Mon, May 20, 2024 at 01:18:50PM +0100 schrieb Jonathan Cameron:
> On Sun, 19 May 2024 23:00:36 +0200
> Dimitri Fedrau <dima.fedrau@gmail.com> wrote:
> 
> > Am Sun, May 19, 2024 at 05:42:48PM +0100 schrieb Jonathan Cameron:
> > > On Fri, 17 May 2024 10:10:50 +0200
> > > Dimitri Fedrau <dima.fedrau@gmail.com> wrote:
> > >   
> > > > The device has four programmable temperature alert outputs which can be
> > > > used to monitor hot or cold-junction temperatures and detect falling and
> > > > rising temperatures. It supports up to 255 degree celsius programmable
> > > > hysteresis. Each alert can be individually configured by setting following
> > > > options in the associated alert configuration register:
> > > > - monitor hot or cold junction temperature
> > > > - monitor rising or falling temperature
> > > > - set comparator or interrupt mode
> > > > - set output polarity
> > > > - enable alert
> > > > 
> > > > This patch binds alert outputs to iio events:
> > > > - alert1: hot junction, rising temperature
> > > > - alert2: hot junction, falling temperature
> > > > - alert3: cold junction, rising temperature
> > > > - alert4: cold junction, falling temperature
> > > > 
> > > > All outputs are set in comparator mode and polarity depends on interrupt
> > > > configuration.
> > > > 
> > > > Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com>  
> > > Hi Dmitri  
> > 
> > Hi Jonathan,
> > 
> > > Please make sure to address all questions in earlier reviews, either by
> > > changing the code, or directly answering the question.
> > >  
> > I did, see: https://lore.kernel.org/linux-iio/20240509204559.GB3614@debian/T/#u
> > or did I miss anything ? I'm a little bit confused.
> 
> I think some emails went astray :( Sorry I didn't check the archive.
> 
> Anyhow, thanks for providing the links.
>
Just thought you were to busy to reply and that's because I had send a new
version of the series.

> Jonathan
> 

Re: [PATCH v2 0/2] Add threshold events support

[Jonathan Cameron]

On Fri, 17 May 2024 10:10:48 +0200
Dimitri Fedrau <dima.fedrau@gmail.com> wrote:

> Add patch for providing index for both channels and add patch for threshold
> events support for the MCP9600 device.

Always mention subsystem and device in the cover letter title!

iio: temperature mcp9600: Add threshold events support.

Makes it easier for people to judge if they want to read a thread or not.

No need to resend for this though.
> 
> Changes in V2:
>   - Remove pretty printing patches from series
>   - Add patch for providing index for both channels(ABI change)!
>     Suggested by Jonathan, hope this okay.
>   - Remove formatting in a precursor patch
>   - Add lock documentation
>   - Add define MCP9600_TEMP_SCALE_NUM and use it instead of MICRO. MICRO is
>     type unsigned long which we have to cast to int when using
>     multiplication or division, because we are handling negative values.
>   - Use switch statement in mcp9600_write_thresh
>   - Replaced generic interrupt handler with four separate interrupt handler
>   - Use one lock instead of four
>   - Added error check for mcp9600_probe_alerts
>      
> 
> Dimitri Fedrau (2):
>   iio: temperature: mcp9600: Provide index for both channels
>   iio: temperature: mcp9600: add threshold events support
> 
>  drivers/iio/temperature/mcp9600.c | 398 +++++++++++++++++++++++++++++-
>  1 file changed, 396 insertions(+), 2 deletions(-)
>