search

davidker / unisys

Master repository for new changes to drivers/staging/unisys and drivers/visorbus
Other
2 stars 1 forks source link

visorbus: Remove visorbus registration with visorchipset #69

Closed ghost closed 8 years ago

ghost commented 8 years ago

Visorchipset and visorbus are the same driver now so no need to wait around for them to be registered not registered. Need to clean it up and simplify it.

ghost commented 8 years ago

Branch: remove_vb_registration_with_vc; commits: c2e69bf4afaee91d2e81deb6e565991654239074, 1d2195af296a750fac8d51cec0efc78fb420ce71, 069aec00587249b3c0e8aa996171f69c1837d59b checkpatch-cleared: yes T710-1 verified: yes

Note:

branch: remove_visorbus_registration_w_visorchipset commit: 78364ddd88373f8d614ba707ec1959beb97e4471 is the predecessor to this branch and its associated commits.

selltc commented 8 years ago

Thomas Gleixner tglx@linutronix.de pointed out during his comments on 6/1 about the need to "convert notifier_lock to mutex", but my understanding about what I just heard from @TurningWrenches and @davidker is that this set of patches will be removing this lock.

selltc commented 8 years ago

I can't convey to you how much I LOVE this patchset! (grokking this code will be so much more pleasant now)

The only thing I would change is these sentences in the patch comments for the first 2 patches in the set:

Previously, visorbus and visorchipset were combined into one driver. As a
result, there is no need to register visorbus with visorchipset any longer.

I would clarify it like this:

When this functionality was first implemented, visorchipset and visorbus were
separate drivers, which necessitated a registration mechanism for them to
communicate.  More-recently, visorchipset and visorbus were combined into a
single driver, and now exist as separate source files within the same driver,
known as 'visorbus'.  This eliminated the need for a registration mechanism,
but it has remained nevertheless until now.  For the sake of simplification,
this registration mechanism is now being removed.
ghost commented 8 years ago

Thanks, Tim! :)

I like the clarification a lot. I will put it in, and then re-push.

ghost commented 8 years ago

Ok, I updated the commit comments, and refreshed the SHA1s above. Thanks!

davidker commented 8 years ago

Okay, I might be missing something, but if you do the notifiers first and get rid of the registration function in that patch aren't you messing up the registration of the responders as well? Aren't we going to be calling NULL devices or not responding and leaving the host in a Starting state? I'm running tests, but it would seem to NOT be a good idea to do it in this order. I'm tried to reverse the patches mechanically but that didn't work, I might try it a little more surgical next time.

selltc commented 8 years ago

VERY good point, David. Sorry I missed that. Could just squashing the patches be the answer?

davidker commented 8 years ago

Ideally I think they should be reversed but squashing the two patches might be the best for the short term and be good enough.

I'll squash/revise comments and send up.

selltc commented 8 years ago

8/15: Greg applied these patches to staging-next with: