Received-SPF: pass (google.com: domain of linux-kernel+bounces-83533-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Tue, 27 Feb 2024 15:46:21 +0000
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, s-hari@ti.com,
	aviel@ti.com, hnagalla@ti.com, praneeth@ti.com
Subject: Re: [PATCH v6 1/4] ASoc: PCM6240: Create PCM6240 Family driver code
Message-ID: <0cba2632-5d79-488a-9eec-dbed4208971b@sirena.org.uk>
References: <20240223123809.503-1-shenghao-ding@ti.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="q/NIaDdKYiyeWvDU"
Content-Disposition: inline
In-Reply-To: <20240223123809.503-1-shenghao-ding@ti.com>


--q/NIaDdKYiyeWvDU
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Fri, Feb 23, 2024 at 08:38:04PM +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",
> +};

So, I see why you're doing this naming thing for the per-device controls
- the device can (and is designed to) allow controlling multiple amps
with a single I2C write.  However this is resulting in something that's
really awkward from an ALSA point of view, the names that are being
generated are very much not idiomatic for control names and probably
aren't going to be terribly meaningful for end users since they're not
associated with where the relevant outputs physically are.  We could
require machines to provide names for everything but that's probably
going to be a miserable experience on x86 and is unhelpful even with DT
so having a default seems to make sense.

I think these should look more like ALSA names, so "I2C %d Device %d..."
though that does make things a bit longer.

> +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",
> +};

For these I'm not clear why we don't just use the name, what's the goal
in having the device number in there?

I think it would make sense to do a version the driver with no user
visible controls (or at least none that need this name generation stuff)
and then make the controls an incremental patch, the driver would
obviously need both bits to be properly useful but it'd mean that the
simple bit could get reviewed and possibly merged separately to the
complicated bit which would probably be easier.

Please also send the output of mixer-test from a machine with this
driver in it as part of the cover letter, it'll make it easier to tell
what's going on.

> +	if (comp->name_prefix) {
> +	/* There's name_prefix defined in DTS. Bin file name will be
> +	 * name_prefix.bin stores the firmware including register setting and
> +	 * params for different filters inside chips, it must be copied into

The comment should be indented like it's inside the if ().

> +	} else {
> +	/* There's NO name_prefix defined in DTS. Bin file name will be
> +	 * device-name[defined in pcmdevice_i2c_id]-i2c-bus_id[0,1,...,N]-
> +	 * sum[1,...,4]dev.bin stores the firmware including register setting
> +	 * and params for different filters inside chips, it must be copied
> +	 * into firmware folder. The same types of pcmdevices sitting on the
> +	 * same i2c bus will be aggregated as one single codec, all of them
> +	 * share the same bin file.
> +	 */
> +		scnprintf(pcm_dev->bin_name, PCMDEVICE_BIN_FILENAME_LEN,
> +			"%s-i2c-%d-%udev.bin", pcm_dev->dev_name, adap->nr,
> +			pcm_dev->ndev);
> +	}

You could also do this as a fallback (ie, try the specified name first
then fall bakc to the bus based numbering), that way if a name is added
later it won't break people's installs on upgrade.

> +static const struct regmap_config pcmdevice_i2c_regmap = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.cache_type = REGCACHE_MAPLE,
> +	.ranges = pcmdevice_ranges,
> +	.num_ranges = ARRAY_SIZE(pcmdevice_ranges),
> +	.max_register = 256 * 128,
> +};

Shouldn't there be some volatile registers if we have interrupts?

--q/NIaDdKYiyeWvDU
Content-Type: application/pgp-signature; name="signature.asc"

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

iQEzBAABCgAdFiEEreZoqmdXGLWf4p/qJNaLcl1Uh9AFAmXeA8wACgkQJNaLcl1U
h9C3Hwf9Gmdxf4Cz4ZF49QuHRx9unEzbCoT6YJxHKoeWCY8Dv+nxvafUfp8NqeXG
eGHw314ytCKdPKj65J0ef3jupWrB3Z4euByOjCBvtRRriVqmCPMdhaCVFA8kKr9U
SyiqAm1z9t9LCeKVcFcKsh6Bc8H+oqx4E1z7IJ91TUhfD+1kK/c2iFyIDb1hjZis
1TXj1840PpqIH62vmfWeYv08SpEKNryODarBU/DZXyDtNewADHC2MLYOJF29PMhb
GeGwOckFtxAwHkkrtwAkaM6V9bRSQ843E4UmDMH60Cm6XlweK32rP1TGsrUR7llO
4vWck8vZ4ivdI5DSDA9rN/KXP62dWA==
=J6Xo
-----END PGP SIGNATURE-----

--q/NIaDdKYiyeWvDU--