On Wed, 20 Mar 2024 18:46:02 +0100 Vasileios Amoiridis <vassilisamir@xxxxxxxxx> wrote: > On Wed, Mar 20, 2024 at 01:16:03PM +0200, Andy Shevchenko wrote: > > On Tue, Mar 19, 2024 at 01:29:25AM +0100, Vasileios Amoiridis wrote: > > > BMP2xx, BMP3xx, and BMP5xx use consecutive buffers for their > > > temperature, pressure and humidity readings. This facilitates > > > the use of burst reads in order to acquire data much faster > > > and in a different way from the one used in oneshot captures. > > > > > > BMP085 and BMP180 use a completely different measurement > > > process that is well defined and is used in their buffer_handler(). > > > > ... > > > > > ret = regmap_bulk_read(data->regmap, BMP280_REG_TEMP_MSB, > > > - data->buf, sizeof(data->buf)); > > > + data->buf, BMP280_NUM_TEMP_BYTES); > > > > > ret = regmap_bulk_read(data->regmap, BMP280_REG_PRESS_MSB, > > > - data->buf, sizeof(data->buf)); > > > + data->buf, BMP280_NUM_PRESS_BYTES); > > > > > ret = regmap_bulk_read(data->regmap, BMP280_REG_HUMIDITY_MSB, > > > - &data->be16, sizeof(data->be16)); > > > + &data->be16, BME280_NUM_HUMIDITY_BYTES); > > > > > - adc_humidity = be16_to_cpu(data->be16); > > > + adc_humidity = get_unaligned_be16(&data->be16); > > > > > ret = regmap_bulk_read(data->regmap, BMP380_REG_TEMP_XLSB, > > > - data->buf, sizeof(data->buf)); > > > + data->buf, BMP280_NUM_TEMP_BYTES); > > > > > ret = regmap_bulk_read(data->regmap, BMP380_REG_PRESS_XLSB, > > > - data->buf, sizeof(data->buf)); > > > + data->buf, BMP280_NUM_PRESS_BYTES); > > > > > ret = regmap_bulk_read(data->regmap, BMP580_REG_TEMP_XLSB, data->buf, > > > - sizeof(data->buf)); > > > + BMP280_NUM_TEMP_BYTES); > > > > > ret = regmap_bulk_read(data->regmap, BMP580_REG_PRESS_XLSB, data->buf, > > > - sizeof(data->buf)); > > > + BMP280_NUM_PRESS_BYTES); > > > > These smell to me as candidates to a separate patch with more explanation why. > > (Yes, with the definitions you introduced.) But I leave it to Jonathan to > > decide if we need to split. The are somewhat confusing, though only when you start doing bulk reads to do these makes sense - so I'm not sure how to do it as 2 patches. Pity we don't have sizeof(be24) available. > > > > ... > > > > The below are applicable to the bmp280_buffer_handler(), > > bmp380_buffer_handler() implementations as well. > > > > ... > > > > > + /* Burst read data registers */ > > > + ret = regmap_bulk_read(data->regmap, BMP580_REG_TEMP_XLSB, > > > + data->buf, 6); > > > > Magic size. > > > > Hi Andy, > > Thank you again for your feedback. When I was writing it, it was > looking as a magic number to me as well but then I though that > since I put the comment above it could be obvious. Now that I see > it again, I think it was not a good idea and maybe some type of > definition like > > #define BMP280_BURST_READ_NUM_BYTES 6 > #define BME280_BURST_READ_NUM_BYTES 8 I think these are sums of the other quantities. Better to express them as such if possible. > > could look better and be more intuitive. > > > ... > > > > > + /* Temperature calculations */ > > > + memcpy(&chan_value, &data->buf[3], 3); > > > > _le24() + sign_extend32()? > > > > In the next line from your comment the _le24 or _be24 takes place. I think you can get that data directly rather than bouncing it via a memcpy. e.g. adc_temp = FIELD_GET(BMP280_MEAS_TRIM_MASK, get_unaligned_be24(&data->buf[3]); > If the sign_extend32() is needed here, shouldn't it also be used > in all the oneshot captures as well? Is this actually signed? Compensated values are, but at this point it's just a raw adc count. So I'm not seeing why we'd sign extend it (unless the device is returning a signed be24 - I haven't checked.) > > > ... > > > > > + /* Pressure calculations */ > > > + memcpy(&chan_value, &data->buf[0], 3); > > > > _le24() + sign_extend32()? > > > > ... > > > > > /* > > > - * Maximum number of consecutive bytes read for a temperature or > > > - * pressure measurement is 3. > > > + * Maximum number of a burst read for temperature, pressure, humidity > > > + * is 8 bytes. > > > */ > > > - if (val_size > 3) > > > + if (val_size > 8) > > > > sizeof() / new definition for the buf[] size? > > > > In a previous commit that I was fixing this SPI driver, Jonathan had mentioned > that there is no need for a specific definition since it will only be used > here so that's why I kept it as is. Never trust me ;) Size of the buf is sensible here. Jonathan > > Cheers, > Vasilis > > > return -EINVAL; > > > > -- > > With Best Regards, > > Andy Shevchenko > > > >
