Re: [PATCH v2 1/2] drivers: iio: filter: admv8818: add bypass mode

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 31 Jul 2023 11:49:26 +0300
Antoniu Miclaus <antoniu.miclaus@analog.com> wrote:

> Add filter bypass mode, which bypasses the low pass filter, high pass
> filter and disables/unregister the clock rate notifier.
> 
> Currently a feature like bypassing the filter is not achievable
> straightforward and not very deductive. The user has to look through the
> code and call the set_lpf_3db_frequency and set_hpf_3db_frequency iio
> attributes from the user interface using the corner cases (freq >
> largest lpf supported by the part, respectively freq < smallest hpf
> supported by the part). Moreover, in such case of bypassing the filter,
> the input clock rate change might mess up things so we want to make sure
> that it is disabled. Also, the feature will help emphasizing the filter
> behavior, therefore adding it in the userspace will ease the
> charcaterization of the filter's effects when active/disabled.

Reasoning is well laid out. Thanks!  It's an unusual feature, but meh
it's also hidden in existing custom ABI so unlikely to cause any
problems.

> 
> It was requested by users of the driver to ease the interaction with
> different configuration modes of the device.
> 
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
Applied to the togreg branch of iio.git and pushed out as testing for 0-day
to take a look at it and see if we missed anything.

> ---
> changes in v2:
>  - improve code readability when setting the filter modes
>  - add more explanations regarding the necessity of this feature in the commit
>    body.
>  drivers/iio/filter/admv8818.c | 65 ++++++++++++++++++++++++++++++-----
>  1 file changed, 56 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/iio/filter/admv8818.c b/drivers/iio/filter/admv8818.c
> index fe8d46cb7f1d..848baa6e3bbf 100644
> --- a/drivers/iio/filter/admv8818.c
> +++ b/drivers/iio/filter/admv8818.c
> @@ -78,6 +78,7 @@ enum {
>  enum {
>  	ADMV8818_AUTO_MODE,
>  	ADMV8818_MANUAL_MODE,
> +	ADMV8818_BYPASS_MODE,
>  };
>  
>  struct admv8818_state {
> @@ -114,7 +115,8 @@ static const struct regmap_config admv8818_regmap_config = {
>  
>  static const char * const admv8818_modes[] = {
>  	[0] = "auto",
> -	[1] = "manual"
> +	[1] = "manual",
> +	[2] = "bypass"
>  };
>  
>  static int __admv8818_hpf_select(struct admv8818_state *st, u64 freq)
> @@ -394,6 +396,36 @@ static int admv8818_reg_access(struct iio_dev *indio_dev,
>  		return regmap_write(st->regmap, reg, write_val);
>  }
>  
> +static int admv8818_filter_bypass(struct admv8818_state *st)
> +{
> +	int ret;
> +
> +	mutex_lock(&st->lock);
> +
> +	ret = regmap_update_bits(st->regmap, ADMV8818_REG_WR0_SW,
> +				 ADMV8818_SW_IN_SET_WR0_MSK |
> +				 ADMV8818_SW_IN_WR0_MSK |
> +				 ADMV8818_SW_OUT_SET_WR0_MSK |
> +				 ADMV8818_SW_OUT_WR0_MSK,
> +				 FIELD_PREP(ADMV8818_SW_IN_SET_WR0_MSK, 1) |
> +				 FIELD_PREP(ADMV8818_SW_IN_WR0_MSK, 0) |
> +				 FIELD_PREP(ADMV8818_SW_OUT_SET_WR0_MSK, 1) |
> +				 FIELD_PREP(ADMV8818_SW_OUT_WR0_MSK, 0));
> +	if (ret)
> +		goto exit;
> +
> +	ret = regmap_update_bits(st->regmap, ADMV8818_REG_WR0_FILTER,
> +				 ADMV8818_HPF_WR0_MSK |
> +				 ADMV8818_LPF_WR0_MSK,
> +				 FIELD_PREP(ADMV8818_HPF_WR0_MSK, 0) |
> +				 FIELD_PREP(ADMV8818_LPF_WR0_MSK, 0));
> +
> +exit:
> +	mutex_unlock(&st->lock);
> +
> +	return ret;
> +}
> +
>  static int admv8818_get_mode(struct iio_dev *indio_dev,
>  			     const struct iio_chan_spec *chan)
>  {
> @@ -411,14 +443,22 @@ static int admv8818_set_mode(struct iio_dev *indio_dev,
>  
>  	if (!st->clkin) {
>  		if (mode == ADMV8818_MANUAL_MODE)
> -			return 0;
> +			goto set_mode;
> +
> +		if (mode == ADMV8818_BYPASS_MODE) {
> +			ret = admv8818_filter_bypass(st);
> +			if (ret)
> +				return ret;
> +
> +			goto set_mode;
> +		}
>  
>  		return -EINVAL;
>  	}
>  
>  	switch (mode) {
>  	case ADMV8818_AUTO_MODE:
> -		if (!st->filter_mode)
> +		if (st->filter_mode == ADMV8818_AUTO_MODE)
>  			return 0;
>  
>  		ret = clk_prepare_enable(st->clkin);
> @@ -434,20 +474,27 @@ static int admv8818_set_mode(struct iio_dev *indio_dev,
>  
>  		break;
>  	case ADMV8818_MANUAL_MODE:
> -		if (st->filter_mode)
> -			return 0;
> +	case ADMV8818_BYPASS_MODE:
> +		if (st->filter_mode == ADMV8818_AUTO_MODE) {
> +			clk_disable_unprepare(st->clkin);
>  
> -		clk_disable_unprepare(st->clkin);
> +			ret = clk_notifier_unregister(st->clkin, &st->nb);
> +			if (ret)
> +				return ret;
> +		}
>  
> -		ret = clk_notifier_unregister(st->clkin, &st->nb);
> -		if (ret)
> -			return ret;
> +		if (mode == ADMV8818_BYPASS_MODE) {
> +			ret = admv8818_filter_bypass(st);
> +			if (ret)
> +				return ret;
> +		}
>  
>  		break;
>  	default:
>  		return -EINVAL;
>  	}
>  
> +set_mode:
>  	st->filter_mode = mode;
>  
>  	return ret;