Re: [PATCH 1/9] ASoC: Add Chameleon v3 audio

[Paweł Anikiel]

Thank you for all the comments, I'm sorry for taking long to respond.

On Fri, Apr 14, 2023 at 7:19 PM Mark Brown <broonie@xxxxxxxxxx> wrote:
>
> 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.

Ok, I'll fix in next patchset

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

Ok, I'll fix in next patchset

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

I'm not sure about this. If I disable just one driver, the entire card
fails to probe (even if some audio device doesn't need that driver).
Does it then make sense to be able to deselect some drivers? Please
correct me if I'm misunderstanding.

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

I will read up on audio-graph-card and see if it's possible to use here

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

The benefit of lagging by an entire period is that we get an IRQ when
the first period is processed, which means we can drop the first call
to snd_pcm_period_elapsed() and have everything be coherent.

Having said that, I did try to remove that logic and simply delay
hw_pointer by one frame, and it appears to work (the playback seems
fine and without glitches). However, I'm worried about calling
snd_pcm_period_elapsed() and then reporting that the hw_pointer hasn't
actually reached the end of the period. Is that ok to do?

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

What do you mean by handled? Should I check the hardware pointer and
check if a period really has elapsed?

>
> > +     dev_info(&pdev->dev, "probed\n");
>
> This is just noise, remove it.

Ok

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

Yes, it's a dummy capture-only codec with fixed 8 channels, arbitrary
rate, and S32_LE format.

> why is it not being added as a CODEC?

Do you mean I should put it in sound/soc/codecs/?

Also, I used the name of the HDMI receiver chip (IT68051), but really
this goes through some extra processing in an FPGA, so the result has
little in common with the chip itself. Do you have any advice on how
it should be named?

Regards,
Paweł