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?
> + 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.
> + aw8898_cfg_write(aw8898, aw8898_cfg);
The "firmware" here is just a list of arbatrary register writes with no
validation of addresses or anything...
> + 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.
> +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...
signature.asc
Description: PGP signature
