On Wed, May 15, 2013 at 09:40:29PM +0300, Jani Nikula wrote:
> Okay, I'm stuck with this a bit, and whichever approach I choose I
> expect there to be a bunch of bikeshedding. So I won't polish this
> further before comments.
>
> Both the intel_dpio_{read,write} and valleyview_{punit,nc}_{read,write}
> use the IOSF sideband interface. They access the same registers and do
> mostly the same stuff, but no shared code. There are even duplicate
> register defines for the same registers. Both have locking, but the
> former use dpio_lock and the latter rps.hw_lock. It's racy.
>
> These patches refactor the sideband access to a single function that
> expects dpio_lock to be held. The dpio_lock is only used for sideband
> stuff, so it's a better match than rps.hw_lock for the purpose. The rps
> stuff still needs rps.hw_lock, since it's used to protect more than just
> the register access, so rps code will need to hold both locks.
>
> The bikeshedding department:
>
> 1) Do we need to propagate sideband errors from the functions (like the
> valleyview_punit_* functions do), or, since the return values are
> never checked anywhere anyway, can we just convert to the
> intel_dpio_* style (functions return the register value only) for all
> of them?
I vote for void return and obnoxious WARNs (WARNs are optional if you
want). There's usually not much we can do if the hw has gone south like
that.
> 2) There will be quite a few more ports. Add new wrappers for each of
> them, or create generic read/write functions that need a port
> argument?
Imo the wrappers aren't too bad right now still, i.e. we can re-bikeshed
this once it happens.
> 3) Should the rps stuff take dpio_lock at a higher level than the
> wrappers? This is pretty much a requirement for the generic r/w
> function.
I'm more unhappy about the inconsistency in locking, since the dpio lock
is now held around large swaths of crtc disable/enable code, but only very
small parts for the rps stuff. I expect this to eventually bite us if dpio
sideband usage keeps growing. But again something we can fix once it blows
up.
> 4) Does dpio really need a devfn different from the rest?
I have no idea about this. Definitely looks ... interesting though ;-) I
think Jesse should take a look here.
> 5) Your additional bikeshedding here. ;)
Dropped one to also move the sbi stuff around.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
