Re: asoc: problem with snd_soc_dai_set_fmt()

[Mark Brown <broonie@opensource.wolfsonmicro.com>]

On Thu, Apr 29, 2010 at 04:22:17PM -0500, Timur Tabi wrote:

> Let's say the DAI driver has not defined a .set_fmt() function.  This means
> that if the fabric driver does this:

*machine*

> ret = snd_soc_dai_set_fmt(cpu_dai, machine_data->dai_format);
> if (ret < 0) {
> 	dev_err(dev, "could not set CPU driver audio format\n");
> 	return ret;
> }

> It's going to think that the DAI driver *rejected* the DAI format.  What
> this means is that I cannot make this function optional.  I have to define
> this function in my CPU driver.

Right, but really this is the case - the driver has completely ignored
what the machine driver was trying to do.  It may be that the default
behaviour is what was asked for, but it may also be that you've asked
for I2S format and got DSP format or something similiarly incompatible.

> int snd_soc_dai_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
> {
> 	if (dai->driver && dai->driver->ops->set_fmt)
> 		return dai->driver->ops->set_fmt(dai, fmt);
> 	else
> 		return 0;
> }

Due to the above issue I don't think this is a good idea - we really
ought to let the machine driver know if the request it made was ignored
in case it is trying to set up something that can't be supported.
Another short term option would be to change the error code to be
something a bit more distinctive than -EINVAL.  If we want to support
very generic machine drivers that genuinely don't know what hardware is
able to do I think we'd be be better off doing something like adding
capability masks to the drivers so these functions can validate what
they're being asked to do, at which point we know the actual format so
returning 0 isn't an issue.

The current expectation is that the machine driver knows what the
hardware is capable of and won't try to set anything silly, in the case
of fixed format devices that don't implement set_fmt() that consists of
just not calling set_fmt() for the DAI at all.

There is a genuine problem with the above code, BTW - the check for
driver->ops has got lost in the suffle.