| Message ID | 9895826669118a1aa1db3f85c2610fa759426c33.1677750859.git.mazziesaccount@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Support ROHM BU27034 ALS sensor | expand |
On Thu, Mar 02, 2023 at 12:57:54PM +0200, Matti Vaittinen wrote: > Some light sensors can adjust both the HW-gain and integration time. > There are cases where adjusting the integration time has similar impact > to the scale of the reported values as gain setting has. > > IIO users do typically expect to handle scale by a single writable 'scale' > entry. Driver should then adjust the gain/time accordingly. > > It however is difficult for a driver to know whether it should change > gain or integration time to meet the requested scale. Usually it is > preferred to have longer integration time which usually improves > accuracy, but there may be use-cases where long measurement times can be > an issue. Thus it can be preferable to allow also changing the > integration time - but mitigate the scale impact by also changing the gain > underneath. Eg, if integration time change doubles the measured values, > the driver can reduce the HW-gain to half. > > The theory of the computations of gain-time-scale is simple. However, > some people (undersigned) got that implemented wrong for more than once. > > Add some gain-time-scale helpers in order to not dublicate errors in all > drivers needing these computations. ... > +/* Is it intentionally _not_ a kernel doc? > + * iio_gts_get_gain - Convert scale to total gain > + * Internal helper for converting scale to total gain. Otherwise this line should go after the fields, I remember kernel doc warnings on the similar cases. > + * @max: Maximum linearized scale. As an example, when scale is creted in creted? IIRC I already pointed out to the very same mistake in your code in the past (sorry, if my memory doesn't serve me well). > + * magnitude of NANOs and max scale is 64.1 - The linearized > + * scale is 64 100 000 000. > + * @scale: Linearized scale to compte the gain for. > + * > + * Return: (floored) gain corresponding to the scales. -EINVAL if scale scales? (Plural?) > + * is invalid. > + */ Same remark to all of the comments. > +{ > + int tmp = 1; > + > + if (scale > max || !scale) > + return -EINVAL; > + > + if (U64_MAX - max < scale) { > + /* Risk of overflow */ > + if (max - scale < scale) > + return 1; > + while (max - scale > scale * (u64) tmp) Space is not required after casting. > + tmp++; > + > + return tmp + 1; Wondering why you can't simplify this to max -= scale; tmp++; > + } > + > + while (max > scale * (u64) tmp) > + tmp++; > + > + return tmp; > +} Missing blank line. > +/* > + * gain_get_scale_fraction - get the gain or time based on scale and known one > + * > + * Internal helper for computing unknown fraction of total gain. > + * Compute either gain or time based on scale and either the gain or time > + * depending on which one is known. > + * > + * @max: Maximum linearized scale. As an example, when scale is creted in creted? Is it mistakenly stored in your spellcheck database? Or is it simply copy'n'paste typo? > + * magnitude of NANOs and max scale is 64.1 - The linearized > + * scale is 64 100 000 000. > + * @scale: Linearized scale to compute the gain/time for. > + * @known: Either integration time or gain depending on which one is known > + * @unknown: Pointer to variable where the computed gain/time is stored > + * > + * Return: 0 on success > + */ ... > +static const struct iio_itime_sel_mul * > + iio_gts_find_itime_by_time(struct iio_gts *gts, int time) Strange indentation. Ditto for all these types of cases. ... > + *lin_scale = (u64) scale_whole * (u64)scaler + (u64)(scale_nano > + / (NANO / scaler)); Strange indentation. Split on the logical (math) parts better. ... > +EXPORT_SYMBOL_GPL(iio_init_iio_gts); I haven't noticed if you put these all exports into a proper namespace. If no, please do. ... > + sort(gains[i], gts->num_hwgain, sizeof(int), iio_gts_gain_cmp, > + NULL); One line is okay. ... > + all_gains = kcalloc(gts->num_itime * gts->num_hwgain, sizeof(int), Something from overflow.h is very suitable here. > + GFP_KERNEL); > + if (!all_gains) > + return -ENOMEM; ... > + memcpy(all_gains, gains[gts->num_itime - 1], gts->num_hwgain * sizeof(int)); Maybe it's better to have a temporary which will be calculated as array_size() for allocation and reused here? ... > + for (i = gts->num_itime - 2; i >= 0; i--) Yeah, if you put this into temporary, like i = gts->num_itime - 1; this becomes while (i--) { Note, you missed {} for better reading. Note, you may re-use that i (maybe renamed to something better in the memcpy() above as well). > + for (j = 0; j < gts->num_hwgain; j++) { > + int candidate = gains[i][j]; > + int chk; > + > + if (candidate > all_gains[new_idx - 1]) { > + all_gains[new_idx] = candidate; > + new_idx++; > + > + continue; > + } > + for (chk = 0; chk < new_idx; chk++) > + if (candidate <= all_gains[chk]) > + break; > + > + if (candidate == all_gains[chk]) > + continue; > + > + memmove(&all_gains[chk + 1], &all_gains[chk], > + (new_idx - chk) * sizeof(int)); > + all_gains[chk] = candidate; > + new_idx++; > + } ... > + gts->avail_all_scales_table = kcalloc(gts->num_avail_all_scales, > + 2 * sizeof(int), GFP_KERNEL); > + if (!gts->avail_all_scales_table) > + ret = -ENOMEM; > + else > + for (i = 0; !ret && i < gts->num_avail_all_scales; i++) Much easier to read if you move this... > + ret = iio_gts_total_gain_to_scale(gts, all_gains[i], > + >s->avail_all_scales_table[i * 2], > + >s->avail_all_scales_table[i * 2 + 1]); ...here as if (ret) break; > + kfree(all_gains); > + if (ret && gts->avail_all_scales_table) > + kfree(gts->avail_all_scales_table); > + > + return ret; But Wouldn't be better to use goto labels? ... > + while (i) { Instead of doing standard while (i--) { > + /* > + * It does not matter if i'th alloc was not succesfull as > + * kfree(NULL) is safe. > + */ You add this comment, ... > + kfree(per_time_gains[i]); > + kfree(per_time_scales[i]); ...an additional loop, ... > + > + i--; ...and a line of code. > + } Why? > + for (i = gts->num_itime - 1; i >= 0; i--) { i = gts->num_itime; while (i--) { > + int new = gts->itime_table[i].time_us; > + > + if (times[idx] < new) { > + times[idx++] = new; > + continue; > + } > + > + for (j = 0; j <= idx; j++) { > + if (times[j] > new) { > + memmove(×[j + 1], ×[j], (idx - j) * sizeof(int)); > + times[j] = new; > + idx++; > + } > + } > + } ... > +void iio_gts_purge_avail_time_table(struct iio_gts *gts) > +{ > + if (gts->num_avail_time_tables) { if (!...) return; > + kfree(gts->avail_time_tables); > + gts->avail_time_tables = NULL; > + gts->num_avail_time_tables = 0; > + } > +} ... > + if (!diff) { Why not positive conditional? if (diff) { ... } else { ... } > + diff = gain - gts->hwgain_table[i].gain; > + best = i; > + } else { > + int tmp = gain - gts->hwgain_table[i].gain; > + > + if (tmp < diff) { > + diff = tmp; > + best = i; > + } > + } ... > + ret = gain_get_scale_fraction(gts->max_scale, scale_linear, mul, gain); > + Redundant blank line. > + if (ret || !iio_gts_valid_gain(gts, *gain)) Why error code is shadowed? > + return -EINVAL; > + ... > + ret = iio_gts_get_scale_linear(gts, old_gain, itime_old->time_us, > + &scale); Single line if fine. > + if (ret) > + return ret; > + > + ret = gain_get_scale_fraction(gts->max_scale, scale, itime_new->mul, > + new_gain); Ditto. > + if (ret) > + return -EINVAL; ... > +#ifndef __GAIN_TIME_SCALE_HELPER__ > +#define __GAIN_TIME_SCALE_HELPER__ __IIO_... ? Missing types.h (at least, haven't checked for more). Missing some forward declarations, at least for struct device.
Thanks for the review Andy, On 3/2/23 17:05, Andy Shevchenko wrote: > On Thu, Mar 02, 2023 at 12:57:54PM +0200, Matti Vaittinen wrote: >> Some light sensors can adjust both the HW-gain and integration time. >> There are cases where adjusting the integration time has similar impact >> to the scale of the reported values as gain setting has. >> >> IIO users do typically expect to handle scale by a single writable 'scale' >> entry. Driver should then adjust the gain/time accordingly. >> >> It however is difficult for a driver to know whether it should change >> gain or integration time to meet the requested scale. Usually it is >> preferred to have longer integration time which usually improves >> accuracy, but there may be use-cases where long measurement times can be >> an issue. Thus it can be preferable to allow also changing the >> integration time - but mitigate the scale impact by also changing the gain >> underneath. Eg, if integration time change doubles the measured values, >> the driver can reduce the HW-gain to half. >> >> The theory of the computations of gain-time-scale is simple. However, >> some people (undersigned) got that implemented wrong for more than once. >> >> Add some gain-time-scale helpers in order to not dublicate errors in all >> drivers needing these computations. > > ... > >> +/* > > Is it intentionally _not_ a kernel doc? yes. >> + * iio_gts_get_gain - Convert scale to total gain > >> + * Internal helper for converting scale to total gain. > > Otherwise this line should go after the fields, I remember kernel doc warnings > on the similar cases. > >> + * @max: Maximum linearized scale. As an example, when scale is creted in > > creted? > > IIRC I already pointed out to the very same mistake in your code in the past > (sorry, if my memory doesn't serve me well). I Don't think you have previously reviewed this patch. >> +{ >> + int tmp = 1; >> + >> + if (scale > max || !scale) >> + return -EINVAL; >> + >> + if (U64_MAX - max < scale) { >> + /* Risk of overflow */ >> + if (max - scale < scale) >> + return 1; > >> + while (max - scale > scale * (u64) tmp) > > Space is not required after casting. > >> + tmp++; >> + >> + return tmp + 1; > > Wondering why you can't simplify this to > > max -= scale; > tmp++; > Sorry, but I don't follow. Can you please show me the complete suggestion? Exactly what should be replaced by the: > max -= scale; > tmp++; >> + } >> + >> + while (max > scale * (u64) tmp) >> + tmp++; >> + >> + return tmp; >> +} >> +/* >> + * gain_get_scale_fraction - get the gain or time based on scale and known one >> + * >> + * Internal helper for computing unknown fraction of total gain. >> + * Compute either gain or time based on scale and either the gain or time >> + * depending on which one is known. >> + * >> + * @max: Maximum linearized scale. As an example, when scale is creted in > > creted? > > Is it mistakenly stored in your spellcheck database? Or is it simply > copy'n'paste typo? Typo. >> + * magnitude of NANOs and max scale is 64.1 - The linearized >> + * scale is 64 100 000 000. >> + * @scale: Linearized scale to compute the gain/time for. >> + * @known: Either integration time or gain depending on which one is known >> + * @unknown: Pointer to variable where the computed gain/time is stored >> + * >> + * Return: 0 on success >> + */ > >> +EXPORT_SYMBOL_GPL(iio_init_iio_gts); > > I haven't noticed if you put these all exports into a proper namespace. > If no, please do. I haven't. And in many cases I actually see very little to no value in doing so as collisions are unlikely when symbols are appropriately prefixed. Anyways, it seems the IIO uses namespaces so let's play along. Any good suggestions for a name? > ... > >> + all_gains = kcalloc(gts->num_itime * gts->num_hwgain, sizeof(int), > > Something from overflow.h is very suitable here. > >> + GFP_KERNEL); >> + if (!all_gains) >> + return -ENOMEM; > > ... > >> + memcpy(all_gains, gains[gts->num_itime - 1], gts->num_hwgain * sizeof(int)); > > Maybe it's better to have a temporary which will be calculated as array_size() > for allocation and reused here? Good suggestion, thanks. > ... > >> + for (i = gts->num_itime - 2; i >= 0; i--) > > Yeah, if you put this into temporary, like > > i = gts->num_itime - 1; > > this becomes > > while (i--) { > I prefer for(). It looks clearer to me... > Note, you may re-use that i (maybe renamed to something better in the memcpy() > above as well). ...but this makes sense so Ok. > Note, you missed {} for better reading. Didn't miss it but left it out intentionally. {} does not help me as indentiation makes it clear. Can add one for you though. > >> + for (j = 0; j < gts->num_hwgain; j++) { >> + int candidate = gains[i][j]; >> + int chk; >> + >> + if (candidate > all_gains[new_idx - 1]) { >> + all_gains[new_idx] = candidate; >> + new_idx++; >> + >> + continue; >> + } >> + for (chk = 0; chk < new_idx; chk++) >> + if (candidate <= all_gains[chk]) >> + break; >> + >> + if (candidate == all_gains[chk]) >> + continue; >> + >> + memmove(&all_gains[chk + 1], &all_gains[chk], >> + (new_idx - chk) * sizeof(int)); >> + all_gains[chk] = candidate; >> + new_idx++; >> + } > > ... > >> + gts->avail_all_scales_table = kcalloc(gts->num_avail_all_scales, >> + 2 * sizeof(int), GFP_KERNEL); >> + if (!gts->avail_all_scales_table) >> + ret = -ENOMEM; >> + else >> + for (i = 0; !ret && i < gts->num_avail_all_scales; i++) > > Much easier to read if you move this... > >> + ret = iio_gts_total_gain_to_scale(gts, all_gains[i], >> + >s->avail_all_scales_table[i * 2], >> + >s->avail_all_scales_table[i * 2 + 1]); > > ...here as > > if (ret) > break; I think the !ret in loop condition is obvious. Adding break and brackets would not improve this. > >> + kfree(all_gains); >> + if (ret && gts->avail_all_scales_table) >> + kfree(gts->avail_all_scales_table); >> + >> + return ret; > > But Wouldn't be better to use goto labels? I don't see benefit in this case. Handling return value and goto would require brackets. The current one is much simpler for me. I am just considering dropping that 'else' which is not needed. > ... > >> + while (i) { > > Instead of doing standard > > while (i--) { > >> + /* >> + * It does not matter if i'th alloc was not succesfull as >> + * kfree(NULL) is safe. >> + */ > > You add this comment, ... > >> + kfree(per_time_gains[i]); >> + kfree(per_time_scales[i]); > > ...an additional loop, ... The comment is there to explain what I think you missed. We have two arrays there. We don't know whether the allocation of first one was successful so we try freeing both. > >> + >> + i--; > > ...and a line of code. > >> + } > > Why? Because, as the comment says, it does not matter if allocation was succesfull. kfree(NULL) is safe. > >> + for (i = gts->num_itime - 1; i >= 0; i--) { > > i = gts->num_itime; > > while (i--) { I prefer for() when initializing a variable is needed. Furthermore, having var-- or --var in a condition is less clear. > >> + int new = gts->itime_table[i].time_us; >> + >> + if (times[idx] < new) { >> + times[idx++] = new; >> + continue; >> + } >> + >> + for (j = 0; j <= idx; j++) { >> + if (times[j] > new) { >> + memmove(×[j + 1], ×[j], (idx - j) * sizeof(int)); >> + times[j] = new; >> + idx++; >> + } >> + } >> + } > > ... > >> +void iio_gts_purge_avail_time_table(struct iio_gts *gts) >> +{ >> + if (gts->num_avail_time_tables) { > > if (!...) > return; Does not improve this as we only have just one small conditional block of code which we either execute or not. It is clear at a glance. Would make sense if we had longer function. >> + kfree(gts->avail_time_tables); >> + gts->avail_time_tables = NULL; >> + gts->num_avail_time_tables = 0; >> + } >> +} > > ... > >> + if (!diff) { > > Why not positive conditional? Because !diff is a special condition and we check explicitly for it. > > if (diff) { > ... > } else { > ... > } > >> + diff = gain - gts->hwgain_table[i].gain; >> + best = i; >> + } else { >> + int tmp = gain - gts->hwgain_table[i].gain; >> + >> + if (tmp < diff) { >> + diff = tmp; >> + best = i; >> + } >> + } > > ... > >> + ret = gain_get_scale_fraction(gts->max_scale, scale_linear, mul, gain); > >> + > > Redundant blank line. Thanks. > >> + if (ret || !iio_gts_valid_gain(gts, *gain)) > > Why error code is shadowed? Not sure what I thought of. Could have been because -EINVAL is in any case the only error I can think of. But yes, it is better to not 'shadow' the return value from gain_get_scale_fraction(); So, thanks. > >> + return -EINVAL; >> + > > ... > >> + ret = iio_gts_get_scale_linear(gts, old_gain, itime_old->time_us, >> + &scale); > > Single line if fine. I like to (mostly) keep the 80-column limit. Helps me to fit three editor windows next to each-others on my laptop screen. >> + if (ret) >> + return ret; >> + >> + ret = gain_get_scale_fraction(gts->max_scale, scale, itime_new->mul, >> + new_gain); > > Ditto. > >> + if (ret) >> + return -EINVAL; > > ... > >> +#ifndef __GAIN_TIME_SCALE_HELPER__ >> +#define __GAIN_TIME_SCALE_HELPER__ > > __IIO_... ? Well spotted. Thanks. I renamed the file and forgot the old guardian. By the way, I will be online only occasionally during the next week. So please don't think I ignore reviews if you don't hear of me. Best Regards, -- Matti
On Thu, 2 Mar 2023 12:57:54 +0200 Matti Vaittinen <mazziesaccount@gmail.com> wrote: > Some light sensors can adjust both the HW-gain and integration time. > There are cases where adjusting the integration time has similar impact > to the scale of the reported values as gain setting has. > > IIO users do typically expect to handle scale by a single writable 'scale' > entry. Driver should then adjust the gain/time accordingly. > > It however is difficult for a driver to know whether it should change > gain or integration time to meet the requested scale. Usually it is > preferred to have longer integration time which usually improves > accuracy, but there may be use-cases where long measurement times can be > an issue. Thus it can be preferable to allow also changing the > integration time - but mitigate the scale impact by also changing the gain > underneath. Eg, if integration time change doubles the measured values, > the driver can reduce the HW-gain to half. > > The theory of the computations of gain-time-scale is simple. However, > some people (undersigned) got that implemented wrong for more than once. > > Add some gain-time-scale helpers in order to not dublicate errors in all > drivers needing these computations. > > Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com> Probably makes sense to put the exports in their own namespace. I've been meaning to finish namespacing the rest of IIO but not gotten around to it yet. As this is a separate library probably makes sense to have a unique namespace for it that the users opt in on. Perhaps IIO_GTS makes sense? Otherwise, as Andy's done a detailed review of this version I'll let you update it for those comments and take another look at v3. Thanks, Jonathan
Hi Jonathan, all On 3/4/23 20:35, Jonathan Cameron wrote: > On Thu, 2 Mar 2023 12:57:54 +0200 > Matti Vaittinen <mazziesaccount@gmail.com> wrote: > >> Some light sensors can adjust both the HW-gain and integration time. >> There are cases where adjusting the integration time has similar impact >> to the scale of the reported values as gain setting has. >> >> IIO users do typically expect to handle scale by a single writable 'scale' >> entry. Driver should then adjust the gain/time accordingly. >> >> It however is difficult for a driver to know whether it should change >> gain or integration time to meet the requested scale. Usually it is >> preferred to have longer integration time which usually improves >> accuracy, but there may be use-cases where long measurement times can be >> an issue. Thus it can be preferable to allow also changing the >> integration time - but mitigate the scale impact by also changing the gain >> underneath. Eg, if integration time change doubles the measured values, >> the driver can reduce the HW-gain to half. >> >> The theory of the computations of gain-time-scale is simple. However, >> some people (undersigned) got that implemented wrong for more than once. >> >> Add some gain-time-scale helpers in order to not dublicate errors in all >> drivers needing these computations. >> >> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com> > > Probably makes sense to put the exports in their own namespace. Andy asked for that as well. And, while I do not really see the usefulness of the namespaces when all symbols are properly prefixed (I only see added complexity there) - I agreed to use one. > > I've been meaning to finish namespacing the rest of IIO but not > gotten around to it yet. > As this is a separate library probably makes sense to have a unique > namespace for it that the users opt in on. > Perhaps IIO_GTS makes sense? Thanks. I think I'll use that. Although, as all of the symbols are prefixed with iio_gts - if I really saw a risk of symbol clash it would probably make more sense to use just about anything else ;) This because if someone else were prefixing symbols with iio_gts - he would likely be using the exactly same namespace. > Otherwise, as Andy's done a detailed review of this version I'll let > you update it for those comments and take another look at v3. This suits me fine. I have v3 almost prepared - but I'll be very much away from the computer next week so it may be the v3 will not be out until later. It may be I won't continue work with this until about after a week. Yours, -- Matti
On Fri, Mar 03, 2023 at 07:54:22AM +0000, Vaittinen, Matti wrote: > On 3/2/23 17:05, Andy Shevchenko wrote: > > On Thu, Mar 02, 2023 at 12:57:54PM +0200, Matti Vaittinen wrote: ... > >> +{ > >> + int tmp = 1; > >> + > >> + if (scale > max || !scale) > >> + return -EINVAL; > >> + > >> + if (U64_MAX - max < scale) { > >> + /* Risk of overflow */ > >> + if (max - scale < scale) > >> + return 1; > > > >> + while (max - scale > scale * (u64) tmp) > > > > Space is not required after casting. > > > >> + tmp++; > >> + > >> + return tmp + 1; > > > > Wondering why you can't simplify this to > > > > max -= scale; > > tmp++; > > > > Sorry, but I don't follow. Can you please show me the complete > suggestion? Exactly what should be replaced by the: I don't understand. Do you see the blank lines I specifically added to show the piece of the code I'm commenting on? For your convenience, the while loop and return statement may be replaced with two simple assignments. > > max -= scale; > > tmp++; > >> + } > >> + > >> + while (max > scale * (u64) tmp) > >> + tmp++; > >> + > >> + return tmp; > >> +} ... > >> +EXPORT_SYMBOL_GPL(iio_init_iio_gts); > > > > I haven't noticed if you put these all exports into a proper namespace. > > If no, please do. > > I haven't. And in many cases I actually see very little to no value in > doing so as collisions are unlikely when symbols are appropriately prefixed. This is a benefit of not polluting (global) namespace with something that is rarely used. Please, try to utilize namespaces more in your nice kernel contribution (there is *no* sarcasm). > Anyways, it seems the IIO uses namespaces so let's play along. Any good > suggestions for a name? IIO_GTS ? ... > >> + for (i = gts->num_itime - 2; i >= 0; i--) > > > > Yeah, if you put this into temporary, like > > > > i = gts->num_itime - 1; > > > > this becomes > > > > while (i--) { > > > > I prefer for(). It looks clearer to me... It has too many characters to parse. That's why I prefer while. Are we going to have principle disagreement on this one? > > Note, you may re-use that i (maybe renamed to something better in the > memcpy() > > above as well). > > ...but this makes sense so Ok. ... > > Note, you missed {} for better reading. > > Didn't miss it but left it out intentionally. {} does not help me as > indentiation makes it clear. Can add one for you though. Not for me. For other people who would read 1+ screen long for-loop body (not everybody has 120+ lines of terminal. ... > >> + for (i = 0; !ret && i < gts->num_avail_all_scales; i++) > > > > Much easier to read if you move this... > > > >> + ret = iio_gts_total_gain_to_scale(gts, all_gains[i], > >> + >s->avail_all_scales_table[i * 2], > >> + >s->avail_all_scales_table[i * 2 + 1]); > > > > ...here as > > > > if (ret) > > break; > > I think the !ret in loop condition is obvious. Adding break and brackets > would not improve this. It moves it to the regular pattern. Yours is not so distributed in the kernel. ... > >> + kfree(all_gains); > >> + if (ret && gts->avail_all_scales_table) > >> + kfree(gts->avail_all_scales_table); > >> + > >> + return ret; > > > > But Wouldn't be better to use goto labels? > > I don't see benefit in this case. Handling return value and goto would > require brackets. The current one is much simpler for me. I am just > considering dropping that 'else' which is not needed. At least it would be consistent with the very same file style in another function. So, why there do you goto, and not here where it makes sense to me? ... > >> + while (i) { > > > > Instead of doing standard > > > > while (i--) { > > > >> + /* > >> + * It does not matter if i'th alloc was not succesfull as > >> + * kfree(NULL) is safe. > >> + */ > > > > You add this comment, ... > > > >> + kfree(per_time_gains[i]); > >> + kfree(per_time_scales[i]); > > > > ...an additional loop, ... > > The comment is there to explain what I think you missed. We have two > arrays there. We don't know whether the allocation of first one was > successful so we try freeing both. > > > > >> + > >> + i--; > > > > ...and a line of code. > > > >> + } > > > > Why? > > Because, as the comment says, it does not matter if allocation was > succesfull. kfree(NULL) is safe. Wouldn't be better to avoid no-op kfree() by explicitly putting the call before hands? kfree(...[i]); while (i--) { ... } ? Much better to understand than what you wrote. I have to read comment on top of the code, with my proposal comment won't be needed. ... > >> + for (i = gts->num_itime - 1; i >= 0; i--) { > > > > i = gts->num_itime; > > > > while (i--) { > > I prefer for() when initializing a variable is needed. Furthermore, > having var-- or --var in a condition is less clear. while (x--) _is_ a pattern for cleaning up something. Your for-loop has a lot of additional (unnecessary) code lines that makes it harder to understand (by the people who are not familiar with the pattern). > >> + } ... > >> +void iio_gts_purge_avail_time_table(struct iio_gts *gts) > >> +{ > >> + if (gts->num_avail_time_tables) { > > > > if (!...) > > return; > > Does not improve this as we only have just one small conditional block > of code which we either execute or not. It is clear at a glance. Would > make sense if we had longer function. I believe it makes sense to have like this even for this long function(s). > >> + kfree(gts->avail_time_tables); > >> + gts->avail_time_tables = NULL; > >> + gts->num_avail_time_tables = 0; > >> + } > >> +} ... > >> + if (!diff) { > > > > Why not positive conditional? > > Because !diff is a special condition and we check explicitly for it. And how my suggestion makes it different? (Note, it's easy to miss the ! in the conditionals, that's why positive ones are preferable.) > > > > if (diff) { > > ... > > } else { > > ... > > } > > > >> + } else { > >> + }
On Sat, Mar 04, 2023 at 09:42:22PM +0200, Matti Vaittinen wrote: > On 3/4/23 20:35, Jonathan Cameron wrote: > > On Thu, 2 Mar 2023 12:57:54 +0200 ... > > I've been meaning to finish namespacing the rest of IIO but not > > gotten around to it yet. > > As this is a separate library probably makes sense to have a unique > > namespace for it that the users opt in on. > > Perhaps IIO_GTS makes sense? > > Thanks. I think I'll use that. Although, as all of the symbols are prefixed > with iio_gts - if I really saw a risk of symbol clash it would probably make > more sense to use just about anything else ;) This because if someone else > were prefixing symbols with iio_gts - he would likely be using the exactly > same namespace. There is another aspect, i.e. global namespace pollution. This saves memory if not used.
On 3/6/23 13:13, Andy Shevchenko wrote: > On Fri, Mar 03, 2023 at 07:54:22AM +0000, Vaittinen, Matti wrote: >> On 3/2/23 17:05, Andy Shevchenko wrote: >>> On Thu, Mar 02, 2023 at 12:57:54PM +0200, Matti Vaittinen wrote: > > ... > >>>> +{ >>>> + int tmp = 1; >>>> + >>>> + if (scale > max || !scale) >>>> + return -EINVAL; >>>> + >>>> + if (U64_MAX - max < scale) { >>>> + /* Risk of overflow */ >>>> + if (max - scale < scale) >>>> + return 1; >>> >>>> + while (max - scale > scale * (u64) tmp) >>> >>> Space is not required after casting. >>> >>>> + tmp++; >>>> + >>>> + return tmp + 1; >>> >>> Wondering why you can't simplify this to >>> >>> max -= scale; >>> tmp++; >>> >> >> Sorry, but I don't follow. Can you please show me the complete >> suggestion? Exactly what should be replaced by the: > > I don't understand. Do you see the blank lines I specifically added to show > the piece of the code I'm commenting on? I saw. I, however, didn't see the point so I was thinking I misunderstood what you wanted to replace. > For your convenience, the while loop and return statement may be replaced with > two simple assignments. > >> > max -= scale; >> > tmp++; > Ah, thanks. Yes. I think that would yield the same result. Not sure if it makes the logic more or less obvious, but it would kill one return path which indeed (in my opinion) could make it look prettier. That would require a local variable or dropping the const from max though. Not sure it's worth that though. > ... > >>>> + for (i = gts->num_itime - 2; i >= 0; i--) >>> >>> Yeah, if you put this into temporary, like >>> >>> i = gts->num_itime - 1; >>> >>> this becomes >>> >>> while (i--) { >>> >> >> I prefer for(). It looks clearer to me... > > It has too many characters to parse. That's why I prefer while. > Are we going to have principle disagreement on this one? > I think we have disagreed whether to prefer while() or for() also in the past. I am reluctant to switch for()s to while()s unless there are other obvious benefits. Especially I dislike the change if it involves -- or ++ in the condition. They tend to make it much more difficult (for me) to see what the value is when comparing and what the value is after exiting the loop. Yet, in this particular case I saw the value of re-using the temporary value in memcpy() as you suggest below. >> > Note, you may re-use that i (maybe renamed to something better in the >> memcpy() >> > above as well). >> >> ...but this makes sense so Ok. > > ... > >>>> + for (i = 0; !ret && i < gts->num_avail_all_scales; i++) >>> >>> Much easier to read if you move this... >>> >>>> + ret = iio_gts_total_gain_to_scale(gts, all_gains[i], >>>> + >s->avail_all_scales_table[i * 2], >>>> + >s->avail_all_scales_table[i * 2 + 1]); >>> >>> ...here as >>> >>> if (ret) >>> break; >> >> I think the !ret in loop condition is obvious. Adding break and brackets >> would not improve this. > > It moves it to the regular pattern. Yours is not so distributed in the kernel. I believe we can find examples of both patterns in kernel. I don't think the "many people use different pattern" is a great reason to add break + brackets which (in my eyes) give no additional value to code I am planning to keep reading also in the future... > ... > >>>> + kfree(all_gains); >>>> + if (ret && gts->avail_all_scales_table) >>>> + kfree(gts->avail_all_scales_table); >>>> + >>>> + return ret; >>> >>> But Wouldn't be better to use goto labels? >> >> I don't see benefit in this case. Handling return value and goto would >> require brackets. The current one is much simpler for me. I am just >> considering dropping that 'else' which is not needed. > > At least it would be consistent with the very same file style in another > function. So, why there do you goto, and not here where it makes sense > to me? > Talking about the iio_gts_build_avail_scale_table()? Difference is that the iio_gts_build_avail_scale_table() has multiple checks for "if this happens - error out". I think using goto has valid use-cases there. To tell the truth - in the past I preferred "arrow code" pattern where I initialized return values to default errors and only entered deeper to nested code when checks were successful - changing error to succes only at the deepest nested level. That however seems to rub the wrong way most of the kernel developers. Hence I am adapted to use gotos for error handling when we have more than only a few failure points. In any case, here we can just do with single if-else (and for) without even having the brackets. This is not a deeply nesting complex construct. > ... > >>>> + while (i) { >>> >>> Instead of doing standard >>> >>> while (i--) { >>> >>>> + /* >>>> + * It does not matter if i'th alloc was not succesfull as >>>> + * kfree(NULL) is safe. >>>> + */ >>> >>> You add this comment, ... >>> >>>> + kfree(per_time_gains[i]); >>>> + kfree(per_time_scales[i]); >>> >>> ...an additional loop, ... >> >> The comment is there to explain what I think you missed. We have two >> arrays there. We don't know whether the allocation of first one was >> successful so we try freeing both. >> >>> >>>> + >>>> + i--; >>> >>> ...and a line of code. >>> >>>> + } >>> >>> Why? >> >> Because, as the comment says, it does not matter if allocation was >> succesfull. kfree(NULL) is safe. > > Wouldn't be better to avoid no-op kfree() by explicitly putting the call before > hands? > > kfree(...[i]); > while (i--) { > ... > } > > ? > > Much better to understand than what you wrote. I have to read comment on top of > the code, with my proposal comment won't be needed. True, I can add one kfree() before the loop. It still does not change the fact that we don't know if allocating the per_time_scales[i] was succesful - unless we also add another label there. Well, it is a few more LOC but avoids the kfree() and the comment so not really unbearable trade :) > ... > >>>> + for (i = gts->num_itime - 1; i >= 0; i--) { >>> >>> i = gts->num_itime; >>> >>> while (i--) { >> >> I prefer for() when initializing a variable is needed. Furthermore, >> having var-- or --var in a condition is less clear. > > while (x--) > > _is_ a pattern for cleaning up something. Your for-loop has a lot of additional > (unnecessary) code lines that makes it harder to understand (by the people who > are not familiar with the pattern). > I do strongly disagree with you regarding while (x--) being simpler to understand than respective for. -- in a condition is terrible - and I rarely use it. I know I used it in v3 for the while (time_idx--) { and I am regretting that already. >>>> + } > > ... > >>>> +void iio_gts_purge_avail_time_table(struct iio_gts *gts) >>>> +{ >>>> + if (gts->num_avail_time_tables) { >>> >>> if (!...) >>> return; >> >> Does not improve this as we only have just one small conditional block >> of code which we either execute or not. It is clear at a glance. Would >> make sense if we had longer function. > > I believe it makes sense to have like this even for this long function(s). So, it seems like we do disagree with this too then. To me it is just obvious here to check if we have tables to free and free them. > >>>> + kfree(gts->avail_time_tables); >>>> + gts->avail_time_tables = NULL; >>>> + gts->num_avail_time_tables = 0; >>>> + } >>>> +} > > ... > >>>> + if (!diff) { >>> >>> Why not positive conditional? >> >> Because !diff is a special condition and we check explicitly for it. > > And how my suggestion makes it different? In example you gave we would be checking if the value is anything else but the specific value we are checking for. It is counter intuitive. > (Note, it's easy to miss the ! in the conditionals, that's why positive ones > are preferable.) Thank you for explaining me the rationale behind the "positive checks". I didn't know missing '!' was seen as a thing. I still don't think being afraid of missing '!' is a good reason to switch to counter intuitive checks. A check "if (!foo)" is a pattern in-kernel if anything and in my opinion people really should be aware of it. (I would much more say that having a constant value on left side of a "equality" check is beneficial as people do really occasionally miss one '=' when meaning '=='. Still, this is not strong enough reason to make counter-intuitive checks. In my books 'avoiding negative checks' is much less of a reason as people (in my experience) do not really miss the '!'.) Best Regards -- Matti
On Mon, Mar 13, 2023 at 01:31:42PM +0200, Matti Vaittinen wrote: > On 3/6/23 13:13, Andy Shevchenko wrote: > > On Fri, Mar 03, 2023 at 07:54:22AM +0000, Vaittinen, Matti wrote: > > > On 3/2/23 17:05, Andy Shevchenko wrote: > > > > On Thu, Mar 02, 2023 at 12:57:54PM +0200, Matti Vaittinen wrote: ... > > > > > + for (i = 0; !ret && i < gts->num_avail_all_scales; i++) > > > > > > > > Much easier to read if you move this... > > > > > > > > > + ret = iio_gts_total_gain_to_scale(gts, all_gains[i], > > > > > + >s->avail_all_scales_table[i * 2], > > > > > + >s->avail_all_scales_table[i * 2 + 1]); > > > > > > > > ...here as > > > > > > > > if (ret) > > > > break; > > > > > > I think the !ret in loop condition is obvious. Adding break and brackets > > > would not improve this. > > > > It moves it to the regular pattern. Yours is not so distributed in the kernel. > > I believe we can find examples of both patterns in kernel. I don't think the > "many people use different pattern" is a great reason to add break + > brackets which (in my eyes) give no additional value to code I am planning > to keep reading also in the future... The problem is that your pattern is not so standard (distributed) and hence less maintainable. ... > > > > > + if (!diff) { > > > > > > > > Why not positive conditional? > > > > > > Because !diff is a special condition and we check explicitly for it. > > > > And how my suggestion makes it different? > > In example you gave we would be checking if the value is anything else but > the specific value we are checking for. It is counter intuitive. > > > (Note, it's easy to miss the ! in the conditionals, that's why positive ones > > are preferable.) > > Thank you for explaining me the rationale behind the "positive checks". I > didn't know missing '!' was seen as a thing. > > I still don't think being afraid of missing '!' is a good reason to switch > to counter intuitive checks. A check "if (!foo)" is a pattern in-kernel if > anything and in my opinion people really should be aware of it. > > (I would much more say that having a constant value on left side of a > "equality" check is beneficial as people do really occasionally miss one '=' > when meaning '=='. Still, this is not strong enough reason to make > counter-intuitive checks. In my books 'avoiding negative checks' is much > less of a reason as people (in my experience) do not really miss the '!'.) It's not a problem when it's a common pattern (like you mentioned if (!foo) return -ENOMEM; or alike), but in your case it's not. I would rather see if (diff == 0) which definitely shows the intention and I wouldn't tell a word against it.
On 3/13/23 16:39, Andy Shevchenko wrote: > On Mon, Mar 13, 2023 at 01:31:42PM +0200, Matti Vaittinen wrote: >> On 3/6/23 13:13, Andy Shevchenko wrote: >>> On Fri, Mar 03, 2023 at 07:54:22AM +0000, Vaittinen, Matti wrote: >>>> On 3/2/23 17:05, Andy Shevchenko wrote: >>>>> On Thu, Mar 02, 2023 at 12:57:54PM +0200, Matti Vaittinen wrote: > > ... > >>>>>> + for (i = 0; !ret && i < gts->num_avail_all_scales; i++) >>>>> >>>>> Much easier to read if you move this... >>>>> >>>>>> + ret = iio_gts_total_gain_to_scale(gts, all_gains[i], >>>>>> + >s->avail_all_scales_table[i * 2], >>>>>> + >s->avail_all_scales_table[i * 2 + 1]); >>>>> >>>>> ...here as >>>>> >>>>> if (ret) >>>>> break; >>>> >>>> I think the !ret in loop condition is obvious. Adding break and brackets >>>> would not improve this. >>> >>> It moves it to the regular pattern. Yours is not so distributed in the kernel. >> >> I believe we can find examples of both patterns in kernel. I don't think the >> "many people use different pattern" is a great reason to add break + >> brackets which (in my eyes) give no additional value to code I am planning >> to keep reading also in the future... > > The problem is that your pattern is not so standard (distributed) and hence > less maintainable. I am sorry but I can't really agree with you on this one. For me adding the break and brackets would just complicate the flow and thus decrease the maintainability. > ... > >>>>>> + if (!diff) { >>>>> >>>>> Why not positive conditional? >>>> >>>> Because !diff is a special condition and we check explicitly for it. >>> >>> And how my suggestion makes it different? >> >> In example you gave we would be checking if the value is anything else but >> the specific value we are checking for. It is counter intuitive. >> >>> (Note, it's easy to miss the ! in the conditionals, that's why positive ones >>> are preferable.) >> >> Thank you for explaining me the rationale behind the "positive checks". I >> didn't know missing '!' was seen as a thing. >> >> I still don't think being afraid of missing '!' is a good reason to switch >> to counter intuitive checks. A check "if (!foo)" is a pattern in-kernel if >> anything and in my opinion people really should be aware of it. >> >> (I would much more say that having a constant value on left side of a >> "equality" check is beneficial as people do really occasionally miss one '=' >> when meaning '=='. Still, this is not strong enough reason to make >> counter-intuitive checks. In my books 'avoiding negative checks' is much >> less of a reason as people (in my experience) do not really miss the '!'.) > > It's not a problem when it's a common pattern (like you mentioned > if (!foo) return -ENOMEM; or alike), but in your case it's not. I think we can find plenty of cases where the if (!foo) is used also for other type of checks. To me the argument about people easily missing the ! in if () just do not sound reasonable. > I would rather see if (diff == 0) which definitely shows the intention > and I wouldn't tell a word against it. I think this depends much of the corner of the kernel you have been working with. As far as I remember, in some parts the kernel the check (foo == 0) was actually discouraged, and check (!foo) was preferred. Personally I like !foo much more - but I can tolerate the (foo == 0) in cases where the purpose is to really see if some measure equals to zero. Other uses where I definitely don't want to use "== 0" are for example checking if a flag is clear, pointer is NULL or "magic value" is zero. In this case we are checking for a magic value. Having this check written as: (diff == 0), would actually falsely suggest me we are checking for the difference of gains being zero. That would really be a clever obfuscation and I am certain the code readers would fall on that trap quite easily. Yours, -- Matti
On Tue, Mar 14, 2023 at 12:28:43PM +0200, Matti Vaittinen wrote: > On 3/13/23 16:39, Andy Shevchenko wrote: > > On Mon, Mar 13, 2023 at 01:31:42PM +0200, Matti Vaittinen wrote: > > > On 3/6/23 13:13, Andy Shevchenko wrote: > > > > On Fri, Mar 03, 2023 at 07:54:22AM +0000, Vaittinen, Matti wrote: > > > > > On 3/2/23 17:05, Andy Shevchenko wrote: > > > > > > On Thu, Mar 02, 2023 at 12:57:54PM +0200, Matti Vaittinen wrote: ... > > > > > > > + for (i = 0; !ret && i < gts->num_avail_all_scales; i++) > > > > > > > > > > > > Much easier to read if you move this... > > > > > > > > > > > > > + ret = iio_gts_total_gain_to_scale(gts, all_gains[i], > > > > > > > + >s->avail_all_scales_table[i * 2], > > > > > > > + >s->avail_all_scales_table[i * 2 + 1]); > > > > > > > > > > > > ...here as > > > > > > > > > > > > if (ret) > > > > > > break; > > > > > > > > > > I think the !ret in loop condition is obvious. Adding break and brackets > > > > > would not improve this. > > > > > > > > It moves it to the regular pattern. Yours is not so distributed in the kernel. > > > > > > I believe we can find examples of both patterns in kernel. I don't think the > > > "many people use different pattern" is a great reason to add break + > > > brackets which (in my eyes) give no additional value to code I am planning > > > to keep reading also in the future... > > > > The problem is that your pattern is not so standard (distributed) and hence > > less maintainable. > > I am sorry but I can't really agree with you on this one. For me adding the > break and brackets would just complicate the flow and thus decrease the > maintainability. So, we may start to have a "fundamental disagreements between Matti and Andy on the code style in the Linux kernel" document that we won't clash on this again. At least the amount of these disagreements seems not decreasing in time. ... > > > > > > > + if (!diff) { > > > > > > > > > > > > Why not positive conditional? > > > > > > > > > > Because !diff is a special condition and we check explicitly for it. > > > > > > > > And how my suggestion makes it different? > > > > > > In example you gave we would be checking if the value is anything else but > > > the specific value we are checking for. It is counter intuitive. > > > > > > > (Note, it's easy to miss the ! in the conditionals, that's why positive ones > > > > are preferable.) > > > > > > Thank you for explaining me the rationale behind the "positive checks". I > > > didn't know missing '!' was seen as a thing. > > > I still don't think being afraid of missing '!' is a good reason to switch > > > to counter intuitive checks. A check "if (!foo)" is a pattern in-kernel if > > > anything and in my opinion people really should be aware of it. > > > > > > (I would much more say that having a constant value on left side of a > > > "equality" check is beneficial as people do really occasionally miss one '=' > > > when meaning '=='. Still, this is not strong enough reason to make > > > counter-intuitive checks. In my books 'avoiding negative checks' is much > > > less of a reason as people (in my experience) do not really miss the '!'.) > > > > It's not a problem when it's a common pattern (like you mentioned > > if (!foo) return -ENOMEM; or alike), but in your case it's not. > > I think we can find plenty of cases where the if (!foo) is used also for Pleading to the quantity and not quality is not an argument, right? > other type of checks. To me the argument about people easily missing the ! > in if () just do not sound reasonable. You may theoretically discuss this, I'm telling from my review background and real cases. > > I would rather see if (diff == 0) which definitely shows the intention > > and I wouldn't tell a word against it. > > I think this depends much of the corner of the kernel you have been working > with. As far as I remember, in some parts the kernel the check > (foo == 0) was actually discouraged, and check (!foo) was preferred. Don't you use your common sense? > Personally I like !foo much more - but I can tolerate the (foo == 0) in > cases where the purpose is to really see if some measure equals to zero. > > Other uses where I definitely don't want to use "== 0" are for example > checking if a flag is clear, pointer is NULL or "magic value" is zero. > > In this case we are checking for a magic value. Having this check written > as: (diff == 0), would actually falsely suggest me we are checking for the > difference of gains being zero. That would really be a clever obfuscation > and I am certain the code readers would fall on that trap quite easily. Testing with !diff sounds like it's a boolean kind and makes a false impression that all other values are almost the same meaning which is not the case. Am I right? That's why diff == 0 shows the exact intention here "I would like to check if diff is 0 because this is *special case*". Making !diff creates less visibility on this. Result: Fundamental disagreement between us.
On Tue, Mar 14, 2023 at 01:31:06PM +0200, Andy Shevchenko wrote: > On Tue, Mar 14, 2023 at 12:28:43PM +0200, Matti Vaittinen wrote: > > On 3/13/23 16:39, Andy Shevchenko wrote: > > > On Mon, Mar 13, 2023 at 01:31:42PM +0200, Matti Vaittinen wrote: > > > > On 3/6/23 13:13, Andy Shevchenko wrote: > > > > > On Fri, Mar 03, 2023 at 07:54:22AM +0000, Vaittinen, Matti wrote: > > > > > > On 3/2/23 17:05, Andy Shevchenko wrote: > > > > > > > On Thu, Mar 02, 2023 at 12:57:54PM +0200, Matti Vaittinen wrote: ... > > > > > > > > + if (!diff) { > > > > > > > > > > > > > > Why not positive conditional? > > > > > > > > > > > > Because !diff is a special condition and we check explicitly for it. > > > > > > > > > > And how my suggestion makes it different? > > > > > > > > In example you gave we would be checking if the value is anything else but > > > > the specific value we are checking for. It is counter intuitive. > > > > > > > > > (Note, it's easy to miss the ! in the conditionals, that's why positive ones > > > > > are preferable.) > > > > > > > > Thank you for explaining me the rationale behind the "positive checks". I > > > > didn't know missing '!' was seen as a thing. > > > > I still don't think being afraid of missing '!' is a good reason to switch > > > > to counter intuitive checks. A check "if (!foo)" is a pattern in-kernel if > > > > anything and in my opinion people really should be aware of it. > > > > > > > > (I would much more say that having a constant value on left side of a > > > > "equality" check is beneficial as people do really occasionally miss one '=' > > > > when meaning '=='. Still, this is not strong enough reason to make > > > > counter-intuitive checks. In my books 'avoiding negative checks' is much > > > > less of a reason as people (in my experience) do not really miss the '!'.) > > > > > > It's not a problem when it's a common pattern (like you mentioned > > > if (!foo) return -ENOMEM; or alike), but in your case it's not. > > > > I think we can find plenty of cases where the if (!foo) is used also for > > Pleading to the quantity and not quality is not an argument, right? > > > other type of checks. To me the argument about people easily missing the ! > > in if () just do not sound reasonable. > > You may theoretically discuss this, I'm telling from my review background > and real cases. > > > > I would rather see if (diff == 0) which definitely shows the intention > > > and I wouldn't tell a word against it. > > > > I think this depends much of the corner of the kernel you have been working > > with. As far as I remember, in some parts the kernel the check > > (foo == 0) was actually discouraged, and check (!foo) was preferred. > > Don't you use your common sense? > > > Personally I like !foo much more - but I can tolerate the (foo == 0) in > > cases where the purpose is to really see if some measure equals to zero. > > > > Other uses where I definitely don't want to use "== 0" are for example > > checking if a flag is clear, pointer is NULL or "magic value" is zero. > > > > In this case we are checking for a magic value. Having this check written > > as: (diff == 0), would actually falsely suggest me we are checking for the > > difference of gains being zero. That would really be a clever obfuscation > > and I am certain the code readers would fall on that trap quite easily. > > Testing with !diff sounds like it's a boolean kind and makes a false > impression that all other values are almost the same meaning which is > not the case. Am I right? That's why diff == 0 shows the exact intention > here "I would like to check if diff is 0 because this is *special case*". > > Making !diff creates less visibility on this. > > Result: Fundamental disagreement between us. JFYI: $ git grep -n 'diff.* == 0[^0-9]' -- drivers/ | wc -l 45 (It happens to have same variable name, but you can imagine that there are much more cases with different variable names in use)
On 3/14/23 13:31, Andy Shevchenko wrote: > On Tue, Mar 14, 2023 at 12:28:43PM +0200, Matti Vaittinen wrote: >> On 3/13/23 16:39, Andy Shevchenko wrote: >>> On Mon, Mar 13, 2023 at 01:31:42PM +0200, Matti Vaittinen wrote: >>>> On 3/6/23 13:13, Andy Shevchenko wrote: >>>>> On Fri, Mar 03, 2023 at 07:54:22AM +0000, Vaittinen, Matti wrote: >>>>>> On 3/2/23 17:05, Andy Shevchenko wrote: >>>>>>> On Thu, Mar 02, 2023 at 12:57:54PM +0200, Matti Vaittinen wrote: > > ... > >>>>>>>> + for (i = 0; !ret && i < gts->num_avail_all_scales; i++) >>>>>>> >>>>>>> Much easier to read if you move this... >>>>>>> >>>>>>>> + ret = iio_gts_total_gain_to_scale(gts, all_gains[i], >>>>>>>> + >s->avail_all_scales_table[i * 2], >>>>>>>> + >s->avail_all_scales_table[i * 2 + 1]); >>>>>>> >>>>>>> ...here as >>>>>>> >>>>>>> if (ret) >>>>>>> break; >>>>>> >>>>>> I think the !ret in loop condition is obvious. Adding break and brackets >>>>>> would not improve this. >>>>> >>>>> It moves it to the regular pattern. Yours is not so distributed in the kernel. >>>> >>>> I believe we can find examples of both patterns in kernel. I don't think the >>>> "many people use different pattern" is a great reason to add break + >>>> brackets which (in my eyes) give no additional value to code I am planning >>>> to keep reading also in the future... >>> >>> The problem is that your pattern is not so standard (distributed) and hence >>> less maintainable. >> >> I am sorry but I can't really agree with you on this one. For me adding the >> break and brackets would just complicate the flow and thus decrease the >> maintainability. > > So, we may start to have a "fundamental disagreements between Matti and Andy on > the code style in the Linux kernel" document that we won't clash on this again. > At least the amount of these disagreements seems not decreasing in time. > :) > ... > >>>>>>>> + if (!diff) { >>>>>>> >>>>>>> Why not positive conditional? >>>>>> >>>>>> Because !diff is a special condition and we check explicitly for it. >>>>> >>>>> And how my suggestion makes it different? >>>> >>>> In example you gave we would be checking if the value is anything else but >>>> the specific value we are checking for. It is counter intuitive. >>>> >>>>> (Note, it's easy to miss the ! in the conditionals, that's why positive ones >>>>> are preferable.) >>>> >>>> Thank you for explaining me the rationale behind the "positive checks". I >>>> didn't know missing '!' was seen as a thing. >>>> I still don't think being afraid of missing '!' is a good reason to switch >>>> to counter intuitive checks. A check "if (!foo)" is a pattern in-kernel if >>>> anything and in my opinion people really should be aware of it. >>>> >>>> (I would much more say that having a constant value on left side of a >>>> "equality" check is beneficial as people do really occasionally miss one '=' >>>> when meaning '=='. Still, this is not strong enough reason to make >>>> counter-intuitive checks. In my books 'avoiding negative checks' is much >>>> less of a reason as people (in my experience) do not really miss the '!'.) >>> >>> It's not a problem when it's a common pattern (like you mentioned >>> if (!foo) return -ENOMEM; or alike), but in your case it's not. >> >> I think we can find plenty of cases where the if (!foo) is used also for > > Pleading to the quantity and not quality is not an argument, right? Eh. I think it has been you who has quite often used that as an argument, right? >> other type of checks. To me the argument about people easily missing the ! >> in if () just do not sound reasonable. > > You may theoretically discuss this, I'm telling from my review background > and real cases. I think we all have been reading the code for quite a few years. Thus, also my statements are done based on real cases, or lack of them thereof. Hence a "Shut up, I have the reviewing experience" is not such a strong argument in my books ;) >>> I would rather see if (diff == 0) which definitely shows the intention >>> and I wouldn't tell a word against it. >> >> I think this depends much of the corner of the kernel you have been working >> with. As far as I remember, in some parts the kernel the check >> (foo == 0) was actually discouraged, and check (!foo) was preferred. > > Don't you use your common sense? I try. I believe this is why I so often clash with people who are just more or less blindly following their preferred idioms. >> Personally I like !foo much more - but I can tolerate the (foo == 0) in >> cases where the purpose is to really see if some measure equals to zero. >> >> Other uses where I definitely don't want to use "== 0" are for example >> checking if a flag is clear, pointer is NULL or "magic value" is zero. >> >> In this case we are checking for a magic value. Having this check written >> as: (diff == 0), would actually falsely suggest me we are checking for the >> difference of gains being zero. That would really be a clever obfuscation >> and I am certain the code readers would fall on that trap quite easily. > > Testing with !diff sounds like it's a boolean kind and makes a false > impression that all other values are almost the same meaning which is > not the case. Am I right? That's why diff == 0 shows the exact intention > here "I would like to check if diff is 0 because this is *special case*". To me the diff == 0 would definitely read "difference of gains is zero". And this is NOT what this check is here for. And no, using !var is _not_ a sign of a boolean for me. It might be if this pattern was not used throughout the kernel for checking if something is _not set_, successful, NULL and yes, often also to see it something is non zero. > Making !diff creates less visibility on this. > > Result: Fundamental disagreement between us. We agree on that :)
On Tue, 14 Mar 2023 12:28:43 +0200 Matti Vaittinen <mazziesaccount@gmail.com> wrote: > On 3/13/23 16:39, Andy Shevchenko wrote: > > On Mon, Mar 13, 2023 at 01:31:42PM +0200, Matti Vaittinen wrote: > >> On 3/6/23 13:13, Andy Shevchenko wrote: > >>> On Fri, Mar 03, 2023 at 07:54:22AM +0000, Vaittinen, Matti wrote: > >>>> On 3/2/23 17:05, Andy Shevchenko wrote: > >>>>> On Thu, Mar 02, 2023 at 12:57:54PM +0200, Matti Vaittinen wrote: > > > > ... > > > >>>>>> + for (i = 0; !ret && i < gts->num_avail_all_scales; i++) > >>>>> > >>>>> Much easier to read if you move this... > >>>>> > >>>>>> + ret = iio_gts_total_gain_to_scale(gts, all_gains[i], > >>>>>> + >s->avail_all_scales_table[i * 2], > >>>>>> + >s->avail_all_scales_table[i * 2 + 1]); > >>>>> > >>>>> ...here as > >>>>> > >>>>> if (ret) > >>>>> break; > >>>> > >>>> I think the !ret in loop condition is obvious. Adding break and brackets > >>>> would not improve this. > >>> > >>> It moves it to the regular pattern. Yours is not so distributed in the kernel. > >> > >> I believe we can find examples of both patterns in kernel. I don't think the > >> "many people use different pattern" is a great reason to add break + > >> brackets which (in my eyes) give no additional value to code I am planning > >> to keep reading also in the future... > > > > The problem is that your pattern is not so standard (distributed) and hence > > less maintainable. > > I am sorry but I can't really agree with you on this one. For me adding > the break and brackets would just complicate the flow and thus decrease > the maintainability. I'm with the if (ret) break; school of thought on this one. Never like for loops with complex conditions, I guess because I've trained my eyes to ignore them ;)
diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig index 0d4447df7200..671d84f98c56 100644 --- a/drivers/iio/light/Kconfig +++ b/drivers/iio/light/Kconfig @@ -183,6 +183,9 @@ config IIO_CROS_EC_LIGHT_PROX To compile this driver as a module, choose M here: the module will be called cros_ec_light_prox. +config IIO_GTS_HELPER + tristate + config GP2AP002 tristate "Sharp GP2AP002 Proximity/ALS sensor" depends on I2C diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile index 6f23817fae6f..82d14ebd3c11 100644 --- a/drivers/iio/light/Makefile +++ b/drivers/iio/light/Makefile @@ -20,6 +20,7 @@ obj-$(CONFIG_CM3323) += cm3323.o obj-$(CONFIG_CM3605) += cm3605.o obj-$(CONFIG_CM36651) += cm36651.o obj-$(CONFIG_IIO_CROS_EC_LIGHT_PROX) += cros_ec_light_prox.o +obj-$(CONFIG_IIO_GTS_HELPER) += iio-gts-helper.o obj-$(CONFIG_GP2AP002) += gp2ap002.o obj-$(CONFIG_GP2AP020A00F) += gp2ap020a00f.o obj-$(CONFIG_HID_SENSOR_ALS) += hid-sensor-als.o diff --git a/drivers/iio/light/iio-gts-helper.c b/drivers/iio/light/iio-gts-helper.c new file mode 100644 index 000000000000..df6c2fffe775 --- /dev/null +++ b/drivers/iio/light/iio-gts-helper.c @@ -0,0 +1,1152 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* gain-time-scale conversion helpers for IIO light sensors + * + * Copyright (c) 2023 Matti Vaittinen <mazziesaccount@gmail.com> + */ + +#include <linux/device.h> +#include <linux/errno.h> +#include <linux/export.h> +#include <linux/minmax.h> +#include <linux/module.h> +#include <linux/slab.h> +#include <linux/sort.h> +#include <linux/units.h> + +#include <linux/iio/types.h> + +#include "iio-gts-helper.h" + +/* + * iio_gts_get_gain - Convert scale to total gain + * + * Internal helper for converting scale to total gain. + * + * @max: Maximum linearized scale. As an example, when scale is creted in + * magnitude of NANOs and max scale is 64.1 - The linearized + * scale is 64 100 000 000. + * @scale: Linearized scale to compte the gain for. + * + * Return: (floored) gain corresponding to the scales. -EINVAL if scale + * is invalid. + */ +static int iio_gts_get_gain(const u64 max, const u64 scale) +{ + int tmp = 1; + + if (scale > max || !scale) + return -EINVAL; + + if (U64_MAX - max < scale) { + /* Risk of overflow */ + if (max - scale < scale) + return 1; + + while (max - scale > scale * (u64) tmp) + tmp++; + + return tmp + 1; + } + + while (max > scale * (u64) tmp) + tmp++; + + return tmp; +} +/* + * gain_get_scale_fraction - get the gain or time based on scale and known one + * + * Internal helper for computing unknown fraction of total gain. + * Compute either gain or time based on scale and either the gain or time + * depending on which one is known. + * + * @max: Maximum linearized scale. As an example, when scale is creted in + * magnitude of NANOs and max scale is 64.1 - The linearized + * scale is 64 100 000 000. + * @scale: Linearized scale to compute the gain/time for. + * @known: Either integration time or gain depending on which one is known + * @unknown: Pointer to variable where the computed gain/time is stored + * + * Return: 0 on success + */ +static int gain_get_scale_fraction(const u64 max, u64 scale, int known, + int *unknown) +{ + int tot_gain; + + tot_gain = iio_gts_get_gain(max, scale); + if (tot_gain < 0) + return tot_gain; + + *unknown = tot_gain / known; + + /* We require total gain to be exact multiple of known * unknown */ + if (!*unknown || *unknown * known != tot_gain) + return -EINVAL; + + return 0; +} + +static const struct iio_itime_sel_mul * + iio_gts_find_itime_by_time(struct iio_gts *gts, int time) +{ + int i; + + if (!gts->num_itime) + return NULL; + + for (i = 0; i < gts->num_itime; i++) + if (gts->itime_table[i].time_us == time) + return >s->itime_table[i]; + + return NULL; +} + +static const struct iio_itime_sel_mul * + iio_gts_find_itime_by_sel(struct iio_gts *gts, int sel) +{ + int i; + + for (i = 0; i < gts->num_itime; i++) + if (gts->itime_table[i].sel == sel) + return >s->itime_table[i]; + + return NULL; +} + +static int iio_gts_delinearize(u64 lin_scale, unsigned long scaler, + int *scale_whole, int *scale_nano) +{ + int frac; + + if (scaler > NANO || !scaler) + return -EINVAL; + + frac = do_div(lin_scale, scaler); + + *scale_whole = lin_scale; + *scale_nano = frac * (NANO / scaler); + + return 0; +} + +static int iio_gts_linearize(int scale_whole, int scale_nano, + unsigned long scaler, u64 *lin_scale) +{ + /* + * Expect scale to be (mostly) NANO or MICRO. Divide divider instead of + * multiplication followed by division to avoid overflow + */ + if (scaler > NANO || !scaler) + return -EINVAL; + + *lin_scale = (u64) scale_whole * (u64)scaler + (u64)(scale_nano + / (NANO / scaler)); + + return 0; +} + +/** + * iio_gts_total_gain_to_scale - convert gain to scale + * @gts: Gain time scale descriptor + * @scale_int: Pointer to integral part of the scale (typically val1) + * @scale_nano: Pointer to ractional part of the scale (nano or ppb) + * @gain_tot: the gain to be converted + * + * Convert the total gain value to scale. NOTE: This does not separate gain + * generated by hwgain or integration time. It is up to caller to decide what + * part of the total gain is due to integration time and what due to hw-gain. + * + * Return: 0 on success. Negative errno on failure + */ +int iio_gts_total_gain_to_scale(struct iio_gts *gts, int total_gain, + int *scale_int, int *scale_nano) +{ + u64 tmp; + + tmp = gts->max_scale; + + do_div(tmp, total_gain); + + return iio_gts_delinearize(tmp, NANO, scale_int, scale_nano); +} +EXPORT_SYMBOL_GPL(iio_gts_total_gain_to_scale); + +/** + * iio_gts_scale_to_total_gain - convert scale to gain + * @gts: Gain time scale descriptor + * @scale_int: Integral part of the scale (typically val1) + * @scale_nano: Fractional part of the scale (nano or ppb) + * @gain_tot: pointer to variable where gain is stored + * + * Convert the scale value to total gain. NOTE: This does not separate gain + * generated by hwgain or integration time. It is up to caller to decide what + * part of the total gain is due to integration time and what due to hw-gain. + * + * Return: 0 on success. Negative errno on failure + */ +int iio_gts_scale_to_total_gain(struct iio_gts *gts, int scale_int, + int scale_nano, int *gain_tot) +{ + u64 lin_scale; + int ret; + + ret = iio_gts_linearize(scale_int, scale_nano, NANO, &lin_scale); + if (ret) + return ret; + + ret = iio_gts_get_gain(gts->max_scale, lin_scale); + if (ret < 0) + return ret; + + *gain_tot = ret; + + return 0; +} +EXPORT_SYMBOL_GPL(iio_gts_scale_to_total_gain); + +/** + * iio_init_iio_gts - Initialize the gain-time-scale helper + * @max_scale_int: integer part of the maximum scale value + * @max_scale_nano: fraction part of the maximum scale value + * @gain_tbl: table describing supported gains + * @num_gain: number of gains in the gaintable + * @tim_tbl: table describing supported integration times. Provide + * the integration time table sorted so that the preferred + * integration time is in the first array index. The search + * functions like the + * iio_gts_find_time_and_gain_sel_for_scale() start search + * from first provided time. + * @num_times: number of times in the time table + * @gts: pointer to the helper struct + * + * Initialize the gain-time-scale helper for use. + * + * Return: 0 on success. + */ +int iio_init_iio_gts(int max_scale_int, int max_scale_nano, + const struct iio_gain_sel_pair *gain_tbl, int num_gain, + const struct iio_itime_sel_mul *tim_tbl, int num_times, + struct iio_gts *gts) +{ + int ret; + + ret = iio_gts_linearize(max_scale_int, max_scale_nano, NANO, + >s->max_scale); + if (ret) + return ret; + + gts->hwgain_table = gain_tbl; + gts->num_hwgain = num_gain; + gts->itime_table = tim_tbl; + gts->num_itime = num_times; + gts->per_time_avail_scale_tables = NULL; + gts->avail_time_tables = NULL; + gts->avail_all_scales_table = NULL; + gts->num_avail_all_scales = 0; + + return 0; +} +EXPORT_SYMBOL_GPL(iio_init_iio_gts); + +/** + * iio_gts_purge_avail_scale_table - free-up the available scale tables + * @gts: Gain time scale descriptor + * + * Free the space reserved by iio_gts_build_avail_scale_table(). Please note + * that the helpers for getting available scales like the + * iio_gts_all_avail_scales() are not usable after this call. Thus, this should + * be only called after these helpers can no longer be called (Eg. after + * the iio-device has been deregistered). + */ +void iio_gts_purge_avail_scale_table(struct iio_gts *gts) +{ + int i; + + if (gts->per_time_avail_scale_tables) { + for (i = 0; i < gts->num_itime; i++) + kfree(gts->per_time_avail_scale_tables[i]); + + kfree(gts->per_time_avail_scale_tables); + gts->per_time_avail_scale_tables = NULL; + } + + kfree(gts->avail_all_scales_table); + gts->avail_all_scales_table = NULL; + + gts->num_avail_all_scales = 0; +} +EXPORT_SYMBOL_GPL(iio_gts_purge_avail_scale_table); + +static int iio_gts_gain_cmp(const void *a, const void *b) +{ + return *(int *)a - *(int *)b; +} + +static int gain_to_scaletables(struct iio_gts *gts, int **gains, int **scales) +{ + int ret, i, j, new_idx; + int *all_gains; + + for (i = 0; i < gts->num_itime; i++) { + /* + * Sort the tables for nice output and for easier finding of + * unique values + */ + sort(gains[i], gts->num_hwgain, sizeof(int), iio_gts_gain_cmp, + NULL); + + /* Convert gains to scales */ + for (j = 0; j < gts->num_hwgain; j++) { + ret = iio_gts_total_gain_to_scale(gts, gains[i][j], + &scales[i][2 * j], + &scales[i][2 * j + 1]); + if (ret) + return ret; + } + } + + all_gains = kcalloc(gts->num_itime * gts->num_hwgain, sizeof(int), + GFP_KERNEL); + if (!all_gains) + return -ENOMEM; + + /* + * We assume all the gains for same integration time were unique. + * It is likely the first time table had greatest time multiplier as + * the times are in the order of preference and greater times are + * usually preferred. Hence we start from the last table which is likely + * to have the smallest total gains + */ + memcpy(all_gains, gains[gts->num_itime - 1], gts->num_hwgain * sizeof(int)); + new_idx = gts->num_hwgain; + + for (i = gts->num_itime - 2; i >= 0; i--) + for (j = 0; j < gts->num_hwgain; j++) { + int candidate = gains[i][j]; + int chk; + + if (candidate > all_gains[new_idx - 1]) { + all_gains[new_idx] = candidate; + new_idx++; + + continue; + } + for (chk = 0; chk < new_idx; chk++) + if (candidate <= all_gains[chk]) + break; + + if (candidate == all_gains[chk]) + continue; + + memmove(&all_gains[chk + 1], &all_gains[chk], + (new_idx - chk) * sizeof(int)); + all_gains[chk] = candidate; + new_idx++; + } + + gts->num_avail_all_scales = new_idx; + + gts->avail_all_scales_table = kcalloc(gts->num_avail_all_scales, + 2 * sizeof(int), GFP_KERNEL); + if (!gts->avail_all_scales_table) + ret = -ENOMEM; + else + for (i = 0; !ret && i < gts->num_avail_all_scales; i++) + ret = iio_gts_total_gain_to_scale(gts, all_gains[i], + >s->avail_all_scales_table[i * 2], + >s->avail_all_scales_table[i * 2 + 1]); + + kfree(all_gains); + if (ret && gts->avail_all_scales_table) + kfree(gts->avail_all_scales_table); + + return ret; +} + +/** + * iio_gts_build_avail_scale_table - create tables of available scales + * @gts: Gain time scale descriptor + * + * Build the tables which can represent the available scales based on the + * originally given gain and time tables. When both time and gain tables are + * given this results: + * 1. A set of tables representing available scales for each supported + * integration time. + * 2. A single table listing all the unique scales that any combination of + * supported gains and times can provide. + * + * NOTE: Space allocated for the tables must be freed using + * iio_gts_purge_avail_scale_table() when the tables are no longer needed. + * + * Return: 0 on success. + */ +int iio_gts_build_avail_scale_table(struct iio_gts *gts) +{ + int **per_time_gains, **per_time_scales, i, j, ret = -ENOMEM; + + per_time_gains = kcalloc(gts->num_itime, sizeof(int *), GFP_KERNEL); + if (!per_time_gains) + return ret; + + per_time_scales = kcalloc(gts->num_itime, sizeof(int *), GFP_KERNEL); + if (!per_time_scales) + goto free_gains; + + for (i = 0; i < gts->num_itime; i++) { + per_time_scales[i] = kcalloc(gts->num_hwgain, 2 * sizeof(int), + GFP_KERNEL); + if (!per_time_scales[i]) + goto err_free_out; + + per_time_gains[i] = kcalloc(gts->num_hwgain, sizeof(int), + GFP_KERNEL); + if (!per_time_gains[i]) + goto err_free_out; + + + for (j = 0; j < gts->num_hwgain; j++) + per_time_gains[i][j] = gts->hwgain_table[j].gain * + gts->itime_table[i].mul; + } + + ret = gain_to_scaletables(gts, per_time_gains, per_time_scales); + if (ret) + goto err_free_out; + + kfree(per_time_gains); + gts->per_time_avail_scale_tables = per_time_scales; + + return 0; + +err_free_out: + while (i) { + /* + * It does not matter if i'th alloc was not succesfull as + * kfree(NULL) is safe. + */ + kfree(per_time_gains[i]); + kfree(per_time_scales[i]); + + i--; + } + kfree(per_time_scales); +free_gains: + kfree(per_time_gains); + + return ret; +} +EXPORT_SYMBOL_GPL(iio_gts_build_avail_scale_table); + +static inline void devm_iio_gts_avail_scale_drop(void *res) +{ + iio_gts_purge_avail_scale_table(res); +} + +/** + * devm_iio_gts_build_avail_scale_table - do managed tables of available scales + * @gts: Gain time scale descriptor + * @dev: Device whose life-time tables are bound to + * + * Create tables of available scales which are freed upon the device detach. + * See the iio_gts_build_avail_scale_table() for more information. + * + * NOTE: after the tables have been purged, the helpers for getting the + * available scales are no longer usable. Care must be taken that unwinding + * is done in correct order (iio device is deregistered prior purging the + * tables). + * + * Return: 0 on success. + */ +int devm_iio_gts_build_avail_scale_table(struct device *dev, + struct iio_gts *gts) +{ + int ret; + + ret = iio_gts_build_avail_scale_table(gts); + if (ret) + return ret; + + return devm_add_action_or_reset(dev, devm_iio_gts_avail_scale_drop, gts); +} +EXPORT_SYMBOL_GPL(devm_iio_gts_build_avail_scale_table); + +/** + * iio_gts_build_avail_time_table - build table of available integration times + * @gts: Gain time scale descriptor + * + * Build the table which can represent the available times to be returned + * to users using the read_avail-callback. + * + * NOTE: Space allocated for the tables must be freed using + * iio_gts_purge_avail_time_table() when the tables are no longer needed. + * + * Return: 0 on success. + */ +int iio_gts_build_avail_time_table(struct iio_gts *gts) +{ + int *times, i, j, idx = 0; + + if (!gts->num_itime) + return 0; + + times = kcalloc(gts->num_itime, sizeof(int), GFP_KERNEL); + if (!times) + return -ENOMEM; + + for (i = gts->num_itime - 1; i >= 0; i--) { + int new = gts->itime_table[i].time_us; + + if (times[idx] < new) { + times[idx++] = new; + continue; + } + + for (j = 0; j <= idx; j++) { + if (times[j] > new) { + memmove(×[j + 1], ×[j], (idx - j) * sizeof(int)); + times[j] = new; + idx++; + } + } + } + gts->avail_time_tables = times; + /* + * This is just to survive a unlikely corner-case where times in the + * given time table were not unique. Else we could just trust the + * gts->num_itime. + */ + gts->num_avail_time_tables = idx; + + return 0; +} +EXPORT_SYMBOL_GPL(iio_gts_build_avail_time_table); + +/** + * iio_gts_purge_avail_time_table - free-up the available integration time table + * @gts: Gain time scale descriptor + * + * Free the space reserved by iio_gts_build_avail_time_table(). Please note + * that the helpers for getting available integration times like the + * iio_gts_avail_times() are not usable after this call. Thus, this should + * be only called after these helpers can no longer be called (Eg. after + * the iio-device has been deregistered). + */ +void iio_gts_purge_avail_time_table(struct iio_gts *gts) +{ + if (gts->num_avail_time_tables) { + kfree(gts->avail_time_tables); + gts->avail_time_tables = NULL; + gts->num_avail_time_tables = 0; + } +} +EXPORT_SYMBOL_GPL(iio_gts_purge_avail_time_table); + +static inline void devm_iio_gts_avail_time_drop(void *res) +{ + iio_gts_purge_avail_time_table(res); +} + +/** + * devm_iio_gts_build_avail_time_table - do managed tables of available times + * @gts: Gain time scale descriptor + * @dev: Device whose life-time tables are bound to + * + * Create a table of available integration times. Table is freed upon the + * device detach. See the iio_gts_build_avail_time_table() for more information. + * + * NOTE: after the tables have been purged, the helpers for getting + * available integration times are no longer usable. Care must be taken that + * unwinding is done in correct order (iio device is deregistered prior + * purging the tables). + * + * Return: 0 on success. + */ +int devm_iio_gts_build_avail_time_table(struct device *dev, struct iio_gts *gts) +{ + int ret; + + if (!gts->num_itime) + return 0; + + ret = iio_gts_build_avail_time_table(gts); + if (ret) + return ret; + + return devm_add_action_or_reset(dev, devm_iio_gts_avail_time_drop, gts); +} +EXPORT_SYMBOL_GPL(devm_iio_gts_build_avail_time_table); + +/** + * iio_gts_build_avail_tables - create tables of available scales and int times + * @gts: Gain time scale descriptor + * + * Build the tables which can represent the available scales and available + * integration times. Availability tables are built based on the originally + * given gain and given time tables. + * + * When both time and gain tables are + * given this results: + * 1. A set of sorted tables representing available scales for each supported + * integration time. + * 2. A single sorted table listing all the unique scales that any combination + * of supported gains and times can provide. + * 3. A sorted table of supported integration times + * + * After these tables are built one can use the iio_gts_all_avail_scales(), + * iio_gts_avail_scales_for_time() and iio_gts_avail_times() helpers to + * implement the read_avail opeations. + * + * NOTE: Space allocated for the tables must be freed using + * iio_gts_purge_avail_tables() when the tables are no longer needed. + * + * Return: 0 on success. + */ +int iio_gts_build_avail_tables(struct iio_gts *gts) +{ + int ret; + + ret = iio_gts_build_avail_scale_table(gts); + if (ret) + return ret; + + ret = iio_gts_build_avail_time_table(gts); + if (ret) + iio_gts_purge_avail_scale_table(gts); + + return ret; +} +EXPORT_SYMBOL_GPL(iio_gts_build_avail_tables); + +/** + * iio_gts_purge_avail_tables - free-up the availability tables + * @gts: Gain time scale descriptor + * + * Free the space reserved by iio_gts_build_avail_tables(). Frees both the + * integration time and scale tables. + * + * Note that the helpers for getting available integration times or scales + * like the iio_gts_avail_times() are not usable after this call. Thus, this + * should be only called after these helpers can no longer be called (Eg. + * after the iio-device has been deregistered). + */ +void iio_gts_purge_avail_tables(struct iio_gts *gts) +{ + iio_gts_purge_avail_time_table(gts); + iio_gts_purge_avail_scale_table(gts); +} +EXPORT_SYMBOL_GPL(iio_gts_purge_avail_tables); + +static void devm_iio_gts_avail_all_drop(void *res) +{ + iio_gts_purge_avail_tables(res); +} + +/** + * devm_iio_gts_build_avail_tables - manged add availability tables + * @gts: Gain time scale descriptor + * + * Build the tables which can represent the available scales and available + * integration times. Availability tables are built based on the originally + * given gain and given time tables. + * + * When both time and gain tables are + * given this results: + * 1. A set of sorted tables representing available scales for each supported + * integration time. + * 2. A single sorted table listing all the unique scales that any combination + * of supported gains and times can provide. + * 3. A sorted table of supported integration times + * + * After these tables are built one can use the iio_gts_all_avail_scales(), + * iio_gts_avail_scales_for_time() and iio_gts_avail_times() helpers to + * implement the read_avail opeations. + * + * The tables are automatically released upon device detach. + * + * NOTE: after the tables have been purged, the helpers for getting + * available scales / integration times are no longer usable. Care must be + * taken that unwinding is done in correct order (iio device is deregistered + * prior purging the tables). + * + * Return: 0 on success. + */ +int devm_iio_gts_build_avail_tables(struct device *dev, struct iio_gts *gts) +{ + int ret; + + ret = iio_gts_build_avail_tables(gts); + if (ret) + return ret; + + return devm_add_action_or_reset(dev, devm_iio_gts_avail_all_drop, gts); +} +EXPORT_SYMBOL_GPL(devm_iio_gts_build_avail_tables); + +/** + * iio_gts_all_avail_scales - helper for listing all available scales + * @gts: Gain time scale descriptor + * @vals: Returned array of supported scales + * @type: Type of returned scale values + * @length: Amount of returned values in array + * + * Returns a value suitable to be returned from read_avail or a negative error + */ +int iio_gts_all_avail_scales(struct iio_gts *gts, const int **vals, int *type, + int *length) +{ + /* + * Using this function prior building the tables is a driver-error + * which should be fixed when the driver is tested for a first time + */ + if (WARN_ON(!gts->num_avail_all_scales)) + return -EINVAL; + + *vals = gts->avail_all_scales_table; + *type = IIO_VAL_INT_PLUS_NANO; + *length = gts->num_avail_all_scales * 2; + + return IIO_AVAIL_LIST; +} +EXPORT_SYMBOL_GPL(iio_gts_all_avail_scales); + +/** + * iio_gts_avail_scales_for_time - list scales for integration time + * @gts: Gain time scale descriptor + * @time: Integration time for which the scales are listed + * @vals: Returned array of supported scales + * @type: Type of returned scale values + * @length: Amount of returned values in array + * + * Drivers which do not allow scale setting to change integration time can + * use this helper to list only the scales whic are valid for given integration + * time. + * + * Returns a value suitable to be returned from read_avail or a negative error + */ +int iio_gts_avail_scales_for_time(struct iio_gts *gts, int time, + const int **vals, int *type, int *length) +{ + int i; + + for (i = 0; i < gts->num_itime; i++) + if (gts->itime_table[i].time_us == time) + break; + + if (i == gts->num_itime) + return -EINVAL; + + *vals = gts->per_time_avail_scale_tables[i]; + *type = IIO_VAL_INT_PLUS_NANO; + *length = gts->num_hwgain * 2; + + return IIO_AVAIL_LIST; +} +EXPORT_SYMBOL_GPL(iio_gts_avail_scales_for_time); + +/** + * iio_gts_avail_times - helper for listing available integration times + * @gts: Gain time scale descriptor + * @vals: Returned array of supported timees + * @type: Type of returned scale values + * @length: Amount of returned values in array + * + * Returns a value suitable to be returned from read_avail or a negative error + */ +int iio_gts_avail_times(struct iio_gts *gts, const int **vals, int *type, + int *length) +{ + /* + * Using this function prior building the tables is a driver-error + * which should be fixed when the driver is tested for a first time + */ + if (WARN_ON(!gts->num_avail_time_tables)) + return -EINVAL; + + *vals = gts->avail_time_tables; + *type = IIO_VAL_INT; + *length = gts->num_avail_time_tables; + + return IIO_AVAIL_LIST; +} +EXPORT_SYMBOL_GPL(iio_gts_avail_times); + +/** + * iio_gts_valid_time - check if given integration time is valid + * @gts: Gain time scale descriptor + * @time_us: Integration time to check + * + * Return: True if given time is supported by device. False if not + */ +bool iio_gts_valid_time(struct iio_gts *gts, int time_us) +{ + return iio_gts_find_itime_by_time(gts, time_us); +} +EXPORT_SYMBOL_GPL(iio_gts_valid_time); + +int iio_gts_find_sel_by_gain(struct iio_gts *gts, int gain) +{ + int i; + + for (i = 0; i < gts->num_hwgain; i++) + if (gts->hwgain_table[i].gain == gain) + return gts->hwgain_table[i].sel; + + return -EINVAL; +} +EXPORT_SYMBOL_GPL(iio_gts_find_sel_by_gain); + +bool iio_gts_valid_gain(struct iio_gts *gts, int gain) +{ + return iio_gts_find_sel_by_gain(gts, gain) >= 0; +} +EXPORT_SYMBOL_GPL(iio_gts_valid_gain); + +int iio_gts_find_gain_by_sel(struct iio_gts *gts, int sel) +{ + int i; + + for (i = 0; i < gts->num_hwgain; i++) + if (gts->hwgain_table[i].sel == sel) + return gts->hwgain_table[i].gain; + + return -EINVAL; +} +EXPORT_SYMBOL_GPL(iio_gts_find_gain_by_sel); + +int iio_gts_get_min_gain(struct iio_gts *gts) +{ + int i, min = -EINVAL; + + for (i = 0; i < gts->num_hwgain; i++) { + int gain = gts->hwgain_table[i].gain; + + if (min == -EINVAL) + min = gain; + else + min = min(min, gain); + } + + return min; +} +EXPORT_SYMBOL_GPL(iio_gts_get_min_gain); + +/** + * iio_find_closest_gain_low - Find the closest lower matching gain + * @gts: Gain time scale descriptor + * @gain: reference gain for which the closest match is searched + * @in_range: indicate if the reference gain was actually in the range of + * supported gains. + * + * Search for closest supported gain that is lower than or equal to the + * gain given as a parameter. This is usable for drivers which do not require + * user to request exact matching gain but rather fo rounding to a supported + * gain value which is equal or lower (setting lower gain is typical for + * avoiding saturation) + * + * Return: The closest matching supported gain or -EINVAL is reference + * gain was smaller than the smallest supported gain. + */ +int iio_find_closest_gain_low(struct iio_gts *gts, int gain, bool *in_range) +{ + int i, diff = 0, min = 0; + int best = -1; + + *in_range = false; + + for (i = 0; i < gts->num_hwgain; i++) { + /* + * It is not expected this function is called for an exactly + * matching gain. + */ + if (unlikely(gain == gts->hwgain_table[i].gain)) { + *in_range = true; + return gain; + } + if (!min) + min = gts->hwgain_table[i].gain; + else + min = min(min, gts->hwgain_table[i].gain); + if (gain > gts->hwgain_table[i].gain) { + if (!diff) { + diff = gain - gts->hwgain_table[i].gain; + best = i; + } else { + int tmp = gain - gts->hwgain_table[i].gain; + + if (tmp < diff) { + diff = tmp; + best = i; + } + } + } else { + /* + * We found valid hwgain which is greater than + * reference. So, unless we return a failure below we + * will have found an in-range gain + */ + *in_range = true; + } + } + /* The requested gain was smaller than anything we support */ + if (!diff) { + *in_range = false; + + return -EINVAL; + } + + return gts->hwgain_table[best].gain; +} +EXPORT_SYMBOL_GPL(iio_find_closest_gain_low); + +int iio_gts_get_int_time_gain_multiplier_by_sel(struct iio_gts *gts, + int sel) +{ + const struct iio_itime_sel_mul *time; + + time = iio_gts_find_itime_by_sel(gts, sel); + if (!time) + return -EINVAL; + + return time->mul; +} +EXPORT_SYMBOL_GPL(iio_gts_get_int_time_gain_multiplier_by_sel); + +/** + * iio_gts_find_gain_for_scale_using_time - Find gain by time and scale + * @gts: Gain time scale descriptor + * @time_sel: Integration time selector correspondig to the time gain is + * searhed for + * @scale_int: Integral part of the scale (typically val1) + * @scale_nano: Fractional part of the scale (nano or ppb) + * @gain: Pointer to value where gain is stored. + * + * In some cases the light sensors may want to find a gain setting which + * corresponds given scale and integration time. Sensors which fill the + * gain and time tables may use this helper to retrieve the gain. + * + * Return: 0 on success. -EINVAL if gain matching the parameters is not + * found. + */ +int iio_gts_find_gain_for_scale_using_time(struct iio_gts *gts, int time_sel, + int scale_int, int scale_nano, + int *gain) +{ + u64 scale_linear; + int ret, mul; + + ret = iio_gts_linearize(scale_int, scale_nano, NANO, &scale_linear); + if (ret) + return ret; + + ret = iio_gts_get_int_time_gain_multiplier_by_sel(gts, time_sel); + if (ret < 0) + return ret; + + mul = ret; + + ret = gain_get_scale_fraction(gts->max_scale, scale_linear, mul, gain); + + if (ret || !iio_gts_valid_gain(gts, *gain)) + return -EINVAL; + + return 0; +} +EXPORT_SYMBOL_GPL(iio_gts_find_gain_for_scale_using_time); + +/* + * iio_gts_find_gain_sel_for_scale_using_time - Fetch gain selector. + * See iio_gts_find_gain_for_scale_using_time() for more information + */ +int iio_gts_find_gain_sel_for_scale_using_time(struct iio_gts *gts, int time_sel, + int scale_int, int scale_nano, + int *gain_sel) +{ + int gain, ret; + + ret = iio_gts_find_gain_for_scale_using_time(gts, time_sel, scale_int, + scale_nano, &gain); + if (ret) + return ret; + + ret = iio_gts_find_sel_by_gain(gts, gain); + if (ret < 0) + return ret; + + *gain_sel = ret; + + return 0; +} +EXPORT_SYMBOL_GPL(iio_gts_find_gain_sel_for_scale_using_time); + +int iio_gts_find_time_and_gain_sel_for_scale(struct iio_gts *gts, int scale_int, + int scale_nano, int *gain_sel, + int *time_sel) +{ + int ret, i; + + for (i = 0; i < gts->num_itime; i++) { + *time_sel = gts->itime_table[i].sel; + ret = iio_gts_find_gain_sel_for_scale_using_time(gts, *time_sel, + scale_int, scale_nano, gain_sel); + if (!ret) + return 0; + } + + return -EINVAL; +} +EXPORT_SYMBOL_GPL(iio_gts_find_time_and_gain_sel_for_scale); + +int iio_gts_find_int_time_by_sel(struct iio_gts *gts, int sel) +{ + const struct iio_itime_sel_mul *itime; + + itime = iio_gts_find_itime_by_sel(gts, sel); + if (!itime) + return -EINVAL; + + return itime->time_us; +} +EXPORT_SYMBOL_GPL(iio_gts_find_int_time_by_sel); + +int iio_gts_find_sel_by_int_time(struct iio_gts *gts, int time) +{ + const struct iio_itime_sel_mul *itime; + + itime = iio_gts_find_itime_by_time(gts, time); + if (!itime) + return -EINVAL; + + return itime->sel; +} +EXPORT_SYMBOL_GPL(iio_gts_find_sel_by_int_time); + +int iio_gts_get_total_gain_by_sel(struct iio_gts *gts, int gsel, int tsel) +{ + int ret, tmp; + + ret = iio_gts_find_gain_by_sel(gts, gsel); + if (ret < 0) + return ret; + + tmp = ret; + + /* + * TODO: Would these helpers provde any value for cases where we just + * use table of gains and no integration time? This would be a standard + * format for gain table representation and regval => gain / gain => + * regval conversions. OTOH, a dummy table based conversion is a memory + * hog in cases where the gain could be computed simply based on simple + * multiplication / bit-shift or by linear_ranges helpers. + * + * Currently we return an error if int-time table is not populated. + */ + ret = iio_gts_get_int_time_gain_multiplier_by_sel(gts, tsel); + if (ret < 0) + return ret; + + return tmp * ret; +} +EXPORT_SYMBOL_GPL(iio_gts_get_total_gain_by_sel); + +int iio_gts_get_total_gain(struct iio_gts *gts, int gain, int time) +{ + const struct iio_itime_sel_mul *itime; + + if (!iio_gts_valid_gain(gts, gain)) + return -EINVAL; + + if (!gts->num_itime) + return gain; + + itime = iio_gts_find_itime_by_time(gts, time); + if (!itime) + return -EINVAL; + + return gain * itime->mul; +} +EXPORT_SYMBOL(iio_gts_get_total_gain); + +static int iio_gts_get_scale_linear(struct iio_gts *gts, int gain, int time, + u64 *scale) +{ + int total_gain; + u64 tmp; + + total_gain = iio_gts_get_total_gain(gts, gain, time); + if (total_gain < 0) + return total_gain; + + tmp = gts->max_scale; + + do_div(tmp, total_gain); + + *scale = tmp; + + return 0; +} + +int iio_gts_get_scale(struct iio_gts *gts, int gain, int time, int *scale_int, + int *scale_nano) +{ + u64 lin_scale; + int ret; + + ret = iio_gts_get_scale_linear(gts, gain, time, &lin_scale); + if (ret) + return ret; + + return iio_gts_delinearize(lin_scale, NANO, scale_int, scale_nano); +} +EXPORT_SYMBOL_GPL(iio_gts_get_scale); + +/** + * iio_gts_find_new_gain_sel_by_old_gain_time - compensate time change + * @gts: Gain time scale descriptor + * @old_gain: Previously set gain + * @old_time_sel: Selector corresponding previously set time + * @new_time_sel: Selector corresponding new time to be set + * @new_gain: Pointer to value where new gain is to be written + * + * We may want to mitigate the scale change caused by setting a new integration + * time (for a light sensor) by also updating the (HW)gain. This helper computes + * new gain value to maintain the scale with new integration time. + * + * Return: 0 on success. -EINVAL if gain matching the new time is not found. + */ +int iio_gts_find_new_gain_sel_by_old_gain_time(struct iio_gts *gts, + int old_gain, int old_time_sel, + int new_time_sel, int *new_gain) +{ + const struct iio_itime_sel_mul *itime_old, *itime_new; + u64 scale; + int ret; + + itime_old = iio_gts_find_itime_by_sel(gts, old_time_sel); + if (!itime_old) + return -EINVAL; + + itime_new = iio_gts_find_itime_by_sel(gts, new_time_sel); + if (!itime_new) + return -EINVAL; + + ret = iio_gts_get_scale_linear(gts, old_gain, itime_old->time_us, + &scale); + if (ret) + return ret; + + ret = gain_get_scale_fraction(gts->max_scale, scale, itime_new->mul, + new_gain); + if (ret) + return -EINVAL; + + if (!iio_gts_valid_gain(gts, *new_gain)) + return -EINVAL; + + return 0; +} +EXPORT_SYMBOL_GPL(iio_gts_find_new_gain_sel_by_old_gain_time); + +MODULE_LICENSE("GPL"); +MODULE_AUTHOR("Matti Vaittinen <mazziesaccount@gmail.com>"); +MODULE_DESCRIPTION("IIO light sensor gain-time-scale helpers"); diff --git a/drivers/iio/light/iio-gts-helper.h b/drivers/iio/light/iio-gts-helper.h new file mode 100644 index 000000000000..7b1f6a0eeb75 --- /dev/null +++ b/drivers/iio/light/iio-gts-helper.h @@ -0,0 +1,130 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* gain-time-scale conversion helpers for IIO light sensors + * + * Copyright (c) 2023 Matti Vaittinen <mazziesaccount@gmail.com> + */ + +#ifndef __GAIN_TIME_SCALE_HELPER__ +#define __GAIN_TIME_SCALE_HELPER__ + +/** + * struct iio_gain_sel_pair - gain - selector values + * + * In many cases devices like light sensors allow setting signal amplification + * (gain) using a register interface. This structure describes amplification + * and corresponding selector (register value) + * + * @gain: Gain (multiplication) value. + * @sel: Selector (usually register value) used to indicate this gain + */ +struct iio_gain_sel_pair { + int gain; + int sel; +}; + +/** + * struct iio_itime_sel_mul - integration time description + * + * In many cases devices like light sensors allow setting the duration of + * collecting data. Typically this duration has also an impact to the magnitude + * of measured values (gain). This structure describes the relation of + * integration time and amplification as well as corresponding selector + * (register value). + * + * An example could be a sensor allowing 50, 100, 200 and 400 mS times. The + * respective multiplication values could be 50 mS => 1, 100 mS => 2, + * 200 mS => 4 and 400 mS => 8 assuming the impact of integration time would be + * linear in a way that when collecting data for 50 mS caused value X, doubling + * the data collection time caused value 2X etc.. + * + * @time_us: Integration time in microseconds. + * @sel: Selector (usually register value) used to indicate this time + * @mul: Multiplication to the values caused by this time. + */ +struct iio_itime_sel_mul { + int time_us; + int sel; + int mul; +}; + +struct iio_gts { + u64 max_scale; + const struct iio_gain_sel_pair *hwgain_table; + int num_hwgain; + const struct iio_itime_sel_mul *itime_table; + int num_itime; + int **per_time_avail_scale_tables; + int *avail_all_scales_table; + int num_avail_all_scales; + int *avail_time_tables; + int num_avail_time_tables; +}; + +#define GAIN_SCALE_GAIN(_gain, _sel) \ +{ \ + .gain = (_gain), \ + .sel = (_sel), \ +} + +#define GAIN_SCALE_ITIME_US(_itime, _sel, _mul) \ +{ \ + .time_us = (_itime), \ + .sel = (_sel), \ + .mul = (_mul), \ +} + +int iio_init_iio_gts(int max_scale_int, int max_scale_nano, + const struct iio_gain_sel_pair *gain_tbl, int num_gain, + const struct iio_itime_sel_mul *tim_tbl, int num_times, + struct iio_gts *gts); + +bool iio_gts_valid_gain(struct iio_gts *gts, int gain); +bool iio_gts_valid_time(struct iio_gts *gts, int time_us); + +int iio_gts_get_total_gain_by_sel(struct iio_gts *gts, int gsel, int tsel); +int iio_gts_get_total_gain(struct iio_gts *gts, int gain, int time); + +int iio_find_closest_gain_low(struct iio_gts *gts, int gain, bool *in_range); +int iio_gts_find_gain_by_sel(struct iio_gts *gts, int sel); +int iio_gts_find_sel_by_gain(struct iio_gts *gts, int gain); +int iio_gts_get_min_gain(struct iio_gts *gts); +int iio_gts_find_int_time_by_sel(struct iio_gts *gts, int sel); +int iio_gts_find_sel_by_int_time(struct iio_gts *gts, int time); + +int iio_gts_get_int_time_gain_multiplier_by_sel(struct iio_gts *gts, + int sel); +int iio_gts_total_gain_to_scale(struct iio_gts *gts, int total_gain, + int *scale_int, int *scale_nano); +int iio_gts_scale_to_total_gain(struct iio_gts *gts, int scale_int, + int scale_nano, int *gain_tot); +int iio_gts_find_gain_sel_for_scale_using_time(struct iio_gts *gts, int time_sel, + int scale_int, int scale_nano, + int *gain_sel); +int iio_gts_find_gain_for_scale_using_time(struct iio_gts *gts, int time_sel, + int scale_int, int scale_nano, + int *gain); +int iio_gts_find_time_and_gain_sel_for_scale(struct iio_gts *gts, int scale_int, + int scale_nano, int *gain_sel, + int *time_sel); +int iio_gts_get_scale(struct iio_gts *gts, int gain, int time, int *scale_int, + int *scale_nano); +int iio_gts_find_new_gain_sel_by_old_gain_time(struct iio_gts *gts, + int old_gain, int old_time_sel, + int new_time_sel, int *new_gain); +int iio_gts_build_avail_tables(struct iio_gts *gts); +int devm_iio_gts_build_avail_tables(struct device *dev, struct iio_gts *gts); +int iio_gts_build_avail_scale_table(struct iio_gts *gts); +int devm_iio_gts_build_avail_scale_table(struct device *dev, struct iio_gts *gts); +int iio_gts_build_avail_time_table(struct iio_gts *gts); +int devm_iio_gts_build_avail_time_table(struct device *dev, struct iio_gts *gts); +void iio_gts_purge_avail_scale_table(struct iio_gts *gts); +void iio_gts_purge_avail_time_table(struct iio_gts *gts); +void iio_gts_purge_avail_tables(struct iio_gts *gts); +int iio_gts_avail_times(struct iio_gts *gts, const int **vals, int *type, + int *length); +int iio_gts_all_avail_scales(struct iio_gts *gts, const int **vals, int *type, + int *length); +int iio_gts_avail_scales_for_time(struct iio_gts *gts, int time, + const int **vals, int *type, int *length); + +#endif
Some light sensors can adjust both the HW-gain and integration time. There are cases where adjusting the integration time has similar impact to the scale of the reported values as gain setting has. IIO users do typically expect to handle scale by a single writable 'scale' entry. Driver should then adjust the gain/time accordingly. It however is difficult for a driver to know whether it should change gain or integration time to meet the requested scale. Usually it is preferred to have longer integration time which usually improves accuracy, but there may be use-cases where long measurement times can be an issue. Thus it can be preferable to allow also changing the integration time - but mitigate the scale impact by also changing the gain underneath. Eg, if integration time change doubles the measured values, the driver can reduce the HW-gain to half. The theory of the computations of gain-time-scale is simple. However, some people (undersigned) got that implemented wrong for more than once. Add some gain-time-scale helpers in order to not dublicate errors in all drivers needing these computations. Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com> --- Currently it is only BU27034 using these in this series. I am however working with drivers for RGB sensors BU27008 and BU27010 which have similar [gain - integration time - scale] - relation. I hope sending those follows soon after the BU27034 is done. Changes since RFCv1: - fix include guardian - Improve kernel doc for iio_init_iio_gts. - Add iio_gts_scale_to_total_gain - Add iio_gts_total_gain_to_scale - Fix review comments from Jonathan - add documentation to few functions - replace 0xffffffffffffffffLLU by U64_MAX - some styling fixes - drop unnecessary NULL checks - order function arguments by in / out purpose - drop GAIN_SCALE_ITIME_MS() - Add helpers for available scales and times - Rename to iio-gts-helpers --- drivers/iio/light/Kconfig | 3 + drivers/iio/light/Makefile | 1 + drivers/iio/light/iio-gts-helper.c | 1152 ++++++++++++++++++++++++++++ drivers/iio/light/iio-gts-helper.h | 130 ++++ 4 files changed, 1286 insertions(+) create mode 100644 drivers/iio/light/iio-gts-helper.c create mode 100644 drivers/iio/light/iio-gts-helper.h