From: Dumitru Ceclan <[email protected]>
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 <[email protected]>
Signed-off-by: Dumitru Ceclan <[email protected]>
---
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])
+{
+ 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;
--
2.43.0
On Fri, 31 May 2024 22:42:29 +0300
Dumitru Ceclan via B4 Relay <[email protected]> wrote:
> From: Dumitru Ceclan <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Dumitru Ceclan <[email protected]>
> ---
> 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;
>
On Sat, 2024-06-01 at 19:49 +0100, Jonathan Cameron wrote:
> On Fri, 31 May 2024 22:42:29 +0300
> Dumitru Ceclan via B4 Relay <[email protected]> wrote:
>
> > From: Dumitru Ceclan <[email protected]>
> >
> > 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 <[email protected]>
> > Signed-off-by: Dumitru Ceclan <[email protected]>
> > ---
> > 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.
>
Yeps, even more given that we're passing/copying the complete array which always
fells awkward to me :)
- Nuno Sá
On 03/06/2024 16:00, Nuno Sá wrote:
> On Sat, 2024-06-01 at 19:49 +0100, Jonathan Cameron wrote:
>> On Fri, 31 May 2024 22:42:29 +0300
>> Dumitru Ceclan via B4 Relay <[email protected]> wrote:
>>
>>> From: Dumitru Ceclan <[email protected]>
>>>
>>> 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 <[email protected]>
>>> Signed-off-by: Dumitru Ceclan <[email protected]>
>>> ---
>>> 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.
>>
>
> Yeps, even more given that we're passing/copying the complete array which always
> fells awkward to me :)
>
> - Nuno Sá
>
>
I rewrote the function, but it feels a bit awkward, perhaps I could get a bit of
advice before sending V5:
static int ad7173_validate_voltage_ain_inputs(struct ad7173_state *st,
unsigned int ain0, unsigned int ain1)
{
struct device *dev = &st->sd.spi->dev;
bool special_input0, special_input1;
special_input0 = ain0 == AD7173_AIN_REF_POS || ain0 == AD7173_AIN_REF_NEG ||
((ain0 == AD7173_AIN_COM_IN_POS || ain0 == AD7173_AIN_COM_IN_NEG) &&
(st->info->has_common_input)) || ain0 == AD4111_VINCOM_INPUT;
special_input1 = (ain1 == AD7173_AIN_REF_POS || ain1 == AD7173_AIN_REF_NEG) ||
((ain1 == AD7173_AIN_COM_IN_POS || ain1 == AD7173_AIN_COM_IN_NEG) &&
(st->info->has_common_input)) || ain1 == AD4111_VINCOM_INPUT;
if (st->info->has_vincom_input) {
if (ain0 == AD4111_VINCOM_INPUT &&
ain1 < st->info->num_voltage_in && /* Normal input */
ain1 >= st->info->num_voltage_in_div) /* Input without divider */
return dev_err_probe(dev, -EINVAL,
"VINCOM must be paired with inputs having divider.\n");
if (ain1 == AD4111_VINCOM_INPUT &&
ain0 < st->info->num_voltage_in && /* Normal input */
ain0 >= st->info->num_voltage_in_div) /* Input without divider */
return dev_err_probe(dev, -EINVAL,
"VINCOM must be paired with inputs having divider.\n");
}
if ((ain0 >= st->info->num_voltage_in && !special_input0) ||
(ain1 >= st->info->num_voltage_in && !special_input1))
return dev_err_probe(dev, -EINVAL,
"Input pin number out of range for pair (%d %d).\n",
ain0, ain1);
if (!special_input0 && !special_input1 &&
((ain0 >= st->info->num_voltage_in_div) !=
(ain1 >= st->info->num_voltage_in_div)))
return dev_err_probe(dev, -EINVAL,
"Both inputs must either have a voltage divider or not have: (%d %d).\n",
ain0, ain1);
return 0;
}
It feels a bit too verbose, but I could not come up with a better way to
incorporate all those cases.
On 6/3/24 8:08 AM, Ceclan, Dumitru wrote:
> On 03/06/2024 16:00, Nuno Sá wrote:
>> On Sat, 2024-06-01 at 19:49 +0100, Jonathan Cameron wrote:
>>> On Fri, 31 May 2024 22:42:29 +0300
>>> Dumitru Ceclan via B4 Relay <[email protected]> wrote:
>>>
>>>> From: Dumitru Ceclan <[email protected]>
>>>>
>>>> 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 <[email protected]>
>>>> Signed-off-by: Dumitru Ceclan <[email protected]>
>>>> ---
>>>> 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.
>>>
>>
>> Yeps, even more given that we're passing/copying the complete array which always
>> fells awkward to me :)
>>
>> - Nuno Sá
>>
>>
>
> I rewrote the function, but it feels a bit awkward, perhaps I could get a bit of
> advice before sending V5:
Maybe we could make this easier to read with macros?
>
> static int ad7173_validate_voltage_ain_inputs(struct ad7173_state *st,
> unsigned int ain0, unsigned int ain1)
> {
> struct device *dev = &st->sd.spi->dev;
> bool special_input0, special_input1;
>
> special_input0 = ain0 == AD7173_AIN_REF_POS || ain0 == AD7173_AIN_REF_NEG ||
> ((ain0 == AD7173_AIN_COM_IN_POS || ain0 == AD7173_AIN_COM_IN_NEG) &&
> (st->info->has_common_input)) || ain0 == AD4111_VINCOM_INPUT;
> special_input1 = (ain1 == AD7173_AIN_REF_POS || ain1 == AD7173_AIN_REF_NEG) ||
> ((ain1 == AD7173_AIN_COM_IN_POS || ain1 == AD7173_AIN_COM_IN_NEG) &&
> (st->info->has_common_input)) || ain1 == AD4111_VINCOM_INPUT;
>
special_input0 = AD7173_IS_SPECIAL_INPUT(ain0);
special_input1 = AD7173_IS_SPECIAL_INPUT(ain1);
> if (st->info->has_vincom_input) {
> if (ain0 == AD4111_VINCOM_INPUT &&
> ain1 < st->info->num_voltage_in && /* Normal input */
> ain1 >= st->info->num_voltage_in_div) /* Input without divider */
> return dev_err_probe(dev, -EINVAL,
> "VINCOM must be paired with inputs having divider.\n");
>
> if (ain1 == AD4111_VINCOM_INPUT &&
> ain0 < st->info->num_voltage_in && /* Normal input */
> ain0 >= st->info->num_voltage_in_div) /* Input without divider */
> return dev_err_probe(dev, -EINVAL,
> "VINCOM must be paired with inputs having divider.\n");
if (AD7173_IS_VINCOM_MISMATCH(ain0, ain1) ||
AD7173_IS_VINCOM_MISMATCH(ain1, ain0)) {
return dev_err_probe(dev, -EINVAL,
"VINCOM must be paired with inputs having divider.\n");
> }
>
> if ((ain0 >= st->info->num_voltage_in && !special_input0) ||
> (ain1 >= st->info->num_voltage_in && !special_input1))
> return dev_err_probe(dev, -EINVAL,
> "Input pin number out of range for pair (%d %d).\n",
> ain0, ain1);
>
> if (!special_input0 && !special_input1 &&
> ((ain0 >= st->info->num_voltage_in_div) !=
> (ain1 >= st->info->num_voltage_in_div)))
> return dev_err_probe(dev, -EINVAL,
> "Both inputs must either have a voltage divider or not have: (%d %d).\n",
> ain0, ain1);
These last two don't seem so bad.
>
> return 0;
> }
>
> It feels a bit too verbose, but I could not come up with a better way to
> incorporate all those cases.
On 03/06/2024 19:00, David Lechner wrote:
> On 6/3/24 8:08 AM, Ceclan, Dumitru wrote:
>> On 03/06/2024 16:00, Nuno Sá wrote:
>>> On Sat, 2024-06-01 at 19:49 +0100, Jonathan Cameron wrote:
>>>> On Fri, 31 May 2024 22:42:29 +0300
>>>> Dumitru Ceclan via B4 Relay <[email protected]> wrote:
>>>>
>>>>> From: Dumitru Ceclan <[email protected]>
>>>>>
>>>>> 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 <[email protected]>
>>>>> Signed-off-by: Dumitru Ceclan <[email protected]>
>>>>> ---
>>>>> 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.
>>>>
>>>
>>> Yeps, even more given that we're passing/copying the complete array which always
>>> fells awkward to me :)
>>>
>>> - Nuno Sá
>>>
>>>
>>
>> I rewrote the function, but it feels a bit awkward, perhaps I could get a bit of
>> advice before sending V5:
>
> Maybe we could make this easier to read with macros?
>
>>
>> static int ad7173_validate_voltage_ain_inputs(struct ad7173_state *st,
>> unsigned int ain0, unsigned int ain1)
>> {
>> struct device *dev = &st->sd.spi->dev;
>> bool special_input0, special_input1;
>>
>> special_input0 = ain0 == AD7173_AIN_REF_POS || ain0 == AD7173_AIN_REF_NEG ||
>> ((ain0 == AD7173_AIN_COM_IN_POS || ain0 == AD7173_AIN_COM_IN_NEG) &&
>> (st->info->has_common_input)) || ain0 == AD4111_VINCOM_INPUT;
>> special_input1 = (ain1 == AD7173_AIN_REF_POS || ain1 == AD7173_AIN_REF_NEG) ||
>> ((ain1 == AD7173_AIN_COM_IN_POS || ain1 == AD7173_AIN_COM_IN_NEG) &&
>> (st->info->has_common_input)) || ain1 == AD4111_VINCOM_INPUT;
>>
>
> special_input0 = AD7173_IS_SPECIAL_INPUT(ain0);
> special_input1 = AD7173_IS_SPECIAL_INPUT(ain1);
>
>> if (st->info->has_vincom_input) {
>> if (ain0 == AD4111_VINCOM_INPUT &&
>> ain1 < st->info->num_voltage_in && /* Normal input */
>> ain1 >= st->info->num_voltage_in_div) /* Input without divider */
>> return dev_err_probe(dev, -EINVAL,
>> "VINCOM must be paired with inputs having divider.\n");
>>
>> if (ain1 == AD4111_VINCOM_INPUT &&
>> ain0 < st->info->num_voltage_in && /* Normal input */
>> ain0 >= st->info->num_voltage_in_div) /* Input without divider */
>> return dev_err_probe(dev, -EINVAL,
>> "VINCOM must be paired with inputs having divider.\n");
>
> if (AD7173_IS_VINCOM_MISMATCH(ain0, ain1) ||
> AD7173_IS_VINCOM_MISMATCH(ain1, ain0)) {
> return dev_err_probe(dev, -EINVAL,
> "VINCOM must be paired with inputs having divider.\n");
>
>> }
>>
>> if ((ain0 >= st->info->num_voltage_in && !special_input0) ||
>> (ain1 >= st->info->num_voltage_in && !special_input1))
>> return dev_err_probe(dev, -EINVAL,
>> "Input pin number out of range for pair (%d %d).\n",
>> ain0, ain1);
>>
>> if (!special_input0 && !special_input1 &&
>> ((ain0 >= st->info->num_voltage_in_div) !=
>> (ain1 >= st->info->num_voltage_in_div)))
>> return dev_err_probe(dev, -EINVAL,
>> "Both inputs must either have a voltage divider or not have: (%d %d).\n",
>> ain0, ain1);
>
> These last two don't seem so bad.
>
>>
Thanks for the quick review :)