Re: [PATCH v5 02/10] ASoC: cs35l41: Move cs35l41_otp_unpack to shared code

[Cezary Rojewski <cezary.rojewski@intel.com>]

On 2021-12-16 12:43 PM, Lucas Tanure wrote:
> ASoC and HDA will do the same cs35l41_otp_unpack, so move it
> to shared code

...

> +static const struct cs35l41_otp_map_element_t *cs35l41_find_otp_map(u32 otp_id)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(cs35l41_otp_map_map); i++) {
> +		if (cs35l41_otp_map_map[i].id == otp_id)
> +			return &cs35l41_otp_map_map[i];
> +	}

The parenthesis could be dropped.

> +	return NULL;
> +}
> +int cs35l41_otp_unpack(struct device *dev, struct regmap *regmap)
> +{
> +	const struct cs35l41_otp_map_element_t *otp_map_match;
> +	const struct cs35l41_otp_packed_element_t *otp_map;
> +	int bit_offset, word_offset, ret, i;
> +	unsigned int bit_sum = 8;
> +	u32 otp_val, otp_id_reg;
> +	u32 *otp_mem;
> +
> +	otp_mem = kmalloc_array(CS35L41_OTP_SIZE_WORDS, sizeof(*otp_mem), GFP_KERNEL);
> +	if (!otp_mem)
> +		return -ENOMEM;
> +
> +	ret = regmap_read(regmap, CS35L41_OTPID, &otp_id_reg);
> +	if (ret) {
> +		dev_err(dev, "Read OTP ID failed: %d\n", ret);
> +		goto err_otp_unpack;
> +	}
> +
> +	otp_map_match = cs35l41_find_otp_map(otp_id_reg);
> +
> +	if (!otp_map_match) {
> +		dev_err(dev, "OTP Map matching ID %d not found\n", otp_id_reg);
> +		ret = -EINVAL;
> +		goto err_otp_unpack;
> +	}

This block could be understood as: assign and check. Surrounding blocks 
that carry similar value do not have a newline between assignment and 
check. My suggestion is to drop that newline here so the block looks 
more cohesive when compared with the rest of the function.

> +
> +	ret = regmap_bulk_read(regmap, CS35L41_OTP_MEM0, otp_mem, CS35L41_OTP_SIZE_WORDS);
> +	if (ret) {
> +		dev_err(dev, "Read OTP Mem failed: %d\n", ret);
> +		goto err_otp_unpack;
> +	}
> +
> +	otp_map = otp_map_match->map;
> +
> +	bit_offset = otp_map_match->bit_offset;
> +	word_offset = otp_map_match->word_offset;
> +
> +	ret = regmap_write(regmap, CS35L41_TEST_KEY_CTL, 0x00000055);
> +	if (ret) {
> +		dev_err(dev, "Write Unlock key failed 1/2: %d\n", ret);
> +		goto err_otp_unpack;
> +	}
> +	ret = regmap_write(regmap, CS35L41_TEST_KEY_CTL, 0x000000AA);
> +	if (ret) {
> +		dev_err(dev, "Write Unlock key failed 2/2: %d\n", ret);
> +		goto err_otp_unpack;
> +	}
> +
> +	for (i = 0; i < otp_map_match->num_elements; i++) {
> +		dev_dbg(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;

The ')' looks off (the '>>' too), at least it does not match the 
convention seen in if-statement above. Choosing single convention could 
improve the readability.

> +			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(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(dev, "Write OTP val failed: %d\n", ret);
> +				goto err_otp_unpack;
> +			}
> +		}
> +	}
> +
> +	ret = regmap_write(regmap, CS35L41_TEST_KEY_CTL, 0x000000CC);
> +	if (ret) {
> +		dev_err(dev, "Write Lock key failed 1/2: %d\n", ret);
> +		goto err_otp_unpack;
> +	}
> +	ret = regmap_write(regmap, CS35L41_TEST_KEY_CTL, 0x00000033);
> +	if (ret) {
> +		dev_err(dev, "Write Lock key failed 2/2: %d\n", ret);
> +		goto err_otp_unpack;
> +	}
> +	ret = 0;

Hmm.. maybe I'm missing something, but isn't the 'ret' already '0' by 
the time we get here?

> +err_otp_unpack:
> +	kfree(otp_mem);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(cs35l41_otp_unpack);
> +