Re: [PATCH 09/13] media: qcom: camss: vfe: Move common code into vfe core

[Depeng Shao <quic_depengs@quicinc.com>]

Hi Bryan,

On 8/15/2024 8:09 AM, Bryan O'Donoghue wrote:

>> +
>> +/*
>> + * vfe_enable_v2 - Enable streaming on VFE line
>> + * @line: VFE line
>> + *
>> + * Return 0 on success or a negative error code otherwise
>> + */
>> +int vfe_enable_v2(struct vfe_line *line)
>> +{
>> +    struct vfe_device *vfe = to_vfe(line);
>> +    int ret;
>> +
>> +    mutex_lock(&vfe->stream_lock);
>> +
>> +    if (vfe->res->hw_ops->enable_irq)
>> +        vfe->res->hw_ops->enable_irq(vfe);
> 
> Right so generally speaking I don't believe we should have any null 
> function pointers.
> 
> We just mandate that to be comitted, an impelmentation must provide a 
> dummy but, in this case when do we ever want a dummy function anyway 
> surely enable_irq() is a fundamental operation that is core to the logic.
> 
> Also a style nit-pick if you get a hw_ops pointer you don't have to jump 
> through so -> many -> indirection -> hoops.
> 

Ok, I will declare the hw_ops first.

> Code will look neater that way.
> 
> I'll go through the vfe_enable() stuff in more detail on your next drop.
> 
> Please ensure again with the hw_version() that you have equivalent logic 
> before and after => no behaviour change similarly with vfe_enable() and 
> friends.
> 
> The objective is to remove code duplication, not to change logical 
> behaviors at all, no matter how seemingly trival that change might be -> 
> hw_version 0xsomenumber instea of 0xX, 0xY 0xZ
> 
> It probably sounds dogmatic but, its safer that way.
> 

Sure, I won't change the original code.

Thanks,
Depeng