| 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
| ||
|
Message-ID: <20250412181902.16492ffb@jic23-huawei>
Date: Sat, 12 Apr 2025 18:19:02 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Antoniu Miclaus <antoniu.miclaus@...log.com>
Cc: <robh@...nel.org>, <conor+dt@...nel.org>, <linux-iio@...r.kernel.org>,
<devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 13/13] iio: adc: ad4080: add driver support
On Fri, 11 Apr 2025 15:36:27 +0300
Antoniu Miclaus <antoniu.miclaus@...log.com> wrote:
> Add support for AD4080 high-speed, low noise, low distortion,
> 20-bit, Easy Drive, successive approximation register (SAR)
> analog-to-digital converter (ADC).
>
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@...log.com>
I'll leave the backend stuff to Nuno who has a better feel than
me for what fits in that interface. So this is just a review
of the rest of this driver.
Various minor comments inline
Thanks,
Jonathan
> diff --git a/drivers/iio/adc/ad4080.c b/drivers/iio/adc/ad4080.c
> new file mode 100644
> index 000000000000..3a0b1ad13765
> --- /dev/null
> +++ b/drivers/iio/adc/ad4080.c
> +/* AD4080_REG_GPIO_CONFIG_B Bit Definition */
> +#define AD4080_GPIO_1_SEL GENMASK(7, 4)
> +#define AD4080_GPIO_0_SEL GENMASK(3, 0)
> +
> +/* AD4080_REG_FIFO_CONFIG Bit Definition */
> +#define AD4080_FIFO_MODE_MSK GENMASK(1, 0)
> +
> +/* AD4080_REG_FILTER_CONFIG Bit Definition */
Better to name the defines to make that association explicit.
#define AD4080_FILTER_CONFIG_SINC_DEC_RATE_MSK etc
> +#define AD4080_SINC_DEC_RATE_MSK GENMASK(6, 3)
> +#define AD4080_FILTER_SEL_MSK GENMASK(1, 0)
> +
> +/* Miscellaneous Definitions */
> +#define AD4080_SW_RESET (BIT(7) | BIT(0))
> +#define AD4080_SPI_READ BIT(7)
> +#define BYTE_ADDR_H GENMASK(15, 8)
> +#define BYTE_ADDR_L GENMASK(7, 0)
Definitely not on those two!
If you are going this you are probably reading into the wrong data type.
> +static const unsigned int ad4080_scale_table[][2] = {
> + {6000, 0},
{ 6000, 0 },
> +};
> +
> +static const char *const ad4080_filter_type_iio_enum[] = {
> + [FILTER_DISABLE] = "disabled",
> + [SINC_1] = "sinc1",
> + [SINC_5] = "sinc5",
> + [SINC_5_COMP] = "sinc5_plus_compensation",
> +};
> +
> +static const int ad4080_dec_rate_iio_enum[] = {
> + 2, 4, 8, 16, 32, 64, 128, 256, 512, 1024
Convention is keep a trailing comma except when we have
an explicit terminating entry (NULL etc)
> +
> +static int ad4080_set_dec_rate(struct iio_dev *dev,
> + const struct iio_chan_spec *chan,
> + unsigned int mode)
> +{
> + struct ad4080_state *st = iio_priv(dev);
> + int ret;
> + unsigned int data;
> + unsigned int reg_val;
> +
> + if (st->filter_type >= SINC_5 && mode >= 512)
> + return -EINVAL;
> +
> + guard(mutex)(&st->lock);
> + ret = regmap_read(st->regmap, AD4080_REG_FILTER_CONFIG, ®_val);
> + if (ret)
> + return ret;
> +
> + data = ((ilog2(mode) - 1) << 3) | (reg_val & AD4080_FILTER_SEL_MSK);
As below. Odd to keep stuff with explicit mask like this rather than more
normal masking out what we are placing. &= ~SINC_RET_DATA_MSK; etc
> + ret = regmap_write(st->regmap, AD4080_REG_FILTER_CONFIG, data);
> + if (ret)
> + return ret;
> +
> + st->dec_rate = mode;
> +
> + return ret;
> +}
> +static int ad4080_lvds_sync_write(struct ad4080_state *st)
> +{
> + unsigned int timeout = 100;
> + bool sync_en;
> + int ret;
> +
> + guard(mutex)(&st->lock);
> + if (st->num_lanes == 1)
> + ret = regmap_write(st->regmap, AD4080_REG_ADC_DATA_INTF_CONFIG_A,
> + AD4080_RESERVED_CONFIG_A_MSK |
> + AD4080_INTF_CHK_EN_MSK);
> + else
> + ret = regmap_write(st->regmap, AD4080_REG_ADC_DATA_INTF_CONFIG_A,
> + AD4080_RESERVED_CONFIG_A_MSK |
> + AD4080_INTF_CHK_EN_MSK |
> + AD4080_SPI_LVDS_LANES_MSK);
> + if (ret)
> + return ret;
> +
> + ret = iio_backend_data_alignment_enable(st->back);
> + if (ret)
> + return ret;
> +
> + do {
> + ret = iio_backend_sync_status_get(st->back, &sync_en);
> + if (ret)
> + return ret;
> +
> + if (!sync_en)
> + dev_dbg(&st->spi->dev, "Not Locked: Running Bit Slip\n");
Maybe sleep a bit before trying again? Tight loops are very dependent on the
host CPU which is probably not what you want here.
> + } while (--timeout && !sync_en);
> +
> + if (timeout) {
> + dev_info(&st->spi->dev, "Success: Pattern correct and Locked!\n");
> + if (st->num_lanes == 1)
> + return regmap_write(st->regmap, AD4080_REG_ADC_DATA_INTF_CONFIG_A,
> + AD4080_RESERVED_CONFIG_A_MSK);
> + else
> + return regmap_write(st->regmap, AD4080_REG_ADC_DATA_INTF_CONFIG_A,
> + AD4080_RESERVED_CONFIG_A_MSK |
> + AD4080_SPI_LVDS_LANES_MSK);
> + } else {
> + dev_info(&st->spi->dev, "LVDS Sync Timeout.\n");
> + if (st->num_lanes == 1) {
> + ret = regmap_write(st->regmap, AD4080_REG_ADC_DATA_INTF_CONFIG_A,
> + AD4080_RESERVED_CONFIG_A_MSK);
> + if (ret)
> + return ret;
> + } else {
> + ret = regmap_write(st->regmap, AD4080_REG_ADC_DATA_INTF_CONFIG_A,
> + AD4080_RESERVED_CONFIG_A_MSK |
> + AD4080_SPI_LVDS_LANES_MSK);
> + if (ret)
> + return ret;
> + }
> +
> + return -ETIMEDOUT;
> + }
> +}
> +
> +static int ad4080_set_filter_type(struct iio_dev *dev,
> + const struct iio_chan_spec *chan,
> + unsigned int mode)
> +{
> + struct ad4080_state *st = iio_priv(dev);
> + int ret;
> + unsigned int data;
> + unsigned int reg_val;
> +
> + if (mode >= SINC_5 && st->dec_rate >= 512)
> + return -EINVAL;
> +
> + guard(mutex)(&st->lock);
> + if (mode)
> + ret = iio_backend_filter_enable(st->back);
> + else
> + ret = iio_backend_filter_disable(st->back);
> + if (ret)
> + return ret;
> +
> + st->filter_en = mode;
> +
> + ret = regmap_read(st->regmap, AD4080_REG_FILTER_CONFIG, ®_val);
> + if (ret)
> + return ret;
> +
> + data = (reg_val & AD4080_SINC_DEC_RATE_MSK) |
> + (mode & AD4080_FILTER_SEL_MSK);
FIELD_PREP() for the second part.
The first is hanging on to one field. Maybe just pull that out with
a FIELD_GET() and write it back again with FIELD_PREP?
Will be more code, but a little less subtle to read!
> +
> + ret = regmap_write(st->regmap, AD4080_REG_FILTER_CONFIG, data);
> + if (ret)
> + return ret;
> +
> + st->filter_type = mode;
> +
> + return ret;
> +}
> +static struct iio_chan_spec_ext_info ad4080_ext_info[] = {
> + IIO_ENUM("filter_type",
> + IIO_SHARED_BY_ALL,
> + &ad4080_filter_type_enum),
very short line wrap - aim for nearer 80 chars.
> + IIO_ENUM_AVAILABLE("filter_type",
> + IIO_SHARED_BY_ALL,
> + &ad4080_filter_type_enum),
> + {}
Trivial preference for
{ }
> +};
> +
> +#define AD4080_CHAN(_chan, _si, _bits, _sign, _shift) \
> + { .type = IIO_VOLTAGE,
Odd indent. Better perhaps as simpler
{ \
.indexed = 1,
etc. \
> + .indexed = 1, \
> + .channel = _chan, \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_SCALE), \
> + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ) | \
> + BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
> + .info_mask_shared_by_all_available = \
> + BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
> + .ext_info = ad4080_ext_info, \
> + .scan_index = _si, \
> + .scan_type = { \
> + .sign = _sign, \
Current indent makes this look really wierd!
> + .realbits = _bits, \
> + .storagebits = 32, \
> + .shift = _shift, \
> + }, \
> + }
> +
> +static const struct iio_chan_spec ad4080_channels[] = {
> + AD4080_CHAN(0, 0, 20, 's', 0)
Don't bother with the macro as it doesn't add anything. Just put that
stuff all here. That will let you skip setting obvious defaults to 0
like the shift.
> +};
> +
> +static const struct ad4080_chip_info ad4080_chip_info = {
> + .name = "AD4080",
> + .product_id = AD4080_CHIP_ID,
> + .scale_table = ad4080_scale_table,
> + .num_scales = ARRAY_SIZE(ad4080_scale_table),
> + .num_channels = 1,
> + .channels = ad4080_channels,
> +};
> +
> +static int ad4080_setup(struct iio_dev *indio_dev)
> +{
> + if (id != AD4080_CHIP_ID)
> + return dev_err_probe(&st->spi->dev, -EINVAL,
> + "Unrecognized CHIP_ID 0x%X\n", id);
A mismatch on ID should not be treated as an error. That breaks the
use of fallback dt compatibles. So convention on these is a dev_info()
and carry on anyway. We've left breadcrumbs if things don't work but
not our role to make sure it is definitely the right hardware in the
firmware description.
> +
> + ret = regmap_set_bits(st->regmap, AD4080_REG_GPIO_CONFIG_A,
> + AD4080_GPO_1_EN);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(st->regmap, AD4080_REG_GPIO_CONFIG_B,
> + FIELD_PREP(AD4080_GPIO_1_SEL, 3));
> + if (ret)
> + return ret;
> +
> + ret = iio_backend_num_lanes_set(st->back, st->num_lanes);
> + if (ret)
> + return ret;
> +
> + ret = iio_backend_self_sync_enable(st->back);
> + if (ret)
> + return ret;
> +
> + if (st->lvds_cnv_en) {
I'd flip this unless you expect to shortly add more optional stuff after this
code
if (!st->lvds_cnv_en)
return 0;
..
> + if (st->num_lanes) {
> + ret = regmap_update_bits(st->regmap,
> + AD4080_REG_ADC_DATA_INTF_CONFIG_B,
> + AD4080_LVDS_CNV_CLK_CNT_MSK,
> + FIELD_PREP(AD4080_LVDS_CNV_CLK_CNT_MSK, 7));
> + if (ret)
> + return ret;
> + }
> +
> + ret = regmap_set_bits(st->regmap,
> + AD4080_REG_ADC_DATA_INTF_CONFIG_B,
> + AD4080_LVDS_CNV_EN_MSK);
> + if (ret)
> + return ret;
> +
> + return ad4080_lvds_sync_write(st);
> + }
> +
> + return 0;
> +}
> +
> +static void ad4080_properties_parse(struct ad4080_state *st)
> +{
> + unsigned int val;
> + int ret;
> +
> + st->lvds_cnv_en = device_property_read_bool(&st->spi->dev,
> + "adi,lvds-cnv-enable");
> +
> + st->num_lanes = 1;
> + ret = device_property_read_u32(&st->spi->dev, "adi,num_lanes", &val);
> + if (!ret)
> + st->num_lanes = val;
Usual trick on these places were we have a default is to pick types correctly
and do.
st->num_lanes = 1;
device_property_read_u32(&st->spi->dev, "adi,num_lanes", &st->num_lanes);
That is, rely on the call being side effect free on error.
> +}
Powered by blists - more mailing lists