Re: [PATCH 13/13] media: qcom: camss: Add support for VFE hardware version Titan 780

[Depeng Shao]

Hi Bryan,

On 8/15/2024 10:21 PM, Depeng Shao wrote:
> Hi Bryan,
>
> On 8/15/2024 8:25 AM, Bryan O'Donoghue wrote:
> > On 12/08/2024 15:41, Depeng Shao wrote:
> > > +void camss_reg_update(struct camss *camss, int hw_id, int port_id, 
> > > bool is_clear)
> > > +{
> > > +    struct csid_device *csid;
> > > +
> > > +    if (hw_id < camss->res->csid_num) {
> > > +        csid = &(camss->csid[hw_id]);
> > > +
> > > +        csid->res->hw_ops->reg_update(csid, port_id, is_clear);
> > > +    }
> > > +}
> >
> > The naming here doesn't make the action clear
> >
> > hw_ops->rup_update(csid, port, clear);

The register name in SWI is IFE_0_TOP_REG_UPDATE_CMD in SM8250 platform, 
and it is CSID0_RUP_AUP_CMD in SM8550, so it isn't only RUP, and AUP is 
also updated, so maybe the original name reg_update is better. This is 
what VFE 480 driver is using.

> > "is_clear" is not required since the type is a bool the "is" is 
> > implied in the the logical state so just "clear" will do.
> >
> > But re: my previous comment on having the ISR do the clear as is done 
> > in the VFE 480, I don't think this is_clear parameter is warranted.
> >
> > We want the calling function to request the rup_update() for the 
> > rup_update() function to wait on completion and the ISR() to do the 
> > clear once the RUP interrupt has been raised.
> >
> > At least I think that's how it should work - could you please 
> > experiment with your code for the flow - as it appears to match the 
> > VFE 480 logic.
>
> Thanks for catching this, I forget to add the rup irq, so this logic is 
> also missed. I have tried it just now, the logic works good, will add it 
> in next version patch.

I go through the code again, and find we don't do the wait for 
completion in VFE 480 driver, this is just used in VFE gen1 driver and 
just during disabling port.

Here, what I tried is clearing rup_aup when receiving the rup irq.

Thanks,
Depeng