| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <62785fbc-74d9-1924-ba85-ce3f06b5b047@gmail.com>
Date: Wed, 25 Jan 2017 11:10:41 -0800
From: Steve Longerbeam <slongerbeam@...il.com>
To: Hans Verkuil <hverkuil@...all.nl>, robh+dt@...nel.org,
mark.rutland@....com, shawnguo@...nel.org, kernel@...gutronix.de,
fabio.estevam@....com, linux@...linux.org.uk, mchehab@...nel.org,
nick@...anahar.org, markus.heiser@...marIT.de,
p.zabel@...gutronix.de, laurent.pinchart+renesas@...asonboard.com,
bparrot@...com, geert@...ux-m68k.org, arnd@...db.de,
sudipm.mukherjee@...il.com, minghsiu.tsai@...iatek.com,
tiffany.lin@...iatek.com, jean-christophe.trotin@...com,
horms+renesas@...ge.net.au, niklas.soderlund+renesas@...natech.se,
robert.jarzmik@...e.fr, songjun.wu@...rochip.com,
andrew-ct.chen@...iatek.com, gregkh@...uxfoundation.org
Cc: devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, linux-media@...r.kernel.org,
devel@...verdev.osuosl.org,
Steve Longerbeam <steve_longerbeam@...tor.com>
Subject: Re: [PATCH v3 22/24] media: imx: Add MIPI CSI-2 OV5640 sensor subdev
driver
On 01/20/2017 06:48 AM, Hans Verkuil wrote:
> On 01/07/2017 03:11 AM, Steve Longerbeam wrote:
>> +
>> + /* cached control settings */
>> + int ctrl_cache[OV5640_MAX_CONTROLS];
> This is just duplicating the cached value in the control framework. I think this can be dropped.
done, see below.
>
>> +
>> +static struct ov5640_control ov5640_ctrls[] = {
>> + {
>> + .set = ov5640_set_agc,
>> + .ctrl = {
>> + .id = V4L2_CID_AUTOGAIN,
>> + .name = "Auto Gain/Exposure Control",
>> + .minimum = 0,
>> + .maximum = 1,
>> + .step = 1,
>> + .default_value = 1,
>> + .type = V4L2_CTRL_TYPE_BOOLEAN,
>> + },
>> + }, {
>> + .set = ov5640_set_exposure,
>> + .ctrl = {
>> + .id = V4L2_CID_EXPOSURE,
>> + .name = "Exposure",
>> + .minimum = 0,
>> + .maximum = 65535,
>> + .step = 1,
>> + .default_value = 0,
>> + .type = V4L2_CTRL_TYPE_INTEGER,
>> + },
>> + }, {
>> + .set = ov5640_set_gain,
>> + .ctrl = {
>> + .id = V4L2_CID_GAIN,
>> + .name = "Gain",
>> + .minimum = 0,
>> + .maximum = 1023,
>> + .step = 1,
>> + .default_value = 0,
>> + .type = V4L2_CTRL_TYPE_INTEGER,
>> + },
>> + }, {
>> + .set = ov5640_set_hue,
>> + .ctrl = {
>> + .id = V4L2_CID_HUE,
>> + .name = "Hue",
>> + .minimum = 0,
>> + .maximum = 359,
>> + .step = 1,
>> + .default_value = 0,
>> + .type = V4L2_CTRL_TYPE_INTEGER,
>> + },
>> + }, {
>> + .set = ov5640_set_contrast,
>> + .ctrl = {
>> + .id = V4L2_CID_CONTRAST,
>> + .name = "Contrast",
>> + .minimum = 0,
>> + .maximum = 255,
>> + .step = 1,
>> + .default_value = 0,
>> + .type = V4L2_CTRL_TYPE_INTEGER,
>> + },
>> + }, {
>> + .set = ov5640_set_saturation,
>> + .ctrl = {
>> + .id = V4L2_CID_SATURATION,
>> + .name = "Saturation",
>> + .minimum = 0,
>> + .maximum = 255,
>> + .step = 1,
>> + .default_value = 64,
>> + .type = V4L2_CTRL_TYPE_INTEGER,
>> + },
>> + }, {
>> + .set = ov5640_set_awb,
>> + .ctrl = {
>> + .id = V4L2_CID_AUTO_WHITE_BALANCE,
>> + .name = "Auto White Balance",
>> + .minimum = 0,
>> + .maximum = 1,
>> + .step = 1,
>> + .default_value = 1,
>> + .type = V4L2_CTRL_TYPE_BOOLEAN,
>> + },
>> + }, {
>> + .set = ov5640_set_red_balance,
>> + .ctrl = {
>> + .id = V4L2_CID_RED_BALANCE,
>> + .name = "Red Balance",
>> + .minimum = 0,
>> + .maximum = 4095,
>> + .step = 1,
>> + .default_value = 0,
>> + .type = V4L2_CTRL_TYPE_INTEGER,
>> + },
>> + }, {
>> + .set = ov5640_set_blue_balance,
>> + .ctrl = {
>> + .id = V4L2_CID_BLUE_BALANCE,
>> + .name = "Blue Balance",
>> + .minimum = 0,
>> + .maximum = 4095,
>> + .step = 1,
>> + .default_value = 0,
>> + .type = V4L2_CTRL_TYPE_INTEGER,
>> + },
>> + },
>> +};
>> +#define OV5640_NUM_CONTROLS ARRAY_SIZE(ov5640_ctrls)
> This should use v4l2_ctrl_new_std() instead of this array.
> Just put a switch on ctrl->id in s_ctrl, and each case calls the corresponding
> set function.
In this case, because there are lots of controls, my preference is to use
table lookup rather than a large switch statement. However I did remove
.name and .type from the table entries, leaving only .def, .min, .max, .step
as required to pass to v4l2_ctrl_new_std(). Also converted to 'struct
v4l2_ctrl_config'
in the table.
>
>> +
>> +static int ov5640_restore_ctrls(struct ov5640_dev *sensor)
>> +{
>> + struct ov5640_control *c;
>> + int i;
>> +
>> + for (i = 0; i < OV5640_NUM_CONTROLS; i++) {
>> + c = &ov5640_ctrls[i];
>> + c->set(sensor, sensor->ctrl_cache[i]);
>> + }
>> +
>> + return 0;
>> +}
> This does the same as v4l2_ctrl_handler_setup() if I understand the code correctly.
yes thanks. I remember looking at this and thinking
v4l2_ctrl_handler_setup()
was setting up the default values rather than the current values, but
after a
re-read it does look to be restoring the current values, which is
exactly what
is needed here.
>> +
>> +static int ov5640_init_controls(struct ov5640_dev *sensor)
>> +{
>> + struct ov5640_control *c;
>> + int i;
>> +
>> + v4l2_ctrl_handler_init(&sensor->ctrl_hdl, OV5640_NUM_CONTROLS);
>> +
>> + for (i = 0; i < OV5640_NUM_CONTROLS; i++) {
>> + c = &ov5640_ctrls[i];
>> +
>> + v4l2_ctrl_new_std(&sensor->ctrl_hdl, &ov5640_ctrl_ops,
>> + c->ctrl.id, c->ctrl.minimum, c->ctrl.maximum,
>> + c->ctrl.step, c->ctrl.default_value);
>> + }
> As mentioned, just drop the ov5640_ctrls array and call v4l2_ctr_new_std for each
> control you're adding.
if really pressed I can be persuaded to use a switch statement and call
v4l2_ctrl_new_std() multiple times, but I don't any problem with using
a table.
>> +
>> +module_i2c_driver(ov5640_i2c_driver);
>> +
>> +MODULE_AUTHOR("Freescale Semiconductor, Inc.");
>> +MODULE_AUTHOR("Steve Longerbeam <steve_longerbeam@...tor.com>");
>> +MODULE_DESCRIPTION("OV5640 MIPI Camera Subdev Driver");
>> +MODULE_LICENSE("GPL");
>> +MODULE_VERSION("1.0");
>>
> Same comments apply to the next patch, so I won't repeat them.
done, I've made the same mods to the ov5642 subdev.
Steve
Powered by blists - more mailing lists