| Message ID | 1459355376-28507-1-git-send-email-broonie@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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.
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
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?
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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
* 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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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;
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(-)