On Wed, 20 Nov 2024 15:33:28 -0600
David Lechner <dlechner@xxxxxxxxxxxx> wrote:
> Simplify the code by using devm_regulator_get_enable_read_voltage().
>
> A small change in behavior here due to moving the enable to the same
> place as the get: supplies now are enabled before the reset GPIO is
> toggled (which is arguably the correct order).
Agreed ordering is weird before this change so that should be fine.
>
> Signed-off-by: David Lechner <dlechner@xxxxxxxxxxxx>
This raises some questions for me (about the original code)
My gut feeling is drop the reading of the regulator voltage but
maybe I'm missing something.
> ---
> drivers/iio/dac/ad7293.c | 66 ++++++++----------------------------------------
> 1 file changed, 11 insertions(+), 55 deletions(-)
>
> diff --git a/drivers/iio/dac/ad7293.c b/drivers/iio/dac/ad7293.c
> index 1d4032670482..58f7926ec3f3 100644
> --- a/drivers/iio/dac/ad7293.c
> +++ b/drivers/iio/dac/ad7293.c
> @@ -141,8 +141,6 @@ struct ad7293_state {
> /* Protect against concurrent accesses to the device, page selection and data content */
> struct mutex lock;
> struct gpio_desc *gpio_reset;
> - struct regulator *reg_avdd;
> - struct regulator *reg_vdrive;
> u8 page_select;
> u8 data[3] __aligned(IIO_DMA_MINALIGN);
> };
> @@ -777,6 +775,7 @@ static int ad7293_reset(struct ad7293_state *st)
> static int ad7293_properties_parse(struct ad7293_state *st)
> {
> struct spi_device *spi = st->spi;
> + int ret;
>
> st->gpio_reset = devm_gpiod_get_optional(&st->spi->dev, "reset",
> GPIOD_OUT_HIGH);
> @@ -784,24 +783,23 @@ static int ad7293_properties_parse(struct ad7293_state *st)
> return dev_err_probe(&spi->dev, PTR_ERR(st->gpio_reset),
> "failed to get the reset GPIO\n");
>
> - st->reg_avdd = devm_regulator_get(&spi->dev, "avdd");
> - if (IS_ERR(st->reg_avdd))
> - return dev_err_probe(&spi->dev, PTR_ERR(st->reg_avdd),
> + ret = devm_regulator_get_enable_read_voltage(&spi->dev, "avdd");
> + if (ret < 0)
> + return dev_err_probe(&spi->dev, ret,
> "failed to get the AVDD voltage\n");
> + if (ret > 5500000 || ret < 4500000)
> + return -EINVAL;
Why is this a driver problem?
>
> - st->reg_vdrive = devm_regulator_get(&spi->dev, "vdrive");
> - if (IS_ERR(st->reg_vdrive))
> - return dev_err_probe(&spi->dev, PTR_ERR(st->reg_vdrive),
> + ret = devm_regulator_get_enable_read_voltage(&spi->dev, "vdrive");
> + if (ret < 0)
> + return dev_err_probe(&spi->dev, ret,
> "failed to get the VDRIVE voltage\n");
> + if (ret > 5500000 || ret < 1700000)
> + return -EINVAL;
Likewise.
Regulators might be fine and we can't read the voltages. If they are out
of spec, that's a design or board configuration problem.
To keep these in place rather than just dropping them, I'd like more
info on why! May well be in the history but I'm busy / lazy (take
your pick) so won't look today.
> return 0;
> }
>
> -static void ad7293_reg_disable(void *data)
> -{
> - regulator_disable(data);
> -}
> -
> static int ad7293_init(struct ad7293_state *st)
> {
> int ret;
> @@ -816,48 +814,6 @@ static int ad7293_init(struct ad7293_state *st)
> if (ret)
> return ret;
>
> - ret = regulator_enable(st->reg_avdd);
> - if (ret) {
> - dev_err(&spi->dev,
> - "Failed to enable specified AVDD Voltage!\n");
> - return ret;
> - }
> -
> - ret = devm_add_action_or_reset(&spi->dev, ad7293_reg_disable,
> - st->reg_avdd);
> - if (ret)
> - return ret;
> -
> - ret = regulator_enable(st->reg_vdrive);
> - if (ret) {
> - dev_err(&spi->dev,
> - "Failed to enable specified VDRIVE Voltage!\n");
> - return ret;
> - }
> -
> - ret = devm_add_action_or_reset(&spi->dev, ad7293_reg_disable,
> - st->reg_vdrive);
> - if (ret)
> - return ret;
> -
> - ret = regulator_get_voltage(st->reg_avdd);
> - if (ret < 0) {
> - dev_err(&spi->dev, "Failed to read avdd regulator: %d\n", ret);
> - return ret;
> - }
> -
> - if (ret > 5500000 || ret < 4500000)
> - return -EINVAL;
> -
> - ret = regulator_get_voltage(st->reg_vdrive);
> - if (ret < 0) {
> - dev_err(&spi->dev,
> - "Failed to read vdrive regulator: %d\n", ret);
> - return ret;
> - }
> - if (ret > 5500000 || ret < 1700000)
> - return -EINVAL;
> -
> /* Check Chip ID */
> ret = __ad7293_spi_read(st, AD7293_REG_DEVICE_ID, &chip_id);
> if (ret)
>
