On Wed, Jan 31, 2018 at 2:57 PM, Daniel Baluta <daniel.baluta@xxxxxxx> wrote: > AK5558 is a 32-bit, 768 kHZ sampling, differential input ADC > for digital audio systems. > > Datasheet is available at: > > https://www.akm.com/akm/en/file/datasheet/AK5558VN.pdf > > Initial patch includes support for normal and TDM modes. > > Signed-off-by: Junichi Wakasugi <wakasugi.jb@xxxxxxxxxxxxxxxxxxxx> > Signed-off-by: Mihai Serban <mihai.serban@xxxxxxx> > Signed-off-by: Shengjiu Wang <shengjiu.wang@xxxxxxx> > Signed-off-by: Daniel Baluta <daniel.baluta@xxxxxxx> 4 authors of the code?! > +/* > + * ak5558.c -- audio driver for AK5558 ADC > + * > + * Copyright (C) 2015 Asahi Kasei Microdevices Corporation > + * Copyright 2018 NXP > + * > + * This software is licensed under the terms of the GNU General Public > + * License version 2, as published by the Free Software Foundation, and > + * may be copied, distributed, and modified under those terms. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. SPDX instead? > + */ > +#include <linux/module.h> > +#include <linux/init.h> It would be one of them, not both. > +#include <linux/i2c.h> > +#include <linux/delay.h> > +#include <linux/slab.h> > +#include <linux/gpio/consumer.h> > +#include <sound/soc.h> > +#include <sound/soc-dapm.h> > +#include <sound/pcm.h> > +#include <sound/pcm_params.h> > +#include <sound/initval.h> > +#include <sound/tlv.h> > +#include <linux/of_gpio.h> It seems redundant. See below. > +#include <linux/regmap.h> > +#include <linux/pm_runtime.h> Better to keep list ordered. > +/* AK5558 Codec Private Data */ > +struct ak5558_priv { > + struct snd_soc_codec codec; > + struct regmap *regmap; > + struct i2c_client *i2c; > + int fs; /* Sampling Frequency */ > + int rclk; /* Master Clock */ > + int pdn_gpio; /* Power on / Reset GPIO */ struct gpio_desc *power_gpio; > + int slots; > + int slot_width; > +}; > +static int ak5558_set_mcki(struct snd_soc_codec *codec, int fs, int rclk) > +{ > + u8 mode; > +#ifndef AK5558_SLAVE_CKS_AUTO > + int mcki_rate; > +#endif > + > + dev_dbg(codec->dev, "%s fs=%d rclk=%d\n", __func__, fs, rclk); __func__ is useless with dev_dbg(). See the run time options of Dynamic Debug. > + > + mode = snd_soc_read(codec, AK5558_02_CONTROL1); > + mode &= ~AK5558_CKS; > + > +#ifdef AK5558_SLAVE_CKS_AUTO > + mode |= AK5558_CKS_AUTO; > +#else > + if (fs != 0 && rclk != 0) { if (fs && rclk) { ? > + if (rclk % fs) > + return -EINVAL; > + mcki_rate = rclk / fs; > + } > +#endif > + snd_soc_update_bits(codec, AK5558_02_CONTROL1, AK5558_CKS, mode); > + > + return 0; > +} > + > +static int ak5558_set_dai_sysclk(struct snd_soc_dai *dai, int clk_id, > + unsigned int freq, int dir) > +{ > + struct snd_soc_codec *codec = dai->codec; > + struct ak5558_priv *ak5558 = snd_soc_codec_get_drvdata(codec); > + > + dev_dbg(dai->dev, "%s(%d)\n", __func__, __LINE__); Useless. Function tracer can do this for you. > + > + ak5558->rclk = freq; > + ak5558_set_mcki(codec, ak5558->fs, ak5558->rclk); > + > + return 0; > +} > + > +static int ak5558_set_dai_fmt(struct snd_soc_dai *dai, unsigned int fmt) > +{ > + struct snd_soc_codec *codec = dai->codec; > + u8 format; > + > + dev_dbg(dai->dev, "%s(%d)\n", __func__, __LINE__); > + default: > + dev_err(codec->dev, "Clock mode unsupported"); dai->dev vs. codec->dev? > + return -EINVAL; > + } > +} > +static int ak5558_set_dai_mute(struct snd_soc_dai *dai, int mute) > +{ > + struct snd_soc_codec *codec = dai->codec; > + struct ak5558_priv *ak5558 = snd_soc_codec_get_drvdata(codec); > + int ndt; > + > + if (mute) { if (!mute) return 0; ? > + ndt = 0; > + if (ak5558->fs != 0) > + ndt = 583000 / ak5558->fs; > + if (ndt < 5) > + ndt = 5; > + msleep(ndt); msleep(max(ndt, 5)); > +} > +static int ak5558_set_tdm_slot(struct snd_soc_dai *dai, unsigned int tx_mask, > + unsigned int rx_mask, int slots, > + int slot_width) > +{ > + struct snd_soc_codec *codec = dai->codec; > + struct ak5558_priv *ak5558 = snd_soc_codec_get_drvdata(codec); > + int tdm_mode = 0; Useless assignment. > + int reg; > + > + ak5558->slots = slots; > + ak5558->slot_width = slot_width; > + > + switch (slots * slot_width) { > + case 128: > + tdm_mode = AK5558_MODE_TDM128; > + break; > + case 256: > + tdm_mode = AK5558_MODE_TDM256; > + break; > + case 512: > + tdm_mode = AK5558_MODE_TDM512; > + break; > + default: > + tdm_mode = AK5558_MODE_NORMAL; > + break; > + } > + return 0; > +} > +static int ak5558_startup(struct snd_pcm_substream *substream, > + struct snd_soc_dai *dai) > +{ > + int ret; > + > + ret = snd_pcm_hw_constraint_list(substream->runtime, 0, > + SNDRV_PCM_HW_PARAM_RATE, > + &ak5558_rate_constraints); > + return ret; return snd_pcm_hw_constraint_list(...); ? > +} > +static int ak5558_init_reg(struct snd_soc_codec *codec) > +{ > + int ret; > + struct ak5558_priv *ak5558 = snd_soc_codec_get_drvdata(codec); > + > + dev_dbg(codec->dev, "%s(%d)\n", __func__, __LINE__); Useless. > + usleep_range(10000, 11000); > + if (gpio_is_valid(ak5558->pdn_gpio)) { > + gpio_set_value_cansleep(ak5558->pdn_gpio, 0); > + usleep_range(1000, 2000); > + gpio_set_value_cansleep(ak5558->pdn_gpio, 1); > + usleep_range(1000, 2000); > + } static void ak5558_power_off(...) { if (!ak5558->power_gpiod) return; gpiod_set_value_cansleep(ak5558->power_gpiod, 0); usleep_range(1000, 2000); } static void ak5558_power_on(...) { if (!ak5558->power_gpiod) return; gpiod_set_value_cansleep(ak5558->power_gpiod, 1); usleep_range(1000, 2000); } _power_off(); _power_on(); > + return 0; > +} > +static int ak5558_remove(struct snd_soc_codec *codec) > +{ > + struct ak5558_priv *ak5558 = snd_soc_codec_get_drvdata(codec); > + > + ak5558_set_bias_level(codec, SND_SOC_BIAS_OFF); > + > + if (gpio_is_valid(ak5558->pdn_gpio)) { > + gpio_set_value_cansleep(ak5558->pdn_gpio, 0); > + usleep_range(1000, 2000); > + } _power_off(); > + > + return 0; > +} > + > +#ifdef CONFIG_PM > +static int ak5558_runtime_suspend(struct device *dev) static int __maybe_unused ... > +{ > + struct ak5558_priv *ak5558 = dev_get_drvdata(dev); > + > + regcache_cache_only(ak5558->regmap, true); > + if (gpio_is_valid(ak5558->pdn_gpio)) { > + gpio_set_value_cansleep(ak5558->pdn_gpio, 0); > + usleep_range(1000, 2000); > + } _power_off(); > + > + return 0; > +} > + > +static int ak5558_runtime_resume(struct device *dev) __maybe_unused > +{ > + struct ak5558_priv *ak5558 = dev_get_drvdata(dev); > + > + if (gpio_is_valid(ak5558->pdn_gpio)) { > + gpio_set_value_cansleep(ak5558->pdn_gpio, 0); > + usleep_range(1000, 2000); Why power off again? > + gpio_set_value_cansleep(ak5558->pdn_gpio, 1); > + usleep_range(1000, 2000); > + } _power_off(); // why? _power_on(); > +} > +#endif ugly #ifdef. > +static int ak5558_i2c_probe(struct i2c_client *i2c, > + const struct i2c_device_id *id) > +{ > + struct device_node *np = i2c->dev.of_node; > + struct ak5558_priv *ak5558; > + int ret = 0; > + > + dev_err(&i2c->dev, "%s(%d)\n", __func__, __LINE__); > + > + ak5558 = devm_kzalloc(&i2c->dev, sizeof(struct ak5558_priv), > + GFP_KERNEL); sizeof(*ak5558) ? > + if (!ak5558) > + return -ENOMEM; > + > + ak5558->regmap = devm_regmap_init_i2c(i2c, &ak5558_regmap); > + if (IS_ERR(ak5558->regmap)) > + return PTR_ERR(ak5558->regmap); > + > + i2c_set_clientdata(i2c, ak5558); > + ak5558->i2c = i2c; > + > + ak5558->pdn_gpio = of_get_named_gpio(np, "ak5558,pdn-gpio", 0); > + if (gpio_is_valid(ak5558->pdn_gpio)) { > + ret = devm_gpio_request_one(&i2c->dev, ak5558->pdn_gpio, > + GPIOF_OUT_INIT_LOW, "ak5558,pdn"); > + if (ret) { > + dev_err(&i2c->dev, "unable to get pdn gpio\n"); > + return ret; > + } > + } devm_gpiod_get_optional(); > +} > +static const struct i2c_device_id ak5558_i2c_id[] = { > + { "ak5558", 0 }, > + { } > +}; > +MODULE_DEVICE_TABLE(i2c, ak5558_i2c_id); Do you really need this? (->probe_new() callback doesn't require above to be present) > +MODULE_AUTHOR("Junichi Wakasugi <wakasugi.jb@xxxxxxxxxxxxxxxxxxxx>"); > +MODULE_AUTHOR("Mihai Serban <mihai.serban@xxxxxxx>"); 4 SoBs, 2 Authors. Please, fix accordingly. > +MODULE_DESCRIPTION("ASoC AK5558 ADC driver"); > +MODULE_LICENSE("GPL"); It is not consistent with the header. > + * ak5558.h -- audio driver for AK5558 SPDX? No name of file in the file is a good practice as well. > + * > + * Copyright (C) 2016 Asahi Kasei Microdevices Corporation > + * Copyright 2018 NXP > +/* AK5558_02_CONTROL1 fields */ > +#define AK5558_DIF 0x02 GENMASK() ? > +#define AK5558_BITS 0x04 Ditto. > +#define AK5558_CKS 0x78 Ditto. > +#define AK5558_MODE_BITS 0x60 Ditto. -- With Best Regards, Andy Shevchenko -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
