On 24/08/2024 14:06, Vladimir Zapolskiy wrote:
Right so generally speaking I don't believe we should have any null
function pointers.
We just mandate that to be comitted, an impelmentation must provide a
dummy but, in this case when do we ever want a dummy function anyway
surely enable_irq() is a fundamental operation that is core to the logic.
Why? What could be a justification here?
a) In principle I don't believe we should be adding an interface that
requires an exception in its first go.
b) I don't think its a good idea to proliferate patterns like this
throughout the code
if (vfe->res->hw_ops->enable_irq)
vfe->res->hw_ops->enable_irq();
too->many->indirection->calls();
easily dealt with but "looks wrong"
https://www.joelonsoftware.com/2005/05/11/making-wrong-code-look-wrong/
So if we are adding callbacks, the exception should be stubbing to an
empty function because 9/10 of implementations find the interface useful.
It might be extremely confusing to see in the code that some not ever
requested interrupts are enabled/disabled, and then to discover that just
some stubs around VFE interrupts are added. And it's the case especially
in this new vfe_enable_v2() function, which I believe is intended for
CAMSS support on new platforms.
What's worse, since these VFE interrupts are not needed on the modern
platforms, it will require to add a proposed dummy "return 0" function
into any CAMSS support for new platforms forever. I believe it'd be better
to clearly say that it's a legacy to have an obligatory support of VFE
interrupts.
I think enable_irq();/disable_irq(); should be handled from wm_start()
and wm_stop() for each VFE so that 480 and after can have shared
top-level logic.
However VFE can raise several error cases which TBH we should probably
be capturing somehow, if not in IRQ then in debugfs and/or trace
https://git.codelinaro.org/bryan.odonoghue/kernel/-/commit/cd88d924eb55f5dfeb2283e6e0eef37d5bd4c1c4
@Depeng can you move your enable_irq(); to camss-vfe-480.c::wm_start()
just after we switch on a WM
https://git.codelinaro.org/bryan.odonoghue/kernel/-/blob/2ea8172164e2b12a629cf3d939edac9a0f7a9368/drivers/media/platform/qcom/camss/camss-vfe-480.c#L127
as well as a disable_irq() to camss-vfe-480.c::wm_stop(); @ the top of
the function ?
https://git.codelinaro.org/bryan.odonoghue/kernel/-/blob/2ea8172164e2b12a629cf3d939edac9a0f7a9368/drivers/media/platform/qcom/camss/camss-vfe-480.c#L131
disable_irq() should also collapse the TOP irq when stream_count == 0
The stream_count variable should ensure the TOP irq value only gets
switched on/off once while also removing the special case from your
proposed amalgamated code.
---
bod
