| Message ID | 20240607-ad4111-v7-5-97e3855900a0@analog.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Add support for AD411x | expand |
On Fri, 2024-06-07 at 17:53 +0300, Dumitru Ceclan via B4 Relay 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. > > Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com> > --- One minor nit that maybe can be tweaked while applying. Anyways, no need for v8 just because of it: Reviewed-by: Nuno Sa <nuno.sa@analog.com> > drivers/iio/adc/ad7173.c | 62 +++++++++++++++++++++++++++++++++-------------- > - > 1 file changed, 43 insertions(+), 19 deletions(-) > > diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c > index 8631f218b69e..1257303b0cf6 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)) > @@ -906,13 +907,48 @@ 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 dev_err_probe(dev, ret, "Cannot use reference %u\n", > + ref_sel); > + > + return 0; could be return ad7173_get_ref_voltage_milli()... - Nuno Sá
On Mon, 2024-06-10 at 09:23 +0200, Nuno Sá wrote: > On Fri, 2024-06-07 at 17:53 +0300, Dumitru Ceclan via B4 Relay 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. > > > > Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com> > > --- > > One minor nit that maybe can be tweaked while applying. Anyways, no need for > v8 > just because of it: > > Reviewed-by: Nuno Sa <nuno.sa@analog.com> > > > drivers/iio/adc/ad7173.c | 62 +++++++++++++++++++++++++++++++++------------ > > -- > > - > > 1 file changed, 43 insertions(+), 19 deletions(-) > > > > diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c > > index 8631f218b69e..1257303b0cf6 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)) > > @@ -906,13 +907,48 @@ 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 dev_err_probe(dev, ret, "Cannot use reference %u\n", > > + ref_sel); > > + > > + return 0; > > could be return ad7173_get_ref_voltage_milli()... > Bahh, disregard my comment. We went over this in v6 :facepalm: - Nuno Sá
diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c index 8631f218b69e..1257303b0cf6 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)) @@ -906,13 +907,48 @@ 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 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; 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 +1002,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 +1015,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;