Re: [PATCH 6/6] ASoC: codecs: Add NeoFidelity NTP8835 codec

[Krzysztof Kozlowski <krzk@kernel.org>]

On 09/07/2024 19:28, Igor Prusov wrote:
> The NeoFidelity NTP8835 adn NTP8835C are 2.1 channel amplifiers with
> mixer and biquad filters. Both amplifiers have identical programming
> interfaces but differ in output signal characteristics.
> 


> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>

Where do you use moduleparam?

> +#include <linux/bits.h>
> +#include <linux/gpio.h>

And gpio?

> +#include <linux/slab.h>
> +#include <linux/of.h>
> +#include <linux/of_gpio.h>

And this?

Please clean up the driver first.

> +#include <linux/reset.h>
> +#include <linux/init.h>
> +#include <linux/i2c.h>
> +#include <linux/regmap.h>
> +
> +#include <sound/initval.h>
> +#include <sound/core.h>
> +#include <sound/pcm.h>
> +#include <sound/pcm_params.h>
> +#include <sound/soc.h>
> +#include <sound/soc-component.h>
> +#include <sound/tlv.h>
> +
> +#include "ntpfw.h"
> +
> +#define NTP8835_FORMATS     (SNDRV_PCM_FMTBIT_S16_LE | \
> +			     SNDRV_PCM_FMTBIT_S20_3LE | \
> +			     SNDRV_PCM_FMTBIT_S24_LE | \
> +			     SNDRV_PCM_FMTBIT_S32_LE)
> +
> +#define NTP8835_INPUT_FMT			0x0
> +#define  NTP8835_INPUT_FMT_MASTER_MODE		BIT(0)
> +#define  NTP8835_INPUT_FMT_GSA_MODE		BIT(1)
> +#define NTP8835_GSA_FMT				0x1
> +#define  NTP8835_GSA_BS_MASK			GENMASK(3, 2)
> +#define  NTP8835_GSA_BS(x)			((x) << 2)
> +#define  NTP8835_GSA_RIGHT_J			BIT(0)
> +#define  NTP8835_GSA_LSB			BIT(1)
> +#define NTP8835_SOFT_MUTE			0x26
> +#define  NTP8835_SOFT_MUTE_SM1			BIT(0)
> +#define  NTP8835_SOFT_MUTE_SM2			BIT(1)
> +#define  NTP8835_SOFT_MUTE_SM3			BIT(2)
> +#define NTP8835_PWM_SWITCH			0x27
> +#define  NTP8835_PWM_SWITCH_POF1		BIT(0)
> +#define  NTP8835_PWM_SWITCH_POF2		BIT(1)
> +#define  NTP8835_PWM_SWITCH_POF3		BIT(2)
> +#define NTP8835_PWM_MASK_CTRL0			0x28
> +#define  NTP8835_PWM_MASK_CTRL0_OUT_LOW		BIT(1)
> +#define  NTP8835_PWM_MASK_CTRL0_FPMLD		BIT(2)
> +#define NTP8835_MASTER_VOL			0x2e
> +#define NTP8835_CHNL_A_VOL			0x2f
> +#define NTP8835_CHNL_B_VOL			0x30
> +#define NTP8835_CHNL_C_VOL			0x31
> +#define REG_MAX					NTP8835_CHNL_C_VOL
> +
> +#define NTP8835_FW_NAME				"eq_8835.bin"
> +#define NTP8835_FW_MAGIC			0x38383335	/* "8835" */
> +


...


> +
> +static void ntp8835_reset_gpio(struct ntp8835_priv *ntp8835, bool active)
> +{
> +	if (active) {
> +		/*
> +		 * According to NTP8835 datasheet, 6.2 Timing Sequence (recommended):
> +		 * Deassert for T2 >= 1ms...
> +		 */
> +		reset_control_deassert(ntp8835->reset);

Explain in comment why do you need to power up device to perform
reset... This sounds odd.

> +		fsleep(1000);
> +
> +		/* ...Assert for T3 >= 0.1us... */
> +		reset_control_assert(ntp8835->reset);
> +		fsleep(1);
> +
> +		/* ...Deassert, and wait for T4 >= 0.5ms before sound on sequence. */
> +		reset_control_deassert(ntp8835->reset);
> +		fsleep(500);
> +	} else {
> +		reset_control_assert(ntp8835->reset);

This function is confusing. It is supposed to perform reset and leave
the device in active state, but here it leaves the device in reset.



> +
> +static struct snd_soc_dai_driver ntp8835_dai = {

Not const?

> +	.name = "ntp8835-amplifier",
> +	.playback = {
> +		.stream_name = "Playback",
> +		.channels_min = 1,
> +		.channels_max = 3,
> +		.rates = SNDRV_PCM_RATE_8000_192000,
> +		.formats = NTP8835_FORMATS,
> +	},
> +	.ops = &ntp8835_dai_ops,
> +};
> +
> +static struct regmap_config ntp8835_regmap = {

Not const?

Judging by weird includes and such simple issues, it looks like you try
to upstream downstream or old code. That's not how you are supposed to
bring new devices. You expect us to perform review on the same issues we
fixed already. Work on newest drivers - take them as template - so you
will not repeat the same issues we already fixed.

> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = REG_MAX,
> +	.cache_type = REGCACHE_MAPLE,
> +};
> +
> +static int ntp8835_i2c_probe(struct i2c_client *i2c)
> +{
> +	struct ntp8835_priv *ntp8835;
> +	struct regmap *regmap;
> +	int ret;
> +
> +	ntp8835 = devm_kzalloc(&i2c->dev, sizeof(struct ntp8835_priv), GFP_KERNEL);

sizeof(*)

> +	if (!ntp8835)
> +		return -ENOMEM;
> +
> +	ntp8835->i2c = i2c;
> +
> +	ntp8835->reset = devm_reset_control_get_shared(&i2c->dev, NULL);

shared is on purpose?

> +	if (IS_ERR(ntp8835->reset))
> +		return dev_err_probe(&i2c->dev, PTR_ERR(ntp8835->reset),
> +				     "Failed to get reset\n");
> +
> +	ret = reset_control_deassert(ntp8835->reset);
> +	if (ret)
> +		return dev_err_probe(&i2c->dev, PTR_ERR(ntp8835->reset),
> +				     "Failed to deassert reset\n");
> +
> +	dev_set_drvdata(&i2c->dev, ntp8835);
> +
> +	ntp8835_reset_gpio(ntp8835, true);
> +
> +	regmap = devm_regmap_init_i2c(i2c, &ntp8835_regmap);
> +	if (IS_ERR(regmap))
> +		return dev_err_probe(&i2c->dev, PTR_ERR(regmap),
> +				     "Failed to allocate regmap\n");
> +
> +	ret = devm_snd_soc_register_component(&i2c->dev, &soc_component_ntp8835,
> +					      &ntp8835_dai, 1);
> +	if (ret)
> +		return dev_err_probe(&i2c->dev, ret,
> +				     "Failed to register component\n");
> +
> +	return 0;
> +}
> +
> +static const struct i2c_device_id ntp8835_i2c_id[] = {
> +	{ "ntp8835", 0 },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(i2c, ntp8835_i2c_id);
> +
> +static const struct of_device_id ntp8835_of_match[] = {
> +	{.compatible = "neofidelity,ntp8835",},
> +	{.compatible = "neofidelity,ntp8835c",},

This does not match your i2c IDs, which leads to troubles when matching
variants.

Anyway, aren't they compatible?


> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, ntp8835_of_match);
> +
> +static struct i2c_driver ntp8835_i2c_driver = {
> +	.probe = ntp8835_i2c_probe,
> +	.id_table = ntp8835_i2c_id,
> +	.driver = {
> +		.name = "NTP8835",

Driver names are lowercase

> +		.of_match_table = ntp8835_of_match,
> +	},
> +};
> +module_i2c_driver(ntp8835_i2c_driver);
> +
> +MODULE_AUTHOR("Igor Prusov <ivprusov@salutedevices.com>");
> +MODULE_DESCRIPTION("NTP8835 Audio Amplifier Driver");
> +MODULE_LICENSE("GPL");

Best regards,
Krzysztof