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

[Bryan O'Donoghue]

On 01/06/2026 19:22, Vijay Kumar Tumati wrote:
> Hi Hangxiang,
>
> On 6/1/2026 8:13 AM, hangxiang.ma@xxxxxxxxxxxxxxxx wrote:
> > On 6/1/26 11:04 PM, Loic Poulain <loic.poulain@xxxxxxxxxxxxxxxx> wrote:
> > > On Mon, Jun 1, 2026 at 4:44 PM Hangxiang Ma
> > > <hangxiang.ma@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxx>
> > > > ---
> > > > 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@xxxxxxxxxxxxxxxx>
> > > >
> > 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