On Fri, Aug 23, 2024 at 10:25:01PM +0300, Andy Shevchenko wrote:
> On Fri, Aug 23, 2024 at 08:17:11PM +0200, Vasileios Amoiridis wrote:
> > This commit adds 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.
> >
> > This commit, adds sleep and forced mode support. Essentially, the sensor
> > sleeps all the time except for when a measurement is requested. When there
> > is a request for a measurement, the sensor is put into forced mode, starts
> > the measurement and after it is done we read the output and we put it again
> > in sleep mode.
> >
> > For really fast and more deterministic measurements, the triggered buffer
> > interface can be used, since the sensor is still used in normal mode for
> > that use case.
> >
> > This commit does not add though support for DEEP STANDBY, Low Power NORMAL
> > and CONTINUOUS modes, supported only by the BMP58x version.
>
> ...
>
> > +static const u8 bmp280_operation_mode[] = { BMP280_MODE_SLEEP,
> > + BMP280_MODE_FORCED,
> > + BMP280_MODE_NORMAL };
>
> Better style is
>
> static const u8 bmp280_operation_mode[] = {
> BMP280_MODE_SLEEP, BMP280_MODE_FORCED, BMP280_MODE_NORMAL,
> };
>
> Also note comma at the end.
>
Looks much better indeed, thanks!
> ...
>
> > +static int bmp280_wait_conv(struct bmp280_data *data)
> > +{
> > + unsigned int reg;
> > + int ret, meas_time;
> > +
> > + meas_time = BMP280_MEAS_OFFSET;
> > +
> > + /* Check if we are using a BME280 device */
> > + if (data->oversampling_humid)
> > + meas_time += (1 << data->oversampling_humid) * BMP280_MEAS_DUR +
>
> BIT(data->oversampling_humid)
ACK.
>
> > + BMP280_PRESS_HUMID_MEAS_OFFSET;
>
> > + /* Pressure measurement time */
> > + meas_time += (1 << data->oversampling_press) * BMP280_MEAS_DUR +
>
> Ditto.
ACK.
>
> > + BMP280_PRESS_HUMID_MEAS_OFFSET;
>
> > + /* Temperature measurement time */
> > + meas_time += (1 << data->oversampling_temp) * BMP280_MEAS_DUR;
>
> Ditto.
>
ACK.
> > + usleep_range(meas_time, meas_time * 12 / 10);
>
> fsleep() ?
Could be used indeed. My concern is that fsleep uses a sleep range
between x and 2x but I don't think it is a problem since these are
oneshot captures.
>
> > + 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 const u8 bmp380_operation_mode[] = { BMP380_MODE_SLEEP,
> > + BMP380_MODE_FORCED,
> > + BMP380_MODE_NORMAL };
>
> As per above.
>
ACK.
> ...
>
> > +static int bmp380_wait_conv(struct bmp280_data *data)
> > +{
>
> As per above comments against bmp280_wait_conv().
>
ACK.
> > + ret = regmap_read(data->regmap, BMP380_REG_STATUS, ®);
> > + if (ret) {
> > + dev_err(data->dev, "failed to read status register\n");
> > + return ret;
> > + }
>
> > +
>
> Choose one style (with or without blank line), as in the above you have no
> blank line in the similar situation.
>
I didn't even notice it, you are right.
> > + 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;
> > + }
> > +
> > + return 0;
> > +}
>
> ...
>
> > + usleep_range(data->start_up_time, data->start_up_time + 500);
>
> fsleep() ? Comment?
>
I could use fsleep(). I didn't add a comment because also before it was
also like this. The code just used hardcoded (2000,2500) while I used
the data->start_up_time. It is mentioned in the datasheet, I could add it.
> ...
>
> > +static const u8 bmp580_operation_mode[] = { BMP580_MODE_SLEEP,
> > + BMP580_MODE_FORCED,
> > + BMP580_MODE_NORMAL };
>
> As per above.
>
ACK.
> ...
>
> > + switch (mode) {
> > + case BMP280_SLEEP:
> > + break;
> > + case BMP280_FORCED:
> > + ret = regmap_set_bits(data->regmap, BMP580_REG_DSP_CONFIG,
> > + BMP580_DSP_IIR_FORCED_FLUSH);
> > + if (ret) {
> > + dev_err(data->dev,
> > + "Could not flush IIR filter constants.\n");
> > + return ret;
> > + }
> > + break;
> > + case BMP280_NORMAL:
> > + break;
>
> Can be unified with _SLEEP case.
>
ACK.
> > + default:
> > + return -EINVAL;
> > + }
>
> ...
>
> > +static int bmp580_wait_conv(struct bmp280_data *data)
> > +{
> > + /*
> > + * Taken from datasheet, Section 2 "Specification, Table 3 "Electrical
> > + * characteristics
>
> Missing period.
ACK.
>
> > + */
> > + 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};
>
> Please, start values on the next line after {. Also make }; to be on a separate line.
>
ACK.
> > + int meas_time;
> > +
> > + meas_time = 4000 + time_conv_temp[data->oversampling_temp] +
> > + time_conv_press[data->oversampling_press];
>
> 4 * USEC_PER_MSEC ?
Since the previous values in the arrays are all in thousands, why should
I make this different?
>
> > + usleep_range(meas_time, meas_time * 12 / 10);
>
> Comment? fsleep() ?
>
The usleep here is for waiting for the sensor to make the conversion,
as the function name points out as well? Should I put it as a comment?
In general, is it considered good practice to add comments above all
sleep functions? I don't think it's a bad idea, I just didn't notice
it somewhere.
> > + return 0;
> > +}
>
> ...
>
> > + usleep_range(2500, 3000);
>
> fsleep() ?
>
ACK.
> --
> With Best Regards,
> Andy Shevchenko
>
>
Cheers,
Vasilis
