[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <be0b5f96-2863-4b10-b003-6829dfb04b95@linaro.org>
Date: Thu, 15 Aug 2024 17:10:06 +0100
From: Bryan O'Donoghue <bryan.odonoghue@...aro.org>
To: Depeng Shao <quic_depengs@...cinc.com>, rfoss@...nel.org,
todor.too@...il.com, mchehab@...nel.org, robh@...nel.org,
krzk+dt@...nel.org, conor+dt@...nel.org
Cc: linux-arm-msm@...r.kernel.org, linux-media@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
kernel@...cinc.com, Yongsheng Li <quic_yon@...cinc.com>
Subject: Re: [PATCH 12/13] media: qcom: camss: Add CSID Gen3 support for
sm8550
On 15/08/2024 16:14, Depeng Shao wrote:
> Hi Bryan,
>
>
>>> ---
>>> drivers/media/platform/qcom/camss/Makefile | 1 +
>>> .../platform/qcom/camss/camss-csid-gen3.c | 339 ++++++++++++++++++
>>> .../platform/qcom/camss/camss-csid-gen3.h | 26 ++
>>
>>
>> So this "gen2" and "gen3" stuff would make sense if we had a number of
>> SoCs based on gen2 and gen3 which were controlled from the upper-level
>> gen2.c and gen3.c.
>>
>> What you're submitting here is csid-780 so the file should be named
>> csid-780.
>>
>> When we add 680 or 880 then it makes sense to try to encapsulate a
>> class of generation into one file - potentially.
>>
>> I'd guess that was the intent behind gen2.c.
>>
>> TL;DR please name your file csid-xxx.c
>
> Sure, I will use csid-780.c
>
>>> +
>>> + writel(val, csid->base + CSID_CSI2_RX_CFG0);
>>> +
>>> + val = 1 << CSI2_RX_CFG1_ECC_CORRECTION_EN;
>>> + if (vc > 3)
>>> + val |= 1 << CSI2_RX_CFG1_VC_MODE;
>>
>> So again these are needless bit-shifts.
>>
>> #define CSI2_RX_CFG1_PACKET_ECC_CORRECTION_EN BIT(0)
>>
>> val = CSI2_RX_CFG1_PACKET_ECC_CORRECTION_EN;
>>
>
> You posted same comments in v3 series, I also replied it.
> https://lore.kernel.org/all/eeaf4f4e-5200-4b13-b38f-3f3385fc2a2b@quicinc.com/
>
> Some of register bits which just need to be configured to 0 or 1, then
> can use BIT(X), but some register bits need to configure a specific
> value, e.g., CSID_RDI_CFG0 bits[22:26] need to configure a vc vaule,
> bits[16:21] need to configure a dt value, then we can't use BIT(x) to
> handle this.
Yes please use macros() to bury any _necessary_ bit shifts to populate a
_bit_field_ away but as an example CSI2_RX_CFG1_PACKET_ECC_CORRECTION_EN
is not a bit-field.
>
>
>>> case MSM_CSID_PAD_SRC:
>>> - if (csid->testgen_mode->cur.val == 0) {
>>> + if (!csid->testgen_mode || csid->testgen_mode->cur.val == 0) {
>>
>> See my comments on adding new guards to core functionality.
>>
>> Is this sm8550 specific or generic ?
>>
>
> It is sm8550 specific, since we don't have testgen mode in sm8550 csid,
> so need to add some guards, the guards are added for similar reason.
Hmm, I see in my tree I just assigned testgen_mode to some dummy data.
You're right, retain this, when we enable testgen as a standalone entity
outside of CSID we can address this again.
>
>>> /* Test generator is disabled, */
>>> /* keep pad formats in sync */
>>> u32 code = fmt->code;
>>> @@ -1042,6 +1042,7 @@ static int csid_init_formats(struct v4l2_subdev
>>> *sd, struct v4l2_subdev_fh *fh)
>>> static int csid_set_test_pattern(struct csid_device *csid, s32 value)
>>> {
>>> struct csid_testgen_config *tg = &csid->testgen;
>>> + const struct csid_hw_ops *hw_ops = csid->res->hw_ops;
>>> /* If CSID is linked to CSIPHY, do not allow to enable test
>>> generator */
>>> if (value && media_pad_remote_pad_first(&csid-
>>> >pads[MSM_CSID_PAD_SINK]))
>>> @@ -1049,7 +1050,10 @@ static int csid_set_test_pattern(struct
>>> csid_device *csid, s32 value)
>>> tg->enabled = !!value;
>>> - return csid->res->hw_ops->configure_testgen_pattern(csid, value);
>>> + if (hw_ops->configure_testgen_pattern)
>>> + return -EOPNOTSUPP;
>>> + else
>>> + return hw_ops->configure_testgen_pattern(csid, value);
>>
>> If you just add a dummy configure_testgen_pattern we can get rid of
>> this branching stuff.
>>
>
> Do you mean add dummy function in csid-780/gen3.c? How about the other
> ops in vfe_ops_780, add dummy function or use NULL? We need to guards if
> we set it as NULL.
See above, you're right what you have is fine.
>
> static int csid_configure_testgen_pattern(struct csid_device *csid, s32
> val)
> {
> return 0;
> }
>
>>> }
>>> /*
>>> @@ -1121,6 +1125,19 @@ int msm_csid_subdev_init(struct camss *camss,
>>> struct csid_device *csid,
>>> csid->base = devm_platform_ioremap_resource_byname(pdev,
>>> res->reg[0]);
>>> if (IS_ERR(csid->base))
>>> return PTR_ERR(csid->base);
>>> +
>>> + /* CSID "top" is a new function in new version HW,
>>> + * CSID can connect to VFE & SFE(Sensor Front End).
>>> + * this connection is controlled by CSID "top" registers.
>>> + * There is only one CSID "top" region for all CSIDs.
>>> + */
>>> + if (!csid_is_lite(csid) && res->reg[1] && !camss-
>>> >csid_top_base) {
>>> + camss->csid_top_base =
>>> + devm_platform_ioremap_resource_byname(pdev, res-
>>> >reg[1]);
>>
>> That's a complex clause.
>>
>> Let me send you a patch to do it a different way.
>>
>
> I was also thinking to addd it in camss level, then I thought it is in
> csid block, so I moved it to csid, but it is also fine to add it in
> camss. Can I add your patch into this series? Just like the csiphy patches.
static const struct resources_wrapper csid_wrapper_res_sm8550 = {
.reg = "csid_wrapper",
};
Yes go ahead, all you should need to do then is add
"&csid_wrapper_res_sm8550" to your resources.
static const struct camss_resources sm8550_resources = {
.version = CAMSS_SM8550,
.pd_name = "top",
.csiphy_res = csiphy_res_sm8550,
.csid_res = csid_res_sm8550,
.ispif_res = NULL,
.vfe_res = vfe_res_sm8550,
.csid_wrapper_res = &csid_wrapper_res_sm8550,
...
};
---
bod
Powered by blists - more mailing lists