On Thu, Sep 24, 2015 at 04:30:05PM +0200, codekipper@gmail.com wrote:
From: Marcus Cooper codekipper@gmail.com
The sun4i, sun6i and sun7i SoC families have an SPDIF block which is capable of playback and capture.
I'm not seeing patches 1 or 2 - what's the story here, are there dependencies? Please use subject lines matching the style for the subsystem and also don't fill your subject lines with noisy tags beyond "[PATCH n/x]", when I look at this in my mail client what I see is:
-> 432 C 09/24 codekipper@gmai ( 27K) [linux-sunxi][alsa-devel][PATCH 3/3] AS
sound/soc/sunxi/Kconfig | 10 + sound/soc/sunxi/Makefile | 4 + sound/soc/sunxi/sunxi-machine-spdif.c | 110 +++++ sound/soc/sunxi/sunxi-spdif.c | 801 ++++++++++++++++++++++++++++++++++
The machine driver and controller driver should be submitted as separate patches for ease of review. Is there a strong reason for not using simple-card?
+void sunxi_snd_txctrl(struct snd_pcm_substream *substream,
struct sunxi_spdif_dev *host, int on)+{
- u32 tmp;
There's no meaningful sharing between the enable and disable paths and only one place either is called, it's better to just inline this into the callers.
- if (!cpu_dai->active) {
ret = clk_prepare_enable(host->clk);if (ret)return ret;- }
Can you move the clock enables to runtime PM and let the core do runtime PM for you?
+static int sunxi_spdif_set_clkdiv(struct snd_soc_dai *cpu_dai,
unsigned int rate, int div)+{
- struct sunxi_spdif_dev *host = snd_soc_dai_get_drvdata(cpu_dai);
- int sample_freq, original_sample_freq;
Why are you implementing a set_clkdiv() operation - is the driver not capable of working out its internal clocking automaticallly?
+static int sunxi_spdif_hw_params(struct snd_pcm_substream *substream,
struct snd_pcm_hw_params *params,struct snd_soc_dai *cpu_dai)+{
- ret = snd_soc_dai_set_fmt(cpu_dai, fmt);
- if (ret < 0)
return ret;
This looks very broken - what is this doing and why?
+static struct snd_soc_dai_driver sunxi_spdif_dai = {
- .playback = {
.channels_min = 2,.channels_max = 2,.rates = SUNXI_RATES,
There was code in the driver to handle mono signals but this says only stereo is supported?
- if (clk_prepare_enable(host->apb_clk)) {
dev_err(&pdev->dev, "try to enable apb_spdif_clk failed\n");return -EINVAL;- }
Don't ignore the error code you got from the API, print it and pass it back.