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).
Signed-off-by: David Lechner <dlechner@baylibre.com>
---
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;
- 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;
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)
--
2.43.0
On Wed, 20 Nov 2024 15:33:28 -0600 David Lechner <dlechner@baylibre.com> 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@baylibre.com> 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) >
© 2016 - 2025 Red Hat, Inc.