| 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
| ||
|
Date: Thu, 12 Jan 2023 19:00:30 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: Hugo Villeneuve <hugo@...ovil.com>
Cc: hvilleneuve@...onoff.com, lars@...afoo.de, robh+dt@...nel.org,
krzysztof.kozlowski+dt@...aro.org, linux-iio@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/2] iio: adc: ti-ads7924: add Texas Instruments
ADS7924 driver
On Tue, 10 Jan 2023 11:01:23 -0500
Hugo Villeneuve <hugo@...ovil.com> wrote:
> From: Hugo Villeneuve <hvilleneuve@...onoff.com>
>
> The Texas Instruments ADS7924 is a 4 channels, 12-bit analog to
> digital converter (ADC) with an I2C interface.
>
> Datasheet: https://www.ti.com/lit/gpn/ads7924
>
> Signed-off-by: Hugo Villeneuve <hvilleneuve@...onoff.com>
Hi Hugo,
A few minor additional comments from me as you are doing a v3
(I might just have tidied them up whilst applying otherwise to save
us all time!)
Thanks,
Jonathan
> diff --git a/drivers/iio/adc/ti-ads7924.c b/drivers/iio/adc/ti-ads7924.c
> new file mode 100644
> index 000000000000..8384f3451236
> --- /dev/null
> +++ b/drivers/iio/adc/ti-ads7924.c
> @@ -0,0 +1,492 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * IIO driver for Texas Instruments ADS7924 ADC, 12-bit, 4-Channels, I2C
> + *
> + * iio/adc/ti-ads7924.c
Not a huge amount of point in having a file name in the file. I don't mind
if you really want it though.
> + * Author: Hugo Villeneuve <hvilleneuve@...onoff.com>
> + * Copyright 2022 DimOnOff
> + *
> + * based on iio/adc/ti-ads1015.c
> + * Copyright (c) 2016, Intel Corporation.
> + *
> + * Datasheet: https://www.ti.com/lit/gpn/ads7924
> + */
> +
> +#define ADS7924_TOTAL_CONVTIME_US (ADS7924_PWRUPTIME_US + ADS7924_ACQTIME_US + \
> + ADS7924_CONVTIME_US)
> +
> +#define ADS7924_SLEEP_DELAY_MS 2000
Unused, so drop for now.
> +static bool ads7924_is_writeable_reg(struct device *dev, unsigned int reg)
> +{
> + switch (reg) {
> + case ADS7924_MODECNTRL_REG:
> + case ADS7924_INTCNTRL_REG:
> + case ADS7924_ULR0_REG:
> + case ADS7924_LLR0_REG:
> + case ADS7924_ULR1_REG:
> + case ADS7924_LLR1_REG:
> + case ADS7924_ULR2_REG:
> + case ADS7924_LLR2_REG:
> + case ADS7924_ULR3_REG:
> + case ADS7924_LLR3_REG:
> + case ADS7924_INTCONFIG_REG:
> + case ADS7924_SLPCONFIG_REG:
> + case ADS7924_ACQCONFIG_REG:
> + case ADS7924_PWRCONFIG_REG:
> + case ADS7924_RESET_REG:
> + return true;
> + default:
> + return false;
> + }
> +}
> +
> +static const struct regmap_config ads7924_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 8,
> + .max_register = ADS7924_RESET_REG,
> + .writeable_reg = ads7924_is_writeable_reg,
> +};
> +
> +static const struct iio_chan_spec ads7924_channels[] = {
> + ADS7924_V_CHAN(0, ADS7924_DATA0_U_REG),
> + ADS7924_V_CHAN(1, ADS7924_DATA1_U_REG),
> + ADS7924_V_CHAN(2, ADS7924_DATA2_U_REG),
> + ADS7924_V_CHAN(3, ADS7924_DATA3_U_REG),
> +};
> +
> +static
> +int ads7924_get_adc_result(struct ads7924_data *data,
Drop the unusual line break as not needed (probably refactoring left over!)
static int ...
> + struct iio_chan_spec const *chan, int *val)
> +{
> + int ret;
> + __be16 be_val;
> +
> + if (chan->channel < 0 || chan->channel >= ADS7924_CHANNELS)
> + return -EINVAL;
> +
> + if (data->conv_invalid) {
> + int conv_time;
> +
> + conv_time = ADS7924_TOTAL_CONVTIME_US;
> + /* Allow 10% for internal clock inaccuracy. */
> + conv_time += conv_time / 10;
> + usleep_range(conv_time, conv_time + 1);
> + data->conv_invalid = false;
> + }
> +
> + ret = regmap_raw_read(data->regmap, ADS7924_AUTO_INCREMENT_BIT |
> + chan->address, &be_val, sizeof(be_val));
> + if (ret)
> + return ret;
> +
> + *val = be16_to_cpu(be_val);
> + *val = *val >> ADS7924_DATA_SHIFT;
*val = be16_to_cpu(be_val) >> ADS7924_DATA_SHIFT;
return 0;
> +
> + return ret;
> +}
> +
> +static int ads7924_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, int *val,
> + int *val2, long mask)
> +{
> + int ret, vref_uv;
> + struct ads7924_data *data = iio_priv(indio_dev);
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW: {
> + mutex_lock(&data->lock);
> + ret = ads7924_get_adc_result(data, chan, val);
> + mutex_unlock(&data->lock);
> + if (ret < 0)
> + dev_dbg(data->dev, "%s error1\n", __func__);
> + else
> + ret = IIO_VAL_INT;
if (ret < 0) {
...
return ret;
}
return IIO_VAL_INT;
> + break;
> + }
> + case IIO_CHAN_INFO_SCALE:
> + vref_uv = regulator_get_voltage(data->vref_reg);
> + if (vref_uv < 0) {
> + /* dummy regulator "get_voltage" returns -EINVAL */
> + ret = -EINVAL;
return -EINVAL;
> + } else {
> + *val = vref_uv / 1000; /* Convert reg voltage to mV */
> + *val2 = ADS7924_BITS;
> + ret = IIO_VAL_FRACTIONAL_LOG2;
return IIO_VAL_FR...
> + }
> + break;
> + default:
> + ret = -EINVAL;
return -EINVAL;
> + break;
> + }
> +
> + return ret;
> +}
> +
> +static const struct iio_info ads7924_info = {
> + .read_raw = ads7924_read_raw,
The tabbed indent doesn't add anything so
.read_raw = ads...
Aligning things is often a bad idea anyway as it just adds noise when inevitably
the alignment needs increasing... Definitely not worth doing for one item.
> +};
> +
> +static int ads7924_get_channels_config(struct i2c_client *client,
> + struct iio_dev *indio_dev)
> +{
> + struct ads7924_data *priv = iio_priv(indio_dev);
> + struct device *dev = priv->dev;
> + struct fwnode_handle *node;
> + int num_channels = 0;
> +
> + device_for_each_child_node(dev, node) {
> + u32 pval;
> + unsigned int channel;
> +
> + if (fwnode_property_read_u32(node, "reg", &pval)) {
> + dev_err(dev, "invalid reg on %pfw\n", node);
> + continue;
> + }
> +
> + channel = pval;
> + if (channel >= ADS7924_CHANNELS) {
> + dev_err(dev, "invalid channel index %d on %pfw\n",
> + channel, node);
> + continue;
> + }
> +
> + num_channels++;
> + }
> +
> + if (num_channels > 0) {
> + dev_dbg(dev, "found %d ADC channels\n", num_channels);
> + return 0;
> + } else {
As per other review. Give us what we expect which is error paths
as out of line.
> + return -EINVAL;
> + }
> +}
> +static int ads7924_reset(struct iio_dev *indio_dev)
> +{
> + struct ads7924_data *data = iio_priv(indio_dev);
> +
> + if (data->reset_gpio) {
> + gpiod_set_value(data->reset_gpio, 1); /* Assert. */
> + /* Educated guess: assert time not specified in datasheet... */
> + mdelay(100);
> + gpiod_set_value(data->reset_gpio, 0); /* Deassert. */
No real point in shared exit path. Early returns mean less code to look at
when following particular flows which is always nice.
return 0;
> + } else {
if returning above, no need to have the else {
> + int ret;
> +
> + /*
> + * A write of 10101010 to this register will generate a
> + * software reset of the ADS7924.
> + */
> + ret = regmap_write(data->regmap, ADS7924_RESET_REG, 0b10101010);
> + if (ret)
> + return ret;
return regmap_write(....)
> + }
> +
> + return 0;
and drop this.
> +};
> +static int ads7924_probe(struct i2c_client *client)
> +{
...
> + ret = devm_add_action_or_reset(dev, ads7924_set_idle_mode, data);
The _or_reset() part of this is there so that if this function fails
it will call the callback before returning an error code. As a result
you will turn the device, before you've ever turned it on.
Register the callback after turning it on, so ...
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "failed to add idle mode action\n");
> +
> + ret = ads7924_set_conv_mode(data, ADS7924_MODECNTRL_AUTO_SCAN);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "failed to set conversion mode\n");
... here.
Powered by blists - more mailing lists