[singalsu]

This patch fixes the issue of audio left muted after a cycle
of mute and unmute commands. The additional improvements are
applying of gain ramp for mute that helps to avoid annoying click
sounds. Also since the driver issues both volume and switch
commands back to back the duplicate scheduling of ramp update is
prevented if there is already a ramp happening. The old ramp
will operate based on new step and direction if it was changed.

Some variable names were changed in function volume_set_chan()
for better readability. The impacted trace print commands were
also updated to not split strings unnecessarily.

Signed-off-by: Seppo Ingalsuo seppo.ingalsuo@linux.intel.com

[paulstelian97]

I can accept it as is even though in general my rule is to have renames and other pass-by coding style improvements as separate commits.

The comment of changing variable names (in the commit message) also hints at a possible split. I won't force it though.

Main change looks good to me.

[lyakh]

minor comments, most (or all?) commented code is are also not introduced by this patch, but it could be improved, while at it.

[lyakh]

nitpick: with your addition I'd regroup this as

if (again)
	return SOF_TASK_STATE_RESCHEDULE;

cd->vol_ramp_active = false;

return SOF_TASK_STATE_COMPLETED;

[singalsu]

Yep, makes sense. I was about to do this myself as well.

[singalsu]

done

[lyakh]

<rant> I find we use zero-initialising allocations (rzalloc()) exceedingly, sometimes also where they aren't really required, and then we also additionally initialise fields of the just allocated object with 0 or NULL of false... Seems strange to me </rant>

[paulstelian97]

@lyakh I'd find it safer than forgetting to zero-initialize where it's needed, for one.

[singalsu]

I'll do whatever you recommend. I'm myself preferring to be explicit with things that need to be initialized. Let me know your preference.

[lyakh]

Don't we need to error out in this case? If we do, I'd revert the test to do

if (chan_is_invalid) {
	trace();
	return -EBADERROR;
}
normal_case();

[paulstelian97]

Maybe some ret = -EINVAL so the following if (ret) return ret; will trigger. Agreed.

[singalsu]

ok

[singalsu]

done

[lyakh]

This is the only path where ret is set and thus, where we might have to abort. I would move the ret < 0 check here and limit the scope of ret definition to just this branch.

[singalsu]

Yes, makes sense.

[singalsu]

done

[lyakh]

ch is defined as int and you check it for < 0, so, I think %d is a better choice here.

[singalsu]

Yep

[singalsu]

done

[lyakh]

ditto for aborting and %d

[singalsu]

ok

[singalsu]

done

[ranj063]

@singalsu typo here "set if channel is muted"?

[singalsu]

Oops, fixed it!

[ranj063]

@singalsu other than the minor typo in the comment, the patch looks good to me!

[lyakh]

You could also move the declaration here

int ret = volume_set_...();

to make it clear that ret isn't needed anywhere else. But probably not worth redoing the patch just for this.

[singalsu]

Unless I'm mistaken that's not done in SOF. I'll fix the typo any way, it's safe to change without need for me to re-test stuff.

[jajanusz]

IMHO what @lyakh suggested is better, BUT @singalsu is right that it's forbidden in SOF. In kernel you can see that they allow declaration on start of the block, but in sound tree there is rule that everything goes to the start of the function, because most of maintainers decided so.

[jajanusz]

Looks like checkpatch doesn't understand strings in our traces, it's something that should be fixed