> -----Original Message----- > From: Jonathan Cameron [mailto:Jonathan.Cameron@xxxxxxxxxx] > Sent: 2022年2月23日 0:57 > To: Bough Chen <haibo.chen@xxxxxxx> > Cc: jic23@xxxxxxxxxx; lars@xxxxxxxxxx; linux-iio@xxxxxxxxxxxxxxx; > dl-linux-imx <linux-imx@xxxxxxx> > Subject: Re: [PATCH] iio: imu: fxos8700: few bug fix for fxos8700 > > On Tue, 22 Feb 2022 12:07:02 +0800 > <haibo.chen@xxxxxxx> wrote: > > > From: Haibo Chen <haibo.chen@xxxxxxx> > > > > 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@xxxxxxx> > > Reviewed-by: Clark Wang <xiaoning.wang@xxxxxxx> > > --- > > 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:
smime.p7s
Description: S/MIME cryptographic signature
