[2/4] iio: dac: mcp4922: get and enable vdd regulator
Commit Message
The MCP4922 family of chips has a vdd power input, which we
model in our device tree binding for it. The driver should get
and enable the vdd regulator as is appropriate.
Signed-off-by: Nicolas Frattaroli <frattaroli.nicolas@gmail.com>
---
drivers/iio/dac/mcp4922.c | 24 ++++++++++++++++++++----
1 file changed, 20 insertions(+), 4 deletions(-)
Comments
On 4/5/23 07:01, Nicolas Frattaroli wrote:
> [...]
> + state->vdd_reg = devm_regulator_get(&spi->dev, "vdd");
> + if (IS_ERR(state->vdd_reg)) {
> + ret = dev_err_probe(&spi->dev, PTR_ERR(state->vdd_reg),
> + "vdd regulator not specified\n");
> + goto error_disable_vref_reg;
> + }
> + ret = regulator_enable(state->vdd_reg);
> + if (ret) {
> + dev_err(&spi->dev, "Failed to enable vdd regulator: %d\n",
> + ret);
> + goto error_disable_vref_reg;
> + }
The two above can be combined into `devm_regulator_get_enable()`. This
will also take care of automatically disabling the regulator on the
error path and on remove.
> +
> spi_set_drvdata(spi, indio_dev);
> id = spi_get_device_id(spi);
> indio_dev->info = &mcp4922_info;
> [...]
On Wed, 5 Apr 2023 16:01:12 +0200
Nicolas Frattaroli <frattaroli.nicolas@gmail.com> wrote:
> The MCP4922 family of chips has a vdd power input, which we
> model in our device tree binding for it. The driver should get
> and enable the vdd regulator as is appropriate.
>
> Signed-off-by: Nicolas Frattaroli <frattaroli.nicolas@gmail.com>
If, before doing this you add a patch using devm for all the
unwinding currently being done by hand in remove() you can
simplify this further use devm_regulator_get_enable()
(Note you can only do that for this regulator as we never touch it
after enable - more complex handling needed for the vref one as
described in review of patch 4.)
That conversion patch is pretty simple, so whilst I don't like asking
people to implement extra features, in this case the simplifications
to what you are doing here make that precusor work justified
Jonathan
> ---
> drivers/iio/dac/mcp4922.c | 24 ++++++++++++++++++++----
> 1 file changed, 20 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/iio/dac/mcp4922.c b/drivers/iio/dac/mcp4922.c
> index da4327624d45..0b9458cbbcff 100644
> --- a/drivers/iio/dac/mcp4922.c
> +++ b/drivers/iio/dac/mcp4922.c
> @@ -31,6 +31,7 @@ struct mcp4922_state {
> unsigned int value[MCP4922_NUM_CHANNELS];
> unsigned int vref_mv;
> struct regulator *vref_reg;
> + struct regulator *vdd_reg;
> u8 mosi[2] __aligned(IIO_DMA_MINALIGN);
> };
>
> @@ -148,10 +149,23 @@ static int mcp4922_probe(struct spi_device *spi)
> if (ret < 0) {
> dev_err(&spi->dev, "Failed to read vref regulator: %d\n",
> ret);
> - goto error_disable_reg;
> + goto error_disable_vref_reg;
> }
> state->vref_mv = ret / 1000;
>
> + state->vdd_reg = devm_regulator_get(&spi->dev, "vdd");
> + if (IS_ERR(state->vdd_reg)) {
> + ret = dev_err_probe(&spi->dev, PTR_ERR(state->vdd_reg),
> + "vdd regulator not specified\n");
> + goto error_disable_vref_reg;
> + }
> + ret = regulator_enable(state->vdd_reg);
> + if (ret) {
> + dev_err(&spi->dev, "Failed to enable vdd regulator: %d\n",
> + ret);
> + goto error_disable_vref_reg;
> + }
> +
> spi_set_drvdata(spi, indio_dev);
> id = spi_get_device_id(spi);
> indio_dev->info = &mcp4922_info;
> @@ -167,12 +181,13 @@ static int mcp4922_probe(struct spi_device *spi)
> if (ret) {
> dev_err(&spi->dev, "Failed to register iio device: %d\n",
> ret);
> - goto error_disable_reg;
> + goto error_disable_vdd_reg;
> }
>
> return 0;
> -
> -error_disable_reg:
> +error_disable_vdd_reg:
> + regulator_disable(state->vdd_reg);
> +error_disable_vref_reg:
> regulator_disable(state->vref_reg);
>
> return ret;
> @@ -185,6 +200,7 @@ static void mcp4922_remove(struct spi_device *spi)
>
> iio_device_unregister(indio_dev);
> state = iio_priv(indio_dev);
> + regulator_disable(state->vdd_reg);
> regulator_disable(state->vref_reg);
> }
>
On Wed, 5 Apr 2023 07:21:00 -0700
Lars-Peter Clausen <lars@metafoo.de> wrote:
> On 4/5/23 07:01, Nicolas Frattaroli wrote:
> > [...]
> > + state->vdd_reg = devm_regulator_get(&spi->dev, "vdd");
> > + if (IS_ERR(state->vdd_reg)) {
> > + ret = dev_err_probe(&spi->dev, PTR_ERR(state->vdd_reg),
> > + "vdd regulator not specified\n");
> > + goto error_disable_vref_reg;
> > + }
> > + ret = regulator_enable(state->vdd_reg);
> > + if (ret) {
> > + dev_err(&spi->dev, "Failed to enable vdd regulator: %d\n",
> > + ret);
> > + goto error_disable_vref_reg;
> > + }
> The two above can be combined into `devm_regulator_get_enable()`. This
> will also take care of automatically disabling the regulator on the
> error path and on remove.
I'm not keen on the ordering of probe wrt to remove that results from mixing
devm and not. Note that already happens because of the gets vs enables
so another reason to take this driver fully devm_ based.
Jonathan
> > +
> > spi_set_drvdata(spi, indio_dev);
> > id = spi_get_device_id(spi);
> > indio_dev->info = &mcp4922_info;
> > [...]
>
>
@@ -31,6 +31,7 @@ struct mcp4922_state {
unsigned int value[MCP4922_NUM_CHANNELS];
unsigned int vref_mv;
struct regulator *vref_reg;
+ struct regulator *vdd_reg;
u8 mosi[2] __aligned(IIO_DMA_MINALIGN);
};
@@ -148,10 +149,23 @@ static int mcp4922_probe(struct spi_device *spi)
if (ret < 0) {
dev_err(&spi->dev, "Failed to read vref regulator: %d\n",
ret);
- goto error_disable_reg;
+ goto error_disable_vref_reg;
}
state->vref_mv = ret / 1000;
+ state->vdd_reg = devm_regulator_get(&spi->dev, "vdd");
+ if (IS_ERR(state->vdd_reg)) {
+ ret = dev_err_probe(&spi->dev, PTR_ERR(state->vdd_reg),
+ "vdd regulator not specified\n");
+ goto error_disable_vref_reg;
+ }
+ ret = regulator_enable(state->vdd_reg);
+ if (ret) {
+ dev_err(&spi->dev, "Failed to enable vdd regulator: %d\n",
+ ret);
+ goto error_disable_vref_reg;
+ }
+
spi_set_drvdata(spi, indio_dev);
id = spi_get_device_id(spi);
indio_dev->info = &mcp4922_info;
@@ -167,12 +181,13 @@ static int mcp4922_probe(struct spi_device *spi)
if (ret) {
dev_err(&spi->dev, "Failed to register iio device: %d\n",
ret);
- goto error_disable_reg;
+ goto error_disable_vdd_reg;
}
return 0;
-
-error_disable_reg:
+error_disable_vdd_reg:
+ regulator_disable(state->vdd_reg);
+error_disable_vref_reg:
regulator_disable(state->vref_reg);
return ret;
@@ -185,6 +200,7 @@ static void mcp4922_remove(struct spi_device *spi)
iio_device_unregister(indio_dev);
state = iio_priv(indio_dev);
+ regulator_disable(state->vdd_reg);
regulator_disable(state->vref_reg);
}