diff mbox series

iio: addac: ad74413r: fix Current Input, Loop Powered Mode

Message ID 20230301115511.849418-1-linux@rasmusvillemoes.dk (mailing list archive)
State Accepted
Headers show
Series iio: addac: ad74413r: fix Current Input, Loop Powered Mode | expand

Commit Message

Rasmus Villemoes March 1, 2023, 11:55 a.m. UTC 
Currently, the driver handles CH_FUNC_CURRENT_INPUT_LOOP_POWER and
CH_FUNC_CURRENT_INPUT_EXT_POWER completely identically. But that's not
correct. In order for CH_FUNC_CURRENT_INPUT_LOOP_POWER to work, two
changes must be made:

(1) expose access to the DAC_CODE_x register so that the intended
output current can be set, i.e. expose the channel as both current
output and current input, and

(2) per the data sheet

  When selecting the current input loop powered function, tie the
  VIOUTN_x pin to ground via the on-chip 200 kΩ resistor by enabling
  the CH_200K_TO_GND bit in the ADC_CONFIGx registers.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
There's of course also CH_FUNC_CURRENT_INPUT_LOOP_POWER_HART which is
likely to require a similar fix, but as I don't have a ad74413r I
can't test that. 

 drivers/iio/addac/ad74413r.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

Comments

Jonathan Cameron March 4, 2023, 5:15 p.m. UTC | #1 
On Wed,  1 Mar 2023 12:55:11 +0100
Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:

> Currently, the driver handles CH_FUNC_CURRENT_INPUT_LOOP_POWER and
> CH_FUNC_CURRENT_INPUT_EXT_POWER completely identically. But that's not
> correct. In order for CH_FUNC_CURRENT_INPUT_LOOP_POWER to work, two
> changes must be made:
> 
> (1) expose access to the DAC_CODE_x register so that the intended
> output current can be set, i.e. expose the channel as both current
> output and current input, and
> 
> (2) per the data sheet
> 
>   When selecting the current input loop powered function, tie the
>   VIOUTN_x pin to ground via the on-chip 200 kΩ resistor by enabling
>   the CH_200K_TO_GND bit in the ADC_CONFIGx registers.
> 
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
> There's of course also CH_FUNC_CURRENT_INPUT_LOOP_POWER_HART which is
> likely to require a similar fix, but as I don't have a ad74413r I
> can't test that. 

Looks correct to me based on the "current input, loop powered" section
of the datasheet, but want to leave a little more time for someone
familiar with the part to comment.

Given me a poke if I seem to have lost this in 2 weeks.  (That happens
less now I track with patchwork, but you never know ;)

Thanks,

Jonathan

> 
>  drivers/iio/addac/ad74413r.c | 21 +++++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/addac/ad74413r.c b/drivers/iio/addac/ad74413r.c
> index f32c8c2fb26d..f5d072092709 100644
> --- a/drivers/iio/addac/ad74413r.c
> +++ b/drivers/iio/addac/ad74413r.c
> @@ -99,6 +99,7 @@ struct ad74413r_state {
>  #define AD74413R_REG_ADC_CONFIG_X(x)		(0x05 + (x))
>  #define AD74413R_ADC_CONFIG_RANGE_MASK		GENMASK(7, 5)
>  #define AD74413R_ADC_CONFIG_REJECTION_MASK	GENMASK(4, 3)
> +#define AD74413R_ADC_CONFIG_CH_200K_TO_GND	BIT(2)
>  #define AD74413R_ADC_RANGE_10V			0b000
>  #define AD74413R_ADC_RANGE_2P5V_EXT_POW		0b001
>  #define AD74413R_ADC_RANGE_2P5V_INT_POW		0b010
> @@ -424,9 +425,20 @@ static int ad74413r_set_channel_dac_code(struct ad74413r_state *st,
>  static int ad74413r_set_channel_function(struct ad74413r_state *st,
>  					 unsigned int channel, u8 func)
>  {
> -	return regmap_update_bits(st->regmap,
> +	int ret;
> +
> +	ret = regmap_update_bits(st->regmap,
>  				  AD74413R_REG_CH_FUNC_SETUP_X(channel),
>  				  AD74413R_CH_FUNC_SETUP_MASK, func);
> +	if (ret)
> +		return ret;
> +
> +	if (func == CH_FUNC_CURRENT_INPUT_LOOP_POWER)
> +		ret = regmap_set_bits(st->regmap,
> +				      AD74413R_REG_ADC_CONFIG_X(channel),
> +				      AD74413R_ADC_CONFIG_CH_200K_TO_GND);
> +
> +	return ret;
>  }
>  
>  static int ad74413r_set_adc_conv_seq(struct ad74413r_state *st,
> @@ -1112,6 +1124,11 @@ static struct iio_chan_spec ad74413r_current_input_channels[] = {
>  	AD74413R_ADC_CURRENT_CHANNEL,
>  };
>  
> +static struct iio_chan_spec ad74413r_current_input_loop_channels[] = {
> +	AD74413R_DAC_CHANNEL(IIO_CURRENT, BIT(IIO_CHAN_INFO_SCALE)),
> +	AD74413R_ADC_CURRENT_CHANNEL,
> +};
> +
>  static struct iio_chan_spec ad74413r_resistance_input_channels[] = {
>  	AD74413R_ADC_CHANNEL(IIO_RESISTANCE, BIT(IIO_CHAN_INFO_PROCESSED)),
>  };
> @@ -1135,7 +1152,7 @@ static const struct ad74413r_channels ad74413r_channels_map[] = {
>  	[CH_FUNC_CURRENT_OUTPUT] = AD74413R_CHANNELS(current_output),
>  	[CH_FUNC_VOLTAGE_INPUT] = AD74413R_CHANNELS(voltage_input),
>  	[CH_FUNC_CURRENT_INPUT_EXT_POWER] = AD74413R_CHANNELS(current_input),
> -	[CH_FUNC_CURRENT_INPUT_LOOP_POWER] = AD74413R_CHANNELS(current_input),
> +	[CH_FUNC_CURRENT_INPUT_LOOP_POWER] = AD74413R_CHANNELS(current_input_loop),
>  	[CH_FUNC_RESISTANCE_INPUT] = AD74413R_CHANNELS(resistance_input),
>  	[CH_FUNC_DIGITAL_INPUT_LOGIC] = AD74413R_CHANNELS(digital_input),
>  	[CH_FUNC_DIGITAL_INPUT_LOOP_POWER] = AD74413R_CHANNELS(digital_input),
Jonathan Cameron March 12, 2023, 3:50 p.m. UTC | #2 
On Sat, 4 Mar 2023 17:15:42 +0000
Jonathan Cameron <jic23@kernel.org> wrote:

> On Wed,  1 Mar 2023 12:55:11 +0100
> Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
> 
> > Currently, the driver handles CH_FUNC_CURRENT_INPUT_LOOP_POWER and
> > CH_FUNC_CURRENT_INPUT_EXT_POWER completely identically. But that's not
> > correct. In order for CH_FUNC_CURRENT_INPUT_LOOP_POWER to work, two
> > changes must be made:
> > 
> > (1) expose access to the DAC_CODE_x register so that the intended
> > output current can be set, i.e. expose the channel as both current
> > output and current input, and
> > 
> > (2) per the data sheet
> > 
> >   When selecting the current input loop powered function, tie the
> >   VIOUTN_x pin to ground via the on-chip 200 kΩ resistor by enabling
> >   the CH_200K_TO_GND bit in the ADC_CONFIGx registers.
> > 
> > Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> > ---
> > There's of course also CH_FUNC_CURRENT_INPUT_LOOP_POWER_HART which is
> > likely to require a similar fix, but as I don't have a ad74413r I
> > can't test that.   
> 
> Looks correct to me based on the "current input, loop powered" section
> of the datasheet, but want to leave a little more time for someone
> familiar with the part to comment.
> 
> Given me a poke if I seem to have lost this in 2 weeks.  (That happens
> less now I track with patchwork, but you never know ;)
> 
> Thanks,
> 
> Jonathan
Applied, Thanks

Jonathan

> 
> > 
> >  drivers/iio/addac/ad74413r.c | 21 +++++++++++++++++++--
> >  1 file changed, 19 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/iio/addac/ad74413r.c b/drivers/iio/addac/ad74413r.c
> > index f32c8c2fb26d..f5d072092709 100644
> > --- a/drivers/iio/addac/ad74413r.c
> > +++ b/drivers/iio/addac/ad74413r.c
> > @@ -99,6 +99,7 @@ struct ad74413r_state {
> >  #define AD74413R_REG_ADC_CONFIG_X(x)		(0x05 + (x))
> >  #define AD74413R_ADC_CONFIG_RANGE_MASK		GENMASK(7, 5)
> >  #define AD74413R_ADC_CONFIG_REJECTION_MASK	GENMASK(4, 3)
> > +#define AD74413R_ADC_CONFIG_CH_200K_TO_GND	BIT(2)
> >  #define AD74413R_ADC_RANGE_10V			0b000
> >  #define AD74413R_ADC_RANGE_2P5V_EXT_POW		0b001
> >  #define AD74413R_ADC_RANGE_2P5V_INT_POW		0b010
> > @@ -424,9 +425,20 @@ static int ad74413r_set_channel_dac_code(struct ad74413r_state *st,
> >  static int ad74413r_set_channel_function(struct ad74413r_state *st,
> >  					 unsigned int channel, u8 func)
> >  {
> > -	return regmap_update_bits(st->regmap,
> > +	int ret;
> > +
> > +	ret = regmap_update_bits(st->regmap,
> >  				  AD74413R_REG_CH_FUNC_SETUP_X(channel),
> >  				  AD74413R_CH_FUNC_SETUP_MASK, func);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (func == CH_FUNC_CURRENT_INPUT_LOOP_POWER)
> > +		ret = regmap_set_bits(st->regmap,
> > +				      AD74413R_REG_ADC_CONFIG_X(channel),
> > +				      AD74413R_ADC_CONFIG_CH_200K_TO_GND);
> > +
> > +	return ret;
> >  }
> >  
> >  static int ad74413r_set_adc_conv_seq(struct ad74413r_state *st,
> > @@ -1112,6 +1124,11 @@ static struct iio_chan_spec ad74413r_current_input_channels[] = {
> >  	AD74413R_ADC_CURRENT_CHANNEL,
> >  };
> >  
> > +static struct iio_chan_spec ad74413r_current_input_loop_channels[] = {
> > +	AD74413R_DAC_CHANNEL(IIO_CURRENT, BIT(IIO_CHAN_INFO_SCALE)),
> > +	AD74413R_ADC_CURRENT_CHANNEL,
> > +};
> > +
> >  static struct iio_chan_spec ad74413r_resistance_input_channels[] = {
> >  	AD74413R_ADC_CHANNEL(IIO_RESISTANCE, BIT(IIO_CHAN_INFO_PROCESSED)),
> >  };
> > @@ -1135,7 +1152,7 @@ static const struct ad74413r_channels ad74413r_channels_map[] = {
> >  	[CH_FUNC_CURRENT_OUTPUT] = AD74413R_CHANNELS(current_output),
> >  	[CH_FUNC_VOLTAGE_INPUT] = AD74413R_CHANNELS(voltage_input),
> >  	[CH_FUNC_CURRENT_INPUT_EXT_POWER] = AD74413R_CHANNELS(current_input),
> > -	[CH_FUNC_CURRENT_INPUT_LOOP_POWER] = AD74413R_CHANNELS(current_input),
> > +	[CH_FUNC_CURRENT_INPUT_LOOP_POWER] = AD74413R_CHANNELS(current_input_loop),
> >  	[CH_FUNC_RESISTANCE_INPUT] = AD74413R_CHANNELS(resistance_input),
> >  	[CH_FUNC_DIGITAL_INPUT_LOGIC] = AD74413R_CHANNELS(digital_input),
> >  	[CH_FUNC_DIGITAL_INPUT_LOOP_POWER] = AD74413R_CHANNELS(digital_input),  
>
diff mbox series

Patch

diff --git a/drivers/iio/addac/ad74413r.c b/drivers/iio/addac/ad74413r.c
index f32c8c2fb26d..f5d072092709 100644
--- a/drivers/iio/addac/ad74413r.c
+++ b/drivers/iio/addac/ad74413r.c
@@ -99,6 +99,7 @@  struct ad74413r_state {
 #define AD74413R_REG_ADC_CONFIG_X(x)		(0x05 + (x))
 #define AD74413R_ADC_CONFIG_RANGE_MASK		GENMASK(7, 5)
 #define AD74413R_ADC_CONFIG_REJECTION_MASK	GENMASK(4, 3)
+#define AD74413R_ADC_CONFIG_CH_200K_TO_GND	BIT(2)
 #define AD74413R_ADC_RANGE_10V			0b000
 #define AD74413R_ADC_RANGE_2P5V_EXT_POW		0b001
 #define AD74413R_ADC_RANGE_2P5V_INT_POW		0b010
@@ -424,9 +425,20 @@  static int ad74413r_set_channel_dac_code(struct ad74413r_state *st,
 static int ad74413r_set_channel_function(struct ad74413r_state *st,
 					 unsigned int channel, u8 func)
 {
-	return regmap_update_bits(st->regmap,
+	int ret;
+
+	ret = regmap_update_bits(st->regmap,
 				  AD74413R_REG_CH_FUNC_SETUP_X(channel),
 				  AD74413R_CH_FUNC_SETUP_MASK, func);
+	if (ret)
+		return ret;
+
+	if (func == CH_FUNC_CURRENT_INPUT_LOOP_POWER)
+		ret = regmap_set_bits(st->regmap,
+				      AD74413R_REG_ADC_CONFIG_X(channel),
+				      AD74413R_ADC_CONFIG_CH_200K_TO_GND);
+
+	return ret;
 }
 
 static int ad74413r_set_adc_conv_seq(struct ad74413r_state *st,
@@ -1112,6 +1124,11 @@  static struct iio_chan_spec ad74413r_current_input_channels[] = {
 	AD74413R_ADC_CURRENT_CHANNEL,
 };
 
+static struct iio_chan_spec ad74413r_current_input_loop_channels[] = {
+	AD74413R_DAC_CHANNEL(IIO_CURRENT, BIT(IIO_CHAN_INFO_SCALE)),
+	AD74413R_ADC_CURRENT_CHANNEL,
+};
+
 static struct iio_chan_spec ad74413r_resistance_input_channels[] = {
 	AD74413R_ADC_CHANNEL(IIO_RESISTANCE, BIT(IIO_CHAN_INFO_PROCESSED)),
 };
@@ -1135,7 +1152,7 @@  static const struct ad74413r_channels ad74413r_channels_map[] = {
 	[CH_FUNC_CURRENT_OUTPUT] = AD74413R_CHANNELS(current_output),
 	[CH_FUNC_VOLTAGE_INPUT] = AD74413R_CHANNELS(voltage_input),
 	[CH_FUNC_CURRENT_INPUT_EXT_POWER] = AD74413R_CHANNELS(current_input),
-	[CH_FUNC_CURRENT_INPUT_LOOP_POWER] = AD74413R_CHANNELS(current_input),
+	[CH_FUNC_CURRENT_INPUT_LOOP_POWER] = AD74413R_CHANNELS(current_input_loop),
 	[CH_FUNC_RESISTANCE_INPUT] = AD74413R_CHANNELS(resistance_input),
 	[CH_FUNC_DIGITAL_INPUT_LOGIC] = AD74413R_CHANNELS(digital_input),
 	[CH_FUNC_DIGITAL_INPUT_LOOP_POWER] = AD74413R_CHANNELS(digital_input),