Hi David, Thank you very much for fixing it. The patch is OK. Reviewed-by: Marius Cristea <marius.cristea@xxxxxxxxxxxxx> 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@xxxxxxxxxxxx> > --- > > 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@xxxxxxxxxxxx > > (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
