Re: [PATCH v2 2/3] ALSA: hda/tas2781: Main code of tas2781 driver for SPI

Andy Shevchenko
2024-04-09T13:21:35Z

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