lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ff261ab4-b59d-48a1-9ede-3c691842d913@quicinc.com>
Date: Thu, 15 Aug 2024 23:14:18 +0800
From: Depeng Shao <quic_depengs@...cinc.com>
To: Bryan O'Donoghue <bryan.odonoghue@...aro.org>, <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

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.


>> +
>> +static void csid_subdev_reg_update(struct csid_device *csid, int 
>> port_id, bool is_clear)
>> +{
>> +    if (is_clear) {
>> +        csid->reg_update &= ~REG_UPDATE_RDI(csid, port_id);
>> +    } else {
>> +        csid->reg_update |= REG_UPDATE_RDI(csid, port_id);
>> +        writel(csid->reg_update, csid->base + CSID_REG_UPDATE_CMD);
>> +    }
>> +}
> 
> Right so this function should
> 
> 1. Write the register
> 2. Wait on a completion
>     See camss-vfe-480.c::vfe_isr_reg_update()
> 3. Have that completion fire in the CSID ISR
> 4. Or timeout
> 5. Returning either 0 for success or -ETIMEDOUT
> 
> to the calling function so that we can be sure the RUP interrupt has 
> fired and completed - or we have appropriately timed out and captured 
> the failure.
> 
> Also - in camss-vfe-480.c the ISR clears the RUP which one assumes is 
> still the required logical flow with the RUP now residing in CSID.
> 

Sure, I forget to add this, will add them in next series.


>>       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.

>>               /* 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.

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.


Thanks,
Depeng


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ