| Message ID | 20240322003713.6918-4-l.rubusch@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: adxl345: add spi-3wire | expand |
On Fri, Mar 22, 2024 at 12:37:13AM +0000, Lothar Rubusch wrote: > Provide the optional spi-3wire in the example. That doesn't match the diff as you don't touch the example. But really, this should say why you need spi-3wire. > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com> > --- > Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml b/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml > index 07cacc3f6..280ed479e 100644 > --- a/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml > +++ b/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml > @@ -32,6 +32,8 @@ properties: > > spi-cpol: true > > + spi-3wire: true > + > interrupts: > maxItems: 1 > > -- > 2.25.1 >
On Fri, Mar 22, 2024 at 3:17 AM Rob Herring <robh@kernel.org> wrote: > > On Fri, Mar 22, 2024 at 12:37:13AM +0000, Lothar Rubusch wrote: > > Provide the optional spi-3wire in the example. > > That doesn't match the diff as you don't touch the example. But really, > this should say why you need spi-3wire. I understand. The change does not add anything to the example. which is definitely wrong. Anyway I'm unsure about this change in particular. I know the spi-3wire binding exists and can be implemented. Not all spi devices offer it. Not all drivers implement it. My patch set tries to implement spi-3wire for the particular accelerometer. Do I need to add something here to dt-bindings documentation of the adxl345? Or, as an optional spi feature, is it covered anyway by documentation of optional spi bindings? So, should I refrase this particular patch or may I drop it entirely? Could you please clarify. > > > > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com> > > --- > > Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml b/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml > > index 07cacc3f6..280ed479e 100644 > > --- a/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml > > +++ b/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml > > @@ -32,6 +32,8 @@ properties: > > > > spi-cpol: true > > > > + spi-3wire: true > > + > > interrupts: > > maxItems: 1 > > > > -- > > 2.25.1 > >
On 23/03/2024 13:04, Lothar Rubusch wrote: > On Fri, Mar 22, 2024 at 3:17 AM Rob Herring <robh@kernel.org> wrote: >> >> On Fri, Mar 22, 2024 at 12:37:13AM +0000, Lothar Rubusch wrote: >>> Provide the optional spi-3wire in the example. >> >> That doesn't match the diff as you don't touch the example. But really, >> this should say why you need spi-3wire. > > I understand. The change does not add anything to the example. which > is definitely wrong. > Anyway I'm unsure about this change in particular. I know the spi-3wire > binding exists and can be implemented. Not all spi devices offer it. Not all > drivers implement it. My patch set tries to implement spi-3wire for the > particular accelerometer. > Do I need to add something here to dt-bindings documentation of the > adxl345? Or, as an optional spi feature, is it covered anyway by > documentation of optional spi bindings? So, should I refrase this particular > patch or may I drop it entirely? Could you please clarify. Whether you need to change bindings or not, dtbs_check will tell you. Just run dtbs_check on your DTS. It does not look like you tested the DTS against bindings. Please run `make dtbs_check W=1` (see Documentation/devicetree/bindings/writing-schema.rst or https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/ for instructions). AFAIR, spi-3wire requires being explicitly mentioned in the device bindings. Best regards, Krzysztof
On Sat, Mar 23, 2024 at 3:27 PM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 23/03/2024 13:04, Lothar Rubusch wrote: > > On Fri, Mar 22, 2024 at 3:17 AM Rob Herring <robh@kernel.org> wrote: > >> > >> On Fri, Mar 22, 2024 at 12:37:13AM +0000, Lothar Rubusch wrote: > >>> Provide the optional spi-3wire in the example. > >> > >> That doesn't match the diff as you don't touch the example. But really, > >> this should say why you need spi-3wire. > > > > I understand. The change does not add anything to the example. which > > is definitely wrong. > > Anyway I'm unsure about this change in particular. I know the spi-3wire > > binding exists and can be implemented. Not all spi devices offer it. Not all > > drivers implement it. My patch set tries to implement spi-3wire for the > > particular accelerometer. > > Do I need to add something here to dt-bindings documentation of the > > adxl345? Or, as an optional spi feature, is it covered anyway by > > documentation of optional spi bindings? So, should I refrase this particular > > patch or may I drop it entirely? Could you please clarify. > > Whether you need to change bindings or not, dtbs_check will tell you. > Just run dtbs_check on your DTS. > I'm not changing upstream DTS. At most, the documentation should mention something. > It does not look like you tested the DTS against bindings. Please run > `make dtbs_check W=1` (see > Documentation/devicetree/bindings/writing-schema.rst or > https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/ > for instructions). > No, I didn't. dtbs_check did not work right out of the box, but it sounds great and I will figure out. Currently my setup is a bit customized. I compile the modules out of tree, dockerized with several DTBOs. I use an automized setup to verify spi, spi-3wire and i2c probing still works on the hardware. It is tested at least somehow. > AFAIR, spi-3wire requires being explicitly mentioned in the device bindings. > > > Best regards, > Krzysztof >
On 23/03/2024 18:44, Lothar Rubusch wrote: > On Sat, Mar 23, 2024 at 3:27 PM Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: >> >> On 23/03/2024 13:04, Lothar Rubusch wrote: >>> On Fri, Mar 22, 2024 at 3:17 AM Rob Herring <robh@kernel.org> wrote: >>>> >>>> On Fri, Mar 22, 2024 at 12:37:13AM +0000, Lothar Rubusch wrote: >>>>> Provide the optional spi-3wire in the example. >>>> >>>> That doesn't match the diff as you don't touch the example. But really, >>>> this should say why you need spi-3wire. >>> >>> I understand. The change does not add anything to the example. which >>> is definitely wrong. >>> Anyway I'm unsure about this change in particular. I know the spi-3wire >>> binding exists and can be implemented. Not all spi devices offer it. Not all >>> drivers implement it. My patch set tries to implement spi-3wire for the >>> particular accelerometer. >>> Do I need to add something here to dt-bindings documentation of the >>> adxl345? Or, as an optional spi feature, is it covered anyway by >>> documentation of optional spi bindings? So, should I refrase this particular >>> patch or may I drop it entirely? Could you please clarify. >> >> Whether you need to change bindings or not, dtbs_check will tell you. >> Just run dtbs_check on your DTS. >> > > I'm not changing upstream DTS. At most, the documentation should > mention something. Nothing should stop you testing from downstream DTS... Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml b/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml index 07cacc3f6..280ed479e 100644 --- a/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml +++ b/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml @@ -32,6 +32,8 @@ properties: spi-cpol: true + spi-3wire: true + interrupts: maxItems: 1
Provide the optional spi-3wire in the example. Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com> --- Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml | 2 ++ 1 file changed, 2 insertions(+)