diff mbox series

[v2] iio: adc: mcp3564: use devm_regulator_get_enable_read_voltage()

Message ID 20240723-iio-regulator-refactor-round-3-v2-1-ae9291201785@baylibre.com (mailing list archive)
State Accepted
Headers show
Series [v2] iio: adc: mcp3564: use devm_regulator_get_enable_read_voltage() | expand

Commit Message

David Lechner July 23, 2024, 6:59 p.m. UTC 
This makes use of the new devm_regulator_get_enable_read_voltage()
helper function to reduce boilerplate code in the MCP3564 ADC driver.

The error message is slightly changed since there are fewer error
return paths.

Setting adc->vref_mv is consolidated into a single place to make the
logic easier to follow.

A use_internal_vref_attr local variable is added to make it more
obvious what the difference between the two iio info structures is.

The return value of the "Unknown Vref" dev_err_probe() is hard-coded to
-ENODEV instead of ret since it was getting a bit far from where ret
was set and logically that is the only value it could have.

Signed-off-by: David Lechner <dlechner@baylibre.com>
---

v2 changes:
* Rename use_auto_zeroing_ref_attr to use_internal_vref_attr
* Link to v1: https://lore.kernel.org/r/20240712-iio-regulator-refactor-round-3-v1-0-835017bae43d@baylibre.com

(other patches from v1 were already applied, so not included in v2)

---
 drivers/iio/adc/mcp3564.c | 54 ++++++++++++++---------------------------------
 1 file changed, 16 insertions(+), 38 deletions(-)


---
base-commit: 472438c7e0e2261c6737a8321f46ef176eef1c8f
change-id: 20240712-iio-regulator-refactor-round-3-17f2a82d2181

Comments

marius.cristea@microchip.com July 26, 2024, 1:50 p.m. UTC | #1 
Hi David,

	Thank you very much for fixing it. The patch is OK.

Reviewed-by: Marius Cristea <marius.cristea@microchip.com>


Thanks,
Marius


On Tue, 2024-07-23 at 13:59 -0500, David Lechner wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> This makes use of the new devm_regulator_get_enable_read_voltage()
> helper function to reduce boilerplate code in the MCP3564 ADC driver.
> 
> The error message is slightly changed since there are fewer error
> return paths.
> 
> Setting adc->vref_mv is consolidated into a single place to make the
> logic easier to follow.
> 
> A use_internal_vref_attr local variable is added to make it more
> obvious what the difference between the two iio info structures is.
> 
> The return value of the "Unknown Vref" dev_err_probe() is hard-coded
> to
> -ENODEV instead of ret since it was getting a bit far from where ret
> was set and logically that is the only value it could have.
> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>
> ---
> 
> v2 changes:
> * Rename use_auto_zeroing_ref_attr to use_internal_vref_attr
> * Link to v1:
> https://lore.kernel.org/r/20240712-iio-regulator-refactor-round-3-v1-0-835017bae43d@baylibre.com
> 
> (other patches from v1 were already applied, so not included in v2)
> 
> ---
>  drivers/iio/adc/mcp3564.c | 54 ++++++++++++++-----------------------
> ----------
>  1 file changed, 16 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/iio/adc/mcp3564.c b/drivers/iio/adc/mcp3564.c
> index d83bed0e63d2..a68f1cd6883e 100644
> --- a/drivers/iio/adc/mcp3564.c
> +++ b/drivers/iio/adc/mcp3564.c
> @@ -349,8 +349,6 @@ struct mcp3564_chip_info {
>   * struct mcp3564_state - working data for a ADC device
>   * @chip_info:         chip specific data
>   * @spi:               SPI device structure
> - * @vref:              the regulator device used as a voltage
> reference in case
> - *                     external voltage reference is used
>   * @vref_mv:           voltage reference value in miliVolts
>   * @lock:              synchronize access to driver's state members
>   * @dev_addr:          hardware device address
> @@ -369,7 +367,6 @@ struct mcp3564_chip_info {
>  struct mcp3564_state {
>         const struct mcp3564_chip_info  *chip_info;
>         struct spi_device               *spi;
> -       struct regulator                *vref;
>         unsigned short                  vref_mv;
>         struct mutex                    lock; /* Synchronize access
> to driver's state members */
>         u8                              dev_addr;
> @@ -1085,11 +1082,6 @@ static int mcp3564_parse_fw_children(struct
> iio_dev *indio_dev)
>         return 0;
>  }
> 
> -static void mcp3564_disable_reg(void *reg)
> -{
> -       regulator_disable(reg);
> -}
> -
>  static void mcp3564_fill_scale_tbls(struct mcp3564_state *adc)
>  {
>         unsigned int pow = adc->chip_info->resolution - 1;
> @@ -1110,7 +1102,7 @@ static void mcp3564_fill_scale_tbls(struct
> mcp3564_state *adc)
>         }
>  }
> 
> -static int mcp3564_config(struct iio_dev *indio_dev)
> +static int mcp3564_config(struct iio_dev *indio_dev, bool
> *use_internal_vref_attr)
>  {
>         struct mcp3564_state *adc = iio_priv(indio_dev);
>         struct device *dev = &adc->spi->dev;
> @@ -1119,6 +1111,7 @@ static int mcp3564_config(struct iio_dev
> *indio_dev)
>         enum mcp3564_ids ids;
>         int ret = 0;
>         unsigned int tmp = 0x01;
> +       bool internal_vref;
>         bool err = false;
> 
>         /*
> @@ -1218,36 +1211,22 @@ static int mcp3564_config(struct iio_dev
> *indio_dev)
> 
>         dev_dbg(dev, "Found %s chip\n", adc->chip_info->name);
> 
> -       adc->vref = devm_regulator_get_optional(dev, "vref");
> -       if (IS_ERR(adc->vref)) {
> -               if (PTR_ERR(adc->vref) != -ENODEV)
> -                       return dev_err_probe(dev, PTR_ERR(adc->vref),
> -                                            "failed to get
> regulator\n");
> +       ret = devm_regulator_get_enable_read_voltage(dev, "vref");
> +       if (ret < 0 && ret != -ENODEV)
> +               return dev_err_probe(dev, ret, "Failed to get vref
> voltage\n");
> +
> +       internal_vref = ret == -ENODEV;
> +       adc->vref_mv = internal_vref ? MCP3564R_INT_VREF_MV : ret /
> MILLI;
> +       *use_internal_vref_attr = internal_vref;
> 
> +       if (internal_vref) {
>                 /* Check if chip has internal vref */
>                 if (!adc->have_vref)
> -                       return dev_err_probe(dev, PTR_ERR(adc->vref),
> -                                            "Unknown Vref\n");
> -               adc->vref = NULL;
> +                       return dev_err_probe(dev, -ENODEV, "Unknown
> Vref\n");
> +
>                 dev_dbg(dev, "%s: Using internal Vref\n", __func__);
>         } else {
> -               ret = regulator_enable(adc->vref);
> -               if (ret)
> -                       return ret;
> -
> -               ret = devm_add_action_or_reset(dev,
> mcp3564_disable_reg,
> -                                              adc->vref);
> -               if (ret)
> -                       return ret;
> -
>                 dev_dbg(dev, "%s: Using External Vref\n", __func__);
> -
> -               ret = regulator_get_voltage(adc->vref);
> -               if (ret < 0)
> -                       return dev_err_probe(dev, ret,
> -                                            "Failed to read vref
> regulator\n");
> -
> -               adc->vref_mv = ret / MILLI;
>         }
> 
>         ret = mcp3564_parse_fw_children(indio_dev);
> @@ -1350,10 +1329,8 @@ static int mcp3564_config(struct iio_dev
> *indio_dev)
>         tmp_reg |= FIELD_PREP(MCP3564_CONFIG0_CLK_SEL_MASK,
> MCP3564_CONFIG0_USE_INT_CLK);
>         tmp_reg |= MCP3456_CONFIG0_BIT6_DEFAULT;
> 
> -       if (!adc->vref) {
> +       if (internal_vref)
>                 tmp_reg |= FIELD_PREP(MCP3456_CONFIG0_VREF_MASK, 1);
> -               adc->vref_mv = MCP3564R_INT_VREF_MV;
> -       }
> 
>         ret = mcp3564_write_8bits(adc, MCP3564_CONFIG0_REG, tmp_reg);
> 
> @@ -1412,6 +1389,7 @@ static int mcp3564_probe(struct spi_device
> *spi)
>         int ret;
>         struct iio_dev *indio_dev;
>         struct mcp3564_state *adc;
> +       bool use_internal_vref_attr;
> 
>         indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*adc));
>         if (!indio_dev)
> @@ -1428,7 +1406,7 @@ static int mcp3564_probe(struct spi_device
> *spi)
>          * enable/disable certain channels
>          * change the sampling rate to the requested value
>          */
> -       ret = mcp3564_config(indio_dev);
> +       ret = mcp3564_config(indio_dev, &use_internal_vref_attr);
>         if (ret)
>                 return dev_err_probe(&spi->dev, ret,
>                                      "Can't configure MCP356X
> device\n");
> @@ -1440,7 +1418,7 @@ static int mcp3564_probe(struct spi_device
> *spi)
>         indio_dev->name = adc->chip_info->name;
>         indio_dev->modes = INDIO_DIRECT_MODE;
> 
> -       if (!adc->vref)
> +       if (use_internal_vref_attr)
>                 indio_dev->info = &mcp3564r_info;
>         else
>                 indio_dev->info = &mcp3564_info;
> 
> ---
> base-commit: 472438c7e0e2261c6737a8321f46ef176eef1c8f
> change-id: 20240712-iio-regulator-refactor-round-3-17f2a82d2181
Jonathan Cameron July 27, 2024, 1:52 p.m. UTC | #2 
On Fri, 26 Jul 2024 13:50:15 +0000
<Marius.Cristea@microchip.com> wrote:

> Hi David,
> 
> 	Thank you very much for fixing it. The patch is OK.
> 
> Reviewed-by: Marius Cristea <marius.cristea@microchip.com>
> 
Applied.

Thanks,

Jonathan
diff mbox series

Patch

diff --git a/drivers/iio/adc/mcp3564.c b/drivers/iio/adc/mcp3564.c
index d83bed0e63d2..a68f1cd6883e 100644
--- a/drivers/iio/adc/mcp3564.c
+++ b/drivers/iio/adc/mcp3564.c
@@ -349,8 +349,6 @@  struct mcp3564_chip_info {
  * struct mcp3564_state - working data for a ADC device
  * @chip_info:		chip specific data
  * @spi:		SPI device structure
- * @vref:		the regulator device used as a voltage reference in case
- *			external voltage reference is used
  * @vref_mv:		voltage reference value in miliVolts
  * @lock:		synchronize access to driver's state members
  * @dev_addr:		hardware device address
@@ -369,7 +367,6 @@  struct mcp3564_chip_info {
 struct mcp3564_state {
 	const struct mcp3564_chip_info	*chip_info;
 	struct spi_device		*spi;
-	struct regulator		*vref;
 	unsigned short			vref_mv;
 	struct mutex			lock; /* Synchronize access to driver's state members */
 	u8				dev_addr;
@@ -1085,11 +1082,6 @@  static int mcp3564_parse_fw_children(struct iio_dev *indio_dev)
 	return 0;
 }
 
-static void mcp3564_disable_reg(void *reg)
-{
-	regulator_disable(reg);
-}
-
 static void mcp3564_fill_scale_tbls(struct mcp3564_state *adc)
 {
 	unsigned int pow = adc->chip_info->resolution - 1;
@@ -1110,7 +1102,7 @@  static void mcp3564_fill_scale_tbls(struct mcp3564_state *adc)
 	}
 }
 
-static int mcp3564_config(struct iio_dev *indio_dev)
+static int mcp3564_config(struct iio_dev *indio_dev, bool *use_internal_vref_attr)
 {
 	struct mcp3564_state *adc = iio_priv(indio_dev);
 	struct device *dev = &adc->spi->dev;
@@ -1119,6 +1111,7 @@  static int mcp3564_config(struct iio_dev *indio_dev)
 	enum mcp3564_ids ids;
 	int ret = 0;
 	unsigned int tmp = 0x01;
+	bool internal_vref;
 	bool err = false;
 
 	/*
@@ -1218,36 +1211,22 @@  static int mcp3564_config(struct iio_dev *indio_dev)
 
 	dev_dbg(dev, "Found %s chip\n", adc->chip_info->name);
 
-	adc->vref = devm_regulator_get_optional(dev, "vref");
-	if (IS_ERR(adc->vref)) {
-		if (PTR_ERR(adc->vref) != -ENODEV)
-			return dev_err_probe(dev, PTR_ERR(adc->vref),
-					     "failed to get regulator\n");
+	ret = devm_regulator_get_enable_read_voltage(dev, "vref");
+	if (ret < 0 && ret != -ENODEV)
+		return dev_err_probe(dev, ret, "Failed to get vref voltage\n");
+
+	internal_vref = ret == -ENODEV;
+	adc->vref_mv = internal_vref ? MCP3564R_INT_VREF_MV : ret / MILLI;
+	*use_internal_vref_attr = internal_vref;
 
+	if (internal_vref) {
 		/* Check if chip has internal vref */
 		if (!adc->have_vref)
-			return dev_err_probe(dev, PTR_ERR(adc->vref),
-					     "Unknown Vref\n");
-		adc->vref = NULL;
+			return dev_err_probe(dev, -ENODEV, "Unknown Vref\n");
+
 		dev_dbg(dev, "%s: Using internal Vref\n", __func__);
 	} else {
-		ret = regulator_enable(adc->vref);
-		if (ret)
-			return ret;
-
-		ret = devm_add_action_or_reset(dev, mcp3564_disable_reg,
-					       adc->vref);
-		if (ret)
-			return ret;
-
 		dev_dbg(dev, "%s: Using External Vref\n", __func__);
-
-		ret = regulator_get_voltage(adc->vref);
-		if (ret < 0)
-			return dev_err_probe(dev, ret,
-					     "Failed to read vref regulator\n");
-
-		adc->vref_mv = ret / MILLI;
 	}
 
 	ret = mcp3564_parse_fw_children(indio_dev);
@@ -1350,10 +1329,8 @@  static int mcp3564_config(struct iio_dev *indio_dev)
 	tmp_reg |= FIELD_PREP(MCP3564_CONFIG0_CLK_SEL_MASK, MCP3564_CONFIG0_USE_INT_CLK);
 	tmp_reg |= MCP3456_CONFIG0_BIT6_DEFAULT;
 
-	if (!adc->vref) {
+	if (internal_vref)
 		tmp_reg |= FIELD_PREP(MCP3456_CONFIG0_VREF_MASK, 1);
-		adc->vref_mv = MCP3564R_INT_VREF_MV;
-	}
 
 	ret = mcp3564_write_8bits(adc, MCP3564_CONFIG0_REG, tmp_reg);
 
@@ -1412,6 +1389,7 @@  static int mcp3564_probe(struct spi_device *spi)
 	int ret;
 	struct iio_dev *indio_dev;
 	struct mcp3564_state *adc;
+	bool use_internal_vref_attr;
 
 	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*adc));
 	if (!indio_dev)
@@ -1428,7 +1406,7 @@  static int mcp3564_probe(struct spi_device *spi)
 	 * enable/disable certain channels
 	 * change the sampling rate to the requested value
 	 */
-	ret = mcp3564_config(indio_dev);
+	ret = mcp3564_config(indio_dev, &use_internal_vref_attr);
 	if (ret)
 		return dev_err_probe(&spi->dev, ret,
 				     "Can't configure MCP356X device\n");
@@ -1440,7 +1418,7 @@  static int mcp3564_probe(struct spi_device *spi)
 	indio_dev->name = adc->chip_info->name;
 	indio_dev->modes = INDIO_DIRECT_MODE;
 
-	if (!adc->vref)
+	if (use_internal_vref_attr)
 		indio_dev->info = &mcp3564r_info;
 	else
 		indio_dev->info = &mcp3564_info;