Re: [PATCH 2/3] media: imx258: Register H/V flip controls

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Jacopo,

Thanks for the patch.

On Tue, Jan 17, 2023 at 11:06:02AM +0100, Jacopo Mondi wrote:
> Register controls for V4L2_CID_HFLIP and V4L2_CID_VFLIP.
> 
> The controls are registered as read-only and enabled by default, as the
> driver embeds a 180 degrees rotation in its programming sequences and
> only supports that mode of operations.
> 
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
>  drivers/media/i2c/imx258.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
> index 3b560865b657..2e0a4ea76589 100644
> --- a/drivers/media/i2c/imx258.c
> +++ b/drivers/media/i2c/imx258.c
> @@ -1151,6 +1151,7 @@ static int imx258_init_controls(struct imx258 *imx258)
>  	struct i2c_client *client = v4l2_get_subdevdata(&imx258->sd);
>  	struct v4l2_fwnode_device_properties props;
>  	struct v4l2_ctrl_handler *ctrl_hdlr;
> +	struct v4l2_ctrl *vflip, *hflip;
>  	s64 vblank_def;
>  	s64 vblank_min;
>  	s64 pixel_rate_min;
> @@ -1158,7 +1159,7 @@ static int imx258_init_controls(struct imx258 *imx258)
>  	int ret;
>  
>  	ctrl_hdlr = &imx258->ctrl_handler;
> -	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 11);
> +	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 13);
>  	if (ret)
>  		return ret;
>  
> @@ -1174,6 +1175,17 @@ static int imx258_init_controls(struct imx258 *imx258)
>  	if (imx258->link_freq)
>  		imx258->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>  
> +	/* The driver only supports one bayer order and flips by default. */
> +	hflip = v4l2_ctrl_new_std(ctrl_hdlr, &imx258_ctrl_ops,
> +				  V4L2_CID_HFLIP, 1, 1, 1, 1);
> +	if (hflip)
> +		hflip->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +
> +	vflip = v4l2_ctrl_new_std(ctrl_hdlr, &imx258_ctrl_ops,
> +				  V4L2_CID_VFLIP, 1, 1, 1, 1);

This defaults the controls to 1, which suggests the image is upside down.

The rotation property has been used to tell the driver the sensor is upside
down, and this has also had the effect of reversing flip contron values so
if they're disabled, the image is upright.

See e.g. the CCS driver.

I'd do the same here.

> +	if (vflip)
> +		vflip->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +
>  	pixel_rate_max = link_freq_to_pixel_rate(link_freq_menu_items[0]);
>  	pixel_rate_min = link_freq_to_pixel_rate(link_freq_menu_items[1]);
>  	/* By default, PIXEL_RATE is read only */

-- 
Kind regards,

Sakari Ailus