From: Mark Brown <broonie@kernel.org> Date: 2016-03-30 16:29:36
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(-)
@@ -1180,15 +1180,6 @@ static int dsi_regulator_init(struct platform_device *dsidev)returnPTR_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");-returnr;-}-}-dsi->vdds_dsi_reg=vdds_dsi;return0;
@@ -114,15 +114,6 @@ static int hdmi_init_regulator(void)returnPTR_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");-returnr;-}-}-hdmi.vdda_reg=reg;return0;
From: Tomi Valkeinen <hidden> Date: 2016-03-31 06:30:11
Hi Mark,
On 30/03/16 19:29, Mark Brown wrote:
quoted hunk
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(-)
@@ -1180,15 +1180,6 @@ static int dsi_regulator_init(struct platform_device *dsidev)returnPTR_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");-returnr;-}-}-
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
From: Mark Brown <broonie@kernel.org> Date: 2016-03-31 16:49:53
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.
From: Tomi Valkeinen <hidden> Date: 2016-03-31 17:11:08
On 31/03/16 19:49, Mark Brown wrote:
On Thu, Mar 31, 2016 at 09:30:11AM +0300, Tomi Valkeinen wrote:
quoted
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.
quoted
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.
quoted
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
From: Mark Brown <broonie@kernel.org> Date: 2016-03-31 17:39:42
On Thu, Mar 31, 2016 at 08:11:08PM +0300, Tomi Valkeinen wrote:
On 31/03/16 19:49, Mark Brown wrote:
quoted
On Thu, Mar 31, 2016 at 09:30:11AM +0300, Tomi Valkeinen wrote:
quoted
quoted
This code did fix an issue, see 02b7a32083b9930543663720758de249b4f6a2a3.
quoted
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...
From: Tony Lindgren <tony@atomide.com> Date: 2016-04-26 16:22:30
Tomi,
* Tomi Valkeinen [off-list ref] [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).
From: Tomi Valkeinen <hidden> Date: 2016-04-26 16:37:24
On 26/04/16 19:22, Tony Lindgren wrote:
Tomi,
* Tomi Valkeinen [off-list ref] [160331 10:31]:
quoted
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).
From: Tony Lindgren <tony@atomide.com> Date: 2016-04-26 16:42:41
* Tomi Valkeinen [off-list ref] [160426 09:38]:
On 26/04/16 19:22, Tony Lindgren wrote:
quoted
* Tomi Valkeinen [off-list ref] [160331 10:31]:
quoted
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).
From: Tomi Valkeinen <hidden> Date: 2016-04-26 16:45:56
On 26/04/16 19:42, Tony Lindgren wrote:
* Tomi Valkeinen [off-list ref] [160426 09:38]:
quoted
On 26/04/16 19:22, Tony Lindgren wrote:
quoted
* Tomi Valkeinen [off-list ref] [160331 10:31]:
quoted
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).