[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <dcc315cb-c787-462e-b61e-20f0e5d43d8f@quicinc.com>
Date: Mon, 25 Mar 2024 21:16:26 +0800
From: Depeng Shao <quic_depengs@...cinc.com>
To: Bryan O'Donoghue <bryan.odonoghue@...aro.org>, <rfoss@...nel.org>,
<todor.too@...il.com>, <andersson@...nel.org>,
<konrad.dybcio@...aro.org>, <mchehab@...nel.org>,
<quic_yon@...cinc.com>
CC: <linux-kernel@...r.kernel.org>, <linux-media@...r.kernel.org>,
<linux-arm-msm@...r.kernel.org>
Subject: Re: [PATCH v2 8/8] media: qcom: camss: Add sm8550 support
Hi Bryan,
On 3/21/2024 12:01 AM, Bryan O'Donoghue wrote:
> On 20/03/2024 14:11, Depeng Shao wrote:
>> Add in functional logic throughout the code to support the SM8550.
>>
>> Signed-off-by: Depeng Shao <quic_depengs@...cinc.com>
>> ---
>> .../media/platform/qcom/camss/camss-csid.c | 19 +++++++++++++++++++
>> .../media/platform/qcom/camss/camss-csiphy.c | 1 +
>> drivers/media/platform/qcom/camss/camss-vfe.c | 7 +++++++
>> .../media/platform/qcom/camss/camss-video.c | 1 +
>> 4 files changed, 28 insertions(+)
>>
>> diff --git a/drivers/media/platform/qcom/camss/camss-csid.c
>> b/drivers/media/platform/qcom/camss/camss-csid.c
>> index eb27d69e89a1..e9203dc15798 100644
>> --- a/drivers/media/platform/qcom/camss/camss-csid.c
>> +++ b/drivers/media/platform/qcom/camss/camss-csid.c
>> @@ -590,6 +590,25 @@ int msm_csid_subdev_init(struct camss *camss,
>> struct csid_device *csid,
>> csid->base = camss->vfe[id].base +
>> VFE_480_LITE_CSID_OFFSET;
>> else
>> csid->base = camss->vfe[id].base + VFE_480_CSID_OFFSET;
>> + } else if (camss->res->version == CAMSS_8550) {
>> + /* for titan 780, CSID lite registers are inside the VFE lite
>> region,
>> + * between the VFE "top" and "bus" registers. this requires
>> + * VFE to be initialized before CSID
>> + */
>> + if (id >= 2)
>> + csid->base = camss->vfe[id].base;
>
> Hard-coded magic numbers are definitely out.
>
> If you need to differentiate - please include something in the struct
> resources so that the flag is always available and we don't have to
> start doing funky magic index/magic number gymnastics.
>
Sure, will update this part.
>> + else {
>> + csid->base =
>> + devm_platform_ioremap_resource_byname(pdev,
>> res->reg[0]);
>> + if (id != 0)
>> + csid->top_base = camss->csid[0].top_base;
>> + else
>> + csid->top_base =
>> + devm_platform_ioremap_resource_byname(pdev,
>> res->reg[1]);
>> + }
>
> What is the point of hooking the TOP base just to clear our the status
> registers ?
>
> We take no meaningful action in the ISR that I can see.
>
The csid top is a new small hardware block, it controls the output of
CSID, since the CSID can connect to Sensor Front End(SFE) or original
VFE in CSID gen3.
>
>> +
>> + if (IS_ERR(csid->base))
>> + return PTR_ERR(csid->base);
>> } else {
>> csid->base = devm_platform_ioremap_resource_byname(pdev,
>> res->reg[0]);
>> if (IS_ERR(csid->base))
>> diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.c
>> b/drivers/media/platform/qcom/camss/camss-csiphy.c
>> index 45b3a8e5dea4..f35af0dd2147 100644
>> --- a/drivers/media/platform/qcom/camss/camss-csiphy.c
>> +++ b/drivers/media/platform/qcom/camss/camss-csiphy.c
>> @@ -579,6 +579,7 @@ int msm_csiphy_subdev_init(struct camss *camss,
>> case CAMSS_845:
>> case CAMSS_8250:
>> case CAMSS_8280XP:
>> + case CAMSS_8550:
>> csiphy->formats = csiphy_formats_sdm845;
>> csiphy->nformats = ARRAY_SIZE(csiphy_formats_sdm845);
>> break;
>> diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c
>> b/drivers/media/platform/qcom/camss/camss-vfe.c
>> index d875237cf244..ff115c5521c6 100644
>> --- a/drivers/media/platform/qcom/camss/camss-vfe.c
>> +++ b/drivers/media/platform/qcom/camss/camss-vfe.c
>> @@ -226,6 +226,7 @@ static u32 vfe_src_pad_code(struct vfe_line *line,
>> u32 sink_code,
>> case CAMSS_845:
>> case CAMSS_8250:
>> case CAMSS_8280XP:
>> + case CAMSS_8550:
>> switch (sink_code) {
>> case MEDIA_BUS_FMT_YUYV8_1X16:
>> {
>> @@ -296,6 +297,10 @@ int vfe_reset(struct vfe_device *vfe)
>> reinit_completion(&vfe->reset_complete);
>> + // The reset has been moved to csid in 8550
>
> Please run checkpatch.pl on your code before submission C++ are not
> allowed.
>
> ---
> bod
Thanks,
Depeng
Powered by blists - more mailing lists