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

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

[Pierre-Louis Bossart]

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-your-work-the-developer-s-certificate-of-origin


> @@ -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");

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1079 bytes --]

On Fri, Dec 10, 2021 at 09:53:39AM -0600, Pierre-Louis Bossart wrote:
> On 12/10/21 9:10 AM, Shumin Chen wrote:

> > +static int es8156_resume(struct snd_soc_component *codec)
> > +{
> > +	return 0;

> es8156_set_bias_level(codec, SND_SOC_BIAS_ON);

> for symmetry with suspend?

Shouldn't be _ON since we're resuming so there should be no running
audio - _STANDBY would be more normal.

> > +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");

Please also move all resource acquisition to the I2C level probe, that
means we don't get the card trying to instantiate repeatedly while the
CODEC isn't ready.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/2] ASoC: convert Everest ES8156 binding to yaml

[Pierre-Louis Bossart]

On 12/10/21 9:10 AM, Shumin Chen wrote:
> This patch converts Everest Semiconductor ES8156 low power audio
> CODEC binding to DT schema.

this doesn't really convert anything but *adds* properties for ES8156.

You're probably missing the device tree folks, added Rob + device tree
mailing list in CC:

> 
> Signed-off-by: Shumin Chen <chenshumin86@sina.com>
> ---
>  .../bindings/sound/everest,es8156.yaml        | 49 +++++++++++++++++++
>  1 file changed, 49 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/sound/everest,es8156.yaml
> 
> diff --git a/Documentation/devicetree/bindings/sound/everest,es8156.yaml b/Documentation/devicetree/bindings/sound/everest,es8156.yaml
> new file mode 100644
> index 000000000000..695d542013c2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/everest,es8156.yaml
> @@ -0,0 +1,49 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/sound/everest,es8156.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Everest ES8156 audio CODEC
> +
> +maintainers:
> +  - Shumin Chen <chenshumin86@sina.com>
> +
> +properties:
> +  compatible:
> +    const: everest,es8156
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    items:
> +      - description: clock for master clock (MCLK)
> +
> +  clock-names:
> +    items:
> +      - const: mclk
> +
> +  "#sound-dai-cells":
> +    const: 0
> +
> +required:
> +  - compatible
> +  - reg
> +  - "#sound-dai-cells"
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c0 {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +      es8156: codec@8 {
> +        compatible = "everest,es8156";
> +        reg = <0x11>;
> +        clocks = <&clks 10>;
> +        clock-names = "mclk";
> +        #sound-dai-cells = <0>;
> +      };
> +    };
> 

Re: [PATCH 2/2] ASoC: convert Everest ES8156 binding to yaml

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 305 bytes --]

On Fri, Dec 10, 2021 at 11:10:41PM +0800, Shumin Chen wrote:

> +properties:
> +  compatible:
> +    const: everest,es8156
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    items:
> +      - description: clock for master clock (MCLK)

This was optional in the code but is mandatory in the binding.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[kernel test robot]

Hi Shumin,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on broonie-sound/for-next]
[also build test WARNING on v5.16-rc4 next-20211208]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Shumin-Chen/This-patches-provide-ASoc-codec-support-for-ES8156/20211210-231527
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
config: x86_64-buildonly-randconfig-r003-20211210 (https://download.01.org/0day-ci/archive/20211211/202112110101.i4FDNZ9c-lkp@intel.com/config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/4ea33be548d361d7097073473fb018e016091622
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Shumin-Chen/This-patches-provide-ASoc-codec-support-for-ES8156/20211210-231527
        git checkout 4ea33be548d361d7097073473fb018e016091622
        # save the config file to linux build tree
        mkdir build_dir
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash sound/soc/codecs/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> sound/soc/codecs/es8156.c:402:5: warning: no previous prototype for 'es8156_headset_detect' [-Wmissing-prototypes]
     402 | int es8156_headset_detect(int jack_insert)
         |     ^~~~~~~~~~~~~~~~~~~~~
   sound/soc/codecs/es8156.c:421:13: warning: 'hp_work' defined but not used [-Wunused-function]
     421 | static void hp_work(struct work_struct *work)
         |             ^~~~~~~
   sound/soc/codecs/es8156.c:386:20: warning: 'es8156_irq_handler' defined but not used [-Wunused-function]
     386 | static irqreturn_t es8156_irq_handler(int irq, void *data)
         |                    ^~~~~~~~~~~~~~~~~~


vim +/es8156_headset_detect +402 sound/soc/codecs/es8156.c

   395	
   396	/*
   397	 * Call from rk_headset_irq_hook_adc.c
   398	 *
   399	 * Enable micbias for HOOK detection and disable external Amplifier
   400	 * when jack insertion.
   401	 */
 > 402	int es8156_headset_detect(int jack_insert)
   403	{
   404		struct es8156_priv *es8156;
   405	
   406		if (!es8156_codec)
   407			return -1;
   408	
   409		es8156 = snd_soc_component_get_drvdata(es8156_codec);
   410	
   411		es8156->hp_inserted = jack_insert;
   412	
   413		/* enable micbias and disable PA */
   414		if (jack_insert)
   415			es8156_enable_spk(es8156, false);
   416	
   417		return 0;
   418	}
   419	EXPORT_SYMBOL(es8156_headset_detect);
   420	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org