Re: [PATCH 09/13] media: qcom: camss: vfe: Move common code into vfe core

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux