TAS2770 can pull down and zero-fill SDOUT when not actively transmitting
TDM slot data. Zero-fill is useful when there are no other amps on the
bus. Pulldown is useful when the chip is attached to a shared bus.
Signed-off-by: James Calligeros <jcalligeros99@gmail.com>
---
.../bindings/sound/ti,tas2770.yaml | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/Documentation/devicetree/bindings/sound/ti,tas2770.yaml b/Documentation/devicetree/bindings/sound/ti,tas2770.yaml
index 8eab98a0f7a25a9c87d2c56fd0635ff8ecee17d0..3eba9bb34a581526f68b6bf2e8437e1f1e03d26f 100644
--- a/Documentation/devicetree/bindings/sound/ti,tas2770.yaml
+++ b/Documentation/devicetree/bindings/sound/ti,tas2770.yaml
@@ -57,6 +57,18 @@ properties:
- 0 # Rising edge
- 1 # Falling edge
+ ti,sdout-pull-down:
+ $ref: /schemas/types.yaml#/definitions/flag
+ description:
+ If present, SDOUT will be pulled low when not
+ transmitting.
+
+ ti,sdout-zero-fill:
+ $ref: /schemas/types.yaml#/definitions/flag
+ description:
+ If present, SDOUT will be zero-filled when not
+ transmitting.
+
'#sound-dai-cells':
# The codec has a single DAI, the #sound-dai-cells=<1>; case is left in for backward
# compatibility but is deprecated.
--
2.48.1On Thu, Feb 27, 2025 at 10:07:44PM +1000, James Calligeros wrote: > TAS2770 can pull down and zero-fill SDOUT when not actively transmitting > TDM slot data. Zero-fill is useful when there are no other amps on the > bus. Pulldown is useful when the chip is attached to a shared bus. > > Signed-off-by: James Calligeros <jcalligeros99@gmail.com> > --- > .../bindings/sound/ti,tas2770.yaml | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/Documentation/devicetree/bindings/sound/ti,tas2770.yaml b/Documentation/devicetree/bindings/sound/ti,tas2770.yaml > index 8eab98a0f7a25a9c87d2c56fd0635ff8ecee17d0..3eba9bb34a581526f68b6bf2e8437e1f1e03d26f 100644 > --- a/Documentation/devicetree/bindings/sound/ti,tas2770.yaml > +++ b/Documentation/devicetree/bindings/sound/ti,tas2770.yaml > @@ -57,6 +57,18 @@ properties: > - 0 # Rising edge > - 1 # Falling edge > > + ti,sdout-pull-down: > + $ref: /schemas/types.yaml#/definitions/flag > + description: > + If present, SDOUT will be pulled low when not > + transmitting. > + > + ti,sdout-zero-fill: > + $ref: /schemas/types.yaml#/definitions/flag > + description: > + If present, SDOUT will be zero-filled when not > + transmitting. Can't you align this with the other property you added? Or extend the existing TDM properties we have. Rob
On Tue, Mar 4, 2025 at 1:50 PM Rob Herring <robh@kernel.org> wrote: > Can't you align this with the other property you added? Or extend the > existing TDM properties we have. I don't think either option makes sense given the functionality. This chip behaves differently to TAS2764, and instead of using a bitmask to determine which slots to ignore, we only get a single bit to tell the chip whether we want it to fill or pull down *all* inactive slots. The property being a u32 mask therefore does not make sense here. Building the logic off the existing generic TDM slot properties would alter behaviour of existing implementations where zero-fill and pulldown may not be required or even wanted. This may continue to be the case going forward so I'd rather make it an explicit opt-in rather than some unconditional thing we try to turn on heuristically. I gave some thought to flipping these bits if a TDM slot mask is passed to the driver, however it can still be the case that we don't want both zero-fill *and* pulldown active at the same time, or as above some implementations may want neither, so we still need to be able to specify them individually. Regards, James
On Wed, Mar 05, 2025 at 01:19:15AM +0000, James Calligeros wrote: > On Tue, Mar 4, 2025 at 1:50 PM Rob Herring <robh@kernel.org> wrote: > > Can't you align this with the other property you added? Or extend the > > existing TDM properties we have. > > I don't think either option makes sense given the functionality. This chip > behaves differently to TAS2764, and instead of using a bitmask to determine > which slots to ignore, we only get a single bit to tell the chip whether we want > it to fill or pull down *all* inactive slots. The property being a u32 mask > therefore does not make sense here. If there's a single bit control, then that just means there's only 1 valid value for a mask in that case. Or maybe a mask is overkill. What's the usecase for fill or pulldown *some* inactive slots? > Building the logic off the existing generic TDM slot properties would alter > behaviour of existing implementations where zero-fill and pulldown may not be > required or even wanted. This may continue to be the case going forward so I'd > rather make it an explicit opt-in rather than some unconditional thing we try to > turn on heuristically. Existing implementations would not have the new/extra properties and would continue to operate as before. Or those drivers could simply ignore the properties. > I gave some thought to flipping these bits if a TDM slot mask is passed to the > driver, however it can still be the case that we don't want both zero-fill *and* > pulldown active at the same time, or as above some implementations may want > neither, so we still need to be able to specify them individually. This just feels like something common because any TDM interface may need to control this. It's not really a property of the chip, but requirement of the TDM interface. Rob
On Wed, Mar 5, 2025 at 11:22 PM Rob Herring <robh@kernel.org> wrote:
> This just feels like something common because any TDM interface may need
> to control this. It's not really a property of the chip, but requirement
> of the TDM interface.
What I'm imagining then is something like:
dai-link@0 {
cpu {
sound-dai = <&some_cpu>;
};
codec {
sound-dai = <&some_codec>;
dai-tdm-tx-zerofill;
dai-tdm-tx-pulldown; /* either or, having both makes no sense */
};
};
Codec drivers would then provide a function to set TDM TX behaviour if they
support it, and export that as a dai op for use by machine drivers
when they parse
the dai link similar to dai-tdm-tx-slot and friends. Is that close to
what you have
in mind?
Regards,
James
On Fri, Mar 07, 2025 at 04:18:31PM +1000, James Calligeros wrote:
> On Wed, Mar 5, 2025 at 11:22 PM Rob Herring <robh@kernel.org> wrote:
> > This just feels like something common because any TDM interface may need
> > to control this. It's not really a property of the chip, but requirement
> > of the TDM interface.
>
> What I'm imagining then is something like:
>
> dai-link@0 {
> cpu {
> sound-dai = <&some_cpu>;
> };
> codec {
> sound-dai = <&some_codec>;
> dai-tdm-tx-zerofill;
> dai-tdm-tx-pulldown; /* either or, having both makes no sense */
If they are mutually exclusive, it's best to design the properties that
way. So something like:
dai-tdm-tx-idle = "zerofill";
dai-tdm-tx-idle = "pulldown";
> };
> };
>
> Codec drivers would then provide a function to set TDM TX behaviour if they
> support it, and export that as a dai op for use by machine drivers
> when they parse
> the dai link similar to dai-tdm-tx-slot and friends. Is that close to
> what you have
> in mind?
How would it work when you need a mask? "dai-tdm-slot-tx-mask" is
enough?
Rob
On Sat, Mar 8, 2025 at 6:51 AM Rob Herring <robh@kernel.org> wrote:
> How would it work when you need a mask? "dai-tdm-slot-tx-mask" is
> enough?
The existing TX/RX slot masks are used to control which slots the codec
is operating on, AIUI. I don't know if it makes sense to alter how codecs
deal with this. Could we combine the suggested dai-tdm-slot-tx-idle
with an optional dai-tdm-slot-tx-idle-mask property? From the machine
driver's perspective, the API would then be similar to the existing
set_tdm_slot ops. The current downstream macaudio machine driver builds
its links by allowing multiple codecs and CPUs to be linked to a DAI,
like so:
dai-link@0 {
cpu {
sound-dai = <&cpu0>, <&cpu1>;
};
codec {
sound-dai = <&speaker0>,
...,
<&speaker6>;
};
};
In this case, the codec-specific mask property was added so that a mask
could be applied to a specific codec rather than the whole dai, however
from upstream drivers tt looks like the way this should be handled is to
have "dai-tdm-slot-tx-idle-mask-n" properties at the dai level, then have
the machine driver set the mask for the appropriate codec during setup. So
for macaudio, assuming speaker5 requires this zerofill mask, we would
have something like this:
dai-link@0 {
cpu {
sound-dai = <&cpu0>, <&cpu1>;
};
codec {
sound-dai = <&speaker0>,
...,
<&speaker6>;
};
/* equivalent to ti,sdout-force-zero-mask = <0xf0f0f0> */
dai-tdm-slot-tx-idle-mode-5 = "zerofill";
/* should this be an array like the slot masks? */
dai-tdm-slot-tx-idle-mask-5 = <0xf0f0f0>;
};
The machine driver then calls something like
snd_soc_dai_set_tdm_idle(speaker5_dai, SND_SOC_TDM_IDLE_MODE_ZEROFILL,
SND_SOC_TDM_IDLE_MODE_NONE, tx_idle_mask, 0);
and the codec driver deals with it however it needs to.
Regards,
James
On Mon, Mar 10, 2025 at 07:30:07PM +1000, James Calligeros wrote:
> On Sat, Mar 8, 2025 at 6:51 AM Rob Herring <robh@kernel.org> wrote:
> > How would it work when you need a mask? "dai-tdm-slot-tx-mask" is
> > enough?
>
> The existing TX/RX slot masks are used to control which slots the codec
> is operating on, AIUI. I don't know if it makes sense to alter how codecs
> deal with this. Could we combine the suggested dai-tdm-slot-tx-idle
> with an optional dai-tdm-slot-tx-idle-mask property? From the machine
> driver's perspective, the API would then be similar to the existing
> set_tdm_slot ops. The current downstream macaudio machine driver builds
> its links by allowing multiple codecs and CPUs to be linked to a DAI,
> like so:
Wouldn't the NOT of dai-tdm-slot-tx-mask be the idle mask?
Don't think about the Linux APIs here. The DT is separate. So think in
terms of what you need to describe the TDM timing/waveform.
>
> dai-link@0 {
> cpu {
> sound-dai = <&cpu0>, <&cpu1>;
> };
> codec {
> sound-dai = <&speaker0>,
> ...,
> <&speaker6>;
> };
> };
>
> In this case, the codec-specific mask property was added so that a mask
> could be applied to a specific codec rather than the whole dai, however
> from upstream drivers tt looks like the way this should be handled is to
> have "dai-tdm-slot-tx-idle-mask-n" properties at the dai level, then have
> the machine driver set the mask for the appropriate codec during setup. So
> for macaudio, assuming speaker5 requires this zerofill mask, we would
> have something like this:
I'm now confused why you need n masks and what does n represent?
Rob
Hi Rob, James,
> On 12. 3. 2025, at 13:58, Rob Herring <robh@kernel.org> wrote:
>
> On Mon, Mar 10, 2025 at 07:30:07PM +1000, James Calligeros wrote:
>> On Sat, Mar 8, 2025 at 6:51 AM Rob Herring <robh@kernel.org> wrote:
>>> How would it work when you need a mask? "dai-tdm-slot-tx-mask" is
>>> enough?
>>
>> The existing TX/RX slot masks are used to control which slots the codec
>> is operating on, AIUI. I don't know if it makes sense to alter how codecs
>> deal with this. Could we combine the suggested dai-tdm-slot-tx-idle
>> with an optional dai-tdm-slot-tx-idle-mask property? From the machine
>> driver's perspective, the API would then be similar to the existing
>> set_tdm_slot ops. The current downstream macaudio machine driver builds
>> its links by allowing multiple codecs and CPUs to be linked to a DAI,
>> like so:
>
> Wouldn't the NOT of dai-tdm-slot-tx-mask be the idle mask?
>
> Don't think about the Linux APIs here. The DT is separate. So think in
> terms of what you need to describe the TDM timing/waveform.
>
>>
>> dai-link@0 {
>> cpu {
>> sound-dai = <&cpu0>, <&cpu1>;
>> };
>> codec {
>> sound-dai = <&speaker0>,
>> ...,
>> <&speaker6>;
>> };
>> };
>>
>> In this case, the codec-specific mask property was added so that a mask
>> could be applied to a specific codec rather than the whole dai, however
>> from upstream drivers tt looks like the way this should be handled is to
>> have "dai-tdm-slot-tx-idle-mask-n" properties at the dai level, then have
>> the machine driver set the mask for the appropriate codec during setup. So
>> for macaudio, assuming speaker5 requires this zerofill mask, we would
>> have something like this:
>
> I'm now confused why you need n masks and what does n represent?
For this setup there are 6 codecs on the same bus but 3 of those are on one
data line and the other 3 are on another data line. Within the SoC these two
data lines (both for codec->SoC direction) are ORed together in front of the
receiver peripheral.
This means we need at least one codec within each group of the three to zero
out the bus for the duration of the slots used by the other group of three.
I solved this by attaching
ti,sdout-force-zero-mask = <0xf0f0f0>;
on one codec from the first group, and
ti,sdout-force-zero-mask = <0x0f0f0f>;
on one from the other group.
FWIW the right form of these masks is not an implementation detail of the
machine driver spilling into the device tree (so that the mask would need
to be different if the machine driver was implemented differently). This is
because the slots used by each codec are specified via a DT property too, e.g.
ti,imon-slot-no = <8>;
ti,vmon-slot-no = <10>;
Martin
> Rob
>
On Wed, Mar 12, 2025 at 10:58 PM Rob Herring <robh@kernel.org> wrote:
>
> On Mon, Mar 10, 2025 at 07:30:07PM +1000, James Calligeros wrote:
> > On Sat, Mar 8, 2025 at 6:51 AM Rob Herring <robh@kernel.org> wrote:
> > > How would it work when you need a mask? "dai-tdm-slot-tx-mask" is
> > > enough?
> >
> > The existing TX/RX slot masks are used to control which slots the codec
> > is operating on, AIUI. I don't know if it makes sense to alter how codecs
> > deal with this. Could we combine the suggested dai-tdm-slot-tx-idle
> > with an optional dai-tdm-slot-tx-idle-mask property? From the machine
> > driver's perspective, the API would then be similar to the existing
> > set_tdm_slot ops. The current downstream macaudio machine driver builds
> > its links by allowing multiple codecs and CPUs to be linked to a DAI,
> > like so:
>
> Wouldn't the NOT of dai-tdm-slot-tx-mask be the idle mask?
Theoretically it should be, and that's probably just what we should do.
We would then just have the dai-tdm-slot-tx-idle-mode property to worry
about. There may be a reason a unique property was added however, as only
some codecs have it set in our downstream DTs. Perhaps Martin can shed
some light on this?
> >
> > dai-link@0 {
> > cpu {
> > sound-dai = <&cpu0>, <&cpu1>;
> > };
> > codec {
> > sound-dai = <&speaker0>,
> > ...,
> > <&speaker6>;
> > };
> > };
> >
> > In this case, the codec-specific mask property was added so that a mask
> > could be applied to a specific codec rather than the whole dai, however
> > from upstream drivers tt looks like the way this should be handled is to
> > have "dai-tdm-slot-tx-idle-mask-n" properties at the dai level, then have
> > the machine driver set the mask for the appropriate codec during setup. So
> > for macaudio, assuming speaker5 requires this zerofill mask, we would
> > have something like this:
>
> I'm now confused why you need n masks and what does n represent?
We can have n cpus linked to m codecs in macaudio, and we need to specify
the TDM properties for each codec individually . There seem to be a couple
of ways upstream drivers deal with this, but the "nicest" way I've seen is
what amlogic[1] does, which is extend the dai-tdm-slot-* properties with
an index (-n) representing the specific codec it's for.
Regards,
James
[1] https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/sound/amlogic%2Caxg-sound-card.yaml
© 2016 - 2025 Red Hat, Inc.