| Message ID | 1645502822-5585-1-git-send-email-haibo.chen@nxp.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: imu: fxos8700: few bug fix for fxos8700 | expand |
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. > > 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. > + 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? > > - /* 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. 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. > /* 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? > + > + /*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 :) > } > > static void fxos8700_chip_uninit(void *data)
> -----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)
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); + if (ret) + return ret; + 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]; + 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; - /* Convert axis to buffer index */ - reg = axis - IIO_MOD_X; + data->buf = ((tmp_data[1] << 8) & 0xff00) | tmp_data[0]; /* Convert to native endianness */ - *val = sign_extend32(be16_to_cpu(data->buf[reg]), 15); + native_data = be16_to_cpu(data->buf); + + /*accel raw data only has 14 bit */ + 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; 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); } static void fxos8700_chip_uninit(void *data)