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]

 



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




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux