Re: [PATCH v2] ASoC: pcm3168a: Add option to force clock consumer

[Stephen Gordon <gordoste@xxxxxxxxxxxx>]

On 6/12/2024 11:22 pm, Mark Brown wrote:
> On Fri, Dec 06, 2024 at 10:54:43PM +1100, Stephen Gordon wrote:
>
> > Scenario is that DAC's clock pins are tied to ADC's clock pins, with
> > codec acting as clock producer for the I2S bus, and the CPU has a single
> > pair of I2S clock pins used for both playback and capture.
> >                                   ,-------------------
> > ---------            ------------| LRCK     DAC
> >       LRCK|-------+--/     -------| BCK   (producer)  C
> > CPU      |       |       /       |                   O
> >        BCK|-------|---+--/        | ----------------  D
> > consumer |       \   |           |                   E
> > ---------         ---------------| LRCK     ADC      C
> >                       `-----------| BCK    (consumer)
> >                                   `--------------------
> > Both DAI links will have codec acting as a clock producer, but we need
> >
> > to configure the codec so that only one of ADC/DAC drives the lines.
> This looks like something that should be done in the board
> configuration, not hard coded in the CODEC driver.
>
> > Add DT options to support this configuration. adc-force-cons/dac-force-cons
> >
> > cause the corresponding component to be configured in consumer mode.
> Any DT bindings should documented which should be done in a separate
> patch.

I will most certainly do this in v3 if I convince you that the DT 
bindings should be

added in the first place :)

> > +       // Force clock consumer mode if needed
> > +       if (pcm3168a->adc_fc && dai->id == PCM3168A_DAI_ADC)
> > +               ms = 0;
> > +       if (pcm3168a->dac_fc && dai->id == PCM3168A_DAI_DAC)
> > +               ms = 0;
> The clock consumer mode should just be configured via the standard
> set_dai_fmt() operation.

I did try to do this using simple_card and I documented my attempt and 
why it seems to

fail in this message:

https://lore.kernel.org/linux-sound/b64a630f-d71f-49ee-a5e7-ea1e3a7f8de5@xxxxxxxxxxxx/

Basically, simple_card appears to set the CPU as producer if you don't 
specify a

producer. I am not sure whether this is a bug.

Also, there aren't any examples I could find in the documentation of DAI 
links with

clock consumer on both ends, so I was wondering if it is even valid.

There is some SoC code that seems like it might not work properly in 
this scenario:

https://github.com/torvalds/linux/blob/b8f52214c61a5b99a54168145378e91b40d10c90/sound/soc/soc-core.c#L1465


> > +       if (dev->of_node) {
> > +               pcm3168a->adc_fc = of_property_read_bool(dev->of_node,
> > +                               "adc-force-cons");
> > +               pcm3168a->dac_fc = of_property_read_bool(dev->of_node,
> > +                               "dac-force-cons");
> > +       }
> These properties are not documented, and you don't need to query to see
> if there's an OF node to query if a property is present.