On Mon, Mar 06, 2023 at 02:13:12PM +0100, Mike Looijmans wrote: > The ADS1100 is a 16-bit ADC (at 8 samples per second). > The ADS1000 is similar, but has a fixed data rate. ... > + /* Value is always 16-bit 2's complement */ > + value = be16_to_cpu(buffer); + Blank line? > + /* Shift result to compensate for bit resolution vs. sample rate */ > + value <<= 16 - ads1100_data_bits(data); + Blank line? > + *val = sign_extend32(value, 15); ... > + microvolts = regulator_get_voltage(data->reg_vdd); > + /* > + * val2 is in 'micro' units, n = val2 / 1000000 > + * result must be millivolts, d = microvolts / 1000 > + * the full-scale value is d/n, corresponds to 2^15, > + * hence the gain = (d / n) >> 15, factoring out the 1000 and moving the > + * bitshift so everything fits in 32-bits yields this formula. > + */ > + gain = ((microvolts + BIT(14)) >> 15) * 1000 / val2; Perhaps adding MICROVOLT_PER_MILLIVOLT (to units.h) and use it here? Besides that it's seems like microvolts = regulator_get_voltage(data->reg_vdd); gain = DIV_ROUNDUP_CLOSEST(microvolts, BIT(15)) * MICROVOLT_PER_MILLIVOLT / val2; > + if (gain <= 0 || gain > 8) > + return -EINVAL; As I commented out in the previous discussion (please, give a chance to the reviewers to answer before issuing a new version of the series) this better to be if (gain < BIT(0) || gain > BIT(3)) which will show the nature of power of two implicitly. > + regval = ffs(gain) - 1; > + ads1100_set_config_bits(data, ADS1100_PGA_MASK, regval); Can be unified in one line. > + return 0; > +} ... > + return ads1100_set_config_bits( > + data, ADS1100_DR_MASK, > + FIELD_PREP(ADS1100_DR_MASK, i)); Wrong indentation. Please, check all your code for this kind of issues. -- With Best Regards, Andy Shevchenko
