Re: [PATCH v2 2/3] ALSA: hda/tas2781: Main code of tas2781 driver for SPI
From: Andy Shevchenko
Date: Tue Apr 09 2024 - 09:21:49 EST
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.
--
With Best Regards,
Andy Shevchenko