From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1763432AbcLSQaS (ORCPT ); Mon, 19 Dec 2016 11:30:18 -0500 Received: from mga06.intel.com ([134.134.136.31]:18590 "EHLO mga06.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758021AbcLSQaO (ORCPT ); Mon, 19 Dec 2016 11:30:14 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,374,1477983600"; d="scan'208";a="1084168599" Subject: Re: [alsa-devel] [PATCH 2/2] ASoC: Intel: boards: Add Baytrail RT5660 machine driver To: Mark Brown , Shrirang Bagul References: <20161219135147.24208-1-shrirang.bagul@canonical.com> <20161219135147.24208-3-shrirang.bagul@canonical.com> <20161219155509.qs7o5tdnz5z6ksuw@sirena.org.uk> Cc: alsa-devel@alsa-project.org, Jorge Fernandez Monteagudo , Arnd Bergmann , Liam Girdwood , Vinod Koul , linux-kernel@vger.kernel.org, Takashi Iwai , Ramesh Babu , Ben Zhang , John Keeping , Sathyanarayana Nujella , Colin Ian King , Jeeja KP From: Pierre-Louis Bossart Message-ID: Date: Mon, 19 Dec 2016 10:30:09 -0600 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:45.0) Gecko/20100101 Thunderbird/45.5.1 MIME-Version: 1.0 In-Reply-To: <20161219155509.qs7o5tdnz5z6ksuw@sirena.org.uk> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 12/19/16 9:55 AM, Mark Brown wrote: > On Mon, Dec 19, 2016 at 09:51:47PM +0800, Shrirang Bagul wrote: > >> + >> +#define DEBUG > > This should be production code... > >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include > > The ordering of the headers is very random here, and is there a matching > platform patch somewhere registering clocks? > >> + /* >> + * Set codec clock source to internal clock before >> + * turning off the platform clock. Codec needs clock >> + * for Jack detection and button press >> + */ >> + ret = snd_soc_dai_set_sysclk(codec_dai, RT5660_SCLK_S_RCCLK, >> + 0, >> + SND_SOC_CLOCK_IN); >> + if (!ret) { >> + if ((byt_rt5660_quirk & BYT_RT5660_MCLK_EN) && priv->mclk) >> + clk_disable_unprepare(priv->mclk); >> + } > > Please write the error handling normally and don't embed the next steps > of the process in where the error handling would usually be. > >> + ret = snd_soc_add_card_controls(card, byt_rt5660_controls, >> + ARRAY_SIZE(byt_rt5660_controls)); >> + if (ret) { >> + dev_err(card->dev, "unable to add card controls\n"); >> + return ret; >> + } > > Why not initialize these from the card struct? > >> + if ((byt_rt5660_quirk & BYT_RT5660_MCLK_EN) && priv->mclk) { >> + /* >> + * The firmware might enable the clock at >> + * boot (this information may or may not >> + * be reflected in the enable clock register). >> + * To change the rate we must disable the clock >> + * first to cover these cases. Due to common >> + * clock framework restrictions that do not allow >> + * to disable a clock that has not been enabled, >> + * we need to enable the clock first. >> + */ >> + ret = clk_prepare_enable(priv->mclk); >> + if (!ret) >> + clk_disable_unprepare(priv->mclk); > > We don't disable the clock after (apparently) temporarily enabling it > here. > >> + /* >> + * Default mode for SSP configuration is TDM 4 slot, override config >> + * with explicit setting to I2S 2ch 16-bit. The word length is set with >> + * dai_set_tdm_slot() since there is no other API exposed >> + */ >> + ret = snd_soc_dai_set_fmt(rtd->cpu_dai, >> + SND_SOC_DAIFMT_I2S | >> + SND_SOC_DAIFMT_NB_IF | >> + SND_SOC_DAIFMT_CBS_CFS >> + ); > > Please use a normal kernel coding style and please initialize this from > the dai_link too, it's always the same. > >> + /* fixup codec name based on HID */ >> + i2c_name = sst_acpi_find_name_from_hid(mach->id); >> + if (i2c_name != NULL) { >> + snprintf(byt_rt5660_codec_name, sizeof(byt_rt5660_codec_name), >> + "%s%s", "i2c-", i2c_name); > > There's no point in formatting a static string in here, just include it > in the string. All this code seems to be largely a copy-paste of the bytcr_rt5640 machine driver and the same comments would apply there. This patch did miss the last correction merged by Mark to deal with errors "ASoC: Intel: bytcr_rt5640: fallback mechanism if MCLK is not enabled" and the same error handling would be needed. The clock registration part isn't in the mainline yet, barring a Christmas miracle we may want to wait until it's available before adding more machine drivers depending on it (I have 5 related changes for rt5651, rt5645, rt5670, max98090 and TI 32xx on my github).