RE: [PATCH V2 2/2] ASoC: max98388: add amplifier driver

[Lee, RyanS]

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>
> Sent: Saturday, June 10, 2023 2:24 AM
> To: “Ryan <ryan.lee.analog@xxxxxxxxx>; lgirdwood@xxxxxxxxx;
> broonie@xxxxxxxxxx; robh+dt@xxxxxxxxxx; krzysztof.kozlowski+dt@xxxxxxxxxx;
> perex@xxxxxxxx; tiwai@xxxxxxxx; rf@xxxxxxxxxxxxxxxxxxxxx; Lee, RyanS
> <RyanS.Lee@xxxxxxxxxx>; wangweidong.a@xxxxxxxxxx;
> shumingf@xxxxxxxxxxx; herve.codina@xxxxxxxxxxx;
> ckeepax@xxxxxxxxxxxxxxxxxxxxx; doug@xxxxxxxxxxxxx;
> ajye_huang@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx; kiseok.jo@xxxxxxxxxxxxxx;
> alsa-devel@xxxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx
> Cc: venkataprasad.potturu@xxxxxxx; kernel test robot <lkp@xxxxxxxxx>
> Subject: Re: [PATCH V2 2/2] ASoC: max98388: add amplifier driver
> 
> [External]
> 
> On 10/06/2023 01:44, “Ryan wrote:
> > From: Ryan Lee <ryans.lee@xxxxxxxxxx>
> >
> > Added Analog Devices MAX98388 amplifier driver.
> > MAX98388 provides a PCM interface for audio data and a standard I2C
> > interface for control data communication.
> >
> > Signed-off-by: Ryan Lee <ryans.lee@xxxxxxxxxx>
> > Reported-by: kernel test robot <lkp@xxxxxxxxx>
> 
> There is nothing to report here.
Probably I misunderstood the mail from the kernel test robot.
Removing the line.

> 
> > Closes:
> > https://urldefense.com/v3/__https://lore.kernel.org/oe-kbuild-all/2023
> > 06082054.jIU9oENf-
> lkp@xxxxxxxxx/__;!!A3Ni8CS0y2Y!46sHiAsmIiXxZ_QXIobho
> > mY8F1f7F2yMYd_65NNFwRlcgut33--RdFjVAbg6jKf7Vs8GaYZ7oA$
> 
> Nothing to close and also broken link. Fix your mailer.
> 
> > ---
> > Changes from v1:
> >   Fixed build warnings.
> >
> >  sound/soc/codecs/Kconfig    |   10 +
> >  sound/soc/codecs/Makefile   |    2 +
> >  sound/soc/codecs/max98388.c | 1042
> > +++++++++++++++++++++++++++++++++++
> >  sound/soc/codecs/max98388.h |  234 ++++++++
> >  4 files changed, 1288 insertions(+)
> >  create mode 100644 sound/soc/codecs/max98388.c  create mode 100644
> > sound/soc/codecs/max98388.h
> 
> ...
> 
> > +
> > +static void max98388_read_deveice_property(struct device *dev,
> > +					   struct max98388_priv *max98388) {
> > +	int value;
> > +
> > +	if (!device_property_read_u32(dev, "adi,vmon-slot-no", &value))
> > +		max98388->v_slot = value & 0xF;
> > +	else
> > +		max98388->v_slot = 0;
> > +
> > +	if (!device_property_read_u32(dev, "adi,imon-slot-no", &value))
> > +		max98388->i_slot = value & 0xF;
> > +	else
> > +		max98388->i_slot = 1;
> > +
> > +	if (device_property_read_bool(dev, "adi,interleave-mode"))
> > +		max98388->interleave_mode = true;
> > +	else
> > +		max98388->interleave_mode = false;
> > +
> > +	if (dev->of_node) {
> > +		max98388->reset_gpio = of_get_named_gpio(dev->of_node,
> > +							 "reset-gpio", 0);
> 
> Nope, use devm
OK.

> 
> > +		if (!gpio_is_valid(max98388->reset_gpio)) {
> > +			dev_err(dev, "Looking up %s property in node %s
> failed %d\n",
> > +				"reset-gpio", dev->of_node->full_name,
> > +				max98388->reset_gpio);
> > +		} else {
> > +			dev_dbg(dev, "reset-gpio=%d",
> > +				max98388->reset_gpio);
> > +		}
> > +	} else {
> > +		/* this makes reset_gpio as invalid */
> > +		max98388->reset_gpio = -1;
> 
> Why? To request it again? It does not make sense.

This was to make gpio_is_valid() = false in order to skip the reset GPIO control in i2c_probe().
I shall remove these codes and keep the minimum configuration in i2c_probe() function.

> 
> > +	}
> > +}
> > +
> > +static int max98388_i2c_probe(struct i2c_client *i2c) {
> > +	int ret = 0;
> > +	int reg = 0;
> > +
> > +	struct max98388_priv *max98388 = NULL;
> > +
> > +	max98388 = devm_kzalloc(&i2c->dev, sizeof(*max98388),
> GFP_KERNEL);
> > +
> 
> Drop blank line.
OK.

> 
> > +	if (!max98388) {
> > +		ret = -ENOMEM;
> 
> return -ENOMEM;
OK.

> 
> > +		return ret;
> > +	}
> > +	i2c_set_clientdata(i2c, max98388);
> > +
> > +	/* regmap initialization */
> > +	max98388->regmap = devm_regmap_init_i2c(i2c,
> &max98388_regmap);
> > +	if (IS_ERR(max98388->regmap)) {
> > +		ret = PTR_ERR(max98388->regmap);
> > +		dev_err(&i2c->dev,
> > +			"Failed to allocate regmap: %d\n", ret);
> > +		return ret;
> 
> return dev_err_probe
OK. Shall fix it.

> 
> > +	}
> > +
> > +	/* voltage/current slot & gpio configuration */
> > +	max98388_read_deveice_property(&i2c->dev, max98388);
> > +
> > +	/* Power on device */
> > +	if (gpio_is_valid(max98388->reset_gpio)) {
> 
> What's this? You request it twice? No.
Will modify the code.

> 
> 
> > +		ret = devm_gpio_request(&i2c->dev, max98388->reset_gpio,
> > +					"MAX98388_RESET");
> > +		if (ret) {
> > +			dev_err(&i2c->dev, "%s: Failed to request gpio %d\n",
> > +				__func__, max98388->reset_gpio);
> 
> return dev_err_probe
OK

> 
> > +			return -EINVAL;
> > +		}
> > +		gpio_direction_output(max98388->reset_gpio, 0);
> > +		msleep(50);
> > +		gpio_direction_output(max98388->reset_gpio, 1);
> 
> 1 means keep in reset, so why do you keep deviec reset afterwards? Was it
> tested? You probably messed up values used for GPIOs as you stated in
> example that it is active low.
The hardware reset function is active low, so the state needs to be high to restart the amp.
The code was functional, but I see room for improvement. I shall modify the code.

> 
> > +		msleep(20);
> > +	}
> > +
> > +	/* Read Revision ID */
> > +	ret = regmap_read(max98388->regmap,
> > +			  MAX98388_R22FF_REV_ID, &reg);
> > +	if (ret < 0) {
> > +		dev_err(&i2c->dev,
> > +			"Failed to read: 0x%02X\n",
> MAX98388_R22FF_REV_ID);
> > +		return ret;
> 
> return dev_err_probe
OK.

> 
> > +	}
> > +	dev_info(&i2c->dev, "MAX98388 revisionID: 0x%02X\n", reg);
> > +
> > +	/* codec registration */
> > +	ret = devm_snd_soc_register_component(&i2c->dev,
> > +					      &soc_codec_dev_max98388,
> > +					      max98388_dai,
> > +					      ARRAY_SIZE(max98388_dai));
> > +	if (ret < 0)
> > +		dev_err(&i2c->dev, "Failed to register codec: %d\n", ret);
> > +
> > +	return ret;
> > +}
> > +
> > +static const struct i2c_device_id max98388_i2c_id[] = {
> > +	{ "max98388", 0},
> > +	{ },
> > +};
> > +
> > +MODULE_DEVICE_TABLE(i2c, max98388_i2c_id);
> > +
> > +#if defined(CONFIG_OF)
> 
> Drop
OK Thanks.

> 
> > +static const struct of_device_id max98388_of_match[] = {
> > +	{ .compatible = "adi,max98388", },
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(of, max98388_of_match); #endif
> > +
> > +#ifdef CONFIG_ACPI
> 
> Drop
OK.

> 
> > +static const struct acpi_device_id max98388_acpi_match[] = {
> > +	{ "ADS8388", 0 },
> > +	{},
> > +};
> > +MODULE_DEVICE_TABLE(acpi, max98388_acpi_match); #endif
> > +
> > +static struct i2c_driver max98388_i2c_driver = {
> > +	.driver = {
> > +		.name = "max98388",
> > +		.of_match_table = of_match_ptr(max98388_of_match),
> > +		.acpi_match_table = ACPI_PTR(max98388_acpi_match),
> 
> Just drop all wrappers. They are useless and only limit your driver (OF can be
> used on some ACPI platforms).
I shall remove all wrappers.
Thanks for the review.

> 
> 
> Best regards,
> Krzysztof