On Tue, Apr 09, 2024 at 10:48:14AM +0800, Baojun Xu wrote:
Main source code for tas2781 driver for SPI.
...
+#ifndef __TAS2781_SPI_H__ +#define __TAS2781_SPI_H__
+ bits.h + mutex.h + time.h? (for struct tm) + types.h
struct calidata is from?..
+#include <sound/tas2781-dsp.h>
Not sure how this is being used.
Also some forward declarations:
+ struct device; + struct firmware; + struct gpio_desc; + struct regmap;
(I might missed something)
...
+#define TASDEVICE_RATES (SNDRV_PCM_RATE_44100 |\
- SNDRV_PCM_RATE_48000 | SNDRV_PCM_RATE_96000 |\
- SNDRV_PCM_RATE_88200)
For lines likes this, the formatting can be
#define TASDEVICE_RATES \ (SNDRV_PCM_RATE_44100 | SNDRV_PCM_RATE_48000 | SNDRV_PCM_RATE_96000 | \ SNDRV_PCM_RATE_88200)
which from my p.o.v. looks better.
...
+#define TAS2781_SPI_MAX_FREQ 4000000
4 * HZZ_PER_MHZ ?
...
+enum device_catlog_id {
Too generic name.
- HP = 0,
- OTHERS
Ditto. Please, add namespace.
+};
...
+#include <linux/acpi.h> +#include <linux/crc8.h> +#include <linux/crc32.h> +#include <linux/efi.h> +#include <linux/firmware.h> +#include <linux/mod_devicetable.h> +#include <linux/module.h> +#include <linux/pm_runtime.h>
+ property.h
+#include <linux/regmap.h> +#include <linux/spi/spi.h> +#include <sound/hda_codec.h> +#include <sound/soc.h> +#include <sound/tlv.h> +#include <sound/tas2781-tlv.h>
...
+/* No standard control callbacks for SNDRV_CTL_ELEM_IFACE_CARD
- Define two controls, one is Volume control callbacks, the other is
- flag setting control callbacks.
- */
/* * Follow the example of the multi-line * comments style as in this comment. Apply * it to all your comments. */
...
- .private_value = (unsigned long)&(struct soc_mixer_control) \
{.reg = xreg, .rreg = xreg, .shift = xshift, \.rshift = xshift, .min = xmin, .max = xmax, \.invert = xinvert} }
It is unreadable.
.private_value = (unsigned long)&(struct soc_mixer_control) { \ .reg = xreg, .rreg = xreg, .shift = xshift, .rshift = xshift, \ .min = xmin, .max = xmax, .invert = xinvert, \ }, \
See the difference?
Please, apply this to all twisted macros: - logical split - leaving trailing commas - better formatting
...
.range_max = 256 * 128,
Perhaps you want to define this as it's used a lot in the C and header files.
...
+static const struct regmap_config tasdevice_regmap = {
- .reg_bits = 8,
- .val_bits = 8,
- .cache_type = REGCACHE_NONE,
- .ranges = tasdevice_ranges,
- .num_ranges = ARRAY_SIZE(tasdevice_ranges),
- .max_register = 256 * 128,
+};
One too many blank lines.
+static void tas2781_spi_reset(struct tasdevice_priv *tas_dev); +static int tasdevice_spi_init(struct tasdevice_priv *tas_priv); +static int tasdevice_spi_amp_putvol(struct tasdevice_priv *tas_priv,
- struct snd_ctl_elem_value *ucontrol, struct soc_mixer_control *mc);
+static int tasdevice_spi_amp_getvol(struct tasdevice_priv *tas_priv,
- struct snd_ctl_elem_value *ucontrol, struct soc_mixer_control *mc);
+static int tasdevice_spi_digital_putvol(struct tasdevice_priv *tas_priv,
- struct snd_ctl_elem_value *ucontrol, struct soc_mixer_control *mc);
+static int tasdevice_spi_digital_getvol(struct tasdevice_priv *tas_priv,
- struct snd_ctl_elem_value *ucontrol, struct soc_mixer_control *mc);
Why do you need forward declarations here?
...
+static int tasdevice_spi_switch_book(struct tasdevice_priv *tas_priv,
- int book)
Can be on a single line.
+{
- struct tasdevice *tasdev = &tas_priv->tasdevice;
- struct regmap *map = tas_priv->regmap;
- int ret = 0;
- if (tasdev->cur_book != book) {
/* Change to page 0 before book change. */ret = regmap_write(map, TASDEVICE_PAGE_SELECT, 0);if (ret < 0) {dev_err(tas_priv->dev, "%s, E=%d\n", __func__, ret);return ret;}ret = regmap_write(map, TASDEVICE_BOOKCTL_REG, book);if (ret < 0)dev_err(tas_priv->dev, "%s, E=%d\n", __func__, ret);
Non-fatal error?
tasdev->cur_book = book;- }
- return ret;
+}
...
+int tasdevice_spi_dev_read(struct tasdevice_priv *tas_priv,
- unsigned int reg, unsigned int *val)
+{
- struct regmap *map = tas_priv->regmap;
- int ret;
- ret = tasdevice_spi_switch_book(tas_priv, TASDEVICE_BOOK_ID(reg));
- if (ret < 0)
return ret;- /*
* In our TAS2781 SPI mode, if read from other book (not book 0),* or read from page number larger than 1 in book 0, one byte more* read is needed, and first byte is a dummy byte, need to be ignored.*/- if ((TASDEVICE_BOOK_ID(reg) > 0) || (TASDEVICE_PAGE_ID(reg) > 1)) {
unsigned char data[2];ret = regmap_bulk_read(map, TASDEVICE_PGRG(reg), data, 2);
sizeof(data) ?
*val = data[1];- } else {
ret = regmap_read(map, TASDEVICE_PGRG(reg), val);- }
- if (ret < 0)
dev_err(tas_priv->dev, "%s, E=%d\n", __func__, ret);- return ret;
+} +EXPORT_SYMBOL_GPL(tasdevice_spi_dev_read);
No namespace, why?
...
- return ret;
+}
Here and in other places, one too many blank lines.
...
- static const unsigned char page_array[CALIB_MAX] = {
0x17, 0x18, 0x18, 0x13, 0x18
Leave trailing comma, can be helpful in the future.
- };
- static const unsigned char rgno_array[CALIB_MAX] = {
0x74, 0x0c, 0x14, 0x70, 0x7c
Ditto.
- };
...
- data = tas_priv->cali_data.data +
tas_priv->index * TASDEVICE_SPEAKER_CALIBRATION_SIZE;- for (j = 0; j < CALIB_MAX; j++) {
rc = tasdevice_spi_dev_bulk_write(tas_priv,TASDEVICE_REG(0, page_array[j], rgno_array[j]),&(data[4 * j]), 4);if (rc < 0)dev_err(tas_priv->dev,"chn %d calib %d bulk_wr err = %d\n",tas_priv->index, j, rc);
The indentation here and in some other places is a mess. Please, take your time to split these in more readable way.
- }
Again, too many unneeded blank lines in the code.
+}
...
I stopped here as there are already enough for next version.