| Message ID | 20250227-apple-codec-changes-v3-17-cbb130030acf@gmail.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | ASoC: tas27{64,70}: improve support for Apple codec variants | expand |
On 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
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
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.
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(+)