| Message ID | 20231215-ad7380-mainline-v3-1-7a11ebf642b9@baylibre.com |
|---|---|
| State | New |
| Headers | show |
| Series | iio: adc: add new ad7380 driver | expand |
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>
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. >
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.
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.
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
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.
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 --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.
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(+)