Hi Andy,
Thank you for the reviews.
On 9/2/24 2:03 PM, Andy Shevchenko wrote:
> On Mon, Sep 02, 2024 at 11:52:29AM +0200, Hans de Goede wrote:
>> The atomisp firmware contains a number of different pipeline binaries
>> inside its firmware file and the driver selects the right one depending
>> on the selected pipeline configuration.
>>
>> Sometimes (e.g. when the selected output resolution is too big) it fails
>> to find a binary. This happens especially when adding support for new
>> sensors.
>>
>> Improve the logging when this happens to make debugging easier:
>>
>> 1. Replace ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE, ...) with standard
>> dev_dbg() calls so that the logs can be enabled with dyndbg
>>
>> 2. Do not dump_stack() when this fails, doing so adds no useful extra
>> info
>>
>> 3. With the dump_stack() call gone, remove the wrapper and rename
>> __ia_css_binary_find() to ia_css_binary_find()
>>
>> 4. On error use dev_err() instead of dev_dbg() so that when things
>> fail it is clear why they fail without needing to enable dyndbg
>>
>> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
>> ---
>> Note the log messages could also do with a bit of rewording and
>> reflowing them to put more arguments on a single line to use less
>> lines. I consider that out of scope for this patch which already
>> enough (see the 1-4 enough) something like this should be done
>> in a follow-up patch IMHO.
>
> Yes, but...
>
> ...
>
>> -static int __ia_css_binary_find(struct ia_css_binary_descr *descr,
>> - struct ia_css_binary *binary) {
>> +int ia_css_binary_find(struct ia_css_binary_descr *descr,
>> + struct ia_css_binary *binary)
>
> ...for example, in this case you touched both lines, so making them a single
> line just reduces the future churn.
Ack, fixed in my media-atomisp branch.
>
> ...
>
>> - ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE,
>> - "ia_css_binary_find() enter: descr=%p, (mode=%d), binary=%p\n",
>> - descr, descr->mode,
>> - binary);
>> + dev_dbg(atomisp_dev,
>> + "ia_css_binary_find() enter: descr=%p, (mode=%d), binary=%p\n",
>
>> + descr, descr->mode,
>> + binary);
>
> So does this.
Ack, fixed in my media-atomisp branch.
>
> ...
>
>> /* print a map of the binary file */
>> - ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE, "BINARY INFO:\n");
>> + dev_dbg(atomisp_dev, "BINARY INFO:\n");
>
> TAB instead of space.
Ack, fixed in my media-atomisp branch.
> ...
>
>> - ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE,
>> - "ia_css_binary_find() [%d] continue: !%d && %d && (%d != %d)\n",
>> - __LINE__, candidate->enable.continuous,
>> - continuous, mode,
>> - IA_CSS_BINARY_MODE_COPY);
>> + dev_dbg(atomisp_dev,
>> + "ia_css_binary_find() [%d] continue: !%d && %d && (%d != %d)\n",
>> + __LINE__, candidate->enable.continuous,
>
>> + continuous, mode,
>> + IA_CSS_BINARY_MODE_COPY);
>
> Now one line?
Ack, fixed in my media-atomisp branch.
> ...
>
>> - ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE,
>> - "ia_css_binary_find() [%d] continue: binary is not striped\n",
>> - __LINE__);
>> + dev_dbg(atomisp_dev,
>> + "ia_css_binary_find() [%d] continue: binary is not striped\n",
>> + __LINE__);
>
> Now the __LINE__ argument is redundant as one may run-time turn it on and off.
> So, trimming it while converting to dev_dbg() makes sense to me as a one
> logical change. Ditto for other __LINE__ cases.
Dropping __LINE__ is the bit which I want to postpone to a follow-up patch,
the messages rejecting certain binaries as not suitable are not very
unique and the __LINE__ print is necessary (atm) to help find which check
exactly is failing. Not ideal I know, something for the TODO list.
> Otherwise it's a good clean up!
Thanks.
Regards,
Hans
