[PATCH 1/5] ALSA: hda: cs35l41: Avoid overwriting register patch

[PATCH 1/5] ALSA: hda: cs35l41: Avoid overwriting register patch

[Lucas Tanure]

From: Charles Keepax <ckeepax@opensource.cirrus.com>

regmap_register_patch can't be used to apply the probe sequence as a
patch is already registers with the regmap by
cs35l41_register_errata_patch and only a single patch can be attached to
a single regmap. The driver doesn't currently rely on a cache sync to
re-apply this probe sequence so simply switch it to a multi write.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
Signed-off-by: Lucas Tanure <tanureal@opensource.cirrus.com>
---
 sound/pci/hda/cs35l41_hda.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/pci/hda/cs35l41_hda.c b/sound/pci/hda/cs35l41_hda.c
index 30b40d865863..c47c5f0b4e59 100644
--- a/sound/pci/hda/cs35l41_hda.c
+++ b/sound/pci/hda/cs35l41_hda.c
@@ -480,7 +480,7 @@ int cs35l41_hda_probe(struct device *dev, const char *device_name, int id, int i
 	acpi_hw_cfg = NULL;
 
 	if (cs35l41->reg_seq->probe) {
-		ret = regmap_register_patch(cs35l41->regmap, cs35l41->reg_seq->probe,
+		ret = regmap_multi_reg_write(cs35l41->regmap, cs35l41->reg_seq->probe,
 					    cs35l41->reg_seq->num_probe);
 		if (ret) {
 			dev_err(cs35l41->dev, "Fail to apply probe reg patch: %d\n", ret);
-- 
2.34.1

[PATCH 2/5] ALSA: hda: cs35l41: Add calls to newly added test key function

[Lucas Tanure]

From: Charles Keepax <ckeepax@opensource.cirrus.com>

The test key now needs to be manually held when calling
cs35l41_register_errata_patch, after patch:

'commit f517ba4924ad ("ASoC: cs35l41: Add support for
hibernate memory retention mode")'

Add the missing function calls to this driver.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
Signed-off-by: Lucas Tanure <tanureal@opensource.cirrus.com>
---
 sound/pci/hda/cs35l41_hda.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/sound/pci/hda/cs35l41_hda.c b/sound/pci/hda/cs35l41_hda.c
index c47c5f0b4e59..509a380f9be7 100644
--- a/sound/pci/hda/cs35l41_hda.c
+++ b/sound/pci/hda/cs35l41_hda.c
@@ -463,6 +463,10 @@ int cs35l41_hda_probe(struct device *dev, const char *device_name, int id, int i
 		goto err;
 	}
 
+	ret = cs35l41_test_key_unlock(cs35l41->dev, cs35l41->regmap);
+	if (ret)
+		goto err;
+
 	ret = cs35l41_register_errata_patch(cs35l41->dev, cs35l41->regmap, reg_revid);
 	if (ret)
 		goto err;
@@ -473,6 +477,10 @@ int cs35l41_hda_probe(struct device *dev, const char *device_name, int id, int i
 		goto err;
 	}
 
+	ret = cs35l41_test_key_lock(cs35l41->dev, cs35l41->regmap);
+	if (ret)
+		goto err;
+
 	ret = cs35l41_hda_apply_properties(cs35l41, acpi_hw_cfg);
 	if (ret)
 		goto err;
-- 
2.34.1

Re: [PATCH 2/5] ALSA: hda: cs35l41: Add calls to newly added test key function

[Cezary Rojewski]

On 2022-01-13 6:07 PM, Lucas Tanure wrote:
> From: Charles Keepax <ckeepax@opensource.cirrus.com>
> 
> The test key now needs to be manually held when calling
> cs35l41_register_errata_patch, after patch:
> 
> 'commit f517ba4924ad ("ASoC: cs35l41: Add support for
> hibernate memory retention mode")'
> 
> Add the missing function calls to this driver.
> 
> Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> Signed-off-by: Lucas Tanure <tanureal@opensource.cirrus.com>

Judging by the commit's message, this patch is a fix for a previously 
added commit so appropriate 'Fixes' tag could prove helpful here.

>   sound/pci/hda/cs35l41_hda.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/sound/pci/hda/cs35l41_hda.c b/sound/pci/hda/cs35l41_hda.c
> index c47c5f0b4e59..509a380f9be7 100644
> --- a/sound/pci/hda/cs35l41_hda.c
> +++ b/sound/pci/hda/cs35l41_hda.c
> @@ -463,6 +463,10 @@ int cs35l41_hda_probe(struct device *dev, const char *device_name, int id, int i
>   		goto err;
>   	}
>   
> +	ret = cs35l41_test_key_unlock(cs35l41->dev, cs35l41->regmap);
> +	if (ret)
> +		goto err;
> +
>   	ret = cs35l41_register_errata_patch(cs35l41->dev, cs35l41->regmap, reg_revid);
>   	if (ret)
>   		goto err;
> @@ -473,6 +477,10 @@ int cs35l41_hda_probe(struct device *dev, const char *device_name, int id, int i
>   		goto err;
>   	}
>   
> +	ret = cs35l41_test_key_lock(cs35l41->dev, cs35l41->regmap);
> +	if (ret)
> +		goto err;
> +
>   	ret = cs35l41_hda_apply_properties(cs35l41, acpi_hw_cfg);
>   	if (ret)
>   		goto err;
> 

[PATCH 3/5] ALSA: hda: cs35l41: Move cs35l41* calls to its own symbol namespace

[Lucas Tanure]

Create own namespace and avoid polluting the global namespace

Signed-off-by: Lucas Tanure <tanureal@opensource.cirrus.com>
---
 sound/pci/hda/cs35l41_hda.c     | 5 ++---
 sound/pci/hda/cs35l41_hda_i2c.c | 1 +
 sound/pci/hda/cs35l41_hda_spi.c | 1 +
 3 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/sound/pci/hda/cs35l41_hda.c b/sound/pci/hda/cs35l41_hda.c
index 509a380f9be7..c4f25e48dcc0 100644
--- a/sound/pci/hda/cs35l41_hda.c
+++ b/sound/pci/hda/cs35l41_hda.c
@@ -514,7 +514,7 @@ int cs35l41_hda_probe(struct device *dev, const char *device_name, int id, int i
 
 	return ret;
 }
-EXPORT_SYMBOL_GPL(cs35l41_hda_probe);
+EXPORT_SYMBOL_NS_GPL(cs35l41_hda_probe, SND_HDA_SCODEC_CS35L41);
 
 int cs35l41_hda_remove(struct device *dev)
 {
@@ -528,8 +528,7 @@ int cs35l41_hda_remove(struct device *dev)
 
 	return 0;
 }
-EXPORT_SYMBOL_GPL(cs35l41_hda_remove);
-
+EXPORT_SYMBOL_NS_GPL(cs35l41_hda_remove, SND_HDA_SCODEC_CS35L41);
 
 MODULE_DESCRIPTION("CS35L41 HDA Driver");
 MODULE_AUTHOR("Lucas Tanure, Cirrus Logic Inc, <tanureal@opensource.cirrus.com>");
diff --git a/sound/pci/hda/cs35l41_hda_i2c.c b/sound/pci/hda/cs35l41_hda_i2c.c
index 4a9462fb5c14..eeb387853ee3 100644
--- a/sound/pci/hda/cs35l41_hda_i2c.c
+++ b/sound/pci/hda/cs35l41_hda_i2c.c
@@ -62,5 +62,6 @@ static struct i2c_driver cs35l41_i2c_driver = {
 module_i2c_driver(cs35l41_i2c_driver);
 
 MODULE_DESCRIPTION("HDA CS35L41 driver");
+MODULE_IMPORT_NS(SND_HDA_SCODEC_CS35L41);
 MODULE_AUTHOR("Lucas Tanure <tanureal@opensource.cirrus.com>");
 MODULE_LICENSE("GPL");
diff --git a/sound/pci/hda/cs35l41_hda_spi.c b/sound/pci/hda/cs35l41_hda_spi.c
index 77426e96c58f..15345a72b9d1 100644
--- a/sound/pci/hda/cs35l41_hda_spi.c
+++ b/sound/pci/hda/cs35l41_hda_spi.c
@@ -59,5 +59,6 @@ static struct spi_driver cs35l41_spi_driver = {
 module_spi_driver(cs35l41_spi_driver);
 
 MODULE_DESCRIPTION("HDA CS35L41 driver");
+MODULE_IMPORT_NS(SND_HDA_SCODEC_CS35L41);
 MODULE_AUTHOR("Lucas Tanure <tanureal@opensource.cirrus.com>");
 MODULE_LICENSE("GPL");
-- 
2.34.1

[PATCH 4/5] ALSA: hda: cs35l41: Tidyup code and add default cases

[Lucas Tanure]

Clean up the code, plus adding default cases for switch
and dev_err_probe use.

Signed-off-by: Lucas Tanure <tanureal@opensource.cirrus.com>
---
 sound/pci/hda/cs35l41_hda.c     | 109 ++++++++++++++++----------------
 sound/pci/hda/cs35l41_hda.h     |   2 +-
 sound/pci/hda/cs35l41_hda_i2c.c |   1 -
 sound/pci/hda/cs35l41_hda_spi.c |   1 -
 4 files changed, 57 insertions(+), 56 deletions(-)

diff --git a/sound/pci/hda/cs35l41_hda.c b/sound/pci/hda/cs35l41_hda.c
index c4f25e48dcc0..cb8331587c17 100644
--- a/sound/pci/hda/cs35l41_hda.c
+++ b/sound/pci/hda/cs35l41_hda.c
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 //
-// cs35l41.c -- CS35l41 ALSA HDA audio driver
+// CS35l41 ALSA HDA audio driver
 //
 // Copyright 2021 Cirrus Logic, Inc.
 //
@@ -17,19 +17,19 @@
 #include "cs35l41_hda.h"
 
 static const struct reg_sequence cs35l41_hda_config[] = {
-	{ CS35L41_PLL_CLK_CTRL,		0x00000430 }, //3200000Hz, BCLK Input, PLL_REFCLK_EN = 1
-	{ CS35L41_GLOBAL_CLK_CTRL,	0x00000003 }, //GLOBAL_FS = 48 kHz
-	{ CS35L41_SP_ENABLES,		0x00010000 }, //ASP_RX1_EN = 1
-	{ CS35L41_SP_RATE_CTRL,		0x00000021 }, //ASP_BCLK_FREQ = 3.072 MHz
-	{ CS35L41_SP_FORMAT,		0x20200200 }, //24 bits, I2S, BCLK Slave, FSYNC Slave
-	{ CS35L41_DAC_PCM1_SRC,		0x00000008 }, //DACPCM1_SRC = ASPRX1
-	{ CS35L41_AMP_DIG_VOL_CTRL,	0x00000000 }, //AMP_VOL_PCM  0.0 dB
-	{ CS35L41_AMP_GAIN_CTRL,	0x00000084 }, //AMP_GAIN_PCM 4.5 dB
-	{ CS35L41_PWR_CTRL2,		0x00000001 }, //AMP_EN = 1
+	{ CS35L41_PLL_CLK_CTRL,		0x00000430 }, // 3200000Hz, BCLK Input, PLL_REFCLK_EN = 1
+	{ CS35L41_GLOBAL_CLK_CTRL,	0x00000003 }, // GLOBAL_FS = 48 kHz
+	{ CS35L41_SP_ENABLES,		0x00010000 }, // ASP_RX1_EN = 1
+	{ CS35L41_SP_RATE_CTRL,		0x00000021 }, // ASP_BCLK_FREQ = 3.072 MHz
+	{ CS35L41_SP_FORMAT,		0x20200200 }, // 24 bits, I2S, BCLK Slave, FSYNC Slave
+	{ CS35L41_DAC_PCM1_SRC,		0x00000008 }, // DACPCM1_SRC = ASPRX1
+	{ CS35L41_AMP_DIG_VOL_CTRL,	0x00000000 }, // AMP_VOL_PCM  0.0 dB
+	{ CS35L41_AMP_GAIN_CTRL,	0x00000084 }, // AMP_GAIN_PCM 4.5 dB
+	{ CS35L41_PWR_CTRL2,		0x00000001 }, // AMP_EN = 1
 };
 
 static const struct reg_sequence cs35l41_hda_start_bst[] = {
-	{ CS35L41_PWR_CTRL2,		0x00000021 }, //BST_EN = 10, AMP_EN = 1
+	{ CS35L41_PWR_CTRL2,		0x00000021 }, // BST_EN = 10, AMP_EN = 1
 	{ CS35L41_PWR_CTRL1,		0x00000001, 3000}, // set GLOBAL_EN = 1
 };
 
@@ -60,7 +60,7 @@ static const struct reg_sequence cs35l41_stop_ext_vspk[] = {
 	{ 0x00000040,			0x00000055 },
 	{ 0x00000040,			0x000000AA },
 	{ 0x00007438,			0x00585941 },
-	{ 0x00002014,			0x00000000, 3000}, //set GLOBAL_EN = 0
+	{ 0x00002014,			0x00000000, 3000}, // set GLOBAL_EN = 0
 	{ 0x0000742C,			0x00000009 },
 	{ 0x00007438,			0x00580941 },
 	{ 0x00011008,			0x00000001 },
@@ -78,7 +78,7 @@ static const struct reg_sequence cs35l41_safe_to_active[] = {
 	{ 0x0000742C,			0x0000000F },
 	{ 0x0000742C,			0x00000079 },
 	{ 0x00007438,			0x00585941 },
-	{ CS35L41_PWR_CTRL1,		0x00000001, 2000 }, //GLOBAL_EN = 1
+	{ CS35L41_PWR_CTRL1,		0x00000001, 2000 }, // GLOBAL_EN = 1
 	{ 0x0000742C,			0x000000F9 },
 	{ 0x00007438,			0x00580941 },
 	{ 0x00000040,			0x000000CC },
@@ -89,8 +89,8 @@ static const struct reg_sequence cs35l41_active_to_safe[] = {
 	{ 0x00000040,			0x00000055 },
 	{ 0x00000040,			0x000000AA },
 	{ 0x00007438,			0x00585941 },
-	{ CS35L41_AMP_DIG_VOL_CTRL,	0x0000A678 }, //AMP_VOL_PCM Mute
-	{ CS35L41_PWR_CTRL2,		0x00000000 }, //AMP_EN = 0
+	{ CS35L41_AMP_DIG_VOL_CTRL,	0x0000A678 }, // AMP_VOL_PCM Mute
+	{ CS35L41_PWR_CTRL2,		0x00000000 }, // AMP_EN = 0
 	{ CS35L41_PWR_CTRL1,		0x00000000 },
 	{ 0x0000742C,			0x00000009, 2000 },
 	{ 0x00007438,			0x00580941 },
@@ -161,11 +161,13 @@ static void cs35l41_hda_playback_hook(struct device *dev, int action)
 		if (reg_seq->close)
 			ret = regmap_multi_reg_write(reg, reg_seq->close, reg_seq->num_close);
 		break;
+	default:
+		ret = -EINVAL;
+		break;
 	}
 
 	if (ret)
 		dev_warn(cs35l41->dev, "Failed to apply multi reg write: %d\n", ret);
-
 }
 
 static int cs35l41_hda_channel_map(struct device *dev, unsigned int tx_num, unsigned int *tx_slot,
@@ -182,20 +184,19 @@ static int cs35l41_hda_bind(struct device *dev, struct device *master, void *mas
 	struct cs35l41_hda *cs35l41 = dev_get_drvdata(dev);
 	struct hda_component *comps = master_data;
 
-	if (comps && cs35l41->index >= 0 && cs35l41->index < HDA_MAX_COMPONENTS)
-		comps = &comps[cs35l41->index];
-	else
+	if (!comps || cs35l41->index < 0 || cs35l41->index >= HDA_MAX_COMPONENTS)
 		return -EINVAL;
 
-	if (!comps->dev) {
-		comps->dev = dev;
-		strscpy(comps->name, dev_name(dev), sizeof(comps->name));
-		comps->playback_hook = cs35l41_hda_playback_hook;
-		comps->set_channel_map = cs35l41_hda_channel_map;
-		return 0;
-	}
+	comps = &comps[cs35l41->index];
+	if (comps->dev)
+		return -EBUSY;
+
+	comps->dev = dev;
+	strscpy(comps->name, dev_name(dev), sizeof(comps->name));
+	comps->playback_hook = cs35l41_hda_playback_hook;
+	comps->set_channel_map = cs35l41_hda_channel_map;
 
-	return -EBUSY;
+	return 0;
 }
 
 static void cs35l41_hda_unbind(struct device *dev, struct device *master, void *master_data)
@@ -235,6 +236,8 @@ static int cs35l41_hda_apply_properties(struct cs35l41_hda *cs35l41,
 		regmap_update_bits(cs35l41->regmap, CS35L41_GPIO_PAD_CONTROL,
 				   CS35L41_GPIO1_CTRL_MASK, 2 << CS35L41_GPIO1_CTRL_SHIFT);
 		break;
+	default:
+		return -EINVAL;
 	}
 
 	switch (hw_cfg->gpio2_func) {
@@ -242,6 +245,8 @@ static int cs35l41_hda_apply_properties(struct cs35l41_hda *cs35l41,
 		regmap_update_bits(cs35l41->regmap, CS35L41_GPIO_PAD_CONTROL,
 				   CS35L41_GPIO2_CTRL_MASK, 2 << CS35L41_GPIO2_CTRL_SHIFT);
 		break;
+	default:
+		return -EINVAL;
 	}
 
 	if (internal_boost) {
@@ -256,11 +261,7 @@ static int cs35l41_hda_apply_properties(struct cs35l41_hda *cs35l41,
 		cs35l41->reg_seq = &cs35l41_hda_reg_seq_ext_bst;
 	}
 
-	ret = cs35l41_hda_channel_map(cs35l41->dev, 0, NULL, 1, (unsigned int *)&hw_cfg->spk_pos);
-	if (ret)
-		return ret;
-
-	return 0;
+	return cs35l41_hda_channel_map(cs35l41->dev, 0, NULL, 1, (unsigned int *)&hw_cfg->spk_pos);
 }
 
 static struct cs35l41_hda_hw_config *cs35l41_hda_read_acpi(struct cs35l41_hda *cs35l41,
@@ -269,7 +270,7 @@ static struct cs35l41_hda_hw_config *cs35l41_hda_read_acpi(struct cs35l41_hda *c
 	struct cs35l41_hda_hw_config *hw_cfg;
 	u32 values[HDA_MAX_COMPONENTS];
 	struct acpi_device *adev;
-	struct device *acpi_dev;
+	struct device *physdev;
 	char *property;
 	size_t nval;
 	int i, ret;
@@ -280,11 +281,11 @@ static struct cs35l41_hda_hw_config *cs35l41_hda_read_acpi(struct cs35l41_hda *c
 		return ERR_PTR(-ENODEV);
 	}
 
-	acpi_dev = get_device(acpi_get_first_physical_node(adev));
+	physdev = get_device(acpi_get_first_physical_node(adev));
 	acpi_dev_put(adev);
 
 	property = "cirrus,dev-index";
-	ret = device_property_count_u32(acpi_dev, property);
+	ret = device_property_count_u32(physdev, property);
 	if (ret <= 0)
 		goto no_acpi_dsd;
 
@@ -294,7 +295,7 @@ static struct cs35l41_hda_hw_config *cs35l41_hda_read_acpi(struct cs35l41_hda *c
 	}
 	nval = ret;
 
-	ret = device_property_read_u32_array(acpi_dev, property, values, nval);
+	ret = device_property_read_u32_array(physdev, property, values, nval);
 	if (ret)
 		goto err;
 
@@ -311,7 +312,9 @@ static struct cs35l41_hda_hw_config *cs35l41_hda_read_acpi(struct cs35l41_hda *c
 		goto err;
 	}
 
-	/* No devm_ version as CLSA0100, in no_acpi_dsd case, can't use devm version */
+	/* To use the same release code for all laptop variants we can't use devm_ version of
+	 * gpiod_get here, as CLSA010* don't have a fully functional bios with an _DSD node
+	 */
 	cs35l41->reset_gpio = fwnode_gpiod_get_index(&adev->fwnode, "reset", cs35l41->index,
 						     GPIOD_OUT_LOW, "cs35l41-reset");
 
@@ -322,46 +325,46 @@ static struct cs35l41_hda_hw_config *cs35l41_hda_read_acpi(struct cs35l41_hda *c
 	}
 
 	property = "cirrus,speaker-position";
-	ret = device_property_read_u32_array(acpi_dev, property, values, nval);
+	ret = device_property_read_u32_array(physdev, property, values, nval);
 	if (ret)
 		goto err_free;
 	hw_cfg->spk_pos = values[cs35l41->index];
 
 	property = "cirrus,gpio1-func";
-	ret = device_property_read_u32_array(acpi_dev, property, values, nval);
+	ret = device_property_read_u32_array(physdev, property, values, nval);
 	if (ret)
 		goto err_free;
 	hw_cfg->gpio1_func = values[cs35l41->index];
 
 	property = "cirrus,gpio2-func";
-	ret = device_property_read_u32_array(acpi_dev, property, values, nval);
+	ret = device_property_read_u32_array(physdev, property, values, nval);
 	if (ret)
 		goto err_free;
 	hw_cfg->gpio2_func = values[cs35l41->index];
 
 	property = "cirrus,boost-peak-milliamp";
-	ret = device_property_read_u32_array(acpi_dev, property, values, nval);
+	ret = device_property_read_u32_array(physdev, property, values, nval);
 	if (ret == 0)
 		hw_cfg->bst_ipk = values[cs35l41->index];
 
 	property = "cirrus,boost-ind-nanohenry";
-	ret = device_property_read_u32_array(acpi_dev, property, values, nval);
+	ret = device_property_read_u32_array(physdev, property, values, nval);
 	if (ret == 0)
 		hw_cfg->bst_ind = values[cs35l41->index];
 
 	property = "cirrus,boost-cap-microfarad";
-	ret = device_property_read_u32_array(acpi_dev, property, values, nval);
+	ret = device_property_read_u32_array(physdev, property, values, nval);
 	if (ret == 0)
 		hw_cfg->bst_cap = values[cs35l41->index];
 
-	put_device(acpi_dev);
+	put_device(physdev);
 
 	return hw_cfg;
 
 err_free:
 	kfree(hw_cfg);
 err:
-	put_device(acpi_dev);
+	put_device(physdev);
 	dev_err(cs35l41->dev, "Failed property %s: %d\n", property, ret);
 
 	return ERR_PTR(ret);
@@ -370,18 +373,18 @@ static struct cs35l41_hda_hw_config *cs35l41_hda_read_acpi(struct cs35l41_hda *c
 	/*
 	 * Device CLSA0100 doesn't have _DSD so a gpiod_get by the label reset won't work.
 	 * And devices created by i2c-multi-instantiate don't have their device struct pointing to
-	 * the correct fwnode, so acpi_dev must be used here
+	 * the correct fwnode, so acpi_dev must be used here.
 	 * And devm functions expect that the device requesting the resource has the correct
-	 * fwnode
+	 * fwnode.
 	 */
 	if (strncmp(hid, "CLSA0100", 8) != 0)
 		return ERR_PTR(-EINVAL);
 
 	/* check I2C address to assign the index */
 	cs35l41->index = id == 0x40 ? 0 : 1;
-	cs35l41->reset_gpio = gpiod_get_index(acpi_dev, NULL, 0, GPIOD_OUT_HIGH);
+	cs35l41->reset_gpio = gpiod_get_index(physdev, NULL, 0, GPIOD_OUT_HIGH);
 	cs35l41->vspk_always_on = true;
-	put_device(acpi_dev);
+	put_device(physdev);
 
 	return NULL;
 }
@@ -416,8 +419,7 @@ int cs35l41_hda_probe(struct device *dev, const char *device_name, int id, int i
 		if (ret == -EBUSY) {
 			dev_info(cs35l41->dev, "Reset line busy, assuming shared reset\n");
 		} else {
-			if (ret != -EPROBE_DEFER)
-				dev_err(cs35l41->dev, "Failed to get reset GPIO: %d\n", ret);
+			dev_err_probe(cs35l41->dev, ret, "Failed to get reset GPIO: %d\n", ret);
 			goto err;
 		}
 	}
@@ -437,7 +439,8 @@ int cs35l41_hda_probe(struct device *dev, const char *device_name, int id, int i
 
 	ret = regmap_read(cs35l41->regmap, CS35L41_IRQ1_STATUS3, &int_sts);
 	if (ret || (int_sts & CS35L41_OTP_BOOT_ERR)) {
-		dev_err(cs35l41->dev, "OTP Boot error\n");
+		dev_err(cs35l41->dev, "OTP Boot status %x error: %d\n",
+			int_sts & CS35L41_OTP_BOOT_ERR, ret);
 		ret = -EIO;
 		goto err;
 	}
@@ -489,7 +492,7 @@ int cs35l41_hda_probe(struct device *dev, const char *device_name, int id, int i
 
 	if (cs35l41->reg_seq->probe) {
 		ret = regmap_multi_reg_write(cs35l41->regmap, cs35l41->reg_seq->probe,
-					    cs35l41->reg_seq->num_probe);
+					     cs35l41->reg_seq->num_probe);
 		if (ret) {
 			dev_err(cs35l41->dev, "Fail to apply probe reg patch: %d\n", ret);
 			goto err;
diff --git a/sound/pci/hda/cs35l41_hda.h b/sound/pci/hda/cs35l41_hda.h
index 76c69a8a22f6..640afc98b686 100644
--- a/sound/pci/hda/cs35l41_hda.h
+++ b/sound/pci/hda/cs35l41_hda.h
@@ -1,6 +1,6 @@
 /* SPDX-License-Identifier: GPL-2.0
  *
- * cs35l41_hda.h -- CS35L41 ALSA HDA audio driver
+ * CS35L41 ALSA HDA audio driver
  *
  * Copyright 2021 Cirrus Logic, Inc.
  *
diff --git a/sound/pci/hda/cs35l41_hda_i2c.c b/sound/pci/hda/cs35l41_hda_i2c.c
index eeb387853ee3..c2397dc53e78 100644
--- a/sound/pci/hda/cs35l41_hda_i2c.c
+++ b/sound/pci/hda/cs35l41_hda_i2c.c
@@ -58,7 +58,6 @@ static struct i2c_driver cs35l41_i2c_driver = {
 	.probe		= cs35l41_hda_i2c_probe,
 	.remove		= cs35l41_hda_i2c_remove,
 };
-
 module_i2c_driver(cs35l41_i2c_driver);
 
 MODULE_DESCRIPTION("HDA CS35L41 driver");
diff --git a/sound/pci/hda/cs35l41_hda_spi.c b/sound/pci/hda/cs35l41_hda_spi.c
index 15345a72b9d1..36815ab4e461 100644
--- a/sound/pci/hda/cs35l41_hda_spi.c
+++ b/sound/pci/hda/cs35l41_hda_spi.c
@@ -55,7 +55,6 @@ static struct spi_driver cs35l41_spi_driver = {
 	.probe		= cs35l41_hda_spi_probe,
 	.remove		= cs35l41_hda_spi_remove,
 };
-
 module_spi_driver(cs35l41_spi_driver);
 
 MODULE_DESCRIPTION("HDA CS35L41 driver");
-- 
2.34.1

Re: [PATCH 4/5] ALSA: hda: cs35l41: Tidyup code and add default cases

[Lucas tanure]

On 1/13/22 17:07, Lucas Tanure wrote:
> Clean up the code, plus adding default cases for switch
> and dev_err_probe use.
> 
> Signed-off-by: Lucas Tanure <tanureal@opensource.cirrus.com>
> ---
>   sound/pci/hda/cs35l41_hda.c     | 109 ++++++++++++++++----------------
>   sound/pci/hda/cs35l41_hda.h     |   2 +-
>   sound/pci/hda/cs35l41_hda_i2c.c |   1 -
>   sound/pci/hda/cs35l41_hda_spi.c |   1 -
>   4 files changed, 57 insertions(+), 56 deletions(-)
> 
> diff --git a/sound/pci/hda/cs35l41_hda.c b/sound/pci/hda/cs35l41_hda.c
> index c4f25e48dcc0..cb8331587c17 100644
> --- a/sound/pci/hda/cs35l41_hda.c
> +++ b/sound/pci/hda/cs35l41_hda.c
> @@ -1,6 +1,6 @@
>   // SPDX-License-Identifier: GPL-2.0
>   //
> -// cs35l41.c -- CS35l41 ALSA HDA audio driver
> +// CS35l41 ALSA HDA audio driver
>   //
>   // Copyright 2021 Cirrus Logic, Inc.
>   //
> @@ -17,19 +17,19 @@
>   #include "cs35l41_hda.h"
>   
>   static const struct reg_sequence cs35l41_hda_config[] = {
> -	{ CS35L41_PLL_CLK_CTRL,		0x00000430 }, //3200000Hz, BCLK Input, PLL_REFCLK_EN = 1
> -	{ CS35L41_GLOBAL_CLK_CTRL,	0x00000003 }, //GLOBAL_FS = 48 kHz
> -	{ CS35L41_SP_ENABLES,		0x00010000 }, //ASP_RX1_EN = 1
> -	{ CS35L41_SP_RATE_CTRL,		0x00000021 }, //ASP_BCLK_FREQ = 3.072 MHz
> -	{ CS35L41_SP_FORMAT,		0x20200200 }, //24 bits, I2S, BCLK Slave, FSYNC Slave
> -	{ CS35L41_DAC_PCM1_SRC,		0x00000008 }, //DACPCM1_SRC = ASPRX1
> -	{ CS35L41_AMP_DIG_VOL_CTRL,	0x00000000 }, //AMP_VOL_PCM  0.0 dB
> -	{ CS35L41_AMP_GAIN_CTRL,	0x00000084 }, //AMP_GAIN_PCM 4.5 dB
> -	{ CS35L41_PWR_CTRL2,		0x00000001 }, //AMP_EN = 1
> +	{ CS35L41_PLL_CLK_CTRL,		0x00000430 }, // 3200000Hz, BCLK Input, PLL_REFCLK_EN = 1
> +	{ CS35L41_GLOBAL_CLK_CTRL,	0x00000003 }, // GLOBAL_FS = 48 kHz
> +	{ CS35L41_SP_ENABLES,		0x00010000 }, // ASP_RX1_EN = 1
> +	{ CS35L41_SP_RATE_CTRL,		0x00000021 }, // ASP_BCLK_FREQ = 3.072 MHz
> +	{ CS35L41_SP_FORMAT,		0x20200200 }, // 24 bits, I2S, BCLK Slave, FSYNC Slave
> +	{ CS35L41_DAC_PCM1_SRC,		0x00000008 }, // DACPCM1_SRC = ASPRX1
> +	{ CS35L41_AMP_DIG_VOL_CTRL,	0x00000000 }, // AMP_VOL_PCM  0.0 dB
> +	{ CS35L41_AMP_GAIN_CTRL,	0x00000084 }, // AMP_GAIN_PCM 4.5 dB
> +	{ CS35L41_PWR_CTRL2,		0x00000001 }, // AMP_EN = 1
>   };
>   
>   static const struct reg_sequence cs35l41_hda_start_bst[] = {
> -	{ CS35L41_PWR_CTRL2,		0x00000021 }, //BST_EN = 10, AMP_EN = 1
> +	{ CS35L41_PWR_CTRL2,		0x00000021 }, // BST_EN = 10, AMP_EN = 1
>   	{ CS35L41_PWR_CTRL1,		0x00000001, 3000}, // set GLOBAL_EN = 1
>   };
>   
> @@ -60,7 +60,7 @@ static const struct reg_sequence cs35l41_stop_ext_vspk[] = {
>   	{ 0x00000040,			0x00000055 },
>   	{ 0x00000040,			0x000000AA },
>   	{ 0x00007438,			0x00585941 },
> -	{ 0x00002014,			0x00000000, 3000}, //set GLOBAL_EN = 0
> +	{ 0x00002014,			0x00000000, 3000}, // set GLOBAL_EN = 0
>   	{ 0x0000742C,			0x00000009 },
>   	{ 0x00007438,			0x00580941 },
>   	{ 0x00011008,			0x00000001 },
> @@ -78,7 +78,7 @@ static const struct reg_sequence cs35l41_safe_to_active[] = {
>   	{ 0x0000742C,			0x0000000F },
>   	{ 0x0000742C,			0x00000079 },
>   	{ 0x00007438,			0x00585941 },
> -	{ CS35L41_PWR_CTRL1,		0x00000001, 2000 }, //GLOBAL_EN = 1
> +	{ CS35L41_PWR_CTRL1,		0x00000001, 2000 }, // GLOBAL_EN = 1
>   	{ 0x0000742C,			0x000000F9 },
>   	{ 0x00007438,			0x00580941 },
>   	{ 0x00000040,			0x000000CC },
> @@ -89,8 +89,8 @@ static const struct reg_sequence cs35l41_active_to_safe[] = {
>   	{ 0x00000040,			0x00000055 },
>   	{ 0x00000040,			0x000000AA },
>   	{ 0x00007438,			0x00585941 },
> -	{ CS35L41_AMP_DIG_VOL_CTRL,	0x0000A678 }, //AMP_VOL_PCM Mute
> -	{ CS35L41_PWR_CTRL2,		0x00000000 }, //AMP_EN = 0
> +	{ CS35L41_AMP_DIG_VOL_CTRL,	0x0000A678 }, // AMP_VOL_PCM Mute
> +	{ CS35L41_PWR_CTRL2,		0x00000000 }, // AMP_EN = 0
>   	{ CS35L41_PWR_CTRL1,		0x00000000 },
>   	{ 0x0000742C,			0x00000009, 2000 },
>   	{ 0x00007438,			0x00580941 },
> @@ -161,11 +161,13 @@ static void cs35l41_hda_playback_hook(struct device *dev, int action)
>   		if (reg_seq->close)
>   			ret = regmap_multi_reg_write(reg, reg_seq->close, reg_seq->num_close);
>   		break;
> +	default:
> +		ret = -EINVAL;
> +		break;
>   	}
>   
>   	if (ret)
>   		dev_warn(cs35l41->dev, "Failed to apply multi reg write: %d\n", ret);
> -
>   }
>   
>   static int cs35l41_hda_channel_map(struct device *dev, unsigned int tx_num, unsigned int *tx_slot,
> @@ -182,20 +184,19 @@ static int cs35l41_hda_bind(struct device *dev, struct device *master, void *mas
>   	struct cs35l41_hda *cs35l41 = dev_get_drvdata(dev);
>   	struct hda_component *comps = master_data;
>   
> -	if (comps && cs35l41->index >= 0 && cs35l41->index < HDA_MAX_COMPONENTS)
> -		comps = &comps[cs35l41->index];
> -	else
> +	if (!comps || cs35l41->index < 0 || cs35l41->index >= HDA_MAX_COMPONENTS)
>   		return -EINVAL;
>   
> -	if (!comps->dev) {
> -		comps->dev = dev;
> -		strscpy(comps->name, dev_name(dev), sizeof(comps->name));
> -		comps->playback_hook = cs35l41_hda_playback_hook;
> -		comps->set_channel_map = cs35l41_hda_channel_map;
> -		return 0;
> -	}
> +	comps = &comps[cs35l41->index];
> +	if (comps->dev)
> +		return -EBUSY;
> +
> +	comps->dev = dev;
> +	strscpy(comps->name, dev_name(dev), sizeof(comps->name));
> +	comps->playback_hook = cs35l41_hda_playback_hook;
> +	comps->set_channel_map = cs35l41_hda_channel_map;
>   
> -	return -EBUSY;
> +	return 0;
>   }
>   
>   static void cs35l41_hda_unbind(struct device *dev, struct device *master, void *master_data)
> @@ -235,6 +236,8 @@ static int cs35l41_hda_apply_properties(struct cs35l41_hda *cs35l41,
>   		regmap_update_bits(cs35l41->regmap, CS35L41_GPIO_PAD_CONTROL,
>   				   CS35L41_GPIO1_CTRL_MASK, 2 << CS35L41_GPIO1_CTRL_SHIFT);
>   		break;
> +	default:
> +		return -EINVAL;
>   	}
>   
>   	switch (hw_cfg->gpio2_func) {
> @@ -242,6 +245,8 @@ static int cs35l41_hda_apply_properties(struct cs35l41_hda *cs35l41,
>   		regmap_update_bits(cs35l41->regmap, CS35L41_GPIO_PAD_CONTROL,
>   				   CS35L41_GPIO2_CTRL_MASK, 2 << CS35L41_GPIO2_CTRL_SHIFT);
>   		break;
> +	default:
> +		return -EINVAL;
This default option introduces issues some laptops.
A new version of this series will be sent.

>   	}
>   
>   	if (internal_boost) {
> @@ -256,11 +261,7 @@ static int cs35l41_hda_apply_properties(struct cs35l41_hda *cs35l41,
>   		cs35l41->reg_seq = &cs35l41_hda_reg_seq_ext_bst;
>   	}
>   
> -	ret = cs35l41_hda_channel_map(cs35l41->dev, 0, NULL, 1, (unsigned int *)&hw_cfg->spk_pos);
> -	if (ret)
> -		return ret;
> -
> -	return 0;
> +	return cs35l41_hda_channel_map(cs35l41->dev, 0, NULL, 1, (unsigned int *)&hw_cfg->spk_pos);
>   }
>   
>   static struct cs35l41_hda_hw_config *cs35l41_hda_read_acpi(struct cs35l41_hda *cs35l41,
> @@ -269,7 +270,7 @@ static struct cs35l41_hda_hw_config *cs35l41_hda_read_acpi(struct cs35l41_hda *c
>   	struct cs35l41_hda_hw_config *hw_cfg;
>   	u32 values[HDA_MAX_COMPONENTS];
>   	struct acpi_device *adev;
> -	struct device *acpi_dev;
> +	struct device *physdev;
>   	char *property;
>   	size_t nval;
>   	int i, ret;
> @@ -280,11 +281,11 @@ static struct cs35l41_hda_hw_config *cs35l41_hda_read_acpi(struct cs35l41_hda *c
>   		return ERR_PTR(-ENODEV);
>   	}
>   
> -	acpi_dev = get_device(acpi_get_first_physical_node(adev));
> +	physdev = get_device(acpi_get_first_physical_node(adev));
>   	acpi_dev_put(adev);
>   
>   	property = "cirrus,dev-index";
> -	ret = device_property_count_u32(acpi_dev, property);
> +	ret = device_property_count_u32(physdev, property);
>   	if (ret <= 0)
>   		goto no_acpi_dsd;
>   
> @@ -294,7 +295,7 @@ static struct cs35l41_hda_hw_config *cs35l41_hda_read_acpi(struct cs35l41_hda *c
>   	}
>   	nval = ret;
>   
> -	ret = device_property_read_u32_array(acpi_dev, property, values, nval);
> +	ret = device_property_read_u32_array(physdev, property, values, nval);
>   	if (ret)
>   		goto err;
>   
> @@ -311,7 +312,9 @@ static struct cs35l41_hda_hw_config *cs35l41_hda_read_acpi(struct cs35l41_hda *c
>   		goto err;
>   	}
>   
> -	/* No devm_ version as CLSA0100, in no_acpi_dsd case, can't use devm version */
> +	/* To use the same release code for all laptop variants we can't use devm_ version of
> +	 * gpiod_get here, as CLSA010* don't have a fully functional bios with an _DSD node
> +	 */
>   	cs35l41->reset_gpio = fwnode_gpiod_get_index(&adev->fwnode, "reset", cs35l41->index,
>   						     GPIOD_OUT_LOW, "cs35l41-reset");
>   
> @@ -322,46 +325,46 @@ static struct cs35l41_hda_hw_config *cs35l41_hda_read_acpi(struct cs35l41_hda *c
>   	}
>   
>   	property = "cirrus,speaker-position";
> -	ret = device_property_read_u32_array(acpi_dev, property, values, nval);
> +	ret = device_property_read_u32_array(physdev, property, values, nval);
>   	if (ret)
>   		goto err_free;
>   	hw_cfg->spk_pos = values[cs35l41->index];
>   
>   	property = "cirrus,gpio1-func";
> -	ret = device_property_read_u32_array(acpi_dev, property, values, nval);
> +	ret = device_property_read_u32_array(physdev, property, values, nval);
>   	if (ret)
>   		goto err_free;
>   	hw_cfg->gpio1_func = values[cs35l41->index];
>   
>   	property = "cirrus,gpio2-func";
> -	ret = device_property_read_u32_array(acpi_dev, property, values, nval);
> +	ret = device_property_read_u32_array(physdev, property, values, nval);
>   	if (ret)
>   		goto err_free;
>   	hw_cfg->gpio2_func = values[cs35l41->index];
>   
>   	property = "cirrus,boost-peak-milliamp";
> -	ret = device_property_read_u32_array(acpi_dev, property, values, nval);
> +	ret = device_property_read_u32_array(physdev, property, values, nval);
>   	if (ret == 0)
>   		hw_cfg->bst_ipk = values[cs35l41->index];
>   
>   	property = "cirrus,boost-ind-nanohenry";
> -	ret = device_property_read_u32_array(acpi_dev, property, values, nval);
> +	ret = device_property_read_u32_array(physdev, property, values, nval);
>   	if (ret == 0)
>   		hw_cfg->bst_ind = values[cs35l41->index];
>   
>   	property = "cirrus,boost-cap-microfarad";
> -	ret = device_property_read_u32_array(acpi_dev, property, values, nval);
> +	ret = device_property_read_u32_array(physdev, property, values, nval);
>   	if (ret == 0)
>   		hw_cfg->bst_cap = values[cs35l41->index];
>   
> -	put_device(acpi_dev);
> +	put_device(physdev);
>   
>   	return hw_cfg;
>   
>   err_free:
>   	kfree(hw_cfg);
>   err:
> -	put_device(acpi_dev);
> +	put_device(physdev);
>   	dev_err(cs35l41->dev, "Failed property %s: %d\n", property, ret);
>   
>   	return ERR_PTR(ret);
> @@ -370,18 +373,18 @@ static struct cs35l41_hda_hw_config *cs35l41_hda_read_acpi(struct cs35l41_hda *c
>   	/*
>   	 * Device CLSA0100 doesn't have _DSD so a gpiod_get by the label reset won't work.
>   	 * And devices created by i2c-multi-instantiate don't have their device struct pointing to
> -	 * the correct fwnode, so acpi_dev must be used here
> +	 * the correct fwnode, so acpi_dev must be used here.
>   	 * And devm functions expect that the device requesting the resource has the correct
> -	 * fwnode
> +	 * fwnode.
>   	 */
>   	if (strncmp(hid, "CLSA0100", 8) != 0)
>   		return ERR_PTR(-EINVAL);
>   
>   	/* check I2C address to assign the index */
>   	cs35l41->index = id == 0x40 ? 0 : 1;
> -	cs35l41->reset_gpio = gpiod_get_index(acpi_dev, NULL, 0, GPIOD_OUT_HIGH);
> +	cs35l41->reset_gpio = gpiod_get_index(physdev, NULL, 0, GPIOD_OUT_HIGH);
>   	cs35l41->vspk_always_on = true;
> -	put_device(acpi_dev);
> +	put_device(physdev);
>   
>   	return NULL;
>   }
> @@ -416,8 +419,7 @@ int cs35l41_hda_probe(struct device *dev, const char *device_name, int id, int i
>   		if (ret == -EBUSY) {
>   			dev_info(cs35l41->dev, "Reset line busy, assuming shared reset\n");
>   		} else {
> -			if (ret != -EPROBE_DEFER)
> -				dev_err(cs35l41->dev, "Failed to get reset GPIO: %d\n", ret);
> +			dev_err_probe(cs35l41->dev, ret, "Failed to get reset GPIO: %d\n", ret);
>   			goto err;
>   		}
>   	}
> @@ -437,7 +439,8 @@ int cs35l41_hda_probe(struct device *dev, const char *device_name, int id, int i
>   
>   	ret = regmap_read(cs35l41->regmap, CS35L41_IRQ1_STATUS3, &int_sts);
>   	if (ret || (int_sts & CS35L41_OTP_BOOT_ERR)) {
> -		dev_err(cs35l41->dev, "OTP Boot error\n");
> +		dev_err(cs35l41->dev, "OTP Boot status %x error: %d\n",
> +			int_sts & CS35L41_OTP_BOOT_ERR, ret);
>   		ret = -EIO;
>   		goto err;
>   	}
> @@ -489,7 +492,7 @@ int cs35l41_hda_probe(struct device *dev, const char *device_name, int id, int i
>   
>   	if (cs35l41->reg_seq->probe) {
>   		ret = regmap_multi_reg_write(cs35l41->regmap, cs35l41->reg_seq->probe,
> -					    cs35l41->reg_seq->num_probe);
> +					     cs35l41->reg_seq->num_probe);
>   		if (ret) {
>   			dev_err(cs35l41->dev, "Fail to apply probe reg patch: %d\n", ret);
>   			goto err;
> diff --git a/sound/pci/hda/cs35l41_hda.h b/sound/pci/hda/cs35l41_hda.h
> index 76c69a8a22f6..640afc98b686 100644
> --- a/sound/pci/hda/cs35l41_hda.h
> +++ b/sound/pci/hda/cs35l41_hda.h
> @@ -1,6 +1,6 @@
>   /* SPDX-License-Identifier: GPL-2.0
>    *
> - * cs35l41_hda.h -- CS35L41 ALSA HDA audio driver
> + * CS35L41 ALSA HDA audio driver
>    *
>    * Copyright 2021 Cirrus Logic, Inc.
>    *
> diff --git a/sound/pci/hda/cs35l41_hda_i2c.c b/sound/pci/hda/cs35l41_hda_i2c.c
> index eeb387853ee3..c2397dc53e78 100644
> --- a/sound/pci/hda/cs35l41_hda_i2c.c
> +++ b/sound/pci/hda/cs35l41_hda_i2c.c
> @@ -58,7 +58,6 @@ static struct i2c_driver cs35l41_i2c_driver = {
>   	.probe		= cs35l41_hda_i2c_probe,
>   	.remove		= cs35l41_hda_i2c_remove,
>   };
> -
>   module_i2c_driver(cs35l41_i2c_driver);
>   
>   MODULE_DESCRIPTION("HDA CS35L41 driver");
> diff --git a/sound/pci/hda/cs35l41_hda_spi.c b/sound/pci/hda/cs35l41_hda_spi.c
> index 15345a72b9d1..36815ab4e461 100644
> --- a/sound/pci/hda/cs35l41_hda_spi.c
> +++ b/sound/pci/hda/cs35l41_hda_spi.c
> @@ -55,7 +55,6 @@ static struct spi_driver cs35l41_spi_driver = {
>   	.probe		= cs35l41_hda_spi_probe,
>   	.remove		= cs35l41_hda_spi_remove,
>   };
> -
>   module_spi_driver(cs35l41_spi_driver);
>   
>   MODULE_DESCRIPTION("HDA CS35L41 driver");

Re: [PATCH 4/5] ALSA: hda: cs35l41: Tidyup code and add default cases

[Takashi Iwai]

On Thu, 13 Jan 2022 18:07:27 +0100,
Lucas Tanure wrote:
> 
> Clean up the code, plus adding default cases for switch
> and dev_err_probe use.

Please split to patches.  There are too much mixed up of changes for
different goals.  There is difference between a white-space cleanup or
such (that is, no significant change of the resultant binary), a
cleanup with a better API usage, and a code refactoring.  And, the
addition of the missing default case is rather the fix, not a clean
up...


thanks,

Takashi

[PATCH 5/5] ACPI / scan: Create platform device for CLSA0100

[Lucas Tanure]

The ACPI device with CLSA0100 is a sound card with
multiple instances of CS35L41 connected by I2C to
the main CPU.

We add an ID to the i2c_multi_instantiate_idsi list
to enumerate all I2C slaves correctly.

Signed-off-by: Lucas Tanure <tanureal@opensource.cirrus.com>
---
 drivers/acpi/scan.c                          | 2 ++
 drivers/platform/x86/i2c-multi-instantiate.c | 8 ++++++++
 2 files changed, 10 insertions(+)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index c215bc8723d0..2a68031d953e 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1753,6 +1753,8 @@ static bool acpi_device_enumeration_by_parent(struct acpi_device *device)
 	 */
 		{"BCM4752", },
 		{"LNV4752", },
+	/* Non-conforming _HID for Cirrus Logic already released */
+		{"CLSA0100", },
 		{}
 	};
 
diff --git a/drivers/platform/x86/i2c-multi-instantiate.c b/drivers/platform/x86/i2c-multi-instantiate.c
index 4956a1df5b90..a51a74933fa9 100644
--- a/drivers/platform/x86/i2c-multi-instantiate.c
+++ b/drivers/platform/x86/i2c-multi-instantiate.c
@@ -147,6 +147,12 @@ static const struct i2c_inst_data int3515_data[]  = {
 	{}
 };
 
+static const struct i2c_inst_data cs35l41_hda[] = {
+	{ "cs35l41-hda", IRQ_RESOURCE_GPIO, 0 },
+	{ "cs35l41-hda", IRQ_RESOURCE_GPIO, 0 },
+	{}
+};
+
 /*
  * Note new device-ids must also be added to i2c_multi_instantiate_ids in
  * drivers/acpi/scan.c: acpi_device_enumeration_by_parent().
@@ -155,6 +161,8 @@ static const struct acpi_device_id i2c_multi_inst_acpi_ids[] = {
 	{ "BSG1160", (unsigned long)bsg1160_data },
 	{ "BSG2150", (unsigned long)bsg2150_data },
 	{ "INT3515", (unsigned long)int3515_data },
+	/* Non-conforming _HID for Cirrus Logic already released */
+	{ "CLSA0100", (unsigned long)cs35l41_hda },
 	{ }
 };
 MODULE_DEVICE_TABLE(acpi, i2c_multi_inst_acpi_ids);
-- 
2.34.1

Re: [PATCH 5/5] ACPI / scan: Create platform device for CLSA0100

[Takashi Iwai]

On Thu, 13 Jan 2022 18:07:28 +0100,
Lucas Tanure wrote:
> 
> The ACPI device with CLSA0100 is a sound card with
> multiple instances of CS35L41 connected by I2C to
> the main CPU.
> 
> We add an ID to the i2c_multi_instantiate_idsi list
> to enumerate all I2C slaves correctly.
> 
> Signed-off-by: Lucas Tanure <tanureal@opensource.cirrus.com>

I think it's better to merge this from sound git tree together with
others in the patch set, presumably for rc1.

It'd be great if ACPI people can take a review and give an ack/nack.


Thanks!

Takashi

> ---
>  drivers/acpi/scan.c                          | 2 ++
>  drivers/platform/x86/i2c-multi-instantiate.c | 8 ++++++++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index c215bc8723d0..2a68031d953e 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -1753,6 +1753,8 @@ static bool acpi_device_enumeration_by_parent(struct acpi_device *device)
>  	 */
>  		{"BCM4752", },
>  		{"LNV4752", },
> +	/* Non-conforming _HID for Cirrus Logic already released */
> +		{"CLSA0100", },
>  		{}
>  	};
>  
> diff --git a/drivers/platform/x86/i2c-multi-instantiate.c b/drivers/platform/x86/i2c-multi-instantiate.c
> index 4956a1df5b90..a51a74933fa9 100644
> --- a/drivers/platform/x86/i2c-multi-instantiate.c
> +++ b/drivers/platform/x86/i2c-multi-instantiate.c
> @@ -147,6 +147,12 @@ static const struct i2c_inst_data int3515_data[]  = {
>  	{}
>  };
>  
> +static const struct i2c_inst_data cs35l41_hda[] = {
> +	{ "cs35l41-hda", IRQ_RESOURCE_GPIO, 0 },
> +	{ "cs35l41-hda", IRQ_RESOURCE_GPIO, 0 },
> +	{}
> +};
> +
>  /*
>   * Note new device-ids must also be added to i2c_multi_instantiate_ids in
>   * drivers/acpi/scan.c: acpi_device_enumeration_by_parent().
> @@ -155,6 +161,8 @@ static const struct acpi_device_id i2c_multi_inst_acpi_ids[] = {
>  	{ "BSG1160", (unsigned long)bsg1160_data },
>  	{ "BSG2150", (unsigned long)bsg2150_data },
>  	{ "INT3515", (unsigned long)int3515_data },
> +	/* Non-conforming _HID for Cirrus Logic already released */
> +	{ "CLSA0100", (unsigned long)cs35l41_hda },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(acpi, i2c_multi_inst_acpi_ids);
> -- 
> 2.34.1
> 

Re: [PATCH 5/5] ACPI / scan: Create platform device for CLSA0100

[Rafael J. Wysocki]

On Fri, Jan 14, 2022 at 5:19 PM Takashi Iwai <tiwai@suse.de> wrote:
>
> On Thu, 13 Jan 2022 18:07:28 +0100,
> Lucas Tanure wrote:
> >
> > The ACPI device with CLSA0100 is a sound card with
> > multiple instances of CS35L41 connected by I2C to
> > the main CPU.
> >
> > We add an ID to the i2c_multi_instantiate_idsi list
> > to enumerate all I2C slaves correctly.
> >
> > Signed-off-by: Lucas Tanure <tanureal@opensource.cirrus.com>
>
> I think it's better to merge this from sound git tree together with
> others in the patch set, presumably for rc1.
>
> It'd be great if ACPI people can take a review and give an ack/nack.

Hans, what do you think?

> > ---
> >  drivers/acpi/scan.c                          | 2 ++
> >  drivers/platform/x86/i2c-multi-instantiate.c | 8 ++++++++
> >  2 files changed, 10 insertions(+)
> >
> > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> > index c215bc8723d0..2a68031d953e 100644
> > --- a/drivers/acpi/scan.c
> > +++ b/drivers/acpi/scan.c
> > @@ -1753,6 +1753,8 @@ static bool acpi_device_enumeration_by_parent(struct acpi_device *device)
> >        */
> >               {"BCM4752", },
> >               {"LNV4752", },
> > +     /* Non-conforming _HID for Cirrus Logic already released */
> > +             {"CLSA0100", },
> >               {}
> >       };
> >
> > diff --git a/drivers/platform/x86/i2c-multi-instantiate.c b/drivers/platform/x86/i2c-multi-instantiate.c
> > index 4956a1df5b90..a51a74933fa9 100644
> > --- a/drivers/platform/x86/i2c-multi-instantiate.c
> > +++ b/drivers/platform/x86/i2c-multi-instantiate.c
> > @@ -147,6 +147,12 @@ static const struct i2c_inst_data int3515_data[]  = {
> >       {}
> >  };
> >
> > +static const struct i2c_inst_data cs35l41_hda[] = {
> > +     { "cs35l41-hda", IRQ_RESOURCE_GPIO, 0 },
> > +     { "cs35l41-hda", IRQ_RESOURCE_GPIO, 0 },
> > +     {}
> > +};
> > +
> >  /*
> >   * Note new device-ids must also be added to i2c_multi_instantiate_ids in
> >   * drivers/acpi/scan.c: acpi_device_enumeration_by_parent().
> > @@ -155,6 +161,8 @@ static const struct acpi_device_id i2c_multi_inst_acpi_ids[] = {
> >       { "BSG1160", (unsigned long)bsg1160_data },
> >       { "BSG2150", (unsigned long)bsg2150_data },
> >       { "INT3515", (unsigned long)int3515_data },
> > +     /* Non-conforming _HID for Cirrus Logic already released */
> > +     { "CLSA0100", (unsigned long)cs35l41_hda },
> >       { }
> >  };
> >  MODULE_DEVICE_TABLE(acpi, i2c_multi_inst_acpi_ids);
> > --
> > 2.34.1
> >

Re: [PATCH 5/5] ACPI / scan: Create platform device for CLSA0100

[Hans de Goede]

Hi,

On 1/14/22 18:51, Rafael J. Wysocki wrote:
> On Fri, Jan 14, 2022 at 5:19 PM Takashi Iwai <tiwai@suse.de> wrote:
>>
>> On Thu, 13 Jan 2022 18:07:28 +0100,
>> Lucas Tanure wrote:
>>>
>>> The ACPI device with CLSA0100 is a sound card with
>>> multiple instances of CS35L41 connected by I2C to
>>> the main CPU.
>>>
>>> We add an ID to the i2c_multi_instantiate_idsi list
>>> to enumerate all I2C slaves correctly.
>>>
>>> Signed-off-by: Lucas Tanure <tanureal@opensource.cirrus.com>
>>
>> I think it's better to merge this from sound git tree together with
>> others in the patch set, presumably for rc1.
>>
>> It'd be great if ACPI people can take a review and give an ack/nack.
> 
> Hans, what do you think?

This patch (5/5) applies on top of:

https://lore.kernel.org/linux-acpi/20211210154050.3713-1-sbinding@opensource.cirrus.com/

Which still needs some work and which really should be merged
through the ACPI tree. IMHO it would be best to simply drop
this (5/5) from this series and move it to the v3 of the
series which I've linked to above.

1-4 can be merged through the alsa tree independently of 5/5 AFAIK.

Regards,

Hans




> 
>>> ---
>>>  drivers/acpi/scan.c                          | 2 ++
>>>  drivers/platform/x86/i2c-multi-instantiate.c | 8 ++++++++
>>>  2 files changed, 10 insertions(+)
>>>
>>> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
>>> index c215bc8723d0..2a68031d953e 100644
>>> --- a/drivers/acpi/scan.c
>>> +++ b/drivers/acpi/scan.c
>>> @@ -1753,6 +1753,8 @@ static bool acpi_device_enumeration_by_parent(struct acpi_device *device)
>>>        */
>>>               {"BCM4752", },
>>>               {"LNV4752", },
>>> +     /* Non-conforming _HID for Cirrus Logic already released */
>>> +             {"CLSA0100", },
>>>               {}
>>>       };
>>>
>>> diff --git a/drivers/platform/x86/i2c-multi-instantiate.c b/drivers/platform/x86/i2c-multi-instantiate.c
>>> index 4956a1df5b90..a51a74933fa9 100644
>>> --- a/drivers/platform/x86/i2c-multi-instantiate.c
>>> +++ b/drivers/platform/x86/i2c-multi-instantiate.c
>>> @@ -147,6 +147,12 @@ static const struct i2c_inst_data int3515_data[]  = {
>>>       {}
>>>  };
>>>
>>> +static const struct i2c_inst_data cs35l41_hda[] = {
>>> +     { "cs35l41-hda", IRQ_RESOURCE_GPIO, 0 },
>>> +     { "cs35l41-hda", IRQ_RESOURCE_GPIO, 0 },
>>> +     {}
>>> +};
>>> +
>>>  /*
>>>   * Note new device-ids must also be added to i2c_multi_instantiate_ids in
>>>   * drivers/acpi/scan.c: acpi_device_enumeration_by_parent().
>>> @@ -155,6 +161,8 @@ static const struct acpi_device_id i2c_multi_inst_acpi_ids[] = {
>>>       { "BSG1160", (unsigned long)bsg1160_data },
>>>       { "BSG2150", (unsigned long)bsg2150_data },
>>>       { "INT3515", (unsigned long)int3515_data },
>>> +     /* Non-conforming _HID for Cirrus Logic already released */
>>> +     { "CLSA0100", (unsigned long)cs35l41_hda },
>>>       { }
>>>  };
>>>  MODULE_DEVICE_TABLE(acpi, i2c_multi_inst_acpi_ids);
>>> --
>>> 2.34.1
>>>
> 

Re: [PATCH 5/5] ACPI / scan: Create platform device for CLSA0100

[Takashi Iwai]

On Fri, 14 Jan 2022 19:56:04 +0100,
Hans de Goede wrote:
> 
> Hi,
> 
> On 1/14/22 18:51, Rafael J. Wysocki wrote:
> > On Fri, Jan 14, 2022 at 5:19 PM Takashi Iwai <tiwai@suse.de> wrote:
> >>
> >> On Thu, 13 Jan 2022 18:07:28 +0100,
> >> Lucas Tanure wrote:
> >>>
> >>> The ACPI device with CLSA0100 is a sound card with
> >>> multiple instances of CS35L41 connected by I2C to
> >>> the main CPU.
> >>>
> >>> We add an ID to the i2c_multi_instantiate_idsi list
> >>> to enumerate all I2C slaves correctly.
> >>>
> >>> Signed-off-by: Lucas Tanure <tanureal@opensource.cirrus.com>
> >>
> >> I think it's better to merge this from sound git tree together with
> >> others in the patch set, presumably for rc1.
> >>
> >> It'd be great if ACPI people can take a review and give an ack/nack.
> > 
> > Hans, what do you think?
> 
> This patch (5/5) applies on top of:
> 
> https://lore.kernel.org/linux-acpi/20211210154050.3713-1-sbinding@opensource.cirrus.com/
> 
> Which still needs some work and which really should be merged
> through the ACPI tree. IMHO it would be best to simply drop
> this (5/5) from this series and move it to the v3 of the
> series which I've linked to above.
> 
> 1-4 can be merged through the alsa tree independently of 5/5 AFAIK.

OK, that's fine.

Lucas, could you submit v3 patches in the suggested way?


thanks,

Takashi

Re: [PATCH 5/5] ACPI / scan: Create platform device for CLSA0100

[tanureal]

On 1/15/22 6:59 AM, Takashi Iwai <tiwai@suse.de> wrote:
> On Fri, 14 Jan 2022 19:56:04 +0100,
> Hans de Goede wrote:
> >
> > Hi,
> >
> > On 1/14/22 18:51, Rafael J. Wysocki wrote:
> >> On Fri, Jan 14, 2022 at 5:19 PM Takashi Iwai <tiwai@suse.de> wrote:
> >>>
> >>> On Thu, 13 Jan 2022 18:07:28 +0100,
> >>> Lucas Tanure wrote:
> >>>>
> >>>> The ACPI device with CLSA0100 is a sound card with
> >>>> multiple instances of CS35L41 connected by I2C to
> >>>> the main CPU.
> >>>>
> >>>> We add an ID to the i2c_multi_instantiate_idsi list
> >>>> to enumerate all I2C slaves correctly.
> >>>>
> >>>> Signed-off-by: Lucas Tanure <tanureal@opensource.cirrus.com>
> >>>
> >>> I think it's better to merge this from sound git tree together with
> >>> others in the patch set, presumably for rc1.
> >>>
> >>> It'd be great if ACPI people can take a review and give an ack/nack.
> >>
> >> Hans, what do you think?
> >
> > This patch (5/5) applies on top of:
> >
> > https://lore.kernel.org/linux-acpi/20211210154050.3713-1-sbinding@opensource.cirrus.com/
> >
> > Which still needs some work and which really should be merged
> > through the ACPI tree. IMHO it would be best to simply drop
> > this (5/5) from this series and move it to the v3 of the
> > series which I've linked to above.
> >
> > 1-4 can be merged through the alsa tree independently of 5/5 AFAIK.
> 
> OK, that's fine.
> 
> Lucas, could you submit v3 patches in the suggested way?
Yes, we will do that.
Thanks
> 
> 
> thanks,
> 
> Takashi
> 

Re: [PATCH 1/5] ALSA: hda: cs35l41: Avoid overwriting register patch

[Cezary Rojewski]

On 2022-01-13 6:07 PM, Lucas Tanure wrote:
> From: Charles Keepax <ckeepax@opensource.cirrus.com>
> 
> regmap_register_patch can't be used to apply the probe sequence as a
> patch is already registers with the regmap by
> cs35l41_register_errata_patch and only a single patch can be attached to
> a single regmap. The driver doesn't currently rely on a cache sync to
> re-apply this probe sequence so simply switch it to a multi write.
> 
> Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> Signed-off-by: Lucas Tanure <tanureal@opensource.cirrus.com>

Please correct me if I'm wrong, but this change looks like a fix for the 
previously added commit:
ALSA: hda: cs35l41: Add support for CS35L41 in HDA systems

and so it would be good to append appropriate 'Fixes' tag here.

> ---
>   sound/pci/hda/cs35l41_hda.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/sound/pci/hda/cs35l41_hda.c b/sound/pci/hda/cs35l41_hda.c
> index 30b40d865863..c47c5f0b4e59 100644
> --- a/sound/pci/hda/cs35l41_hda.c
> +++ b/sound/pci/hda/cs35l41_hda.c
> @@ -480,7 +480,7 @@ int cs35l41_hda_probe(struct device *dev, const char *device_name, int id, int i
>   	acpi_hw_cfg = NULL;
>   
>   	if (cs35l41->reg_seq->probe) {
> -		ret = regmap_register_patch(cs35l41->regmap, cs35l41->reg_seq->probe,
> +		ret = regmap_multi_reg_write(cs35l41->regmap, cs35l41->reg_seq->probe,
>   					    cs35l41->reg_seq->num_probe);
>   		if (ret) {
>   			dev_err(cs35l41->dev, "Fail to apply probe reg patch: %d\n", ret);
>