Re: [RFC][PATCH 07/10 v2] ASoC: hisilicon: Add hi6210 i2s audio driver

From: Mark Brown <broonie@kernel.org>
To: John Stultz <john.stultz@linaro.org>
Cc: lkml <linux-kernel@vger.kernel.org>,
	Andy Green <andy.green@linaro.org>,
	Zhangfei Gao <zhangfei.gao@linaro.org>,
	Jingoo Han <jg1.han@samsung.com>,
	Krzysztof Kozlowski <k.kozlowski@samsung.com>,
	Maxime Ripard <maxime.ripard@free-electrons.com>,
	Vinod Koul <vinod.koul@intel.com>,
	Dan Williams <dan.j.williams@intel.com>,
	Liam Girdwood <lgirdwood@gmail.com>,
	Jaroslav Kysela <perex@perex.cz>, Takashi Iwai <tiwai@suse.com>,
	Wei Xu <xuwei5@hisilicon.com>, Rob Herring <robh+dt@kernel.org>,
	Andy Green <andy@warmcat.com>
Subject: Re: [RFC][PATCH 07/10 v2] ASoC: hisilicon: Add hi6210 i2s audio driver
Date: Wed, 20 Jul 2016 18:08:27 +0100	[thread overview]
Message-ID: <20160720170827.GJ6509@sirena.org.uk> (raw)
In-Reply-To: <CALAqxLV1Nn7W5DkTG4zwjVCYyT_H9KiWW-_SPEY=ngrDpemHvA@mail.gmail.com>

[-- Attachment #1: Type: text/plain, Size: 1862 bytes --]

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.
> 
> 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 = 16;
> >> +             dma_data->addr_width = 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 terse.

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]