[PATCH] omapfb: Fix regulator API abuse in dss.c and hdmi4.c

[PATCH] omapfb: Fix regulator API abuse in dss.c and hdmi4.c

[Mark Brown]

The voltage changing code in this driver is broken and should be
removed.  The driver sets a single, exact voltage on probe.  Unless
there is a very good reason for this (which should be documented in
comments) constraints like this need to be set via the machine
constraints, voltage setting in a driver is expected to be used in cases
where the voltage varies at runtime.

In addition client drivers should almost never be calling
regulator_can_set_voltage(), if the device needs to set a voltage it
needs to set the voltage and the regulator core will handle the case
where the regulator is fixed voltage.  If the driver can skip setting
the voltage it should just never set the voltage.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/video/fbdev/omap2/omapfb/dss/dsi.c   | 9 ---------
 drivers/video/fbdev/omap2/omapfb/dss/hdmi4.c | 9 ---------
 2 files changed, 18 deletions(-)

diff --git a/drivers/video/fbdev/omap2/omapfb/dss/dsi.c b/drivers/video/fbdev/omap2/omapfb/dss/dsi.c
index 0eec073b3919..cfd0e3d5f36a 100644
--- a/drivers/video/fbdev/omap2/omapfb/dss/dsi.c
+++ b/drivers/video/fbdev/omap2/omapfb/dss/dsi.c
@@ -1180,15 +1180,6 @@ static int dsi_regulator_init(struct platform_device *dsidev)
 		return PTR_ERR(vdds_dsi);
 	}
 
-	if (regulator_can_change_voltage(vdds_dsi)) {
-		r = regulator_set_voltage(vdds_dsi, 1800000, 1800000);
-		if (r) {
-			devm_regulator_put(vdds_dsi);
-			DSSERR("can't set the DSI regulator voltage\n");
-			return r;
-		}
-	}
-
 	dsi->vdds_dsi_reg = vdds_dsi;
 
 	return 0;
diff --git a/drivers/video/fbdev/omap2/omapfb/dss/hdmi4.c b/drivers/video/fbdev/omap2/omapfb/dss/hdmi4.c
index 7103c659a534..68b5ce1610ea 100644
--- a/drivers/video/fbdev/omap2/omapfb/dss/hdmi4.c
+++ b/drivers/video/fbdev/omap2/omapfb/dss/hdmi4.c
@@ -114,15 +114,6 @@ static int hdmi_init_regulator(void)
 		return PTR_ERR(reg);
 	}
 
-	if (regulator_can_change_voltage(reg)) {
-		r = regulator_set_voltage(reg, 1800000, 1800000);
-		if (r) {
-			devm_regulator_put(reg);
-			DSSWARN("can't set the regulator voltage\n");
-			return r;
-		}
-	}
-
 	hdmi.vdda_reg = reg;
 
 	return 0;
-- 
2.8.0.rc3

Re: [PATCH] omapfb: Fix regulator API abuse in dss.c and hdmi4.c

[Tomi Valkeinen]

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

Hi Mark,

On 30/03/16 19:29, Mark Brown wrote:
> The voltage changing code in this driver is broken and should be
> removed.  The driver sets a single, exact voltage on probe.  Unless
> there is a very good reason for this (which should be documented in
> comments) constraints like this need to be set via the machine
> constraints, voltage setting in a driver is expected to be used in cases
> where the voltage varies at runtime.
> 
> In addition client drivers should almost never be calling
> regulator_can_set_voltage(), if the device needs to set a voltage it
> needs to set the voltage and the regulator core will handle the case
> where the regulator is fixed voltage.  If the driver can skip setting
> the voltage it should just never set the voltage.
> 
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
>  drivers/video/fbdev/omap2/omapfb/dss/dsi.c   | 9 ---------
>  drivers/video/fbdev/omap2/omapfb/dss/hdmi4.c | 9 ---------
>  2 files changed, 18 deletions(-)
> 
> diff --git a/drivers/video/fbdev/omap2/omapfb/dss/dsi.c b/drivers/video/fbdev/omap2/omapfb/dss/dsi.c
> index 0eec073b3919..cfd0e3d5f36a 100644
> --- a/drivers/video/fbdev/omap2/omapfb/dss/dsi.c
> +++ b/drivers/video/fbdev/omap2/omapfb/dss/dsi.c
> @@ -1180,15 +1180,6 @@ static int dsi_regulator_init(struct platform_device *dsidev)
>  		return PTR_ERR(vdds_dsi);
>  	}
>  
> -	if (regulator_can_change_voltage(vdds_dsi)) {
> -		r = regulator_set_voltage(vdds_dsi, 1800000, 1800000);
> -		if (r) {
> -			devm_regulator_put(vdds_dsi);
> -			DSSERR("can't set the DSI regulator voltage\n");
> -			return r;
> -		}
> -	}
> -

This code did fix an issue, see 02b7a32083b9930543663720758de249b4f6a2a3.

Now, even at the time when I wrote that fix, it did feel a bit odd to
me. I have to say I don't remember the discussion that led to the patch,
perhaps it was something along "yes, the driver should not need to do
that, but for the time being do it".

So where are these "machine constraints" defined?

 Tomi


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] omapfb: Fix regulator API abuse in dss.c and hdmi4.c

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 941 bytes --]

On Thu, Mar 31, 2016 at 09:30:11AM +0300, Tomi Valkeinen wrote:

> This code did fix an issue, see 02b7a32083b9930543663720758de249b4f6a2a3.

That change isn't sensible, especially the _can_change_voltage() like I
said in the commit log.

> Now, even at the time when I wrote that fix, it did feel a bit odd to
> me. I have to say I don't remember the discussion that led to the patch,
> perhaps it was something along "yes, the driver should not need to do
> that, but for the time being do it".

That wasn't a discusion I was involved in, a quick google suggests it
might've been off-list.

> So where are these "machine constraints" defined?

They're the DT.  Your machine constraints just seem broken and need to
be fixed, most likely whoever wrote the constraints for the platform
completely failed to understand the purpose of constraints and filled in
the maximum range of voltages that the regulator in use on the board can
support.

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

Re: [PATCH] omapfb: Fix regulator API abuse in dss.c and hdmi4.c

[Tomi Valkeinen]

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

On 31/03/16 19:49, Mark Brown wrote:
> On Thu, Mar 31, 2016 at 09:30:11AM +0300, Tomi Valkeinen wrote:
> 
>> This code did fix an issue, see 02b7a32083b9930543663720758de249b4f6a2a3.
> 
> That change isn't sensible, especially the _can_change_voltage() like I
> said in the commit log.

I may remember wrong, but I think regulator_set_voltage() failed if
regulator_can_change_voltage() returned false. So I ended up having the
'if' there. But I may remember wrong, or maybe it's been changed since.

>> Now, even at the time when I wrote that fix, it did feel a bit odd to
>> me. I have to say I don't remember the discussion that led to the patch,
>> perhaps it was something along "yes, the driver should not need to do
>> that, but for the time being do it".
> 
> That wasn't a discusion I was involved in, a quick google suggests it
> might've been off-list.

That's possible. With quick googling, this may have longer history than
the patch I sent.

>> So where are these "machine constraints" defined?
> 
> They're the DT.  Your machine constraints just seem broken and need to
> be fixed, most likely whoever wrote the constraints for the platform
> completely failed to understand the purpose of constraints and filled in
> the maximum range of voltages that the regulator in use on the board can
> support.

Ok.

Tero, Tony, with a quick look, for example omap5-board-common.dtsi
defines ldo4_reg having range from 1.5 to 1.8. That should be changed to
1.8 only, right? ldo1_reg has a range too, I'm not familiar with that
LDO's use, but it's for camera so my guess is that it should be 1.8 too.

I can have a more thorough look tomorrow, and do a test run on omap5
uevm (with Mark's patch).

I wonder why we have the same code in hdmi4. Again with a quick look,
omap4 boards seem to use vdac for hdmi, and vdac doesn't have any
constraints in twl6030.dtsi, so I presume it's a fixed-voltage.

Anyway, I'm happy to apply this patch (and we need similar for hdmi5,
and also for omapdrm), we just need to do any necessary fixes to the
.dts first.

Although strictly speaking, I guess that's breaking backward
compatibility...

 Tomi


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] omapfb: Fix regulator API abuse in dss.c and hdmi4.c

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1198 bytes --]

On Thu, Mar 31, 2016 at 08:11:08PM +0300, Tomi Valkeinen wrote:
> On 31/03/16 19:49, Mark Brown wrote:
> > On Thu, Mar 31, 2016 at 09:30:11AM +0300, Tomi Valkeinen wrote:

> >> This code did fix an issue, see 02b7a32083b9930543663720758de249b4f6a2a3.

> > That change isn't sensible, especially the _can_change_voltage() like I
> > said in the commit log.

> I may remember wrong, but I think regulator_set_voltage() failed if
> regulator_can_change_voltage() returned false. So I ended up having the
> 'if' there. But I may remember wrong, or maybe it's been changed since.

That's not been the case since 2012 but your change was written in
2014...

> I wonder why we have the same code in hdmi4. Again with a quick look,
> omap4 boards seem to use vdac for hdmi, and vdac doesn't have any
> constraints in twl6030.dtsi, so I presume it's a fixed-voltage.

Yes, if no range is specified the regulator API won't touch the set
voltage.

> Anyway, I'm happy to apply this patch (and we need similar for hdmi5,
> and also for omapdrm), we just need to do any necessary fixes to the
> .dts first.

> Although strictly speaking, I guess that's breaking backward
> compatibility...

Will anyone notice?

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

Re: [PATCH] omapfb: Fix regulator API abuse in dss.c and hdmi4.c

[Tony Lindgren]

Tomi,

* Tomi Valkeinen <tomi.valkeinen@ti.com> [160331 10:31]:
> Tero, Tony, with a quick look, for example omap5-board-common.dtsi
> defines ldo4_reg having range from 1.5 to 1.8. That should be changed to
> 1.8 only, right? ldo1_reg has a range too, I'm not familiar with that
> LDO's use, but it's for camera so my guess is that it should be 1.8 too.
> 
> I can have a more thorough look tomorrow, and do a test run on omap5
> uevm (with Mark's patch).

Any news on this fix?

Tony

Re: [PATCH] omapfb: Fix regulator API abuse in dss.c and hdmi4.c

[Tomi Valkeinen]

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



On 26/04/16 19:22, Tony Lindgren wrote:
> Tomi,
> 
> * Tomi Valkeinen <tomi.valkeinen@ti.com> [160331 10:31]:
>> Tero, Tony, with a quick look, for example omap5-board-common.dtsi
>> defines ldo4_reg having range from 1.5 to 1.8. That should be changed to
>> 1.8 only, right? ldo1_reg has a range too, I'm not familiar with that
>> LDO's use, but it's for camera so my guess is that it should be 1.8 too.
>>
>> I can have a more thorough look tomorrow, and do a test run on omap5
>> uevm (with Mark's patch).
> 
> Any news on this fix?

I sent the DT changes to you some time ago:

http://www.spinics.net/lists/linux-omap/msg127893.html
http://www.spinics.net/lists/linux-omap/msg127894.html

We can get rid of the dss code after those are in.

 Tomi


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] omapfb: Fix regulator API abuse in dss.c and hdmi4.c

[Tony Lindgren]

* Tomi Valkeinen <tomi.valkeinen@ti.com> [160426 09:38]:
> On 26/04/16 19:22, Tony Lindgren wrote:
> > * Tomi Valkeinen <tomi.valkeinen@ti.com> [160331 10:31]:
> >> Tero, Tony, with a quick look, for example omap5-board-common.dtsi
> >> defines ldo4_reg having range from 1.5 to 1.8. That should be changed to
> >> 1.8 only, right? ldo1_reg has a range too, I'm not familiar with that
> >> LDO's use, but it's for camera so my guess is that it should be 1.8 too.
> >>
> >> I can have a more thorough look tomorrow, and do a test run on omap5
> >> uevm (with Mark's patch).
> > 
> > Any news on this fix?
> 
> I sent the DT changes to you some time ago:
> 
> http://www.spinics.net/lists/linux-omap/msg127893.html
> http://www.spinics.net/lists/linux-omap/msg127894.html

OK thanks yeah I have those tagged as fixes.

> We can get rid of the dss code after those are in.

OK so I'll apply the fixes and forget about this thread :)

Thanks,

Tony

Re: [PATCH] omapfb: Fix regulator API abuse in dss.c and hdmi4.c

[Tomi Valkeinen]

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



On 26/04/16 19:42, Tony Lindgren wrote:
> * Tomi Valkeinen <tomi.valkeinen@ti.com> [160426 09:38]:
>> On 26/04/16 19:22, Tony Lindgren wrote:
>>> * Tomi Valkeinen <tomi.valkeinen@ti.com> [160331 10:31]:
>>>> Tero, Tony, with a quick look, for example omap5-board-common.dtsi
>>>> defines ldo4_reg having range from 1.5 to 1.8. That should be changed to
>>>> 1.8 only, right? ldo1_reg has a range too, I'm not familiar with that
>>>> LDO's use, but it's for camera so my guess is that it should be 1.8 too.
>>>>
>>>> I can have a more thorough look tomorrow, and do a test run on omap5
>>>> uevm (with Mark's patch).
>>>
>>> Any news on this fix?
>>
>> I sent the DT changes to you some time ago:
>>
>> http://www.spinics.net/lists/linux-omap/msg127893.html
>> http://www.spinics.net/lists/linux-omap/msg127894.html
> 
> OK thanks yeah I have those tagged as fixes.
> 
>> We can get rid of the dss code after those are in.
> 
> OK so I'll apply the fixes and forget about this thread :)

Fixes for v4.6? If so, I can push the regulator cleanup to omapfb and
omapdrm for 4.7, which would be nice.

 Tomi


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]