Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <bf84b93c-0b24-99f6-9c70-2f4c677cff18@linaro.org>
Date:   Tue, 20 Dec 2022 17:07:07 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.6.0
Subject: Re: [PATCH] ALSA SoC: Texas Instruments TAS2781 Audio Smart Amp
Content-Language: en-US
To:     Kevin Lu <luminlong@139.com>, lgirdwood@gmail.com,
        broonie@kernel.org, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org
Cc:     alsa-devel@alsa-project.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, shenghao-ding@ti.com,
        kevin-lu@ti.com, navada@ti.com, peeyush@ti.com
References: <20221220144203.2194-1-luminlong@139.com>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <20221220144203.2194-1-luminlong@139.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 20/12/2022 15:42, Kevin Lu wrote:
> The TAS2781 driver implements a flexible and configurable register setting
> for one, two, even multiple TAS2781 chips. All the register setting are in
> a bin file. Almost no specific register setting can be found in the code.
> 
> Signed-off-by: Kevin Lu <luminlong@139.com>
> ---
>  sound/soc/codecs/Kconfig       |   13 +
>  sound/soc/codecs/Makefile      |    2 +
>  sound/soc/codecs/tas2781-dsp.c | 2483 ++++++++++++++++++++++++++++++++
>  sound/soc/codecs/tas2781-dsp.h |  213 +++
>  sound/soc/codecs/tas2781-i2c.c | 2143 +++++++++++++++++++++++++++
>  sound/soc/codecs/tas2781.h     |  208 +++
>  6 files changed, 5062 insertions(+)
>  create mode 100644 sound/soc/codecs/tas2781-dsp.c
>  create mode 100644 sound/soc/codecs/tas2781-dsp.h
>  create mode 100644 sound/soc/codecs/tas2781-i2c.c
>  create mode 100644 sound/soc/codecs/tas2781.h

Your threading and patch formatting is broken. Read before sending patches:
https://elixir.bootlin.com/linux/v5.19-rc5/source/Documentation/process/submitting-patches.rst

> 
> diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
> index 7022e6286..31d2d9594 100644
> --- a/sound/soc/codecs/Kconfig
> +++ b/sound/soc/codecs/Kconfig
> @@ -222,6 +222,7 @@ config SND_SOC_ALL_CODECS
>  	imply SND_SOC_TAS2764
>  	imply SND_SOC_TAS2770
>  	imply SND_SOC_TAS2780
> +	imply SND_SOC_TAS2781
>  	imply SND_SOC_TAS5086
>  	imply SND_SOC_TAS571X
>  	imply SND_SOC_TAS5720
> @@ -1573,6 +1574,18 @@ config SND_SOC_TAS2780
>  	  Enable support for Texas Instruments TAS2780 high-efficiency
>  	  digital input mono Class-D audio power amplifiers.
>  

(...)

> +static struct i2c_driver tasdevice_i2c_driver = {
> +	.driver = {
> +		.name = "tas2781-codec",
> +		.owner = THIS_MODULE,
> +		.of_match_table = of_match_ptr(tasdevice_of_match),
> +		.pm = &tasdevice_pm_ops,
> +	},
> +	.probe	= tasdevice_i2c_probe,
> +	.remove = tasdevice_i2c_remove,
> +	.id_table = tasdevice_id,
> +};
> +
> +module_i2c_driver(tasdevice_i2c_driver);
> +
> +MODULE_AUTHOR("Shenghao Ding <shenghao-ding@ti.com>");
> +MODULE_AUTHOR("Kevin Lu <kevin-lu@ti.com>");
> +MODULE_DESCRIPTION("ASoC TAS2781 Driver");
> +MODULE_LICENSE("GPL");
> diff --git a/sound/soc/codecs/tas2781.h b/sound/soc/codecs/tas2781.h
> new file mode 100644
> index 000000000..db4ec752b
> --- /dev/null
> +++ b/sound/soc/codecs/tas2781.h
> @@ -0,0 +1,208 @@
> +/* SPDX-License-Identifier: GPL-2.0
> + *
> + * ALSA SoC Texas Instruments TAS2781 Audio Smart Amplifier
> + *
> + * Copyright (C) 2022 - 2023 Texas Instruments Incorporated
> + * https://www.ti.com
> + *
> + * The TAS2781 driver implements a flexible and configurable register setting
> + * for one, two, even multiple TAS2781 chips. All the register setting are in
> + * a bin file. Almost no specific register setting can be found in the code.
> + *
> + * Author: Shenghao Ding <shenghao-ding@ti.com>
> + *         Kevin Lu <kevin-lu@ti.com>
> + */
> +
> +#ifndef __TAS2781_H__
> +#define __TAS2781_H__
> +
> +#include "tas2781-dsp.h"
> +
> +#define SMARTAMP_MODULE_NAME	("tas2781")

This does not belong to headers.

> +#define MAX_LENGTH				(128)

Why () ?

> +
> +#define TASDEVICE_RETRY_COUNT	(3)
> +#define TASDEVICE_ERROR_FAILED	(-2)
> +
> +#define TASDEVICE_RATES	(SNDRV_PCM_RATE_44100 |\
> +	SNDRV_PCM_RATE_48000 | SNDRV_PCM_RATE_96000 |\
> +	SNDRV_PCM_RATE_88200)
> +#define TASDEVICE_MAX_CHANNELS (8)
> +
> +#define TASDEVICE_FORMATS	(SNDRV_PCM_FMTBIT_S16_LE | \
> +	SNDRV_PCM_FMTBIT_S24_LE | \
> +	SNDRV_PCM_FMTBIT_S32_LE)
> +
> +/*PAGE Control Register (available in page0 of each book) */

Missing space

> +#define TASDEVICE_PAGE_SELECT	(0x00)
> +#define TASDEVICE_BOOKCTL_PAGE	(0x00)
> +#define TASDEVICE_BOOKCTL_REG	(127)
> +#define TASDEVICE_BOOK_ID(reg)		(reg / (256 * 128))
> +#define TASDEVICE_PAGE_ID(reg)		((reg % (256 * 128)) / 128)
> +#define TASDEVICE_PAGE_REG(reg)		((reg % (256 * 128)) % 128)
> +#define TASDEVICE_REG(book, page, reg)	(((book * 256 * 128) + \
> +					(page * 128)) + reg)
> +
> +	/*Software Reset */

Wrong coding style.

> +#define TAS2871_REG_SWRESET  TASDEVICE_REG(0x0, 0X0, 0x02)
> +#define TAS2871_REG_SWRESET_RESET  (0x1 << 0)
> +
> +	/* Enable Global addresses */

Wrong coding style.

> +#define TAS2871_MISC_CFG2  TASDEVICE_REG(0x0, 0X0, 0x07)
> +#define TAS2871_GLOBAL_MASK (0x1 << 1)
> +
> +#define SMS_HTONS(a, b)  ((((a)&0x00FF)<<8) | \
> +				((b)&0x00FF))
> +#define SMS_HTONL(a, b, c, d) ((((a)&0x000000FF)<<24) |\
> +					(((b)&0x000000FF)<<16) | \
> +					(((c)&0x000000FF)<<8) | \
> +					((d)&0x000000FF))
> +
> +	/*I2C Checksum */
> +#define TASDEVICE_I2CChecksum  TASDEVICE_REG(0x0, 0x0, 0x7E)
> +
> +	/* Volume control */
> +#define TAS2781_DVC_LVL TASDEVICE_REG(0x0, 0x0, 0x1A)
> +#define TAS2781_AMP_LEVEL TASDEVICE_REG(0x0, 0x0, 0x03)
> +#define TAS2781_AMP_LEVEL_MASK GENMASK(5, 1)
> +
> +#define TASDEVICE_CMD_SING_W  (0x1)
> +#define TASDEVICE_CMD_BURST  (0x2)
> +#define TASDEVICE_CMD_DELAY  (0x3)
> +#define TASDEVICE_CMD_FIELD_W  (0x4)
> +
> +enum audio_device {
> +	GENERAL_AUDEV = 0,
> +	TAS2781	  = 1,
> +};
> +
> +struct smartpa_gpio_info {
> +	unsigned char ndev;
> +	int mnResetGpio[MaxChn];
> +};
> +
> +#define SMS_HTONS(a, b)  ((((a)&0x00FF)<<8) | ((b)&0x00FF))
> +#define SMS_HTONL(a, b, c, d) ((((a)&0x000000FF)<<24) | \
> +	(((b)&0x000000FF)<<16) | (((c)&0x000000FF)<<8) | \
> +	((d)&0x000000FF))
> +
> +struct Tbookpage {
> +	unsigned char mnBook;
> +	unsigned char mnPage;
> +};
> +
> +struct Ttasdevice {
> +	unsigned int mnDevAddr;
> +	unsigned int mnErrCode;
> +	short mnCurrentProgram;
> +	short mnCurrentConfiguration;
> +	short mnCurrentRegConf;
> +	int mnIRQGPIO;
> +	int mnResetGpio;
> +	int mn_irq;
> +	int PowerStatus;
> +	bool bDSPBypass;
> +	bool bIrq_enabled;
> +	bool bLoading;
> +	bool bLoaderr;
> +	bool mbCalibrationLoaded;
> +	struct Tbookpage mnBkPg;
> +	struct TFirmware *mpCalFirmware;
> +};
> +
> +struct global_addr {
> +	struct Tbookpage mnBkPg;
> +	unsigned int mnDevAddr;
> +	int ref_cnt;
> +};
> +
> +struct tas_control {
> +	struct snd_kcontrol_new *tasdevice_profile_controls;
> +	int nr_controls;
> +};
> +
> +struct tasdevice_irqinfo {
> +	int mn_irq_gpio;
> +	int mn_irq;
> +	struct delayed_work irq_work;
> +	bool mb_irq_enable;
> +};
> +
> +struct tasdevice_priv {
> +	struct device *dev;
> +	void *client;
> +	struct regmap *regmap;
> +	struct mutex codec_lock;
> +	struct mutex dev_lock;
> +	struct Ttasdevice tasdevice[MaxChn];
> +	struct TFirmware *mpFirmware;
> +	struct tasdevice_regbin mtRegbin;
> +	struct smartpa_gpio_info mtRstGPIOs;
> +	struct tasdevice_irqinfo mIrqInfo;
> +	struct tas_control tas_ctrl;
> +	struct global_addr glb_addr;
> +	int mnCurrentProgram;
> +	int mnCurrentConfiguration;
> +	unsigned int chip_id;
> +	int (*read)(struct tasdevice_priv *tas_dev, enum channel chn,
> +		unsigned int reg, unsigned int *pValue);
> +	int (*write)(struct tasdevice_priv *tas_dev, enum channel chn,
> +		unsigned int reg, unsigned int Value);
> +	int (*bulk_read)(struct tasdevice_priv *tas_dev, enum channel chn,
> +		unsigned int reg, unsigned char *pData, unsigned int len);
> +	int (*bulk_write)(struct tasdevice_priv *tas_dev, enum channel chn,
> +		unsigned int reg, unsigned char *pData, unsigned int len);
> +	int (*set_calibration)(void *tas_dev, enum channel chl,
> +		int calibration);
> +	void (*tas2781_reset)(struct tasdevice_priv *tas_dev);
> +	void (*tas2781_set_global)(struct tasdevice_priv *tas_dev);
> +	int (*fw_parse_variable_header)(struct tasdevice_priv *tas_dev,
> +		const struct firmware *pFW, int offset);
> +	int (*fw_parse_program_data)(struct tasdevice_priv *tas_dev,
> +		struct TFirmware *pFirmware,
> +		const struct firmware *pFW, int offset);
> +	int (*fw_parse_configuration_data)(struct tasdevice_priv *tas_dev,
> +		struct TFirmware *pFirmware,
> +		const struct firmware *pFW, int offset);
> +	int (*tasdevice_load_block)(struct tasdevice_priv *tas_dev,
> +		struct TBlock *pBlock);
> +	int (*fw_parse_calibration_data)(struct tasdevice_priv *tas_dev,
> +		struct TFirmware *pFirmware,
> +		const struct firmware *pFW, int offset);
> +	void (*irq_work_func)(struct tasdevice_priv *tas_dev);
> +	int fw_state;
> +	unsigned int magic_num;
> +	unsigned char ndev;
> +	unsigned char dev_name[32];
> +	unsigned char regbin_binaryname[64];
> +	unsigned char dsp_binaryname[64];
> +	unsigned char cal_binaryname[MaxChn][64];
> +	bool mb_runtime_suspend;
> +	struct delayed_work powercontrol_work;
> +	void *codec;
> +	int sysclk;
> +	int pstream;
> +	int cstream;
> +	bool mbCalibrationLoaded;

You need to adjust your coding style to Linux. This is some C++ crap.


Best regards,
Krzysztof