From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757089AbbIXRah (ORCPT ); Thu, 24 Sep 2015 13:30:37 -0400 Received: from mezzanine.sirena.org.uk ([106.187.55.193]:48536 "EHLO mezzanine.sirena.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754126AbbIXRaf (ORCPT ); Thu, 24 Sep 2015 13:30:35 -0400 Date: Thu, 24 Sep 2015 10:29:47 -0700 From: Mark Brown To: codekipper@gmail.com Cc: maxime.ripard@free-electrons.com, lgirdwood@gmail.com, linux-arm-kernel@lists.infradead.org, linux-sunxi@googlegroups.com, linux-kernel@vger.kernel.org, alsa-devel@alsa-project.org, be17068@iperbole.bo.it Message-ID: <20150924172947.GA30445@sirena.org.uk> References: <1443105005-4083-1-git-send-email-codekipper@gmail.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="UBSsiRpyblo+v+13" Content-Disposition: inline In-Reply-To: <1443105005-4083-1-git-send-email-codekipper@gmail.com> X-Cookie: NOBODY EXPECTS THE SPANISH INQUISITION! User-Agent: Mutt/1.5.23 (2014-03-12) X-SA-Exim-Connect-IP: 70.35.39.2 X-SA-Exim-Mail-From: broonie@sirena.org.uk Subject: Re: [linux-sunxi][alsa-devel][PATCH 3/3] ASOC: sunxi: Add support for the spdif block X-SA-Exim-Version: 4.2.1 (built Mon, 26 Dec 2011 16:24:06 +0000) X-SA-Exim-Scanned: Yes (on mezzanine.sirena.org.uk) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --UBSsiRpyblo+v+13 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Sep 24, 2015 at 04:30:05PM +0200, codekipper@gmail.com wrote: > From: Marcus Cooper >=20 > 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 =3D 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 =3D 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 =3D 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 =3D { > + .playback =3D { > + .channels_min =3D 2, > + .channels_max =3D 2, > + .rates =3D 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. --UBSsiRpyblo+v+13 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAEBCAAGBQJWBDMLAAoJECTWi3JdVIfQu/gH/2uQeVGvS5FVelcHL7XEJlRd pfIfIDx+QW2Gewrct72soDCJ2Ha4wVuV5ZpMYox6zj4RXKX0johAHauFog/nluC/ afnvVKvnugAJph8FX1/vHxSLArxMQSFbofswvhT0thC1fb+lksfrmpr5lhVcRfAh o+tXmUWYjXP7/NYcil2wswxDPythSHIEZ7H/xapmZdoBK2/nEqKsyHEfxdYf7yIJ fXMBMqKSuO+oBGuPsOluD9DT8Vt3rkiSFIhw0qlW1BJ/3NrZwyfF9STT+Shf6rlI 7OFgfOHp8aXkJ3lf82pFu4m7EJX9y3ocDiiHmFkK+32WK8zyZjByTaHpjZywNfU= =s/l0 -----END PGP SIGNATURE----- --UBSsiRpyblo+v+13--