From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753027AbcGTA2n (ORCPT ); Tue, 19 Jul 2016 20:28:43 -0400 Received: from mezzanine.sirena.org.uk ([106.187.55.193]:35040 "EHLO mezzanine.sirena.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752987AbcGTA2m (ORCPT ); Tue, 19 Jul 2016 20:28:42 -0400 Date: Wed, 20 Jul 2016 01:28:35 +0100 From: Mark Brown To: John Stultz Cc: lkml , Andy Green , Zhangfei Gao , Jingoo Han , Krzysztof Kozlowski , Maxime Ripard , Vinod Koul , Dan Williams , Liam Girdwood , Jaroslav Kysela , Takashi Iwai , Wei Xu , Rob Herring , Andy Green Message-ID: <20160720002835.GC6509@sirena.org.uk> References: <1468970566-24498-1-git-send-email-john.stultz@linaro.org> <1468970566-24498-8-git-send-email-john.stultz@linaro.org> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="gr/z0/N6AeWAPJVB" Content-Disposition: inline In-Reply-To: <1468970566-24498-8-git-send-email-john.stultz@linaro.org> X-Cookie: Sin boldly. User-Agent: Mutt/1.6.0 (2016-04-01) X-SA-Exim-Connect-IP: 2a01:348:6:8808:fab::3 X-SA-Exim-Mail-From: broonie@sirena.org.uk Subject: Re: [RFC][PATCH 07/10 v2] ASoC: hisilicon: Add hi6210 i2s audio driver 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 --gr/z0/N6AeWAPJVB Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Tue, Jul 19, 2016 at 04:22:43PM -0700, John Stultz wrote: > sound/soc/Kconfig | 1 + > sound/soc/Makefile | 1 + > sound/soc/hisilicon/Kconfig | 5 + > sound/soc/hisilicon/Makefile | 1 + > sound/soc/hisilicon/hi6210-i2s.c | 678 +++++++++++++++++++++++++++++++++++++++ > sound/soc/hisilicon/hi6210-i2s.h | 276 ++++++++++++++++ > 6 files changed, 962 insertions(+) This is adding a new binding without documenting it and still looks like it's far more than an I2S controller. > + switch (params_rate(params)) { > + default: > + dev_err(cpu_dai->dev, "Bad rate\n"); > + return -EINVAL; We should tell the user what rate. > + if (bits == HII2S_BITS_24) { > + i2s->bits = 32; > + dma_data->addr_width = 3; > + } else { > + i2s->bits = 16; > + dma_data->addr_width = 2; > + } This looks like it should be a switch statement, there's some similar stuff for the channels. > + _hi6210_i2s_set_fmt(i2s, substream); Why is this not in line given that this is the only user? > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + if (!res) { > + ret = -ENODEV; > + goto err2; > + } > + > + i2s->base = devm_ioremap_resource(dev, res); devm_ioremap_resource() will check the error. > +err3: > + while (--i2s->clocks) > + clk_put(i2s->clk[i2s->clocks]); > + > +err2: > + kfree(i2s); You switched to using devm_ but left the error handling. --gr/z0/N6AeWAPJVB Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAEBCAAGBQJXjsWyAAoJECTWi3JdVIfQAnMH/2zSKvlsmLNFIaA8DafQqgos dLoSOdtQbm9lF0cipuKnldIt6SF984dryv2Pbd33lOqEuvNKr/eyfvhac3e5o06+ CxQGdtU+8laNovYN0AUipRLNUfmUCn4NLgrXPc0JMhUu8NO12zsMtR1Ul8GGgs51 fh64ZJJP4FVIL1bUD0vlz10R+/tHZCMdxyUbA3UZtt2s3FLdXiFHO+djwuVNeK+b z2T8IE6SB74boTDK5XpB/jd0zfHvA6e0IRrS8AmuED4gBJTLz6UZTBnFQ713p0YM 6UZsqT+kuD97278kX5OKyHXAjENhDt1CUOBcS9U09fLvhSeb4vcMFbjcNYnQZ3I= =P0zp -----END PGP SIGNATURE----- --gr/z0/N6AeWAPJVB--