| Message ID | 20240531-ad4111-v4-3-64607301c057@analog.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add support for AD411x | expand |
On Fri, 31 May 2024 22:42:29 +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 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@baylibre.com> > Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com> > --- > 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 <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 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@baylibre.com> > > Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com> > > --- > > 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 <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 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@baylibre.com> >>> Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com> >>> --- >>> 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 <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 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@baylibre.com> >>>> Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com> >>>> --- >>>> 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 <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 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@baylibre.com> >>>>> Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com> >>>>> --- >>>>> 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 :)
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;