| Message ID | 20190326103146.24795-17-tomi.valkeinen@ti.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | drm/bridge: tc358767: DP support | expand |
On 26.03.2019 11:31, Tomi Valkeinen wrote: > tc_main_link_enable() checks if videomode has been set, and fails if > there's no videomode. As tc_main_link_enable() no longer depends on the > videomode, we can drop the check. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com> -- Regards Andrzej
Hi Tomi, Thank you for the patch. On Tue, Mar 26, 2019 at 12:31:40PM +0200, Tomi Valkeinen wrote: > tc_main_link_enable() checks if videomode has been set, and fails if > there's no videomode. As tc_main_link_enable() no longer depends on the > videomode, we can drop the check. Shouldn't you move the check to the stream enable function ? Or if it's not needed there, explain why in the commit message ? > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> > --- > drivers/gpu/drm/bridge/tc358767.c | 4 ---- > 1 file changed, 4 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c > index 1c61f6205e43..ece330c05b9f 100644 > --- a/drivers/gpu/drm/bridge/tc358767.c > +++ b/drivers/gpu/drm/bridge/tc358767.c > @@ -776,10 +776,6 @@ static int tc_main_link_enable(struct tc_data *tc) > u8 tmp[8]; > u32 error; > > - /* display mode should be set at this point */ > - if (!tc->mode) > - return -EINVAL; > - > dev_dbg(tc->dev, "link enable\n"); > > tc_write(DP0CTL, 0);
On 21/04/2019 01:14, Laurent Pinchart wrote: > Hi Tomi, > > Thank you for the patch. > > On Tue, Mar 26, 2019 at 12:31:40PM +0200, Tomi Valkeinen wrote: >> tc_main_link_enable() checks if videomode has been set, and fails if >> there's no videomode. As tc_main_link_enable() no longer depends on the >> videomode, we can drop the check. > > Shouldn't you move the check to the stream enable function ? Or if it's > not needed there, explain why in the commit message ? True. I believe it is not needed. I don't think bridge_enable should be called at all, if there has not been a mode set before it. If there's no mode, bridge enable presumably would fail for any bridge... Tomi
Hi Tomi, On Fri, May 03, 2019 at 11:10:54AM +0300, Tomi Valkeinen wrote: > On 21/04/2019 01:14, Laurent Pinchart wrote: > > On Tue, Mar 26, 2019 at 12:31:40PM +0200, Tomi Valkeinen wrote: > >> tc_main_link_enable() checks if videomode has been set, and fails if > >> there's no videomode. As tc_main_link_enable() no longer depends on the > >> videomode, we can drop the check. > > > > Shouldn't you move the check to the stream enable function ? Or if it's > > not needed there, explain why in the commit message ? > > True. I believe it is not needed. I don't think bridge_enable should be > called at all, if there has not been a mode set before it. If there's no > mode, bridge enable presumably would fail for any bridge... That's my understanding too. A quick check in the core could be useful, and mentioning this in the commit message too. Ideally the documentation of the bridge operations should be updated to make this clear :-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c index 1c61f6205e43..ece330c05b9f 100644 --- a/drivers/gpu/drm/bridge/tc358767.c +++ b/drivers/gpu/drm/bridge/tc358767.c @@ -776,10 +776,6 @@ static int tc_main_link_enable(struct tc_data *tc) u8 tmp[8]; u32 error; - /* display mode should be set at this point */ - if (!tc->mode) - return -EINVAL; - dev_dbg(tc->dev, "link enable\n"); tc_write(DP0CTL, 0);
tc_main_link_enable() checks if videomode has been set, and fails if there's no videomode. As tc_main_link_enable() no longer depends on the videomode, we can drop the check. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> --- drivers/gpu/drm/bridge/tc358767.c | 4 ---- 1 file changed, 4 deletions(-)