On Tue, Sep 03, 2024 at 05:26:38PM +0300, Andy Shevchenko wrote:
> On Mon, Sep 02, 2024 at 08:42:19PM +0200, Vasileios Amoiridis wrote:
> > Add forced mode support in sensors BMP28x, BME28x, BMP3xx and BMP58x.
> > Sensors BMP18x and BMP085 are old and do not support this feature so
> > their operation is not affected at all.
> >
> > Essentially, up to now, the rest of the sensors were used in normal mode
> > all the time. This means that they are continuously doing measurements
> > even though these measurements are not used. Even though the sensor does
> > provide PM support, to cover all the possible use cases, the sensor needs
> > to go into sleep mode and wake up whenever necessary.
> >
> > The idea is that the sensor is by default in sleep mode, wakes up in
> > forced mode when a oneshot capture is requested, or in normal mode
> > when the buffer is enabled. The difference lays in the fact that in
> > forced mode, the sensor does only one conversion and goes back to sleep
> > while in normal mode, the sensor does continuous measurements with the
> > frequency that was set in the ODR registers.
> >
> > The bmpX_chip_config() functions which are responsible for applying
> > the requested configuration to the sensor, are modified accordingly
> > in order to set the sensor by default in sleep mode.
> >
> > DEEP STANDBY, Low Power NORMAL and CONTINUOUS modes, supported only by
> > the BMP58x version, are not added.
Hi Andy,
Thanks for finding again the time to review this!
>
> ...
>
> > +static int bmp280_wait_conv(struct bmp280_data *data)
> > +{
> > + unsigned int reg, meas_time_us;
> > + int ret;
> > +
> > + /* Check if we are using a BME280 device */
> > + if (data->oversampling_humid)
> > + meas_time_us += BMP280_PRESS_HUMID_MEAS_OFFSET +
> > + (BIT(data->oversampling_humid) * BMP280_MEAS_DUR);
>
> The outer parentheses are not needed.
>
True, will fix that.
> > + /* Pressure measurement time */
> > + meas_time_us += BMP280_PRESS_HUMID_MEAS_OFFSET +
> > + (BIT(data->oversampling_press) * BMP280_MEAS_DUR);
>
> Ditto.
ACK.
>
> > + /* Temperature measurement time */
> > + meas_time_us += BIT(data->oversampling_temp) * BMP280_MEAS_DUR;
> > +
> > + /* Waiting time according to the BM(P/E)2 Sensor API */
> > + fsleep(meas_time_us);
> > +
> > + ret = regmap_read(data->regmap, BMP280_REG_STATUS, ®);
> > + if (ret) {
> > + dev_err(data->dev, "failed to read status register.\n");
> > + return ret;
> > + }
> > +
> > + if (reg & BMP280_REG_STATUS_MEAS_BIT) {
> > + dev_err(data->dev, "Measurement cycle didn't complete.\n");
> > + return -EBUSY;
> > + }
> > +
> > + return 0;
> > +}
>
> ...
>
> > +static int bmp380_wait_conv(struct bmp280_data *data)
> > +{
> > + unsigned int reg;
> > + int ret, meas_time_us;
> > +
> > + /* Offset measurement time */
> > + meas_time_us = BMP380_MEAS_OFFSET;
> > +
> > + /* Pressure measurement time */
> > + meas_time_us += BMP380_PRESS_MEAS_OFFSET +
> > + (BIT(data->oversampling_press) * BMP380_MEAS_DUR);
>
> Ditto.
>
ACK.
> > + /* Temperature measurement time */
> > + meas_time_us += BMP380_TEMP_MEAS_OFFSET +
> > + (BIT(data->oversampling_temp) * BMP380_MEAS_DUR);
>
> Ditto.
>
ACK.
> > + /* Measurement time defined in Datasheet Section 3.9.2 */
> > + fsleep(meas_time_us);
> > +
> > + ret = regmap_read(data->regmap, BMP380_REG_STATUS, ®);
> > + if (ret) {
> > + dev_err(data->dev, "failed to read status register.\n");
> > + return ret;
> > + }
>
> > + if (!(reg & BMP380_STATUS_DRDY_PRESS_MASK) ||
> > + !(reg & BMP380_STATUS_DRDY_TEMP_MASK)) {
> > + dev_err(data->dev, "Measurement cycle didn't complete.\n");
> > + return -EBUSY;
> > + }
>
> Alternatively
>
> if (!((reg & BMP380_STATUS_DRDY_PRESS_MASK) &&
> !(reg & BMP380_STATUS_DRDY_TEMP_MASK)) {
> dev_err(data->dev, "Measurement cycle didn't complete.\n");
> return -EBUSY;
> }
>
Why would I use && instead of || ? I just need one of the 2 to be true
(one of the 2 measurements is not complete) and I can trigger the error
action.
> > + return 0;
> > +}
>
> ...
>
> > +static int bmp580_wait_conv(struct bmp280_data *data)
> > +{
> > + /*
> > + * Taken from datasheet, Section 2 "Specification, Table 3 "Electrical
> > + * characteristics.
> > + */
> > + static const int time_conv_press[] = {
> > + 0, 1050, 1785, 3045, 5670, 10920, 21420, 42420,
> > + 84420,
> > + };
> > + static const int time_conv_temp[] = {
> > + 0, 1050, 1105, 1575, 2205, 3465, 6090, 11340,
> > + 21840,
> > + };
> > + int meas_time_us;
>
> > + meas_time_us = 4 * USEC_PER_MSEC + time_conv_temp[data->oversampling_temp]
> > + + time_conv_press[data->oversampling_press];
>
> meas_time_us = 4 * USEC_PER_MSEC + time_conv_temp[data->oversampling_temp] +
> time_conv_press[data->oversampling_press];
>
> OR
>
> meas_time_us = 4 * USEC_PER_MSEC +
> time_conv_temp[data->oversampling_temp] +
> time_conv_press[data->oversampling_press];
>
>
ACK.
> > + /* Measurement time mentioned in Chapter 2, Table 4 of the datasheet. */
>
> Since there is a constant in use (4ms) it would be nice to explain it
> separately, the rest kinda obvious from the variable names.
> So it allows roughly understand the timeout value without even looking into
> the datasheet.
>
True, I can do that.
> > + fsleep(meas_time_us);
> > +
> > + return 0;
> > +}
>
> ...
>
> > + fsleep(data->start_up_time + 500);
>
> Ditto.
>
> Something like
>
> /* 500us margin for ... */
>
> (but write the real meaning of it).
>
ACK.
> --
> With Best Regards,
> Andy Shevchenko
>
>
Best regards,
Vasilis
