On 24/05/19 16:21, Richard Fitzgerald wrote:

On 24/05/19 15:56, Mark Brown wrote:

...

On Fri, May 24, 2019 at 11:41:55AM +0100, Charles Keepax wrote:

...

+    /* +     * Just read a register a few times to ensure the internal +     * oscillator sends out a few clocks. +     */ +    for (i = 0; i < 4; i++) { +        ret = regmap_read(madera->regmap, MADERA_SOFTWARE_RESET, &val); +        if (ret) +            dev_err(madera->dev, +                "%s Failed to read register: %d (%d)\n", +                __func__, ret, i);

Why use %s to format the __func__ rather than just concatenate?

GCC docs say that it's a magic variable so cannot be concatenated with string literals. Though I never tried concatenation to see if it works.

...

...

+    }

+    udelay(300);

So we read the register a few times then add a few hundred us of delay after?  Surely that delay is going to be negligable compared to the time spent on I/O?

The register reads are to create clock cycles in the silicon, not to generate delay.

Sorry, just re-read your comment and realized I'd misread it. It's a hardware requirement that after generating the internal clocks there must be a delay. I.e. we require a combination of a guaranteed number of SYSCLKs followed by a guaranteed minimum delay.

...

...

+int madera_sysclk_ev(struct snd_soc_dapm_widget *w, +             struct snd_kcontrol *kcontrol, int event) +{ +    struct snd_soc_component *component = snd_soc_dapm_to_component(w->dapm); +    struct madera_priv *priv = snd_soc_component_get_drvdata(component);

+    madera_spin_sysclk(priv);

+    return 0; +} +EXPORT_SYMBOL_GPL(madera_sysclk_ev);

This will delay both before and after every power up and power down. Are you sure that makes sense?

I think that's correct but we can re-check with hardware people. It's not just a delay, it needs to ensure there are always a certain number of SYSCLK cycles in the hardware to avoid leaving certain state machines in limbo.

...

...

+    ret = madera_check_speaker_overheat(madera, &warn, &shutdown); +    if (ret) +        shutdown = true; /* for safety attempt to shutdown on error */

+    if (shutdown) { +        dev_crit(madera->dev, "Thermal shutdown\n"); +        ret = regmap_update_bits(madera->regmap, +                     MADERA_OUTPUT_ENABLES_1, +                     MADERA_OUT4L_ENA | +                     MADERA_OUT4R_ENA, 0); +        if (ret != 0) +            dev_crit(madera->dev, +                 "Failed to disable speaker outputs: %d\n", +                 ret); +    } else if (warn) { +        dev_crit(madera->dev, "Thermal warning\n"); +    }

+    return IRQ_HANDLED;

We will flag the interrupt as handled if there was neither a warning nor a critical overheat?  I'd expect some warning about a spurious interrupt at least.

...

+static int madera_get_variable_u32_array(struct madera_priv *priv, +                     const char *propname, +                     u32 *dest, +                     int n_max, +                     int multiple) +{ +    struct madera *madera = priv->madera; +    int n, ret;

+    n = device_property_read_u32_array(madera->dev, propname, NULL, 0); +    if (n == -EINVAL) { +        return 0;    /* missing, ignore */ +    } else if (n < 0) { +        dev_warn(madera->dev, "%s malformed (%d)\n", +             propname, n); +        return n; +    } else if ((n % multiple) != 0) { +        dev_warn(madera->dev, "%s not a multiple of %d entries\n", +             propname, multiple); +        return -EINVAL; +    }

+    if (n > n_max) +        n = n_max;

+    ret = device_property_read_u32_array(madera->dev, propname, dest, n);

+    if (ret < 0) +        return ret; +    else +        return n; +}

This feels like it should perhaps be a generic OF helper function - there's nothing driver specific I'm seeing here and arrays that need to be a multiple of N entries aren't that uncommon I think.

...

+    mutex_lock(&priv->rate_lock); +    cached_rate = priv->adsp_rate_cache[adsp_num]; +    mutex_unlock(&priv->rate_lock);

What's this lock protecting?  The value can we read can change as soon as the lock is released and we're just reading a single word here rather than traversing a data structure that might change under us or something.

...

+void madera_destroy_bus_error_irq(struct madera_priv *priv, int dsp_num) +{ +    struct madera *madera = priv->madera;

+    madera_free_irq(madera, +            madera_dsp_bus_error_irqs[dsp_num], +            &priv->adsp[dsp_num]); +} +EXPORT_SYMBOL_GPL(madera_destroy_bus_error_irq);

We use free rather than destroy normally?

...

+static const char * const madera_dfc_width_text[MADERA_DFC_WIDTH_ENUM_SIZE] = { +    "8bit", "16bit", "20bit", "24bit", "32bit", +};

Spaces might make these more readable.

...

+static void madera_sleep(unsigned int delay) +{ +    if (delay < 20) { +        delay *= 1000; +        usleep_range(delay, delay + 500); +    } else { +        msleep(delay); +    } +}

This feels like it might make sense as a helper function as well - I could've sworn there was one already but I can't immediately find it.