[alsa-devel] asoc: problem with snd_soc_dai_set_fmt()
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; }
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.
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.
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.
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.
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.
participants (3)
-
jassi brar -
Mark Brown -
Timur Tabi