diff mbox series

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

Message ID 20231215-ad7380-mainline-v3-1-7a11ebf642b9@baylibre.com
State New
Headers show
Series iio: adc: add new ad7380 driver | expand

Commit Message

David Lechner Dec. 15, 2023, 10:32 a.m. UTC
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.

Signed-off-by: David Lechner <dlechner@baylibre.com>
---

The rest of this series is ready to merge, so just looking for an ack from
Mark on this one.

 .../devicetree/bindings/spi/spi-peripheral-props.yaml        | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Comments

Rob Herring (Arm) Dec. 15, 2023, 7:58 p.m. UTC | #1
On Fri, 15 Dec 2023 04:32:02 -0600, David Lechner 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.
> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>
> ---
> 
> The rest of this series is ready to merge, so just looking for an ack from
> Mark on this one.
> 
>  .../devicetree/bindings/spi/spi-peripheral-props.yaml        | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>
Jonathan Cameron Jan. 7, 2024, 4:43 p.m. UTC | #2
On Fri, 15 Dec 2023 04:32:02 -0600
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.
> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>
> ---
> 
> The rest of this series is ready to merge, so just looking for an ack from
> Mark on this one.

Mark, could you take a look at this SPI binding change when you have time?

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.

Thanks,

Jonathan

> 
>  .../devicetree/bindings/spi/spi-peripheral-props.yaml        | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> index 15938f81fdce..1c8e71c18234 100644
> --- a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> +++ b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> @@ -67,6 +67,18 @@ properties:
>      enum: [0, 1, 2, 4, 8]
>      default: 1
>  
> +  spi-rx-bus-channels:
> +    description:
> +      The number of parallel channels for read transfers. The difference between
> +      this and spi-rx-bus-width is that a value N for spi-rx-bus-channels means
> +      the SPI bus is receiving one bit each of N different words at the same
> +      time whereas a value M for spi-rx-bus-width means that the bus is
> +      receiving M bits of a single word at the same time. It is also possible to
> +      use both properties at the same time, meaning the bus is receiving M bits
> +      of N different words at the same time.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    default: 1
> +
>    spi-rx-delay-us:
>      description:
>        Delay, in microseconds, after a read transfer.
>
Mark Brown Jan. 7, 2024, 9:26 p.m. UTC | #3
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.

> 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.
David Lechner Jan. 7, 2024, 11:02 p.m. UTC | #4
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.
Krzysztof Kozlowski Jan. 8, 2024, 8:40 a.m. UTC | #5
On 08/01/2024 00:02, David Lechner wrote:
> 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

There are exceptions. I documented them now:

https://lore.kernel.org/linux-devicetree/20240108083750.16350-2-krzysztof.kozlowski@linaro.org/T/#u

Best regards,
Krzysztof
Mark Brown Jan. 8, 2024, 4:39 p.m. UTC | #6
On Sun, Jan 07, 2024 at 05:02:56PM -0600, David Lechner wrote:
> On Sun, Jan 7, 2024 at 3:27 PM Mark Brown <broonie@kernel.org> wrote:

> > 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:

...

> 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.

I don't think it's sensible to try to make a binding for this without
having actually tried to put a system together that uses it and made
sure that everything is joined up properly, the thing about complete
bindings is more for things that are handle turning than for things that
are substantial new features in subsystems.
David Lechner Jan. 8, 2024, 5:15 p.m. UTC | #7
On Mon, Jan 8, 2024 at 10:39 AM Mark Brown <broonie@kernel.org> wrote:
>
> On Sun, Jan 07, 2024 at 05:02:56PM -0600, David Lechner wrote:
> > On Sun, Jan 7, 2024 at 3:27 PM Mark Brown <broonie@kernel.org> wrote:
>
> > > 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:
>
> ...
>
> > 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.
>
> I don't think it's sensible to try to make a binding for this without
> having actually tried to put a system together that uses it and made
> sure that everything is joined up properly, the thing about complete
> bindings is more for things that are handle turning than for things that
> are substantial new features in subsystems.

We do have plans to eventually implement such a feature in an
FPGA-based SPI controller, so if we need to wait until then for the
binding, then we can do that. But it would be really nice if we could
find a way forward for the IIO driver in this series without having to
wait for the resolution of new SPI controller feature for the complete
DT bindings.

DT/IIO maintainers, if I resubmit this series with the
`spi-rx-bus-channels` parts removed from the iio/adc/adi,ad7380.yaml
bindings, would that be acceptable? (Also resubmitting without this
patch of course.)
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
index 15938f81fdce..1c8e71c18234 100644
--- a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
+++ b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
@@ -67,6 +67,18 @@  properties:
     enum: [0, 1, 2, 4, 8]
     default: 1
 
+  spi-rx-bus-channels:
+    description:
+      The number of parallel channels for read transfers. The difference between
+      this and spi-rx-bus-width is that a value N for spi-rx-bus-channels means
+      the SPI bus is receiving one bit each of N different words at the same
+      time whereas a value M for spi-rx-bus-width means that the bus is
+      receiving M bits of a single word at the same time. It is also possible to
+      use both properties at the same time, meaning the bus is receiving M bits
+      of N different words at the same time.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    default: 1
+
   spi-rx-delay-us:
     description:
       Delay, in microseconds, after a read transfer.