| Message ID | 20240527-ad4111-v3-3-7e9eddbbd3eb@analog.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support for AD411x | expand |
On Mon, 2024-05-27 at 20:02 +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. > > Reviewed-by: David Lechner <dlechner@baylibre.com> > Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com> > --- > drivers/iio/adc/ad7173.c | 62 ++++++++++++++++++++++++++++++++++-------------- > 1 file changed, 44 insertions(+), 18 deletions(-) > > diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c > index 9e507e2c66f0..8a53821c8e58 100644 > --- a/drivers/iio/adc/ad7173.c > +++ b/drivers/iio/adc/ad7173.c > @@ -906,6 +906,44 @@ 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 ain[2]) > +{ > + struct device *dev = &st->sd.spi->dev; > + > + for (int i = 0; i < 2; 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 dev_err_probe(dev, ret, "Cannot use reference %u\n", > + ref_sel); > + > + return 0; If you need a v4, I would just 'return ad7173_get_ref_voltage_milli(...)'. Any error log needed should be done inside ad7173_get_ref_voltage_milli(). Anyways: Reviewed-by: Nuno Sa <nuno.sa@analog.com> - Nuno Sá
On Wed, 2024-05-29 at 14:27 +0200, Nuno Sá wrote: > On Mon, 2024-05-27 at 20:02 +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. > > > > Reviewed-by: David Lechner <dlechner@baylibre.com> > > Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com> > > --- > > drivers/iio/adc/ad7173.c | 62 ++++++++++++++++++++++++++++++++++-------------- > > 1 file changed, 44 insertions(+), 18 deletions(-) > > > > diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c > > index 9e507e2c66f0..8a53821c8e58 100644 > > --- a/drivers/iio/adc/ad7173.c > > +++ b/drivers/iio/adc/ad7173.c > > @@ -906,6 +906,44 @@ 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 ain[2]) Pass the pointer and size of it... Also, it should be made 'const' > > +{ > > + struct device *dev = &st->sd.spi->dev; > > + > > + for (int i = 0; i < 2; i++) { Use the size in here... At the very least, ARRAY_SIZE() if you keep it like this. > > + 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 dev_err_probe(dev, ret, "Cannot use reference %u\n", > > + ref_sel); > > + > > + return 0; > > If you need a v4, I would just 'return ad7173_get_ref_voltage_milli(...)'. Any > error > log needed should be done inside ad7173_get_ref_voltage_milli(). Anyways: > > Reviewed-by: Nuno Sa <nuno.sa@analog.com> > In fact, no tag :). Just realized the above in another patch.. - Nuno Sá
On 29/05/2024 15:49, Nuno Sá wrote: > On Wed, 2024-05-29 at 14:27 +0200, Nuno Sá wrote: >> On Mon, 2024-05-27 at 20:02 +0300, Dumitru Ceclan via B4 Relay wrote: >>> From: Dumitru Ceclan <dumitru.ceclan@analog.com> ... >>> +static int ad7173_validate_voltage_ain_inputs(struct ad7173_state *st, >>> + unsigned int ain[2]) > > Pass the pointer and size of it... Also, it should be made 'const' > I'm learning here: what is the purpose of passing the size of it? This is a specific case where the size will always be 2 Also, this will make it awkward in the following patch where I'm using the assumption of size 2 to check if both inputs have a voltage divider ain[(i+1) %2] >>> +{ >>> + struct device *dev = &st->sd.spi->dev; >>> + >>> + for (int i = 0; i < 2; i++) { > > Use the size in here... At the very least, ARRAY_SIZE() if you keep it like this. >
On Thu, 2024-05-30 at 17:45 +0300, Ceclan, Dumitru wrote: > On 29/05/2024 15:49, Nuno Sá wrote: > > On Wed, 2024-05-29 at 14:27 +0200, Nuno Sá wrote: > > > On Mon, 2024-05-27 at 20:02 +0300, Dumitru Ceclan via B4 Relay wrote: > > > > From: Dumitru Ceclan <dumitru.ceclan@analog.com> > > ... > > > > > +static int ad7173_validate_voltage_ain_inputs(struct ad7173_state *st, > > > > + unsigned int ain[2]) > > > > Pass the pointer and size of it... Also, it should be made 'const' > > > > I'm learning here: what is the purpose of passing the size of it? > This is a specific case where the size will always be 2 > Basically readability... Yes, in this case it will be a stretch to assume we'll ever have anything bigger than 2 (so the scalability argument is not so applicable) so I'm ok if you don't pass the size. It's just I really dislike (as a practice) to have raw/magic numbers in the code. In here, it won't be that bad as by the context, one can easily understand the meaning of 2. Nevertheless, I would, still, at the very least consider to either use a #define or a better name for the iterator (anything more meaningful than 'i' so that it looks more understandable than 'i < 2') - Nuno Sá >
On Fri, 31 May 2024 09:10:43 +0200 Nuno Sá <noname.nuno@gmail.com> wrote: > On Thu, 2024-05-30 at 17:45 +0300, Ceclan, Dumitru wrote: > > On 29/05/2024 15:49, Nuno Sá wrote: > > > On Wed, 2024-05-29 at 14:27 +0200, Nuno Sá wrote: > > > > On Mon, 2024-05-27 at 20:02 +0300, Dumitru Ceclan via B4 Relay wrote: > > > > > From: Dumitru Ceclan <dumitru.ceclan@analog.com> > > > > ... > > > > > > > +static int ad7173_validate_voltage_ain_inputs(struct ad7173_state *st, > > > > > + unsigned int ain[2]) > > > > > > Pass the pointer and size of it... Also, it should be made 'const' > > > > > > > I'm learning here: what is the purpose of passing the size of it? > > This is a specific case where the size will always be 2 > > > > Basically readability... Yes, in this case it will be a stretch to assume we'll ever > have anything bigger than 2 (so the scalability argument is not so applicable) so I'm > ok if you don't pass the size. It's just I really dislike (as a practice) to have > raw/magic numbers in the code. In here, it won't be that bad as by the context, one > can easily understand the meaning of 2. Nevertheless, I would, still, at the very > least consider to either use a #define or a better name for the iterator (anything > more meaningful than 'i' so that it looks more understandable than 'i < 2') > I'm late to the game, but I'd just split it into two parameters. Code is shorter as well. static int ad7173_validate_voltage_ain_inputs(struct ad7173_state *st, unsigned int ain0, unsigned int ain1) { if (ain0 >= st->info->num_inputs || ain1 >= st->info->num_inputs) return dev_err_probe(&st->sd.spi->dev, -EINVAL, "Input pin number out of range for pair (%d %d).\n", ain0, ain1); return 0; } > - Nuno Sá > > >
diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c index 9e507e2c66f0..8a53821c8e58 100644 --- a/drivers/iio/adc/ad7173.c +++ b/drivers/iio/adc/ad7173.c @@ -906,6 +906,44 @@ 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 ain[2]) +{ + struct device *dev = &st->sd.spi->dev; + + for (int i = 0; i < 2; 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 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; @@ -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); + 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;