| Message ID | 20230510112349.939991-3-aidanmacdonald.0x0@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On 10/05/2023 13:23, Aidan MacDonald wrote: > Device tree bindings for the ESS ES9218P codec, which uses an > I2C control interface. > Thank you for your patch. There is something to discuss/improve. Subject: reverse dt-bindings and ASoC. Subject: drop second/last, redundant "bindings". The "dt-bindings" prefix is already stating that these are bindings. > Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com> > --- > .../bindings/sound/ess,es9218p.yaml | 104 ++++++++++++++++++ > 1 file changed, 104 insertions(+) > create mode 100644 Documentation/devicetree/bindings/sound/ess,es9218p.yaml > > diff --git a/Documentation/devicetree/bindings/sound/ess,es9218p.yaml b/Documentation/devicetree/bindings/sound/ess,es9218p.yaml > new file mode 100644 > index 000000000000..d205b6a8bbb2 > --- /dev/null > +++ b/Documentation/devicetree/bindings/sound/ess,es9218p.yaml > @@ -0,0 +1,104 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/sound/ess,es9218p.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: ESS ES9218P audio codec > + > +maintainers: > + - Aidan MacDonald <aidanmacdonald.0x0@gmail.com> > + allOf with ref to dai-common. > +properties: > + compatible: > + const: ess,es9218p > + > + reg: > + maxItems: 1 > + > + "#sound-dai-cells": > + const: 0 > + > + avdd-supply: > + description: > + Definition of the regulator used for digital core power supply. Drop "Definition of the regulator used for" > + > + vcca-supply: > + description: > + Definition of the regulator used for oscillator and DAC power supply. > + Ditto > + avcc3v3-supply: > + description: > + Definition of the 3.3V regulator for amplifier, switch, and charge pumps. Drop "Definition of" > + > + avcc1v8-supply: > + description: > + Definition of the 1.8V regulator for amplifier, switch, and charge pumps. Ditto > + > + clocks: > + items: > + - description: clock for master clock (MCLK) > + > + clock-names: > + items: > + - const: mclk Drop entire names, not really useful. > + > + reset-gpios: > + maxItems: 1 > + description: > + Pin used for codec hardware reset, corresponds to the RESETB pin. > + > + ess,max-clock-div: > + $ref: /schemas/types.yaml#/definitions/uint32 > + description: > + Sets the maximum MCLK divider for generating the internal CLK. > + CLK must be at least 20x the I2C bus speed or I2C transactions > + will fail. The maximum divider should be chosen to ensure that > + CLK will not fall below the limit. > + enum: > + - 1 > + - 2 > + - 4 > + - 8 > + default: 1 Why do you need to customize it per board? > + > + ess,oscillator-pad-bias: > + $ref: /schemas/types.yaml#/definitions/uint32 > + description: Sets the oscillator pad drive bias. 0 - full bias, 15 - disabled. > + minimum: 0 > + maximum: 15 > + default: 0 > + > +required: > + - compatible > + - reg > + - "#sound-dai-cells" > + - avdd-supply > + - vcca-supply > + - avcc3v3-supply > + - avcc1v8-supply > + - clocks > + - clock-names > + - reset-gpios > + > +additionalProperties: false Instead: unevaluatedProperties: false > + > +examples: Best regards, Krzysztof
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> writes: > On 10/05/2023 13:23, Aidan MacDonald wrote: >> ... >> + ess,max-clock-div: >> + $ref: /schemas/types.yaml#/definitions/uint32 >> + description: >> + Sets the maximum MCLK divider for generating the internal CLK. >> + CLK must be at least 20x the I2C bus speed or I2C transactions >> + will fail. The maximum divider should be chosen to ensure that >> + CLK will not fall below the limit. >> + enum: >> + - 1 >> + - 2 >> + - 4 >> + - 8 >> + default: 1 > > Why do you need to customize it per board? > There's no generic API to read or change the bus speed (SCL frequency) for I2C adapters, so it's impossible to calculate a limit on the MCLK divider automatically. Defaulting to 1 is always safe, but means wasting power at lower sample rates. If you know what the bus speed is you can use a higher divider limit to save power, and it has to be done at the board/firmware level because that's the only place where the bus speed is known. >> + >> + ess,oscillator-pad-bias: >> + $ref: /schemas/types.yaml#/definitions/uint32 >> + description: Sets the oscillator pad drive bias. 0 - full bias, 15 - disabled. >> + minimum: 0 >> + maximum: 15 >> + default: 0 >> + >> +required: >> + - compatible >> + - reg >> + - "#sound-dai-cells" >> + - avdd-supply >> + - vcca-supply >> + - avcc3v3-supply >> + - avcc1v8-supply >> + - clocks >> + - clock-names >> + - reset-gpios >> + >> +additionalProperties: false > > Instead: unevaluatedProperties: false > > >> + >> +examples: > > Best regards, > Krzysztof Everything else agreed, I'll send v2 later today. Regards, Aidan
On 11/05/2023 12:15, Aidan MacDonald wrote: > > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> writes: > >> On 10/05/2023 13:23, Aidan MacDonald wrote: >>> ... >>> + ess,max-clock-div: >>> + $ref: /schemas/types.yaml#/definitions/uint32 >>> + description: >>> + Sets the maximum MCLK divider for generating the internal CLK. >>> + CLK must be at least 20x the I2C bus speed or I2C transactions >>> + will fail. The maximum divider should be chosen to ensure that >>> + CLK will not fall below the limit. >>> + enum: >>> + - 1 >>> + - 2 >>> + - 4 >>> + - 8 >>> + default: 1 >> >> Why do you need to customize it per board? >> > > There's no generic API to read or change the bus speed (SCL frequency) > for I2C adapters, so it's impossible to calculate a limit on the MCLK > divider automatically. > > Defaulting to 1 is always safe, but means wasting power at lower sample > rates. If you know what the bus speed is you can use a higher divider > limit to save power, and it has to be done at the board/firmware level > because that's the only place where the bus speed is known. If I understand correctly, you only miss a way to get bus_freq_hz from i2c_timings to calculate the divider by yourself? This looks like Linux limitation, so we shouldn't push it into DT, but rather fix Linux. The I2C bus rate is known, the MCLK rate as well, so divider is possible to deduce. I am actually surprised that I2C core does not store the timings anywhere and each bus host has to deal with it on its own. Best regards, Krzysztof
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> writes: > On 11/05/2023 12:15, Aidan MacDonald wrote: >> >> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> writes: >> >>> On 10/05/2023 13:23, Aidan MacDonald wrote: >>>> ... >>>> + ess,max-clock-div: >>>> + $ref: /schemas/types.yaml#/definitions/uint32 >>>> + description: >>>> + Sets the maximum MCLK divider for generating the internal CLK. >>>> + CLK must be at least 20x the I2C bus speed or I2C transactions >>>> + will fail. The maximum divider should be chosen to ensure that >>>> + CLK will not fall below the limit. >>>> + enum: >>>> + - 1 >>>> + - 2 >>>> + - 4 >>>> + - 8 >>>> + default: 1 >>> >>> Why do you need to customize it per board? >>> >> >> There's no generic API to read or change the bus speed (SCL frequency) >> for I2C adapters, so it's impossible to calculate a limit on the MCLK >> divider automatically. >> >> Defaulting to 1 is always safe, but means wasting power at lower sample >> rates. If you know what the bus speed is you can use a higher divider >> limit to save power, and it has to be done at the board/firmware level >> because that's the only place where the bus speed is known. > > If I understand correctly, you only miss a way to get bus_freq_hz from > i2c_timings to calculate the divider by yourself? This looks like Linux > limitation, so we shouldn't push it into DT, but rather fix Linux. The > I2C bus rate is known, the MCLK rate as well, so divider is possible to > deduce. > > I am actually surprised that I2C core does not store the timings > anywhere and each bus host has to deal with it on its own. > > Best regards, > Krzysztof I agree it'd be better if Linux provided access the bus frequency, but even if that API was added it will take time for every I2C adapter to support it. So in that case we would still need a DT property to provide a safe limit or use a safe default, and miss potential power savings. I'd prefer to add the DT property to allow power savings to be had now, and drop it if/when the kernel gets an API for bus frequency. That will be safe from a compatibility point of view -- the property won't be providing any useful information so it won't matter if old DTs have it. Regards, Aidan
On 11/05/2023 13:26, Aidan MacDonald wrote: > > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> writes: > >> On 11/05/2023 12:15, Aidan MacDonald wrote: >>> >>> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> writes: >>> >>>> On 10/05/2023 13:23, Aidan MacDonald wrote: >>>>> ... >>>>> + ess,max-clock-div: >>>>> + $ref: /schemas/types.yaml#/definitions/uint32 >>>>> + description: >>>>> + Sets the maximum MCLK divider for generating the internal CLK. >>>>> + CLK must be at least 20x the I2C bus speed or I2C transactions >>>>> + will fail. The maximum divider should be chosen to ensure that >>>>> + CLK will not fall below the limit. >>>>> + enum: >>>>> + - 1 >>>>> + - 2 >>>>> + - 4 >>>>> + - 8 >>>>> + default: 1 >>>> >>>> Why do you need to customize it per board? >>>> >>> >>> There's no generic API to read or change the bus speed (SCL frequency) >>> for I2C adapters, so it's impossible to calculate a limit on the MCLK >>> divider automatically. >>> >>> Defaulting to 1 is always safe, but means wasting power at lower sample >>> rates. If you know what the bus speed is you can use a higher divider >>> limit to save power, and it has to be done at the board/firmware level >>> because that's the only place where the bus speed is known. >> >> If I understand correctly, you only miss a way to get bus_freq_hz from >> i2c_timings to calculate the divider by yourself? This looks like Linux >> limitation, so we shouldn't push it into DT, but rather fix Linux. The >> I2C bus rate is known, the MCLK rate as well, so divider is possible to >> deduce. >> >> I am actually surprised that I2C core does not store the timings >> anywhere and each bus host has to deal with it on its own. >> >> Best regards, >> Krzysztof > > I agree it'd be better if Linux provided access the bus frequency, but > even if that API was added it will take time for every I2C adapter to > support it. So in that case we would still need a DT property to provide > a safe limit or use a safe default, and miss potential power savings. > > I'd prefer to add the DT property to allow power savings to be had now, > and drop it if/when the kernel gets an API for bus frequency. That will > be safe from a compatibility point of view -- the property won't be > providing any useful information so it won't matter if old DTs have it. OK, sounds good. Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/sound/ess,es9218p.yaml b/Documentation/devicetree/bindings/sound/ess,es9218p.yaml new file mode 100644 index 000000000000..d205b6a8bbb2 --- /dev/null +++ b/Documentation/devicetree/bindings/sound/ess,es9218p.yaml @@ -0,0 +1,104 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/sound/ess,es9218p.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: ESS ES9218P audio codec + +maintainers: + - Aidan MacDonald <aidanmacdonald.0x0@gmail.com> + +properties: + compatible: + const: ess,es9218p + + reg: + maxItems: 1 + + "#sound-dai-cells": + const: 0 + + avdd-supply: + description: + Definition of the regulator used for digital core power supply. + + vcca-supply: + description: + Definition of the regulator used for oscillator and DAC power supply. + + avcc3v3-supply: + description: + Definition of the 3.3V regulator for amplifier, switch, and charge pumps. + + avcc1v8-supply: + description: + Definition of the 1.8V regulator for amplifier, switch, and charge pumps. + + clocks: + items: + - description: clock for master clock (MCLK) + + clock-names: + items: + - const: mclk + + reset-gpios: + maxItems: 1 + description: + Pin used for codec hardware reset, corresponds to the RESETB pin. + + ess,max-clock-div: + $ref: /schemas/types.yaml#/definitions/uint32 + description: + Sets the maximum MCLK divider for generating the internal CLK. + CLK must be at least 20x the I2C bus speed or I2C transactions + will fail. The maximum divider should be chosen to ensure that + CLK will not fall below the limit. + enum: + - 1 + - 2 + - 4 + - 8 + default: 1 + + ess,oscillator-pad-bias: + $ref: /schemas/types.yaml#/definitions/uint32 + description: Sets the oscillator pad drive bias. 0 - full bias, 15 - disabled. + minimum: 0 + maximum: 15 + default: 0 + +required: + - compatible + - reg + - "#sound-dai-cells" + - avdd-supply + - vcca-supply + - avcc3v3-supply + - avcc1v8-supply + - clocks + - clock-names + - reset-gpios + +additionalProperties: false + +examples: + - | + #include <dt-bindings/gpio/gpio.h> + i2c { + #address-cells = <1>; + #size-cells = <0>; + codec@48 { + compatible = "ess,es9218p"; + reg = <0x48>; + #sound-dai-cells = <0>; + avdd-supply = <&avdd>; + vcca-supply = <&vcca>; + avcc3v3-supply = <&avcc3v3>; + avcc1v8-supply = <&avcc1v8>; + clocks = <&xtal_clock>; + clock-names = "mclk"; + reset-gpios = <&gpio 1 GPIO_ACTIVE_LOW>; + }; + };
Device tree bindings for the ESS ES9218P codec, which uses an I2C control interface. Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com> --- .../bindings/sound/ess,es9218p.yaml | 104 ++++++++++++++++++ 1 file changed, 104 insertions(+) create mode 100644 Documentation/devicetree/bindings/sound/ess,es9218p.yaml