Re: [PATCH 1/7] drm/i915: move dpll_info to header

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Mar 20, 2018 at 02:50:26PM -0700, Lucas De Marchi wrote:
> On Tue, Mar 20, 2018 at 2:56 AM, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> > On Mon, Mar 19, 2018 at 11:24:17PM -0700, Lucas De Marchi wrote:
> >> This will allow the struct to be embedded in intel_shared_dpll.
> >>
> >> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_dpll_mgr.c |  7 -------
> >>  drivers/gpu/drm/i915/intel_dpll_mgr.h | 10 ++++++++++
> >>  2 files changed, 10 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> >> index 51c5ae4e9116..52d6e731c3e9 100644
> >> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> >> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> >> @@ -1877,13 +1877,6 @@ static void intel_ddi_pll_init(struct drm_device *dev)
> >>       }
> >>  }
> >>
> >> -struct dpll_info {
> >> -     const char *name;
> >> -     const int id;
> >> -     const struct intel_shared_dpll_funcs *funcs;
> >> -     uint32_t flags;
> >> -};
> >> -
> >>  struct intel_dpll_mgr {
> >>       const struct dpll_info *dpll_info;
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.h b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> >> index f24ccf443d25..e99d6385478a 100644
> >> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.h
> >> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> >> @@ -205,6 +205,16 @@ struct intel_shared_dpll_funcs {
> >>                            struct intel_dpll_hw_state *hw_state);
> >>  };
> >>
> >> +/**
> >> + * struct dpll_info - display PLL platform specific info
> >> + */
> >> +struct dpll_info {
> >> +     const char *name;
> >> +     const int id;
> >
> > The const here seems pointless. Well, I guess if we go with the copy
> > then it might not be. Although then we get to wonder why 'flags' isn't
> > const.
> 
> Here I wanted to do just a code move. Any change would be on top. Since it seems
> the pointer approach is preferred, I'm just going ahead and dropping this.
> 
> >
> >> +     const struct intel_shared_dpll_funcs *funcs;
> >> +     uint32_t flags;
> >
> > This structure seems to be poorly organized for 64bit machines.
> 
> 
> Yes, there's a 4-bytes  right there.  Fixing this unfortunately
> involves changing
> all tables inside dpll_mgr.c.

Those aren't using named initializers?

> I'm sending the reordering as the last
> patch in v2.
> 
> thanks
> Lucas De Marchi
> 
> >
> >> +};
> >> +
> >>  /**
> >>   * struct intel_shared_dpll - display PLL with tracked state and users
> >>   */
> >> --
> >> 2.14.3
> >
> > --
> > Ville Syrjälä
> > Intel OTC
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Lucas De Marchi

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx