On Fri, 31 May 2024 16:19:32 -0500
David Lechner <dlechner@xxxxxxxxxxxx> wrote:
> This makes use of the new devm_regulator_get_enable_read_voltage()
> function to reduce boilerplate code.
>
> Error messages have changed slightly since there are now fewer places
> where we print an error. The rest of the logic of selecting which
> supply to use as the reference voltage remains the same.
>
> Also 1000 is replaced by MILLI in a few places for consistency.
>
> Signed-off-by: David Lechner <dlechner@xxxxxxxxxxxx>
Ouch diff didn't like this one much and it is a bit hard to read.
One case where I think this has an unintended side effect.
See below.
> ---
> drivers/iio/adc/ad7192.c | 98 +++++++++++++++++-------------------------------
> 1 file changed, 35 insertions(+), 63 deletions(-)
>
> diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
> index 0789121236d6..e08bf066b3f6 100644
> --- a/drivers/iio/adc/ad7192.c
> +++ b/drivers/iio/adc/ad7192.c
> @@ -200,8 +200,6 @@ struct ad7192_chip_info {
>
> struct ad7192_state {
> const struct ad7192_chip_info *chip_info;
> - struct regulator *avdd;
> - struct regulator *vref;
> struct clk *mclk;
> u16 int_vref_mv;
> u32 aincom_mv;
> @@ -1189,17 +1187,11 @@ static const struct ad7192_chip_info ad7192_chip_info_tbl[] = {
> },
> };
>
> -static void ad7192_reg_disable(void *reg)
> -{
> - regulator_disable(reg);
> -}
> -
> static int ad7192_probe(struct spi_device *spi)
> {
> struct ad7192_state *st;
> struct iio_dev *indio_dev;
> - struct regulator *aincom;
> - int ret;
> + int ret, avdd_mv;
>
> if (!spi->irq) {
> dev_err(&spi->dev, "no IRQ?\n");
> @@ -1219,74 +1211,54 @@ static int ad7192_probe(struct spi_device *spi)
> * Newer firmware should provide a zero volt fixed supply if wired to
> * ground.
> */
> - aincom = devm_regulator_get_optional(&spi->dev, "aincom");
> - if (IS_ERR(aincom)) {
> - if (PTR_ERR(aincom) != -ENODEV)
> - return dev_err_probe(&spi->dev, PTR_ERR(aincom),
> - "Failed to get AINCOM supply\n");
> -
> + ret = devm_regulator_get_enable_read_voltage(&spi->dev, "aincom");
> + if (ret == -ENODEV)
> st->aincom_mv = 0;
> - } else {
> - ret = regulator_enable(aincom);
> - if (ret)
> - return dev_err_probe(&spi->dev, ret,
> - "Failed to enable specified AINCOM supply\n");
> + else if (ret < 0)
> + return dev_err_probe(&spi->dev, ret, "Failed to get AINCOM voltage\n");
> + else
> + st->aincom_mv = ret / MILLI;
>
> - ret = devm_add_action_or_reset(&spi->dev, ad7192_reg_disable, aincom);
> + /* AVDD can optionally be used as reference voltage */
> + ret = devm_regulator_get_enable_read_voltage(&spi->dev, "avdd");
> + if (ret == -EINVAL) {
> + /*
> + * We get -EINVAL if avdd is a supply with unknown voltage. We
> + * still need to enable it since it is also a power supply.
> + */
> + ret = devm_regulator_get_enable(&spi->dev, "avdd");
What happens if the entry simply isn't there in the DT.
Previously I think the regulator framework would supply a stub whereas
the devm_regulator_get_enable_read_voltage() returns -ENODEV so isn't
caught by the handling and I think it should be.
> if (ret)
> - return ret;
> -
> - ret = regulator_get_voltage(aincom);
> - if (ret < 0)
> return dev_err_probe(&spi->dev, ret,
> - "Device tree error, AINCOM voltage undefined\n");
> - st->aincom_mv = ret / MILLI;
> - }
> + "Failed to enable AVDD supply\n");
>
> - st->avdd = devm_regulator_get(&spi->dev, "avdd");
> - if (IS_ERR(st->avdd))
> - return PTR_ERR(st->avdd);
> -
> - ret = regulator_enable(st->avdd);
> - if (ret) {
> - dev_err(&spi->dev, "Failed to enable specified AVdd supply\n");
> - return ret;
> + avdd_mv = 0;
> + } else if (ret < 0) {
> + return dev_err_probe(&spi->dev, ret, "Failed to get AVDD voltage\n");
> + } else {
> + avdd_mv = ret / MILLI;
> }
>
> - ret = devm_add_action_or_reset(&spi->dev, ad7192_reg_disable, st->avdd);
> - if (ret)
> - return ret;
>
> ret = devm_regulator_get_enable(&spi->dev, "dvdd");
> if (ret)
> return dev_err_probe(&spi->dev, ret, "Failed to enable specified DVdd supply\n");
>
> - st->vref = devm_regulator_get_optional(&spi->dev, "vref");
> - if (IS_ERR(st->vref)) {
> - if (PTR_ERR(st->vref) != -ENODEV)
> - return PTR_ERR(st->vref);
> -
> - ret = regulator_get_voltage(st->avdd);
> - if (ret < 0)
> - return dev_err_probe(&spi->dev, ret,
> - "Device tree error, AVdd voltage undefined\n");
> + /*
> + * This is either REFIN1 or REFIN2 depending on adi,refin2-pins-enable.
> + * If this supply is not present, fall back to AVDD as reference.
> + */
> + ret = devm_regulator_get_enable_read_voltage(&spi->dev, "vref");
> + if (ret == -ENODEV) {
> + if (avdd_mv == 0)
> + return dev_err_probe(&spi->dev, -ENODEV,
> + "No reference voltage available\n");
> +
> + st->int_vref_mv = avdd_mv;
> + } else if (ret < 0) {
> + return ret;
> } else {
> - ret = regulator_enable(st->vref);
> - if (ret) {
> - dev_err(&spi->dev, "Failed to enable specified Vref supply\n");
> - return ret;
> - }
> -
> - ret = devm_add_action_or_reset(&spi->dev, ad7192_reg_disable, st->vref);
> - if (ret)
> - return ret;
> -
> - ret = regulator_get_voltage(st->vref);
> - if (ret < 0)
> - return dev_err_probe(&spi->dev, ret,
> - "Device tree error, Vref voltage undefined\n");
> + st->int_vref_mv = ret / MILLI;
> }
> - st->int_vref_mv = ret / 1000;
>
> st->chip_info = spi_get_device_match_data(spi);
> indio_dev->name = st->chip_info->name;
>
