RE: [PATCH] iio: imu: fxos8700: few bug fix for fxos8700

[Bough Chen <haibo.chen@nxp.com>]

[-- Attachment #1: Type: text/plain, Size: 8304 bytes --]

> -----Original Message-----
> From: Jonathan Cameron [mailto:Jonathan.Cameron@Huawei.com]
> Sent: 2022年2月23日 0:57
> To: Bough Chen <haibo.chen@nxp.com>
> Cc: jic23@kernel.org; lars@metafoo.de; linux-iio@vger.kernel.org;
> dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH] iio: imu: fxos8700: few bug fix for fxos8700
> 
> On Tue, 22 Feb 2022 12:07:02 +0800
> <haibo.chen@nxp.com> wrote:
> 
> > From: Haibo Chen <haibo.chen@nxp.com>
> >
> > 1, z raw data always 0, regmap_buk_read use the wrong length. fix it
> > and optmize read the only need data.
> > 2, use the correct register address when try to read raw data.
> > 3, before set scale, need to set the sensor to standby mode. otherwise
> > the scale set is not work.
> > 4, give the correct offset when config odr bit.
> 
> Sounds like 4 patches to me. Whenever you have a list of what a patch does
> you should probably split it up.  Would be a lot easier to review as one
> patch per issue.  For now I've just take a quick general look.
> 
> 
Hi Jonathan

Thanks for your suggestion, I will split few patches in the next version.

> >
> > Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
> > Reviewed-by: Clark Wang <xiaoning.wang@nxp.com>
> > ---
> >  drivers/iio/imu/fxos8700_core.c | 66
> > +++++++++++++++++++++++----------
> >  1 file changed, 47 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/iio/imu/fxos8700_core.c
> > b/drivers/iio/imu/fxos8700_core.c index ab288186f36e..1896d6db6d77
> > 100644
> > --- a/drivers/iio/imu/fxos8700_core.c
> > +++ b/drivers/iio/imu/fxos8700_core.c
> > @@ -162,12 +162,11 @@
> >
> >  #define FXOS8700_DEVICE_ID          0xC7
> >  #define FXOS8700_PRE_DEVICE_ID      0xC4
> > -#define FXOS8700_DATA_BUF_SIZE      3
> >
> >  struct fxos8700_data {
> >  	struct regmap *regmap;
> >  	struct iio_trigger *trig;
> > -	__be16 buf[FXOS8700_DATA_BUF_SIZE] ____cacheline_aligned;
> > +	__be16 buf ____cacheline_aligned;
> >  };
> >
> >  /* Regmap info */
> > @@ -345,7 +344,8 @@ static int fxos8700_set_active_mode(struct
> > fxos8700_data *data,  static int fxos8700_set_scale(struct
> fxos8700_data *data,
> >  			      enum fxos8700_sensor t, int uscale)  {
> > -	int i;
> > +	int i, ret, val;
> > +	bool active_mode;
> >  	static const int scale_num = ARRAY_SIZE(fxos8700_accel_scale);
> >  	struct device *dev = regmap_get_device(data->regmap);
> >
> > @@ -354,6 +354,23 @@ static int fxos8700_set_scale(struct
> fxos8700_data *data,
> >  		return -EINVAL;
> >  	}
> >
> > +	ret = regmap_read(data->regmap, FXOS8700_CTRL_REG1, &val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	active_mode = val & FXOS8700_ACTIVE;
> > +
> > +	if (active_mode) {
> > +		/*
> > +		 * The device must be in standby mode to change any of the
> > +		 * other fields within CTRL_REG1
> > +		 */
> > +		ret = regmap_write(data->regmap, FXOS8700_CTRL_REG1,
> > +				   val & ~FXOS8700_ACTIVE);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> >  	for (i = 0; i < scale_num; i++)
> >  		if (fxos8700_accel_scale[i].uscale == uscale)
> >  			break;
> > @@ -361,8 +378,12 @@ static int fxos8700_set_scale(struct
> fxos8700_data *data,
> >  	if (i == scale_num)
> >  		return -EINVAL;
> >
> > -	return regmap_write(data->regmap, FXOS8700_XYZ_DATA_CFG,
> > +	ret = regmap_write(data->regmap, FXOS8700_XYZ_DATA_CFG,
> >  			    fxos8700_accel_scale[i].bits);
> 
> Realign these parameters with the opening bracket.
> 
> > +	if (ret)
> > +		return ret;
> 
> blank line here.
> 
> > +	return regmap_update_bits(data->regmap, FXOS8700_CTRL_REG1,
> > +				  FXOS8700_ACTIVE, active_mode);
> >  }
> >
> >  static int fxos8700_get_scale(struct fxos8700_data *data, @@ -393,23
> > +414,29 @@ static int fxos8700_get_scale(struct fxos8700_data *data,
> > static int fxos8700_get_data(struct fxos8700_data *data, int chan_type,
> >  			     int axis, int *val)
> >  {
> > -	u8 base, reg;
> > -	int ret;
> > +	u8 base, offset;
> >  	enum fxos8700_sensor type = fxos8700_to_sensor(chan_type);
> > +	u8 tmp_data[2];
> We loop around this every now and then. It 'happens' to be the case that
> currently (or last time I checked) regmap_bulk_read always copied the data
> and hence uses a dma safe buffer internally. That is not guaranteed by the
> interface however so when we last asked Mark Brown he suggested we
> should assume that it requires the same level of dma buffer safety as the
bus
> subsystems being used.
> 
> Thus for any driver doing bulk accesses to SPI device, you need a DMA safe
> buffer.  Which is what the __cacheline_aligned buffer in iio_priv() is for
in
> this driver.

Thanks for your sharing, I will take care of this.

> 
> > +	u16 native_data;
> > +	int ret;
> >
> > -	base = type ? FXOS8700_OUT_X_MSB : FXOS8700_M_OUT_X_MSB;
> > +	base = type ? FXOS8700_M_OUT_X_MSB : FXOS8700_OUT_X_MSB;
> > +	offset = axis - IIO_MOD_X;
> >
> > -	/* Block read 6 bytes of device output registers to avoid data loss
*/
> > -	ret = regmap_bulk_read(data->regmap, base, data->buf,
> > -			       FXOS8700_DATA_BUF_SIZE);
> > +	ret = regmap_bulk_read(data->regmap, base + offset, &tmp_data[0],
> > +2);
> >  	if (ret)
> > -		return ret;
> > +		return -EIO;
> 
> Why eat the error return of the bulk_read and replace it with a
potentially
> less informative one?

My bad, will fix.

> 
> >
> > -	/* Convert axis to buffer index */
> > -	reg = axis - IIO_MOD_X;
> >
> > +	data->buf = ((tmp_data[1] << 8) & 0xff00) | tmp_data[0];
> 
> tmp_data[1] is a u8 so that masking isn't doing anything other than
possibly
> fixing some type conversion issues.

Oh, you are correct, I will fix that.
> 
> However, this is an endian operation, so express it as such
> get_unaligned_be16(tmp_data); or similar.  Maybe even just use a __be16
> and be16_to_cpu() directly on that.

I will re-do this code.

> 
> 
> >  	/* Convert to native endianness */
> > -	*val = sign_extend32(be16_to_cpu(data->buf[reg]), 15);
> > +	native_data = be16_to_cpu(data->buf);
> 
> This looks wrong.  You've already done a be to cpu conversion (via the
> shifts above) now y ou are doing it again. Why?

For this sensor, according to the RM
For the first register(address 01), we get data[13~6], and for the second
register(address 02), we get 8 bit data, the upper 6 bit is data[5~0],
Seems I made the logic complicated.

> 
> > +
> > +	/*accel raw data only has 14 bit */
> 
> /* Accel ...
> 
> > +	if (!type)
> > +		native_data = native_data >> 2;
> > +
> > +	*val = sign_extend32(native_data, 15);
> >
> >  	return 0;
> >  }
> > @@ -462,6 +489,7 @@ static int fxos8700_get_odr(struct fxos8700_data
> *data, enum fxos8700_sensor t,
> >  		return ret;
> >
> >  	val &= FXOS8700_CTRL_ODR_MSK;
> > +	val = val >> 3;
> 
> FIELD_GET() would be easier to read for this.
> 
> >
> >  	for (i = 0; i < odr_num; i++)
> >  		if (val == fxos8700_odr[i].bits)
> > @@ -592,14 +620,14 @@ static int fxos8700_chip_init(struct
> fxos8700_data *data, bool use_spi)
> >  	if (ret)
> >  		return ret;
> >
> > -	/* Max ODR (800Hz individual or 400Hz hybrid), active mode */
> > -	ret = regmap_write(data->regmap, FXOS8700_CTRL_REG1,
> > -			   FXOS8700_CTRL_ODR_MAX | FXOS8700_ACTIVE);
> > +	/* Set for max full-scale range (+/-8G) */
> > +	ret = regmap_write(data->regmap, FXOS8700_XYZ_DATA_CFG,
> MODE_8G);
> >  	if (ret)
> >  		return ret;
> >
> > -	/* Set for max full-scale range (+/-8G) */
> > -	return regmap_write(data->regmap, FXOS8700_XYZ_DATA_CFG,
> MODE_8G);
> > +	/* Max ODR (800Hz individual or 400Hz hybrid), active mode */
> > +	return regmap_write(data->regmap, FXOS8700_CTRL_REG1,
> > +			   FXOS8700_CTRL_ODR_MAX << 3 | FXOS8700_ACTIVE);
> 
> Preference for FIELD_PREP() to make ti clear what you are shifting left
and
> why.
> Given you have FXOS8700_CTRL_ODR_MSK that is easy to add here.
> Mind you it's a noop as ODR_MAX == 0 anyway :)

Thanks for your suggestion, I just want to optimize that I config ODR_MAX,
but seems
It better only mentioned this in the comment, do not need add in the code.

Best Regards
Haibo Chen
> 
> 
> >  }
> >
> >  static void fxos8700_chip_uninit(void *data)


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 9551 bytes --]