Messages in this thread | | | Date | Mon, 11 Sep 2023 11:10:47 +0200 | Subject | Re: [PATCH v4 15/17] media: qcom: camss: Move vfe_disable into a common routine where applicable | From | Hans Verkuil <> |
| |
On 08/09/2023 12:36, Bryan O'Donoghue wrote: > On 08/09/2023 11:24, Konrad Dybcio wrote: >> On 8.09.2023 12:21, Bryan O'Donoghue wrote: >>> On 08/09/2023 11:04, Konrad Dybcio wrote: >>>> On 8.09.2023 12:02, Konrad Dybcio wrote: >>>>> On 7.09.2023 18:44, Bryan O'Donoghue wrote: >>>>>> We can move vfe_disable() into a common routine in the core VFE file >>>>>> provided we make wm_stop() a VFE specific callback. >>>>>> >>>>>> The callback is required to capture the case where VFE 17x currently isn't >>>>>> VC enabled where as VFE 480 is. >>>>>> >>>>>> Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>>>>> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> >>>>>> --- >>>>> Acked-by: Konrad Dybcio <konrad.dybcio@linaro.org> >>>>> >>>>> Konrad >>>> Actually there's >>>> >>>> ret = vfe_reset(vfe); >>>> >>>> return ret; >>>> >>>> >>>> which could just be >>>> >>>> return vfe_reset(vfe); >>>> >>>> >>>> Konrad >>> >>> On purpose. >>> >>> I prefer the ret = ; return ret; pattern since it makes it easier / less work to >>> >>> ret = fn(); >>> if (ret) >>> goto error; >>> >>> error: >>> return ret; >> There's no error label in vfe_disable_output >> >> Konrad > > No there is not. Its a pattern I use to make adding jump labels easier later on.
This adds a bunch of extra lines just in case something might happen in the future. That is generally a bad idea, so please change this. As you can see it just causes reviewers to trip over this with exactly the question you got here.
> > Just like you use the pattern of appending "," to aggregate initialisation.
Adding a comma at the end doesn't add extra lines. To be honest, I don't have a strong opinion on this either way. Personally I would probably use a comma if it is likely that the list would be extended in the future, and leave it out if I am pretty certain that won't happen. In any case, I don't mind either way.
Regards,
Hans
> > --- > bod
| |