[alsa-devel] [Sound-open-firmware] [PATCH 01/21] ASoC: SOF: Intel: Add BYT, CHT and BSW DSP HW support.

[Pierre-Louis Bossart]

>> +struct snd_sof_dsp_ops sof_cht_ops = {
>> +       /* device init */
>> +       .probe          = byt_probe,
>> +
>> +       /* DSP core boot / reset */
>> +       .run            = byt_run,
>> +       .reset          = byt_reset,
>> +
>> +       /* Register IO */
>> +       .write          = sof_io_write,
>> +       .read           = sof_io_read,
>> +       .write64        = sof_io_write64,
>> +       .read64         = sof_io_read64,
>> +
>> +       /* Block IO */
>> +       .block_read     = sof_block_read,
>> +       .block_write    = sof_block_write,
>> +
>> +       /* doorbell */
>> +       .irq_handler    = byt_irq_handler,
>> +       .irq_thread     = byt_irq_thread,
>> +
> What is the reason for having irq_handler/irq_thread functions inside the
> snd_sof_dsp_ops structure?
>
> These functions are never used outside via sdev->ops pointer.

Good point indeed, thanks for raising it. We were in the middle of 
tagging which ops are required/optional (feedback from Mark) but we 
started from the core and should have looked at the structure definition.

Most drivers are "self-contained" and can reference the irq_thread and 
irq_handler directly.

The exception where the abstraction is used is internal to the HDaudio 
stuff:

intel/hda.c:    ret = request_threaded_irq(sdev->ipc_irq, 
hda_dsp_ipc_irq_handler,
intel/hda.c: sof_ops(sdev)->irq_thread, IRQF_SHARED,

That's useful since there a minor variations between hardware 
generations and you want to hide the hardware-specific parts.

But as you point out, it's a "private" use of ops callbacks, the core 
doesn't touch this.

I have no explanation other than legacy/historical reasons or a shortcut 
to make one's life easier during development.  Liam and Keyon might know?

We could try and move this to a more "private" structure, the 
"chip_info" part might be more suitable for this?