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

* Re: [PATCH 2/2] ASoC: mediatek: mt6359: add MT6359 accdet driver
       [not found] ` <1609935546-11722-3-git-send-email-argus.lin@mediatek.com>
@ 2021-01-15 17:07   ` Mark Brown
  0 siblings, 0 replies; 4+ messages in thread
From: Mark Brown @ 2021-01-15 17:07 UTC (permalink / raw)
  To: Argus Lin
  Cc: Liam Girdwood, Rob Herring, Matthias Brugger, Jaroslav Kysela,
	Takashi Iwai, Pierre-Louis Bossart, Geert Uytterhoeven,
	Srinivas Kandagatla, Charles Keepax, Arnd Bergmann, Jack Yu,
	Shuming Fan, Dan Murphy, Lars-Peter Clausen, Alexandre Belloni,
	Jiaxin Yu, Tzung-Bi Shih, Shane.Chien, Chipeng Chang, alsa-devel,
	wsd_upstream, devicetree, linux-kernel, linux-arm-kernel,
	linux-mediatek

[-- 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 --]

^ permalink raw reply	[flat|nested] 4+ messages in thread