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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <b7877d50-430d-4170-bef4-4dfe1ad8c11d@ixit.cz>
Date: Sun, 18 Jan 2026 13:45:24 +0100
From: David Heidelberg <david@...t.cz>
To: Kieran Bingham <kieran.bingham@...asonboard.com>,
 Bryan O'Donoghue <bryan.odonoghue@...aro.org>,
 Casey Connolly <casey.connolly@...aro.org>, "Dr. Git" <drgitx@...il.com>,
 Luca Weiss <luca.weiss@...rphone.com>,
 Mauro Carvalho Chehab <mchehab@...nel.org>,
 Petr Hodina <phodina@...tonmail.com>, Robert Foss <rfoss@...nel.org>,
 Todor Tomov <todor.too@...il.com>,
 Vladimir Zapolskiy <vladimir.zapolskiy@...aro.org>
Cc: Konrad Dybcio <konrad.dybcio@....qualcomm.com>,
 Joel Selvaraj <foss@...lselvaraj.com>, Kieran Bingham <kbingham@...nel.org>,
 Sakari Ailus <sakari.ailus@...ux.intel.com>, linux-media@...r.kernel.org,
 linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org,
 phone-devel@...r.kernel.org
Subject: Re: [PATCH v3 2/8] media: qcom: camss: csiphy-3ph: Use odd bits for
 configuring C-PHY lanes

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@...t.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 <david@...t.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

[...]