Re: [RFC][PATCH 43/43] WIP: drm/i915: Type safe register read/write

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, 24 Sep 2015, Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx wrote:
>> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>> > 
>> > 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@xxxxxxxxxxxxxxx>
>> 
>> 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.

BR,
Jani.



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

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux