Messages in this thread | | | Date | Wed, 1 Mar 2023 17:30:03 +0200 | From | Andy Shevchenko <> | Subject | Re: [PATCH v3 2/2] iio: adc: Add TI ADS1100 and ADS1000 |
| |
On Tue, Feb 28, 2023 at 07:31:51AM +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.
...
> + /* Shift result to compensate for bit resolution vs. sample rate */ > + value <<= 16 - ads1100_data_bits(data); > + *val = sign_extend32(value, 15);
Why not simply
*val = sign_extend32(value, ads1100_data_bits(data) - 1);
?
(Double check for off-by-one usage)
...
> + /* Calculate: gain = ((microvolts / 1000) / (val2 / 1000000)) >> 15 */
Can you use more math / plain English to describe the formula? Otherwise we can see the very same in the code and point of the comment is doubtful.
> + gain = ((microvolts + BIT(14)) >> 15) * 1000 / val2;
Something from units.h?
...
> + for (i = 0; i < 4; i++) { > + if (BIT(i) == gain) {
ffs()/__ffs() (look at the documentation for the difference and use proper one).
> + ads1100_set_config_bits(data, ADS1100_PGA_MASK, i); > + return 0; > + } > + }
...
> + for (i = 0; i < size; ++i) {
Why pre-increment?
> + if (ads1100_data_rate[i] == rate) { > + return ads1100_set_config_bits( > + data, ADS1100_DR_MASK,
Strange indentation.
> + FIELD_PREP(ADS1100_DR_MASK, i)); > + }
Do you need {} ?
> + }
...
> + int millivolts = regulator_get_voltage(data->reg_vdd) / 1000;
units.h?
...
> + data->scale_avail[i * 2] = millivolts;
I would write ' * 2 + 0]', but it's up to you.
> + data->scale_avail[i * 2 + 1] = 15 + i;
...
> + *val = regulator_get_voltage(data->reg_vdd) / 1000;
units.h?
...
> + *val = ads1100_data_rate[ > + FIELD_GET(ADS1100_DR_MASK, data->config)];
Strange indentation, just use a single line.
...
> + ret = devm_iio_device_register(dev, indio_dev); > + if (ret < 0)
Why ' < 0'?
> + return dev_err_probe(dev, ret, > + "Failed to register IIO device\n");
...
> +static int ads1100_runtime_suspend(struct device *dev) > +{ > + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev)); > + struct ads1100_data *data = iio_priv(indio_dev); > + > + ads1100_set_config_bits(data, ADS1100_CFG_SC, ADS1100_SINGLESHOT); > + regulator_disable(data->reg_vdd);
Wrong devm / non-devm ordering.
> + return 0; > +}
...
> +static const struct i2c_device_id ads1100_id[] = { > + { "ads1100", }, > + { "ads1000", },
Inner commas are not needed.
> + {} > +};
...
> +static const struct of_device_id ads1100_of_match[] = { > + { .compatible = "ti,ads1100", }, > + { .compatible = "ti,ads1000", },
Ditto.
> + {} > +};
...
> +
Redundant blank line.
> +module_i2c_driver(ads1100_driver);
-- With Best Regards, Andy Shevchenko
| |