| Message ID | 20240430120535.46097-2-dima.fedrau@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add threshold events support and some minor cleanup | expand |
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), > },
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), > > }, >
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), > > > }, > > >
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 ...
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), },
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(+)