[patch v5 2/5] ASoC: codecs: Implementation of aw883xx configuration file parsing function

[Mark Brown]

On Fri, Nov 25, 2022 at 05:27:24PM +0800, wangweidong.a at awinic.com wrote:

> +	check_sum = GET_32_DATA(*(p_check_sum + 3), *(p_check_sum + 2),
> +				*(p_check_sum + 1), *(p_check_sum));

We've got the be32_to_cpu() and so on macros - I suspect that
GET_32_DATA() should be one of those.

> +static int aw_check_data_version(struct aw_bin *bin, int bin_num)
> +{
> +	int i = 0;
> +
> +	for (i = DATA_VERSION_V1; i < DATA_VERSION_MAX; i++) {
> +		if (bin->header_info[bin_num].bin_data_ver == i)
> +			return 0;
> +	}
> +	pr_err("aw_bin_parse Unrecognized this bin data version\n");
> +	return -DATA_VER_ERR;
> +}

This seems like an inefficient way of writing

	if (bin->header_info[bin_num].bin_data_ver < DATA_VERSION_V1 ||
	    bin->header_info[bin_num].bin_data_ver > DATA_VERSION_MAX ||)

surely?

> +static void aw_get_single_bin_header_1_0_0(struct aw_bin *bin)
> +{
> +	int i;
> +
> +	bin->header_info[bin->all_bin_parse_num].header_len = 60;
> +	bin->header_info[bin->all_bin_parse_num].check_sum =
> +		GET_32_DATA(*(bin->p_addr + 3), *(bin->p_addr + 2),
> +				*(bin->p_addr + 1), *(bin->p_addr));

The standard way of writing this would be with a packed struct with
endianness annotations, that's a bit less error prone than this.  I also
didn't spot the size validation that ensures that we're not walking past
the end of the binary image anywhere in the code, it might've been there
but it could do with being rather more obvious.  There are some size
checks further down but it's not clear that they align with what's going
on here.

> +static int aw_parse_each_of_multi_bins_1_0_0(unsigned int bin_num, int bin_serial_num,
> +				      struct aw_bin *bin)
> +{

Given a function with an each_of name I'd expect to see a loop over
multiple binaries?  I see the loop in the caller but it's a bit
confusing.  Perhaps one_of.

> +	for (i = 0; i < cfg_hdr->a_ddt_num; ++i) {
> +		if ((cfg_dde[i].data_type == ACF_SEC_TYPE_MUTLBIN) &&
> +			(aw_dev->chip_id == cfg_dde[i].chip_id) &&
> +			((aw_dev->i2c->adapter->nr == cfg_dde[i].dev_bus) &&
> +			(aw_dev->i2c->addr == cfg_dde[i].dev_addr))) {
> +			(*scene_num)++;
> +			}
> +	}

Some of the indentation in this code is really hard to read - for
example here it'd be better to align the if conditions with the brackets
in the if statement to separate from the code that gets run if the
condition is true.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20221129/e064eb08/attachment.sig>