| 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:
<SJ0PR03MB622408B804B293176E80AC3191822@SJ0PR03MB6224.namprd03.prod.outlook.com>
Date: Sat, 17 Aug 2024 11:59:09 +0000
From: "Tinaco, Mariel" <Mariel.Tinaco@...log.com>
To: Jonathan Cameron <jic23@...nel.org>
CC: "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Lars-Peter
Clausen <lars@...afoo.de>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski
<krzk+dt@...nel.org>,
"Hennerich, Michael" <Michael.Hennerich@...log.com>,
Conor Dooley <conor+dt@...nel.org>,
Marcelo Schmitt
<marcelo.schmitt1@...il.com>,
Dimitri Fedrau <dima.fedrau@...il.com>,
David
Lechner <dlechner@...libre.com>,
Nuno Sá
<noname.nuno@...il.com>
Subject: RE: [PATCH v2 2/2] iio: dac: support the ad8460 Waveform DAC
> -----Original Message-----
> From: Jonathan Cameron <jic23@...nel.org>
> Sent: Saturday, August 3, 2024 6:30 PM
> To: Tinaco, Mariel <Mariel.Tinaco@...log.com>
> Cc: linux-iio@...r.kernel.org; devicetree@...r.kernel.org; linux-
> kernel@...r.kernel.org; Lars-Peter Clausen <lars@...afoo.de>; Rob Herring
> <robh@...nel.org>; Krzysztof Kozlowski <krzk+dt@...nel.org>; Hennerich,
> Michael <Michael.Hennerich@...log.com>; Conor Dooley
> <conor+dt@...nel.org>; Marcelo Schmitt <marcelo.schmitt1@...il.com>;
> Dimitri Fedrau <dima.fedrau@...il.com>; David Lechner
> <dlechner@...libre.com>; Nuno Sá <noname.nuno@...il.com>
> Subject: Re: [PATCH v2 2/2] iio: dac: support the ad8460 Waveform DAC
>
> [External]
>
> On Tue, 30 Jul 2024 11:05:09 +0800
> Mariel Tinaco <Mariel.Tinaco@...log.com> wrote:
>
> > The AD8460 is a “bits in, power out” high voltage, high-power,
> > high-speed driver optimized for large output current (up to ±1 A)
> > and high slew rate (up to ±1800 V/μs) at high voltage (up to ±40 V)
> > into capacitive loads.
> >
> > A digital engine implements user-configurable features: modes for
> > digital input, programmable supply current, and fault monitoring
> > and programmable protection settings for output current,
> > output voltage, and junction temperature. The AD8460 operates on
> > high voltage dual supplies up to ±55 V and a single low voltage
> > supply of 5 V.
> >
> > Signed-off-by: Mariel Tinaco <Mariel.Tinaco@...log.com>
> Hi Mariel,
>
> The test bot detected problems don't give me a high degree of confidence
> in this driver in general. Please be much more careful and thorough in
> build tests for v3.
>
This is a lapse on my end. I'll be more careful the next time. Apologies
> Various minor comments inline
>
> Jonathan
>
> > diff --git a/drivers/iio/dac/ad8460.c b/drivers/iio/dac/ad8460.c
> > new file mode 100644
> > index 000000000..a94fa4526
> > --- /dev/null
> > +++ b/drivers/iio/dac/ad8460.c
> > @@ -0,0 +1,976 @@
>
> > +static int ad8460_set_hvdac_word(struct ad8460_state *state,
> > + int index,
> > + int val)
> > +{
> > + put_unaligned_le16(val & 0x3FFF, &state->spi_tx_buf);
>
> GENMASK for that constant.
>
Created GENMASK for mask of full data byte
#define AD8460_DATA_BYTE_FULL_MSK GENMASK(13, 0)
static int ad8460_set_hvdac_word(struct ad8460_state *state, int index, int val)
{
put_unaligned_le16(FIELD_PREP(AD8460_DATA_BYTE_FULL_MSK, val),
&state->spi_tx_buf);
> > +
> > + return regmap_bulk_write(state->regmap,
> AD8460_HVDAC_DATA_WORD_LOW(index),
> > + state->spi_tx_buf,
> AD8460_DATA_BYTE_WORD_LENGTH);
> > +}
>
>
> > +
> > +static int ad8460_write_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan,
> > + int val,
> > + int val2,
> > + long mask)
>
> Aim for wrapping to below 80 chars, not much shorter.
> Feel free to group parameters though if you want to.
>
Fitted most lines to 80 chars
> > +{
> > + struct ad8460_state *state = iio_priv(indio_dev);
> > +
> > + switch (mask) {
> > + case IIO_CHAN_INFO_RAW:
> > + switch (chan->type) {
> > + case IIO_VOLTAGE:
> > + iio_device_claim_direct_scoped(return -EBUSY,
> indio_dev)
> > + return ad8460_set_sample(state, val);
> > + unreachable();
> > + case IIO_CURRENT:
> > + return regmap_write(state->regmap,
> AD8460_CTRL_REG(0x04),
> > +
> FIELD_PREP(AD8460_SET_QUIESCENT_CURRENT_MSK, val));
> > + default:
> > + return -EINVAL;
> > + }
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static int ad8460_read_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan,
> > + int *val,
> > + int *val2,
> > + long mask)
> > +{
> > + struct ad8460_state *state = iio_priv(indio_dev);
> > + int data, ret;
> > +
> > + switch (mask) {
> > + case IIO_CHAN_INFO_RAW:
> > + switch (chan->type) {
> > + case IIO_VOLTAGE:
> > + scoped_guard(mutex, &state->lock) {
> > + ret = ad8460_get_hvdac_word(state, 0,
> &data);
> > + if (ret)
> > + return ret;
> > + }
> > + *val = data;
> > + return IIO_VAL_INT;
> > + case IIO_CURRENT:
> > + ret = regmap_read(state->regmap,
> AD8460_CTRL_REG(0x04), &data);
> > + if (ret)
> > + return ret;
> > + *val = data;
> > + return IIO_VAL_INT;
> > + case IIO_TEMP:
> > + ret = iio_read_channel_raw(state->tmp_adc_channel,
> &data);
> > + if (ret)
> > + return ret;
> > + *val = data;
> > + return IIO_VAL_INT;
> > + default:
> > + return -EINVAL;
> > + }
> > + case IIO_CHAN_INFO_SAMP_FREQ:
> > + *val = clk_get_rate(state->sync_clk);
> > + return IIO_VAL_INT;
> > + case IIO_CHAN_INFO_SCALE:
> > + /* vCONV = vNOMINAL_SPAN * (DAC_CODE / 2**14) - 40V
> /*
> * vCONV ...
>
> for IIO multiline comment syntax.
>
Followed syntax
case IIO_CHAN_INFO_SCALE:
/*
* vCONV = vNOMINAL_SPAN * (DAC_CODE / 2**14) - 40V
* vMAX = vNOMINAL_SPAN * (2**14 / 2**14) - 40V
* vMIN = vNOMINAL_SPAN * (0 / 2**14) - 40V
* vADJ = vCONV * (2000 / rSET) * (vREF / 1.2)
* vSPAN = vADJ_MAX - vADJ_MIN
* See datasheet page 49, section FULL-SCALE REDUCTION
*/
> > + * vMAX = vNOMINAL_SPAN * (2**14 / 2**14) - 40V
> > + * vMIN = vNOMINAL_SPAN * (0 / 2**14) - 40V
> > + * vADJ = vCONV * (2000 / rSET) * (vREF / 1.2)
> > + * vSPAN = vADJ_MAX - vADJ_MIN
> > + * See datasheet page 49, section FULL-SCALE REDUCTION
> > + */
> > + *val = AD8460_NOMINAL_VOLTAGE_SPAN * 2000 * state-
> >refio_1p2v_mv;
> > + *val2 = state->rset_ohms * 1200;
> > + return IIO_VAL_FRACTIONAL;
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static int ad8460_select_fault_type(int chan_type, enum
> iio_event_direction dir)
> > +{
> > + switch (chan_type) {
> > + case IIO_VOLTAGE:
> > + switch (dir) {
> > + case IIO_EV_DIR_RISING:
> > + return AD8460_OVERVOLTAGE_POS;
> > + case IIO_EV_DIR_FALLING:
> > + return AD8460_OVERVOLTAGE_NEG;
> > + default:
> > + return -EINVAL;
> > + }
> > + break;
> as below.
>
> > + case IIO_CURRENT:
> > + switch (dir) {
> > + case IIO_EV_DIR_RISING:
> > + return AD8460_OVERCURRENT_SRC;
> > + case IIO_EV_DIR_FALLING:
> > + return AD8460_OVERCURRENT_SNK;
> > + default:
> > + return -EINVAL;
> > + }
> > + break;
> Can't get here. So drop the break.
> > + case IIO_TEMP:
> > + switch (dir) {
> > + case IIO_EV_DIR_RISING:
> > + return AD8460_OVERTEMPERATURE;
> > + default:
> > + return -EINVAL;
> > + }
> > + default:
> > + return -EINVAL;
> > + }
> > +}
>
> > +static int ad8460_read_event_config(struct iio_dev *indio_dev,
> > + const struct iio_chan_spec *chan,
> > + enum iio_event_type type,
> > + enum iio_event_direction dir)
> > +{
> > + struct ad8460_state *state = iio_priv(indio_dev);
> > + unsigned int fault;
> > + bool en;
> > + int ret;
> > +
> > + if (type != IIO_EV_TYPE_THRESH)
> > + return -EINVAL;
> > +
> > + fault = ad8460_select_fault_type(chan->type, dir);
> > + if (fault < 0)
> > + return fault;
> > +
> > + ret = ad8460_get_fault_threshold_en(state, fault, &en);
> > + return ret ?: en;
>
> Keep to simpler to read
> if (ret)
> return ret;
>
> return en;
>
> > +}
>
> > +
> > +static struct iio_chan_spec_ext_info ad8460_ext_info[] = {
> > + AD8460_CHAN_EXT_INFO("raw0", 0, IIO_SEPARATE,
> > + ad8460_dac_input_read, ad8460_dac_input_write),
> > + AD8460_CHAN_EXT_INFO("raw1", 1, IIO_SEPARATE,
> > + ad8460_dac_input_read, ad8460_dac_input_write),
> > + AD8460_CHAN_EXT_INFO("raw2", 2, IIO_SEPARATE,
> > + ad8460_dac_input_read, ad8460_dac_input_write),
> > + AD8460_CHAN_EXT_INFO("raw3", 3, IIO_SEPARATE,
> > + ad8460_dac_input_read, ad8460_dac_input_write),
> > + AD8460_CHAN_EXT_INFO("raw4", 4, IIO_SEPARATE,
> > + ad8460_dac_input_read, ad8460_dac_input_write),
> > + AD8460_CHAN_EXT_INFO("raw5", 5, IIO_SEPARATE,
> > + ad8460_dac_input_read, ad8460_dac_input_write),
> > + AD8460_CHAN_EXT_INFO("raw6", 6, IIO_SEPARATE,
> > + ad8460_dac_input_read, ad8460_dac_input_write),
> > + AD8460_CHAN_EXT_INFO("raw7", 7, IIO_SEPARATE,
> > + ad8460_dac_input_read, ad8460_dac_input_write),
> > + AD8460_CHAN_EXT_INFO("raw8", 8, IIO_SEPARATE,
> > + ad8460_dac_input_read, ad8460_dac_input_write),
> > + AD8460_CHAN_EXT_INFO("raw9", 9, IIO_SEPARATE,
> > + ad8460_dac_input_read, ad8460_dac_input_write),
> > + AD8460_CHAN_EXT_INFO("raw10", 10, IIO_SEPARATE,
> > + ad8460_dac_input_read, ad8460_dac_input_write),
> > + AD8460_CHAN_EXT_INFO("raw11", 11, IIO_SEPARATE,
> > + ad8460_dac_input_read, ad8460_dac_input_write),
> > + AD8460_CHAN_EXT_INFO("raw12", 12, IIO_SEPARATE,
> > + ad8460_dac_input_read, ad8460_dac_input_write),
> > + AD8460_CHAN_EXT_INFO("raw13", 13, IIO_SEPARATE,
> > + ad8460_dac_input_read, ad8460_dac_input_write),
> > + AD8460_CHAN_EXT_INFO("raw14", 14, IIO_SEPARATE,
> > + ad8460_dac_input_read, ad8460_dac_input_write),
> > + AD8460_CHAN_EXT_INFO("raw15", 15, IIO_SEPARATE,
> > + ad8460_dac_input_read, ad8460_dac_input_write),
> > + AD8460_CHAN_EXT_INFO("toggle_en", 0, IIO_SEPARATE,
> > + ad8460_read_toggle_en,
> > + ad8460_write_toggle_en),
> > + AD8460_CHAN_EXT_INFO("symbol", 0, IIO_SEPARATE,
> > + ad8460_read_symbol,
> > + ad8460_write_symbol),
>
> Wrap closer to 80 chars.
>
> > + AD8460_CHAN_EXT_INFO("powerdown", 0, IIO_SEPARATE,
> > + ad8460_read_powerdown,
> ad8460_write_powerdown),
> > + IIO_ENUM("powerdown_mode", IIO_SEPARATE,
> &ad8460_powerdown_mode_enum),
> > + IIO_ENUM_AVAILABLE("powerdown_mode", IIO_SHARED_BY_TYPE,
> > + &ad8460_powerdown_mode_enum),
> > + {}
> > +};
> > +
> > +static const struct iio_event_spec ad8460_events[] = {
> > + {
> > + .type = IIO_EV_TYPE_THRESH,
> > + .dir = IIO_EV_DIR_RISING,
> > + .mask_separate = BIT(IIO_EV_INFO_VALUE) |
> > + BIT(IIO_EV_INFO_ENABLE),
> > + },
> > + {
> > + .type = IIO_EV_TYPE_THRESH,
> > + .dir = IIO_EV_DIR_FALLING,
> > + .mask_separate = BIT(IIO_EV_INFO_VALUE) |
> > + BIT(IIO_EV_INFO_ENABLE),
> > + },
> > +};
> > +
> > +#define AD8460_VOLTAGE_CHAN(_chan) { \
> > + .type = IIO_VOLTAGE, \
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_SAMP_FREQ) | \
> > + BIT(IIO_CHAN_INFO_RAW) | \
> > + BIT(IIO_CHAN_INFO_SCALE), \
> > + .output = 1, \
> > + .indexed = 1, \
> > + .channel = (_chan), \
> > + .scan_index = 0, \
> > + .scan_type = { \
> > + .sign = 'u', \
> > + .realbits = 14, \
> > + .storagebits = 16, \
> > + .endianness = IIO_LE, \
> > + }, \
> > + .ext_info = ad8460_ext_info, \
> > + .event_spec = ad8460_events, \
> > + .num_event_specs = ARRAY_SIZE(ad8460_events), \
> > +}
> > +
> > +#define AD8460_CURRENT_CHAN(_chan) { \
> > + .type = IIO_CURRENT, \
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> > + .output = 0, \
>
> I'm guessing this is also not used with the buffer.
> set scan_index = -1 if so.
>
> > + .indexed = 1, \
> > + .channel = (_chan), \
> > + .scan_index = 1, \
> > + .scan_type = { \
> > + .sign = 'u', \
> > + .realbits = 8, \
> > + .storagebits = 8, \
> > + .endianness = IIO_LE, \
> Generally don't provide these if you aren't using them for buffered
> capture. It's just unnecessary noise in the driver.
>
> > + }, \
> > + .event_spec = ad8460_events, \
> > + .num_event_specs = ARRAY_SIZE(ad8460_events), \
> > +}
> > +
> > +#define AD8460_TEMP_CHAN(_chan) { \
> > + .type = IIO_TEMP, \
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> > + .output = 1, \
> > + .indexed = 1, \
> > + .channel = (_chan), \
> > + .scan_index = 2, \
> > + .scan_type = { \
> No sizing?
> I suspect you don't want to present this for the buffered interface.
> If so, set .scan_index = -1
>
Refactored the channel macros like thise
#define AD8460_VOLTAGE_CHAN { \
.type = IIO_VOLTAGE, \
.info_mask_separate = BIT(IIO_CHAN_INFO_SAMP_FREQ) | \
BIT(IIO_CHAN_INFO_RAW) | \
BIT(IIO_CHAN_INFO_SCALE), \
.output = 1, \
.indexed = 1, \
.channel = 0, \
.scan_index = 0, \
.scan_type = { \
.sign = 'u', \
.realbits = 14, \
.storagebits = 16, \
.endianness = IIO_LE, \
}, \
.ext_info = ad8460_ext_info, \
.event_spec = ad8460_events, \
.num_event_specs = ARRAY_SIZE(ad8460_events), \
}
#define AD8460_CURRENT_CHAN { \
.type = IIO_CURRENT, \
.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
.output = 0, \
.indexed = 1, \
.channel = 0, \
.scan_index = -1, \
.event_spec = ad8460_events, \
.num_event_specs = ARRAY_SIZE(ad8460_events), \
}
#define AD8460_TEMP_CHAN { \
.type = IIO_TEMP, \
.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
.output = 1, \
.indexed = 1, \
.channel = 0, \
.scan_index = -1, \
.event_spec = ad8460_events, \
.num_event_specs = 1, \
}
> > + .sign = 'u', \
> > + .endianness = IIO_LE, \
> > + }, \
> > + .event_spec = ad8460_events, \
> > + .num_event_specs = 1, \
> > +}
> > +
> > +static const struct iio_chan_spec ad8460_channels[] = {
> > + AD8460_VOLTAGE_CHAN(0),
> > + AD8460_CURRENT_CHAN(0),
> > +};
> > +
> > +static const struct iio_chan_spec ad8460_channels_with_tmp_adc[] = {
> > + AD8460_VOLTAGE_CHAN(0),
> > + AD8460_CURRENT_CHAN(0),
> > + AD8460_TEMP_CHAN(0),
> > +};
> Unless you plan to very soon add support for devices with more channels,
> drop the parameter and hardcode 0 in the definitions.
>
> Chances are that if you do get a more complex device with more channels
> you will need more than just a channel number anyway so this code
> will change either way. Hence better to keep it simple for now.
>
Dropped the channel parameter
static const struct iio_chan_spec ad8460_channels[] = {
AD8460_VOLTAGE_CHAN,
AD8460_CURRENT_CHAN,
};
static const struct iio_chan_spec ad8460_channels_with_tmp_adc[] = {
AD8460_VOLTAGE_CHAN,
AD8460_CURRENT_CHAN,
AD8460_TEMP_CHAN,
};
>
> > +static int ad8460_probe(struct spi_device *spi)
> > +{
> > + struct ad8460_state *state;
> > + struct iio_dev *indio_dev;
> > + struct regulator *refio_1p2v;
> > + u32 tmp[2], temp;
> > + int ret;
> > +
> > + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*state));
> > + if (!indio_dev)
> > + return -ENOMEM;
> > +
> > + state = iio_priv(indio_dev);
> > + mutex_init(&state->lock);
> > +
> > + indio_dev->name = "ad8460";
> > + indio_dev->info = &ad8460_info;
> > +
> > + state->spi = spi;
> > +
> > + state->regmap = devm_regmap_init_spi(spi,
> &ad8460_regmap_config);
> > + if (IS_ERR(state->regmap))
> > + return dev_err_probe(&spi->dev, PTR_ERR(state->regmap),
> > + "Failed to initialize regmap");
> > +
> > + state->sync_clk = devm_clk_get_enabled(&spi->dev, NULL);
>
> Lots of use of spi->dev, I'd suggest
> struct device *dev = &spi->dev;
> then replace all these instances with that local variable.
>
Applied this structure
struct device *dev;
state->spi = spi;
dev = &spi->dev;
state->regmap = devm_regmap_init_spi(spi, &ad8460_regmap_config);
if (IS_ERR(state->regmap))
return dev_err_probe(dev, PTR_ERR(state->regmap),
"Failed to initialize regmap");
> > + if (IS_ERR(state->sync_clk))
> > + return dev_err_probe(&spi->dev, PTR_ERR(state->sync_clk),
> > + "Failed to get sync clk\n");
> > +
> > + state->tmp_adc_channel = devm_iio_channel_get(&spi->dev, "ad8460-
> tmp");
> > + if (IS_ERR_OR_NULL(state->tmp_adc_channel)) {
> > + indio_dev->channels = ad8460_channels;
> > + indio_dev->num_channels = ARRAY_SIZE(ad8460_channels);
> > + } else {
> > + indio_dev->channels = ad8460_channels_with_tmp_adc;
> > + indio_dev->num_channels =
> ARRAY_SIZE(ad8460_channels_with_tmp_adc);
> > + dev_info(&spi->dev, "Found ADC channel to read TMP pin\n");
>
> That will be apparent anyway once driver is registered, so don't fill the log
> with messages like this. dev_dbg() perhaps or drop it entirely.
>
Dropped the dev_info print
> > + }
> > +
> > + ret = devm_regulator_get_enable_read_voltage(&spi->dev,
> "refio_1p2v");
> > + if (ret == -ENODEV) {
> > + state->refio_1p2v_mv = 1200;
> > + } else if (ret < 0) {
> > + return dev_err_probe(&spi->dev, PTR_ERR(vrefio),
> > + "Failed to get reference voltage\n");
> > + } else {
> > + state->refio_1p2v_mv = ret / 1000;
> > + }
> > +
> > + if (!in_range(ret, AD8460_MIN_VREFIO_UV,
> AD8460_MAX_VREFIO_UV))
> > + return dev_err_probe(&spi->dev, -EINVAL,
> > + "Invalid ref voltage range(%u mV) [%u mV,
> %u mV]\n",
> > + ret, AD8460_MIN_VREFIO_UV / 1000,
>
> Why print ret rather than state->refio_1p2v_mv?
> Having a dev_err_probe() with ret as a later parameter is rather confusing
> if nothing else.
>
Applied changes
ret = devm_regulator_get_enable_read_voltage(dev, "refio_1p2v");
if (ret == -ENODEV)
state->refio_1p2v_mv = 1200;
else if (ret >= 0)
state->refio_1p2v_mv = ret / 1000;
else
return dev_err_probe(dev, ret, "Failed to get reference voltage\n");
if (!in_range(state->refio_1p2v_mv, AD8460_MIN_VREFIO_UV,
AD8460_MAX_VREFIO_UV))
return dev_err_probe(dev, -EINVAL,
"Invalid ref voltage range(%u mV) [%u mV, %u mV]\n",
state->refio_1p2v_mv,
AD8460_MIN_VREFIO_UV / 1000,
AD8460_MAX_VREFIO_UV / 1000);
>
> > + AD8460_MAX_VREFIO_UV / 1000);
> > +
> > + ret = device_property_read_u32(&spi->dev, "adi,rset-ohms", &state-
> >rset_ohms);
> > + if (ret) {
> > + state->rset_ohms = 2000;
> > + } else {
> > + if (!in_range(state->rset_ohms, AD8460_MIN_RSET_OHMS,
> > + AD8460_MAX_RSET_OHMS))
>
> Might as well combine as
>
> else if (!...
> Then can also drop some brackets as single statements in each leg.
>
Applied changes.
ret = device_property_read_u32(dev, "adi,external-resistor-ohms",
&state->ext_resistor_ohms);
if (ret)
state->ext_resistor_ohms = 2000;
else if (!in_range(state->ext_resistor_ohms, AD8460_MIN_EXT_RESISTOR_OHMS,
AD8460_MAX_EXT_RESISTOR_OHMS))
return dev_err_probe(dev, -EINVAL,
"Invalid resistor set range(%u) [%u, %u]\n",
state->ext_resistor_ohms,
AD8460_MIN_EXT_RESISTOR_OHMS,
AD8460_MAX_EXT_RESISTOR_OHMS);
>
> > + return dev_err_probe(&spi->dev, -EINVAL,
> > + "Invalid resistor set range(%u) [%u,
> %u]\n",
> > + state->rset_ohms,
> > + AD8460_MIN_RSET_OHMS,
> > + AD8460_MAX_RSET_OHMS);
> > + }
> > +
> > + /* Arm the device by default */
> > + ret = device_property_read_u32_array(&spi->dev, "adi,range-
> microamp",
> > + tmp, ARRAY_SIZE(tmp));
> > + if (!ret) {
> > + ret = regmap_write(state->regmap, AD8460_CTRL_REG(0x08),
> > + (1 << 7) |
> AD8460_CURRENT_LIMIT_CONV(tmp[1]));
>
> What is the (1 << 7)? Feels like a magic number that should have a define.
>
Used FIELD_PREP for this section. Used existing BITFIELD for (1 << 7)
if (!ret) {
if (in_range(tmp[1], 0, AD8460_ABS_MAX_OVERCURRENT_UA))
regmap_write(state->regmap, AD8460_CTRL_REG(0x08),
FIELD_PREP(AD8460_FAULT_ARM_MSK, 1) |
AD8460_CURRENT_LIMIT_CONV(tmp[1]));
> > + if (ret)
> > + return dev_err_probe(&spi->dev, -EINVAL,
> ret not -EINVAL;
>
> > + "overcurrent src not valid: %d uA",
> > + tmp[1]);
> > +
> > + ret = regmap_write(state->regmap, AD8460_CTRL_REG(0x09),
> > + (1 << 7) |
> AD8460_CURRENT_LIMIT_CONV(abs(tmp[0])));
> > + if (ret)
> > + return dev_err_probe(&spi->dev, -EINVAL,
>
> same here and everywhere else where you are eating a potentially more
> meaningful error
> value.
>
> > + "overcurrent snk not valid: %d uA",
> > + tmp[0]);
> > + }
> > +
> > + ret = device_property_read_u32_array(&spi->dev, "adi,range-
> microvolt",
> > + tmp, ARRAY_SIZE(tmp));
> > + if (!ret) {
>
> This is currently documented in the binding as only taking one set of values.
> Seems a lot more flexible here.
>
Changed bindings so that this value would be flexible
> > + ret = regmap_write(state->regmap, AD8460_CTRL_REG(0x0A),
> > + (1 << 7) |
> AD8460_VOLTAGE_LIMIT_CONV(tmp[1]));
> > + if (ret)
> > + return dev_err_probe(&spi->dev, -EINVAL,
> > + "positive overvoltage not valid: %d
> uV",
> > + tmp[1]);
> > +
> > + ret = regmap_write(state->regmap, AD8460_CTRL_REG(0x0B),
> > + (1 << 7) |
> AD8460_VOLTAGE_LIMIT_CONV(abs(tmp[0])));
> > + if (ret)
> > + return dev_err_probe(&spi->dev, -EINVAL,
> > + "negative overvoltage not valid: %d
> uV",
> > + tmp[0]);
> > + }
> > +
> > + ret = device_property_read_u32(&spi->dev, "adi,temp-max-
> millicelsius", &temp);
> I'd handled as a default of whatever is already in that
> register. Just write temp = default; before this call.
>
> That way you just don't check ret for the property query. If it's there the
> value in temp will be updated, if not we'll have the default.
>
> Consider similar for the other cases.
> It will make it easier to tell from the driver what happens if a property is not
> provided. Also document defaults in the dt binding.
>
Regarding the default, I just made sure that the register would only be updated
If the custom attribute is defined in the device tree, and if the value is within
Range, as per the datasheet. If it is not met, the register will not be touched
/* Arm the device by default */
ret = device_property_read_u32_array(dev, "adi,range-microamp",
tmp, ARRAY_SIZE(tmp));
if (!ret) {
if (in_range(tmp[1], 0, AD8460_ABS_MAX_OVERCURRENT_UA))
regmap_write(state->regmap, AD8460_CTRL_REG(0x08),
FIELD_PREP(AD8460_FAULT_ARM_MSK, 1) |
AD8460_CURRENT_LIMIT_CONV(tmp[1]));
if (in_range(tmp[0], -AD8460_ABS_MAX_OVERCURRENT_UA, 0))
regmap_write(state->regmap, AD8460_CTRL_REG(0x09),
FIELD_PREP(AD8460_FAULT_ARM_MSK, 1) |
AD8460_CURRENT_LIMIT_CONV(abs(tmp[0])));
}
Should there be a processing for the return of regmap_write? I didn't handle it here
> > + if (!ret) {
> > + ret = regmap_write(state->regmap, AD8460_CTRL_REG(0x0C),
> > + (1 << 7) |
> AD8460_TEMP_LIMIT_CONV(abs(temp)));
> > + if (ret)
> > + return dev_err_probe(&spi->dev, -EINVAL,
> > + "overtemperature not valid: %d",
> > + temp);
> > + }
> > +
> > + ret = ad8460_reset(state);
> > + if (ret)
> > + return ret;
> > +
> > + /* Enables DAC by default */
> > + ret = regmap_update_bits(state->regmap, AD8460_CTRL_REG(0x01),
> > + AD8460_HVDAC_SLEEP_MSK,
> > + FIELD_PREP(AD8460_HVDAC_SLEEP_MSK,
> 0));
> > + if (ret)
> > + return ret;
> > +
> > + indio_dev->modes = INDIO_DIRECT_MODE;
> > + indio_dev->setup_ops = &ad8460_buffer_setup_ops;
> > +
> > + ret = devm_iio_dmaengine_buffer_setup_ext(&spi->dev, indio_dev,
> "tx",
> > +
> IIO_BUFFER_DIRECTION_OUT);
> > + if (ret)
> > + return dev_err_probe(&spi->dev, ret,
> > + "Failed to get DMA buffer\n");
> > +
> > + ret = devm_iio_device_register(&spi->dev, indio_dev);
>
> return devm_iio_device_register()
>
> > + if (ret)
> > + return ret;
> > +
> > + return 0;
> > +}
Best regards,
Mariel
Powered by blists - more mailing lists