Re: [PATCH] ASoC: fsl_ssi: Remove unneeded 'i2s-slave' property

Re: [PATCH] ASoC: fsl_ssi: Remove unneeded 'i2s-slave' property

[Fabio.Estevam]

Sorry, I missed to add alsa-devel@alsa-project.org on Cc.
________________________________________
From: Timur Tabi <timur@tabi.org>
Sent: Thursday, September 18, 2014 3:37 PM
To: Nicolin Chen
Cc: Estevam Fabio-R49496; broonie@kernel.org; Xiubo Li-B47053
Subject: Re: [PATCH] ASoC: fsl_ssi: Remove unneeded 'i2s-slave' property

On 09/18/2014 01:32 PM, Nicolin Chen wrote:
> On Thu, Sep 18, 2014 at 03:11:17PM -0300, Fabio Estevam wrote:
>> There is no need to use 'i2s-slave' property, since master/slave configuration
>> are passed via machine layer.
>>
>> This change does not break existing users because they do check for slave
>> mode inside sound/soc/fsl/mpc8610_hpcd.c/p1022_ds.c and p1022_rdk.c
>
> Besides this change, I'm wondering if we can take a further step to drop
> this entire property from Device Tree binding document as newer platforms
> may no long need this property in their DT bindings.

I think the property is necessary if anyone takes a P1022 or MPC8610 and
uses it in a platform that doesn't support i2s-slave.  But currently,
the code doesn't support anything else.  So technically, we could remove
it, and if anyone does want to support another configuration, he will
have to add that support the "new" way.

Re: [PATCH] ASoC: fsl_ssi: Remove unneeded 'i2s-slave' property

[Nicolin Chen]

On Thu, Sep 18, 2014 at 06:46:42PM +0000, Fabio.Estevam@freescale.com wrote:
> On 09/18/2014 01:32 PM, Nicolin Chen wrote:
> > On Thu, Sep 18, 2014 at 03:11:17PM -0300, Fabio Estevam wrote:
> >> There is no need to use 'i2s-slave' property, since master/slave configuration
> >> are passed via machine layer.
> >>
> >> This change does not break existing users because they do check for slave
> >> mode inside sound/soc/fsl/mpc8610_hpcd.c/p1022_ds.c and p1022_rdk.c
> >
> > Besides this change, I'm wondering if we can take a further step to drop
> > this entire property from Device Tree binding document as newer platforms
> > may no long need this property in their DT bindings.
> 
> I think the property is necessary if anyone takes a P1022 or MPC8610 and
> uses it in a platform that doesn't support i2s-slave.  But currently,
> the code doesn't support anything else.  So technically, we could remove
> it, and if anyone does want to support another configuration, he will
> have to add that support the "new" way.

Thank you for the comments. Then we may use compatible check for
P1022 and MPC8610 to limit their DAI formats.

For the property, do you agree to modify it like this:

- fsl,mode:         The operating mode for the AC97 interface only.
                    "ac97-slave" - AC97 mode, SSI is clock slave
                    "ac97-master" - AC97 mode, SSI is clock master
                    (There's no need for other DAI format like I2S
		    mode to engage this property.)

And how about this patch? I'm personally okay with this change.
We may also send an extra patch to refine the document after the
agreement's done.

Thanks
Nicolin

Re: [PATCH] ASoC: fsl_ssi: Remove unneeded 'i2s-slave' property

[Timur Tabi]

Nicolin Chen wrote:
> Thank you for the comments. Then we may use compatible check for
> P1022 and MPC8610 to limit their DAI formats.
>
> For the property, do you agree to modify it like this:
>
> - fsl,mode:         The operating mode for the AC97 interface only.
>                      "ac97-slave" - AC97 mode, SSI is clock slave
>                      "ac97-master" - AC97 mode, SSI is clock master
>                      (There's no need for other DAI format like I2S
> 		    mode to engage this property.)
>
> And how about this patch? I'm personally okay with this change.
> We may also send an extra patch to refine the document after the
> agreement's done.

You can't just redefine a property, because that breaks backwards 
compatibility with older device trees or older kernels.

Re: [PATCH] ASoC: fsl_ssi: Remove unneeded 'i2s-slave' property

[Nicolin Chen]

On Thu, Sep 18, 2014 at 07:29:32PM -0500, Timur Tabi wrote:
> >For the property, do you agree to modify it like this:
> >
> >- fsl,mode:         The operating mode for the AC97 interface only.
> >                     "ac97-slave" - AC97 mode, SSI is clock slave
> >                     "ac97-master" - AC97 mode, SSI is clock master
> >                     (There's no need for other DAI format like I2S
> >		    mode to engage this property.)

> You can't just redefine a property, because that breaks backwards
> compatibility with older device trees or older kernels.

Sorry, I'm not sure what's the problem here. It just drops i2s stuff
and leave AC97 part as we discussed. Why would old DTs be broken?
Is that possible for you to give me an example to understand the
point?

Thank you
Nicolin

Re: [PATCH] ASoC: fsl_ssi: Remove unneeded 'i2s-slave' property

[Timur Tabi]

Nicolin Chen wrote:
>> You can't just redefine a property, because that breaks backwards
>> >compatibility with older device trees or older kernels.

> Sorry, I'm not sure what's the problem here. It just drops i2s stuff
> and leave AC97 part as we discussed. Why would old DTs be broken?
> Is that possible for you to give me an example to understand the
> point?

If you have an older kernel with a newer device tree (that doesn't have 
"i2s-mode"), the driver will fail to load because "i2s-mode" is missing.

Re: [PATCH] ASoC: fsl_ssi: Remove unneeded 'i2s-slave' property

[Nicolin Chen]

On Fri, Sep 19, 2014 at 07:51:46AM -0500, Timur Tabi wrote:
> Nicolin Chen wrote:
> >>You can't just redefine a property, because that breaks backwards
> >>>compatibility with older device trees or older kernels.
> 
> >Sorry, I'm not sure what's the problem here. It just drops i2s stuff
> >and leave AC97 part as we discussed. Why would old DTs be broken?
> >Is that possible for you to give me an example to understand the
> >point?
> 
> If you have an older kernel with a newer device tree (that doesn't have
> "i2s-mode"), the driver will fail to load because "i2s-mode" is missing.

I thought only the backward compatibility for DT is demanded.....
But I still don't get why people would use new DT without updating
kernel. It's pretty fair to use old DT with newer kernel because
they want bug-fix patches.

Thank you
Nicolin

Re: [PATCH] ASoC: fsl_ssi: Remove unneeded 'i2s-slave' property

[Timur Tabi]

Nicolin Chen wrote:
> I thought only the backward compatibility for DT is demanded.....

I have higher standards :-)

> But I still don't get why people would use new DT without updating
> kernel. It's pretty fair to use old DT with newer kernel because
> they want bug-fix patches.

The device tree is shipped with the kernel, so some people say that 
backwards compatibility is not needed anywhere.  I think it's important 
to provide backwards compatibility both ways.

I generally prefer some kind of transition period, where you can use new 
or old kernel with a new or old device tree.

However, let me look at Fabio's patches some more.

Re: [PATCH] ASoC: fsl_ssi: Remove unneeded 'i2s-slave' property

[Nicolin Chen]

On Fri, Sep 19, 2014 at 06:49:18PM -0500, Timur Tabi wrote:
> Nicolin Chen wrote:
> >I thought only the backward compatibility for DT is demanded.....
> 
> I have higher standards :-)
> 
> >But I still don't get why people would use new DT without updating
> >kernel. It's pretty fair to use old DT with newer kernel because
> >they want bug-fix patches.
> 
> The device tree is shipped with the kernel, so some people say that
> backwards compatibility is not needed anywhere.  I think it's important to
> provide backwards compatibility both ways.
> 
> I generally prefer some kind of transition period, where you can use new or
> old kernel with a new or old device tree.

I see..

> However, let me look at Fabio's patches some more.

Yes, please.