| Message ID | 20210530005917.20953-5-liambeguin@gmail.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | iio: afe: add temperature rescaling support | expand |
Hi! Thanks for the patch! On 2021-05-30 02:59, Liam Beguin wrote: > From: Liam Beguin <lvb@xiphos.com> > > This is a preparatory change required for the addition of temperature > sensing front ends. I think this is too simplistic. I think that if the upstream iio-dev has an offset, it should be dealt with (i.e. be rescaled). The rescale driver cannot ignore such an upstream offset and then throw in some other unrelated offset of its own. That would be thoroughly confusing. Also, I see no reason of expose an offset channel if there is no offset. Cheers, Peter > Signed-off-by: Liam Beguin <lvb@xiphos.com> > --- > drivers/iio/afe/iio-rescale.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c > index 4d0813b274d1..3bd1f11f21db 100644 > --- a/drivers/iio/afe/iio-rescale.c > +++ b/drivers/iio/afe/iio-rescale.c > @@ -31,6 +31,7 @@ struct rescale { > struct iio_chan_spec_ext_info *ext_info; > s32 numerator; > s32 denominator; > + s32 offset; > }; > > static int rescale_read_raw(struct iio_dev *indio_dev, > @@ -52,6 +53,10 @@ static int rescale_read_raw(struct iio_dev *indio_dev, > *val2 = rescale->denominator; > > return IIO_VAL_FRACTIONAL; > + case IIO_CHAN_INFO_OFFSET: > + *val = rescale->offset; > + > + return IIO_VAL_INT; > default: > return -EINVAL; > } > @@ -119,8 +124,10 @@ static int rescale_configure_channel(struct device *dev, > return -EINVAL; > } > > - chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | > - BIT(IIO_CHAN_INFO_SCALE); > + chan->info_mask_separate = > + BIT(IIO_CHAN_INFO_RAW) | > + BIT(IIO_CHAN_INFO_SCALE) | > + BIT(IIO_CHAN_INFO_OFFSET); > > if (iio_channel_has_available(schan, IIO_CHAN_INFO_RAW)) > chan->info_mask_separate_available |= BIT(IIO_CHAN_INFO_RAW); > @@ -280,6 +287,7 @@ static int rescale_probe(struct platform_device *pdev) > rescale->cfg = of_device_get_match_data(dev); > rescale->numerator = 1; > rescale->denominator = 1; > + rescale->offset = 0; > > ret = rescale->cfg->props(dev, rescale); > if (ret) >
Hi Peter, On Mon May 31, 2021 at 4:52 AM EDT, Peter Rosin wrote: > Hi! > > Thanks for the patch! > > On 2021-05-30 02:59, Liam Beguin wrote: > > From: Liam Beguin <lvb@xiphos.com> > > > > This is a preparatory change required for the addition of temperature > > sensing front ends. > > I think this is too simplistic. I think that if the upstream iio-dev has > an offset, it should be dealt with (i.e. be rescaled). The rescale > driver > cannot ignore such an upstream offset and then throw in some other > unrelated offset of its own. That would be thoroughly confusing. I'm not sure I fully understand. The upstream offset should be dealt with when calling iio_read_channel_processed(). That was my main motivation behind using the IIO core to get a processed value. > > Also, I see no reason of expose an offset channel if there is no offset. You're right, I'll add conditions to only expose an offset when required. Liam > > Cheers, > Peter > > > Signed-off-by: Liam Beguin <lvb@xiphos.com> > > --- > > drivers/iio/afe/iio-rescale.c | 12 ++++++++++-- > > 1 file changed, 10 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c > > index 4d0813b274d1..3bd1f11f21db 100644 > > --- a/drivers/iio/afe/iio-rescale.c > > +++ b/drivers/iio/afe/iio-rescale.c > > @@ -31,6 +31,7 @@ struct rescale { > > struct iio_chan_spec_ext_info *ext_info; > > s32 numerator; > > s32 denominator; > > + s32 offset; > > }; > > > > static int rescale_read_raw(struct iio_dev *indio_dev, > > @@ -52,6 +53,10 @@ static int rescale_read_raw(struct iio_dev *indio_dev, > > *val2 = rescale->denominator; > > > > return IIO_VAL_FRACTIONAL; > > + case IIO_CHAN_INFO_OFFSET: > > + *val = rescale->offset; > > + > > + return IIO_VAL_INT; > > default: > > return -EINVAL; > > } > > @@ -119,8 +124,10 @@ static int rescale_configure_channel(struct device *dev, > > return -EINVAL; > > } > > > > - chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | > > - BIT(IIO_CHAN_INFO_SCALE); > > + chan->info_mask_separate = > > + BIT(IIO_CHAN_INFO_RAW) | > > + BIT(IIO_CHAN_INFO_SCALE) | > > + BIT(IIO_CHAN_INFO_OFFSET); > > > > if (iio_channel_has_available(schan, IIO_CHAN_INFO_RAW)) > > chan->info_mask_separate_available |= BIT(IIO_CHAN_INFO_RAW); > > @@ -280,6 +287,7 @@ static int rescale_probe(struct platform_device *pdev) > > rescale->cfg = of_device_get_match_data(dev); > > rescale->numerator = 1; > > rescale->denominator = 1; > > + rescale->offset = 0; > > > > ret = rescale->cfg->props(dev, rescale); > > if (ret) > >
On 2021-05-31 15:36, Liam Beguin wrote: > Hi Peter, > > On Mon May 31, 2021 at 4:52 AM EDT, Peter Rosin wrote: >> Hi! >> >> Thanks for the patch! >> >> On 2021-05-30 02:59, Liam Beguin wrote: >>> From: Liam Beguin <lvb@xiphos.com> >>> >>> This is a preparatory change required for the addition of temperature >>> sensing front ends. >> >> I think this is too simplistic. I think that if the upstream iio-dev has >> an offset, it should be dealt with (i.e. be rescaled). The rescale >> driver >> cannot ignore such an upstream offset and then throw in some other >> unrelated offset of its own. That would be thoroughly confusing. > > I'm not sure I fully understand. The upstream offset should be dealt > with when calling iio_read_channel_processed(). That was my main > motivation behind using the IIO core to get a processed value. You can rescale a channel with an offset, but without using processed values. I.e. the upstream channel provides raw values, a scale and an offset. The current rescale code ignores the upstream offset. I did not need that when I created the driver, and at a glace it felt "difficult". So I punted. But if the rescaler is going to start to handle offsets of any kind, it will get very confusing if the upstream offset is ignored. The proper way to do that is not something I have thought deeply about, and I don't know what the proper behavior is. For a processed channel, the offset is baked into the value that is scaled. Maybe the sane thing is to do that for a non-processed channel as well? But that gets a bit ugly, as it is counter to the simplicity, beauty and efficiency of the rescaler driver. In the non-processed case the driver is just adjusting the scale value. But since we are talking about proportional relationships, it should be possible to rescale a non-processed channel with an offset by just adjusting the offset in some way related to the rescale factor. Doing it with integer math is the "difficult" part... Cheers, Peter
On Mon May 31, 2021 at 10:08 AM EDT, Peter Rosin wrote: > On 2021-05-31 15:36, Liam Beguin wrote: > > Hi Peter, > > > > On Mon May 31, 2021 at 4:52 AM EDT, Peter Rosin wrote: > >> Hi! > >> > >> Thanks for the patch! > >> > >> On 2021-05-30 02:59, Liam Beguin wrote: > >>> From: Liam Beguin <lvb@xiphos.com> > >>> > >>> This is a preparatory change required for the addition of temperature > >>> sensing front ends. > >> > >> I think this is too simplistic. I think that if the upstream iio-dev has > >> an offset, it should be dealt with (i.e. be rescaled). The rescale > >> driver > >> cannot ignore such an upstream offset and then throw in some other > >> unrelated offset of its own. That would be thoroughly confusing. > > > > I'm not sure I fully understand. The upstream offset should be dealt > > with when calling iio_read_channel_processed(). That was my main > > motivation behind using the IIO core to get a processed value. > > You can rescale a channel with an offset, but without using processed > values. I.e. the upstream channel provides raw values, a scale and an > offset. The current rescale code ignores the upstream offset. I did not > need that when I created the driver, and at a glace it felt "difficult". > So I punted. I understand what you meant now. At first, I tried to apply the upstream offset from inside the rescaler. As you said it felt difficult and it felt like this must've been implemented somewhere else before. After looking around, I noticed that the code to do that was already part of inkern.c and exposed through iio_read_channel_processed(). If the upstream channel doesn't provide a processed value, the upstream offset and scale are automatically applied. So with the changes in [3/9] the rescaler's raw value becomes the upstream channel's processed value. This seems like an easier and probably cleaner way of adding offset support in the rescaler. Does that make sense? Cheers, Liam > > But if the rescaler is going to start to handle offsets of any kind, it > will get very confusing if the upstream offset is ignored. The proper > way to do that is not something I have thought deeply about, and I > don't know what the proper behavior is. For a processed channel, the > offset is baked into the value that is scaled. Maybe the sane thing > is to do that for a non-processed channel as well? But that gets a bit > ugly, as it is counter to the simplicity, beauty and efficiency of the > rescaler driver. In the non-processed case the driver is just adjusting > the scale value. But since we are talking about proportional > relationships, it should be possible to rescale a non-processed > channel with an offset by just adjusting the offset in some way related > to the rescale factor. Doing it with integer math is the "difficult" > part... > > Cheers, > Peter
On 2021-05-31 16:51, Liam Beguin wrote: > On Mon May 31, 2021 at 10:08 AM EDT, Peter Rosin wrote: >> On 2021-05-31 15:36, Liam Beguin wrote: >>> Hi Peter, >>> >>> On Mon May 31, 2021 at 4:52 AM EDT, Peter Rosin wrote: >>>> Hi! >>>> >>>> Thanks for the patch! >>>> >>>> On 2021-05-30 02:59, Liam Beguin wrote: >>>>> From: Liam Beguin <lvb@xiphos.com> >>>>> >>>>> This is a preparatory change required for the addition of temperature >>>>> sensing front ends. >>>> >>>> I think this is too simplistic. I think that if the upstream iio-dev has >>>> an offset, it should be dealt with (i.e. be rescaled). The rescale >>>> driver >>>> cannot ignore such an upstream offset and then throw in some other >>>> unrelated offset of its own. That would be thoroughly confusing. >>> >>> I'm not sure I fully understand. The upstream offset should be dealt >>> with when calling iio_read_channel_processed(). That was my main >>> motivation behind using the IIO core to get a processed value. >> >> You can rescale a channel with an offset, but without using processed >> values. I.e. the upstream channel provides raw values, a scale and an >> offset. The current rescale code ignores the upstream offset. I did not >> need that when I created the driver, and at a glace it felt "difficult". >> So I punted. > > I understand what you meant now. > > At first, I tried to apply the upstream offset from inside the rescaler. > As you said it felt difficult and it felt like this must've been > implemented somewhere else before. > > After looking around, I noticed that the code to do that was already > part of inkern.c and exposed through iio_read_channel_processed(). > If the upstream channel doesn't provide a processed value, the upstream > offset and scale are automatically applied. > > So with the changes in [3/9] the rescaler's raw value becomes the > upstream channel's processed value. > > This seems like an easier and probably cleaner way of adding offset > support in the rescaler. > > Does that make sense? Yes, it does. Doing generic calculations like this efficiently with integer math without losing precision is ... difficult. I think that perhaps IF the upstream channel has an offset, the rescaler could revert to always use the upstream processed channel in preference of the raw channel. That would fix the missing support for upstream offset and still not penalize the sweet case of no upstream offset. Because the processed channel costs processing for each and every sample and I think it should be avoided as much as possible. Does that make sense? Or are a bunch of drivers adding an explicit zero offset "just because"? That would be a nuisance. Cheers, Peter
On Mon May 31, 2021 at 12:25 PM EDT, Peter Rosin wrote: > On 2021-05-31 16:51, Liam Beguin wrote: > > On Mon May 31, 2021 at 10:08 AM EDT, Peter Rosin wrote: > >> On 2021-05-31 15:36, Liam Beguin wrote: > >>> Hi Peter, > >>> > >>> On Mon May 31, 2021 at 4:52 AM EDT, Peter Rosin wrote: > >>>> Hi! > >>>> > >>>> Thanks for the patch! > >>>> > >>>> On 2021-05-30 02:59, Liam Beguin wrote: > >>>>> From: Liam Beguin <lvb@xiphos.com> > >>>>> > >>>>> This is a preparatory change required for the addition of temperature > >>>>> sensing front ends. > >>>> > >>>> I think this is too simplistic. I think that if the upstream iio-dev has > >>>> an offset, it should be dealt with (i.e. be rescaled). The rescale > >>>> driver > >>>> cannot ignore such an upstream offset and then throw in some other > >>>> unrelated offset of its own. That would be thoroughly confusing. > >>> > >>> I'm not sure I fully understand. The upstream offset should be dealt > >>> with when calling iio_read_channel_processed(). That was my main > >>> motivation behind using the IIO core to get a processed value. > >> > >> You can rescale a channel with an offset, but without using processed > >> values. I.e. the upstream channel provides raw values, a scale and an > >> offset. The current rescale code ignores the upstream offset. I did not > >> need that when I created the driver, and at a glace it felt "difficult". > >> So I punted. > > > > I understand what you meant now. > > > > At first, I tried to apply the upstream offset from inside the rescaler. > > As you said it felt difficult and it felt like this must've been > > implemented somewhere else before. > > > > After looking around, I noticed that the code to do that was already > > part of inkern.c and exposed through iio_read_channel_processed(). > > If the upstream channel doesn't provide a processed value, the upstream > > offset and scale are automatically applied. > > > > So with the changes in [3/9] the rescaler's raw value becomes the > > upstream channel's processed value. > > > > This seems like an easier and probably cleaner way of adding offset > > support in the rescaler. > > > > Does that make sense? > > Yes, it does. Doing generic calculations like this efficiently with > integer math without losing precision is ... difficult. You're right, I realized it's more complicated that it seems working on this. > > I think that perhaps IF the upstream channel has an offset, the > rescaler could revert to always use the upstream processed channel in > preference of the raw channel. That would fix the missing support for > upstream offset and still not penalize the sweet case of no upstream > offset. Because the processed channel costs processing for each and > every sample and I think it should be avoided as much as possible. > > Does that make sense? Totally, I see what you're saying and will give it a try. I still believe it would make sense to get the upstream scaling factor the same way, to avoid duplicating that code. Also it might be confusing to have the raw value be the upstream raw value in some cases and the upstream processed value in others. > > Or are a bunch of drivers adding an explicit zero offset "just because"? > That would be a nuisance. A quick search seems to indicate that this isn't the case. Thanks for your time, Liam > > Cheers, > Peter
On Mon, 31 May 2021 13:42:09 -0400 "Liam Beguin" <liambeguin@gmail.com> wrote: > On Mon May 31, 2021 at 12:25 PM EDT, Peter Rosin wrote: > > On 2021-05-31 16:51, Liam Beguin wrote: > > > On Mon May 31, 2021 at 10:08 AM EDT, Peter Rosin wrote: > > >> On 2021-05-31 15:36, Liam Beguin wrote: > > >>> Hi Peter, > > >>> > > >>> On Mon May 31, 2021 at 4:52 AM EDT, Peter Rosin wrote: > > >>>> Hi! > > >>>> > > >>>> Thanks for the patch! > > >>>> > > >>>> On 2021-05-30 02:59, Liam Beguin wrote: > > >>>>> From: Liam Beguin <lvb@xiphos.com> > > >>>>> > > >>>>> This is a preparatory change required for the addition of temperature > > >>>>> sensing front ends. > > >>>> > > >>>> I think this is too simplistic. I think that if the upstream iio-dev has > > >>>> an offset, it should be dealt with (i.e. be rescaled). The rescale > > >>>> driver > > >>>> cannot ignore such an upstream offset and then throw in some other > > >>>> unrelated offset of its own. That would be thoroughly confusing. > > >>> > > >>> I'm not sure I fully understand. The upstream offset should be dealt > > >>> with when calling iio_read_channel_processed(). That was my main > > >>> motivation behind using the IIO core to get a processed value. > > >> > > >> You can rescale a channel with an offset, but without using processed > > >> values. I.e. the upstream channel provides raw values, a scale and an > > >> offset. The current rescale code ignores the upstream offset. I did not > > >> need that when I created the driver, and at a glace it felt "difficult". > > >> So I punted. > > > > > > I understand what you meant now. > > > > > > At first, I tried to apply the upstream offset from inside the rescaler. > > > As you said it felt difficult and it felt like this must've been > > > implemented somewhere else before. > > > > > > After looking around, I noticed that the code to do that was already > > > part of inkern.c and exposed through iio_read_channel_processed(). > > > If the upstream channel doesn't provide a processed value, the upstream > > > offset and scale are automatically applied. > > > > > > So with the changes in [3/9] the rescaler's raw value becomes the > > > upstream channel's processed value. > > > > > > This seems like an easier and probably cleaner way of adding offset > > > support in the rescaler. > > > > > > Does that make sense? > > > > Yes, it does. Doing generic calculations like this efficiently with > > integer math without losing precision is ... difficult. > > You're right, I realized it's more complicated that it seems working on > this. Yup, particularly given the processed version doesn't work because of scale precision loss. To avoid that mess you would have to do the maths to rescale the offset. If we assume offsets are integer (not always true, but often true for ADCs) then it wouldn't be too bad, but you will need to handle all the different ways scale can be specified (or support a subset perhaps). > > > > > I think that perhaps IF the upstream channel has an offset, the > > rescaler could revert to always use the upstream processed channel in > > preference of the raw channel. That would fix the missing support for > > upstream offset and still not penalize the sweet case of no upstream > > offset. Because the processed channel costs processing for each and > > every sample and I think it should be avoided as much as possible. > > > > Does that make sense? > > Totally, I see what you're saying and will give it a try. > > I still believe it would make sense to get the upstream scaling factor > the same way, to avoid duplicating that code. > > Also it might be confusing to have the raw value be the upstream raw > value in some cases and the upstream processed value in others. > > > > > Or are a bunch of drivers adding an explicit zero offset "just because"? > > That would be a nuisance. > > A quick search seems to indicate that this isn't the case. We were pretty rigorous about this, though there are drivers that have variable offsets where it will 'sometimes' be 0. It's an interesting corner, that we've been avoiding, but probably not too bad to do at least the common combinations. The fun will come when you are trying to sensible combine a scaled offset and your new offset and need to do integer maths for. A/(2**B) + C.D There will probably be cases where we just take a decent stab at it and assume precision might not be great. Jonathan > > Thanks for your time, > Liam > > > > > Cheers, > > Peter >
diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c index 4d0813b274d1..3bd1f11f21db 100644 --- a/drivers/iio/afe/iio-rescale.c +++ b/drivers/iio/afe/iio-rescale.c @@ -31,6 +31,7 @@ struct rescale { struct iio_chan_spec_ext_info *ext_info; s32 numerator; s32 denominator; + s32 offset; }; static int rescale_read_raw(struct iio_dev *indio_dev, @@ -52,6 +53,10 @@ static int rescale_read_raw(struct iio_dev *indio_dev, *val2 = rescale->denominator; return IIO_VAL_FRACTIONAL; + case IIO_CHAN_INFO_OFFSET: + *val = rescale->offset; + + return IIO_VAL_INT; default: return -EINVAL; } @@ -119,8 +124,10 @@ static int rescale_configure_channel(struct device *dev, return -EINVAL; } - chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | - BIT(IIO_CHAN_INFO_SCALE); + chan->info_mask_separate = + BIT(IIO_CHAN_INFO_RAW) | + BIT(IIO_CHAN_INFO_SCALE) | + BIT(IIO_CHAN_INFO_OFFSET); if (iio_channel_has_available(schan, IIO_CHAN_INFO_RAW)) chan->info_mask_separate_available |= BIT(IIO_CHAN_INFO_RAW); @@ -280,6 +287,7 @@ static int rescale_probe(struct platform_device *pdev) rescale->cfg = of_device_get_match_data(dev); rescale->numerator = 1; rescale->denominator = 1; + rescale->offset = 0; ret = rescale->cfg->props(dev, rescale); if (ret)