On Tue, 14 May 2019 15:25:55 -0500 Eddie James <eajames@xxxxxxxxxxxxxxxxxx> wrote: > On 5/11/19 4:48 AM, Jonathan Cameron wrote: > > On Wed, 8 May 2019 14:35:28 -0500 > > Eddie James <eajames@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx> > > 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. That would be a 'slightly' interesting interpretation of the ABI, but I'd 'probably' let it go as any correct userspace should be fine receiving data at a higher rate than it asked for. Jonathan > > 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; > >> >