Re: [PATCH v3 1/3] dt-bindings: spi: add spi-rx-bus-channels peripheral property

From: David Lechner <dlechner@baylibre.com>
To: Mark Brown <broonie@kernel.org>
Cc: "Jonathan Cameron" <jic23@kernel.org>,
	linux-iio@vger.kernel.org, linux-spi@vger.kernel.org,
	devicetree@vger.kernel.org, "Rob Herring" <robh+dt@kernel.org>,
	"Krzysztof Kozlowski" <krzysztof.kozlowski+dt@linaro.org>,
	"Conor Dooley" <conor+dt@kernel.org>,
	"Michael Hennerich" <michael.hennerich@analog.com>,
	"Nuno Sá" <nuno.sa@analog.com>,
	"Liam Girdwood" <lgirdwood@gmail.com>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 1/3] dt-bindings: spi: add spi-rx-bus-channels peripheral property
Date: Sun, 7 Jan 2024 17:02:56 -0600	[thread overview]
Message-ID: <CAMknhBE7eUMzcD0bdymrhL2Lw3FubB3aHDWmJFD7YnaGNYmQ9w@mail.gmail.com> (raw)
In-Reply-To: <f431e418-0b7c-4362-be26-9d2f03e0de07@sirena.org.uk>

On Sun, Jan 7, 2024 at 3:27 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Sun, Jan 07, 2024 at 04:43:56PM +0000, Jonathan Cameron wrote:
> > David Lechner <dlechner@baylibre.com> wrote:
>
> > > This adds a new spi-rx-bus-channels property to the generic spi
> > > peripheral property bindings. This property is used to describe
> > > devices that have parallel data output channels.
>
> > > This property is different from spi-rx-bus-width in that the latter
> > > means that we are reading multiple bits of a single word at one time
> > > while the former means that we are reading single bits of multiple words
> > > at the same time.
>
> > Mark, could you take a look at this SPI binding change when you have time?
>
> Please submit patches using subject lines reflecting the style for the
> subsystem, this makes it easier for people to identify relevant patches.
> Look at what existing commits in the area you're changing are doing and
> make sure your subject lines visually resemble what they're doing.
> There's no need to resubmit to fix this alone.

Are you saying that `spi: dt-bindings:` should be preferred over
`dt-bindings: spi:`?

I thought I was doing it right since I was following the guidelines of
[1] which says:

> The preferred subject prefix for binding patches is:
>     "dt-bindings: <binding dir>: ..."

[1]: https://www.kernel.org/doc/html//v6.7/devicetree/bindings/submitting-patches.html

>
> > I don't want to apply it without your view on whether this makes sense
> > from a general SPI point of view as we all hate maintaining bindings
> > if they turn out to not be sufficiently future looking etc and we need
> > to deprecate them in favour of something else.
>
> This makes no sense to me without a corresponding change in the SPI core
> and possibly controller support, though I guess you could do data
> manging to rewrite from a normal parallel SPI to this for a pure
> software implementation.  I also see nothing in the driver that even
> attempts to parse this so I can't see how it could possibly work.

We currently don't have a controller that supports this. This is just
an attempt to make a complete binding for a peripheral according to
[2] which says:

> DO attempt to make bindings complete even if a driver doesn't support some features

[2]: https://www.kernel.org/doc/html//v6.7/devicetree/bindings/writing-bindings.html

So, will DT maintainers accept an incomplete binding for the
peripheral? Or will you reconsider this without SPI core support if I
can explain it better? It doesn't seem like a reasonable request to
expect us to spend time developing software that we don't need at this
time just to get a complete DT binding accepted for a feature that
isn't being used.

In the SPI core, I would expect this property to correspond to new
flags `SPI_RX_2_CH`, `SPI_RX_4_CH`, `SPI_RX_8_CH` and it would have
checks similar to other flags to make sure controller supports the
flag if the peripheral requires it. Likewise, struct spi_transfer
would probably need a rx_n_ch field similar to rx_nbits to specify if
individual xfers use the feature. But beyond that, yes I agree it
would be difficult to say how it should work without implementing it
on actual hardware.