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@xxxxxxxxx>
> wrote:
>
> > Signed-off-by: Xiong Zhang <xiong.y.zhang@xxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxxxxxxxx
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
