Re: [PATCH v10 3/3] iio: dac: Add AD5529R DAC driver support

Add support for AD5529R 16-channel, 12/16 bit Digital to Analog Converter
from Analog Devices.

The device communicates over SPI and supports per-channel output range
configuration. An optional external 4.096V reference can be used in
place of the internal reference.

Signed-off-by: Janani Sunil <janani.sunil@analog.com>

Just a couple of minor additions from me.

Why do we need those outer () ?

...

+static int ad5529r_parse_channel_ranges(struct device *dev,
+					struct ad5529r_state *st)
+{
+	unsigned long channel_mask = 0;
+	s32 vals[2];

Where possible, I'd move these into narrower scope.
That tends to make the code a little easier to reason about.

+	int ret, range_idx;
+	u32 ch;
+
+	device_for_each_child_node_scoped(dev, child) {
+		if (st->num_channels == ARRAY_SIZE(st->channels))
+			return dev_err_probe(dev, -ECHRNG, "Too many channels\n");
+
+		ret = fwnode_property_read_u32(child, "reg", &ch);
+		if (ret)
+			return dev_err_probe(dev, ret,
+					     "Missing reg property in channel node\n");
+
+		if (ch >= AD5529R_MAX_CHANNELS)
+			return dev_err_probe(dev, -EINVAL,
+					     "Channel %u exceeds maximum 15\n",
+					     ch);
+
+		if (channel_mask & BIT(ch))
+			return dev_err_probe(dev, -EINVAL,
+					     "Duplicate channel %u\n", ch);
+
+		channel_mask |= BIT(ch);
+
+		if (fwnode_property_present(child, "output-range-microvolt")) {
+			/*
+			 * DT stores cells as raw 32-bit values; signed endpoints are
+			 * encoded by dtc in two's-complement and then interpreted
+			 * here as s32.
+			 */
+			ret = fwnode_property_read_u32_array(child,
+							     "output-range-microvolt",
+							     (u32 *)vals, ARRAY_SIZE(vals));
+			if (ret < 0)
+				return dev_err_probe(dev, ret,
+						     "Failed to read range for ch %u\n",
+						     ch);
+
+			range_idx = ad5529r_find_output_range(vals);
+			if (range_idx < 0)
+				return dev_err_probe(dev, range_idx,
+						     "Invalid range [%d %d] for ch %u\n",
+						     vals[0], vals[1], ch);
+		} else {
+			range_idx = AD5529R_RANGE_0V_5V;
+		}
+
+		st->output_range_idx[ch] = range_idx;
+		ret = regmap_write(st->regmap_16bit,
+				   AD5529R_REG_OUT_RANGE(ch), range_idx);
+		if (ret)
+			return dev_err_probe(dev, ret,
+					     "Failed to configure range for ch %u\n",
+					     ch);
+
+		st->channels[st->num_channels++] = AD5529R_DAC_CHANNEL(ch);
+	}
+
+	return 0;
+}
+

+static int ad5529r_probe(struct spi_device *spi)
+{
+	struct regmap_config regmap_16bit_cfg;
+	struct regmap_config regmap_8bit_cfg;
+	struct device *dev = &spi->dev;
+	struct iio_dev *indio_dev;
+	struct ad5529r_state *st;
+	bool external_vref;
+	u32 dev_addr = 0;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	st = iio_priv(indio_dev);
+
+	st->model_data = spi_get_device_match_data(spi);
+	if (!st->model_data)
+		return dev_err_probe(dev, -ENODATA,
+				     "Failed to identify device variant\n");
+

Move the default value setting of dev_addr() down here.
	dev_addr = 0;

or better yet, check for presence of property
	if (device_property_present(dev, "spi-device-addr")) {
		ret = device_property_read_u32(dev, "spi-dev-addr", &dev_addr);
		if (ret < 0)
			return ret;

		if (dev_addr > 3)
			return dev_err_probe();
	} else {
		dev_addr = 0;
	}
as that gives us clean checking for corrupted properties and setting
of a default if no attempt was made ot set it to anything else.

+	device_property_read_u32(dev, "spi-device-addr", &dev_addr);
+	if (dev_addr > 3)
+		return dev_err_probe(dev, -EINVAL,
+				     "spi-device-addr %u out of range [0, 3]\n",
+				     dev_addr);

Thanks

Jonathan

-- 
Jonathan Cameron [off-list ref]