[PATCHv2] ASoC: fsl-sai: using 'lsb-first' property instead of 'big-endian-data'.

[PATCHv2] ASoC: fsl-sai: using 'lsb-first' property instead of 'big-endian-data'.

[Xiubo Li]

The 'big-endian-data' property is originally used to indicate whether the
LSB firstly or MSB firstly will be transmitted to the CODEC or received
from the CODEC, and there has nothing relation to the memory data.

Generally, if the audio data in big endian format, which will be using the
bytes reversion, Here this can only be used to bits reversion.

So using the 'lsb-first' instead of 'big-endian-data' can make the code
to be readable easier and more easy to understand what this property is
used to do.

This property used for configuring whether the LSB or the MSB is transmitted
first for the fifo data.

Signed-off-by: Xiubo Li <Li.Xiubo-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
Cc: Nicolin Chen <nicoleotsuka-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 Documentation/devicetree/bindings/sound/fsl-sai.txt | 8 ++++----
 sound/soc/fsl/fsl_sai.c                             | 6 +++---
 sound/soc/fsl/fsl_sai.h                             | 2 +-
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/Documentation/devicetree/bindings/sound/fsl-sai.txt b/Documentation/devicetree/bindings/sound/fsl-sai.txt
index 06a405e..4956b14 100644
--- a/Documentation/devicetree/bindings/sound/fsl-sai.txt
+++ b/Documentation/devicetree/bindings/sound/fsl-sai.txt
@@ -20,9 +20,9 @@ Required properties:
   See ../pinctrl/pinctrl-bindings.txt for details of the property values.
 - big-endian: Boolean property, required if all the FTM_PWM registers
   are big-endian rather than little-endian.
-- big-endian-data: If this property is absent, the little endian mode will
-  be in use as default, or the big endian mode will be in use for all the
-  fifo data.
+- lsb-first: Configures whether the LSB or the MSB is transmitted first for
+  the fifo data. If this property is absent, the MSB is transmitted first as
+  default, or the LSB is transmitted first.
 - fsl,sai-synchronous-rx: This is a boolean property. If present, indicating
   that SAI will work in the synchronous mode (sync Tx with Rx) which means
   both the transimitter and receiver will send and receive data by following
@@ -53,5 +53,5 @@ sai2: sai@40031000 {
 	      dmas = <&edma0 0 VF610_EDMA_MUXID0_SAI2_TX>,
 		   <&edma0 0 VF610_EDMA_MUXID0_SAI2_RX>;
 	      big-endian;
-	      big-endian-data;
+	      lsb-first;
 };
diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c
index a6eb784..7eeb1dd 100644
--- a/sound/soc/fsl/fsl_sai.c
+++ b/sound/soc/fsl/fsl_sai.c
@@ -175,7 +175,7 @@ static int fsl_sai_set_dai_fmt_tr(struct snd_soc_dai *cpu_dai,
 	bool tx = fsl_dir == FSL_FMT_TRANSMITTER;
 	u32 val_cr2 = 0, val_cr4 = 0;
 
-	if (!sai->big_endian_data)
+	if (!sai->is_lsb_first)
 		val_cr4 |= FSL_SAI_CR4_MF;
 
 	/* DAI mode */
@@ -304,7 +304,7 @@ static int fsl_sai_hw_params(struct snd_pcm_substream *substream,
 	val_cr5 |= FSL_SAI_CR5_WNW(word_width);
 	val_cr5 |= FSL_SAI_CR5_W0W(word_width);
 
-	if (sai->big_endian_data)
+	if (sai->is_lsb_first)
 		val_cr5 |= FSL_SAI_CR5_FBT(0);
 	else
 		val_cr5 |= FSL_SAI_CR5_FBT(word_width - 1);
@@ -573,7 +573,7 @@ static int fsl_sai_probe(struct platform_device *pdev)
 	if (of_device_is_compatible(pdev->dev.of_node, "fsl,imx6sx-sai"))
 		sai->sai_on_imx = true;
 
-	sai->big_endian_data = of_property_read_bool(np, "big-endian-data");
+	sai->is_lsb_first = of_property_read_bool(np, "lsb-first");
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	base = devm_ioremap_resource(&pdev->dev, res);
diff --git a/sound/soc/fsl/fsl_sai.h b/sound/soc/fsl/fsl_sai.h
index 2cded44..3466720 100644
--- a/sound/soc/fsl/fsl_sai.h
+++ b/sound/soc/fsl/fsl_sai.h
@@ -132,7 +132,7 @@ struct fsl_sai {
 	struct clk *bus_clk;
 	struct clk *mclk_clk[FSL_SAI_MCLK_MAX];
 
-	bool big_endian_data;
+	bool is_lsb_first;
 	bool is_dsp_mode;
 	bool sai_on_imx;
 	bool synchronous[2];
-- 
1.8.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2] ASoC: fsl-sai: using 'lsb-first' property instead of 'big-endian-data'.

[Mark Rutland]

On Fri, Aug 29, 2014 at 08:12:12AM +0100, Xiubo Li wrote:
> The 'big-endian-data' property is originally used to indicate whether the
> LSB firstly or MSB firstly will be transmitted to the CODEC or received
> from the CODEC, and there has nothing relation to the memory data.
> 
> Generally, if the audio data in big endian format, which will be using the
> bytes reversion, Here this can only be used to bits reversion.
> 
> So using the 'lsb-first' instead of 'big-endian-data' can make the code
> to be readable easier and more easy to understand what this property is
> used to do.
> 
> This property used for configuring whether the LSB or the MSB is transmitted
> first for the fifo data.
> 
> Signed-off-by: Xiubo Li <Li.Xiubo-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> Cc: Nicolin Chen <nicoleotsuka-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/sound/fsl-sai.txt | 8 ++++----
>  sound/soc/fsl/fsl_sai.c                             | 6 +++---
>  sound/soc/fsl/fsl_sai.h                             | 2 +-
>  3 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/sound/fsl-sai.txt b/Documentation/devicetree/bindings/sound/fsl-sai.txt
> index 06a405e..4956b14 100644
> --- a/Documentation/devicetree/bindings/sound/fsl-sai.txt
> +++ b/Documentation/devicetree/bindings/sound/fsl-sai.txt
> @@ -20,9 +20,9 @@ Required properties:
>    See ../pinctrl/pinctrl-bindings.txt for details of the property values.
>  - big-endian: Boolean property, required if all the FTM_PWM registers
>    are big-endian rather than little-endian.
> -- big-endian-data: If this property is absent, the little endian mode will
> -  be in use as default, or the big endian mode will be in use for all the
> -  fifo data.
> +- lsb-first: Configures whether the LSB or the MSB is transmitted first for
> +  the fifo data. If this property is absent, the MSB is transmitted first as
> +  default, or the LSB is transmitted first.

Both Nicolin [1] and Mark [2] asked for the old property to be kept
around for compatibility reasons. Those requests seem to have been
ignored entirely; the commit message doesn't even describe why you
believe removing support for the old property is safe.

So NAK unless this patch is updated to either:

- Keep support for the old property, but mark it as deprecated in the
  documentation (having a warning if said property is used is also
  fine).

- Describe why it is safe to remove the property outright, and both Mark
  and Nicolin agree that this is fine.

The former is the preferred way of doing things, and the latter should
be the exceptional case. We don't even save much here my dropping
support for the old property.

I have no proeprty with addition of the the new property; the rationale
for the new naming makes sense to me.

Thanks,
Mark.

[1] http://lkml.kernel.org/r/20140827070544.GO17528-GFdadSzt00ze9xe1eoZjHA@public.gmane.org
[2] http://lkml.kernel.org/r/CAGoOwPSTJaT0UA4yaYR0x49C8UHbmKAR8PzUfV9ZWVW5XBfV5w-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2] ASoC: fsl-sai: using 'lsb-first' property instead of 'big-endian-data'.

[Mark Brown]

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

On Fri, Aug 29, 2014 at 12:06:01PM +0100, Mark Rutland wrote:
> On Fri, Aug 29, 2014 at 08:12:12AM +0100, Xiubo Li wrote:

> > The 'big-endian-data' property is originally used to indicate whether the
> > LSB firstly or MSB firstly will be transmitted to the CODEC or received
> > from the CODEC, and there has nothing relation to the memory data.

> Both Nicolin [1] and Mark [2] asked for the old property to be kept
> around for compatibility reasons. Those requests seem to have been
> ignored entirely; the commit message doesn't even describe why you
> believe removing support for the old property is safe.

The response was that the relevant chips aren't out in the wild so it's
vanishingly unlikely that anyone would have a real DT with this option
enabled.  I'll defer to Nicolin on if that's the case or not.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCHv2] ASoC: fsl-sai: using 'lsb-first' property instead of 'big-endian-data'.

[Nicolin Chen]

On Fri, Aug 29, 2014 at 12:59:43PM +0100, Mark Brown wrote:
> On Fri, Aug 29, 2014 at 12:06:01PM +0100, Mark Rutland wrote:
> > On Fri, Aug 29, 2014 at 08:12:12AM +0100, Xiubo Li wrote:
> 
> > > The 'big-endian-data' property is originally used to indicate whether the
> > > LSB firstly or MSB firstly will be transmitted to the CODEC or received
> > > from the CODEC, and there has nothing relation to the memory data.
> 
> > Both Nicolin [1] and Mark [2] asked for the old property to be kept
> > around for compatibility reasons. Those requests seem to have been
> > ignored entirely; the commit message doesn't even describe why you
> > believe removing support for the old property is safe.
> 
> The response was that the relevant chips aren't out in the wild so it's
> vanishingly unlikely that anyone would have a real DT with this option
> enabled.  I'll defer to Nicolin on if that's the case or not.

Yea, it sounds like so. I thought Vf610 also used this property. But if
only LS1 using it, I think we may consent this rename right?

Nicolin
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2] ASoC: fsl-sai: using 'lsb-first' property instead of 'big-endian-data'.

[Mark Rutland]

On Fri, Aug 29, 2014 at 05:40:11PM +0100, Nicolin Chen wrote:
> On Fri, Aug 29, 2014 at 12:59:43PM +0100, Mark Brown wrote:
> > On Fri, Aug 29, 2014 at 12:06:01PM +0100, Mark Rutland wrote:
> > > On Fri, Aug 29, 2014 at 08:12:12AM +0100, Xiubo Li wrote:
> > 
> > > > The 'big-endian-data' property is originally used to indicate whether the
> > > > LSB firstly or MSB firstly will be transmitted to the CODEC or received
> > > > from the CODEC, and there has nothing relation to the memory data.
> > 
> > > Both Nicolin [1] and Mark [2] asked for the old property to be kept
> > > around for compatibility reasons. Those requests seem to have been
> > > ignored entirely; the commit message doesn't even describe why you
> > > believe removing support for the old property is safe.
> > 
> > The response was that the relevant chips aren't out in the wild so it's
> > vanishingly unlikely that anyone would have a real DT with this option
> > enabled.  I'll defer to Nicolin on if that's the case or not.
> 
> Yea, it sounds like so. I thought Vf610 also used this property. But if
> only LS1 using it, I think we may consent this rename right?

If you two are happy, then fine by me. My only concern was that the
rationale for this being safe wasn't in the commit message.

Mark.

Re: [PATCHv2] ASoC: fsl-sai: using 'lsb-first' property instead of 'big-endian-data'.

[Nicolin Chen]

On Fri, Aug 29, 2014 at 06:26:02PM +0100, Mark Rutland wrote:
> On Fri, Aug 29, 2014 at 05:40:11PM +0100, Nicolin Chen wrote:
> > On Fri, Aug 29, 2014 at 12:59:43PM +0100, Mark Brown wrote:
> > > On Fri, Aug 29, 2014 at 12:06:01PM +0100, Mark Rutland wrote:
> > > > On Fri, Aug 29, 2014 at 08:12:12AM +0100, Xiubo Li wrote:
> > > 
> > > > > The 'big-endian-data' property is originally used to indicate whether the
> > > > > LSB firstly or MSB firstly will be transmitted to the CODEC or received
> > > > > from the CODEC, and there has nothing relation to the memory data.
> > > 
> > > > Both Nicolin [1] and Mark [2] asked for the old property to be kept
> > > > around for compatibility reasons. Those requests seem to have been
> > > > ignored entirely; the commit message doesn't even describe why you
> > > > believe removing support for the old property is safe.
> > > 
> > > The response was that the relevant chips aren't out in the wild so it's
> > > vanishingly unlikely that anyone would have a real DT with this option
> > > enabled.  I'll defer to Nicolin on if that's the case or not.
> > 
> > Yea, it sounds like so. I thought Vf610 also used this property. But if
> > only LS1 using it, I think we may consent this rename right?
> 
> If you two are happy, then fine by me. My only concern was that the
> rationale for this being safe wasn't in the commit message.

Agreed, it's better to be described in the commit comments as well.

And regarding the compatibility with the old DT, I personally prefer
it to be added in the change for safety but if this case is allowable
which I'm not so sure about it, I'm fairly okay with the rename.

Nicolin

Re: [PATCHv2] ASoC: fsl-sai: using 'lsb-first' property instead of 'big-endian-data'.

[Mark Brown]

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

On Fri, Aug 29, 2014 at 10:40:20AM -0700, Nicolin Chen wrote:

> Agreed, it's better to be described in the commit comments as well.

> And regarding the compatibility with the old DT, I personally prefer
> it to be added in the change for safety but if this case is allowable
> which I'm not so sure about it, I'm fairly okay with the rename.

If you're happy I can easily add a note, but keeping the old property
would remove all doubt :)  I'll wait for either an ack or a respin.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCHv2] ASoC: fsl-sai: using 'lsb-first' property instead of 'big-endian-data'.

[Nicolin Chen]

On Fri, Aug 29, 2014 at 08:11:41PM +0100, Mark Brown wrote:
> On Fri, Aug 29, 2014 at 10:40:20AM -0700, Nicolin Chen wrote:
> 
> > Agreed, it's better to be described in the commit comments as well.
> 
> > And regarding the compatibility with the old DT, I personally prefer
> > it to be added in the change for safety but if this case is allowable
> > which I'm not so sure about it, I'm fairly okay with the rename.
> 
> If you're happy I can easily add a note, but keeping the old property

That will be helpful, thank you :)

> would remove all doubt :)  I'll wait for either an ack or a respin.

Yea, I'm fine with it. Acked.

Thanks
Nicolin

Re: [PATCHv2] ASoC: fsl-sai: using 'lsb-first' property instead of 'big-endian-data'.

[Mark Brown]

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

On Fri, Aug 29, 2014 at 03:12:12PM +0800, Xiubo Li wrote:
> The 'big-endian-data' property is originally used to indicate whether the
> LSB firstly or MSB firstly will be transmitted to the CODEC or received
> from the CODEC, and there has nothing relation to the memory data.

Applied, thanks.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]