Re: [PATCH v6 1/4] ASoc: PCM6240: Create PCM6240 Family driver code

[Mark Brown]

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?

Attachment: signature.asc
Description: PGP signature