[PATCH 1/6] iio: ad7949: Support internal Vref

[PATCH 1/6] iio: ad7949: Support internal Vref

[Adam Michaelis]

Adding configurable (via device tree) options to select either an external
reference voltage (default, original implementation) or one of the two
internal reference voltages provided by the AD7949 part family.

Signed-off-by: Adam Michaelis <adam.michaelis@rockwellcollins.com>
---
 drivers/iio/adc/ad7949.c | 84 +++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 66 insertions(+), 18 deletions(-)

diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
index ac0ffff6c5ae..1c49eed298d8 100644
--- a/drivers/iio/adc/ad7949.c
+++ b/drivers/iio/adc/ad7949.c
@@ -11,6 +11,7 @@
 #include <linux/module.h>
 #include <linux/regulator/consumer.h>
 #include <linux/spi/spi.h>
+#include <linux/of.h>
 
 #define AD7949_MASK_CHANNEL_SEL		GENMASK(9, 7)
 #define AD7949_MASK_TOTAL		GENMASK(13, 0)
@@ -24,6 +25,18 @@ enum {
 	ID_AD7689,
 };
 
+enum ad7949_ref_sel {
+	AD7949_REF_2V5 = 0, /* 2.5V internal ref + temp sensor */
+	AD7949_REF_4V0, /* 4.096V internal ref + temp sensor */
+	AD7949_REF_EXT_TEMP, /* External ref + temp sensor, no buffer */
+	AD7949_REF_EXT_TEMP_BUF, /* External ref + temp sensor + buffer */
+	AD7949_REF_RSRV_4,
+	AD7949_REF_RSRV_5,
+	AD7949_REF_EXT, /* External ref, no temp, no buffer */
+	AD7949_REF_EXT_BUF, /* External ref + buffer, no temp */
+	AD7949_REF_MAX,
+};
+
 struct ad7949_adc_spec {
 	u8 num_channels;
 	u8 resolution;
@@ -41,6 +54,7 @@ struct ad7949_adc_spec {
  * @vref: regulator generating Vref
  * @iio_dev: reference to iio structure
  * @spi: reference to spi structure
+ * @ref_sel: selected reference voltage source
  * @resolution: resolution of the chip
  * @cfg: copy of the configuration register
  * @current_channel: current channel in use
@@ -51,6 +65,7 @@ struct ad7949_adc_chip {
 	struct regulator *vref;
 	struct iio_dev *indio_dev;
 	struct spi_device *spi;
+	enum ad7949_ref_sel ref_sel;
 	u8 resolution;
 	u16 cfg;
 	unsigned int current_channel;
@@ -187,11 +202,20 @@ static int ad7949_spi_read_raw(struct iio_dev *indio_dev,
 		return IIO_VAL_INT;
 
 	case IIO_CHAN_INFO_SCALE:
-		ret = regulator_get_voltage(ad7949_adc->vref);
-		if (ret < 0)
-			return ret;
+		if (ad7949_adc->vref) {
+			ret = regulator_get_voltage(ad7949_adc->vref);
+			if (ret < 0)
+				return ret;
+
+			*val = ret / 5000;
+		} else if (ad7949_adc->ref_sel == AD7949_REF_2V5) {
+			*val = 2500;
+		} else if (ad7949_adc->ref_sel == AD7949_REF_4V0) {
+			*val = 4096;
+		} else {
+			return -EINVAL;
+		}
 
-		*val = ret / 5000;
 		return IIO_VAL_INT;
 	}
 
@@ -223,10 +247,18 @@ static int ad7949_spi_init(struct ad7949_adc_chip *ad7949_adc)
 {
 	int ret;
 	int val;
+	u16 adc_config = 0;
 
-	/* Sequencer disabled, CFG readback disabled, IN0 as default channel */
 	ad7949_adc->current_channel = 0;
-	ret = ad7949_spi_write_cfg(ad7949_adc, 0x3C79, AD7949_MASK_TOTAL);
+	ad7949_adc->cfg = 0;
+
+	adc_config = (0x3c << 8);
+	adc_config |= ((ad7949_adc->current_channel & 0x07) << 7);
+	adc_config |= (1 << 6);
+	adc_config |= (ad7949_adc->ref_sel << 3);
+	adc_config |= 1;
+
+	ret = ad7949_spi_write_cfg(ad7949_adc, adc_config, AD7949_MASK_TOTAL);
 
 	/*
 	 * Do two dummy conversions to apply the first configuration setting.
@@ -245,6 +277,7 @@ static int ad7949_spi_probe(struct spi_device *spi)
 	struct ad7949_adc_chip *ad7949_adc;
 	struct iio_dev *indio_dev;
 	int ret;
+	u32 temp;
 
 	indio_dev = devm_iio_device_alloc(dev, sizeof(*ad7949_adc));
 	if (!indio_dev) {
@@ -263,21 +296,34 @@ static int ad7949_spi_probe(struct spi_device *spi)
 	ad7949_adc = iio_priv(indio_dev);
 	ad7949_adc->indio_dev = indio_dev;
 	ad7949_adc->spi = spi;
+	ad7949_adc->vref = NULL;
 
 	spec = &ad7949_adc_spec[spi_get_device_id(spi)->driver_data];
 	indio_dev->num_channels = spec->num_channels;
 	ad7949_adc->resolution = spec->resolution;
 
-	ad7949_adc->vref = devm_regulator_get(dev, "vref");
-	if (IS_ERR(ad7949_adc->vref)) {
-		dev_err(dev, "fail to request regulator\n");
-		return PTR_ERR(ad7949_adc->vref);
-	}
-
-	ret = regulator_enable(ad7949_adc->vref);
-	if (ret < 0) {
-		dev_err(dev, "fail to enable regulator\n");
-		return ret;
+	ret = of_property_read_u32(ad7949_adc->indio_dev->dev.of_node,
+			"adi,reference-select",
+			&temp);
+	if ((ret == 0) && (temp < AD7949_REF_MAX))
+		ad7949_adc->ref_sel = (enum ad7949_ref_sel)temp;
+	else
+		ad7949_adc->ref_sel = AD7949_REF_EXT_BUF;
+
+	/* Check whether using external Vref */
+	if ((ad7949_adc->ref_sel != AD7949_REF_2V5) &&
+			(ad7949_adc->ref_sel != AD7949_REF_4V0)) {
+		ad7949_adc->vref = devm_regulator_get(dev, "vref");
+		if (IS_ERR(ad7949_adc->vref)) {
+			dev_err(dev, "fail to request regulator\n");
+			return PTR_ERR(ad7949_adc->vref);
+		}
+
+		ret = regulator_enable(ad7949_adc->vref);
+		if (ret < 0) {
+			dev_err(dev, "fail to enable regulator\n");
+			return ret;
+		}
 	}
 
 	mutex_init(&ad7949_adc->lock);
@@ -298,7 +344,8 @@ static int ad7949_spi_probe(struct spi_device *spi)
 
 err:
 	mutex_destroy(&ad7949_adc->lock);
-	regulator_disable(ad7949_adc->vref);
+	if (ad7949_adc->vref)
+		regulator_disable(ad7949_adc->vref);
 
 	return ret;
 }
@@ -310,7 +357,8 @@ static int ad7949_spi_remove(struct spi_device *spi)
 
 	iio_device_unregister(indio_dev);
 	mutex_destroy(&ad7949_adc->lock);
-	regulator_disable(ad7949_adc->vref);
+	if (ad7949_adc->vref)
+		regulator_disable(ad7949_adc->vref);
 
 	return 0;
 }
-- 
1.9.1

[PATCH 2/6] dt-bindings: iio: ad7949: Add adi,reference-select

[Adam Michaelis]

Adding optional parameter to AD7949 to specify the source for the
reference voltage signal. Default value is maintaned with option '6' to
match previous version of driver.

Signed-off-by: Adam Michaelis <adam.michaelis@rockwellcollins.com>
---
 .../devicetree/bindings/iio/adc/ad7949.txt         | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/iio/adc/ad7949.txt b/Documentation/devicetree/bindings/iio/adc/ad7949.txt
index c7f5057356b1..14ee9a2cb2a5 100644
--- a/Documentation/devicetree/bindings/iio/adc/ad7949.txt
+++ b/Documentation/devicetree/bindings/iio/adc/ad7949.txt
@@ -6,11 +6,29 @@ Required properties:
 	* "adi,ad7682"
 	* "adi,ad7689"
  - reg: spi chip select number for the device
- - vref-supply: The regulator supply for ADC reference voltage
 
-Example:
+Optional properties:
+ - adi,reference-select: Select the reference voltage source to use
+ when converting the input voltages. Valid values are:
+   0: Internal 2.5V reference; temperature sensor enabled
+   1: Internal 4.096V reference; temperature sensor enabled
+   2: External reference, temperature sensor enabled, no buffer
+   3: External reference, temperature sensor enabled, buffer enabled
+   6: External reference, temperature sensor disabled, no buffer
+   7: External reference, temperature sensor disabled, buffer enabled
+ - vref-supply: The regulator supply for ADC reference voltage. Required
+ if external reference selected by 'adi,reference-select'.
+
+Examples:
 adc@0 {
 	compatible = "adi,ad7949";
 	reg = <0>;
+	adi,reference-select = <2>;
 	vref-supply = <&vdd_supply>;
 };
+
+adc@0 {
+	compatible = "adi,ad7949";
+	reg = <0>;
+	adi,reference-select = <0>;
+};
-- 
1.9.1

[PATCH 3/6] iio: ad7949: Support configuration read-back

[Adam Michaelis]

Adds device tree parameter to set the configuration read-back bit
in the configuration register to tell the AD7949 to include the value of
the configuration register at the time the current sample was acquired
when reading from the part.

Further work must be done to make read-back information available to
consumer.

Signed-off-by: Adam Michaelis <adam.michaelis@rockwellcollins.com>
---
 drivers/iio/adc/ad7949.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
index 1c49eed298d8..1d75fff698d1 100644
--- a/drivers/iio/adc/ad7949.c
+++ b/drivers/iio/adc/ad7949.c
@@ -55,6 +55,7 @@ struct ad7949_adc_spec {
  * @iio_dev: reference to iio structure
  * @spi: reference to spi structure
  * @ref_sel: selected reference voltage source
+ * @cfg_readback: whether reads will include configuration data
  * @resolution: resolution of the chip
  * @cfg: copy of the configuration register
  * @current_channel: current channel in use
@@ -66,6 +67,7 @@ struct ad7949_adc_chip {
 	struct iio_dev *indio_dev;
 	struct spi_device *spi;
 	enum ad7949_ref_sel ref_sel;
+	bool cfg_readback;
 	u8 resolution;
 	u16 cfg;
 	unsigned int current_channel;
@@ -256,7 +258,7 @@ static int ad7949_spi_init(struct ad7949_adc_chip *ad7949_adc)
 	adc_config |= ((ad7949_adc->current_channel & 0x07) << 7);
 	adc_config |= (1 << 6);
 	adc_config |= (ad7949_adc->ref_sel << 3);
-	adc_config |= 1;
+	adc_config |= (ad7949_adc->cfg_readback ? 0 : 1);
 
 	ret = ad7949_spi_write_cfg(ad7949_adc, adc_config, AD7949_MASK_TOTAL);
 
@@ -302,6 +304,10 @@ static int ad7949_spi_probe(struct spi_device *spi)
 	indio_dev->num_channels = spec->num_channels;
 	ad7949_adc->resolution = spec->resolution;
 
+	ad7949_adc->cfg_readback = of_property_read_bool(
+			ad7949_adc->indio_dev->dev.of_node,
+			"adi,cfg-readback");
+
 	ret = of_property_read_u32(ad7949_adc->indio_dev->dev.of_node,
 			"adi,reference-select",
 			&temp);
-- 
1.9.1

[PATCH 4/6] dt-bindings: iio: ad7949: Add cfg-readback option

[Adam Michaelis]

Adding optional parameter to the ad7949 to indicate that the part should
be configured to include the configuration register value used when
acquiring the current sample value.

Signed-off-by: Adam Michaelis <adam.michaelis@rockwellcollins.com>
---
 Documentation/devicetree/bindings/iio/adc/ad7949.txt | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/adc/ad7949.txt b/Documentation/devicetree/bindings/iio/adc/ad7949.txt
index 14ee9a2cb2a5..cd3572828243 100644
--- a/Documentation/devicetree/bindings/iio/adc/ad7949.txt
+++ b/Documentation/devicetree/bindings/iio/adc/ad7949.txt
@@ -18,6 +18,8 @@ Optional properties:
    7: External reference, temperature sensor disabled, buffer enabled
  - vref-supply: The regulator supply for ADC reference voltage. Required
  if external reference selected by 'adi,reference-select'.
+ - adi,cfg-readback: If defined, set bit in configuration register to
+ read-back the configuration used for the current sample value.
 
 Examples:
 adc@0 {
@@ -32,3 +34,10 @@ adc@0 {
 	reg = <0>;
 	adi,reference-select = <0>;
 };
+
+adc@0 {
+	compatible = "adi,ad7949";
+	reg = <0>;
+	adi,reference-select = <1>;
+	adi,cfg-readback;
+};
-- 
1.9.1

[PATCH 5/6] iio: ad7949: Fix SPI interfacing for 14-bit messages

[Adam Michaelis]

The AD7949 (but not the other two models supported by this driver) uses
samples 14 bits wide. When attempting to communicate through certain SPI
controllers that require power-of-2 word widths, this fails. Adding
logic to pack the 14-bit messages into the most-significant bits of a
16-bit message for communicating over byte-based SPI bus.

Only able to test with AD7949 part, but should support the 16-bit
samples of the AD7682 and AD7689, as well.

Signed-off-by: Adam Michaelis <adam.michaelis@rockwellcollins.com>
---
 drivers/iio/adc/ad7949.c | 97 ++++++++++++++++++++++++------------------------
 1 file changed, 48 insertions(+), 49 deletions(-)

diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
index 1d75fff698d1..dc1ae4c143b0 100644
--- a/drivers/iio/adc/ad7949.c
+++ b/drivers/iio/adc/ad7949.c
@@ -18,6 +18,7 @@
 #define AD7949_OFFSET_CHANNEL_SEL	7
 #define AD7949_CFG_READ_BACK		0x1
 #define AD7949_CFG_REG_SIZE_BITS	14
+#define AD7949_BUFFER_LEN 4
 
 enum {
 	ID_AD7949 = 0,
@@ -71,7 +72,7 @@ struct ad7949_adc_chip {
 	u8 resolution;
 	u16 cfg;
 	unsigned int current_channel;
-	u32 buffer ____cacheline_aligned;
+	u8 buffer[AD7949_BUFFER_LEN] ____cacheline_aligned;
 };
 
 static bool ad7949_spi_cfg_is_read_back(struct ad7949_adc_chip *ad7949_adc)
@@ -82,41 +83,40 @@ static bool ad7949_spi_cfg_is_read_back(struct ad7949_adc_chip *ad7949_adc)
 	return false;
 }
 
-static int ad7949_spi_bits_per_word(struct ad7949_adc_chip *ad7949_adc)
-{
-	int ret = ad7949_adc->resolution;
-
-	if (ad7949_spi_cfg_is_read_back(ad7949_adc))
-		ret += AD7949_CFG_REG_SIZE_BITS;
-
-	return ret;
-}
-
 static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val,
 				u16 mask)
 {
-	int ret;
-	int bits_per_word = ad7949_spi_bits_per_word(ad7949_adc);
-	int shift = bits_per_word - AD7949_CFG_REG_SIZE_BITS;
+	int ret = 0;
 	struct spi_message msg;
-	struct spi_transfer tx[] = {
-		{
-			.tx_buf = &ad7949_adc->buffer,
-			.len = 4,
-			.bits_per_word = bits_per_word,
-		},
-	};
-
-	ad7949_adc->cfg = (val & mask) | (ad7949_adc->cfg & ~mask);
-	ad7949_adc->buffer = ad7949_adc->cfg << shift;
-	spi_message_init_with_transfers(&msg, tx, 1);
-	ret = spi_sync(ad7949_adc->spi, &msg);
+	struct spi_transfer tx;
+	u16 tmp_cfg = 0;
+
+	(void)memset(&tx, 0, sizeof(tx));
+	(void)memset(ad7949_adc->buffer, 0, AD7949_BUFFER_LEN);
+
+	tmp_cfg = (val & mask) | (ad7949_adc->cfg & ~mask);
+
+	if (tmp_cfg != ad7949_adc->cfg) {
+		ad7949_adc->cfg = tmp_cfg;
+
+		/* Pack 14-bit value into 2 bytes, MSB first */
+		ad7949_adc->buffer[0] = ((ad7949_adc->cfg & 0x7f00) >> 6) |
+			((ad7949_adc->cfg & 0xc0) >> 6);
+		ad7949_adc->buffer[1] = (ad7949_adc->cfg & 0x007f) << 2;
+
+		tx.tx_buf = ad7949_adc->buffer;
+		tx.len = ad7949_spi_cfg_is_read_back(ad7949_adc) ? 4 : 2;
+
+		spi_message_init_with_transfers(&msg, &tx, 1);
+		ret = spi_sync(ad7949_adc->spi, &msg);
+
+		/*
+		 * This delay is to avoid a new request before the required
+		 * time to send a new command to the device
+		 */
+		udelay(2);
+	}
 
-	/*
-	 * This delay is to avoid a new request before the required time to
-	 * send a new command to the device
-	 */
-	udelay(2);
 	return ret;
 }
 
@@ -124,16 +124,10 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
 				   unsigned int channel)
 {
 	int ret;
-	int bits_per_word = ad7949_spi_bits_per_word(ad7949_adc);
-	int mask = GENMASK(ad7949_adc->resolution, 0);
 	struct spi_message msg;
-	struct spi_transfer tx[] = {
-		{
-			.rx_buf = &ad7949_adc->buffer,
-			.len = 4,
-			.bits_per_word = bits_per_word,
-		},
-	};
+	struct spi_transfer tx;
+
+	ad7949_adc->current_channel = channel;
 
 	ret = ad7949_spi_write_cfg(ad7949_adc,
 				   channel << AD7949_OFFSET_CHANNEL_SEL,
@@ -141,24 +135,29 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
 	if (ret)
 		return ret;
 
-	ad7949_adc->buffer = 0;
-	spi_message_init_with_transfers(&msg, tx, 1);
+	(void)memset(&tx, 0, sizeof(tx));
+	(void)memset(ad7949_adc->buffer, 0, AD7949_BUFFER_LEN);
+
+	tx.rx_buf = ad7949_adc->buffer;
+	tx.len = ad7949_spi_cfg_is_read_back(ad7949_adc) ? 4 : 2;
+
+	spi_message_init_with_transfers(&msg, &tx, 1);
 	ret = spi_sync(ad7949_adc->spi, &msg);
 	if (ret)
 		return ret;
 
 	/*
-	 * This delay is to avoid a new request before the required time to
-	 * send a new command to the device
+	 * This delay is to avoid a new request before the required time
+	 * to send a new command to the device.
 	 */
 	udelay(2);
 
-	ad7949_adc->current_channel = channel;
+	*val = (ad7949_adc->buffer[0] << 8) | ad7949_adc->buffer[1];
 
-	if (ad7949_spi_cfg_is_read_back(ad7949_adc))
-		*val = (ad7949_adc->buffer >> AD7949_CFG_REG_SIZE_BITS) & mask;
-	else
-		*val = ad7949_adc->buffer & mask;
+	if (ad7949_adc->resolution == 14) {
+		/* 14-bit value in 16-bit buffer */
+		*val = *val >> 2;
+	}
 
 	return 0;
 }
-- 
1.9.1

[PATCH 6/6] iio: ad7949: Fix dummy read cycle placement

[Adam Michaelis]

The AD7949 requires two conversion cycles following any configuration
change (including changing the analog channel being sampled). Therefore,
adding a dummy read cycle when config is changed and removing the extra
cycles at initial configuration.

Signed-off-by: Adam Michaelis <adam.michaelis@rockwellcollins.com>
---
 drivers/iio/adc/ad7949.c | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
index dc1ae4c143b0..4e8ad4217e5b 100644
--- a/drivers/iio/adc/ad7949.c
+++ b/drivers/iio/adc/ad7949.c
@@ -115,6 +115,25 @@ static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val,
 		 * time to send a new command to the device
 		 */
 		udelay(2);
+
+		/*
+		 * Perform extra read cycle to allow configuration, acquisition,
+		 * and conversion sequences to complete for new configuration.
+		 */
+		(void)memset(&tx, 0, sizeof(tx));
+		(void)memset(ad7949_adc->buffer, 0, AD7949_BUFFER_LEN);
+
+		tx.rx_buf = ad7949_adc->buffer;
+		tx.len = ad7949_spi_cfg_is_read_back(ad7949_adc) ? 4 : 2;
+
+		spi_message_init_with_transfers(&msg, &tx, 1);
+		ret = spi_sync(ad7949_adc->spi, &msg);
+
+		/*
+		 * This delay is to avoid a new request before the required time
+		 * to send a new command to the device.
+		 */
+		udelay(2);
 	}
 
 	return ret;
@@ -247,7 +266,6 @@ static int ad7949_spi_reg_access(struct iio_dev *indio_dev,
 static int ad7949_spi_init(struct ad7949_adc_chip *ad7949_adc)
 {
 	int ret;
-	int val;
 	u16 adc_config = 0;
 
 	ad7949_adc->current_channel = 0;
@@ -261,13 +279,6 @@ static int ad7949_spi_init(struct ad7949_adc_chip *ad7949_adc)
 
 	ret = ad7949_spi_write_cfg(ad7949_adc, adc_config, AD7949_MASK_TOTAL);
 
-	/*
-	 * Do two dummy conversions to apply the first configuration setting.
-	 * Required only after the start up of the device.
-	 */
-	ad7949_spi_read_channel(ad7949_adc, &val, ad7949_adc->current_channel);
-	ad7949_spi_read_channel(ad7949_adc, &val, ad7949_adc->current_channel);
-
 	return ret;
 }
 
-- 
1.9.1

Re: [PATCH 1/6] iio: ad7949: Support internal Vref

[Couret Charles-Antoine]

Hi,
I read your patch series and it seems really good. Thank you for the 
improvements. :)
But I have one comment about it there:

Le 01/05/2019 à 23:16, Adam Michaelis a écrit :
> @@ -223,10 +247,18 @@ static int ad7949_spi_init(struct ad7949_adc_chip *ad7949_adc)
>   {
>   	int ret;
>   	int val;
> +	u16 adc_config = 0;
>   
> -	/* Sequencer disabled, CFG readback disabled, IN0 as default channel */
>   	ad7949_adc->current_channel = 0;
> -	ret = ad7949_spi_write_cfg(ad7949_adc, 0x3C79, AD7949_MASK_TOTAL);
> +	ad7949_adc->cfg = 0;
> +
> +	adc_config = (0x3c << 8);
> +	adc_config |= ((ad7949_adc->current_channel & 0x07) << 7);
> +	adc_config |= (1 << 6);
> +	adc_config |= (ad7949_adc->ref_sel << 3);
> +	adc_config |= 1;
> +
> +	ret = ad7949_spi_write_cfg(ad7949_adc, adc_config, AD7949_MASK_TOTAL);

That should be great to replace some _magic constants_ by a more 
explicit name, shouldn't it? Especially (0x3c << 8) and (1 << 6 parts).

Regards,

Charles-Antoine Couret

Re: [PATCH 2/6] dt-bindings: iio: ad7949: Add adi,reference-select

[Jonathan Cameron]

On Wed,  1 May 2019 16:16:59 -0500
Adam Michaelis <adam.michaelis@rockwellcollins.com> wrote:

> Adding optional parameter to AD7949 to specify the source for the
> reference voltage signal. Default value is maintaned with option '6' to
> match previous version of driver.
> 
> Signed-off-by: Adam Michaelis <adam.michaelis@rockwellcollins.com>
> ---
>  .../devicetree/bindings/iio/adc/ad7949.txt         | 22 ++++++++++++++++++++--
>  1 file changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/ad7949.txt b/Documentation/devicetree/bindings/iio/adc/ad7949.txt
> index c7f5057356b1..14ee9a2cb2a5 100644
> --- a/Documentation/devicetree/bindings/iio/adc/ad7949.txt
> +++ b/Documentation/devicetree/bindings/iio/adc/ad7949.txt
> @@ -6,11 +6,29 @@ Required properties:
>  	* "adi,ad7682"
>  	* "adi,ad7689"
>   - reg: spi chip select number for the device
> - - vref-supply: The regulator supply for ADC reference voltage
>  
> -Example:
> +Optional properties:
> + - adi,reference-select: Select the reference voltage source to use
> + when converting the input voltages. Valid values are:
So my immediate thought here is we are mapping one binding to several
different things. Some of which are definitely better described in other
ways.

So let us break it down:

Internal vs external.
- External should require a regulator.  If the regulator is there, normal
assumption would be you want to use it.

Which internal reference?  Hmm. This would be incompatible with the external
regulator and I'd expect the presence of such a regulator to override this.
That does need a new binding.
adi,internal-reference-milivolts = 2500 or 4096.   Much nicer to have
real numbers for someone wondering how it is configured than an enum.

Temperature sensor enabled: Why is this a devicetree question rather than
a runtime decision?

Buffer enabled: This needs a custom binding
adi,external-reference-buffer-enable or something like that?

Makes for a more consistent binding where some elements can be common
across similar devices.  It would be good to see if similar bindings
already exist.  Potentially tings like the reference-buffer enable
may be worth making standard ADC properties rather than device
specific.

Thanks,

Jonathan

	
> +   0: Internal 2.5V reference; temperature sensor enabled
> +   1: Internal 4.096V reference; temperature sensor enabled
> +   2: External reference, temperature sensor enabled, no buffer
> +   3: External reference, temperature sensor enabled, buffer enabled
> +   6: External reference, temperature sensor disabled, no buffer
> +   7: External reference, temperature sensor disabled, buffer enabled
> + - vref-supply: The regulator supply for ADC reference voltage. Required
> + if external reference selected by 'adi,reference-select'.
> +
> +Examples:
>  adc@0 {
>  	compatible = "adi,ad7949";
>  	reg = <0>;
> +	adi,reference-select = <2>;
>  	vref-supply = <&vdd_supply>;
>  };
> +
> +adc@0 {
> +	compatible = "adi,ad7949";
> +	reg = <0>;
> +	adi,reference-select = <0>;
> +};

Re: [PATCH 2/6] dt-bindings: iio: ad7949: Add adi,reference-select

[Adam Michaelis]

The property name "adi,reference-select" was copied from the
adi,ad7124 bindings as a similar hardware register configuration value
field. If the property was separated into three independent fields,
there would be a lot of explanation and checking required since many
of the combinations are invalid (for example, temperature sensor and
buffer are always enabled if internal reference is used). I could
possibly see removing the temperature sensor configuration from the
device tree, but, the current driver (even after these patches) does
not provide any support to read the temperature sensor's value. I
include that information in the configuration options as a summary of
the datasheet.

On Sun, May 5, 2019 at 7:22 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Wed,  1 May 2019 16:16:59 -0500
> Adam Michaelis <adam.michaelis@rockwellcollins.com> wrote:
>
> > Adding optional parameter to AD7949 to specify the source for the
> > reference voltage signal. Default value is maintaned with option '6' to
> > match previous version of driver.
> >
> > Signed-off-by: Adam Michaelis <adam.michaelis@rockwellcollins.com>
> > ---
> >  .../devicetree/bindings/iio/adc/ad7949.txt         | 22 ++++++++++++++++++++--
> >  1 file changed, 20 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/iio/adc/ad7949.txt b/Documentation/devicetree/bindings/iio/adc/ad7949.txt
> > index c7f5057356b1..14ee9a2cb2a5 100644
> > --- a/Documentation/devicetree/bindings/iio/adc/ad7949.txt
> > +++ b/Documentation/devicetree/bindings/iio/adc/ad7949.txt
> > @@ -6,11 +6,29 @@ Required properties:
> >       * "adi,ad7682"
> >       * "adi,ad7689"
> >   - reg: spi chip select number for the device
> > - - vref-supply: The regulator supply for ADC reference voltage
> >
> > -Example:
> > +Optional properties:
> > + - adi,reference-select: Select the reference voltage source to use
> > + when converting the input voltages. Valid values are:
> So my immediate thought here is we are mapping one binding to several
> different things. Some of which are definitely better described in other
> ways.
>
> So let us break it down:
>
> Internal vs external.
> - External should require a regulator.  If the regulator is there, normal
> assumption would be you want to use it.
>
> Which internal reference?  Hmm. This would be incompatible with the external
> regulator and I'd expect the presence of such a regulator to override this.
> That does need a new binding.
> adi,internal-reference-milivolts = 2500 or 4096.   Much nicer to have
> real numbers for someone wondering how it is configured than an enum.
>
> Temperature sensor enabled: Why is this a devicetree question rather than
> a runtime decision?
>
> Buffer enabled: This needs a custom binding
> adi,external-reference-buffer-enable or something like that?
>
> Makes for a more consistent binding where some elements can be common
> across similar devices.  It would be good to see if similar bindings
> already exist.  Potentially tings like the reference-buffer enable
> may be worth making standard ADC properties rather than device
> specific.
>
> Thanks,
>
> Jonathan
>
>
> > +   0: Internal 2.5V reference; temperature sensor enabled
> > +   1: Internal 4.096V reference; temperature sensor enabled
> > +   2: External reference, temperature sensor enabled, no buffer
> > +   3: External reference, temperature sensor enabled, buffer enabled
> > +   6: External reference, temperature sensor disabled, no buffer
> > +   7: External reference, temperature sensor disabled, buffer enabled
> > + - vref-supply: The regulator supply for ADC reference voltage. Required
> > + if external reference selected by 'adi,reference-select'.
> > +
> > +Examples:
> >  adc@0 {
> >       compatible = "adi,ad7949";
> >       reg = <0>;
> > +     adi,reference-select = <2>;
> >       vref-supply = <&vdd_supply>;
> >  };
> > +
> > +adc@0 {
> > +     compatible = "adi,ad7949";
> > +     reg = <0>;
> > +     adi,reference-select = <0>;
> > +};
>


-- 

Adam Michaelis | Sr Software Engineer | Comm Engineering | Mission Systems

COLLINS AEROSPACE

400 Collins Road, Cedar Rapids, IA 52498 U.S.A.

Tel: +1 319 295 4102

adam.michaelis@collins.com | collinsaerospace.com



CONFIDENTIALITY WARNING: This message may contain proprietary and/or
privileged information of Collins Aerospace and its affiliated
companies. If you are not the intended recipient, please 1) Do not
disclose, copy, distribute or use this message or its contents. 2)
Advise the sender by return email. 3) Delete all copies (including all
attachments) from your computer. Your cooperation is greatly
appreciated.

Re: [PATCH 2/6] dt-bindings: iio: ad7949: Add adi,reference-select

[Adam Michaelis]

On Sun, May 5, 2019 at 7:22 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Wed,  1 May 2019 16:16:59 -0500
> Adam Michaelis <adam.michaelis@rockwellcollins.com> wrote:
>
> > Adding optional parameter to AD7949 to specify the source for the
> > reference voltage signal. Default value is maintaned with option '6' to
> > match previous version of driver.
> >
> > Signed-off-by: Adam Michaelis <adam.michaelis@rockwellcollins.com>
> > ---
> >  .../devicetree/bindings/iio/adc/ad7949.txt         | 22 ++++++++++++++++++++--
> >  1 file changed, 20 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/iio/adc/ad7949.txt b/Documentation/devicetree/bindings/iio/adc/ad7949.txt
> > index c7f5057356b1..14ee9a2cb2a5 100644
> > --- a/Documentation/devicetree/bindings/iio/adc/ad7949.txt
> > +++ b/Documentation/devicetree/bindings/iio/adc/ad7949.txt
> > @@ -6,11 +6,29 @@ Required properties:
> >       * "adi,ad7682"
> >       * "adi,ad7689"
> >   - reg: spi chip select number for the device
> > - - vref-supply: The regulator supply for ADC reference voltage
> >
> > -Example:
> > +Optional properties:
> > + - adi,reference-select: Select the reference voltage source to use
> > + when converting the input voltages. Valid values are:
> So my immediate thought here is we are mapping one binding to several
> different things. Some of which are definitely better described in other
> ways.
>
> So let us break it down:
>
> Internal vs external.
> - External should require a regulator.  If the regulator is there, normal
> assumption would be you want to use it.
>
> Which internal reference?  Hmm. This would be incompatible with the external
> regulator and I'd expect the presence of such a regulator to override this.
> That does need a new binding.
> adi,internal-reference-milivolts = 2500 or 4096.   Much nicer to have
> real numbers for someone wondering how it is configured than an enum.
>
> Temperature sensor enabled: Why is this a devicetree question rather than
> a runtime decision?
>
> Buffer enabled: This needs a custom binding
> adi,external-reference-buffer-enable or something like that?
>
> Makes for a more consistent binding where some elements can be common
> across similar devices.  It would be good to see if similar bindings
> already exist.  Potentially tings like the reference-buffer enable
> may be worth making standard ADC properties rather than device
> specific.
>
> Thanks,
>
> Jonathan
>
The property name "adi,reference-select" was copied from the
adi,ad7124 bindings as a similar hardware register configuration value
field. If the property was separated into three independent fields,
there would be a lot of explanation and checking required since many
of the combinations are invalid (for example, temperature sensor and
buffer are always enabled if internal reference is used). I could
possibly see removing the temperature sensor configuration from the
device tree, but, the current driver (even after these patches) does
not provide any support to read the temperature sensor's value. I
include that information in the configuration options as a summary of
the datasheet.

Adam
>
> > +   0: Internal 2.5V reference; temperature sensor enabled
> > +   1: Internal 4.096V reference; temperature sensor enabled
> > +   2: External reference, temperature sensor enabled, no buffer
> > +   3: External reference, temperature sensor enabled, buffer enabled
> > +   6: External reference, temperature sensor disabled, no buffer
> > +   7: External reference, temperature sensor disabled, buffer enabled
> > + - vref-supply: The regulator supply for ADC reference voltage. Required
> > + if external reference selected by 'adi,reference-select'.
> > +
> > +Examples:
> >  adc@0 {
> >       compatible = "adi,ad7949";
> >       reg = <0>;
> > +     adi,reference-select = <2>;
> >       vref-supply = <&vdd_supply>;
> >  };
> > +
> > +adc@0 {
> > +     compatible = "adi,ad7949";
> > +     reg = <0>;
> > +     adi,reference-select = <0>;
> > +};
>

Re: [PATCH 2/6] dt-bindings: iio: ad7949: Add adi,reference-select

[Jonathan Cameron]

On Tue, 7 May 2019 13:21:03 -0500
Adam Michaelis <adam.michaelis@collins.com> wrote:

> On Sun, May 5, 2019 at 7:22 AM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Wed,  1 May 2019 16:16:59 -0500
> > Adam Michaelis <adam.michaelis@rockwellcollins.com> wrote:
> >  
> > > Adding optional parameter to AD7949 to specify the source for the
> > > reference voltage signal. Default value is maintaned with option '6' to
> > > match previous version of driver.
> > >
> > > Signed-off-by: Adam Michaelis <adam.michaelis@rockwellcollins.com>
> > > ---
> > >  .../devicetree/bindings/iio/adc/ad7949.txt         | 22 ++++++++++++++++++++--
> > >  1 file changed, 20 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/Documentation/devicetree/bindings/iio/adc/ad7949.txt b/Documentation/devicetree/bindings/iio/adc/ad7949.txt
> > > index c7f5057356b1..14ee9a2cb2a5 100644
> > > --- a/Documentation/devicetree/bindings/iio/adc/ad7949.txt
> > > +++ b/Documentation/devicetree/bindings/iio/adc/ad7949.txt
> > > @@ -6,11 +6,29 @@ Required properties:
> > >       * "adi,ad7682"
> > >       * "adi,ad7689"
> > >   - reg: spi chip select number for the device
> > > - - vref-supply: The regulator supply for ADC reference voltage
> > >
> > > -Example:
> > > +Optional properties:
> > > + - adi,reference-select: Select the reference voltage source to use
> > > + when converting the input voltages. Valid values are:  
> > So my immediate thought here is we are mapping one binding to several
> > different things. Some of which are definitely better described in other
> > ways.
> >
> > So let us break it down:
> >
> > Internal vs external.
> > - External should require a regulator.  If the regulator is there, normal
> > assumption would be you want to use it.
> >
> > Which internal reference?  Hmm. This would be incompatible with the external
> > regulator and I'd expect the presence of such a regulator to override this.
> > That does need a new binding.
> > adi,internal-reference-milivolts = 2500 or 4096.   Much nicer to have
> > real numbers for someone wondering how it is configured than an enum.
> >
> > Temperature sensor enabled: Why is this a devicetree question rather than
> > a runtime decision?
> >
> > Buffer enabled: This needs a custom binding
> > adi,external-reference-buffer-enable or something like that?
> >
> > Makes for a more consistent binding where some elements can be common
> > across similar devices.  It would be good to see if similar bindings
> > already exist.  Potentially tings like the reference-buffer enable
> > may be worth making standard ADC properties rather than device
> > specific.
> >
> > Thanks,
> >
> > Jonathan
> >  
> The property name "adi,reference-select" was copied from the
> adi,ad7124 bindings as a similar hardware register configuration value
> field. If the property was separated into three independent fields,
> there would be a lot of explanation and checking required since many
> of the combinations are invalid (for example, temperature sensor and
> buffer are always enabled if internal reference is used). I could
> possibly see removing the temperature sensor configuration from the
> device tree, but, the current driver (even after these patches) does
> not provide any support to read the temperature sensor's value. I
> include that information in the configuration options as a summary of
> the datasheet.

There would certainly be nothing wrong with ignoring the temperature
sensor element for now.  It is the sort of thing it's possible no one will
ever actually add.

The reference select for 7124 was both much simpler than this and is
per channel.  To use a regulator presence in that case would require
defining a separate regulator for each channel.  The aim is always
to have the most readable possible (and generic) bindings, but
sometimes it really is too hard to do and we fall back on manufacturer
specific ones.  Here I don't think that is true.

I agree there is a small amount of additional complexity to validating
the provided settings, but it's not going to be that complex.

First see if there is an external regulator.
If there is check for buffer enable (and possibly temperature enable).

If no external buffer then check for internal ref.

There is no need to check for invalid combinations. The documentation
needs to include this but the code doesn't.

Thanks

Jonathan



> 
> Adam
> >  
> > > +   0: Internal 2.5V reference; temperature sensor enabled
> > > +   1: Internal 4.096V reference; temperature sensor enabled
> > > +   2: External reference, temperature sensor enabled, no buffer
> > > +   3: External reference, temperature sensor enabled, buffer enabled
> > > +   6: External reference, temperature sensor disabled, no buffer
> > > +   7: External reference, temperature sensor disabled, buffer enabled
> > > + - vref-supply: The regulator supply for ADC reference voltage. Required
> > > + if external reference selected by 'adi,reference-select'.
> > > +
> > > +Examples:
> > >  adc@0 {
> > >       compatible = "adi,ad7949";
> > >       reg = <0>;
> > > +     adi,reference-select = <2>;
> > >       vref-supply = <&vdd_supply>;
> > >  };
> > > +
> > > +adc@0 {
> > > +     compatible = "adi,ad7949";
> > > +     reg = <0>;
> > > +     adi,reference-select = <0>;
> > > +};  
> >  

Re: [External] Re: [PATCH 2/6] dt-bindings: iio: ad7949: Add adi,reference-select

[Adam Michaelis]

On Sat, May 11, 2019 at 5:39 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Tue, 7 May 2019 13:21:03 -0500
> Adam Michaelis <adam.michaelis@collins.com> wrote:
>
> > On Sun, May 5, 2019 at 7:22 AM Jonathan Cameron <jic23@kernel.org> wrote:
> > >
> > > On Wed,  1 May 2019 16:16:59 -0500
> > > Adam Michaelis <adam.michaelis@rockwellcollins.com> wrote:
> > >
> > > > Adding optional parameter to AD7949 to specify the source for the
> > > > reference voltage signal. Default value is maintaned with option '6' to
> > > > match previous version of driver.
> > > >
> > > > Signed-off-by: Adam Michaelis <adam.michaelis@rockwellcollins.com>
> > > > ---
> > > >  .../devicetree/bindings/iio/adc/ad7949.txt         | 22 ++++++++++++++++++++--
> > > >  1 file changed, 20 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/iio/adc/ad7949.txt b/Documentation/devicetree/bindings/iio/adc/ad7949.txt
> > > > index c7f5057356b1..14ee9a2cb2a5 100644
> > > > --- a/Documentation/devicetree/bindings/iio/adc/ad7949.txt
> > > > +++ b/Documentation/devicetree/bindings/iio/adc/ad7949.txt
> > > > @@ -6,11 +6,29 @@ Required properties:
> > > >       * "adi,ad7682"
> > > >       * "adi,ad7689"
> > > >   - reg: spi chip select number for the device
> > > > - - vref-supply: The regulator supply for ADC reference voltage
> > > >
> > > > -Example:
> > > > +Optional properties:
> > > > + - adi,reference-select: Select the reference voltage source to use
> > > > + when converting the input voltages. Valid values are:
> > > So my immediate thought here is we are mapping one binding to several
> > > different things. Some of which are definitely better described in other
> > > ways.
> > >
> > > So let us break it down:
> > >
> > > Internal vs external.
> > > - External should require a regulator.  If the regulator is there, normal
> > > assumption would be you want to use it.
> > >
> > > Which internal reference?  Hmm. This would be incompatible with the external
> > > regulator and I'd expect the presence of such a regulator to override this.
> > > That does need a new binding.
> > > adi,internal-reference-milivolts = 2500 or 4096.   Much nicer to have
> > > real numbers for someone wondering how it is configured than an enum.
> > >
> > > Temperature sensor enabled: Why is this a devicetree question rather than
> > > a runtime decision?
> > >
> > > Buffer enabled: This needs a custom binding
> > > adi,external-reference-buffer-enable or something like that?
> > >
> > > Makes for a more consistent binding where some elements can be common
> > > across similar devices.  It would be good to see if similar bindings
> > > already exist.  Potentially tings like the reference-buffer enable
> > > may be worth making standard ADC properties rather than device
> > > specific.
> > >
> > > Thanks,
> > >
> > > Jonathan
> > >
> > The property name "adi,reference-select" was copied from the
> > adi,ad7124 bindings as a similar hardware register configuration value
> > field. If the property was separated into three independent fields,
> > there would be a lot of explanation and checking required since many
> > of the combinations are invalid (for example, temperature sensor and
> > buffer are always enabled if internal reference is used). I could
> > possibly see removing the temperature sensor configuration from the
> > device tree, but, the current driver (even after these patches) does
> > not provide any support to read the temperature sensor's value. I
> > include that information in the configuration options as a summary of
> > the datasheet.
>
> There would certainly be nothing wrong with ignoring the temperature
> sensor element for now.  It is the sort of thing it's possible no one will
> ever actually add.
>
> The reference select for 7124 was both much simpler than this and is
> per channel.  To use a regulator presence in that case would require
> defining a separate regulator for each channel.  The aim is always
> to have the most readable possible (and generic) bindings, but
> sometimes it really is too hard to do and we fall back on manufacturer
> specific ones.  Here I don't think that is true.
>
> I agree there is a small amount of additional complexity to validating
> the provided settings, but it's not going to be that complex.
>
> First see if there is an external regulator.
> If there is check for buffer enable (and possibly temperature enable).
>
> If no external buffer then check for internal ref.
>
> There is no need to check for invalid combinations. The documentation
> needs to include this but the code doesn't.
>
> Thanks
>
> Jonathan
>
>
After taking a closer look at the datasheet, I have overhauled this part in V3
of the patch series such that it is still an enumeration for
reference-select, but
there are only 4 values solely relating to the voltage reference for the part:
 - Internal 2.5V
 - Internal 4.096V
 - External on pin REF
 - External on pin REFIN
The temp sensor is ignored, and the "buffer" logic is corrected to
refer to which
pin the external reference is supplied by (which side of the internal buffer).

Thanks for the feedback

Adam
>
> >
> > Adam
> > >
> > > > +   0: Internal 2.5V reference; temperature sensor enabled
> > > > +   1: Internal 4.096V reference; temperature sensor enabled
> > > > +   2: External reference, temperature sensor enabled, no buffer
> > > > +   3: External reference, temperature sensor enabled, buffer enabled
> > > > +   6: External reference, temperature sensor disabled, no buffer
> > > > +   7: External reference, temperature sensor disabled, buffer enabled
> > > > + - vref-supply: The regulator supply for ADC reference voltage. Required
> > > > + if external reference selected by 'adi,reference-select'.
> > > > +
> > > > +Examples:
> > > >  adc@0 {
> > > >       compatible = "adi,ad7949";
> > > >       reg = <0>;
> > > > +     adi,reference-select = <2>;
> > > >       vref-supply = <&vdd_supply>;
> > > >  };
> > > > +
> > > > +adc@0 {
> > > > +     compatible = "adi,ad7949";
> > > > +     reg = <0>;
> > > > +     adi,reference-select = <0>;
> > > > +};
> > >
>

[PATCH 2/6] dt-bindings: iio: ad7949: Add adi,reference-select

[Adam Michaelis]

From d228a1a119e33aff91f481fb8ab301a027b5a4ac Mon Sep 17 00:00:00 2001
From: Adam Michaelis <adam.michaelis@rockwellcollins.com>
Date: Thu, 25 Apr 2019 15:22:00 -0500
Subject: [PATCH 2/6] dt-bindings: iio: ad7949: Add adi,reference-select

Adding optional parameter to AD7949 to specify the source for the
reference voltage signal. Default value is maintaned with option '6' to
match previous version of driver.

Signed-off-by: Adam Michaelis <adam.michaelis@rockwellcollins.com>
---
Sorry about the re-sends - GMail inserted HTML on me and it bounced from vger.
---
 .../devicetree/bindings/iio/adc/ad7949.txt         | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/iio/adc/ad7949.txt
b/Documentation/devicetree/bindings/iio/adc/ad7949.txt
index c7f5057356b1..14ee9a2cb2a5 100644
--- a/Documentation/devicetree/bindings/iio/adc/ad7949.txt
+++ b/Documentation/devicetree/bindings/iio/adc/ad7949.txt
@@ -6,11 +6,29 @@ Required properties:
  * "adi,ad7682"
  * "adi,ad7689"
  - reg: spi chip select number for the device
- - vref-supply: The regulator supply for ADC reference voltage

-Example:
+Optional properties:
+ - adi,reference-select: Select the reference voltage source to use
+ when converting the input voltages. Valid values are:
+   0: Internal 2.5V reference; temperature sensor enabled
+   1: Internal 4.096V reference; temperature sensor enabled
+   2: External reference, temperature sensor enabled, no buffer
+   3: External reference, temperature sensor enabled, buffer enabled
+   6: External reference, temperature sensor disabled, no buffer
+   7: External reference, temperature sensor disabled, buffer enabled
+ - vref-supply: The regulator supply for ADC reference voltage. Required
+ if external reference selected by 'adi,reference-select'.
+
+Examples:
 adc@0 {
  compatible = "adi,ad7949";
  reg = <0>;
+ adi,reference-select = <2>;
  vref-supply = <&vdd_supply>;
 };
+
+adc@0 {
+ compatible = "adi,ad7949";
+ reg = <0>;
+ adi,reference-select = <0>;
+};
-- 
1.9.1