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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <c42ea55b-fb2f-417e-882a-a06f7660ea22@sirena.org.uk>
Date:   Tue, 25 Apr 2023 17:27:01 +0100
From:   Mark Brown <broonie@...nel.org>
To:     Paweł Anikiel <pan@...ihalf.com>
Cc:     alsa-devel@...a-project.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, lgirdwood@...il.com, perex@...ex.cz,
        tiwai@...e.com, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, dinguyen@...nel.org,
        lars@...afoo.de, nuno.sa@...log.com, upstream@...ihalf.com
Subject: Re: [PATCH 1/9] ASoC: Add Chameleon v3 audio

On Tue, Apr 25, 2023 at 05:59:52PM +0200, Paweł Anikiel wrote:

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

Consider what happens if someone for example reuses the I2S controller
in a different board.

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

It should be fine, things should be working off the hw_pointer.

> > > +     reg = readl(i2s->iobase_irq + I2S_IRQ_CLR);
> > > +     if (!reg)
> > > +             return IRQ_NONE;

> > > +     if (reg & I2S_IRQ_RX_BIT)
> > > +
> > > +     if (reg & I2S_IRQ_TX_BIT) {

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

The driver is checking for specific bits in the status register but
blindly acknowledging all bits that are set, and reporting IRQ_HANDLED
even if none are set.

> > why is it not being added as a CODEC?

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

Yes.

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

If it's genuinely unrelated to the capabilities of the actual chip then
just putting a standalone driver in the platform directory is fine, but
the code should be clear about that otherwise it looks like the code for
the device could be shared.

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)