Received-SPF: pass (google.com: domain of linux-kernel+bounces-136929-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Tue, 9 Apr 2024 16:21:27 +0300
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To: Baojun Xu <baojun.xu@ti.com>
Cc: tiwai@suse.de, robh+dt@kernel.org, lgirdwood@gmail.com, perex@perex.cz,
	pierre-louis.bossart@linux.intel.com, kevin-lu@ti.com,
	shenghao-ding@ti.com, navada@ti.com, alsa-devel@alsa-project.org,
	linux-kernel@vger.kernel.org, liam.r.girdwood@intel.com,
	yung-chuan.liao@linux.intel.com, broonie@kernel.org, soyer@irl.hu
Subject: Re: [PATCH v2 2/3] ALSA: hda/tas2781: Main code of tas2781 driver
 for SPI
Message-ID: <ZhVA1_HTET4Q4T9d@smile.fi.intel.com>
References: <20240409024816.1180-1-baojun.xu@ti.com>
 <20240409024816.1180-3-baojun.xu@ti.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240409024816.1180-3-baojun.xu@ti.com>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo

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