[PATCH 1/4] staging:iio:dac:ad5791: Use correct DAC bit-size

[PATCH 1/4] staging:iio:dac:ad5791: Use correct DAC bit-size

[Lars-Peter Clausen]

Commit c5b99396 ("staging:iio:dac:ad5791 chan spec conversion.") introduced a
small bug, using storagebits instead of realbits throughout the driver, which
causes the driver to work incorrectly. This patch fixes it.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/staging/iio/dac/ad5791.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/iio/dac/ad5791.c b/drivers/staging/iio/dac/ad5791.c
index fbf446d..9a76c43 100644
--- a/drivers/staging/iio/dac/ad5791.c
+++ b/drivers/staging/iio/dac/ad5791.c
@@ -234,11 +234,11 @@ static int ad5791_read_raw(struct iio_dev *indio_dev,
 			return ret;
 		*val &= AD5791_DAC_MASK;
 		*val >>= chan->scan_type.shift;
-		*val -= (1 << (chan->scan_type.storagebits - 1));
+		*val -= (1 << (chan->scan_type.realbits - 1));
 		return IIO_VAL_INT;
 	case (1 << IIO_CHAN_INFO_SCALE_SHARED):
 		*val = 0;
-		*val2 = (st->vref_mv * 1000) >> chan->scan_type.storagebits;
+		*val2 = (st->vref_mv * 1000) >> chan->scan_type.realbits;
 		return IIO_VAL_INT_PLUS_MICRO;
 	default:
 		return -EINVAL;
@@ -257,8 +257,8 @@ static int ad5791_write_raw(struct iio_dev *indio_dev,
 
 	switch (mask) {
 	case 0:
-		val += (1 << (chan->scan_type.storagebits - 1));
-		val &= AD5791_RES_MASK(chan->scan_type.storagebits);
+		val += (1 << (chan->scan_type.realbits - 1));
+		val &= AD5791_RES_MASK(chan->scan_type.realbits);
 		val <<= chan->scan_type.shift;
 
 		return ad5791_spi_write(st->spi, chan->address, val);
-- 
1.7.6.3

[PATCH 2/4] staging:iio:dac:ad5791: Allow asymmetrical reference voltages

[Lars-Peter Clausen]

The ad5791 currently assumes that the negative and positive supply have the
same absolute value, which is not necessarily true. This patch introduces a
offset attribute which will contain the negative supply voltage scaled
according to the iio spec. The raw attribute now accepts values in the range
of 0 to max instead of -max/2 to max/2.

While we are at it also fix the vref span calculation. Since both positive and
negative reference voltages are specificed as absolute values we need to add
them and not subtract them to get the reference voltage span.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/staging/iio/dac/ad5791.c |   24 ++++++++++++++++--------
 drivers/staging/iio/dac/ad5791.h |    2 ++
 2 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/iio/dac/ad5791.c b/drivers/staging/iio/dac/ad5791.c
index 9a76c43..c465fcf 100644
--- a/drivers/staging/iio/dac/ad5791.c
+++ b/drivers/staging/iio/dac/ad5791.c
@@ -77,7 +77,8 @@ static int ad5791_spi_read(struct spi_device *spi, u8 addr, u32 *val)
 	.indexed = 1,					\
 	.address = AD5791_ADDR_DAC0,			\
 	.channel = 0,					\
-	.info_mask = (1 << IIO_CHAN_INFO_SCALE_SHARED),	\
+	.info_mask = (1 << IIO_CHAN_INFO_SCALE_SHARED) | \
+		(1 << IIO_CHAN_INFO_OFFSET_SHARED),	\
 	.scan_type = IIO_ST('u', bits, 24, shift)	\
 }
 
@@ -225,6 +226,7 @@ static int ad5791_read_raw(struct iio_dev *indio_dev,
 			   long m)
 {
 	struct ad5791_state *st = iio_priv(indio_dev);
+	u64 val64;
 	int ret;
 
 	switch (m) {
@@ -234,12 +236,16 @@ static int ad5791_read_raw(struct iio_dev *indio_dev,
 			return ret;
 		*val &= AD5791_DAC_MASK;
 		*val >>= chan->scan_type.shift;
-		*val -= (1 << (chan->scan_type.realbits - 1));
 		return IIO_VAL_INT;
 	case (1 << IIO_CHAN_INFO_SCALE_SHARED):
 		*val = 0;
 		*val2 = (st->vref_mv * 1000) >> chan->scan_type.realbits;
 		return IIO_VAL_INT_PLUS_MICRO;
+	case (1 << IIO_CHAN_INFO_OFFSET_SHARED):
+		val64 = (((u64)st->vref_neg_mv) << chan->scan_type.realbits);
+		do_div(val64, st->vref_mv);
+		*val = -val64;
+		return IIO_VAL_INT;
 	default:
 		return -EINVAL;
 	}
@@ -257,7 +263,6 @@ static int ad5791_write_raw(struct iio_dev *indio_dev,
 
 	switch (mask) {
 	case 0:
-		val += (1 << (chan->scan_type.realbits - 1));
 		val &= AD5791_RES_MASK(chan->scan_type.realbits);
 		val <<= chan->scan_type.shift;
 
@@ -309,12 +314,15 @@ static int __devinit ad5791_probe(struct spi_device *spi)
 	st->pwr_down = true;
 	st->spi = spi;
 
-	if (!IS_ERR(st->reg_vss) && !IS_ERR(st->reg_vdd))
-		st->vref_mv = (pos_voltage_uv - neg_voltage_uv) / 1000;
-	else if (pdata)
-		st->vref_mv = pdata->vref_pos_mv - pdata->vref_neg_mv;
-	else
+	if (!IS_ERR(st->reg_vss) && !IS_ERR(st->reg_vdd)) {
+		st->vref_mv = (pos_voltage_uv + neg_voltage_uv) / 1000;
+		st->vref_neg_mv = neg_voltage_uv / 1000;
+	} else if (pdata) {
+		st->vref_mv = pdata->vref_pos_mv + pdata->vref_neg_mv;
+		st->vref_neg_mv = pdata->vref_neg_mv;
+	} else {
 		dev_warn(&spi->dev, "reference voltage unspecified\n");
+	}
 
 	ret = ad5791_spi_write(spi, AD5791_ADDR_SW_CTRL, AD5791_SWCTRL_RESET);
 	if (ret)
diff --git a/drivers/staging/iio/dac/ad5791.h b/drivers/staging/iio/dac/ad5791.h
index 2a50a11..fd7edbd 100644
--- a/drivers/staging/iio/dac/ad5791.h
+++ b/drivers/staging/iio/dac/ad5791.h
@@ -82,6 +82,7 @@ struct ad5791_chip_info {
  * @reg_vss:		negative supply regulator
  * @chip_info:		chip model specific constants
  * @vref_mv:		actual reference voltage used
+ * @vref_neg_mv:	voltage of the negative supply
  * @pwr_down_mode	current power down mode
  */
 
@@ -91,6 +92,7 @@ struct ad5791_state {
 	struct regulator		*reg_vss;
 	const struct ad5791_chip_info	*chip_info;
 	unsigned short			vref_mv;
+	unsigned int			vref_neg_mv;
 	unsigned			ctrl;
 	unsigned			pwr_down_mode;
 	bool				pwr_down;
-- 
1.7.6.3

[PATCH 3/4] staging:iio:dac:ad5791: Convert attributes to new naming spec

[Lars-Peter Clausen]

Add the missing "voltage" chan_type to the powerdown attributes.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/staging/iio/dac/ad5791.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/iio/dac/ad5791.c b/drivers/staging/iio/dac/ad5791.c
index c465fcf..9ba45f1 100644
--- a/drivers/staging/iio/dac/ad5791.c
+++ b/drivers/staging/iio/dac/ad5791.c
@@ -158,24 +158,24 @@ static ssize_t ad5791_write_dac_powerdown(struct device *dev,
 	return ret ? ret : len;
 }
 
-static IIO_DEVICE_ATTR(out_powerdown_mode, S_IRUGO |
+static IIO_DEVICE_ATTR(out_voltage_powerdown_mode, S_IRUGO |
 			S_IWUSR, ad5791_read_powerdown_mode,
 			ad5791_write_powerdown_mode, 0);
 
-static IIO_CONST_ATTR(out_powerdown_mode_available,
+static IIO_CONST_ATTR(out_voltage_powerdown_mode_available,
 			"6kohm_to_gnd three_state");
 
 #define IIO_DEV_ATTR_DAC_POWERDOWN(_num, _show, _store, _addr)		\
-	IIO_DEVICE_ATTR(out##_num##_powerdown,				\
+	IIO_DEVICE_ATTR(out_voltage##_num##_powerdown,			\
 			S_IRUGO | S_IWUSR, _show, _store, _addr)
 
 static IIO_DEV_ATTR_DAC_POWERDOWN(0, ad5791_read_dac_powerdown,
 				   ad5791_write_dac_powerdown, 0);
 
 static struct attribute *ad5791_attributes[] = {
-	&iio_dev_attr_out0_powerdown.dev_attr.attr,
-	&iio_dev_attr_out_powerdown_mode.dev_attr.attr,
-	&iio_const_attr_out_powerdown_mode_available.dev_attr.attr,
+	&iio_dev_attr_out_voltage0_powerdown.dev_attr.attr,
+	&iio_dev_attr_out_voltage_powerdown_mode.dev_attr.attr,
+	&iio_const_attr_out_voltage_powerdown_mode_available.dev_attr.attr,
 	NULL,
 };
 
-- 
1.7.6.3

[PATCH 4/4] staging:iio:dac:ad5791: Fix scale unit

[Lars-Peter Clausen]

Scale is currently reported in volts instead of millivolts. This patch fixes it.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/staging/iio/dac/ad5791.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/staging/iio/dac/ad5791.c b/drivers/staging/iio/dac/ad5791.c
index 9ba45f1..6fbca8d 100644
--- a/drivers/staging/iio/dac/ad5791.c
+++ b/drivers/staging/iio/dac/ad5791.c
@@ -239,7 +239,7 @@ static int ad5791_read_raw(struct iio_dev *indio_dev,
 		return IIO_VAL_INT;
 	case (1 << IIO_CHAN_INFO_SCALE_SHARED):
 		*val = 0;
-		*val2 = (st->vref_mv * 1000) >> chan->scan_type.realbits;
+		*val2 = (((u64)st->vref_mv) * 1000000ULL) >> chan->scan_type.realbits;
 		return IIO_VAL_INT_PLUS_MICRO;
 	case (1 << IIO_CHAN_INFO_OFFSET_SHARED):
 		val64 = (((u64)st->vref_neg_mv) << chan->scan_type.realbits);
-- 
1.7.6.3

Re: [PATCH 1/4] staging:iio:dac:ad5791: Use correct DAC bit-size

[Jonathan Cameron]

On 10/19/11 13:23, Lars-Peter Clausen wrote:
> Commit c5b99396 ("staging:iio:dac:ad5791 chan spec conversion.") introduced a
> small bug, using storagebits instead of realbits throughout the driver, which
> causes the driver to work incorrectly. This patch fixes it.
oops
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Acked-by: Jonathan Cameron <jic23@cam.ac.uk>
> ---
>  drivers/staging/iio/dac/ad5791.c |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/iio/dac/ad5791.c b/drivers/staging/iio/dac/ad5791.c
> index fbf446d..9a76c43 100644
> --- a/drivers/staging/iio/dac/ad5791.c
> +++ b/drivers/staging/iio/dac/ad5791.c
> @@ -234,11 +234,11 @@ static int ad5791_read_raw(struct iio_dev *indio_dev,
>  			return ret;
>  		*val &= AD5791_DAC_MASK;
>  		*val >>= chan->scan_type.shift;
> -		*val -= (1 << (chan->scan_type.storagebits - 1));
> +		*val -= (1 << (chan->scan_type.realbits - 1));
>  		return IIO_VAL_INT;
>  	case (1 << IIO_CHAN_INFO_SCALE_SHARED):
>  		*val = 0;
> -		*val2 = (st->vref_mv * 1000) >> chan->scan_type.storagebits;
> +		*val2 = (st->vref_mv * 1000) >> chan->scan_type.realbits;
>  		return IIO_VAL_INT_PLUS_MICRO;
>  	default:
>  		return -EINVAL;
> @@ -257,8 +257,8 @@ static int ad5791_write_raw(struct iio_dev *indio_dev,
>  
>  	switch (mask) {
>  	case 0:
> -		val += (1 << (chan->scan_type.storagebits - 1));
> -		val &= AD5791_RES_MASK(chan->scan_type.storagebits);
> +		val += (1 << (chan->scan_type.realbits - 1));
> +		val &= AD5791_RES_MASK(chan->scan_type.realbits);
>  		val <<= chan->scan_type.shift;
>  
>  		return ad5791_spi_write(st->spi, chan->address, val);

Re: [PATCH 2/4] staging:iio:dac:ad5791: Allow asymmetrical reference voltages

[Jonathan Cameron]

On 10/19/11 13:23, Lars-Peter Clausen wrote:
> The ad5791 currently assumes that the negative and positive supply have the
> same absolute value, which is not necessarily true. This patch introduces a
> offset attribute which will contain the negative supply voltage scaled
> according to the iio spec. The raw attribute now accepts values in the range
> of 0 to max instead of -max/2 to max/2.
> 
> While we are at it also fix the vref span calculation. Since both positive and
> negative reference voltages are specificed as absolute values we need to add
> them and not subtract them to get the reference voltage span.
Everything commented here is fine, but one odd looking removal...
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> ---
>  drivers/staging/iio/dac/ad5791.c |   24 ++++++++++++++++--------
>  drivers/staging/iio/dac/ad5791.h |    2 ++
>  2 files changed, 18 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/staging/iio/dac/ad5791.c b/drivers/staging/iio/dac/ad5791.c
> index 9a76c43..c465fcf 100644
> --- a/drivers/staging/iio/dac/ad5791.c
> +++ b/drivers/staging/iio/dac/ad5791.c
> @@ -77,7 +77,8 @@ static int ad5791_spi_read(struct spi_device *spi, u8 addr, u32 *val)
>  	.indexed = 1,					\
>  	.address = AD5791_ADDR_DAC0,			\
>  	.channel = 0,					\
> -	.info_mask = (1 << IIO_CHAN_INFO_SCALE_SHARED),	\
> +	.info_mask = (1 << IIO_CHAN_INFO_SCALE_SHARED) | \
> +		(1 << IIO_CHAN_INFO_OFFSET_SHARED),	\
>  	.scan_type = IIO_ST('u', bits, 24, shift)	\
>  }
>  
> @@ -225,6 +226,7 @@ static int ad5791_read_raw(struct iio_dev *indio_dev,
>  			   long m)
>  {
>  	struct ad5791_state *st = iio_priv(indio_dev);
> +	u64 val64;
>  	int ret;
>  
>  	switch (m) {
> @@ -234,12 +236,16 @@ static int ad5791_read_raw(struct iio_dev *indio_dev,
>  			return ret;
>  		*val &= AD5791_DAC_MASK;
>  		*val >>= chan->scan_type.shift;
> -		*val -= (1 << (chan->scan_type.realbits - 1));
>  		return IIO_VAL_INT;
>  	case (1 << IIO_CHAN_INFO_SCALE_SHARED):
>  		*val = 0;
>  		*val2 = (st->vref_mv * 1000) >> chan->scan_type.realbits;
>  		return IIO_VAL_INT_PLUS_MICRO;
> +	case (1 << IIO_CHAN_INFO_OFFSET_SHARED):
> +		val64 = (((u64)st->vref_neg_mv) << chan->scan_type.realbits);
> +		do_div(val64, st->vref_mv);
> +		*val = -val64;
> +		return IIO_VAL_INT;
>  	default:
>  		return -EINVAL;
>  	}
> @@ -257,7 +263,6 @@ static int ad5791_write_raw(struct iio_dev *indio_dev,
>  
>  	switch (mask) {
>  	case 0:
> -		val += (1 << (chan->scan_type.realbits - 1));
This line looks suspicious.  What is it doing in this patch?
I'll be honest and say I'm not sure why it was there in the first place, but
I can't immediately see what it has to do with the rest of the patch.
>  		val &= AD5791_RES_MASK(chan->scan_type.realbits);
>  		val <<= chan->scan_type.shift;
>  
> @@ -309,12 +314,15 @@ static int __devinit ad5791_probe(struct spi_device *spi)
>  	st->pwr_down = true;
>  	st->spi = spi;
>  
> -	if (!IS_ERR(st->reg_vss) && !IS_ERR(st->reg_vdd))
> -		st->vref_mv = (pos_voltage_uv - neg_voltage_uv) / 1000;
> -	else if (pdata)
> -		st->vref_mv = pdata->vref_pos_mv - pdata->vref_neg_mv;
> -	else
> +	if (!IS_ERR(st->reg_vss) && !IS_ERR(st->reg_vdd)) {
> +		st->vref_mv = (pos_voltage_uv + neg_voltage_uv) / 1000;
> +		st->vref_neg_mv = neg_voltage_uv / 1000;
> +	} else if (pdata) {
> +		st->vref_mv = pdata->vref_pos_mv + pdata->vref_neg_mv;
> +		st->vref_neg_mv = pdata->vref_neg_mv;
> +	} else {
>  		dev_warn(&spi->dev, "reference voltage unspecified\n");
> +	}
>  
>  	ret = ad5791_spi_write(spi, AD5791_ADDR_SW_CTRL, AD5791_SWCTRL_RESET);
>  	if (ret)
> diff --git a/drivers/staging/iio/dac/ad5791.h b/drivers/staging/iio/dac/ad5791.h
> index 2a50a11..fd7edbd 100644
> --- a/drivers/staging/iio/dac/ad5791.h
> +++ b/drivers/staging/iio/dac/ad5791.h
> @@ -82,6 +82,7 @@ struct ad5791_chip_info {
>   * @reg_vss:		negative supply regulator
>   * @chip_info:		chip model specific constants
>   * @vref_mv:		actual reference voltage used
> + * @vref_neg_mv:	voltage of the negative supply
>   * @pwr_down_mode	current power down mode
>   */
>  
> @@ -91,6 +92,7 @@ struct ad5791_state {
>  	struct regulator		*reg_vss;
>  	const struct ad5791_chip_info	*chip_info;
>  	unsigned short			vref_mv;
> +	unsigned int			vref_neg_mv;
>  	unsigned			ctrl;
>  	unsigned			pwr_down_mode;
>  	bool				pwr_down;

Re: [PATCH 3/4] staging:iio:dac:ad5791: Convert attributes to new naming spec

[Jonathan Cameron]

On 10/19/11 13:23, Lars-Peter Clausen wrote:
> Add the missing "voltage" chan_type to the powerdown attributes.
Dratt. I thought I'd fixed all these. Guess I forgot the
output devices.  Good catch
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Acked-by: Jonathan Cameron <jic23@cam.ac.uk>
> ---
>  drivers/staging/iio/dac/ad5791.c |   12 ++++++------
>  1 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/iio/dac/ad5791.c b/drivers/staging/iio/dac/ad5791.c
> index c465fcf..9ba45f1 100644
> --- a/drivers/staging/iio/dac/ad5791.c
> +++ b/drivers/staging/iio/dac/ad5791.c
> @@ -158,24 +158,24 @@ static ssize_t ad5791_write_dac_powerdown(struct device *dev,
>  	return ret ? ret : len;
>  }
>  
> -static IIO_DEVICE_ATTR(out_powerdown_mode, S_IRUGO |
> +static IIO_DEVICE_ATTR(out_voltage_powerdown_mode, S_IRUGO |
>  			S_IWUSR, ad5791_read_powerdown_mode,
>  			ad5791_write_powerdown_mode, 0);
>  
> -static IIO_CONST_ATTR(out_powerdown_mode_available,
> +static IIO_CONST_ATTR(out_voltage_powerdown_mode_available,
>  			"6kohm_to_gnd three_state");
>  
>  #define IIO_DEV_ATTR_DAC_POWERDOWN(_num, _show, _store, _addr)		\
> -	IIO_DEVICE_ATTR(out##_num##_powerdown,				\
> +	IIO_DEVICE_ATTR(out_voltage##_num##_powerdown,			\
>  			S_IRUGO | S_IWUSR, _show, _store, _addr)
>  
>  static IIO_DEV_ATTR_DAC_POWERDOWN(0, ad5791_read_dac_powerdown,
>  				   ad5791_write_dac_powerdown, 0);
>  
>  static struct attribute *ad5791_attributes[] = {
> -	&iio_dev_attr_out0_powerdown.dev_attr.attr,
> -	&iio_dev_attr_out_powerdown_mode.dev_attr.attr,
> -	&iio_const_attr_out_powerdown_mode_available.dev_attr.attr,
> +	&iio_dev_attr_out_voltage0_powerdown.dev_attr.attr,
> +	&iio_dev_attr_out_voltage_powerdown_mode.dev_attr.attr,
> +	&iio_const_attr_out_voltage_powerdown_mode_available.dev_attr.attr,
>  	NULL,
>  };
>  

Re: [PATCH 4/4] staging:iio:dac:ad5791: Fix scale unit

[Jonathan Cameron]

On 10/19/11 13:23, Lars-Peter Clausen wrote:
> Scale is currently reported in volts instead of millivolts. This patch fixes it.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Acked-by: Jonathan Cameron <jic23@cam.ac.uk>
> ---
>  drivers/staging/iio/dac/ad5791.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/staging/iio/dac/ad5791.c b/drivers/staging/iio/dac/ad5791.c
> index 9ba45f1..6fbca8d 100644
> --- a/drivers/staging/iio/dac/ad5791.c
> +++ b/drivers/staging/iio/dac/ad5791.c
> @@ -239,7 +239,7 @@ static int ad5791_read_raw(struct iio_dev *indio_dev,
>  		return IIO_VAL_INT;
>  	case (1 << IIO_CHAN_INFO_SCALE_SHARED):
>  		*val = 0;
> -		*val2 = (st->vref_mv * 1000) >> chan->scan_type.realbits;
> +		*val2 = (((u64)st->vref_mv) * 1000000ULL) >> chan->scan_type.realbits;
>  		return IIO_VAL_INT_PLUS_MICRO;
>  	case (1 << IIO_CHAN_INFO_OFFSET_SHARED):
>  		val64 = (((u64)st->vref_neg_mv) << chan->scan_type.realbits);

Re: [PATCH 2/4] staging:iio:dac:ad5791: Allow asymmetrical reference voltages

[Lars-Peter Clausen]

On 10/19/2011 03:01 PM, Jonathan Cameron wrote:
> On 10/19/11 13:23, Lars-Peter Clausen wrote:
>> The ad5791 currently assumes that the negative and positive supply have the
>> same absolute value, which is not necessarily true. This patch introduces a
>> offset attribute which will contain the negative supply voltage scaled
>> according to the iio spec. The raw attribute now accepts values in the range
>> of 0 to max instead of -max/2 to max/2.
>>
>> While we are at it also fix the vref span calculation. Since both positive and
>> negative reference voltages are specificed as absolute values we need to add
>> them and not subtract them to get the reference voltage span.
> Everything commented here is fine, but one odd looking removal...
>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
>> ---
>>  drivers/staging/iio/dac/ad5791.c |   24 ++++++++++++++++--------
>>  drivers/staging/iio/dac/ad5791.h |    2 ++
>>  2 files changed, 18 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/staging/iio/dac/ad5791.c b/drivers/staging/iio/dac/ad5791.c
>> index 9a76c43..c465fcf 100644
>> --- a/drivers/staging/iio/dac/ad5791.c
>> +++ b/drivers/staging/iio/dac/ad5791.c
>> @@ -77,7 +77,8 @@ static int ad5791_spi_read(struct spi_device *spi, u8 addr, u32 *val)
>>  	.indexed = 1,					\
>>  	.address = AD5791_ADDR_DAC0,			\
>>  	.channel = 0,					\
>> -	.info_mask = (1 << IIO_CHAN_INFO_SCALE_SHARED),	\
>> +	.info_mask = (1 << IIO_CHAN_INFO_SCALE_SHARED) | \
>> +		(1 << IIO_CHAN_INFO_OFFSET_SHARED),	\
>>  	.scan_type = IIO_ST('u', bits, 24, shift)	\
>>  }
>>  
>> @@ -225,6 +226,7 @@ static int ad5791_read_raw(struct iio_dev *indio_dev,
>>  			   long m)
>>  {
>>  	struct ad5791_state *st = iio_priv(indio_dev);
>> +	u64 val64;
>>  	int ret;
>>  
>>  	switch (m) {
>> @@ -234,12 +236,16 @@ static int ad5791_read_raw(struct iio_dev *indio_dev,
>>  			return ret;
>>  		*val &= AD5791_DAC_MASK;
>>  		*val >>= chan->scan_type.shift;
>> -		*val -= (1 << (chan->scan_type.realbits - 1));
>>  		return IIO_VAL_INT;
>>  	case (1 << IIO_CHAN_INFO_SCALE_SHARED):
>>  		*val = 0;
>>  		*val2 = (st->vref_mv * 1000) >> chan->scan_type.realbits;
>>  		return IIO_VAL_INT_PLUS_MICRO;
>> +	case (1 << IIO_CHAN_INFO_OFFSET_SHARED):
>> +		val64 = (((u64)st->vref_neg_mv) << chan->scan_type.realbits);
>> +		do_div(val64, st->vref_mv);
>> +		*val = -val64;
>> +		return IIO_VAL_INT;
>>  	default:
>>  		return -EINVAL;
>>  	}
>> @@ -257,7 +263,6 @@ static int ad5791_write_raw(struct iio_dev *indio_dev,
>>  
>>  	switch (mask) {
>>  	case 0:
>> -		val += (1 << (chan->scan_type.realbits - 1));
> This line looks suspicious.  What is it doing in this patch?
> I'll be honest and say I'm not sure why it was there in the first place, but
> I can't immediately see what it has to do with the rest of the patch.

The driver would previously accept values from -(1 << realbits) / 2 to (1 <<
realbits) / 2
for the raw attribute. But this only really works if the the reference
voltages are symmetrical.
So what this patch does is to change the raw attribute to accept values from
0 to (1 << realbits) -1
and add the offset attribute which contains the negative offset.

>>  		val &= AD5791_RES_MASK(chan->scan_type.realbits);
>>  		val <<= chan->scan_type.shift;
>>  
>> @@ -309,12 +314,15 @@ static int __devinit ad5791_probe(struct spi_device *spi)
>>  	st->pwr_down = true;
>>  	st->spi = spi;
>>  
>> -	if (!IS_ERR(st->reg_vss) && !IS_ERR(st->reg_vdd))
>> -		st->vref_mv = (pos_voltage_uv - neg_voltage_uv) / 1000;
>> -	else if (pdata)
>> -		st->vref_mv = pdata->vref_pos_mv - pdata->vref_neg_mv;
>> -	else
>> +	if (!IS_ERR(st->reg_vss) && !IS_ERR(st->reg_vdd)) {
>> +		st->vref_mv = (pos_voltage_uv + neg_voltage_uv) / 1000;
>> +		st->vref_neg_mv = neg_voltage_uv / 1000;
>> +	} else if (pdata) {
>> +		st->vref_mv = pdata->vref_pos_mv + pdata->vref_neg_mv;
>> +		st->vref_neg_mv = pdata->vref_neg_mv;
>> +	} else {
>>  		dev_warn(&spi->dev, "reference voltage unspecified\n");
>> +	}
>>  
>>  	ret = ad5791_spi_write(spi, AD5791_ADDR_SW_CTRL, AD5791_SWCTRL_RESET);
>>  	if (ret)
>> diff --git a/drivers/staging/iio/dac/ad5791.h b/drivers/staging/iio/dac/ad5791.h
>> index 2a50a11..fd7edbd 100644
>> --- a/drivers/staging/iio/dac/ad5791.h
>> +++ b/drivers/staging/iio/dac/ad5791.h
>> @@ -82,6 +82,7 @@ struct ad5791_chip_info {
>>   * @reg_vss:		negative supply regulator
>>   * @chip_info:		chip model specific constants
>>   * @vref_mv:		actual reference voltage used
>> + * @vref_neg_mv:	voltage of the negative supply
>>   * @pwr_down_mode	current power down mode
>>   */
>>  
>> @@ -91,6 +92,7 @@ struct ad5791_state {
>>  	struct regulator		*reg_vss;
>>  	const struct ad5791_chip_info	*chip_info;
>>  	unsigned short			vref_mv;
>> +	unsigned int			vref_neg_mv;
>>  	unsigned			ctrl;
>>  	unsigned			pwr_down_mode;
>>  	bool				pwr_down;
>

Re: [PATCH 2/4] staging:iio:dac:ad5791: Allow asymmetrical reference voltages

[Jonathan Cameron]

On 10/19/11 14:21, Lars-Peter Clausen wrote:
> On 10/19/2011 03:01 PM, Jonathan Cameron wrote:
>> On 10/19/11 13:23, Lars-Peter Clausen wrote:
>>> The ad5791 currently assumes that the negative and positive supply have the
>>> same absolute value, which is not necessarily true. This patch introduces a
>>> offset attribute which will contain the negative supply voltage scaled
>>> according to the iio spec. The raw attribute now accepts values in the range
>>> of 0 to max instead of -max/2 to max/2.
>>>
>>> While we are at it also fix the vref span calculation. Since both positive and
>>> negative reference voltages are specificed as absolute values we need to add
>>> them and not subtract them to get the reference voltage span.
>> Everything commented here is fine, but one odd looking removal...
Got it now thanks.
>>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Acked-by: Jonathan Cameron <jic23@cam.ac.uk>

>>> ---
>>>  drivers/staging/iio/dac/ad5791.c |   24 ++++++++++++++++--------
>>>  drivers/staging/iio/dac/ad5791.h |    2 ++
>>>  2 files changed, 18 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/staging/iio/dac/ad5791.c b/drivers/staging/iio/dac/ad5791.c
>>> index 9a76c43..c465fcf 100644
>>> --- a/drivers/staging/iio/dac/ad5791.c
>>> +++ b/drivers/staging/iio/dac/ad5791.c
>>> @@ -77,7 +77,8 @@ static int ad5791_spi_read(struct spi_device *spi, u8 addr, u32 *val)
>>>  	.indexed = 1,					\
>>>  	.address = AD5791_ADDR_DAC0,			\
>>>  	.channel = 0,					\
>>> -	.info_mask = (1 << IIO_CHAN_INFO_SCALE_SHARED),	\
>>> +	.info_mask = (1 << IIO_CHAN_INFO_SCALE_SHARED) | \
>>> +		(1 << IIO_CHAN_INFO_OFFSET_SHARED),	\
>>>  	.scan_type = IIO_ST('u', bits, 24, shift)	\
>>>  }
>>>  
>>> @@ -225,6 +226,7 @@ static int ad5791_read_raw(struct iio_dev *indio_dev,
>>>  			   long m)
>>>  {
>>>  	struct ad5791_state *st = iio_priv(indio_dev);
>>> +	u64 val64;
>>>  	int ret;
>>>  
>>>  	switch (m) {
>>> @@ -234,12 +236,16 @@ static int ad5791_read_raw(struct iio_dev *indio_dev,
>>>  			return ret;
>>>  		*val &= AD5791_DAC_MASK;
>>>  		*val >>= chan->scan_type.shift;
>>> -		*val -= (1 << (chan->scan_type.realbits - 1));
>>>  		return IIO_VAL_INT;
>>>  	case (1 << IIO_CHAN_INFO_SCALE_SHARED):
>>>  		*val = 0;
>>>  		*val2 = (st->vref_mv * 1000) >> chan->scan_type.realbits;
>>>  		return IIO_VAL_INT_PLUS_MICRO;
>>> +	case (1 << IIO_CHAN_INFO_OFFSET_SHARED):
>>> +		val64 = (((u64)st->vref_neg_mv) << chan->scan_type.realbits);
>>> +		do_div(val64, st->vref_mv);
>>> +		*val = -val64;
>>> +		return IIO_VAL_INT;
>>>  	default:
>>>  		return -EINVAL;
>>>  	}
>>> @@ -257,7 +263,6 @@ static int ad5791_write_raw(struct iio_dev *indio_dev,
>>>  
>>>  	switch (mask) {
>>>  	case 0:
>>> -		val += (1 << (chan->scan_type.realbits - 1));
>> This line looks suspicious.  What is it doing in this patch?
>> I'll be honest and say I'm not sure why it was there in the first place, but
>> I can't immediately see what it has to do with the rest of the patch.
> 
> The driver would previously accept values from -(1 << realbits) / 2 to (1 <<
> realbits) / 2
> for the raw attribute. But this only really works if the the reference
> voltages are symmetrical.
> So what this patch does is to change the raw attribute to accept values from
> 0 to (1 << realbits) -1
> and add the offset attribute which contains the negative offset.
> 
>>>  		val &= AD5791_RES_MASK(chan->scan_type.realbits);
>>>  		val <<= chan->scan_type.shift;
>>>  
>>> @@ -309,12 +314,15 @@ static int __devinit ad5791_probe(struct spi_device *spi)
>>>  	st->pwr_down = true;
>>>  	st->spi = spi;
>>>  
>>> -	if (!IS_ERR(st->reg_vss) && !IS_ERR(st->reg_vdd))
>>> -		st->vref_mv = (pos_voltage_uv - neg_voltage_uv) / 1000;
>>> -	else if (pdata)
>>> -		st->vref_mv = pdata->vref_pos_mv - pdata->vref_neg_mv;
>>> -	else
>>> +	if (!IS_ERR(st->reg_vss) && !IS_ERR(st->reg_vdd)) {
>>> +		st->vref_mv = (pos_voltage_uv + neg_voltage_uv) / 1000;
>>> +		st->vref_neg_mv = neg_voltage_uv / 1000;
>>> +	} else if (pdata) {
>>> +		st->vref_mv = pdata->vref_pos_mv + pdata->vref_neg_mv;
>>> +		st->vref_neg_mv = pdata->vref_neg_mv;
>>> +	} else {
>>>  		dev_warn(&spi->dev, "reference voltage unspecified\n");
>>> +	}
>>>  
>>>  	ret = ad5791_spi_write(spi, AD5791_ADDR_SW_CTRL, AD5791_SWCTRL_RESET);
>>>  	if (ret)
>>> diff --git a/drivers/staging/iio/dac/ad5791.h b/drivers/staging/iio/dac/ad5791.h
>>> index 2a50a11..fd7edbd 100644
>>> --- a/drivers/staging/iio/dac/ad5791.h
>>> +++ b/drivers/staging/iio/dac/ad5791.h
>>> @@ -82,6 +82,7 @@ struct ad5791_chip_info {
>>>   * @reg_vss:		negative supply regulator
>>>   * @chip_info:		chip model specific constants
>>>   * @vref_mv:		actual reference voltage used
>>> + * @vref_neg_mv:	voltage of the negative supply
>>>   * @pwr_down_mode	current power down mode
>>>   */
>>>  
>>> @@ -91,6 +92,7 @@ struct ad5791_state {
>>>  	struct regulator		*reg_vss;
>>>  	const struct ad5791_chip_info	*chip_info;
>>>  	unsigned short			vref_mv;
>>> +	unsigned int			vref_neg_mv;
>>>  	unsigned			ctrl;
>>>  	unsigned			pwr_down_mode;
>>>  	bool				pwr_down;
>>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[PATCH 2/4] staging:iio:dac:ad5791: Allow asymmetrical reference voltages

[Lars-Peter Clausen]

The ad5791 currently assumes that the negative and positive supply have the
same absolute value, which is not necessarily true. This patch introduces a
offset attribute which will contain the negative supply voltage scaled
according to the iio spec. The raw attribute now accepts values in the range
of 0 to max instead of -max/2 to max/2.

While we are at it also fix the vref span calculation. Since both positive and
negative reference voltages are specificed as absolute values we need to add
them and not subtract them to get the reference voltage span.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Acked-by: Jonathan Cameron <jic23@cam.ac.uk>
---
 drivers/staging/iio/dac/ad5791.c |   24 ++++++++++++++++--------
 drivers/staging/iio/dac/ad5791.h |    2 ++
 2 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/iio/dac/ad5791.c b/drivers/staging/iio/dac/ad5791.c
index 9a76c43..c465fcf 100644
--- a/drivers/staging/iio/dac/ad5791.c
+++ b/drivers/staging/iio/dac/ad5791.c
@@ -77,7 +77,8 @@ static int ad5791_spi_read(struct spi_device *spi, u8 addr, u32 *val)
 	.indexed = 1,					\
 	.address = AD5791_ADDR_DAC0,			\
 	.channel = 0,					\
-	.info_mask = (1 << IIO_CHAN_INFO_SCALE_SHARED),	\
+	.info_mask = (1 << IIO_CHAN_INFO_SCALE_SHARED) | \
+		(1 << IIO_CHAN_INFO_OFFSET_SHARED),	\
 	.scan_type = IIO_ST('u', bits, 24, shift)	\
 }
 
@@ -225,6 +226,7 @@ static int ad5791_read_raw(struct iio_dev *indio_dev,
 			   long m)
 {
 	struct ad5791_state *st = iio_priv(indio_dev);
+	u64 val64;
 	int ret;
 
 	switch (m) {
@@ -234,12 +236,16 @@ static int ad5791_read_raw(struct iio_dev *indio_dev,
 			return ret;
 		*val &= AD5791_DAC_MASK;
 		*val >>= chan->scan_type.shift;
-		*val -= (1 << (chan->scan_type.realbits - 1));
 		return IIO_VAL_INT;
 	case (1 << IIO_CHAN_INFO_SCALE_SHARED):
 		*val = 0;
 		*val2 = (st->vref_mv * 1000) >> chan->scan_type.realbits;
 		return IIO_VAL_INT_PLUS_MICRO;
+	case (1 << IIO_CHAN_INFO_OFFSET_SHARED):
+		val64 = (((u64)st->vref_neg_mv) << chan->scan_type.realbits);
+		do_div(val64, st->vref_mv);
+		*val = -val64;
+		return IIO_VAL_INT;
 	default:
 		return -EINVAL;
 	}
@@ -257,7 +263,6 @@ static int ad5791_write_raw(struct iio_dev *indio_dev,
 
 	switch (mask) {
 	case 0:
-		val += (1 << (chan->scan_type.realbits - 1));
 		val &= AD5791_RES_MASK(chan->scan_type.realbits);
 		val <<= chan->scan_type.shift;
 
@@ -309,12 +314,15 @@ static int __devinit ad5791_probe(struct spi_device *spi)
 	st->pwr_down = true;
 	st->spi = spi;
 
-	if (!IS_ERR(st->reg_vss) && !IS_ERR(st->reg_vdd))
-		st->vref_mv = (pos_voltage_uv - neg_voltage_uv) / 1000;
-	else if (pdata)
-		st->vref_mv = pdata->vref_pos_mv - pdata->vref_neg_mv;
-	else
+	if (!IS_ERR(st->reg_vss) && !IS_ERR(st->reg_vdd)) {
+		st->vref_mv = (pos_voltage_uv + neg_voltage_uv) / 1000;
+		st->vref_neg_mv = neg_voltage_uv / 1000;
+	} else if (pdata) {
+		st->vref_mv = pdata->vref_pos_mv + pdata->vref_neg_mv;
+		st->vref_neg_mv = pdata->vref_neg_mv;
+	} else {
 		dev_warn(&spi->dev, "reference voltage unspecified\n");
+	}
 
 	ret = ad5791_spi_write(spi, AD5791_ADDR_SW_CTRL, AD5791_SWCTRL_RESET);
 	if (ret)
diff --git a/drivers/staging/iio/dac/ad5791.h b/drivers/staging/iio/dac/ad5791.h
index 2a50a11..fd7edbd 100644
--- a/drivers/staging/iio/dac/ad5791.h
+++ b/drivers/staging/iio/dac/ad5791.h
@@ -82,6 +82,7 @@ struct ad5791_chip_info {
  * @reg_vss:		negative supply regulator
  * @chip_info:		chip model specific constants
  * @vref_mv:		actual reference voltage used
+ * @vref_neg_mv:	voltage of the negative supply
  * @pwr_down_mode	current power down mode
  */
 
@@ -91,6 +92,7 @@ struct ad5791_state {
 	struct regulator		*reg_vss;
 	const struct ad5791_chip_info	*chip_info;
 	unsigned short			vref_mv;
+	unsigned int			vref_neg_mv;
 	unsigned			ctrl;
 	unsigned			pwr_down_mode;
 	bool				pwr_down;
-- 
1.7.6.3

Re: [PATCH 2/4] staging:iio:dac:ad5791: Allow asymmetrical reference voltages

[Dan Carpenter]

On Wed, Oct 19, 2011 at 05:47:50PM +0200, Lars-Peter Clausen wrote:
> @@ -225,6 +226,7 @@ static int ad5791_read_raw(struct iio_dev *indio_dev,
>  			   long m)
>  {
>  	struct ad5791_state *st = iio_priv(indio_dev);
> +	u64 val64;
>  	int ret;
>  
>  	switch (m) {
> @@ -234,12 +236,16 @@ static int ad5791_read_raw(struct iio_dev *indio_dev,
>  			return ret;
>  		*val &= AD5791_DAC_MASK;
>  		*val >>= chan->scan_type.shift;
> -		*val -= (1 << (chan->scan_type.realbits - 1));
>  		return IIO_VAL_INT;
>  	case (1 << IIO_CHAN_INFO_SCALE_SHARED):
>  		*val = 0;
>  		*val2 = (st->vref_mv * 1000) >> chan->scan_type.realbits;
>  		return IIO_VAL_INT_PLUS_MICRO;
> +	case (1 << IIO_CHAN_INFO_OFFSET_SHARED):
> +		val64 = (((u64)st->vref_neg_mv) << chan->scan_type.realbits);
> +		do_div(val64, st->vref_mv);
> +		*val = -val64;
> +		return IIO_VAL_INT;

Why does iio use switch over a bitfield?  If the values are mutually
exclusive then why not just use an enum?

Hm...  "m" stands for mask and it gets created using the
IIO_UNMOD_EVENT_CODE() macro or sometimes it's just one bit at a
time copied from &chan->info_mask...  Odd.

regards,
dan carpenter

Re: [PATCH 2/4] staging:iio:dac:ad5791: Allow asymmetrical reference voltages

[Lars-Peter Clausen]

On 10/19/2011 11:04 PM, Dan Carpenter wrote:
> On Wed, Oct 19, 2011 at 05:47:50PM +0200, Lars-Peter Clausen wrote:
>> @@ -225,6 +226,7 @@ static int ad5791_read_raw(struct iio_dev *indio_dev,
>>  			   long m)
>>  {
>>  	struct ad5791_state *st = iio_priv(indio_dev);
>> +	u64 val64;
>>  	int ret;
>>  
>>  	switch (m) {
>> @@ -234,12 +236,16 @@ static int ad5791_read_raw(struct iio_dev *indio_dev,
>>  			return ret;
>>  		*val &= AD5791_DAC_MASK;
>>  		*val >>= chan->scan_type.shift;
>> -		*val -= (1 << (chan->scan_type.realbits - 1));
>>  		return IIO_VAL_INT;
>>  	case (1 << IIO_CHAN_INFO_SCALE_SHARED):
>>  		*val = 0;
>>  		*val2 = (st->vref_mv * 1000) >> chan->scan_type.realbits;
>>  		return IIO_VAL_INT_PLUS_MICRO;
>> +	case (1 << IIO_CHAN_INFO_OFFSET_SHARED):
>> +		val64 = (((u64)st->vref_neg_mv) << chan->scan_type.realbits);
>> +		do_div(val64, st->vref_mv);
>> +		*val = -val64;
>> +		return IIO_VAL_INT;
> 
> Why does iio use switch over a bitfield?  If the values are mutually
> exclusive then why not just use an enum?
> 

I wondered the very same and couldn't find a good explanation. I wanted to
write a small RFC today converting the core and maybe 2-3 drivers to use the
unshifted variants to see what Jonathan thinks about it.

> Hm...  "m" stands for mask and it gets created using the
> IIO_UNMOD_EVENT_CODE() macro or sometimes it's just one bit at a
> time copied from &chan->info_mask...  Odd.
> 

IIO_UNMOD_EVENT_CODE is only used for events which use a different callback
function, so for the read_raw/write_raw callbacks we currently only ever
pass one channel info type at a time.

- Lars

Re: [PATCH 2/4] staging:iio:dac:ad5791: Allow asymmetrical reference voltages

[Jonathan Cameron]

Lars-Peter Clausen <lars@metafoo.de> wrote:

>On 10/19/2011 11:04 PM, Dan Carpenter wrote:
>> On Wed, Oct 19, 2011 at 05:47:50PM +0200, Lars-Peter Clausen wrote:
>>> @@ -225,6 +226,7 @@ static int ad5791_read_raw(struct iio_dev
>*indio_dev,
>>>  			   long m)
>>>  {
>>>  	struct ad5791_state *st = iio_priv(indio_dev);
>>> +	u64 val64;
>>>  	int ret;
>>>  
>>>  	switch (m) {
>>> @@ -234,12 +236,16 @@ static int ad5791_read_raw(struct iio_dev
>*indio_dev,
>>>  			return ret;
>>>  		*val &= AD5791_DAC_MASK;
>>>  		*val >>= chan->scan_type.shift;
>>> -		*val -= (1 << (chan->scan_type.realbits - 1));
>>>  		return IIO_VAL_INT;
>>>  	case (1 << IIO_CHAN_INFO_SCALE_SHARED):
>>>  		*val = 0;
>>>  		*val2 = (st->vref_mv * 1000) >> chan->scan_type.realbits;
>>>  		return IIO_VAL_INT_PLUS_MICRO;
>>> +	case (1 << IIO_CHAN_INFO_OFFSET_SHARED):
>>> +		val64 = (((u64)st->vref_neg_mv) << chan->scan_type.realbits);
>>> +		do_div(val64, st->vref_mv);
>>> +		*val = -val64;
>>> +		return IIO_VAL_INT;
>> 
>> Why does iio use switch over a bitfield?  If the values are mutually
>> exclusive then why not just use an enum?
>> 
>
>I wondered the very same and couldn't find a good explanation. I wanted
>to
>write a small RFC today converting the core and maybe 2-3 drivers to
>use the
>unshifted variants to see what Jonathan thinks about it.
It is legacy cruft from a while back. So I would be happy to see it go.
>
>> Hm...  "m" stands for mask and it gets created using the
>> IIO_UNMOD_EVENT_CODE() macro or sometimes it's just one bit at a
>> time copied from &chan->info_mask...  Odd.
>> 
>
>IIO_UNMOD_EVENT_CODE is only used for events which use a different
>callback
>function, so for the read_raw/write_raw callbacks we currently only
>ever
>pass one channel info type at a time.
>
>- Lars
>--
>To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Sent from my Android phone 
with K-9 Mail. Please excuse my brevity.