On Mon, 31 May 2021 13:42:09 -0400
"Liam Beguin" [off-list ref] wrote:

On Mon May 31, 2021 at 12:25 PM EDT, Peter Rosin wrote:

quoted

On 2021-05-31 16:51, Liam Beguin wrote:  

quoted

On Mon May 31, 2021 at 10:08 AM EDT, Peter Rosin wrote:  

quoted

On 2021-05-31 15:36, Liam Beguin wrote:  

quoted

Hi Peter,

On Mon May 31, 2021 at 4:52 AM EDT, Peter Rosin wrote:  

quoted

Hi!

Thanks for the patch!

On 2021-05-30 02:59, Liam Beguin wrote:  

quoted

From: Liam Beguin <redacted>

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).

quoted

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.

quoted

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

quoted

Cheers,
Peter