Re: [RFC][PATCH 1/2] ASoC: add snd_soc_of_parse_daifmt()

[Stephen Warren <swarren@wwwdotorg.org>]

On 11/29/2012 05:35 PM, Kuninori Morimoto wrote:
> 
> Hi Stephen
> 
>> I think it'd be more typical to represent as a single integer property,
>> where the value is an enumeration indicating the type.
> 
> Sorry, I couldn't understand correctly this.
> Do you mean like this ?
>
>   snd.soc.daifmt.format      = <3> /* SND_SOC_DAIFMT_LEFT_J */
>   snd.soc.daifmt.clock_gate  = <1> /* SND_SOC_DAIFMT_CONT */
>   snd.soc.daifmt.inversion   = <3> /* SND_SOC_DAIFMT_IB_NF */
>   snd.soc.daifmt.hw_clock    = <2> /* SND_SOC_DAIFMT_CBS_CFM */

Sorry for the slow response.

What I meant was that rather than:

> +	ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt,
> +					 "i2s", SND_SOC_DAIFMT_I2S);
> +	ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt,
> +					 "right_j", SND_SOC_DAIFMT_RIGHT_J);
> +	ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt,
> +					 "left_j", SND_SOC_DAIFMT_LEFT_J);
> +	ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt,
> +					 "dsp_a", SND_SOC_DAIFMT_DSP_A);
> +	ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt,
> +					 "dsp_b", SND_SOC_DAIFMT_DSP_B);

I'd expect to see something more like:

fmt = of_property_read_u32_array(np, "bit-format");

But perhaps your binding is representing the set of possible legal
formats? I had assumed it was attempting to represent the single
specific format to choose.

Related, I also wasn't suggesting combining format, clock-gate,
inversion, ... into a single property.

> I added SND_SOC_DAIFMT_xxx here,
> but this style is not readable/understandable for me.
> And it is difficult to keep compatible if the number was changed.
> (never happen ? I'm not sure)

Well, once a DT binding is created, you can't change the numbers, or you
would break the ability for an old DT to work with a newer kernel.

> How about to use string ?
> 
>   snd.soc.daifmt.format      = "left_j"
>   snd.soc.daifmt.clock_gate  = "cont"
>   snd.soc.daifmt.inversion   = "ib_nf"
>   snd.soc.daifmt.hw_clock    = "cbs_cfm"

That's probably the best we can do for now. Using a pre-processor would
be best:

#define SND_SOC_DAIFMT_LEFT_J 3

snd.soc.daifmt.format = <SND_SOC_DAIFMT_LEFT_J>;

... but we can't do that yet...