linux-kernel - Re: [Device-drivers-devel] [PATCH] Add driver for Analog Devices ADAU1701 SigmaDSP

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <AANLkTikynw7vCsqqyNZW-LFuGUKKQEtG3s4YCZjpOvko@mail.gmail.com>
Date:	Mon, 7 Mar 2011 06:50:15 -0500
From:	Mike Frysinger <vapier.adi@...il.com>
To:	Mark Brown <broonie@...nsource.wolfsonmicro.com>
Cc:	cliff.cai@...log.com, device-drivers-devel@...ckfin.uclinux.org,
	akpm@...ux-foundation.org, linux-kernel@...r.kernel.org,
	alsa-devel@...a-project.org, lrg@...mlogic.co.uk
Subject: Re: [Device-drivers-devel] [PATCH] Add driver for Analog Devices
 ADAU1701 SigmaDSP

On Mon, Mar 7, 2011 at 06:41, Mark Brown wrote:
> On Mon, Mar 07, 2011 at 09:11:42AM +0800, cliff.cai@...log.com wrote:
>> From: Cliff Cai <cliff.cai@...log.com>
>> It needs to include "linux/sigma.h" which is still in Andrew Morton's
>> tree.
>
> Is this needed by other trees?

no

> I can't apply this driver until it's merged into ASoC via some means

and andrew has been holding off on merging the firmware driver until
something in mainline needs it.  once a driver gets to the point where
you're fine with merging it, we can bug akpm to push the sigma
firmware loader to linus.

>>       select SND_SOC_PCM3008
>>       select SND_SOC_SPDIF
>>       select SND_SOC_SSM2602 if I2C
>> +     select SND_SOC_ADAU1701 if I2C
>>       select SND_SOC_STAC9766 if SND_SOC_AC97_BUS
>>       select SND_SOC_TLV320AIC23 if I2C
>
> This presumably also needs to select the DSP API otherwise it's not
> going to build.

by "DSP API" i'm guessing you mean the Sigma firmware loader

>> +static ssize_t adau1371_dsp_load(struct device *dev,
>> +                               struct device_attribute *attr,
>> +                               const char *buf, size_t count)
>> +{
>> +     int ret = 0;
>> +     struct adau1701_priv *adau1701 = dev_get_drvdata(dev);
>> +     struct snd_soc_codec *codec = adau1701->codec;
>> +     ret = adau1701_setprogram(codec);
>> +     if (ret)
>> +             return ret;
>> +     else
>> +             return count;
>> +}
>> +static DEVICE_ATTR(dsp, 0644, NULL, adau1371_dsp_load);
>
> You're marking the file as supporting both read and write but only
> providing write functionality, and the write functionality totally
> ignores the written data.
>
> More generally this doesn't seem like a great interface - apparently the
> device requries that firmware be loaded but the whole firmware load
> process isn't at all joined up with the driver code meaning that the
> application layer has to figure out when firmware needs to be reloaded,
> there's no understanding on the part of the driver of what the firmware
> on the device is doing or how it's glued into the audio subsystem.

the API should allow for userspace to specify the firmware name, but
that's about it.  it is up to the userspace application to arbitrarily
load different firmware files on the fly into the codec without the
codec caring what's going into it.  often times the firmware is DSP
code to do different post processing on the audio stream (cleanup or
whatever).
-mike
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/