Re: [PATCH] ASoC: nau8825: disable crosstalk by default

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Nov 23, 2017 at 3:26 PM, John Hsu <KCHSU0@xxxxxxxxxxx> wrote:
> On 11/23/2017 12:43 PM, Wu-Cheng Li (李務誠) wrote:
>>
>> On Thu, Nov 23, 2017 at 11:19 AM, John Hsu <KCHSU0@xxxxxxxxxxx> wrote:
>>>
>>> The driver makes the crosstalk funciton disabled by default
>>> which can simplify the codec function. The platform may not
>>> need this funciton and reduce the potential risk. Therefore,
>>> We change the property "nuvoton,crosstalk-bypass" to
>>> "nuvoton,crosstalk-enable". The crosstalk measurement is enabled
>>> if the property is set. Otherwise, it is disabled. Besides,
>>> add more condition in the entry point of the crosstalk sequence
>>> to disable the function completely.
>>>
>>> Signed-off-by: John Hsu <KCHSU0@xxxxxxxxxxx>
>>> ---
>>>  Documentation/devicetree/bindings/sound/nau8825.txt |  4 ++--
>>>  sound/soc/codecs/nau8825.c                          | 21
>>> +++++++++++++--------
>>>  sound/soc/codecs/nau8825.h                          |  2 +-
>>>  3 files changed, 16 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/sound/nau8825.txt
>>> b/Documentation/devicetree/bindings/sound/nau8825.txt
>>> index 2f5e973..d16d968 100644
>>> --- a/Documentation/devicetree/bindings/sound/nau8825.txt
>>> +++ b/Documentation/devicetree/bindings/sound/nau8825.txt
>>> @@ -69,7 +69,7 @@ Optional properties:
>>>    - nuvoton,jack-insert-debounce: number from 0 to 7 that sets debounce
>>> time to 2^(n+2) ms
>>>    - nuvoton,jack-eject-debounce: number from 0 to 7 that sets debounce
>>> time to 2^(n+2) ms
>>>
>>> -  - nuvoton,crosstalk-bypass: make crosstalk function bypass if set.
>>> +  - nuvoton,crosstalk-enable: make crosstalk function enable if set.
>>>
>>>    - clocks: list of phandle and clock specifier pairs according to
>>> common clock bindings for the
>>>        clocks described in clock-names
>>> @@ -98,7 +98,7 @@ Example:
>>>        nuvoton,short-key-debounce = <2>;
>>>        nuvoton,jack-insert-debounce = <7>;
>>>        nuvoton,jack-eject-debounce = <7>;
>>> -      nuvoton,crosstalk-bypass;
>>> +      nuvoton,crosstalk-enable;
>>>
>>>        clock-names = "mclk";
>>>        clocks = <&tegra_car TEGRA210_CLK_CLK_OUT_2>;
>>> diff --git a/sound/soc/codecs/nau8825.c b/sound/soc/codecs/nau8825.c
>>> index 714ce17..2d6c536 100644
>>> --- a/sound/soc/codecs/nau8825.c
>>> +++ b/sound/soc/codecs/nau8825.c
>>> @@ -792,6 +792,10 @@ static void nau8825_xtalk_work(struct work_struct
>>> *work)
>>>         struct nau8825 *nau8825 = container_of(
>>>                 work, struct nau8825, xtalk_work);
>>>
>>> +       /* block when crosstalk disabled */
>>> +       if (!nau8825->xtalk_enable)
>>
>> This can only happen when NAU8825_IMPEDANCE_MEAS_IRQ is triggered.
>> I suggest moving the check to nau8825_interrupt(). Do not schedule a work
>> if xtalk_enable is false.
>
>
> The condition check also exists in nau8825_interrupt. It's indeed
> redundant here, but just for accident.

There are two places that schedule a work. One is already surrounded by a check
of xtalk_enable. The other doesn't. It's better to be consistent for
both places to
check xtalk_enable before scheduling a work. Besides, If the caller
tries to schedule
nau8825_xtalk_work when crosstalk is disabled, something is wrong.
>
>>> +               return;
>>> +
>>>         nau8825_xtalk_measure(nau8825);
>>>         /* To determine the cross talk side tone gain when reach
>>>          * the impedance measure state.
>>> @@ -815,11 +819,12 @@ static void nau8825_xtalk_work(struct work_struct
>>> *work)
>>>
>>>  static void nau8825_xtalk_cancel(struct nau8825 *nau8825)
>>>  {
>>> -       /* If the xtalk_protect is true, that means the process is still
>>> -        * on going. The driver forces to cancel the cross talk task and
>>> +       /* If the crosstalk is eanbled and the process is on going,
>>> +        * the driver forces to cancel the cross talk task and
>>>          * restores the configuration to original status.
>>>          */
>>> -       if (nau8825->xtalk_protect) {
>>> +       if (nau8825->xtalk_enable && nau8825->xtalk_protect &&
>>> +               nau8825->xtalk_state != NAU8825_XTALK_DONE) {
>>
>> Do we need to check xtalk_state? It looks like xtalk_state won't be
>> NAU8825_XTALK_DONE if xtalk_protect is true.
>
>
> Suppose the crosstalk is enabled. In normal case, the jack insertion
> and crosstalk measurement is on going. The xtalk_protect can present
> the action and do recovery if the measurement is cancelled on the way.
> In the case, the xtalk_state check just add the limitation where the
> recovery is unnecessary when the crosstalk is in done state.
> But the xtalk_state check also works when resume. After resume, the
> xtalk_protect flag will also been raised for the protection of jack
> detection from playback. But the codec will detect both insetion and
> ejection. If the jack ejects, the eject interruption will happen and
> go to the cancel function. The xtalk_state check can avoid the
> recovery.

I see. Then why do we need to check xtalk_protect here? Checking
xtalk_state is enough. Right?
>
>
>>>                 cancel_work_sync(&nau8825->xtalk_work);
>>>                 nau8825_xtalk_clean(nau8825);
>>>         }
>>> @@ -1686,7 +1691,7 @@ static irqreturn_t nau8825_interrupt(int irq, void
>>> *data)
>>>         } else if (active_irq & NAU8825_HEADSET_COMPLETION_IRQ) {
>>>                 if (nau8825_is_jack_inserted(regmap)) {
>>>                         event |= nau8825_jack_insert(nau8825);
>>> -                       if (!nau8825->xtalk_bypass &&
>>> !nau8825->high_imped) {
>>> +                       if (nau8825->xtalk_enable &&
>>> !nau8825->high_imped) {
>>>                                 /* Apply the cross talk suppression in
>>> the
>>>                                  * headset without high impedance.
>>>                                  */
>>> @@ -2440,8 +2445,8 @@ static void nau8825_print_device_properties(struct
>>> nau8825 *nau8825)
>>>                         nau8825->jack_insert_debounce);
>>>         dev_dbg(dev, "jack-eject-debounce:  %d\n",
>>>                         nau8825->jack_eject_debounce);
>>> -       dev_dbg(dev, "crosstalk-bypass:     %d\n",
>>> -                       nau8825->xtalk_bypass);
>>> +       dev_dbg(dev, "crosstalk-enable:     %d\n",
>>> +                       nau8825->xtalk_enable);
>>>  }
>>>
>>>  static int nau8825_read_device_properties(struct device *dev,
>>> @@ -2506,8 +2511,8 @@ static int nau8825_read_device_properties(struct
>>> device *dev,
>>>                 &nau8825->jack_eject_debounce);
>>>         if (ret)
>>>                 nau8825->jack_eject_debounce = 0;
>>> -       nau8825->xtalk_bypass = device_property_read_bool(dev,
>>> -               "nuvoton,crosstalk-bypass");
>>> +       nau8825->xtalk_enable = device_property_read_bool(dev,
>>> +               "nuvoton,crosstalk-enable");
>>>
>>>         nau8825->mclk = devm_clk_get(dev, "mclk");
>>>         if (PTR_ERR(nau8825->mclk) == -EPROBE_DEFER) {
>>> diff --git a/sound/soc/codecs/nau8825.h b/sound/soc/codecs/nau8825.h
>>> index 8aee5c86..199d6ea 100644
>>> --- a/sound/soc/codecs/nau8825.h
>>> +++ b/sound/soc/codecs/nau8825.h
>>> @@ -476,7 +476,7 @@ struct nau8825 {
>>>         int xtalk_event_mask;
>>>         bool xtalk_protect;
>>>         int imp_rms[NAU8825_XTALK_IMM];
>>> -       int xtalk_bypass;
>>> +       int xtalk_enable;
>>>  };
>>>
>>>  int nau8825_enable_jack_detect(struct snd_soc_codec *codec,
>>> --
>>> 2.6.4
>>>
>
>
>
> ===========================================================================================
> The privileged confidential information contained in this email is intended
> for use only by the addressees as indicated by the original sender of this
> email. If you are not the addressee indicated in this email or are not
> responsible for delivery of the email to such a person, please kindly reply
> to the sender indicating this fact and delete all copies of it from your
> computer and network server immediately. Your cooperation is highly
> appreciated. It is advised that any unauthorized use of confidential
> information of Nuvoton is strictly prohibited; and any information in this
> email irrelevant to the official business of Nuvoton shall be deemed as
> neither given nor endorsed by Nuvoton.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel




[Index of Archives]     [ALSA User]     [Linux Audio Users]     [Pulse Audio]     [Kernel Archive]     [Asterisk PBX]     [Photo Sharing]     [Linux Sound]     [Video 4 Linux]     [Gimp]     [Yosemite News]

  Powered by Linux