Messages in this thread | | | Date | Tue, 25 Apr 2023 17:27:01 +0100 | From | Mark Brown <> | 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. [unhandled content-type:application/pgp-signature] | |