> Hi Himanshu, > > This was close to the point where I'd take it and make the few remaining > fixes myself. I'm still bothered however by the fact the casts in the > various calibration functions are still not all justified so please take > another look at that. Frankly it looks like the original author > threw in casts because they didn't want to have to think about which ones > actually do anything! Ok. I will remove the ones mentioned below. > Few other things to fix up for v5 as well. I will send the fixes in an hour. > Jonathan > > > > +#define BME680_NB_CONV_MASK GENMASK(3, 0) > > +#define BME680_RUN_GAS_EN_BIT BIT(4) > > odd looking spacing above. I don't know why this is showing like that in the diff output, but I have checked the code by apllying to my test tree(git am <patch>) and there was no such spurious spacing! > > +/* > > + * Taken from Bosch BME680 API: > > + * https://github.com/BoschSensortec/BME680_driver/blob/63bb5336/bme680.c#L937 > > + * > > + * Returns humidity measurement in percent, resolution is 0.001 percent. Output > > + * value of "43215" represents 43.215 %rH. > > + */ > > +static u32 bme680_compensate_humid(struct bme680_data *data, > > + u16 adc_humid) > > +{ > > + struct bme680_calib *calib = &data->bme680; > > + s32 var1, var2, var3, var4, var5, var6, temp_scaled, calc_hum; > > + > There is still substantial over the top casting in here. > > data->t_fine is already a 32 bit integer for example. Will do. > > + temp_scaled = (((s32) data->t_fine * 5) + 128) >> 8; > > + var1 = (adc_humid - ((s32) ((s32) calib->par_h1 * 16))) - > > the outer s32 looks like it won't do anything to me as the inner bit > will already be an s32. OK. > > + (((temp_scaled * (s32) calib->par_h3) / 100) >> 1); > > + var2 = ((s32) calib->par_h2 * (((temp_scaled * (s32) calib->par_h4) / > > + ((s32) 100)) + (((temp_scaled * ((temp_scaled * > > + (s32) calib->par_h5) / 100)) >> 6) / 100) + > > + (s32) (1 << 14))) >> 10; > > Lots more casts of dubious merit in here.. Ok. > > + var3 = var1 * var2; > > + var4 = (s32) calib->par_h6 << 7; > > + var4 = (var4 + ((temp_scaled * (s32) calib->par_h7) / 100)) >> 4; > > + var5 = ((var3 >> 14) * (var3 >> 14)) >> 10; > > + var6 = (var4 * var5) >> 1; > > + calc_hum = (((var3 + var6) >> 10) * 1000) >> 12; > > + > > + if (calc_hum > 100000) /* Cap at 100%rH */ > > + calc_hum = 100000; > > + else if (calc_hum < 0) > > + calc_hum = 0; > > + > > + return calc_hum; > > +} > > +static u32 bme680_compensate_gas(struct bme680_data *data, u16 gas_res_adc, > > + u8 gas_range) > > +{ > > + struct bme680_calib *calib = &data->bme680; > > + s64 var1; > > + u64 var2; > > + s64 var3; > > + u32 calc_gas_res; > > + > > + /* Look up table 1 for the possible gas range values */ > > + u32 lookupTable1[16] = {2147483647u, 2147483647u, 2147483647u, > > + 2147483647u, 2147483647u, 2126008810u, > > + 2147483647u, 2130303777u, 2147483647u, > > + 2147483647u, 2143188679u, 2136746228u, > > + 2147483647u, 2126008810u, 2147483647u, > > + 2147483647u}; > > + /* Look up table 2 for the possible gas range values */ > > + u32 lookupTable2[16] = {4096000000u, 2048000000u, 1024000000u, > > + 512000000u, 255744255u, 127110228u, 64000000u, > > + 32258064u, 16016016u, 8000000u, 4000000u, > > + 2000000u, 1000000u, 500000u, 250000u, 125000u}; > > You can mark these two arrays as const I think. Sure. > > + > > + var1 = ((1340 + (5 * (s64) calib->range_sw_err)) * > > + ((s64) lookupTable1[gas_range])) >> 16; > > + var2 = (((s64) ((s64) gas_res_adc << 15) - 16777216) + var1); > > Unless something odd is going on the outer cast just casts the bit > that has already been forced to s64 to s64 so is pointless. Ok. > > + var3 = (((s64) lookupTable2[gas_range] * (s64) var1) >> 9); > > + calc_gas_res = (u32) ((var3 + ((s64) var2 >> 1)) / (s64) var2); > > 64 bit division, does that need to be do_div so as to support 32 bit > platforms? Also, why does var 2 want to be cast to a 64 bit? > You'll need to go back to 32 bit anyway to use do_div. I will change to do_div(). > > + > > + if ((check & BME680_GAS_STAB_BIT) == 0) { > > + /* > > + * occurs if either the gas heating duration was insuffient > > + * to reach the target heater temperature or the target > > + * heater temperature was too high for the heater sink to > > + * reach. > > + */ > > Odd comment indenting. I would move it before the if to make it > more 'natural'. Still clearly applies to this block without looking 'odd'. Will do. > > +static int bme680_read_raw(struct iio_dev *indio_dev, > > + struct iio_chan_spec const *chan, > > + int *val, int *val2, long mask) > > +{ > > + struct bme680_data *data = iio_priv(indio_dev); > > + > > + switch (mask) { > > + case IIO_CHAN_INFO_PROCESSED: > > + switch (chan->type) { > > + case IIO_TEMP: > > + return bme680_read_temp(data, val, val2); > > + case IIO_PRESSURE: > > + return bme680_read_press(data, val, val2); > > + case IIO_HUMIDITYRELATIVE: > > + return bme680_read_humid(data, val, val2); > > + case IIO_RESISTANCE: > > + return bme680_read_gas(data, val); > > + default: > > + return -EINVAL; > > + } > > + case IIO_CHAN_INFO_OVERSAMPLING_RATIO: > > + switch (chan->type) { > > + case IIO_TEMP: > > + *val = 1 << data->oversampling_temp; > > + return IIO_VAL_INT; > > + case IIO_PRESSURE: > > + *val = 1 << data->oversampling_press; > > + return IIO_VAL_INT; > > + case IIO_HUMIDITYRELATIVE: > > + *val = 1 << data->oversampling_humid; > > + return IIO_VAL_INT; > > + default: > > + return -EINVAL; > > + } > > + default: > > + return -EINVAL; > > + } > > + > > Don't think you can get here so this code should not be here. Ok. > > + default: > > + return -EINVAL; > > + } > > + default: > > + return -EINVAL; > > + } > > + > > You can't get here so no point in having this return. I'll delete it. OK. > > + ret = devm_add_action(dev, bme680_core_remove, indio_dev); > > + if (ret < 0) { > > + dev_err(dev, "failed to register remove action\n"); > > + return ret; > > + } > > I think this is logically in the wrong place. The things it's actually > doing is unwinding the register below, but at this stage you haven't > performed that registration. > > If this is all I fine, I 'might' move it and apply anyway. > > I'm assuming that there will shortly be more in there than a simple > unregister (otherwise you could just have used devm_iio_device_register... > > I'd prefer that for now you just use devm_iio_device_register > and drop this until you need it. OK. > > + > > + ret = regmap_write(regmap, BME680_REG_SOFT_RESET_I2C, > > + BME680_CMD_SOFTRESET); > > + if (ret < 0) > > It's not something I'm that bothered by, but you are a little random > in whether a given error outputs a debug message or not... I missed this. Will fix. -- Himanshu Jha Undergraduate Student Department of Electronics & Communication Guru Tegh Bahadur Institute of Technology -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
