| 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:
<PH0PR03MB714102E85132F30477EE7680F9AA2@PH0PR03MB7141.namprd03.prod.outlook.com>
Date: Mon, 7 Apr 2025 08:07:08 +0000
From: "Paller, Kim Seer" <KimSeer.Paller@...log.com>
To: Jonathan Cameron <jic23@...nel.org>
CC: Lars-Peter Clausen <lars@...afoo.de>,
"Hennerich, Michael"
<Michael.Hennerich@...log.com>,
Rob Herring <robh@...nel.org>,
Krzysztof
Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>,
"linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>
Subject: RE: [PATCH v3 3/3] iio: dac: ad3530r: Add driver for AD3530R and
AD3531R
> -----Original Message-----
> From: Jonathan Cameron <jic23@...nel.org>
> Sent: Sunday, April 6, 2025 12:12 AM
> To: Paller, Kim Seer <KimSeer.Paller@...log.com>
> Cc: Lars-Peter Clausen <lars@...afoo.de>; Hennerich, Michael
> <Michael.Hennerich@...log.com>; Rob Herring <robh@...nel.org>; Krzysztof
> Kozlowski <krzk+dt@...nel.org>; Conor Dooley <conor+dt@...nel.org>; linux-
> iio@...r.kernel.org; linux-kernel@...r.kernel.org; devicetree@...r.kernel.org
> Subject: Re: [PATCH v3 3/3] iio: dac: ad3530r: Add driver for AD3530R and
> AD3531R
>
> [External]
>
> On Thu, 3 Apr 2025 13:33:57 +0800
> Kim Seer Paller <kimseer.paller@...log.com> wrote:
>
> Hi Kim,
> > 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.
>
>
> As below. Given the bindings provide for use with an external ADC to read a
> wide range of signals, if intent is not to provide that support 'yet'
> (which is fine) then add a paragraph here to say that.
>
> >
> > Signed-off-by: Kim Seer Paller <kimseer.paller@...log.com>
>
>
> A few additional comments from me inline. Particular fun is the long running
> SPI regmap DMA safety assumptions question.
> For now I think we have to assume bulk read/write still need a DMA safe
> buffer.
>
> Jonathan
>
> > diff --git a/drivers/iio/dac/ad3530r.c b/drivers/iio/dac/ad3530r.c new
> > file mode 100644 index
> >
> 0000000000000000000000000000000000000000..4b757e19f0c8349999f72e
> 53abb1
> > a4f483a44eb2
> > --- /dev/null
> > +++ b/drivers/iio/dac/ad3530r.c
> > @@ -0,0 +1,514 @@
>
> > +
> > +#define AD3530R_INTERFACE_CONFIG_A 0x00
> > +#define AD3530R_OUTPUT_OPERATING_MODE_0 0x20
> > +#define AD3530R_OUTPUT_OPERATING_MODE_1 0x21
> > +#define AD3530R_OUTPUT_CONTROL_0 0x2A
> > +#define AD3530R_REFERENCE_CONTROL_0 0x3C
> > +#define AD3530R_SW_LDAC_TRIG_A 0xE5
> > +#define AD3530R_INPUT_CH(c) (2 * (c) + 0xEB)
> > +
> > +#define AD3531R_SW_LDAC_TRIG_A 0xDD
> > +#define AD3531R_INPUT_CH(c) (2 * (c) + 0xE3)
>
> I'd add a define for the first channel and then just have the rest of this in the
> device specific function that uses it.
>
> > +
> > +#define AD3530R_SW_LDAC_TRIG_MASK BIT(7)
> > +#define AD3530R_OUTPUT_CONTROL_MASK BIT(2)
> > +#define AD3530R_REFERENCE_CONTROL_MASK BIT(0)
> > +#define AD3530R_REG_VAL_MASK GENMASK(15, 0)
> > +
> > +#define AD3530R_SW_RESET (BIT(7) | BIT(0))
> > +#define AD3530R_MAX_CHANNELS 8
> > +#define AD3531R_MAX_CHANNELS 4
> > +#define AD3530R_CH(c) (c)
>
> This seems a little unnecessary...
> Given it is only called in device specific functions I'd just encode that there.
>
> > +#define AD3530R_32KOHM_POWERDOWN_MODE 3
> > +#define AD3530R_INTERNAL_VREF_MV 2500
> > +#define AD3530R_LDAC_PULSE_US 100
>
> > +static int ad3530r_dac_write(struct ad3530r_state *st, unsigned int chan,
> > + unsigned int val)
> > +{
> > + int ret;
> > + __be16 reg_val;
> > +
> > + guard(mutex)(&st->lock);
> > + reg_val = cpu_to_be16(val);
> > +
> > + ret = regmap_bulk_write(st->regmap, st->chip_info-
> >input_ch_reg(chan),
> > + ®_val, sizeof(reg_val));
>
> As below. regmap_bulk_write() shouldn't assume the buffer is bounced so
> needs a DMA safe buffer.
>
> > + if (ret)
> > + return ret;
> > +
> > + if (st->ldac_gpio)
> > + return ad3530r_trigger_hw_ldac(st->ldac_gpio);
> > +
> > + return regmap_update_bits(st->regmap, st->chip_info-
> >sw_ldac_trig_reg,
> > + AD3530R_SW_LDAC_TRIG_MASK,
> > +
> FIELD_PREP(AD3530R_SW_LDAC_TRIG_MASK, 1)); }
> > +
> > +static int ad3530r_read_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan,
> > + int *val, int *val2, long info) {
> > + struct ad3530r_state *st = iio_priv(indio_dev);
> > + int ret;
> > + __be16 reg_val;
> > +
> > + switch (info) {
> > + case IIO_CHAN_INFO_RAW:
> > + ret = regmap_bulk_read(st->regmap,
> > + st->chip_info->input_ch_reg(chan-
> >channel),
> > + ®_val, sizeof(reg_val));
>
> So this runs into the old question of whether we need a DMA safe buffer for a
> regmap_bulk_read when the bus is SPI. In practice we probably don't because
> of internal details of the regmap but I believe nothing has changed on the
> guidance to not assume that in drivers. So this regval should be DMA safe.
> Easiest option is a
> __be16 val __aligned(IIO_DMA_MINALIGN) at then end of the st structure
> though we will also need to take a mutex to prevent multiple uses of that
> buffer.
>
> Maybe we should revisit this with Mark. I checked briefly and 'think'
> there is always a copy.
>
> > + if (ret)
> > + return ret;
> > +
> > + *val = FIELD_GET(AD3530R_REG_VAL_MASK,
> be16_to_cpu(reg_val));
> > +
> > + return IIO_VAL_INT;
> > + case IIO_CHAN_INFO_SCALE:
> > + *val = st->vref_mv;
> > + *val2 = 16;
> > +
> > + return IIO_VAL_FRACTIONAL_LOG2;
> > + default:
> > + return -EINVAL;
> > + }
> > +}
>
> > +
> > +#define AD3530R_CHAN_EXT_INFO(_name, _what, _shared, _read, _write) {
> \
> > + .name = (_name), \
> > + .read = (_read), \
> > + .write = (_write), \
> > + .private = (_what), \
> > + .shared = (_shared), \
> > +}
> > +
> > +static const struct iio_chan_spec_ext_info ad3530r_ext_info[] = {
> > + AD3530R_CHAN_EXT_INFO("powerdown", 0, IIO_SEPARATE,
> > + ad3530r_get_dac_powerdown,
> > + ad3530r_set_dac_powerdown),
> As there is only one don't use a macro.
>
>
> {
> .name = "powerdown",
> .shared = IIO_SEPARATE,
> .read = ...
> .write= ...
> },
>
> > + 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) {
> \
> > + .type = IIO_VOLTAGE, \
> > + .indexed = 1, \
> > + .channel = _chan, \
> > + .output = 1, \
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> \
> > + BIT(IIO_CHAN_INFO_SCALE), \
> > + .ext_info = ad3530r_ext_info, \
> > +}
> > +
> > +static const struct iio_chan_spec ad3530r_channels[] = {
>
> Hmm. You only have output channels but we have the stuff to read the mux
> value on a pin. (Nuno mentioned this as well).
> If that is enabled I'd expect to see all the input channels to reflect what we
> might read by putting it on that pin and reading it with an ADC we are
> consumer.
>
> I'm fine if we make that a job for another day though just state that really
> clearly in the patch description.
Thank you for the feedback. I agree to not support this for now, as I need to
further understand the implementation on my end. I'll state this limitation
in the patch description.
> > + AD3530R_CHAN(0),
> > + AD3530R_CHAN(1),
> > + AD3530R_CHAN(2),
> > + AD3530R_CHAN(3),
> > + AD3530R_CHAN(4),
> > + AD3530R_CHAN(5),
> > + AD3530R_CHAN(6),
> > + AD3530R_CHAN(7),
> > +};
> > +
> > +static const struct iio_chan_spec ad3531r_channels[] = {
> > + AD3530R_CHAN(0),
> > + AD3530R_CHAN(1),
> > + AD3530R_CHAN(2),
> > + AD3530R_CHAN(3),
> > +};
Powered by blists - more mailing lists