| Message ID | 20240606-ad4111-v6-5-573981fb3e2e@analog.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add support for AD411x | expand |
On Thu, 2024-06-06 at 19:07 +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> > --- > 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 <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> >> --- >> 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/71452f6882efe6a181d477914488617d28a38e2f.camel@gmail.com/ 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 <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> > > > --- > > > 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/71452f6882efe6a181d477914488617d28a38e2f.camel@gmail.com/ > > 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á
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;