Re: [PATCH v3 2/8] media: qcom: camss: csiphy-3ph: Use odd bits for configuring C-PHY lanes

[David Heidelberg]

On 18/01/2026 13:05, Kieran Bingham wrote:

Quoting Bryan O'Donoghue (2026-01-17 21:38:17)

On 17/01/2026 15:36, David Heidelberg via B4 Relay wrote:

From: David Heidelberg <da...@ixit.cz>

So far, only D-PHY mode was supported, which uses even bits when enabling
or masking lanes. For C-PHY configuration, the hardware instead requires
using the odd bits.

Since there can be unrecognized configuration allow returning failure.

Signed-off-by: David Heidelberg <da...@ixit.cz>
---
   .../platform/qcom/camss/camss-csiphy-2ph-1-0.c     |  8 ++--
   .../platform/qcom/camss/camss-csiphy-3ph-1-0.c     | 49 
+++++++++++++++++-----
   drivers/media/platform/qcom/camss/camss-csiphy.c   |  4 +-
   drivers/media/platform/qcom/camss/camss-csiphy.h   |  6 +--
   4 files changed, 47 insertions(+), 20 deletions(-)

diff --git a/drivers/media/platform/qcom/camss/camss-csiphy-2ph-1-0.c 
b/drivers/media/platform/qcom/camss/camss-csiphy-2ph-1-0.c
index 9d67e7fa6366a..bb4b91f69616b 100644
--- a/drivers/media/platform/qcom/camss/camss-csiphy-2ph-1-0.c
+++ b/drivers/media/platform/qcom/camss/camss-csiphy-2ph-1-0.c
@@ -94,9 +94,9 @@ static u8 csiphy_settle_cnt_calc(s64 link_freq, u32 
timer_clk_rate)
       return settle_cnt;
   }

-static void csiphy_lanes_enable(struct csiphy_device *csiphy,

-                             struct csiphy_config *cfg,
-                             s64 link_freq, u8 lane_mask)
+static int csiphy_lanes_enable(struct csiphy_device *csiphy,
+                            struct csiphy_config *cfg,
+                            s64 link_freq, u8 lane_mask)
   {
       struct csiphy_lanes_cfg *c = &cfg->csi2->lane_cfg;
       u8 settle_cnt;
@@ -132,6 +132,8 @@ static void csiphy_lanes_enable(struct csiphy_device 
*csiphy,
               writel_relaxed(0x3f, csiphy->base +
                              CAMSS_CSI_PHY_INTERRUPT_CLEARn(l));
       }
+
+     return 0;
   }

static void csiphy_lanes_disable(struct csiphy_device *csiphy,

diff --git a/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c 
b/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c
index 4154832745525..f3a8625511e1e 100644
--- a/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c
+++ b/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c
@@ -14,6 +14,7 @@
   #include <linux/delay.h>
   #include <linux/interrupt.h>
   #include <linux/io.h>
+#include <linux/media-bus-format.h>

#define CSIPHY_3PH_LNn_CFG1(n) (0x000 + 0x100 * (n))

   #define CSIPHY_3PH_LNn_CFG1_SWI_REC_DLY_PRG (BIT(7) | BIT(6))
@@ -993,13 +994,22 @@ static void csiphy_gen2_config_lanes(struct csiphy_device 
*csiphy,

static u8 csiphy_get_lane_mask(struct csiphy_lanes_cfg *lane_cfg)

   {
-     u8 lane_mask;
-     int i;
+     u8 lane_mask = 0;

- lane_mask = CSIPHY_3PH_CMN_CSI_COMMON_CTRL5_CLK_ENABLE;

+     switch (lane_cfg->phy_cfg) {
+     case V4L2_MBUS_CSI2_CPHY:
+             for (int i = 0; i < lane_cfg->num_data; i++)
+                     lane_mask |= (1 << lane_cfg->data[i].pos) + 1;

1 << anything == BIT(anything)

I've always disliked the look of this code and now it occurs to me why.

This code is analogous to:

lane_mask |= BIT(lane_cfg->data[i].pos) + 1);

I see that addition to a bit mask and get a little bit scared.

This gives:
   pos   mask
    0    0b00000010 (note 0 bit is zero here but 1 on all others)
    1    0b00000011
    2    0b00000101
    3    0b00001001
    4    0b00010001

Is that expected?

Can data[i].pos ever be position 0 ??

I assume this starts at position 1 - and the +1 here is to always set
the zeroth bit ?

Perhapse this might be precise to convey that in such a case?

   lane_mask |= BIT(pos) | 1;

I guess it depends on what this is really being used for which I don't
have in my context.

If it's relevant, that's how the usage looks like:

https://gitlab.com/sdm845/sdm845-next/-/blob/sdm845-next/arch/arm64/boot/dts/qcom/sdm845-oneplus-common.dtsi#L559

David>

--
Kieran

[...]