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

[Mark Brown]

On 7 Aug 2010, at 23:29, Mike Frysinger <vapier.adi at gmail.com> 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.