Re: [PATCH 12/13] media: qcom: camss: Add CSID Gen3 support for sm8550

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

 



On 15/08/2024 16:14, Depeng Shao wrote:
Hi Bryan,


---
  drivers/media/platform/qcom/camss/Makefile    |   1 +
  .../platform/qcom/camss/camss-csid-gen3.c     | 339 ++++++++++++++++++
  .../platform/qcom/camss/camss-csid-gen3.h     |  26 ++


So this "gen2" and "gen3" stuff would make sense if we had a number of SoCs based on gen2 and gen3 which were controlled from the upper-level gen2.c and gen3.c.

What you're submitting here is csid-780 so the file should be named csid-780.

When we add 680 or 880 then it makes sense to try to encapsulate a class of generation into one file - potentially.

I'd guess that was the intent behind gen2.c.

TL;DR please name your file csid-xxx.c

Sure, I will use csid-780.c

+
+    writel(val, csid->base + CSID_CSI2_RX_CFG0);
+
+    val = 1 << CSI2_RX_CFG1_ECC_CORRECTION_EN;
+    if (vc > 3)
+        val |= 1 << CSI2_RX_CFG1_VC_MODE;

So again these are needless bit-shifts.

#define CSI2_RX_CFG1_PACKET_ECC_CORRECTION_EN BIT(0)

val = CSI2_RX_CFG1_PACKET_ECC_CORRECTION_EN;


You posted same comments in v3 series, I also replied it.
https://lore.kernel.org/all/eeaf4f4e-5200-4b13-b38f-3f3385fc2a2b@xxxxxxxxxxx/

Some of register bits which just need to be configured to 0 or 1, then can use BIT(X), but some register bits need to configure a specific value, e.g.,  CSID_RDI_CFG0 bits[22:26] need to configure a vc vaule, bits[16:21] need to configure a dt value, then we can't use BIT(x) to handle this.

Yes please use macros() to bury any _necessary_ bit shifts to populate a _bit_field_ away but as an example CSI2_RX_CFG1_PACKET_ECC_CORRECTION_EN is not a bit-field.




      case MSM_CSID_PAD_SRC:
-        if (csid->testgen_mode->cur.val == 0) {
+        if (!csid->testgen_mode || csid->testgen_mode->cur.val == 0) {

See my comments on adding new guards to core functionality.

Is this sm8550 specific or generic ?


It is sm8550 specific, since we don't have testgen mode in sm8550 csid, so need to add some guards, the guards are added for similar reason.

Hmm, I see in my tree I just assigned testgen_mode to some dummy data. You're right, retain this, when we enable testgen as a standalone entity outside of CSID we can address this again.


              /* Test generator is disabled, */
              /* keep pad formats in sync */
              u32 code = fmt->code;
@@ -1042,6 +1042,7 @@ static int csid_init_formats(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
  static int csid_set_test_pattern(struct csid_device *csid, s32 value)
  {
      struct csid_testgen_config *tg = &csid->testgen;
+    const struct csid_hw_ops *hw_ops = csid->res->hw_ops;
      /* If CSID is linked to CSIPHY, do not allow to enable test generator */       if (value && media_pad_remote_pad_first(&csid- >pads[MSM_CSID_PAD_SINK])) @@ -1049,7 +1050,10 @@ static int csid_set_test_pattern(struct csid_device *csid, s32 value)
      tg->enabled = !!value;
-    return csid->res->hw_ops->configure_testgen_pattern(csid, value);
+    if (hw_ops->configure_testgen_pattern)
+        return -EOPNOTSUPP;
+    else
+        return hw_ops->configure_testgen_pattern(csid, value);

If you just add a dummy configure_testgen_pattern we can get rid of this branching stuff.


Do you mean add dummy function in csid-780/gen3.c? How about the other ops in vfe_ops_780, add dummy function or use NULL? We need to guards if we set it as NULL.

See above, you're right what you have is fine.


static int csid_configure_testgen_pattern(struct csid_device *csid, s32 val)
{
     return 0;
}

  }
  /*
@@ -1121,6 +1125,19 @@ int msm_csid_subdev_init(struct camss *camss, struct csid_device *csid,           csid->base = devm_platform_ioremap_resource_byname(pdev, res->reg[0]);
          if (IS_ERR(csid->base))
              return PTR_ERR(csid->base);
+
+        /* CSID "top" is a new function in new version HW,
+         * CSID can connect to VFE & SFE(Sensor Front End).
+         * this connection is controlled by CSID "top" registers.
+         * There is only one CSID "top" region for all CSIDs.
+         */
+        if (!csid_is_lite(csid) && res->reg[1] && !camss- >csid_top_base) {
+            camss->csid_top_base =
+                devm_platform_ioremap_resource_byname(pdev, res- >reg[1]);

That's a complex clause.

Let me send you a patch to do it a different way.


I was also thinking to addd it in camss level, then I thought it is in csid block, so I moved it to csid, but it is also fine to add it in camss. Can I add your patch into this series? Just like the csiphy patches.

static const struct resources_wrapper csid_wrapper_res_sm8550 = {
        .reg = "csid_wrapper",
};

Yes go ahead, all you should need to do then is add "&csid_wrapper_res_sm8550" to your resources.

static const struct camss_resources sm8550_resources = {
        .version = CAMSS_SM8550,
        .pd_name = "top",
        .csiphy_res = csiphy_res_sm8550,
        .csid_res = csid_res_sm8550,
        .ispif_res = NULL,
        .vfe_res = vfe_res_sm8550,
        .csid_wrapper_res = &csid_wrapper_res_sm8550,
...
};

---
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