Received-SPF: pass (google.com: domain of linux-kernel+bounces-56634-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Wed, 7 Feb 2024 16:40:38 +0200
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To: Shenghao Ding <shenghao-ding@ti.com>
Cc: broonie@kernel.org, lgirdwood@gmail.com, perex@perex.cz,
	pierre-louis.bossart@linux.intel.com, 13916275206@139.com,
	alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org,
	liam.r.girdwood@intel.com, bard.liao@intel.com,
	mengdong.lin@intel.com, yung-chuan.liao@linux.intel.com,
	baojun.xu@ti.com, kevin-lu@ti.com, navada@ti.com, tiwai@suse.de,
	soyer@irl.hu
Subject: Re: [PATCH v7] ASoc: tas2783: Add tas2783 codec driver
Message-ID: <ZcOWZlXu1fL_haFU@smile.fi.intel.com>
References: <20240207054743.1504-1-shenghao-ding@ti.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240207054743.1504-1-shenghao-ding@ti.com>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo

On Wed, Feb 07, 2024 at 01:47:42PM +0800, Shenghao Ding wrote:
> The tas2783 is a smart audio amplifier with integrated MIPI SoundWire
> interface (Version 1.2.1 compliant), I2C, and I2S/TDM interfaces designed
> for portable applications. An on-chip DSP supports Texas Instruments
> SmartAmp speaker protection algorithm. The integrated speaker voltage and
> current sense provides for real-time monitoring of lodspeakers.
> 
> The ASoC component provides the majority of the functionality of the
> device, all the audio functions.

..

> +#include <linux/crc32.h>
> +#include <linux/efi.h>
> +#include <linux/err.h>
> +#include <linux/firmware.h>
> +#include <linux/init.h>
> +#include <linux/module.h>

> +#include <linux/of.h>

Unused header. Maybe you use it as a "proxy"? Don't do this, include what you
use directly (with some exceptions when we know that one header guarantees to
include another).

> +#include <linux/pm.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> +#include <linux/soundwire/sdw.h>
> +#include <linux/soundwire/sdw_registers.h>
> +#include <linux/soundwire/sdw_type.h>
> +#include <sound/pcm_params.h>
> +#include <sound/sdw.h>
> +#include <sound/soc.h>
> +#include <sound/tlv.h>

+ Blank line?

> +#include <sound/tas2781-tlv.h>
> +
> +#include "tas2783.h"

..

> +		/* Only reset register was volatiled.
> +		 * Software reset. Bit is self clearing.
> +		 * 0b = Don't reset
> +		 * 1b = reset
> +		 */

/*
 * The above style of the multi-line comment is used
 * solely by net subsystem. Please, fix all comments
 * in your driver accordingly.
 */

..

> +static const struct regmap_config tasdevice_regmap = {
> +	.reg_bits = 32,
> +	.val_bits = 8,
> +	.readable_reg = tas2783_readable_register,
> +	.volatile_reg = tas2783_volatile_register,

> +	.max_register = 0x44ffffff,

I'm always wondering how this can work in debugfs when one tries to dump all
registers...

> +	.reg_defaults = tas2783_reg_defaults,
> +	.num_reg_defaults = ARRAY_SIZE(tas2783_reg_defaults),
> +	.cache_type = REGCACHE_RBTREE,
> +	.use_single_read = true,
> +	.use_single_write = true,
> +};

..

> +static int tasdevice_clamp(int val, int max, unsigned int invert)
> +{
> +	/* Keep in valid area, out of range value don't care. */
> +	if (val > max)
> +		val = max;
> +	if (invert)
> +		val = max - val;
> +	if (val < 0)
> +		val = 0;
> +	return val;

Can it be as simple as

	val = clamp(val, 0, max);
	if (invert)
		return max - val;
	return val;

?

> +}

..

> +		dev_err(tas_dev->dev, "%s, wrong parameter.\n", __func__);

Usually using __func__ in error messages signals about them being poorly
written.

..

> +		dev_err(tas_dev->dev, "%s, get digital vol error %x.\n",
> +			__func__, ret);

Like here, you repeat __func__ contents in the message itself.

..

> +	mask = (1 << fls(mc->max)) - 1;

Wouldn't roundup_pow_of_two() or roundown_pow_of_two() abe more explicit?

..

> +	mask = (1 << fls(mc->max)) - 1;

Ditto.

..

> +	reg_start = (u8 *)(cali_data + (tas_dev->sdw_peripheral->id.unique_id
> +		- TAS2783_ID_MIN) * sizeof(tas2783_cali_reg));

Strange indentation.

> +	for (int i = 0; i < ARRAY_SIZE(tas2783_cali_reg); i++) {
> +		ret = regmap_bulk_write(map, tas2783_cali_reg[i],
> +			&reg_start[4 * i], 4);

Ditto.

> +		if (ret) {
> +			dev_err(tas_dev->dev, "Cali failed %x:%d\n",
> +				tas2783_cali_reg[i], ret);
> +			break;
> +		}
> +	}

..

> +	if (status != 0) {

if (status)

> +		/* Failed got calibration data from EFI. */
> +		dev_dbg(tas_dev->dev, "No calibration data in UEFI.");
> +		return 0;
> +	}

..

> +		/* Date and time of calibration was done. */
> +		time64_to_tm(tmp_val[20], 0, tm);
> +		dev_dbg(tas_dev->dev, "%4ld-%2d-%2d, %2d:%2d:%2d\n",
> +			tm->tm_year, tm->tm_mon, tm->tm_mday,
> +			tm->tm_hour, tm->tm_min, tm->tm_sec);

Use respective %pt

..

> +	img_sz = le32_to_cpup((__le32 *)&buf[offset]);

Potentially broken alignment. In any case this code is bad.
Use get_unaligned_le32() instead.

..

> +		dev_err(tas_dev->dev, "Size not matching, %d %u",
> +			(int)fmw->size, img_sz);

No castings in printf(). It's rarely when you need one. Here is just an
indication of mistype.

..

> +		if (ret != 0) {

		if (ret)

> +			dev_dbg(tas_dev->dev, "Load FW fail: %d.\n", ret);
> +			goto out;
> +		}
> +		offset += sizeof(unsigned int)*5 + p->length;

Missing spaces around '*'. And why magic number? What is it meaning?

..

> +	value_sdw |= ((tas_dev->sdw_peripheral->dev_num & 1) << 4);

Outer parentheses are not needed, perhaps BIT(0) instead of 1 will
be better to understand?

> +static const struct snd_soc_dapm_widget tasdevice_dapm_widgets[] = {
> +	SND_SOC_DAPM_AIF_IN("ASI", "ASI Playback", 0, SND_SOC_NOPM, 0, 0),
> +	SND_SOC_DAPM_AIF_OUT("ASI OUT", "ASI Capture", 0, SND_SOC_NOPM,
> +		0, 0),
> +	SND_SOC_DAPM_OUTPUT("OUT"),
> +	SND_SOC_DAPM_INPUT("DMIC")

Leave trailing comma as it's not a terminator.

> +};
> +
> +static const struct snd_soc_dapm_route tasdevice_audio_map[] = {
> +	{"OUT", NULL, "ASI"},
> +	{"ASI OUT", NULL, "DMIC"}

Ditto.

> +};

..

> +	dev_dbg(dai->dev, "%s %s", __func__, dai->name);

__func__ in dev_dbg() makes a little sense as we may enable it dynamically
(when Dynamic Debug is on). Generally speaking no debug messages should use
__LINE__, __FILE__, or __func__ in the modern kernel code.

..

> +	scnprintf(tas_dev->rca_binaryname, 64, "tas2783-%d-%x.bin",

sizeof() ?

> +		tas_dev->sdw_peripheral->bus->link_id,
> +		tas_dev->sdw_peripheral->id.unique_id);

..

> +out:

Useless label, you can return directly.

> +	return ret;

..

> +out:
> +	return ret;

Ditto.

..

> +	struct  tasdevice_priv *tas_priv = dev_get_drvdata(&slave->dev);

Too many spaces.

..

> +	tas_dev->regmap = devm_regmap_init_sdw(peripheral,
> +		&tasdevice_regmap);

One line?

> +	if (IS_ERR(tas_dev->regmap)) {
> +		ret = PTR_ERR(tas_dev->regmap);
> +		dev_err(dev, "Failed %d of devm_regmap_init_sdw.", ret);
> +	} else
> +		ret = tasdevice_init(tas_dev);
> +
> +	return ret;

	if (IS_ERR(tas_dev->regmap))
		return dev_err_probe(dev, PTR_ERR(tas_dev->regmap),
				     "Failed devm_regmap_init_sdw.");

	return tasdevice_init(tas_dev);

..


> +static int tasdevice_sdw_remove(struct sdw_slave *peripheral)
> +{
> +	struct tasdevice_priv *tas_dev = dev_get_drvdata(&peripheral->dev);
> +
> +	if (tas_dev->first_hw_init)
> +		pm_runtime_disable(tas_dev->dev);
> +
> +	pm_runtime_put_noidle(tas_dev->dev);
> +	return 0;

Are you sure this is correct order of calls as we have a lot of cleaning up
happening here?

> +}

..

> +static const struct sdw_device_id tasdevice_sdw_id[] = {
> +	SDW_SLAVE_ENTRY(0x0102, 0x0000, 0),
> +	{},

No comma for the terminator line.

> +};

> +

Unneeded blank line.

> +MODULE_DEVICE_TABLE(sdw, tasdevice_sdw_id);

..

> +#define TAS2783_PROBE_TIMEOUT 5000

Missing units suffix (_US? _MS?)

> +static int __maybe_unused tas2783_sdca_dev_resume(struct device *dev)

No new code should use __maybe_unused for PM callbacks. Use pm_ptr() and
respective new PM macros.

..

> +static const struct dev_pm_ops tas2783_sdca_pm = {
> +	SET_SYSTEM_SLEEP_PM_OPS(tas2783_sdca_dev_suspend,
> +		tas2783_sdca_dev_resume)
> +	SET_RUNTIME_PM_OPS(tas2783_sdca_dev_suspend,
> +		tas2783_sdca_dev_resume, NULL)
> +};

Use new PM macros.

> +static struct sdw_driver tasdevice_sdw_driver = {
> +	.driver = {
> +		.name = "slave-tas2783",
> +		.pm = &tas2783_sdca_pm,
> +	},
> +	.probe = tasdevice_sdw_probe,
> +	.remove = tasdevice_sdw_remove,
> +	.ops = &tasdevice_sdw_ops,
> +	.id_table = tasdevice_sdw_id,
> +};

> +

Unneeded blank line.

> +module_sdw_driver(tasdevice_sdw_driver);

..

> +#ifndef __TAS2783_H__
> +#define __TAS2783_H__

+ linux/bits.h
+ linux/time.h
+ linux/types.h

+ sound/pcm.h

and so on, use IWYU (include what you use) principle.

Note, for the pointers you may use forward declarations, like

struct device;
struct regmap;

struct snd_soc_component;

-- 
With Best Regards,
Andy Shevchenko