Date:   Wed, 1 Mar 2023 17:30:03 +0200
From:   Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To:     Mike Looijmans <mike.looijmans@topic.nl>
Cc:     devicetree@vger.kernel.org, linux-iio@vger.kernel.org,
        Caleb Connolly <caleb.connolly@linaro.org>,
        ChiYuan Huang <cy_huang@richtek.com>,
        ChiaEn Wu <chiaen_wu@richtek.com>,
        Cosmin Tanislav <demonsingur@gmail.com>,
        Ibrahim Tilki <Ibrahim.Tilki@analog.com>,
        Jonathan Cameron <jic23@kernel.org>,
        Lars-Peter Clausen <lars@metafoo.de>,
        Ramona Bolboaca <ramona.bolboaca@analog.com>,
        William Breathitt Gray <william.gray@linaro.org>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 2/2] iio: adc: Add TI ADS1100 and ADS1000
Message-ID: <Y/9vez/fzLD5dRVF@smile.fi.intel.com>
References: <20230228063151.17598-1-mike.looijmans@topic.nl>
 <20230228063151.17598-2-mike.looijmans@topic.nl>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230228063151.17598-2-mike.looijmans@topic.nl>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
Precedence: bulk

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