linux-kernel - Re: [PATCH 2/4] iio: adc: ad4695: implement calibration support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240821173352.000000b6@Huawei.com>
Date: Wed, 21 Aug 2024 17:33:52 +0100
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: David Lechner <dlechner@...libre.com>
CC: Jonathan Cameron <jic23@...nel.org>, Michael Hennerich
	<Michael.Hennerich@...log.com>, Nuno Sá
	<nuno.sa@...log.com>, Jonathan Corbet <corbet@....net>,
	<linux-iio@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<linux-doc@...r.kernel.org>
Subject: Re: [PATCH 2/4] iio: adc: ad4695: implement calibration support

On Wed, 21 Aug 2024 11:12:12 -0500
David Lechner <dlechner@...libre.com> wrote:

> On 8/21/24 8:16 AM, Jonathan Cameron wrote:
> > On Tue, 20 Aug 2024 10:58:36 -0500
> > David Lechner <dlechner@...libre.com> wrote:
> >   
> >> The AD4695 has a calibration feature that allows the user to compensate
> >> for variations in the analog front end. This implements this feature in
> >> the driver using the standard `calibgain` and `calibbias` attributes.
> >>
> >> Signed-off-by: David Lechner <dlechner@...libre.com>  
> > Hi David,
> > 
> > Whilst some of the messy value manipulation is unavoidable
> > (oh for signed integer zero!), I wonder if we can at least
> > move one case into the core.
> > 
> > See below.
> >   
> >> +
> >> +static int ad4695_write_raw(struct iio_dev *indio_dev,
> >> +			    struct iio_chan_spec const *chan,
> >> +			    int val, int val2, long mask)
> >> +{
> >> +	struct ad4695_state *st = iio_priv(indio_dev);
> >> +	unsigned int reg_val;
> >> +	int ret;
> >> +
> >> +	iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> >> +		switch (mask) {
> >> +		case IIO_CHAN_INFO_CALIBSCALE:
> >> +			switch (chan->type) {
> >> +			case IIO_VOLTAGE:
> >> +				if (val < 0 || val2 < 0)
> >> +					reg_val = 0;
> >> +				else if (val > 1)
> >> +					reg_val = U16_MAX;
> >> +				else
> >> +					reg_val = (val * (1 << 16) +
> >> +						   mul_u64_u32_div(val2, 1 << 16,
> >> +								   MICRO)) / 2;  
> > Maybe worth extending iio_write_channel_info() to handle
> > IIO_VAL_FRACTIONAL_LOG2()?
> > It'll look much like this and you'll need to provide write_raw_get_fmt()
> > so the core know what to do with the value formatting.
> > 
> > I don't really like the mixture here between the read path being able
> > to rely on the core code to deal with the /2^X and the write path not.  
> 
> Sounds like a good idea to me.
> 
> It seems like we would need to add an extra out parameter to 
> write_raw_get_fmt to say what we want the X in 2^X to be. For
> example, we would want to make sure the val2 we get in write_raw
> for this driver is 15.
Yes.  (I tried to reply to say I'd neglected this but managed to
just email myself. oops.)

Maybe it's too complex to bother given that.  Definitely a job
for another day rather than something to block this series on.


Jonathan

> 
> >> +
> >> +				return regmap_write(st->regmap16,
> >> +					AD4695_REG_GAIN_IN(chan->scan_index),
> >> +					reg_val);
> >> +			default:
> >> +				return -EINVAL;  
> > 
> > 
> >   
>