| 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: <aAexmOU1e-7hXq6Y@smile.fi.intel.com>
Date: Tue, 22 Apr 2025 18:11:20 +0300
From: Andy Shevchenko <andy@...nel.org>
To: Kim Seer Paller <kimseer.paller@...log.com>
Cc: Jonathan Cameron <jic23@...nel.org>,
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>,
David Lechner <dlechner@...libre.com>,
Nuno Sá <noname.nuno@...il.com>,
Nuno Sá <nuno.sa@...log.com>,
linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
devicetree@...r.kernel.org
Subject: Re: [PATCH v5 3/3] iio: dac: ad3530r: Add driver for AD3530R and
AD3531R
On Mon, Apr 21, 2025 at 12:24:54PM +0800, 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.
...
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/cleanup.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
I don't see how you use this. But
+ dev_printk.h
> +#include <linux/gpio/consumer.h>
> +#include <linux/iio/iio.h>
> +#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>
...
> +#define AD3530R_SW_RESET (BIT(7) | BIT(0))
> +#define AD3530R_MAX_CHANNELS 8
> +#define AD3531R_MAX_CHANNELS 4
Sounds to me that these two shouldn't be grouped with the rest here. Perhaps
move them out to after the LDAC_PULSE?
> +#define AD3530R_INTERNAL_VREF_MV 2500
_mV (yes, with Volts and Amperes we use proper spelling).
> +#define AD3530R_LDAC_PULSE_US 100
...
> + int (*input_ch_reg)(unsigned int c);
c? channel?
...
> + int vref_mv;
_mV
...
> +static int ad3530r_input_ch_reg(unsigned int c)
> +{
> + return 2 * c + AD3530R_INPUT_CH;
> +}
> +
> +static int ad3531r_input_ch_reg(unsigned int c)
> +{
> + return 2 * c + AD3531R_INPUT_CH;
> +}
c --> channel
...
> +static const char * const ad3530r_powerdown_modes[] = {
> + "1kohm_to_gnd",
kOhm
> + "7.7kohm_to_gnd",
Ditto.
> + "32kohm_to_gnd",
Ditto.
> +};
...
> +static const struct iio_enum ad3530r_powerdown_mode_enum = {
> + .items = ad3530r_powerdown_modes,
> + .num_items = ARRAY_SIZE(ad3530r_powerdown_modes),
+ array_size.h
> + .get = ad3530r_get_powerdown_mode,
> + .set = ad3530r_set_powerdown_mode,
> +};
...
> +static ssize_t ad3530r_get_dac_powerdown(struct iio_dev *indio_dev,
> + uintptr_t private,
> + const struct iio_chan_spec *chan,
> + char *buf)
> +{
> + struct ad3530r_state *st = iio_priv(indio_dev);
> +
> + guard(mutex)(&st->lock);
> + return sysfs_emit(buf, "%d\n", st->chan[chan->channel].powerdown);
+ sysfs.h
> +}
...
> +static ssize_t ad3530r_set_dac_powerdown(struct iio_dev *indio_dev,
> + uintptr_t private,
> + const struct iio_chan_spec *chan,
> + const char *buf, size_t len)
> +{
> + struct ad3530r_state *st = iio_priv(indio_dev);
> + int ret;
> + unsigned int mask, val, reg;
> + bool powerdown;
> +
> + ret = kstrtobool(buf, &powerdown);
+ kstrtox.h
> + if (ret)
> + return ret;
> +
> + guard(mutex)(&st->lock);
> + mask = GENMASK(chan->address + 1, chan->address);
> + reg = chan->channel < AD3531R_MAX_CHANNELS ?
> + AD3530R_OUTPUT_OPERATING_MODE_0 :
> + AD3530R_OUTPUT_OPERATING_MODE_1;
> + val = (powerdown ? st->chan[chan->channel].powerdown_mode : 0)
> + << chan->address;
Please, move the operator to the previous line, Or even
... pdmode;
pdmode = powerdown ? st->chan[chan->channel].powerdown_mode : 0;
val = pdmode << ...;
> + ret = regmap_update_bits(st->regmap, reg, mask, val);
> + if (ret)
> + return ret;
> +
> + st->chan[chan->channel].powerdown = powerdown;
> +
> + return len;
> +}
...
> + struct ad3530r_state *st = iio_priv(indio_dev);
> +
> + switch (info) {
> + case IIO_CHAN_INFO_RAW:
> + if (val < 0 || val > U16_MAX)
U16_MAX is an abstract type with this limit, do you have any predefined HW
limit instead? Probably better to use that one as defined via BIT() / GENMASK().
> + return -EINVAL;
> +
> + return ad3530r_dac_write(st, chan->channel, val);
> + default:
> + return -EINVAL;
> + }
...
> +static const struct iio_chan_spec_ext_info ad3530r_ext_info[] = {
> + {
> + .name = "powerdown",
> + .shared = IIO_SEPARATE,
> + .read = ad3530r_get_dac_powerdown,
> + .write = ad3530r_set_dac_powerdown,
> + },
> + IIO_ENUM("powerdown_mode", IIO_SEPARATE, &ad3530r_powerdown_mode_enum),
> + IIO_ENUM_AVAILABLE("powerdown_mode", IIO_SHARED_BY_TYPE,
> + &ad3530r_powerdown_mode_enum),
> + { }
> +};
> +
> +#define AD3530R_CHAN(_chan, _pos) { \
Slightly better to have the curly braces on the same column as it's easier to
read.
#define AD3530R_CHAN(_chan, _pos) \
{ \
(and make it one TAB less for the backslash).
> + .type = IIO_VOLTAGE, \
> + .indexed = 1, \
> + .channel = _chan, \
> + .address = _pos, \
> + .output = 1, \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> + BIT(IIO_CHAN_INFO_SCALE), \
> + .ext_info = ad3530r_ext_info, \
> +}
...
> +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;
> + u8 range_multiplier;
+ types.h (and especially for boolean type along with true/false definitions.
> +
> + 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");
+ err.h
> + if (reset_gpio) {
> + /* Perform hardware reset */
> + fsleep(1000);
(1 * USEC_PER_MSEC) ?
> + 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);
10 * USEC_PER_MSEC
With these constants it's less error prone (when 3 or more zeroes) and easier
to get the units without looking into fsleep() implementation / documentation.
> + range_multiplier = 1;
> + if (device_property_read_bool(dev, "adi,range-double")) {
> + ret = regmap_set_bits(st->regmap, AD3530R_OUTPUT_CONTROL_0,
> + AD3530R_OUTPUT_CONTROL_RANGE);
> + if (ret)
> + return ret;
> +
> + range_multiplier = 2;
> + }
> +
> + 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 = range_multiplier * AD3530R_INTERNAL_VREF_MV;
> + }
> +
> + if (has_external_vref)
> + st->vref_mv = range_multiplier * vref / 1000;
MILLI?
> + /* 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_LOW);
> + if (IS_ERR(st->ldac_gpio))
> + return dev_err_probe(dev, PTR_ERR(st->ldac_gpio),
> + "Failed to get ldac GPIO\n");
> +
> + return 0;
> +}
...
> + vref = devm_regulator_get_enable_read_voltage(dev, "ref");
> + if (vref < 0 && vref != -ENODEV)
> + return vref;
> +
> + has_external_vref = vref != -ENODEV;
Wouldn't be better just make this 0 when it's == -ENODEV and check just the
value without having this additional boolean variable (note, I haven't checked
the meaning of Vref == 0 in case it's possible in real life and hardware
behaves adequately)?
> + if (!st->chip_info->internal_ref_support && !has_external_vref)
> + return -ENODEV;
> + ret = ad3530r_setup(st, vref, has_external_vref);
> + if (ret)
> + return ret;
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists