| Message ID | 1429134141-17924-1-git-send-email-cernekee@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Apr 15, 2015 at 02:42:19PM -0700, Kevin Cernekee wrote:
> +- VDD-supply: regulator phandle for the AVDD/DVDD/HP_VDD supply
This is clearly not correct - if there are three separate physical
supplies there should be three separate regulators requested. They may
all resolve to one physical regulator on the board you are working with
but that might not be true on other boards.
Resending from the correct account. On Sat, Apr 18, 2015 at 4:16 AM, Mark Brown <broonie@kernel.org> wrote: > On Wed, Apr 15, 2015 at 02:42:19PM -0700, Kevin Cernekee wrote: > >> +- VDD-supply: regulator phandle for the AVDD/DVDD/HP_VDD supply > > This is clearly not correct - if there are three separate physical > supplies there should be three separate regulators requested. They may > all resolve to one physical regulator on the board you are working with > but that might not be true on other boards. In the "simplified diagram," TI shows a single AVDD/DVDD/HP_VDD supply: http://www.ti.com/lit/ds/symlink/tas5717.pdf#2 Page 20 also suggests the use of a single 3.3V supply for AVDD/DVDD/HP_VDD. But this combines a number of separate pins. On 5711 we have dedicated pins for: PVDD_A PVDD_B PVDD_C PVDD_D AVDD DVDD On 5717 we have dedicated pins for: PVDD_AB PVDD_CD AVDD DVDD HPVDD I didn't see anything in the datasheet suggesting it is OK to have different voltages or power states on the various supply pins (other than the special voltage on PVDD). I can add as many regulator entries as appropriate. What do you recommend?
On Mon, Apr 20, 2015 at 02:18:56PM -0700, Kevin Cernekee wrote: > On Sat, Apr 18, 2015 at 4:16 AM, Mark Brown <broonie@kernel.org> wrote: > > On Wed, Apr 15, 2015 at 02:42:19PM -0700, Kevin Cernekee wrote: > >> +- VDD-supply: regulator phandle for the AVDD/DVDD/HP_VDD supply > > This is clearly not correct - if there are three separate physical > > supplies there should be three separate regulators requested. They may > > all resolve to one physical regulator on the board you are working with > > but that might not be true on other boards. > In the "simplified diagram," TI shows a single AVDD/DVDD/HP_VDD supply: > http://www.ti.com/lit/ds/symlink/tas5717.pdf#2 > Page 20 also suggests the use of a single 3.3V supply for AVDD/DVDD/HP_VDD. > But this combines a number of separate pins. On 5711 we have > dedicated pins for: > PVDD_A > PVDD_B > PVDD_C > PVDD_D > AVDD > DVDD Yes, those are three separate supplies that are typically tied together - it looks like PVDD has high current draw so is tied through multiple pins. I'd not be surprised to see systems with AVDD tied to a separate pin, analogue supplies often benefit from low noise supplies separate to digital ones. Indeed if you look at the pin descriptions the analogue and digital supplies even have separate grounds. > I didn't see anything in the datasheet suggesting it is OK to have > different voltages or power states on the various supply pins (other > than the special voltage on PVDD). That's more likely to go wrong if they're not controlled separately than if they are since we will only be able to turn a single supply on or off.. > I can add as many regulator entries as appropriate. What do you recommend? Have the driver accurately reflect the hardware, request one regulator per supply.
On Mon, Apr 20, 2015 at 3:03 PM, Mark Brown <broonie@kernel.org> wrote: > On Mon, Apr 20, 2015 at 02:18:56PM -0700, Kevin Cernekee wrote: >> On Sat, Apr 18, 2015 at 4:16 AM, Mark Brown <broonie@kernel.org> wrote: >> > On Wed, Apr 15, 2015 at 02:42:19PM -0700, Kevin Cernekee wrote: > >> >> +- VDD-supply: regulator phandle for the AVDD/DVDD/HP_VDD supply > >> > This is clearly not correct - if there are three separate physical >> > supplies there should be three separate regulators requested. They may >> > all resolve to one physical regulator on the board you are working with >> > but that might not be true on other boards. > >> In the "simplified diagram," TI shows a single AVDD/DVDD/HP_VDD supply: > >> http://www.ti.com/lit/ds/symlink/tas5717.pdf#2 > >> Page 20 also suggests the use of a single 3.3V supply for AVDD/DVDD/HP_VDD. > >> But this combines a number of separate pins. On 5711 we have >> dedicated pins for: > >> PVDD_A >> PVDD_B >> PVDD_C >> PVDD_D >> AVDD >> DVDD > > Yes, those are three separate supplies that are typically tied together > - it looks like PVDD has high current draw so is tied through multiple > pins. Each half-bridge output OUT_{A,B,C,D} has its own dedicated PVDD_{A,B,C,D} supply on 5711: http://www.ti.com/lit/ds/symlink/tas5711.pdf#7 I don't understand the statement: "those are three separate supplies that are typically tied together." AVDD/DVDD are 3.3v fixed but PVDD is 8V minimum (5711) or 4.5V minimum (5717/5719). So AVDD/DVDD can never be driven by the same regulator output as PVDD. Perhaps you were thinking of HPVDD/AVDD/DVDD on 5717/5719? The simplified application diagram on page 2 does show that AVDD/DVDD are typically tied together, and that PVDD_* are also typically tied together. But as you pointed out, there may be situations where that isn't the case, so that leaves us with up to 6 separate supplies on 5711, or 5 on 5717/5719. The typical case is probably closer to 2 or 3, though. > I'd not be surprised to see systems with AVDD tied to a separate > pin, analogue supplies often benefit from low noise supplies separate to > digital ones. Indeed if you look at the pin descriptions the analogue > and digital supplies even have separate grounds. > >> I didn't see anything in the datasheet suggesting it is OK to have >> different voltages or power states on the various supply pins (other >> than the special voltage on PVDD). > > That's more likely to go wrong if they're not controlled separately than > if they are since we will only be able to turn a single supply on or > off.. > >> I can add as many regulator entries as appropriate. What do you recommend? > > Have the driver accurately reflect the hardware, request one regulator > per supply. So, request one regulator per VDD pin? Or group all of PVDD_* into a single regulator?
On Mon, Apr 20, 2015 at 03:48:19PM -0700, Kevin Cernekee wrote: > I don't understand the statement: "those are three separate supplies > that are typically tied together." AVDD/DVDD are 3.3v fixed but PVDD > is 8V minimum (5711) or 4.5V minimum (5717/5719). So AVDD/DVDD can > never be driven by the same regulator output as PVDD. Perhaps you > were thinking of HPVDD/AVDD/DVDD on 5717/5719? Yes, all the supplies. I'd misremembered which ones you were saying were tied together on your board. > The simplified application diagram on page 2 does show that AVDD/DVDD > are typically tied together, and that PVDD_* are also typically tied > together. But as you pointed out, there may be situations where that > isn't the case, so that leaves us with up to 6 separate supplies on > 5711, or 5 on 5717/5719. The typical case is probably closer to 2 or > 3, though. To repeat, the typical case is *not* *relevant*. All that matters is the set of supplies the device has. > > Have the driver accurately reflect the hardware, request one regulator > > per supply. > So, request one regulator per VDD pin? Or group all of PVDD_* into a > single regulator? Please group them into supplies as documented in the datasheet. If they are documented as distinct things they should be distinct things, if they are just the same supply coming in through multiple pins for power reasons they should be one supply.
diff --git a/Documentation/devicetree/bindings/sound/tas571x.txt b/Documentation/devicetree/bindings/sound/tas571x.txt new file mode 100644 index 000000000000..ac704c1f143d --- /dev/null +++ b/Documentation/devicetree/bindings/sound/tas571x.txt @@ -0,0 +1,34 @@ +Texas Instruments TAS5711/TAS5717/TAS5719 stereo power amplifiers + +The codec is controlled through an I2C interface. It also has two other +signals that can be wired up to GPIOs: reset (strongly recommended), and +powerdown (optional). + +Required properties: + +- compatible: "ti,tas5711", "ti,tas5717", or "ti,tas5719" +- reg: The I2C address of the device +- #sound-dai-cells: must be equal to 0 + +Optional properties: + +- reset-gpios: GPIO specifier for the TAS571x's active low reset line +- pdn-gpios: GPIO specifier for the TAS571x's active low powerdown line +- clocks: clock phandle for the MCLK input +- clock-names: should be "mclk" +- VDD-supply: regulator phandle for the AVDD/DVDD/HP_VDD supply +- PVDD-supply: regulator phandle for the PVDD supply + +Example: + + tas5717: audio-codec@2a { + compatible = "ti,tas5717"; + reg = <0x2a>; + #sound-dai-cells = <0>; + + reset-gpios = <&gpio5 1 GPIO_ACTIVE_HIGH>; + pdn-gpios = <&gpio5 2 GPIO_ACTIVE_HIGH>; + + clocks = <&clk_core CLK_I2S>; + clock-names = "mclk"; + };
Document the bindings for the soon-to-be-added tas571x driver. Signed-off-by: Kevin Cernekee <cernekee@chromium.org> --- .../devicetree/bindings/sound/tas571x.txt | 34 ++++++++++++++++++++++ 1 file changed, 34 insertions(+) create mode 100644 Documentation/devicetree/bindings/sound/tas571x.txt