| Message ID | 20240611094810.27475-2-piotr.wojtaszczyk@timesys.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/2] ASoC: fsl: Add i2s and pcm drivers for LPC32xx CPUs | expand |
On 11/06/2024 11:47, Piotr Wojtaszczyk wrote: > Add nxp,lpc3220-i2s DT binding documentation. > > Signed-off-by: Piotr Wojtaszczyk <piotr.wojtaszczyk@timesys.com> > --- > + > +maintainers: > + - Piotr Wojtaszczyk <piotr.wojtaszczyk@timesys.com> > + > +properties: > + compatible: > + enum: > + - nxp,lpc3220-i2s > + > + reg: > + maxItems: 1 > + > + clocks: > + items: > + - description: input clock of the peripheral. > + I do not see my comment about DAI being addressed. <form letter> This is a friendly reminder during the review process. It seems my or other reviewer's previous comments were not fully addressed. Maybe the feedback got lost between the quotes, maybe you just forgot to apply it. Please go back to the previous discussion and either implement all requested changes or keep discussing them. </<form letter> > +required: > + - compatible > + - reg > + - clocks > + - clock-names Drop > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/clock/lpc32xx-clock.h> > + > + i2s0: i2s@20094000 { Drop label, not used. > + compatible = "nxp,lpc3220-i2s"; > + reg = <0x20094000 0x1000>; > + clocks = <&clk LPC32XX_CLK_I2S0>; > + clock-names = "i2s_clk"; Not tested. Drop. > + status = "disabled"; Then what is the point of example? Drop. Your DTS was also not tested. 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). Best regards, Krzysztof
On Tue, Jun 11, 2024 at 11:47:52AM +0200, Piotr Wojtaszczyk wrote: > Changes for v2: > - Added maintainers field > - Dropped clock-names > - Dropped unused unneded interrupts field > +required: > + - compatible > + - reg > + - clocks > + - clock-names Some of the dropping of clock-names was missed.
On 11/06/2024 11:47, Piotr Wojtaszczyk wrote: > Add nxp,lpc3220-i2s DT binding documentation. > > Signed-off-by: Piotr Wojtaszczyk <piotr.wojtaszczyk@timesys.com> > --- > Changes for v2: > - Added maintainers field > - Dropped clock-names > - Dropped unused unneded interrupts field Does the device has interrupts or not? This should justify decision, not current usage by drivers. Best regards, Krzysztof
On Tue, 11 Jun 2024 11:47:52 +0200, Piotr Wojtaszczyk wrote: > Add nxp,lpc3220-i2s DT binding documentation. > > Signed-off-by: Piotr Wojtaszczyk <piotr.wojtaszczyk@timesys.com> > --- > Changes for v2: > - Added maintainers field > - Dropped clock-names > - Dropped unused unneded interrupts field > > .../bindings/sound/nxp,lpc3220-i2s.yaml | 47 +++++++++++++++++++ > 1 file changed, 47 insertions(+) > create mode 100644 Documentation/devicetree/bindings/sound/nxp,lpc3220-i2s.yaml > My bot found errors running 'make dt_binding_check' on your patch: yamllint warnings/errors: dtschema/dtc warnings/errors: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/sound/nxp,lpc3220-i2s.example.dtb: i2s@20094000: 'clock-names' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/sound/nxp,lpc3220-i2s.yaml# doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240611094810.27475-2-piotr.wojtaszczyk@timesys.com The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.
On Tue, Jun 11, 2024 at 12:18 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> I do not see my comment about DAI being addressed.
Were you asking if it's a DAI? yes it is.
On Tue, Jun 11, 2024 at 12:45 PM Krzysztof Kozlowski <krzk@kernel.org> wrote: > > Changes for v2: > > - Added maintainers field > > - Dropped clock-names > > - Dropped unused unneded interrupts field > > Does the device has interrupts or not? This should justify decision, not > current usage by drivers. Yes the device has interrupts but feeding data FIFOs is handled by DMA (amba-pl08x.c). Should I declare interrupts despite they are not used in the compatible driver?
On 12/06/2024 10:06, Piotr Wojtaszczyk wrote: > On Tue, Jun 11, 2024 at 12:45 PM Krzysztof Kozlowski <krzk@kernel.org> wrote: >>> Changes for v2: >>> - Added maintainers field >>> - Dropped clock-names >>> - Dropped unused unneded interrupts field >> >> Does the device has interrupts or not? This should justify decision, not >> current usage by drivers. > Yes the device has interrupts but feeding data FIFOs is handled by DMA > (amba-pl08x.c). > Should I declare interrupts despite they are not used in the compatible driver? Yes. Best regards, Krzysztof
On 12/06/2024 10:02, Piotr Wojtaszczyk wrote: > On Tue, Jun 11, 2024 at 12:18 PM Krzysztof Kozlowski <krzk@kernel.org> wrote: >> I do not see my comment about DAI being addressed. > Were you asking if it's a DAI? yes it is. > Then you miss $ref to dai-common and defining sound-dai-cells like in other bindings. Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/sound/nxp,lpc3220-i2s.yaml b/Documentation/devicetree/bindings/sound/nxp,lpc3220-i2s.yaml new file mode 100644 index 000000000000..66e48d8a5a1b --- /dev/null +++ b/Documentation/devicetree/bindings/sound/nxp,lpc3220-i2s.yaml @@ -0,0 +1,47 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/sound/nxp,lpc3220-i2s.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: NXP LPC32XX I2S Controller + +description: + The I2S controller in LPC32XX SoCs to interface codecs and other audo devices. + +maintainers: + - Piotr Wojtaszczyk <piotr.wojtaszczyk@timesys.com> + +properties: + compatible: + enum: + - nxp,lpc3220-i2s + + reg: + maxItems: 1 + + clocks: + items: + - description: input clock of the peripheral. + +required: + - compatible + - reg + - clocks + - clock-names + +additionalProperties: false + +examples: + - | + #include <dt-bindings/clock/lpc32xx-clock.h> + + i2s0: i2s@20094000 { + compatible = "nxp,lpc3220-i2s"; + reg = <0x20094000 0x1000>; + clocks = <&clk LPC32XX_CLK_I2S0>; + clock-names = "i2s_clk"; + status = "disabled"; + }; + +...
Add nxp,lpc3220-i2s DT binding documentation. Signed-off-by: Piotr Wojtaszczyk <piotr.wojtaszczyk@timesys.com> --- Changes for v2: - Added maintainers field - Dropped clock-names - Dropped unused unneded interrupts field .../bindings/sound/nxp,lpc3220-i2s.yaml | 47 +++++++++++++++++++ 1 file changed, 47 insertions(+) create mode 100644 Documentation/devicetree/bindings/sound/nxp,lpc3220-i2s.yaml