linux-kernel - Re: [PATCH 2/2] ASoC: Intel: boards: Add Baytrail RT5660 machine driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20161219155509.qs7o5tdnz5z6ksuw@sirena.org.uk>
Date:   Mon, 19 Dec 2016 15:55:09 +0000
From:   Mark Brown <broonie@...nel.org>
To:     Shrirang Bagul <shrirang.bagul@...onical.com>
Cc:     alsa-devel@...a-project.org, linux-kernel@...r.kernel.org,
        Liam Girdwood <lgirdwood@...il.com>,
        Jaroslav Kysela <perex@...ex.cz>,
        Takashi Iwai <tiwai@...e.com>,
        Vinod Koul <vinod.koul@...el.com>,
        Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>,
        Jeeja KP <jeeja.kp@...el.com>, Jie Yang <yang.jie@...el.com>,
        Sathyanarayana Nujella <sathyanarayana.nujella@...el.com>,
        John Keeping <john@...anate.com>,
        Ramesh Babu <ramesh.babu@...el.com>,
        Arnd Bergmann <arnd@...db.de>,
        Colin Ian King <colin.king@...onical.com>,
        Jorge Fernandez Monteagudo <jorgefm@...sa.com>,
        Ben Zhang <benzh@...omium.org>
Subject: Re: [PATCH 2/2] ASoC: Intel: boards: Add Baytrail RT5660 machine
 driver

On Mon, Dec 19, 2016 at 09:51:47PM +0800, Shrirang Bagul wrote:

> +
> +#define DEBUG

This should be production code...

> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/acpi.h>
> +#include <linux/device.h>
> +#include <linux/dmi.h>
> +#include <linux/slab.h>
> +#include <asm/cpu_device_id.h>
> +#include <asm/platform_sst_audio.h>
> +#include <linux/clk.h>

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.

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)