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, ®id);
> + if (ret < 0) {
> + dev_err(cs35l41->dev, "Get Device ID failed\n");
> + goto err;
> + }
> +
> + ret = regmap_read(cs35l41->regmap, CS35L41_REVID, ®_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;
> +}
> +
>
