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()