Re: [PATCH v2 20/23] OMAPDSS: MANAGER: Update display sysfs store

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

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

On Thu, 2012-08-30 at 17:10 +0530, Archit Taneja wrote:
> The display sysfs attribute's store function needs to be changed with the
> introduction of outputs.
> 
> Providing a manager to the display isn't enough to create a link now, the
> manager needs and output to connect to. A manager's display store file only
> has the information of the manager and the desired display, it is not aware
> of which output should the manager connect to.
> 
> Because of this, a new constraint needs to be set up when setting a display via
> this sysfs file. The constraint is that the desired display should already be
> connected to an output before calling this sysfs function.
> 
> This might break some existing user space stuff which uses sysfs directly. But
> in most cases dss_recheck_connections will connect displays to floating outputs.
> DSS sysfs files are being planned to be remove anyway, so it's not much of a
> harm.
> 
> Signed-off-by: Archit Taneja <archit@ti.com>
> ---
>  drivers/video/omap2/dss/manager.c |   25 ++++++++++++++++++++-----
>  1 file changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/video/omap2/dss/manager.c b/drivers/video/omap2/dss/manager.c
> index fd39f66..d808ce2 100644
> --- a/drivers/video/omap2/dss/manager.c
> +++ b/drivers/video/omap2/dss/manager.c
> @@ -74,18 +74,33 @@ static ssize_t manager_display_store(struct omap_overlay_manager *mgr,
>  	if (dssdev)
>  		DSSDBG("display %s found\n", dssdev->name);
>  
> -	if (mgr->get_device(mgr)) {
> -		r = mgr->unset_device(mgr);
> +	if (mgr->output) {
> +		if (mgr->output->device) {
> +			r = mgr->output->unset_device(mgr->output);
> +			if (r) {
> +				goto put_device;
> +				DSSERR("failed to unset device from output\n");
> +			}
> +		}
> +
> +		r = mgr->unset_output(mgr);
>  		if (r) {
> -			DSSERR("failed to unset display\n");
> +			DSSERR("failed to unset current output\n");
>  			goto put_device;
>  		}
>  	}
>  
>  	if (dssdev) {
> -		r = mgr->set_device(mgr, dssdev);
> +		struct omap_dss_output *out = dssdev->output;
> +
> +		if (!out) {
> +			DSSERR("no output connected to device\n");
> +			goto put_device;
> +		}
> +
> +		r = mgr->set_output(mgr, out);
>  		if (r) {
> -			DSSERR("failed to set manager\n");
> +			DSSERR("failed to set manager output\n");
>  			goto put_device;
>  		}
>  

Hmm, I think this is a bit broken. If I read this right, the unlinking
removes both mgr-output link and the output-dssdev link. But the linking
part only sets up the mgr-output link.

So if there's a very simple configuration with one display, if you
unlink it via sysfs you can't link it back again.

The store function gets the mgr and the dssdev as arguments. I think
this could be changed so that the function would "guess" the needed
output. Well, not so much a guess, because a dssdev can only be
connected to one output, defined by the HW design. That is basically
what the recheck_connections does, we could use the same method here.

Then this sysfs file should work just like before.

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]