Support ROHM BU27034 ALS sensor

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.

+/*

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

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

+EXPORT_SYMBOL_GPL(iio_init_iio_gts);

drivers/iio/light/iio-gts-helper.c new file +3 -0

+			    GFP_KERNEL);
+	if (!all_gains)
+		return -ENOMEM;

drivers/iio/light/iio-gts-helper.c new file +22 -0

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

drivers/iio/light/iio-gts-helper.c new file +15 -0

+		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(&times[j + 1], &times[j], (idx - j) * sizeof(int));
+				times[j] = new;
+				idx++;
+			}
+		}
+	}

drivers/iio/light/iio-gts-helper.c new file +5 -0

+		kfree(gts->avail_time_tables);
+		gts->avail_time_tables = NULL;
+		gts->num_avail_time_tables = 0;
+	}
+}

drivers/iio/light/iio-gts-helper.c new file +10 -0

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

(quoted diff) +2 -0

+		return -EINVAL;
+

(quoted diff) +2 -0

+	if (ret)
+		return -EINVAL;

Quoted 19 lines · Matti Vaittinen

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.

Quoted 2 lines

Add some gain-time-scale helpers in order to not dublicate errors in all drivers needing these computations.

...

Quoted 1 line

+/*

Is it intentionally _not_ a kernel doc?

Quoted 3 lines

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

Quoted 1 line

+ * @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).

drivers/iio/light/iio-gts-helper.c line 45

Quoted 12 lines

drivers/iio/light/iio-gts-helper.c new file +11 -0

+{
+	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.

drivers/iio/light/iio-gts-helper.c line 48

Quoted 3 lines

drivers/iio/light/iio-gts-helper.c new file +3 -0

+			tmp++;
+
+		return tmp + 1;

Wondering why you can't simplify this to

		max -= scale;
		tmp++;

		max -= scale;
		tmp++;

Quoted 6 lines

drivers/iio/light/iio-gts-helper.c new file +6 -0

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

drivers/iio/light/iio-gts-helper.c line 63

Quoted 10 lines

drivers/iio/light/iio-gts-helper.c new file +9 -0

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

Quoted 8 lines

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

Quoted 1 line

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

...

drivers/iio/light/iio-gts-helper.c line 309

Quoted 1 line

drivers/iio/light/iio-gts-helper.c new file +1 -0

+	all_gains = kcalloc(gts->num_itime * gts->num_hwgain, sizeof(int),

Something from overflow.h is very suitable here.

Quoted 3 lines

drivers/iio/light/iio-gts-helper.c new file +3 -0

+			    GFP_KERNEL);
+	if (!all_gains)
+		return -ENOMEM;

...

drivers/iio/light/iio-gts-helper.c line 321

Quoted 1 line

drivers/iio/light/iio-gts-helper.c new file +1 -0

+	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?

...

drivers/iio/light/iio-gts-helper.c line 324

Quoted 1 line

drivers/iio/light/iio-gts-helper.c new file +1 -0

+	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 may re-use that i (maybe renamed to something better in the

above as well).

Note, you missed {} for better reading.

Quoted 23 lines

drivers/iio/light/iio-gts-helper.c new file +22 -0

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

...

drivers/iio/light/iio-gts-helper.c line 355

Quoted 6 lines

drivers/iio/light/iio-gts-helper.c new file +6 -0

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

drivers/iio/light/iio-gts-helper.c line 358

Quoted 3 lines

drivers/iio/light/iio-gts-helper.c new file +3 -0

+			ret = iio_gts_total_gain_to_scale(gts, all_gains[i],
+					&gts->avail_all_scales_table[i * 2],
+					&gts->avail_all_scales_table[i * 2 + 1]);

...here as

		if (ret)
			break;

drivers/iio/light/iio-gts-helper.c line 364

Quoted 6 lines

drivers/iio/light/iio-gts-helper.c new file +5 -0

+	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?

...

drivers/iio/light/iio-gts-helper.c line 423

Quoted 1 line

drivers/iio/light/iio-gts-helper.c new file +1 -0

+	while (i) {

Instead of doing standard

	while (i--) {

drivers/iio/light/iio-gts-helper.c line 427

Quoted 4 lines

drivers/iio/light/iio-gts-helper.c new file +4 -0

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

You add this comment, ...

drivers/iio/light/iio-gts-helper.c line 429

Quoted 2 lines

drivers/iio/light/iio-gts-helper.c new file +2 -0

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

...an additional loop, ...

drivers/iio/light/iio-gts-helper.c line 431

Quoted 3 lines

drivers/iio/light/iio-gts-helper.c new file +2 -0

+
+		i--;

...and a line of code.

Review

Quoted 1 line

(quoted diff) +1 -0

+	}

Why?

drivers/iio/light/iio-gts-helper.c line 497

Quoted 2 lines

drivers/iio/light/iio-gts-helper.c new file +1 -0

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

	i = gts->num_itime;

	while (i--) {

Quoted 16 lines

drivers/iio/light/iio-gts-helper.c new file +15 -0

+		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(&times[j + 1], &times[j], (idx - j) * sizeof(int));
+				times[j] = new;
+				idx++;
+			}
+		}
+	}

...

drivers/iio/light/iio-gts-helper.c line 537

Quoted 3 lines

drivers/iio/light/iio-gts-helper.c new file +3 -0

+void iio_gts_purge_avail_time_table(struct iio_gts *gts)
+{
+	if (gts->num_avail_time_tables) {

	if (!...)
		return;

Quoted 5 lines

drivers/iio/light/iio-gts-helper.c new file +5 -0

+		kfree(gts->avail_time_tables);
+		gts->avail_time_tables = NULL;
+		gts->num_avail_time_tables = 0;
+	}
+}

...

Review

Quoted 1 line

(quoted diff) +1 -0

+			if (!diff) {

Why not positive conditional?

			if (diff) {
				...
			} else {
				...
			}

Quoted 10 lines

drivers/iio/light/iio-gts-helper.c new file +10 -0

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

...

drivers/iio/light/iio-gts-helper.c line 949

Quoted 3 lines

drivers/iio/light/iio-gts-helper.c new file +2 -0

+	ret = gain_get_scale_fraction(gts->max_scale, scale_linear, mul, gain);
 
+

Redundant blank line.

drivers/iio/light/iio-gts-helper.c line 950

Quoted 2 lines

drivers/iio/light/iio-gts-helper.c new file +1 -0

+	if (ret || !iio_gts_valid_gain(gts, *gain))

Why error code is shadowed?

Quoted 3 lines

(quoted diff) +2 -0

+		return -EINVAL;
+

...

drivers/iio/light/iio-gts-helper.c line 1134

Quoted 2 lines

drivers/iio/light/iio-gts-helper.c new file +2 -0

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

Single line if fine.

drivers/iio/light/iio-gts-helper.c line 1139

Quoted 5 lines

drivers/iio/light/iio-gts-helper.c new file +5 -0

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

Ditto.

Quoted 2 lines

(quoted diff) +2 -0

+	if (ret)
+		return -EINVAL;

...

drivers/iio/light/iio-gts-helper.h line 8

Quoted 2 lines

drivers/iio/light/iio-gts-helper.h new file +2 -0

+#ifndef __GAIN_TIME_SCALE_HELPER__
+#define __GAIN_TIME_SCALE_HELPER__

__IIO_... ?

Quoted 1 line · Andy Shevchenko

On Thu, Mar 02, 2023 at 12:57:54PM +0200, Matti Vaittinen wrote:

Quoted 21 lines

drivers/iio/light/iio-gts-helper.c line 45

Quoted 12 lines

drivers/iio/light/iio-gts-helper.c new file +11 -0

+{
+	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.

drivers/iio/light/iio-gts-helper.c line 48

Quoted 3 lines

drivers/iio/light/iio-gts-helper.c new file +3 -0

+			tmp++;
+
+		return tmp + 1;

Wondering why you can't simplify this to

Quoted 3 lines

		max -= scale;
		tmp++;

Sorry, but I don't follow. Can you please show me the complete suggestion? Exactly what should be replaced by the:

Quoted 10 lines

		max -= scale;
		tmp++;

Quoted 7 lines

drivers/iio/light/iio-gts-helper.c new file +7 -0

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

Quoted 4 lines

Quoted 1 line

+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?

Quoted 4 lines

drivers/iio/light/iio-gts-helper.c line 324

Quoted 1 line

drivers/iio/light/iio-gts-helper.c new file +1 -0

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

Yeah, if you put this into temporary, like

Quoted 6 lines

	i = gts->num_itime - 1;

this becomes

	while (i--) {

I prefer for(). It looks clearer to me...

Quoted 1 line

Note, you may re-use that i (maybe renamed to something better in the

memcpy()

Quoted 1 line

above as well).

...but this makes sense so Ok.

Quoted 1 line

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.

Quoted 10 lines

drivers/iio/light/iio-gts-helper.c line 355

Quoted 1 line

drivers/iio/light/iio-gts-helper.c new file +1 -0

+		for (i = 0; !ret && i < gts->num_avail_all_scales; i++)

Much easier to read if you move this...

drivers/iio/light/iio-gts-helper.c line 358

Quoted 3 lines

drivers/iio/light/iio-gts-helper.c new file +3 -0

+			ret = iio_gts_total_gain_to_scale(gts, all_gains[i],
+					&gts->avail_all_scales_table[i * 2],
+					&gts->avail_all_scales_table[i * 2 + 1]);

...here as

Quoted 2 lines

		if (ret)
			break;

I think the !ret in loop condition is obvious. Adding break and brackets would not improve this.

Quoted 7 lines

drivers/iio/light/iio-gts-helper.c line 364

Quoted 5 lines

drivers/iio/light/iio-gts-helper.c new file +5 -0

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

Quoted 13 lines

drivers/iio/light/iio-gts-helper.c line 423

Quoted 1 line

drivers/iio/light/iio-gts-helper.c new file +1 -0

+	while (i) {

Instead of doing standard

	while (i--) {

drivers/iio/light/iio-gts-helper.c line 427

Quoted 4 lines

drivers/iio/light/iio-gts-helper.c new file +4 -0

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

You add this comment, ...

Quoted 4 lines

drivers/iio/light/iio-gts-helper.c line 429

Quoted 2 lines

drivers/iio/light/iio-gts-helper.c new file +2 -0

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

Quoted 6 lines

drivers/iio/light/iio-gts-helper.c line 431

Quoted 3 lines

drivers/iio/light/iio-gts-helper.c new file +2 -0

+
+		i--;

...and a line of code.

Quoted 3 lines

Review

Quoted 1 line

(quoted diff) +1 -0

+	}

Why?

Because, as the comment says, it does not matter if allocation was succesfull. kfree(NULL) is safe.

Quoted 5 lines

drivers/iio/light/iio-gts-helper.c line 497

Quoted 1 line

drivers/iio/light/iio-gts-helper.c new file +1 -0

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

Quoted 1 line

Quoted 1 line

(quoted diff) +1 -0

+	}

Quoted 6 lines

drivers/iio/light/iio-gts-helper.c line 537

Quoted 3 lines

drivers/iio/light/iio-gts-helper.c new file +3 -0

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

Quoted 5 lines

Quoted 5 lines

drivers/iio/light/iio-gts-helper.c new file +5 -0

+		kfree(gts->avail_time_tables);
+		gts->avail_time_tables = NULL;
+		gts->num_avail_time_tables = 0;
+	}
+}

Quoted 3 lines

Review

Quoted 1 line

(quoted diff) +1 -0

+			if (!diff) {

Why not positive conditional?

Because !diff is a special condition and we check explicitly for it.

Quoted 11 lines

			if (diff) {
				...
			} else {
				...
			}

Quoted 3 lines

drivers/iio/light/iio-gts-helper.c new file +2 -0

+			} else {
 
+			}

Quoted 2 lines · Matti

Quoted 1 line · Andy Shevchenko

On Thu, Mar 02, 2023 at 12:57:54PM +0200, Matti Vaittinen wrote:

...

Quoted 25 lines

Quoted 24 lines

drivers/iio/light/iio-gts-helper.c line 45

Quoted 12 lines

drivers/iio/light/iio-gts-helper.c new file +11 -0

+{
+	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.

drivers/iio/light/iio-gts-helper.c line 48

Quoted 3 lines

drivers/iio/light/iio-gts-helper.c new file +3 -0

+			tmp++;
+
+		return tmp + 1;

Wondering why you can't simplify this to

		max -= scale;
		tmp++;

Quoted 2 lines

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.

Quoted 2 lines

Quoted 2 lines

		max -= scale;
		tmp++;

...

Quoted 8 lines

Quoted 8 lines

drivers/iio/light/iio-gts-helper.c line 324

Quoted 1 line

drivers/iio/light/iio-gts-helper.c new file +1 -0

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

Yeah, if you put this into temporary, like

	i = gts->num_itime - 1;

this becomes

Quoted 4 lines

Quoted 2 lines

	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?

Quoted 5 lines

Quoted 1 line

Note, you may re-use that i (maybe renamed to something better in the

memcpy()

Quoted 1 line

above as well).

...but this makes sense so Ok.

...

Quoted 10 lines

Quoted 10 lines

drivers/iio/light/iio-gts-helper.c line 355

Quoted 1 line

drivers/iio/light/iio-gts-helper.c new file +1 -0

+		for (i = 0; !ret && i < gts->num_avail_all_scales; i++)

Much easier to read if you move this...

drivers/iio/light/iio-gts-helper.c line 358

Quoted 3 lines

drivers/iio/light/iio-gts-helper.c new file +3 -0

+			ret = iio_gts_total_gain_to_scale(gts, all_gains[i],
+					&gts->avail_all_scales_table[i * 2],
+					&gts->avail_all_scales_table[i * 2 + 1]);

...here as

Quoted 5 lines

Quoted 2 lines

		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.

...

Quoted 8 lines

Quoted 7 lines

drivers/iio/light/iio-gts-helper.c line 364

Quoted 5 lines

drivers/iio/light/iio-gts-helper.c new file +5 -0

+	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?

Quoted 3 lines

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?

...

Quoted 28 lines

Quoted 13 lines

drivers/iio/light/iio-gts-helper.c line 423

Quoted 1 line

drivers/iio/light/iio-gts-helper.c new file +1 -0

+	while (i) {

Instead of doing standard

	while (i--) {

drivers/iio/light/iio-gts-helper.c line 427

Quoted 4 lines

drivers/iio/light/iio-gts-helper.c new file +4 -0

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

You add this comment, ...

Quoted 4 lines

drivers/iio/light/iio-gts-helper.c line 429

Quoted 2 lines

drivers/iio/light/iio-gts-helper.c new file +2 -0

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

Quoted 6 lines

drivers/iio/light/iio-gts-helper.c line 431

Quoted 3 lines

drivers/iio/light/iio-gts-helper.c new file +2 -0

+
+		i--;

...and a line of code.

Quoted 6 lines

Quoted 3 lines

Review

Quoted 1 line

(quoted diff) +1 -0

+	}

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.

...

Quoted 6 lines

Quoted 5 lines

drivers/iio/light/iio-gts-helper.c line 497

Quoted 1 line

drivers/iio/light/iio-gts-helper.c new file +1 -0

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

	i = gts->num_itime;

	while (i--) {

Quoted 2 lines

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

Quoted 1 line

Quoted 1 line

Quoted 1 line

(quoted diff) +1 -0

+	}

...

Quoted 7 lines

Quoted 6 lines

drivers/iio/light/iio-gts-helper.c line 537

Quoted 3 lines

drivers/iio/light/iio-gts-helper.c new file +3 -0

+void iio_gts_purge_avail_time_table(struct iio_gts *gts)
+{
+	if (gts->num_avail_time_tables) {

	if (!...)
		return;

Quoted 3 lines

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

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

drivers/iio/light/iio-gts-helper.c new file +5 -0

+		kfree(gts->avail_time_tables);
+		gts->avail_time_tables = NULL;
+		gts->num_avail_time_tables = 0;
+	}
+}

...

Quoted 5 lines

Quoted 3 lines

Review

Quoted 1 line

(quoted diff) +1 -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.)

Quoted 3 lines · Andy Shevchenko

Quoted 2 lines · Matti

Quoted 1 line · Andy Shevchenko

On Thu, Mar 02, 2023 at 12:57:54PM +0200, Matti Vaittinen wrote:

Quoted 16 lines

Quoted 15 lines

Quoted 4 lines

drivers/iio/light/iio-gts-helper.c line 355

Quoted 1 line

drivers/iio/light/iio-gts-helper.c new file +1 -0

+		for (i = 0; !ret && i < gts->num_avail_all_scales; i++)

Much easier to read if you move this...

Quoted 8 lines

drivers/iio/light/iio-gts-helper.c line 358

Quoted 3 lines

drivers/iio/light/iio-gts-helper.c new file +3 -0

+			ret = iio_gts_total_gain_to_scale(gts, all_gains[i],
+					&gts->avail_all_scales_table[i * 2],
+					&gts->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.

Quoted 1 line

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

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

Review

Quoted 1 line

(quoted diff) +1 -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.

Quoted 2 lines

(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 '!'.)

Quoted 4 lines · Matti Vaittinen

Quoted 3 lines · Andy Shevchenko

Quoted 2 lines · Matti

Quoted 1 line · Andy Shevchenko

On Thu, Mar 02, 2023 at 12:57:54PM +0200, Matti Vaittinen wrote:

...

Quoted 18 lines

Quoted 17 lines

Quoted 10 lines

Quoted 10 lines

drivers/iio/light/iio-gts-helper.c line 355

Quoted 1 line

drivers/iio/light/iio-gts-helper.c new file +1 -0

+		for (i = 0; !ret && i < gts->num_avail_all_scales; i++)

Much easier to read if you move this...

drivers/iio/light/iio-gts-helper.c line 358

Quoted 3 lines

drivers/iio/light/iio-gts-helper.c new file +3 -0

+			ret = iio_gts_total_gain_to_scale(gts, all_gains[i],
+					&gts->avail_all_scales_table[i * 2],
+					&gts->avail_all_scales_table[i * 2 + 1]);

...here as

Quoted 5 lines

Quoted 2 lines

		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.

Quoted 4 lines

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.

...

Quoted 21 lines

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

Review

Quoted 1 line

(quoted diff) +1 -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.

Quoted 2 lines

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

Quoted 5 lines

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

Quoted 5 lines · Andy Shevchenko

Quoted 4 lines · Matti Vaittinen

Quoted 3 lines · Andy Shevchenko

Quoted 2 lines · Matti

Quoted 1 line · Andy Shevchenko

On Thu, Mar 02, 2023 at 12:57:54PM +0200, Matti Vaittinen wrote:

Quoted 23 lines

Quoted 22 lines

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Review

Quoted 1 line

(quoted diff) +1 -0

+		for (i = 0; !ret && i < gts->num_avail_all_scales; i++)

Much easier to read if you move this...

Review

Quoted 3 lines

(quoted diff) +3 -0

+			ret = iio_gts_total_gain_to_scale(gts, all_gains[i],
+					&gts->avail_all_scales_table[i * 2],
+					&gts->avail_all_scales_table[i * 2 + 1]);

...here as

Quoted 7 lines

Quoted 5 lines

Quoted 2 lines

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

Quoted 2 lines

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.

Quoted 26 lines

Quoted 25 lines

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

Review

Quoted 1 line

(quoted diff) +1 -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.

Quoted 2 lines

(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 '!'.)

Quoted 2 lines

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.

Quoted 2 lines

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.

Quoted 6 lines · Matti Vaittinen

Quoted 5 lines · Andy Shevchenko

Quoted 4 lines · Matti Vaittinen

Quoted 3 lines · Andy Shevchenko

Quoted 2 lines · Matti

Quoted 1 line · Andy Shevchenko

On Thu, Mar 02, 2023 at 12:57:54PM +0200, Matti Vaittinen wrote:

Quoted 26 lines

Quoted 26 lines

Quoted 25 lines

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

Review

Quoted 1 line

(quoted diff) +1 -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.

Quoted 2 lines

(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 '!'.)

Quoted 4 lines

Quoted 2 lines

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?

Quoted 2 lines

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.

Quoted 6 lines

Quoted 2 lines

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?

Quoted 10 lines

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.

Quoted 6 lines · Matti Vaittinen

Quoted 5 lines · Andy Shevchenko

Quoted 4 lines · Matti Vaittinen

Quoted 3 lines · Andy Shevchenko

Quoted 2 lines · Matti

Quoted 1 line · Andy Shevchenko

On Thu, Mar 02, 2023 at 12:57:54PM +0200, Matti Vaittinen wrote:

...

Quoted 26 lines

Quoted 25 lines

Quoted 16 lines

Quoted 16 lines

Quoted 15 lines

Quoted 4 lines

Review

Quoted 1 line

(quoted diff) +1 -0

+		for (i = 0; !ret && i < gts->num_avail_all_scales; i++)

Much easier to read if you move this...

Quoted 8 lines

Review

Quoted 3 lines

(quoted diff) +3 -0

+			ret = iio_gts_total_gain_to_scale(gts, all_gains[i],
+					&gts->avail_all_scales_table[i * 2],
+					&gts->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.

Quoted 6 lines

Quoted 1 line

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.

Quoted 3 lines

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.

...

Quoted 26 lines

Quoted 26 lines

Quoted 25 lines

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

Review

Quoted 1 line

(quoted diff) +1 -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.

Quoted 2 lines

(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 '!'.)

Quoted 4 lines

Quoted 2 lines

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?

Quoted 2 lines

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.

Quoted 6 lines

Quoted 2 lines

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?

Quoted 10 lines

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.

Quoted 31 lines · Andy Shevchenko

Quoted 4 lines · Matti Vaittinen

Quoted 3 lines · Andy Shevchenko

Quoted 2 lines · Matti

Quoted 1 line · Andy Shevchenko

On Thu, Mar 02, 2023 at 12:57:54PM +0200, Matti Vaittinen wrote:

...

Quoted 22 lines

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Review

Quoted 1 line

(quoted diff) +1 -0

+		for (i = 0; !ret && i < gts->num_avail_all_scales; i++)  

Much easier to read if you move this...

Review

Quoted 3 lines

(quoted diff) +3 -0

+			ret = iio_gts_total_gain_to_scale(gts, all_gains[i],
+					&gts->avail_all_scales_table[i * 2],
+					&gts->avail_all_scales_table[i * 2 + 1]);  

...here as

Quoted 7 lines

Quoted 5 lines

Quoted 2 lines

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

Quoted 2 lines

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.

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 <[email protected]>

Quoted 19 lines · On Thu, 2 Mar 2023 12:57:54 +0200 Matti Vaittinen <[email protected]>…

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.

Quoted 4 lines

Add some gain-time-scale helpers in order to not dublicate errors in all drivers needing these computations.

Signed-off-by Matti Vaittinen <[email protected]>

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.

Quoted 1 line · Jonathan Cameron

On Thu, 2 Mar 2023 12:57:54 +0200

Quoted 5 lines

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.

ROHM BU27034 is an ambient light sesnor with 3 channels and 3 photo diodes capable of detecting a very wide range of illuminance. Typical application is adjusting LCD and backlight power of TVs and mobile phones.

Add initial support for the ROHM BU27034 ambient light sensor.

NOTE:
- Driver exposes 4 channels. One IIO_LIGHT channel providing the calculated lux values based on measured data from diodes #0 and #1. Additionally 3 IIO_INTENSITY channels are emitting the raw register data from all diodes for more intense user-space computations.
- Sensor has adjustible GAIN values ranging from 1x to 4096x.
- Sensor has adjustible measurement times 5, 55, 100, 200 and 400 mS. Driver does not support 5 mS which has special limitations.
- Driver exposes standard 'scale' adjustment which is implemented by:
1) Trying to adjust only the GAIN
2) If GAIN adjustment only can't provide requested scale, adjusting both the time and the gain is

attempted.
- Driver exposes writable INT_TIME property which can be used for adjusting the measurement time. Time adjustment will also cause the driver to adjust the GAIN so that the overall scale is not changed.
- Runtime PM is not implemented.
- Driver starts the measurement on the background when it is probed. This improves the respnse time to read-requests

ROHM BU27034 is an ambient light sesnor with 3 channels and 3 photo diodes capable of detecting a very wide range of illuminance. Typical application is adjusting LCD and backlight power of TVs and mobile phones.

Add initial support for the ROHM BU27034 ambient light sensor.

NOTE:
- Driver exposes 4 channels. One IIO_LIGHT channel providing the calculated lux values based on measured data from diodes #0 and #1. Additionally 3 IIO_INTENSITY channels are emitting the raw register data from all diodes for more intense user-space computations.
- Sensor has adjustible GAIN values ranging from 1x to 4096x.
- Sensor has adjustible measurement times 5, 55, 100, 200 and 400 mS. Driver does not support 5 mS which has special limitations.
- Driver exposes standard 'scale' adjustment which is implemented by:
1) Trying to adjust only the GAIN
2) If GAIN adjustment only can't provide requested scale, adjusting both the time and the gain is attempted.
- Driver exposes writable INT_TIME property which can be used for adjusting the measurement time. Time adjustment will also cause the driver to adjust the GAIN so that the overall scale is not changed.
- Runtime PM is not implemented.
- Driver starts the measurement on the background when it is probed. This improves the respnse time to read-requests compared to starting the read only when data is requested. When the most accurate 400 mS measurement time is used, data reads would last quite long if measurement was started only on demand. This, however, is not appealing for users who would prefere power saving over measurement response time.

obj-$(CONFIG_OPT3001) += opt3001.o
obj-$(CONFIG_PA12203001) += pa12203001.o

+obj-$(CONFIG_ROHM_BU27034) += rohm-bu27034.o

obj-$(CONFIG_RPR0521) += rpr0521.o
obj-$(CONFIG_SENSORS_TSL2563) += tsl2563.o
obj-$(CONFIG_SI1133) += si1133.o

+#define BU27034_REG_DATA2_HI 0x55

drivers/iio/light/rohm-bu27034.c new file +11 -0

+/*
+ * Available scales with gain 1x - 4096x, timings 55, 100, 200, 400 mS
+ * Time impacts to gain: 1x, 2x, 4x, 8x.
+ *
+ * => Max total gain is HWGAIN * gain by integration time (8 * 4096) = 32768
+ *
+ * Using NANO precision for scale we must use scale 64x corresponding gain 1x
+ * to avoid precision loss. (32x would result scale 976 562.5(nanos).
+ */
+#define BU27034_SCALE_1X	64
+

(quoted diff) +1 -0

+	};

(quoted diff) +1 -0

+	};

(quoted diff) +2 -0

+			return -EINVAL;
+	}

drivers/iio/light/rohm-bu27034.c new file +7 -0

+	if (helper64 < 0xFFFFFFFFFFFFFLLU) {
+		helper64 *= gain0;
+		do_div(helper64, helper);
+	} else {
+		do_div(helper64, helper);
+		helper64 *= gain0;
+	}

drivers/iio/light/rohm-bu27034.c new file +4 -0

+retry:
+	/* Get new value from sensor if data is ready */
+	if (bu27034_has_valid_sample(data)) {
+		ret = regmap_bulk_read(data->regmap, BU27034_REG_DATA0_LO,

(quoted diff) +1 -0

+	}

drivers/iio/light/rohm-bu27034.c new file +4 -0

+	if (ret < 0)
+		return ret;
+
+	msleep(ret / 1000);

(quoted diff) +2 -0

+		return ret;
+	}

drivers/iio/light/rohm-bu27034.c new file +3 -0

+	if (ret < 0)
+		return dev_err_probe(dev, ret,
+				     "Unable to register iio device\n");

Quoted 26 lines · Matti Vaittinen

ROHM BU27034 is an ambient light sesnor with 3 channels and 3 photo diodes capable of detecting a very wide range of illuminance. Typical application is adjusting LCD and backlight power of TVs and mobile phones.

Add initial support for the ROHM BU27034 ambient light sensor.

NOTE:
- Driver exposes 4 channels. One IIO_LIGHT channel providing the calculated lux values based on measured data from diodes #0 and #1. Additionally 3 IIO_INTENSITY channels are emitting the raw register data from all diodes for more intense user-space computations.
- Sensor has adjustible GAIN values ranging from 1x to 4096x.
- Sensor has adjustible measurement times 5, 55, 100, 200 and 400 mS. Driver does not support 5 mS which has special limitations.
- Driver exposes standard 'scale' adjustment which is implemented by:
1) Trying to adjust only the GAIN
2) If GAIN adjustment only can't provide requested scale, adjusting both the time and the gain is attempted.
- Driver exposes writable INT_TIME property which can be used for adjusting the measurement time. Time adjustment will also cause the driver to adjust the GAIN so that the overall scale is not changed.

Quoted 8 lines

- Runtime PM is not implemented.
- Driver starts the measurement on the background when it is probed. This improves the respnse time to read-requests compared to starting the read only when data is requested. When the most accurate 400 mS measurement time is used, data reads would last quite long if measurement was started only on demand. This, however, is not appealing for users who would prefere power saving over measurement response time.

...

drivers/iio/light/Kconfig config JSA1212 · line 297

Quoted 4 lines

drivers/iio/light/Kconfig +3 -0

+config ROHM_BU27034
+	tristate "ROHM BU27034 ambient light sensor"
 
+	depends on I2C

How? I do not see a such.

drivers/iio/light/Kconfig config JSA1212 · line 305

Quoted 8 lines

drivers/iio/light/Kconfig +8 -0

+	select REGMAP_I2C
+	select IIO_GTS_HELPER
+	help
+	  Enable support for the ROHM BU27034 ambient light sensor. ROHM BU27034
+	  is an ambient light sesnor with 3 channels and 3 photo diodes capable
+	  of detecting a very wide range of illuminance.
+	  Typical application is adjusting LCD and backlight power of TVs and
+	  mobile phones.

Module name?

...

Quoted 4 lines

  obj-$(CONFIG_OPT3001)		+= opt3001.o
  obj-$(CONFIG_PA12203001)	+= pa12203001.o

+obj-$(CONFIG_ROHM_BU27034) += rohm-bu27034.o

If you see, most of the components are without vendor prefix, why rohm is special? Like you are expecting the very same filename for something else?

...

drivers/iio/light/rohm-bu27034.c line 20

Quoted 3 lines

drivers/iio/light/rohm-bu27034.c new file +3 -0

+#include <linux/iio/iio.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/kfifo_buf.h>

Sorted?

...

drivers/iio/light/rohm-bu27034.c line 44

Quoted 3 lines

drivers/iio/light/rohm-bu27034.c new file +3 -0

+#define BU27034_REG_DATA0_LO		0x50
+#define BU27034_REG_DATA1_LO		0x52
+#define BU27034_REG_DATA2_LO		0x54

I would drop _LO in all these

Quoted 1 line

+#define BU27034_REG_DATA2_HI 0x55

and rename somehow this to something like _END / _MAX (similar to the fields. Perhaps you would need _START / _MIN above.

...

Quoted 15 lines

drivers/iio/light/rohm-bu27034.c new file +15 -0

+/*
+ * Available scales with gain 1x - 4096x, timings 55, 100, 200, 400 mS
+ * Time impacts to gain: 1x, 2x, 4x, 8x.
+ *
+ * => Max total gain is HWGAIN * gain by integration time (8 * 4096) = 32768
+ *
+ * Using NANO precision for scale we must use scale 64x corresponding gain 1x
+ * to avoid precision loss. (32x would result scale 976 562.5(nanos).
+ */
+#define BU27034_SCALE_1X	64
+
+#define BU27034_GSEL_1X		0x00
+#define BU27034_GSEL_4X		0x08
+#define BU27034_GSEL_16X	0x0a
+#define BU27034_GSEL_32X	0x0b

drivers/iio/light/rohm-bu27034.c line 107

Quoted 6 lines

drivers/iio/light/rohm-bu27034.c new file +6 -0

+#define BU27034_GSEL_64X	0x0c
+#define BU27034_GSEL_256X	0x18
+#define BU27034_GSEL_512X	0x19
+#define BU27034_GSEL_1024X	0x1a
+#define BU27034_GSEL_2048X	0x1b
+#define BU27034_GSEL_4096X	0x1c

Shouldn't the values be in plain decimal?

Otherwise I would like to understand bit mapping inside these hex values.

...

drivers/iio/light/rohm-bu27034.c line 165

Quoted 1 line

drivers/iio/light/rohm-bu27034.c new file +1 -0

+	.indexed = 1							\

+ Comma at the end.

...

drivers/iio/light/rohm-bu27034.c line 389

Quoted 4 lines

drivers/iio/light/rohm-bu27034.c new file +4 -0

+	static const int reg[] = {
+		[BU27034_CHAN_DATA0] = BU27034_REG_MODE_CONTROL2,
+		[BU27034_CHAN_DATA1] = BU27034_REG_MODE_CONTROL3,
+		[BU27034_CHAN_DATA2] = BU27034_REG_MODE_CONTROL2

Ditto.

Quoted 2 lines

(quoted diff) +1 -0

+	};

...

drivers/iio/light/rohm-bu27034.c line 445

Quoted 3 lines

drivers/iio/light/rohm-bu27034.c new file +3 -0

+	struct bu27034_gain_check gains[3] = {
+		{ .chan = BU27034_CHAN_DATA0, },
+		{ .chan = BU27034_CHAN_DATA1, },

Inner commas are not needed.

drivers/iio/light/rohm-bu27034.c line 446

Quoted 1 line

drivers/iio/light/rohm-bu27034.c new file +1 -0

+		{ .chan = BU27034_CHAN_DATA2 }

But here the outer one is good to have.

Review

Quoted 1 line

(quoted diff) +1 -0

+	};

...

drivers/iio/light/rohm-bu27034.c line 547

Quoted 4 lines

drivers/iio/light/rohm-bu27034.c new file +4 -0

+	if (chan == BU27034_CHAN_ALS) {
+		if (val == 0 && val2 == 1000)
+			return 0;
+		else

Redundant 'else'

. And probably here is better to use standard pattern for "checking for error first".

Quoted 3 lines

(quoted diff) +2 -0

+			return -EINVAL;
+	}

...

Review

Quoted 1 line

(quoted diff) +1 -0

+		if (helper64 < 0xFFFFFFFFFFFFFLLU) {

Perhaps this needs a definition.

drivers/iio/light/rohm-bu27034.c line 825

Quoted 9 lines

drivers/iio/light/rohm-bu27034.c new file +7 -0

+			helper64 *= gain0;
+			do_div(helper64, ch0);
+		} else {
+			do_div(helper64, ch0);
+			helper64 *= gain0;
+		}
 
 
+	/* Same overflow check here */

Why not a helper function?

Quoted 8 lines

drivers/iio/light/rohm-bu27034.c new file +7 -0

+	if (helper64 < 0xFFFFFFFFFFFFFLLU) {
+		helper64 *= gain0;
+		do_div(helper64, helper);
+	} else {
+		do_div(helper64, helper);
+		helper64 *= gain0;
+	}

...

drivers/iio/light/rohm-bu27034.c line 924

Quoted 1 line

drivers/iio/light/rohm-bu27034.c new file +1 -0

+	return (val & BU27034_MASK_VALID);

Unneeded parentheses.

...

Quoted 4 lines

drivers/iio/light/rohm-bu27034.c new file +4 -0

+retry:
+	/* Get new value from sensor if data is ready */
+	if (bu27034_has_valid_sample(data)) {
+		ret = regmap_bulk_read(data->regmap, BU27034_REG_DATA0_LO,

drivers/iio/light/rohm-bu27034.c line 981

Quoted 10 lines

drivers/iio/light/rohm-bu27034.c new file +10 -0

+				       res, size);
+		if (ret)
+			return ret;
+
+		bu27034_invalidate_read_data(data);
+	} else {
+		/* No new data in sensor. Wait and retry */
+		msleep(25);
+
+		goto retry;

There is no way out. What might go wrong?

Quoted 2 lines

(quoted diff) +1 -0

+	}

...

Review

Quoted 1 line

(quoted diff) +1 -0

+	ret = bu27034_get_int_time(data);

_get_int_time_us() ? (Looking at the below code)

Quoted 4 lines

drivers/iio/light/rohm-bu27034.c new file +4 -0

+	if (ret < 0)
+		return ret;
+
+	msleep(ret / 1000);

...

drivers/iio/light/rohm-bu27034.c line 1053

Quoted 1 line

drivers/iio/light/rohm-bu27034.c new file +1 -0

+	 * Avoid div by zeroi. Not using max() as the data may not be in

zeroi?

...

drivers/iio/light/rohm-bu27034.c line 1056

Quoted 1 line

drivers/iio/light/rohm-bu27034.c new file +1 -0

+	if (!res[0])

Positive conditional?

drivers/iio/light/rohm-bu27034.c line 1062

Quoted 7 lines

drivers/iio/light/rohm-bu27034.c new file +6 -0

+		ch0 = 1;
+	else
+		ch0 = le16_to_cpu(res[0]);
+
+	if (!res[1])
+		ch1 = 1;

Ditto.

Review

Quoted 2 lines

(quoted diff) +2 -0

+	else
+		ch1 = le16_to_cpu(res[1]);

But why not to read and convert first and then check.

This at least will
correctly compare 0 to the LE16 0 (yes, it's the same for 0, but strictly speaking the bits order of lvalue and rvalue is different).

...

drivers/iio/light/rohm-bu27034.c line 1256

Quoted 10 lines

drivers/iio/light/rohm-bu27034.c new file +10 -0

+	switch (mask) {
+	case IIO_CHAN_INFO_INT_TIME:
+		return iio_gts_avail_times(&data->gts, vals, type, length);
+	case IIO_CHAN_INFO_SCALE:
+		return iio_gts_all_avail_scales(&data->gts, vals, type, length);
+	default:
+		break;
+	}
+
+	return -EINVAL;

You may do it from default case.

...

drivers/iio/light/rohm-bu27034.c line 1299

Quoted 2 lines

drivers/iio/light/rohm-bu27034.c new file +2 -0

+	ret = regmap_read_poll_timeout(data->regmap, BU27034_REG_MODE_CONTROL4,
+				       val, (val & BU27034_MASK_VALID),

Redundant parentheses.

drivers/iio/light/rohm-bu27034.c line 1304

Quoted 6 lines

drivers/iio/light/rohm-bu27034.c new file +5 -0

+				       BU27034_DATA_WAIT_TIME_US,
+				       BU27034_TOTAL_DATA_WAIT_TIME_US);
+	if (ret) {
+		dev_err(data->dev, "data polling %s\n",
+			!(val & BU27034_MASK_VALID) ? "timeout" : "fail");

Why not positive conditional in ternary?

Quoted 3 lines

(quoted diff) +2 -0

+		return ret;
+	}

...

drivers/iio/light/rohm-bu27034.c line 1417

Quoted 3 lines

drivers/iio/light/rohm-bu27034.c new file +3 -0

+	fwnode = dev_fwnode(dev);
+	if (!fwnode)
+		return -ENODEV;

So, you deliberately disable a possibility to instantiate this from user space, why?

...

drivers/iio/light/rohm-bu27034.c line 1466

Quoted 3 lines

drivers/iio/light/rohm-bu27034.c new file +3 -0

+	ret = devm_iio_kfifo_buffer_setup(dev, idev, &bu27034_buffer_ops);
+
+	ret = devm_iio_device_register(dev, idev);

Don't you find something strange in between?

Quoted 4 lines

drivers/iio/light/rohm-bu27034.c new file +3 -0

+	if (ret < 0)
+		return dev_err_probe(dev, ret,
+				     "Unable to register iio device\n");

...

drivers/iio/light/rohm-bu27034.c line 1475

Quoted 1 line

drivers/iio/light/rohm-bu27034.c new file +1 -0

+	{ .compatible = "rohm,bu27034", },

Inner comma is not needed.

Quoted 8 lines

Quoted 5 lines

  
  obj-$(CONFIG_OPT3001)		+= opt3001.o
  obj-$(CONFIG_PA12203001)	+= pa12203001.o  

+obj-$(CONFIG_ROHM_BU27034) += rohm-bu27034.o

If you see, most of the components are without vendor prefix, why rohm is special? Like you are expecting the very same filename for something else?

No. I don't.

Using the vendor prefix in _file name_ was suggested to me by Lee already a few years ago. And I am actually grateful he did. I've found that _very_ useful as it simplifies finding the files I am looking for. What comes to the config option name, being able to easily search for the configs by vendor name has also been helpful.

Quoted 8 lines

...

drivers/iio/light/rohm-bu27034.c line 20

Quoted 3 lines

drivers/iio/light/rohm-bu27034.c new file +3 -0

+#include <linux/iio/iio.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/kfifo_buf.h>  

Sorted?

Sure, thanks.

Quoted 9 lines

...

drivers/iio/light/rohm-bu27034.c line 44

Quoted 3 lines

drivers/iio/light/rohm-bu27034.c new file +3 -0

+#define BU27034_REG_DATA0_LO		0x50
+#define BU27034_REG_DATA1_LO		0x52
+#define BU27034_REG_DATA2_LO		0x54  

I would drop _LO in all these

Quoted 4 lines

Quoted 1 line

+#define BU27034_REG_DATA2_HI 0x55

and rename somehow this to something like _END / _MAX (similar to the fields. Perhaps you would need _START / _MIN above.

I don't think this would improve anything. The _LO / _HI are descriptive as we have only two registers for each channel, _LO and _HI being more or less standard abbreviations for low and high.

Quoted 18 lines

...

Quoted 15 lines

drivers/iio/light/rohm-bu27034.c new file +15 -0

+/*
+ * Available scales with gain 1x - 4096x, timings 55, 100, 200, 400 mS
+ * Time impacts to gain: 1x, 2x, 4x, 8x.
+ *
+ * => Max total gain is HWGAIN * gain by integration time (8 * 4096) = 32768
+ *
+ * Using NANO precision for scale we must use scale 64x corresponding gain 1x
+ * to avoid precision loss. (32x would result scale 976 562.5(nanos).
+ */
+#define BU27034_SCALE_1X	64
+
+#define BU27034_GSEL_1X		0x00
+#define BU27034_GSEL_4X		0x08
+#define BU27034_GSEL_16X	0x0a
+#define BU27034_GSEL_32X	0x0b

Quoted 8 lines

drivers/iio/light/rohm-bu27034.c line 107

Quoted 6 lines

drivers/iio/light/rohm-bu27034.c new file +6 -0

+#define BU27034_GSEL_64X	0x0c
+#define BU27034_GSEL_256X	0x18
+#define BU27034_GSEL_512X	0x19
+#define BU27034_GSEL_1024X	0x1a
+#define BU27034_GSEL_2048X	0x1b
+#define BU27034_GSEL_4096X	0x1c  

Shouldn't the values be in plain decimal?

Why?

Quoted 2 lines

Otherwise I would like to understand bit mapping inside these hex values.

I like having register values in hex. It makes it obvious they don't necessarily directly match any 'real world' human-readable values.

Quoted 3 lines

Review

Quoted 1 line

(quoted diff) +1 -0

+		if (helper64 < 0xFFFFFFFFFFFFFLLU) {  

Perhaps this needs a definition.

I like seeing the value here. It makes this less obfuscating. Comment makes the purpose obvious so adding a define would not really give any extra advantage.

Quoted 11 lines

drivers/iio/light/rohm-bu27034.c line 819

Quoted 7 lines

drivers/iio/light/rohm-bu27034.c new file +6 -0

+			helper64 *= gain0;
+			do_div(helper64, ch0);
+		} else {
+			do_div(helper64, ch0);
+			helper64 *= gain0;
+		}  

...> >> + if (!res[0])

Quoted 1 line

Positive conditional?

No. Again, we check for the very specific case where res has all bits zeroed. Inverse condition is counter intuitive.

Quoted 10 lines

drivers/iio/light/rohm-bu27034.c line 1062

Quoted 7 lines

drivers/iio/light/rohm-bu27034.c new file +6 -0

+		ch0 = 1;
+	else
+		ch0 = le16_to_cpu(res[0]);
+
+	if (!res[1])
+		ch1 = 1;  

Ditto.

Quoted 4 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+	else
+		ch1 = le16_to_cpu(res[1]);  

But why not to read and convert first and then check.

Because conversion is not needed if channel data is zero.

Quoted 4 lines

This at least will
correctly compare 0 to the LE16 0 (yes, it's the same for 0, but strictly speaking the bits order of lvalue and rvalue is different).

and hence we check for !res[0]

Quoted 16 lines

...

drivers/iio/light/rohm-bu27034.c line 1256

Quoted 10 lines

drivers/iio/light/rohm-bu27034.c new file +10 -0

+	switch (mask) {
+	case IIO_CHAN_INFO_INT_TIME:
+		return iio_gts_avail_times(&data->gts, vals, type, length);
+	case IIO_CHAN_INFO_SCALE:
+		return iio_gts_all_avail_scales(&data->gts, vals, type, length);
+	default:
+		break;
+	}
+
+	return -EINVAL;  

You may do it from default case.

I think we have discussed this one in the past too. I like having return at the end of a non void function.

Quoted 26 lines

Quoted 17 lines

...

Quoted 15 lines

drivers/iio/light/rohm-bu27034.c new file +15 -0

+/*
+ * Available scales with gain 1x - 4096x, timings 55, 100, 200, 400 mS
+ * Time impacts to gain: 1x, 2x, 4x, 8x.
+ *
+ * => Max total gain is HWGAIN * gain by integration time (8 * 4096) = 32768
+ *
+ * Using NANO precision for scale we must use scale 64x corresponding gain 1x
+ * to avoid precision loss. (32x would result scale 976 562.5(nanos).
+ */
+#define BU27034_SCALE_1X	64
+
+#define BU27034_GSEL_1X		0x00
+#define BU27034_GSEL_4X		0x08
+#define BU27034_GSEL_16X	0x0a
+#define BU27034_GSEL_32X	0x0b

Quoted 8 lines

drivers/iio/light/rohm-bu27034.c line 107

Quoted 6 lines

drivers/iio/light/rohm-bu27034.c new file +6 -0

+#define BU27034_GSEL_64X	0x0c
+#define BU27034_GSEL_256X	0x18
+#define BU27034_GSEL_512X	0x19
+#define BU27034_GSEL_1024X	0x1a
+#define BU27034_GSEL_2048X	0x1b
+#define BU27034_GSEL_4096X	0x1c

Shouldn't the values be in plain decimal?

Quoted 1 line

Why?

Normally go with datasheet on this as it makes reviewer simpler. But datasheet is in binary so meh.

Quoted 1 line

Quoted 4 lines

Quoted 1 line

Otherwise I would like to understand bit mapping inside these hex values.

I like having register values in hex. It makes it obvious they don't necessarily directly match any 'real world' human-readable values.

Perhaps a cross reference to the table in the spec is appropriate here?

whilst there are some patterns they aren't terribly consistent so probably best to just point at the table in the spec.

Quoted 4 lines

Quoted 3 lines

Review

Quoted 1 line

(quoted diff) +1 -0

+		if (helper64 < 0xFFFFFFFFFFFFFLLU) {

Perhaps this needs a definition.

Quoted 3 lines

I like seeing the value here. It makes this less obfuscating. Comment makes the purpose obvious so adding a define would not really give any extra advantage.

It's not immediately obvious why it is that many f's. Perhaps change to refer to number of bits (which is what matters really I think) and then use GENMASK() to fill this in? I think it's 52 bits?

Quoted 30 lines

Quoted 11 lines

drivers/iio/light/rohm-bu27034.c line 819

Quoted 6 lines

drivers/iio/light/rohm-bu27034.c new file +6 -0

+			helper64 *= gain0;
+			do_div(helper64, ch0);
+		} else {
+			do_div(helper64, ch0);
+			helper64 *= gain0;
+		}

...> >> + if (!res[0])

Positive conditional?

No. Again, we check for the very specific case where res has all bits zeroed. Inverse condition is counter intuitive.

Quoted 14 lines

drivers/iio/light/rohm-bu27034.c line 1062

Quoted 7 lines

drivers/iio/light/rohm-bu27034.c new file +6 -0

+		ch0 = 1;
+	else
+		ch0 = le16_to_cpu(res[0]);
+
+	if (!res[1])
+		ch1 = 1;

Ditto.

Review

Quoted 2 lines

(quoted diff) +2 -0

+	else
+		ch1 = le16_to_cpu(res[1]);

But why not to read and convert first and then check.

Quoted 1 line

Because conversion is not needed if channel data is zero.

Conversion is trivial. I agree with Andy here that the logic would look a bit simpler as (taking it a little further)

	ch0 = max(1, le16_to_cpu(res[0]));

Quoted 15 lines

Quoted 14 lines

drivers/iio/light/rohm-bu27034.c line 1256

Quoted 11 lines

drivers/iio/light/rohm-bu27034.c new file +10 -0

+	switch (mask) {
+	case IIO_CHAN_INFO_INT_TIME:
+		return iio_gts_avail_times(&data->gts, vals, type, length);
+	case IIO_CHAN_INFO_SCALE:
+		return iio_gts_all_avail_scales(&data->gts, vals, type, length);
+	default:
+		break;
+	}
+
+	return -EINVAL;

You may do it from default case.

Quoted 2 lines

I think we have discussed this one in the past too. I like having return at the end of a non void function.

Given all the earlier returns and the fact that the compiler will shout at you if it you can get here and it is missing, I'd also suggest just putting it in the switch statement.

ROHM BU27034 is an ambient light sesnor with 3 channels and 3 photo diodes capable of detecting a very wide range of illuminance. Typical application is adjusting LCD and backlight power of TVs and mobile phones.

Add initial support for the ROHM BU27034 ambient light sensor.

NOTE:
- Driver exposes 4 channels. One IIO_LIGHT channel providing the calculated lux values based on measured data from diodes #0 and #1. Additionally 3 IIO_INTENSITY channels are emitting the raw register data from all diodes for more intense user-space computations.
- Sensor has adjustible GAIN values ranging from 1x to 4096x.
- Sensor has adjustible measurement times 5, 55, 100, 200 and 400 mS. Driver does not support 5 mS which has special limitations.
- Driver exposes standard 'scale' adjustment which is implemented by:

1) Trying to adjust only the GAIN
2) If GAIN adjustment only can't provide requested scale, adjusting both the time and the gain is attempted.
- Driver exposes writable INT_TIME property which can be used for adjusting the measurement time. Time adjustment will also cause the driver to adjust the GAIN so that the overall scale is not changed.

- Runtime PM is not implemented.

- Driver starts the measurement on the background when it is probed. This improves the respnse time to read-requests

compared to starting the read only when data is requested. When the most accurate 400 mS measurement time is used, data reads would last quite long if measurement was started only on demand. This, however, is not appealing for users who would prefere power saving over measurement response time.

Signed-off-by Matti Vaittinen <[email protected]>

---
Changes since RFCv1
- (really) protect read-only registers
- fix get and set gain
- buffered mode
- Protect the whole sequences including meas_en/meas_dis to avoid messing up the enable / disable order
- typofixes / doc improvements
- change dropped GAIN_SCALE_ITIME_MS() to GAIN_SCALE_ITIME_US()
- use more accurate scale for lux channel (milli lux)
- provide available scales / integration times (using helpers).
- adapt to renamed iio-gts-helpers.h file
- bu27034 - longer lines in Kconfig
- Drop bu27034_meas_en and bu27034_meas_dis wrappers.
- Change device-name from bu27034-als to bu27034
---

drivers/iio/light/rohm-bu27034.c

Quoted 1 line

drivers/iio/light/rohm-bu27034.c +0 -0

new file mode 100644
index 000000000000..b8ca29af22e7

+ * And because the sleep can't wake up _exactly_ at given time this would be
+ * inevitable even if the sensor clock would be perfectly phase-locked to CPU
+ * clock - which we can't say is the case.
+ *
+ * This is still fragile. No matter how big advance do we have, we will still
+ * risk of losing a sample because things can in a rainy-day skenario be
+ * delayed a lot. Yet, more we reserve the time for polling, more we also lose
+ * the performance by spending cycles polling the register. So, selecting this
+ * value is a balancing dance between severity of wasting CPU time and severity
+ * of losing samples.
+ *
+ * In most cases losing the samples is not _that_ crucial because light levels
+ * tend to change slowly.
+ */

drivers/iio/light/rohm-bu27034.c new file +5 -0

+
+enum {
+	BU27034_CHAN_ALS,
+	BU27034_CHAN_DATA0,
+	BU27034_CHAN_DATA1,

drivers/iio/light/rohm-bu27034.c new file +24 -0

+		.channel = BU27034_CHAN_ALS,
+		.scan_index = BU27034_CHAN_ALS,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 32,
+			.storagebits = 32,
+			.endianness = IIO_CPU,
+		},
+	},
+	/*
+	 * The BU27034 DATA0 and DATA1 channels are both on the visible light
+	 * area (mostly). The data0 sensitivity peaks at 500nm, DATA1 at 600nm.
+	 * These wave lengths are pretty much on the border of colours making
+	 * these a poor candidates for R/G/B standardization. Hence they're both
+	 * marked as clear channels
+	 */
+	BU27034_CHAN_DATA(DATA0, IIO_MOD_LIGHT_CLEAR),
+	BU27034_CHAN_DATA(DATA1, IIO_MOD_LIGHT_CLEAR),
+	BU27034_CHAN_DATA(DATA2, IIO_MOD_LIGHT_IR),
+	IIO_CHAN_SOFT_TIMESTAMP(4),
+};
 
 
+/* Caller should hold the lock to protect data->int_time */
+static int bu27034_set_int_time(struct bu27034_data *data, int time)
+{

drivers/iio/light/rohm-bu27034.c new file +48 -0

+}
+
+/*
+ * We try to change the time in such way that the scale is maintained for
+ * given channels by adjusting gain so that it compensates the time change.
+ */
+static int bu27034_try_set_int_time(struct bu27034_data *data, int time_us)
+{
+	int ret, int_time_old, int_time_new, i;
+	struct bu27034_gain_check gains[3] = {
+		{ .chan = BU27034_CHAN_DATA0, },
+		{ .chan = BU27034_CHAN_DATA1, },
+		{ .chan = BU27034_CHAN_DATA2 }
+	};
+	int numg = ARRAY_SIZE(gains);
+
+	mutex_lock(&data->mutex);
+	ret = bu27034_get_int_time(data);
+	if (ret < 0)
+		goto unlock_out;
+
+	int_time_old = ret;
+
+	ret = bu27034_validate_int_time(data, time_us);
+	if (ret < 0) {
+		dev_err(data->dev, "Unsupported integration time %u\n",
+			time_us);
+
+		goto unlock_out;
+	}
+
+	int_time_new = ret;
+
+	if (int_time_new == int_time_old) {
+		ret = 0;
+		goto unlock_out;
+	}
+
+	for (i = 0; i < numg; i++) {
+		ret = bu27034_get_gain(data, gains[i].chan,
+				       &gains[i].old_gain);
+		if (ret)
+			goto unlock_out;
+
+		gains[i].new_gain = gains[i].old_gain * int_time_old /
+				    int_time_new;
+
+		if (!iio_gts_valid_gain(&data->gts, gains[i].new_gain)) {

drivers/iio/light/rohm-bu27034.c new file +6 -0

+			 * integration time change and we can't support the
+			 * scale - then the caller should be prepared to
+			 * 'pick up the pieces and deal with the fact that the
+			 * scale changed'.
+			 */
+			ret = iio_find_closest_gain_low(&data->gts,

drivers/iio/light/rohm-bu27034.c new file +28 -0

+	 */
+	for (i = 0; i < numg; i++) {
+		ret = bu27034_set_gain(data, gains[i].chan, gains[i].new_gain);
+		if (ret)
+			goto unlock_out;
+	}
+
+	ret = bu27034_set_int_time(data, int_time_new);
+
+unlock_out:
+	mutex_unlock(&data->mutex);
+
+	return ret;
+}
+
+static int bu27034_set_scale(struct bu27034_data *data, int chan,
+			    int val, int val2)
+{
+	int ret, time_sel, gain_sel, i;
+	bool found = false;
+
+	if (chan == BU27034_CHAN_ALS) {
+		if (val == 0 && val2 == 1000)
+			return 0;
+		else
+			return -EINVAL;
+	}
+