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 <david@xxxxxxx>
> > >
> > > 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 <david@xxxxxxx>
> > > ---
> > >    .../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

[...]