linux-kernel - Re: [PATCH 2/2] iio: adc: Add driver for TI ADS1100 and ADS1000 chips

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <b3fde4a8-e344-9d3d-0b1e-9a11c4dd09de@topic.nl>
Date:   Fri, 17 Feb 2023 16:10:49 +0100
From:   Mike Looijmans <mike.looijmans@...ic.nl>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
CC:     devicetree@...r.kernel.org, linux-iio@...r.kernel.org,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>,
        Caleb Connolly <caleb.connolly@...aro.org>,
        ChiYuan Huang <cy_huang@...htek.com>,
        ChiaEn Wu <chiaen_wu@...htek.com>,
        Cosmin Tanislav <demonsingur@...il.com>,
        Geert Uytterhoeven <geert+renesas@...der.be>,
        Ibrahim Tilki <Ibrahim.Tilki@...log.com>,
        Jonathan Cameron <jic23@...nel.org>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Marcelo Schmitt <marcelo.schmitt1@...il.com>,
        Ramona Bolboaca <ramona.bolboaca@...log.com>,
        William Breathitt Gray <william.gray@...aro.org>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] iio: adc: Add driver for TI ADS1100 and ADS1000 chips

Thanks for your quick feedback. Replies below (skipping signature added 
by borked mailserver)
I'll post a v2, want to do some testing first with the changes and I'll 
have hardware access by the end of next week.
No further comment from me means I agree and will change as requested.

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@...icproducts.com
W: www.topic.nl

Please consider the environment before printing this e-mail
On 17-02-2023 13:29, Andy Shevchenko wrote:
> On Fri, Feb 17, 2023 at 10:31:28AM +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.
> Any Datasheet link available?

Yes, will add a friendly link.

> ...
>
>> +static const int ads1100_data_rate[] = {128, 32, 16, 8};
>> +static const int ads1100_data_rate_scale[] = {2048, 8192, 16384, 32768};
>> +static const int ads1100_gain[] = {1, 2, 4, 8};
> Do you need all of them as tables? They all can be derived from a single table
> or without any table at all (just three values).

I think the "data_rate" tables make the driver smaller in size, it's not 
a straight power-of-two list. I want them anyway for passing as "avail" 
sequences.

The "gain" is a simple power-of-two and might be replaced with code, but 
makes the "avail" more complex. Even in this case, the code to generate 
the list will be larger than the list itself, and I need the list in 
memory for the IIO_AVAIL function anyway.

> ...
>
>> +#ifdef CONFIG_PM
> Why?

I based this on the ADS1015 driver. Which appears to be outdated, I'll 
convert to DEFINE_RUNTIME_DEV_PM_OPS

+ int ret;
>> +	u8 buffer[2];
> __be16 buffer;
>
>> +
>> +	if (chan != 0)
>> +		return -EINVAL;
>> +
>> +	ret = i2c_master_recv(data->client, buffer, sizeof(buffer));
>> +	if (ret < 0) {
>> +		dev_err(&data->client->dev, "I2C read fail: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	*val = (s16)(((u16)buffer[0] << 8) | buffer[1]);
> 	(s16)be16_to_cpu();
>
> But (s16) looks suspicious. Should you use sign_extend32()?

The chip always returns a 16-bit 2's complement value, even when only 12 
bits are actually used. I'll use sign_extend anyway, just to improve 
readability and take away doubts such as these.

-- 

Mike Looijmans