-----Original Message----- From: Mark Brown <broonie@xxxxxxxxxx> Sent: Thursday, February 16, 2023 07:01 To: Guenter Roeck <groeck@xxxxxxxxxx> Cc: David.Rau.opensource <David.Rau.opensource@xxxxxxxxxxxxxx>; support.opensource@xxxxxxxxxxx; lgirdwood@xxxxxxxxx; perex@xxxxxxxx; tiwai@xxxxxxxx; alsa-devel@xxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; bailideng@xxxxxxxxxx; Guenter Roeck <groeck@xxxxxxxxxxxx> Subject: Re: [PATCH] ASoC: da7219: Improve the IRQ process to increase the stability On Wed, Feb 15, 2023 at 08:06:35AM -0800, Guenter Roeck wrote: > On Wed, Feb 15, 2023 at 5:10 AM Mark Brown <broonie@xxxxxxxxxx> wrote: >> > Copying in Guenter given the issues he raised with this, not >> > deleting context for his benefit. It looks like this should avoid >> > the issues with the interrupt appearing locked up. >> It should since it limits the delay to cases where jack_inserted is >> false, but on the other side it hides the delay in an odd way. >> ... >> Effectively this seems to be quite similar to moving the conditional >> sleep to the place where cancel_work_sync() is called. I would assume >> that will fix the problem (after all, the msleep() is no longer called >> unconditionally), but I don't see the benefit of introducing a worker >> to do that. Also, since there is no guarantee that the worker actually >> started by the time cancel_work_sync() is called, I would suspect that >> it may result in unexpected behavior if the worker has not started by >> that time, which I would assume can happen if the system is heavily >> loaded. It also makes the use of the ground switch (i.e., when to set >> and when to drop it) even more of a mystery than it is right now. Here is the scenario of this patch. When receiving the interrupts, da7219_aad_pre_irq_thread will be invoked at first. If it is a Jack plug-in event, the outer task jack_det_work will be created to enable GND switch with the conditional delay. And then returns IRQ_WAKE_THREAD to call da7219_aad_irq_thread. da7219_aad_irq_thread is almost as same as previous one but do cancel_work_sync if jack_det_work is triggered previously. >> Having said that, I don't really know or understand the code, so maybe >> this all makes sense and my feedback should be ignored. Your feedback and clarification are important to improve this driver together. Thanks. >Yes, I would certainly welcome more clarity especially around the ground switch. OTOH it does seem like an improvement over the current situation so I think I'll go ahead and apply it for now, >hopefully it can be improved upon in future. Thanks for the kind support always.
