Re: [PATCH v3 2/2] iio: adc: Add TI ADS1100 and ADS1000
From: Andy Shevchenko
Date: Wed Mar 01 2023 - 10:30:14 EST
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