Re: [PATCH v3 2/6] iio: light: Add gain-time-scale helpers

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 3/6/23 14:52, Andy Shevchenko wrote:
On Mon, Mar 06, 2023 at 11:17:15AM +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.

...

+/*

If it's deliberately not a kernel doc, why to bother to have it looking as one?
It's really a provocative to some people who will come with a patches to "fix"
this...

I just liked the kernel-doc format. It's a standard way of explaining the parameters and returned value. Function however is intended to be internal and thus I don't see a need to make this "official kernel doc".


+ * 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 created
+ *		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 scale. -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;

Can you wait for the comments to appear a bit longer, please?
I have answered to your query in the previous discussion.


Yep. Sorry for that. I just wanted to get the v3 out before I left the computer for a week to allow potential reviewers to check the quite a bit reworked series.

+	}
+
+	while (max > scale * (u64) tmp)

No space for castings?

Thanks,


+		tmp++;
+
+	return tmp;
+}

...

+	/*
+	 * Expect scale to be (mostly) NANO or MICRO. Divide divider instead of
+	 * multiplication followed by division to avoid overflow

Missing period.

+	 */
+	if (scaler > NANO || !scaler)
+		return -EINVAL;

Shouldn't be OVERFLOW for the first one?

-EOVERFLOW? I guess it could be. Thanks.

...

+	*lin_scale = (u64) scale_whole * (u64)scaler +

No space for casting?

Yes. Thanks

...

+	ret = iio_gts_linearize(max_scale_int, max_scale_nano, NANO,
+				   &gts->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;

Just wondering why we can't simply

	memset(0)

beforehand and drop all these 0 assignments?


I guess we can :)

...

+		/*
+		 * Sort the tables for nice output and for easier finding of
+		 * unique values

Missing period. Please, check the style of multi-line comments. I believe it's
even mentioned in the documentation.

+		 */

...

+		sort(gains[i], gts->num_hwgain, sizeof(int), iio_gts_gain_cmp,
+		     NULL);

One line reads better?

I try mostly to keep the good old 80 chars as I often have 3 terminal windows fitted on my laptop screen. It works best with the short lines.


...

+	if (ret && gts->avail_all_scales_table)

In one case you commented that free(NULL) is okay, in the other, you add
a duplicative check. Why?

Sorry but what do you mean by dublicative check?

Usually I avoid the kfree(NULL). That's why I commented on it in that another case where it was not explicitly disallowed. I'll change that for v4 to avoid kfree(NULL) as you suggested.


+		kfree(gts->avail_all_scales_table);

...

+	per_time_gains = kcalloc(gts->num_itime, sizeof(int *), GFP_KERNEL);

sizeof(type) is error prone in comparison to sizeof(*var).

Yes and no. In majority of cases where we see sizeof(*var) - the *var is no longer a pointer as having pointers to pointers is not _that_ common. When we see sizeof(type *) - we instantly know it is a size of a pointer and not a size of some other type.

So yes, while having sizeof(*var) makes us tolerant to errors caused by variable type changes - it makes us prone to human reader errors. Also, if someone changes type of *var from pointer to some other type - then he/she is likely to in any case need to revise the array alloactions too.

While I in general agree with you that the sizeof(variable) is better than sizeof(type) - I see that in cases like this the sizeof(type *) is clearer.


+	if (!per_time_gains)
+		return ret;
+
+	per_time_scales = kcalloc(gts->num_itime, sizeof(int *), GFP_KERNEL);

Ditto.

+	if (!per_time_scales)
+		goto free_gains;

...

+err_free_out:
+	while (i) {
+		/*
+		 * It does not matter if i'th alloc was not succesfull as
+		 * kfree(NULL) is safe.
+		 */

Instead, can be just a free of the known allocated i:th member first followed
by traditional pattern. In that case comment will become redundant.


I replied to this in your comments regarding the v2. Sorry for splitting the discussion by sending v3 so soon.


+		kfree(per_time_scales[i]);
+		kfree(per_time_gains[i]);
+
+		i--;
+	}

...

+	for (i = gts->num_itime - 1; i >= 0; i--) {

	while (i--) {

makes it easier to parse.

This is also something I replied for v2. I think we have a fundamental disagreement on this one :/


+/**
+ * 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

Missing a return section. Have you run kernel doc to validate this?
No. I think I have never run the kernel doc - probably time to do so :) Thanks.

Missing period.

Seems these problems occur in many function descriptions.

+ */

...

+	/*
+	 * Using this function prior building the tables is a driver-error
+	 * which should be fixed when the driver is tested for a first time

Missing period.

+	 */
+	if (WARN_ON(!gts->num_avail_all_scales))

Does this justify panic? Note, that any WARN() can become an Oops followed by
panic and reboot.

No. My initial thinking was that this could only happen if a driver which do not build the tables tries to use the helpers. That, in my books, would have been 100% reproducible driver error, happening when ever the available scales were read.

I did overlook the case where freeing of tables is done in wrong order. That type of bug could well escape the initial testing and no - it should not bring down the machine. I'll drop the WARN. Thanks.


+		return -EINVAL;

...

+	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);

I was staring at this and have got no clue why it's not a dead code.

Nor can I. It seems obvious to me that the one who wrote this had no idea what he was doing XD

Well, I must have had some initial idea of using the minimum value to something - but I can't remember what would've been the use. Maybe I was initially thinking that I'll return the smallest value in-range if the gain given as a parameter was smaller than any of the supported ones.

Thank you for reading this carefully and pointing it out! Well spotted!


+		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;
+				}
+			}

			int tmp = gain - gts->hwgain_table[i].gain;

			if (!diff || tmp < diff) {
				diff = tmp;
				best = i;
			}

?

Or did you miss using 'min'? (And I'm wondering how variable name and min()
macro are not conflicting with each other.

+		} 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;

...

+	ret = iio_gts_get_scale_linear(gts, old_gain, itime_old->time_us,
+				       &scale);

Still can be one line.

+	if (ret)
+		return ret;
+
+	ret = gain_get_scale_fraction(gts->max_scale, scale, itime_new->mul,
+				      new_gain);

Ditto.

I still prefer the 80-chars when it does not mean some terribly awkward split - or really many rows of arguments.


+	if (ret)
+		return -EINVAL;

...

+++ b/drivers/iio/light/iio-gts-helper.h

Is it _only_ for a Light type of sensors?

This is a very good question. I was asking this from myself but as I don't know better I just decided to put it under the light where I'll have the use-cases. We can move it to drivers/iio if there will be users outside the light sensors.


...

+#ifndef __IIO_GTS_HELPER__
+#define __IIO_GTS_HELPER__

If yes, perhaps adding LIGHT here?

I'd like to keep this matching the file name, especially when I don't know for sure this can't be used elsewhere.

Yours,
	-- Matti

--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~




[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux