Re: [PATCH v3 2/2] iio: adc: Add TI ADS1100 and ADS1000

[Mike Looijmans <mike.looijmans@topic.nl>]

Comments below. Mailserver has a top-post fetish and will inject a 
signature here somewhere...

No further comment from me means "agree, will implement in v4"...



Met vriendelijke groet / kind regards,

Mike Looijmans
System Expert


TOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands

T: +31 (0) 499 33 69 69
E: mike.looijmans@topicproducts.com
W: www.topic.nl

Please consider the environment before printing this e-mail
On 01-03-2023 16:30, Andy Shevchenko wrote:
> 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);
>
> ?

As discussed with  Jonathan Cameron, the register is right-justified and 
the number of bits depend on the data rate. Rather than having the 
"scale" change when the sample rate changes, we chose to adjust the 
sample result so it's always left-justified.


>> +	/* 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.

I'll try to explain it better.


>
>> +	gain = ((microvolts + BIT(14)) >> 15) * 1000 / val2;
> Something from units.h?

Could put MILLI here, but I doubt if that improves things. Its actually 
MICRO/(MICRO/MILLI) given the explanation above... Not helping much...


> ...
>
>> +	for (i = 0; i < 4; i++) {
>> +		if (BIT(i) == gain) {
> ffs()/__ffs() (look at the documentation for the difference and use proper one).

Thought of it, but I'd rather have it return EINVAL for attempting to 
set the analog gain to "7" (0nly 1,2,4,8 allowed).


> ...
>> +	for (i = 0; i < size; ++i) {
> Why pre-increment?

Spent too much time with other coding guidelines, missed this one... 
Will change.


>
> ...
>
>> +	int millivolts = regulator_get_voltage(data->reg_vdd) / 1000;
> units.h?

Should I write:

regulator_get_voltage(data->reg_vdd) / (MICROS / MILLIS);

I doubt that improves readability.

> ...
> ...
>
>> +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.

Don't understand your remark, can you explain further please?

devm / non-devm ordering would be related to the "probe" function. As 
far as I can tell, I'm not allocating resources after the devm calls. 
And the "remove" is empty.



-- 
Mike Looijmans