| Message ID | 1439192716-26237-1-git-send-email-xiong.y.zhang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
I believe this function could be added along with the next patch that is the first to use it... Or it would be good to have a good commit message explaining why this function is needed and what is be used for... more bikeshedings inline: On Mon, Aug 10, 2015 at 12:39 AM Xiong Zhang <xiong.y.zhang@intel.com> wrote: > Signed-off-by: Xiong Zhang <xiong.y.zhang@intel.com> > --- > drivers/gpu/drm/i915/intel_drv.h | 1 + > drivers/gpu/drm/i915/intel_panel.c | 10 ++++++++++ > 2 files changed, 11 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > b/drivers/gpu/drm/i915/intel_drv.h > index 47cef0e..f57a0b4 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1287,6 +1287,7 @@ int intel_panel_init(struct intel_panel *panel, > void intel_panel_fini(struct intel_panel *panel); > void intel_fixed_panel_mode(const struct drm_display_mode *fixed_mode, > struct drm_display_mode *adjusted_mode); > +bool intel_panel_scale_none(struct intel_panel *panel); > void intel_pch_panel_fitting(struct intel_crtc *crtc, > struct intel_crtc_state *pipe_config, > int fitting_mode); > diff --git a/drivers/gpu/drm/i915/intel_panel.c > b/drivers/gpu/drm/i915/intel_panel.c > index e2ab3f6..4a573ac 100644 > --- a/drivers/gpu/drm/i915/intel_panel.c > +++ b/drivers/gpu/drm/i915/intel_panel.c > @@ -46,6 +46,16 @@ intel_fixed_panel_mode(const struct drm_display_mode > *fixed_mode, > drm_mode_set_crtcinfo(adjusted_mode, 0); > } > > +bool > +intel_panel_scale_none(struct intel_panel *panel) > double negations always confuses me, when reading next patches it took few seconds to realize on next patch that !scale_none was == fixed_mode... but meh, I never have good suggestions to avoid double negations... so up to you... > +{ > + if (panel->fitting_mode == DRM_MODE_SCALE_NONE || > + panel->fixed_mode == NULL) > + return true; > + else > + return false; > this could be just return (panel->fitting_mode == DRM_MODE_SCALE_NONE || panel->fixed_mode == NULL) or !<statement> if you remove the double negation... > +} > + > /** > * intel_find_panel_downclock - find the reduced downclock for LVDS in > EDID > * @dev: drm device > -- > 1.8.2.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx >
On Mon, Aug 10, 2015 at 06:23:19PM +0000, Rodrigo Vivi wrote: > I believe this function could be added along with the next patch that is > the first to use it... > Or it would be good to have a good commit message explaining why this > function is needed and what is be used for... Yes, please don't split up patches so much that you end up adding dead code or dead structures - always try to have at least a minimal user for something. If you want to split things up really tiny then go the other way round: First add the callers, then add the implementation. That way reviewers can understand the big scope first and then dig into details. But this one here really is small enough to just be squashed in. -Daniel > > more bikeshedings inline: > > On Mon, Aug 10, 2015 at 12:39 AM Xiong Zhang <xiong.y.zhang@intel.com> > wrote: > > > Signed-off-by: Xiong Zhang <xiong.y.zhang@intel.com> > > --- > > drivers/gpu/drm/i915/intel_drv.h | 1 + > > drivers/gpu/drm/i915/intel_panel.c | 10 ++++++++++ > > 2 files changed, 11 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > > b/drivers/gpu/drm/i915/intel_drv.h > > index 47cef0e..f57a0b4 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -1287,6 +1287,7 @@ int intel_panel_init(struct intel_panel *panel, > > void intel_panel_fini(struct intel_panel *panel); > > void intel_fixed_panel_mode(const struct drm_display_mode *fixed_mode, > > struct drm_display_mode *adjusted_mode); > > +bool intel_panel_scale_none(struct intel_panel *panel); > > void intel_pch_panel_fitting(struct intel_crtc *crtc, > > struct intel_crtc_state *pipe_config, > > int fitting_mode); > > diff --git a/drivers/gpu/drm/i915/intel_panel.c > > b/drivers/gpu/drm/i915/intel_panel.c > > index e2ab3f6..4a573ac 100644 > > --- a/drivers/gpu/drm/i915/intel_panel.c > > +++ b/drivers/gpu/drm/i915/intel_panel.c > > @@ -46,6 +46,16 @@ intel_fixed_panel_mode(const struct drm_display_mode > > *fixed_mode, > > drm_mode_set_crtcinfo(adjusted_mode, 0); > > } > > > > +bool > > +intel_panel_scale_none(struct intel_panel *panel) > > > > double negations always confuses me, when reading next patches it took few > seconds to realize on next patch that !scale_none was == fixed_mode... > but meh, I never have good suggestions to avoid double negations... so up > to you... > > > > +{ > > + if (panel->fitting_mode == DRM_MODE_SCALE_NONE || > > + panel->fixed_mode == NULL) > > + return true; > > + else > > + return false; > > > > this could be just return (panel->fitting_mode == DRM_MODE_SCALE_NONE || > panel->fixed_mode == NULL) > or !<statement> if you remove the double negation... > > > > +} > > + > > /** > > * intel_find_panel_downclock - find the reduced downclock for LVDS in > > EDID > > * @dev: drm device > > -- > > 1.8.2.1 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> On Mon, Aug 10, 2015 at 06:23:19PM +0000, Rodrigo Vivi wrote: > > I believe this function could be added along with the next patch that > > is the first to use it... > > Or it would be good to have a good commit message explaining why this > > function is needed and what is be used for... > > Yes, please don't split up patches so much that you end up adding dead code > or dead structures - always try to have at least a minimal user for something. > > If you want to split things up really tiny then go the other way round: > First add the callers, then add the implementation. That way reviewers can > understand the big scope first and then dig into details. But this one here really > is small enough to just be squashed in. > -Daniel > [Zhang, Xiong Y] Thanks for teaching me how to handle this. I'll follow it in the next version. thanks
On 14.08.2015 08:18, Zhang, Xiong Y wrote: >> On Mon, Aug 10, 2015 at 06:23:19PM +0000, Rodrigo Vivi wrote: >>> I believe this function could be added along with the next patch that >>> is the first to use it... >>> Or it would be good to have a good commit message explaining why this >>> function is needed and what is be used for... >> >> Yes, please don't split up patches so much that you end up adding dead code >> or dead structures - always try to have at least a minimal user for something. >> >> If you want to split things up really tiny then go the other way round: >> First add the callers, then add the implementation. That way reviewers can >> understand the big scope first and then dig into details. But this one here really >> is small enough to just be squashed in. >> -Daniel >> > [Zhang, Xiong Y] Thanks for teaching me how to handle this. I'll follow it in the next version. > thanks Was there a second version that got submitted? I can't find it if so.
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 47cef0e..f57a0b4 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1287,6 +1287,7 @@ int intel_panel_init(struct intel_panel *panel, void intel_panel_fini(struct intel_panel *panel); void intel_fixed_panel_mode(const struct drm_display_mode *fixed_mode, struct drm_display_mode *adjusted_mode); +bool intel_panel_scale_none(struct intel_panel *panel); void intel_pch_panel_fitting(struct intel_crtc *crtc, struct intel_crtc_state *pipe_config, int fitting_mode); diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c index e2ab3f6..4a573ac 100644 --- a/drivers/gpu/drm/i915/intel_panel.c +++ b/drivers/gpu/drm/i915/intel_panel.c @@ -46,6 +46,16 @@ intel_fixed_panel_mode(const struct drm_display_mode *fixed_mode, drm_mode_set_crtcinfo(adjusted_mode, 0); } +bool +intel_panel_scale_none(struct intel_panel *panel) +{ + if (panel->fitting_mode == DRM_MODE_SCALE_NONE || + panel->fixed_mode == NULL) + return true; + else + return false; +} + /** * intel_find_panel_downclock - find the reduced downclock for LVDS in EDID * @dev: drm device
Signed-off-by: Xiong Zhang <xiong.y.zhang@intel.com> --- drivers/gpu/drm/i915/intel_drv.h | 1 + drivers/gpu/drm/i915/intel_panel.c | 10 ++++++++++ 2 files changed, 11 insertions(+)