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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxxxxxxxxxxx>
---
  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");




[Index of Archives]     [Linux Kernel Development]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux