[1/5] ASoC: madera: Add DT bindings for Cirrus Logic Madera codecs
diff mbox series

Message ID 20190524104158.30731-1-ckeepax@opensource.cirrus.com
State Superseded
Headers show
Series
  • [1/5] ASoC: madera: Add DT bindings for Cirrus Logic Madera codecs
Related show

Checks

Context Check Description
robh/checkpatch success

Commit Message

Charles Keepax May 24, 2019, 10:41 a.m. UTC
From: Richard Fitzgerald <rf@opensource.cirrus.com>

The Cirrus Logic Madera codecs are a family of related codecs with
extensive digital and analogue I/O, digital mixing and routing,
signal processing and programmable DSPs.

Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 Documentation/devicetree/bindings/sound/madera.txt | 67 ++++++++++++++++++++++
 MAINTAINERS                                        |  1 +
 include/dt-bindings/sound/madera.h                 | 29 ++++++++++
 3 files changed, 97 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/madera.txt
 create mode 100644 include/dt-bindings/sound/madera.h

Comments

Mark Brown May 24, 2019, 2:56 p.m. UTC | #1
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?

> +	}
> +
> +	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?

> +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?

> +
> +	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.
Richard Fitzgerald May 24, 2019, 3:21 p.m. UTC | #2
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.

> 
>> +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.
>
Richard Fitzgerald May 24, 2019, 3:24 p.m. UTC | #3
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.
>>
>
Charles Keepax May 24, 2019, 3:33 p.m. UTC | #4
On Fri, May 24, 2019 at 03:56:03PM +0100, 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?
> 

Really the error message is quite out of character in general,
we don't normally print the func and it doesn't really say what
failed. I will refactor it.

> > +	}
> > +
> > +	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?
> 

This is one of those technically situations. We have to do
the reads and we have to have at least 300uS after those.
Normally there will be enough slack in the system but better to
guarantee it. Maybe a comment?

> > +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?
> 

Regrettably yes, or well at least I am sure that despite a lot of
complaining this is what the hardware guys insisted needed done.

> > +
> > +	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.
> 

Yeah I will update that one.

> > +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.
> 

I will have a look and see if we might do a generic helper
function for this one.

> > +	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.
> 

Overall the lock protects anything that changes a rate on the
chip, since we have to do that weird spin_sysclk thing
before/after anything that does that.

It might normally be safe without the lock here I guess, since
you are only really protecting against the reads of adsp_rate_cache.
But suppose that might depend on the architecture if the read/writes
arn't effectively atomic (although that is probably unlikely on most
modern architectures).

> > +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?
> 

Yup will fix that.

> > +static const char * const madera_dfc_width_text[MADERA_DFC_WIDTH_ENUM_SIZE] = {
> > +	"8bit", "16bit", "20bit", "24bit", "32bit",
> > +};
> 
> Spaces might make these more readable.
> 

No problem to fix that too.

> > +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.

I do vaguely remember some discussion that it was preferred
that people had to think about which delay function to call,
rather than having a generic helper. TBF I could probably
just switch it all to use msleep, for the most part this is
a quite small optimisation of path bring up (although it was
asked for by one of our customers).

Thanks,
Charles
Mark Brown May 26, 2019, 12:18 p.m. UTC | #5
On Fri, May 24, 2019 at 04:24:10PM +0100, Richard Fitzgerald wrote:
> On 24/05/19 16:21, Richard Fitzgerald wrote:
> > On 24/05/19 15:56, Mark Brown wrote:

Please fix your mail client to word wrap within paragraphs at something
substantially less than 80 columns.  Doing this makes your messages much
easier to read and reply to.  You've also got some replies inserted at
the wrong quote level.

> > > 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.

OK, the comment could use a bit of clarification then to say that reads
are explicitly required as opposed to being purely to generate a delay.

> > > 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.

That sounds like you might want both _POST_PMU and _POST_PMD but do you
really need the _PREs as well?
Charles Keepax May 27, 2019, 8:32 a.m. UTC | #6
On Sun, May 26, 2019 at 01:18:46PM +0100, Mark Brown wrote:
> On Fri, May 24, 2019 at 04:24:10PM +0100, Richard Fitzgerald wrote:
> > On 24/05/19 16:21, Richard Fitzgerald wrote:
> > > On 24/05/19 15:56, Mark Brown wrote:
> > > > 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.
> 
> That sounds like you might want both _POST_PMU and _POST_PMD but do you
> really need the _PREs as well?

Yeah the requirement from the hardware guys was that we needed a
guard band between changing the SYSCLK state and other things
happening. So it has to happen both before and after the state of
SYSCLK changes. It is certainly far from ideal but it is what it
is.

Thanks,
Charles

Patch
diff mbox series

diff --git a/Documentation/devicetree/bindings/sound/madera.txt b/Documentation/devicetree/bindings/sound/madera.txt
new file mode 100644
index 0000000000000..1114fcf1aa4c2
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/madera.txt
@@ -0,0 +1,67 @@ 
+Cirrus Logic Madera class audio codecs
+
+This describes audio configuration bindings for these codecs.
+
+See also the core bindings for the parent MFD driver:
+See Documentation/devicetree/bindings/mfd/madera.txt
+
+and defines for values used in these bindings:
+include/dt-bindings/sound/madera.h
+
+These properties are all contained in the parent MFD node.
+
+Optional properties:
+  - cirrus,dmic-ref : Indicates how the MICBIAS pins have been externally
+    connected to DMICs on each input, one cell per input.
+    <IN1 IN2 IN3 ...>
+    A value of 0 indicates MICVDD and is the default, other values depend on the
+    codec:
+    For CS47L35 one of the CS47L35_DMIC_REF_xxx values
+    For all other codecs one of the MADERA_DMIC_REF_xxx values
+    Also see the datasheet for a description of the INn_DMIC_SUP field.
+
+  - cirrus,inmode : A list of input mode settings for each input. A maximum of
+    16 cells, with four cells per input in the order INnAL, INnAR INnBL INnBR.
+    For non-muxed inputs the first two cells for that input set the mode for
+    the left and right channel and the second two cells must be 0.
+    For muxed inputs the first two cells for that input set the mode of the
+    left and right A inputs and the second two cells set the mode of the left
+    and right B inputs.
+    Valid mode values are one of the MADERA_INMODE_xxx. If the array is shorter
+    than the number of inputs the unspecified inputs default to
+    MADERA_INMODE_DIFF.
+
+  - cirrus,out-mono : Mono bit for each output, must contain six cells if
+    specified. A non-zero value indicates the corresponding output is mono.
+
+  - cirrus,max-channels-clocked : Maximum number of channels that I2S clocks
+    will be generated for. Useful when clock master for systems where the I2S
+    bus has multiple data lines.
+    One cell for each AIF, use a value of zero for AIFs that should be handled
+    normally.
+
+  - cirrus,pdm-fmt : PDM speaker data format, must contain 2 cells
+    (OUT5 and OUT6). See the PDM_SPKn_FMT field in the datasheet for a
+    description of this value.
+    The second cell is ignored for codecs that do not have OUT6.
+
+  - cirrus,pdm-mute : PDM mute format, must contain 2 cells
+    (OUT5 and OUT6). See the PDM_SPKn_CTRL_1 register in the datasheet for a
+    description of this value.
+    The second cell is ignored for codecs that do not have OUT6.
+
+Example:
+
+cs47l35@0 {
+	compatible = "cirrus,cs47l35";
+
+	cirrus,dmic-ref = <0 0 CS47L35_DMIC_REF_MICBIAS1B 0>;
+	cirrus,inmode = <
+		MADERA_INMODE_DMIC MADERA_INMODE_DMIC /* IN1A digital */
+		MADERA_INMODE_SE   MADERA_INMODE_SE   /* IN1B single-ended */
+		MADERA_INMODE_DIFF MADERA_INMODE_DIFF /* IN2 differential */
+		0 0 	/* not used on this codec */
+	>;
+	cirrus,out-mono = <0 0 0 0 0 0>;
+	cirrus,max-channels-clocked = <2 0 0>;
+};
diff --git a/MAINTAINERS b/MAINTAINERS
index 5cfbea4ce5750..642cb5610dd50 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3915,6 +3915,7 @@  W:	https://github.com/CirrusLogic/linux-drivers/wiki
 S:	Supported
 F:	Documentation/devicetree/bindings/mfd/madera.txt
 F:	Documentation/devicetree/bindings/pinctrl/cirrus,madera-pinctrl.txt
+F:	include/dt-bindings/sound/madera*
 F:	include/linux/irqchip/irq-madera*
 F:	include/linux/mfd/madera/*
 F:	drivers/gpio/gpio-madera*
diff --git a/include/dt-bindings/sound/madera.h b/include/dt-bindings/sound/madera.h
new file mode 100644
index 0000000000000..9ff4eae5259b0
--- /dev/null
+++ b/include/dt-bindings/sound/madera.h
@@ -0,0 +1,29 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Device Tree defines for Madera codecs
+ *
+ * Copyright (C) 2016-2017 Cirrus Logic, Inc. and
+ *                         Cirrus Logic International Semiconductor Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef DT_BINDINGS_SOUND_MADERA_H
+#define DT_BINDINGS_SOUND_MADERA_H
+
+#define MADERA_INMODE_DIFF		0
+#define MADERA_INMODE_SE		1
+#define MADERA_INMODE_DMIC		2
+
+#define MADERA_DMIC_REF_MICVDD		0
+#define MADERA_DMIC_REF_MICBIAS1	1
+#define MADERA_DMIC_REF_MICBIAS2	2
+#define MADERA_DMIC_REF_MICBIAS3	3
+
+#define CS47L35_DMIC_REF_MICBIAS1B	1
+#define CS47L35_DMIC_REF_MICBIAS2A	2
+#define CS47L35_DMIC_REF_MICBIAS2B	3
+
+#endif