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)) {
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;elseinfo->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