From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753023AbcGTRJK (ORCPT ); Wed, 20 Jul 2016 13:09:10 -0400 Received: from mezzanine.sirena.org.uk ([106.187.55.193]:38016 "EHLO mezzanine.sirena.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752238AbcGTRJI (ORCPT ); Wed, 20 Jul 2016 13:09:08 -0400 Date: Wed, 20 Jul 2016 18:08:27 +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: <20160720170827.GJ6509@sirena.org.uk> References: <1468970566-24498-1-git-send-email-john.stultz@linaro.org> <1468970566-24498-8-git-send-email-john.stultz@linaro.org> <20160720002835.GC6509@sirena.org.uk> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="ev7mvGV+3JQuI2Eo" Content-Disposition: inline In-Reply-To: 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 --ev7mvGV+3JQuI2Eo Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Jul 20, 2016 at 09:53:09AM -0700, John Stultz wrote: > Oh Crud. I forgot to add the cc list to those files. Sorry for that. >=20 > Here they are on the list: > https://lkml.org/lkml/2016/7/19/820 > https://lkml.org/lkml/2016/7/19/815 Please send them normally when you resubmit. > As for being more then an i2s controller, I don't have access to the > docs this was written with, so I'm not really sure what I can do to > properly extend this driver beyond acting as an i2s driver. I'll take > a look, but again, my lack of familiarity with ASoC means I may need > some extra guidance. So apologies up front. At the very least calling this an I2S controller is clearly inappropriate so there's that. > >> + } else { > >> + i2s->bits =3D 16; > >> + dma_data->addr_width =3D 2; > >> + } > > This looks like it should be a switch statement, there's some similar > > stuff for the channels. > So yea, the switch selection above this does the validation and then > I'm just applying the change here. Yes is a perfectly good word... > I can duplicate the switch in both cases, but this seems a little more te= rse. The point with using switch statements is that they don't need to be rewritten to add new cases when they come along and are the idiomatic way of doing an if A then B tree. The goal isn't to get the minimal number of characters or anything. > >> + _hi6210_i2s_set_fmt(i2s, substream); > > Why is this not in line given that this is the only user? > I think it breaks up the function some, but I can move it inline. Just pulling a random bit of the middle function out with a weirdly named subfunction isn't really clarifying, it's making me jump all over the source file to wonder why this is split and where else might be trying to do the same thing. --ev7mvGV+3JQuI2Eo Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAEBCAAGBQJXj7AKAAoJECTWi3JdVIfQYYYIAIHpLGdd6Ba1PEa/p0EAku7s +a8z42jFFyvCBr3pTa+s9ZLddP9AqIJ+Klmfg5pR74w4huN76YqQflEL6cxd3rfy vh0QoLt3IgUJwmoc8/xO5JSbzqkQJBmWnTGNJGhCTPTAmnInSxeYU1JhYj0WFVK3 to7xFkUeozkn8ckuiQErOirgbIukeM7+FvhjnF5monZlnQ9yYPvByvQMexkjcG6S 5aLWudfUjl3Y8KG1AwyemBxOb9sQSbxTJarJqA0aq4G/zGqeECucT9zeBcIRTJNc sxmxJbFOEwSjYVNHmqNO7Lm728zRzJimtgnWoN8V6+0wFfGB167umsMDEBnic+o= =5ArA -----END PGP SIGNATURE----- --ev7mvGV+3JQuI2Eo--