diff mbox series

[1/6] drm/bridge: Export drm_bridge_detach

Message ID 20190315130825.9005-2-jagan@amarulasolutions.com
State New
Headers
Series drm/bridge: Add ICN6211 MIPI-DSI/RGB bridge |

Commit Message

Jagan Teki March 15, 2019, 1:08 p.m. UTC 
Export drm_bridge_detach from drm bridge core so-that it
can use on respective interface or bridge driver while
detaching the bridge.

Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
 drivers/gpu/drm/drm_bridge.c | 1 +
 include/drm/drm_bridge.h     | 1 +
 2 files changed, 2 insertions(+)

Comments

Paul Kocialkowski March 15, 2019, 1:27 p.m. UTC | #1 
Hi Jakan,

On Fri, 2019-03-15 at 18:38 +0530, Jagan Teki wrote:
> Export drm_bridge_detach from drm bridge core so-that it
> can use on respective interface or bridge driver while
> detaching the bridge.

I don't see why this change is required based on the commit log. The
DRM bridge code clearly indicates that drm_bridge_attach should *not*
be balanced with a drm_bridge_detach call in the driver, so this seems
quite wrong.

The DRM core itself should handle detaching the bridge, not the driver.
Is there any reason why you need to do things differently for DSI?

Cheers,

Paul

> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> ---
>  drivers/gpu/drm/drm_bridge.c | 1 +
>  include/drm/drm_bridge.h     | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 138b2711d389..569d4f345429 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -159,6 +159,7 @@ void drm_bridge_detach(struct drm_bridge *bridge)
>  
>  	bridge->dev = NULL;
>  }
> +EXPORT_SYMBOL(drm_bridge_detach);
>  
>  /**
>   * DOC: bridge callbacks
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index 9da8c93f7976..4955e3e50fa4 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -301,6 +301,7 @@ void drm_bridge_remove(struct drm_bridge *bridge);
>  struct drm_bridge *of_drm_find_bridge(struct device_node *np);
>  int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
>  		      struct drm_bridge *previous);
> +void drm_bridge_detach(struct drm_bridge *bridge);
>  
>  bool drm_bridge_mode_fixup(struct drm_bridge *bridge,
>  			   const struct drm_display_mode *mode,
> -- 
> 2.18.0.321.gffc6fa0e3
>
Jagan Teki March 18, 2019, 4:48 p.m. UTC | #2 
Hi Paul,

On Fri, Mar 15, 2019 at 6:58 PM Paul Kocialkowski
<paul.kocialkowski@bootlin.com> wrote:
>
> Hi Jakan,
>
> On Fri, 2019-03-15 at 18:38 +0530, Jagan Teki wrote:
> > Export drm_bridge_detach from drm bridge core so-that it
> > can use on respective interface or bridge driver while
> > detaching the bridge.
>
> I don't see why this change is required based on the commit log. The
> DRM bridge code clearly indicates that drm_bridge_attach should *not*
> be balanced with a drm_bridge_detach call in the driver, so this seems
> quite wrong.
>
> The DRM core itself should handle detaching the bridge, not the driver.
> Is there any reason why you need to do things differently for DSI?

Yes, you are correct the detach of bridge is being taking care via
drm_encoder_cleanup. This patch exported explicitly, since we need to
taken care bridge detach during unbind even exynos_drm_dsi in other
patch seems using detach by explicitly pointing. so I think the better
approach is to use drm_encoder_cleanup in unbind, what do you say?

Jagan.
Paul Kocialkowski March 18, 2019, 4:57 p.m. UTC | #3 
Hi,

Le lundi 18 mars 2019 à 22:18 +0530, Jagan Teki a écrit :
> Hi Paul,
> 
> On Fri, Mar 15, 2019 at 6:58 PM Paul Kocialkowski
> <paul.kocialkowski@bootlin.com> wrote:
> > Hi Jakan,
> > 
> > On Fri, 2019-03-15 at 18:38 +0530, Jagan Teki wrote:
> > > Export drm_bridge_detach from drm bridge core so-that it
> > > can use on respective interface or bridge driver while
> > > detaching the bridge.
> > 
> > I don't see why this change is required based on the commit log. The
> > DRM bridge code clearly indicates that drm_bridge_attach should *not*
> > be balanced with a drm_bridge_detach call in the driver, so this seems
> > quite wrong.
> > 
> > The DRM core itself should handle detaching the bridge, not the driver.
> > Is there any reason why you need to do things differently for DSI?
> 
> Yes, you are correct the detach of bridge is being taking care via
> drm_encoder_cleanup. This patch exported explicitly, since we need to
> taken care bridge detach during unbind even exynos_drm_dsi in other
> patch seems using detach by explicitly pointing.

I can see that from your patches, but you are not explaining why you
need the change. And if the framework doesn't work for your case, you
should certainly try and fix the framework instead of working around
the issue.

Anyway, you should probably look into using drm_panel_bridge_add, it
might fix the underlying issue on its own.

> so I think the better approach is to use drm_encoder_cleanup in
> unbind, what do you say?

I any case, you need to state what your problem is (in the commit log,
and not even in subsequent discussions) so that we can have a chance to
understand it and provide some feedback about what is an appropriate
fix and what is not. We can't understand the fix if we can't understand
the underlying issue.

Cheers,

Paul
Jagan Teki March 18, 2019, 5:07 p.m. UTC | #4 
On Mon, Mar 18, 2019 at 10:27 PM Paul Kocialkowski
<paul.kocialkowski@bootlin.com> wrote:
>
> Hi,
>
> Le lundi 18 mars 2019 à 22:18 +0530, Jagan Teki a écrit :
> > Hi Paul,
> >
> > On Fri, Mar 15, 2019 at 6:58 PM Paul Kocialkowski
> > <paul.kocialkowski@bootlin.com> wrote:
> > > Hi Jakan,
> > >
> > > On Fri, 2019-03-15 at 18:38 +0530, Jagan Teki wrote:
> > > > Export drm_bridge_detach from drm bridge core so-that it
> > > > can use on respective interface or bridge driver while
> > > > detaching the bridge.
> > >
> > > I don't see why this change is required based on the commit log. The
> > > DRM bridge code clearly indicates that drm_bridge_attach should *not*
> > > be balanced with a drm_bridge_detach call in the driver, so this seems
> > > quite wrong.
> > >
> > > The DRM core itself should handle detaching the bridge, not the driver.
> > > Is there any reason why you need to do things differently for DSI?
> >
> > Yes, you are correct the detach of bridge is being taking care via
> > drm_encoder_cleanup. This patch exported explicitly, since we need to
> > taken care bridge detach during unbind even exynos_drm_dsi in other
> > patch seems using detach by explicitly pointing.
>
> I can see that from your patches, but you are not explaining why you
> need the change. And if the framework doesn't work for your case, you
> should certainly try and fix the framework instead of working around
> the issue.

Please see below comments.

>
> Anyway, you should probably look into using drm_panel_bridge_add, it
> might fix the underlying issue on its own.
>
> > so I think the better approach is to use drm_encoder_cleanup in
> > unbind, what do you say?
>
> I any case, you need to state what your problem is (in the commit log,
> and not even in subsequent discussions) so that we can have a chance to
> understand it and provide some feedback about what is an appropriate
> fix and what is not. We can't understand the fix if we can't understand
> the underlying issue.

True, if my intentions is trying to fix some issue, but as I said in
previous mail since the other dsi (exynos_drm_dsi) driver is
explicitly detaching I presume it require or missed the export, not
thought about fix and neither I mentioned on the commit head.  Anyway
thanks for the review, will update the code accordingly in next
version.
diff mbox series

Patch

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 138b2711d389..569d4f345429 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -159,6 +159,7 @@  void drm_bridge_detach(struct drm_bridge *bridge)
 
 	bridge->dev = NULL;
 }
+EXPORT_SYMBOL(drm_bridge_detach);
 
 /**
  * DOC: bridge callbacks
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 9da8c93f7976..4955e3e50fa4 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -301,6 +301,7 @@  void drm_bridge_remove(struct drm_bridge *bridge);
 struct drm_bridge *of_drm_find_bridge(struct device_node *np);
 int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
 		      struct drm_bridge *previous);
+void drm_bridge_detach(struct drm_bridge *bridge);
 
 bool drm_bridge_mode_fixup(struct drm_bridge *bridge,
 			   const struct drm_display_mode *mode,