[1/4] drm/i915: Adding intel_panel_scale_none() helper function
diff mbox

Message ID 1439192716-26237-1-git-send-email-xiong.y.zhang@intel.com
State New
Headers show

Commit Message

Xiong Zhang Aug. 10, 2015, 7:45 a.m. UTC
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(+)

Comments

Rodrigo Vivi Aug. 10, 2015, 6:23 p.m. UTC | #1
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
>
Daniel Vetter Aug. 12, 2015, 1 p.m. UTC | #2
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
Xiong Zhang Aug. 14, 2015, 5:18 a.m. UTC | #3
> 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
Timo Aaltonen Oct. 14, 2016, 1:03 p.m. UTC | #4
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.

Patch
diff mbox

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