Messages in this thread | | | Date | Fri, 17 Dec 2021 13:59:16 +0100 | Subject | Re: [PATCH v5 02/10] ASoC: cs35l41: Move cs35l41_otp_unpack to shared code | From | Cezary Rojewski <> |
| |
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); > +
| |