[PATCH] usb: i.MX: improve regulator handling

[PATCH] usb: i.MX: improve regulator handling

[Uwe Kleine-König]

Instead of just ignoring errors related to regulator getting error out.
In case there is no regulator in the device tree, regulator_get() returns
the dummy regulator and not an error code, so the change is right for
this situation, too.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/usb/imx/chipidea-imx.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/imx/chipidea-imx.c b/drivers/usb/imx/chipidea-imx.c
index 786beede6d89..dd0e3c1a2a58 100644
--- a/drivers/usb/imx/chipidea-imx.c
+++ b/drivers/usb/imx/chipidea-imx.c
@@ -267,8 +267,11 @@ static int imx_chipidea_probe(struct device_d *dev)
 	}
 
 	ci->vbus = regulator_get(dev, "vbus");
-	if (IS_ERR(ci->vbus))
-		ci->vbus = NULL;
+	if (IS_ERR(ci->vbus)) {
+		ret = ERR_PTR(ci->vbus);
+		dev_err(dev, "Cannot get vbus regulator: %s\n", strerror(-ret));
+		return ret;
+	}
 
 	/*
 	 * Some devices have more than one clock, in this case they are enabled
-- 
2.28.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] usb: i.MX: improve regulator handling

[Marco Felsch]

Hi Uwe,

On 20-10-20 13:15, Uwe Kleine-König wrote:
> Instead of just ignoring errors related to regulator getting error out.
> In case there is no regulator in the device tree, regulator_get() returns
> the dummy regulator and not an error code, so the change is right for
> this situation, too.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/usb/imx/chipidea-imx.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/imx/chipidea-imx.c b/drivers/usb/imx/chipidea-imx.c
> index 786beede6d89..dd0e3c1a2a58 100644
> --- a/drivers/usb/imx/chipidea-imx.c
> +++ b/drivers/usb/imx/chipidea-imx.c
> @@ -267,8 +267,11 @@ static int imx_chipidea_probe(struct device_d *dev)
>  	}
>  
>  	ci->vbus = regulator_get(dev, "vbus");
> -	if (IS_ERR(ci->vbus))
> -		ci->vbus = NULL;
> +	if (IS_ERR(ci->vbus)) {
> +		ret = ERR_PTR(ci->vbus);
> +		dev_err(dev, "Cannot get vbus regulator: %s\n", strerror(-ret));
> +		return ret;

Returning the error here can cause problems with exisiting boards, e.g.
if the regulator support is missing for the specified vbus regulator.
This is often the case since we have very limited regulator support for
now.

Regards,
  Marco

> +	}

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] usb: i.MX: improve regulator handling

[Sascha Hauer]

On Tue, Oct 20, 2020 at 03:33:17PM +0200, Marco Felsch wrote:
> Hi Uwe,
> 
> On 20-10-20 13:15, Uwe Kleine-König wrote:
> > Instead of just ignoring errors related to regulator getting error out.
> > In case there is no regulator in the device tree, regulator_get() returns
> > the dummy regulator and not an error code, so the change is right for
> > this situation, too.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> >  drivers/usb/imx/chipidea-imx.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/usb/imx/chipidea-imx.c b/drivers/usb/imx/chipidea-imx.c
> > index 786beede6d89..dd0e3c1a2a58 100644
> > --- a/drivers/usb/imx/chipidea-imx.c
> > +++ b/drivers/usb/imx/chipidea-imx.c
> > @@ -267,8 +267,11 @@ static int imx_chipidea_probe(struct device_d *dev)
> >  	}
> >  
> >  	ci->vbus = regulator_get(dev, "vbus");
> > -	if (IS_ERR(ci->vbus))
> > -		ci->vbus = NULL;
> > +	if (IS_ERR(ci->vbus)) {
> > +		ret = ERR_PTR(ci->vbus);
> > +		dev_err(dev, "Cannot get vbus regulator: %s\n", strerror(-ret));
> > +		return ret;
> 
> Returning the error here can cause problems with exisiting boards, e.g.
> if the regulator support is missing for the specified vbus regulator.
> This is often the case since we have very limited regulator support for
> now.

But when there is a regulator we also have to control it, right?

Sascha


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] usb: i.MX: improve regulator handling

[Marco Felsch]

On 20-10-20 16:09, Sascha Hauer wrote:
> On Tue, Oct 20, 2020 at 03:33:17PM +0200, Marco Felsch wrote:
> > Hi Uwe,
> > 
> > On 20-10-20 13:15, Uwe Kleine-König wrote:
> > > Instead of just ignoring errors related to regulator getting error out.
> > > In case there is no regulator in the device tree, regulator_get() returns
> > > the dummy regulator and not an error code, so the change is right for
> > > this situation, too.
> > > 
> > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > ---
> > >  drivers/usb/imx/chipidea-imx.c | 7 +++++--
> > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/usb/imx/chipidea-imx.c b/drivers/usb/imx/chipidea-imx.c
> > > index 786beede6d89..dd0e3c1a2a58 100644
> > > --- a/drivers/usb/imx/chipidea-imx.c
> > > +++ b/drivers/usb/imx/chipidea-imx.c
> > > @@ -267,8 +267,11 @@ static int imx_chipidea_probe(struct device_d *dev)
> > >  	}
> > >  
> > >  	ci->vbus = regulator_get(dev, "vbus");
> > > -	if (IS_ERR(ci->vbus))
> > > -		ci->vbus = NULL;
> > > +	if (IS_ERR(ci->vbus)) {
> > > +		ret = ERR_PTR(ci->vbus);
> > > +		dev_err(dev, "Cannot get vbus regulator: %s\n", strerror(-ret));
> > > +		return ret;
> > 
> > Returning the error here can cause problems with exisiting boards, e.g.
> > if the regulator support is missing for the specified vbus regulator.
> > This is often the case since we have very limited regulator support for
> > now.
> 
> But when there is a regulator we also have to control it, right?

So you need to add each regulator driver or worst case you need to add
PMIC drivers. If I remember correctly, I added the same for mci which
broke a lot of boards. Later you reverted those commit. Now Oleksij
added the regulator support for the fec driver and people are starting 
to remove the phy-supply handle from the barebox-dt's (commit 84cf5cfa9a
("ARM: dts: imx6qdl: pfla02: Remove fec phy-supply")). I'm not again
this patch, just wanted to show the consequences of it.

Regards,
  Marco

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] usb: i.MX: improve regulator handling

[Marco Felsch]

On 20-10-20 16:28, Marco Felsch wrote:
> On 20-10-20 16:09, Sascha Hauer wrote:
> > On Tue, Oct 20, 2020 at 03:33:17PM +0200, Marco Felsch wrote:
> > > Hi Uwe,
> > > 
> > > On 20-10-20 13:15, Uwe Kleine-König wrote:
> > > > Instead of just ignoring errors related to regulator getting error out.
> > > > In case there is no regulator in the device tree, regulator_get() returns
> > > > the dummy regulator and not an error code, so the change is right for
> > > > this situation, too.
> > > > 
> > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > > ---
> > > >  drivers/usb/imx/chipidea-imx.c | 7 +++++--
> > > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/usb/imx/chipidea-imx.c b/drivers/usb/imx/chipidea-imx.c
> > > > index 786beede6d89..dd0e3c1a2a58 100644
> > > > --- a/drivers/usb/imx/chipidea-imx.c
> > > > +++ b/drivers/usb/imx/chipidea-imx.c
> > > > @@ -267,8 +267,11 @@ static int imx_chipidea_probe(struct device_d *dev)
> > > >  	}
> > > >  
> > > >  	ci->vbus = regulator_get(dev, "vbus");
> > > > -	if (IS_ERR(ci->vbus))
> > > > -		ci->vbus = NULL;
> > > > +	if (IS_ERR(ci->vbus)) {
> > > > +		ret = ERR_PTR(ci->vbus);
> > > > +		dev_err(dev, "Cannot get vbus regulator: %s\n", strerror(-ret));
> > > > +		return ret;
> > > 
> > > Returning the error here can cause problems with exisiting boards, e.g.
> > > if the regulator support is missing for the specified vbus regulator.
> > > This is often the case since we have very limited regulator support for
> > > now.
> > 
> > But when there is a regulator we also have to control it, right?
> 
> So you need to add each regulator driver or worst case you need to add
> PMIC drivers. If I remember correctly, I added the same for mci which
> broke a lot of boards. Later you reverted those commit. Now Oleksij
> added the regulator support for the fec driver and people are starting 
> to remove the phy-supply handle from the barebox-dt's (commit 84cf5cfa9a
> ("ARM: dts: imx6qdl: pfla02: Remove fec phy-supply")). I'm not again
> this patch, just wanted to show the consequences of it.

Sorry I have to correct myself, pls check the linux driver:

static int ci_get_platdata(struct device *dev,
			   struct ci_hdrc_platform_data *platdata)
{

...

/* Get the vbus regulator */
platdata->reg_vbus = devm_regulator_get_optional(dev, "vbus");
if (PTR_ERR(platdata->reg_vbus) == -EPROBE_DEFER) {
	return -EPROBE_DEFER;
} else if (PTR_ERR(platdata->reg_vbus) == -ENODEV) {
	/* no vbus regulator is needed */
	platdata->reg_vbus = NULL;
} else if (IS_ERR(platdata->reg_vbus)) {
	dev_err(dev, "Getting regulator error: %ld\n",
		PTR_ERR(platdata->reg_vbus));
	return PTR_ERR(platdata->reg_vbus);
}

...

}

So if ENODEV is returned we can assign NULL here and the coming
deep-probe v3 address the -EPROBE_DEFER failure. So I'm not sure if this
patch is correct.

Regards,
  Marco

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] usb: i.MX: improve regulator handling

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 4584 bytes --]

On Tue, Oct 20, 2020 at 04:58:44PM +0200, Marco Felsch wrote:
> On 20-10-20 16:28, Marco Felsch wrote:
> > On 20-10-20 16:09, Sascha Hauer wrote:
> > > On Tue, Oct 20, 2020 at 03:33:17PM +0200, Marco Felsch wrote:
> > > > Hi Uwe,
> > > > 
> > > > On 20-10-20 13:15, Uwe Kleine-König wrote:
> > > > > Instead of just ignoring errors related to regulator getting error out.
> > > > > In case there is no regulator in the device tree, regulator_get() returns
> > > > > the dummy regulator and not an error code, so the change is right for
> > > > > this situation, too.
> > > > > 
> > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > > > ---
> > > > >  drivers/usb/imx/chipidea-imx.c | 7 +++++--
> > > > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/usb/imx/chipidea-imx.c b/drivers/usb/imx/chipidea-imx.c
> > > > > index 786beede6d89..dd0e3c1a2a58 100644
> > > > > --- a/drivers/usb/imx/chipidea-imx.c
> > > > > +++ b/drivers/usb/imx/chipidea-imx.c
> > > > > @@ -267,8 +267,11 @@ static int imx_chipidea_probe(struct device_d *dev)
> > > > >  	}
> > > > >  
> > > > >  	ci->vbus = regulator_get(dev, "vbus");
> > > > > -	if (IS_ERR(ci->vbus))
> > > > > -		ci->vbus = NULL;
> > > > > +	if (IS_ERR(ci->vbus)) {
> > > > > +		ret = ERR_PTR(ci->vbus);
> > > > > +		dev_err(dev, "Cannot get vbus regulator: %s\n", strerror(-ret));
> > > > > +		return ret;
> > > > 
> > > > Returning the error here can cause problems with exisiting boards, e.g.
> > > > if the regulator support is missing for the specified vbus regulator.
> > > > This is often the case since we have very limited regulator support for
> > > > now.
> > > 
> > > But when there is a regulator we also have to control it, right?
> > 
> > So you need to add each regulator driver or worst case you need to add
> > PMIC drivers.

Right, as if you don't the hardware might be off and USB won't work
without a useful error message.

> > If I remember correctly, I added the same for mci which
> > broke a lot of boards. Later you reverted those commit. Now Oleksij
> > added the regulator support for the fec driver and people are starting 
> > to remove the phy-supply handle from the barebox-dt's (commit 84cf5cfa9a
> > ("ARM: dts: imx6qdl: pfla02: Remove fec phy-supply")). I'm not again

Looking at 84cf5cfa9a there is at least a comment missing about why this
property is deleted. Something like

diff --git a/arch/arm/dts/imx6qdl-phytec-pfla02.dtsi b/arch/arm/dts/imx6qdl-phytec-pfla02.dtsi
index b83511cb011f..7a12e2a06be4 100644
--- a/arch/arm/dts/imx6qdl-phytec-pfla02.dtsi
+++ b/arch/arm/dts/imx6qdl-phytec-pfla02.dtsi
@@ -84,6 +84,11 @@
 };
 
 &fec {
+	/*
+	 * barebox doesn't have a driver for the PMIC providing the phy-supply
+	 * (dlg,da9063). So remove the phy-supply property and rely on the
+	 * PMIC's reset default which has this supply enabled.
+	 */
 	/delete-property/ phy-supply;
 };
 

> > this patch, just wanted to show the consequences of it.
> 
> Sorry I have to correct myself, pls check the linux driver:
> 
> static int ci_get_platdata(struct device *dev,
> 			   struct ci_hdrc_platform_data *platdata)
> {
> 
> ...
> 
> /* Get the vbus regulator */
> platdata->reg_vbus = devm_regulator_get_optional(dev, "vbus");
> if (PTR_ERR(platdata->reg_vbus) == -EPROBE_DEFER) {
> 	return -EPROBE_DEFER;
> } else if (PTR_ERR(platdata->reg_vbus) == -ENODEV) {
> 	/* no vbus regulator is needed */
> 	platdata->reg_vbus = NULL;
> } else if (IS_ERR(platdata->reg_vbus)) {
> 	dev_err(dev, "Getting regulator error: %ld\n",
> 		PTR_ERR(platdata->reg_vbus));
> 	return PTR_ERR(platdata->reg_vbus);
> }

The difference between regulator_get and regulator_get_optional is that
the former doesn't return -ENODEV but yields the dummy regulator
instead. (Yes, this is the inversed semantic compared with
gpio_get_optional() and clk_get_optional().) So using
devm_regulator_get_optional and ignoring -ENODEV is kind of strange.  So
I think the above can be simplified to:

	platdata->reg_vbus = devm_regulator_get(dev, "vbus");
	if (IS_ERR(platdata->reg_vbus))
		return dev_err_probe(dev, PTR_ERR(platdata->reg_vbus),
				     "Failed to get vbus regulator\n");
	

and then it is more obvious that my barebox patch does the same.

Am I missing something?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 149 bytes --]

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] usb: i.MX: improve regulator handling

[Sascha Hauer]

On Wed, Oct 21, 2020 at 07:49:07AM +0200, Uwe Kleine-König wrote:
> On Tue, Oct 20, 2020 at 04:58:44PM +0200, Marco Felsch wrote:
> > On 20-10-20 16:28, Marco Felsch wrote:
> > > On 20-10-20 16:09, Sascha Hauer wrote:
> > > > On Tue, Oct 20, 2020 at 03:33:17PM +0200, Marco Felsch wrote:
> > > > > Hi Uwe,
> > > > > 
> > > > > On 20-10-20 13:15, Uwe Kleine-König wrote:
> > > > > > Instead of just ignoring errors related to regulator getting error out.
> > > > > > In case there is no regulator in the device tree, regulator_get() returns
> > > > > > the dummy regulator and not an error code, so the change is right for
> > > > > > this situation, too.
> > > > > > 
> > > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > > > > ---
> > > > > >  drivers/usb/imx/chipidea-imx.c | 7 +++++--
> > > > > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/usb/imx/chipidea-imx.c b/drivers/usb/imx/chipidea-imx.c
> > > > > > index 786beede6d89..dd0e3c1a2a58 100644
> > > > > > --- a/drivers/usb/imx/chipidea-imx.c
> > > > > > +++ b/drivers/usb/imx/chipidea-imx.c
> > > > > > @@ -267,8 +267,11 @@ static int imx_chipidea_probe(struct device_d *dev)
> > > > > >  	}
> > > > > >  
> > > > > >  	ci->vbus = regulator_get(dev, "vbus");
> > > > > > -	if (IS_ERR(ci->vbus))
> > > > > > -		ci->vbus = NULL;
> > > > > > +	if (IS_ERR(ci->vbus)) {
> > > > > > +		ret = ERR_PTR(ci->vbus);
> > > > > > +		dev_err(dev, "Cannot get vbus regulator: %s\n", strerror(-ret));
> > > > > > +		return ret;
> > > > > 
> > > > > Returning the error here can cause problems with exisiting boards, e.g.
> > > > > if the regulator support is missing for the specified vbus regulator.
> > > > > This is often the case since we have very limited regulator support for
> > > > > now.
> > > > 
> > > > But when there is a regulator we also have to control it, right?
> > > 
> > > So you need to add each regulator driver or worst case you need to add
> > > PMIC drivers.
> 
> Right, as if you don't the hardware might be off and USB won't work
> without a useful error message.
> 
> > > If I remember correctly, I added the same for mci which
> > > broke a lot of boards. Later you reverted those commit. Now Oleksij
> > > added the regulator support for the fec driver and people are starting 
> > > to remove the phy-supply handle from the barebox-dt's (commit 84cf5cfa9a
> > > ("ARM: dts: imx6qdl: pfla02: Remove fec phy-supply")). I'm not again
> 
> Looking at 84cf5cfa9a there is at least a comment missing about why this
> property is deleted. Something like
> 
> diff --git a/arch/arm/dts/imx6qdl-phytec-pfla02.dtsi b/arch/arm/dts/imx6qdl-phytec-pfla02.dtsi
> index b83511cb011f..7a12e2a06be4 100644
> --- a/arch/arm/dts/imx6qdl-phytec-pfla02.dtsi
> +++ b/arch/arm/dts/imx6qdl-phytec-pfla02.dtsi
> @@ -84,6 +84,11 @@
>  };
>  
>  &fec {
> +	/*
> +	 * barebox doesn't have a driver for the PMIC providing the phy-supply
> +	 * (dlg,da9063). So remove the phy-supply property and rely on the
> +	 * PMIC's reset default which has this supply enabled.
> +	 */
>  	/delete-property/ phy-supply;
>  };

Yes, such a comment should really be there. I don't like this approach
very much though as this devicetree no longer boots Linux properly.
Just ignoring errors is also not very nice as it means that we hide real
errors. Adding each and every regulator driver seems desirable, but
sometimes just not necessary.

We could add a way for boards to allow a dummy regulator for a specific
device. This could be a function call or maybe a barebox specific devicetree
property.

Sascha


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] usb: i.MX: improve regulator handling

[Sascha Hauer]

On Tue, Oct 20, 2020 at 01:15:37PM +0200, Uwe Kleine-König wrote:
> Instead of just ignoring errors related to regulator getting error out.
> In case there is no regulator in the device tree, regulator_get() returns
> the dummy regulator and not an error code, so the change is right for
> this situation, too.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/usb/imx/chipidea-imx.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/imx/chipidea-imx.c b/drivers/usb/imx/chipidea-imx.c
> index 786beede6d89..dd0e3c1a2a58 100644
> --- a/drivers/usb/imx/chipidea-imx.c
> +++ b/drivers/usb/imx/chipidea-imx.c
> @@ -267,8 +267,11 @@ static int imx_chipidea_probe(struct device_d *dev)
>  	}
>  
>  	ci->vbus = regulator_get(dev, "vbus");
> -	if (IS_ERR(ci->vbus))
> -		ci->vbus = NULL;
> +	if (IS_ERR(ci->vbus)) {
> +		ret = ERR_PTR(ci->vbus);
> +		dev_err(dev, "Cannot get vbus regulator: %s\n", strerror(-ret));
> +		return ret;
> +	}

Ok, as Marco noted we had regressions in similar cases where we didn't
have a driver for the regulator, but knew that it was default enabled.
Therefore I am dropping this patch.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] usb: i.MX: improve regulator handling

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 1852 bytes --]

On Wed, Oct 21, 2020 at 12:05:48PM +0200, Sascha Hauer wrote:
> On Tue, Oct 20, 2020 at 01:15:37PM +0200, Uwe Kleine-König wrote:
> > Instead of just ignoring errors related to regulator getting error out.
> > In case there is no regulator in the device tree, regulator_get() returns
> > the dummy regulator and not an error code, so the change is right for
> > this situation, too.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> >  drivers/usb/imx/chipidea-imx.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/usb/imx/chipidea-imx.c b/drivers/usb/imx/chipidea-imx.c
> > index 786beede6d89..dd0e3c1a2a58 100644
> > --- a/drivers/usb/imx/chipidea-imx.c
> > +++ b/drivers/usb/imx/chipidea-imx.c
> > @@ -267,8 +267,11 @@ static int imx_chipidea_probe(struct device_d *dev)
> >  	}
> >  
> >  	ci->vbus = regulator_get(dev, "vbus");
> > -	if (IS_ERR(ci->vbus))
> > -		ci->vbus = NULL;
> > +	if (IS_ERR(ci->vbus)) {
> > +		ret = ERR_PTR(ci->vbus);
> > +		dev_err(dev, "Cannot get vbus regulator: %s\n", strerror(-ret));
> > +		return ret;
> > +	}
> 
> Ok, as Marco noted we had regressions in similar cases where we didn't
> have a driver for the regulator, but knew that it was default enabled.
> Therefore I am dropping this patch.

So you prefer "fails on some boards with a missleading (or at least
little helpful) error message" over "fails on some (other) boards with
an obvious error indication"?

I think I would have decided this differently. And IMHO then this should
at least result in a runtime warning instead of being silently
optimistic.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 149 bytes --]

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] usb: i.MX: improve regulator handling

[Sascha Hauer]

On Wed, Oct 21, 2020 at 02:03:43PM +0200, Uwe Kleine-König wrote:
> On Wed, Oct 21, 2020 at 12:05:48PM +0200, Sascha Hauer wrote:
> > On Tue, Oct 20, 2020 at 01:15:37PM +0200, Uwe Kleine-König wrote:
> > > Instead of just ignoring errors related to regulator getting error out.
> > > In case there is no regulator in the device tree, regulator_get() returns
> > > the dummy regulator and not an error code, so the change is right for
> > > this situation, too.
> > > 
> > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > ---
> > >  drivers/usb/imx/chipidea-imx.c | 7 +++++--
> > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/usb/imx/chipidea-imx.c b/drivers/usb/imx/chipidea-imx.c
> > > index 786beede6d89..dd0e3c1a2a58 100644
> > > --- a/drivers/usb/imx/chipidea-imx.c
> > > +++ b/drivers/usb/imx/chipidea-imx.c
> > > @@ -267,8 +267,11 @@ static int imx_chipidea_probe(struct device_d *dev)
> > >  	}
> > >  
> > >  	ci->vbus = regulator_get(dev, "vbus");
> > > -	if (IS_ERR(ci->vbus))
> > > -		ci->vbus = NULL;
> > > +	if (IS_ERR(ci->vbus)) {
> > > +		ret = ERR_PTR(ci->vbus);
> > > +		dev_err(dev, "Cannot get vbus regulator: %s\n", strerror(-ret));
> > > +		return ret;
> > > +	}
> > 
> > Ok, as Marco noted we had regressions in similar cases where we didn't
> > have a driver for the regulator, but knew that it was default enabled.
> > Therefore I am dropping this patch.
> 
> So you prefer "fails on some boards with a missleading (or at least
> little helpful) error message" over "fails on some (other) boards with
> an obvious error indication"?
> 
> I think I would have decided this differently. And IMHO then this should
> at least result in a runtime warning instead of being silently
> optimistic.

Have a look at what I suggested in my other mail. With an additional
device tree property saying "barebox: Ignore absent regulator" I would
have a way telling people how to fix the regression. With that
implemented we could merge your patch.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] usb: i.MX: improve regulator handling

[Marco Felsch]

On 20-10-21 07:49, Uwe Kleine-König wrote:
> On Tue, Oct 20, 2020 at 04:58:44PM +0200, Marco Felsch wrote:
> > On 20-10-20 16:28, Marco Felsch wrote:
> > > On 20-10-20 16:09, Sascha Hauer wrote:
> > > > On Tue, Oct 20, 2020 at 03:33:17PM +0200, Marco Felsch wrote:
> > > > > Hi Uwe,
> > > > > 
> > > > > On 20-10-20 13:15, Uwe Kleine-König wrote:
> > > > > > Instead of just ignoring errors related to regulator getting error out.
> > > > > > In case there is no regulator in the device tree, regulator_get() returns
> > > > > > the dummy regulator and not an error code, so the change is right for
> > > > > > this situation, too.
> > > > > > 
> > > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > > > > ---
> > > > > >  drivers/usb/imx/chipidea-imx.c | 7 +++++--
> > > > > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/usb/imx/chipidea-imx.c b/drivers/usb/imx/chipidea-imx.c
> > > > > > index 786beede6d89..dd0e3c1a2a58 100644
> > > > > > --- a/drivers/usb/imx/chipidea-imx.c
> > > > > > +++ b/drivers/usb/imx/chipidea-imx.c
> > > > > > @@ -267,8 +267,11 @@ static int imx_chipidea_probe(struct device_d *dev)
> > > > > >  	}
> > > > > >  
> > > > > >  	ci->vbus = regulator_get(dev, "vbus");
> > > > > > -	if (IS_ERR(ci->vbus))
> > > > > > -		ci->vbus = NULL;
> > > > > > +	if (IS_ERR(ci->vbus)) {
> > > > > > +		ret = ERR_PTR(ci->vbus);
> > > > > > +		dev_err(dev, "Cannot get vbus regulator: %s\n", strerror(-ret));
> > > > > > +		return ret;
> > > > > 
> > > > > Returning the error here can cause problems with exisiting boards, e.g.
> > > > > if the regulator support is missing for the specified vbus regulator.
> > > > > This is often the case since we have very limited regulator support for
> > > > > now.
> > > > 
> > > > But when there is a regulator we also have to control it, right?
> > > 
> > > So you need to add each regulator driver or worst case you need to add
> > > PMIC drivers.
> 
> Right, as if you don't the hardware might be off and USB won't work
> without a useful error message.

Adding a warining seems valid to me but returning a error is wrong since
the mainline driver explicite says: "/* no vbus regulator is needed */".

> > > If I remember correctly, I added the same for mci which
> > > broke a lot of boards. Later you reverted those commit. Now Oleksij
> > > added the regulator support for the fec driver and people are starting 
> > > to remove the phy-supply handle from the barebox-dt's (commit 84cf5cfa9a
> > > ("ARM: dts: imx6qdl: pfla02: Remove fec phy-supply")). I'm not again
> 
> Looking at 84cf5cfa9a there is at least a comment missing about why this
> property is deleted. Something like
> 
> diff --git a/arch/arm/dts/imx6qdl-phytec-pfla02.dtsi b/arch/arm/dts/imx6qdl-phytec-pfla02.dtsi
> index b83511cb011f..7a12e2a06be4 100644
> --- a/arch/arm/dts/imx6qdl-phytec-pfla02.dtsi
> +++ b/arch/arm/dts/imx6qdl-phytec-pfla02.dtsi
> @@ -84,6 +84,11 @@
>  };
>  
>  &fec {
> +	/*
> +	 * barebox doesn't have a driver for the PMIC providing the phy-supply
> +	 * (dlg,da9063). So remove the phy-supply property and rely on the
> +	 * PMIC's reset default which has this supply enabled.
> +	 */
>  	/delete-property/ phy-supply;
>  };
>  
> 
> > > this patch, just wanted to show the consequences of it.
> > 
> > Sorry I have to correct myself, pls check the linux driver:
> > 
> > static int ci_get_platdata(struct device *dev,
> > 			   struct ci_hdrc_platform_data *platdata)
> > {
> > 
> > ...
> > 
> > /* Get the vbus regulator */
> > platdata->reg_vbus = devm_regulator_get_optional(dev, "vbus");
> > if (PTR_ERR(platdata->reg_vbus) == -EPROBE_DEFER) {
> > 	return -EPROBE_DEFER;
> > } else if (PTR_ERR(platdata->reg_vbus) == -ENODEV) {
> > 	/* no vbus regulator is needed */
> > 	platdata->reg_vbus = NULL;
> > } else if (IS_ERR(platdata->reg_vbus)) {
> > 	dev_err(dev, "Getting regulator error: %ld\n",
> > 		PTR_ERR(platdata->reg_vbus));
> > 	return PTR_ERR(platdata->reg_vbus);
> > }
> 
> The difference between regulator_get and regulator_get_optional is that
> the former doesn't return -ENODEV but yields the dummy regulator
> instead. 

Nope, regulator_get returns dummy-regulators too and -ENODEV is returned
in case of regulator_get_optional() too, pls. check:

/* Internal regulator request function */
struct regulator *_regulator_get(struct device *dev, const char *id,
				 enum regulator_get_type get_type)
{
    ...
}

Regards,
  Marco

> (Yes, this is the inversed semantic compared with
> gpio_get_optional() and clk_get_optional().) So using
> devm_regulator_get_optional and ignoring -ENODEV is kind of strange.  So
> I think the above can be simplified to:
> 
> 	platdata->reg_vbus = devm_regulator_get(dev, "vbus");
> 	if (IS_ERR(platdata->reg_vbus))
> 		return dev_err_probe(dev, PTR_ERR(platdata->reg_vbus),
> 				     "Failed to get vbus regulator\n");
> 	
> 
> and then it is more obvious that my barebox patch does the same.
> 
> Am I missing something?
> 
> Best regards
> Uwe
> 
> -- 
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |



> _______________________________________________
> barebox mailing list
> barebox@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/barebox


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] usb: i.MX: improve regulator handling

[Marco Felsch]

On 20-10-21 16:29, Marco Felsch wrote:
> On 20-10-21 07:49, Uwe Kleine-König wrote:
> > On Tue, Oct 20, 2020 at 04:58:44PM +0200, Marco Felsch wrote:
> > > On 20-10-20 16:28, Marco Felsch wrote:
> > > > On 20-10-20 16:09, Sascha Hauer wrote:
> > > > > On Tue, Oct 20, 2020 at 03:33:17PM +0200, Marco Felsch wrote:
> > > > > > Hi Uwe,
> > > > > > 
> > > > > > On 20-10-20 13:15, Uwe Kleine-König wrote:
> > > > > > > Instead of just ignoring errors related to regulator getting error out.
> > > > > > > In case there is no regulator in the device tree, regulator_get() returns
> > > > > > > the dummy regulator and not an error code, so the change is right for
> > > > > > > this situation, too.
> > > > > > > 
> > > > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > > > > > ---
> > > > > > >  drivers/usb/imx/chipidea-imx.c | 7 +++++--
> > > > > > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/usb/imx/chipidea-imx.c b/drivers/usb/imx/chipidea-imx.c
> > > > > > > index 786beede6d89..dd0e3c1a2a58 100644
> > > > > > > --- a/drivers/usb/imx/chipidea-imx.c
> > > > > > > +++ b/drivers/usb/imx/chipidea-imx.c
> > > > > > > @@ -267,8 +267,11 @@ static int imx_chipidea_probe(struct device_d *dev)
> > > > > > >  	}
> > > > > > >  
> > > > > > >  	ci->vbus = regulator_get(dev, "vbus");
> > > > > > > -	if (IS_ERR(ci->vbus))
> > > > > > > -		ci->vbus = NULL;
> > > > > > > +	if (IS_ERR(ci->vbus)) {
> > > > > > > +		ret = ERR_PTR(ci->vbus);
> > > > > > > +		dev_err(dev, "Cannot get vbus regulator: %s\n", strerror(-ret));
> > > > > > > +		return ret;
> > > > > > 
> > > > > > Returning the error here can cause problems with exisiting boards, e.g.
> > > > > > if the regulator support is missing for the specified vbus regulator.
> > > > > > This is often the case since we have very limited regulator support for
> > > > > > now.
> > > > > 
> > > > > But when there is a regulator we also have to control it, right?
> > > > 
> > > > So you need to add each regulator driver or worst case you need to add
> > > > PMIC drivers.
> > 
> > Right, as if you don't the hardware might be off and USB won't work
> > without a useful error message.
> 
> Adding a warining seems valid to me but returning a error is wrong since
> the mainline driver explicite says: "/* no vbus regulator is needed */".
> 
> > > > If I remember correctly, I added the same for mci which
> > > > broke a lot of boards. Later you reverted those commit. Now Oleksij
> > > > added the regulator support for the fec driver and people are starting 
> > > > to remove the phy-supply handle from the barebox-dt's (commit 84cf5cfa9a
> > > > ("ARM: dts: imx6qdl: pfla02: Remove fec phy-supply")). I'm not again
> > 
> > Looking at 84cf5cfa9a there is at least a comment missing about why this
> > property is deleted. Something like
> > 
> > diff --git a/arch/arm/dts/imx6qdl-phytec-pfla02.dtsi b/arch/arm/dts/imx6qdl-phytec-pfla02.dtsi
> > index b83511cb011f..7a12e2a06be4 100644
> > --- a/arch/arm/dts/imx6qdl-phytec-pfla02.dtsi
> > +++ b/arch/arm/dts/imx6qdl-phytec-pfla02.dtsi
> > @@ -84,6 +84,11 @@
> >  };
> >  
> >  &fec {
> > +	/*
> > +	 * barebox doesn't have a driver for the PMIC providing the phy-supply
> > +	 * (dlg,da9063). So remove the phy-supply property and rely on the
> > +	 * PMIC's reset default which has this supply enabled.
> > +	 */
> >  	/delete-property/ phy-supply;
> >  };
> >  
> > 
> > > > this patch, just wanted to show the consequences of it.
> > > 
> > > Sorry I have to correct myself, pls check the linux driver:
> > > 
> > > static int ci_get_platdata(struct device *dev,
> > > 			   struct ci_hdrc_platform_data *platdata)
> > > {
> > > 
> > > ...
> > > 
> > > /* Get the vbus regulator */
> > > platdata->reg_vbus = devm_regulator_get_optional(dev, "vbus");
> > > if (PTR_ERR(platdata->reg_vbus) == -EPROBE_DEFER) {
> > > 	return -EPROBE_DEFER;
> > > } else if (PTR_ERR(platdata->reg_vbus) == -ENODEV) {
> > > 	/* no vbus regulator is needed */
> > > 	platdata->reg_vbus = NULL;
> > > } else if (IS_ERR(platdata->reg_vbus)) {
> > > 	dev_err(dev, "Getting regulator error: %ld\n",
> > > 		PTR_ERR(platdata->reg_vbus));
> > > 	return PTR_ERR(platdata->reg_vbus);
> > > }
> > 
> > The difference between regulator_get and regulator_get_optional is that
> > the former doesn't return -ENODEV but yields the dummy regulator
> > instead. 
> 
> Nope, regulator_get returns dummy-regulators too and -ENODEV is returned
> in case of regulator_get_optional() too, pls. check:
> 
> /* Internal regulator request function */
> struct regulator *_regulator_get(struct device *dev, const char *id,
> 				 enum regulator_get_type get_type)
> {
>     ...
> }

Sorry I read with one eye only...

> > (Yes, this is the inversed semantic compared with
> > gpio_get_optional() and clk_get_optional().) So using
> > devm_regulator_get_optional and ignoring -ENODEV is kind of strange.  So
> > I think the above can be simplified to:
> > 
> > 	platdata->reg_vbus = devm_regulator_get(dev, "vbus");
> > 	if (IS_ERR(platdata->reg_vbus))
> > 		return dev_err_probe(dev, PTR_ERR(platdata->reg_vbus),
> > 				     "Failed to get vbus regulator\n");

This will (re-)introduce the dummy regulator message, pls check commit
782c1c49f3db ("usb: chipidea: core: change vbus-regulator as optional")

Anyway, this is a bit off-topic here.

> > and then it is more obvious that my barebox patch does the same.
> > 
> > Am I missing something?

We don't have dummy regulators and in barebox we need to set it to NULL
instead. A few system integrators also adding supply nodes only to avoid
the dummy regulator message. Technically the regulator nodes most the
time are not required since they relying on their default value, however
those default values are retrieved. Linux has support for a wide range
of regulators and this works fine. In Barebox we don't have this wide
range support and your commit will break existing working boards.

Regards,
  Marco

> > 
> > Best regards
> > Uwe
> > 
> > -- 
> > Pengutronix e.K.                           | Uwe Kleine-König            |
> > Industrial Linux Solutions                 | https://www.pengutronix.de/ |
> 
> 
> 
> > _______________________________________________
> > barebox mailing list
> > barebox@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/barebox
> 
> 
> -- 
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> 
> _______________________________________________
> barebox mailing list
> barebox@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/barebox
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] usb: i.MX: improve regulator handling

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 7527 bytes --]

On Wed, Oct 21, 2020 at 05:28:09PM +0200, Marco Felsch wrote:
> On 20-10-21 16:29, Marco Felsch wrote:
> > On 20-10-21 07:49, Uwe Kleine-König wrote:
> > > On Tue, Oct 20, 2020 at 04:58:44PM +0200, Marco Felsch wrote:
> > > > On 20-10-20 16:28, Marco Felsch wrote:
> > > > > On 20-10-20 16:09, Sascha Hauer wrote:
> > > > > > On Tue, Oct 20, 2020 at 03:33:17PM +0200, Marco Felsch wrote:
> > > > > > > Hi Uwe,
> > > > > > > 
> > > > > > > On 20-10-20 13:15, Uwe Kleine-König wrote:
> > > > > > > > Instead of just ignoring errors related to regulator getting error out.
> > > > > > > > In case there is no regulator in the device tree, regulator_get() returns
> > > > > > > > the dummy regulator and not an error code, so the change is right for
> > > > > > > > this situation, too.
> > > > > > > > 
> > > > > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > > > > > > ---
> > > > > > > >  drivers/usb/imx/chipidea-imx.c | 7 +++++--
> > > > > > > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > > > > > > > 
> > > > > > > > diff --git a/drivers/usb/imx/chipidea-imx.c b/drivers/usb/imx/chipidea-imx.c
> > > > > > > > index 786beede6d89..dd0e3c1a2a58 100644
> > > > > > > > --- a/drivers/usb/imx/chipidea-imx.c
> > > > > > > > +++ b/drivers/usb/imx/chipidea-imx.c
> > > > > > > > @@ -267,8 +267,11 @@ static int imx_chipidea_probe(struct device_d *dev)
> > > > > > > >  	}
> > > > > > > >  
> > > > > > > >  	ci->vbus = regulator_get(dev, "vbus");
> > > > > > > > -	if (IS_ERR(ci->vbus))
> > > > > > > > -		ci->vbus = NULL;
> > > > > > > > +	if (IS_ERR(ci->vbus)) {
> > > > > > > > +		ret = ERR_PTR(ci->vbus);
> > > > > > > > +		dev_err(dev, "Cannot get vbus regulator: %s\n", strerror(-ret));
> > > > > > > > +		return ret;
> > > > > > > 
> > > > > > > Returning the error here can cause problems with exisiting boards, e.g.
> > > > > > > if the regulator support is missing for the specified vbus regulator.
> > > > > > > This is often the case since we have very limited regulator support for
> > > > > > > now.
> > > > > > 
> > > > > > But when there is a regulator we also have to control it, right?
> > > > > 
> > > > > So you need to add each regulator driver or worst case you need to add
> > > > > PMIC drivers.
> > > 
> > > Right, as if you don't the hardware might be off and USB won't work
> > > without a useful error message.
> > 
> > Adding a warining seems valid to me but returning a error is wrong since
> > the mainline driver explicite says: "/* no vbus regulator is needed */".
> > 
> > > > > If I remember correctly, I added the same for mci which
> > > > > broke a lot of boards. Later you reverted those commit. Now Oleksij
> > > > > added the regulator support for the fec driver and people are starting 
> > > > > to remove the phy-supply handle from the barebox-dt's (commit 84cf5cfa9a
> > > > > ("ARM: dts: imx6qdl: pfla02: Remove fec phy-supply")). I'm not again
> > > 
> > > Looking at 84cf5cfa9a there is at least a comment missing about why this
> > > property is deleted. Something like
> > > 
> > > diff --git a/arch/arm/dts/imx6qdl-phytec-pfla02.dtsi b/arch/arm/dts/imx6qdl-phytec-pfla02.dtsi
> > > index b83511cb011f..7a12e2a06be4 100644
> > > --- a/arch/arm/dts/imx6qdl-phytec-pfla02.dtsi
> > > +++ b/arch/arm/dts/imx6qdl-phytec-pfla02.dtsi
> > > @@ -84,6 +84,11 @@
> > >  };
> > >  
> > >  &fec {
> > > +	/*
> > > +	 * barebox doesn't have a driver for the PMIC providing the phy-supply
> > > +	 * (dlg,da9063). So remove the phy-supply property and rely on the
> > > +	 * PMIC's reset default which has this supply enabled.
> > > +	 */
> > >  	/delete-property/ phy-supply;
> > >  };
> > >  
> > > 
> > > > > this patch, just wanted to show the consequences of it.
> > > > 
> > > > Sorry I have to correct myself, pls check the linux driver:
> > > > 
> > > > static int ci_get_platdata(struct device *dev,
> > > > 			   struct ci_hdrc_platform_data *platdata)
> > > > {
> > > > 
> > > > ...
> > > > 
> > > > /* Get the vbus regulator */
> > > > platdata->reg_vbus = devm_regulator_get_optional(dev, "vbus");
> > > > if (PTR_ERR(platdata->reg_vbus) == -EPROBE_DEFER) {
> > > > 	return -EPROBE_DEFER;
> > > > } else if (PTR_ERR(platdata->reg_vbus) == -ENODEV) {
> > > > 	/* no vbus regulator is needed */
> > > > 	platdata->reg_vbus = NULL;
> > > > } else if (IS_ERR(platdata->reg_vbus)) {
> > > > 	dev_err(dev, "Getting regulator error: %ld\n",
> > > > 		PTR_ERR(platdata->reg_vbus));
> > > > 	return PTR_ERR(platdata->reg_vbus);
> > > > }
> > > 
> > > The difference between regulator_get and regulator_get_optional is that
> > > the former doesn't return -ENODEV but yields the dummy regulator
> > > instead. 
> > 
> > Nope, regulator_get returns dummy-regulators too and -ENODEV is returned
> > in case of regulator_get_optional() too, pls. check:
> > 
> > /* Internal regulator request function */
> > struct regulator *_regulator_get(struct device *dev, const char *id,
> > 				 enum regulator_get_type get_type)
> > {
> >     ...
> > }
> 
> Sorry I read with one eye only...
> 
> > > (Yes, this is the inversed semantic compared with
> > > gpio_get_optional() and clk_get_optional().) So using
> > > devm_regulator_get_optional and ignoring -ENODEV is kind of strange.  So
> > > I think the above can be simplified to:
> > > 
> > > 	platdata->reg_vbus = devm_regulator_get(dev, "vbus");
> > > 	if (IS_ERR(platdata->reg_vbus))
> > > 		return dev_err_probe(dev, PTR_ERR(platdata->reg_vbus),
> > > 				     "Failed to get vbus regulator\n");
> 
> This will (re-)introduce the dummy regulator message, pls check commit
> 782c1c49f3db ("usb: chipidea: core: change vbus-regulator as optional")
> 
> Anyway, this is a bit off-topic here.
> 
> > > and then it is more obvious that my barebox patch does the same.
> > > 
> > > Am I missing something?
> 
> We don't have dummy regulators and in barebox we need to set it to NULL
> instead. A few system integrators also adding supply nodes only to avoid
> the dummy regulator message. Technically the regulator nodes most the
> time are not required since they relying on their default value, however
> those default values are retrieved. Linux has support for a wide range
> of regulators and this works fine. In Barebox we don't have this wide
> range support and your commit will break existing working boards.

This sounds more harsh than (IMHO) appropriate. They break because my
suggested change makes it obvious that the support is incomplete. And I
don't think APIs or drivers that are optimistic (huh, there is no way to
satisfy the request, let's assume it works just fine and return a dummy)
are a good idea. That sounds like a temporary measure that became the
long term solution.

In my eyes it's wrong to paper over such things and it's better to check
in a strict way (and so gain proper error messages). And if there are
problems remove the regulator in the device tree (as done in 84cf5cfa9a
but with a proper comment).

Having said that I don't understand the suggested solutions yet that
solve this issue using a property that makes barebox do the right thing
(in the absence of a regulator driver) and still make the dtb suitable
for linux booting.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 149 bytes --]

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox