* [PATCH] drm/sun4i: Clear encoder->bridge if a bridge is not found @ 2016-08-30 12:22 ` Chen-Yu Tsai 0 siblings, 0 replies; 21+ messages in thread From: Chen-Yu Tsai @ 2016-08-30 12:22 UTC (permalink / raw) To: Maxime Ripard, David Airlie Cc: Chen-Yu Tsai, dri-devel, linux-arm-kernel, linux-kernel The KMS helpers (drm_atomic_helper_check_modeset/mode_fixup) pass encoder->bridge directly to drm_bridge_mode_fixup, which expects a valid pointer, or NULL (in which case it just returns). Clear encoder->bridge if a bridge is not found, instead of keeping the ERR_PTR value. Since other drm_bridge functions also follow this pattern of checking for a non-NULL pointer, we can drop the ifs around the calls and just pass the pointer directly. Fixes: 894f5a9f4b4a ("drm/sun4i: Add bridge support") Signed-off-by: Chen-Yu Tsai <wens@csie.org> --- drivers/gpu/drm/sun4i/sun4i_rgb.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/sun4i/sun4i_rgb.c b/drivers/gpu/drm/sun4i/sun4i_rgb.c index d4e52522ec53..ee0795152a33 100644 --- a/drivers/gpu/drm/sun4i/sun4i_rgb.c +++ b/drivers/gpu/drm/sun4i/sun4i_rgb.c @@ -154,8 +154,7 @@ static void sun4i_rgb_encoder_enable(struct drm_encoder *encoder) if (!IS_ERR(tcon->panel)) drm_panel_enable(tcon->panel); - if (!IS_ERR(encoder->bridge)) - drm_bridge_enable(encoder->bridge); + drm_bridge_enable(encoder->bridge); sun4i_tcon_channel_enable(tcon, 0); } @@ -170,8 +169,7 @@ static void sun4i_rgb_encoder_disable(struct drm_encoder *encoder) sun4i_tcon_channel_disable(tcon, 0); - if (!IS_ERR(encoder->bridge)) - drm_bridge_disable(encoder->bridge); + drm_bridge_disable(encoder->bridge); if (!IS_ERR(tcon->panel)) drm_panel_disable(tcon->panel); @@ -230,6 +228,9 @@ int sun4i_rgb_init(struct drm_device *drm) return 0; } + if (IS_ERR(encoder->bridge)) + encoder->bridge = NULL; + drm_encoder_helper_add(&rgb->encoder, &sun4i_rgb_enc_helper_funcs); ret = drm_encoder_init(drm, @@ -266,7 +267,7 @@ int sun4i_rgb_init(struct drm_device *drm) } } - if (!IS_ERR(encoder->bridge)) { + if (encoder->bridge) { encoder->bridge->encoder = &rgb->encoder; ret = drm_bridge_attach(drm, encoder->bridge); -- 2.9.3 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH] drm/sun4i: Clear encoder->bridge if a bridge is not found @ 2016-08-30 12:22 ` Chen-Yu Tsai 0 siblings, 0 replies; 21+ messages in thread From: Chen-Yu Tsai @ 2016-08-30 12:22 UTC (permalink / raw) To: linux-arm-kernel The KMS helpers (drm_atomic_helper_check_modeset/mode_fixup) pass encoder->bridge directly to drm_bridge_mode_fixup, which expects a valid pointer, or NULL (in which case it just returns). Clear encoder->bridge if a bridge is not found, instead of keeping the ERR_PTR value. Since other drm_bridge functions also follow this pattern of checking for a non-NULL pointer, we can drop the ifs around the calls and just pass the pointer directly. Fixes: 894f5a9f4b4a ("drm/sun4i: Add bridge support") Signed-off-by: Chen-Yu Tsai <wens@csie.org> --- drivers/gpu/drm/sun4i/sun4i_rgb.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/sun4i/sun4i_rgb.c b/drivers/gpu/drm/sun4i/sun4i_rgb.c index d4e52522ec53..ee0795152a33 100644 --- a/drivers/gpu/drm/sun4i/sun4i_rgb.c +++ b/drivers/gpu/drm/sun4i/sun4i_rgb.c @@ -154,8 +154,7 @@ static void sun4i_rgb_encoder_enable(struct drm_encoder *encoder) if (!IS_ERR(tcon->panel)) drm_panel_enable(tcon->panel); - if (!IS_ERR(encoder->bridge)) - drm_bridge_enable(encoder->bridge); + drm_bridge_enable(encoder->bridge); sun4i_tcon_channel_enable(tcon, 0); } @@ -170,8 +169,7 @@ static void sun4i_rgb_encoder_disable(struct drm_encoder *encoder) sun4i_tcon_channel_disable(tcon, 0); - if (!IS_ERR(encoder->bridge)) - drm_bridge_disable(encoder->bridge); + drm_bridge_disable(encoder->bridge); if (!IS_ERR(tcon->panel)) drm_panel_disable(tcon->panel); @@ -230,6 +228,9 @@ int sun4i_rgb_init(struct drm_device *drm) return 0; } + if (IS_ERR(encoder->bridge)) + encoder->bridge = NULL; + drm_encoder_helper_add(&rgb->encoder, &sun4i_rgb_enc_helper_funcs); ret = drm_encoder_init(drm, @@ -266,7 +267,7 @@ int sun4i_rgb_init(struct drm_device *drm) } } - if (!IS_ERR(encoder->bridge)) { + if (encoder->bridge) { encoder->bridge->encoder = &rgb->encoder; ret = drm_bridge_attach(drm, encoder->bridge); -- 2.9.3 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH] drm/sun4i: Clear encoder->bridge if a bridge is not found 2016-08-30 12:22 ` Chen-Yu Tsai (?) @ 2016-08-30 12:56 ` Maxime Ripard -1 siblings, 0 replies; 21+ messages in thread From: Maxime Ripard @ 2016-08-30 12:56 UTC (permalink / raw) To: Chen-Yu Tsai; +Cc: David Airlie, dri-devel, linux-arm-kernel, linux-kernel [-- Attachment #1: Type: text/plain, Size: 2114 bytes --] Hi, On Tue, Aug 30, 2016 at 08:22:23PM +0800, Chen-Yu Tsai wrote: > The KMS helpers (drm_atomic_helper_check_modeset/mode_fixup) pass > encoder->bridge directly to drm_bridge_mode_fixup, which expects a > valid pointer, or NULL (in which case it just returns). > > Clear encoder->bridge if a bridge is not found, instead of keeping > the ERR_PTR value. > > Since other drm_bridge functions also follow this pattern of checking > for a non-NULL pointer, we can drop the ifs around the calls and just > pass the pointer directly. > > Fixes: 894f5a9f4b4a ("drm/sun4i: Add bridge support") > Signed-off-by: Chen-Yu Tsai <wens@csie.org> > --- > drivers/gpu/drm/sun4i/sun4i_rgb.c | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/sun4i/sun4i_rgb.c b/drivers/gpu/drm/sun4i/sun4i_rgb.c > index d4e52522ec53..ee0795152a33 100644 > --- a/drivers/gpu/drm/sun4i/sun4i_rgb.c > +++ b/drivers/gpu/drm/sun4i/sun4i_rgb.c > @@ -154,8 +154,7 @@ static void sun4i_rgb_encoder_enable(struct drm_encoder *encoder) > if (!IS_ERR(tcon->panel)) > drm_panel_enable(tcon->panel); > > - if (!IS_ERR(encoder->bridge)) > - drm_bridge_enable(encoder->bridge); > + drm_bridge_enable(encoder->bridge); > > sun4i_tcon_channel_enable(tcon, 0); > } > @@ -170,8 +169,7 @@ static void sun4i_rgb_encoder_disable(struct drm_encoder *encoder) > > sun4i_tcon_channel_disable(tcon, 0); > > - if (!IS_ERR(encoder->bridge)) > - drm_bridge_disable(encoder->bridge); > + drm_bridge_disable(encoder->bridge); I'd rather keep those changes, it makes it obvious that it's something optionnal, that can be set to NULL. > if (!IS_ERR(tcon->panel)) > drm_panel_disable(tcon->panel); > @@ -230,6 +228,9 @@ int sun4i_rgb_init(struct drm_device *drm) > return 0; > } > > + if (IS_ERR(encoder->bridge)) > + encoder->bridge = NULL; > + And that could be the else condition of the if statement below. Thanks! Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] drm/sun4i: Clear encoder->bridge if a bridge is not found @ 2016-08-30 12:56 ` Maxime Ripard 0 siblings, 0 replies; 21+ messages in thread From: Maxime Ripard @ 2016-08-30 12:56 UTC (permalink / raw) To: Chen-Yu Tsai; +Cc: linux-arm-kernel, dri-devel, linux-kernel [-- Attachment #1.1: Type: text/plain, Size: 2114 bytes --] Hi, On Tue, Aug 30, 2016 at 08:22:23PM +0800, Chen-Yu Tsai wrote: > The KMS helpers (drm_atomic_helper_check_modeset/mode_fixup) pass > encoder->bridge directly to drm_bridge_mode_fixup, which expects a > valid pointer, or NULL (in which case it just returns). > > Clear encoder->bridge if a bridge is not found, instead of keeping > the ERR_PTR value. > > Since other drm_bridge functions also follow this pattern of checking > for a non-NULL pointer, we can drop the ifs around the calls and just > pass the pointer directly. > > Fixes: 894f5a9f4b4a ("drm/sun4i: Add bridge support") > Signed-off-by: Chen-Yu Tsai <wens@csie.org> > --- > drivers/gpu/drm/sun4i/sun4i_rgb.c | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/sun4i/sun4i_rgb.c b/drivers/gpu/drm/sun4i/sun4i_rgb.c > index d4e52522ec53..ee0795152a33 100644 > --- a/drivers/gpu/drm/sun4i/sun4i_rgb.c > +++ b/drivers/gpu/drm/sun4i/sun4i_rgb.c > @@ -154,8 +154,7 @@ static void sun4i_rgb_encoder_enable(struct drm_encoder *encoder) > if (!IS_ERR(tcon->panel)) > drm_panel_enable(tcon->panel); > > - if (!IS_ERR(encoder->bridge)) > - drm_bridge_enable(encoder->bridge); > + drm_bridge_enable(encoder->bridge); > > sun4i_tcon_channel_enable(tcon, 0); > } > @@ -170,8 +169,7 @@ static void sun4i_rgb_encoder_disable(struct drm_encoder *encoder) > > sun4i_tcon_channel_disable(tcon, 0); > > - if (!IS_ERR(encoder->bridge)) > - drm_bridge_disable(encoder->bridge); > + drm_bridge_disable(encoder->bridge); I'd rather keep those changes, it makes it obvious that it's something optionnal, that can be set to NULL. > if (!IS_ERR(tcon->panel)) > drm_panel_disable(tcon->panel); > @@ -230,6 +228,9 @@ int sun4i_rgb_init(struct drm_device *drm) > return 0; > } > > + if (IS_ERR(encoder->bridge)) > + encoder->bridge = NULL; > + And that could be the else condition of the if statement below. Thanks! Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 819 bytes --] [-- Attachment #2: Type: text/plain, Size: 160 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH] drm/sun4i: Clear encoder->bridge if a bridge is not found @ 2016-08-30 12:56 ` Maxime Ripard 0 siblings, 0 replies; 21+ messages in thread From: Maxime Ripard @ 2016-08-30 12:56 UTC (permalink / raw) To: linux-arm-kernel Hi, On Tue, Aug 30, 2016 at 08:22:23PM +0800, Chen-Yu Tsai wrote: > The KMS helpers (drm_atomic_helper_check_modeset/mode_fixup) pass > encoder->bridge directly to drm_bridge_mode_fixup, which expects a > valid pointer, or NULL (in which case it just returns). > > Clear encoder->bridge if a bridge is not found, instead of keeping > the ERR_PTR value. > > Since other drm_bridge functions also follow this pattern of checking > for a non-NULL pointer, we can drop the ifs around the calls and just > pass the pointer directly. > > Fixes: 894f5a9f4b4a ("drm/sun4i: Add bridge support") > Signed-off-by: Chen-Yu Tsai <wens@csie.org> > --- > drivers/gpu/drm/sun4i/sun4i_rgb.c | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/sun4i/sun4i_rgb.c b/drivers/gpu/drm/sun4i/sun4i_rgb.c > index d4e52522ec53..ee0795152a33 100644 > --- a/drivers/gpu/drm/sun4i/sun4i_rgb.c > +++ b/drivers/gpu/drm/sun4i/sun4i_rgb.c > @@ -154,8 +154,7 @@ static void sun4i_rgb_encoder_enable(struct drm_encoder *encoder) > if (!IS_ERR(tcon->panel)) > drm_panel_enable(tcon->panel); > > - if (!IS_ERR(encoder->bridge)) > - drm_bridge_enable(encoder->bridge); > + drm_bridge_enable(encoder->bridge); > > sun4i_tcon_channel_enable(tcon, 0); > } > @@ -170,8 +169,7 @@ static void sun4i_rgb_encoder_disable(struct drm_encoder *encoder) > > sun4i_tcon_channel_disable(tcon, 0); > > - if (!IS_ERR(encoder->bridge)) > - drm_bridge_disable(encoder->bridge); > + drm_bridge_disable(encoder->bridge); I'd rather keep those changes, it makes it obvious that it's something optionnal, that can be set to NULL. > if (!IS_ERR(tcon->panel)) > drm_panel_disable(tcon->panel); > @@ -230,6 +228,9 @@ int sun4i_rgb_init(struct drm_device *drm) > return 0; > } > > + if (IS_ERR(encoder->bridge)) > + encoder->bridge = NULL; > + And that could be the else condition of the if statement below. Thanks! Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160830/9deb77a5/attachment.sig> ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] drm/sun4i: Clear encoder->bridge if a bridge is not found 2016-08-30 12:56 ` Maxime Ripard @ 2016-08-30 15:51 ` Chen-Yu Tsai -1 siblings, 0 replies; 21+ messages in thread From: Chen-Yu Tsai @ 2016-08-30 15:51 UTC (permalink / raw) To: Maxime Ripard Cc: Chen-Yu Tsai, David Airlie, dri-devel, linux-arm-kernel, linux-kernel On Tue, Aug 30, 2016 at 8:56 PM, Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > Hi, > > On Tue, Aug 30, 2016 at 08:22:23PM +0800, Chen-Yu Tsai wrote: >> The KMS helpers (drm_atomic_helper_check_modeset/mode_fixup) pass >> encoder->bridge directly to drm_bridge_mode_fixup, which expects a >> valid pointer, or NULL (in which case it just returns). >> >> Clear encoder->bridge if a bridge is not found, instead of keeping >> the ERR_PTR value. >> >> Since other drm_bridge functions also follow this pattern of checking >> for a non-NULL pointer, we can drop the ifs around the calls and just >> pass the pointer directly. >> >> Fixes: 894f5a9f4b4a ("drm/sun4i: Add bridge support") >> Signed-off-by: Chen-Yu Tsai <wens@csie.org> >> --- >> drivers/gpu/drm/sun4i/sun4i_rgb.c | 11 ++++++----- >> 1 file changed, 6 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/gpu/drm/sun4i/sun4i_rgb.c b/drivers/gpu/drm/sun4i/sun4i_rgb.c >> index d4e52522ec53..ee0795152a33 100644 >> --- a/drivers/gpu/drm/sun4i/sun4i_rgb.c >> +++ b/drivers/gpu/drm/sun4i/sun4i_rgb.c >> @@ -154,8 +154,7 @@ static void sun4i_rgb_encoder_enable(struct drm_encoder *encoder) >> if (!IS_ERR(tcon->panel)) >> drm_panel_enable(tcon->panel); >> >> - if (!IS_ERR(encoder->bridge)) >> - drm_bridge_enable(encoder->bridge); >> + drm_bridge_enable(encoder->bridge); >> >> sun4i_tcon_channel_enable(tcon, 0); >> } >> @@ -170,8 +169,7 @@ static void sun4i_rgb_encoder_disable(struct drm_encoder *encoder) >> >> sun4i_tcon_channel_disable(tcon, 0); >> >> - if (!IS_ERR(encoder->bridge)) >> - drm_bridge_disable(encoder->bridge); >> + drm_bridge_disable(encoder->bridge); > > I'd rather keep those changes, it makes it obvious that it's something > optionnal, that can be set to NULL. OK. >> if (!IS_ERR(tcon->panel)) >> drm_panel_disable(tcon->panel); >> @@ -230,6 +228,9 @@ int sun4i_rgb_init(struct drm_device *drm) >> return 0; >> } >> >> + if (IS_ERR(encoder->bridge)) >> + encoder->bridge = NULL; >> + > > And that could be the else condition of the if statement below. That would be a bit confusing, changing it after calling drm_encoder_init. The code says it ok to do though. ChenYu > Thanks! > Maxime > > -- > Maxime Ripard, Free Electrons > Embedded Linux and Kernel engineering > http://free-electrons.com ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH] drm/sun4i: Clear encoder->bridge if a bridge is not found @ 2016-08-30 15:51 ` Chen-Yu Tsai 0 siblings, 0 replies; 21+ messages in thread From: Chen-Yu Tsai @ 2016-08-30 15:51 UTC (permalink / raw) To: linux-arm-kernel On Tue, Aug 30, 2016 at 8:56 PM, Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > Hi, > > On Tue, Aug 30, 2016 at 08:22:23PM +0800, Chen-Yu Tsai wrote: >> The KMS helpers (drm_atomic_helper_check_modeset/mode_fixup) pass >> encoder->bridge directly to drm_bridge_mode_fixup, which expects a >> valid pointer, or NULL (in which case it just returns). >> >> Clear encoder->bridge if a bridge is not found, instead of keeping >> the ERR_PTR value. >> >> Since other drm_bridge functions also follow this pattern of checking >> for a non-NULL pointer, we can drop the ifs around the calls and just >> pass the pointer directly. >> >> Fixes: 894f5a9f4b4a ("drm/sun4i: Add bridge support") >> Signed-off-by: Chen-Yu Tsai <wens@csie.org> >> --- >> drivers/gpu/drm/sun4i/sun4i_rgb.c | 11 ++++++----- >> 1 file changed, 6 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/gpu/drm/sun4i/sun4i_rgb.c b/drivers/gpu/drm/sun4i/sun4i_rgb.c >> index d4e52522ec53..ee0795152a33 100644 >> --- a/drivers/gpu/drm/sun4i/sun4i_rgb.c >> +++ b/drivers/gpu/drm/sun4i/sun4i_rgb.c >> @@ -154,8 +154,7 @@ static void sun4i_rgb_encoder_enable(struct drm_encoder *encoder) >> if (!IS_ERR(tcon->panel)) >> drm_panel_enable(tcon->panel); >> >> - if (!IS_ERR(encoder->bridge)) >> - drm_bridge_enable(encoder->bridge); >> + drm_bridge_enable(encoder->bridge); >> >> sun4i_tcon_channel_enable(tcon, 0); >> } >> @@ -170,8 +169,7 @@ static void sun4i_rgb_encoder_disable(struct drm_encoder *encoder) >> >> sun4i_tcon_channel_disable(tcon, 0); >> >> - if (!IS_ERR(encoder->bridge)) >> - drm_bridge_disable(encoder->bridge); >> + drm_bridge_disable(encoder->bridge); > > I'd rather keep those changes, it makes it obvious that it's something > optionnal, that can be set to NULL. OK. >> if (!IS_ERR(tcon->panel)) >> drm_panel_disable(tcon->panel); >> @@ -230,6 +228,9 @@ int sun4i_rgb_init(struct drm_device *drm) >> return 0; >> } >> >> + if (IS_ERR(encoder->bridge)) >> + encoder->bridge = NULL; >> + > > And that could be the else condition of the if statement below. That would be a bit confusing, changing it after calling drm_encoder_init. The code says it ok to do though. ChenYu > Thanks! > Maxime > > -- > Maxime Ripard, Free Electrons > Embedded Linux and Kernel engineering > http://free-electrons.com ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] drm/sun4i: Clear encoder->bridge if a bridge is not found 2016-08-30 15:51 ` Chen-Yu Tsai @ 2016-08-31 11:09 ` Chen-Yu Tsai -1 siblings, 0 replies; 21+ messages in thread From: Chen-Yu Tsai @ 2016-08-31 11:09 UTC (permalink / raw) To: Chen-Yu Tsai Cc: Maxime Ripard, David Airlie, dri-devel, linux-arm-kernel, linux-kernel Hi Maxime, On Tue, Aug 30, 2016 at 11:51 PM, Chen-Yu Tsai <wens@csie.org> wrote: > On Tue, Aug 30, 2016 at 8:56 PM, Maxime Ripard > <maxime.ripard@free-electrons.com> wrote: >> Hi, >> >> On Tue, Aug 30, 2016 at 08:22:23PM +0800, Chen-Yu Tsai wrote: >>> The KMS helpers (drm_atomic_helper_check_modeset/mode_fixup) pass >>> encoder->bridge directly to drm_bridge_mode_fixup, which expects a >>> valid pointer, or NULL (in which case it just returns). >>> >>> Clear encoder->bridge if a bridge is not found, instead of keeping >>> the ERR_PTR value. >>> >>> Since other drm_bridge functions also follow this pattern of checking >>> for a non-NULL pointer, we can drop the ifs around the calls and just >>> pass the pointer directly. >>> >>> Fixes: 894f5a9f4b4a ("drm/sun4i: Add bridge support") >>> Signed-off-by: Chen-Yu Tsai <wens@csie.org> >>> --- >>> drivers/gpu/drm/sun4i/sun4i_rgb.c | 11 ++++++----- >>> 1 file changed, 6 insertions(+), 5 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/sun4i/sun4i_rgb.c b/drivers/gpu/drm/sun4i/sun4i_rgb.c >>> index d4e52522ec53..ee0795152a33 100644 >>> --- a/drivers/gpu/drm/sun4i/sun4i_rgb.c >>> +++ b/drivers/gpu/drm/sun4i/sun4i_rgb.c >>> @@ -154,8 +154,7 @@ static void sun4i_rgb_encoder_enable(struct drm_encoder *encoder) >>> if (!IS_ERR(tcon->panel)) >>> drm_panel_enable(tcon->panel); >>> >>> - if (!IS_ERR(encoder->bridge)) >>> - drm_bridge_enable(encoder->bridge); >>> + drm_bridge_enable(encoder->bridge); >>> >>> sun4i_tcon_channel_enable(tcon, 0); >>> } >>> @@ -170,8 +169,7 @@ static void sun4i_rgb_encoder_disable(struct drm_encoder *encoder) >>> >>> sun4i_tcon_channel_disable(tcon, 0); >>> >>> - if (!IS_ERR(encoder->bridge)) >>> - drm_bridge_disable(encoder->bridge); >>> + drm_bridge_disable(encoder->bridge); >> >> I'd rather keep those changes, it makes it obvious that it's something >> optionnal, that can be set to NULL. > > OK. What about having a comment instead? Saves an extra branch condition, while still showing that it's optional. ChenYu > >>> if (!IS_ERR(tcon->panel)) >>> drm_panel_disable(tcon->panel); >>> @@ -230,6 +228,9 @@ int sun4i_rgb_init(struct drm_device *drm) >>> return 0; >>> } >>> >>> + if (IS_ERR(encoder->bridge)) >>> + encoder->bridge = NULL; >>> + >> >> And that could be the else condition of the if statement below. > > That would be a bit confusing, changing it after calling drm_encoder_init. > The code says it ok to do though. > > ChenYu > >> Thanks! >> Maxime >> >> -- >> Maxime Ripard, Free Electrons >> Embedded Linux and Kernel engineering >> http://free-electrons.com ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH] drm/sun4i: Clear encoder->bridge if a bridge is not found @ 2016-08-31 11:09 ` Chen-Yu Tsai 0 siblings, 0 replies; 21+ messages in thread From: Chen-Yu Tsai @ 2016-08-31 11:09 UTC (permalink / raw) To: linux-arm-kernel Hi Maxime, On Tue, Aug 30, 2016 at 11:51 PM, Chen-Yu Tsai <wens@csie.org> wrote: > On Tue, Aug 30, 2016 at 8:56 PM, Maxime Ripard > <maxime.ripard@free-electrons.com> wrote: >> Hi, >> >> On Tue, Aug 30, 2016 at 08:22:23PM +0800, Chen-Yu Tsai wrote: >>> The KMS helpers (drm_atomic_helper_check_modeset/mode_fixup) pass >>> encoder->bridge directly to drm_bridge_mode_fixup, which expects a >>> valid pointer, or NULL (in which case it just returns). >>> >>> Clear encoder->bridge if a bridge is not found, instead of keeping >>> the ERR_PTR value. >>> >>> Since other drm_bridge functions also follow this pattern of checking >>> for a non-NULL pointer, we can drop the ifs around the calls and just >>> pass the pointer directly. >>> >>> Fixes: 894f5a9f4b4a ("drm/sun4i: Add bridge support") >>> Signed-off-by: Chen-Yu Tsai <wens@csie.org> >>> --- >>> drivers/gpu/drm/sun4i/sun4i_rgb.c | 11 ++++++----- >>> 1 file changed, 6 insertions(+), 5 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/sun4i/sun4i_rgb.c b/drivers/gpu/drm/sun4i/sun4i_rgb.c >>> index d4e52522ec53..ee0795152a33 100644 >>> --- a/drivers/gpu/drm/sun4i/sun4i_rgb.c >>> +++ b/drivers/gpu/drm/sun4i/sun4i_rgb.c >>> @@ -154,8 +154,7 @@ static void sun4i_rgb_encoder_enable(struct drm_encoder *encoder) >>> if (!IS_ERR(tcon->panel)) >>> drm_panel_enable(tcon->panel); >>> >>> - if (!IS_ERR(encoder->bridge)) >>> - drm_bridge_enable(encoder->bridge); >>> + drm_bridge_enable(encoder->bridge); >>> >>> sun4i_tcon_channel_enable(tcon, 0); >>> } >>> @@ -170,8 +169,7 @@ static void sun4i_rgb_encoder_disable(struct drm_encoder *encoder) >>> >>> sun4i_tcon_channel_disable(tcon, 0); >>> >>> - if (!IS_ERR(encoder->bridge)) >>> - drm_bridge_disable(encoder->bridge); >>> + drm_bridge_disable(encoder->bridge); >> >> I'd rather keep those changes, it makes it obvious that it's something >> optionnal, that can be set to NULL. > > OK. What about having a comment instead? Saves an extra branch condition, while still showing that it's optional. ChenYu > >>> if (!IS_ERR(tcon->panel)) >>> drm_panel_disable(tcon->panel); >>> @@ -230,6 +228,9 @@ int sun4i_rgb_init(struct drm_device *drm) >>> return 0; >>> } >>> >>> + if (IS_ERR(encoder->bridge)) >>> + encoder->bridge = NULL; >>> + >> >> And that could be the else condition of the if statement below. > > That would be a bit confusing, changing it after calling drm_encoder_init. > The code says it ok to do though. > > ChenYu > >> Thanks! >> Maxime >> >> -- >> Maxime Ripard, Free Electrons >> Embedded Linux and Kernel engineering >> http://free-electrons.com ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] drm/sun4i: Clear encoder->bridge if a bridge is not found 2016-08-31 11:09 ` Chen-Yu Tsai (?) @ 2016-08-31 16:41 ` Maxime Ripard -1 siblings, 0 replies; 21+ messages in thread From: Maxime Ripard @ 2016-08-31 16:41 UTC (permalink / raw) To: Chen-Yu Tsai; +Cc: David Airlie, dri-devel, linux-arm-kernel, linux-kernel [-- Attachment #1: Type: text/plain, Size: 740 bytes --] On Wed, Aug 31, 2016 at 07:09:23PM +0800, Chen-Yu Tsai wrote: > >>> sun4i_tcon_channel_disable(tcon, 0); > >>> > >>> - if (!IS_ERR(encoder->bridge)) > >>> - drm_bridge_disable(encoder->bridge); > >>> + drm_bridge_disable(encoder->bridge); > >> > >> I'd rather keep those changes, it makes it obvious that it's something > >> optionnal, that can be set to NULL. > > > > OK. > > What about having a comment instead? Saves an extra branch condition, > while still showing that it's optional. I'm not sure we have to worry about an extra branch condition, but yeah, that works for me. Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] drm/sun4i: Clear encoder->bridge if a bridge is not found @ 2016-08-31 16:41 ` Maxime Ripard 0 siblings, 0 replies; 21+ messages in thread From: Maxime Ripard @ 2016-08-31 16:41 UTC (permalink / raw) To: Chen-Yu Tsai; +Cc: linux-arm-kernel, dri-devel, linux-kernel [-- Attachment #1.1: Type: text/plain, Size: 740 bytes --] On Wed, Aug 31, 2016 at 07:09:23PM +0800, Chen-Yu Tsai wrote: > >>> sun4i_tcon_channel_disable(tcon, 0); > >>> > >>> - if (!IS_ERR(encoder->bridge)) > >>> - drm_bridge_disable(encoder->bridge); > >>> + drm_bridge_disable(encoder->bridge); > >> > >> I'd rather keep those changes, it makes it obvious that it's something > >> optionnal, that can be set to NULL. > > > > OK. > > What about having a comment instead? Saves an extra branch condition, > while still showing that it's optional. I'm not sure we have to worry about an extra branch condition, but yeah, that works for me. Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 819 bytes --] [-- Attachment #2: Type: text/plain, Size: 160 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH] drm/sun4i: Clear encoder->bridge if a bridge is not found @ 2016-08-31 16:41 ` Maxime Ripard 0 siblings, 0 replies; 21+ messages in thread From: Maxime Ripard @ 2016-08-31 16:41 UTC (permalink / raw) To: linux-arm-kernel On Wed, Aug 31, 2016 at 07:09:23PM +0800, Chen-Yu Tsai wrote: > >>> sun4i_tcon_channel_disable(tcon, 0); > >>> > >>> - if (!IS_ERR(encoder->bridge)) > >>> - drm_bridge_disable(encoder->bridge); > >>> + drm_bridge_disable(encoder->bridge); > >> > >> I'd rather keep those changes, it makes it obvious that it's something > >> optionnal, that can be set to NULL. > > > > OK. > > What about having a comment instead? Saves an extra branch condition, > while still showing that it's optional. I'm not sure we have to worry about an extra branch condition, but yeah, that works for me. Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160831/05fc9ca6/attachment.sig> ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] drm/sun4i: Clear encoder->bridge if a bridge is not found 2016-08-30 15:51 ` Chen-Yu Tsai (?) @ 2016-08-31 15:40 ` Maxime Ripard -1 siblings, 0 replies; 21+ messages in thread From: Maxime Ripard @ 2016-08-31 15:40 UTC (permalink / raw) To: Chen-Yu Tsai; +Cc: David Airlie, dri-devel, linux-arm-kernel, linux-kernel [-- Attachment #1: Type: text/plain, Size: 2762 bytes --] On Tue, Aug 30, 2016 at 11:51:26PM +0800, Chen-Yu Tsai wrote: > On Tue, Aug 30, 2016 at 8:56 PM, Maxime Ripard > <maxime.ripard@free-electrons.com> wrote: > > Hi, > > > > On Tue, Aug 30, 2016 at 08:22:23PM +0800, Chen-Yu Tsai wrote: > >> The KMS helpers (drm_atomic_helper_check_modeset/mode_fixup) pass > >> encoder->bridge directly to drm_bridge_mode_fixup, which expects a > >> valid pointer, or NULL (in which case it just returns). > >> > >> Clear encoder->bridge if a bridge is not found, instead of keeping > >> the ERR_PTR value. > >> > >> Since other drm_bridge functions also follow this pattern of checking > >> for a non-NULL pointer, we can drop the ifs around the calls and just > >> pass the pointer directly. > >> > >> Fixes: 894f5a9f4b4a ("drm/sun4i: Add bridge support") > >> Signed-off-by: Chen-Yu Tsai <wens@csie.org> > >> --- > >> drivers/gpu/drm/sun4i/sun4i_rgb.c | 11 ++++++----- > >> 1 file changed, 6 insertions(+), 5 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/sun4i/sun4i_rgb.c b/drivers/gpu/drm/sun4i/sun4i_rgb.c > >> index d4e52522ec53..ee0795152a33 100644 > >> --- a/drivers/gpu/drm/sun4i/sun4i_rgb.c > >> +++ b/drivers/gpu/drm/sun4i/sun4i_rgb.c > >> @@ -154,8 +154,7 @@ static void sun4i_rgb_encoder_enable(struct drm_encoder *encoder) > >> if (!IS_ERR(tcon->panel)) > >> drm_panel_enable(tcon->panel); > >> > >> - if (!IS_ERR(encoder->bridge)) > >> - drm_bridge_enable(encoder->bridge); > >> + drm_bridge_enable(encoder->bridge); > >> > >> sun4i_tcon_channel_enable(tcon, 0); > >> } > >> @@ -170,8 +169,7 @@ static void sun4i_rgb_encoder_disable(struct drm_encoder *encoder) > >> > >> sun4i_tcon_channel_disable(tcon, 0); > >> > >> - if (!IS_ERR(encoder->bridge)) > >> - drm_bridge_disable(encoder->bridge); > >> + drm_bridge_disable(encoder->bridge); > > > > I'd rather keep those changes, it makes it obvious that it's something > > optionnal, that can be set to NULL. > > OK. > > >> if (!IS_ERR(tcon->panel)) > >> drm_panel_disable(tcon->panel); > >> @@ -230,6 +228,9 @@ int sun4i_rgb_init(struct drm_device *drm) > >> return 0; > >> } > >> > >> + if (IS_ERR(encoder->bridge)) > >> + encoder->bridge = NULL; > >> + > > > > And that could be the else condition of the if statement below. > > That would be a bit confusing, changing it after calling drm_encoder_init. > The code says it ok to do though. The magic really happens only after the encoder has been attached to something, so it's really safe. Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] drm/sun4i: Clear encoder->bridge if a bridge is not found @ 2016-08-31 15:40 ` Maxime Ripard 0 siblings, 0 replies; 21+ messages in thread From: Maxime Ripard @ 2016-08-31 15:40 UTC (permalink / raw) To: Chen-Yu Tsai; +Cc: linux-arm-kernel, dri-devel, linux-kernel [-- Attachment #1.1: Type: text/plain, Size: 2762 bytes --] On Tue, Aug 30, 2016 at 11:51:26PM +0800, Chen-Yu Tsai wrote: > On Tue, Aug 30, 2016 at 8:56 PM, Maxime Ripard > <maxime.ripard@free-electrons.com> wrote: > > Hi, > > > > On Tue, Aug 30, 2016 at 08:22:23PM +0800, Chen-Yu Tsai wrote: > >> The KMS helpers (drm_atomic_helper_check_modeset/mode_fixup) pass > >> encoder->bridge directly to drm_bridge_mode_fixup, which expects a > >> valid pointer, or NULL (in which case it just returns). > >> > >> Clear encoder->bridge if a bridge is not found, instead of keeping > >> the ERR_PTR value. > >> > >> Since other drm_bridge functions also follow this pattern of checking > >> for a non-NULL pointer, we can drop the ifs around the calls and just > >> pass the pointer directly. > >> > >> Fixes: 894f5a9f4b4a ("drm/sun4i: Add bridge support") > >> Signed-off-by: Chen-Yu Tsai <wens@csie.org> > >> --- > >> drivers/gpu/drm/sun4i/sun4i_rgb.c | 11 ++++++----- > >> 1 file changed, 6 insertions(+), 5 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/sun4i/sun4i_rgb.c b/drivers/gpu/drm/sun4i/sun4i_rgb.c > >> index d4e52522ec53..ee0795152a33 100644 > >> --- a/drivers/gpu/drm/sun4i/sun4i_rgb.c > >> +++ b/drivers/gpu/drm/sun4i/sun4i_rgb.c > >> @@ -154,8 +154,7 @@ static void sun4i_rgb_encoder_enable(struct drm_encoder *encoder) > >> if (!IS_ERR(tcon->panel)) > >> drm_panel_enable(tcon->panel); > >> > >> - if (!IS_ERR(encoder->bridge)) > >> - drm_bridge_enable(encoder->bridge); > >> + drm_bridge_enable(encoder->bridge); > >> > >> sun4i_tcon_channel_enable(tcon, 0); > >> } > >> @@ -170,8 +169,7 @@ static void sun4i_rgb_encoder_disable(struct drm_encoder *encoder) > >> > >> sun4i_tcon_channel_disable(tcon, 0); > >> > >> - if (!IS_ERR(encoder->bridge)) > >> - drm_bridge_disable(encoder->bridge); > >> + drm_bridge_disable(encoder->bridge); > > > > I'd rather keep those changes, it makes it obvious that it's something > > optionnal, that can be set to NULL. > > OK. > > >> if (!IS_ERR(tcon->panel)) > >> drm_panel_disable(tcon->panel); > >> @@ -230,6 +228,9 @@ int sun4i_rgb_init(struct drm_device *drm) > >> return 0; > >> } > >> > >> + if (IS_ERR(encoder->bridge)) > >> + encoder->bridge = NULL; > >> + > > > > And that could be the else condition of the if statement below. > > That would be a bit confusing, changing it after calling drm_encoder_init. > The code says it ok to do though. The magic really happens only after the encoder has been attached to something, so it's really safe. Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 819 bytes --] [-- Attachment #2: Type: text/plain, Size: 160 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH] drm/sun4i: Clear encoder->bridge if a bridge is not found @ 2016-08-31 15:40 ` Maxime Ripard 0 siblings, 0 replies; 21+ messages in thread From: Maxime Ripard @ 2016-08-31 15:40 UTC (permalink / raw) To: linux-arm-kernel On Tue, Aug 30, 2016 at 11:51:26PM +0800, Chen-Yu Tsai wrote: > On Tue, Aug 30, 2016 at 8:56 PM, Maxime Ripard > <maxime.ripard@free-electrons.com> wrote: > > Hi, > > > > On Tue, Aug 30, 2016 at 08:22:23PM +0800, Chen-Yu Tsai wrote: > >> The KMS helpers (drm_atomic_helper_check_modeset/mode_fixup) pass > >> encoder->bridge directly to drm_bridge_mode_fixup, which expects a > >> valid pointer, or NULL (in which case it just returns). > >> > >> Clear encoder->bridge if a bridge is not found, instead of keeping > >> the ERR_PTR value. > >> > >> Since other drm_bridge functions also follow this pattern of checking > >> for a non-NULL pointer, we can drop the ifs around the calls and just > >> pass the pointer directly. > >> > >> Fixes: 894f5a9f4b4a ("drm/sun4i: Add bridge support") > >> Signed-off-by: Chen-Yu Tsai <wens@csie.org> > >> --- > >> drivers/gpu/drm/sun4i/sun4i_rgb.c | 11 ++++++----- > >> 1 file changed, 6 insertions(+), 5 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/sun4i/sun4i_rgb.c b/drivers/gpu/drm/sun4i/sun4i_rgb.c > >> index d4e52522ec53..ee0795152a33 100644 > >> --- a/drivers/gpu/drm/sun4i/sun4i_rgb.c > >> +++ b/drivers/gpu/drm/sun4i/sun4i_rgb.c > >> @@ -154,8 +154,7 @@ static void sun4i_rgb_encoder_enable(struct drm_encoder *encoder) > >> if (!IS_ERR(tcon->panel)) > >> drm_panel_enable(tcon->panel); > >> > >> - if (!IS_ERR(encoder->bridge)) > >> - drm_bridge_enable(encoder->bridge); > >> + drm_bridge_enable(encoder->bridge); > >> > >> sun4i_tcon_channel_enable(tcon, 0); > >> } > >> @@ -170,8 +169,7 @@ static void sun4i_rgb_encoder_disable(struct drm_encoder *encoder) > >> > >> sun4i_tcon_channel_disable(tcon, 0); > >> > >> - if (!IS_ERR(encoder->bridge)) > >> - drm_bridge_disable(encoder->bridge); > >> + drm_bridge_disable(encoder->bridge); > > > > I'd rather keep those changes, it makes it obvious that it's something > > optionnal, that can be set to NULL. > > OK. > > >> if (!IS_ERR(tcon->panel)) > >> drm_panel_disable(tcon->panel); > >> @@ -230,6 +228,9 @@ int sun4i_rgb_init(struct drm_device *drm) > >> return 0; > >> } > >> > >> + if (IS_ERR(encoder->bridge)) > >> + encoder->bridge = NULL; > >> + > > > > And that could be the else condition of the if statement below. > > That would be a bit confusing, changing it after calling drm_encoder_init. > The code says it ok to do though. The magic really happens only after the encoder has been attached to something, so it's really safe. Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160831/6f45c0e7/attachment.sig> ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] drm/sun4i: Clear encoder->bridge if a bridge is not found 2016-08-31 15:40 ` Maxime Ripard (?) @ 2016-08-31 16:27 ` Daniel Vetter -1 siblings, 0 replies; 21+ messages in thread From: Daniel Vetter @ 2016-08-31 16:27 UTC (permalink / raw) To: Maxime Ripard; +Cc: Chen-Yu Tsai, linux-arm-kernel, dri-devel, linux-kernel On Wed, Aug 31, 2016 at 05:40:02PM +0200, Maxime Ripard wrote: > On Tue, Aug 30, 2016 at 11:51:26PM +0800, Chen-Yu Tsai wrote: > > On Tue, Aug 30, 2016 at 8:56 PM, Maxime Ripard > > <maxime.ripard@free-electrons.com> wrote: > > > Hi, > > > > > > On Tue, Aug 30, 2016 at 08:22:23PM +0800, Chen-Yu Tsai wrote: > > >> The KMS helpers (drm_atomic_helper_check_modeset/mode_fixup) pass > > >> encoder->bridge directly to drm_bridge_mode_fixup, which expects a > > >> valid pointer, or NULL (in which case it just returns). > > >> > > >> Clear encoder->bridge if a bridge is not found, instead of keeping > > >> the ERR_PTR value. > > >> > > >> Since other drm_bridge functions also follow this pattern of checking > > >> for a non-NULL pointer, we can drop the ifs around the calls and just > > >> pass the pointer directly. > > >> > > >> Fixes: 894f5a9f4b4a ("drm/sun4i: Add bridge support") > > >> Signed-off-by: Chen-Yu Tsai <wens@csie.org> > > >> --- > > >> drivers/gpu/drm/sun4i/sun4i_rgb.c | 11 ++++++----- > > >> 1 file changed, 6 insertions(+), 5 deletions(-) > > >> > > >> diff --git a/drivers/gpu/drm/sun4i/sun4i_rgb.c b/drivers/gpu/drm/sun4i/sun4i_rgb.c > > >> index d4e52522ec53..ee0795152a33 100644 > > >> --- a/drivers/gpu/drm/sun4i/sun4i_rgb.c > > >> +++ b/drivers/gpu/drm/sun4i/sun4i_rgb.c > > >> @@ -154,8 +154,7 @@ static void sun4i_rgb_encoder_enable(struct drm_encoder *encoder) > > >> if (!IS_ERR(tcon->panel)) > > >> drm_panel_enable(tcon->panel); > > >> > > >> - if (!IS_ERR(encoder->bridge)) > > >> - drm_bridge_enable(encoder->bridge); > > >> + drm_bridge_enable(encoder->bridge); > > >> > > >> sun4i_tcon_channel_enable(tcon, 0); > > >> } > > >> @@ -170,8 +169,7 @@ static void sun4i_rgb_encoder_disable(struct drm_encoder *encoder) > > >> > > >> sun4i_tcon_channel_disable(tcon, 0); > > >> > > >> - if (!IS_ERR(encoder->bridge)) > > >> - drm_bridge_disable(encoder->bridge); > > >> + drm_bridge_disable(encoder->bridge); > > > > > > I'd rather keep those changes, it makes it obvious that it's something > > > optionnal, that can be set to NULL. > > > > OK. > > > > >> if (!IS_ERR(tcon->panel)) > > >> drm_panel_disable(tcon->panel); > > >> @@ -230,6 +228,9 @@ int sun4i_rgb_init(struct drm_device *drm) > > >> return 0; > > >> } > > >> > > >> + if (IS_ERR(encoder->bridge)) > > >> + encoder->bridge = NULL; > > >> + > > > > > > And that could be the else condition of the if statement below. > > > > That would be a bit confusing, changing it after calling drm_encoder_init. > > The code says it ok to do though. > > The magic really happens only after the encoder has been attached to > something, so it's really safe. s/attached/registered using drm_dev_register(). Which should happen _way_ later for all drivers which have gotten rid of their ->load callback and implemented the recommend driver load sequence. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] drm/sun4i: Clear encoder->bridge if a bridge is not found @ 2016-08-31 16:27 ` Daniel Vetter 0 siblings, 0 replies; 21+ messages in thread From: Daniel Vetter @ 2016-08-31 16:27 UTC (permalink / raw) To: Maxime Ripard; +Cc: Chen-Yu Tsai, dri-devel, linux-arm-kernel, linux-kernel On Wed, Aug 31, 2016 at 05:40:02PM +0200, Maxime Ripard wrote: > On Tue, Aug 30, 2016 at 11:51:26PM +0800, Chen-Yu Tsai wrote: > > On Tue, Aug 30, 2016 at 8:56 PM, Maxime Ripard > > <maxime.ripard@free-electrons.com> wrote: > > > Hi, > > > > > > On Tue, Aug 30, 2016 at 08:22:23PM +0800, Chen-Yu Tsai wrote: > > >> The KMS helpers (drm_atomic_helper_check_modeset/mode_fixup) pass > > >> encoder->bridge directly to drm_bridge_mode_fixup, which expects a > > >> valid pointer, or NULL (in which case it just returns). > > >> > > >> Clear encoder->bridge if a bridge is not found, instead of keeping > > >> the ERR_PTR value. > > >> > > >> Since other drm_bridge functions also follow this pattern of checking > > >> for a non-NULL pointer, we can drop the ifs around the calls and just > > >> pass the pointer directly. > > >> > > >> Fixes: 894f5a9f4b4a ("drm/sun4i: Add bridge support") > > >> Signed-off-by: Chen-Yu Tsai <wens@csie.org> > > >> --- > > >> drivers/gpu/drm/sun4i/sun4i_rgb.c | 11 ++++++----- > > >> 1 file changed, 6 insertions(+), 5 deletions(-) > > >> > > >> diff --git a/drivers/gpu/drm/sun4i/sun4i_rgb.c b/drivers/gpu/drm/sun4i/sun4i_rgb.c > > >> index d4e52522ec53..ee0795152a33 100644 > > >> --- a/drivers/gpu/drm/sun4i/sun4i_rgb.c > > >> +++ b/drivers/gpu/drm/sun4i/sun4i_rgb.c > > >> @@ -154,8 +154,7 @@ static void sun4i_rgb_encoder_enable(struct drm_encoder *encoder) > > >> if (!IS_ERR(tcon->panel)) > > >> drm_panel_enable(tcon->panel); > > >> > > >> - if (!IS_ERR(encoder->bridge)) > > >> - drm_bridge_enable(encoder->bridge); > > >> + drm_bridge_enable(encoder->bridge); > > >> > > >> sun4i_tcon_channel_enable(tcon, 0); > > >> } > > >> @@ -170,8 +169,7 @@ static void sun4i_rgb_encoder_disable(struct drm_encoder *encoder) > > >> > > >> sun4i_tcon_channel_disable(tcon, 0); > > >> > > >> - if (!IS_ERR(encoder->bridge)) > > >> - drm_bridge_disable(encoder->bridge); > > >> + drm_bridge_disable(encoder->bridge); > > > > > > I'd rather keep those changes, it makes it obvious that it's something > > > optionnal, that can be set to NULL. > > > > OK. > > > > >> if (!IS_ERR(tcon->panel)) > > >> drm_panel_disable(tcon->panel); > > >> @@ -230,6 +228,9 @@ int sun4i_rgb_init(struct drm_device *drm) > > >> return 0; > > >> } > > >> > > >> + if (IS_ERR(encoder->bridge)) > > >> + encoder->bridge = NULL; > > >> + > > > > > > And that could be the else condition of the if statement below. > > > > That would be a bit confusing, changing it after calling drm_encoder_init. > > The code says it ok to do though. > > The magic really happens only after the encoder has been attached to > something, so it's really safe. s/attached/registered using drm_dev_register(). Which should happen _way_ later for all drivers which have gotten rid of their ->load callback and implemented the recommend driver load sequence. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH] drm/sun4i: Clear encoder->bridge if a bridge is not found @ 2016-08-31 16:27 ` Daniel Vetter 0 siblings, 0 replies; 21+ messages in thread From: Daniel Vetter @ 2016-08-31 16:27 UTC (permalink / raw) To: linux-arm-kernel On Wed, Aug 31, 2016 at 05:40:02PM +0200, Maxime Ripard wrote: > On Tue, Aug 30, 2016 at 11:51:26PM +0800, Chen-Yu Tsai wrote: > > On Tue, Aug 30, 2016 at 8:56 PM, Maxime Ripard > > <maxime.ripard@free-electrons.com> wrote: > > > Hi, > > > > > > On Tue, Aug 30, 2016 at 08:22:23PM +0800, Chen-Yu Tsai wrote: > > >> The KMS helpers (drm_atomic_helper_check_modeset/mode_fixup) pass > > >> encoder->bridge directly to drm_bridge_mode_fixup, which expects a > > >> valid pointer, or NULL (in which case it just returns). > > >> > > >> Clear encoder->bridge if a bridge is not found, instead of keeping > > >> the ERR_PTR value. > > >> > > >> Since other drm_bridge functions also follow this pattern of checking > > >> for a non-NULL pointer, we can drop the ifs around the calls and just > > >> pass the pointer directly. > > >> > > >> Fixes: 894f5a9f4b4a ("drm/sun4i: Add bridge support") > > >> Signed-off-by: Chen-Yu Tsai <wens@csie.org> > > >> --- > > >> drivers/gpu/drm/sun4i/sun4i_rgb.c | 11 ++++++----- > > >> 1 file changed, 6 insertions(+), 5 deletions(-) > > >> > > >> diff --git a/drivers/gpu/drm/sun4i/sun4i_rgb.c b/drivers/gpu/drm/sun4i/sun4i_rgb.c > > >> index d4e52522ec53..ee0795152a33 100644 > > >> --- a/drivers/gpu/drm/sun4i/sun4i_rgb.c > > >> +++ b/drivers/gpu/drm/sun4i/sun4i_rgb.c > > >> @@ -154,8 +154,7 @@ static void sun4i_rgb_encoder_enable(struct drm_encoder *encoder) > > >> if (!IS_ERR(tcon->panel)) > > >> drm_panel_enable(tcon->panel); > > >> > > >> - if (!IS_ERR(encoder->bridge)) > > >> - drm_bridge_enable(encoder->bridge); > > >> + drm_bridge_enable(encoder->bridge); > > >> > > >> sun4i_tcon_channel_enable(tcon, 0); > > >> } > > >> @@ -170,8 +169,7 @@ static void sun4i_rgb_encoder_disable(struct drm_encoder *encoder) > > >> > > >> sun4i_tcon_channel_disable(tcon, 0); > > >> > > >> - if (!IS_ERR(encoder->bridge)) > > >> - drm_bridge_disable(encoder->bridge); > > >> + drm_bridge_disable(encoder->bridge); > > > > > > I'd rather keep those changes, it makes it obvious that it's something > > > optionnal, that can be set to NULL. > > > > OK. > > > > >> if (!IS_ERR(tcon->panel)) > > >> drm_panel_disable(tcon->panel); > > >> @@ -230,6 +228,9 @@ int sun4i_rgb_init(struct drm_device *drm) > > >> return 0; > > >> } > > >> > > >> + if (IS_ERR(encoder->bridge)) > > >> + encoder->bridge = NULL; > > >> + > > > > > > And that could be the else condition of the if statement below. > > > > That would be a bit confusing, changing it after calling drm_encoder_init. > > The code says it ok to do though. > > The magic really happens only after the encoder has been attached to > something, so it's really safe. s/attached/registered using drm_dev_register(). Which should happen _way_ later for all drivers which have gotten rid of their ->load callback and implemented the recommend driver load sequence. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] drm/sun4i: Clear encoder->bridge if a bridge is not found 2016-08-31 16:27 ` Daniel Vetter (?) @ 2016-08-31 16:43 ` Maxime Ripard -1 siblings, 0 replies; 21+ messages in thread From: Maxime Ripard @ 2016-08-31 16:43 UTC (permalink / raw) To: Chen-Yu Tsai, linux-arm-kernel, dri-devel, linux-kernel [-- Attachment #1: Type: text/plain, Size: 838 bytes --] On Wed, Aug 31, 2016 at 06:27:08PM +0200, Daniel Vetter wrote: > > > >> + if (IS_ERR(encoder->bridge)) > > > >> + encoder->bridge = NULL; > > > >> + > > > > > > > > And that could be the else condition of the if statement below. > > > > > > That would be a bit confusing, changing it after calling drm_encoder_init. > > > The code says it ok to do though. > > > > The magic really happens only after the encoder has been attached to > > something, so it's really safe. > > s/attached/registered using drm_dev_register(). Which should > happen _way_ later for all drivers which have gotten rid of their ->load > callback and implemented the recommend driver load sequence. My bad, thanks! Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] drm/sun4i: Clear encoder->bridge if a bridge is not found @ 2016-08-31 16:43 ` Maxime Ripard 0 siblings, 0 replies; 21+ messages in thread From: Maxime Ripard @ 2016-08-31 16:43 UTC (permalink / raw) To: Chen-Yu Tsai, linux-arm-kernel, dri-devel, linux-kernel [-- Attachment #1.1: Type: text/plain, Size: 838 bytes --] On Wed, Aug 31, 2016 at 06:27:08PM +0200, Daniel Vetter wrote: > > > >> + if (IS_ERR(encoder->bridge)) > > > >> + encoder->bridge = NULL; > > > >> + > > > > > > > > And that could be the else condition of the if statement below. > > > > > > That would be a bit confusing, changing it after calling drm_encoder_init. > > > The code says it ok to do though. > > > > The magic really happens only after the encoder has been attached to > > something, so it's really safe. > > s/attached/registered using drm_dev_register(). Which should > happen _way_ later for all drivers which have gotten rid of their ->load > callback and implemented the recommend driver load sequence. My bad, thanks! Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 819 bytes --] [-- Attachment #2: Type: text/plain, Size: 160 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH] drm/sun4i: Clear encoder->bridge if a bridge is not found @ 2016-08-31 16:43 ` Maxime Ripard 0 siblings, 0 replies; 21+ messages in thread From: Maxime Ripard @ 2016-08-31 16:43 UTC (permalink / raw) To: linux-arm-kernel On Wed, Aug 31, 2016 at 06:27:08PM +0200, Daniel Vetter wrote: > > > >> + if (IS_ERR(encoder->bridge)) > > > >> + encoder->bridge = NULL; > > > >> + > > > > > > > > And that could be the else condition of the if statement below. > > > > > > That would be a bit confusing, changing it after calling drm_encoder_init. > > > The code says it ok to do though. > > > > The magic really happens only after the encoder has been attached to > > something, so it's really safe. > > s/attached/registered using drm_dev_register(). Which should > happen _way_ later for all drivers which have gotten rid of their ->load > callback and implemented the recommend driver load sequence. My bad, thanks! Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160831/39904c8d/attachment.sig> ^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2016-08-31 16:43 UTC | newest] Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-08-30 12:22 [PATCH] drm/sun4i: Clear encoder->bridge if a bridge is not found Chen-Yu Tsai 2016-08-30 12:22 ` Chen-Yu Tsai 2016-08-30 12:56 ` Maxime Ripard 2016-08-30 12:56 ` Maxime Ripard 2016-08-30 12:56 ` Maxime Ripard 2016-08-30 15:51 ` Chen-Yu Tsai 2016-08-30 15:51 ` Chen-Yu Tsai 2016-08-31 11:09 ` Chen-Yu Tsai 2016-08-31 11:09 ` Chen-Yu Tsai 2016-08-31 16:41 ` Maxime Ripard 2016-08-31 16:41 ` Maxime Ripard 2016-08-31 16:41 ` Maxime Ripard 2016-08-31 15:40 ` Maxime Ripard 2016-08-31 15:40 ` Maxime Ripard 2016-08-31 15:40 ` Maxime Ripard 2016-08-31 16:27 ` Daniel Vetter 2016-08-31 16:27 ` Daniel Vetter 2016-08-31 16:27 ` Daniel Vetter 2016-08-31 16:43 ` Maxime Ripard 2016-08-31 16:43 ` Maxime Ripard 2016-08-31 16:43 ` Maxime Ripard
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.