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

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <ceb4f367-b910-578a-24bc-7c222e2f0b0f@topic.nl>
Date:   Mon, 6 Mar 2023 07:31:36 +0100
From:   Mike Looijmans <mike.looijmans@...ic.nl>
To:     Jonathan Cameron <jic23@...nel.org>
CC:     devicetree@...r.kernel.org, linux-iio@...r.kernel.org,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Caleb Connolly <caleb.connolly@...aro.org>,
        ChiYuan Huang <cy_huang@...htek.com>,
        ChiaEn Wu <chiaen_wu@...htek.com>,
        Cosmin Tanislav <demonsingur@...il.com>,
        Ibrahim Tilki <Ibrahim.Tilki@...log.com>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Ramona Bolboaca <ramona.bolboaca@...log.com>,
        William Breathitt Gray <william.gray@...aro.org>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 2/2] iio: adc: Add TI ADS1100 and ADS1000


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 04-03-2023 18:57, Jonathan Cameron wrote:
> On Tue, 28 Feb 2023 07:31:51 +0100
> Mike Looijmans <mike.looijmans@...ic.nl> wrote:
>
>> The ADS1100 is a 16-bit ADC (at 8 samples per second).
>> The ADS1000 is similar, but has a fixed data rate.
>>
>> Signed-off-by: Mike Looijmans <mike.looijmans@...ic.nl>
> Hi Mike,
>
> A few minor things + one request for a test as trying to chase a possible
> ref count overflow around the runtime_pm was giving me a enough of a headache
> that it's easier to ask you just to poke it and see.  If it doesn't fail as
> I expect I'll take a closer look!

Will do, but it may take a few days to get access to the hardware again. 
I'll report on that later.

>> +static int ads1100_set_scale(struct ads1100_data *data, int val, int val2)
>> +{
>> +	int microvolts;
>> +	int gain;
>> +	int i;
>> +
>> +	/* With Vdd between 2.7 and 5V, the scale is always below 1 */
>> +	if (val)
>> +		return -EINVAL;
>> +
>> +	microvolts = regulator_get_voltage(data->reg_vdd);
>> +	/* Calculate: gain = ((microvolts / 1000) / (val2 / 1000000)) >> 15 */
>> +	gain = ((microvolts + BIT(14)) >> 15) * 1000 / val2;
>> +
>> +	for (i = 0; i < 4; i++) {
>> +		if (BIT(i) == gain) {
>> +			ads1100_set_config_bits(data, ADS1100_PGA_MASK, i);
>> +			return 0;
>> +		}
>> +	}
> Andy's suggestion of something like..
> 	if (!gain)
> 		return -EINVAL;
> 	i = ffs(gain);
> 	if (i >= 4 || BIT(i) != gain)
> 		return -EINVAL;
>
> 	ads...
>
> Is perhaps nicer than the loop.

Yes, takes out a loop.


>
>> +static void ads1100_disable_continuous(void *data)
>> +{
>> +	ads1100_set_config_bits(data, ADS1100_CFG_SC, ADS1100_SINGLESHOT);
>> +}
>> +
>> +static int ads1100_probe(struct i2c_client *client)
>> +{
>> +	struct iio_dev *indio_dev;
>> +	struct ads1100_data *data;
>> +	struct device *dev = &client->dev;
>> +	int ret;
>> +
>> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
>> +	if (!indio_dev)
>> +		return -ENOMEM;
>> +
>> +	data = iio_priv(indio_dev);
>> +	i2c_set_clientdata(client, indio_dev);
> You can avoid the slightly nasty mix of i2c_set_clientdata vs dev_get_drvdata()
> below by taking advantage of the fact you have a local dev pointer.
>
> 	dev_set_drvdata(dev, indio_dev);
> and no confusing mix is left.  Of course it's doing the same thing but to my
> mind slightly nicer to use the same one.

Since I don't need indio_dev anywhere, I might as well say this directly:

dev_set_drvdata(dev, data);

(Only the pm_ routines use this)

> ...
>

-- 
Mike Looijmans