diff mbox series

[2/4] ASoC: dt-bindings: wlf,wm8524: Add a property to specify power up sequency time

Message ID 20230222113945.3390672-2-chancel.liu@nxp.com
State New
Headers
Series [1/4] ASoC: dt-bindings: wlf,wm8524: Convert to json-schema |

Commit Message

Chancel Liu Feb. 22, 2023, 11:39 a.m. UTC 
  This property specifies power up to audio out time. It's necessary
beacause this device has to wait some time before ready to output audio
after MCLK, BCLK and MUTE=1 are enabled. For more details about the
timing constraints, please refer to WTN0302 on
https://www.cirrus.com/products/wm8524/

Signed-off-by: Chancel Liu <chancel.liu@nxp.com>
---
 .../devicetree/bindings/sound/wlf,wm8524.yaml          | 10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

Krzysztof Kozlowski Feb. 22, 2023, 12:42 p.m. UTC | #1 
On 22/02/2023 12:39, Chancel Liu wrote:
> This property specifies power up to audio out time. It's necessary
> beacause this device has to wait some time before ready to output audio

typo... run spellcheck, also on the subject

> after MCLK, BCLK and MUTE=1 are enabled. For more details about the
> timing constraints, please refer to WTN0302 on
> https://www.cirrus.com/products/wm8524/
> 
> Signed-off-by: Chancel Liu <chancel.liu@nxp.com>
> ---
>  .../devicetree/bindings/sound/wlf,wm8524.yaml          | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/sound/wlf,wm8524.yaml b/Documentation/devicetree/bindings/sound/wlf,wm8524.yaml
> index 09c54cc7de95..54b4da5470e4 100644
> --- a/Documentation/devicetree/bindings/sound/wlf,wm8524.yaml
> +++ b/Documentation/devicetree/bindings/sound/wlf,wm8524.yaml
> @@ -21,6 +21,15 @@ properties:
>      description:
>        a GPIO spec for the MUTE pin.
>  
> +  wlf,power-up-delay-ms:
> +    maximum: 1500

maximum is 1003. Where do you see 1500?

minimum: 82

> +    default: 100
> +    description:
> +      Power up sequency delay time in millisecond. It specifies power up to

typo: sequence?

> +      audio out time. For more details about the timing constraints of this
> +      device, please refer to WTN0302 on
> +      https://www.cirrus.com/products/wm8524/.

According to WTN0302 this might or might not include regulator
ramp-up-delay. You should clearly indicate which part of it this delay
is to not mix up with ramp up. IOW, mention exactly from where the value
comes (e.g. Δt POWER UP TO AUDIO OUT TIMING table, depending on sampling
clock rate). Otherwise you introduce quite loose property which will be
including regulator ramp up in some cases...

Best regards,
Krzysztof
Mark Brown Feb. 22, 2023, 12:50 p.m. UTC | #2 
On Wed, Feb 22, 2023 at 07:39:43PM +0800, Chancel Liu wrote:
> This property specifies power up to audio out time. It's necessary
> beacause this device has to wait some time before ready to output audio
> after MCLK, BCLK and MUTE=1 are enabled. For more details about the
> timing constraints, please refer to WTN0302 on
> https://www.cirrus.com/products/wm8524/

According to that the delay is a property of MCLK and the sample rate
rather than a per board constant, it shouldn't be in DT but rather the
driver should figure out the required delay on each startup.
Chancel Liu Feb. 24, 2023, 10:54 a.m. UTC | #3 
> On 22/02/2023 12:39, Chancel Liu wrote:
> > This property specifies power up to audio out time. It's necessary
> > beacause this device has to wait some time before ready to output audio
> 
> typo... run spellcheck, also on the subject
> 
> > after MCLK, BCLK and MUTE=1 are enabled. For more details about the
> > timing constraints, please refer to WTN0302 on
> >
> >
> > Signed-off-by: Chancel Liu <chancel.liu@nxp.com>
> > ---
> >  .../devicetree/bindings/sound/wlf,wm8524.yaml          | 10
> ++++++++++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/sound/wlf,wm8524.yaml
> b/Documentation/devicetree/bindings/sound/wlf,wm8524.yaml
> > index 09c54cc7de95..54b4da5470e4 100644
> > --- a/Documentation/devicetree/bindings/sound/wlf,wm8524.yaml
> > +++ b/Documentation/devicetree/bindings/sound/wlf,wm8524.yaml
> > @@ -21,6 +21,15 @@ properties:
> >      description:
> >        a GPIO spec for the MUTE pin.
> >
> > +  wlf,power-up-delay-ms:
> > +    maximum: 1500
> 
> maximum is 1003. Where do you see 1500?
> 
> minimum: 82
> 

Yes, you are absolutely right. From the power up to audio out timing table in
WTN0302, the minimum number is 82 and the maximum is 1003.

Consider the following possibilities:
1. These timings may depend on the system design
2. These timings may be simulated results
3. These timings may be the minimum values

I set a larger value trying to extend the time. The larger value of course
introduces unwanted time delay but it benefits on avoiding beginning audio
lost.

I also did some tests on a board. If I want to work on 48KHZ sample rate and
512FS, the recommended value is 143. But the test result showed 143ms is not
enough. I increased the value to 500ms and could hear the beginning sound.

Maybe it's a better choice to let DT set the suitable value? Is there a similar
situation before?

> > +    default: 100
> > +    description:
> > +      Power up sequency delay time in millisecond. It specifies power up to
> 
> typo: sequence?
> 

Sorry. I must avoid these spelling mistakes.

> > +      audio out time. For more details about the timing constraints of this
> > +      device, please refer to WTN0302 on
> > +
> 
> According to WTN0302 this might or might not include regulator
> ramp-up-delay. You should clearly indicate which part of it this delay
> is to not mix up with ramp up. IOW, mention exactly from where the value
> comes (e.g. Δt POWER UP TO AUDIO OUT TIMING table, depending on sampling
> clock rate). Otherwise you introduce quite loose property which will be
> including regulator ramp up in some cases...
> 
> Best regards,
> Krzysztof

Yes. This property is designed for power up to audio out timing. I need to
clearly point this out.

Thank you very much for your suggestions!

Regards, 
Chancel Liu
Chancel Liu Feb. 24, 2023, 10:56 a.m. UTC | #4 
> On Wed, Feb 22, 2023 at 07:39:43PM +0800, Chancel Liu wrote:
> > This property specifies power up to audio out time. It's necessary
> > beacause this device has to wait some time before ready to output
> > audio after MCLK, BCLK and MUTE=1 are enabled. For more details about
> > the timing constraints, please refer to WTN0302 on
> > https://www.cirrus.com/products/wm8524/
> 
> According to that the delay is a property of MCLK and the sample rate rather
> than a per board constant, it shouldn't be in DT but rather the driver should
> figure out the required delay on each startup.

I can't agree with you more. From the power up to audio out timing table in
WTN0302, the delay depends on sample rate and MCLK. Driver should calculate it
rather than read it from DT. However as I mentioned in my last email, values in
the table seem not accurate for all systems. It's a kind of compromise to get
the value from DT. Do other codecs have a similar situation?

Thank you very much for your suggestions!

Regards, 
Chancel Liu
Krzysztof Kozlowski Feb. 24, 2023, 10:59 a.m. UTC | #5 
On 24/02/2023 11:56, Chancel Liu wrote:
>> On Wed, Feb 22, 2023 at 07:39:43PM +0800, Chancel Liu wrote:
>>> This property specifies power up to audio out time. It's necessary
>>> beacause this device has to wait some time before ready to output
>>> audio after MCLK, BCLK and MUTE=1 are enabled. For more details about
>>> the timing constraints, please refer to WTN0302 on
>>> https://www.cirrus.com/products/wm8524/
>>
>> According to that the delay is a property of MCLK and the sample rate rather
>> than a per board constant, it shouldn't be in DT but rather the driver should
>> figure out the required delay on each startup.
> 
> I can't agree with you more. From the power up to audio out timing table in
> WTN0302, the delay depends on sample rate and MCLK. Driver should calculate it
> rather than read it from DT. However as I mentioned in my last email, values in
> the table seem not accurate for all systems. It's a kind of compromise to get
> the value from DT. Do other codecs have a similar situation?

DT is for hardware properties, not for software compromises.


Best regards,
Krzysztof
Krzysztof Kozlowski Feb. 24, 2023, 11 a.m. UTC | #6 
On 24/02/2023 11:54, Chancel Liu wrote:
>> On 22/02/2023 12:39, Chancel Liu wrote:
>>> This property specifies power up to audio out time. It's necessary
>>> beacause this device has to wait some time before ready to output audio
>>
>> typo... run spellcheck, also on the subject
>>
>>> after MCLK, BCLK and MUTE=1 are enabled. For more details about the
>>> timing constraints, please refer to WTN0302 on
>>>
>>>
>>> Signed-off-by: Chancel Liu <chancel.liu@nxp.com>
>>> ---
>>>  .../devicetree/bindings/sound/wlf,wm8524.yaml          | 10
>> ++++++++++
>>>  1 file changed, 10 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/sound/wlf,wm8524.yaml
>> b/Documentation/devicetree/bindings/sound/wlf,wm8524.yaml
>>> index 09c54cc7de95..54b4da5470e4 100644
>>> --- a/Documentation/devicetree/bindings/sound/wlf,wm8524.yaml
>>> +++ b/Documentation/devicetree/bindings/sound/wlf,wm8524.yaml
>>> @@ -21,6 +21,15 @@ properties:
>>>      description:
>>>        a GPIO spec for the MUTE pin.
>>>
>>> +  wlf,power-up-delay-ms:
>>> +    maximum: 1500
>>
>> maximum is 1003. Where do you see 1500?
>>
>> minimum: 82
>>
> 
> Yes, you are absolutely right. From the power up to audio out timing table in
> WTN0302, the minimum number is 82 and the maximum is 1003.
> 
> Consider the following possibilities:
> 1. These timings may depend on the system design
> 2. These timings may be simulated results
> 3. These timings may be the minimum values
> 
> I set a larger value trying to extend the time. The larger value of course
> introduces unwanted time delay but it benefits on avoiding beginning audio
> lost.
> 
> I also did some tests on a board. If I want to work on 48KHZ sample rate and
> 512FS, the recommended value is 143. But the test result showed 143ms is not
> enough. I increased the value to 500ms and could hear the beginning sound.

Maybe you miss proper regulator ramp-up?

> 
> Maybe it's a better choice to let DT set the suitable value? Is there a similar
> situation before?

That's not really good argument. DT should describe hardware and if this
property does not match hardware, it means you mix this with something
else. Something not for DT.


Best regards,
Krzysztof
Chancel Liu Feb. 27, 2023, 9:06 a.m. UTC | #7 
> On 24/02/2023 11:54, Chancel Liu wrote:
> >> On 22/02/2023 12:39, Chancel Liu wrote:
> >>> This property specifies power up to audio out time. It's necessary
> >>> beacause this device has to wait some time before ready to output audio
> >>
> >> typo... run spellcheck, also on the subject
> >>
> >>> after MCLK, BCLK and MUTE=1 are enabled. For more details about the
> >>> timing constraints, please refer to WTN0302 on
> >>>
> >>>
> >>> Signed-off-by: Chancel Liu <chancel.liu@nxp.com>
> >>> ---
> >>>  .../devicetree/bindings/sound/wlf,wm8524.yaml          | 10
> >> ++++++++++
> >>>  1 file changed, 10 insertions(+)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/sound/wlf,wm8524.yaml
> >> b/Documentation/devicetree/bindings/sound/wlf,wm8524.yaml
> >>> index 09c54cc7de95..54b4da5470e4 100644
> >>> --- a/Documentation/devicetree/bindings/sound/wlf,wm8524.yaml
> >>> +++ b/Documentation/devicetree/bindings/sound/wlf,wm8524.yaml
> >>> @@ -21,6 +21,15 @@ properties:
> >>>      description:
> >>>        a GPIO spec for the MUTE pin.
> >>>
> >>> +  wlf,power-up-delay-ms:
> >>> +    maximum: 1500
> >>
> >> maximum is 1003. Where do you see 1500?
> >>
> >> minimum: 82
> >>
> >
> > Yes, you are absolutely right. From the power up to audio out timing table in
> > WTN0302, the minimum number is 82 and the maximum is 1003.
> >
> > Consider the following possibilities:
> > 1. These timings may depend on the system design
> > 2. These timings may be simulated results
> > 3. These timings may be the minimum values
> >
> > I set a larger value trying to extend the time. The larger value of course
> > introduces unwanted time delay but it benefits on avoiding beginning audio
> > lost.
> >
> > I also did some tests on a board. If I want to work on 48KHZ sample rate and
> > 512FS, the recommended value is 143. But the test result showed 143ms is not
> > enough. I increased the value to 500ms and could hear the beginning sound.
> 
> Maybe you miss proper regulator ramp-up?
> 
> >
> > Maybe it's a better choice to let DT set the suitable value? Is there a similar
> > situation before?
> 
> That's not really good argument. DT should describe hardware and if this
> property does not match hardware, it means you mix this with something
> else. Something not for DT.
> 
> 
> Best regards,
> Krzysztof

OK. The patches for adding such property are not really good. I need to find a
better way to address the issue.

I think PATCH 1 and PATCH 3 can be kept. So I will modify them according
comments.

Thank you for your review. That benefits me a lot.

Regards, 
Chancel Liu
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/sound/wlf,wm8524.yaml b/Documentation/devicetree/bindings/sound/wlf,wm8524.yaml
index 09c54cc7de95..54b4da5470e4 100644
--- a/Documentation/devicetree/bindings/sound/wlf,wm8524.yaml
+++ b/Documentation/devicetree/bindings/sound/wlf,wm8524.yaml
@@ -21,6 +21,15 @@  properties:
     description:
       a GPIO spec for the MUTE pin.
 
+  wlf,power-up-delay-ms:
+    maximum: 1500
+    default: 100
+    description:
+      Power up sequency delay time in millisecond. It specifies power up to
+      audio out time. For more details about the timing constraints of this
+      device, please refer to WTN0302 on
+      https://www.cirrus.com/products/wm8524/.
+
 required:
   - compatible
   - wlf,mute-gpios
@@ -34,4 +43,5 @@  examples:
     wm8524: codec {
             compatible = "wlf,wm8524";
             wlf,mute-gpios = <&gpio1 8 GPIO_ACTIVE_LOW>;
+            wlf,power-up-delay-ms = <300>;
     };