Re: [PATCH 1/2] ASoC: add ES8156 codec driver

On 12/10/21 9:10 AM, Shumin Chen wrote:

Add a codec driver for the Everest ES8156, based on code provided by Will from Everest Sem> Signed-off-by: Shumin Chen chenshumin86@sina.com

There's an additional reference below:

+ * Author: Will pengxiaoxin@everset-semi.com

This should probably come with a Signed-off-by tag from 'Will'

https://www.kernel.org/doc/html/latest/process/submitting-patches.html#sign-...

@@ -0,0 +1,614 @@ +// SPDX-License-Identifier: GPL-2.0-only +/*

• • es8156.c -- es8156 ALSA SoC audio driver
• • Copyright Everest Semiconductor Co.,Ltd
• • Author: Will pengxiaoxin@everset-semi.com

• •     Shumin Chen <chenshumin86@sina.com>
    

• */

+#include <linux/clk.h> +#include <linux/module.h> +#include <linux/moduleparam.h> +#include <linux/init.h> +#include <linux/delay.h> +#include <linux/pm.h> +#include <linux/i2c.h> +#include <linux/spi/spi.h> +#include <linux/platform_device.h> +#include <linux/slab.h> +#include <linux/of_gpio.h> +#include <sound/core.h> +#include <sound/pcm.h> +#include <sound/pcm_params.h> +#include <sound/tlv.h> +#include <sound/soc.h> +#include <sound/soc-dapm.h> +#include <sound/initval.h> +#include <linux/proc_fs.h> +#include <linux/gpio.h> +#include <linux/interrupt.h> +#include <linux/irq.h> +#include <linux/regmap.h>

usually the headers are added by alphabetical order.

+#include "es8156.h"

+#define INVALID_GPIO -1 +#define GPIO_LOW 0 +#define GPIO_HIGH 1 +#define es8156_DEF_VOL 0xBF +#define MCLK 1

edit: these defines are used below to enable/disable parts of the code. I don't think it's quite right to do so, you would want to use means to read properties from platform firmware or use 'optional' versions of functions to get clocks and gpios.

+static struct snd_soc_component *es8156_codec;

+static const struct reg_default es8156_reg_defaults[] = {

• {0x00, 0x1c}, {0x01, 0x20}, {0x02, 0x00}, {0x03, 0x01},
• {0x04, 0x00}, {0x05, 0x04}, {0x06, 0x11}, {0x07, 0x00},
• {0x08, 0x06}, {0x09, 0x00}, {0x0a, 0x50}, {0x0b, 0x50},
• {0x0c, 0x00}, {0x0d, 0x10}, {0x10, 0x40}, {0x10, 0x40},
• {0x11, 0x00}, {0x12, 0x04}, {0x13, 0x11}, {0x14, 0xbf},
• {0x15, 0x00}, {0x16, 0x00}, {0x17, 0xf7}, {0x18, 0x00},
• {0x19, 0x20}, {0x1a, 0x00}, {0x20, 0x16}, {0x21, 0x7f},
• {0x22, 0x00}, {0x23, 0x86}, {0x24, 0x00}, {0x25, 0x07},
• {0xfc, 0x00}, {0xfd, 0x81}, {0xfe, 0x55}, {0xff, 0x10},

+};

+/* codec private data */ +struct es8156_priv {

• struct regmap *regmap;
• unsigned int dmic_amic;
• unsigned int sysclk;
• struct snd_pcm_hw_constraint_list *sysclk_constraints;
• struct clk *mclk;
• int debounce_time;
• int hp_det_invert;
• struct delayed_work work;
• int spk_ctl_gpio;
• int hp_det_gpio;
• bool muted;
• bool hp_inserted;
• bool spk_active_level;
• int pwr_count;

+};

+/*

• • es8156_reset
• */

+static int es8156_reset(struct snd_soc_component *codec) +{

• snd_soc_component_write(codec, ES8156_RESET_REG00, 0x1c);
• usleep_range(5000, 5500);
• return snd_soc_component_write(codec, ES8156_RESET_REG00, 0x03);

it's a bit odd that you care about the return of the function only in the second call, is this intentional? Or is this a shortcut for

snd_soc_component_write(codec, ES8156_RESET_REG00, 0x1c); usleep_range(5000, 5500); snd_soc_component_write(codec, ES8156_RESET_REG00, 0x03); return 0;

?

+static int es8156_set_dai_fmt(struct snd_soc_dai *codec_dai,

• 	      unsigned int fmt)
  

+{

• struct snd_soc_component *codec = codec_dai->component;
• /* set master/slave audio interface */

we use clock provider and consumer terms now.

• switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {

SND_SOC_DAIFMT_CLOCK_PROVIDER_MASK

• case SND_SOC_DAIFMT_CBM_CFM:/* es8156 master */

CBP_CFP

• snd_soc_component_update_bits(codec, ES8156_SCLK_MODE_REG02, 0x01, 0x01);
  

• break;
  

• case SND_SOC_DAIFMT_CBS_CFS:/* es8156 slave */

CBC_CFC

+static int es8156_mute(struct snd_soc_dai *dai, int mute, int stream) +{

• struct snd_soc_component *codec = dai->component;
• struct es8156_priv *es8156 = snd_soc_component_get_drvdata(codec);
• es8156->muted = mute;
• if (mute) {

if (!es8156->hp_inserted) for symmetry with the case below?

• es8156_enable_spk(es8156, false);
  

• msleep(100);
  

• snd_soc_component_update_bits(codec, ES8156_DAC_MUTE_REG13, 0x08, 0x08);
  

• } else if (dai->stream_active[SNDRV_PCM_STREAM_PLAYBACK]) {

• snd_soc_component_update_bits(codec, ES8156_DAC_MUTE_REG13, 0x08, 0x00);
  

• if (!es8156->hp_inserted)
  

• 	es8156_enable_spk(es8156, true);
  

• }
• return 0;

+}

+static const struct snd_soc_dai_ops es8156_ops = {

• .startup = NULL,
• .hw_params = es8156_pcm_hw_params,
• .set_fmt = es8156_set_dai_fmt,
• .set_sysclk = NULL,
• .mute_stream = es8156_mute,
• .shutdown = NULL,

+};

don't add callbacks that are NULL.

+static struct snd_soc_dai_driver es8156_dai = {

• .name = "ES8156 HiFi",
• .playback = {

• .stream_name = "Playback",
  

• .channels_min = 1,
  

• .channels_max = 2,
  

• .rates = es8156_RATES,
  

• .formats = es8156_FORMATS,
  

• },
• .ops = &es8156_ops,
• .symmetric_rate = 1,

not sure what the symmetry might mean if there is only playback support.

Likewise the tests above for the playback only direction can only be always true then?

+static int es8156_init_regs(struct snd_soc_component *codec) +{

• /* set clock and analog power */
• snd_soc_component_write(codec, ES8156_SCLK_MODE_REG02, 0x04);
• snd_soc_component_write(codec, ES8156_ANALOG_SYS1_REG20, 0x2A);
• snd_soc_component_write(codec, ES8156_ANALOG_SYS2_REG21, 0x3C);
• snd_soc_component_write(codec, ES8156_ANALOG_SYS3_REG22, 0x08);
• snd_soc_component_write(codec, ES8156_ANALOG_LP_REG24, 0x07);
• snd_soc_component_write(codec, ES8156_ANALOG_SYS4_REG23, 0x00);
• /* set powerup time */
• snd_soc_component_write(codec, ES8156_TIME_CONTROL1_REG0A, 0x01);
• snd_soc_component_write(codec, ES8156_TIME_CONTROL2_REG0B, 0x01);
• /* set digtal volume */

typo: digital

• snd_soc_component_write(codec, ES8156_VOLUME_CONTROL_REG14, 0xBF);
• /* set MCLK */
• snd_soc_component_write(codec, ES8156_MAINCLOCK_CTL_REG01, 0x21);
• snd_soc_component_write(codec, ES8156_P2S_CONTROL_REG0D, 0x14);
• snd_soc_component_write(codec, ES8156_MISC_CONTROL3_REG18, 0x00);
• snd_soc_component_write(codec, ES8156_CLOCK_ON_OFF_REG08, 0x3F);
• snd_soc_component_write(codec, ES8156_RESET_REG00, 0x02);
• snd_soc_component_write(codec, ES8156_RESET_REG00, 0x03);
• snd_soc_component_write(codec, ES8156_ANALOG_SYS5_REG25, 0x20);
• return 0;

+}

+static int es8156_suspend(struct snd_soc_component *codec) +{

• es8156_set_bias_level(codec, SND_SOC_BIAS_OFF);
• return 0;

+}

+static int es8156_resume(struct snd_soc_component *codec) +{

• return 0;

es8156_set_bias_level(codec, SND_SOC_BIAS_ON);

for symmetry with suspend?

+static int es8156_probe(struct snd_soc_component *codec) +{

• struct es8156_priv *es8156 = snd_soc_component_get_drvdata(codec);
• int ret = 0;
• es8156_codec = codec;

+#if MCLK

• es8156->mclk = devm_clk_get(codec->dev, "mclk");
• if (PTR_ERR(es8156->mclk) == -EPROBE_DEFER)

• return -EPROBE_DEFER;
  

• ret = clk_prepare_enable(es8156->mclk);

+#endif

Unclear how MCLK will be enabled in a build, did you mean

es8156->mclk = devm_clk_get_optional(codec->dev, "mclk");

• es8156_reset(codec);
• es8156_init_regs(codec);
• return ret;

+}

+/* es8156 7bit i2c address:CE pin:0 0x08 / CE pin:1 0x09 */ +static int es8156_i2c_probe(struct i2c_client *i2c,

• 	    const struct i2c_device_id *id)
  

+{

• struct es8156_priv *es8156;
• int ret = -1;
• es8156 = devm_kzalloc(&i2c->dev, sizeof(*es8156), GFP_KERNEL);
• if (!es8156)

• return -ENOMEM;
  

• es8156->debounce_time = 200;
• es8156->hp_det_invert = 0;
• es8156->pwr_count = 0;
• es8156->hp_inserted = false;
• es8156->muted = true;
• es8156->regmap = devm_regmap_init_i2c(i2c, &es8156_regmap_config);
• if (IS_ERR(es8156->regmap)) {

• ret = PTR_ERR(es8156->regmap);
  

• dev_err(&i2c->dev, "Failed to init regmap: %d\n", ret);
  

• return ret;
  

• }
• i2c_set_clientdata(i2c, es8156);

+#ifdef HP_DET_FUNTION

• es8156->spk_ctl_gpio = of_get_named_gpio_flags(np,

• 				       "spk-con-gpio",
  

• 				       0,
  

• 				       &flags);
  

• if (es8156->spk_ctl_gpio < 0) {

• dev_info(&i2c->dev, "Can not read property spk_ctl_gpio\n");
  

• es8156->spk_ctl_gpio = INVALID_GPIO;
  

• } else {

• es8156->spk_active_level = !(flags & OF_GPIO_ACTIVE_LOW);
  

• ret = devm_gpio_request_one(&i2c->dev, es8156->spk_ctl_gpio,
  

• 			    GPIOF_DIR_OUT, NULL);
  

• if (ret) {
  

• 	dev_err(&i2c->dev, "Failed to request spk_ctl_gpio\n");
  

• 	return ret;
  

• }
  

• es8156_enable_spk(es8156, false);
  

• }
• es8156->hp_det_gpio = of_get_named_gpio_flags(np,

• 				      "hp-det-gpio",
  

• 				      0,
  

• 				      &flags);
  

• if (es8156->hp_det_gpio < 0) {

• dev_info(&i2c->dev, "Can not read property hp_det_gpio\n");
  

• es8156->hp_det_gpio = INVALID_GPIO;
  

• } else {

• INIT_DELAYED_WORK(&es8156->work, hp_work);
  

• es8156->hp_det_invert = !!(flags & OF_GPIO_ACTIVE_LOW);
  

• ret = devm_gpio_request_one(&i2c->dev, es8156->hp_det_gpio,
  

• 			    GPIOF_IN, "hp det");
  

• if (ret < 0)
  

• 	return ret;
  

• hp_irq = gpio_to_irq(es8156->hp_det_gpio);
  

• ret = devm_request_threaded_irq(&i2c->dev, hp_irq, NULL,
  

• 				es8156_irq_handler,
  

• 				IRQF_TRIGGER_FALLING |
  

• 				IRQF_TRIGGER_RISING |
  

• 				IRQF_ONESHOT,
  

• 				"es8156_interrupt", es8156);
  

• if (ret < 0) {
  

• 	dev_err(&i2c->dev, "request_irq failed: %d\n", ret);
  

• 	return ret;
  

• }
  

• schedule_delayed_work(&es8156->work,
  

• 		      msecs_to_jiffies(es8156->debounce_time));
  

• }

+#endif

same, this will be dead code. You have to use a property or a get_optional helper.

• ret = snd_soc_register_component(&i2c->dev,

• 		     &soc_codec_dev_es8156,
  

• 		     &es8156_dai, 1);
  

use devm_?

• return ret;

+}

+static int es8156_i2c_remove(struct i2c_client *client) +{

• snd_soc_unregister_component(&client->dev);
• return 0;

+}

can be removed if devm is used above.

+static void es8156_i2c_shutdown(struct i2c_client *client) +{

• struct es8156_priv *es8156 = i2c_get_clientdata(client);
• if (es8156_codec != NULL) {

• es8156_enable_spk(es8156, false);
  

• msleep(20);
  

• es8156_set_bias_level(es8156_codec, SND_SOC_BIAS_OFF);
  

• }

unclear shutdown and remove use difference sequences? Isn't this not needed when removing the driver?

+MODULE_DESCRIPTION("ASoC es8156 driver"); +MODULE_AUTHOR("Will pengxiaoxin@everset-semi.com");

you would definitively need a Signed-off-by from this author.

+MODULE_AUTHOR("Shumin Chen chenshumin86@sina.com"); +MODULE_LICENSE("GPL");