From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f48.google.com (mail-wm1-f48.google.com [209.85.128.48]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 9DB793806C4 for ; Mon, 1 Jun 2026 20:29:16 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.128.48 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1780345759; cv=none; b=dxMzC9C7rRT3hA5Qfa/OQyUl5haTXkf2L8khUOjOyMinIa1at7rfJ7ubOwTMDl6Mmyoywb/lMQEbjP4uaij9dhzxH5aO2Spa72Iq1SChsmqV41CKEh56lE2dCIobPaTWE7tvL3JUAPvSUGtEbqqhmYgIjkPoTcujNjQDICyq7Is= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1780345759; c=relaxed/simple; bh=fqT6LZZWvvuCiVxgMDtlsXYgUKBFR0XwxahTM1RHgnQ=; h=Message-ID:Date:MIME-Version:Subject:To:References:From: In-Reply-To:Content-Type; b=S9hRwJ6dyHf1ZJbR8SbJbSa/+gdaGePZGf3w3rI8C/kCyvKBxHk7aPbd71HJ9wud0pBJD+ufADCx3TdLbAI2+qYVaPO2AR5kspklSdMrE/WA808fpgXfS4uLQOoyv062RgI/64+XphWfvVDfLH54SV2PXF2gwUZDei2bzIDUkJU= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linaro.org; spf=pass smtp.mailfrom=linaro.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b=jSX1zweN; arc=none smtp.client-ip=209.85.128.48 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linaro.org Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b="jSX1zweN" Received: by mail-wm1-f48.google.com with SMTP id 5b1f17b1804b1-490426d72f7so92199795e9.3 for ; Mon, 01 Jun 2026 13:29:16 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1780345755; x=1780950555; darn=vger.kernel.org; h=content-transfer-encoding:in-reply-to:content-language:from :references:to:subject:user-agent:mime-version:date:message-id:from :to:cc:subject:date:message-id:reply-to; bh=FuBG5thkiczhmSMU1mytIPUE9n1pNCrGrOz79kkF6lo=; b=jSX1zweNlgE4yP4XzxPGGUA5cVUVYGI9HeZQWYmZEI1ehBOEE/lqpmPOJvKhx4ypeU 3mc79TQkbNIQ87JboSL343OlZ8nLCOt4yNR1lFKb0/77DN8j14oKjSjI2knTtxyMXe18 8rsXaDIg+g0pI04Hmy9C9Tr5hjX1705expWHbCYLKHzE8SrI0d2gHKR+sIATd7+wSJHi 4yA1h8EEOVirGi1KJbYQXy9Xn68fZ3eStRbeEt47UJBOp8AbSkp7mlTsa3pNFhvP88FD X3xPyKYqT9/1+yQKbQLYxQbbSVI+CYMYiP+8QJMPEu2+utGG49pTVFx6dqgEpBUY8FNZ GYTw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1780345755; x=1780950555; h=content-transfer-encoding:in-reply-to:content-language:from :references:to:subject:user-agent:mime-version:date:message-id :x-gm-gg:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=FuBG5thkiczhmSMU1mytIPUE9n1pNCrGrOz79kkF6lo=; b=VOK4DIc/YwGDwN704uVWrmGLnQntpKE5WgW7o1S9b87BkbmL0QUvmwwxm50qRON7gj 2s1xKlTkc6N6gFy142rLK/GseHAvWq5djTXB+DkHAkZKKHYUCFrv+8Nk0Qto/NJMeTVD OYTMz+9Hfy1N5BPag1kf+e4KFecAMNsT0awciOJ4we+PCnqpv5aDgo9XDZLEINBsSwIX UBo7RC64UinDzXmKUBQlR+ryvPwhs3djbN7sqpirD6opikd4GP3xajJUTNelMB4uM0jE Vc+OOLNEXH8iGfmv/fMOuE8XyhiunU7eFLuhJJ0NwtdDU7faqakt9XcFK9JGBneVON07 YJgA== X-Forwarded-Encrypted: i=1; AFNElJ+2JfibdGXu56UPNTr2arJ+rPtW3PGxsjkGlYNwv3SiCUT1kDL3aMdg/2ZKRhOLFtc8WYt6KoXjmt7DCoY=@vger.kernel.org X-Gm-Message-State: AOJu0YwqcCLPvRyTTEXv+6wm0APSXlB5K+8xEb8rQ7w942x5vznYlLZB p8dOILxs5HbAKXCRSZgBk3GWlFfnmiPPd7p3UyDB6qxFIiIbfvwKAuMWO0xnqPPxoGs= X-Gm-Gg: Acq92OEFBDzuZJXcOhc6M7oAm2Vaw8ac68eA0VgG2oXdOI9PBTKePdt3O/1ULRS+f4T DMZ2B8KJNIkeGZ9/JRvKLn/ZeVXIUZsJPcxH6PFvKzdXQBOBhGksoXuEq70q/gLivAKGNJxMRaF 6y3JfSeq4jhu0KJ/DrNHIrBaAgzyCCDuOFArS6cuPRlGBQYoSI+sfhI/THepGsLtDfehi1wwowH eQk8B69o/tXIUbbY+9cMaDYiTGKYo22bI1d5M7obfafyBOKZXmJWLQTpWrAHBwVtd6AHZUA37Of b9C5qFGWTIKVFhGfXPveAn2MtAk1+cCFkcoCy28yKvF5HNwDpM21zn9Y/kpchrO4bsfaHRVLllC R/OtXwFIAOAOmi8Ti/MwWURzak0JNknjD5Rtslr63vagSC43OrTk3x/m9CP9rQnI5pWT0wXgX+G mA4HrGdxOYoAJkInVSL7rdxg4X9CKWCE13JcU2GDkO0TlG7A== X-Received: by 2002:a05:600c:1c0a:b0:490:5074:651e with SMTP id 5b1f17b1804b1-490a2940746mr237991485e9.25.1780345755113; Mon, 01 Jun 2026 13:29:15 -0700 (PDT) Received: from [192.168.0.101] ([109.76.233.76]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-490b18cf3f9sm1530325e9.21.2026.06.01.13.29.14 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 01 Jun 2026 13:29:14 -0700 (PDT) Message-ID: <2768617d-e031-441b-be05-a6c8efa6615c@linaro.org> Date: Mon, 1 Jun 2026 21:29:13 +0100 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] media: qcom: camss: csid: Consolidate CSI2_RX_CFG0_PHY_SEL_BASE_IDX definition To: Vijay Kumar Tumati , hangxiang.ma@oss.qualcomm.com, Loic Poulain , Robert Foss , Todor Tomov , Vladimir Zapolskiy , Mauro Carvalho Chehab , linux-media@vger.kernel.org, linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org References: <20260601-camss-macro-v1-1-cabf1fb99241@oss.qualcomm.com> <215786e4-e85c-4af0-9993-5c6331c87817@oss.qualcomm.com> From: Bryan O'Donoghue Content-Language: en-US In-Reply-To: <215786e4-e85c-4af0-9993-5c6331c87817@oss.qualcomm.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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 wrote: >>> On Mon, Jun 1, 2026 at 4:44 PM Hangxiang Ma >>> 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 >>> > --- >>> > 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 >>> > >>> >> 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