[PATCH 0/5] Add threshold events support and some minor cleanup

[PATCH 0/5] Add threshold events support and some minor cleanup

[Dimitri Fedrau]

This patch adds threshold events support for the MCP9600 device.

Based on fix:
3c324a40b7c3 iio: temperature: mcp9600: Fix temperature reading for negative values

Dimitri Fedrau (5):
  iio: temperature: mcp9600: set channel2 member
  iio: temperature: mcp9600: Share scale by all channels
  iio: temperature: mcp9600: add newlines after if statements
  iio: temperature: mcp9600: Fix line exceeding 80 columns
  iio: temperature: mcp9600: add threshold events support

 drivers/iio/temperature/mcp9600.c | 381 +++++++++++++++++++++++++++++-
 1 file changed, 368 insertions(+), 13 deletions(-)

-- 
2.39.2

[PATCH 1/5] iio: temperature: mcp9600: set channel2 member

[Dimitri Fedrau]

Set channel2 member of channel 0 to IIO_MOD_TEMP_OBJECT and set modified
member to 1.

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

diff --git a/drivers/iio/temperature/mcp9600.c b/drivers/iio/temperature/mcp9600.c
index 7a3eef5d5e75..e277edb4ae4b 100644
--- a/drivers/iio/temperature/mcp9600.c
+++ b/drivers/iio/temperature/mcp9600.c
@@ -26,6 +26,8 @@ static const struct iio_chan_spec mcp9600_channels[] = {
 	{
 		.type = IIO_TEMP,
 		.address = MCP9600_HOT_JUNCTION,
+		.channel2 = IIO_MOD_TEMP_OBJECT,
+		.modified = 1,
 		.info_mask_separate =
 			BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
 	},
-- 
2.39.2

Re: [PATCH 1/5] iio: temperature: mcp9600: set channel2 member

[Jonathan Cameron]

On Tue, 30 Apr 2024 14:05:31 +0200
Dimitri Fedrau <dima.fedrau@gmail.com> wrote:

> Set channel2 member of channel 0 to IIO_MOD_TEMP_OBJECT and set modified
> member to 1.
This an ABI change, so needs a strong argument + must be a fix 
rather than an improvement.  So why does this need to change?

Jonathan

> 
> Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com>
> ---
>  drivers/iio/temperature/mcp9600.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/iio/temperature/mcp9600.c b/drivers/iio/temperature/mcp9600.c
> index 7a3eef5d5e75..e277edb4ae4b 100644
> --- a/drivers/iio/temperature/mcp9600.c
> +++ b/drivers/iio/temperature/mcp9600.c
> @@ -26,6 +26,8 @@ static const struct iio_chan_spec mcp9600_channels[] = {
>  	{
>  		.type = IIO_TEMP,
>  		.address = MCP9600_HOT_JUNCTION,
> +		.channel2 = IIO_MOD_TEMP_OBJECT,
> +		.modified = 1,
>  		.info_mask_separate =
>  			BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
>  	},

Re: [PATCH 1/5] iio: temperature: mcp9600: set channel2 member

[Dimitri Fedrau]

Am Tue, Apr 30, 2024 at 01:11:02PM +0100 schrieb Jonathan Cameron:
> On Tue, 30 Apr 2024 14:05:31 +0200
> Dimitri Fedrau <dima.fedrau@gmail.com> wrote:
> 
> > Set channel2 member of channel 0 to IIO_MOD_TEMP_OBJECT and set modified
> > member to 1.
> This an ABI change, so needs a strong argument + must be a fix 
> rather than an improvement.  So why does this need to change?
>
Hi Jonathan,

I don't know if it is an valid argument but when using tool "iio_info"
the temp_object wasn't displayed at all. After adding these two lines
the temp_object is displayed. Don't know if it is a problem with the
userspace tools.

iio_info version: 0.25 (git tag:b6028fde)
Libiio version: 0.25 (git tag: b6028fd) backends: local xml ip usb serial

Besides that it eases distinction between the two channels in the last
patch, but I think this argument is not strong enough. :)

Best regards,
Dimitri

> 
> > 
> > Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com>
> > ---
> >  drivers/iio/temperature/mcp9600.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/iio/temperature/mcp9600.c b/drivers/iio/temperature/mcp9600.c
> > index 7a3eef5d5e75..e277edb4ae4b 100644
> > --- a/drivers/iio/temperature/mcp9600.c
> > +++ b/drivers/iio/temperature/mcp9600.c
> > @@ -26,6 +26,8 @@ static const struct iio_chan_spec mcp9600_channels[] = {
> >  	{
> >  		.type = IIO_TEMP,
> >  		.address = MCP9600_HOT_JUNCTION,
> > +		.channel2 = IIO_MOD_TEMP_OBJECT,
> > +		.modified = 1,
> >  		.info_mask_separate =
> >  			BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
> >  	},
> 

Re: [PATCH 1/5] iio: temperature: mcp9600: set channel2 member

[Jonathan Cameron]

On Tue, 30 Apr 2024 14:21:57 +0200
Dimitri Fedrau <dima.fedrau@gmail.com> wrote:

> Am Tue, Apr 30, 2024 at 01:11:02PM +0100 schrieb Jonathan Cameron:
> > On Tue, 30 Apr 2024 14:05:31 +0200
> > Dimitri Fedrau <dima.fedrau@gmail.com> wrote:
> >   
> > > Set channel2 member of channel 0 to IIO_MOD_TEMP_OBJECT and set modified
> > > member to 1.  
> > This an ABI change, so needs a strong argument + must be a fix 
> > rather than an improvement.  So why does this need to change?
> >  
> Hi Jonathan,
> 
> I don't know if it is an valid argument but when using tool "iio_info"
> the temp_object wasn't displayed at all. After adding these two lines
> the temp_object is displayed. Don't know if it is a problem with the
> userspace tools.

Just to check, it displayed not temperature channel for this?

If you could send the file listing of the appropriate
/sys/bus/iio/devices/iio\:deviceX/ directory that would be great.

It is possible the tools don't cope with a mixture of modified and unmodified
channels (without index).  Whilst the ABI docs don't say you can't do this
it is a rather obscure corner case.

The maping from hotjunction to object isn't totally clear to me.
Mind you neither is the mapping from cold junction to ambient (that one is
a bit stronger as the datasheet tables assume
Cold Junction Temperature == Ambient Temperature.

Example of why I don't like this is object is no obvious if the hotjunction
is in a gas or liquid.  The object defintion was I think added for infrared
temperature sensors where you get nothing meaningful without an object to
emit the infrared.

An alternative would be to provide an index for both channels. Also an ABI
change, but avoids the object / hot junction issue and I would assume works
fine with iio_info.

Jonathan



> 
> iio_info version: 0.25 (git tag:b6028fde)
> Libiio version: 0.25 (git tag: b6028fd) backends: local xml ip usb serial
> 
> Besides that it eases distinction between the two channels in the last
> patch, but I think this argument is not strong enough. :)
> 
> Best regards,
> Dimitri
> 
> >   
> > > 
> > > Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com>
> > > ---
> > >  drivers/iio/temperature/mcp9600.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/drivers/iio/temperature/mcp9600.c b/drivers/iio/temperature/mcp9600.c
> > > index 7a3eef5d5e75..e277edb4ae4b 100644
> > > --- a/drivers/iio/temperature/mcp9600.c
> > > +++ b/drivers/iio/temperature/mcp9600.c
> > > @@ -26,6 +26,8 @@ static const struct iio_chan_spec mcp9600_channels[] = {
> > >  	{
> > >  		.type = IIO_TEMP,
> > >  		.address = MCP9600_HOT_JUNCTION,
> > > +		.channel2 = IIO_MOD_TEMP_OBJECT,
> > > +		.modified = 1,
> > >  		.info_mask_separate =
> > >  			BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
> > >  	},  
> >   
> 

Re: [PATCH 1/5] iio: temperature: mcp9600: set channel2 member

[Dimitri Fedrau]

Am Sun, May 05, 2024 at 11:15:41AM +0100 schrieb Jonathan Cameron:
> On Tue, 30 Apr 2024 14:21:57 +0200
> Dimitri Fedrau <dima.fedrau@gmail.com> wrote:
> 
> > Am Tue, Apr 30, 2024 at 01:11:02PM +0100 schrieb Jonathan Cameron:
> > > On Tue, 30 Apr 2024 14:05:31 +0200
> > > Dimitri Fedrau <dima.fedrau@gmail.com> wrote:
> > >   
> > > > Set channel2 member of channel 0 to IIO_MOD_TEMP_OBJECT and set modified
> > > > member to 1.  
> > > This an ABI change, so needs a strong argument + must be a fix 
> > > rather than an improvement.  So why does this need to change?
> > >  
> > Hi Jonathan,
> > 
> > I don't know if it is an valid argument but when using tool "iio_info"
> > the temp_object wasn't displayed at all. After adding these two lines
> > the temp_object is displayed. Don't know if it is a problem with the
> > userspace tools.
> 
> Just to check, it displayed not temperature channel for this?
>
It did it correctly when reading from /sys/bus/iio/devices/iio\:device0
but when using "iio_info" it didn't. See below.

> If you could send the file listing of the appropriate
> /sys/bus/iio/devices/iio\:deviceX/ directory that would be great.
> 

root@raspberrypi3-64:~# ls -al /sys/bus/iio/devices/iio\:device0/
drwxr-xr-x    3 root     root             0 May  4 20:11 .
drwxr-xr-x    4 root     root             0 May  4 20:11 ..
-rw-r--r--    1 root     root          4096 May  4 20:11 in_temp_ambient_raw
-rw-r--r--    1 root     root          4096 May  4 20:11 in_temp_ambient_scale
-rw-r--r--    1 root     root          4096 May  4 20:11 in_temp_raw
-rw-r--r--    1 root     root          4096 May  4 20:11 in_temp_scale
-r--r--r--    1 root     root          4096 May  4 20:11 name
lrwxrwxrwx    1 root     root             0 May  4 20:11 of_node -> ../../../../../../../firmware/devicetree/base/soc/i2c@7e804000/temperature-sensor@67
drwxr-xr-x    2 root     root             0 May  4 20:11 power
lrwxrwxrwx    1 root     root             0 May  4 20:11 subsystem -> ../../../../../../../bus/iio
-rw-r--r--    1 root     root          4096 May  4 20:11 uevent
-r--r--r--    1 root     root          4096 May  4 20:11 waiting_for_supplier

> It is possible the tools don't cope with a mixture of modified and unmodified
> channels (without index).  Whilst the ABI docs don't say you can't do this
> it is a rather obscure corner case.
> 

I think you are right, below are my findings. When doing a cat from
in_temp_ambient_raw or in_temp_raw the results are correct, when using
iio_info there is only a single channel displayed with four attributes.

root@raspberrypi3-64:~# cat /sys/bus/iio/devices/iio\:device0/in_temp_ambient_raw; cat /sys/bus/iio/devices/iio\:device0/in_temp_raw; iio_info
314
540
iio_info version: 0.25 (git tag:b6028fde)
Libiio version: 0.25 (git tag: b6028fd) backends: local xml ip usb serial
IIO context created with local backend.
Backend version: 0.25 (git tag: b6028fd)
Backend description string: Linux raspberrypi3-64 6.9.0-rc1-g7584c270afae-dirty #239 SMP PREEMPT Thu May  9 20:11:36 CEST 2024 aarch64
IIO context has 2 attributes:
        local,kernel: 6.9.0-rc1-g7584c270afae-dirty
        uri: local:
IIO context has 3 devices:
        hwmon0: cpu_thermal
                1 channels found:
                        temp1:  (input)
                        1 channel-specific attributes found:
                                attr  0: input value: 42932
                No trigger on this device
        hwmon1: rpi_volt
                1 channels found:
                        in0:  (input)
                        1 channel-specific attributes found:
                                attr  0: lcrit_alarm value: 0
                No trigger on this device
        iio:device0: mcp9600
                1 channels found:
                        temp_ambient:  (input)
                        4 channel-specific attributes found:
                                attr  0: raw value: 314
                                attr  1: raw value: 314
                                attr  2: scale value: 62.500000
                                attr  3: scale value: 62.500000
                1 device-specific attributes found:
                                attr  0: waiting_for_supplier value: 0
                No trigger on this device




With the patch setting modified and channel2:


root@raspberrypi3-64:~# ls -al /sys/bus/iio/devices/iio\:device0/
drwxr-xr-x    3 root     root             0 May  9 18:21 .
drwxr-xr-x    4 root     root             0 May  9 18:21 ..
-rw-r--r--    1 root     root          4096 May  9 18:22 in_temp_ambient_raw
-rw-r--r--    1 root     root          4096 May  9 18:22 in_temp_ambient_scale
-rw-r--r--    1 root     root          4096 May  9 18:22 in_temp_object_raw
-rw-r--r--    1 root     root          4096 May  9 18:22 in_temp_object_scale
-r--r--r--    1 root     root          4096 May  9 18:21 name
lrwxrwxrwx    1 root     root             0 May  9 18:22 of_node -> ../../../../../../../firmware/devicetree/base/soc/i2c@7e804000/temperature-sensor@67
drwxr-xr-x    2 root     root             0 May  9 18:22 power
lrwxrwxrwx    1 root     root             0 May  9 18:22 subsystem -> ../../../../../../../bus/iio
-rw-r--r--    1 root     root          4096 May  9 18:22 uevent
-r--r--r--    1 root     root          4096 May  9 18:22 waiting_for_supplier

root@raspberrypi3-64:~# cat /sys/bus/iio/devices/iio\:device0/in_temp_ambient_raw; cat /sys/bus/iio/devices/iio\:device0/in_temp_object_raw; iio_info                                                              
318
523
iio_info version: 0.25 (git tag:b6028fde)
Libiio version: 0.25 (git tag: b6028fd) backends: local xml ip usb serial
IIO context created with local backend.
Backend version: 0.25 (git tag: b6028fd)
Backend description string: Linux raspberrypi3-64 6.9.0-rc1-g51f9ab5c4102-dirty #240 SMP PREEMPT Thu May  9 20:21:32 CEST 2024 aarch64
IIO context has 2 attributes:
        local,kernel: 6.9.0-rc1-g51f9ab5c4102-dirty
        uri: local:
IIO context has 3 devices:
        hwmon0: cpu_thermal
                1 channels found:
                        temp1:  (input)
                        1 channel-specific attributes found:
                                attr  0: input value: 45084
                No trigger on this device
        hwmon1: rpi_volt
                1 channels found:
                        in0:  (input)
                        1 channel-specific attributes found:
                                attr  0: lcrit_alarm value: 0
                No trigger on this device
        iio:device0: mcp9600
                2 channels found:
                        temp_ambient:  (input)
                        2 channel-specific attributes found:
                                attr  0: raw value: 317
                                attr  1: scale value: 62.500000
                        temp_object:  (input)
                        2 channel-specific attributes found:
                                attr  0: raw value: 532
                                attr  1: scale value: 62.500000
                1 device-specific attributes found:
                                attr  0: waiting_for_supplier value: 0
                No trigger on this device

> The maping from hotjunction to object isn't totally clear to me.
> Mind you neither is the mapping from cold junction to ambient (that one is
> a bit stronger as the datasheet tables assume
> Cold Junction Temperature == Ambient Temperature.
> 
> Example of why I don't like this is object is no obvious if the hotjunction
> is in a gas or liquid.  The object defintion was I think added for infrared
> temperature sensors where you get nothing meaningful without an object to
> emit the infrared.
> 
Thanks for the explanation, somehow I was satisfied with the mapping
from hotjunction to object. :) If the object is gas or liquid ?!

> An alternative would be to provide an index for both channels. Also an ABI
> change, but avoids the object / hot junction issue and I would assume works
> fine with iio_info.
> 
I will have a look into it and come up with a patch eventually. Don't
know if its worth changing the ABI. What do you think ?

Dimitri

...

[PATCH 2/5] iio: temperature: mcp9600: Share scale by all channels

[Dimitri Fedrau]

Move bit IIO_CHAN_INFO_SCALE from info_mask_separate to
info_mask_shared_by_all since temperature format is the same for all
channels.

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

diff --git a/drivers/iio/temperature/mcp9600.c b/drivers/iio/temperature/mcp9600.c
index e277edb4ae4b..74e0782fb073 100644
--- a/drivers/iio/temperature/mcp9600.c
+++ b/drivers/iio/temperature/mcp9600.c
@@ -28,16 +28,16 @@ static const struct iio_chan_spec mcp9600_channels[] = {
 		.address = MCP9600_HOT_JUNCTION,
 		.channel2 = IIO_MOD_TEMP_OBJECT,
 		.modified = 1,
-		.info_mask_separate =
-			BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SCALE),
 	},
 	{
 		.type = IIO_TEMP,
 		.address = MCP9600_COLD_JUNCTION,
 		.channel2 = IIO_MOD_TEMP_AMBIENT,
 		.modified = 1,
-		.info_mask_separate =
-			BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SCALE),
 	},
 };
 
-- 
2.39.2

Re: [PATCH 2/5] iio: temperature: mcp9600: Share scale by all channels

[Jonathan Cameron]

On Tue, 30 Apr 2024 14:05:32 +0200
Dimitri Fedrau <dima.fedrau@gmail.com> wrote:

> Move bit IIO_CHAN_INFO_SCALE from info_mask_separate to
> info_mask_shared_by_all since temperature format is the same for all
> channels.
> 
> Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com>

Whilst this shouldn't be a problem for well behaved userspace,
it is an ABI change so as a general rule we don't 'fix' cases like
this once they have been in a kernel release.

We may be break someone's hand crafted scripts :(

Jonathan

> ---
>  drivers/iio/temperature/mcp9600.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/temperature/mcp9600.c b/drivers/iio/temperature/mcp9600.c
> index e277edb4ae4b..74e0782fb073 100644
> --- a/drivers/iio/temperature/mcp9600.c
> +++ b/drivers/iio/temperature/mcp9600.c
> @@ -28,16 +28,16 @@ static const struct iio_chan_spec mcp9600_channels[] = {
>  		.address = MCP9600_HOT_JUNCTION,
>  		.channel2 = IIO_MOD_TEMP_OBJECT,
>  		.modified = 1,
> -		.info_mask_separate =
> -			BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SCALE),
>  	},
>  	{
>  		.type = IIO_TEMP,
>  		.address = MCP9600_COLD_JUNCTION,
>  		.channel2 = IIO_MOD_TEMP_AMBIENT,
>  		.modified = 1,
> -		.info_mask_separate =
> -			BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SCALE),
>  	},
>  };
>  

Re: [PATCH 2/5] iio: temperature: mcp9600: Share scale by all channels

[Dimitri Fedrau]

Am Tue, Apr 30, 2024 at 01:09:34PM +0100 schrieb Jonathan Cameron:
> On Tue, 30 Apr 2024 14:05:32 +0200
> Dimitri Fedrau <dima.fedrau@gmail.com> wrote:
> 
> > Move bit IIO_CHAN_INFO_SCALE from info_mask_separate to
> > info_mask_shared_by_all since temperature format is the same for all
> > channels.
> > 
> > Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com>
> 
> Whilst this shouldn't be a problem for well behaved userspace,
> it is an ABI change so as a general rule we don't 'fix' cases like
> this once they have been in a kernel release.
> 
> We may be break someone's hand crafted scripts :(
>
Ok, will remove the patch from the series.

Best regards,
Dimitri Fedrau

> 
> > ---
> >  drivers/iio/temperature/mcp9600.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/iio/temperature/mcp9600.c b/drivers/iio/temperature/mcp9600.c
> > index e277edb4ae4b..74e0782fb073 100644
> > --- a/drivers/iio/temperature/mcp9600.c
> > +++ b/drivers/iio/temperature/mcp9600.c
> > @@ -28,16 +28,16 @@ static const struct iio_chan_spec mcp9600_channels[] = {
> >  		.address = MCP9600_HOT_JUNCTION,
> >  		.channel2 = IIO_MOD_TEMP_OBJECT,
> >  		.modified = 1,
> > -		.info_mask_separate =
> > -			BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
> > +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> > +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SCALE),
> >  	},
> >  	{
> >  		.type = IIO_TEMP,
> >  		.address = MCP9600_COLD_JUNCTION,
> >  		.channel2 = IIO_MOD_TEMP_AMBIENT,
> >  		.modified = 1,
> > -		.info_mask_separate =
> > -			BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
> > +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> > +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SCALE),
> >  	},
> >  };
> >  
> 

[PATCH 3/5] iio: temperature: mcp9600: add newlines after if statements

[Dimitri Fedrau]

Add newlines after if statements following coding guide lines.

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

diff --git a/drivers/iio/temperature/mcp9600.c b/drivers/iio/temperature/mcp9600.c
index 74e0782fb073..4003bc8c83d3 100644
--- a/drivers/iio/temperature/mcp9600.c
+++ b/drivers/iio/temperature/mcp9600.c
@@ -72,6 +72,7 @@ static int mcp9600_read_raw(struct iio_dev *indio_dev,
 		ret = mcp9600_read(data, chan, val);
 		if (ret)
 			return ret;
+
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_SCALE:
 		*val = 62;
@@ -95,6 +96,7 @@ static int mcp9600_probe(struct i2c_client *client)
 	ret = i2c_smbus_read_byte_data(client, MCP9600_DEVICE_ID);
 	if (ret < 0)
 		return dev_err_probe(&client->dev, ret, "Failed to read device ID\n");
+
 	if (ret != MCP9600_DEVICE_ID_MCP9600)
 		dev_warn(&client->dev, "Expected ID %x, got %x\n",
 				MCP9600_DEVICE_ID_MCP9600, ret);
-- 
2.39.2

[PATCH 4/5] iio: temperature: mcp9600: Fix line exceeding 80 columns

[Dimitri Fedrau]

Fix line exceeding 80 columns by introducing new variable "dev" and
apply it wherever possible in mcp9600_probe.

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

diff --git a/drivers/iio/temperature/mcp9600.c b/drivers/iio/temperature/mcp9600.c
index 4003bc8c83d3..cb1c1c1c361d 100644
--- a/drivers/iio/temperature/mcp9600.c
+++ b/drivers/iio/temperature/mcp9600.c
@@ -89,19 +89,20 @@ static const struct iio_info mcp9600_info = {
 
 static int mcp9600_probe(struct i2c_client *client)
 {
+	struct device *dev = &client->dev;
 	struct iio_dev *indio_dev;
 	struct mcp9600_data *data;
 	int ret;
 
 	ret = i2c_smbus_read_byte_data(client, MCP9600_DEVICE_ID);
 	if (ret < 0)
-		return dev_err_probe(&client->dev, ret, "Failed to read device ID\n");
+		return dev_err_probe(dev, ret, "Failed to read device ID\n");
 
 	if (ret != MCP9600_DEVICE_ID_MCP9600)
-		dev_warn(&client->dev, "Expected ID %x, got %x\n",
-				MCP9600_DEVICE_ID_MCP9600, ret);
+		dev_warn(dev, "Expected ID %x, got %x\n",
+			 MCP9600_DEVICE_ID_MCP9600, ret);
 
-	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
 	if (!indio_dev)
 		return -ENOMEM;
 
@@ -114,7 +115,7 @@ static int mcp9600_probe(struct i2c_client *client)
 	indio_dev->channels = mcp9600_channels;
 	indio_dev->num_channels = ARRAY_SIZE(mcp9600_channels);
 
-	return devm_iio_device_register(&client->dev, indio_dev);
+	return devm_iio_device_register(dev, indio_dev);
 }
 
 static const struct i2c_device_id mcp9600_id[] = {
-- 
2.39.2

[PATCH 5/5] 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 | 358 +++++++++++++++++++++++++++++-
 1 file changed, 354 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/temperature/mcp9600.c b/drivers/iio/temperature/mcp9600.c
index cb1c1c1c361d..f7e1b4e3253d 100644
--- a/drivers/iio/temperature/mcp9600.c
+++ b/drivers/iio/temperature/mcp9600.c
@@ -6,21 +6,80 @@
  * Author: <andrew.hepp@ahepp.dev>
  */
 
+#include <linux/bitfield.h>
+#include <linux/bitops.h>
 #include <linux/err.h>
 #include <linux/i2c.h>
 #include <linux/init.h>
+#include <linux/math.h>
+#include <linux/minmax.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/units.h>
 
+#include <linux/iio/events.h>
 #include <linux/iio/iio.h>
 
 /* MCP9600 registers */
-#define MCP9600_HOT_JUNCTION 0x0
-#define MCP9600_COLD_JUNCTION 0x2
-#define MCP9600_DEVICE_ID 0x20
+#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_DEVICE_ID		0x20
 
 /* MCP9600 device id value */
-#define MCP9600_DEVICE_ID_MCP9600 0x40
+#define MCP9600_DEVICE_ID_MCP9600	0x40
+
+#define MCP9600_ALERT_COUNT		4
+
+#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[] = {
 	{
@@ -30,6 +89,8 @@ static const struct iio_chan_spec mcp9600_channels[] = {
 		.modified = 1,
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
 		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SCALE),
+		.event_spec = mcp9600_events,
+		.num_event_specs = ARRAY_SIZE(mcp9600_events),
 	},
 	{
 		.type = IIO_TEMP,
@@ -38,11 +99,15 @@ static const struct iio_chan_spec mcp9600_channels[] = {
 		.modified = 1,
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
 		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SCALE),
+		.event_spec = mcp9600_events,
+		.num_event_specs = ARRAY_SIZE(mcp9600_events),
 	},
 };
 
 struct mcp9600_data {
 	struct i2c_client *client;
+	struct mutex lock[MCP9600_ALERT_COUNT];
+	int irq[MCP9600_ALERT_COUNT];
 };
 
 static int mcp9600_read(struct mcp9600_data *data,
@@ -83,10 +148,292 @@ static int mcp9600_read_raw(struct iio_dev *indio_dev,
 	}
 }
 
+static int mcp9600_get_alert_index(int channel2, enum iio_event_direction dir)
+{
+	switch (channel2) {
+	case IIO_MOD_TEMP_OBJECT:
+		if (dir == IIO_EV_DIR_RISING)
+			return MCP9600_ALERT1;
+		else
+			return MCP9600_ALERT2;
+	case IIO_MOD_TEMP_AMBIENT:
+		if (dir == IIO_EV_DIR_RISING)
+			return MCP9600_ALERT3;
+		else
+			return MCP9600_ALERT4;
+	default:
+		return -EINVAL;
+	}
+}
+
+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->channel2, dir);
+	if (i < 0)
+		return i;
+
+	ret = i2c_smbus_read_byte_data(client, MCP9600_ALERT_CFG(i + 1));
+	if (ret < 0)
+		return ret;
+
+	return (ret & MCP9600_ALERT_CFG_ENABLE);
+}
+
+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->channel2, dir);
+	if (i < 0)
+		return i;
+
+	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->channel2, dir);
+	if (i < 0)
+		return i;
+
+	guard(mutex)(&data->lock[i]);
+	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(ret, 15) >> 2;
+	*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 offset which is not signed, therefore we have
+	 * to include directions when calculating the real hysteresis value.
+	 */
+	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 * (int)MICRO) - val2) :
+			    ((val * (int)MICRO) + val2);
+
+	/* Hot junction temperature range is from –200 to 1800 degree celsius */
+	if (chan->channel2 == IIO_MOD_TEMP_OBJECT &&
+	   (s_val < (MCP9600_MIN_TEMP_HOT_JUNCTION * (int)MICRO) ||
+	    s_val > (MCP9600_MAX_TEMP_HOT_JUNCTION * (int)MICRO)))
+		return -EINVAL;
+
+	/* Cold junction temperature range is from –40 to 125 degree celsius */
+	if (chan->channel2 == IIO_MOD_TEMP_AMBIENT &&
+	   (s_val < (MCP9600_MIN_TEMP_COLD_JUNCTION * (int)MICRO) ||
+	    s_val > (MCP9600_MAX_TEMP_COLD_JUNCTION * (int)MICRO)))
+		return -EINVAL;
+
+	i = mcp9600_get_alert_index(chan->channel2, dir);
+	if (i < 0)
+		return i;
+
+	guard(mutex)(&data->lock[i]);
+	if (info == 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 / (int)(MICRO >> 4));
+		return i2c_smbus_write_word_swapped(client,
+						    MCP9600_ALERT_LIMIT(i + 1),
+						    thresh);
+	}
+
+	/* 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) * (int)(MICRO >> 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) / MICRO);
+
+	return i2c_smbus_write_byte_data(client,
+					 MCP9600_ALERT_HYSTERESIS(i + 1),
+					 hyst);
+}
+
 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_alert_handler(int irq, void *private)
+{
+	struct iio_dev *indio_dev = private;
+	struct mcp9600_data *data = iio_priv(indio_dev);
+	enum iio_event_direction dir;
+	enum iio_modifier mod;
+	int i, ret;
+
+	for (i = 0; i < MCP9600_ALERT_COUNT; i++) {
+		if (data->irq[i] == irq)
+			break;
+	}
+
+	if (i >= MCP9600_ALERT_COUNT)
+		return IRQ_NONE;
+
+	ret = i2c_smbus_read_byte_data(data->client, MCP9600_STATUS);
+	if (ret < 0)
+		return IRQ_HANDLED;
+
+	switch (ret & MCP9600_STATUS_ALERT(i)) {
+	case 0:
+		return IRQ_NONE;
+	case MCP9600_STATUS_ALERT(MCP9600_ALERT1):
+		mod = IIO_MOD_TEMP_OBJECT;
+		dir = IIO_EV_DIR_RISING;
+		break;
+	case MCP9600_STATUS_ALERT(MCP9600_ALERT2):
+		mod = IIO_MOD_TEMP_OBJECT;
+		dir = IIO_EV_DIR_FALLING;
+		break;
+	case MCP9600_STATUS_ALERT(MCP9600_ALERT3):
+		mod = IIO_MOD_TEMP_AMBIENT;
+		dir = IIO_EV_DIR_RISING;
+		break;
+	case MCP9600_STATUS_ALERT(MCP9600_ALERT4):
+		mod = IIO_MOD_TEMP_AMBIENT;
+		dir = IIO_EV_DIR_FALLING;
+		break;
+	default:
+		return IRQ_HANDLED;
+	}
+
+	iio_push_event(indio_dev,
+		       IIO_MOD_EVENT_CODE(IIO_TEMP, 0, mod,
+					  IIO_EV_TYPE_THRESH, dir),
+		       iio_get_time_ns(indio_dev));
+
+	return IRQ_HANDLED;
+}
+
+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++) {
+		data->irq[i] = 0;
+		mutex_init(&data->lock[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,
+						IRQF_ONESHOT, "mcp9600",
+						indio_dev);
+		if (ret)
+			return ret;
+
+		data->irq[i] = irq;
+	}
+
+	return 0;
+}
+
 static int mcp9600_probe(struct i2c_client *client)
 {
 	struct device *dev = &client->dev;
@@ -109,6 +456,8 @@ static int mcp9600_probe(struct i2c_client *client)
 	data = iio_priv(indio_dev);
 	data->client = client;
 
+	mcp9600_probe_alerts(indio_dev);
+
 	indio_dev->info = &mcp9600_info;
 	indio_dev->name = "mcp9600";
 	indio_dev->modes = INDIO_DIRECT_MODE;
@@ -140,6 +489,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 5/5] iio: temperature: mcp9600: add threshold events support

[kernel test robot]

Hi Dimitri,

kernel test robot noticed the following build errors:

[auto build test ERROR on jic23-iio/togreg]
[also build test ERROR on linus/master v6.9-rc6 next-20240430]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Dimitri-Fedrau/iio-temperature-mcp9600-set-channel2-member/20240430-200914
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link:    https://lore.kernel.org/r/20240430120535.46097-6-dima.fedrau%40gmail.com
patch subject: [PATCH 5/5] iio: temperature: mcp9600: add threshold events support
config: i386-buildonly-randconfig-005-20240501 (https://download.01.org/0day-ci/archive/20240501/202405010420.2KNGPYh5-lkp@intel.com/config)
compiler: gcc-13 (Ubuntu 13.2.0-4ubuntu3) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240501/202405010420.2KNGPYh5-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202405010420.2KNGPYh5-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/iio/temperature/mcp9600.c: In function 'mcp9600_probe_alerts':
>> drivers/iio/temperature/mcp9600.c:407:28: error: implicit declaration of function 'irq_get_trigger_type' [-Werror=implicit-function-declaration]
     407 |                 irq_type = irq_get_trigger_type(irq);
         |                            ^~~~~~~~~~~~~~~~~~~~
>> drivers/iio/temperature/mcp9600.c:408:33: error: 'IRQ_TYPE_EDGE_RISING' undeclared (first use in this function)
     408 |                 if (irq_type == IRQ_TYPE_EDGE_RISING)
         |                                 ^~~~~~~~~~~~~~~~~~~~
   drivers/iio/temperature/mcp9600.c:408:33: note: each undeclared identifier is reported only once for each function it appears in
   cc1: some warnings being treated as errors


vim +/irq_get_trigger_type +407 drivers/iio/temperature/mcp9600.c

   382	
   383	static int mcp9600_probe_alerts(struct iio_dev *indio_dev)
   384	{
   385		struct mcp9600_data *data = iio_priv(indio_dev);
   386		struct i2c_client *client = data->client;
   387		struct device *dev = &client->dev;
   388		struct fwnode_handle *fwnode = dev_fwnode(dev);
   389		unsigned int irq_type;
   390		int ret, irq, i;
   391		u8 val;
   392	
   393		/*
   394		 * alert1: hot junction, rising temperature
   395		 * alert2: hot junction, falling temperature
   396		 * alert3: cold junction, rising temperature
   397		 * alert4: cold junction, falling temperature
   398		 */
   399		for (i = 0; i < MCP9600_ALERT_COUNT; i++) {
   400			data->irq[i] = 0;
   401			mutex_init(&data->lock[i]);
   402			irq = fwnode_irq_get_byname(fwnode, mcp9600_alert_name[i]);
   403			if (irq <= 0)
   404				continue;
   405	
   406			val = 0;
 > 407			irq_type = irq_get_trigger_type(irq);
 > 408			if (irq_type == IRQ_TYPE_EDGE_RISING)
   409				val |= MCP9600_ALERT_CFG_ACTIVE_HIGH;
   410	
   411			if (i == MCP9600_ALERT2 || i == MCP9600_ALERT4)
   412				val |= MCP9600_ALERT_CFG_FALLING;
   413	
   414			if (i == MCP9600_ALERT3 || i == MCP9600_ALERT4)
   415				val |= MCP9600_ALERT_CFG_COLD_JUNCTION;
   416	
   417			ret = i2c_smbus_write_byte_data(client,
   418							MCP9600_ALERT_CFG(i + 1),
   419							val);
   420			if (ret < 0)
   421				return ret;
   422	
   423			ret = devm_request_threaded_irq(dev, irq, NULL,
   424							mcp9600_alert_handler,
   425							IRQF_ONESHOT, "mcp9600",
   426							indio_dev);
   427			if (ret)
   428				return ret;
   429	
   430			data->irq[i] = irq;
   431		}
   432	
   433		return 0;
   434	}
   435	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Jonathan Cameron]

On Tue, 30 Apr 2024 14:05:35 +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>
> ---
Various comments inline.

Jonathan

>  drivers/iio/temperature/mcp9600.c | 358 +++++++++++++++++++++++++++++-
>  1 file changed, 354 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/temperature/mcp9600.c b/drivers/iio/temperature/mcp9600.c
> index cb1c1c1c361d..f7e1b4e3253d 100644
> --- a/drivers/iio/temperature/mcp9600.c
> +++ b/drivers/iio/temperature/mcp9600.c
> @@ -6,21 +6,80 @@
>   * Author: <andrew.hepp@ahepp.dev>
>   */
>  
> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>
>  #include <linux/err.h>
>  #include <linux/i2c.h>
>  #include <linux/init.h>
> +#include <linux/math.h>
> +#include <linux/minmax.h>
>  #include <linux/mod_devicetable.h>
>  #include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/units.h>
>  
> +#include <linux/iio/events.h>
>  #include <linux/iio/iio.h>
>  
>  /* MCP9600 registers */
> -#define MCP9600_HOT_JUNCTION 0x0

As below. Reformating in a precursor patch. I wouldn't necessarily bother
though as aligning defines is usually more effort than it is worth over time.

> -#define MCP9600_COLD_JUNCTION 0x2
> -#define MCP9600_DEVICE_ID 0x20
> +#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_DEVICE_ID		0x20
>  
>  /* MCP9600 device id value */
> -#define MCP9600_DEVICE_ID_MCP9600 0x40
> +#define MCP9600_DEVICE_ID_MCP9600	0x40

If you want to reformatting existing lines, do it in a precursor patch - not
buried in here.

>  
>  struct mcp9600_data {
>  	struct i2c_client *client;
> +	struct mutex lock[MCP9600_ALERT_COUNT];

All locks need documentation.  What data is this protecting?

> +	int irq[MCP9600_ALERT_COUNT];
>  };
>  
>  static int mcp9600_read(struct mcp9600_data *data,
> @@ -83,10 +148,292 @@ static int mcp9600_read_raw(struct iio_dev *indio_dev,
>  	}
>  }
>  
> +static int mcp9600_get_alert_index(int channel2, enum iio_event_direction dir)
> +{
> +	switch (channel2) {
> +	case IIO_MOD_TEMP_OBJECT:
> +		if (dir == IIO_EV_DIR_RISING)
> +			return MCP9600_ALERT1;
> +		else
> +			return MCP9600_ALERT2;
> +	case IIO_MOD_TEMP_AMBIENT:
> +		if (dir == IIO_EV_DIR_RISING)
> +			return MCP9600_ALERT3;
> +		else
> +			return MCP9600_ALERT4;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +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->channel2, dir);
> +	if (i < 0)
> +		return i;
> +
> +	ret = i2c_smbus_read_byte_data(client, MCP9600_ALERT_CFG(i + 1));
> +	if (ret < 0)
> +		return ret;
> +
> +	return (ret & MCP9600_ALERT_CFG_ENABLE);

FIELD_GET() even if it happens to be bit(0) as then we don't have to go
check that's the case.

> +}
> +
> +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->channel2, dir);
> +	if (i < 0)
> +		return i;
> +
> +	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);

A read modify write cycle like this normally needs some locking to ensure another
access didn't change the other bits in the register.


> +}
> +
> +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->channel2, dir);
> +	if (i < 0)
> +		return i;
> +
> +	guard(mutex)(&data->lock[i]);
> +	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(ret, 15) >> 2;
Use sign_extend32(FIELD_GET(...), 13)
So which bits are extracted is obvious in the code.

> +	*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 offset which is not signed, therefore we have
> +	 * to include directions when calculating the real hysteresis value.
> +	 */
> +	if (dir == IIO_EV_DIR_RISING)
> +		*val -= (*val2 * ret);
> +	else
> +		*val += (*val2 * ret);

I don't follow this maths.  Hysteresis is an unsigned offset.  Maybe some confusion
over the ABI?  

> +
> +	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 * (int)MICRO) - val2) :
> +			    ((val * (int)MICRO) + val2);
> +
> +	/* Hot junction temperature range is from –200 to 1800 degree celsius */
> +	if (chan->channel2 == IIO_MOD_TEMP_OBJECT &&
> +	   (s_val < (MCP9600_MIN_TEMP_HOT_JUNCTION * (int)MICRO) ||
> +	    s_val > (MCP9600_MAX_TEMP_HOT_JUNCTION * (int)MICRO)))

Why the casts?

> +		return -EINVAL;
> +
> +	/* Cold junction temperature range is from –40 to 125 degree celsius */
> +	if (chan->channel2 == IIO_MOD_TEMP_AMBIENT &&
> +	   (s_val < (MCP9600_MIN_TEMP_COLD_JUNCTION * (int)MICRO) ||
> +	    s_val > (MCP9600_MAX_TEMP_COLD_JUNCTION * (int)MICRO)))
> +		return -EINVAL;
> +
> +	i = mcp9600_get_alert_index(chan->channel2, dir);
> +	if (i < 0)
> +		return i;
> +
> +	guard(mutex)(&data->lock[i]);
> +	if (info == IIO_EV_INFO_VALUE) {

I would use a switch statement so it is obvious what each case is.

> +		/*
> +		 * Shift length 4 bits = 2(15:2) + 2(0.25 LSB), temperature is
> +		 * stored in two’s complement format.
> +		 */
> +		thresh = (s16)(s_val / (int)(MICRO >> 4));
> +		return i2c_smbus_write_word_swapped(client,
> +						    MCP9600_ALERT_LIMIT(i + 1),
> +						    thresh);
> +	}
> +
> +	/* 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) * (int)(MICRO >> 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) / MICRO);
> +
> +	return i2c_smbus_write_byte_data(client,
> +					 MCP9600_ALERT_HYSTERESIS(i + 1),
> +					 hyst);
> +}
> +
>  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_alert_handler(int irq, void *private)
> +{
> +	struct iio_dev *indio_dev = private;
> +	struct mcp9600_data *data = iio_priv(indio_dev);
> +	enum iio_event_direction dir;
> +	enum iio_modifier mod;
> +	int i, ret;
> +
> +	for (i = 0; i < MCP9600_ALERT_COUNT; i++) {
> +		if (data->irq[i] == irq)

This search for a match is a little messy. I'd be tempted
to wrap the generic handler in a per instance interrupt handler
(so have 4 functions) and thus move this matching to the place
where they are registered, not the interrupt handler.

There isn't a lot of shared code in here so you may be better
off just having 4 separate interrupt handler implementations.

> +			break;
> +	}
> +
> +	if (i >= MCP9600_ALERT_COUNT)
> +		return IRQ_NONE;
> +
> +	ret = i2c_smbus_read_byte_data(data->client, MCP9600_STATUS);
> +	if (ret < 0)
> +		return IRQ_HANDLED;
> +
> +	switch (ret & MCP9600_STATUS_ALERT(i)) {
> +	case 0:
> +		return IRQ_NONE;
> +	case MCP9600_STATUS_ALERT(MCP9600_ALERT1):
> +		mod = IIO_MOD_TEMP_OBJECT;
> +		dir = IIO_EV_DIR_RISING;
> +		break;
> +	case MCP9600_STATUS_ALERT(MCP9600_ALERT2):
> +		mod = IIO_MOD_TEMP_OBJECT;
> +		dir = IIO_EV_DIR_FALLING;
> +		break;
> +	case MCP9600_STATUS_ALERT(MCP9600_ALERT3):
> +		mod = IIO_MOD_TEMP_AMBIENT;
> +		dir = IIO_EV_DIR_RISING;
> +		break;
> +	case MCP9600_STATUS_ALERT(MCP9600_ALERT4):
> +		mod = IIO_MOD_TEMP_AMBIENT;
> +		dir = IIO_EV_DIR_FALLING;
> +		break;
> +	default:
> +		return IRQ_HANDLED;
> +	}
> +
> +	iio_push_event(indio_dev,
> +		       IIO_MOD_EVENT_CODE(IIO_TEMP, 0, mod,
> +					  IIO_EV_TYPE_THRESH, dir),
> +		       iio_get_time_ns(indio_dev));
> +
> +	return IRQ_HANDLED;
> +}
> +
> +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++) {
> +		data->irq[i] = 0;

All of data is zeroed already so this should not be needed.

> +		mutex_init(&data->lock[i]);

Why per interrupt locks?  Seems unlikely to be a big problem
to share one.

> +		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,
> +						IRQF_ONESHOT, "mcp9600",
> +						indio_dev);
> +		if (ret)
> +			return ret;
> +
> +		data->irq[i] = irq;
> +	}
> +
> +	return 0;
> +}
> +
>  static int mcp9600_probe(struct i2c_client *client)
>  {
>  	struct device *dev = &client->dev;
> @@ -109,6 +456,8 @@ static int mcp9600_probe(struct i2c_client *client)
>  	data = iio_priv(indio_dev);
>  	data->client = client;
>  
> +	mcp9600_probe_alerts(indio_dev);

Why no error check? 

> +
>  	indio_dev->info = &mcp9600_info;
>  	indio_dev->name = "mcp9600";
>  	indio_dev->modes = INDIO_DIRECT_MODE;
> @@ -140,6 +489,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");

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

[Dimitri Fedrau]

Am Sun, May 05, 2024 at 06:47:00PM +0100 schrieb Jonathan Cameron:
> On Tue, 30 Apr 2024 14:05:35 +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>
> > ---
> Various comments inline.
> 
> Jonathan
> 
> >  drivers/iio/temperature/mcp9600.c | 358 +++++++++++++++++++++++++++++-
> >  1 file changed, 354 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/iio/temperature/mcp9600.c b/drivers/iio/temperature/mcp9600.c
> > index cb1c1c1c361d..f7e1b4e3253d 100644
> > --- a/drivers/iio/temperature/mcp9600.c
> > +++ b/drivers/iio/temperature/mcp9600.c
> > @@ -6,21 +6,80 @@
> >   * Author: <andrew.hepp@ahepp.dev>
> >   */
> >  
> > +#include <linux/bitfield.h>
> > +#include <linux/bitops.h>
> >  #include <linux/err.h>
> >  #include <linux/i2c.h>
> >  #include <linux/init.h>
> > +#include <linux/math.h>
> > +#include <linux/minmax.h>
> >  #include <linux/mod_devicetable.h>
> >  #include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/units.h>
> >  
> > +#include <linux/iio/events.h>
> >  #include <linux/iio/iio.h>
> >  
> >  /* MCP9600 registers */
> > -#define MCP9600_HOT_JUNCTION 0x0
> 
> As below. Reformating in a precursor patch. I wouldn't necessarily bother
> though as aligning defines is usually more effort than it is worth over time.
>
Ok, will skip the aligning.

> > -#define MCP9600_COLD_JUNCTION 0x2
> > -#define MCP9600_DEVICE_ID 0x20
> > +#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_DEVICE_ID		0x20
> >  
> >  /* MCP9600 device id value */
> > -#define MCP9600_DEVICE_ID_MCP9600 0x40
> > +#define MCP9600_DEVICE_ID_MCP9600	0x40
> 
> If you want to reformatting existing lines, do it in a precursor patch - not
> buried in here.
> 
Ok, will skip the aligning.

> >  
> >  struct mcp9600_data {
> >  	struct i2c_client *client;
> > +	struct mutex lock[MCP9600_ALERT_COUNT];
> 
> All locks need documentation.  What data is this protecting?
> 
It protects hysteresis values, since these are represented as offsets
from thresholds. To read/write hysteresis values we need to read first
the corresponding threshold value. This can lead to race conditions, as
both can be accessed concurrently via sysfs.

> > +	int irq[MCP9600_ALERT_COUNT];
> >  };
> >  
> >  static int mcp9600_read(struct mcp9600_data *data,
> > @@ -83,10 +148,292 @@ static int mcp9600_read_raw(struct iio_dev *indio_dev,
> >  	}
> >  }
> >  
> > +static int mcp9600_get_alert_index(int channel2, enum iio_event_direction dir)
> > +{
> > +	switch (channel2) {
> > +	case IIO_MOD_TEMP_OBJECT:
> > +		if (dir == IIO_EV_DIR_RISING)
> > +			return MCP9600_ALERT1;
> > +		else
> > +			return MCP9600_ALERT2;
> > +	case IIO_MOD_TEMP_AMBIENT:
> > +		if (dir == IIO_EV_DIR_RISING)
> > +			return MCP9600_ALERT3;
> > +		else
> > +			return MCP9600_ALERT4;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> > +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->channel2, dir);
> > +	if (i < 0)
> > +		return i;
> > +
> > +	ret = i2c_smbus_read_byte_data(client, MCP9600_ALERT_CFG(i + 1));
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	return (ret & MCP9600_ALERT_CFG_ENABLE);
> 
> FIELD_GET() even if it happens to be bit(0) as then we don't have to go
> check that's the case.
> 
Ok.
> > +}
> > +
> > +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->channel2, dir);
> > +	if (i < 0)
> > +		return i;
> > +
> > +	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);
> 
> A read modify write cycle like this normally needs some locking to ensure another
> access didn't change the other bits in the register.
> 
Each alert has it's own configuration register. All bits except the
enable bit are set during probe and there is no need besides the enable
bit to set them.

> 
> > +}
> > +
> > +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->channel2, dir);
> > +	if (i < 0)
> > +		return i;
> > +
> > +	guard(mutex)(&data->lock[i]);
> > +	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(ret, 15) >> 2;
> Use sign_extend32(FIELD_GET(...), 13)
> So which bits are extracted is obvious in the code.
> 
Ok.
> > +	*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 offset which is not signed, therefore we have
> > +	 * to include directions when calculating the real hysteresis value.
> > +	 */
> > +	if (dir == IIO_EV_DIR_RISING)
> > +		*val -= (*val2 * ret);
> > +	else
> > +		*val += (*val2 * ret);
> 
> I don't follow this maths.  Hysteresis is an unsigned offset.  Maybe some confusion
> over the ABI?  
> 
The alert hysteresis range is from 0 to 255 degree celsius. The
direction bit in the alert configuration register defines whether the
value is below or above the corresponding threshold. The offset here is
the threshold itself. I will improve the comment.

> > +
> > +	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 * (int)MICRO) - val2) :
> > +			    ((val * (int)MICRO) + val2);
> > +
> > +	/* Hot junction temperature range is from –200 to 1800 degree celsius */
> > +	if (chan->channel2 == IIO_MOD_TEMP_OBJECT &&
> > +	   (s_val < (MCP9600_MIN_TEMP_HOT_JUNCTION * (int)MICRO) ||
> > +	    s_val > (MCP9600_MAX_TEMP_HOT_JUNCTION * (int)MICRO)))
> 
> Why the casts?
> 
Missed to remove them. Will fix it in the next version of the patch.
> > +		return -EINVAL;
> > +
> > +	/* Cold junction temperature range is from –40 to 125 degree celsius */
> > +	if (chan->channel2 == IIO_MOD_TEMP_AMBIENT &&
> > +	   (s_val < (MCP9600_MIN_TEMP_COLD_JUNCTION * (int)MICRO) ||
> > +	    s_val > (MCP9600_MAX_TEMP_COLD_JUNCTION * (int)MICRO)))
> > +		return -EINVAL;
> > +
> > +	i = mcp9600_get_alert_index(chan->channel2, dir);
> > +	if (i < 0)
> > +		return i;
> > +
> > +	guard(mutex)(&data->lock[i]);
> > +	if (info == IIO_EV_INFO_VALUE) {
> 
> I would use a switch statement so it is obvious what each case is.
> 
Ok.
> > +		/*
> > +		 * Shift length 4 bits = 2(15:2) + 2(0.25 LSB), temperature is
> > +		 * stored in two’s complement format.
> > +		 */
> > +		thresh = (s16)(s_val / (int)(MICRO >> 4));
> > +		return i2c_smbus_write_word_swapped(client,
> > +						    MCP9600_ALERT_LIMIT(i + 1),
> > +						    thresh);
> > +	}
> > +
> > +	/* 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) * (int)(MICRO >> 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) / MICRO);
> > +
> > +	return i2c_smbus_write_byte_data(client,
> > +					 MCP9600_ALERT_HYSTERESIS(i + 1),
> > +					 hyst);
> > +}
> > +
> >  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_alert_handler(int irq, void *private)
> > +{
> > +	struct iio_dev *indio_dev = private;
> > +	struct mcp9600_data *data = iio_priv(indio_dev);
> > +	enum iio_event_direction dir;
> > +	enum iio_modifier mod;
> > +	int i, ret;
> > +
> > +	for (i = 0; i < MCP9600_ALERT_COUNT; i++) {
> > +		if (data->irq[i] == irq)
> 
> This search for a match is a little messy. I'd be tempted
> to wrap the generic handler in a per instance interrupt handler
> (so have 4 functions) and thus move this matching to the place
> where they are registered, not the interrupt handler.
> 
> There isn't a lot of shared code in here so you may be better
> off just having 4 separate interrupt handler implementations.
> 
You are right, it is definitely the better solution.

> > +			break;
> > +	}
> > +
> > +	if (i >= MCP9600_ALERT_COUNT)
> > +		return IRQ_NONE;
> > +
> > +	ret = i2c_smbus_read_byte_data(data->client, MCP9600_STATUS);
> > +	if (ret < 0)
> > +		return IRQ_HANDLED;
> > +
> > +	switch (ret & MCP9600_STATUS_ALERT(i)) {
> > +	case 0:
> > +		return IRQ_NONE;
> > +	case MCP9600_STATUS_ALERT(MCP9600_ALERT1):
> > +		mod = IIO_MOD_TEMP_OBJECT;
> > +		dir = IIO_EV_DIR_RISING;
> > +		break;
> > +	case MCP9600_STATUS_ALERT(MCP9600_ALERT2):
> > +		mod = IIO_MOD_TEMP_OBJECT;
> > +		dir = IIO_EV_DIR_FALLING;
> > +		break;
> > +	case MCP9600_STATUS_ALERT(MCP9600_ALERT3):
> > +		mod = IIO_MOD_TEMP_AMBIENT;
> > +		dir = IIO_EV_DIR_RISING;
> > +		break;
> > +	case MCP9600_STATUS_ALERT(MCP9600_ALERT4):
> > +		mod = IIO_MOD_TEMP_AMBIENT;
> > +		dir = IIO_EV_DIR_FALLING;
> > +		break;
> > +	default:
> > +		return IRQ_HANDLED;
> > +	}
> > +
> > +	iio_push_event(indio_dev,
> > +		       IIO_MOD_EVENT_CODE(IIO_TEMP, 0, mod,
> > +					  IIO_EV_TYPE_THRESH, dir),
> > +		       iio_get_time_ns(indio_dev));
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +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++) {
> > +		data->irq[i] = 0;
> 
> All of data is zeroed already so this should not be needed.
>
Yes.

> > +		mutex_init(&data->lock[i]);
> 
> Why per interrupt locks?  Seems unlikely to be a big problem
> to share one.
>
I think the code is misleading here. The locks are not per interrupt,
just for setting thresholds, since each alert has its own configuration
registers. Locks are just needed because hysteresis values depends on
threshold values. I think there should be no problem when using a single
lock for setting thresholds/hysteresis values across all alerts. Don't
know if its worth having the four locks since it it not the fastpath.

> > +		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,
> > +						IRQF_ONESHOT, "mcp9600",
> > +						indio_dev);
> > +		if (ret)
> > +			return ret;
> > +
> > +		data->irq[i] = irq;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  static int mcp9600_probe(struct i2c_client *client)
> >  {
> >  	struct device *dev = &client->dev;
> > @@ -109,6 +456,8 @@ static int mcp9600_probe(struct i2c_client *client)
> >  	data = iio_priv(indio_dev);
> >  	data->client = client;
> >  
> > +	mcp9600_probe_alerts(indio_dev);
> 
> Why no error check? 
> 
Missed that one. Thanks.
> > +
> >  	indio_dev->info = &mcp9600_info;
> >  	indio_dev->name = "mcp9600";
> >  	indio_dev->modes = INDIO_DIRECT_MODE;
> > @@ -140,6 +489,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");
> 

Dimitri