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