Re: [PATCH, V4, 2/5] spi: bcm-qspi: Add SPI flash and MSPI driver

From: Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
To: Florian Fainelli <f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Kamal Dasu <kdasu.kdev-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>,
	Kamal Dasu <kamal.dasu-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>,
	linux-spi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	Jayachandran C <jchandra-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>,
	bcm-kernel-feedback-list
	<bcm-kernel-feedback-list-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>,
	Vikram Prakash
	<vikram.prakash-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>,
	Andy Fung <andy.fung-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>,
	Jon Mason <jon.mason-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>,
	Yendapally Reddy Dhananjaya Reddy
	<yendapally.reddy-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
Subject: Re: [PATCH, V4, 2/5] spi: bcm-qspi: Add SPI flash and MSPI driver
Date: Wed, 13 Jul 2016 23:36:38 +0100	[thread overview]
Message-ID: <20160713223638.GI9976@sirena.org.uk> (raw)
In-Reply-To: <5786B401.2050306-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

[-- Attachment #1: Type: text/plain, Size: 1995 bytes --]

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]