| Message ID | 1346326845-16583-4-git-send-email-archit@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 2012-08-30 at 17:10 +0530, Archit Taneja wrote: > An output entity represented by the struct omap_dss_output connects to a > omap_dss_device entity. Add functions to set or unset an output's device. This > is similar to how managers and devices were connected previously. An output can > connect to a device without being connected to a manager. However, the output > needs to eventually connect to a manager so that the connected panel can be > enabled. > > Keep the omap_overlay_manager pointer in omap_dss_device for now to prevent > breaking things. This will be removed later when outputs are supported > completely. > > Signed-off-by: Archit Taneja <archit@ti.com> > --- > drivers/video/omap2/dss/output.c | 67 ++++++++++++++++++++++++++++++++++++++ > include/video/omapdss.h | 5 +++ > 2 files changed, 72 insertions(+) > > diff --git a/drivers/video/omap2/dss/output.c b/drivers/video/omap2/dss/output.c > index 7d81be5..abc3aa2 100644 > --- a/drivers/video/omap2/dss/output.c > +++ b/drivers/video/omap2/dss/output.c > @@ -24,9 +24,76 @@ > #include "dss.h" > > static LIST_HEAD(output_list); > +static DEFINE_MUTEX(output_lock); > + > +static int dss_output_set_device(struct omap_dss_output *out, > + struct omap_dss_device *dssdev) > +{ > + int r; > + > + mutex_lock(&output_lock); > + > + if (out->device) { > + DSSERR("output already has device %s connected to it\n", > + out->device->name); > + r = -EINVAL; > + goto err; > + } > + > + if (out->type != dssdev->type) { > + DSSERR("output type and display type don't match\n"); > + r = -EINVAL; > + goto err; > + } > + > + out->device = dssdev; > + dssdev->output = out; > + > + mutex_unlock(&output_lock); > + > + return 0; > +err: > + mutex_unlock(&output_lock); > + > + return r; > +} > + > +static int dss_output_unset_device(struct omap_dss_output *out) > +{ > + int r; > + > + mutex_lock(&output_lock); > + > + if (!out->device) { > + DSSERR("output doesn't have a device connected to it\n"); > + r = -EINVAL; > + goto err; > + } > + > + if (out->device->state != OMAP_DSS_DISPLAY_DISABLED) { > + DSSERR("device %s is not disabled, cannot unset device\n", > + out->device->name); > + r = -EINVAL; > + goto err; > + } > + > + out->device->output = NULL; > + out->device = NULL; > + > + mutex_unlock(&output_lock); > + > + return 0; > +err: > + mutex_unlock(&output_lock); > + > + return r; > +} > > void dss_register_output(struct omap_dss_output *out) > { > + out->set_device = &dss_output_set_device; > + out->unset_device = &dss_output_unset_device; > + > list_add_tail(&out->list, &output_list); > } I don't think there's need for this indirection. We should use function pointers only when the func pointer may lead to different functions. Here we'll always have just one function, dss_output_set_device. We can as well call the function directly. I know we have similar func pointers for ovls/mgrs currently, but I don't think they are good either. They are a relic from the time we supported "virtual" overlays and managers, and thus could have different implementations for the operations. Tomi
On Friday 31 August 2012 05:33 PM, Tomi Valkeinen wrote: > On Thu, 2012-08-30 at 17:10 +0530, Archit Taneja wrote: >> An output entity represented by the struct omap_dss_output connects to a >> omap_dss_device entity. Add functions to set or unset an output's device. This >> is similar to how managers and devices were connected previously. An output can >> connect to a device without being connected to a manager. However, the output >> needs to eventually connect to a manager so that the connected panel can be >> enabled. >> >> Keep the omap_overlay_manager pointer in omap_dss_device for now to prevent >> breaking things. This will be removed later when outputs are supported >> completely. >> >> Signed-off-by: Archit Taneja <archit@ti.com> >> --- >> drivers/video/omap2/dss/output.c | 67 ++++++++++++++++++++++++++++++++++++++ >> include/video/omapdss.h | 5 +++ >> 2 files changed, 72 insertions(+) >> >> diff --git a/drivers/video/omap2/dss/output.c b/drivers/video/omap2/dss/output.c >> index 7d81be5..abc3aa2 100644 >> --- a/drivers/video/omap2/dss/output.c >> +++ b/drivers/video/omap2/dss/output.c >> @@ -24,9 +24,76 @@ >> #include "dss.h" >> >> static LIST_HEAD(output_list); >> +static DEFINE_MUTEX(output_lock); >> + >> +static int dss_output_set_device(struct omap_dss_output *out, >> + struct omap_dss_device *dssdev) >> +{ >> + int r; >> + >> + mutex_lock(&output_lock); >> + >> + if (out->device) { >> + DSSERR("output already has device %s connected to it\n", >> + out->device->name); >> + r = -EINVAL; >> + goto err; >> + } >> + >> + if (out->type != dssdev->type) { >> + DSSERR("output type and display type don't match\n"); >> + r = -EINVAL; >> + goto err; >> + } >> + >> + out->device = dssdev; >> + dssdev->output = out; >> + >> + mutex_unlock(&output_lock); >> + >> + return 0; >> +err: >> + mutex_unlock(&output_lock); >> + >> + return r; >> +} >> + >> +static int dss_output_unset_device(struct omap_dss_output *out) >> +{ >> + int r; >> + >> + mutex_lock(&output_lock); >> + >> + if (!out->device) { >> + DSSERR("output doesn't have a device connected to it\n"); >> + r = -EINVAL; >> + goto err; >> + } >> + >> + if (out->device->state != OMAP_DSS_DISPLAY_DISABLED) { >> + DSSERR("device %s is not disabled, cannot unset device\n", >> + out->device->name); >> + r = -EINVAL; >> + goto err; >> + } >> + >> + out->device->output = NULL; >> + out->device = NULL; >> + >> + mutex_unlock(&output_lock); >> + >> + return 0; >> +err: >> + mutex_unlock(&output_lock); >> + >> + return r; >> +} >> >> void dss_register_output(struct omap_dss_output *out) >> { >> + out->set_device = &dss_output_set_device; >> + out->unset_device = &dss_output_unset_device; >> + >> list_add_tail(&out->list, &output_list); >> } > > I don't think there's need for this indirection. We should use function > pointers only when the func pointer may lead to different functions. > Here we'll always have just one function, dss_output_set_device. We can > as well call the function directly. Okay. I understand that. But in general, don't func pointers prevent us from exporting more symbols? > > I know we have similar func pointers for ovls/mgrs currently, but I > don't think they are good either. They are a relic from the time we > supported "virtual" overlays and managers, and thus could have different > implementations for the operations. Oh okay, I guess you mean the L4/sDMA updates for DSI command mode. Archit -- 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 Fri, 2012-08-31 at 17:54 +0530, Archit Taneja wrote: > On Friday 31 August 2012 05:33 PM, Tomi Valkeinen wrote: > > I don't think there's need for this indirection. We should use function > > pointers only when the func pointer may lead to different functions. > > Here we'll always have just one function, dss_output_set_device. We can > > as well call the function directly. > > Okay. I understand that. But in general, don't func pointers prevent us > from exporting more symbols? Yes. But I'm not sure if there's any real downside to exporting, as long as the names are prefixed properly so that there are no name clashes. > > I know we have similar func pointers for ovls/mgrs currently, but I > > don't think they are good either. They are a relic from the time we > > supported "virtual" overlays and managers, and thus could have different > > implementations for the operations. > > Oh okay, I guess you mean the L4/sDMA updates for DSI command mode. Yep. Tomi
diff --git a/drivers/video/omap2/dss/output.c b/drivers/video/omap2/dss/output.c index 7d81be5..abc3aa2 100644 --- a/drivers/video/omap2/dss/output.c +++ b/drivers/video/omap2/dss/output.c @@ -24,9 +24,76 @@ #include "dss.h" static LIST_HEAD(output_list); +static DEFINE_MUTEX(output_lock); + +static int dss_output_set_device(struct omap_dss_output *out, + struct omap_dss_device *dssdev) +{ + int r; + + mutex_lock(&output_lock); + + if (out->device) { + DSSERR("output already has device %s connected to it\n", + out->device->name); + r = -EINVAL; + goto err; + } + + if (out->type != dssdev->type) { + DSSERR("output type and display type don't match\n"); + r = -EINVAL; + goto err; + } + + out->device = dssdev; + dssdev->output = out; + + mutex_unlock(&output_lock); + + return 0; +err: + mutex_unlock(&output_lock); + + return r; +} + +static int dss_output_unset_device(struct omap_dss_output *out) +{ + int r; + + mutex_lock(&output_lock); + + if (!out->device) { + DSSERR("output doesn't have a device connected to it\n"); + r = -EINVAL; + goto err; + } + + if (out->device->state != OMAP_DSS_DISPLAY_DISABLED) { + DSSERR("device %s is not disabled, cannot unset device\n", + out->device->name); + r = -EINVAL; + goto err; + } + + out->device->output = NULL; + out->device = NULL; + + mutex_unlock(&output_lock); + + return 0; +err: + mutex_unlock(&output_lock); + + return r; +} void dss_register_output(struct omap_dss_output *out) { + out->set_device = &dss_output_set_device; + out->unset_device = &dss_output_unset_device; + list_add_tail(&out->list, &output_list); } diff --git a/include/video/omapdss.h b/include/video/omapdss.h index 2926a04..b3fba19 100644 --- a/include/video/omapdss.h +++ b/include/video/omapdss.h @@ -518,6 +518,10 @@ struct omap_dss_output { struct omap_overlay_manager *manager; struct omap_dss_device *device; + + int (*set_device) (struct omap_dss_output *out, + struct omap_dss_device *dssdev); + int (*unset_device) (struct omap_dss_output *out); }; struct omap_dss_device { @@ -619,6 +623,7 @@ struct omap_dss_device { enum omap_display_caps caps; struct omap_overlay_manager *manager; + struct omap_dss_output *output; enum omap_dss_display_state state;
An output entity represented by the struct omap_dss_output connects to a omap_dss_device entity. Add functions to set or unset an output's device. This is similar to how managers and devices were connected previously. An output can connect to a device without being connected to a manager. However, the output needs to eventually connect to a manager so that the connected panel can be enabled. Keep the omap_overlay_manager pointer in omap_dss_device for now to prevent breaking things. This will be removed later when outputs are supported completely. Signed-off-by: Archit Taneja <archit@ti.com> --- drivers/video/omap2/dss/output.c | 67 ++++++++++++++++++++++++++++++++++++++ include/video/omapdss.h | 5 +++ 2 files changed, 72 insertions(+)