[alsa-devel] [Uclinux-dist-devel] [PATCH 5/6] ASoC: new ADAU1761 codec driver

[Mike Frysinger]

On Sat, Aug 7, 2010 at 18:36, Mark Brown wrote:
> On 7 Aug 2010, at 23:29, Mike Frysinger wrote:
>>>> +static ssize_t adau1371_dsp_load(struct device *dev,
>>>> +                               struct device_attribute *attr,
>>>> +                               const char *buf, size_t count)
>>>> +{
>>>> +     struct snd_soc_device *socdev = dev_get_drvdata(dev);
>>>> +     struct snd_soc_codec *codec = socdev->card->codec;
>>>> +     int ret = 0;
>>>> +
>>>> +     ret = adau1761_setprogram(codec);
>>>> +     if (ret)
>>>> +             return ret;
>>>> +     else
>>>> +             return count;
>>>> +}
>>>> +static DEVICE_ATTR(dsp, 0644, NULL, adau1371_dsp_load);
>>>
>>> This looks redundant - it doesn't offer any opportunity to configure the
>>> firmware to download and you're already (as one one would expect)
>>> automatically downloading the firmware. If this is being exposed at all it
>>> should be via debugfs.
>>
>> SigmaDSP parts have the ability to change firmwares on the fly.  this
>> isnt a debug feature, it's somewhat core to these parts.  idea being
>> that you can load up different mini signal processing engines based on
>> the current needs.
>
> Right, but see my point about there being no opportunity to configure the
> firmware - if you were allowing the user to select between multiple
> firmwares available then I expect that the value written into the sysfs file
> would be parsed as the name of the new firmware file to load or otherwise
> select a new firmware. With the current code the value written is totally
> ignored and the application layer has to overwrite the firmware in the
> filesystem (or have a custom helper program which does the equivalent
> thereof) so as far as the kernel is concerned it's going to reload the same
> firmware which isn't ideal.

yes, that is the current limitation that'll need to be fixed.  not a
big hassle for us to replace the file when doing initial testing ;).
-mike