diff mbox series

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

Message ID 20230731084928.8302-1-antoniu.miclaus@analog.com (mailing list archive)
State Accepted
Headers show
Series [v2,1/2] drivers: iio: filter: admv8818: add bypass mode | expand

Commit Message

Antoniu Miclaus July 31, 2023, 8:49 a.m. UTC
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.

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>
---
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(-)

Comments

Jonathan Cameron Aug. 5, 2023, 6:52 p.m. UTC | #1
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;
diff mbox series

Patch

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;