On Fri, Apr 14, 2023 at 04:01:55PM +0200, Paweł Anikiel wrote:

> ---
> .../boot/dts/socfpga_arria10_chameleonv3.dts | 28 ++

Updates to the DT should be in a separate patch.

> sound/soc/chameleonv3/chv3-audio.c | 111 ++++++
> sound/soc/chameleonv3/chv3-i2s.c | 347 ++++++++++++++++++
> sound/soc/chameleonv3/chv3-it68051.c | 41 +++

The machine driver and board drivers (if needed) should also be separate
patches - one patch per driver.

> +config SND_SOC_CHV3
> + tristate "SoC Audio support for Chameleon v3"
> + select SND_SOC_SSM2602
> + select SND_SOC_SSM2602_I2C
> + help
> + Say Y if you want to add audio support for the Chameleon v3.

It woudl be better to have a separate selectable symbol for each drier.

> +static int chv3_ssm2603_hw_params(struct snd_pcm_substream *substream,
> + struct snd_pcm_hw_params *params)
> +{
> + struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
> + struct snd_soc_dai *dai = asoc_rtd_to_codec(rtd, 0);
> +
> + return snd_soc_dai_set_sysclk(dai, 0, 22579200, SND_SOC_CLOCK_IN);
> +}

This could be done once at init, though in general I can't tell why this
isn't audio-graph-card.

> + * Because of the two pointer design, the ring buffer can never be full. With
> + * capture this isn't a problem, because the hardware being the producer
> + * will wait for the consumer index to move out of the way. With playback,
> + * however, this is problematic, because ALSA wants to fill up the buffer
> + * completely when waiting for hardware. In the .ack callback, the driver
> + * would have to wait for the consumer index to move out of the way by
> + * busy-waiting, which would keep stalling the kernel for quite a long time.
> + *
> + * The workaround to this problem is to "lie" to ALSA that the hw_pointer
> + * is one period behind what it actually is (see chv3_dma_pointer). This
> + * way, ALSA will not try to fill up the entire buffer, and all callbacks
> + * are wait-free.

Would it not be better to just lag by one (or some small number of)
sample instead?

> +static irqreturn_t chv3_i2s_isr(int irq, void *data)
> +{
> + struct chv3_i2s_dev *i2s = data;
> + u32 reg;
> +
> + reg = readl(i2s->iobase_irq + I2S_IRQ_CLR);
> + if (!reg)
> + return IRQ_NONE;
> +
> + if (reg & I2S_IRQ_RX_BIT)
> + snd_pcm_period_elapsed(i2s->rx_substream);
> +
> + if (reg & I2S_IRQ_TX_BIT) {
> + if (i2s->tx_ready)
> + snd_pcm_period_elapsed(i2s->tx_substream);
> + i2s->tx_ready = 1;
> + }
> +
> + writel(reg, i2s->iobase_irq + I2S_IRQ_CLR);
> +
> + return IRQ_HANDLED;
> +}

Really we should only ack things that were handled here and report
appropriately, that's defensive against bugs causing interrupts to
scream and shared interrupts.

> + dev_info(&pdev->dev, "probed\n");

This is just noise, remove it.

> +++ b/sound/soc/chameleonv3/chv3-it68051.c
> @@ -0,0 +1,41 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +#include <linux/module.h>
> +#include <sound/soc.h>
> +
> +static struct snd_soc_dai_driver it68051_dai = {
> + .name = "it68051-hifi",
> + .capture = {
> + .stream_name = "Capture",
> + .channels_min = 8,
> + .channels_max = 8,
> + .rates = SNDRV_PCM_RATE_CONTINUOUS,
> + .formats = SNDRV_PCM_FMTBIT_S32_LE,
> + },
> +};
> +
> +static const struct snd_soc_component_driver soc_component_dev_it68051 = {
> +};

This looks awfully like it's a generic CODEC driver for a device with no
control available, why is it not being added as a CODEC?

Attachments:

(No filename) (3.53 kB)
signature.asc (499.00 B)
Download all attachments

2023-04-14 17:36:14

by Mark Brown

[permalink] [raw]

Subject: Re: [PATCH 5/9] ASoC: ssm2602: Add workaround for playback with external MCLK

On Fri, Apr 14, 2023 at 04:01:59PM +0200, Paweł Anikiel wrote:

> Apply a workaround for what seems to be a hardware quirk: when using
> an external MCLK signal, powering on Output and DAC for the first time
> produces output distortions unless they're powered together with whole
> chip power.

This doesn't seem coherent, these are multiple register writes so
clearly can't be done at the same moment as initial power on. Clearly
there's some other constraint here.

> The workaround powers them on in probe for the first time, as doing it
> later may be impossible (e.g. when starting playback while recording,
> whole chip power will already be on).

It doesn't do that, it powers them on at component probe.

> Here are some sequences run at the very start before a sw reset (and
> later using one of the NOT OK sequences from above):
>
> ssmset 0x09 0x01 # core
> ssmset 0x06 0x07 # chip, out, dac
> OK

I can't tell what any of this is trying to say, especially given all the
magic numbers, and obviously no actual use of the driver should be
writing directly to the register map.

> + /* Workaround for what seems to be a hardware quirk: when using an
> + * external MCLK signal, powering on Output and DAC for the first
> + * time produces output distortions unless they're powered together
> + * with whole chip power. We power them here for the first time,
> + * as doing it later may be impossible (e.g. when starting playback
> + * while recording, whole chip power will already be on)
> + */
> + regmap_write(ssm2602->regmap, SSM2602_ACTIVE, 0x01);
> + regmap_write(ssm2602->regmap, SSM2602_PWR, 0x07);
> + regmap_write(ssm2602->regmap, SSM2602_RESET, 0x00);
> +

The rest of the driver uses symbolic names for register values, this
code should too.

This also seems buggy in that it writes non-default values to the
hardware then does a reset, meaning that the cache and hardware values
will be out of sync, and since it only happens on probe there will be an
issue after suspend if power is removed. It looks like this would be
most comfortably implemented as a register patch applied as soon as the
regmap is instantiated. See regmap_register_patch().

Attachments:

(No filename) (2.19 kB)
signature.asc (499.00 B)
Download all attachments

2023-04-14 17:49:49

(No filename) (760.00 B)
signature.asc (499.00 B)
Download all attachments

2023-04-26 13:17:47

by Krzysztof Kozlowski

[permalink] [raw]

Subject: Re: [PATCH 2/9] dt-bindings: ASoC: Add chv3-i2s

On 25/04/2023 18:01, Paweł Anikiel wrote:
> On Fri, Apr 14, 2023 at 7:00 PM Krzysztof Kozlowski
> <[email protected]> wrote:
>>
>> On 14/04/2023 16:01, Paweł Anikiel wrote:
>>> Add binding for chv3-i2s device.
>>
>> Your subject needs improvements:
>> 1. ASoC goes before bindings
>> 2. You miss some meaningful title of device. "chv3-i2s" can be anything,
>> so add Google or Google Chameleon. Or use entire compatible.
>
> Would "ASoC: dt-bindings: Add Google Chameleon v3 I2S device" be better?

Yes, thanks.

>
>>
>>
>>>
>>> Signed-off-by: Paweł Anikiel <[email protected]>
>>> ---
>>> .../bindings/sound/google,chv3-i2s.yaml | 42 +++++++++++++++++++
>>> 1 file changed, 42 insertions(+)
>>> create mode 100644 Documentation/devicetree/bindings/sound/google,chv3-i2s.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/sound/google,chv3-i2s.yaml b/Documentation/devicetree/bindings/sound/google,chv3-i2s.yaml
>>> new file mode 100644
>>> index 000000000000..6f49cf059ac5
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/sound/google,chv3-i2s.yaml
>>> @@ -0,0 +1,42 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/sound/google,chv3-i2s.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Google Chameleon v3 I2S device
>>> +
>>> +maintainers:
>>> + - Paweł Anikiel <[email protected]>
>>> +
>>> +description: |
>>> + I2S device for the Google Chameleon v3. The device handles both RX
>>> + and TX using a producer/consumer ring buffer design.
>>> +
>>> +properties:
>>> + compatible:
>>> + const: google,chv3-i2s
>>
>> Missing blank line.
>>
>> Is chv3 the name of your SoC? Where are the SoC bindings? What's exactly
>> the versioning scheme for it (compatibles must be specific, not generic).
>
> The Chameleon v3 is based around an Intel Arria 10 SoC FPGA. The i2s
> device is implemented inside the FPGA. Does this case require SoC
> bindings?

No, I was mistaken. I somehow get impression that's for Pixel... Sorry
for the noise.

>
>>
>>> + reg:
>>> + items:
>>> + - description: core registers
>>> + - description: irq registers
>>
>> As well...
>>
>>> + interrupts:
>>> + maxItems: 1
>>> +
>>> +required:
>>> + - compatible
>>> + - reg
>>> + - interrupts
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> + - |
>>> + #include <dt-bindings/interrupt-controller/irq.h>
>>> +
>>> + i2s0: i2s@c0060300 {
>>> + compatible = "google,chv3-i2s";
>>> + reg = <0xc0060300 0x100>,
>>> + <0xc0060f00 0x10>;
>>> + interrupts = <0 20 IRQ_TYPE_LEVEL_HIGH>;
>>
>> Isn't 0 also a known define?
>
> Do you mean this?
> interrupts = <GIC_SPI 20 IRQ_TYPE_LEVEL_HIGH>;

Yes, please.

Best regards,
Krzysztof