On Fri, 31 May 2024 22:42:29 +0300
Dumitru Ceclan via B4 Relay <devnull+dumitru.ceclan.analog.com@xxxxxxxxxx> wrote:
> From: Dumitru Ceclan <dumitru.ceclan@xxxxxxxxxx>
>
> Move validation of analog inputs and reference voltage selection to
> separate functions to reduce the size of the channel config parsing
> function and improve readability.
> Add defines for the number of analog inputs in a channel.
>
> Reviewed-by: David Lechner <dlechner@xxxxxxxxxxxx>
> Signed-off-by: Dumitru Ceclan <dumitru.ceclan@xxxxxxxxxx>
> ---
> drivers/iio/adc/ad7173.c | 71 ++++++++++++++++++++++++++++++++++--------------
> 1 file changed, 50 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
> index 6e249628bc64..a20831d99aa5 100644
> --- a/drivers/iio/adc/ad7173.c
> +++ b/drivers/iio/adc/ad7173.c
> @@ -60,6 +60,7 @@
> #define AD7173_CH_SETUP_AINPOS_MASK GENMASK(9, 5)
> #define AD7173_CH_SETUP_AINNEG_MASK GENMASK(4, 0)
>
> +#define AD7173_NO_AINS_PER_CHANNEL 2
> #define AD7173_CH_ADDRESS(pos, neg) \
> (FIELD_PREP(AD7173_CH_SETUP_AINPOS_MASK, pos) | \
> FIELD_PREP(AD7173_CH_SETUP_AINNEG_MASK, neg))
> @@ -623,6 +624,7 @@ static int ad7173_setup(struct iio_dev *indio_dev)
> static unsigned int ad7173_get_ref_voltage_milli(struct ad7173_state *st,
> u8 reference_select)
> {
> + struct device *dev = &st->sd.spi->dev;
> int vref;
>
> switch (reference_select) {
> @@ -646,9 +648,11 @@ static unsigned int ad7173_get_ref_voltage_milli(struct ad7173_state *st,
> return -EINVAL;
> }
>
> - if (vref < 0)
> + if (vref < 0) {
> + dev_err(dev, "Cannot use reference %u. Error:%d\n",
> + reference_select, vref);
> return vref;
> -
> + }
> return vref / (MICRO / MILLI);
> }
>
> @@ -905,13 +909,50 @@ 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,
> + const unsigned int ain[AD7173_NO_AINS_PER_CHANNEL])
I was late to the game in replying to previous thread.
This is neater without the loop and with 2 parameters. Anyhow see reply to v3.
If you end up respining for other reasons consider doing that as well.
Maybe keep the struct device *dev though which I suggested dropping as
looking at the full patch it is more consistent to keep that.
> +{
> + struct device *dev = &st->sd.spi->dev;
> +
> + for (int i = 0; i < AD7173_NO_AINS_PER_CHANNEL; i++) {
> + if (ain[i] < st->info->num_inputs)
> + continue;
> +
> + 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 ret;
> +
> + return 0;
> +}
> +
> static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
> {
> struct ad7173_channel *chans_st_arr, *chan_st_priv;
> struct ad7173_state *st = iio_priv(indio_dev);
> struct device *dev = indio_dev->dev.parent;
> struct iio_chan_spec *chan_arr, *chan;
> - unsigned int ain[2], chan_index = 0;
> + unsigned int ain[AD7173_NO_AINS_PER_CHANNEL], chan_index = 0;
> int ref_sel, ret, num_channels;
>
> num_channels = device_get_child_node_count(dev);
> @@ -965,11 +1006,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",
> @@ -980,19 +1019,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;
>
