* [PATCH] iio: adc: fsl-imx25-gcq: initialize regulators as needed
@ 2021-06-25 7:43 Alexandru Ardelean
2021-07-04 18:01 ` Jonathan Cameron
0 siblings, 1 reply; 4+ messages in thread
From: Alexandru Ardelean @ 2021-06-25 7:43 UTC (permalink / raw)
To: linux-iio, linux-kernel; +Cc: jic23, shawnguo, s.hauer, Alexandru Ardelean
The driver tries to initialize all possible regulators from the DT, then
match the external regulators with each channel and then release all unused
regulators.
We can change the logic a bit to initialize regulators only when at least
one channel needs them.
This change creates a mx25_gcq_ext_regulator_setup() function that is
called only for the external regulators. If there's already a reference to
an external regulator, the function will just exit early with no error.
This way, the driver doesn't need to keep any track of these regulators
during init.
Signed-off-by: Alexandru Ardelean <aardelean@deviqon.com>
---
drivers/iio/adc/fsl-imx25-gcq.c | 57 ++++++++++++++++-----------------
1 file changed, 28 insertions(+), 29 deletions(-)
diff --git a/drivers/iio/adc/fsl-imx25-gcq.c b/drivers/iio/adc/fsl-imx25-gcq.c
index ab5139e911c3..31776f80f847 100644
--- a/drivers/iio/adc/fsl-imx25-gcq.c
+++ b/drivers/iio/adc/fsl-imx25-gcq.c
@@ -172,13 +172,37 @@ static const struct regmap_config mx25_gcq_regconfig = {
.reg_stride = 4,
};
+static int mx25_gcq_ext_regulator_setup(struct device *dev,
+ struct mx25_gcq_priv *priv, u32 refp)
+{
+ char reg_name[12];
+ int ret;
+
+ if (priv->vref[refp])
+ return 0;
+
+ ret = snprintf(reg_name, sizeof(reg_name), "vref-%s",
+ mx25_gcq_refp_names[refp]);
+ if (ret < 0)
+ return ret;
+
+ priv->vref[refp] = devm_regulator_get_optional(dev, reg_name);
+ if (IS_ERR(priv->vref[refp])) {
+ dev_err(dev,
+ "Error, trying to use external voltage reference without a %s regulator.",
+ reg_name);
+ return PTR_ERR(priv->vref[refp]);
+ }
+
+ return 0;
+}
+
static int mx25_gcq_setup_cfgs(struct platform_device *pdev,
struct mx25_gcq_priv *priv)
{
struct device_node *np = pdev->dev.of_node;
struct device_node *child;
struct device *dev = &pdev->dev;
- unsigned int refp_used[4] = {};
int ret, i;
/*
@@ -194,19 +218,6 @@ static int mx25_gcq_setup_cfgs(struct platform_device *pdev,
MX25_ADCQ_CFG_IN(i) |
MX25_ADCQ_CFG_REFN_NGND2);
- /*
- * First get all regulators to store them in channel_vref_mv if
- * necessary. Later we use that information for proper IIO scale
- * information.
- */
- priv->vref[MX25_ADC_REFP_INT] = NULL;
- priv->vref[MX25_ADC_REFP_EXT] =
- devm_regulator_get_optional(&pdev->dev, "vref-ext");
- priv->vref[MX25_ADC_REFP_XP] =
- devm_regulator_get_optional(&pdev->dev, "vref-xp");
- priv->vref[MX25_ADC_REFP_YP] =
- devm_regulator_get_optional(&pdev->dev, "vref-yp");
-
for_each_child_of_node(np, child) {
u32 reg;
u32 refp = MX25_ADCQ_CFG_REFP_INT;
@@ -233,11 +244,10 @@ static int mx25_gcq_setup_cfgs(struct platform_device *pdev,
case MX25_ADC_REFP_EXT:
case MX25_ADC_REFP_XP:
case MX25_ADC_REFP_YP:
- if (IS_ERR(priv->vref[refp])) {
- dev_err(dev, "Error, trying to use external voltage reference without a vref-%s regulator.",
- mx25_gcq_refp_names[refp]);
+ ret = mx25_gcq_ext_regulator_setup(&pdev->dev, priv, refp);
+ if (ret) {
of_node_put(child);
- return PTR_ERR(priv->vref[refp]);
+ return ret;
}
priv->channel_vref_mv[reg] =
regulator_get_voltage(priv->vref[refp]);
@@ -253,8 +263,6 @@ static int mx25_gcq_setup_cfgs(struct platform_device *pdev,
return -EINVAL;
}
- ++refp_used[refp];
-
/*
* Shift the read values to the correct positions within the
* register.
@@ -285,15 +293,6 @@ static int mx25_gcq_setup_cfgs(struct platform_device *pdev,
regmap_write(priv->regs, MX25_ADCQ_CR,
MX25_ADCQ_CR_PDMSK | MX25_ADCQ_CR_QSM_FQS);
- /* Remove unused regulators */
- for (i = 0; i != 4; ++i) {
- if (!refp_used[i]) {
- if (!IS_ERR_OR_NULL(priv->vref[i]))
- devm_regulator_put(priv->vref[i]);
- priv->vref[i] = NULL;
- }
- }
-
return 0;
}
--
2.31.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] iio: adc: fsl-imx25-gcq: initialize regulators as needed
2021-06-25 7:43 [PATCH] iio: adc: fsl-imx25-gcq: initialize regulators as needed Alexandru Ardelean
@ 2021-07-04 18:01 ` Jonathan Cameron
2021-09-09 12:33 ` Alexandru Ardelean
0 siblings, 1 reply; 4+ messages in thread
From: Jonathan Cameron @ 2021-07-04 18:01 UTC (permalink / raw)
To: Alexandru Ardelean; +Cc: linux-iio, linux-kernel, shawnguo, s.hauer
On Fri, 25 Jun 2021 10:43:25 +0300
Alexandru Ardelean <aardelean@deviqon.com> wrote:
> The driver tries to initialize all possible regulators from the DT, then
> match the external regulators with each channel and then release all unused
> regulators.
>
> We can change the logic a bit to initialize regulators only when at least
> one channel needs them.
>
> This change creates a mx25_gcq_ext_regulator_setup() function that is
> called only for the external regulators. If there's already a reference to
> an external regulator, the function will just exit early with no error.
>
> This way, the driver doesn't need to keep any track of these regulators
> during init.
>
> Signed-off-by: Alexandru Ardelean <aardelean@deviqon.com>
whilst I agree this is a bit cleaner, I definitely want to see review from
those more familiar with the device before I take it!
Thanks,
Jonathan
> ---
> drivers/iio/adc/fsl-imx25-gcq.c | 57 ++++++++++++++++-----------------
> 1 file changed, 28 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/iio/adc/fsl-imx25-gcq.c b/drivers/iio/adc/fsl-imx25-gcq.c
> index ab5139e911c3..31776f80f847 100644
> --- a/drivers/iio/adc/fsl-imx25-gcq.c
> +++ b/drivers/iio/adc/fsl-imx25-gcq.c
> @@ -172,13 +172,37 @@ static const struct regmap_config mx25_gcq_regconfig = {
> .reg_stride = 4,
> };
>
> +static int mx25_gcq_ext_regulator_setup(struct device *dev,
> + struct mx25_gcq_priv *priv, u32 refp)
> +{
> + char reg_name[12];
> + int ret;
> +
> + if (priv->vref[refp])
> + return 0;
> +
> + ret = snprintf(reg_name, sizeof(reg_name), "vref-%s",
> + mx25_gcq_refp_names[refp]);
> + if (ret < 0)
> + return ret;
> +
> + priv->vref[refp] = devm_regulator_get_optional(dev, reg_name);
> + if (IS_ERR(priv->vref[refp])) {
> + dev_err(dev,
> + "Error, trying to use external voltage reference without a %s regulator.",
> + reg_name);
> + return PTR_ERR(priv->vref[refp]);
> + }
> +
> + return 0;
> +}
> +
> static int mx25_gcq_setup_cfgs(struct platform_device *pdev,
> struct mx25_gcq_priv *priv)
> {
> struct device_node *np = pdev->dev.of_node;
> struct device_node *child;
> struct device *dev = &pdev->dev;
> - unsigned int refp_used[4] = {};
> int ret, i;
>
> /*
> @@ -194,19 +218,6 @@ static int mx25_gcq_setup_cfgs(struct platform_device *pdev,
> MX25_ADCQ_CFG_IN(i) |
> MX25_ADCQ_CFG_REFN_NGND2);
>
> - /*
> - * First get all regulators to store them in channel_vref_mv if
> - * necessary. Later we use that information for proper IIO scale
> - * information.
> - */
> - priv->vref[MX25_ADC_REFP_INT] = NULL;
> - priv->vref[MX25_ADC_REFP_EXT] =
> - devm_regulator_get_optional(&pdev->dev, "vref-ext");
> - priv->vref[MX25_ADC_REFP_XP] =
> - devm_regulator_get_optional(&pdev->dev, "vref-xp");
> - priv->vref[MX25_ADC_REFP_YP] =
> - devm_regulator_get_optional(&pdev->dev, "vref-yp");
> -
> for_each_child_of_node(np, child) {
> u32 reg;
> u32 refp = MX25_ADCQ_CFG_REFP_INT;
> @@ -233,11 +244,10 @@ static int mx25_gcq_setup_cfgs(struct platform_device *pdev,
> case MX25_ADC_REFP_EXT:
> case MX25_ADC_REFP_XP:
> case MX25_ADC_REFP_YP:
> - if (IS_ERR(priv->vref[refp])) {
> - dev_err(dev, "Error, trying to use external voltage reference without a vref-%s regulator.",
> - mx25_gcq_refp_names[refp]);
> + ret = mx25_gcq_ext_regulator_setup(&pdev->dev, priv, refp);
> + if (ret) {
> of_node_put(child);
> - return PTR_ERR(priv->vref[refp]);
> + return ret;
> }
> priv->channel_vref_mv[reg] =
> regulator_get_voltage(priv->vref[refp]);
> @@ -253,8 +263,6 @@ static int mx25_gcq_setup_cfgs(struct platform_device *pdev,
> return -EINVAL;
> }
>
> - ++refp_used[refp];
> -
> /*
> * Shift the read values to the correct positions within the
> * register.
> @@ -285,15 +293,6 @@ static int mx25_gcq_setup_cfgs(struct platform_device *pdev,
> regmap_write(priv->regs, MX25_ADCQ_CR,
> MX25_ADCQ_CR_PDMSK | MX25_ADCQ_CR_QSM_FQS);
>
> - /* Remove unused regulators */
> - for (i = 0; i != 4; ++i) {
> - if (!refp_used[i]) {
> - if (!IS_ERR_OR_NULL(priv->vref[i]))
> - devm_regulator_put(priv->vref[i]);
> - priv->vref[i] = NULL;
> - }
> - }
> -
> return 0;
> }
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] iio: adc: fsl-imx25-gcq: initialize regulators as needed
2021-07-04 18:01 ` Jonathan Cameron
@ 2021-09-09 12:33 ` Alexandru Ardelean
2021-09-26 15:08 ` Jonathan Cameron
0 siblings, 1 reply; 4+ messages in thread
From: Alexandru Ardelean @ 2021-09-09 12:33 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: linux-iio, Linux Kernel Mailing List, shawnguo, s.hauer
On Sun, 4 Jul 2021 at 20:59, Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Fri, 25 Jun 2021 10:43:25 +0300
> Alexandru Ardelean <aardelean@deviqon.com> wrote:
>
> > The driver tries to initialize all possible regulators from the DT, then
> > match the external regulators with each channel and then release all unused
> > regulators.
> >
> > We can change the logic a bit to initialize regulators only when at least
> > one channel needs them.
> >
> > This change creates a mx25_gcq_ext_regulator_setup() function that is
> > called only for the external regulators. If there's already a reference to
> > an external regulator, the function will just exit early with no error.
> >
> > This way, the driver doesn't need to keep any track of these regulators
> > during init.
> >
> > Signed-off-by: Alexandru Ardelean <aardelean@deviqon.com>
>
> whilst I agree this is a bit cleaner, I definitely want to see review from
> those more familiar with the device before I take it!
ping on this
>
> Thanks,
>
> Jonathan
>
> > ---
> > drivers/iio/adc/fsl-imx25-gcq.c | 57 ++++++++++++++++-----------------
> > 1 file changed, 28 insertions(+), 29 deletions(-)
> >
> > diff --git a/drivers/iio/adc/fsl-imx25-gcq.c b/drivers/iio/adc/fsl-imx25-gcq.c
> > index ab5139e911c3..31776f80f847 100644
> > --- a/drivers/iio/adc/fsl-imx25-gcq.c
> > +++ b/drivers/iio/adc/fsl-imx25-gcq.c
> > @@ -172,13 +172,37 @@ static const struct regmap_config mx25_gcq_regconfig = {
> > .reg_stride = 4,
> > };
> >
> > +static int mx25_gcq_ext_regulator_setup(struct device *dev,
> > + struct mx25_gcq_priv *priv, u32 refp)
> > +{
> > + char reg_name[12];
> > + int ret;
> > +
> > + if (priv->vref[refp])
> > + return 0;
> > +
> > + ret = snprintf(reg_name, sizeof(reg_name), "vref-%s",
> > + mx25_gcq_refp_names[refp]);
> > + if (ret < 0)
> > + return ret;
> > +
> > + priv->vref[refp] = devm_regulator_get_optional(dev, reg_name);
> > + if (IS_ERR(priv->vref[refp])) {
> > + dev_err(dev,
> > + "Error, trying to use external voltage reference without a %s regulator.",
> > + reg_name);
> > + return PTR_ERR(priv->vref[refp]);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > static int mx25_gcq_setup_cfgs(struct platform_device *pdev,
> > struct mx25_gcq_priv *priv)
> > {
> > struct device_node *np = pdev->dev.of_node;
> > struct device_node *child;
> > struct device *dev = &pdev->dev;
> > - unsigned int refp_used[4] = {};
> > int ret, i;
> >
> > /*
> > @@ -194,19 +218,6 @@ static int mx25_gcq_setup_cfgs(struct platform_device *pdev,
> > MX25_ADCQ_CFG_IN(i) |
> > MX25_ADCQ_CFG_REFN_NGND2);
> >
> > - /*
> > - * First get all regulators to store them in channel_vref_mv if
> > - * necessary. Later we use that information for proper IIO scale
> > - * information.
> > - */
> > - priv->vref[MX25_ADC_REFP_INT] = NULL;
> > - priv->vref[MX25_ADC_REFP_EXT] =
> > - devm_regulator_get_optional(&pdev->dev, "vref-ext");
> > - priv->vref[MX25_ADC_REFP_XP] =
> > - devm_regulator_get_optional(&pdev->dev, "vref-xp");
> > - priv->vref[MX25_ADC_REFP_YP] =
> > - devm_regulator_get_optional(&pdev->dev, "vref-yp");
> > -
> > for_each_child_of_node(np, child) {
> > u32 reg;
> > u32 refp = MX25_ADCQ_CFG_REFP_INT;
> > @@ -233,11 +244,10 @@ static int mx25_gcq_setup_cfgs(struct platform_device *pdev,
> > case MX25_ADC_REFP_EXT:
> > case MX25_ADC_REFP_XP:
> > case MX25_ADC_REFP_YP:
> > - if (IS_ERR(priv->vref[refp])) {
> > - dev_err(dev, "Error, trying to use external voltage reference without a vref-%s regulator.",
> > - mx25_gcq_refp_names[refp]);
> > + ret = mx25_gcq_ext_regulator_setup(&pdev->dev, priv, refp);
> > + if (ret) {
> > of_node_put(child);
> > - return PTR_ERR(priv->vref[refp]);
> > + return ret;
> > }
> > priv->channel_vref_mv[reg] =
> > regulator_get_voltage(priv->vref[refp]);
> > @@ -253,8 +263,6 @@ static int mx25_gcq_setup_cfgs(struct platform_device *pdev,
> > return -EINVAL;
> > }
> >
> > - ++refp_used[refp];
> > -
> > /*
> > * Shift the read values to the correct positions within the
> > * register.
> > @@ -285,15 +293,6 @@ static int mx25_gcq_setup_cfgs(struct platform_device *pdev,
> > regmap_write(priv->regs, MX25_ADCQ_CR,
> > MX25_ADCQ_CR_PDMSK | MX25_ADCQ_CR_QSM_FQS);
> >
> > - /* Remove unused regulators */
> > - for (i = 0; i != 4; ++i) {
> > - if (!refp_used[i]) {
> > - if (!IS_ERR_OR_NULL(priv->vref[i]))
> > - devm_regulator_put(priv->vref[i]);
> > - priv->vref[i] = NULL;
> > - }
> > - }
> > -
> > return 0;
> > }
> >
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] iio: adc: fsl-imx25-gcq: initialize regulators as needed
2021-09-09 12:33 ` Alexandru Ardelean
@ 2021-09-26 15:08 ` Jonathan Cameron
0 siblings, 0 replies; 4+ messages in thread
From: Jonathan Cameron @ 2021-09-26 15:08 UTC (permalink / raw)
To: Alexandru Ardelean
Cc: linux-iio, Linux Kernel Mailing List, shawnguo, s.hauer
On Thu, 9 Sep 2021 15:33:31 +0300
Alexandru Ardelean <aardelean@deviqon.com> wrote:
> On Sun, 4 Jul 2021 at 20:59, Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Fri, 25 Jun 2021 10:43:25 +0300
> > Alexandru Ardelean <aardelean@deviqon.com> wrote:
> >
> > > The driver tries to initialize all possible regulators from the DT, then
> > > match the external regulators with each channel and then release all unused
> > > regulators.
> > >
> > > We can change the logic a bit to initialize regulators only when at least
> > > one channel needs them.
> > >
> > > This change creates a mx25_gcq_ext_regulator_setup() function that is
> > > called only for the external regulators. If there's already a reference to
> > > an external regulator, the function will just exit early with no error.
> > >
> > > This way, the driver doesn't need to keep any track of these regulators
> > > during init.
> > >
> > > Signed-off-by: Alexandru Ardelean <aardelean@deviqon.com>
> >
> > whilst I agree this is a bit cleaner, I definitely want to see review from
> > those more familiar with the device before I take it!
>
> ping on this
I'm not keen to have this fall through the cracks as it is a nice cleanup.
So I've applied it. Note minor change as mentioned below.
Applied to the togreg branch of iio.git and pushed out as testing for 0-day
to see if it can find anything we missed.
Thanks,
Jonathan
>
> >
> > Thanks,
> >
> > Jonathan
> >
> > > ---
> > > drivers/iio/adc/fsl-imx25-gcq.c | 57 ++++++++++++++++-----------------
> > > 1 file changed, 28 insertions(+), 29 deletions(-)
> > >
> > > diff --git a/drivers/iio/adc/fsl-imx25-gcq.c b/drivers/iio/adc/fsl-imx25-gcq.c
> > > index ab5139e911c3..31776f80f847 100644
> > > --- a/drivers/iio/adc/fsl-imx25-gcq.c
> > > +++ b/drivers/iio/adc/fsl-imx25-gcq.c
> > > @@ -172,13 +172,37 @@ static const struct regmap_config mx25_gcq_regconfig = {
> > > .reg_stride = 4,
> > > };
> > >
> > > +static int mx25_gcq_ext_regulator_setup(struct device *dev,
> > > + struct mx25_gcq_priv *priv, u32 refp)
> > > +{
> > > + char reg_name[12];
> > > + int ret;
> > > +
> > > + if (priv->vref[refp])
> > > + return 0;
> > > +
> > > + ret = snprintf(reg_name, sizeof(reg_name), "vref-%s",
> > > + mx25_gcq_refp_names[refp]);
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + priv->vref[refp] = devm_regulator_get_optional(dev, reg_name);
> > > + if (IS_ERR(priv->vref[refp])) {
Could be a deferred probe response so to avoid polluting log
return dev_err_probe(dev, PTR_ERR(priv->vref[refp]), ...)
> > > + dev_err(dev,
> > > + "Error, trying to use external voltage reference without a %s regulator.",
> > > + reg_name);
> > > + return PTR_ERR(priv->vref[refp]);
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > static int mx25_gcq_setup_cfgs(struct platform_device *pdev,
> > > struct mx25_gcq_priv *priv)
> > > {
> > > struct device_node *np = pdev->dev.of_node;
> > > struct device_node *child;
> > > struct device *dev = &pdev->dev;
> > > - unsigned int refp_used[4] = {};
> > > int ret, i;
> > >
> > > /*
> > > @@ -194,19 +218,6 @@ static int mx25_gcq_setup_cfgs(struct platform_device *pdev,
> > > MX25_ADCQ_CFG_IN(i) |
> > > MX25_ADCQ_CFG_REFN_NGND2);
> > >
> > > - /*
> > > - * First get all regulators to store them in channel_vref_mv if
> > > - * necessary. Later we use that information for proper IIO scale
> > > - * information.
> > > - */
> > > - priv->vref[MX25_ADC_REFP_INT] = NULL;
> > > - priv->vref[MX25_ADC_REFP_EXT] =
> > > - devm_regulator_get_optional(&pdev->dev, "vref-ext");
> > > - priv->vref[MX25_ADC_REFP_XP] =
> > > - devm_regulator_get_optional(&pdev->dev, "vref-xp");
> > > - priv->vref[MX25_ADC_REFP_YP] =
> > > - devm_regulator_get_optional(&pdev->dev, "vref-yp");
> > > -
> > > for_each_child_of_node(np, child) {
> > > u32 reg;
> > > u32 refp = MX25_ADCQ_CFG_REFP_INT;
> > > @@ -233,11 +244,10 @@ static int mx25_gcq_setup_cfgs(struct platform_device *pdev,
> > > case MX25_ADC_REFP_EXT:
> > > case MX25_ADC_REFP_XP:
> > > case MX25_ADC_REFP_YP:
> > > - if (IS_ERR(priv->vref[refp])) {
> > > - dev_err(dev, "Error, trying to use external voltage reference without a vref-%s regulator.",
> > > - mx25_gcq_refp_names[refp]);
> > > + ret = mx25_gcq_ext_regulator_setup(&pdev->dev, priv, refp);
> > > + if (ret) {
> > > of_node_put(child);
> > > - return PTR_ERR(priv->vref[refp]);
> > > + return ret;
> > > }
> > > priv->channel_vref_mv[reg] =
> > > regulator_get_voltage(priv->vref[refp]);
> > > @@ -253,8 +263,6 @@ static int mx25_gcq_setup_cfgs(struct platform_device *pdev,
> > > return -EINVAL;
> > > }
> > >
> > > - ++refp_used[refp];
> > > -
> > > /*
> > > * Shift the read values to the correct positions within the
> > > * register.
> > > @@ -285,15 +293,6 @@ static int mx25_gcq_setup_cfgs(struct platform_device *pdev,
> > > regmap_write(priv->regs, MX25_ADCQ_CR,
> > > MX25_ADCQ_CR_PDMSK | MX25_ADCQ_CR_QSM_FQS);
> > >
> > > - /* Remove unused regulators */
> > > - for (i = 0; i != 4; ++i) {
> > > - if (!refp_used[i]) {
> > > - if (!IS_ERR_OR_NULL(priv->vref[i]))
> > > - devm_regulator_put(priv->vref[i]);
> > > - priv->vref[i] = NULL;
> > > - }
> > > - }
> > > -
> > > return 0;
> > > }
> > >
> >
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-09-26 15:04 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-25 7:43 [PATCH] iio: adc: fsl-imx25-gcq: initialize regulators as needed Alexandru Ardelean
2021-07-04 18:01 ` Jonathan Cameron
2021-09-09 12:33 ` Alexandru Ardelean
2021-09-26 15:08 ` Jonathan Cameron
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).