On 02/08/2019 00:42, Mark Brown wrote:
On Tue, Jul 30, 2019 at 05:28:11PM +0100, Thomas Preston wrote:
On 30/07/2019 16:50, Mark Brown wrote:
Like I say it's not just debugfs though, there's the standard driver interface too.
Ah right, I understand. So if we run the turn-on diagnostics routine, there's nothing stopping anyone from interacting with the device in other ways.
I guess there's no way to share that mutex with ALSA? In that case, it doesn't matter if this mutex is there or not - this feature is incompatible. How compatible do debugfs interfaces have to be? I was under the impression anything goes. I would argue that the debugfs is better off for having the mutex so that no one re-reads "diagnostic" within the 5s poll timeout.
It's not really something that's supported; like Charles says the DAPM mutex is exposed but if the regular controls would still be able to do stuff. It is kind of a "you broke it, you fix it" thing but on the other hand it's better to make things safer if we can since it might not be obvious later on why things are broken.
Alternatively, this diagnostic feature could be handled with an external-handler kcontrol SOC_SINGLE_EXT? I'm not sure if this is an atomic interface either.
What would be acceptable?
Yes, that's definitely doable - we've got some other drivers with similar things like calibration triggers exposed that way.
One problem with using a kcontrol as a trigger for the turn-on diagnostic is that the diagnostic routine has a "return value".
It goes like this: - Bring device to low-quiescent state - Initiate diagnostics - Poll for diagnostics-complete bit - Read the four channel status registers
The final read clears the status registers, so this isn't something I can just do with regmap.
One idea I had was to initiate the turn-on diagnostics using a kcontrol, whose handler saves the four channel status registers and an epoch in tda7802_priv. Then this can be read from debugfs. But it seems strange to have to turn on this control over here, then go over there and read this value.
Hm, maybe a better idea is to have the turn on diagnostic only run on device probe (as its name suggests!), and print something to dmesg:
modprobe tda7802 turn_on_diagnostic=1
tda7802-codec i2c-TDA7802:00: Turn on diagnostic 04 04 04 04
Kirill Marinushkin mentioned this in the first review [0], it just didn't really sink in until now!