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

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

+static int iio_gts_get_gain(const u64 max, const u64 scale)
+{
+	int tmp = 1;
+
+	if (scale > max || !scale)
+		return -EINVAL;

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

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

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

+		     (u64)(scale_nano / (NANO / scaler));

+EXPORT_SYMBOL_NS_GPL(iio_gts_total_gain_to_scale, IIO_GTS_HELPER);

(quoted diff) +1 -0

+		 */

(quoted diff) +1 -0

+		kfree(gts->avail_all_scales_table);

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

+	if (!per_time_scales)
+		goto free_gains;

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

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

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

+ */

(quoted diff) +1 -0

+		return -EINVAL;

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

+	for (i = 0; i < gts->num_hwgain; i++) {
+		/*
+		 * It is not expected this function is called for an exactly
+		 * matching gain.

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

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

(quoted diff) +2 -0

+	if (ret)
+		return -EINVAL;

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

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

Quoted 5 lines

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.

...

Quoted 1 line

+/*

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

Quoted 13 lines

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

...

Quoted 1 line

+EXPORT_SYMBOL_NS_GPL(iio_gts_total_gain_to_scale, IIO_GTS_HELPER);

I would say _HELPER part is too much, but fine with me.

Quoted 1 line

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

Is it _only_ for a Light type of sensors?

Quoted 25 lines · On Mon, 6 Mar 2023 14:52:57 +0200 Andy Shevchenko…

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

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

...

Quoted 5 lines

Quoted 1 line

+/*

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

Just make it kernel-doc.

Quoted 13 lines

Quoted 13 lines

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

Quoted 5 lines

...

Quoted 1 line

+EXPORT_SYMBOL_NS_GPL(iio_gts_total_gain_to_scale, IIO_GTS_HELPER);

I would say _HELPER part is too much, but fine with me.

Hmm. I think I like the HELPER bit as separates it from being a driver. Of course I might change my mind after a few sleeps.

Quoted 4 lines

Quoted 2 lines

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

Is it _only_ for a Light type of sensors?

I'd move it up a directory and allow for other users.

Jonathan

Quoted 3 lines · Jonathan Cameron

Quoted 1 line · On Mon, 6 Mar 2023 14:52:57 +0200 Andy Shevchenko…

On Mon, Mar 06, 2023 at 11:17:15AM +0200, Matti Vaittinen wrote:

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

+EXPORT_SYMBOL_NS_GPL(iio_gts_total_gain_to_scale, IIO_GTS_HELPER);

I would say _HELPER part is too much, but fine with me.

Hmm. I think I like the HELPER bit as separates it from being a driver. Of course I might change my mind after a few sleeps.

Ever considered a career as a politician? ;) (No offense intended - and feel free to change your mind on this. I don't expect this to be done tomorrow)

Quoted 4 lines · Matti Vaittinen

Quoted 3 lines · Jonathan Cameron

Quoted 1 line · On Mon, 6 Mar 2023 14:52:57 +0200 Andy Shevchenko…

On Mon, Mar 06, 2023 at 11:17:15AM +0200, Matti Vaittinen wrote:

...

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

Quoted 1 line

+EXPORT_SYMBOL_NS_GPL(iio_gts_total_gain_to_scale, IIO_GTS_HELPER);

I would say _HELPER part is too much, but fine with me.

Quoted 5 lines

Quoted 2 lines

Hmm. I think I like the HELPER bit as separates it from being a driver. Of course I might change my mind after a few sleeps.

Ever considered a career as a politician? ;) (No offense intended - and feel free to change your mind on this. I don't expect this to be done tomorrow)

It will be a one liner in the provider if you use DEFAULT_SYMBOL_NAMESPACE definition.

Quoted 5 lines · Andy Shevchenko

Quoted 4 lines · Matti Vaittinen

Quoted 3 lines · Jonathan Cameron

Quoted 1 line · On Mon, 6 Mar 2023 14:52:57 +0200 Andy Shevchenko…

On Mon, Mar 06, 2023 at 11:17:15AM +0200, Matti Vaittinen wrote:

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

Quoted 1 line

+EXPORT_SYMBOL_NS_GPL(iio_gts_total_gain_to_scale, IIO_GTS_HELPER);

I would say _HELPER part is too much, but fine with me.

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

Hmm. I think I like the HELPER bit as separates it from being a driver. Of course I might change my mind after a few sleeps.

Ever considered a career as a politician? ;) (No offense intended - and feel free to change your mind on this. I don't expect this to be done tomorrow)

It will be a one liner in the provider if you use DEFAULT_SYMBOL_NAMESPACE definition.

Oh. I didn't know about DEFAULT_SYMBOL_NAMESPACE - or if I did, I had forgot it. My memory has never been great and seems to be getting worse all the time...

I don't know what to think of this define though. I can imagine that someone who is not familiar with it could be very confused as to why the symbols are not found even though EXPORT_SYMBOL or EXPORT_SYMBOL_GPL are used. OTOH, I think I once saw an error about symbols being in a namespace (when trying to use one without the namespace). This should probably just be a good enough hint for finding out what's going on.

Luckily, I think all the exports in this case were oneliners even with the namespace explicitly spelled. Well, I think that for one or two exports the semicolon did slip to col 81 or 82 - but I am not sure if fixing this weighs more than the clarity of explicitly showing the namespace in export.

Well, I guess I can go with either of these ways - do you have a strong opinion on using the DEFAULT_SYMBOL_NAMESPACE?

Quoted 12 lines · Andy Shevchenko

Quoted 4 lines · Matti Vaittinen

Quoted 3 lines · Jonathan Cameron

Quoted 1 line · On Mon, 6 Mar 2023 14:52:57 +0200 Andy Shevchenko…

On Mon, Mar 06, 2023 at 11:17:15AM +0200, Matti Vaittinen wrote:

...

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

Quoted 1 line

+EXPORT_SYMBOL_NS_GPL(iio_gts_total_gain_to_scale, IIO_GTS_HELPER);

I would say _HELPER part is too much, but fine with me.

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

Hmm. I think I like the HELPER bit as separates it from being a driver. Of course I might change my mind after a few sleeps.

Ever considered a career as a politician? ;) (No offense intended - and feel free to change your mind on this. I don't expect this to be done tomorrow)

It will be a one liner in the provider if you use DEFAULT_SYMBOL_NAMESPACE definition.

Oh. I didn't know about DEFAULT_SYMBOL_NAMESPACE - or if I did, I had forgot it. My memory has never been great and seems to be getting worse all the time...

I don't know what to think of this define though. I can imagine that someone who is not familiar with it could be very confused as to why the symbols are not found even though EXPORT_SYMBOL or EXPORT_SYMBOL_GPL are used. OTOH, I think I once saw an error about symbols being in a namespace (when trying to use one without the namespace). This should probably just be a good enough hint for finding out what's going on.

Luckily, I think all the exports in this case were oneliners even with the namespace explicitly spelled. Well, I think that for one or two exports the semicolon did slip to col 81 or 82 - but I am not sure if fixing this weighs more than the clarity of explicitly showing the namespace in export.

Well, I guess I can go with either of these ways - do you have a strong opinion on using the DEFAULT_SYMBOL_NAMESPACE?

Yours,

	--Matti

Quoted 34 lines · Matti

Quoted 12 lines · Andy Shevchenko

Quoted 4 lines · Matti Vaittinen

Quoted 3 lines · Jonathan Cameron

Quoted 1 line · On Mon, 6 Mar 2023 14:52:57 +0200 Andy Shevchenko…

On Mon, Mar 06, 2023 at 11:17:15AM +0200, Matti Vaittinen wrote:

...

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

Quoted 1 line

+EXPORT_SYMBOL_NS_GPL(iio_gts_total_gain_to_scale, IIO_GTS_HELPER);

I would say _HELPER part is too much, but fine with me.

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

Hmm. I think I like the HELPER bit as separates it from being a driver. Of course I might change my mind after a few sleeps.

Ever considered a career as a politician? ;) (No offense intended - and feel free to change your mind on this. I don't expect this to be done tomorrow)

It will be a one liner in the provider if you use DEFAULT_SYMBOL_NAMESPACE definition.

Oh. I didn't know about DEFAULT_SYMBOL_NAMESPACE - or if I did, I had forgot it. My memory has never been great and seems to be getting worse all the time...

I don't know what to think of this define though. I can imagine that someone who is not familiar with it could be very confused as to why the symbols are not found even though EXPORT_SYMBOL or EXPORT_SYMBOL_GPL are used. OTOH, I think I once saw an error about symbols being in a namespace (when trying to use one without the namespace). This should probably just be a good enough hint for finding out what's going on.

Quoted 9 lines

Luckily, I think all the exports in this case were oneliners even with the namespace explicitly spelled. Well, I think that for one or two exports the semicolon did slip to col 81 or 82 - but I am not sure if fixing this weighs more than the clarity of explicitly showing the namespace in export.

Well, I guess I can go with either of these ways - do you have a strong opinion on using the DEFAULT_SYMBOL_NAMESPACE?

If it's in the C file, then I can cope with doing it this way. Don't do it in the compiler options though. That got ripped out of CXL because it was considered a bad idea to hide the namespace away like that.

Personally I prefer the namespace of the symbols explicit in each export as they are easy to find that way.

Quoted 7 lines

Quoted 6 lines

Quoted 2 lines

+/*

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

Quoted 2 lines

Just make it kernel-doc.

Are you sure...? I don't like the idea of polluting generated docs with documentation for this type of tiny internal pieces not usable outside this component anyways...

Quoted 17 lines

Quoted 17 lines

Quoted 14 lines

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

...

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

Quoted 5 lines

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.

...

Quoted 1 line

+/*

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

Quoted 24 lines

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

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

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

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

Quoted 4 lines

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

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

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

Quoted 3 lines

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

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

No space for castings?

Quoted 5 lines

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

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

...

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

Quoted 3 lines

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

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

Missing period.

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

Quoted 3 lines

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

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

Shouldn't be OVERFLOW for the first one?

...

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

Quoted 1 line

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

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

No space for casting?

...

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

Quoted 13 lines

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

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

...

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

Quoted 3 lines

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

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

Quoted 1 line

(quoted diff) +1 -0

+		 */

...

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

Quoted 2 lines

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

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

One line reads better?

...

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

Quoted 1 line

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

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

Quoted 2 lines

(quoted diff) +1 -0

+		kfree(gts->avail_all_scales_table);

...

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

Quoted 1 line

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

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

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

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

Quoted 5 lines

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

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

Ditto.

Quoted 2 lines

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

+	if (!per_time_scales)
+		goto free_gains;

...

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

Quoted 6 lines

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

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

Quoted 5 lines

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

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

...

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

Quoted 1 line

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

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

	while (i--) {

makes it easier to parse.

Quoted 9 lines

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

Missing period.

Seems these problems occur in many function descriptions.

Quoted 1 line

+ */

...

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

Quoted 3 lines

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

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

Review

Quoted 2 lines

(quoted diff) +2 -0

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

Quoted 2 lines

(quoted diff) +1 -0

+		return -EINVAL;

...

Quoted 9 lines

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

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

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

Quoted 4 lines

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

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

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

Quoted 13 lines

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

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

Quoted 17 lines

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

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

...

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

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

Still can be one line.

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

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;

...

Quoted 1 line

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

Is it _only_ for a Light type of sensors?

...

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

Quoted 2 lines

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

+#ifndef __IIO_GTS_HELPER__
+#define __IIO_GTS_HELPER__

If yes, perhaps adding LIGHT here?

Quoted 1 line · Andy Shevchenko

On Mon, Mar 06, 2023 at 11:17:15AM +0200, Matti Vaittinen wrote:

Quoted 5 lines

Quoted 1 line

+/*

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

Quoted 12 lines

Quoted 12 lines

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

Quoted 4 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

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

Shouldn't be OVERFLOW for the first one?

-EOVERFLOW? I guess it could be. Thanks.

Quoted 4 lines

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

Quoted 2 lines

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

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

Quoted 4 lines

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

Quoted 1 line

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

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

Quoted 1 line

Quoted 1 line

(quoted diff) +1 -0

+		kfree(gts->avail_all_scales_table);

Quoted 3 lines

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

Quoted 1 line

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

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

Quoted 1 line

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

Quoted 1 line

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

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

Quoted 10 lines

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

Quoted 6 lines

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

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

Quoted 5 lines

Quoted 5 lines

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

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

Quoted 5 lines

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

Quoted 1 line

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

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

Quoted 6 lines

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

Quoted 4 lines

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

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

Quoted 2 lines · Matti Vaittinen

Quoted 1 line · Andy Shevchenko

On Mon, Mar 06, 2023 at 11:17:15AM +0200, Matti Vaittinen wrote:

...

Quoted 6 lines

Quoted 5 lines

Quoted 1 line

+/*

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

Quoted 3 lines

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

The problem as I pointed out with your approach it's unmaintainable. And I even explained why I consider it this way.

Quoted 8 lines

Quoted 5 lines

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

Quoted 3 lines

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

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

With it on one line

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

You have N at the last column which quite likely suggests that it's NULL. So, I don't think it's a big issue to put on a single line.

Quoted 5 lines

Quoted 4 lines

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

Quoted 1 line

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

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

Quoted 5 lines

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.

So, and with it you put now a double check for NULL, do you think it's okay? I don't.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

(quoted diff) +1 -0

+		kfree(gts->avail_all_scales_table);

...

Quoted 14 lines

Quoted 3 lines

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

Quoted 1 line

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

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

Quoted 2 lines

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.

Still get a fundamental disagreement on this. I would insist, but I'm not a maintainer, so you are lucky :-) if Jonathan will not force you to follow my way.

...

Quoted 4 lines

Quoted 4 lines

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

Quoted 1 line

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

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

	while (i--) {

Quoted 4 lines

Quoted 1 line

makes it easier to parse.

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

Yes, and I will continue insisting on while (foo--). That why I won't give you my tags :-)

...

Quoted 10 lines

Quoted 6 lines

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

Quoted 4 lines

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

+		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

Quoted 6 lines

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!

Hint: run always make W=1 when building kernel.

It will show defined but not used cases and combined with nowadays default -Werror won't be compilable.

Quoted 3 lines · Andy Shevchenko

Quoted 2 lines · Matti Vaittinen

Quoted 1 line · Andy Shevchenko

On Mon, Mar 06, 2023 at 11:17:15AM +0200, Matti Vaittinen wrote:

Quoted 11 lines

Quoted 10 lines

Quoted 4 lines

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

Quoted 1 line

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

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

Quoted 2 lines

So, and with it you put now a double check for NULL, do you think it's okay? I don't.

I don't see the double check. I see only one check just above the kfree()? Where is the other check?

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

(quoted diff) +1 -0

+		kfree(gts->avail_all_scales_table);

Quoted 4 lines · Matti Vaittinen

Quoted 3 lines · Andy Shevchenko

Quoted 2 lines · Matti Vaittinen

Quoted 1 line · Andy Shevchenko

On Mon, Mar 06, 2023 at 11:17:15AM +0200, Matti Vaittinen wrote:

...

Quoted 14 lines

Quoted 13 lines

Quoted 10 lines

Quoted 4 lines

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

Quoted 1 line

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

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

So, and with it you put now a double check for NULL, do you think it's okay? I don't.

Quoted 2 lines

I don't see the double check. I see only one check just above the kfree()? Where is the other check?

	if (... gts->avail_all_scales_table)

is a double to one, which is inside kfree(). I.o.w. kfree() is NULL-aware and you know that.

Quoted 20 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

(quoted diff) +1 -0

+		kfree(gts->avail_all_scales_table);  

...

Quoted 14 lines

Quoted 3 lines

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

Quoted 1 line

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

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

Quoted 6 lines

Quoted 2 lines

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.

Still get a fundamental disagreement on this. I would insist, but I'm not a maintainer, so you are lucky :-) if Jonathan will not force you to follow my way.

In a code you are maintaining it is good to have it in your way as you're responsible for it. This is also why I insist on having things in a way I can read best for a code I plan to maintain - unless the subsystem maintainers see it hard to maintain for them. So, let's see if Jonathan has strong opinions on this one :)

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

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

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

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

new file mode 100644
index 000000000000..4b5a417946f4

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

+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

Quoted 16 lines · On Mon, 6 Mar 2023 11:17:15 +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.

Quoted 7 lines

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

Trying not to duplicate what Andy has raised...

At some stage I want to go through the maths very carefully but it's not happening today and I don't want to delay resolving other remaining comments so that can wait for a later version. I'm sure it's fine but I like to be paranoid :)

Quoted 7 lines

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

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

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

Quoted 10 lines

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

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

All of them want to be in the namespace.

Quoted 5 lines

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

new file mode 100644
index 000000000000..4b5a417946f4

...

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

Quoted 9 lines

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

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

Given most modern IIO drivers use fully devm_ based probing, for now I would not expose anything else. That will reduce the interface a lot which I think is probably a good thing at this stage.

Keep the non devm stuff internally though as it is a nice structure to have an I can see we may want some of these in non devm form in the future.

Similarly - for now don't expose the individual table building functions as we may never need them in drivers. We (more or less) only support interfaces that are used and so far they aren't.

For other functions it's worth thinking about whether to not export them initially. I haven't been through them all to figure out what is not currently used.

Quoted 11 lines

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

+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  

Quoted 2 lines · On Sun, 12 Mar 2023 17:06:38 +0000 Jonathan Cameron <[email protected]> wrote:

On Mon, 6 Mar 2023 11:17:15 +0200
Matti Vaittinen <[email protected]> wrote:

Quoted 7 lines

Given most modern IIO drivers use fully devm_ based probing, for now I would not expose anything else. That will reduce the interface a lot which I think is probably a good thing at this stage.

Keep the non devm stuff internally though as it is a nice structure to have an I can see we may want some of these in non devm form in the future.

Quoted 7 lines

Similarly - for now don't expose the individual table building functions as we may never need them in drivers. We (more or less) only support interfaces that are used and so far they aren't.

For other functions it's worth thinking about whether to not export them initially. I haven't been through them all to figure out what is not currently used.

Ah. I forgot the tests that don't have a device so can't use devm.

Quoted 4 lines · Jonathan Cameron

Quoted 2 lines · On Sun, 12 Mar 2023 17:06:38 +0000 Jonathan Cameron <[email protected]> wrote:

On Mon, 6 Mar 2023 11:17:15 +0200
Matti Vaittinen <[email protected]> wrote:

...

Quoted 3 lines

Quoted 3 lines

Given most modern IIO drivers use fully devm_ based probing, for now I would not expose anything else. That will reduce the interface a lot which I think is probably a good thing at this stage.

Quoted 3 lines

Quoted 3 lines

Keep the non devm stuff internally though as it is a nice structure to have an I can see we may want some of these in non devm form in the future.

Quoted 4 lines

Quoted 4 lines

Similarly - for now don't expose the individual table building functions as we may never need them in drivers. We (more or less) only support interfaces that are used and so far they aren't.

Quoted 3 lines

Quoted 3 lines

For other functions it's worth thinking about whether to not export them initially. I haven't been through them all to figure out what is not currently used.

Quoted 2 lines

Quoted 1 line

Ah. I forgot the tests that don't have a device so can't use devm.

Why not? I have seen, IIRC, test cases inside the kernel that fakes the device for that.

Quoted 3 lines · Andy Shevchenko

Quoted 2 lines · Jonathan Cameron

On Sun, 12 Mar 2023 17:06:38 +0000
Jonathan Cameron <[email protected]> wrote:

Quoted 4 lines

Quoted 1 line

Ah. I forgot the tests that don't have a device so can't use devm.

Why not? I have seen, IIRC, test cases inside the kernel that fakes the device for that.

I'd appreciated any pointer for such an example if you have one at hand. (I can do the digging if you don't though!)

I am not a fan of unit tests. They add huge amount of inertia to development, and in worst case, they stop people from contributing where improving a feature requires test code modification(s). And harder the test code is to understand, worse the unwanted side-effects. Also, harder the test code is to read, more time and effort it requires to analyze a test failure... Hence, I am _very_ conservative what comes to adding size of test code with anything that is not strictly required.

After that being said, unit tests are a great tool when carefully used - and I assume/hope stubbing a device for devm_ tests does not add much extra... But let me see if I can find an example :)

Quoted 4 lines · Matti Vaittinen

Quoted 3 lines · Andy Shevchenko

Quoted 2 lines · Jonathan Cameron

On Sun, 12 Mar 2023 17:06:38 +0000
Jonathan Cameron <[email protected]> wrote:

...

Quoted 16 lines

Quoted 4 lines

Quoted 1 line

Ah. I forgot the tests that don't have a device so can't use devm.

Why not? I have seen, IIRC, test cases inside the kernel that fakes the device for that.

I'd appreciated any pointer for such an example if you have one at hand. (I can do the digging if you don't though!)

I am not a fan of unit tests. They add huge amount of inertia to development, and in worst case, they stop people from contributing where improving a feature requires test code modification(s). And harder the test code is to understand, worse the unwanted side-effects. Also, harder the test code is to read, more time and effort it requires to analyze a test failure... Hence, I am _very_ conservative what comes to adding size of test code with anything that is not strictly required.

Quoted 3 lines

After that being said, unit tests are a great tool when carefully used - and I assume/hope stubbing a device for devm_ tests does not add much extra... But let me see if I can find an example :)

drivers/gpu/drm/tests/drm_managed_test.c ?

(somewhere underneath:

  ret = platform_driver_register(&fake_platform_driver);

which suggests... what exactly? :-)

Quoted 28 lines · Andy Shevchenko

Quoted 4 lines · Matti Vaittinen

Quoted 3 lines · Andy Shevchenko

Quoted 2 lines · Jonathan Cameron

On Sun, 12 Mar 2023 17:06:38 +0000
Jonathan Cameron <[email protected]> wrote:

...

Quoted 19 lines

Quoted 4 lines

Quoted 1 line

Ah. I forgot the tests that don't have a device so can't use devm.

Why not? I have seen, IIRC, test cases inside the kernel that fakes the device for that.

I'd appreciated any pointer for such an example if you have one at hand. (I can do the digging if you don't though!)

I am not a fan of unit tests. They add huge amount of inertia to development, and in worst case, they stop people from contributing where improving a feature requires test code modification(s). And harder the test code is to understand, worse the unwanted side-effects. Also, harder the test code is to read, more time and effort it requires to analyze a test failure... Hence, I am _very_ conservative what comes to adding size of test code with anything that is not strictly required.

After that being said, unit tests are a great tool when carefully used - and I assume/hope stubbing a device for devm_ tests does not add much extra... But let me see if I can find an example :)

Quoted 7 lines

drivers/gpu/drm/tests/drm_managed_test.c ?

(somewhere underneath:

  ret = platform_driver_register(&fake_platform_driver);

which suggests... what exactly? :-)

Quoted 6 lines · Matti Vaittinen

Quoted 5 lines · Andy Shevchenko

Quoted 4 lines · Matti Vaittinen

Quoted 3 lines · Andy Shevchenko

Quoted 2 lines · Jonathan Cameron

On Sun, 12 Mar 2023 17:06:38 +0000
Jonathan Cameron <[email protected]> wrote:

Quoted 26 lines

Quoted 26 lines

Quoted 25 lines

Quoted 5 lines

Quoted 1 line

Ah. I forgot the tests that don't have a device so can't use devm.

Why not? I have seen, IIRC, test cases inside the kernel that fakes the device
for that.

I'd appreciated any pointer for such an example if you have one at hand. (I
can do the digging if you don't though!)

I am not a fan of unit tests. They add huge amount of inertia to development, and in worst case, they stop people from contributing where improving a feature requires test code modification(s). And harder the test
code is to understand, worse the unwanted side-effects. Also, harder the test code is to read, more time and effort it requires to analyze a test failure... Hence, I am _very_ conservative what comes to adding size of test
code with anything that is not strictly required.

After that being said, unit tests are a great tool when carefully used - and
I assume/hope stubbing a device for devm_ tests does not add much extra...
But let me see if I can find an example :)

Quoted 7 lines

Quoted 7 lines

drivers/gpu/drm/tests/drm_managed_test.c ?

(somewhere underneath:

  ret = platform_driver_register(&fake_platform_driver);

which suggests... what exactly? :-)

Thanks to pointer from Andy I found the drm_kunit_helper_[alloc/free]_device() functions. I renamed them to test_kunit_helper_[alloc/free]_device(), move them to drivers/base, add declarations to include/kunit/test-helpers.h fixed KConfigs and existing callers + added the tests for managed interfaces. I have this in place in my personal playground where I am working towards the v4 of the series.

...

After that I asked from Maxime if he had a reason to not make those generic and available to other subsystems besides drm in the first place...

And Maxime was kind enough to point me to the fact that something like this was done in the CCF context:
https://lore.kernel.org/all/[email protected]/

I like the 'single function to get the dummy device which can be passed to devm'-approach used in drm helpers. I do also like Stephen's idea of having the prototypes in kunit/platform_device.h which matches the linux/platform_device.h.

However, I don't know when Stephen's work will be finished and merged to IIO-tree so that it could be used/extended for the needs of these tests.

Meanwhile, I don't think it makes sense to go forward with my changes splitting the helpers out of drm until we see what Stephen's changes will bring us. On the other hand, I don't like delaying the gts-helpers or the sensor drivers.

So, any suggestions what I should do? I see following options:

1) Drop the tests for managed interfaces for now.
2) Add the tests with a yet-another duplicate implementation of the dummy device for devm.
3) Add the tests using the helpers from drm as they are now.

option 1):
I like it as it would be an easy way (for now) - but I hate it as it may be a hard way as well. In my experience, when a driver/helper lands upstream it will get first few fixes quite fast - and not having a test available upstream when this happens is bad. Bad because it means the out-of-tree test may get broken, and bad because there is no easy way to test the fixes.

option 2):
I hate it because it makes the test code more complex - and duplicates the kernel code which is never nice. This could be reworked later when Stephens work is done though.

option 3):
It's in general not nice to use functions exported for some other subsystem's specific purposes. This would however keep the test code at minimum, while leaving the same "I swear I'll fix this later when dependencies have settled" - possibility as option 2) did.

Oh, in theory there is option 4) to just send out the changes I did(*) which pull the drm_kunit_helper_[alloc/free]_device() out of the DRM - but I guess that would lead some extra work to merge this later with stuff Stephen's series does introduce.

Any suggestions which of the options to proceed with?

(*) For those interested in seeing the result of pulling the drm_kunit_helper_[alloc/free]_device() out of DRM tests, below are links to my personal playground with following remarks:
1) code one finds from there may be 100% untested
2) code one finds there may be written just for fun, or for a very specific purpose
3) code one finds there is generally not maintained, may be rebased, may vanish or turn into rabbits or turn you into a rabbit when you wear a top hat.

commits to look at there are
https://github.com/M-Vaittinen/linux/commit/15d07e799f7c7fddc91030b16266d4a8bbaf1cc1
https://github.com/M-Vaittinen/linux/commit/6b4c4ba38b1f838fb0074befd2ca8734604464da

On Wed, Mar 15, 2023 at 12:51:26PM +0200, Matti Vaittinen wrote:

My opinion on that, you should not depend on the others work if they are slow. It might take ages for them to finish and upstream that. Why should you wait?

OTOH you may inform them with your patches that may have a chance to land before theirs. It might motivate them to speed up their work and coordinate the outcome that all stakeholders will be happy.

That said, I would choose option 4), i.e. provide a series where you decouple thingy from DRM and Cc to Stephen with a cover letter note that explains why you chosen this way and what alternatives you are open to.

Quoted 35 lines · Matti Vaittinen

Quoted 34 lines · Andy Shevchenko

Quoted 4 lines · Matti Vaittinen

Quoted 3 lines · Andy Shevchenko

Quoted 2 lines · Jonathan Cameron

On Sun, 12 Mar 2023 17:06:38 +0000
Jonathan Cameron <[email protected]> wrote:

...

Quoted 25 lines

Quoted 5 lines

Quoted 1 line

Ah. I forgot the tests that don't have a device so can't use devm.

Why not? I have seen, IIRC, test cases inside the kernel that fakes the device
for that.

I'd appreciated any pointer for such an example if you have one at hand. (I
can do the digging if you don't though!)

I am not a fan of unit tests. They add huge amount of inertia to development, and in worst case, they stop people from contributing where improving a feature requires test code modification(s). And harder the test
code is to understand, worse the unwanted side-effects. Also, harder the test code is to read, more time and effort it requires to analyze a test failure... Hence, I am _very_ conservative what comes to adding size of test
code with anything that is not strictly required.

After that being said, unit tests are a great tool when carefully used - and
I assume/hope stubbing a device for devm_ tests does not add much extra...
But let me see if I can find an example :)

Quoted 7 lines

Quoted 7 lines

drivers/gpu/drm/tests/drm_managed_test.c ?

(somewhere underneath:

  ret = platform_driver_register(&fake_platform_driver);

which suggests... what exactly? :-)

Thanks to pointer from Andy I found the drm_kunit_helper_[alloc/free]_device() functions. I renamed them to test_kunit_helper_[alloc/free]_device(), move them to drivers/base, add declarations to include/kunit/test-helpers.h fixed KConfigs and existing callers + added the tests for managed interfaces. I have this in place in my personal playground where I am working towards the v4 of the series.

...

After that I asked from Maxime if he had a reason to not make those generic and available to other subsystems besides drm in the first place...

And Maxime was kind enough to point me to the fact that something like this was done in the CCF context:
https://lore.kernel.org/all/[email protected]/

I like the 'single function to get the dummy device which can be passed to devm'-approach used in drm helpers. I do also like Stephen's idea of having the prototypes in kunit/platform_device.h which matches the linux/platform_device.h.

However, I don't know when Stephen's work will be finished and merged to IIO-tree so that it could be used/extended for the needs of these tests.

Meanwhile, I don't think it makes sense to go forward with my changes splitting the helpers out of drm until we see what Stephen's changes will bring us. On the other hand, I don't like delaying the gts-helpers or the sensor drivers.

So, any suggestions what I should do? I see following options:

1) Drop the tests for managed interfaces for now.
2) Add the tests with a yet-another duplicate implementation of the dummy device for devm.
3) Add the tests using the helpers from drm as they are now.

option 1):
I like it as it would be an easy way (for now) - but I hate it as it may be a hard way as well. In my experience, when a driver/helper lands upstream it will get first few fixes quite fast - and not having a test available upstream when this happens is bad. Bad because it means the out-of-tree test may get broken, and bad because there is no easy way to test the fixes.

option 2):
I hate it because it makes the test code more complex - and duplicates the kernel code which is never nice. This could be reworked later when Stephens work is done though.

option 3):
It's in general not nice to use functions exported for some other subsystem's specific purposes. This would however keep the test code at minimum, while leaving the same "I swear I'll fix this later when dependencies have settled" - possibility as option 2) did.

Oh, in theory there is option 4) to just send out the changes I did(*) which pull the drm_kunit_helper_[alloc/free]_device() out of the DRM - but I guess that would lead some extra work to merge this later with stuff Stephen's series does introduce.

Any suggestions which of the options to proceed with?

Quoted 103 lines · Matti Vaittinen

Quoted 35 lines · Matti Vaittinen

Quoted 34 lines · Andy Shevchenko

Quoted 4 lines · Matti Vaittinen

Quoted 3 lines · Andy Shevchenko

Quoted 2 lines · Jonathan Cameron

On Sun, 12 Mar 2023 17:06:38 +0000
Jonathan Cameron <[email protected]> wrote:

...

Quoted 25 lines

Quoted 5 lines

Quoted 1 line

Ah. I forgot the tests that don't have a device so can't use devm.

Why not? I have seen, IIRC, test cases inside the kernel that fakes the device
for that.

I'd appreciated any pointer for such an example if you have one at hand. (I
can do the digging if you don't though!)

I am not a fan of unit tests. They add huge amount of inertia to development, and in worst case, they stop people from contributing where improving a feature requires test code modification(s). And harder the test
code is to understand, worse the unwanted side-effects. Also, harder the test code is to read, more time and effort it requires to analyze a test failure... Hence, I am _very_ conservative what comes to adding size of test
code with anything that is not strictly required.

After that being said, unit tests are a great tool when carefully used - and
I assume/hope stubbing a device for devm_ tests does not add much extra...
But let me see if I can find an example :)

Quoted 7 lines

Quoted 7 lines

drivers/gpu/drm/tests/drm_managed_test.c ?

(somewhere underneath:

  ret = platform_driver_register(&fake_platform_driver);

which suggests... what exactly? :-)

Thanks to pointer from Andy I found the drm_kunit_helper_[alloc/free]_device() functions. I renamed them to test_kunit_helper_[alloc/free]_device(), move them to drivers/base, add declarations to include/kunit/test-helpers.h fixed KConfigs and existing callers + added the tests for managed interfaces. I have this in place in my personal playground where I am working towards the v4 of the series.

...

After that I asked from Maxime if he had a reason to not make those generic and available to other subsystems besides drm in the first place...

And Maxime was kind enough to point me to the fact that something like this was done in the CCF context:
https://lore.kernel.org/all/[email protected]/

I like the 'single function to get the dummy device which can be passed to devm'-approach used in drm helpers. I do also like Stephen's idea of having the prototypes in kunit/platform_device.h which matches the linux/platform_device.h.

However, I don't know when Stephen's work will be finished and merged to IIO-tree so that it could be used/extended for the needs of these tests.

Meanwhile, I don't think it makes sense to go forward with my changes splitting the helpers out of drm until we see what Stephen's changes will bring us. On the other hand, I don't like delaying the gts-helpers or the sensor drivers.

So, any suggestions what I should do? I see following options:

1) Drop the tests for managed interfaces for now.
2) Add the tests with a yet-another duplicate implementation of the dummy device for devm.
3) Add the tests using the helpers from drm as they are now.

option 1):
I like it as it would be an easy way (for now) - but I hate it as it may be a hard way as well. In my experience, when a driver/helper lands upstream it will get first few fixes quite fast - and not having a test available upstream when this happens is bad. Bad because it means the out-of-tree test may get broken, and bad because there is no easy way to test the fixes.

option 2):
I hate it because it makes the test code more complex - and duplicates the kernel code which is never nice. This could be reworked later when Stephens work is done though.

option 3):
It's in general not nice to use functions exported for some other subsystem's specific purposes. This would however keep the test code at minimum, while leaving the same "I swear I'll fix this later when dependencies have settled" - possibility as option 2) did.

Oh, in theory there is option 4) to just send out the changes I did(*) which pull the drm_kunit_helper_[alloc/free]_device() out of the DRM - but I guess that would lead some extra work to merge this later with stuff Stephen's series does introduce.

Quoted 1 line

Any suggestions which of the options to proceed with?

I think the best course of action would be to synchronize with Stephen, and make sure that whatever patch you're doing can be used for his work.

Once it works for both of you, then I guess it can go through the kunit tree and you will use it both.

Quoted 16 lines · On Mon, 6 Mar 2023 11:17:15 +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.

Quoted 7 lines

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

Trying not to duplicate what Andy has raised...

At some stage I want to go through the maths very carefully but it's not happening today and I don't want to delay resolving other remaining comments so that can wait for a later version. I'm sure it's fine but I like to be paranoid :)

Quoted 11 lines

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

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

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

Quoted 6 lines

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

+	if (!itime)
+		return -EINVAL;
+
+	return gain * itime->mul;
+}
+EXPORT_SYMBOL(iio_gts_get_total_gain);

All of them want to be in the namespace.

Quoted 7 lines

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

new file mode 100644
index 000000000000..4b5a417946f4

...

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

Quoted 9 lines

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

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

Given most modern IIO drivers use fully devm_ based probing, for now I would not expose anything else. That will reduce the interface a lot which I think is probably a good thing at this stage.

Keep the non devm stuff internally though as it is a nice structure to have an I can see we may want some of these in non devm form in the future.

Similarly - for now don't expose the individual table building functions as we may never need them in drivers. We (more or less) only support interfaces that are used and so far they aren't.

For other functions it's worth thinking about whether to not export them initially. I haven't been through them all to figure out what is not currently used.

Quoted 11 lines

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

+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

ROHM BU27034 is an ambient light sensor 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. In addition, 3 IIO_INTENSITY channels are emitting the raw register data from all diodes for more intense user-space computations.
- Sensor has GAIN values that can be adjusted from 1x to 4096x.
- Sensor has adjustible measurement times of 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 alone can't provide requested scale, adjusting both the time and the gain is attempted.
- Driver exposes writable INT_TIME property that can be used for adjusting the measurement time. Time adjustment will also cause the driver to try to adjust the GAIN so that the overall scale is kept as close to the original as possible.

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

---

Please note: There are few "unfinished" discussions regarding at least:

- PROCESSED channel scale.

- Implementation details when avoiding division by zero.

I have implemented those for this version in a way that I see the best. It would have been better to wait for discussions to finish - but I will be away from the computer for a week - so I wanted to send out the v3 with fixes so that people who are interested can do a review while I am away.

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

new file mode 100644
index 000000000000..6135a8a2a4f3
@@ -0,0 +1,1501 @@

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

+
+#define BU27034_REG_MODE_CONTROL3	0x43
+#define BU27034_REG_MODE_CONTROL4	0x44
+#define BU27034_MASK_MEAS_EN		BIT(0)

+ * the data read when measurement has finished. Hence we poll the VALID bit in
+ * a thread. We will try to wake the thread BU27034_MEAS_WAIT_PREMATURE_MS
+ * milliseconds before the expected sampling time to prevent the drifting. Eg,
+ * If we constantly wake up a bit too late we would eventually skip a sample.
+ * 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 +3 -0

+#define BU27034_MEAS_WAIT_PREMATURE_MS	5
+#define BU27034_DATA_WAIT_TIME_US	1000
+#define BU27034_TOTAL_DATA_WAIT_TIME_US (BU27034_MEAS_WAIT_PREMATURE_MS * 1000)

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

+				      BIT(IIO_CHAN_INFO_SCALE),
+		.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),

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

+
+static int bu27034_get_gain_sel(struct bu27034_data *data, int chan)
+{
+	int ret, val;
+
+	switch (chan) {
+	case BU27034_CHAN_DATA0:
+	case BU27034_CHAN_DATA1:
+	{

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

+	}
+
+	return -EINVAL;
+}
 
+static int bu27034_get_scale(struct bu27034_data *data, int channel, int *val,
+			      int *val2)
+{
+	int ret;
+
+	if (channel == BU27034_CHAN_ALS) {

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

+
+	return ret;
+}

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

+static int bu27034_calc_mlux(struct bu27034_data *data, __le16 *res, int *val)
+{
+	unsigned int gain0, gain1, meastime;
+	unsigned int d1_d0_ratio_scaled;
+	u16  ch0, ch1;
+	u64 helper64;
+	int ret;
+
+	/*
+	 * We return 0 luxes if calculation fails. This should be reasonably
+	 * easy to spot from the buffers especially if raw-data channels show
+	 * valid values
+	 */
+	*val = 0;
+
+	/*  Avoid div by zero. */

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

+
+static int bu27034_read_raw(struct iio_dev *idev,
+			   struct iio_chan_spec const *chan,
+			   int *val, int *val2, long mask)
+{
+	struct bu27034_data *data = iio_priv(idev);
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_INT_TIME:

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

+		 */
+		if (*val == 50000)
+			*val = 55000;
+
+		return IIO_VAL_INT;
+
+	case IIO_CHAN_INFO_SCALE:
+		return bu27034_get_scale(data, chan->channel, val, val2);
+
+	case IIO_CHAN_INFO_RAW:
+	{
+		if (chan->type != IIO_INTENSITY)
+			return -EINVAL;
+
+		if (chan->channel < BU27034_CHAN_DATA0 ||
+		    chan->channel > BU27034_CHAN_DATA2)

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

+		 * care here. Buffered version should be used if performance is
+		 * an issue.
+		 */
+		ret = bu27034_get_single_result(data, chan->channel, val);
+
+		mutex_unlock(&data->mutex);
+		iio_device_release_direct_mode(idev);
+
+		if (ret)
+			return ret;
+
+		return IIO_VAL_INT;
+	}
+
+	case IIO_CHAN_INFO_PROCESSED:
+		if (chan->type != IIO_LIGHT)
+			return -EINVAL;
+
+		/* Don't mess with measurement enabling while buffering */
+		ret = iio_device_claim_direct_mode(idev);
+		if (ret)
+			return ret;
+
+		mutex_lock(&data->mutex);
+
+		ret = bu27034_get_mlux(data, val);
+
+		mutex_unlock(&data->mutex);
+		iio_device_release_direct_mode(idev);
+
+		if (ret)

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

+}
+
 
 
+
+static int bu27034_wait_for_data(struct bu27034_data *data)
+{
+	int ret, val;

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

+	bu27034_invalidate_read_data(data);
+
+	return ret;
+}
+
+static int bu27034_buffer_thread(void *arg)
+{
+	struct iio_dev *idev = arg;
+	struct bu27034_data *data;
+	int wait_ms;
+
+	data = iio_priv(idev);
+
+	wait_ms = bu27034_get_int_time(data);
+	wait_ms /= 1000;
+
+	wait_ms -=  BU27034_MEAS_WAIT_PREMATURE_MS;
+
+	while (!kthread_should_stop()) {
+		int ret;

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

+			int mlux;
+
+			ret = bu27034_calc_mlux(data, &data->scan.channels[0],
+					       &mlux);
+			if (ret)
+				dev_err(data->dev, "failed to calculate lux\n");
+
+			/*
+			 * The maximum milli lux value we get with gain 1x time
+			 * 55mS data ch0 = 0xffff ch1 = 0xffff fits in 26 bits
+			 * so there should be no problem returning int from
+			 * computations and casting it to u32
+			 */
+			data->scan.mlux = (u32)mlux;
 
+		}
+		iio_push_to_buffers_with_timestamp(idev, &data->scan, tstamp);
+	}
+
+	return 0;
+}

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

+static int bu27034_probe(struct i2c_client *i2c)
+{
+	struct device *dev = &i2c->dev;
+	struct bu27034_data *data;
+	struct regmap *regmap;
+	struct iio_dev *idev;
+	unsigned int part_id, reg;
+	int ret;
+
+	regmap = devm_regmap_init_i2c(i2c, &bu27034_regmap);

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

+		return dev_err_probe(dev, ret, "Failed to get regulator\n");
+

Quoted 31 lines · On Mon, 6 Mar 2023 11:23:50 +0200 Matti Vaittinen <[email protected]>…

ROHM BU27034 is an ambient light sensor 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. In addition, 3 IIO_INTENSITY channels are emitting the raw register data from all diodes for more intense user-space computations.
- Sensor has GAIN values that can be adjusted from 1x to 4096x.
- Sensor has adjustible measurement times of 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 alone can't provide requested scale, adjusting both the time and the gain is attempted.
- Driver exposes writable INT_TIME property that can be used for adjusting the measurement time. Time adjustment will also cause the driver to try to adjust the GAIN so that the overall scale is kept as close to the original as possible.

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

---

Quoted 3 lines

Please note: There are few "unfinished" discussions regarding at least:

- PROCESSED channel scale.

Hopefully the reply in the other thread covered this. It's not PROCESSED as you need to apply the * 1000 so just make it _RAW.

Quoted 7 lines

- Implementation details when avoiding division by zero.

I have implemented those for this version in a way that I see the best. It would have been better to wait for discussions to finish - but I will be away from the computer for a week - so I wanted to send out the v3 with fixes so that people who are interested can do a review while I am away.

I'm getting behind with review anyway so a week delay on this sounds great to me ;) I might get to your other series as a result though don't think I'll get there today.

Quoted 23 lines

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

+static int bu27034_calc_mlux(struct bu27034_data *data, __le16 *res, int *val)
+{
+	unsigned int gain0, gain1, meastime;
+	unsigned int d1_d0_ratio_scaled;
+	u16  ch0, ch1;
+	u64 helper64;
+	int ret;
+
+	/*
+	 * We return 0 luxes if calculation fails. This should be reasonably
+	 * easy to spot from the buffers especially if raw-data channels show
+	 * valid values
+	 */
+	*val = 0;
+
+	/*  Avoid div by zero. */
+	if (!res[0])
+		ch0 = 1;
+	else
+		ch0 = le16_to_cpu(res[0]);
+
+	if (!res[1])

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

Quoted 4 lines

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

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

As per other thread. Really don't like the check on 0 before
the endian conversion. Sure it can be done, but it's a micro optimization that seems unnecessary.

...

Quoted 14 lines

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

+
+static int bu27034_read_raw(struct iio_dev *idev,
+			   struct iio_chan_spec const *chan,
+			   int *val, int *val2, long mask)
+{
+	struct bu27034_data *data = iio_priv(idev);
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_INT_TIME:
+		*val = bu27034_get_int_time(data);
+		if (*val < 0)
+			return *val;
+

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

Quoted 6 lines

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

+		/*
+		 * We use 50000 uS internally for all calculations and only
+		 * convert it to 55000 before returning it to the user.
+		 *
+		 * This is because the data-sheet says the time is 55 mS - but
+		 * vendor provided computations used 50 mS.

No chance of a clarification? would be lovely to not do this!

Quoted 17 lines

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

+		 */
+		if (*val == 50000)
+			*val = 55000;
+
+		return IIO_VAL_INT;
+
+	case IIO_CHAN_INFO_SCALE:
+		return bu27034_get_scale(data, chan->channel, val, val2);
+
+	case IIO_CHAN_INFO_RAW:
+	{
+		if (chan->type != IIO_INTENSITY)
+			return -EINVAL;
+
+		if (chan->channel < BU27034_CHAN_DATA0 ||
+		    chan->channel > BU27034_CHAN_DATA2)

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

Quoted 10 lines

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

+			return -EINVAL;
+
+		/* Don't mess with measurement enabling while buffering */
+		ret = iio_device_claim_direct_mode(idev);
+		if (ret)
+			return ret;
+
+		mutex_lock(&data->mutex);
+		/*
+		 * Reading one channel at a time is ineffiecient but we don't

spell check comments.

Quoted 37 lines

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

+		 * care here. Buffered version should be used if performance is
+		 * an issue.
+		 */
+		ret = bu27034_get_single_result(data, chan->channel, val);
+
+		mutex_unlock(&data->mutex);
+		iio_device_release_direct_mode(idev);
+
+		if (ret)
+			return ret;
+
+		return IIO_VAL_INT;
+	}
+
+	case IIO_CHAN_INFO_PROCESSED:
+		if (chan->type != IIO_LIGHT)
+			return -EINVAL;
+
+		/* Don't mess with measurement enabling while buffering */
+		ret = iio_device_claim_direct_mode(idev);
+		if (ret)
+			return ret;
+
+		mutex_lock(&data->mutex);
+
+		ret = bu27034_get_mlux(data, val);
+
+		mutex_unlock(&data->mutex);
+		iio_device_release_direct_mode(idev);
+
+		if (ret)
+			return ret;
+
+		return IIO_VAL_INT;
+
+	default:
+		return -EINVAL;

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

Quoted 4 lines

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

+
+	}
+
+	return ret;

I would hope you can't get here. If you can fix that so that the lack of return here allows the compiler to know you didn't intend to get here and hence complain about it.

Quoted 18 lines

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

+static int bu27034_probe(struct i2c_client *i2c)
+{
+	struct device *dev = &i2c->dev;
+	struct bu27034_data *data;
+	struct regmap *regmap;
+	struct iio_dev *idev;
+	unsigned int part_id, reg;
+	int ret;
+
+	regmap = devm_regmap_init_i2c(i2c, &bu27034_regmap);
+	if (IS_ERR(regmap))
+		return dev_err_probe(dev, PTR_ERR(regmap),
+				     "Failed to initialize Regmap\n");
+
+	idev = devm_iio_device_alloc(dev, sizeof(*data));
+	if (!idev)
+		return -ENOMEM;

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

Quoted 3 lines

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

+
+	ret = devm_regulator_get_enable(dev, "vdd");
+	if (ret && ret != -ENODEV)

Why the special -ENODEV handling?
You should get a stub regulator if one isn't provided by firmware. If you don't get a stub, or a real regulator that's a failure so we should return the error code and fail the probe.

Support ROHM BU27034 ALS sensor

This series adds support for ROHM BU27034 Ambient Light Sensor.

The BU27034 has configurable gain and measurement (integration) time settings. Both of these have inversely proportional relation to the sensor's intensity channel scale.

Many users only set the scale, which means that many drivers attempt to 'guess' the best gain+time combination to meet the scale. Usually this is the biggest integration time which allows setting the requested scale. Typically, increasing the integration time has better accuracy than increasing the gain, which often amplifies the noise as well as the real signal.

However, there may be cases where more responsive sensors are needed. So, in some cases the longest integration times may not be what the user prefers. The driver has no way of knowing this.

Hence, the approach taken by this series is to allow user to set both the scale and the integration time with following logic:

1. When scale is set, the existing integration time is tried to be maintained as a first priority.
1a) If the requested scale can't be met by current time, then also
other time + gain combinations are searched. If scale can be met by some other integration time, then the new time may be applied. If the time setting is common for all channels, then also other channels must be able to maintain their scale with this new time (by changing their gain). The new times are scanned in the order of preference (typically the longest times first).
1b) If the requested scale can be met using current time, then only
the gain for the channel is changed.

2. When the integration time change - scale is tried to be maintained. When integration time change is requested also gain for all impacted channels is adjusted so that the scale is not changed, or is chaned as little as possible. This is different from the RFCv1 where the request was rejected if suitable gain couldn't be found for some channel(s).

This logic is simple. When total gain (either caused by time or hw-gain) is doubled, the scale gets halved. Also, the supported times are given a 'multiplier' value which tells how much they increase the total gain.

However, when I wrote this logic in bu27034 driver, I made quite a few errors on the way - and driver got pretty big. As I am writing drivers for two other sensors (RGB C/IR + flicker BU27010 and RGB C/IR BU27008) with similar gain-time-scale logic I thought that adding common helpers for these computations might be wise. I hope this way all the bugs will be concentrated in one place and not in every individual driver ;)

Hence, this series also intriduces IIO gain-time-scale helpers (abbreviated as gts-helpers) + a couple of KUnit tests for the most hairy parts.

I can't help thinking that there should've been simpler way of computing the gain-time-scale conversions. Also, pretty good speed improvements might be available if some of the do_div()s could be replaced by >>. This, however, is not a priority for my light-sensor use-case where speed demands are not that big.

Finally, these added helpers do provide some value also for drivers which only:
a) allow gain change
or
b) allow changing both the time and gain but so that the time-change is not reflected in register values.

For a) we provide the gain - selector (register value) table format + selector to gain look-ups, gain <-> scale conversions and the available scales helpers.

For latter case we also provide the time-tables, and actually all the APIs should be usable by setting the time multiplier to 1. (not testeted thoroughly though).