Re: [RFC V2 1/2] ASoC: img: Add binding document for Pistachio audio card

Re: [RFC V2 1/2] ASoC: img: Add binding document for Pistachio audio card

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 647 bytes --]

On Tue, Jan 26, 2016 at 02:34:26PM +0000, Damien Horsley wrote:

> +  - clock-names : Includes the following entries:
> +        "audio_pll"  The audio PLL
> +        "i2s_mclk"   The i2s reference clock
> +                     Also connected to i2s_out_0_mclk output
> +        "dac_mclk"   Dac reference clock. Connected to i2s_dac_clk output

Why are these (especially the dac_mclk and i2s_mclk) properties of the
card and not of the drivers for the respective devices?

> +  - img,mute-gpio : phandle of the mute gpio
> +
> +  - img,hp-det-gpio : phandle of the headphone detect gpio

The DT maintainers would prefer those to be named -gpios.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [RFC V2 1/2] ASoC: img: Add binding document for Pistachio audio card

[Damien Horsley]

On 27/01/16 14:57, Mark Brown wrote:
> On Tue, Jan 26, 2016 at 02:34:26PM +0000, Damien Horsley wrote:
> 
>> +  - clock-names : Includes the following entries:
>> +        "audio_pll"  The audio PLL
>> +        "i2s_mclk"   The i2s reference clock
>> +                     Also connected to i2s_out_0_mclk output
>> +        "dac_mclk"   Dac reference clock. Connected to i2s_dac_clk output
> 
> Why are these (especially the dac_mclk and i2s_mclk) properties of the
> card and not of the drivers for the respective devices?
> 

Due to the shared nature of these clocks. Individual components cannot
be responsible for controlling these as this could break configurations
for other components that are sharing the clocks. Only the card driver
has visibility of all of the components and their requirements.

audio_pll is used by spdif out, parallel out, i2s out, and i2s in if
there are codecs on the i2s in path that make use of i2s_mclk and
dac_mclk (derived from audio_pll)

i2s_mclk and dac_mclk can be used by both the i2s in and i2s out paths
on some boards

>> +  - img,mute-gpio : phandle of the mute gpio
>> +
>> +  - img,hp-det-gpio : phandle of the headphone detect gpio
> 
> The DT maintainers would prefer those to be named -gpios.
> 

Ok

Re: [RFC V2 1/2] ASoC: img: Add binding document for Pistachio audio card

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1152 bytes --]

On Wed, Jan 27, 2016 at 03:18:20PM +0000, Damien Horsley wrote:
> On 27/01/16 14:57, Mark Brown wrote:
> > On Tue, Jan 26, 2016 at 02:34:26PM +0000, Damien Horsley wrote:

> >> +  - clock-names : Includes the following entries:
> >> +        "audio_pll"  The audio PLL
> >> +        "i2s_mclk"   The i2s reference clock
> >> +                     Also connected to i2s_out_0_mclk output
> >> +        "dac_mclk"   Dac reference clock. Connected to i2s_dac_clk output

> > Why are these (especially the dac_mclk and i2s_mclk) properties of the
> > card and not of the drivers for the respective devices?

> Due to the shared nature of these clocks. Individual components cannot
> be responsible for controlling these as this could break configurations
> for other components that are sharing the clocks. Only the card driver
> has visibility of all of the components and their requirements.

You're talking about the code that decides what rates to set the clock
at, not where the properties are placed in the DT.

> i2s_mclk and dac_mclk can be used by both the i2s in and i2s out paths
> on some boards

Multiple devices can reference the same clock.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [RFC V2 1/2] ASoC: img: Add binding document for Pistachio audio card

[Damien Horsley]

On 27/01/16 16:00, Mark Brown wrote:
> On Wed, Jan 27, 2016 at 03:18:20PM +0000, Damien Horsley wrote:
>> On 27/01/16 14:57, Mark Brown wrote:
>>> On Tue, Jan 26, 2016 at 02:34:26PM +0000, Damien Horsley wrote:
> 
>>>> +  - clock-names : Includes the following entries:
>>>> +        "audio_pll"  The audio PLL
>>>> +        "i2s_mclk"   The i2s reference clock
>>>> +                     Also connected to i2s_out_0_mclk output
>>>> +        "dac_mclk"   Dac reference clock. Connected to i2s_dac_clk output
> 
>>> Why are these (especially the dac_mclk and i2s_mclk) properties of the
>>> card and not of the drivers for the respective devices?
> 
>> Due to the shared nature of these clocks. Individual components cannot
>> be responsible for controlling these as this could break configurations
>> for other components that are sharing the clocks. Only the card driver
>> has visibility of all of the components and their requirements.
> 
> You're talking about the code that decides what rates to set the clock
> at, not where the properties are placed in the DT.
> 
>> i2s_mclk and dac_mclk can be used by both the i2s in and i2s out paths
>> on some boards
> 
> Multiple devices can reference the same clock.
> 

audio_pll is referenced exclusively by the card device

i2s_mclk and dac_mclk can also be referenced by other devices. The i2s
out controller references i2s_mclk, and codec devices can reference
i2s_mclk/dac_mclk dependent on their system clock requirements

without a reference to i2s_mclk and dac_mclk in the card driver, it
would not be possible to control the divisors and gates for these clocks
in the following cases:

Simplistic codecs that do not have drivers

Codec drivers that do not call clk_set_rate and clk_enable/clk_disable

Re: [RFC V2 1/2] ASoC: img: Add binding document for Pistachio audio card

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1042 bytes --]

On Wed, Jan 27, 2016 at 05:13:09PM +0000, Damien Horsley wrote:

> audio_pll is referenced exclusively by the card device

That one *may* be plausible.

> i2s_mclk and dac_mclk can also be referenced by other devices. The i2s
> out controller references i2s_mclk, and codec devices can reference
> i2s_mclk/dac_mclk dependent on their system clock requirements

The clock API copes perfectly happily with this.

> without a reference to i2s_mclk and dac_mclk in the card driver, it
> would not be possible to control the divisors and gates for these clocks
> in the following cases:

> Simplistic codecs that do not have drivers

> Codec drivers that do not call clk_set_rate and clk_enable/clk_disable

Nonsense, if there is no driver or the driver doesn't do what you want
then fix that.  Don't bodge things at the wrong abstraction layer, that
just creates problems later on when someone comes along and does things
properly or tries to use the device tree outside of your particular
implementation (eg, when working with a differnet OS).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [RFC V2 1/2] ASoC: img: Add binding document for Pistachio audio card

[Damien Horsley]

On 27/01/16 20:14, Mark Brown wrote:
> On Wed, Jan 27, 2016 at 05:13:09PM +0000, Damien Horsley wrote:
> 
>> audio_pll is referenced exclusively by the card device
> 
> That one *may* be plausible.
> 
>> i2s_mclk and dac_mclk can also be referenced by other devices. The i2s
>> out controller references i2s_mclk, and codec devices can reference
>> i2s_mclk/dac_mclk dependent on their system clock requirements
> 
> The clock API copes perfectly happily with this.
> 
>> without a reference to i2s_mclk and dac_mclk in the card driver, it
>> would not be possible to control the divisors and gates for these clocks
>> in the following cases:
> 
>> Simplistic codecs that do not have drivers
> 
>> Codec drivers that do not call clk_set_rate and clk_enable/clk_disable
> 
> Nonsense, if there is no driver or the driver doesn't do what you want
> then fix that.  Don't bodge things at the wrong abstraction layer, that
> just creates problems later on when someone comes along and does things
> properly or tries to use the device tree outside of your particular
> implementation (eg, when working with a differnet OS).
> 

Okay. If it can be expected that the codec driver will call these then
the references will not be required in the card driver for the codecs.

The i2s out controller also uses i2s_mclk, and calls clk_set_rate in
hw_params at present. Would creating a set_sysclk callback for the i2s
out controller, then calling snd_soc_dai_set_sysclk on the cpu dai in
addition to the codec dais in the card driver be the correct way to
manage this?

Re: [RFC V2 1/2] ASoC: img: Add binding document for Pistachio audio card

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 395 bytes --]

On Thu, Jan 28, 2016 at 02:17:37PM +0000, Damien Horsley wrote:

> The i2s out controller also uses i2s_mclk, and calls clk_set_rate in
> hw_params at present. Would creating a set_sysclk callback for the i2s
> out controller, then calling snd_soc_dai_set_sysclk on the cpu dai in
> addition to the codec dais in the card driver be the correct way to
> manage this?

That'd be more normal, yes.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]