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@xxxxxxxxxxxxxxxx> >>>>>> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@xxxxxxxxxx> >>>>>> --- >>>>> Acked-by: Konrad Dybcio <konrad.dybcio@xxxxxxxxxx> >>>>> >>>>> 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
