Re: [PATCH v2 1/4] media: qcom: camss: vfe: Add VBIF setting support

[Bryan O'Donoghue]

On 26/05/2025 17:23, Vincent Knecht wrote:

Do you mean to just rename to vfe_vbif_write_reg()


Yep, its more natural language.

---
bod

Re: [PATCH v2 1/4] media: qcom: camss: vfe: Add VBIF setting support

[Bryan O'Donoghue]

On 26/05/2025 17:20, Vincent Knecht wrote:

You have both

if (vfe->res->has_vbif) {

and the above switch, there's no point in checking this twice in two
different ways.

Choose one, suggest has_vbif is enough.

I think the switch is still needed, so that distinct settings
can be applied for different SoCs.

Or should I just apply the 8939 settings unconditionally, and keep it
as an exercice for the next dev who need other settings for another SoC
to deal with ? 🙂

Thanks


The flag should be enough.

---
bod

Re: [PATCH v2 1/4] media: qcom: camss: vfe: Add VBIF setting support

[Vincent Knecht]

Le lundi 26 mai 2025 à 09:13 +0100, Bryan O'Donoghue a écrit :
> On 25/05/2025 20:25, Vincent Knecht via B4 Relay wrote:
> > +void vfe_vbif_reg_write(struct vfe_device *vfe, u32 reg, u32 val);
> 
> write_reg() / read_reg()
> 
> ---
> bod

Do you mean to just rename to vfe_vbif_write_reg(),
or that I should also add a getter function ?

Re: [PATCH v2 1/4] media: qcom: camss: vfe: Add VBIF setting support

[Bryan O'Donoghue]

On 25/05/2025 20:25, Vincent Knecht via B4 Relay wrote:

From: Vincent Knecht 

Some devices need writing values to VFE VBIF registers.
Add helper functions to do this.

Signed-off-by: Vincent Knecht 
---
  drivers/media/platform/qcom/camss/Makefile |  1 +
  drivers/media/platform/qcom/camss/camss-vfe-4-1.c  | 11 
  drivers/media/platform/qcom/camss/camss-vfe-vbif.c | 30 ++
  drivers/media/platform/qcom/camss/camss-vfe-vbif.h | 19 ++
  drivers/media/platform/qcom/camss/camss-vfe.c  |  9 +++
  drivers/media/platform/qcom/camss/camss-vfe.h  |  3 +++
  6 files changed, 73 insertions(+)

diff --git a/drivers/media/platform/qcom/camss/Makefile 
b/drivers/media/platform/qcom/camss/Makefile
index 
d26a9c24a430a831e0d865db4d96142da5276653..4c66d29ae505ae5adc717ae98f77fb736a6e15b9
 100644
--- a/drivers/media/platform/qcom/camss/Makefile
+++ b/drivers/media/platform/qcom/camss/Makefile
@@ -21,6 +21,7 @@ qcom-camss-objs += \
camss-vfe-680.o \
camss-vfe-780.o \
camss-vfe-gen1.o \
+   camss-vfe-vbif.o \
camss-vfe.o \
camss-video.o \
camss-format.o \
diff --git a/drivers/media/platform/qcom/camss/camss-vfe-4-1.c 
b/drivers/media/platform/qcom/camss/camss-vfe-4-1.c
index 
901677293d971cf761944a660ef719af38203f22..520bc16f2a05e34f457a828ecdb1f9502a0470c1
 100644
--- a/drivers/media/platform/qcom/camss/camss-vfe-4-1.c
+++ b/drivers/media/platform/qcom/camss/camss-vfe-4-1.c
@@ -15,6 +15,7 @@
  #include "camss.h"
  #include "camss-vfe.h"
  #include "camss-vfe-gen1.h"
+#include "camss-vfe-vbif.h"
  
  #define VFE_0_HW_VERSION		0x000
  
@@ -733,6 +734,7 @@ static void vfe_set_qos(struct vfe_device *vfe)

  {
u32 val = VFE_0_BUS_BDG_QOS_CFG_0_CFG;
u32 val7 = VFE_0_BUS_BDG_QOS_CFG_7_CFG;
+   int ret;
  
  	writel_relaxed(val, vfe->base + VFE_0_BUS_BDG_QOS_CFG_0);

writel_relaxed(val, vfe->base + VFE_0_BUS_BDG_QOS_CFG_1);
@@ -742,6 +744,15 @@ static void vfe_set_qos(struct vfe_device *vfe)
writel_relaxed(val, vfe->base + VFE_0_BUS_BDG_QOS_CFG_5);
writel_relaxed(val, vfe->base + VFE_0_BUS_BDG_QOS_CFG_6);
writel_relaxed(val7, vfe->base + VFE_0_BUS_BDG_QOS_CFG_7);
+
+   /* SoC-specific VBIF settings */
+   if (vfe->res->has_vbif) {
+   ret = vfe_vbif_apply_settings(vfe);
+   if (ret < 0)
+   dev_err_ratelimited(vfe->camss->dev,
+   "VFE: VBIF error %d\n",
+   ret);


Over multiple lines encapsulate with {}


+   }
  }
  
  static void vfe_set_ds(struct vfe_device *vfe)

diff --git a/drivers/media/platform/qcom/camss/camss-vfe-vbif.c 
b/drivers/media/platform/qcom/camss/camss-vfe-vbif.c
new file mode 100644
index 
..2ae61b7baa148f0ad63fe3b8751aeb7b8fc12d81
--- /dev/null
+++ b/drivers/media/platform/qcom/camss/camss-vfe-vbif.c
@@ -0,0 +1,30 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * camss-vfe-vbif.c
+ *
+ * Qualcomm MSM Camera Subsystem - VFE VBIF Module
+ *
+ * Copyright (c) 2025, The Linux Foundation. All rights reserved.
+ *
+ */
+
+#include 
+
+#include "camss.h"
+#include "camss-vfe.h"
+#include "camss-vfe-vbif.h"
+
+void vfe_vbif_reg_write(struct vfe_device *vfe, u32 reg, u32 val)
+{
+   writel_relaxed(val, vfe->vbif_base + reg);
+}
+
+int vfe_vbif_apply_settings(struct vfe_device *vfe)
+{
+   switch (vfe->camss->res->version) {
+   default:
+   break;
+   }


You have both

if (vfe->res->has_vbif) {

and the above switch, there's no point in checking this twice in two 
different ways.


Choose one, suggest has_vbif is enough.


+
+   return 0;
+}
diff --git a/drivers/media/platform/qcom/camss/camss-vfe-vbif.h 
b/drivers/media/platform/qcom/camss/camss-vfe-vbif.h
new file mode 100644
index 
..cef1e17dba1f2335a2c8de070bcb6afde98eef87
--- /dev/null
+++ b/drivers/media/platform/qcom/camss/camss-vfe-vbif.h
@@ -0,0 +1,19 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * camss-vfe-vbif.h
+ *
+ * Qualcomm MSM Camera Subsystem - VFE VBIF Module
+ *
+ * Copyright (c) 2025, The Linux Foundation. All rights reserved.
+ *
+ */
+#ifndef QC_MSM_CAMSS_VFE_VBIF_H
+#define QC_MSM_CAMSS_VFE_VBIF_H
+
+#include "camss-vfe.h"
+
+void vfe_vbif_reg_write(struct vfe_device *vfe, u32 reg, u32 val);
+
+int vfe_vbif_apply_settings(struct vfe_device *vfe);
+
+#endif /* QC_MSM_CAMSS_VFE_VBIF_H */
diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c 
b/drivers/media/platform/qcom/camss/camss-vfe.c
index 
4bca6c3abaff9b898ea879674a3ff8f3592d3139..3138562d399444c5cf2ae96bf16b75b85ff5c5ca
 100644
--- a/drivers/media/platform/qcom/camss/camss-vfe.c
+++ b/drivers/media/platform/qcom/camss/camss-vfe.c
@@ -1807,6 +1807,15 @@ int msm_vfe_subdev_init(struct camss *camss, struct 
vfe_de

Re: [PATCH v2 1/4] media: qcom: camss: vfe: Add VBIF setting support

[Bryan O'Donoghue]

On 25/05/2025 20:25, Vincent Knecht via B4 Relay wrote:

+void vfe_vbif_reg_write(struct vfe_device *vfe, u32 reg, u32 val);


write_reg() / read_reg()

---
bod