Re: [PATCH 14/14] [media] fix warning on v4l2_subdev_call() result interpreted as bool

[Hans Verkuil]

On 14/07/17 11:36, Arnd Bergmann wrote:
> v4l2_subdev_call is a macro returning whatever the callback return
> type is, usually 'int'. With gcc-7 and ccache, this can lead to
> many wanings like:
> 
> media/platform/pxa_camera.c: In function 'pxa_mbus_build_fmts_xlate':
> media/platform/pxa_camera.c:766:27: error: ?: using integer constants in boolean context [-Werror=int-in-bool-context]
>   while (!v4l2_subdev_call(subdev, pad, enum_mbus_code, NULL, &code)) {
> media/atomisp/pci/atomisp2/atomisp_cmd.c: In function 'atomisp_s_ae_window':
> media/atomisp/pci/atomisp2/atomisp_cmd.c:6414:52: error: ?: using integer constants in boolean context [-Werror=int-in-bool-context]
>   if (v4l2_subdev_call(isp->inputs[asd->input_curr].camera,
> 
> The best workaround I could come up with is to change all the
> callers that use the return code from v4l2_subdev_call() in an
> 'if' or 'while' condition.
> 
> In case of simple 'if' checks, adding a temporary variable is
> usually ok, and sometimes this can be used to propagate or
> print an error code, so I do that.
> 
> For the 'while' loops, I ended up adding an otherwise useless
> comparison with zero, which unfortunately makes the code a little
> uglied.
> 
> Signed-off-by: Arnd Bergmann <arnd@xxxxxxxx>
> ---
>  drivers/media/pci/cx18/cx18-ioctl.c                      |  6 ++++--
>  drivers/media/pci/saa7146/mxb.c                          |  5 +++--
>  drivers/media/platform/atmel/atmel-isc.c                 |  4 ++--
>  drivers/media/platform/atmel/atmel-isi.c                 |  4 ++--
>  drivers/media/platform/blackfin/bfin_capture.c           |  4 ++--
>  drivers/media/platform/omap3isp/ispccdc.c                |  5 +++--
>  drivers/media/platform/pxa_camera.c                      |  3 ++-
>  drivers/media/platform/rcar-vin/rcar-core.c              |  2 +-
>  drivers/media/platform/rcar-vin/rcar-dma.c               |  4 +++-
>  drivers/media/platform/soc_camera/soc_camera.c           |  4 ++--
>  drivers/media/platform/stm32/stm32-dcmi.c                |  4 ++--
>  drivers/media/platform/ti-vpe/cal.c                      |  6 ++++--
>  drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c | 13 +++++++------
>  13 files changed, 37 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/media/pci/cx18/cx18-ioctl.c b/drivers/media/pci/cx18/cx18-ioctl.c
> index 80b902b12a78..1803f28fc501 100644
> --- a/drivers/media/pci/cx18/cx18-ioctl.c
> +++ b/drivers/media/pci/cx18/cx18-ioctl.c
> @@ -188,6 +188,7 @@ static int cx18_g_fmt_sliced_vbi_cap(struct file *file, void *fh,
>  {
>  	struct cx18 *cx = fh2id(fh)->cx;
>  	struct v4l2_sliced_vbi_format *vbifmt = &fmt->fmt.sliced;
> +	int ret;
>  
>  	/* sane, V4L2 spec compliant, defaults */
>  	vbifmt->reserved[0] = 0;
> @@ -201,8 +202,9 @@ static int cx18_g_fmt_sliced_vbi_cap(struct file *file, void *fh,
>  	 * digitizer/slicer.  Note, cx18_av_vbi() wipes the passed in
>  	 * fmt->fmt.sliced under valid calling conditions
>  	 */
> -	if (v4l2_subdev_call(cx->sd_av, vbi, g_sliced_fmt, &fmt->fmt.sliced))
> -		return -EINVAL;
> +	ret = v4l2_subdev_call(cx->sd_av, vbi, g_sliced_fmt, &fmt->fmt.sliced);
> +	if (ret)
> +		return ret;

Please keep the -EINVAL here. I can't be 100% certain that returning 'ret' wouldn't
break something.

>  
>  	vbifmt->service_set = cx18_get_service_set(vbifmt);
>  	return 0;

<snip>

> diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
> index 97093baf28ac..fe56a037f065 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
> @@ -6405,19 +6405,20 @@ int atomisp_s_ae_window(struct atomisp_sub_device *asd,
>  	struct atomisp_device *isp = asd->isp;
>  	/* Coverity CID 298071 - initialzize struct */
>  	struct v4l2_subdev_selection sel = { 0 };
> +	int ret;
>  
>  	sel.r.left = arg->x_left;
>  	sel.r.top = arg->y_top;
>  	sel.r.width = arg->x_right - arg->x_left + 1;
>  	sel.r.height = arg->y_bottom - arg->y_top + 1;
>  
> -	if (v4l2_subdev_call(isp->inputs[asd->input_curr].camera,
> -			     pad, set_selection, NULL, &sel)) {
> -		dev_err(isp->dev, "failed to call sensor set_selection.\n");
> -		return -EINVAL;
> -	}
> +	ret = v4l2_subdev_call(isp->inputs[asd->input_curr].camera,
> +			       pad, set_selection, NULL, &sel);
> +	if (ret)
> +		dev_err(isp->dev, "failed to call sensor set_selection: %d\n",
> +			ret);

Same here: just keep the 'return -EINVAL'.

>  
> -	return 0;
> +	return ret;
>  }
>  
>  int atomisp_flash_enable(struct atomisp_sub_device *asd, int num_frames)
> 

This is all very hackish, though. I'm not terribly keen on this patch. It's not
clear to me *why* these warnings appear in your setup.

Regards,

	Hans