On Tue, May 03, 2016 at 05:20:00PM +0800, John Hsu wrote:
> On 5/3/2016 12:27 AM, Mark Brown wrote:
> > On Fri, Apr 29, 2016 at 04:15:17PM +0800, John Hsu wrote:

> > > +	/* Mask all interruptions except jack insertion interruption */
> > > +	regmap_write(regmap, NAU8825_REG_INTERRUPT_DIS_CTRL, 0xfffe);

> > So if any other interrupts occur then things will break...

> The codec only has headset output and its function works when headset is
> connected. When headset is not connected, the driver only permit insertion
> interruption to happen. After insertion, the internal clock of codec turns
> on and all other interruptions just will be enabled. Without clock, the
> codec can detect jack insertion only.

People do surprising things with devices - they may not wire up the
headphone detection for some reason, or may connect some external
circuit.

> > This is ignoring the attempt to set up a clock but returning success
> > which is going to break things, printing the warning is dubious (a
> > system could be built without detection for example, or a speaker driver
> > connected) but probably OK in itself but the fact that we don't tell the
> > caller may make things worse.

> For clear expression, we should print error message and return error to
> caller. Is it right?

It'd be better to just accept the configuration but what you suggest is
less bad than just completely ignoring the problem.

> > > +		nau8825_eject_jack(nau8825);
> > > +		snd_soc_jack_report(nau8825->jack, 0, SND_JACK_HEADSET);
> > > +	}
> > > +	enable_irq(nau8825->irq);

> > The interrupt is optional (that bug appears to be already present in the
> > driver but should be fixed).

> If headset ejection happened when system suspend. After resume, the codec
> won't detect the change and no report to the kernel. The application does
> not aware the device change and still outputs stream to the headset device.
> For the issue, the driver has to notice the application of ejection ever
> happened in suspend. Let the application to change its device correctly.

This does not address the issue at all.  The interrupt is optional, it
may not have been wired up and the probe function handles that case
gracefully.