Date:   Sun, 12 Mar 2023 17:08:48 +0000
From:   Jonathan Cameron <jic23@kernel.org>
To:     Matti Vaittinen <mazziesaccount@gmail.com>
Cc:     Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>,
        Lars-Peter Clausen <lars@metafoo.de>,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        Shreeya Patel <shreeya.patel@collabora.com>,
        Paul Gazzillo <paul@pgazz.com>,
        Dmitry Osipenko <dmitry.osipenko@collabora.com>,
        Zhigang Shi <Zhigang.Shi@liteon.com>,
        linux-kernel@vger.kernel.org, linux-iio@vger.kernel.org
Subject: Re: [PATCH v3 2/6] iio: light: Add gain-time-scale helpers
Message-ID: <20230312170848.651b5b2c@jic23-huawei>
In-Reply-To: <20230312170638.3e6807b7@jic23-huawei>
References: <cover.1678093787.git.mazziesaccount@gmail.com>
        <a4cb9a34ca027867ac014ffe93ca7e8245ce263f.1678093787.git.mazziesaccount@gmail.com>
        <20230312170638.3e6807b7@jic23-huawei>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

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  
>