On Wed, Jul 13, 2016 at 02:34:57PM -0700, Florian Fainelli wrote:

On 07/13/2016 04:10 AM, Mark Brown wrote:

quoted

It's not really about reuse, it's about maintainability.  All the code
for handling this within the driver is quite unusual and makes the
driver harder to understand and review.  That's going to have an impact
now and make things harder to follow in future too.  Fitting in with the
frameworks means that we get the benefit of the structure and support
code that the frameworks provide while minimizing the amount of unusal
code in the driver.

This is not unusual at all, there are tons of peripherals that embed a
L2/L3 interrupt controller, yet keep the code localized because the
interrupt sources are not visible outside of the specific block and it
would not make sense to demultiplex these different interrupt sources as
individually requestable interrupt lines when the consumer is also local
and self contained.

If it was unconditionally in the perhipheral it'd be fairly normal and
standard but it's not, it's a completely separate and optional register
block with a bunch of separate code in a driver which already has above
average complexity even for the bits that belong in it.

This is exactly what is going on here, and it this makes the driver more
self contained without external dependencies to an irqchip driver to an
interrupt controller provider.

Like I say it just isn't, it's an obviously separate interrupt
controller.

The reusability argument is kind of the only one that makes sense here,
maintaining two drivers instead of one seems like more maintenance
burden imho.

It's definitely adding to the review complexity as things stand mainly
due to the obvious disconnect from the actual IP.  It should take very
little time to refactor and it's really hard to see it ever taking much
time to maintain in future, though it might be helpful if one of your
hardware engineers ever tries to reuse that IP.