| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <5d84869757beea84ca9b60daf3c1b4ad1f05b9ec.camel@microchip.com>
Date: Fri, 26 Jul 2024 13:50:15 +0000
From: <Marius.Cristea@...rochip.com>
To: <jic23@...nel.org>, <dlechner@...libre.com>
CC: <lgirdwood@...il.com>, <broonie@...nel.org>,
<linux-kernel@...r.kernel.org>, <linux-iio@...r.kernel.org>
Subject: Re: [PATCH v2] iio: adc: mcp3564: use
devm_regulator_get_enable_read_voltage()
Hi David,
Thank you very much for fixing it. The patch is OK.
Reviewed-by: Marius Cristea <marius.cristea@...rochip.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@...libre.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
Powered by blists - more mailing lists