[v3,22/27] staging: iio: resolver: ad2s1210: convert LOS threshold to event attr

Commit Message

David Lechner Sept. 29, 2023, 5:23 p.m. UTC

From: David Lechner <[e-mail]>

From: David Lechner <[e-mail]>

The AD2S1210 has a programmable threshold for the loss of signal (LOS)
fault. This fault is triggered when either the sine or cosine input
falls below the threshold voltage.

This patch converts the custom device LOS threshold attribute to an
event falling edge threshold attribute on a new monitor signal channel.
The monitor signal is an internal signal that combines the amplitudes
of the sine and cosine inputs as well as the current angle and position
output. This signal is used to detect faults in the input signals.

The attribute now uses millivolts instead of the raw register value in
accordance with the IIO ABI.

Emitting the event will be implemented in a later patch.

Signed-off-by: David Lechner <[e-mail]>
---

v3 changes: This is a new patch in v3

 drivers/staging/iio/resolver/ad2s1210.c | 75 +++++++++++++++++++++++++++++++--
 1 file changed, 71 insertions(+), 4 deletions(-)

Comments

Jonathan Cameron Sept. 30, 2023, 3:32 p.m. UTC | #1

On Fri, 29 Sep 2023 12:23:27 -0500
David Lechner <[e-mail]> wrote:

> From: David Lechner <[e-mail]>
> 
> From: David Lechner <[e-mail]>
> 
> The AD2S1210 has a programmable threshold for the loss of signal (LOS)
> fault. This fault is triggered when either the sine or cosine input
> falls below the threshold voltage.
> 
> This patch converts the custom device LOS threshold attribute to an
> event falling edge threshold attribute on a new monitor signal channel.
> The monitor signal is an internal signal that combines the amplitudes
> of the sine and cosine inputs as well as the current angle and position
> output. This signal is used to detect faults in the input signals.
> 
> The attribute now uses millivolts instead of the raw register value in
> accordance with the IIO ABI.
> 
> Emitting the event will be implemented in a later patch.
> 
> Signed-off-by: David Lechner <[e-mail]>

I think I'm fine with treating these internal signals like this, but
I would ideally like someone from Analog devices to take a look at how
these are being done and make sure our interpretations of the signals
make sense to them.  We are pushing the boundaries a little here (though
we have done similar before for fault events I think.)

Jonathan

Jonathan Cameron Sept. 30, 2023, 3:42 p.m. UTC | #2

On Fri, 29 Sep 2023 12:23:27 -0500
David Lechner <[e-mail]> wrote:

> From: David Lechner <[e-mail]>
> 
> From: David Lechner <[e-mail]>
> 
> The AD2S1210 has a programmable threshold for the loss of signal (LOS)
> fault. This fault is triggered when either the sine or cosine input
> falls below the threshold voltage.
> 
> This patch converts the custom device LOS threshold attribute to an
> event falling edge threshold attribute on a new monitor signal channel.
> The monitor signal is an internal signal that combines the amplitudes
> of the sine and cosine inputs as well as the current angle and position
> output. This signal is used to detect faults in the input signals.

Hmm. Looking forwards, I'm less sure that we should be shoving all these
error conditions onto one channel. Fundamentally we have
sine and cosine inputs. I think we should treat those as separate channels
and include a third differential channel between them.

So this one becomes a double event (you need to signal it on both
cosine and sine channels).  The DOS overange is similar.
The DOS mismatch is a threshold on the differential channel giving

events/in_altvoltage0_thresh_falling_value
events/in_altvoltage1_thresh_falling_value (these match)
events/in_altvoltage0_thresh_rising_value
events/in_altvoltage1_thresh_rising_value (matches previous which is fine)
events/in_altvoltage1-0_mag_rising_value

Does that work here?  Avoids smashing different types of signals together.
We could even do the LOT as differential between two angle channels
(tracking one and measured one) but meh that's getting complex.

Note this will rely on channel labels to make the above make any sense at all.

Jonathan


> 
> The attribute now uses millivolts instead of the raw register value in
> accordance with the IIO ABI.
> 
> Emitting the event will be implemented in a later patch.
> 
> Signed-off-by: David Lechner <[e-mail]>
> ---
> 
> v3 changes: This is a new patch in v3
> 
>  drivers/staging/iio/resolver/ad2s1210.c | 75 +++++++++++++++++++++++++++++++--
>  1 file changed, 71 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
> index 5cc8106800d6..7abbc184c351 100644
> --- a/drivers/staging/iio/resolver/ad2s1210.c
> +++ b/drivers/staging/iio/resolver/ad2s1210.c
> @@ -66,6 +66,11 @@
>  #define PHASE_360_DEG_TO_RAD_INT 6
>  #define PHASE_360_DEG_TO_RAD_MICRO 283185
>  
> +/* Threshold voltage registers have 1 LSB == 38 mV */
> +#define THRESHOLD_MILLIVOLT_PER_LSB 38
> +/* max voltage for threshold registers is 0x7F * 38 mV */
> +#define THRESHOLD_RANGE_STR "[0 38 4826]"
> +
>  enum ad2s1210_mode {
>  	MOD_POS = 0b00,
>  	MOD_VEL = 0b01,
> @@ -448,6 +453,38 @@ static const int ad2s1210_lot_threshold_urad_per_lsb[] = {
>  	1237, /* 16-bit: same as 14-bit */
>  };
>  
> +static int ad2s1210_get_voltage_threshold(struct ad2s1210_state *st,
> +					  unsigned int reg, int *val)
> +{
> +	unsigned int reg_val;
> +	int ret;
> +
> +	mutex_lock(&st->lock);
> +	ret = regmap_read(st->regmap, reg, &reg_val);
> +	mutex_unlock(&st->lock);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	*val = reg_val * THRESHOLD_MILLIVOLT_PER_LSB;
> +	return IIO_VAL_INT;
> +}
> +
> +static int ad2s1210_set_voltage_threshold(struct ad2s1210_state *st,
> +					  unsigned int reg, int val)
> +{
> +	unsigned int reg_val;
> +	int ret;
> +
> +	reg_val = val / THRESHOLD_MILLIVOLT_PER_LSB;
> +
> +	mutex_lock(&st->lock);
> +	ret = regmap_write(st->regmap, reg, reg_val);
> +	mutex_unlock(&st->lock);
> +
> +	return ret;
> +}
> +
>  static int ad2s1210_get_lot_high_threshold(struct ad2s1210_state *st,
>  					   int *val, int *val2)
>  {
> @@ -706,9 +743,6 @@ static int ad2s1210_write_raw(struct iio_dev *indio_dev,
>  static IIO_DEVICE_ATTR(fault, 0644,
>  		       ad2s1210_show_fault, ad2s1210_clear_fault, 0);
>  
> -static IIO_DEVICE_ATTR(los_thrd, 0644,
> -		       ad2s1210_show_reg, ad2s1210_store_reg,
> -		       AD2S1210_REG_LOS_THRD);
>  static IIO_DEVICE_ATTR(dos_ovr_thrd, 0644,
>  		       ad2s1210_show_reg, ad2s1210_store_reg,
>  		       AD2S1210_REG_DOS_OVR_THRD);
> @@ -745,6 +779,16 @@ static const struct iio_event_spec ad2s1210_phase_event_spec[] = {
>  	},
>  };
>  
> +static const struct iio_event_spec ad2s1210_monitor_signal_event_spec[] = {
> +	{
> +		/* Sine/cosine below LOS threshold fault. */
> +		.type = IIO_EV_TYPE_THRESH,
> +		.dir = IIO_EV_DIR_FALLING,
> +		/* Loss of signal threshold. */
> +		.mask_separate = BIT(IIO_EV_INFO_VALUE),
> +	},
> +};
> +
>  static const struct iio_chan_spec ad2s1210_channels[] = {
>  	{
>  		.type = IIO_ANGL,
> @@ -803,12 +847,19 @@ static const struct iio_chan_spec ad2s1210_channels[] = {
>  		.scan_index = -1,
>  		.info_mask_separate = BIT(IIO_CHAN_INFO_FREQUENCY),
>  		.info_mask_separate_available = BIT(IIO_CHAN_INFO_FREQUENCY),
> +	}, {
> +		/* monitor signal */
> +		.type = IIO_ALTVOLTAGE,
> +		.indexed = 1,
> +		.channel = 0,
> +		.scan_index = -1,
> +		.event_spec = ad2s1210_monitor_signal_event_spec,
> +		.num_event_specs = ARRAY_SIZE(ad2s1210_monitor_signal_event_spec),
>  	},
>  };
>  
>  static struct attribute *ad2s1210_attributes[] = {
>  	&iio_dev_attr_fault.dev_attr.attr,
> -	&iio_dev_attr_los_thrd.dev_attr.attr,
>  	&iio_dev_attr_dos_ovr_thrd.dev_attr.attr,
>  	&iio_dev_attr_dos_mis_thrd.dev_attr.attr,
>  	&iio_dev_attr_dos_rst_max_thrd.dev_attr.attr,
> @@ -847,11 +898,13 @@ IIO_CONST_ATTR(in_phase0_mag_value_available,
>  	       __stringify(PHASE_44_DEG_TO_RAD_MICRO) " "
>  	       __stringify(PHASE_360_DEG_TO_RAD_INT) "."
>  	       __stringify(PHASE_360_DEG_TO_RAD_MICRO));
> +IIO_CONST_ATTR(in_altvoltage0_thresh_falling_value_available, THRESHOLD_RANGE_STR);
>  IIO_DEVICE_ATTR_RO(in_angl1_thresh_rising_value_available, 0);
>  IIO_DEVICE_ATTR_RO(in_angl1_thresh_rising_hysteresis_available, 0);
>  
>  static struct attribute *ad2s1210_event_attributes[] = {
>  	&iio_const_attr_in_phase0_mag_value_available.dev_attr.attr,
> +	&iio_const_attr_in_altvoltage0_thresh_falling_value_available.dev_attr.attr,
>  	&iio_dev_attr_in_angl1_thresh_rising_value_available.dev_attr.attr,
>  	&iio_dev_attr_in_angl1_thresh_rising_hysteresis_available.dev_attr.attr,
>  	NULL,
> @@ -904,6 +957,13 @@ static int ad2s1210_read_event_value(struct iio_dev *indio_dev,
>  		default:
>  			return -EINVAL;
>  		}
> +	case IIO_ALTVOLTAGE:
> +		if (chan->output)
> +			return -EINVAL;
> +		if (type == IIO_EV_TYPE_THRESH && dir == IIO_EV_DIR_FALLING)
> +			return ad2s1210_get_voltage_threshold(st,
> +						AD2S1210_REG_LOS_THRD, val);
> +		return -EINVAL;
>  	case IIO_PHASE:
>  		return ad2s1210_get_phase_lock_range(st, val, val2);
>  	default:
> @@ -930,6 +990,13 @@ static int ad2s1210_write_event_value(struct iio_dev *indio_dev,
>  		default:
>  			return -EINVAL;
>  		}
> +	case IIO_ALTVOLTAGE:
> +		if (chan->output)
> +			return -EINVAL;
> +		if (type == IIO_EV_TYPE_THRESH && dir == IIO_EV_DIR_FALLING)
> +			return ad2s1210_set_voltage_threshold(st,
> +						AD2S1210_REG_LOS_THRD, val);
> +		return -EINVAL;
>  	case IIO_PHASE:
>  		return ad2s1210_set_phase_lock_range(st, val, val2);
>  	default:
>

Jonathan Cameron Sept. 30, 2023, 3:46 p.m. UTC | #3

On Sat, 30 Sep 2023 16:42:51 +0100
Jonathan Cameron <[e-mail]> wrote:

> On Fri, 29 Sep 2023 12:23:27 -0500
> David Lechner <[e-mail]> wrote:
> 
> > From: David Lechner <[e-mail]>
> > 
> > From: David Lechner <[e-mail]>
> > 
> > The AD2S1210 has a programmable threshold for the loss of signal (LOS)
> > fault. This fault is triggered when either the sine or cosine input
> > falls below the threshold voltage.
> > 
> > This patch converts the custom device LOS threshold attribute to an
> > event falling edge threshold attribute on a new monitor signal channel.
> > The monitor signal is an internal signal that combines the amplitudes
> > of the sine and cosine inputs as well as the current angle and position
> > output. This signal is used to detect faults in the input signals.  
> 
> Hmm. Looking forwards, I'm less sure that we should be shoving all these
> error conditions onto one channel. Fundamentally we have
> sine and cosine inputs. I think we should treat those as separate channels
> and include a third differential channel between them.
> 
> So this one becomes a double event (you need to signal it on both
> cosine and sine channels).  The DOS overange is similar.
> The DOS mismatch is a threshold on the differential channel giving
> 
> events/in_altvoltage0_thresh_falling_value
> events/in_altvoltage1_thresh_falling_value (these match)
> events/in_altvoltage0_thresh_rising_value
> events/in_altvoltage1_thresh_rising_value (matches previous which is fine)
> events/in_altvoltage1-0_mag_rising_value

Sorry, got the syntax wrong :( Should have checked the ABI docs.

events/in_altvoltage1-altvoltage0_mag_rising_value

> 
> Does that work here?  Avoids smashing different types of signals together.
> We could even do the LOT as differential between two angle channels
> (tracking one and measured one) but meh that's getting complex.
> 
> Note this will rely on channel labels to make the above make any sense at all.
> 
> Jonathan
> 
> 
> > 
> > The attribute now uses millivolts instead of the raw register value in
> > accordance with the IIO ABI.
> > 
> > Emitting the event will be implemented in a later patch.
> > 
> > Signed-off-by: David Lechner <[e-mail]>
> > ---
> > 
> > v3 changes: This is a new patch in v3
> > 
> >  drivers/staging/iio/resolver/ad2s1210.c | 75 +++++++++++++++++++++++++++++++--
> >  1 file changed, 71 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
> > index 5cc8106800d6..7abbc184c351 100644
> > --- a/drivers/staging/iio/resolver/ad2s1210.c
> > +++ b/drivers/staging/iio/resolver/ad2s1210.c
> > @@ -66,6 +66,11 @@
> >  #define PHASE_360_DEG_TO_RAD_INT 6
> >  #define PHASE_360_DEG_TO_RAD_MICRO 283185
> >  
> > +/* Threshold voltage registers have 1 LSB == 38 mV */
> > +#define THRESHOLD_MILLIVOLT_PER_LSB 38
> > +/* max voltage for threshold registers is 0x7F * 38 mV */
> > +#define THRESHOLD_RANGE_STR "[0 38 4826]"
> > +
> >  enum ad2s1210_mode {
> >  	MOD_POS = 0b00,
> >  	MOD_VEL = 0b01,
> > @@ -448,6 +453,38 @@ static const int ad2s1210_lot_threshold_urad_per_lsb[] = {
> >  	1237, /* 16-bit: same as 14-bit */
> >  };
> >  
> > +static int ad2s1210_get_voltage_threshold(struct ad2s1210_state *st,
> > +					  unsigned int reg, int *val)
> > +{
> > +	unsigned int reg_val;
> > +	int ret;
> > +
> > +	mutex_lock(&st->lock);
> > +	ret = regmap_read(st->regmap, reg, &reg_val);
> > +	mutex_unlock(&st->lock);
> > +
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	*val = reg_val * THRESHOLD_MILLIVOLT_PER_LSB;
> > +	return IIO_VAL_INT;
> > +}
> > +
> > +static int ad2s1210_set_voltage_threshold(struct ad2s1210_state *st,
> > +					  unsigned int reg, int val)
> > +{
> > +	unsigned int reg_val;
> > +	int ret;
> > +
> > +	reg_val = val / THRESHOLD_MILLIVOLT_PER_LSB;
> > +
> > +	mutex_lock(&st->lock);
> > +	ret = regmap_write(st->regmap, reg, reg_val);
> > +	mutex_unlock(&st->lock);
> > +
> > +	return ret;
> > +}
> > +
> >  static int ad2s1210_get_lot_high_threshold(struct ad2s1210_state *st,
> >  					   int *val, int *val2)
> >  {
> > @@ -706,9 +743,6 @@ static int ad2s1210_write_raw(struct iio_dev *indio_dev,
> >  static IIO_DEVICE_ATTR(fault, 0644,
> >  		       ad2s1210_show_fault, ad2s1210_clear_fault, 0);
> >  
> > -static IIO_DEVICE_ATTR(los_thrd, 0644,
> > -		       ad2s1210_show_reg, ad2s1210_store_reg,
> > -		       AD2S1210_REG_LOS_THRD);
> >  static IIO_DEVICE_ATTR(dos_ovr_thrd, 0644,
> >  		       ad2s1210_show_reg, ad2s1210_store_reg,
> >  		       AD2S1210_REG_DOS_OVR_THRD);
> > @@ -745,6 +779,16 @@ static const struct iio_event_spec ad2s1210_phase_event_spec[] = {
> >  	},
> >  };
> >  
> > +static const struct iio_event_spec ad2s1210_monitor_signal_event_spec[] = {
> > +	{
> > +		/* Sine/cosine below LOS threshold fault. */
> > +		.type = IIO_EV_TYPE_THRESH,
> > +		.dir = IIO_EV_DIR_FALLING,
> > +		/* Loss of signal threshold. */
> > +		.mask_separate = BIT(IIO_EV_INFO_VALUE),
> > +	},
> > +};
> > +
> >  static const struct iio_chan_spec ad2s1210_channels[] = {
> >  	{
> >  		.type = IIO_ANGL,
> > @@ -803,12 +847,19 @@ static const struct iio_chan_spec ad2s1210_channels[] = {
> >  		.scan_index = -1,
> >  		.info_mask_separate = BIT(IIO_CHAN_INFO_FREQUENCY),
> >  		.info_mask_separate_available = BIT(IIO_CHAN_INFO_FREQUENCY),
> > +	}, {
> > +		/* monitor signal */
> > +		.type = IIO_ALTVOLTAGE,
> > +		.indexed = 1,
> > +		.channel = 0,
> > +		.scan_index = -1,
> > +		.event_spec = ad2s1210_monitor_signal_event_spec,
> > +		.num_event_specs = ARRAY_SIZE(ad2s1210_monitor_signal_event_spec),
> >  	},
> >  };
> >  
> >  static struct attribute *ad2s1210_attributes[] = {
> >  	&iio_dev_attr_fault.dev_attr.attr,
> > -	&iio_dev_attr_los_thrd.dev_attr.attr,
> >  	&iio_dev_attr_dos_ovr_thrd.dev_attr.attr,
> >  	&iio_dev_attr_dos_mis_thrd.dev_attr.attr,
> >  	&iio_dev_attr_dos_rst_max_thrd.dev_attr.attr,
> > @@ -847,11 +898,13 @@ IIO_CONST_ATTR(in_phase0_mag_value_available,
> >  	       __stringify(PHASE_44_DEG_TO_RAD_MICRO) " "
> >  	       __stringify(PHASE_360_DEG_TO_RAD_INT) "."
> >  	       __stringify(PHASE_360_DEG_TO_RAD_MICRO));
> > +IIO_CONST_ATTR(in_altvoltage0_thresh_falling_value_available, THRESHOLD_RANGE_STR);
> >  IIO_DEVICE_ATTR_RO(in_angl1_thresh_rising_value_available, 0);
> >  IIO_DEVICE_ATTR_RO(in_angl1_thresh_rising_hysteresis_available, 0);
> >  
> >  static struct attribute *ad2s1210_event_attributes[] = {
> >  	&iio_const_attr_in_phase0_mag_value_available.dev_attr.attr,
> > +	&iio_const_attr_in_altvoltage0_thresh_falling_value_available.dev_attr.attr,
> >  	&iio_dev_attr_in_angl1_thresh_rising_value_available.dev_attr.attr,
> >  	&iio_dev_attr_in_angl1_thresh_rising_hysteresis_available.dev_attr.attr,
> >  	NULL,
> > @@ -904,6 +957,13 @@ static int ad2s1210_read_event_value(struct iio_dev *indio_dev,
> >  		default:
> >  			return -EINVAL;
> >  		}
> > +	case IIO_ALTVOLTAGE:
> > +		if (chan->output)
> > +			return -EINVAL;
> > +		if (type == IIO_EV_TYPE_THRESH && dir == IIO_EV_DIR_FALLING)
> > +			return ad2s1210_get_voltage_threshold(st,
> > +						AD2S1210_REG_LOS_THRD, val);
> > +		return -EINVAL;
> >  	case IIO_PHASE:
> >  		return ad2s1210_get_phase_lock_range(st, val, val2);
> >  	default:
> > @@ -930,6 +990,13 @@ static int ad2s1210_write_event_value(struct iio_dev *indio_dev,
> >  		default:
> >  			return -EINVAL;
> >  		}
> > +	case IIO_ALTVOLTAGE:
> > +		if (chan->output)
> > +			return -EINVAL;
> > +		if (type == IIO_EV_TYPE_THRESH && dir == IIO_EV_DIR_FALLING)
> > +			return ad2s1210_set_voltage_threshold(st,
> > +						AD2S1210_REG_LOS_THRD, val);
> > +		return -EINVAL;
> >  	case IIO_PHASE:
> >  		return ad2s1210_set_phase_lock_range(st, val, val2);
> >  	default:
> >   
>

David Lechner Oct. 2, 2023, 4:09 p.m. UTC | #4

On Sat, Sep 30, 2023 at 10:42 AM Jonathan Cameron <[e-mail]> wrote:
>
> On Fri, 29 Sep 2023 12:23:27 -0500
> David Lechner <[e-mail]> wrote:
>
> > From: David Lechner <[e-mail]>
> >
> > From: David Lechner <[e-mail]>
> >
> > The AD2S1210 has a programmable threshold for the loss of signal (LOS)
> > fault. This fault is triggered when either the sine or cosine input
> > falls below the threshold voltage.
> >
> > This patch converts the custom device LOS threshold attribute to an
> > event falling edge threshold attribute on a new monitor signal channel.
> > The monitor signal is an internal signal that combines the amplitudes
> > of the sine and cosine inputs as well as the current angle and position
> > output. This signal is used to detect faults in the input signals.
>
> Hmm. Looking forwards, I'm less sure that we should be shoving all these
> error conditions onto one channel. Fundamentally we have
> sine and cosine inputs. I think we should treat those as separate channels
> and include a third differential channel between them.

At first, I did consider a differential channel as you suggested in
v2. However, the datasheet is quite clear that the LOS and DOS faults
(and only those faults) come from a signal it calls the "monitor
signal". This signal is defined as:

    Monitor = A1 * sin(theta)  * sin(phi) + A2 * cos(theta) * cos(phi)

where A1 * sin(theta) is the the sine input, A2 * cos(theta) is the
cosine input and phi is the position output. So mathematically
speaking, there is no signal that is the difference between the two
inputs. (See "Theory of Operation" section in the datasheet.)

But if we want to hide these internal details and don't care about a
strict definition of "differential", then what is suggested below
seems fine.

>
> So this one becomes a double event (you need to signal it on both
> cosine and sine channels).  The DOS overange is similar.
> The DOS mismatch is a threshold on the differential channel giving
>
> events/in_altvoltage0_thresh_falling_value
> events/in_altvoltage1_thresh_falling_value (these match)
> events/in_altvoltage0_thresh_rising_value
> events/in_altvoltage1_thresh_rising_value (matches previous which is fine)
> events/in_altvoltage1-altvoltage0_mag_rising_value
>
> Does that work here?  Avoids smashing different types of signals together.
> We could even do the LOT as differential between two angle channels
> (tracking one and measured one) but meh that's getting complex.>
> Note this will rely on channel labels to make the above make any sense at all.

I think this could be OK - I think what matters most is having some
documentation that maps the faults and registers on the chip to the
iio names. Where would the sine/cosine clipping fault fit in though? I
got a bit too creative and used X_OR_Y to differentiate it (see
discussion in "staging: iio: resolver: ad2s1210: implement fault
events"). Strictly speaking, it should probably be a type: threshold,
direction: either event on both the sine and cosine input channels
(another double event) since it occurs if either of the signal exceeds
the power or ground rail voltage. But we already have threshold rising
and threshold falling on these channels with a different meaning. I
guess it could call it magnitude instead of a threshold?

kernel test robot Oct. 3, 2023, 2:21 p.m. UTC | #5

Hi David,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 5e99f692d4e32e3250ab18d511894ca797407aec]

url:    https://github.com/intel-lab-lkp/linux/commits/David-Lechner/dt-bindings-iio-resolver-add-devicetree-bindings-for-ad2s1210/20230930-014031
base:   5e99f692d4e32e3250ab18d511894ca797407aec
patch link:    https://lore.kernel.org/r/20230929-ad2s1210-mainline-v3-22-fa4364281745%40baylibre.com
patch subject: [PATCH v3 22/27] staging: iio: resolver: ad2s1210: convert LOS threshold to event attr
config: i386-randconfig-062-20231003 (https://download.01.org/0day-ci/archive/20231003/[e-mail]/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231003/[e-mail]/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[e-mail]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[e-mail]/

sparse warnings: (new ones prefixed by >>)
   drivers/staging/iio/resolver/ad2s1210.c:896:1: sparse: sparse: symbol 'iio_const_attr_in_phase0_mag_value_available' was not declared. Should it be static?
>> drivers/staging/iio/resolver/ad2s1210.c:901:1: sparse: sparse: symbol 'iio_const_attr_in_altvoltage0_thresh_falling_value_available' was not declared. Should it be static?
   drivers/staging/iio/resolver/ad2s1210.c:902:1: sparse: sparse: symbol 'iio_dev_attr_in_angl1_thresh_rising_value_available' was not declared. Should it be static?
   drivers/staging/iio/resolver/ad2s1210.c:903:1: sparse: sparse: symbol 'iio_dev_attr_in_angl1_thresh_rising_hysteresis_available' was not declared. Should it be static?

Hennerich, Michael Oct. 4, 2023, 11:01 a.m. UTC | #6

> -----Original Message-----
> From: Jonathan Cameron <[e-mail]>
> Sent: Samstag, 30. September 2023 17:32
> To: David Lechner <[e-mail]>
> Cc: [e-mail]; [e-mail]; linux-
> [e-mail]; David Lechner <[e-mail]>; Rob Herring
> <[e-mail]>; Krzysztof Kozlowski
> <[e-mail]>; Conor Dooley <[e-mail]>;
> Hennerich, Michael <[e-mail]>; Sa, Nuno
> <[e-mail]>; Axel Haslam <[e-mail]>; Philip Molloy
> <[e-mail]>; [e-mail]
> Subject: Re: [PATCH v3 22/27] staging: iio: resolver: ad2s1210: convert LOS
> threshold to event attr
> 
> 
> On Fri, 29 Sep 2023 12:23:27 -0500
> David Lechner <[e-mail]> wrote:
> 
> > From: David Lechner <[e-mail]>
> >
> > From: David Lechner <[e-mail]>
> >
> > The AD2S1210 has a programmable threshold for the loss of signal (LOS)
> > fault. This fault is triggered when either the sine or cosine input
> > falls below the threshold voltage.
> >
> > This patch converts the custom device LOS threshold attribute to an
> > event falling edge threshold attribute on a new monitor signal channel.
> > The monitor signal is an internal signal that combines the amplitudes
> > of the sine and cosine inputs as well as the current angle and
> > position output. This signal is used to detect faults in the input signals.
> >
> > The attribute now uses millivolts instead of the raw register value in
> > accordance with the IIO ABI.
> >
> > Emitting the event will be implemented in a later patch.
> >
> > Signed-off-by: David Lechner <[e-mail]>
> 
> I think I'm fine with treating these internal signals like this, but I would ideally
> like someone from Analog devices to take a look at how these are being done
> and make sure our interpretations of the signals make sense to them.  We are
> pushing the boundaries a little here (though we have done similar before for
> fault events I think.)

Hi Jonathan,
David and I we also had some internal discussion related to this.
I'm sure these fault events and thresholds are understood correctly.
Doing it this or the other way, it needs to be properly documented in order to make sense.
So from my perspective whatever makes the most sense from a IIO ABI
perspective, is the way to forward.

-Michael

> 
> Jonathan

Jonathan Cameron Oct. 5, 2023, 2:16 p.m. UTC | #7

On Wed, 4 Oct 2023 11:01:56 +0000
"Hennerich, Michael" <[e-mail]> wrote:

> > -----Original Message-----
> > From: Jonathan Cameron <[e-mail]>
> > Sent: Samstag, 30. September 2023 17:32
> > To: David Lechner <[e-mail]>
> > Cc: [e-mail]; [e-mail]; linux-
> > [e-mail]; David Lechner <[e-mail]>; Rob Herring
> > <[e-mail]>; Krzysztof Kozlowski
> > <[e-mail]>; Conor Dooley <[e-mail]>;
> > Hennerich, Michael <[e-mail]>; Sa, Nuno
> > <[e-mail]>; Axel Haslam <[e-mail]>; Philip Molloy
> > <[e-mail]>; [e-mail]
> > Subject: Re: [PATCH v3 22/27] staging: iio: resolver: ad2s1210: convert LOS
> > threshold to event attr
> > 
> > 
> > On Fri, 29 Sep 2023 12:23:27 -0500
> > David Lechner <[e-mail]> wrote:
> >   
> > > From: David Lechner <[e-mail]>
> > >
> > > From: David Lechner <[e-mail]>
> > >
> > > The AD2S1210 has a programmable threshold for the loss of signal (LOS)
> > > fault. This fault is triggered when either the sine or cosine input
> > > falls below the threshold voltage.
> > >
> > > This patch converts the custom device LOS threshold attribute to an
> > > event falling edge threshold attribute on a new monitor signal channel.
> > > The monitor signal is an internal signal that combines the amplitudes
> > > of the sine and cosine inputs as well as the current angle and
> > > position output. This signal is used to detect faults in the input signals.
> > >
> > > The attribute now uses millivolts instead of the raw register value in
> > > accordance with the IIO ABI.
> > >
> > > Emitting the event will be implemented in a later patch.
> > >
> > > Signed-off-by: David Lechner <[e-mail]>  
> > 
> > I think I'm fine with treating these internal signals like this, but I would ideally
> > like someone from Analog devices to take a look at how these are being done
> > and make sure our interpretations of the signals make sense to them.  We are
> > pushing the boundaries a little here (though we have done similar before for
> > fault events I think.)  
> 
> Hi Jonathan,
> David and I we also had some internal discussion related to this.
> I'm sure these fault events and thresholds are understood correctly.
> Doing it this or the other way, it needs to be properly documented in order to make sense.
> So from my perspective whatever makes the most sense from a IIO ABI
> perspective, is the way to forward.

Great - as long as keep to a logical mapping I quite like the events
approach.  Most of these faults are real thresholds on things being measured
(even if those 'things' are signals from which stuff is derived for the main
measurements the device is making.)

Jonathan

> 
> -Michael
> 
> > 
> > Jonathan  
>

Jonathan Cameron Oct. 5, 2023, 2:37 p.m. UTC | #8

On Mon, 2 Oct 2023 11:09:11 -0500
David Lechner <[e-mail]> wrote:

> On Sat, Sep 30, 2023 at 10:42 AM Jonathan Cameron <[e-mail]> wrote:
> >
> > On Fri, 29 Sep 2023 12:23:27 -0500
> > David Lechner <[e-mail]> wrote:
> >  
> > > From: David Lechner <[e-mail]>
> > >
> > > From: David Lechner <[e-mail]>
> > >
> > > The AD2S1210 has a programmable threshold for the loss of signal (LOS)
> > > fault. This fault is triggered when either the sine or cosine input
> > > falls below the threshold voltage.
> > >
> > > This patch converts the custom device LOS threshold attribute to an
> > > event falling edge threshold attribute on a new monitor signal channel.
> > > The monitor signal is an internal signal that combines the amplitudes
> > > of the sine and cosine inputs as well as the current angle and position
> > > output. This signal is used to detect faults in the input signals.  
> >
> > Hmm. Looking forwards, I'm less sure that we should be shoving all these
> > error conditions onto one channel. Fundamentally we have
> > sine and cosine inputs. I think we should treat those as separate channels
> > and include a third differential channel between them.  
> 
> At first, I did consider a differential channel as you suggested in
> v2. However, the datasheet is quite clear that the LOS and DOS faults
> (and only those faults) come from a signal it calls the "monitor
> signal". This signal is defined as:
> 
>     Monitor = A1 * sin(theta)  * sin(phi) + A2 * cos(theta) * cos(phi)
> 
> where A1 * sin(theta) is the the sine input, A2 * cos(theta) is the
> cosine input and phi is the position output. So mathematically
> speaking, there is no signal that is the difference between the two
> inputs. (See "Theory of Operation" section in the datasheet.)

Hmm. That's certainly a bit more complex than I expected.
Relying on the brief description led me astray.

It's related to the differences in the measured and  as if
theta == phi and A1 == A2 (ideal) then it will be A1.

I can see it's relevant to DOS, but not LOS.  The description of LOS
seems to overlap a number of different things unfortunately.



> 
> But if we want to hide these internal details and don't care about a
> strict definition of "differential", then what is suggested below
> seems fine.

Probably best to introduce that monitor signal though we'll have
to be a bit vague about what it is which has the side effect that
anyone trying to understand what on earth these faults are is going
to be confused (having read the datasheet section a couple of times
I'm not 100% sure...)

> 
> >
> > So this one becomes a double event (you need to signal it on both
> > cosine and sine channels).  The DOS overange is similar.
> > The DOS mismatch is a threshold on the differential channel giving
> >
> > events/in_altvoltage0_thresh_falling_value
> > events/in_altvoltage1_thresh_falling_value (these match)
> > events/in_altvoltage0_thresh_rising_value
> > events/in_altvoltage1_thresh_rising_value (matches previous which is fine)
> > events/in_altvoltage1-altvoltage0_mag_rising_value
> >
> > Does that work here?  Avoids smashing different types of signals together.
> > We could even do the LOT as differential between two angle channels
> > (tracking one and measured one) but meh that's getting complex.>
> > Note this will rely on channel labels to make the above make any sense at all.  
> 
> I think this could be OK - I think what matters most is having some
> documentation that maps the faults and registers on the chip to the
> iio names. Where would the sine/cosine clipping fault fit in though? I
> got a bit too creative and used X_OR_Y to differentiate it (see
> discussion in "staging: iio: resolver: ad2s1210: implement fault
> events"). Strictly speaking, it should probably be a type: threshold,
> direction: either event on both the sine and cosine input channels
> (another double event) since it occurs if either of the signal exceeds
> the power or ground rail voltage. But we already have threshold rising
> and threshold falling on these channels with a different meaning. I
> guess it could call it magnitude instead of a threshold?

Tricky indeed.  Though I guess we only hit the clipping case after
LOS or DOS fires or if their thresholds are set too wide (is that
even possible?).  So it is useful to report it as we are already in
error? Or can we combine the cases by treating it as a cap on the
threshold controls for LOS and DOS?

Even when they aren't just there for error reporting, designers
seem to always come up with new create signals to use for event
detection and sometimes it's a real struggle to map them to
something general.

Jonathan

David Lechner Oct. 5, 2023, 7:25 p.m. UTC | #9

On Thu, Oct 5, 2023 at 9:37 AM Jonathan Cameron <[e-mail]> wrote:
>
> On Mon, 2 Oct 2023 11:09:11 -0500
> David Lechner <[e-mail]> wrote:
>
> > On Sat, Sep 30, 2023 at 10:42 AM Jonathan Cameron <[e-mail]> wrote:
> > >
> > > On Fri, 29 Sep 2023 12:23:27 -0500
> > > David Lechner <[e-mail]> wrote:
> > >
> > > > From: David Lechner <[e-mail]>
> > > >
> > > > From: David Lechner <[e-mail]>
> > > >
> > > > The AD2S1210 has a programmable threshold for the loss of signal (LOS)
> > > > fault. This fault is triggered when either the sine or cosine input
> > > > falls below the threshold voltage.
> > > >
> > > > This patch converts the custom device LOS threshold attribute to an
> > > > event falling edge threshold attribute on a new monitor signal channel.
> > > > The monitor signal is an internal signal that combines the amplitudes
> > > > of the sine and cosine inputs as well as the current angle and position
> > > > output. This signal is used to detect faults in the input signals.
> > >
> > > Hmm. Looking forwards, I'm less sure that we should be shoving all these
> > > error conditions onto one channel. Fundamentally we have
> > > sine and cosine inputs. I think we should treat those as separate channels
> > > and include a third differential channel between them.
> >
> > At first, I did consider a differential channel as you suggested in
> > v2. However, the datasheet is quite clear that the LOS and DOS faults
> > (and only those faults) come from a signal it calls the "monitor
> > signal". This signal is defined as:
> >
> >     Monitor = A1 * sin(theta)  * sin(phi) + A2 * cos(theta) * cos(phi)
> >
> > where A1 * sin(theta) is the the sine input, A2 * cos(theta) is the
> > cosine input and phi is the position output. So mathematically
> > speaking, there is no signal that is the difference between the two
> > inputs. (See "Theory of Operation" section in the datasheet.)
>
> Hmm. That's certainly a bit more complex than I expected.
> Relying on the brief description led me astray.
>
> It's related to the differences in the measured and  as if
> theta == phi and A1 == A2 (ideal) then it will be A1.
>
> I can see it's relevant to DOS, but not LOS.  The description of LOS
> seems to overlap a number of different things unfortunately.
>

One thing to watch out for in the datasheet is the difference between
the fault output pins and the fault bits read over the bus. The LOS
output pin does indicate one or more of multiple faults, but we are
not currently using that. We are only looking at the fault bits which
are more granular.

>
>
> >
> > But if we want to hide these internal details and don't care about a
> > strict definition of "differential", then what is suggested below
> > seems fine.
>
> Probably best to introduce that monitor signal though we'll have
> to be a bit vague about what it is which has the side effect that
> anyone trying to understand what on earth these faults are is going
> to be confused (having read the datasheet section a couple of times
> I'm not 100% sure...)
>
> >
> > >
> > > So this one becomes a double event (you need to signal it on both
> > > cosine and sine channels).  The DOS overange is similar.
> > > The DOS mismatch is a threshold on the differential channel giving
> > >
> > > events/in_altvoltage0_thresh_falling_value
> > > events/in_altvoltage1_thresh_falling_value (these match)
> > > events/in_altvoltage0_thresh_rising_value
> > > events/in_altvoltage1_thresh_rising_value (matches previous which is fine)
> > > events/in_altvoltage1-altvoltage0_mag_rising_value
> > >
> > > Does that work here?  Avoids smashing different types of signals together.
> > > We could even do the LOT as differential between two angle channels
> > > (tracking one and measured one) but meh that's getting complex.>
> > > Note this will rely on channel labels to make the above make any sense at all.
> >
> > I think this could be OK - I think what matters most is having some
> > documentation that maps the faults and registers on the chip to the
> > iio names. Where would the sine/cosine clipping fault fit in though? I
> > got a bit too creative and used X_OR_Y to differentiate it (see
> > discussion in "staging: iio: resolver: ad2s1210: implement fault
> > events"). Strictly speaking, it should probably be a type: threshold,
> > direction: either event on both the sine and cosine input channels
> > (another double event) since it occurs if either of the signal exceeds
> > the power or ground rail voltage. But we already have threshold rising
> > and threshold falling on these channels with a different meaning. I
> > guess it could call it magnitude instead of a threshold?
>
> Tricky indeed.  Though I guess we only hit the clipping case after
> LOS or DOS fires or if their thresholds are set too wide (is that
> even possible?).

I suppose it _could_ be possible on the high side if the AVDD voltage
supply was selected to be less than the 4.4V max of the threshold
voltage registers.

> So it is useful to report it as we are already in
> error? Or can we combine the cases by treating it as a cap on the
> threshold controls for LOS and DOS?

I found the clipping error useful while developing this driver since
it help identify that we had a gain setting wrong on the excitation
output (on the circuit board) which in turn caused the inputs to be
overdriven. But, yes when this happened, it also always triggered at
least one or more of the LOS and DOS faults as well.

>
> Even when they aren't just there for error reporting, designers
> seem to always come up with new create signals to use for event
> detection and sometimes it's a real struggle to map them to
> something general.
>
> Jonathan
>
>

Patch

diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
index 5cc8106800d6..7abbc184c351 100644
--- a/drivers/staging/iio/resolver/ad2s1210.c
+++ b/drivers/staging/iio/resolver/ad2s1210.c
@@ -66,6 +66,11 @@ 
 #define PHASE_360_DEG_TO_RAD_INT 6
 #define PHASE_360_DEG_TO_RAD_MICRO 283185
 
+/* Threshold voltage registers have 1 LSB == 38 mV */
+#define THRESHOLD_MILLIVOLT_PER_LSB 38
+/* max voltage for threshold registers is 0x7F * 38 mV */
+#define THRESHOLD_RANGE_STR "[0 38 4826]"
+
 enum ad2s1210_mode {
 	MOD_POS = 0b00,
 	MOD_VEL = 0b01,
@@ -448,6 +453,38 @@  static const int ad2s1210_lot_threshold_urad_per_lsb[] = {
 	1237, /* 16-bit: same as 14-bit */
 };
 
+static int ad2s1210_get_voltage_threshold(struct ad2s1210_state *st,
+					  unsigned int reg, int *val)
+{
+	unsigned int reg_val;
+	int ret;
+
+	mutex_lock(&st->lock);
+	ret = regmap_read(st->regmap, reg, &reg_val);
+	mutex_unlock(&st->lock);
+
+	if (ret < 0)
+		return ret;
+
+	*val = reg_val * THRESHOLD_MILLIVOLT_PER_LSB;
+	return IIO_VAL_INT;
+}
+
+static int ad2s1210_set_voltage_threshold(struct ad2s1210_state *st,
+					  unsigned int reg, int val)
+{
+	unsigned int reg_val;
+	int ret;
+
+	reg_val = val / THRESHOLD_MILLIVOLT_PER_LSB;
+
+	mutex_lock(&st->lock);
+	ret = regmap_write(st->regmap, reg, reg_val);
+	mutex_unlock(&st->lock);
+
+	return ret;
+}
+
 static int ad2s1210_get_lot_high_threshold(struct ad2s1210_state *st,
 					   int *val, int *val2)
 {
@@ -706,9 +743,6 @@  static int ad2s1210_write_raw(struct iio_dev *indio_dev,
 static IIO_DEVICE_ATTR(fault, 0644,
 		       ad2s1210_show_fault, ad2s1210_clear_fault, 0);
 
-static IIO_DEVICE_ATTR(los_thrd, 0644,
-		       ad2s1210_show_reg, ad2s1210_store_reg,
-		       AD2S1210_REG_LOS_THRD);
 static IIO_DEVICE_ATTR(dos_ovr_thrd, 0644,
 		       ad2s1210_show_reg, ad2s1210_store_reg,
 		       AD2S1210_REG_DOS_OVR_THRD);
@@ -745,6 +779,16 @@  static const struct iio_event_spec ad2s1210_phase_event_spec[] = {
 	},
 };
 
+static const struct iio_event_spec ad2s1210_monitor_signal_event_spec[] = {
+	{
+		/* Sine/cosine below LOS threshold fault. */
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_FALLING,
+		/* Loss of signal threshold. */
+		.mask_separate = BIT(IIO_EV_INFO_VALUE),
+	},
+};
+
 static const struct iio_chan_spec ad2s1210_channels[] = {
 	{
 		.type = IIO_ANGL,
@@ -803,12 +847,19 @@  static const struct iio_chan_spec ad2s1210_channels[] = {
 		.scan_index = -1,
 		.info_mask_separate = BIT(IIO_CHAN_INFO_FREQUENCY),
 		.info_mask_separate_available = BIT(IIO_CHAN_INFO_FREQUENCY),
+	}, {
+		/* monitor signal */
+		.type = IIO_ALTVOLTAGE,
+		.indexed = 1,
+		.channel = 0,
+		.scan_index = -1,
+		.event_spec = ad2s1210_monitor_signal_event_spec,
+		.num_event_specs = ARRAY_SIZE(ad2s1210_monitor_signal_event_spec),
 	},
 };
 
 static struct attribute *ad2s1210_attributes[] = {
 	&iio_dev_attr_fault.dev_attr.attr,
-	&iio_dev_attr_los_thrd.dev_attr.attr,
 	&iio_dev_attr_dos_ovr_thrd.dev_attr.attr,
 	&iio_dev_attr_dos_mis_thrd.dev_attr.attr,
 	&iio_dev_attr_dos_rst_max_thrd.dev_attr.attr,
@@ -847,11 +898,13 @@  IIO_CONST_ATTR(in_phase0_mag_value_available,
 	       __stringify(PHASE_44_DEG_TO_RAD_MICRO) " "
 	       __stringify(PHASE_360_DEG_TO_RAD_INT) "."
 	       __stringify(PHASE_360_DEG_TO_RAD_MICRO));
+IIO_CONST_ATTR(in_altvoltage0_thresh_falling_value_available, THRESHOLD_RANGE_STR);
 IIO_DEVICE_ATTR_RO(in_angl1_thresh_rising_value_available, 0);
 IIO_DEVICE_ATTR_RO(in_angl1_thresh_rising_hysteresis_available, 0);
 
 static struct attribute *ad2s1210_event_attributes[] = {
 	&iio_const_attr_in_phase0_mag_value_available.dev_attr.attr,
+	&iio_const_attr_in_altvoltage0_thresh_falling_value_available.dev_attr.attr,
 	&iio_dev_attr_in_angl1_thresh_rising_value_available.dev_attr.attr,
 	&iio_dev_attr_in_angl1_thresh_rising_hysteresis_available.dev_attr.attr,
 	NULL,
@@ -904,6 +957,13 @@  static int ad2s1210_read_event_value(struct iio_dev *indio_dev,
 		default:
 			return -EINVAL;
 		}
+	case IIO_ALTVOLTAGE:
+		if (chan->output)
+			return -EINVAL;
+		if (type == IIO_EV_TYPE_THRESH && dir == IIO_EV_DIR_FALLING)
+			return ad2s1210_get_voltage_threshold(st,
+						AD2S1210_REG_LOS_THRD, val);
+		return -EINVAL;
 	case IIO_PHASE:
 		return ad2s1210_get_phase_lock_range(st, val, val2);
 	default:
@@ -930,6 +990,13 @@  static int ad2s1210_write_event_value(struct iio_dev *indio_dev,
 		default:
 			return -EINVAL;
 		}
+	case IIO_ALTVOLTAGE:
+		if (chan->output)
+			return -EINVAL;
+		if (type == IIO_EV_TYPE_THRESH && dir == IIO_EV_DIR_FALLING)
+			return ad2s1210_set_voltage_threshold(st,
+						AD2S1210_REG_LOS_THRD, val);
+		return -EINVAL;
 	case IIO_PHASE:
 		return ad2s1210_set_phase_lock_range(st, val, val2);
 	default: