On 10/30/23 12:20, Mark Brown wrote:
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?
Good suggestion, we could indeed check that the registers are NOT in the range [0,0xBF] for all ports - only the range [0xC0..FF] is allowed for implementation-defined values. I'll try to cook something up.
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.
Having the interrupt registers as readable could be problematic, there's a known race condition where the drivers need to do a read after a write, and I am a bit worried if we have two agents reading the same thing. It's the only case I am aware of where a read establishes a state.
+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.
I was only asking was the acronym stood for to make it easier to look-up. Not asking for any technical details.