From: Angelo Dureghello <adureghello@baylibre.com>

Extracting common code, to share common code to be used later
by the AXI driver version (ad3552r-axi.c).

Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
---
 drivers/iio/dac/Makefile         |   2 +-
 drivers/iio/dac/ad3552r-common.c | 173 +++++++++++++++++++++++
 drivers/iio/dac/ad3552r.c        | 293 ++++-----------------------------------
 drivers/iio/dac/ad3552r.h        | 190 +++++++++++++++++++++++++
 4 files changed, 390 insertions(+), 268 deletions(-)

diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile
index 2cf148f16306..56a125f56284 100644
--- a/drivers/iio/dac/Makefile
+++ b/drivers/iio/dac/Makefile
@@ -4,7 +4,7 @@
 #
 
 # When adding new entries keep the list in alphabetical order
-obj-$(CONFIG_AD3552R) += ad3552r.o
+obj-$(CONFIG_AD3552R) += ad3552r.o ad3552r-common.o
 obj-$(CONFIG_AD5360) += ad5360.o
 obj-$(CONFIG_AD5380) += ad5380.o
 obj-$(CONFIG_AD5421) += ad5421.o
diff --git a/drivers/iio/dac/ad3552r-common.c b/drivers/iio/dac/ad3552r-common.c
new file mode 100644
index 000000000000..624f3f97cdea
--- /dev/null
+++ b/drivers/iio/dac/ad3552r-common.c
@@ -0,0 +1,173 @@
+// SPDX-License-Identifier: GPL-2.0+
+//
+// Copyright (c) 2010-2024 Analog Devices Inc.
+// Copyright (c) 2024 Baylibre, SAS
+
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/property.h>
+#include <linux/regulator/consumer.h>
+
+#include "ad3552r.h"
+
+const s32 ad3552r_ch_ranges[AD3552R_MAX_RANGES][2] = {
+	[AD3552R_CH_OUTPUT_RANGE_0__2P5V]	= { 0, 2500 },
+	[AD3552R_CH_OUTPUT_RANGE_0__5V]		= { 0, 5000 },
+	[AD3552R_CH_OUTPUT_RANGE_0__10V]	= { 0, 10000 },
+	[AD3552R_CH_OUTPUT_RANGE_NEG_5__5V]	= { -5000, 5000 },
+	[AD3552R_CH_OUTPUT_RANGE_NEG_10__10V]	= { -10000, 10000 }
+};
+EXPORT_SYMBOL(ad3552r_ch_ranges);
+
+const s32 ad3542r_ch_ranges[AD3542R_MAX_RANGES][2] = {
+	[AD3542R_CH_OUTPUT_RANGE_0__2P5V]	= { 0, 2500 },
+	[AD3542R_CH_OUTPUT_RANGE_0__3V]		= { 0, 3000 },
+	[AD3542R_CH_OUTPUT_RANGE_0__5V]		= { 0, 5000 },
+	[AD3542R_CH_OUTPUT_RANGE_0__10V]	= { 0, 10000 },
+	[AD3542R_CH_OUTPUT_RANGE_NEG_2P5__7P5V]	= { -2500, 7500 },
+	[AD3542R_CH_OUTPUT_RANGE_NEG_5__5V]	= { -5000, 5000 }
+};
+EXPORT_SYMBOL(ad3542r_ch_ranges);
+
+u16 ad3552r_calc_custom_gain(u8 p, u8 n, s16 goffs)
+{
+	u16 reg;
+
+	reg = FIELD_PREP(AD3552R_MASK_CH_RANGE_OVERRIDE, 1);
+	reg |= FIELD_PREP(AD3552R_MASK_CH_GAIN_SCALING_P, p);
+	reg |= FIELD_PREP(AD3552R_MASK_CH_GAIN_SCALING_N, n);
+	reg |= FIELD_PREP(AD3552R_MASK_CH_OFFSET_BIT_8, abs((s32)goffs) >> 8);
+	reg |= FIELD_PREP(AD3552R_MASK_CH_OFFSET_POLARITY, (s32)goffs < 0);
+
+	return reg;
+}
+
+int ad3552r_get_ref_voltage(struct device *dev)
+{
+	int voltage;
+	int delta = 100000;
+
+	voltage = devm_regulator_get_enable_read_voltage(dev, "vref");
+	if (voltage < 0 && voltage != -ENODEV)
+		return dev_err_probe(dev, voltage,
+				     "Error getting vref voltage\n");
+
+	if (voltage == -ENODEV) {
+		if (device_property_read_bool(dev, "adi,vref-out-en"))
+			return AD3552R_INTERNAL_VREF_PIN_2P5V;
+		else
+			return AD3552R_INTERNAL_VREF_PIN_FLOATING;
+	}
+
+	if (voltage > 2500000 + delta || voltage < 2500000 - delta) {
+		dev_warn(dev, "vref-supply must be 2.5V");
+		return -EINVAL;
+	}
+
+	return AD3552R_EXTERNAL_VREF_PIN_INPUT;
+}
+
+int ad3552r_get_drive_strength(struct device *dev, u32 *val)
+{
+	int err;
+
+	err = device_property_read_u32(dev, "adi,sdo-drive-strength", val);
+	if (err)
+		return err;
+
+	if (*val > 3) {
+		dev_err(dev,
+			"adi,sdo-drive-strength must be less than 4\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+int ad3552r_get_custom_gain(struct device *dev, struct fwnode_handle *child,
+			    u8 *gs_p, u8 *gs_n, u16 *rfb, s16 *goffs)
+{
+	int err;
+	u32 val;
+	struct fwnode_handle *gain_child __free(fwnode_handle) =
+				fwnode_get_named_child_node(child,
+				"custom-output-range-config");
+
+	if (!gain_child)
+		return dev_err_probe(dev, -EINVAL,
+				     "custom-output-range-config mandatory\n");
+
+	err = fwnode_property_read_u32(gain_child, "adi,gain-scaling-p", &val);
+	if (err)
+		return dev_err_probe(dev, err,
+				     "adi,gain-scaling-p mandatory\n");
+	*gs_p = val;
+
+	err = fwnode_property_read_u32(gain_child, "adi,gain-scaling-n", &val);
+	if (err)
+		return dev_err_probe(dev, err,
+				     "adi,gain-scaling-n property mandatory\n");
+	*gs_n = val;
+
+	err = fwnode_property_read_u32(gain_child, "adi,rfb-ohms", &val);
+	if (err)
+		return dev_err_probe(dev, err,
+				     "adi,rfb-ohms mandatory\n");
+	*rfb = val;
+
+	err = fwnode_property_read_u32(gain_child, "adi,gain-offset", &val);
+	if (err)
+		return dev_err_probe(dev, err,
+				     "adi,gain-offset mandatory\n");
+	*goffs = val;
+
+	return 0;
+}
+
+static int ad3552r_find_range(u16 id, s32 *vals)
+{
+	int i, len;
+	const s32 (*ranges)[2];
+
+	if (id == AD3542R_ID) {
+		len = ARRAY_SIZE(ad3542r_ch_ranges);
+		ranges = ad3542r_ch_ranges;
+	} else {
+		len = ARRAY_SIZE(ad3552r_ch_ranges);
+		ranges = ad3552r_ch_ranges;
+	}
+
+	for (i = 0; i < len; i++)
+		if (vals[0] == ranges[i][0] * 1000 &&
+		    vals[1] == ranges[i][1] * 1000)
+			return i;
+
+	return -EINVAL;
+}
+
+int ad3552r_get_output_range(struct device *dev, enum ad3552r_id chip_id,
+			     struct fwnode_handle *child, u32 *val)
+{
+	int ret;
+	s32 vals[2];
+
+	/* This property is optional, so returning -ENOENT if missing */
+	if (!fwnode_property_present(child, "adi,output-range-microvolt"))
+		return -ENOENT;
+
+	ret = fwnode_property_read_u32_array(child,
+					     "adi,output-range-microvolt",
+					     vals, 2);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				"invalid adi,output-range-microvolt\n");
+
+	ret = ad3552r_find_range(chip_id, vals);
+	if (ret < 0)
+		return dev_err_probe(dev, ret,
+			"invalid adi,output-range-microvolt value\n");
+
+	*val = ret;
+
+	return 0;
+}
diff --git a/drivers/iio/dac/ad3552r.c b/drivers/iio/dac/ad3552r.c
index c27706c5ba10..173282e5e1a1 100644
--- a/drivers/iio/dac/ad3552r.c
+++ b/drivers/iio/dac/ad3552r.c
@@ -11,185 +11,9 @@
 #include <linux/iio/trigger_consumer.h>
 #include <linux/iopoll.h>
 #include <linux/kernel.h>
-#include <linux/regulator/consumer.h>
 #include <linux/spi/spi.h>
 
-/* Register addresses */
-/* Primary address space */
-#define AD3552R_REG_ADDR_INTERFACE_CONFIG_A		0x00
-#define   AD3552R_MASK_SOFTWARE_RESET			(BIT(7) | BIT(0))
-#define   AD3552R_MASK_ADDR_ASCENSION			BIT(5)
-#define   AD3552R_MASK_SDO_ACTIVE			BIT(4)
-#define AD3552R_REG_ADDR_INTERFACE_CONFIG_B		0x01
-#define   AD3552R_MASK_SINGLE_INST			BIT(7)
-#define   AD3552R_MASK_SHORT_INSTRUCTION		BIT(3)
-#define AD3552R_REG_ADDR_DEVICE_CONFIG			0x02
-#define   AD3552R_MASK_DEVICE_STATUS(n)			BIT(4 + (n))
-#define   AD3552R_MASK_CUSTOM_MODES			GENMASK(3, 2)
-#define   AD3552R_MASK_OPERATING_MODES			GENMASK(1, 0)
-#define AD3552R_REG_ADDR_CHIP_TYPE			0x03
-#define   AD3552R_MASK_CLASS				GENMASK(7, 0)
-#define AD3552R_REG_ADDR_PRODUCT_ID_L			0x04
-#define AD3552R_REG_ADDR_PRODUCT_ID_H			0x05
-#define AD3552R_REG_ADDR_CHIP_GRADE			0x06
-#define   AD3552R_MASK_GRADE				GENMASK(7, 4)
-#define   AD3552R_MASK_DEVICE_REVISION			GENMASK(3, 0)
-#define AD3552R_REG_ADDR_SCRATCH_PAD			0x0A
-#define AD3552R_REG_ADDR_SPI_REVISION			0x0B
-#define AD3552R_REG_ADDR_VENDOR_L			0x0C
-#define AD3552R_REG_ADDR_VENDOR_H			0x0D
-#define AD3552R_REG_ADDR_STREAM_MODE			0x0E
-#define   AD3552R_MASK_LENGTH				GENMASK(7, 0)
-#define AD3552R_REG_ADDR_TRANSFER_REGISTER		0x0F
-#define   AD3552R_MASK_MULTI_IO_MODE			GENMASK(7, 6)
-#define   AD3552R_MASK_STREAM_LENGTH_KEEP_VALUE		BIT(2)
-#define AD3552R_REG_ADDR_INTERFACE_CONFIG_C		0x10
-#define   AD3552R_MASK_CRC_ENABLE			(GENMASK(7, 6) |\
-							 GENMASK(1, 0))
-#define   AD3552R_MASK_STRICT_REGISTER_ACCESS		BIT(5)
-#define AD3552R_REG_ADDR_INTERFACE_STATUS_A		0x11
-#define   AD3552R_MASK_INTERFACE_NOT_READY		BIT(7)
-#define   AD3552R_MASK_CLOCK_COUNTING_ERROR		BIT(5)
-#define   AD3552R_MASK_INVALID_OR_NO_CRC		BIT(3)
-#define   AD3552R_MASK_WRITE_TO_READ_ONLY_REGISTER	BIT(2)
-#define   AD3552R_MASK_PARTIAL_REGISTER_ACCESS		BIT(1)
-#define   AD3552R_MASK_REGISTER_ADDRESS_INVALID		BIT(0)
-#define AD3552R_REG_ADDR_INTERFACE_CONFIG_D		0x14
-#define   AD3552R_MASK_ALERT_ENABLE_PULLUP		BIT(6)
-#define   AD3552R_MASK_MEM_CRC_EN			BIT(4)
-#define   AD3552R_MASK_SDO_DRIVE_STRENGTH		GENMASK(3, 2)
-#define   AD3552R_MASK_DUAL_SPI_SYNCHROUNOUS_EN		BIT(1)
-#define   AD3552R_MASK_SPI_CONFIG_DDR			BIT(0)
-#define AD3552R_REG_ADDR_SH_REFERENCE_CONFIG		0x15
-#define   AD3552R_MASK_IDUMP_FAST_MODE			BIT(6)
-#define   AD3552R_MASK_SAMPLE_HOLD_DIFFERENTIAL_USER_EN	BIT(5)
-#define   AD3552R_MASK_SAMPLE_HOLD_USER_TRIM		GENMASK(4, 3)
-#define   AD3552R_MASK_SAMPLE_HOLD_USER_ENABLE		BIT(2)
-#define   AD3552R_MASK_REFERENCE_VOLTAGE_SEL		GENMASK(1, 0)
-#define AD3552R_REG_ADDR_ERR_ALARM_MASK			0x16
-#define   AD3552R_MASK_REF_RANGE_ALARM			BIT(6)
-#define   AD3552R_MASK_CLOCK_COUNT_ERR_ALARM		BIT(5)
-#define   AD3552R_MASK_MEM_CRC_ERR_ALARM		BIT(4)
-#define   AD3552R_MASK_SPI_CRC_ERR_ALARM		BIT(3)
-#define   AD3552R_MASK_WRITE_TO_READ_ONLY_ALARM		BIT(2)
-#define   AD3552R_MASK_PARTIAL_REGISTER_ACCESS_ALARM	BIT(1)
-#define   AD3552R_MASK_REGISTER_ADDRESS_INVALID_ALARM	BIT(0)
-#define AD3552R_REG_ADDR_ERR_STATUS			0x17
-#define   AD3552R_MASK_REF_RANGE_ERR_STATUS			BIT(6)
-#define   AD3552R_MASK_DUAL_SPI_STREAM_EXCEEDS_DAC_ERR_STATUS	BIT(5)
-#define   AD3552R_MASK_MEM_CRC_ERR_STATUS			BIT(4)
-#define   AD3552R_MASK_RESET_STATUS				BIT(0)
-#define AD3552R_REG_ADDR_POWERDOWN_CONFIG		0x18
-#define   AD3552R_MASK_CH_DAC_POWERDOWN(ch)		BIT(4 + (ch))
-#define   AD3552R_MASK_CH_AMPLIFIER_POWERDOWN(ch)	BIT(ch)
-#define AD3552R_REG_ADDR_CH0_CH1_OUTPUT_RANGE		0x19
-#define   AD3552R_MASK_CH_OUTPUT_RANGE_SEL(ch)		((ch) ? GENMASK(7, 4) :\
-							 GENMASK(3, 0))
-#define AD3552R_REG_ADDR_CH_OFFSET(ch)			(0x1B + (ch) * 2)
-#define   AD3552R_MASK_CH_OFFSET_BITS_0_7		GENMASK(7, 0)
-#define AD3552R_REG_ADDR_CH_GAIN(ch)			(0x1C + (ch) * 2)
-#define   AD3552R_MASK_CH_RANGE_OVERRIDE		BIT(7)
-#define   AD3552R_MASK_CH_GAIN_SCALING_N		GENMASK(6, 5)
-#define   AD3552R_MASK_CH_GAIN_SCALING_P		GENMASK(4, 3)
-#define   AD3552R_MASK_CH_OFFSET_POLARITY		BIT(2)
-#define   AD3552R_MASK_CH_OFFSET_BIT_8			BIT(0)
-/*
- * Secondary region
- * For multibyte registers specify the highest address because the access is
- * done in descending order
- */
-#define AD3552R_SECONDARY_REGION_START			0x28
-#define AD3552R_REG_ADDR_HW_LDAC_16B			0x28
-#define AD3552R_REG_ADDR_CH_DAC_16B(ch)			(0x2C - (1 - ch) * 2)
-#define AD3552R_REG_ADDR_DAC_PAGE_MASK_16B		0x2E
-#define AD3552R_REG_ADDR_CH_SELECT_16B			0x2F
-#define AD3552R_REG_ADDR_INPUT_PAGE_MASK_16B		0x31
-#define AD3552R_REG_ADDR_SW_LDAC_16B			0x32
-#define AD3552R_REG_ADDR_CH_INPUT_16B(ch)		(0x36 - (1 - ch) * 2)
-/* 3 bytes registers */
-#define AD3552R_REG_START_24B				0x37
-#define AD3552R_REG_ADDR_HW_LDAC_24B			0x37
-#define AD3552R_REG_ADDR_CH_DAC_24B(ch)			(0x3D - (1 - ch) * 3)
-#define AD3552R_REG_ADDR_DAC_PAGE_MASK_24B		0x40
-#define AD3552R_REG_ADDR_CH_SELECT_24B			0x41
-#define AD3552R_REG_ADDR_INPUT_PAGE_MASK_24B		0x44
-#define AD3552R_REG_ADDR_SW_LDAC_24B			0x45
-#define AD3552R_REG_ADDR_CH_INPUT_24B(ch)		(0x4B - (1 - ch) * 3)
-
-/* Useful defines */
-#define AD3552R_MAX_CH					2
-#define AD3552R_MASK_CH(ch)				BIT(ch)
-#define AD3552R_MASK_ALL_CH				GENMASK(1, 0)
-#define AD3552R_MAX_REG_SIZE				3
-#define AD3552R_READ_BIT				BIT(7)
-#define AD3552R_ADDR_MASK				GENMASK(6, 0)
-#define AD3552R_MASK_DAC_12B				0xFFF0
-#define AD3552R_DEFAULT_CONFIG_B_VALUE			0x8
-#define AD3552R_SCRATCH_PAD_TEST_VAL1			0x34
-#define AD3552R_SCRATCH_PAD_TEST_VAL2			0xB2
-#define AD3552R_GAIN_SCALE				1000
-#define AD3552R_LDAC_PULSE_US				100
-
-enum ad3552r_ch_vref_select {
-	/* Internal source with Vref I/O floating */
-	AD3552R_INTERNAL_VREF_PIN_FLOATING,
-	/* Internal source with Vref I/O at 2.5V */
-	AD3552R_INTERNAL_VREF_PIN_2P5V,
-	/* External source with Vref I/O as input */
-	AD3552R_EXTERNAL_VREF_PIN_INPUT
-};
-
-enum ad3552r_id {
-	AD3541R_ID = 0x400b,
-	AD3542R_ID = 0x4009,
-	AD3551R_ID = 0x400a,
-	AD3552R_ID = 0x4008,
-};
-
-enum ad3552r_ch_output_range {
-	/* Range from 0 V to 2.5 V. Requires Rfb1x connection */
-	AD3552R_CH_OUTPUT_RANGE_0__2P5V,
-	/* Range from 0 V to 5 V. Requires Rfb1x connection  */
-	AD3552R_CH_OUTPUT_RANGE_0__5V,
-	/* Range from 0 V to 10 V. Requires Rfb2x connection  */
-	AD3552R_CH_OUTPUT_RANGE_0__10V,
-	/* Range from -5 V to 5 V. Requires Rfb2x connection  */
-	AD3552R_CH_OUTPUT_RANGE_NEG_5__5V,
-	/* Range from -10 V to 10 V. Requires Rfb4x connection  */
-	AD3552R_CH_OUTPUT_RANGE_NEG_10__10V,
-};
-
-static const s32 ad3552r_ch_ranges[][2] = {
-	[AD3552R_CH_OUTPUT_RANGE_0__2P5V]	= {0, 2500},
-	[AD3552R_CH_OUTPUT_RANGE_0__5V]		= {0, 5000},
-	[AD3552R_CH_OUTPUT_RANGE_0__10V]	= {0, 10000},
-	[AD3552R_CH_OUTPUT_RANGE_NEG_5__5V]	= {-5000, 5000},
-	[AD3552R_CH_OUTPUT_RANGE_NEG_10__10V]	= {-10000, 10000}
-};
-
-enum ad3542r_ch_output_range {
-	/* Range from 0 V to 2.5 V. Requires Rfb1x connection */
-	AD3542R_CH_OUTPUT_RANGE_0__2P5V,
-	/* Range from 0 V to 3 V. Requires Rfb1x connection  */
-	AD3542R_CH_OUTPUT_RANGE_0__3V,
-	/* Range from 0 V to 5 V. Requires Rfb1x connection  */
-	AD3542R_CH_OUTPUT_RANGE_0__5V,
-	/* Range from 0 V to 10 V. Requires Rfb2x connection  */
-	AD3542R_CH_OUTPUT_RANGE_0__10V,
-	/* Range from -2.5 V to 7.5 V. Requires Rfb2x connection  */
-	AD3542R_CH_OUTPUT_RANGE_NEG_2P5__7P5V,
-	/* Range from -5 V to 5 V. Requires Rfb2x connection  */
-	AD3542R_CH_OUTPUT_RANGE_NEG_5__5V,
-};
-
-static const s32 ad3542r_ch_ranges[][2] = {
-	[AD3542R_CH_OUTPUT_RANGE_0__2P5V]	= {0, 2500},
-	[AD3542R_CH_OUTPUT_RANGE_0__3V]		= {0, 3000},
-	[AD3542R_CH_OUTPUT_RANGE_0__5V]		= {0, 5000},
-	[AD3542R_CH_OUTPUT_RANGE_0__10V]	= {0, 10000},
-	[AD3542R_CH_OUTPUT_RANGE_NEG_2P5__7P5V]	= {-2500, 7500},
-	[AD3542R_CH_OUTPUT_RANGE_NEG_5__5V]	= {-5000, 5000}
-};
+#include "ad3552r.h"
 
 enum ad3552r_ch_gain_scaling {
 	/* Gain scaling of 1 */
@@ -693,75 +517,35 @@ static void ad3552r_calc_gain_and_offset(struct ad3552r_desc *dac, s32 ch)
 	dac->ch_data[ch].offset_dec = div_s64(tmp, span);
 }
 
-static int ad3552r_find_range(const struct ad3552r_model_data *model_data,
-			      s32 *vals)
-{
-	int i;
-
-	for (i = 0; i < model_data->num_ranges; i++)
-		if (vals[0] == model_data->ranges_table[i][0] * 1000 &&
-		    vals[1] == model_data->ranges_table[i][1] * 1000)
-			return i;
-
-	return -EINVAL;
-}
-
 static int ad3552r_configure_custom_gain(struct ad3552r_desc *dac,
 					 struct fwnode_handle *child,
 					 u32 ch)
 {
 	struct device *dev = &dac->spi->dev;
-	u32 val;
 	int err;
 	u8 addr;
-	u16 reg = 0, offset;
-
-	struct fwnode_handle *gain_child __free(fwnode_handle)
-		= fwnode_get_named_child_node(child,
-					      "custom-output-range-config");
-	if (!gain_child)
-		return dev_err_probe(dev, -EINVAL,
-				     "mandatory custom-output-range-config property missing\n");
-
-	dac->ch_data[ch].range_override = 1;
-	reg |= FIELD_PREP(AD3552R_MASK_CH_RANGE_OVERRIDE, 1);
-
-	err = fwnode_property_read_u32(gain_child, "adi,gain-scaling-p", &val);
-	if (err)
-		return dev_err_probe(dev, err,
-				     "mandatory adi,gain-scaling-p property missing\n");
-	reg |= FIELD_PREP(AD3552R_MASK_CH_GAIN_SCALING_P, val);
-	dac->ch_data[ch].p = val;
-
-	err = fwnode_property_read_u32(gain_child, "adi,gain-scaling-n", &val);
-	if (err)
-		return dev_err_probe(dev, err,
-				     "mandatory adi,gain-scaling-n property missing\n");
-	reg |= FIELD_PREP(AD3552R_MASK_CH_GAIN_SCALING_N, val);
-	dac->ch_data[ch].n = val;
-
-	err = fwnode_property_read_u32(gain_child, "adi,rfb-ohms", &val);
-	if (err)
-		return dev_err_probe(dev, err,
-				     "mandatory adi,rfb-ohms property missing\n");
-	dac->ch_data[ch].rfb = val;
+	u16 reg;
 
-	err = fwnode_property_read_u32(gain_child, "adi,gain-offset", &val);
+	err = ad3552r_get_custom_gain(dev, child,
+				      &dac->ch_data[ch].p,
+				      &dac->ch_data[ch].n,
+				      &dac->ch_data[ch].rfb,
+				      &dac->ch_data[ch].gain_offset);
 	if (err)
-		return dev_err_probe(dev, err,
-				     "mandatory adi,gain-offset property missing\n");
-	dac->ch_data[ch].gain_offset = val;
+		return err;
 
-	offset = abs((s32)val);
-	reg |= FIELD_PREP(AD3552R_MASK_CH_OFFSET_BIT_8, (offset >> 8));
+	dac->ch_data[ch].range_override = 1;
 
-	reg |= FIELD_PREP(AD3552R_MASK_CH_OFFSET_POLARITY, (s32)val < 0);
 	addr = AD3552R_REG_ADDR_CH_GAIN(ch);
 	err = ad3552r_write_reg(dac, addr,
-				offset & AD3552R_MASK_CH_OFFSET_BITS_0_7);
+				abs((s32)dac->ch_data[ch].gain_offset) &
+				AD3552R_MASK_CH_OFFSET_BITS_0_7);
 	if (err)
 		return dev_err_probe(dev, err, "Error writing register\n");
 
+	reg = ad3552r_calc_custom_gain(dac->ch_data[ch].p, dac->ch_data[ch].n,
+				       dac->ch_data[ch].gain_offset);
+
 	err = ad3552r_write_reg(dac, addr, reg);
 	if (err)
 		return dev_err_probe(dev, err, "Error writing register\n");
@@ -772,30 +556,19 @@ static int ad3552r_configure_custom_gain(struct ad3552r_desc *dac,
 static int ad3552r_configure_device(struct ad3552r_desc *dac)
 {
 	struct device *dev = &dac->spi->dev;
-	int err, cnt = 0, voltage, delta = 100000;
-	u32 vals[2], val, ch;
+	int err, cnt = 0;
+	u32 val, ch;
 
 	dac->gpio_ldac = devm_gpiod_get_optional(dev, "ldac", GPIOD_OUT_HIGH);
 	if (IS_ERR(dac->gpio_ldac))
 		return dev_err_probe(dev, PTR_ERR(dac->gpio_ldac),
 				     "Error getting gpio ldac");
 
-	voltage = devm_regulator_get_enable_read_voltage(dev, "vref");
-	if (voltage < 0 && voltage != -ENODEV)
-		return dev_err_probe(dev, voltage, "Error getting vref voltage\n");
+	err = ad3552r_get_ref_voltage(dev);
+	if (err < 0)
+		return err;
 
-	if (voltage == -ENODEV) {
-		if (device_property_read_bool(dev, "adi,vref-out-en"))
-			val = AD3552R_INTERNAL_VREF_PIN_2P5V;
-		else
-			val = AD3552R_INTERNAL_VREF_PIN_FLOATING;
-	} else {
-		if (voltage > 2500000 + delta || voltage < 2500000 - delta) {
-			dev_warn(dev, "vref-supply must be 2.5V");
-			return -EINVAL;
-		}
-		val = AD3552R_EXTERNAL_VREF_PIN_INPUT;
-	}
+	val = err;
 
 	err = ad3552r_update_reg_field(dac,
 				       AD3552R_REG_ADDR_SH_REFERENCE_CONFIG,
@@ -804,13 +577,8 @@ static int ad3552r_configure_device(struct ad3552r_desc *dac)
 	if (err)
 		return err;
 
-	err = device_property_read_u32(dev, "adi,sdo-drive-strength", &val);
+	err = ad3552r_get_drive_strength(dev, &val);
 	if (!err) {
-		if (val > 3) {
-			dev_err(dev, "adi,sdo-drive-strength must be less than 4\n");
-			return -EINVAL;
-		}
-
 		err = ad3552r_update_reg_field(dac,
 					       AD3552R_REG_ADDR_INTERFACE_CONFIG_D,
 					       AD3552R_MASK_SDO_DRIVE_STRENGTH,
@@ -835,21 +603,12 @@ static int ad3552r_configure_device(struct ad3552r_desc *dac)
 					     "reg must be less than %d\n",
 					     dac->model_data->num_hw_channels);
 
-		if (fwnode_property_present(child, "adi,output-range-microvolt")) {
-			err = fwnode_property_read_u32_array(child,
-							     "adi,output-range-microvolt",
-							     vals,
-							     2);
-			if (err)
-				return dev_err_probe(dev, err,
-					"adi,output-range-microvolt property could not be parsed\n");
-
-			err = ad3552r_find_range(dac->model_data, vals);
-			if (err < 0)
-				return dev_err_probe(dev, err,
-						     "Invalid adi,output-range-microvolt value\n");
+		err = ad3552r_get_output_range(dev, dac->model_data->chip_id,
+					       child, &val);
+		if (err && err != -ENOENT)
+			return err;
 
-			val = err;
+		if (!err) {
 			if (ch == 0)
 				val = FIELD_PREP(AD3552R_MASK_CH_OUTPUT_RANGE_SEL(0), val);
 			else
diff --git a/drivers/iio/dac/ad3552r.h b/drivers/iio/dac/ad3552r.h
new file mode 100644
index 000000000000..b1caa3c3e807
--- /dev/null
+++ b/drivers/iio/dac/ad3552r.h
@@ -0,0 +1,190 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * AD3552R Digital <-> Analog converters common header
+ *
+ * Copyright 2021-2024 Analog Devices Inc.
+ * Author: Angelo Dureghello <adureghello@baylibre.com>
+ */
+
+#ifndef __DRIVERS_IIO_DAC_AD3552R_H__
+#define __DRIVERS_IIO_DAC_AD3552R_H__
+
+/* Register addresses */
+/* Primary address space */
+#define AD3552R_REG_ADDR_INTERFACE_CONFIG_A		0x00
+#define   AD3552R_MASK_SOFTWARE_RESET			(BIT(7) | BIT(0))
+#define   AD3552R_MASK_ADDR_ASCENSION			BIT(5)
+#define   AD3552R_MASK_SDO_ACTIVE			BIT(4)
+#define AD3552R_REG_ADDR_INTERFACE_CONFIG_B		0x01
+#define   AD3552R_MASK_SINGLE_INST			BIT(7)
+#define   AD3552R_MASK_SHORT_INSTRUCTION		BIT(3)
+#define AD3552R_REG_ADDR_DEVICE_CONFIG			0x02
+#define   AD3552R_MASK_DEVICE_STATUS(n)			BIT(4 + (n))
+#define   AD3552R_MASK_CUSTOM_MODES			GENMASK(3, 2)
+#define   AD3552R_MASK_OPERATING_MODES			GENMASK(1, 0)
+#define AD3552R_REG_ADDR_CHIP_TYPE			0x03
+#define   AD3552R_MASK_CLASS				GENMASK(7, 0)
+#define AD3552R_REG_ADDR_PRODUCT_ID_L			0x04
+#define AD3552R_REG_ADDR_PRODUCT_ID_H			0x05
+#define AD3552R_REG_ADDR_CHIP_GRADE			0x06
+#define   AD3552R_MASK_GRADE				GENMASK(7, 4)
+#define   AD3552R_MASK_DEVICE_REVISION			GENMASK(3, 0)
+#define AD3552R_REG_ADDR_SCRATCH_PAD			0x0A
+#define AD3552R_REG_ADDR_SPI_REVISION			0x0B
+#define AD3552R_REG_ADDR_VENDOR_L			0x0C
+#define AD3552R_REG_ADDR_VENDOR_H			0x0D
+#define AD3552R_REG_ADDR_STREAM_MODE			0x0E
+#define   AD3552R_MASK_LENGTH				GENMASK(7, 0)
+#define AD3552R_REG_ADDR_TRANSFER_REGISTER		0x0F
+#define   AD3552R_MASK_MULTI_IO_MODE			GENMASK(7, 6)
+#define   AD3552R_MASK_STREAM_LENGTH_KEEP_VALUE		BIT(2)
+#define AD3552R_REG_ADDR_INTERFACE_CONFIG_C		0x10
+#define   AD3552R_MASK_CRC_ENABLE			(GENMASK(7, 6) |\
+							 GENMASK(1, 0))
+#define   AD3552R_MASK_STRICT_REGISTER_ACCESS		BIT(5)
+#define AD3552R_REG_ADDR_INTERFACE_STATUS_A		0x11
+#define   AD3552R_MASK_INTERFACE_NOT_READY		BIT(7)
+#define   AD3552R_MASK_CLOCK_COUNTING_ERROR		BIT(5)
+#define   AD3552R_MASK_INVALID_OR_NO_CRC		BIT(3)
+#define   AD3552R_MASK_WRITE_TO_READ_ONLY_REGISTER	BIT(2)
+#define   AD3552R_MASK_PARTIAL_REGISTER_ACCESS		BIT(1)
+#define   AD3552R_MASK_REGISTER_ADDRESS_INVALID		BIT(0)
+#define AD3552R_REG_ADDR_INTERFACE_CONFIG_D		0x14
+#define   AD3552R_MASK_ALERT_ENABLE_PULLUP		BIT(6)
+#define   AD3552R_MASK_MEM_CRC_EN			BIT(4)
+#define   AD3552R_MASK_SDO_DRIVE_STRENGTH		GENMASK(3, 2)
+#define   AD3552R_MASK_DUAL_SPI_SYNCHROUNOUS_EN		BIT(1)
+#define   AD3552R_MASK_SPI_CONFIG_DDR			BIT(0)
+#define AD3552R_REG_ADDR_SH_REFERENCE_CONFIG		0x15
+#define   AD3552R_MASK_IDUMP_FAST_MODE			BIT(6)
+#define   AD3552R_MASK_SAMPLE_HOLD_DIFF_USER_EN		BIT(5)
+#define   AD3552R_MASK_SAMPLE_HOLD_USER_TRIM		GENMASK(4, 3)
+#define   AD3552R_MASK_SAMPLE_HOLD_USER_ENABLE		BIT(2)
+#define   AD3552R_MASK_REFERENCE_VOLTAGE_SEL		GENMASK(1, 0)
+#define AD3552R_REG_ADDR_ERR_ALARM_MASK			0x16
+#define   AD3552R_MASK_REF_RANGE_ALARM			BIT(6)
+#define   AD3552R_MASK_CLOCK_COUNT_ERR_ALARM		BIT(5)
+#define   AD3552R_MASK_MEM_CRC_ERR_ALARM		BIT(4)
+#define   AD3552R_MASK_SPI_CRC_ERR_ALARM		BIT(3)
+#define   AD3552R_MASK_WRITE_TO_READ_ONLY_ALARM		BIT(2)
+#define   AD3552R_MASK_PARTIAL_REGISTER_ACCESS_ALARM	BIT(1)
+#define   AD3552R_MASK_REGISTER_ADDRESS_INVALID_ALARM	BIT(0)
+#define AD3552R_REG_ADDR_ERR_STATUS			0x17
+#define   AD3552R_MASK_REF_RANGE_ERR_STATUS		BIT(6)
+#define   AD3552R_MASK_STREAM_EXCEEDS_DAC_ERR_STATUS	BIT(5)
+#define   AD3552R_MASK_MEM_CRC_ERR_STATUS		BIT(4)
+#define   AD3552R_MASK_RESET_STATUS			BIT(0)
+#define AD3552R_REG_ADDR_POWERDOWN_CONFIG		0x18
+#define   AD3552R_MASK_CH_DAC_POWERDOWN(ch)		BIT(4 + (ch))
+#define   AD3552R_MASK_CH_AMPLIFIER_POWERDOWN(ch)	BIT(ch)
+#define AD3552R_REG_ADDR_CH0_CH1_OUTPUT_RANGE		0x19
+#define   AD3552R_MASK_CH0_RANGE			GENMASK(2, 0)
+#define   AD3552R_MASK_CH1_RANGE			GENMASK(6, 4)
+#define   AD3552R_MASK_CH_OUTPUT_RANGE			GENMASK(7, 0)
+#define   AD3552R_MASK_CH_OUTPUT_RANGE_SEL(ch)		((ch) ? \
+							 GENMASK(7, 4) : \
+							 GENMASK(3, 0))
+#define AD3552R_REG_ADDR_CH_OFFSET(ch)			(0x1B + (ch) * 2)
+#define   AD3552R_MASK_CH_OFFSET_BITS_0_7		GENMASK(7, 0)
+#define AD3552R_REG_ADDR_CH_GAIN(ch)			(0x1C + (ch) * 2)
+#define   AD3552R_MASK_CH_RANGE_OVERRIDE		BIT(7)
+#define   AD3552R_MASK_CH_GAIN_SCALING_N		GENMASK(6, 5)
+#define   AD3552R_MASK_CH_GAIN_SCALING_P		GENMASK(4, 3)
+#define   AD3552R_MASK_CH_OFFSET_POLARITY		BIT(2)
+#define   AD3552R_MASK_CH_OFFSET_BIT_8			BIT(0)
+/*
+ * Secondary region
+ * For multibyte registers specify the highest address because the access is
+ * done in descending order
+ */
+#define AD3552R_SECONDARY_REGION_START			0x28
+#define AD3552R_REG_ADDR_HW_LDAC_16B			0x28
+#define AD3552R_REG_ADDR_CH_DAC_16B(ch)			(0x2C - (1 - (ch)) * 2)
+#define AD3552R_REG_ADDR_DAC_PAGE_MASK_16B		0x2E
+#define AD3552R_REG_ADDR_CH_SELECT_16B			0x2F
+#define AD3552R_REG_ADDR_INPUT_PAGE_MASK_16B		0x31
+#define AD3552R_REG_ADDR_SW_LDAC_16B			0x32
+#define AD3552R_REG_ADDR_CH_INPUT_16B(ch)		(0x36 - (1 - (ch)) * 2)
+/* 3 bytes registers */
+#define AD3552R_REG_START_24B				0x37
+#define AD3552R_REG_ADDR_HW_LDAC_24B			0x37
+#define AD3552R_REG_ADDR_CH_DAC_24B(ch)			(0x3D - (1 - (ch)) * 3)
+#define AD3552R_REG_ADDR_DAC_PAGE_MASK_24B		0x40
+#define AD3552R_REG_ADDR_CH_SELECT_24B			0x41
+#define AD3552R_REG_ADDR_INPUT_PAGE_MASK_24B		0x44
+#define AD3552R_REG_ADDR_SW_LDAC_24B			0x45
+#define AD3552R_REG_ADDR_CH_INPUT_24B(ch)		(0x4B - (1 - (ch)) * 3)
+
+/* Useful defines */
+#define AD3552R_MAX_CH					2
+#define AD3552R_MASK_CH(ch)				BIT(ch)
+#define AD3552R_MASK_ALL_CH				GENMASK(1, 0)
+#define AD3552R_MAX_REG_SIZE				3
+#define AD3552R_READ_BIT				BIT(7)
+#define AD3552R_ADDR_MASK				GENMASK(6, 0)
+#define AD3552R_MASK_DAC_12B				GENMASK(15, 4)
+#define AD3552R_DEFAULT_CONFIG_B_VALUE			0x8
+#define AD3552R_SCRATCH_PAD_TEST_VAL1			0x34
+#define AD3552R_SCRATCH_PAD_TEST_VAL2			0xB2
+#define AD3552R_GAIN_SCALE				1000
+#define AD3552R_LDAC_PULSE_US				100
+
+#define AD3552R_MAX_RANGES	5
+#define AD3542R_MAX_RANGES	6
+
+extern const s32 ad3552r_ch_ranges[AD3552R_MAX_RANGES][2];
+extern const s32 ad3542r_ch_ranges[AD3542R_MAX_RANGES][2];
+
+enum ad3552r_id {
+	AD3541R_ID = 0x400b,
+	AD3542R_ID = 0x4009,
+	AD3551R_ID = 0x400a,
+	AD3552R_ID = 0x4008,
+};
+
+enum ad3552r_ch_vref_select {
+	/* Internal source with Vref I/O floating */
+	AD3552R_INTERNAL_VREF_PIN_FLOATING,
+	/* Internal source with Vref I/O at 2.5V */
+	AD3552R_INTERNAL_VREF_PIN_2P5V,
+	/* External source with Vref I/O as input */
+	AD3552R_EXTERNAL_VREF_PIN_INPUT
+};
+
+enum ad3542r_ch_output_range {
+	/* Range from 0 V to 2.5 V. Requires Rfb1x connection */
+	AD3542R_CH_OUTPUT_RANGE_0__2P5V,
+	/* Range from 0 V to 3 V. Requires Rfb1x connection  */
+	AD3542R_CH_OUTPUT_RANGE_0__3V,
+	/* Range from 0 V to 5 V. Requires Rfb1x connection  */
+	AD3542R_CH_OUTPUT_RANGE_0__5V,
+	/* Range from 0 V to 10 V. Requires Rfb2x connection  */
+	AD3542R_CH_OUTPUT_RANGE_0__10V,
+	/* Range from -2.5 V to 7.5 V. Requires Rfb2x connection  */
+	AD3542R_CH_OUTPUT_RANGE_NEG_2P5__7P5V,
+	/* Range from -5 V to 5 V. Requires Rfb2x connection  */
+	AD3542R_CH_OUTPUT_RANGE_NEG_5__5V,
+};
+
+enum ad3552r_ch_output_range {
+	/* Range from 0 V to 2.5 V. Requires Rfb1x connection */
+	AD3552R_CH_OUTPUT_RANGE_0__2P5V,
+	/* Range from 0 V to 5 V. Requires Rfb1x connection  */
+	AD3552R_CH_OUTPUT_RANGE_0__5V,
+	/* Range from 0 V to 10 V. Requires Rfb2x connection  */
+	AD3552R_CH_OUTPUT_RANGE_0__10V,
+	/* Range from -5 V to 5 V. Requires Rfb2x connection  */
+	AD3552R_CH_OUTPUT_RANGE_NEG_5__5V,
+	/* Range from -10 V to 10 V. Requires Rfb4x connection  */
+	AD3552R_CH_OUTPUT_RANGE_NEG_10__10V,
+};
+
+int ad3552r_get_output_range(struct device *dev, enum ad3552r_id id,
+			     struct fwnode_handle *child, u32 *val);
+int ad3552r_get_custom_gain(struct device *dev, struct fwnode_handle *child,
+			    u8 *gs_p, u8 *gs_n, u16 *rfb, s16 *goffs);
+u16 ad3552r_calc_custom_gain(u8 p, u8 n, s16 goffs);
+int ad3552r_get_ref_voltage(struct device *dev);
+int ad3552r_get_drive_strength(struct device *dev, u32 *val);
+
+#endif /* __DRIVERS_IIO_DAC_AD3552R_H__ */

-- 
2.45.0.rc1

On Thu, 19 Sep 2024 11:20:04 +0200
Angelo Dureghello <adureghello@baylibre.com> wrote:

> From: Angelo Dureghello <adureghello@baylibre.com>
> 
> Extracting common code, to share common code to be used later
> by the AXI driver version (ad3552r-axi.c).
> 
> Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
For these, main request is to move them to a namespace + GPL is
probably the appropriate choice here.


> ---
>  drivers/iio/dac/Makefile         |   2 +-
>  drivers/iio/dac/ad3552r-common.c | 173 +++++++++++++++++++++++
>  drivers/iio/dac/ad3552r.c        | 293 ++++-----------------------------------
>  drivers/iio/dac/ad3552r.h        | 190 +++++++++++++++++++++++++
>  4 files changed, 390 insertions(+), 268 deletions(-)
> 
> diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile
> index 2cf148f16306..56a125f56284 100644
> --- a/drivers/iio/dac/Makefile
> +++ b/drivers/iio/dac/Makefile
> @@ -4,7 +4,7 @@
>  #
>  
>  # When adding new entries keep the list in alphabetical order
> -obj-$(CONFIG_AD3552R) += ad3552r.o
> +obj-$(CONFIG_AD3552R) += ad3552r.o ad3552r-common.o
>  obj-$(CONFIG_AD5360) += ad5360.o
>  obj-$(CONFIG_AD5380) += ad5380.o
>  obj-$(CONFIG_AD5421) += ad5421.o
> diff --git a/drivers/iio/dac/ad3552r-common.c b/drivers/iio/dac/ad3552r-common.c
> new file mode 100644
> index 000000000000..624f3f97cdea
> --- /dev/null
> +++ b/drivers/iio/dac/ad3552r-common.c
> @@ -0,0 +1,173 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +//
> +// Copyright (c) 2010-2024 Analog Devices Inc.
> +// Copyright (c) 2024 Baylibre, SAS
> +
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/property.h>
> +#include <linux/regulator/consumer.h>
> +
> +#include "ad3552r.h"
> +
> +const s32 ad3552r_ch_ranges[AD3552R_MAX_RANGES][2] = {
> +	[AD3552R_CH_OUTPUT_RANGE_0__2P5V]	= { 0, 2500 },
> +	[AD3552R_CH_OUTPUT_RANGE_0__5V]		= { 0, 5000 },
> +	[AD3552R_CH_OUTPUT_RANGE_0__10V]	= { 0, 10000 },
> +	[AD3552R_CH_OUTPUT_RANGE_NEG_5__5V]	= { -5000, 5000 },
> +	[AD3552R_CH_OUTPUT_RANGE_NEG_10__10V]	= { -10000, 10000 }
> +};
> +EXPORT_SYMBOL(ad3552r_ch_ranges);

GPL and namespace them to avoid poluting the general namespace with driver
specific exports.

EXPORT_SYMBOL_NS_GPL() etc.


> +
> +u16 ad3552r_calc_custom_gain(u8 p, u8 n, s16 goffs)
> +{
> +	u16 reg;
> +
> +	reg = FIELD_PREP(AD3552R_MASK_CH_RANGE_OVERRIDE, 1);
> +	reg |= FIELD_PREP(AD3552R_MASK_CH_GAIN_SCALING_P, p);
> +	reg |= FIELD_PREP(AD3552R_MASK_CH_GAIN_SCALING_N, n);
> +	reg |= FIELD_PREP(AD3552R_MASK_CH_OFFSET_BIT_8, abs((s32)goffs) >> 8);
Hmm. Not sure the s32 case does anything useful here.
Also this is a little messy from local view of code. It is not obvious
that only BIT(0) can be set here.  I'd be tempted to mask that
before passing to FIELD_PREP()

> +	reg |= FIELD_PREP(AD3552R_MASK_CH_OFFSET_POLARITY, (s32)goffs < 0);

Why do you need the s32 cast for this last line?

> +
> +	return reg;
> +}
> +
> +int ad3552r_get_ref_voltage(struct device *dev)
> +{
> +	int voltage;
> +	int delta = 100000;
> +
> +	voltage = devm_regulator_get_enable_read_voltage(dev, "vref");
> +	if (voltage < 0 && voltage != -ENODEV)
> +		return dev_err_probe(dev, voltage,
> +				     "Error getting vref voltage\n");
> +
> +	if (voltage == -ENODEV) {
> +		if (device_property_read_bool(dev, "adi,vref-out-en"))
> +			return AD3552R_INTERNAL_VREF_PIN_2P5V;
> +		else
> +			return AD3552R_INTERNAL_VREF_PIN_FLOATING;
> +	}
> +
> +	if (voltage > 2500000 + delta || voltage < 2500000 - delta) {
> +		dev_warn(dev, "vref-supply must be 2.5V");
> +		return -EINVAL;
> +	}

Obviously this is legacy code, but why do we care in the driver?
If someone has circuitry or configuration that is wrong, do we need to check
that?  I guess it does little harm though.

> +
> +	return AD3552R_EXTERNAL_VREF_PIN_INPUT;
> +}
> +
> +int ad3552r_get_drive_strength(struct device *dev, u32 *val)
> +{
> +	int err;
> +
> +	err = device_property_read_u32(dev, "adi,sdo-drive-strength", val);
> +	if (err)
> +		return err;
> +
> +	if (*val > 3) {

Usually we avoid setting values passed back on error if it is easy to do so.
I'd bounce via a local variable and only set *val = drive_strength
after you know it is in range.

> +		dev_err(dev,
> +			"adi,sdo-drive-strength must be less than 4\n");
> +		return -EINVAL;
Is dev_err_probe() appropriate here?  I haven't checked if this is called
from non probe paths so ignore this comment if it is.
> +	}
> +
> +	return 0;
> +}
> +
> +int ad3552r_get_custom_gain(struct device *dev, struct fwnode_handle *child,
> +			    u8 *gs_p, u8 *gs_n, u16 *rfb, s16 *goffs)
> +{
> +	int err;
> +	u32 val;
> +	struct fwnode_handle *gain_child __free(fwnode_handle) =
> +				fwnode_get_named_child_node(child,
One tab more than the line above is fine for cases like this and makes for
more readable code.

> +				"custom-output-range-config");

Align this final parameter with c of child.

> +
> +	if (!gain_child)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "custom-output-range-config mandatory\n");
> +
> +	err = fwnode_property_read_u32(gain_child, "adi,gain-scaling-p", &val);
> +	if (err)
> +		return dev_err_probe(dev, err,
> +				     "adi,gain-scaling-p mandatory\n");
> +	*gs_p = val;
> +
> +	err = fwnode_property_read_u32(gain_child, "adi,gain-scaling-n", &val);
> +	if (err)
> +		return dev_err_probe(dev, err,
> +				     "adi,gain-scaling-n property mandatory\n");
> +	*gs_n = val;
> +
> +	err = fwnode_property_read_u32(gain_child, "adi,rfb-ohms", &val);
> +	if (err)
> +		return dev_err_probe(dev, err,
> +				     "adi,rfb-ohms mandatory\n");
> +	*rfb = val;
> +
> +	err = fwnode_property_read_u32(gain_child, "adi,gain-offset", &val);
> +	if (err)
> +		return dev_err_probe(dev, err,
> +				     "adi,gain-offset mandatory\n");
> +	*goffs = val;
> +
> +	return 0;
> +}
> +
> +static int ad3552r_find_range(u16 id, s32 *vals)
> +{
> +	int i, len;
> +	const s32 (*ranges)[2];
> +
> +	if (id == AD3542R_ID) {

This is already in your model_data. Use that not another lookup via
an ID enum.  The ID enum approach doesn't scale as we add more parts
as it scatters device specific code through the driver.


> +		len = ARRAY_SIZE(ad3542r_ch_ranges);
> +		ranges = ad3542r_ch_ranges;
> +	} else {
> +		len = ARRAY_SIZE(ad3552r_ch_ranges);
> +		ranges = ad3552r_ch_ranges;
> +	}
> +
> +	for (i = 0; i < len; i++)
> +		if (vals[0] == ranges[i][0] * 1000 &&
> +		    vals[1] == ranges[i][1] * 1000)
> +			return i;
> +
> +	return -EINVAL;
> +}
> +
> +int ad3552r_get_output_range(struct device *dev, enum ad3552r_id chip_id,
> +			     struct fwnode_handle *child, u32 *val)
As above, don't pass the enum. Either pass the model_data or pass the
actual stuff you need which is the ranges array and size of that array.

> +{
> +	int ret;
> +	s32 vals[2];
> +
> +	/* This property is optional, so returning -ENOENT if missing */
> +	if (!fwnode_property_present(child, "adi,output-range-microvolt"))
> +		return -ENOENT;
> +
> +	ret = fwnode_property_read_u32_array(child,
> +					     "adi,output-range-microvolt",
> +					     vals, 2);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				"invalid adi,output-range-microvolt\n");
> +
> +	ret = ad3552r_find_range(chip_id, vals);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret,
> +			"invalid adi,output-range-microvolt value\n");
> +
> +	*val = ret;
> +
> +	return 0;
> +}

Thanks,

Jonathan

Hi Jonathan,

On 29.09.2024 12:57, Jonathan Cameron wrote:
> On Thu, 19 Sep 2024 11:20:04 +0200
> Angelo Dureghello <adureghello@baylibre.com> wrote:
> 
> > From: Angelo Dureghello <adureghello@baylibre.com>
> > 
> > Extracting common code, to share common code to be used later
> > by the AXI driver version (ad3552r-axi.c).
> > 
> > Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
> For these, main request is to move them to a namespace + GPL is
> probably the appropriate choice here.
> 
> 
> > ---
> >  drivers/iio/dac/Makefile         |   2 +-
> >  drivers/iio/dac/ad3552r-common.c | 173 +++++++++++++++++++++++
> >  drivers/iio/dac/ad3552r.c        | 293 ++++-----------------------------------
> >  drivers/iio/dac/ad3552r.h        | 190 +++++++++++++++++++++++++
> >  4 files changed, 390 insertions(+), 268 deletions(-)
> > 
> > diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile
> > index 2cf148f16306..56a125f56284 100644
> > --- a/drivers/iio/dac/Makefile
> > +++ b/drivers/iio/dac/Makefile
> > @@ -4,7 +4,7 @@
> >  #
> >  
> >  # When adding new entries keep the list in alphabetical order
> > -obj-$(CONFIG_AD3552R) += ad3552r.o
> > +obj-$(CONFIG_AD3552R) += ad3552r.o ad3552r-common.o
> >  obj-$(CONFIG_AD5360) += ad5360.o
> >  obj-$(CONFIG_AD5380) += ad5380.o
> >  obj-$(CONFIG_AD5421) += ad5421.o
> > diff --git a/drivers/iio/dac/ad3552r-common.c b/drivers/iio/dac/ad3552r-common.c
> > new file mode 100644
> > index 000000000000..624f3f97cdea
> > --- /dev/null
> > +++ b/drivers/iio/dac/ad3552r-common.c
> > @@ -0,0 +1,173 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +//
> > +// Copyright (c) 2010-2024 Analog Devices Inc.
> > +// Copyright (c) 2024 Baylibre, SAS
> > +
> > +#include <linux/device.h>
> > +#include <linux/module.h>
> > +#include <linux/property.h>
> > +#include <linux/regulator/consumer.h>
> > +
> > +#include "ad3552r.h"
> > +
> > +const s32 ad3552r_ch_ranges[AD3552R_MAX_RANGES][2] = {
> > +	[AD3552R_CH_OUTPUT_RANGE_0__2P5V]	= { 0, 2500 },
> > +	[AD3552R_CH_OUTPUT_RANGE_0__5V]		= { 0, 5000 },
> > +	[AD3552R_CH_OUTPUT_RANGE_0__10V]	= { 0, 10000 },
> > +	[AD3552R_CH_OUTPUT_RANGE_NEG_5__5V]	= { -5000, 5000 },
> > +	[AD3552R_CH_OUTPUT_RANGE_NEG_10__10V]	= { -10000, 10000 }
> > +};
> > +EXPORT_SYMBOL(ad3552r_ch_ranges);
> 
> GPL and namespace them to avoid poluting the general namespace with driver
> specific exports.
> 
> EXPORT_SYMBOL_NS_GPL() etc.
> 
> 
> > +
> > +u16 ad3552r_calc_custom_gain(u8 p, u8 n, s16 goffs)
> > +{
> > +	u16 reg;
> > +
> > +	reg = FIELD_PREP(AD3552R_MASK_CH_RANGE_OVERRIDE, 1);
> > +	reg |= FIELD_PREP(AD3552R_MASK_CH_GAIN_SCALING_P, p);
> > +	reg |= FIELD_PREP(AD3552R_MASK_CH_GAIN_SCALING_N, n);
> > +	reg |= FIELD_PREP(AD3552R_MASK_CH_OFFSET_BIT_8, abs((s32)goffs) >> 8);
> Hmm. Not sure the s32 case does anything useful here.
> Also this is a little messy from local view of code. It is not obvious
> that only BIT(0) can be set here.  I'd be tempted to mask that
> before passing to FIELD_PREP()
> 
> > +	reg |= FIELD_PREP(AD3552R_MASK_CH_OFFSET_POLARITY, (s32)goffs < 0);
> 
> Why do you need the s32 cast for this last line?
> 
> > +
> > +	return reg;
> > +}
> > +
> > +int ad3552r_get_ref_voltage(struct device *dev)
> > +{
> > +	int voltage;
> > +	int delta = 100000;
> > +
> > +	voltage = devm_regulator_get_enable_read_voltage(dev, "vref");
> > +	if (voltage < 0 && voltage != -ENODEV)
> > +		return dev_err_probe(dev, voltage,
> > +				     "Error getting vref voltage\n");
> > +
> > +	if (voltage == -ENODEV) {
> > +		if (device_property_read_bool(dev, "adi,vref-out-en"))
> > +			return AD3552R_INTERNAL_VREF_PIN_2P5V;
> > +		else
> > +			return AD3552R_INTERNAL_VREF_PIN_FLOATING;
> > +	}
> > +
> > +	if (voltage > 2500000 + delta || voltage < 2500000 - delta) {
> > +		dev_warn(dev, "vref-supply must be 2.5V");
> > +		return -EINVAL;
> > +	}
> 
> Obviously this is legacy code, but why do we care in the driver?
> If someone has circuitry or configuration that is wrong, do we need to check
> that?  I guess it does little harm though.
> 
> > +
> > +	return AD3552R_EXTERNAL_VREF_PIN_INPUT;
> > +}
> > +
> > +int ad3552r_get_drive_strength(struct device *dev, u32 *val)
> > +{
> > +	int err;
> > +
> > +	err = device_property_read_u32(dev, "adi,sdo-drive-strength", val);
> > +	if (err)
> > +		return err;
> > +
> > +	if (*val > 3) {
>


 
> Usually we avoid setting values passed back on error if it is easy to do so.
> I'd bounce via a local variable and only set *val = drive_strength
> after you know it is in range.
> 
> > +		dev_err(dev,
> > +			"adi,sdo-drive-strength must be less than 4\n");
> > +		return -EINVAL;
> Is dev_err_probe() appropriate here?  I haven't checked if this is called
> from non probe paths so ignore this comment if it is.
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +int ad3552r_get_custom_gain(struct device *dev, struct fwnode_handle *child,
> > +			    u8 *gs_p, u8 *gs_n, u16 *rfb, s16 *goffs)
> > +{
> > +	int err;
> > +	u32 val;
> > +	struct fwnode_handle *gain_child __free(fwnode_handle) =
> > +				fwnode_get_named_child_node(child,
> One tab more than the line above is fine for cases like this and makes for
> more readable code.
>
Aligning with c then line comes to long. 
I can offer, as in other drivers:

int ad3552r_get_custom_gain(struct device *dev, struct fwnode_handle *child,
			    u8 *gs_p, u8 *gs_n, u16 *rfb, s16 *goffs)
{
	int err;
	u32 val;
	struct fwnode_handle *gain_child __free(fwnode_handle) =
		fwnode_get_named_child_node(child,
					    "custom-output-range-config");

Also, do you prefer 80 or 100 as eol limit ?

 
> > +				"custom-output-range-config");
> 
> Align this final parameter with c of child.
> 
> > +
> > +	if (!gain_child)
> > +		return dev_err_probe(dev, -EINVAL,
> > +				     "custom-output-range-config mandatory\n");
> > +
> > +	err = fwnode_property_read_u32(gain_child, "adi,gain-scaling-p", &val);
> > +	if (err)
> > +		return dev_err_probe(dev, err,
> > +				     "adi,gain-scaling-p mandatory\n");
> > +	*gs_p = val;
> > +
> > +	err = fwnode_property_read_u32(gain_child, "adi,gain-scaling-n", &val);
> > +	if (err)
> > +		return dev_err_probe(dev, err,
> > +				     "adi,gain-scaling-n property mandatory\n");
> > +	*gs_n = val;
> > +
> > +	err = fwnode_property_read_u32(gain_child, "adi,rfb-ohms", &val);
> > +	if (err)
> > +		return dev_err_probe(dev, err,
> > +				     "adi,rfb-ohms mandatory\n");
> > +	*rfb = val;
> > +
> > +	err = fwnode_property_read_u32(gain_child, "adi,gain-offset", &val);
> > +	if (err)
> > +		return dev_err_probe(dev, err,
> > +				     "adi,gain-offset mandatory\n");
> > +	*goffs = val;
> > +
> > +	return 0;
> > +}
> > +
> > +static int ad3552r_find_range(u16 id, s32 *vals)
> > +{
> > +	int i, len;
> > +	const s32 (*ranges)[2];
> > +
> > +	if (id == AD3542R_ID) {
> 
> This is already in your model_data. Use that not another lookup via
> an ID enum.  The ID enum approach doesn't scale as we add more parts
> as it scatters device specific code through the driver.
>

This function is only used internally to this common part.
 
> 
> > +		len = ARRAY_SIZE(ad3542r_ch_ranges);
> > +		ranges = ad3542r_ch_ranges;
> > +	} else {
> > +		len = ARRAY_SIZE(ad3552r_ch_ranges);
> > +		ranges = ad3552r_ch_ranges;
> > +	}
> > +
> > +	for (i = 0; i < len; i++)
> > +		if (vals[0] == ranges[i][0] * 1000 &&
> > +		    vals[1] == ranges[i][1] * 1000)
> > +			return i;
> > +
> > +	return -EINVAL;
> > +}
> > +
> > +int ad3552r_get_output_range(struct device *dev, enum ad3552r_id chip_id,
> > +			     struct fwnode_handle *child, u32 *val)
> As above, don't pass the enum. Either pass the model_data or pass the
> actual stuff you need which is the ranges array and size of that array.
>

Cannot pass model data, structures of the 2 drviers are different.
If i pass arrays, the logic of deciding what array (checking the id)
must be done in the drivers, but in this way, there will be less
common code.
 
> > +{
> > +	int ret;
> > +	s32 vals[2];
> > +
> > +	/* This property is optional, so returning -ENOENT if missing */
> > +	if (!fwnode_property_present(child, "adi,output-range-microvolt"))
> > +		return -ENOENT;
> > +
> > +	ret = fwnode_property_read_u32_array(child,
> > +					     "adi,output-range-microvolt",
> > +					     vals, 2);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret,
> > +				"invalid adi,output-range-microvolt\n");
> > +
> > +	ret = ad3552r_find_range(chip_id, vals);
> > +	if (ret < 0)
> > +		return dev_err_probe(dev, ret,
> > +			"invalid adi,output-range-microvolt value\n");
> > +
> > +	*val = ret;
> > +
> > +	return 0;
> > +}
> 
> Thanks,
> 
> Jonathan
> 
> 

Thanks,

-- 
Regards,
  Angelo

> > > +int ad3552r_get_custom_gain(struct device *dev, struct fwnode_handle *child,
> > > +			    u8 *gs_p, u8 *gs_n, u16 *rfb, s16 *goffs)
> > > +{
> > > +	int err;
> > > +	u32 val;
> > > +	struct fwnode_handle *gain_child __free(fwnode_handle) =
> > > +				fwnode_get_named_child_node(child,  
> > One tab more than the line above is fine for cases like this and makes for
> > more readable code.
> >  
> Aligning with c then line comes to long. 
> I can offer, as in other drivers:
> 
> int ad3552r_get_custom_gain(struct device *dev, struct fwnode_handle *child,
> 			    u8 *gs_p, u8 *gs_n, u16 *rfb, s16 *goffs)
> {
> 	int err;
> 	u32 val;
> 	struct fwnode_handle *gain_child __free(fwnode_handle) =
> 		fwnode_get_named_child_node(child,
> 					    "custom-output-range-config");

That looks good to me

> 
> Also, do you prefer 80 or 100 as eol limit ?

Prefer 80, but not at the cost of readability

> 
>  
> > > +				"custom-output-range-config");  
> > 
> > Align this final parameter with c of child.
> >   
>

> > > +static int ad3552r_find_range(u16 id, s32 *vals)
> > > +{
> > > +	int i, len;
> > > +	const s32 (*ranges)[2];
> > > +
> > > +	if (id == AD3542R_ID) {  
> > 
> > This is already in your model_data. Use that not another lookup via
> > an ID enum.  The ID enum approach doesn't scale as we add more parts
> > as it scatters device specific code through the driver.
> >  
> 
> This function is only used internally to this common part.
>  
> >   
> > > +		len = ARRAY_SIZE(ad3542r_ch_ranges);
> > > +		ranges = ad3542r_ch_ranges;
> > > +	} else {
> > > +		len = ARRAY_SIZE(ad3552r_ch_ranges);
> > > +		ranges = ad3552r_ch_ranges;
> > > +	}
> > > +
> > > +	for (i = 0; i < len; i++)
> > > +		if (vals[0] == ranges[i][0] * 1000 &&
> > > +		    vals[1] == ranges[i][1] * 1000)
> > > +			return i;
> > > +
> > > +	return -EINVAL;
> > > +}
> > > +
> > > +int ad3552r_get_output_range(struct device *dev, enum ad3552r_id chip_id,
> > > +			     struct fwnode_handle *child, u32 *val)  
> > As above, don't pass the enum. Either pass the model_data or pass the
> > actual stuff you need which is the ranges array and size of that array.
> >  
> 
> Cannot pass model data, structures of the 2 drviers are different.
> If i pass arrays, the logic of deciding what array (checking the id)
> must be done in the drivers, but in this way, there will be less
> common code.

I'd prefer that to having an ID register look up in here.

Roughly speaking looking up by ID is almost always the wrong
way to go for long term scalability of a driver as more parts
are added.

Jonathan