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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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