Messages in this thread | | | Subject | Re: [PATCH v2 3/3] iio: dps310: Add pressure sensing capability | From | Eddie James <> | Date | Tue, 14 May 2019 15:25:55 -0500 |
| |
On 5/11/19 4:48 AM, Jonathan Cameron wrote: > On Wed, 8 May 2019 14:35:28 -0500 > Eddie James <eajames@linux.ibm.com> wrote: > >> The DPS310 supports measurement of pressure, so support that in the >> driver. Use background measurement like the temperature sensing and >> default to lowest precision and lowest measurement rate. >> >> Signed-off-by: Eddie James <eajames@linux.ibm.com> > Hi Eddie, > > A few comments inline. One is around how you might look at adding > fifo support (pushing to an IIO buffer) in the future... The IIO > data model isn't as flexible as this device can be, so we may need > to put some restrictions on combinations of options. > > Jonathan >> --- >> drivers/iio/pressure/dps310.c | 331 +++++++++++++++++++++++++++++++++++++++--- >> 1 file changed, 307 insertions(+), 24 deletions(-) >> >> diff --git a/drivers/iio/pressure/dps310.c b/drivers/iio/pressure/dps310.c >> index c42808e..a7ee28c 100644 >> --- a/drivers/iio/pressure/dps310.c >> +++ b/drivers/iio/pressure/dps310.c >> @@ -11,11 +11,11 @@ >> * c0 * 0.5 + c1 * T_raw / kT °C >> * >> * TODO: >> - * - Pressure sensor readings >> * - Optionally support the FIFO >> */ >> >> #include <linux/i2c.h> >> +#include <linux/math64.h> >> #include <linux/module.h> >> #include <linux/regmap.h> >> >> @@ -29,6 +29,8 @@ >> #define DPS310_TMP_B1 0x04 >> #define DPS310_TMP_B2 0x05 >> #define DPS310_PRS_CFG 0x06 >> +#define DPS310_PRS_RATE_BITS GENMASK(6, 4) >> +#define DPS310_PRS_PRC_BITS GENMASK(3, 0) >> #define DPS310_TMP_CFG 0x07 >> #define DPS310_TMP_RATE_BITS GENMASK(6, 4) >> #define DPS310_TMP_PRC_BITS GENMASK(3, 0) >> @@ -82,6 +84,8 @@ struct dps310_data { >> struct regmap *regmap; >>
>> + >> static bool dps310_is_writeable_reg(struct device *dev, unsigned int reg) >> { >> switch (reg) { >> @@ -253,24 +387,141 @@ static int dps310_write_raw(struct iio_dev *iio, >> { >> struct dps310_data *data = iio_priv(iio); >> >> - if (chan->type != IIO_TEMP) >> + switch (mask) { >> + case IIO_CHAN_INFO_SAMP_FREQ: >> + switch (chan->type) { >> + case IIO_PRESSURE: >> + return dps310_set_pres_samp_freq(data, val); >> + >> + case IIO_TEMP: >> + return dps310_set_temp_samp_freq(data, val); > This may need a bit of thought if there is any chance we will later support > the fifo. > > The IIO model is that of scans that read all channels at each 'trigger'. In > devices like this which allow for different sampling rates for different sensor > channels there are two options. > > 1) Don't support it. > 2) Deal with registering two separate IIO devices and do the demux in the > driver to the relevant one. > > All depends on whether there is a substantial usecase for different sampling > rates or not. Here I suspect the answer is not. > > The complexity is that, you then need to work out how to 'upgrade' the > interface when buffered support is added. Obvious options are: > > 1) Refuse to move to buffered mode if the sampling frequencies are different. > 2) Force the slower channel to be sampled faster if that is possible. > 3) Change to only having one exposed sampling frequency at all - the problem > with this last one is that it changes the ABI for existing users. > > It may be no one ever cares about the fifo mode though as high speed pressure > measurement is 'unusual' ;)
Thanks for the comments Jonathan. I will follow your suggestions throughout the driver.
The sampling rates are a bit confusing for me, and I haven't looked into the buffered mode at all. Are you saying that in the current form, it won't work to use different sampling frequencies for the two sensors (without buffered mode I mean)? I haven't noticed any problems in my tests. I'm inclined force the slower channel to be sampled faster if necessary when buffered mode is implemented.
Thanks,
Eddie
> >> + >> + default: >> + return -EINVAL; >> + } >> + >> + case IIO_CHAN_INFO_OVERSAMPLING_RATIO: >> + switch (chan->type) { >> + case IIO_PRESSURE: >> + return dps310_set_pres_precision(data, val); >> + >> + case IIO_TEMP: >> + return dps310_set_temp_precision(data, val); >> + >> + default: >> + return -EINVAL; >> + } >> + >> + default: >> return -EINVAL; >> + } >> +} >> + >> +static int dps310_calculate_pressure(struct dps310_data *data) >> +{ >> + int i, r, t_ready; >> + int kpi = dps310_get_pres_k(data); >> + int kti = dps310_get_temp_k(data); >> + s64 rem = 0ULL; >> + s64 pressure = 0ULL; >> + s64 p; >> + s64 t; >> + s64 denoms[7]; >> + s64 nums[7]; >> + s64 rems[7]; >> + s64 kp; >> + s64 kt; >> + >> + if (kpi < 0) >> + return kpi; >> + >> + if (kti < 0) >> + return kti; >> + >> + kp = (s64)kpi; >> + kt = (s64)kti; >> + >> + /* Refresh temp if it's ready, otherwise just use the latest value */ >> + r = regmap_read(data->regmap, DPS310_MEAS_CFG, &t_ready); >> + if (r >= 0 && t_ready & DPS310_TMP_RDY) >> + dps310_read_temp_ready(data); >> + >> + p = (s64)data->pressure_raw; >> + t = (s64)data->temp_raw; >> + >> + /* Section 4.9.1 of the DPS310 spec; algebra'd to avoid underflow */ >> + nums[0] = (s64)data->c00; >> + denoms[0] = 1LL; >> + nums[1] = p * (s64)data->c10; >> + denoms[1] = kp; >> + nums[2] = p * p * (s64)data->c20; >> + denoms[2] = kp * kp; >> + nums[3] = p * p * p * (s64)data->c30; >> + denoms[3] = kp * kp * kp; >> + nums[4] = t * (s64)data->c01; >> + denoms[4] = kt; >> + nums[5] = t * p * (s64)data->c11; >> + denoms[5] = kp * kt; >> + nums[6] = t * p * p * (s64)data->c21; >> + denoms[6] = kp * kp * kt; >> + >> + /* Kernel lacks a div64_s64_rem function; denoms are all positive */ >> + for (i = 0; i < 7; ++i) { >> + u64 rem; >> + >> + if (nums[i] < 0LL) { >> + pressure -= div64_u64_rem(-nums[i], denoms[i], &rem); >> + rems[i] = -rem; >> + } else { >> + pressure += div64_u64_rem(nums[i], denoms[i], &rem); >> + rems[i] = (s64)rem; >> + } >> + } >> + >> + /* Increase precision and calculate the remainder sum */ >> + for (i = 0; i < 7; ++i) >> + rem += div64_s64((s64)rems[i] * 1000000000LL, denoms[i]); >> + >> + pressure += div_s64(rem, 1000000000LL); >> + >> + return (int)pressure; >> +} >> + >> +static int dps310_read_pressure(struct dps310_data *data, int *val, int *val2, >> + long mask) >> +{ >> + int ret; >> >> switch (mask) { >> case IIO_CHAN_INFO_SAMP_FREQ: >> - return dps310_set_temp_samp_freq(data, val); >> + *val = dps310_get_pres_samp_freq(data); >> + return IIO_VAL_INT; >> + >> + case IIO_CHAN_INFO_RAW: >> + ret = dps310_read_pres_raw(data); >> + if (ret) >> + return ret; >> + >> + *val = dps310_calculate_pressure(data); > This is rather far from raw :) It might be better at this point to just > go for PROCESSED and apply the scale in here. > >> + return IIO_VAL_INT; >> + >> + case IIO_CHAN_INFO_SCALE: >> + *val = 1; >> + *val2 = 1000; /* Convert Pa to KPa per IIO ABI */ >> + return IIO_VAL_FRACTIONAL; >> + >> case IIO_CHAN_INFO_OVERSAMPLING_RATIO: >> - return dps310_set_temp_precision(data, val); >> + *val = dps310_get_pres_precision(data); >> + return IIO_VAL_INT; >> + >> default: >> return -EINVAL; >> } >> } >> >> -static int dps310_read_raw(struct iio_dev *iio, >> - struct iio_chan_spec const *chan, >> - int *val, int *val2, long mask) >> +static int dps310_read_temp(struct dps310_data *data, int *val, int *val2, >> + long mask) >> { >> - struct dps310_data *data = iio_priv(iio); >> int ret; >> >> switch (mask) { >> @@ -279,7 +530,7 @@ static int dps310_read_raw(struct iio_dev *iio, >> return IIO_VAL_INT; >> >> case IIO_CHAN_INFO_RAW: >> - ret = dps310_read_temp(data); >> + ret = dps310_read_temp_raw(data); >> if (ret) >> return ret; >> >> @@ -312,6 +563,24 @@ static int dps310_read_raw(struct iio_dev *iio, >> } >> } >> >> +static int dps310_read_raw(struct iio_dev *iio, >> + struct iio_chan_spec const *chan, >> + int *val, int *val2, long mask) >> +{ >> + struct dps310_data *data = iio_priv(iio); >> + >> + switch (chan->type) { >> + case IIO_PRESSURE: >> + return dps310_read_pressure(data, val, val2, mask); >> + >> + case IIO_TEMP: >> + return dps310_read_temp(data, val, val2, mask); >> + >> + default: >> + return -EINVAL; >> + } >> +} >> + >> static const struct regmap_config dps310_regmap_config = { >> .reg_bits = 8, >> .val_bits = 8, >> @@ -393,6 +662,13 @@ static int dps310_probe(struct i2c_client *client, >> return PTR_ERR(data->regmap); >> >> /* >> + * Set up pressure sensor in single sample, one measurement per second >> + * mode >> + */ >> + r = regmap_write(data->regmap, DPS310_PRS_CFG, >> + DPS310_CALC_RATE(1) | DPS310_CALC_PRC(1)); >> + >> + /* >> * Set up external (MEMS) temperature sensor in single sample, one >> * measurement per second mode >> */ >> @@ -402,16 +678,23 @@ static int dps310_probe(struct i2c_client *client, >> if (r < 0) >> goto err; >> >> - /* Temp shift is disabled when PRC <= 8 */ >> + /* Temp and pressure shifts are disabled when PRC <= 8 */ >> r = regmap_write_bits(data->regmap, DPS310_CFG_REG, >> - DPS310_TMP_SHIFT_EN, 0); >> + DPS310_TMP_SHIFT_EN | DPS310_PRS_SHIFT_EN, 0); >> + if (r < 0) >> + goto err; >> + >> + /* MEAS_CFG doesn't seem to update unless first written with 0 */ >> + r = regmap_write_bits(data->regmap, DPS310_MEAS_CFG, >> + DPS310_MEAS_CTRL_BITS, 0); >> if (r < 0) >> goto err; >> >> - /* Turn on temperature measurement in the background */ >> + /* Turn on temperature and pressure measurement in the background */ >> r = regmap_write_bits(data->regmap, DPS310_MEAS_CFG, >> DPS310_MEAS_CTRL_BITS, >> - DPS310_TEMP_EN | DPS310_BACKGROUND); >> + DPS310_PRS_EN | DPS310_TEMP_EN | >> + DPS310_BACKGROUND); >> if (r < 0) >> goto err; >> >> @@ -424,7 +707,7 @@ static int dps310_probe(struct i2c_client *client, >> if (r < 0) >> goto err; >> >> - r = dps310_get_temp_coef(data); >> + r = dps310_get_coefs(data); >> if (r < 0) >> goto err; >>
| |