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 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 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. If the sign_extend32() is needed here, shouldn't it also be used in all the oneshot captures as well? > ... > > > + /* 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. Cheers, Vasilis > > return -EINVAL; > > -- > With Best Regards, > Andy Shevchenko > >
