| Message ID | 20200211114038.21035-1-stanislav.lisovskiy@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1] drm/i915: Call intel_edp_init_connector only for eDP. | expand |
On Tue, Feb 11, 2020 at 01:40:38PM +0200, Stanislav Lisovskiy wrote: > I guess it would still be nice to make the code less confusing > and do not call eDP specific function, for non-eDP connectors > just to immediately return true(success) value as a hack. > > So simply extracted that check out from this function, > that we simply don't call it for non-eDP connectors. Bingo. > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > --- > drivers/gpu/drm/i915/display/intel_dp.c | 13 ++++++------- > 1 file changed, 6 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c > index f4dede6253f8..9bd36197a43d 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp.c > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > @@ -7370,9 +7370,6 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp, > intel_wakeref_t wakeref; > struct edid *edid; > > - if (!intel_dp_is_edp(intel_dp)) > - return true; > - > INIT_DELAYED_WORK(&intel_dp->panel_vdd_work, edp_panel_vdd_work); > > /* > @@ -7590,10 +7587,12 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port, > intel_dp_mst_encoder_init(intel_dig_port, > intel_connector->base.base.id); > > - if (!intel_edp_init_connector(intel_dp, intel_connector)) { > - intel_dp_aux_fini(intel_dp); > - intel_dp_mst_encoder_cleanup(intel_dig_port); > - goto fail; > + if (intel_dp_is_edp(intel_dp)) { > + if (!intel_edp_init_connector(intel_dp, intel_connector)) { > + intel_dp_aux_fini(intel_dp); > + intel_dp_mst_encoder_cleanup(intel_dig_port); > + goto fail; > + } Yes, one jump less when reading the code. You could also use a&&b to reduce the indent: Reviewed-by: Imre Deak <imre.deak@intel.com> > } > > intel_dp_add_properties(intel_dp, connector); > -- > 2.24.1.485.gad05a3d8e5 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, 11 Feb 2020, Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> wrote: > I guess it would still be nice to make the code less confusing > and do not call eDP specific function, for non-eDP connectors > just to immediately return true(success) value as a hack. > > So simply extracted that check out from this function, > that we simply don't call it for non-eDP connectors. Bingo. Fair enough, I guess... Though I could be persuaded to take a patch for the reverse, because generally we prefer localized knowledge in the callee than in a potentially irrelevant place in the caller. Consider the intel_dp_mst_encoder_init() call in the context of this patch. We call it, and the function itself decides whether MST init is relevant or not. But I presume you wouldn't suggest pulling in all the conditions from there one level higher? Would your opinion on intel_edp_init_connector() be different if the conditions were more than just the single intel_dp_is_edp()? Or if we moved all of eDP support to a separate file? BR, Jani. > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > --- > drivers/gpu/drm/i915/display/intel_dp.c | 13 ++++++------- > 1 file changed, 6 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c > index f4dede6253f8..9bd36197a43d 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp.c > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > @@ -7370,9 +7370,6 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp, > intel_wakeref_t wakeref; > struct edid *edid; > > - if (!intel_dp_is_edp(intel_dp)) > - return true; > - > INIT_DELAYED_WORK(&intel_dp->panel_vdd_work, edp_panel_vdd_work); > > /* > @@ -7590,10 +7587,12 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port, > intel_dp_mst_encoder_init(intel_dig_port, > intel_connector->base.base.id); > > - if (!intel_edp_init_connector(intel_dp, intel_connector)) { > - intel_dp_aux_fini(intel_dp); > - intel_dp_mst_encoder_cleanup(intel_dig_port); > - goto fail; > + if (intel_dp_is_edp(intel_dp)) { > + if (!intel_edp_init_connector(intel_dp, intel_connector)) { > + intel_dp_aux_fini(intel_dp); > + intel_dp_mst_encoder_cleanup(intel_dig_port); > + goto fail; > + } > } > > intel_dp_add_properties(intel_dp, connector);
On Tue, 2020-02-11 at 15:03 +0200, Jani Nikula wrote: > On Tue, 11 Feb 2020, Stanislav Lisovskiy < > stanislav.lisovskiy@intel.com> wrote: > > I guess it would still be nice to make the code less confusing > > and do not call eDP specific function, for non-eDP connectors > > just to immediately return true(success) value as a hack. > > > > So simply extracted that check out from this function, > > that we simply don't call it for non-eDP connectors. Bingo. > > Fair enough, I guess... > > Though I could be persuaded to take a patch for the reverse, because > generally we prefer localized knowledge in the callee than in a > potentially irrelevant place in the caller. > > Consider the intel_dp_mst_encoder_init() call in the context of this > patch. We call it, and the function itself decides whether MST init > is > relevant or not. But I presume you wouldn't suggest pulling in all > the > conditions from there one level higher? > > Would your opinion on intel_edp_init_connector() be different if the > conditions were more than just the single intel_dp_is_edp()? Or if we > moved all of eDP support to a separate file? Well, to me at least intel_edp_init_connector means that we are going to init an eDP connector, which already assumes that, we already should know that this is an eDP connector :) Otherwise it should have somewhat different name, not saying that this is the best philosophy, but generally I would prefer the functions to be solely responsible for a single thing so that that init function is supposed to do only init, but not also some detection/checking or any other side effects. So that there is a clear visibility what we are doing, if it's an init then we really do only init or if we supposed to detect something first, let it be a separate thing.. Also this uses a return value in confusing way, i.e returning "Success" status for non-eDP from intel_edp_init_connector seems kind of confusing. Stan > > BR, > Jani. > > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_dp.c | 13 ++++++------- > > 1 file changed, 6 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c > > b/drivers/gpu/drm/i915/display/intel_dp.c > > index f4dede6253f8..9bd36197a43d 100644 > > --- a/drivers/gpu/drm/i915/display/intel_dp.c > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > > @@ -7370,9 +7370,6 @@ static bool intel_edp_init_connector(struct > > intel_dp *intel_dp, > > intel_wakeref_t wakeref; > > struct edid *edid; > > > > - if (!intel_dp_is_edp(intel_dp)) > > - return true; > > - > > INIT_DELAYED_WORK(&intel_dp->panel_vdd_work, > > edp_panel_vdd_work); > > > > /* > > @@ -7590,10 +7587,12 @@ intel_dp_init_connector(struct > > intel_digital_port *intel_dig_port, > > intel_dp_mst_encoder_init(intel_dig_port, > > intel_connector->base.base.id); > > > > - if (!intel_edp_init_connector(intel_dp, intel_connector)) { > > - intel_dp_aux_fini(intel_dp); > > - intel_dp_mst_encoder_cleanup(intel_dig_port); > > - goto fail; > > + if (intel_dp_is_edp(intel_dp)) { > > + if (!intel_edp_init_connector(intel_dp, > > intel_connector)) { > > + intel_dp_aux_fini(intel_dp); > > + intel_dp_mst_encoder_cleanup(intel_dig_port); > > + goto fail; > > + } > > } > > > > intel_dp_add_properties(intel_dp, connector); > >
On Tue, 11 Feb 2020, "Lisovskiy, Stanislav" <stanislav.lisovskiy@intel.com> wrote: > On Tue, 2020-02-11 at 15:03 +0200, Jani Nikula wrote: >> On Tue, 11 Feb 2020, Stanislav Lisovskiy < >> stanislav.lisovskiy@intel.com> wrote: >> > I guess it would still be nice to make the code less confusing >> > and do not call eDP specific function, for non-eDP connectors >> > just to immediately return true(success) value as a hack. >> > >> > So simply extracted that check out from this function, >> > that we simply don't call it for non-eDP connectors. Bingo. >> >> Fair enough, I guess... >> >> Though I could be persuaded to take a patch for the reverse, because >> generally we prefer localized knowledge in the callee than in a >> potentially irrelevant place in the caller. >> >> Consider the intel_dp_mst_encoder_init() call in the context of this >> patch. We call it, and the function itself decides whether MST init >> is >> relevant or not. But I presume you wouldn't suggest pulling in all >> the >> conditions from there one level higher? >> >> Would your opinion on intel_edp_init_connector() be different if the >> conditions were more than just the single intel_dp_is_edp()? Or if we >> moved all of eDP support to a separate file? > > Well, to me at least intel_edp_init_connector means that we are going > to init an eDP connector, which already assumes that, we already should > know that this is an eDP connector :) > Otherwise it should have somewhat different name, not saying that this > is the best philosophy, but generally I would prefer the functions to > be solely responsible for a single thing so that that init function is > supposed to do only init, but not also some detection/checking or any > other side effects. > > So that there is a clear visibility what we are doing, if it's an init > then we really do only init or if we supposed to detect something > first, let it be a separate thing.. > > Also this uses a return value in confusing way, i.e returning "Success" > status for non-eDP from intel_edp_init_connector seems kind of > confusing. Again, how is this different from intel_dp_mst_encoder_init()? With the *exactly* same rationale you'd end up with this: if (HAS_DP_MST(i915) && !intel_dp_is_edp(intel_dp)) && !(INTEL_GEN(i915) < 12 && port == PORT_A) && !(INTEL_GEN(i915) < 11 && port == PORT_E)) intel_dp_mst_encoder_init(intel_dig_port, intel_connector->base.base.id); Surely the information is better localized in a SPOT in the MST code? BR, Jani. > > Stan > >> >> BR, >> Jani. >> >> >> > >> > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> >> > --- >> > drivers/gpu/drm/i915/display/intel_dp.c | 13 ++++++------- >> > 1 file changed, 6 insertions(+), 7 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c >> > b/drivers/gpu/drm/i915/display/intel_dp.c >> > index f4dede6253f8..9bd36197a43d 100644 >> > --- a/drivers/gpu/drm/i915/display/intel_dp.c >> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c >> > @@ -7370,9 +7370,6 @@ static bool intel_edp_init_connector(struct >> > intel_dp *intel_dp, >> > intel_wakeref_t wakeref; >> > struct edid *edid; >> > >> > - if (!intel_dp_is_edp(intel_dp)) >> > - return true; >> > - >> > INIT_DELAYED_WORK(&intel_dp->panel_vdd_work, >> > edp_panel_vdd_work); >> > >> > /* >> > @@ -7590,10 +7587,12 @@ intel_dp_init_connector(struct >> > intel_digital_port *intel_dig_port, >> > intel_dp_mst_encoder_init(intel_dig_port, >> > intel_connector->base.base.id); >> > >> > - if (!intel_edp_init_connector(intel_dp, intel_connector)) { >> > - intel_dp_aux_fini(intel_dp); >> > - intel_dp_mst_encoder_cleanup(intel_dig_port); >> > - goto fail; >> > + if (intel_dp_is_edp(intel_dp)) { >> > + if (!intel_edp_init_connector(intel_dp, >> > intel_connector)) { >> > + intel_dp_aux_fini(intel_dp); >> > + intel_dp_mst_encoder_cleanup(intel_dig_port); >> > + goto fail; >> > + } >> > } >> > >> > intel_dp_add_properties(intel_dp, connector); >> >>
On Tue, 2020-02-11 at 15:55 +0200, Jani Nikula wrote: > On Tue, 11 Feb 2020, "Lisovskiy, Stanislav" < > stanislav.lisovskiy@intel.com> wrote: > > On Tue, 2020-02-11 at 15:03 +0200, Jani Nikula wrote: > > > On Tue, 11 Feb 2020, Stanislav Lisovskiy < > > > stanislav.lisovskiy@intel.com> wrote: > > > > I guess it would still be nice to make the code less confusing > > > > and do not call eDP specific function, for non-eDP connectors > > > > just to immediately return true(success) value as a hack. > > > > > > > > So simply extracted that check out from this function, > > > > that we simply don't call it for non-eDP connectors. Bingo. > > > > > > Fair enough, I guess... > > > > > > Though I could be persuaded to take a patch for the reverse, > > > because > > > generally we prefer localized knowledge in the callee than in a > > > potentially irrelevant place in the caller. > > > > > > Consider the intel_dp_mst_encoder_init() call in the context of > > > this > > > patch. We call it, and the function itself decides whether MST > > > init > > > is > > > relevant or not. But I presume you wouldn't suggest pulling in > > > all > > > the > > > conditions from there one level higher? > > > > > > Would your opinion on intel_edp_init_connector() be different if > > > the > > > conditions were more than just the single intel_dp_is_edp()? Or > > > if we > > > moved all of eDP support to a separate file? > > > > Well, to me at least intel_edp_init_connector means that we are > > going > > to init an eDP connector, which already assumes that, we already > > should > > know that this is an eDP connector :) > > Otherwise it should have somewhat different name, not saying that > > this > > is the best philosophy, but generally I would prefer the functions > > to > > be solely responsible for a single thing so that that init function > > is > > supposed to do only init, but not also some detection/checking or > > any > > other side effects. > > > > So that there is a clear visibility what we are doing, if it's an > > init > > then we really do only init or if we supposed to detect something > > first, let it be a separate thing.. > > > > Also this uses a return value in confusing way, i.e returning > > "Success" > > status for non-eDP from intel_edp_init_connector seems kind of > > confusing. > > Again, how is this different from intel_dp_mst_encoder_init()? > > With the *exactly* same rationale you'd end up with this: > > if (HAS_DP_MST(i915) && !intel_dp_is_edp(intel_dp)) && > !(INTEL_GEN(i915) < 12 && port == PORT_A) && > !(INTEL_GEN(i915) < 11 && port == PORT_E)) > intel_dp_mst_encoder_init(intel_dig_port, > intel_connector- > >base.base.id); > > Surely the information is better localized in a SPOT in the MST code? Well, you can just take all those checks and put them into separate function. Something like: bool intel_dp_supports_mst(intel_dp) { if (HAS_DP_MST(i915) && !intel_dp_is_edp(intel_dp)) && !(INTEL_GEN(i915) < 12 && port == PORT_A) && !(INTEL_GEN(i915) < 11 && port == PORT_E)) return true; return false; } so, then you would have it nicely looking and understandable: if (intel_dp_supports_mst(intel_dp)) intel_dp_mst_encoder_init(intel_dig_port, intel_connector->base.base.id); Anyway, I'm _not_ stating that this is _always_ the best way, but I don't see at least any reasons currently why it couldn't be done so. Stan > > > BR, > Jani. > > > > > Stan > > > > > > > > BR, > > > Jani. > > > > > > > > > > > > > > Signed-off-by: Stanislav Lisovskiy < > > > > stanislav.lisovskiy@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/display/intel_dp.c | 13 ++++++------- > > > > 1 file changed, 6 insertions(+), 7 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c > > > > b/drivers/gpu/drm/i915/display/intel_dp.c > > > > index f4dede6253f8..9bd36197a43d 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_dp.c > > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > > > > @@ -7370,9 +7370,6 @@ static bool > > > > intel_edp_init_connector(struct > > > > intel_dp *intel_dp, > > > > intel_wakeref_t wakeref; > > > > struct edid *edid; > > > > > > > > - if (!intel_dp_is_edp(intel_dp)) > > > > - return true; > > > > - > > > > INIT_DELAYED_WORK(&intel_dp->panel_vdd_work, > > > > edp_panel_vdd_work); > > > > > > > > /* > > > > @@ -7590,10 +7587,12 @@ intel_dp_init_connector(struct > > > > intel_digital_port *intel_dig_port, > > > > intel_dp_mst_encoder_init(intel_dig_port, > > > > intel_connector- > > > > >base.base.id); > > > > > > > > - if (!intel_edp_init_connector(intel_dp, > > > > intel_connector)) { > > > > - intel_dp_aux_fini(intel_dp); > > > > - intel_dp_mst_encoder_cleanup(intel_dig_port); > > > > - goto fail; > > > > + if (intel_dp_is_edp(intel_dp)) { > > > > + if (!intel_edp_init_connector(intel_dp, > > > > intel_connector)) { > > > > + intel_dp_aux_fini(intel_dp); > > > > + intel_dp_mst_encoder_cleanup(intel_dig_ > > > > port); > > > > + goto fail; > > > > + } > > > > } > > > > > > > > intel_dp_add_properties(intel_dp, connector); > > > > > > > >
On Tue, 11 Feb 2020, "Lisovskiy, Stanislav" <stanislav.lisovskiy@intel.com> wrote: > Well, you can just take all those checks and put them into separate > function. Something like: > > bool intel_dp_supports_mst(intel_dp) { > if (HAS_DP_MST(i915) && !intel_dp_is_edp(intel_dp)) && > !(INTEL_GEN(i915) < 12 && port == PORT_A) && > !(INTEL_GEN(i915) < 11 && port == PORT_E)) > return true; > return false; > } > > so, then you would have it nicely looking and understandable: > > if (intel_dp_supports_mst(intel_dp)) > intel_dp_mst_encoder_init(intel_dig_port, > intel_connector->base.base.id); > > Anyway, I'm _not_ stating that this is _always_ the best way, but > I don't see at least any reasons currently why it couldn't be done so. It's fine, until you realize you need to call a function with the condition from more than one place, and you need to remember to have the same conditions in all the places. So the condition is no longer in one isolated place. It's not like we haven't thought about this before. ;) BR, Jani.
On Tue, Feb 11, 2020 at 03:55:45PM +0200, Jani Nikula wrote: > On Tue, 11 Feb 2020, "Lisovskiy, Stanislav" <stanislav.lisovskiy@intel.com> wrote: > > On Tue, 2020-02-11 at 15:03 +0200, Jani Nikula wrote: > >> On Tue, 11 Feb 2020, Stanislav Lisovskiy < > >> stanislav.lisovskiy@intel.com> wrote: > >> > I guess it would still be nice to make the code less confusing > >> > and do not call eDP specific function, for non-eDP connectors > >> > just to immediately return true(success) value as a hack. > >> > > >> > So simply extracted that check out from this function, > >> > that we simply don't call it for non-eDP connectors. Bingo. > >> > >> Fair enough, I guess... > >> > >> Though I could be persuaded to take a patch for the reverse, because > >> generally we prefer localized knowledge in the callee than in a > >> potentially irrelevant place in the caller. > >> > >> Consider the intel_dp_mst_encoder_init() call in the context of this > >> patch. We call it, and the function itself decides whether MST init > >> is > >> relevant or not. But I presume you wouldn't suggest pulling in all > >> the > >> conditions from there one level higher? > >> > >> Would your opinion on intel_edp_init_connector() be different if the > >> conditions were more than just the single intel_dp_is_edp()? Or if we > >> moved all of eDP support to a separate file? > > > > Well, to me at least intel_edp_init_connector means that we are going > > to init an eDP connector, which already assumes that, we already should > > know that this is an eDP connector :) > > Otherwise it should have somewhat different name, not saying that this > > is the best philosophy, but generally I would prefer the functions to > > be solely responsible for a single thing so that that init function is > > supposed to do only init, but not also some detection/checking or any > > other side effects. > > > > So that there is a clear visibility what we are doing, if it's an init > > then we really do only init or if we supposed to detect something > > first, let it be a separate thing.. > > > > Also this uses a return value in confusing way, i.e returning "Success" > > status for non-eDP from intel_edp_init_connector seems kind of > > confusing. > > Again, how is this different from intel_dp_mst_encoder_init()? > > With the *exactly* same rationale you'd end up with this: > > if (HAS_DP_MST(i915) && !intel_dp_is_edp(intel_dp)) && > !(INTEL_GEN(i915) < 12 && port == PORT_A) && > !(INTEL_GEN(i915) < 11 && port == PORT_E)) > intel_dp_mst_encoder_init(intel_dig_port, > intel_connector->base.base.id); > > Surely the information is better localized in a SPOT in the MST code? I find it also clearer to bring the intel_dp_is_edp() check out from intel_edp_init_connector(): we init the connector to be either an eDP or DP connector. Atm it's not clear after intel_edp_init_connector() returns success if the connector is eDP or DP type at that point. Stan's change would improve that. > > > BR, > Jani. > > > > > Stan > > > >> > >> BR, > >> Jani. > >> > >> > >> > > >> > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > >> > --- > >> > drivers/gpu/drm/i915/display/intel_dp.c | 13 ++++++------- > >> > 1 file changed, 6 insertions(+), 7 deletions(-) > >> > > >> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c > >> > b/drivers/gpu/drm/i915/display/intel_dp.c > >> > index f4dede6253f8..9bd36197a43d 100644 > >> > --- a/drivers/gpu/drm/i915/display/intel_dp.c > >> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > >> > @@ -7370,9 +7370,6 @@ static bool intel_edp_init_connector(struct > >> > intel_dp *intel_dp, > >> > intel_wakeref_t wakeref; > >> > struct edid *edid; > >> > > >> > - if (!intel_dp_is_edp(intel_dp)) > >> > - return true; > >> > - > >> > INIT_DELAYED_WORK(&intel_dp->panel_vdd_work, > >> > edp_panel_vdd_work); > >> > > >> > /* > >> > @@ -7590,10 +7587,12 @@ intel_dp_init_connector(struct > >> > intel_digital_port *intel_dig_port, > >> > intel_dp_mst_encoder_init(intel_dig_port, > >> > intel_connector->base.base.id); > >> > > >> > - if (!intel_edp_init_connector(intel_dp, intel_connector)) { > >> > - intel_dp_aux_fini(intel_dp); > >> > - intel_dp_mst_encoder_cleanup(intel_dig_port); > >> > - goto fail; > >> > + if (intel_dp_is_edp(intel_dp)) { > >> > + if (!intel_edp_init_connector(intel_dp, > >> > intel_connector)) { > >> > + intel_dp_aux_fini(intel_dp); > >> > + intel_dp_mst_encoder_cleanup(intel_dig_port); > >> > + goto fail; > >> > + } > >> > } > >> > > >> > intel_dp_add_properties(intel_dp, connector); > >> > >> > > -- > Jani Nikula, Intel Open Source Graphics Center > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, 2020-02-11 at 16:12 +0200, Jani Nikula wrote: > On Tue, 11 Feb 2020, "Lisovskiy, Stanislav" < > stanislav.lisovskiy@intel.com> wrote: > > Well, you can just take all those checks and put them into separate > > function. Something like: > > > > bool intel_dp_supports_mst(intel_dp) { > > if (HAS_DP_MST(i915) && !intel_dp_is_edp(intel_dp)) && > > !(INTEL_GEN(i915) < 12 && port == PORT_A) && > > !(INTEL_GEN(i915) < 11 && port == PORT_E)) > > return true; > > return false; > > } > > > > so, then you would have it nicely looking and understandable: > > > > if (intel_dp_supports_mst(intel_dp)) > > intel_dp_mst_encoder_init(intel_dig_port, > > intel_connector->base.base.id); > > > > Anyway, I'm _not_ stating that this is _always_ the best way, but > > I don't see at least any reasons currently why it couldn't be done > > so. > > It's fine, until you realize you need to call a function with the > condition from more than one place, and you need to remember to have > the > same conditions in all the places. So the condition is no longer in > one > isolated place. It's not like we haven't thought about this before. > ;) Well, that is why I didn't say that this is always a best approach. :) Sure, if I had to do lots of calls of this function(even though this should be reviewed then, why we have to do it in a multiple places), I would then just put this check and init together into some helper function. Stan > > BR, > Jani. > >
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c index f4dede6253f8..9bd36197a43d 100644 --- a/drivers/gpu/drm/i915/display/intel_dp.c +++ b/drivers/gpu/drm/i915/display/intel_dp.c @@ -7370,9 +7370,6 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp, intel_wakeref_t wakeref; struct edid *edid; - if (!intel_dp_is_edp(intel_dp)) - return true; - INIT_DELAYED_WORK(&intel_dp->panel_vdd_work, edp_panel_vdd_work); /* @@ -7590,10 +7587,12 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port, intel_dp_mst_encoder_init(intel_dig_port, intel_connector->base.base.id); - if (!intel_edp_init_connector(intel_dp, intel_connector)) { - intel_dp_aux_fini(intel_dp); - intel_dp_mst_encoder_cleanup(intel_dig_port); - goto fail; + if (intel_dp_is_edp(intel_dp)) { + if (!intel_edp_init_connector(intel_dp, intel_connector)) { + intel_dp_aux_fini(intel_dp); + intel_dp_mst_encoder_cleanup(intel_dig_port); + goto fail; + } } intel_dp_add_properties(intel_dp, connector);
I guess it would still be nice to make the code less confusing and do not call eDP specific function, for non-eDP connectors just to immediately return true(success) value as a hack. So simply extracted that check out from this function, that we simply don't call it for non-eDP connectors. Bingo. Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> --- drivers/gpu/drm/i915/display/intel_dp.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)