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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html