> -----Original Message-----
> From: Nikula, Jani <jani.nikula@xxxxxxxxx>
> Sent: Friday, August 12, 2022 12:26 PM
> To: Murthy, Arun R <arun.r.murthy@xxxxxxxxx>; intel-
> gfx@xxxxxxxxxxxxxxxxxxxxx
> Cc: De Marchi, Lucas <lucas.demarchi@xxxxxxxxx>
> Subject: RE: [PATCH 09/39] drm/i915: move and group gmbus
> members under display.gmbus
>
> On Fri, 12 Aug 2022, "Murthy, Arun R" <arun.r.murthy@xxxxxxxxx> wrote:
> >> -----Original Message-----
> >> From: Intel-gfx <intel-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf
> >> Of Jani Nikula
> >> Sent: Thursday, August 11, 2022 8:37 PM
> >> To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> >> Cc: Nikula, Jani <jani.nikula@xxxxxxxxx>; De Marchi, Lucas
> >> <lucas.demarchi@xxxxxxxxx>
> >> Subject: [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@xxxxxxxxx>
> >> ---
> >> 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@xxxxxxxxx>
Thanks and Regards,
Arun R Murthy
-------------------
