[alsa-devel] [PATCH 1/1] ASoC: sti: fix compilation warnings

[Takashi Iwai]

On Wed, 09 Sep 2015 19:04:46 +0200,
Arnaud Pouliquen wrote:
> 
> Add check on of_property_read_string return to avoid compilation warning.

The purpose of the patch isn't to avoid compilation warning.  It's for
fixing the bugs in the code.  Fixing compile warning is just a
consequence, not the goal.

Please rewrite the subject and the change log text appropriately.


Some review comments below:

> -	of_property_read_u32(pnode, "version", &player->ver);
> -	if (player->ver == SND_ST_UNIPERIF_VERSION_UNKNOWN) {
> +	if ((of_property_read_u32(pnode, "version", &player->ver)) ||

Better to drop superfluous parentheses.

> +	    (player->ver == SND_ST_UNIPERIF_VERSION_UNKNOWN)) {
>  		dev_err(dev, "Unknown uniperipheral version ");
>  		return -EINVAL;
>  	}

In general, such a sanity check should be rather checking the valid
value ranges, something like:
	if (ver < VER_MIN || ver > VER_MAX)
		error();
In short, you can't trust the value passed by DT, so you need to check
it strictly (if needed).

But this doesn't have to be covered by this patch.  Better to be in
another fix patch.

> @@ -998,19 +998,25 @@ static int uni_player_parse_dt(struct platform_device *pdev,
>  	if (player->ver >= SND_ST_UNIPERIF_VERSION_UNI_PLR_TOP_1_0)
>  		info->underflow_enabled = 1;
>  
> -	of_property_read_u32(pnode, "uniperiph-id", &info->id);
> +	if (of_property_read_u32(pnode, "uniperiph-id", &info->id)) {
> +		dev_err(dev, "uniperipheral id not defined");
> +		return -EINVAL;
> +	}
>  
>  	/* Read the device mode property */
> -	of_property_read_string(pnode, "mode", &mode);
> -
> -	if (strcasecmp(mode, "hdmi") == 0)
> -		info->player_type = SND_ST_UNIPERIF_PLAYER_TYPE_HDMI;
> -	else if (strcasecmp(mode, "pcm") == 0)
> -		info->player_type = SND_ST_UNIPERIF_PLAYER_TYPE_PCM;
> -	else if (strcasecmp(mode, "spdif") == 0)
> -		info->player_type = SND_ST_UNIPERIF_PLAYER_TYPE_SPDIF;
> -	else
> -		info->player_type = SND_ST_UNIPERIF_PLAYER_TYPE_NONE;
> +	if (!of_property_read_string(pnode, "mode", &mode)) {
> +		if (strcasecmp(mode, "hdmi") == 0)
> +			info->player_type = SND_ST_UNIPERIF_PLAYER_TYPE_HDMI;
> +		else if (strcasecmp(mode, "pcm") == 0)
> +			info->player_type = SND_ST_UNIPERIF_PLAYER_TYPE_PCM;
> +		else if (strcasecmp(mode, "spdif") == 0)
> +			info->player_type = SND_ST_UNIPERIF_PLAYER_TYPE_SPDIF;
> +		else
> +			info->player_type = SND_ST_UNIPERIF_PLAYER_TYPE_NONE;
> +	} else {
> +		dev_err(dev, "uniperipheral mode not defined");
> +		return -EINVAL;
> +	}

A form like below would be clearer:

	if (of_property_read_string(pnode, "mode", &mode)) {
		dev_err(dev, "uniperipheral mode not defined");
		return -EINVAL;
	}

	if (strcasecmp(mode, "hdmi") == 0)
		info->player_type = SND_ST_UNIPERIF_PLAYER_TYPE_HDMI;
	....


thanks,

Takashi