| 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: <20221123174528.00001207@Huawei.com>
Date: Wed, 23 Nov 2022 17:45:28 +0000
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Ciprian Regus <ciprian.regus@...log.com>
CC: <jic23@...nel.org>, <krzysztof.kozlowski+dt@...aro.org>,
<robh+dt@...nel.org>, <linux-iio@...r.kernel.org>,
<devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 2/2] drivers: iio: dac: Add AD5754 DAC driver
On Fri, 18 Nov 2022 19:24:07 +0200
Ciprian Regus <ciprian.regus@...log.com> wrote:
> The AD5724/AD5734/AD5754 are quad, 12-/14-/16-bit, serial input, voltage
> output DACs. The devices operate from single-supply voltages from +4.5 V
> up to +16.5 V or dual-supply voltages from ±4.5 V up to ±16.5 V. The
> input coding is user-selectable twos complement or offset binary for a
> bipolar output (depending on the state of Pin BIN/2sComp), and straight
> binary for a unipolar output.
>
> Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/AD5724_5734_5754.pdf
> Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad5722_5732_5752.pdf
> Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad5724r_5734r_5754r.pdf
> Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/AD5722R_5732R_5752R.pdf
> Signed-off-by: Ciprian Regus <ciprian.regus@...log.com>
Hi Ciprian,
Took another look and found a few more things given you'll be spinning
again for the dt binding.
Only significant one is the handling of error returns form the optional regulator.
Jonathan
> diff --git a/drivers/iio/dac/ad5754.c b/drivers/iio/dac/ad5754.c
> new file mode 100644
> index 000000000000..81cfda2efa0f
> --- /dev/null
> +++ b/drivers/iio/dac/ad5754.c
> @@ -0,0 +1,672 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2022 Analog Devices, Inc.
> + * Author: Ciprian Regus <ciprian.regus@...log.com>
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/iio/iio.h>
> +#include <linux/kernel.h>
> +#include <linux/linear_range.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +
> +#include <asm/unaligned.h>
> +
> +#define AD5754_INT_VREF_mV 2500
> +#define AD5754_FRAME_SIZE 3
> +#define AD5754_MAX_CHANNELS 4
> +#define AD5754_MAX_RESOLUTION 16
> +#define uV_PER_mV 1000
In common with units.h probably better to spell out than mix case
(as someone will come along and 'fix' this for you ;)
#define MICROVOLT_PER_MILLIVOLT
Or just use MICRO/MILLI inline from units.h and rely on compiler
work the maths out for you.
> +
> +static const struct iio_chan_spec ad5754_2_channels[2] = {
> + AD5754_CHANNEL(0),
> + AD5754_CHANNEL(1),
> +};
> +
> +static const struct iio_chan_spec ad5754_4_channels[4] = {
> + AD5754_CHANNEL(0),
> + AD5754_CHANNEL(1),
> + AD5754_CHANNEL(2),
> + AD5754_CHANNEL(3),
Why the double tab for alignment of these?
> +};
..
> +
> +static int ad5754_reg_write(void *context, unsigned int reg, unsigned int val)
> +{
> + struct ad5754_state *st = context;
> + struct spi_transfer xfer = {
> + .tx_buf = st->buff,
> + .len = 3,
> + };
> +
> + st->buff[0] = reg;
> + put_unaligned_be16(val, &st->buff[1]);
> +
> + return spi_sync_transfer(st->spi, &xfer, 1);
spi_write()?
Same thing under the hood but slightly shorter code here.
> +};
> +
> +static int ad5754_probe(struct spi_device *spi)
> +{
> + struct device *dev = &spi->dev;
> + struct iio_dev *indio_dev;
> + struct ad5754_state *st;
> + int ret;
> +
> +
> + st->vref_reg = devm_regulator_get_optional(dev, "vref");
> + if (IS_ERR(st->vref_reg)) {
Subtle corner here.
The return may not indicate that vref was not provided, it might return
-EPROBEDEFER for example to indicate that we need to back off until
another driver is loaded.
As such, you need separate handling for -ENODEV (I think that's the return
for not specified) which is what you have here, and other error codes which
should be a probe failure via a dev_err_probe() to deal with the deferred case
debug info.
> + if (!st->chip_info->internal_vref)
> + return dev_err_probe(dev, PTR_ERR(st->vref_reg),
> + "Failed to get the vref regulator\n");
> +
> + ret = regmap_update_bits(st->regmap,
> + AD5754_REG_ADDR(AD5754_PWR_REG, AD5754_PU_ADDR),
> + AD5754_INT_REF_MASK,
> + FIELD_PREP(AD5754_INT_REF_MASK, 1));
> + if (ret)
> + return ret;
> +
> + ret = devm_add_action_or_reset(dev, ad5754_disable_int_ref, st);
> + if (ret)
> + return ret;
> + } else {
> + ret = regulator_enable(st->vref_reg);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "Failed to enable the vref regulator\n");
> +
> + ret = devm_add_action_or_reset(dev, ad5754_regulator_disable, st->vref_reg);
> + if (ret)
> + return ret;
> + }
...
> +static const struct spi_device_id ad5754_id[] = {
> + { "ad5722", (kernel_ulong_t)&ad5754_chip_info_data[AD5722], },
> + { "ad5732", (kernel_ulong_t)&ad5754_chip_info_data[AD5732], },
> + { "ad5752", (kernel_ulong_t)&ad5754_chip_info_data[AD5752], },
> + { "ad5724", (kernel_ulong_t)&ad5754_chip_info_data[AD5724], },
> + { "ad5734", (kernel_ulong_t)&ad5754_chip_info_data[AD5734], },
> + { "ad5754", (kernel_ulong_t)&ad5754_chip_info_data[AD5754], },
> + { "ad5722r", (kernel_ulong_t)&ad5754_chip_info_data[AD5722R], },
> + { "ad5732r", (kernel_ulong_t)&ad5754_chip_info_data[AD5732R], },
> + { "ad5752r", (kernel_ulong_t)&ad5754_chip_info_data[AD5752R], },
> + { "ad5724r", (kernel_ulong_t)&ad5754_chip_info_data[AD5724R], },
> + { "ad5734r", (kernel_ulong_t)&ad5754_chip_info_data[AD5734R], },
> + { "ad5754r", (kernel_ulong_t)&ad5754_chip_info_data[AD5754R], },
> + {}
> +};
> +
> +static const struct of_device_id ad5754_dt_id[] = {
> + {
> + .compatible = "adi,ad5722",
> + .data = &ad5754_chip_info_data[AD5722],
Whatever order you end up with in the DT binding after Krzysztof's review
replicate here and for the spi_device_ids above.
> + }, {
> + .compatible = "adi,ad5732",
> + .data = &ad5754_chip_info_data[AD5732],
> + }, {
> + .compatible = "adi,ad5752",
> + .data = &ad5754_chip_info_data[AD5752],
> + }, {
> + .compatible = "adi,ad5724",
> + .data = &ad5754_chip_info_data[AD5724],
> + }, {
> + .compatible = "adi,ad5734",
> + .data = &ad5754_chip_info_data[AD5734],
> + }, {
> + .compatible = "adi,ad5754",
> + .data = &ad5754_chip_info_data[AD5754],
> + }, {
> + .compatible = "adi,ad5722r",
> + .data = &ad5754_chip_info_data[AD5722R],
> + }, {
> + .compatible = "adi,ad5732r",
> + .data = &ad5754_chip_info_data[AD5732R],
> + }, {
> + .compatible = "adi,ad5752r",
> + .data = &ad5754_chip_info_data[AD5752R],
> + }, {
> + .compatible = "adi,ad5724r",
> + .data = &ad5754_chip_info_data[AD5724R],
> + }, {
> + .compatible = "adi,ad5734r",
> + .data = &ad5754_chip_info_data[AD5734R],
> + }, {
> + .compatible = "adi,ad5754r",
> + .data = &ad5754_chip_info_data[AD5754R],
> + },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, ad5754_dt_id);
Powered by blists - more mailing lists