linux-kernel - Re: [PATCH v3] ASoC: tas2783: Add source files for tas2783 driver.

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: <9d922584-288a-4b73-83ef-477d1bc58521@sirena.org.uk>
Date:   Mon, 30 Oct 2023 17:20:55 +0000
From:   Mark Brown <broonie@...nel.org>
To:     Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
Cc:     Baojun Xu <baojun.xu@...com>, lgirdwood@...il.com, perex@...ex.cz,
        alsa-devel@...a-project.org, linux-kernel@...r.kernel.org,
        kevin-lu@...com, shenghao-ding@...com, peeyush@...com,
        navada@...com, tiwai@...e.de
Subject: Re: [PATCH v3] ASoC: tas2783: Add source files for tas2783 driver.

On Mon, Oct 30, 2023 at 11:05:39AM -0500, Pierre-Louis Bossart wrote:

> > +static bool tas2783_readable_register(struct device *dev, unsigned int reg)
> > +{
> > +	switch (reg) {
> > +	case 0x000 ... 0x080:	/* Data port 0. */

> No, this is wrong. All the data port 'standard' registers are "owned" by
> the SoundWire core and handled during the port prepare/configure/bank
> switch routines. Do not use them for regmap.

> In other words, you *shall* only define vendor-specific registers in
> this codec driver.

This seems to come up a moderate amount and is an understandable thing
to do - could you (or someone else who knows SoundWire) perhaps send a
patch for the regmap SoundWire integration which does some validation
here during registration and at least prints a warning?

Also worth noting that the default is going to be that the registers are
readable if the driver doesn't configure anything at all so perhaps at
least for just readability this might be worth revisiting.

> > +static const struct snd_soc_dapm_widget tasdevice_dapm_widgets[] = {
> > +	SND_SOC_DAPM_AIF_IN("ASI", "ASI Playback", 0, SND_SOC_NOPM, 0, 0),
> > +	SND_SOC_DAPM_AIF_OUT("ASI OUT", "ASI Capture", 0, SND_SOC_NOPM,
> > +		0, 0),
> > +	SND_SOC_DAPM_SPK("SPK", NULL),
> > +	SND_SOC_DAPM_OUTPUT("OUT"),
> > +	SND_SOC_DAPM_INPUT("DMIC")
> > +};

> Can you clarify what "ASI" is?

ASI seems to be a fairly commonly used name in TI devices...  In general
naming that corresponds to the datasheet should be fine, especially for
internal only things like this sort of DAPM widget.  I'd guess it's
something like Audio Serial Interface but not actually gone and looked.

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)