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
| ||
|
Date: Tue, 26 Apr 2016 11:22:40 -0500 From: Andreas Dannenberg <dannenberg@...com> To: Mark Brown <broonie@...nel.org> CC: <alsa-devel@...a-project.org>, <devicetree@...r.kernel.org>, Liam Girdwood <lgirdwood@...il.com>, Jaroslav Kysela <perex@...ex.cz>, Takashi Iwai <tiwai@...e.com>, Rob Herring <robh+dt@...nel.org>, Pawel Moll <pawel.moll@....com>, Mark Rutland <mark.rutland@....com>, Ian Campbell <ijc+devicetree@...lion.org.uk>, Kumar Gala <galak@...eaurora.org>, <linux-kernel@...r.kernel.org> Subject: Re: [PATCH v3 2/2] ASoC: codecs: add support for TAS5720 digital amplifier Hi Mark, thanks for the continued feedback, please see below... On Tue, Apr 26, 2016 at 04:43:13PM +0100, Mark Brown wrote: > On Mon, Apr 25, 2016 at 03:17:36PM -0500, Andreas Dannenberg wrote: > > This looks mostly good, a few small things below. > > > + switch (width) { > > + case 16: > > + case 18: > > + case 20: > > + case 24: > > + /* > > + * We only support the different left-justified serial audio > > + * formats in which case there is nothing to configure in the > > + * TAS5720. > > + */ > > + break; > > + default: > > + dev_err(codec->dev, "unsupported sample size: %d\n", width); > > + return -EINVAL; > > + } > > If the driver doesn't do anything just remove the code. Well it's doing something which is making sure the nobody passes in a sample size that's not supported. Wouldn't we want to catch this? > > > + if ((curr_fault & TAS5720_OCE) && !(tas5720->last_fault & TAS5720_OCE)) > > + dev_warn(dev, "The Class-D output stage has experienced an over current event\n"); > > "Class D over current". The verbosity is making the line over long and > the phrasing is a bit unclear (and makes it seem less critical than it > really is). These should probably be dev_crit() or somthing too, over > current and similar events on a speaker output are generally extremely > serious. The overlong line goes through checkpatch --strict and looks like an accepted practice to prevent breaking "grep" for example. The text is more or less from the datasheet to give people something they can cross-associate. But I can try to short this a bit. You made a good point with the dev_crit(), I can certainly promote this over current condition. It happens for example if I drive too big of a load (a heavy-duty jumper wire in my case, simulating a short of the output stage). If so, we should also consider the other two fault conditions. For the "over temp" error condition (which is actually really hard to create on the bench, I've to get the EVM up to like 150C and things start smelling a bit) this should probably be dev_crit() as well. And then, maybe leave the "DC error" as a warning, since it's less critical than the other two conditions. Thoughts? Regards, Andreas
Powered by blists - more mailing lists