Re: [PATCH v5] media: imx208: Add imx208 camera sensor driver

[Tomasz Figa <tfiga@chromium.org>]

[+Laurent and Sylwester]

On Wed, Aug 8, 2018 at 4:08 PM Ping-chung Chen
<ping-chung.chen@intel.com> wrote:
[snip]
> +
> +/* Digital gain control */
> +#define IMX208_REG_GR_DIGITAL_GAIN     0x020e
> +#define IMX208_REG_R_DIGITAL_GAIN      0x0210
> +#define IMX208_REG_B_DIGITAL_GAIN      0x0212
> +#define IMX208_REG_GB_DIGITAL_GAIN     0x0214
> +#define IMX208_DGTL_GAIN_MIN           0
> +#define IMX208_DGTL_GAIN_MAX           4096
> +#define IMX208_DGTL_GAIN_DEFAULT       0x100
> +#define IMX208_DGTL_GAIN_STEP           1
> +
[snip]
> +/* Initialize control handlers */
> +static int imx208_init_controls(struct imx208 *imx208)
> +{
[snip]
> +       v4l2_ctrl_new_std(ctrl_hdlr, &imx208_ctrl_ops, V4L2_CID_DIGITAL_GAIN,
> +                         IMX208_DGTL_GAIN_MIN, IMX208_DGTL_GAIN_MAX,
> +                         IMX208_DGTL_GAIN_STEP,
> +                         IMX208_DGTL_GAIN_DEFAULT);

We have a problem here. The sensor supports only a discrete range of
values here - {1, 2, 4, 8, 16} (multiplied by 256, since the value is
fixed point). This makes it possible for the userspace to set values
that are not allowed by the sensor specification and also leaves no
way to enumerate the supported values.

I can see two solutions here:

1) Define the control range from 0 to 4 and treat it as an exponent of
2, so that the value for the sensor becomes (1 << val) * 256.
(Suggested by Sakari offline.)

This approach has the problem of losing the original unit (and scale)
of the value.

2) Use an integer menu control, which reports only the supported
discrete values - {1, 2, 4, 8, 16}.

With this approach, userspace can enumerate the real gain values, but
we would either need to introduce a new control (e.g.
V4L2_CID_DIGITAL_GAIN_DISCRETE) or abuse the specification and
register V4L2_CID_DIGITAL_GAIN as an integer menu.

Any opinions or better ideas?

Best regards,
Tomasz