Re: [PATCH 7/7] ASoC: dt-bindings: ES8389: Add members about HPF and clock

Re: [PATCH 7/7] ASoC: dt-bindings: ES8389: Add members about HPF and clock

[Krzysztof Kozlowski]

On Tue, Jun 09, 2026 at 11:06:23AM +0800, Zhang Yi wrote:
> Add members related to HPF and mclk_source

Please organize the patch documenting the ABI (DT bindings)
before the patch using that ABI.
See also: https://elixir.bootlin.com/linux/v6.14-rc6/source/Documentation/devicetree/bindings/submitting-patches.rst#L46

> 
> Signed-off-by: Zhang Yi <zhangyi@everest-semi.com>
> ---
>  .../bindings/sound/everest,es8389.yaml        | 23 +++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/sound/everest,es8389.yaml b/Documentation/devicetree/bindings/sound/everest,es8389.yaml
> index 75ce0bc48..be92014c0 100644
> --- a/Documentation/devicetree/bindings/sound/everest,es8389.yaml
> +++ b/Documentation/devicetree/bindings/sound/everest,es8389.yaml
> @@ -38,6 +38,27 @@ properties:
>      description:
>        Interface power supply.
>  
> +  everest,mclk-from-sclk:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description:
> +      Indicates that SCLK is used as the internal clock.

And what happens with mclk in such case? Is it still wired?

> +
> +  everest,hpfl:
> +    $ref: /schemas/types.yaml#/definitions/uint8
> +    description:
> +      the HPF value of ADCL.

Is HPF value in dB? If so, use proper unit suffix and proper units.

> +    minimum: 0x00
> +    maximum: 0x0f
> +    default: 0x0a
> +
> +  everest,hpfr:
> +    $ref: /schemas/types.yaml#/definitions/uint8
> +    description:
> +      the HPF value of ADCR.
> +    minimum: 0x00
> +    maximum: 0x0f
> +    default: 0x0a
> +
>  required:
>    - compatible
>    - reg
> @@ -58,5 +79,7 @@ examples:
>          #sound-dai-cells = <0>;
>          vddd-supply = <&vdd3v3>;
>          vdda-supply = <&vdd3v3>;
> +        everest,hpfl = [0a];

<0xa>? What did you want to say here?

Best regards,
Krzysztof

RE: [PATCH 7/7] ASoC: dt-bindings: ES8389: Add members about HPF and clock

[Zhang Yi]

> Please organize the patch documenting the ABI (DT bindings)
> before the patch using that ABI.
> See also: https://elixir.bootlin.com/linux/v6.14-rc6/source/Documentation/devicetree/bindings/submitting-patches.rst#L46

Thanks for the reminder

> > +  everest,mclk-from-sclk:
> > +    $ref: /schemas/types.yaml#/definitions/flag
> > +    description:
> > +      Indicates that SCLK is used as the internal clock.
> 
> And what happens with mclk in such case? Is it still wired?

Yes, setting mclk-from-sclk does not affect the MCLK connection.

> > +
> > +  everest,hpfl:
> > +    $ref: /schemas/types.yaml#/definitions/uint8
> > +    description:
> > +      the HPF value of ADCL.
> 
> Is HPF value in dB? If so, use proper unit suffix and proper units.

No, the values here correspond to the values in the registers.
The value is not in dB

> >          vddd-supply = <&vdd3v3>;
> >          vdda-supply = <&vdd3v3>;
> > +        everest,hpfl = [0a];
> 
> <0xa>? What did you want to say here?

I just wanted to give an example to show how to set the values of everest,hpfl to 0x0a.

Re: [PATCH 7/7] ASoC: dt-bindings: ES8389: Add members about HPF and clock

[Krzysztof Kozlowski]

On 10/06/2026 11:58, Zhang Yi wrote:
>> Please organize the patch documenting the ABI (DT bindings)
>> before the patch using that ABI.
>> See also: https://elixir.bootlin.com/linux/v6.14-rc6/source/Documentation/devicetree/bindings/submitting-patches.rst#L46
> 
> Thanks for the reminder
> 
>>> +  everest,mclk-from-sclk:
>>> +    $ref: /schemas/types.yaml#/definitions/flag
>>> +    description:
>>> +      Indicates that SCLK is used as the internal clock.
>>
>> And what happens with mclk in such case? Is it still wired?
> 
> Yes, setting mclk-from-sclk does not affect the MCLK connection.

I am asking about wiring of the device. If MCLK is used from SCLK, but
SCLK is used as the internal clock, then how can you still have MCLK
connected?

> 
>>> +
>>> +  everest,hpfl:
>>> +    $ref: /schemas/types.yaml#/definitions/uint8
>>> +    description:
>>> +      the HPF value of ADCL.
>>
>> Is HPF value in dB? If so, use proper unit suffix and proper units.
> 
> No, the values here correspond to the values in the registers.
> The value is not in dB

What are the meanings of the register values?

> 
>>>          vddd-supply = <&vdd3v3>;
>>>          vdda-supply = <&vdd3v3>;
>>> +        everest,hpfl = [0a];
>>
>> <0xa>? What did you want to say here?
> 
> I just wanted to give an example to show how to set the values of everest,hpfl to 0x0a.

So use syntax I asked.

Best regards,
Krzysztof

Re: [PATCH 7/7] ASoC: dt-bindings: ES8389: Add members about HPF and clock

[Krzysztof Kozlowski]

On 15/06/2026 08:07, Zhang Yi wrote:
>>>>> +    $ref: /schemas/types.yaml#/definitions/flag
>>>>> +    description:
>>>>> +      Indicates that SCLK is used as the internal clock.
>>>>
>>>> And what happens with mclk in such case? Is it still wired?
>>>
>>> Yes, setting mclk-from-sclk does not affect the MCLK connection.
>>
>> I am asking about wiring of the device. If MCLK is used from SCLK, but
>> SCLK is used as the internal clock, then how can you still have MCLK
>> connected?
> 
> If MCLK is derived from SCLK, whether the 'MCLK' pin on the device is
> connected or not has no effect on operation.

If MCLK is connected, why would you derive it from SCLK? That's the
point - your clocks property already might be telling everything needed
here.

> 
>>>>> +
>>>>> +  everest,hpfl:
>>>>> +    $ref: /schemas/types.yaml#/definitions/uint8
>>>>> +    description:
>>>>> +      the HPF value of ADCL.
>>>>
>>>> Is HPF value in dB? If so, use proper unit suffix and proper units.
>>>
>>> No, the values here correspond to the values in the registers.
>>> The value is not in dB
>>
>> What are the meanings of the register values?
> 
> The value of everest,hpfl is equal to the value of the corresponding register,
> not the cutoff frequency of the HPF.

You keep avoiding answers.

I don't want you to encode standard units as register values.


Best regards,
Krzysztof

Re: [PATCH 7/7] ASoC: dt-bindings: ES8389: Add members about HPF and clock

[Krzysztof Kozlowski]

On 15/06/2026 12:14, Zhang Yi wrote:
>>>>> Yes, setting mclk-from-sclk does not affect the MCLK connection.
>>>>
>>>> I am asking about wiring of the device. If MCLK is used from SCLK, but
>>>> SCLK is used as the internal clock, then how can you still have MCLK
>>>> connected?
>>>
>>> If MCLK is derived from SCLK, whether the 'MCLK' pin on the device is
>>> connected or not has no effect on operation.
>>
>> If MCLK is connected, why would you derive it from SCLK? That's the
>> point - your clocks property already might be telling everything needed
>> here.
> 
> This is because sometimes the codec does not work properly
> when using the MCLK provided by the CPU.

OK.

> 
>>>>>>> +
>>>>>>> +  everest,hpfl:
>>>>>>> +    $ref: /schemas/types.yaml#/definitions/uint8
>>>>>>> +    description:
>>>>>>> +      the HPF value of ADCL.
>>>>>>
>>>>>> Is HPF value in dB? If so, use proper unit suffix and proper units.
>>>>>
>>>>> No, the values here correspond to the values in the registers.
>>>>> The value is not in dB
>>>>
>>>> What are the meanings of the register values?
>>>
>>> The value of everest,hpfl is equal to the value of the corresponding register,
>>> not the cutoff frequency of the HPF.
>>
>> You keep avoiding answers.
>>
>> I don't want you to encode standard units as register values.
> 
> I think I understand what you mean.
> Do you want me to enter the physical values in DTS and then convert them to register values in the code?

If you have clear physical values then this should be rather used in DTS.


Best regards,
Krzysztof

Re: [PATCH 2/7] ASoC: codecs: ES8389: Fix the issue about mclk_src

[Krzysztof Kozlowski]

On Tue, Jun 09, 2026 at 11:06:18AM +0800, Zhang Yi wrote:
> Fix the issue with incorrect modifications to mclk_src

What issue? Your commit msgs are really poor - explain nothing. You just
duplicated subject... and anything can be a fix.

> 
> Signed-off-by: Zhang Yi <zhangyi@everest-semi.com>
> ---
>  sound/soc/codecs/es8389.c | 12 ++++--------
>  sound/soc/codecs/es8389.h |  5 ++---
>  2 files changed, 6 insertions(+), 11 deletions(-)
> 
> diff --git a/sound/soc/codecs/es8389.c b/sound/soc/codecs/es8389.c
> index 9c8164916..7a9d5d42a 100644
> --- a/sound/soc/codecs/es8389.c
> +++ b/sound/soc/codecs/es8389.c
> @@ -36,8 +36,8 @@ struct	es8389_private {
>  	unsigned int sysclk;
>  	int mastermode;
>  
> -	u8 mclk_src;
>  	u8 vddd;
> +	bool mclk_src;
>  	int version;
>  	enum snd_soc_bias_level bias_level;
>  };
> @@ -607,9 +607,9 @@ static int es8389_pcm_hw_params(struct snd_pcm_substream *substream,
>  	regmap_update_bits(es8389->regmap, ES8389_ADC_FORMAT_MUTE, ES8389_DATA_LEN_MASK, state);
>  	regmap_update_bits(es8389->regmap, ES8389_DAC_FORMAT_MUTE, ES8389_DATA_LEN_MASK, state);
>  
> -	if (es8389->mclk_src == ES8389_SCLK_PIN) {
> +	if (es8389->mclk_src) {
>  		regmap_update_bits(es8389->regmap, ES8389_MASTER_CLK,
> -					ES8389_MCLK_SOURCE, es8389->mclk_src);
> +					ES8389_MCLK_MASK, ES8389_MCLK_FROM_SCLK);
>  		es8389->sysclk = params_channels(params) * params_width(params) * params_rate(params);
>  	}
>  
> @@ -897,11 +897,7 @@ static int es8389_probe(struct snd_soc_component *component)
>  	int ret, i;
>  	struct es8389_private *es8389 = snd_soc_component_get_drvdata(component);
>  
> -	ret = device_property_read_u8(component->dev, "everest,mclk-src", &es8389->mclk_src);

Why are you changing implemented ABI?

Best regards,
Krzysztof

RE: [PATCH 2/7] ASoC: codecs: ES8389: Fix the issue about mclk_src

[Zhang Yi]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 984 bytes --]

> > Fix the issue with incorrect modifications to mclk_src
> 
> What issue? Your commit msgs are really poor - explain nothing. You just
> duplicated subject... and anything can be a fix.

I'm sorry I didn't explain that clearly.
When the system needs to be configured to use the MCLK from the SCLK pin,
the old code still sets the relevant registers to use the MCLK from the MCLK pin.
I will include a more detailed description in future versions.

> >  	struct es8389_private *es8389 = snd_soc_component_get_drvdata(component);
> >  
> > -	ret = device_property_read_u8(component->dev, "everest,mclk-src", &es8389->mclk_src);
> 
> Why are you changing implemented ABI?

In the old ABI, `mclk_src` was defined as `u8`, which meant that users could set `mclk_src` to any value in the DTS¡ªsuch as `0x02`,
but the code wouldn't recognize what that value represented.
The actual purpose of `mclk_src` is to indicate whether `sclk` should be used as `mclk`.
So I've changed it to bool.

Re: [PATCH 2/7] ASoC: codecs: ES8389: Fix the issue about mclk_src

[Krzysztof Kozlowski]

On 10/06/2026 12:06, Zhang Yi wrote:
>>> Fix the issue with incorrect modifications to mclk_src
>>
>> What issue? Your commit msgs are really poor - explain nothing. You just
>> duplicated subject... and anything can be a fix.
> 
> I'm sorry I didn't explain that clearly.
> When the system needs to be configured to use the MCLK from the SCLK pin,
> the old code still sets the relevant registers to use the MCLK from the MCLK pin.
> I will include a more detailed description in future versions.
> 
>>>  	struct es8389_private *es8389 = snd_soc_component_get_drvdata(component);
>>>  
>>> -	ret = device_property_read_u8(component->dev, "everest,mclk-src", &es8389->mclk_src);
>>
>> Why are you changing implemented ABI?
> 
> In the old ABI, `mclk_src` was defined as `u8`, which meant that users could set `mclk_src` to any value in the DTS—such as `0x02`,
> but the code wouldn't recognize what that value represented.
> The actual purpose of `mclk_src` is to indicate whether `sclk` should be used as `mclk`.
> So I've changed it to bool.

You break ABI implemented by this driver, so you must clearly document
it with reasons, why breaking is necessary. Honestly, without proper
reason I would just answer that you must keep it backwards compatible.

Best regards,
Krzysztof

Re: [PATCH 7/7] ASoC: dt-bindings: ES8389: Add members about HPF and clock

[zhangyi]

> Please organize the patch documenting the ABI (DT bindings)

> before the patch using that ABI.

> See also:
https://elixir.bootlin.com/linux/v6.14-rc6/source/Documentation/devicetree/b
indings/submitting-patches.rst#L46

 

Thanks for the reminder

 

> > +  everest,mclk-from-sclk:

> > +    $ref: /schemas/types.yaml#/definitions/flag

> > +    description:

> > +      Indicates that SCLK is used as the internal clock.

> 

> And what happens with mclk in such case? Is it still wired?

 

Yes, setting mclk-from-sclk does not affect the MCLK connection.

 

> > +

> > +  everest,hpfl:

> > +    $ref: /schemas/types.yaml#/definitions/uint8

> > +    description:

> > +      the HPF value of ADCL.

> 

> Is HPF value in dB? If so, use proper unit suffix and proper units.

 

No, the values here correspond to the values in the registers.

The value is not in dB

 

> >          vddd-supply = <&vdd3v3>;

> >          vdda-supply = <&vdd3v3>;

> > +        everest,hpfl = [0a];

> 

> <0xa>? What did you want to say here?

 

I just wanted to give an example to show how to set the values of
everest,hpfl to 0x0a.

 

Re: [PATCH 7/7] ASoC: dt-bindings: ES8389: Add members about HPF and clock

[Krzysztof Kozlowski]

On Tue, Jun 09, 2026 at 03:17:32PM +0800, Zhang Yi wrote:
> Add members related to HPF and mclk_source
> 
> Signed-off-by: Zhang Yi <zhangyi@everest-semi.com>
> ---

You already sent v1 and received feedback.

You just completely ignored it.

NAK

Best regards,
Krzysztof

Re: [PATCH 7/7] ASoC: dt-bindings: ES8389: Add members about HPF and clock

[zhangyi]

> You already sent v1 and received feedback.
> 
> You just completely ignored it

When I submitted the v0 patch, I received feedback and suggestions from
Sashiko.
I wasn't sure how to handle Sashiko's feedback, and I mistakenly assumed
that if my patch didn't pass Sashiko's tests, you wouldn't receive it. 
So, after modifying the code based on Sashiko's reasonable suggestions, I
submitted a patch with the same version ID.
As a result, you received two consecutive patches from me. I didn't mean to
ignore your valuable feedback, and I apologize for this.

So, when I receive feedback from Sashiko, do I need to wait for the
maintainers to provide their feedback before incorporating the changes into
the next version's patch?