linux-kernel - Re: [PATCH v4 1/4] ASoc: PCM6240: Create PCM6240 Family driver code

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <160173b0-098e-493f-93b1-8b831838e0a0@linux.intel.com>
Date: Thu, 8 Feb 2024 08:38:04 -0600
From: Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
To: Shenghao Ding <shenghao-ding@...com>, broonie@...nel.org,
 conor+dt@...nel.org, krzysztof.kozlowski@...aro.org,
 devicetree@...r.kernel.org, robh+dt@...nel.org,
 andriy.shevchenko@...ux.intel.com, linux-sound@...r.kernel.org,
 liam.r.girdwood@...el.com, lgirdwood@...il.com, linux-kernel@...r.kernel.org
Cc: kevin-lu@...com, baojun.xu@...com, v-po@...com, navada@...com,
 perex@...ex.cz, j-mcpherson@...com, 13916275206@....com,
 mohit.chawla@...com, soyer@....hu, jkhuang3@...com, tiwai@...e.de,
 pdjuandi@...com, manisha.agrawal@...com, s-hari@...com, aviel@...com,
 hnagalla@...com, praneeth@...com
Subject: Re: [PATCH v4 1/4] ASoc: PCM6240: Create PCM6240 Family driver code




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

Controls are exposed to user-space, and it helps if it's easy to
identify which device is which.

But below you are using the I2C address, is this 'stable' enough so that
userspace can still identify the controls and set them accordingly with
amixer or UCM?


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

generic comment for this patch: you may want to follow the same
convention for error log, sometimes it's %s, or %s: or no %s


> +static int pcmdevice_codec_probe(struct snd_soc_component *codec)
> +{
> +	struct pcmdevice_priv *pcm_dev = snd_soc_component_get_drvdata(codec);
> +	struct i2c_adapter *adap = pcm_dev->client->adapter;
> +	int ret, i, j;
> +
> +	mutex_lock(&pcm_dev->codec_lock);
> +	pcm_dev->component = codec;
> +	pcm_dev->fw_state = PCMDEVICE_FW_LOAD_OK;
> +
> +	for (i = 0; i < pcm_dev->ndev; i++) {
> +		for (j = 0; j < 2; j++) {
> +			ret = pcmdev_gain_ctrl_add(pcm_dev, i, j);
> +			if (ret < 0)
> +				goto out;
> +		}
> +	}
> +
> +	/* device-name[defined in pcmdevice_i2c_id]-i2c-bus_id[0,1,...,N]-
> +	 * sum[1,2,...,4]dev-reg.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->regbin_name, PCMDEVICE_REGBIN_FILENAME_LEN,
> +		"%s-i2c-%d-%udev-reg.bin", pcm_dev->dev_name, adap->nr,
> +		pcm_dev->ndev);
> +
> +	ret = request_firmware_nowait(THIS_MODULE, FW_ACTION_UEVENT,
> +		pcm_dev->regbin_name, pcm_dev->dev, GFP_KERNEL, pcm_dev,
> +		pcmdev_regbin_ready);

I already had a question early on whether these addresses are 'stable',
but here the device address is used to fetch firmware, and there is no
prefix or directory to identify platform-specific settings.

I don't know how this might work for a distribution. There needs to be a
way to detect what system this is at run-time, and make sure we don't
use settings for platform XYZ on platform ABC.


> +static int pcmdevice_set_dai_sysclk(struct snd_soc_dai *codec_dai,
> +	int clk_id, unsigned int freq, int dir)
> +{
> +	struct pcmdevice_priv *pcm_dev = snd_soc_dai_get_drvdata(codec_dai);
> +
> +	pcm_dev->sysclk = freq;

check clock values?

> +
> +	return 0;
> +}