diff mbox series

[V2,1/2] ASoC: dt-bindings: max98388: add amplifier driver

Message ID 20230609234417.1139839-1-ryan.lee.analog@gmail.com
State Not Applicable, archived
Headers show
Series [V2,1/2] ASoC: dt-bindings: max98388: add amplifier driver | expand

Checks

Context Check Description
robh/checkpatch success
robh/patch-applied success
robh/dtbs-check warning build log
robh/dt-meta-schema success

Commit Message

“Ryan June 9, 2023, 11:44 p.m. UTC 
From: Ryan Lee <ryans.lee@analog.com>

Add dt-bindings information for Analog Devices MAX98388 I2S Amplifier

Signed-off-by: Ryan Lee <ryans.lee@analog.com>
---
Changes from v1:
  Removed unnecessary blank line and description. Modified quotes.

 .../bindings/sound/adi,max98388.yaml          | 79 +++++++++++++++++++
 1 file changed, 79 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/adi,max98388.yaml


base-commit: 53ab6975c12d1ad86c599a8927e8c698b144d669

Comments

Krzysztof Kozlowski June 10, 2023, 9:08 a.m. UTC | #1 
On 10/06/2023 01:44, “Ryan wrote:
> From: Ryan Lee <ryans.lee@analog.com>
> 
> Add dt-bindings information for Analog Devices MAX98388 I2S Amplifier
> 
> Signed-off-by: Ryan Lee <ryans.lee@analog.com>
> ---
> Changes from v1:
>   Removed unnecessary blank line and description. Modified quotes.


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof
Krzysztof Kozlowski June 10, 2023, 9:24 a.m. UTC | #2 
On 10/06/2023 01:44, “Ryan wrote:
> From: Ryan Lee <ryans.lee@analog.com>
> 
> 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@analog.com>
> Reported-by: kernel test robot <lkp@intel.com>

There is nothing to report here.

> Closes: 
> https://urldefense.com/v3/__https://lore.kernel.org/oe-kbuild-all/2023
> 06082054.jIU9oENf-lkp@intel.com/__;!!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

> +		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.

> +	}
> +}
> +
> +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.

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

return -ENOMEM;

> +		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

> +	}
> +
> +	/* 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.


> +		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

> +			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.

> +		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

> +	}
> +	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

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

Drop

> +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).


Best regards,
Krzysztof
Lee, RyanS June 13, 2023, 6:08 a.m. UTC | #3 
> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: Saturday, June 10, 2023 2:24 AM
> To: “Ryan <ryan.lee.analog@gmail.com>; lgirdwood@gmail.com;
> broonie@kernel.org; robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org;
> perex@perex.cz; tiwai@suse.com; rf@opensource.cirrus.com; Lee, RyanS
> <RyanS.Lee@analog.com>; wangweidong.a@awinic.com;
> shumingf@realtek.com; herve.codina@bootlin.com;
> ckeepax@opensource.cirrus.com; doug@schmorgal.com;
> ajye_huang@compal.corp-partner.google.com; kiseok.jo@irondevice.com;
> alsa-devel@alsa-project.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Cc: venkataprasad.potturu@amd.com; kernel test robot <lkp@intel.com>
> 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@analog.com>
> >
> > 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@analog.com>
> > Reported-by: kernel test robot <lkp@intel.com>
> 
> 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@intel.com/__;!!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
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/sound/adi,max98388.yaml b/Documentation/devicetree/bindings/sound/adi,max98388.yaml
new file mode 100644
index 000000000000..93ccd5905736
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/adi,max98388.yaml
@@ -0,0 +1,79 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/sound/adi,max98388.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices MAX98388 Speaker Amplifier
+
+maintainers:
+  - Ryan Lee <ryans.lee@analog.com>
+
+description:
+  The MAX98388 is a mono Class-D speaker amplifier with I/V feedback.
+  The device provides a PCM interface for audio data and a standard
+  I2C interface for control data communication.
+
+allOf:
+  - $ref: dai-common.yaml#
+
+properties:
+  compatible:
+    enum:
+      - adi,max98388
+
+  reg:
+    maxItems: 1
+
+  '#sound-dai-cells':
+    const: 0
+
+  adi,vmon-slot-no:
+    description: slot number of the voltage feedback monitor
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 0
+    maximum: 15
+    default: 0
+
+  adi,imon-slot-no:
+    description: slot number of the current feedback monitor
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 0
+    maximum: 15
+    default: 1
+
+  adi,interleave-mode:
+    description:
+      For cases where a single combined channel for the I/V feedback data
+      is not sufficient, the device can also be configured to share
+      a single data output channel on alternating frames.
+      In this configuration, the current and voltage data will be frame
+      interleaved on a single output channel.
+    type: boolean
+
+  reset-gpios:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - '#sound-dai-cells'
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+        max98388: amplifier@39 {
+            compatible = "adi,max98388";
+            reg = <0x39>;
+            #sound-dai-cells = <0>;
+            adi,vmon-slot-no = <0>;
+            adi,imon-slot-no = <1>;
+            adi,interleave-mode;
+            reset-gpios = <&gpio 4 GPIO_ACTIVE_LOW>;
+        };
+    };