Re: [PATCH v2] media: atmel: atmel-isc-base: expose white balance as v4l2 controls

[Hans Verkuil]

On 11/19/19 11:03 AM, Eugen.Hristev@xxxxxxxxxxxxx wrote:
> From: Eugen Hristev <eugen.hristev@xxxxxxxxxxxxx>
> 
> This exposes the white balance configuration of the ISC as v4l2 controls
> into userspace.
> There are 8 controls available:
> 4 gain controls , sliders, for each of the BAYER components: R, B, GR, GB.
> These gains are multipliers for each component, in format unsigned 0:4:9 with
> a default value of 512 (1.0 multiplier).
> 4 offset controls, sliders, for each of the BAYER components: R, B, GR, GB.
> These offsets are added/substracted from each component, in format signed
> 1:12:0 with a default value of 0 (+/- 0)
> 
> To expose this to userspace, added 8 custom controls, in an auto cluster.
> 
> To summarize the functionality:
> The auto cluster switch is the auto white balance control, and it works
> like this:
> AWB ==1 : autowhitebalance is on, the do_white_balance button is inactive,
> the gains/offsets are inactive, but volatile and readable.
> Thus, the results of the whitebalance algorithm are available to userspace to
> read at any time.
> AWB ==0: autowhitebalance is off, cluster is in manual mode, user can configure
> the gain/offsets directly. More than that, if the do_white_balance button is
> pressed, the driver will perform one-time-adjustment, (preferably with color
> checker card) and the userspace can read again the new values.
> 
> With this feature, the userspace can save the coefficients and reinstall them
> for example after reboot or reprobing the driver.
> 
> Signed-off-by: Eugen Hristev <eugen.hristev@xxxxxxxxxxxxx>
> ---

<snip>

> diff --git a/drivers/media/platform/atmel/atmel-isc.h b/drivers/media/platform/atmel/atmel-isc.h
> index bfaed2f..23adb4e 100644
> --- a/drivers/media/platform/atmel/atmel-isc.h
> +++ b/drivers/media/platform/atmel/atmel-isc.h
> @@ -213,7 +213,6 @@ struct isc_device {
>  	struct fmt_config	try_config;
>  
>  	struct isc_ctrls	ctrls;
> -	struct v4l2_ctrl	*do_wb_ctrl;
>  	struct work_struct	awb_work;
>  
>  	struct mutex		lock; /* serialize access to file operations */
> @@ -223,6 +222,28 @@ struct isc_device {
>  
>  	struct isc_subdev_entity	*current_subdev;
>  	struct list_head		subdev_entities;
> +
> +	struct {
> +#define ISC_CTRL_DO_WB 1
> +#define ISC_CTRL_R_BAL 2
> +#define ISC_CTRL_B_BAL 3
> +#define ISC_CTRL_GR_BAL 4
> +#define ISC_CTRL_GB_BAL 5
> +#define ISC_CTRL_R_OFF 6
> +#define ISC_CTRL_B_OFF 7
> +#define ISC_CTRL_GR_OFF 8
> +#define ISC_CTRL_GB_OFF 9
> +		struct v4l2_ctrl	*awb_ctrl;
> +		struct v4l2_ctrl	*do_wb_ctrl;
> +		struct v4l2_ctrl	*r_bal_ctrl;
> +		struct v4l2_ctrl	*b_bal_ctrl;
> +		struct v4l2_ctrl	*gr_bal_ctrl;
> +		struct v4l2_ctrl	*gb_bal_ctrl;
> +		struct v4l2_ctrl	*r_off_ctrl;
> +		struct v4l2_ctrl	*b_off_ctrl;
> +		struct v4l2_ctrl	*gr_off_ctrl;
> +		struct v4l2_ctrl	*gb_off_ctrl;
> +	};
>  };
>  
>  #define GAMMA_MAX	2
> @@ -242,4 +263,13 @@ int isc_clk_init(struct isc_device *isc);
>  void isc_subdev_cleanup(struct isc_device *isc);
>  void isc_clk_cleanup(struct isc_device *isc);
>  
> +#define ISC_CID_R_BAL			(V4L2_CID_CAMERA_CLASS_BASE + 1000)
> +#define ISC_CID_B_BAL			(V4L2_CID_CAMERA_CLASS_BASE + 1001)
> +#define ISC_CID_GR_BAL			(V4L2_CID_CAMERA_CLASS_BASE + 1002)
> +#define ISC_CID_GB_BAL			(V4L2_CID_CAMERA_CLASS_BASE + 1003)
> +#define ISC_CID_R_OFFSET		(V4L2_CID_CAMERA_CLASS_BASE + 1004)
> +#define ISC_CID_B_OFFSET		(V4L2_CID_CAMERA_CLASS_BASE + 1005)
> +#define ISC_CID_GR_OFFSET		(V4L2_CID_CAMERA_CLASS_BASE + 1006)
> +#define ISC_CID_GB_OFFSET		(V4L2_CID_CAMERA_CLASS_BASE + 1007)

1) You need to document what these controls actually do in this header.

2) You need to claim a range for these controls in v4l2-controls.h.
   Search for "The base for" in that header to see how it is done.
   Currently private controls are all for USER_BASE. I think that should
   be done here as well since these controls all relate to the white balance
   control. In fact, control clusters whose controls are in different control
   classes are a bit weird.

Regards,

	Hans

> +
>  #endif
>