| 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: <20250418153714.501d3cb0@jic23-huawei>
Date: Fri, 18 Apr 2025 15:37:14 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: David Lechner <dlechner@...libre.com>
Cc: Kim Seer Paller <kimseer.paller@...log.com>, Lars-Peter Clausen
<lars@...afoo.de>, Michael Hennerich <Michael.Hennerich@...log.com>, Rob
Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor
Dooley <conor+dt@...nel.org>, Nuno Sá
<noname.nuno@...il.com>, linux-iio@...r.kernel.org,
linux-kernel@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH v4 3/3] iio: dac: ad3530r: Add driver for AD3530R and
AD3531R
On Wed, 16 Apr 2025 14:23:11 -0500
David Lechner <dlechner@...libre.com> wrote:
> On 4/12/25 12:57 AM, Kim Seer Paller wrote:
> > The AD3530/AD3530R (8-channel) and AD3531/AD3531R (4-channel) are
> > low-power, 16-bit, buffered voltage output DACs with software-
> > programmable gain controls, providing full-scale output spans of 2.5V or
> > 5V for reference voltages of 2.5V. These devices operate from a single
> > 2.7V to 5.5V supply and are guaranteed monotonic by design. The "R"
> > variants include a 2.5V, 5ppm/°C internal reference, which is disabled
> > by default.
> >
> > Support for monitoring internal die temperature, output voltages, and
> > current of a selected channel via the MUXOUT pin using an external ADC
> > is currently not implemented.
> >
> > Signed-off-by: Kim Seer Paller <kimseer.paller@...log.com>
> > ---
>
> Looks very good now. :-)
>
> A made a few comments but maybe nothing serious enough to require a v5.
We have plenty of time in the cycle, so I think I would prefer a v5
just tidying up these last few bits.
Thanks,
Jonathan
>
> > diff --git a/drivers/iio/dac/ad3530r.c b/drivers/iio/dac/ad3530r.c
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..ffa04f678b86d8da6f5e47c35c265b6648121843
> > --- /dev/null
> > +++ b/drivers/iio/dac/ad3530r.c
> > @@ -0,0 +1,506 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * AD3530R/AD3530 8-channel, 16-bit Voltage Output DAC Driver
> > + * AD3531R/AD3531 4-channel, 16-bit Voltage Output DAC Driver
> > + *
> > + * Copyright 2025 Analog Devices Inc.
> > + */
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/bits.h>
> > +#include <linux/cleanup.h>
> > +#include <linux/delay.h>
> > +#include <linux/device.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/kernel.h>
>
> Usually, we try to avoid including kernel.h - it includes too much.
>
> > +#include <linux/module.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/mutex.h>
> > +#include <linux/property.h>
> > +#include <linux/regmap.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/spi/spi.h>
> > +
> ...
>
> > +
> > +static int ad3530r_setup(struct ad3530r_state *st, int vref,
> > + bool has_external_vref)
> > +{
> > + struct device *dev = regmap_get_device(st->regmap);
> > + struct gpio_desc *reset_gpio;
> > + int i, ret;
> > + bool has_range_multiplier;
> > +
> > + reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> > + if (IS_ERR(reset_gpio))
> > + return dev_err_probe(dev, PTR_ERR(reset_gpio),
> > + "Failed to get reset GPIO\n");
> > +
> > + if (reset_gpio) {
> > + /* Perform hardware reset */
> > + fsleep(1000);
> > + gpiod_set_value_cansleep(reset_gpio, 0);
> > + } else {
> > + /* Perform software reset */
> > + ret = regmap_update_bits(st->regmap, AD3530R_INTERFACE_CONFIG_A,
> > + AD3530R_SW_RESET, AD3530R_SW_RESET);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + fsleep(10000);
> > +
> > + has_range_multiplier = false;
> > + if (device_property_present(dev, "adi,range-double")) {
>
> Since this is a flag, I think device_property_read_bool() is preferred.
>
> > + ret = regmap_set_bits(st->regmap, AD3530R_OUTPUT_CONTROL_0,
> > + AD3530R_OUTPUT_CONTROL_RANGE);
> > + if (ret)
> > + return ret;
> > +
> > + has_range_multiplier = true;
> > + }
> > +
> > + if (!has_external_vref && st->chip_info->internal_ref_support) {
> > + ret = regmap_set_bits(st->regmap, AD3530R_REFERENCE_CONTROL_0,
> > + AD3530R_REFERENCE_CONTROL_SEL);
> > + if (ret)
> > + return ret;
> > +
> > + st->vref_mv = has_range_multiplier ?
> > + 2 * AD3530R_INTERNAL_VREF_MV :
> > + AD3530R_INTERNAL_VREF_MV;
> > + }
> > +
> > + if (has_external_vref)
> > + st->vref_mv = has_range_multiplier ? 2 * vref / 1000 : vref / 1000;
> > +
>
> I think this would be simpler as:
>
> st->vref_mv = range_multiplier * vref / 1000;
>
> where range_multiplier is 1 or 2.
>
> > + /* Set operating mode to normal operation. */
> > + ret = regmap_write(st->regmap, AD3530R_OUTPUT_OPERATING_MODE_0,
> > + AD3530R_NORMAL_OPERATION);
> > + if (ret)
> > + return ret;
> > +
> > + if (st->chip_info->num_channels > AD3531R_MAX_CHANNELS) {
> > + ret = regmap_write(st->regmap, AD3530R_OUTPUT_OPERATING_MODE_1,
> > + AD3530R_NORMAL_OPERATION);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + for (i = 0; i < st->chip_info->num_channels; i++)
> > + st->chan[i].powerdown_mode = AD3530R_POWERDOWN_32K;
> > +
> > + st->ldac_gpio = devm_gpiod_get_optional(dev, "ldac", GPIOD_OUT_HIGH);
>
> I guess it doesn't matter which state this starts in but GPIOD_OUT_LOW seems
> more natural since we toggle it high the low later.
>
> > + if (IS_ERR(st->ldac_gpio))
> > + return dev_err_probe(dev, PTR_ERR(st->ldac_gpio),
> > + "Failed to get ldac GPIO\n");
> > +
> > + return 0;
> > +}
> > +
Powered by blists - more mailing lists