asoc: problem with snd_soc_dai_set_fmt()

asoc: problem with snd_soc_dai_set_fmt()

[Timur Tabi]

There is a problem with snd_soc_dai_set_fmt():

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 -EINVAL;
}

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

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.

May I propose this:

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;
}

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: asoc: problem with snd_soc_dai_set_fmt()

[Mark Brown]

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.

Re: asoc: problem with snd_soc_dai_set_fmt()

[Timur Tabi]

On Thu, Apr 29, 2010 at 5:18 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> 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*

Grr... I've been calling it the fabric driver for a long time, and I
know I didn't make up that term.

> Right, but really this is the case - the driver has completely ignored
> what the machine driver was trying to do.

But there are several functions like that, such as the _pll function.
What if the CPU driver really doesn't care what the frequency is or
the PLL or whatever?  Do I need to define stub functions for all of
these?

>  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.

But if the CPU driver does not define a function to check for that,
isn't that the same thing as saying, "I really don't care -- I'll
support whatever you want"?

> 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.

IMHO, ignored != not supported.

For instance, it's possible for a CPU driver initially to support only
some configurations, so the set_fmt function would be necessary.
Later, the CPU driver could be updated to support all possible
configurations, and it could know that via some other mechanism (like
the device tree), and so it would be redundant if it let the machine
driver know what that configuration is.

This is what I'm doing to my SSI driver.  Now that it probes the SSI
nodes directly, it doesn't need the machine driver to tell it what the
capabilities are.

> Another short term option would be to change the error code to be
> something a bit more distinctive than -EINVAL.

That would help.

> The current expectation is that the machine driver knows what the
> hardware is capable of

But not necessarily at compile time!  I was hoping to use this as a
mechanism for determining the capabilities at run time.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: asoc: problem with snd_soc_dai_set_fmt()

[Mark Brown]

On Thu, Apr 29, 2010 at 06:30:19PM -0500, Timur Tabi wrote:
> On Thu, Apr 29, 2010 at 5:18 PM, Mark Brown
> > On Thu, Apr 29, 2010 at 04:22:17PM -0500, Timur Tabi wrote:

> >> that if the fabric driver does this:

> > *machine*

> Grr... I've been calling it the fabric driver for a long time, and I
> know I didn't make up that term.

You may have missed some of my more subtle hints previously :)

> > Right, but really this is the case - the driver has completely ignored
> > what the machine driver was trying to do.

> But there are several functions like that, such as the _pll function.
> What if the CPU driver really doesn't care what the frequency is or
> the PLL or whatever?  Do I need to define stub functions for all of
> these?

The driver should expose whatever functionality the hardware has.  If
your CPU driver doesn't have any PLLs/FLLs that can be usefully
configured by the user using set_pll() then it shouldn't implement a
set_pll() function.  If it does have such hardware then it should
implement set_pll() so users can configure them, unless it's always
possible for the driver to do the configuration autonomously for all
situations in which case it should just do that.

> >  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.

> But if the CPU driver does not define a function to check for that,
> isn't that the same thing as saying, "I really don't care -- I'll
> support whatever you want"?

Not at all.  It'll mean that either the device is fixed function or
configurability has not yet been implemented.  The set_fmt() call is how
the driver finds out what the bus is doing (and many of the formats are
indistinguishable from each other without semantic information so
looking at the wire isn't going to help).

> > 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.

> IMHO, ignored != not supported.

No, it really does mean that it's ignoring what it's being told to do.

It may be that the configuration that was requested just happens to be
what the hardware is set up to do but there's no way for the core to
tell that, all it knows is that it didn't have a mechanism to pass on
the configuration to the driver.

> For instance, it's possible for a CPU driver initially to support only
> some configurations, so the set_fmt function would be necessary.
> Later, the CPU driver could be updated to support all possible
> configurations, and it could know that via some other mechanism (like
> the device tree), and so it would be redundant if it let the machine
> driver know what that configuration is.

If the machine driver is relying on the configuration having been done
by some other part of the system then it shouldn't be trying to
explicitly set the format in the first place.  If it doesn't care if the
configuration it requested was actually implemented then it should just
ignore the return value.

> This is what I'm doing to my SSI driver.  Now that it probes the SSI
> nodes directly, it doesn't need the machine driver to tell it what the
> capabilities are.

Well, if your machine driver knows that the device tree will configure
everything it needs then it shouldn't be trying to set the format in the
first place, just as if it knows that the hardware is fixed function.
In your case presumably the machine driver is finding the configuration
it is trying to apply from the device tree so you should just make it
handle missing information.

> > The current expectation is that the machine driver knows what the
> > hardware is capable of

> But not necessarily at compile time!  I was hoping to use this as a
> mechanism for determining the capabilities at run time.

As I said in my previous message if you want to query the capabilities
of the CODEC you'd need to add a method for the CODEC to report
capabilities.

As has been repeatedly discussed there is a strong expectation that the
machine driver knows the details of the hardware it's gluing together
since this is the basic function of the driver.  Your desire to write a
generic machine driver which will work with a very wide range of boards
is not something that the existing code makes any effort to support.

Re: asoc: problem with snd_soc_dai_set_fmt()

[jassi brar]

On Fri, Apr 30, 2010 at 7:18 AM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Thu, Apr 29, 2010 at 04:22:17PM -0500, Timur Tabi wrote:
>> 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.
Though I am ok with the status quo -- snd_soc_dai_ops members being
truly optional and machine driver writers knowing both the DAIs and calling
only appropriate functions, but if we are to move to more generic machine
drivers how about hiding such members of snd_soc_dai_ops from machine
drivers and let the machine drivers specify the exact requirement to ASoC
via some generic enough data structure. Part of that configuration can be
done by ASoC while the platform specific stuff passed onto DAI drivers.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: asoc: problem with snd_soc_dai_set_fmt()

[Mark Brown]

On Fri, Apr 30, 2010 at 10:28:57AM +0900, jassi brar wrote:
> On Fri, Apr 30, 2010 at 7:18 AM, Mark Brown

> > 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.

...

> Though I am ok with the status quo -- snd_soc_dai_ops members being
> truly optional and machine driver writers knowing both the DAIs and calling
> only appropriate functions, but if we are to move to more generic machine
> drivers how about hiding such members of snd_soc_dai_ops from machine
> drivers and let the machine drivers specify the exact requirement to ASoC
> via some generic enough data structure. Part of that configuration can be
> done by ASoC while the platform specific stuff passed onto DAI drivers.

Yes, the capability information I suggested above would be required to
implement such a thing.  It does need to be very much optional, though,
to allow room for more complex systems.