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],®_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;