Hi Bryan,
On 8/15/2024 8:09 AM, Bryan O'Donoghue wrote:
+
+/*
+ * vfe_enable_v2 - Enable streaming on VFE line
+ * @line: VFE line
+ *
+ * Return 0 on success or a negative error code otherwise
+ */
+int vfe_enable_v2(struct vfe_line *line)
+{
+ struct vfe_device *vfe = to_vfe(line);
+ int ret;
+
+ mutex_lock(&vfe->stream_lock);
+
+ if (vfe->res->hw_ops->enable_irq)
+ vfe->res->hw_ops->enable_irq(vfe);
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.
Also a style nit-pick if you get a hw_ops pointer you don't have to jump
through so -> many -> indirection -> hoops.
Ok, I will declare the hw_ops first.
Code will look neater that way.
I'll go through the vfe_enable() stuff in more detail on your next drop.
Please ensure again with the hw_version() that you have equivalent logic
before and after => no behaviour change similarly with vfe_enable() and
friends.
The objective is to remove code duplication, not to change logical
behaviors at all, no matter how seemingly trival that change might be ->
hw_version 0xsomenumber instea of 0xX, 0xY 0xZ
It probably sounds dogmatic but, its safer that way.
Sure, I won't change the original code.
Thanks,
Depeng
