Re: [PATCH 2/2] ASoC: mediatek: mt6359: add MT6359 accdet driver

Re: [PATCH 2/2] ASoC: mediatek: mt6359: add MT6359 accdet driver

[Mark Brown]

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

On Wed, Jan 06, 2021 at 08:19:06PM +0800, Argus Lin wrote:
> MT6359 audio codec support accessory detect features, adds MT6359
> accdet driver to support plug detection and key detection.

> ---
>  sound/soc/codecs/Kconfig         |    7 +
>  sound/soc/codecs/Makefile        |    2 +
>  sound/soc/codecs/mt6359-accdet.c | 1951 ++++++++++++++++++++++++++++++++++++++
>  sound/soc/codecs/mt6359-accdet.h |  136 +++
>  sound/soc/codecs/mt6359.h        | 1863 +++++++++++++++++++++++++++++++++---

This driver is *huge*.  Looking through the code it feels like there's a
lot of things that are written with mostly duplicated code that differs
only in data so you could shrink things down a lot by refactoring things
to have one copy of the code and pass different data into it.

>  	  Enable support for the platform which uses MT6359 as
>  	  external codec device.
> +config SND_SOC_MT6359_ACCDET

Missing blank line here.

> +++ b/sound/soc/codecs/mt6359-accdet.c
> @@ -0,0 +1,1951 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2021 MediaTek Inc.

Please make the entire comment a C++ one so things look more
intentional.

> +#include "mt6359-accdet.h"
> +#include "mt6359.h"
> +/* grobal variable definitions */

Spelling mistake and you need more blank lines here.

> +#define REGISTER_VAL(x)	((x) - 1)
> +#define HAS_CAP(_c, _x)	\
> +	({typeof(_c)c = (_c); \
> +	typeof(_x)x = (_x); \
> +	(((c) & (x)) == (x)); })

These need namepsacing.

> +static struct mt63xx_accdet_data *accdet;
> +
> +static struct head_dts_data accdet_dts;
> +struct pwm_deb_settings *cust_pwm_deb;

You'd need a *very* good reason to be using global data rather than
storing anything in the device's driver data like most drivers.  There's
extensive use of global data here, and lots of raw pr_ prints rather
than dev_ prints as well - this doesn't look like how a Linux driver is
supposed to be written.

> +
> +const struct of_device_id mt6359_accdet_of_match[] = {
> +	{
> +		.compatible = "mediatek,mt6359-accdet",
> +		.data = &mt6359_accdet,

Given that this is specific to a particular PMIC why does this need a
compatible string?

> +/* global function declaration */
> +
> +static u64 mt6359_accdet_get_current_time(void)
> +{
> +	return sched_clock();
> +}

It is probably best to remove this wrapper.

> +static bool mt6359_accdet_timeout_ns(u64 start_time_ns, u64 timeout_time_ns)
> +{
> +	u64 cur_time = 0;
> +	u64 elapse_time = 0;
> +
> +	/* get current tick, ns */
> +	cur_time = mt6359_accdet_get_current_time();
> +	if (cur_time < start_time_ns) {
> +		start_time_ns = cur_time;
> +		/* 400us */
> +		timeout_time_ns = 400 * 1000;
> +	}
> +	elapse_time = cur_time - start_time_ns;
> +
> +	/* check if timeout */
> +	if (timeout_time_ns <= elapse_time)
> +		return false;
> +
> +	return true;
> +}

There must be a generic implementation of this already surely?

> +static unsigned int check_key(unsigned int v)
> +{

This looks a lot like open coding of the functionality of the extcon
adc_jack functionality.

> +static void send_key_event(unsigned int keycode, unsigned int flag)
> +{
> +	int report = 0;
> +
> +	switch (keycode) {
> +	case DW_KEY:
> +		if (flag != 0)
> +			report = SND_JACK_BTN_1;

What does flag mean?  At the very least it needs renaming.

> +static void send_status_event(unsigned int cable_type, unsigned int status)
> +{
> +	int report = 0;

This is one of those places that looks like it could be code with
different data passed in.

> +
> +	switch (cable_type) {
> +	case HEADSET_NO_MIC:
> +		if (status)
> +			report = SND_JACK_HEADPHONE;
> +		else
> +			report = 0;
> +		snd_soc_jack_report(&accdet->jack, report, SND_JACK_HEADPHONE);
> +		/* when plug 4-pole out, if both AB=3 AB=0 happen,3-pole plug
> +		 * in will be incorrectly reported, then 3-pole plug-out is
> +		 * reported,if no mantory 4-pole plug-out, icon would be
> +		 * visible.
> +		 */
> +		if (status == 0) {
> +			report = 0;
> +			snd_soc_jack_report(&accdet->jack, report, SND_JACK_MICROPHONE);
> +		}
> +		pr_info("accdet HEADPHONE(3-pole) %s\n",
> +			status ? "PlugIn" : "PlugOut");

You shouldn't be spamming the logs for normal events like this.

> +	regmap_read(accdet->regmap, ACCDET_IRQ_ADDR, &val);
> +	while (val & ACCDET_IRQ_MASK_SFT &&
> +	       mt6359_accdet_timeout_ns(cur_time, ACCDET_TIME_OUT))
> +		;

This is open coding regmap_read_poll_timeout(), this pattern is repeated
in several places.

> +static inline void clear_accdet_eint(unsigned int eintid)
> +{
> +	if ((eintid & PMIC_EINT0) == PMIC_EINT0) {

The == part is redundant here, and again this is another place where it
feels like there's duplicated code that should be using data.  All this
interrupt handling code is really extremely difficult to follow, there's
*lots* of functions all open coding many individual register bits
sometimes redundantly and it's very hard to follow what it's supposed to
be doing.  I can't help but think that in addition to making things data
driven writing more linear code without these abstraction layers would
help a lot with comprehensibility.

> +static irqreturn_t mtk_accdet_irq_handler_thread(int irq, void *data)
> +{
> +	accdet_irq_handle();
> +
> +	return IRQ_HANDLED;
> +}

Why does this wrapper function exist - AFAICT it's just introducing a
bug given that the called function is able to detect spurious interrupts
but this unconditionally reports IRQ_HANDLED.

> +int mt6359_accdet_init(struct snd_soc_component *component,
> +		       struct snd_soc_card *card)
> +{
> +	int ret = 0;
> +	struct mt63xx_accdet_data *priv =
> +			snd_soc_card_get_drvdata(component->card);

> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(mt6359_accdet_init);

This is a weird interface, what's going on here?

> +int mt6359_accdet_set_drvdata(struct snd_soc_card *card)
> +{
> +	snd_soc_card_set_drvdata(card, accdet);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(mt6359_accdet_set_drvdata);

This is setting off *massive* alarm bells in that it seems to try to
claim the card level driver data for this specific driver, again what's
going on here?

> +module_init(mt6359_accdet_soc_init);
> +module_exit(mt6359_accdet_soc_exit);

module_platform_driver()

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/2] Add mediatek MT6359 accdet driver

[Argus Lin]

Dear maintainers:
Can you reply to me if any opinion?
thanks

B.R.
Argus

On Wed, 2021-01-06 at 20:19 +0800, Argus Lin wrote:
> MT6359 audio codec support accessory detect features, the general features
> are jack plug detection and key detection.
> 
> All of 3-pole and 4-pole jack are supported.
> 
> change since v1:
>   - adds mt6359 accdet binding document
>   - adds mt6359 accdet driver
> 
> 
> Argus Lin (2):
>   dt-bindings: mediatek: mt6359: add ASoC mt6359 accdet document
>   ASoC: mediatek: mt6359: add MT6359 accdet driver
> 
>  .../devicetree/bindings/sound/mt6359-accdet.yaml   |  142 ++
>  sound/soc/codecs/Kconfig                           |    7 +
>  sound/soc/codecs/Makefile                          |    2 +
>  sound/soc/codecs/mt6359-accdet.c                   | 1951 ++++++++++++++++++++
>  sound/soc/codecs/mt6359-accdet.h                   |  136 ++
>  sound/soc/codecs/mt6359.h                          | 1863 +++++++++++++++++--
>  6 files changed, 3995 insertions(+), 106 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/sound/mt6359-accdet.yaml
>  create mode 100644 sound/soc/codecs/mt6359-accdet.c
>  create mode 100644 sound/soc/codecs/mt6359-accdet.h
> 
> --
> 1.8.1.1.dirty
> 

Re: [PATCH 0/2] Add mediatek MT6359 accdet driver

[Mark Brown]

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

On Wed, Feb 03, 2021 at 02:57:42PM +0800, Argus Lin wrote:
> Dear maintainers:
> Can you reply to me if any opinion?
> thanks

Please don't send content free pings and please allow a reasonable time
for review.  People get busy, go on holiday, attend conferences and so 
on so unless there is some reason for urgency (like critical bug fixes)
please allow at least a couple of weeks for review.  If there have been
review comments then people may be waiting for those to be addressed.

Sending content free pings adds to the mail volume (if they are seen at
all) which is often the problem and since they can't be reviewed
directly if something has gone wrong you'll have to resend the patches
anyway, so sending again is generally a better approach though there are
some other maintainers who like them - if in doubt look at how patches
for the subsystem are normally handled.

IIRC I did review this...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] dt-bindings: mediatek: mt6359: add ASoC mt6359 accdet document

[Argus Lin]

Dear maintainers:
Can you reply to me if any opinion?
thanks

B.R.
Argus

On Wed, 2021-01-06 at 20:19 +0800, Argus Lin wrote:
> This patch adds MediaTek MT6359 accdet document.
> 
> Signed-off-by: Argus Lin <argus.lin@mediatek.com>
> ---
>  .../devicetree/bindings/sound/mt6359-accdet.yaml   | 142 +++++++++++++++++++++
>  1 file changed, 142 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/sound/mt6359-accdet.yaml
> 
> diff --git a/Documentation/devicetree/bindings/sound/mt6359-accdet.yaml b/Documentation/devicetree/bindings/sound/mt6359-accdet.yaml
> new file mode 100644
> index 0000000..fde0d29
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/mt6359-accdet.yaml
> @@ -0,0 +1,142 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/sound/mt6359-accdet.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Mediatek Technologies Inc. MT6359 ASoC accdet driver bindings
> +
> +maintainers:
> +  - Chipeng Chang <chipeng.chang@mediatek.com>
> +
> +description: |
> +  This binding describes the Mediatek Technologies MT6359 ASoC
> +  accdet driver.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - mediatek,mt6359-accdet
> +      - mediatek,pmic-accdet
> +
> +  accdet-name:
> +    maxItems: 1
> +    description: named as "mt6359-accdet" for snd_soc_card_jack.
> +
> +  accdet-mic-vol:
> +    maxItems: 1
> +    description: |
> +      accdet micbias1 voltage.
> +
> +      enum:
> +        - 0x00 # 1.7v - micbias1 volage is 1.7v.
> +        - 0x01 # 1.8v - micbias1 volage is 1.8v.
> +        - 0x02 # 1.9v - micbias1 volage is 1.9v.
> +        - 0x03 # 2.0v - micbias1 volage is 2.0v.
> +        - 0x04 # 2.1v - micbias1 volage is 2.1v.
> +        - 0x05 # 2.5v - micbias1 volage is 2.5v.
> +        - 0x06 # 2.6v - micbias1 volage is 2.6v.
> +        - 0x07 # 2.7v - micbias1 volage is 2.7v.
> +        - 0x08 # 2.8v - micbias1 volage is 2.8v.
> +        - 0x09 # 2.85v - micbias1 volage is 2.85v.
> +
> +  accdet-plugout-debounce:
> +    maxItems: 1
> +    description: For using ap gpio eint only, it is sw debounce time.
> +
> +  accdet-mic-mode:
> +    maxItems: 1
> +    description: |
> +      value as 1, 2, 6 to indicate ACC/DCC mode. default is DCC mode 2.
> +
> +      enum:
> +        - 0x01 # ACC - ACC mode.
> +        - 0x02 # DCC - low cost without in bias.
> +        - 0x06 # DCC - low cost with in bias.
> +
> +  eint_use_ext_res:
> +    maxItems: 1
> +    description: select HP_EINT pull up resistance, default 0 use internal resistance.
> +
> +  headset-mode-setting:
> +    maxItems: 15
> +    description: |
> +       headset-mode-setting : Indicates customized pwm, debounce setting
> +       accdet pwm_width, pwm_thresh, fall_delay, rise_delay
> +       debounce0, debounce1, debounce3, debounce4
> +       eint pwm_width, eint pwm_thresh
> +       eint deb(debounce0, debounce1, debounce2, debounce3), inv_debounce.
> +
> +  headset-use-ap-eint:
> +    maxItems: 1
> +    description: indicates to use ap gpio.
> +
> +  headset-eint-num:
> +    maxItems: 1
> +    description: |
> +      incicates pmic eint usage.
> +
> +      enum:
> +        - 0x0 # PMIC_EINT0 - use pmic eint0 to trigger plug interrupt.
> +        - 0x1 # PMIC_EINT1 - use pmic eint1 to trigger plug interrupt.
> +        - 0x2 # PMIC_BI_EINT - use pmic eint0/1 to trigger plug interrupt.
> +
> +  headset-eint-trig-mode:
> +    maxItems: 1
> +    description: |
> +      incicates pmic eint trigger mode.
> +
> +      enum:
> +        - 0x0 # PMIC_GPIO - use pmic gpio to detect plug status by setting polarity to high or low.
> +        - 0x1 # PMIC_INVERTER - use pmic inverter to detect plug status.
> +
> +  headset-key-mode:
> +    maxItems: 1
> +    description: |
> +      incicates key mode type.
> +
> +      enum:
> +        - 0x0 # THREE_KEY - support 3-key key mode.
> +        - 0x1 # FOUR_KEY - support 4-key key mode.
> +        - 0x1 # TRI_KEY_CDD - support 3-key google CDD key mode.
> +
> +  headset-three-key-threshold:
> +    maxItems: 4
> +    description: |
> +      3 key device detection threshold: 0--MD_MAX--UP_MAX--DW_MAX.
> +
> +  headset-three-key-threshold-CDD:
> +    maxItems: 4
> +    description: |
> +       3 key CDD device detection threshold: 0--MD_MAX--UP_MAX--DW_MAX.
> +
> +  headset-four-key-threshold:
> +    maxItems: 4
> +    description: |
> +      4 key device detection threshold: 0--MD_MAX--VOICE_MAX--UP_MAX--DW_MAX.
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    accdet: accdet {
> +        compatible = "mediatek,mt6359-accdet";
> +        accdet-name = "mt6359-accdet";
> +        accdet-mic-vol = <8>;
> +        accdet-plugout-debounce = <1>;
> +        accdet-mic-mode = <2>;
> +        eint_use_ext_res = <0>;
> +        headset-mode-setting = <0x500 0x500 1 0x1f0
> +                                0x800 0x800 0x20 0x44
> +                                0x4 0x1
> +                                0x5 0x3 0x3 0x5 0xe>;
> +        headset-use-ap-eint = <0>;
> +        headset-eint-num = <0>;
> +        headset-eint-trig-mode = <1>;
> +        headset-key-mode = <0>;
> +        headset-three-key-threshold = <0 80 220 400>;
> +        headset-three-key-threshold-CDD = <0 121 192 600>;
> +        headset-four-key-threshold = <0 58 121 192 400>;
> +        status = "okay";
> +    };
> +...
> --
> 1.8.1.1.dirty
>