[Intel-gfx] [RFC][PATCH 43/43] WIP: drm/i915: Type safe register read/write

[Daniel Vetter]

On Mon, Sep 28, 2015 at 04:03:09PM +0300, Ville Syrjälä wrote:
> On Mon, Sep 28, 2015 at 03:56:14PM +0300, Jani Nikula wrote:
> > On Thu, 24 Sep 2015, Ville Syrjälä <ville.syrjala at linux.intel.com> wrote:
> > > On Wed, Sep 23, 2015 at 05:23:27PM +0200, Daniel Vetter wrote:
> > >> On Fri, Sep 18, 2015 at 08:03:56PM +0300, ville.syrjala at linux.intel.com wrote:
> > >> > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > >> > 
> > >> > Make I915_READ and I915_WRITE more type safe by wrapping the register
> > >> > offset in a struct. This should eliminate most of the fumbles we've had
> > >> > with misplaced parens.
> > >> > 
> > >> > This only takes care of normal mmio registers. We could extend the idea
> > >> > to other register types and define each with its own struct. That way
> > >> > you wouldn't be able to accidentally pass the wrong thing to a specific
> > >> > register access function.
> > >> > 
> > >> > There are a few uglies left:
> > >> > - switch statements don't like structs as case values, even if
> > >> >   you case 'case DEADBEEF.reg:'. Fortunately we don't have too many
> > >> >   of those so can maybe switch them to use some port enums or such,
> > >> >   or if all else fails if ladders
> > >> > - cmd parser is still iffed out. I was just tool lazy to do that
> > >> >   for an RFC
> > >> > - the vgpu stuff is ugly cause I didn't spend any time figuring out
> > >> >   what it really wants to do
> > >> > - maybe something else I'm overlooking?
> > >> > 
> > >> > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > >> 
> > >> I like this very much, which is also why I started pulling in prep patches already.
> > >> 
> > >> One bikeshed though: I think this is one of the very few exceptions where
> > >> coding style says typedefs are ok, since this really is just an opaque
> > >> datatype that gets passed around and only opened-up in low-level code
> > >> similar to ptes. So my vote is on i915_reg_t. And since we might want to
> > >> extend this to other register io functions maybe call it i915_mmio_reg_t.
> > >
> > > Chris wanted the same, and so I've made the change already. If you want to
> > > take sneak peek, it's available in the update branch I mentioned in my earlier
> > > reply to the cover letter.
> > 
> > You're too fast, making the change already... ;) IMO the shorter the
> > better for the common case, and I'd argue we can use i915_reg_t for
> > mmio, and something else for the rest.
> 
> Easy sed job if we want to go that way.

Ack on i915_reg_t from my side.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch