Received-SPF: pass (google.com: domain of linux-kernel+bounces-133116-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Fri, 5 Apr 2024 15:09:46 +0100
From: Mark Brown <broonie@kernel.org>
To: Shenghao Ding <shenghao-ding@ti.com>
Cc: linux-kernel@vger.kernel.org, lgirdwood@gmail.com, robh+dt@kernel.org,
	krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org,
	linux-sound@vger.kernel.org, devicetree@vger.kernel.org,
	perex@perex.cz, tiwai@suse.com, 13916275206@139.com,
	mohit.chawla@ti.com, soyer@irl.hu, jkhuang3@ti.com, tiwai@suse.de,
	pdjuandi@ti.com, manisha.agrawal@ti.com, aviel@ti.com,
	hnagalla@ti.com, praneeth@ti.com, Baojun.Xu@fpt.com
Subject: Re: [PATCH v8 1/4] ASoc: PCM6240: Create PCM6240 Family driver code
Message-ID: <dc5dad72-09cc-4001-afdc-1cd92cac312e@sirena.org.uk>
References: <20240403003159.389-1-shenghao-ding@ti.com>
 <20240403003159.389-2-shenghao-ding@ti.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="zE+8122Hjp+Flwjb"
Content-Disposition: inline
In-Reply-To: <20240403003159.389-2-shenghao-ding@ti.com>


--zE+8122Hjp+Flwjb
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Wed, Apr 03, 2024 at 08:31:55AM +0800, Shenghao Ding wrote:

> +static const char *const pcmdev_ctrl_name[] = {
> +	"%s-i2c-%d-dev%d-ch%d-ana-gain",
> +	"%s-i2c-%d-dev%d-ch%d-digi-gain",
> +	"%s-i2c-%d-dev%d-ch%d-fine-gain",
> +};
> +
> +static const char *const pcmdev_ctrl_name_with_prefix[] = {
> +	"%s-dev%d-ch%d-ana-gain",
> +	"%s-dev%d-ch%d-digi-gain",
> +	"%s-dev%d-ch%d-fine-gain",
> +};

These still don't look like idiomatic ALSA control names, if nothing
else volume controls should end with Volume but the whole all lower case
with -s thing really isn't idiomatic.

> +static int pcmdev_put_volsw(struct snd_kcontrol *kcontrol,
> +	struct snd_ctl_elem_value *ucontrol, int vol_ctrl_type)
> +{

> +	err = pcmdev_dev_update_bits(pcm_dev, dev_no, reg, val_mask, val);
> +	if (err) {
> +		dev_err(pcm_dev->dev, "%s: update_bits err = %d\n",
> +			__func__, err);
> +		err = 0;
> +	}

Why don't we report errors to users?

> +		case PCMDEVICE_CMD_DELAY: {
> +			unsigned int delay_time = 0;
> +
> +			if (subblk_offset + 2 > sublocksize) {
> +				dev_err(pcm_dev->dev,
> +					"%s: deley out of boundary\n",
> +					__func__);
> +				break;
> +			}
> +			delay_time = get_unaligned_be16(&data[2]) * 1000;
> +			usleep_range(delay_time, delay_time + 50);
> +			subblk_offset += 2;
> +		}
> +			break;
> +		case PCMDEVICE_CMD_FIELD_W:
> +		if (subblk_offset + 6 > sublocksize) {
> +			dev_err(pcm_dev->dev,
> +				"%s: bit write out of memory\n", __func__);
> +			break;
> +		}
> +		ret = pcmdev_dev_update_bits(pcm_dev, chn,
> +			PCMDEVICE_REG(data[subblk_offset + 3],
> +			data[subblk_offset + 4]), data[subblk_offset + 1],
> +			data[subblk_offset + 5]);

The indentation here is all messed up, things not indented for the case
blocks and so on.

--zE+8122Hjp+Flwjb
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQEzBAABCgAdFiEEreZoqmdXGLWf4p/qJNaLcl1Uh9AFAmYQBikACgkQJNaLcl1U
h9Bstgf/fZgVu4w8dXZljCEZ/q4J2JEc9LqY60UgCz5OG6rDlq7Vngmm398tJIiX
L+9sNepPJRkNh1QGaujU/yK/aB9WBkiQqEKAPmspBYbqDf1ypAPjOtP8pJeKiHc3
MyLJ/NiblAMt3+rdt23gLWhy2LV6tLAj+b/sM09vwSCVy0eqnkt+R7awszCsxb7k
ZmZ2I4JCC41iqzZlWg+8qgJTiVD8JfOHhINxj0wuYwqwtdQOqGj/sVsv3e0spq6L
aZJs4WRnnFKJy8jkrCs4TIGe2qyqnBHECYd+68hdjwuoWIjoVPqpceAIjD/HhIPG
XXfAKY4FRSJONbDd4A5685V5ctPODg==
=prZg
-----END PGP SIGNATURE-----

--zE+8122Hjp+Flwjb--