Re: [PATCH] media: qcom: camss: csid: Consolidate CSI2_RX_CFG0_PHY_SEL_BASE_IDX definition

[Bryan O'Donoghue <bryan.odonoghue@linaro.org>]

On 01/06/2026 19:22, Vijay Kumar Tumati wrote:
> Hi Hangxiang,
> 
> On 6/1/2026 8:13 AM, hangxiang.ma@oss.qualcomm.com wrote:
>> On 6/1/26 11:04 PM, Loic Poulain <loic.poulain@oss.qualcomm.com> wrote:
>>> On Mon, Jun 1, 2026 at 4:44 PM Hangxiang Ma
>>> <hangxiang.ma@oss.qualcomm.com> wrote:
>>> >
>>> > Move the duplicate CSI2_RX_CFG0_PHY_SEL_BASE_IDX definition from
>>> > camss-csid-680.c and camss-csid-gen3.c into the shared camss-csid.h
>>> > header. This eliminates redundancy and makes the constant available
>>> > to future CSID implementations.
>>>
>>> Taking that direction, I don’t think this is the only instance of
>>> redundancy, so why single out this one in particular? Should we
>>> consider one-line cleanups across all similar cases? Also, other CSID
>>> drivers follow the same pattern but use different identifiers for that
>>> define (e.g. csid-340).
>>>
>>> Also, introducing such low-level, register-aligned naming
>>> (CSI2_RX_CFG0_PHY...)  in what is supposed to be a generic
>>> CSID header doesn’t seem appropriate.
>>>
>>> Regards,
>>> Loic
>>>
>>>
>>>
>>> >
>>> > Signed-off-by: Hangxiang Ma <hangxiang.ma@oss.qualcomm.com>
>>> > ---
>>> > Move the duplicate CSI2_RX_CFG0_PHY_SEL_BASE_IDX definition from
>>> > camss-csid-680.c and camss-csid-gen3.c into the shared camss-csid.h
>>> > header. This eliminates redundancy and makes the constant available
>>> > to future CSID implementations.
>>> > ---
>>> >   drivers/media/platform/qcom/camss/camss-csid-680.c  | 1 -
>>> >   drivers/media/platform/qcom/camss/camss-csid-gen3.c | 1 -
>>> >   drivers/media/platform/qcom/camss/camss-csid.h      | 2 ++
>>> >   3 files changed, 2 insertions(+), 2 deletions(-)
>>> >
>>> > diff --git a/drivers/media/platform/qcom/camss/camss-csid-680.c b/ 
>>> drivers/media/platform/qcom/camss/camss-csid-680.c
>>> > index 345a67c8fb94..bf7164085ddb 100644
>>> > --- a/drivers/media/platform/qcom/camss/camss-csid-680.c
>>> > +++ b/drivers/media/platform/qcom/camss/camss-csid-680.c
>>> > @@ -101,7 +101,6 @@
>>> >   #define CSI2_RX_CFG0_DL2_INPUT_SEL                      12
>>> >   #define CSI2_RX_CFG0_DL3_INPUT_SEL                      16
>>> >   #define CSI2_RX_CFG0_PHY_NUM_SEL                        20
>>> > -#define CSI2_RX_CFG0_PHY_SEL_BASE_IDX                   1
>>> >   #define CSI2_RX_CFG0_PHY_TYPE_SEL                       24
>>> >   #define CSI2_RX_CFG0_TPG_MUX_EN                         BIT(27)
>>> >   #define CSI2_RX_CFG0_TPG_MUX_SEL                        
>>> GENMASK(29, 28)
>>> > diff --git a/drivers/media/platform/qcom/camss/camss-csid-gen3.c b/ 
>>> drivers/media/platform/qcom/camss/camss-csid-gen3.c
>>> > index 0fdbf75fb27d..da9458cd178b 100644
>>> > --- a/drivers/media/platform/qcom/camss/camss-csid-gen3.c
>>> > +++ b/drivers/media/platform/qcom/camss/camss-csid-gen3.c
>>> > @@ -105,7 +105,6 @@
>>> >   #define CSID_RDI_IRQ_SUBSAMPLE_PERIOD(rdi)     
>>> (csid_is_lite(csid) && IS_CSID_690(csid) ?\
>>> >                                                          (0x34C + 
>>> 0x100 * (rdi)) :\
>>> >                                                          (0x54C + 
>>> 0x100 * (rdi)))
>>> > -#define CSI2_RX_CFG0_PHY_SEL_BASE_IDX  1
>>> >
>>> >   static void __csid_configure_rx(struct csid_device *csid,
>>> >                                  struct csid_phy_config *phy, int vc)
>>> > diff --git a/drivers/media/platform/qcom/camss/camss-csid.h b/ 
>>> drivers/media/platform/qcom/camss/camss-csid.h
>>> > index 5296b10f6bac..059ac94ad1be 100644
>>> > --- a/drivers/media/platform/qcom/camss/camss-csid.h
>>> > +++ b/drivers/media/platform/qcom/camss/camss-csid.h
>>> > @@ -27,6 +27,8 @@
>>> >   /* CSID hardware can demultiplex up to 4 outputs */
>>> >   #define MSM_CSID_MAX_SRC_STREAMS       4
>>> >
>>> > +/* CSIPHY to hardware PHY selector mapping */
>>> > +#define CSI2_RX_CFG0_PHY_SEL_BASE_IDX 1
>>> >   #define CSID_RESET_TIMEOUT_MS 500
>>> >
>>> >   enum csid_testgen_mode {
>>> >
>>> > ---
>>> > base-commit: 697a0e31ee66f5ddb929c09895139779fff33f20
>>> > change-id: 20260601-camss-macro-3d40c4d4e90d
>>> >
>>> > Best regards,
>>> > --
>>> > Hangxiang Ma <hangxiang.ma@oss.qualcomm.com>
>>> >
>>>
>> Thanks Loic, Bryan pointed this out in last review cycle and suggested 
>> to split it as a standalone series. This idea comes from KNP series as 
>> I was once suggested to move this macro into one common header to 
>> remove redundancy. I think your are correct after fully consideration. 
>> I will make changes only for KNP and put it in driver.
>>
>> Best regards,
>> Hangxiang
>>
> So this patch is not necessary any more, is it? If there is anything 
> specifically to be done to withdraw this, just for it to be clear to 
> Bryan, can you please?
> 
> Thanks,
> Vijay.
> 

I don't like endlessly redefining the same things over and over again. 
Its wasteful and error prone.

Burying it in camss-csid yes I agree is not appropriate - however for 
specific classes of silicon - it is entirely appropriate.

That surely is the entire point of naming things gen2, gen3, gen4 etc 
instead of silicon-version-x

Hence camss-csid-genX.h for sharing generation specific things.

---
bod