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

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

 



Couple of nit-picks and one issue with error handling, see below.

> +static int cs35l41_otp_unpack(void *data)
> +{
> +	struct cs35l41_private *cs35l41 = data;
> +	u32 *otp_mem = NULL;

initialization is not necessary? Even a static analyzer would not
complain here, would it?

> +	int i;
> +	int bit_offset, word_offset;
> +	unsigned int bit_sum = 8;
> +	u32 otp_val, otp_id_reg;
> +	const struct cs35l41_otp_map_element_t *otp_map_match = NULL;

same, this is directly assigned below.

> +	const struct cs35l41_otp_packed_element_t *otp_map = NULL;

same, you assign it with

otp_map = otp_map_match->map;

> +	unsigned int orig_spi_freq;
> +	int ret;
> +
> +	otp_mem = kmalloc_array(CS35L41_OTP_SIZE_WORDS, sizeof(*otp_mem),
> +							GFP_KERNEL);
> +	if (!otp_mem)
> +		return -ENOMEM;
> +
> +	ret = regmap_read(cs35l41->regmap, CS35L41_OTPID, &otp_id_reg);
> +	if (ret < 0) {
> +		dev_err(cs35l41->dev, "Read OTP ID failed\n");
> +		ret = -EINVAL;
> +		goto err_otp_unpack;
> +	}
> +
> +	otp_map_match = cs35l41_find_otp_map(otp_id_reg);
> +
> +	if (otp_map_match == NULL) {

if (!otp_map_match)

> +		dev_err(cs35l41->dev, "OTP Map matching ID %d not found\n",
> +				otp_id_reg);
> +		ret = -EINVAL;
> +		goto err_otp_unpack;
> +	}
> +
> +	if (cs35l41->otp_setup)
> +		cs35l41->otp_setup(cs35l41, true, &orig_spi_freq);
> +
> +	ret = regmap_bulk_read(cs35l41->regmap, CS35L41_OTP_MEM0, otp_mem,
> +						CS35L41_OTP_SIZE_WORDS);
> +	if (ret < 0) {
> +		dev_err(cs35l41->dev, "Read OTP Mem failed\n");
> +		ret = -EINVAL;
> +		goto err_otp_unpack;
> +	}
> +
> +	if (cs35l41->otp_setup)
> +		cs35l41->otp_setup(cs35l41, false, &orig_spi_freq);
> +
> +	otp_map = otp_map_match->map;
> +
> +	bit_offset = otp_map_match->bit_offset;
> +	word_offset = otp_map_match->word_offset;
> +
> +	ret = regmap_write(cs35l41->regmap, CS35L41_TEST_KEY_CTL, 0x00000055);
> +	if (ret < 0) {
> +		dev_err(cs35l41->dev, "Write Unlock key failed 1/2\n");
> +		ret = -EINVAL;
> +		goto err_otp_unpack;
> +	}
> +	ret = regmap_write(cs35l41->regmap, CS35L41_TEST_KEY_CTL, 0x000000AA);
> +	if (ret < 0) {
> +		dev_err(cs35l41->dev, "Write Unlock key failed 2/2\n");
> +		ret = -EINVAL;
> +		goto err_otp_unpack;
> +	}
> +
> +	for (i = 0; i < otp_map_match->num_elements; i++) {
> +		dev_dbg(cs35l41->dev,
> +			   "bitoffset= %d, word_offset=%d, bit_sum mod 32=%d\n",
> +					 bit_offset, word_offset, bit_sum % 32);
> +		if (bit_offset + otp_map[i].size - 1 >= 32) {
> +			otp_val = (otp_mem[word_offset] &
> +					GENMASK(31, bit_offset)) >>
> +					bit_offset;
> +			otp_val |= (otp_mem[++word_offset] &
> +					GENMASK(bit_offset +
> +						otp_map[i].size - 33, 0)) <<
> +					(32 - bit_offset);
> +			bit_offset += otp_map[i].size - 32;
> +		} else {
> +
> +			otp_val = (otp_mem[word_offset] &
> +				GENMASK(bit_offset + otp_map[i].size - 1,
> +					bit_offset)) >>	bit_offset;
> +			bit_offset += otp_map[i].size;
> +		}
> +		bit_sum += otp_map[i].size;
> +
> +		if (bit_offset == 32) {
> +			bit_offset = 0;
> +			word_offset++;
> +		}
> +
> +		if (otp_map[i].reg != 0) {
> +			ret = regmap_update_bits(cs35l41->regmap,
> +						otp_map[i].reg,
> +						GENMASK(otp_map[i].shift +
> +							otp_map[i].size - 1,
> +						otp_map[i].shift),
> +						otp_val << otp_map[i].shift);
> +			if (ret < 0) {
> +				dev_err(cs35l41->dev, "Write OTP val failed\n");
> +				ret = -EINVAL;
> +				goto err_otp_unpack;
> +			}
> +		}
> +	}
> +
> +	ret = regmap_write(cs35l41->regmap, CS35L41_TEST_KEY_CTL, 0x000000CC);
> +	if (ret < 0) {
> +		dev_err(cs35l41->dev, "Write Lock key failed 1/2\n");
> +		ret = -EINVAL;
> +		goto err_otp_unpack;
> +	}
> +	ret = regmap_write(cs35l41->regmap, CS35L41_TEST_KEY_CTL, 0x00000033);
> +	if (ret < 0) {
> +		dev_err(cs35l41->dev, "Write Lock key failed 2/2\n");
> +		ret = -EINVAL;
> +		goto err_otp_unpack;
> +	}
> +	ret = 0;
> +
> +err_otp_unpack:
> +	kfree(otp_mem);
> +	return ret;
> +}

> +static int cs35l41_main_amp_event(struct snd_soc_dapm_widget *w,
> +		struct snd_kcontrol *kcontrol, int event)
> +{
> +	struct snd_soc_component *component =
> +		snd_soc_dapm_to_component(w->dapm);
> +	struct cs35l41_private *cs35l41 =
> +		snd_soc_component_get_drvdata(component);
> +	int ret = 0;
> +	int i;
> +	bool pdn;
> +	unsigned int val;

reverse x-mas tree style for declarations?

[...]

> +static int cs35l41_pcm_hw_params(struct snd_pcm_substream *substream,
> +				 struct snd_pcm_hw_params *params,
> +				 struct snd_soc_dai *dai)
> +{
> +	struct cs35l41_private *cs35l41 =
> +			snd_soc_component_get_drvdata(dai->component);
> +	int i;
> +	unsigned int rate = params_rate(params);
> +	u8 asp_wl;

reverse xmas-tree, move 'int i' to last line of declaration block?

> +	for (i = 0; i < ARRAY_SIZE(cs35l41_fs_rates); i++) {
> +		if (rate == cs35l41_fs_rates[i].rate)
> +			break;
> +	}
> +
> +	if (i >= ARRAY_SIZE(cs35l41_fs_rates)) {
> +		dev_err(cs35l41->dev, "%s: Unsupported rate: %u\n",
> +						__func__, rate);
> +		return -EINVAL;
> +	}
> +
> +	asp_wl = params_width(params);
> +
> +	if (i < ARRAY_SIZE(cs35l41_fs_rates))
> +		regmap_update_bits(cs35l41->regmap, CS35L41_GLOBAL_CLK_CTRL,
> +			CS35L41_GLOBAL_FS_MASK,
> +			cs35l41_fs_rates[i].fs_cfg << CS35L41_GLOBAL_FS_SHIFT);
> +
> +	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> +		regmap_update_bits(cs35l41->regmap, CS35L41_SP_FORMAT,
> +				CS35L41_ASP_WIDTH_RX_MASK,
> +				asp_wl << CS35L41_ASP_WIDTH_RX_SHIFT);
> +		regmap_update_bits(cs35l41->regmap, CS35L41_SP_RX_WL,
> +				CS35L41_ASP_RX_WL_MASK,
> +				asp_wl << CS35L41_ASP_RX_WL_SHIFT);
> +		if (cs35l41->i2s_mode) {
> +			regmap_update_bits(cs35l41->regmap,
> +					CS35L41_SP_FRAME_RX_SLOT,
> +					CS35L41_ASP_RX1_SLOT_MASK,
> +					((cs35l41->pdata.right_channel) ? 1 : 0)
> +					 << CS35L41_ASP_RX1_SLOT_SHIFT);
> +			regmap_update_bits(cs35l41->regmap,
> +					CS35L41_SP_FRAME_RX_SLOT,
> +					CS35L41_ASP_RX2_SLOT_MASK,
> +					((cs35l41->pdata.right_channel) ? 0 : 1)
> +					 << CS35L41_ASP_RX2_SLOT_SHIFT);
> +		}
> +	} else {
> +		regmap_update_bits(cs35l41->regmap, CS35L41_SP_FORMAT,
> +				CS35L41_ASP_WIDTH_TX_MASK,
> +				asp_wl << CS35L41_ASP_WIDTH_TX_SHIFT);
> +		regmap_update_bits(cs35l41->regmap, CS35L41_SP_TX_WL,
> +				CS35L41_ASP_TX_WL_MASK,
> +				asp_wl << CS35L41_ASP_TX_WL_SHIFT);
> +	}
> +
> +	return 0;
> +}
> +

> +static int cs35l41_boost_config(struct cs35l41_private *cs35l41,
> +		int boost_ind, int boost_cap, int boost_ipk)
> +{
> +	int ret;

move this last?

> +	unsigned char bst_lbst_val, bst_cbst_range, bst_ipk_scaled;
> +	struct regmap *regmap = cs35l41->regmap;
> +	struct device *dev = cs35l41->dev;
> +
> +	switch (boost_ind) {
> +	case 1000:	/* 1.0 uH */
> +		bst_lbst_val = 0;
> +		break;
> +	case 1200:	/* 1.2 uH */
> +		bst_lbst_val = 1;
> +		break;
> +	case 1500:	/* 1.5 uH */
> +		bst_lbst_val = 2;
> +		break;
> +	case 2200:	/* 2.2 uH */
> +		bst_lbst_val = 3;
> +		break;
> +	default:
> +		dev_err(dev, "Invalid boost inductor value: %d nH\n",
> +				boost_ind);
> +		return -EINVAL;
> +	}
> +
> +	switch (boost_cap) {
> +	case 0 ... 19:
> +		bst_cbst_range = 0;
> +		break;
> +	case 20 ... 50:
> +		bst_cbst_range = 1;
> +		break;
> +	case 51 ... 100:
> +		bst_cbst_range = 2;
> +		break;
> +	case 101 ... 200:
> +		bst_cbst_range = 3;
> +		break;
> +	default:	/* 201 uF and greater */
> +		bst_cbst_range = 4;
> +	}
> +
> +	ret = regmap_update_bits(regmap, CS35L41_BSTCVRT_COEFF,
> +			CS35L41_BST_K1_MASK,
> +			cs35l41_bst_k1_table[bst_lbst_val][bst_cbst_range]
> +				<< CS35L41_BST_K1_SHIFT);
> +	if (ret) {
> +		dev_err(dev, "Failed to write boost K1 coefficient\n");
> +		return ret;
> +	}
> +
> +	ret = regmap_update_bits(regmap, CS35L41_BSTCVRT_COEFF,
> +			CS35L41_BST_K2_MASK,
> +			cs35l41_bst_k2_table[bst_lbst_val][bst_cbst_range]
> +				<< CS35L41_BST_K2_SHIFT);
> +	if (ret) {
> +		dev_err(dev, "Failed to write boost K2 coefficient\n");
> +		return ret;
> +	}
> +
> +	ret = regmap_update_bits(regmap, CS35L41_BSTCVRT_SLOPE_LBST,
> +			CS35L41_BST_SLOPE_MASK,
> +			cs35l41_bst_slope_table[bst_lbst_val]
> +				<< CS35L41_BST_SLOPE_SHIFT);
> +	if (ret) {
> +		dev_err(dev, "Failed to write boost slope coefficient\n");
> +		return ret;
> +	}
> +
> +	ret = regmap_update_bits(regmap, CS35L41_BSTCVRT_SLOPE_LBST,
> +			CS35L41_BST_LBST_VAL_MASK,
> +			bst_lbst_val << CS35L41_BST_LBST_VAL_SHIFT);
> +	if (ret) {
> +		dev_err(dev, "Failed to write boost inductor value\n");
> +		return ret;
> +	}
> +
> +	if ((boost_ipk < 1600) || (boost_ipk > 4500)) {
> +		dev_err(dev, "Invalid boost inductor peak current: %d mA\n",
> +				boost_ipk);
> +		return -EINVAL;
> +	}
> +	bst_ipk_scaled = ((boost_ipk - 1600) / 50) + 0x10;
> +
> +	ret = regmap_update_bits(regmap, CS35L41_BSTCVRT_PEAK_CUR,
> +			CS35L41_BST_IPK_MASK,
> +			bst_ipk_scaled << CS35L41_BST_IPK_SHIFT);
> +	if (ret) {
> +		dev_err(dev, "Failed to write boost inductor peak current\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +

> +int cs35l41_probe(struct cs35l41_private *cs35l41,
> +				struct cs35l41_platform_data *pdata)
> +{
> +	int ret;
> +	u32 regid, reg_revid, i, mtl_revid, int_status, chipid_match;
> +	int timeout;
> +	int irq_pol = 0;
> +
> +
> +	for (i = 0; i < CS35L41_NUM_SUPPLIES; i++)
> +		cs35l41->supplies[i].supply = cs35l41_supplies[i];
> +
> +	ret = devm_regulator_bulk_get(cs35l41->dev, CS35L41_NUM_SUPPLIES,
> +					cs35l41->supplies);
> +	if (ret != 0) {
> +		dev_err(cs35l41->dev,
> +			"Failed to request core supplies: %d\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	if (pdata) {
> +		cs35l41->pdata = *pdata;
> +	} else {
> +		ret = cs35l41_handle_pdata(cs35l41->dev, &cs35l41->pdata,
> +					     cs35l41);
> +		if (ret != 0) {
> +			ret = -ENODEV;
> +			goto err;

so here you will disable regulators that have not been enabled, is it
intentional?

mixing gotos and returns is confusing...

and in addition you will set the reset_gpio that you only get a couple
of lines below, that will be a page fault?

> +		}
> +	}
> +
> +	ret = regulator_bulk_enable(CS35L41_NUM_SUPPLIES, cs35l41->supplies);
> +	if (ret != 0) {
> +		dev_err(cs35l41->dev,
> +			"Failed to enable core supplies: %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* returning NULL can be an option if in stereo mode */
> +	cs35l41->reset_gpio = devm_gpiod_get_optional(cs35l41->dev, "reset",
> +							GPIOD_OUT_LOW);
> +	if (IS_ERR(cs35l41->reset_gpio)) {
> +		ret = PTR_ERR(cs35l41->reset_gpio);
> +		cs35l41->reset_gpio = NULL;
> +		if (ret == -EBUSY) {
> +			dev_info(cs35l41->dev,
> +				 "Reset line busy, assuming shared reset\n");
> +		} else {
> +			dev_err(cs35l41->dev,
> +				"Failed to get reset GPIO: %d\n", ret);
> +			goto err;
> +		}
> +	}
> +	if (cs35l41->reset_gpio) {
> +		/* satisfy minimum reset pulse width spec */
> +		usleep_range(2000, 2100);
> +		gpiod_set_value_cansleep(cs35l41->reset_gpio, 1);
> +	}
> +
> +	usleep_range(2000, 2100);
> +
> +	timeout = 100;
> +	do {
> +		if (timeout == 0) {
> +			dev_err(cs35l41->dev,
> +				"Timeout waiting for OTP_BOOT_DONE\n");
> +			ret = -EBUSY;
> +			goto err;
> +		}
> +		usleep_range(1000, 1100);
> +		regmap_read(cs35l41->regmap, CS35L41_IRQ1_STATUS4, &int_status);
> +		timeout--;
> +	} while (!(int_status & CS35L41_OTP_BOOT_DONE));
> +
> +	regmap_read(cs35l41->regmap, CS35L41_IRQ1_STATUS3, &int_status);
> +	if (int_status & CS35L41_OTP_BOOT_ERR) {
> +		dev_err(cs35l41->dev, "OTP Boot error\n");
> +		ret = -EINVAL;
> +		goto err;
> +	}
> +
> +	ret = regmap_read(cs35l41->regmap, CS35L41_DEVID, &regid);
> +	if (ret < 0) {
> +		dev_err(cs35l41->dev, "Get Device ID failed\n");
> +		goto err;
> +	}
> +
> +	ret = regmap_read(cs35l41->regmap, CS35L41_REVID, &reg_revid);
> +	if (ret < 0) {
> +		dev_err(cs35l41->dev, "Get Revision ID failed\n");
> +		goto err;
> +	}
> +
> +	mtl_revid = reg_revid & CS35L41_MTLREVID_MASK;
> +
> +	/* CS35L41 will have even MTLREVID
> +	 * CS35L41R will have odd MTLREVID
> +	 */
> +	chipid_match = (mtl_revid % 2) ? CS35L41R_CHIP_ID : CS35L41_CHIP_ID;
> +	if (regid != chipid_match) {
> +		dev_err(cs35l41->dev, "CS35L41 Device ID (%X). Expected ID %X\n",
> +			regid, chipid_match);
> +		ret = -ENODEV;
> +		goto err;
> +	}
> +
> +	switch (reg_revid) {
> +	case CS35L41_REVID_A0:
> +		ret = regmap_register_patch(cs35l41->regmap,
> +				cs35l41_reva0_errata_patch,
> +				ARRAY_SIZE(cs35l41_reva0_errata_patch));
> +		if (ret < 0) {
> +			dev_err(cs35l41->dev,
> +				"Failed to apply A0 errata patch %d\n", ret);
> +			goto err;
> +		}
> +		break;
> +	case CS35L41_REVID_B0:
> +		ret = regmap_register_patch(cs35l41->regmap,
> +				cs35l41_revb0_errata_patch,
> +				ARRAY_SIZE(cs35l41_revb0_errata_patch));
> +		if (ret < 0) {
> +			dev_err(cs35l41->dev,
> +				"Failed to apply B0 errata patch %d\n", ret);
> +			goto err;
> +		}
> +		break;
> +	case CS35L41_REVID_B2:
> +		ret = regmap_register_patch(cs35l41->regmap,
> +				cs35l41_revb2_errata_patch,
> +				ARRAY_SIZE(cs35l41_revb2_errata_patch));
> +		if (ret < 0) {
> +			dev_err(cs35l41->dev,
> +				"Failed to apply B2 errata patch %d\n", ret);
> +			goto err;
> +		}
> +		break;
> +	}
> +
> +	irq_pol = cs35l41_irq_gpio_config(cs35l41);
> +
> +	/* Set interrupt masks for critical errors */
> +	regmap_write(cs35l41->regmap, CS35L41_IRQ1_MASK1,
> +			CS35L41_INT1_MASK_DEFAULT);
> +
> +	ret = devm_request_threaded_irq(cs35l41->dev, cs35l41->irq, NULL,
> +			cs35l41_irq, IRQF_ONESHOT | IRQF_SHARED | irq_pol,
> +			"cs35l41", cs35l41);
> +
> +	/* CS35L41 needs INT for PDN_DONE */
> +	if (ret != 0) {
> +		dev_err(cs35l41->dev, "Failed to request IRQ: %d\n", ret);
> +		ret = -ENODEV;
> +		goto err;
> +	}
> +
> +	ret = cs35l41_otp_unpack(cs35l41);
> +	if (ret < 0) {
> +		dev_err(cs35l41->dev, "OTP Unpack failed\n");
> +		goto err;
> +	}
> +
> +	ret = regmap_write(cs35l41->regmap, CS35L41_DSP1_CCM_CORE_CTRL, 0);
> +	if (ret < 0) {
> +		dev_err(cs35l41->dev, "Write CCM_CORE_CTRL failed\n");
> +		goto err;
> +	}
> +
> +	ret = regmap_update_bits(cs35l41->regmap, CS35L41_PWR_CTRL2,
> +				 CS35L41_AMP_EN_MASK, 0);
> +	if (ret < 0) {
> +		dev_err(cs35l41->dev, "Write CS35L41_PWR_CTRL2 failed\n");
> +		goto err;
> +	}
> +
> +	ret = devm_snd_soc_register_component(cs35l41->dev,
> +					&soc_component_dev_cs35l41,
> +					cs35l41_dai, ARRAY_SIZE(cs35l41_dai));
> +	if (ret < 0) {
> +		dev_err(cs35l41->dev, "%s: Register codec failed\n", __func__);
> +		goto err;
> +	}
> +
> +	ret = cs35l41_set_pdata(cs35l41);
> +	if (ret < 0) {
> +		dev_err(cs35l41->dev, "%s: Set pdata failed\n", __func__);
> +		goto err;
> +	}
> +
> +	dev_info(cs35l41->dev, "Cirrus Logic CS35L41 (%x), Revision: %02X\n",
> +			regid, reg_revid);
> +
> +	return 0;
> +
> +err:
> +	regulator_bulk_disable(CS35L41_NUM_SUPPLIES, cs35l41->supplies);
> +	gpiod_set_value_cansleep(cs35l41->reset_gpio, 0);
> +	return ret;
> +}
> +
> +int cs35l41_remove(struct cs35l41_private *cs35l41)
> +{
> +	regmap_write(cs35l41->regmap, CS35L41_IRQ1_MASK1, 0xFFFFFFFF);
> +	regulator_bulk_disable(CS35L41_NUM_SUPPLIES, cs35l41->supplies);
> +	gpiod_set_value_cansleep(cs35l41->reset_gpio, 0);
> +	return 0;
> +}
> +
>



[Index of Archives]     [ALSA User]     [Linux Audio Users]     [Pulse Audio]     [Kernel Archive]     [Asterisk PBX]     [Photo Sharing]     [Linux Sound]     [Video 4 Linux]     [Gimp]     [Yosemite News]

  Powered by Linux