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

[Tomi Valkeinen <tomi.valkeinen@ti.com>]

[-- 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 --]