| Message ID | 1470897673-29292-2-git-send-email-dhinakaran.pandiyan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Aug 10, 2016 at 11:41:12PM -0700, Dhinakaran Pandiyan wrote: > No functional change, just clean up. Debug messages now print out clock > units. Additionally, the configuration bits, which are 1:1 mapped to the > clock frequency and don't convey much information are not printed out. > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris
On Thu, 11 Aug 2016, Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> wrote: > No functional change, just clean up. Debug messages now print out clock > units. Additionally, the configuration bits, which are 1:1 mapped to the > clock frequency and don't convey much information are not printed out. IMO the cleanups here are subjective, i.e. another day someone might come in and rewrite it back to having just one code path for return instead of many. And someone might prefer early returns for errors. If you really wanted to clean this up, you could abstract the config lookup based on clock, and it would be a less subjective improvement. > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > --- > drivers/gpu/drm/i915/intel_audio.c | 20 ++++++++------------ > 1 file changed, 8 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c > index 3efce0e..9465f54 100644 > --- a/drivers/gpu/drm/i915/intel_audio.c > +++ b/drivers/gpu/drm/i915/intel_audio.c > @@ -104,21 +104,17 @@ static u32 audio_config_hdmi_pixel_clock(const struct drm_display_mode *adjusted > int i; > > for (i = 0; i < ARRAY_SIZE(hdmi_audio_clock); i++) { > - if (adjusted_mode->crtc_clock == hdmi_audio_clock[i].clock) > - break; > - } > - > - if (i == ARRAY_SIZE(hdmi_audio_clock)) { > - DRM_DEBUG_KMS("HDMI audio pixel clock setting for %d not found, falling back to defaults\n", > - adjusted_mode->crtc_clock); > - i = 1; > + if (adjusted_mode->crtc_clock == hdmi_audio_clock[i].clock) { > + DRM_DEBUG_KMS("Configuring audio for HDMI pixel clk %dkHz\n", > + hdmi_audio_clock[i].clock); > + return hdmi_audio_clock[i].config; > + } > } > > - DRM_DEBUG_KMS("Configuring HDMI audio for pixel clock %d (0x%08x)\n", > - hdmi_audio_clock[i].clock, > - hdmi_audio_clock[i].config); > + DRM_DEBUG_KMS("No config. for HDMI pixel clk %dkHz, using default %dkHz\n", Please at least fix the typos. > + adjusted_mode->crtc_clock, hdmi_audio_clock[1].clock); > > - return hdmi_audio_clock[i].config; > + return hdmi_audio_clock[1].config; > } > > static int audio_config_get_n(const struct drm_display_mode *mode, int rate)
On Thu, 2016-08-11 at 10:52 +0300, Jani Nikula wrote: > On Thu, 11 Aug 2016, Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> wrote: > > No functional change, just clean up. Debug messages now print out clock > > units. Additionally, the configuration bits, which are 1:1 mapped to the > > clock frequency and don't convey much information are not printed out. > > IMO the cleanups here are subjective, i.e. another day someone might > come in and rewrite it back to having just one code path for return > instead of many. And someone might prefer early returns for errors. Fair enough. The change in return style is subjective, I will drop this patch. Thanks for pointing it out. > If you really wanted to clean this up, you could abstract the config > lookup based on clock, and it would be a less subjective improvement. > I guess, this would involve an additional data structure. I will leave it as it is. > > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > --- > > drivers/gpu/drm/i915/intel_audio.c | 20 ++++++++------------ > > 1 file changed, 8 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c > > index 3efce0e..9465f54 100644 > > --- a/drivers/gpu/drm/i915/intel_audio.c > > +++ b/drivers/gpu/drm/i915/intel_audio.c > > @@ -104,21 +104,17 @@ static u32 audio_config_hdmi_pixel_clock(const struct drm_display_mode *adjusted > > int i; > > > > for (i = 0; i < ARRAY_SIZE(hdmi_audio_clock); i++) { > > - if (adjusted_mode->crtc_clock == hdmi_audio_clock[i].clock) > > - break; > > - } > > - > > - if (i == ARRAY_SIZE(hdmi_audio_clock)) { > > - DRM_DEBUG_KMS("HDMI audio pixel clock setting for %d not found, falling back to defaults\n", > > - adjusted_mode->crtc_clock); > > - i = 1; > > + if (adjusted_mode->crtc_clock == hdmi_audio_clock[i].clock) { > > + DRM_DEBUG_KMS("Configuring audio for HDMI pixel clk %dkHz\n", > > + hdmi_audio_clock[i].clock); > > + return hdmi_audio_clock[i].config; > > + } > > } > > > > - DRM_DEBUG_KMS("Configuring HDMI audio for pixel clock %d (0x%08x)\n", > > - hdmi_audio_clock[i].clock, > > - hdmi_audio_clock[i].config); > > + DRM_DEBUG_KMS("No config. for HDMI pixel clk %dkHz, using default %dkHz\n", > > Please at least fix the typos. > Probably the abbreviation was not obvious. I guess, the clock frequency units are standard in i915 too. Thanks for you time. > > + adjusted_mode->crtc_clock, hdmi_audio_clock[1].clock); > > > > - return hdmi_audio_clock[i].config; > > + return hdmi_audio_clock[1].config; > > } > > > > static int audio_config_get_n(const struct drm_display_mode *mode, int rate) >
diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c index 3efce0e..9465f54 100644 --- a/drivers/gpu/drm/i915/intel_audio.c +++ b/drivers/gpu/drm/i915/intel_audio.c @@ -104,21 +104,17 @@ static u32 audio_config_hdmi_pixel_clock(const struct drm_display_mode *adjusted int i; for (i = 0; i < ARRAY_SIZE(hdmi_audio_clock); i++) { - if (adjusted_mode->crtc_clock == hdmi_audio_clock[i].clock) - break; - } - - if (i == ARRAY_SIZE(hdmi_audio_clock)) { - DRM_DEBUG_KMS("HDMI audio pixel clock setting for %d not found, falling back to defaults\n", - adjusted_mode->crtc_clock); - i = 1; + if (adjusted_mode->crtc_clock == hdmi_audio_clock[i].clock) { + DRM_DEBUG_KMS("Configuring audio for HDMI pixel clk %dkHz\n", + hdmi_audio_clock[i].clock); + return hdmi_audio_clock[i].config; + } } - DRM_DEBUG_KMS("Configuring HDMI audio for pixel clock %d (0x%08x)\n", - hdmi_audio_clock[i].clock, - hdmi_audio_clock[i].config); + DRM_DEBUG_KMS("No config. for HDMI pixel clk %dkHz, using default %dkHz\n", + adjusted_mode->crtc_clock, hdmi_audio_clock[1].clock); - return hdmi_audio_clock[i].config; + return hdmi_audio_clock[1].config; } static int audio_config_get_n(const struct drm_display_mode *mode, int rate)
No functional change, just clean up. Debug messages now print out clock units. Additionally, the configuration bits, which are 1:1 mapped to the clock frequency and don't convey much information are not printed out. Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> --- drivers/gpu/drm/i915/intel_audio.c | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-)