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.
Signed-off-by: Dumitru Ceclan <[email protected]>
---
drivers/iio/adc/ad7173.c | 68 +++++++++++++++++++++++++++++++++---------------
1 file changed, 47 insertions(+), 21 deletions(-)
diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
index 8631f218b69e..4040edbd1c32 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))
@@ -629,6 +630,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) {
@@ -652,9 +654,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);
}
@@ -906,13 +910,47 @@ 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 ain0, unsigned int ain1)
+{
+ struct device *dev = &st->sd.spi->dev;
+
+ if (ain0 >= st->info->num_inputs ||
+ ain1 >= st->info->num_inputs)
+ return dev_err_probe(dev, -EINVAL,
+ "Input pin number out of range for pair (%d %d).\n",
+ ain0, ain1);
+
+ 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);
@@ -966,11 +1004,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[0], ain[1]);
+ if (ret)
+ return ret;
ret = fwnode_property_match_property_string(child,
"adi,reference-select",
@@ -981,19 +1017,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 Thu, 2024-06-06 at 19:07 +0300, Dumitru Ceclan via B4 Relay 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.
>
> Signed-off-by: Dumitru Ceclan <[email protected]>
> ---
> drivers/iio/adc/ad7173.c | 68 +++++++++++++++++++++++++++++++++--------------
> -
> 1 file changed, 47 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
> index 8631f218b69e..4040edbd1c32 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))
> @@ -629,6 +630,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) {
> @@ -652,9 +654,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);
> }
unrelated?
- Nuno Sá
On 07/06/2024 12:04, Nuno Sá wrote:
> On Thu, 2024-06-06 at 19:07 +0300, Dumitru Ceclan via B4 Relay 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.
>>
>> Signed-off-by: Dumitru Ceclan <[email protected]>
>> ---
>> drivers/iio/adc/ad7173.c | 68 +++++++++++++++++++++++++++++++++--------------
>> -
>> 1 file changed, 47 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
>> index 8631f218b69e..4040edbd1c32 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))
>> @@ -629,6 +630,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) {
>> @@ -652,9 +654,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);
>> }
>
> unrelated?
>
> - Nuno Sá
>
Hmm, maybe I misunderstood "Any error log needed should be done inside ad7173_get_ref_voltage_milli()"
https://lore.kernel.org/all/[email protected]/
This change should be in a different patch or should it not've been done
this way?
On Fri, 2024-06-07 at 12:37 +0300, Ceclan, Dumitru wrote:
> On 07/06/2024 12:04, Nuno Sá wrote:
> > On Thu, 2024-06-06 at 19:07 +0300, Dumitru Ceclan via B4 Relay 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.
> > >
> > > Signed-off-by: Dumitru Ceclan <[email protected]>
> > > ---
> > > drivers/iio/adc/ad7173.c | 68 +++++++++++++++++++++++++++++++++----------
> > > ----
> > > -
> > > 1 file changed, 47 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
> > > index 8631f218b69e..4040edbd1c32 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))
> > > @@ -629,6 +630,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) {
> > > @@ -652,9 +654,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);
> > > }
> >
> > unrelated?
> >
> > - Nuno Sá
> >
>
> Hmm, maybe I misunderstood "Any error log needed should be done inside
> ad7173_get_ref_voltage_milli()"
> https://lore.kernel.org/all/[email protected]/
>
> This change should be in a different patch or should it not've been done
> this way?
Ohh right... Mentioning this particular log change in the commit message would
avoid my question. Also, note that you're still doing:
ret = ad7173_get_ref_voltage_milli(st, ref_sel);
if (ret < 0)
return ret;
instead of:
return ad7173_get_ref_voltage_milli(...)
which defeats the purpose of having the log inside
ad7173_get_ref_voltage_milli()
But after stepping back I see ad7173_get_ref_voltage_milli() is also being used
in a non probe path. Hence, doing the log out of the function using
dev_err_probe() may be reason enough to keep things as you had before my
comments. Will leave that up to you (sorry for the noise).
- Nuno Sá