Re: [PATCH v2 1/2] ASoC: cs35l41: CS35L41 Boosted Smart Amplifier

diff --git a/include/sound/cs35l41.h b/include/sound/cs35l41.h new file mode 100644 index 000000000000..f386d80fd62b --- /dev/null +++ b/include/sound/cs35l41.h @@ -0,0 +1,83 @@ +/* SPDX-License-Identifier: GPL-2.0 */

+/*

• • linux/sound/cs35l41.h -- Platform data for CS35L41
• • Copyright (c) 2017-2021 Cirrus Logic Inc.
• • Author: David Rhodes david.rhodes@cirrus.com
• • This program is free software; you can redistribute it and/or modify
• • it under the terms of the GNU General Public License version 2 as
• • published by the Free Software Foundation.

Aren't those 3 lines redundant, you've already added the SPDX statement above?

[...]

diff --git a/sound/soc/codecs/cs35l41-i2c.c b/sound/soc/codecs/cs35l41-i2c.c new file mode 100644 index 000000000000..51ef27a637c1 --- /dev/null +++ b/sound/soc/codecs/cs35l41-i2c.c @@ -0,0 +1,126 @@ +// SPDX-License-Identifier: GPL-2.0

+/*

• • cs35l41-i2c.c -- CS35l41 I2C driver
• • Copyright 2017-2021 Cirrus Logic, Inc.
• • Author: David Rhodes david.rhodes@cirrus.com
• • This program is free software; you can redistribute it and/or modify
• • it under the terms of the GNU General Public License version 2 as
• • published by the Free Software Foundation.

same here, the SPDX tag gives the info already?

• */

+#include <linux/module.h> +#include <linux/moduleparam.h> +#include <linux/version.h> +#include <linux/kernel.h> +#include <linux/init.h> +#include <linux/delay.h> +#include <linux/i2c.h> +#include <linux/slab.h> +#include <linux/platform_device.h> +#include <linux/of_device.h> +#include <linux/regmap.h> +#include <linux/gpio/consumer.h> +#include <linux/regulator/consumer.h> +#include <linux/acpi.h>

Alphabetical order?

+#include "cs35l41.h" +#include <sound/cs35l41.h>

+static struct regmap_config cs35l41_regmap_i2c = {

• .reg_bits = 32,
• .val_bits = 32,
• .reg_stride = CS35L41_REGSTRIDE,
• .reg_format_endian = REGMAP_ENDIAN_BIG,
• .val_format_endian = REGMAP_ENDIAN_BIG,
• .max_register = CS35L41_LASTREG,
• .reg_defaults = cs35l41_reg,
• .num_reg_defaults = ARRAY_SIZE(cs35l41_reg),
• .volatile_reg = cs35l41_volatile_reg,
• .readable_reg = cs35l41_readable_reg,
• .precious_reg = cs35l41_precious_reg,
• .cache_type = REGCACHE_RBTREE,

+};

+static const struct i2c_device_id cs35l41_id_i2c[] = {

• {"cs35l40", 0},
• {"cs35l41", 0},
• {}

+};

+MODULE_DEVICE_TABLE(i2c, cs35l41_id_i2c);

+static int cs35l41_i2c_probe(struct i2c_client *client,

• 		const struct i2c_device_id *id)
  

+{

• struct cs35l41_private *cs35l41;
• struct device *dev = &client->dev;
• struct cs35l41_platform_data *pdata = dev_get_platdata(dev);
• const struct regmap_config *regmap_config = &cs35l41_regmap_i2c;
• int ret;
• cs35l41 = devm_kzalloc(dev, sizeof(struct cs35l41_private), GFP_KERNEL);
• if (cs35l41 == NULL)

if (!cs35l41)

• return -ENOMEM;
  

• cs35l41->dev = dev;
• cs35l41->irq = client->irq;
• cs35l41->otp_setup = NULL;

already done with the kzalloc?

• i2c_set_clientdata(client, cs35l41);
• cs35l41->regmap = devm_regmap_init_i2c(client, regmap_config);
• if (IS_ERR(cs35l41->regmap)) {

• ret = PTR_ERR(cs35l41->regmap);
  

• dev_err(cs35l41->dev, "Failed to allocate register map: %d\n",
  

• 	ret);
  

• return ret;
  

• }
• return cs35l41_probe(cs35l41, pdata);

+}

+static int cs35l41_i2c_remove(struct i2c_client *client) +{

• struct cs35l41_private *cs35l41 = i2c_get_clientdata(client);
• return cs35l41_remove(cs35l41);

+}

+#ifdef CONFIG_OF +static const struct of_device_id cs35l41_of_match[] = {

• {.compatible = "cirrus,cs35l40"},
• {.compatible = "cirrus,cs35l41"},
• {},

+}; +MODULE_DEVICE_TABLE(of, cs35l41_of_match); +#endif

+#ifdef CONFIG_ACPI +static const struct acpi_device_id cs35l41_acpi_match[] = {

• { "CSC3541", 0 },

Where does this "CSC" come from? it's not an ACPI ID listed here: https://uefi.org/acpi_id_list

why not use the 1013 PCI ID, e.g. as done for CS42L42?

• {},

+}; +MODULE_DEVICE_TABLE(acpi, cs35l41_acpi_match); +#endif

+static struct i2c_driver cs35l41_i2c_driver = {

• .driver = {

• .name		= "cs35l41",
  

• .of_match_table = of_match_ptr(cs35l41_of_match),
  

• .acpi_match_table = ACPI_PTR(cs35l41_acpi_match),
  

• },
• .id_table = cs35l41_id_i2c,
• .probe = cs35l41_i2c_probe,
• .remove = cs35l41_i2c_remove,

+};

+module_i2c_driver(cs35l41_i2c_driver);

+MODULE_DESCRIPTION("I2C CS35L41 driver"); +MODULE_AUTHOR("David Rhodes, Cirrus Logic Inc, david.rhodes@cirrus.com"); +MODULE_LICENSE("GPL"); diff --git a/sound/soc/codecs/cs35l41-spi.c b/sound/soc/codecs/cs35l41-spi.c new file mode 100644 index 000000000000..582b7b484c21 --- /dev/null +++ b/sound/soc/codecs/cs35l41-spi.c @@ -0,0 +1,152 @@ +// SPDX-License-Identifier: GPL-2.0

+/*

• • cs35l41-spi.c -- CS35l41 SPI driver
• • Copyright 2017-2021 Cirrus Logic, Inc.
• • Author: David Rhodes david.rhodes@cirrus.com
• • This program is free software; you can redistribute it and/or modify
• • it under the terms of the GNU General Public License version 2 as
• • published by the Free Software Foundation.

redundant?

• */

+#include <linux/module.h> +#include <linux/moduleparam.h> +#include <linux/version.h> +#include <linux/kernel.h> +#include <linux/init.h> +#include <linux/delay.h> +#include <linux/platform_device.h> +#include <linux/regmap.h> +#include <linux/gpio/consumer.h> +#include <linux/spi/spi.h> +#include <linux/regulator/consumer.h> +#include <linux/acpi.h>

alphabetical order?

+#include "cs35l41.h" +#include <sound/cs35l41.h>

+static struct regmap_config cs35l41_regmap_spi = {

• .reg_bits = 32,
• .val_bits = 32,
• .pad_bits = 16,
• .reg_stride = CS35L41_REGSTRIDE,
• .reg_format_endian = REGMAP_ENDIAN_BIG,
• .val_format_endian = REGMAP_ENDIAN_BIG,
• .max_register = CS35L41_LASTREG,
• .reg_defaults = cs35l41_reg,
• .num_reg_defaults = ARRAY_SIZE(cs35l41_reg),
• .volatile_reg = cs35l41_volatile_reg,
• .readable_reg = cs35l41_readable_reg,
• .precious_reg = cs35l41_precious_reg,
• .cache_type = REGCACHE_RBTREE,

+};

+static const struct spi_device_id cs35l41_id_spi[] = {

• {"cs35l40", 0},
• {"cs35l41", 0},
• {}

+};

+MODULE_DEVICE_TABLE(spi, cs35l41_id_spi);

+static void cs35l41_spi_otp_setup(struct cs35l41_private *cs35l41,

• 			bool is_pre_setup, unsigned int *freq)
  

+{

• struct spi_device *spi = NULL;

unnecessary init

• u32 orig_spi_freq;
• spi = to_spi_device(cs35l41->dev);
• if (!spi)

• return;
  

• if (is_pre_setup) {

• orig_spi_freq = spi->max_speed_hz;
  

• if (orig_spi_freq > CS35L41_SPI_MAX_FREQ_OTP) {
  

• 	spi->max_speed_hz = CS35L41_SPI_MAX_FREQ_OTP;
  

• 	spi_setup(spi);
  

• }
  

• *freq = orig_spi_freq;
  

• } else {

• if (spi->max_speed_hz != *freq) {
  

• 	spi->max_speed_hz = *freq;
  

• 	spi_setup(spi);
  

• }
  

• }

+}

+static int cs35l41_spi_probe(struct spi_device *spi) +{

• const struct regmap_config *regmap_config = &cs35l41_regmap_spi;
• struct cs35l41_platform_data *pdata =

• 			dev_get_platdata(&spi->dev);
  

• struct cs35l41_private *cs35l41;
• int ret;
• cs35l41 = devm_kzalloc(&spi->dev,

• 	       sizeof(struct cs35l41_private),
  

• 	       GFP_KERNEL);
  

• if (cs35l41 == NULL)

if (!cs35l41)

• return -ENOMEM;
  

• spi_set_drvdata(spi, cs35l41);
• cs35l41->regmap = devm_regmap_init_spi(spi, regmap_config);
• if (IS_ERR(cs35l41->regmap)) {

• ret = PTR_ERR(cs35l41->regmap);
  

• dev_err(&spi->dev, "Failed to allocate register map: %d\n",
  

• 	ret);
  

• return ret;
  

• }
• cs35l41->dev = &spi->dev;
• cs35l41->irq = spi->irq;
• cs35l41->otp_setup = cs35l41_spi_otp_setup;
• return cs35l41_probe(cs35l41, pdata);

+}

+static int cs35l41_spi_remove(struct spi_device *spi) +{

• struct cs35l41_private *cs35l41 = spi_get_drvdata(spi);
• return cs35l41_remove(cs35l41);

+}

+#ifdef CONFIG_OF +static const struct of_device_id cs35l41_of_match[] = {

• {.compatible = "cirrus,cs35l40"},
• {.compatible = "cirrus,cs35l41"},
• {},

+}; +MODULE_DEVICE_TABLE(of, cs35l41_of_match); +#endif

+#ifdef CONFIG_ACPI +static const struct acpi_device_id cs35l41_acpi_match[] = {

• { "CSC3541", 0 }, /* Cirrus Logic PCI ID + part ID */

Wrong comment or wrong ID, "CSC" is clearly not a PCI ID?

• {},

+}; +MODULE_DEVICE_TABLE(acpi, cs35l41_acpi_match); +#endif

+static struct spi_driver cs35l41_spi_driver = {

• .driver = {

• .name		= "cs35l41",
  

• .of_match_table = of_match_ptr(cs35l41_of_match),
  

• .acpi_match_table = ACPI_PTR(cs35l41_acpi_match),
  

• },
• .id_table = cs35l41_id_spi,
• .probe = cs35l41_spi_probe,
• .remove = cs35l41_spi_remove,

+};

+module_spi_driver(cs35l41_spi_driver);

+MODULE_DESCRIPTION("SPI CS35L41 driver"); +MODULE_AUTHOR("David Rhodes, Cirrus Logic Inc, david.rhodes@cirrus.com"); +MODULE_LICENSE("GPL"); diff --git a/sound/soc/codecs/cs35l41-tables.c b/sound/soc/codecs/cs35l41-tables.c new file mode 100644 index 000000000000..16fb083e3902 --- /dev/null +++ b/sound/soc/codecs/cs35l41-tables.c @@ -0,0 +1,617 @@ +// SPDX-License-Identifier: GPL-2.0

+/*

• • cs35l41-tables.c -- CS35L41 ALSA SoC audio driver
• • Copyright 2017-2021 Cirrus Logic, Inc.
• • Author: David Rhodes david.rhodes@cirrus.com
• • This program is free software; you can redistribute it and/or modify
• • it under the terms of the GNU General Public License version 2 as
• • published by the Free Software Foundation.

ditto, redundant.

diff --git a/sound/soc/codecs/cs35l41.c b/sound/soc/codecs/cs35l41.c new file mode 100644 index 000000000000..1a1ea8a4f165 --- /dev/null +++ b/sound/soc/codecs/cs35l41.c @@ -0,0 +1,1770 @@ +// SPDX-License-Identifier: GPL-2.0

+/*

• • cs35l41.c -- CS35l41 ALSA SoC audio driver
• • Copyright 2017-2021 Cirrus Logic, Inc.
• • Author: David Rhodes david.rhodes@cirrus.com
• • This program is free software; you can redistribute it and/or modify
• • it under the terms of the GNU General Public License version 2 as
• • published by the Free Software Foundation.

ditto, redundant.

• */

+#include <linux/module.h> +#include <linux/moduleparam.h> +#include <linux/version.h> +#include <linux/kernel.h> +#include <linux/init.h> +#include <linux/delay.h> +#include <linux/slab.h> +#include <linux/regulator/consumer.h> +#include <linux/gpio/consumer.h> +#include <linux/regmap.h> +#include <linux/property.h> +#include <linux/of_device.h> +#include <sound/core.h> +#include <sound/pcm.h> +#include <sound/pcm_params.h> +#include <sound/soc.h> +#include <sound/soc-dapm.h> +#include <sound/initval.h> +#include <sound/tlv.h> +#include <linux/err.h>

alphabetical order?

+static irqreturn_t cs35l41_irq(int irq, void *data) +{

• struct cs35l41_private *cs35l41 = data;
• unsigned int status[4] = {0, 0, 0, 0};
• unsigned int masks[4] = {0, 0, 0, 0};

are those inits necessary, you override them below with the regmap reads?

• unsigned int i;
• for (i = 0; i < ARRAY_SIZE(status); i++) {

• regmap_read(cs35l41->regmap,
  

• 	    CS35L41_IRQ1_STATUS1 + (i * CS35L41_REGSTRIDE),
  

• 	    &status[i]);
  

• regmap_read(cs35l41->regmap,
  

• 	    CS35L41_IRQ1_MASK1 + (i * CS35L41_REGSTRIDE),
  

• 	    &masks[i]);
  

• }
• /* Check to see if unmasked bits are active */
• if (!(status[0] & ~masks[0]) && !(status[1] & ~masks[1]) &&

• !(status[2] & ~masks[2]) && !(status[3] & ~masks[3]))
  

• return IRQ_NONE;
  

• if (status[3] & CS35L41_OTP_BOOT_DONE) {

• regmap_update_bits(cs35l41->regmap, CS35L41_IRQ1_MASK4,
  

• 		CS35L41_OTP_BOOT_DONE, CS35L41_OTP_BOOT_DONE);
  

• }
• /*

• * The following interrupts require a
  

• * protection release cycle to get the
  

• * speaker out of Safe-Mode.
  

• */
  

• if (status[0] & CS35L41_AMP_SHORT_ERR) {

• dev_crit(cs35l41->dev, "Amp short error\n");
  

dev_crit_ratelimited? This is in an interrupt routine and can flood the console. same in the rest of the routine.

• regmap_write(cs35l41->regmap, CS35L41_IRQ1_STATUS1,
  

• 			CS35L41_AMP_SHORT_ERR);
  

• regmap_write(cs35l41->regmap, CS35L41_PROTECT_REL_ERR_IGN, 0);
  

• regmap_update_bits(cs35l41->regmap, CS35L41_PROTECT_REL_ERR_IGN,
  

• 			CS35L41_AMP_SHORT_ERR_RLS,
  

• 			CS35L41_AMP_SHORT_ERR_RLS);
  

• regmap_update_bits(cs35l41->regmap, CS35L41_PROTECT_REL_ERR_IGN,
  

• 			CS35L41_AMP_SHORT_ERR_RLS, 0);
  

• }
• if (status[0] & CS35L41_TEMP_WARN) {

• dev_crit(cs35l41->dev, "Over temperature warning\n");
  

• regmap_write(cs35l41->regmap, CS35L41_IRQ1_STATUS1,
  

• 			CS35L41_TEMP_WARN);
  

• regmap_write(cs35l41->regmap, CS35L41_PROTECT_REL_ERR_IGN, 0);
  

• regmap_update_bits(cs35l41->regmap, CS35L41_PROTECT_REL_ERR_IGN,
  

• 			CS35L41_TEMP_WARN_ERR_RLS,
  

• 			CS35L41_TEMP_WARN_ERR_RLS);
  

• regmap_update_bits(cs35l41->regmap, CS35L41_PROTECT_REL_ERR_IGN,
  

• 			CS35L41_TEMP_WARN_ERR_RLS, 0);
  

• }
• if (status[0] & CS35L41_TEMP_ERR) {

• dev_crit(cs35l41->dev, "Over temperature error\n");
  

• regmap_write(cs35l41->regmap, CS35L41_IRQ1_STATUS1,
  

• 			CS35L41_TEMP_ERR);
  

• regmap_write(cs35l41->regmap, CS35L41_PROTECT_REL_ERR_IGN, 0);
  

• regmap_update_bits(cs35l41->regmap, CS35L41_PROTECT_REL_ERR_IGN,
  

• 			CS35L41_TEMP_ERR_RLS,
  

• 			CS35L41_TEMP_ERR_RLS);
  

• regmap_update_bits(cs35l41->regmap, CS35L41_PROTECT_REL_ERR_IGN,
  

• 			CS35L41_TEMP_ERR_RLS, 0);
  

• }
• if (status[0] & CS35L41_BST_OVP_ERR) {

• dev_crit(cs35l41->dev, "VBST Over Voltage error\n");
  

• regmap_update_bits(cs35l41->regmap, CS35L41_PWR_CTRL2,
  

• 			CS35L41_BST_EN_MASK, 0);
  

• regmap_write(cs35l41->regmap, CS35L41_IRQ1_STATUS1,
  

• 			CS35L41_BST_OVP_ERR);
  

• regmap_write(cs35l41->regmap, CS35L41_PROTECT_REL_ERR_IGN, 0);
  

• regmap_update_bits(cs35l41->regmap, CS35L41_PROTECT_REL_ERR_IGN,
  

• 			CS35L41_BST_OVP_ERR_RLS,
  

• 			CS35L41_BST_OVP_ERR_RLS);
  

• regmap_update_bits(cs35l41->regmap, CS35L41_PROTECT_REL_ERR_IGN,
  

• 			CS35L41_BST_OVP_ERR_RLS, 0);
  

• regmap_update_bits(cs35l41->regmap, CS35L41_PWR_CTRL2,
  

• 			CS35L41_BST_EN_MASK,
  

• 			CS35L41_BST_EN_DEFAULT <<
  

• 			CS35L41_BST_EN_SHIFT);
  

• }
• if (status[0] & CS35L41_BST_DCM_UVP_ERR) {

• dev_crit(cs35l41->dev, "DCM VBST Under Voltage Error\n");
  

• regmap_update_bits(cs35l41->regmap, CS35L41_PWR_CTRL2,
  

• 			CS35L41_BST_EN_MASK, 0);
  

• regmap_write(cs35l41->regmap, CS35L41_IRQ1_STATUS1,
  

• 			CS35L41_BST_DCM_UVP_ERR);
  

• regmap_write(cs35l41->regmap, CS35L41_PROTECT_REL_ERR_IGN, 0);
  

• regmap_update_bits(cs35l41->regmap, CS35L41_PROTECT_REL_ERR_IGN,
  

• 			CS35L41_BST_UVP_ERR_RLS,
  

• 			CS35L41_BST_UVP_ERR_RLS);
  

• regmap_update_bits(cs35l41->regmap, CS35L41_PROTECT_REL_ERR_IGN,
  

• 			CS35L41_BST_UVP_ERR_RLS, 0);
  

• regmap_update_bits(cs35l41->regmap, CS35L41_PWR_CTRL2,
  

• 			CS35L41_BST_EN_MASK,
  

• 			CS35L41_BST_EN_DEFAULT <<
  

• 			CS35L41_BST_EN_SHIFT);
  

• }
• if (status[0] & CS35L41_BST_SHORT_ERR) {

• dev_crit(cs35l41->dev, "LBST error: powering off!\n");
  

• regmap_update_bits(cs35l41->regmap, CS35L41_PWR_CTRL2,
  

• 			CS35L41_BST_EN_MASK, 0);
  

• regmap_write(cs35l41->regmap, CS35L41_IRQ1_STATUS1,
  

• 			CS35L41_BST_SHORT_ERR);
  

• regmap_write(cs35l41->regmap, CS35L41_PROTECT_REL_ERR_IGN, 0);
  

• regmap_update_bits(cs35l41->regmap, CS35L41_PROTECT_REL_ERR_IGN,
  

• 			CS35L41_BST_SHORT_ERR_RLS,
  

• 			CS35L41_BST_SHORT_ERR_RLS);
  

• regmap_update_bits(cs35l41->regmap, CS35L41_PROTECT_REL_ERR_IGN,
  

• 			CS35L41_BST_SHORT_ERR_RLS, 0);
  

• regmap_update_bits(cs35l41->regmap, CS35L41_PWR_CTRL2,
  

• 			CS35L41_BST_EN_MASK,
  

• 			CS35L41_BST_EN_DEFAULT <<
  

• 			CS35L41_BST_EN_SHIFT);
  

• }
• return IRQ_HANDLED;

+}

[...]

diff --git a/sound/soc/codecs/cs35l41.h b/sound/soc/codecs/cs35l41.h new file mode 100644 index 000000000000..57af9d67ba72 --- /dev/null +++ b/sound/soc/codecs/cs35l41.h @@ -0,0 +1,755 @@ +/* SPDX-License-Identifier: GPL-2.0 */

+/*

• • cs35l41.h -- CS35L41 ALSA SoC audio driver
• • Copyright 2017-2021 Cirrus Logic, Inc.
• • Author: David Rhodes david.rhodes@cirrus.com
• • This program is free software; you can redistribute it and/or modify
• • it under the terms of the GNU General Public License version 2 as
• • published by the Free Software Foundation.

redundant?