[PATCH v2 2/2] ASoC: codecs: add support for Everest ES7202 PDM ADC

[Mark Brown]

> Add support for the Everest ES7202, a 2-channel PDM analog-to-digital
> converter controlled over I2C. The device outputs PDM data to the SoC
> PDM controller and is configured through registers for supply voltage,
> analog front-end and per-channel PGA gain.

This looks pretty good, a few relatively minor issues below but nothing
major.

> --- a/sound/soc/codecs/Kconfig
> +++ b/sound/soc/codecs/Kconfig
> @@ -1274,6 +1274,16 @@ config SND_SOC_HDMI_CODEC
>  config SND_SOC_ES7134
>  	tristate "Everest Semi ES7134 CODEC"
>  
> +config SND_SOC_ES7202
> +	tristate "Everest Semi ES7202 CODEC"
> +	depends on I2C
> +	select REGMAP_I2C

You should also add this to SND_SOC_ALL_CODECS.

> --- /dev/null
> +++ b/sound/soc/codecs/es7202.c
> @@ -0,0 +1,367 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2020 Everest Semiconductor Co Ltd.,
> + * Copyright (C) 2026 Jun Yan <jerrysteve1101 at gmail.com>
> + */

Please make the whole comment block C++ so things look more intentional.

> +#include <linux/bitfield.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <sound/pcm.h>
> +#include <sound/pcm_params.h>

linux/of.h and sound/pcm_params.h aren't needed I think.

> +static bool es7202_volatile_reg(struct device *dev, unsigned int reg)
> +{
> +	/* status and device-id registers are read-only, never cached */
> +	return reg == ES7202_CHIP_STA ||
> +	       reg == ES7202_DEV_ID1 ||
> +	       reg == ES7202_DEV_ID0;
> +}

A switch statement would be neater and would probably generate
equivalent code.  Are you sure that the ID registers are actually
volatile, that's quite unusual?  If they've volatile the hardware is
accessed on each read, if you just omit the default then they will be
cached after the first read.

Also should the rest register be uncached so we don't try to write it
back out as part of a cache sync by mistake?

> +static const struct regmap_config es7202_regmap = {
> +	.reg_bits	= 8,
> +	.val_bits	= 8,
> +	.max_register	= ES7202_DEV_ID0,
> +	.reg_defaults	= es7202_reg_defaults,
> +	.num_reg_defaults = ARRAY_SIZE(es7202_reg_defaults),
> +	.volatile_reg	= es7202_volatile_reg,
> +	.cache_type	= REGCACHE_MAPLE,

I notice that other Everest devices set use_single_read and
use_single_write - especially for write we can easily trigger
multi-register write as part of a regcache sync so if the device lacks
support it'll create bugs.

> +	es7202->supply = devm_regulator_get_optional(dev, "vdd");
> +	if (IS_ERR(es7202->supply)) {

Unless the device can operate without power this should be a normal
regulator_get(), the core will substitude a dummy regulator if it's not
configurd by the firmware.

-- 
Mark Brown <broonie at kernel.org>