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; -- With Best Regards, Andy Shevchenko