Messages in this thread | | | Date | Thu, 24 Sep 2015 10:29:47 -0700 | From | Mark Brown <> | Subject | Re: [linux-sunxi][alsa-devel][PATCH 3/3] ASOC: sunxi: Add support for the spdif block |
| |
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. [unhandled content-type:application/pgp-signature] | |