ASoC: ssm2602: Fix ADC powerup sequencing

Message ID 20180517133009.26079-1-m.felsch@pengutronix.de
State Changes Requested, archived
Headers show
Series
  • ASoC: ssm2602: Fix ADC powerup sequencing
Related show

Commit Message

Marco Felsch May 17, 2018, 1:30 p.m.
From: Philipp Zabel <p.zabel@pengutronix.de>

According to the ssm2603 data sheet (control register sequencing), the
digital core should be activated only after all necessary bits in the
power register are enabled, and a delay determined by the decoupling
capacitor on the VMID pin has passed. If the digital core is activated
too early, or even before the ADC is powered up, audible artifacts
appear at the beginning of the recorded signal.

The digital core is also needed for playback, so when recording starts
it may already be enabled. This means we cannot get the power sequence
correct when we want to be able to start recording after playback.

As a workaround put the MIC mute switch into the DAPM routes. This
way we can keep the recording disabled until the MIC Bias has settled
and thus get rid of audible artifacts.

The delay we have to wait depends on a board specific capacitor
connected to the VMID pins, so make the delay configurable in the device
tree.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 .../devicetree/bindings/sound/adi,ssm2602.txt |  7 +++++
 sound/soc/codecs/ssm2602.c                    | 30 +++++++++++++++++--
 2 files changed, 35 insertions(+), 2 deletions(-)

Comments

Rob Herring May 17, 2018, 4:14 p.m. | #1
On Thu, May 17, 2018 at 8:30 AM, Marco Felsch <m.felsch@pengutronix.de> wrote:
> From: Philipp Zabel <p.zabel@pengutronix.de>
>
> According to the ssm2603 data sheet (control register sequencing), the
> digital core should be activated only after all necessary bits in the
> power register are enabled, and a delay determined by the decoupling
> capacitor on the VMID pin has passed. If the digital core is activated
> too early, or even before the ADC is powered up, audible artifacts
> appear at the beginning of the recorded signal.
>
> The digital core is also needed for playback, so when recording starts
> it may already be enabled. This means we cannot get the power sequence
> correct when we want to be able to start recording after playback.
>
> As a workaround put the MIC mute switch into the DAPM routes. This
> way we can keep the recording disabled until the MIC Bias has settled
> and thus get rid of audible artifacts.
>
> The delay we have to wait depends on a board specific capacitor
> connected to the VMID pins, so make the delay configurable in the device
> tree.
>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
>  .../devicetree/bindings/sound/adi,ssm2602.txt |  7 +++++
>  sound/soc/codecs/ssm2602.c                    | 30 +++++++++++++++++--
>  2 files changed, 35 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/sound/adi,ssm2602.txt b/Documentation/devicetree/bindings/sound/adi,ssm2602.txt
> index 3b3302fe399b..9334d48c0462 100644
> --- a/Documentation/devicetree/bindings/sound/adi,ssm2602.txt
> +++ b/Documentation/devicetree/bindings/sound/adi,ssm2602.txt
> @@ -11,9 +11,16 @@ Required properties:
>    - reg : the I2C address of the device for I2C, the chip select
>            number for SPI.
>
> +Optional properties:
> +
> +  - startup-delay-us : delay between powering on and activating the digital
> +                       core, determined by the decoupling capacitor C on the
> +                      VMID pin: delay [µs] = C [µF] * 25000 / 3.5
> +

We already have similarly defined property. Please reuse that. See mmc
pwrseq binding.

Also, please split bindings to separate patch.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Philipp Zabel May 18, 2018, 10:46 a.m. | #2
Hi Rob,

On Thu, 2018-05-17 at 11:14 -0500, Rob Herring wrote:
> On Thu, May 17, 2018 at 8:30 AM, Marco Felsch <m.felsch@pengutronix.de> wrote:
> > From: Philipp Zabel <p.zabel@pengutronix.de>
> > 
> > According to the ssm2603 data sheet (control register sequencing), the
> > digital core should be activated only after all necessary bits in the
> > power register are enabled, and a delay determined by the decoupling
> > capacitor on the VMID pin has passed. If the digital core is activated
> > too early, or even before the ADC is powered up, audible artifacts
> > appear at the beginning of the recorded signal.
> > 
> > The digital core is also needed for playback, so when recording starts
> > it may already be enabled. This means we cannot get the power sequence
> > correct when we want to be able to start recording after playback.
> > 
> > As a workaround put the MIC mute switch into the DAPM routes. This
> > way we can keep the recording disabled until the MIC Bias has settled
> > and thus get rid of audible artifacts.
> > 
> > The delay we have to wait depends on a board specific capacitor
> > connected to the VMID pins, so make the delay configurable in the device
> > tree.
> > 
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > ---
> >  .../devicetree/bindings/sound/adi,ssm2602.txt |  7 +++++
> >  sound/soc/codecs/ssm2602.c                    | 30 +++++++++++++++++--
> >  2 files changed, 35 insertions(+), 2 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/sound/adi,ssm2602.txt b/Documentation/devicetree/bindings/sound/adi,ssm2602.txt
> > index 3b3302fe399b..9334d48c0462 100644
> > --- a/Documentation/devicetree/bindings/sound/adi,ssm2602.txt
> > +++ b/Documentation/devicetree/bindings/sound/adi,ssm2602.txt
> > @@ -11,9 +11,16 @@ Required properties:
> >    - reg : the I2C address of the device for I2C, the chip select
> >            number for SPI.
> > 
> > +Optional properties:
> > +
> > +  - startup-delay-us : delay between powering on and activating the digital
> > +                       core, determined by the decoupling capacitor C on the
> > +                      VMID pin: delay [µs] = C [µF] * 25000 / 3.5
> > +
> 
> We already have similarly defined property. Please reuse that. See mmc
> pwrseq binding.

Do you mean 'post-power-on-delay-ms' from 'mmc-pwseq-simple'?
It is documented as:

- post-power-on-delay-ms : Delay in ms after powering the card and
        de-asserting the reset-gpios (if any)

The startup delay here is not after powering the whole IC or deasserting
resets and before it can be used, but after powering up a specific part
of the codec (the ADC) and before unmuting the MIC input to the digital
core during start of decoding. With this in mind, do you still think the
property should be called the same as the mmc full-chip poweron delay?
If so, would it be acceptable to use post-power-on-delay-us to keep the
microsecond resolution?

thanks
Philipp
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rob Herring May 23, 2018, 4:53 p.m. | #3
On Fri, May 18, 2018 at 12:46:49PM +0200, Philipp Zabel wrote:
> Hi Rob,
> 
> On Thu, 2018-05-17 at 11:14 -0500, Rob Herring wrote:
> > On Thu, May 17, 2018 at 8:30 AM, Marco Felsch <m.felsch@pengutronix.de> wrote:
> > > From: Philipp Zabel <p.zabel@pengutronix.de>
> > > 
> > > According to the ssm2603 data sheet (control register sequencing), the
> > > digital core should be activated only after all necessary bits in the
> > > power register are enabled, and a delay determined by the decoupling
> > > capacitor on the VMID pin has passed. If the digital core is activated
> > > too early, or even before the ADC is powered up, audible artifacts
> > > appear at the beginning of the recorded signal.
> > > 
> > > The digital core is also needed for playback, so when recording starts
> > > it may already be enabled. This means we cannot get the power sequence
> > > correct when we want to be able to start recording after playback.
> > > 
> > > As a workaround put the MIC mute switch into the DAPM routes. This
> > > way we can keep the recording disabled until the MIC Bias has settled
> > > and thus get rid of audible artifacts.
> > > 
> > > The delay we have to wait depends on a board specific capacitor
> > > connected to the VMID pins, so make the delay configurable in the device
> > > tree.
> > > 
> > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > > ---
> > >  .../devicetree/bindings/sound/adi,ssm2602.txt |  7 +++++
> > >  sound/soc/codecs/ssm2602.c                    | 30 +++++++++++++++++--
> > >  2 files changed, 35 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/sound/adi,ssm2602.txt b/Documentation/devicetree/bindings/sound/adi,ssm2602.txt
> > > index 3b3302fe399b..9334d48c0462 100644
> > > --- a/Documentation/devicetree/bindings/sound/adi,ssm2602.txt
> > > +++ b/Documentation/devicetree/bindings/sound/adi,ssm2602.txt
> > > @@ -11,9 +11,16 @@ Required properties:
> > >    - reg : the I2C address of the device for I2C, the chip select
> > >            number for SPI.
> > > 
> > > +Optional properties:
> > > +
> > > +  - startup-delay-us : delay between powering on and activating the digital
> > > +                       core, determined by the decoupling capacitor C on the
> > > +                      VMID pin: delay [µs] = C [µF] * 25000 / 3.5
> > > +
> > 
> > We already have similarly defined property. Please reuse that. See mmc
> > pwrseq binding.
> 
> Do you mean 'post-power-on-delay-ms' from 'mmc-pwseq-simple'?
> It is documented as:
> 
> - post-power-on-delay-ms : Delay in ms after powering the card and
>         de-asserting the reset-gpios (if any)
> 
> The startup delay here is not after powering the whole IC or deasserting
> resets and before it can be used, but after powering up a specific part
> of the codec (the ADC) and before unmuting the MIC input to the digital
> core during start of decoding. With this in mind, do you still think the
> property should be called the same as the mmc full-chip poweron delay?
> If so, would it be acceptable to use post-power-on-delay-us to keep the
> microsecond resolution?

Okay, then I'd suggest something a bit more specific. Perhaps 
"pre-unmute-delay-us" and document in a common location.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Marco Felsch May 25, 2018, 9:47 a.m. | #4
Hi Rob,

On 18-05-23 11:53, Rob Herring wrote:
> On Fri, May 18, 2018 at 12:46:49PM +0200, Philipp Zabel wrote:
> > Hi Rob,
> > 
> > On Thu, 2018-05-17 at 11:14 -0500, Rob Herring wrote:
> > > On Thu, May 17, 2018 at 8:30 AM, Marco Felsch <m.felsch@pengutronix.de> wrote:
> > > > From: Philipp Zabel <p.zabel@pengutronix.de>
> > > > 
> > > > According to the ssm2603 data sheet (control register sequencing), the
> > > > digital core should be activated only after all necessary bits in the
> > > > power register are enabled, and a delay determined by the decoupling
> > > > capacitor on the VMID pin has passed. If the digital core is activated
> > > > too early, or even before the ADC is powered up, audible artifacts
> > > > appear at the beginning of the recorded signal.
> > > > 
> > > > The digital core is also needed for playback, so when recording starts
> > > > it may already be enabled. This means we cannot get the power sequence
> > > > correct when we want to be able to start recording after playback.
> > > > 
> > > > As a workaround put the MIC mute switch into the DAPM routes. This
> > > > way we can keep the recording disabled until the MIC Bias has settled
> > > > and thus get rid of audible artifacts.
> > > > 
> > > > The delay we have to wait depends on a board specific capacitor
> > > > connected to the VMID pins, so make the delay configurable in the device
> > > > tree.
> > > > 
> > > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > > > ---
> > > >  .../devicetree/bindings/sound/adi,ssm2602.txt |  7 +++++
> > > >  sound/soc/codecs/ssm2602.c                    | 30 +++++++++++++++++--
> > > >  2 files changed, 35 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/sound/adi,ssm2602.txt b/Documentation/devicetree/bindings/sound/adi,ssm2602.txt
> > > > index 3b3302fe399b..9334d48c0462 100644
> > > > --- a/Documentation/devicetree/bindings/sound/adi,ssm2602.txt
> > > > +++ b/Documentation/devicetree/bindings/sound/adi,ssm2602.txt
> > > > @@ -11,9 +11,16 @@ Required properties:
> > > >    - reg : the I2C address of the device for I2C, the chip select
> > > >            number for SPI.
> > > > 
> > > > +Optional properties:
> > > > +
> > > > +  - startup-delay-us : delay between powering on and activating the digital
> > > > +                       core, determined by the decoupling capacitor C on the
> > > > +                      VMID pin: delay [µs] = C [µF] * 25000 / 3.5
> > > > +
> > > 
> > > We already have similarly defined property. Please reuse that. See mmc
> > > pwrseq binding.
> > 
> > Do you mean 'post-power-on-delay-ms' from 'mmc-pwseq-simple'?
> > It is documented as:
> > 
> > - post-power-on-delay-ms : Delay in ms after powering the card and
> >         de-asserting the reset-gpios (if any)
> > 
> > The startup delay here is not after powering the whole IC or deasserting
> > resets and before it can be used, but after powering up a specific part
> > of the codec (the ADC) and before unmuting the MIC input to the digital
> > core during start of decoding. With this in mind, do you still think the
> > property should be called the same as the mmc full-chip poweron delay?
> > If so, would it be acceptable to use post-power-on-delay-us to keep the
> > microsecond resolution?
> 
> Okay, then I'd suggest something a bit more specific. Perhaps 
> "pre-unmute-delay-us" and document in a common location.
> 
> Rob

The delay is not just for the line-in/mic path it is also for the out
path. More technical, it is needed to charge the decouple capacity which
provides the voltage bias for the analog input/output frontends.

I found the: "ti,charge-period (sound/ti,tas5086.txt)" binding which
represents nearly the same but it is not common. One opportunity would be to
introduce a common "charge-period-us" binding and change the ti binding the
common binding later.

Would that be okay?

Marco
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown May 25, 2018, 10:26 a.m. | #5
On Fri, May 25, 2018 at 11:47:24AM +0200, Marco Felsch wrote:

> I found the: "ti,charge-period (sound/ti,tas5086.txt)" binding which
> represents nearly the same but it is not common. One opportunity would be to
> introduce a common "charge-period-us" binding and change the ti binding the
> common binding later.

Are you sure what you have is a charge period and not some stabalization
time or something?
Marco Felsch May 25, 2018, 11:42 a.m. | #6
Hi Mark,

On 18-05-25 11:26, Mark Brown wrote:
> On Fri, May 25, 2018 at 11:47:24AM +0200, Marco Felsch wrote:
> 
> > I found the: "ti,charge-period (sound/ti,tas5086.txt)" binding which
> > represents nearly the same but it is not common. One opportunity would be to
> > introduce a common "charge-period-us" binding and change the ti binding the
> > common binding later.
> 
> Are you sure what you have is a charge period and not some stabalization
> time or something?

According to the data sheet:
8<-----
As described in the Digital Core Clock section of the
Theory of Operation, insert enough delay time to charge
the VMID decoupling capacitor before setting the active
bit [Register R9, Bit D0].
8<-----
I think yes, it is a charge period.

Also the formula for the delay time (t = C × 25,000/3.5) depends only on
the capacity size.

Marco
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown May 25, 2018, 2:52 p.m. | #7
On Fri, May 25, 2018 at 01:42:53PM +0200, Marco Felsch wrote:

> Also the formula for the delay time (t = C × 25,000/3.5) depends only on
> the capacity size.

Why not just have the user specify the capacitance of the capacitor on
the rail which they can directly read from the schematic rather than
forcing them to do the calcualtion?  That seems a bit clearer and more
user friendly (plus if someone decides the spec was wrong it's easier to
roll out fixes).
Philipp Zabel May 25, 2018, 3:18 p.m. | #8
On Fri, 2018-05-25 at 15:52 +0100, Mark Brown wrote:
> On Fri, May 25, 2018 at 01:42:53PM +0200, Marco Felsch wrote:
> 
> > Also the formula for the delay time (t = C × 25,000/3.5) depends only on
> > the capacity size.
> 
> Why not just have the user specify the capacitance of the capacitor on
> the rail which they can directly read from the schematic rather than
> forcing them to do the calcualtion?  That seems a bit clearer and more
> user friendly (plus if someone decides the spec was wrong it's easier to
> roll out fixes).

The exact capacitance may not be known or vary above the nominal value
because of cheap components, and the formula from the datasheet is just
a guideline.

I'd expect the usual method to set this delay to be semi-empirical:
"start from the value calculated from datasheet and schematics and then
increase until no more audio artifacts on a representative sample of
boards".
I think it is be better to specify a delay that works than a bogus
capacitance value that happens to correspond to a delay that works.

regards
Philipp
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rob Herring May 25, 2018, 3:42 p.m. | #9
On Fri, May 25, 2018 at 4:47 AM, Marco Felsch <m.felsch@pengutronix.de> wrote:
> Hi Rob,
>
> On 18-05-23 11:53, Rob Herring wrote:
>> On Fri, May 18, 2018 at 12:46:49PM +0200, Philipp Zabel wrote:
>> > Hi Rob,
>> >
>> > On Thu, 2018-05-17 at 11:14 -0500, Rob Herring wrote:
>> > > On Thu, May 17, 2018 at 8:30 AM, Marco Felsch <m.felsch@pengutronix.de> wrote:
>> > > > From: Philipp Zabel <p.zabel@pengutronix.de>
>> > > >
>> > > > According to the ssm2603 data sheet (control register sequencing), the
>> > > > digital core should be activated only after all necessary bits in the
>> > > > power register are enabled, and a delay determined by the decoupling
>> > > > capacitor on the VMID pin has passed. If the digital core is activated
>> > > > too early, or even before the ADC is powered up, audible artifacts
>> > > > appear at the beginning of the recorded signal.
>> > > >
>> > > > The digital core is also needed for playback, so when recording starts
>> > > > it may already be enabled. This means we cannot get the power sequence
>> > > > correct when we want to be able to start recording after playback.
>> > > >
>> > > > As a workaround put the MIC mute switch into the DAPM routes. This
>> > > > way we can keep the recording disabled until the MIC Bias has settled
>> > > > and thus get rid of audible artifacts.
>> > > >
>> > > > The delay we have to wait depends on a board specific capacitor
>> > > > connected to the VMID pins, so make the delay configurable in the device
>> > > > tree.
>> > > >
>> > > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
>> > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
>> > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
>> > > > ---
>> > > >  .../devicetree/bindings/sound/adi,ssm2602.txt |  7 +++++
>> > > >  sound/soc/codecs/ssm2602.c                    | 30 +++++++++++++++++--
>> > > >  2 files changed, 35 insertions(+), 2 deletions(-)
>> > > >
>> > > > diff --git a/Documentation/devicetree/bindings/sound/adi,ssm2602.txt b/Documentation/devicetree/bindings/sound/adi,ssm2602.txt
>> > > > index 3b3302fe399b..9334d48c0462 100644
>> > > > --- a/Documentation/devicetree/bindings/sound/adi,ssm2602.txt
>> > > > +++ b/Documentation/devicetree/bindings/sound/adi,ssm2602.txt
>> > > > @@ -11,9 +11,16 @@ Required properties:
>> > > >    - reg : the I2C address of the device for I2C, the chip select
>> > > >            number for SPI.
>> > > >
>> > > > +Optional properties:
>> > > > +
>> > > > +  - startup-delay-us : delay between powering on and activating the digital
>> > > > +                       core, determined by the decoupling capacitor C on the
>> > > > +                      VMID pin: delay [µs] = C [µF] * 25000 / 3.5
>> > > > +
>> > >
>> > > We already have similarly defined property. Please reuse that. See mmc
>> > > pwrseq binding.
>> >
>> > Do you mean 'post-power-on-delay-ms' from 'mmc-pwseq-simple'?
>> > It is documented as:
>> >
>> > - post-power-on-delay-ms : Delay in ms after powering the card and
>> >         de-asserting the reset-gpios (if any)
>> >
>> > The startup delay here is not after powering the whole IC or deasserting
>> > resets and before it can be used, but after powering up a specific part
>> > of the codec (the ADC) and before unmuting the MIC input to the digital
>> > core during start of decoding. With this in mind, do you still think the
>> > property should be called the same as the mmc full-chip poweron delay?
>> > If so, would it be acceptable to use post-power-on-delay-us to keep the
>> > microsecond resolution?
>>
>> Okay, then I'd suggest something a bit more specific. Perhaps
>> "pre-unmute-delay-us" and document in a common location.
>>
>> Rob
>
> The delay is not just for the line-in/mic path it is also for the out
> path. More technical, it is needed to charge the decouple capacity which
> provides the voltage bias for the analog input/output frontends.
>
> I found the: "ti,charge-period (sound/ti,tas5086.txt)" binding which
> represents nearly the same but it is not common. One opportunity would be to
> introduce a common "charge-period-us" binding and change the ti binding the
> common binding later.
>
> Would that be okay?

Sure, sounds fine to me.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown May 25, 2018, 5:24 p.m. | #10
On Fri, May 25, 2018 at 05:18:09PM +0200, Philipp Zabel wrote:
> On Fri, 2018-05-25 at 15:52 +0100, Mark Brown wrote:
> > On Fri, May 25, 2018 at 01:42:53PM +0200, Marco Felsch wrote:

> > > Also the formula for the delay time (t = C × 25,000/3.5) depends only on
> > > the capacity size.

> > Why not just have the user specify the capacitance of the capacitor on
> > the rail which they can directly read from the schematic rather than
> > forcing them to do the calcualtion?  That seems a bit clearer and more
> > user friendly (plus if someone decides the spec was wrong it's easier to
> > roll out fixes).

> The exact capacitance may not be known or vary above the nominal value
> because of cheap components, and the formula from the datasheet is just
> a guideline.

That variability is going to apply just as much to the charge time
calculations/measurements as it is to the initial capacitance value -
the results are going to be very much garbage in, garbage out.

> I'd expect the usual method to set this delay to be semi-empirical:
> "start from the value calculated from datasheet and schematics and then
> increase until no more audio artifacts on a representative sample of
> boards".
> I think it is be better to specify a delay that works than a bogus
> capacitance value that happens to correspond to a delay that works.

If this is varying so drastically per board/system that it's relevant
then we're already into problematic territory.  For most devices we just
have a number for the part, not something that varies so wildly that
each system needs to configure it.
Philipp Zabel June 5, 2018, 9:58 a.m. | #11
Hi Mark,

On Fri, 2018-05-25 at 18:24 +0100, Mark Brown wrote:
> On Fri, May 25, 2018 at 05:18:09PM +0200, Philipp Zabel wrote:
> > On Fri, 2018-05-25 at 15:52 +0100, Mark Brown wrote:
> > > On Fri, May 25, 2018 at 01:42:53PM +0200, Marco Felsch wrote:
> > > > Also the formula for the delay time (t = C × 25,000/3.5) depends only on
> > > > the capacity size.
> > > Why not just have the user specify the capacitance of the capacitor on
> > > the rail which they can directly read from the schematic rather than
> > > forcing them to do the calcualtion?  That seems a bit clearer and more
> > > user friendly (plus if someone decides the spec was wrong it's easier to
> > > roll out fixes).
> > The exact capacitance may not be known or vary above the nominal value
> > because of cheap components, and the formula from the datasheet is just
> > a guideline.
> 
> That variability is going to apply just as much to the charge time
> calculations/measurements as it is to the initial capacitance value -
> the results are going to be very much garbage in, garbage out.

True.

> > I'd expect the usual method to set this delay to be semi-empirical:
> > "start from the value calculated from datasheet and schematics and then
> > increase until no more audio artifacts on a representative sample of
> > boards".
> > I think it is be better to specify a delay that works than a bogus
> > capacitance value that happens to correspond to a delay that works.
> 
> If this is varying so drastically per board/system that it's relevant
> then we're already into problematic territory.  For most devices we just
> have a number for the part, not something that varies so wildly that
> each system needs to configure it.

I'm not arguing this should be configured per individual device, just
per board DT.

It's just that my experience of one datapoint consists of a board where
I had to increase the delay to about three times the delay calculated
from the nominal capacitance according to the datasheet before audible
artifacts disappeared reliably on multiple devices.
Putting the extended delay into the device tree with a comment
explaining its empirical nature seemed more straightforward than putting
a bogus capacitance value, that differs from the schematics, and that I
have never measured.

Also, by using the capacitance we are guaranteed to end up with a codec
specific property name (adi,vmid-decoupling-capacitance ?) as opposed to
the possibility of defining a common delay property that could be used
for different codecs.

regards
Philipp
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown June 5, 2018, 4:06 p.m. | #12
On Tue, Jun 05, 2018 at 11:58:51AM +0200, Philipp Zabel wrote:
> On Fri, 2018-05-25 at 18:24 +0100, Mark Brown wrote:

> > If this is varying so drastically per board/system that it's relevant
> > then we're already into problematic territory.  For most devices we just
> > have a number for the part, not something that varies so wildly that
> > each system needs to configure it.

> I'm not arguing this should be configured per individual device, just
> per board DT.

Even per board is a very surprising amount of variability TBH.  

> It's just that my experience of one datapoint consists of a board where
> I had to increase the delay to about three times the delay calculated
> from the nominal capacitance according to the datasheet before audible
> artifacts disappeared reliably on multiple devices.
> Putting the extended delay into the device tree with a comment
> explaining its empirical nature seemed more straightforward than putting
> a bogus capacitance value, that differs from the schematics, and that I
> have never measured.

The thing that's surprising me here is that there's even a board to
board variance that's so bad that it needs to be configured like this -
usually these things are just hard coded into the driver so they work
for everyone rather than needing per-board tuning.  It seems entirely
possible that the formula quoted in the datasheet is just nonsense in
general and that the driver would be better off using your number for
all systems for example.  If we do have to hard code something in the DT
it seems better to hard code something that is at least coming from a
spec rather than a measured number that needs to come from an average
over a batch of boards since at least we can revise how we handle the
number without needing to do something that shows up in the ABI.

> Also, by using the capacitance we are guaranteed to end up with a codec
> specific property name (adi,vmid-decoupling-capacitance ?) as opposed to
> the possibility of defining a common delay property that could be used
> for different codecs.

That's actually something I want to avoid since the delays tend to need
to come in the middle of write sequences which makes a general property
a lot more complicated, and obviously some devices have multiple delays
too.

Patch

diff --git a/Documentation/devicetree/bindings/sound/adi,ssm2602.txt b/Documentation/devicetree/bindings/sound/adi,ssm2602.txt
index 3b3302fe399b..9334d48c0462 100644
--- a/Documentation/devicetree/bindings/sound/adi,ssm2602.txt
+++ b/Documentation/devicetree/bindings/sound/adi,ssm2602.txt
@@ -11,9 +11,16 @@  Required properties:
   - reg : the I2C address of the device for I2C, the chip select
           number for SPI.
 
+Optional properties:
+
+  - startup-delay-us : delay between powering on and activating the digital
+                       core, determined by the decoupling capacitor C on the
+		       VMID pin: delay [µs] = C [µF] * 25000 / 3.5
+
  Example:
 
 	ssm2602: ssm2602@1a {
 		compatible = "adi,ssm2602";
 		reg = <0x1a>;
+		startup-delay-us = <34000>; /* 4.7 µF * 25000 / 3.5 -> ~34 ms */
 	};
diff --git a/sound/soc/codecs/ssm2602.c b/sound/soc/codecs/ssm2602.c
index 501a4e73b185..1b99f5d44f8d 100644
--- a/sound/soc/codecs/ssm2602.c
+++ b/sound/soc/codecs/ssm2602.c
@@ -26,9 +26,11 @@ 
  * 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
  */
 
+#include <linux/delay.h>
 #include <linux/module.h>
 #include <linux/regmap.h>
 #include <linux/slab.h>
+#include <linux/of.h>
 
 #include <sound/pcm.h>
 #include <sound/pcm_params.h>
@@ -46,6 +48,7 @@  struct ssm2602_priv {
 
 	enum ssm2602_type type;
 	unsigned int clk_out_pwr;
+	u32 startup_delay_us;
 };
 
 /*
@@ -111,7 +114,6 @@  SOC_SINGLE_TLV("Sidetone Playback Volume", SSM2602_APANA, 6, 3, 1,
 
 SOC_SINGLE("Mic Boost (+20dB)", SSM2602_APANA, 0, 1, 0),
 SOC_SINGLE("Mic Boost2 (+20dB)", SSM2602_APANA, 8, 1, 0),
-SOC_SINGLE("Mic Switch", SSM2602_APANA, 1, 1, 1),
 };
 
 /* Output Mixer */
@@ -121,10 +123,26 @@  SOC_DAPM_SINGLE("HiFi Playback Switch", SSM2602_APANA, 4, 1, 0),
 SOC_DAPM_SINGLE("Mic Sidetone Switch", SSM2602_APANA, 5, 1, 0),
 };
 
+static const struct snd_kcontrol_new mic_ctl =
+	SOC_DAPM_SINGLE("Switch", SSM2602_APANA, 1, 1, 1);
+
 /* Input mux */
 static const struct snd_kcontrol_new ssm2602_input_mux_controls =
 SOC_DAPM_ENUM("Input Select", ssm2602_enum[0]);
 
+static int ssm2602_mic_switch_event(struct snd_soc_dapm_widget *w,
+				struct snd_kcontrol *kcontrol, int event)
+{
+	struct snd_soc_codec *codec = snd_soc_dapm_to_codec(w->dapm);
+	struct ssm2602_priv *ssm2602 = snd_soc_codec_get_drvdata(codec);
+
+	if (ssm2602->startup_delay_us)
+		usleep_range(ssm2602->startup_delay_us,
+			     ssm2602->startup_delay_us * 2);
+
+	return 0;
+}
+
 static const struct snd_soc_dapm_widget ssm260x_dapm_widgets[] = {
 SND_SOC_DAPM_DAC("DAC", "HiFi Playback", SSM2602_PWR, 3, 1),
 SND_SOC_DAPM_ADC("ADC", "HiFi Capture", SSM2602_PWR, 2, 1),
@@ -146,6 +164,9 @@  SND_SOC_DAPM_MIXER("Output Mixer", SSM2602_PWR, 4, 1,
 SND_SOC_DAPM_MUX("Input Mux", SND_SOC_NOPM, 0, 0, &ssm2602_input_mux_controls),
 SND_SOC_DAPM_MICBIAS("Mic Bias", SSM2602_PWR, 1, 1),
 
+SND_SOC_DAPM_SWITCH_E("Mic Switch", SSM2602_APANA, 1, 1, &mic_ctl,
+		ssm2602_mic_switch_event, SND_SOC_DAPM_PRE_PMU),
+
 SND_SOC_DAPM_OUTPUT("LHPOUT"),
 SND_SOC_DAPM_OUTPUT("RHPOUT"),
 SND_SOC_DAPM_INPUT("MICIN"),
@@ -178,9 +199,11 @@  static const struct snd_soc_dapm_route ssm2602_routes[] = {
 	{"LHPOUT", NULL, "Output Mixer"},
 
 	{"Input Mux", "Line", "Line Input"},
-	{"Input Mux", "Mic", "Mic Bias"},
+	{"Input Mux", "Mic", "Mic Switch"},
 	{"ADC", NULL, "Input Mux"},
 
+	{"Mic Switch", NULL, "Mic Bias"},
+
 	{"Mic Bias", NULL, "MICIN"},
 };
 
@@ -645,6 +668,9 @@  int ssm2602_probe(struct device *dev, enum ssm2602_type type,
 	if (ssm2602 == NULL)
 		return -ENOMEM;
 
+	of_property_read_u32(dev->of_node, "startup-delay-us",
+			     &ssm2602->startup_delay_us);
+
 	dev_set_drvdata(dev, ssm2602);
 	ssm2602->type = type;
 	ssm2602->regmap = regmap;