diff mbox series

[4/6] iio: adc: ad7173: refactor ain and vref selection

Message ID 20240401-ad4111-v1-4-34618a9cc502@analog.com (mailing list archive)
State Changes Requested
Headers show
Series Add support for AD411x | expand

Commit Message

Dumitru Ceclan via B4 Relay April 1, 2024, 3:32 p.m. UTC 
From: Dumitru Ceclan <dumitru.ceclan@analog.com>

Move validation of analog inputs and reference voltage selection to
separate functions.

Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
---
 drivers/iio/adc/ad7173.c | 59 +++++++++++++++++++++++++++++++++---------------
 1 file changed, 41 insertions(+), 18 deletions(-)

Comments

David Lechner April 1, 2024, 7:40 p.m. UTC | #1 
On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay
<devnull+dumitru.ceclan.analog.com@kernel.org> wrote:
>
> From: Dumitru Ceclan <dumitru.ceclan@analog.com>
>
> Move validation of analog inputs and reference voltage selection to
> separate functions.
>
> Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
> ---

Same as my comment on PATCH 3/6. We would like to know why this change
is being made.
Ceclan, Dumitru April 3, 2024, 10:03 a.m. UTC | #2 
On 01/04/2024 22:40, David Lechner wrote:
> On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay
> <devnull+dumitru.ceclan.analog.com@kernel.org> wrote:
>>
>> From: Dumitru Ceclan <dumitru.ceclan@analog.com>
>>
>> Move validation of analog inputs and reference voltage selection to
>> separate functions.
>>
>> Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
>> ---
> 
> Same as my comment on PATCH 3/6. We would like to know why this change
> is being made.

Move validation of analog inputs and reference voltage selection to
separate functions to reduce the size of the channel config parsing function.

Good?
David Lechner April 3, 2024, 4:02 p.m. UTC | #3 
On Wed, Apr 3, 2024 at 5:03 AM Ceclan, Dumitru <mitrutzceclan@gmail.com> wrote:
>
> On 01/04/2024 22:40, David Lechner wrote:
> > On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay
> > <devnull+dumitru.ceclan.analog.com@kernel.org> wrote:
> >>
> >> From: Dumitru Ceclan <dumitru.ceclan@analog.com>
> >>
> >> Move validation of analog inputs and reference voltage selection to
> >> separate functions.
> >>
> >> Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
> >> ---
> >
> > Same as my comment on PATCH 3/6. We would like to know why this change
> > is being made.
>
> Move validation of analog inputs and reference voltage selection to
> separate functions to reduce the size of the channel config parsing function.
>
> Good?

Better. But it still only says what is being done and doesn't answer
the question "why?".

"to reduce the size of the function to make it easier to read"
explains why reducing the size of the function makes it an
improvement.
Jonathan Cameron April 6, 2024, 3:03 p.m. UTC | #4 
On Mon, 01 Apr 2024 18:32:22 +0300
Dumitru Ceclan via B4 Relay <devnull+dumitru.ceclan.analog.com@kernel.org> wrote:

> From: Dumitru Ceclan <dumitru.ceclan@analog.com>
> 
> Move validation of analog inputs and reference voltage selection to
> separate functions.
> 
> Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
A few line wrapping comments inline. 

> ---
>  drivers/iio/adc/ad7173.c | 59 +++++++++++++++++++++++++++++++++---------------
>  1 file changed, 41 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
> index 699bc6970790..bf5a5b384fe2 100644
> --- a/drivers/iio/adc/ad7173.c
> +++ b/drivers/iio/adc/ad7173.c
> @@ -910,6 +910,41 @@ static int ad7173_register_clk_provider(struct iio_dev *indio_dev)
>  					   &st->int_clk_hw);
>  }
>  
> +static int ad7173_validate_voltage_ain_inputs(struct ad7173_state *st,
> +					      unsigned int ain[2])
> +{
> +	struct device *dev = &st->sd.spi->dev;
> +
> +	if (ain[0] >= st->info->num_inputs ||
> +	    ain[1] >= st->info->num_inputs)

No need to line wrap the above - its under 80 chars on one line with the
new reduced indent due to factoring this out.

> +		return dev_err_probe(dev, -EINVAL,
> +			"Input pin number out of range for pair (%d %d).\n",
> +			ain[0], ain[1]);
> +
> +	return 0;
> +}
> +
> +static int ad7173_validate_reference(struct ad7173_state *st, int ref_sel)
> +{
> +	struct device *dev = &st->sd.spi->dev;
> +	int ret;
> +
> +	if (ref_sel == AD7173_SETUP_REF_SEL_INT_REF && !st->info->has_int_ref)
> +		return dev_err_probe(dev, -EINVAL,
> +			"Internal reference is not available on current model.\n");
> +
> +	if (ref_sel == AD7173_SETUP_REF_SEL_EXT_REF2 && !st->info->has_ref2)
> +		return dev_err_probe(dev, -EINVAL,
> +			"External reference 2 is not available on current model.\n");
> +
> +	ret = ad7173_get_ref_voltage_milli(st, ref_sel);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret,
> +			"Cannot use reference %u\n", ref_sel);

Can pull the string to previous line and then align ref_sel just after (
whilst still remaining under 80 chars and end up a little prettier.


> +
> +	return 0;
> +}
> +
>  static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
>  {
>  	struct ad7173_channel *chans_st_arr, *chan_st_priv;
> @@ -970,11 +1005,9 @@ static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
>  		if (ret)
>  			return ret;
>  
> -		if (ain[0] >= st->info->num_inputs ||
> -		    ain[1] >= st->info->num_inputs)
> -			return dev_err_probe(dev, -EINVAL,
> -				"Input pin number out of range for pair (%d %d).\n",
> -				ain[0], ain[1]);
> +		ret = ad7173_validate_voltage_ain_inputs(st, ain);
> +		if (ret)
> +			return ret;
>  
>  		ret = fwnode_property_match_property_string(child,
>  							    "adi,reference-select",
> @@ -985,19 +1018,9 @@ static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
>  		else
>  			ref_sel = ret;
>  
> -		if (ref_sel == AD7173_SETUP_REF_SEL_INT_REF &&
> -		    !st->info->has_int_ref)
> -			return dev_err_probe(dev, -EINVAL,
> -				"Internal reference is not available on current model.\n");
> -
> -		if (ref_sel == AD7173_SETUP_REF_SEL_EXT_REF2 && !st->info->has_ref2)
> -			return dev_err_probe(dev, -EINVAL,
> -				"External reference 2 is not available on current model.\n");
> -
> -		ret = ad7173_get_ref_voltage_milli(st, ref_sel);
> -		if (ret < 0)
> -			return dev_err_probe(dev, ret,
> -					     "Cannot use reference %u\n", ref_sel);
> +		ret = ad7173_validate_reference(st, ref_sel);
> +		if (ret)
> +			return ret;
>  
>  		if (ref_sel == AD7173_SETUP_REF_SEL_INT_REF)
>  			st->adc_mode |= AD7173_ADC_MODE_REF_EN;
>
diff mbox series

Patch

diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
index 699bc6970790..bf5a5b384fe2 100644
--- a/drivers/iio/adc/ad7173.c
+++ b/drivers/iio/adc/ad7173.c
@@ -910,6 +910,41 @@  static int ad7173_register_clk_provider(struct iio_dev *indio_dev)
 					   &st->int_clk_hw);
 }
 
+static int ad7173_validate_voltage_ain_inputs(struct ad7173_state *st,
+					      unsigned int ain[2])
+{
+	struct device *dev = &st->sd.spi->dev;
+
+	if (ain[0] >= st->info->num_inputs ||
+	    ain[1] >= st->info->num_inputs)
+		return dev_err_probe(dev, -EINVAL,
+			"Input pin number out of range for pair (%d %d).\n",
+			ain[0], ain[1]);
+
+	return 0;
+}
+
+static int ad7173_validate_reference(struct ad7173_state *st, int ref_sel)
+{
+	struct device *dev = &st->sd.spi->dev;
+	int ret;
+
+	if (ref_sel == AD7173_SETUP_REF_SEL_INT_REF && !st->info->has_int_ref)
+		return dev_err_probe(dev, -EINVAL,
+			"Internal reference is not available on current model.\n");
+
+	if (ref_sel == AD7173_SETUP_REF_SEL_EXT_REF2 && !st->info->has_ref2)
+		return dev_err_probe(dev, -EINVAL,
+			"External reference 2 is not available on current model.\n");
+
+	ret = ad7173_get_ref_voltage_milli(st, ref_sel);
+	if (ret < 0)
+		return dev_err_probe(dev, ret,
+			"Cannot use reference %u\n", ref_sel);
+
+	return 0;
+}
+
 static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
 {
 	struct ad7173_channel *chans_st_arr, *chan_st_priv;
@@ -970,11 +1005,9 @@  static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
 		if (ret)
 			return ret;
 
-		if (ain[0] >= st->info->num_inputs ||
-		    ain[1] >= st->info->num_inputs)
-			return dev_err_probe(dev, -EINVAL,
-				"Input pin number out of range for pair (%d %d).\n",
-				ain[0], ain[1]);
+		ret = ad7173_validate_voltage_ain_inputs(st, ain);
+		if (ret)
+			return ret;
 
 		ret = fwnode_property_match_property_string(child,
 							    "adi,reference-select",
@@ -985,19 +1018,9 @@  static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
 		else
 			ref_sel = ret;
 
-		if (ref_sel == AD7173_SETUP_REF_SEL_INT_REF &&
-		    !st->info->has_int_ref)
-			return dev_err_probe(dev, -EINVAL,
-				"Internal reference is not available on current model.\n");
-
-		if (ref_sel == AD7173_SETUP_REF_SEL_EXT_REF2 && !st->info->has_ref2)
-			return dev_err_probe(dev, -EINVAL,
-				"External reference 2 is not available on current model.\n");
-
-		ret = ad7173_get_ref_voltage_milli(st, ref_sel);
-		if (ret < 0)
-			return dev_err_probe(dev, ret,
-					     "Cannot use reference %u\n", ref_sel);
+		ret = ad7173_validate_reference(st, ref_sel);
+		if (ret)
+			return ret;
 
 		if (ref_sel == AD7173_SETUP_REF_SEL_INT_REF)
 			st->adc_mode |= AD7173_ADC_MODE_REF_EN;