[libcamera-devel] [PATCH 03/10] libcamera: ipu3: Fold mediaBusToFormat() into only caller

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Tue, Jun 02, 2020 at 03:39:02AM +0200, Niklas Söderlund wrote:
> Make the code easier to read and refactor.

I'm not necessarily opposed to this, but given that mapping between
media bus formats and V4L2 pixel formats on video nodes is an operation
that most pipeline handlers need to perform, and that it is
device-specific, would it make sense to try and keep it in separate
functions that would have the same name in all pipeline handlers ? We
could even create an static const std::map for that, and remove the
function.

If you think the code is better with this patch, I'm fine with it.

> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 38 ++++++++++++++--------------
>  1 file changed, 19 insertions(+), 19 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 6df1e29281941ebf..f7363244e1d2d0ff 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -133,8 +133,6 @@ public:
>  	int start();
>  	int stop();
>  
> -	static V4L2PixelFormat mediaBusToFormat(unsigned int code);
> -
>  	V4L2VideoDevice *output_;
>  	V4L2Subdevice *csi2_;
>  	CameraSensor *sensor_;
> @@ -1504,7 +1502,25 @@ int CIO2Device::configure(const Size &size,
>  	if (ret)
>  		return ret;
>  
> -	outputFormat->fourcc = mediaBusToFormat(sensorFormat.mbus_code);
> +	V4L2PixelFormat v4l2Format;
> +	switch (sensorFormat.mbus_code) {
> +	case MEDIA_BUS_FMT_SBGGR10_1X10:
> +		v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SBGGR10);
> +		break;
> +	case MEDIA_BUS_FMT_SGBRG10_1X10:
> +		v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGBRG10);
> +		break;
> +	case MEDIA_BUS_FMT_SGRBG10_1X10:
> +		v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGRBG10);
> +		break;
> +	case MEDIA_BUS_FMT_SRGGB10_1X10:
> +		v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SRGGB10);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	outputFormat->fourcc = v4l2Format;
>  	outputFormat->size = sensorFormat.size;
>  	outputFormat->planesCount = 1;
>  
> @@ -1578,22 +1594,6 @@ int CIO2Device::stop()
>  	return output_->streamOff();
>  }
>  
> -V4L2PixelFormat CIO2Device::mediaBusToFormat(unsigned int code)
> -{
> -	switch (code) {
> -	case MEDIA_BUS_FMT_SBGGR10_1X10:
> -		return V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SBGGR10);
> -	case MEDIA_BUS_FMT_SGBRG10_1X10:
> -		return V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGBRG10);
> -	case MEDIA_BUS_FMT_SGRBG10_1X10:
> -		return V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGRBG10);
> -	case MEDIA_BUS_FMT_SRGGB10_1X10:
> -		return V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SRGGB10);
> -	default:
> -		return {};
> -	}
> -}
> -
>  REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3);
>  
>  } /* namespace libcamera */

-- 
Regards,

Laurent Pinchart