Re: [PATCH 11/16] media: qcom: camss: csid: Add v4l2 ctrl if TPG mode isn't disabled

[Depeng Shao]

Hi Bryan,

On 12/11/2024 11:44 PM, Bryan O'Donoghue wrote:
> On 11/12/2024 14:07, Depeng Shao wrote:
> > There is no CSID TPG in some modern HW, so the v4l2 ctrl in CSID driver
>
> "some modern HW" => "on some SoCs"
>
> > shouldn't be registered. Checking the supported TPG modes to indicate
> > if the TPG HW is existing or not, and only register v4l2 ctrl for CSID
>
> "TP HW is existing or not, and only register" => "TPG hardware exists or 
> not and oly registering"
>
> No need to abbreviate hardware to HW.
>
> >   only when TPG HW is existing.
>
> "when the TPG hardware exists" you could also say "is present" instead 
> of "exists".
>
> You have additional whitespace in your log before " only"
>
> >

Thanks for the suggestion, will update in new version.


> >           }
> >           if (!csid->testgen.enabled &&
> > @@ -838,7 +840,8 @@ static void csid_try_format(struct csid_device *csid,
> >           break;
> >       case MSM_CSID_PAD_SRC:
> > -        if (csid->testgen_mode->cur.val == 0) {
> > +        if (csid->testgen.nmodes == CSID_PAYLOAD_MODE_DISABLED ||
>
> if (csid->ctrls ||
>
> feels like a more natural test. Less cumbersome and also less typing.
>
> I think that change should be feasible, could you please update your 
> logic from if (csid->testgen.nmodes == CSID_PAYLOAD_MODE_DISABLED) to if 
> (csid->ctrls)
>
> Otherwise looks good but, I'll wait to see your next version before 
> giving an RB.

csid->ctrls is not a pointer, so it is always true.

Thanks,
Depeng