Messages in this thread | | | Date | Mon, 6 Mar 2023 07:31:36 +0100 | From | Mike Looijmans <> | 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@topicproducts.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@topic.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@topic.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
| |