> 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@xxxxxxxxxx>
> + *
> + * 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@xxxxxxxxxx>
> + *
> + * 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@xxxxxxxxxx>");
> +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@xxxxxxxxxx>
> + *
> + * 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@xxxxxxxxxx>");
> +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@xxxxxxxxxx>
> + *
> + * 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@xxxxxxxxxx>
> + *
> + * 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@xxxxxxxxxx>
> + *
> + * 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?
