| Message ID | 20221109165331.29332-12-rf@opensource.cirrus.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for the Cirrus Logic CS48L32 audio codecs | expand |
On Wed, 09 Nov 2022 16:53:30 +0000, Richard Fitzgerald wrote: > Codecs in this family have multiple digital and analog audio I/O that > support a variety of external hardware connections and configurations. > > Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com> > --- > .../bindings/sound/cirrus,cs48l32.yaml | 96 +++++++++++++++++++ > include/dt-bindings/sound/cs48l32.h | 25 +++++ > 2 files changed, 121 insertions(+) > create mode 100644 Documentation/devicetree/bindings/sound/cirrus,cs48l32.yaml > create mode 100644 include/dt-bindings/sound/cs48l32.h > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: ./Documentation/devicetree/bindings/sound/cirrus,cs48l32.yaml:44:20: [error] syntax error: mapping values are not allowed here (syntax) dtschema/dtc warnings/errors: make[1]: *** Deleting file 'Documentation/devicetree/bindings/sound/cirrus,cs48l32.example.dts' Documentation/devicetree/bindings/sound/cirrus,cs48l32.yaml:44:20: mapping values are not allowed here make[1]: *** [Documentation/devicetree/bindings/Makefile:26: Documentation/devicetree/bindings/sound/cirrus,cs48l32.example.dts] Error 1 make[1]: *** Waiting for unfinished jobs.... ./Documentation/devicetree/bindings/sound/cirrus,cs48l32.yaml:44:20: mapping values are not allowed here /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/sound/cirrus,cs48l32.yaml: ignoring, error parsing file make: *** [Makefile:1492: dt_binding_check] Error 2 doc reference errors (make refcheckdocs): Documentation/devicetree/bindings/sound/cirrus,cs48l32.yaml: Documentation/devicetree/bindings/mfd/cirrus,cs48l32.yaml See https://patchwork.ozlabs.org/patch/ This check can fail if there are any dependencies. The base for a patch series is generally the most recent rc1. 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.
On 09/11/2022 17:53, Richard Fitzgerald wrote: > Codecs in this family have multiple digital and analog audio I/O that > support a variety of external hardware connections and configurations. > > Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com> > --- > .../bindings/sound/cirrus,cs48l32.yaml | 96 +++++++++++++++++++ > include/dt-bindings/sound/cs48l32.h | 25 +++++ > 2 files changed, 121 insertions(+) > create mode 100644 Documentation/devicetree/bindings/sound/cirrus,cs48l32.yaml > create mode 100644 include/dt-bindings/sound/cs48l32.h > > diff --git a/Documentation/devicetree/bindings/sound/cirrus,cs48l32.yaml b/Documentation/devicetree/bindings/sound/cirrus,cs48l32.yaml > new file mode 100644 > index 000000000000..70fb294c6dc1 > --- /dev/null > +++ b/Documentation/devicetree/bindings/sound/cirrus,cs48l32.yaml > @@ -0,0 +1,96 @@ > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/sound/cirrus,cs48l32.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Cirrus Logic CS48L31/32/33 audio CODECs > + > +maintainers: > + - patches@opensource.cirrus.com > + > +description: | > + This describes audio configuration bindings for these codecs. Don't start with "This". Instead describe the hardware. > + > + See also the core bindings for the parent MFD driver: > + > + Documentation/devicetree/bindings/mfd/cirrus,cs48l32.yaml Same comment as for pinctrl patch. > + > + and defines for values used in these bindings: > + > + include/dt-bindings/sound/cs48l32.h > + > + The properties are all contained in the parent MFD node. > + > +properties: Missing compatible. What's the point to organize bindings like that? The schema on its own does nothing - does not match anything. > + '#sound-dai-cells': > + const: 1 > + > + cirrus,in-type: > + description: > + A list of input type settings for each input. A maximum of 8 cells, > + with four cells per input in the order INnL_1, INnR_1 INnL_2 INnR_2. > + (where _1 and _2 are the alternative mux selections for that INn). > + If the array is shorter than the number of inputs the unspecified > + inputs default to CS48L32_IN_TYPE_DIFF. > + $ref: "/schemas/types.yaml#/definitions/uint32-matrix" Drop quotes. > + minItems: 1 > + maxItems: 8 > + items: > + items: > + - description: > + The first cell is INnL_1 input type. One of the CS48L32_IN_TYPE_xxx. > + For non-muxed inputs this sets the type of INnL. What is the "input type"? Referring to constants is not enough, especially that they are not descriptive. Explain here the values. > + minimum: 0 > + maximum: 1 > + - description: > + The second cell is INnR_1 input type. One of the CS48L32_IN_TYPE_xxx. > + For non-muxed inputs this sets the type of INnR. > + minimum: 0 > + maximum: 1 > + - description: > + The third cell is INnL_2 input type. One of the CS48L32_IN_TYPE_xxx. > + For non-muxed inputs this cell must be 0. > + minimum: 0 > + maximum: 1 > + - description: > + The fourth cell is INnR_2 input type. One of the CS48L32_IN_TYPE_xxx. > + For non-muxed inputs this cell must be 0. > + minimum: 0 > + maximum: 1 > + > + cirrus,max-channels-clocked: > + description: > + Maximum number of channels that clocks will be generated for. When using > + multiple data lines, every sample slot can transfer multiple channels > + (one per data line). This pdata sets the maximum number of slots. > + One cell for each ASP, use a value of zero for ASPs that should be > + handled normally. > + $ref: /schemas/types.yaml#/definitions/uint32-array > + minItems: 1 > + maxItems: 4 > + items: > + default: 0 > + > + cirrus,pdm-sup: > + description: > + Indicates how the MICBIAS pins have been externally connected to DMICs > + on each input. One cell per input (IN1, IN2, ...). One of the > + CS48L32_MICBIAS_xxx values. > + See the INn_PDM_SUP field in the datasheet for a description. No, explain here. > + $ref: /schemas/types.yaml#/definitions/uint32-array > + minItems: 1 > + maxItems: 4 > + > +examples: > + - | > + cs48l32@0 { Node names should be generic. https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation > + compatible = "cirrus,cs48l32"; > + Use 4 spaces for example indentation. > + cirrus,in-type = < > + CS48L32_IN_TYPE_DIFF CS48L32_IN_TYPE_DIFF /* IN1[LR]_1 differential */ > + CS48L32_IN_TYPE_SE CS48L32_IN_TYPE_SE /* IN1[LR]_2 single-ended */ > + CS48L32_IN_TYPE_DIFF CS48L32_IN_TYPE_DIFF /* IN2[LR]_1 differential */ > + >; > + cirrus,max-channels-clocked = <2 0 0>; > + }; > diff --git a/include/dt-bindings/sound/cs48l32.h b/include/dt-bindings/sound/cs48l32.h > new file mode 100644 > index 000000000000..0b774da0a6c8 > --- /dev/null > +++ b/include/dt-bindings/sound/cs48l32.h > @@ -0,0 +1,25 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ Dual license. > +/* > + * Device Tree defines for CS48L32 codec. > + * > + * Copyright (C) 2016-2018, 2022 Cirrus Logic, Inc. and > + * Cirrus Logic International Semiconductor Ltd. > + */ > + > +#ifndef DT_BINDINGS_SOUND_CS48L32_H > +#define DT_BINDINGS_SOUND_CS48L32_H > + > +#define CS48L32_IN_TYPE_DIFF 0 > +#define CS48L32_IN_TYPE_SE 1 > + > +#define CS48L32_PDM_SUP_VOUT_MIC 0 > +#define CS48L32_PDM_SUP_MICBIAS1 1 > +#define CS48L32_PDM_SUP_MICBIAS2 2 > +#define CS48L32_PDM_SUP_MICBIAS3 3 > + > +#define CS48L32_PDM_FMT_MODE_A_LSB_FIRST 0x0000 > +#define CS48L32_PDM_FMT_MODE_B_LSB_FIRST 0x4000 > +#define CS48L32_PDM_FMT_MODE_A_MSB_FIRST 0x8000 > +#define CS48L32_PDM_FMT_MODE_B_MSB_FIRST 0xc000 Register values do not belong to bindings. > + > +#endif Best regards, Krzysztof
On 14/11/2022 08:45, Krzysztof Kozlowski wrote: > On 09/11/2022 17:53, Richard Fitzgerald wrote: >> Codecs in this family have multiple digital and analog audio I/O that >> support a variety of external hardware connections and configurations. >> >> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com> >> --- >> .../bindings/sound/cirrus,cs48l32.yaml | 96 +++++++++++++++++++ >> include/dt-bindings/sound/cs48l32.h | 25 +++++ >> 2 files changed, 121 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/sound/cirrus,cs48l32.yaml >> create mode 100644 include/dt-bindings/sound/cs48l32.h >> >> diff --git a/Documentation/devicetree/bindings/sound/cirrus,cs48l32.yaml b/Documentation/devicetree/bindings/sound/cirrus,cs48l32.yaml >> new file mode 100644 >> index 000000000000..70fb294c6dc1 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/sound/cirrus,cs48l32.yaml >> @@ -0,0 +1,96 @@ >> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/sound/cirrus,cs48l32.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Cirrus Logic CS48L31/32/33 audio CODECs >> + >> +maintainers: >> + - patches@opensource.cirrus.com >> + >> +description: | >> + This describes audio configuration bindings for these codecs. > > Don't start with "This". Instead describe the hardware. > >> + >> + See also the core bindings for the parent MFD driver: >> + >> + Documentation/devicetree/bindings/mfd/cirrus,cs48l32.yaml > > Same comment as for pinctrl patch. > >> + >> + and defines for values used in these bindings: >> + >> + include/dt-bindings/sound/cs48l32.h >> + >> + The properties are all contained in the parent MFD node. >> + >> +properties: > > Missing compatible. What's the point to organize bindings like that? The > schema on its own does nothing - does not match anything. Do you mean child drivers should not share the MFD node? Or do you mean that if they share the MFD node all the child driver bindings should be documented in the MFD schema instead of having a sub-schema for each class of hardware functionality? I'm certainly willing to collapse all the bindings into a single MFD schema yaml. For this driver we followed the same structure that was accepted for madera (and there was some discussion when we upstreamed madera about how the bindings should be organized which resulted in them being changed). We pretty much assumed that the safe bet was to do the same that was accepted by the maintainer last time around.
On 14/11/2022 12:00, Richard Fitzgerald wrote: > On 14/11/2022 08:45, Krzysztof Kozlowski wrote: >> On 09/11/2022 17:53, Richard Fitzgerald wrote: >>> Codecs in this family have multiple digital and analog audio I/O that >>> support a variety of external hardware connections and configurations. >>> >>> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com> >>> --- >>> .../bindings/sound/cirrus,cs48l32.yaml | 96 +++++++++++++++++++ >>> include/dt-bindings/sound/cs48l32.h | 25 +++++ >>> 2 files changed, 121 insertions(+) >>> create mode 100644 Documentation/devicetree/bindings/sound/cirrus,cs48l32.yaml >>> create mode 100644 include/dt-bindings/sound/cs48l32.h >>> >>> diff --git a/Documentation/devicetree/bindings/sound/cirrus,cs48l32.yaml b/Documentation/devicetree/bindings/sound/cirrus,cs48l32.yaml >>> new file mode 100644 >>> index 000000000000..70fb294c6dc1 >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/sound/cirrus,cs48l32.yaml >>> @@ -0,0 +1,96 @@ >>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause >>> +%YAML 1.2 >>> +--- >>> +$id: http://devicetree.org/schemas/sound/cirrus,cs48l32.yaml# >>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>> + >>> +title: Cirrus Logic CS48L31/32/33 audio CODECs >>> + >>> +maintainers: >>> + - patches@opensource.cirrus.com >>> + >>> +description: | >>> + This describes audio configuration bindings for these codecs. >> >> Don't start with "This". Instead describe the hardware. >> >>> + >>> + See also the core bindings for the parent MFD driver: >>> + >>> + Documentation/devicetree/bindings/mfd/cirrus,cs48l32.yaml >> >> Same comment as for pinctrl patch. >> >>> + >>> + and defines for values used in these bindings: >>> + >>> + include/dt-bindings/sound/cs48l32.h >>> + >>> + The properties are all contained in the parent MFD node. >>> + >>> +properties: >> >> Missing compatible. What's the point to organize bindings like that? The >> schema on its own does nothing - does not match anything. > > Do you mean child drivers should not share the MFD node? Or do you mean > that if they share the MFD node all the child driver bindings should be > documented in the MFD schema instead of having a sub-schema for each > class of hardware functionality? I mean, that regular binding has a compatible which allows the schema to be matched. Splitting parts from top-level properties is used only for re-usable shared/common schemas, which does not seem the case here. > > I'm certainly willing to collapse all the bindings into a single MFD > schema yaml. For this driver we followed the same structure that was > accepted for madera (and there was some discussion when we upstreamed > madera about how the bindings should be organized which resulted in > them being changed). We pretty much assumed that the safe bet was to do > the same that was accepted by the maintainer last time around. Just merge it with MFD binding. Best regards, Krzysztof
On 14/11/2022 11:03, Krzysztof Kozlowski wrote: > On 14/11/2022 12:00, Richard Fitzgerald wrote: >> On 14/11/2022 08:45, Krzysztof Kozlowski wrote: >>> On 09/11/2022 17:53, Richard Fitzgerald wrote: >>>> Codecs in this family have multiple digital and analog audio I/O that >>>> support a variety of external hardware connections and configurations. >>>> >>>> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com> >>>> --- >>>> .../bindings/sound/cirrus,cs48l32.yaml | 96 +++++++++++++++++++ >>>> include/dt-bindings/sound/cs48l32.h | 25 +++++ >>>> 2 files changed, 121 insertions(+) >>>> create mode 100644 Documentation/devicetree/bindings/sound/cirrus,cs48l32.yaml >>>> create mode 100644 include/dt-bindings/sound/cs48l32.h >>>> >>>> diff --git a/Documentation/devicetree/bindings/sound/cirrus,cs48l32.yaml b/Documentation/devicetree/bindings/sound/cirrus,cs48l32.yaml >>>> new file mode 100644 >>>> index 000000000000..70fb294c6dc1 >>>> --- /dev/null >>>> +++ b/Documentation/devicetree/bindings/sound/cirrus,cs48l32.yaml >>>> @@ -0,0 +1,96 @@ >>>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause >>>> +%YAML 1.2 >>>> +--- >>>> +$id: http://devicetree.org/schemas/sound/cirrus,cs48l32.yaml# >>>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>>> + >>>> +title: Cirrus Logic CS48L31/32/33 audio CODECs >>>> + >>>> +maintainers: >>>> + - patches@opensource.cirrus.com >>>> + >>>> +description: | >>>> + This describes audio configuration bindings for these codecs. >>> >>> Don't start with "This". Instead describe the hardware. >>> >>>> + >>>> + See also the core bindings for the parent MFD driver: >>>> + >>>> + Documentation/devicetree/bindings/mfd/cirrus,cs48l32.yaml >>> >>> Same comment as for pinctrl patch. >>> >>>> + >>>> + and defines for values used in these bindings: >>>> + >>>> + include/dt-bindings/sound/cs48l32.h >>>> + >>>> + The properties are all contained in the parent MFD node. >>>> + >>>> +properties: >>> >>> Missing compatible. What's the point to organize bindings like that? The >>> schema on its own does nothing - does not match anything. >> >> Do you mean child drivers should not share the MFD node? Or do you mean >> that if they share the MFD node all the child driver bindings should be >> documented in the MFD schema instead of having a sub-schema for each >> class of hardware functionality? > > I mean, that regular binding has a compatible which allows the schema to > be matched. > > Splitting parts from top-level properties is used only for re-usable > shared/common schemas, which does not seem the case here. > Ok, that's good. None of these drivers are re-useable standalone. I'll squash the bindings all into MFD schema for V2. >> >> I'm certainly willing to collapse all the bindings into a single MFD >> schema yaml. For this driver we followed the same structure that was >> accepted for madera (and there was some discussion when we upstreamed >> madera about how the bindings should be organized which resulted in >> them being changed). We pretty much assumed that the safe bet was to do >> the same that was accepted by the maintainer last time around. > > Just merge it with MFD binding. > > Best regards, > Krzysztof >
diff --git a/Documentation/devicetree/bindings/sound/cirrus,cs48l32.yaml b/Documentation/devicetree/bindings/sound/cirrus,cs48l32.yaml new file mode 100644 index 000000000000..70fb294c6dc1 --- /dev/null +++ b/Documentation/devicetree/bindings/sound/cirrus,cs48l32.yaml @@ -0,0 +1,96 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/sound/cirrus,cs48l32.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Cirrus Logic CS48L31/32/33 audio CODECs + +maintainers: + - patches@opensource.cirrus.com + +description: | + This describes audio configuration bindings for these codecs. + + See also the core bindings for the parent MFD driver: + + Documentation/devicetree/bindings/mfd/cirrus,cs48l32.yaml + + and defines for values used in these bindings: + + include/dt-bindings/sound/cs48l32.h + + The properties are all contained in the parent MFD node. + +properties: + '#sound-dai-cells': + const: 1 + + cirrus,in-type: + description: + A list of input type settings for each input. A maximum of 8 cells, + with four cells per input in the order INnL_1, INnR_1 INnL_2 INnR_2. + (where _1 and _2 are the alternative mux selections for that INn). + If the array is shorter than the number of inputs the unspecified + inputs default to CS48L32_IN_TYPE_DIFF. + $ref: "/schemas/types.yaml#/definitions/uint32-matrix" + minItems: 1 + maxItems: 8 + items: + items: + - description: + The first cell is INnL_1 input type. One of the CS48L32_IN_TYPE_xxx. + For non-muxed inputs this sets the type of INnL. + minimum: 0 + maximum: 1 + - description: + The second cell is INnR_1 input type. One of the CS48L32_IN_TYPE_xxx. + For non-muxed inputs this sets the type of INnR. + minimum: 0 + maximum: 1 + - description: + The third cell is INnL_2 input type. One of the CS48L32_IN_TYPE_xxx. + For non-muxed inputs this cell must be 0. + minimum: 0 + maximum: 1 + - description: + The fourth cell is INnR_2 input type. One of the CS48L32_IN_TYPE_xxx. + For non-muxed inputs this cell must be 0. + minimum: 0 + maximum: 1 + + cirrus,max-channels-clocked: + description: + Maximum number of channels that clocks will be generated for. When using + multiple data lines, every sample slot can transfer multiple channels + (one per data line). This pdata sets the maximum number of slots. + One cell for each ASP, use a value of zero for ASPs that should be + handled normally. + $ref: /schemas/types.yaml#/definitions/uint32-array + minItems: 1 + maxItems: 4 + items: + default: 0 + + cirrus,pdm-sup: + description: + Indicates how the MICBIAS pins have been externally connected to DMICs + on each input. One cell per input (IN1, IN2, ...). One of the + CS48L32_MICBIAS_xxx values. + See the INn_PDM_SUP field in the datasheet for a description. + $ref: /schemas/types.yaml#/definitions/uint32-array + minItems: 1 + maxItems: 4 + +examples: + - | + cs48l32@0 { + compatible = "cirrus,cs48l32"; + + cirrus,in-type = < + CS48L32_IN_TYPE_DIFF CS48L32_IN_TYPE_DIFF /* IN1[LR]_1 differential */ + CS48L32_IN_TYPE_SE CS48L32_IN_TYPE_SE /* IN1[LR]_2 single-ended */ + CS48L32_IN_TYPE_DIFF CS48L32_IN_TYPE_DIFF /* IN2[LR]_1 differential */ + >; + cirrus,max-channels-clocked = <2 0 0>; + }; diff --git a/include/dt-bindings/sound/cs48l32.h b/include/dt-bindings/sound/cs48l32.h new file mode 100644 index 000000000000..0b774da0a6c8 --- /dev/null +++ b/include/dt-bindings/sound/cs48l32.h @@ -0,0 +1,25 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * Device Tree defines for CS48L32 codec. + * + * Copyright (C) 2016-2018, 2022 Cirrus Logic, Inc. and + * Cirrus Logic International Semiconductor Ltd. + */ + +#ifndef DT_BINDINGS_SOUND_CS48L32_H +#define DT_BINDINGS_SOUND_CS48L32_H + +#define CS48L32_IN_TYPE_DIFF 0 +#define CS48L32_IN_TYPE_SE 1 + +#define CS48L32_PDM_SUP_VOUT_MIC 0 +#define CS48L32_PDM_SUP_MICBIAS1 1 +#define CS48L32_PDM_SUP_MICBIAS2 2 +#define CS48L32_PDM_SUP_MICBIAS3 3 + +#define CS48L32_PDM_FMT_MODE_A_LSB_FIRST 0x0000 +#define CS48L32_PDM_FMT_MODE_B_LSB_FIRST 0x4000 +#define CS48L32_PDM_FMT_MODE_A_MSB_FIRST 0x8000 +#define CS48L32_PDM_FMT_MODE_B_MSB_FIRST 0xc000 + +#endif
Codecs in this family have multiple digital and analog audio I/O that support a variety of external hardware connections and configurations. Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com> --- .../bindings/sound/cirrus,cs48l32.yaml | 96 +++++++++++++++++++ include/dt-bindings/sound/cs48l32.h | 25 +++++ 2 files changed, 121 insertions(+) create mode 100644 Documentation/devicetree/bindings/sound/cirrus,cs48l32.yaml create mode 100644 include/dt-bindings/sound/cs48l32.h