Re: [PATCH 11/13] media: qcom: camss: Add notify interface in camss driver

From: Depeng Shao
Date: Thu Jul 11 2024 - 07:56:36 EST


Hi Bryan,

On 7/10/2024 7:54 PM, Bryan O'Donoghue wrote:
On 09/07/2024 17:06, Depeng Shao wrote:
The main v4l2 process logic in camss vfe subdev driver, so the vfe driver
handles the buf done irq and register update configuration. But the buf
done irq and register update configuration have been moved CSID HW in
Titan 780 and other new platform, so vfe driver needs to call into CSID
driver to configure the register update. And CSID driver also needs to
call into vfe driver to notify of the buf done irq.

Adding this notify interface in camss structure to do the subdevs cross

  /*
   * csid_isr - CSID module interrupt service routine
   * @irq: Interrupt line
@@ -341,6 +353,14 @@ static irqreturn_t csid_isr(int irq, void *dev)
          if (csid->phy.en_vc & BIT(i)) {
              val = readl_relaxed(csid->base + CSID_CSI2_RDIN_IRQ_STATUS(i));
              writel_relaxed(val, csid->base + CSID_CSI2_RDIN_IRQ_CLEAR(i));
+
+            if (buf_done_val & BIT(BUF_DONE_IRQ_STATUS_RDI_OFFSET + i)) {
+                /* For Titan 780, Buf Done IRQ&REG has been moved to CSID from VFE.
+                 * Once CSID received Buf Done, need notify this event to VFE.
+                 * Trigger VFE to handle Buf Done process.
+                 */
+                csid->camss->notify(&csid->subdev, CAMSS_MSG_BUF_DONE, (void *)&i);

Instead of a generic notify function with an enum passed to a switch lets just have a dedicated function for each functional callback.

csid->camss->reg_update_cmd();
csid->camss->reg_clear_cmd();

We can get rid of a switch and an additional enum that way, plus redundant "event not supported" error checking.


Sure, will update the code based on suggestion.


+            }
          }
      val = 1 << IRQ_CMD_CLEAR;
@@ -434,6 +454,23 @@ static void csid_subdev_init(struct csid_device *csid)
      csid->testgen.nmodes = CSID_PAYLOAD_MODE_NUM_SUPPORTED_GEN2;
  }
+static void csid_subdev_process_msg(struct csid_device *csid, unsigned int msg_type, void *arg)
+{
+    int msg_data = *(int *)arg;
+
+    switch (msg_type) {
+    case CAMSS_MSG_RUP:
+        csid_reg_update(csid, msg_data);
+        break;
+    case CAMSS_MSG_RUP_CLEAR:
+        csid_reg_update_clear(csid, msg_data);
+        break;
+    default:
+        dev_err(csid->camss->dev, "NOT Supported EVT Type\n");
+        break;
+    }
+}
+
  const struct csid_hw_ops csid_ops_gen3 = {
      .configure_stream = csid_configure_stream,
      .configure_testgen_pattern = csid_configure_testgen_pattern,
@@ -442,4 +479,5 @@ const struct csid_hw_ops csid_ops_gen3 = {
      .reset = csid_reset,
      .src_pad_code = csid_src_pad_code,
      .subdev_init = csid_subdev_init,
+    .process_msg = csid_subdev_process_msg,


diff --git a/drivers/media/platform/qcom/camss/camss-vfe.h b/drivers/media/platform/qcom/camss/camss-vfe.h
index 10e2cc3c0b83..a8b09ce9941b 100644
--- a/drivers/media/platform/qcom/camss/camss-vfe.h
+++ b/drivers/media/platform/qcom/camss/camss-vfe.h
@@ -115,6 +115,7 @@ struct vfe_hw_ops {
      int (*vfe_halt)(struct vfe_device *vfe);
      void (*violation_read)(struct vfe_device *vfe);
      void (*vfe_wm_stop)(struct vfe_device *vfe, u8 wm);
+    void (*process_msg)(struct vfe_device *vfe, unsigned int msg_type, void *arg);
  };
  struct vfe_isr_ops {
diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
index 1f1f44f6fbb2..abeb0918e47d 100644
--- a/drivers/media/platform/qcom/camss/camss.c
+++ b/drivers/media/platform/qcom/camss/camss.c
@@ -2202,6 +2202,52 @@ static void camss_genpd_cleanup(struct camss *camss)
      dev_pm_domain_detach(camss->genpd, true);
  }
+static void camss_notify_msg(struct v4l2_subdev *sd,
+            unsigned int msg_type, void *arg)
+{
+    struct v4l2_device *v4l2_dev = sd->v4l2_dev;
+    struct camss *camss = to_camss(v4l2_dev);
+    struct vfe_device *vfe;
+    struct vfe_line *vfe_line;
+    struct csid_device *csid;
+    int evt_data = *(int *)arg;
+
+    switch (msg_type) {
+    case CAMSS_MSG_BUF_DONE:
+        csid = v4l2_get_subdevdata(sd);
+        vfe = &(camss->vfe[csid->id]);
+        if (vfe->res->hw_ops->process_msg)
+            vfe->res->hw_ops->process_msg(vfe,
+                CAMSS_MSG_BUF_DONE, (void *)&evt_data);
+        break;
+
+    case CAMSS_MSG_RUP:
+        vfe_line = v4l2_get_subdevdata(sd);
+        vfe = to_vfe(vfe_line);
+        csid = &(camss->csid[vfe->id]);
+
+        if (csid->res->hw_ops->process_msg)
+            csid->res->hw_ops->process_msg(csid,
+                CAMSS_MSG_RUP, (void *)&evt_data);
+        break;
+
+    case CAMSS_MSG_RUP_CLEAR:
+        vfe_line = v4l2_get_subdevdata(sd);
+        vfe = to_vfe(vfe_line);
+        csid = &(camss->csid[vfe->id]);
+
+        if (csid->res->hw_ops->process_msg)
+            csid->res->hw_ops->process_msg(csid,
+                CAMSS_MSG_RUP_CLEAR, (void *)&evt_data);
+
+        break;
+
+    default:
+        dev_err(camss->dev, "Not supported evt type\n");
+        break;
+    }
+}

Instead of having a central swiss army knife notify() callback these should be CSID or VFE specific callbacks vfe->buf_done() csid->msg_rup() etc.


I will try to add the callback in CSID and VFE driver.

Thanks,
Depeng