| 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: <20211216180040.3723a906@jic23-huawei>
Date: Thu, 16 Dec 2021 18:00:40 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: Mihail Chindris <mihail.chindris@...log.com>
Cc: <linux-kernel@...r.kernel.org>, <linux-iio@...r.kernel.org>,
<lars@...afoo.de>, <Michael.Hennerich@...log.com>,
<nuno.sa@...log.com>, <dragos.bogdan@...log.com>,
<alexandru.ardelean@...log.com>
Subject: Re: [PATCH v7 2/2] drivers:iio:dac: Add AD3552R driver support
On Mon, 13 Dec 2021 11:08:25 +0000
Mihail Chindris <mihail.chindris@...log.com> wrote:
> The AD3552R-16 is a low drift ultrafast, 16-bit accuracy,
> current output digital-to-analog converter (DAC) designed
> to generate multiple output voltage span ranges.
> The AD3552R-16 operates with a fixed 2.5V reference.
>
> Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad3552r.pdf
>
> Signed-off-by: Mihail Chindris <mihail.chindris@...log.com>
Hi Mihail,
Found a few tiny things on final read though. I'll fix them whilst applying.
Series applied to the togreg branch of iio.git and pushed out as testing for
0-day to have a first poke at it.
Nice driver,
Thanks,
Jonathan
> +
> +static int ad3552r_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int val,
> + int val2,
> + long mask)
> +{
> + struct ad3552r_desc *dac = iio_priv(indio_dev);
> + int err = 0;
Set in all paths, so no need to initialize.
> +
> + mutex_lock(&dac->lock);
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + err = ad3552r_write_reg(dac,
> + AD3552R_REG_ADDR_CH_DAC_24B(chan->channel),
> + val);
> + break;
> + case IIO_CHAN_INFO_ENABLE:
> + err = ad3552r_set_ch_value(dac, AD3552R_CH_DAC_POWERDOWN,
> + chan->channel, !val);
> + break;
> + default:
> + err = -EINVAL;
> + break;
> + }
> + mutex_unlock(&dac->lock);
> +
> + return err;
> +}
> +
...
> +static int ad3552r_configure_device(struct ad3552r_desc *dac)
> +{
> + struct device *dev = &dac->spi->dev;
> + struct fwnode_handle *child;
> + struct regulator *vref;
> + int err, cnt = 0, voltage, delta = 100000;
> + u32 vals[2], val, ch;
> +
> + dac->gpio_ldac = devm_gpiod_get_optional(dev, "ldac", GPIOD_OUT_HIGH);
> + if (IS_ERR(dac->gpio_ldac))
> + return dev_err_probe(dev, PTR_ERR(dac->gpio_ldac),
> + "Error getting gpio ldac");
> +
> + vref = devm_regulator_get_optional(dev, "vref");
> + if (IS_ERR(vref)) {
> + if (PTR_ERR(vref) != -ENODEV)
> + return dev_err_probe(dev, PTR_ERR(vref),
> + "Error getting vref");
> +
> + if (device_property_read_bool(dev, "adi,vref-out-en"))
> + val = AD3552R_INTERNAL_VREF_PIN_2P5V;
> + else
> + val = AD3552R_INTERNAL_VREF_PIN_FLOATING;
> + } else {
> + err = regulator_enable(vref);
> + if (err) {
> + dev_err(dev, "Failed to enable external vref supply\n");
> + return err;
> + }
> +
> + err = devm_add_action_or_reset(dev, ad3552r_reg_disable, vref);
> + if (err) {
> + regulator_disable(vref);
> + return err;
> + }
> +
> + voltage = regulator_get_voltage(vref);
> + if (voltage > 2500000 + delta || voltage < 2500000 - delta) {
> + dev_warn(dev, "vref-supply must be 2.5V");
> + return -EINVAL;
> + }
> + val = AD3552R_EXTERNAL_VREF_PIN_INPUT;
> + }
> +
> + err = ad3552r_update_reg_field(dac,
> + addr_mask_map[AD3552R_VREF_SELECT][0],
> + addr_mask_map[AD3552R_VREF_SELECT][1],
> + val);
> + if (err)
> + return err;
> +
> + err = device_property_read_u32(dev, "adi,sdo-drive-strength", &val);
> + if (!err) {
> + if (val > 3) {
> + dev_err(dev, "adi,sdo-drive-strength must be less than 4\n");
> + return -EINVAL;
> + }
> +
> + err = ad3552r_update_reg_field(dac,
> + addr_mask_map[AD3552R_SDO_DRIVE_STRENGTH][0],
> + addr_mask_map[AD3552R_SDO_DRIVE_STRENGTH][1],
> + val);
> + if (err)
> + return err;
> + }
> +
> + dac->num_ch = device_get_child_node_count(dev);
> + if (!dac->num_ch) {
> + dev_err(dev, "No channels defined\n");
> + return -ENODEV;
> + }
> +
> + device_for_each_child_node(dev, child) {
> + err = fwnode_property_read_u32(child, "reg", &ch);
> + if (err) {
> + dev_err(dev, "mandatory reg property missing\n");
> + goto put_child;
> + }
> + if (ch >= AD3552R_NUM_CH) {
> + dev_err(dev, "reg must be less than %d\n",
> + AD3552R_NUM_CH);
> + err = -EINVAL;
> + goto put_child;
> + }
> +
> + if (fwnode_property_present(child, "adi,output-range-microvolt")) {
> + err = fwnode_property_read_u32_array(child,
> + "adi,output-range-microvolt",
> + vals,
> + 2);
> + if (err) {
> + dev_err(dev,
> + "mandatory adi,output-range-microvolt property missing\n");
Trivial but that comment doesn't make sense. You know it it exists as you check that
above, so failure here means something wrong with it. I'll tweak this if it's
all I find in this round of review to something like "failed to parse adi,out..."
> + goto put_child;
> + }
> +
> + err = ad3552r_find_range(dac->chip_id, vals);
> + if (err < 0) {
> + dev_err(dev,
> + "Invalid adi,output-range-microvolt value\n");
> + goto put_child;
> + }
> + val = err;
> + err = ad3552r_set_ch_value(dac,
> + AD3552R_CH_OUTPUT_RANGE_SEL,
> + ch, val);
> + if (err)
> + goto put_child;
> +
> + dac->ch_data[ch].range = val;
> + } else if (dac->chip_id == AD3542R_ID) {
> + dev_err(dev,
> + "adi,output-range-microvolt is required for ad3542r\n");
> + err = -EINVAL;
> + goto put_child;
> + } else {
> + err = ad3552r_configure_custom_gain(dac, child, ch);
> + if (err)
> + goto put_child;
> + }
> +
> + ad3552r_calc_gain_and_offset(dac, ch);
> + dac->enabled_ch |= BIT(ch);
> +
> + err = ad3552r_set_ch_value(dac, AD3552R_CH_SELECT, ch, 1);
> + if (err < 0)
> + goto put_child;
> +
> + dac->channels[cnt] = AD3552R_CH_DAC(ch);
> + ++cnt;
> +
> + }
Powered by blists - more mailing lists