Message ID | 20241217-ad4695-oversampling-v1-1-0b045d835dac@baylibre.com (mailing list archive) |
---|---|
State | Changes Requested |
Headers | show |
Series | iio: adc: ad4695: add oversampling support | expand |
On Tue, 17 Dec 2024 16:47:28 -0500 Trevor Gamblin <tgamblin@baylibre.com> wrote: > Add support for the ad4695's oversampling feature when SPI offload is > available. This allows the ad4695 to set oversampling ratios on a > per-channel basis, raising the effective-number-of-bits from 16 > (OSR == 1) to 17 (4), 18 (16), or 19 (64) for a given sample (i.e. one > full cycle through the auto-sequencer). The logic for reading and > writing sampling frequency for a given channel is also adjusted based on > the current oversampling ratio. > > The non-offload case isn't supported as there isn't a good way to > trigger the CNV pin in this mode. Support could be added in the future > if a use-case arises. > > Signed-off-by: Trevor Gamblin <tgamblin@baylibre.com> Hi Trevor, The clamping fun of get_calibbias seems overkill. If this isn't going to ever overflow an s64 maybe just use the high precision to do it the easy way. I'm not sure you can't just fit it in an s32 for that matter. I've just not done the maths to check. Jonathan > +static unsigned int ad4695_get_calibbias(int val, int val2, int osr) > +{ > + unsigned int reg_val; > + > + switch (osr) { > + case 4: > + if (val2 >= 0 && val > S16_MAX / 2) > + reg_val = S16_MAX; > + else if ((val2 < 0 ? -val : val) < S16_MIN / 2) It has been a while, but IIRC if val2 < 0 then val == 0 as otherwise we carry the sign in the val part. Sometimes we generalize that to make life easier for driver writers but I think you can use that here to simplify things. (for background look at __iio_str_to_fixpoint() - it's a bit of a hack to deal with integers have no negative 0) if (val > S16_MAX / 2) ... else if (val < S16_MIN / 2) ... else if (val2 < 0) etc You may feel it is better to keep the code considering the val2 < 0 when val != 0 case and I don't mind that as it's not wrong, just overly complex! If you can easily clamp the overall range you can just do some maths with enough precision to get one number (probably a s64) and clamp that. Easy to sanity check for overflow based on val to ensure no overflows. > + reg_val = S16_MIN; > + else if (val2 < 0) > + reg_val = clamp_t(int, > + -(val * 2 + -val2 * 2 / MICRO), > + S16_MIN, S16_MAX); > + else if (val < 0) > + reg_val = clamp_t(int, > + val * 2 - val2 * 2 / MICRO, > + S16_MIN, S16_MAX); > + else > + reg_val = clamp_t(int, > + val * 2 + val2 * 2 / MICRO, > + S16_MIN, S16_MAX); > + return reg_val; > + case 16: > + if (val2 >= 0 && val > S16_MAX) > + reg_val = S16_MAX; > + else if ((val2 < 0 ? -val : val) < S16_MIN) > + reg_val = S16_MIN; > + else if (val2 < 0) > + reg_val = clamp_t(int, > + -(val + -val2 / MICRO), > + S16_MIN, S16_MAX); > + else if (val < 0) > + reg_val = clamp_t(int, > + val - val2 / MICRO, > + S16_MIN, S16_MAX); > + else > + reg_val = clamp_t(int, > + val + val2 / MICRO, > + S16_MIN, S16_MAX); > + return reg_val; > + case 64: > + if (val2 >= 0 && val > S16_MAX * 2) > + reg_val = S16_MAX; > + else if ((val2 < 0 ? -val : val) < S16_MIN * 2) > + reg_val = S16_MIN; > + else if (val2 < 0) > + reg_val = clamp_t(int, > + -(val / 2 + -val2 / 2 / MICRO), > + S16_MIN, S16_MAX); > + else if (val < 0) > + reg_val = clamp_t(int, > + val / 2 - val2 / 2 / MICRO, > + S16_MIN, S16_MAX); > + else > + reg_val = clamp_t(int, > + val / 2 + val2 / 2 / MICRO, > + S16_MIN, S16_MAX); > + return reg_val; > + default: > + if (val2 >= 0 && val > S16_MAX / 4) > + reg_val = S16_MAX; > + else if ((val2 < 0 ? -val : val) < S16_MIN / 4) > + reg_val = S16_MIN; > + else if (val2 < 0) > + reg_val = clamp_t(int, > + -(val * 4 + -val2 * 4 / MICRO), > + S16_MIN, S16_MAX); > + else if (val < 0) > + reg_val = clamp_t(int, > + val * 4 - val2 * 4 / MICRO, > + S16_MIN, S16_MAX); > + else > + reg_val = clamp_t(int, > + val * 4 + val2 * 4 / MICRO, > + S16_MIN, S16_MAX); > + return reg_val; > + } > +} > +
On 2024-12-19 11:13, Jonathan Cameron wrote: > On Tue, 17 Dec 2024 16:47:28 -0500 > Trevor Gamblin <tgamblin@baylibre.com> wrote: > >> Add support for the ad4695's oversampling feature when SPI offload is >> available. This allows the ad4695 to set oversampling ratios on a >> per-channel basis, raising the effective-number-of-bits from 16 >> (OSR == 1) to 17 (4), 18 (16), or 19 (64) for a given sample (i.e. one >> full cycle through the auto-sequencer). The logic for reading and >> writing sampling frequency for a given channel is also adjusted based on >> the current oversampling ratio. >> >> The non-offload case isn't supported as there isn't a good way to >> trigger the CNV pin in this mode. Support could be added in the future >> if a use-case arises. >> >> Signed-off-by: Trevor Gamblin <tgamblin@baylibre.com> > Hi Trevor, > > The clamping fun of get_calibbias seems overkill. If this isn't going to ever > overflow an s64 maybe just use the high precision to do it the easy way. > I'm not sure you can't just fit it in an s32 for that matter. I've just > not done the maths to check. Hi Jonathan, Thanks for your feedback. I'll look it over again, do some testing, and see if it can be simplified. Trevor
On 2024-12-19 11:13, Jonathan Cameron wrote: > On Tue, 17 Dec 2024 16:47:28 -0500 > Trevor Gamblin <tgamblin@baylibre.com> wrote: > >> Add support for the ad4695's oversampling feature when SPI offload is >> available. This allows the ad4695 to set oversampling ratios on a >> per-channel basis, raising the effective-number-of-bits from 16 >> (OSR == 1) to 17 (4), 18 (16), or 19 (64) for a given sample (i.e. one >> full cycle through the auto-sequencer). The logic for reading and >> writing sampling frequency for a given channel is also adjusted based on >> the current oversampling ratio. >> >> The non-offload case isn't supported as there isn't a good way to >> trigger the CNV pin in this mode. Support could be added in the future >> if a use-case arises. >> >> Signed-off-by: Trevor Gamblin <tgamblin@baylibre.com> > Hi Trevor, > > The clamping fun of get_calibbias seems overkill. If this isn't going to ever > overflow an s64 maybe just use the high precision to do it the easy way. > I'm not sure you can't just fit it in an s32 for that matter. I've just > not done the maths to check. > > Jonathan > > >> +static unsigned int ad4695_get_calibbias(int val, int val2, int osr) >> +{ >> + unsigned int reg_val; >> + >> + switch (osr) { >> + case 4: >> + if (val2 >= 0 && val > S16_MAX / 2) >> + reg_val = S16_MAX; >> + else if ((val2 < 0 ? -val : val) < S16_MIN / 2) > It has been a while, but IIRC if val2 < 0 then val == 0 as otherwise > we carry the sign in the val part. Sometimes we generalize that to > make life easier for driver writers but I think you can use that here > to simplify things. > > (for background look at __iio_str_to_fixpoint() - it's a bit of a hack > to deal with integers have no negative 0) > > if (val > S16_MAX / 2) > ... > else if (val < S16_MIN / 2) > ... > else if (val2 < 0) etc > > You may feel it is better to keep the code considering the val2 < 0 when > val != 0 case and I don't mind that as it's not wrong, just overly complex! > > If you can easily clamp the overall range you can just do some maths > with enough precision to get one number (probably a s64) and clamp that. > Easy to sanity check for overflow based on val to ensure no overflows. Hi Jonathan, I'm reviewing this again but I'm not entirely clear what you mean. Are you suggesting that the entire switch block could be simplified (i.e. eliminating the previous simplification for the val2 < 0 case in the process), or that the calls to clamp_t can be combined? I've tested out simplifying the val2 < 0 case locally and driver functionality still seems OK. Maybe I'm missing a third option. - Trevor > > > > >> + reg_val = S16_MIN; >> + else if (val2 < 0) >> + reg_val = clamp_t(int, >> + -(val * 2 + -val2 * 2 / MICRO), >> + S16_MIN, S16_MAX); >> + else if (val < 0) >> + reg_val = clamp_t(int, >> + val * 2 - val2 * 2 / MICRO, >> + S16_MIN, S16_MAX); >> + else >> + reg_val = clamp_t(int, >> + val * 2 + val2 * 2 / MICRO, >> + S16_MIN, S16_MAX); >> + return reg_val; >> + case 16: >> + if (val2 >= 0 && val > S16_MAX) >> + reg_val = S16_MAX; >> + else if ((val2 < 0 ? -val : val) < S16_MIN) >> + reg_val = S16_MIN; >> + else if (val2 < 0) >> + reg_val = clamp_t(int, >> + -(val + -val2 / MICRO), >> + S16_MIN, S16_MAX); >> + else if (val < 0) >> + reg_val = clamp_t(int, >> + val - val2 / MICRO, >> + S16_MIN, S16_MAX); >> + else >> + reg_val = clamp_t(int, >> + val + val2 / MICRO, >> + S16_MIN, S16_MAX); >> + return reg_val; >> + case 64: >> + if (val2 >= 0 && val > S16_MAX * 2) >> + reg_val = S16_MAX; >> + else if ((val2 < 0 ? -val : val) < S16_MIN * 2) >> + reg_val = S16_MIN; >> + else if (val2 < 0) >> + reg_val = clamp_t(int, >> + -(val / 2 + -val2 / 2 / MICRO), >> + S16_MIN, S16_MAX); >> + else if (val < 0) >> + reg_val = clamp_t(int, >> + val / 2 - val2 / 2 / MICRO, >> + S16_MIN, S16_MAX); >> + else >> + reg_val = clamp_t(int, >> + val / 2 + val2 / 2 / MICRO, >> + S16_MIN, S16_MAX); >> + return reg_val; >> + default: >> + if (val2 >= 0 && val > S16_MAX / 4) >> + reg_val = S16_MAX; >> + else if ((val2 < 0 ? -val : val) < S16_MIN / 4) >> + reg_val = S16_MIN; >> + else if (val2 < 0) >> + reg_val = clamp_t(int, >> + -(val * 4 + -val2 * 4 / MICRO), >> + S16_MIN, S16_MAX); >> + else if (val < 0) >> + reg_val = clamp_t(int, >> + val * 4 - val2 * 4 / MICRO, >> + S16_MIN, S16_MAX); >> + else >> + reg_val = clamp_t(int, >> + val * 4 + val2 * 4 / MICRO, >> + S16_MIN, S16_MAX); >> + return reg_val; >> + } >> +} >> +
On Thu, 2 Jan 2025 13:19:19 -0500 Trevor Gamblin <tgamblin@baylibre.com> wrote: > On 2024-12-19 11:13, Jonathan Cameron wrote: > > On Tue, 17 Dec 2024 16:47:28 -0500 > > Trevor Gamblin <tgamblin@baylibre.com> wrote: > > > >> Add support for the ad4695's oversampling feature when SPI offload is > >> available. This allows the ad4695 to set oversampling ratios on a > >> per-channel basis, raising the effective-number-of-bits from 16 > >> (OSR == 1) to 17 (4), 18 (16), or 19 (64) for a given sample (i.e. one > >> full cycle through the auto-sequencer). The logic for reading and > >> writing sampling frequency for a given channel is also adjusted based on > >> the current oversampling ratio. > >> > >> The non-offload case isn't supported as there isn't a good way to > >> trigger the CNV pin in this mode. Support could be added in the future > >> if a use-case arises. > >> > >> Signed-off-by: Trevor Gamblin <tgamblin@baylibre.com> > > Hi Trevor, > > > > The clamping fun of get_calibbias seems overkill. If this isn't going to ever > > overflow an s64 maybe just use the high precision to do it the easy way. > > I'm not sure you can't just fit it in an s32 for that matter. I've just > > not done the maths to check. > > > > Jonathan > > > > > >> +static unsigned int ad4695_get_calibbias(int val, int val2, int osr) > >> +{ > >> + unsigned int reg_val; > >> + > >> + switch (osr) { > >> + case 4: > >> + if (val2 >= 0 && val > S16_MAX / 2) > >> + reg_val = S16_MAX; > >> + else if ((val2 < 0 ? -val : val) < S16_MIN / 2) > > It has been a while, but IIRC if val2 < 0 then val == 0 as otherwise > > we carry the sign in the val part. Sometimes we generalize that to > > make life easier for driver writers but I think you can use that here > > to simplify things. > > > > (for background look at __iio_str_to_fixpoint() - it's a bit of a hack > > to deal with integers have no negative 0) > > > > if (val > S16_MAX / 2) > > ... > > else if (val < S16_MIN / 2) > > ... > > else if (val2 < 0) etc > > > > You may feel it is better to keep the code considering the val2 < 0 when > > val != 0 case and I don't mind that as it's not wrong, just overly complex! > > > > If you can easily clamp the overall range you can just do some maths > > with enough precision to get one number (probably a s64) and clamp that. > > Easy to sanity check for overflow based on val to ensure no overflows. > > Hi Jonathan, > > I'm reviewing this again but I'm not entirely clear what you mean. > > Are you suggesting that the entire switch block could be simplified > (i.e. eliminating the previous simplification for the val2 < 0 case in > the process), or that the calls to clamp_t can be combined? > > I've tested out simplifying the val2 < 0 case locally and driver > functionality still seems OK. Maybe I'm missing a third option. The extra info we can use is that val2 is always positive if val != 0 and it never takes a value beyond +- MICRO because otherwise val would be non 0 instead. Taking original code and ruling out cases. + case 4: + if (val2 >= 0 && val > S16_MAX / 2) // If val is non 0 then val2 is postive, so // if (val > S16_MAX / 2) // reg_val = S16_MAX; + reg_val = S16_MAX; + else if ((val2 < 0 ? -val : val) < S16_MIN / 2) // If val2 < 0 then val == 0 which is never less than S16_MIN / 2 // So this condition never happens. + reg_val = S16_MIN; + else if (val2 < 0) // likewise, this is actually clamping val2 * 2 / MICRO which // is never going to be anywhere near S16_MIN or S16_MAX as I think // it is always between +1 and -1 as val2 itself is limited to -MICRO to MICRO + reg_val = clamp_t(int, + -(val * 2 + -val2 * 2 / MICRO), + S16_MIN, S16_MAX); + else if (val < 0) //This one is fine. + reg_val = clamp_t(int, + val * 2 - val2 * 2 / MICRO, + S16_MIN, S16_MAX); + else //As is this one + reg_val = clamp_t(int, + val * 2 + val2 * 2 / MICRO, + S16_MIN, S16_MAX); + return reg_val; Maybe trick is to reorder into 3 conditions and set the value in a temporary integer. int val_calc; if (val > 0) val_calc = val * 2 + val2 * 2 / MICRO; else if (val < 0) val_calc = -(val * 2 - val2 * 2 / MICRO); else /* Only now does val2 sign matter as val == 0 */ val_calc = val2 * 2 / MICRO; Which can simplify because we know val is 0 for last case. Whether this is worth doing depends on trade off between docs needed to explain the code and shorter code. /* Note that val2 > 0 if val != 0 and val2 range +- MICRO */ if (val < 0) val_calc = val * 2 - val2 * 2 / MICRO; else val_calc = val * 2 + val2 * 2 / MICRO; reg_val = clamp_t(int, val_calc, S16_MIN, S16_MAX); One trivial additional simplication below. You might also be able to scale temporary up by 2 and ust have the switch statement set a scaling value. In this case scale == 4 in other cases below, 2, 1, and 8 for the default if (val < 0) val_calc = val * scale - val2 * scale / MICRO; else val_calc = val * scale + val2 * scale / MICRO; val_calc /= 2; /* to remove the factor of 2 */ reg_val = clamp_t (int, val_calc, S16_MIN, S16_MAX); after the switch statement with comments when setting scale on the * 2 multiplier to avoid the / 2 for case 64. > > - Trevor > > > > > > > > > > >> + reg_val = S16_MIN; > >> + else if (val2 < 0) > >> + reg_val = clamp_t(int, > >> + -(val * 2 + -val2 * 2 / MICRO), > >> + S16_MIN, S16_MAX); > >> + else if (val < 0) > >> + reg_val = clamp_t(int, > >> + val * 2 - val2 * 2 / MICRO, > >> + S16_MIN, S16_MAX); > >> + else > >> + reg_val = clamp_t(int, > >> + val * 2 + val2 * 2 / MICRO, > >> + S16_MIN, S16_MAX); > >> + return reg_val; > >> + case 16: > >> + if (val2 >= 0 && val > S16_MAX) > >> + reg_val = S16_MAX; > >> + else if ((val2 < 0 ? -val : val) < S16_MIN) > >> + reg_val = S16_MIN; > >> + else if (val2 < 0) > >> + reg_val = clamp_t(int, > >> + -(val + -val2 / MICRO), > >> + S16_MIN, S16_MAX); > >> + else if (val < 0) > >> + reg_val = clamp_t(int, > >> + val - val2 / MICRO, > >> + S16_MIN, S16_MAX); > >> + else > >> + reg_val = clamp_t(int, > >> + val + val2 / MICRO, > >> + S16_MIN, S16_MAX); > >> + return reg_val; > >> + case 64: > >> + if (val2 >= 0 && val > S16_MAX * 2) > >> + reg_val = S16_MAX; > >> + else if ((val2 < 0 ? -val : val) < S16_MIN * 2) > >> + reg_val = S16_MIN; > >> + else if (val2 < 0) > >> + reg_val = clamp_t(int, > >> + -(val / 2 + -val2 / 2 / MICRO), > >> + S16_MIN, S16_MAX); > >> + else if (val < 0) > >> + reg_val = clamp_t(int, > >> + val / 2 - val2 / 2 / MICRO, For these val2 / 2 / MICRO always 0 so value of val2 never matters. > >> + S16_MIN, S16_MAX); > >> + else > >> + reg_val = clamp_t(int, > >> + val / 2 + val2 / 2 / MICRO, > >> + S16_MIN, S16_MAX); > >> + return reg_val; > >> + default: > >> + if (val2 >= 0 && val > S16_MAX / 4) > >> + reg_val = S16_MAX; > >> + else if ((val2 < 0 ? -val : val) < S16_MIN / 4) > >> + reg_val = S16_MIN; > >> + else if (val2 < 0) > >> + reg_val = clamp_t(int, > >> + -(val * 4 + -val2 * 4 / MICRO), > >> + S16_MIN, S16_MAX); > >> + else if (val < 0) > >> + reg_val = clamp_t(int, > >> + val * 4 - val2 * 4 / MICRO, > >> + S16_MIN, S16_MAX); > >> + else > >> + reg_val = clamp_t(int, > >> + val * 4 + val2 * 4 / MICRO, > >> + S16_MIN, S16_MAX); > >> + return reg_val; > >> + } > >> +} > >> +
On 2025-01-04 07:30, Jonathan Cameron wrote: > On Thu, 2 Jan 2025 13:19:19 -0500 > Trevor Gamblin <tgamblin@baylibre.com> wrote: > >> On 2024-12-19 11:13, Jonathan Cameron wrote: >>> On Tue, 17 Dec 2024 16:47:28 -0500 >>> Trevor Gamblin <tgamblin@baylibre.com> wrote: >>> >>>> Add support for the ad4695's oversampling feature when SPI offload is >>>> available. This allows the ad4695 to set oversampling ratios on a >>>> per-channel basis, raising the effective-number-of-bits from 16 >>>> (OSR == 1) to 17 (4), 18 (16), or 19 (64) for a given sample (i.e. one >>>> full cycle through the auto-sequencer). The logic for reading and >>>> writing sampling frequency for a given channel is also adjusted based on >>>> the current oversampling ratio. >>>> >>>> The non-offload case isn't supported as there isn't a good way to >>>> trigger the CNV pin in this mode. Support could be added in the future >>>> if a use-case arises. >>>> >>>> Signed-off-by: Trevor Gamblin <tgamblin@baylibre.com> >>> Hi Trevor, >>> >>> The clamping fun of get_calibbias seems overkill. If this isn't going to ever >>> overflow an s64 maybe just use the high precision to do it the easy way. >>> I'm not sure you can't just fit it in an s32 for that matter. I've just >>> not done the maths to check. >>> >>> Jonathan >>> >>> >>>> +static unsigned int ad4695_get_calibbias(int val, int val2, int osr) >>>> +{ >>>> + unsigned int reg_val; >>>> + >>>> + switch (osr) { >>>> + case 4: >>>> + if (val2 >= 0 && val > S16_MAX / 2) >>>> + reg_val = S16_MAX; >>>> + else if ((val2 < 0 ? -val : val) < S16_MIN / 2) >>> It has been a while, but IIRC if val2 < 0 then val == 0 as otherwise >>> we carry the sign in the val part. Sometimes we generalize that to >>> make life easier for driver writers but I think you can use that here >>> to simplify things. >>> >>> (for background look at __iio_str_to_fixpoint() - it's a bit of a hack >>> to deal with integers have no negative 0) >>> >>> if (val > S16_MAX / 2) >>> ... >>> else if (val < S16_MIN / 2) >>> ... >>> else if (val2 < 0) etc >>> >>> You may feel it is better to keep the code considering the val2 < 0 when >>> val != 0 case and I don't mind that as it's not wrong, just overly complex! >>> >>> If you can easily clamp the overall range you can just do some maths >>> with enough precision to get one number (probably a s64) and clamp that. >>> Easy to sanity check for overflow based on val to ensure no overflows. >> Hi Jonathan, >> >> I'm reviewing this again but I'm not entirely clear what you mean. >> >> Are you suggesting that the entire switch block could be simplified >> (i.e. eliminating the previous simplification for the val2 < 0 case in >> the process), or that the calls to clamp_t can be combined? >> >> I've tested out simplifying the val2 < 0 case locally and driver >> functionality still seems OK. Maybe I'm missing a third option. Hi Jonathan, > The extra info we can use is that val2 is always positive > if val != 0 and it never takes a value beyond +- MICRO because > otherwise val would be non 0 instead. > > > Taking original code and ruling out cases. > + case 4: > + if (val2 >= 0 && val > S16_MAX / 2) > // If val is non 0 then val2 is postive, so > // if (val > S16_MAX / 2) > // reg_val = S16_MAX; > > + reg_val = S16_MAX; > + else if ((val2 < 0 ? -val : val) < S16_MIN / 2) > > // If val2 < 0 then val == 0 which is never less than S16_MIN / 2 > // So this condition never happens. Thanks for catching these. > > + reg_val = S16_MIN; > + else if (val2 < 0) > // likewise, this is actually clamping val2 * 2 / MICRO which > // is never going to be anywhere near S16_MIN or S16_MAX as I think > // it is always between +1 and -1 as val2 itself is limited to -MICRO to MICRO > > + reg_val = clamp_t(int, > + -(val * 2 + -val2 * 2 / MICRO), > + S16_MIN, S16_MAX); > + else if (val < 0) > //This one is fine. > + reg_val = clamp_t(int, > + val * 2 - val2 * 2 / MICRO, > + S16_MIN, S16_MAX); > + else > //As is this one > + reg_val = clamp_t(int, > + val * 2 + val2 * 2 / MICRO, > + S16_MIN, S16_MAX); > + return reg_val; > > Maybe trick is to reorder into 3 conditions and set the value in a temporary integer. > int val_calc; > if (val > 0) > val_calc = val * 2 + val2 * 2 / MICRO; > else if (val < 0) > val_calc = -(val * 2 - val2 * 2 / MICRO); > else /* Only now does val2 sign matter as val == 0 */ > val_calc = val2 * 2 / MICRO; I've been testing out these simplifications (but using the scaling suggestion from below, which is great - for some reason I had it in my head that doing so wasn't an option). These seem to have some issues with signs for particularly small calibbias values. I think it's because while my (val2 < 0) case was doing unnecessary clamping, the math itself was OK. I did some more experimenting, and came up with a new version of the function that looks like this: static unsigned int ad4695_get_calibbias(int val, int val2, int osr) { int val_calc, scale; switch (osr) { case 4: scale = 4; break; case 16: scale = 2; break; case 64: scale = 1; break; default: scale = 8; break; } /* Note that val2 > 0 if val != 0 and val2 range +- MICRO */ if (val < 0) val_calc = val * scale - val2 * scale / MICRO; else if (val2 < 0) /* if val2 < 0 then val == 0 */ val_calc = -(-val2 * scale / MICRO); else val_calc = val * scale + val2 * scale / MICRO; val_calc /= 2; return clamp_t(int, val_calc, S16_MIN, S16_MAX); } This seems to match all of the expected outputs for the pre-simplification version in this patch series when I test it. If there are no issues with it, I'll send a v2. > > Which can simplify because we know val is 0 for last case. > Whether this is worth doing depends on trade off between > docs needed to explain the code and shorter code. > > /* Note that val2 > 0 if val != 0 and val2 range +- MICRO */ > if (val < 0) > val_calc = val * 2 - val2 * 2 / MICRO; > else > val_calc = val * 2 + val2 * 2 / MICRO; > > reg_val = clamp_t(int, val_calc, S16_MIN, S16_MAX); > > One trivial additional simplication below. > > You might also be able to scale temporary up by 2 and ust > have the switch statement set a scaling value. > > In this case scale == 4 in other cases below, 2, 1, and 8 for the default > > > if (val < 0) > val_calc = val * scale - val2 * scale / MICRO; > else > val_calc = val * scale + val2 * scale / MICRO; > > val_calc /= 2; /* to remove the factor of 2 */ > > reg_val = clamp_t (int, val_calc, S16_MIN, S16_MAX); > after the switch statement with comments when setting scale on the * 2 > multiplier to avoid the / 2 for case 64. > >> - Trevor >> >>> >>> >>> >>>> + reg_val = S16_MIN; >>>> + else if (val2 < 0) >>>> + reg_val = clamp_t(int, >>>> + -(val * 2 + -val2 * 2 / MICRO), >>>> + S16_MIN, S16_MAX); >>>> + else if (val < 0) >>>> + reg_val = clamp_t(int, >>>> + val * 2 - val2 * 2 / MICRO, >>>> + S16_MIN, S16_MAX); >>>> + else >>>> + reg_val = clamp_t(int, >>>> + val * 2 + val2 * 2 / MICRO, >>>> + S16_MIN, S16_MAX); >>>> + return reg_val; >>>> + case 16: >>>> + if (val2 >= 0 && val > S16_MAX) >>>> + reg_val = S16_MAX; >>>> + else if ((val2 < 0 ? -val : val) < S16_MIN) >>>> + reg_val = S16_MIN; >>>> + else if (val2 < 0) >>>> + reg_val = clamp_t(int, >>>> + -(val + -val2 / MICRO), >>>> + S16_MIN, S16_MAX); >>>> + else if (val < 0) >>>> + reg_val = clamp_t(int, >>>> + val - val2 / MICRO, >>>> + S16_MIN, S16_MAX); >>>> + else >>>> + reg_val = clamp_t(int, >>>> + val + val2 / MICRO, >>>> + S16_MIN, S16_MAX); >>>> + return reg_val; >>>> + case 64: >>>> + if (val2 >= 0 && val > S16_MAX * 2) >>>> + reg_val = S16_MAX; >>>> + else if ((val2 < 0 ? -val : val) < S16_MIN * 2) >>>> + reg_val = S16_MIN; >>>> + else if (val2 < 0) >>>> + reg_val = clamp_t(int, >>>> + -(val / 2 + -val2 / 2 / MICRO), >>>> + S16_MIN, S16_MAX); >>>> + else if (val < 0) >>>> + reg_val = clamp_t(int, >>>> + val / 2 - val2 / 2 / MICRO, > For these val2 / 2 / MICRO always 0 so value of val2 never matters. > >>>> + S16_MIN, S16_MAX); >>>> + else >>>> + reg_val = clamp_t(int, >>>> + val / 2 + val2 / 2 / MICRO, >>>> + S16_MIN, S16_MAX); >>>> + return reg_val; >>>> + default: >>>> + if (val2 >= 0 && val > S16_MAX / 4) >>>> + reg_val = S16_MAX; >>>> + else if ((val2 < 0 ? -val : val) < S16_MIN / 4) >>>> + reg_val = S16_MIN; >>>> + else if (val2 < 0) >>>> + reg_val = clamp_t(int, >>>> + -(val * 4 + -val2 * 4 / MICRO), >>>> + S16_MIN, S16_MAX); >>>> + else if (val < 0) >>>> + reg_val = clamp_t(int, >>>> + val * 4 - val2 * 4 / MICRO, >>>> + S16_MIN, S16_MAX); >>>> + else >>>> + reg_val = clamp_t(int, >>>> + val * 4 + val2 * 4 / MICRO, >>>> + S16_MIN, S16_MAX); >>>> + return reg_val; >>>> + } >>>> +} >>>> +
On 1/7/25 2:21 PM, Trevor Gamblin wrote: > > On 2025-01-04 07:30, Jonathan Cameron wrote: >> On Thu, 2 Jan 2025 13:19:19 -0500 >> Trevor Gamblin <tgamblin@baylibre.com> wrote: >> >>> On 2024-12-19 11:13, Jonathan Cameron wrote: >>>> On Tue, 17 Dec 2024 16:47:28 -0500 >>>> Trevor Gamblin <tgamblin@baylibre.com> wrote: >>>> >>>>> Add support for the ad4695's oversampling feature when SPI offload is >>>>> available. This allows the ad4695 to set oversampling ratios on a >>>>> per-channel basis, raising the effective-number-of-bits from 16 >>>>> (OSR == 1) to 17 (4), 18 (16), or 19 (64) for a given sample (i.e. one >>>>> full cycle through the auto-sequencer). The logic for reading and >>>>> writing sampling frequency for a given channel is also adjusted based on >>>>> the current oversampling ratio. >>>>> >>>>> The non-offload case isn't supported as there isn't a good way to >>>>> trigger the CNV pin in this mode. Support could be added in the future >>>>> if a use-case arises. >>>>> >>>>> Signed-off-by: Trevor Gamblin <tgamblin@baylibre.com> ... >> Maybe trick is to reorder into 3 conditions and set the value in a temporary integer. >> int val_calc; >> if (val > 0) >> val_calc = val * 2 + val2 * 2 / MICRO; >> else if (val < 0) >> val_calc = -(val * 2 - val2 * 2 / MICRO); >> else /* Only now does val2 sign matter as val == 0 */ >> val_calc = val2 * 2 / MICRO; > > I've been testing out these simplifications (but using the scaling suggestion from below, which is great - for some reason I had it in my head that doing so wasn't an option). > > These seem to have some issues with signs for particularly small calibbias values. I think it's because while my (val2 < 0) case was doing unnecessary clamping, the math itself was OK. > Mail is easier to read when wrapped to 80 chars. ;-) > I did some more experimenting, and came up with a new version of the function that looks like this: > > static unsigned int ad4695_get_calibbias(int val, int val2, int osr) > { > int val_calc, scale; > > switch (osr) { > case 4: > scale = 4; > break; > case 16: > scale = 2; > break; > case 64: > scale = 1; > break; > default: > scale = 8; > break; > } > > /* Note that val2 > 0 if val != 0 and val2 range +- MICRO */ This comment doesn't seem 100% accurate. val2 range is (-MICRO, MICRO) if val == 0 or [0, MICRO) if val != 0. > if (val < 0) > val_calc = val * scale - val2 * scale / MICRO; > else if (val2 < 0) > /* if val2 < 0 then val == 0 */ > val_calc = -(-val2 * scale / MICRO); Could also write this as `val2 * scale / (int)MICRO` lest someone try to remove the double negative and break it (because MICRO is unsigned). This also calls into question if MICRO and similar macros should actually be unsigned because it can lead to subtle bugs since it is perfectly reasonable to expect -1 * MICRO to be -1000000, but it isn't. > else > val_calc = val * scale + val2 * scale / MICRO; > > val_calc /= 2; > > return clamp_t(int, val_calc, S16_MIN, S16_MAX); > } > > This seems to match all of the expected outputs for the pre-simplification version in this patch series when I test it. If there are no issues with it, I'll send a v2. Probably not a big deal, but there is unhanded overflow when val is near S32_MAX or S32_MIN.
On 2025-01-07 16:02, David Lechner wrote: > On 1/7/25 2:21 PM, Trevor Gamblin wrote: >> On 2025-01-04 07:30, Jonathan Cameron wrote: >>> On Thu, 2 Jan 2025 13:19:19 -0500 >>> Trevor Gamblin <tgamblin@baylibre.com> wrote: >>> >>>> On 2024-12-19 11:13, Jonathan Cameron wrote: >>>>> On Tue, 17 Dec 2024 16:47:28 -0500 >>>>> Trevor Gamblin <tgamblin@baylibre.com> wrote: >>>>> >>>>>> Add support for the ad4695's oversampling feature when SPI offload is >>>>>> available. This allows the ad4695 to set oversampling ratios on a >>>>>> per-channel basis, raising the effective-number-of-bits from 16 >>>>>> (OSR == 1) to 17 (4), 18 (16), or 19 (64) for a given sample (i.e. one >>>>>> full cycle through the auto-sequencer). The logic for reading and >>>>>> writing sampling frequency for a given channel is also adjusted based on >>>>>> the current oversampling ratio. >>>>>> >>>>>> The non-offload case isn't supported as there isn't a good way to >>>>>> trigger the CNV pin in this mode. Support could be added in the future >>>>>> if a use-case arises. >>>>>> >>>>>> Signed-off-by: Trevor Gamblin <tgamblin@baylibre.com> > ... > >>> Maybe trick is to reorder into 3 conditions and set the value in a temporary integer. >>> int val_calc; >>> if (val > 0) >>> val_calc = val * 2 + val2 * 2 / MICRO; >>> else if (val < 0) >>> val_calc = -(val * 2 - val2 * 2 / MICRO); >>> else /* Only now does val2 sign matter as val == 0 */ >>> val_calc = val2 * 2 / MICRO; >> I've been testing out these simplifications (but using the scaling suggestion from below, which is great - for some reason I had it in my head that doing so wasn't an option). >> >> These seem to have some issues with signs for particularly small calibbias values. I think it's because while my (val2 < 0) case was doing unnecessary clamping, the math itself was OK. >> > Mail is easier to read when wrapped to 80 chars. ;-) My bad. > > >> I did some more experimenting, and came up with a new version of the function that looks like this: >> >> static unsigned int ad4695_get_calibbias(int val, int val2, int osr) >> { >> int val_calc, scale; >> >> switch (osr) { >> case 4: >> scale = 4; >> break; >> case 16: >> scale = 2; >> break; >> case 64: >> scale = 1; >> break; >> default: >> scale = 8; >> break; >> } >> >> /* Note that val2 > 0 if val != 0 and val2 range +- MICRO */ > This comment doesn't seem 100% accurate. val2 range is (-MICRO, MICRO) if > val == 0 or [0, MICRO) if val != 0. Alright, will fix this. > >> if (val < 0) >> val_calc = val * scale - val2 * scale / MICRO; >> else if (val2 < 0) >> /* if val2 < 0 then val == 0 */ >> val_calc = -(-val2 * scale / MICRO); > Could also write this as `val2 * scale / (int)MICRO` lest someone try to remove > the double negative and break it (because MICRO is unsigned). And this. > > This also calls into question if MICRO and similar macros should actually be > unsigned because it can lead to subtle bugs since it is perfectly reasonable > to expect -1 * MICRO to be -1000000, but it isn't. > >> else >> val_calc = val * scale + val2 * scale / MICRO; >> >> val_calc /= 2; >> >> return clamp_t(int, val_calc, S16_MIN, S16_MAX); >> } >> >> This seems to match all of the expected outputs for the pre-simplification version in this patch series when I test it. If there are no issues with it, I'll send a v2. > Probably not a big deal, but there is unhanded overflow when val is near S32_MAX > or S32_MIN. Should I handle that with an extra call to clamp_t()?
On 1/8/25 1:54 PM, Trevor Gamblin wrote: > > On 2025-01-07 16:02, David Lechner wrote: >> On 1/7/25 2:21 PM, Trevor Gamblin wrote: >>> On 2025-01-04 07:30, Jonathan Cameron wrote: >>>> On Thu, 2 Jan 2025 13:19:19 -0500 >>>> Trevor Gamblin <tgamblin@baylibre.com> wrote: >>>> >>>>> On 2024-12-19 11:13, Jonathan Cameron wrote: >>>>>> On Tue, 17 Dec 2024 16:47:28 -0500 >>>>>> Trevor Gamblin <tgamblin@baylibre.com> wrote: >>>>>> ... >>> else >>> val_calc = val * scale + val2 * scale / MICRO; >>> >>> val_calc /= 2; >>> >>> return clamp_t(int, val_calc, S16_MIN, S16_MAX); >>> } >>> >>> This seems to match all of the expected outputs for the pre-simplification version in this patch series when I test it. If there are no issues with it, I'll send a v2. >> Probably not a big deal, but there is unhanded overflow when val is near S32_MAX >> or S32_MIN. > Should I handle that with an extra call to clamp_t()? It wouldn't hurt. val = clamp_t(int, val, S32_MIN / 8, S32_MAX / 8); before the rest of the math should do the trick since the max scale is 8.
diff --git a/drivers/iio/adc/ad4695.c b/drivers/iio/adc/ad4695.c index c8cd73d19e86..320f2c1a0877 100644 --- a/drivers/iio/adc/ad4695.c +++ b/drivers/iio/adc/ad4695.c @@ -79,6 +79,7 @@ #define AD4695_REG_CONFIG_IN_MODE BIT(6) #define AD4695_REG_CONFIG_IN_PAIR GENMASK(5, 4) #define AD4695_REG_CONFIG_IN_AINHIGHZ_EN BIT(3) +#define AD4695_REG_CONFIG_IN_OSR_SET GENMASK(1, 0) #define AD4695_REG_UPPER_IN(n) (0x0040 | (2 * (n))) #define AD4695_REG_LOWER_IN(n) (0x0060 | (2 * (n))) #define AD4695_REG_HYST_IN(n) (0x0080 | (2 * (n))) @@ -127,6 +128,7 @@ struct ad4695_channel_config { bool bipolar; enum ad4695_in_pair pin_pairing; unsigned int common_mode_mv; + unsigned int oversampling_ratio; }; struct ad4695_state { @@ -306,6 +308,65 @@ static const struct regmap_bus ad4695_regmap_bus = { .val_format_endian_default = REGMAP_ENDIAN_BIG, }; +enum { + AD4695_SCAN_TYPE_OSR_1, + AD4695_SCAN_TYPE_OSR_4, + AD4695_SCAN_TYPE_OSR_16, + AD4695_SCAN_TYPE_OSR_64, +}; + +static const struct iio_scan_type ad4695_scan_type_offload_u[] = { + [AD4695_SCAN_TYPE_OSR_1] = { + .sign = 'u', + .realbits = 16, + .shift = 3, + .storagebits = 32, + }, + [AD4695_SCAN_TYPE_OSR_4] = { + .sign = 'u', + .realbits = 17, + .shift = 2, + .storagebits = 32, + }, + [AD4695_SCAN_TYPE_OSR_16] = { + .sign = 'u', + .realbits = 18, + .shift = 1, + .storagebits = 32, + }, + [AD4695_SCAN_TYPE_OSR_64] = { + .sign = 'u', + .realbits = 19, + .storagebits = 32, + }, +}; + +static const struct iio_scan_type ad4695_scan_type_offload_s[] = { + [AD4695_SCAN_TYPE_OSR_1] = { + .sign = 's', + .realbits = 16, + .shift = 3, + .storagebits = 32, + }, + [AD4695_SCAN_TYPE_OSR_4] = { + .sign = 's', + .realbits = 17, + .shift = 2, + .storagebits = 32, + }, + [AD4695_SCAN_TYPE_OSR_16] = { + .sign = 's', + .realbits = 18, + .shift = 1, + .storagebits = 32, + }, + [AD4695_SCAN_TYPE_OSR_64] = { + .sign = 's', + .realbits = 19, + .storagebits = 32, + }, +}; + static const struct iio_chan_spec ad4695_channel_template = { .type = IIO_VOLTAGE, .indexed = 1, @@ -343,6 +404,10 @@ static const char * const ad4695_power_supplies[] = { "avdd", "vio" }; +static const int ad4695_oversampling_ratios[] = { + 1, 4, 16, 64, +}; + static const struct ad4695_chip_info ad4695_chip_info = { .name = "ad4695", .max_sample_rate = 500 * KILO, @@ -519,6 +584,29 @@ static int ad4695_set_ref_voltage(struct ad4695_state *st, int vref_mv) FIELD_PREP(AD4695_REG_REF_CTRL_VREF_SET, val)); } +/** + * ad4695_osr_to_regval - convert ratio to OSR register value + * @ratio: ratio to check + * + * Check if ratio is present in the list of available ratios and return + * the corresponding value that needs to be written to the register to + * select that ratio. + * + * Returns: register value (0 to 3) or -EINVAL if there is not an exact + * match + */ +static int ad4695_osr_to_regval(int ratio) +{ + int i; + + for (i = 0; i < ARRAY_SIZE(ad4695_oversampling_ratios); i++) { + if (ratio == ad4695_oversampling_ratios[i]) + return i; + } + + return -EINVAL; +} + static int ad4695_write_chn_cfg(struct ad4695_state *st, struct ad4695_channel_config *cfg) { @@ -945,10 +1033,18 @@ static int ad4695_read_raw(struct iio_dev *indio_dev, int *val, int *val2, long mask) { struct ad4695_state *st = iio_priv(indio_dev); + const struct iio_scan_type *scan_type; struct ad4695_channel_config *cfg = &st->channels_cfg[chan->scan_index]; - u8 realbits = chan->scan_type.realbits; + unsigned int osr = st->channels_cfg[chan->scan_index].oversampling_ratio; unsigned int reg_val; int ret, tmp; + u8 realbits; + + scan_type = iio_get_current_scan_type(indio_dev, chan); + if (IS_ERR(scan_type)) + return PTR_ERR(scan_type); + + realbits = scan_type->realbits; switch (mask) { case IIO_CHAN_INFO_RAW: @@ -957,7 +1053,7 @@ static int ad4695_read_raw(struct iio_dev *indio_dev, if (ret) return ret; - if (chan->scan_type.sign == 's') + if (scan_type->sign == 's') *val = sign_extend32(st->raw_data, realbits - 1); else *val = st->raw_data; @@ -969,7 +1065,7 @@ static int ad4695_read_raw(struct iio_dev *indio_dev, switch (chan->type) { case IIO_VOLTAGE: *val = st->vref_mv; - *val2 = chan->scan_type.realbits; + *val2 = realbits; return IIO_VAL_FRACTIONAL_LOG2; case IIO_TEMP: /* T_scale (°C) = raw * V_REF (mV) / (-1.8 mV/°C * 2^16) */ @@ -1030,8 +1126,26 @@ static int ad4695_read_raw(struct iio_dev *indio_dev, tmp = sign_extend32(reg_val, 15); - *val = tmp / 4; - *val2 = abs(tmp) % 4 * MICRO / 4; + switch (cfg->oversampling_ratio) { + case 1: + *val = tmp / 4; + *val2 = abs(tmp) % 4 * MICRO / 4; + break; + case 4: + *val = tmp / 2; + *val2 = abs(tmp) % 2 * MICRO / 2; + break; + case 16: + *val = tmp; + *val2 = 0; + break; + case 64: + *val = tmp * 2; + *val2 = 0; + break; + default: + return -EINVAL; + } if (tmp < 0 && *val2) { *val *= -1; @@ -1044,6 +1158,14 @@ static int ad4695_read_raw(struct iio_dev *indio_dev, default: return -EINVAL; } + case IIO_CHAN_INFO_OVERSAMPLING_RATIO: + switch (chan->type) { + case IIO_VOLTAGE: + *val = st->channels_cfg[chan->scan_index].oversampling_ratio; + return IIO_VAL_INT; + default: + return -EINVAL; + } case IIO_CHAN_INFO_SAMP_FREQ: { struct pwm_state state; @@ -1051,7 +1173,11 @@ static int ad4695_read_raw(struct iio_dev *indio_dev, if (ret) return ret; - *val = DIV_ROUND_UP_ULL(NSEC_PER_SEC, state.period); + /* + * The effective sampling frequency for a channel is the input + * frequency divided by the channel's OSR value. + */ + *val = DIV_ROUND_UP_ULL(NSEC_PER_SEC, state.period * osr); return IIO_VAL_INT; } @@ -1072,12 +1198,114 @@ static int ad4695_write_raw_get_fmt(struct iio_dev *indio_dev, } } +static int ad4695_set_osr_val(struct ad4695_state *st, + struct iio_chan_spec const *chan, + int val) +{ + int osr = ad4695_osr_to_regval(val); + + if (osr < 0) + return osr; + + switch (chan->type) { + case IIO_VOLTAGE: + st->channels_cfg[chan->scan_index].oversampling_ratio = val; + return regmap_update_bits(st->regmap, + AD4695_REG_CONFIG_IN(chan->scan_index), + AD4695_REG_CONFIG_IN_OSR_SET, + FIELD_PREP(AD4695_REG_CONFIG_IN_OSR_SET, osr)); + default: + return -EINVAL; + } +} + +static unsigned int ad4695_get_calibbias(int val, int val2, int osr) +{ + unsigned int reg_val; + + switch (osr) { + case 4: + if (val2 >= 0 && val > S16_MAX / 2) + reg_val = S16_MAX; + else if ((val2 < 0 ? -val : val) < S16_MIN / 2) + reg_val = S16_MIN; + else if (val2 < 0) + reg_val = clamp_t(int, + -(val * 2 + -val2 * 2 / MICRO), + S16_MIN, S16_MAX); + else if (val < 0) + reg_val = clamp_t(int, + val * 2 - val2 * 2 / MICRO, + S16_MIN, S16_MAX); + else + reg_val = clamp_t(int, + val * 2 + val2 * 2 / MICRO, + S16_MIN, S16_MAX); + return reg_val; + case 16: + if (val2 >= 0 && val > S16_MAX) + reg_val = S16_MAX; + else if ((val2 < 0 ? -val : val) < S16_MIN) + reg_val = S16_MIN; + else if (val2 < 0) + reg_val = clamp_t(int, + -(val + -val2 / MICRO), + S16_MIN, S16_MAX); + else if (val < 0) + reg_val = clamp_t(int, + val - val2 / MICRO, + S16_MIN, S16_MAX); + else + reg_val = clamp_t(int, + val + val2 / MICRO, + S16_MIN, S16_MAX); + return reg_val; + case 64: + if (val2 >= 0 && val > S16_MAX * 2) + reg_val = S16_MAX; + else if ((val2 < 0 ? -val : val) < S16_MIN * 2) + reg_val = S16_MIN; + else if (val2 < 0) + reg_val = clamp_t(int, + -(val / 2 + -val2 / 2 / MICRO), + S16_MIN, S16_MAX); + else if (val < 0) + reg_val = clamp_t(int, + val / 2 - val2 / 2 / MICRO, + S16_MIN, S16_MAX); + else + reg_val = clamp_t(int, + val / 2 + val2 / 2 / MICRO, + S16_MIN, S16_MAX); + return reg_val; + default: + if (val2 >= 0 && val > S16_MAX / 4) + reg_val = S16_MAX; + else if ((val2 < 0 ? -val : val) < S16_MIN / 4) + reg_val = S16_MIN; + else if (val2 < 0) + reg_val = clamp_t(int, + -(val * 4 + -val2 * 4 / MICRO), + S16_MIN, S16_MAX); + else if (val < 0) + reg_val = clamp_t(int, + val * 4 - val2 * 4 / MICRO, + S16_MIN, S16_MAX); + else + reg_val = clamp_t(int, + val * 4 + val2 * 4 / MICRO, + S16_MIN, S16_MAX); + return reg_val; + } +} + static int ad4695_write_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan, int val, int val2, long mask) { struct ad4695_state *st = iio_priv(indio_dev); unsigned int reg_val; + unsigned int osr = st->channels_cfg[chan->scan_index].oversampling_ratio; iio_device_claim_direct_scoped(return -EBUSY, indio_dev) { switch (mask) { @@ -1102,23 +1330,7 @@ static int ad4695_write_raw(struct iio_dev *indio_dev, case IIO_CHAN_INFO_CALIBBIAS: switch (chan->type) { case IIO_VOLTAGE: - if (val2 >= 0 && val > S16_MAX / 4) - reg_val = S16_MAX; - else if ((val2 < 0 ? -val : val) < S16_MIN / 4) - reg_val = S16_MIN; - else if (val2 < 0) - reg_val = clamp_t(int, - -(val * 4 + -val2 * 4 / MICRO), - S16_MIN, S16_MAX); - else if (val < 0) - reg_val = clamp_t(int, - val * 4 - val2 * 4 / MICRO, - S16_MIN, S16_MAX); - else - reg_val = clamp_t(int, - val * 4 + val2 * 4 / MICRO, - S16_MIN, S16_MAX); - + reg_val = ad4695_get_calibbias(val, val2, osr); return regmap_write(st->regmap16, AD4695_REG_OFFSET_IN(chan->scan_index), reg_val); @@ -1127,15 +1339,27 @@ static int ad4695_write_raw(struct iio_dev *indio_dev, } case IIO_CHAN_INFO_SAMP_FREQ: { struct pwm_state state; - - if (val <= 0 || val > st->chip_info->max_sample_rate) + /* + * Limit the maximum acceptable sample rate according to + * the channel's oversampling ratio. + */ + u64 max_osr_rate = DIV_ROUND_UP_ULL(st->chip_info->max_sample_rate, + osr); + + if (val <= 0 || val > max_osr_rate) return -EINVAL; guard(mutex)(&st->cnv_pwm_lock); pwm_get_state(st->cnv_pwm, &state); - state.period = DIV_ROUND_UP_ULL(NSEC_PER_SEC, val); + /* + * The required sample frequency for a given OSR is the + * input frequency multiplied by it. + */ + state.period = DIV_ROUND_UP_ULL(NSEC_PER_SEC, val * osr); return pwm_apply_might_sleep(st->cnv_pwm, &state); } + case IIO_CHAN_INFO_OVERSAMPLING_RATIO: + return ad4695_set_osr_val(st, chan, val); default: return -EINVAL; } @@ -1148,18 +1372,40 @@ static int ad4695_read_avail(struct iio_dev *indio_dev, const int **vals, int *type, int *length, long mask) { + int ret; static const int ad4695_calibscale_available[6] = { /* Range of 0 (inclusive) to 2 (exclusive) */ 0, 15, 1, 15, U16_MAX, 15 }; - static const int ad4695_calibbias_available[6] = { + static const int ad4695_calibbias_available[4][6] = { /* * Datasheet says FSR/8 which translates to signed/4. The step - * depends on oversampling ratio which is always 1 for now. + * depends on oversampling ratio, so we need four different + * ranges to select from. */ - S16_MIN / 4, 0, 0, MICRO / 4, S16_MAX / 4, S16_MAX % 4 * MICRO / 4 + { + S16_MIN / 4, 0, + 0, MICRO / 4, + S16_MAX / 4, S16_MAX % 4 * MICRO / 4 + }, + { + S16_MIN / 2, 0, + 0, MICRO / 2, + S16_MAX / 2, S16_MAX % 2 * MICRO / 2, + }, + { + S16_MIN, 0, + 1, 0, + S16_MAX, 0, + }, + { + S16_MIN * 2, 0, + 2, 0, + S16_MAX * 2, 0, + }, }; struct ad4695_state *st = iio_priv(indio_dev); + unsigned int osr = st->channels_cfg[chan->scan_index].oversampling_ratio; switch (mask) { case IIO_CHAN_INFO_CALIBSCALE: @@ -1174,16 +1420,36 @@ static int ad4695_read_avail(struct iio_dev *indio_dev, case IIO_CHAN_INFO_CALIBBIAS: switch (chan->type) { case IIO_VOLTAGE: - *vals = ad4695_calibbias_available; + ret = ad4695_osr_to_regval(osr); + if (ret < 0) + return ret; + /* + * Select the appropriate calibbias array based on the + * OSR value in the register. + */ + *vals = ad4695_calibbias_available[ret]; *type = IIO_VAL_INT_PLUS_MICRO; return IIO_AVAIL_RANGE; default: return -EINVAL; } case IIO_CHAN_INFO_SAMP_FREQ: + /* Max sample rate for the channel depends on OSR */ + st->sample_freq_range[2] = + DIV_ROUND_UP_ULL(st->chip_info->max_sample_rate, osr); *vals = st->sample_freq_range; *type = IIO_VAL_INT; return IIO_AVAIL_RANGE; + case IIO_CHAN_INFO_OVERSAMPLING_RATIO: + switch (chan->type) { + case IIO_VOLTAGE: + *vals = ad4695_oversampling_ratios; + *length = ARRAY_SIZE(ad4695_oversampling_ratios); + *type = IIO_VAL_INT; + return IIO_AVAIL_LIST; + default: + return -EINVAL; + } default: return -EINVAL; } @@ -1217,6 +1483,26 @@ static int ad4695_debugfs_reg_access(struct iio_dev *indio_dev, return -EINVAL; } +static int ad4695_get_current_scan_type(const struct iio_dev *indio_dev, + const struct iio_chan_spec *chan) +{ + struct ad4695_state *st = iio_priv(indio_dev); + unsigned int osr = st->channels_cfg[chan->scan_index].oversampling_ratio; + + switch (osr) { + case 1: + return AD4695_SCAN_TYPE_OSR_1; + case 4: + return AD4695_SCAN_TYPE_OSR_4; + case 16: + return AD4695_SCAN_TYPE_OSR_16; + case 64: + return AD4695_SCAN_TYPE_OSR_64; + default: + return -EINVAL; + } +} + static const struct iio_info ad4695_info = { .read_raw = &ad4695_read_raw, .write_raw_get_fmt = &ad4695_write_raw_get_fmt, @@ -1225,6 +1511,15 @@ static const struct iio_info ad4695_info = { .debugfs_reg_access = &ad4695_debugfs_reg_access, }; +static const struct iio_info ad4695_offload_info = { + .read_raw = &ad4695_read_raw, + .write_raw_get_fmt = &ad4695_write_raw_get_fmt, + .write_raw = &ad4695_write_raw, + .get_current_scan_type = &ad4695_get_current_scan_type, + .read_avail = &ad4695_read_avail, + .debugfs_reg_access = &ad4695_debugfs_reg_access, +}; + static int ad4695_parse_channel_cfg(struct ad4695_state *st) { struct device *dev = &st->spi->dev; @@ -1240,6 +1535,9 @@ static int ad4695_parse_channel_cfg(struct ad4695_state *st) chan_cfg->highz_en = true; chan_cfg->channel = i; + /* This is the default OSR after reset */ + chan_cfg->oversampling_ratio = 1; + *iio_chan = ad4695_channel_template; iio_chan->channel = i; iio_chan->scan_index = i; @@ -1408,6 +1706,7 @@ static int ad4695_probe_spi_offload(struct iio_dev *indio_dev, struct dma_chan *rx_dma; int ret, i; + indio_dev->info = &ad4695_offload_info; indio_dev->num_channels = st->chip_info->num_voltage_inputs + 1; indio_dev->setup_ops = &ad4695_offload_buffer_setup_ops; @@ -1458,6 +1757,7 @@ static int ad4695_probe_spi_offload(struct iio_dev *indio_dev, for (i = 0; i < indio_dev->num_channels; i++) { struct iio_chan_spec *chan = &st->iio_chan[i]; + struct ad4695_channel_config *cfg = &st->channels_cfg[i]; /* * NB: When using offload support, all channels need to have the @@ -1473,6 +1773,24 @@ static int ad4695_probe_spi_offload(struct iio_dev *indio_dev, /* add sample frequency for PWM CNV trigger */ chan->info_mask_separate |= BIT(IIO_CHAN_INFO_SAMP_FREQ); chan->info_mask_separate_available |= BIT(IIO_CHAN_INFO_SAMP_FREQ); + + /* Add the oversampling properties only for voltage channels */ + if (chan->type != IIO_VOLTAGE) + continue; + + chan->info_mask_separate |= BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO); + chan->info_mask_separate_available |= + BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO); + chan->has_ext_scan_type = 1; + if (cfg->bipolar) { + chan->ext_scan_type = ad4695_scan_type_offload_s; + chan->num_ext_scan_type = + ARRAY_SIZE(ad4695_scan_type_offload_s); + } else { + chan->ext_scan_type = ad4695_scan_type_offload_u; + chan->num_ext_scan_type = + ARRAY_SIZE(ad4695_scan_type_offload_u); + } } return devm_iio_dmaengine_buffer_setup_with_handle(dev, indio_dev,
Add support for the ad4695's oversampling feature when SPI offload is available. This allows the ad4695 to set oversampling ratios on a per-channel basis, raising the effective-number-of-bits from 16 (OSR == 1) to 17 (4), 18 (16), or 19 (64) for a given sample (i.e. one full cycle through the auto-sequencer). The logic for reading and writing sampling frequency for a given channel is also adjusted based on the current oversampling ratio. The non-offload case isn't supported as there isn't a good way to trigger the CNV pin in this mode. Support could be added in the future if a use-case arises. Signed-off-by: Trevor Gamblin <tgamblin@baylibre.com> --- drivers/iio/adc/ad4695.c | 378 +++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 348 insertions(+), 30 deletions(-)