Re: [PATCH 1/3 v3] iio: magnetometer: ak8974: Correct realbits

From: Jonathan Cameron <jic23@kernel.org>
To: "Michał Mirosław" <mirq-linux@rere.qmqm.pl>
Cc: Linus Walleij <linus.walleij@linaro.org>,
	linux-iio@vger.kernel.org, Hartmut Knaack <knaack.h@gmx.de>,
	Lars-Peter Clausen <lars@metafoo.de>,
	Peter Meerwald-Stadler <pmeerw@pmeerw.net>,
	linux-input@vger.kernel.org,
	Nick Reitemeyer <nick.reitemeyer@web.de>,
	Stephan Gerhold <stephan@gerhold.net>
Subject: Re: [PATCH 1/3 v3] iio: magnetometer: ak8974: Correct realbits
Date: Sat, 18 Apr 2020 15:30:02 +0100	[thread overview]
Message-ID: <20200418153002.62626a88@archlinux> (raw)
In-Reply-To: <20200417140521.GB18337@qmqm.qmqm.pl>

On Fri, 17 Apr 2020 16:05:21 +0200
Michał Mirosław <mirq-linux@rere.qmqm.pl> wrote:

> On Fri, Apr 17, 2020 at 01:40:18PM +0200, Linus Walleij wrote:
> > The original AK8974 has 16 bits of actual value, while the
> > HSCDTD008A has 15 bits and the AMI305 and AMI306 has 12 bits.
> > Correct this by providing an extra parameter to the channel
> > macro and define a separate set of channels for each variant
> > of the chip. The resolution is the actual resolution of the
> > internal ADC of the chip.
> > 
> > The values are stored in a S16 in 2's complement so all 16
> > bits are used for storing (no shifting needed).
> > 
> > The AMI305, AMI306 and HSCDTD008A valid bits are picked from
> > respective datasheet.
> > 
> > My best educated guess is that AK8974 is also 12 bits. The
> > AK8973 is an 8 bit and earlier version, and the sibling
> > drivers AMI305 and AMI306 are 12 bits, so it makes sense
> > to assume that the AK8974 is also 12 bits.  
> [...]
> > -#define AK8974_AXIS_CHANNEL(axis, index)				\
> > +#define AK8974_AXIS_CHANNEL(axis, index, bits)				\
> >  	{								\
> >  		.type = IIO_MAGN,					\
> >  		.modified = 1,						\
> > @@ -662,16 +662,42 @@ static const struct iio_chan_spec_ext_info ak8974_ext_info[] = {
> >  		.scan_index = index,					\
> >  		.scan_type = {						\
> >  			.sign = 's',					\
> > -			.realbits = 16,					\
> > +			.realbits = bits,				\
> >  			.storagebits = 16,				\
> >  			.endianness = IIO_LE				\
> >  		},							\
> >  	}
> >  
> > +/*
> > + * We have no datasheet for the AK8974 but we guess that its
> > + * ADC is 12 bits.
> > + */
> >  static const struct iio_chan_spec ak8974_channels[] = {
> > -	AK8974_AXIS_CHANNEL(X, 0),
> > -	AK8974_AXIS_CHANNEL(Y, 1),
> > -	AK8974_AXIS_CHANNEL(Z, 2),
> > +	AK8974_AXIS_CHANNEL(X, 0, 12),
> > +	AK8974_AXIS_CHANNEL(Y, 1, 12),
> > +	AK8974_AXIS_CHANNEL(Z, 2, 12),
> > +	IIO_CHAN_SOFT_TIMESTAMP(3),
> > +};
> > +
> > +/*
> > + * The AMI305 and AMI306 have 12 bit ADC resolution according to
> > + * datasheets.
> > + */
> > +static const struct iio_chan_spec ami30x_channels[] = {
> > +	AK8974_AXIS_CHANNEL(X, 0, 12),
> > +	AK8974_AXIS_CHANNEL(Y, 1, 12),
> > +	AK8974_AXIS_CHANNEL(Z, 2, 12),
> > +	IIO_CHAN_SOFT_TIMESTAMP(3),
> > +};  
> 
> Maybe call it channels_12bit[] and then you wouldn't need to make
> am exact duplicate for ak8974?

Agreed they should be combined.  I've not problem with just picking
one device name though on a first come first named basis...

Up to you.

Jonathan

> 
> Best Regards,
> Michał Mirosław