| Message ID | 20230414140203.707729-1-pan@semihalf.com |
|---|---|
| Headers | show |
| Series | Add Chameleon v3 ASoC audio | expand |
On 14/04/2023 16:01, Paweł Anikiel wrote: > Add machine and platform drivers for ASoC audio on Chameleon v3. > > The board has two audio sources: HDMI audio from the it68051 chip > (RX only), and analog audio from the ssm2603 chip (RX and TX). > > Signed-off-by: Paweł Anikiel <pan@semihalf.com> > --- > .../boot/dts/socfpga_arria10_chameleonv3.dts | 28 ++ No way. DTS is always, always separate. Best regards, Krzysztof
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?
On Fri, Apr 14, 2023 at 04:02:01PM +0200, Paweł Anikiel wrote: > @@ -352,6 +355,10 @@ static int ssm2602_mute(struct snd_soc_dai *dai, int mute, int direction) > else > regmap_update_bits(ssm2602->regmap, SSM2602_APDIGI, > APDIGI_ENABLE_DAC_MUTE, 0); > + > + if (ssm2602->mute_gpiod) > + gpiod_set_value_cansleep(ssm2602->mute_gpiod, mute); > + It seems pointless to control both the mute register and the GPIO mute here, we gain nothing but overhead from having the GPIO. What I would suggest is that if the GPIO is present then that is used in _mute() and we add a control allowing the user to mute and unmute via the register.
On Fri, 14 Apr 2023 16:01:54 +0200, Paweł Anikiel wrote: > The Google Chameleon v3 is a device made for testing audio and video > paths of other devices. This patchset adds support for ASoC audio on > this device. It has two audio sources: HDMI audio from the it68051 chip > (RX only), and analog audio from the ssm2603 chip (RX and TX). > > The patchset adds the ASoC platform and machine drivers, as well as some > changes to the existing ssm2602 codec driver. > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next Thanks! [6/9] ASoC: ssm2602: Add support for CLKDIV2 commit: 8076c586bbc1c62e075e58f41dafdd8b5022b24d All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark
On Fri, Apr 14, 2023 at 6:47 PM Mark Brown <broonie@kernel.org> wrote: > > On Fri, Apr 14, 2023 at 04:01:54PM +0200, Paweł Anikiel wrote: > > > sound/soc/Makefile | 1 + > > sound/soc/chameleonv3/Kconfig | 7 + > > sound/soc/chameleonv3/Makefile | 2 + > > sound/soc/chameleonv3/chv3-audio.c | 111 ++++++ > > sound/soc/chameleonv3/chv3-i2s.c | 347 ++++++++++++++++++ > > sound/soc/chameleonv3/chv3-it68051.c | 41 +++ > > Please at least make a directory for Google as a vendor, we don't want > people adding directories for each individual product. That said > generally we add machine drivers in the directory for the relevant SoC > family, is there any reason that pattern isn't followed here? The board is based around an Intel Arria 10 SoC FPGA. The ring buffer device and all the routing is implemented inside the FPGA. Is it ok to put the machine driver in the product directory in this case?. As for the directory path, would sound/soc/google/chameleonv3/* be ok? Regards, Paweł
On Tue, Apr 25, 2023 at 05:58:59PM +0200, Paweł Anikiel wrote: > > Please at least make a directory for Google as a vendor, we don't want > > people adding directories for each individual product. That said > > generally we add machine drivers in the directory for the relevant SoC > > family, is there any reason that pattern isn't followed here? > The board is based around an Intel Arria 10 SoC FPGA. The ring buffer > device and all the routing is implemented inside the FPGA. Is it ok to > put the machine driver in the product directory in this case?. As for > the directory path, would sound/soc/google/chameleonv3/* be ok? Does the individual product really need a directory - nobody's going to reuse the IP on the FPGA for anything?