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.
