Re: [PATCH v2 10/12] spi: add driver for J-Core SPI controller

[Mark Brown]

On Mon, May 23, 2016 at 04:29:38PM -0400, Rich Felker wrote:
> On Mon, May 23, 2016 at 04:30:37PM +0100, Mark Brown wrote:

> > One question here is why this is even part of a series - it's adding a
> > new controller driver which wouldn't normally have any sort of direct
> > build or other dependency on anything else or have other things
> > depending on it.  Unless there are dependencies it is generally best to
> > send separate changes separately as far as possible so that there is no
> > need to worry about issues with one part of the series slowing down
> > other parts of the series.

> I grouped the patches as a series because they make up support for a
> complete SoC. While some of the peripheral drivers may well be useful
> for non-J2 systems in the future (the cores are all open source, BSD
> licensed), there's no short-term benefit to having these drivers
> without the main J2 support.

That's what -next is for, merging everyone's trees together to give
something to test.  Practically speaking most maintainence is done at
the build level, it's how we do all the other SoCs so that people can
see what's going on at the subsystem level.

> > No, the reader has to be able to tell what the code is doing.  If this
> > made sense the thing to do would be to write out the logic operations
> > explicitly to make it clear that every step is deliberate.  However in
> > this case it sounds like the code is just plain buggy, though - the
> > driver is being asked to set a specific chip select to a specific value
> > but instead of just doing that it is also trying to also change some
> > other settings.

> It may be redundant, if the general SPI framework handles mutual
> exclusion of chipselects, but it's not buggy. Only a single chipselect
> can be active (low) at once; with multiple chips selected very bad
> things will happen. I don't see any documentation of the high-level
> SPI framework in Linux and what (if anything) it does to ensure that
> all other chipselects are disabled before enabling one, which I why I
> wrote the code so that the other chipselects are explicitly disabled.

There is no such guarantee because there are applications where it
makes sense to write to multiple chips simultaneously - this is one
common way of doing simultaneous updates over multiple audio audio
CODECs to ensure synchronization for example.  It's also just not
something that it makes any sense to worry about at the indivudal driver
level.

The generic code is responsible for ensuring that things work well,
writing bodges that silently try to work around the generic code is
always a recipie for fragility, especially if that code is hard to
understand.  Either the driver was making unwarranted assumptions that
break use cases it didn't think of or we end up having to find and fix
issues multiple times due to duplication.

Attachment: signature.asc
Description: PGP signature