On Thu, Aug 29, 2024 at 03:31:48PM +0300, Andy Shevchenko wrote:
> On Wed, Aug 28, 2024 at 10:51:24PM +0200, Vasileios Amoiridis wrote:
> > This commit adds forced mode support in sensors BMP28x, BME28x, BMP3xx
>
> s/This commit, adds/Add/
>
> The imperative mode is documented in Submitting Patches.
>
> > 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
>
> Déjà-vu feeling... Ah, first line is the same!
>
I see your point, I can work this out better.
> > 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 int bmp280_wait_conv(struct bmp280_data *data)
> > +{
> > + unsigned int reg;
>
> > + int ret, meas_time;
>
> Why meas_time is signed?
> Also, please name it with a unit suffix
No reason, I can make it unsigned. Unit suffix is a good addition
indeed!!!
>
> unsigned int meas_time_us;
>
> (and check the rest of the patch for the similar).
>
True, thanks!!!
> > +
> > +
>
> A single blank line is enough. Also check all patches for this.
>
ACK.
> > + /* Check if we are using a BME280 device */
> > + if (data->oversampling_humid)
>
> > + meas_time += BIT(data->oversampling_humid) * BMP280_MEAS_DUR +
> > + BMP280_PRESS_HUMID_MEAS_OFFSET;
>
> Indentation issue, the same seems in all of similar expressions in this patch.
>
It seems I have indentation issues in other places as well.
I think I remember checkpatch.pl informing me about those but I didn't
got anything back...
> Also play with this form and check if it looks better
>
> meas_time += BMP280_PRESS_HUMID_MEAS_OFFSET +
> BIT(data->oversampling_humid) * BMP280_MEAS_DUR;
>
> (at least I found it better to read as first we apply constants, followed by
> longer variable-based calculations).
>
I see your point, I can try it.
> > + /* Pressure measurement time */
> > + meas_time += BIT(data->oversampling_press) * BMP280_MEAS_DUR +
> > + BMP280_PRESS_HUMID_MEAS_OFFSET;
> > +
> > + /* Temperature measurement time */
> > + meas_time += BIT(data->oversampling_temp) * BMP280_MEAS_DUR;
> > +
> > + /* Waiting time according to the BM(P/E)2 Sensor API */
> > + fsleep(meas_time);
> > +
> > + 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;
> > +}
>
> ...
>
> int bmp280_chip_config(struct bmp280_data *data)
>
> > BMP280_OSRS_TEMP_MASK |
> > BMP280_OSRS_PRESS_MASK |
> > BMP280_MODE_MASK,
> > - osrs | BMP280_MODE_NORMAL);
> > + osrs | BMP280_MODE_SLEEP);
> > if (ret) {
> > dev_err(data->dev, "failed to write ctrl_meas register\n");
> > return ret;
>
> This _feels_ like a separate change. I haven't found anything explicitly
> describing it in the commit message. Did I miss it?
>
Well this change is because before, the sensor was by default in
NORMAL_MODE so whenever we were writing a different setting (Output
data rate, oversampling ratio) to the sensor, the NORMAL_MODE was
chosen. There was no idea of SLEEP or FORCED MODE.
While now, since this commits adds the idea of SLEEP_MODE
by default (FORCED_MODE for oneshot captures, and NORMAL_MODE for
buffer/trigger) we need to keep the sensor in SLEEP_MODE as well
when we change its configuration.
I believe it belongs to this commit. Maybe though, I should mention
this change explicitly in the commit message?
> ...
>
> > + /*
> > + * According to the BMP3 Sensor API, the sensor needs 5000ms
>
> I believe it's a typo in unit suffix.
>
Yes indeed its a typo, I wanted to say 5000us. The fsleep(5000) is correct.
> If not, this should be very well described to explain why 5 seconds is needed.
>
> > + * in order to go to the sleep mode.
> > + */
> > + fsleep(5000);
>
> ...
>
> > +{
> > + int ret;
> > +
> > + switch (mode) {
> > + case BMP280_SLEEP:
> > + case BMP280_NORMAL:
> > + 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");
>
> Temporary variable for data->dev?
>
That could help, yeah!
> > + return ret;
> > + }
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + ret = regmap_write_bits(data->regmap, BMP580_REG_ODR_CONFIG,
> > + BMP580_MODE_MASK,
> > + FIELD_PREP(BMP580_MODE_MASK,
> > + bmp580_operation_mode[mode]));
> > + if (ret) {
> > + dev_err(data->dev, "failed to write power control register\n");
> > + return ret;
> > + }
> > +
> > + data->op_mode = mode;
> > +
> > + 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
> > + };
>
> Mind the comma at the end.
>
ACK.
> And in programming hardware we quite often operate with power-of-2 things, so I
> recommend to have 8 per line,
>
> static const int time_conv_press[] = {
> 0, 1050, 1785, 3045, 5670, 10920, 21420, 42420, /* 0-7 */
> 84420, /* 8 */
> };
>
I was not aware of this convention, I can do it.
> > + static const int time_conv_temp[] = {
> > + 0, 1050, 1105, 1575, 2205, 3465, 6090, 11340, 21840
> > + };
>
> Ditto.
>
ACK.
> > +
>
> Stray blank line. This is a definition block, we don't need blank lines in it.
>
ACK.
> > + int meas_time;
> > +
> > + meas_time = 4 * USEC_PER_MSEC + time_conv_temp[data->oversampling_temp]
> > + + time_conv_press[data->oversampling_press];
> > +
> > + /* Measurement time mentioned in Chapter 2, Table 4 of the datasheet. */
> > + fsleep(meas_time);
> > +
> > + return 0;
> > +}
>
> ...
>
> > /* From datasheet's table 4: electrical characteristics */
>
> With this change the comment seems odd. Can you elaborate more?
>
I can elaborate more in the comment yes.
> > - usleep_range(2500, 3000);
> > + fsleep(data->start_up_time + 500);
>
> Also, can we name it start_up_time_us?
> It's fine to postpone renaming if it takes too many unrelated changes.
>
I can maybe do it in a separate commit because you have already pointed
out things that could be improved with styling.
> ...
>
> > + usleep_range(2500, 3000);
>
> fsleep()? Comment?
>
ACK.
> ...
>
> > usleep_range(data->start_up_time, data->start_up_time + 100);
>
> This is already in the code, but maybe switching to fsleep() and adding
> a comment to explain how it's calculated (based on the spec? Reference?),
> so in a separate change?
>
Yes, that would be good!!!
Cheers,
Vasilis
> --
> With Best Regards,
> Andy Shevchenko
>
>
