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.
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
