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

  Some light sensors can adjust both the HW-gain and integration time.
There are cases where adjusting the integration time has similar impact
to the scale of the reported values as gain setting has.

IIO users do typically expect to handle scale by a single writable 'scale'
entry. Driver should then adjust the gain/time accordingly.

It however is difficult for a driver to know whether it should change
gain or integration time to meet the requested scale. Usually it is
preferred to have longer integration time which usually improves
accuracy, but there may be use-cases where long measurement times can be
an issue. Thus it can be preferable to allow also changing the
integration time - but mitigate the scale impact by also changing the gain
underneath. Eg, if integration time change doubles the measured values,
the driver can reduce the HW-gain to half.

The theory of the computations of gain-time-scale is simple. However,
some people (undersigned) got that implemented wrong for more than once.

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

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>

---
Currently it is only BU27034 using these in this series. I am however working
with drivers for RGB sensors BU27008 and BU27010 which have similar
[gain - integration time - scale] - relation. I hope sending those
follows soon after the BU27034 is done.

Changes:
v2 => v3: (mostly fixes based on review by Andy)
- Fix typos
- Styling fixes
- Use namespace for exported symbols
- Protect allocs against argument overflow
- Fix include protection name
- add types.h inclusion and struct device forward declaration

RFCv1 => v2:
- fix include guardian
- Improve kernel doc for iio_init_iio_gts.
- Add iio_gts_scale_to_total_gain
- Add iio_gts_total_gain_to_scale
- Fix review comments from Jonathan
  - add documentation to few functions
  - replace 0xffffffffffffffffLLU by U64_MAX
  - some styling fixes
  - drop unnecessary NULL checks
  - order function arguments by  in / out purpose
  - drop GAIN_SCALE_ITIME_MS()
- Add helpers for available scales and times
- Rename to iio-gts-helpers
---
 drivers/iio/light/Kconfig          |    3 +
 drivers/iio/light/Makefile         |    1 +
 drivers/iio/light/iio-gts-helper.c | 1158 ++++++++++++++++++++++++++++
 drivers/iio/light/iio-gts-helper.h |  134 ++++
 4 files changed, 1296 insertions(+)
 create mode 100644 drivers/iio/light/iio-gts-helper.c
 create mode 100644 drivers/iio/light/iio-gts-helper.h
  

On Mon, Mar 06, 2023 at 11:17:15AM +0200, Matti Vaittinen wrote:
> Some light sensors can adjust both the HW-gain and integration time.
> There are cases where adjusting the integration time has similar impact
> to the scale of the reported values as gain setting has.
> 
> IIO users do typically expect to handle scale by a single writable 'scale'
> entry. Driver should then adjust the gain/time accordingly.
> 
> It however is difficult for a driver to know whether it should change
> gain or integration time to meet the requested scale. Usually it is
> preferred to have longer integration time which usually improves
> accuracy, but there may be use-cases where long measurement times can be
> an issue. Thus it can be preferable to allow also changing the
> integration time - but mitigate the scale impact by also changing the gain
> underneath. Eg, if integration time change doubles the measured values,
> the driver can reduce the HW-gain to half.
> 
> The theory of the computations of gain-time-scale is simple. However,
> some people (undersigned) got that implemented wrong for more than once.
> 
> Add some gain-time-scale helpers in order to not dublicate errors in all
> drivers needing these computations.

...

> +/*

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

> + * iio_gts_get_gain - Convert scale to total gain
> + *
> + * Internal helper for converting scale to total gain.
> + *
> + * @max:	Maximum linearized scale. As an example, when scale is created
> + *		in magnitude of NANOs and max scale is 64.1 - The linearized
> + *		scale is 64 100 000 000.
> + * @scale:	Linearized scale to compte the gain for.
> + *
> + * Return:	(floored) gain corresponding to the scale. -EINVAL if scale
> + *		is invalid.
> + */
> +static int iio_gts_get_gain(const u64 max, const u64 scale)
> +{
> +	int tmp = 1;
> +
> +	if (scale > max || !scale)
> +		return -EINVAL;
> +
> +	if (U64_MAX - max < scale) {
> +		/* Risk of overflow */
> +		if (max - scale < scale)
> +			return 1;

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

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

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

No space for castings?

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

...

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

Missing period.

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

Shouldn't be OVERFLOW for the first one?

...

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

No space for casting?

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

...

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

...

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

...

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

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

> +		 */

...

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

One line reads better?

...

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

> +		kfree(gts->avail_all_scales_table);

...

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

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

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

Ditto.

> +	if (!per_time_scales)
> +		goto free_gains;

...

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

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

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

...

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

	while (i--) {

makes it easier to parse.

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

> + */

...

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

Missing period.

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

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

> +		return -EINVAL;

...

> +	for (i = 0; i < gts->num_hwgain; i++) {
> +		/*
> +		 * It is not expected this function is called for an exactly
> +		 * matching gain.
> +		 */
> +		if (unlikely(gain == gts->hwgain_table[i].gain)) {
> +			*in_range = true;
> +			return gain;
> +		}

> +		if (!min)
> +			min = gts->hwgain_table[i].gain;
> +		else
> +			min = min(min, gts->hwgain_table[i].gain);

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

> +		if (gain > gts->hwgain_table[i].gain) {
> +			if (!diff) {
> +				diff = gain - gts->hwgain_table[i].gain;
> +				best = i;
> +			} else {
> +				int tmp = gain - gts->hwgain_table[i].gain;
> +
> +				if (tmp < diff) {
> +					diff = tmp;
> +					best = i;
> +				}
> +			}

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

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

?

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

> +		} else {
> +			/*
> +			 * We found valid hwgain which is greater than
> +			 * reference. So, unless we return a failure below we
> +			 * will have found an in-range gain
> +			 */
> +			*in_range = true;
> +		}
> +	}
> +	/* The requested gain was smaller than anything we support */
> +	if (!diff) {
> +		*in_range = false;
> +
> +		return -EINVAL;
> +	}
> +
> +	return gts->hwgain_table[best].gain;

...

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

Still can be one line.

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

Ditto.

> +	if (ret)
> +		return -EINVAL;

...

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

Is it _only_ for a Light type of sensors?

...

> +#ifndef __IIO_GTS_HELPER__
> +#define __IIO_GTS_HELPER__

If yes, perhaps adding LIGHT here?
  

On Mon, 6 Mar 2023 14:52:57 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Mon, Mar 06, 2023 at 11:17:15AM +0200, Matti Vaittinen wrote:
> > Some light sensors can adjust both the HW-gain and integration time.
> > There are cases where adjusting the integration time has similar impact
> > to the scale of the reported values as gain setting has.
> > 
> > IIO users do typically expect to handle scale by a single writable 'scale'
> > entry. Driver should then adjust the gain/time accordingly.
> > 
> > It however is difficult for a driver to know whether it should change
> > gain or integration time to meet the requested scale. Usually it is
> > preferred to have longer integration time which usually improves
> > accuracy, but there may be use-cases where long measurement times can be
> > an issue. Thus it can be preferable to allow also changing the
> > integration time - but mitigate the scale impact by also changing the gain
> > underneath. Eg, if integration time change doubles the measured values,
> > the driver can reduce the HW-gain to half.
> > 
> > The theory of the computations of gain-time-scale is simple. However,
> > some people (undersigned) got that implemented wrong for more than once.
> > 
> > Add some gain-time-scale helpers in order to not dublicate errors in all
> > drivers needing these computations.  
> 
> ...
> 
> > +/*  
> 
> If it's deliberately not a kernel doc, why to bother to have it looking as one?
> It's really a provocative to some people who will come with a patches to "fix"
> this...

Just make it kernel-doc.

> 
> > + * iio_gts_get_gain - Convert scale to total gain
> > + *
> > + * Internal helper for converting scale to total gain.
> > + *
> > + * @max:	Maximum linearized scale. As an example, when scale is created
> > + *		in magnitude of NANOs and max scale is 64.1 - The linearized
> > + *		scale is 64 100 000 000.
> > + * @scale:	Linearized scale to compte the gain for.
> > + *
> > + * Return:	(floored) gain corresponding to the scale. -EINVAL if scale
> > + *		is invalid.
> > + */

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





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

On Mon, 6 Mar 2023 11:17:15 +0200
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> Some light sensors can adjust both the HW-gain and integration time.
> There are cases where adjusting the integration time has similar impact
> to the scale of the reported values as gain setting has.
> 
> IIO users do typically expect to handle scale by a single writable 'scale'
> entry. Driver should then adjust the gain/time accordingly.
> 
> It however is difficult for a driver to know whether it should change
> gain or integration time to meet the requested scale. Usually it is
> preferred to have longer integration time which usually improves
> accuracy, but there may be use-cases where long measurement times can be
> an issue. Thus it can be preferable to allow also changing the
> integration time - but mitigate the scale impact by also changing the gain
> underneath. Eg, if integration time change doubles the measured values,
> the driver can reduce the HW-gain to half.
> 
> The theory of the computations of gain-time-scale is simple. However,
> some people (undersigned) got that implemented wrong for more than once.
> 
> Add some gain-time-scale helpers in order to not dublicate errors in all
> drivers needing these computations.
> 
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>

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

> +int iio_gts_get_total_gain(struct iio_gts *gts, int gain, int time)
> +{
> +	const struct iio_itime_sel_mul *itime;
> +
> +	if (!iio_gts_valid_gain(gts, gain))
> +		return -EINVAL;
> +
> +	if (!gts->num_itime)
> +		return gain;
> +
> +	itime = iio_gts_find_itime_by_time(gts, time);
> +	if (!itime)
> +		return -EINVAL;
> +
> +	return gain * itime->mul;
> +}
> +EXPORT_SYMBOL(iio_gts_get_total_gain);

All of them want to be in the namespace.



> diff --git a/drivers/iio/light/iio-gts-helper.h b/drivers/iio/light/iio-gts-helper.h
> new file mode 100644
> index 000000000000..4b5a417946f4
> --- /dev/null
> +++ b/drivers/iio/light/iio-gts-helper.h

...

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

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

On Sun, 12 Mar 2023 17:06:38 +0000
Jonathan Cameron <jic23@kernel.org> wrote:

> On Mon, 6 Mar 2023 11:17:15 +0200
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
> > Some light sensors can adjust both the HW-gain and integration time.
> > There are cases where adjusting the integration time has similar impact
> > to the scale of the reported values as gain setting has.
> > 
> > IIO users do typically expect to handle scale by a single writable 'scale'
> > entry. Driver should then adjust the gain/time accordingly.
> > 
> > It however is difficult for a driver to know whether it should change
> > gain or integration time to meet the requested scale. Usually it is
> > preferred to have longer integration time which usually improves
> > accuracy, but there may be use-cases where long measurement times can be
> > an issue. Thus it can be preferable to allow also changing the
> > integration time - but mitigate the scale impact by also changing the gain
> > underneath. Eg, if integration time change doubles the measured values,
> > the driver can reduce the HW-gain to half.
> > 
> > The theory of the computations of gain-time-scale is simple. However,
> > some people (undersigned) got that implemented wrong for more than once.
> > 
> > Add some gain-time-scale helpers in order to not dublicate errors in all
> > drivers needing these computations.
> > 
> > Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>  
> 
> 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 :)
> 
> > +int iio_gts_get_total_gain(struct iio_gts *gts, int gain, int time)
> > +{
> > +	const struct iio_itime_sel_mul *itime;
> > +
> > +	if (!iio_gts_valid_gain(gts, gain))
> > +		return -EINVAL;
> > +
> > +	if (!gts->num_itime)
> > +		return gain;
> > +
> > +	itime = iio_gts_find_itime_by_time(gts, time);
> > +	if (!itime)
> > +		return -EINVAL;
> > +
> > +	return gain * itime->mul;
> > +}
> > +EXPORT_SYMBOL(iio_gts_get_total_gain);  
> 
> All of them want to be in the namespace.
> 
> 
> 
> > diff --git a/drivers/iio/light/iio-gts-helper.h b/drivers/iio/light/iio-gts-helper.h
> > new file mode 100644
> > index 000000000000..4b5a417946f4
> > --- /dev/null
> > +++ b/drivers/iio/light/iio-gts-helper.h  
> 
> ...
> 
> > +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.
> 
Ah. I forgot the tests that don't have a device so can't use devm.

Ah well I guess we have to keep some of the other cases.


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

On Sun, Mar 12, 2023 at 05:08:48PM +0000, Jonathan Cameron wrote:
> On Sun, 12 Mar 2023 17:06:38 +0000
> Jonathan Cameron <jic23@kernel.org> wrote:
> > On Mon, 6 Mar 2023 11:17:15 +0200
> > Matti Vaittinen <mazziesaccount@gmail.com> wrote:

...

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

On 3/6/23 14:52, Andy Shevchenko wrote:
> On Mon, Mar 06, 2023 at 11:17:15AM +0200, Matti Vaittinen wrote:
>> Some light sensors can adjust both the HW-gain and integration time.
>> There are cases where adjusting the integration time has similar impact
>> to the scale of the reported values as gain setting has.
>>
>> IIO users do typically expect to handle scale by a single writable 'scale'
>> entry. Driver should then adjust the gain/time accordingly.
>>
>> It however is difficult for a driver to know whether it should change
>> gain or integration time to meet the requested scale. Usually it is
>> preferred to have longer integration time which usually improves
>> accuracy, but there may be use-cases where long measurement times can be
>> an issue. Thus it can be preferable to allow also changing the
>> integration time - but mitigate the scale impact by also changing the gain
>> underneath. Eg, if integration time change doubles the measured values,
>> the driver can reduce the HW-gain to half.
>>
>> The theory of the computations of gain-time-scale is simple. However,
>> some people (undersigned) got that implemented wrong for more than once.
>>
>> Add some gain-time-scale helpers in order to not dublicate errors in all
>> drivers needing these computations.
> 
> ...
> 
>> +/*
> 
> If it's deliberately not a kernel doc, why to bother to have it looking as one?
> It's really a provocative to some people who will come with a patches to "fix"
> this...

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

> 
>> + * iio_gts_get_gain - Convert scale to total gain
>> + *
>> + * Internal helper for converting scale to total gain.
>> + *
>> + * @max:	Maximum linearized scale. As an example, when scale is created
>> + *		in magnitude of NANOs and max scale is 64.1 - The linearized
>> + *		scale is 64 100 000 000.
>> + * @scale:	Linearized scale to compte the gain for.
>> + *
>> + * Return:	(floored) gain corresponding to the scale. -EINVAL if scale
>> + *		is invalid.
>> + */
>> +static int iio_gts_get_gain(const u64 max, const u64 scale)
>> +{
>> +	int tmp = 1;
>> +
>> +	if (scale > max || !scale)
>> +		return -EINVAL;
>> +
>> +	if (U64_MAX - max < scale) {
>> +		/* Risk of overflow */
>> +		if (max - scale < scale)
>> +			return 1;
> 
>> +		while (max - scale > scale * (u64)tmp)
>> +			tmp++;
>> +
>> +		return tmp + 1;
> 
> Can you wait for the comments to appear a bit longer, please?
> I have answered to your query in the previous discussion.
> 

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

>> +	}
>> +
>> +	while (max > scale * (u64) tmp)
> 
> No space for castings?

Thanks,

> 
>> +		tmp++;
>> +
>> +	return tmp;
>> +}
> 
> ...
> 
>> +	/*
>> +	 * Expect scale to be (mostly) NANO or MICRO. Divide divider instead of
>> +	 * multiplication followed by division to avoid overflow
> 
> Missing period.
> 
>> +	 */
>> +	if (scaler > NANO || !scaler)
>> +		return -EINVAL;
> 
> Shouldn't be OVERFLOW for the first one?

-EOVERFLOW? I guess it could be. Thanks.

> ...
> 
>> +	*lin_scale = (u64) scale_whole * (u64)scaler +
> 
> No space for casting?

Yes. Thanks

> ...
> 
>> +	ret = iio_gts_linearize(max_scale_int, max_scale_nano, NANO,
>> +				   &gts->max_scale);
>> +	if (ret)
>> +		return ret;
>> +
>> +	gts->hwgain_table = gain_tbl;
>> +	gts->num_hwgain = num_gain;
>> +	gts->itime_table = tim_tbl;
>> +	gts->num_itime = num_times;
>> +	gts->per_time_avail_scale_tables = NULL;
>> +	gts->avail_time_tables = NULL;
>> +	gts->avail_all_scales_table = NULL;
>> +	gts->num_avail_all_scales = 0;
> 
> Just wondering why we can't simply
> 
> 	memset(0)
> 
> beforehand and drop all these 0 assignments?
> 

I guess we can :)

> ...
> 
>> +		/*
>> +		 * Sort the tables for nice output and for easier finding of
>> +		 * unique values
> 
> Missing period. Please, check the style of multi-line comments. I believe it's
> even mentioned in the documentation.
> 
>> +		 */
> 
> ...
> 
>> +		sort(gains[i], gts->num_hwgain, sizeof(int), iio_gts_gain_cmp,
>> +		     NULL);
> 
> One line reads better?

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

> 
> ...
> 
>> +	if (ret && gts->avail_all_scales_table)
> 
> In one case you commented that free(NULL) is okay, in the other, you add
> a duplicative check. Why?

Sorry but what do you mean by dublicative check?

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

> 
>> +		kfree(gts->avail_all_scales_table);
> 
> ...
> 
>> +	per_time_gains = kcalloc(gts->num_itime, sizeof(int *), GFP_KERNEL);
> 
> sizeof(type) is error prone in comparison to sizeof(*var).

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

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

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

> 
>> +	if (!per_time_gains)
>> +		return ret;
>> +
>> +	per_time_scales = kcalloc(gts->num_itime, sizeof(int *), GFP_KERNEL);
> 
> Ditto.
> 
>> +	if (!per_time_scales)
>> +		goto free_gains;
> 
> ...
> 
>> +err_free_out:
>> +	while (i) {
>> +		/*
>> +		 * It does not matter if i'th alloc was not succesfull as
>> +		 * kfree(NULL) is safe.
>> +		 */
> 
> Instead, can be just a free of the known allocated i:th member first followed
> by traditional pattern. In that case comment will become redundant.
>

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


>> +		kfree(per_time_scales[i]);
>> +		kfree(per_time_gains[i]);
>> +
>> +		i--;
>> +	}
> 
> ...
> 
>> +	for (i = gts->num_itime - 1; i >= 0; i--) {
> 
> 	while (i--) {
> 
> makes it easier to parse.

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

> 
>> +/**
>> + * iio_gts_all_avail_scales - helper for listing all available scales
>> + * @gts:	Gain time scale descriptor
>> + * @vals:	Returned array of supported scales
>> + * @type:	Type of returned scale values
>> + * @length:	Amount of returned values in array
>> + *
>> + * Returns a value suitable to be returned from read_avail or a negative error
> 
> Missing a return section. Have you run kernel doc to validate this?
No. I think I have never run the kernel doc - probably time to do so :) 
Thanks.

> Missing period.
> 
> Seems these problems occur in many function descriptions.
> 
>> + */
> 
> ...
> 
>> +	/*
>> +	 * Using this function prior building the tables is a driver-error
>> +	 * which should be fixed when the driver is tested for a first time
> 
> Missing period.
> 
>> +	 */
>> +	if (WARN_ON(!gts->num_avail_all_scales))
> 
> Does this justify panic? Note, that any WARN() can become an Oops followed by
> panic and reboot.

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

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

> 
>> +		return -EINVAL;
> 
> ...
> 
>> +	for (i = 0; i < gts->num_hwgain; i++) {
>> +		/*
>> +		 * It is not expected this function is called for an exactly
>> +		 * matching gain.
>> +		 */
>> +		if (unlikely(gain == gts->hwgain_table[i].gain)) {
>> +			*in_range = true;
>> +			return gain;
>> +		}
> 
>> +		if (!min)
>> +			min = gts->hwgain_table[i].gain;
>> +		else
>> +			min = min(min, gts->hwgain_table[i].gain);
> 
> I was staring at this and have got no clue why it's not a dead code.

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

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

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

> 
>> +		if (gain > gts->hwgain_table[i].gain) {
>> +			if (!diff) {
>> +				diff = gain - gts->hwgain_table[i].gain;
>> +				best = i;
>> +			} else {
>> +				int tmp = gain - gts->hwgain_table[i].gain;
>> +
>> +				if (tmp < diff) {
>> +					diff = tmp;
>> +					best = i;
>> +				}
>> +			}
> 
> 			int tmp = gain - gts->hwgain_table[i].gain;
> 
> 			if (!diff || tmp < diff) {
> 				diff = tmp;
> 				best = i;
> 			}
> 
> ?
> 
> Or did you miss using 'min'? (And I'm wondering how variable name and min()
> macro are not conflicting with each other.
> 
>> +		} else {
>> +			/*
>> +			 * We found valid hwgain which is greater than
>> +			 * reference. So, unless we return a failure below we
>> +			 * will have found an in-range gain
>> +			 */
>> +			*in_range = true;
>> +		}
>> +	}
>> +	/* The requested gain was smaller than anything we support */
>> +	if (!diff) {
>> +		*in_range = false;
>> +
>> +		return -EINVAL;
>> +	}
>> +
>> +	return gts->hwgain_table[best].gain;
> 
> ...
> 
>> +	ret = iio_gts_get_scale_linear(gts, old_gain, itime_old->time_us,
>> +				       &scale);
> 
> Still can be one line.
> 
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = gain_get_scale_fraction(gts->max_scale, scale, itime_new->mul,
>> +				      new_gain);
> 
> Ditto.

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


>> +	if (ret)
>> +		return -EINVAL;
> 
> ...
> 
>> +++ b/drivers/iio/light/iio-gts-helper.h
> 
> Is it _only_ for a Light type of sensors?

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

> 
> ...
> 
>> +#ifndef __IIO_GTS_HELPER__
>> +#define __IIO_GTS_HELPER__
> 
> If yes, perhaps adding LIGHT here?

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

Yours,
	-- Matti
  

On 3/12/23 19:06, Jonathan Cameron wrote:
> On Mon, 6 Mar 2023 11:17:15 +0200
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> Some light sensors can adjust both the HW-gain and integration time.
>> There are cases where adjusting the integration time has similar impact
>> to the scale of the reported values as gain setting has.
>>
>> IIO users do typically expect to handle scale by a single writable 'scale'
>> entry. Driver should then adjust the gain/time accordingly.
>>
>> It however is difficult for a driver to know whether it should change
>> gain or integration time to meet the requested scale. Usually it is
>> preferred to have longer integration time which usually improves
>> accuracy, but there may be use-cases where long measurement times can be
>> an issue. Thus it can be preferable to allow also changing the
>> integration time - but mitigate the scale impact by also changing the gain
>> underneath. Eg, if integration time change doubles the measured values,
>> the driver can reduce the HW-gain to half.
>>
>> The theory of the computations of gain-time-scale is simple. However,
>> some people (undersigned) got that implemented wrong for more than once.
>>
>> Add some gain-time-scale helpers in order to not dublicate errors in all
>> drivers needing these computations.
>>
>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> 
> 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 :)
> 

This is more than welcome! I tried to add some test cases for verifying 
some parts - but extra pair of eyes is always more than appreciated! 
I've written and read way too many bugs to not appreciate a healthy 
amount of paranoia :)

>> +int iio_gts_get_total_gain(struct iio_gts *gts, int gain, int time)
>> +{
>> +	const struct iio_itime_sel_mul *itime;
>> +
>> +	if (!iio_gts_valid_gain(gts, gain))
>> +		return -EINVAL;
>> +
>> +	if (!gts->num_itime)
>> +		return gain;
>> +
>> +	itime = iio_gts_find_itime_by_time(gts, time);
>> +	if (!itime)
>> +		return -EINVAL;
>> +
>> +	return gain * itime->mul;
>> +}
>> +EXPORT_SYMBOL(iio_gts_get_total_gain);
> 
> All of them want to be in the namespace.

Seems like I accidentally did not use the EXPORT_SYMBOL_GPL for this 
one. It must thus have evaded my conversion to name space one. Thanks!

> 
> 
>> diff --git a/drivers/iio/light/iio-gts-helper.h b/drivers/iio/light/iio-gts-helper.h
>> new file mode 100644
>> index 000000000000..4b5a417946f4
>> --- /dev/null
>> +++ b/drivers/iio/light/iio-gts-helper.h
> 
> ...
> 
>> +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.
> 
>> +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
>
  

On 3/12/23 18:51, Jonathan Cameron wrote:
> On Mon, 6 Mar 2023 14:52:57 +0200
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> 
>> On Mon, Mar 06, 2023 at 11:17:15AM +0200, Matti Vaittinen wrote:
>>> Some light sensors can adjust both the HW-gain and integration time.
>>> There are cases where adjusting the integration time has similar impact
>>> to the scale of the reported values as gain setting has.
>>>
>>> IIO users do typically expect to handle scale by a single writable 'scale'
>>> entry. Driver should then adjust the gain/time accordingly.
>>>
>>> It however is difficult for a driver to know whether it should change
>>> gain or integration time to meet the requested scale. Usually it is
>>> preferred to have longer integration time which usually improves
>>> accuracy, but there may be use-cases where long measurement times can be
>>> an issue. Thus it can be preferable to allow also changing the
>>> integration time - but mitigate the scale impact by also changing the gain
>>> underneath. Eg, if integration time change doubles the measured values,
>>> the driver can reduce the HW-gain to half.
>>>
>>> The theory of the computations of gain-time-scale is simple. However,
>>> some people (undersigned) got that implemented wrong for more than once.
>>>
>>> Add some gain-time-scale helpers in order to not dublicate errors in all
>>> drivers needing these computations.
>>
>> ...
>>
>>> +/*
>>
>> If it's deliberately not a kernel doc, why to bother to have it looking as one?
>> It's really a provocative to some people who will come with a patches to "fix"
>> this...
> 
> 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...

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

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

Ok. I'll do the move for the next version.

Yours,
	-- Matti

> Jonathan
  

On 3/13/23 14:40, Andy Shevchenko wrote:
> On Sun, Mar 12, 2023 at 05:08:48PM +0000, Jonathan Cameron wrote:
>> On Sun, 12 Mar 2023 17:06:38 +0000
>> Jonathan Cameron <jic23@kernel.org> wrote:
>>> On Mon, 6 Mar 2023 11:17:15 +0200
>>> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
> ...
> 
>>> 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.

Probably at any 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.

Ok. I was pondering this while writing these APIs. I was just thinking 
that _maybe_ someone has an driver where they do not use devm for a 
reason. Allowing a "non devm" variants for such is likely to be needed. 
Hence, I was thinking that having a non devm version could be beneficial 
from the start to avoid someone being tempted to just mix the readily 
available devm with manual unwinding...

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

I was thinking of this too. It was just the small 'avoid extra 
operations [like unnecessary endianess conversions :p] when 
needed'-voice in me that started screaming when I though of exporting 
only the 'build all' and 'purge all' APIs...

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

I think I can go through them. There are a few that aren't currently used.

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

Yours,
	-- Matti
  

On Mon, Mar 13, 2023 at 02:56:59PM +0200, Matti Vaittinen wrote:
> On 3/12/23 18:51, Jonathan Cameron wrote:
> > On Mon, 6 Mar 2023 14:52:57 +0200
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > > On Mon, Mar 06, 2023 at 11:17:15AM +0200, Matti Vaittinen wrote:

...

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

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

On Mon, Mar 13, 2023 at 02:47:45PM +0200, Matti Vaittinen wrote:
> On 3/6/23 14:52, Andy Shevchenko wrote:
> > On Mon, Mar 06, 2023 at 11:17:15AM +0200, Matti Vaittinen wrote:

...

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

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

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

...

> > > +	if (scaler > NANO || !scaler)
> > > +		return -EINVAL;
> > 
> > Shouldn't be OVERFLOW for the first one?
> 
> -EOVERFLOW? I guess it could be. Thanks.

Yes.

...

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

...

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

> > > +		kfree(gts->avail_all_scales_table);

...

> > > +	per_time_gains = kcalloc(gts->num_itime, sizeof(int *), GFP_KERNEL);
> > 
> > sizeof(type) is error prone in comparison to sizeof(*var).
> 
> Yes and no. In majority of cases where we see sizeof(*var) - the *var is no
> longer a pointer as having pointers to pointers is not _that_ common. When
> we see sizeof(type *) - we instantly know it is a size of a pointer and not
> a size of some other type.
> 
> So yes, while having sizeof(*var) makes us tolerant to errors caused by
> variable type changes - it makes us prone to human reader errors. Also, if
> someone changes type of *var from pointer to some other type - then he/she
> is likely to in any case need to revise the array alloactions too.
> 
> While I in general agree with you that the sizeof(variable) is better than
> sizeof(type) - I see that in cases like this the sizeof(type *) is clearer.

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.

...

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

Ditto.

...

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

You can have repeated it here :-)

Yes, two labels and one kfree() makes the comment go away. To me that comment
is more confusing than helping.

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

...

> > > +	for (i = gts->num_itime - 1; i >= 0; i--) {
> > 
> > 	while (i--) {
> > 
> > makes it easier to parse.
> 
> This is also something I replied for v2. I think we have a fundamental
> disagreement on this one :/

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

...

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

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.
  

On Mon, Mar 13, 2023 at 03:11:52PM +0200, Matti Vaittinen wrote:
> On 3/13/23 14:40, Andy Shevchenko wrote:
> > On Sun, Mar 12, 2023 at 05:08:48PM +0000, Jonathan Cameron wrote:
> > > On Sun, 12 Mar 2023 17:06:38 +0000
> > > Jonathan Cameron <jic23@kernel.org> wrote:

...

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

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

(somewhere underneath:

 ret = platform_driver_register(&fake_platform_driver);

which suggests... what exactly? :-)
  

On 3/13/23 15:25, Andy Shevchenko wrote:
> On Mon, Mar 13, 2023 at 02:47:45PM +0200, Matti Vaittinen wrote:
>> On 3/6/23 14:52, Andy Shevchenko wrote:
>>> On Mon, Mar 06, 2023 at 11:17:15AM +0200, Matti Vaittinen wrote:
> 
> ...
> 
>>>> +/*
>>>
>>> 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".
> 
> The problem as I pointed out with your approach it's unmaintainable. And
> I even explained why I consider it this way.

Yes. You told me that it asks for people to turn it to kernel doc. If 
that happens, apply the patch and it is kernel doc. I don't see how 
unmaintainable it is. I think this is just creating a problem we may 
never face - and if we do, we can solve it by applying the 'problem' then.

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

Trusting suggestions like this in a kernel code would be a big problem 
to me. I would ask myself - "do you feel lucky"?

Well, my favourite editor would wrap the line - so I would see the NULL 
at the next row. Not indented properly causing it to be harder to read 
than the code which is properly manually split and indented. It is much 
less of a problem for me to "waste" a row here and see the line properly 
split.

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

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

> 
>>>> +		kfree(gts->avail_all_scales_table);
> 
> ...
> 
>>>> +	per_time_gains = kcalloc(gts->num_itime, sizeof(int *), GFP_KERNEL);
>>>
>>> sizeof(type) is error prone in comparison to sizeof(*var).
>>
>> Yes and no. In majority of cases where we see sizeof(*var) - the *var is no
>> longer a pointer as having pointers to pointers is not _that_ common. When
>> we see sizeof(type *) - we instantly know it is a size of a pointer and not
>> a size of some other type.
>>
>> So yes, while having sizeof(*var) makes us tolerant to errors caused by
>> variable type changes - it makes us prone to human reader errors. Also, if
>> someone changes type of *var from pointer to some other type - then he/she
>> is likely to in any case need to revise the array alloactions too.
>>
>> While I in general agree with you that the sizeof(variable) is better than
>> sizeof(type) - I see that in cases like this the sizeof(type *) is clearer.
> 
> 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 :)

> ...
> 
>>>> +	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 :/
> 
> Yes, and I will continue insisting on while (foo--).
> That why I won't give you my tags :-)

Well, I am planning to keep reading this code when/if it is being 
patched. Hence I am so reluctant to change it to something that makes it 
harder for me to follow. Meanwhile, I understand that you don't want to 
tag something you don't agree with.

> ...
> 
>>>> +		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!
> 
> Hint: run always `make W=1` when building kernel.

Ah. I thought I had that and sparse enabled. It seems I disabled all 
extra checks from my build scripts a while ago to speed-up compilation 
when I was bisecting...

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

Thanks for the review.

--Matti
  

On 3/13/23 15:29, Andy Shevchenko wrote:
> On Mon, Mar 13, 2023 at 03:11:52PM +0200, Matti Vaittinen wrote:
>> On 3/13/23 14:40, Andy Shevchenko wrote:
>>> On Sun, Mar 12, 2023 at 05:08:48PM +0000, Jonathan Cameron wrote:
>>>> On Sun, 12 Mar 2023 17:06:38 +0000
>>>> Jonathan Cameron <jic23@kernel.org> wrote:
> 
> ...
> 
>>>> 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 :)
> 
> drivers/gpu/drm/tests/drm_managed_test.c ?
> 
> (somewhere underneath:
> 
>   ret = platform_driver_register(&fake_platform_driver);
> 
> which suggests... what exactly? :-)
> 

Thanks!

--Matti
  

On Mon, Mar 13, 2023 at 03:59:03PM +0200, Matti Vaittinen wrote:
> On 3/13/23 15:25, Andy Shevchenko wrote:
> > On Mon, Mar 13, 2023 at 02:47:45PM +0200, Matti Vaittinen wrote:
> > > On 3/6/23 14:52, Andy Shevchenko wrote:
> > > > On Mon, Mar 06, 2023 at 11:17:15AM +0200, Matti Vaittinen wrote:

...

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

> > > > > +		kfree(gts->avail_all_scales_table);
  

On 3/13/23 16:17, Andy Shevchenko wrote:
> On Mon, Mar 13, 2023 at 03:59:03PM +0200, Matti Vaittinen wrote:
>> On 3/13/23 15:25, Andy Shevchenko wrote:
>>> On Mon, Mar 13, 2023 at 02:47:45PM +0200, Matti Vaittinen wrote:
>>>> On 3/6/23 14:52, Andy Shevchenko wrote:
>>>>> On Mon, Mar 06, 2023 at 11:17:15AM +0200, Matti Vaittinen wrote:
> 
> ...
> 
>>>>>> +	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.
>>
>> 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.

Ah. I thought you suggested I had double check in the code I wrote. Now 
I see what you meant.

Yes, I think that check should be dropped.

-- Matti
  

On 3/13/23 15:14, Andy Shevchenko wrote:
> On Mon, Mar 13, 2023 at 02:56:59PM +0200, Matti Vaittinen wrote:
>> On 3/12/23 18:51, Jonathan Cameron wrote:
>>> On Mon, 6 Mar 2023 14:52:57 +0200
>>> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
>>>> On Mon, Mar 06, 2023 at 11:17:15AM +0200, Matti Vaittinen wrote:
> 
> ...
> 
>>>>> +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)
> 
> 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

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

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

On Tue, Mar 14, 2023 at 06:19:35AM +0000, Vaittinen, Matti wrote:
> On 3/13/23 15:14, Andy Shevchenko wrote:
> > On Mon, Mar 13, 2023 at 02:56:59PM +0200, Matti Vaittinen wrote:
> >> On 3/12/23 18:51, Jonathan Cameron wrote:
> >>> On Mon, 6 Mar 2023 14:52:57 +0200
> >>> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> >>>> On Mon, Mar 06, 2023 at 11:17:15AM +0200, Matti Vaittinen wrote:

...

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

If you asking me, I'm fine with either way. Usually the latter makes sense
when we expect APIs in the certain module to:
1) always belong to the single namespace, *and / or*
2) be expanded in the future w/o bothering about their (default) NS, *and not*
3) be a single exported function for the feasible future.

Also you made a good point about line length, but with all respect, I prefer
100 than 80 and I do not believe we ever will have function name + NS longer
than that.
  

On 3/13/23 15:59, Matti Vaittinen wrote:
> On 3/13/23 15:29, Andy Shevchenko wrote:
>> On Mon, Mar 13, 2023 at 03:11:52PM +0200, Matti Vaittinen wrote:
>>> On 3/13/23 14:40, Andy Shevchenko wrote:
>>>> On Sun, Mar 12, 2023 at 05:08:48PM +0000, Jonathan Cameron wrote:
>>>>> On Sun, 12 Mar 2023 17:06:38 +0000
>>>>> Jonathan Cameron <jic23@kernel.org> wrote:
>>
>> ...
>>
>>>>> 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 :)
>>
>> 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/20230302013822.1808711-1-sboyd@kernel.org/

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:
> On 3/13/23 15:59, Matti Vaittinen wrote:
> > On 3/13/23 15:29, Andy Shevchenko wrote:
> > > On Mon, Mar 13, 2023 at 03:11:52PM +0200, Matti Vaittinen wrote:
> > > > On 3/13/23 14:40, Andy Shevchenko wrote:
> > > > > On Sun, Mar 12, 2023 at 05:08:48PM +0000, Jonathan Cameron wrote:
> > > > > > On Sun, 12 Mar 2023 17:06:38 +0000
> > > > > > Jonathan Cameron <jic23@kernel.org> wrote:

...

> > > > > > 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 :)
> > > 
> > > 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/20230302013822.1808711-1-sboyd@kernel.org/
> 
> 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

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.
  

On Wed, Mar 15, 2023 at 04:12:59PM +0200, Andy Shevchenko wrote:
> 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.

Ah, and before doing that you actually may ask Stephen on his roadmap about
that. If no answer for a week or so, go with option 4.
  

On Wed, Mar 15, 2023 at 12:51:26PM +0200, Matti Vaittinen wrote:
> On 3/13/23 15:59, Matti Vaittinen wrote:
> > On 3/13/23 15:29, Andy Shevchenko wrote:
> > > On Mon, Mar 13, 2023 at 03:11:52PM +0200, Matti Vaittinen wrote:
> > > > On 3/13/23 14:40, Andy Shevchenko wrote:
> > > > > On Sun, Mar 12, 2023 at 05:08:48PM +0000, Jonathan Cameron wrote:
> > > > > > On Sun, 12 Mar 2023 17:06:38 +0000
> > > > > > Jonathan Cameron <jic23@kernel.org> wrote:
> > > 
> > > ...
> > > 
> > > > > > 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 :)
> > > 
> > > 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/20230302013822.1808711-1-sboyd@kernel.org/
> 
> 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?

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.

Maxime
  

On 3/17/23 12:19, Maxime Ripard wrote:
> On Wed, Mar 15, 2023 at 12:51:26PM +0200, Matti Vaittinen wrote:
>> On 3/13/23 15:59, Matti Vaittinen wrote:
>>> On 3/13/23 15:29, Andy Shevchenko wrote:
>>>> On Mon, Mar 13, 2023 at 03:11:52PM +0200, Matti Vaittinen wrote:
>>>>> On 3/13/23 14:40, Andy Shevchenko wrote:
>>>>>> On Sun, Mar 12, 2023 at 05:08:48PM +0000, Jonathan Cameron wrote:
>>>>>>> On Sun, 12 Mar 2023 17:06:38 +0000
>>>>>>> Jonathan Cameron <jic23@kernel.org> wrote:
>>>>
>>>> ...
>>>>
>>>>>>> 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 :)
>>>>
>>>> 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/20230302013822.1808711-1-sboyd@kernel.org/
>>
>> 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?
> 
> 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.

Thanks Andy and Maxime! I appreciate your help.

I guess I'll ping Stephen with a separate mail (although he's in CC 
here) and CC the relevant patches to him as well. I assume that gives 
him a chance to take a look what I am doing and suggest changes if needed.

My contribution is anyways small - it's mostly just renaming and moving 
those two SRm helper APIs + changing the callers. Most of that is 
probably outside the scope of his work - it would just fit the same 
files he will be adding =)

Yours,
	-- Matti


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

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

On Tue, 14 Mar 2023 06:19:35 +0000
"Vaittinen, Matti" <Matti.Vaittinen@fi.rohmeurope.com> wrote:

> On 3/13/23 15:14, Andy Shevchenko wrote:
> > On Mon, Mar 13, 2023 at 02:56:59PM +0200, Matti Vaittinen wrote:  
> >> On 3/12/23 18:51, Jonathan Cameron wrote:  
> >>> On Mon, 6 Mar 2023 14:52:57 +0200
> >>> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:  
> >>>> On Mon, Mar 06, 2023 at 11:17:15AM +0200, Matti Vaittinen wrote:  
> > 
> > ...
> >   
> >>>>> +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)  
> > 
> > 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?
> 

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.


> 
> Yours,
> 	--Matti
>
  

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

You can use :internal or :external to pick up only the docs you want
when including this stuff form a .rst file.

https://docs.kernel.org/doc-guide/kernel-doc.html


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

> >   
> >>>> +		kfree(gts->avail_all_scales_table);  
> > 
> > ...
> >   
> >>>> +	per_time_gains = kcalloc(gts->num_itime, sizeof(int *), GFP_KERNEL);  
> >>>
> >>> sizeof(type) is error prone in comparison to sizeof(*var).  
> >>
> >> Yes and no. In majority of cases where we see sizeof(*var) - the *var is no
> >> longer a pointer as having pointers to pointers is not _that_ common. When
> >> we see sizeof(type *) - we instantly know it is a size of a pointer and not
> >> a size of some other type.
> >>
> >> So yes, while having sizeof(*var) makes us tolerant to errors caused by
> >> variable type changes - it makes us prone to human reader errors. Also, if
> >> someone changes type of *var from pointer to some other type - then he/she
> >> is likely to in any case need to revise the array alloactions too.
> >>
> >> While I in general agree with you that the sizeof(variable) is better than
> >> sizeof(type) - I see that in cases like this the sizeof(type *) is clearer.  
> > 
> > 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 :)

This is one where I strongly prefer sizeof(*per_time_gains)
because it's easier to review.  I don't care so much if it's easier to
modify as reality is these rarely get modified.

I often just 'fix' these up whilst applying.

Jonathan
  

On 3/18/23 19:17, Jonathan Cameron wrote:
> On Tue, 14 Mar 2023 06:19:35 +0000
> "Vaittinen, Matti" <Matti.Vaittinen@fi.rohmeurope.com> wrote:
> 
>> On 3/13/23 15:14, Andy Shevchenko wrote:
>>> On Mon, Mar 13, 2023 at 02:56:59PM +0200, Matti Vaittinen wrote:
>>>> On 3/12/23 18:51, Jonathan Cameron wrote:
>>>>> On Mon, 6 Mar 2023 14:52:57 +0200
>>>>> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
>>>>>> On Mon, Mar 06, 2023 at 11:17:15AM +0200, Matti Vaittinen wrote:
>>>
>>> ...
>>>    
>>>>>>> +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)
>>>
>>> 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?
>>
> 
> 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.

I share the same view on this. I did use the DEFAULT_SYMBOL_NAMESPACE 
for v4 - but I'll drop that for v5 and go back with the explicit 
name-space usage.

Yours,
	-- Matti