On Wed, Jul 13, 2016 at 02:34:57PM -0700, Florian Fainelli wrote: > On 07/13/2016 04:10 AM, Mark Brown wrote: > > 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.
Attachment:
signature.asc
Description: PGP signature
