[Intel-gfx] [PATCH 09/39] drm/i915: move and group gmbus members under display.gmbus

[Murthy, Arun R]

> -----Original Message-----
> From: Nikula, Jani <jani.nikula at intel.com>
> Sent: Friday, August 12, 2022 12:26 PM
> To: Murthy, Arun R <arun.r.murthy at intel.com>; intel-
> gfx at lists.freedesktop.org
> Cc: De Marchi, Lucas <lucas.demarchi at intel.com>
> Subject: RE: [Intel-gfx] [PATCH 09/39] drm/i915: move and group gmbus
> members under display.gmbus
> 
> On Fri, 12 Aug 2022, "Murthy, Arun R" <arun.r.murthy at intel.com> wrote:
> >> -----Original Message-----
> >> From: Intel-gfx <intel-gfx-bounces at lists.freedesktop.org> On Behalf
> >> Of Jani Nikula
> >> Sent: Thursday, August 11, 2022 8:37 PM
> >> To: intel-gfx at lists.freedesktop.org
> >> Cc: Nikula, Jani <jani.nikula at intel.com>; De Marchi, Lucas
> >> <lucas.demarchi at intel.com>
> >> Subject: [Intel-gfx] [PATCH 09/39] drm/i915: move and group gmbus
> >> members under display.gmbus
> >>
> >> Move display related members under drm_i915_private display sub-
> struct.
> >>
> >> Signed-off-by: Jani Nikula <jani.nikula at intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/display/intel_cdclk.c    |  6 +--
> >>  .../gpu/drm/i915/display/intel_display_core.h | 23 ++++++++++
> >>  drivers/gpu/drm/i915/display/intel_dp_aux.c   |  2 +-
> >>  drivers/gpu/drm/i915/display/intel_gmbus.c    | 46 +++++++++----------
> >>  drivers/gpu/drm/i915/i915_drv.h               | 16 -------
> >>  drivers/gpu/drm/i915/i915_irq.c               |  4 +-
> >>  drivers/gpu/drm/i915/i915_reg.h               | 14 +++---
> >>  7 files changed, 59 insertions(+), 52 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c
> >> b/drivers/gpu/drm/i915/display/intel_cdclk.c
> >> index 6095f5800a2e..ea40c75c2986 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> >> @@ -2098,12 +2098,12 @@ static void intel_set_cdclk(struct
> >> drm_i915_private *dev_priv,
> >>        * functions use cdclk. Not all platforms/ports do,
> >>        * but we'll lock them all for simplicity.
> >>        */
> >> -     mutex_lock(&dev_priv->gmbus_mutex);
> >> +     mutex_lock(&dev_priv->display.gmbus.mutex);
> >>       for_each_intel_dp(&dev_priv->drm, encoder) {
> >>               struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> >>
> >>               mutex_lock_nest_lock(&intel_dp->aux.hw_mutex,
> >> -                                  &dev_priv->gmbus_mutex);
> >> +                                  &dev_priv->display.gmbus.mutex);
> >>       }
> >>
> >>       intel_cdclk_set_cdclk(dev_priv, cdclk_config, pipe); @@ -2113,7
> >> +2113,7 @@ static void intel_set_cdclk(struct drm_i915_private
> >> +*dev_priv,
> >>
> >>               mutex_unlock(&intel_dp->aux.hw_mutex);
> >>       }
> >> -     mutex_unlock(&dev_priv->gmbus_mutex);
> >> +     mutex_unlock(&dev_priv->display.gmbus.mutex);
> >>
> >>       for_each_intel_encoder_with_psr(&dev_priv->drm, encoder) {
> >>               struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> >> diff -- git a/drivers/gpu/drm/i915/display/intel_display_core.h
> >> b/drivers/gpu/drm/i915/display/intel_display_core.h
> >> index 306584c038c9..fe19d4f9a9ab 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_display_core.h
> >> +++ b/drivers/gpu/drm/i915/display/intel_display_core.h
> >> @@ -6,7 +6,11 @@
> >>  #ifndef __INTEL_DISPLAY_CORE_H__
> >>  #define __INTEL_DISPLAY_CORE_H__
> >>
> >> +#include <linux/mutex.h>
> >>  #include <linux/types.h>
> >> +#include <linux/wait.h>
> >> +
> >> +#include "intel_gmbus.h"
> >>
> >>  struct drm_i915_private;
> >>  struct intel_atomic_state;
> >> @@ -78,6 +82,25 @@ struct intel_display {
> >>               /* Display internal color functions */
> >>               const struct intel_color_funcs *color;
> >>       } funcs;
> >> +
> >> +     /* Grouping using anonymous structs. Keep sorted. */
> >> +     struct {
> >> +             /*
> >> +              * Base address of where the gmbus and gpio blocks are
> >> located
> >> +              * (either on PCH or on SoC for platforms without PCH).
> >> +              */
> >> +             u32 mmio_base;
> >> +
> >> +             /*
> >> +              * gmbus.mutex protects against concurrent usage of the
> >> single
> >> +              * hw gmbus controller on different i2c buses.
> >> +              */
> >> +             struct mutex mutex;
> >> +
> >> +             struct intel_gmbus *bus[GMBUS_NUM_PINS];
> >> +
> >> +             wait_queue_head_t wait_queue;
> >> +     } gmbus;
> >>  };
> >
> > Can this be moved to struct intel_dp?
> 
> Well, no. The data here is shared across all of them.
> 
Then maybe we might need to think on this. I somehow feel having this under display wont look good. Since it's a low level bus communication, can be tagged under bus related to interface.
I agree from your other comments that this is just a first step to cleanup i915 and there is scope to cleanup further.

Reviewed-by: Arun R Murthy <arun.r.murthy at intel.com>

Thanks and Regards,
Arun R Murthy
-------------------