Hi Mark,
On 07-07-2025 2:02 p.m., Mark Brown wrote:
On Sat, Jul 05, 2025 at 02:03:06PM +0200, Luca Weiss wrote:
+static void aw8898_update_dev_mode(struct aw8898 *aw8898)
+{
+ unsigned int mode = AW8898_SYSCTRL_SPK_MODE;
+
+ if (aw8898->dev_mode == AW8898_RECEIVER)
+ mode = AW8898_SYSCTRL_RCV_MODE;
+
+ regmap_update_bits(aw8898->regmap, AW8898_SYSCTRL,
+ AW8898_SYSCTRL_MODE_MASK,
+ FIELD_PREP(AW8898_SYSCTRL_MODE_MASK, mode));
+}
Why is this open coded rather than just being a standard enum? AFAICT
we never reference dev_mode outside of here or the _get() and put()
callbacks. You might be looking for a _VALUE_ENUM?
Thanks for the reference, I'll take a look!
+ if (!fw) {
+ dev_err(&aw8898->client->dev, "Failed to load firmware\n");
+ return;
+ }
+
+ dev_dbg(&aw8898->client->dev, "Loaded %s - size: %zu\n", AW8898_CFG_NAME,
fw->size);
We print the firmware name we were looking for if we loaded it, but not
if we failed to load it when it's probably more useful.
I can remove the debug print, I think if the firmware fails to load
we'll also get an error print from the driver core with the file name?
But I can double check it.
+ aw8898_cfg_write(aw8898, aw8898_cfg);
The "firmware" here is just a list of arbatrary register writes with no
validation of addresses or anything...
Yes... Got any suggestions how to make it better? This "firmware" file
is the one that's also usable with the original driver from the
amplifier vendor.
I honestly haven't really checked whether all the registers that are
written there are documented well enough in the datasheet I have, so
that this sequence could be replaced by proper DT values. But for sure
already I know that some registers which are used and functional, are
not documented at all unfortunately.
+ switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
+ case SND_SOC_DAIFMT_I2S:
+ if ((fmt & SND_SOC_DAIFMT_CLOCK_PROVIDER_MASK)
+ != SND_SOC_DAIFMT_CBC_CFC) {
+ dev_err(component->dev, "Invalid codec master mode:
%d\n",
Clock provider mode.
Sure, will update.
+static int aw8898_startup(struct snd_pcm_substream *substream,
+ struct snd_soc_dai *dai)
+{
+ struct aw8898 *aw8898 = snd_soc_component_get_drvdata(dai->component);
+ unsigned int val;
+ int err;
+
+ err = regmap_read_poll_timeout(aw8898->regmap, AW8898_SYSST,
+ val, val & AW8898_SYSST_PLLS,
+ 2000, 1 * USEC_PER_SEC);
What's this actually checking? You shouldn't rely on I2S being clocked
prior to trigger...
I've also taken this from the original driver, so I do not know the
original purpose of it.
The register description is "System Status Register" "PLL locked status.
1: locked, 0: unlocked", so presumably waiting for the amplifier itself
to get ready for playing audio.
Regards
Luca
