Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
Date:   Mon, 30 Oct 2023 17:20:55 +0000
From:   Mark Brown <broonie@kernel.org>
To:     Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Cc:     Baojun Xu <baojun.xu@ti.com>, lgirdwood@gmail.com, perex@perex.cz,
        alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org,
        kevin-lu@ti.com, shenghao-ding@ti.com, peeyush@ti.com,
        navada@ti.com, tiwai@suse.de
Subject: Re: [PATCH v3] ASoC: tas2783: Add source files for tas2783 driver.
Message-ID: <9d922584-288a-4b73-83ef-477d1bc58521@sirena.org.uk>
References: <20231028092409.96813-1-baojun.xu@ti.com>
 <7f4465c1-5e8e-4c5f-bbff-d2c930326986@linux.intel.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="5LMN/5Y4nwdbngJK"
Content-Disposition: inline
In-Reply-To: <7f4465c1-5e8e-4c5f-bbff-d2c930326986@linux.intel.com>
Precedence: bulk


--5LMN/5Y4nwdbngJK
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

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.

--5LMN/5Y4nwdbngJK
Content-Type: application/pgp-signature; name="signature.asc"

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

iQEzBAABCgAdFiEEreZoqmdXGLWf4p/qJNaLcl1Uh9AFAmU/5fYACgkQJNaLcl1U
h9ARhAgAhgRuLFI+OnWYzXEkD7ojB6ZTuPex3LKj3PhI8JGwXsJbN41iFSzInS10
tMksTNi7ewYcziVKhNzn6Jq+XlWPNlCgtuxE6/hL8UdKt2KqsJGA9r7DKXNp2/kz
8zM3B/I/fRtVb6vPRyAzI7XPkFxjBWkqeraDYLXY1Rdw3EYhn9VDLy6+PLoGiRG2
08tlnKKEBG6PMytmIrqmAP+VRJbq8YLH6wnRoggvvtKGj+xhpLjyqm5wO3+LDgqi
bE1C2z7l/gqouaVoeK2vVVaXKPGZkTH0eZ7Ksk4WQ8aoakuxbGQnH6LdEh25N2es
DNLcFKS5P4KGRORpzNVluc7RB23jDw==
=dOoN
-----END PGP SIGNATURE-----

--5LMN/5Y4nwdbngJK--