Re: [RFC PATCH v5 13/14] media: tegra-video: Add CSI MIPI pads calibration

From: Dmitry Osipenko
Date: Wed Jul 29 2020 - 20:53:11 EST


30.07.2020 03:55, Sowjanya Komatineni ÐÐÑÐÑ:
>
> On 7/29/20 5:52 PM, Sowjanya Komatineni wrote:
>>
>> On 7/29/20 5:43 PM, Dmitry Osipenko wrote:
>>> 30.07.2020 03:27, Sowjanya Komatineni ÐÐÑÐÑ:
>>> ...
>>>>>> Secondly, perhaps a failed calibration isn't a very critical error?
>>>>>> Hence just printing a warning message should be enough.
>>>>> Using dev_err to report calibration failure. Are you suggesting to use
>>>>> dev_warn instead of dev_err?
>>> I meant that failing s_stream might be unnecessary.
>>>
>>> The dev_warn should be more appropriate for a non-critical errors.
>>>
>>>>>> Could you please make a patch that factors all ON/OFF code paths
>>>>>> into a
>>>>>> separate functions? It's a bit difficult to follow the combined code,
>>>>>> especially partial changes in the patches. Thanks in advance!
>>>>> what do you mean by partial changes in patches?
>>>>>
>>>>> Can you please be more clear?
>>>> Also please specify what ON/OFF code paths you are referring to when
>>>> you
>>>> say to move into separate functions?
>>> I meant to change all the code like this:
>>>
>>> set(on) {
>>> ÂÂÂÂ if (on) {
>>> ÂÂÂÂÂÂÂ ...
>>> ÂÂÂÂÂÂÂ return;
>>> ÂÂÂÂ }
>>>
>>> ÂÂÂÂ if (!on)
>>> ÂÂÂÂÂÂ ...
>>>
>>> ÂÂÂÂ return;
>>> }
>>>
>>> to somewhat like this:
>>>
>>> set(on) {
>>> ÂÂÂÂ if (on)
>>> ÂÂÂÂÂÂ ret = enable();
>>> ÂÂÂÂ else
>>> ÂÂÂÂÂÂ ret = disable();
>>>
>>> ÂÂÂÂ return ret;
>>> }
>>
>> You mean to change tegra_channel_set_stream() ?

Yes, and tegra_csi_s_stream().

> changing tegra_channel_set_stream() to have like below will have
> redundant calls as most of the code b/w enable and disable is same
> except calling them in reverse order based on on/off and doing MIPI
> calibration only during ON
>
>
> if (on)
> ÂÂÂ ret = enable()
> else
> ÂÂÂ ret = disable()
> return ret;

Readability should be more important than number of lines.