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

[Jonathan Cameron]

On Wed, 26 Jul 2023 17:33:30 +0300
Antoniu Miclaus <antoniu.miclaus@xxxxxxxxxx> wrote:

> Add filter bypass mode, which bypasses the low pass filter, high pass
> filter and disables/unregister the clock rate notifier.
> 
> The patch contains minimal changes in order to add the functionality.
> 
> It was requested by users of the driver to ease the interaction with
> different configuration modes of the device.

Hi Antoniu

I'd like to understand more about the use case for this.  My assumption
is that you'd do this if there is appropriate signal conditioning off
chip.  If that's the case I'd expect to see this as a device tree binding
thing rather than exposed to userspace.

Also, I can see that it may be useful to separately control the input and
output filter bypassing which this doesn't enable.

Hence need some use case information to decide if this is a reasonable
addition to the userspace ABI.

Other comments inline.

Thanks,

Jonathan


> 
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@xxxxxxxxxx>
> ---
>  drivers/iio/filter/admv8818.c | 51 ++++++++++++++++++++++++++++++++---
>  1 file changed, 48 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/filter/admv8818.c b/drivers/iio/filter/admv8818.c
> index fe8d46cb7f1d..f0d6bb606507 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,7 +443,10 @@ 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)
> +			goto bypass_filter;

Flow wise, this is a little difficult to follow.
I'd be tempted to just duplicate the small amount of
handling below in the two paths that I can see end up going the code
that configures bypass.

>  
>  		return -EINVAL;
>  	}
> @@ -434,8 +469,9 @@ static int admv8818_set_mode(struct iio_dev *indio_dev,
>  
>  		break;
>  	case ADMV8818_MANUAL_MODE:
> +	case ADMV8818_BYPASS_MODE:
>  		if (st->filter_mode)

This was ugly in the first place as it relied on the values of the enum being 0
and 1 (without them being specified as such).  I'd tidy this up whilst here
as now we have 0 1 2 so it's harder to follow than before.  Just check against
the appropriate values eg != AUTO



> -			return 0;
> +			break;
>  
>  		clk_disable_unprepare(st->clkin);
>  
> @@ -448,6 +484,15 @@ static int admv8818_set_mode(struct iio_dev *indio_dev,
>  		return -EINVAL;
>  	}
>  
> +bypass_filter:
> +	if (st->filter_mode != ADMV8818_BYPASS_MODE &&
> +	    mode == ADMV8818_BYPASS_MODE) {
> +		ret = admv8818_filter_bypass(st);

I'd like to see this up in the switch statement even if that means duplicating
a little more code.  Rethink this function so as to make it more
readable than it ends up after this change.

> +		if (ret)
> +			return ret;
> +	}
> +
> +set_mode:
>  	st->filter_mode = mode;
>  
>  	return ret;