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.
...
> - 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.
...
> /* 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.
...
> - 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?
...
> - 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.
...
Otherwise it's a good clean up!
--
With Best Regards,
Andy Shevchenko
