Re: [PATCH v4 07/17] staging: iio: resolver: ad2s1210: convert LOT threshold attrs to event attrs

[Jonathan Cameron <jic23@kernel.org>]

On Thu,  5 Oct 2023 19:50:24 -0500
David Lechner <dlechner@baylibre.com> wrote:

> The AD2S1210 monitors the internal error signal (difference between
> estimated angle and measured angle) to determine a loss of position
> tracking (LOT) condition. When the error value exceeds a threshold, a
> fault is triggered. This threshold is user-configurable.
> 
> This patch converts the custom lot_high_thrd and lot_low_thrd attributes
> in the ad2s1210 driver to standard event attributes. This will allow
> tooling to be able to expose these in a generic way.
> 
> Since the low threshold determines the hysteresis, it requires some
> special handling to expose the difference between the high and low
> register values as the hysteresis instead of exposing the low register
> value directly.
> 
> The attributes also return the values in radians now as required by the
> ABI.
> 
> Actually emitting the fault event will be done in a later patch.
> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>
I noticed it first on next patch, but we are a bit vague on what the units
of event thresholds are - normally tracking an associated channel.  That
gets tricky if there isn't an associated channel read facility.

I've applied this anyway, but we may want to think a bit more on that.
One approach is to add IIO_CHAN_INFO_SCALE and read_raw support for these
channels. 

Not what you have here is valid without one of those because absence
of _scale, means _scale == 1 anyway so we are good with having these
in radians.

Jonathan

> ---
> 
> v4 changes:
> * Fixed missing static qualifier on attribute definition.
> 
> v3 changes: This is a new patch in v3
> 
>  drivers/staging/iio/resolver/ad2s1210.c | 191 ++++++++++++++++++++++++++++++--
>  1 file changed, 183 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
> index 4d651a2d0f38..12437f697f79 100644
> --- a/drivers/staging/iio/resolver/ad2s1210.c
> +++ b/drivers/staging/iio/resolver/ad2s1210.c
> @@ -443,6 +443,123 @@ static int ad2s1210_set_phase_lock_range(struct ad2s1210_state *st,
>  	return ret;
>  }
>  
> +/* map resolution to microradians/LSB for LOT registers */
> +static const int ad2s1210_lot_threshold_urad_per_lsb[] = {
> +	6184, /* 10-bit: ~0.35 deg/LSB, 45 deg max */
> +	2473, /* 12-bit: ~0.14 deg/LSB, 18 deg max */
> +	1237, /* 14-bit: ~0.07 deg/LSB, 9 deg max */
> +	1237, /* 16-bit: same as 14-bit */
> +};
> +
> +static int ad2s1210_get_lot_high_threshold(struct ad2s1210_state *st,
> +					   int *val, int *val2)
> +{
> +	unsigned int reg_val;
> +	int ret;
> +
> +	mutex_lock(&st->lock);
> +	ret = regmap_read(st->regmap, AD2S1210_REG_LOT_HIGH_THRD, &reg_val);
> +	mutex_unlock(&st->lock);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	*val = 0;
> +	*val2 = reg_val * ad2s1210_lot_threshold_urad_per_lsb[st->resolution];
> +	return IIO_VAL_INT_PLUS_MICRO;
> +}
> +
> +static int ad2s1210_set_lot_high_threshold(struct ad2s1210_state *st,
> +					   int val, int val2)
> +{
> +	unsigned int high_reg_val, low_reg_val, hysteresis;
> +	int ret;
> +
> +	/* all valid values are between 0 and pi/4 radians */
> +	if (val != 0)
> +		return -EINVAL;
> +
> +	mutex_lock(&st->lock);
> +	/*
> +	 * We need to read both high and low registers first so we can preserve
> +	 * the hysteresis.
> +	 */
> +	ret = regmap_read(st->regmap, AD2S1210_REG_LOT_HIGH_THRD, &high_reg_val);
> +	if (ret < 0)
> +		goto error_ret;
> +
> +	ret = regmap_read(st->regmap, AD2S1210_REG_LOT_LOW_THRD, &low_reg_val);
> +	if (ret < 0)
> +		goto error_ret;
> +
> +	hysteresis = high_reg_val - low_reg_val;
> +	high_reg_val = val2 / ad2s1210_lot_threshold_urad_per_lsb[st->resolution];
> +	low_reg_val = high_reg_val - hysteresis;
> +
> +	ret = regmap_write(st->regmap, AD2S1210_REG_LOT_HIGH_THRD, high_reg_val);
> +	if (ret < 0)
> +		goto error_ret;
> +
> +	ret = regmap_write(st->regmap, AD2S1210_REG_LOT_LOW_THRD, low_reg_val);
> +
> +error_ret:
> +	mutex_unlock(&st->lock);
> +
> +	return ret;
> +}
> +
> +static int ad2s1210_get_lot_low_threshold(struct ad2s1210_state *st,
> +					  int *val, int *val2)
> +{
> +	unsigned int high_reg_val, low_reg_val;
> +	int ret;
> +
> +	mutex_lock(&st->lock);
> +	ret = regmap_read(st->regmap, AD2S1210_REG_LOT_HIGH_THRD, &high_reg_val);
> +	if (ret < 0)
> +		goto error_ret;
> +
> +	ret = regmap_read(st->regmap, AD2S1210_REG_LOT_LOW_THRD, &low_reg_val);
> +
> +error_ret:
> +	mutex_unlock(&st->lock);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	/* sysfs value is hysteresis rather than actual low value */
> +	*val = 0;
> +	*val2 = (high_reg_val - low_reg_val) *
> +		ad2s1210_lot_threshold_urad_per_lsb[st->resolution];
> +	return IIO_VAL_INT_PLUS_MICRO;
> +}
> +
> +static int ad2s1210_set_lot_low_threshold(struct ad2s1210_state *st,
> +					  int val, int val2)
> +{
> +	unsigned int reg_val, hysteresis;
> +	int ret;
> +
> +	/* all valid values are between 0 and pi/4 radians */
> +	if (val != 0)
> +		return -EINVAL;
> +
> +	hysteresis = val2 / ad2s1210_lot_threshold_urad_per_lsb[st->resolution];
> +
> +	mutex_lock(&st->lock);
> +	ret = regmap_read(st->regmap, AD2S1210_REG_LOT_HIGH_THRD, &reg_val);
> +	if (ret < 0)
> +		goto error_ret;
> +
> +	ret = regmap_write(st->regmap, AD2S1210_REG_LOT_LOW_THRD,
> +			   reg_val - hysteresis);
> +
> +error_ret:
> +	mutex_unlock(&st->lock);
> +
> +	return ret;
> +}
> +
>  static int ad2s1210_get_excitation_frequency(struct ad2s1210_state *st, int *val)
>  {
>  	unsigned int reg_val;
> @@ -608,12 +725,19 @@ static IIO_DEVICE_ATTR(dos_rst_max_thrd, 0644,
>  static IIO_DEVICE_ATTR(dos_rst_min_thrd, 0644,
>  		       ad2s1210_show_reg, ad2s1210_store_reg,
>  		       AD2S1210_REG_DOS_RST_MIN_THRD);
> -static IIO_DEVICE_ATTR(lot_high_thrd, 0644,
> -		       ad2s1210_show_reg, ad2s1210_store_reg,
> -		       AD2S1210_REG_LOT_HIGH_THRD);
> -static IIO_DEVICE_ATTR(lot_low_thrd, 0644,
> -		       ad2s1210_show_reg, ad2s1210_store_reg,
> -		       AD2S1210_REG_LOT_LOW_THRD);
> +
> +static const struct iio_event_spec ad2s1210_position_event_spec[] = {
> +	{
> +		/* Tracking error exceeds LOT threshold fault. */
> +		.type = IIO_EV_TYPE_THRESH,
> +		.dir = IIO_EV_DIR_RISING,
> +		.mask_separate =
> +			/* Loss of tracking high threshold. */
> +			BIT(IIO_EV_INFO_VALUE) |
> +			/* Loss of tracking low threshold. */
> +			BIT(IIO_EV_INFO_HYSTERESIS),
> +	},
> +};
>  
>  static const struct iio_event_spec ad2s1210_phase_event_spec[] = {
>  	{
> @@ -657,6 +781,15 @@ static const struct iio_chan_spec ad2s1210_channels[] = {
>  				      BIT(IIO_CHAN_INFO_SCALE),
>  	},
>  	IIO_CHAN_SOFT_TIMESTAMP(2),
> +	{
> +		/* used to configure LOT thresholds and get tracking error */
> +		.type = IIO_ANGL,
> +		.indexed = 1,
> +		.channel = 1,
> +		.scan_index = -1,
> +		.event_spec = ad2s1210_position_event_spec,
> +		.num_event_specs = ARRAY_SIZE(ad2s1210_position_event_spec),
> +	},
>  	{
>  		/* used to configure phase lock range and get phase lock error */
>  		.type = IIO_PHASE,
> @@ -684,8 +817,6 @@ static struct attribute *ad2s1210_attributes[] = {
>  	&iio_dev_attr_dos_mis_thrd.dev_attr.attr,
>  	&iio_dev_attr_dos_rst_max_thrd.dev_attr.attr,
>  	&iio_dev_attr_dos_rst_min_thrd.dev_attr.attr,
> -	&iio_dev_attr_lot_high_thrd.dev_attr.attr,
> -	&iio_dev_attr_lot_low_thrd.dev_attr.attr,
>  	NULL,
>  };
>  
> @@ -693,14 +824,40 @@ static const struct attribute_group ad2s1210_attribute_group = {
>  	.attrs = ad2s1210_attributes,
>  };
>  
> +static ssize_t
> +in_angl1_thresh_rising_value_available_show(struct device *dev,
> +					    struct device_attribute *attr,
> +					    char *buf)
> +{
> +	struct ad2s1210_state *st = iio_priv(dev_to_iio_dev(dev));
> +	int step = ad2s1210_lot_threshold_urad_per_lsb[st->resolution];
> +
> +	return sysfs_emit(buf, "[0 0.%06d 0.%06d]\n", step, step * 0x7F);
> +}
> +
> +static ssize_t
> +in_angl1_thresh_rising_hysteresis_available_show(struct device *dev,
> +						 struct device_attribute *attr,
> +						 char *buf)
> +{
> +	struct ad2s1210_state *st = iio_priv(dev_to_iio_dev(dev));
> +	int step = ad2s1210_lot_threshold_urad_per_lsb[st->resolution];
> +
> +	return sysfs_emit(buf, "[0 0.%06d 0.%06d]\n", step, step * 0x7F);
> +}
> +
>  static IIO_CONST_ATTR(in_phase0_mag_rising_value_available,
>  		      __stringify(PHASE_44_DEG_TO_RAD_INT) "."
>  		      __stringify(PHASE_44_DEG_TO_RAD_MICRO) " "
>  		      __stringify(PHASE_360_DEG_TO_RAD_INT) "."
>  		      __stringify(PHASE_360_DEG_TO_RAD_MICRO));
> +static IIO_DEVICE_ATTR_RO(in_angl1_thresh_rising_value_available, 0);
> +static IIO_DEVICE_ATTR_RO(in_angl1_thresh_rising_hysteresis_available, 0);
>  
>  static struct attribute *ad2s1210_event_attributes[] = {
>  	&iio_const_attr_in_phase0_mag_rising_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,
>  };
>  
> @@ -742,6 +899,15 @@ static int ad2s1210_read_event_value(struct iio_dev *indio_dev,
>  	struct ad2s1210_state *st = iio_priv(indio_dev);
>  
>  	switch (chan->type) {
> +	case IIO_ANGL:
> +		switch (info) {
> +		case IIO_EV_INFO_VALUE:
> +			return ad2s1210_get_lot_high_threshold(st, val, val2);
> +		case IIO_EV_INFO_HYSTERESIS:
> +			return ad2s1210_get_lot_low_threshold(st, val, val2);
> +		default:
> +			return -EINVAL;
> +		}
>  	case IIO_PHASE:
>  		return ad2s1210_get_phase_lock_range(st, val, val2);
>  	default:
> @@ -759,6 +925,15 @@ static int ad2s1210_write_event_value(struct iio_dev *indio_dev,
>  	struct ad2s1210_state *st = iio_priv(indio_dev);
>  
>  	switch (chan->type) {
> +	case IIO_ANGL:
> +		switch (info) {
> +		case IIO_EV_INFO_VALUE:
> +			return ad2s1210_set_lot_high_threshold(st, val, val2);
> +		case IIO_EV_INFO_HYSTERESIS:
> +			return ad2s1210_set_lot_low_threshold(st, val, val2);
> +		default:
> +			return -EINVAL;
> +		}
>  	case IIO_PHASE:
>  		return ad2s1210_set_phase_lock_range(st, val, val2);
>  	default:
>